musig: Add hash context tests #1866

pull mllwchrry wants to merge 1 commits into bitcoin-core:master from mllwchrry:musig-hash-ctx-tests changing 1 files +92 −0

mllwchrry commented at 5:44 PM on June 9, 2026: contributor

Adds missing hash context tests for the musig module, covering thepubkey_agg, nonce_gen, and nonce_process functions. Follows the same pattern as the hash context tests for other modules added in #1777.
real-or-random added the label assurance on Jun 10, 2026
real-or-random added the label tweak/refactor on Jun 10, 2026
real-or-random removed the label tweak/refactor on Jun 10, 2026
real-or-random added the label tweak/refactor on Jun 10, 2026
real-or-random commented at 11:34 AM on June 10, 2026: contributor

@furszy Want to review this?

in src/modules/musig/tests_impl.h:625 in e0b1f34ca0

 620 | +
 621 | +    CHECK(secp256k1_ec_pubkey_create(ctx, &pk, sk));
 622 | +    pks[0] = &pk;
 623 | +
 624 | +    /* Default behavior. No ctx-provided SHA256 compression */
 625 | +    CHECK(secp256k1_musig_pubkey_agg(ctx, NULL, &cache_default, pks, 1));

theStack commented at 1:10 PM on August 24, 2026:

nit, for a slightly more precise check, here and for other CHECK instances:

    CHECK(secp256k1_musig_pubkey_agg(ctx, NULL, &cache_default, pks, 1) == 1);

(see also PR #1481 which enforces that)

theStack approved
theStack commented at 1:21 PM on August 24, 2026: contributor

ACK e0b1f34ca0203905779ecf2b518bf9666c40c787

Note that there are three more musig API functions that use the hash context, so adding tests for them would make sense, either here or in a separate PR: _musig_nonce_gen_counter (could probably tested alongside of the existing _musig_nonce_gen) and _musig_partial_{sign,sig_verify}.
furszy commented at 2:54 PM on August 24, 2026: member

ACK e0b1f34ca0203905779ecf2b518bf9666c40c787
mllwchrry force-pushed on Aug 25, 2026
mllwchrry commented at 10:57 AM on August 25, 2026: contributor

Thanks for the reviews! I applied a == 1 check to other CHECKs and added tests for nonce_gen_counter, partial_sign, and partial_sig_verify.
in src/modules/musig/tests_impl.h:810 in 6ccc27b7a3
```
 805 | +    ctx->hash_ctx.fn_sha256_compression = sha256_musig_partial_sig_verify;
 806 | +    CHECK(secp256k1_musig_partial_sig_verify(ctx, &partial_sig, &pubnonce, &pk, &keyagg_cache, &session) == 0);
 807 | +    CHECK(sha256_musig_partial_sig_verify_called);
 808 | +
 809 | +    secp256k1_context_destroy(ctx);
 810 | +}
```
furszy commented at 6:55 PM on August 25, 2026:

These two tests can be merged, they share the same setup. You can just call the first sign function, then set the called flag to false and call the verify one.

And you could apply the same concept for the others as well. Merging sha256_musig_nonce_process within this shouldn't be hard either.
mllwchrry force-pushed on Aug 26, 2026
mllwchrry commented at 1:54 PM on August 26, 2026: contributor

Thanks @furszy! That's a great idea. Consolidated all the flow tests into one musig_ctx_sha256 that builds the session once, keeping nonce_gen_counter separate as it isn't part of the sign/verify flow.
theStack approved
theStack commented at 2:14 PM on September 16, 2026: contributor

ACK ab6925b8ebbced112e30bf009f5958c0173eebb1

nit: if you need/want to retouch, could still fold the _nonce_gen_counter test inside the large one (so the "check that ... override takes effect in every musig function" comment fully applies)
musig: add hash context tests fac66a1b4e
mllwchrry force-pushed on Sep 16, 2026
mllwchrry commented at 2:37 PM on September 16, 2026: contributor

Retouched, _nonce_gen_counter is now part of the big test.
theStack approved
theStack commented at 3:05 PM on September 16, 2026: contributor

ACK fac66a1b4e4cdc65ad635cbdd1dc2acf31ece35b

Contributors

Labels

assurance tweak/refactor

Linked (view graph)

#1777 Make SHA256 compression runtime pluggable #1940 silentpayments: add hash ctx (SHA256 compression override) test