Repository navigation
Conversation
|
Can you trim that PR description down, it smells a lot like LLM slop? (I did not read it FTR) |
Done. I didn't know LLM usage has to be disclosed here. Good to know now, I'll take this into account. I used LLM heavily when working on this: code, tests, commits, bitcoin core compatibility verification, etc. |
|
I'm gonna say concept-NACK on this inline implementation of key aggregation. Though it is kinda a cool workaround for "we don't have a version of rust-secp that supports musig and is supported by rust-bitcoin".. The resulting code is just too big, hard to verify and hard to maintain. Instead we need to get rust-secp 0.29.x to support musig somehow. |
|
@apoelstra I agree. Crypto primitives should live in the The current stable version of What is the plan? Wait for the 0.32.0 release, then backport MuSig2 support to 0.29.x, then switch to that version here and implement |
|
Roughly, yes. The backport is not trivial because there are C linker issues and semver issues to navigate. |
Add a descriptor engine to entropylab-wasm (new src/descriptor.rs, one new export: el_desc_derive). rust-miniscript 13.1.0 (exact pin, Cargo.lock) parses the descriptor — BIP380-386 key expressions, xpub and xprv/WIF alike, a supplied #checksum verified — and derives the address, scriptPubKey, and participant keys at a child index. Multipath (<0;1>) descriptors are refused: one call derives one output, so callers pass a single branch. BIP390 musig() key expressions are layered on top of rust-miniscript (upstream support is still in review: rust-bitcoin/rust-miniscript#954): participants are derived, KeySort-ed, and aggregated per BIP327, a trailing /NUM/.../* path derives from the aggregate's BIP328 synthetic xpub, and the concrete aggregate key is substituted back for rust-miniscript to finish. Two more upstream gaps are closed in the same pass: rawtr(musig(...)) — rust-miniscript does not parse rawtr — and taproot sortedmulti_a, which is derived, sorted as x-only bytes, and rewritten to the multi_a it denotes. musig() outside tr()/rawtr(), nested musig(), hardened aggregate steps, and ranged participants under a ranged musig() are all rejected per BIP390. Crate tests pin the published BIP327 key-aggregation, BIP328 synthetic-xpub and BIP390 descriptor vectors (valid and invalid), plus a BIP86 vector. The regenerated artifact (miniscript adds ~540 KB of wasm) is committed per convention; CI rebuilds it from the Rust sources and tests the fresh build, so the committed copy cannot hide a broken crate. Verified: cargo test 9/9; node --test test/wasm-boundary.test.mjs and the full tracked suite (832 pass, 0 fail); npm run build && npm run verify; cargo +nightly miri test (allocator suite; the secp256k1 C statics are Miri-incompatible, as before).
Add a descriptor engine to entropylab-wasm (new src/descriptor.rs, one new export: el_desc_derive). rust-miniscript 13.1.0 (exact pin, Cargo.lock) parses the descriptor — BIP380-386 key expressions, xpub and xprv/WIF alike, a supplied #checksum verified — and derives the address, scriptPubKey, and participant keys at a child index. Multipath (<0;1>) descriptors are refused: one call derives one output, so callers pass a single branch. BIP390 musig() key expressions are layered on top of rust-miniscript (upstream support is still in review: rust-bitcoin/rust-miniscript#954): participants are derived, KeySort-ed, and aggregated per BIP327, a trailing /NUM/.../* path derives from the aggregate's BIP328 synthetic xpub, and the concrete aggregate key is substituted back for rust-miniscript to finish. Two more upstream gaps are closed in the same pass: rawtr(musig(...)) — rust-miniscript does not parse rawtr — and taproot sortedmulti_a, which is derived, sorted as x-only bytes, and rewritten to the multi_a it denotes. musig() outside tr()/rawtr(), nested musig(), hardened aggregate steps, and ranged participants under a ranged musig() are all rejected per BIP390. Crate tests pin the published BIP327 key-aggregation, BIP328 synthetic-xpub and BIP390 descriptor vectors (valid and invalid), plus a BIP86 vector. The regenerated artifact (miniscript adds ~540 KB of wasm) is committed per convention; CI rebuilds it from the Rust sources and tests the fresh build, so the committed copy cannot hide a broken crate. Verified: cargo test 9/9; node --test test/wasm-boundary.test.mjs and the full tracked suite (832 pass, 0 fail); npm run build && npm run verify; cargo +nightly miri test (allocator suite; the secp256k1 C statics are Miri-incompatible, as before).
Removes the crate's musig() layer — BIP327 KeySort/KeyAgg, BIP328 synthetic-xpub derivation, and the rawtr(musig()) template handling — along with the BIP327/328/390 vector tests on the Rust side and the JS boundary, and the README mentions. Descriptor evaluation keeps everything the app uses: BIP380-386 via rust-miniscript, the taproot sortedmulti_a rewrite (derived, sorted as x-only bytes, rewritten to multi_a), #checksum verification, and multipath refusal. A descriptor containing musig() now fails loudly in rust-miniscript's parser. The wasm artifact sheds ~17 KB of base64. The support can return when upstream lands it (rust-bitcoin/rust-miniscript#954). Verified: cargo test 4/4; npm test (tracked suites) 849 pass / 0 fail; fuzz:msig 1000 configs / 18,856 checks, 0 mismatches; fuzz:lifehash clean; npm run build && npm run verify; test:browser (Firefox, Chrome).
Add a descriptor engine to entropylab-wasm (new src/descriptor.rs, one new export: el_desc_derive). rust-miniscript 13.1.0 (exact pin, Cargo.lock) parses the descriptor — BIP380-386 key expressions, xpub and xprv/WIF alike, a supplied #checksum verified — and derives the address, scriptPubKey, and participant keys at a child index. Multipath (<0;1>) descriptors are refused: one call derives one output, so callers pass a single branch. BIP390 musig() key expressions are layered on top of rust-miniscript (upstream support is still in review: rust-bitcoin/rust-miniscript#954): participants are derived, KeySort-ed, and aggregated per BIP327, a trailing /NUM/.../* path derives from the aggregate's BIP328 synthetic xpub, and the concrete aggregate key is substituted back for rust-miniscript to finish. Two more upstream gaps are closed in the same pass: rawtr(musig(...)) — rust-miniscript does not parse rawtr — and taproot sortedmulti_a, which is derived, sorted as x-only bytes, and rewritten to the multi_a it denotes. musig() outside tr()/rawtr(), nested musig(), hardened aggregate steps, and ranged participants under a ranged musig() are all rejected per BIP390. Crate tests pin the published BIP327 key-aggregation, BIP328 synthetic-xpub and BIP390 descriptor vectors (valid and invalid), plus a BIP86 vector. The regenerated artifact (miniscript adds ~540 KB of wasm) is committed per convention; CI rebuilds it from the Rust sources and tests the fresh build, so the committed copy cannot hide a broken crate. Verified: cargo test 9/9; node --test test/wasm-boundary.test.mjs and the full tracked suite (832 pass, 0 fail); npm run build && npm run verify; cargo +nightly miri test (allocator suite; the secp256k1 C statics are Miri-incompatible, as before).
Removes the crate's musig() layer — BIP327 KeySort/KeyAgg, BIP328 synthetic-xpub derivation, and the rawtr(musig()) template handling — along with the BIP327/328/390 vector tests on the Rust side and the JS boundary, and the README mentions. Descriptor evaluation keeps everything the app uses: BIP380-386 via rust-miniscript, the taproot sortedmulti_a rewrite (derived, sorted as x-only bytes, rewritten to multi_a), #checksum verification, and multipath refusal. A descriptor containing musig() now fails loudly in rust-miniscript's parser. The wasm artifact sheds ~17 KB of base64. The support can return when upstream lands it (rust-bitcoin/rust-miniscript#954). Verified: cargo test 4/4; npm test (tracked suites) 849 pass / 0 fail; fuzz:msig 1000 configs / 18,856 checks, 0 mismatches; fuzz:lifehash clean; npm run build && npm run verify; test:browser (Firefox, Chrome).
|
1b1f458 needs rebase |
fametrano
left a comment
There was a problem hiding this comment.
Two problems, both outside KeyAgg, so they should carry over to a version built on rust-secp's MuSig2.
-
musig()accepts x-only and uncompressed participants:tr(musig(f9308a019258c31049344f85f89d5229b531c845836f99b08601f113bce036f9,03dff1d7...,023590a9...)) tr(musig(0414fc03...cf,03dff1d7...))BIP327 KeyAgg takes 33-byte compressed keys, and BIP386 forbids uncompressed keys inside
tr(). Bitcoin Core parses each participant in itsMUSIGcontext (descriptor.cpp#L2102), where a 32-byte key is "Pubkey is invalid" (L2014-L2024) and an uncompressed one is "Uncompressed keys are not allowed" (L1986, L2011). btclib-wallet rejects both too. Here the x-only case is documented as intended (key.rs#L112) and both are tested as valid (mod.rs#L2246, mod.rs#L2261, key.rs#L2152). The uncompressed key also passes the Tap context check becauseis_x_only_keyis true for everymusig()(key.rs#L1865). -
update_input_with_descriptorpanics on any descriptor that containsmusig():panicked at src/psbt/mod.rs:995:22: definite keys cannot be multikeysKeySourceLookUpexpectsfull_derivation_path()to beSomefor a definite key, but it isNoneforMusig(psbt/mod.rs#L995, key.rs#L1003).update_output_with_descriptorgoes through the same helper (psbt/mod.rs#L1096), so it should panic the same way. Returning a path would not fix it, becausemaster_fingerprint()is 00000000 forMusig(key.rs#L975). Until PSBT supportsmusig()(BIP373), these calls should return an error.
How I tested: I used a test file outside the PR, at 1b1f458. It parses 13 tr() descriptors with musig(): the four BIP390 tr() vectors, plus reordered, ranged, multipath, repeated and single participants, and musig() in multi_a. It derives them (indexes 0 to 2 where ranged) and compares the 35 scriptPubKeys with btclib-wallet: all match. All BIP390 invalid vectors except combo() are rejected (bips 927b6de). The panic shows in update_input_with_descriptor for tr(musig(A,B)/0/*), tr(musig(A/0/*,B/0/*)) and musig() in a leaf. cargo test --lib passes.
|
@apoelstra, rebased and addressed the review findings. The inline key aggregation still needs to be replaced with rust-secp's MuSig2 support; the fixes in this push don't depend on it. @fametrano, thanks for the review. Both issues are fixed:
This PR depends on three supplementary fixes. Their commits come first in this PR:
Regarding the last commit in the pushed PR version. I found that planner The series now has these parts, in order:
I'll drop the carried parts from this PR as they land upstream. The context-check commits also fix three things #991 doesn't cover yet: the sortedmulti constructors (the #915 rebase issue from June), the leaves in For key aggregation, I have a MuSig2 backport to secp256k1 0.29.1 from May. It's a proof of concept: it uses 🤖 AI disclosure: I used Claude and Codex for the fixes, tests, reviews, and this comment. |
|
Thanks, I re-checked at 45793bd and both problems are fixed. I used a throwaway test outside the PR:
A nit: This does not cover the inline KeyAgg. |
|
I appreciate the disclosures but FYI I am not going to read these massive AI textwall comments so it's likely I'll miss stuff. But:
|
When applying a script-path plan to a fresh PSBT input, update_psbt_input recorded the signing keys but omitted the selected script's leaf hash. rust-bitcoin's Psbt::sign uses these hashes to choose scripts to sign, so it produced no script-path signature. Initialize new key-origin entries with the selected leaf hash. New key-path entries remain empty; existing entries continue to accumulate hashes without duplicates. Test leaf-hash metadata, repeated updates, and signing a freshly prepared script spend. Assisted-by: Claude Opus 5.5
DefiniteDescriptorKey::new checks for wildcards, hardened steps and multipath derivation, but not for BIP32's maximum depth of 255. A key that passes these checks but derives past that depth makes rust-bitcoin return MaximumDepthExceeded, and derive_public_key turns this error into a panic labeled "cryptographically unreachable". A descriptor string can trigger it: an xpub with a long enough path parses, becomes definite, and panics when its script pubkey is computed. Return the new NonDefiniteKeyError::MaximumDepthExceeded from DefiniteDescriptorKey::new instead. Derivation at an index and into_definite go through it, so they return the error too. Only the steps after the extended key count, since its depth already includes its origin steps. Assisted-by: Claude Opus 5.5
A key expression that derives past BIP32's maximum depth of 255 can never produce a key. The previous commit rejects such a key when it becomes definite. Reject it when parsing too, so that the error points at the string that caused it. Count the steps after the extended key, plus one for a wildcard, for xpubs and xprvs, including multipath keys. Origin steps don't count. Assisted-by: Claude Opus 5.5
Miniscript::from_tree checks the global rules of the script context on every node. Move that loop into check_global_validity_recursive, so that the descriptor constructors can run the same checks. No behavior change. Assisted-by: Claude Opus 5.5
Parsing a descriptor checks the rules of the script context on every node of its Miniscript: which keys the context allows and its script size limits. Wsh::new, Sh::new and Bare::new run only the top-level checks, Tr::new checks only the internal key, and new_sortedmulti runs no checks at all since SortedMultiVec was removed in rust-bitcoin#915. Constructors like Miniscript::multi and Miniscript::pk don't check the context either, and Miniscript::from_ast checks only the node it builds. So the descriptor constructors accept Miniscript that parsing rejects. For example, Descriptor::new_wsh_sortedmulti accepts an x-only key. The result doesn't parse back from its Display, and into_definite panics when the translation checks the context again. Descriptor::new_sh_sortedmulti accepts 16 keys, which make a 547-byte redeem script, and address() then panics on its assertion that the size was checked. Descriptor::new_tr accepts a leaf with an uncompressed key, and into_definite panics on it too. Call check_global_validity_recursive in Wsh::new, Sh::new and Bare::new, and on every leaf in Tr::new. Build both new_sortedmulti constructors through Wsh::new and Sh::new. SortedMultiVec also ran the local checks of the context, but a multisig that passes the global checks passes the local ones too. Assisted-by: Claude Opus 5.5
Check that the descriptor constructors reject the keys and the script size that parsing rejects, also for a key below the root and in a Taproot leaf, and that the sortedmulti constructors build what parsing builds, with the keys sorted in the script. Assisted-by: Claude Opus 5.5
When a leaf of the Taproot tree doesn't compile, compile_tr and compile_tr_native return the error, but compile_tr_private_experimental unwraps it and panics. Return the error, like the other two do. The next commit makes a leaf with an uncompressed key fail to compile, which would make this function panic. Assisted-by: Claude Opus 5.5
check_global_consensus_validity checks the keys of pk_k() and multi() against the script context, but not the key of pk_h(). So pkh() accepts keys that pk() rejects: x-only keys outside Taproot, and uncompressed keys in Segwit v0 and Taproot. Descriptor parsing relies on this check, so wsh(pkh(<uncompressed key>)), wsh(pkh(<x-only key>)), sh(pkh(<x-only key>)) and the bare c:pk_h(<x-only key>) parse, while the same keys in pk() don't. Bitcoin Core rejects the wsh() and sh() ones because of the key. The compiler tries both pk_k() and pk_h() for a key and drops the fragments that fail this check. When pk_k() fails, pk_h() remains, so compiling pk(<uncompressed key>) for wsh() gives wsh(pkh(<uncompressed key>)). Such an output can't be spent with a standard transaction, since Segwit v0 signature checks require compressed keys. For a Taproot leaf, the compiler gives tr(K,pkh(<uncompressed key>)), which doesn't parse. Check the key of pk_h() like the key of pk_k() in all four contexts. Terminal::RawPkH holds only a hash, so it stays unchecked. Miniscript::from_str and tr() leaves already rejected these keys in validate(). They now fail earlier, with a context error instead of a validation error. As with pk_k(), Miniscript::from_ast, translate_pk and parsing with permissive ValidationParams now reject such keys in pk_h() too. Assisted-by: Claude Opus 5.5
Parse pkh() with compressed, uncompressed and x-only keys in every context. Check that compiling pk() with a key that the context doesn't allow fails instead of falling back to pk_h(), in every Taproot compile function too. Assisted-by: Claude Opus 5.5
Teach the expression tree to preserve derivation suffixes after function-style key nodes, such as musig(A,B)/0/*. The parser records postfix data on the node that owns the closing parenthesis. Key-expression callers parse a terminal key from its name as before, and serialize a function-style key expression back to a string to parse it. Taproot internal keys, miniscript key arguments and policy pk() arguments use these helpers. The miniscript and policy parsers first collect their nodes in pre-order, skipping the arguments of fragments that take keys, so pk() and multi() parse a function-style key argument as a whole. The mathematical notation of semantic policies reads a pk() argument up to its matching parenthesis. This keeps policies lifted from musig() descriptors parseable from their string form. Non-key consumers reject postfixes through the standard node validation helpers, including threshold parsing, so Miniscript fragments cannot silently accept key-derivation syntax. Assisted-by: Claude Opus 5.5
Add DescriptorPublicKey::Musig and DescriptorMusigKey so descriptors can retain participant keys, aggregate-level derivation, and wildcard state. Route construction through DescriptorMusigKey::new. That keeps the parser's BIP390 invariants attached to the type itself: no empty or nested aggregates, no x-only or uncompressed participants, no invalid aggregate-level derivation, no hardened aggregate derivation, and no mismatched participant multipath lengths. Aggregate-level multipath paths must form a single BIP389 step, so that the key can be written out and parsed back; like BIP389, this rejects a repeated index such as musig(...)/<0;0>/*. Aggregate-level derivation starts from the BIP328 synthetic xpub, at depth 0, so its paths may take at most 255 steps, counting a wildcard as one. DefiniteDescriptorKey::new checks the depth of participants like that of other keys. BIP390 forbids empty aggregates but does not require at least two participants. Keep musig(K) valid to match Bitcoin Core's musig() descriptor behavior. Participants must be compressed keys, because BIP327 KeyAgg takes 33-byte keys; Bitcoin Core rejects x-only and uncompressed participants too. Extend the DescriptorPublicKey helper surface to the new variant so musig() round-trips and can participate in descriptor derivation before aggregate key computation is wired in. into_single_descriptors splits a multipath musig() key like other multipath keys and copies a single-path one into every descriptor. The network helper distinguishes mixed participant xpub networks, and musig() reports no synthetic master fingerprint. Context helpers describe the aggregate key: musig() reports itself as an x-only key, which keeps it out of non-Taproot contexts. Assisted-by: Claude Opus 5.5
Implement the descriptor-facing subset of BIP327 key aggregation and BIP328 synthetic xpub derivation in a private descriptor::musig module. Definite musig() keys derive participant keys, sort them as BIP390 requires, aggregate them with BIP327 coefficients, and apply aggregate-level unhardened derivation through the BIP328 synthetic xpub when needed. The backend returns MusigKeyAggError on failure: a participant that is not definite, a failed aggregate-level BIP32 derivation, or BIP327's point-at-infinity aggregate. DefiniteDescriptorKey::derive_public_key keeps its infallible interface and panics if derivation fails, as it already does for xpubs. Keys deeper than BIP32 allows never become definite, so only cryptographic edge cases remain, such as the infinity aggregate, which is unreachable for non-adversarial participants. Document that panic. Tests pin BIP328 aggregate/xpub vectors, synthetic xpub metadata, and BIP390 scriptPubKey vectors including duplicated participants with aggregate derivation. Assisted-by: Claude Opus 5.5
Deriving a definite musig() key returns only the final public key. That key does not reveal the participant keys, the BIP327 aggregate before aggregate-level derivation, or the BIP328 synthetic xpub that aggregate-level derivation starts from. Callers need these to set up MuSig2 signing sessions or to fill BIP373 PSBT fields, which this crate does not support yet. Add public helpers that return this information, so callers do not have to reimplement BIP390 derivation rules, BIP327 key aggregation, or the BIP328 synthetic xpub: - DescriptorMusigKey::derivation_kind tells whether derivation applies to the participants before aggregation or to the aggregate through the synthetic xpub; - DescriptorMusigKey::aggregate_public_key and synthetic_xpub return the aggregate key and its synthetic xpub; - DescriptorMusigKey::derived_participants_at_index and DescriptorPublicKey::for_each_leaf_key give the participant keys; - DescriptorMusigKey::try_derive_public_key derives the final key like DefiniteDescriptorKey::derive_public_key, but returns MusigKeyAggError instead of panicking if key aggregation or aggregate-level derivation fails. Assisted-by: Claude Opus 5.5
Make Descriptor::parse_descriptor parse musig() participants itself instead of treating the aggregate as one opaque key string. It shares the musig() parser with DescriptorPublicKey::from_str, passing a participant parser that also accepts secret keys, so the aggregate is built directly from the parsed participants. Each secret participant is converted to its public descriptor key and inserted into the KeyMap individually. There is no aggregate secret key to store. A nested musig() is rejected before its participants are parsed. to_string_with_secret reconstructs the musig() expression with secret participants when the KeyMap has them. It renders the aggregate derivation suffix from DescriptorMusigKey directly, preserving round-trip serialization without reparsing the public key string. Tests cover BIP32 xprv and WIF participants because those pass through different DescriptorSecretKey parsing paths. Assisted-by: Claude Opus 5.5
Add descriptor-level coverage for musig() in Taproot script key positions. The tests exercise pk(musig(...)), pkh(musig(...)), multi_a(..., musig(...)) and sortedmulti_a(..., musig(...)), including a ranged aggregate derivation case. Add invalid placement coverage for legacy and Segwit v0 contexts, top-level musig(), musig() as a Taproot script branch, and descriptor level nested musig(). pkh(musig(...)) is rejected in sh(), wsh(), sh(wsh()) and bare descriptors, also below the root of the script. This keeps musig() accepted only as Taproot key material. Also cover the policy compiler and the descriptor constructors. A policy with a musig() key compiles only for Taproot, and the constructors reject musig() keys outside Taproot. Both rely on the context checks of pk_h() and of the descriptor constructors from the earlier commits. These tests document the intended BIP390 boundary: musig() is a key expression for Taproot descriptors, not a replacement for legacy public-key expressions or Miniscript fragments. Assisted-by: Claude Opus 5.5
The PSBT updaters record a (fingerprint, derivation path) key source for every key of the descriptor. A musig() key has no single key source: full_derivation_path() is None for it, so KeySourceLookUp panicked with "definite keys cannot be multikeys". This affected PsbtExt::update_input_with_descriptor, update_output_with_descriptor, and both update_with_descriptor_unchecked methods of PsbtInputExt and PsbtOutputExt. Returning a key source would not help: master_fingerprint() is 00000000 for musig(), and the correct metadata is defined by BIP373 (MuSig2 participant keys and per-participant derivations), which the PSBT types of rust-bitcoin 0.32 cannot express. Return an error instead. The check runs while translating keys, before any PSBT field is written, so a failed update leaves the PSBT unchanged. API changes: - add psbt::DescriptorUpdateError with a MusigUnsupported variant; - PsbtInputExt::update_with_descriptor_unchecked and PsbtOutputExt::update_with_descriptor_unchecked return it instead of descriptor::NonDefiniteKeyError, which they could never return since they take definite keys; - add MusigUnsupported variants to UtxoUpdateError and OutputUpdateError. Assisted-by: Claude Opus 5.5
Assets identify keys by key source: a master fingerprint and a derivation path. A musig() key has no single key source: master_fingerprint() is 00000000 for every aggregate, and full_derivation_paths() returns the aggregate-level paths. Assets added for one musig() key therefore matched any other aggregate with the same path, and Plan::update_psbt_input then wrote (00000000, path) origins into the PSBT. Never match musig() keys in Assets, and skip them when building Assets from descriptor keys. Spend paths that need a musig() key become unplannable with Assets, which cannot express that a wallet is able to complete a MuSig2 session. Other spend paths of the same descriptor stay plannable. Other asset providers, such as signature maps or a PSBT holding the final MuSig2 signature, can still plan musig() spends. For those, update_psbt_input now writes no tap_key_origins entry for the musig() key instead of a fabricated one. The right metadata is defined by BIP373, which is not supported yet. Document what master_fingerprint(), full_derivation_path() and full_derivation_paths() return for musig() keys. Assisted-by: Claude Opus 5.5
A musig() key reports itself as an x-only key, so the script context checks keep it out of non-Taproot scripts. But the error then reads "x-only key musig(...) not allowed in Segwitv0", which is confusing when every participant is a compressed key. Add MiniscriptKey::is_musig, which defaults to false, and check it first in the Legacy, Segwitv0 and Bare contexts. They now return ScriptContextError::MusigNotAllowed, which reads "musig() key ... is only allowed in tr(), not in Segwitv0". Pkh::new and Wpkh::new use the same check. Assisted-by: Claude Opus 5.5
|
@apoelstra thanks. The context checks are now #1069 and #1070 (merge #1069 first). I'll switch to the secp @fametrano thanks for re-checking.
This is addressed in commit "miniscript: report musig() outside tr() with its own error". |
…ends 3e9074d plan: fix missing signatures for fresh Taproot script spends (Boris Nagaev) Pull request description: When `Plan::update_psbt_input` prepares a fresh PSBT input for a Taproot script spend, it records the signing keys but gives their `tap_key_origins` entries an empty leaf-hash list. That list tells the signer which scripts to sign. With no hashes listed, rust-bitcoin's `Psbt::sign` produces no script-path signature, even when it has the required private key. Initialize new entries with the selected script's leaf hash. New key-path entries get an empty list, since those spends use no script. The tests check leaf hashes for key spends and `pkh`/`multi_a` leaves, accumulation without duplicates across repeated updates, and signing a freshly prepared script spend. This pre-existing bug was found while working on #954. _:robot: AI disclosure: The patch and tests were generated with Claude Opus 5.5. Codex reviewed the change and drafted this description._ ACKs for top commit: apoelstra: ACK 3e9074d; successfully ran local tests Tree-SHA512: a23985fb48085105faa6b120e21be6220eeee4d4e4a0ab5da542babff0c9121467005e3040f1c5593078e7db812a39918897d26b50fc6e8a48c93df9991f8764
f2862f7 descriptor: reject keys deeper than BIP32 allows when parsing (Boris Nagaev) 613ed0d descriptor: reject definite keys deeper than BIP32 allows (Boris Nagaev) Pull request description: `DefiniteDescriptorKey::new` checks for wildcards, hardened steps and multipath derivation, but not for BIP32's maximum depth of 255. A key that passes these checks but derives past that depth makes rust-bitcoin return `MaximumDepthExceeded`, and `derive_public_key` turns that error into a panic labeled "cryptographically unreachable". A descriptor string is enough to trigger it: `tr(xpub/0/0/.../0)` with 256 steps below a master xpub parses, passes `into_definite()`, and panics in `script_pubkey()`. This is missing validation in miniscript. Deriving that deep used to overflow rust-bitcoin's depth counter (rust-bitcoin/rust-bitcoin#4308). `rust-bitcoin 0.32.6` made it a recoverable error (rust-bitcoin/rust-bitcoin#4434), which miniscript doesn't handle. The first commit makes `DefiniteDescriptorKey::new` return the new `NonDefiniteKeyError::MaximumDepthExceeded`. `into_definite()` and derivation at an index go through it, so they report the error too. Only the steps after the extended key count, since its depth already includes the steps of its origin. The second commit rejects such key expressions when parsing, with the new `MalformedKeyDataKind::MaximumDepthExceeded`. It counts a wildcard as one more step and covers xprvs and multipath keys. It's a separate commit because some strings that parse today will stop parsing, even though no key can be derived from them. It can be dropped if parsing should stay unchanged. Together, they remove the deterministic depth panic. Derivation can still fail in BIP32's invalid-child case, which has a probability below 2^-127 and stays `unreachable!`. Tests cover depths 255 and 256, a nonzero starting depth, wildcard derivation, multipath keys, xprvs and origin paths. This pre-existing bug was found while working on #954. 🤖 _AI disclosure: The patch, tests and this description were generated with Claude Opus 5.5. Codex reviewed the approach._ ACKs for top commit: apoelstra: ACK f2862f7; successfully ran local tests Tree-SHA512: 671b1a6b284fe1a3357c9b39236ad41cbe267059c18c4db93a147c76038fb349db7897a6662530d958ca61e97c11eb81718c14316dd6f8df2ad1449612bf5dd4
Summary
This PR adds descriptor support for the BIP390
musig()key expression in Taproot descriptors and Tap Miniscript key positions.This addresses rust-bitcoin/rust-miniscript#182.
The implementation covers:
musig(A,B)/0/*, in descriptors and policies;musig()as a first-classDescriptorPublicKeyvariant;musig()participants;musig()descriptors, and not matchingmusig()keys in plannerAssets, until BIP373 support;The first commits come from four separate PRs and will be dropped once those merge:
Plan::update_psbt_input;musig()validation builds on it;pkh()keys, against the script context, which keepsmusig()out of non-Taproot scripts.🤖 AI disclosure: The original patches were generated with Codex and cross-validated with Claude. The review fixes were generated with Claude Opus 5.5 and reviewed by Codex.