descriptor: support hash terminals in WalletPolicy translator#935
Open
bg002h wants to merge 1 commit intorust-bitcoin:masterfrom
Open
descriptor: support hash terminals in WalletPolicy translator#935bg002h wants to merge 1 commit intorust-bitcoin:masterfrom
bg002h wants to merge 1 commit intorust-bitcoin:masterfrom
Conversation
This patch was authored by Claude Opus 4.7 under direction from a human
(the project author of a BIP 388-aware backup format that uses
WalletPolicy as its policy type — see "Context" below). Every
algorithmic decision, the test matrix, and the commit text was produced
by the model; the human reviewed and directed the work but did not
hand-write the diff. The patch was tested locally as described in the
"Test coverage" section before submission. Reviewing it with the same
scrutiny you'd apply to any AI-generated contribution is appropriate.
----
The WalletPolicyTranslator used translate_hash_fail! in both directions,
which made WalletPolicy::into_descriptor() and from_descriptor() panic
on any descriptor containing a sha256/hash256/ripemd160/hash160 terminal.
Hash terminals are perfectly valid in segwit-v0 miniscript, and BIP 388
wallet policies place no restriction on them — HTLC-style spending paths
(sig + preimage) are a primary use case.
Replace the panicking translator with manual sha256/hash256/ripemd160/
hash160 methods that bridge the impedance mismatch between the two key
types' associated hash types: KeyExpression uses `String` (storing hex
for template round-tripping), DescriptorPublicKey uses the concrete
`bitcoin::hashes::*::Hash` types.
- KeyExpression -> DescriptorPublicKey: parse the hex String into the
binary Hash via the existing FromStr impl. Errors out via a new
WalletPolicyError::TranslatorInvalidHashHex(kind, raw) variant if the
template somehow held an invalid hex string (in practice the template
parser already validates length, so the error is a defensive guard).
- DescriptorPublicKey -> KeyExpression: render the binary Hash to its
canonical lowercase-hex Display form. Infallible.
Test coverage (all six are round-trip-shaped):
- {sha256,hash256,ripemd160,hash160}_terminal_round_trips_through_translator:
one named test per hash type, each round-tripping
`wsh(and_v(v:pk(@0/**),HASH(<hex>)))` through Descriptor::from_str ->
WalletPolicy::from_str -> into_descriptor and asserting equality with
the directly-parsed Descriptor.
- all_ordered_pairs_of_distinct_hash_types_round_trip: 4·3 = 12 ordered
pairs of distinct hash types in
`wsh(and_v(v:and_v(v:pk(@0/**),A(...)),B(...)))`, each round-tripped
the same way. Guards against cross-type interference.
- translator_invalid_hash_hex_corrupted_round_trip: drives each hash
type's translator forward (Hash -> hex String, infallible) and
reverse (String -> Hash, fallible) for canonical input, then verifies
that corrupted hex surfaces TranslatorInvalidHashHex(kind, raw) with
the right Display string. Reaches the new variant via the private
translator API since the public parser validates hex up-front.
Default-feature: 152 -> 158 tests passing. All-features: 171 -> 176.
clippy --lib -D warnings clean. cargo fmt --check clean.
Context: Discovered while building a BIP 388-aware backup format on top
of `WalletPolicy` (https://github.com/bg002h/descriptor-mnemonic); the
panic blocked a corpus entry that uses sha256 inside an HTLC pattern.
The published miniscript v12 doesn't expose WalletPolicy yet, so this
is the first downstream user surfacing the bug.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Apr 30, 2026
trevarj
reviewed
Apr 30, 2026
Contributor
trevarj
left a comment
trevarj
left a comment
There was a problem hiding this comment.
Another oversight by me in the BIP388 implementation!
This does make sense and should not error when hashes are in the descriptor template.
I wonder if the tests can be made a little more concise. They seem overly verbose to test such a simple thing...perhaps I'm just getting replaced by Claude...
Contributor
|
Oh yeah...it would be nice if this is committed by you instead of Claude. Just my opinion though, I don't know how Andrew feels about that. |
Author
|
I can apologize via Claude too, and I’m sure it would do it better! That said, Claude was totally comfortable posting as if it were me…I added the human tag at the bottom by telling Claude it can’t pretend to be human. That said, as best I can tell this is a real enough issue to be passed along in a “don’t just complain, do something” fashion. |
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.
The current
WalletPolicyTranslatorusestranslate_hash_fail!in both directions (KeyExpression↔DescriptorPublicKey), so any miniscript template containing a hash terminal (sha256/hash256/ripemd160/hash160) returnsWalletPolicyErrorfromWalletPolicy::from_descriptorandWalletPolicy::into_descriptor.This PR replaces the macro with real translator implementations:
KeyExpression → DescriptorPublicKey(used byinto_descriptor): parse the hexStringinto the concretebitcoin::hashes::*::Hashtypes.DescriptorPublicKey → KeyExpression(used byfrom_descriptor[_unchecked]): render the binaryHashback to lowercase hex via the type's existingDisplayimpl.Adds a new
WalletPolicyError::TranslatorInvalidHashHex(&'static str, String)variant to surface parse failures from the inbound direction.Motivation
Found this while building md-codec, a BIP 388 wallet-policy encoder. Real-world test vectors include hash terminals, e.g.:
wsh(andor(pk(@0/**),sha256(b94d27b9...),and_v(v:pk(@1/**),older(144))))tr(@0/**,and_v(v:hash160(...),pk(@1/**)))(andsha256/hash256/ripemd160variants)These are valid BIP 388 templates (and present in the existing miniscript fragment grammar) but currently can't round-trip through
WalletPolicy.Note on routing
This was originally filed as apoelstra/rust-miniscript#1 ~3 days ago, then re-routed here on the realization that
wallet_policy/already lives atrust-bitcoin/rust-miniscript:master. apoelstra's #1 is being closed in favor of this PR. Same branch head (bg002h:fix/wallet-policy-hash-terminals), same content.—Brian (via Claude — forgive me, I'm a physician!)