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

Bump wallet version to 159900 and remove the `usehd` option #11250

Merged
merged 2 commits into from Sep 8, 2017

Conversation

Projects
None yet
8 participants
@achow101
Copy link
Member

achow101 commented Sep 5, 2017

Bump the wallet version number to 159900 so that new wallets made without a default key will no longer work on previous versions at all. Also remove the usehd option to avoid weird interaction with wallet version numbers and HD-ness of wallets.

@fanquake fanquake added the Wallet label Sep 5, 2017

src/wallet/wallet.cpp Outdated
@@ -3849,17 +3845,6 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)

walletInstance->SetBestChain(chainActive.GetLocator());
}
else if (gArgs.IsArgSet("-usehd")) {
bool useHD = gArgs.GetBoolArg("-usehd", DEFAULT_USE_HD_WALLET);

This comment has been minimized.

@laanwj

laanwj Sep 5, 2017

Member

I think we want to keep raising an (deprecation) error if -usehd is provided

This comment has been minimized.

@achow101

achow101 Sep 5, 2017

Author Member

Done

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Sep 5, 2017

The option needs to be removed from GetWalletHelpString help too

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Sep 5, 2017

Removal of usehd needs a small mention in the release-notes.md

@achow101 achow101 force-pushed the achow101:bump-wallet-version branch Sep 5, 2017

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Sep 6, 2017

Concept ACK, we need this asap, to prevent problems with wallets created with master on older versions (due to lack of default key triggering HD master key rollover).

@laanwj laanwj added this to Blockers in High-priority for review Sep 6, 2017

src/wallet/wallet.cpp Outdated
InitError(strprintf(_("Error loading %s: You can't enable HD on an already existing non-HD wallet"), walletFile));
return nullptr;
}
InitError(_("Error: -usehd is no longer a valid option. HD Wallets cannot be disabled."));

This comment has been minimized.

@jonasschnelli

jonasschnelli Sep 6, 2017

Member

ultra-nit: usually I think we write "HD wallets".

@achow101 achow101 force-pushed the achow101:bump-wallet-version branch Sep 6, 2017

src/wallet/wallet.cpp Outdated
InitError(strprintf(_("Error loading %s: You can't enable HD on an already existing non-HD wallet"), walletFile));
return nullptr;
}
InitError(_("Error: -usehd is no longer a valid option. HD wallets cannot be disabled."));

This comment has been minimized.

@jonasschnelli

jonasschnelli Sep 6, 2017

Member

nit: maybe we should write "[Creating] HD wallets cannot be disabled". Otherwise it may confuse if you habe usehd=1 (or =0) in your config.

This comment has been minimized.

@sipa

sipa Sep 7, 2017

Member

I think the error should only be shown when the value passed to -usehd disagrees with the version number. That way setups with usehd=1 in the config file don't break. The error message could explain how to use -upgradewallet to enable HD on an existing non-HD wallet.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Sep 6, 2017

Tested ACK 62fad1266f4ab41453a86ca83141dc1b4791d20f (modulo possible nit)
https://bitcoin.jonasschnelli.ch/build/291

@TheBlueMatt
Copy link
Contributor

TheBlueMatt left a comment

utACK 62fad1266f4ab41453a86ca83141dc1b4791d20f

src/wallet/wallet.cpp Outdated
@@ -3829,17 +3829,13 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)

if (fFirstRun)
{
// Create new keyUser and set as default key
if (gArgs.GetBoolArg("-usehd", DEFAULT_USE_HD_WALLET) && !walletInstance->IsHDEnabled()) {
// ensure this wallet.dat can only be opened by clients supporting HD with chain split and has no default key

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Sep 6, 2017

Contributor

nit: s/has no/expecting no/

@@ -3829,17 +3829,13 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)

if (fFirstRun)
{
// Create new keyUser and set as default key
if (gArgs.GetBoolArg("-usehd", DEFAULT_USE_HD_WALLET) && !walletInstance->IsHDEnabled()) {

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Sep 6, 2017

Contributor

After discussion I believe the removal of the !IsHDEnabled check here is fine but I could totally see it breaking something very subtly, so would be good to get more eyeballs on this line.

This comment has been minimized.

@jonasschnelli

jonasschnelli Sep 7, 2017

Member

IMO the IsHDEnabled() check was there because you could salvage the wallet which may then would have triggered a fFirstRun and recreated a HD master key. But this is no longer true since fFirstRun is always false if there is a key in the wallet (and HD master key is also a standard key).

@laanwj laanwj self-assigned this Sep 7, 2017

achow101 added some commits Sep 5, 2017

Remove usehd option and warn when it is used
Removed the -usehd option so wallets cannot be made to be non-hd
anymore. A warning will be displayed when the option is set.

@achow101 achow101 force-pushed the achow101:bump-wallet-version branch to 713a920 Sep 7, 2017

@achow101

This comment has been minimized.

Copy link
Member Author

achow101 commented Sep 7, 2017

Comments addressed. I reverted the -usehd warning to just be the original warning with an additional statement for -usehd=0 option. It now says

You can't disable HD on an already existing HD wallet or create new non-HD wallets.

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Sep 8, 2017

utACK 713a920

@laanwj laanwj merged commit 713a920 into bitcoin:master Sep 8, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details

laanwj added a commit that referenced this pull request Sep 8, 2017

Merge #11250: Bump wallet version to 159900 and remove the `usehd` op…
…tion

713a920 Remove usehd option and warn when it is used (Andrew Chow)
d4c18f7 Bump wallet version number to 159900 (Andrew Chow)

Pull request description:

  Bump the wallet version number to 159900 so that new wallets made without a default key will no longer work on previous versions at all. Also remove the `usehd` option to avoid weird interaction with wallet version numbers and HD-ness of wallets.

Tree-SHA512: dd7965505bfad6a926c79afd423236f509229a398a8398076f8d57d90a5974243f9459a61225c4daee560c796f427445c9e55a3ad528a3a97a9123ca6a1269ab

@achow101 achow101 deleted the achow101:bump-wallet-version branch Sep 8, 2017

sipa added a commit that referenced this pull request Sep 8, 2017

Merge #11285: Add -usehd to excluded args in check-doc.py
7d03418 Add -usehd to excluded args in check-doc.py (MeshCollider)

Pull request description:

  All Travis builds on master are currently failing due to contrib/devtools/check-doc.py picking up `-usehd` in `src/wallet/wallet.cpp#L3845` as an undocumented argument (removed in #11250). Just need to add it to the list of unsupported, deprecated and duplicate args in check-doc.py so that it's ignored. Otherwise all builds on top of #11250 will fail until this is merged.

Tree-SHA512: 205c9be759b04bc3b85ac2b53fd455b3c0e229320d8e2b7f7d0ef5d5bd8033594b38a2d948250894ee2f4451584aca698476cd4b5cdf82955925683e3068a67c
@MarcoFalke
Copy link
Member

MarcoFalke left a comment

Post merge Concept ACK, but needs feedback addressed

@@ -3852,7 +3848,7 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
else if (gArgs.IsArgSet("-usehd")) {
bool useHD = gArgs.GetBoolArg("-usehd", DEFAULT_USE_HD_WALLET);
if (walletInstance->IsHDEnabled() && !useHD) {
InitError(strprintf(_("Error loading %s: You can't disable HD on an already existing HD wallet"), walletFile));
InitError(strprintf(_("Error loading %s: You can't disable HD on an already existing HD wallet or create new non-HD wallets."), walletFile));

This comment has been minimized.

@MarcoFalke

MarcoFalke Sep 10, 2017

Member

This new warning will only be printed in subsequent loads of the wallet (after the first start) since the code branch is never executed on first start.

@@ -29,7 +29,6 @@ std::string GetWalletHelpString(bool showDebug)
strUsage += HelpMessageOpt("-salvagewallet", _("Attempt to recover private keys from a corrupt wallet on startup"));
strUsage += HelpMessageOpt("-spendzeroconfchange", strprintf(_("Spend unconfirmed change when sending transactions (default: %u)"), DEFAULT_SPEND_ZEROCONF_CHANGE));
strUsage += HelpMessageOpt("-txconfirmtarget=<n>", strprintf(_("If paytxfee is not set, include enough fee so transactions begin confirmation on average within n blocks (default: %u)"), DEFAULT_TX_CONFIRM_TARGET));
strUsage += HelpMessageOpt("-usehd", _("Use hierarchical deterministic key generation (HD) after BIP32. Only has effect during wallet creation/first start") + " " + strprintf(_("(default: %u)"), DEFAULT_USE_HD_WALLET));

This comment has been minimized.

@MarcoFalke

MarcoFalke Sep 10, 2017

Member

Needs mention in the release notes and fixup for check-doc.py


def run_test (self):
tmpdir = self.options.tmpdir

# Make sure can't switch off usehd after wallet creation
self.stop_node(1)
self.assert_start_raises_init_error(1, ['-usehd=0'], 'already existing HD wallet')

This comment has been minimized.

@MarcoFalke

MarcoFalke Sep 10, 2017

Member

Why is this test removed?

This comment has been minimized.

@achow101

achow101 Sep 12, 2017

Author Member

Oops. That was removed originally when I had -usehd always return an error regardless.

@laanwj laanwj removed this from Blockers in High-priority for review Sep 14, 2017

@xor-freenet

This comment has been minimized.

Copy link

xor-freenet commented Mar 6, 2018

@achow101

Also remove the usehd option to avoid weird interaction with wallet version numbers and HD-ness of wallets.

Can you please clarify what that "weird interaction" is?
Thanks!

@achow101

This comment has been minimized.

Copy link
Member Author

achow101 commented Mar 6, 2018

The version number of a HD wallet is greater than or equal to 130000. But this makes the wallet version number 159900. So, by the previous version number constraint, all newly created wallets must be a HD wallet. Creating a non-HD wallet would mean that the version number has to be less than 130000, but then the new wallet cannot include the fix provided by the new version number. Thus we must either make some hack to allow new non-HD wallets sanely, or don't make new non-HD wallets at all. I chose the latter.

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Mar 6, 2018

@xor-freenet

This comment has been minimized.

Copy link

xor-freenet commented Mar 6, 2018

@achow101

The version number of a HD wallet is greater than or equal to 130000. But this makes the wallet version number 159900. So, by the previous version number constraint, all newly created wallets must be a HD wallet. Creating a non-HD wallet would mean that the version number has to be less than 130000, but then the new wallet cannot include the fix provided by the new version number. Thus we must either make some hack to allow new non-HD wallets sanely, or don't make new non-HD wallets at all. I chose the latter.

Thanks.
This sounds like a job for a simple "boolean isHD" stored somewhere in the wallet for all wallets > version 159900 to track the HD feature independently of the version number.
Can you please add that and re-add the ability to disable HD with -usehd=0?

@MarcoFalke

@xor-freenet See #11582

Thanks. I have read it but it doesn't end with the re-addition of -usehd=0 and the reasons given do not justify HD's security drawback of deriving all keys from a single piece of entropy - which is fixed by using non-HD wallets where each address has its own entropy.
That PR was closed because the author needed to fully disable key generation completely and that can be done by other means, so the subject of the security drawback of HD wallets was completely ignored. The PR wasn't really about usehd.

@xor-freenet

This comment has been minimized.

Copy link

xor-freenet commented Mar 6, 2018

@achow101
(Though I just said that #11582 wasn't really about usehd please notice I merely meant its unrelated goal of disabling key generation, it still does in fact provide code to re-add usehd which you can re-use.)

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Mar 6, 2018

If you don't have enough entropy to generate a single seed you surely won't have enough entropy to generate multiple (keypoolsize) keys.

@achow101

This comment has been minimized.

Copy link
Member Author

achow101 commented Mar 6, 2018

@xor-freenet Not to be rude, but if you want that feature, you can either implement it yourself or find someone to implement it for you. Of course just implementing it is not enough and you will need to defend why it is a good idea to allow non-HD wallets and how your implementation does not break backwards compatibility and is safe. I am not interested in doing that.

@xor-freenet

This comment has been minimized.

Copy link

xor-freenet commented Mar 6, 2018

@MarcoFalke
@achow101

If you don't have enough entropy to generate a single seed you surely won't have enough entropy to generate multiple (keypoolsize) keys.

The problem isn't a lack of entropy at key generation, the problem is that the single piece of entropy is going to be re-used for an indefinite amount of years for as long as a wallet is used.
Attacks on the crypto may arise where the attacker can derive the master key from a small set of multiple transactions of the victim wallet (or a bug may just leak the key into a single transaction). Then with HD all coins are gone in the wallet.

In other words: With non-HD wallets address re-use was greatly discouraged by the Bitcoin community because publishing a transaction leaks cryptographic information about the private key. Now with HD it's suddenly OK to re-use something which derives from the same key?

Even worse this also means that cold storage isn't really cold storage because key material is constantly leaked by every transaction you send from it by thumb drive.

@xor-freenet

This comment has been minimized.

Copy link

xor-freenet commented Mar 6, 2018

@achow101

@xor-freenet Not to be rude, but if you want that feature, you can either implement it yourself or find someone to implement it for you. Of course just implementing it is not enough and you will need to defend why it is a good idea to allow non-HD wallets and how your implementation does not break backwards compatibility and is safe. I am not interested in doing that.

Oh sorry my problem isn't that I wouldn't understand I should do it if I want it, it's merely that I'm more expertised in Java than C++ and it would take me much more time to implement it than for someone who frequently works with the Bitcoin codebase.
I can certainly shelf out e.g. a $50 bounty (paid in BTC of course) if that motivates you? If not feel free to make up a number :)

W.r.t. to backwards compatibility: Well the removal of usehd removed a command line option which worked in a previous version and now doesn't work in the current, isn't that the definition of backwards compatibility breakage?

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Mar 6, 2018

@xor-freenet Are you aware that you can create a wallet with a previous version (create a no-hd wallet) and use it with the current version?

https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes/release-notes-0.16.0.md#hd-wallets-by-default

@xor-freenet

This comment has been minimized.

Copy link

xor-freenet commented Mar 7, 2018

@MarcoFalke

@xor-freenet Are you aware that you can create a wallet with a previous version (create a no-hd wallet) and use it with the current version?

Yes, thank you. Requiring the usage of obsolete software versions to access security features is a mutual exclusion though, old software is insecure by definition.

Besides they won't stay available forever, it's entirely possible they stop working on e.g. recent Linux versions due to things changing about Linux. And repairing usehd won't be easier once the legacy versions have become unusable as the currently existing patches won't apply cleanly anymore due to merge conflicts.

I'd really be very very happy if this issue could be sorted out right away.
I fully accept that the usability benefit of HD wallet backups being permanently valid is good for newbies and thus HD should be the default.
However improved usability for newbies shouldn't hurt the security of professional use and thus it should be possible to disable HD.
Bitcoin Core will have to keep compatibility for non-HD keys forever anyway due to the large amount of existing funds stored in them, so it would be really sad if the feature wasn't accessible for new users anymore due to lack of a < 50 line patch to expose it to the UI.

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