wallet: ignore (but warn) on duplicate -wallet parameters

jonasschnelli commented at 3:56 pm on October 20, 2020: contributor

I expect that there are many users with load on startup wallet definitions in bitcoin.conf or via startup CLI argument. With the new settings.json r/w configuration file, users unloading and loading a wallet through the GUI or via the RPC calls might end up with a duplicate -wallet entry (one that still remains in bitcoin.conf or CLI) plus the new duplication in settings.json due to the unload/load.

Steps to reproduce

create wallet (if via RPC set load_on_startup or unloadwallet/loadwallet then set load_on_startup).
stop bitcoin
start bitcoind again with same --wallet=mywallet

I guess it is acceptable to skip duplicates.

jonasschnelli added the label Wallet on Oct 20, 2020

jonasschnelli added this to the milestone 0.21.0 on Oct 20, 2020

achow101 commented at 6:42 pm on October 20, 2020: member

ACK 4dd26ef6e65f09f187d947497f15ada80ed5e6f5

DrahtBot commented at 1:30 am on October 21, 2020: member

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

Conflicts

No conflicts as of last run.

in test/functional/wallet_multiwallet.py:138 in 3dc473d81b outdated

119@@ -120,9 +120,6 @@ def wallet_file(name):
120         self.nodes[0].assert_start_raises_init_error(['-walletdir=wallets'], 'Error: Specified -walletdir "wallets" is a relative path', cwd=data_dir())
121         self.nodes[0].assert_start_raises_init_error(['-walletdir=debug.log'], 'Error: Specified -walletdir "debug.log" is not a directory', cwd=data_dir())
122 
123-        # should not initialize if there are duplicate wallets
124-        self.nodes[0].assert_start_raises_init_error(['-wallet=w1', '-wallet=w1'], 'Error: Error loading wallet w1. Duplicate -wallet filename specified.')

MarcoFalke commented at 8:40 am on October 21, 2020:

The test can probably be kept, using node.stop_node(expected_stderr='War...

Edit: Also needs to be squashed to not break git bisect on this test case

MarcoFalke commented at 8:40 am on October 21, 2020: member

left a test nit. No opinion on the behavior change

luke-jr commented at 7:29 pm on October 24, 2020: member

Seems like a better fix would be to not save wallets in settings.json if they’re specified by config/commandline? Seems pretty weird to run with a -wallet option one time, and get it autoloaded from then on…

laanwj commented at 11:24 am on October 29, 2020: member

@luke-jr Yes, managing manual loading versus command-line loading in a separate way to avoid this problem (e.g. a flag per wallet?) could be a further improvement.

I do think this is at least a lot better than loading wallets twice or sudden startup errors.

ryanofsky commented at 12:24 pm on October 29, 2020: member

Seems pretty weird to run with a -wallet option one time, and get it autoloaded from then on…

I don’t think that’s what’s happening. List of wallets to load is only modified if you call createwallet/loadwallet/unloadwallet with load_on_startup=True, or if you cread/load/unload interactively from the GUI

ryanofsky approved

ryanofsky commented at 12:37 pm on October 29, 2020: member

Code review ACK 3dc473d81b933fc3b803d45d3c7d687189cebd6c. (Just realized I never acked this. I think I waited after seeing the test fail initially).

This change seems more good than bad. Current structure of the code doesn’t allow implementation to be more simple, and I think the set of problems this fixes is pretty obscure. But the problems are real and it seems sensible for duplicate wallet settings to trigger a warning instead of an error.

In case this PR conflicts with #20186 and one of the two PRs needs to be rebased, I would want to prioritize #20186 because I think it fixes a more noticeable regression. But it doesn’t look like the two PRs conflict.

MarcoFalke commented at 2:10 pm on October 29, 2020: member

This still breaks git bisect, so it can’t be merged: #20199 (review)

in src/wallet/load.cpp:68 in 4dd26ef6e6 outdated

64@@ -65,8 +65,8 @@ bool VerifyWallets(interfaces::Chain& chain)
65         const fs::path path = fs::absolute(wallet_file, GetWalletDir());
66 
67         if (!wallet_paths.insert(path).second) {
68-            chain.initError(strprintf(_("Error loading wallet %s. Duplicate -wallet filename specified."), wallet_file));
69-            return false;
70+            chain.initWarning(strprintf(_("Ignoring duplicate -wallet filename %s."), wallet_file));

hebasto commented at 9:08 pm on October 30, 2020:

4dd26ef6e65f09f187d947497f15ada80ed5e6f5

nit: The term “filename” seems a bit incorrect for the -wallet argument value:

0            chain.initWarning(strprintf(_("Ignoring duplicate -wallet=%s argument."), wallet_file));

hebasto commented at 9:08 pm on October 30, 2020: member

Approach ACK 3dc473d81b933fc3b803d45d3c7d687189cebd6c, tested on Linux Mint 20 (x86_64).

promag commented at 12:11 pm on November 1, 2020: member

Code review ACK but like Marco mentions this should be squashed. @jonasschnelli I’ve squashed but kept the test f68091d06d3e95094da38491d6f6e852af4a3540 like:

0        # should warn if there are duplicate wallets
1        self.start_node(0, ['-wallet=w1', '-wallet=w1'])
2        self.stop_node(0, 'Warning: Ignoring duplicate -wallet filename w1.')

MarcoFalke added the label Waiting for author on Nov 2, 2020

jnewbery renamed this:
~~Ignoring (but warn) on dublicate -wallet parameters~~
Ignoring (but warn) on duplicate -wallet parameters
on Nov 2, 2020

jnewbery renamed this:
~~Ignoring (but warn) on duplicate -wallet parameters~~
wallet: ignore (but warn) on duplicate -wallet parameters
on Nov 2, 2020

MarcoFalke removed the label Waiting for author on Nov 2, 2020

DrahtBot added the label Needs rebase on Nov 2, 2020

jonasschnelli force-pushed on Nov 3, 2020

jonasschnelli commented at 10:36 am on November 3, 2020: contributor

Rebased and squashed. Re-added the test after @MarcoFalke and @promag recommendation.

jonasschnelli force-pushed on Nov 3, 2020

MarcoFalke commented at 10:45 am on November 3, 2020: member

nit: There is a typo in the commit message, too

dublicate -> duplicate

jonasschnelli commented at 10:47 am on November 3, 2020: contributor

@MarcoFalke: thanks for spotting. Fixed.

jonasschnelli force-pushed on Nov 3, 2020

MarcoFalke commented at 10:57 am on November 3, 2020: member

I presume the steps to reproduce in OP no longer work. The first two steps need to be removed and the third one replaced with “create wallet (if via RPC set load_on_startup)”

jonasschnelli commented at 11:03 am on November 3, 2020: contributor

Fixed the steps in OP.

Ignoring (but warn) on duplicate -wallet parameters 58cfbc38e0

jonasschnelli force-pushed on Nov 3, 2020

DrahtBot removed the label Needs rebase on Nov 3, 2020

fanquake requested review from meshcollider on Nov 4, 2020

meshcollider commented at 7:04 am on November 4, 2020: contributor

Code review ACK 58cfbc38e040925b51cb8d35d23b50e9cf06fb2a

MarcoFalke commented at 12:54 pm on November 4, 2020: member

I am getting an “Er” “Error:” when testing

0$ ./src/qt/bitcoin-qt -regtest -datadir=/tmp -wallet=/tmp/regtest/wallets/load/ -wallet=load

Screenshot from 2020-11-04 13-52-33

jonasschnelli commented at 1:33 pm on November 4, 2020: contributor

@MarcoFalke: I’m getting a different error (seems valid though): ./src/qt/bitcoin-qt –regtest –server –datadir=/tmp/dummy4 –wallet=/tmp/dummy4/regtest/wallets/test –wallet=test

Can you track down the error and why you get an empty string?

promag commented at 2:07 pm on November 4, 2020: member

@MarcoFalke are you able to resize the error dialog?

After

0$ mkdir -p /tmp/regtest/wallets/load
1$ ./src/qt/bitcoin-qt -regtest -datadir=/tmp -wallet=/tmp/regtest/wallets/load/ -wallet=load

I get Screenshot 2020-11-04 at 14 06 41

in src/wallet/load.cpp:95 in 58cfbc38e0

89@@ -90,7 +90,11 @@ bool VerifyWallets(interfaces::Chain& chain)
90 bool LoadWallets(interfaces::Chain& chain)
91 {
92     try {
93+        std::set<fs::path> wallet_paths;
94         for (const std::string& name : gArgs.GetArgs("-wallet")) {
95+            if (!wallet_paths.insert(name).second) {

MarcoFalke commented at 3:16 pm on November 4, 2020:

0            if (!wallet_paths.insert(fs::absolute(name, GetWalletDir()).second) {

you’ll probably need something like this

ryanofsky commented at 5:12 pm on November 4, 2020:

re: #20199 (review)

you’ll probably need something like this

This doesn’t seem like this best thing. What is it supposed to do? The name specified -wallet=<name> is significant and shouldn’t be normalized or transformed for things to work right now. It needs to be used as the RPC endpoint name, so for example must match the bitcoin-cli -rpcwallet=<name> string exactly and will be shown in listwallets and getwalletinfo exactly.

We should figure out what’s causing the error you reported #20199 (comment). It seems promag and jonas tried to reproduce it but haven’t been able, if I’m following correctly

ryanofsky commented at 5:25 pm on November 4, 2020:

re: #20199 (review)

We should figure out what’s causing the error you reported #20199 (comment). It seems promag and jonas tried to reproduce it but haven’t been able, if I’m following correctly

I’d also be curious to know if that error is caused by this PR or happens without it. We know this PR is a reasonable tweak and narrow fix for a specific issue. So if there’s some related problem that this PR doesn’t fix, it could make sense to address separately.

MarcoFalke commented at 7:27 pm on November 4, 2020:

What is it supposed to do?

It is simply using the same duplicate check when loading all wallet that is used previously when verifying all wallets. The name isn’t normalized or touched at all. The absolute path is only used to check for duplicates.

It seems promag and jonas tried to reproduce it but haven’t been able

Jonas has reproduced it. I suspect the error message is different because jonas used a bdb wallet and I used a sqlite wallet.

I’d also be curious to know if that error is caused by this PR or happens without it.

The error won’t happen without this pr because it will fail early if a duplicate wallet is specified.

ryanofsky commented at 8:19 pm on November 4, 2020:

It seems promag and jonas tried to reproduce it but haven’t been able

Jonas has reproduced it. I suspect the error message is different because jonas used a bdb wallet and I used a sqlite wallet.

Is this referring to #20199 (comment)? The error Jonas reported there is a legitimate error that should not be a warning because the two wallet names are not the same and the wallets have different RPC endpoints. But it would be ideal to skip the warning in that case and only show the error. So that is a good point. I believe the solution would be to change wallet_paths.insert(path) to wallet_paths.insert(wallet_file) in the VerifyWallets function

achow101 commented at 10:21 pm on November 4, 2020:

Jonas has reproduced it. I suspect the error message is different because jonas used a bdb wallet and I used a sqlite wallet.

There are two different errors each related to the respective database type used.

When BDB is used (as Jonas did), the error is because of BDB’s requirement of unique wallet files. It is opening the second wallet and seeing that it’s BDB ID is one that is already loaded. So it gives the error that the wallet.dat file is already loaded.

When SQLite is used (as Marco did), the error is because an exclusive lock has been acquired on the database file already and another one cannot be acquired on the same file. It seems like that error message isn’t being propagated up to the dialog, but clicking “Show details” will show the error.

Both issues would be solved as Marco proposes, but I agree with @ryanofsky here that these are different wallet names and we would treat them as separate wallets if we ignored these DB specific errors.

ryanofsky commented at 11:28 pm on November 4, 2020:

Just to add on, I think this PR does what it intended and is good in it’s current form, but could be improved now or in the future to:

Check for duplicate wallet_file values instead of duplicate path values in VerifyWallets() to avoid pointlessly showing a warning dialog before showing a different error dialog as described #20199 (comment). The warning could also be disabled in more cases (to exclude settings.json wallets) or just dropped entirely and never shown. But maybe the warning is useful for catching accidental misconfigurations.
Avoid the blank error dialog described #20199 (comment). Probably the GUI should default to showing the original string instead of an empty string if a translation isn’t available.
Improve the error text “Unable to obtain an exclusive lock on the database, is it being used by another bitcoind?”. Current text doesn’t mention the filename, and in this case the lock is held by the same bitcoin-qt process not a different bitcoind process. Something like “Cannot open database file ‘{m_file_path}’ which is already opened, perhaps in a different process”’ might be better.

MarcoFalke commented at 6:53 am on November 5, 2020:

Ok, clearly the wallet.dat files are the same, but I didn’t know that they get different RPC endpoints (even when in the default wallets folder). Thanks for pointing that out.

ryanofsky approved

ryanofsky commented at 5:19 pm on November 4, 2020: member

Code review ACK 58cfbc38e040925b51cb8d35d23b50e9cf06fb2a. Changes since previous review: rebased, tweaked warning message, squashed/fixed test

achow101 commented at 10:16 pm on November 4, 2020: member

Tested ACK 58cfbc38e040925b51cb8d35d23b50e9cf06fb2a

MarcoFalke merged this on Nov 5, 2020

MarcoFalke closed this on Nov 5, 2020

sidhujag referenced this in commit 3ae418f297 on Nov 5, 2020

deadalnix referenced this in commit d4f10e131a on Dec 22, 2021

DrahtBot locked this on Feb 15, 2022

wallet: ignore (but warn) on duplicate -wallet parameters #20199

Conflicts