1ecb94ebe9800900c7dd3a4f9883c600e25eecf7 build: Make `SECP_VALGRIND_CHECK` preserve `CPPFLAGS` (Hennadii Stepanov)
Pull request description:
It was overlooked in #862 and #1027.
ACKs for top commit:
real-or-random:
utACK 1ecb94ebe9800900c7dd3a4f9883c600e25eecf7
Tree-SHA512: 263fc600ce9743e4aad767150f706bf7d4325dabb9c363ed57f08fe38faea94d7d1999804947cffeacbe698bb6d959ee6de3f6e50400050a390ecc0db957e426
35ada3b954ccc6c54628fb3bcc0365d176297019 tests: lint wycheproof's python script (RandomLattice)
Pull request description:
This PR lints tests_wycheproof_generate.py according to bitcoin's python linting scripts. This is a follow-up to PR #1245.
ACKs for top commit:
sipa:
utACK 35ada3b954ccc6c54628fb3bcc0365d176297019
real-or-random:
utACK 35ada3b954ccc6c54628fb3bcc0365d176297019
Tree-SHA512: ea405060d2e73ff3543626687de5bc5282be923b914bd5c8c53e65df8dca9bea0000c416603095efff29bc7ae43c2081454c4e506db0f6805443d023fbffaf4c
ef49a11d29601e09e94134975c968e92c0214102 build: allow static or shared but not both (Cory Fields)
36b0adf1b90139a41fdcb94390d0bb06e9224795 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 ef49a11d29601e09e94134975c968e92c0214102, 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 ef49a11d29601e09e94134975c968e92c0214102
Tree-SHA512: 8870de305176fdb677caff0fdfc6f8c59c0e906489cb72bc9980e551002812685e59e20d731f2a82e33628bdfbb7261eafd6f228038cad3ec83bd74335959600
a575339c0282ba49a4f46c9c660a4cc3b6bfc703 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 a575339c0282ba49a4f46c9c660a4cc3b6bfc703
real-or-random:
utACK a575339c0282ba49a4f46c9c660a4cc3b6bfc703
Tree-SHA512: 994b1f19b4c513f6d070ed259a5d6f221a0c2450271ec824c5eba1cd0ecace276de391c170285bfeae96aaf8f1e0f7fe6260966ded0336c75c522ab6c56d182c
This PR lints tests_wycheproof_generate.py according to pylint.
This is a follow-up to PR #1245.
Co-authored-by: Sean Andersen <6730974+andozw@users.noreply.github.com>
06c67dea9f6d46d3e24e810900fbb03045eae641 autotools: Don't regenerate Wycheproof header automatically (Tim Ruffing)
Pull request description:
This is a hot fix for https://github.com/bitcoin/bitcoin/pull/27445 .
---
Pregenerated files that we distribute should not have dependencies in Makefile.am. For rationale, see the comments about the precomputed table files.
See also https://github.com/bitcoin/bitcoin/pull/27445#issuecomment-1502994264 .
ACKs for top commit:
hebasto:
ACK 06c67dea9f6d46d3e24e810900fbb03045eae641
RandomLattice:
ACK 06c67dea9f
Tree-SHA512: fa7f44eaa1c7e42ecba5829ac1b8ae8b5826d1a1551e01c3caf37af780bd5c102c8f54e88520723937f7016d93c67b62a334c7a28b96c4f422a38fcf8e6a1984
6a37b2a5ea9075c5dff14b3067c61114a334a2ba changelog: Fix link (Tim Ruffing)
Pull request description:
Top commit has no ACKs.
Tree-SHA512: 70d50c8fe958a197eb527e51c6f8120609e3166d93bfc1bbec75a3cb565c406d5ba0e6d088a724dcfda422b6594abf53f507211946a0533515df371d5d2a91bf
e5de45460953c8ae16521b1928ac14de218998a3 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 e5de45460953c8ae16521b1928ac14de218998a3
real-or-random:
ACK e5de45460953c8ae16521b1928ac14de218998a3
Tree-SHA512: e9684f14ff3f5225a4a4949b490e07527d559c28aa61ed03c03bc52ea64785f0b80b9e1b1628665eacf24006526271ea0fb108629c9c3c1d758e52d214a056f1
0f8642079b0f2e4874393792f5854e3c33742cbd Add exhaustive tests for ecmult_const_xonly (Pieter Wuille)
4485926ace489d87929be5218ae1ff3aa8591006 Add x-only ecmult_const version for x=n/d (Pieter Wuille)
Pull request description:
This implements a generalization of Peter Dettman's sqrt-less x-only random-base multiplication algorithm from #262, using the Jacobi symbol algorithm from #979. The generalization is to permit the X coordinate of the base point to be specified as a fraction $n/d$:
To compute $x(q \cdot P)$, where $x(P) = n/d$:
* Compute $g=n^3 + 7d^3$.
* Let $P' = (ng, g^2, 1)$ (the Jacobian coordinates of $P$ mapped to the isomorphic curve $y^2 = x^3 + 7(dg)^3$).
* Compute the Jacobian coordinates $(X',Y',Z') = q \cdot P'$ on the isomorphic curve.
* Return $X'/(dgZ'^2)$, which is the affine x coordinate on the isomorphic curve $X/Z'^2$ mapped back to secp256k1.
This ability to specify the X coordinate as a fraction is useful in the context of x-only [Elligator Swift](https://eprint.iacr.org/2022/759), which can decode to X coordinates on the curve without inversions this way.
ACKs for top commit:
jonasnick:
ACK 0f8642079b0f2e4874393792f5854e3c33742cbd
real-or-random:
ACK 0f8642079b0f2e4874393792f5854e3c33742cbd
Tree-SHA512: eeedb3045bfabcb4bcaf3a1738067c83a5ea9a79b150b8fd1c00dc3f68505d34c19654885a90e2292ae40ddf40a58dfb27197d98eebcf5d6d9e25897e07ae595
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>
3d1f430f9f32d45885b0a10b448c0f15386c423d Make position of * in pointer declarations in include/ consistent (Jonas Nick)
Pull request description:
ACKs for top commit:
sipa:
utACK 3d1f430f9f32d45885b0a10b448c0f15386c423d. I have not verified these are the only instances where changes would need to be made.
apoelstra:
utACK 3d1f430 from me too. I also value consistency more than either specific choice.'
real-or-random:
utACK 3d1f430f9f
Tree-SHA512: 6361880f4a47e58c83623f094dd121882752fa805e275033cd638d1e8d3477ade9037e5d9e34a57ae46013848648bd7ab764cad326133f2d3435c9a70a0c841b
4a496a36fb07d6cc8c99e591994f4ce0c3b1174c 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 4a496a36fb07d6cc8c99e591994f4ce0c3b1174c
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
5bb03c29116409ace8855e64bf2e2b2d45871469 Replace `SECP256K1_ECMULT_TABLE_VERIFY` macro by a function (Hennadii Stepanov)
4429a8c218d7bf7bc6de1de88bc31c834f771385 Suppress `-Wunused-parameter` when building for coverage analysis (Hennadii Stepanov)
Pull request description:
ACKs for top commit:
real-or-random:
utACK 5bb03c29116409ace8855e64bf2e2b2d45871469
jonasnick:
ACK 5bb03c29116409ace8855e64bf2e2b2d45871469
Tree-SHA512: 19a395434ecefea201a03fc45b3f0b88f1520908926ac1207bbc6570034b1141b49c3c98e66819dcd9069dfdd28c7c6fbe957f13fb6bd178fd57ce65bfbb8fbd
3e43041be68c1288ad9897525a15e21945fb3eb9 No need to subtract 1 before doing a right shift (roconnor-blockstream)
Pull request description:
ACKs for top commit:
real-or-random:
utACK 3e43041be68c1288ad9897525a15e21945fb3eb9
jonasnick:
ACK 3e43041be68c1288ad9897525a15e21945fb3eb9
Tree-SHA512: bcecda11eae3fb845bef7af88c6171bedcd933872d08a9849c0a250cb6c9e982a88bd45e8a8364a4a348f8be413fc91ee04cf8fa78adae44e584e3ad7ec544cf
fd2a408647ba0f999b7b217977cc68773fa35257 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 (427bc3cdcfbc74778070494daab1ae5108c71368):
```
$ 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 fd2a408647ba0f999b7b217977cc68773fa35257.
sipa:
ACK fd2a408647ba0f999b7b217977cc68773fa35257
Tree-SHA512: abf8ad332631672c036844f69c5599917c49e12c4402bf9066f93a692d3007b1914bd3eea8f83f0141c1b09d5c88ebc5e6c8bfbb5444b7b3471749f7b901ca59
4ebd82852d3ad00ab579b26173575a4f4642ea76 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 4ebd82852d3ad00ab579b26173575a4f4642ea76
jonasnick:
ACK 4ebd82852d3ad00ab579b26173575a4f4642ea76
Tree-SHA512: c24211e5219907e41e2c5792255734bd50ca5866a4863abbb3ec174ed92d1792dd10563a94c08e8fecd6cdf776a9c49ca87e8f9806a023d9081ecc0d55ae3e66
96dd0625112672e841eea723398cc2a1c3489a30 build: bump CMake minimum requirement to 3.13 (Cory Fields)
Pull request description:
As requested here: https://github.com/bitcoin-core/secp256k1/pull/1230#issuecomment-1464730218 . Ping @hebasto
Among other things this allows us to link against object libraries.
3.13 has been mentioned several times as a good overlap between newish features and widespread Linux availability.
ACKs for top commit:
hebasto:
ACK 96dd0625112672e841eea723398cc2a1c3489a30
real-or-random:
utACK 96dd0625112672e841eea723398cc2a1c3489a30
Tree-SHA512: 6c744809aa393b48ef10b3d46c6630370c388a8d375116bfad65c6c907e69c36ed71c1579b9d5c3aa976f70b1cd70e837c1a0226910a43539435125115b32568
8e79c7ed11fa50bd6b8a3d3203b2fc330a0c37ea build: Ensure no optimization when building for coverage analysis (Hennadii Stepanov)
Pull request description:
#944 introduced a regression when building for coverage analysis. The `-O2` flag from the default Autoconf's `CFLAGS` overrides the coverage-specific `-O0` one, which makes coverage analysis results [less reliable](https://gcc.gnu.org/onlinedocs/gcc/Gcov-and-Optimization.html).
This PR restores the pre-#944 behaviour.
In contrast to an alternative smaller diff:
```diff
--- a/configure.ac
+++ b/configure.ac
@@ -240,7 +240,7 @@ fi
if test x"$enable_coverage" = x"yes"; then
SECP_CONFIG_DEFINES="$SECP_CONFIG_DEFINES -DCOVERAGE=1"
- SECP_CFLAGS="-O0 --coverage $SECP_CFLAGS"
+ CFLAGS="$CFLAGS -O0 --coverage "
LDFLAGS="--coverage $LDFLAGS"
else
# Most likely the CFLAGS already contain -O2 because that is autoconf's default.
```
this PR ensures that the user always has the last word.
FWIW, Bitcoin Core uses a similar [approach](460e394625/configure.ac (L879-L884)).
ACKs for top commit:
jonasnick:
tested ACK 8e79c7ed11fa50bd6b8a3d3203b2fc330a0c37ea
real-or-random:
utACK 8e79c7ed11fa50bd6b8a3d3203b2fc330a0c37ea
Tree-SHA512: f04b55921d397bd7c003ec0283101d3908f3fb507789c855e1b6d5abd150e7d6281d5eeb8fefbb7d6a55b3c6f29a19324f570eee009794f8fa9bca956229e7ce
