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

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

Merged
merged 1 commit into from Jul 19, 2016

Conversation

Projects
None yet
4 participants
@jonasschnelli
Member

jonasschnelli commented Jul 18, 2016

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.

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Jul 19, 2016

Contributor

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.

Contributor

TheBlueMatt commented Jul 19, 2016

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.

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

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jul 19, 2016

Contributor

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

@TheBlueMatt

TheBlueMatt Jul 19, 2016

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 19, 2016

Member

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.

Member

jonasschnelli commented Jul 19, 2016

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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 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.

Member

laanwj commented Jul 19, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 19, 2016

Member

utACK a4f137f

Member

laanwj commented Jul 19, 2016

utACK a4f137f

@laanwj laanwj merged commit a4f137f into bitcoin:master Jul 19, 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 19, 2016

Merge #8367: [Wallet] Ensure <0.13 clients can't open HD wallets
a4f137f [Wallet] Ensure <0.13 clients can't open HD wallets (Jonas Schnelli)
@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Jul 19, 2016

Contributor

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???)

Contributor

TheBlueMatt commented Jul 19, 2016

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???)

@@ -3299,6 +3299,9 @@ bool CWallet::InitLoadWallet()
key.MakeNewKey(true);
if (!walletInstance->SetHDMasterKey(key))
throw std::runtime_error("CWallet::GenerateNewKey(): Storing master key failed");

This comment has been minimized.

@MarcoFalke

MarcoFalke Jul 19, 2016

Member

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

@MarcoFalke

MarcoFalke Jul 19, 2016

Member

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

This comment has been minimized.

@MarcoFalke

MarcoFalke Jul 19, 2016

Member

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

MarcoFalke Jul 19, 2016

Member

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment