Enable libsecp256k1 ecdh module, add ECDH function to CKey

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?

0bool 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.

0bool ComputeECDHSecret(const CKey& key, const CPubKey& pubkey, CPrivKey& secret_out)
1{
2    secp256k1_pubkey pubkey_internal;
3    if (!secp256k1_ec_pubkey_parse(secp256k1_context_sign, &pubkey_internal, pubkey.data(), pubkey.size())) {
4        return false;
5    }
6
7    secret_out.resize(32);
8    return secp256k1_ecdh(secp256k1_context_sign, &secret_out[0], &pubkey_internal, key.begin()) == 1;
9}

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

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

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

DrahtBot locked this on Oct 31, 2022

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

Conflicts