test: refactor: simplify tests by using `_ecmult_gen_ge` helper, add test #1864

pull theStack wants to merge 2 commits into bitcoin-core:master from theStack:test-dedup-using-ecmult_gen_ge-helper changing 2 files +26 −17

theStack commented at 5:57 PM on June 8, 2026: contributor

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_ge helper. The second commit adds a simple unit tests, verifying for random scalars that the result of secp256k1_ecmult_gen_ge matches the two expected steps (secp256k1_ecmult_gen_gej plus Jacobian->affine conersion via secp256k1_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.

test: refactor: simplify tests by using `_ecmult_gen_ge` helper

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 #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.

ca68daf8e1

real-or-random added the label assurance on Jun 9, 2026
real-or-random added the label tweak/refactor on Jun 9, 2026
real-or-random commented at 8:00 AM on June 9, 2026: contributor

Concept ACK

Maybe secp256k1_ecmult_gen_ge should itself get a dedicated test
theStack commented at 3:16 PM on June 9, 2026: contributor

Maybe secp256k1_ecmult_gen_ge should itself get a dedicated test

Makes sense, added one. I decided to run the loop COUNT times... that's maybe a bit exaggerated, but it isn't noticeable in the runtime (tested two $ time SECP256K1_TEST_ITERS=1000 ./build/bin/tests -t=ecmult runs with and without the new test, and they both took about ~90 seconds on my machine).
theStack renamed this:
~~test: refactor: simplify tests by using `_ecmult_gen_ge` helper~~
test: refactor: simplify tests by using `_ecmult_gen_ge` helper, add test
on Jun 9, 2026
real-or-random approved
real-or-random commented at 11:32 AM on June 10, 2026: contributor

utACK bb51480fe4c46786f0d17ac7ebd84004ddcc4df9
real-or-random requested review from Copilot on Jun 11, 2026
?
copilot_work_started real-or-random
in src/tests.c:5786 in bb51480fe4
```
5780 | @@ -5786,6 +5781,25 @@ static void run_ecmult_constants(void) {
5781 |      }
5782 |  }
5783 |  
5784 | +static void run_ecmult_gen_ge(void) {
5785 | +    /* Test that secp256k1_ecmult_gen_ge result matches secp256k1_ecmult_gen_gej with
5786 | +     * manual Jaboci->affine conversion (secp256k1_ge_set_gej) over random scalars */
```
Copilot commented at 8:51 AM on June 11, 2026:

Spelling/terminology in this new test comment is off: it says "Jaboci" (should be "Jacobian") and uses an arrow for a coordinate conversion. Fixing the wording makes the intent clearer and avoids propagating the typo.

theStack commented at 3:58 PM on June 11, 2026:

Took that one, thanks.

Copilot commented at 8:51 AM on June 11, 2026: none

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_gej sequences in tests with secp256k1_ecmult_gen_ge.
Add a new run_ecmult_gen_ge test that compares secp256k1_ecmult_gen_ge output to the manual gej -> ge conversion 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.

💡 <a href="/bitcoin-core/secp256k1/new/master?filename=.github/instructions/*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.

test: add unit test for `_ecmult_gen_ge` 2ee79e77e6
in src/tests.c:5797 in bb51480fe4 outdated
```
5792 | +        secp256k1_ge result_ge, expected_ge;
5793 | +
5794 | +        testutil_random_scalar_order_test(&scalar);
5795 | +        secp256k1_ecmult_gen_gej(&CTX->ecmult_gen_ctx, &result_gej, &scalar);
5796 | +        secp256k1_ge_set_gej(&expected_ge, &result_gej);
5797 | +        secp256k1_ecmult_gen_ge(&CTX->ecmult_gen_ctx, &result_ge, &scalar);
```
Copilot commented at 8:51 AM on June 11, 2026:

This test reintroduces the exact pattern the new helper is meant to prevent: after calling secp256k1_ecmult_gen_gej, the temporary Jacobian point is not cleared. Even in tests, clearing it is cheap and keeps the codebase consistent about not leaving potentially scalar-dependent Jacobian state on the stack.

real-or-random commented at 12:20 PM on June 11, 2026:

No.
theStack force-pushed on Jun 11, 2026
real-or-random approved
real-or-random commented at 6:46 AM on June 12, 2026: contributor

utACK 2ee79e77e65bf5f7f747df1d978d04f1ace1e3a0
real-or-random merged this on Jun 12, 2026
real-or-random closed this on Jun 12, 2026

Contributors

theStack

real-or-random

Copilot

Labels

assurance tweak/refactor

Linked (view graph)

#1861 refactor: introduce `_ecmult_gen_ge` helper (preventing accidental gej leaks)