[rpc] Add initialblockdownload to getblockchaininfo #11258

jnewbery commented at 6:14 PM on September 6, 2017: member

Exposing whether the node is in IBD would help for testing, and may be useful in general, particularly for developers.

First discussed in #10357 here: #10357#pullrequestreview-59963870

... we could simplify this (and possibly other) tests by just adding a way to know if a node is in IBD. I'd like to do that, but I'm not sure it makes sense to complicate this PR with discussion over how that information should be made available. Eg it's not clear to me that the notion of being in IBD is worth exposing to the casual user, versus a hidden rpc call or something, since the definition has changed over time, and may continue to change in the future. But I still do agree that at least for testing purposes it would be far simpler to expose the field somehow...

This PR currently implements the simplest way of doing this: adding an initialblockdownload field to getblockchaininfo. Other approaches we could take:

add a new debug RPC method that exposes IBD and potentially other information.
add a parameter to getblockchaininfo, eg debug_info, which would cause it to return debug information including IBD
add a query string to the url ?debug=true which would cause RPCs to return additional debug information.

I quite like the idea of (3). Feedback on these and other approaches very much welcomed! @sdaftuar @laanwj

laanwj added the label RPC/REST/ZMQ on Sep 6, 2017

laanwj commented at 8:36 PM on September 6, 2017: member

Concept ACK

add a query string to the url ?debug=true which would cause RPCs to return additional debug information.

I don't like adding query string arguments to the RPC mechanism. This would add yet another way of passing in arguments, which is confusing, IMO. If this is to be made optional, adding an optional debug argument to get*info seems to be a better way, as it works within the system.

TheBlueMatt commented at 8:50 PM on September 6, 2017: member

utACK eb45b179abe2a79a990fb8c0a437654e8aa6b653

in src/rpc/blockchain.cpp:1134 in eb45b179ab outdated

1130 | @@ -1131,6 +1131,7 @@ UniValue getblockchaininfo(const JSONRPCRequest& request)
1131 |              "  \"difficulty\": xxxxxx,     (numeric) the current difficulty\n"
1132 |              "  \"mediantime\": xxxxxx,     (numeric) median time for the current best block\n"
1133 |              "  \"verificationprogress\": xxxx, (numeric) estimate of verification progress [0..1]\n"
1134 | +            "  \"initialblockdownload\": xxxx, (bool) is this node in Initial Block Download mode\n"

MarcoFalke commented at 8:54 PM on September 6, 2017:

Maybe just mention that the meaning may change any time and it is only meant for debugging?

promag commented at 8:58 PM on September 6, 2017:

s/mode/state?

promag commented at 8:58 PM on September 6, 2017:

Nit, align descriptions 🙄.

promag commented at 9:05 PM on September 6, 2017:

I think it is useful in general.

laanwj commented at 9:07 PM on September 6, 2017:

I agree with @MarcoFalke, even though it might be useful in general this should mention that the interpretation (and possibly even use) of this is internal and subject to change.

in test/functional/blockchain.py:182 in eb45b179ab outdated

 102 | +
 103 | +        assert_equal(res["chain"], "regtest")
 104 | +        assert_equal(res["blocks"], 200)
 105 | +        assert_equal(res["headers"], 200)
 106 | +        assert_equal(len(res["bestblockhash"]), 64)
 107 | +        assert "difficulty" in res

promag commented at 9:01 PM on September 6, 2017:

Instead of multiple assert str in res, how about assert_equal(res.keys(), ...)? This way if someone adds a new response field or changes the response, it must update this tests too.

So only assert known values, and exact keys.

promag commented at 9:02 PM on September 6, 2017: member

Concept ACK.

promag commented at 9:16 PM on September 6, 2017: member

Another approach is to create the generic read-only calls:

liststates returns an array of state names that can be queried;
querystate state_name returns the state value;
waitstate state_name (timeout) waits for state change or timeout and returns the state value;

In this case: bitcoin-cli querystate initialblockdownload.

Well, just an approach as @jnewbery asked 😄.

laanwj commented at 9:30 PM on September 6, 2017: member

@promag The idea is ok, though one problem is that that creates cross-cutting RPC calls, whereas we've been moving to subsystem-specific ones for a while (this was one of the reasons to get rid of the generic getinfo).

promag commented at 11:16 PM on September 6, 2017: member

one problem is that that creates cross-cutting RPC calls

I don't agree with that, it only forces the same RPC API for all states, but each subsystem deals with it's states, it's not like getinfo which is a mix of data.

So there would be a middleware where each subsystem must declare the available states and the RPC handler just delegates the state evaluation there.

In other words, I also disagree with having fat RPC's, but I agree with one horizontal API. For instance, the config is horizontal for all subsystems.

gmaxwell commented at 9:30 PM on September 8, 2017: contributor

Concept ACK but I'm somewhat concerned that IsInitialBlockDownload means something that is not equal to what the user means by "in initial block download". A node is unable to know if its caught up or not, and this is a somewhat reliable heuristic. But for example, if you have a node partitioned off on some fork or something it may well return false here. The concern might be addressable by just throwing the word "estimated" or similar in there somewhere.

morcos commented at 6:31 PM on September 11, 2017: member

ACK eb45b17

I agree slightly better to add some warning to the description of IBD, but that's a nit as far as I'm concerned

jnewbery force-pushed on Sep 12, 2017

jnewbery commented at 3:09 PM on September 12, 2017: member

Added wording "The use and interpretation of this property may change between releases."
added whitespace-only commit to fix up alignment for help text
rebased on master

MarcoFalke commented at 3:36 PM on September 12, 2017: member

Please remove the [WIP], when ready for review/merge

jnewbery renamed this:
~~[WIP] [rpc] Add initialblockdownload to getblockchaininfo~~
[rpc] Add initialblockdownload to getblockchaininfo
on Sep 12, 2017

jnewbery commented at 4:33 PM on September 12, 2017: member

Ready for review/merge. PR title updated.

Travis failed for The command "if [ -d ~/.bitcoin ]; then false; fi" exited with 1. Does the cache need to be cleared?

sipa commented at 4:40 PM on September 12, 2017: member

@jnewbery Rebase to fix that.

MarcoFalke commented at 4:41 PM on September 12, 2017: member

This was already rebased and we have the rm -rf in travis' yaml. I think the issue needs more investigation. (Might want to revert from the travis' yaml temporarily).

jnewbery force-pushed on Oct 26, 2017

jnewbery commented at 3:58 PM on October 26, 2017: member

Rebased.

I've updated the help text to:

"initialblockdownload": xxxx, (bool) (debug information) estimate of whether this node is in Initial Block Download mode.

to address Greg's comment: #11258 (comment)

No functional changes.

I think this is ready for merge.

[rpc] Add initialblockdownload to getblockchaininfo bd9c18171d

[trivial] (whitespace only) fix getblockchaininfo alignment 11413646be

jnewbery force-pushed on Oct 26, 2017

jnewbery commented at 4:12 PM on October 26, 2017: member

Silent rebase conflict. Should be fixed now

luke-jr commented at 9:14 AM on November 10, 2017: member

The current commits only add comments to the functional test (claiming it tests it), but doesn't actually add any tests...?

jnewbery commented at 5:53 PM on November 10, 2017: member

The current commits only add comments to the functional test (claiming it tests it), but doesn't actually add any tests...?

https://github.com/bitcoin/bitcoin/pull/11258/files#diff-31748911f612ce1d09bc82d04f452592R62 adds initialblockdownload to the expected keys returned in getblockchaininfo

promag commented at 6:45 PM on November 10, 2017: member

Unless @luke-jr expects a test for the value.

jnewbery commented at 8:14 PM on November 10, 2017: member

Unless @luke-jr expects a test for the value.

Almost none of the fields from getblockchaininfo are tested for value. PRs welcome to change that.

It's slightly frustrating that this is a minimal functionality PR that has been open for 2 months, ACK'ed by several people, rebased many times, and is now getting nitted because the test coverage for the new functionality doesn't exceed the test coverage for existing functionality. The fact that we're getting down to these micronits suggests it's time to merge this PR (or abandon it).

achow101 commented at 8:16 PM on November 10, 2017: member

utACK 11413646be2a6d0bf1c857753bfcec0af60c72b8

sipa merged this on Nov 11, 2017

sipa closed this on Nov 11, 2017

sipa referenced this in commit 033c78671b on Nov 11, 2017

jnewbery deleted the branch on Nov 11, 2017

luke-jr referenced this in commit 78861e1edd on Nov 11, 2017

luke-jr referenced this in commit 4007c82946 on Nov 11, 2017

PastaPastaPasta referenced this in commit 480401cd42 on Jan 17, 2020

PastaPastaPasta referenced this in commit b4fbfe443b on Jan 22, 2020

PastaPastaPasta referenced this in commit c5a7046e93 on Jan 22, 2020

PastaPastaPasta referenced this in commit c7ed85fac5 on Jan 29, 2020

ckti referenced this in commit e7b99021ac on Mar 28, 2021

DrahtBot locked this on Sep 8, 2021