747ada35877d4392c453b7c7249465fb382125ea test: Silent noisy clang warnings about Valgrind code on macOS x86_64 (Hennadii Stepanov)
Pull request description:
Since #1206, on macOS x86_64 with Valgrind installed, clang emits a massive amount of `-Wreserved-identifier` and `-Wreserved-macro-identifier` warnings from the `valgrind/valgrind.h` and `valgrind/memcheck.h` headers.
This PR prevents warnings emitted for the Valgrind code.
ACKs for top commit:
real-or-random:
utACK 747ada35877d4392c453b7c7249465fb382125ea
Tree-SHA512: dd1b2b9db2d471939fdc30f9d8fd106a12f21ec5008ca98d8ebe3087d7ea352d564e8bbd0cec59a004e084af3a84d4680cb81f2ef6fe13cf164b7691e33f437d
It is not neccessary for the second argument in `secp256k1_fe_equal_var`
(or `secp256k1_fe_equal`) to have magnitude = 1.
Hence, removed the `secp256k1_fe_normalize_weak` call for those argument.
b7c685e74adbd83937990e90f076600fabf8ccf0 Save _normalize_weak calls in group add methods (Peter Dettman)
c83afa66e0c324e42d13adff0e4f7db9b2868788 Tighten group magnitude limits (Peter Dettman)
173e8d061a8d1526f80d9ae79dd7f0371d38f7e0 Implement current magnitude assumptions (Peter Dettman)
49afd2f5d8c323d32a21f2fe182823b6d7704eb2 Take use of _fe_verify_magnitude in field_impl.h (Sebastian Falbesoner)
4e9661fc426c6068b2472f52a772c312bc26acc9 Add _fe_verify_magnitude (no-op unless VERIFY is enabled) (Peter Dettman)
690b0fc05abd76cb7f6bd87e88bf7b8b0fd1ab70 add missing group element invariant checks (Sebastian Falbesoner)
Pull request description:
This PR picks up #1032 by peterdettman. It's essentially a rebase on master; the original first commit (09dbba561fdb9d57a2cc9842ce041d9ba29a6189) which introduced group verification methods has mostly been replaced by PR #1299 (commit f20266722ac93ca66d1beb0d2f2d2469b95aafea) and what remains now is only adding a few missing checks at some places. The remaining commits are unchanged, though some (easy-to-solve) conflicts appeared through cherry-picking. The last commit which actually removes the `normalize_weak` calls is obviously the critical one and needs the most attention for review.
ACKs for top commit:
sipa:
utACK b7c685e74adbd83937990e90f076600fabf8ccf0
real-or-random:
ACK b7c685e74adbd83937990e90f076600fabf8ccf0
jonasnick:
ACK b7c685e74adbd83937990e90f076600fabf8ccf0
Tree-SHA512: f15167eff7ef6ed971c726a4d738de9a15be95b0c947d7e38329e7b16656202b7113497d36625304e784866349f2293f6f1d8cb97df35393af9ea465a4156da3
Also update the operations count comments in each of the affected
functions accordingly and remove a redundant VERIFY_CHECK in
secp256k1_gej_add_ge (the infinity value range check [0,1] is already
covered by secp256k1_gej_verify above).
Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
Co-authored-by: Tim Ruffing <crypto@timruffing.de>
Co-authored-by: Jonas Nick <jonasd.nick@gmail.com>
- adjust test methods that randomize magnitudes
Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
Co-authored-by: Jonas Nick <jonasd.nick@gmail.com>
Remove also the explicit magnitude restriction `a->x.magnitude <= 31`
in `secp256k1_gej_eq_x_var` (introduced in commit
07c0e8b82e2cea87f85263512945fed7adffea18), as this is implied by the
new limits.
Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
The group element checks `secp256k1_{ge,gej}_verify` have first been
implemented and added in commit f20266722ac93ca66d1beb0d2f2d2469b95aafea
(PR #1299). This commit adds additional verification calls in group
functions, to match the ones that were originally proposed in commit
09dbba561fdb9d57a2cc9842ce041d9ba29a6189 of WIP-PR #1032 (which is
obviously not rebased on #1299 yet).
Also, for easier review, all functions handling group elements are
structured in the following wasy for easier review (idea suggested by
Tim Ruffing):
- on entry, verify all input ge, gej (and fe)
- empty line
- actual function body
- empty line
- on exit, verify all output ge, gej
Co-authored-by: Peter Dettman <peter.dettman@gmail.com>
Co-authored-by: Tim Ruffing <crypto@timruffing.de>
When configured with `--disable-module-ecdh --enable-module-recovery`, then
`./tests 64 81af32fd7ab8c9cbc2e62a689f642106` fails with
```
src/modules/ellswift/tests_impl.h:396: test condition failed: secp256k1_memcmp_var(share32_bad, share32a, 32) != 0
```
This tests verifies that changing the `party` bit of the
`secp256k1_ellswift_xdh` function results in a different share. However, that's
not the case when the secret keys of both parties are the same and this is
actually what happens in the observed test failure. The keys can be equal in
this test case because they are created by the `random_scalar_order_test`
function whose output is not uniformly random (it's biased towards 0).
This commit restores the assummption that the secret keys differ.
b79ba8aa4c074b2cd09188f6f85ba68d6b80fe50 field: Use `restrict` consistently in fe_sqrt (Tim Ruffing)
Pull request description:
That is, use it also in the definition and not only the declaration.
I believe this was the intention of commit
be82bd8e03, but it was omitted there.
edit: Changed the description. I'm not entirely sure but after looking at the standard, I tend to think this is more than a cosmetic change, and only this change actually makes the parameters `restrict`. Anyway, I believe making them `restrict` was simply forgotten in be82bd8e0347e090037ff1d30a22a9d614db8c9f.
ACKs for top commit:
sipa:
utACK b79ba8aa4c074b2cd09188f6f85ba68d6b80fe50
Tree-SHA512: eecec7674d8cef7833d50f4041b87241ca8de4839aa8027df1c422b89f5a1bcef3916ac785057a596c459ce1aa9d41e5a21ecb6fed9c5d15a1d9f588c7ee208e
600c5adcd59240305e22918943f45dceeabb7e93 clean up in-comment Sage code (refer to secp256k1_params.sage, update to Python3) (Sebastian Falbesoner)
Pull request description:
Some of the C source files contain contain in-comment Sage code calculating secp256k1 parameters that are already defined in the file secp256k1_params.sage. Replace that by a corresponding load instruction and access the necessary variables. In ecdsa_impl.h, update the comment to use a one-line shell command calling sage to get the values.
The remaining code (test `test_add_neg_y_diff_x` in tests.c) is updated to work with a current version based on Python3 (Sage 9.0+, see https://wiki.sagemath.org/Python3-Switch).
The latter can be seen as a small follow-up to PR #849 (commit 13c88efed0005eb6745a222963ee74564054eafb).
ACKs for top commit:
sipa:
ACK 600c5adcd59240305e22918943f45dceeabb7e93
real-or-random:
ACK 600c5adcd59240305e22918943f45dceeabb7e93
Tree-SHA512: a9e52f6afbce65edd9ab14203612c3d423639f450fe8f0d269a3dda04bebefa95b607f7aa0faec864cb78b46d49f281632bb1277118749b7d8613e9f5dcc8f3d
Some of the C source files contain contain in-comment Sage code
calculating secp256k1 parameters that are already defined in the file
secp256k1_params.sage. Replace that by a corresponding load instruction
and access the necessary variables. In ecdsa_impl.h, update the comment
to use a one-line shell command calling sage to get the values.
The remaining code (test `test_add_neg_y_diff_x` in tests.c) is updated
to work with a current version based on Python3 (Sage 9.0+, see
https://wiki.sagemath.org/Python3-Switch).
The latter can be seen as a small follow-up to PR #849 (commit
13c88efed0005eb6745a222963ee74564054eafb).
That is, use it also in the definition and not only the declaration.
I believe this was the intention of commit
be82bd8e0347e090037ff1d30a22a9d614db8c9f, but it was omitted there.
07c0e8b82e2cea87f85263512945fed7adffea18 group: remove unneeded normalize_weak in `secp256k1_gej_eq_x_var` (Sebastian Falbesoner)
efa76c4bf7cab1c22aa476cd2730e891450ad4a0 group: remove unneeded normalize_weak in `secp256k1_ge_is_valid_var` (Sebastian Falbesoner)
Pull request description:
This PR removes unneeded normalize_weak calls in two group element functions:
* `secp256k1_ge_is_valid_var`: After calculating the right-hand side of the elliptic curve equation (x^3 + 7), the field element `x3` has a magnitude of 2 (1 as result of `secp256k1_fe_mul`, then increased by 1 due to `secp256k1_fe_add_int`). This is fine for `secp256k1_fe_equal_var`, as the second parameter only requires the magnitude to not exceed 31, and the normalize_weak call is hence not needed and can be dropped. Note that the interface description for `secp256k1_fe_equal` (which also applies to `secp256k1_fe_equal_var`) once stated that _both_ parameters need to have magnitude 1, but that was corrected in commit 7d7d43c6dd2741853de4631881d77ae38a14cd23.
* `secp256k1_gej_eq_x_var`: By requiring that the input group element's X coordinate (`a->x`) has a magnitude of <= 31, the normalize_weak call and also the field element variable `r2` are not needed anymore and hence can be dropped.
ACKs for top commit:
sipa:
utACK 07c0e8b82e2cea87f85263512945fed7adffea18
jonasnick:
ACK 07c0e8b82e2cea87f85263512945fed7adffea18
Tree-SHA512: 9037e4af881ce7bf3347414d6da06b99e3d318733ba4f70e8b24d2320c2f26d022144e17bd6b95c1a4ef1be3825a4464e56ce2d2b3ae7bbced04257048832b7f
c6cd2b15a007ad0a2d5c4656ae641ba442d8b2fe ci: Add task for static library on Windows + CMake (Hennadii Stepanov)
020bf69a44ba700624d09de0c18ceb867369d24e build: Add extensive docs on visibility issues (Tim Ruffing)
0196e8ade16e2b2d8efadac01d8520205553ee39 build: Introduce `SECP256k1_DLL_EXPORT` macro (Hennadii Stepanov)
9f1b1904a358e4ce7248c6542e8c7ac143ba0e3f refactor: Replace `SECP256K1_API_VAR` with `SECP256K1_API` (Hennadii Stepanov)
ae9db95ceaa2605138fac9c237c640acea3f3bd6 build: Introduce `SECP256K1_STATIC` macro for Windows users (Hennadii Stepanov)
Pull request description:
Previous attempts:
- https://github.com/bitcoin-core/secp256k1/pull/1346
- https://github.com/bitcoin-core/secp256k1/pull/1362
The result is as follows:
1. Simple, concise and extensively documented code.
2. Explicitly documented use cases with no ambiguities.
3. No workarounds for linker warnings.
4. Solves one item in https://github.com/bitcoin-core/secp256k1/issues/1235.
ACKs for top commit:
real-or-random:
utACK c6cd2b15a007ad0a2d5c4656ae641ba442d8b2fe
Tree-SHA512: d58694452d630aefbd047916033249891bc726b7475433aaaa7c3ea2a07ded8f185a598385b67c2ee3440ec5904ff9d9452c97b0961d84dcb2eb2cf46caa171e
b6b9834e8da7f3fd91b95f750a4ee7a10bf67435 small fixes (Alejandro)
Pull request description:
Corrected some typos
ACKs for top commit:
real-or-random:
ACK b6b9834e8da7f3fd91b95f750a4ee7a10bf67435
Tree-SHA512: c40c22c66f1067ecca351f08cca07a78b00bb98af2f6cfb08c25d0b1db6845e0e32ace1954c386db7020cf9fc7ae973ff15bd6d9c0144f3d21ea28c15741050f
By requiring that the input group element's X coordinate (`a->x`) has a
magnitude of <= 31, the normalize_weak call and also the field element
variable `r2` are not needed anymore and hence can be dropped.
It is a non-Libtool-specific way to explicitly specify the user's
intention to consume a static `libseck256k1`.
This change allows to get rid of MSVC linker warnings LNK4217 and
LNK4286. Also, it makes possible to merge the `SECP256K1_API` and
`SECP256K1_API_VAR` into one.
There are several instances in the tests where random non-zero field
elements are generated by calling `random_fe_test` in a do/while-loop.
This commit deduplicates all these by introducing a
`random_fe_non_zero_test` helper. Note that some instances checked the
is-zero condition via `secp256k1_fe_normalizes_to_zero_var`, which is
unnecessary, as the result of `random_fe_test` is already normalized (so
strictly speaking, this is not a pure refactor).
There is a function `random_fe_test` which does exactly the
same, so use that instead. Note that it's also moved up before the
`random_group_element_test` function, in order to avoid needing a forward
declaration.
be8ff3a02aeff87c60d49883a1b2afa8b2999bbe field: Static-assert that int args affecting magnitude are constant (Tim Ruffing)
Pull request description:
See #1001.
Try to revert the lines in `tests.c` to see the error message in action.
ACKs for top commit:
sipa:
ACK be8ff3a02aeff87c60d49883a1b2afa8b2999bbe. Verified by introducing some non-constant expressions and seeing compilation fail.
theStack:
ACK be8ff3a02aeff87c60d49883a1b2afa8b2999bbe
Tree-SHA512: 8befec6ee64959cdc7f3e29b4b622410794cfaf69e9df8df17600390a93bc787dba5cf86239de6eb2e99c038b9aca5461e4b3c82f0e0c4cf066ad7c689941b19
7d8d5c86df8b27b45e80ed50341dd0ce64546c0f tests: refactor: take use of `secp256k1_ge_x_on_curve_var` (Sebastian Falbesoner)
Pull request description:
The recently merged ellswift PR (#1129) introduced a helper `secp256k1_ge_x_on_curve_var` to check if a given X coordinate is on the curve (i.e. the expression x^3 + 7 is square, see commit 79e5b2a8b80f507e2c9936ff1c4e2fb39bc66a4e). This can be used for code deduplication in the `ecmult_const_mult_xonly` test.
(Found this instance via `$ git grep add_int.*SECP256K1_B`, I think it's the only one where the helper can be used.)
ACKs for top commit:
sipa:
utACK 7d8d5c86df8b27b45e80ed50341dd0ce64546c0f
real-or-random:
utACK 7d8d5c86df8b27b45e80ed50341dd0ce64546c0f
Tree-SHA512: aebff9b5ef2f6f6664ce89e4e1272cb55b6aac81cfb379652c4b7ab30dd1d7fd82a2c3b47c7b7429755ba28f011a3a9e2e6d3aa5c77d3b105d159104c24b89f3
The recently merged ellswift PR (#1129) introduced a helper
`secp256k1_ge_x_on_curve_var` to check if a given X coordinate is
valid (i.e. the expression x^3 + 7 is square, see commit
79e5b2a8b80f507e2c9936ff1c4e2fb39bc66a4e). This can be used for code
deduplication in the `ecmult_const_mult_xonly` test.
5b7bf2e9d4ee02cbec1105ad6e890c34a4da1beb Use `__shiftright128` intrinsic in `secp256k1_u128_rshift` on MSVC (Hennadii Stepanov)
Pull request description:
Closes https://github.com/bitcoin-core/secp256k1/issues/1324.
As the `__shiftright128` [docs](https://learn.microsoft.com/en-us/cpp/intrinsics/shiftright128) state:
> The `Shift` value is always modulo 64...
it is not applicable for the `n >= 64` branch.
ACKs for top commit:
sipa:
utACK 5b7bf2e9d4ee02cbec1105ad6e890c34a4da1beb
real-or-random:
ACK 5b7bf2e9d4ee02cbec1105ad6e890c34a4da1beb tested with MSVC x64
Tree-SHA512: bc4c245a9da83c783a0479e751a4bc2ec77a34b99189fcc4431033a5420c93b610f3b960d3f23c15bce2eb010beba665b3e84d468b3fdab3d5846d4f27016898
e449af6872445d33a0796224fcb733be6476ad36 Drop no longer needed `#include "../include/secp256k1.h"` (Hennadii Stepanov)
Pull request description:
The removed header includes have not been needed since https://github.com/bitcoin-core/secp256k1/pull/1231.
Test suggestions:
1. Using Autottols-based build system:
```
./autogen.sh
./configure
make clean-precomp
make
```
2. Using CMake-based build system:
```
cmake -B build -DCMAKE_C_INCLUDE_WHAT_YOU_USE="include-what-you-use"
cmake --build build --target secp256k1_precomputed
```
ACKs for top commit:
sipa:
utACK e449af6872445d33a0796224fcb733be6476ad36
real-or-random:
utACK e449af6872445d33a0796224fcb733be6476ad36
Tree-SHA512: 5aed7a88e1e03fcc2306c43817712c0652ecf6145679dd17f4719376818d372f619e4180bdaee548f2e82aaccbe6a2ff4c37203121d939af545128c8c48b933e
The scheme implemented is described below, and largely follows the paper
"SwiftEC: Shallue–van de Woestijne Indifferentiable Function To Elliptic Curves",
by Chavez-Saab, Rodriguez-Henriquez, and Tibouchi
(https://eprint.iacr.org/2022/759).
A new 64-byte public key format is introduced, with the property that *every*
64-byte array is an encoding for a non-infinite curve point. Each curve point
has roughly 2^256 distinct encodings. This permits disguising public keys as
uniformly random bytes.
The new API functions:
* secp256k1_ellswift_encode: convert a normal public key to an ellswift 64-byte
public key, using additional entropy to pick among the many possible
encodings.
* secp256k1_ellswift_decode: convert an ellswift 64-byte public key to a normal
public key.
* secp256k1_ellswift_create: a faster and safer equivalent to calling
secp256k1_ec_pubkey_create + secp256k1_ellswift_encode.
* secp256k1_ellswift_xdh: x-only ECDH directly on ellswift 64-byte public keys,
where the key encodings are fed to the hash function.
The scheme itself is documented in secp256k1_ellswift.h.
f1652528be5a287a3c33a4fae1e5763693333c2b Normalize ge produced from secp256k1_pubkey_load (stratospher)
Pull request description:
The output `ge` in secp256k1_pubkey_load is normalized when `sizeof(secp256k1_ge_storage) = 64` but not when it's not 64. ARG_CHECK at the end of the function assumes normalization. So normalize ge in the other code path too.
context: [#1129(comment)](https://github.com/bitcoin-core/secp256k1/pull/1129/files#r1196167066)
ACKs for top commit:
sipa:
utACK f1652528be5a287a3c33a4fae1e5763693333c2b
real-or-random:
ACK f1652528be5a287a3c33a4fae1e5763693333c2b tested by changing the two `== 64` checks to `== 65`
Tree-SHA512: 0de1caad85ccdb42053f8e09576135257c88fda88455ef25e7640049c05a1e03d1e9bae1cd132d2e6fc327fd79929257a8b21fe1cc41c82374b6cd88e6744aa3
