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
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
"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.
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.
42f8c51402 cmake: Add `SECP256K1_LATE_CFLAGS` configure option (Hennadii Stepanov)
Pull request description:
This PR enables users to override compiler flags that have been set by the CMake-based build system, such as warning flags.
The Autotools-based build system has the same feature out-of-the-box.
See more details [here](https://github.com/bitcoin-core/secp256k1/issues/1235#issuecomment-1465330925).
Here are some examples of the new option usage:
```
cmake -S . -B build -DSECP256K1_LATE_CFLAGS="-Wno-extra -Wlong-long"
```
```
cmake -S . -B build -DSECP256K1_BUILD_EXAMPLES=ON -DSECP256K1_LATE_CFLAGS=-O1
cmake --build build
...
In function ‘secp256k1_ecmult_strauss_wnaf’,
inlined from ‘secp256k1_ecmult’ at /home/hebasto/git/secp256k1/src/ecmult_impl.h:353:5:
/home/hebasto/git/secp256k1/src/ecmult_impl.h:291:5: warning: ‘aux’ may be used uninitialized [-Wmaybe-uninitialized]
291 | secp256k1_ge_table_set_globalz(ECMULT_TABLE_SIZE(WINDOW_A) * no, state->pre_a, state->aux);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /home/hebasto/git/secp256k1/src/secp256k1.c:29:
/home/hebasto/git/secp256k1/src/ecmult_impl.h: In function ‘secp256k1_ecmult’:
/home/hebasto/git/secp256k1/src/group_impl.h:174:13: note: by argument 3 of type ‘const secp256k1_fe *’ to ‘secp256k1_ge_table_set_globalz’ declared here
174 | static void secp256k1_ge_table_set_globalz(size_t len, secp256k1_ge *a, const secp256k1_fe *zr) {
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /home/hebasto/git/secp256k1/src/secp256k1.c:30:
/home/hebasto/git/secp256k1/src/ecmult_impl.h:345:18: note: ‘aux’ declared here
345 | secp256k1_fe aux[ECMULT_TABLE_SIZE(WINDOW_A)];
| ^~~
...
```
Please note that in the last case providing `env CFLAGS=-O1` or `-DCMAKE_C_FLAGS=-O1` won't work.
ACKs for top commit:
real-or-random:
ACK 42f8c51402
Tree-SHA512: 2b152e420a4a8ffd5f67857de03ae5ba9f2223e535ac01a867c1025e0619180d8255fdd1e5fb8279b290f0a1c96bcc874043ef968fcd99b1ff4e13041a91b1e1
This also makes the order in which module options are processed
consistent between CMake and autotools (the reverse order of the listing
printed to stdout).
The scheme implemented is described below, and largely follows the paper
"SwiftEC: Shallue–van de Woestijne Indifferentiable Function To Elliptic Curves",
by Chavez-Saab, Rodriguez-Henriquez, and Tibouchi
(https://eprint.iacr.org/2022/759).
A new 64-byte public key format is introduced, with the property that *every*
64-byte array is an encoding for a non-infinite curve point. Each curve point
has roughly 2^256 distinct encodings. This permits disguising public keys as
uniformly random bytes.
The new API functions:
* secp256k1_ellswift_encode: convert a normal public key to an ellswift 64-byte
public key, using additional entropy to pick among the many possible
encodings.
* secp256k1_ellswift_decode: convert an ellswift 64-byte public key to a normal
public key.
* secp256k1_ellswift_create: a faster and safer equivalent to calling
secp256k1_ec_pubkey_create + secp256k1_ellswift_encode.
* secp256k1_ellswift_xdh: x-only ECDH directly on ellswift 64-byte public keys,
where the key encodings are fed to the hash function.
The scheme itself is documented in secp256k1_ellswift.h.
ef49a11d29 build: allow static or shared but not both (Cory Fields)
36b0adf1b9 build: remove warning until it's reproducible (Cory Fields)
Pull request description:
Continuing from here: https://github.com/bitcoin-core/secp256k1/issues/1224#issuecomment-1460438227
Unfortunately it wasn't really possible to keep a clean diff here because of the nature of the change. I suggest reviewing the lib creation stuff in its entirety, sorry about that :\
Rather than allowing for shared and static libs to be built at the same time like autotools, this PR switches to the CMake convention of allowing only 1.
A new `BUILD_SHARED_LIBS` option is added to match CMake convention, as well as a `SECP256K1_DISABLE_SHARED` option which overrides it. That way even projects which have `BUILD_SHARED_LIBS=1` can opt-into a static libsecp in particular.
Details:
Two object libraries are created: `secp256k1_asm` and `secp256k1_precomputed_objs`. Some tests/benchmarks use the object libraries directly, some link against the real lib: `secp256k1`.
Because the objs don't know what they're going to be linked into, they need to be told how to deal with PIC.
The `DEFINE_SYMBOL` property sets the `DLL_EXPORT` define as necessary (when building a shared lib)
ACKs for top commit:
hebasto:
re-ACK ef49a11d29, only [suggested](https://github.com/bitcoin-core/secp256k1/pull/1230#pullrequestreview-1388191165) changes since my recent [review](https://github.com/bitcoin-core/secp256k1/pull/1230#pullrequestreview-1352125381).
real-or-random:
ACK ef49a11d29
Tree-SHA512: 8870de305176fdb677caff0fdfc6f8c59c0e906489cb72bc9980e551002812685e59e20d731f2a82e33628bdfbb7261eafd6f228038cad3ec83bd74335959600
5bb03c2911 Replace `SECP256K1_ECMULT_TABLE_VERIFY` macro by a function (Hennadii Stepanov)
4429a8c218 Suppress `-Wunused-parameter` when building for coverage analysis (Hennadii Stepanov)
Pull request description:
ACKs for top commit:
real-or-random:
utACK 5bb03c2911
jonasnick:
ACK 5bb03c2911
Tree-SHA512: 19a395434ecefea201a03fc45b3f0b88f1520908926ac1207bbc6570034b1141b49c3c98e66819dcd9069dfdd28c7c6fbe957f13fb6bd178fd57ce65bfbb8fbd
