f47bbc07f0 test: add unit tests for secp256k1_scalar_check_overflow (Rohit Yadav)
Pull request description:
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.
ACKs for top commit:
theStack:
re-ACK f47bbc07f0
real-or-random:
utACK f47bbc07f0
Tree-SHA512: dad3aa31ecf3f296843c907ac3d9aa5a9b9cb839b36aa3b59e49c853c60c58291412e70dff37dc15f8e14023a8f1e1aba87395065607612d5f6cfa92e14e73b5
It seems that there is no good reason to do this and it's even
considered bad practice, see e.g. https://stackoverflow.com/a/605858
This commit touches mostly test code, the only two functions used
in production are `secp256k1_context_{create,clone}`.
Instances were found manually via `$ git grep "malloc("`
2f73e5281d group: Avoid using infinity field directly in other modules (Tim Ruffing)
Pull request description:
Minor refactoring to make the abstraction cleaner
ACKs for top commit:
hebasto:
ACK 2f73e5281d, I have reviewed the code and it looks OK.
theStack:
ACK 2f73e5281d
Tree-SHA512: eae5ad1ce81f491adb48ab1cbf04211f8d43e41255abcacc958fa3dcb1de5021707d56ed1b009a6f3f6c45cd8f20c1f2677891690a3c0a467fc7e064af2512a8
2f4546ce56 test: add --log option to display tests execution (furszy)
95b9953ea4 test: Add option to display all available tests (furszy)
953f7b0088 test: support running specific tests/modules targets (furszy)
0302c1a3d7 test: add --help for command-line options (furszy)
9ec3bfe22d test: adapt modules to the new test infrastructure (furszy)
48789dafc2 test: introduce (mini) unit test framework (furszy)
9cce703863 refactor: move 'gettime_i64()' to tests_common.h (furszy)
Pull request description:
Early Note:
Don’t be scared by the PR’s line changes count — most of it’s just doc or part of the test framework API.
Context:
Currently, all tests run single-threaded sequentially and the library lacks the ability to specify which test (or group of tests) you would like to run. This is not only inconvenient as more tests are added but also time consuming during development and affects downstream projects that may want to parallelize the workload (such as Bitcoin-Core CI).
PR Goal:
Introduce a lightweight, extensible C89 unit test framework with no dynamic memory allocations, providing a structured way to register, execute, and report tests. The framework supports named command-line arguments in `-key=value` form, parallel test execution across multiple worker processes, granular test selection (selecting tests either by name or by module name), and time accumulation reports.
The introduced framework supports:
* `-help` or `-h`: display list of available commands along with their descriptions.
* `-jobs=<num>`: distribute tests across multiple worker processes (default: sequential if 0).
* `-target=<name>` or `-t=<name>`: run only specific tests by name; can be repeated to select multiple tests.
* `-target=<module name>`, `-t=<module>` Run all tests within a specific module (can be provided multiple times)
* `-seed=<hex>`: set a specific RNG seed (defaults to random if unspecified).
* `-iterations=<n>`: specify the number of iterations.
* `-list_tests`: display list of available tests and modules you can run.
* `-log=<0|1>`: enable or disable test execution logging (default: 0 = disabled).
Beyond these features, the idea is to also make future developments smoother, as adding new tests require only a single entry in the central test registry, and new command-line options can be introduced easily by extending the framework’s `parse_arg()` function.
Compatibility Note:
The framework continues accepting the two positional arguments previously supported (iterations and seed), ensuring existing workflows remain intact.
Testing Notes:
Have fun. You can quickly try it through `./tests -j=<workers_num>` for parallel execution or `./tests -t=<test_name>` to run a specific test (call `./tests -print_tests` to display all available tests and modules).
Extra Note:
I haven't checked the exhaustive tests file so far, but I will soon. For now, this only runs all tests declared in the `tests` binary.
Testing Results: (Current master branch vs PR in seconds)
* Raspberry Pi 5: master \~100 s → PR \~38 s (5 jobs)
* MacBook Pro M1: master \~30 s → PR \~10 s (6 jobs)
ACKs for top commit:
theStack:
re-ACK 2f4546ce56
real-or-random:
ACK 2f4546ce56
hebasto:
ACK 2f4546ce56.
Tree-SHA512: 85ca2cbb620b84b35b353d5d4cf093c388fc3851ca405eeb0e458f8fa72b60534bccd357c7edabf8fc9aa93d9ad0a6fbac3dd5c4d5f9dfdf4d8701a9834755b9
Lightweight unit testing framework, providing a structured way to define,
execute, and report tests. It includes a central test registry, a flexible
command-line argument parser of the form "--key=value" / "-k=value" /
"-key=value" (facilitating future framework extensions), ability to run
tests in parallel and accumulated test time logging reports.
So far the supported command-line args are:
- "--jobs=<num>" or "-j=<num>" to specify the number of parallel workers.
- "--seed=<hex>" to specify the RNG seed (random if not set).
- "--iterations=<num>" or "-i=<num>" to specify the number of iterations.
Compatibility Note:
To stay compatible with previous versions, the framework also supports
the two original positional arguments: the iterations count and the
RNG seed (in that order).
Move the sha256_tag_test_internal function out of the musig module
into tests.c. This makes it available to other modules wishing to verify tagged
hashes without needing to duplicate the function.
Change the function signature to expect a const unsigned char and update
the tagged hash tests to use static const unsigned char character
arrays (where necessary).
Add a comment for each tag. This is done as a convenience for checking
the strings against the protocol specifications, where the tags are
normally specified as strings.
Update tests in the ellswift and schnorrsig modules to use the
sha256_tag_test_internal helper function.
These function aliases have been described as DEPRECATED in the public
API docs already many years ago (see #701, commit 41fc7856), and in
addition explicit deprecation warnings are shown by the compiler at
least since the first official release 0.2.0 (see PR #1089, commit
fc94a2da), so it should be fine to just remove them by now.
Co-authored-by: Tim Ruffing <crypto@timruffing.de>
765ef53335 Clear _gej instances after point multiplication to avoid potential leaks (Sebastian Falbesoner)
349e6ab916 Introduce separate _clear functions for hash module (Tim Ruffing)
99cc9fd6d0 Don't rely on memset to set signed integers to 0 (Tim Ruffing)
97c57f42ba Implement various _clear() functions with secp256k1_memclear() (Tim Ruffing)
9bb368d146 Use secp256k1_memclear() to clear stack memory instead of memset() (Tim Ruffing)
e3497bbf00 Separate between clearing memory and setting to zero in tests (Tim Ruffing)
d79a6ccd43 Separate secp256k1_fe_set_int( . , 0 ) from secp256k1_fe_clear() (Tim Ruffing)
1c08126222 Add secp256k1_memclear() for clearing secret data (Tim Ruffing)
e7d384488e Don't clear secrets in pippenger implementation (Tim Ruffing)
Pull request description:
This PR picks up #636 (which in turn picked up #448, so this is take number three) and is essentially a rebase on master.
Some changes to the original PR:
* the clearing function now has the `secp256k1_` prefix again, since the related helper `_memczero` got it as well (see PR #835 / commit e89278f211)
* the original commit b17a7df8145a6a86d49c354c7e7b59a432ea5346 ("Make _set_fe_int( . , 0 ) set magnitude to 0") is not needed anymore, since it was already applied in PR #943 (commit d49011f54c)
* clearing of stack memory with `secp256k1_memclear` is now also done on modules that have been newly introduced since then, i.e. schnorr and ellswift (of course, there is still no guarantee that all places where clearing is necessary are covered)
So far I haven't looked at any disassembly and possible performance implications yet (there were some concerns expressed in https://github.com/bitcoin-core/secp256k1/pull/636#issuecomment-620118629), happy to go deeper there if this gets Concept ACKed.
The proposed method of using a memory barrier to prevent optimizating away the memset is still used in BoringSSL (where it was originally picked up from) and in the Linux Kernel, see e.g. 5af122c3df/crypto/mem.c (L335) and d456068672/include/linux/string.h (L348) / d456068672/include/linux/compiler.h (L102)Fixes#185.
ACKs for top commit:
sipa:
reACK 765ef53335
real-or-random:
ACK 765ef53335
Tree-SHA512: 5a034d5ad14178c06928022459f3d4f0877d06f576b24ab07b86b3608b0b3e9273217b8309a1db606f024f3032731f13013114b1e0828964b578814d1efb2959
0f73caf7c6 test, ci: Lower default iteration count to 16 (Hennadii Stepanov)
Pull request description:
The number of test iterations in the CI remains the same.
Resolves https://github.com/bitcoin-core/secp256k1/issues/1561.
```
$ ./build/src/tests
test count = 16
random seed = 59ea2b21267ec0ef0b4d13821292489f
random run = 2936c044f82c7598a866869b9d954d42
no problems found
```
ACKs for top commit:
sipa:
utACK 0f73caf7c6
jonasnick:
ACK 0f73caf7c6
Tree-SHA512: 84b265dc5d2780b3ea0a38f50ac8871d850ef2c97f33a0a5816baf20ac71c01db8b85696b343b089d7116d9cdb9450a6ca668229d95e54a39920d0e91a3127b3
The number of test iterations in the CI remains unchanged.
Additionally, the minimum iteration counts to enable the
`test_ecmult_constants_2bit` test is adjusted from 35 to 16, so it is
run by default.
Note that the already existing function `random_fe_magnitude` is removed
and the call-sites are adapted to pass the magnitude range of 8
(the maximum for secp256k1_fe_mul and secp256k1_fe_sqr) explicitly.
Function secp256k1_ecmult_multi_var expects to be called with a non-NULL
error_callback parameter. Fix the invocation in test_ecmult_accumulate
to do this.
While at it, wrap the call in a CHECK macro to ensure it succeeds.
Fixes: https://github.com/bitcoin-core/secp256k1/issues/1527
Instead of having the starting point of the ecmult_gen computation be
offset, do it with the final point. This enables reasoning over the
set of points reachable in intermediary computations, which can be
leveraged by potential future optimization.
Because the final point is in affine coordinates, its projective
blinding is no longer possible. It will be reintroduced again in
a different way, in a later commit.
Also introduce some more comments and more descriptive names.
Currently the `run_sqr` test doesn't do anything with the
result of the `fe_sqr` call. Improve that by checking that
the equation `(x+y)*(x-y) = x^2 - y^2` holds for some random
values y, as suggested in issue #1471 by real-or-random.
The existing loop for generating the x values is kept as-is.
`check_fe_equal` is a wrapper around `secp256k1_fe_equal` that takes
care of normalization. Since it doesn't check anything itself, the
CHECK macro is needed at the call-sites to actually ensure equality.
Without this commit, the res[i][1] test vectors are unused. They were introduced
to test the correctness of scalar_sqr(x) and scalar_mul(x, x). These tests were
deleted as part of removing scalar_sqr in commit
5437e7bdfb.
