Simple, backwards compatible RPC multiwallet support. #10829

ryanofsky · 2017-07-14T19:59:04Z

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

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>

promag · 2017-07-14T20:09:38Z

Will review later.

meshcollider · 2017-07-14T20:13:00Z

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

sipa · 2017-07-15T00:33:50Z

$ ./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 · 2017-07-15T15:11:41Z

src/rpc/server.cpp

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


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 · 2017-07-15T15:39:32Z

src/rpc/server.cpp

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


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.

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

Yes that makes sense to me.

sipa · 2017-07-15T18:19:46Z

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 · 2017-07-17T07:03:54Z

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 · 2017-07-17T09:47:15Z

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 · 2017-07-17T10:17:57Z

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 · 2017-07-17T10:18:55Z

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

laanwj · 2017-07-17T10:53:51Z

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 · 2017-07-17T11:21:05Z

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 · 2017-07-17T17:04:32Z

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 [RPC] Various rpc argument fixes #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 · 2017-07-17T17:57:03Z

@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 · 2017-07-17T18:54:49Z

utACK 36ecc16

ryanofsky · 2017-07-18T02:39:24Z

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

6b9faf7 [QA] add basic multiwallet test (Jonas Schnelli) 979d0b8 [tests] [wallet] Add wallet endpoint support to authproxy (John Newbery) 76603b1 Select wallet based on the given endpoint (Jonas Schnelli) 32c9710 Fix test_bitcoin circular dependency issue (Jonas Schnelli) 31e0720 Add wallet endpoint support to bitcoin-cli (-usewallet) (Jonas Schnelli) dd2185c Register wallet endpoint (Jonas Schnelli) Pull request description: Alternative for #10829 and #10650. It adds the most simplest form of wallet based endpoint support (`/wallet/<filename>`). No v1 and no node/wallet endpoint split. Tree-SHA512: 23de1fd2f9b48d94682928b582fb6909e16ca507c2ee19e1f989d5a4f3aa706194c4b1fe8854d1d79ba531b7092434239776cae1ae715ff536e829424f59f9be

6b9faf7 [QA] add basic multiwallet test (Jonas Schnelli) 979d0b8 [tests] [wallet] Add wallet endpoint support to authproxy (John Newbery) 76603b1 Select wallet based on the given endpoint (Jonas Schnelli) 32c9710 Fix test_bitcoin circular dependency issue (Jonas Schnelli) 31e0720 Add wallet endpoint support to bitcoin-cli (-usewallet) (Jonas Schnelli) dd2185c Register wallet endpoint (Jonas Schnelli) Pull request description: Alternative for bitcoin#10829 and bitcoin#10650. It adds the most simplest form of wallet based endpoint support (`/wallet/<filename>`). No v1 and no node/wallet endpoint split. Tree-SHA512: 23de1fd2f9b48d94682928b582fb6909e16ca507c2ee19e1f989d5a4f3aa706194c4b1fe8854d1d79ba531b7092434239776cae1ae715ff536e829424f59f9be

This was referenced Jul 14, 2017

Multiwallet: add RPC endpoint support #10650

Closed

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

Closed

fanquake added RPC/REST/ZMQ Wallet labels Jul 15, 2017

morcos approved these changes Jul 15, 2017

View reviewed changes

laanwj added this to the 0.15.0 milestone Jul 17, 2017

ryanofsky force-pushed the pr/multiparam branch from 21eeaa4 to 36ecc16 Compare July 17, 2017 09:44

jonasschnelli mentioned this pull request Jul 17, 2017

Multiwallet: simplest endpoint support #10849

Merged

ryanofsky closed this Jul 18, 2017

ryanofsky mentioned this pull request Jul 19, 2017

Remove -usewallet #10868

Closed

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

Simple, backwards compatible RPC multiwallet support. #10829

Simple, backwards compatible RPC multiwallet support. #10829

Uh oh!

Conversation

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

Uh oh!

Uh oh!

promag commented Jul 14, 2017

Uh oh!

meshcollider commented Jul 14, 2017

Uh oh!

sipa commented Jul 15, 2017

Uh oh!

morcos left a comment

Choose a reason for hiding this comment

Uh oh!

morcos Jul 15, 2017

Choose a reason for hiding this comment

Uh oh!

morcos Jul 15, 2017

Choose a reason for hiding this comment

Uh oh!

laanwj Jul 17, 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!

morcos Jul 17, 2017

Choose a reason for hiding this comment

Uh oh!

sipa commented Jul 15, 2017

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

jonasschnelli commented Jul 17, 2017

Uh oh!

jonasschnelli commented Jul 17, 2017

Uh oh!

laanwj commented Jul 17, 2017

Uh oh!

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

Uh oh!

Uh oh!

instagibbs commented Jul 17, 2017

Uh oh!

instagibbs commented Jul 17, 2017

Uh oh!

achow101 commented Jul 17, 2017

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

ryanofsky commented Jul 14, 2017 •

edited

Loading

laanwj Jul 17, 2017 •

edited

Loading

laanwj commented Jul 17, 2017 •

edited

Loading

ryanofsky commented Jul 17, 2017 •

edited

Loading

ryanofsky commented Jul 17, 2017 •

edited

Loading

ryanofsky commented Jul 18, 2017 •

edited

Loading