Wallet Policy (BIP-0388) corrections - #1019
Open
trevarj wants to merge 8 commits into
Open
Conversation
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).
… 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
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.
After using a powerful (Claude Fable 5) model to review the
wallet_policy module and check it for correctness and API suggestions,
I went back and forth with it over a few iterations to make a bunch of
fixes. These will be better to get in before a release has the
WalletPolicy stuff since there are some breaking API changes.
Commit by commit is the way to review. Also wouldn't mind more LLM
crosschecking, even though I had it review the changes multiple times
after making some tweaks myself.