Merge bitcoin/bitcoin#32944: wallet: Remove upgradewallet RPC

d89c6fa wallet: Remove `upgradewallet` RPC (w0xlt)

Pull request description:

  Based on discussions  in bitcoin/bitcoin#32803, this PR proposes removing the ` upgradewallet`  RPC.

ACKs for top commit:
  maflcko:
    review ACK d89c6fa 🤙
  achow101:
    ACK d89c6fa
  pablomartin4btc:
    ACK d89c6fa
  brunoerg:
    Concept & light cr ACK d89c6fa

Tree-SHA512: 9ab89c9137ff83d7826da6b9d00d3617149a5d144129086a2685ee525087534c5ed06259075c0689ded52d33e075acb5067d185be04ecc638e27469f958f9a56

Author: achow101

doc/release-notes-32944.md

@@ -0,0 +1,3 @@
+# RPC
+
+`upgradewallet` has been removed. It was unused and only applied to unsupported legacy wallets.

src/rpc/client.cpp

@@ -275,7 +275,6 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "logging", 0, "include" },
     { "logging", 1, "exclude" },
     { "disconnectnode", 1, "nodeid" },
-    { "upgradewallet", 0, "version" },
     { "gethdkeys", 0, "active_only" },
     { "gethdkeys", 0, "options" },
     { "gethdkeys", 0, "private" },

src/wallet/rpc/wallet.cpp

@@ -490,69 +490,6 @@ static RPCHelpMan unloadwallet()
     };
 }
 
-static RPCHelpMan upgradewallet()
-{
-    return RPCHelpMan{
-        "upgradewallet",
-        "Upgrade the wallet. Upgrades to the latest version if no version number is specified.\n"
-        "New keys may be generated and a new wallet backup will need to be made.",
-        {
-            {"version", RPCArg::Type::NUM, RPCArg::Default{int{FEATURE_LATEST}}, "The version number to upgrade to. Default is the latest wallet version."}
-        },
-        RPCResult{
-            RPCResult::Type::OBJ, "", "",
-            {
-                {RPCResult::Type::STR, "wallet_name", "Name of wallet this operation was performed on"},
-                {RPCResult::Type::NUM, "previous_version", "Version of wallet before this operation"},
-                {RPCResult::Type::NUM, "current_version", "Version of wallet after this operation"},
-                {RPCResult::Type::STR, "result", /*optional=*/true, "Description of result, if no error"},
-                {RPCResult::Type::STR, "error", /*optional=*/true, "Error message (if there is one)"}
-            },
-        },
-        RPCExamples{
-            HelpExampleCli("upgradewallet", "169900")
-            + HelpExampleRpc("upgradewallet", "169900")
-        },
-        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
-{
-    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
-    if (!pwallet) return UniValue::VNULL;
-
-    EnsureWalletIsUnlocked(*pwallet);
-
-    int version = 0;
-    if (!request.params[0].isNull()) {
-        version = request.params[0].getInt<int>();
-    }
-    bilingual_str error;
-    const int previous_version{pwallet->GetVersion()};
-    const bool wallet_upgraded{pwallet->UpgradeWallet(version, error)};
-    const int current_version{pwallet->GetVersion()};
-    std::string result;
-
-    if (wallet_upgraded) {
-        if (previous_version == current_version) {
-            result = "Already at latest version. Wallet version unchanged.";
-        } else {
-            result = strprintf("Wallet upgraded successfully from version %i to version %i.", previous_version, current_version);
-        }
-    }
-
-    UniValue obj(UniValue::VOBJ);
-    obj.pushKV("wallet_name", pwallet->GetName());
-    obj.pushKV("previous_version", previous_version);
-    obj.pushKV("current_version", current_version);
-    if (!result.empty()) {
-        obj.pushKV("result", result);
-    } else {
-        CHECK_NONFATAL(!error.empty());
-        obj.pushKV("error", error.original);
-    }
-    return obj;
-},
-    };
-}
-
 RPCHelpMan simulaterawtransaction()
 {
     return RPCHelpMan{
@@ -1025,7 +962,6 @@ std::span<const CRPCCommand> GetWalletRPCCommands()
         {"wallet", &simulaterawtransaction},
         {"wallet", &sendall},
         {"wallet", &unloadwallet},
-        {"wallet", &upgradewallet},
         {"wallet", &walletcreatefundedpsbt},
 #ifdef ENABLE_EXTERNAL_SIGNER
         {"wallet", &walletdisplayaddress},

src/wallet/scriptpubkeyman.h

@@ -118,9 +118,6 @@ class ScriptPubKeyMan
     /* Returns true if the wallet can give out new addresses. This means it has keys in the keypool or can generate new keys */
     virtual bool CanGetAddresses(bool internal = false) const { return false; }
 
-    /** Upgrades the wallet to the specified version */
-    virtual bool Upgrade(int prev_version, int new_version, bilingual_str& error) { return true; }
-
     virtual bool HavePrivateKeys() const { return false; }
     virtual bool HaveCryptedKeys() const { return false; }
 

src/wallet/wallet.cpp

@@ -3194,39 +3194,6 @@ const CAddressBookData* CWallet::FindAddressBookEntry(const CTxDestination& dest
     return &address_book_it->second;
 }
 
-bool CWallet::UpgradeWallet(int version, bilingual_str& error)
-{
-    int prev_version = GetVersion();
-    if (version == 0) {
-        WalletLogPrintf("Performing wallet upgrade to %i\n", FEATURE_LATEST);
-        version = FEATURE_LATEST;
-    } else {
-        WalletLogPrintf("Allowing wallet upgrade up to %i\n", version);
-    }
-    if (version < prev_version) {
-        error = strprintf(_("Cannot downgrade wallet from version %i to version %i. Wallet version unchanged."), prev_version, version);
-        return false;
-    }
-
-    LOCK(cs_wallet);
-
-    // Do not upgrade versions to any version between HD_SPLIT and FEATURE_PRE_SPLIT_KEYPOOL unless already supporting HD_SPLIT
-    if (!CanSupportFeature(FEATURE_HD_SPLIT) && version >= FEATURE_HD_SPLIT && version < FEATURE_PRE_SPLIT_KEYPOOL) {
-        error = strprintf(_("Cannot upgrade a non HD split wallet from version %i to version %i without upgrading to support pre-split keypool. Please use version %i or no version specified."), prev_version, version, FEATURE_PRE_SPLIT_KEYPOOL);
-        return false;
-    }
-
-    // Permanently upgrade to the version
-    SetMinVersion(GetClosestWalletFeature(version));
-
-    for (auto spk_man : GetActiveScriptPubKeyMans()) {
-        if (!spk_man->Upgrade(prev_version, version, error)) {
-            return false;
-        }
-    }
-    return true;
-}
-
 void CWallet::postInitProcess()
 {
     // Add wallet transactions that aren't already in a block to mempool

src/wallet/wallet.h

@@ -945,9 +945,6 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
         LogInfo("%s %s", GetDisplayName(), tfm::format(wallet_fmt, params...));
     };
 
-    /** Upgrade the wallet */
-    bool UpgradeWallet(int version, bilingual_str& error);
-
     //! Returns all unique ScriptPubKeyMans in m_internal_spk_managers and m_external_spk_managers
     std::set<ScriptPubKeyMan*> GetActiveScriptPubKeyMans() const;
     bool IsActiveScriptPubKeyMan(const ScriptPubKeyMan& spkm) const;

test/functional/wallet_multiwallet.py

@@ -183,9 +183,6 @@ def wallet_file(name):
         open(not_a_dir, 'a', encoding="utf8").close()
         self.nodes[0].assert_start_raises_init_error(['-walletdir=' + not_a_dir], 'Error: Specified -walletdir "' + not_a_dir + '" is not a directory')
 
-        self.log.info("Do not allow -upgradewallet with multiwallet")
-        self.nodes[0].assert_start_raises_init_error(['-upgradewallet'], "Error: Error parsing command line arguments: Invalid parameter -upgradewallet")
-
         # if wallets/ doesn't exist, datadir should be the default wallet dir
         wallet_dir2 = data_dir('walletdir')
         os.rename(wallet_dir(), wallet_dir2)