diff options
author | MarcoFalke <falke.marco@gmail.com> | 2020-11-25 12:46:20 +0100 |
---|---|---|
committer | MarcoFalke <falke.marco@gmail.com> | 2020-11-25 12:46:27 +0100 |
commit | afdfd3c8c1ce96adae11809e3989de381137fee9 (patch) | |
tree | b04f24e703b44db3861e99d17af8cbe58c5d66d2 /test/functional | |
parent | 1e9e4b68f3cbd1af3de5806aeff952e9bf0ab8fc (diff) | |
parent | 3eb6f8b2e61c24a22ea9396d86672307845f35eb (diff) | |
download | bitcoin-afdfd3c8c1ce96adae11809e3989de381137fee9.tar.xz |
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
Diffstat (limited to 'test/functional')
-rwxr-xr-x | test/functional/wallet_upgradewallet.py | 80 |
1 files changed, 45 insertions, 35 deletions
diff --git a/test/functional/wallet_upgradewallet.py b/test/functional/wallet_upgradewallet.py index 8ab4b3f76c..d0bb6135a8 100755 --- a/test/functional/wallet_upgradewallet.py +++ b/test/functional/wallet_upgradewallet.py @@ -22,9 +22,7 @@ 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 +90,32 @@ 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 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() @@ -158,14 +182,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,32 +191,28 @@ 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('') 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 from version 60000 to version 40000. Wallet version unchanged.") wallet.unloadwallet() assert_equal(before_checksum, sha256sum_file(node_master_wallet)) node_master.loadwallet('') @@ -208,8 +222,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 @@ -236,16 +249,13 @@ 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 from version {} to version {} without upgrading to " + "support pre-split keypool. Please use version 169900 or no version specified.".format(130000, version)) 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 +281,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,8 +342,8 @@ 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) @@ -342,5 +351,6 @@ class UpgradeWalletTest(BitcoinTestFramework): v16_3_kvs = dump_bdb_kv(v16_3_wallet) assert b'\x0adefaultkey' not in v16_3_kvs + if __name__ == '__main__': UpgradeWalletTest().main() |