From 19b2b2778557c35ec85a94f27dbbb8d94a19e7a1 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Wed, 25 Nov 2020 12:46:20 +0100 Subject: [PATCH] Merge #20403: wallet: upgradewallet fixes, improvements, test coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 32 +++++++++++--- src/wallet/wallet.cpp | 4 +- src/wallet/walletutil.cpp | 9 ++-- test/functional/wallet_upgradewallet.py | 56 +++++++++++++++---------- 4 files changed, 68 insertions(+), 33 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index ff5a71a706..a078348314 100644 --- a/src/wallet/rpcwallet.cpp +++ b/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)) { - throw JSONRPCError(RPC_WALLET_ERROR, error.original); + 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); - 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; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 248d5b5c3d..26bf6575f4 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4877,6 +4877,7 @@ std::shared_ptr 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; } diff --git a/src/wallet/walletutil.cpp b/src/wallet/walletutil.cpp index ba529e9d06..68bae737a8 100644 --- a/src/wallet/walletutil.cpp +++ b/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; - if (version >= FEATURE_HD) return FEATURE_HD; - if (version >= FEATURE_COMPRPUBKEY) return FEATURE_COMPRPUBKEY; - if (version >= FEATURE_WALLETCRYPT) return FEATURE_WALLETCRYPT; - if (version >= FEATURE_BASE) return FEATURE_BASE; + const std::array wallet_features{{FEATURE_LATEST, FEATURE_HD, FEATURE_COMPRPUBKEY, FEATURE_WALLETCRYPT, FEATURE_BASE}}; + for (const WalletFeature& wf : wallet_features) { + if (version >= wf) return wf; + } return static_cast(0); } diff --git a/test/functional/wallet_upgradewallet.py b/test/functional/wallet_upgradewallet.py index 3f33d27110..f7cf9ee8da 100755 --- a/test/functional/wallet_upgradewallet.py +++ b/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"] - - # 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) + self.test_upgradewallet(wallet, previous_version=120200, expected_version=120200) # 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), {}) - new_version = wallet.getwalletinfo()["walletversion"] - # upgraded wallet would have 120200 but no HD seed actually appeared - assert_equal(120200, new_version) + self.log.info("Test upgradewallet to HD will have version 120200 but no HD seed actually appeared") + self.test_upgradewallet(wallet, previous_version=61000, expected_version=120200, requested_version=169900) # 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 - wallet.upgradewallet(120199) - assert_equal(61000, wallet.getwalletinfo()['walletversion']) + # Test an "upgrade" from 61000 to 120199 has no effect, as the next version is 120200 + self.test_upgradewallet(wallet, previous_version=61000, requested_version=120199, expected_version=61000) 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) - assert_equal(120200, wallet.getwalletinfo()['walletversion']) + self.test_upgradewallet(wallet, previous_version=61000, requested_version=120200) # 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()