Pass privkey export DER compression flag correctly#14195
Pass privkey export DER compression flag correctly#14195fingera wants to merge 1 commit intobitcoin:masterfrom
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 :-) |
src/key.cpp
Outdated
There was a problem hiding this comment.
src/key.cpp:173:95: warning: implicit conversion bool -> 'int' [readability-implicit-bool-conversion]
src/key.cpp
Outdated
There was a problem hiding this comment.
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.
Huh, apparently @practicalswift told you to do this.
Can you explain @practicalswift ?
There was a problem hiding this comment.
@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; |
fingera
force-pushed
the
5-fix-get-priv
branch
from
67e17da to
575b403
Compare
laanwj
changed the title
|
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
6 participants
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