Enable libsecp256k1 ecdh module, add ECDH function to CKey #14049

pull jonasschnelli wants to merge 2 commits into bitcoin:master from jonasschnelli:2018/08/bip151_ecdh changing 4 files +36 −1

jonasschnelli commented at 2:52 PM on August 24, 2018: contributor

This add ComputeECDHSecret(const CPubKey& pubkey, CPrivKey& secret_out) to CKey (including a test).

This is a subset of #14032 and a pre-requirement for BIP324.
in src/test/key_tests.cpp:173 in 4a97faa231 outdated
```
 156 | +    CPrivKey secret2;
 157 | +    BOOST_CHECK(key1.ComputeECDHSecret(pubkey2, secret1));
 158 | +    BOOST_CHECK(key2.ComputeECDHSecret(pubkey1, secret2));
 159 | +    BOOST_CHECK(secret1.size() == 32);
 160 | +    BOOST_CHECK(secret1 == secret2);
 161 |  }
```
gmaxwell commented at 4:29 PM on August 24, 2018:

Needs a test where the pubkey doesn't decode.

jonasschnelli commented at 11:58 AM on August 27, 2018:

Added

gmaxwell commented at 4:02 PM on August 27, 2018:

Thanks but please also add one that could actually have come in over the wire for BIP151, -- the first byte isn't sent there. What I'm hoping we check for is that it correctly rejects points that aren't on the curve here, rather than performing operations on the twist.

Sjors commented at 9:03 AM on September 2, 2018:

The new pubkeydata[9] = 0xFF; line checks that, right? Maybe add a comment to clarify?
in src/key.cpp:297 in 4a97faa231 outdated
```
 292 | +    if (!secp256k1_ec_pubkey_parse(secp256k1_context_sign, &pubkey_internal, pubkey.data(), pubkey.size())) {
 293 | +        return false;
 294 | +    }
 295 | +
 296 | +    secret_out.resize(32);
 297 | +    return secp256k1_ecdh(secp256k1_context_sign, &secret_out[0], &pubkey_internal, &keydata[0]) == 1;
```
Empact commented at 11:38 PM on August 24, 2018:

Is it better to pass a SECP256K1_CONTEXT_NONE context, given the context is not currently used by these functions?

gmaxwell commented at 4:32 AM on August 25, 2018:

That would require creating a dummy context just for this purpose, so I don't think that would be best.

l2a5b1 commented at 7:24 AM on August 26, 2018:

Maybe replace the &keydata[0] argument with keydata.data()?

l2a5b1 commented at 7:34 AM on August 26, 2018:

Maybe rename secp256k1_context_sign to secp256k1_context?

jonasschnelli commented at 11:58 AM on August 27, 2018:

Fixed.
in src/key.h:132 in 4a97faa231 outdated
```
 127 | @@ -128,6 +128,9 @@ class CKey
 128 |      //! Derive BIP32 child key.
 129 |      bool Derive(CKey& keyChild, ChainCode &ccChild, unsigned int nChild, const ChainCode& cc) const;
 130 |  
 131 | +    // computes an EC Diffie-Hellman 256bit secret
 132 | +    bool ComputeECDHSecret(const CPubKey& pubkey, CPrivKey& secret_out) const;
```
l2a5b1 commented at 7:24 AM on August 26, 2018:

Extract ComputeECDHSecret from CKey to make it a freestanding function?

bool ComputeECDHSecret(const CKey& key, const CPubKey& pubkey, CPrivKey& secret_out);

Although it is convenient to add new BIP responsibilities directly to CKey, I would recommend against adding more responsibilities to CKey than it already has.
jonasschnelli commented at 9:04 AM on September 27, 2018:

I think this makes sense to be a member function (in align with Derive and other member functions of CKey).
l2a5b1 commented at 12:30 PM on September 27, 2018:

"A function that can be simply, elegantly, and efficiently implemented as a freestanding function (that is, as a nonmember function) should be implemented outside the class." (Stroustrup) is a very commonly used and well documented best practice in C++ development.

See C.4. Make a function a member only if it needs direct access to the representation of a class for more info and guidelines.

bool ComputeECDHSecret(const CKey& key, const CPubKey& pubkey, CPrivKey& secret_out) { secp256k1_pubkey pubkey_internal; if (!secp256k1_ec_pubkey_parse(secp256k1_context_sign, &pubkey_internal, pubkey.data(), pubkey.size())) { return false; } secret_out.resize(32); return secp256k1_ecdh(secp256k1_context_sign, &secret_out[0], &pubkey_internal, key.begin()) == 1; }
Enable libsecp256k1 ecdh module, add ecdh function to CKey e2a640e0e7
jonasschnelli force-pushed on Aug 27, 2018
QA: Add EC Diffie-Hellman test 7ab854a23e
jonasschnelli force-pushed on Aug 28, 2018
jonasschnelli referenced this in commit 955ac1d876 on Aug 29, 2018
in configure.ac:1435 in 7ab854a23e
```
1431 | @@ -1432,7 +1432,7 @@ if test x$need_bundled_univalue = xyes; then
1432 |    AC_CONFIG_SUBDIRS([src/univalue])
1433 |  fi
1434 |  
1435 | -ac_configure_args="${ac_configure_args} --disable-shared --with-pic --with-bignum=no --enable-module-recovery --disable-jni"
1436 | +ac_configure_args="${ac_configure_args} --disable-shared --with-pic --with-bignum=no --enable-module-recovery --enable-experimental --enable-module-ecdh --disable-jni"
```
Sjors commented at 9:04 AM on September 2, 2018:

Is this --enable-experimental "permanent"? Perhaps this change should be behind a ./configure --enable-experimental-ecdh, or is overly pedantic?

jonasschnelli commented at 9:03 AM on September 27, 2018:

It's an upstream question. Should be asked at https://github.com/bitcoin-core/secp256k1

Sjors commented at 10:15 AM on September 28, 2018:

It's declared experimental upstream. Asking upstream to no longer declare it experimental is an upstream question. My point is about using it here; as long as it's experimental upstream, shouldn't it be considered experimental here?

jonasschnelli commented at 1:20 PM on August 13, 2019:

I think it's best to not hide it behind a configuration option on our side. I think we should always compile BIP324 features but only activate them when passing a startup argument (can be switched to default later)
succotash approved
DrahtBot added the label Build system on Mar 19, 2019
DrahtBot added the label Tests on Mar 19, 2019
DrahtBot added the label Utils/log/libs on Mar 19, 2019
DrahtBot closed this on May 7, 2019
DrahtBot commented at 5:06 PM on May 7, 2019: member

The last travis run for this pull request was 252 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.
DrahtBot reopened this on May 7, 2019
DrahtBot commented at 9:09 AM on August 9, 2019: member


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #16573 (build: disable building libsecp256k1 benchmarks by fanquake)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
DrahtBot commented at 8:48 PM on August 9, 2019: member

Needs rebase
DrahtBot added the label Needs rebase on Aug 9, 2019
dongcarl commented at 4:26 PM on June 2, 2020: member

Rebased this PR for you :relaxed:: https://github.com/bitcoin/bitcoin/compare/master...dongcarl:2020-06-bip151_ecdh-rebased
fanquake commented at 12:24 PM on August 18, 2021: member

My understanding is that someone else is helping with / taking over these changes, and that the BIP is still being overhauled. I think we'll be better off with new PRs, and clean discussion when work on the implementation resumes in this repo. Changes from here are be cherry-picked if / when needed. So I'm going to close this PR for now.
fanquake closed this on Aug 18, 2021
dhruv added this to the "Closed unmerged" column in a project