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.
33dc7e4d3e 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 33dc7e4d3e
real-or-random:
utACK 33dc7e4d3e
Tree-SHA512: f75ded8d971f54d1e871bcc4d815ba367b3e154eea2f18309ecaf9053e22f986bfffcf28418367f8055b65a5a0b245fee045adfcb63a2196df5e2f3aa6c97b89
10271356c8 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 10271356c8
real-or-random:
utACK 10271356c8
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.
8185e72d29 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 8185e72d29
Tree-SHA512: 0f41ca8303bd3d6efefcd3a544c7bd7dfcf464c57c779c876da4a77cacd262e6c963449d493fdf5a641b0d10b655c8c67fe8a147145b6533328d7bf5344313e1
355bbdf38a Add changelog entry for signed-digit ecmult_const algorithm (Pieter Wuille)
21f49d9bec Remove unused secp256k1_scalar_shr_int (Pieter Wuille)
115fdc7232 Remove unused secp256k1_wnaf_const (Pieter Wuille)
aa9f3a3c00 ecmult_const: add/improve tests (Jonas Nick)
4d16e90111 Signed-digit based ecmult_const algorithm (Pieter Wuille)
ba523be067 make SECP256K1_SCALAR_CONST reduce modulo exhaustive group order (Pieter Wuille)
2140da9cd5 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 355bbdf38a
siv2r:
ACK 355bbdf
real-or-random:
ACK 355bbdf38a
Tree-SHA512: 13db572cb7f9be00bf0931c65fcd8bc8b5545be86a8c8700bd6a79ad9e4d9e5e79e7f763f92ca6a91d9717a355f8162204b0ea821b6ae99d58cb400497ddc656
fa4d6c76b6 ci/cirrus: Add native ARM64 persistent workers (MarcoFalke)
2262d0eaab ci/cirrus: Bring back skeleton .cirrus.yml without jobs (Tim Ruffing)
Pull request description:
ACKs for top commit:
real-or-random:
ACK fa4d6c76b6
hebasto:
re-ACK fa4d6c76b6, only last two commits have been squashed since my recent [review](https://github.com/bitcoin-core/secp256k1/pull/1426#pullrequestreview-1636119941).
Tree-SHA512: d1fee99d54a41a4126f7eb72695a56137c925dc9ce7cd692a60ea1262ac0789bbd6aa4e4dfc030f0d97d06aeeae0724a5f2d794a85ff533c6cf3cd215f6a4b7a
c45b7c4fbb refactor: introduce testutil.h (deduplicate `random_fe_`, `ge_equals_` helpers) (Sebastian Falbesoner)
dc5514144f 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 c45b7c4fbb
siv2r:
ACK `c45b7c4` (reviewed the changes and tests for both the commits passed locally).
Tree-SHA512: 4ffa66dd0b8392d7d0083a71e7b0682ad18f9261fd4ce8548c3059b497d3462db97e16114fded9787661ca447a877a27f5b996bd7d47e6f91c4454079d28a8ac
421d84855a ci: Align Autotools/CMake `CI_INSTALL` directory names (Hennadii Stepanov)
9f005c60d6 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
9b118bc7fb release cleanup: bump version after 0.4.0 (Jonas Nick)
Pull request description:
based on #1415
ACKs for top commit:
sipa:
ACK 9b118bc7fb
hebasto:
ACK 9b118bc7fb
real-or-random:
ACK 9b118bc7fb
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.
8659a01714 ci: Add `release` job (Hennadii Stepanov)
f9b38894ba 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 8659a01714
real-or-random:
ACK 8659a01714
Tree-SHA512: 84e03fa07f8c41aec0f6d1ccb4ac3643e85d370ef7e388b335365deadb555f2d9ef7e5d80e1255a18e790a774e04ca66f265b9441402b183d4c535a97688f20f
2635068abf ci/gha: Let MSan continue checking after errors in all jobs (Tim Ruffing)
e78c7b68eb ci/Dockerfile: Reduce size of Docker image further (Tim Ruffing)
2f0d3bbffb ci/Dockerfile: Warn if `ulimit -n` is too high when running Docker (Tim Ruffing)
4b8a647ad3 ci/gha: Add ARM64 QEMU jobs for clang and clang-snapshot (Tim Ruffing)
6ebe7d2bb3 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 2635068abf.
Tree-SHA512: d290bdd8e8e2a2a2b6ccb1b25ecdc9662c51dab745068a98044b9abed75232d13cb9d2ddc2c63c908dcff6a12317f0c7a35db3288c57bc3b814793f7fce059fd
The underlying issue does not affect our CI hosts, but is an issue on my
development machine (Arch Linux). In particular, this affects the vanilla
configuration of Docker on systemd, which has effectively no limit:
11400a3f5a/pkg/docker-engine/common/systemd/docker.service (L31)
I hope this saves future generations some precious hours of their life.
This commit switches to a new strategy to make sure we're installing the
most recent LLVM packages. Before this commit, we used the unversioned
LLVM packages (e.g., `clang` instead of `clang-18`), which are supposed
to provide the latest snapshot, but this is broken for arm64 [1],
which we want to add in a later PR.
Anyway, the new approach is cleaner because it does not require us to
fiddle with the installed `clang` package by removing a symlink.
[1] https://github.com/llvm/llvm-project/issues/64790
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
