tests: Improve secp256k1_scalar_check_overflow tests (Issue #1812) #1819

pull therohityadav wants to merge 1 commits into bitcoin-core:master from therohityadav:test-overflow changing 1 files +50 −9

therohityadav commented at 7:51 AM on February 4, 2026: contributor
This Pull Request improves the tests for secp256k1_scalar_check_overflow as requested in #1812.

Changes:
- Removed the redundant "all ones" check from run_scalar_tests.
- Added a new dedicated test function test_scalar_check_overflow.
- Added static checks for edge cases: 0, N-1, N, N+1, and MAX.
- Added random input tests that verify check_overflow against a manual byte comparison.
Fixes #1812.

in src/tests.c:2248 in a27e7512a7

2243 | +    CHECK(secp256k1_scalar_check_overflow(&max) == 1);
2244 | +
2245 | +    /* Random inputs */
2246 | +    for (i = 0; i < 256; i++) {
2247 | +        int expected_overflow;
2248 | +

hebasto commented at 9:13 AM on February 4, 2026:

in src/tests.c:2257 in a27e7512a7

2252 | +            b32[j] = (seed >> 24) & 0xFF;
2253 | +        }
2254 | +
2255 | +        /* Force top bits to be 0xFF sometimes to ensure we hit overflows */
2256 | +        if (i % 2 == 0) {
2257 | +            memset(b32, 0xFF, 16);

hebasto commented at 9:14 AM on February 4, 2026:

            memset(b32, 0xFF, 16);

hebasto commented at 9:14 AM on February 4, 2026: member

Trailing spaces should be removed to pass the CI.
therohityadav commented at 10:52 AM on February 4, 2026: contributor

Trailing spaces should be removed to pass the CI.

Thank you, I have removed the trailing whitespace and all the tests passed.

in src/tests.c:2222 in 58be14be2d outdated

2217 | +    int overflow = 0;
2218 | +    unsigned char b32[32];
2219 | +    static unsigned int seed = 5381; /* Simple RNG seed */
2220 | +
2221 | +    /* The group order N */
2222 | +    const unsigned char n_bytes[32] = {

real-or-random commented at 11:00 AM on February 4, 2026:

There's already secp256k1_group_order_bytes in testutil.h.

in src/tests.c:2253 in 58be14be2d outdated

2248 | +
2249 | +        /* Generate random 32 bytes using a simple Linear Congruential Generator */
2250 | +        for (j = 0; j < 32; j++) {
2251 | +            seed = seed * 1664525 + 1013904223;
2252 | +            b32[j] = (seed >> 24) & 0xFF;
2253 | +        }

real-or-random commented at 11:02 AM on February 4, 2026:

See again testutil.h where we have functions to generate random bytes for the tests.

in src/tests.c:2243 in 58be14be2d outdated

2238 | +
2239 | +    /* N + 1 */
2240 | +    CHECK(secp256k1_scalar_check_overflow(&n_plus_1) == 1);
2241 | +
2242 | +    /* 2^256 - 1 */
2243 | +    CHECK(secp256k1_scalar_check_overflow(&max) == 1);

real-or-random commented at 11:03 AM on February 4, 2026:

    secp256k1_scalar_set_int(&s, 0);
    CHECK(secp256k1_scalar_check_overflow(&s) == 0);
    CHECK(secp256k1_scalar_check_overflow(&n_minus_1) == 0);
    CHECK(secp256k1_scalar_check_overflow(&n) == 1);
    CHECK(secp256k1_scalar_check_overflow(&n_plus_1) == 1);
    CHECK(secp256k1_scalar_check_overflow(&max) == 1);

I think these comments don't add a lot; the identifiers are already clear.

in src/tests.c:2218 in 58be14be2d outdated
```
2213 | +    );
2214 | +
2215 | +    int i;
2216 | +    int j;
2217 | +    int overflow = 0;
2218 | +    unsigned char b32[32];
```
real-or-random commented at 11:04 AM on February 4, 2026:

some of these declarations can also be moved inside the loop block

in src/tests.c:2246 in 58be14be2d outdated

2241 | +
2242 | +    /* 2^256 - 1 */
2243 | +    CHECK(secp256k1_scalar_check_overflow(&max) == 1);
2244 | +
2245 | +    /* Random inputs */
2246 | +    for (i = 0; i < 256; i++) {

real-or-random commented at 11:04 AM on February 4, 2026:

The number of iterations should depend on COUNT, see other tests.

real-or-random commented at 11:04 AM on February 4, 2026: contributor

Concept ACK
real-or-random commented at 11:05 AM on February 4, 2026: contributor

Thank you, I have removed the trailing whitespace and all the tests passed.

Thanks, and please squash your commits. :)
real-or-random added the label assurance on Feb 4, 2026
real-or-random added the label tweak/refactor on Feb 4, 2026
therohityadav force-pushed on Feb 4, 2026
therohityadav commented at 1:01 PM on February 4, 2026: contributor
Thank you, I have removed the trailing whitespace and all the tests passed.

Thanks, and please squash your commits. :)

Thanks for the Concept ACK and the review! I have addressed all the feedback:
- Restored the global count variable and used it for the loop iteration.
- Switched to testrand256 for randomness.
- Used secp256k1_group_order_bytes to avoid duplicating constants.
- Squashed the commits into one.
real-or-random commented at 2:06 PM on February 4, 2026: contributor

I don't see any diff compared to your last push. (See https://github.com/bitcoin-core/secp256k1/compare/58be14be2de04cc39df0f286826f0b19b8809fe0..83568d863521ba25a2f3bc791f0776a898cdb2cb)

But I see you squashed. Did you push the wrong commit?
therohityadav force-pushed on Feb 4, 2026
in src/tests.c:2226 in 1cee90f551
```
2221 | +    CHECK(secp256k1_scalar_check_overflow(&n_minus_1) == 0);
2222 | +    CHECK(secp256k1_scalar_check_overflow(&n) == 1);
2223 | +    CHECK(secp256k1_scalar_check_overflow(&n_plus_1) == 1);
2224 | +    CHECK(secp256k1_scalar_check_overflow(&max) == 1);
2225 | +
2226 | +    for (i = 0; i < count; i++) {
```
real-or-random commented at 4:10 PM on February 4, 2026:

I literally meant COUNT, a global variable in the tests. This loop could run 2 * COUNT times, I think.

therohityadav commented at 5:28 PM on February 4, 2026:

I literally meant COUNT, a global variable in the tests. This loop could run 2 * COUNT times, I think.

Thank you for the catch. I have removed the local count variable and updated the loop to use the global COUNT with 2 * COUNT iterations.
real-or-random commented at 4:10 PM on February 4, 2026: contributor

Thanks for the update, I have just one more comment
therohityadav force-pushed on Feb 4, 2026
in src/tests.c:2246 in 6b49efd652 outdated
```
2240 | +    }
2241 | +}
2242 | +
2243 |  static void run_scalar_tests(void) {
2244 |      int i;
2245 | +    test_scalar_check_overflow();
```
theStack commented at 6:02 PM on February 4, 2026:

non-blocking nit: could add a newline before to separate the declaration block

therohityadav commented at 6:29 PM on February 4, 2026:

non-blocking nit: could add a newline before to separate the declaration block

I have added the newline to separate the declaration block.

Regarding the scalar constant deduplication: I am looking into doing that in a follow-up PR to keep this one focused.
theStack approved
theStack commented at 6:07 PM on February 4, 2026: contributor

Concept and code-review ACK 6b49efd652b0e370375dcf746de233fd67271dce

As a follow-up idea, maybe a deduplication of these scalar constants with other tests could make sense (see e.g. $ git grep -i 0xbaeedce6).
therohityadav force-pushed on Feb 4, 2026
test: add unit tests for secp256k1_scalar_check_overflow f47bbc07f0
therohityadav force-pushed on Feb 4, 2026
theStack approved
theStack commented at 7:16 PM on February 4, 2026: contributor

re-ACK f47bbc07f0a1979a9e363898c5191811f9d8def5
real-or-random approved
real-or-random commented at 7:24 PM on February 4, 2026: contributor

utACK f47bbc07f0a1979a9e363898c5191811f9d8def5
real-or-random merged this on Feb 4, 2026
real-or-random closed this on Feb 4, 2026
real-or-random commented at 7:25 PM on February 4, 2026: contributor

Thanks for your contribution!

Contributors

Labels

assurance tweak/refactor

Linked (view graph)

#1812 tests: Improve _scalar_check_overflow tests #1820 test: deduplicate scalar and field constants for n-1 and p-1

tests: Improve secp256k1_scalar_check_overflow tests (Issue #1812) #1819

Changes: