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
da7bc1b803 include: in doc, remove article in front of "pointer" (Jonas Nick)
aa3dd5280b include: make doc about ctx more consistent (Jonas Nick)
e3f690015a include: remove obvious "cannot be NULL" doc (Jonas Nick)
Pull request description:
ACKs for top commit:
sipa:
ACK da7bc1b803
real-or-random:
ACK da7bc1b803
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.
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
5437e7bdfb.
74a4d974d5 doc: Add ABI checking with `check-abi.sh` to the Release Process (Hennadii Stepanov)
e7f830e32c Add `tools/check-abi.sh` (Hennadii Stepanov)
Pull request description:
ACKs for top commit:
real-or-random:
ACK 74a4d974d5 it compares the right commits now
jonasnick:
re-Concept ACK 74a4d974d5
Tree-SHA512: bcca5246837f899d43ced3b0099a8e123f4fd2db7d15684bda22657649521db0c87f76696bfbd93b4dfdec6c4851e99c26c7e37cc5a1a78e9b1a296850a067fe
3928b7c383 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 3928b7c383
theStack:
ACK 3928b7c383
Tree-SHA512: ad62c1b72d6a487473b182e6aadc7765711385add8c6576bf15c2015db82721f19e3d635f7a29316c2ee7e3c73bc55e2cd4f46ec13180be93d6fe8641f47e7d2
e02f313b1f 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 e02f313b1f
RandyMcMillan:
ACK e02f313
Tree-SHA512: f82691a8f5db82a1e9683e52ce8e952ebd56b476a2817c5a876ce4638254b7b4ac93175318fb59598ed5532f33433951d75afea03724ef4419c3e1bd12ca8c20
0e5ea62207 CONTRIBUTING: add some coding and style conventions (Jonas Nick)
1a432cb982 README: update first sentence (Jonas Nick)
0922a047fb docs: move coverage report instructions to CONTRIBUTING (Jonas Nick)
76880e4015 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 0e5ea62207
real-or-random:
ACK 0e5ea62207
Tree-SHA512: ffdbab22982fd632de92e81bd135f141ac86e24cc0dcfc0e1ae12b0d2a2e4f91377ab2c0cc440cb919889eaed8bfc1447b880fa1430fd771b956f2af0fe3766e
04af0ba162 Replace ge_equals_ge[,j] calls with group.h equality calls (Pieter Wuille)
60525f6c14 Add unit tests for group.h equality functions (Pieter Wuille)
a47cd97d51 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 04af0ba162
stratospher:
ACK 04af0ba.
Tree-SHA512: 49bc409ffa980144d1305c9389a846af45f0a97bfec19d016929056aa918c6a9f020dbe8549f5318fa8e6a4108621cc3cce60331aa0634f84619a1104d20a62a
bb4672342e remove VERIFY_SETUP define (Sebastian Falbesoner)
a3a3e11acd remove unneeded VERIFY_SETUP uses in ECMULT_CONST_TABLE_GET_GE macro (Sebastian Falbesoner)
a0fb68a2e7 introduce and use SECP256K1_SCALAR_VERIFY macro (Sebastian Falbesoner)
cf25c86d05 introduce and use SECP256K1_{FE,GE,GEJ}_VERIFY macros (Sebastian Falbesoner)
5d89bc031b remove superfluous `#ifdef VERIFY`/`#endif` preprocessor conditions (Sebastian Falbesoner)
c2688f8de9 redefine VERIFY_CHECK to empty in production (non-VERIFY) mode (Sebastian Falbesoner)
Pull request description:
As suggested in #1381, this PR reworks the policy for VERIFY_CHECK and when to use #ifdef VERIFY, by:
- redefining VERIFY_CHECK to empty in production (non-VERIFY) mode
- removing many then superflous #ifdef VERIFY blocks (if they exclusively contained VERIFY_CHECKs)
- introducing uppercase macros around verify_ functions and using them for better readabiliy
What is _not_ included yet is the proposed renaming from "_check" to "_assert":
> And while we're touching this anyway, we could consider renaming "check" to "assert", which is a more precise term. (In fact, if we redefine VERIFY_CHECK to be empty in production, we have almost reimplemented assert.h...)
This should be easy to achieve with simple search-and-replace (e.g. using sed), but I was hesitant as this would probably case annoying merge conflicts on some of the open PRs. Happy to add this if the rename if desired (#1381 didn't get any feedback about the renaming idea yet).
ACKs for top commit:
stratospher:
ACK bb46723.
real-or-random:
utACK bb4672342e
Tree-SHA512: 226ca609926dea638aa3bb537d29d4fac8b8302dcd9da35acf767ba9573e5221d2dae04ea26c15d80a50ed70af1ab0dca10642c21df7dbdda432fa237a5ef2cc
This define was seemingly introduced for VERIFY mode code with side
effects (for setup purposes), that should just be executed without any
checks. The same can be achieved by putting it in an `#if VERIFY` block,
so we can remove it.
By providing an uppercase variant of these verification functions, it is
better visible that it is test code and surrounding `#ifdef VERIFY`
blocks can be removed (if there is no other code around that could
remain in production mode), as they don't serve their purpose any more.
At some places intentional blank lines are inserted for grouping and
better readadbility.
Now that the `VERIFY_CHECK` compiles to empty in non-VERIFY mode, blocks
that only consist of these macros don't need surrounding `#ifdef VERIFY`
conditions anymore.
At some places intentional blank lines are inserted for grouping and
better readadbility.
