#36332 sign: add musig2 data after validating musig2 involvement

ReviewabilityReviewsAgreementSize
#36332 achow101 sign: add musig2 data after validating musig2 involvementWaiting on author1Blocked20 + 48 tests
  • Prevent leaking unrelated MuSig2 keys and derivation paths into PSBTs when multiple descriptors are used
  • Reviewing now is worthwhile, though an inverted test assertion remains to be updated
  • Unaddressed objection: a functional test assertion is inverted (polespinasa)
  • Verified by testing across five descriptor variants (fametrano)
  • Concept approval with suggestions to optimize repeated key lookups (Bicaru20)
  • Suggested avoiding repeated participant pubkey lookups during signing (vicjuma)
  • 20 lines added or modified outside tests
  • 48 lines added or modified in tests
  • 18 lines removed in total
  • 2 files, 3 commits
RPC / REST / ZMQ#18 of 55
P3 · bug fix
  • P3 because it fixes data correctness in PSBT RPC outputs when multiple descriptors are supplied
  • MuSig2 signing is an advanced workflow, making this bug fix lower priority than core RPC work
  • Ranking pass: Moved up 7: injecting unrelated key-origin metadata into shared PSBTs is a data-correctness and privacy leak, above the other PSBT polish items.
Ranking notes
Reviewability: ReadyStalePaused
Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets
Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs.
Size: lines added or modified outside tests, then in tests; greener = smaller.

Goal

Defers populating MuSig2 aggregate pubkeys and participant BIP 32 derivation paths until after verifying that the aggregate is actually relevant to the script being signed. In `SignTaproot`, MuSig2 aggregate pubkeys are no longer inserted unconditionally into `SignatureData`. Instead, `SignMuSig2` queries the provider, checks aggregate key involvement, and only then records the MuSig2 pubkeys and participant key origins.

Problem: When RPC methods like `descriptorprocesspsbt` or `utxoupdatepsbt` are invoked with multiple descriptors, all descriptor data is combined into a single `SigningProvider`. The signer assumed all MuSig2 entries in the provider applied to the input being signed, leaking unrelated MuSig2 participant pubkeys, derivation paths, and leaf hashes into inputs and outputs where they do not belong.

Evidence of importance:

  • Fixes issue #36323: 'wallet/psbt/musig2: overpopulating participant leaf_hashes in utxoupdatepsbt and descriptorprocesspsbt'
  • PR description explains that 'if one of the given descriptors involved a MuSig2, the MuSig2 information would be accidentally added to the SignatureData which results in those fields appearing in the resulting PSBT'

Scope: Small patch to `src/script/sign.cpp` with functional regression tests in `test/functional/rpc_psbt.py`.

Risk of a mistake: medium (wallet, rpc_api). A flaw in MuSig2 signing or PSBT metadata population could omit required participant details or create malformed PSBTs, blocking transaction signing or finalization.

Changed since the previous assessment: Diff provided; confirms the mechanism in `SignMuSig2` and `SignTaproot` alongside regression coverage in `rpc_psbt.py`.

Reviewability

Paused · Waiting on author

Open objections from Bicaru20, polespinasa, vicjuma with no author reply for 31 days; review would repeat what has been asked.

Waiting on (from the thread read):

  • author: fix the test assertion in rpc_psbt.py and address reviewer suggestions 2026-09-28

Agreement

Blocked

objection that needs a change, nobody has replied (polespinasa)

Thread notes: Re-evaluated fametrano's edited comment c:5940991797; the edit adds clarification explaining to Zeegaths why participant keys lose their leaf hashes on key-path-only descriptors.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
vicjuma (contributor)suggestion, approachavoid repeated lookups of GetAllMuSig2ParticipantPubkeys on every signing attempt if possibleopenansweredno2026-09-25: 'Called on every signing attempt, is it optimal to avoid repeated lookups'
Bicaru20 (contributor)suggestion, approachavoid repeated calls to GetAllMuSig2ParticipantPubkeys by creating agg_keys in SignTaproot or using insert_or_assignopenansweredno2026-09-27: 'Create the agg_keys in SignTaproot so we only call GetAllMuSig2ParticipantPubkeys once'
Bicaru20 (contributor)suggestion, stylecheck the descriptor directly in the test loop instead of enumerating by indexopenansweredno2026-09-27: 'nit: I think it would make the test easy to follow if the condition check directly the descriptor'
polespinasa (member)objection, correctnessan assertion in test/functional/rpc_psbt.py is inverted/wrongopenchangedno2026-09-28: 'This assert is wrong'
polespinasa (member)suggestion, correctnessassert that musig2_participant_pubkeys are present in decoded psbt scopeopenansweredno2026-09-28: 'Maybe assert here than the fields are present?'

Support:

  • vicjuma (contributor) (ACK, 2026-09-25): (no reason given) [not substantive]
  • Bicaru20 (contributor) (Concept ACK, 2026-09-27) [read: sign.cpp]: agrees with concept and notes the code in sign.cpp could be optimized to avoid repeated GetAllMuSig2ParticipantPubkeys calls
  • fametrano (contributor) (2026-10-01) [tested]: tested that the fix works across the issue's descriptor and four variants; will review again once the assert polespinasa flagged is fixed

Participants:

  • vicjuma (contributor, support, 3 statements 2026-09-25 to 2026-09-25): ACKed; suggested avoiding repeated lookups of GetAllMuSig2ParticipantPubkeys
  • Bicaru20 (contributor, support, 3 statements 2026-09-27 to 2026-09-27): Concept ACK; suggested optimizations in sign.cpp and a test simplification nit
  • Zeegaths (none, neutral, 1 statements 2026-09-27 to 2026-09-27): reported RPC comparison test results between master and PR
  • polespinasa (member, objection, 2 statements 2026-09-28 to 2026-09-28): pointed out an incorrect assert in rpc_psbt.py and suggested checking for participant pubkeys
  • fametrano (contributor, support, 2 statements 2026-10-01 to 2026-10-01): tested that the fix works across five descriptor variants; waiting on test assert fix to re-review

Files

48 lines under test/bench/ci.

  • test/functional/rpc_psbt.py +48/-0
  • src/script/sign.cpp +20/-18

Card

The short text the ranking pass compares across PRs.

Prevents unrelated MuSig2 metadata from being injected into PSBT inputs and outputs. When processing PSBTs with multiple descriptors via descriptorprocesspsbt or utxoupdatepsbt, MuSig2 aggregate and participant BIP 32 derivation data were populated before verifying whether the input script actually involved that aggregate key. MuSig2 data insertion is now deferred in SignMuSig2 until after relevance is validated. This resolves issue #36323 without external dependencies.

Data

dossier JSON · extract JSON · model ledger, generated 2026-10-23T01:16, confidence high, input hash c69bf336b065bf6c
thread read through 2026-10-01 (11 statements), head bca240a0e0
labels: none

Categories

RPC / REST / ZMQ

P3 · bug fix (ranking pass: Moved up 7: injecting unrelated key-origin metadata into shared PSBTs is a data-correctness and privacy leak, above the other PSBT polish items.)

Prevents `descriptorprocesspsbt` and `utxoupdatepsbt` from leaking unrelated MuSig2 participant pubkeys, derivation paths, and leaf hashes into PSBT inputs where they do not belong (#36323). This fixes a clear data correctness issue in PSBT RPC outputs when multiple descriptors are supplied, but addresses an edge case in advanced MuSig2 signing workflows that can be deferred behind category-critical RPC work.

Membership: Fixes PSBT output data correctness for RPC methods `descriptorprocesspsbt` and `utxoupdatepsbt` without modifying wallet code, matching the PSBT RPC borderline rule.

Factors: security/stability 1, bug 2, performance 0, user value 1, leverage 0