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