2ee79e77e6 test: add unit test for `_ecmult_gen_ge` (Sebastian Falbesoner)
ca68daf8e1 test: refactor: simplify tests by using `_ecmult_gen_ge` helper (Sebastian Falbesoner)
Pull request description:
This PR is a small follow-up to #1861. If the generator point multiplication result in Jacobian coordinates is immediately converted to affine coordinates after and is not needed for anything else, we can deduplicate by using the new `secp256k1_ecmult_gen_ge` helper. The second commit adds a simple unit tests, verifying for random scalars that the result of `secp256k1_ecmult_gen_ge` matches the two expected steps (`secp256k1_ecmult_gen_gej` plus Jacobian->affine conersion via `secp256k1_ge_set_gej`).
Note that in a very strict sense the first commit is not a refactor, as the Jacobian object is now cleared out which was not done on master, but for the logic in the tests this shouldn't matter at all.
ACKs for top commit:
real-or-random:
utACK 2ee79e77e6
Tree-SHA512: 452895b6f6e70c686063afb051d25dab1d086aac28081c4a3071a3dbe7dae964e806f9fe8052b88a7da4305a0cf636badc2dba817cae96aae0a35b2bc9675c03
240578eef5 bench: add internal benchmark for `secp256k1_fe_normalize_var` (Sebastian Falbesoner)
Pull request description:
While addressing the review suggestion https://github.com/bitcoin-core/secp256k1/pull/1765#discussion_r3238616034 ([b10c mirror link](https://mirror.b10c.me/bitcoin-core-secp256k1/1765/#discussion_r3238616034)), I noticed that we don't have an internal benchmark for the variable-time variant of `_fe_normalize` yet, so this PR adds one. IIUC it's fine to repeatedly apply the operation on the same (already normalized at latest after the first loop iteration) field element for benchmarking purposes and don't put in an effort to reach the [final reduction code path](b11340b3ce/src/field_5x52_impl.h (L120-L132)), considering how extremely unlikely it is to reach it in practice.
Results on my machine:
```
$ ./build/bin/bench_internal normalize
Benchmark , Min(us) , Avg(us) , Max(us)
field_normalize , 0.0103 , 0.0106 , 0.0128
field_normalize_var , 0.00545 , 0.00546 , 0.00547
field_normalize_weak , 0.00352 , 0.00354 , 0.00363
```
ACKs for top commit:
real-or-random:
utACK 240578eef5
Tree-SHA512: 4480e65b24c9e3c498389c5faf807cc44ae2a421d4500dd95066f9bb4f4885c67d2a6e1875e93912b96422963fd1430f724d442f30eb152faf86302ba266bd94
If the generator point multiplication result in Jacobian coordinates is
immediately converted to affine coordinates after and is not needed for
anything else, we can deduplicate by using the helper introduced in #1861.
Note that in a very strict sense this is not a refactor, as the Jacobian
object is now cleared out which was not done on master, but for the logic
in the tests this shouldn't matter at all.
9e017e5062 refactor: rename `_ecmult_gen` -> `_ecmult_gen_gej` for consistency (Sebastian Falbesoner)
a3296d5e23 refactor: introduce `_ecmult_gen_ge` helper (preventing accidental gej leaks) (Sebastian Falbesoner)
Pull request description:
Scalar multiplication with the generator point frequently involves a conversion to affine coordinates and clearing out the temporary Jacobian group element object after to avoid leaking secret key material (see 765ef53335 / #1579 for that last part), i.e. executing the following three functions:
* `secp256k1_ecmult_gen(ctx, &rj, ...)`
* `secp256k1_ge_set_gej(&r, &rj)`
* `secp256k1_gej_clear(&rj)`
This PR introduces a corresponding helper to deduplicate code and mitigate the risk that the last step is forgotten (which can easily happen, as it would not be detected by tests). It is applied in the code paths for ECDSA signing, Schnorr signing, public key creation and ecmult_gen blinding setup. The only remaining instance where we directly call `_ecmult_gen` is for [musig nonce generation](a39093de15/src/modules/musig/session_impl.h (L416)), as we apply batch inversion there for the two points.
The idea came up during a conversation with furszy, who caught that the gej clearing was missing in the silentpayments module (sending function) as well (see https://github.com/bitcoin-core/secp256k1/pull/1765#issuecomment-4482838033, [b10c mirror link](https://mirror.b10c.me/bitcoin-core-secp256k1/1765/#issuecomment-4482838033)).
If this gets conceptual support, I'd be curious to hear naming suggestions, as I'm not sure if the current one is fits well to the existing terminology (maybe `ecmult_gen_ge` or `ecmult_gen_to_affine`?).
ACKs for top commit:
real-or-random:
utACK 9e017e5062
furszy:
ACK 9e017e5062
Tree-SHA512: e9dc96c4301622e5b258de5c2cff5bd9f27d07d3a5f881d937c3db526e2c0a7ba5772ea24585af37be359e0510136916a15b39316c7c351d068c09545c003b6e
Now that we have a function `_ecmult_gen_ge`, it makes sense to rename
the existing function `_ecmult_gen` to `_ecmult_gen_gej` for
consistency, to signal that the result is a Jacobian group element.
This diff was created by applying
```
$ sed -i s/secp256k1_ecmult_gen\(/secp256k1_ecmult_gen_gej\(/g $(git ls-files)
```
Scalar multiplication with the generator point frequently involves a
conversion to affine coordinates and clearing out the temporary Jacobian
group element object after to avoid leaking secret key material, i.e.
executing the following three steps:
- secp256k1_ecmult_gen(ctx, &rj, ...)
- secp256k1_ge_set_gej(&r, &rj)
- secp256k1_gej_clear(&rj)
This commit introduces a corresponding helper to deduplicate code
and mitigate the risk that last step is forgotten (which can easily
happen and is not detected by tests).
The idea came up during a conversation with furszy, see
https://github.com/bitcoin-core/secp256k1/pull/1765#issuecomment-4482838033
5698e66c64 Add exhaustive test for ECDH module (Sebastian Falbesoner)
Pull request description:
This PR adds an exhaustive test for the ECDH module, looping over all key combinations and verifying the commutativity property (ECDH(i\*G, j) == ECDH(j\*G, i)) and checking against a recalculated ECDH result (by manually invoking the default ECDH hash function on the precalculated group element `group[i * j]`'s coordinates). The existing test coverage is already solid (including Wycheproof test vectors), but I figured it likely wouldn't hurt to add this as well.
ACKs for top commit:
sipa:
ACK 5698e66c64
real-or-random:
utACK 5698e66c64
Tree-SHA512: e80b8508ee61e3bf5230951393a08c8937f19d2d16220ff2b02fe69b039195a1545809d3ea420dfed1f192c3711f403c63e86c3af2bb2fc4ab1254388ba50287
c5be752e2e gha: Simplify sync.yml according to sync-upstream.sh changes (Tim Ruffing)
260fad30aa sync-upstream: Optionally switch to sync branch (Tim Ruffing)
f0ff45f3ab sync-upstream: Simplify (Tim Ruffing)
Pull request description:
This is a half-rewrite of the sync-upstream script because I still wasn't happy with it. Looking for early feedback on this.
Advantages:
- The script does not hardcode any repo URLs, branches, etc.
- Nicer separation of responsibilities between the script and the GHA workflow. This brings back the usefulness of the script when used locally outside GHA.
- Much simpler code, and we could simplify also the GHA workflow.
Still to do:
- Rewrite the "Tips" section in the generated PR description, e.g., add how to resolve merge conflicts.
- Change the GHA workflow accordingly.
ACKs for top commit:
mllwchrry:
ACK c5be752
Tree-SHA512: 7335b4690642c07965366da952a74065ec404f750e0c6c7e735a9a48285cbe6b9c7db27ff4bdeba1d5ce59717d77be1ecb2334200b56a1a0a2c0fcddc7530d0b
af1fdd1215 tests: compare full MuSig aggregate nonce (w0xlt)
Pull request description:
This PR has been created by a GitHub Actions workflow without human involvement.
[bitcoin-core/secp256k1#1854]: tests: compare full MuSig aggregate nonce
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:
mllwchrry:
ACK 8363a2d
Tree-SHA512: 55406814de269c612159922ae521dd874887107ee9bd26cff81e2e2cca28ade74a0609e5c48e25a7d9913698899da146aef8b3eebf96e5ad70933a884d75a655
40a0d874a6 doc: correct API docs for ECDSA signing out-params (s/array/signature object/) (Sebastian Falbesoner)
Pull request description:
This PR is a late follow-up to https://github.com/bitcoin-core/secp256k1/pull/282, adapting the signature out param description to the API change.
This is currently a minimum-diff based on existing API doc descriptions in the touched header files (for e.g. `_ecdsa_signature_parse_{compact,der}`, `_ecdsa_recoverable_signature_parse_compact`). For more consistency across modules, one could adopt the wording used in the musig module, e.g. "pointer to a structure to store the created signature".
ACKs for top commit:
real-or-random:
utACK 40a0d874a6
furszy:
ACK 40a0d874a6
Tree-SHA512: 028e87bb77be6118264ab14dce2037b4f8dd680b9d7a2c273773633d342e7f0597332932cd2654f07ab0d151f1de367f716205af4f9bb15bb83076a10b49d06c
af1fdd1215 tests: compare full MuSig aggregate nonce (w0xlt)
Pull request description:
This PR fixes a MuSig nonce aggregation test that only compared the first 33 bytes of the serialized 66-byte aggregate nonce.
ACKs for top commit:
theStack:
ACK af1fdd1215
real-or-random:
utACK af1fdd1215
Tree-SHA512: a580bb1b43177cb2986bda2f595ec73af8fb98381fdab71b79142032c6d64d685b81963b3a7252e7772818512f6016ee8f564ff92c39d28d8e67d8afd26c96d7
8479eafa57 musig: always clear out secret key in `secp256k1_musig_nonce_gen_counter` (Sebastian Falbesoner)
3cca6451a2 ci: Bump GCC snapshot major version to 17 (Hennadii Stepanov)
285cb788e9 ci: Replace `ilammy/msvc-dev-cmd` with manual MSVC setup (Hennadii Stepanov)
Pull request description:
This PR has been created by a GitHub Actions workflow without human involvement.
[bitcoin-core/secp256k1#1846]: ci: Replace `ilammy/msvc-dev-cmd` with manual MSVC setup
[bitcoin-core/secp256k1#1848]: ci: Bump GCC snapshot major version to 17
[bitcoin-core/secp256k1#1849]: musig: always clear out secret key in `secp256k1_musig_nonce_gen_counter`
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:
mllwchrry:
ACK b11340b. This sync PR can be merged as-is.
DarkWindman:
ACK b11340b3ce
Tree-SHA512: 4b311e9bfa21f00b4780202c08af9d00380f5b3df40704641764d706cfc3408615b6206c7c82ca915b56c27a354bdf24680674269a28eefe7d4e93adc06cbaa5
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
baac08d207 modules, tests: Port bitcoin-core/secp256k1#1777 and bitcoin-core/secp256k1#1824 to zkp-specific code (DarkWindman)
c5cd9d6d9a gha: Bump deprecated GHA workflow dependencies (Tim Ruffing)
43fca0ff55 ecdsa: VERIFY_CHECK result of _fe_set_b32_limit (Tim Ruffing)
b84635ed3b tests: Fix C89 function pointer initialization in ellswift tests (mllwchrry)
3a403639dc eckey: Call ecmult with NULL instead of zero scalar (Tim Ruffing)
7e68c0c88b ecmult: Document and test ng=NULL in ecmult (Tim Ruffing)
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)
921b9711ea util: introduce and use `ARRAY_SIZE` macro (Sebastian Falbesoner)
Pull request description:
[https://github.com/bitcoin-core/secp256k1/pull/1824]: util: introduce and use ARRAY_SIZE macro
[https://github.com/bitcoin-core/secp256k1/pull/1777]: Make SHA256 compression runtime pluggable
[https://github.com/bitcoin-core/secp256k1/pull/1834]: ecmult: Document and test ng=NULL in ecmult
[https://github.com/bitcoin-core/secp256k1/pull/1837]: tests: Fix function pointer initialization C89 error in ellswift tests
[https://github.com/bitcoin-core/secp256k1/pull/1839]: ecdsa: VERIFY_CHECK result of _fe_set_b32_limit
[https://github.com/bitcoin-core/secp256k1/pull/1841]: gha: Bump deprecated GHA workflow dependencies
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:
mllwchrry:
ACK baac08d
real-or-random:
ACK baac08d207
Tree-SHA512: 082c9b52ae79716b8770fabd4956f4253ad7eeadd3242aed040178d1149e5e4c92f02af2829b110bebb14c44f8de00a9d9d51646565c9e0b25df0ba9fc148f75
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
e371ab5df0 sync-upstream: Restore SYNC_PAT for gh commands (DarkWindman)
Pull request description:
This PR restores the `SYNC_PAT` requirement due to restrictions on opening PRs with the default 'GITHUB_TOKEN'.
Since the authorship of the PR is now linked to the user who created the PAT, a note was added to the PR body to clarify that it was created by a GitHub Actions workflow and not by a human being.
Additionally, the `permissions:` section has been removed, as `GITHUB_TOKEN` is no longer in use.
ACKs for top commit:
real-or-random:
ACK e371ab5df0
Tree-SHA512: e61fde9538edc95a049e1208894c5cafe2295180cc6d02e35a364974e04aba913c39256ac7293685919650021d97a1737075f782c11383b761f0979f8bd70e8a
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
b96b655a82 include: fix a minor grammar mistake in the rangeproof description (Mykyta Redko)
Pull request description:
ACKs for top commit:
real-or-random:
ACK b96b655a82
Tree-SHA512: 96bfa2aaaf3779337fdc7aa8c8edefa00340ca10ee268dbc4b78cd3a14b99e3527a58c196cf29cc848617756a58b2ae34445fb44e3a8f386d9e6d3f72ad4fbc6
efa4e11b65 sync-upstream: Use GITHUB_TOKEN for gh commands (DarkWindman)
Pull request description:
Optimize token usage in the sync workflow.
**Token usage:**
- **PAT** : Used for `git push` operations that modify workflow files
- `GITHUB_TOKEN`: Used for GitHub CLI commands (`gh repo set-default`, `gh pr create`)
ACKs for top commit:
real-or-random:
ACK efa4e11b65
Tree-SHA512: 9deb68d74e69ac1a3f6785f04132aef8c3357d326e9e9efe9a86842db12972a46f81f124dd3ddda7425ebd3ddd86cfd4905006fe20ae9d9ca0f0be07bca64891
