Repository navigation
Backport wallet policy fixes to 13.x - #1042
Merged
apoelstra merged 17 commits intoOct 4, 2026
Merged
Conversation
BIP-388 key information items are bare KEY expressions and the derivation comes from the template's key placeholders, but the translator returned the key information item verbatim, so wpkh(@0/**) materialized as wpkh(xpub...) instead of wpkh(xpub.../<0;1>/*). - take the derivation path and wildcard from the placeholder when materializing a descriptor key - reject a key information item that is not an extended key, since there is nothing to append a derivation to Fixes rust-bitcoin#1014
BIP-388 says a placeholder @i must never appear for the first time before an occurrence of @j for some j < i, and that reuses of one placeholder must have pairwise disjoint paths. The check compared only adjacent placeholders, so it rejected valid templates: wsh(multi(2,@0/**,@1/**,@1/<2;3>/*)) failed because @1 followed @1, and the @0 reuse variant failed because @0 followed @1.
key_info was a Vec<DescriptorPublicKey>, which can hold a plain public key or a key carrying its own derivation, neither of which BIP-388 allows. It also held one entry per key occurrence, so a descriptor reusing one key with disjoint paths, such as sh(multi(1,K/<0;1>/*,K/<2;3>/*)), produced the template sh(multi(1,@0/**,@1/<2;3>/*)) and a key vector holding the same xpub twice, rather than sh(multi(1,@0/**,@0/<2;3>/*)) with one entry. - add `KeyInfo`, an extended public key plus optional origin. - take `Vec<KeyInfo>` in `set_key_info`, matching how the crate takes owned collections elsewhere, and add a `key_info` getter, now that the items cannot be malformed.
…istinct BIP-388 requires the deserialized keys of the key information vector to be pairwise distinct. This was not checked, so one key could fill two placeholders, turning wsh(multi(2,K,K)) into a 2-of-2 that a single keyholder satisfies alone.
The template parser required a key placeholder to be followed by "/**" or "/<NUM;NUM>/*", but from_descriptor built key expressions straight from descriptor keys and never checked their shape, so the checked constructor emitted templates that WalletPolicy::from_str rejects: wpkh(xpub.../0/*) became wpkh(@0/0/*) and a bare wpkh(xpub...) became wpkh(@0).
trevarj
marked this pull request as ready for review
September 4, 2026 14:11
Contributor
Author
|
@apoelstra this backport would help us pin to a rust-miniscript here wizardsardine/bhwi#68 All just cherry picked from master like in #1009 |
Member
|
Sorry for letting this slip. Will review now. |
Member
|
In c66df3f: This adds an error variant to On master we should make this |
Contributor
Author
|
@apoelstra thanks for the review. I added a54c407 which is using an existing variant instead. PR against master for non_exhaustive is up |
trevarj
force-pushed
the
13.x/wallet-policy-followups
branch
from
September 30, 2026 04:56
9407cc0 to
a54c407
Compare
Member
|
Oops, I was wrong. I'll drop your fixup commit. You can see from the public docs that none of the wallet policy stuff, errors or otherwise, have been published. These things were backported in #1009 but not released. So we are free to break them. In d71c0bc: This disables a test. Can you squash it into the previous commit so that all commits pass tests? |
apoelstra
force-pushed
the
13.x/wallet-policy-followups
branch
from
September 30, 2026 12:25
a54c407 to
5525760
Compare
Member
|
In 5525760: Can you forward-port this to master? |
Member
|
Done reviewing 5525760. Only blocker is that that one test commit needs to be squashed. Went over each commit, checking the range-diff against the original PR. (I used GLM-5.3-Flash to determine the right range-diff commands; in future I would prefer cherry-pick trailers or at least a "Backport of: #1031" trailer if possible.) Checked each one by hand for breaking API changes. Also asked the LLM to review but I don't think it did much other than looking at range-diffs, which were all clean. Its one note was that, as I said, the final commit fixes a bug which is currently live on master. |
… grammar - parse the placeholder grammar directly: "@N/**" or "@N/<NUM;NUM>/*" with two distinct canonical NUMs, sharing the digits-only, no leading zeros rule between the key index and the NUMs via parse_canonical_num - collapse only a whole "/<0;1>/*" derivation to "/**" on display, instead of a global replace
…criptor - drop from_descriptor_unchecked and validate in from_descriptor - add n_keys(), which is the number of key placeholders in the template
BIP-388 requires at least one key placeholder and the corresponding key. - reject a template with no key placeholder - reject Descriptor::Bare in validate
- use the concrete bitcoin hash types, exactly as DescriptorPublicKey does, so garbage hex fails inside the miniscript parser
`Miniscript::translate_pk` and `Descriptor::translate_pk`, along with the concrete and semantic policy equivalents, drove their reconstruction from a right-to-left post-order iterator. That existed only so a node could be rebuilt as `Terminal::AndB(stack.pop().unwrap(), stack.pop().unwrap())`. - iterate with `post_order_iter` everywhere `rtl_post_order_iter` was used - add a crate-private `StackExt` trait on `Vec<T>`; `pop2`/`pop3` pop the top two or three elements and pass them, left to right, to a constructor or closure - `pop_n` drains the top `n` elements in order, for the `Vec`-shaped policy nodes, and `pop_thresh` rebuilds a threshold with the same `k` Fixes rust-bitcoin#1022
- add an infallible constructor for the single-path case
- the fields and validate() are now private to the submodule, so every construction path validates - the TryFrom<&str> template branch goes through a private from_template instead of a struct literal
With `translate_pk` now walking the descriptor in left-to-right order, `from_descriptor` no longer needs a separate `iter_pk` pass to number the keys in textual order. - drop the `iter_pk` pre-pass in `from_descriptor` - number each distinct key at its first occurrence in the translator
Every construction and mutation of a `WalletPolicy` now runs through the same checker. - replace `validate` with a `check_policy` fn that the constructors call before building the struct - `set_key_info` drops its own count check, rejects an empty argument and revalidates through `check_policy` before assigning
- keep validated state construction and mutation in the private policy module - move translation and public API implementations to the parent module
- preserve textual key order for stateful translators - cover wallet policies with distinct internal and script keys Assisted-by: OpenAI Codex GPT-5.6 Sol
trevarj
force-pushed
the
13.x/wallet-policy-followups
branch
from
September 30, 2026 13:41
5525760 to
12c1ebc
Compare
Contributor
Author
|
@apoelstra ok, force pushed after the squashing of the test commit. Forward port is at #1055 |
apoelstra
force-pushed
the
13.x/wallet-policy-followups
branch
from
October 1, 2026 11:50
12c1ebc to
c64d2c4
Compare
apoelstra
added a commit
that referenced
this pull request
Oct 5, 2026
76fe10f fix(descriptor): translate Taproot internal key first (Trevor Arjeski) Pull request description: Translate the Taproot internal key before its script tree so stateful translators preserve textual key order. This fixes wallet-policy conversion for descriptors with distinct internal and script keys. Forward-port of 5525760 from #1042. ACKs for top commit: apoelstra: ACK 76fe10f; successfully ran local tests Tree-SHA512: 812472aa2fed2084dc2929ca36d767bacb9ab080f9a98049b35ce92117faf421866afd6c553b0202ddee75754707a4a08f7b824bcef43555c2d8179aa83983f6
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
2 participants
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.
Backports BIP-388 wallet policy fixes and required translation-order fixes to release-13.x.