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] Add initialblockdownload to getblockchaininfo #11258

Merged
merged 2 commits into from Nov 11, 2017

Conversation

Projects
None yet
10 participants
@jnewbery
Member

jnewbery commented Sep 6, 2017

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

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

  1. add a new debug RPC method that exposes IBD and potentially other information.
  2. add a parameter to getblockchaininfo, eg debug_info, which would cause it to return debug information including IBD
  3. 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 laanwj added the RPC/REST/ZMQ label Sep 6, 2017

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Sep 6, 2017

Member

Concept ACK

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

Member

laanwj commented Sep 6, 2017

Concept ACK

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

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Sep 6, 2017

Contributor

utACK eb45b17

Contributor

TheBlueMatt commented Sep 6, 2017

utACK eb45b17

@promag

Concept ACK.

Show outdated Hide outdated src/rpc/blockchain.cpp
Show outdated Hide outdated test/functional/blockchain.py
@promag

This comment has been minimized.

Show comment
Hide comment
@promag

promag Sep 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 😄.

Member

promag commented Sep 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Sep 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).

Member

laanwj commented Sep 6, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@promag

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

Member

promag commented Sep 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Sep 8, 2017

Member

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.

Member

gmaxwell commented Sep 8, 2017

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

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Sep 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

Member

morcos commented Sep 11, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery Sep 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
Member

jnewbery commented Sep 12, 2017

  • 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

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Sep 12, 2017

Member

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

Member

MarcoFalke commented Sep 12, 2017

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

@jnewbery jnewbery changed the title from [WIP] [rpc] Add initialblockdownload to getblockchaininfo to [rpc] Add initialblockdownload to getblockchaininfo Sep 12, 2017

@jnewbery

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery Sep 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?

Member

jnewbery commented Sep 12, 2017

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Sep 12, 2017

Member

@jnewbery Rebase to fix that.

Member

sipa commented Sep 12, 2017

@jnewbery Rebase to fix that.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Sep 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).

Member

MarcoFalke commented Sep 12, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jnewbery

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

Member

jnewbery commented Oct 26, 2017

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.

@jnewbery

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery Oct 26, 2017

Member

Silent rebase conflict. Should be fixed now

Member

jnewbery commented Oct 26, 2017

Silent rebase conflict. Should be fixed now

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Nov 10, 2017

Member

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

Member

luke-jr commented Nov 10, 2017

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

@jnewbery

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery Nov 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

Member

jnewbery commented Nov 10, 2017

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

This comment has been minimized.

Show comment
Hide comment
@promag

promag Nov 10, 2017

Member

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

Member

promag commented Nov 10, 2017

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

@jnewbery

This comment has been minimized.

Show comment
Hide comment
@jnewbery

jnewbery Nov 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).

Member

jnewbery commented Nov 10, 2017

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

This comment has been minimized.

Show comment
Hide comment
@achow101
Member

achow101 commented Nov 10, 2017

utACK 1141364

@sipa sipa merged commit 1141364 into bitcoin:master Nov 11, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

sipa added a commit that referenced this pull request Nov 11, 2017

Merge #11258: [rpc] Add initialblockdownload to getblockchaininfo
1141364 [trivial] (whitespace only) fix getblockchaininfo alignment (John Newbery)
bd9c181 [rpc] Add initialblockdownload to getblockchaininfo (John Newbery)

Pull request description:

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

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

  1. add a new debug RPC method that exposes `IBD` and potentially other information.
  2. add a parameter to `getblockchaininfo`, eg `debug_info`, which would cause it to return debug information including IBD
  3. 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

Tree-SHA512: a6dedd47f8c9bd38769cc597524466250041136feb33500644b9c48d0ffe4e3eeeb2587b5bbc6420364ebdd2667df807fbb50416f9a7913bbf11a14ea86dc0d4

@jnewbery jnewbery deleted the jnewbery:expose_ibd branch Nov 11, 2017

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Nov 11, 2017

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Nov 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment