Upstream PRs 1058, 1518, 1522, 1523, 1525, 1524, 1526, 1528, 1517, 1532, 1488, 1533#2
Open
DarkWindman wants to merge 50 commits intomasterfrom
Open
Upstream PRs 1058, 1518, 1522, 1523, 1525, 1524, 1526, 1528, 1517, 1532, 1488, 1533#2DarkWindman wants to merge 50 commits intomasterfrom
DarkWindman wants to merge 50 commits intomasterfrom
Conversation
Instead of having the starting point of the ecmult_gen computation be offset, do it with the final point. This enables reasoning over the set of points reachable in intermediary computations, which can be leveraged by potential future optimization. Because the final point is in affine coordinates, its projective blinding is no longer possible. It will be reintroduced again in a different way, in a later commit. Also introduce some more comments and more descriptive names.
The old code overwrote the input at the start of the function, making a call like secp256k1_scalar_inverse(&x,&x) always fail.
This introduces the signed-digit multi-comb multiplication algorithm for constant-time G multiplications (ecmult_gen). It is based on section 3.3 of "Fast and compact elliptic-curve cryptography" by Mike Hamburg (see https://eprint.iacr.org/2012/309). Original implementation by Peter Dettman, with changes by Pieter Wuille to use scalars for recoding, and additional comments.
It is unnecessary to recompute this term needed by the SDMC algorithm for every multiplication; move it into the context scalar_offset value instead.
The old code would trigger UB when count=32.
The existing code needs to deal with the edge case that bit_pos >= 256, which would lead to an out-of-bounds read from secp256k1_scalar. Instead, recode the scalar into an array of uint32_t with enough zero padding at the end to alleviate the issue. This also simplifies the code, and is necessary for a security improvement in a follow-up commit. Original code by Peter Dettman, with modifications by Pieter Wuille.
Co-authored-by: Tim Ruffing <crypto@timruffing.de>
… algorithm 4c341f8 Add changelog entry for SDMC (Pieter Wuille) a043940 Permit COMB_BITS < 256 for exhaustive tests (Pieter Wuille) 39b2f2a Add test case for ecmult_gen recoded = {-1,0,1} (Pieter Wuille) 644e86d Reintroduce projective blinding (Pieter Wuille) 07810d9 Reduce side channels from single-bit reads (Peter Dettman) a0d32b5 Optimization: use Nx32 representation for recoded bits (Peter Dettman) e03dcc4 Make secp256k1_scalar_get_bits support 32-bit reads (Pieter Wuille) 5005abe Rename scalar_get_bits -> scalar_get_bits_limb32; return uint32_t (Pieter Wuille) 6247f48 Optimization: avoid unnecessary doublings in precomputation (Peter Dettman) 15d0cca Optimization: first table lookup needs no point addition (Pieter Wuille) 7a33db3 Optimization: move (2^COMB_BITS-1)/2 term into ctx->scalar_offset (Pieter Wuille) ed2a056 Provide 3 configurations accessible through ./configure (Pieter Wuille) 5f7be9f Always generate tables for current (blocks,teeth) config (Pieter Wuille) fde1dfc Signed-digit multi-comb ecmult_gen algorithm (Peter Dettman) 486518b Make exhaustive tests's scalar_inverse(&x,&x) work (Pieter Wuille) ab45c3e Initial gej blinding -> final ge blinding (Pieter Wuille) aa00a6b Introduce CEIL_DIV macro and use it (Tim Ruffing) Pull request description: ACKs for top commit: real-or-random: reACK 4c341f8 jonasnick: ACK 4c341f8 stratospher: ACK 4c341f8. Did [these benchmarks](bitcoin-core/secp256k1#1058 (comment)) and saw a 12.4% on gcc 13.2.0 and 11.5% on clang 15.0.0. Also summarised how the precomputed table generation works [here](https://github.com/stratospher/blogosphere/blob/main/sdmc.md) for future me :) Tree-SHA512: 9a11138e4fb98b98e85c82cd46ed78b29fbe63d6efe61654ef519a64b1e175d63395a8a931c1646f9df8c7daacd796d5fe2384899d5a13a2c7ed2ded696ceed5
Co-authored-by: Tim Ruffing <crypto@timruffing.de> Co-authored-by: Russell O'Connor <roconnor@blockstream.io>
7d2591c Add secp256k1_pubkey_sort (Jonas Nick) Pull request description: This PR adds a `secp256k1_pubkey_sort` function the the public API which was originally part of the musig PR (#1479). However, I opened a separate PR because it adds internal functions that are also used by the WIP silent payments module. ACKs for top commit: sipa: ACK 7d2591c josibake: ACK bitcoin-core/secp256k1@7d2591c real-or-random: ACK 7d2591c Tree-SHA512: d0e4464dc9cd4bdb35cc5d9bb4c37a7b71233328319165d49bc940d8d3394a2d74a43d2f73ee7bfe8f3f90a466ee8afcdca75cfbbf3969e218d76b89f4af55fb
Without this commit, the check-abi shell script outputs false positives because it consider some headers public that are actually not public.
…ic headers dd69556 check-abi: explicitly provide public headers (Jonas Nick) Pull request description: Without this commit, the check-abi shell script outputs false positives because it consider some headers public that are actually not public. ACKs for top commit: real-or-random: ACK bitcoin-core/secp256k1@dd69556 hebasto: ACK dd69556, tested on Ubuntu 24.04. Tree-SHA512: b26e61639061f5fbbdd47569ba04f91c627feeefc43ec3d529a3ac4012ab6487aa1904bd38100ed190dcaebdffe60895a8c99346720d5dee84a0c457ec3b6f94
Function secp256k1_ecmult_multi_var expects to be called with a non-NULL error_callback parameter. Fix the invocation in test_ecmult_accumulate to do this. While at it, wrap the call in a CHECK macro to ensure it succeeds. Fixes: bitcoin-core/secp256k1#1527
…when using as subproject 9f4c8cd cmake: Fix `check_arm32_assembly` when using as subproject (Hennadii Stepanov) Pull request description: When integrating libsecpk1 in a downstream project like this: ```cmake set(SECP256K1_ASM arm32 CACHE STRING "" FORCE) add_subdirectory(src/secp256k1) ``` it fails to configure: ``` CMake Error at /home/hebasto/git/bitcoin/build/check_arm32_assembly/CMakeFiles/CMakeTmp/CMakeLists.txt:21 (target_sources): Cannot find source file: /home/hebasto/git/bitcoin/cmake/source_arm32.s CMake Error at /home/hebasto/git/bitcoin/build/check_arm32_assembly/CMakeFiles/CMakeTmp/CMakeLists.txt:20 (add_executable): No SOURCES given to target: cmTC_d0f0b CMake Error at src/secp256k1/cmake/CheckArm32Assembly.cmake:2 (try_compile): Failed to generate test project build system. Call Stack (most recent call first): src/secp256k1/CMakeLists.txt:127 (check_arm32_assembly) ``` This PR fixes this issue, which was overlooked in bitcoin-core/secp256k1#1304. ACKs for top commit: real-or-random: utACK 9f4c8cd theuni: utACK 9f4c8cd Tree-SHA512: 47d97ad0fb2e3779523c2111ea75906671a0fb3f50646e29dee195f53106ace69af5e4abc92c765f0eee6973528ce9195b94377d0157209230c958894d4049fb
…i_var` with a non-`NULL` error callback 9554362 tests: call secp256k1_ecmult_multi_var with a non-NULL error callback (Nicolas Iooss) Pull request description: Hello, This Pull Request fixes the issue reported in bitcoin-core/secp256k1#1527. Function `secp256k1_ecmult_multi_var` expects to be called with a non-`NULL` `error_callback` parameter. Fix the invocation in `test_ecmult_accumulate` to do this. While at it, wrap the call in a `CHECK` macro to ensure it succeeds. ACKs for top commit: real-or-random: utACK 9554362 siv2r: ACK 9554362, I have also verified that other invocations of `ecmult_multi_var` (in tests) don’t use `NULL` for the error callback function argument. Tree-SHA512: 6a9f6c10c575794da75f2254d6fbbc195de889c81a371ce35ab38e2e5483aa1e25ec0bcd5aa8d6a32a1493586f73430208a4bd0613e373571d2f04d63dbc4a1c
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
to make it more promiment
The rename was done with the following command: $ sed -i 's/random_group_element_/random_ge_/g' $(git grep -l random_group_element_)
…e_magnitude` Note that the already existing function `random_fe_magnitude` is removed and the call-sites are adapted to pass the magnitude range of 8 (the maximum for secp256k1_fe_mul and secp256k1_fe_sqr) explicitly.
Can be reviewed via `--color-moved=dimmed-zebra`.
The rename was done with the following command: $ sed -i 's/secp256k1_testrand/testrand/g' $(git grep -l secp256k1_testrand)
…time_tests ebfb82e ci: Add job with -fsanitize-memory-param-retval (Tim Ruffing) e1bef09 configure: Move "experimental" warning to bottom (Tim Ruffing) 55e5d97 autotools: Disable eager MSan in ctime_tests (Tim Ruffing) Pull request description: This is the autotools solution for #1516. Alternatively, we could have a full-blown `--enable-msan` option, but it's more work, and I'm not convinced that it's necessary or at least much better. hebasto If you're Concept ACK, are you willing to work on an equivalent PR for CMake? ACKs for top commit: hebasto: ACK ebfb82e, tested on Ubuntu 24.04 with different clang versions (from 15 to 18) and different build configurations. CI changes look OK as well. Tree-SHA512: c083d778fd50bd35c2e29b7fe0d92b98d912ee5ac7809ae73067d050a0d3c42b3483260f1286d0023cdb802a3c3006bf932ecf60ce81b942de1c9824374c0132
Clang >= 16 has `-fsanitize-memory-param-retval` turned on by default, which is incompatible with
This change makes both Autotools and CMake build systems consistent.
…_tests f55703b autotools: Delete unneeded compiler test (Hennadii Stepanov) 396e885 autotools: Align MSan checking code with CMake's implementation (Hennadii Stepanov) abde59f cmake: Report more compiler details in summary (Hennadii Stepanov) 7abf979 cmake: Disable `ctime_tests` if build with `-fsanitize=memory` (Hennadii Stepanov) Pull request description: Same as bitcoin-core/secp256k1#1517, but for the CMakle build system. The second commit improves the configure summary (similar to hebasto/bitcoin#189. ACKs for top commit: real-or-random: ACK f55703b Tree-SHA512: 18190c062ae6e27d0ecbe7460cc22c960b25c0d35aa4b94f151d4b1c48f16e99fd5ecdfcb359784f95995292633d30d3d23b75a12be3aca5afffcc1e7e7daf31
218f0cc ci: Add native macOS arm64 job (Hennadii Stepanov) Pull request description: This PR starts using the [new](https://github.blog/changelog/2024-01-30-github-actions-introducing-the-new-m1-macos-runner-available-to-open-source/) M1 macOS runner. The alternative approach might be using a matrix, but it is not trivial to implement. ACKs for top commit: real-or-random: ACK 218f0cc Tree-SHA512: 709e836909fa2a90248f689f4c57192d1daecc53abd3d2d1b8b892a7deb6fdd008bf8f7270ab39da5b0f994d0ea4cf0767dab3a07c6dfc2109a9735af1072f3f
…tions (#1491) e73f6f8 tests: refactor: drop `secp256k1_` prefix from testrand.h functions (Sebastian Falbesoner) 0ee7453 tests: refactor: add `testutil_` prefix to testutil.h functions (Sebastian Falbesoner) 0c6bc76 tests: refactor: move `random_` helpers from tests.c to testutil.h (Sebastian Falbesoner) 0fef847 tests: refactor: rename `random_field_element_magnitude` -> `random_fe_magnitude` (Sebastian Falbesoner) 59db007 tests: refactor: rename `random_group_element_...` -> `random_ge_...` (Sebastian Falbesoner) Pull request description: This PR is an attempt at tidying up test util functions, as suggested in #1491. The following changes are done: * rename `_group_element...` functions to `_ge...` * rename `_field_element...` functions to `_fe...` * move `random_` helpers from tests.c to testutil.h (the alternative would be testrand.h, but to my understanding, this one is meant to contain the actual RNG implementation rather than helpers using it; happy to move the helpers there if that is preferred though) * prefix testutil.h functions with `testutil_` * prefix testrand.h functions with `testrand_` (this is currently done in a sloppy way by simply dropping the `secp256k1_` prefix, so some functions don't have the full prefix, like e.g. `testrand256`; naming suggestions welcome) ACKs for top commit: sipa: utACK e73f6f8 real-or-random: utACK e73f6f8 Tree-SHA512: c87a35a9f7f23d4bbb87a1ff0d40dd5fbd7d976719ca1027cad187ac44aa2db3ae887ac620639d2287c260e701a5963830b52048692d3e6b38b5eb6cdf17b854
This merge contains conflicts that need manual resolution. Files with conflicts will be marked with conflict markers.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
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#1058]: Signed-digit multi-comb ecmult_gen algorithm
[bitcoin-core/secp256k1#1518]: Add secp256k1_pubkey_sort
[bitcoin-core/secp256k1#1522]: release: prepare for 0.5.0
[bitcoin-core/secp256k1#1523]: release cleanup: bump version after 0.5.0
[bitcoin-core/secp256k1#1525]: changelog: Correct 0.5.0 release date
[bitcoin-core/secp256k1#1524]: check-abi: explicitly provide public headers
[bitcoin-core/secp256k1#1526]: cmake: Fix
check_arm32_assemblywhen using as subproject[bitcoin-core/secp256k1#1528]: tests: call
secp256k1_ecmult_multi_varwith a non-NULLerror callback[bitcoin-core/secp256k1#1517]: autotools: Disable eager MSan in ctime_tests
[bitcoin-core/secp256k1#1532]: cmake: Disable eager MSan in ctime_tests
[bitcoin-core/secp256k1#1488]: ci: Add native macOS arm64 job
[bitcoin-core/secp256k1#1533]: tests: refactor: tidy up util functions (#1491)
This PR was automatically generated by GitHub Actions.
Tips:
git show --remerge-diff <pr-branch>to show the conflict resolution in the merge commit.git read-tree --reset -u <pr-branch>to replay these resolutions during the conflict resolution stage when recreating the PR branch locally.Be aware that this may discard your index as well as the uncommitted changes and untracked files in your worktree.