[wallet] Remove vchDefaultKey and have better first run detection #10952

pull achow101 wants to merge 1 commits into bitcoin:master from achow101:remove-defaultkey changing 7 files +23 −38
  1. achow101 commented at 0:25 am on July 29, 2017: member

    Removes vchDefaultKey which was only used for first run detection. Improves wallet first run detection by checking to see if any keys were read from the database.

    This also fixes a (rather contrived) case where an encrypted non-HD wallet has corruption such that the default key is no longer valid and is loaded into a Core version that supports HD wallets. This causes a runtime exception since a new hd master key is generated as the software believes the wallet file is newly created but cannot add the generated key to the wallet since it is encrypted. I was only able to replicate this error by creating a non-hd wallet, encrypting it, then editing the wallet using db_dump and db_load before loading the wallet with hd enabled. This problem has been reported by two users so it is something that can happen, although that raises the question of “what corrupted the default key”.

    ~P.S. I don’t know what’s up with the whitespace changes. I think my text editor is doing something stupid but I don’t think those are important enough to attempt undoing them.~ Undid those

  2. fanquake added the label Wallet on Jul 29, 2017
  3. achow101 force-pushed on Jul 29, 2017
  4. gmaxwell commented at 9:37 pm on July 29, 2017: contributor

    So you still need to write a default key unless we’re going to bump the wallet version again for this, because otherwise you’ll make it backwards incompatible with software without this patch.

    Otherwise, Concept ACK!

  5. achow101 commented at 9:56 pm on July 29, 2017: member
    How about bump the wallet version again? If there are any other wallet changes that need a version bump, we could do them all at the same time.
  6. gmaxwell commented at 0:41 am on July 30, 2017: contributor
    I would be fine with that but I think it might be easier to get this bugfix merged without it.
  7. TheBlueMatt commented at 2:36 pm on July 30, 2017: member
    Sounds like maybe something that we should look into for 15, then, though I’d really like to look into the source of the corruption here, rather than patching over it.
  8. achow101 force-pushed on Jul 31, 2017
  9. achow101 commented at 6:18 pm on July 31, 2017: member
    @gmaxwell I added WriteDefaultKey back in so that a default key will be written to maintain backwards compatibility. The key will not be read in.
  10. achow101 force-pushed on Jul 31, 2017
  11. achow101 force-pushed on Aug 2, 2017
  12. gmaxwell approved
  13. gmaxwell commented at 7:28 am on August 3, 2017: contributor
    utACK
  14. TheBlueMatt commented at 4:15 pm on August 3, 2017: member
    I really dont think this is the right way to fix this issue. Can we instead ensure that, if the wallet has keys but vchDefaultKey is invalid, an error is printed like “your wallet is corrupted, seems you disk is silently corrupting things”, and then make sure -salvagewallet is able to properly recover from this state?
  15. achow101 force-pushed on Aug 3, 2017
  16. achow101 commented at 11:44 pm on August 3, 2017: member

    I have updated this with a few changes.

    I removed the defaultkey writing again. New wallets will no longer have a default key. If a defaultkey is found in the wallet, it must be valid, otherwise it will exit with a wallet corruption error.

    It turns out that -salvagewallet may have been the cause of all of these issues. -salvagewallet apparently does not write a default key to the wallet, which with an encrypted wallet and with HD enabled, will cause the aforementioned runtime exception and crash. If a user does have a corrupted default key, then -salvagewallet will remove it, as it did in the past.

  17. achow101 force-pushed on Aug 3, 2017
  18. in src/wallet/wallet.cpp:3953 in 8f3ea174a7 outdated 
    3953-            if (!walletInstance->SetAddressBook(walletInstance->vchDefaultKey.GetID(), "", "receive")) {
3954-                InitError(_("Cannot write default address") += "\n");
3955+
3956+        // Generate an address for the address book and to fill the keypool
3957+        CPubKey key;
3958+        if (walletInstance->GetKeyFromPool(key, false)) {
    sipa commented at 2:30 am on August 4, 2017:
    Can you call walletInstance->TopUpKeyPool(); here instead of this entire section? No need to waste a key and an address book entry for this.
    achow101 commented at 3:27 am on August 4, 2017:
    Done
  19. in src/wallet/walletdb.cpp:455 in 8f3ea174a7 outdated 
    446@@ -452,7 +447,13 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
447         }
448         else if (strType == "defaultkey")
449         {
450-            ssValue >> pwallet->vchDefaultKey;
451+            CPubKey vchPubKey;
452+            ssKey >> vchPubKey;
453+            if (!vchPubKey.IsValid())
454+            {
455+                strErr = "Error reading wallet database: Default Key corrupt";
    sipa commented at 2:36 am on August 4, 2017:
    “defaultkey” is not part of IsKeyType, so this failure will just be a non-critical warning upon loading. I assume that’s intentional?
    achow101 commented at 3:28 am on August 4, 2017:
    It should be a critical warning and exit with an error. I have fixed this.
  20. in src/wallet/walletdb.cpp:453 in 8f3ea174a7 outdated 
    446@@ -452,7 +447,13 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
447         }
448         else if (strType == "defaultkey")
449         {
450-            ssValue >> pwallet->vchDefaultKey;
451+            CPubKey vchPubKey;
452+            ssKey >> vchPubKey;
453+            if (!vchPubKey.IsValid())
454+            {
    sipa commented at 2:37 am on August 4, 2017:
    Code style nit: { on the same like as if.
    achow101 commented at 3:27 am on August 4, 2017:
    Done
  21. achow101 force-pushed on Aug 4, 2017
  22. achow101 force-pushed on Aug 4, 2017
  23. MarcoFalke commented at 6:17 am on August 4, 2017: member

    AssertionError in wallet-hd:

    0AssertionError: not(m/0'/0'/0' == m/0'/0'/1')
  24. achow101 force-pushed on Aug 4, 2017
  25. achow101 commented at 11:42 pm on August 4, 2017: member
    Can this be tagged for 0.15.0? It’s a bug fix for a problem with -salvagewallet which would result in further corruption and unusability of people’s wallets.
  26. sipa added this to the milestone 0.15.0 on Aug 5, 2017
  27. sipa commented at 2:26 am on August 5, 2017: member
    Tagged for 0.15, as it fixes a bug when using -savagewallet with encrypted wallets.
  28. TheBlueMatt commented at 8:56 pm on August 6, 2017: member

    Because one of the two cases of corruption in the wild was that the default key was simply deleted, can you also add a if(wallet version < x && !have_default_key)… in the load?

    On August 4, 2017 2:17:43 AM EDT, MarcoFalke notifications@github.com wrote:

    AssertionError in wallet-hd:

    0AssertionError: not(m/0'/0'/0' == m/0'/0'/1')
1
2-- 
3You are receiving this because you commented.
4Reply to this email directly or view it on GitHub:
5https://github.com/bitcoin/bitcoin/pull/10952#issuecomment-320166041
  29. achow101 commented at 0:18 am on August 7, 2017: member
    @TheBlueMatt What for?
  30. TheBlueMatt commented at 1:09 am on August 7, 2017: member
    @achow101 because its a trivial check and its a type of corruption we’ve seen in wallets on the wild…it costs nothing to add a check for it.
  31. sipa commented at 1:16 am on August 7, 2017: member
    @TheBlueMatt The current code requires that if a default key is present, it must be valid. Do you think an extra check is needed to verify that it hasn’t been accidentally deleted?
  32. TheBlueMatt commented at 1:20 am on August 7, 2017: member

    Considering that we’ve seen it in the wild and its clear indication that there is something wrong with your hardware/wallet storage, I’d say it would be prudent.

    On 08/06/17 21:16, Pieter Wuille wrote:

    @TheBlueMatt https://github.com/thebluematt The current code requires that if a default key is present, it must be valid. Do you think an extra check is needed to verify that it hasn’t been accidentally deleted?

    — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/10952#issuecomment-320546207, or mute the thread https://github.com/notifications/unsubscribe-auth/AAnoHoggbwbRS_yma0eQ6gmvVwaxu4YJks5sVmV0gaJpZM4OnMtA.

  33. achow101 commented at 1:36 am on August 7, 2017: member

    @TheBlueMatt I meant what do you want it to check for and error on and why do that check? That below a certain version number you must have a valid default key? If that is the case, that isn’t possible since this PR does not change the version number, thus all wallets will have to have a valid default key.

    Furthermore, I believe the corruption that we are seeing in the wild is a result of -savagewallet not passing the default key through to the new wallet file, not because of hardware corruption or the like.

  34. TheBlueMatt commented at 1:44 am on August 7, 2017: member

    Ahh, OK, I missed the salvagewallet-possible-culprit. PR looks good as-is to me.

    On 08/06/17 21:36, Andrew Chow wrote:

    @TheBlueMatt https://github.com/thebluematt I meant what do you want it to check for and error on? That below a certain version number you must have a valid default key? If that is the case, that isn’t possible since this PR does not change the version number, thus all wallets will have to have a valid default key.

    Furthermore, I believe the corruption that we are seeing in the wild is a result of |-savagewallet| not passing the default key through to the new wallet file, not because of hardware corruption or the like.

    — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/10952#issuecomment-320548013, or mute the thread https://github.com/notifications/unsubscribe-auth/AAnoHh7moN-GrfZywt97DqoUu2eT-Mc6ks5sVmoVgaJpZM4OnMtA.

  35. laanwj commented at 8:20 am on August 7, 2017: member

    Nit: please also remove the "defaultkey" parsing in walletdb.cpp ReadKeyValue

    utACK otherwise

  36. achow101 commented at 8:40 am on August 7, 2017: member
    @laanwj That was left in due address @TheBlueMatt’s concerns about warning users about possible corruption. It was modified so that the default key must either exist and be valid or not exist at all. The reason for this is that if it exists and is not valid, then corruption has occurred and the user will be warned accordingly.
  37. laanwj commented at 8:42 am on August 7, 2017: member
    That’s true, could have used a comment, it now looks a bit silly just throwing away the result. But I agree.
  38. laanwj commented at 10:40 am on August 7, 2017: member

    I tested this with an old wallet and got “Error: Error loading wallet.dat: Wallet corrupted”, will check why. Edit: Log output is:

    02017-08-07 11:42:25 init message: Loading wallet...
12017-08-07 11:42:25 Error: Error loading wallet.dat: Wallet corrupted
2Error: Error loading wallet.dat: Wallet corrupted

    Looks like with this wallet, the IsKeyType(strType) || strType == "defaultkey" check makes LoadWallet return DB_CORRUPT.

  39. in src/wallet/walletdb.cpp:451 in 31577609cf outdated 
    446@@ -452,7 +447,12 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
447         }
448         else if (strType == "defaultkey")
449         {
450-            ssValue >> pwallet->vchDefaultKey;
451+            CPubKey vchPubKey;
452+            ssKey >> vchPubKey;
    laanwj commented at 12:06 pm on August 7, 2017:
    This should be ssValue >> vchPubKey. Otherwise this will fail for every wallet with a default key record! (when you change this, please add a comment too why we’re parsing here and throwing away the result)
    achow101 commented at 5:30 pm on August 7, 2017:
    Oops. Fixed.
  40. jnewbery commented at 5:27 pm on August 7, 2017: member

    Intuitively for me, the logic for determining whether this is a first run should remain in the CWallet::LoadWallet() function. It makes sense for me that CWalletDB remains as a modifier object for the wallet, and simply reads/writes data to the wallet database. Is it possible to leave the first_run logic in CWallet::LoadWallet() (by checking that mapKeys is empty or similar)?

    I’m not all that familiar with wallet code, so I may be wrong about this. I’ll defer to those who have a better understanding of the wallet model.

    I also don’t think this necessarily needs to go into v0.15 if we don’t have time to fully review it. The problems with salvagewallet are long-standing and not made any worse in v0.15.

  41. achow101 force-pushed on Aug 7, 2017
  42. achow101 commented at 5:56 pm on August 7, 2017: member

    Is it possible to leave the first_run logic in CWallet::LoadWallet() (by checking that mapKeys is empty or similar)?

    That would probably be better to do.

    I also don’t think this necessarily needs to go into v0.15 if we don’t have time to fully review it. The problems with salvagewallet are long-standing and not made any worse in v0.15.

    0.13 broke salvagewallet with HD wallets. Anyone who uses salvagewallet on an encrypted wallet is currently going to run into problems just starting their wallet in the first place.

  43. achow101 force-pushed on Aug 7, 2017
  44. jnewbery commented at 6:23 pm on August 7, 2017: member

    0.13 broke salvagewallet with HD wallets.

    To be clear, I’m not arguing against this PR - it’s obviously good and we should do it. My point is that we don’t need to rush it for v0.15. The number of users who haven’t yet upgraded to v0.13, but will upgrade to v0.15 before we release v0.15.1 is going to be ~ zero. Not including the fix in v0.15 doesn’t make the problem any worse.

  45. laanwj commented at 9:05 am on August 8, 2017: member

    Intuitively for me, the logic for determining whether this is a first run should remain in the CWallet::LoadWallet() function.

    I thought about this too and came to the opposite conclusion: wouldn’t the database layer be the foremost authority on whether it just created a new database file, or opened an existing one? I think this PR goes the right direction.

  46. jnewbery commented at 9:59 am on August 8, 2017: member

    I guess it depends on what we mean by ‘first run’. If it’s ’the first time we create a file’, then yes I agree that the database layer is the authority. If it’s ‘a wallet without any of these objects’, then I would have thought the right place would be at the wallet layer.

    But that’s just my intuition. You know this code better than I do, so if you think this is the right direction, I won’t object.

  47. laanwj commented at 10:08 am on August 8, 2017: member

    I guess it depends on what we mean by ‘first run’. If it’s ’the first time we create a file’, then yes I agree that the database layer is the authority. If it’s ‘a wallet without any of these objects’, then I would have thought the right place would be at the wallet layer.

    CWalletDB is not just the low-level database though - it also manages the format. After all it iterates over application-specific key/value pairs in CWalletDB::LoadWallet. So any “application format” checking belongs there too, IMO. Counting “a wallet without any of these objects” belongs naturally in the place where all the objects are iterated over.

    wallet/db.cpp has the low-level, application-independent database handling. If you’d argue that that is an abstraction level too low to put this, I’d agree.

    I also can’t help that all these classes and objects are named confusingly.

    Edit: Anyhow, I wish it had a “format tag” key/value pair that could be instantly checked for instead of the convoluted “does it have any of these objects” checking. But that’s impossible to introduce in a backward-compatible way I guess…

  48. laanwj commented at 10:14 am on August 8, 2017: member

    To be clear, I’m not arguing against this PR - it’s obviously good and we should do it. My point is that we don’t need to rush it for v0.15.

    I’d prefer to drop it for 0.15.0 too, to be honest. It’s not clear that the risk is acceptable to merge this into rc1 last minute.

  49. laanwj added this to the milestone 0.15.1 on Aug 8, 2017
  50. laanwj removed this from the milestone 0.15.0 on Aug 8, 2017
  51. achow101 commented at 4:17 pm on August 8, 2017: member
    I changed the detection back to CWallet::LoadWallet() since I think it is more logical to be there. That was my original plan, I just couldn’t figure out how since I forgot that it has access to mapKeys, etc.
  52. jnewbery commented at 7:10 pm on August 14, 2017: member
    Looks good. Tested ACK b8aa9729a5dbeb883857b14918aa6c6d2b71eea4 (although needs rebase)
  53. jonasschnelli commented at 7:27 pm on August 15, 2017: contributor
    Needs rebase.
  54. achow101 force-pushed on Aug 15, 2017
  55. achow101 commented at 9:07 pm on August 15, 2017: member
    rebased
  56. jnewbery commented at 9:26 pm on August 15, 2017: member
    utACK 1a7bc5aac60a37b6f9f0632546467d716a3fd5e4
  57. sipa commented at 9:45 pm on August 15, 2017: member
    There seems to be a problem in keypool_topup.py.
  58. Remove vchDefaultKey and have better first run detection 
    Removes vchDefaultKey which was only used for first run detection.
Improves wallet first run detection by checking to see if any keys
were read from the database.

This will now also check for a valid defaultkey for backwards
compatibility reasons and to check for any corruption.

Keys will stil be generated on the first one, but there won't be
any shown in the address book as was previously done.
    e53615b443
  59. achow101 force-pushed on Aug 15, 2017
  60. achow101 commented at 10:06 pm on August 15, 2017: member
    Fixed that I think. It’s an off-by-one since default key is no longer created and consuming a key from the keypool.
  61. laanwj commented at 8:06 am on August 16, 2017: member

    since default key is no longer created and consuming a key from the keypool.

    Finally :-)

  62. jonasschnelli commented at 3:21 pm on August 18, 2017: contributor
    Great to see the default key vanish… utACK e53615b443894fb96d5eb885b3812776c1b1033b
  63. laanwj merged this on Aug 18, 2017
  64. laanwj closed this on Aug 18, 2017
  65. laanwj referenced this in commit 262167393d on Aug 18, 2017
  66. achow101 deleted the branch on Aug 29, 2017
  67. laanwj removed this from the milestone 0.15.1 on Sep 5, 2017
  68. laanwj commented at 10:26 pm on September 5, 2017: member
    Removing 0.15.1 milestone, as it can create problems at the moment when opening a new wallet with older versions.
  69. PastaPastaPasta referenced this in commit 1958749b71 on Sep 19, 2019
  70. furszy referenced this in commit 0ca58efcd7 on Sep 30, 2019
  71. furszy referenced this in commit 6637fd18b9 on Sep 30, 2019
  72. random-zebra referenced this in commit ed83481494 on Oct 11, 2019
  73. CaveSpectre11 referenced this in commit be31e6b1d2 on Dec 14, 2019
  74. PastaPastaPasta referenced this in commit e993ab47d4 on Jan 31, 2020
  75. UdjinM6 referenced this in commit 052e7fcffd on Feb 4, 2020
  76. KolbyML referenced this in commit 586440dae6 on Nov 21, 2020
  77. ckti referenced this in commit 6c6ed61a04 on Mar 28, 2021
  78. DrahtBot locked this on Sep 8, 2021


achow101 gmaxwell TheBlueMatt sipa MarcoFalke laanwj jnewbery jonasschnelli

Labels
Wallet

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-05 01:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me