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.
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
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.
As suggested in issue #1381, this will make things simpler and
improve code readability, as we don't need to force omitting of
evaluations on a case-by-case basis anymore and hence can remove
lots of `#ifdef VERIFY`/`#endif` lines (see next commit). Plus,
VERIFY_CHECK behaves now identical in both non-VERIFY and coverage mode,
making the latter not special anymore and hopefully decreasing
maintenance burden. The idea of "side-effect safety" is given up.
Note that at two places in the ellswift module void-casts of return
values have to be inserted for non-VERIFY builds, in order to avoid
"variable ... set but not used [-Wunused-but-set-variable]"
warnings.
dcdda31f2cda13839a4285d8601118c041b18c13 Tighten secp256k1_fe_mul_inner's VERIFY_BITS checks (Russell O'Connor)
8e2a5fe908faa2ad0b847b3e5c42662614c8fa88 correct assertion for secp256k1_fe_mul_inner (roconnor-blockstream)
Pull request description:
Based on the surrounding asserts, 112 bits before this line, and 61 bits after this line, this assertion should be 113 bits. Notably the commensurate line in secp256k1_fe_sqr_inner is correctly assert to be 113 bits.
ACKs for top commit:
real-or-random:
ACK dcdda31f2cda13839a4285d8601118c041b18c13 tested with asm disabled
Tree-SHA512: c35170e37d9a6d1413dd625032028129ab2eccee7da86697ab9641b68ad78efd7251953d51e7acaefd14888d3fd61877f9f05349c44f6fc0133ce9b3921b0e1a
Widely available versions of GCC and Clang beat our field asm on -O2.
In particular, GCC 10.5.0, which is Bitcoin Core's current compiler
for official x86_64 builds, produces code that is > 20% faster for
fe_mul and > 10% faster for signature verification (see #726).
These are the alternatives to this PR:
We could replace our current asm with the fastest compiler output
that we can find. This is potentially faster, but it has multiple
drawbacks:
- It's more coding work because it needs detailed benchmarks (e.g.,
with many compiler/options).
- It's more review work because we need to deal with inline asm
(including clobbers etc.) and there's a lack of experts reviewers
in this area.
- It's not unlikely that we'll fall behind again in a few compiler
versions, and then we have to deal with this again, i.e., redo the
benchmarks. Given our history here, I doubt that we'll revolve
this timely.
We could change the default of the asm build option to off. But this
will also disable the scalar asm, which is still faster.
We could split the build option into two separate options for field
and scalar asm and only disable the field asm by default. But this
adds complexity to the build and to the test matrix.
My conclusion is that this PR gets the low-hanging fruit in terms of
performance. It simplifies our code significantly. It's clearly an
improvement, and it's very easy to review. Whether re-introducing
better asm (whether from a compiler or from CryptOpt) is worth the
hassle can be evaluated separately, and should not hold up this
improvement.
Solves #726.
33dc7e4d3e1947af4c84c09ecc75ea2eeed3f7e0 asm: add .note.GNU-stack section for non-exec stack (fanquake)
Pull request description:
With this in place, we no-longer see warnings like the following:
```bash
/usr/lib/gcc-cross/arm-linux-gnueabihf/12/../../../../arm-linux-gnueabihf/bin/ld: warning: field_10x26_arm.o: missing .note.GNU-stack section implies executable stack
/usr/lib/gcc-cross/arm-linux-gnueabihf/12/../../../../arm-linux-gnueabihf/bin/ld: NOTE: This behaviour is deprecated and will be removed in a future version of the linker
```
Should close#1434.
ACKs for top commit:
sipa:
utACK 33dc7e4d3e1947af4c84c09ecc75ea2eeed3f7e0
real-or-random:
utACK 33dc7e4d3e1947af4c84c09ecc75ea2eeed3f7e0
Tree-SHA512: f75ded8d971f54d1e871bcc4d815ba367b3e154eea2f18309ecaf9053e22f986bfffcf28418367f8055b65a5a0b245fee045adfcb63a2196df5e2f3aa6c97b89
With this in place, we no-longer see warnings like the following:
```bash
/usr/lib/gcc-cross/arm-linux-gnueabihf/12/../../../../arm-linux-gnueabihf/bin/ld: warning: field_10x26_arm.o: missing .note.GNU-stack section implies executable stack
/usr/lib/gcc-cross/arm-linux-gnueabihf/12/../../../../arm-linux-gnueabihf/bin/ld: NOTE: This behaviour is deprecated and will be removed in a future version of the linker
```
Should close#1434.
Based on the surrounding asserts, 112 bits before this line, and 61 bits after this line, this assertion should be 113 bits. Notably the commensurate line in secp256k1_fe_sqr_inner is correctly assert to be 113 bits.
c45b7c4fbbf41b011f138c465a58322a36664fd3 refactor: introduce testutil.h (deduplicate `random_fe_`, `ge_equals_` helpers) (Sebastian Falbesoner)
dc5514144fb9d412aa3845432b053ee06a27da37 tests: simplify `random_fe_non_zero` (remove loop limit and unneeded normalize) (Sebastian Falbesoner)
Pull request description:
`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 (which was already suggested in https://github.com/bitcoin-core/secp256k1/pull/1118#discussion_r1067259954); 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.
ACKs for top commit:
real-or-random:
utACK c45b7c4fbbf41b011f138c465a58322a36664fd3
siv2r:
ACK `c45b7c4` (reviewed the changes and tests for both the commits passed locally).
Tree-SHA512: 4ffa66dd0b8392d7d0083a71e7b0682ad18f9261fd4ce8548c3059b497d3462db97e16114fded9787661ca447a877a27f5b996bd7d47e6f91c4454079d28a8ac
This commit also explicitly initializes shortpubkey. For some reason, removing
surrounding, unrelated lines results in gcc warnings when configured with
--enable-ctime-tests=no --with-valgrind=no.
d23da6d55714271c720fee58fbff5e5ef2fe193f use secp256k1_scalar_verify checks (stratospher)
c7d0454932b42a9728b55033c94e000b1dbbb6f2 add verification for scalars (stratospher)
ad152151b06a40aaf6cd90561356ff451996455d update max scalar in scalar_cmov_test and fix schnorrsig_verify exhaustive test (stratospher)
Pull request description:
From #1360. This PR:
1. adds `secp256k1_scalar_verify` to make sure scalars are reduced mod the group order in VERIFY mode
2. uses `secp256k1_scalar_verify` in all the scalar functions except `secp256k1_scalar_clear`, `secp256k1_scalar_reduce_512`, `secp256k1_scalar_mul_512` and `secp256k1_scalar_*_var` functions in `scalar_low_impl.h`
ACKs for top commit:
real-or-random:
utACK d23da6d55714271c720fee58fbff5e5ef2fe193f
theStack:
Code-review ACK d23da6d55714271c720fee58fbff5e5ef2fe193f
Tree-SHA512: a371b319d948198c4038d35c9ea58f4b94de4dc312215e2b78a323c2acd4ae1355d97935c558b388774832d6d0058b97ff8ca50c3aab40b9ede5307760d0a505
`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.
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>
- `secp256k1_scalar_set_int` in scalar_low uses input mod EXHAUSTIVE_TEST_ORDER
- directly store s in sig64 without reducing it mod the group order for testing
