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

laanwj commented at 8:12 pm on September 29, 2016: member

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

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

sipa commented at 12:50 pm on October 4, 2016: member

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 commented at 1:19 pm on October 4, 2016: member

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 commented at 2:33 pm on October 4, 2016: member

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 commented at 2:45 pm on October 4, 2016: member

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 commented at 2:49 pm on October 4, 2016: member

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 closed this on Oct 4, 2016

laanwj commented at 7:40 pm on June 15, 2017: member

Reopening after IRC discussion.

laanwj reopened this on Jun 15, 2017

rpc: Handle `getinfo` locally in bitcoin-cli w/ `-getinfo`

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.

157f0bff4e

laanwj force-pushed on Jun 15, 2017

laanwj commented at 8:28 pm on June 15, 2017: member

rebased

in src/bitcoin-cli.cpp:49 in 157f0bff4e

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("-getinfo", _("Get general information from the remote server"));

jnewbery commented at 6:17 pm on July 10, 2017:

nit: please place alphabetically within help messages.

in src/bitcoin-cli.cpp:321 in 157f0bff4e

316@@ -230,8 +317,8 @@ UniValue CallRPC(const std::string& strMethod, const UniValue& params)
317     evhttp_add_header(output_headers, "Authorization", (std::string("Basic ") + EncodeBase64(strRPCUserColonPass)).c_str());
318 
319     // Attach request data
320-    std::string strRequest = JSONRPCRequestObj(strMethod, params, 1).write() + "\n";
321-    struct evbuffer* output_buffer = evhttp_request_get_output_buffer(req.get());
322+    std::string strRequest = rh->PrepareRequest(strMethod, args).write() + "\n";
323+    struct evbuffer * output_buffer = evhttp_request_get_output_buffer(req.get());

jnewbery commented at 6:47 pm on July 10, 2017:

nit: why add a space here?

in src/bitcoin-cli.cpp:248 in 157f0bff4e

244+        result.pushKV("proxy", batch[ID_NETWORKINFO]["result"]["networks"][0]["proxy"]);
245+        result.pushKV("difficulty", batch[ID_BLOCKCHAININFO]["result"]["difficulty"]);
246+        result.pushKV("testnet", UniValue(batch[ID_BLOCKCHAININFO]["result"]["chain"].get_str() == "test"));
247+        if (!batch[ID_WALLETINFO].isNull()) {
248+            result.pushKV("keypoololdest", batch[ID_WALLETINFO]["result"]["keypoololdest"]);
249+            result.pushKV("keypoolsize", batch[ID_WALLETINFO]["result"]["keypoolsize"]);

jnewbery commented at 6:53 pm on July 10, 2017:

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:

 0→ bitcoin-cli getinfo
 1{
 2  "version": 149900,
 3...
 4  "keypoolsize": 199,
 5...
 6}
 7
 8→ bitcoin-cli -getinfo
 9{
10  "version": 149900,
11...
12  "keypoolsize": 99,
13...
14}

jnewbery commented at 5:30 am on July 16, 2017:

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.

in src/rpc/protocol.cpp:129 in 157f0bff4e

123@@ -124,3 +124,19 @@ void DeleteAuthCookie()
124     }
125 }
126 
127+std::vector<UniValue> JSONRPCProcessBatchReply(const UniValue &in, size_t num)
128+{
129+    if (!in.isArray())

jnewbery commented at 7:09 pm on July 10, 2017:

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

jnewbery commented at 7:11 pm on July 10, 2017: member

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 commented at 5:30 am on July 16, 2017: member

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

fanquake added this to the milestone 0.15.0 on Jul 16, 2017

TheBlueMatt commented at 10:55 pm on July 16, 2017: member

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 commented at 11:26 pm on July 16, 2017: member

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 commented at 11:59 pm on July 16, 2017: member

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

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” notifications@github.com 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 closed this on Jul 17, 2017

laanwj commented at 6:25 am on July 17, 2017: member

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

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

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” notifications@github.com 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 https://github.com/bitcoin/bitcoin/pull/8843#issuecomment-315702093, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHutr-cDdul0LBUNhs50Q4Hy8AMxAf8ks5sOyQLgaJpZM4KKbKh .

laanwj commented at 9:25 am on July 17, 2017: member

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” laanwj@gmail.com 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” notifications@github.com 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 https://github.com/bitcoin/bitcoin/pull/8843#issuecomment-315702093, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHutr-cDdul0LBUNhs50Q4Hy8AMxAf8ks5sOyQLgaJpZM4KKbKh .

promag commented at 9:48 am on July 17, 2017: member

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.

:+1: for custom scripting.

achow101 referenced this in commit 6432850158 on Jul 18, 2017

achow101 referenced this in commit ecde516693 on Aug 15, 2017

achow101 referenced this in commit e697d546c2 on Aug 15, 2017

jnewbery referenced this in commit bbd37c108f on Aug 23, 2017

luke-jr referenced this in commit b179c1b251 on Sep 2, 2017

achow101 referenced this in commit 67fed9376c on Sep 5, 2017

achow101 referenced this in commit 0a7355eb1d on Sep 5, 2017

laanwj referenced this in commit c9a4aa8a0e on Sep 28, 2017

PastaPastaPasta referenced this in commit 3c971291bb on Feb 13, 2020

PastaPastaPasta referenced this in commit 78ab47562c on Feb 13, 2020

PastaPastaPasta referenced this in commit f2024366d8 on Feb 29, 2020

ckti referenced this in commit 1082a0ab1a on Mar 28, 2021

DrahtBot locked this on Sep 8, 2021

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

Implementation

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