`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
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.
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.
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
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!).
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
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
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.
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.
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
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("`
