Pass privkey export DER compression flag correctly

fingera commented at 1:29 am on September 11, 2018: contributor

The argument to ec_privkey_export_der is BOOLEAN GetPrivKey call ec_privkey_export_der to use the flag

SECP256K1_EC_COMPRESSED is true SECP256K1_EC_UNCOMPRESSED is true

fingera commented at 1:38 am on September 11, 2018: contributor

Is it changing ec_privkey_export_der? Delete another branch that will never be reached

Empact commented at 3:28 am on September 11, 2018: member

Looks like a bug to me…

DrahtBot commented at 3:39 am on September 11, 2018: member

#12461 (scripted-diff: Rename key size consts to be relative to their class by Empact)

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.

achow101 commented at 4:20 am on September 11, 2018: member

Please make your description and title more descriptive.

fingera renamed this:
~~is this a bug?~~
fix export privkey der always compressed
on Sep 11, 2018

practicalswift commented at 7:48 am on September 11, 2018: contributor

@fingera Nice first-time contribution! Out of curiosity, how did you find this? :-)

Related: #10041 and #10043

fingera commented at 8:14 am on September 11, 2018: contributor

You found out before, why not fix it?

fingera closed this on Sep 11, 2018

fingera commented at 8:22 am on September 11, 2018: contributor

@practicalswift https://github.com/fingera/fingera_libc/blob/master/tests/btc/key.cpp

practicalswift commented at 8:26 am on September 11, 2018: contributor

@fingera Please re-open the PR: this is still confusing and hence unresolved :-)

fingera reopened this on Sep 11, 2018

in src/key.cpp:173 in 4a15844bda outdated

169@@ -170,7 +170,7 @@ CPrivKey CKey::GetPrivKey() const {
170     size_t privkeylen;
171     privkey.resize(PRIVATE_KEY_SIZE);
172     privkeylen = PRIVATE_KEY_SIZE;
173-    ret = ec_privkey_export_der(secp256k1_context_sign, privkey.data(), &privkeylen, begin(), fCompressed ? SECP256K1_EC_COMPRESSED : SECP256K1_EC_UNCOMPRESSED);
174+    ret = ec_privkey_export_der(secp256k1_context_sign, privkey.data(), &privkeylen, begin(), fCompressed);

practicalswift commented at 12:08 pm on September 11, 2018:

0src/key.cpp:173:95: warning: implicit conversion bool -> 'int' [readability-implicit-bool-conversion]

in src/key.cpp:173 in 67e17da34d outdated

169@@ -170,7 +170,7 @@ CPrivKey CKey::GetPrivKey() const {
170     size_t privkeylen;
171     privkey.resize(PRIVATE_KEY_SIZE);
172     privkeylen = PRIVATE_KEY_SIZE;
173-    ret = ec_privkey_export_der(secp256k1_context_sign, privkey.data(), &privkeylen, begin(), fCompressed ? SECP256K1_EC_COMPRESSED : SECP256K1_EC_UNCOMPRESSED);
174+    ret = ec_privkey_export_der(secp256k1_context_sign, privkey.data(), &privkeylen, begin(), fCompressed ? 1 : 0);

laanwj commented at 8:30 am on September 12, 2018:

fCompressed is a C++ bool, this gets coerced to 1 or 0 automatically; so you can leave out the ? 1 : 0.

the type bool can be converted to int with the value false becoming 0 and true becoming 1. https://en.cppreference.com/w/cpp/language/implicit_conversion

laanwj commented at 8:31 am on September 12, 2018:

Huh, apparently @practicalswift told you to do this. Can you explain @practicalswift ?

practicalswift commented at 8:46 am on September 12, 2018:

@laanwj Explicit is better than implicit :-) See https://clang.llvm.org/extra/clang-tidy/checks/readability-implicit-bool-conversion.html for a more technical rationale :-)

laanwj commented at 8:33 am on September 12, 2018: member

utACK (after squash), this clearly fixes a bug, ec_privkey_export_der does not take a bit field (secp256k1_ec_pubkey_serialize is what takes SECP256K1_EC_COMPRESSED=258 and SECP256K1_EC_UNCOMPRESSED=2). I’m surprised none of the tests catches this.

practicalswift commented at 8:51 am on September 12, 2018: contributor

utACK 67e17da34de278c0b0bab0caad0ad877a053b0d6 modulo squash and a more informative commit message :-)

laanwj commented at 8:57 am on September 12, 2018: member

suggested on IRC was, to prevent the bug as well as the conversion warning without using really contorted C++:

 0diff --git a/src/key.cpp b/src/key.cpp
 1index df452cd3302ee6aff363b8fc8ea328b69d8bfb55..80d6589a3c36b823402b81d759ff43520037c43a 100644
 2--- a/src/key.cpp
 3+++ b/src/key.cpp
 4@@ -89,7 +89,7 @@ static int ec_privkey_import_der(const secp256k1_context* ctx, unsigned char *ou
 5  * will be set to the number of bytes used in the buffer.
 6  * key32 must point to a 32-byte raw private key.
 7  */
 8-static int ec_privkey_export_der(const secp256k1_context *ctx, unsigned char *privkey, size_t *privkeylen, const unsigned char *key32, int compressed) {
 9+static int ec_privkey_export_der(const secp256k1_context *ctx, unsigned char *privkey, size_t *privkeylen, const unsigned char *key32, bool compressed) {
10     assert(*privkeylen >= CKey::PRIVATE_KEY_SIZE);
11     secp256k1_pubkey pubkey;
12     size_t pubkeylen = 0;
13@@ -170,7 +170,7 @@ CPrivKey CKey::GetPrivKey() const {
14     size_t privkeylen;
15     privkey.resize(PRIVATE_KEY_SIZE);
16     privkeylen = PRIVATE_KEY_SIZE;
17-    ret = ec_privkey_export_der(secp256k1_context_sign, privkey.data(), &privkeylen, begin(), fCompressed ? SECP256K1_EC_COMPRESSED : SECP256K1_EC_UNCOMPRESSED);
18+    ret = ec_privkey_export_der(secp256k1_context_sign, privkey.data(), &privkeylen, begin(), fCompressed);
19     assert(ret);
20     privkey.resize(privkeylen);
21     return privkey;

fix GetPrivKey 575b403d74

fingera force-pushed on Sep 12, 2018

laanwj renamed this:
~~fix export privkey der always compressed~~
Pass privkey export DER compression flag correctly
on Sep 13, 2018

laanwj commented at 8:03 am on September 13, 2018: member

merged via 9a565a8282236f29334a6ec2de6a03358f5ce86e

laanwj closed this on Sep 13, 2018

pull[bot] referenced this in commit 81b3f3c7a3 on Sep 13, 2018

fingera deleted the branch on Sep 13, 2018

deadalnix referenced this in commit 4af6f8885c on Nov 10, 2020

PastaPastaPasta referenced this in commit ed22e454ef on Jun 27, 2021

PastaPastaPasta referenced this in commit 37ce0af0e2 on Jun 28, 2021

PastaPastaPasta referenced this in commit 65eeedecb1 on Jun 29, 2021

PastaPastaPasta referenced this in commit cdcd166819 on Jul 1, 2021

MarcoFalke locked this on Sep 8, 2021

Pass privkey export DER compression flag correctly #14195