[Wallet] Ensure <0.13 clients can't open HD wallets #8367

pull jonasschnelli wants to merge 1 commits into bitcoin:master from jonasschnelli:2016/07/hd_minversion_014 changing 2 files +5 −1

jonasschnelli commented at 8:58 PM on July 18, 2016: contributor

In current master, you could run a 0.13 HD wallet in a non HD compatible 0.12 client, resulting in a mix of HD and non HD key. You could even refill the wallets keypool running a 0.12 client and use it on 0.13 which would lead to a getnewaddress call in 0.13 responding a non HD address.

This PR ensures that HD wallets will at least require version 0.13.

This only affects newly created wallets and only if the user had not passed -usehd=0 (old <0.13 created wallets will still run on <0.13 clients once they where opened in 0.13).

Correct 0.14 branch / replacement for #8343.
[Wallet] Ensure <0.13 clients can't open HD wallets a4f137f367
jonasschnelli added the label Wallet on Jul 18, 2016
TheBlueMatt commented at 1:53 AM on July 19, 2016: member

This might complicate future wallet changes...it seems to me that this locks us in such that the next change to wallet version must either be the same non-default or we will, at that time, have to force hd wallets to be the default. I'm not strictly aginst this, just noting that it seems it was not at all discussed but locks us into a future path.

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

3298 | @@ -3299,6 +3299,9 @@ bool CWallet::InitLoadWallet()
3299 |              key.MakeNewKey(true);
3300 |              if (!walletInstance->SetHDMasterKey(key))
3301 |                  throw std::runtime_error("CWallet::GenerateNewKey(): Storing master key failed");
3302 | +
3303 | +            // ensure this wallet.dat can only be opened by clients supporting HD
3304 | +            walletInstance->SetMinVersion(FEATURE_HD);

TheBlueMatt commented at 1:54 AM on July 19, 2016:

I think this call should go above SetHDMasterKey...cant have it crash in between and leave the wallet with an HD seed but no minversion set (not that it really matters, but, you know....)

jonasschnelli commented at 5:24 AM on July 19, 2016: contributor

Thanks Matt for the review. I agree.

We could change FEATURE_HD to FEATURE_FLAGS and store a set of string in wallet.dat in this case.

See the relevant discussion: https://botbot.me/freenode/bitcoin-core-dev/2016-07-18/?msg=69815345&page=1

Working on a PR that could be an alternative for this one.
laanwj commented at 9:54 AM on July 19, 2016: member

@TheBlueMatt Right - we've discussed a better system to handle wallet versioning and features in the future. The current system is sub-par. However for now we need this, it is important for consistency that wallets which have HD enabled aren't used with older releases.
laanwj commented at 10:13 AM on July 19, 2016: member

utACK a4f137f
laanwj merged this on Jul 19, 2016
laanwj closed this on Jul 19, 2016
laanwj referenced this in commit 045106b4f1 on Jul 19, 2016
TheBlueMatt commented at 7:16 PM on July 19, 2016: member

I did not mean to imply that I think this is bad...In fact I prefer this to doing something complicated like adding feature flags significantly. Just felt it needed pointing out that this locks in our future options (though why did we not fix the write-order prior to merge???)
in src/wallet/wallet.cpp:None in a4f137f367
```
3298 | @@ -3299,6 +3299,9 @@ bool CWallet::InitLoadWallet()
3299 |              key.MakeNewKey(true);
3300 |              if (!walletInstance->SetHDMasterKey(key))
3301 |                  throw std::runtime_error("CWallet::GenerateNewKey(): Storing master key failed");
```
MarcoFalke commented at 7:27 PM on July 19, 2016:

This is dead code. You might as well remove it.

MarcoFalke commented at 8:00 PM on July 19, 2016:

Also the strings in the exceptions have the wrong function names. (Here as well as SetHDMasterKey())

Might as well rework exceptions/return values for those?
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#8343 [Wallet] Ensure <0.13 clients can't open HD wallets #8369 [FOR LATER USE][WIP][Wallet] add support for a flexible "set of features"