8479eafa57 musig: always clear out secret key in `secp256k1_musig_nonce_gen_counter` (Sebastian Falbesoner)
Pull request description:
Even though `secp256k1_musig_nonce_gen_internal` can currently only fail if the surrounding API function is misused (invalid `keypair` or `keyagg_cache` parameters, making the corresponding [seckey validation](c1a9e4fe64/src/modules/musig/session_impl.h (L391)) or [pubkey](c1a9e4fe64/src/modules/musig/session_impl.h (L397)) load calls fail), clearing out the stack memory holding the secret key as well in this case seems reasonable to follow best practices.
The issue was reported off-band by l0rinc (thanks!), in the course of analyzing the secp repository with AI tooling.
ACKs for top commit:
furszy:
ACK 8479eafa57
real-or-random:
utACK 8479eafa57
Tree-SHA512: dc15ed7518c6cd0b1b86d2e0382c546374e94a1c1fa15639ba3db27e083ce53a24ddf4d3cd3328b4dc229258d8cbb0e01f4b63f025d04254845f2bf20cfa5289
Even though `secp256k1_musig_nonce_gen_internal` can currently only fail
if the API is misused (invalid `keypair` or `keyagg_cache` parameters),
clear out the buffer holding secret key data as well in this case to
follow best practices.
The issue was found and reported by l0rinc using GPT 5.5 (Thanks!).
3cca6451a2 ci: Bump GCC snapshot major version to 17 (Hennadii Stepanov)
Pull request description:
See https://gcc.gnu.org/pipermail/gcc/2026-April/248048.html.
ACKs for top commit:
real-or-random:
utACK 3cca6451a2
Tree-SHA512: 36c975c500cb0411f20189a3b451b58268ff15be08ff444833e27ae37b53bf7581ba8a8c48b393a9b96050f8c498242ba2fb5e593caee492f1fcb1182cc948bc
285cb788e9 ci: Replace `ilammy/msvc-dev-cmd` with manual MSVC setup (Hennadii Stepanov)
Pull request description:
The `ilammy/msvc-dev-cmd` repository seems [abandoned](https://github.com/ilammy/msvc-dev-cmd/issues/103) and should be considered unsafe.
This PR updates the workflow to load the MSVC environment variables directly via [`vcvars64.bat`](https://learn.microsoft.com/en-us/cpp/build/building-on-the-command-line).
For reference, the Bitcoin Core project removed `ilammy/msvc-dev-cmd` in https://github.com/bitcoin/bitcoin/pull/32513.
**Note for Maintainers:** Once this PR is merged and other PRs are rebased on top of it, the `ilammy/msvc-dev-cmd` action should be removed from the "Action permission" settings in this repository.
ACKs for top commit:
real-or-random:
utACK 285cb788e9
Tree-SHA512: 3faa9a316438ae3f4e7352890a77baaf0bf0adda4086111344d8279dc869a42ea837269527268fad1a2dd2e1893fd8fc51e5c3b205f394926b07988579a10ad9
43fca0ff55 ecdsa: VERIFY_CHECK result of _fe_set_b32_limit (Tim Ruffing)
Pull request description:
This also avoids a spurious `-Wmaybe-uninitialized` warning emitted by gcc 16 (snapshot) when compiling with `-DDETERMINISTIC`.
Alternative to #1838 by @mllwchrry who tried very a similar thing as this PR but couldn't convince the compiler. (The GCC snapshot is very annoying: a simple `VERIFY_CHECK(secp256k1_fe_set_b32_limit(&xr, c))` doesn't do the trick. I found this variant here with a local store rather by accident.)
ACKs for top commit:
mllwchrry:
ACK 43fca0f
theStack:
utACK 43fca0ff55
Tree-SHA512: 2550043e953675db7614f98bbdffb706721834967ef36f7c905f7cbfeee5d88189a9acfcd64865ef822bb0e3272d228440bdfb1124228afe083e025056e53212
b84635ed3b tests: Fix C89 function pointer initialization in ellswift tests (mllwchrry)
Pull request description:
Fixes a C89 pedantic compliance error in `src/modules/ellswift/tests_impl.h` where function pointer array initialization is not allowed at declaration time.
This error was exposed while I was testing the improved test coverage in CI. The initial plan was to simplify the configuration of modules in CI by enabling all modules by default and testing the disabling of each module independently.
Error: src/modules/ellswift/tests_impl.h:442:110: error: initializer element is not computable at load time [-Wpedantic].
The error occurred when running the `x86_64_debian` GitHub Actions CI job, which uses GCC 16 (snapshot) with strict flags (-std=c89 -pedantic -pedantic-errors -Werror). See this action run for reference: https://github.com/mllwchrry/secp256k1/actions/runs/23301905657/job/67769464566.
The fix uses `if/else` to assign function pointers after declaration, matching the pattern already used in the same file.
While this is a minor C89 compliance issue, it blocks the potential CI simplification.
ACKs for top commit:
real-or-random:
utACK b84635ed3b
theStack:
ACK b84635ed3b
Tree-SHA512: 61e42afe9c3a215f817b1bf475ea66c103b4af6c598a9d7ee9e1a97789ac6f4e025260b4cd0e2ec219bd73706c7aa2799c58ab904916d9594626cf3c07e4b983
3a403639dc eckey: Call ecmult with NULL instead of zero scalar (Tim Ruffing)
7e68c0c88b ecmult: Document and test ng=NULL in ecmult (Tim Ruffing)
Pull request description:
ACKs for top commit:
theStack:
re-ACK 3a403639dc
Tree-SHA512: 954928d4dfa120845c6e899c1a69ad0408072809551d42735eac491b8bc41249eb25d7c57cfa4f44763167620b5cb78639b5c396c0a342c47b0afc48a088c755
4d92a083bc sha256: speed up writes using multi-block compression (furszy)
0753f8b909 Add API to override SHA256 compression at runtime (furszy)
fdb6a91a5e Introduce hash context to support pluggable SHA256 compression (furszy)
Pull request description:
Tackling the long-standing request #702.
Right now we ship our own SHA256 implementation, a standard baseline version that does not take advantage of any hardware-optimized instruction, and it cannot be accessed by the embedding application - it is for internal usage only.
This means embedding applications often have to implement or include a different version for their use cases, wasting space on constrained environments, and in performance-sensitive setups it forces them to use a slower path than what the platform provides. Many projects already rely on tuned SHA-NI / ARMv8 / or other hardware-optimized code, so always using the baseline implementation we ship within the library is not ideal.
These changes allow users to supply their own SHA256 compression function at runtime, while preserving the existing default behavior for everyone else. This is primarily intended for environments where the available SHA256 implementation is detected dynamically and recompiling the library with a different implementation is not feasible (equivalent build-time functionality will come in a follow-up PR).
It introduces a new API:
```C89
secp256k1_context_set_sha256_transform_callback(ctx, fn_transform)
```
This function installs the optimized SHA256 compression into the `secp256k1_context`, which is then used by all internal computations. Important: The provided function is verified to be output-equivalent to the original one.
As a quick example, using this functionality in Bitcoin-Core will be very straightforward: f68bef06d9
ACKs for top commit:
real-or-random:
ACK 4d92a083bc
w0xlt:
ACK 4d92a083bc
theStack:
ACK 4d92a083bc
Tree-SHA512: 058e2e82071f1ca77254b684458292c621e60d65bbcc5500574429717e7db75bc9f3221129fafd11eb5d33e666a5efec5e9844460d3b194ef3b6b16f2df28fb9
921b9711ea util: introduce and use `ARRAY_SIZE` macro (Sebastian Falbesoner)
Pull request description:
This PR is another tiny improvement found while working on #1765, with the goal to avoid code repetition.
The `ARRAY_SIZE` macro definition is pretty wide-spread in C projects and e.g. matches the one [used in the Linux Kernel](9702969978/include/linux/array_size.h (L11)) (without the additional check to reject pointers, as we would need GNU C for that, see e.g. https://stackoverflow.com/a/19455169; not sure if a useful counterpart exists that only relies on C89). Replacement instances were identified via `$ git grep sizeof.*/.*sizeof`.
ACKs for top commit:
w0xlt:
ACK 921b9711ea
real-or-random:
utACK 921b9711ea
Tree-SHA512: 44b6bf0132cf00fade526a3fc04e03dc896d04874123614c032206b61f97c81f94d139b6cc0c108eceaa699251580c19420d230b3150607303ca2cb7ab9a0bcb
This introduces `secp256k1_context_set_sha256_compression()`,
which allows users to provide their own SHA256 block-compression
function at runtime.
This is useful in setups where the fastest implementation can only
be determined dynamically based on the available CPU features, and
rebuilding the library is not possible.
The callback is installed on the `secp256k1_context` and is then used
by all operations that compute SHA256 hashes. As part of the setup,
the library performs sanity checks to ensure that the supplied
function is equivalent to the default transform.
Passing NULL to the callback setter restores the built-in
implementation.
This is purely a mechanical change with no behavior change.
It introduces a secp256k1_hash_ctx struct inside secp256k1_context
and propagates it to all SHA256-related operations.
This sets up the ability to provide a hardware-optimized SHA256
compression function at runtime in a follow-up commit.
c49c9be504 bench: Update help functions in bench and bench_internal (kevkevinpal)
Pull request description:
### Motivation
This change is motivated by https://github.com/bitcoin-core/secp256k1/pull/1793#pullrequestreview-3644885897
> While aligning implementation across all benchmarks, argv could be passed to the help() in bench.c and bench_internal.c.
### Description
In the `bench` and `bench_internal` `help` functions `argv` was not being passed. In this change, we pass in argv and use it in the help text.
ACKs for top commit:
real-or-random:
ACK c49c9be504
Tree-SHA512: 77184db4bf5c16827f19d888af73939f4139cc2e84ae5256d995cf61f606d5865928480fc009a0185e1a6843f3c38dd1b858d1316e524c9b165459c7367f2318
8d0eda07e9 testrand: Remove testrand_finish (Tim Ruffing)
Pull request description:
This removes printing of the "random run = " at the end of the tests. I haven't seen a single case where this proved to be useful. And as of 48789dafc2, this is anyway printed only at the end of the exhaustive tests and not the normal tests, so the probability that this will be useful in the future is very low.
ACKs for top commit:
sipa:
ACK 8d0eda07e9
Tree-SHA512: e0a688e2c81afbf7a11204f1be71b472eb3ec23086c7dc742a069b7ddfc837fcf9ade9e04f8c3f79e8b07d38d05bf4979f6e3ca68a480e45de0c1ecb94b0a6f5
This removes printing of the "random run = " at the end of the tests. I
haven't seen a single case where this proved to be useful. And as of
48789dafc2, this is anyway printed only at
the end of the exhaustive tests and not the normal tests, so the
probability that this will be useful in the future is very low.
f48b1bfa5d hash: add midstate initializer and use it for tagged hashes (w0xlt)
Pull request description:
Each tagged hash midstate function (e.g., `secp256k1_schnorrsig_sha256_tagged`) calls `secp256k1_sha256_initialize` before immediately overwriting every field it sets: `s[0]` through `s[7]` and `bytes`. The `buf[64]` member does not need initialization either, because `bytes` is set to 64, which means the buffer position (`bytes & 0x3F`) (`= bytes % 64`) is 0, so buf is always written before being read.
Remove the 11 redundant `secp256k1_sha256_initialize` calls across the `schnorrsig`, `ellswift`, and `musig` modules.
ACKs for top commit:
real-or-random:
utACK f48b1bfa5d
theStack:
Code-review ACK f48b1bfa5d
Tree-SHA512: 769beb96f3921cc3c180ed0d17484ffa0dc78041c889a8e56603679d8eaca5fe13e63759ada78f83d8e0ff7aae392e6bcbc1a9fe8b959105ea4a3d8ef51abf15
Introduce secp256k1_sha256_initialize_midstate() in the hash layer and use it at all tagged-hash midstate call sites across schnorrsig, musig, and ellswift.
Document the byte-counter contract at the declaration site in hash.h and add run_sha256_initialize_midstate_tests() to directly verify helper behavior against initialize_tagged.
Also switch the helper to take const uint32_t state[8] to reduce argument-order risk at call sites.
76e92cfeea Revert "ci, docker: Fix LLVM repository signature failure" (Hennadii Stepanov)
Pull request description:
This reverts commit 0ffb1749a5, as the underlying [issue](https://github.com/llvm/llvm-project/issues/153385) has been resolved.
ACKs for top commit:
real-or-random:
ACK 76e92cfeea
Tree-SHA512: 3cab40ab5d3c1d180b81414ec212481468898ec36dba22acce5fd0dc0b506c0beefc5d9df27bf9e94c1aa006ba18f70072bb1fcbc31acdddaead678009f82c19
b99a94c382 Add tests for bad scalar inputs in ellswift XDH (gzJx0DuTRHytnHe7P5RmMbPf3wKy2BztweVGXTf)
307b49f1b9 ellswift: fix overflow flag handling in secp256k1_ellswift_xdh (gzJx0DuTRHytnHe7P5RmMbPf3wKy2BztweVGXTf)
Pull request description:
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.
Previous PR to the wrong repository: https://github.com/bitcoin/bitcoin/pull/34558
ACKs for top commit:
kevkevinpal:
ACK b99a94c382
real-or-random:
utACK b99a94c382
theStack:
re-ACK b99a94c382
Tree-SHA512: 6222cd7616c7429f4c05180257f925720b7f9743fa440667a2327f94cb134a160cdf498dca1713ffc470ab3a6ca3275aafbd14b2e790766fe10ddb5ce6970e80
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.
ed02466d3f ci: Load Docker image by ID from builder step (Hennadii Stepanov)
Pull request description:
Fixes loading wrong Docker images. For instance, see https://github.com/bitcoin-core/secp256k1/pull/1821#issuecomment-3899080578.
ACKs for top commit:
real-or-random:
utACK ed02466d3f
Tree-SHA512: 4de31bebe64d2b2adfbc5e1f2cbdea5e609a5640d17949bfe5aef9071948693ae7d8ac81772dd9620b101a72b553f38511b882119987e3c8342b6544571eca93
In the bench and bench_internal help functions argv was not being
passed, in this change we pass in argv[0] and use it in the help text.
Additionally instead of passing all of argv in bench_ecmult we now
just pass argv[0] and is used as the executable_path variable.
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
3ae72e7867 ci: Disable Docker build summary generation (Hennadii Stepanov)
Pull request description:
The generated Docker build [summaries](https://github.com/bitcoin-core/secp256k1/actions/runs/21595861407) provide little practical value to the development workflow and clutter the CI output.
This PR disables them.
ACKs for top commit:
real-or-random:
utACK 3ae72e7867
Tree-SHA512: 0b28520765d5aa1c43ae7025c9be082742bc3784f743b4983947236bceb0255b2fa82cdf81d284470eeb83bda72b442019e051048319681bff09ac190d9b52f6
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("`
