fuzz: Add musig harness #36073

nervana21 commented at 8:02 AM on August 25, 2026: contributor

This PR adds a fuzz target for src/musig. Previously, the module APIs had no dedicated harness, so fail paths were underexercised.

It covers:

MuSig2AggregatePubkeys
MuSig2AggregatePubkeys (with key aggregation cache and optional expected aggregate)
CreateMuSig2SyntheticXpub
MuSig2SessionID
Signing-shaped path:
- CreateMuSig2Nonce
- CreateMuSig2PartialSig with the generated secnonce
- CreateMuSig2AggregateSig

DrahtBot added the label Fuzzing on Aug 25, 2026

DrahtBot commented at 8:02 AM on August 25, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36073.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	jeanpablojp

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

nervana21 commented at 8:03 AM on August 25, 2026: contributor

Coverage: https://nervana21.github.io/coverage/musig/ llvm-cov on 2bea78212c1047671ea93b93d345e0aea9042a69 (includes PR35422).

jeanpablojp commented at 12:35 PM on September 8, 2026: contributor

Concept ACK.

The four signing functions in the module have no unit test today, only the wallet_musig.py functional test, so the harness covers ground that was empty.

One thing about the never-generated secnonce arm. I built the target at this head and ran it against random input. The target aborts on 8 of 1000 files, with [libsecp256k1] illegal argument: secp256k1_memcmp_var(&secnonce->data[0], secp256k1_musig_secnonce_magic, 4) == 0. Dropping just that arm makes the same files pass clean.

The 60 second CI run ends at lim: 8, and 2000 inputs of up to 8 bytes did not abort the target once. oss-fuzz picks its targets out of the binary itself, so this target goes in with the next oss-fuzz build after the merge.

Would it be worth moving that arm into #35422, where it gets the guard that makes it meaningful, and letting this harness go in on its own?

fuzz: Add musig harness 13956bfd64

in src/test/fuzz/musig.cpp:139 in da584ef92c outdated

 134 | +                for (const CPubKey& pubkey : pubkeys) {
 135 | +                    if (pubkey == our_pubkey) {
 136 | +                        pubnonces[pubkey] = our_pubnonce;
 137 | +                    } else {
 138 | +                        // Sized bytes, not generated nonces.
 139 | +                        pubnonces[pubkey] = fdp.ConsumeBytes<uint8_t>(MUSIG2_PUBNONCE_SIZE);

jeanpablojp commented at 12:35 PM on September 8, 2026:

These bytes are almost never a valid pair of nonce points, so a session with more than one participant almost always stops before signing.

Instrumenting CreateMuSig2PartialSig and dropping the secnonce arm so the run survives, a 300 second libFuzzer run made 98213 calls, 75903 of them single-participant. 230 of the 6135 two-participant calls returned a signature, and none of the 16175 with three or more. CreateMuSig2AggregateSig never returned a signature.

Giving each participant a key from the fuzz input and a nonce from CreateMuSig2Nonce, and feeding the partial sigs back into the aggregation instead of fresh ConsumeUInt256 values, plain random inputs gave 400 partial sigs and 145 aggregate signatures over one to five participants. Worth going that way and keeping the raw bytes as a separate case, so the parse failure stays reachable?

nervana21 commented at 10:40 PM on September 8, 2026:

Ahh, makes sense, thanks! I think I've understood and taken your suggestions. Please let me know what you think.

in src/test/fuzz/musig.cpp:83 in da584ef92c

  78 | +                if (fdp.ConsumeBool()) {
  79 | +                    expected = MuSig2AggregatePubkeys(pubkeys);
  80 | +                    // Plant expected != agg so the mismatch return stays reachable.
  81 | +                    if (expected.has_value() && fdp.ConsumeBool()) {
  82 | +                        std::vector<unsigned char> ser{expected->begin(), expected->end()};
  83 | +                        if (!ser.empty()) {

jeanpablojp commented at 12:35 PM on September 8, 2026:

This ser only exists when expected has a value, and then it comes from the aggregate pubkey, which is always 33 bytes. Could the check go?

nervana21 commented at 2:26 PM on September 8, 2026:

Done.

in src/test/fuzz/CMakeLists.txt:74 in da584ef92c

  70 | @@ -71,6 +71,7 @@ add_executable(fuzz
  71 |    minisketch.cpp
  72 |    mini_miner.cpp
  73 |    muhash.cpp
  74 | +  musig.cpp

jeanpablojp commented at 12:35 PM on September 8, 2026:

nit: the list is almost entirely alphabetical, so musig.cpp would go after multiplication_overflow.cpp.

nervana21 commented at 1:00 PM on September 8, 2026:

Done.

nervana21 commented at 10:41 PM on September 8, 2026: contributor

Would it be worth moving that arm into #35422, where it gets the guard that makes it meaningful, and letting this harness go in on its own?

Yes, I think this is a better approach. I've dropped that call from this harness.

nervana21 force-pushed on Sep 8, 2026

nervana21 commented at 10:58 PM on September 8, 2026: contributor

Updates:

Addressed @jeanpablojp's feedback
Addressed @jeanpablojp's feedback. Also in the raw bytes branch, moved the tweak list to before CreateMuSig2Nonce to match the ordering of the newly added arm.
Addressed @jeanpablojp's feedback
Addressed @jeanpablojp's feedback