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

jonasschnelli commented at 7:21 PM on July 21, 2016: contributor

Reported by @dooglus.

jonasschnelli added the label Wallet on Jul 21, 2016

in src/wallet/rpcwallet.cpp:None in f2a14f8477 outdated

2080 | @@ -2081,7 +2081,7 @@ UniValue encryptwallet(const UniValue& params, bool fHelp)
2081 |      // slack space in .dat files; that is bad if the old data is
2082 |      // unencrypted private keys. So:
2083 |      StartShutdown();
2084 | -    return "wallet encrypted; Bitcoin server stopping, restart to run with encrypted wallet. The keypool has been flushed, you need to make a new backup.";
2085 | +    return "wallet encrypted; Bitcoin server stopping, restart to run with encrypted wallet. The keypool has been flushed and a new HD seed was generated (if you are using HD). You need to make a new backup.";

luke-jr commented at 7:22 PM on July 21, 2016:

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

jonasschnelli commented at 7:23 PM on July 21, 2016:

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

laanwj commented at 9:55 AM on July 26, 2016:

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

in src/wallet/wallet.cpp:None in f2a14f8477 outdated

 625 | @@ -626,6 +626,16 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
 626 |  
 627 |          Lock();
 628 |          Unlock(strWalletPassphrase);
 629 | +
 630 | +        // if we are using HD, replace the HD master key with a new one
 631 | +        if (!hdChain.masterKeyID.IsNull()) {
 632 | +            CKey key;
 633 | +            key.MakeNewKey(true);
 634 | +            return false;

luke-jr commented at 7:22 PM on July 21, 2016:

??????????

jonasschnelli commented at 7:25 PM on July 21, 2016:

It will report that the encryption did fail, restarting bitcoin-core will result in using the old seed with a valid wallet. Not sure if this is ideal.

jonasschnelli commented at 7:32 PM on July 21, 2016:

Arg. No. This was for testing. Will remove it.

luke-jr commented at 7:23 PM on July 21, 2016: member

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

MarcoFalke commented at 7:24 PM on July 21, 2016: member

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

jonasschnelli commented at 7:26 PM on July 21, 2016: contributor

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

in qa/rpc-tests/keypool.py:None in f2a14f8477 outdated

  11 | @@ -12,13 +12,23 @@ class KeyPoolTest(BitcoinTestFramework):
  12 |  
  13 |      def run_test(self):
  14 |          nodes = self.nodes
  15 | +        addrBeforeEncrypting = nodes[0].getnewaddress()
  16 | +        addrBeforeEncryptingData = nodes[0].validateaddress(addrBeforeEncrypting)
  17 | +        walletInfoOld = nodes[0].getwalletinfo()
  18 | +        assert(addrBeforeEncryptingData['hdmasterkeyid'] == walletInfoOld['masterkeyid'])

MarcoFalke commented at 7:27 PM on July 21, 2016:

Nit: please_use_lowercase_and_underscore

jonasschnelli force-pushed on Jul 21, 2016

jonasschnelli commented at 7:41 PM on July 21, 2016: contributor

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

MarcoFalke commented at 7:59 PM on July 21, 2016: member

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

in src/wallet/wallet.cpp:None in 5a2b0a9e12 outdated

 625 | @@ -626,6 +626,15 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
 626 |  
 627 |          Lock();
 628 |          Unlock(strWalletPassphrase);
 629 | +
 630 | +        // if we are using HD, replace the HD master key with a new one
 631 | +        if (!hdChain.masterKeyID.IsNull()) {
 632 | +            CKey key;
 633 | +            key.MakeNewKey(true);
 634 | +            if (!SetHDMasterKey(key))

dooglus commented at 9:03 PM on July 21, 2016:

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

MarcoFalke commented at 7:43 AM on July 22, 2016:

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.

jonasschnelli commented at 8:24 AM on July 22, 2016:

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 commented at 9:35 PM on July 21, 2016: contributor

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

pstratem commented at 10:32 PM on July 21, 2016: contributor

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

dooglus commented at 2:24 AM on July 22, 2016: contributor

@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 commented at 2:40 AM on July 22, 2016: contributor

@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 commented at 8:26 AM on July 22, 2016: contributor

@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 commented at 9:33 AM on July 22, 2016: contributor

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.

jonasschnelli added this to the milestone 0.13.0 on Jul 22, 2016

jonasschnelli added the label Priority Medium on Jul 22, 2016

laanwj added the label Needs backport on Jul 22, 2016

in src/wallet/wallet.h:None in 0242ea4120 outdated

 899 | @@ -900,8 +900,11 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
 900 |      /* Set the HD chain model (chain child index counters) */
 901 |      bool SetHDChain(const CHDChain& chain, bool memonly);
 902 |  
 903 | +    /* Generates a new HD master key (will not be activated) */
 904 | +    CPubKey GenerateNewHDMasterKey(CKey& key);

instagibbs commented at 5:45 PM on July 22, 2016:

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

in src/wallet/wallet.cpp:None in 0242ea4120 outdated

1185 | +
1186 | +    // calculate the pubkey
1187 | +    CPubKey pubkey = key.GetPubKey();
1188 | +    assert(key.VerifyPubKey(pubkey));
1189 | +
1190 | +    // set the hd keypath to "m" -> Master, refere the masterkeyid to itself

instagibbs commented at 5:50 PM on July 22, 2016:

refers*

in src/wallet/wallet.cpp:None in 0242ea4120 outdated

 630 | +        // if we are using HD, replace the HD master key (seed) with a new one
 631 | +        if (!hdChain.masterKeyID.IsNull()) {
 632 | +            CKey key;
 633 | +            CPubKey masterPubKey = GenerateNewHDMasterKey(key);;
 634 | +            if (!SetHDMasterKey(masterPubKey))
 635 | +                return false;

instagibbs commented at 6:02 PM on July 22, 2016:

I think this is ok, but someone more knowledgeable should make sure that returning now before re-locking doesn't have a negative side-effect vs previous behavior.

jonasschnelli force-pushed on Jul 22, 2016

in src/wallet/wallet.cpp:None in f162e7a43c outdated

 630 | +        // if we are using HD, replace the HD master key (seed) with a new one
 631 | +        if (!hdChain.masterKeyID.IsNull()) {
 632 | +            CKey key;
 633 | +            CPubKey masterPubKey = GenerateNewHDMasterKey();
 634 | +            if (!SetHDMasterKey(masterPubKey))
 635 | +                return false;

jonasschnelli commented at 6:44 PM on July 22, 2016:

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 commented at 6:45 PM on July 22, 2016: contributor

Fixed @instagibbs nits.

pstratem commented at 3:19 AM on July 23, 2016: contributor

@jonasschnelli needs to be one commit

laanwj commented at 11:57 AM on July 25, 2016: member

Is this only aimed at 0.13 on purpose?

jonasschnelli commented at 1:19 PM on July 25, 2016: contributor

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 commented at 1:25 PM on July 27, 2016: member

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

instagibbs commented at 1:28 PM on July 27, 2016: member

utACK https://github.com/bitcoin/bitcoin/commit/77c912d21c8cd153f4503c65225a5a46990cc85a

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

jonasschnelli force-pushed on Jul 27, 2016

[Wallet] Add CKeyMetadata record for HDMasterKey(s), factor out HD key generation de45c065f0

jonasschnelli force-pushed on Jul 27, 2016

jonasschnelli commented at 2:03 PM on July 27, 2016: contributor

Rebased (against 0.13).

laanwj commented at 10:05 AM on July 28, 2016: member

code-review ACK de45c06, going to test

laanwj commented at 10:35 AM on July 28, 2016: member

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.

laanwj merged this on Jul 28, 2016

laanwj closed this on Jul 28, 2016

laanwj referenced this in commit c3c82c48d9 on Jul 28, 2016

laanwj referenced this in commit 2266b43e33 on Jul 28, 2016

MarcoFalke removed the label Needs backport on Jul 31, 2016

MarcoFalke commented at 1:53 PM on July 31, 2016: member

Removed "needs backport"

in src/wallet/wallet.cpp:None in de45c065f0

 625 | @@ -626,6 +626,15 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
 626 |  
 627 |          Lock();
 628 |          Unlock(strWalletPassphrase);
 629 | +
 630 | +        // if we are using HD, replace the HD master key (seed) with a new one
 631 | +        if (!hdChain.masterKeyID.IsNull()) {
 632 | +            CKey key;

dooglus commented at 2:06 PM on July 31, 2016:

Unused variable.

in src/wallet/wallet.cpp:None in de45c065f0

1198 | +        // mem store the metadata
1199 | +        mapKeyMetadata[pubkey.GetID()] = metadata;
1200 | +
1201 | +        // write the key&metadata to the database
1202 | +        if (!AddKeyPubKey(key, pubkey))
1203 | +            throw std::runtime_error("CWallet::GenerateNewKey(): AddKey failed");

MarcoFalke commented at 2:15 PM on July 31, 2016:

Nit: Wrong name

DrahtBot locked this on Sep 8, 2021