use variable-time group addition in _ec_pubkey_combine #1587

1. 
   theStack commented at 5:02 pm on August 14, 2024: contributor

   Unless I’m missing something, it seems that there is no need to use constant-time group addition for secp256k1_ec_pubkey_combine, so the faster variable-time addition function _gej_add_ge_var can be used instead. Happy to add a benchmark if wanted.
2. use variable-time group addition in `_ec_pubkey_combine` 319ff2cde5
3. real-or-random added the label performance on Aug 17, 2024
4. real-or-random added the label refactor/smell on Aug 17, 2024
5. Bigheem approved
6. 
   jonasnick commented at 1:33 pm on September 3, 2024: contributor

   I don’t know what the original intention of using constant time addition was. Maybe @sipa remembers. It was added here: https://github.com/bitcoin-core/secp256k1/pull/212/files#diff-6f71b0372be086d45b4f2740508c03a21835d87008840032fbb767f419fd988aR552

   I’m aware that some implementations use pubkey_combine to add “secret” group elements in a “blind DH” (see here, here and here). I haven’t checked in detail if using variable time addition leads to a sidechannel that isn’t already there with constant time addition. In any case, constant time addition is not something one can expect from libsecp’s API (it’s called “public key combine”).

7. 
   real-or-random commented at 1:30 pm on September 25, 2024: contributor

   I don’t know what the original intention of using constant time addition was. Maybe @sipa remembers. It was added here: #212 (files)

   The function was added for key aggregation in multi-signatures (predating MuSig(2)). Variable-time group addition was available back then.

   Perhaps the idea was that the individual keys to be aggregated are secrets. One can make that point (also in MuSig), but I think our current implicit policy is that only secret keys are secret when it comes to timing (and public keys are, well, public).

   I’m aware that some implementations use pubkey_combine to add “secret” group elements in a “blind DH” (see here, here and here). I haven’t checked in detail if using variable time addition leads to a sidechannel that isn’t already there with constant time addition. In any case, constant time addition is not something one can expect from libsecp’s API (it’s called “public key combine”).

   Hm, yeah. I suggest not bothering with these functions for now. I think our long-term plan is to deprecate them and replace them with a properly designed hazmat API (see for example #1471 (comment)). No matter, if we’ll do this in the future or not, it’s not a big deal for performance if this function stays constant-time.

8. 
   theStack commented at 2:49 pm on September 25, 2024: contributor

   Hm, yeah. I suggest not bothering with these functions for now. I think our long-term plan is to deprecate them and replace them with a properly designed hazmat API (see for example #1471 (comment)). No matter, if we’ll do this in the future or not, it’s not a big deal for performance if this function stays constant-time.

   Makes sense! Closing this PR accordingly.

9. theStack closed this on Sep 25, 2024

   ---

10. theStack deleted the branch on Sep 25, 2024

Contributors
theStack Bigheem jonasnick real-or-random

Labels
performance refactor/smell