Review if ECDH is still experimental #665

1. 
   ysangkok commented at 8:50 pm on September 19, 2019: none

   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.

2. 
   jonasnick commented at 5:58 pm on October 7, 2019: contributor

   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.

3. 
   sipa commented at 7:38 pm on January 7, 2020: contributor

   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.
4. 
   gmaxwell commented at 7:58 pm on January 7, 2020: contributor

   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.
5. 
   gmaxwell commented at 3:12 pm on January 8, 2020: contributor

   See #709.
6. 
   jonasnick commented at 8:11 pm on April 17, 2020: contributor

   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.
7. jonasnick added this to the milestone initial release on Apr 28, 2020
8. jonasnick cross-referenced this on Sep 8, 2020 from issue Stop treating ECDH as experimental by jonasnick
9. 
   real-or-random commented at 11:22 am on September 14, 2020: contributor

   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.

10. real-or-random closed this on Oct 21, 2020

    ---

Contributors
ysangkok jonasnick sipa gmaxwell real-or-random

Milestone
stable release (1.0.0-rc.1)