#35422 musig: Require generated secnonce for partial sig
https://github.com/bitcoin/bitcoin/pull/35422 · · +50/-8 in 4 files, 1 commits · labels: none
Goal
- Prevent crashes and undefined behavior when partial signing with uninitialized MuSig2 nonces
- Ensure nonce validity checks accurately reflect whether generation actually succeeded
Defers allocation of the underlying secure memory for `MuSig2SecNonce` until `secp256k1_musig_nonce_gen` succeeds so that `IsValid()` accurately reflects a generated nonce. Additionally guards `CreateMuSig2PartialSig` to fail early if an uninitialized nonce is supplied, and adds regression test coverage for the nonce lifecycle.
Problem: Previously, `MuSig2SecNonce` pre-allocated memory upon construction, making `IsValid()` return true before nonce generation took place. Attempting a partial sign with an uninitialized nonce caused libsecp256k1 to crash or invoke undefined behavior.
Category: Utilities (logging, arguments, libraries) (#10 of 66)
P3 · crash fix
- P3 because it prevents crashes from uninitialized nonces in MuSig2 signing helpers
- P3 because it addresses an edge case found via fuzzing rather than standard usage
- P3 because it hardens cryptographic wrapper invariants against misuse
Prevents crashes or undefined behavior when partial signing with uninitialized MuSig2 nonces. While existing production code paths do not typically trigger this, the bug was caught during fuzzing, and enforcing proper lifecycle invariants improves cryptographic API safety.
Membership: Touches MuSig2 cryptographic wrappers and signing helpers in src/musig.cpp and src/musig.h.
Factors: security/stability 1, bug 1, performance 0, user value 0, leverage 1
Reviewability: Ready
- Ready for review
- Reviewer feedback has been addressed and no blockers remain
The author incorporated reviewer feedback, tests pass, and no blockers are open.
Author status: Active; addressed review feedback and force-pushed updates.
Resolved concerns:
- achow101 pointed out normal execution should not store failed nonces and suggested setting the unique_ptr in CreateMuSig2Nonce rather than maintaining extra state; author adopted this approach.
- real-or-random clarified that libsecp crashes on uninitialized structs because they are invalid representations rather than due to BIP 327 64-byte zero checks; author updated the PR motivation accordingly.
Agreement: Neutral
- Technical clarifications on libsecp nonce handling addressed (real-or-random)
- Suggested cleanup to defer nonce allocation was incorporated (achow101)
- No formal ACKs posted yet
Reviewer comments from achow101 and real-or-random were addressed by the author, but no formal ACKs have been posted yet.
Reviewers provided architectural suggestions and technical clarifications rather than outright concept approval, and the author addressed all feedback in subsequent pushes without further objections.
- achow101 suggested setting the unique_ptr after successful nonce generation instead of tracking boolean state.
- real-or-random clarified libsecp256k1 expectations regarding valid secnonce pointers.
- nervana21 updated the PR to incorporate achow101's design and real-or-random's clarifications.
Review verdicts (DrahtBot): 0
Files
30 lines under test/bench/ci.
- src/test/bip328_tests.cpp +30/-0
- src/musig.cpp +13/-5
- src/musig.h +6/-2
- src/script/sign.cpp +1/-1
Card
This pull request ensures that MuSig2 secret nonces allocate secure memory and report as valid only after successful nonce generation, preventing uninitialized nonces from being passed to libsecp256k1 partial signing functions. Passing an uninitialized nonce previously triggered a crash or undefined behavior in libsecp256k1, which was surfaced during fuzz testing. Reviewers discussed the correct abstraction boundaries and suggested creating the secure unique_ptr upon nonce generation, which the author implemented. The code is ready for review and awaiting re-review from commenters.