Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Select first Core wallet if no rpc_wallet_file specified #395

Open
wants to merge 1 commit into
base: master
from

Conversation

@kristapsk
Copy link
Contributor

commented Sep 8, 2019

Work around situations when no rpc_wallet_file is specified in joinmarket.cfg and at some point for some reason additional wallet(s) are loaded in Bitcoin Core. Before this JM crashes with exceptions in such cases. Now it will select first wallet returned by listwallets, assuming it's the default / right one.

See also bitcoin/bitcoin#16832.

@kristapsk

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2019

Tested with v0.16.3 (single wallet loaded) and current master (both with single and multiple wallets loaded).

@takinbo

This comment has been minimized.

Copy link

commented Sep 8, 2019

Naturally, if all wallets have names (the default wallet does not have one), then you should specify the one to use in rpc_wallet_file. One issue is that the user is unable to use the default wallet because rpc_wallet_file does not load the default wallet especially when it is blank.

This patch makes an assumption for the user that their intention is to use the first wallet of all loaded wallets which will result in unexpected behaviour. My suggestion will be to:

  1. If rpc_wallet_file is blank, we assume that the default wallet (with no name) is what is being referred to and load that when there are multiple loaded wallets in bitcoin core.
  2. If there's no "default wallet", that is all wallets have names, print an error requesting that the user specify the wallet to be used.
@kristapsk

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2019

all wallets have names (the default wallet does not have one)

Not in all cases. I have one v0.16.3 instance (running for years, upgraded from older versions), where default wallet is called "wallet.dat".

Probably printing error at start-up is better, I don't have strong opinion about this right now. What do others think?

In any case, there is no risk of losing funds. Wrong (unexpected) behaviour can happen if user specially unloads (closes) default wallet, has more than one wallet and JM will then try to import JM wallet addresses into new Core wallet as watch-only. But that can happen also if user creates and opens new wallet, closes the default one and then runs JM.

@AdamISZ

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

I also have "wallet.dat" as output from listwallets, I'm on 0.18.0 and have created additional wallets.
It seems like this patch is OK, no? If a user has created additional wallets and then leaves it blank it will revert to wallet.dat. I'm presuming that if a user never used multiwallet, then listwallets will either return "" or throw, or return [] ... seems like this code would handle all of those appropriately.

It looks OK to me, but @chris-belcher since you did the multiwallet PR probably it's better if you check this and merge if appropriate?

@chris-belcher

This comment has been minimized.

Copy link
Contributor

commented Sep 17, 2019

I'm also a bit concerned about this code making assumptions and possibly resulting in unexpected behavour. I have a slight (but only slight) preference for instead outputting an error message telling users to set rpc_wallet_file.

If we do keep this version of the code, then add a debug statement that explains what is happening (e.g. "required rpc wallet file not found, using: " + wallet_name) so that a user could figure what happened.

@kristapsk

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

There is one problem with quiting with an error - it's impossible currently to specify rpc_wallet_file as empty string. Then we need to figure out the way to solve that one (differenting between empty string and not having parameter at all). Also would need to do restart_cb stuff here as for GUI you want to have error message in GUI too. Guess I will just add debug message for now.

@kristapsk kristapsk force-pushed the kristapsk:rpc_wallet_file-default branch from 3c2ec4e to a29fa62 Sep 18, 2019

@kristapsk

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

Decided to log it as info, not debug, as it seems like a potentially important message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.