Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Aug 18, 2017

Conversation

Projects
None yet
9 participants
@achow101
Copy link
Member

achow101 commented Jul 29, 2017

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

@fanquake fanquake added the Wallet label Jul 29, 2017

@achow101 achow101 force-pushed the achow101:remove-defaultkey branch Jul 29, 2017

@gmaxwell

This comment has been minimized.

Copy link
Member

gmaxwell commented Jul 29, 2017

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

This comment has been minimized.

Copy link
Member Author

achow101 commented Jul 29, 2017

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

This comment has been minimized.

Copy link
Member

gmaxwell commented Jul 30, 2017

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

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Jul 30, 2017

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 achow101 force-pushed the achow101:remove-defaultkey branch Jul 31, 2017

@achow101

This comment has been minimized.

Copy link
Member Author

achow101 commented Jul 31, 2017

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

@achow101 achow101 force-pushed the achow101:remove-defaultkey branch 2 times, most recently from 06bd881 Jul 31, 2017

@gmaxwell
Copy link
Member

gmaxwell left a comment

utACK

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Aug 3, 2017

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 achow101 force-pushed the achow101:remove-defaultkey branch Aug 3, 2017

@achow101

This comment has been minimized.

Copy link
Member Author

achow101 commented Aug 3, 2017

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.

@achow101 achow101 force-pushed the achow101:remove-defaultkey branch Aug 3, 2017

src/wallet/wallet.cpp Outdated

// Generate an address for the address book and to fill the keypool
CPubKey key;
if (walletInstance->GetKeyFromPool(key, false)) {

This comment has been minimized.

@sipa

sipa Aug 4, 2017

Member

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

This comment has been minimized.

@achow101

achow101 Aug 4, 2017

Author Member

Done

src/wallet/walletdb.cpp Outdated
ssKey >> vchPubKey;
if (!vchPubKey.IsValid())
{
strErr = "Error reading wallet database: Default Key corrupt";

This comment has been minimized.

@sipa

sipa Aug 4, 2017

Member

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

This comment has been minimized.

@achow101

achow101 Aug 4, 2017

Author Member

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

src/wallet/walletdb.cpp Outdated
CPubKey vchPubKey;
ssKey >> vchPubKey;
if (!vchPubKey.IsValid())
{

This comment has been minimized.

@sipa

sipa Aug 4, 2017

Member

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

This comment has been minimized.

@achow101

achow101 Aug 4, 2017

Author Member

Done

@achow101 achow101 force-pushed the achow101:remove-defaultkey branch 2 times, most recently Aug 4, 2017

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Aug 4, 2017

AssertionError in wallet-hd:

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

@achow101 achow101 force-pushed the achow101:remove-defaultkey branch Aug 4, 2017

@achow101

This comment has been minimized.

Copy link
Member Author

achow101 commented Aug 4, 2017

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 sipa added this to the 0.15.0 milestone Aug 5, 2017

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Aug 5, 2017

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

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Aug 6, 2017

@achow101

This comment has been minimized.

Copy link
Member Author

achow101 commented Aug 7, 2017

@TheBlueMatt What for?

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Aug 7, 2017

@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

This comment has been minimized.

Copy link
Member

sipa commented Aug 7, 2017

@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

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Aug 7, 2017

@achow101

This comment has been minimized.

Copy link
Member Author

achow101 commented Aug 7, 2017

@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 achow101 force-pushed the achow101:remove-defaultkey branch Aug 7, 2017

@achow101

This comment has been minimized.

Copy link
Member Author

achow101 commented Aug 7, 2017

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.

@achow101 achow101 force-pushed the achow101:remove-defaultkey branch Aug 7, 2017

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Aug 7, 2017

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.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Aug 8, 2017

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.

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Aug 8, 2017

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.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Aug 8, 2017

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

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Aug 8, 2017

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.

@laanwj laanwj modified the milestones: 0.15.1, 0.15.0 Aug 8, 2017

@achow101

This comment has been minimized.

Copy link
Member Author

achow101 commented Aug 8, 2017

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

This comment has been minimized.

Copy link
Member

jnewbery commented Aug 14, 2017

Looks good. Tested ACK b8aa9729a5dbeb883857b14918aa6c6d2b71eea4 (although needs rebase)

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Aug 15, 2017

Needs rebase.

@achow101

This comment has been minimized.

Copy link
Member Author

achow101 commented Aug 15, 2017

rebased

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Aug 15, 2017

utACK 1a7bc5aac60a37b6f9f0632546467d716a3fd5e4

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Aug 15, 2017

There seems to be a problem in keypool_topup.py.

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.

@achow101 achow101 force-pushed the achow101:remove-defaultkey branch to e53615b Aug 15, 2017

@achow101

This comment has been minimized.

Copy link
Member Author

achow101 commented Aug 15, 2017

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

This comment has been minimized.

Copy link
Member

laanwj commented Aug 16, 2017

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

Finally :-)

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Aug 18, 2017

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

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Aug 18, 2017

Merge #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

@achow101 achow101 deleted the achow101:remove-defaultkey branch Aug 29, 2017

@laanwj laanwj removed this from the 0.15.1 milestone Sep 5, 2017

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Sep 5, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.