rpc: query general daemon information via RPC #17958

brakmic commented at 6:05 PM on January 18, 2020: contributor

This PR implements a new feature requested in #17952

A new call getgeneralinfo is now available for sending general daemon information back to clients. Currently, this data is only available via GUI (debug window / information tab).

The RPC execution looks like this:

bitcoin-cli -regtest getgeneralinfo

{
  "clientversion": "v0.19.99.0-a654626f0-dirty",
  "useragent": "/Satoshi:0.19.99/",
  "datadir": "/Users/brakmic/Library/Application Support/Bitcoin/regtest",
  "blocksdir": "/Users/brakmic/Library/Application Support/Bitcoin/regtest/blocks",
  "startuptime": "2020-01-18T17:49:50Z"
}

bitcoin-cli -regtest help getgeneralinfo

getgeneralnfo

Returns data about the bitcoin daemon.

Result:
  {
    "clientversion": "v0.19.99.0-a654626f0",       (string) Client version
    "useragent":"/Satoshi:0.19.99/",               (string) Client name
    "datadir":"/home/user/.bitcoin",               (string) Data directory path
    "blocksdir":"/home/user/.bitcoin/blocks",      (string) Blocks directory path
    "startuptime":"2020-01-18T17:49:50Z",          (string) Startup time
  }

Examples:
> bitcoin-cli getgeneralinfo 
> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id":"curltest", "method": "getgeneralinfo", "params": [] }' -H 'content-type: text/plain;' http://127.0.0.1:8332/

However, I am not sure if the current format in "startuptime" field is acceptable. Maybe there is a better function that should be used? Currently I am using the FormatISO8601DateTime function.

Test A new functional test rpc_getgeneralinfo.py is available.

Regards,

brakmic renamed this:
~~rpc: return general daemon information to client~~
rpc: query general daemon information via RPC
on Jan 18, 2020

in src/rpc/misc.cpp:482 in 1ba064be4d outdated

 477 | +        obj.pushKV("datadir", GetDataDir().string());
 478 | +        obj.pushKV("blocksdir", GetBlocksDir().string());
 479 | +        obj.pushKV("startuptime", FormatISO8601DateTime(GetStartupTime()));
 480 | +        return obj;
 481 | +
 482 | +}

msafi commented at 6:13 PM on January 18, 2020:

Is it better to move this function to server.cpp since that file contains the majority of control commands, like getrpcinfo and uptime?

brakmic commented at 6:25 PM on January 18, 2020:

I could move them there. Actually, I put them in misc.cpp because I wasn't sure where to put it best. Maybe we should wait for additional input from others before changing the current variant?

in src/rpc/misc.cpp:461 in 1ba064be4d outdated

 456 | +    RPCHelpMan{"getgeneralnfo",
 457 | +                "\nReturns data about the bitcoin daemon.\n",
 458 | +                {},
 459 | +                RPCResult{
 460 | +                "  {\n"
 461 | +                "    \"clientversion\": \"version string\",     (string) Client version\n"

msafi commented at 6:20 PM on January 18, 2020:

In my opinion, it would be more helpful to show values here that look like the real ones, like this

{
  "clientversion": "v0.19.99.0-a654626f0-dirty",
  "useragent": "/Satoshi:0.19.99/",
  "datadir": "/Users/brakmic/Library/Application Support/Bitcoin/regtest",
  "blocksdir": "/Users/brakmic/Library/Application Support/Bitcoin/regtest/blocks",
  "startuptime": "2020-01-18T17:49:50Z"
}

That would better communicate to the user what to expect...

brakmic commented at 6:25 PM on January 18, 2020:

Yeah, I will put more realistic values in there.

in src/rpc/misc.cpp:465 in 1ba064be4d outdated

 460 | +                "  {\n"
 461 | +                "    \"clientversion\": \"version string\",     (string) Client version\n"
 462 | +                "    \"useragent\":\"client name\",             (string) Client name\n"
 463 | +                "    \"datadir\":\"datadir path\",              (string) Data directory path\n"
 464 | +                "    \"blocksdir\":\"blocksdir path\",          (string) Blocks directory path\n"
 465 | +                "    \"startuptime\":\"string\",                (string) Startup time\n"

msafi commented at 6:22 PM on January 18, 2020:

uptime in server.cpp returns type int64_t. I think for consistency it would be better for startuptime to return the same thing. The consumer of the timestamp can choose how to format it.

brakmic commented at 6:27 PM on January 18, 2020:

Well, the GUI shows a properly formatted / localized time. Returning a numeric value would maybe collide with user expectations? Or we could simply return two values (raw number and localized one). In any case, this should not be a big problem. Let's wait for more input, shall we? :)

msafi commented at 6:23 PM on January 18, 2020: none

Thank you for implementing this @brakmic, left a few comments!

in src/rpc/misc.cpp:465 in 9c65cb5698 outdated

 460 | +                "  {\n"
 461 | +                "    \"clientversion\": \"v0.19.99.0-a654626f0\",       (string) Client version\n"
 462 | +                "    \"useragent\":\"/Satoshi:0.19.99/\",               (string) Client name\n"
 463 | +                "    \"datadir\":\"/home/user/.bitcoin\",               (string) Data directory path\n"
 464 | +                "    \"blocksdir\":\"/home/user/.bitcoin/blocks\",      (string) Blocks directory path\n"
 465 | +                "    \"startuptime\":\"2020-01-18T17:49:50Z\",          (string) Startup time\n"

jonatack commented at 6:47 PM on January 18, 2020:

I think this should not contain values, just types and description. See the code for other rpcs.

brakmic commented at 7:18 PM on January 18, 2020:

I have consulted other RPC's and I must admit that there is no consistency, imo. Some examples use dummy values (for example: getaddednodeinfo) while others show potential values like "true|false" or data types like "n" (for example: getnettotals). All in all, I don't know what's the right way, but I will follow your advice and simply put some short info entries for every field.

in test/functional/rpc_getgeneralinfo.py:12 in 9c65cb5698 outdated

   7 | +from test_framework.test_framework import BitcoinTestFramework
   8 | +from test_framework.util import assert_equal, try_rpc
   9 | +
  10 | +class GetGeneralInfoTest(BitcoinTestFramework):
  11 | +    def set_test_params(self):
  12 | +        self.setup_clean_chain = True

jonatack commented at 6:49 PM on January 18, 2020:

Do you need a clean chain? If not, use False to use the cached data; see test/functional/README.md:

Set the `self.setup_clean_chain` variable in `set_test_params()`
to control whether or not to use the cached data directories.

in test/functional/rpc_getgeneralinfo.py:16 in 9c65cb5698 outdated

  11 | +    def set_test_params(self):
  12 | +        self.setup_clean_chain = True
  13 | +        self.num_nodes = 1
  14 | +
  15 | +    def run_test(self):
  16 | +        # Test getgeneralinfo

jonatack commented at 6:50 PM on January 18, 2020:

Please use a docstring and/or logging.

jonatack commented at 7:06 PM on January 18, 2020:

Perhaps add these tests into an existing testfile like interface_bitcoin_cli.py.

brakmic commented at 7:28 PM on January 18, 2020:

Would this mean that I should remove rpc_getgeneralinfo.py? Shouldn't every RPC call have its own test file?

jonatack commented at 7:41 PM on January 18, 2020:

Oh right, never mind, here it's implemented as an RPC and not the CLI. I conflated the two since it seemed like a CLI call :) About separate tests: We want the functional tests to run as quickly as possible, so if it's feasible to add this small test in a logical place in an existing setup, it might be better. That said, I wouldn't worry about it unless this PR gets Concept ACKs and others say the same.

brakmic commented at 7:56 PM on January 18, 2020:

Thanks! Ok, then I'll leave it as it is for now and will then come back when/if this PR gets enough ACKs.

in test/functional/rpc_getgeneralinfo.py:13 in 9c65cb5698 outdated

   8 | +from test_framework.util import assert_equal, try_rpc
   9 | +
  10 | +class GetGeneralInfoTest(BitcoinTestFramework):
  11 | +    def set_test_params(self):
  12 | +        self.setup_clean_chain = True
  13 | +        self.num_nodes = 1

jonatack commented at 7:03 PM on January 18, 2020:

perhaps

    def skip_test_if_missing_module(self):
        self.skip_if_no_cli()

also, does the test require the wallet being built and loaded to function?

brakmic commented at 7:25 PM on January 18, 2020:

Yes, many thanks!

jonatack commented at 7:07 PM on January 18, 2020: member

<strike>-0.1 on adding this. I'm not sure what the use case is; there are already calls like bitcoin-cli -version and bitcoin-cli -getinfo, and unless I'm missing something the user knows any custom dir settings either from having set them in the .conf file or from passing them as startup option args. Perhaps as additions to -getinfo, if really desired? See also #17314.</strike>

Edit: I didn't understand the motivation on first review.

brakmic commented at 7:22 PM on January 18, 2020: contributor

-0.1 on adding this. I'm not sure what the use case is; there are already calls like bitcoin-cli -version and bitcoin-cli -getinfo, and unless I'm missing something the user knows any custom dir settings either from having set them in the .conf file or from passing them as startup option args. Perhaps as additions to -getinfo, if really desired? See also #17314.

If there is any problem with this PR I will close it, no problem. I simply had some free time, so I thought, let's implement one of the requests that aren't that complex for me ;)

DrahtBot added the label RPC/REST/ZMQ on Jan 18, 2020

DrahtBot added the label Tests on Jan 18, 2020

jonatack commented at 8:15 PM on January 18, 2020: member

I understand better now what the use case is after @msafi's explanation.

DrahtBot commented at 9:32 PM on January 18, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18038 (P2P: Mempool tracks locally submitted transactions to improve privacy by amitiuttarwar)
#18037 (Util: Allow scheduler to be mocked by amitiuttarwar)
#17585 (rpc: deprecate getaddressinfo label by jonatack)
#17577 (refactor: deduplicate the message sign/verify code by vasild)
#16365 (Log RPC parameters (arguments) if -debug=rpcparams by LarryRuane)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

emilengler commented at 4:29 PM on January 19, 2020: contributor

Why isn't the BerkeleyDB version included?

brakmic commented at 4:35 PM on January 19, 2020: contributor

Why isn't the BerkeleyDB version included?

Because it wasn't asked for in the feature request. However, this shouldn't be a problem to include later. But first I'll wait for more ACKs/NACKs regarding the PR itself before changing anything.

emilengler commented at 4:36 PM on January 19, 2020: contributor

Because it wasn't asked for in the feature request.

If we display the GUI stuff in text form we should display it all including the BDB version

brakmic commented at 4:37 PM on January 19, 2020: contributor

Because it wasn't asked for in the feature request.

If we display the GUI stuff in text form we should display it all including the BDB version

No problem, this can be included. However, first we should wait for more reviews.

in test/functional/rpc_getgeneralinfo.py:19 in e69f468bcf outdated

  14 | +
  15 | +    def skip_test_if_missing_module(self):
  16 | +        self.skip_if_no_cli()
  17 | +
  18 | +    def run_test(self):
  19 | +        """Test getgeneralinfo."""

emilengler commented at 4:37 PM on January 19, 2020:

Please use # if it's a one line comment

brakmic commented at 4:39 PM on January 19, 2020:

Please, check previous reviews from @jonatack I have already changed those comments from # to doctype.

jonatack commented at 5:29 PM on January 19, 2020:

I agree with @emilengler and would suggest spending some time looking at the other tests and the PEP8 style guide.

in test/functional/rpc_getgeneralinfo.py:23 in e69f468bcf outdated

  18 | +    def run_test(self):
  19 | +        """Test getgeneralinfo."""
  20 | +        """Check if 'getgeneralinfo' is callable."""
  21 | +        try_rpc(None, None, self.nodes[0].getgeneralinfo, None, None)
  22 | +        """Check if 'getgeneralinfo' is idempotent (multiple requests return same data)."""
  23 | +        json1 = self.nodes[0].getgeneralinfo()

emilengler commented at 4:38 PM on January 19, 2020:

Why not put this into a tuple

brakmic commented at 4:41 PM on January 19, 2020:

I don't understand the context. However, I am not that experienced in python so it could be that my code is not following best-practices.

jonatack commented at 5:28 PM on January 19, 2020:

As a general thought, I'd humbly suggest spending time looking at existing tests here, reviewing, practising writing PRs without opening them, and polishing PRs more before adding them to the stack of open PRs to review. Maybe I'm doing this wrong, but I have dozens of finished commits that I've written locally without opening PRs to (a) to learn the codebase, and (b) to maintain a ratio of 5-15 PRs reviewed or issues tested per PR opened, as the number of open issues and PRs keeps growing and review/testing is where resources are needed.

brakmic commented at 5:35 PM on January 19, 2020:

Well, the best way to learn something is by doing it. Reading docs is fine, sure, but in the end you have to "say something" so others can hear you (and give you a proper critique). Docs don't correct you if you get something wrong. Anyway, I think it's best to close this PR, just to make the PR-mountain a bit smaller. This RPC call isn't that important anyway, I think.

jonatack commented at 5:38 PM on January 19, 2020:

I have a lot to learn but these two articles based on the past ten months might be helpful:

msafi commented at 5:39 PM on January 19, 2020:

@jonatack Is this PR really that bad? I spent a few weeks looking at the codebase and tests and I would have raised a similar PR.

Will read the articles, thanks!

jonatack commented at 5:56 PM on January 19, 2020:

@jonatack Is this PR really that bad? I spent a few weeks looking at the codebase and tests and I would have raised a similar PR.

Well, we all do. I think the best way for you to answer your question is to review a few dozen of the open PRs. Here are the high-priority ones that really need review: https://github.com/bitcoin/bitcoin/projects/8

MarcoFalke changes_requested

MarcoFalke commented at 4:41 PM on January 19, 2020: member

How is this different from the existing RPCs uptime and getnetworkinfo?

msafi commented at 4:53 PM on January 19, 2020: none

@MarcoFalke startuptime can be derived from uptime and useragent seems to be the same as subversion in networkinfo so I guess these two could be removed.

But the following data is missing from getnetworkinfo:

Version build number and suffix
datadir
blocksdir
BerkeleyDB version (if we wanna include that)

MarcoFalke commented at 8:26 PM on January 19, 2020: member

Version build number and suffix

It is in getnetworkinfo (as an int, and not a str unfortunately)

datadir, blocksdir

What would the use case be?

BerkeleyDB version (if we wanna include that)

This should go in getwalletinfo, but I can't see a use case for that

emilengler commented at 8:29 PM on January 19, 2020: contributor

This should go in getwalletinfo, but I can't see a use case for that

Why is it included in the GUI then? IMO everything which accessible through the GUI should be accessible through the RPC interface as well. Why even bother about a concept that is already implemented in some way?

jonatack commented at 9:08 PM on January 19, 2020: member

Sure, it's a large, legacy codebase and there are inconsistencies. Meanwhile, is a regression or CVE getting past review somewhere? WRT external wallets using the RPC interface, I agree more feedback on the use cases may be helpful.

msafi commented at 10:59 PM on January 19, 2020: none

@MarcoFalke The use-case is to build a GUI that works fully through RPC.

Jonas Schnelli said this:

If I would have some wishes open, one of them would be that the GUI works perfectly asynchronous over the RPC interface

I'm working on such a GUI. But regardless of what I'm working on, if there's a desire to rearchitect the Bitcoin Core GUI to work through RPC then we need to make changes like what's in this PR.

Does this help explain the use case?

The other case that can be made for this type of change is what @emilengler said: if you make some information available on the GUI, why not also make it available through RPC?

jonasschnelli commented at 5:35 AM on January 20, 2020: contributor

General Concept ACK.

I think it can be useful to get the Version build number and suffix through RPC. Also, it should not hurt to add the blocksdir and datadir to an existing RPC call (though not sure where it would fit, getnetworkinfo or getblockchaininfo may suit best). Unsure about adding a new call.

If there is a use case for the BDB version info (is there?), it should (must?!) probably be under getwalletinfo.

msafi commented at 7:36 AM on January 20, 2020: none

Because the BDB version is available on the GUI and it's easy to add, I think it should be included.

I think it might help avoid subjectivity and decision fatigue if we were to establish some general criteria/guidelines for what is allowed to be added to RPC.

jonasschnelli commented at 7:49 AM on January 20, 2020: contributor

Because the BDB version is available on the GUI and it's easy to add, I think it should be included.

The GUI isn't perfect and a pure 1:1 replication through the RPC API is maybe not the best approach.

I would rather think about possible use cases why someone would want to know the Berkley-DB version (wallet file incompatibility?).

I think it might help avoid subjectivity and decision fatigue if we were to establish some general criteria/guidelines for what is allowed to be added to RPC.

I guess the answer is: "it depends". IMO additional information through RPC should solve a real use case (which I can see for a remote RPC GUI for blocksdir/datadir/version-build-number).

jonatack commented at 8:32 AM on January 20, 2020: member

I agree. If I had to give a general guideline as an exercise, I'd say, in roughly decreasing order of priority: fixing bugs, improving security/test coverage, addressing real use cases, doing nothing or doing less/removing code, doing more/adding code.

fanquake commented at 9:56 AM on January 20, 2020: member

I think it might help avoid subjectivity and decision fatigue if we were to establish some general criteria/guidelines for what is allowed to be added to RPC. @msafi In my opinion, even though it may have happened in the past, the bar for adding a new RPC to bitcoind is currently far higher than a single developer (or two) wanting it for a (proof-of-concept) project.

New RPCs come with maintenance and and backwards-compatibility overheads, and once added, can be near impossible to modify/remove, which can hinder lower level, higher priority refactoring efforts. See the history of getinfo, a general purpose RPC, for an example of that. Any time we're considering adding a new RPC, it needs be worthwhile.

bitcoind currently provides > 120 different RPCs, implementing an extensive range of functionality. Looking at your project, which I assume is what you'd like this for, it seems like you currently implement support for 6 or 7 of our general, informational RPC calls. Is it fair to say there isn't too much of a rush for Bitcoin Core to add a new RPC, assuming you're going to build out a fully functional GUI, add wallet functionality, network monitoring etc?

As has already been mentioned, it doesn't make much sense for us to add any information to a new RPC that can already be derived from an existing one (or any other way), so if this is going to be added, lets make sure we're not duplicating anything.

It's also been mentioned that some of the information might be better suited to an existing RPC, rather than adding a new one, I also agree with this. i.e the bdb version going in getwalletinfo.

More generally, if you're interested in the modularization of Bitcoin Core (which is a step towards implementing alternative GUIs), and what work is being done on that front, I'd suggest looking at the Process Separation Project, if you haven't already; along with PRs like #10102.

promag commented at 10:13 AM on January 20, 2020: member

Agree with @fanquake. @msafi my suggestion is to keep working on the gui-over-rpc and skip this detail. With that you'll be in a better position to request these changes.

This should go in getwalletinfo, but I can't see a use case for that

Why is it included in the GUI then? IMO everything which accessible through the GUI should be accessible through the RPC interface as well. Why even bother about a concept that is already implemented in some way? @emilengler it's a different case, the GUI is the node.

emilengler commented at 1:01 PM on January 20, 2020: contributor

@emilengler it's a different case, the GUI is the node.

The question was not what's the node. I was more referring to the way how the/a node can be accessed (RPC, GUI)

promag commented at 2:00 PM on January 20, 2020: member

how the/a node can be accessed

Right, either you access a remote node or the local node. The existing GUI is to access the local node, and in that case makes sense to see the datadir. But for remote access, the datadir is not that relevant, or is it?

msafi commented at 6:27 PM on January 20, 2020: none

the bar for adding a new RPC to bitcoind is currently far higher than a single developer (or two) wanting it for a (proof-of-concept) project.

I agree. That's how it should be. That's why I didn't want this feature request to be about my project because in that case it's not worth the overhead for Bitcoin Core. But my being the only one to request it doesn't mean I'm the only one wanting it.

That's why I agree with the thinking that any information on the GUI should be available through RPC (with some caveats, like has been mentioned).

Is it fair to say there isn't too much of a rush for Bitcoin Core to add a new RPC, assuming you're going to build out a fully functional GUI, add wallet functionality, network monitoring etc?

I wanted make a complete end-user release of Orange with just the node syncing feature. Then build incrementally.

The getgeneralinfo feature in this PR is not the blocker. The blockers are the header sync progress, init messages, and shutdown messages not being available through RPC. I was planning more follow-up feature requests (and PRs 😱) to add those.

Anyways, while I still think it would be useful to bring the RPC interface closer to the GUI, I understand the concerns around priority, urgency, and resources. So I'll accept whatever is decided here. With regard to my project, something will be figured out eventually...

emilengler commented at 7:24 PM on January 20, 2020: contributor

But for remote access, the datadir is not that relevant, or is it?

It is. The RPC interface is also used by the admins with bitcoin-cli who might wanna know where it is located

in src/rpc/misc.cpp:479 in e69f468bcf outdated

 474 | +        UniValue obj(UniValue::VOBJ);
 475 | +        obj.pushKV("clientversion", FormatFullVersion());
 476 | +        obj.pushKV("useragent", strSubVersion);
 477 | +        obj.pushKV("datadir", GetDataDir().string());
 478 | +        obj.pushKV("blocksdir", GetBlocksDir().string());
 479 | +        obj.pushKV("startuptime", FormatISO8601DateTime(GetStartupTime()));

luke-jr commented at 5:33 PM on January 27, 2020:

Times should be seconds-since-epoch Numbers, not formatted strings.

brakmic commented at 5:36 PM on January 27, 2020:

I tried to imitate the output in the UI, hence the formatting. However, this is no problem, I can return it as seconds-since-epoch. But I think this PR will never get into the code anyway (please, check previous discussion if not done already).

brakmic commented at 5:47 PM on January 27, 2020:

Have updated the code. Thanks for the hint.

rpc: implement getgeneralinfo cdbd38df13

DrahtBot commented at 8:55 AM on February 2, 2020: member

Needs rebase

DrahtBot added the label Needs rebase on Feb 2, 2020

fanquake commented at 5:37 AM on February 3, 2020: member

Going to close this for now. Maybe we can revisit it in the future.

fanquake closed this on Feb 3, 2020

luke-jr referenced this in commit 8f938a7b55 on Feb 9, 2020

DrahtBot locked this on Feb 15, 2022