Fix bug in CKey DER encoding#10043
Conversation
| if (!ec_privkey_import_der(secp256k1_context_sign, (unsigned char*)begin(), &privkey[0], privkey.size())) | ||
| return false; | ||
| fCompressed = fCompressedIn; | ||
| assert(privkey[1] == (fCompressed ? 0x81 : 0x82)); |
There was a problem hiding this comment.
Not sure if this is correct and if we should abort if this the assumption is not true.
There was a problem hiding this comment.
Looks like we do not call this function at all anyway...
|
I am quite surprised we even export private key in DER, I never had a need for it. Why is it used ? |
|
AFAIK the only place where we use DER private keys is when storing/retrieving from the wallet.dat BDB database. We should probably change that, but reading DER must be supported at least. |
|
Thanks for the fix! |
I'm not entirely sure. I haven't analysed the full ramification. At the moment, I'm not sure what happens if you import an uncompressed WIF priv key. Or what happened when we introduced the bug with already existing uncompressed DER encoded keys in the wallet. |
|
(as was discussed in the last meeting) This has no effect, we never read it. It's just idiocy in the format that a public key is saved inside the private key at all. I think it should just always set compressed here. We use the public key outside of the private key for everything interesting. |
|
OK so this is not really a bug, just a peculiarity of our data format. |
Yes. Agree. We could optimise this out maybe together with the new, non backward compatible HD internal/external database format. |
|
Closing |
6 participants
At the moment, due to a mistake in our code, we DER export uncompressed keys as compressed.
Addresses #10041.