Review if ECDH is still experimental

[ysangkok]

ECDH is needed for Lightning, and it would be convenient to use the implementation here.

ECDH was marked as experimental back in 2015, and the API was last changed more than a year ago.

I am wondering how I can help make the ECDH implementation considered stable.

[jonasnick]

This is a good suggestion and I agree that it would be really useful if the ECDH module would graduate from being experimental. It's already used by a few projects (elements, clightning, ...) but if they depend on an experimental module, they can be viewed themselves as experimental. But at some point the software used in the Bitcoin space must mature beyond that.

One thing that marking the module as experimental is expected to do a moderately good job at is scaring developers not familiar with applied cryptography away from using low-level primitives like ECDH. However, "experimental" and "unsafe-if-used-incorrectly" are different qualities. See also the discussion in #352.

API-wise the module seems relatively stable. There's #262, #352 and #363, which would require a different API, but as far as I can see they could be added as additional functions instead of changing the existing secp256k1_ecdh arguments.

Most importantly though, I don't know how much the code was reviewed. While the ECDH module is very small, it uses ecmult_const which isn't used by anything else. I have not read it. In particular, it would be important that the function is actually constant-time, which could be investigated with tools like ctgrind or sidefuzz in addition to code review.

[sipa]

I think it's time to make the ECDH non-experimental. For me the biggest hurdle was the question whether the API would remain stable over time, but since #354 I think we're good.

[gmaxwell]

Has anyone attempted yet verify that its timing doesn't depend on the secret (I mean in operation, rather than by just reading the code)? If not, I can do this.

[gmaxwell]

See #709.

[jonasnick]

I checked that the ecmult_const tests are no less rigorous than the ecmult and ecmult_gen tests. ecmult_const is exhaustively tested on the small group. Moreover, the tests achieve full branch coverage with #741. As far as I know there is nothing really standing in the way of making the ECDH module non-experimental.

[real-or-random]

API-wise the module seems relatively stable. There's #262, #352 and #363, which would require a different API, but as far as I can see they could be added as additional functions instead of changing the existing secp256k1_ecdh arguments.

We should think about closing these PRs if we're currently not interested in them. I don't think people care about ECDH performance enough to switch to a different variant.