#36028 miniscript: make typing rule match BIP-379 exactly
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36028 miniscript: make typing rule match BIP-379 exactly | Waiting on author | 0 | Strong | 4 | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Aligns the typing rules in Bitcoin Core's Miniscript implementation (`src/script/miniscript.cpp`) with the exact text of BIP-379. Specifically, it simplifies type properties for `WRAP_D`, `OR_B`, and `THRESH` so that the 'e' (uniquely dissatisfiable) property matches the written specification even when the expression is already malleable.
Problem: Bitcoin Core's Miniscript type computation diverged in edge cases from the BIP-379 text for malleable expressions. While mathematically equivalent for validity and malleability checking, this divergence caused discrepancies when comparing against proposed BIP-379 test vectors.
Reviewability
Paused · Waiting on author
Open objections from fametrano, guggero with no author reply for 30 days; review would repeat what has been asked.
Waiting on (from the thread read):
- author: author response to alternative approach from bips#2267 and test vector results 2026-10-02
- author: author response to suggestions regarding alternative approach from bips#2267 and test vector results 2026-10-02
Agreement
Strong
substantive support, no open objection (guggero)
Thread notes: Added suggestions from guggero and fametrano concerning bips#2267 and test vector results indicating the typing rules may be superseded by dropping properties on malleable expressions. / Added suggestions from guggero and fametrano reflecting subsequent upstream discussions in BIP-379 and rust-miniscript about dropping s/f/e on malleable expressions.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| guggero (contributor) | suggestion, approach | State that the two deleted lines were no-ops to clarify that the change is behavior-preserving | open | answered | no | 2026-08-22: 'These two deleted lines were no-ops in the first place, which may be worth stating for reviewers who want to convince themselves the change is behavior-preserving' |
| guggero (contributor) | suggestion, approach | Point to bitcoin/bips#2267 to clarify that property descriptions only bind for expressions meeting malleability requirements, preventing the rule from appearing to contradict BIP-379's description of 'e' | open | answered | no | 2026-08-22: 'One thing you may want to point at from here: after this PR, Core assigns "e" to expressions whose dissatisfaction is provably not unique, which contradicts the BIP's own description of the property' |
| guggero (contributor) | suggestion, approach | Adopt the new approach from bitcoin/bips#2267 and rust-miniscript#1037 (dropping s/f/e on malleable expressions), as implemented in guggero's branch | open | answered | no | 2026-08-26: 'Feel free to take whatever you find useful from this commit (assuming this is the direction that finds agreement): https://github.com/guggero/bitcoin/commit/576eb5f3252bd4159c302ae2bfe70b6ae71f6e33' |
| fametrano (contributor) | suggestion, approach | Consider that BIP-379 test vectors in bips#2240 only need dropping s/f/e on malleable expressions (making this PR's rule changes redundant), while this PR's rules alone fail 18,188 vector rows | open | answered | no | 2026-10-02: 'So the vectors only need that drop (as in guggero's 576eb5f), with or without the rule changes here.' |
Support:
- guggero (contributor) (Concept ACK, 2026-08-22) [read: src/script/miniscript.cpp]: I agree on the approach and the Go implementation in btcsuite/btcd#2568 and the [test vector BIP PR](https://github.com/bitcoin/bips/pull/2240) are now updated. Assuming the [`rust-miniscript` PR](https://github.com/rust-bitcoin/rust-miniscript/pull/1032) is also accepted and merged, the three imple
Participants:
- guggero (contributor, support, 4 statements 2026-08-22 to 2026-08-26): Concept ACK; provided analysis and later suggested alternative approach from bips#2267 and rust-miniscript#1037
- fametrano (contributor, neutral, 1 statements 2026-10-02 to 2026-10-02): tested BIP-379 vectors, observing that they only require dropping s/f/e on malleable expressions rather than these rule changes
Dependencies
Enables:
Files
0 lines under test/bench/ci.
- src/script/miniscript.cpp +4/-6
Card
The short text the ranking pass compares across PRs.
Sipa aligns Bitcoin Core's Miniscript typing rules with the exact wording of BIP-379 for expressions involving the uniquely dissatisfiable property. The change does not alter descriptor acceptance or spendability in practice, but aligns intermediate type evaluation with proposed BIP-379 test vectors. Guggero provided a Concept ACK while noting ongoing spec clarifications in BIP PR 2267 and rust-miniscript. The PR is paused in draft status waiting for agreement on the upstream BIP test vector PR.
Data
thread read through 2026-10-02 (5 statements), head 2631ece7b5
labels: Descriptors · draft
Categories
Wallet
P3 · cleanup (ranking pass: Moved down: explicitly equivalent in outcome; value is cross-implementation spec alignment only.)
Aligns Miniscript typing rules with the BIP-379 specification to resolve discrepancies against proposed test vectors. While the author notes the typing rule was 'mathematically equivalent for validity and malleability checking', fixing the edge-case divergence ensures cross-implementation consistency with BIP-379 test vectors.
Membership: Touches Miniscript typing rules in `src/script/miniscript.cpp`, which forms the descriptor parsing and analysis subsystem covered under descriptor management, matching the Descriptors label.
Factors: security/stability 0, bug 0, performance 0, user value 0, leverage 1