wallet: upgradewallet fixes, improvements, test coverage #20403

jonatack · 2020-11-16T18:21:02Z

This follows up on #18836 and #20282 to fix and improve the as-yet unreleased upgradewallet feature and also implement review follow-up in #18836 (comment).

This PR fixes 4 upgradewallet issues:

this bug: wallet: upgradewallet fixes, improvements, test coverage #20403 (comment)
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)
}

src/wallet/wallet.cpp

achow101 · 2020-11-16T18:34:27Z

Code review ACK b95925c

DrahtBot · 2020-11-16T18:36:45Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

jonatack · 2020-11-16T18:46:27Z

Removed the cast in UpgradeWallet() per git diff b95925c 377fa31 (thanks!)

@@ -4128,7 +4128,7 @@ bool CWallet::UpgradeWallet(int& version, bilingual_str& error, std::vector<bili
     // Permanently upgrade to the version
     const WalletFeature new_version{GetClosestWalletFeature(version)};
     SetMinVersion(new_version);
-    version = static_cast<int>(new_version); // return actual updated version
+    version = new_version; // return actual updated version

achow101 · 2020-11-16T18:50:23Z

ACK 377fa31

src/wallet/rpcwallet.cpp

jonatack · 2020-11-17T16:50:19Z

Rebased after the merge of #20139 and addressed #20403 (comment) with 877aba8.

src/wallet/rpcwallet.cpp

src/wallet/wallet.cpp

test/functional/wallet_upgradewallet.py

jonatack · 2020-11-18T12:49:06Z

Found an edge case bug in the first commit, "wallet: enable CWallet::UpgradeWallet to return updated version", and the fix led to a simpler implementation.

Fixed two tests in the third commit, "wallet: fix and improve upgradewallet result responses"; thanks @MarcoFalke for catching one of them. The other one led to seeing the edge case bug.

Re-verified build and the wallet_upgradewallet.py test at each commit.

Diff: git diff dfec0a8 25909e6

jon@purity:~/projects/bitcoin/bitcoin (upgradewallet-improvements)$ git diff dfec0a8 25909e6
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 17631c58e7..10a7a07ee7 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4127,15 +4127,15 @@ bool CWallet::UpgradeWallet(int& version, bilingual_str& error)
     }
 
     // Permanently upgrade to the version
-    const WalletFeature new_version{GetClosestWalletFeature(version)};
-    SetMinVersion(new_version);
-    version = new_version; // return actual updated version
+    SetMinVersion(GetClosestWalletFeature(version));
 
     for (auto spk_man : GetActiveScriptPubKeyMans()) {
         if (!spk_man->Upgrade(prev_version, version, error)) {
             return false;
         }
     }
+
+    version = GetVersion(); // return actual updated version
     return true;
 }
 
diff --git a/test/functional/wallet_upgradewallet.py b/test/functional/wallet_upgradewallet.py
index cec3dd2b6d..7cae913fa1 100755
--- a/test/functional/wallet_upgradewallet.py
+++ b/test/functional/wallet_upgradewallet.py
@@ -90,10 +90,10 @@ 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, unchanged=False):
-        new_version = previous_version if unchanged else requested_version
+    def test_upgradewallet(self, wallet, previous_version, requested_version, expected_version=None, no_arg=False, unchanged=False):
+        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),
+        assert_equal(wallet.upgradewallet() if no_arg else wallet.upgradewallet(requested_version),
             {
                 "wallet_name": "",
                 "previous_version": previous_version,
@@ -182,7 +182,7 @@ class UpgradeWalletTest(BitcoinTestFramework):
         copy_v16()
         wallet = node_master.get_wallet_rpc(self.default_wallet_name)
         self.log.info("Test upgradewallet without version arguments")
-        self.test_upgradewallet(wallet, previous_version=159900, requested_version=169900)
+        self.test_upgradewallet(wallet, previous_version=159900, requested_version=169900, no_arg=True)
         # wallet should still contain the same balance
         assert_equal(wallet.getbalance(), v16_3_balance)
 
@@ -342,7 +342,7 @@ class UpgradeWalletTest(BitcoinTestFramework):
         old_kvs = dump_bdb_kv(node_master_wallet)
         defaultkey = old_kvs[b'\x0adefaultkey']
         self.log.info("Upgrade the wallet. Should still have the same default key.")
-        self.test_upgradewallet(wallet, previous_version=139900, requested_version=169900)
+        self.test_upgradewallet(wallet, previous_version=139900, requested_version=159900, expected_version=169900)
         new_kvs = dump_bdb_kv(node_master_wallet)
         up_defaultkey = new_kvs[b'\x0adefaultkey']
         assert_equal(defaultkey, up_defaultkey)

jonatack · 2020-11-18T12:54:41Z

test/functional/wallet_upgradewallet.py

-        # 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, expected_version=169900)


@achow101 sanity check, if current version is 139900 FEATURE_HD, and upgradewallet 159900 FEATURE_NO_DEFAULT_KEY is called, is the wallet actually upgrading to 169900 FEATURE_PRE_SPLIT_KEYPOOL/FEATURE_LATEST the expected behavior ?

Versions less than FEATURE_HD_SPLIT cannot upgrade to FEATURE_HD_SPLIT or FEATURE_NO_DEFAULT_KEY. They must upgrade to FEATURE_PRE_SPLIT_KEYPOOL at which time both the previous features will also be applied.

If a user specifies FEATURE_NO_DEFAULT_KEY on FEATURE_HD wallet, we should probably give an error and tell the user they can't do that.

Edit: This test is FEATURE_HD_SPLIT, not FEATURE_HD. I'm not sure why it jumps to FEATURE_LATEST.

Found it. Here's a fix.

diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index d2e1be6402..7dbbf17302 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 7cae913fa1..fdd52ba574 100755 --- a/test/functional/wallet_upgradewallet.py +++ b/test/functional/wallet_upgradewallet.py @@ -342,7 +342,7 @@ class UpgradeWalletTest(BitcoinTestFramework): old_kvs = dump_bdb_kv(node_master_wallet) defaultkey = old_kvs[b'\x0adefaultkey'] self.log.info("Upgrade the wallet. Should still have the same default key.") - self.test_upgradewallet(wallet, previous_version=139900, requested_version=159900, expected_version=169900) + self.test_upgradewallet(wallet, previous_version=139900, requested_version=159900, expected_version=159900) new_kvs = dump_bdb_kv(node_master_wallet) up_defaultkey = new_kvs[b'\x0adefaultkey'] assert_equal(defaultkey, up_defaultkey)

This particular change probably needs backport to 0.21, so I'll make a separate PR for it.

src/wallet/wallet.cpp

jonatack · 2020-11-18T15:27:10Z

Dropped the first commit to call GetVersion() in RPCHelpMan upgradewallet().

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 bitcoin#20403 (comment)

jonatack · 2020-11-19T08:08:46Z

Pulled in #20420.

test/functional/wallet_upgradewallet.py

achow101 · 2020-11-19T19:05:32Z

ACK 3eb6f8b

luke-jr · 2020-11-24T23:17:19Z

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");


"a" is correct...

https://www.referencepointsoftware.com/a-vs-an-before-an-abbreviation/

maflcko

review ACK 3eb6f8b 🛡

Show signature and timestamp

Signature:

Timestamp of file with hash 1140e347b1496cc50a43fa6fca8080f5ac91dbec5a7d8191668b108f5c1bf70d -

test/functional/wallet_upgradewallet.py

src/wallet/rpcwallet.cpp

maflcko · 2020-11-25T11:53:05Z

Backported in #20490

…coverage

…est coverage ca8cd89 wallet: fix and improve upgradewallet error responses (Jon Atack) 99d56e3 wallet: fix and improve upgradewallet result responses (Jon Atack) 2498b04 Don't upgrade to HD split if it is already supported (Andrew Chow) c46c18b wallet: refactor GetClosestWalletFeature() (Jon Atack) Pull request description: Github-Pull: #20403 Rebased-From: c46c18b Github-Pull: #20403 Rebased-From: 2498b04 Github-Pull: #20403 Rebased-From: 99d56e3 Github-Pull: #20403 Rebased-From: ca8cd89 Top commit has no ACKs. Tree-SHA512: b18a1d015c963298740c585385eaa056988464112c88a519fe619be22dc78a8f6a102365cf799c50b781a77a09bec82b58ce411ab007b48f8b5de876e9c75060

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 bitcoin/bitcoin#20403 (comment)

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 bitcoin/bitcoin#20403 (comment) (cherry picked from commit 2498b04)

This was referenced Nov 16, 2020

wallet: upgradewallet fixes and additional tests #18836

Merged

wallet: change upgradewallet return type to be an object #20282

Merged

achow101 reviewed Nov 16, 2020

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

jonatack force-pushed the upgradewallet-improvements branch from b95925c to 377fa31 Compare November 16, 2020 18:44

jonatack mentioned this pull request Nov 16, 2020

Wallet: do not return warnings from UpgradeWallet() #20139

Merged

DrahtBot added RPC/REST/ZMQ Wallet labels Nov 16, 2020

maflcko reviewed Nov 17, 2020

View reviewed changes

src/wallet/rpcwallet.cpp Show resolved Hide resolved

DrahtBot added the Needs rebase label Nov 17, 2020

maflcko added the Needs backport (0.21) label Nov 17, 2020

jonatack force-pushed the upgradewallet-improvements branch from 377fa31 to 877aba8 Compare November 17, 2020 16:39

maflcko reviewed Nov 17, 2020

View reviewed changes

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

DrahtBot removed the Needs rebase label Nov 17, 2020

jonatack force-pushed the upgradewallet-improvements branch from 877aba8 to dfec0a8 Compare November 17, 2020 18:42

maflcko reviewed Nov 18, 2020

View reviewed changes

test/functional/wallet_upgradewallet.py Outdated Show resolved Hide resolved

maflcko added this to the 0.21.0 milestone Nov 18, 2020

jonatack force-pushed the upgradewallet-improvements branch from dfec0a8 to 25909e6 Compare November 18, 2020 12:46

jonatack commented Nov 18, 2020

View reviewed changes

maflcko reviewed Nov 18, 2020

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

wallet: refactor GetClosestWalletFeature()

c46c18b

jonatack force-pushed the upgradewallet-improvements branch from 25909e6 to e8e053f Compare November 18, 2020 15:24

achow101 mentioned this pull request Nov 18, 2020

wallet: Don't upgrade to HD split if it is already supported #20420

Closed

jonatack force-pushed the upgradewallet-improvements branch from e8e053f to 8507c28 Compare November 19, 2020 08:03

achow101 reviewed Nov 19, 2020

View reviewed changes

test/functional/wallet_upgradewallet.py Outdated Show resolved Hide resolved

test/functional/wallet_upgradewallet.py Show resolved Hide resolved

jonatack added 3 commits November 19, 2020 20:00

wallet: fix and improve upgradewallet result responses

99d56e3

wallet: fix and improve upgradewallet error responses

ca8cd89

wallet (not for backport): improve upgradewallet error messages

3eb6f8b

jonatack force-pushed the upgradewallet-improvements branch from 8507c28 to 3eb6f8b Compare November 19, 2020 19:03

luke-jr reviewed Nov 24, 2020

View reviewed changes

maflcko approved these changes Nov 25, 2020

View reviewed changes

test/functional/wallet_upgradewallet.py Show resolved Hide resolved

src/wallet/rpcwallet.cpp Show resolved Hide resolved

maflcko merged commit afdfd3c into bitcoin:master Nov 25, 2020

maflcko mentioned this pull request Nov 25, 2020

[backport] wallet: upgradewallet fixes, improvements, test coverage #20490

Merged

maflcko removed the Needs backport (0.21) label Nov 25, 2020

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 25, 2020

Merge bitcoin#20403: wallet: upgradewallet fixes, improvements, test …

b91d3bb

…coverage

jonatack deleted the upgradewallet-improvements branch November 25, 2020 16:25

jonatack mentioned this pull request Dec 2, 2020

[tests] Allow outbound & block-relay-only connections in functional tests. #19315

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wallet: upgradewallet fixes, improvements, test coverage #20403

wallet: upgradewallet fixes, improvements, test coverage #20403

jonatack commented Nov 16, 2020 •

edited

Loading

achow101 commented Nov 16, 2020

DrahtBot commented Nov 16, 2020 •

edited

Loading

jonatack commented Nov 16, 2020

achow101 commented Nov 16, 2020

jonatack commented Nov 17, 2020

jonatack commented Nov 18, 2020 •

edited

Loading

jonatack Nov 18, 2020 •

edited

Loading

achow101 Nov 18, 2020 •

edited

Loading

achow101 Nov 18, 2020 •

edited

Loading

jonatack commented Nov 18, 2020

jonatack commented Nov 19, 2020

achow101 commented Nov 19, 2020

luke-jr Nov 24, 2020

jonatack Nov 24, 2020

maflcko left a comment

maflcko commented Nov 25, 2020

wallet: upgradewallet fixes, improvements, test coverage #20403

wallet: upgradewallet fixes, improvements, test coverage #20403

Conversation

jonatack commented Nov 16, 2020 • edited Loading

achow101 commented Nov 16, 2020

DrahtBot commented Nov 16, 2020 • edited Loading

Conflicts

jonatack commented Nov 16, 2020

achow101 commented Nov 16, 2020

jonatack commented Nov 17, 2020

jonatack commented Nov 18, 2020 • edited Loading

jonatack Nov 18, 2020 • edited Loading

Choose a reason for hiding this comment

achow101 Nov 18, 2020 • edited Loading

Choose a reason for hiding this comment

achow101 Nov 18, 2020 • edited Loading

Choose a reason for hiding this comment

jonatack commented Nov 18, 2020

jonatack commented Nov 19, 2020

achow101 commented Nov 19, 2020

luke-jr Nov 24, 2020

Choose a reason for hiding this comment

jonatack Nov 24, 2020

Choose a reason for hiding this comment

maflcko left a comment

Choose a reason for hiding this comment

maflcko commented Nov 25, 2020

jonatack commented Nov 16, 2020 •

edited

Loading

DrahtBot commented Nov 16, 2020 •

edited

Loading

jonatack commented Nov 18, 2020 •

edited

Loading

jonatack Nov 18, 2020 •

edited

Loading

achow101 Nov 18, 2020 •

edited

Loading

achow101 Nov 18, 2020 •

edited

Loading