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

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

Merged
merged 2 commits into from Jul 28, 2016

Conversation

@jonasschnelli
Copy link
Member

commented Jul 21, 2016

Reported by @dooglus.

Fixes #8383

@@ -2081,7 +2081,7 @@ UniValue encryptwallet(const UniValue& params, bool fHelp)
// slack space in .dat files; that is bad if the old data is
// unencrypted private keys. So:
StartShutdown();
return "wallet encrypted; Bitcoin server stopping, restart to run with encrypted wallet. The keypool has been flushed, you need to make a new backup.";
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.";

This comment has been minimized.

Copy link
@luke-jr

luke-jr Jul 21, 2016

Member

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

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Jul 21, 2016

Author Member

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

This comment has been minimized.

Copy link
@laanwj

laanwj Jul 26, 2016

Member

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

@luke-jr

View changes

src/wallet/wallet.cpp Outdated
if (!hdChain.masterKeyID.IsNull()) {
CKey key;
key.MakeNewKey(true);
return false;

This comment has been minimized.

Copy link
@luke-jr

luke-jr Jul 21, 2016

Member

??????????

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Jul 21, 2016

Author Member

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.

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Jul 21, 2016

Author Member

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

@luke-jr

This comment has been minimized.

Copy link
Member

commented Jul 21, 2016

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jul 21, 2016

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

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

commented Jul 21, 2016

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

@MarcoFalke

View changes

qa/rpc-tests/keypool.py Outdated
addrBeforeEncrypting = nodes[0].getnewaddress()
addrBeforeEncryptingData = nodes[0].validateaddress(addrBeforeEncrypting)
walletInfoOld = nodes[0].getwalletinfo()
assert(addrBeforeEncryptingData['hdmasterkeyid'] == walletInfoOld['masterkeyid'])

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jul 21, 2016

Member

Nit: please_use_lowercase_and_underscore

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2016/07/hd_enc branch Jul 21, 2016

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

commented Jul 21, 2016

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jul 21, 2016

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

View changes

src/wallet/wallet.cpp Outdated
if (!hdChain.masterKeyID.IsNull()) {
CKey key;
key.MakeNewKey(true);
if (!SetHDMasterKey(key))

This comment has been minimized.

Copy link
@dooglus

dooglus Jul 21, 2016

Contributor

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

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jul 22, 2016

Member

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.

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Jul 22, 2016

Author Member

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

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2016

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

@pstratem

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2016

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

@dooglus

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2016

@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

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2016

@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

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2016

@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

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2016

Pushed a commit that...

  1. ...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)
  2. ...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

View changes

src/wallet/wallet.h Outdated
@@ -900,8 +900,11 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
/* Set the HD chain model (chain child index counters) */
bool SetHDChain(const CHDChain& chain, bool memonly);

/* Generates a new HD master key (will not be activated) */
CPubKey GenerateNewHDMasterKey(CKey& key);

This comment has been minimized.

Copy link
@instagibbs

instagibbs Jul 22, 2016

Member

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

@instagibbs

View changes

src/wallet/wallet.cpp Outdated
CPubKey pubkey = key.GetPubKey();
assert(key.VerifyPubKey(pubkey));

// set the hd keypath to "m" -> Master, refere the masterkeyid to itself

This comment has been minimized.

Copy link
@instagibbs

instagibbs Jul 22, 2016

Member

refers*

@instagibbs

View changes

src/wallet/wallet.cpp Outdated
CKey key;
CPubKey masterPubKey = GenerateNewHDMasterKey(key);;
if (!SetHDMasterKey(masterPubKey))
return false;

This comment has been minimized.

Copy link
@instagibbs

instagibbs Jul 22, 2016

Member

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 jonasschnelli force-pushed the jonasschnelli:2016/07/hd_enc branch Jul 22, 2016

CKey key;
CPubKey masterPubKey = GenerateNewHDMasterKey();
if (!SetHDMasterKey(masterPubKey))
return false;

This comment has been minimized.

Copy link
@jonasschnelli

jonasschnelli Jul 22, 2016

Author Member

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

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2016

Fixed @instagibbs nits.

@pstratem

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2016

@jonasschnelli needs to be one commit

@laanwj

This comment has been minimized.

Copy link
Member

commented Jul 25, 2016

Is this only aimed at 0.13 on purpose?

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2016

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

This comment has been minimized.

Copy link
Member

commented Jul 27, 2016

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

@instagibbs

This comment has been minimized.

Copy link
Member

commented Jul 27, 2016

utACK 77c912d

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2016/07/hd_enc branch Jul 27, 2016

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2016/07/hd_enc branch to de45c06 Jul 27, 2016

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

commented Jul 27, 2016

Rebased (against 0.13).

@laanwj

This comment has been minimized.

Copy link
Member

commented Jul 28, 2016

code-review ACK de45c06, going to test

@laanwj

This comment has been minimized.

Copy link
Member

commented Jul 28, 2016

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 laanwj merged commit de45c06 into bitcoin:0.13 Jul 28, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Jul 28, 2016
Merge #8389: [0.13] Create a new HD seed after encrypting the wallet
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)
laanwj added a commit that referenced this pull request Jul 28, 2016
Port from 0.13: Create a new HD seed after encrypting the wallet
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
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jul 31, 2016

Removed "needs backport"


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

This comment has been minimized.

Copy link
@dooglus

dooglus Jul 31, 2016

Contributor

Unused variable.


// write the key&metadata to the database
if (!AddKeyPubKey(key, pubkey))
throw std::runtime_error("CWallet::GenerateNewKey(): AddKey failed");

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jul 31, 2016

Member

Nit: Wrong name

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