CI / x86_64: Linux (Debian stable) (clang, map[env_vars:map[CFLAGS:-O1 ECDH:yes ELLSWIFT:yes EXTRAKEYS:yes MUSIG:yes RECOVERY:yes SCHNORRSIG:yes]]) (push) Has been cancelled
CI / x86_64: Linux (Debian stable) (clang, map[env_vars:map[ELLSWIFT:yes EXTRAKEYS:yes MUSIG:yes RECOVERY:yes SCHNORRSIG:yes WIDEMUL:int128]]) (push) Has been cancelled
CI / x86_64: Linux (Debian stable) (clang-snapshot, map[env_vars:map[BENCH:no BUILD:distcheck CTIMETESTS:no WITH_VALGRIND:no]]) (push) Has been cancelled
CI / x86_64: Linux (Debian stable) (clang-snapshot, map[env_vars:map[CFLAGS:-O1 ECDH:yes ELLSWIFT:yes EXTRAKEYS:yes MUSIG:yes RECOVERY:yes SCHNORRSIG:yes]]) (push) Has been cancelled
CI / x86_64: Linux (Debian stable) (clang-snapshot, map[env_vars:map[ELLSWIFT:yes EXTRAKEYS:yes MUSIG:yes RECOVERY:yes SCHNORRSIG:yes WIDEMUL:int128]]) (push) Has been cancelled
CI / x86_64: Linux (Debian stable) (gcc, map[env_vars:map[CFLAGS:-O1 ECDH:yes ELLSWIFT:yes EXTRAKEYS:yes MUSIG:yes RECOVERY:yes SCHNORRSIG:yes]]) (push) Has been cancelled
CI / x86_64: Linux (Debian stable) (gcc, map[env_vars:map[ELLSWIFT:yes EXTRAKEYS:yes MUSIG:yes RECOVERY:yes SCHNORRSIG:yes WIDEMUL:int128]]) (push) Has been cancelled
CI / x86_64: Linux (Debian stable) (gcc-snapshot, map[env_vars:map[BENCH:no BUILD:distcheck CTIMETESTS:no WITH_VALGRIND:no]]) (push) Has been cancelled
CI / x86_64: Linux (Debian stable) (gcc-snapshot, map[env_vars:map[CFLAGS:-O1 ECDH:yes ELLSWIFT:yes EXTRAKEYS:yes MUSIG:yes RECOVERY:yes SCHNORRSIG:yes]]) (push) Has been cancelled
CI / x86_64: Linux (Debian stable) (gcc-snapshot, map[env_vars:map[ELLSWIFT:yes EXTRAKEYS:yes MUSIG:yes RECOVERY:yes SCHNORRSIG:yes WIDEMUL:int128]]) (push) Has been cancelled
CI / i686: Linux (Debian stable) (clang --target=i686-pc-linux-gnu -isystem /usr/i686-linux-gnu/include, map[env_vars:map[]]) (push) Has been cancelled
CI / MSan (clang, map[env_vars:map[CFLAGS:-fsanitize=memory -fsanitize-recover=memory -fsanitize-memory-param-retval -g CTIMETESTS:no]]) (push) Has been cancelled
CI / MSan (clang, map[env_vars:map[CFLAGS:-fsanitize=memory -fsanitize-recover=memory -g -O3 CTIMETESTS:yes ECMULTGENKB:2 ECMULTWINDOW:2]]) (push) Has been cancelled
CI / MSan (clang-snapshot, map[env_vars:map[CFLAGS:-fsanitize=memory -fsanitize-recover=memory -fsanitize-memory-param-retval -g CTIMETESTS:no]]) (push) Has been cancelled
CI / MSan (clang-snapshot, map[env_vars:map[CFLAGS:-fsanitize=memory -fsanitize-recover=memory -g -O3 CTIMETESTS:yes ECMULTGENKB:2 ECMULTWINDOW:2]]) (push) Has been cancelled
72867fd682 tests: Port hash context tests to zkp modules (mllwchrry)
Pull request description:
Ports the hash context tests from bitcoin-core/secp256k1#1777 to the zkp-specific modules.
ACKs for top commit:
apoelstra:
ACK 72867fd682279ff2c79cf13f4f8d8484048d2527; successfully ran local tests
Tree-SHA512: 1714a4c5fd793b32a0f00a92670a48709e4d01917bb81739428601c6e88e89f871cdc1294024e6df7272d6c16e681309d9b3448adcd07ce37bfa2be2dc3f4172
598e22dcde whitelist: document the degenerate W = -P_i destination (DarkWindman)
7ea12c2dca whitelist: document that the parsed key count is untrusted (DarkWindman)
e8c3396597 whitelist: honour the documented parse initialization guarantee (DarkWindman)
1de3864ff9 bppp: check for overflow in generator allocation (DarkWindman)
Pull request description:
Small defensive fixes + docs for whitelist/bppp:
- bppp: overflow check in `generators_create` allocation.
- whitelist: signature_parse always leaves `sig` in a valid-or-canonically-invalid state; guard serialize against untrusted `n_keys`.
- whitelist: document that `n_keys` is attacker-controlled and must not be used to size the arrays passed to `_verify`.
- whitelist: document the accepted degenerate `W = -P_i` destination.
No API/format changes. New tests cover theallocation overflow and the parse-failure guarantee.
ACKs for top commit:
apoelstra:
ACK 598e22dcde2152003c6a60f04196cc5fe2e2a090; successfully ran local tests
mllwchrry:
ACK 598e22d.
Tree-SHA512: 4c218775a4c2e53091f3ac11e5d93caf17083216afa167fb5c1a1cc38c0a7a2cb1e0564cf9e63c5391d72aba9fc42d90a64b9a5f288ae763f0bf9b4b20f3b2e1
a69a662d05 Fix missing clears on secret values (mllwchrry)
Pull request description:
Several zkp module functions leave secret scalars and buffers live on the stack after use.
Adds the missing clears. Also removes a redundant `prep` clear in `secp256k1_rangeproof_sign_impl` that was already cleared earlier on the same path.
ACKs for top commit:
apoelstra:
ACK a69a662d052f2a3839798fdb7d98864d26ccf2cb; successfully ran local tests
Tree-SHA512: b11acfaad3bbc26b7597918afdb90aaa563eb2974bc2522626f70457fa354bad349bc698b72371de9df9004bc1e16fee3afb437135cafb1e0c85dcc2535e2987
cde28971a2 rangeproof: warn that nonce must not be reused across differing arguments (mllwchrry)
Pull request description:
ACKs for top commit:
apoelstra:
ACK cde28971a2274beeeb2e9df27342d956b5664d44; successfully ran local tests
jonasnick:
ACK cde28971a2
Tree-SHA512: 890100ed778e06d1e4ed6fed03080a74676ff804c5f6844e3d2530b7f26a4ec715b8c1f81e3d1fdbbd25c28f50d966802115e493993bfc64ba336296593e7a71
The s-values produced by secp256k1_surjection_genrand previously
depended only on their indices and the difference between
input_blinding_key and output_blinding_key. Calls with the same
difference therefore reused s-values even when their proof inputs
differed.
For proofs with the same used-input selection and honest input index,
the same generated s-value was used as the signing nonce. Reusing this
nonce across different proof messages allowed recovery of the
blinding-key difference. The remaining repeated s-values also revealed
the honest input index.
This commit affects proof generation only; verification is unchanged.
217fe59bbf tests: register run_util_tests and run_ec_commit, remove dead print_vector (mllwchrry)
21645c03a2 SECURITY.md: remove Jonas Nick from trusted keys (Jonas Nick)
6a599a4428 scalar: correct `_scalar_get_bits_{limb32,var}` input condition docs (Sebastian Falbesoner)
994b35010d field: correct fe_equal's b magnitude bound (Lőrinc)
0cad3df503 Improve checks for scalar _get_bits methods (Peter.Dettman)
9d75769dec tests: Fix GCC 17 snapshot warning (Tim Ruffing)
66260b78a2 ci: add 'brew trust' invocation to macOS CI (fanquake)
71fcd8410e field: force-inline 5x52 mul and sqr (Lőrinc)
a77dacad9a test: enable -Wunused-function in test suite (Fix#1831) (kallal79)
1eab757207 cmake: Fix shared library versioning on OpenBSD (Hennadii Stepanov)
a401c5145a cmake: Fix shared library versioning on NetBSD (Hennadii Stepanov)
8a0f4002c7 cmake, refactor: Improve documenting in `SetLibtoolAbiVersion` module (Hennadii Stepanov)
acf2084aa7 cmake, refactor: Introduce `SetLibtoolAbiVersion` module (Hennadii Stepanov)
d7125e517d test: musig: fix dead "aggnonce encodes two points at infinity" check (Sebastian Falbesoner)
2ee79e77e6 test: add unit test for `_ecmult_gen_ge` (Sebastian Falbesoner)
ca68daf8e1 test: refactor: simplify tests by using `_ecmult_gen_ge` helper (Sebastian Falbesoner)
240578eef5 bench: add internal benchmark for `secp256k1_fe_normalize_var` (Sebastian Falbesoner)
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)
5698e66c64 Add exhaustive test for ECDH module (Sebastian Falbesoner)
40a0d874a6 doc: correct API docs for ECDSA signing out-params (s/array/signature object/) (Sebastian Falbesoner)
Pull request description:
*Note: This PR has been created by a GitHub Actions workflow without human involvement.*
This PR syncs the following upstream PRs:
* bitcoin-core/secp256k1#1851: doc: correct API docs for ECDSA signing out-params (s/array/signature object/)
* bitcoin-core/secp256k1#1852: Add exhaustive test for ECDH module
* bitcoin-core/secp256k1#1861: refactor: introduce `_ecmult_gen_ge` helper (preventing accidental gej leaks)
* bitcoin-core/secp256k1#1855: bench: add internal benchmark for `secp256k1_fe_normalize_var`
* bitcoin-core/secp256k1#1864: test: refactor: simplify tests by using `_ecmult_gen_ge` helper, add test
* bitcoin-core/secp256k1#1867: test: musig: fix dead "aggnonce encodes two points at infinity" check
* bitcoin-core/secp256k1#1860: cmake: Emulate Libtool's behavior on NetBSD and OpenBSD
* bitcoin-core/secp256k1#1865: test: enable -Wunused-function in test suite (Fix#1831)
* bitcoin-core/secp256k1#1859: field: force-inline 5x52 mul and sqr
* bitcoin-core/secp256k1#1879: ci: add 'brew trust' invocation to macOS CI
* bitcoin-core/secp256k1#1881: tests: Fix GCC 17 snapshot warning
* bitcoin-core/secp256k1#1845: Improve checks for scalar _get_bits methods
* bitcoin-core/secp256k1#1877: field: correct `fe_equal` magnitude bound for `b`
* bitcoin-core/secp256k1#1882: scalar: correct `_scalar_get_bits_{limb32,var}` input condition docs
* bitcoin-core/secp256k1#1884: SECURITY.md: remove Jonas Nick from trusted keys
Usage hints:
* If this PR has merge conflicts, resolve these by switching to the PR branch and merging the base branch into it using `git merge <base-branch>`.
* To show the conflict resolution diff from an existing merge commit, use `git show --remerge-diff <merge-commit>`.
* In case you are recreating the PR branch locally, you can (during the conflict resolution state) replay this conflict resolution diff using `git read-tree --reset -u <merge-commit>`.
Be aware that this may discard your index as well as the uncommitted changes and untracked files in your worktree.
ACKs for top commit:
DarkWindman:
ACK 217fe59bbf
real-or-random:
ACK 217fe59bbf
Tree-SHA512: 761fb08be08afcbaa3a43f7432c92a5e65113545a2a25131baf3385d7e354d2a913856f667c8478729877a074d98c0e58fc0c92b17b70a583cbd96d1f478d7f2
21645c03a2 SECURITY.md: remove Jonas Nick from trusted keys (Jonas Nick)
Pull request description:
As announced in `#bitcoin-core-dev` IRC, I’m stepping down as a libsecp256k1 maintainer because I’m no longer able to dedicate the time and attention that the project deserves. This PR removes me from the contact list in `SECURITY.md`.
ACKs for top commit:
real-or-random:
ACK 21645c03a2🫡
theStack:
ACK 21645c03a2🫡
furszy:
ACK 21645c03a2🫡
Tree-SHA512: c7d5fc32476e377766e9f4c07f78a4d8389b213f319070810456879996c6b3ac25949ae65a500f79b9108c3fb52b53a735842b438438349a3f5e2d3df1ba0e1e
6a599a4428 scalar: correct `_scalar_get_bits_{limb32,var}` input condition docs (Sebastian Falbesoner)
Pull request description:
This PR is a small correction of documented (off-by-one) input conditions for the `scalar_get_bits_{limb32,var}`, that came up during reviewing #1845.
ACKs for top commit:
real-or-random:
ACK 6a599a4428
Tree-SHA512: 2f0982b6d69d2abeebe0c9d82161e85feaae317d1e3ac2f415338044c8725a0442e14d998053e62f1b5f59f785a31796b5b3c4b36d9eb516ff53e9bb11429e78
994b35010d field: correct fe_equal's b magnitude bound (Lőrinc)
Pull request description:
**Problem:** While reviewing this area with Fable, I noticed that `secp256k1_fe_equal(a, b)` documented `b` up to magnitude 31.
Review pointed out that the implementation negates `a` before adding `b`, so the actual internal bound is 30.
**Fix:** Lower the documented and checked bound to 30.
This keeps `fe_equal` unchanged at runtime and matches current callers, which use much smaller magnitudes.
**Test:** Add a focused test with random field elements whose magnitudes are randomized within the accepted `a <= 1` and `b <= 30` bounds.
ACKs for top commit:
theStack:
ACK 994b35010d
real-or-random:
utACK 994b35010d
Tree-SHA512: 034b90b4020c65fcb335ab361341424e08355f64deefaf153d94b05304e68760c9f771e4bc3e0fd664aee164d7e9f5f533d001f21207ef968c52f67d2c23a74a
`secp256k1_fe_equal` negates `a` before adding `b`.
That gives the temporary value magnitude 2, and the following field addition requires the input magnitudes to sum to at most 32.
So the largest `b` magnitude the implementation can accept is 30, not 31.
Lower the documented and checked bound for `b` to 30.
Adjust the focused test to use random field elements with randomized magnitudes within the accepted `a <= 1` and `b <= 30` bounds.
Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
Co-authored-by: Tim Ruffing <me@real-or-random.org>
0cad3df503 Improve checks for scalar _get_bits methods (Peter.Dettman)
Pull request description:
Improves the `VERIFY_CHECK`s in all `_scalar_get_bits_limb32` and `_scalar_get_bits_var` methods.
The initial prompt was noticing that scalar_4x64_impl/`secp256k1_scalar_get_bits_limb32` was not restricting to 32-bit limbs correctly. Then missing range checks for `offset` were added and all such checks rewritten to avoid overflow.
With these changes, the _low and _4x64 impls of `_get_bits_var` can no longer forward to `_get_bits_limb32`, so those calls were inlined instead.
ACKs for top commit:
sipa:
ACK 0cad3df503
theStack:
ACK 0cad3df503
real-or-random:
utACK 0cad3df503
Tree-SHA512: 753991d586fe5695dd33af6c261c5458ab659be94204626166503af7d403748abb76d791846857a16383cbd38a1f84af9fde74b4327d68d706f88b6531ee7546
9d75769dec tests: Fix GCC 17 snapshot warning (Tim Ruffing)
Pull request description:
Passing a non-malloc pointer to free() would be UB. In this case, the
free() line is never actually reached (and GCC 17 fails to prove this)
in a correct implementation of secp256k1_scratch_space_destroy(), but
the test shouldn't rely on the correctness of the tested function.
Alternative to #1880. I think this is cleaner
- it doesn't recreate the scratch space in the middle of some other tests
- it has an obvious matching (malloc, free) pair
- it additionally checks that only `magic` is accessed
ACKs for top commit:
hebasto:
ACK 9d75769dec, I have reviewed the code and it looks OK.
theStack:
re-ACK 9d75769dec
Tree-SHA512: eb1a73c04e1996aeecabaa804e6a4fb1f5e6dfba4b9a775b6dd33d147be17185115a5aa9b7d9bf50260c20849e159d1b57f9ee41901d3d691dec25f2643b7cd2
Passing a non-malloc pointer to free() would be UB. In this case, the
free() line is never actually reached (and GCC 17 fails to prove this)
in a correct implementation of secp256k1_scratch_space_destroy(), but
the test shouldn't rely on the correctness of the tested function.
66260b78a2 ci: add 'brew trust' invocation to macOS CI (fanquake)
Pull request description:
This is probably needed to fix the issues in the macOS Valgrind jobs. i.e in #1877: https://github.com/bitcoin-core/secp256k1/actions/runs/28111568199/job/83240127830?pr=1877#step:4:199
```bash
Cache restored from key: x86_64-macos-native-valgrind-6ebc928a1e40da9db2f283a1c4ac74cfcff9f06687481fd0396a7b4c26ecfb09
Run brew link valgrind
Error: Refusing to load formula louisbrunner/valgrind/valgrind from untrusted tap louisbrunner/valgrind.
Run `brew trust --formula louisbrunner/valgrind/valgrind` or `brew trust louisbrunner/valgrind` to trust it.
Error: Process completed with exit code 1.
```
ACKs for top commit:
real-or-random:
utACK 66260b78a2
hebasto:
ACK 66260b78a2.
Tree-SHA512: 447f7687d1cb2ed983f77efea509c7a5255e2436ee0260767b7f9faa71c2c81fca5bc24477761e36cf27c8afdd72acccbb7475371dc233f11cd35d1d1d78d268
Should fix issues like:
> Error: Refusing to load formula louisbrunner/valgrind/valgrind from untrusted tap louisbrunner/valgrind.
> Run `brew trust --formula louisbrunner/valgrind/valgrind` or `brew trust louisbrunner/valgrind` to trust it.
> Error: Process completed with exit code 1.
a77dacad9a test: enable -Wunused-function in test suite (Fix#1831) (kallal79)
Pull request description:
This PR addresses issue #1831 by enabling the `-Wunused-function` compiler warning within the test suite.
Currently, `-Wno-unused-function` is passed globally to disable warnings about unused functions, making it too easy to write a test case but forget to actually call it. To catch untested helper functions safely, this PR uses GCC/Clang pragmas scoped strictly to the body of the test files.
### Changes Made:
- Added `#pragma GCC diagnostic warning "-Wunused-function"` directly after the `#include` statements in `src/tests.c`, `src/tests_exhaustive.c`, `src/ctime_tests.c`, and `src/unit_test.c`.
Fixes#1831
ACKs for top commit:
real-or-random:
ACK a77dacad9a
hebasto:
ACK a77dacad9a.
Tree-SHA512: 775d633d9d2e95154b6718270ce1687a1b20c2c8cc67c909953b3395d76e6853e6be1d6a95d8aef3f15a78dec3497bea8e3864e36830977e11beb42ea9abcc31
1eab757207 cmake: Fix shared library versioning on OpenBSD (Hennadii Stepanov)
a401c5145a cmake: Fix shared library versioning on NetBSD (Hennadii Stepanov)
8a0f4002c7 cmake, refactor: Improve documenting in `SetLibtoolAbiVersion` module (Hennadii Stepanov)
acf2084aa7 cmake, refactor: Introduce `SetLibtoolAbiVersion` module (Hennadii Stepanov)
Pull request description:
This is a continuation of https://github.com/bitcoin-core/secp256k1/pull/1685.
Additionally, the logic has been factored out into its own module and the documentation has been also improved.
ACKs for top commit:
real-or-random:
utACK 1eab757207
Tree-SHA512: 24738053d3049f0ce551b0d05d62642d7f1e6645967288fbe30ce4799f4e64594e88a8fa2dd9109efd6cfc5666d7c5fe7a3bb99f0f06766d70b2a9362721e3c9
d7125e517d test: musig: fix dead "aggnonce encodes two points at infinity" check (Sebastian Falbesoner)
Pull request description:
Due to the missing `CHECK` around, the return values were discarded and nothing was actually checked here.
(Fwiw I prompted two AI models (MiniMax M3 and Opus 4.8) to find more similar instances in tests with bare statements that miss a surrounding `CHECK` in tests, and both didn't find any.)
ACKs for top commit:
real-or-random:
utACK d7125e517d
hebasto:
ACK d7125e517d, I have reviewed the code and it looks OK.
Tree-SHA512: 6eab61ce51a414e0555413bde29cf582b70fbf4a24ad1aae135bf88f28e3ee25ece8c79b7ccc254288395fedf6b2547931d5e00b0090146f1b83e43acc6570d7
The 5x52 field multiplication and squaring routines are hot in group arithmetic and scalar multiplication.
Use the new `SECP256K1_FORCE_INLINE` for the thin wrappers and `int128` inner helpers so compilers can schedule the 64x64->128 arithmetic without a call boundary.
Across the measured GCC and MSVC Release builds, this improves ECDSA verification by 0.6% to 9.1%, ECDH by 0.7% to 9.3%, and Schnorr verification by 0.6% to 9.6%.
The direct field benchmarks generally show the intended effect on field squaring and multiplication, while Clang results are mostly flat and less consistently positive.
This is a code-size tradeoff: the tested static library builds grew by about 4.6% to 4.7%, and the tested Windows Release DLL grew by 14.1%.
Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Co-authored-by: Tim Ruffing <crypto@timruffing.de>
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
Kgothatso Ngako
Andrew Poelstra
mllwchrry
DarkWindman
Jonas Nick
merge-script
Sebastian Falbesoner
Lőrinc
fanquake
kallal79
Hennadii Stepanov