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

Scalar multiplication with the generator point frequently involves a conversion to affine coordinates and clearing out the temporary Jacobian group element object after to avoid leaking secret key material (see 765ef53335a3e0fafdafe1e757f6fe0789f2797f / #1579 for that last part), i.e. executing the following three functions:

• secp256k1_ecmult_gen(ctx, &rj, ...)
• secp256k1_ge_set_gej(&r, &rj)
• secp256k1_gej_clear(&rj)

This PR introduces a corresponding helper 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 code paths for ECDSA signing, Schnorr signing, public key creation and ecmult_gen blinding setup. The only remaining instance where we directly call _ecmult_gen is for musig nonce generation, as we apply batch inversion there for the two points.

The idea came up during a conversation with furszy, who caught that the gej clearing was missing in the silentpayments module (sending function) as well (see #1765 (comment), b10c mirror link).

If this gets conceptual support, I'd be curious to hear naming suggestions, as I'm not sure if the current one is fits well to the existing terminology (maybe ecmult_gen_ge or ecmult_gen_to_affine?).

Concept ACK

If this gets conceptual support, I'd be curious to hear naming suggestions, as I'm not sure if the current one is fits well to the existing terminology (maybe ecmult_gen_ge or ecmult_gen_to_affine?).

I think ge_ecmult_gen is in line with the naming of the other group functions. (We could also consider renaming the existing one to gej_ecmult_gen.)

If this gets conceptual support, I'd be curious to hear naming suggestions, as I'm not sure if the current one is fits well to the existing terminology (maybe ecmult_gen_ge or ecmult_gen_to_affine?).

I think ge_ecmult_gen is in line with the naming of the other group functions. (We could also consider renaming the existing one to gej_ecmult_gen.)

Thanks, done. I went ahead and did the proposed _ecmult_gen -> _gej_ecmult_gen rename as well in a second commit (it can easily be dropped if considered out-of-scope; not sure what's a good stopping point for this PR, one could in theory even go further and rename the other ecmult_ functions to include the gej prefix as well...).

Thanks for the feedback.

I agree that naming consistency with existing group functions is important. I’m fine with using ge_ecmult_gen if that better matches the current terminology, and the renaming commit can be dropped if preferred.

I tried to keep the scope limited and avoided broader renaming for now, but I’m happy to adjust based on maintainers’ preference.

I think ge_ecmult_gen is in line with the naming of the other group functions. (We could also consider renaming the existing one to gej_ecmult_gen.)

Now that I'm looking at this again, I start to see the drawback of this: These functions live in the ecmult module but now they have a ge_ or gej_ prefix which is usually reserved for the group module.

What about a) moving the ge_ecmult_gen helper to group and b) keeping ecmult_gen as is in ecmult but re-exporting it in group as gej_ecmult_gen_, e.g., using a #define? This would keep the identifier<->filename naming convention but adds a bit of complexity due to the re-export. And it makes ecmult a submodule of group (philosophically), and it I think this makes sense.

Sorry, all of this is bike shedding. But at least I'd like to hear more opinions here given that we're renaming these fundamental functions.

Now that I'm looking at this again, I start to see the drawback of this: These functions live in the ecmult module but now they have a ge_ or gej_ prefix which is usually reserved for the group module.

Ah that's a good point indeed!

What about a) moving the ge_ecmult_gen helper to group and b) keeping ecmult_gen as is in ecmult but re-exporting it in group as gej_ecmult_gen_, e.g., using a #define? This would keep the identifier<->filename naming convention but adds a bit of complexity due to the re-export. And it makes ecmult a submodule of group (philosophically), and it I think this makes sense.

I've tried the a) part now and it seems that this would introduce a circular dependency between the group and ecmult_gen modules. Due to needing the secp256k1_ecmult_gen_context struct type, group.h would need to include ecmult_gen.h, which in turn includes group.h again. (Also, we would need to include scalar.h in group.h as well which wasn't needed before, but maybe that part would be fine?).

Sorry, all of this is bike shedding. But at least I'd like to hear more opinions here given that we're renaming these fundamental functions.

No worries, I agree that this shouldn't be rushed. For #1765, the helper would be a nice-to-have for making the sending function a tiny bit smaller (-3 LOC for calculating A = a * G) and thus potentially easier to review, but it's certainly not urgent. I'm changing the PR to draft state until we figure out where to best place the new helper and how to handle (re)naming.