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

achow101 commented at 11:15 PM on September 5, 2017: member

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 added the label Wallet on Sep 5, 2017

in src/wallet/wallet.cpp:3853 in b17ae3cab4 outdated

3844 | @@ -3849,17 +3845,6 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
3845 |  
3846 |          walletInstance->SetBestChain(chainActive.GetLocator());
3847 |      }
3848 | -    else if (gArgs.IsArgSet("-usehd")) {
3849 | -        bool useHD = gArgs.GetBoolArg("-usehd", DEFAULT_USE_HD_WALLET);

laanwj commented at 11:23 PM on September 5, 2017:

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

achow101 commented at 11:52 PM on September 5, 2017:

Done

laanwj commented at 11:24 PM on September 5, 2017: member

The option needs to be removed from GetWalletHelpString help too

MarcoFalke added the label Needs release notes on Sep 5, 2017

MarcoFalke commented at 11:28 PM on September 5, 2017: member

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

achow101 force-pushed on Sep 5, 2017

laanwj commented at 12:06 AM on September 6, 2017: member

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 added this to the "Blockers" column in a project

in src/wallet/wallet.cpp:3849 in b6ef6430cd outdated

3853 | -        }
3854 | -        if (!walletInstance->IsHDEnabled() && useHD) {
3855 | -            InitError(strprintf(_("Error loading %s: You can't enable HD on an already existing non-HD wallet"), walletFile));
3856 | -            return nullptr;
3857 | -        }
3858 | +        InitError(_("Error: -usehd is no longer a valid option. HD Wallets cannot be disabled."));

jonasschnelli commented at 8:58 PM on September 6, 2017:

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

achow101 force-pushed on Sep 6, 2017

in src/wallet/wallet.cpp:3849 in 62fad1266f outdated

3853 | -        }
3854 | -        if (!walletInstance->IsHDEnabled() && useHD) {
3855 | -            InitError(strprintf(_("Error loading %s: You can't enable HD on an already existing non-HD wallet"), walletFile));
3856 | -            return nullptr;
3857 | -        }
3858 | +        InitError(_("Error: -usehd is no longer a valid option. HD wallets cannot be disabled."));

jonasschnelli commented at 10:31 PM on September 6, 2017:

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.

sipa commented at 5:44 PM on September 7, 2017:

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 commented at 10:32 PM on September 6, 2017: contributor

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

in src/wallet/wallet.cpp:3832 in 62fad1266f outdated

3828 | @@ -3829,17 +3829,13 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
3829 |  
3830 |      if (fFirstRun)
3831 |      {
3832 | -        // Create new keyUser and set as default key
3833 | -        if (gArgs.GetBoolArg("-usehd", DEFAULT_USE_HD_WALLET) && !walletInstance->IsHDEnabled()) {
3834 | +        // ensure this wallet.dat can only be opened by clients supporting HD with chain split and has no default key

TheBlueMatt commented at 11:13 PM on September 6, 2017:

nit: s/has no/expecting no/

in src/wallet/wallet.cpp:3833 in 62fad1266f outdated

3828 | @@ -3829,17 +3829,13 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
3829 |  
3830 |      if (fFirstRun)
3831 |      {
3832 | -        // Create new keyUser and set as default key
3833 | -        if (gArgs.GetBoolArg("-usehd", DEFAULT_USE_HD_WALLET) && !walletInstance->IsHDEnabled()) {

TheBlueMatt commented at 11:44 PM on September 6, 2017:

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.

jonasschnelli commented at 5:25 PM on September 7, 2017:

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

TheBlueMatt commented at 11:44 PM on September 6, 2017: member

utACK 62fad1266f4ab41453a86ca83141dc1b4791d20f

laanwj assigned laanwj on Sep 7, 2017

Bump wallet version number to 159900 d4c18f7330

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.

713a92073b

achow101 force-pushed on Sep 7, 2017

achow101 commented at 11:39 PM on September 7, 2017: member

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 commented at 12:00 AM on September 8, 2017: contributor

utACK 713a92073b3b391f48d4fa146bd334893ec94b8f

laanwj merged this on Sep 8, 2017

laanwj closed this on Sep 8, 2017

laanwj referenced this in commit c22a53cd63 on Sep 8, 2017

achow101 deleted the branch on Sep 8, 2017

sipa referenced this in commit 3255d6347b on Sep 8, 2017

in src/wallet/wallet.cpp:3851 in 713a92073b

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

MarcoFalke commented at 3:26 AM on September 10, 2017:

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.

in src/wallet/init.cpp:32 in 713a92073b

  28 | @@ -29,7 +29,6 @@ std::string GetWalletHelpString(bool showDebug)
  29 |      strUsage += HelpMessageOpt("-salvagewallet", _("Attempt to recover private keys from a corrupt wallet on startup"));
  30 |      strUsage += HelpMessageOpt("-spendzeroconfchange", strprintf(_("Spend unconfirmed change when sending transactions (default: %u)"), DEFAULT_SPEND_ZEROCONF_CHANGE));
  31 |      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));
  32 | -    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));

MarcoFalke commented at 3:26 AM on September 10, 2017:

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

in test/functional/wallet-hd.py:25 in 713a92073b

  21 |      def run_test (self):
  22 |          tmpdir = self.options.tmpdir
  23 |  
  24 | -        # Make sure can't switch off usehd after wallet creation
  25 | -        self.stop_node(1)
  26 | -        self.assert_start_raises_init_error(1, ['-usehd=0'], 'already existing HD wallet')

MarcoFalke commented at 3:27 AM on September 10, 2017:

Why is this test removed?

achow101 commented at 1:43 PM on September 12, 2017:

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

MarcoFalke commented at 3:28 AM on September 10, 2017: member

Post merge Concept ACK, but needs feedback addressed

laanwj removed this from the "Blockers" column in a project

xor-freenet commented at 10:03 PM on March 6, 2018: none

@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 commented at 10:09 PM on March 6, 2018: member

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 commented at 10:11 PM on March 6, 2018: member

@xor-freenet See #11582

xor-freenet commented at 10:26 PM on March 6, 2018: none

@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 commented at 10:30 PM on March 6, 2018: none

@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 commented at 10:34 PM on March 6, 2018: member

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 commented at 10:35 PM on March 6, 2018: member

@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 commented at 10:40 PM on March 6, 2018: none

@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 commented at 10:45 PM on March 6, 2018: none

@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 commented at 11:03 PM on March 6, 2018: member

@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 commented at 11:30 AM on March 7, 2018: none

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

sipa removed the label Needs release notes on Aug 14, 2018

MarcoFalke locked this on Sep 8, 2021