Merge bitcoin-core/secp256k1#1947: musig: test nonce_gen_counter with random counters
What the developer wrote
Authored by merge-script
Merge bitcoin-core/secp256k1#1947: musig: test nonce_gen_counter with random counters
b819a790f06122d5a53c0320e79c0dc486349fbd musig: test nonce_gen_counter with random counters (ViniciusCestarii)
Pull request description:
The only output check of `nonce_gen_counter` uses `nonrepeating_cnt = 0`, so the following mutant replacing `secp256k1_write_be64` with `secp256k1_write_be32` (which could enable nonce reuse) survived because zero serializes the same either way.
```diff
diff --git a/src/modules/musig/session_impl.h b/src/modules/musig/session_impl.h
index d8a3cca..710c4c5 100644
--- a/src/modules/musig/session_impl.h
+++ b/src/modules/musig/session_impl.h
@@ -447,7 +447,7 @@ int secp256k1_musig_nonce_gen_counter(const secp256k1_context* ctx, secp256k1_mu
memset(secnonce, 0, sizeof(*secnonce));
ARG_CHECK(keypair != NULL);
- secp256k1_write_be64(buf, nonrepeating_cnt);
+ secp256k1_write_be32(buf, nonrepeating_cnt);
/* keypair_sec and keypair_pub do not fail if the arguments are not NULL */
ret = secp256k1_keypair_sec(ctx, seckey, keypair);
VERIFY_CHECK(ret);
```
This add a new test, which checks that for random 64-bit counters, `nonce_gen_counter` gives the same nonce as `nonce_gen`.
ACKs for top commit:
real-or-random:
utACK b819a790f06122d5a53c0320e79c0dc486349fbd
Tree-SHA512: 1f16375880808eacea1f4db9931f345f2f1b44a2724d61710beb5e39e956d4f28bd6707c90b8300f033282bee207fd2f44c039cb01ef0675858d415ad6ea2978
What changed, and why it matters
This commit only adds a new test to the MuSig cryptographic module. It does not change any production code. The new test verifies that two different nonce-generation functions produce matching results when given equivalent random 64-bit counters. The pull request description mentions a hypothetical mutation (replacing a 64-bit write with a 32-bit write) that could survive testing if only zero-valued counters were used, but that mutation is not present in the code and is only described as motivation for stronger testing.
No security action required. This is a test-hardening change. Reviewers may optionally verify that the new test correctly exercises the 64-bit counter serialization path and that the existing `secp256k1_write_be64` call in `session_impl.h` remains intact.
Security signals we found
MuSig nonce generation is security-critical; nonce reuse can leak private keys.
The PR description discusses a mutation that could enable nonce reuse, but the mutation is not in the committed code.
The change is test-only; no production cryptographic logic is altered.
Evidence from the diff
The diff adds musig_nonce_gen_counter_test() in src/modules/musig/tests_impl.h and registers it in the MuSig test suite. The test loops COUNT times, each iteration generating a random 64-bit nonrepeating_cnt, serializing it with secp256k1_write_be64(session_secrand32, nonrepeating_cnt), and then comparing the public nonces produced by secp256k1_musig_nonce_gen_counter(..., nonrepeating_cnt, ...) and secp256k1_musig_nonce_gen(..., session_secrand32, sk, &pk, ...). No implementation code in session_impl.h or elsewhere is modified. The PR description references a mutant that would replace secp256k1_write_be64 with secp256k1_write_be32 as the reason the test is needed, but the commit itself is purely a test addition.
Changed components
src/modules/musig/tests_impl.hInspect captured patch +24 / −0
### src/modules/musig/tests_impl.h
@@ -1138,6 +1138,29 @@ static void musig_test_static_nonce_gen_counter(void) {
CHECK(secp256k1_memcmp_var(pubnonce66, expected_pubnonce, sizeof(pubnonce66)) == 0);
}
+/* Checks that nonce_gen_counter matches nonce_gen */
+static void musig_nonce_gen_counter_test(void) {
+ secp256k1_musig_secnonce secnonce;
+ secp256k1_musig_pubnonce pubnonce[2];
+ secp256k1_keypair keypair;
+ secp256k1_pubkey pk;
+ unsigned char sk[32];
+ int i;
+
+ testrand256(sk);
+ CHECK(create_keypair_and_pk(&keypair, &pk, sk));
+
+ for (i = 0; i < COUNT; i++) {
+ unsigned char session_secrand32[32] = { 0 };
+ uint64_t nonrepeating_cnt = testrand64();
+
+ secp256k1_write_be64(session_secrand32, nonrepeating_cnt);
+ CHECK(secp256k1_musig_nonce_gen_counter(CTX, &secnonce, &pubnonce[0], nonrepeating_cnt, &keypair, NULL, NULL, NULL) == 1);
+ CHECK(secp256k1_musig_nonce_gen(CTX, &secnonce, &pubnonce[1], session_secrand32, sk, &pk, NULL, NULL, NULL) == 1);
+ CHECK(secp256k1_memcmp_var(&pubnonce[0], &pubnonce[1], sizeof(pubnonce[0])) == 0);
+ }
+}
+
/* --- Test registry --- */
REPEAT_TEST(musig_simple_test)
/* Run multiple times to ensure that pk and nonce have different y parities */
@@ -1156,6 +1179,7 @@ static const struct tf_test_entry tests_musig[] = {
CASE1(musig_test_vectors_tweak),
CASE1(musig_test_vectors_sigagg),
CASE1(musig_test_static_nonce_gen_counter),
+ CASE1(musig_nonce_gen_counter_test),
};
#endifWhy this scored 12/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.