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
  1. 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.

  2. 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:
  3. 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);
  4. hebasto commented at 9:14 AM on February 4, 2026: member

    Trailing spaces should be removed to pass the CI.

  5. 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.

  6. 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.

  7. 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.

  8. 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.

  9. 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

  10. 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.

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

    Concept ACK

  12. 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. :)

  13. real-or-random added the label assurance on Feb 4, 2026
  14. real-or-random added the label tweak/refactor on Feb 4, 2026
  15. therohityadav force-pushed on Feb 4, 2026
  16. 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.
  17. 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?

  18. therohityadav force-pushed on Feb 4, 2026
  19. 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.

  20. real-or-random commented at 4:10 PM on February 4, 2026: contributor

    Thanks for the update, I have just one more comment

  21. therohityadav force-pushed on Feb 4, 2026
  22. in src/tests.c:2245 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.

  23. theStack approved
  24. 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).

  25. therohityadav force-pushed on Feb 4, 2026
  26. test: add unit tests for secp256k1_scalar_check_overflow f47bbc07f0
  27. therohityadav force-pushed on Feb 4, 2026
  28. theStack approved
  29. theStack commented at 7:16 PM on February 4, 2026: contributor

    re-ACK f47bbc07f0a1979a9e363898c5191811f9d8def5

  30. real-or-random approved
  31. real-or-random commented at 7:24 PM on February 4, 2026: contributor

    utACK f47bbc07f0a1979a9e363898c5191811f9d8def5

  32. real-or-random merged this on Feb 4, 2026
  33. real-or-random closed this on Feb 4, 2026
  34. real-or-random commented at 7:25 PM on February 4, 2026: contributor

    Thanks for your contribution!

  35. fanquake referenced this in commit fa3704594c on Feb 19, 2026
  36. fanquake referenced this in commit 46ca85d851 on Feb 24, 2026
  37. fanquake referenced this in commit 9478a62d21 on Feb 25, 2026
  38. fanquake referenced this in commit 0dab9edba4 on Mar 3, 2026
  39. fanquake referenced this in commit a50dbd2bcd on Mar 4, 2026
  40. real-or-random referenced this in commit 7a5f153d71 on Mar 4, 2026
  41. fanquake referenced this in commit ec7ea286b0 on Mar 10, 2026
  42. fanquake referenced this in commit 6458812af8 on Mar 17, 2026
  43. fanquake referenced this in commit 791949a724 on Mar 21, 2026
  44. fanquake referenced this in commit cffa9320f2 on Mar 23, 2026
  45. theStack referenced this in commit 994739fc33 on Mar 26, 2026
  46. fanquake referenced this in commit dfc5233b8c on Mar 27, 2026
  47. theStack referenced this in commit 0326234c51 on Apr 2, 2026
  48. fanquake referenced this in commit dfd54c959e on Apr 9, 2026
  49. vmta referenced this in commit 1ddc2f947f on Apr 26, 2026
  50. vmta referenced this in commit 56c40fe100 on Apr 27, 2026
Contributors
therohityadavhebastoreal-or-randomtheStack
Labels
assurancetweak/refactor
Linked (view graph)
#1812 tests: Improve _scalar_check_overflow tests#1820 test: deduplicate scalar and field constants for n-1 and p-1
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/secp256k1. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-19 06:52 UTC