miniscript: don't use malleable satisfier, even for or_i dissatisfactions#980
Merged
Merged
Conversation
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
force-pushed
the
2026-06/mallsat
branch
from
7ea3555 to
99501a5
Compare
Member
Author
|
On 99501a5 successfully ran local tests |
This was referenced Jun 7, 2026
apoelstra
added a commit
that referenced
this pull request
Jun 14, 2026
476dac5 Remove Ord and PartialOrd from AbsLockTime and RelLockTime. (Henry Romp) 013ad7d Mark timelock kind conflicts as Witness::Impossible in concatenate_rev. (Henry Romp) 46b2812 Sort same-kind locktimes by consensus u32 in Terminal::cmp. (Henry Romp) f168ca0 Add manual Ord impls for concrete and semantic Policy. (Henry Romp) 99501a5 miniscript: don't use malleable satisfier, even for or_i dissatisfactions (Andrew Poelstra) cfb93eb descriptor: fix up regression_895 test (Andrew Poelstra) Pull request description: 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 ACKs for top commit: apoelstra: ACK 476dac5; successfully ran local tests Tree-SHA512: b0aec263d46a1219c7089a3bd89026da661deacf2f9fafe66238716a4eeb5bdddec2a75b7fc8f21f59900c299c1feee765ed188eb57cab384936e3f613ce841d
Member
Author
|
@tcharding I accidentally merged this by merging #982 which builds on top of it. Would you mind doing a cursory post-merge review of it? I don't think it makes sense to do a "deep" review since this stuff is way in the weeds, and no user has ever noticed the bugs it fixes/describes. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Significantly cleans up the
regression_895unit test, which I need to do to support the next PR in theValidationParamsseries. Along the way I found multiple issues (#976 and #979).Fixes #976