wallet: Display non-HD error on first run #11307

1. 
   MarcoFalke commented at 10:49 AM on September 12, 2017: member

   On current master a fresh wallet created with -usehd=0 is silently created as HD wallet. An error should be displayed on the first run.

   Also, this restores a test that was removed in c22a53c

   Fixes: #11313

2. wallet: Display non-HD error on first run fadf31ef02
3. MarcoFalke added the label Wallet on Sep 12, 2017
4. 
   meshcollider commented at 11:00 AM on September 12, 2017: contributor

   utACK https://github.com/bitcoin/bitcoin/pull/11307/commits/fadf31ef02b35f438ce2cf7048830f6974cdb515

5. in src/wallet/wallet.cpp:3849 in fadf31ef02

   3846 | @@ -3843,9 +3847,9 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
   3847 |          walletInstance->SetBestChain(chainActive.GetLocator());
   3848 |      }
   3849 |      else if (gArgs.IsArgSet("-usehd")) {
   

   ---

   ---

   

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

   Instead of moving the code block up, couldn't this just be made an if instead of else if? If we want to show this error before doing any of the wallet creating stuff, this entire check could just be moved to before the first run block.

   ---

   

   MarcoFalke commented at 2:29 PM on September 12, 2017:

   Yes, the else can be removed for better clarity. However, I'd still prefer that the new-non-hd-error is guarded by first-run. This way we can return early and don't have to deal with TopUpKeyPool, which might take a "long" time...

   I don't think this whole check can be moved to before the first run block, as IsHDEnabled should be called after (a potential) SetHDMasterKey.

6. 
   achow101 commented at 1:42 PM on September 12, 2017: member

   ~Concept ACK~

   utACK fadf31ef02b35f438ce2cf7048830f6974cdb515

7. 
   TheBlueMatt commented at 2:49 PM on September 12, 2017: member

   utACK fadf31ef02b35f438ce2cf7048830f6974cdb515

8. in src/wallet/wallet.cpp:3831 in fadf31ef02

   3826 | @@ -3827,6 +3827,10 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
   3827 |      if (fFirstRun)
   3828 |      {
   3829 |          // ensure this wallet.dat can only be opened by clients supporting HD with chain split and expects no default key
   3830 | +        if (!gArgs.GetBoolArg("-usehd", true)) {
   3831 | +            InitError(strprintf(_("Error creating %s: You can't create non-HD wallets with this version."), walletFile));
   

   ---

   ---

   

   promag commented at 4:43 PM on September 12, 2017:

   Missing test?

9. in src/wallet/wallet.cpp:3832 in fadf31ef02

   3826 | @@ -3827,6 +3827,10 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
   3827 |      if (fFirstRun)
   3828 |      {
   3829 |          // ensure this wallet.dat can only be opened by clients supporting HD with chain split and expects no default key
   3830 | +        if (!gArgs.GetBoolArg("-usehd", true)) {
   3831 | +            InitError(strprintf(_("Error creating %s: You can't create non-HD wallets with this version."), walletFile));
   3832 | +            return nullptr;
   

   ---

   ---

   

   promag commented at 4:45 PM on September 12, 2017:

   Is it me or walletInstance is leaked?

   ---

   

   achow101 commented at 10:57 PM on September 12, 2017:

   I believe it is, but that is not a problem since the program just exits here. The same behavior happens earlier too with other InitErrors.

10. 
    achow101 commented at 3:33 PM on September 15, 2017: member

    This needs a 0.16.0 milestone

11. jonasschnelli added this to the milestone 0.16.0 on Sep 15, 2017
12. 
    jonasschnelli commented at 6:07 PM on September 15, 2017: contributor

    utACK fadf31ef02b35f438ce2cf7048830f6974cdb515

    Added 0.16 milestone.

13. 
    instagibbs commented at 6:59 PM on September 18, 2017: member

    is there a link to the relevant issue?

14. 
    MarcoFalke commented at 7:11 PM on September 18, 2017: member

    @instagibbs You can try it yourself with -usehd=0 or see #11313

15. 
    instagibbs commented at 7:20 PM on September 18, 2017: member

    Thanks, having the relevant issue in the PR description helps.

16. laanwj merged this on Sep 19, 2017
17. laanwj closed this on Sep 19, 2017

    ---

18. laanwj referenced this in commit 4f7e37e26c on Sep 19, 2017
19. MarcoFalke deleted the branch on Sep 19, 2017
20. MarcoFalke locked this on Sep 8, 2021

Contributors

MarcoFalkemeshcolliderachow101TheBlueMattpromagjonasschnelliinstagibbs

Milestone
0.16.0