tests: Improve secp256k1_scalar_check_overflow tests (Issue #1812)

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.

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:

0            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:

0    secp256k1_scalar_set_int(&s, 0);
1    CHECK(secp256k1_scalar_check_overflow(&s) == 0);
2    CHECK(secp256k1_scalar_check_overflow(&n_minus_1) == 0);
3    CHECK(secp256k1_scalar_check_overflow(&n) == 1);
4    CHECK(secp256k1_scalar_check_overflow(&n_plus_1) == 1);
5    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!

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

Changes: