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

[real-or-random]

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.

[theStack]

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: 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).

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]

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.

[theStack]

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

[theStack]

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]

Oh indeed, will fix.

[theStack]

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

[real-or-random]

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]

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]

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.

[apoelstra]

ACK af1c969; successfully ran local tests