Remove -usewallet

jnewbery commented at 4:06 pm on July 18, 2017: member

For merge after v0.15 (or perhaps as a bugfix for v0.15)

Adds better validation of bitcoin-cli wallet argument to make sure it is only specified once, and that is only ever picked up from the command line, not from a config file (which could have bad results if someone was writing a script and forgot a command line argument).
Renames -usewallet to -wallet

If there is zero or one -wallet argument in bitcoin.conf (ie single wallet mode), there is no change from pre-0.15 behaviour. No -wallet argument is required for bitcoin.cli.

If there is more than one -wallet argument in bitcoin.conf (ie multiwallet), then a single command-line -wallet argument must be provided to bitcoin.cli.

It is always an error to specify more than one -wallet command line argument, and the request will be rejected by bitcoin.cli on the client side.

jonasschnelli added the label Wallet on Jul 18, 2017

jnewbery commented at 4:07 pm on July 18, 2017: member

@ryanofsky - should address your concern about usewallet

in src/bitcoin-cli.cpp:110 in 761c5b160c outdated

102@@ -103,8 +103,31 @@ static int AppInitRPC(int argc, char* argv[])
103         fprintf(stderr, "Error: Specified data directory \"%s\" does not exist.\n", GetArg("-datadir", "").c_str());
104         return EXIT_FAILURE;
105     }
106+    // For bitcoin-cli, the -wallet parameter can only be specified from the
107+    // command line, not in the conf file. This is because the conf file can
108+    // contain multiple -wallet args, so it's not possible to determine which
109+    // one to use.  Furthermore, if bitcoin-cli used the -wallet arg from the
110+    // conf file, we'd get 'trapped' in wallet mode and not be able to issue

ryanofsky commented at 4:41 pm on July 18, 2017:

In commit “bitcoin-cli shouldn’t read the -wallet argument from conf file”

We should no longer have trapped in wallet problem anymore because node commands should currently work on the wallet endpoint.

jnewbery commented at 6:52 pm on July 18, 2017:

Thanks. Fixed

in src/bitcoin-cli.cpp:115 in 761c5b160c outdated

110+    // conf file, we'd get 'trapped' in wallet mode and not be able to issue
111+    // commands to the node.
112+    //
113+    // Store the command line -wallet arg here so we can restore it after
114+    // parsing the conf file.
115+    bool use_wallet = false;

ryanofsky commented at 4:43 pm on July 18, 2017:

In commit “bitcoin-cli shouldn’t read the -wallet argument from conf file”

Maybe eliminate this variable and use !wallet.empty() instead.

jnewbery commented at 6:53 pm on July 18, 2017:

Yes, much better. Thanks

in src/bitcoin-cli.cpp:123 in 761c5b160c outdated

122+        use_wallet = true;
123+    }
124     try {
125         ReadConfigFile(GetArg("-conf", BITCOIN_CONF_FILENAME));
126+        // restore the command line -wallet argument
127+        gArgs.ClearArg("-wallet");

ryanofsky commented at 4:50 pm on July 18, 2017:

In commit “[utils] bitcoin-cli shouldn’t read the -wallet argument from conf file”

I think it would make control flow clearer, and also simplify the PR if you just passed wallet_name variable directly from AppInitRPC -> CallRPC without going through the gArgs class. You could do this by making wallet_name a static global in this file (static const std::string g_wallet_name;) or an output argument (std::string&) for this function that could be passed to other functions.

This way you wouldn’t need the ClearArg method or SoftSet call or need to make any changes outside of bitcoin-cli.

jnewbery commented at 6:56 pm on July 18, 2017:

I prefer not to set this as a global or pass it through main(), CommandLineRPC() and CallRPC(). It makes sense to me to set up all the config in AppInitRPC() and have it contained in the standard gArgs global.

Happy to reconsider if making changes to util.cpp is a blocker for this PR.

ryanofsky commented at 7:24 pm on July 18, 2017:

Happy to reconsider if making changes to util.cpp is a blocker for this PR.

Not a big deal to me. If someone else objects to the util.cpp change though, we should just be aware that it isn’t a critical part of the PR and there are various ways to get the same behavior.

in src/util.cpp:487 in f2c4d06a60 outdated

481@@ -482,7 +482,21 @@ void ArgsManager::ForceSetArg(const std::string& strArg, const std::string& strV
482     mapMultiArgs[strArg].push_back(strValue);
483 }
484 
485-
486+bool ArgsManager::ClearArg(const std::string& strArg)
487+{
488+    LOCK(cs_args);

ryanofsky commented at 4:59 pm on July 18, 2017:

In commit “[args] allow args to be cleared from ArgsManager”

Not sure, but I think you might be able to write this as return mapArgs.erase(strArg) | mapMultiArgs.erase(strArg). Should avoid || though because that will short circuit.

jnewbery commented at 6:52 pm on July 18, 2017:

Neat! Thanks. Fixed

ryanofsky commented at 5:25 pm on July 18, 2017: member

Thanks for doing this. I think this is a better interface, and it also addresses @morcos comment about conflicting -usewallet values. Implementation could and probably should be simplified, though.

For anybody reading the PR description, this might sound pretty complicated, but the description is mostly referring to existing behavior that isn’t changing. In particular none of what’s described about how bitcoind interprets arguments or handles RPCs is changing in this PR. This change only does two things:

Adds better validation of bitcoin-cli wallet argument to make sure it is only specified once, and that is only ever picked up from the command line, not from a config file (which could have bad results if someone was writing a script and forgot a command line argument).
Renames -usewallet to -wallet

The first part is arguably a bugfix, so maybe this could be included in 0.15.

jnewbery commented at 5:30 pm on July 18, 2017: member

@ryanofsky Thanks for the feedback. You’re right - the PR description was overly long and confusing for something that’s actually very simple. I’ve reworked it using your words.

Yes, perhaps this could go in 0.15 as a bugfix.

Will address your technical feedback soon.

sipa commented at 5:34 pm on July 18, 2017: member

If we want to replace -usewallet with -wallet for bitcoin-cli, it should go in 0.15 or we’d be breaking backward compatibility in later?

jnewbery commented at 5:46 pm on July 18, 2017: member

If we want to replace -usewallet with -wallet for bitcoin-cli, it should go in 0.15 or we’d be breaking backward compatibility in later?

Yes, but as I’ve mentioned before, I’m not very concerned about backward compatibility for an interface that should be marked as experimental.

That said, I’m happy for this to go in v0.15 as a bugfix.

sipa commented at 5:49 pm on July 18, 2017: member

I’m not very concerned about backward compatibility for an interface that should be marked as experimental.

Still no reason to go intentionally plan to break it.

jnewbery commented at 6:02 pm on July 18, 2017: member

Still no reason to go intentionally plan to break it.

Sorry, I’m confused about what you’re advocating for? I’ve already said I’m happy for this to go into v0.15 as a bugfix.

sipa commented at 6:06 pm on July 18, 2017: member

@jnewbery I think we should either decide to have it, and get it in now, or not at all.

jnewbery commented at 6:09 pm on July 18, 2017: member

ok, my vote is for having it now.

aside: on a philosophical level I disagree that we should be forever bound to an interface that hasn’t been fully agreed or designed. Presumably any users that are active enough to enable multiwallet in its first release and update all their client software are perfectly able to change usewallet to wallet at a later date.

[args] allow args to be cleared from ArgsManager 7a2bffb744

[utils] bitcoin-cli shouldn't read the -wallet argument from conf file 561d0190d3

jnewbery force-pushed on Jul 18, 2017

jnewbery commented at 6:57 pm on July 18, 2017: member

Addressed @ryanofsky’s comments

ryanofsky commented at 7:30 pm on July 18, 2017: member

utACK 561d0190d326c4bb75d9db03cabbeea10b9ec5cb

gmaxwell commented at 7:59 pm on July 18, 2017: contributor

But I want to be able to tell my daemon to load wallets one two and three, while make my bitcoin-cli default to wallet one.

ryanofsky commented at 8:06 pm on July 18, 2017: member

But I want to be able to tell my daemon to load wallets one two and three, while make my bitcoin-cli default to wallet one.

We could allow this, but the thought is that it’s dangerous because maybe you’ll forget to add an explicit -wallet=filename argument in one of the cases where you don’t intend to use your default wallet (maybe in a single bitcoin-cli invocation as part of a larger script) and then you will accidentally perform an action on the wrong wallet.

gmaxwell commented at 8:09 pm on July 18, 2017: contributor

@ryanofsky That danger doesn’t hold w the only allow one argument change.

Example usecase is joinmarket wallet and a normal wallet. Joinmarket will always use the rpc, but the cli I want to always use the normal wallet.

jnewbery commented at 8:13 pm on July 18, 2017: member

But I want to be able to tell my daemon to load wallets one two and three, while make my bitcoin-cli default to wallet one.

Then write a wrapper around your cli :slightly_smiling_face:

I agree with @ryanofsky - not having a default wallet is much safer for all users.

gmaxwell commented at 8:16 pm on July 18, 2017: contributor

A setting I explicitly set isn’t a default though. I agree it shouldn’t have a default. The argument you’re making would apply identically to the testnet vs mainnet switch (which is also multiple wallets and also unsafe if you get the wrong one).

ryanofsky commented at 8:19 pm on July 18, 2017: member

Yeah, I’m not actually against allowing users to explicitly set a default wallet. I was just trying to explain what the rationale was. @gmaxwell, are you suggesting with the one-argument limit that you might want to allow users to specify a default wallet in the conf file, but then disallow specifying a second wallet on the command line that would override it? That seems like a weird behavior, but I guess would be safe by john’s definition.

jnewbery commented at 8:22 pm on July 18, 2017: member

I agree - the testnet / mainnet switch in the .conf file is dangerous and has tripped lots of people up, including me several times. Having to specify it explicitly it for each bitcoin-cli call would be safer (but irritating).

I’m not even clear on what exact functionality you want - are you saying -usewallet in the conf file should be the default? But that it can be overridden by -usewallet on the command line?

ryanofsky commented at 8:25 pm on July 18, 2017: member

If we are ok with explicitly set default wallets, maybe it would best to have a “-defaultwallet” config setting that gets implemented on server side rather than client side. This would allow for consistent behavior even across different clients, and provide functionality that couldn’t be replicated with a simple wrapper script or command alias.

sipa commented at 8:40 pm on July 18, 2017: member

What is the rationale for renaming -usewallet to -wallet (over just rejecting duplicate -usewallet and/or having -defaultwallet)? Besides being slightly shorter, I don’t see the advantage.

ryanofsky commented at 8:47 pm on July 18, 2017: member

What is the rationale for renaming -usewallet to -wallet (over just rejecting duplicate -usewallet and/or -defaultwallet)?

From my point of view I think the consistency is good. If we just have a consistent “-wallet=filename” option accepted by bitcoind, bitcoin-qt, bitcoin-cli tools, it is harder to screw up using them. Especially if we want to have support for default wallets like Greg wants, it seems like asking for trouble expecting users to remember “-usewallet” instead of “-wallet” on the command line for this one tool, and punishing them if they forget by not showing any error and falling back to another wallet written in some config file.

sipa commented at 8:53 pm on July 18, 2017: member

I’m not sure it’s a great thing for consistency, if it needs such exceptional processing (ignoring something in the conf file is unique…).

jnewbery commented at 8:54 pm on July 18, 2017: member

Exactly what @ryanofsky said.

And less importantly, because -use is content free. All arguments are used.

ryanofsky commented at 9:10 pm on July 18, 2017: member

I’m not sure it’s a great thing for consistency, if it needs such exceptional processing (ignoring something in the conf file is unique…).

I don’t necessarily think it should be ignored in the conf file. That’s really just a quirk of the way this PR is implemented. I think my preferred implementation would check to see if a command line argument is specified, and if so override anything in the config file (obviously). Else if no command line argument is specified, and the configuration file has more than one value specified, exit with an error message, otherwise use the value from the config file. Behavior would be indistinguishable in the common case where bitcoind and bitcoin-cli share the same config file.

I think John favored current approach though to prioritize safety just always get the wallet name from the command line when an RPC call is being made.

jnewbery commented at 9:26 pm on July 18, 2017: member

That’s really just a quirk of the way this PR is implemented.

Correct - in terms of implementation, it’s simpler to just clear out the .conf file wallet argument (since wallet is a multimap). The end result is exactly as Russ has described.

I’m happy to review alternative implementations if that’s confused people.

sipa commented at 9:30 pm on July 18, 2017: member

I think my objection goes away if -wallet in the config file is not being ignored, but there are just rules that prevent specifying it twice on the command line.

sipa commented at 10:06 pm on July 18, 2017: member

After some offline discussion with @jnewbery, Concept ACK. I misunderstood how little effect “ignore the config file” had on the semantics of -wallet.

I believe the behaviour of the -wallet option can effectively be modelled as “the command-line -wallet arguments replace the config file ones as a whole, rather than appending to them”. I think that’s fine. Ideally, I think that interpretation can be extended slightly further, by making bitcoin-cli use (all) wallet= lines from the config if no -wallet is provided on the cmdline.

Having a -defaultwallet can be an independent feature that is added later, or not.

ryanofsky commented at 10:32 pm on July 18, 2017: member

Sounds good. I’m happy with this PR as it is, as well as the other options that have been discussed. Note that the current PR doesn’t exactly model command line -wallet options “replacing config file ones as a whole”, because it clears config wallet options even when no command line option specified. The change I was talking about earlier would give you that behavior (and potentially less safe results when bitcoind and bitcoin-cli read different config files). FWIW, I implemented it here https://github.com/ryanofsky/bitcoin/commit/b701085e0c35e9e09f44d8e9ba8303617e445ca3

jnewbery commented at 10:42 pm on July 18, 2017: member

@ryanofsky - I think your branch is much nicer (modulo the two things we just chatted about). Mind if I cherry pick it here?

achow101 commented at 1:54 am on July 19, 2017: member

ACK 561d0190d326c4bb75d9db03cabbeea10b9ec5cb

jonasschnelli commented at 9:32 am on July 19, 2017: contributor

Agree with @jnewbery that @ryanofsky ryanofsky/bitcoin@b701085 is preferable.

I like the current -usewallet for the reasons that users can/will/must clearly distinct between running with multiple wallets (d/QT -wallet) and accessing a specific wallet (-usewallet).

I’m aware of the different layers (d/Qt vs bitcoin-cli) but from the users perspective, differentiation of an argument between the daemon and the cli is difficult to understand (once its running multiple wallets, once its selecting a specific wallet). Having two identical arguments that have total different effects is not ideal (even cross binaries).

I think for 0.15 we should merge the multiple -usewallet arguments blocker: #10867

Also, if we want to go with a default wallet (in future and we already do this for Qt [Index 0]), we could reuse the -usewallet argument for the daemon and/or-only Qt.

ryanofsky commented at 12:11 pm on July 19, 2017: member

#10867 doesn’t address usability problem I referred to above where user forgets “use” prefix in a bitcoin-cli call, their -wallet=filename is ignored by bitcoin-cli, resulting in server error or the RPC call ignoring specified filename and accessing wrong wallet if server configured with a different single wallet.

-usewallet option was ~~briefly necessary~~ added in moment of confusion while the wallet endpoint didn’t allow node calls, but now having two differently spelled options is just adding a footgun and source of complexity.

Anyway, I believe John is currently working on an improved version of ryanofsky/bitcoin@b701085 that actually shrinks it down further, removes global variable, and fixes bug where I forgot to remove “use” prefix (footgun!).

jonasschnelli commented at 12:53 pm on July 19, 2017: contributor

I can’t follow the potential source of confusion. Please help me to understand.

The current master approach is:

Daemon (server) uses -wallet to specify (additional) wallets to run with.
CLI (client) uses -usewallet to specify which of the available wallets (specified though daemons -wallet) should be used.
bitcoin.conf’s -wallet arguments do not affect bitcoin-cli.
bitcoin.conf’s -usewallet arguments does allow to define a default wallet (sounds okay to me if someone adds this intentionally, the term use makes it clear).
providing an -usewallet argument in CLI will “override” the default bitcoin.conf -usewallet directive (can be blocked by merging #10867).

This PR would lead to:

Daemon and client do both use -wallet (once for defining multiple wallets, once for defining a single wallet to use for executing a command, which are two completely different use-cases).
Disabling the option to have a default wallet by explicitly setting -usewallet in bitcoin.conf (may be okay)
Add code to detect and reject the -wallet arguments in bitcoin.conf and only accept them via command line (?)

IMO from the users perspective, the current master approach seems much more logical. Also, I don’t see the footgun source (both approaches have risks selecting the wrong wallet).

ryanofsky commented at 1:09 pm on July 19, 2017: member

A simple example of confusion (mentioned twice above) is user forgets to add “use” prefix to bitcoin-cli and the argument gets ignored.

Notice how you have 5 bullet points to describe current behavior and one bullet point to describe proposed behavior (the second two bullets are just contrasting with current behavior.)

Ideal scenario to me is our tools consistently accept -wallet=filename to denote which wallet to use. If tool supports accessing multiple wallets, it accepts multiple -wallet=filename options, if it doesn’t it shows an error when more than one is provided.

jonasschnelli commented at 1:29 pm on July 19, 2017: contributor

A simple example of confusion (mentioned twice above) is user forgets to add “use” prefix to bitcoin-cli and the argument gets ignored.

If he forgets the use prefix and uses -wallet instead, the call would not be executed in multiwallet environment because he had not specified a wallet.

I don’t think we should avoid setting a default wallet via -usewallet in bitcoin.conf, this seems to me after a desirable use-cases and, because it’s different to -wallet and has the term -use in it, I think its acceptable.

If we don’t want that, then we should ignore -usewallet in the conffile-reader logic.

Ideal scenario to me is our tools consistently accept -wallet=filename to denote which wallet to use.

But that would not be true for bitcoind/Qt because you don’t specify which wallet (single) to use (you do more specify which wallets you want to have accessible). Especially for Qt that’s not true (there always index 0 will be used).

I fail to see the advantages of having a single -wallet argument for both tools because IMO they have different implications. I think it is a source of confusion.

ryanofsky commented at 2:31 pm on July 19, 2017: member

If he forgets the use prefix and uses -wallet instead, the call would not be executed in multiwallet environment because he had not specified a wallet.

Yes, that is what I meant by “-wallet=filename is ignored by bitcoin-cli, resulting in server error or the RPC call ignoring specified filename and accessing wrong wallet if server configured with a different single wallet.” The worse case of the two is the case where server is single wallet and filename provided just gets silently ignored instead of checked on the server. Anyway, even if we disagree about how bad it is for bitcoin-cli to silently ignore “-wallet” options, I think we can agree that in the most common use-case (calling bitcoin-cli with a wallet filename on command line and no custom default config), having differently spelled -usewallet and -wallet arguments that get used by some commands and ignored by others does add some new opportunities for confusion and errors even if we disagree about how bad the confusion and errors are. And in any case (I will explain below) adding confusion to command line interface to support a default wallet feature is unnecessary.

I don’t think we should avoid setting a default wallet via -usewallet in bitcoin.conf, this seems to me after a desirable use-cases and, because it’s different to -wallet and has the term -use in it, I think its acceptable.

I agree that being able to set default wallet is useful. All I’m saying is that better way to do it is to call the default wallet option -defaultwallet instead of -usewallet and implement it server side instead of client side. Implementing it server side would have advantage of working with clients other than bitcoin-cli. It would avoid adding unnecessary complexity to command line in the common case where it isn’t used, and avoid the complicated interactions you described in your five bullet points when it is used. Finally, it could interact better with per-user default wallets (which are necessarily implemented server-side) which luke-jr at least has mentioned use cases for if we decide to add support for them in the future.

jnewbery commented at 2:42 pm on July 19, 2017: member

I think I’ve sown unnecessary confusion with this PR. All that it’s doing is:

If bitcoind is running multiwallet, bitcoin-cli must be called with exactly one -wallet command line argument to access wallet RPCs.

That’s it. With singlewallet mode, everything runs exactly as before. With non-wallet RPCs, everything runs exactly as before. If more than one -wallet argument is provided on the command line, the call fails.

Having two different arguments would make this more confusing, especially if one of them is called usewallet. How do users know that usewallet means bitcoin-cli uses this but not bitcoind and wallet means bitcoind uses this but not bitcoin-cli? How do they know that usewallet in .conf means default wallet and on the command line means override default wallet? How do they know that usewallet is the default wallet if you’re using bitcoin-cli but there is no default wallet if you’re using RPC over HTTP? They don’t, because the use is usewallet is ambiguous enough to be meaningless.

morcos commented at 3:37 pm on July 19, 2017: member

Concept ACK

ryanofsky commented at 4:19 pm on July 19, 2017: member

https://github.com/ryanofsky/bitcoin/commit/4ae1c6858e55f6f715918fe3c82403a8c8e69828 is another implementation of this PR with identical behavior, which might be a little easier to discuss.

Sadly, though, treating command line arguments exactly the same as config file arguments might be an inviolable principle in bitcoin that would make these approaches unacceptable (https://botbot.me/freenode/bitcoin-core-dev/msg/88779993/). So users might have to deal with differently spelled wallet options for different commands. Not ideal from my point of view (and of course predictably avoidable if we just treated wallet filenames like normal parameters #10829 (comment)), but ok enough.

jnewbery commented at 6:36 pm on July 19, 2017: member

As far as I can tell, this PR is functionally equivalent to the command-line -wallet replacing the .conf file wallet args, which is standard for all arguments. It has to be handled slightly carefully because of the way mapmultiargs work, and if we really wanted to be formal about “the command-line -wallet arguments replace the config-file one as a whole, rather than appending to”, then we’d need bitcoin-cli to have knowledge of which RPCs are wallet calls and which are server calls.

I think @ryanofsky’s branch is as good as this, although I haven’t manually tested it yet.

laanwj commented at 8:43 am on July 20, 2017: member

I prefer not to overload -wallet both for bitcoind, which has “what wallets to load”, and bitcoin-cli which has “what wallet to choose”. These meanings are different enough. Also one option is multi-value, the other single-value. This requires special casing to make work, which I don’t think is worth it. (if bitcoind and bitcoin-cli read different configuration files, this issue wouldn’t exist as there would be no overlap in namespaces - but alas)

The current approach of introducing a new argument name for bitcoin-cli to select the wallet (now -rpcwallet) has less potential of confusing, and doesn’t require special casing of argument handling, so is preferable to me.

ryanofsky commented at 1:47 pm on July 20, 2017: member

Should be closed? (#10883 is merged)

jnewbery commented at 2:11 pm on July 20, 2017: member

I still don’t understand why -rpcwallet is preferred. bitcoind is a node where -wallet means “this is the wallet I wish to use”. Since 0.15, multiple wallets are allowed. bitcoin-cli is a command line interface where wallet would mean “this is the wallet I wish to issue a command to”. Obviously only one wallet can be used in this context. So yes, perhaps this is a little bit overloaded, but only in the most intuitive and expected way.

As for special-casing, the only way wallet is treated functionally differently is that in bitcoin-cli a wallet on the command-line overrides wallet in .conf, but for bitcoind wallet on the command-line appends to the wallets in bitcoin.conf. That’s really the only way it could work, since appending doesn’t make sense for bitcoin-cli where only one wallet is allowed.

(and FWIW, command-line arguments appending to .conf arguments instead of replacing them is very counter-intuitive)

But yes, you’re right - the decision has been made. I’ll close this.

jnewbery closed this on Jul 20, 2017

DrahtBot locked this on Sep 8, 2021

Remove -usewallet #10868

The current master approach is:

This PR would lead to: