group: VERIFY input/output ge/gej/fe exhaustively #1923

real-or-random commented at 10:06 AM on August 25, 2026: contributor

This PR splits the functions in group_impl.h in a wrapper that only performs VERIFY and an _impl function that has the actual code. This ensures that the post VERIFY calls are not skipped in case of early returns.

Also, this PR adds VERIFY calls wherever they were missing (inside group_impl.h)

The new structure is similar to field_impl.h but a bit simpler because we don't need to deal with two different implementations. A real difference is that, in non-VERIFY mode, field_impl.h delegates via #defines (ensure there's no overhead due to a function call) and here I decided to delegate via function calls. It keeps the code a bit simpler to read (and maybe also simpler to parser for tools such as language servers). The _impl functions all have SECP256K1_INLINE. I think every sane compiler will inline the function calls in non-VERIFY mode (even without SECP256K1_INLINE) because the body of the wrapper is really just a single function call then with the same signature.

Follow-up PR can cover modifications of ges and fes outside the group and field modules, e.g., ecmult modifies ges/gejs directly. Maybe it will be good that it does this only through group functions but we'll need to see; in C++ this module could legitimately be considered a "friend" of group.

real-or-random added the label tweak/refactor on Aug 25, 2026

real-or-random force-pushed on Aug 25, 2026

real-or-random added the label assurance on Aug 25, 2026

theStack commented at 2:17 PM on August 26, 2026: contributor

Concept ACK

Out of curiosity I tried out if following a "no early returns" policy in the group module instead could be a viable alternative to splitting up functions, but as expected it's quite ugly overall: https://github.com/theStack/secp256k1/commit/2f818928543f9dd8c78cd2c2a3af3f0ee589fb02 (and introducing goto would probably be very controversial in general, even though I personally think usage in some "common cleanup" scenarios it's okay).

The _impl functions all have SECP256K1_INLINE. I think every sane compiler will inline the function calls in non-VERIFY mode (even without SECP256K1_INLINE) because the body of the wrapper is really just a single function call then with the same signature.

That would also be my guess. If we are worried about that, using the recently introduced SECP256K1_FORCE_INLINE macro could maybe be an option.

real-or-random commented at 4:00 PM on August 26, 2026: contributor

Out of curiosity I tried out if following a "no early returns" policy in the group module instead could be a viable alternative to splitting up functions, but as expected it's quite ugly overall: theStack@2f81892 (and introducing goto would probably be very controversial in general, even though I personally think usage in some "common cleanup" scenarios it's okay).

Yeah, goto was my first attempt, I personally don't think goto is evil. If used carefully, it certainly cleaner code than using if/else to eliminate early returns.

But I think the wrapping approach in this PR is conceptually clean, avoids the goto debate and it matches what we do in the field module, so I picked that one.

in src/group_impl.h:1096 in 430445e288

1092 | @@ -1093,15 +1093,22 @@ static int secp256k1_ge_is_in_correct_subgroup(const secp256k1_ge* ge) {
1093 |      return secp256k1_ge_impl_is_in_correct_subgroup(ge);
1094 |  }
1095 |  
1096 | -static int secp256k1_ge_x_on_curve_var(const secp256k1_fe *x) {
1097 | +static int secp256k1_ge_impl_x_on_curve_var(const secp256k1_fe *x) {

theStack commented at 4:50 PM on August 26, 2026:

looks like this is the only instance of a _{ge,gej}_impl_... function that doesn't have the _INLINE keyword, is that intentional?

real-or-random commented at 6:45 AM on August 27, 2026:

fixed

in src/group.h:59 in bb4790f568

  54 | @@ -55,6 +55,9 @@ typedef struct {
  55 |  /** Set a group element equal to the point with given X and Y coordinates */
  56 |  static void secp256k1_ge_set_xy(secp256k1_ge *r, const secp256k1_fe *x, const secp256k1_fe *y);
  57 |  
  58 | +/** Set r to the affine coordinates of the Jacobian point (a.x, a.y, 1/zi). */
  59 | +static void secp256k1_ge_set_ge_zinv(secp256k1_ge *r, const secp256k1_ge *a, const secp256k1_fe *zi);

theStack commented at 5:00 PM on August 26, 2026:

the same could be done for secp256k1_ge_set_gej_zinv (probably even more relevant as that one is called outside of group_impl.h [in ecmult_impl.h], while secp256k1_ge_set_ge_zinv is currently not)

real-or-random commented at 6:44 AM on August 27, 2026:

fixed

theStack commented at 5:01 PM on August 26, 2026: contributor

LGTM, will check tomorrow if there is any size difference of the library binary between master and this PR (which would indicate whether the _impl functions are actually inlined or not).

real-or-random force-pushed on Aug 27, 2026

in src/group.h:96 in af1c96918f outdated

  91 | +/** Set r to the affine coordinates of the Jacobian point (a.x, a.y, 1/zi).
  92 | + *  If a is infinity, then r will be infinity. */
  93 | +static void secp256k1_ge_set_ge_zinv(secp256k1_ge *r, const secp256k1_ge *a, const secp256k1_fe *zi);
  94 | +
  95 | +/** Set r to the affine coordinates of the Jacobian point (a.x, a.y, 1/zi), ignoring a.z.
  96 | + *  If a is infinity, then r will be infinity. */

theStack commented at 1:46 PM on August 27, 2026:

for both _ge_set_{ge,gej}_zinv: according to the precondition checks a is not allowed to be infinity (we could replace r->infinity = a->infinity with r->infinity = 0 accordingly I guess)

real-or-random commented at 2:45 PM on August 27, 2026:

Oh indeed, will fix.

real-or-random commented at 8:36 AM on September 7, 2026:

added a commit

in src/group_impl.h:1132 in ce204b34db

1126 | @@ -1120,35 +1127,60 @@ static int secp256k1_ge_x_frac_on_curve_var(const secp256k1_fe *xn, const secp25
1127 |       return secp256k1_fe_is_square_var(&r);
1128 |  }
1129 |  
1130 | -static void secp256k1_ge_to_bytes(unsigned char *buf, const secp256k1_ge *a) {
1131 | +static int secp256k1_ge_x_frac_on_curve_var(const secp256k1_fe *xn, const secp256k1_fe *xd) {
1132 | +    SECP256K1_FE_VERIFY(xn);
1133 | +    SECP256K1_FE_VERIFY(xd);

theStack commented at 1:50 PM on August 27, 2026:

could move the VERIFY_CHECK(!secp256k1_fe_normalizes_to_zero_var(xd)); line from the _impl function to here

real-or-random commented at 2:44 PM on August 27, 2026:

Yeah, this was the style I was trying to follow (like in field the same). But now I'm wondering if it's the best one.

Wouldn't it make more readable to have these "semantic" conditions in the _impl function because this is where our brains need them for understanding? edit: Me overlooking the VERIFY_CHECK in _ge_set_{ge,gej}_zinv (your previous comment) somewhat proves this.

(The setting in field is a bit different: there are two different implementations but they share the preconditions, so it makes sense to check them in the wrapper.)

Also thinking about saving some lines:

Removing the blank line between _impl and wrapper to make them visually appear together.
Removing the blank lines within _impl.
Keeping all inputs on one line and all outputs on one line.

Like this:

SECP256K1_INLINE static void secp256k1_gej_impl_cmov(secp256k1_gej *r, const secp256k1_gej *a, int flag) {
    VERIFY_CHECK(flag == 0 || flag == 1);
    secp256k1_fe_cmov(&r->x, &a->x, flag);
    secp256k1_fe_cmov(&r->y, &a->y, flag);
    secp256k1_fe_cmov(&r->z, &a->z, flag);
    r->infinity ^= (r->infinity ^ a->infinity) & flag;
}
SECP256K1_INLINE static void secp256k1_gej_cmov(secp256k1_gej *r, const secp256k1_gej *a, int flag) {
    SECP256K1_GEJ_VERIFY(r); SECP256K1_GEJ_VERIFY(a);
    secp256k1_gej_impl_cmov(r, a, flag);
    SECP256K1_GEJ_VERIFY(r);
}

What do you think?

theStack commented at 1:23 PM on August 28, 2026:

Wouldn't it make more readable to have these "semantic" conditions in the _impl function because this is where our brains need them for understanding? edit: Me overlooking the VERIFY_CHECK in _ge_set_{ge,gej}_zinv (your previous comment) somewhat proves this.

Good point. So the idea then is to only use the _{FE,GE,GEJ}_VERIFY (depending on the types used in the in and out parameters) checks in the wrappers which mostly checks magnitude conditions and the "semantic" conditions would be in the _impl functions, right?

Also thinking about saving some lines: ... What do you think?

Nice idea, looks good to me; I think I wouldn't mind still having blank lines in the _impl functions (at least from a ratio point of view they are less wasteful than in the wrappers :p), but either way seems fine.

real-or-random commented at 1:34 PM on August 28, 2026:

I think I wouldn't mind still having blank lines in the _impl functions

ah sorry, yes, I meant removing the blank lines in the wrappers (as in my example). will implement.

real-or-random commented at 8:36 AM on September 7, 2026:

done

apoelstra approved

apoelstra commented at 10:23 AM on September 6, 2026: contributor

ACK af1c96918f7355ae267f5f997378dd063505a3a4; successfully ran local tests

group: Split functions into pre/post VERIFY and _impl

This ensures that the post VERIFY calls are not skipped in case of early
returns.

cddd125f72

group: Remove redundant SECP256K1_GEJ_VERIFY

Redundant after the previous commit.

1bd7492181

group: Add missing FE_VERIFY checks on rzr outputs 0711443e6d

group: Cover all input/output ge/gej/fe with VERIFY calls 4f3bd158d5

real-or-random force-pushed on Sep 7, 2026

group: Export secp256k1_ge_set_ge(j)_zinv properly in group.h 031dbae659

group/refactor: Use constant expression for readability

Both touched functions have a `VERIFY_CHECK(!a->infinity)`.

c36269ef95

real-or-random force-pushed on Sep 7, 2026

real-or-random commented at 8:37 AM on September 7, 2026: contributor

forced-pushed, diff: https://github.com/bitcoin-core/secp256k1/compare/af1c96918f7355ae267f5f997378dd063505a3a4..c36269ef95da99498b3b62736f09dc42ffe5d094

ready from my side

theStack approved

theStack commented at 3:26 PM on September 7, 2026: contributor

ACK c36269ef95da99498b3b62736f09dc42ffe5d094

real-or-random requested review from Copilot on Sep 7, 2026

?

copilot_work_started real-or-random

Copilot commented at 8:06 PM on September 7, 2026: none

🔵 Needs a closer look

The broad refactor affects core cryptographic group arithmetic and warrants final human validation.

<details> <summary>Pull request overview</summary>

Refactors group operations so VERIFY builds consistently validate inputs and outputs, including early-return paths.

Changes:

Splits group operations into inline implementations and verification wrappers.
Adds missing field and group-element checks.
Exposes declarations for inverse-Z conversion helpers.
</details>

<details> <summary>File summaries</summary>

File	Description
`src/group.h`	Declares inverse-Z conversion helpers.
`src/group_impl.h`	Adds verification wrappers around group operations.

</details>

<details> <summary>Review details</summary>

Files reviewed: 2/2 changed files
Comments generated: 0
Review effort level: Balanced
</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>

theStack merged this on Sep 9, 2026

theStack closed this on Sep 9, 2026