[0.13] Create a new HD seed after encrypting the wallet #8389

jonasschnelli · 2016-07-21T19:21:20Z

Reported by @dooglus.

Fixes #8383

luke-jr · 2016-07-21T19:22:31Z

src/wallet/rpcwallet.cpp

Maybe the code should just check if HD is being used?

I have though about that but I think it's not worth adding another check here.

I don't think that's worth it either, just for the message

luke-jr · 2016-07-21T19:23:22Z

Will this destroy the old HD seed? Seems like it'd be valuable to keep it around somewhere...

maflcko · 2016-07-21T19:24:59Z

@jonasschnelli Could you open this against master, please? (The backport can be done after merge and discussion/nits etc.)

jonasschnelli · 2016-07-21T19:26:26Z

@luke-jr The old HD seed is still available over dumpwallet. You can get the CKeyID from the used masterkey when calling validateaddress.

maflcko · 2016-07-21T19:27:06Z

qa/rpc-tests/keypool.py

Nit: please_use_lowercase_and_underscore

jonasschnelli · 2016-07-21T19:41:32Z

Fixed the nits.
Sorry for opening this against 0.13 in the first place.

maflcko · 2016-07-21T19:59:44Z

So this makes it impossible to encrypt a seed which is already in use and is planned to be used in the future, but I guess for 0.13 this is fair enough.

Concept ACK

dooglus · 2016-07-21T21:03:20Z

src/wallet/wallet.cpp

SetHDMasterKey() can only return true. When it fails it throws an exception. Do we need to catch that exception?

Agree that it is confusing to have bool SomeFunction(), where the return value indicates if something bad happens but at the same time this is unused and exceptions are thrown. This should be made consistent.

I think this solution is fine for the moment. The return value of SetHDMasterKey() is for future extensions and I think its useful even if its not possible to get a false returned with the current implementation.

The exception will already be caught though the RPC/Server logic (EncryptWallet() will only be called in the RPC context where we "handle" exceptions).

pstratem · 2016-07-21T21:35:45Z

@jonasschnelli Does BDB guarantee the order in which keys are iterated? If not then this isn't guaranteed to work.

pstratem · 2016-07-21T22:32:56Z

@jonasschnelli Indeed you're going to need to use the key metadata timestamp to select which hdchain to use.

dooglus · 2016-07-22T02:24:58Z

@pstratem It reads to me as if the old hdchain key is overwritten by the new one:

bool CWalletDB::WriteHDChain(const CHDChain& chain)
{
    nWalletDBUpdated++;
    return Write(std::string("hdchain"), chain);
}

This WriteHDChain() function is used each time a new address is generated:

    // update the chain model in the database
    if (!CWalletDB(strWalletFile).WriteHDChain(hdChain))

The original chain could be stored under a different database key if we need to keep it around somewhere.

pstratem · 2016-07-22T02:40:18Z

@dooglus indeed you're right... I'm not sure that is any better

certainly the old hd seed needs to be saved, possibly just with a prefix to make it not conflict or be used.

jonasschnelli · 2016-07-22T08:26:48Z

@pstratem: CWalletDB and BDB is a key value storage as far as I know. Writing the same key at later stage should replace the current value. It may be possible to have the old K/V still stored in the database, though, CWalletDB/BDB needs to make sure the later write operation will result in the laster read/mem-map operation during load wallet.

But I agree we need to keep track of the older/replaced seed. Maybe we should add something to the CKeyMetadata in that case (or at least CAddressBook).

jonasschnelli · 2016-07-22T09:33:32Z

Pushed a commit that...

...factors out the creation of a new HD Master key (will be called now during init creation of the wallet as well as during encryption of the wallet)
...stores a CKeyMetadata record together with a HD master key where we set a hdkeypath="m" for later identifying this key as hd master key

https://github.com/bitcoin/bitcoin/pull/8206/files would make use of this. It would show old/rewritten master keys in the dumped wallet.

instagibbs · 2016-07-22T17:45:28Z

src/wallet/wallet.h

nit: argument is never used anywhere, but I could see this being useful in future

jonasschnelli · 2016-07-22T18:44:59Z

src/wallet/wallet.cpp

I think this error detection case is extremely unlikely and probably on the same level then an exception during NewKeyPool().
I don't see a better way of handling this case.

jonasschnelli · 2016-07-22T18:45:16Z

Fixed @instagibbs nits.

pstratem · 2016-07-23T03:19:35Z

@jonasschnelli needs to be one commit

laanwj · 2016-07-25T11:57:21Z

Is this only aimed at 0.13 on purpose?

jonasschnelli · 2016-07-25T13:19:24Z

No. I should have opened this PR against master. I'll open a PR against master as soon as this one is "final" (maybe it cleanly applies to master).

laanwj · 2016-07-27T13:25:07Z

~~Tested ACK 77c912d~~
Sorry, wrong issue - will test this one later

instagibbs · 2016-07-27T13:28:29Z

utACK 77c912d

…y generation

jonasschnelli · 2016-07-27T14:03:19Z

Rebased (against 0.13).

laanwj · 2016-07-28T10:05:16Z

code-review ACK de45c06, going to test

laanwj · 2016-07-28T10:35:40Z

I did this:

$ rm ~/.bitcoin/regtest/wallet.dat
$ src/bitcoind -regtest -rpcport=1234 -daemon -usehd=1
Bitcoin server starting
$ src/bitcoin-cli  -regtest -rpcport=1234 dumpwallet "/tmp/dump_pre"
$ src/bitcoin-cli  -regtest -rpcport=1234 encryptwallet "pass"
wallet encrypted; Bitcoin server stopping, restart to run with encrypted wallet. The keypool has been flushed, you need to make a new backup.
$ src/bitcoind -regtest -rpcport=1234 -daemon -usehd=1
Bitcoin server starting
$ src/bitcoin-cli  -regtest -rpcport=1234 walletpassphrase "pass" 12345
$ src/bitcoin-cli  -regtest -rpcport=1234 dumpwallet "/tmp/dump_post"
$ src/bitcoin-cli  -regtest -rpcport=1234 stop
$ diff  -du /tmp/dump_pre /tmp/dump_post

Result: http://www.hastebin.com/ajuhahofan.diff

Looks OK to me. The old reserve keys have been demoted to "change", and there are new reserve keys in the keypool. These keys are different, suggesting a new master key was used to generate them.

The old master key was kept around as "oldhdmaster":

-cSoMD7tEsoJNfSS3cL6N3E9gXpuQh7wgs79RDUBE6NjTEWS6Ptda 2016-07-28T10:48:28Z hdmaster=1 # addr=msqHNkZcD6ybT7xdCkquN4esLPLmoRyk8b hdkeypath=m
+cSoMD7tEsoJNfSS3cL6N3E9gXpuQh7wgs79RDUBE6NjTEWS6Ptda 2016-07-28T10:48:28Z inactivehdmaster=1 # addr=msqHNkZcD6ybT7xdCkquN4esLPLmoRyk8b hdkeypath=m

And a new one was added

+cRrbkixFuTz1esCbJTz3QqNGyEcPjFCjVyHRTy1JDxKDbHYXn9Qd 2016-07-28T10:48:43Z hdmaster=1 # addr=n3phnt6MDc2xUvKXrf8iW6ZiwtL2i7Rusm hdkeypath=m

The single key that is automatically generated (with empty label) at initialization stayed the same

 cRj4c74AFYdH9W1vbM3UgnaCeGbwztu3qb11NgDReiudtS2Q1ZAr 2016-07-28T10:48:28Z label= # addr=mwR8SR9sfKk7Q8z99xpdcCUNtccq81iRup hdkeypath=m/0'/0'/0'

For the new reserve keys it starts counting again at m/0'/0'/0'

+cNRrj6BtSxcXySqU6WgWgGEHMARifd1VXCVECdsXx4ervHE7iS4t 2016-07-28T10:48:43Z reserve=1 # addr=moBwgfzRf8H4ZjweCbQcckuJbMC16jwtD1 hdkeypath=m/0'/0'/0'

I think everything is as expected. Tested ACK.

de45c06 [Wallet] Add CKeyMetadata record for HDMasterKey(s), factor out HD key generation (Jonas Schnelli) f142c11 [0.13] Create a new HD seed after encrypting the wallet (Jonas Schnelli)

Forward-ports two commits from 0.13: - [0.13] Create a new HD seed after encrypting the wallet - [Wallet] Add CKeyMetadata record for HDMasterKey(s), factor out HD key generation Github-Pull: #8389 Rebased-From: f142c11 de45c06

maflcko · 2016-07-31T13:53:34Z

Removed "needs backport"

dooglus · 2016-07-31T14:06:37Z

src/wallet/wallet.cpp

+
+        // if we are using HD, replace the HD master key (seed) with a new one
+        if (!hdChain.masterKeyID.IsNull()) {
+            CKey key;


Unused variable.

jonasschnelli added the Wallet label Jul 21, 2016

luke-jr reviewed Jul 21, 2016
View reviewed changes

maflcko reviewed Jul 21, 2016
View reviewed changes

qa/rpc-tests/keypool.py Outdated

Copy link

Member

maflcko Jul 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: please_use_lowercase_and_underscore

jonasschnelli force-pushed the 2016/07/hd_enc branch from f2a14f8 to 5a2b0a9 Compare July 21, 2016 19:41

dooglus reviewed Jul 21, 2016
View reviewed changes

jonasschnelli mentioned this pull request Jul 22, 2016

[Wallet] Add HD xpriv to dumpwallet #8206

Merged

jonasschnelli added this to the 0.13.0 milestone Jul 22, 2016

jonasschnelli added the Priority Medium label Jul 22, 2016

laanwj added the Needs backport label Jul 22, 2016

instagibbs reviewed Jul 22, 2016
View reviewed changes

src/wallet/wallet.h Outdated

Copy link

Member

instagibbs Jul 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: argument is never used anywhere, but I could see this being useful in future

jonasschnelli force-pushed the 2016/07/hd_enc branch from 0242ea4 to f162e7a Compare July 22, 2016 18:42

jonasschnelli reviewed Jul 22, 2016
View reviewed changes

[0.13] Create a new HD seed after encrypting the wallet

f142c11

jonasschnelli force-pushed the 2016/07/hd_enc branch from f162e7a to 6165210 Compare July 27, 2016 13:57

[Wallet] Add CKeyMetadata record for HDMasterKey(s), factor out HD ke…

de45c06

…y generation

jonasschnelli force-pushed the 2016/07/hd_enc branch from 6165210 to de45c06 Compare July 27, 2016 14:03

laanwj merged commit de45c06 into bitcoin:0.13 Jul 28, 2016

laanwj mentioned this pull request Jul 28, 2016

encrypting wallet reuses keys that have been written to disk in the clear #8383

Closed

maflcko removed the Needs backport label Jul 31, 2016

dooglus reviewed Jul 31, 2016
View reviewed changes

maflcko mentioned this pull request Jul 31, 2016

[Wallet][Trivial] Fix exception message to reference actual thrower. #8376

Closed

kallerosenbaum mentioned this pull request May 16, 2018

Misleading "Encrypt wallet" GUI process #13245

Closed

chappjc mentioned this pull request Dec 10, 2020

BTC deposit address not in dex wallet decred/dcrdex#879

Closed

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

[0.13] Create a new HD seed after encrypting the wallet #8389

[0.13] Create a new HD seed after encrypting the wallet #8389

Uh oh!

Conversation

jonasschnelli commented Jul 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Jul 21, 2016

Uh oh!

maflcko commented Jul 21, 2016

Uh oh!

jonasschnelli commented Jul 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Jul 21, 2016

Uh oh!

maflcko commented Jul 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pstratem commented Jul 21, 2016

Uh oh!

pstratem commented Jul 21, 2016

Uh oh!

dooglus commented Jul 22, 2016

Uh oh!

pstratem commented Jul 22, 2016

Uh oh!

jonasschnelli commented Jul 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Jul 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Jul 22, 2016

Uh oh!

pstratem commented Jul 23, 2016

Uh oh!

laanwj commented Jul 25, 2016

Uh oh!

jonasschnelli commented Jul 25, 2016

Uh oh!

laanwj commented Jul 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

instagibbs commented Jul 27, 2016

Uh oh!

jonasschnelli commented Jul 27, 2016

Uh oh!

laanwj commented Jul 28, 2016

Uh oh!

laanwj commented Jul 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jul 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jonasschnelli commented Jul 22, 2016 •

edited

Loading

laanwj commented Jul 27, 2016 •

edited

Loading

laanwj commented Jul 28, 2016 •

edited

Loading