feat: enable HD wallets by default (#5807)

## Issue being fixed or feature implemented HD wallets are old-existsing feature, appeared in Dash years ago, but enabling HD wallets is not trivial task that requires multiple steps and command line/rpc calls. Let's have them enabled by default. ## What was done? - HD wallets are enabled by default. Currently behavior `dashd`, `dash-qt` are similar to run with option `-usehd=1` - the rpc `upgradewallet` do not let to upgrade from non-HD wallet to HD wallet to don't encourage user use non-crypted wallets (postponed till v21) - the initialization of ScriptPubKey is updated to be sure that encypted HD seed is never written on disk (if passphrase is provided) - enabled and dashified a script `wallet_upgradewallet.py` which test compatibility between different versions of wallet ## What is not done? - wallet tool still does not support passhprase, HD seed can appear on disk - there's no dialog that show user a mnemonic phrase and encourage him to make a paper backup Before removing a command line 'usehd' (backport bitcoin#11250) need to make at least one major release for fail-over option (if someone wish to use non-HD wallets only). ## How Has This Been Tested? Run unit and functional tests. Enabled new functional test `wallet_upgradewallet.py` that has been backported long time ago but waited this PR to be enabled. ## Breaking Changes HD wallets are created by default. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone --------- Co-authored-by: UdjinM6 <[email protected]>
dashpay · Feb 9, 2024 · 8dba655 · 8dba655
1 parent 19681d0
commit 8dba655
Show file tree

Hide file tree

Showing 10 changed files with 154 additions and 139 deletions.
diff --git a/doc/release-notes-5807.md b/doc/release-notes-5807.md
@@ -0,0 +1,7 @@
+# Notable changes
+
+## HD Wallets Enabled by Default
+
+In this release, we are taking a significant step towards enhancing the Dash wallet's usability by enabling Hierarchical Deterministic (HD) wallets by default. This change aligns the behavior of `dashd` and `dash-qt` with the previously optional `-usehd=1` flag, making HD wallets the standard for all users.
+
+While HD wallets are now enabled by default to improve user experience, Dash Core still allows for the creation of non-HD wallets by using the `-usehd=0` flag. However, users should be aware that this option is intended for legacy support and will be removed in future releases. Importantly, even with an HD wallet, users can still import non-HD private keys, ensuring flexibility in managing their funds.
diff --git a/src/qt/forms/createwalletdialog.ui b/src/qt/forms/createwalletdialog.ui
@@ -50,7 +50,7 @@
       <string>Encrypt Wallet</string>
      </property>
      <property name="checked">
-      <bool>false</bool>
+      <bool>true</bool>
      </property>
     </widget>
    </item>

diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
@@ -2681,76 +2681,53 @@ static UniValue upgradetohd(const JSONRPCRequest& request)
         },
     }.Check(request);
 
-    std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
-    if (!wallet) return NullUniValue;
-    CWallet* const pwallet = wallet.get();
-    LegacyScriptPubKeyMan* spk_man = pwallet->GetLegacyScriptPubKeyMan();
-    if (!spk_man) {
-        throw JSONRPCError(RPC_WALLET_ERROR, "This type of wallet does not support this command");
-    }
+    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
+    if (!pwallet) return NullUniValue;
 
     bool generate_mnemonic = request.params[0].isNull() || request.params[0].get_str().empty();
-
-    {
-        LOCK(pwallet->cs_wallet);
-
-        // Do not do anything to HD wallets
-        if (pwallet->IsHDEnabled()) {
-            throw JSONRPCError(RPC_WALLET_ERROR, "Cannot upgrade a wallet to HD if it is already upgraded to HD.");
-        }
-
-        if (!pwallet->SetMaxVersion(FEATURE_HD)) {
-            throw JSONRPCError(RPC_WALLET_ERROR, "Cannot downgrade wallet");
+    SecureString secureWalletPassphrase;
+    secureWalletPassphrase.reserve(100);
+    // TODO: get rid of this .c_str() by implementing SecureString::operator=(std::string)
+    // Alternately, find a way to make request.params[0] mlock()'d to begin with.
+    if (!request.params[2].isNull()) {
+        secureWalletPassphrase = request.params[2].get_str().c_str();
+        if (!pwallet->Unlock(secureWalletPassphrase)) {
+            throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "The wallet passphrase entered was incorrect");
         }
+    }
 
-        if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
-            throw JSONRPCError(RPC_WALLET_ERROR, "Error: Private keys are disabled for this wallet");
-        }
+    EnsureWalletIsUnlocked(pwallet.get());
 
-        bool prev_encrypted = pwallet->IsCrypted();
+    SecureString secureMnemonic;
+    secureMnemonic.reserve(256);
+    if (!generate_mnemonic) {
+        secureMnemonic = request.params[0].get_str().c_str();
+    }
 
-        SecureString secureWalletPassphrase;
-        secureWalletPassphrase.reserve(100);
-        // TODO: get rid of this .c_str() by implementing SecureString::operator=(std::string)
-        // Alternately, find a way to make request.params[0] mlock()'d to begin with.
-        if (request.params[2].isNull()) {
-            if (prev_encrypted) {
-                throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Cannot upgrade encrypted wallet to HD without the wallet passphrase");
-            }
-        } else {
-            secureWalletPassphrase = request.params[2].get_str().c_str();
-            if (!pwallet->Unlock(secureWalletPassphrase)) {
-                throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "The wallet passphrase entered was incorrect");
-            }
-        }
+    SecureString secureMnemonicPassphrase;
+    secureMnemonicPassphrase.reserve(256);
+    if (!request.params[1].isNull()) {
+        secureMnemonicPassphrase = request.params[1].get_str().c_str();
+    }
+    // TODO: breaking changes kept for v21!
+    // instead upgradetohd let's use more straightforward 'sethdseed'
+    constexpr bool is_v21 = false;
+    const int previous_version{pwallet->GetVersion()};
+    if (is_v21 && previous_version >= FEATURE_HD) {
+        return JSONRPCError(RPC_WALLET_ERROR, "Already at latest version. Wallet version unchanged.");
+    }
 
-        SecureString secureMnemonic;
-        secureMnemonic.reserve(256);
-        if (!generate_mnemonic) {
-            secureMnemonic = request.params[0].get_str().c_str();
-        }
+    bilingual_str error;
+    const bool wallet_upgraded{pwallet->UpgradeToHD(secureMnemonic, secureMnemonicPassphrase, secureWalletPassphrase, error)};
 
-        SecureString secureMnemonicPassphrase;
-        secureMnemonicPassphrase.reserve(256);
-        if (!request.params[1].isNull()) {
-            secureMnemonicPassphrase = request.params[1].get_str().c_str();
+    if (!secureWalletPassphrase.empty() && !pwallet->IsCrypted()) {
+        if (!pwallet->EncryptWallet(secureWalletPassphrase)) {
+            throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Failed to encrypt HD wallet");
         }
+    }
 
-        pwallet->WalletLogPrintf("Upgrading wallet to HD\n");
-        pwallet->SetMinVersion(FEATURE_HD);
-
-        if (prev_encrypted) {
-            if (!pwallet->GenerateNewHDChainEncrypted(secureMnemonic, secureMnemonicPassphrase, secureWalletPassphrase)) {
-                throw JSONRPCError(RPC_WALLET_ERROR, "Failed to generate encrypted HD wallet");
-            }
-        } else {
-            spk_man->GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase);
-            if (!secureWalletPassphrase.empty()) {
-                if (!pwallet->EncryptWallet(secureWalletPassphrase)) {
-                    throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Failed to encrypt HD wallet");
-                }
-            }
-        }
+    if (!wallet_upgraded) {
+        throw JSONRPCError(RPC_WALLET_ERROR, error.original);
     }
 
     // If you are generating new mnemonic it is assumed that the addresses have never gotten a transaction before, so you don't need to rescan for transactions

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
@@ -297,6 +297,10 @@ std::shared_ptr<CWallet> CreateWallet(interfaces::Chain& chain, interfaces::Coin
         status = DatabaseStatus::FAILED_CREATE;
         return nullptr;
     }
+    if (gArgs.GetBoolArg("-usehd", DEFAULT_USE_HD_WALLET)) {
+        wallet->WalletLogPrintf("Set HD by default\n");
+        wallet->SetMinVersion(FEATURE_HD);
+    }
 
     // Encrypt the wallet
     if (!passphrase.empty() && !(wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
@@ -306,29 +310,25 @@ std::shared_ptr<CWallet> CreateWallet(interfaces::Chain& chain, interfaces::Coin
             return nullptr;
         }
         if (!create_blank) {
+            {
+                // TODO: drop this condition after removing option to create non-HD wallets
+                // related backport bitcoin#11250
+                if (wallet->GetVersion() >= FEATURE_HD) {
+                    if (!wallet->GenerateNewHDChainEncrypted(/*secureMnemonic=*/"", /*secureMnemonicPassphrase=*/"", passphrase)) {
+                       error = Untranslated("Error: Failed to generate encrypted HD wallet");
+                       status = DatabaseStatus::FAILED_CREATE;
+                       return nullptr;
+                    }
+                }
+            }
+
             // Unlock the wallet
             if (!wallet->Unlock(passphrase)) {
                 error = Untranslated("Error: Wallet was encrypted but could not be unlocked");
                 status = DatabaseStatus::FAILED_ENCRYPT;
                 return nullptr;
             }
 
-            // Set a HD chain for the wallet
-            // TODO: re-enable this and `keypoolsize_hd_internal` check in `wallet_createwallet.py`
-            // when HD is the default mode (make sure this actually works!)...
-            // if (auto spk_man = wallet->GetLegacyScriptPubKeyMan() {
-            //   if (!spk_man->GenerateNewHDChainEncrypted("", "", passphrase)) {
-            //     throw JSONRPCError(RPC_WALLET_ERROR, "Failed to generate encrypted HD wallet");
-            //   }
-            // }
-            // ... and drop this
-            LOCK(wallet->cs_wallet);
-            wallet->UnsetWalletFlag(WALLET_FLAG_BLANK_WALLET);
-            if (auto spk_man = wallet->GetLegacyScriptPubKeyMan()) {
-                spk_man->NewKeyPool();
-            }
-            // end TODO
-
             // backup the wallet we just encrypted
             if (!wallet->AutoBackupWallet("", error, warnings) && !error.original.empty()) {
                 status = DatabaseStatus::FAILED_ENCRYPT;
@@ -4617,6 +4617,10 @@ std::shared_ptr<CWallet> CWallet::Create(interfaces::Chain& chain, interfaces::C
             if (gArgs.GetBoolArg("-usehd", DEFAULT_USE_HD_WALLET) && !walletInstance->IsHDEnabled()) {
                 std::string strSeed = gArgs.GetArg("-hdseed", "not hex");
 
+                // ensure this wallet.dat can only be opened by clients supporting HD
+                walletInstance->WalletLogPrintf("Upgrading wallet to HD\n");
+                walletInstance->SetMinVersion(FEATURE_HD);
+
                 if (gArgs.IsArgSet("-hdseed") && IsHex(strSeed)) {
                     CHDChain newHdChain;
                     std::vector<unsigned char> vchSeed = ParseHex(strSeed);
@@ -4646,10 +4650,6 @@ std::shared_ptr<CWallet> CWallet::Create(interfaces::Chain& chain, interfaces::C
                     }
                 }
 
-                // ensure this wallet.dat can only be opened by clients supporting HD
-                walletInstance->WalletLogPrintf("Upgrading wallet to HD\n");
-                walletInstance->SetMinVersion(FEATURE_HD);
-
                 // clean up
                 gArgs.ForceRemoveArg("hdseed");
                 gArgs.ForceRemoveArg("mnemonic");
@@ -4913,7 +4913,45 @@ bool CWallet::UpgradeWallet(int version, bilingual_str& error)
         return false;
     }
 
+    // TODO: consider discourage users to skip passphrase for HD wallets for v21
+    if (false && nMaxVersion >= FEATURE_HD && !IsHDEnabled()) {
+        error = Untranslated("You should use upgradetohd RPC to upgrade non-HD wallet to HD");
+        return false;
+    }
+
     SetMaxVersion(nMaxVersion);
+
+    return true;
+}
+
+bool CWallet::UpgradeToHD(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, const SecureString& secureWalletPassphrase, bilingual_str& error)
+{
+    LOCK(cs_wallet);
+
+    // Do not do anything to HD wallets
+    if (IsHDEnabled()) {
+        error = Untranslated("Cannot upgrade a wallet to HD if it is already upgraded to HD.");
+        return false;
+    }
+
+    if (IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
+        error = Untranslated("Private keys are disabled for this wallet");
+        return false;
+    }
+
+    WalletLogPrintf("Upgrading wallet to HD\n");
+    SetMinVersion(FEATURE_HD);
+
+    auto spk_man = GetOrCreateLegacyScriptPubKeyMan();
+    bool prev_encrypted = IsCrypted();
+    if (prev_encrypted) {
+        if (!GenerateNewHDChainEncrypted(secureMnemonic, secureMnemonicPassphrase, secureWalletPassphrase)) {
+            error = Untranslated("Failed to generate encrypted HD wallet");
+            return false;
+        }
+    } else {
+        spk_man->GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase);
+    }
     return true;
 }
 

diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
@@ -91,7 +91,7 @@ static const CAmount HIGH_TX_FEE_PER_KB = COIN / 100;
 static const CAmount HIGH_MAX_TX_FEE = 100 * HIGH_TX_FEE_PER_KB;
 
 //! if set, all keys will be derived by using BIP39/BIP44
-static const bool DEFAULT_USE_HD_WALLET = false;
+static const bool DEFAULT_USE_HD_WALLET = true;
 
 class CCoinControl;
 class CKey;
@@ -1279,6 +1279,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
     /* Returns true if HD is enabled */
     bool IsHDEnabled() const;
 
+    // TODO: move it to scriptpubkeyman
     /* Generates a new HD chain */
     bool GenerateNewHDChainEncrypted(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, const SecureString& secureWalletPassphrase);
 
@@ -1331,6 +1332,9 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
     /** Upgrade the wallet */
     bool UpgradeWallet(int version, bilingual_str& error);
 
+    /** Upgrade non-HD wallet to HD wallet */
+    bool UpgradeToHD(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, const SecureString& secureWalletPassphrase, bilingual_str& error);
+
     //! Returns all unique ScriptPubKeyMans in m_internal_spk_managers and m_external_spk_managers
     std::set<ScriptPubKeyMan*> GetActiveScriptPubKeyMans() const;
 

diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp
@@ -28,9 +28,11 @@ static void WalletCreate(CWallet* wallet_instance)
 
     // generate a new HD seed
     wallet_instance->SetupLegacyScriptPubKeyMan();
-    // NOTE: we do not yet create HD wallets by default
-    // auto spk_man = wallet_instance->GetLegacyScriptPubKeyMan();
-    // spk_man->GenerateNewHDChain("", "");
+    auto spk_man = wallet_instance->GetLegacyScriptPubKeyMan();
+    // NOTE: drop this condition after removing option to create non-HD wallets
+    if (spk_man->IsHDEnabled()) {
+        spk_man->GenerateNewHDChain(/*secureMnemonic=*/"", /*secureMnemonicPassphrase=*/"");
+    }
 
     tfm::format(std::cout, "Topping up keypool...\n");
     wallet_instance->TopUpKeyPool();

diff --git a/test/functional/tool_wallet.py b/test/functional/tool_wallet.py
@@ -97,21 +97,6 @@ def test_tool_wallet_info(self):
         # shasum_before = self.wallet_shasum()
         timestamp_before = self.wallet_timestamp()
         self.log.debug('Wallet file timestamp before calling info: {}'.format(timestamp_before))
-        out = textwrap.dedent('''\
-            Wallet info
-            ===========
-            Encrypted: no
-            HD (hd seed available): no
-            Keypool Size: 1
-            Transactions: 0
-            Address Book: 1
-        ''')
-        self.assert_tool_output(out, '-wallet=' + self.default_wallet_name, 'info')
-
-        self.start_node(0)
-        self.nodes[0].upgradetohd()
-        self.stop_node(0)
-
         out = textwrap.dedent('''\
             Wallet info
             ===========
@@ -122,6 +107,7 @@ def test_tool_wallet_info(self):
             Address Book: 1
         ''')
         self.assert_tool_output(out, '-wallet=' + self.default_wallet_name, 'info')
+
         timestamp_after = self.wallet_timestamp()
         self.log.debug('Wallet file timestamp after calling info: {}'.format(timestamp_after))
         self.log_wallet_timestamp_comparison(timestamp_before, timestamp_after)

diff --git a/test/functional/wallet_createwallet.py b/test/functional/wallet_createwallet.py
@@ -113,9 +113,7 @@ def run_test(self):
         # There should only be 1 key
         walletinfo = w6.getwalletinfo()
         assert_equal(walletinfo['keypoolsize'], 1)
-        # TODO: re-enable  this when HD is the default mode
-        # assert_equal(walletinfo['keypoolsize_hd_internal'], 1)
-        # end TODO
+        assert_equal(walletinfo['keypoolsize_hd_internal'], 1)
         # Allow empty passphrase, but there should be a warning
         resp = self.nodes[0].createwallet(wallet_name='w7', disable_private_keys=False, blank=False, passphrase='')
         assert_equal(resp['warning'], 'Empty string given as passphrase, wallet will not be encrypted.')

diff --git a/test/functional/wallet_upgradetohd.py b/test/functional/wallet_upgradetohd.py
@@ -21,12 +21,13 @@
 class WalletUpgradeToHDTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 1
+        self.extra_args = [['-usehd=0']]
 
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
 
     def setup_network(self):
-        self.add_nodes(self.num_nodes)
+        self.add_nodes(self.num_nodes, self.extra_args)
         self.start_nodes()
         self.import_deterministic_coinbase_privkeys()
 
@@ -69,7 +70,8 @@ def run_test(self):
         self.log.info("Should no longer be able to start it with HD disabled")
         self.stop_node(0)
         node.assert_start_raises_init_error(['-usehd=0'], "Error: Error loading %s: You can't disable HD on an already existing HD wallet" % self.default_wallet_name)
-        self.start_node(0)
+        self.extra_args = []
+        self.start_node(0, [])
         balance_after = node.getbalance()
 
         self.recover_non_hd()
@@ -188,7 +190,7 @@ def run_test(self):
         node.stop()
         node.wait_until_stopped()
         self.start_node(0, extra_args=['-rescan'])
-        assert_raises_rpc_error(-14, "Cannot upgrade encrypted wallet to HD without the wallet passphrase", node.upgradetohd, mnemonic)
+        assert_raises_rpc_error(-13, "Error: Please enter the wallet passphrase with walletpassphrase first.", node.upgradetohd, mnemonic)
         assert_raises_rpc_error(-14, "The wallet passphrase entered was incorrect", node.upgradetohd, mnemonic, "", "wrongpass")
         assert node.upgradetohd(mnemonic, "", walletpass)
         assert_raises_rpc_error(-13, "Error: Please enter the wallet passphrase with walletpassphrase first.", node.dumphdinfo)