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: Add window final block height to getchaintxstats #16695

Merged
merged 1 commit into from Aug 29, 2019

Conversation

@leto
Copy link
Contributor

leto commented Aug 23, 2019

This patch is motivated by the desire to make the output of getchaintxstats more useful and optimized for applications to consume and render the data.

Firstly, this data is already available to the RPC, no additional work is done. Currently additional RPC calls will be needed to look up the height of the final block in the window or the block height that began the window.

By adding the block height of the final block in the window, the JSON is "self-contained" and applications can calculate the exact block height range of the window with no additional RPC requests.
For example, a web application which wants to render historical information for getchaintxstats RPC on various window sizes might call the RPC with various window lengths, once per day, and store the JSON results somewhere. Because the final block height of each dataset is included, it's no extra work to determine the exact block window range of each JSON response.

@leto

This comment has been minimized.

Copy link
Contributor Author

leto commented Aug 23, 2019

FYI all travis CI tests passed, but one job which uses TSAN timed out with: No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

@MarcoFalke MarcoFalke changed the title Add window final block height to getchaintxstats rpc: Add window final block height to getchaintxstats Aug 23, 2019
@MarcoFalke MarcoFalke removed the Tests label Aug 23, 2019
@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Aug 23, 2019

Code review utACK. No opinion on concept.

@leto

This comment has been minimized.

Copy link
Contributor Author

leto commented Aug 24, 2019

This code does not seem to actually conflict with 16539, that looks like a false positive from the bot

@promag

This comment has been minimized.

Copy link
Member

promag commented Aug 25, 2019

ACK 9a018e7, could have minor release note though.

@leto

This comment has been minimized.

Copy link
Contributor Author

leto commented Aug 25, 2019

@promag I added a new temporary release note file as I saw documented in the dev notes, please let me know if I did it correctly. This is the addition which I put in the Updated RPCs section:

+- The `getchaintxstats` RPC now returns the additional key of
+  `window_final_block_height`
@bitcoin bitcoin deleted a comment from DrahtBot Aug 25, 2019
@promag

This comment has been minimized.

Copy link
Member

promag commented Aug 25, 2019

This is enough:

Updated RPCs
------------

- The `getchaintxstats` RPC now returns the additional key of
  `window_final_block_height`.

which will be manually merged in the final release notes.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Aug 25, 2019

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

Conflicts

No conflicts as of last run.

@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Aug 29, 2019

@leto Could you follow up here with the smaller release notes?

@leto

This comment has been minimized.

Copy link
Contributor Author

leto commented Aug 29, 2019

@fanquake Sorry, I didn't understand that as a request. I thought I followed the dev notes exactly, and then @promag summarized them above.

What exactly is being asked?

@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Aug 29, 2019

What exactly is being asked?

@leto no problem. Basically we'd just like you to modify the release-notes-16695.md file to only include the text which is relevant to this pull request. i.e only this:

Updated RPCs
------------

- The `getchaintxstats` RPC now returns the additional key of
  `window_final_block_height`.

You can checkout the release notes changes in this PR, #16647, as an example of what I mean.

@promag

This comment has been minimized.

Copy link
Member

promag commented Aug 29, 2019

Just have that in the release note file.

@leto

This comment has been minimized.

Copy link
Contributor Author

leto commented Aug 29, 2019

@promag @fanquake ok, just fixed that with a new commit. I can also squash these 3 commits into 1 if that is preferred

@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Aug 29, 2019

just fixed that with a new commit. I can also squash these 3 commits into 1 if that is preferred

Thanks for the quick follow up. Yes please squash your commits.

The getchaintxstats RPC now returns the additional key of window_final_block_height
@leto leto force-pushed the leto:getchaintxstats_height branch from 0e0c902 to d48c1e8 Aug 29, 2019
@leto

This comment has been minimized.

Copy link
Contributor Author

leto commented Aug 29, 2019

@fanquake latest push has everything in a single commit. Hopefully Travis likes it

Copy link
Member

promag left a comment

ACK d48c1e8.

laanwj added a commit that referenced this pull request Aug 29, 2019
d48c1e8 Add window final block height to getchaintxstats (Jonathan "Duke" Leto)

Pull request description:

  This patch is motivated by the desire to make the output of `getchaintxstats` more useful and optimized for applications to consume and render the data.

  Firstly, this data is already available to the RPC, no additional work is done. Currently additional RPC calls will be needed to look up the height of the final block in the window or the block height that began the window.

  By adding the block height of the final block in the window, the JSON is "self-contained" and applications can calculate the exact block height range of the window with no additional RPC requests.
  For example, a web application which wants to render historical information for `getchaintxstats` RPC on various window sizes might call the RPC with various window lengths, once per day, and store the JSON results somewhere. Because the final block height of each dataset is included, it's no extra work to determine the exact block window range of each JSON response.

ACKs for top commit:
  promag:
    ACK d48c1e8.

Tree-SHA512: fd4952c125f81a4ad18f7c78498c6b3e265b93cb574832166ac25596321ce84957f971f3f78f37d7e42638dc65f2a5d4d760f289873c9c2f2a82eb00a0f87c3f
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Aug 29, 2019

ACK d48c1e8

@laanwj laanwj merged commit d48c1e8 into bitcoin:master Aug 29, 2019
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@leto leto deleted the leto:getchaintxstats_height branch Aug 30, 2019
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Sep 3, 2019
The getchaintxstats RPC now returns the additional key of window_final_block_height

Github-Pull: bitcoin#16695
Rebased-From: d48c1e8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.