tests: check results before using outputs #1897

l0rinc commented at 11:43 PM on July 26, 2026: contributor

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.

tests: check exhaustive ecmult success

The exhaustive matrix compared the Jacobian output without requiring `secp256k1_ecmult_multi_var` to succeed.
Require success before consuming the result so a failure cannot satisfy an infinity case accidentally.

658c7edc24

ellswift: check test operation results

The EllSwift tests inspected outputs without requiring encoding, decoding, or public-key loading to succeed.
They also discarded the field parser result despite relying on its no-overflow precondition.
Check each deterministic success result before inspecting its output.

564afb0b06

recovery: check exhaustive API results

The exhaustive recovery tests decoded and verified signatures without requiring recoverable signing or conversion to succeed.
Check those documented success results before consuming the outputs, matching the ordinary exhaustive signing test.

1d3f72d3fa

extrakeys: check test pubkey loads

The x-only parity test compared fields from two loaded points without requiring either load to succeed.
Check both setup operations before reading their outputs.

0618af8131

musig: check infinity test setup

`pubnonce_summing_to_inf` initializes both sums to infinity before loading the nonce objects.
A load failure could therefore satisfy the expected infinity checks without validating the constructed pair.
Require both nonce-loading operations to succeed before testing their outputs.

b8de1bc30f

silentpayments: check test serialization

The sender vector test compared serialized x-only public keys without requiring serialization to succeed.
Check success before comparing the output bytes.

93280c2291

ec: check pubkey sort test results

Two successful sort paths validated only the reordered arrays and discarded the API result.
Check success before inspecting the sorted output.

a2ad68cd81

real-or-random approved

real-or-random commented at 7:31 AM on July 27, 2026: contributor

Weak Concept ACK -- I think sometimes there's also something to having the CHECKs only in the "primary" targets of a test case because this makes the code self-documenting and thus more readable. But when it comes to semantics, CHECKing more can't hurt and the earlier an abort, the easier it is to identify the cause of a failing test.

utACK a2ad68cd81787ba70b2caff0a57998ae8a5b4834

in src/modules/ellswift/tests_impl.h:311 in a2ad68cd81

 309 |           * pass ell64 as claimed (but incorrect) encoding for sec32 here; this works
 310 |           * because the "hasher" function we use here ignores the ell64 arguments. */
 311 |          ret = secp256k1_ellswift_xdh(CTX, share32, ell64, ell64, sec32, i & 1, &ellswift_xdh_hash_x32, NULL);
 312 |          CHECK(ret);
 313 | -        (void)secp256k1_fe_set_b32_limit(&share_x, share32); /* no overflow is possible */
 314 | +        CHECK(secp256k1_fe_set_b32_limit(&share_x, share32)); /* no overflow is possible */

real-or-random commented at 7:32 AM on July 27, 2026:

If you touch this again:

        CHECK(secp256k1_fe_set_b32_limit(&share_x, share32) == 1); /* no overflow is possible */

If this is done everywhere, we should also do it here.

151henry151 commented at 2:50 AM on July 29, 2026: none

Tested ACK a2ad68cd81787ba70b2caff0a57998ae8a5b4834

Walked all seven commits. Musig and exhaustive ecmult_multi_var are real initialized-infinity false-passes; the other CHECKs are expected-success setup (failure would not satisfy the later asserts the same way). No obvious sibling miss in the touched files.

Tested with: ./configure --enable-experimental --enable-module-ellswift --enable-module-extrakeys --enable-module-recovery --enable-module-schnorrsig --enable-module-musig --enable-module-silentpayments make -j$(nproc) check — passed

real-or-random added the label assurance on Jul 29, 2026

real-or-random added the label tweak/refactor on Jul 29, 2026