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
furszy commented at 2:33 PM on October 12, 2026: member
utACK fac66a1b4e4cdc65ad635cbdd1dc2acf31ece35b

Not blocking thought: a multi-probe macro would be so nice. But it needs __VA_ARGS__, which is C99. Another idea to simplify this could be to use a single probe with a reset function:
```
DEFINE_SHA256_TRANSFORM_PROBE(sha256_musig)

/* Returns whether the probe ran since the last call, and resets the flag */
static int sha256_musig_take_called(void) {
    int called = sha256_musig_called;
    sha256_musig_called = 0;
    return called;
}

// Then the called checks would just need to use:
CHECK(!sha256_musig_take_called());
```
(Going further, the take + reset function could be placed in the util file with a macro name too).

But all of this is not really needed atm. Just some extra thoughts.
theStack merged this on Oct 12, 2026
theStack closed this on Oct 12, 2026
theStack commented at 8:02 PM on October 12, 2026: contributor

@furszy: seems reasonable to simplify using a take-and-reset helper (and yes, we should consider switching to C99 to enable even nicer possibilities), would be to nice to explore this in a follow-up.

Contributors

Labels

assurance tweak/refactor

Linked (view graph)

#1777 Make SHA256 compression runtime pluggable #1940 silentpayments: add hash ctx (SHA256 compression override) test #1955 tests: add hash context test for `secp256k1_tagged_sha256`