bef448f9af cmake: Fix library ABI versioning (Hennadii Stepanov)
Pull request description:
This change emulates Libtool to make sure Libtool and CMake agree on the ABI version.
To test, one needs to simulate a release with backward-compatible API changes, which means the following changes in `configure.ac` and `CMakeLists.txt`:
- incrementing of `*_LIB_VERSION_CURRENT`
- setting `*_LIB_VERSION_REVISION` to zero
- incrementing of `*_LIB_VERSION_AGE`
ACKs for top commit:
real-or-random:
ACK bef448f9af diff looks good and I tested on Linux
Tree-SHA512: f7551fc7377ea50c8bc32d14108a034a1f91ebbb63d5fec562e5cc28416637834b9a4dcba3692df1780adcd1212ad4f238dc0219ab5add68bd88a5a458572ee5
An executable target in the `COMMAND` option will automatically be
replaced by the location of the executable created at build time.
This change fixes tests for Windows binaries using Wine.
a273d74b2e cmake: Improve version comparison (Hennadii Stepanov)
6a58b483ef cmake: Use `if(... IN_LIST ...)` command (Hennadii Stepanov)
2445808c02 cmake: Use dedicated `GENERATOR_IS_MULTI_CONFIG` property (Hennadii Stepanov)
9f8703ef17 cmake: Use dedicated `CMAKE_HOST_APPLE` variable (Hennadii Stepanov)
8c2017035a cmake: Use recommended `add_compile_definitions` command (Hennadii Stepanov)
04d4cc071a cmake: Add `DESCRIPTION` and `HOMEPAGE_URL` options to `project` command (Hennadii Stepanov)
8a8b6536ef cmake: Use `SameMinorVersion` compatibility mode (Hennadii Stepanov)
Pull request description:
This PR:
- resolves two items from #1235, including a bugfix with package version compatibility
- includes other improvements which have become available for CMake 3.13+.
To test the `GENERATOR_IS_MULTI_CONFIG` property on Linux, one can use the "[Ninja Multi-Config](https://cmake.org/cmake/help/latest/generator/Ninja%20Multi-Config.html)" generator:
```sh
cmake -S . -B build -G "Ninja Multi-Config"
```
ACKs for top commit:
real-or-random:
ACK a273d74b2e
theuni:
ACK a273d74b2e
Tree-SHA512: f31c4f0f30bf368303e70ab8952cde5cc8c70a5e79a04f879abcbee3d0a8d8c598379fb38f5142cb1f8ff5f9dcfc8b8eb4c13c975a1d05fdcc92d9c805a59d9a
69e1ec0331 Get rid of secp256k1_fe_const_b (Pieter Wuille)
Pull request description:
Replaces #1282.
Its only remaining use is in a test introduced in #1118, and it is easily replaced by the new `secp256k1_fe_add_int` from #1217.
ACKs for top commit:
real-or-random:
utACK 69e1ec0331
Tree-SHA512: 6ada192e0643fc5326198b60f019a5081444f9ba0a5b8ba6236f2a526829d8e5e479556600a604d9bc96c7ba86e3aab813f93c66679287d2135e95a2b75f5d3e
8e142ca410 Move `SECP256K1_INLINE` macro definition out from `include/secp256k1.h` (Hennadii Stepanov)
77445898a5 Remove `SECP256K1_INLINE` usage from examples (Hennadii Stepanov)
Pull request description:
From [IRC](https://gnusha.org/secp256k1/2023-01-31.log):
> 06:29 \< hebasto\> What are reasons to define the `SECP256K1_INLINE` macro in user's `include/secp256k1.h` header, while it is used internally only?
> 06:32 \< hebasto\> I mean, any other (or a new dedicated) header in `src` looks more appropriate, no?
> 06:35 \< sipa\> I think it may just predate any "utility" internal headers.
> 06:42 \< sipa\> I think it makes sense to move it to util.h
Pros:
- it is a step in direction to better organized headers (in context of #924, #1039)
Cons:
- code duplication for `SECP256K1_GNUC_PREREQ` macro
ACKs for top commit:
sipa:
utACK 8e142ca410
real-or-random:
utACK 8e142ca410
Tree-SHA512: 180e0ba7c2ef242b765f20698b67d06c492b7b70866c21db27c18d8b2e85c3e11f86c6cb99ffa88bbd23891ce3ee8a24bc528f2c91167ec2fddc167463f78eac
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
a575339c02 Remove bits argument from secp256k1_wnaf_const (always 256) (Pieter Wuille)
Pull request description:
There is little reason for having the number of bits in the scalar as a parameter, as I don't think there are any (current) use cases for non-256-bit scalars.
ACKs for top commit:
jonasnick:
ACK a575339c02
real-or-random:
utACK a575339c02
Tree-SHA512: 994b1f19b4c513f6d070ed259a5d6f221a0c2450271ec824c5eba1cd0ecace276de391c170285bfeae96aaf8f1e0f7fe6260966ded0336c75c522ab6c56d182c
e5de454609 tests: Add Wycheproof ECDSA vectors (RandomLattice)
Pull request description:
This PR adds a test using the Wycheproof vectors as outlined in #1106. We add all 463 ECDSA test vectors. These vectors cover:
- edge cases in arithmetic operations
- signatures with special values for (r,s) that should be rejected
- special cases of public keys
The vectors are pulled from the Wycheproof project using a python script to emit C code.
All the new ECDSA Wycheproof vectors pass.
ACKs for top commit:
sipa:
ACK e5de454609
real-or-random:
ACK e5de454609
Tree-SHA512: e9684f14ff3f5225a4a4949b490e07527d559c28aa61ed03c03bc52ea64785f0b80b9e1b1628665eacf24006526271ea0fb108629c9c3c1d758e52d214a056f1
Adds a test using the Wycheproof vectors as outlined in #1106. The
vectors are taken from the Wycheproof repo. We use a python script
to convert the JSON-formatted vectors into C code.
Co-authored-by: Sean Andersen <6730974+andozw@users.noreply.github.com>
4a496a36fb ct: Use volatile "trick" in all fe/scalar cmov implementations (Tim Ruffing)
Pull request description:
Apparently clang 15 is able to compile our cmov code into a branch, at least for fe_cmov and fe_storage_cmov. This commit makes the condition volatile in all cmov implementations (except ge but that one only calls into the fe impls).
This is just a quick fix. We should still look into other methods, e.g., asm and #457. We should also consider not caring about constant-time in scalar_low_impl.h
We should also consider testing on very new compilers in nightly CI, see https://github.com/bitcoin-core/secp256k1/pull/864#issuecomment-769211867
ACKs for top commit:
jonasnick:
ACK 4a496a36fb
Tree-SHA512: a6010f9d752e45f01f88b804a9b27e77caf5ddf133ddcbc4235b94698bda41c9276bf588c93710e538250d1a96844bcec198ec5459e675f166ceaaa42da921d5
Apparently clang 15 is able to compile our cmov code into a branch,
at least for fe_cmov and fe_storage_cmov. This commit makes the
condition volatile in all cmov implementations (except ge but that
one only calls into the fe impls).
This is just a quick fix. We should still look into other methods,
e.g., asm and #457. We should also consider not caring about
constant-time in scalar_low_impl.h
We should also consider testing on very new compilers in nightly CI,
see https://github.com/bitcoin-core/secp256k1/pull/864#issuecomment-769211867
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
fd2a408647 Set ARM ASM symbol visibility to `hidden` (Hennadii Stepanov)
Pull request description:
Solves one item in #1181.
To test on arm-32bit hardware, run:
```
$ ./autogen.sh && ./configure --enable-experimental --with-asm=arm && make
```
On master branch (427bc3cdcf):
```
$ nm -D .libs/libsecp256k1.so | grep secp256k1_fe
0000e2bc T secp256k1_fe_mul_inner
0000e8dc T secp256k1_fe_sqr_inner
```
With this PR:
```
$ nm -D .libs/libsecp256k1.so | grep secp256k1_fe | wc -l
0
```
For reference, see https://sourceware.org/binutils/docs/as/Hidden.html.
ACKs for top commit:
theuni:
ACK fd2a408647.
sipa:
ACK fd2a408647
Tree-SHA512: abf8ad332631672c036844f69c5599917c49e12c4402bf9066f93a692d3007b1914bd3eea8f83f0141c1b09d5c88ebc5e6c8bfbb5444b7b3471749f7b901ca59
4ebd82852d Apply Checks only in VERIFY mode. (roconnor-blockstream)
Pull request description:
This is already done in `field_5x52_impl.h`.
ACKs for top commit:
sipa:
ACK 4ebd82852d
jonasnick:
ACK 4ebd82852d
Tree-SHA512: c24211e5219907e41e2c5792255734bd50ca5866a4863abbb3ec174ed92d1792dd10563a94c08e8fecd6cdf776a9c49ca87e8f9806a023d9081ecc0d55ae3e66
5d8f53e312 Remove redudent checks. (Russell O'Connor)
Pull request description:
These abs checks are implied by the subsequent line, and with the subsequent line written as it is, no underflow is possible with signed integers.
Follows up on https://github.com/bitcoin-core/secp256k1/pull/1218.
ACKs for top commit:
sipa:
utACK 5d8f53e312
real-or-random:
ACK 5d8f53e312
Tree-SHA512: ddd6758638fe634866fdaf900224372e2e51cb81ef4d024f169fbc39fff38ef1b29e90e0732877e8910158b82bc428ee9c3a4031882c2850b22ad87cc63ee305
The implementation calls the secp256k1_modinvNN_jacobi_var code, falling back
to computing a square root in the (extremely rare) case it failed converge.
This introduces variants of the divsteps-based GCD algorithm used for
modular inverses to compute Jacobi symbols. Changes compared to
the normal vartime divsteps:
* Only positive matrices are used, guaranteeing that f and g remain
positive.
* An additional jac variable is updated to track sign changes during
matrix computation.
* There is (so far) no proof that this algorithm terminates within
reasonable amount of time for every input, but experimentally it
appears to almost always need less than 900 iterations. To account
for that, only a bounded number of iterations is performed (1500),
after which failure is returned. In VERIFY mode a lower iteration
count is used to make sure that callers exercise their fallback.
* The algorithm converges to f=g=gcd(f0,g0) rather than g=0. To keep
this test simple, the end condition is f=1, which won't be reached
if started with non-coprime or g=0 inputs. Because of that we only
support coprime non-zero inputs.
