Fix bug in CKey DER encoding #10043

1. 
   jonasschnelli commented at 8:53 am on March 21, 2017: contributor

   At the moment, due to a mistake in our code, we DER export uncompressed keys as compressed. Addresses #10041.
2. CKey::SetPrivKey assert the compressed flag fe5ca6f765
3. Fix bug in CKey DER encoding 85745423b3
4. in src/key.cpp:138 in 85745423b3

   134@@ -135,6 +135,7 @@ bool CKey::SetPrivKey(const CPrivKey &privkey, bool fCompressedIn) {
   135     if (!ec_privkey_import_der(secp256k1_context_sign, (unsigned char*)begin(), &privkey[0], privkey.size()))
   136         return false;
   137     fCompressed = fCompressedIn;
   138+    assert(privkey[1] == (fCompressed ? 0x81 : 0x82));
   

   ---

   ---

   

   jonasschnelli commented at 8:54 am on March 21, 2017:

   Not sure if this is correct and if we should abort if this the assumption is not true.

   ---

   

   paveljanik commented at 7:22 pm on March 21, 2017:

   Looks like we do not call this function at all anyway…
5. fanquake added the label Bug on Mar 21, 2017
6. jonasschnelli renamed this:
   2017/03/fix der comp
   Fix bug in CKey DER encoding
   on Mar 21, 2017
7. 
   NicolasDorier commented at 9:22 am on March 21, 2017: contributor

   I am quite surprised we even export private key in DER, I never had a need for it. Why is it used ?
8. 
   jonasschnelli commented at 9:33 am on March 21, 2017: contributor

   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.
9. 
   laanwj commented at 10:19 am on March 21, 2017: member

   Thanks for the fix! How does this affect users of the wallet or RPC? Does this need backports?
10. 
    jonasschnelli commented at 10:30 am on March 21, 2017: contributor

    How does this affect users of the wallet or RPC? Does this need backports?

    I’m not entirely sure. I haven’t analysed the full ramification. It looks like, that we only use the DER encoding during serialisation to and deserialisation from the database. Internally it should be treated correctly. Walletdump, etc. should be correct.

    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.

11. 
    gmaxwell commented at 1:42 pm on March 29, 2017: contributor

    (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.

12. 
    laanwj commented at 1:15 pm on April 7, 2017: member

    OK so this is not really a bug, just a peculiarity of our data format. I think then that we should close this and keep things as-is (and later on, switch to a better storage format)
13. 
    jonasschnelli commented at 1:31 pm on April 7, 2017: contributor

    OK so this is not really a bug, just a peculiarity of our data format. I think then that we should close this and keep things as-is (and later on, switch to a better storage format)

    Yes. Agree. We could optimise this out maybe together with the new, non backward compatible HD internal/external database format.

14. 
    jonasschnelli commented at 1:31 pm on April 7, 2017: contributor

    Closing
15. jonasschnelli closed this on Apr 7, 2017

    ---

16. DrahtBot locked this on Sep 8, 2021

Contributors
jonasschnelli paveljanik NicolasDorier laanwj gmaxwell

Labels
Bug