Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass privkey export DER compression flag correctly #14195

Closed
wants to merge 1 commit into from

Conversation

@fingera
Copy link
Contributor

@fingera fingera commented Sep 11, 2018

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
Copy link
Contributor Author

@fingera fingera commented Sep 11, 2018

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

@Empact
Copy link
Member

@Empact Empact commented Sep 11, 2018

Looks like a bug to me...

@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Sep 11, 2018

Note to reviewers: This pull request conflicts with the following ones:
  • #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
Copy link
Member

@achow101 achow101 commented Sep 11, 2018

Please make your description and title more descriptive.

@fingera fingera changed the title is this a bug? fix export privkey der always compressed Sep 11, 2018
@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Sep 11, 2018

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

Related: #10041 and #10043

@fingera
Copy link
Contributor Author

@fingera fingera commented Sep 11, 2018

You found out before, why not fix it?

@fingera fingera closed this Sep 11, 2018
@fingera
Copy link
Contributor Author

@fingera fingera commented Sep 11, 2018

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented Sep 11, 2018

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

@fingera fingera reopened this Sep 11, 2018
@@ -170,7 +170,7 @@ CPrivKey CKey::GetPrivKey() const {
size_t privkeylen;
privkey.resize(PRIVATE_KEY_SIZE);
privkeylen = PRIVATE_KEY_SIZE;
ret = ec_privkey_export_der(secp256k1_context_sign, privkey.data(), &privkeylen, begin(), fCompressed ? SECP256K1_EC_COMPRESSED : SECP256K1_EC_UNCOMPRESSED);
ret = ec_privkey_export_der(secp256k1_context_sign, privkey.data(), &privkeylen, begin(), fCompressed);
Copy link
Contributor

@practicalswift practicalswift Sep 11, 2018

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

src/key.cpp Outdated
@@ -170,7 +170,7 @@ CPrivKey CKey::GetPrivKey() const {
size_t privkeylen;
privkey.resize(PRIVATE_KEY_SIZE);
privkeylen = PRIVATE_KEY_SIZE;
ret = ec_privkey_export_der(secp256k1_context_sign, privkey.data(), &privkeylen, begin(), fCompressed ? SECP256K1_EC_COMPRESSED : SECP256K1_EC_UNCOMPRESSED);
ret = ec_privkey_export_der(secp256k1_context_sign, privkey.data(), &privkeylen, begin(), fCompressed ? 1 : 0);
Copy link
Member

@laanwj laanwj Sep 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

Copy link
Member

@laanwj laanwj Sep 12, 2018

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

Copy link
Contributor

@practicalswift practicalswift Sep 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
Copy link
Member

@laanwj laanwj commented Sep 12, 2018

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
Copy link
Contributor

@practicalswift practicalswift commented Sep 12, 2018

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

@laanwj
Copy link
Member

@laanwj laanwj commented Sep 12, 2018

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

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

@fingera fingera force-pushed the 5-fix-get-priv branch from 67e17da to 575b403 Sep 12, 2018
@laanwj laanwj changed the title fix export privkey der always compressed Pass privkey export DER compression flag correctly Sep 13, 2018
@laanwj
Copy link
Member

@laanwj laanwj commented Sep 13, 2018

merged via 9a565a8

@laanwj laanwj closed this Sep 13, 2018
pull bot pushed a commit to jaschadub/bitcoin that referenced this issue Sep 13, 2018
9a565a8 Pass export privkey DER compression flag correctly (liuyujun)

Tree-SHA512: 3fa1d7568ce133cd22708f453f3252c1138b1c40a821e90546d83bee4aac117ac8d848fa46cb45efad1031ce03cd5ba2d6c89b459abca6703aa2a957531e7edf
@fingera fingera deleted the 5-fix-get-priv branch Sep 13, 2018
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Nov 10, 2020
Summary:
By passing a bitfield where a boolean was expected, the result was
always compressed. Fix this.

This is a backport of Core [[bitcoin/bitcoin#14195 | PR14195]]

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D8344
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Jun 27, 2021
9a565a8 Pass export privkey DER compression flag correctly (liuyujun)

Tree-SHA512: 3fa1d7568ce133cd22708f453f3252c1138b1c40a821e90546d83bee4aac117ac8d848fa46cb45efad1031ce03cd5ba2d6c89b459abca6703aa2a957531e7edf
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Jun 28, 2021
9a565a8 Pass export privkey DER compression flag correctly (liuyujun)

Tree-SHA512: 3fa1d7568ce133cd22708f453f3252c1138b1c40a821e90546d83bee4aac117ac8d848fa46cb45efad1031ce03cd5ba2d6c89b459abca6703aa2a957531e7edf
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Jun 29, 2021
9a565a8 Pass export privkey DER compression flag correctly (liuyujun)

Tree-SHA512: 3fa1d7568ce133cd22708f453f3252c1138b1c40a821e90546d83bee4aac117ac8d848fa46cb45efad1031ce03cd5ba2d6c89b459abca6703aa2a957531e7edf
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this issue Jul 1, 2021
9a565a8 Pass export privkey DER compression flag correctly (liuyujun)

Tree-SHA512: 3fa1d7568ce133cd22708f453f3252c1138b1c40a821e90546d83bee4aac117ac8d848fa46cb45efad1031ce03cd5ba2d6c89b459abca6703aa2a957531e7edf
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants