refactor: introduce `ecmult_const_ge` helper (preventing accidential gej leaks) #1933

pull theStack wants to merge 3 commits into bitcoin-core:master from theStack:ecmult_const_ge changing 7 files +35 −42

theStack commented at 11:07 PM on September 8, 2026: contributor
This PR is the counterpart of #1861 for _ecmult_const: constant-time scalar multiplication with arbitrary points frequently involves a conversion to affine coordinates and clearing out the temporary Jacobian group element object after to avoid leaking secret key material, i.e. executing the following three functions:
- secp256k1_ecmult_const(&rj, ...)
- secp256k1_ge_set_gej(&r, &rj)
- secp256k1_gej_clear(&rj)
A helper ecmult_const_ge is introduced to deduplicate code and mitigate the risk that the last step is forgotten (which can easily happen, as it would not be detected by tests). It is applied in the ECDH and silentpayments modules.

The idea came up in the course of reviewing the DLEQ module, where a gej clearing was missing, see #1802 (review).
real-or-random requested review from Copilot on Sep 9, 2026
real-or-random commented at 7:07 AM on September 9, 2026: contributor

Concept ACK
real-or-random added the label side-channel on Sep 9, 2026
real-or-random added the label tweak/refactor on Sep 9, 2026

Copilot commented at 7:24 AM on September 9, 2026: none

[!WARNING] Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Refactors constant-time scalar multiplication with arbitrary points by introducing secp256k1_ecmult_const_ge to deduplicate the common “multiply → convert to affine → clear Jacobian” pattern and reduce the risk of missing gej clearing (potential secret material remnants).

Changes:

Rename the Jacobian-returning helper from secp256k1_ecmult_const to secp256k1_ecmult_const_gej.
Add secp256k1_ecmult_const_ge which returns affine ge and clears the internal temporary gej.
Update ECDH, silentpayments, benches, and tests to use the new helpers.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

<details> <summary>Show a summary per file</summary>

File	Description
src/ecmult_const_impl.h	Renames core helper and adds `secp256k1_ecmult_const_ge` that clears temporary Jacobian state.
src/ecmult_const.h	Updates prototypes to expose the new helper names (`_gej` and `_ge`).
src/modules/ecdh/main_impl.h	Switches to `secp256k1_ecmult_const_ge`, removing manual `gej` clearing in ECDH.
src/modules/silentpayments/main_impl.h	Switches to `secp256k1_ecmult_const_ge`, removing manual `gej` clearing in silentpayments.
src/tests.c	Updates many test call sites to new helper names; simplifies some test flows to operate in affine.
src/tests_exhaustive.c	Updates exhaustive test call site to new `_gej` name.
src/bench_ecmult.c	Updates benchmark call site to new `_gej` name.

</details>

💡 <a href="/bitcoin-core/secp256k1/new/master?filename=.github/skills/code-review/SKILL.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add a code-review agent skill</a> or configure MCP servers for context-aware, tailored reviews. <a href="https://docs.github.com/copilot/how-tos/use-copilot-agents/request-a-code-review/use-code-review?tool=webui#mcp-servers-and-agent-skills" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn more in the docs.</a>

in src/ecmult_const.h:20 in 2abc345526

  12 | @@ -13,7 +13,8 @@
  13 |  /**
  14 |   * Multiply: R = q*A (in constant-time for q)
  15 |   */
  16 | -static void secp256k1_ecmult_const(secp256k1_gej *r, const secp256k1_ge *a, const secp256k1_scalar *q);
  17 | +static void secp256k1_ecmult_const_gej(secp256k1_gej *r, const secp256k1_ge *a, const secp256k1_scalar *q);
  18 | +static void secp256k1_ecmult_const_ge(secp256k1_ge *r, const secp256k1_ge *a, const secp256k1_scalar *q);
  19 |  
  20 |  /**
  21 |   * Same as secp256k1_ecmult_const, but takes in an x coordinate of the base point

Copilot commented at 7:24 AM on September 9, 2026:

This comment references secp256k1_ecmult_const, which is renamed in this PR. Update the documentation to refer to secp256k1_ecmult_const_gej (or whichever function the comment is describing) to avoid stale API docs.

theStack commented at 1:42 PM on September 9, 2026:

Done.

in src/ecmult_const.h:17 in 2abc345526 outdated
```
  12 | @@ -13,7 +13,8 @@
  13 |  /**
  14 |   * Multiply: R = q*A (in constant-time for q)
  15 |   */
  16 | -static void secp256k1_ecmult_const(secp256k1_gej *r, const secp256k1_ge *a, const secp256k1_scalar *q);
  17 | +static void secp256k1_ecmult_const_gej(secp256k1_gej *r, const secp256k1_ge *a, const secp256k1_scalar *q);
  18 | +static void secp256k1_ecmult_const_ge(secp256k1_ge *r, const secp256k1_ge *a, const secp256k1_scalar *q);
```
Copilot commented at 7:24 AM on September 9, 2026:

In src/ecmult_const_impl.h, secp256k1_ecmult_const_ge is defined as SECP256K1_INLINE static ..., but the header prototypes omit SECP256K1_INLINE. While this typically compiles, aligning the declaration style with the definition improves consistency and helps ensure expected inlining behavior across compilers/build flags.

theStack commented at 1:42 PM on September 9, 2026:

Seems we generally don't add SECP256K1_INLINE to header prototypes [1], so I'll leave it as-is in this PR.

[1] though there are two exceptions: int128.h and testrand.h, see $ git grep SECP256K1_INLINE.*\;
?
copilot_work_started real-or-random

refactor: introduce `_ecmult_const_ge` helper (preventing accidental gej leaks)

Constant-time scalar multiplication with arbitrary points frequently
involves a conversion to affine coordinates and clearing out the
temporary Jacobian group element object after to avoid leaking secret
key material, i.e. executing the following three steps:
    - secp256k1_ecmult_const(&rj, ...)
    - secp256k1_ge_set_gej(&r, &rj)
    - secp256k1_gej_clear(&rj)

This commit introduces a corresponding helper to deduplicate code
and mitigate the risk that last step is forgotten (which can easily
happen and is not detected by tests).

ace566f66c

refactor: rename `_ecmult_const` -> `_ecmult_const_gej` for consistency

Now that we have a function `_ecmult_const_ge`, it makes sense to rename
the existing function `_ecmult_const` to `_ecmult_const_gej` for
consistency, to signal that the result is a Jacobian group element.

This diff was created by applying
```
$ sed -i s/secp256k1_ecmult_const\(/secp256k1_ecmult_const_gej\(/g $(git ls-files)
```

b4fbee61e9

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

If the ecmult_const 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 the
previous commit.

02e00f54a0

theStack force-pushed on Sep 9, 2026

Contributors

theStack

real-or-random

Copilot

Labels

side-channel tweak/refactor

Linked (view graph)

#1802 Add DLEQ proof implementing BIP 374 #1861 refactor: introduce `_ecmult_gen_ge` helper (preventing accidental gej leaks)#1934 Clear secret-dependent variables in `_ecmult_const_xonly`