Replace -upgradewallet startup option with upgradewallet RPC

achow101 commented at 3:49 am on April 6, 2019: member

-upgradewallet is largely incompatible with many recent wallet features and versions. For example, it was disabled if multiple wallets were used and would not work with encrypted wallets that were being upgraded to HD.

This PR does away with the old method of upgrading upon startup and instead allows users to upgrade their wallets via an upgradewallet RPC. This does largely the same thing as the old -upgradewallet option but because the wallet is loaded, it can be unlocked to upgrade to HD. Furthermore it is compatible with multiwallet as it works on the individual wallet that is specified by the RPC.

fanquake added the label RPC/REST/ZMQ on Apr 6, 2019

fanquake added the label Wallet on Apr 6, 2019

laanwj commented at 7:11 am on April 6, 2019: member

No opposition to having this, but wouldn’t bitcoin-wallet-tool be the designated place for this?

achow101 commented at 3:51 pm on April 6, 2019: member

No opposition to having this, but wouldn’t bitcoin-wallet-tool be the designated place for this?

I’m not really a fan of putting things that users might actually use and want to use in a place that is less accessible. bitcoin-wallet-tool is command line only and many users won’t know how to use it. With an RPC, most users will be able to use it since there is a RPC console in the GUI.

achow101 force-pushed on Apr 6, 2019

in src/wallet/rpcwallet.cpp:3800 in 0612304818 outdated

3796@@ -3797,7 +3797,7 @@ UniValue sethdseed(const JSONRPCRequest& request)
3797 
3798     // Do not do anything to non-HD wallets
3799     if (!pwallet->CanSupportFeature(FEATURE_HD)) {
3800-        throw JSONRPCError(RPC_WALLET_ERROR, "Cannot set a HD seed on a non-HD wallet. Start with -upgradewallet in order to upgrade a non-HD wallet to HD");
3801+        throw JSONRPCError(RPC_WALLET_ERROR, "Cannot set a HD seed on a non-HD wallet. Use the upgradewallet command in order to upgrade a non-HD wallet to HD");

MarcoFalke commented at 4:19 pm on April 6, 2019:

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

achow101 commented at 4:56 pm on April 6, 2019:

Done

in src/wallet/rpcwallet.cpp:4021 in 0612304818 outdated

4017@@ -4018,6 +4018,92 @@ UniValue walletcreatefundedpsbt(const JSONRPCRequest& request)
4018     return result;
4019 }
4020 
4021+UniValue upgradewallet(const JSONRPCRequest& request)

MarcoFalke commented at 4:19 pm on April 6, 2019:

0static UniValue upgradewallet(const JSONRPCRequest& request)

achow101 commented at 4:56 pm on April 6, 2019:

Done

in src/wallet/rpcwallet.cpp:4032 in 0612304818 outdated

4027+        return NullUniValue;
4028+    }
4029+
4030+    if (request.fHelp || request.params.size() > 1)
4031+        throw std::runtime_error(
4032+            RPCHelpMan{"upgradewallet",

MarcoFalke commented at 4:21 pm on April 6, 2019:

Could use a const object that is in scope even when no help was not requested? That way checking the arguments could be done through the HelpMan in the future. See for example const RPCHelpMan help{"sendmany", for what I mean.

achow101 commented at 4:56 pm on April 6, 2019:

Done

in src/wallet/rpcwallet.cpp:4074 in 0612304818 outdated

4069+    // Do not upgrade versions to any version between HD_SPLIT and FEATURE_PRE_SPLIT_KEYPOOL unless already supporting HD_SPLIT
4070+    if (!pwallet->CanSupportFeature(FEATURE_HD_SPLIT) && version >= FEATURE_HD_SPLIT && version < FEATURE_PRE_SPLIT_KEYPOOL) {
4071+        throw JSONRPCError(RPC_WALLET_ERROR, "Cannot upgrade a non HD split wallet without upgrading to support pre split keypool. Please use the upgradewallet command.");
4072+    }
4073+
4074+    pwallet->SetMinVersion(version); // permanently upgrade the wallet after making sure we can upgrade

MarcoFalke commented at 4:24 pm on April 6, 2019:

This fails to compile.

Mind to make a move-only of the upgrade code into a standalone member function, that is merely called by the RPC instead of moving the whole wallet logic into the rpc file?

achow101 commented at 4:56 pm on April 6, 2019:

Done

MarcoFalke commented at 4:25 pm on April 6, 2019: member

Just some style nits

MarcoFalke commented at 4:26 pm on April 6, 2019: member

Related: #13044

achow101 force-pushed on Apr 6, 2019

DrahtBot commented at 5:26 pm on April 6, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18699 (wallet: Avoid translating RPC errors by MarcoFalke)
#18647 (rpc: remove g_rpc_node & g_rpc_chain by brakmic)
#18531 (rpc: Assert that RPCArg names are equal to CRPCCommand ones by MarcoFalke)
#17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)
#17127 (util: Correct permissions for datadir and wallets subdir by hebasto)
#16224 (gui: Bilingual GUI error messages by hebasto)
#15719 (Drop Chain::requestMempoolTransactions method by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jnewbery commented at 6:48 pm on April 6, 2019: member

Concept ACK although I agree with @laanwj that a utility function that does not require access to the chainstate or P2P rightly lives in the wallet tool.

I’m not really a fan of putting things that users might actually use and want to use in a place that is less accessible […] With an RPC, most users will be able to use it since there is a RPC console in the GUI.

I’d disagree that RPC is accessible to users who aren’t able to use the command line. We shouldn’t encourage users to type commands into the debug command window if they’re not able to use the command line. Besides, upgradewallet is currently command line only, so moving it to the wallet tool maintains exactly the same level of accessibility.

sipa commented at 7:04 pm on April 6, 2019: member

Is this something that we’d like to be accessible to wallets why they’re loaded? If so, it needs to be an RPC (and possibly also in the wallet tool).

achow101 commented at 8:04 pm on April 6, 2019: member

Is this something that we’d like to be accessible to wallets why they’re loaded?

I would like it to be.

jnewbery commented at 8:26 pm on April 7, 2019: member

Is this something that we’d like to be accessible to wallets while they’re loaded?

This seems to me to potentially add complexity in future. I could imagine there may be some potential upgrade actions that would conflict badly when executed on a running wallet. Why not keep it simple and only allow upgrade when the wallet is not running?

EDIT: to be clear, I’m not NACKing this. I think an upgradewallet RPC is certainly an improvement over the command line option, and doesn’t preclude also adding it to the wallet tool. I just think that the offline wallet tool would be an even better place for it.

promag commented at 2:25 pm on April 8, 2019: member

Concept ACK removing the -upgradewallet argument.

Why not add upgrade option to loadwallet RPC?

achow101 commented at 2:34 pm on April 9, 2019: member

Why not add upgrade option to loadwallet RPC?

I think it makes more sense to have upgrading be separate from loading.

jnewbery commented at 2:46 pm on April 9, 2019: member

I think it makes more sense to have upgrading be separate from loading.

Can you justify that? Upgrading wallets is something that most users will very rarely (if ever) have to do. What’s the benefit to adding the flexibility to upgrade wallets during runtime? The downsides seem pretty obvious to me (potential increased complexity and having to reason about interactions with other wallet operations) so I’d like to understand what you’re weighing that against.

achow101 commented at 6:48 am on April 10, 2019: member

I’d disagree that RPC is accessible to users who aren’t able to use the command line. We shouldn’t encourage users to type commands into the debug command window if they’re not able to use the command line. Besides, upgradewallet is currently command line only, so moving it to the wallet tool maintains exactly the same level of accessibility.

I disagree. It is far easier for users to understand and use the RPC console than it is for them to use and understand the wallet tool. The instructions for opening up the RPC console and then entering a command are far simpler than the instructions for using the wallet tool. There is exactly one set of instructions that users have to follow with the only thing that they need to consider is their wallet name and making sure that that wallet is selected.

Conversely, with the wallet tool, the instructions for using it are dependent on how the user installed the software (i.e. they need to know the install location) and whether they changed the data directory. Then they have to specify the wallet in the command line which may involve escaping or quoting. All of these are extra options and information that users need to know which they may not. With the RPC console, all that stuff is already handled for them. The instructions are far simpler and easier to understand when using the RPC console than when using the command line wallet tool.

In a similar vein, I disagree with “We shouldn’t encourage users to type commands into the debug command window if they’re not able to use the command line.” As I mentioned, using the RPC console is far easier than using the command line. While they are similar, the command line has so many extra rules and gotchas that the RPC console does not. A user can be fine and comfortable using the RPC console but not able to use the command line simply because the command line does many other things that can cause screwups.

Can you justify that? Upgrading wallets is something that most users will very rarely (if ever) have to do. What’s the benefit to adding the flexibility to upgrade wallets during runtime?

Conceptually, loading and upgrading are two separate functions that should be defined separately. From a UX standpoint, it makes more sense to users to do an explicit upgrade as a separate command rather than as an argument to some other command. If bundled with loadwallet, I suspect that many users would miss the important step of unloading the wallet first and generally cause headaches all around as they complain and ask how to actually upgrade their wallets. Keep in mind that while upgrades may be infrequent, many users are still using non-HD wallets and have been waiting for an easy way to upgrade their non-HD wallets to HD; upgradewallet would provide that easy method.

The downsides seem pretty obvious to me (potential increased complexity and having to reason about interactions with other wallet operations) so I’d like to understand what you’re weighing that against.

I really don’t think that’s a reason against this. By that logic we just shouldn’t change anything because it increases complexity and we have to reason about interactions with other operations.

Moving upgrading to its own code will actually make it less complex and simpler. Currently I have it as largely a move-only, but it can actually be simplified. Keeping it as part of loading means that it has to distinguish between a new wallet and a wallet being upgraded. They follow largely the same codepaths but have just slightly different logic. The fact that these two are together in CreateWalletFromFile actually makes the upgrade logic hard to understand and reason about. I had a hard time determining whether the upgrade logic was actually upgrading correctly. By separating the upgrade logic into its own function, we can clean it up so that what it does is not confusing and can be followed. This actually reduces the complexity of the code.

MarcoFalke commented at 1:31 pm on April 10, 2019: member

Concept ACK moving it to either or both (tool and rpc)

flack commented at 6:34 pm on April 12, 2019: contributor

The RPC variant probably has the advantage that the function could be added to the File menu later on (maybe right next to “Backup wallet…”), which would also alleviate the concerns about having users enter random stuff in the console

DrahtBot added the label Needs rebase on Apr 17, 2019

luke-jr commented at 9:12 pm on April 20, 2019: member

Nit: commit messages misspell upgrade multiple times

in src/wallet/wallet.cpp:3834 in 65e296dcc6 outdated

4063@@ -4064,67 +4064,6 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
4064         }
4065     }
4066 
4067-    int prev_version = walletInstance->GetVersion();
4068-    if (gArgs.GetBoolArg("-upgradewallet", fFirstRun))

luke-jr commented at 9:19 pm on April 20, 2019:

This is removed in the wrong commit.

achow101 commented at 11:28 pm on April 20, 2019:

Fixed.

luke-jr changes_requested

luke-jr commented at 9:31 pm on April 20, 2019: member

The first commit is convoluted/difficult to review and broken (-upgradewallet no longer works).

achow101 force-pushed on Apr 20, 2019

achow101 commented at 11:28 pm on April 20, 2019: member

Nit: commit messages misspell upgrade multiple times

Fixed

The first commit is convoluted/difficult to review

How so? It’s mostly just a move

broken (-upgradewallet no longer works).

Should be fixed

luke-jr commented at 11:44 pm on April 20, 2019: member

Re first commit: Since it’s on the object, you drop the walletInstance-> everywhere. I’d suggest a minimal moveonly commit, then a subsequent one that makes the changes.

in src/wallet/wallet.cpp:4044 in d9a66f9ed6 outdated

4060-        }
4061-        walletInstance->SetMaxVersion(nMaxVersion);
4062-    }
4063-
4064-    // Upgrade to HD if explicit upgrade
4065     if (gArgs.GetBoolArg("-upgradewallet", false)) {

luke-jr commented at 11:46 pm on April 20, 2019:

Part of this is supposed to run by default if fFirstRun.

achow101 commented at 3:48 am on April 22, 2019:

That part is completely redundant. It just sets the wallet version but SetMinVersion is already called below in the if (fFirstRun) block. Furthermore, that call forces the wallet version to be the latest version so whatever version number specified for -upgradewallet that would have been set is overridden anyways.

achow101 commented at 4:25 am on April 22, 2019: member

I’ve done some commit reorganization as suggested by @luke-jr so this should be easier to review now.

achow101 force-pushed on Apr 22, 2019

DrahtBot removed the label Needs rebase on Apr 22, 2019

DrahtBot added the label Needs rebase on May 29, 2019

in src/wallet/wallet.cpp:4113 in 30b71d308e outdated

4272+        return false;
4273+    }
4274+    SetMaxVersion(nMaxVersion);
4275+
4276+    // Upgrade to HD if explicit upgrade
4277+    LOCK(cs_wallet);

darosior commented at 2:30 pm on July 9, 2019:

This comment isnt related to the lock, is it ?

achow101 commented at 8:50 pm on July 9, 2019:

Removed

darosior approved

darosior commented at 2:52 pm on July 9, 2019: member

ACK 91d610f35753072990d8928c424a7940145ad10e

I tested it to upgrade a wallet from 0.11.1 (wallet version 6000) to 0.18 (wallet version 169900). It solves #16091 by letting user with encrypted old wallet to upgrade it.

achow101 force-pushed on Jul 9, 2019

achow101 commented at 8:50 pm on July 9, 2019: member

Rebased

DrahtBot removed the label Needs rebase on Jul 9, 2019

DrahtBot added the label Needs rebase on Jul 24, 2019

achow101 force-pushed on Jul 25, 2019

DrahtBot removed the label Needs rebase on Jul 25, 2019

DrahtBot added the label Needs rebase on Aug 2, 2019

achow101 force-pushed on Oct 16, 2019

DrahtBot removed the label Needs rebase on Oct 16, 2019

DrahtBot added the label Needs rebase on Oct 21, 2019

achow101 force-pushed on Oct 21, 2019

DrahtBot removed the label Needs rebase on Oct 21, 2019

achow101 force-pushed on Oct 23, 2019

DrahtBot added the label Needs rebase on Oct 29, 2019

achow101 force-pushed on Oct 29, 2019

DrahtBot removed the label Needs rebase on Oct 29, 2019

DrahtBot added the label Needs rebase on Nov 4, 2019

achow101 commented at 6:44 pm on November 4, 2019: member

This is currently getting messed up by the ScriptPubKeyMan changes. Will fix, rebase, and address comments after ScriptPubKeyMan is fully introduced.

codewiz commented at 11:30 am on November 30, 2019: none

@achow101, could you please rebase your patches?

achow101 commented at 2:32 am on December 1, 2019: member

Will fix, rebase, and address comments after ScriptPubKeyMan is fully introduced.

fanquake added the label Waiting for author on Dec 1, 2019

achow101 force-pushed on Dec 2, 2019

DrahtBot removed the label Needs rebase on Dec 3, 2019

achow101 commented at 1:17 am on December 4, 2019: member

I’ve rebased and fixed this up a bit, but it does kind of conflict with the ScriptPubKeyMan changes.

DrahtBot added the label Needs rebase on Jan 30, 2020

achow101 force-pushed on Jan 30, 2020

MarcoFalke removed the label Waiting for author on Jan 30, 2020

DrahtBot removed the label Needs rebase on Jan 30, 2020

DrahtBot added the label Needs rebase on Mar 31, 2020

darosior commented at 1:24 pm on March 31, 2020: member

Happy to review this once rebased as there are still users broken after multiple releases (it’s been almost a year now… :/), see #16091.

achow101 force-pushed on Apr 1, 2020

DrahtBot removed the label Needs rebase on Apr 1, 2020

DrahtBot added the label Needs rebase on Apr 6, 2020

MarcoFalke commented at 9:24 pm on April 6, 2020: member

Maybe split out the first commit (move-only) as a separate pull, to cut down on the conflicts in the future?

Move wallet upgrading to its own function 9c16b1735f

Only run UpgradeWallet if the wallet needs to be upgraded 1833237123

Have UpgradeWallet take the version to upgrade to and an error message out parameter c988f27937

Make UpgradeWallet a member function of CWallet 1e48796c99

Add upgradewallet RPC 92263cce5b

Remove -upgradewallet startup option 0d32d66148

achow101 force-pushed on Apr 13, 2020

DrahtBot removed the label Needs rebase on Apr 13, 2020

meshcollider commented at 2:21 am on April 16, 2020: contributor

Code review ACK 0d32d661481f099af572e7a08a50e17bcc165c44

I think this is definitely an improvement and do agree it should be an RPC not in the wallet tool. Tests? 🤔

darosior approved

darosior commented at 9:11 am on April 16, 2020: member

ACK 0d32d661481f099af572e7a08a50e17bcc165c44

Tested and it fixes both #16091 and #14422. (Used an encrypted pre-HD (0.11.1) wallet, and succesfully upgraded with upgradewallet, keypool refilled)

in src/wallet/wallet.cpp:3834 in 1e48796c99 outdated

3830@@ -3831,7 +3831,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
3831     }
3832 
3833     if (gArgs.GetBoolArg("-upgradewallet", false)) {
3834-        if (!UpgradeWallet(walletInstance, gArgs.GetBoolArg("-upgradewallet", 0), error, warnings)) {
3835+        if (!UpgradeWallet(gArgs.GetBoolArg("-upgradewallet", 0), error, warnings)) {

MarcoFalke commented at 11:02 am on April 19, 2020:

in commit 1e48796c99b63aa8fa8451ce7b0c20759ea43500

This is a bug and it needs to call walletInstance->UpgradeWallet, but I see the code is removed in the next commit anyway

MarcoFalke commented at 1:50 pm on April 20, 2020:

Luckily it doesn’t compile:

0wallet/wallet.cpp:3831:14: error: call to non-static member function without an object argument
1        if (!UpgradeWallet(gArgs.GetBoolArg("-upgradewallet", 0), error, warnings)) {
2             ^~~~~~~~~~~~~
31 error generated.

in src/wallet/rpcwallet.cpp:4261 in 92263cce5b outdated

4256+
4257+    RPCHelpMan{"upgradewallet",
4258+        "\nUpgrade the wallet. Upgrades to the latest version if no version number is specified\n"
4259+        "New keys may be generated and a new wallet backup will need to be made.",
4260+        {
4261+            {"version", RPCArg::Type::NUM, /* default */ strprintf("%d", FEATURE_LATEST), "The version number to upgrade to. Default is the latest wallet version"}

MarcoFalke commented at 11:04 am on April 19, 2020:

0            {"version", RPCArg::Type::NUM, /* default */ strprintf("%d", WalletFeature::LATEST), "The version number to upgrade to. Default is the latest wallet version"}

Unrelated nit for future cleanup: This should probably be scoped and not in the global namespace

MarcoFalke approved

MarcoFalke commented at 11:05 am on April 19, 2020: member

Going to merge this for 0.21.0

wen tests, sir?

ACK 0d32d661481f099af572e7a08a50e17bcc165c44 🚵

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 0d32d661481f099af572e7a08a50e17bcc165c44 🚵
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUjK6gv/TC3347Yc0B3KJppRFbyvMZndV7FLINr0i6jsR6RlkeXC4tqTCGat8KRC
 8QKKRgJwpE8AYLmomgQlodds6OZ0NOxGDr+XkGfYcvZSXH8y8XPjrtgLgRVPpUeFg
 9F5tfZFGLttBnKhnL/n44vV+lGEbAIwDlTpKw+86vM9r8qf00KtKdLEwhhN1ZjZsk
103EM4NdZ7wxDzGTK8URF1egnFhaJayVLLQEXyWNiV4PQTDnnEr9SAr4N0B0khROML
11ecpTlhzKGayQeYGYMNsvFJZl1FnSiuTosgjFbVRCyBHSeRvVhmEhSgGr3p9Ee9Pq
12MycIK++P+r36bI/6uQo1L15mFDSITOPENPxBeRCar1btx0C/g0tK9TU0k5ji6OKH
13cKueJdNsWxTnzJZJgNizVI/uY3UAk3MV5tN8oBkDKWXCU4kom8k5qbZKKWGBAh05
14rUech6uc17vAPuk/XWk20dwVst50h5XdlwMd9EUzPn13Quk7xe06bDkJcD7RtfWd
15g8IYq3zK
16=LvtB
17-----END PGP SIGNATURE-----

Timestamp of file with hash 7716a881705c80f791d9f508a2608fc565f0f69a935156d7ee952e846c5c6af2 -

MarcoFalke merged this on Apr 19, 2020

MarcoFalke closed this on Apr 19, 2020

sidhujag referenced this in commit 1cc5c17658 on Apr 20, 2020

deadalnix referenced this in commit 2a5a2056cb on Oct 15, 2020

deadalnix referenced this in commit 1a996fb25c on Oct 15, 2020

jasonbcox referenced this in commit ddb1c65648 on Oct 15, 2020

jasonbcox referenced this in commit daeced2a29 on Oct 15, 2020

jasonbcox referenced this in commit 9c39015044 on Oct 15, 2020

jasonbcox referenced this in commit 0ce1779c0e on Oct 15, 2020

DrahtBot locked this on Feb 15, 2022

Replace -upgradewallet startup option with upgradewallet RPC #15761

Conflicts