Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Multiwallet: add RPC endpoint support #10650
Conversation
jonasschnelli
added the
Wallet
label
Jun 22, 2017
|
This would make much more sense as |
No. Please no query string. POST data to |
ryanofsky
reviewed
Jun 22, 2017
utACK 6c4b1ba 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.
| - 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", "") != "") { |
ryanofsky
Jun 22, 2017
Contributor
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)"));
ryanofsky
Jul 12, 2017
Contributor
In commit "Add -wallet endpoint support to bitcoin-cli"
Previous comment not addressed (#10650 (comment)).
| + // check if we should use a special wallet endpoint | ||
| + std::string endpoint = "/"; | ||
| + if (GetArg("-wallet", "") != "") { | ||
| + endpoint = "/wallet/"+GetArg("-wallet", ""); |
ryanofsky
Jun 22, 2017
Contributor
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.
| + method = self._service_name | ||
| + elements = self._service_name.split(".") | ||
| + if len(elements) > 1: | ||
| + urlAdd = "/wallet/" + '.'.join(elements[:-1]) |
ryanofsky
Jun 22, 2017
Contributor
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()
ryanofsky
Jul 13, 2017
Contributor
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__)
jnewbery
Jul 13, 2017
Member
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.
|
Concept ACK, will review.
I tend to agree:
|
|
I guess personally I don't see how 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. |
| + } | ||
| + } | ||
| + // no wallet found with the given endpoint | ||
| + throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet not found ("+SanitizeString(requestedWallet)+")"); |
ryanofsky
Jun 22, 2017
•
Contributor
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 ("%-_.~!*'();:@&=+$,/?#[]").
ryanofsky
Jul 12, 2017
Contributor
In commit "Select wallet based on the given endpoint"
Previous comment not addressed (#10650 (comment))
ryanofsky
Jul 13, 2017
Contributor
Thread #10650 (comment)
Error string cleanup not (yet) addressed.
|
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? |
|
Yes. Versioning makes probably sense at this point. I could think of 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? I don't think there are huge advantages/disadvantages between query-string vs path. It's probably a design choice, matter of taste. |
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 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 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 |
ryanofsky
referenced
this pull request
Jun 22, 2017
Closed
Simple, backwards compatible RPC multiwallet support (superseded by #10829) #10653
|
Another multiwallet RPC implemention for discussion: #10653 |
|
Added the /v1/ path element. @ryanofsky pointed out that we should urlencode/decode the walletID in the path element. 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. |
What's this about? I think I missed this point...
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. |
If we would decouple the wallet completely, a specific endpoint could probably be the better approach to switch between processes or/and even implementations.
Yeah. Not sure if custom wallet names is a good idea. AFAIK most multiwallet application offer a custom wallet naming.
Indeed. API designUsing 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): For me, that makes perfect sense. Same for HTTP posts: {
"title": "new title",
"body": "updated body",
"state": "open",
"base": "master"
} |
|
To summarize current state of things, we have 3 (nonexclusive) choices for allowing existing RPC methods work on multiple wallets:
|
|
Long term, here's one suggestion of how this could work:
I think that achieves the multi-user functionality from #10615 without the drawbacks:
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. |
| @@ -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) { |
| + return escaped.str(); | ||
| +} | ||
| + | ||
| +std::string urlDecode(const std::string &urlEncoded) { |
| @@ -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 = "/"; |
| @@ -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; } |
This was referenced Jul 5, 2017
|
Rebased and overhauled. What can be done outside – this already large – PR:
Passed travis now (see circular dependency fix 386c299 [ping @theuni]). |
jonasschnelli
added this to the
0.15.0
milestone
Jul 9, 2017
TheBlueMatt
reviewed
Jul 10, 2017
Note that /v1/node is never registered as an HTTP handler, so calls to v1/node fail.
| @@ -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())); |
TheBlueMatt
Jul 10, 2017
Contributor
Note that "Add -wallet endpoint support to bitcoin-cli" doesnt build as urlDecode is undefined here (you need to swap commit ordering).
ryanofsky
Jul 12, 2017
Contributor
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.
| @@ -10,6 +10,7 @@ | ||
| #include <cstdlib> | ||
| #include <cstring> | ||
| #include <errno.h> | ||
| +#include <iomanip> |
jonasschnelli
Jul 13, 2017
Member
Oh,. yes. This is a relict of a custom URIencode/decode implementation. Will remove.
| @@ -226,6 +226,11 @@ static bool InitRPCAuthentication() | ||
| return true; | ||
| } | ||
| +void RegisterJSONEndpoint(const std::string& endpoint, bool exactMatch) |
TheBlueMatt
Jul 10, 2017
Contributor
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).
ryanofsky
Jul 12, 2017
Contributor
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.
jonasschnelli
Jul 13, 2017
Member
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.
| - { "blockchain", "verifychain", &verifychain, true, {"checklevel","nblocks"} }, | ||
| - | ||
| - { "blockchain", "preciousblock", &preciousblock, true, {"blockhash"} }, | ||
| + { "blockchain", "/v1/node/", "getblockchaininfo", &getblockchaininfo, true, {} }, |
TheBlueMatt
Jul 10, 2017
Contributor
Can we use some constant for this instead of duplicating the string everyhwere?
ryanofsky
reviewed
Jul 12, 2017
•
Most important changes requested:
-
Make it an error to a request a wallet name ifvpwalletsis 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-walletoption, #10650 (comment), and for new URL scheme. -
Fix broken commit history, or consolidate commits.
| @@ -226,6 +226,11 @@ static bool InitRPCAuthentication() | ||
| return true; | ||
| } | ||
| +void RegisterJSONEndpoint(const std::string& endpoint, bool exactMatch) |
ryanofsky
Jul 12, 2017
Contributor
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.
| @@ -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())); |
ryanofsky
Jul 12, 2017
Contributor
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.
| @@ -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())); |
ryanofsky
Jul 12, 2017
Contributor
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.
| - return vpwallets.empty() ? nullptr : vpwallets[0]; | ||
| + LOCK(cs_main); // for now, protect vpwallets via cs_main | ||
| + | ||
| + if (vpwallets.empty()) { |
ryanofsky
Jul 12, 2017
Contributor
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];
ryanofsky
Jul 13, 2017
Contributor
Thread #10650 (comment)
Silently ignoring invalid requestedWallet values not (yet) addressed.
| + } | ||
| + 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()); |
ryanofsky
Jul 12, 2017
Contributor
In commit "Select wallet based on the given endpoint"
requestedWallet should be percent decoded here.
ryanofsky
Jul 13, 2017
Contributor
Thread #10650 (comment)
Adding percent decoding not (yet) addressed.
| @@ -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() |
ryanofsky
Jul 12, 2017
Contributor
In commit "Register /wallet/* endpoint in RPC server"
Name MaybeRegisterWalletRPCEndpoints might be more descriptive since nothing happens if wallet is disabled.
| + } | ||
| + } | ||
| + // no wallet found with the given endpoint | ||
| + throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet not found ("+SanitizeString(requestedWallet)+")"); |
ryanofsky
Jul 12, 2017
Contributor
In commit "Select wallet based on the given endpoint"
Previous comment not addressed (#10650 (comment))
| + } | ||
| + else { | ||
| + // default endpoint, use first wallet | ||
| + return vpwallets[0]; |
ryanofsky
Jul 12, 2017
Contributor
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.
ryanofsky
Jul 13, 2017
Contributor
Thread #10650 (comment)
Unsafe fallthrough to default wallet not (yet) addressed.
jonasschnelli
Jul 13, 2017
Member
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?
ryanofsky
Jul 13, 2017
•
Contributor
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)).
| @@ -3793,7 +3793,7 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile) | ||
| int64_t nStart = GetTimeMillis(); | ||
| bool fFirstRun = true; | ||
| std::unique_ptr<CWalletDBWrapper> dbw(new CWalletDBWrapper(&bitdb, walletFile)); | ||
| - CWallet *walletInstance = new CWallet(std::move(dbw)); | ||
| + CWallet *walletInstance = new CWallet(std::move(dbw), walletFile); // use filename as walletID for now |
ryanofsky
Jul 12, 2017
Contributor
In commit "Add a walletID to CWallet (use filename for now)"
I believe this commit should be dropped because we already have a CWallet::GetName() method and the duplicative functionality introduced here with mysterious "for now" comments is just confusing.
| @@ -237,9 +237,13 @@ UniValue CallRPC(const std::string& strMethod, const UniValue& params) | ||
| // check if we should use a special wallet endpoint | ||
| std::string endpoint = "/"; | ||
| - if (GetArg("-wallet", "") != "") { | ||
| + std::string walletName = GetArg("-wallet", ""); | ||
| + if (walletName != "") { |
ryanofsky
Jul 12, 2017
Contributor
In commit "Add urlencode/decode via libevent2"
More idiomatic / efficient to write !walletName.empty()
| - 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", "") != "") { |
ryanofsky
Jul 12, 2017
Contributor
In commit "Add -wallet endpoint support to bitcoin-cli"
Previous comment not addressed (#10650 (comment)).
| // always use v1 endpoint if -wallet has been provided | ||
| - endpoint = "/v1/wallet/"+urlEncode(GetArg("-wallet", "")); |
ryanofsky
Jul 12, 2017
Contributor
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.
| - endpoint = "/v1/wallet/"+urlEncode(GetArg("-wallet", "")); | ||
| + char *encodedURI = evhttp_uriencode(walletName.c_str(), walletName.size(), false); | ||
| + if (encodedURI) { | ||
| + endpoint = "/v1/wallet/"+ std::string(encodedURI); |
ryanofsky
Jul 12, 2017
Contributor
In commit "Add urlencode/decode via libevent2"
Memory leak here, need to free encodedURI.
| @@ -665,3 +665,14 @@ void UnregisterHTTPHandler(const std::string &prefix, bool exactMatch) | ||
| } | ||
| } | ||
| +std::string urlDecode(const std::string &urlEncoded) { | ||
| + std::string res = ""; |
ryanofsky
Jul 12, 2017
Contributor
In commit "Add urlencode/decode via libevent2"
More idiomatic / efficient to just use the default string constructor (write std::string res;).
| + method = self._service_name | ||
| + elements = self._service_name.split(".") | ||
| + if len(elements) > 1: | ||
| + urlAdd = "/v1/wallet/" + '.'.join(elements[:-1]) |
ryanofsky
Jul 12, 2017
Contributor
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.
ryanofsky
Jul 13, 2017
Contributor
Thread #10650 (comment)
Test framework comment not (yet) addressed.
| @@ -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 |
ryanofsky
Jul 12, 2017
Contributor
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.
jonasschnelli
Jul 13, 2017
Member
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.
ryanofsky
Jul 13, 2017
•
Contributor
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.
jonasschnelli
Jul 13, 2017
•
Member
They are in this PR because I thought we have decided to not support node command on /v1/wallet.
ryanofsky
Jul 13, 2017
•
Contributor
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.
jnewbery
Jul 13, 2017
Member
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
ryanofsky
Jul 13, 2017
•
Contributor
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).)
|
@jonasschnelli build change looks fine. |
|
Concept ACK on this approach for multiwallet, but needs rebase and addressing of comments. |
| + char *encodedURI = evhttp_uriencode(walletName.c_str(), walletName.size(), false); | ||
| + if (encodedURI) { | ||
| + endpoint = "/v1/wallet/"+ std::string(encodedURI); | ||
| + } |
| + char *encodedURI = evhttp_uriencode(walletName.c_str(), walletName.size(), false); | ||
| + if (encodedURI) { | ||
| + endpoint = "/v1/wallet/"+ std::string(encodedURI); | ||
| + } |
| @@ -10,6 +10,7 @@ | ||
| #include <cstdlib> | ||
| #include <cstring> | ||
| #include <errno.h> | ||
| +#include <iomanip> |
|
It'd be great to get this in for 0.15. I'm holding off on my review until @TheBlueMatt and @ryanofsky comments are addressed. If there's anything I can do to help this be ready in time for feature freeze, let me know. |
|
I just want to point out again that while I am fine with adding support for multiwallet RPC calls via URL endpoints, we do have a vastly simpler alternative available which in my opinion (still) it would be better to roll out first: multiwallet RPC access via named argument master...ryanofsky:pr/multiparam. Disadvantages of master...ryanofsky:pr/multiparam:
Advantages of master...ryanofsky:pr/multiparam
|
| - return vpwallets.empty() ? nullptr : vpwallets[0]; | ||
| + LOCK(cs_main); // for now, protect vpwallets via cs_main | ||
| + | ||
| + if (vpwallets.empty()) { |
ryanofsky
Jul 13, 2017
Contributor
Thread #10650 (comment)
Silently ignoring invalid requestedWallet values not (yet) addressed.
| + } | ||
| + 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()); |
ryanofsky
Jul 13, 2017
Contributor
Thread #10650 (comment)
Adding percent decoding not (yet) addressed.
| + } | ||
| + } | ||
| + // no wallet found with the given endpoint | ||
| + throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet not found ("+SanitizeString(requestedWallet)+")"); |
ryanofsky
Jul 13, 2017
Contributor
Thread #10650 (comment)
Error string cleanup not (yet) addressed.
| + } | ||
| + else { | ||
| + // default endpoint, use first wallet | ||
| + return vpwallets[0]; |
ryanofsky
Jul 13, 2017
Contributor
Thread #10650 (comment)
Unsafe fallthrough to default wallet not (yet) addressed.
| - int r = evhttp_make_request(evcon.get(), req.get(), EVHTTP_REQ_POST, "/"); | ||
| + // check if we should use a special wallet endpoint | ||
| + std::string endpoint = "/"; | ||
| + std::string walletName = GetArg("-wallet", ""); |
ryanofsky
Jul 13, 2017
•
Contributor
In commit "Add urlencode/decode via libevent2"
Commit description should be updated to reflect that this is adding a new option to bitcoin-cl. Or maybe this commit should solely update bitcoin-cli, and the urldecode should be moved to the "Select wallet based on the given endpoint" commit.
Also the new bitcoin-cli option needs to be documented, see #10650 (comment)
jnewbery
Jul 13, 2017
Member
This is still not addressed. If you're going to add extra options to bitcoin-cli, please also update the help text.
Strongly recommend this change goes in its own commit.
| + method = self._service_name | ||
| + elements = self._service_name.split(".") | ||
| + if len(elements) > 1: | ||
| + urlAdd = "/v1/wallet/" + '.'.join(elements[:-1]) |
ryanofsky
Jul 13, 2017
Contributor
Thread #10650 (comment)
Test framework comment not (yet) addressed.
|
|
jonasschnelli |
This commit was signed with a verified signature.
jonasschnelli
Jonas Schnelli
|
858bfcf
|
|
Rebased and tried to address |
| + } | ||
| + else { | ||
| + // default endpoint, use first wallet if possible | ||
| + return vpwallets.empty() ? nullptr : vpwallets[0]; |
jnewbery
Jul 13, 2017
Member
Please see @ryanofsky's previous comment: #10650 (comment)
Returning a 'default' wallet is dangerous for users. If there is more than one wallet loaded and the RPC does not specify a wallet endpoint, then the call should fail.
| @@ -191,3 +191,6 @@ def _get_response(self): | ||
| else: | ||
| log.debug("<-- [%.6f] %s"%(elapsed,responsedata)) | ||
| return response | ||
| + | ||
| + def __truediv__(self, relative_uri): | ||
| + return AuthServiceProxy("{}{}".format(self.__service_url, relative_uri), self._service_name, connection=self.__conn) |
jnewbery
Jul 13, 2017
Member
In commit 'Fix AuthProxy multiwallet support'.
I think using / "path is slightly better since it's consistent with the Python path API, as @ryanofsky mentioned in IRC, but I don't care too strongly.
If you do chose to make this change, please squash with the previous commit.
ryanofsky
Jul 13, 2017
Contributor
In commit "[QA] Fix AuthProxy multiwallet support"
Should drop this commit. This should be {}/{} for consistency with python's pathlib API, and you should change all the / "/path" expressions to / "path" again for consistency with normal python path expressions. (Personally also I think the double slashes look strange).
| + assert_equal(walletinfo['immature_balance'], 0) | ||
| + | ||
| + w3 = self.nodes[0] / "/v1/wallet/w2" | ||
| + walletinfo = w2.getwalletinfo() |
jnewbery
Jul 13, 2017
Member
the w3 assignment is wrong, and this line doesn't appear to be testing anything
| + self.nodes = self.start_nodes(1, self.options.tmpdir, self.extra_args[:1]) | ||
| + | ||
| + def run_test(self): | ||
| + self.nodes[0].generate(1) |
|
Also added the bitcoin-cli help string for |
| - { "control", "help", &help, true, {"command"} }, | ||
| - { "control", "stop", &stop, true, {} }, | ||
| - { "control", "uptime", &uptime, true, {} }, | ||
| + { "control", "/v1/*" , "help", &help, true, {"command"} }, |
jnewbery
Jul 13, 2017
Member
it's a bit strange that the help RPC returns help for non-wallet commands when called on a wallet endpoint
| + { "rawtransactions", "/v1/node/", "decoderawtransaction", &decoderawtransaction, true, {"hexstring"} }, | ||
| + { "rawtransactions", "/v1/node/", "decodescript", &decodescript, true, {"hexstring"} }, | ||
| + { "rawtransactions", "/v1/node/", "sendrawtransaction", &sendrawtransaction, false, {"hexstring","allowhighfees"} }, | ||
| + { "rawtransactions", "/v1/*", "signrawtransaction", &signrawtransaction, false, {"hexstring","prevtxs","privkeys","sighashtype"} }, /* uses wallet if enabled */ |
jnewbery
Jul 13, 2017
Member
Until wallet and server RPCs are fully separated, the following RPCs should also allow wildcard endpoints:
validateaddresscreatemultisig
We can leave getinfo since it's deprecated.
ryanofsky
Jul 13, 2017
Contributor
Thread #10650 (comment)
Wildcard endpoints for wallet-optional rpc not yet addressed.
jnewbery
suggested changes
Jul 13, 2017
A few nits. I think this is almost ready for merge. The one remaining blocking issue for me is the default wallet selection. If there is more than one wallet loaded, then there should not be a default wallet, especially since there's no way to find out which wallet is default. That's a recipe for users to accidentally call wallet commands on the wrong wallet.
Of course, if there is only one wallet loaded, then defaulting to the only wallet is fine (and necessary for backwards compatibility).
The rest of the nits can be addressed in follow-up PRs.
| @@ -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 |
ryanofsky
Jul 13, 2017
•
Contributor
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).)
| @@ -310,6 +311,9 @@ bool CRPCTable::appendCommand(const std::string& name, const CRPCCommand* pcmd) | ||
| if (IsRPCRunning()) | ||
| return false; | ||
| + // make sure we register the command endpoint | ||
| + RegisterJSONEndpoint(pcmd->endpoint, false); |
ryanofsky
Jul 13, 2017
Contributor
In commit "Split node / wallet RPC calls based on the endpoint"
@morcos pointed out that the endpoints get added to a vector (std::vector<HTTPPathHandler> pathHandlers) which will now contain the same handlers dozens of times. Should consider converting pathHandlers to a map or set to avoid this, or adding a todo comment to address it later.
| @@ -493,7 +497,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 != "/") { |
ryanofsky
Jul 13, 2017
Contributor
In commit "Split node / wallet RPC calls based on the endpoint"
Why the first check? I don't think requesting an empty URI is possible over HTTP. Definitely add a comment if this is needed for something.
| CWallet *GetWalletForJSONRPCRequest(const JSONRPCRequest& request) | ||
| { | ||
| - // TODO: Some way to access secondary wallets | ||
| - return vpwallets.empty() ? nullptr : vpwallets[0]; | ||
| + LOCK(cs_main); // for now, protect vpwallets via cs_main |
ryanofsky
Jul 13, 2017
Contributor
In commit "Select wallet based on the given endpoint"
Would be good to either expand comment, or have it reference an issue, or just drop this locking statement because it isn't needed yet and isn't used in any other places where vpwallets is accessed.
| + if (request.URI.substr(0, WALLET_ENDPOINT_BASE.size()) == WALLET_ENDPOINT_BASE) { | ||
| + // wallet endpoint was used | ||
| + std::string requestedWallet = urlDecode(request.URI.substr(WALLET_ENDPOINT_BASE.size())); | ||
| + for(CWallet* pwallet : vpwallets) { |
ryanofsky
Jul 13, 2017
Contributor
In commit "Select wallet based on the given endpoint"
Maybe use Luke's CWalletRef syntax to be consistent & portable.
| // always use v1 endpoint if -wallet has been provided | ||
| - endpoint = "/v1/wallet/"+urlEncode(GetArg("-wallet", "")); |
ryanofsky
Jul 13, 2017
Contributor
In commit "Add urlencode/decode via libevent2"
Should move these bitcoin-cli changes to prior "Add -wallet endpoint support to bitcoin-cli" commit (which doesn't currently) compile.
| + std::string endpoint = "/"; | ||
| + if (GetArg("-wallet", "") != "") { | ||
| + // always use v1 endpoint if -wallet has been provided | ||
| + endpoint = "/v1/wallet/"+urlEncode(GetArg("-wallet", "")); |
ryanofsky
Jul 13, 2017
Contributor
In commit "Add -wallet endpoint support to bitcoin-cli"
This doesn't compile because urlEncode function isn't defined. You should fix by moving bitcoin-cli changes from "Add urlencode/decode via libevent2" commit to this commit.
| @@ -191,3 +191,6 @@ def _get_response(self): | ||
| else: | ||
| log.debug("<-- [%.6f] %s"%(elapsed,responsedata)) | ||
| return response | ||
| + | ||
| + def __truediv__(self, relative_uri): | ||
| + return AuthServiceProxy("{}{}".format(self.__service_url, relative_uri), self._service_name, connection=self.__conn) |
ryanofsky
Jul 13, 2017
Contributor
In commit "[QA] Fix AuthProxy multiwallet support"
Should drop this commit. This should be {}/{} for consistency with python's pathlib API, and you should change all the / "/path" expressions to / "path" again for consistency with normal python path expressions. (Personally also I think the double slashes look strange).
| @@ -310,6 +311,9 @@ bool CRPCTable::appendCommand(const std::string& name, const CRPCCommand* pcmd) | ||
| if (IsRPCRunning()) | ||
| return false; | ||
| + // make sure we register the command endpoint | ||
| + RegisterJSONEndpoint(pcmd->endpoint, false); |
morcos
Jul 13, 2017
Contributor
nit: This seems pretty inefficient since so many of the commands have the same endpoint, they'll be registered over and over.
| @@ -46,6 +46,7 @@ std::string HelpMessageCli() | ||
| strUsage += HelpMessageOpt("-rpcpassword=<pw>", _("Password for JSON-RPC connections")); | ||
| strUsage += HelpMessageOpt("-rpcclienttimeout=<n>", strprintf(_("Timeout in seconds during HTTP requests, or 0 for no timeout. (default: %d)"), DEFAULT_HTTP_CLIENT_TIMEOUT)); | ||
| strUsage += HelpMessageOpt("-stdin", _("Read extra arguments from standard input, one per line until EOF/Ctrl-D (recommended for sensitive information such as passphrases)")); | ||
| + strUsage += HelpMessageOpt("-wallet=<walletname>", _("Use wallet endpoint for given <walletname>")); |
| + std::string endpoint = "/"; | ||
| + if (GetArg("-wallet", "") != "") { | ||
| + // always use v1 endpoint if -wallet has been provided | ||
| + endpoint = "/v1/wallet/"+urlEncode(GetArg("-wallet", "")); |
morcos
Jul 13, 2017
Contributor
urlEncode doesn't exist. I think if you just squashed 6a99141 (the following commit) into this one, you'd be ok.
| CWallet *GetWalletForJSONRPCRequest(const JSONRPCRequest& request) | ||
| { | ||
| - // TODO: Some way to access secondary wallets | ||
| - return vpwallets.empty() ? nullptr : vpwallets[0]; | ||
| + LOCK(cs_main); // for now, protect vpwallets via cs_main |
morcos
Jul 13, 2017
Contributor
Can we establish whether vpwallets is supposed to be protected by cs_main or not?
| + w1.generate(1) | ||
| + | ||
| + #check default wallet balance | ||
| + walletinfo = self.nodes[0].getwalletinfo() |
laanwj
added
to Blockers in High-priority for review
Jul 13, 2017
|
Fixed reported points:
|
|
|
ryanofsky
reviewed
Jul 13, 2017
•
utACK 741b8a6 assuming remaining bugs are fixed.
I do not like this PR, and I think URL endpoints add a lot of complexity to the code and make the client interface bad and awkward for no good reason. The change adding a simple wallet named arg is vastly simpler, more future-proof, and more client-friendly: #10650 (comment).
| @@ -155,6 +155,10 @@ struct HTTPPathHandler | ||
| prefix(_prefix), exactMatch(_exactMatch), handler(_handler) | ||
| { | ||
| } | ||
| + friend bool operator==(const HTTPPathHandler& a, const HTTPPathHandler& b) |
ryanofsky
Jul 13, 2017
Contributor
In commit "Split node / wallet RPC calls based on the endpoint"
This is a dangerous operator to define because it ignores the contents of handler member. Please drop this and just pass a lambda to std::find to do the comparison you want.
| @@ -648,7 +652,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(pathHandlers.begin(), pathHandlers.end(), pathHandler) == pathHandlers.end()) { |
ryanofsky
Jul 13, 2017
Contributor
In commit "Split node / wallet RPC calls based on the endpoint"
If std::find returns an item and item's handler is not identical to handler this should throw or assert. Otherwise code trying to register a different handler will silently fail.
jonasschnelli
Jul 13, 2017
Member
I think that is something we can change later. Or do you think its crucial?
ryanofsky
Jul 13, 2017
Contributor
Thread #10650 (comment)
I think that is something we can change later. Or do you think its crucial?
Not crucial (this isn't a bug and I already acked), but this seems very easy, unless I'm missing something?
auto it = find_if();
if (it == end) push_back(handler); else assert (it->handler == handler.handler);
| + { "rawtransactions", "/v1/node/", "decoderawtransaction", &decoderawtransaction, true, {"hexstring"} }, | ||
| + { "rawtransactions", "/v1/node/", "decodescript", &decodescript, true, {"hexstring"} }, | ||
| + { "rawtransactions", "/v1/node/", "sendrawtransaction", &sendrawtransaction, false, {"hexstring","allowhighfees"} }, | ||
| + { "rawtransactions", "/v1/*", "signrawtransaction", &signrawtransaction, false, {"hexstring","prevtxs","privkeys","sighashtype"} }, /* uses wallet if enabled */ |
ryanofsky
Jul 13, 2017
Contributor
Thread #10650 (comment)
Wildcard endpoints for wallet-optional rpc not yet addressed.
| + throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Wallet file does not exist or is not loaded"); | ||
| + } | ||
| + else { | ||
| + if (vpwallets.size() > 1) { |
ryanofsky
Jul 13, 2017
Contributor
In commit "Select wallet based on the given endpoint":
I think this will prevent wallet commands from showing up in bitcoin-cli help when multiple wallets are loaded. You may need to add && !request.fHelp.
| + else { | ||
| + if (vpwallets.size() > 1) { | ||
| + // don't allow to use a default wallet in multiwallet | ||
| + throw JSONRPCError(RPC_WALLET_ERROR, "No wallet was defined but multiple wallets are present"); |
ryanofsky
Jul 13, 2017
Contributor
In commit "Select wallet based on the given endpoint":
I think it's preferable just to return nullptr in this case. That way commands that don't need a wallet can still function (validateaddress, createmultisig).
|
Fixed the travis error. |
|
|
jonasschnelli |
This commit was signed with a verified signature.
jonasschnelli
Jonas Schnelli
|
6e390d1
|
|
| +# Copyright (c) 2017 The Bitcoin Core developers | ||
| +# Distributed under the MIT software license, see the accompanying | ||
| +# file COPYING or http://www.opensource.org/licenses/mit-license.php. | ||
| +"""Test the wallet.""" |
| + self.num_nodes = 1 | ||
| + self.extra_args = [['-wallet=w1', '-wallet=w2','-wallet=w3']] | ||
| + | ||
| + def setup_network(self): |
jnewbery
Jul 13, 2017
Member
This method override is not required (it's doing the same thing as the base method would do)
| + w1 = self.nodes[0] / "v1/wallet/w1" | ||
| + w1.generate(1) | ||
| + | ||
| + #accessing default wallet must be impossible |
jnewbery
Jul 13, 2017
Member
nit: there's no such thing as default wallet. Comment should say accessing wallet RPC without using wallet endpoint fails
| + w1.generate(1) | ||
| + | ||
| + #accessing default wallet must be impossible | ||
| + assert_raises(JSONRPCException, self.nodes[0].getwalletinfo) |
jnewbery
Jul 13, 2017
Member
please use the assert_raises_jsonrpc() function to check the error code and message
| + assert_raises(JSONRPCException, self.nodes[0].getwalletinfo) | ||
| + | ||
| + #check w1 wallet balance | ||
| + w1 = self.nodes[0] / "v1/wallet/w1" |
| + walletinfo = w2.getwalletinfo() | ||
| + assert_equal(walletinfo['immature_balance'], 0) | ||
| + | ||
| + w3 = self.nodes[0] / "v1/wallet/w3" |
jnewbery
Jul 13, 2017
Member
seems like w3 isn't really adding any value to this test. Suggest you remove it.
| @@ -46,6 +46,7 @@ std::string HelpMessageCli() | ||
| strUsage += HelpMessageOpt("-rpcpassword=<pw>", _("Password for JSON-RPC connections")); | ||
| strUsage += HelpMessageOpt("-rpcclienttimeout=<n>", strprintf(_("Timeout in seconds during HTTP requests, or 0 for no timeout. (default: %d)"), DEFAULT_HTTP_CLIENT_TIMEOUT)); | ||
| strUsage += HelpMessageOpt("-stdin", _("Read extra arguments from standard input, one per line until EOF/Ctrl-D (recommended for sensitive information such as passphrases)")); | ||
| + strUsage += HelpMessageOpt("-wallet=<walletname>", _("Defines which wallet should be used if a wallet related command gets executed (default: default-wallet if only one present, non-optional if running multiple wallets)")); |
ryanofsky
Jul 13, 2017
Contributor
Guess I missed this. I think -usewallet is worse because it is inconsistent with existing bitcoin -wallet parameter which this mirrors. I also think my suggested documentation #10650 (comment) is better because it keeps this consistency and won't leave the user scratching their head about what "walletname" is supposed to mean:
strUsage += HelpMessageOpt("-wallet=<file>", _("Send RPC for non-default wallet on RPC server (argument is wallet filename in bitcoind directory)"));
jnewbery
Jul 13, 2017
Member
we want -usewallet. If there are multiple -wallet parameters in bitcoin.conf, then I don't think it'll be possible to specify which wallet bitcoin-cli should use.
instagibbs
Jul 13, 2017
Member
further, I get "trapped" if I have a wallet entry in my conf file, since any subsequent -cli call will use that parameter, including stop, etc, and throw Incorrect endpoint used errors. (unless I'm misunderstanding how it's used)
ryanofsky
Jul 13, 2017
Contributor
Thread #10650 (comment).
I see, yeah. I guess -wallet won't work unless bitcoin-cli is changed to explicitly ignore -wallet values that come from the conf file, which might not be a bad idea, but I'm not advocating for.
Interestingly, I guess if if bitcoin-cli is changed to accept -usewallet and support for node calls on the wallet endpoint is added, users will be able to specify a "default wallet" in their configs.
jnewbery
Jul 13, 2017
Member
users will be able to specify a "default wallet" in their configs
shhhh. Don't tell anyone about the secret hidden default wallet feature.
jonasschnelli
and others
added some commits
Jul 13, 2017
|
|
jonasschnelli |
This commit was signed with a verified signature.
jonasschnelli
Jonas Schnelli
|
7506021
|
||
|
|
jonasschnelli |
This commit was signed with a verified signature.
jonasschnelli
Jonas Schnelli
|
f232ef5
|
||
|
|
jonasschnelli |
This commit was signed with a verified signature.
jonasschnelli
Jonas Schnelli
|
555b51d
|
||
|
jnewbery |
This commit was signed with a verified signature.
jonasschnelli
Jonas Schnelli
|
9710df9
|
||
|
|
jonasschnelli |
This commit was signed with a verified signature.
jonasschnelli
Jonas Schnelli
|
19fe32f
|
||
|
|
jonasschnelli |
This commit was signed with a verified signature.
jonasschnelli
Jonas Schnelli
|
759dc37
|
|
|
Changes look great. Manually tested ACK 759dc37 I think this is ready for merge. Thanks for your work on this! |
|
tACK 759dc37 To recap behavior:
nit: The behavior of |
There is no default wallet. There should be no functional difference between wallets when multiple wallets are loaded. |
|
@jnewbery ok let me rephrase: non-endpoint calls will still use that wallet(just like before). |
|
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.
|
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. |
| 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) { |
promag
Jul 14, 2017
Contributor
if (request.URI.compare(0, WALLET_ENDPOINT_BASE.size(), WALLET_ENDPOINT_BASE) == 0)| + // no wallet found with the given endpoint | ||
| + throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Wallet file does not exist or is not loaded"); | ||
| + } | ||
| + else { |
promag
Jul 14, 2017
Contributor
The } else { can be removed since the if blocks always exists the function.
| @@ -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()) { |
promag
Jul 14, 2017
Contributor
evhttp_uridecode works with an empty string, as such this if can be removed.
| + if (decoded) { | ||
| + res = std::string(decoded); | ||
| + free(decoded); | ||
| + } |
jonasschnelli
Jul 14, 2017
Member
I guess there is no need for an else, it will just return an empty string if URLencode failed which is fine IMO.
| @@ -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 != "/") { |
|
@jnewbery My mistake, I probably iterated too fast through options. Can confirm that it gives |
morcos
approved these changes
Jul 14, 2017
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.
| @@ -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()) { |
promag
Jul 14, 2017
Contributor
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));
jonasschnelli
Jul 14, 2017
Member
@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.
|
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:
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 |
|
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 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 |
|
I might have misunderstood what was meant by the 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 |
|
@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... |
ryanofsky
referenced
this pull request
Jul 14, 2017
Closed
Simple, backwards compatible RPC multiwallet support. #10829
|
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). |
laanwj
modified the milestone: 0.16.0, 0.15.0
Jul 17, 2017
jonasschnelli
referenced
this pull request
Jul 17, 2017
Merged
Multiwallet: simplest endpoint support #10849
laanwj
removed
from Blockers in High-priority for review
Jul 17, 2017
laanwj
added a commit
that referenced
this pull request
Jul 18, 2017
|
|
laanwj |
This commit was signed with a verified signature.
laanwj
Wladimir J. van der Laan
|
bde4f93
|
|
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 commentedJun 22, 2017
•
edited
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
walletIDis 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
authproxyis also extended to allow calls likeself.nodes[0].<walletID>.<method>.There is finally also a basic multiwallet RPC test.
Contains #10649.