158f9e5eae cmake: Do not modify build types when integrating by downstream project (Hennadii Stepanov)
Pull request description:
The `CMAKE_BUILD_TYPE` and `CMAKE_CONFIGURATION_TYPES` must be managed by the downstream project.
Suggesting to review with `git diff -w`.
Fixes `std::out_of_range` exception from CMake in https://github.com/hebasto/bitcoin/pull/192 when running configuration step using "Ninja Multi-Config" generator:
```
$ cmake -B build -G "Ninja Multi-Config"
...
-- Configuring done (17.1s)
terminate called after throwing an instance of 'std::out_of_range'
what(): map::at
Aborted (core dumped)
```
Here are related discussions:
- https://discourse.cmake.org/t/uncaught-exception-when-trying-to-generate-a-project-using-ninja-multi-config/11051
- https://gitlab.kitware.com/cmake/cmake/-/issues/26064
ACKs for top commit:
real-or-random:
ACK 158f9e5eae
Tree-SHA512: b3040f40438d530f14b7e0f7d523e74b5843d88d250ff7955a99cc8c451feb9471a48134d1a89b3651b3f8195f91c17135c7b8a5d3ab092c8d35275b57743b8c
4706be2cd0 cmake: Reimplement `SECP256K1_APPEND_CFLAGS` using Bitcoin Core approach (Hennadii Stepanov)
c2764dbb99 cmake: Rename `SECP256K1_LATE_CFLAGS` to `SECP256K1_APPEND_CFLAGS` (Hennadii Stepanov)
Pull request description:
This PR address this https://github.com/hebasto/bitcoin/issues/239#issuecomment-2182713690:
> For consistency with libsecp256k1:
>
> > > Is this code block supposed to achieve the same as our `SECP256K1_LATE_CFLAGS` (implemented by a user-defined function `all_targets_add_compile_options`) in libsecp256k1?
> >
> >
> > It is. But this approach guaranties to override even options that are abstracted by CMake, for instance [#157 (comment)](https://github.com/hebasto/bitcoin/pull/157#issuecomment-2090465123).
>
> * If we agree that appending to rule variables is superior, should we also do this in libsecp256k1?
>
> * And/or should we rename the `SECP256K1_LATE_CFLAGS` variable to `APPEND_CFLAGS`?
ACKs for top commit:
real-or-random:
utACK 4706be2cd0
Tree-SHA512: 24603886c4d6ab4e31836a67d5759f9855a60c6c9d34cfc6fc4023bd309cd51c15d986ac0b77a434f9fdc6d5e97dcd3b8484d8f5ef5d8f840f47dc141de18084
f87a3589f4 cmake: Do not set `CTEST_TEST_TARGET_ALIAS` (Hennadii Stepanov)
Pull request description:
An alias for the "test" target can be confusing for the downstream project.
For instance, when integrating using `add_subdirectory(secp256k1 EXCLUDE_FROM_ALL)` (see https://github.com/hebasto/bitcoin/pull/192), test binaries are not being built by default. But the `check` alias target is exposed to the downstream project build system, which in turn fails:
```
$ make -C build check
...
Unable to find executable: /home/hebasto/git/bitcoin/build/src/secp256k1/src/exhaustive_tests
3/3 Test #3: exhaustive_tests .................***Not Run 0.00 sec
0% tests passed, 3 tests failed out of 3
Total Test time (real) = 0.03 sec
The following tests FAILED:
1 - noverify_tests (Not Run)
2 - tests (Not Run)
3 - exhaustive_tests (Not Run)
Errors while running CTest
...
```
This PR fixes this issue by deleting the `CTEST_TEST_TARGET_ALIAS` usage.
ACKs for top commit:
real-or-random:
utACK f87a3589f4
Tree-SHA512: ccf3f30939cf1747471ea15260f7caa6dad3f510e5771245ecbfbef3cc0b0e7c8ac551519d0892bf2544c91467d8d67d2c6e6bc52f56c384b174b88bcf377d4a
ec4c002faa cmake: Simplify `PROJECT_IS_TOP_LEVEL` emulation (Hennadii Stepanov)
cae9a7ad14 cmake: Do not set emulated PROJECT_IS_TOP_LEVEL as cache variable (Hennadii Stepanov)
Pull request description:
As CMake's cache is a global database, modifying it within a project integrated with the `add_subdirectory()` command, which may also include using the `FetchContent` module, could potentially affect downstream projects and sibling ones.
ACKs for top commit:
real-or-random:
utACK ec4c002faa
theuni:
utACK ec4c002faa
Tree-SHA512: de2c8c583367028d06701f79fc5232b351622c8496d196aad8c22a1ec4e450af53e556a4f6526ed47250f818143a69a12f5fc8cc755c864510e67530dacde66e
e73f6f8fd9 tests: refactor: drop `secp256k1_` prefix from testrand.h functions (Sebastian Falbesoner)
0ee7453a99 tests: refactor: add `testutil_` prefix to testutil.h functions (Sebastian Falbesoner)
0c6bc76dcd tests: refactor: move `random_` helpers from tests.c to testutil.h (Sebastian Falbesoner)
0fef8479be tests: refactor: rename `random_field_element_magnitude` -> `random_fe_magnitude` (Sebastian Falbesoner)
59db007f0f tests: refactor: rename `random_group_element_...` -> `random_ge_...` (Sebastian Falbesoner)
Pull request description:
This PR is an attempt at tidying up test util functions, as suggested in #1491. The following changes are done:
* rename `_group_element...` functions to `_ge...`
* rename `_field_element...` functions to `_fe...`
* move `random_` helpers from tests.c to testutil.h (the alternative would be testrand.h, but to my understanding, this one is meant to contain the actual RNG implementation rather than helpers using it; happy to move the helpers there if that is preferred though)
* prefix testutil.h functions with `testutil_`
* prefix testrand.h functions with `testrand_` (this is currently done in a sloppy way by simply dropping the `secp256k1_` prefix, so some functions don't have the full prefix, like e.g. `testrand256`; naming suggestions welcome)
ACKs for top commit:
sipa:
utACK e73f6f8fd9
real-or-random:
utACK e73f6f8fd9
Tree-SHA512: c87a35a9f7f23d4bbb87a1ff0d40dd5fbd7d976719ca1027cad187ac44aa2db3ae887ac620639d2287c260e701a5963830b52048692d3e6b38b5eb6cdf17b854
f55703ba49 autotools: Delete unneeded compiler test (Hennadii Stepanov)
396e885886 autotools: Align MSan checking code with CMake's implementation (Hennadii Stepanov)
abde59f52d cmake: Report more compiler details in summary (Hennadii Stepanov)
7abf979a43 cmake: Disable `ctime_tests` if build with `-fsanitize=memory` (Hennadii Stepanov)
Pull request description:
Same as https://github.com/bitcoin-core/secp256k1/pull/1517, but for the CMakle build system.
The second commit improves the configure summary (similar to https://github.com/hebasto/bitcoin/pull/189.
ACKs for top commit:
real-or-random:
ACK f55703ba49
Tree-SHA512: 18190c062ae6e27d0ecbe7460cc22c960b25c0d35aa4b94f151d4b1c48f16e99fd5ecdfcb359784f95995292633d30d3d23b75a12be3aca5afffcc1e7e7daf31
ebfb82ee2f ci: Add job with -fsanitize-memory-param-retval (Tim Ruffing)
e1bef0961c configure: Move "experimental" warning to bottom (Tim Ruffing)
55e5d975db autotools: Disable eager MSan in ctime_tests (Tim Ruffing)
Pull request description:
This is the autotools solution for #1516.
Alternatively, we could have a full-blown `--enable-msan` option, but it's more work, and I'm not convinced that it's necessary or at least much better.
hebasto If you're Concept ACK, are you willing to work on an equivalent PR for CMake?
ACKs for top commit:
hebasto:
ACK ebfb82ee2f, tested on Ubuntu 24.04 with different clang versions (from 15 to 18) and different build configurations. CI changes look OK as well.
Tree-SHA512: c083d778fd50bd35c2e29b7fe0d92b98d912ee5ac7809ae73067d050a0d3c42b3483260f1286d0023cdb802a3c3006bf932ecf60ce81b942de1c9824374c0132
Note that the already existing function `random_fe_magnitude` is removed
and the call-sites are adapted to pass the magnitude range of 8
(the maximum for secp256k1_fe_mul and secp256k1_fe_sqr) explicitly.
9554362b15 tests: call secp256k1_ecmult_multi_var with a non-NULL error callback (Nicolas Iooss)
Pull request description:
Hello,
This Pull Request fixes the issue reported in https://github.com/bitcoin-core/secp256k1/issues/1527. Function `secp256k1_ecmult_multi_var` expects to be called with a non-`NULL` `error_callback` parameter. Fix the invocation in `test_ecmult_accumulate` to do this. While at it, wrap the call in a `CHECK` macro to ensure it succeeds.
ACKs for top commit:
real-or-random:
utACK 9554362b15
siv2r:
ACK 9554362, I have also verified that other invocations of `ecmult_multi_var` (in tests) don’t use `NULL` for the error callback function argument.
Tree-SHA512: 6a9f6c10c575794da75f2254d6fbbc195de889c81a371ce35ab38e2e5483aa1e25ec0bcd5aa8d6a32a1493586f73430208a4bd0613e373571d2f04d63dbc4a1c
Detecting whether it is the top level by comparing the value of
`CMAKE_SOURCE_DIR` with `CMAKE_CURRENT_SOURCE_DIR` is supported by all
versions of CMake and is a very common pattern.
9f4c8cd730 cmake: Fix `check_arm32_assembly` when using as subproject (Hennadii Stepanov)
Pull request description:
When integrating libsecpk1 in a downstream project like this:
```cmake
set(SECP256K1_ASM arm32 CACHE STRING "" FORCE)
add_subdirectory(src/secp256k1)
```
it fails to configure:
```
CMake Error at /home/hebasto/git/bitcoin/build/check_arm32_assembly/CMakeFiles/CMakeTmp/CMakeLists.txt:21 (target_sources):
Cannot find source file:
/home/hebasto/git/bitcoin/cmake/source_arm32.s
CMake Error at /home/hebasto/git/bitcoin/build/check_arm32_assembly/CMakeFiles/CMakeTmp/CMakeLists.txt:20 (add_executable):
No SOURCES given to target: cmTC_d0f0b
CMake Error at src/secp256k1/cmake/CheckArm32Assembly.cmake:2 (try_compile):
Failed to generate test project build system.
Call Stack (most recent call first):
src/secp256k1/CMakeLists.txt:127 (check_arm32_assembly)
```
This PR fixes this issue, which was overlooked in https://github.com/bitcoin-core/secp256k1/pull/1304.
ACKs for top commit:
real-or-random:
utACK 9f4c8cd730
theuni:
utACK 9f4c8cd730
Tree-SHA512: 47d97ad0fb2e3779523c2111ea75906671a0fb3f50646e29dee195f53106ace69af5e4abc92c765f0eee6973528ce9195b94377d0157209230c958894d4049fb
Function secp256k1_ecmult_multi_var expects to be called with a non-NULL
error_callback parameter. Fix the invocation in test_ecmult_accumulate
to do this.
While at it, wrap the call in a CHECK macro to ensure it succeeds.
Fixes: https://github.com/bitcoin-core/secp256k1/issues/1527
dd695563e6 check-abi: explicitly provide public headers (Jonas Nick)
Pull request description:
Without this commit, the check-abi shell script outputs false positives because it consider some headers public that are actually not public.
ACKs for top commit:
real-or-random:
ACK dd695563e6
hebasto:
ACK dd695563e6, tested on Ubuntu 24.04.
Tree-SHA512: b26e61639061f5fbbdd47569ba04f91c627feeefc43ec3d529a3ac4012ab6487aa1904bd38100ed190dcaebdffe60895a8c99346720d5dee84a0c457ec3b6f94
7d2591ce12 Add secp256k1_pubkey_sort (Jonas Nick)
Pull request description:
This PR adds a `secp256k1_pubkey_sort` function the the public API which was originally part of the musig PR (#1479). However, I opened a separate PR because it adds internal functions that are also used by the WIP silent payments module.
ACKs for top commit:
sipa:
ACK 7d2591ce12
josibake:
ACK 7d2591ce12
real-or-random:
ACK 7d2591ce12
Tree-SHA512: d0e4464dc9cd4bdb35cc5d9bb4c37a7b71233328319165d49bc940d8d3394a2d74a43d2f73ee7bfe8f3f90a466ee8afcdca75cfbbf3969e218d76b89f4af55fb
4c341f89ab Add changelog entry for SDMC (Pieter Wuille)
a043940253 Permit COMB_BITS < 256 for exhaustive tests (Pieter Wuille)
39b2f2a321 Add test case for ecmult_gen recoded = {-1,0,1} (Pieter Wuille)
644e86de9a Reintroduce projective blinding (Pieter Wuille)
07810d9abb Reduce side channels from single-bit reads (Peter Dettman)
a0d32b597d Optimization: use Nx32 representation for recoded bits (Peter Dettman)
e03dcc44b5 Make secp256k1_scalar_get_bits support 32-bit reads (Pieter Wuille)
5005abee60 Rename scalar_get_bits -> scalar_get_bits_limb32; return uint32_t (Pieter Wuille)
6247f485b6 Optimization: avoid unnecessary doublings in precomputation (Peter Dettman)
15d0cca2a6 Optimization: first table lookup needs no point addition (Pieter Wuille)
7a33db35cd Optimization: move (2^COMB_BITS-1)/2 term into ctx->scalar_offset (Pieter Wuille)
ed2a056f3d Provide 3 configurations accessible through ./configure (Pieter Wuille)
5f7be9f6a5 Always generate tables for current (blocks,teeth) config (Pieter Wuille)
fde1dfcd8d Signed-digit multi-comb ecmult_gen algorithm (Peter Dettman)
486518b350 Make exhaustive tests's scalar_inverse(&x,&x) work (Pieter Wuille)
ab45c3e089 Initial gej blinding -> final ge blinding (Pieter Wuille)
aa00a6b892 Introduce CEIL_DIV macro and use it (Tim Ruffing)
Pull request description:
ACKs for top commit:
real-or-random:
reACK 4c341f89ab
jonasnick:
ACK 4c341f89ab
stratospher:
ACK 4c341f8. Did [these benchmarks](https://github.com/bitcoin-core/secp256k1/pull/1058#issuecomment-1002807283) and saw a 12.4% on gcc 13.2.0 and 11.5% on clang 15.0.0. Also summarised how the precomputed table generation works [here](https://github.com/stratospher/blogosphere/blob/main/sdmc.md) for future me :)
Tree-SHA512: 9a11138e4fb98b98e85c82cd46ed78b29fbe63d6efe61654ef519a64b1e175d63395a8a931c1646f9df8c7daacd796d5fe2384899d5a13a2c7ed2ded696ceed5
