Simple, backwards compatible RPC multiwallet support (superseded by #10829)

ryanofsky commented at 11:54 pm on June 22, 2017: member

This PR is superseded by #10829

(This got closed and couldn’t be reopened because the branch pointer changed.)

This change allows existing RPCs to work on multiple wallets by calling those RPCs with a wallet=filename named argument. Example usage:

bitcoind -regtest -wallet=w1.dat -wallet=w2.dat
bitcoin-cli -regtest -named getwalletinfo wallet=w1.dat
bitcoin-cli -regtest -named getwalletinfo wallet=w2.dat
bitcoin-cli -regtest -named getbalance wallet=w2.dat

Individual RPCs can override handling of the wallet named argument, but if they don’t, the GetWalletForJSONRPCRequest function will automatically chose the right wallet based on the argument value.

This change only allows JSON-RPC calls made with named arguments to access multiple wallets. JSON-RPC calls made with positional arguments will still continue to access the default wallet like before.

Simple, backwards compatible RPC multiwallet support.

This change allows existing RPCs to work on multiple wallets by calling those
RPCs with a wallet=filename named argument. Example usage:

    bitcoind -regtest -wallet=w1.dat -wallet=w2.dat
    bitcoin-cli -regtest -named getwalletinfo wallet=w1.dat
    bitcoin-cli -regtest -named getwalletinfo wallet=w2.dat
    bitcoin-cli -regtest -named getbalance wallet=w2.dat

Individual RPCs can override handling of the wallet named argument, but if they
don't, the `GetWalletForJSONRPCRequest` function will automatically chose the
right wallet based on the argument value.

This change only allows JSON-RPC calls made with named arguments to access
multiple wallets. JSON-RPC calls made with positional arguments will still
continue to access the default wallet as before.

01bd4bef23

fanquake added the label RPC/REST/ZMQ on Jun 23, 2017

fanquake added the label Wallet on Jun 23, 2017

jonasschnelli commented at 8:50 am on June 23, 2017: contributor

This is a simple and effective change. I could think this is viable for a first step.

The downsides of this are: -> Incompatible with non name based RPC calls -> Harder for later process separation (wallet switch is then in the JSON/Data layer)

in src/wallet/rpcwallet.cpp:35 in 01bd4bef23

30@@ -31,7 +31,14 @@
31 
32 CWallet *GetWalletForJSONRPCRequest(const JSONRPCRequest& request)
33 {
34-    // TODO: Some way to access secondary wallets
35+    if (!request.wallet.empty()) {
36+        for (const auto& wallet : ::vpwallets) {

jonasschnelli commented at 8:51 am on June 23, 2017:

I guess vpwallets needs concurrency protection.

promag commented at 11:12 am on July 4, 2017:

Why? vpwallets is initialized in InitLoadWallet and then never changes.

jnewbery commented at 11:13 am on July 4, 2017:

Because we may want to add dynamic loading/unloading/creation of wallets in future. In fact, I’m working on a branch that does just that.

promag commented at 11:21 am on July 4, 2017:

Sure! When that happens proper concurrency protection must be added no? IMO is work for other PR.

ryanofsky commented at 6:41 pm on July 5, 2017:

Yeah, I don’t think anyone is suggesting it’s needed in this PR.

laanwj commented at 11:26 am on June 24, 2017: member

I kind of like this solution. It’s simple and elegant. A pity no one brought this up at the IRC meeting two weeks ago when this was discussed. I hadn’t realized name-based arguments had made this so easy.

Endpoint-based has the slight advantage that the RPC client code only has to make one change to use a different wallet: use a different URL. This needs a change to pass the argument for every RPC call, and forgetting one may result in hard-to-troubleshoot, possibly even dangerous issues (as it is not enforced, so it uses the default wallet by accident).

gmaxwell commented at 7:21 pm on June 27, 2017: contributor

A pity no one brought this up at the IRC meeting two weeks ago when this was discussed

12:46 < luke-jr> BTW, did we consider a generic JSON-RPC “wallet” named param? 12:46 < sipa> luke-jr: i suggested that before, yes 12:46 < sipa> luke-jr: i’d be fine with it, except it’s less compatible with a future change that moves it to a different process (which will necessarily have a new endpoint) 12:46 < jonasschnelli> luke-jr: generic wallet named parameter seems fine. Is it mixable with the non-named parameter? 12:47 < sipa> so i think this guideline may help: arguments that select something that may in the future move to another process, should go into the path 12:47 < sipa> other things should be RPC named args

I’m fine with supporting this however, it would certainly be easier to use in some cases. We don’t have to support the entire future today– when the wallet is separated the API could change.

Or not.

I’m just commenting to point out that it did come up in the meeting! :)

jnewbery commented at 7:47 pm on June 27, 2017: member

I like this approach. It seems like the simplest and most straightforward way of exposing multiwallet functionality through the RPC interface. Longer term, if we do separate wallets out into separate processes, then I think multiple endpoints (#10650) makes sense. So my preference would be to do this now, with a warning that the API is unstable and will likely change in the future.

I haven’t reviewed the code for any of the PRs yet. The above comment is simply on the concept. I intend to to look at the code for all four proposals soon.

Note that the OP for this PR has a wrong link. #10653 is an alternate implementation … should be #10661 is an alternate implementation …

ryanofsky commented at 0:34 am on June 28, 2017: member

Thanks, updated the description. I do think #10661 is preferable to this PR. Although #10661 and this PR look almost the same externally (both accepting new wallet= named parameters for wallet methods), there are some differences:

Wallet parameters are documented in #10661, while in this PR they are completely undocumented.
Extra wallet= parameters on non-wallet methods trigger errors in #10661, while here they are silently ignored.
Passing wallet values as positional arguments is possible in #10661, while here they have to be passed as named arguments.

ryanofsky commented at 0:50 am on June 28, 2017: member

Just want to point out that both this PR and #10661 are compatible with #10615 and #10650. Accepting wallet JSON-RPC arguments doesn’t prevent us from restricting access to wallets based on endpoint or user authorization in the future. (In #10661, I’m keeping the unused request argument to GetWalletForJSONRPCRequest specifically to make sure this is easy to do later on.)

This needs a change to pass the argument for every RPC call, and forgetting one may result in hard-to-troubleshoot, possibly even dangerous issues (as it is not enforced, so it uses the default wallet by accident).

This is a really good point that I didn’t think of. I think in general this could be a dangerous side effect of supporting a “default wallet” for RPC calls in any of the 4 PRs. A user could accidentally forget to specify the wallet in a single API call, and wind up affecting a different wallet than intended. Maybe it would be good to kill the notion of a “default wallet” for RPCs entirely by requiring the wallet to be explicitly specified whenever more than one is loaded. This could be done by returning null from GetWalletForJSONRPCRequest whenever vpwallets.size() > 1 when no wallet is specified in any of the PRs.

sipa commented at 0:54 am on June 28, 2017: member

Extra wallet= parameters on non-wallet methods trigger errors in #10661, while here they are silently ignored.

This seems to be the largest downside of this approach to me, but it seems fixable by adding a flag to the RPC dispatch tables to mark wallet RPC specially?

ryanofsky commented at 1:02 am on June 28, 2017: member

This seems to be the largest downside of this approach to me, but it seems fixable by adding a flag to the RPC dispatch tables to mark wallet RPC specially?

Sure, but adding more special treatment of wallet parameters to this PR would just make me prefer #10661 even more. I don’t like the idea of wallet parameters having magical attributes that make them different from other parameters when #10661 shows they can work just fine as normal arguments.

sipa commented at 1:07 am on June 28, 2017: member

@ryanofsky I don’t think that’s so bad if you see it as a preparation for moving the wallet to a separate process entirely.

ryanofsky commented at 1:26 am on June 28, 2017: member

@ryanofsky I don’t think that’s so bad if you see it as a preparation for moving the wallet to a separate process entirely.

Agree that magic arguments are less bad if they are magic and disappearing (i.e. temporary). But I don’t see any advantage in having them at all when you can have normal arguments that don’t require special treatment. Am I missing something?

sipa commented at 1:37 am on June 28, 2017: member

@ryanofsky I think it’s very ugly that wallet RPCs need to even be aware of the fact that there is such a thing as multiple wallets. In a hypothetical future where every wallet runs as its own process, there is no need for something like that. It seems much cleaner to add some plumbing to make sure RPCs don’t need to be written with that in mind.

jnewbery commented at 2:12 pm on June 28, 2017: member

I’ve read the code for all the proposals now, and my preference is for #10650, implemented as a query option, since it seems like that’s what we’d want for wallet separation (I think we’d also want to be able to access the wallet RPC calls directly on the wallet process). If that’s going to take a long time to review/argue over/merge, then I don’t see any harm in merging this PR sooner with a warning that the API may change in future.

I agree with sipa that adding a wallet argument to all of the RPCs is ugly and should be removed if multiwallet is implemented on separate processes, so I prefer this PR over #10661, despite the drawbacks mentioned above.

Obviously this should be accompanied by tests if we decide to merge it. If there are concept ACKs, I’d be happy to help with those. The multiwallet.py test in #10604 may be a good starting point.

in src/rpc/server.cpp:460 in 01bd4bef23

455@@ -456,6 +456,11 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c
456             hole += 1;
457         }
458     }
459+    auto wallet = argsIn.find("wallet");
460+    if (wallet != argsIn.end() && wallet->second->isStr()) {

promag commented at 11:13 am on July 4, 2017:

I would say wallet argument must be a string otherwise RPC_INVALID_PARAMETER should be raised.

ryanofsky commented at 6:10 pm on July 5, 2017:

RPC_INVALID_PARAMETER will be raised when wallet isn’t a string because !argsIn.empty() will be true below.

in src/wallet/rpcwallet.cpp:40 in 01bd4bef23

36+        for (const auto& wallet : ::vpwallets) {
37+            if (request.wallet == wallet->GetName()) {
38+                return wallet;
39+            }
40+        }
41+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Requested wallet does not exist or is not loaded");

promag commented at 11:23 am on July 4, 2017:

Mixed feelings here, could we just return nullptr because if no wallet is loaded then it is the same outcome: wallet not found?

ryanofsky commented at 6:49 pm on July 5, 2017:

Reason for throwing here instead of returning nullptr is to make it an error to pass an invalid wallet filename to RPC calls like validateaddress, createmultisig, or getinfo where having a wallet is optional.

in src/rpc/server.h:62 in 01bd4bef23

57+    /**
58+     * Optional wallet name, set for backwards compatibility if the RPC method
59+     * was called with a named "wallet" parameter and the RPC method
60+     * implementation doesn't handle it itself.
61+     */
62+    std::string wallet;

luke-jr commented at 6:46 pm on July 4, 2017:

Wallets should be passed as a CWallet reference, not as a string…

ryanofsky commented at 6:39 pm on July 5, 2017:

Wallets should be passed as a CWallet reference, not as a string…

Advantage of doing that is that it could simplify test code wanting to emulate RPC calls by avoiding the need for it to have to manipulate vpwallets. Disadvantage is it would require sticking an #ifdef ENABLE_WALLET in the PR and moving wallet-selection logic from the wallet layer (GetWalletForJSONRPCRequest) to the rpc layer (transformNamedArguments).

luke-jr commented at 11:30 pm on July 5, 2017:

Another advantage is to properly call RPCs from the GUI without going wallet->string->wallet (which would be just begging for bugs).

ryanofsky commented at 7:26 pm on July 6, 2017: member

Closing in favor of 10650 (https://botbot.me/freenode/bitcoin-core-dev/msg/88240462/), even though I do think 10650 is a worse solution: #10650 (comment)

There is a newer version of the code in this PR which adds a test & makes it an error not to specify a wallet when more than one is loaded: https://github.com/bitcoin/bitcoin/compare/master...ryanofsky:pr/multiparam

ryanofsky closed this on Jul 6, 2017

ryanofsky commented at 6:09 pm on July 13, 2017: member

Longer term, if we do separate wallets out into separate processes, then I think multiple endpoints (#10650) makes sense.

FWIW, I was implementing RPC dispatch to wallet processes yesterday and found the opposite. With #10653 and #10661, rpc/server.cpp can take the wallet name directly from the RPC params and dispatch the request to the right wallet process, while with #10653, because the wallet URL demangling happens in GetWalletForJSONRPCRequest which is called by individual wallet methods, the RPC server has no way of knowing which wallet process to forward incoming requests to, so the URL demangling logic either has to be moved, or the server has to forward RPC requests to multiple wallet processes.

I don’t think these issues are very significant, but I’m just pointing this out because I think discussion around wallet process separation and multiwallet RPC support has been muddled and confused. I think pretty much any multiwallet RPC selection mechanism could be made to work cleanly with different multiprocess setups. I also think that the best multiprocess setup would just let wallet processes handle RPC calls on their own, which would eliminate the need for any multiwallet RPC selection mechanism.

sipa commented at 6:32 pm on July 13, 2017: member

@ryanofsky The reason why endpoints are preferable as a preparation for multiprocess has nothing to do with the server side, but with the client side. Having clients treat your wallet as a separate endpoint means they’re well prepared for a future where it’s a separate process - just change the endpoint.

ryanofsky commented at 6:55 pm on July 13, 2017: member

@ryanofsky The reason why endpoints are preferable as a preparation for multiprocess has nothing to do with the server side, but with the client side. Having clients treat your wallet as a separate endpoint means they’re well prepared for a future where it’s a separate process - just change the endpoint.

Thanks, that is helpful. I was thinking all along that endpoints are an unnecessary and mildly obnoxious change to want to roll out right now, but I guess you are saying the obnoxiousness is more of a feature than a bug because it will prepare users for more obnoxiousness in the future. Or maybe that’s not a completely fair description, but I think I get the idea a little better now.

morcos commented at 7:56 pm on July 14, 2017: member

Do we not want the behavior here where defaulting to vpwallets[0] only works if vpwallets.size() == 1?

ryanofsky commented at 7:57 pm on July 14, 2017: member

Do we not want the behavior here where defaulting to vpwallets[0] only works if vpwallets.size() == 1?

That behavior is implemented in the branch ( https://github.com/bitcoin/bitcoin/compare/master...ryanofsky:pr/multiparam), it’s just not showing up in the PR because the PR is closed

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

Reopened as #10829.

ryanofsky renamed this:
~~Simple, backwards compatible RPC multiwallet support~~
Simple, backwards compatible RPC multiwallet support (superseded by #10829)
on Jul 14, 2017

DrahtBot locked this on Sep 8, 2021

Simple, backwards compatible RPC multiwallet support (superseded by #10829) #10653