From c46c18b788cb0862aafbb116fd37936cbed6a431 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Mon, 16 Nov 2020 18:55:32 +0100 Subject: [PATCH 1/4] wallet: refactor GetClosestWalletFeature() --- src/wallet/walletutil.cpp | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/wallet/walletutil.cpp b/src/wallet/walletutil.cpp index 702293e6c..88a52cdf6 100644 --- a/src/wallet/walletutil.cpp +++ b/src/wallet/walletutil.cpp @@ -87,13 +87,9 @@ bool IsFeatureSupported(int wallet_version, int feature_version) WalletFeature GetClosestWalletFeature(int version) { - if (version >= FEATURE_LATEST) return FEATURE_LATEST; - if (version >= FEATURE_PRE_SPLIT_KEYPOOL) return FEATURE_PRE_SPLIT_KEYPOOL; - if (version >= FEATURE_NO_DEFAULT_KEY) return FEATURE_NO_DEFAULT_KEY; - if (version >= FEATURE_HD_SPLIT) return FEATURE_HD_SPLIT; - 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_PRE_SPLIT_KEYPOOL, FEATURE_NO_DEFAULT_KEY, FEATURE_HD_SPLIT, FEATURE_HD, FEATURE_COMPRPUBKEY, FEATURE_WALLETCRYPT, FEATURE_BASE}}; + for (const WalletFeature& wf : wallet_features) { + if (version >= wf) return wf; + } return static_cast(0); } From 2498b04ce88696a3216fc38b7d393906b733e8b1 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Wed, 18 Nov 2020 13:14:19 -0500 Subject: [PATCH 2/4] Don't upgrade to HD split if it is already supported It is unnecessary to upgrade to FEATURE_HD_SPLIT if this feature is already supported by the wallet. Because upgrading to FEATURE_HD_SPLIT actually requires upgrading to FEATURE_PRE_SPLIT_KEYPOOL, users would accidentally be upgraded to FEATURE_PRE_SPLIT_KEYPOOL instead of nothing being done. Fixes the issue described at https://github.com/bitcoin/bitcoin/pull/20403#discussion_r526063920 --- src/wallet/scriptpubkeyman.cpp | 2 +- test/functional/wallet_upgradewallet.py | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index d2e1be640..7dbbf1730 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -453,7 +453,7 @@ bool LegacyScriptPubKeyMan::Upgrade(int prev_version, int new_version, bilingual hd_upgrade = true; } // Upgrade to HD chain split if necessary - if (IsFeatureSupported(new_version, FEATURE_HD_SPLIT)) { + if (!IsFeatureSupported(prev_version, FEATURE_HD_SPLIT) && IsFeatureSupported(new_version, FEATURE_HD_SPLIT)) { WalletLogPrintf("Upgrading wallet to use HD chain split\n"); m_storage.SetMinVersion(FEATURE_PRE_SPLIT_KEYPOOL); split_upgrade = FEATURE_HD_SPLIT > prev_version; diff --git a/test/functional/wallet_upgradewallet.py b/test/functional/wallet_upgradewallet.py index 8ab4b3f76..8d3cd16ff 100755 --- a/test/functional/wallet_upgradewallet.py +++ b/test/functional/wallet_upgradewallet.py @@ -338,6 +338,7 @@ class UpgradeWalletTest(BitcoinTestFramework): new_kvs = dump_bdb_kv(node_master_wallet) up_defaultkey = new_kvs[b'\x0adefaultkey'] assert_equal(defaultkey, up_defaultkey) + assert_equal(wallet.getwalletinfo()["walletversion"], 159900) # 0.16.3 doesn't have a default key v16_3_kvs = dump_bdb_kv(v16_3_wallet) assert b'\x0adefaultkey' not in v16_3_kvs From 99d56e357159c7154f69f28cb5587c5ca20d6594 Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Mon, 16 Nov 2020 18:23:10 +0100 Subject: [PATCH 3/4] wallet: fix and improve upgradewallet result responses --- src/wallet/rpcwallet.cpp | 28 +++++++++++-- test/functional/wallet_upgradewallet.py | 55 ++++++++++++------------- 2 files changed, 51 insertions(+), 32 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 8b08d7a5e..ef8281bfe 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -4214,7 +4214,7 @@ static RPCHelpMan sethdseed() // Do not do anything to non-HD wallets if (!pwallet->CanSupportFeature(FEATURE_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"); + 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"); } EnsureWalletIsUnlocked(pwallet); @@ -4445,14 +4445,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)"} }, }, @@ -4475,10 +4479,26 @@ static RPCHelpMan upgradewallet() version = request.params[0].get_int(); } bilingual_str error; - if (!pwallet->UpgradeWallet(version, 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) { throw JSONRPCError(RPC_WALLET_ERROR, error.original); + } else 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); + } if (!error.empty()) { obj.pushKV("error", error.original); } diff --git a/test/functional/wallet_upgradewallet.py b/test/functional/wallet_upgradewallet.py index 8d3cd16ff..bcfa1a93f 100755 --- a/test/functional/wallet_upgradewallet.py +++ b/test/functional/wallet_upgradewallet.py @@ -22,7 +22,6 @@ from test_framework.messages import deser_compact_size, deser_string from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, - assert_greater_than, assert_is_hex_string, assert_raises_rpc_error, sha256sum_file, @@ -92,6 +91,20 @@ class UpgradeWalletTest(BitcoinTestFramework): v16_3_node.submitblock(b) assert_equal(v16_3_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 run_test(self): self.nodes[0].generatetoaddress(101, self.nodes[0].getnewaddress()) self.dumb_sync_blocks() @@ -158,14 +171,8 @@ 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(new_version, old_version) + self.log.info("Test upgradewallet without a version argument") + self.test_upgradewallet(wallet, previous_version=159900, expected_version=169900) # wallet should still contain the same balance assert_equal(wallet.getbalance(), v16_3_balance) @@ -173,25 +180,20 @@ class UpgradeWalletTest(BitcoinTestFramework): wallet = node_master.get_wallet_rpc(self.default_wallet_name) # should have no master key hash before conversion assert_equal('hdseedid' in wallet.getwalletinfo(), False) - # calling upgradewallet with explicit version number - # should return nothing if successful - assert_equal(wallet.upgradewallet(169900), {}) - new_version = wallet.getwalletinfo()["walletversion"] - # upgraded wallet should have version 169900 - assert_equal(new_version, 169900) + self.log.info("Test upgradewallet with explicit version number") + self.test_upgradewallet(wallet, previous_version=60000, requested_version=169900) # after conversion master key hash should be present assert_is_hex_string(wallet.getwalletinfo()['hdseedid']) - self.log.info('Intermediary versions don\'t effect anything') + self.log.info("Intermediary versions don't effect anything") copy_non_hd() # Wallet starts with 60000 assert_equal(60000, wallet.getwalletinfo()['walletversion']) wallet.unloadwallet() before_checksum = sha256sum_file(node_master_wallet) node_master.loadwallet('') - # Can "upgrade" to 129999 which should have no effect on the wallet - wallet.upgradewallet(129999) - assert_equal(60000, wallet.getwalletinfo()['walletversion']) + # Test an "upgrade" from 60000 to 129999 has no effect, as the next version is 130000 + self.test_upgradewallet(wallet, previous_version=60000, requested_version=129999, expected_version=60000) wallet.unloadwallet() assert_equal(before_checksum, sha256sum_file(node_master_wallet)) node_master.loadwallet('') @@ -208,8 +210,7 @@ class UpgradeWalletTest(BitcoinTestFramework): orig_kvs = dump_bdb_kv(node_master_wallet) assert b'\x07hdchain' not in orig_kvs # Upgrade to HD, no split - wallet.upgradewallet(130000) - assert_equal(130000, wallet.getwalletinfo()['walletversion']) + self.test_upgradewallet(wallet, previous_version=60000, requested_version=130000) # 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) assert b'\x07hdchain' in new_kvs @@ -244,8 +245,7 @@ class UpgradeWalletTest(BitcoinTestFramework): assert_equal(130000, wallet.getwalletinfo()['walletversion']) self.log.info('Upgrade HD to HD chain split') - wallet.upgradewallet(169900) - assert_equal(169900, wallet.getwalletinfo()['walletversion']) + self.test_upgradewallet(wallet, previous_version=130000, requested_version=169900) # Check that the hdchain updated correctly new_kvs = dump_bdb_kv(node_master_wallet) hd_chain = new_kvs[b'\x07hdchain'] @@ -271,8 +271,7 @@ class UpgradeWalletTest(BitcoinTestFramework): self.log.info('Upgrade non-HD to HD chain split') copy_non_hd() - wallet.upgradewallet(169900) - assert_equal(169900, wallet.getwalletinfo()['walletversion']) + self.test_upgradewallet(wallet, previous_version=60000, requested_version=169900) # Check that the hdchain updated correctly new_kvs = dump_bdb_kv(node_master_wallet) hd_chain = new_kvs[b'\x07hdchain'] @@ -333,15 +332,15 @@ class UpgradeWalletTest(BitcoinTestFramework): # Check the wallet has a default key initially old_kvs = dump_bdb_kv(node_master_wallet) defaultkey = old_kvs[b'\x0adefaultkey'] - # Upgrade the wallet. Should still have the same default key - wallet.upgradewallet(159900) + self.log.info("Upgrade the wallet. Should still have the same default key.") + self.test_upgradewallet(wallet, previous_version=139900, requested_version=159900) new_kvs = dump_bdb_kv(node_master_wallet) up_defaultkey = new_kvs[b'\x0adefaultkey'] assert_equal(defaultkey, up_defaultkey) - assert_equal(wallet.getwalletinfo()["walletversion"], 159900) # 0.16.3 doesn't have a default key v16_3_kvs = dump_bdb_kv(v16_3_wallet) assert b'\x0adefaultkey' not in v16_3_kvs + if __name__ == '__main__': UpgradeWalletTest().main() From ca8cd893bb56bf5d455154b0498b1f58f77d20ed Mon Sep 17 00:00:00 2001 From: Jon Atack Date: Tue, 17 Nov 2020 15:57:14 +0100 Subject: [PATCH 4/4] wallet: fix and improve upgradewallet error responses --- src/wallet/rpcwallet.cpp | 16 ++++++++-------- test/functional/wallet_upgradewallet.py | 24 ++++++++++++++++-------- 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index ef8281bfe..2e3b34cbe 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -4484,12 +4484,12 @@ static RPCHelpMan upgradewallet() const int current_version{pwallet->GetVersion()}; std::string result; - if (!wallet_upgraded) { - throw JSONRPCError(RPC_WALLET_ERROR, error.original); - } else 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); + 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); @@ -4498,8 +4498,8 @@ static RPCHelpMan upgradewallet() obj.pushKV("current_version", current_version); if (!result.empty()) { obj.pushKV("result", result); - } - if (!error.empty()) { + } else { + CHECK_NONFATAL(!error.empty()); obj.pushKV("error", error.original); } return obj; diff --git a/test/functional/wallet_upgradewallet.py b/test/functional/wallet_upgradewallet.py index bcfa1a93f..9b8410e0d 100755 --- a/test/functional/wallet_upgradewallet.py +++ b/test/functional/wallet_upgradewallet.py @@ -23,7 +23,6 @@ from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, assert_is_hex_string, - assert_raises_rpc_error, sha256sum_file, ) @@ -105,6 +104,18 @@ class UpgradeWalletTest(BitcoinTestFramework): ) 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(101, self.nodes[0].getnewaddress()) self.dumb_sync_blocks() @@ -200,7 +211,7 @@ class UpgradeWalletTest(BitcoinTestFramework): 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=60000, requested_version=40000, msg="Cannot downgrade wallet") wallet.unloadwallet() assert_equal(before_checksum, sha256sum_file(node_master_wallet)) node_master.loadwallet('') @@ -237,12 +248,9 @@ class UpgradeWalletTest(BitcoinTestFramework): assert_equal('m/0\'/0\'/1\'', info['hdkeypath']) self.log.info('Cannot upgrade to HD Split, needs Pre Split Keypool') - assert_raises_rpc_error(-4, 'Cannot upgrade a non HD split wallet without upgrading to support pre split keypool', wallet.upgradewallet, 139900) - assert_equal(130000, wallet.getwalletinfo()['walletversion']) - assert_raises_rpc_error(-4, 'Cannot upgrade a non HD split wallet without upgrading to support pre split keypool', wallet.upgradewallet, 159900) - assert_equal(130000, wallet.getwalletinfo()['walletversion']) - assert_raises_rpc_error(-4, 'Cannot upgrade a non HD split wallet without upgrading to support pre split keypool', wallet.upgradewallet, 169899) - assert_equal(130000, wallet.getwalletinfo()['walletversion']) + for version in [139900, 159900, 169899]: + self.test_upgradewallet_error(wallet, previous_version=130000, requested_version=version, + msg="Cannot upgrade a non HD split wallet without upgrading to support pre split keypool. Please use version 169900 or no version specified.") self.log.info('Upgrade HD to HD chain split') self.test_upgradewallet(wallet, previous_version=130000, requested_version=169900)