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
c09519f0e3 ci: Drop workaround for Valgrind older than 3.20.0 (Hennadii Stepanov)
Pull request description:
This is no longer needed in the current CI framework.
If someone runs the CI scripts locally, it is reasonable to expect that they are using up-to-date tools, including Valgrind.
ACKs for top commit:
real-or-random:
ACK c09519f0e3
Tree-SHA512: ef840dc8fd6e29110e194e35139050d9720bf8b38e89497790dcf5f2f2ce603805b572fb7408b4ae47bda31289a05548671ac41923f6ab3d1fbf288855f058d0
8bc50b72ff ci: Switch to macOS 15 Sequoia Intel-based image (Hennadii Stepanov)
Pull request description:
This is an alternative to https://github.com/bitcoin-core/secp256k1/pull/1755.
More details from the GHA developers:
> Apple has discontinued support for the x86_64 (Intel) architecture going forward. GitHub will no longer support this architecture on macOS after the macOS 15 runner image is retired in Fall 2027.
ACKs for top commit:
real-or-random:
ACK 8bc50b72ff
Tree-SHA512: 2c3de907b8910193b77bb201e1c7aad3f70f89804efe4b3ead23181d45a912f677cd533e819914cf1b346aa01d2f273fd5f1e63d7b8d284320d1e4e6198d8578
2f4546ce56 test: add --log option to display tests execution (furszy)
95b9953ea4 test: Add option to display all available tests (furszy)
953f7b0088 test: support running specific tests/modules targets (furszy)
0302c1a3d7 test: add --help for command-line options (furszy)
9ec3bfe22d test: adapt modules to the new test infrastructure (furszy)
48789dafc2 test: introduce (mini) unit test framework (furszy)
9cce703863 refactor: move 'gettime_i64()' to tests_common.h (furszy)
Pull request description:
Early Note:
Don’t be scared by the PR’s line changes count — most of it’s just doc or part of the test framework API.
Context:
Currently, all tests run single-threaded sequentially and the library lacks the ability to specify which test (or group of tests) you would like to run. This is not only inconvenient as more tests are added but also time consuming during development and affects downstream projects that may want to parallelize the workload (such as Bitcoin-Core CI).
PR Goal:
Introduce a lightweight, extensible C89 unit test framework with no dynamic memory allocations, providing a structured way to register, execute, and report tests. The framework supports named command-line arguments in `-key=value` form, parallel test execution across multiple worker processes, granular test selection (selecting tests either by name or by module name), and time accumulation reports.
The introduced framework supports:
* `-help` or `-h`: display list of available commands along with their descriptions.
* `-jobs=<num>`: distribute tests across multiple worker processes (default: sequential if 0).
* `-target=<name>` or `-t=<name>`: run only specific tests by name; can be repeated to select multiple tests.
* `-target=<module name>`, `-t=<module>` Run all tests within a specific module (can be provided multiple times)
* `-seed=<hex>`: set a specific RNG seed (defaults to random if unspecified).
* `-iterations=<n>`: specify the number of iterations.
* `-list_tests`: display list of available tests and modules you can run.
* `-log=<0|1>`: enable or disable test execution logging (default: 0 = disabled).
Beyond these features, the idea is to also make future developments smoother, as adding new tests require only a single entry in the central test registry, and new command-line options can be introduced easily by extending the framework’s `parse_arg()` function.
Compatibility Note:
The framework continues accepting the two positional arguments previously supported (iterations and seed), ensuring existing workflows remain intact.
Testing Notes:
Have fun. You can quickly try it through `./tests -j=<workers_num>` for parallel execution or `./tests -t=<test_name>` to run a specific test (call `./tests -print_tests` to display all available tests and modules).
Extra Note:
I haven't checked the exhaustive tests file so far, but I will soon. For now, this only runs all tests declared in the `tests` binary.
Testing Results: (Current master branch vs PR in seconds)
* Raspberry Pi 5: master \~100 s → PR \~38 s (5 jobs)
* MacBook Pro M1: master \~30 s → PR \~10 s (6 jobs)
ACKs for top commit:
theStack:
re-ACK 2f4546ce56
real-or-random:
ACK 2f4546ce56
hebasto:
ACK 2f4546ce56.
Tree-SHA512: 85ca2cbb620b84b35b353d5d4cf093c388fc3851ca405eeb0e458f8fa72b60534bccd357c7edabf8fc9aa93d9ad0a6fbac3dd5c4d5f9dfdf4d8701a9834755b9
15d014804e ci: Drop default for `inputs.command` in `run-in-docker-action` (Hennadii Stepanov)
1decc49a1f ci: Use YAML anchor and aliases for repeated "CI script" steps (Hennadii Stepanov)
dff1bc107d ci, refactor: Generalize use of `matrix.configuration.env_vars` (Hennadii Stepanov)
4b644da199 ci: Use YAML anchor and aliases for repeated "Print logs" steps (Hennadii Stepanov)
a889cd93df ci: Bump `actions/checkout` version (Hennadii Stepanov)
574c2f3080 ci: Use YAML anchor and aliases for repeated "Checkout" steps (Hennadii Stepanov)
Pull request description:
GHA YAML parser now [supports](https://github.com/actions/runner/issues/1182#issuecomment-3156285802) anchors.
This PR makes use of that support to [DRY](https://en.wikipedia.org/wiki/Don%27t_repeat_yourself) the workflow code.
ACKs for top commit:
real-or-random:
utACK 15d014804e
Tree-SHA512: a25a226fec0053242bc46b8c9815067a35af632cfbffefcc5cd4c96a67c0535dde447753099cbc74ecc64072d36aef2aa78c105b66f43cb3134ffa1ae60dca1e
53585f93b7 ci: Use clang-snapshot in "MSan" job (Hennadii Stepanov)
6894c964f3 Fix Clang 21+ `-Wuninitialized-const-pointer` warning when using MSan (Hennadii Stepanov)
Pull request description:
In Bitcoin Core, the "MSan" CI jobs use the latest tagged Clang available from http://apt.llvm.org.
This PR applies similar changes and switches the "MSan" CI jobs to clang-snapshot.
This exposes problematic code that was reported in https://github.com/bitcoin/bitcoin/issues/33284.
ACKs for top commit:
real-or-random:
utACK 53585f93b7
Tree-SHA512: 79bc10f1d0a60ed67b518eb8fab9a48146a4ef1fff95c8775717be3a950b323ae89a12999504ed8446f164da426894abe02f9fb61b5ca19453d549a34873b73b
f163c35897 ci: Set `DEBIAN_FRONTEND=noninteractive` (Hennadii Stepanov)
70ae177ca0 ci: Bump `docker/build-push-action` version (Hennadii Stepanov)
b2a95a420f ci: Drop `tags` input for `docker/build-push-action` (Hennadii Stepanov)
122014edb3 ci: Add `scope` parameter to `cache-{to,from}` options (Hennadii Stepanov)
Pull request description:
This PR fixes an issue where only the latest image cache was available.
For other minor improvements, see the individual commit messages.
ACKs for top commit:
real-or-random:
utACK f163c35897
Tree-SHA512: 7178c447d32e5c06e42d33ed32c9088fc19ca6a67369f2a8f6672b0ec010a516d4bb3a70a1847eec76e034ec22d6df778f6d421a04ba603ae18526a6f4104e65
Add support for specifying single tests or modules to run via the
"--target" or "-t" command-line option. Multiple targets can be
provided; only the specified tests or all tests in the specified
module/s will run instead of the full suite.
Examples:
-t=<test name> runs an specific test.
-t=<module name> runs all tests within the specified module.
Both options can be provided multiple times.
Lightweight unit testing framework, providing a structured way to define,
execute, and report tests. It includes a central test registry, a flexible
command-line argument parser of the form "--key=value" / "-k=value" /
"-key=value" (facilitating future framework extensions), ability to run
tests in parallel and accumulated test time logging reports.
So far the supported command-line args are:
- "--jobs=<num>" or "-j=<num>" to specify the number of parallel workers.
- "--seed=<hex>" to specify the RNG seed (random if not set).
- "--iterations=<num>" or "-i=<num>" to specify the number of iterations.
Compatibility Note:
To stay compatible with previous versions, the framework also supports
the two original positional arguments: the iterations count and the
RNG seed (in that order).
4d90585fea docs: Improve API docs of _context_set_illegal_callback (Tim Ruffing)
895f53d1cf docs: Clarify that callback can be called more than once (Tim Ruffing)
Pull request description:
The tests in #1698 reminded me that some functions, e.g., `secp256k1_ec_pubkey_cmp`, may call the illegal callback more than once (see https://github.com/bitcoin-core/secp256k1/pull/1390#discussion_r1279194655 for more context). This PR clarifies the API docs to state explicitly that this is possible.
This is the simplest solution. Any production code should crash anyway if it encounters a callback. And in debug code or in our test code, it doesn't really matter whether you see an error message once or twice.
The alternative is to provide a guarantee that the callback is called only once. But that would make our code more complex for no good reason.
The second commit fixes a few typos, wording, and consistency.
ACKs for top commit:
stratospher:
ACK 4d90585.
theStack:
re-ACK 4d90585fea
Tree-SHA512: 97c31d68851e845b21e9ec2530432603917c019580feba98b62014b538f61be94ba963bf619217720d8f7331ac830e97e62c76c02e7297d3cf73dd085e6f4ca2
dfe284ed2d bench: improve context creation in ECDH benchmark (Sebastian Falbesoner)
Pull request description:
Calling `secp256k1_context_create` with `SECP256K1_FLAGS_TYPE_CONTEXT` seems to be confusing and not strictly API-compliant, as the only allowed (non-deprecated) value is `SECP256K1_CONTEXT_NONE`, even if the former happens to map to the latter currently.
Fix this by not dynamically creating a context in the first place and switch to using the static context, as it is sufficient for this benchmark and presumably matches what the "no capabilities" comment intended back then.
Alternatives are:
* keep the signing context and only fix the name, i.e. s/_FLAGS_TYPE_CONTEXT/_CONTEXT_NONE/
* use `secp256k1_context_static` everywhere directly and get rid of the `ctx` field in the `bench_ecdh_data` struct (less flexible for future changes, deviates from other bench structures)
Found while reviewing #1698, see https://github.com/bitcoin-core/secp256k1/pull/1698#discussion_r2350395913.
ACKs for top commit:
sipa:
utACK dfe284ed2d
stratospher:
ACK dfe284e. not sure whether the alternative is preferred though.
real-or-random:
utACK dfe284ed2d
furszy:
ACK dfe284ed2d
Tree-SHA512: 106d115ec11577fcd66e93293289545aa0fa78480f4bdc8c440963e4e6b050c81d4775268cfa0e1ab44db4c08a5768a0ff80f74b866e550ddd22a4e17ccb9014
ab560078aa build: Fix warnings in x86_64 assembly check (Hennadii Stepanov)
Pull request description:
On the master branch @ 10dab907e7, the x86_64 assembly check in both the Autotools and CMake build systems can fail depending on externally provided flags. For example:
```
$ env CFLAGS="-Wall -Werror" ./configure --with-asm=x86_64
<snip>
checking for x86_64 assembly availability... no
configure: error: x86_64 assembly requested but not available
```
or
```
$ env CFLAGS="-Wall -Werror" cmake -B build -DSECP256K1_ASM=x86_64
<snip>
-- Performing Test HAVE_X86_64_ASM
-- Performing Test HAVE_X86_64_ASM - Failed
CMake Error at CMakeLists.txt:111 (message):
x86_64 assembly requested but not available.
-- Configuring incomplete, errors occurred!
```
The same issue occurs in CI jobs that build on Windows using clang-cl.
This PR fixes both build systems.
ACKs for top commit:
real-or-random:
utACK ab560078aa
furszy:
ACK ab560078aa
Tree-SHA512: d556b642d58c601e7f027ac54975249e05a8b3927c5efd229be43d264b024d00eab9973193adb52f2f60075fca0571644662d61150a19098820091ced2d56fa0
7321bdf27b doc: clarify API doc of `secp256k1_ecdsa_recover` return value (Jonas Nick)
Pull request description:
ACKs for top commit:
jonasnick:
ACK 7321bdf27b
Tree-SHA512: a7bac228cf504f6c8a53fdc15c7f3241ac27c214e10b9aabdf962e8d7c4c5107d0c0898467bd3f491cecf0ca2851c604da1ec5520a353b27c8aa02ce3daf2334
Calling `secp256k1_context_create` with `SECP256K1_FLAGS_TYPE_CONTEXT`
seems to be not strictly API-compliant, as the only allowed
(non-deprecated) value is `SECP256K1_CONTEXT_NONE`, even if the
former happens to map to the latter currently.
Fix this by not dynamically creating a context in the first place and
switch to using the static context, as it is sufficient for this
benchmark and presumably matches what the "no capabilities" comment
intended back then.
Relocate the clock time getter to tests_common.h to
make it easily reusable across test programs. This
will be useful for the upcoming unit test framework.
Context - why not placing it inside testutil.h?:
The bench program links against the production-compiled library,
not its own compiled version. Therefore, `gettime_i64()` cannot
be moved to testutil.h, because testutil.h calls
`secp256k1_pubkey_save()`, which exists only in the internal
secp256k1.c and not in the public API.
399b582a5f Split memclear into two versions (John Moffett)
Pull request description:
Replace `memset` which can still be optimized out as `secnonce` isn't read later in this function. The API makes it clear the callee is responsible for it, so we need to assure it's cleared properly.
ACKs for top commit:
real-or-random:
utACK 399b582a5f thanks!
jonasnick:
ACK 399b582a5f
Tree-SHA512: 98cd6c44a45218ad66ca99ceacb9bbccee7ff105957363bcde9c3e17d55ac4a81afc881af8adfce7ff3d6ef5d75f3b111d7220256914ca0bedeb43769b38dd20
7ebaa134a7 check-abi: remove support for obsolete CMake library output location (src/libsecp256k1.so) (Sebastian Falbesoner)
Pull request description:
The CMake library output location was changed from "src/" to "lib/" in PR #1553, supporting the old location (presumably done to avoid having users to reconfigure existing CMake builds for a temporary transition window) shouldn't be necessary anymore.
ACKs for top commit:
real-or-random:
utACK 7ebaa134a7
Tree-SHA512: 7a4e86260d9593ba4616590c927ae8753b007d51426a86eeb6f8fa35338cfa6c9b05d962f32d0d84a3271340edff2cd50f9180ce1357ab49b7850b58fa017192
secp256k1_memclear has the side effect of undefining bytes for
valgrind checks. In some cases, we may want to zero bytes
but allow subsequent reads. So we split memclear into
memclear_explicit, which makes no guarantees about the content
of the buffer on return, and memzero_explicit, which guarantees
zero value on return.
Change the memset in partial_sign to use memzero_explicit.
806de38bfc doc: mention ctx requirement for `_ellswift_create` (not secp256k1_context_static) (Sebastian Falbesoner)
Pull request description:
Public functions that require a context for generator point multiplication (i.e. `ctx->ecmult_gen_ctx` is built) usually denote this in the API header by mentioning to not use `secp256k1_context_static`, so add this for `_ellswift_create` as well. This seems the only instance where this is missing currently, see
```
$ git grep ecmult_gen_context_is_built
$ git grep ctx:.*context_static
```
(note that in the musig and schnorr modules, two public functions for nonce generation / signing map to a single internal function where the context requirement is checked).
Noted while reviewing #1698.
ACKs for top commit:
sipa:
ACK 806de38bfc
jonasnick:
ACK 806de38bfc
real-or-random:
ACK 806de38bfc
josibake:
ACK 806de38bfc
Tree-SHA512: 902e9e21060e09e8e7d72fec8cdc42e0ed18f95824d3220100d7b65720511f934d38e3e556e38bb510d98284bccc12b857f329997640d1c07edd5b55ef6d8e09
325d65a8cf Rename and clear var containing k or -k (John Moffett)
Pull request description:
Follow-up to #1579. `buf` still holds the nonce or its negation, so ought to be cleared.
ACKs for top commit:
theStack:
Code review re-ACK 325d65a8cf
real-or-random:
utACK 325d65a8cf
Tree-SHA512: a2fe39d7c44cebc0abe712828d521c2a7aba1db2d9dc5fc811dcaf96f1e45494dba5d7f016b6d9200ab523641ff62083686dbc942284e0f548183aaf60d8bfa2
960ba5f9c6 Use size_t instead of int for RFC6979 outlen copy (John Moffett)
Pull request description:
If `outlen > INT_MAX` it results in segfault or hang (when `outlen` is a multiple of 2^32) on most implementations due to conversion in: `int now = outlen` producing negative values or zero. Unreachable in current code and highly improbable in future practice, but fits contract better and fixes a couple of compiler warnings.
ACKs for top commit:
real-or-random:
utACK 960ba5f9c6
theStack:
Code-review ACK 960ba5f9c6
Tree-SHA512: b91ee2fd3e962000f1b98a42e6f3c70cb3738c639fef8c2ce0cf53f49fe55da3e5d332eabbd8cbe9cdccb4e9c0ae70d3390a41f9468fd23ded3318596548c68f
