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

ryanofsky · 2017-06-22T23:54:39Z

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.

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.

jonasschnelli · 2017-06-23T08:50:55Z

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)

jonasschnelli · 2017-06-23T08:51:33Z

src/wallet/rpcwallet.cpp

@@ -31,7 +31,14 @@

 CWallet *GetWalletForJSONRPCRequest(const JSONRPCRequest& request)
 {
-    // TODO: Some way to access secondary wallets
+    if (!request.wallet.empty()) {
+        for (const auto& wallet : ::vpwallets) {


I guess vpwallets needs concurrency protection.

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

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.

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

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

laanwj · 2017-06-24T11:26:58Z

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 · 2017-06-27T19:21:28Z

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 · 2017-06-27T19:47:54Z

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 · 2017-06-28T00:34:06Z

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 Add optional wallet=filename arguments to wallet RPCs #10661, while in this PR they are completely undocumented.
Extra wallet= parameters on non-wallet methods trigger errors in Add optional wallet=filename arguments to wallet RPCs #10661, while here they are silently ignored.
Passing wallet values as positional arguments is possible in Add optional wallet=filename arguments to wallet RPCs #10661, while here they have to be passed as named arguments.

ryanofsky · 2017-06-28T00:50:14Z

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 · 2017-06-28T00:54:03Z

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 · 2017-06-28T01:02:42Z

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 · 2017-06-28T01:07:00Z

@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 · 2017-06-28T01:26:52Z

@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 · 2017-06-28T01:37:12Z

@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 · 2017-06-28T14:12:54Z

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.

promag · 2017-07-04T11:13:53Z

src/rpc/server.cpp

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


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

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

promag · 2017-07-04T11:21:54Z

src/wallet/rpcwallet.cpp

@@ -31,7 +31,14 @@

 CWallet *GetWalletForJSONRPCRequest(const JSONRPCRequest& request)
 {
-    // TODO: Some way to access secondary wallets
+    if (!request.wallet.empty()) {
+        for (const auto& wallet : ::vpwallets) {


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

promag · 2017-07-04T11:23:48Z

src/wallet/rpcwallet.cpp

+                return wallet;
+            }
+        }
+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Requested wallet does not exist or is not loaded");


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

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.

luke-jr · 2017-07-04T18:46:06Z

src/rpc/server.h

+     * was called with a named "wallet" parameter and the RPC method
+     * implementation doesn't handle it itself.
+     */
+    std::string wallet;


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

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).

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

ryanofsky · 2017-07-06T19:26:04Z

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: master...ryanofsky:pr/multiparam

ryanofsky · 2017-07-13T18:09:28Z

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 · 2017-07-13T18:32:43Z

@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 · 2017-07-13T18:55:58Z

@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 · 2017-07-14T19:56:23Z

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

ryanofsky · 2017-07-14T19:57:52Z

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 ( master...ryanofsky:pr/multiparam), it's just not showing up in the PR because the PR is closed

ryanofsky · 2017-07-14T20:06:56Z

Reopened as #10829.

ryanofsky mentioned this pull request Jun 22, 2017

Multiwallet: add RPC endpoint support #10650

Closed

fanquake added RPC/REST/ZMQ Wallet labels Jun 23, 2017

jonasschnelli reviewed Jun 23, 2017

View reviewed changes

ryanofsky mentioned this pull request Jun 23, 2017

Add optional wallet=filename arguments to wallet RPCs #10661

Closed

promag reviewed Jul 4, 2017

View reviewed changes

luke-jr reviewed Jul 4, 2017

View reviewed changes

ryanofsky closed this Jul 6, 2017

ryanofsky mentioned this pull request Jul 14, 2017

Simple, backwards compatible RPC multiwallet support. #10829

Closed

ryanofsky changed the title ~~Simple, backwards compatible RPC multiwallet support~~ Simple, backwards compatible RPC multiwallet support (superseded by #10829) Jul 14, 2017

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Uh oh!

Conversation

ryanofsky commented Jun 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Jun 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jun 24, 2017

Uh oh!

gmaxwell commented Jun 27, 2017

Uh oh!

jnewbery commented Jun 27, 2017

Uh oh!

ryanofsky commented Jun 28, 2017

Uh oh!

ryanofsky commented Jun 28, 2017

Uh oh!

sipa commented Jun 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Jun 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Jun 28, 2017

Uh oh!

ryanofsky commented Jun 28, 2017

Uh oh!

sipa commented Jun 28, 2017

Uh oh!

jnewbery commented Jun 28, 2017

Uh oh!

promag Jul 4, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Jul 13, 2017

Uh oh!

sipa commented Jul 13, 2017

Uh oh!

ryanofsky commented Jul 13, 2017

Uh oh!

morcos commented Jul 14, 2017

Uh oh!

ryanofsky commented Jul 14, 2017

Uh oh!

ryanofsky commented Jul 14, 2017

Uh oh!

Uh oh!

ryanofsky commented Jun 22, 2017 •

edited

Loading

sipa commented Jun 28, 2017 •

edited

Loading

ryanofsky commented Jun 28, 2017 •

edited

Loading

promag Jul 4, 2017 •

edited

Loading

ryanofsky commented Jul 6, 2017 •

edited

Loading