foster/bitcoin-bitcoin-core
0
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-09 10:43:19 -05:00
Code Issues Activity

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

Browse source 
3eb6f8b2e6 wallet (not for backport): improve upgradewallet error messages (Jon Atack)
ca8cd893bb wallet: fix and improve upgradewallet error responses (Jon Atack)
99d56e3571 wallet: fix and improve upgradewallet result responses (Jon Atack)
2498b04ce8 Don't upgrade to HD split if it is already supported (Andrew Chow)
c46c18b788 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 3eb6f8b2e6 🛡

Tree-SHA512: b767314069e26b5933b123acfea6aa40708507f504bdb22884da020a4ca1332af38a7072b061e36281533af9f4e236d94d3c129daf6fe5b55241127537038eed
This commit is contained in:
MarcoFalke 2020-11-25 12:46:20 +01:00
commit afdfd3c8c1
No known key found for this signature in database
GPG key ID: D2EA4850E7528B25
5 changed files with 79 additions and 54 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

32
src/wallet/rpcwallet.cpp
View file

@ -4234,7 +4234,7 @@ static RPCHelpMan sethdseed()
// Do not do anything to non-HD wallets // Do not do anything to non-HD wallets
if (!pwallet->CanSupportFeature(FEATURE_HD)) { 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); EnsureWalletIsUnlocked(pwallet);
@ -4465,14 +4465,18 @@ static RPCHelpMan walletcreatefundedpsbt()
static RPCHelpMan upgradewallet() static RPCHelpMan upgradewallet()
{ {
return 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.", "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{
RPCResult::Type::OBJ, "", "", 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)"} {RPCResult::Type::STR, "error", /* optional */ true, "Error message (if there is one)"}
}, },
}, },
@ -4495,11 +4499,27 @@ static RPCHelpMan upgradewallet()
version = request.params[0].get_int(); version = request.params[0].get_int();
} }
bilingual_str error; 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); 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); obj.pushKV("error", error.original);
} }
return obj; return obj;

2
src/wallet/scriptpubkeyman.cpp
View file

@ -456,7 +456,7 @@ bool LegacyScriptPubKeyMan::Upgrade(int prev_version, int new_version, bilingual
hd_upgrade = true; hd_upgrade = true;
} }
// Upgrade to HD chain split if necessary // 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"); WalletLogPrintf("Upgrading wallet to use HD chain split\n");
m_storage.SetMinVersion(FEATURE_PRE_SPLIT_KEYPOOL); m_storage.SetMinVersion(FEATURE_PRE_SPLIT_KEYPOOL);
split_upgrade = FEATURE_HD_SPLIT > prev_version; split_upgrade = FEATURE_HD_SPLIT > prev_version;

7
src/wallet/wallet.cpp
View file

@ -4111,9 +4111,8 @@ bool CWallet::UpgradeWallet(int version, bilingual_str& error)
} else { } else {
WalletLogPrintf("Allowing wallet upgrade up to %i\n", version); WalletLogPrintf("Allowing wallet upgrade up to %i\n", version);
} }
if (version < prev_version) if (version < prev_version) {
{ error = strprintf(_("Cannot downgrade wallet from version %i to version %i. Wallet version unchanged."), prev_version, version);
error = _("Cannot downgrade wallet");
return false; return false;
} }
@ -4121,7 +4120,7 @@ bool CWallet::UpgradeWallet(int version, bilingual_str& error)
// Do not upgrade versions to any version between HD_SPLIT and FEATURE_PRE_SPLIT_KEYPOOL unless already supporting HD_SPLIT // 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) { if (!CanSupportFeature(FEATURE_HD_SPLIT) && version >= FEATURE_HD_SPLIT && version < FEATURE_PRE_SPLIT_KEYPOOL) {
error = _("Cannot upgrade a non HD split wallet without upgrading to support pre split keypool. Please use version 169900 or no version specified."); 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; return false;
} }

12
src/wallet/walletutil.cpp
View file

@ -91,13 +91,9 @@ bool IsFeatureSupported(int wallet_version, int feature_version)
WalletFeature GetClosestWalletFeature(int version) WalletFeature GetClosestWalletFeature(int version)
{ {
if (version >= FEATURE_LATEST) return FEATURE_LATEST; const std::array<WalletFeature, 8> wallet_features{{FEATURE_LATEST, FEATURE_PRE_SPLIT_KEYPOOL, FEATURE_NO_DEFAULT_KEY, FEATURE_HD_SPLIT, FEATURE_HD, FEATURE_COMPRPUBKEY, FEATURE_WALLETCRYPT, FEATURE_BASE}};
if (version >= FEATURE_PRE_SPLIT_KEYPOOL) return FEATURE_PRE_SPLIT_KEYPOOL; for (const WalletFeature& wf : wallet_features) {
if (version >= FEATURE_NO_DEFAULT_KEY) return FEATURE_NO_DEFAULT_KEY; if (version >= wf) return wf;
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;
return static_cast<WalletFeature>(0); return static_cast<WalletFeature>(0);
} }

80
test/functional/wallet_upgradewallet.py
View file

@ -22,9 +22,7 @@ from test_framework.messages import deser_compact_size, deser_string
from test_framework.test_framework import BitcoinTestFramework from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import ( from test_framework.util import (
assert_equal, assert_equal,
assert_greater_than,
assert_is_hex_string, assert_is_hex_string,
assert_raises_rpc_error,
sha256sum_file, sha256sum_file,
) )
@ -92,6 +90,32 @@ class UpgradeWalletTest(BitcoinTestFramework):
v16_3_node.submitblock(b) v16_3_node.submitblock(b)
assert_equal(v16_3_node.getblockcount(), to_height) 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): def run_test(self):
self.nodes[0].generatetoaddress(101, self.nodes[0].getnewaddress()) self.nodes[0].generatetoaddress(101, self.nodes[0].getnewaddress())
self.dumb_sync_blocks() self.dumb_sync_blocks()
@ -158,14 +182,8 @@ class UpgradeWalletTest(BitcoinTestFramework):
self.restart_node(0) self.restart_node(0)
copy_v16() copy_v16()
wallet = node_master.get_wallet_rpc(self.default_wallet_name) wallet = node_master.get_wallet_rpc(self.default_wallet_name)
old_version = wallet.getwalletinfo()["walletversion"] self.log.info("Test upgradewallet without a version argument")
self.test_upgradewallet(wallet, previous_version=159900, expected_version=169900)
# 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)
# wallet should still contain the same balance # wallet should still contain the same balance
assert_equal(wallet.getbalance(), v16_3_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) wallet = node_master.get_wallet_rpc(self.default_wallet_name)
# should have no master key hash before conversion # should have no master key hash before conversion
assert_equal('hdseedid' in wallet.getwalletinfo(), False) assert_equal('hdseedid' in wallet.getwalletinfo(), False)
# calling upgradewallet with explicit version number self.log.info("Test upgradewallet with explicit version number")
# should return nothing if successful self.test_upgradewallet(wallet, previous_version=60000, requested_version=169900)
assert_equal(wallet.upgradewallet(169900), {})
new_version = wallet.getwalletinfo()["walletversion"]
# upgraded wallet should have version 169900
assert_equal(new_version, 169900)
# after conversion master key hash should be present # after conversion master key hash should be present
assert_is_hex_string(wallet.getwalletinfo()['hdseedid']) 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() copy_non_hd()
# Wallet starts with 60000 # Wallet starts with 60000
assert_equal(60000, wallet.getwalletinfo()['walletversion']) assert_equal(60000, wallet.getwalletinfo()['walletversion'])
wallet.unloadwallet() wallet.unloadwallet()
before_checksum = sha256sum_file(node_master_wallet) before_checksum = sha256sum_file(node_master_wallet)
node_master.loadwallet('') node_master.loadwallet('')
# Can "upgrade" to 129999 which should have no effect on the wallet # Test an "upgrade" from 60000 to 129999 has no effect, as the next version is 130000
wallet.upgradewallet(129999) self.test_upgradewallet(wallet, previous_version=60000, requested_version=129999, expected_version=60000)
assert_equal(60000, wallet.getwalletinfo()['walletversion'])
wallet.unloadwallet() wallet.unloadwallet()
assert_equal(before_checksum, sha256sum_file(node_master_wallet)) assert_equal(before_checksum, sha256sum_file(node_master_wallet))
node_master.loadwallet('') node_master.loadwallet('')
self.log.info('Wallets cannot be downgraded') self.log.info('Wallets cannot be downgraded')
copy_non_hd() 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() wallet.unloadwallet()
assert_equal(before_checksum, sha256sum_file(node_master_wallet)) assert_equal(before_checksum, sha256sum_file(node_master_wallet))
node_master.loadwallet('') node_master.loadwallet('')
@ -208,8 +222,7 @@ class UpgradeWalletTest(BitcoinTestFramework):
orig_kvs = dump_bdb_kv(node_master_wallet) orig_kvs = dump_bdb_kv(node_master_wallet)
assert b'\x07hdchain' not in orig_kvs assert b'\x07hdchain' not in orig_kvs
# Upgrade to HD, no split # Upgrade to HD, no split
wallet.upgradewallet(130000) self.test_upgradewallet(wallet, previous_version=60000, requested_version=130000)
assert_equal(130000, wallet.getwalletinfo()['walletversion'])
# Check that there is now a hd chain and it is version 1, no internal chain counter # 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) new_kvs = dump_bdb_kv(node_master_wallet)
assert b'\x07hdchain' in new_kvs assert b'\x07hdchain' in new_kvs
@ -236,16 +249,13 @@ class UpgradeWalletTest(BitcoinTestFramework):
assert_equal('m/0\'/0\'/1\'', info['hdkeypath']) assert_equal('m/0\'/0\'/1\'', info['hdkeypath'])
self.log.info('Cannot upgrade to HD Split, needs Pre Split Keypool') 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) for version in [139900, 159900, 169899]:
assert_equal(130000, wallet.getwalletinfo()['walletversion']) self.test_upgradewallet_error(wallet, previous_version=130000, requested_version=version,
assert_raises_rpc_error(-4, 'Cannot upgrade a non HD split wallet without upgrading to support pre split keypool', wallet.upgradewallet, 159900) msg="Cannot upgrade a non HD split wallet from version {} to version {} without upgrading to "
assert_equal(130000, wallet.getwalletinfo()['walletversion']) "support pre-split keypool. Please use version 169900 or no version specified.".format(130000, version))
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'])
self.log.info('Upgrade HD to HD chain split') self.log.info('Upgrade HD to HD chain split')
wallet.upgradewallet(169900) self.test_upgradewallet(wallet, previous_version=130000, requested_version=169900)
assert_equal(169900, wallet.getwalletinfo()['walletversion'])
# Check that the hdchain updated correctly # Check that the hdchain updated correctly
new_kvs = dump_bdb_kv(node_master_wallet) new_kvs = dump_bdb_kv(node_master_wallet)
hd_chain = new_kvs[b'\x07hdchain'] hd_chain = new_kvs[b'\x07hdchain']
@ -271,8 +281,7 @@ class UpgradeWalletTest(BitcoinTestFramework):
self.log.info('Upgrade non-HD to HD chain split') self.log.info('Upgrade non-HD to HD chain split')
copy_non_hd() copy_non_hd()
wallet.upgradewallet(169900) self.test_upgradewallet(wallet, previous_version=60000, requested_version=169900)
assert_equal(169900, wallet.getwalletinfo()['walletversion'])
# Check that the hdchain updated correctly # Check that the hdchain updated correctly
new_kvs = dump_bdb_kv(node_master_wallet) new_kvs = dump_bdb_kv(node_master_wallet)
hd_chain = new_kvs[b'\x07hdchain'] hd_chain = new_kvs[b'\x07hdchain']
@ -333,8 +342,8 @@ class UpgradeWalletTest(BitcoinTestFramework):
# Check the wallet has a default key initially # Check the wallet has a default key initially
old_kvs = dump_bdb_kv(node_master_wallet) old_kvs = dump_bdb_kv(node_master_wallet)
defaultkey = old_kvs[b'\x0adefaultkey'] defaultkey = old_kvs[b'\x0adefaultkey']
# Upgrade the wallet. Should still have the same default key self.log.info("Upgrade the wallet. Should still have the same default key.")
wallet.upgradewallet(159900) self.test_upgradewallet(wallet, previous_version=139900, requested_version=159900)
new_kvs = dump_bdb_kv(node_master_wallet) new_kvs = dump_bdb_kv(node_master_wallet)
up_defaultkey = new_kvs[b'\x0adefaultkey'] up_defaultkey = new_kvs[b'\x0adefaultkey']
assert_equal(defaultkey, up_defaultkey) assert_equal(defaultkey, up_defaultkey)
@ -342,5 +351,6 @@ class UpgradeWalletTest(BitcoinTestFramework):
v16_3_kvs = dump_bdb_kv(v16_3_wallet) v16_3_kvs = dump_bdb_kv(v16_3_wallet)
assert b'\x0adefaultkey' not in v16_3_kvs assert b'\x0adefaultkey' not in v16_3_kvs
if __name__ == '__main__': if __name__ == '__main__':
UpgradeWalletTest().main() UpgradeWalletTest().main()