← Watch feed
Merge bitcoin-core/secp256k1#1897: tests: check results before using outputs
Public commit record
96/100 · Strong
What the developer wrote
Authored by merge-script
Merge bitcoin-core/secp256k1#1897: tests: check results before using outputs
a2ad68cd81787ba70b2caff0a57998ae8a5b4834 ec: check pubkey sort test results (Lőrinc)
93280c22917c018f4e99d76e4398e66405c887bd silentpayments: check test serialization (Lőrinc)
b8de1bc30f917c0ab9f05aeaeb494834e81513e2 musig: check infinity test setup (Lőrinc)
0618af81317307466e9968592880a54229885edc extrakeys: check test pubkey loads (Lőrinc)
1d3f72d3fa8e476cdc3344dbb5aebeaf7a6efe89 recovery: check exhaustive API results (Lőrinc)
564afb0b06e6124092fd6fa1239b1b397f742e41 ellswift: check test operation results (Lőrinc)
658c7edc24832592969ccbd1bdbc5c59e1ccc5d5 tests: check exhaustive ecmult success (Lőrinc)
Pull request description:
**Problem:** Several tests inspect outputs from deterministic operations without checking the result.
The later output comparisons usually still exercise behavior, but a failed setup call can leave an output that accidentally satisfies a later assertion.
In `pubnonce_summing_to_inf`, `secp256k1_musig_sum_pubnonces` initializes both sums to infinity before a pubnonce load can fail, so the test could pass without constructing a valid opposing-nonce pair.
**Fix:** Check the result of each deterministic operation before reading its output in the affected exhaustive, EllSwift, recovery, extrakeys, MuSig, silent-payments, and public-key sort tests.
Leave deliberately ignored results alone when the test permits either result or validates a mutated value rather than an auxiliary carry, overflow, or sign flag.
ACKs for top commit:
151henry151:
Tested ACK a2ad68cd81787ba70b2caff0a57998ae8a5b4834
real-or-random:
utACK a2ad68cd81787ba70b2caff0a57998ae8a5b4834
theStack:
ACK a2ad68cd81787ba70b2caff0a57998ae8a5b4834
Tree-SHA512: 9955e9170c53d87cd851351234133660d5d4a26190bd9054b5bef75ed11c785a0168f92ca9ec634d9599da019dad473dc30e4f635eeef699abc39407a5e28c5c
✓ Specific, descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context✓ Mentions testing or verification✓ Links an issue, advisory, or supporting reference✓ Names security-relevant behavior explicitly
What changed, and why it matters
This commit fixes test-suite bugs, not the cryptographic library itself. Several test cases were using outputs from functions without first checking whether those functions succeeded. In rare cases a failed setup step could leave a value that accidentally passed a later check, making the test look successful when it shouldn't. The patch adds result checks so tests fail loudly if a setup step fails. There is no direct security risk to users of the library.
Recommended action
No urgent action for downstream users. Developers should ensure CI runs the updated tests. The change is safe to include in normal updates.
Security signals we found
01
Test-only hardening
02
Missing return-value checks in test code
03
Potential false-positive test passes on setup failure
04
No production code changes
Evidence from the diff
The merge adds CHECK(…) wrappers around deterministic API calls in test code for EllSwift, extrakeys, MuSig, recovery, silent payments, pubkey sort, and exhaustive ecmult. The only behavioral change is in tests; no production code is modified. The PR description notes one concrete case: in pubnonce_summing_to_inf, sums were initialized to infinity before a pubnonce load could fail, so the infinity checks could pass even if no valid opposing-nonce pair was constructed. The fix ensures test setup failures are caught rather than potentially masked by downstream assertions.
Changed components
src/modules/ellswift/tests_exhaustive_impl.hsrc/modules/ellswift/tests_impl.hsrc/modules/extrakeys/tests_impl.hsrc/modules/musig/tests_impl.hsrc/modules/recovery/tests_exhaustive_impl.hsrc/modules/silentpayments/tests_impl.hsrc/tests.csrc/tests_exhaustive.cInspect captured patch +21 / −21
### src/modules/ellswift/tests_exhaustive_impl.h
@@ -30,8 +30,8 @@ static void test_exhaustive_ellswift(const secp256k1_context *ctx, const secp256
CHECK(secp256k1_ellswift_create(ctx, ell64, sec32, NULL));
/* Decode ellswift pubkey and check that it matches the precomputed group element. */
- secp256k1_ellswift_decode(ctx, &pub_decoded, ell64);
- secp256k1_pubkey_load(ctx, &ge_decoded, &pub_decoded);
+ CHECK(secp256k1_ellswift_decode(ctx, &pub_decoded, ell64) == 1);
+ CHECK(secp256k1_pubkey_load(ctx, &ge_decoded, &pub_decoded) == 1);
CHECK(secp256k1_ge_eq_var(&ge_decoded, &group[i]));
}
}
### src/modules/ellswift/tests_impl.h
@@ -249,9 +249,9 @@ void ellswift_encode_decode_roundtrip_tests(void) {
secp256k1_pubkey_save(&pubkey, &g);
testrand256(rnd32);
/* Convert the public key to ElligatorSwift and back. */
- secp256k1_ellswift_encode(CTX, ell64, &pubkey, rnd32);
- secp256k1_ellswift_decode(CTX, &pubkey2, ell64);
- secp256k1_pubkey_load(CTX, &g2, &pubkey2);
+ CHECK(secp256k1_ellswift_encode(CTX, ell64, &pubkey, rnd32) == 1);
+ CHECK(secp256k1_ellswift_decode(CTX, &pubkey2, ell64) == 1);
+ CHECK(secp256k1_pubkey_load(CTX, &g2, &pubkey2) == 1);
/* Compare with original. */
CHECK(secp256k1_ge_eq_var(&g, &g2));
}
@@ -276,8 +276,8 @@ void ellswift_create_tests(void) {
ret = secp256k1_ellswift_create(CTX, ell64, sec32, (i & 1) ? auxrnd32 : NULL);
CHECK(ret);
/* Decode it, and compare with traditionally-computed public key. */
- secp256k1_ellswift_decode(CTX, &pub, ell64);
- secp256k1_pubkey_load(CTX, &dec, &pub);
+ CHECK(secp256k1_ellswift_decode(CTX, &pub, ell64) == 1);
+ CHECK(secp256k1_pubkey_load(CTX, &dec, &pub) == 1);
secp256k1_ecmult(&res, NULL, &secp256k1_scalar_zero, &sec);
CHECK(secp256k1_gej_eq_ge_var(&res, &dec));
}
@@ -300,15 +300,15 @@ void ellswift_compute_shared_secret_tests(void) {
/* Generate random ElligatorSwift encoding for the remote key and decode it. */
testrand256_test(ell64);
testrand256_test(ell64 + 32);
- secp256k1_ellswift_decode(CTX, &pub, ell64);
- secp256k1_pubkey_load(CTX, &dec, &pub);
+ CHECK(secp256k1_ellswift_decode(CTX, &pub, ell64) == 1);
+ CHECK(secp256k1_pubkey_load(CTX, &dec, &pub) == 1);
secp256k1_gej_set_ge(&decj, &dec);
/* Compute the X coordinate of seckey*pubkey using ellswift_xdh. Note that we
* pass ell64 as claimed (but incorrect) encoding for sec32 here; this works
* because the "hasher" function we use here ignores the ell64 arguments. */
ret = secp256k1_ellswift_xdh(CTX, share32, ell64, ell64, sec32, i & 1, &ellswift_xdh_hash_x32, NULL);
CHECK(ret);
- (void)secp256k1_fe_set_b32_limit(&share_x, share32); /* no overflow is possible */
+ CHECK(secp256k1_fe_set_b32_limit(&share_x, share32)); /* no overflow is possible */
SECP256K1_FE_VERIFY(&share_x);
/* Compute seckey*pubkey directly. */
secp256k1_ecmult(&resj, &decj, &sec, NULL);
### src/modules/extrakeys/tests_impl.h
@@ -53,8 +53,8 @@ static void test_xonly_pubkey(void) {
CHECK(secp256k1_xonly_pubkey_from_pubkey(CTX, &xonly_pk, &pk_parity, &pk) == 1);
CHECK(secp256k1_memcmp_var(&xonly_pk, &pk, sizeof(xonly_pk)) != 0);
CHECK(pk_parity == 1);
- secp256k1_pubkey_load(CTX, &pk1, &pk);
- secp256k1_pubkey_load(CTX, &pk2, (secp256k1_pubkey *) &xonly_pk);
+ CHECK(secp256k1_pubkey_load(CTX, &pk1, &pk) == 1);
+ CHECK(secp256k1_pubkey_load(CTX, &pk2, (secp256k1_pubkey *) &xonly_pk) == 1);
CHECK(secp256k1_fe_equal(&pk1.x, &pk2.x) == 1);
secp256k1_fe_negate(&y, &pk2.y, 1);
CHECK(secp256k1_fe_equal(&pk1.y, &y) == 1);
### src/modules/musig/tests_impl.h
@@ -104,7 +104,7 @@ static void pubnonce_summing_to_inf(secp256k1_musig_pubnonce *pubnonce) {
secp256k1_ge_neg(&ge[1], &ge[1]);
}
- secp256k1_musig_sum_pubnonces(CTX, summed_pubnonces, pubnonce_ptr, 2);
+ CHECK(secp256k1_musig_sum_pubnonces(CTX, summed_pubnonces, pubnonce_ptr, 2) == 1);
CHECK(secp256k1_gej_is_infinity(&summed_pubnonces[0]));
CHECK(secp256k1_gej_is_infinity(&summed_pubnonces[1]));
}
@@ -372,7 +372,7 @@ static void musig_api_tests(void) {
{
/* Check that the aggnonce encodes two points at infinity */
secp256k1_ge aggnonce_pt[2];
- secp256k1_musig_aggnonce_load(CTX, aggnonce_pt, &aggnonce);
+ CHECK(secp256k1_musig_aggnonce_load(CTX, aggnonce_pt, &aggnonce) == 1);
for (i = 0; i < 2; i++) {
CHECK(secp256k1_ge_is_infinity(&aggnonce_pt[i]) == 1);
}
### src/modules/recovery/tests_exhaustive_impl.h
@@ -33,7 +33,7 @@ static void test_exhaustive_recovery_sign(const secp256k1_context *ctx, const se
secp256k1_scalar_get_b32(sk32, &sk);
secp256k1_scalar_get_b32(msg32, &msg);
- secp256k1_ecdsa_sign_recoverable(ctx, &rsig, msg32, sk32, secp256k1_nonce_function_smallint, &k);
+ CHECK(secp256k1_ecdsa_sign_recoverable(ctx, &rsig, msg32, sk32, secp256k1_nonce_function_smallint, &k) == 1);
/* Check directly */
secp256k1_ecdsa_recoverable_signature_load(ctx, &r, &s, &recid, &rsig);
@@ -59,7 +59,7 @@ static void test_exhaustive_recovery_sign(const secp256k1_context *ctx, const se
CHECK(recid == expected_recid);
/* Convert to a standard sig then check */
- secp256k1_ecdsa_recoverable_signature_convert(ctx, &sig, &rsig);
+ CHECK(secp256k1_ecdsa_recoverable_signature_convert(ctx, &sig, &rsig) == 1);
secp256k1_ecdsa_signature_load(ctx, &r, &s, &sig);
/* Note that we compute expected_r *after* signing -- this is important
* because our nonce-computing function function might change k during
@@ -129,7 +129,7 @@ static void test_exhaustive_recovery_verify(const secp256k1_context *ctx, const
/* Verify by converting to a standard signature and calling verify */
secp256k1_ecdsa_recoverable_signature_save(&rsig, &r_s, &s_s, recid);
- secp256k1_ecdsa_recoverable_signature_convert(ctx, &sig, &rsig);
+ CHECK(secp256k1_ecdsa_recoverable_signature_convert(ctx, &sig, &rsig) == 1);
memcpy(&nonconst_ge, &group[sk_s], sizeof(nonconst_ge));
secp256k1_pubkey_save(&pk, &nonconst_ge);
CHECK(should_verify ==
### src/modules/silentpayments/tests_impl.h
@@ -138,7 +138,7 @@ static void test_recipient_sort_helper(unsigned char (*sp_addresses[3])[2][33],
);
CHECK(ret == 1);
for (i = 0; i < 3; i++) {
- secp256k1_xonly_pubkey_serialize(CTX, xonly_ser, &generated_outputs[i]);
+ CHECK(secp256k1_xonly_pubkey_serialize(CTX, xonly_ser, &generated_outputs[i]) == 1);
CHECK(secp256k1_memcmp_var(xonly_ser, (*sp_outputs[i]), 32) == 0);
}
}
### src/tests.c
@@ -6944,7 +6944,7 @@ static void test_sort_helper(secp256k1_pubkey *pk, size_t *pk_order, size_t n_pk
for (i = 0; i < n_pk; i++) {
pk_test[i] = &pk[pk_order[i]];
}
- secp256k1_ec_pubkey_sort(CTX, pk_test, n_pk);
+ CHECK(secp256k1_ec_pubkey_sort(CTX, pk_test, n_pk) == 1);
for (i = 0; i < n_pk; i++) {
CHECK(secp256k1_memcmp_var(pk_test[i], &pk[i], sizeof(*pk_test[i])) == 0);
}
@@ -7033,7 +7033,7 @@ static void test_sort(void) {
testutil_random_pubkey_test(&pk[j]);
pk_ptr[j] = &pk[j];
}
- secp256k1_ec_pubkey_sort(CTX, pk_ptr, 5);
+ CHECK(secp256k1_ec_pubkey_sort(CTX, pk_ptr, 5) == 1);
for (j = 1; j < 5; j++) {
CHECK(secp256k1_ec_pubkey_sort_cmp(&pk_ptr[j - 1], &pk_ptr[j], CTX) <= 0);
}
### src/tests_exhaustive.c
@@ -220,7 +220,7 @@ static void test_exhaustive_ecmult_multi(const secp256k1_context *ctx, const sec
data.pt[0] = group[x];
data.pt[1] = group[y];
- secp256k1_ecmult_multi_var(&ctx->error_callback, scratch, &tmp, &g_sc, ecmult_multi_callback, &data, 2);
+ CHECK(secp256k1_ecmult_multi_var(&ctx->error_callback, scratch, &tmp, &g_sc, ecmult_multi_callback, &data, 2) == 1);
CHECK(secp256k1_gej_eq_ge_var(&tmp, &group[(i * x + j * y + k) % EXHAUSTIVE_TEST_ORDER]));
}
}Risk score
Our methodology →Why this scored 20/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.