search

bitcoin-core / secp256k1

Optimized C library for EC operations on curve secp256k1
MIT License
2.06k stars 1k forks source link

use variable-time group addition in `_ec_pubkey_combine` #1587

Closed theStack closed 1 week ago

theStack commented 1 month ago

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.

real-or-random commented 1 week ago

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 https://github.com/bitcoin-core/secp256k1/pull/1471#issuecomment-1967953283). 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.

theStack commented 1 week ago

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.