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
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.
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
buf currently holds k or -k and isn't cleared, so clear it and rename to
nonce32 to clarify its sensitivity and match how it is named in the
corresponding ECDSA sign_inner.
5153cf1c91 tests: refactor tagged hash tests (josibake)
Pull request description:
Opened in response to https://github.com/bitcoin-core/secp256k1/pull/1698#discussion_r2269449070
---
We use tagged hashes in `modules/musig`, `modules/schnorrsig`, `modules/ellswift`, and the proposed `modules/silentpayments`. In looking for inspiration on how to add tagged hash midstate verification for https://github.com/bitcoin-core/secp256k1/pull/1698, it seemed like a good opportunity to DRY up the code across all of the modules.
I chose the convention used in the ellswift module as this seems the most idiomatic C. Since the tags are normally specified as strings in the BIPs, I also added a comment above each char array for convenience.
If its deemed too invasive to refactor the existing modules in this PR, I'm happy to drop the refactor commits for the ellswift and schnorrsig modules. All I need for https://github.com/bitcoin-core/secp256k1/pull/1698 is the first commit which moves the utility function out of the musig module to make it available to use in the silent payments module.
ACKs for top commit:
real-or-random:
utACK 5153cf1c91 assuming CI passes
theStack:
Code-review ACK 5153cf1c91
Tree-SHA512: 335ec3ee6a265e13cc379968f8fa1624534bef2389e4e21b85e6a9572ce1bd9dee4eabd2cb6d187ac974db3ab8246c2626d309ccfbee5744c30cf7560d1e261c
Move the sha256_tag_test_internal function out of the musig module
into tests.c. This makes it available to other modules wishing to verify tagged
hashes without needing to duplicate the function.
Change the function signature to expect a const unsigned char and update
the tagged hash tests to use static const unsigned char character
arrays (where necessary).
Add a comment for each tag. This is done as a convenience for checking
the strings against the protocol specifications, where the tags are
normally specified as strings.
Update tests in the ellswift and schnorrsig modules to use the
sha256_tag_test_internal helper function.
Otherwise, commands fail with the error:
```
Stderr of gcov was >><< End of stderr
Exception was >>Got function secp256k1_scalar_split_lambda on multiple lines: 67, 142.
You can run gcovr with --merge-mode-functions=MERGE_MODE.
The available values for MERGE_MODE are described in the documentation.<< End of stderr
```
24ba8ff168 chore(ci): Fix typo in Dockerfile comment (Maximilian Hubert)
Pull request description:
Corrects the spelling of "dpkg-dev" in a comment within the Dockerfile.
The comment explains the reason for installing the `dpkg-dev` package..
ACKs for top commit:
real-or-random:
ACK 24ba8ff168
Tree-SHA512: 79742bcd8018f6435d1113a08e44bacca6027faa5171e564b6c6f9cc8a5630aac32f8f611d276ef50b7f03d842bc9228d368c99bdc816decf947d39956caed2c
c25c3c8a88 test: update wycheproof test vectors (josibake)
Pull request description:
Pull in updated test vectors. This update is done as a follow-up to #1711.
ACKs for top commit:
real-or-random:
ACK c25c3c8a88
Tree-SHA512: 6f3b41460155a9f78d23711e471e73f732d3f6cdc2442f2f024540c04c3d4ab2ca376d4db020e99b093cd6a95c11f7dd3220f18727d62c4995a9a069d362406d
7b07b22957 cmake: Avoid contaminating parent project's cache with BUILD_SHARED_LIBS (Hennadii Stepanov)
Pull request description:
The CMake cache is global in scope. Therefore, setting the standard cache variable `BUILD_SHARED_LIBS` can inadvertently affect the behavior of a parent project.
Consider configuring Bitcoin Core without explicit setting `BUILD_SHARED_LIBS`:
```
$ cmake -B build -DBUILD_KERNEL_LIB=ON
```
According to CMake’s documentation, this should configure `libbitcoinkernel` as `STATIC`.
However, that's not the case:
```
$ cmake --build build -t libbitcoinkernel
[143/143] Linking CXX shared library lib/libbitcoinkernel.so
```
This PR:
1. Sets the `BUILD_SHARED_LIBS` cache variable only when `libsecp256k1` is the top-level project.
2. Removes the `SECP256K1_DISABLE_SHARED` cache variable. This enables parent projects that include libsecp256k1 as a subproject to rely solely on standard CMake variables for configuring the library type. During integration into a parent project, the static library can be forced as demonstrated [here](https://github.com/bitcoin-core/minisketch/pull/75#issuecomment-2984025132).
ACKs for top commit:
purpleKarrot:
ACK 7b07b22957
theuni:
utACK 7b07b22957
Tree-SHA512: 399a02e86093656ce70d9a0292a1f30834bcc5b9cf0f77d6465adc5e8a4d8e779684adedc40942eb931fed857399e4176a23fdbdf45f277184b28159fbc932d2
The CMake cache is global in scope. Therefore, setting the standard
cache variable `BUILD_SHARED_LIBS` can inadvertently affect the behavior
of a parent project.
This change:
1. Sets the `BUILD_SHARED_LIBS` cache variable only when libsecp256k1 is
the top-level project.
2. Removes the `SECP256K1_DISABLE_SHARED` cache variable. This enables
parent projects that include libsecp256k1 as a subproject to rely
solely on standard CMake variables for configuring the library type.
