Multiwallet: add RPC endpoint support #10650

jonasschnelli · 2017-06-22T07:37:49Z

This adds a /wallet/* endpoint to the RPC server (optional).

The wallet RPC functions will try to find the selected wallet by comparing the given endpoint with all available wallets/walletIDs.

The walletID is currently defined by the filename (-wallet=<filename>), ideally, we later add a wallet RPC call to set the walletID (should be written to the wallet database).

This also includes endpoint support for bitcoin-cli.
There is a new argument -wallet=<walletID>. If set, the given walletID will be used to call the endpoint with a scheme of /wallet/< walletID >.

QA's authproxy is also extended to allow calls like self.nodes[0].<walletID>.<method>.

There is finally also a basic multiwallet RPC test.

Contains #10649.

luke-jr · 2017-06-22T07:56:47Z

This would make much more sense as ?wallet=WALLETID than trying to emulate a path...

jonasschnelli · 2017-06-22T09:30:42Z

This would make much more sense as ?wallet=WALLETID than trying to emulate a path...

No. Please no query string.
That doesn't make sense to me.

POST data to /wallet/<name> seems more reasonable to me then POST data to /?wallet=<name>

ryanofsky

utACK 6c4b1ba5d05295b68f91fda74fc1841d76ce01fc with whatever link fixes are needed for travis.

This is a nice, clean change, and I personally think it is more user friendly than requiring an -rpcauth setup in order to use multiwallet like #10615 (although auth wallet selection and endpoint wallet selection are basically compatible each other and both PRs could be merged).

I would agree with luke and prefer /?wallet=<name> to /wallet/<name> because former is more extensible and would allow more query options to be added in the future (like custom timeouts), while leaving the top level url path free for a more traditional use like API versioning.

ryanofsky · 2017-06-22T10:38:00Z

src/bitcoin-cli.cpp

+    // check if we should use a special wallet endpoint
+    std::string endpoint = "/";
+    if (GetArg("-wallet", "") != "") {
+        endpoint = "/wallet/"+GetArg("-wallet", "");


In commit "Add -wallet endpoint support to bitcoin-cli":

Probably should url encode in case filename contains spaces, reserved characters, or unicode (utf8) https://en.wikipedia.org/wiki/Percent-encoding#Percent-encoding_reserved_characters

Corresponding decode would be needed in GetWalletForJSONRPCRequest.

ryanofsky · 2017-06-22T11:25:51Z

test/functional/test_framework/authproxy.py

+        method = self._service_name
+        elements = self._service_name.split(".")
+        if len(elements) > 1:
+            urlAdd = "/wallet/" + '.'.join(elements[:-1])


In commit "[QA] Add support for wallet endpoints in Authproxy"

It seems hacky to me for AuthServiceProxy to be aware of multiwallet and to be munging urls and service name strings this way.

I think it would be better if AuthServiceProxy didn't know anything about bitcoin urls and just let the caller control the request. There are many ways to allow this, but a simple one might be to define:

def __idiv__(self, relative_uri): return AuthServiceProxy("{}/{}".format(self.__service_url, relative_uri), self._service_name, connection=self.__conn)

Then you could call methods on individual wallets with:

w1 = self.nodes[0] / "wallet/w1.dat" w1.getwalletinfo() w1.getbalance()

I think this actually should use __truediv__ not __idiv__,
which is for python 2.x (https://docs.python.org/3/reference/datamodel.html?highlight=truediv#object.__truediv__)

I agree - we should try to make minimal changes to authproxy.

Later, once TestNode is merged, it'll be very straightforward to add wallet methods to the TestNode class, which is where I think they should live - not in the authproxy layer.

ryanofsky · 2017-06-22T11:54:51Z

src/bitcoin-cli.cpp

-    int r = evhttp_make_request(evcon.get(), req.get(), EVHTTP_REQ_POST, "/");
+    // check if we should use a special wallet endpoint
+    std::string endpoint = "/";
+    if (GetArg("-wallet", "") != "") {


In commit "Add -wallet endpoint support to bitcoin-cli":

You should update HelpMessageCli to mention the new argument.

Since I started making a similar change (but didn't get very far) you could steal my help string:

strUsage += HelpMessageOpt("-wallet=<file>", _("Send RPC for non-default wallet on RPC server (argument is wallet filename in bitcoind directory)"));

In commit "Add -wallet endpoint support to bitcoin-cli"

Previous comment not addressed (#10650 (comment)).

laanwj · 2017-06-22T13:36:28Z

Concept ACK, will review.

POST data to /wallet/ seems more reasonable to me then POST data to /?wallet=

I tend to agree:

The use of query strings is usually avoided in modern APIs because they make URLs less readable, compared to path segments.
Also for POST the query data is in the payload. It is very uncommon to have query arguments in the URL.

ryanofsky · 2017-06-22T15:06:04Z

I guess personally I don't see how /?wallet=<name> is "less readable" then /wallet/<name> and I think I mentioned some practical reasons above for wanting to prefer query strings, but I don't have a very strong preference.

If we want to go down the road of encoding request metadata in path segments instead of query parameters maybe we should put more thought into what the path hierarchy should be? It isn't obvious to me that you'd want to put wallet selection at the very top of the path hierarchy instead of something else like an API version version number.

ryanofsky · 2017-06-22T15:31:04Z

src/wallet/rpcwallet.cpp

+            }
+        }
+        // no wallet found with the given endpoint
+        throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet not found ("+SanitizeString(requestedWallet)+")");


In commit "Select wallet based on the given endpoint"

Maybe change error message to something like "Wallet file does not exist or is not loaded" to avoid implying that the wallet doesn't exist when it might just not be loaded.

Also, since this is a URI, sanitize call should be changed to allow % character and probably all reserved and unreserved characters from RFC3986 ("%-_.~!*'();:@&=+$,/?#[]").

In commit "Select wallet based on the given endpoint"

Previous comment not addressed (#10650 (comment))

Thread #10650 (comment)

Error string cleanup not (yet) addressed.

gmaxwell · 2017-06-22T18:42:02Z

My initial impression was "I thought we were going to put an RPC api version in the path" too. FWIW. I don't know what the norms are in this kind of rpc thing. Are there parallels in hosted wallet service APIs in the Bitcoin space already?

jonasschnelli · 2017-06-22T19:39:53Z

Yes. Versioning makes probably sense at this point.
The often seen design advice is /api/v1/wallet?id=walletid, though not sure if we want the /api/.

I could think of /v1/wallet/walletID.

The query string makes sense if we expect multiple non-hierarchical inputs.

If we start to use a URI query-string == non-hierarchical inputs, where would be the main difference between the query-string key/value level and the JSON key/value level?
What are the arguments for a query-string rather then put the wallet id into the JSON RPC request?

I don't think there are huge advantages/disadvantages between query-string vs path. It's probably a design choice, matter of taste.

ryanofsky · 2017-06-22T20:37:04Z

If we start to use a URI query-string == non-hierarchical inputs, where would be the main difference between the query-string key/value level and the JSON key/value level?

I think for simplicity we should almost always prefer using JSON-RPC parameters for passing request options. If I were designing a bitcoin JSON-RPC API from scratch, I'd require a fixed, basic, future-proof request-uri like /api/v1 and require request options to be specified in JSON-RPC parameters, not in out-of-band url path extensions, or query strings, or http headers.

But in this case we're talking about updating an existing JSON-RPC API with dozens of commands that already have their own ways of interpreting JSON-RPC parameters and that all assume they'll be interacting with a single wallet. So for backwards compatability, I think it's a good idea to allow an out-of-band wallet=filename option that can be tacked on to the request-uri query string and onto the bitcoin-cli command line, and that will control which wallet the existing API methods will access by default, without having to update dozens of individual method implementations, or change API in an incompatible way.

I think if you want to come up with complicated new URL schemes or authorization schemes to provide more structured and controlled access to the API, that could be great, but it goes beyond what you need for basic backward-compatible multiwallet support, which is a simple wallet=filename option that you can tack on to requests and command lines.

ryanofsky · 2017-06-22T23:55:42Z

Another multiwallet RPC implemention for discussion: #10653

jonasschnelli · 2017-06-23T08:35:54Z

Added the /v1/ path element.

@ryanofsky pointed out that we should urlencode/decode the walletID in the path element.
Not having to do this is one advantage of putting the walletID into the JSON part (but again, having the wallet selector in the JSON layer makes later server separation harder).

The walletID should in future not be a representation of the filename. User should be able to manually choose the walletID. Ideally, we write the walletID into the wallet database. There we could only allow alphanumeric chars.

ryanofsky · 2017-06-23T11:06:23Z

but again, having the wallet selector in the JSON layer makes later server separation harder

What's this about? I think I missed this point...

Ideally, we write the walletID into the wallet database. There we could only allow alphanumeric chars.

I don't think I understand the advantages of adding a "walletID." The user already has to choose one name for the wallet when they make a filename, so what's the advantage of being able to choose two potentially different names for the same wallet? It seems like this would only add confusion.

Also, even you only allow alphanumerics, unless you restrict to english, you still need percent encoding for unicode.

jonasschnelli · 2017-06-23T11:34:28Z

but again, having the wallet selector in the JSON layer makes later server separation harder

What's this about? I think I missed this point...

If we would decouple the wallet completely, a specific endpoint could probably be the better approach to switch between processes or/and even implementations.

I don't think I understand the advantages of adding a "walletID." The user already has to choose one name for the wallet when they make a filename, so what's the advantage of being able to choose two potentially different names for the same wallet? It seems like this would only add confusion.

Yeah. Not sure if custom wallet names is a good idea. AFAIK most multiwallet application offer a custom wallet naming.
But I agree, walletID should probably be the filename, otherwise this may lead to huge confusion.

Also, even you only allow alphanumerics, unless you restrict to English, you still need percent encoding for unicode.

Indeed.

API design

Using the HTTP request path makes sense to me.

URLEncode/Decode would then be required if we use the path approach (not required if we embed the walletID in the JSON payload).

Lets have a look at GitHub's API (I don't say its good just because it's GitHub):
https://api.github.com/repos/bitcoin/bitcoin/issues?state=closed.

For me, that makes perfect sense.
We locate a resource via: https://api.github.com/repos/bitcoin/bitcoin/issues and query it with state=closed.

Same for HTTP posts:
https://api.github.com/repos/bitcoin/bitcoin/pulls/10000.
Then a JSON post of

{
  "title": "new title",
  "body": "updated body",
  "state": "open",
  "base": "master"
}

ryanofsky · 2017-06-23T19:41:27Z

To summarize current state of things, we have 3 (nonexclusive) choices for allowing existing RPC methods work on multiple wallets:

Add optional "wallet" JSON-RPC parameters to wallet RPC methods. There implementations of this in Simple, backwards compatible RPC multiwallet support (superseded by #10829) #10653 and Add optional wallet=filename arguments to wallet RPCs #10661.
Add a wallet option to the JSON-RPC endpoint URL. Could just be a query option, or could be baked into structure of a new request-path scheme. This is implemented here in Multiwallet: add RPC endpoint support #10650.
Add a wallet option to -rpcauth config strings. This is implemented in RPC: Allow rpcauth configs to specify a 4th parameter naming a specific wallet #10615.

jnewbery · 2017-06-28T14:03:25Z

Long term, here's one suggestion of how this could work:

each wallet runs in its own process, and binds to a separate local port. Users can access RPCs for wallet commands directly on that port
each wallet has its own rpcauth config, to control access to that wallet. A single user may have access to multiple wallets
The bitcoin-server RPC server can dispatch wallet commands to individual wallets based on which endpoint the RPC was received on (either as a query option or request-path scheme). We could either make bitcoin-server RPC users have superuser access to all wallets or authenticate per call.

I think that achieves the multi-user functionality from #10615 without the drawbacks:

it's more secure since individual wallet users won't have access to the bitcoin-server RPC interface, and will only be connecting to the wallet process
it doesn't tie users to as single wallet
it doesn't add dependencies from bitcoin-server to bitcoin-wallet.

Short-term, we can implement this PR, either with query options or a request-path scheme. When the wallet is separated, the interface doesn't have to change, since this is (3) from the design above. My preference is for a query option, since this seems more flexible and doesn't restrict the API scheme if we want to use a different request-path in future.

If it's going to take time to get this PR merged, I think we should merge #10653 as a short-term measure, with warnings that the API will change.

promag · 2017-07-04T13:54:36Z

src/utilstrencodings.cpp

@@ -701,3 +702,28 @@ bool ParseFixedPoint(const std::string &val, int decimals, int64_t *amount_out)
    return true;
 }

+std::string urlEncode(const std::string &urlPart) {


Use evhttp_uriencode()?

promag · 2017-07-04T13:54:52Z

src/utilstrencodings.cpp

+    return escaped.str();
+}
+
+std::string urlDecode(const std::string &urlEncoded) {


Use evhttp_uridecode()?

Oh! Very good point. Will do that.

promag · 2017-07-04T13:58:55Z

src/bitcoin-cli.cpp

@@ -235,7 +235,13 @@ UniValue CallRPC(const std::string& strMethod, const UniValue& params)
    assert(output_buffer);
    evbuffer_add(output_buffer, strRequest.data(), strRequest.size());

-    int r = evhttp_make_request(evcon.get(), req.get(), EVHTTP_REQ_POST, "/");
+    // check if we should use a special wallet endpoint
+    std::string endpoint = "/";


Use evhttp_uri and it's primitives?

I guess for the endpoint a plain std::string is okay.

promag · 2017-07-04T14:03:00Z

src/wallet/wallet.h

@@ -1122,6 +1124,8 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
       caller must ensure the current wallet version is correct before calling
       this function). */
    bool SetHDMasterKey(const CPubKey& key);
+
+    const std::string GetWalletID() { return walletID; }


jonasschnelli · 2017-07-07T14:44:12Z

Rebased and overhauled.
Added urlencode/urldecode via libevent (thanks @promag for the hint).
There is now a /v1/node/ and a /v1/wallet/<filename> endpoint.
The endpoint-split is not very mature yet, but does the job (we should then mark it as experimental in the 0.15 release notes).

What can be done outside – this already large – PR:

Better API versioning
RPC tests multiwallet support (currently approach self.nodes[1].<walletfilename>.metho�d may not be the best)
Release notes

Passed travis now (see circular dependency fix 386c299 [ping @theuni]).

TheBlueMatt

Note that /v1/node is never registered as an HTTP handler, so calls to v1/node fail.

TheBlueMatt · 2017-07-10T15:09:50Z

src/wallet/rpcwallet.cpp

@@ -43,7 +43,7 @@ CWallet *GetWalletForJSONRPCRequest(const JSONRPCRequest& request)
    }
    else if (request.URI.substr(0, WALLET_ENDPOINT_BASE.size()) == WALLET_ENDPOINT_BASE) {
        // wallet endpoint was used
-        std::string requestedWallet = request.URI.substr(WALLET_ENDPOINT_BASE.size());
+        std::string requestedWallet = urlDecode(request.URI.substr(WALLET_ENDPOINT_BASE.size()));


Note that "Add -wallet endpoint support to bitcoin-cli" doesnt build as urlDecode is undefined here (you need to swap commit ordering).

In commit "Add -wallet endpoint support to bitcoin-cli"

This change belongs in commit "Select wallet based on the given endpoint" not in this bitcoin-cli commit. Also the commit introducing url encode/decode functions needs to be pushed back earlier in the history like Matt said for this to compile.

Alternately you could consolidate some of the commits. I don't think having 12 commits in this PR accomplishes very much when many of the commits introduce functionality that isn't called anywhere and doesn't make sense without context from later commits.

TheBlueMatt · 2017-07-10T15:22:11Z

src/utilstrencodings.cpp

@@ -10,6 +10,7 @@
 #include <cstdlib>
 #include <cstring>
 #include <errno.h>
+#include <iomanip>


Oh,. yes. This is a relict of a custom URIencode/decode implementation. Will remove.

TheBlueMatt · 2017-07-10T15:31:04Z

src/httprpc.cpp

@@ -226,6 +226,11 @@ static bool InitRPCAuthentication()
    return true;
 }

+void RegisterJSONEndpoint(const std::string& endpoint, bool exactMatch)


This seems superfluous. Why not just either auto-register based on the endpoint in the commands table or just register everything explicitly in httprpc.cpp (its only 4 things).

In commit "Expose JSON endpoint registration"

I don't see the reasoning for this either... If you decide to keep this, maybe document the function with a comment to explain why the urls shouldn't be listed in a single place.

a) I think we don't want an #ifdef ENABLE_WALLE in httpserver.cpp
b) Using RegisterHTTPHandler(endpoint, exactMatch, HTTPReq_JSONRPC); from the point where we can register based on RPC-tables endpoints would result in exposing RegisterHTTPHandler and HTTPReq_JSONRPC which seems unideal.

But the idea of register based on the tables endpoints makes sense, will implement but will also keep the RegisterJSONEndpoint function.

TheBlueMatt · 2017-07-10T15:31:35Z

src/rpc/blockchain.cpp

-    { "blockchain",         "verifychain",            &verifychain,            true,  {"checklevel","nblocks"} },
-
-    { "blockchain",         "preciousblock",          &preciousblock,          true,  {"blockhash"} },
+    { "blockchain",         "/v1/node/",  "getblockchaininfo",      &getblockchaininfo,      true,  {} },


Can we use some constant for this instead of duplicating the string everyhwere?

ryanofsky

Most important changes requested:

~~~Make it an error to a request a wallet name if vpwallets is empty, instead of silently ignoring the requested wallet, #10650 (comment)~~~
Make it an error not to explicitly specify wallet name when more than one wallet is loaded, #10650 (comment)
~~~Add documentation for new bitcoin-cli -wallet option, #10650 (comment), and for new URL scheme.~~~
~~~Fix broken commit history, or consolidate commits.~~~

ryanofsky · 2017-07-12T10:51:51Z

src/httprpc.cpp

@@ -226,6 +226,11 @@ static bool InitRPCAuthentication()
    return true;
 }

+void RegisterJSONEndpoint(const std::string& endpoint, bool exactMatch)


In commit "Expose JSON endpoint registration"

I don't see the reasoning for this either... If you decide to keep this, maybe document the function with a comment to explain why the urls shouldn't be listed in a single place.

ryanofsky · 2017-07-12T10:57:26Z

src/wallet/rpcwallet.cpp

@@ -3044,3 +3048,11 @@ void RegisterWalletRPCCommands(CRPCTable &t)
    for (unsigned int vcidx = 0; vcidx < ARRAYLEN(commands); vcidx++)
        t.appendCommand(commands[vcidx].name, &commands[vcidx]);
 }
+
+void RegisterWalletRPCEndpoints()


In commit "Register /wallet/* endpoint in RPC server"

Name MaybeRegisterWalletRPCEndpoints might be more descriptive since nothing happens if wallet is disabled.

ryanofsky · 2017-07-12T11:04:20Z

src/wallet/rpcwallet.cpp

+            }
+        }
+        // no wallet found with the given endpoint
+        throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet not found ("+SanitizeString(requestedWallet)+")");


In commit "Select wallet based on the given endpoint"

Previous comment not addressed (#10650 (comment))

ryanofsky · 2017-07-12T11:11:13Z

src/wallet/rpcwallet.cpp

-    return vpwallets.empty() ? nullptr : vpwallets[0];
+    LOCK(cs_main); // for now, protect vpwallets via cs_main
+
+    if (vpwallets.empty()) {


In commit "Select wallet based on the given endpoint"

This logic doesn't seem right because it will silently ignore the requestedWallet value when vpwallets is empty. It would be better to trigger the "Requested wallet not found" error if requestedWallet is nonempty when vpwallets is empty instead of silently ignoring the requestedWallet value.

You could fix this by just deleting this early return and changing the last line to return vpwallets.empty() ? nullptr : vpwallets[0];

Thread #10650 (comment)

Silently ignoring invalid requestedWallet values not (yet) addressed.

ryanofsky · 2017-07-12T11:23:25Z

src/wallet/rpcwallet.cpp

+    }
+    else {
+        // default endpoint, use first wallet
+        return vpwallets[0];


In commit "Select wallet based on the given endpoint"

Per @laanwj's comment at #10653 (comment), I think it would be a good idea for this to raise an error if vpwallets.size() is not 1. If somebody has loaded multiple wallets, they should have to explicitly specify which wallet they want to call. Otherwise, it's easy to imagine writing a script that makes many rpc calls, and forgets to add the -wallet= option in one call, and suddenly winds up using funds or balances from the wrong wallet, causing dangerous or difficult to debug errors.

Having a default wallet is needed for backwards compatibility when there is 1 wallet, but otherwise it just seems like a dangerous and not very useful idea to want to support right now.

Thread #10650 (comment)

Unsafe fallthrough to default wallet not (yet) addressed.

I'm not sure about that one.
The GUI also has a default wallet. Wouldn't throwing an exception also not work when someone runs with -disablewallet?

Thread #10650 (comment)

I think you could change this line to something like return vpwallets.size() == 1 ? vpwallets[0] : nullptr; without bad side effects for the gui or disablewallet. This would also let you drop the early return for vpwallet.empty() above, which would make sure the requestedWallet value is always properly validated (#10650 (comment)).

ryanofsky · 2017-07-12T14:42:06Z

src/httpserver.cpp

@@ -665,3 +665,14 @@ void UnregisterHTTPHandler(const std::string &prefix, bool exactMatch)
    }
 }

+std::string urlDecode(const std::string &urlEncoded) {
+    std::string res = "";


In commit "Add urlencode/decode via libevent2"

More idiomatic / efficient to just use the default string constructor (write std::string res;).

ryanofsky · 2017-07-12T14:43:39Z

src/bitcoin-cli.cpp

        // always use v1 endpoint if -wallet has been provided
-        endpoint = "/v1/wallet/"+urlEncode(GetArg("-wallet", ""));


In commit "Add urlencode/decode via libevent2"

Something seems to have gone a little haywire in the rebased history. This commit seems to be inlining a nonexistent urlEncode function, which I think would be a step backwards from having a reusable urlEncode function that complements urlDecode.

Also this commit has unrelated changes to src/utilstrencodings.cpp and src/wallet/rpcwallet.cpp which should be reverted.

ryanofsky · 2017-07-12T14:49:56Z

test/functional/test_framework/authproxy.py

+        method = self._service_name
+        elements = self._service_name.split(".")
+        if len(elements) > 1:
+            urlAdd = "/v1/wallet/" + '.'.join(elements[:-1])


In commit "[QA] Add support for wallet endpoints in Authproxy"

It's fragile, over complicated, and not necessary to add multiwallet code and hardcoded url fragments in authproxy. I suggested a simpler approach here: #10650 (comment), adding a one-line __idiv__ method.

Thread #10650 (comment)

Test framework comment not (yet) addressed.

ryanofsky · 2017-07-12T15:30:49Z

src/rpc/server.cpp

@@ -493,7 +493,12 @@ UniValue CRPCTable::execute(const JSONRPCRequest &request) const
    const CRPCCommand *pcmd = tableRPC[request.strMethod];
    if (!pcmd)
        throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Method not found");
-
+    if (request.URI != "" && request.URI != "/") {
+        // enforce correct endpoint usage


In commit "Split node / wallet RPC calls based on the endpoint"

I don't see what this has to do with multiwallet support and I think this commit and previous commit should go into a separate PR if we want to introduce a more heavily embroidered URL scheme beyond /v1/wallet/filename.

Also if there's going to be a JSON-RPC URL scheme, it needs to be documented and explained somewhere (maybe a new doc/JSONRPC-interface.md file), and should probably get a some more discussion. Personally I don't see why people seem to be in love with convoluted URL schemes. We are not implementing a REST API which is centered around URLs. We are implementing a RPC API where you call a method, pass JSON arguments, and get a JSON return value. I don't think it's good to be adding restrictions and side-channels around the JSON requests without explicit reason.

Supporting a ?wallet=filename query option or /wallet/filename path selector makes a certain amount of sense if we think JSON-RPC named arguments are too awkward, or because we want to implement a multiprocess framework like the one John described where the RPC handler has "superuser access" to all wallets and forwards calls to them (#10650 (comment)).

Supporting /v1, /v2 path selectors maybe also makes sense if we want to be able to make breaking changes to RPC methods. And maybe requiring node / wallet URL endpoints also makes sense for reasons that I don't understand, but I definitely think those reasons deserve to be discussed, and to get some documentation, and not to be shoehorned into the last 2 commits of a tangentially related PR.

In the last weeks IRC meeting I had the feeling we had consensus about splitting the calls into node non node.
IMO the v1 approach (while still supporting / [v0]) gives us the chance to eventually clean up some APIs (accounts?!). Not sure if we are going to do this but at least we would have the chance.
This is why I think we should mark the V1 api as "experimental" and "unstable" which then allows us to remove/add things without caring to much about API compatibility.

Thread #10650 (comment)

But why do those two commits need to be in this PR? All I am saying is move these to a separate PR. These changes aren't needed for multiwallet, add a bunch of boilerplate to the code and complexity to review, and introduce a bunch of rules and design details (even if marked "experimental") that have never been discussed anywhere and aren't documented at all.

We are right before the deadline for 0.15 features and trying to get some kind of multiwallet support in, which only requires support for /v1/wallet/filename urls which the other 10 commits in your PR implement. The 2 commits which expand the URL scheme and change the way RPC calls are dispatched should be moved to a separate PR and not rushed in before 0.15 because they greatly increase complexity of the PR and are not needed for multiwallet RPC access.

They are in this PR because I thought we have decided to not support node command on /v1/wallet.

Thread #10650 (comment)

They are in this PR because I thought we have decided to not support node command on /v1/wallet.

Interesting. So will wallet-optional methods like validateaddress and createmultisig then only work on vpwallet[0] and not other wallets?

In any case, I am trying to suggest that you can significantly simplify this PR and focus it to the task at hand (adding multiwallet RPC support) by not doing this.

For what it's worth, I agree with @ryanofsky here. It'd be good to keep this PR to the minimal functionality needed for RPC multiwallet access. We can easily add /v1/node later if people want it.

will wallet-optional methods like validateaddress and createmultisig...

Hopefully those will all be split into wallet/non-wallet methods by 0.16. See #10583, #10570, etc

Thread #10650 (comment)

I'm not going to keep objecting, but will just note that I'd still prefer to see this commit ("Split node / wallet RPC calls based on the endpoint") dropped from the PR because of the complexity it adds. (Also because of my general failure to understand why people seem to want to overlay a REST-style url scheme on top of an RPC-style API, #10650 (comment).)

ryanofsky · 2017-07-12T15:35:03Z

src/wallet/rpcwallet.cpp

@@ -2434,6 +2434,7 @@ UniValue getwalletinfo(const JSONRPCRequest& request)
    UniValue obj(UniValue::VOBJ);

    size_t kpExternalSize = pwallet->KeypoolCountExternalKeys();
+    obj.push_back(Pair("walletid", pwallet->GetWalletID()));


In commit "Add walletid to getwalletinfo"

Should be called wallet_path as in #10733, or walletname as in #10604 and use the GetName method, unless we actually do want to introduce a new wallet id concept. This commit could be also be dropped and left for the other prs.

theuni · 2017-07-12T18:25:36Z

@jonasschnelli build change looks fine.

sipa · 2017-07-12T19:18:16Z

Concept ACK on this approach for multiwallet, but needs rebase and addressing of comments.

jnewbery · 2017-07-14T15:06:29Z

first -wallet is "default wallet"

There is no default wallet. There should be no functional difference between wallets when multiple wallets are loaded.

instagibbs · 2017-07-14T15:08:28Z

@jnewbery ok let me rephrase: non-endpoint calls will still use that wallet(just like before).

ryanofsky · 2017-07-14T15:22:15Z

I think this PR started off bad and has gotten worse, and I'm writing another post to go into what I think the new problems are. But because I think there's a good chance that post will be ignored or disputed, I want to suggest 2 more changes that I think would make this PR less bad.

Rename -usewallet to -rpcwallet or similar. Prefix "use" is kind of ridiculous unless you think someone is going to go through the trouble of setting an option that they don't actually want to be used.
Rename /v1 endpoint to /v1-unstable or /rpc/v1-unstable. I think the /rpc prefix would be good to add because now that the RPC handler is annexing uri-path space, it should try to leave other parts of the space open for different applications like a possible web interface. But I think the -unstable suffix is more important because plain v1 v2 could imply that we have some kind of plan for versioning and backwards compatibility, which we don't, given the fact that that we have talked about the v1 namespace being "EXPERIMENTAL" https://botbot.me/freenode/bitcoin-core-dev/msg/88537348/ and as recently as yesterday have discussed making incompatible changes.

jnewbery · 2017-07-14T15:23:46Z

non-endpoint calls will still use that wallet(just like before).

Huh? No. That's the point. We want to avoid RPCs defaulting to any wallet when they're called without endpoints. Having a 'default' wallet risks users accidentally calling a method on the wrong wallet.

promag · 2017-07-14T15:04:30Z

src/wallet/rpcwallet.cpp

 CWallet *GetWalletForJSONRPCRequest(const JSONRPCRequest& request)
 {
-    // TODO: Some way to access secondary wallets
-    return vpwallets.empty() ? nullptr : vpwallets[0];
+    if (request.URI.substr(0, WALLET_ENDPOINT_BASE.size()) == WALLET_ENDPOINT_BASE) {


if (request.URI.compare(0, WALLET_ENDPOINT_BASE.size(), WALLET_ENDPOINT_BASE) == 0)

promag · 2017-07-14T15:08:14Z

src/wallet/rpcwallet.cpp

+        // no wallet found with the given endpoint
+        throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Wallet file does not exist or is not loaded");
+    }
+    else {


The } else { can be removed since the if blocks always exists the function.

promag · 2017-07-14T15:12:40Z

src/httpserver.cpp

+        if (decoded) {
+            res = std::string(decoded);
+            free(decoded);
+        }


} else {?

I guess there is no need for an else, it will just return an empty string if URLencode failed which is fine IMO.

promag · 2017-07-14T15:17:22Z

src/httpserver.cpp

@@ -665,3 +669,14 @@ void UnregisterHTTPHandler(const std::string &prefix, bool exactMatch)
    }
 }

+std::string urlDecode(const std::string &urlEncoded) {
+    std::string res;
+    if (!urlEncoded.empty()) {


evhttp_uridecode works with an empty string, as such this if can be removed.

promag · 2017-07-14T15:19:03Z

src/rpc/server.cpp

@@ -494,6 +498,13 @@ UniValue CRPCTable::execute(const JSONRPCRequest &request) const
    if (!pcmd)
        throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Method not found");

+    // enfore endpoint only for non / and "" endpoint (the later is used by the GUI)
+    if (request.URI != "" && request.URI != "/") {


if (!request.URI.empty() && ...)

instagibbs · 2017-07-14T15:39:49Z

@jnewbery My mistake, I probably iterated too fast through options. Can confirm that it gives Method not found (disabled) when 2+ wallets are loaded.

morcos

utACK 759dc37

Thanks for doing this!

I think @ryanofsky has some good points, but it's really not the fault of this PR, it's important to get this in for 0.15 and for better or worse we decided on this approach as a project.

Regarding his 2 suggested changes, I think both would be improvements but aren't particularly necessary if there is disagreement.

morcos · 2017-07-14T15:29:36Z

src/httpserver.cpp

@@ -648,7 +648,11 @@ HTTPRequest::RequestMethod HTTPRequest::GetRequestMethod()
 void RegisterHTTPHandler(const std::string &prefix, bool exactMatch, const HTTPRequestHandler &handler)
 {
    LogPrint(BCLog::HTTP, "Registering HTTP handler for %s (exactmatch %d)\n", prefix, exactMatch);
-    pathHandlers.push_back(HTTPPathHandler(prefix, exactMatch, handler));
+    HTTPPathHandler pathHandler(prefix, exactMatch, handler);
+    if (std::find_if(pathHandlers.begin(), pathHandlers.end(), [pathHandler](const HTTPPathHandler a){ return (a.prefix == pathHandler.prefix && a.exactMatch == pathHandler.exactMatch); }) == pathHandlers.end()) {


nit: should your lambda capture list and argument be references?

What about something along these lines:

// only add handlers if they do not exists yet for (auto handler : pathHandlers) { if (handler.prefix == prefix && handler.exactMatch == exactMath) { return; } } pathHandlers.push_back(HTTPPathHandler(prefix, exactMatch, handler));

Another option is to implement HTTPPathHandler::operator==?

@promag: I think your solution would be a slower find algorithm (but doesn't matter). I had the == operator in an earlier version but @ryanofsky said (and I agreed) that this may be dangerous if we not check all of the instance variables (including the handler).

@morcos: Yes, Should be referenced.

ryanofsky · 2017-07-14T17:03:49Z

This PR is looking worse than before. This is bad engineering and interface design, which creates usability problems with confusing errors and restrictions, and will lead to hacks, workarounds, and churn in the future. All of which is pointless because we have a much simpler alternative in master...ryanofsky:pr/multiparam which has none of these problems and is 100% compatible with endpoints if we want to add them later.

Previously and with master...ryanofsky:pr/multiparam, to perform an operation you would call a RPC method on the server, pass it a set of JSON parameters, and get back a JSON result. This is a simple, straightforward paradigm familiar to anyone with any background in programming. Now, for no one's benefit, we want to overlay complexity and restrictions on top of this when we haven't actually figured out what the restrictions should be and are still discussing different possible workarounds for the usability problems this change causes.

Instead of having method calls with simple parameters, we are now throwing a REST URL scheme into the mix (without adopting other REST principles, which don't make sense for a non-REST RPC interface to begin with), so instead of (method + params -> result) we now have (method + params + uri-path with some redundant information already derivable from method and params and incomplete and changing consistency requirements -> result). But this has been my objection all along, so here is why I now think this change is worse than before:

By passing the wallet out of band, instead of like any other normal RPC parameter, we have to give it special treatment that adds complexity to bitcoin client. We have to make it a command line option, but we can't make it consistent with bitcoind's -wallet=filename option, because then it will get picked up from the config file and create a trapped in wallet situation where it is impossible to call node methods. Workaround is to introduce a pointless "use" prefix that exists for no other bitcoin-cli option so we now have -usewallet=filename alongside -wallet=filename. (Perhaps in the future we could add -dontusewallet=filename and -maybeusewallet=filename to expand the variety of ways our tools take their wallet filenames.)
To avoid more cases of the trapped in wallet problem above, it was proposed to have bitcoin-cli look at the method name and determine whether to silently drop the -usewallet option in bitcoin-cli. Third party RPC utils and some future QT multiwallet RPC console will have to do something similar, and these changes are going to be messy. I'm not looking forward to reviewing them, and they would be unnecessary if we just treated the wallet name like any other normal RPC parameter.

As an alternate solution to trapped in wallet problems, it was proposed to allow /v1/wallet/filename endpoint to actually accept both wallet AND node RPCs. This of course makes sense on some level, but it would seem to undermine any motivation for adding these restrictions in the first place. If I'm supposed to believe that we live in an upside-down world where creating confusing interface restrictions for no reason now is good for users because it will teach them to send node and wallet requests to different endpoints in the future, isn't that goal undermined by creating a brand new endpoint that's in a different place than before but still accepts both node and wallet commands?

I'm maybe just failing to understand what is motivating this change. I think there is a boiling frog thing going on because this PR started off simpler without encoding, and node endpoints, and default wallet restrictions, and the -usewallet inconsistency, and sort of metastasized over time into it's current state. But meanwhile we have a simpler alternative (that without help and tests adds exactly 11 lines of code) and requires no changes to RPC clients, and is completely compatible with endpoints if we ever wanted to add them later. Obviously, if you accept the pleasure is pain, pain is pleasure rationale for uri-path restrictions, then #10650 is pretty ideal. But otherwise I think master...ryanofsky:pr/multiparam is a much simpler and saner way to roll out multiwallet support.

jnewbery · 2017-07-14T17:56:40Z

Thanks @ryanofsky - I think those are all good points, and I'm sympathetic to them.

At this point, I think we should go ahead with merging this PR. It's had lots of review, and already has 4 ACKs.

We still have some freedom with the interface after this is merged. The feature is going to be marked as experimental, and anyone who uses multiwallet should be prepared for the interface to be tweaked as the feature becomes more mature. For example, in bitcoin.cli, we could just ignore any -wallet arguments in the .conf file, and have the user explicitly have to set -wallet on the command line. I think that would allay your concerns there. Should we do that in this PR? Perhaps, but at this point there's already been so much churn in this PR, that I think we can delay that until a later PR.

So in order to get something merged which is now very well reviewed and tested, we should go ahead with this PR and tidy up the rough edges later.

ryanofsky · 2017-07-14T18:01:01Z

So in order to get something merged which is now very well reviewed and tested, we should go ahead with this PR and tidy up the rough edges later.

Ok but these changes are never going to happen later if they don't get done before merging this PR: #10650 (comment), so please say something if there is a problem with -unstable or -rpcwallet

jnewbery · 2017-07-14T18:11:31Z

-unstable : I disagree that there is an implication that v1 commits us to backwards compatibility, so I don't think this is necessary
-rpcwallet vs -usewallet : Yours is slightly better, but I don't think it matters that much.

jonasschnelli · 2017-07-14T18:22:29Z

I agree with most nits from @promag and @morcos but for the sake of getting this merged before the freeze I'm no longer force-push fixing nits.

ryanofsky · 2017-07-14T18:54:14Z

-unstable : I disagree that there is an implication that v1 commits us to backwards compatibility, so I don't think this is necessary

I might have misunderstood what was meant by the v1 endpoint being "experimental." If experimental just means that the endpoint might go away in the future (to be replaced by v2 v3 etc), then agree it doesn't have to be specially marked, though, it would be nicer if it were.

But if "experimental" means that behavior of the endpoint is unstable and can change without concern for backward compatibility then it definitely should be marked -unstable, because when you use the same version number for two different things you definitely are implying that the new thing is compatible with the old thing.

sipa · 2017-07-14T19:43:23Z

@ryanofsky's commit looks so simple that I'm inclined at this point to go with that, and push the v1 API to a later version...

morcos · 2017-07-14T19:57:33Z

@sipa I think I'm also fine with that.
This appears merge ready, lets get a few quick reviews of #10653 and then we could merge either one

ryanofsky · 2017-07-14T20:00:26Z

This appears merge ready, lets get a few quick reviews of #10653 and then we could merge either one

Github won't let me reopen #10653 (and contained an old snapshot of the branch) so I opened #10829.

laanwj · 2017-07-17T07:17:33Z

It looks like this implementation is still too controversial to make it into 0.15, so removing the milestone (sorry, this is a very painful decision for me too).

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

ryanofsky · 2017-07-25T16:02:03Z

Should be closed or rebased. One part of this PR which I think is good and didn't make it in with #10849 was addition of the RPC_WALLET_NOT_FOUND error code.

jonasschnelli · 2017-09-05T22:50:57Z

Closing for now... seems no longer required (or at least controversial).

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

jonasschnelli added the Wallet label Jun 22, 2017

jonasschnelli force-pushed the 2017/06/wallet_rpc_endpoint branch from d47c387 to 6c4b1ba Compare June 22, 2017 07:39

ryanofsky reviewed Jun 22, 2017

View reviewed changes

ryanofsky mentioned this pull request Jun 22, 2017

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

Closed

jonasschnelli force-pushed the 2017/06/wallet_rpc_endpoint branch from 88ee520 to 1140b58 Compare June 23, 2017 08:30

promag reviewed Jul 4, 2017

View reviewed changes

jnewbery mentioned this pull request Jul 5, 2017

[wallet] [tests] Add listwallets RPC, include wallet name in getwalletinfo and add multiwallet test #10604

Merged

ryanofsky mentioned this pull request Jul 6, 2017

Add optional wallet=filename arguments to wallet RPCs #10661

Closed

jonasschnelli force-pushed the 2017/06/wallet_rpc_endpoint branch 2 times, most recently from 35dab3f to 386c299 Compare July 7, 2017 14:03

jonasschnelli added this to the 0.15.0 milestone Jul 9, 2017

TheBlueMatt reviewed Jul 10, 2017

View reviewed changes

ryanofsky reviewed Jul 12, 2017

View reviewed changes

promag reviewed Jul 14, 2017

View reviewed changes

morcos approved these changes Jul 14, 2017

View reviewed changes

ryanofsky mentioned this pull request Jul 14, 2017

Simple, backwards compatible RPC multiwallet support. #10829

Closed

laanwj modified the milestones: 0.16.0, 0.15.0 Jul 17, 2017

jonasschnelli mentioned this pull request Jul 17, 2017

Multiwallet: simplest endpoint support #10849

Merged

jonasschnelli closed this Sep 5, 2017

ryanofsky mentioned this pull request Nov 17, 2017

External wallet files #11687

Merged

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

		// always use v1 endpoint if -wallet has been provided
		endpoint = "/v1/wallet/"+urlEncode(GetArg("-wallet", ""));

Multiwallet: add RPC endpoint support #10650

Multiwallet: add RPC endpoint support #10650

Uh oh!

Conversation

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

Uh oh!

Uh oh!

luke-jr commented Jun 22, 2017

Uh oh!

jonasschnelli commented Jun 22, 2017

Uh oh!

ryanofsky left a comment

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!

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

Uh oh!

Uh oh!

ryanofsky commented Jun 22, 2017

Uh oh!

ryanofsky Jun 22, 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!

gmaxwell commented Jun 22, 2017

Uh oh!

jonasschnelli commented Jun 22, 2017

Uh oh!

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

Uh oh!

Uh oh!

ryanofsky commented Jun 22, 2017

Uh oh!

jonasschnelli commented Jun 23, 2017

Uh oh!

ryanofsky commented Jun 23, 2017

Uh oh!

jonasschnelli commented Jun 23, 2017

API design

Uh oh!

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

Uh oh!

Uh oh!

jnewbery commented Jun 28, 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!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Jun 22, 2017 •

edited

Loading

laanwj commented Jun 22, 2017 •

edited

Loading

ryanofsky Jun 22, 2017 •

edited

Loading

ryanofsky commented Jun 22, 2017 •

edited

Loading

ryanofsky commented Jun 23, 2017 •

edited

Loading

jonasschnelli commented Jul 7, 2017 •

edited

Loading

ryanofsky left a comment •

edited

Loading