Simple, backwards compatible RPC multiwallet support.

[ryanofsky]

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

[promag]

Will review later.

[MeshCollider]

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

[sipa]

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

Named arguments work as expected:

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

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

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

[morcos]

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

utACK 21eeaa4

[morcos]

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.

[morcos]

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]

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]

Yes that makes sense to me.

[sipa]

Tested 21eeaa4 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]

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)

[ryanofsky]

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) 21eeaa4 -> 36ecc16 (pr/multiparam.2 -> pr/multiparam.3)

[jonasschnelli]

utACK 36ecc16.

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]

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

[laanwj]

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]

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]

notes:

1. 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)
2. 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 36ecc16

[instagibbs]

@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]

utACK 36ecc16

[ryanofsky]

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