Conversation
Sjors
force-pushed
the
2025/06/musig2-power
branch
from
7b0dd9f to
f842cae
Compare
2 tasks
Sjors
force-pushed
the
2025/06/musig2-power
branch
from
2ecd3b6 to
0f809dd
Compare
This was referenced Jul 18, 2025
This was referenced Jul 30, 2025
Sjors
force-pushed
the
2025/06/musig2-power
branch
from
c524af5 to
66705f3
Compare
Owner
Author
|
Fresh rebase after bitcoin#31244 landed. |
Owner
Author
|
Added bitcoin#33135 and the latest change to bitcoin#33008 (storing one hmac record per policy and fingerprint combination). |
Owner
Author
|
The The fixup commits reflect changes made to the original branches. I'll up the commit history occasionally when one of its dependencies lands. |
16 tasks
Owner
Author
|
bitcoin#29675 landed! Waiting for bitcoin#29136 to get a rebase so I can update the stack here. |
Sjors
force-pushed
the
2025/06/musig2-power
branch
from
c1959d3 to
a42a034
Compare
Owner
Author
|
Rebased! bitcoin#32821 and bitcoin#33135 also landed. Down from 67 commits to 44. I haven't had a chance to manually retest the whole flow though. |
unused() descriptors do not have scriptPubKeys. Instead, the wallet uses them to store keys without having any scripts to watch for.
Sjors
force-pushed
the
2025/06/musig2-power
branch
from
a42a034 to
25e66d9
Compare
Owner
Author
|
Rebased after bitcoin#32471 landed. Down to 37 commits. |
Sjors
force-pushed
the
2025/06/musig2-power
branch
from
25e66d9 to
b7bb56e
Compare
A helper method to obtain all unused(key) descriptor SPKMs.
When a wallet contains only an unused(KEY) descriptor, use it. Previously the user would have to call listdescriptors and manually specify it.
Sjors
force-pushed
the
2025/06/musig2-power
branch
from
b7bb56e to
14c7fce
Compare
Sjors
force-pushed
the
2025/06/musig2-power
branch
from
14c7fce to
018d066
Compare
…tor' into codex/rebuild-musig2-power-20260501
…x/rebuild-musig2-power-20260501
…to codex/rebuild-musig2-power-20260501
DerivePolicy iterates {BECH32M, BECH32} \xc3\x97 {receive, change}
looking for descriptors that could back a BIP388 policy. A wallet that
holds only BECH32M descriptors (e.g. a Taproot/MuSig2 wallet)
legitimately has no BECH32 spk_man, so GetScriptPubKeyMan() returns
nullptr for those buckets. Likewise the dynamic_cast to
DescriptorScriptPubKeyMan is allowed to fail if a non-descriptor spk_man
somehow ended up registered.
Both null cases are recoverable -- we just want to skip the bucket and
move on -- so wrapping them in Assume() is wrong: in debug builds
Assume() aborts on null, which currently crashes bitcoin-qt when a user
triggers address verification on a MuSig2 wallet via the GUI:
wallet/wallet.cpp:NNN ... DerivePolicy(...): Assertion `spk_man'
failed.
Drop the Assume() wrappers; plain null checks are sufficient.
CWallet::AddWalletDescriptor created a plain DescriptorScriptPubKeyMan even when the wallet has WALLET_FLAG_EXTERNAL_SIGNER set, while LoadDescriptorScriptPubKeyMan (used by loadwallet) correctly upgrades to ExternalSignerScriptPubKeyMan. This left descriptors imported via importdescriptors / smart-createwalletdescriptor wired to a plain SPKM that doesn't know how to talk to the external signer, so any path that goes through ExternalSignerScriptPubKeyMan-only methods (BIP388 policy dispatch, displayaddress, FillPSBT for hardware wallets) silently skipped them until the wallet was unloaded and reloaded. Mirror LoadDescriptorScriptPubKeyMan's IsWalletFlagSet check so a freshly added descriptor is wired to the correct SPKM type from the start.
Adds a minimal mocks/signer_musig.py that implements the policy variants of enumerate, getdescriptors, register and displayaddress, and exercises CWallet::DisplayAddress's BIP388 dispatch through the new ExternalSignerScriptPubKeyMan::DisplayAddressPolicy helper. The test imports a tr(musig(...)) descriptor into the dedicated wallet_signer_musig2.py test via the mock signer's getdescriptors response, registers the policy, and asserts walletdisplayaddress round-trips a freshly derived MuSig2 address and rejects a wrong echo.
Multi-round signing flows like MuSig2 take two passes: round 1 exchanges public nonces, round 2 produces partial signatures. A single FillPSBT(sign=true) call therefore can never complete a fresh MuSig2 spend -- the user has to feed the round-1 PSBT back into FillPSBT a second time. The 'send' RPC, the GUI's send-coins dialog, the GUI's PSBT-operations dialog, and the bumpfee path all suffer from this: each makes a single sign call and returns the round-1 PSBT to the user. Hoist the second-pass logic into CWallet::FillPSBT itself so every caller benefits without code duplication. After the first pass leaves the PSBT incomplete, do a structural check: every input that participates in a MuSig2 session must already have a complete set of pub nonces from every expected participant. If so, run the same pass again on a copy; on success adopt it, on failure preserve the round-1 result so the caller can ferry the PSBT out-of-band as before. Skipped for non-MuSig2 PSBTs (no participants entries) and for MuSig2 PSBTs where some cosigner's nonces are still missing, so we don't issue an unnecessary device-confirmation prompt. Note: PSBT_IN_MUSIG2_PARTICIPANT_PUBKEYS entries can't be paired to PSBT_IN_MUSIG2_PUB_NONCE entries by aggregate pubkey -- the former stores the pre-tweak aggregate while the latter stores the post-tweak one (after BIP32 derivations and the BIP86 taproot tweak). The structural check sidesteps that. Refactor the existing one-pass body into a fill_pass lambda so both rounds share it.
Extend mocks/signer_musig.py with a signtx command that replays staged round-1 pubnonces and round-2 partial-signature PSBTs, and an injected mock_signtx_error JSON-error hook so the BIP388 policy signing failure path can be exercised. Extend test_bip388_musig2_policy in wallet_signer_musig2.py: - Happy path: stage a real 2-of-2 MuSig2 dance with two regular wallets on node 0, then drive the BIP388 wallet on node 1 through walletprocesspsbt. Both signtx calls from the MuSig2 round-1/round-2 retry in CWallet::FillPSBT are consumed and the result is broadcastable. - Hard-fail path: signer returns a structured error, and the wallet surfaces EXTERNAL_SIGNER_FAILED (-25). The BIP388 wallet has no local privkeys, so the local pass adds nothing; the test asserts the wallet does not silently return the unchanged PSBT. The mock's getdescriptors slot is wired so createwallet with external_signer=True auto-imports the MuSig2 descriptor as an ExternalSignerScriptPubKeyMan, the only SPKM kind that the BIP388 dispatch in CWallet::FillPSBT recognises.
ExternalSigner::SignTransactionPolicy invokes the signer subprocess via
RunCommandParseJSON, which throws std::runtime_error on a non-zero exit
status (e.g. device unplugged mid-flow, or hwi-rs reporting a transport
error). FillPSBTPolicy currently lets that exception escape and bubble
up through the RPC layer as a generic JSON-RPC internal error, instead
of the structured PSBTError::EXTERNAL_SIGNER_FAILED that callers get
when the signer instead returned a well-formed {"error":...} payload.
Wrap the call so a runtime_error is folded into the same failure_reason
+ signer_ok=false path. Behavior for the well-formed-error path is
unchanged. Preparation for a follow-up that lets callers distinguish a
failure that happened after the local pass already advanced a multi-
round MuSig2 PSBT from one where it didn't.
Extend mocks/signer_musig.py with a mock_signtx_crash hook that makes policy signtx exit non-zero on demand. A later commit uses this hook once the live MuSig2 test has a returned PSBT that can exercise the intended hard-fail path cleanly.
Multi-round signing flows like MuSig2 take more than one FillPSBT pass: round 1 exchanges public nonces, round 2 produces partial signatures. Currently FillPSBTPolicy lets the local descriptor signer contribute first (e.g. a hot-key cosigner's round-1 pubnonce generated from a freshly-stashed secnonce), then unconditionally hard-fails the whole call if the external signer is unavailable (EXTERNAL_SIGNER_FAILED). That throws away the just-contributed local pubnonce and -- more importantly -- the matching secnonce stored in the SPKM, forcing the user to restart from scratch when the device comes back. Treat a signer error as soft when the local pass on this call actually advanced the PSBT -- specifically, when it added at least one new MuSig2 pubnonce or partial signature. In that case we log the signer error, keep the partial PSBT, and return success; the outer FillPSBT loop already reports complete=false to the caller, so e.g. send returns a round-1 PSBT and a later walletprocesspsbt (with the device back online) can drive the device round and then round 2, reusing the stashed secnonce. When the local pass added nothing -- single-round flows with no MuSig2 in the PSBT, or a MuSig2 replay where the hot cosigner has already contributed everything it can in a previous call -- we hard- fail. Otherwise a no-op walletprocesspsbt against a missing signer would silently succeed and mislead the caller into thinking the unchanged PSBT was now done.
Walks all BIP 32 key expressions inside a descriptor (including musig() participants and subdescriptors) and returns each (KeyOriginInfo, root CExtPubKey) pair. Lets callers map descriptor xpubs back to the master fingerprints they came from without having to round-trip through ExpandPrivate. Used in the next commit by importdescriptors to bind known wallet HD seeds to descriptor xpubs at import time.
Returns every depth-0 extended key the wallet knows the private key for, indexed by the seed's master fingerprint. Walks both active descriptors and unused(KEY) SPKMs added via addhdkey. Used in the next commit by importdescriptors to look up xprvs that match a descriptor's key origins.
When importing a descriptor into a wallet that has private keys enabled, walk the descriptor's key origins and bind any xprv the wallet already knows (via addhdkey or an active descriptor) by master fingerprint + path. Verifies the derived xpub matches the descriptor's root xpub before binding to guard against fingerprint collisions. This lets callers import e.g. tr(musig([fp_A/87h/1h/0h]xpub_A,[fp_B/87h/1h/0h]xpub_B)/<0;1>/*) into a wallet that holds B's xprv without manually splicing the xprv into the descriptor string. The previous workaround forced the caller to round-trip through derivehdkey private=true and substitute the xprv back in -- error-prone, and exposes raw xprvs to anywhere the descriptor string travels.
Three subcases in wallet_importdescriptors.py: - single-key wpkh autobinds the wallet's addhdkey'd xprv and produces a fully-signing descriptor (round-trips a real spend); - a descriptor whose origin fingerprint matches no wallet seed still hits the existing 'Cannot import descriptor without private keys' guard (autobind is a no-op); - a tr(musig(...)) descriptor with two xpub-only cosigners imports into a wallet that holds both xprvs and runs both MuSig2 rounds + finalize in a single walletprocesspsbt. Also rewrite wallet_signer_musig2.py's MuSig2 createwallet/ displayaddress subtests to lean on the same autobind path: the wallet now adds an HD seed via `addhdkey`, derives the local m/87h/1h/0h account via `derivehdkey`, and imports an xpub-only `tr(musig(...))` descriptor that the autobind logic re-attaches to the wallet's own xprv. This drops the previous LOCAL_XPRV constant and the unloadwallet/loadwallet workaround; the wallet upgrade in `wallet: upgrade to ExternalSignerScriptPubKeyMan in AddWalletDescriptor` already produces an external-signer SPKM on import. The mock signer's `displayaddress` no longer requires a `--policy-name` argument so the same mock binary can serve both the pre-policy createwallet/displayaddress flow and the BIP388 policy flow further down the test, removing the need for the restart_node + signer.py shim.
Teach ExternalSignerScriptPubKeyMan to enumerate every connected signer while keeping the single-signer wrapper for callers that still require exactly one device. Fan CWallet's BIP388 RegisterPolicy, DisplayAddress and FillPSBT paths out to connected signers with matching registered policy metadata. This lets multi-device MuSig2 wallets collect every cosigner's nonce and partial signature in one walletprocesspsbt flow, while preserving the single-signer fallback for non-policy address display. Add a two-device wallet_signer_musig2.py case that verifies registerpolicy records both devices and that one walletprocesspsbt call completes the full MuSig2 signing flow.
Sjors
force-pushed
the
2025/06/musig2-power
branch
from
ce95473 to
df22882
Compare
Owner
Author
|
Rebased and rewrote commit history so it plays more nicely on top of the dedicated musig2 external tests added by bitcoin#33112. The functional test also emulates the external signer in a more sane way. That said, much vibe remains. I (locally) ran the integration tests in Sjors/async-hwi#2 against the latest code here and they still pass. I'm going to wait for some of the |
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.
Combines the following:
addhdkeyRPC to add just keys to wallets via newunused(KEY)descriptor bitcoin/bitcoin#29136And then implements BIP388 on top.
Can we be tested with either:
The pull request to upstream bitcoin/bitcoin should be in good shape. The remaining code here is currently a bit heavily vibe coded. It needs more polishing and especially the use of BIP388 and storing an hmac, needs conceptual discussion.
Update note to self:
(update master on the fork repo if needed)
Tests to run as a quick sanity check: