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

jonasschnelli · 2020-10-20T15:56:48Z

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.

achow101 · 2020-10-20T18:42:48Z

ACK 4dd26ef6e65f09f187d947497f15ada80ed5e6f5

DrahtBot · 2020-10-21T01:30:24Z

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

Conflicts

No conflicts as of last run.

maflcko

left a test nit. No opinion on the behavior change

test/functional/wallet_multiwallet.py

luke-jr · 2020-10-24T19:29:26Z

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 · 2020-10-29T11:24:03Z

@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 · 2020-10-29T12:24:04Z

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

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.

maflcko · 2020-10-29T14:10:36Z

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

hebasto

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

hebasto · 2020-10-30T21:08:09Z

src/wallet/load.cpp

4dd26ef6e65f09f187d947497f15ada80ed5e6f5

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

Suggested change

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

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

promag

Code review ACK but like Marco mentions this should be squashed.

@jonasschnelli I've squashed but kept the test f68091d like:

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

jonasschnelli · 2020-11-03T10:36:14Z

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

maflcko · 2020-11-03T10:45:38Z

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

dublicate -> duplicate

jonasschnelli · 2020-11-03T10:47:55Z

@MarcoFalke: thanks for spotting. Fixed.

maflcko · 2020-11-03T10:57:38Z

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 · 2020-11-03T11:03:31Z

Fixed the steps in OP.

meshcollider

Code review ACK 58cfbc3

maflcko · 2020-11-04T12:54:22Z

I am getting an "Er" "Error:" when testing

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

jonasschnelli · 2020-11-04T13:33:22Z

@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 · 2020-11-04T14:07:13Z

@MarcoFalke are you able to resize the error dialog?

After

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

I get

maflcko · 2020-11-04T15:16:51Z

src/wallet/load.cpp

    try {
+        std::set<fs::path> wallet_paths;
        for (const std::string& name : gArgs.GetArgs("-wallet")) {
+            if (!wallet_paths.insert(name).second) {


Suggested change

if (!wallet_paths.insert(name).second) {

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

you'll probably need something like this

re: #20199 (comment)

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

re: #20199 (comment)

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.

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.

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

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.

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 wallet: ignore (but warn) on duplicate -wallet parameters #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 wallet: ignore (but warn) on duplicate -wallet parameters #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.

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

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

achow101 · 2020-11-04T22:16:56Z

Tested ACK 58cfbc3

…arameters 58cfbc3 Ignoring (but warn) on duplicate -wallet parameters (Jonas Schnelli) Pull request description: 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. ACKs for top commit: achow101: Tested ACK 58cfbc3 meshcollider: Code review ACK 58cfbc3 ryanofsky: Code review ACK 58cfbc3. Changes since previous review: rebased, tweaked warning message, squashed/fixed test Tree-SHA512: f94e5a999bdd7dc291f0bc142911b0a8033929350d6f6a35b58c4a06a3c8f83147be0f0c402d4e946dedbbcc85b7e023b672c731b6d7a8984d4780017c961cfb

Summary: 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. This is a backport of [[bitcoin/bitcoin#20199 | core#20199]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10718

jonasschnelli added the Wallet label Oct 20, 2020

jonasschnelli added this to the 0.21.0 milestone Oct 20, 2020

This was referenced Oct 21, 2020

multiprocess: Add bitcoin-gui -ipcconnect option #19461

Draft

multiprocess: Add bitcoin-wallet -ipcconnect option #19460

Draft

maflcko reviewed Oct 21, 2020

View reviewed changes

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

DrahtBot mentioned this pull request Oct 21, 2020

Multiprocess bitcoin #10102

Draft

DrahtBot mentioned this pull request Oct 29, 2020

wallet: Create named SQLite wallet files instead of wallet directories #20260

Closed

ryanofsky approved these changes Oct 29, 2020

View reviewed changes

This was referenced Oct 30, 2020

Disable and fix tests for when BDB is not compiled #20267

Merged

tests: Update more tests to work with descriptor wallets #18788

Merged

hebasto reviewed Oct 30, 2020

View reviewed changes

promag reviewed Nov 1, 2020

View reviewed changes

maflcko added the Waiting for author label Nov 2, 2020

jnewbery changed the title ~~Ignoring (but warn) on dublicate -wallet parameters~~ Ignoring (but warn) on duplicate -wallet parameters Nov 2, 2020

jnewbery changed the title ~~Ignoring (but warn) on duplicate -wallet parameters~~ wallet: ignore (but warn) on duplicate -wallet parameters Nov 2, 2020

maflcko removed the Waiting for author label Nov 2, 2020

DrahtBot added the Needs rebase label Nov 2, 2020

jonasschnelli force-pushed the 2020/10/de-duplicate-wallets branch from 3dc473d to f8ca45a Compare November 3, 2020 10:29

jonasschnelli force-pushed the 2020/10/de-duplicate-wallets branch from f8ca45a to 39887b8 Compare November 3, 2020 10:37

jonasschnelli force-pushed the 2020/10/de-duplicate-wallets branch from 39887b8 to 4c27e28 Compare November 3, 2020 10:48

Ignoring (but warn) on duplicate -wallet parameters

58cfbc3

jonasschnelli force-pushed the 2020/10/de-duplicate-wallets branch from 4c27e28 to 58cfbc3 Compare November 3, 2020 11:07

DrahtBot removed the Needs rebase label Nov 3, 2020

fanquake requested a review from meshcollider November 4, 2020 02:00

meshcollider reviewed Nov 4, 2020

View reviewed changes

maflcko reviewed Nov 4, 2020

View reviewed changes

ryanofsky approved these changes Nov 4, 2020

View reviewed changes

maflcko merged commit 83650e4 into bitcoin:master Nov 5, 2020

maflcko mentioned this pull request Nov 5, 2020

error message is empty when no translation is available bitcoin-core/gui#128

Closed

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

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

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

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

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

Uh oh!

Conversation

jonasschnelli commented Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Oct 20, 2020

Uh oh!

DrahtBot commented Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

luke-jr commented Oct 24, 2020

Uh oh!

laanwj commented Oct 29, 2020

Uh oh!

ryanofsky commented Oct 29, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Oct 29, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto Oct 30, 2020

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Nov 3, 2020

Uh oh!

maflcko commented Nov 3, 2020

Uh oh!

jonasschnelli commented Nov 3, 2020

Uh oh!

maflcko commented Nov 3, 2020

Uh oh!

jonasschnelli commented Nov 3, 2020

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Nov 4, 2020

Uh oh!

jonasschnelli commented Nov 4, 2020

Uh oh!

promag commented Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko Nov 4, 2020

Choose a reason for hiding this comment

Uh oh!

ryanofsky Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Nov 4, 2020

Choose a reason for hiding this comment

Uh oh!

maflcko Nov 4, 2020

Choose a reason for hiding this comment

Uh oh!

ryanofsky Nov 4, 2020

Choose a reason for hiding this comment

Uh oh!

achow101 Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

jonasschnelli commented Oct 20, 2020 •

edited

Loading

DrahtBot commented Oct 21, 2020 •

edited

Loading

promag commented Nov 4, 2020 •

edited

Loading

ryanofsky Nov 4, 2020 •

edited

Loading

achow101 Nov 4, 2020 •

edited

Loading

ryanofsky Nov 4, 2020 •

edited

Loading