rpc: Handle `getinfo` client-side in bitcoin-cli w/ `-getinfo` #8843

laanwj · 2016-09-29T20:12:23Z

(follow-up to #8780)

There have been some requests for a client-side getinfo and this is my PoC of how to do it. The high-level idea is to have one command to gather and show various information and statistics about a bitcoind in one go.

This could be used as an opportunity to make getinfo better, it doesn't have to have exactly the same fields as the original RPC, e.g. a chain instead of testnet flag would make sense (but it is currently the same, according to this table: #8455 (comment)).

If this is considered a good idea some of the logic could be moved up to rpcclient.cpp and used in the GUI console as well.

Implementation

This adds the infrastructure BaseRequestHandler class that takes care of converting bitcoin-cli arguments into a JSON-RPC request object, and converting the reply into a JSON object that can be shown as result.

This is subsequently used to handle the -getinfo option, which sends a JSON-RPC batch request to the RPC server with ["getnetworkinfo", "getblockchaininfo", "getwalletinfo"], and after reply combines the result into what looks like a getinfo result.

sipa · 2016-10-04T12:50:58Z

Concept ACK. This allows us to maintain backward compatibility with "bitcoin-cli getinfo" without keeping it at the RPC level. Should "-getinfo" automatically trigger when the getinfo RPC is requested?

laanwj · 2016-10-04T13:19:37Z

Should "-getinfo" automatically trigger when the getinfo RPC is requested?

Not sure about that. That's how I had it first but this would blur the line between what bitcoin-cli calls an RPC call and what is actually available on the API endpoint.

Maybe "bitcoin-cli getinfo" if it fails (with the code that the call is missing) could print an helpful message to use "-getinfo" instead.

luke-jr · 2016-10-04T14:33:57Z

Eh, IMO the only backward-compatibility need for getinfo is at the RPC level. RPC isn't really designed for end users, and in any case, end users can adapt to changes easier than legacy software making RPC calls.

I don't particularly care about whether -getinfo is added to bitcoin-cli, but it doesn't seem like a viable migration path or substitute for the getinfo RPC.

laanwj · 2016-10-04T14:45:32Z

I don't particularly care about whether -getinfo is added to bitcoin-cli, but it doesn't seem like a viable migration path or substitute for the getinfo RPC.

You are right that RPC is not there for end users.
However, bitcoin-cli is there for end users, and many people use bitcoind+bitcoin-cli as a console wallet.

This is not just for backwards-compatibility. As I say in my OP

The high-level idea is to have one command to gather and show various information and statistics about a bitcoind in one go.

I'd be fine with completely different output here. However, the idea of having a single command that combines and prints various status information makes sense, IMO.

laanwj · 2016-10-04T14:49:49Z

And yes, this could be an external Python script too. I don't really care. Although history has shown that those scripts hardly get maintained nor used, e.g. remember contrib/bitrpc? Thought so.

(so if that is a requirement I'm just going to keep it as a private script and not bother with this, this may be too much of an ego-PR anyway)

laanwj · 2017-06-15T19:40:22Z

Reopening after IRC discussion.

This adds the infrastructure `BaseRequestHandler` class that takes care of converting bitcoin-cli arguments into a JSON-RPC request object, and converting the reply into a JSON object that can be shown as result. This is subsequently used to handle the `-getinfo` option, which sends a JSON-RPC batch request to the RPC server with `["getnetworkinfo", "getblockchaininfo", "getwalletinfo"]`, and after reply combines the result into what looks like a `getinfo` result. There have been some requests for a client-side `getinfo` and this is my PoC of how to do it. If this is considered a good idea some of the logic could be moved up to rpcclient.cpp and used in the GUI console as well.

laanwj · 2017-06-15T20:28:44Z

rebased

jnewbery

Looks good.

Should "-getinfo" automatically trigger when the getinfo RPC is requested?

That's how I had it first but this would blur the line between what bitcoin-cli calls an RPC call and what is actually available on the API endpoint.

I think bitcoin-cli getinfo should continue to work. I would also lean towards not using the bitcoin-cli -getinfo form. Yes, it's a bit magic, but it would mean maximum compatibility with any scripts, etc that are calling bitcoin-cli getinfo, and there'd be no excuse to not remove the actual RPC. If you're feeling really fastidious, you could add an extra "warnings" field to the returned object to say that getinfo isn't a real RPC. The returned object already has an "errors" field, so that wouldn't look too strange.

It's a shame that we don't have support in test/functional for bitcoin-cli tests. I don't think it's worth adding that just for this new function, but longer term we should aim to add tests.

Just one piece of functional feedback, and a couple of nits.

jnewbery · 2017-07-10T18:17:28Z

src/bitcoin-cli.cpp

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


nit: please place alphabetically within help messages.

jnewbery · 2017-07-10T18:47:09Z

src/bitcoin-cli.cpp

-    std::string strRequest = JSONRPCRequestObj(strMethod, params, 1).write() + "\n";
-    struct evbuffer* output_buffer = evhttp_request_get_output_buffer(req.get());
+    std::string strRequest = rh->PrepareRequest(strMethod, args).write() + "\n";
+    struct evbuffer * output_buffer = evhttp_request_get_output_buffer(req.get());


nit: why add a space here?

jnewbery · 2017-07-10T18:53:44Z

src/bitcoin-cli.cpp

+        result.pushKV("testnet", UniValue(batch[ID_BLOCKCHAININFO]["result"]["chain"].get_str() == "test"));
+        if (!batch[ID_WALLETINFO].isNull()) {
+            result.pushKV("keypoololdest", batch[ID_WALLETINFO]["result"]["keypoololdest"]);
+            result.pushKV("keypoolsize", batch[ID_WALLETINFO]["result"]["keypoolsize"]);


slight incompatibility here since HD split. For HD split wallets, keypoolsize refers to the size of the external keypool, and there's a new field returned in getwalletinfo called keypoolsize_hd_internal, which refers to the size of the internal keypool. This implementation should return the sum of those two if keypoolsize_hd_internal exists.

Otherwise:

→ bitcoin-cli getinfo { "version": 149900, ... "keypoolsize": 199, ... } → bitcoin-cli -getinfo { "version": 149900, ... "keypoolsize": 99, ... }

Actually, I've got this backwards. getinfo should be updated to be consistent with getwalletinfo, not making -getinfo consistent with the incorrect getinfo.

I think that can be done as a separate commit in this PR. Let me know if you want me to make a branch with that commit.

jnewbery · 2017-07-10T19:09:21Z

src/rpc/protocol.cpp

@@ -124,3 +124,19 @@ void DeleteAuthCookie()
    }
 }

+std::vector<UniValue> JSONRPCProcessBatchReply(const UniValue &in, size_t num)
+{
+    if (!in.isArray())


nit: new code so should use braces for if blocks (sorry!)

jnewbery · 2017-07-16T05:30:49Z

I'd love for this to make it in to v0.15.0, to ease the removal of getinfo in (hopefully) v0.15.1.

TheBlueMatt · 2017-07-16T22:55:06Z

I am concerned that this breaks the atomicity of the previously-atomic RPC getinfo, which may be very surprising for users. I'm not sure how much it matters if you get inconsistent data in results from bitcoin-cli, since most automated things will likely be using RPC directly, but I'm not convinced it is a great direction.

promag · 2017-07-16T23:26:03Z

NACK. Keep it simple for cli. IMO this is a good example for what to not do in a cli and can lead to others do the same in their cli.. This doesn't break atomicity as there is no getinfo, at the very least that should be a concern in #8843. The ones requesting getinfo should either just make the calls or understand what they really need.

jnewbery · 2017-07-16T23:59:09Z

I'm not convinced it is a great direction

this is a good example for what to not do in a cli and can lead to others do the same in their cli

Idealogical purity is fine in its place, but this is an isolated change to help those who are currently use getinfo from the command line. I don't think it sets a precedent for doing more hacks with the cli.

Perhaps don't document this? Or document it as a debug option? Would that be better?

To be clear: I'm not advocating strongly for this PR. If the consensus view is that this is not required to remove getinfo then I'm happy to go along with that. However, I'm certainly not concerned about it setting precedent and I don't think that a good reason to not merge this.

TheBlueMatt · 2017-07-17T00:05:05Z

I'm having trouble parsing your comment @promag. My concern with atomicity is that you could eg get a getwalletinfo result which is current, then a getblockchaininfo result a block later and end up with a very confused getinfo. Forcing the client to make separate calls clarifies that somewhat. Anyway, I'm curious what the previous IRC discussion was, let's bring this up at next week's meeting (or on IRC on Monday) and figure out what it is that the use-case is.

…

On July 16, 2017 7:26:08 PM EDT, "João Barbosa" ***@***.***> wrote: NACK. Keep it simple for cli. IMO this is a good example for what to not do in a cli and can lead to others do the same in their cli.. This doesn't break atomicity as there is no getinfo, at the very least that should be a concern in #8843. The ones requesting getinfo should either just make the calls or understand what they really need.

laanwj · 2017-07-17T06:25:29Z

Closing this, I don't feel like addressing the atomicity concern, it seems arbitrary, never came up before, and a way to call multiple RPC calls atomically is WAY out of scope for this. Not going to maintain this anymore.

promag · 2017-07-17T09:06:15Z

but this is an isolated change to help those who are currently use getinfo from the command line

@jnewbery that's what deprecation is for.

Forcing the client to make separate calls clarifies that somewhat.

@TheBlueMatt I totally agree with you! Sorry if I wasn't clear.

@laanwj IMO best solution is to add support for http://www.jsonrpc.org/specification#batch, where the locks are held while handling the batch request.

laanwj · 2017-07-17T09:16:29Z

I use a batch in the implementation of this! Holding all the locks for the whole time seems a bad idea though. A batch might include, or fully consist of, RPCs that don't need any locks at all. Locks have been pushed down, just a version ago, to be able to handle them separately per call implementation. It could be done optionally; e.g. submit a list of locks with the batch. But wtf, I had never before this even heard of a use case for that, seems very rare.

…

On Jul 17, 2017 11:06 AM, "João Barbosa" ***@***.***> wrote: but this is an isolated change to help those who are currently use getinfo from the command line @jnewbery <https://github.com/jnewbery> that's what deprecation is for. Forcing the client to make separate calls clarifies that somewhat. @TheBlueMatt <https://github.com/thebluematt> I totally agree with you! Sorry if I wasn't clear. @laanwj <https://github.com/laanwj> IMO best solution is to add support for http://www.jsonrpc.org/specification#batch, where the locks are held while handling the batch request. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8843 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHutr-cDdul0LBUNhs50Q4Hy8AMxAf8ks5sOyQLgaJpZM4KKbKh> .

laanwj · 2017-07-17T09:25:09Z

Anyhow I'm no longer going to work on this after the so-manyeth time this gets shot down for out-of-the-blue reasons. I don't even use it myself. The getinfo output doesn't provide a useful selection of information. I have a custom script that prints what I'm interested in, and suggest everyone does the same, easy enough.

…

On Jul 17, 2017 11:16 AM, "Wladimir" ***@***.***> wrote: I use a batch in the implementation of this! Holding all the locks for the whole time seems a bad idea though. A batch might include, or fully consist of, RPCs that don't need any locks at all. Locks have been pushed down, just a version ago, to be able to handle them separately per call implementation. It could be done optionally; e.g. submit a list of locks with the batch. But wtf, I had never before this even heard of a use case for that, seems very rare. On Jul 17, 2017 11:06 AM, "João Barbosa" ***@***.***> wrote: > but this is an isolated change to help those who are currently use > getinfo from the command line > > @jnewbery <https://github.com/jnewbery> that's what deprecation is for. > > Forcing the client to make separate calls clarifies that somewhat. > > @TheBlueMatt <https://github.com/thebluematt> I totally agree with you! > Sorry if I wasn't clear. > > @laanwj <https://github.com/laanwj> IMO best solution is to add support > for http://www.jsonrpc.org/specification#batch, where the locks are held > while handling the batch request. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#8843 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAHutr-cDdul0LBUNhs50Q4Hy8AMxAf8ks5sOyQLgaJpZM4KKbKh> > . >

promag · 2017-07-17T09:48:20Z

I use a batch in the implementation of this!

@laanwj sorry I've overlook the code and did not realize there is already support for server side batch requests.

👍 for custom scripting.

@laanwj

…8843) 5e69a43 Add test for bitcoin-cli -getinfo (John Newbery) 3826253 rpc: Handle `getinfo` locally in bitcoin-cli w/ `-getinfo` (Wladimir J. van der Laan) Pull request description: Since @laanwj doesn't want to maintain these changes anymore, I will. This PR is a revival of #8843. I have addressed @jnewbery's comments. Regarding atomicity, I don't think that is a concern here. This is explicitly a new API and those who use it will know that this is different and that it is not atomic. Tree-SHA512: 9664ed13a5557bda8c43f34d6527669a641f260b7830e592409b28c845258fc7e0fdd85dd42bfa88c103fea3ecdfede5f81e3d91870e2accba81c6d6de6b21ff

@laanwj

…al of bitcoin#8843) 5e69a43 Add test for bitcoin-cli -getinfo (John Newbery) 3826253 rpc: Handle `getinfo` locally in bitcoin-cli w/ `-getinfo` (Wladimir J. van der Laan) Pull request description: Since @laanwj doesn't want to maintain these changes anymore, I will. This PR is a revival of bitcoin#8843. I have addressed @jnewbery's comments. Regarding atomicity, I don't think that is a concern here. This is explicitly a new API and those who use it will know that this is different and that it is not atomic. Tree-SHA512: 9664ed13a5557bda8c43f34d6527669a641f260b7830e592409b28c845258fc7e0fdd85dd42bfa88c103fea3ecdfede5f81e3d91870e2accba81c6d6de6b21ff

@laanwj

…al of bitcoin#8843) 5e69a43 Add test for bitcoin-cli -getinfo (John Newbery) 3826253 rpc: Handle `getinfo` locally in bitcoin-cli w/ `-getinfo` (Wladimir J. van der Laan) Pull request description: Since @laanwj doesn't want to maintain these changes anymore, I will. This PR is a revival of bitcoin#8843. I have addressed @jnewbery's comments. Regarding atomicity, I don't think that is a concern here. This is explicitly a new API and those who use it will know that this is different and that it is not atomic. Tree-SHA512: 9664ed13a5557bda8c43f34d6527669a641f260b7830e592409b28c845258fc7e0fdd85dd42bfa88c103fea3ecdfede5f81e3d91870e2accba81c6d6de6b21ff

@laanwj

…al of bitcoin#8843) 5e69a43 Add test for bitcoin-cli -getinfo (John Newbery) 3826253 rpc: Handle `getinfo` locally in bitcoin-cli w/ `-getinfo` (Wladimir J. van der Laan) Pull request description: Since @laanwj doesn't want to maintain these changes anymore, I will. This PR is a revival of bitcoin#8843. I have addressed @jnewbery's comments. Regarding atomicity, I don't think that is a concern here. This is explicitly a new API and those who use it will know that this is different and that it is not atomic. Tree-SHA512: 9664ed13a5557bda8c43f34d6527669a641f260b7830e592409b28c845258fc7e0fdd85dd42bfa88c103fea3ecdfede5f81e3d91870e2accba81c6d6de6b21ff

laanwj added the RPC/REST/ZMQ label Sep 29, 2016

laanwj mentioned this pull request Sep 30, 2016

[rpc] Deprecate getinfo #8780

Merged

laanwj closed this Oct 4, 2016

laanwj reopened this Jun 15, 2017

laanwj force-pushed the 2016_09_getinfo_clientside branch from 9552a3a to 157f0bf Compare June 15, 2017 20:28

jnewbery mentioned this pull request Jul 7, 2017

[wallet] Remove Wallet dependencies from init.cpp #10762

Merged

jnewbery reviewed Jul 10, 2017

View reviewed changes

fanquake added this to the 0.15.0 milestone Jul 16, 2017

laanwj mentioned this pull request Jul 17, 2017

[rpc] Give users one final warning before removing getinfo #10841

Closed

laanwj closed this Jul 17, 2017

achow101 mentioned this pull request Jul 18, 2017

Handle getinfo in bitcoin-cli w/ -getinfo (revival of #8843) #10871

Merged

jnewbery pushed a commit to jnewbery/bitcoin that referenced this pull request Aug 23, 2017

Address comments from bitcoin#8843

bbd37c1

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Sep 2, 2017

Address comments from bitcoin#8843

b179c1b

laanwj mentioned this pull request Feb 26, 2018

Remove suggestion "getinfo" #12540

Closed

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

rpc: Handle getinfo client-side in bitcoin-cli w/ -getinfo #8843

rpc: Handle getinfo client-side in bitcoin-cli w/ -getinfo #8843

Uh oh!

Conversation

laanwj commented Sep 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Implementation

Uh oh!

sipa commented Oct 4, 2016

Uh oh!

laanwj commented Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr commented Oct 4, 2016

Uh oh!

laanwj commented Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Jun 15, 2017

Uh oh!

laanwj commented Jun 15, 2017

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery Jul 10, 2017

Choose a reason for hiding this comment

Uh oh!

jnewbery Jul 10, 2017

Choose a reason for hiding this comment

Uh oh!

jnewbery Jul 10, 2017

Choose a reason for hiding this comment

Uh oh!

jnewbery Jul 16, 2017

Choose a reason for hiding this comment

Uh oh!

jnewbery Jul 10, 2017

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Jul 16, 2017

Uh oh!

TheBlueMatt commented Jul 16, 2017

Uh oh!

promag commented Jul 16, 2017

Uh oh!

jnewbery commented Jul 16, 2017

Uh oh!

TheBlueMatt commented Jul 17, 2017 via email

Uh oh!

laanwj commented Jul 17, 2017

Uh oh!

promag commented Jul 17, 2017

Uh oh!

laanwj commented Jul 17, 2017 via email

Uh oh!

laanwj commented Jul 17, 2017 via email

Uh oh!

promag commented Jul 17, 2017

Uh oh!

Uh oh!

rpc: Handle `getinfo` client-side in bitcoin-cli w/ `-getinfo` #8843

rpc: Handle `getinfo` client-side in bitcoin-cli w/ `-getinfo` #8843

laanwj commented Sep 29, 2016 •

edited

Loading

laanwj commented Oct 4, 2016 •

edited

Loading

laanwj commented Oct 4, 2016 •

edited

Loading

laanwj commented Oct 4, 2016 •

edited

Loading