The secp256k1_ellswift_xdh function uses overflow = secp256k1_scalar_is_zero(&s) which overwrites the overflow flag from the preceding secp256k1_scalar_set_b32 call. This means secret keys >= the curve order are silently accepted (reduced mod n) instead of being rejected.
The fix changes = to |=, matching the correct pattern already used in secp256k1_ecdh (main_impl.h, line 51).
The ECDH module's test suite explicitly tests overflow rejection (passes secp256k1_group_order_bytes as a key and checks the function returns 0). The ellswift test suite has no corresponding test, which is why this went undetected.
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
97b3c47849 refactor: remove unnecessary `malloc` result casts (Sebastian Falbesoner)
Pull request description:
While working on benchmark code for #1765, I noticed that in some instances we explicitly cast `malloc` results in the codebase. It seems that there is no good reason to do this in C, 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("`.
ACKs for top commit:
real-or-random:
Weak Concept ACK && Code Review ACK 97b3c47849
w0xlt:
ACK 97b3c47849
Tree-SHA512: 74aa9f47eb52b7f2a6fcb69deb6aef0c0daa136c5deedfba1228218ef178c722212d8e9936fd2946d2035df932637ca4df49c98ddde488c6b009a74c4d5df316
1bc74a22f8 test: show both Autotools and CMake usage for ctime_tests (8144225309)
Pull request description:
When building with CMake and running `ctime_tests` outside valgrind, users see:
```
Usage: libtool --mode=execute valgrind ./ctime_tests
```
CMake users don't have libtool. Show both commands.
### Before
```
$ ./build/bin/ctime_tests
This test can only usefully be run inside valgrind because it was not compiled under msan.
Usage: libtool --mode=execute valgrind ./ctime_tests
```
### After
```
$ ./build/bin/ctime_tests
This test can only usefully be run inside valgrind because it was not compiled under msan.
Usage: valgrind ./ctime_tests (or with Autotools: libtool --mode=execute valgrind ./ctime_tests)
```
Fixes#1697
ACKs for top commit:
real-or-random:
utACK 1bc74a22f8
Tree-SHA512: d35c332c75fe3df66928cb8b137e11995c67a57744985a50a539d1d9f24cf39ee46f17c6f6a501664a62f67e11b7bb041ba0e1eed6632bf7dccdb57a2c88f9bc
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("`
748c0fdd67 Add CMake build directory patterns to `.gitignore` (Hennadii Stepanov)
7eb86bdb01 autotools: Rename `build-aux` to `autotools-aux` (Hennadii Stepanov)
Pull request description:
Whenever I work on changes that require comparison, such as benchmarking, I end up with two or more build directories that provide different binary variants simultaneously. Adding these build directories to `.gitignore` makes the workflow a bit easier.
Additionally, a trivial refactoring is included to reduce the code.
ACKs for top commit:
real-or-random:
utACK 748c0fdd67
furszy:
ACK 748c0fdd67
Tree-SHA512: 948917dcdc2ec6d5a2227f35ef9208fdbc62c56047db1c60b39f6da632642847aefa18f136986f9f15f08e0b2385964afe9a311346b728536323c54b4f0e3f04
47eb70959a ecmult: Use size_t for array indices in _odd_multiplies_table (Tim Ruffing)
bb1d199de5 ecmult: Use size_t for array indices into tables (Tim Ruffing)
Pull request description:
I don't think the current code is incorrect, but using `size_t` improves readability because the type makes it clear that we're dealing with array indices.
Also, making the result of the `ECMULT_TABLE_SIZE` macro (hopefully) a `size_t` fixes a compiler warning on MSVC, see #1791.
ACKs for top commit:
hebasto:
re-ACK 47eb70959a.
jonasnick:
ACK 47eb70959a
theStack:
ACK 47eb70959a
Tree-SHA512: e484fd610d50e972021c0184a683993364290eb58e09b65f9521b4507ec8d0639b402c67002005630b389bc863a7aa05b75f7224524dbcbafbfa5f9a4812b4a5
In this change the get_iters function was updated to print an error
message and then return 0. In the functions that use get_iters they
print the help text and then EXIT_FAILURE
bd5ced1fe1 doc/bench: added help text for SECP256K1_BENCH_ITERS env var for bench_ecmult (kevkevinpal)
Pull request description:
ACKs for top commit:
real-or-random:
utACK bd5ced1fe1
hebasto:
ACK bd5ced1fe1, I have reviewed the code and it looks OK. Tested on Ubuntu 25.10.
jonasnick:
ACK bd5ced1fe1
Tree-SHA512: 7cfc1a8915717bdfe2901f20f578e23368ece9937a40f36805a0a5b741f97a0502a085c973f6912b96c2bca921ef1654908cfe2c90c0601a7ffa92de4415dc62
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
ae00c552df Add VERIFY_CHECKs that flags are 0 or 1 (John Moffett)
Pull request description:
Flags for constant-time masking rely on the values being exactly `0` or `1` rather than `0` or true (any nonzero). One function, `secp256k1_fe_cmov` [documents](e7f7083b53/src/field.h (L315)) and [`VERIFY_CHECK`s](e7f7083b53/src/field_impl.h (L365)) this, but most don't.
This updates the documentation and adds `VERIFY_CHECK`s enforcing `flag == 0 || flag == 1` for:
`secp256k1_fe_storage_cmov`
`secp256k1_gej_cmov`
`secp256k1_ge_storage_cmov`
`secp256k1_scalar_cadd_bit`
`secp256k1_scalar_cond_negate`
`secp256k1_scalar_cmov`
`secp256k1_int_cmov`
ACKs for top commit:
furszy:
ACK ae00c55
hebasto:
re-ACK ae00c552df.
Tree-SHA512: c9d358929d39d93b0aea602d318429f7e82af96bf601f048a1cdeb0621b8adc6d1204648d352aa2060cb0f63db6dcf0da863854375ed313cea44dfad61c19a18
3b5b03f301 doc/bench: Added cmake build options to bench error messages (kevkevinpal)
Pull request description:
## Motivation
I wanted to try and run the benchmarking scripts and I noticed the recovery benchmark in `bench.c`. I wanted to run but I was using `cmake` and the error message telling me to use `./configure -enable-module-recovery` wasn't sufficient.
I figure rather than forcing users to look into the `CMakeLists.txt` file or anywhere else we should add this to the output
## Solution
I appended to the message to include the `-DSECP256K1_ENABLE_MODULE_...=ON` in the message.
ACKs for top commit:
real-or-random:
utACK 3b5b03f301
hebasto:
ACK 3b5b03f301, I have reviewed the code and it looks OK.
Tree-SHA512: 3a6c966b65ab3f0d6dda81e5dd95529087db3f2901f2686af68c16079ec5b323568f3c9acb155d92a6d50b4102faf0844d0d87216df001adbf69cab4ce86dabc
No behavior changes.
Refactors the previously monolithic ElligatorSwift test into isolated,
independent test cases. Doing so allows the test suite to execute
these cases in parallel rather than sequentially.
Overall, seen 35-40% tests time reduction locally.
This is quite useful for the Debug build with no optimizations,
which is noticeably slow.
#### Local Debug-build Results (7 jobs):
- master: 138.0 seconds.
- this PR: 89.3 seconds.
(~1.55× speedup, ~35% reduction)
#### Local Release-build Results (7 jobs):
- master: 9.5 seconds.
- this PR: 5.9 seconds.
(~1.61× speedup, ~38% reduction)
Flags for constant-time masking rely
on the values being exactly 0 or 1 rather
than 0 or true. Add VERIFY_CHECKs to enforce
in VERIFY builds as a preventative
measure and add documentation where relevant.
3daab83a60 refactor: remove ret from secp256k1_ec_pubkey_serialize (kevkevinpal)
Pull request description:
This is a follow-up to https://github.com/bitcoin-core/secp256k1/pull/1774#discussion_r2539737079
It is pretty straightforward to remove `ret` and to just return either `0` or `1`
ACKs for top commit:
real-or-random:
utACK 3daab83a60
theStack:
ACK 3daab83a60
Tree-SHA512: ce598d917455a2d25297436bf2b900a9e88a638617cb79ca22e467135035c334b6815911fe4429ff44dbd877e6d10a346d0b37f2e5a7459e5b35854023832d27
f5e815f430 remove secp256k1_eckey_pubkey_serialize function (Sebastian Falbesoner)
0d3659c547 use new `_eckey_pubkey_serialize{33,65}` functions in modules (ellswift,musig) (Sebastian Falbesoner)
adb76f82ea use new `_eckey_pubkey_serialize{33,65}` functions in public API (Sebastian Falbesoner)
fc7458ca3e introduce `secp256k1_eckey_pubkey_serialize{33,65}` functions (Sebastian Falbesoner)
Pull request description:
This PR splits up the pubkey serialization function `secp256k1_eckey_pubkey_serialize` into two variants for the compressed (33 bytes) and uncompressed (65 bytes) public key output format each, where only non-infinity group elements as input are allowed. The motivation is to simplify call-sites significantly, as they currently need to introduce two variables and a VERIFY_CHECKs on the return value and the in/out size parameter within a pre-processor block, typically leading to 8 lines of code. By using the new functions, the code is reduced to a single line of code that just calls the function (see #1773). This is helpful for already existing modules on master (ellswift, musig) and upcoming ones (silentpayments, see #1765).
One drawback is that the public API function `secp256k1_ec_pubkey_serialize` is now slightly more complex (we now call one of two functions instead of a single one, depending on whether the compressed flag is set or not), but that should hopefully not be a problem.
The commits are intentionally kept small to ease review, happy to squash them if that is preferred.
(Kudos to w0xlt for the initial idea (https://github.com/bitcoin-core/secp256k1/pull/1765#pullrequestreview-3462461331) and to real-or-random for the suggestion to split the already existing function (https://github.com/bitcoin-core/secp256k1/issues/1773#issuecomment-3540461718).)
ACKs for top commit:
real-or-random:
utACK f5e815f430
w0xlt:
ACK f5e815f430
Tree-SHA512: da576bbeae477f31ba76c0001f8df08b51fe5e31d67b422a238348ead3341bf37f0c1509ad9d0a93b63e6d61c152707c85beabd02f4eac3b3bdcff129e0ea750
This change improves separation from CMake build directories, which
typically use the "build" prefix.
Additionally, corresponding `.gitignore` entries have been refactored.
26166c4f5f ecmult_multi: reduce strauss memory usage by 30% (Jonas Nick)
Pull request description:
This is a draft because I'm not sure about the cleanest way to implement it.
ACKs for top commit:
real-or-random:
ACK 26166c4f5f benchmarks show no significant difference (only tried low point counts)
siv2r:
tACK 26166c4
hebasto:
ACK 26166c4f5f, I have reviewed the code and it looks OK.
Tree-SHA512: f289daee0b0b51451331eefdd99200a78bd83539365d38465c038dc0e6ad940daf821119f7161b08a2390cf046e3859a8f950f2fe881a427aba16353031def7d
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
Add support for specifying single tests or modules to run via the
"--target" or "-t" command-line option. Multiple targets can be
provided; only the specified tests or all tests in the specified
module/s will run instead of the full suite.
Examples:
-t=<test name> runs an specific test.
-t=<module name> runs all tests within the specified module.
Both options can be provided multiple times.
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).
Calling `secp256k1_context_create` with `SECP256K1_FLAGS_TYPE_CONTEXT`
seems to be not strictly API-compliant, as the only allowed
(non-deprecated) value is `SECP256K1_CONTEXT_NONE`, even if the
former happens to map to the latter currently.
Fix this by not dynamically creating a context in the first place and
switch to using the static context, as it is sufficient for this
benchmark and presumably matches what the "no capabilities" comment
intended back then.
Relocate the clock time getter to tests_common.h to
make it easily reusable across test programs. This
will be useful for the upcoming unit test framework.
Context - why not placing it inside testutil.h?:
The bench program links against the production-compiled library,
not its own compiled version. Therefore, `gettime_i64()` cannot
be moved to testutil.h, because testutil.h calls
`secp256k1_pubkey_save()`, which exists only in the internal
secp256k1.c and not in the public API.
secp256k1_memclear has the side effect of undefining bytes for
valgrind checks. In some cases, we may want to zero bytes
but allow subsequent reads. So we split memclear into
memclear_explicit, which makes no guarantees about the content
of the buffer on return, and memzero_explicit, which guarantees
zero value on return.
Change the memset in partial_sign to use memzero_explicit.
325d65a8cf Rename and clear var containing k or -k (John Moffett)
Pull request description:
Follow-up to #1579. `buf` still holds the nonce or its negation, so ought to be cleared.
ACKs for top commit:
theStack:
Code review re-ACK 325d65a8cf
real-or-random:
utACK 325d65a8cf
Tree-SHA512: a2fe39d7c44cebc0abe712828d521c2a7aba1db2d9dc5fc811dcaf96f1e45494dba5d7f016b6d9200ab523641ff62083686dbc942284e0f548183aaf60d8bfa2
