tests: Improve secp256k1_scalar_check_overflow tests (Issue #1812)#1819
Conversation
hebasto
left a comment
There was a problem hiding this comment.
Trailing spaces should be removed to pass the CI.
Thank you, I have removed the trailing whitespace and all the tests passed. |
Thanks, and please squash your commits. :) |
58be14b to
83568d8
Compare
Thanks for the Concept ACK and the review! I have addressed all the feedback:
|
|
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? |
83568d8 to
1cee90f
Compare
real-or-random
left a comment
There was a problem hiding this comment.
Thanks for the update, I have just one more comment
src/tests.c
Outdated
| CHECK(secp256k1_scalar_check_overflow(&n_plus_1) == 1); | ||
| CHECK(secp256k1_scalar_check_overflow(&max) == 1); | ||
|
|
||
| for (i = 0; i < count; i++) { |
There was a problem hiding this comment.
I literally meant COUNT, a global variable in the tests. This loop could run 2 * COUNT times, I think.
There was a problem hiding this comment.
I literally meant
COUNT, a global variable in the tests. This loop could run2 * COUNTtimes, 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.
1cee90f to
6b49efd
Compare
|
|
||
| static void run_scalar_tests(void) { | ||
| int i; | ||
| test_scalar_check_overflow(); |
There was a problem hiding this comment.
non-blocking nit: could add a newline before to separate the declaration block
There was a problem hiding this comment.
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.
6b49efd to
1e7dfc0
Compare
1e7dfc0 to
f47bbc0
Compare
|
Thanks for your contribution! |
ac561601b8 Merge bitcoin-core/secp256k1#1760: cmake: Add dynamic test discovery to improve parallelism c7a7f732bd Merge bitcoin-core/secp256k1#1821: ellswift: fix overflow flag handling in secp256k1_ellswift_xdh b99a94c382 Add tests for bad scalar inputs in ellswift XDH 307b49f1b9 ellswift: fix overflow flag handling in secp256k1_ellswift_xdh 322d0a4358 Merge bitcoin-core/secp256k1#1823: ci: Load Docker image by ID from builder step ed02466d3f ci: Load Docker image by ID from builder step 1d146ac3ed Merge bitcoin-core/secp256k1#1819: tests: Improve secp256k1_scalar_check_overflow tests (Issue bitcoin#1812) f47bbc07f0 test: add unit tests for secp256k1_scalar_check_overflow d071aa56d5 Merge bitcoin-core/secp256k1#1815: refactor: remove unnecessary `malloc` result casts 99ab4a105e Merge bitcoin-core/secp256k1#1817: ci: Disable Docker build summary generation c5da3bde9c Merge bitcoin-core/secp256k1#1818: ci: Enforce base-10 evaluation 97de5120cf Merge bitcoin-core/secp256k1#1804: test: show both CMake and Autotools usage for ctime_tests 4fb7ccf5d4 ci: Enforce base-10 evaluation 3ae72e7867 ci: Disable Docker build summary generation 97b3c47849 refactor: remove unnecessary `malloc` result casts 1bc74a22f8 test: show both Autotools and CMake usage for ctime_tests 8354618e02 cmake: Set `LABELS` property for tests 29f26ec3cf cmake: Integrate DiscoverTests and normalize test names f95b263f23 cmake: Add DiscoverTests module 4ac651144b cmake, refactor: Deduplicate test-related code git-subtree-dir: src/secp256k1 git-subtree-split: ac561601b8a3452bc1869746de423359944d4e00
dfe042feb2 Merge bitcoin-core/secp256k1#1828: Revert "ci, docker: Fix LLVM repository signature failure" 76e92cfeea Revert "ci, docker: Fix LLVM repository signature failure" ac561601b8 Merge bitcoin-core/secp256k1#1760: cmake: Add dynamic test discovery to improve parallelism c7a7f732bd Merge bitcoin-core/secp256k1#1821: ellswift: fix overflow flag handling in secp256k1_ellswift_xdh b99a94c382 Add tests for bad scalar inputs in ellswift XDH 307b49f1b9 ellswift: fix overflow flag handling in secp256k1_ellswift_xdh 322d0a4358 Merge bitcoin-core/secp256k1#1823: ci: Load Docker image by ID from builder step ed02466d3f ci: Load Docker image by ID from builder step 1d146ac3ed Merge bitcoin-core/secp256k1#1819: tests: Improve secp256k1_scalar_check_overflow tests (Issue bitcoin#1812) f47bbc07f0 test: add unit tests for secp256k1_scalar_check_overflow d071aa56d5 Merge bitcoin-core/secp256k1#1815: refactor: remove unnecessary `malloc` result casts 99ab4a105e Merge bitcoin-core/secp256k1#1817: ci: Disable Docker build summary generation c5da3bde9c Merge bitcoin-core/secp256k1#1818: ci: Enforce base-10 evaluation 97de5120cf Merge bitcoin-core/secp256k1#1804: test: show both CMake and Autotools usage for ctime_tests 4fb7ccf5d4 ci: Enforce base-10 evaluation 3ae72e7867 ci: Disable Docker build summary generation 97b3c47849 refactor: remove unnecessary `malloc` result casts 1bc74a22f8 test: show both Autotools and CMake usage for ctime_tests 8354618e02 cmake: Set `LABELS` property for tests 29f26ec3cf cmake: Integrate DiscoverTests and normalize test names f95b263f23 cmake: Add DiscoverTests module 4ac651144b cmake, refactor: Deduplicate test-related code git-subtree-dir: src/secp256k1 git-subtree-split: dfe042feb28dcc8e2a1bea5566ba28afb1ebb4a9
3019186a6d Merge bitcoin-core/secp256k1#1829: ci: Fix leftover use of old ECMULTGENPRECISION 79e9f25237 ci: Fix leftover use of old ECMULTGENPRECISION dfe042feb2 Merge bitcoin-core/secp256k1#1828: Revert "ci, docker: Fix LLVM repository signature failure" 76e92cfeea Revert "ci, docker: Fix LLVM repository signature failure" ac561601b8 Merge bitcoin-core/secp256k1#1760: cmake: Add dynamic test discovery to improve parallelism c7a7f732bd Merge bitcoin-core/secp256k1#1821: ellswift: fix overflow flag handling in secp256k1_ellswift_xdh b99a94c382 Add tests for bad scalar inputs in ellswift XDH 307b49f1b9 ellswift: fix overflow flag handling in secp256k1_ellswift_xdh 322d0a4358 Merge bitcoin-core/secp256k1#1823: ci: Load Docker image by ID from builder step ed02466d3f ci: Load Docker image by ID from builder step 1d146ac3ed Merge bitcoin-core/secp256k1#1819: tests: Improve secp256k1_scalar_check_overflow tests (Issue bitcoin#1812) f47bbc07f0 test: add unit tests for secp256k1_scalar_check_overflow d071aa56d5 Merge bitcoin-core/secp256k1#1815: refactor: remove unnecessary `malloc` result casts 99ab4a105e Merge bitcoin-core/secp256k1#1817: ci: Disable Docker build summary generation c5da3bde9c Merge bitcoin-core/secp256k1#1818: ci: Enforce base-10 evaluation 97de5120cf Merge bitcoin-core/secp256k1#1804: test: show both CMake and Autotools usage for ctime_tests 4fb7ccf5d4 ci: Enforce base-10 evaluation 3ae72e7867 ci: Disable Docker build summary generation 97b3c47849 refactor: remove unnecessary `malloc` result casts 1bc74a22f8 test: show both Autotools and CMake usage for ctime_tests 8354618e02 cmake: Set `LABELS` property for tests 29f26ec3cf cmake: Integrate DiscoverTests and normalize test names f95b263f23 cmake: Add DiscoverTests module 4ac651144b cmake, refactor: Deduplicate test-related code git-subtree-dir: src/secp256k1 git-subtree-split: 3019186a6d46c63e3c520b03eb7bb0441fcd0452
b9cb1cbfd7 Merge bitcoin-core/secp256k1#1824: util: introduce and use `ARRAY_SIZE` macro c0a2aba088 Merge bitcoin-core/secp256k1#1811: bench: Update help functions in bench and bench_internal 10f546a2c0 Merge bitcoin-core/secp256k1#1832: testrand: Remove testrand_finish 8d0eda07e9 testrand: Remove testrand_finish 95e6815843 Merge bitcoin-core/secp256k1#1825: hash: remove redundant `secp256k1_sha256_initialize` in tagged hash midstate functions f48b1bfa5d hash: add midstate initializer and use it for tagged hashes 3019186a6d Merge bitcoin-core/secp256k1#1829: ci: Fix leftover use of old ECMULTGENPRECISION 79e9f25237 ci: Fix leftover use of old ECMULTGENPRECISION dfe042feb2 Merge bitcoin-core/secp256k1#1828: Revert "ci, docker: Fix LLVM repository signature failure" 76e92cfeea Revert "ci, docker: Fix LLVM repository signature failure" ac561601b8 Merge bitcoin-core/secp256k1#1760: cmake: Add dynamic test discovery to improve parallelism c7a7f732bd Merge bitcoin-core/secp256k1#1821: ellswift: fix overflow flag handling in secp256k1_ellswift_xdh 921b9711ea util: introduce and use `ARRAY_SIZE` macro b99a94c382 Add tests for bad scalar inputs in ellswift XDH 307b49f1b9 ellswift: fix overflow flag handling in secp256k1_ellswift_xdh 322d0a4358 Merge bitcoin-core/secp256k1#1823: ci: Load Docker image by ID from builder step ed02466d3f ci: Load Docker image by ID from builder step c49c9be504 bench: Update help functions in bench and bench_internal 1d146ac3ed Merge bitcoin-core/secp256k1#1819: tests: Improve secp256k1_scalar_check_overflow tests (Issue bitcoin#1812) f47bbc07f0 test: add unit tests for secp256k1_scalar_check_overflow d071aa56d5 Merge bitcoin-core/secp256k1#1815: refactor: remove unnecessary `malloc` result casts 99ab4a105e Merge bitcoin-core/secp256k1#1817: ci: Disable Docker build summary generation c5da3bde9c Merge bitcoin-core/secp256k1#1818: ci: Enforce base-10 evaluation 97de5120cf Merge bitcoin-core/secp256k1#1804: test: show both CMake and Autotools usage for ctime_tests 4fb7ccf5d4 ci: Enforce base-10 evaluation 3ae72e7867 ci: Disable Docker build summary generation 97b3c47849 refactor: remove unnecessary `malloc` result casts 1bc74a22f8 test: show both Autotools and CMake usage for ctime_tests 8354618e02 cmake: Set `LABELS` property for tests 29f26ec3cf cmake: Integrate DiscoverTests and normalize test names f95b263f23 cmake: Add DiscoverTests module 4ac651144b cmake, refactor: Deduplicate test-related code git-subtree-dir: src/secp256k1 git-subtree-split: b9cb1cbfd72c9391c337ed1e30d71355dde65248
1aafe15139 Merge bitcoin-core/secp256k1#1777: Make SHA256 compression runtime pluggable b9cb1cbfd7 Merge bitcoin-core/secp256k1#1824: util: introduce and use `ARRAY_SIZE` macro 4d92a083bc sha256: speed up writes using multi-block compression 0753f8b909 Add API to override SHA256 compression at runtime fdb6a91a5e Introduce hash context to support pluggable SHA256 compression c0a2aba088 Merge bitcoin-core/secp256k1#1811: bench: Update help functions in bench and bench_internal 10f546a2c0 Merge bitcoin-core/secp256k1#1832: testrand: Remove testrand_finish 8d0eda07e9 testrand: Remove testrand_finish 95e6815843 Merge bitcoin-core/secp256k1#1825: hash: remove redundant `secp256k1_sha256_initialize` in tagged hash midstate functions f48b1bfa5d hash: add midstate initializer and use it for tagged hashes 3019186a6d Merge bitcoin-core/secp256k1#1829: ci: Fix leftover use of old ECMULTGENPRECISION 79e9f25237 ci: Fix leftover use of old ECMULTGENPRECISION dfe042feb2 Merge bitcoin-core/secp256k1#1828: Revert "ci, docker: Fix LLVM repository signature failure" 76e92cfeea Revert "ci, docker: Fix LLVM repository signature failure" ac561601b8 Merge bitcoin-core/secp256k1#1760: cmake: Add dynamic test discovery to improve parallelism c7a7f732bd Merge bitcoin-core/secp256k1#1821: ellswift: fix overflow flag handling in secp256k1_ellswift_xdh 921b9711ea util: introduce and use `ARRAY_SIZE` macro b99a94c382 Add tests for bad scalar inputs in ellswift XDH 307b49f1b9 ellswift: fix overflow flag handling in secp256k1_ellswift_xdh 322d0a4358 Merge bitcoin-core/secp256k1#1823: ci: Load Docker image by ID from builder step ed02466d3f ci: Load Docker image by ID from builder step c49c9be504 bench: Update help functions in bench and bench_internal 1d146ac3ed Merge bitcoin-core/secp256k1#1819: tests: Improve secp256k1_scalar_check_overflow tests (Issue bitcoin#1812) f47bbc07f0 test: add unit tests for secp256k1_scalar_check_overflow d071aa56d5 Merge bitcoin-core/secp256k1#1815: refactor: remove unnecessary `malloc` result casts 99ab4a105e Merge bitcoin-core/secp256k1#1817: ci: Disable Docker build summary generation c5da3bde9c Merge bitcoin-core/secp256k1#1818: ci: Enforce base-10 evaluation 97de5120cf Merge bitcoin-core/secp256k1#1804: test: show both CMake and Autotools usage for ctime_tests 4fb7ccf5d4 ci: Enforce base-10 evaluation 3ae72e7867 ci: Disable Docker build summary generation 97b3c47849 refactor: remove unnecessary `malloc` result casts 1bc74a22f8 test: show both Autotools and CMake usage for ctime_tests 8354618e02 cmake: Set `LABELS` property for tests 29f26ec3cf cmake: Integrate DiscoverTests and normalize test names f95b263f23 cmake: Add DiscoverTests module 4ac651144b cmake, refactor: Deduplicate test-related code git-subtree-dir: src/secp256k1 git-subtree-split: 1aafe15139976b0142d791aaf4963de3fc1ff736
…17, 1815, 1819, 1823, 1821, 1760, 1828, 1829, 1825, 1832, 1811 48cbd78 modules: Port bitcoin-core/secp256k1#1825 to zkp-specific code (mllwchrry) 126501f modules: Port bitcoin-core/secp256k1#1815 to zkp-specific code (mllwchrry) c49c9be bench: Update help functions in bench and bench_internal (kevkevinpal) 8d0eda0 testrand: Remove testrand_finish (Tim Ruffing) f48b1bf hash: add midstate initializer and use it for tagged hashes (w0xlt) 79e9f25 ci: Fix leftover use of old ECMULTGENPRECISION (Tim Ruffing) 76e92cf Revert "ci, docker: Fix LLVM repository signature failure" (Hennadii Stepanov) 8354618 cmake: Set `LABELS` property for tests (Hennadii Stepanov) 29f26ec cmake: Integrate DiscoverTests and normalize test names (Hennadii Stepanov) f95b263 cmake: Add DiscoverTests module (Hennadii Stepanov) 4ac6511 cmake, refactor: Deduplicate test-related code (Hennadii Stepanov) b99a94c Add tests for bad scalar inputs in ellswift XDH (gzJx0DuTRHytnHe7P5RmMbPf3wKy2BztweVGXTf) 307b49f ellswift: fix overflow flag handling in secp256k1_ellswift_xdh (gzJx0DuTRHytnHe7P5RmMbPf3wKy2BztweVGXTf) ed02466 ci: Load Docker image by ID from builder step (Hennadii Stepanov) f47bbc0 test: add unit tests for secp256k1_scalar_check_overflow (Rohit Yadav) 97b3c47 refactor: remove unnecessary `malloc` result casts (Sebastian Falbesoner) 3ae72e7 ci: Disable Docker build summary generation (Hennadii Stepanov) 4fb7ccf ci: Enforce base-10 evaluation (Hennadii Stepanov) 1bc74a2 test: show both Autotools and CMake usage for ctime_tests (8144225309) 86cae58 build: Add `-Wleading-whitespace=spaces` compiler flag (Hennadii Stepanov) fb229e7 build: Add `-Wtrailing-whitespace=any` compiler flag (Hennadii Stepanov) 13e3bee refactor: Remove trailing whitespace (Hennadii Stepanov) 2ccff6e ci: Add weekly schedule (Hennadii Stepanov) 2f18567 ci: Rotate Docker cache keys every 4 weeks (Hennadii Stepanov) 0ffb174 ci, docker: Fix LLVM repository signature failure (Hennadii Stepanov) 0267b65 release process: mention the `[Unreleased]` link clearly (Jonas Nick) 748c0fd Add CMake build directory patterns to `.gitignore` (Hennadii Stepanov) 7eb86bd autotools: Rename `build-aux` to `autotools-aux` (Hennadii Stepanov) 47eb709 ecmult: Use size_t for array indices in _odd_multiplies_table (Tim Ruffing) bb1d199 ecmult: Use size_t for array indices into tables (Tim Ruffing) Pull request description: Merge bitcoin-core/secp256k1#1794: ecmult: Use size_t for array indices Merge bitcoin-core/secp256k1#1775: Add CMake build directory patterns to `.gitignore` Merge bitcoin-core/secp256k1#1814: release process: mention the `[Unreleased]` link clearly Merge bitcoin-core/secp256k1#1816: ci: Rotate Docker cache keys Merge bitcoin-core/secp256k1#1813: Remove trailing spaces and introduce `-Wtrailing-whitespace=any` compiler flag Merge bitcoin-core/secp256k1#1804: test: show both CMake and Autotools usage for ctime_tests Merge bitcoin-core/secp256k1#1818: ci: Enforce base-10 evaluation Merge bitcoin-core/secp256k1#1817: ci: Disable Docker build summary generation Merge bitcoin-core/secp256k1#1815: refactor: remove unnecessary `malloc` result casts Merge bitcoin-core/secp256k1#1819: tests: Improve secp256k1_scalar_check_overflow tests (Issue #1812) Merge bitcoin-core/secp256k1#1823: ci: Load Docker image by ID from builder step Merge bitcoin-core/secp256k1#1821: ellswift: fix overflow flag handling in secp256k1_ellswift_xdh Merge bitcoin-core/secp256k1#1760: cmake: Add dynamic test discovery to improve parallelism Merge bitcoin-core/secp256k1#1828: Revert "ci, docker: Fix LLVM repository signature failure" Merge bitcoin-core/secp256k1#1829: ci: Fix leftover use of old ECMULTGENPRECISION Merge bitcoin-core/secp256k1#1825: hash: remove redundant `secp256k1_sha256_initialize` in tagged hash midstate functions Merge bitcoin-core/secp256k1#1832: testrand: Remove testrand_finish Merge bitcoin-core/secp256k1#1811: bench: Update help functions in bench and bench_internal This PR can be recreated with `./contrib/sync-upstream.sh -b master range c0a2aba`. Tips: * Use `git show --remerge-diff <pr-branch>` to show the conflict resolution in the merge commit. * Use `git read-tree --reset -u <pr-branch>` to replay these resolutions during the conflict resolution stage when recreating the PR branch locally. Be aware that this may discard your index as well as the uncommitted changes and untracked files in your worktree. ACKs for top commit: real-or-random: ACK 48cbd78 Tree-SHA512: 1ea96bf3b7458c02bb472ca7423eb21defe6640ea58a993ac24603d6a8d44ac9bbeab7e2ca5f993918fa1388b9e69b308dcc67277afffd55cb833ecf07051c75
1aafe15139 Merge bitcoin-core/secp256k1#1777: Make SHA256 compression runtime pluggable b9cb1cbfd7 Merge bitcoin-core/secp256k1#1824: util: introduce and use `ARRAY_SIZE` macro 4d92a083bc sha256: speed up writes using multi-block compression 0753f8b909 Add API to override SHA256 compression at runtime fdb6a91a5e Introduce hash context to support pluggable SHA256 compression c0a2aba088 Merge bitcoin-core/secp256k1#1811: bench: Update help functions in bench and bench_internal 10f546a2c0 Merge bitcoin-core/secp256k1#1832: testrand: Remove testrand_finish 8d0eda07e9 testrand: Remove testrand_finish 95e6815843 Merge bitcoin-core/secp256k1#1825: hash: remove redundant `secp256k1_sha256_initialize` in tagged hash midstate functions f48b1bfa5d hash: add midstate initializer and use it for tagged hashes 3019186a6d Merge bitcoin-core/secp256k1#1829: ci: Fix leftover use of old ECMULTGENPRECISION 79e9f25237 ci: Fix leftover use of old ECMULTGENPRECISION dfe042feb2 Merge bitcoin-core/secp256k1#1828: Revert "ci, docker: Fix LLVM repository signature failure" 76e92cfeea Revert "ci, docker: Fix LLVM repository signature failure" ac561601b8 Merge bitcoin-core/secp256k1#1760: cmake: Add dynamic test discovery to improve parallelism c7a7f732bd Merge bitcoin-core/secp256k1#1821: ellswift: fix overflow flag handling in secp256k1_ellswift_xdh 921b9711ea util: introduce and use `ARRAY_SIZE` macro b99a94c382 Add tests for bad scalar inputs in ellswift XDH 307b49f1b9 ellswift: fix overflow flag handling in secp256k1_ellswift_xdh 322d0a4358 Merge bitcoin-core/secp256k1#1823: ci: Load Docker image by ID from builder step ed02466d3f ci: Load Docker image by ID from builder step c49c9be504 bench: Update help functions in bench and bench_internal 1d146ac3ed Merge bitcoin-core/secp256k1#1819: tests: Improve secp256k1_scalar_check_overflow tests (Issue bitcoin#1812) f47bbc07f0 test: add unit tests for secp256k1_scalar_check_overflow d071aa56d5 Merge bitcoin-core/secp256k1#1815: refactor: remove unnecessary `malloc` result casts 99ab4a105e Merge bitcoin-core/secp256k1#1817: ci: Disable Docker build summary generation c5da3bde9c Merge bitcoin-core/secp256k1#1818: ci: Enforce base-10 evaluation 97de5120cf Merge bitcoin-core/secp256k1#1804: test: show both CMake and Autotools usage for ctime_tests 4fb7ccf5d4 ci: Enforce base-10 evaluation 3ae72e7867 ci: Disable Docker build summary generation 97b3c47849 refactor: remove unnecessary `malloc` result casts 1bc74a22f8 test: show both Autotools and CMake usage for ctime_tests 8354618e02 cmake: Set `LABELS` property for tests 29f26ec3cf cmake: Integrate DiscoverTests and normalize test names f95b263f23 cmake: Add DiscoverTests module 4ac651144b cmake, refactor: Deduplicate test-related code git-subtree-dir: src/secp256k1 git-subtree-split: 1aafe15139976b0142d791aaf4963de3fc1ff736
This Pull Request improves the tests for
secp256k1_scalar_check_overflowas requested in #1812.Changes:
run_scalar_tests.test_scalar_check_overflow.0,N-1,N,N+1, andMAX.check_overflowagainst a manual byte comparison.Fixes #1812.