Upstream PRs 1426, 1430, 1184, 1437, 1442, 1441, 1445, 1438, 1393, 1446, 1450, 1451, 1431, 990, 1455, 1380, 1465, 1466, 1473, 1474, 1476, 1480, 1468, 1482, 1249 #285
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
Co-authored-by: Tim Ruffing <crypto@timruffing.de>
fa4d6c7 ci/cirrus: Add native ARM64 persistent workers (MarcoFalke) 2262d0e ci/cirrus: Bring back skeleton .cirrus.yml without jobs (Tim Ruffing) Pull request description: ACKs for top commit: real-or-random: ACK fa4d6c7 hebasto: re-ACK fa4d6c7, only last two commits have been squashed since my recent [review](bitcoin-core/secp256k1#1426 (review)). Tree-SHA512: d1fee99d54a41a4126f7eb72695a56137c925dc9ce7cd692a60ea1262ac0789bbd6aa4e4dfc030f0d97d06aeeae0724a5f2d794a85ff533c6cf3cd215f6a4b7a
We're not solely using cirrus anymore and github already displays the CI status at a different location.
Co-authored-by: Jonas Nick <jonasd.nick@gmail.com> Co-authored-by: Tim Ruffing <crypto@timruffing.de>
* add test case for a=infinity The corresponding ecmult_const branch was not tested before this commit. * add test for edge cases
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.
…gorithm 355bbdf Add changelog entry for signed-digit ecmult_const algorithm (Pieter Wuille) 21f49d9 Remove unused secp256k1_scalar_shr_int (Pieter Wuille) 115fdc7 Remove unused secp256k1_wnaf_const (Pieter Wuille) aa9f3a3 ecmult_const: add/improve tests (Jonas Nick) 4d16e90 Signed-digit based ecmult_const algorithm (Pieter Wuille) ba523be make SECP256K1_SCALAR_CONST reduce modulo exhaustive group order (Pieter Wuille) 2140da9 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 355bbdf siv2r: ACK 355bbdf real-or-random: ACK 355bbdf Tree-SHA512: 13db572cb7f9be00bf0931c65fcd8bc8b5545be86a8c8700bd6a79ad9e4d9e5e79e7f763f92ca6a91d9717a355f8162204b0ea821b6ae99d58cb400497ddc656
…shot compilers 8185e72 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 8185e72 Tree-SHA512: 0f41ca8303bd3d6efefcd3a544c7bd7dfcf464c57c779c876da4a77cacd262e6c963449d493fdf5a641b0d10b655c8c67fe8a147145b6533328d7bf5344313e1
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.
These temporaries seem to been inadvertently changed to signed during a refactoring. Generally, bit shifting is frowned upon for signed values.
These changes bring the checks to the same values used at the corresponding positions in secp256k1_fe_sqr_inner.
…ed in secp256k1_fe_sqr_inner 1027135 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 1027135 real-or-random: utACK 1027135 Tree-SHA512: a9fefe4b146163209662cd435422beb3c9561eb9e83110454184f70df2292992f39ec1971143428e039a80cad2f6285db74de2f059e877ad8756ff739269b67a
…or non-exec stack 33dc7e4 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 33dc7e4 real-or-random: utACK 33dc7e4 Tree-SHA512: f75ded8d971f54d1e871bcc4d815ba367b3e154eea2f18309ecaf9053e22f986bfffcf28418367f8055b65a5a0b245fee045adfcb63a2196df5e2f3aa6c97b89
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.
because we don't know whether it's an optimization.
…internal 1ddd76a bench: add --help option to bench_internal (Sebastian Falbesoner) Pull request description: While coming up with commands for running the benchmarks for issue bitcoin-core/secp256k1#726 (comment), 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 bitcoin-core/secp256k1#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 1ddd76a siv2r: ACK 1ddd76a, tested the `--help` option locally, and it works as expected. Tree-SHA512: d117641a5f25a7cbf83881f3acceae99624528a0cbb2405efdbe1a3a2762b4d6b251392e954aaa32f6771069d31143743770fccafe198084c12258dedb0856fc
…_mul_inner dcdda31 Tighten secp256k1_fe_mul_inner's VERIFY_BITS checks (Russell O'Connor) 8e2a5fe 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 dcdda31 tested with asm disabled Tree-SHA512: c35170e37d9a6d1413dd625032028129ab2eccee7da86697ab9641b68ad78efd7251953d51e7acaefd14888d3fd61877f9f05349c44f6fc0133ce9b3921b0e1a
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.
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.
Replaces "ctx: a secp256k1 context object" with "ctx: pointer to a context object". Also removes the word "existing".
This gets rid of an untested code path. Resolves #1352. secp256k1_ge_storage is a struct with two secp256k1_fe_storage fields. The C standard allows the compiler to add padding between the fields and at the end of the struct, but no sane compiler in the end would do this: The only reason to add padding is to ensure alignment, but such padding is never necessary between two fields of the same type. Similarly, secp256k1_fe_storage is a struct with a single array of uintXX_t. No padding is allowed between array elements. Again, C allows the compiler to insert padding at the end of the struct, but there's no absolute reason to do so in this case. For the uintXX_t itself, this guaranteed to have no padding bits, i.e., it's guaranteed to have exactly XX bits. So I claim that for any existing compiler in the real world, sizeof(secp256k1_ge_storage) == 64.
This also splits the big "&&" expression into separate expressions. If we ever see an assertion fail, the error message will tell it precisely which one failed.
da7bc1b include: in doc, remove article in front of "pointer" (Jonas Nick) aa3dd52 include: make doc about ctx more consistent (Jonas Nick) e3f6900 include: remove obvious "cannot be NULL" doc (Jonas Nick) Pull request description: ACKs for top commit: sipa: ACK da7bc1b real-or-random: ACK da7bc1b Tree-SHA512: 809f312fa0cd1e9502ac79b8a1c502b87e6dfc2db8ad6bbd96d7ddbdaadad0c3b6110fa704b770c353cd34d5bf5547541cbb5f2779425d7419b584e721c854c2
…k1_ge_storage) == 64 code path ba5d72d assumptions: Use new STATIC_ASSERT macro (Tim Ruffing) e53c2d9 Require that sizeof(secp256k1_ge_storage) == 64 (Tim Ruffing) d0ba2ab util: Add STATIC_ASSERT macro (Tim Ruffing) Pull request description: This gets rid of an untested code path. Resolves bitcoin-core/secp256k1#1352. This is a bit opinionated in the sense that it adds a static assertion where it's needed in `secp256k1_pubkey_save` and `secp256k1_pubkey_load`. I think this is justified in this case. It helps the reviewer verify that these functions are correct. See individual commit messages. ACKs for top commit: sipa: utACK ba5d72d jonasnick: ACK ba5d72d Tree-SHA512: 2553c0610b62bcda6d4ef26eb26b5b2e07acf723bcd299691a2d02da57af22b8763f63c2d4adb17d30de8825b6157be6e4f0398147854fbabdf8b865fb0b5c88
b37fdb2 check-abi: Minor UI improvements (Tim Ruffing) ad5f589 check-abi: Default to HEAD for new version (Tim Ruffing) 9fb7e2f release process: Style and formatting nits (Tim Ruffing) e7053d0 release process: Add email step (Tim Ruffing) 429d21d release process: Run sanity checks on release PR (Tim Ruffing) Pull request description: ACKs for top commit: hebasto: ACK b37fdb2. jonasnick: ACK b37fdb2 Tree-SHA512: 6e18a5b897d29a3dd3a73ba81623dd91c04fa6730fb56374b924dc84baaec8c55d0c689ee1a41dab9a03ccd566082fc59ffb5d68cafd536a136fc7aaac2d8ef5
This also makes the order in which module options are processed consistent between CMake and autotools (the reverse order of the listing printed to stdout).
… dependencies e682267 build: Error if required module explicitly off (Tim Ruffing) 89ec583 build: Clean up handling of module dependencies (Tim Ruffing) Pull request description: This is a cleanup which makes it easier to add further modules with dependencies, e.g., in #1452. The diff looks larger than it is because I also reordered the modules and made the order consistent between CMake and autotools. (We noticed that the current logic could be improved in BlockstreamResearch#275.) ACKs for top commit: jonasnick: ACK e682267 hebasto: ACK e682267. Tree-SHA512: 040e791e5b5b9b8845a39632633a45ca759391455910bdefba2b7b77c6340e65df6eda18199ae2ad65c30ee2fc6630471437aec143c26fe09ae4c11409a37622
… configure option 42f8c51 cmake: Add `SECP256K1_LATE_CFLAGS` configure option (Hennadii Stepanov) Pull request description: This PR enables users to override compiler flags that have been set by the CMake-based build system, such as warning flags. The Autotools-based build system has the same feature out-of-the-box. See more details [here](bitcoin-core/secp256k1#1235 (comment)). Here are some examples of the new option usage: ``` cmake -S . -B build -DSECP256K1_LATE_CFLAGS="-Wno-extra -Wlong-long" ``` ``` cmake -S . -B build -DSECP256K1_BUILD_EXAMPLES=ON -DSECP256K1_LATE_CFLAGS=-O1 cmake --build build ... In function ‘secp256k1_ecmult_strauss_wnaf’, inlined from ‘secp256k1_ecmult’ at /home/hebasto/git/secp256k1/src/ecmult_impl.h:353:5: /home/hebasto/git/secp256k1/src/ecmult_impl.h:291:5: warning: ‘aux’ may be used uninitialized [-Wmaybe-uninitialized] 291 | secp256k1_ge_table_set_globalz(ECMULT_TABLE_SIZE(WINDOW_A) * no, state->pre_a, state->aux); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from /home/hebasto/git/secp256k1/src/secp256k1.c:29: /home/hebasto/git/secp256k1/src/ecmult_impl.h: In function ‘secp256k1_ecmult’: /home/hebasto/git/secp256k1/src/group_impl.h:174:13: note: by argument 3 of type ‘const secp256k1_fe *’ to ‘secp256k1_ge_table_set_globalz’ declared here 174 | static void secp256k1_ge_table_set_globalz(size_t len, secp256k1_ge *a, const secp256k1_fe *zr) { | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ In file included from /home/hebasto/git/secp256k1/src/secp256k1.c:30: /home/hebasto/git/secp256k1/src/ecmult_impl.h:345:18: note: ‘aux’ declared here 345 | secp256k1_fe aux[ECMULT_TABLE_SIZE(WINDOW_A)]; | ^~~ ... ``` Please note that in the last case providing `env CFLAGS=-O1` or `-DCMAKE_C_FLAGS=-O1` won't work. ACKs for top commit: real-or-random: ACK 42f8c51 Tree-SHA512: 2b152e420a4a8ffd5f67857de03ae5ba9f2223e535ac01a867c1025e0619180d8255fdd1e5fb8279b290f0a1c96bcc874043ef968fcd99b1ff4e13041a91b1e1
jonasnick
reviewed
Jan 19, 2024
|
While we're at it: in |
|
That all makes sense, I'll address it next week. |
real-or-random
force-pushed
the
temp-merge-1249
branch
from
5626ff8
to
097b960
Compare
Like upstream bitcoin-core/secp256k1#1476 .
It was a verbatim copy of secp256k1_ec_pubkey_cmp.
Remove explicity VERIFY_CHECKs in keyaggcoef_internal since normalization should be checked in the fe_* functions.
real-or-random
force-pushed
the
temp-merge-1249
branch
from
097b960
to
b673a43
Compare
|
I've addressed all your comments. |
jonasnick
approved these changes
Jan 23, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[bitcoin-core/secp256k1#1426]: ci/cirrus: Add native ARM64 jobs
[bitcoin-core/secp256k1#1430]: README: remove CI badge
[bitcoin-core/secp256k1#1184]: Signed-digit based ecmult_const algorithm
[bitcoin-core/secp256k1#1437]: ci: Ignore internal errors of snapshot compilers
[bitcoin-core/secp256k1#1442]: Return temporaries to being unsigned in secp256k1_fe_sqr_inner
[bitcoin-core/secp256k1#1441]: asm: add .note.GNU-stack section for non-exec stack
[bitcoin-core/secp256k1#1445]: bench: add --help option to bench_internal
[bitcoin-core/secp256k1#1438]: correct assertion for secp256k1_fe_mul_inner
[bitcoin-core/secp256k1#1393]: Implement new policy for VERIFY_CHECK and #ifdef VERIFY (issue #1381)
[bitcoin-core/secp256k1#1446]: field: Remove x86_64 asm
[bitcoin-core/secp256k1#1450]: Add group.h ge/gej equality functions
[bitcoin-core/secp256k1#1451]: changelog: add entry for "field: Remove x86_64 asm"
[bitcoin-core/secp256k1#1431]: Add CONTRIBUTING.md
[bitcoin-core/secp256k1#990]: Add comment on length checks when parsing ECDSA sigs
[bitcoin-core/secp256k1#1455]: doc: improve secp256k1_fe_set_b32_mod doc
[bitcoin-core/secp256k1#1380]: Add ABI checking tool for release process
[bitcoin-core/secp256k1#1465]: release: prepare for 0.4.1
[bitcoin-core/secp256k1#1466]: release cleanup: bump version after 0.4.1
[bitcoin-core/secp256k1#1473]: Fix typos
[bitcoin-core/secp256k1#1474]: tests: restore scalar_mul test
[bitcoin-core/secp256k1#1476]: include: make docs more consistent
[bitcoin-core/secp256k1#1480]: Get rid of untested sizeof(secp256k1_ge_storage) == 64 code path
[bitcoin-core/secp256k1#1468]: v0.4.1 release aftermath
[bitcoin-core/secp256k1#1482]: build: Clean up handling of module dependencies
[bitcoin-core/secp256k1#1249]: cmake: Add
SECP256K1_LATE_CFLAGSconfigure optionThis PR can be recreated with
./contrib/sync-upstream.sh -b master range e4af41c61b0bb55fc9614cb39df8e455715b4dd4.Tip: Use
git show --remerge-diffto show the changes manually added to the merge commit.This also cherry-picks a few commits from https://github.com/jonasnick/secp256k1-zkp/commits/musig2-upstream/ and one bitcoin-core/secp256k1#1479 .