Simple, backwards compatible RPC multiwallet support.

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

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.

The wallet= parameter is mandatory if multiple wallets are loaded, and this change only allows JSON-RPC calls made with named arguments to access multiple wallets, so wallet RPC calls made positional arguments will not work if more than one wallet is loaded.

Multiwallet python test based on code originally written by @jonasschnelli

This PR is a simpler alternative to #10615, #10650, #10661, and #10849 that allows multiwallet RPC access from bitcoin-cli, python and any other JSON-RPC client that supports named parameters. It is compatible with these other changes and doesn’t prevent adding support for new request-uri endpoints and authorization parameters in the future, but it doesn’t require these things right now.

This PR is a newer version of #10653 which was closed (and couldn’t be reopened because the branch pointer changed).

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.

The wallet= parameter is mandatory if multiple wallets are loaded, and this
change only allows JSON-RPC calls made with named arguments to access multiple
wallets, so wallet RPC calls made positional arguments will not work if more
than one wallet is loaded.

Multiwallet python test based on code originally written by
Jonas Schnelli <dev@jonasschnelli.ch>

36ecc16ba7

promag commented at 8:09 pm on July 14, 2017: member

Will review later.

meshcollider commented at 8:13 pm on July 14, 2017: contributor

Code changes look good, concept ACK, but haven’t read the other alternative PRs so can’t say which implementation I prefer yet

fanquake added the label RPC/REST/ZMQ on Jul 15, 2017

fanquake added the label Wallet on Jul 15, 2017

sipa commented at 0:33 am on July 15, 2017: member

0$ ./src/bitcoind -daemon -wallet=w1.dat -wallet=w2.dat

Named arguments work as expected:

0$ ./src/bitcoin-cli -named getbalance wallet=w1.dat
10.00000000

The PR description says that the default wallet is used with positional arguments, however:

0$ ./src/bitcoin-cli getbalance
1error code: -32601
2error message:
3Method not found (disabled)

in src/rpc/server.cpp:217 in 21eeaa43d4 outdated

212@@ -213,6 +213,12 @@ std::string CRPCTable::help(const std::string& strCommand, const JSONRPCRequest&
213                     std::string firstLetter = category.substr(0,1);
214                     boost::to_upper(firstLetter);
215                     strRet += "== " + firstLetter + category.substr(1) + " ==\n";
216+                    if (category == "wallet") {
217+                        strRet += "\nWhen more than one wallet is loaded (multiple -`wallet=filename` options passed\n"

morcos commented at 3:11 pm on July 15, 2017:

I think it might be nice to also add this string to each of the individual wallet helps? Or if there is some way to return it when it is the cause of the error?

At least for me I rarely use the overall help or I grep it for the type of thing I’m looking for since it is so much output. And I worry that users will waste a lot of time not realizing what the problem is.

in src/rpc/server.cpp:481 in 21eeaa43d4 outdated

477@@ -472,6 +478,11 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c
478             hole += 1;
479         }
480     }
481+    auto wallet = argsIn.find("wallet");

morcos commented at 3:39 pm on July 15, 2017:

Named arguments allow you to specify an argument more than once and then the last specified value controls.
I’m not sure if this is desirable behavior before this PR, but certainly after this PR it could lead to unexpected actions if for some reason multiple wallet arguments are specified.

laanwj commented at 6:55 am on July 17, 2017:

Named arguments allow you to specify an argument more than once and then the last specified value controls.

We could make it an error to specify a key twice, that seems acceptable in the context of JSON parsing because the standard doesn’t say anything about duplicate keys (so it can depend on the context). But let’s not add that to the scope of this PR.

(this would need to be enforced for any named argument, not just wallet=)

morcos commented at 10:14 am on July 17, 2017:

Yes that makes sense to me.

morcos approved

morcos commented at 3:41 pm on July 15, 2017: member

This works. I left some feedback that I think is worth thinking about, but not obviously worth changing.

utACK 21eeaa4

sipa commented at 6:19 pm on July 15, 2017: member

Tested 21eeaa43d415d4397663eaee06d40decab0073f1 a bit. Named arguments work with multiwallet and single wallet, and positional arguments work with single wallet. I’m not sure if the observed behavior for multiwallet with positional arguments in intended.

laanwj commented at 7:03 am on July 17, 2017: member

This is much simpler in implementation than #10650.

But for the record I’ve argued against named-argument based multiwallet before: #10653 (comment) . Forgetting to provide the named argument anywhere in the end-user code will cause the operation to happen on the default wallet. This can result in dangerous, or at least hard to debug, mistakes.

(this concern could be avoided by having wallet= argument mandatory if multiple wallets are loaded)

laanwj added this to the milestone 0.15.0 on Jul 17, 2017

ryanofsky force-pushed on Jul 17, 2017

ryanofsky commented at 9:47 am on July 17, 2017: member

this concern could be avoided by having wallet= argument mandatory if multiple wallets are loaded

This is actually what the PR does, the commit message was just out of date.

The PR description says that the default wallet is used with positional arguments, however […] I’m not sure if the observed behavior for multiwallet with positional arguments in intended.

Sorry the commit message was just out of date.

Amended commit message (no code changes) 21eeaa43d415d4397663eaee06d40decab0073f1 -> 36ecc16ba7df491690a4723540ccaabbe217b1b6 (pr/multiparam.2 -> pr/multiparam.3)

jonasschnelli commented at 10:17 am on July 17, 2017: contributor

utACK 36ecc16ba7df491690a4723540ccaabbe217b1b6.

It is a clean and simple change. I’m still in favour of the more extensible approach of endpoint which would allow to run multiple wallet implementations with the same API while not fiddling with the JSON layer. Providing the wallet selector within the JSON data layer seems to me as sort of a data layer violation.

But the change is simple and works.

jonasschnelli commented at 10:18 am on July 17, 2017: contributor

Nevertheless I added a simple endpoint based PR (#10849) which is +103 −7 versus this +80 −2.

laanwj commented at 10:53 am on July 17, 2017: member

Providing the wallet selector within the JSON data layer seems to me as sort of a data layer violation.

I agree, ideally words such as “wallet” should not appear in rpc/server.cpp, and that should definitely be changed later. But it’s ok for 0.15 I guess, this is easiest to review last minute.

ryanofsky commented at 11:21 am on July 17, 2017: member

I’m still in favour of the more extensible approach of endpoint which would allow to run multiple wallet implementations with the same API while not fiddling with the JSON layer. Providing the wallet selector within the JSON data layer seems to me as sort of a data layer violation.

This will allow multiwallet to work with no code in json-rpc layer with an additional change that moves named -> positional argument conversion out of the json-rpc layer into rpc method callbacks (or a backwards-compatible callback wrapper to avoid code changes in existing callbacks).

This is a change that would sense anyway to simplify the RPC layer and make it possible to write new, more readable RPC methods that can access parameters by name instead of number. As you can tell from the comment I added to the JSONRPCRequest::params member, current behavior is more complicated and less efficient than I think it should be.

instagibbs commented at 5:04 pm on July 17, 2017: member

notes:

giving two+ named arguments wallet results in the last one being used for the call (I think named arguments really shouldn’t be able to accept duplicates regardless, might be out of scope)
we’d probably want to give priority to named arg fixes like #10783 if this is merged, since non-working named arguments means you can’t use multiwallet for that type of call. Would be very confusing to user.

tACK https://github.com/bitcoin/bitcoin/pull/10829/commits/36ecc16ba7df491690a4723540ccaabbe217b1b6

instagibbs commented at 5:57 pm on July 17, 2017: member

@morcos points out (2) is not right, user will simply have to enter in all values rather than a subset. So fix is less urgent than I was thinking

achow101 commented at 6:54 pm on July 17, 2017: member

utACK 36ecc16ba7df491690a4723540ccaabbe217b1b6

ryanofsky commented at 2:39 am on July 18, 2017: member

Closing in favor of #10849, which despite:

causing the wallet option treated differently than every other existing RPC option
preventing us from designing a stable and extensible uri-path scheme (it puts wallet at root of a new undocumented, unstable uri-path scheme with /v1 segment added then removed again, /node segment added and removed again, /wallet -> node passthrough added and removed then added again, just in past few days)
requiring changes to bitcoin-cli and python client code that would otherwise be unnecessary

Does have the advantage of not requiring named parameters, or requiring that a wallet parameter be passed in individual RPC calls (most JSON-RPC tools should allow request URI to be easily specified, and some JSON-RPC tools might make binding arguments more difficult than changing the URI, though neither of these things happen to be true for bitcoin-cli or the python test client).

#10849 does also fix the worst problems of #10650 (unnecessary code complexity & spurious errors due to lack of wallet -> node passthrough).

ryanofsky closed this on Jul 18, 2017

laanwj referenced this in commit bde4f937ae on Jul 18, 2017

PastaPastaPasta referenced this in commit fdf34ff655 on Aug 2, 2019

PastaPastaPasta referenced this in commit c796551a70 on Aug 6, 2019

barrystyle referenced this in commit f2f1b9ff58 on Jan 22, 2020

MarcoFalke locked this on Sep 8, 2021

Simple, backwards compatible RPC multiwallet support. #10829