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

pull jonasschnelli wants to merge 3 commits into bitcoin:master from jonasschnelli:2016/07/hd_minversion changing 8 files +92 −78

jonasschnelli commented at 9:51 AM on July 15, 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.12.99.

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).
jonasschnelli added the label Wallet on Jul 15, 2016
jonasschnelli added this to the milestone 0.13.0 on Jul 15, 2016
MarcoFalke commented at 12:41 PM on July 15, 2016: member

Concept ACK
MarcoFalke commented at 8:52 PM on July 16, 2016: member

quickly tested ACK 51a4ee8
in src/wallet/wallet.h:None in 51a4ee889e outdated
```
  77 | @@ -78,7 +78,8 @@ enum WalletFeature
  78 |      FEATURE_WALLETCRYPT = 40000, // wallet encryption
  79 |      FEATURE_COMPRPUBKEY = 60000, // compressed public keys
  80 |  
  81 | -    FEATURE_LATEST = 60000
  82 | +    FEATURE_HD = 129900, // Hierarchical key derivation after BIP32 (HD Wallet)
```
MarcoFalke commented at 9:01 PM on July 16, 2016:

Strictly speaking not all 12.99's support HD, so we may want to adjust to 130000 after the branch off.

laanwj commented at 5:35 AM on July 18, 2016:

This isn't derived from the actual version, is it? It's just a marker constant, the exact value doesn't matter as long as it is higher than all other FEATURE_*. So if you want it 130000, just make it 130000 already. Bah, it is checked against the CLIENT_VERSION, I was wrong. Ok, let's do this after the release branch.

jonasschnelli commented at 5:58 AM on July 18, 2016:

Yes. We can try to merge this once we split of the masterbranch and had increase the version.

laanwj commented at 7:08 AM on July 18, 2016:

Yes using the client version here is not very nice, if we had a dedicated wallet version constant this problem could have been avoided, as it could have been bumped at the moment HD support was introduced instead of arbitrary delineation based on release-process constraints.

But for now that's the best solution.

MarcoFalke commented at 7:17 AM on July 18, 2016:

@jonasschnelli Could you already bump this so we don't lose time later? (In case you are not around when the branch off happens)

jonasschnelli commented at 7:28 AM on July 18, 2016:

@MarcoFalke If I bump this to 130000 now, HD would no longer work on current master IMO.

For <0.13 incompatibility, we need to stick to the current check based on CLIENT_VERSION. https://github.com/bitcoin/bitcoin/blob/master/src/wallet/walletdb.cpp#L637

Once we change the wallet database format, we could introduce a set of strings about the features the wallet database requires/supports.

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

If I bump this to 130000 now, HD would no longer work on current master IMO.

True, but I think the plan was to merge this after the bump to 13.99 into master?

No strong opinion, though.

laanwj commented at 10:27 AM on July 18, 2016:

Version have been bumped, you can update this now
laanwj removed this from the milestone 0.13.0 on Jul 18, 2016
laanwj added this to the milestone 0.13.0 on Jul 18, 2016
build: bump version to 0.13.0 084d1ddf8f

build: Release notes update

Fill in the header, and move items to the appropriate part of the
release notes structure.

37269105c8

jtimon commented at 3:27 PM on July 18, 2016: contributor

ut ACK 51a4ee8

in doc/release-notes.md:None in 51a4ee889e outdated

 196 | @@ -197,6 +197,8 @@ You can't disable HD key generation once you have created a HD wallet.
 197 |  
 198 |  There is no distinction between internal (change) and external keys.
 199 |  
 200 | +HD wallets are incompatible with older versions of Bitcoin Core.

MarcoFalke commented at 3:35 PM on July 18, 2016:

I think you need to open a separate pull against .13 now that the release notes were moved.

jonasschnelli force-pushed on Jul 18, 2016
[Wallet] Ensure <0.13 clients can't open HD wallets 3b38a6a96a
jonasschnelli force-pushed on Jul 18, 2016
jonasschnelli commented at 8:55 PM on July 18, 2016: contributor

Sorry. Have to close and reopen (new PR) this because use this local branch for #8366.
jonasschnelli closed this on Jul 18, 2016
jtimon commented at 7:49 PM on July 20, 2016: contributor

Was a replacement PR opened?
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Milestone
0.13.0

Linked (view graph)

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