Skip to content

descriptor: support hash terminals in WalletPolicy translator#1

Closed
bg002h wants to merge 1 commit intoapoelstra:2026-04/followup-895from
bg002h:fix/wallet-policy-hash-terminals
Closed

descriptor: support hash terminals in WalletPolicy translator#1
bg002h wants to merge 1 commit intoapoelstra:2026-04/followup-895from
bg002h:fix/wallet-policy-hash-terminals

Conversation

@bg002h
Copy link
Copy Markdown

@bg002h bg002h commented Apr 27, 2026

AI authorship disclosure

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.

Replaces the panicking translator with manual hash 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. New WalletPolicyError::TranslatorInvalidHashHex(kind, raw) variant for invalid hex (defensive — the template parser already validates length, so unreachable in practice through the public API).
  • DescriptorPublicKey -> KeyExpression: render the binary Hash to its canonical lowercase-hex Display form. Infallible.

Test coverage

All six new tests are round-trip-shaped:

  • {sha256,hash256,ripemd160,hash160}_terminal_round_trips_through_translator — one named test per hash type, end-to-end through Descriptor::from_str -> WalletPolicy::from_str -> into_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(...))). Guards against cross-type interference.
  • translator_invalid_hash_hex_corrupted_round_trip — drives each hash type's translator forward (Hash → hex, infallible) and reverse (hex → Hash, fallible) for canonical input, then verifies corrupted hex surfaces the new error variant. Reaches the variant via the private translator API since the public parser validates hex up-front.

Default-feature: 152 → 158 tests. 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; 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.

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>
@bg002h
Copy link
Copy Markdown
Author

bg002h commented Apr 27, 2026

Feel free to ignore this...I can see all sorts of footguns/scam opportunities that could come from seemingly secure home-rolled constructions implementing minimal security.

I appreciate your work on this and codex32, both of which are key to my attempt at a bip & implementation for doing something like bip 39 for wallet descriptors.

bg002h pushed a commit to bg002h/descriptor-mnemonic that referenced this pull request Apr 28, 2026
Final v0.2 phase. 10 design questions resolved before execution:

- G-1: ship v0.2.0 WITH the workspace [patch] block (path c per user
  direction 2026-04-28); document in Cargo.toml + README + tag message.
  v0.1.x precedent. [patch] removal becomes a v0.2.x patch-release item
  when apoelstra/rust-miniscript#1 merges.
- G-2: NEW CHANGELOG.md (Keep-a-Changelog format) covering v0.1.1 →
  v0.2.0
- G-3: NEW MIGRATION.md covering 3 breaking changes (closes 3 tracker
  FOLLOWUPS entries)
- G-4: MSRV unchanged at 1.85 (BM/Forney didn't require bump)
- G-5: dispatch one Opus 4.7 audit subagent (cargo public-api + cargo
  semver-checks)
- G-6: triage 6 v0.2-nice-to-have items: apply 4 inline (BCH style
  cluster, tap decode error naming, encoder count cast hardening,
  chunking-mode test name sweep); defer 2 (with_chunking_mode builder,
  CLI fingerprint flag)
- G-7: bump order with v0.2.json regeneration step (the generator
  field embeds the wdm-codec version, so the bump 0.2.0-dev → 0.2.0
  changes the v0.2.json SHA — must regenerate + update the lock
  constant)
- G-8: tag message scope (release summary + API delta + breaking
  changes + quality summary + dep note + BIP state)
- G-9: expected FOLLOWUPS state at tag (~9 open, ~50 resolved)
- G-10: push order — main first, then tag

Phase G is controller-driven (mirrors v0.1.0 Phase 10's closure
pattern); subagent dispatch only for the public-API audit.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bg002h pushed a commit to bg002h/descriptor-mnemonic that referenced this pull request Apr 28, 2026
Continuation of plan-review revisions (the sed pass earlier landed
the trivial Cursor API + filename fixes; this commit lands the
structural changes blocked by file-modification race):

- NEW Pre-Phase-0 section: worktree creation (Pre-0.1), miniscript v13
  API surface verification (Pre-0.2: ShInner variants, Sh constructors,
  Cursor API), and miniscript dep SHA capture (Pre-0.3) for rollback
  defense if apoelstra/rust-miniscript#1 rebases mid-implementation
- Phase 7 RENAMED to "Cargo version bump + Vectors regeneration + SHA
  pin update" — explicitly OWNS the Cargo.toml version bump (Task 7.1)
  because env!()-based GENERATOR_FAMILY needs the bump compiled in
  before regen
- Phase 8 RENAMED to "Cargo.lock refresh (only)" — no longer claims
  to own the version bump, just refreshes lock to match
- Task 7.3 renumbered to 7.2 (Verify-GENERATOR_FAMILY task removed
  since Pre-0.2 covers it)

Plan now READY-FOR-EXECUTION per holistic reviewer's verdict.
Remaining minor revisions (test-code helper cites in Tasks 1.1/2.6,
risk-register additions, Task 6.4 hostile-input cite, Task 11.7
release-notes generation) deferred — subagent will be able to
work from existing code patterns and the spec for those.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bg002h pushed a commit to bg002h/descriptor-mnemonic that referenced this pull request Apr 30, 2026
…hash-terminals

v0.10.1 needs both pending fork PRs (apoelstra/rust-miniscript#1
hash-terminals + #2 template-accessor) available via the workspace
[patch] block. The fork's `md-codec-local-stack` branch stacks both
commits; pushed to the bg002h fork remote so CI can clone it.

- .github/workflows/ci.yml: 3 jobs (test/clippy/doc) now clone
  md-codec-local-stack at b308afd instead of fix/wallet-policy-hash-terminals
  at dbfe077.
- Cargo.toml [patch] rationale corrected (md-codec-local-stack is now
  remote, not local-only) — the prior framing was wrong about
  "pushing would pollute per-PR branches" since md-codec-local-stack
  is a dedicated separate branch.

Also includes the v0.10.1 reviewer artifact + the forward-looking
FOLLOWUPS entry filed by the reviewer:
- design/agent-reports/v0-10-1-tier-2-kiv-walk-review.md
- design/FOLLOWUPS.md `tier-2-gate-revisit-when-public-set-key-info-lands`

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bg002h
Copy link
Copy Markdown
Author

bg002h commented Apr 30, 2026

Sorry for the mess…I’m targeting PR to the wrong fork

@bg002h
Copy link
Copy Markdown
Author

bg002h commented Apr 30, 2026

Closing in favor of rust-bitcoin/rust-miniscript#935 — re-routed to true upstream (since wallet_policy/ already lives at rust-bitcoin/rust-miniscript:master). Same branch head (bg002h:fix/wallet-policy-hash-terminals), same content. Apologies for the spam.

—Brian (via Claude — forgive me, I'm a physician!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant