Merge #20403: wallet: upgradewallet fixes, improvements, test coverage

3eb6f8b2e61c24a22ea9396d86672307845f35eb wallet (not for backport): improve upgradewallet error messages (Jon Atack)
ca8cd893bb56bf5d455154b0498b1f58f77d20ed wallet: fix and improve upgradewallet error responses (Jon Atack)
99d56e357159c7154f69f28cb5587c5ca20d6594 wallet: fix and improve upgradewallet result responses (Jon Atack)
2498b04ce88696a3216fc38b7d393906b733e8b1 Don't upgrade to HD split if it is already supported (Andrew Chow)
c46c18b788cb0862aafbb116fd37936cbed6a431 wallet: refactor GetClosestWalletFeature() (Jon Atack)

Pull request description:

  This follows up on #18836 and #20282 to fix and improve the as-yet unreleased `upgradewallet` feature and also implement review follow-up in https://github.com/bitcoin/bitcoin/pull/18836#discussion_r519328607.

  This PR fixes 4 upgradewallet issues:

  - this bug: https://github.com/bitcoin/bitcoin/pull/20403#discussion_r526063920
  - it returns nothing in the absence of an RPC error, which isn't reassuring for users
  - it returns the same thing both in the case of a successful upgrade and when no upgrade took place
  - the error message object is currently dead code

  This PR fixes the above and provides:

  ...user feedback to not silently return without upgrading
  ```
  {
    "wallet_name": "disable private keys",
    "previous_version": 169900,
    "current_version": 169900,
    "result": "Already at latest version. Wallet version unchanged."
  }
  ```
  ...better feedback after successfully upgrading
  ```
  {
    "wallet_name": "watch-only",
    "previous_version": 159900,
    "current_version": 169900,
    "result": "Wallet upgraded successfully from version 159900 to version 169900."
  }
  ```
  ...helpful error responses
  ```
  {
    "wallet_name": "blank",
    "previous_version": 169900,
    "current_version": 169900,
    "error": "Cannot downgrade wallet from version 169900 to version 159900. Wallet version unchanged."
  }
  {
    "wallet_name": "blank",
    "previous_version": 130000,
    "current_version": 130000,
    "error": "Cannot upgrade a non HD split wallet from version 130000 to version 169899 without upgrading to support pre-split keypool. Please use version 169900 or no version specified."
  }
  ```
  updated help:
  ```
  upgradewallet ( version )

  Upgrade the wallet. Upgrades to the latest version if no version number is specified.
  New keys may be generated and a new wallet backup will need to be made.
  Arguments:
  1. version    (numeric, optional, default=169900) The version number to upgrade to. Default is the latest wallet version.

  Result:
  {                            (json object)
    "wallet_name" : "str",     (string) Name of wallet this operation was performed on
    "previous_version" : n,    (numeric) Version of wallet before this operation
    "current_version" : n,     (numeric) Version of wallet after this operation
    "result" : "str",          (string, optional) Description of result, if no error
    "error" : "str"            (string, optional) Error message (if there is one)
  }
  ```

ACKs for top commit:
  achow101:
    ACK  3eb6f8b
  MarcoFalke:
    review ACK 3eb6f8b2e61c24a22ea9396d86672307845f35eb 🛡

Tree-SHA512: b767314069e26b5933b123acfea6aa40708507f504bdb22884da020a4ca1332af38a7072b061e36281533af9f4e236d94d3c129daf6fe5b55241127537038eed

src/wallet/rpcwallet.cpp

@@ -4308,7 +4308,7 @@ static RPCHelpMan sethdseed()
 
     // Do not do anything to non-HD wallets
     if (!pwallet->CanSupportFeature(FEATURE_HD)) {
-        throw JSONRPCError(RPC_WALLET_ERROR, "Cannot set an HD seed on a non-HD wallet. Use the upgradewallet RPC in order to upgrade a non-HD wallet to HD");
+        throw JSONRPCError(RPC_WALLET_ERROR, "Cannot set a HD seed on a non-HD wallet. Use the upgradewallet RPC in order to upgrade a non-HD wallet to HD");
     }
     if (pwallet->IsHDEnabled()) {
         throw JSONRPCError(RPC_WALLET_ERROR, "Cannot set a HD seed. The wallet already has a seed");
@@ -4540,14 +4540,18 @@ static RPCHelpMan walletcreatefundedpsbt()
 static RPCHelpMan upgradewallet()
 {
     return RPCHelpMan{"upgradewallet",
-        "\nUpgrade the wallet. Upgrades to the latest version if no version number is specified\n"
+        "\nUpgrade 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, /* default */ strprintf("%d", FEATURE_LATEST), "The version number to upgrade to. Default is the latest wallet version"}
+            {"version", RPCArg::Type::NUM, /* default */ strprintf("%d", 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)"}
             },
         },
@@ -4570,11 +4574,27 @@ static RPCHelpMan upgradewallet()
         version = request.params[0].get_int();
     }
     bilingual_str error;
-    if (!pwallet->UpgradeWallet(version, error)) {
+    const int previous_version{pwallet->GetVersion()};
-        throw JSONRPCError(RPC_WALLET_ERROR, error.original);
+    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);
-    if (!error.empty()) {
+    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;

src/wallet/wallet.cpp

@@ -4877,6 +4877,7 @@ std::shared_ptr<CWallet> CWallet::Create(interfaces::Chain& chain, interfaces::C
 
 bool CWallet::UpgradeWallet(int version, bilingual_str& error)
 {
+    int prev_version = GetVersion();
     int nMaxVersion = version;
     auto nMinVersion = DEFAULT_USE_HD_WALLET ? FEATURE_LATEST : FEATURE_COMPRPUBKEY;
     if (nMaxVersion == 0) {
@@ -4888,13 +4889,14 @@ bool CWallet::UpgradeWallet(int version, bilingual_str& error)
     }
 
     if (nMaxVersion < GetVersion()) {
-        error = Untranslated("Cannot downgrade wallet");
+        error = strprintf(_("Cannot downgrade wallet from version %i to version %i. Wallet version unchanged."), prev_version, version);
         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");
+        error = strprintf(_("Cannot upgrade a non HD wallet from version %i to version %i which is non-HD wallet. Use upgradetohd RPC"), prev_version, version);
         return false;
     }
 

src/wallet/walletutil.cpp

@@ -36,10 +36,9 @@ bool IsFeatureSupported(int wallet_version, int feature_version)
 
 WalletFeature GetClosestWalletFeature(int version)
 {
-    if (version >= FEATURE_LATEST) return FEATURE_LATEST;
+    const std::array<WalletFeature, 5> wallet_features{{FEATURE_LATEST, FEATURE_HD, FEATURE_COMPRPUBKEY, FEATURE_WALLETCRYPT, FEATURE_BASE}};
-    if (version >= FEATURE_HD) return FEATURE_HD;
+    for (const WalletFeature& wf : wallet_features) {
-    if (version >= FEATURE_COMPRPUBKEY) return FEATURE_COMPRPUBKEY;
+        if (version >= wf) return wf;
-    if (version >= FEATURE_WALLETCRYPT) return FEATURE_WALLETCRYPT;
+    }
-    if (version >= FEATURE_BASE) return FEATURE_BASE;
     return static_cast<WalletFeature>(0);
 }

test/functional/wallet_upgradewallet.py

@@ -18,9 +18,7 @@ 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,
-    assert_raises_rpc_error,
     sha256sum_file,
 )
 
@@ -71,6 +69,32 @@ class UpgradeWalletTest(BitcoinTestFramework):
             v18_2_node.submitblock(b)
         assert_equal(v18_2_node.getblockcount(), to_height)
 
+    def test_upgradewallet(self, wallet, previous_version, requested_version=None, expected_version=None):
+        unchanged = expected_version == previous_version
+        new_version = previous_version if unchanged else expected_version if expected_version else requested_version
+        assert_equal(wallet.getwalletinfo()["walletversion"], previous_version)
+        assert_equal(wallet.upgradewallet(requested_version),
+            {
+                "wallet_name": "",
+                "previous_version": previous_version,
+                "current_version": new_version,
+                "result": "Already at latest version. Wallet version unchanged." if unchanged else "Wallet upgraded successfully from version {} to version {}.".format(previous_version, new_version),
+            }
+        )
+        assert_equal(wallet.getwalletinfo()["walletversion"], new_version)
+
+    def test_upgradewallet_error(self, wallet, previous_version, requested_version, msg):
+        assert_equal(wallet.getwalletinfo()["walletversion"], previous_version)
+        assert_equal(wallet.upgradewallet(requested_version),
+            {
+                "wallet_name": "",
+                "previous_version": previous_version,
+                "current_version": previous_version,
+                "error": msg,
+            }
+        )
+        assert_equal(wallet.getwalletinfo()["walletversion"], previous_version)
+
     def run_test(self):
         self.nodes[0].generatetoaddress(COINBASE_MATURITY + 1, self.nodes[0].getnewaddress())
         self.dumb_sync_blocks()
@@ -131,14 +155,7 @@ class UpgradeWalletTest(BitcoinTestFramework):
         self.restart_node(0)
         copy_v16()
         wallet = node_master.get_wallet_rpc(self.default_wallet_name)
-        old_version = wallet.getwalletinfo()["walletversion"]
+        self.test_upgradewallet(wallet, previous_version=120200, expected_version=120200)
-
-        # calling upgradewallet without version arguments
-        # should return nothing if successful
-        assert_equal(wallet.upgradewallet(), {})
-        new_version = wallet.getwalletinfo()["walletversion"]
-        # upgraded wallet version should be greater than older one
-        assert_greater_than_or_equal(new_version, old_version)
         # wallet should still contain the same balance
         assert_equal(wallet.getbalance(), v18_2_balance)
 
@@ -149,10 +166,8 @@ class UpgradeWalletTest(BitcoinTestFramework):
         # calling upgradewallet with explicit version number
         # should return nothing if successful
 
-        assert_equal(wallet.upgradewallet(169900), {})
+        self.log.info("Test upgradewallet to HD will have version 120200 but no HD seed actually appeared")
-        new_version = wallet.getwalletinfo()["walletversion"]
+        self.test_upgradewallet(wallet, previous_version=61000, expected_version=120200, requested_version=169900)
-        # upgraded wallet would have 120200 but no HD seed actually appeared
-        assert_equal(120200, new_version)
         # after conversion master key hash should not be present yet
         assert 'hdchainid' not in wallet.getwalletinfo()
         assert_equal(wallet.upgradetohd(), True)
@@ -160,23 +175,23 @@ class UpgradeWalletTest(BitcoinTestFramework):
         assert_equal(new_version, 120200)
         assert_is_hex_string(wallet.getwalletinfo()['hdchainid'])
 
-        self.log.info('Intermediary versions don\'t effect anything')
+        self.log.info("Intermediary versions don't effect anything")
         copy_non_hd()
         # Wallet starts with 61000 (legacy "latest")
         assert_equal(61000, wallet.getwalletinfo()['walletversion'])
         wallet.unloadwallet()
         before_checksum = sha256sum_file(node_master_wallet)
         node_master.loadwallet('')
-        # Can "upgrade" to 120199 which should have no effect on the wallet
+        # Test an "upgrade" from 61000 to 120199 has no effect, as the next version is 120200
-        wallet.upgradewallet(120199)
+        self.test_upgradewallet(wallet, previous_version=61000, requested_version=120199, expected_version=61000)
-        assert_equal(61000, wallet.getwalletinfo()['walletversion'])
         wallet.unloadwallet()
         assert_equal(before_checksum, sha256sum_file(node_master_wallet))
         node_master.loadwallet('')
 
         self.log.info('Wallets cannot be downgraded')
         copy_non_hd()
-        assert_raises_rpc_error(-4, 'Cannot downgrade wallet', wallet.upgradewallet, 40000)
+        self.test_upgradewallet_error(wallet, previous_version=61000, requested_version=40000,
+            msg="Cannot downgrade wallet from version 61000 to version 40000. Wallet version unchanged.")
         wallet.unloadwallet()
         assert_equal(before_checksum, sha256sum_file(node_master_wallet))
         node_master.loadwallet('')
@@ -186,8 +201,7 @@ class UpgradeWalletTest(BitcoinTestFramework):
         orig_kvs = dump_bdb_kv(node_master_wallet)
         assert b'\x07hdchain' not in orig_kvs
         # Upgrade to HD
-        wallet.upgradewallet(120200)
+        self.test_upgradewallet(wallet, previous_version=61000, requested_version=120200)
-        assert_equal(120200, wallet.getwalletinfo()['walletversion'])
         # Check that there is now a hd chain and it is version 1, no internal chain counter
         new_kvs = dump_bdb_kv(node_master_wallet)
         wallet.upgradetohd()