f7f0184ba1 msan: notate more variable assignments from assembly code (Cory Fields)
a61339149f change inconsistent array param to pointer (Cory Fields)
Pull request description:
This was missed in 31ba404944 because older versions of clang did not complain about it. But clang-17, at least, does.
The array-as-a-param makes this annoying because `sizeof(l)` is not helpful. I'd be happy to change the size calculation if there are any better suggestions or strong preferences.
ACKs for top commit:
sipa:
utACK f7f0184ba1
real-or-random:
ACK f7f0184ba1 tests work fine with clang 17 and `./configure CFLAGS="-fsanitize=memory -fsanitize-memory-track-origins=2 -fno-omit-frame-pointer -g -O1 -fno-optimize-sibling-calls" CC=clang`
Tree-SHA512: 8ab22209ef322a10f500b123c82ae5e7141ae1da0e7a890cbf90bd7d2eb11f397db4ccfe15a1666f2f49228585cccbf5bec741effebd1e2c6012cb7ea1689675
a5e8ab2484 ci: Add sanitizer env variables to debug output (Tim Ruffing)
84a93de4d2 ci: Add workaround for ASLR bug in sanitizers (Tim Ruffing)
Pull request description:
Fixes#1506.
This also adds the sanitizer env variables to our debug output as suggested in the same issue.
ACKs for top commit:
sipa:
utACK a5e8ab2484
jonasnick:
ACK a5e8ab2484
Tree-SHA512: 5162d14eeec01e088c600ed77e21c5ffd4dec23327b7e81b5ecac59b7c535cac97cd7b7b744c767766036dfc6d9152a9933eb326cf4065d56c46e2ee858da662
3a9b1d46a3 New Experimental Module: Incremental Half-Aggregation for Schnorr Signatures (Benedikt)
Pull request description:
Revisited PR #130 by jonasnick.
I am happy to hear your thoughts.
**Summary of changes compared to #130:**
- Address comments from rustyrussell
- Use tagged hash
- Compute hashes with common prefix by copying midstate
- Allow Incremental Aggregation and make code consistent with the [draft spec](https://github.com/BlockstreamResearch/cross-input-aggregation/blob/master/half-aggregation.mediawiki)
ACKs for top commit:
real-or-random:
ACK 3a9b1d46a3
Tree-SHA512: 27239033f8b28ecf87ea310b3dd5a19dbbe6fd07495db71ef7017f8f444ec25a12897087d1bea0a2e9c3df77d7f17c38b183d7fe768858da2180f26624add4aa
"... neither can be equal to b." could suggest that the values are not
allowed to be identical, but what is meant here is that the mentioned
inputs shouldn't point to the same object.
Currently the `run_sqr` test doesn't do anything with the
result of the `fe_sqr` call. Improve that by checking that
the equation `(x+y)*(x-y) = x^2 - y^2` holds for some random
values y, as suggested in issue #1471 by real-or-random.
The existing loop for generating the x values is kept as-is.
31ba404944 msan: notate variable assignments from assembly code (Cory Fields)
e7ea32e30a msan: Add SECP256K1_CHECKMEM_MSAN_DEFINE which applies to memory sanitizer and not valgrind (Cory Fields)
Pull request description:
msan isn't smart enough to see that these are set without some help.
This was pointed out here: https://github.com/bitcoin-core/secp256k1/pull/1169#issuecomment-1370003449
With this commit, msan output is clean even with x86 asm turned on.
ACKs for top commit:
real-or-random:
utACK 31ba404944
hebasto:
re-ACK 31ba404944.
Tree-SHA512: c9c51fe542247e1e0a93f6d0063d119cf777ca8c1b7e9c8e45e168a2020dc503872eb2a78004725de81267a3ce78c923be1f8546fb92a3e95fc7ef034e5ba932
`check_fe_equal` is a wrapper around `secp256k1_fe_equal` that takes
care of normalization. Since it doesn't check anything itself, the
CHECK macro is needed at the call-sites to actually ensure equality.
The previous implementation returns an off-curve point for the input t=0.
This rewrite addresses that issue by implicity returning the on-curve point
(d, sqrt(1 + b)), which is the point that the paper Indifferentiable Hashing
to Barreto–Naehrig Curves suggests returning in this case.
Note: At the moment it is cryptographically impossible for the input t to be 0.
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
e6822678ea build: Error if required module explicitly off (Tim Ruffing)
89ec583ccf build: Clean up handling of module dependencies (Tim Ruffing)
Pull request description:
This is a cleanup which makes it easier to add further modules with dependencies, e.g., in #1452. The diff looks larger than it is because I also reordered the modules and made the order consistent between CMake and autotools.
(We noticed that the current logic could be improved in https://github.com/BlockstreamResearch/secp256k1-zkp/pull/275.)
ACKs for top commit:
jonasnick:
ACK e6822678ea
hebasto:
ACK e6822678ea.
Tree-SHA512: 040e791e5b5b9b8845a39632633a45ca759391455910bdefba2b7b77c6340e65df6eda18199ae2ad65c30ee2fc6630471437aec143c26fe09ae4c11409a37622
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).
ba5d72d626 assumptions: Use new STATIC_ASSERT macro (Tim Ruffing)
e53c2d9ffc Require that sizeof(secp256k1_ge_storage) == 64 (Tim Ruffing)
d0ba2abbff util: Add STATIC_ASSERT macro (Tim Ruffing)
Pull request description:
This gets rid of an untested code path. Resolves https://github.com/bitcoin-core/secp256k1/issues/1352.
This is a bit opinionated in the sense that it adds a static assertion where it's needed in `secp256k1_pubkey_save` and `secp256k1_pubkey_load`. I think this is justified in this case. It helps the reviewer verify that these functions are correct.
See individual commit messages.
ACKs for top commit:
sipa:
utACK ba5d72d626
jonasnick:
ACK ba5d72d626
Tree-SHA512: 2553c0610b62bcda6d4ef26eb26b5b2e07acf723bcd299691a2d02da57af22b8763f63c2d4adb17d30de8825b6157be6e4f0398147854fbabdf8b865fb0b5c88
