Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpc: query general daemon information via RPC #17958

Closed
wants to merge 1 commit into from

Conversation

@brakmic
Copy link
Contributor

brakmic commented Jan 18, 2020

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 brakmic changed the title rpc: return general daemon information to client rpc: query general daemon information via RPC Jan 18, 2020
Copy link

msafi left a comment

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

obj.pushKV("startuptime", FormatISO8601DateTime(GetStartupTime()));
return obj;

}

This comment has been minimized.

Copy link
@msafi

msafi Jan 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?

This comment has been minimized.

Copy link
@brakmic

brakmic Jan 18, 2020

Author Contributor

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?

src/rpc/misc.cpp Outdated Show resolved Hide resolved
" \"useragent\":\"client name\", (string) Client name\n"
" \"datadir\":\"datadir path\", (string) Data directory path\n"
" \"blocksdir\":\"blocksdir path\", (string) Blocks directory path\n"
" \"startuptime\":\"string\", (string) Startup time\n"

This comment has been minimized.

Copy link
@msafi

msafi Jan 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.

This comment has been minimized.

Copy link
@brakmic

brakmic Jan 18, 2020

Author Contributor

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? :)

@brakmic brakmic force-pushed the brakmic:rpc-implement-getgeneralinfo branch 5 times, most recently from 54a6998 to 9c65cb5 Jan 18, 2020
Copy link
Member

jonatack left a comment

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

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

src/rpc/misc.cpp Outdated Show resolved Hide resolved
test/functional/rpc_getgeneralinfo.py Outdated Show resolved Hide resolved
test/functional/rpc_getgeneralinfo.py Outdated Show resolved Hide resolved
test/functional/rpc_getgeneralinfo.py Show resolved Hide resolved
test/functional/rpc_getgeneralinfo.py Outdated Show resolved Hide resolved
@brakmic brakmic force-pushed the brakmic:rpc-implement-getgeneralinfo branch from 9c65cb5 to 714b0d0 Jan 18, 2020
@brakmic

This comment has been minimized.

Copy link
Contributor Author

brakmic commented Jan 18, 2020

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

@brakmic brakmic force-pushed the brakmic:rpc-implement-getgeneralinfo branch 2 times, most recently from b9683a5 to e69f468 Jan 18, 2020
@jonatack

This comment has been minimized.

Copy link
Member

jonatack commented Jan 18, 2020

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

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Jan 18, 2020

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

This comment has been minimized.

Copy link
Contributor

emilengler commented Jan 19, 2020

Why isn't the BerkeleyDB version included?

@brakmic

This comment has been minimized.

Copy link
Contributor Author

brakmic commented Jan 19, 2020

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

This comment has been minimized.

Copy link
Contributor

emilengler commented Jan 19, 2020

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

This comment has been minimized.

Copy link
Contributor Author

brakmic commented Jan 19, 2020

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.

"""Check if 'getgeneralinfo' is callable."""
try_rpc(None, None, self.nodes[0].getgeneralinfo, None, None)
"""Check if 'getgeneralinfo' is idempotent (multiple requests return same data)."""
json1 = self.nodes[0].getgeneralinfo()

This comment has been minimized.

Copy link
@emilengler

emilengler Jan 19, 2020

Contributor

Why not put this into a tuple

This comment has been minimized.

Copy link
@brakmic

brakmic Jan 19, 2020

Author Contributor

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.

This comment has been minimized.

Copy link
@jonatack

jonatack Jan 19, 2020

Member

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.

This comment has been minimized.

Copy link
@brakmic

brakmic Jan 19, 2020

Author Contributor

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.

This comment has been minimized.

Copy link
@jonatack

This comment has been minimized.

Copy link
@msafi

msafi Jan 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!

This comment has been minimized.

Copy link
@jonatack

jonatack Jan 19, 2020

Member

@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

Copy link
Member

MarcoFalke left a comment

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

@msafi

This comment has been minimized.

Copy link

msafi commented Jan 19, 2020

@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

This comment has been minimized.

Copy link
Member

MarcoFalke commented Jan 19, 2020

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

This comment has been minimized.

Copy link
Contributor

emilengler commented Jan 19, 2020

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

This comment has been minimized.

Copy link
Member

jonatack commented Jan 19, 2020

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

This comment has been minimized.

Copy link

msafi commented Jan 19, 2020

@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

This comment has been minimized.

Copy link
Member

jonasschnelli commented Jan 20, 2020

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

This comment has been minimized.

Copy link

msafi commented Jan 20, 2020

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

This comment has been minimized.

Copy link
Member

jonasschnelli commented Jan 20, 2020

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

This comment has been minimized.

Copy link
Member

jonatack commented Jan 20, 2020

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

This comment has been minimized.

Copy link
Member

fanquake commented Jan 20, 2020

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

This comment has been minimized.

Copy link
Member

promag commented Jan 20, 2020

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

This comment has been minimized.

Copy link
Contributor

emilengler commented Jan 20, 2020

@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

This comment has been minimized.

Copy link
Member

promag commented Jan 20, 2020

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

This comment has been minimized.

Copy link

msafi commented Jan 20, 2020

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

This comment has been minimized.

Copy link
Contributor

emilengler commented Jan 20, 2020

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

src/rpc/misc.cpp Outdated Show resolved Hide resolved
@brakmic brakmic force-pushed the brakmic:rpc-implement-getgeneralinfo branch from e69f468 to 40a8395 Jan 27, 2020
@brakmic brakmic force-pushed the brakmic:rpc-implement-getgeneralinfo branch from 40a8395 to cdbd38d Jan 27, 2020
@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Feb 2, 2020

Needs rebase
@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Feb 3, 2020

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

@fanquake fanquake closed this Feb 3, 2020
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Feb 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants
You can’t perform that action at this time.