Avoid reading the old hd master key during wallet encryption #10115

pull TheBlueMatt wants to merge 1 commits into bitcoin:master from TheBlueMatt:2017-03-cleanup-sethdmasterkey changing 2 files +8 −13

TheBlueMatt commented at 4:40 PM on March 29, 2017: member

This is a minor clenaup after #9294.

This makes SetHDMasterKey responsible for maintinaing the CHDChain version instead of always creating it with the latest version and making EncryptWallet responsible for keeping the version from changing.
fanquake added the label Wallet on Mar 30, 2017
jonasschnelli commented at 7:03 AM on March 30, 2017: contributor

utACK fdd15f770a649be57a1c6abc346cd4d0da831493
in src/wallet/wallet.cpp:640 in fdd15f770a outdated
```
 638 | @@ -639,9 +639,7 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
 639 |          if (IsHDEnabled()) {
 640 |              CKey key;
```
jnewbery commented at 2:54 PM on April 13, 2017:

nit: remove this unused variable in this commit (it was unused before your PR, but since you're here...)

in src/wallet/wallet.cpp:642 in fdd15f770a outdated

 638 | @@ -639,9 +639,7 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
 639 |          if (IsHDEnabled()) {
 640 |              CKey key;
 641 |              CPubKey masterPubKey = GenerateNewHDMasterKey();
 642 | -            // preserve the old chains version to not break backward compatibility
 643 | -            CHDChain oldChain = GetHDChain();
 644 | -            if (!SetHDMasterKey(masterPubKey, &oldChain))
 645 | +            if (!SetHDMasterKey(masterPubKey))

jnewbery commented at 2:56 PM on April 13, 2017:

suggestion only: combine this with the line above to:

if (!SetHDMasterKey(GenerateNewHDMasterKey()))

Also: curly braces for single line if block please.

in src/wallet/wallet.h:1060 in fdd15f770a outdated

1055 | @@ -1056,9 +1056,9 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
1056 |      CPubKey GenerateNewHDMasterKey();
1057 |      
1058 |      /* Set the current HD master key (will reset the chain child index counters)
1059 | -       If possibleOldChain is provided, the parameters from the old chain (version)
1060 | -       will be preserved. */
1061 | -    bool SetHDMasterKey(const CPubKey& key, CHDChain *possibleOldChain = nullptr);
1062 | +       Sets the master keys version based on the current wallet version (so make
1063 | +       its set appropriately before calling this). */

jnewbery commented at 3:00 PM on April 13, 2017:

Couple of grammar/typos here. Suggest you replace with: The master key version is set to the current wallet version, so the caller must ensure that current wallet version is correct before calling this function. or similar

jnewbery approved
jnewbery commented at 3:00 PM on April 13, 2017: member

couple of nits, but otherwise utACK fdd15f770a649be57a1c6abc346cd4d0da831493
TheBlueMatt force-pushed on Apr 13, 2017
TheBlueMatt commented at 3:19 PM on April 13, 2017: member

Resolved @jnewbery's minor comments.

Avoid reading the old hd master key during wallet encryption

This makes SetHDMasterKey responsible for maintinaing the CHDChain
version instead of always creating it with the latest version and
making EncryptWallet responsible for keeping the version from
changing.

185c7f08be

in src/wallet/wallet.h:1059 in e2da46dba9 outdated

1055 | @@ -1056,9 +1056,10 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
1056 |      CPubKey GenerateNewHDMasterKey();
1057 |      
1058 |      /* Set the current HD master key (will reset the chain child index counters)
1059 | -       If possibleOldChain is provided, the parameters from the old chain (version)
1060 | -       will be preserved. */
1061 | -    bool SetHDMasterKey(const CPubKey& key, CHDChain *possibleOldChain = nullptr);
1062 | +       Sets the master keys version based on the current wallet version (so the

jnewbery commented at 3:41 PM on April 13, 2017:

nit: you missed one of the grammar errors: s/keys/key's/ . There's only one of them.

(sorry - normally I wouldn't nit for grammar, but this could be misleading - hence my earlier suggestion to reword)

TheBlueMatt commented at 3:55 PM on April 13, 2017:

Oops, sorry, missed that one

TheBlueMatt force-pushed on Apr 13, 2017
jnewbery commented at 4:01 PM on April 13, 2017: member

utACK 185c7f08be68eec878f4b32b3a52145dd57e13bd
laanwj merged this on May 3, 2017
laanwj closed this on May 3, 2017
laanwj referenced this in commit d3dce0eb67 on May 3, 2017
PastaPastaPasta referenced this in commit 885c8ffcf9 on Jun 10, 2019
PastaPastaPasta referenced this in commit 15bf32da60 on Jun 10, 2019
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#9294 Use internal HD chain for change outputs (hd split)