2ccff6eb73 ci: Add weekly schedule (Hennadii Stepanov)
2f18567d24 ci: Rotate Docker cache keys every 4 weeks (Hennadii Stepanov)
0ffb1749a5 ci, docker: Fix LLVM repository signature failure (Hennadii Stepanov)
Pull request description:
This is an alternative to https://github.com/bitcoin-core/secp256k1/pull/1807 that avoids introducing a new workflow with the write permissions.
Closes https://github.com/bitcoin-core/secp256k1/issues/1691.
The 4-week rotation interval was chosen based on the following [rationale](https://github.com/bitcoin-core/secp256k1/pull/1816#issuecomment-3833536293):
> My thinking is that we may want to take only every fourth one. I assume this is still good enough to catch changes introduced by new compiler optimizations, and this is what we care about.
>
> We could just take the ISO week number mod 4. That results in an off-by-one error after every (rare) year with 53 ISO weeks, but ok, who cares... And if the cache is evicted for whatever other reason, we'll also get the most recent snapshot, but also that seems acceptable.
---
**IMPORTANT NOTE:** Due to a mere coincidence, LLVM apt signatures became [rejected](https://github.com/llvm/llvm-project/issues/153385) by Debian Trixie today. A commit containing a temporary workaround has been included to address this.
ACKs for top commit:
real-or-random:
ACK 2ccff6eb73
Tree-SHA512: c0362b107169d7cd7d36e0f7286d0bd183b734963beaa3915f198bedfd83f14222b779cb87eb6de2b1b940592954947d348a17a416e5db737a757397bd916447
0267b65512 release process: mention the `[Unreleased]` link clearly (Jonas Nick)
Pull request description:
Adding this link was forgotten in the first version of the 0.7.1 release PR but caught in PR review.
ACKs for top commit:
hebasto:
ACK 0267b65512.
sipa:
ACK 0267b65512
real-or-random:
utACK 0267b65512
Tree-SHA512: a7eb30bbd3a0760402a61170a986c4de3f62c99f15780b336c474ddcb044d7916122fd1521c57cf31bb6c9fd466484542027bcbf699b3880564b8822d5af5520
The LLVM apt repository uses legacy SHA1 signatures which are now
rejected by the stricter Sequoia PGP policy.
This change extends the 'sha1.second_preimage_resistance' cutoff date to
9999-01-01 in the default Sequoia config. This effectively whitelists
the legacy signature algorithm, preventing "OpenPGP signature
verification failed" errors during `apt-get update`.
See https://github.com/llvm/llvm-project/issues/153385.
748c0fdd67 Add CMake build directory patterns to `.gitignore` (Hennadii Stepanov)
7eb86bdb01 autotools: Rename `build-aux` to `autotools-aux` (Hennadii Stepanov)
Pull request description:
Whenever I work on changes that require comparison, such as benchmarking, I end up with two or more build directories that provide different binary variants simultaneously. Adding these build directories to `.gitignore` makes the workflow a bit easier.
Additionally, a trivial refactoring is included to reduce the code.
ACKs for top commit:
real-or-random:
utACK 748c0fdd67
furszy:
ACK 748c0fdd67
Tree-SHA512: 948917dcdc2ec6d5a2227f35ef9208fdbc62c56047db1c60b39f6da632642847aefa18f136986f9f15f08e0b2385964afe9a311346b728536323c54b4f0e3f04
47eb70959a ecmult: Use size_t for array indices in _odd_multiplies_table (Tim Ruffing)
bb1d199de5 ecmult: Use size_t for array indices into tables (Tim Ruffing)
Pull request description:
I don't think the current code is incorrect, but using `size_t` improves readability because the type makes it clear that we're dealing with array indices.
Also, making the result of the `ECMULT_TABLE_SIZE` macro (hopefully) a `size_t` fixes a compiler warning on MSVC, see #1791.
ACKs for top commit:
hebasto:
re-ACK 47eb70959a.
jonasnick:
ACK 47eb70959a
theStack:
ACK 47eb70959a
Tree-SHA512: e484fd610d50e972021c0184a683993364290eb58e09b65f9521b4507ec8d0639b402c67002005630b389bc863a7aa05b75f7224524dbcbafbfa5f9a4812b4a5
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
This change improves separation from CMake build directories, which
typically use the "build" prefix.
Additionally, corresponding `.gitignore` entries have been refactored.
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
