Multiwallet: add RPC endpoint support #10650

jonasschnelli commented at 7:37 AM on June 22, 2017: contributor

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.

jonasschnelli added the label Wallet on Jun 22, 2017

jonasschnelli force-pushed on Jun 22, 2017

luke-jr commented at 7:56 AM on June 22, 2017: member

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

jonasschnelli commented at 9:30 AM on June 22, 2017: contributor

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>

in src/bitcoin-cli.cpp:241 in 2753ea62b9 outdated

 234 | @@ -235,7 +235,12 @@ UniValue CallRPC(const std::string& strMethod, const UniValue& params)
 235 |      assert(output_buffer);
 236 |      evbuffer_add(output_buffer, strRequest.data(), strRequest.size());
 237 |  
 238 | -    int r = evhttp_make_request(evcon.get(), req.get(), EVHTTP_REQ_POST, "/");
 239 | +    // check if we should use a special wallet endpoint
 240 | +    std::string endpoint = "/";
 241 | +    if (GetArg("-wallet", "") != "") {
 242 | +        endpoint = "/wallet/"+GetArg("-wallet", "");

ryanofsky commented at 10:38 AM on June 22, 2017:

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.

in test/functional/test_framework/authproxy.py:147 in 8775248d06 outdated

 143 | +        # check for a possible wallet endpoint
 144 | +        urlAdd = ""
 145 | +        method = self._service_name
 146 | +        elements = self._service_name.split(".")
 147 | +        if len(elements) > 1:
 148 | +            urlAdd = "/wallet/" + '.'.join(elements[:-1])

ryanofsky commented at 11:25 AM on June 22, 2017:

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 commented at 1:20 PM on July 13, 2017:

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 commented at 1:35 PM on July 13, 2017:

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.

in src/bitcoin-cli.cpp:240 in 2753ea62b9 outdated

 234 | @@ -235,7 +235,12 @@ UniValue CallRPC(const std::string& strMethod, const UniValue& params)
 235 |      assert(output_buffer);
 236 |      evbuffer_add(output_buffer, strRequest.data(), strRequest.size());
 237 |  
 238 | -    int r = evhttp_make_request(evcon.get(), req.get(), EVHTTP_REQ_POST, "/");
 239 | +    // check if we should use a special wallet endpoint
 240 | +    std::string endpoint = "/";
 241 | +    if (GetArg("-wallet", "") != "") {

ryanofsky commented at 11:54 AM on June 22, 2017:

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 commented at 2:27 PM on July 12, 2017:

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

Previous comment not addressed (https://github.com/bitcoin/bitcoin/pull/10650#discussion_r123489861).

ryanofsky commented at 11:58 AM on June 22, 2017: member

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.

laanwj commented at 1:36 PM on June 22, 2017: member

Concept ACK, will review.

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

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 commented at 3:06 PM on June 22, 2017: member

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.

in src/wallet/rpcwallet.cpp:52 in 1fa7d7e26b outdated

  49 | +            if (pwallet->GetWalletID() == requestedWallet) {
  50 | +                return pwallet;
  51 | +            }
  52 | +        }
  53 | +        // no wallet found with the given endpoint
  54 | +        throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet not found ("+SanitizeString(requestedWallet)+")");

ryanofsky commented at 3:31 PM on June 22, 2017:

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 commented at 11:04 AM on July 12, 2017:

In commit "Select wallet based on the given endpoint"

Previous comment not addressed (https://github.com/bitcoin/bitcoin/pull/10650#discussion_r123543935)

ryanofsky commented at 12:57 PM on July 13, 2017:

Thread #10650 (review)

Error string cleanup not (yet) addressed.

gmaxwell commented at 6:42 PM on June 22, 2017: contributor

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 commented at 7:39 PM on June 22, 2017: contributor

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 commented at 8:37 PM on June 22, 2017: member

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 commented at 11:55 PM on June 22, 2017: member

Another multiwallet RPC implemention for discussion: #10653

jonasschnelli force-pushed on Jun 23, 2017

jonasschnelli commented at 8:35 AM on June 23, 2017: contributor

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 commented at 11:06 AM on June 23, 2017: member

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 commented at 11:34 AM on June 23, 2017: contributor

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 commented at 7:41 PM on June 23, 2017: member

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 #10653 and #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 #10650.
Add a wallet option to -rpcauth config strings. This is implemented in #10615.

jnewbery commented at 2:03 PM on June 28, 2017: member

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.

in src/utilstrencodings.cpp:705 in b5088c505f outdated

 701 | @@ -701,3 +702,28 @@ bool ParseFixedPoint(const std::string &val, int decimals, int64_t *amount_out)
 702 |      return true;
 703 |  }
 704 |  
 705 | +std::string urlEncode(const std::string &urlPart) {

promag commented at 1:54 PM on July 4, 2017:

Use evhttp_uriencode()?

in src/utilstrencodings.cpp:726 in b5088c505f outdated

 721 | +    }
 722 | +
 723 | +    return escaped.str();
 724 | +}
 725 | +
 726 | +std::string urlDecode(const std::string &urlEncoded) {

promag commented at 1:54 PM on July 4, 2017:

Use evhttp_uridecode()?

jonasschnelli commented at 3:08 PM on July 4, 2017:

Oh! Very good point. Will do that.

in src/bitcoin-cli.cpp:240 in b5088c505f outdated

 234 | @@ -235,7 +235,13 @@ UniValue CallRPC(const std::string& strMethod, const UniValue& params)
 235 |      assert(output_buffer);
 236 |      evbuffer_add(output_buffer, strRequest.data(), strRequest.size());
 237 |  
 238 | -    int r = evhttp_make_request(evcon.get(), req.get(), EVHTTP_REQ_POST, "/");
 239 | +    // check if we should use a special wallet endpoint
 240 | +    std::string endpoint = "/";

promag commented at 1:58 PM on July 4, 2017:

Use evhttp_uri and it's primitives?

jonasschnelli commented at 1:50 PM on July 7, 2017:

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

in src/wallet/wallet.h:1131 in b5088c505f outdated

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

promag commented at 2:03 PM on July 4, 2017:

const.

jonasschnelli force-pushed on Jul 7, 2017

jonasschnelli commented at 2:44 PM on July 7, 2017: contributor

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>.method may not be the best)
Release notes

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

jonasschnelli added this to the milestone 0.15.0 on Jul 9, 2017

in src/wallet/rpcwallet.cpp:40 in ea785096c0 outdated

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

TheBlueMatt commented at 3:09 PM on July 10, 2017:

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

ryanofsky commented at 2:34 PM on July 12, 2017:

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.

in src/utilstrencodings.cpp:13 in 386c299270 outdated

   9 | @@ -10,6 +10,7 @@
  10 |  #include <cstdlib>
  11 |  #include <cstring>
  12 |  #include <errno.h>
  13 | +#include <iomanip>

TheBlueMatt commented at 3:22 PM on July 10, 2017:

Wait, why?

promag commented at 8:53 PM on July 12, 2017:

🤔

jonasschnelli commented at 11:49 AM on July 13, 2017:

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

in src/httprpc.cpp:229 in 617ce96f11 outdated

 225 | @@ -226,6 +226,11 @@ static bool InitRPCAuthentication()
 226 |      return true;
 227 |  }
 228 |  
 229 | +void RegisterJSONEndpoint(const std::string& endpoint, bool exactMatch)

TheBlueMatt commented at 3:31 PM on July 10, 2017:

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 commented at 10:51 AM on July 12, 2017:

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 commented at 11:58 AM on July 13, 2017:

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.

in src/rpc/blockchain.cpp:1538 in 386c299270 outdated

1553 | -    { "blockchain",         "gettxoutsetinfo",        &gettxoutsetinfo,        true,  {} },
1554 | -    { "blockchain",         "pruneblockchain",        &pruneblockchain,        true,  {"height"} },
1555 | -    { "blockchain",         "verifychain",            &verifychain,            true,  {"checklevel","nblocks"} },
1556 | -
1557 | -    { "blockchain",         "preciousblock",          &preciousblock,          true,  {"blockhash"} },
1558 | +    { "blockchain",         "/v1/node/",  "getblockchaininfo",      &getblockchaininfo,      true,  {} },

TheBlueMatt commented at 3:31 PM on July 10, 2017:

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

TheBlueMatt commented at 3:31 PM on July 10, 2017: member

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

in src/wallet/rpcwallet.cpp:3052 in 177fc1a6b7 outdated

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

ryanofsky commented at 10:57 AM on July 12, 2017:

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

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

in src/wallet/rpcwallet.cpp:40 in 0e7d0c129a outdated

  35 | @@ -36,8 +36,26 @@ static const std::string WALLET_ENDPOINT_BASE = "/v1/wallet/";
  36 |  
  37 |  CWallet *GetWalletForJSONRPCRequest(const JSONRPCRequest& request)
  38 |  {
  39 | -    // TODO: Some way to access secondary wallets
  40 | -    return vpwallets.empty() ? nullptr : vpwallets[0];
  41 | +    LOCK(cs_main); // for now, protect vpwallets via cs_main
  42 | +
  43 | +    if (vpwallets.empty()) {

ryanofsky commented at 11:11 AM on July 12, 2017:

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 commented at 12:56 PM on July 13, 2017:

Thread #10650 (review)

Silently ignoring invalid requestedWallet values not (yet) addressed.

in src/wallet/rpcwallet.cpp:56 in 0e7d0c129a outdated

  54 | +        // no wallet found with the given endpoint
  55 | +        throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet not found ("+SanitizeString(requestedWallet)+")");
  56 | +    }
  57 | +    else {
  58 | +        // default endpoint, use first wallet
  59 | +        return vpwallets[0];

ryanofsky commented at 11:23 AM on July 12, 2017:

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 commented at 12:59 PM on July 13, 2017:

Thread #10650 (review)

Unsafe fallthrough to default wallet not (yet) addressed.

jonasschnelli commented at 3:28 PM on July 13, 2017:

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 commented at 3:40 PM on July 13, 2017:

Thread #10650 (review)

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 (https://github.com/bitcoin/bitcoin/pull/10650#discussion_r126926410).

in src/wallet/wallet.cpp:3796 in 7025861eeb outdated

3792 | @@ -3793,7 +3793,7 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
3793 |      int64_t nStart = GetTimeMillis();
3794 |      bool fFirstRun = true;
3795 |      std::unique_ptr<CWalletDBWrapper> dbw(new CWalletDBWrapper(&bitdb, walletFile));
3796 | -    CWallet *walletInstance = new CWallet(std::move(dbw));
3797 | +    CWallet *walletInstance = new CWallet(std::move(dbw), walletFile); // use filename as walletID for now

ryanofsky commented at 2:20 PM on July 12, 2017:

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.

in src/wallet/rpcwallet.cpp:45 in 0e7d0c129a outdated

  43 | +    if (vpwallets.empty()) {
  44 | +        return nullptr;
  45 | +    }
  46 | +    else if (request.URI.substr(0, WALLET_ENDPOINT_BASE.size()) == WALLET_ENDPOINT_BASE) {
  47 | +        // wallet endpoint was used
  48 | +        std::string requestedWallet = request.URI.substr(WALLET_ENDPOINT_BASE.size());

ryanofsky commented at 2:24 PM on July 12, 2017:

In commit "Select wallet based on the given endpoint"

requestedWallet should be percent decoded here.

ryanofsky commented at 12:56 PM on July 13, 2017:

Thread #10650 (review)

Adding percent decoding not (yet) addressed.

in src/bitcoin-cli.cpp:246 in 73dbe48ced outdated

 242 | +    if (walletName != "") {
 243 |          // always use v1 endpoint if -wallet has been provided
 244 | -        endpoint = "/v1/wallet/"+urlEncode(GetArg("-wallet", ""));
 245 | +        char *encodedURI = evhttp_uriencode(walletName.c_str(), walletName.size(), false);
 246 | +        if (encodedURI) {
 247 | +            endpoint = "/v1/wallet/"+ std::string(encodedURI);

ryanofsky commented at 2:37 PM on July 12, 2017:

In commit "Add urlencode/decode via libevent2"

Memory leak here, need to free encodedURI.

in src/bitcoin-cli.cpp:242 in 73dbe48ced outdated

 236 | @@ -237,9 +237,13 @@ UniValue CallRPC(const std::string& strMethod, const UniValue& params)
 237 |  
 238 |      // check if we should use a special wallet endpoint
 239 |      std::string endpoint = "/";
 240 | -    if (GetArg("-wallet", "") != "") {
 241 | +    std::string walletName = GetArg("-wallet", "");
 242 | +    if (walletName != "") {

ryanofsky commented at 2:38 PM on July 12, 2017:

In commit "Add urlencode/decode via libevent2"

More idiomatic / efficient to write !walletName.empty()

in src/httpserver.cpp:669 in 73dbe48ced outdated

 664 | @@ -665,3 +665,14 @@ void UnregisterHTTPHandler(const std::string &prefix, bool exactMatch)
 665 |      }
 666 |  }
 667 |  
 668 | +std::string urlDecode(const std::string &urlEncoded) {
 669 | +    std::string res = "";

ryanofsky commented at 2:42 PM on July 12, 2017:

In commit "Add urlencode/decode via libevent2"

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

in src/bitcoin-cli.cpp:242 in 73dbe48ced outdated

 236 | @@ -237,9 +237,13 @@ UniValue CallRPC(const std::string& strMethod, const UniValue& params)
 237 |  
 238 |      // check if we should use a special wallet endpoint
 239 |      std::string endpoint = "/";
 240 | -    if (GetArg("-wallet", "") != "") {
 241 | +    std::string walletName = GetArg("-wallet", "");
 242 | +    if (walletName != "") {
 243 |          // always use v1 endpoint if -wallet has been provided
 244 | -        endpoint = "/v1/wallet/"+urlEncode(GetArg("-wallet", ""));

ryanofsky commented at 2:43 PM on July 12, 2017:

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.

in test/functional/test_framework/authproxy.py:147 in 72fee26bc4 outdated

 143 | +        # check for a possible wallet endpoint
 144 | +        urlAdd = ""
 145 | +        method = self._service_name
 146 | +        elements = self._service_name.split(".")
 147 | +        if len(elements) > 1:
 148 | +            urlAdd = "/v1/wallet/" + '.'.join(elements[:-1])

ryanofsky commented at 2:49 PM on July 12, 2017:

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 (review), adding a one-line __idiv__ method.

ryanofsky commented at 1:04 PM on July 13, 2017:

Thread #10650 (review)

Test framework comment not (yet) addressed.

in src/rpc/server.cpp:503 in b62a4bbcb1 outdated

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

ryanofsky commented at 3:30 PM on July 12, 2017:

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 (https://github.com/bitcoin/bitcoin/pull/10650#issuecomment-311669411).

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 commented at 11:26 AM on July 13, 2017:

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 commented at 12:04 PM on July 13, 2017:

Thread #10650 (review)

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 commented at 12:23 PM on July 13, 2017:

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

ryanofsky commented at 12:37 PM on July 13, 2017:

Thread #10650 (review)

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 commented at 2:11 PM on July 13, 2017:

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 commented at 4:59 PM on July 13, 2017:

Thread #10650 (review)

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

in src/wallet/rpcwallet.cpp:2437 in 6fe03854ab outdated

2433 | @@ -2434,6 +2434,7 @@ UniValue getwalletinfo(const JSONRPCRequest& request)
2434 |      UniValue obj(UniValue::VOBJ);
2435 |  
2436 |      size_t kpExternalSize = pwallet->KeypoolCountExternalKeys();
2437 | +    obj.push_back(Pair("walletid", pwallet->GetWalletID()));

ryanofsky commented at 3:35 PM on July 12, 2017:

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.

ryanofsky commented at 4:07 PM on July 12, 2017: member

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 (review)~~~
Make it an error not to explicitly specify wallet name when more than one wallet is loaded, #10650 (review)
~~~Add documentation for new bitcoin-cli -wallet option, #10650 (review), and for new URL scheme.~~~
~~~Fix broken commit history, or consolidate commits.~~~

theuni commented at 6:25 PM on July 12, 2017: member

@jonasschnelli build change looks fine.

sipa commented at 7:18 PM on July 12, 2017: member

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

in src/bitcoin-cli.cpp:251 in 386c299270 outdated

 242 | +    if (walletName != "") {
 243 | +        // always use v1 endpoint if -wallet has been provided
 244 | +        char *encodedURI = evhttp_uriencode(walletName.c_str(), walletName.size(), false);
 245 | +        if (encodedURI) {
 246 | +            endpoint = "/v1/wallet/"+ std::string(encodedURI);
 247 | +        }

promag commented at 8:51 PM on July 12, 2017:

Free encodedURI.

promag commented at 8:52 PM on July 12, 2017:

Else should fail?

jnewbery commented at 9:02 PM on July 12, 2017: member

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.

jonasschnelli force-pushed on Jul 13, 2017

ryanofsky commented at 12:43 PM on July 13, 2017: member

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 https://github.com/bitcoin/bitcoin/compare/master...ryanofsky:pr/multiparam.

Disadvantages of https://github.com/bitcoin/bitcoin/compare/master...ryanofsky:pr/multiparam:

No support for positional arguments. Requires use of named JSONRPC parameters to access multiple wallets.
Adds a few lines of wallet-specific code to src/rpc/server.cpp.
Change will be unnecessary in the future if wallet RPC handling is moved to separate processes (https://github.com/bitcoin/bitcoin/pull/10653#issuecomment-311532389), though this criticism applies to all current changes implementing multiwallet RPC support, including the changes in this PR.

Advantages of https://github.com/bitcoin/bitcoin/compare/master...ryanofsky:pr/multiparam

Extremely simple implementation.
Does not require any changes to bitcoin-cli, python test framework, or any other JSON-RPC client that already supports named arguments.
Compatible with all future new URL schemes, including the one implemented in this PR.
Has test, documentation, and is ready for review and merge into 0.15.0. Unlike this PR, the multiwallet RPC interface is actually documented in bitcoin-cli -help and in RPC help method output`.

jonasschnelli force-pushed on Jul 13, 2017

in src/bitcoin-cli.cpp:241 in 26c259d088 outdated

 234 | @@ -235,7 +235,21 @@ UniValue CallRPC(const std::string& strMethod, const UniValue& params)
 235 |      assert(output_buffer);
 236 |      evbuffer_add(output_buffer, strRequest.data(), strRequest.size());
 237 |  
 238 | -    int r = evhttp_make_request(evcon.get(), req.get(), EVHTTP_REQ_POST, "/");
 239 | +    // check if we should use a special wallet endpoint
 240 | +    std::string endpoint = "/";
 241 | +    std::string walletName = GetArg("-wallet", "");

ryanofsky commented at 1:02 PM on July 13, 2017:

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 (review)

jnewbery commented at 4:12 PM on July 13, 2017:

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.

Expose JSON endpoint registration 858bfcfd6c

jonasschnelli force-pushed on Jul 13, 2017

jonasschnelli commented at 3:58 PM on July 13, 2017: contributor

Rebased and tried to address ~~nits~~points. Added @jnewbery's AuthProxy approach.

in src/rpc/server.cpp:502 in 37258d6bf3 outdated

 496 | @@ -493,7 +497,12 @@ UniValue CRPCTable::execute(const JSONRPCRequest &request) const
 497 |      const CRPCCommand *pcmd = tableRPC[request.strMethod];
 498 |      if (!pcmd)
 499 |          throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Method not found");
 500 | -
 501 | +    if (request.URI != "" && request.URI != "/") {

ryanofsky commented at 4:12 PM on July 13, 2017:

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.

in src/wallet/rpcwallet.cpp:55 in 37258d6bf3 outdated

  50 | +        // no wallet found with the given endpoint
  51 | +        throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Wallet file does not exist or is not loaded");
  52 | +    }
  53 | +    else {
  54 | +        // default endpoint, use first wallet if possible
  55 | +        return vpwallets.empty() ? nullptr : vpwallets[0];

jnewbery commented at 4:21 PM on July 13, 2017:

Please see @ryanofsky's previous comment: #10650 (review)

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.

in src/wallet/rpcwallet.cpp:38 in 37258d6bf3 outdated

  35 | +
  36 |  CWallet *GetWalletForJSONRPCRequest(const JSONRPCRequest& request)
  37 |  {
  38 | -    // TODO: Some way to access secondary wallets
  39 | -    return vpwallets.empty() ? nullptr : vpwallets[0];
  40 | +    LOCK(cs_main); // for now, protect vpwallets via cs_main

ryanofsky commented at 4:22 PM on July 13, 2017:

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.

morcos commented at 5:06 PM on July 13, 2017:

Can we establish whether vpwallets is supposed to be protected by cs_main or not?

in src/wallet/rpcwallet.cpp:43 in 37258d6bf3 outdated

  40 | +    LOCK(cs_main); // for now, protect vpwallets via cs_main
  41 | +
  42 | +    if (request.URI.substr(0, WALLET_ENDPOINT_BASE.size()) == WALLET_ENDPOINT_BASE) {
  43 | +        // wallet endpoint was used
  44 | +        std::string requestedWallet = urlDecode(request.URI.substr(WALLET_ENDPOINT_BASE.size()));
  45 | +        for(CWallet* pwallet : vpwallets) {

ryanofsky commented at 4:23 PM on July 13, 2017:

In commit "Select wallet based on the given endpoint"

Maybe use Luke's CWalletRef syntax to be consistent & portable.

in test/functional/test_framework/authproxy.py:196 in 37258d6bf3 outdated

 190 | @@ -191,3 +191,6 @@ def _get_response(self):
 191 |          else:
 192 |              log.debug("<-- [%.6f] %s"%(elapsed,responsedata))
 193 |          return response
 194 | +
 195 | +    def __truediv__(self, relative_uri):
 196 | +        return AuthServiceProxy("{}{}".format(self.__service_url, relative_uri), self._service_name, connection=self.__conn)

jnewbery commented at 4:23 PM on July 13, 2017:

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 commented at 5:11 PM on July 13, 2017:

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

in test/functional/multiwallet.py:38 in 37258d6bf3 outdated

  33 | +        w2 = self.nodes[0] / "/v1/wallet/w2"
  34 | +        walletinfo = w2.getwalletinfo()
  35 | +        assert_equal(walletinfo['immature_balance'], 0)
  36 | +
  37 | +        w3 = self.nodes[0] / "/v1/wallet/w2"
  38 | +        walletinfo = w2.getwalletinfo()

jnewbery commented at 4:25 PM on July 13, 2017:

the w3 assignment is wrong, and this line doesn't appear to be testing anything

jonasschnelli commented at 4:32 PM on July 13, 2017:

Fixed.

jonasschnelli force-pushed on Jul 13, 2017

in test/functional/multiwallet.py:21 in 37258d6bf3 outdated

  16 | +
  17 | +    def setup_network(self):
  18 | +        self.nodes = self.start_nodes(1, self.options.tmpdir, self.extra_args[:1])
  19 | +
  20 | +    def run_test(self):
  21 | +        self.nodes[0].generate(1)

jnewbery commented at 4:32 PM on July 13, 2017:

generate is a wallet call, so should specify a wallet endpoint.

jonasschnelli commented at 4:34 PM on July 13, 2017:

Fixed.

jonasschnelli commented at 4:33 PM on July 13, 2017: contributor

Also added the bitcoin-cli help string for -wallet.

in src/rpc/server.cpp:284 in 37258d6bf3 outdated

 280 | @@ -280,9 +281,9 @@ static const CRPCCommand vRPCCommands[] =
 281 |  { //  category              name                      actor (function)         okSafe argNames
 282 |    //  --------------------- ------------------------  -----------------------  ------ ----------
 283 |      /* Overall control/query calls */
 284 | -    { "control",            "help",                   &help,                   true,  {"command"}  },
 285 | -    { "control",            "stop",                   &stop,                   true,  {}  },
 286 | -    { "control",            "uptime",                 &uptime,                 true,  {}  },
 287 | +    { "control",            "/v1/*"    , "help",                   &help,                   true,  {"command"}  },

jnewbery commented at 4:34 PM on July 13, 2017:

it's a bit strange that the help RPC returns help for non-wallet commands when called on a wallet endpoint

jonasschnelli force-pushed on Jul 13, 2017

in src/rpc/server.cpp:315 in 37258d6bf3 outdated

 310 | @@ -310,6 +311,9 @@ bool CRPCTable::appendCommand(const std::string& name, const CRPCCommand* pcmd)
 311 |      if (IsRPCRunning())
 312 |          return false;
 313 |  
 314 | +    // make sure we register the command endpoint
 315 | +    RegisterJSONEndpoint(pcmd->endpoint, false);

ryanofsky commented at 4:46 PM on July 13, 2017:

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.

morcos commented at 4:52 PM on July 13, 2017:

nit: This seems pretty inefficient since so many of the commands have the same endpoint, they'll be registered over and over.

in src/rpc/rawtransaction.cpp:908 in 37258d6bf3 outdated

 914 | +    { "rawtransactions",    "/v1/node/",  "getrawtransaction",      &getrawtransaction,      true,       {"txid","verbose"} },
 915 | +    { "rawtransactions",    "/v1/node/",  "createrawtransaction",   &createrawtransaction,   true,       {"inputs","outputs","locktime","replaceable"} },
 916 | +    { "rawtransactions",    "/v1/node/",  "decoderawtransaction",   &decoderawtransaction,   true,       {"hexstring"} },
 917 | +    { "rawtransactions",    "/v1/node/",  "decodescript",           &decodescript,           true,       {"hexstring"} },
 918 | +    { "rawtransactions",    "/v1/node/",  "sendrawtransaction",     &sendrawtransaction,     false,      {"hexstring","allowhighfees"} },
 919 | +    { "rawtransactions",    "/v1/*",      "signrawtransaction",     &signrawtransaction,     false,      {"hexstring","prevtxs","privkeys","sighashtype"} }, /* uses wallet if enabled */

jnewbery commented at 4:59 PM on July 13, 2017:

Until wallet and server RPCs are fully separated, the following RPCs should also allow wildcard endpoints:

validateaddress
createmultisig

We can leave getinfo since it's deprecated.

ryanofsky commented at 7:50 PM on July 13, 2017:

Thread #10650 (review)

Wildcard endpoints for wallet-optional rpc not yet addressed.

jonasschnelli commented at 8:27 PM on July 13, 2017:

Just fixed.

in src/bitcoin-cli.cpp:49 in 619a298759 outdated

  45 | @@ -46,6 +46,7 @@ std::string HelpMessageCli()
  46 |      strUsage += HelpMessageOpt("-rpcpassword=<pw>", _("Password for JSON-RPC connections"));
  47 |      strUsage += HelpMessageOpt("-rpcclienttimeout=<n>", strprintf(_("Timeout in seconds during HTTP requests, or 0 for no timeout. (default: %d)"), DEFAULT_HTTP_CLIENT_TIMEOUT));
  48 |      strUsage += HelpMessageOpt("-stdin", _("Read extra arguments from standard input, one per line until EOF/Ctrl-D (recommended for sensitive information such as passphrases)"));
  49 | +    strUsage += HelpMessageOpt("-wallet=<walletname>", _("Use wallet endpoint for given <walletname>"));

morcos commented at 4:59 PM on July 13, 2017:

nit: Could be more descriptive

jnewbery changes_requested

jnewbery commented at 5:03 PM on July 13, 2017: member

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.

in src/bitcoin-cli.cpp:243 in 619a298759 outdated

 235 | @@ -235,7 +236,13 @@ UniValue CallRPC(const std::string& strMethod, const UniValue& params)
 236 |      assert(output_buffer);
 237 |      evbuffer_add(output_buffer, strRequest.data(), strRequest.size());
 238 |  
 239 | -    int r = evhttp_make_request(evcon.get(), req.get(), EVHTTP_REQ_POST, "/");
 240 | +    // check if we should use a special wallet endpoint
 241 | +    std::string endpoint = "/";
 242 | +    if (GetArg("-wallet", "") != "") {
 243 | +        // always use v1 endpoint if -wallet has been provided
 244 | +        endpoint = "/v1/wallet/"+urlEncode(GetArg("-wallet", ""));

morcos commented at 5:05 PM on July 13, 2017:

urlEncode doesn't exist. I think if you just squashed 6a99141 (the following commit) into this one, you'd be ok.

ryanofsky commented at 5:05 PM on July 13, 2017:

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.

in src/bitcoin-cli.cpp:243 in 6a99141a41 outdated

 237 | @@ -238,9 +238,17 @@ UniValue CallRPC(const std::string& strMethod, const UniValue& params)
 238 |  
 239 |      // check if we should use a special wallet endpoint
 240 |      std::string endpoint = "/";
 241 | -    if (GetArg("-wallet", "") != "") {
 242 | +    std::string walletName = GetArg("-wallet", "");
 243 | +    if (walletName != "") {
 244 |          // always use v1 endpoint if -wallet has been provided
 245 | -        endpoint = "/v1/wallet/"+urlEncode(GetArg("-wallet", ""));

ryanofsky commented at 5:07 PM on July 13, 2017:

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.

in test/functional/multiwallet.py:25 in edc0668ea8 outdated

  20 | +    def run_test(self):
  21 | +        w1 = self.nodes[0] / "/v1/wallet/w1"
  22 | +        w1.generate(1)
  23 | +
  24 | +        #check default wallet balance
  25 | +        walletinfo = self.nodes[0].getwalletinfo()

morcos commented at 5:13 PM on July 13, 2017:

This should break if you have multiple wallets defined

morcos changes_requested

morcos commented at 5:15 PM on July 13, 2017: member

Getting close

laanwj added this to the "Blockers" column in a project

jonasschnelli force-pushed on Jul 13, 2017

jonasschnelli commented at 7:27 PM on July 13, 2017: contributor

Fixed reported points:

Disabled default wallet selecting when multiple wallets are present
Avoid registering multiple of the same endpoints
Removed cs_main lock for vpwallets (lock can be added once we can load wallets outside of init)
Fixed AuthProxy endpoint "/" problem

in test/functional/multiwallet.py:28 in 741b8a6d1e outdated

  23 | +
  24 | +        #accessing default wallet must be impossible
  25 | +        assert_raises(JSONRPCException, self.nodes[0].getwalletinfo)
  26 | +
  27 | +        #check w1 wallet balance
  28 | +        w1 = self.nodes[0] / "v1/wallet/w1"

jnewbery commented at 7:39 PM on July 13, 2017:

duplicate. Not required

in test/functional/multiwallet.py:33 in 741b8a6d1e outdated

  32 | +        #check w1 wallet balance
  33 | +        w2 = self.nodes[0] / "v1/wallet/w2"
  34 | +        walletinfo = w2.getwalletinfo()
  35 | +        assert_equal(walletinfo['immature_balance'], 0)
  36 | +
  37 | +        w3 = self.nodes[0] / "v1/wallet/w3"

jnewbery commented at 7:39 PM on July 13, 2017:

seems like w3 isn't really adding any value to this test. Suggest you remove it.

in test/functional/multiwallet.py:5 in 741b8a6d1e outdated

   0 | @@ -0,0 +1,49 @@
   1 | +#!/usr/bin/env python3
   2 | +# Copyright (c) 2017 The Bitcoin Core developers
   3 | +# Distributed under the MIT software license, see the accompanying
   4 | +# file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 | +"""Test the wallet."""

jnewbery commented at 7:41 PM on July 13, 2017:

nit: please fix docstring

in test/functional/multiwallet.py:17 in 741b8a6d1e outdated

  12 | +        super().__init__()
  13 | +        self.setup_clean_chain = True
  14 | +        self.num_nodes = 1
  15 | +        self.extra_args = [['-wallet=w1', '-wallet=w2','-wallet=w3']]
  16 | +
  17 | +    def setup_network(self):

jnewbery commented at 7:42 PM on July 13, 2017:

This method override is not required (it's doing the same thing as the base method would do)

in test/functional/multiwallet.py:24 in 741b8a6d1e outdated

  19 | +
  20 | +    def run_test(self):
  21 | +        w1 = self.nodes[0] / "v1/wallet/w1"
  22 | +        w1.generate(1)
  23 | +
  24 | +        #accessing default wallet must be impossible

jnewbery commented at 7:43 PM on July 13, 2017:

nit: there's no such thing as default wallet. Comment should say accessing wallet RPC without using wallet endpoint fails

in src/httpserver.cpp:158 in 2a9b15a7c8 outdated

 154 | @@ -155,6 +155,10 @@ struct HTTPPathHandler
 155 |          prefix(_prefix), exactMatch(_exactMatch), handler(_handler)
 156 |      {
 157 |      }
 158 | +    friend bool operator==(const HTTPPathHandler& a, const HTTPPathHandler& b)

ryanofsky commented at 7:45 PM on July 13, 2017:

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.

jonasschnelli commented at 8:24 PM on July 13, 2017:

Good point. Will fix.

in src/httpserver.cpp:656 in 2a9b15a7c8 outdated

 651 | @@ -648,7 +652,11 @@ HTTPRequest::RequestMethod HTTPRequest::GetRequestMethod()
 652 |  void RegisterHTTPHandler(const std::string &prefix, bool exactMatch, const HTTPRequestHandler &handler)
 653 |  {
 654 |      LogPrint(BCLog::HTTP, "Registering HTTP handler for %s (exactmatch %d)\n", prefix, exactMatch);
 655 | -    pathHandlers.push_back(HTTPPathHandler(prefix, exactMatch, handler));
 656 | +    HTTPPathHandler pathHandler(prefix, exactMatch, handler);
 657 | +    if (std::find(pathHandlers.begin(), pathHandlers.end(), pathHandler) == pathHandlers.end()) {

ryanofsky commented at 7:47 PM on July 13, 2017:

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 commented at 8:24 PM on July 13, 2017:

I think that is something we can change later. Or do you think its crucial?

ryanofsky commented at 8:38 PM on July 13, 2017:

Thread #10650 (review)

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

in src/bitcoin-cli.cpp:49 in 741b8a6d1e outdated

  45 | @@ -46,6 +46,7 @@ std::string HelpMessageCli()
  46 |      strUsage += HelpMessageOpt("-rpcpassword=<pw>", _("Password for JSON-RPC connections"));
  47 |      strUsage += HelpMessageOpt("-rpcclienttimeout=<n>", strprintf(_("Timeout in seconds during HTTP requests, or 0 for no timeout. (default: %d)"), DEFAULT_HTTP_CLIENT_TIMEOUT));
  48 |      strUsage += HelpMessageOpt("-stdin", _("Read extra arguments from standard input, one per line until EOF/Ctrl-D (recommended for sensitive information such as passphrases)"));
  49 | +    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)"));

jnewbery commented at 7:50 PM on July 13, 2017:

As discussed in IRC, -usewallet might be better here

ryanofsky commented at 9:00 PM on July 13, 2017:

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 (review) 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 commented at 9:14 PM on July 13, 2017:

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 commented at 9:18 PM on July 13, 2017:

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 commented at 9:25 PM on July 13, 2017:

Thread #10650 (review).

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 commented at 9:30 PM on July 13, 2017:

users will be able to specify a "default wallet" in their configs

shhhh. Don't tell anyone about the secret hidden default wallet feature.

in src/wallet/rpcwallet.cpp:50 in 1177da005e outdated

  47 | +        }
  48 | +        // no wallet found with the given endpoint
  49 | +        throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Wallet file does not exist or is not loaded");
  50 | +    }
  51 | +    else {
  52 | +        if (vpwallets.size() > 1) {

ryanofsky commented at 7:55 PM on July 13, 2017:

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.

in src/wallet/rpcwallet.cpp:52 in 1177da005e outdated

  49 | +        throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Wallet file does not exist or is not loaded");
  50 | +    }
  51 | +    else {
  52 | +        if (vpwallets.size() > 1) {
  53 | +            // don't allow to use a default wallet in multiwallet
  54 | +            throw JSONRPCError(RPC_WALLET_ERROR, "No wallet was defined but multiple wallets are present");

ryanofsky commented at 7:57 PM on July 13, 2017:

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

instagibbs commented at 8:00 PM on July 13, 2017: member

FAIL! : RPCNestedTests::rpcNestedTests() Caught unhandled exception in travis

ryanofsky commented at 8:07 PM on July 13, 2017: member

utACK 741b8a6d1e4e86473b06c60a004393d15ed9f846 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).

jonasschnelli force-pushed on Jul 13, 2017

jonasschnelli commented at 8:09 PM on July 13, 2017: contributor

Fixed the travis error. We indeed need to exclude the empty endpoint "" from endpoint enforcement. The GUI uses that. So reverted that part in server.cpp.

Split node / wallet RPC calls based on the endpoint 6e390d1c82

jonasschnelli force-pushed on Jul 13, 2017

jonasschnelli commented at 8:28 PM on July 13, 2017: contributor

Fixed the std::find / == operator issue reported by @ryanofsky.
Switch validateaddress, createmultisig to the wildcard v1 endpoint /v1/*.

in test/functional/multiwallet.py:25 in d350a16cee outdated

  20 | +    def run_test(self):
  21 | +        w1 = self.nodes[0] / "v1/wallet/w1"
  22 | +        w1.generate(1)
  23 | +
  24 | +        #accessing default wallet must be impossible
  25 | +        assert_raises(JSONRPCException, self.nodes[0].getwalletinfo)

jnewbery commented at 8:49 PM on July 13, 2017:

please use the assert_raises_jsonrpc() function to check the error code and message

jnewbery commented at 8:50 PM on July 13, 2017: member

Looking really good now. Just a few more nits!

jonasschnelli force-pushed on Jul 14, 2017

Add wallet endpoint support to bitcoin-cli (-usewallet) 7506021afa

Add urlencode/decode via libevent2 f232ef53ec

Select wallet based on the given endpoint 19fe32fc54

Fix test_bitcoin circular dependency issue 555b51d2e7

[tests] [wallet] Add wallet endpoint support to authproxy 9710df955d

[QA] add basic multiwallet test 759dc37f3d

jonasschnelli force-pushed on Jul 14, 2017

jonasschnelli commented at 7:55 AM on July 14, 2017: contributor

Changed bitcoin-cli's parameter to -usewallet
Fixed @jnewbery nits on the RPC test

jnewbery commented at 2:29 PM on July 14, 2017: member

Changes look great. Manually tested ACK https://github.com/bitcoin/bitcoin/pull/10650/commits/759dc37f3de53f660c73650c0678f1ee6544490b

I think this is ready for merge.

Thanks for your work on this!

instagibbs commented at 2:57 PM on July 14, 2017: member

tACK https://github.com/bitcoin/bitcoin/pull/10650/commits/759dc37f3de53f660c73650c0678f1ee6544490b

To recap behavior: ~~- first -wallet is used when no endpoint is given~~

-wallet is ignored -cli side
-usewallet correctly complains when file isn't loaded
empty -usewallet allows non-wallet functionality calls, ala bitcoin-cli -usewallet="" getinfo

nit: The behavior of -usewallet="" is inconsistent when used with wallet functionality. Gives Method not found (disabled) error, which isn't very clear. This can be fixed later...

in src/wallet/rpcwallet.cpp:38 in 759dc37f3d

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

promag commented at 3:04 PM on July 14, 2017:

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

jnewbery commented at 3:06 PM on July 14, 2017: member

first -wallet is "default wallet"

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

in src/wallet/rpcwallet.cpp:49 in 759dc37f3d

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

promag commented at 3:08 PM on July 14, 2017:

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

instagibbs commented at 3:08 PM on July 14, 2017: member

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

in src/httpserver.cpp:679 in 759dc37f3d

 674 | +    if (!urlEncoded.empty()) {
 675 | +        char *decoded = evhttp_uridecode(urlEncoded.c_str(), false, NULL);
 676 | +        if (decoded) {
 677 | +            res = std::string(decoded);
 678 | +            free(decoded);
 679 | +        }

promag commented at 3:12 PM on July 14, 2017:

} else {?

jonasschnelli commented at 6:16 PM on July 14, 2017:

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

in src/httpserver.cpp:674 in 759dc37f3d

 668 | @@ -665,3 +669,14 @@ void UnregisterHTTPHandler(const std::string &prefix, bool exactMatch)
 669 |      }
 670 |  }
 671 |  
 672 | +std::string urlDecode(const std::string &urlEncoded) {
 673 | +    std::string res;
 674 | +    if (!urlEncoded.empty()) {

promag commented at 3:17 PM on July 14, 2017:

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

in src/rpc/server.cpp:502 in 759dc37f3d

 497 | @@ -494,6 +498,13 @@ UniValue CRPCTable::execute(const JSONRPCRequest &request) const
 498 |      if (!pcmd)
 499 |          throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Method not found");
 500 |  
 501 | +    // enfore endpoint only for non / and "" endpoint (the later is used by the GUI)
 502 | +    if (request.URI != "" && request.URI != "/") {

promag commented at 3:19 PM on July 14, 2017:

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

ryanofsky commented at 3:22 PM on July 14, 2017: member

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 commented at 3:23 PM on July 14, 2017: member

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.

in src/httpserver.cpp:652 in 759dc37f3d

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

morcos commented at 3:29 PM on July 14, 2017:

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

promag commented at 4:04 PM on July 14, 2017:

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

promag commented at 4:05 PM on July 14, 2017:

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

jonasschnelli commented at 6:20 PM on July 14, 2017:

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

instagibbs commented at 3:39 PM on July 14, 2017: member

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

morcos commented at 3:43 PM on July 14, 2017: member

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.

ryanofsky commented at 5:03 PM on July 14, 2017: member

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 https://github.com/bitcoin/bitcoin/compare/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 https://github.com/bitcoin/bitcoin/compare/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 https://github.com/bitcoin/bitcoin/compare/master...ryanofsky:pr/multiparam is a much simpler and saner way to roll out multiwallet support.

jnewbery commented at 5:56 PM on July 14, 2017: member

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 commented at 6:01 PM on July 14, 2017: member

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 commented at 6:11 PM on July 14, 2017: member

-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 commented at 6:22 PM on July 14, 2017: contributor

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 commented at 6:54 PM on July 14, 2017: member

-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 commented at 7:43 PM on July 14, 2017: member

@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 commented at 7:57 PM on July 14, 2017: member

@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 commented at 8:00 PM on July 14, 2017: member

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 commented at 7:17 AM on July 17, 2017: member

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 added this to the milestone 0.16.0 on Jul 17, 2017

laanwj removed this from the milestone 0.15.0 on Jul 17, 2017

laanwj removed this from the "Blockers" column in a project

laanwj referenced this in commit bde4f937ae on Jul 18, 2017

ryanofsky commented at 4:02 PM on July 25, 2017: member

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 commented at 10:50 PM on September 5, 2017: contributor

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

jonasschnelli closed this on Sep 5, 2017

PastaPastaPasta referenced this in commit fdf34ff655 on Aug 2, 2019

PastaPastaPasta referenced this in commit c796551a70 on Aug 6, 2019

barrystyle referenced this in commit f2f1b9ff58 on Jan 22, 2020

DrahtBot locked this on Sep 8, 2021