-
Notifications
You must be signed in to change notification settings - Fork 36.2k
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
Conversation
Is it changing ec_privkey_export_der? Delete another branch that will never be reached |
Looks like a bug to me... |
Note to reviewers: 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. |
Please make your description and title more descriptive. |
You found out before, why not fix it? |
@fingera Please re-open the PR: this is still confusing and hence unresolved :-) |
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, apparently @practicalswift told you to do this.
Can you explain @practicalswift ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 :-)
utACK (after squash), this clearly fixes a bug, |
utACK 67e17da34de278c0b0bab0caad0ad877a053b0d6 modulo squash and a more informative commit message :-) |
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; |
67e17da
to
575b403
Compare
merged via 9a565a8 |
9a565a8 Pass export privkey DER compression flag correctly (liuyujun) Tree-SHA512: 3fa1d7568ce133cd22708f453f3252c1138b1c40a821e90546d83bee4aac117ac8d848fa46cb45efad1031ce03cd5ba2d6c89b459abca6703aa2a957531e7edf
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
9a565a8 Pass export privkey DER compression flag correctly (liuyujun) Tree-SHA512: 3fa1d7568ce133cd22708f453f3252c1138b1c40a821e90546d83bee4aac117ac8d848fa46cb45efad1031ce03cd5ba2d6c89b459abca6703aa2a957531e7edf
9a565a8 Pass export privkey DER compression flag correctly (liuyujun) Tree-SHA512: 3fa1d7568ce133cd22708f453f3252c1138b1c40a821e90546d83bee4aac117ac8d848fa46cb45efad1031ce03cd5ba2d6c89b459abca6703aa2a957531e7edf
9a565a8 Pass export privkey DER compression flag correctly (liuyujun) Tree-SHA512: 3fa1d7568ce133cd22708f453f3252c1138b1c40a821e90546d83bee4aac117ac8d848fa46cb45efad1031ce03cd5ba2d6c89b459abca6703aa2a957531e7edf
9a565a8 Pass export privkey DER compression flag correctly (liuyujun) Tree-SHA512: 3fa1d7568ce133cd22708f453f3252c1138b1c40a821e90546d83bee4aac117ac8d848fa46cb45efad1031ce03cd5ba2d6c89b459abca6703aa2a957531e7edf
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