test: refactor: simplify tests by using _ecmult_gen_ge helper, add test#1864
Merged
real-or-random merged 2 commits intoJun 12, 2026
Merged
Conversation
If the generator point multiplication result in Jacobian coordinates is immediately converted to affine coordinates after and is not needed for anything else, we can deduplicate by using the helper introduced in bitcoin-core#1861. Note that in a very strict sense this is not a refactor, as the Jacobian object is now cleared out which was not done on master, but for the logic in the tests this shouldn't matter at all.
real-or-random
left a comment
Contributor
There was a problem hiding this comment.
Concept ACK
Maybe secp256k1_ecmult_gen_ge should itself get a dedicated test
Contributor
Author
Makes sense, added one. I decided to run the loop |
_ecmult_gen_ge helper_ecmult_gen_ge helper, add test
There was a problem hiding this comment.
Pull request overview
This PR updates the test suite to use the new secp256k1_ecmult_gen_ge helper where generator scalar multiplication results are immediately needed in affine coordinates, and adds a unit test to confirm secp256k1_ecmult_gen_ge matches the gej-plus-conversion path.
Changes:
- Replace
secp256k1_ecmult_gen_gej+secp256k1_ge_set_gejsequences in tests withsecp256k1_ecmult_gen_ge. - Add a new
run_ecmult_gen_getest that comparessecp256k1_ecmult_gen_geoutput to the manualgej -> geconversion over random scalars. - Update exhaustive tests to use the affine helper directly.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/tests.c |
Refactors generator-multiply test callsites to secp256k1_ecmult_gen_ge and adds a new unit test for the helper. |
src/tests_exhaustive.c |
Simplifies the exhaustive ecmult_gen verification by using secp256k1_ecmult_gen_ge directly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bb51480 to
2ee79e7
Compare
sedited
pushed a commit
to bitcoin/bitcoin
that referenced
this pull request
Jun 19, 2026
bd0287d650 Merge bitcoin-core/secp256k1#1859: field: force-inline 5x52 mul and sqr fdcf2d41e2 Merge bitcoin-core/secp256k1#1865: test: enable -Wunused-function in test suite (Fix #1831) b2d2bd362d Merge bitcoin-core/secp256k1#1860: cmake: Emulate Libtool's behavior on NetBSD and OpenBSD 87bec430bf Merge bitcoin-core/secp256k1#1867: test: musig: fix dead "aggnonce encodes two points at infinity" check 71fcd8410e field: force-inline 5x52 mul and sqr a77dacad9a test: enable -Wunused-function in test suite (Fix #1831) aea86bc350 Merge bitcoin-core/secp256k1#1864: test: refactor: simplify tests by using `_ecmult_gen_ge` helper, add test 2ee79e77e6 test: add unit test for `_ecmult_gen_ge` d7125e517d test: musig: fix dead "aggnonce encodes two points at infinity" check 1eab757207 cmake: Fix shared library versioning on OpenBSD a401c5145a cmake: Fix shared library versioning on NetBSD 8a0f4002c7 cmake, refactor: Improve documenting in `SetLibtoolAbiVersion` module acf2084aa7 cmake, refactor: Introduce `SetLibtoolAbiVersion` module 0f4a7e6bf9 Merge bitcoin-core/secp256k1#1855: bench: add internal benchmark for `secp256k1_fe_normalize_var` ca68daf8e1 test: refactor: simplify tests by using `_ecmult_gen_ge` helper 13db747f2b Merge bitcoin-core/secp256k1#1861: refactor: introduce `_ecmult_gen_ge` helper (preventing accidental gej leaks) 9e017e5062 refactor: rename `_ecmult_gen` -> `_ecmult_gen_gej` for consistency a3296d5e23 refactor: introduce `_ecmult_gen_ge` helper (preventing accidental gej leaks) c63062380f Merge bitcoin-core/secp256k1#1852: Add exhaustive test for ECDH module 240578eef5 bench: add internal benchmark for `secp256k1_fe_normalize_var` 5698e66c64 Add exhaustive test for ECDH module a39093de15 Merge bitcoin-core/secp256k1#1851: doc: correct API docs for ECDSA signing out-params (s/array/signature object/) 8363a2d8d1 Merge bitcoin-core/secp256k1#1854: tests: compare full MuSig aggregate nonce af1fdd1215 tests: compare full MuSig aggregate nonce 40a0d874a6 doc: correct API docs for ECDSA signing out-params (s/array/signature object/) b11340b3ce Merge bitcoin-core/secp256k1#1849: musig: always clear out secret key in `secp256k1_musig_nonce_gen_counter` 8479eafa57 musig: always clear out secret key in `secp256k1_musig_nonce_gen_counter` c1a9e4fe64 Merge bitcoin-core/secp256k1#1848: ci: Bump GCC snapshot major version to 17 3cca6451a2 ci: Bump GCC snapshot major version to 17 ea174fe045 Merge bitcoin-core/secp256k1#1846: ci: Replace `ilammy/msvc-dev-cmd` with manual MSVC setup 285cb788e9 ci: Replace `ilammy/msvc-dev-cmd` with manual MSVC setup git-subtree-dir: src/secp256k1 git-subtree-split: bd0287d650c24dc41e0362675a9f6a49ee952def
sedited
added a commit
to bitcoin/bitcoin
that referenced
this pull request
Jun 19, 2026
1f3f0a4 Squashed 'src/secp256k1/' changes from 7262adb4b4..bd0287d650 (fanquake) Pull request description: Includes: * bitcoin-core/secp256k1#1846 * bitcoin-core/secp256k1#1848 * bitcoin-core/secp256k1#1849 * bitcoin-core/secp256k1#1851 * bitcoin-core/secp256k1#1852 * bitcoin-core/secp256k1#1854 * bitcoin-core/secp256k1#1855 * bitcoin-core/secp256k1#1859 * bitcoin-core/secp256k1#1860 * bitcoin-core/secp256k1#1861 * bitcoin-core/secp256k1#1864 * bitcoin-core/secp256k1#1865 * bitcoin-core/secp256k1#1867 The most interesting change to call out / that reviewers might want to test probably being bitcoin-core/secp256k1#1859. ACKs for top commit: theStack: ACK 9caae50 hebasto: ACK 9caae50. sedited: ACK 9caae50 andrewtoth: ACK 9caae50 Tree-SHA512: 187e76d7f7ed603d81b7c355abfb184f37658fdb5109a2c78f72c615cdf862dfc9c3e2ff4556eb047d59348ba1a7030ca38342682c56dbfdb51610d2cc1de405
Kino1994
pushed a commit
to Kino1994/bitcoin-full-history
that referenced
this pull request
Jun 28, 2026
1f3f0a4 Squashed 'src/secp256k1/' changes from 7262adb4b4..bd0287d650 (fanquake) Pull request description: Includes: * bitcoin-core/secp256k1#1846 * bitcoin-core/secp256k1#1848 * bitcoin-core/secp256k1#1849 * bitcoin-core/secp256k1#1851 * bitcoin-core/secp256k1#1852 * bitcoin-core/secp256k1#1854 * bitcoin-core/secp256k1#1855 * bitcoin-core/secp256k1#1859 * bitcoin-core/secp256k1#1860 * bitcoin-core/secp256k1#1861 * bitcoin-core/secp256k1#1864 * bitcoin-core/secp256k1#1865 * bitcoin-core/secp256k1#1867 The most interesting change to call out / that reviewers might want to test probably being bitcoin-core/secp256k1#1859. ACKs for top commit: theStack: ACK eaad00f hebasto: ACK eaad00f. sedited: ACK eaad00f andrewtoth: ACK eaad00f Tree-SHA512: 187e76d7f7ed603d81b7c355abfb184f37658fdb5109a2c78f72c615cdf862dfc9c3e2ff4556eb047d59348ba1a7030ca38342682c56dbfdb51610d2cc1de405
real-or-random
added a commit
to BlockstreamResearch/secp256k1-zkp
that referenced
this pull request
Jul 9, 2026
…65, 1859, 1879, 1881, 1845, 1877, 1882, 1884 217fe59 tests: register run_util_tests and run_ec_commit, remove dead print_vector (mllwchrry) 21645c0 SECURITY.md: remove Jonas Nick from trusted keys (Jonas Nick) 6a599a4 scalar: correct `_scalar_get_bits_{limb32,var}` input condition docs (Sebastian Falbesoner) 994b350 field: correct fe_equal's b magnitude bound (Lőrinc) 0cad3df Improve checks for scalar _get_bits methods (Peter.Dettman) 9d75769 tests: Fix GCC 17 snapshot warning (Tim Ruffing) 66260b7 ci: add 'brew trust' invocation to macOS CI (fanquake) 71fcd84 field: force-inline 5x52 mul and sqr (Lőrinc) a77daca test: enable -Wunused-function in test suite (Fix #1831) (kallal79) 1eab757 cmake: Fix shared library versioning on OpenBSD (Hennadii Stepanov) a401c51 cmake: Fix shared library versioning on NetBSD (Hennadii Stepanov) 8a0f400 cmake, refactor: Improve documenting in `SetLibtoolAbiVersion` module (Hennadii Stepanov) acf2084 cmake, refactor: Introduce `SetLibtoolAbiVersion` module (Hennadii Stepanov) d7125e5 test: musig: fix dead "aggnonce encodes two points at infinity" check (Sebastian Falbesoner) 2ee79e7 test: add unit test for `_ecmult_gen_ge` (Sebastian Falbesoner) ca68daf test: refactor: simplify tests by using `_ecmult_gen_ge` helper (Sebastian Falbesoner) 240578e bench: add internal benchmark for `secp256k1_fe_normalize_var` (Sebastian Falbesoner) 9e017e5 refactor: rename `_ecmult_gen` -> `_ecmult_gen_gej` for consistency (Sebastian Falbesoner) a3296d5 refactor: introduce `_ecmult_gen_ge` helper (preventing accidental gej leaks) (Sebastian Falbesoner) 5698e66 Add exhaustive test for ECDH module (Sebastian Falbesoner) 40a0d87 doc: correct API docs for ECDSA signing out-params (s/array/signature object/) (Sebastian Falbesoner) Pull request description: *Note: This PR has been created by a GitHub Actions workflow without human involvement.* This PR syncs the following upstream PRs: * bitcoin-core/secp256k1#1851: doc: correct API docs for ECDSA signing out-params (s/array/signature object/) * bitcoin-core/secp256k1#1852: Add exhaustive test for ECDH module * bitcoin-core/secp256k1#1861: refactor: introduce `_ecmult_gen_ge` helper (preventing accidental gej leaks) * bitcoin-core/secp256k1#1855: bench: add internal benchmark for `secp256k1_fe_normalize_var` * bitcoin-core/secp256k1#1864: test: refactor: simplify tests by using `_ecmult_gen_ge` helper, add test * bitcoin-core/secp256k1#1867: test: musig: fix dead "aggnonce encodes two points at infinity" check * bitcoin-core/secp256k1#1860: cmake: Emulate Libtool's behavior on NetBSD and OpenBSD * bitcoin-core/secp256k1#1865: test: enable -Wunused-function in test suite (Fix #1831) * bitcoin-core/secp256k1#1859: field: force-inline 5x52 mul and sqr * bitcoin-core/secp256k1#1879: ci: add 'brew trust' invocation to macOS CI * bitcoin-core/secp256k1#1881: tests: Fix GCC 17 snapshot warning * bitcoin-core/secp256k1#1845: Improve checks for scalar _get_bits methods * bitcoin-core/secp256k1#1877: field: correct `fe_equal` magnitude bound for `b` * bitcoin-core/secp256k1#1882: scalar: correct `_scalar_get_bits_{limb32,var}` input condition docs * bitcoin-core/secp256k1#1884: SECURITY.md: remove Jonas Nick from trusted keys Usage hints: * If this PR has merge conflicts, resolve these by switching to the PR branch and merging the base branch into it using `git merge <base-branch>`. * To show the conflict resolution diff from an existing merge commit, use `git show --remerge-diff <merge-commit>`. * In case you are recreating the PR branch locally, you can (during the conflict resolution state) replay this conflict resolution diff using `git read-tree --reset -u <merge-commit>`. Be aware that this may discard your index as well as the uncommitted changes and untracked files in your worktree. ACKs for top commit: DarkWindman: ACK 217fe59 real-or-random: ACK 217fe59 Tree-SHA512: 761fb08be08afcbaa3a43f7432c92a5e65113545a2a25131baf3385d7e354d2a913856f667c8478729877a074d98c0e58fc0c92b17b70a583cbd96d1f478d7f2
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
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.
This PR is a small follow-up to #1861. If the generator point multiplication result in Jacobian coordinates is immediately converted to affine coordinates after and is not needed for anything else, we can deduplicate by using the new
secp256k1_ecmult_gen_gehelper. The second commit adds a simple unit tests, verifying for random scalars that the result ofsecp256k1_ecmult_gen_gematches the two expected steps (secp256k1_ecmult_gen_gejplus Jacobian->affine conersion viasecp256k1_ge_set_gej).Note that in a very strict sense the first commit is not a refactor, as the Jacobian object is now cleared out which was not done on master, but for the logic in the tests this shouldn't matter at all.