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
44b205e9ee Revert "cmake: configure libsecp256k1.pc during install" (Daniel Pfeifer)
Pull request description:
This reverts commit 7106dce6fd.
This causes a regression for packaging, as the generated `.pc` file will contain the location that was used to build the package archive.
ACKs for top commit:
real-or-random:
ACK 44b205e9ee
Tree-SHA512: f4331c23534260bc21e69ccff65abad766e6a51aaba350c40674e32b3200768ca3c6742671e425aa8fcb681deb430cd6b3e20f34b9e8f8ab909371cf5e8976c5
0dfe387dbe cmake: support the use of launchers in ctest -S scripts (Daniel Pfeifer)
Pull request description:
When `CTEST_USE_LAUNCHERS` is set to `ON` in a `ctest -S` script, the configure step fails with the error message:
```
CMake Error:
CTEST_USE_LAUNCHERS is enabled, but the RULE_LAUNCH_COMPILE global property
is not defined.
Did you forget to include(CTest) in the toplevel CMakeLists.txt ?
```
However, `include(CTest)` produces unwanted clutter. `include(CTestUseLaunchers)` is a more lightweight alternative.
To reproduce the issue, run the following script with and without the PR applied.
```cmake
#!/usr/bin/env -S ctest -VV -S
set(CTEST_SOURCE_DIRECTORY "/path/to/secp256k1")
set(CTEST_BINARY_DIRECTORY "/path/to/secp256k1-build")
set(CTEST_CMAKE_GENERATOR "Ninja")
set(CTEST_USE_LAUNCHERS ON)
ctest_empty_binary_directory(${CTEST_BINARY_DIRECTORY})
ctest_start("Experimental")
ctest_configure()
ctest_build()
```
ACKs for top commit:
hebasto:
ACK 0dfe387dbe.
Tree-SHA512: 643d0fabd19ddfd5a64a0b34cfcca8ea2cff64438ee3441ba9a1745618daf99c468ec201ea6992c542d4e33152f4833691c42c8a90301fe3dfc3f0f49f755e55
This reverts commit 7106dce6fd.
This causes a regression for packaging, as the generated
`.pc` file will contain the location that was used to build
the package archive.
7106dce6fd cmake: configure libsecp256k1.pc during install (Daniel Pfeifer)
Pull request description:
When installing to a given prefix, make sure that the .pc file contains that prefix rather than the value of `CMAKE_INSTALL_PREFIX` that the project was configured with.
ACKs for top commit:
real-or-random:
ACK 7106dce6fd I verified that it fixes the path in libsecp256k1.pc
Tree-SHA512: 34841513d2dc52234718eab56ecb9224aa1e13ad2d13cd103624b355e0627c37441363ad24293e07da7a748191e6ed2b67649b489bf874bab35346146b78c16f
When installing to a given prefix, make sure that the .pc file contains
that prefix rather than the value of CMAKE_INSTALL_PREFIX that the
project was configured with.
37dd422b5c cmake: Emulate Libtool's behavior on FreeBSD (Hennadii Stepanov)
Pull request description:
Building the master branch @ f24b838bed on FreeBSD 14.3:
- with Autotools:
```
$ ./autogen.sh
$ ./configure --disable-static --prefix /tmp/AUTOTOOLS
$ gmake
$ gmake install
$ tree /tmp/AUTOTOOLS/lib
/tmp/AUTOTOOLS/lib
├── libsecp256k1.la
├── libsecp256k1.so -> libsecp256k1.so.5.0.1
├── libsecp256k1.so.5 -> libsecp256k1.so.5.0.1
├── libsecp256k1.so.5.0.1
└── pkgconfig
└── libsecp256k1.pc
2 directories, 5 files
```
- with CMake:
```
$ cmake -B build -DCMAKE_INSTALL_PREFIX=/tmp/CMAKE
$ cmake --build build
$ cmake --install build
$ tree /tmp/CMAKE/lib
/tmp/CMAKE/lib
├── cmake
│ └── libsecp256k1
│ ├── libsecp256k1-config-version.cmake
│ ├── libsecp256k1-config.cmake
│ ├── libsecp256k1-targets-relwithdebinfo.cmake
│ └── libsecp256k1-targets.cmake
├── libsecp256k1.so -> libsecp256k1.so.5
├── libsecp256k1.so.5
└── pkgconfig
└── libsecp256k1.pc
4 directories, 7 files
```
With this PR:
```
$ cmake -B build -DCMAKE_INSTALL_PREFIX=/tmp/CMAKE+PR
$ cmake --build build
$ cmake --install build
$ tree /tmp/CMAKE+PR/lib
/tmp/CMAKE+PR/lib
├── cmake
│ └── libsecp256k1
│ ├── libsecp256k1-config-version.cmake
│ ├── libsecp256k1-config.cmake
│ ├── libsecp256k1-targets-relwithdebinfo.cmake
│ └── libsecp256k1-targets.cmake
├── libsecp256k1.so -> libsecp256k1.so.5
├── libsecp256k1.so.5 -> libsecp256k1.so.5.0.1
├── libsecp256k1.so.5.0.1
└── pkgconfig
└── libsecp256k1.pc
4 directories, 8 files
```
From [FreeBSD Developers' Handbook](https://docs.freebsd.org/en/books/developers-handbook/policies/#policies-shlib):
> If you are adding shared library support to a port or other piece of software that does not have one, the version numbers should follow these rules. Generally, the resulting numbers will have nothing to do with the release version of the software.
>
> For ports:
>
> - Prefer using the number already selected by upstream
>
> - If upstream provides symbol versioning, ensure that we use their script
ACKs for top commit:
real-or-random:
utACK 37dd422b5c
Tree-SHA512: b603d7e293ae1fb15c2b3c05957dcc3cbe94294083ad1d8cb00b06b0e295597fa09719d32c18d628670952b6d00467f5bc884be9ab791baf59ec265e26032470
145ae3e28d cmake: add a helper for linking into static libs (Cory Fields)
Pull request description:
As discussed here: https://github.com/bitcoin-core/secp256k1/pull/1674#issuecomment-2934819801
Parent projects (Bitcoin Core in this case) may wish to include secp256k1 in another static library (libbitcoinkernel) so that users are not forced to bring their own static libsecp256k1.
Unfortunately, CMake lacks the machinery to link (combine) one static lib into another.
To work around this, secp256k1_objs is exposed as an interface library which parent projects can "link" into static libs.
ACKs for top commit:
hebasto:
ACK 145ae3e28d, tested on Ubuntu 24.04 using cmake 3.22.6 and the default cmake 3.28.3.
stickies-v:
Light ACK 145ae3e28d
Tree-SHA512: bfe72e3f337eadce8bdbe613e4ce2f2cd92046f811c447311e5670af9d52dbf5b9dc91866f69251f52a7632ad66d6df102fb6f4c1de2688bb7611b7b42e969a3
819210974b README: add link to musig example, generalize module enabling hint (Sebastian Falbesoner)
Pull request description:
ACKs for top commit:
real-or-random:
ACK 819210974b
Tree-SHA512: 9bfc7e59f0b6c6bacc591abe9835d1e6129e4daf286b91b35fad83ddf7d870a534d83ce2c27165ed8612f05695a5f15a3ef7bebbcbe63a5fe843d4c0617ebc9f
