Internal merge 1661#6
Open
mllwchrry wants to merge 37 commits into
Open
Conversation
This is a dump mechanical translation of secp256k1_ge_set_all_gej_var that assumes that inputs are not infinity.
No semantic changes.
Due to similarity to the public API function `secp256k1_ec_pubkey_serialize`, public API flags like `SECP256K1_EC_COMPRESSED` are sometimes mistakingly passed to newly proposed code (this is currently the case for several modules in secp256k1-zkp, see BlockstreamResearch#300). which is currently not detected. To avoid this in the future, a VERIFY_CHECK is added to check that the `compressed` argument is either 0 or 1.
…ecp256k1_eckey_pubkey_serialize` 1823594 Verify `compressed` argument in `secp256k1_eckey_pubkey_serialize` (Sebastian Falbesoner) Pull request description: Due to similarity to the public API function `secp256k1_ec_pubkey_serialize`, public API flags like `SECP256K1_EC_COMPRESSED` are sometimes mistakingly passed to `secp256k1_eckey_pubkey_serialize` in newly proposed code (this is currently the case for several modules in secp256k1-zkp, see BlockstreamResearch#300), which is currently not detected. To avoid this in the future, a VERIFY_CHECK is added to check that the `compressed` argument is either 0 or 1. ACKs for top commit: real-or-random: utACK 1823594 stratospher: ACK 1823594. Got tests failures when passing public API flags to `secp256k1_eckey_pubkey_serialize`. Tree-SHA512: ca542afc87f33e436ba33dc55b285dfe3759007c446ef94503bc1044c7a0a7f7b2208ae82e2c9743fc5fa38cf386127f3fbfa02d2c242f28fab3041ee46f153b
These function aliases have been described as DEPRECATED in the public API docs already many years ago (see #701, commit 41fc785), and in addition explicit deprecation warnings are shown by the compiler at least since the first official release 0.2.0 (see PR #1089, commit fc94a2d), so it should be fine to just remove them by now. Co-authored-by: Tim Ruffing <crypto@timruffing.de>
…musig for own public nonces 64228a6 musig: Use _ge_set_all_gej for own public nonces (Tim Ruffing) 300aab1 tests: Improve _ge_set_all_gej(_var) tests (Tim Ruffing) 365f274 group: Simplify secp256k1_ge_set_all_gej (Tim Ruffing) d3082dd group: Add constant-time secp256k1_ge_set_all_gej (Tim Ruffing) Pull request description: As suggested in bitcoin-core/secp256k1#1479 (comment) ACKs for top commit: theStack: re-ACK 64228a6 sipa: ACK 64228a6 Tree-SHA512: f62a95e44dc09bb55a64da0640ad323e7ef5acc262d3c2aea6787eae0918769ea97da466b7d602e59693e4fb85c5ec9a67fdfba8b890624467855b6d1e1596c0
Fixes a silent merge conflict between #1614 and #1579.
… `clang-cl` 4c50d73 ci: Add new "Windows (clang-cl)" job (Hennadii Stepanov) 84c0bd1 cmake: Adjust diagnostic flags for clang-cl (Hennadii Stepanov) Pull request description: When building with `clang-cl` on Windows, the output is cluttered with warning messages because compiler diagnostic flags are not applied correctly: ``` > cmake -B build -G Ninja -DCMAKE_C_COMPILER="C:\Users\hebasto\Downloads\clang+llvm-18.1.8-x86_64-pc-windows-msvc\bin\clang-cl.exe" > cmake --build build [1/16] Building C object src\CMakeFiles\bench.dir\bench.c.obj In file included from C:\Users\hebasto\secp256k1\src\bench.c:11: C:\Users\hebasto\secp256k1\src\util.h(34,13): warning: unused function 'print_buf_plain' [-Wunused-function] 34 | static void print_buf_plain(const unsigned char *buf, size_t len) { | ^~~~~~~~~~~~~~~ 1 warning generated. [2/16] Building C object src\CMakeFiles\secp256k1_precomputed.dir\precomputed_ecmult_gen.c.obj In file included from C:\Users\hebasto\secp256k1\src\precomputed_ecmult_gen.c:3: In file included from C:\Users\hebasto\secp256k1\src\group.h:10: In file included from C:\Users\hebasto\secp256k1\src\field.h:10: C:\Users\hebasto\secp256k1\src\util.h(34,13): warning: unused function 'print_buf_plain' [-Wunused-function] 34 | static void print_buf_plain(const unsigned char *buf, size_t len) { | ^~~~~~~~~~~~~~~ ``` This PR resolves this issue. --- **Additional note for reviewers:** The VS builtin clang can also be used assuming that the following VS components are installed:  The user can generate a build system on Windows as follows: - Using the default "Visual Studio" generator: ``` cmake -B build -T ClangCL ``` - Using the "Ninja" generator: ``` cmake -B build -G Ninja -DCMAKE_C_COMPILER=clang-cl ``` --- Required for downstream projects which aim to build with `clang-cl` (see bitcoin/bitcoin#31456). ACKs for top commit: real-or-random: utACK 4c50d73 Tree-SHA512: 439eb53afd7be65d538cd569f3d095f58325bd26ffc5014ca5f94320689a45b20c9a5a963170578214a20fd3233ec15ef6ab75ab96ce3a4314c282b1b6229ca1
Before this commit, we didn't print *_example.log files and test_suite.log. Printing is now handled in a separate action, which avoids code duplication and makes the ci.yml file more readable. This changes the folding/grouping of the log output in the GitHub Actions CI, but I think the new variant is as good as the old one. Furthermore, the condition for printing the logs is changed from "always()" to "!cancelled()". This ensures that logs will still be printed if previous steps such as the CI script failed, but that they won't be printed if the entire run is cancelled (e.g., by clicking a button in the UI or through a force-push to the PR). This is in line with a recommendation in the GHA docs: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/evaluate-expressions-in-workflows-and-actions#always
…eparate action 59860bc gha: Print all *.log files, in a separate action (Tim Ruffing) Pull request description: Before this commit, we didn't print *_example.log files and test_suite.log. Printing is now handled in a separate action, which avoids code duplication and makes the ci.yml file more readable. This changes the folding/grouping of the log output in the GitHub Actions CI, but I think the new variant is as good as the old one. Furthermore, the condition for printing the logs is changed from "always()" to "!cancelled()". This ensures that logs will still be printed if previous steps such as the CI script failed, but that they won't be printed if the entire run is cancelled (e.g., by clicking a button in the UI or through a force-push to the PR). This is in line with a recommendation in the GHA docs: https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/evaluate-expressions-in-workflows-and-actions#always ACKs for top commit: hebasto: ACK 59860bc. sipa: ACK 59860bc Tree-SHA512: ca11f5e5f01660964276b9c2e11c22caeed8492e9c7ffaa2078aaaa733005c63242fc93a1056124fb8f1f83019d46818c12b10142fb10f43270a8562fd10885a
This change makes the `-fvisibility=hidden` compiler option unnecessary.
Co-authored-by: Tim Ruffing <crypto@timruffing.de>
…gate,tweak_add,tweak_mul}` aliases from API 37d2c60 Remove deprecated _ec_privkey_{negate,tweak_add,tweak_mul} aliases (Sebastian Falbesoner) Pull request description: ACKs for top commit: real-or-random: utACK 37d2c60 sipa: utACK 37d2c60 jonasnick: ACK 37d2c60 Tree-SHA512: 5d3c836c3c4d5cde143fe5b5235f9fc108174439b056f3418834f33d12ea28bdf09d11a81917d679b4b9a93da26304241c8fe389549e72796bbda116e9ff4945
…test for it d147876 build: Drop no longer needed `-fvisibility=hidden` compiler option (Hennadii Stepanov) 8ed1d83 ci: Run `tools/symbol-check.py` (Hennadii Stepanov) 41d32ab test: Add `tools/symbol-check.py` (Hennadii Stepanov) 8854805 Introduce `SECP256K1_LOCAL_VAR` macro (Hennadii Stepanov) Pull request description: Closes bitcoin-core/secp256k1#1181. [Catches](bitcoin-core/secp256k1#1359 (comment)) the actual symbol visibility issue. Replaces bitcoin-core/secp256k1#1135. ACKs for top commit: real-or-random: reACK d147876 Tree-SHA512: 4d39f3c4cd32afa2b4418ca79331c72827c76a49a5422afa7c85e60d00a750b91b1b1ab10d91ba578f5817dd938016751168758461fb89de8da56f7d005ae2da
This callback function has been unused since a1d52e3
…legal_callback_fn 51907fa tests: remove unused uncounting_illegal_callback_fn (Jonas Nick) Pull request description: ACKs for top commit: real-or-random: utACK bitcoin-core/secp256k1@51907fa Tree-SHA512: 6959671e31938be833eb17ee762aa6d0e7b6789c951058cd32e46bfd95ad96a65625a0b976541b4e6c7d648fdad8af66b8e5503b38fed6387216d281a1513eed
This makes the usage of the atribute consistent. In the musig and ellswift module, functions that return 1 always already don't have the WARN_UNUSED_RESULT attribute. In secp256k1.h and the extrakeys module, this has only been the case partially. In all cases where this was removed, the function only returns 0 if the illegal callback has been called.
Fixes the following bash error when make fails:
./ci/ci.sh: line 100: return: can only `return' from a function or
sourced script
d87c3bc ci: Fix exiting from ci.sh on error (Tim Ruffing) Pull request description: Fixes the following bash error when make fails: ./ci/ci.sh: line 100: return: can only `return' from a function or sourced script ACKs for top commit: hebasto: ACK d87c3bc Tree-SHA512: 5ecd0f550f7659cc41b403fdb7d5d3d37d1a167d585cca02b0aca209c8b9592bb3067cf11aeb80775666e7232f31bf05cf1bb97fec8c67f3bc5fe2243ddbbcfa
Fixes #1658.
… for functions always returning 1 1b6e081 include: remove WARN_UNUSED_RESULT for functions always returning 1 (Jonas Nick) Pull request description: This makes the usage of the atribute consistent. In the musig and ellswift module, functions that return 1 always already don't have the WARN_UNUSED_RESULT attribute. In secp256k1.h and the extrakeys module, this has only been the case partially. In all cases where this was removed, the function only returns 0 if the illegal callback has been called. Fixes #1379 ACKs for top commit: real-or-random: utACK bitcoin-core/secp256k1@1b6e081 sipa: utACK 1b6e081 Tree-SHA512: 5d1f2563ddde34fb721dd0b96622e0888a9c72f95af6f1c8a94f7f1f72ca934b6af98a3357c1e922d8611a9869264bb0f3ceb7bed0164c6c3a6f92f9950d4f19
…n summary 20b05c9 configure: Show exhaustive tests in summary (Tim Ruffing) Pull request description: ACKs for top commit: hebasto: ACK 20b05c9, it aligns now with the CMake script: https://github.com/bitcoin-core/secp256k1/blob/e56716a3bcaf108bb7365ee8f2b06f9ec68504c8/CMakeLists.txt#L348-L350 sipa: utACK 20b05c9 jonasnick: ACK 20b05c9 Tree-SHA512: 30744ea5e5b7441ad252868c83cebfec2b02b75786b9ea55d4b0b0a696f1c7df39c48c243b29b13839a9f3a7757c192da8be0dd95412678a7583b123db6e99ac
mllwchrry
force-pushed
the
internal-merge-1654
branch
from
5052ea4 to
53c6f04
Compare
mllwchrry
force-pushed
the
internal-merge-1661
branch
from
92dcec4 to
87a98fc
Compare
DarkWindman
force-pushed
the
internal-merge-1654
branch
3 times, most recently
from
315d214 to
a307800
Compare
DarkWindman
force-pushed
the
internal-merge-1661
branch
2 times, most recently
from
a3a0890 to
0537991
Compare
DarkWindman
force-pushed
the
internal-merge-1661
branch
from
0537991 to
92dfca3
Compare
The `macos-13` image has been deprecated and will be unavailable soon. See: actions/runner-images#13045.
mllwchrry
pushed a commit
that referenced
this pull request
Mar 3, 2026
…to improve parallelism 8354618 cmake: Set `LABELS` property for tests (Hennadii Stepanov) 29f26ec cmake: Integrate DiscoverTests and normalize test names (Hennadii Stepanov) f95b263 cmake: Add DiscoverTests module (Hennadii Stepanov) 4ac6511 cmake, refactor: Deduplicate test-related code (Hennadii Stepanov) Pull request description: This PR implements the idea suggested in bitcoin-core/secp256k1#1734 (review) and is based on the work from bitcoin/bitcoin#33483. Here is an example of the `ctest` output: ``` $ ctest --test-dir build -j $(nproc) Test project /home/hebasto/dev/secp256k1/secp256k1/build Start 1: secp256k1.noverify_tests.selftest_tests Start 2: secp256k1.noverify_tests.all_proper_context_tests Start 3: secp256k1.noverify_tests.all_static_context_tests Start 4: secp256k1.noverify_tests.deprecated_context_flags_test <snip> 193/196 Test #31: secp256k1.noverify_tests.ecmult_constants ......................... Passed 5.32 sec 194/196 Test BlockstreamResearch#184: secp256k1.tests.ellswift_xdh_correctness_tests .................... Passed 5.62 sec 195/196 Test BlockstreamResearch#191: secp256k1.exhaustive_tests ........................................ Passed 6.97 sec 196/196 Test BlockstreamResearch#126: secp256k1.tests.ecmult_constants .................................. Passed 9.60 sec 100% tests passed, 0 tests failed out of 196 Label Time Summary: secp256k1_example = 0.02 sec*proc (5 tests) secp256k1_exhaustive = 6.97 sec*proc (1 test) secp256k1_noverify_tests = 23.77 sec*proc (95 tests) secp256k1_tests = 43.67 sec*proc (95 tests) Total Test time (real) = 10.21 sec ``` For comparison, here is the output for the master branch on the same machine: ``` $ ctest --test-dir build -j $(nproc) Test project /home/hebasto/dev/secp256k1/secp256k1/build Start 1: secp256k1_noverify_tests Start 2: secp256k1_tests Start 3: secp256k1_exhaustive_tests Start 4: secp256k1_ecdsa_example Start 5: secp256k1_ecdh_example Start 6: secp256k1_schnorr_example Start 7: secp256k1_ellswift_example Start 8: secp256k1_musig_example 1/8 Test #4: secp256k1_ecdsa_example .......... Passed 0.00 sec 2/8 Test #5: secp256k1_ecdh_example ........... Passed 0.00 sec 3/8 Test #6: secp256k1_schnorr_example ........ Passed 0.00 sec 4/8 Test #7: secp256k1_ellswift_example ....... Passed 0.00 sec 5/8 Test #8: secp256k1_musig_example .......... Passed 0.00 sec 6/8 Test #3: secp256k1_exhaustive_tests ....... Passed 6.26 sec 7/8 Test #1: secp256k1_noverify_tests ......... Passed 14.31 sec 8/8 Test #2: secp256k1_tests .................. Passed 31.65 sec 100% tests passed, 0 tests failed out of 8 Total Test time (real) = 31.65 sec ``` --- **New Feature:** As the number of tests has grown, the _labels_ have been introduced to simplify test management. Now, one can run: ``` $ ctest --test-dir build -j $(nproc) -L example Test project /home/hebasto/dev/secp256k1/secp256k1/build Start 192: secp256k1.example.ecdsa Start 193: secp256k1.example.ecdh Start 194: secp256k1.example.schnorr Start 195: secp256k1.example.ellswift Start 196: secp256k1.example.musig 1/5 Test BlockstreamResearch#192: secp256k1.example.ecdsa .......... Passed 0.00 sec 2/5 Test BlockstreamResearch#193: secp256k1.example.ecdh ........... Passed 0.00 sec 3/5 Test BlockstreamResearch#194: secp256k1.example.schnorr ........ Passed 0.00 sec 4/5 Test BlockstreamResearch#195: secp256k1.example.ellswift ....... Passed 0.00 sec 5/5 Test BlockstreamResearch#196: secp256k1.example.musig .......... Passed 0.00 sec 100% tests passed, 0 tests failed out of 5 Label Time Summary: secp256k1_example = 0.01 sec*proc (5 tests) Total Test time (real) = 0.01 sec ``` or ``` $ ctest --test-dir build -j $(nproc) -LE tests Test project /home/hebasto/dev/secp256k1/secp256k1/build Start 192: secp256k1.example.ecdsa Start 193: secp256k1.example.ecdh Start 194: secp256k1.example.schnorr Start 195: secp256k1.example.ellswift Start 196: secp256k1.example.musig Start 191: secp256k1.exhaustive_tests 1/6 Test BlockstreamResearch#192: secp256k1.example.ecdsa .......... Passed 0.00 sec 2/6 Test BlockstreamResearch#193: secp256k1.example.ecdh ........... Passed 0.00 sec 3/6 Test BlockstreamResearch#194: secp256k1.example.schnorr ........ Passed 0.00 sec 4/6 Test BlockstreamResearch#195: secp256k1.example.ellswift ....... Passed 0.00 sec 5/6 Test BlockstreamResearch#196: secp256k1.example.musig .......... Passed 0.00 sec 6/6 Test BlockstreamResearch#191: secp256k1.exhaustive_tests ....... Passed 6.19 sec 100% tests passed, 0 tests failed out of 6 Label Time Summary: secp256k1_example = 0.01 sec*proc (5 tests) secp256k1_exhaustive = 6.19 sec*proc (1 test) Total Test time (real) = 6.20 sec ``` ACKs for top commit: purpleKarrot: ACK 8354618 furszy: Tested ACK 8354618 Tree-SHA512: 8c506ab08491aba4836b3058a8a09c929c6dd097c11e4e6f4deb20cf602285e73c3fd8a2c2040f7e92a058c7f8fc09752fa9de2ce80f7673adbdd505237ed262
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
6 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.
No description provided.