From b19f4635634564a36add8f56767c816c36a27773 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Mon, 4 Aug 2025 00:23:13 +0300 Subject: [PATCH 1/2] fix(rpc): return correct error codes in `upgradetohd` rpc --- src/wallet/rpc/wallet.cpp | 122 ++++++++++++++++++-------- src/wallet/wallet.cpp | 118 +++---------------------- src/wallet/wallet.h | 7 -- test/functional/wallet_upgradetohd.py | 5 +- 4 files changed, 97 insertions(+), 155 deletions(-) diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index 9a3cc0c288e8c..27bbbe569cb3f 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -371,51 +371,101 @@ static RPCHelpMan upgradetohd() if (!pwallet) return NullUniValue; bool generate_mnemonic = request.params[0].isNull() || request.params[0].get_str().empty(); - SecureString secureWalletPassphrase; - secureWalletPassphrase.reserve(100); - if (request.params[2].isNull()) { - if (pwallet->IsCrypted()) { - throw JSONRPCError(RPC_WALLET_UNLOCK_NEEDED, "Error: Wallet encrypted but passphrase not supplied to RPC."); + { + LOCK(pwallet->cs_wallet); + + SecureString secureWalletPassphrase; + secureWalletPassphrase.reserve(100); + + if (request.params[2].isNull()) { + if (pwallet->IsCrypted()) { + throw JSONRPCError(RPC_WALLET_UNLOCK_NEEDED, "Error: Wallet encrypted but passphrase not supplied to RPC."); + } + } else { + // 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. + secureWalletPassphrase = request.params[2].get_str().c_str(); } - } else { - // 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. - secureWalletPassphrase = request.params[2].get_str().c_str(); - } - SecureString secureMnemonic; - secureMnemonic.reserve(256); - if (!generate_mnemonic) { - secureMnemonic = request.params[0].get_str().c_str(); - } + SecureString secureMnemonic; + secureMnemonic.reserve(256); + if (!generate_mnemonic) { + secureMnemonic = request.params[0].get_str().c_str(); + } - SecureString secureMnemonicPassphrase; - secureMnemonicPassphrase.reserve(256); - if (!request.params[1].isNull()) { - secureMnemonicPassphrase = request.params[1].get_str().c_str(); - } + 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."); - } + // 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."); + } - bilingual_str error; - const bool wallet_upgraded{pwallet->UpgradeToHD(secureMnemonic, secureMnemonicPassphrase, secureWalletPassphrase, error)}; + // 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 (!secureWalletPassphrase.empty() && !pwallet->IsCrypted()) { - if (!pwallet->EncryptWallet(secureWalletPassphrase)) { - throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Failed to encrypt HD wallet"); + if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { + throw JSONRPCError(RPC_WALLET_ERROR, "Private keys are disabled for this wallet"); } - } - if (!wallet_upgraded) { - throw JSONRPCError(RPC_WALLET_ERROR, error.original); - } + pwallet->WalletLogPrintf("Upgrading wallet to HD\n"); + pwallet->SetMinVersion(FEATURE_HD); + + bool is_locked = pwallet->IsLocked(); + + if (pwallet->IsCrypted()) { + if (secureWalletPassphrase.empty()) { + throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: Wallet encrypted but supplied empty wallet passphrase"); + } + + // We are intentionally re-locking the wallet so we can validate passphrase + // by verifying if it can unlock the wallet + pwallet->Lock(); + + // Unlock the wallet + if (!pwallet->Unlock(secureWalletPassphrase)) { + throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The wallet passphrase entered was incorrect"); + } + } + + if (pwallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { + pwallet->SetupDescriptorScriptPubKeyMans(secureMnemonic, secureMnemonicPassphrase); + } else { + auto spk_man = pwallet->GetLegacyScriptPubKeyMan(); + if (!spk_man) { + throw JSONRPCError(RPC_WALLET_ERROR, "Error: Legacy ScriptPubKeyMan is not available"); + } + + if (pwallet->IsCrypted()) { + pwallet->WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + spk_man->GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase, encryption_key); + return true; + }); + } else { + spk_man->GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase); + } + } + + if (is_locked) { + // Relock the wallet + pwallet->Lock(); + } + + if (!secureWalletPassphrase.empty() && !pwallet->IsCrypted()) { + if (!pwallet->EncryptWallet(secureWalletPassphrase)) { + throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Failed to encrypt HD wallet"); + } + } + } // pwallet->cs_wallet // 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 bool rescan = request.params[3].isNull() ? !generate_mnemonic : request.params[3].get_bool(); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 3c939bcca018e..07b8d0e956537 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -353,11 +353,17 @@ std::shared_ptr CreateWallet(WalletContext& context, const std::string& // TODO: drop this condition after removing option to create non-HD wallets // related backport bitcoin#11250 if (wallet->GetVersion() >= FEATURE_HD) { - if (!wallet->GenerateNewHDChain(/*secureMnemonic=*/"", /*secureMnemonicPassphrase=*/"", passphrase)) { - error = Untranslated("Error: Failed to generate encrypted HD wallet"); - status = DatabaseStatus::FAILED_CREATE; - return nullptr; + auto spk_man = wallet->GetLegacyScriptPubKeyMan(); + if (!spk_man) { + error = Untranslated("Error: Legacy ScriptPubKeyMan is not available"); + status = DatabaseStatus::FAILED_ENCRYPT; + return nullptr; } + + wallet->WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + spk_man->GenerateNewHDChain(/*secureMnemonic=*/"", /*secureMnemonicPassphrase=*/"", encryption_key); + return true; + }); } } @@ -3203,52 +3209,6 @@ bool CWallet::UpgradeWallet(int version, bilingual_str& error) 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); - - if (IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { - if (IsCrypted()) { - if (secureWalletPassphrase.empty()) { - error = Untranslated("Error: Wallet encrypted but supplied empty wallet passphrase"); - return false; - } - - // Unlock the wallet - if (!Unlock(secureWalletPassphrase)) { - error = Untranslated("Error: The wallet passphrase entered was incorrect"); - return false; - } - } - SetupDescriptorScriptPubKeyMans(secureMnemonic, secureMnemonicPassphrase); - - if (IsCrypted()) { - // Relock the wallet - Lock(); - } - } else { - if (!GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase, secureWalletPassphrase)) { - error = Untranslated("Failed to generate HD wallet"); - return false; - } - } - return true; -} - const CAddressBookData* CWallet::FindAddressBookEntry(const CTxDestination& dest, bool allow_change) const { const auto& address_book_it = m_address_book.find(dest); @@ -3779,64 +3739,6 @@ void CWallet::ConnectScriptPubKeyManNotifiers() } } -bool CWallet::GenerateNewHDChain(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, const SecureString& secureWalletPassphrase) -{ - auto spk_man = GetLegacyScriptPubKeyMan(); - if (!spk_man) { - throw std::runtime_error(strprintf("%s: spk_man is not available", __func__)); - } - - if (IsCrypted()) { - if (secureWalletPassphrase.empty()) { - throw std::runtime_error(strprintf("%s: encrypted but supplied empty wallet passphrase", __func__)); - } - - bool is_locked = IsLocked(); - - CCrypter crypter; - CKeyingMaterial vMasterKey; - - // We are intentionally re-locking the wallet so we can validate vMasterKey - // by verifying if it can unlock the wallet - Lock(); - - LOCK(cs_wallet); - for (const auto& [_, master_key] : mapMasterKeys) { - CKeyingMaterial _vMasterKey; - if (!crypter.SetKeyFromPassphrase(secureWalletPassphrase, master_key.vchSalt, master_key.nDeriveIterations, master_key.nDerivationMethod)) { - return false; - } - // Try another key if it cannot be decrypted or the key is incapable of encrypting - if (!crypter.Decrypt(master_key.vchCryptedKey, _vMasterKey) || _vMasterKey.size() != WALLET_CRYPTO_KEY_SIZE) { - continue; - } - // The likelihood of the plaintext being gibberish but also of the expected size is low but not zero. - // If it can unlock the wallet, it's a good key. - if (Unlock(_vMasterKey)) { - vMasterKey = _vMasterKey; - break; - } - } - - // We got a gibberish key... - if (vMasterKey.empty()) { - // Mimicking the error message of RPC_WALLET_PASSPHRASE_INCORRECT as it's possible - // that the user may see this error when interacting with the upgradetohd RPC - throw std::runtime_error("Error: The wallet passphrase entered was incorrect"); - } - - spk_man->GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase, vMasterKey); - - if (is_locked) { - Lock(); - } - } else { - spk_man->GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase); - } - - return true; -} - void CWallet::UpdateProgress(const std::string& title, int nProgress) { ShowProgress(title, nProgress); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 0e89400962a21..092f23b145a63 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -915,10 +915,6 @@ 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 GenerateNewHDChain(const SecureString& secureMnemonic, const SecureString& secureMnemonicPassphrase, const SecureString& secureWalletPassphrase = ""); - /* Returns true if the wallet can give out new addresses. This means it has keys in the keypool or can generate new keys */ bool CanGetAddresses(bool internal = false) const; @@ -975,9 +971,6 @@ 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/test/functional/wallet_upgradetohd.py b/test/functional/wallet_upgradetohd.py index aa770716cff1f..733eace8415e5 100755 --- a/test/functional/wallet_upgradetohd.py +++ b/test/functional/wallet_upgradetohd.py @@ -203,10 +203,7 @@ def run_test(self): node.wait_until_stopped() self.start_node(0, extra_args=['-rescan']) assert_raises_rpc_error(-13, "Error: Wallet encrypted but passphrase not supplied to RPC.", node.upgradetohd, mnemonic) - if not self.options.descriptors: - assert_raises_rpc_error(-1, "Error: The wallet passphrase entered was incorrect", node.upgradetohd, mnemonic, "", "wrongpass") - else: - assert_raises_rpc_error(-4, "Error: The wallet passphrase entered was incorrect", node.upgradetohd, mnemonic, "", "wrongpass") + assert_raises_rpc_error(-14, "Error: The wallet passphrase entered was incorrect", node.upgradetohd, mnemonic, "", "wrongpass") assert node.upgradetohd(mnemonic, "", walletpass) if not self.options.descriptors: assert_raises_rpc_error(-13, "Error: Please enter the wallet passphrase with walletpassphrase first.", node.dumphdinfo) From 746e5f87f19ca58970a0db9cf81d03664f3bf109 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Mon, 4 Aug 2025 13:20:42 +0300 Subject: [PATCH 2/2] fix: always leave the wallet in a locked state afer `upgradetohd` --- src/wallet/rpc/wallet.cpp | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index 27bbbe569cb3f..91c27a1a99401 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -420,8 +420,6 @@ static RPCHelpMan upgradetohd() pwallet->WalletLogPrintf("Upgrading wallet to HD\n"); pwallet->SetMinVersion(FEATURE_HD); - bool is_locked = pwallet->IsLocked(); - if (pwallet->IsCrypted()) { if (secureWalletPassphrase.empty()) { throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: Wallet encrypted but supplied empty wallet passphrase"); @@ -455,12 +453,11 @@ static RPCHelpMan upgradetohd() } } - if (is_locked) { - // Relock the wallet + if (pwallet->IsCrypted()) { + // Relock encrypted wallet pwallet->Lock(); - } - - if (!secureWalletPassphrase.empty() && !pwallet->IsCrypted()) { + } else if (!secureWalletPassphrase.empty()) { + // Encrypt non-encrypted wallet if (!pwallet->EncryptWallet(secureWalletPassphrase)) { throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Failed to encrypt HD wallet"); }