primitives: remove Ord from AbsLockTime and RelLockTime by 151henry151 · Pull Request #982 · rust-bitcoin/rust-miniscript

151henry151 · 2026-06-07T19:43:02Z

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

This is supposed to be testing mixed timelocks, but it uses descriptors that are invalid for several unrelated reasons. These will be enforced in a later PR, so fix the issues now. Specifically: * It is not allowed to use the same `unavailable_key` multiple times * The rightmost branch must not be sigless; the timelock needs to have a pubkey and'ed with it. Otherwise the whole script is malleable. * Similarly the "cheap dissatisfaction" of v:after(144),pk({})) must not be sigless. We turn it into v:after(144),and_v(v:pk({})),pk({})) which requires a signature even to be dissatisfied. * Also, the giant comments don't accurately reflect what's going on in this test. They claim that post-rust-bitcoin#895, we will refuse to create a satisfaction if we'd be forced into mixing timelocks. But we don't. We actually do the same wrong thing as before, just for different reasons. The resulting test now passes even when all `ValidationParams` are turned on, though in this PR we don't confirm this. HOWEVER, despite the test working with a non-malleable descriptor now, the satisfaction that this test produces is malleable. For this I have filed rust-bitcoin#976, which will be fixed in the next commit.

…ions ...unless the user has explicitly requested a malleable satisfaction. Fixes rust-bitcoin#976

apoelstra · 2026-06-08T13:24:12Z

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 · 2026-06-08T22:56:11Z

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 · 2026-06-09T22:26:37Z

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 · 2026-06-10T03:42:20Z

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 · 2026-06-12T22:48:48Z

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 · 2026-06-12T23:44:09Z

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 · 2026-06-13T15:19:50Z

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.

Replace derive(Ord) with variant-name ordering and same-kind locktime comparison via cmp_by_consensus on the locktime types, and add regression tests.

151henry151 · 2026-06-13T15:46:04Z

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 · 2026-06-13T16:15:38Z

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 · 2026-06-13T16:47:23Z

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

trevarj · 2026-06-13T16:48:36Z

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!

Compare After and Older nodes via cmp_by_consensus rather than locktime Ord, and add a Miniscript ordering regression test.

Mixed height/time merges now invalidate the whole satisfaction path instead of leaving a valid stack with None timelock metadata. Add max helpers on locktime types and update satisfier docs.

Drop the consensus-u32 ordering that incorrectly compared height-based and time-based locktimes against each other.

151henry151 · 2026-06-13T17:05:14Z

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 · 2026-06-13T19:36:46Z

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 · 2026-06-14T12:54:12Z

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.

apoelstra added 2 commits June 5, 2026 18:12

miniscript: don't use malleable satisfier, even for or_i dissatisfact…

99501a5

…ions ...unless the user has explicitly requested a malleable satisfaction. Fixes rust-bitcoin#976

151henry151 mentioned this pull request Jun 7, 2026

Can we drop Ord on AbsLockTime and RelLockTime? #979

Closed

trevarj reviewed Jun 8, 2026

View reviewed changes

Comment thread src/miniscript/satisfy/mod.rs Outdated

Comment thread src/miniscript/satisfy/mod.rs

trevarj reviewed Jun 9, 2026

View reviewed changes

Comment thread src/primitives/absolute_locktime.rs Outdated

151henry151 force-pushed the 2026-06/drop-locktime-ord branch from 66b99c2 to 5cd8ab8 Compare June 12, 2026 23:40

Add manual Ord impls for concrete and semantic Policy.

f168ca0

Replace derive(Ord) with variant-name ordering and same-kind locktime comparison via cmp_by_consensus on the locktime types, and add regression tests.

151henry151 force-pushed the 2026-06/drop-locktime-ord branch from 5cd8ab8 to c170d55 Compare June 13, 2026 15:43

151henry151 force-pushed the 2026-06/drop-locktime-ord branch from c170d55 to 67f2b2d Compare June 13, 2026 16:45

151henry151 added 3 commits June 13, 2026 12:53

Sort same-kind locktimes by consensus u32 in Terminal::cmp.

46b2812

Compare After and Older nodes via cmp_by_consensus rather than locktime Ord, and add a Miniscript ordering regression test.

Mark timelock kind conflicts as Witness::Impossible in concatenate_rev.

013ad7d

Mixed height/time merges now invalidate the whole satisfaction path instead of leaving a valid stack with None timelock metadata. Add max helpers on locktime types and update satisfier docs.

Remove Ord and PartialOrd from AbsLockTime and RelLockTime.

476dac5

Drop the consensus-u32 ordering that incorrectly compared height-based and time-based locktimes against each other.

151henry151 force-pushed the 2026-06/drop-locktime-ord branch from 67f2b2d to 476dac5 Compare June 13, 2026 16:55

apoelstra approved these changes Jun 14, 2026

View reviewed changes

apoelstra merged commit 460d5f1 into rust-bitcoin:master Jun 14, 2026
14 checks passed

apoelstra mentioned this pull request Jun 14, 2026

miniscript: don't use malleable satisfier, even for or_i dissatisfactions #980

Merged

Uh oh!

Conversation

151henry151 commented Jun 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra commented Jun 8, 2026

Uh oh!

Uh oh!

Uh oh!

151henry151 commented Jun 8, 2026

Uh oh!

Uh oh!

apoelstra commented Jun 9, 2026

Uh oh!

151henry151 commented Jun 10, 2026

Uh oh!

apoelstra commented Jun 12, 2026

Uh oh!

151henry151 commented Jun 12, 2026

Uh oh!

apoelstra commented Jun 13, 2026

Uh oh!

151henry151 commented Jun 13, 2026

Uh oh!

apoelstra commented Jun 13, 2026

Uh oh!

151henry151 commented Jun 13, 2026

Uh oh!

trevarj commented Jun 13, 2026 via email

Uh oh!

151henry151 commented Jun 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra commented Jun 13, 2026

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

apoelstra commented Jun 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

151henry151 commented Jun 7, 2026 •

edited

Loading

151henry151 commented Jun 13, 2026 •

edited

Loading