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

achow101 · 2017-07-29T00:25:09Z

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

gmaxwell · 2017-07-29T21:37:01Z

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!

achow101 · 2017-07-29T21:56:17Z

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.

gmaxwell · 2017-07-30T00:41:58Z

I would be fine with that but I think it might be easier to get this bugfix merged without it.

TheBlueMatt · 2017-07-30T14:36:30Z

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.

achow101 · 2017-07-31T18:18:37Z

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

gmaxwell

utACK

TheBlueMatt · 2017-08-03T16:15:36Z

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?

achow101 · 2017-08-03T23:44:34Z

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.

sipa · 2017-08-04T02:30:17Z

src/wallet/wallet.cpp

Can you call walletInstance->TopUpKeyPool(); here instead of this entire section? No need to waste a key and an address book entry for this.

sipa · 2017-08-04T02:36:50Z

src/wallet/walletdb.cpp

"defaultkey" is not part of IsKeyType, so this failure will just be a non-critical warning upon loading. I assume that's intentional?

It should be a critical warning and exit with an error. I have fixed this.

sipa · 2017-08-04T02:37:09Z

src/wallet/walletdb.cpp

Code style nit: { on the same like as if.

maflcko · 2017-08-04T06:17:38Z

AssertionError in wallet-hd:

AssertionError: not(m/0'/0'/0' == m/0'/0'/1')

achow101 · 2017-08-04T23:42:14Z

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.

sipa · 2017-08-05T02:26:52Z

Tagged for 0.15, as it fixes a bug when using -savagewallet with encrypted wallets.

TheBlueMatt · 2017-08-06T20:56:26Z

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 ***@***.***> wrote: AssertionError in wallet-hd: ```py AssertionError: not(m/0'/0'/0' == m/0'/0'/1') -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #10952 (comment)

achow101 · 2017-08-07T00:18:10Z

@TheBlueMatt What for?

TheBlueMatt · 2017-08-07T01:09:24Z

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

sipa · 2017-08-07T01:16:32Z

@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?

TheBlueMatt · 2017-08-07T01:20:15Z

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 <#10952 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAnoHoggbwbRS_yma0eQ6gmvVwaxu4YJks5sVmV0gaJpZM4OnMtA>.

achow101 · 2017-08-07T01:36:17Z

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

achow101 · 2017-08-08T16:17:50Z

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.

jnewbery · 2017-08-14T19:10:34Z

Looks good. Tested ACK b8aa9729a5dbeb883857b14918aa6c6d2b71eea4 (although needs rebase)

jonasschnelli · 2017-08-15T19:27:57Z

Needs rebase.

achow101 · 2017-08-15T21:07:42Z

rebased

jnewbery · 2017-08-15T21:26:27Z

utACK 1a7bc5aac60a37b6f9f0632546467d716a3fd5e4

sipa · 2017-08-15T21:45:33Z

There seems to be a problem in keypool_topup.py.

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.

achow101 · 2017-08-15T22:06:33Z

Fixed that I think. It's an off-by-one since default key is no longer created and consuming a key from the keypool.

laanwj · 2017-08-16T08:06:45Z

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

Finally :-)

jonasschnelli · 2017-08-18T15:21:41Z

Great to see the default key vanish...
utACK e53615b

… detection e53615b Remove vchDefaultKey and have better first run detection (Andrew Chow) Pull request description: 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](https://bitcointalk.org/index.php?topic=1993244.0) [users](https://bitcointalk.org/index.php?topic=1746976.msg17511261#msg17511261) 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 Tree-SHA512: 63b485f356566e8ffa033ad9b7101f7f6b56372b29ec2a43b947b0eeb1ada4c2cfe24740515d013aedd5f51aa1890dfbe499d2c5c062fc1b5d272324728a7d55

laanwj · 2017-09-05T22:26:48Z

Removing 0.15.1 milestone, as it can create problems at the moment when opening a new wallet with older versions.

…emove vchDefaultKey and have better first run detection". [Wallet][Startup][DB][Backport] Don't create any default address

…m wallet.h 83d8d00 [Trivial] Init.cpp, instead of use LogPrint, use error. (furszy) 6637fd1 [Wallet][Startup][DB][Backport] bitcoin#10952 BTC back port. Named "Remove vchDefaultKey and have better first run detection". (furszy) Pull request description: Back port from bitcoin#10952. This PR is on top of #1029 because of both PRs modifying `init.cpp`. Check only the last commit. ACKs for top commit: random-zebra: ACK 83d8d00 Warrows: ACK 83d8d00 Fuzzbawls: ACK 83d8d00 Tree-SHA512: 0e05f7f80897a421c541580abbd10af37db083a43c6edf49f61e8d1977c58c6e486b1f17e0eac4fb4e8257200eea3e04b5510321cb4484e110f466f5a5132151

…rst run detection e53615b Remove vchDefaultKey and have better first run detection (Andrew Chow) Pull request description: 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](https://bitcointalk.org/index.php?topic=1993244.0) [users](https://bitcointalk.org/index.php?topic=1746976.msg17511261#msg17511261) 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 Tree-SHA512: 63b485f356566e8ffa033ad9b7101f7f6b56372b29ec2a43b947b0eeb1ada4c2cfe24740515d013aedd5f51aa1890dfbe499d2c5c062fc1b5d272324728a7d55

…rst run… (#3319) * Merge bitcoin#10952: [wallet] Remove vchDefaultKey and have better first run detection e53615b Remove vchDefaultKey and have better first run detection (Andrew Chow) Pull request description: 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](https://bitcointalk.org/index.php?topic=1993244.0) [users](https://bitcointalk.org/index.php?topic=1746976.msg17511261#msg17511261) 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 Tree-SHA512: 63b485f356566e8ffa033ad9b7101f7f6b56372b29ec2a43b947b0eeb1ada4c2cfe24740515d013aedd5f51aa1890dfbe499d2c5c062fc1b5d272324728a7d55 * Update src/wallet/wallet.cpp Co-Authored-By: UdjinM6 <[email protected]> Co-authored-by: Wladimir J. van der Laan <[email protected]> Co-authored-by: UdjinM6 <[email protected]>

…emove vchDefaultKey and have better first run detection". [Wallet][Startup][DB][Backport] Don't create any default address

fanquake added the Wallet label Jul 29, 2017

achow101 force-pushed the remove-defaultkey branch from ca3759d to b156448 Compare July 29, 2017 19:19

achow101 force-pushed the remove-defaultkey branch from b156448 to 94df789 Compare July 31, 2017 18:17

achow101 force-pushed the remove-defaultkey branch 2 times, most recently from 06bd881 to 1a22b28 Compare August 2, 2017 00:25

gmaxwell approved these changes Aug 3, 2017

View reviewed changes

achow101 force-pushed the remove-defaultkey branch from 1a22b28 to 29824af Compare August 3, 2017 23:41

achow101 force-pushed the remove-defaultkey branch from 29824af to 8f3ea17 Compare August 3, 2017 23:50

sipa reviewed Aug 4, 2017

View reviewed changes

achow101 force-pushed the remove-defaultkey branch 2 times, most recently from 27598a0 to f1e4eff Compare August 4, 2017 03:26

achow101 force-pushed the remove-defaultkey branch from f1e4eff to 3157760 Compare August 4, 2017 06:43

sipa added this to the 0.15.0 milestone Aug 5, 2017

achow101 mentioned this pull request Aug 5, 2017

-salvagewallet seems to be causing more issues than it fixes, it should be removed #10991

Closed

jonasschnelli mentioned this pull request Aug 15, 2017

[wallet] Use fixed pubkey during wallet init for default key #10621

Closed

achow101 force-pushed the remove-defaultkey branch from b8aa972 to 1a7bc5a Compare August 15, 2017 21:06

achow101 force-pushed the remove-defaultkey branch from 1a7bc5a to e53615b Compare August 15, 2017 22:06

laanwj merged commit e53615b into bitcoin:master Aug 18, 2017

laanwj mentioned this pull request Aug 24, 2017

Wallet generates a (spurious?) key when creating a new wallet #8416

Closed

achow101 deleted the remove-defaultkey branch August 29, 2017 15:28

laanwj removed this from the 0.15.1 milestone Sep 5, 2017

achow101 mentioned this pull request Dec 7, 2017

Allow the default key to be unavailable #2566

Merged

furszy mentioned this pull request Sep 30, 2019

[Wallet][Startup][DB][Backport] Remove vchDefaultKey from wallet.h PIVX-Project/PIVX#1030

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Uh oh!

Conversation

achow101 commented Jul 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmaxwell commented Jul 29, 2017

Uh oh!

achow101 commented Jul 29, 2017

Uh oh!

gmaxwell commented Jul 30, 2017

Uh oh!

TheBlueMatt commented Jul 30, 2017

Uh oh!

achow101 commented Jul 31, 2017

Uh oh!

gmaxwell left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Aug 3, 2017

Uh oh!

achow101 commented Aug 3, 2017

Uh oh!

sipa Aug 4, 2017

Choose a reason for hiding this comment

Uh oh!

achow101 Aug 4, 2017

Choose a reason for hiding this comment

Uh oh!

sipa Aug 4, 2017

Choose a reason for hiding this comment

Uh oh!

achow101 Aug 4, 2017

Choose a reason for hiding this comment

Uh oh!

sipa Aug 4, 2017

Choose a reason for hiding this comment

Uh oh!

achow101 Aug 4, 2017

Choose a reason for hiding this comment

Uh oh!

maflcko commented Aug 4, 2017

Uh oh!

achow101 commented Aug 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Aug 5, 2017

Uh oh!

TheBlueMatt commented Aug 6, 2017 via email

Uh oh!

achow101 commented Aug 7, 2017

Uh oh!

TheBlueMatt commented Aug 7, 2017

Uh oh!

sipa commented Aug 7, 2017

Uh oh!

TheBlueMatt commented Aug 7, 2017 via email

Uh oh!

achow101 commented Aug 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Aug 8, 2017

Uh oh!

jnewbery commented Aug 14, 2017

Uh oh!

jonasschnelli commented Aug 15, 2017

Uh oh!

achow101 commented Aug 15, 2017

Uh oh!

jnewbery commented Aug 15, 2017

Uh oh!

sipa commented Aug 15, 2017

Uh oh!

achow101 commented Aug 15, 2017

Uh oh!

laanwj commented Aug 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Aug 18, 2017

achow101 commented Jul 29, 2017 •

edited

Loading

achow101 commented Aug 4, 2017 •

edited

Loading

achow101 commented Aug 7, 2017 •

edited

Loading

laanwj commented Aug 16, 2017 •

edited

Loading