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.
7ab8b0cc01 release cleanup: bump version after 0.7.0 (Jonas Nick)
Pull request description:
Based on #1707.
ACKs for top commit:
real-or-random:
utACK 7ab8b0cc01
Tree-SHA512: 2eea54938ffa2a2c0503607566cb2900b76d5ff6d6285c65afe6fc09fa8b961f210ad915276618c5f43e3ade55252d6533c4c72a5e12c4db4c30fb20235a88df
cde4130898 musig/tests: initialize keypair (Jonas Nick)
Pull request description:
The keypair is unused in musig_partial_sign, but clang-snapshot gives a compiler warning anyway.
ACKs for top commit:
real-or-random:
ACK cde4130898
Tree-SHA512: 2a6c2b31c3ef8b5353c5dbdc156d052ff802d1f62d22214d952ee46223a9f87bc36b3ecdd49788381db379a01be83ded9def752747769b3751f3f058d57403e6
40b4a06520 changelog: update (Jonas Nick)
Pull request description:
This update includes all PRs that have a [`needs-changelog` tag](https://github.com/bitcoin-core/secp256k1/pulls?q=is%3Apr+is%3Aclosed+label%3Aneeds-changelog):
- Make static context const #1639
- include: remove WARN_UNUSED_RESULT for functions always returning 1 #1659
- cmake: Bump minimum required CMake version to 3.22 #1675#1675
- added: cmake: add a helper for linking into static libs #1678
- doc: Promote "Building with CMake" to standard procedure #1680
- also added cmake: Emulate Libtool's behavior on FreeBSD #1685
I had added the `needs-changelog` tag to #1685 because it seems noteworthy, but otherwise have not found any additional merged PR that would require a changelog entry.
I'm not sure if the changelog entry for #1678 makes sense (CC theuni)
ACKs for top commit:
real-or-random:
ACK 40b4a06520
Tree-SHA512: 678c409aea13ea9e9adaddd92f3d8da4fc98aeb9d661df9ff0f5093ad788c5ca0f194831c134675474cf9540679f3314086ee894f781afa3ea327d8001f53cd9
c82d84bb86 build: add CMake option for disabling symbol visibility attributes (Cory Fields)
ce7923874f build: Add SECP256K1_NO_API_VISIBILITY_ATTRIBUTES (Tim Ruffing)
e5297f6d79 build: Refactor visibility logic (Tim Ruffing)
Pull request description:
This is less invasive than #1695. The latter might be the right thing in a new library (and then we'd probably not support autotools in the first place), but any semantic change to this code has the potential to create news bug, or at least breakages for downstream users.
This is different from #1677 in that it does not set `hidden` explicitly. I agree with the comment in #1677 that setting `hidden` violates the principle of least surprise.
So this similar in spirit to #1674. So I wonder if this should also include
3eef7362c4. I'd say no, `fvisibility` should then set by the user. But can you, in CMake, set `CMAKE_C_VISIBILITY_PRESET` from a parent project?
ACKs for top commit:
hebasto:
ACK c82d84bb86, I have reviewed the code and it looks OK.
Tree-SHA512: dad36c32a108d813e8d4e1849260af43f79a9aa8fbfb9a42b07d737e0467924a511110df0a2c6761539a1587b617a1b11123610a3db9d4cdf2b985dfb3eb21da
bf082221ff cmake: Make `secp256k1_objs` inherit interface defines from `secp256k1` (Hennadii Stepanov)
Pull request description:
This PR effectively adds `-DSECP256K1_STATIC` to usage requirements of `secp256k1_objs` on Windows, preventing LNK4217 linker warnings.
I overlooked this issue while reviewing https://github.com/bitcoin-core/secp256k1/pull/1678.
ACKs for top commit:
real-or-random:
utACK bf082221ff
Tree-SHA512: d31e963378b795fd2be88f26574561f1e16b4c2fe10f348f6bfcf1bcc7e68da81ff93475548e9513514ded8eef5a0bd5e2eac2f4a9c5b879e0ab171e49dfcad5
3352f9d667 ci: enable musig module for native macOS arm64 job (Sebastian Falbesoner)
Pull request description:
Seems like this is the only CI job where the musig module wasn't enabled (roughly checked via `$ git grep "SCHNORRSIG: 'yes',"`, to find lists where modules are enabled explicitly).
ACKs for top commit:
real-or-random:
ACK 3352f9d667
Tree-SHA512: 70bf57e49fea2477e70331bd264556990dd2b4b57277174aace987a906b9d5a19d240038f2b8845239c2c12c51d7340f91ed779b1ccf8e8b232b8a746278e79c
