primitives: remove Ord from AbsLockTime and RelLockTime

[151henry151]

This PR implements #979 by removing Ord and PartialOrd from AbsLockTime and RelLockTime. Those impls compared height- and time-based locktimes as raw consensus u32 values, which is not a meaningful total order. The wrapper types exist because Miniscript locktimes cannot be zero; ordering was incidental.

Policy types still need a total order, so we replace derive(Ord) on concrete and semantic Policy with explicit variant-name ordering and consensus-u32 comparison of matching After/Older nodes via cmp_by_consensus on the locktime types. Terminal::cmp uses the same method, and Miniscript::Ord picks that up through node.cmp. Policy and Miniscript ordering may change; that ordering was never a stable public contract.

In concatenate_rev, same-unit locktimes merge via max (bitcoin partial_cmp); mixed height/time on one path yields Witness::Impossible, so plan building fails or picks another branch. Composite satisfiers may still expose both locktime kinds. regression_895 expects into_plan_mall to fail on the height-vs-time case while into_plan still succeeds with the time-based locktime.

Stacked on PR #980 (2026-06/mallsat) — merge #980 first. Breaking changes; changelog and version bump left to maintainers.

Fixes #979

Tested with:
cargo test --all-features — passed
just fmt-check — passed
just sane — passed

[apoelstra]

concept ACK. Will wait for #980 to merge before reviewing in detail.

later merges can overwrite an earlier None with a Some, so a height/time conflict may not appear in the final Plan timelock fields.

lmao. This timelock-mixing thing (and regression_895 which explores it) really is the gift that keeps on giving.

I think instead we should return None for the whole satisfaction in this case. Which means changing the return value for concatenate_rev and causing a big diff. But I think it's the right thing to do.

If you think it's too big a diff, we can just accept this PR as-is, update the regression test with even more explanation/history, and file a bug to improve it later.

Eventually I kinda think we should do two parallel satisfactions: one with height-based timelocks and one with time-based ones. This will be a little bit tricky to implement since there are four possibilities (time vs height for both relative and absolute locktimes) and we don't want to waste time or space in the case of a "normal descriptor" that has only one kind (or no kinds) of timelocks.

Branch selection is unaffected (minimum/minimum_mall choose by witness cost, not timelocks), and the scenario requires impossible satisfier conditions anyway.

I'm don't think it's actually impossible. The docs for Satisfier::check_older and check_after have a warning saying "don't accept both time-based and height-based timelocks!" but (a) nobody is going to read this, and (b) it's hard to avoid doing this, since you can just take a tuple of two satisfiers and then it'll accept the union of the two.

I'd like to just remove this warning, or at least weaken it.

[151henry151]

Pushed 2c0d98b: mixed height/time in concatenate_rev now yields Witness::Impossible, with max_same_kind on the locktime types and updated regression_895 / satisfier docs. PR description updated to match.

[apoelstra]

In 1140b69:

I think we should sort locktimes numerically by their u32 representation. I could be convinced either way...but the numeric comparison is cheaper to implement and I suspect it matches user intuition a bit better.

In this commit we should also update the ord impls for Miniscript to use the new locktime comparison function.

[151henry151]

Addressed in 66b99c2: same-kind locktime ordering in Policy::Ord and Terminal::cmp now uses consensus u32 instead of display strings. Miniscript::Ord already delegates to Terminal::cmp via self.node.cmp, so no separate change there.

[apoelstra]

Please don't add commits which change other commits. Use git-rebase to edit the original commits in-place. This helps us review code commit by commit.

[151henry151]

Please don't add commits which change other commits. Use git-rebase to edit the original commits in-place. This helps us review code commit by commit.

Rebased so that review feedback is folded into the original commits, not added as fix-ups on top. Each commit builds cleanly.

[apoelstra]

In bd68e35:

It would be a better abstraction to have locktime_consensus_cmp take a locktime and do the conversion itself. You will need two methods, one for AbsLockTime and one for RelLockTime. Also, it should be a method (still (pub(crate))) rather than a standalone function. I suggest calling it cmp_by_consensus or cmp_by_consensus_encoding.

[151henry151]

In bd68e35:

It would be a better abstraction to have locktime_consensus_cmp take a locktime and do the conversion itself. You will need two methods, one for AbsLockTime and one for RelLockTime. Also, it should be a method (still (pub(crate))) rather than a standalone function. I suggest calling it cmp_by_consensus or cmp_by_consensus_encoding.

Done — cmp_by_consensus on AbsLockTime and RelLockTime, folded into the original commit.

I was about to step out the door to go to my wedding; we're leaving in two hours! I'll be gone about a week, no service, camping out in the wilderness for the celebration. So if it seems like I drop off the map, don't give up on me, I'll be back in seven days. =)

[apoelstra]

Nice! Almost there. In c170d55 can you update the doccomments (where you removed "which implements Ord" to say "that cannot be zero"?

The nonzero invariant is ultimately why we have these wrapper types. The Ord thing was just a bonus.

[151henry151]

Updated the struct docs in c170d55 to say "that cannot be zero."

[trevarj]

Henry Romp ***@***.***> writes:

I was about to step out the door to go to my wedding; we're leaving in two hours! I'll be gone about a week, no service, camping out in the wilderness for the celebration. So if it seems like I drop off the map, don't give up on me, I'll be back in seven days. =)

LOL what the heck...congratulations and enjoy!

[151henry151]

While updating the PR description I noticed just fmt-check failing on long cmp_by_consensus lines in display.rs. Ran cargo fmt and folded the result into the Terminal::cmp commit.

[apoelstra]

queued up an ack/merge. Enjoy your wedding! If CI passes it'll be merged when you're back, or else it'll wait for you.

[apoelstra]

ACK 476dac5; successfully ran local tests

[apoelstra]

Lol, dammit, I didn't realize that this built on #980. I did not intend to merge #980 since it hasn't been reviewed by anyone but its author (me).

Interesting process failure here... my reviewing system saw that the two commits from 980 were "already reviewed" and skipped over them, so when I ran local-ci 982 next it only showed me the new commits.