42f8c5140227dbdd8ae7eaaecd914e705e1b12d0 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 42f8c5140227dbdd8ae7eaaecd914e705e1b12d0
Tree-SHA512: 2b152e420a4a8ffd5f67857de03ae5ba9f2223e535ac01a867c1025e0619180d8255fdd1e5fb8279b290f0a1c96bcc874043ef968fcd99b1ff4e13041a91b1e1
e6822678ea05c431b4f43be9dfbde54e0f7f645b build: Error if required module explicitly off (Tim Ruffing)
89ec583ccf01d9201fdab6a6c1682e6c27224b16 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 e6822678ea05c431b4f43be9dfbde54e0f7f645b
hebasto:
ACK e6822678ea05c431b4f43be9dfbde54e0f7f645b.
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).
ba5d72d62659f9305d2be30b2ac89ce9480a0e78 assumptions: Use new STATIC_ASSERT macro (Tim Ruffing)
e53c2d9ffc0b0096881e30e388c3fb040f35e05d Require that sizeof(secp256k1_ge_storage) == 64 (Tim Ruffing)
d0ba2abbff2dcd4ca355f648d61fc6520f929949 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 ba5d72d62659f9305d2be30b2ac89ce9480a0e78
jonasnick:
ACK ba5d72d62659f9305d2be30b2ac89ce9480a0e78
Tree-SHA512: 2553c0610b62bcda6d4ef26eb26b5b2e07acf723bcd299691a2d02da57af22b8763f63c2d4adb17d30de8825b6157be6e4f0398147854fbabdf8b865fb0b5c88
da7bc1b803b14274bc1687514e5da6a3e1cd9765 include: in doc, remove article in front of "pointer" (Jonas Nick)
aa3dd5280b4a046c03bd344bfd7f1499199a1f3c include: make doc about ctx more consistent (Jonas Nick)
e3f690015a21d6404cdec30666f721001b493172 include: remove obvious "cannot be NULL" doc (Jonas Nick)
Pull request description:
ACKs for top commit:
sipa:
ACK da7bc1b803b14274bc1687514e5da6a3e1cd9765
real-or-random:
ACK da7bc1b803b14274bc1687514e5da6a3e1cd9765
Tree-SHA512: 809f312fa0cd1e9502ac79b8a1c502b87e6dfc2db8ad6bbd96d7ddbdaadad0c3b6110fa704b770c353cd34d5bf5547541cbb5f2779425d7419b584e721c854c2
This also splits the big "&&" expression into separate expressions. If
we ever see an assertion fail, the error message will tell it precisely
which one failed.
This gets rid of an untested code path. Resolves#1352.
secp256k1_ge_storage is a struct with two secp256k1_fe_storage fields.
The C standard allows the compiler to add padding between the fields and
at the end of the struct, but no sane compiler in the end would do this:
The only reason to add padding is to ensure alignment, but such padding
is never necessary between two fields of the same type.
Similarly, secp256k1_fe_storage is a struct with a single array of
uintXX_t. No padding is allowed between array elements. Again, C allows
the compiler to insert padding at the end of the struct, but there's no
absolute reason to do so in this case.
For the uintXX_t itself, this guaranteed to have no padding bits, i.e.,
it's guaranteed to have exactly XX bits.
So I claim that for any existing compiler in the real world,
sizeof(secp256k1_ge_storage) == 64.
e2eb3fae407f0a081a19baeb2ea22eb965fa9674 Make *key_cache const in musig_pubkey_get (Sanket Kanjalkar)
Pull request description:
ACKs for top commit:
jonasnick:
ACK e2eb3fae407f0a081a19baeb2ea22eb965fa9674
Tree-SHA512: 24d1375bd48440e805e82d8a7d371eebfa98f7ef2c7a60d86c720e8512b5fa5bb70499ea821f9cef81c73145a3569c243fa0ecb1c29d7c31c4515dafeba80e23
3dbfb48946b9d2a98acef23674617510cf1b3386 tests: restore scalar_mul test (Jonas Nick)
Pull request description:
Without this commit, the res[i][1] test vectors are unused. They were introduced to test the correctness of scalar_sqr(x) and scalar_mul(x, x). These tests were deleted as part of removing scalar_sqr in commit
[5437e7bdfbffddf69fdf7b4af7e997c78f5dafbf](5437e7bdfb (diff-c2d5f1f7616875ab71cd41b053cfb428696988ff89642b931a0963d50f34f7e8L2195)).
Discovered in https://github.com/bitcoin-core/secp256k1/discussions/1463 by Coding-Enthusiast (thanks!).
ACKs for top commit:
real-or-random:
utACK 3dbfb48946b9d2a98acef23674617510cf1b3386
Tree-SHA512: 914e08db3efaa1cef546a9730096e740478c422d41fedb2b71ec3a7ea962f81740a05dc7e7c1fb191088f6d38b5690479c7d0864ca8abf2b2e9c4334f03ca605
Without this commit, the res[i][1] test vectors are unused. They were introduced
to test the correctness of scalar_sqr(x) and scalar_mul(x, x). These tests were
deleted as part of removing scalar_sqr in commit
5437e7bdfbffddf69fdf7b4af7e997c78f5dafbf.
74a4d974d5c81fbc437287dffc453028509682ab doc: Add ABI checking with `check-abi.sh` to the Release Process (Hennadii Stepanov)
e7f830e32c61ac4cf6c562b477063ccf35940ba9 Add `tools/check-abi.sh` (Hennadii Stepanov)
Pull request description:
ACKs for top commit:
real-or-random:
ACK 74a4d974d5c81fbc437287dffc453028509682ab it compares the right commits now
jonasnick:
re-Concept ACK 74a4d974d5c81fbc437287dffc453028509682ab
Tree-SHA512: bcca5246837f899d43ced3b0099a8e123f4fd2db7d15684bda22657649521db0c87f76696bfbd93b4dfdec6c4851e99c26c7e37cc5a1a78e9b1a296850a067fe
3928b7c38367947756b7d506f431bfb7bbbac5d0 doc: improve secp256k1_fe_set_b32_mod doc (Coding Enthusiast)
Pull request description:
As discussed in #1453
This only changes the `secp256k1_fe_impl_set_b32_mod` comment since I think `secp256k1_fe_set_b32_limit` doc is clear enough.
ACKs for top commit:
sipa:
ACK 3928b7c38367947756b7d506f431bfb7bbbac5d0
theStack:
ACK 3928b7c38367947756b7d506f431bfb7bbbac5d0
Tree-SHA512: ad62c1b72d6a487473b182e6aadc7765711385add8c6576bf15c2015db82721f19e3d635f7a29316c2ee7e3c73bc55e2cd4f46ec13180be93d6fe8641f47e7d2
e02f313b1f251ccb363ae1ac24016d87c1be9009 Add comment on length checks when parsing ECDSA sigs (Tim Ruffing)
Pull request description:
I claim the check can be removed but I don't want to touch this
stable and well-tested code.
On the way, we fix grammar in another comment.
ACKs for top commit:
sipa:
ACK e02f313b1f251ccb363ae1ac24016d87c1be9009
RandyMcMillan:
ACK e02f313
Tree-SHA512: f82691a8f5db82a1e9683e52ce8e952ebd56b476a2817c5a876ce4638254b7b4ac93175318fb59598ed5532f33433951d75afea03724ef4419c3e1bd12ca8c20
0e5ea6220707d9c96e06efc43bce3d5a3b3a06f2 CONTRIBUTING: add some coding and style conventions (Jonas Nick)
1a432cb98220f29ac47639d30a6dbb3aa679a441 README: update first sentence (Jonas Nick)
0922a047fb2a225fd89802bbd6f2d0919cd50bca docs: move coverage report instructions to CONTRIBUTING (Jonas Nick)
76880e40151ddb18d0cd0549502d5ade95f501d6 Add CONTRIBUTING.md including scope and guidelines for new code (Jonas Nick)
Pull request description:
Following offline discussions, this PR documents the scope of the library and the requirements for adding new modules. I think this fixes most of #997. It also updates the README very slightly.
In addition, I added some coding conventions that I remembered explaining to new contributors in the past year. Even though it's far from exhaustive, I think this is an easy improvement to the CONTRIBUTING.md. Feel free to suggest more conventions.
ACKs for top commit:
sipa:
ACK 0e5ea6220707d9c96e06efc43bce3d5a3b3a06f2
real-or-random:
ACK 0e5ea6220707d9c96e06efc43bce3d5a3b3a06f2
Tree-SHA512: ffdbab22982fd632de92e81bd135f141ac86e24cc0dcfc0e1ae12b0d2a2e4f91377ab2c0cc440cb919889eaed8bfc1447b880fa1430fd771b956f2af0fe3766e
04af0ba162b152073455a5ccbb2c5833ae6d1d57 Replace ge_equals_ge[,j] calls with group.h equality calls (Pieter Wuille)
60525f6c14ad37719c6ea2deee19ec7b3654f061 Add unit tests for group.h equality functions (Pieter Wuille)
a47cd97d51e37c38ecf036d04e48518f6b0063f7 Add group.h ge/gej equality functions (Pieter Wuille)
Pull request description:
This pull requests removes the test-only functions `ge_equals_ge` and `ge_equals_gej`, and replaces them with proper group.h functions `secp256k1_ge_eq_var` and `secp256k1_gej_eq_ge_var` (mimicking the existing `secp256k1_gej_eq_var` function).
This drops some of the arbitrary and undocumented magnitude restristrictions these functions have, makes them properly tested on their own, and makes their semantics cleaner (I'm always left checking whether `ge_equals_ge` does a `CHECK` internally or whether it returns a value...).
ACKs for top commit:
real-or-random:
utACK 04af0ba162b152073455a5ccbb2c5833ae6d1d57
stratospher:
ACK 04af0ba.
Tree-SHA512: 49bc409ffa980144d1305c9389a846af45f0a97bfec19d016929056aa918c6a9f020dbe8549f5318fa8e6a4108621cc3cce60331aa0634f84619a1104d20a62a
