`random_fe_non_zero` contains a loop iteration limit that ensures that
we abort if `random_fe` ever yielded zero more than ten times in a row.
This construct was first introduced in PR #19 (commit 09ca4f32) for
random non-square field elements and was later refactored into the
non-zero helper in PR #25 (commit 6d6102fe). The copy-over to the
exhaustive tests happened recently in PR #1118 (commit 0f864207).
This case seems to be practically irrelevant and I'd argue for keeping
things simple and removing it; if there's really a worry that the test's
random generator is heavily biased towards certain values or value
ranges then there should consequently be checks at other places too
(e.g. directly in `random_fe` for 256-bit values that repeatedly
overflow, i.e. >= p).
Also, the _fe_normalize call is not needed and can be removed, as the
result of `random_fe` is already normalized.
d78bec7001fe6f5ed8d5b215bf61e7b74e3369ca ci: Remove Windows MSVC tasks from Cirrus CI (Hennadii Stepanov)
3545dc2b9bdbf856c1e0288120ef1cde99daa7ec ci, gha: Run all MSVC tests on Windows natively (Hennadii Stepanov)
Pull request description:
ACKs for top commit:
real-or-random:
ACK d78bec7001fe6f5ed8d5b215bf61e7b74e3369ca
jonasnick:
ACK d78bec7001fe6f5ed8d5b215bf61e7b74e3369ca
Tree-SHA512: b58162a9f0827dceb1c7eb6fb7c759c0bffcf3e0d24cc7e6628ad71d1faaabaffb9d8de6fcd3d07bfcaca409632a13f711f9ad871a30718139557544cf91b4bf
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
175db31149fff4b3dc3d3dab021f289d7e98381c ci: Drop no longer needed `PATH` variable update on Windows (Hennadii Stepanov)
116d2ab3df630455f23a7b21f50237689879ecc0 cmake: Set `ENVIRONMENT` property for examples on Windows (Hennadii Stepanov)
cef373997c29c5e6077b9367c92812bcc99bc8bf cmake, refactor: Use helper function instead of interface library (Hennadii Stepanov)
Pull request description:
This PR simplifies running examples on Windows, because the DLL must reside either in the same folder where the executable is or somewhere in PATH.
It is an alternative to #1233.
ACKs for top commit:
real-or-random:
utACK 175db31149fff4b3dc3d3dab021f289d7e98381c
Tree-SHA512: 8188018589a5bcf0179647a039cdafcce661dc103a70a5bb9e6b6f680b899332ba30b1e9ef5dad2a8c22c315d7794747e49d8cf2e391eebea21e3d8505ee334b
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>
b097a466c168dcdb3fde435ec4a1e0b63609f55d util: remove unused checked_realloc (Cory Fields)
Pull request description:
Usage was removed in 6fe50439 . This should be a NOOP.
Noticed when analyzing for zenbleed exposure: stdlib calls that aren't optimized away.
In this case realloc isn't making it into the final binary, but as far as I can tell this is completely dead code and should be dropped.
ACKs for top commit:
jonasnick:
ACK b097a466c168dcdb3fde435ec4a1e0b63609f55d
real-or-random:
ACK b097a466c168dcdb3fde435ec4a1e0b63609f55d
Tree-SHA512: d4249215eddd4035be2b50a8bb48b8a681abdab4ab41ca53f6c2a2507edfbc9ffa39ba22eb48e7da52f978e224198294495ce64f9d571d98c19283b20b82a63a
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>
c424e2fb43c8ed959b2af7b2216028ce2a023488 ellswift: fix probabilistic test failure when swapping sides (Jonas Nick)
Pull request description:
Reported by jonatack in https://github.com/bitcoin/bitcoin/issues/28079.
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 assumption that the secret keys differ.
ACKs for top commit:
sipa:
utACK c424e2fb43c8ed959b2af7b2216028ce2a023488
real-or-random:
utACK c424e2fb43c8ed959b2af7b2216028ce2a023488
Tree-SHA512: d1ab61473a77478f9aeffb21ad73e0bba478c90d8573c72ec89d2e0140434cc65c9d5f4d56e5f259931dc68fc1800695c6cd5d63d9cfce4c1c4d6744eeaa2028
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.
This change simplifies running examples on Windows, because the DLL
must reside either in the same folder where the executable is or
somewhere in PATH.
