Replace -upgradewallet startup option with upgradewallet RPC #15761

achow101 · 2019-04-06T03:49:13Z

-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.

laanwj · 2019-04-06T07:11:40Z

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

achow101 · 2019-04-06T15:51:23Z

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.

maflcko

Just some style nits

src/wallet/rpcwallet.cpp

maflcko · 2019-04-06T16:26:36Z

Related: #13044

DrahtBot · 2019-04-06T17:26:01Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

wallet: Avoid translating RPC errors #18699 (wallet: Avoid translating RPC errors by MarcoFalke)
rpc: remove g_rpc_node & g_rpc_chain #18647 (rpc: remove g_rpc_node & g_rpc_chain by brakmic)
rpc: remove deprecated CRPCCommand constructor #18531 (rpc: Assert that RPCArg names are equal to CRPCCommand ones by MarcoFalke)
Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs #17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)
util: Set safe permissions for data directory and wallets/ subdir #17127 (util: Correct permissions for datadir and wallets subdir by hebasto)
gui: Bilingual GUI error messages #16224 (gui: Bilingual GUI error messages by hebasto)
Wallet passive startup #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 · 2019-04-06T18:48:34Z

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 · 2019-04-06T19:04:34Z

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 · 2019-04-06T20:04:34Z

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

I would like it to be.

jnewbery · 2019-04-07T20:26:16Z

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 · 2019-04-08T14:25:38Z

Concept ACK removing the -upgradewallet argument.

Why not add upgrade option to loadwallet RPC?

achow101 · 2019-04-09T14:34:21Z

Why not add upgrade option to loadwallet RPC?

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

jnewbery · 2019-04-09T14:46:59Z

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 · 2019-04-10T06:48:45Z

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.

maflcko · 2019-04-10T13:31:32Z

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

flack · 2019-04-12T18:34:44Z

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

luke-jr · 2019-04-20T21:12:42Z

Nit: commit messages misspell upgrade multiple times

luke-jr · 2019-04-20T21:19:11Z

src/wallet/wallet.cpp

@@ -4064,67 +4064,6 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
        }
    }

-    int prev_version = walletInstance->GetVersion();
-    if (gArgs.GetBoolArg("-upgradewallet", fFirstRun))


This is removed in the wrong commit.

luke-jr

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

achow101 · 2019-04-20T23:28:42Z

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 · 2019-04-20T23:44:00Z

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.

luke-jr · 2019-04-20T23:46:08Z

src/wallet/wallet.cpp

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


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

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 · 2019-04-22T04:25:36Z

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

maflcko · 2020-04-06T21:24:57Z

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

…e out parameter

meshcollider

Code review ACK 0d32d66

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

darosior

ACK 0d32d66

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

maflcko

Going to merge this for 0.21.0

wen tests, sir?

ACK 0d32d66 🚵

Show signature and timestamp

Signature:

Timestamp of file with hash 7716a881705c80f791d9f508a2608fc565f0f69a935156d7ee952e846c5c6af2 -

maflcko · 2020-04-19T11:02:58Z

src/wallet/wallet.cpp

@@ -3831,7 +3831,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
    }

    if (gArgs.GetBoolArg("-upgradewallet", false)) {
-        if (!UpgradeWallet(walletInstance, gArgs.GetBoolArg("-upgradewallet", 0), error, warnings)) {
+        if (!UpgradeWallet(gArgs.GetBoolArg("-upgradewallet", 0), error, warnings)) {


in commit 1e48796

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

Luckily it doesn't compile:

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

maflcko · 2020-04-19T11:04:19Z

src/wallet/rpcwallet.cpp

+        "\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"}


Suggested change

{"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", 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

…dewallet RPC 0d32d66 Remove -upgradewallet startup option (Andrew Chow) 92263cc Add upgradewallet RPC (Andrew Chow) 1e48796 Make UpgradeWallet a member function of CWallet (Andrew Chow) c988f27 Have UpgradeWallet take the version to upgrade to and an error message out parameter (Andrew Chow) 1833237 Only run UpgradeWallet if the wallet needs to be upgraded (Andrew Chow) 9c16b17 Move wallet upgrading to its own function (Andrew Chow) Pull request description: `-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. ACKs for top commit: meshcollider: Code review ACK 0d32d66 darosior: ACK 0d32d66 MarcoFalke: ACK 0d32d66 🚵 Tree-SHA512: b425bf6f5d605e26506889d63c780895482f07cbc086193218e031e8504d3072d41e90d65cd41bcc98ee4c1eb048954bc5d4ac85435f7394892373aac89a3b0a

Summary: Backport of Core [[bitcoin/bitcoin#15761 | PR15761]] [1/6] : bitcoin/bitcoin@9c16b17 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7922

Summary: Backport of Core [[bitcoin/bitcoin#15761 | PR15761]] [2/6] : bitcoin/bitcoin@1833237 Depends on D7922 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7923

…e out parameter Summary: Backport of Core [[bitcoin/bitcoin#15761 | PR15761]] [3/6] : bitcoin/bitcoin@c988f27 Depends on D7923 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7924

Summary: Backport of Core [[bitcoin/bitcoin#15761 | PR15761]] [4/6] : bitcoin/bitcoin@1e48796 Depends on D7924 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7925

Summary: Backport of Core [[bitcoin/bitcoin#15761 | PR15761]] [5/6] : bitcoin/bitcoin@92263cc Depends on D7925 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D7926

Summary: Backport of Core [[bitcoin/bitcoin#15761 | PR15761]] [6/6] : bitcoin/bitcoin@0d32d66 Depends on D7926 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7927

fanquake added RPC/REST/ZMQ Wallet labels Apr 6, 2019

achow101 force-pushed the upgradewallet-rpc branch from 36772ea to 0612304 Compare April 6, 2019 15:54

maflcko reviewed Apr 6, 2019

View reviewed changes

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

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

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

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

achow101 force-pushed the upgradewallet-rpc branch from 0612304 to 65e296d Compare April 6, 2019 16:56

achow101 mentioned this pull request Apr 6, 2019

Native descriptor wallets #15764

Closed

DrahtBot added the Needs rebase label Apr 17, 2019

luke-jr reviewed Apr 20, 2019

View reviewed changes

luke-jr suggested changes Apr 20, 2019

View reviewed changes

achow101 force-pushed the upgradewallet-rpc branch from 65e296d to bd92de2 Compare April 20, 2019 23:27

luke-jr reviewed Apr 20, 2019

View reviewed changes

achow101 force-pushed the upgradewallet-rpc branch from bd92de2 to 30b71d3 Compare April 22, 2019 04:25

DrahtBot mentioned this pull request Apr 5, 2020

rpc: remove deprecated CRPCCommand constructor #18531

Merged

DrahtBot added the Needs rebase label Apr 6, 2020

achow101 added 6 commits April 13, 2020 13:20

Move wallet upgrading to its own function

9c16b17

Only run UpgradeWallet if the wallet needs to be upgraded

1833237

Have UpgradeWallet take the version to upgrade to and an error messag…

c988f27

…e out parameter

Make UpgradeWallet a member function of CWallet

1e48796

Add upgradewallet RPC

92263cc

Remove -upgradewallet startup option

0d32d66

achow101 force-pushed the upgradewallet-rpc branch from 238f7df to 0d32d66 Compare April 13, 2020 17:36

DrahtBot removed the Needs rebase label Apr 13, 2020

DrahtBot mentioned this pull request Apr 15, 2020

rpc: remove g_rpc_node & g_rpc_chain #18647

Closed

meshcollider reviewed Apr 16, 2020

View reviewed changes

darosior approved these changes Apr 16, 2020

View reviewed changes

DrahtBot mentioned this pull request Apr 18, 2020

wallet: Avoid translating RPC errors #18699

Merged

maflcko approved these changes Apr 19, 2020

View reviewed changes

maflcko merged commit b470c75 into bitcoin:master Apr 19, 2020

This was referenced Apr 21, 2020

wallet: Remove -upgradewallet from dummywallet #18730

Closed

Can't upgrade wallet with 0.17.0's new -upgradewallet feature #14422

Closed

jnewbery mentioned this pull request Apr 24, 2020

[RFC] Long term plan for wallet command-line args #13044

Closed

maflcko mentioned this pull request Apr 25, 2020

test: Write tests for upgradewallet RPC #18767

Closed

vertiond mentioned this pull request Jan 5, 2021

Replace -upgradewallet startup option with upgradewallet RPC vertcoin-project/vertcoin-core#131

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

Replace -upgradewallet startup option with upgradewallet RPC #15761

Replace -upgradewallet startup option with upgradewallet RPC #15761

achow101 commented Apr 6, 2019

laanwj commented Apr 6, 2019

achow101 commented Apr 6, 2019

maflcko left a comment

maflcko commented Apr 6, 2019

DrahtBot commented Apr 6, 2019 •

edited

Loading

jnewbery commented Apr 6, 2019

sipa commented Apr 6, 2019

achow101 commented Apr 6, 2019

jnewbery commented Apr 7, 2019 •

edited

Loading

promag commented Apr 8, 2019

achow101 commented Apr 9, 2019

jnewbery commented Apr 9, 2019

achow101 commented Apr 10, 2019

maflcko commented Apr 10, 2019

flack commented Apr 12, 2019

luke-jr commented Apr 20, 2019

luke-jr Apr 20, 2019

achow101 Apr 20, 2019

luke-jr left a comment

achow101 commented Apr 20, 2019

luke-jr commented Apr 20, 2019

luke-jr Apr 20, 2019

achow101 Apr 22, 2019

achow101 commented Apr 22, 2019

maflcko commented Apr 6, 2020 •

edited

Loading

meshcollider left a comment

darosior left a comment

maflcko left a comment

maflcko Apr 19, 2020

maflcko Apr 20, 2020

maflcko Apr 19, 2020

	{"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", WalletFeature::LATEST), "The version number to upgrade to. Default is the latest wallet version"}

Replace -upgradewallet startup option with upgradewallet RPC #15761

Replace -upgradewallet startup option with upgradewallet RPC #15761

Conversation

achow101 commented Apr 6, 2019

laanwj commented Apr 6, 2019

achow101 commented Apr 6, 2019

maflcko left a comment

Choose a reason for hiding this comment

maflcko commented Apr 6, 2019

DrahtBot commented Apr 6, 2019 • edited Loading

Conflicts

jnewbery commented Apr 6, 2019

sipa commented Apr 6, 2019

achow101 commented Apr 6, 2019

jnewbery commented Apr 7, 2019 • edited Loading

promag commented Apr 8, 2019

achow101 commented Apr 9, 2019

jnewbery commented Apr 9, 2019

achow101 commented Apr 10, 2019

maflcko commented Apr 10, 2019

flack commented Apr 12, 2019

luke-jr commented Apr 20, 2019

luke-jr Apr 20, 2019

Choose a reason for hiding this comment

achow101 Apr 20, 2019

Choose a reason for hiding this comment

luke-jr left a comment

Choose a reason for hiding this comment

achow101 commented Apr 20, 2019

luke-jr commented Apr 20, 2019

luke-jr Apr 20, 2019

Choose a reason for hiding this comment

achow101 Apr 22, 2019

Choose a reason for hiding this comment

achow101 commented Apr 22, 2019

maflcko commented Apr 6, 2020 • edited Loading

meshcollider left a comment

Choose a reason for hiding this comment

darosior left a comment

Choose a reason for hiding this comment

maflcko left a comment

Choose a reason for hiding this comment

maflcko Apr 19, 2020

Choose a reason for hiding this comment

maflcko Apr 20, 2020

Choose a reason for hiding this comment

maflcko Apr 19, 2020

Choose a reason for hiding this comment

DrahtBot commented Apr 6, 2019 •

edited

Loading

jnewbery commented Apr 7, 2019 •

edited

Loading

maflcko commented Apr 6, 2020 •

edited

Loading