Enable libsecp256k1 ecdh module, add ECDH function to CKey #14049
Conversation
src/test/key_tests.cpp
Outdated
| BOOST_CHECK(key1.ComputeECDHSecret(pubkey2, secret1)); | ||
| BOOST_CHECK(key2.ComputeECDHSecret(pubkey1, secret2)); | ||
| BOOST_CHECK(secret1.size() == 32); | ||
| BOOST_CHECK(secret1 == secret2); | ||
| } |
There was a problem hiding this comment.
Needs a test where the pubkey doesn't decode.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The new pubkeydata[9] = 0xFF; line checks that, right? Maybe add a comment to clarify?
src/key.cpp
Outdated
| } | ||
|
|
||
| secret_out.resize(32); | ||
| return secp256k1_ecdh(secp256k1_context_sign, &secret_out[0], &pubkey_internal, &keydata[0]) == 1; |
There was a problem hiding this comment.
Is it better to pass a SECP256K1_CONTEXT_NONE context, given the context is not currently used by these functions?
There was a problem hiding this comment.
That would require creating a dummy context just for this purpose, so I don't think that would be best.
There was a problem hiding this comment.
Maybe rename secp256k1_context_sign to secp256k1_context?
| @@ -128,6 +128,9 @@ class CKey | |||
| //! Derive BIP32 child key. | |||
| bool Derive(CKey& keyChild, ChainCode &ccChild, unsigned int nChild, const ChainCode& cc) const; | |||
|
|
|||
| // computes an EC Diffie-Hellman 256bit secret | |||
| bool ComputeECDHSecret(const CPubKey& pubkey, CPrivKey& secret_out) const; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think this makes sense to be a member function (in align with Derive and other member functions of CKey).
There was a problem hiding this comment.
"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;
}
jonasschnelli
force-pushed
the
2018/08/bip151_ecdh
branch
from
4a97faa to
0ee936a
Compare
jonasschnelli
force-pushed
the
2018/08/bip151_ecdh
branch
from
0ee936a to
7ab854a
Compare
| @@ -1432,7 +1432,7 @@ if test x$need_bundled_univalue = xyes; then | |||
| AC_CONFIG_SUBDIRS([src/univalue]) | |||
| fi | |||
|
|
|||
| ac_configure_args="${ac_configure_args} --disable-shared --with-pic --with-bignum=no --enable-module-recovery --disable-jni" | |||
| ac_configure_args="${ac_configure_args} --disable-shared --with-pic --with-bignum=no --enable-module-recovery --enable-experimental --enable-module-ecdh --disable-jni" | |||
There was a problem hiding this comment.
Is this --enable-experimental "permanent"? Perhaps this change should be behind a ./configure --enable-experimental-ecdh, or is overly pedantic?
There was a problem hiding this comment.
It's an upstream question. Should be asked at https://github.com/bitcoin-core/secp256k1
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
| 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. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
| Needs rebase |
|
Rebased this PR for you |
|
My understanding is that someone else is helping with / taking over these changes, and that the BIP is still being overhauled. |
This add
ComputeECDHSecret(const CPubKey& pubkey, CPrivKey& secret_out)toCKey(including a test).This is a subset of #14032 and a pre-requirement for BIP324.