c09215f7af bench: fail early if user inputs invalid value for SECP256K1_BENCH_ITERS (kevkevinpal)
Pull request description:
### Description
Motivated by https://github.com/bitcoin-core/secp256k1/pull/1793#issuecomment-3719488071
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
### Before
```
secp256k1 $ SECP256K1_BENCH_ITERS=abc ./build/bin/bench
Benchmark , Min(us) , Avg(us) , Max(us)
Floating point exception (core dumped)
```
### After
```
secp256k1 $ SECP256K1_BENCH_ITERS=abc ./build/bin/bench
Invalid value for SECP256K1_BENCH_ITERS must be a positive integer: abc
Benchmarks the following algorithms:
- ECDSA signing/verification
- ECDH key exchange (optional module)
- Schnorr signatures (optional module)
- ElligatorSwift (optional module)
The default number of iterations for each benchmark is 20000. This can be
customized using the SECP256K1_BENCH_ITERS environment variable.
Usage: ./bench [args]
By default, all benchmarks will be run.
args:
help : display this help and exit
ecdsa : all ECDSA algorithms--sign, verify, recovery (if enabled)
ecdsa_sign : ECDSA siging algorithm
ecdsa_verify : ECDSA verification algorithm
ec : all EC public key algorithms (keygen)
ec_keygen : EC public key generation
ecdh : ECDH key exchange algorithm
schnorrsig : all Schnorr signature algorithms (sign, verify)
schnorrsig_sign : Schnorr sigining algorithm
schnorrsig_verify : Schnorr verification algorithm
ellswift : all ElligatorSwift benchmarks (encode, decode, keygen, ecdh)
ellswift_encode : ElligatorSwift encoding
ellswift_decode : ElligatorSwift decoding
ellswift_keygen : ElligatorSwift key generation
ellswift_ecdh : ECDH on ElligatorSwift keys
```
ACKs for top commit:
hebasto:
re-ACK c09215f7af.
real-or-random:
utACK c09215f7af
Tree-SHA512: 356df69e356db0b201339d40a6ffbcf29e4b7cc1e6aa82c00e1e7a2a7d11c47dd9c51baabcc63cabcff2ab42e2746a3cab659205f871a85122edda4a599d56c8
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
29ac4d8491 sage: verify Eisenstein integer connection for GLV constants (Justsomebuddy)
Pull request description:
## Summary
Add assertions to `gen_split_lambda_constants.sage` to verify that the GLV decomposition constants arise from the Eisenstein integer factorization of the group order N.
Specifically:
- `N = a^2 + a*b + b^2` (norm equation in Z[ω])
- `λ = b/a mod N` (eigenvalue from Z[ω]/(π) ≅ Z/NZ isomorphism)
This addresses the suggestion in #1798 to document/verify the algebraic origin of these constants in the sage script rather than C comments.
## Details
The group order N factors as N = π·π̄ in the Eisenstein integers Z[ω], where:
- ω = (-1 + √-3)/2 is a primitive cube root of unity
- π = a - b·ω is an Eisenstein prime with norm N(π) = a² + ab + b²
The GLV constants (A1, B1) correspond to the Eisenstein factors (b, -a), and the endomorphism eigenvalue λ arises naturally as the image of ω under the quotient map Z[ω] → Z[ω]/(π) ≅ Z/NZ.
Closes#1798
ACKs for top commit:
real-or-random:
utACK 29ac4d8491
Tree-SHA512: 6c36dacac00baf513db447a14f49c91d434c80ed79f9282d080938e3e53d39f0b68d07d62900da648d817eba3777505e9ef9306bc129f4521f524b4c64bcda49
Add assertions to verify that the GLV decomposition constants arise
from the Eisenstein integer factorization of the group order N.
The group order factors as N = pi * conj(pi) in Z[w], where pi = A - B*w
is an Eisenstein prime. The GLV eigenvalue LAMBDA = B/A mod N, which is
the image of w^2 under the isomorphism Z[w]/(pi) -> Z/NZ.
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
0406cfc4d1 doc: include arg -DUSE_EXTERNAL_DEFAULT_CALLBACKS=1 for cmake (kevkevinpal)
Pull request description:
### Motivation
This is motivated by this comment https://github.com/bitcoin-core/secp256k1/pull/1778#pullrequestreview-3578687144
### Rationale
It makes sense to add documentation on how to configure for CMake. I can reword if other wording is preferred
ACKs for top commit:
hebasto:
ACK 0406cfc4d1.
real-or-random:
ACK 0406cfc4d1
Tree-SHA512: 06b8cc84fc2c080045eb1c16af6b236b5d7472696d2488821c64a07b55a12c32e77c65c001a7223e92edeb6150bf15b7a7367dd254307c7cb91debdb924574f0
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
d822b29021 test: split monolithic ellswift test into independent cases (furszy)
Pull request description:
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, I'm seeing 35-40% tests time reduction locally.
This is quite useful for Debug builds with no optimizations,
which are 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)
#### rp5 Release-build results (5 jobs):
- master: 39 seconds.
- this PR: 24 seconds.
(~1.62× speedup, ~38% reduction)
ACKs for top commit:
hebasto:
re-ACK d822b29021.
theStack:
ACK d822b29021
Tree-SHA512: b3d93f183c5eb5856a146c38719a8ca7ac42ac2a521b3cae3da6950881eb1745a047df9790db2b60dd56501c5ba40fc78e89d0c9b4db777f32520ed3c27ab9d1
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
8bcda186d2 test: Add non-NULL checks for "pointer of array" API functions (Sebastian Falbesoner)
5a08c1bcdc Add ARG_CHECKs to ensure "array of pointers" elements are non-NULL (Sebastian Falbesoner)
Pull request description:
We currently have five public API functions that take an "array of pointers" as input parameter:
* `secp256k1_ec_pubkey_combine` (`ins`: array of pointers to public keys to add)
* `secp256k1_ec_pubkey_sort` (`pubkeys`: array of pointers to public keys to sort)
* `secp256k1_musig_pubkey_agg` (`pubkeys`: array of pointers to public keys to aggregate)
* `secp256k1_musig_nonce_agg` (`pubnonces`: array of pointers to public nonces to aggregate)
* `secp256k1_musig_partial_sig_agg` (`partial_sigs`: array of pointers to partial signatures to aggregate)
Out of these, only `_ec_pubkey_combine` verifies that the individual pointer elements in the array are non-NULL each:
e7f7083b53/src/secp256k1.c (L774-L775)
This PR adds corresponding `ARG_CHECKS` for the other API functions as well, in order to avoid running into potential UB due to NULL pointer dereference. It seems to me that the tiny run-time overhead is worth it doing this for consistency and to help users in case the arrays are set up incorrectly (I'm thinking e.g. of language binding writers where getting this right might be a bit more involved).
Looking into this was motivated by a [review of furszy](https://github.com/bitcoin-core/secp256k1/pull/1765#discussion_r2590644412) (thanks!), who pointed out that the non-NULL checks are missing in at least one API function in the silentpayments module PR as well. Happy to add some `CHECK_ILLEGAL` tests if there is conceptual support for this PR.
ACKs for top commit:
kevkevinpal:
utACK [8bcda18](8bcda186d2)
john-moffett:
utACK 8bcda186d2
real-or-random:
utACK 8bcda186d2
w0xlt:
ACK 8bcda186d2
Tree-SHA512: 24acd6606526e3acb994e3361fde15771aa6706a6f3e7a6ae70b9a9ddb81ac1eedaac2025a027b890cecf98dab20dc378b94edde6c726888c44b9d35b7581ee1
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
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
f252da7e6e ci: Use Python virtual environment in "x86_64-macos-native" job (Hennadii Stepanov)
Pull request description:
Fixes https://github.com/bitcoin-core/secp256k1/issues/1768.
This PR explicitly sets up a virtual environment instead of using `uv`, as suggested [here](https://github.com/bitcoin-core/secp256k1/issues/1768#issue-3599176194), for the following reasons:
1. It does not require granting a third-party action access to the repository.
2. This approach is already used in other parts of the CI framework (it’s unclear why it was missed in https://github.com/bitcoin-core/secp256k1/pull/1359 in the first place).
ACKs for top commit:
real-or-random:
ACK f252da7e6e
Tree-SHA512: df167db9b69ea4a055565c64ea0daa8c0d7b6a12510c35c22309f00a192a8cc4fbbdb0920bacd25547ea8c8f6a7853c2603be35234bd2bac88f610ea848f6120
153eea20c2 bench: Use `ALIGNMENT` macro instead of hardcoded value (Hennadii Stepanov)
Pull request description:
This PR brings consistency with the rest of the code and appears more correct.
ACKs for top commit:
real-or-random:
utACK 153eea20c2
Tree-SHA512: 0fc61a390205ef29b99cfce9cb0d365930b7a93aa0f2aaaa00bfd9e0fc34ac928a3fe13b096f394b7d35e95e9ede7c47ecb054de2846be1e1aa33a84d14942cf
c09519f0e3 ci: Drop workaround for Valgrind older than 3.20.0 (Hennadii Stepanov)
Pull request description:
This is no longer needed in the current CI framework.
If someone runs the CI scripts locally, it is reasonable to expect that they are using up-to-date tools, including Valgrind.
ACKs for top commit:
real-or-random:
ACK c09519f0e3
Tree-SHA512: ef840dc8fd6e29110e194e35139050d9720bf8b38e89497790dcf5f2f2ce603805b572fb7408b4ae47bda31289a05548671ac41923f6ab3d1fbf288855f058d0
8bc50b72ff ci: Switch to macOS 15 Sequoia Intel-based image (Hennadii Stepanov)
Pull request description:
This is an alternative to https://github.com/bitcoin-core/secp256k1/pull/1755.
More details from the GHA developers:
> Apple has discontinued support for the x86_64 (Intel) architecture going forward. GitHub will no longer support this architecture on macOS after the macOS 15 runner image is retired in Fall 2027.
ACKs for top commit:
real-or-random:
ACK 8bc50b72ff
Tree-SHA512: 2c3de907b8910193b77bb201e1c7aad3f70f89804efe4b3ead23181d45a912f677cd533e819914cf1b346aa01d2f273fd5f1e63d7b8d284320d1e4e6198d8578
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
15d014804e ci: Drop default for `inputs.command` in `run-in-docker-action` (Hennadii Stepanov)
1decc49a1f ci: Use YAML anchor and aliases for repeated "CI script" steps (Hennadii Stepanov)
dff1bc107d ci, refactor: Generalize use of `matrix.configuration.env_vars` (Hennadii Stepanov)
4b644da199 ci: Use YAML anchor and aliases for repeated "Print logs" steps (Hennadii Stepanov)
a889cd93df ci: Bump `actions/checkout` version (Hennadii Stepanov)
574c2f3080 ci: Use YAML anchor and aliases for repeated "Checkout" steps (Hennadii Stepanov)
Pull request description:
GHA YAML parser now [supports](https://github.com/actions/runner/issues/1182#issuecomment-3156285802) anchors.
This PR makes use of that support to [DRY](https://en.wikipedia.org/wiki/Don%27t_repeat_yourself) the workflow code.
ACKs for top commit:
real-or-random:
utACK 15d014804e
Tree-SHA512: a25a226fec0053242bc46b8c9815067a35af632cfbffefcc5cd4c96a67c0535dde447753099cbc74ecc64072d36aef2aa78c105b66f43cb3134ffa1ae60dca1e
53585f93b7 ci: Use clang-snapshot in "MSan" job (Hennadii Stepanov)
6894c964f3 Fix Clang 21+ `-Wuninitialized-const-pointer` warning when using MSan (Hennadii Stepanov)
Pull request description:
In Bitcoin Core, the "MSan" CI jobs use the latest tagged Clang available from http://apt.llvm.org.
This PR applies similar changes and switches the "MSan" CI jobs to clang-snapshot.
This exposes problematic code that was reported in https://github.com/bitcoin/bitcoin/issues/33284.
ACKs for top commit:
real-or-random:
utACK 53585f93b7
Tree-SHA512: 79bc10f1d0a60ed67b518eb8fab9a48146a4ef1fff95c8775717be3a950b323ae89a12999504ed8446f164da426894abe02f9fb61b5ca19453d549a34873b73b
f163c35897 ci: Set `DEBIAN_FRONTEND=noninteractive` (Hennadii Stepanov)
70ae177ca0 ci: Bump `docker/build-push-action` version (Hennadii Stepanov)
b2a95a420f ci: Drop `tags` input for `docker/build-push-action` (Hennadii Stepanov)
122014edb3 ci: Add `scope` parameter to `cache-{to,from}` options (Hennadii Stepanov)
Pull request description:
This PR fixes an issue where only the latest image cache was available.
For other minor improvements, see the individual commit messages.
ACKs for top commit:
real-or-random:
utACK f163c35897
Tree-SHA512: 7178c447d32e5c06e42d33ed32c9088fc19ca6a67369f2a8f6672b0ec010a516d4bb3a70a1847eec76e034ec22d6df778f6d421a04ba603ae18526a6f4104e65
