Multiwallet: simplest endpoint support

jonasschnelli commented at 10:14 am on July 17, 2017: contributor

Alternative for #10829 and #10650. It adds the most simplest form of wallet based endpoint support (/wallet/<filename>). No v1 and no node/wallet endpoint split.

Register wallet endpoint dd2185c291

jonasschnelli added the label Wallet on Jul 17, 2017

laanwj commented at 10:55 am on July 17, 2017: member

Looks good to me too, utACK https://github.com/bitcoin/bitcoin/pull/10849/commits/c8115e31325312061682bf966b4475359cbc5215.

laanwj added this to the milestone 0.15.0 on Jul 17, 2017

ryanofsky commented at 11:37 am on July 17, 2017: member

This has no documentation. If we think this approach is a good idea, I would like to see in what way it can be explained to users. Is there going to be a doc/JSPON-RPC-interface.md to complement https://github.com/bitcoin/bitcoin/blob/master/doc/REST-interface.md? Will endpoint be considered experimental (unchanging semantics but possibly temporary) or unstable (possibly changing semantics and possibly temporary)?

Also, from the other issue I think something like -rpcwallet=filename would be better than -usewallet=filename. It’s hard to see how “use” could convey any meaning when you would expect any command line option that you specify to be used.

And, also from other issue I think ‘?wallet=filename makes more sense than /wallet/filename as more extensible and conventional way of passing options (as opposed to required arguments) in urls.

laanwj commented at 11:42 am on July 17, 2017: member

And, also from other issue I think ‘?wallet=filename makes more sense than /wallet/filename as more extensible and conventional way of passing options (as opposed to required arguments) in urls.

Definitely no! Let’s not repeat this: JSON-RPC uses POST only, and with POST it is weird to have ?a=b URL-encoded arguments in the URL as well. It also implies more arguments could be added later, which is not true. This is only to select the wallet, which is inherently hierarchical. (also e.g. ?a=b syntax isn’t deterministic, it’s possible to add arguments that are ignored, change the order of the arguments, etc, whereas a /wallet/<name>/ URL is simply a fixed thing)

ryanofsky commented at 11:47 am on July 17, 2017: member

POST it is weird to have ?a=b URL-encoded arguments in the URL as well

Don’t know your source for this but this is done all the time.

It also implies more arguments could be added later, which is not true.

I don’t know why it wouldn’t be true. I can think of lots of RPC options you might want to add outside the RPC request: timeouts, completions handlers, etc.

?a=b syntax isn’t deterministic, it’s possible to add arguments that are ignored, change the order of the arguments, etc, whereas a /wallet// URL is simply a fixed thing

Not true, you don’t have to ignore any option, but it is good practice to accept options in any order. This is one of the reasons people choose to used named options instead of positional arguments.

laanwj commented at 11:49 am on July 17, 2017: member

I don’t know why it it wouldn’t be true, but ok. I can think of lots of RPC options you might want to add outside the RPC request: timeouts, completions handlers, etc.

Those really shouldn’t be passed in the URL. The URL locates something, such as the wallet in this case.

ryanofsky commented at 11:52 am on July 17, 2017: member

Those really shouldn’t be passed in the URL. The URL locates something, such as the wallet in this case.

They shouldn’t be part of the uri-path, but you would have to explain more why they shouldn’t be options in a query string…

gmaxwell commented at 11:56 am on July 17, 2017: contributor

ryanofsky: Among other reasons the path approach will already work with most but not all software, e.g. you change your RPC endpoint url to be one with the path, and tada– done, no source code modifications in some things. I believe a query string would not achieve this, and in some cases require deep spelunking.

I could see an argument that something like a timeout might make sense as a query like parameter, but a wallet seems like a pretty perfect ’navigation’ like element.

laanwj commented at 12:15 pm on July 17, 2017: member

Is there going to be a doc/JSPON-RPC-interface.md to complement https://github.com/bitcoin/bitcoin/blob/master/doc/REST-interface.md?

And yes, there should be. Documentation is good. But that’s not a requirement for the feature freeze. This is an experimental API, it could be changed completely for next release, so not having it documented 100% on first release is not a problem. (we should make an attempt in the release notes, of course)

ryanofsky commented at 12:17 pm on July 17, 2017: member

you change your RPC endpoint url to be one with the patch, and tada– done

This is true for this PR but not #10650 which is a big reason why I think this PR is better and less user-hostile than #10650. @laanwj, my bigger point is about extensibility . We want to add a wallet option right now. Great, so that can be as simple as adding ?wallet=filename to the query string to provide the nice user benefit Greg cites. And maybe you have a problem with timeouts or completion handlers, but lets say we want to add a ?network=name parameter in the future (maybe to add extra way to specify that an operation is done on mainnet or testnet, or in some dystopia where there are multiple chains, but no need to go there…). The point is you want to be able to add some new option. With query strings you have a standard way to do this. ?wallet=filename&network=name&timeout=30s works just as well as ?timeout=30s&wallet=filename&network=name as established by long-running convention.

If we use paths instead, are we requiring /wallet/filename/network/name or /network/name/filename or /wallet/filename?network=name, or something else completely arbitrary based on historical accident? Or because a filename is a “location” but a network name isn’t?

I guess another thing that is motivating me is that I would like to see less uri-path space annexed for this one multiwallet feature. Right now json-rpc calls go directly to / handler and REST calls go directly to /rest handler. Everything else right now (but not with these endpoint prs) is wide open right now. I’d be happier if more of the path space could be free for other applications like a web interface. If these prs added stuff under an /rpc path, that would also seem like an improvement to me.

Also, so my other points doesn’t get lost above, it would be great to see something done on documentation and -usewallet if we are taking this endpoint approach. Even if there is not “100% documentation” I think it would be very helpful to see uri-path endpoints explained from user point of view.

gmaxwell commented at 12:24 pm on July 17, 2017: contributor

I’d be happier if more of the path space could be free for other applications like a web interface. If all this stuff was moved under an /rpc path

It’s under /wallet/ at least, so I think it won’t have any clashes with other use.

As far as usewallet– I think we intend that argument to eventually specify all wallets that get loaded, not just specific to rpc. e.g. it would also tell it which ones to load in the UI when that support is complete.

ryanofsky commented at 12:39 pm on July 17, 2017: member

It’s under /wallet/ at least, so I think it won’t have any clashes with other use.

I could easily imagine a web interface wanting to handle /wallet path. The point is that if RPCs are restricted to exactly one path / (current case) or two paths / and /rpc then it’s then it’s clean and easy to do anything else with the rest of the space. If there’s a bigger changing list of RPC paths, that would seem to make things messier.

As far as usewallet– I think we intend that argument to eventually specify all wallets that get loaded, not just specific to rpc. e.g. it would also tell it which ones to load in the UI when that support is complete.

That’s interesting. I thought the UI was would just have tabs or a dropdown to choose between any of the loaded wallets. But I guess the advantage of having the bitcoin-qt executable accept a -usewallet=filename instead of a more descriptive -showwallet=filename or -onlyshowwallet=filename would be that you can specify -usewallet in your bitcoind.conf and have it picked up by both bitcoin-cli and bitcoin-qt.

in src/bitcoin-cli.cpp:246 in c8115e3132 outdated

241@@ -241,7 +242,20 @@ UniValue CallRPC(const std::string& strMethod, const UniValue& params)
242     assert(output_buffer);
243     evbuffer_add(output_buffer, strRequest.data(), strRequest.size());
244 
245-    int r = evhttp_make_request(evcon.get(), req.get(), EVHTTP_REQ_POST, "/");
246+    // check if we should use a special wallet endpoint
247+    std::string endpoint;

jnewbery commented at 2:33 pm on July 17, 2017:

bug: if no wallet is specified then the endpoint will be "" instead of “/”

result: bitcoin-cli doesn’t work without a -usewallet parameter:

0→ bitcoin-cli getinfo
1error: no response from server

Fix is:

0    std::string endpoint = "/";

jnewbery commented at 3:25 pm on July 17, 2017: member

It’s a shame that this PR doesn’t prevent individual wallet endpoints from accessing server-level RPCs. However that can be added at a later date.

I have a branch here: https://github.com/jnewbery/bitcoin/tree/pr10849 that does a couple of things:

fixes #10849 (review) (in commit bugfix - allow bitcoin-cli to work without -wallet parameter)
prevents bitcoin-cli from reading -wallet arguments from the conf file and removes the -usewallet argument. That means that to run a command on a wallet in multiwallet mode, the bitcoin-cli user must specify -wallet on the command line.

Feel free to squash the first commit and cherry-pick the next two if you think they’re useful.

Add wallet endpoint support to bitcoin-cli (-usewallet) 31e07203bd

Fix test_bitcoin circular dependency issue 32c9710c50

Select wallet based on the given endpoint 76603b1325

[tests] [wallet] Add wallet endpoint support to authproxy 979d0b8a65

[QA] add basic multiwallet test 6b9faf7470

jonasschnelli force-pushed on Jul 17, 2017

jonasschnelli commented at 3:43 pm on July 17, 2017: contributor

Force push fixed the endpoint issue (thanks @jnewbery). I think we should keep it minimal with a hope this can slip in before 0.15 freeze.

Fixes can be done later.

jnewbery commented at 4:05 pm on July 17, 2017: member

Fixes can be done later.

Agree. Multiwallet interface should be considered unstable for v0.15.

achow101 commented at 6:30 pm on July 17, 2017: member

So this will remove the concept of “default wallet” from the RPCs? Any attempt to use a wallet RPC without specifying a wallet will fail, right?

jonasschnelli commented at 6:36 pm on July 17, 2017: contributor

@achow101: only if you call a wallet RPC and if you run with multiple wallets.

achow101 commented at 6:39 pm on July 17, 2017: member

@jonasschnelli Ok, cool!

utACK 6b9faf747003995417d6a66fad64d2537c371092

in test/functional/multiwallet.py:15 in 6b9faf7470

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

theuni commented at 11:01 pm on July 17, 2017:

Could you change one of these to just ‘w’ to catch possible length extension bugs? If we’re asking for w2, we want to make sure that we don’t get w. on accident.

theuni commented at 0:21 am on July 18, 2017: member

Code review utACK 6b9faf747003995417d6a66fad64d2537c371092. No opinion on the endpoint discussion.

in src/bitcoin-cli.cpp:254 in 31e07203bd outdated

250+        char *encodedURI = evhttp_uriencode(walletName.c_str(), walletName.size(), false);
251+        if (encodedURI) {
252+            endpoint = "/wallet/"+ std::string(encodedURI);
253+            free(encodedURI);
254+        }
255+        else {

sipa commented at 1:58 am on July 18, 2017:

Nit: else on the same line as ‘}’

sipa commented at 2:08 am on July 18, 2017: member

utACK 6b9faf747003995417d6a66fad64d2537c371092

I like this endpoints based approach, as it’s simple for clients to adopt (just make the URI configurable, which immediately prepares it for future changes that move things to other hosts/ports), and does not force adopting named arguments.

As for not introducing ‘v1’ or separating off node calls, I think this is fine. Once we do introduce a new API, it’ll need more thought about which calls are available where (ideally, reasonable clients only need access to one URI).

meshcollider commented at 2:59 am on July 18, 2017: contributor

utACK https://github.com/bitcoin/bitcoin/commit/6b9faf747003995417d6a66fad64d2537c371092 looks good to me

morcos commented at 1:36 pm on July 18, 2017: member

utACK 6b9faf7

However, this badly needs documentation for the user.

Also I am not happy with the fact that passing multiple -usewallet arguments to the commandline results in the last wallet getting used, that should be an error.

laanwj merged this on Jul 18, 2017

laanwj closed this on Jul 18, 2017

laanwj referenced this in commit bde4f937ae on Jul 18, 2017

jnewbery commented at 4:07 pm on July 18, 2017: member

@morcos - does #10868 (for merge after v0.15) resolve your issue with -usewallet?

Suggestions for where to put the documentation? I’m happy to work on that.

NicolasDorier commented at 4:23 pm on September 2, 2017: contributor

This is incompatible with RPC batching. :( (you can’t batch a /wallets/ request with one which does not use it)

NicolasDorier commented at 5:06 pm on September 2, 2017: contributor

Ok disregard my comment. I thought that adding /wallet/w1 to a non wallet RPC method would fail. This is not the case, so no issue!

in test/functional/multiwallet.py:28 in 6b9faf7470

23+
24+        #check w1 wallet balance
25+        walletinfo = w1.getwalletinfo()
26+        assert_equal(walletinfo['immature_balance'], 50)
27+
28+        #check w1 wallet balance

MarcoFalke commented at 8:17 pm on November 10, 2017:

nit: w2

MarcoFalke commented at 8:18 pm on November 10, 2017: member

Post-merge utACK 6b9faf7

MarcoFalke referenced this in commit 3d6ad40777 on Nov 22, 2017

ftrader referenced this in commit 23870542d3 on Feb 5, 2018

zkbot referenced this in commit 0d9fbb02d6 on Aug 4, 2018

zkbot referenced this in commit baae90489b on Aug 4, 2018

zkbot referenced this in commit 29de8c8456 on Aug 5, 2018

zkbot referenced this in commit 6eecedba2c on Aug 5, 2018

zkbot referenced this in commit 8df048b1de on Aug 5, 2018

attilaaf referenced this in commit 91175ae4bb on May 25, 2019

PastaPastaPasta referenced this in commit fdf34ff655 on Aug 2, 2019

PastaPastaPasta referenced this in commit c796551a70 on Aug 6, 2019

PastaPastaPasta referenced this in commit bbfba296d3 on Jan 17, 2020

PastaPastaPasta referenced this in commit 2fb855a5fc on Jan 22, 2020

PastaPastaPasta referenced this in commit 8226417806 on Jan 22, 2020

barrystyle referenced this in commit f2f1b9ff58 on Jan 22, 2020

PastaPastaPasta referenced this in commit 44e8cec7fd on Jan 29, 2020

PastaPastaPasta referenced this in commit 6db8d3596a on Jan 29, 2020

PastaPastaPasta referenced this in commit a97ac0f09a on Jan 29, 2020

ckti referenced this in commit 05d8916d40 on Mar 28, 2021

random-zebra referenced this in commit 2d50b6e3b9 on Jun 9, 2021

gades referenced this in commit 279f90738d on Jun 30, 2021

DrahtBot locked this on Sep 8, 2021

malbit referenced this in commit 1990c3bdfd on Nov 5, 2021

gades referenced this in commit d12eb61017 on Feb 20, 2022

Multiwallet: simplest endpoint support #10849