The number of test iterations in the CI remains unchanged.
Additionally, the minimum iteration counts to enable the
`test_ecmult_constants_2bit` test is adjusted from 35 to 16, so it is
run by default.
7057d3c9af ci: Silent Homebrew's noisy reinstall warnings (Hennadii Stepanov)
Pull request description:
Homebrew's warnings are quite noisy on the master branch:
This PR silents them to allow us to focus on any other CI infra warnings once they happen.
ACKs for top commit:
real-or-random:
utACK 7057d3c9af
Tree-SHA512: 4dbdb459e97cb5956219214fd7407ec52fc018dba24ae945337ad5c94df510472cfc708f9f67d4df5b8e983bf4e78b2efd56533f28113b3463a1397d4d7a032a
763d938cf0 ci: only enable extrakeys module when schnorrsig is enabled (Jonas Nick)
af551ab9db tests: do not use functions from extrakeys module (Jonas Nick)
Pull request description:
ACKs for top commit:
sipa:
ACK 763d938cf0
hebasto:
ACK 763d938cf0.
Tree-SHA512: a5d52e69af896e7bb43c6826f269b5f26e8d1b8c0d040d71a369fdaebf894e47dcf9ff1bbe1b62864adb85437d2c93eabd655c4d690d5d894b712145c2bd252f
e2af491263 ci: Switch to the new default value of the precomputed table for signing (Hennadii Stepanov)
d94a9273f8 build: Adjust the default size of the precomputed table for signing (Hennadii Stepanov)
Pull request description:
This PR implements the [outcomes](https://github.com/bitcoin-core/secp256k1/issues/1549#issuecomment-2200559257) from today's IRC meeting:
1. The default size of the precomputed table for signing is now aligned with Bitcoin Core's [default](a057869aa3).
2. The default value in CI has been updated to reflect the new default.
ACKs for top commit:
sipa:
utACK e2af491263
real-or-random:
utACK e2af491263
Tree-SHA512: aa9db5bc2aec29a35a503a80617a4c096e9909648084fe1ce43b5dd7e74dd812e7642305bd5bc13eb581efc23f12904e200e13cb1a35955b773e05ab4f84be4e
9420eece24 cmake: Bump CMake minimum required version up to 3.16 (Hennadii Stepanov)
Pull request description:
Debian 10 [reached](https://wiki.debian.org/DebianReleases) EOL LTS yesterday, on 2024-06-30.
There no longer seem to be compelling reasons to maintain support for CMake 3.13.
The suggested minimum required version, CMake 3.16, is shipped with Ubuntu 20.04 LTS, which is [supported](https://wiki.ubuntu.com/Releases) until April 2025.
Debian 11 ships with CMake 3.18 (3.25 in backports). In [other](https://repology.org/project/cmake/versions) major distros and package managers, CMake versions are not older.
ACKs for top commit:
fanquake:
ACK 9420eece24 - If the only significant change is for some MSVC compat code, this doesn't seem super compelling, but seems ok either way.
real-or-random:
utACK 9420eece24
Tree-SHA512: 7066f053f9820f9ddeb27f9e8042edb67a31cd61e29eadff12b50d63bbe278f6ffd15f84035932bc4c337461a7eb342339ed62e46e100c5447106a702fad229c
b8fe33332b cmake: Fixed O3 replacement (Eduardo Menges Mattje)
Pull request description:
Old replacement of `O3` in `CMAKE_C_FLAGS_RELEASE` skip spaces, which is problematic. For instance, if `CMAKE_C_FLAGS_RELEASE = "-O3 -DFOO"`, regex will replace it with `-O2-DFOO`, which causes a compile error.
This patch changes this behavior, keeping whichever space exists between the flags.
If I may question, what is the rationale behind replacing `O3` with `O2`? Changing the user's flags is a bad practice overall, and I don't see how this replacement is beneficial.
ACKs for top commit:
hebasto:
re-ACK b8fe33332b.
Tree-SHA512: d84091eb594af695805784e87f49d38ac08a7cf42978c844383882196389a0d83fc5f72e6394887b9961cc3f96538962f4ee86ef3eeca2c96509c64428046c70
This should hopefully be useful as orientation for users implementing
the key exchange part of BIP324. Conceptually the example is not very
different to the ECDH one, so a lot of code/comments are just copied
(e.g. context creation, secret key generation, shared secret comparison,
console output, cleanup with secret key clearing).
4d9645bee0 cmake: Remove "AUTO" value of `SECP256K1_ECMULT_GEN_KB` option (Hennadii Stepanov)
a06805ee74 cmake: Remove "AUTO" value of `SECP256K1_ECMULT_WINDOW_SIZE` option (Hennadii Stepanov)
26b94ee92a autotools: Remove "auto" value of `--with-ecmult-gen-kb` option (Hennadii Stepanov)
122dbaeb37 autotools: Remove "auto" value of `--with-ecmult-window` option (Hennadii Stepanov)
Pull request description:
"auto" implies that a value is being chosen based on build system introspection or host system capabilities. However, for the `--with-ecmult-window` and `--with-ecmult-gen-kb` options, the values "auto" are hardcoded, which might lead to confusion.
This PR replaces "auto" with more appropriate default values.
If Concept ACKed, I'll add equivalent commits for CMake.
ACKs for top commit:
sipa:
utACK 4d9645bee0
real-or-random:
utACK 4d9645bee0 good from my side, but let's see if we can get more (Concept) ACKs
Tree-SHA512: 9e68f73682c5310c68d2337594f13b99a52bfc365564e39df2e412b576635c90cccd2298406a4281f014916c4a1710e19c7390f05a4b0acbd09869bfb56f36ac
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
"AUTO" implies that a value is being chosen based on build system
introspection or host system capabilities. However, for the
`SECP256K1_ECMULT_GEN_KB` option, the value "AUTO" is hardcoded, which
might lead to confusion.
This change replaces "AUTO" with a more appropriate default value.
"AUTO" implies that a value is being chosen based on build system
introspection or host system capabilities. However, for the
`SECP256K1_ECMULT_WINDOW_SIZE` option, the value "AUTO" is hardcoded,
which might lead to confusion.
This change replaces "AUTO" with a more appropriate default value.
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
"auto" implies that a value is being chosen based on build system
introspection or host system capabilities. However, for the
`--with-ecmult-gen-kb` option, the value "auto" is hardcoded, which
might lead to confusion.
This change replaces "auto" with a more appropriate default value.
"auto" implies that a value is being chosen based on build system
introspection or host system capabilities. However, for the
`--with-ecmult-window` option, the value "auto" is hardcoded, which
might lead to confusion.
This change replaces "auto" with a more appropriate default value.
