From 09fd49832caff8b6da266efc8287dd3625e60950 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Mon, 8 Jan 2024 20:59:22 +0700 Subject: [PATCH] feat: cummulative changes from #5807 use HD wallets by default --- src/qt/forms/createwalletdialog.ui | 2 +- src/wallet/rpcwallet.cpp | 97 ++++++++++--------------- src/wallet/wallet.cpp | 74 ++++++++++++++----- src/wallet/wallet.h | 6 +- src/wallet/wallettool.cpp | 8 +- test/functional/tool_wallet.py | 16 +--- test/functional/wallet_createwallet.py | 4 +- test/functional/wallet_upgradetohd.py | 8 +- test/functional/wallet_upgradewallet.py | 67 ++++++++--------- 9 files changed, 145 insertions(+), 137 deletions(-) diff --git a/src/qt/forms/createwalletdialog.ui b/src/qt/forms/createwalletdialog.ui index 7002d1f6f06862..2639f551b11778 100644 --- a/src/qt/forms/createwalletdialog.ui +++ b/src/qt/forms/createwalletdialog.ui @@ -50,7 +50,7 @@ Encrypt Wallet - false + true diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 55d9810745eedf..646a9cd53cdd0c 100644 --- 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 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 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 index 9cbe5bbcecd97b..37c943c90a33d7 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -297,6 +297,10 @@ std::shared_ptr CreateWallet(interfaces::Chain& chain, interfaces::Coin status = DatabaseStatus::FAILED_CREATE; return nullptr; } + if (gArgs.GetBoolArg("-usehd", DEFAULT_USE_HD_WALLET)) { + LogPrintf("set HD by default\n"); + wallet->SetMinVersion(FEATURE_HD); + } // Encrypt the wallet if (!passphrase.empty() && !(wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { @@ -313,21 +317,17 @@ std::shared_ptr CreateWallet(interfaces::Chain& chain, interfaces::Coin 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(); + { + // TODO: drop this condition after removing option to create non-HD wallets + // related backport bitcoin#11250 + if (wallet->GetVersion() >= FEATURE_HD) { + if (!wallet->GenerateNewHDChainEncrypted("", "", passphrase)) { + error = Untranslated("Error: Failed to generate encrypted HD wallet"); + status = DatabaseStatus::FAILED_CREATE; + return nullptr; + } + } } - // end TODO // backup the wallet we just encrypted if (!wallet->AutoBackupWallet("", error, warnings) && !error.original.empty()) { @@ -4609,6 +4609,10 @@ std::shared_ptr 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 vchSeed = ParseHex(strSeed); @@ -4638,10 +4642,6 @@ std::shared_ptr 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"); @@ -4905,7 +4905,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 index 31590aa736c879..0d69c18e4a6ac1 100644 --- 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 GetActiveScriptPubKeyMans() const; diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp index e69a94b9734121..5cf81b3cae0c4c 100644 --- 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("", ""); + } 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 index eab68d49f3469b..d82fee2a0ad198 100755 --- 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 index bb1afe87304f76..61af66ee7fccf1 100755 --- 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 index d7af8099607e72..64cf39cf57a399 100755 --- 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) diff --git a/test/functional/wallet_upgradewallet.py b/test/functional/wallet_upgradewallet.py index b3e02003292d78..88df2951fab677 100755 --- a/test/functional/wallet_upgradewallet.py +++ b/test/functional/wallet_upgradewallet.py @@ -12,10 +12,11 @@ import shutil from test_framework.blocktools import COINBASE_MATURITY -from test_framework.test_framework import (BitcoinTestFramework, SkipTest) +from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, assert_greater_than, + assert_greater_than_or_equal, assert_is_hex_string, ) @@ -25,27 +26,22 @@ def set_test_params(self): self.setup_clean_chain = True self.num_nodes = 3 self.extra_args = [ - [], # current wallet version - ["-usehd=1"], # v0.16.3 wallet - ["-usehd=0"] # v0.15.2 wallet + [], # current wallet version + ["-usehd=1"], # v18.2.2 wallet + ["-usehd=0"] # v0.16.1.1 wallt ] - self.wallet_names = [self.default_wallet_name] + self.wallet_names = [self.default_wallet_name, None, None] def skip_test_if_missing_module(self): self.skip_if_no_wallet() self.skip_if_no_bdb() self.skip_if_no_previous_releases() - # TODO: this test doesn't work yet - raise SkipTest("Test wallet_upgradewallet.py is not adapted for Dash Core yet.") - - def setup_network(self): - self.setup_nodes() def setup_nodes(self): self.add_nodes(self.num_nodes, extra_args=self.extra_args, versions=[ None, - 160300, - 150200, + 18020200, # that's last version before `default wallets` are created + 160101, # that's oldest version that support `import_deterministic_coinbase_privkeys` ]) self.start_nodes() self.import_deterministic_coinbase_privkeys() @@ -61,13 +57,13 @@ def dumb_sync_blocks(self): Further info: https://github.com/bitcoin/bitcoin/pull/18774#discussion_r416967844 """ node_from = self.nodes[0] - v16_3_node = self.nodes[1] + v18_2_node = self.nodes[1] to_height = node_from.getblockcount() height = self.nodes[1].getblockcount() for i in range(height, to_height+1): b = node_from.getblock(blockhash=node_from.getblockhash(i), verbose=0) - v16_3_node.submitblock(b) - assert_equal(v16_3_node.getblockcount(), to_height) + v18_2_node.submitblock(b) + assert_equal(v18_2_node.getblockcount(), to_height) def run_test(self): self.nodes[0].generatetoaddress(COINBASE_MATURITY + 1, self.nodes[0].getnewaddress()) @@ -76,28 +72,28 @@ def run_test(self): res = self.nodes[0].getblockchaininfo() assert_equal(res['blocks'], COINBASE_MATURITY + 1) node_master = self.nodes[0] - v16_3_node = self.nodes[1] - v15_2_node = self.nodes[2] + v18_2_node = self.nodes[1] + v16_1_node = self.nodes[2] # Send coins to old wallets for later conversion checks. - v16_3_wallet = v16_3_node.get_wallet_rpc('wallet.dat') - v16_3_address = v16_3_wallet.getnewaddress() - node_master.generatetoaddress(COINBASE_MATURITY + 1, v16_3_address) + v18_2_wallet = v18_2_node.get_wallet_rpc(self.default_wallet_name) + v18_2_address = v18_2_wallet.getnewaddress() + node_master.generatetoaddress(COINBASE_MATURITY + 1, v18_2_address) self.dumb_sync_blocks() - v16_3_balance = v16_3_wallet.getbalance() + v18_2_balance = v18_2_wallet.getbalance() self.log.info("Test upgradewallet RPC...") # Prepare for copying of the older wallet node_master_wallet_dir = os.path.join(node_master.datadir, "regtest/wallets") - v16_3_wallet = os.path.join(v16_3_node.datadir, "regtest/wallets/wallet.dat") - v15_2_wallet = os.path.join(v15_2_node.datadir, "regtest/wallet.dat") + v18_2_wallet = os.path.join(v18_2_node.datadir, "regtest/wallets/wallet.dat") + v16_1_wallet = os.path.join(v16_1_node.datadir, "regtest/wallets/wallet.dat") self.stop_nodes() # Copy the 0.16.3 wallet to the last Dash Core version and open it: shutil.rmtree(node_master_wallet_dir) os.mkdir(node_master_wallet_dir) shutil.copy( - v16_3_wallet, + v18_2_wallet, node_master_wallet_dir ) self.restart_node(0, ['-nowallet']) @@ -111,16 +107,16 @@ def run_test(self): assert_equal(wallet.upgradewallet(), "") new_version = wallet.getwalletinfo()["walletversion"] # upgraded wallet version should be greater than older one - assert_greater_than(new_version, old_version) + assert_greater_than_or_equal(new_version, old_version) # wallet should still contain the same balance - assert_equal(wallet.getbalance(), v16_3_balance) + assert_equal(wallet.getbalance(), v18_2_balance) self.stop_node(0) - # Copy the 0.15.2 wallet to the last Dash Core version and open it: + # Copy the 19.3.0 wallet to the last Dash Core version and open it: shutil.rmtree(node_master_wallet_dir) os.mkdir(node_master_wallet_dir) shutil.copy( - v15_2_wallet, + v16_1_wallet, node_master_wallet_dir ) self.restart_node(0, ['-nowallet']) @@ -131,12 +127,17 @@ def run_test(self): assert_equal('hdseedid' in wallet.getwalletinfo(), False) # calling upgradewallet with explicit version number # should return nothing if successful - assert_equal(wallet.upgradewallet(169900), "") + assert_equal(wallet.upgradewallet(120200), "") + + new_version = wallet.getwalletinfo()["walletversion"] + # upgraded wallet would not have 120200 version until HD seed actually appeared + assert_greater_than(120200, new_version) + # after conversion master key hash should not be present yet + assert 'hdchainid' not in wallet.getwalletinfo() + assert_equal(wallet.upgradetohd(), True) new_version = wallet.getwalletinfo()["walletversion"] - # upgraded wallet should have version 169900 - assert_equal(new_version, 169900) - # after conversion master key hash should be present - assert_is_hex_string(wallet.getwalletinfo()['hdseedid']) + assert_equal(new_version, 120200) + assert_is_hex_string(wallet.getwalletinfo()['hdchainid']) if __name__ == '__main__': UpgradeWalletTest().main()