Merge #6797: fix(rpc): return correct error codes in upgradetohd rpc

746e5f8 fix: always leave the wallet in a locked state afer `upgradetohd` (UdjinM6)
b19f463 fix(rpc): return correct error codes in `upgradetohd` rpc (UdjinM6)

Pull request description:

  ## Issue being fixed or feature implemented
  `upgradetohd` rpc returns wrong error codes

  ## What was done?
  Move wallet "upgrade to HD" code used in rpc only to `upgradetohd` rpc, return correct error codes. We also don't need all that logic to extract wallet master encryption key because it's already known when wallet is unlocked.

  ## How Has This Been Tested?
  Run tests

  ## Breaking Changes
  `upgradetohd` should return correct error codes now which could probably break things if you relied on them, see tests

  ## Checklist:
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  knst:
    utACK 746e5f8

Tree-SHA512: 829d1f9d805f7a87f628a9c7f4ae93cb545c8113cea97f430639165dae124db17d389c2f5c663214b074da8ae541435ab7df2b672f78370fe883ac77b3cfed42

Author: PastaPastaPasta

src/wallet/rpc/wallet.cpp

@@ -371,51 +371,98 @@ 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);
+
+        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 (pwallet->IsCrypted()) {
+            // Relock encrypted wallet
+            pwallet->Lock();
+        } else if (!secureWalletPassphrase.empty()) {
+            // Encrypt non-encrypted wallet
+            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();

src/wallet/wallet.cpp

@@ -353,11 +353,17 @@ std::shared_ptr<CWallet> 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;
+                        });
                 }
             }
 
@@ -3209,52 +3215,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);
@@ -3785,64 +3745,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);

src/wallet/wallet.h

@@ -919,10 +919,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;
 
@@ -979,9 +975,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<ScriptPubKeyMan*> GetActiveScriptPubKeyMans() const;
 

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)