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

laanwj · Sep 29, 2016

(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 · Oct 4, 2016

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 · Oct 4, 2016

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 · Oct 4, 2016

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 · Oct 4, 2016

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 · Oct 4, 2016

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 · Jun 15, 2017

Reopening after IRC discussion.

laanwj · Jun 15, 2017

rebased

jnewbery · Jul 10, 2017

jnewbery reviewed Jul 10, 2017

View changes

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.

nit: please place alphabetically within help messages.

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.

nit: why add a space here?

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

jnewbery · Jul 16, 2017

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 · Jul 16, 2017

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 · Jul 16, 2017

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 · Jul 16, 2017

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 · Jul 17, 2017

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.

…

laanwj · Jul 17, 2017

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 · Jul 17, 2017

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 · Jul 17, 2017

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.

…

laanwj · Jul 17, 2017

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.

…

promag · Jul 17, 2017

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 added the RPC/REST/ZMQ label Sep 29, 2016

laanwj referenced this pull request Sep 30, 2016
Merged
[rpc] Deprecate getinfo #8780

laanwj closed this Oct 4, 2016

laanwj reopened this Jun 15, 2017

jnewbery referenced this pull request Jul 7, 2017
Open
[WIP] [wallet] Remove Wallet dependencies from init.cpp #10762

fanquake added this to the 0.15.0 milestone Jul 16, 2017

laanwj referenced this pull request Jul 17, 2017
Closed
[rpc] Give users one final warning before removing getinfo #10841

laanwj closed this Jul 17, 2017

achow101 added a commit to achow101/bitcoin that referenced this pull request Jul 18, 2017

achow101 Address comments from #8843 6432850

achow101 referenced this pull request Jul 18, 2017
Open
Handle getinfo in bitcoin-cli w/ -getinfo (revival of #8843) #10871

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

+    +        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"]);

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

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

bitcoin/bitcoin

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

laanwj commented Sep 29, 2016 • edited

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

laanwj referenced this pull request Sep 30, 2016

[rpc] Deprecate getinfo #8780

sipa commented Oct 4, 2016

laanwj commented Oct 4, 2016 • edited

luke-jr commented Oct 4, 2016

laanwj commented Oct 4, 2016 • edited

laanwj commented Oct 4, 2016 • edited

laanwj closed this Oct 4, 2016

laanwj commented Jun 15, 2017

laanwj reopened this Jun 15, 2017

laanwj commented Jun 15, 2017

jnewbery referenced this pull request Jul 7, 2017

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

jnewbery reviewed Jul 10, 2017

View changes

jnewbery commented Jul 16, 2017

fanquake added this to the 0.15.0 milestone Jul 16, 2017

TheBlueMatt commented Jul 16, 2017

promag commented Jul 16, 2017

jnewbery commented Jul 16, 2017

TheBlueMatt commented Jul 17, 2017

laanwj referenced this pull request Jul 17, 2017

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

laanwj closed this Jul 17, 2017

laanwj commented Jul 17, 2017

promag commented Jul 17, 2017

laanwj commented Jul 17, 2017

laanwj commented Jul 17, 2017

promag commented Jul 17, 2017

achow101 added a commit to achow101/bitcoin that referenced this pull request Jul 18, 2017

achow101 referenced this pull request Jul 18, 2017

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

bitcoin/bitcoin

Join GitHub today

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

Conversation

laanwj commented Sep 29, 2016 • edited

Implementation

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

laanwj referenced this pull request Sep 30, 2016

sipa commented Oct 4, 2016

laanwj commented Oct 4, 2016 • edited

luke-jr commented Oct 4, 2016

laanwj commented Oct 4, 2016 • edited

laanwj commented Oct 4, 2016 • edited

laanwj closed this Oct 4, 2016

laanwj commented Jun 15, 2017

laanwj reopened this Jun 15, 2017

laanwj commented Jun 15, 2017

jnewbery referenced this pull request Jul 7, 2017

jnewbery reviewed Jul 10, 2017 View changes

jnewbery commented Jul 16, 2017

fanquake added this to the 0.15.0 milestone Jul 16, 2017

TheBlueMatt commented Jul 16, 2017

promag commented Jul 16, 2017

jnewbery commented Jul 16, 2017

TheBlueMatt commented Jul 17, 2017

laanwj referenced this pull request Jul 17, 2017

laanwj closed this Jul 17, 2017

laanwj commented Jul 17, 2017

promag commented Jul 17, 2017

laanwj commented Jul 17, 2017

laanwj commented Jul 17, 2017

promag commented Jul 17, 2017

achow101 added a commit to achow101/bitcoin that referenced this pull request Jul 18, 2017

achow101 referenced this pull request Jul 18, 2017

jnewbery reviewed Jul 10, 2017

View changes