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.
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
