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
1ddd76af0a735b7fcbec7f37c0d99a7db9893ac1 bench: add --help option to bench_internal (Sebastian Falbesoner)
Pull request description:
While coming up with commands for running the benchmarks for issue https://github.com/bitcoin-core/secp256k1/issues/726#issuecomment-1824625653, I noticed that in contrast to `bench{_ecmult}`, `bench_internal` doesn't have a help option yet and figured it would be nice to have one. A comparable past PR is https://github.com/bitcoin-core/secp256k1/pull/1008. Benchmark categories appear in the same order as they are executed, the concrete benchmark names in parantheses per category are listed in alphabetical order.
ACKs for top commit:
real-or-random:
utACK 1ddd76af0a735b7fcbec7f37c0d99a7db9893ac1
siv2r:
ACK 1ddd76a, tested the `--help` option locally, and it works as expected.
Tree-SHA512: d117641a5f25a7cbf83881f3acceae99624528a0cbb2405efdbe1a3a2762b4d6b251392e954aaa32f6771069d31143743770fccafe198084c12258dedb0856fc
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
10271356c8fc34395850ac70df5902571945fbea Return temporaries to being unsigned in secp256k1_fe_sqr_inner (roconnor-blockstream)
Pull request description:
These temporaries seem to been inadvertently changed to signed during a refactoring. Generally, bit shifting is frowned upon for signed values.
ACKs for top commit:
sipa:
utACK 10271356c8fc34395850ac70df5902571945fbea
real-or-random:
utACK 10271356c8fc34395850ac70df5902571945fbea
Tree-SHA512: a9fefe4b146163209662cd435422beb3c9561eb9e83110454184f70df2292992f39ec1971143428e039a80cad2f6285db74de2f059e877ad8756ff739269b67a
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.
8185e72d299bc77de9c06cc92fd1988676df3bc1 ci: Ignore internal errors in snapshot compilers (Hennadii Stepanov)
Pull request description:
It was discussed on today's IRC meeting.
ACKs for top commit:
real-or-random:
ACK 8185e72d299bc77de9c06cc92fd1988676df3bc1
Tree-SHA512: 0f41ca8303bd3d6efefcd3a544c7bd7dfcf464c57c779c876da4a77cacd262e6c963449d493fdf5a641b0d10b655c8c67fe8a147145b6533328d7bf5344313e1
355bbdf38a2f932daadd02325a0d90d902cb2af4 Add changelog entry for signed-digit ecmult_const algorithm (Pieter Wuille)
21f49d9bec518a769029f809817444a984e735ab Remove unused secp256k1_scalar_shr_int (Pieter Wuille)
115fdc7232a80872c99f88589a5a3608ba757f1d Remove unused secp256k1_wnaf_const (Pieter Wuille)
aa9f3a3c004469033709dc8138892e66adf0b030 ecmult_const: add/improve tests (Jonas Nick)
4d16e90111c050de3b7e25ac451d87cd4e3f874e Signed-digit based ecmult_const algorithm (Pieter Wuille)
ba523be067d6e45957d154838cb9da942704f01a make SECP256K1_SCALAR_CONST reduce modulo exhaustive group order (Pieter Wuille)
2140da9cd5d490d8462d5c7cc909755edc10c1e6 Add secp256k1_scalar_half for halving scalars (+ tests/benchmarks). (Pieter Wuille)
Pull request description:
Using some insights learned from #1058, this replaces the fixed-wnaf ecmult_const algorithm with a signed-digit based one. Conceptually both algorithms are very similar, in that they boil down to summing precomputed odd multiples of the input points. Practically however, the new algorithm is simpler because it's just using scalar operations, rather than relying on wnaf machinery with skew terms to guarantee odd multipliers.
The idea is that we can compute $q \cdot A$ as follows:
* Let $s = f(q)$, for some function $f()$.
* Compute $(s_1, s_2)$ such that $s = s_1 + \lambda s_2$, using `secp256k1_scalar_lambda_split`.
* Let $v_1 = s_1 + 2^{128}$ and $v_2 = s_2 + 2^{128}$ (such that the $v_i$ are positive and $n$ bits long).
* Computing the result as $$\sum_{i=0}^{n-1} (2v_1[i]-1) 2^i A + \sum_{i=0}^{n-1} (2v_2[i]-1) 2^i \lambda A$$ where $x[i]$ stands for the *i*'th bit of $x$, so summing positive and negative powers of two times $A$, based on the bits of $v_1.$
The comments in `ecmult_const_impl.h` show that if $f(q) = (q + (1+\lambda)(2^n - 2^{129} - 1))/2 \mod n$, the result will equal $q \cdot A$.
This last step can be performed in groups of multiple bits at once, by looking up entries in a precomputed table of odd multiples of $A$ and $\lambda A$, and then multiplying by a power of two before proceeding to the next group.
The result is slightly faster (I measure ~2% speedup), but significantly simpler as it only uses scalar arithmetic to determine the table lookup values. The speedup is due to the fact that no skew corrections at the end are needed, and less overhead to determine table indices. The precomputed table sizes are also made independent from the `ecmult` ones, after observing that the optimal table size is bigger here (which also gives a small speedup).
ACKs for top commit:
jonasnick:
ACK 355bbdf38a2f932daadd02325a0d90d902cb2af4
siv2r:
ACK 355bbdf
real-or-random:
ACK 355bbdf38a2f932daadd02325a0d90d902cb2af4
Tree-SHA512: 13db572cb7f9be00bf0931c65fcd8bc8b5545be86a8c8700bd6a79ad9e4d9e5e79e7f763f92ca6a91d9717a355f8162204b0ea821b6ae99d58cb400497ddc656
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.
fa4d6c76b6dc249d02c926ad75725556614fdddd ci/cirrus: Add native ARM64 persistent workers (MarcoFalke)
2262d0eaab2aa1181f900acd2d8cd568047599b6 ci/cirrus: Bring back skeleton .cirrus.yml without jobs (Tim Ruffing)
Pull request description:
ACKs for top commit:
real-or-random:
ACK fa4d6c76b6dc249d02c926ad75725556614fdddd
hebasto:
re-ACK fa4d6c76b6dc249d02c926ad75725556614fdddd, only last two commits have been squashed since my recent [review](https://github.com/bitcoin-core/secp256k1/pull/1426#pullrequestreview-1636119941).
Tree-SHA512: d1fee99d54a41a4126f7eb72695a56137c925dc9ce7cd692a60ea1262ac0789bbd6aa4e4dfc030f0d97d06aeeae0724a5f2d794a85ff533c6cf3cd215f6a4b7a
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
421d84855aeb9324b4f131c3e6c7ee8cf4f5f4e5 ci: Align Autotools/CMake `CI_INSTALL` directory names (Hennadii Stepanov)
9f005c60d6067d94279a3e3e81ec178bf4e2f227 cmake: Install `libsecp256k1.pc` file (Hennadii Stepanov)
Pull request description:
This PR allows downstream projects to use pkg-config to search for the libsecp256k1 library that is built with CMake.
Addressed https://github.com/bitcoin-core/secp256k1/discussions/1419#discussioncomment-6922896:
> We could just ship the pkg-config file also in CMake builds.
ACKs for top commit:
real-or-random:
ACK 421d84855a I compared the generated pc files and they match in autotools and CMake
Tree-SHA512: 8e54eb7c76bc727ab18715258c06cc2a419c6c04892a2bd7bfe34392f9a3223f673ff84d2d21b00b3c222b357f02296ec49c872532d98ea0a2f17ef1ed6b6ac1
9b118bc7fb12203e9bf98eb23bfdd5746adde2b3 release cleanup: bump version after 0.4.0 (Jonas Nick)
Pull request description:
based on #1415
ACKs for top commit:
sipa:
ACK 9b118bc7fb12203e9bf98eb23bfdd5746adde2b3
hebasto:
ACK 9b118bc7fb12203e9bf98eb23bfdd5746adde2b3
real-or-random:
ACK 9b118bc7fb12203e9bf98eb23bfdd5746adde2b3
Tree-SHA512: 76df87c41bdc3379df4e88619645f5110010d7713ebe20bad3e7c99472bd62b90f4bd3c6b558ad5a23119acc4734e39383d96a9800e4a43dfadc086ef66fd0ab
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.
8659a01714c1b4fcd349ee1a7d733f6934c5d184 ci: Add `release` job (Hennadii Stepanov)
f9b38894baee6b726217e28a4d4591ffdf233e47 ci: Update `actions/checkout` version (Hennadii Stepanov)
Pull request description:
This PR introduces a new "Release" job that conducts sanity checks as defined in [`doc/release-process.md`](https://github.com/bitcoin-core/secp256k1/blob/master/doc/release-process.md#sanity-checks).
ACKs for top commit:
sipa:
ACK 8659a01714c1b4fcd349ee1a7d733f6934c5d184
real-or-random:
ACK 8659a01714c1b4fcd349ee1a7d733f6934c5d184
Tree-SHA512: 84e03fa07f8c41aec0f6d1ccb4ac3643e85d370ef7e388b335365deadb555f2d9ef7e5d80e1255a18e790a774e04ca66f265b9441402b183d4c535a97688f20f
2635068abf93ebcf7f200eef14acafc3300e32f7 ci/gha: Let MSan continue checking after errors in all jobs (Tim Ruffing)
e78c7b68eb7e91cf9b88408c053867f148d62ffa ci/Dockerfile: Reduce size of Docker image further (Tim Ruffing)
2f0d3bbffb288621f4232c90424f77d44cc69166 ci/Dockerfile: Warn if `ulimit -n` is too high when running Docker (Tim Ruffing)
4b8a647ad3a59c2421d1d4c6e653f21d453e2612 ci/gha: Add ARM64 QEMU jobs for clang and clang-snapshot (Tim Ruffing)
6ebe7d2bb39978e70d85a718919d4af31cc00bc2 ci/Dockerfile: Always use versioned clang packages (Tim Ruffing)
Pull request description:
Solves one item in https://github.com/bitcoin-core/secp256k1/issues/1392.
This PR also has a few tweaks to the Dockerfile, see individual commits.
---
I'll follow up soon with a PR for ARM64/gcc. This will rely on Cirrus CI.
ACKs for top commit:
hebasto:
ACK 2635068abf93ebcf7f200eef14acafc3300e32f7.
Tree-SHA512: d290bdd8e8e2a2a2b6ccb1b25ecdc9662c51dab745068a98044b9abed75232d13cb9d2ddc2c63c908dcff6a12317f0c7a35db3288c57bc3b814793f7fce059fd
