rpc: make sure upgradetohd always has the passphrase for UpgradeToHD

kwvg · kwvg · commit 69c37f4ec257 · 2024-07-17T16:31:33.000Z
earlier it was possible to make it all the way to `EncryptSecret`
without actually having the passphrase in hand until being told off
by `CCrypter::SetKey`, we should avoid that.

also, let's get rid of checks that `UpgradeToHD` is now taking
responsibility for. no point in checking if the wallet is unlocked
as it has no bearing on your ability to upgrade the wallet.
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
@@ -2779,11 +2779,13 @@ static RPCHelpMan upgradetohd()
 {
     return RPCHelpMan{"upgradetohd",
         "\nUpgrades non-HD wallets to HD.\n"
+        "\nIf your wallet is encrypted, the wallet passphrase must be supplied. Supplying an incorrect"
+        "\npassphrase may result in your wallet getting locked.\n"
         "\nWarning: You will need to make a new backup of your wallet after setting the HD wallet mnemonic.\n",
         {
             {"mnemonic", RPCArg::Type::STR, /* default */ "", "Mnemonic as defined in BIP39 to use for the new HD wallet. Use an empty string \"\" to generate a new random mnemonic."},
             {"mnemonicpassphrase", RPCArg::Type::STR, /* default */ "", "Optional mnemonic passphrase as defined in BIP39"},
-            {"walletpassphrase", RPCArg::Type::STR, /* default */ "", "If your wallet is encrypted you must have your wallet passphrase here. If your wallet is not encrypted specifying wallet passphrase will trigger wallet encryption."},
+            {"walletpassphrase", RPCArg::Type::STR, /* default */ "", "If your wallet is encrypted you must have your wallet passphrase here. If your wallet is not encrypted, specifying wallet passphrase will trigger wallet encryption."},
             {"rescan", RPCArg::Type::BOOL, /* default */ "false if mnemonic is empty", "Whether to rescan the blockchain for missing transactions or not"},
         },
         RPCResult{
@@ -2793,6 +2795,7 @@ static RPCHelpMan upgradetohd()
             HelpExampleCli("upgradetohd", "")
     + HelpExampleCli("upgradetohd", "\"mnemonicword1 ... mnemonicwordN\"")
     + HelpExampleCli("upgradetohd", "\"mnemonicword1 ... mnemonicwordN\" \"mnemonicpassphrase\"")
+    + HelpExampleCli("upgradetohd", "\"mnemonicword1 ... mnemonicwordN\" \"\" \"walletpassphrase\"")
     + HelpExampleCli("upgradetohd", "\"mnemonicword1 ... mnemonicwordN\" \"mnemonicpassphrase\" \"walletpassphrase\"")
         },
         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
@@ -2803,17 +2806,17 @@ static RPCHelpMan upgradetohd()
     bool generate_mnemonic = request.params[0].isNull() || request.params[0].get_str().empty();
     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 (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();
     }
 
-    EnsureWalletIsUnlocked(pwallet.get());
-
     SecureString secureMnemonic;
     secureMnemonic.reserve(256);
     if (!generate_mnemonic) {
@@ -2825,6 +2828,7 @@ static RPCHelpMan upgradetohd()
     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;
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
@@ -5683,7 +5683,9 @@ bool CWallet::GenerateNewHDChain(const SecureString& secureMnemonic, const Secur
 
         // We got a gibberish key...
         if (vMasterKey.empty()) {
-            throw std::runtime_error(strprintf("%s: supplied incorrect passphrase", __func__));
+            // 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);
diff --git a/test/functional/wallet_upgradetohd.py b/test/functional/wallet_upgradetohd.py
@@ -190,8 +190,8 @@ def run_test(self):
         node.stop()
         node.wait_until_stopped()
         self.start_node(0, extra_args=['-rescan'])
-        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_raises_rpc_error(-13, "Error: Wallet encrypted but passphrase not supplied to RPC.", node.upgradetohd, mnemonic)
+        assert_raises_rpc_error(-1,  "Error: 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)
         node.walletpassphrase(walletpass, 100)

Original file line number	Diff line number	Diff line change
`@@ -5683,7 +5683,9 @@ bool CWallet::GenerateNewHDChain(const SecureString& secureMnemonic, const Secur`
`5683`	`5683`
`5684`	`5684`	`// We got a gibberish key...`
`5685`	`5685`	`if (vMasterKey.empty()) {`
`5686`		`- throw std::runtime_error(strprintf("%s: supplied incorrect passphrase", __func__));`
	`5686`	`+ // Mimicking the error message of RPC_WALLET_PASSPHRASE_INCORRECT as it's possible`
	`5687`	`+ // that the user may see this error when interacting with the upgradetohd RPC`
	`5688`	`+ throw std::runtime_error("Error: The wallet passphrase entered was incorrect");`
`5687`	`5689`	`}`
`5688`	`5690`
`5689`	`5691`	`spk_man->GenerateNewHDChain(secureMnemonic, secureMnemonicPassphrase, vMasterKey);`