Add loadwallet and createwallet load_on

ryanofsky commented at 9:50 pm on May 1, 2019: member

This maintains a persistent list of wallets stored in settings that will automatically be loaded on startup. Being able to load a wallet automatically on startup will be more useful in the GUI, but it’s reasonable to expose this feature by RPC as well.

DrahtBot added the label Build system on May 1, 2019

DrahtBot added the label GUI on May 1, 2019

DrahtBot added the label RPC/REST/ZMQ on May 1, 2019

DrahtBot added the label Tests on May 1, 2019

DrahtBot added the label Utils/log/libs on May 1, 2019

DrahtBot added the label Wallet on May 1, 2019

DrahtBot commented at 0:41 am on May 2, 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:

#19099 (refactor: Move wallet methods out of chain.h and node.h by ryanofsky)
#18654 (rpc: separate bumpfee’s psbt creation function into psbtbumpfee by achow101)
#16365 (Log RPC parameters (arguments) if -debug=rpcparams by LarryRuane)
#15719 (Wallet passive startup 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.

ryanofsky referenced this in commit f6a9cf0700 on May 3, 2019

ryanofsky force-pushed on May 3, 2019

ryanofsky referenced this in commit 39005eb09f on May 7, 2019

ryanofsky force-pushed on May 7, 2019

ryanofsky referenced this in commit 70675c3e49 on May 8, 2019

ryanofsky force-pushed on May 8, 2019

DrahtBot added the label Needs rebase on May 16, 2019

jonasschnelli commented at 10:23 am on May 18, 2019: contributor

Strong Concept ACK “Loosing” the wallets especially in conjunction with pruning is a concept hard to understand.

ryanofsky force-pushed on Jun 8, 2019

DrahtBot removed the label Needs rebase on Jun 8, 2019

in src/wallet/rpcwallet.cpp:2705 in 9d5c780dcb outdated

2700@@ -2688,6 +2701,11 @@ static UniValue createwallet(const JSONRPCRequest& request)
2701         flags |= WALLET_FLAG_BLANK_WALLET;
2702     }
2703 
2704+    bool load_on_startup = false;
2705+    if (!request.params[4].isNull() && request.params[4].isBool()) {

promag commented at 2:17 pm on June 25, 2019:

Nit, remove && request.params[4].isBool()?

ryanofsky commented at 1:22 am on June 5, 2020:

re: #15937 (review)

Nit, remove && request.params[4].isBool()?

Thanks, removed

in src/wallet/rpcwallet.cpp:2641 in 9d5c780dcb outdated

2636     std::string error, warning;
2637     std::shared_ptr<CWallet> const wallet = LoadWallet(*g_rpc_interfaces->chain, location, error, warning);
2638     if (!wallet) throw JSONRPCError(RPC_WALLET_ERROR, error);
2639 
2640+    if (load_on_startup && !AddWalletSetting(wallet->chain(), location.GetName())) {
2641+        throw JSONRPCError(RPC_MISC_ERROR, "Wallet loaded, but load-on-startup list could not be written, so wallet "

promag commented at 2:19 pm on June 25, 2019:

An error is thrown but wallet remains loaded. Either:

make this a warning in the response;
move AddWalletSetting() before loading - which must be reverted if the actual load fails;
unload the wallet before throwing.

ryanofsky commented at 1:17 am on June 5, 2020:

re: #15937 (review)

An error is thrown but wallet remains loaded. Either:

make this a warning in the response;

move AddWalletSetting() before loading - which must be reverted if the actual load fails;

unload the wallet before throwing.

Made warning

promag commented at 2:24 pm on June 25, 2019: member

but it’s reasonable to expose this feature by RPC as well

For GUI this makes perfect sense, for RPC I’m not sure if there’s a use case that pays off.

Quickly reviewed just 9d5c780dcb8d573aee934d110a27194a637fed8d.

laanwj referenced this in commit a7aec7ad97 on Nov 8, 2019

sidhujag referenced this in commit 16a52029e7 on Nov 9, 2019

ryanofsky referenced this in commit 5873a9c6ce on Dec 3, 2019

ryanofsky force-pushed on Dec 3, 2019

luke-jr commented at 3:32 am on January 29, 2020: member

For GUI this makes perfect sense, for RPC I’m not sure if there’s a use case that pays off.

bitcoind also autoloads wallets as configured. (And it only really makes sense in general to use bitcoin_rw.conf for this…)

ryanofsky referenced this in commit 6c6846cd58 on Jun 4, 2020

ryanofsky referenced this in commit 44849d047f on Jun 4, 2020

ryanofsky referenced this in commit 73b489863e on Jun 4, 2020

ryanofsky force-pushed on Jun 5, 2020

ryanofsky renamed this:
~~WIP: Add loadwallet and createwallet load_on_startup options~~
Add loadwallet and createwallet load_on_startup options
on Jun 5, 2020

ryanofsky marked this as ready for review on Jun 5, 2020

ryanofsky referenced this in commit 14849d8b24 on Jun 5, 2020

ryanofsky referenced this in commit eb966f421c on Jun 5, 2020

ryanofsky referenced this in commit 8f099645ac on Jun 11, 2020

ryanofsky referenced this in commit c6e86083ce on Jun 11, 2020

ryanofsky referenced this in commit 81c2e91ca9 on Jun 11, 2020

achow101 commented at 5:02 pm on June 11, 2020: member

Concept ACK. I think I’ll rebase #15454 on top of this.

ryanofsky commented at 2:04 pm on June 12, 2020: member

re: #15937#pullrequestreview-254036448

For GUI this makes perfect sense, for RPC I’m not sure if there’s a use case that pays off.

Use case is not having to manually create, edit, restart and test with a bitcoin.conf file just to use multiple wallets. I also think the gui implementation will be simpler and better tested building on the rpc implementation (even if some code has to move).

achow101 commented at 7:16 pm on June 12, 2020: member

Is it intended that the default wallet won’t be loaded anymore when a wallet is explicitly loaded? That seems like it would cause problems for users who are using both the default wallet and other wallets.

Also, why default to not loading on start? I think it makes sense that restarting would load all previously loaded wallets too.

ryanofsky commented at 8:43 pm on June 12, 2020: member

Is it intended that the default wallet won’t be loaded anymore when a wallet is explicitly loaded? That seems like it would cause problems for users who are using both the default wallet and other wallets.

Good catch. No that’s a bug. Should continue to load default wallet unless it was explicitly overridden with a -wallet or -nowallet argument or config value (as before). Will fix and add some tests.

Also, why default to not loading on start? I think it makes sense that restarting would load all previously loaded wallets too.

I think that’s be a natural followup change, and maybe a good thing to in conjunction with the GUI PR to make default RPC behavior align with GUI behavior.

Reason why it is not done here is because I would like this change to be backward compatible and not break existing workflows (or require updating lots of tests). This PR is just trying to add a simple feature to demonstrate loading and saving a setting, not be a big user facing change. But if anything here gets in the way of the larger feature, I’d be fine with closing this PR and just going for the big change.

ryanofsky referenced this in commit 15b93eeea3 on Jun 16, 2020

ryanofsky force-pushed on Jun 16, 2020

ryanofsky commented at 10:35 pm on June 16, 2020: member

re: #15937 (comment)

Is it intended that the default wallet won’t be loaded anymore when a wallet is explicitly loaded? That seems like it would cause problems for users who are using both the default wallet and other wallets.

Good catch. No that’s a bug. Should continue to load default wallet unless it was explicitly overridden with a -wallet or -nowallet argument or config value (as before). Will fix and add some tests.

Fixed. The existing test covered this case so I updated it and added some new tests.

Updated 36b3318a480eaa6fcc683b6f85ae60e809083faf -> 9b69729015eee6c70d0209396bd6d34024e2aed4 (pr/walset.7 -> pr/walset.8, compare) with default wallet fix

achow101 commented at 4:42 pm on June 18, 2020: member

ACK 9b69729015eee6c70d0209396bd6d34024e2aed4

achow101 commented at 7:19 pm on June 18, 2020: member

Did a bit of testing, and it seems another issue is that unloadwallet will cause the default wallet to no longer be loaded as well. If multiple wallets are loaded with -wallet= and then one of those is unloaded, the setting will get written without the default wallet which causes it to also not be loaded.

Additionally if a wallet is set to load on startup but is also specified in the command line, we will fail to start with a Duplicate wallet file error.

ryanofsky commented at 8:15 pm on June 18, 2020: member

Did a bit of testing, and it seems another issue is that unloadwallet will cause the default wallet to no longer be loaded as well. If multiple wallets are loaded with -wallet= and then one of those is unloaded, the setting will get written without the default wallet which causes it to also not be loaded.

This was intentional. This idea is for this pr to be backwards compatible as long as you don’t use use load_on_startup=true. Once you actively start using load_on_startup=true, it jettisons the old behavior where a default "" wallet you unloaded and didn’t ask for can get created/loaded automatically on a future startup. If a default "" wallet was already loaded for you on startup and you didn’t unload it, it will still keep being loaded (for now: after #15454 we can drop this behavior too). This gives one of the advantages from #15454 of being able to control what wallets are loaded on startup without having to edit a text file, but it keeps things backwards compatible until we roll out the more impactful #15454 changes.

If this isn’t conservative enough, I can add a load_on_startup true/false/null argument to unloadwallet RPC, where the default is to not change the startup setting instead of removing unloaded wallets from the startup list. This might be useful if maybe there are use-cases for wanting to unload a wallet temporarily but set it to be reloaded or still keep loading it on startup.

Additionally if a wallet is set to load on startup but is also specified in the command line, we will fail to start with a Duplicate wallet file error.

I think the error is actually informative and good, but I would like to improve the error message, because it’s already ambiguous and confusing when its thrown from RPCs #19232 (comment). It could be improved here or in the other PR.

achow101 commented at 8:35 pm on June 18, 2020: member

Once you actively start using load_on_startup=true, it jettisons the old behavior where a default "" wallet you unloaded and didn’t ask for can get created/loaded automatically on a future startup. If a default "" wallet was already loaded for you on startup and you didn’t unload it,

I agree that’s what should happen, but this issue was because I had somehow had a default wallet loaded on startup, unloaded a different wallet, and on restart, found that the default wallet was no longer being loaded. The expectation was that it would still be loaded because it had been loaded automatically earlier and hadn’t been unloaded.

Although now I can’t replicate this. I’m not sure what sequence of steps arrived at that.

achow101 referenced this in commit a891f82ae4 on Jun 22, 2020

in src/wallet/load.cpp:136 in 9b69729015 outdated

131+    if (unset) {
132+        // If dynamic wallet setting is being added for the first time, and the
133+        // default SoftSetArg wallet is loaded, add the default wallet to the
134+        // list so it still gets loaded next startup. This can be removed after
135+        // https://github.com/bitcoin/bitcoin/pull/15454, when there is no more
136+        // default SoftSetArg wallet.

achow101 commented at 10:47 pm on June 22, 2020:

I don’t think we can remove this behavior in #15454 as we still are loading the default wallet if it exists. So we still need this behavior there.

ryanofsky commented at 9:26 pm on June 25, 2020:

re: #15937 (review)

I don’t think we can remove this behavior in #15454 as we still are loading the default wallet if it exists. So we still need this behavior there.

Thanks, rethinking this and looking at #15454, there’s no need to have a temporary workaround here at all if WalletInit::Construct initializes the wallet setting, so the PR now does that and drops this change.

ryanofsky referenced this in commit 6ec69d5e65 on Jun 25, 2020

ryanofsky referenced this in commit 3e4f2b2c28 on Jun 25, 2020

ryanofsky force-pushed on Jun 25, 2020

ryanofsky commented at 9:36 pm on June 25, 2020: member

Rebased 9b69729015eee6c70d0209396bd6d34024e2aed4 -> cd2e0e090beb4c8f2ea61257eed76a7585643da0 (pr/walset.8 -> pr/walset.9, compare) on top of #15935 pr/rwset.13, also including changes to make load_on_startup option more flexible and simplify handling of default wallet Rebased cd2e0e090beb4c8f2ea61257eed76a7585643da0 -> 43f8faefc3e47b06ce20ffd0df85589cde7fa21d (pr/walset.9 -> pr/walset.10, compare) due to conflict with #19331 Rebased 43f8faefc3e47b06ce20ffd0df85589cde7fa21d -> 1a73d115c4b9e9ede60822d6e9fa1421c81387f2 (pr/walset.10 -> pr/walset.11, compare) on top of #15935 pr/rwset.14 Rebased 1a73d115c4b9e9ede60822d6e9fa1421c81387f2 -> 65bcf8e17a5a3c2324996d369415dde597a834e0 (pr/walset.11 -> pr/walset.12, compare) on top of #15935 pr/rwset.15 due to #19493 travis failures https://travis-ci.org/github/bitcoin/bitcoin/builds/707135059 and conflict with #18923 Rebased 65bcf8e17a5a3c2324996d369415dde597a834e0 -> 4801c7f8f5dc8f6cf09e1afe8f928d267258d2cc (pr/walset.12 -> pr/walset.13, compare) on top of #15935 pr/rwset.17, also improved documentation and added release notes Rebased 4801c7f8f5dc8f6cf09e1afe8f928d267258d2cc -> 0b146a4d682edb30b52af151eacce76d526dc91d (pr/walset.13 -> pr/walset.14, compare) after base #15935 merge Updated 0b146a4d682edb30b52af151eacce76d526dc91d -> 3062303a682492f3c90e64eb008dbdfe54e0cb60 (pr/walset.14 -> pr/walset.15, compare) fixing release note typo

ryanofsky referenced this in commit a91fdd7ae5 on Jun 25, 2020

DrahtBot added the label Needs rebase on Jul 1, 2020

ryanofsky force-pushed on Jul 2, 2020

DrahtBot removed the label Needs rebase on Jul 2, 2020

achow101 referenced this in commit 5c9231c87a on Jul 6, 2020

DrahtBot added the label Needs rebase on Jul 11, 2020

ryanofsky referenced this in commit dfe5964723 on Jul 11, 2020

ryanofsky referenced this in commit a9ba460906 on Jul 11, 2020

ryanofsky force-pushed on Jul 11, 2020

DrahtBot removed the label Needs rebase on Jul 11, 2020

DrahtBot added the label Needs rebase on Jul 11, 2020

ryanofsky referenced this in commit 7b129c849b on Jul 13, 2020

ryanofsky force-pushed on Jul 13, 2020

DrahtBot removed the label Needs rebase on Jul 13, 2020

ryanofsky referenced this in commit b25ed47202 on Jul 21, 2020

ryanofsky referenced this in commit 9c69cfe4c5 on Jul 22, 2020

ryanofsky force-pushed on Jul 23, 2020

MarcoFalke referenced this in commit f4cfa6d019 on Jul 23, 2020

MarcoFalke removed the label Build system on Jul 23, 2020

MarcoFalke removed the label GUI on Jul 23, 2020

MarcoFalke removed the label Tests on Jul 23, 2020

MarcoFalke removed the label Utils/log/libs on Jul 23, 2020

MarcoFalke added the label Settings on Jul 23, 2020

MarcoFalke added the label Needs rebase on Jul 23, 2020

MarcoFalke added the label Utils/log/libs on Jul 23, 2020

DrahtBot removed the label Needs rebase on Jul 23, 2020

ryanofsky force-pushed on Jul 23, 2020

jonatack commented at 11:45 am on July 24, 2020: member

Concept ACK.

I would definitely find this useful and use it.

sidhujag referenced this in commit bcd2ed79e3 on Jul 24, 2020

meshcollider commented at 0:12 am on August 2, 2020: contributor

Concept ACK

in test/functional/wallet_startup.py:39 in 3062303a68 outdated

34+        self.nodes[0].unloadwallet(wallet_name='w0', load_on_startup=False)
35+        self.nodes[0].unloadwallet(wallet_name='w4', load_on_startup=False)
36+        self.nodes[0].loadwallet(filename='w4', load_on_startup=True)
37+        assert_equal(set(self.nodes[0].listwallets()), set(('', 'w1', 'w2', 'w3', 'w4')))
38+        self.stop_nodes()
39+        self.start_node(0)

achow101 commented at 6:29 pm on August 5, 2020:

nit: this could be self.restart_node(0)

ryanofsky commented at 6:18 pm on August 13, 2020:

re: #15937 (review)

nit: this could be self.restart_node(0)

Thanks, updated

achow101 commented at 6:29 pm on August 5, 2020: member

ACK 3062303a682492f3c90e64eb008dbdfe54e0cb60

promag commented at 11:11 pm on August 5, 2020: member

Tested ACK 3062303a682492f3c90e64eb008dbdfe54e0cb60. Played around and works as expected.

How would you see this option in the GUI? A checkbox somewhere?

Could also test with load_on_startup omitted.

achow101 commented at 0:12 am on August 6, 2020: member

How would you see this option in the GUI? A checkbox somewhere?

For #15454, the GUI implementation is to just make this the default.

Warchant referenced this in commit 451c272143 on Aug 6, 2020

promag commented at 10:08 pm on August 7, 2020: member

@achow101 need to review #15454 again. Do you mean load_on_startup=true for menus create and load? If so, I think menu unload should set load_on_startup=false.

achow101 commented at 4:29 pm on August 10, 2020: member

Do you mean load_on_startup=true for menus create and load?

load_on_startup=true for loading, and load_on_startup=false on unloading.

DrahtBot added the label Needs rebase on Aug 13, 2020

Add loadwallet and createwallet RPC load_on_startup options

This maintains a persistent list of wallets stored in settings that will
automatically be loaded on startup. Being able to load a wallet automatically
on startup will be more useful in the GUI when the option to create wallets is
added in #15006, but it's reasonable to expose this feature by RPC as well.

642ad31b41

ryanofsky force-pushed on Aug 13, 2020

ryanofsky commented at 6:23 pm on August 13, 2020: member

Rebased 3062303a682492f3c90e64eb008dbdfe54e0cb60 -> 904bc013e36dc6ea9d013e664d5e9446aae94c7b (pr/walset.15 -> pr/walset.16, compare) due to conflict with #18654 Updated 904bc013e36dc6ea9d013e664d5e9446aae94c7b -> 61709e87c6a92e12e8ecf4cdffc058ebb565ccdc (pr/walset.16 -> pr/walset.17, compare) with test suggestion

achow101 commented at 6:35 pm on August 13, 2020: member

re-ACK 61709e87c6a92e12e8ecf4cdffc058ebb565ccdc. Only changes since last was rebase and test suggestion.

DrahtBot removed the label Needs rebase on Aug 13, 2020

in test/functional/wallet_startup.py:43 in 61709e87c6 outdated

38+        self.restart_node(0)
39+        assert_equal(set(self.nodes[0].listwallets()), set(('', 'w2', 'w4')))
40+        self.nodes[0].unloadwallet(wallet_name='', load_on_startup=False)
41+        self.nodes[0].unloadwallet(wallet_name='w4', load_on_startup=False)
42+        self.nodes[0].loadwallet(filename='w3', load_on_startup=True)
43+        self.nodes[0].loadwallet(filename='', load_on_startup=False)

meshcollider commented at 9:28 pm on August 13, 2020:

very minor suggestion: load_on_startup is already false for this wallet so it may be a more interesting test to check load without a value keeps that previous setting here.

ryanofsky commented at 9:42 pm on August 13, 2020:

re: #15937 (review)

very minor suggestion: load_on_startup is already false for this wallet so it may be a more interesting test to check load without a value keeps that previous setting here.

Good idea, updated

meshcollider commented at 9:29 pm on August 13, 2020: contributor

Code review ACK 61709e87c6a92e12e8ecf4cdffc058ebb565ccdc

ryanofsky force-pushed on Aug 13, 2020

ryanofsky commented at 9:45 pm on August 13, 2020: member

Updated 61709e87c6a92e12e8ecf4cdffc058ebb565ccdc -> 642ad31b418bbf8da06cb3641329b0810e18e55b (pr/walset.17 -> pr/walset.18, compare) with test suggestion

meshcollider commented at 10:01 pm on August 13, 2020: contributor

re-utACK 642ad31b418bbf8da06cb3641329b0810e18e55b

Only change is modification to test as suggested

achow101 commented at 10:52 pm on August 13, 2020: member

re-ACK 642ad31b418bbf8da06cb3641329b0810e18e55b Only change is the test

meshcollider merged this on Aug 15, 2020

meshcollider closed this on Aug 15, 2020

promag commented at 0:30 am on August 15, 2020: member

Do you mean load_on_startup=true for menus create and load?

load_on_startup=true for loading, and load_on_startup=false on unloading.

Not sure if you interpret create as loading, but IMO created wallets should have load_on_startup=true too.

sidhujag referenced this in commit 0294731068 on Aug 16, 2020

jonatack commented at 8:22 am on August 16, 2020: member

Posthumous strong concept ACK – looking forward to using this feature.

ryanofsky referenced this in commit 1044521921 on Oct 19, 2020

ryanofsky referenced this in commit bc2b0d99a5 on Oct 19, 2020

ryanofsky referenced this in commit e368d2bb3b on Oct 21, 2020

ryanofsky referenced this in commit 23bc6999c2 on Oct 21, 2020

ryanofsky referenced this in commit 01476a88a6 on Oct 21, 2020

MarcoFalke referenced this in commit 42b66a6b81 on Oct 29, 2020

sidhujag referenced this in commit 48063676f9 on Oct 29, 2020

sidhujag referenced this in commit c1677b430c on Nov 10, 2020

Fabcien referenced this in commit 62bae4b8a9 on Sep 27, 2021

DrahtBot locked this on Feb 15, 2022

Add loadwallet and createwallet load_on_startup options #15937

Conflicts