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

Add block download progress to metrics UI #2484

Merged
merged 3 commits into from Jul 29, 2017

Conversation

@str4d
Copy link
Contributor

commented Jun 26, 2017

No description provided.

@str4d str4d added this to the 1.0.11 Release milestone Jun 26, 2017

@daira daira removed this from the 1.0.11 Release milestone Jul 3, 2017

@daira

daira approved these changes Jul 3, 2017

Copy link
Contributor

left a comment

utACK modulo comments.

double averageSpacing = (targetSpacing + checkpointSpacing) / 2;
int netheight = medianHeight + ((GetTime() - tipmediantime) / averageSpacing);
// Round to nearest ten to reduce noise
netheight = (netheight / 10) * 10;

This comment has been minimized.

Copy link
@daira

daira Jul 3, 2017

Contributor

This is not rounding to the nearest 10; it's rounding down to a multiple of 10.

// checkpoint, and use that to estimate the current network height.
auto chainParams = Params();
auto checkpointData = chainParams.Checkpoints();
int medianHeight = height - 5;

This comment has been minimized.

Copy link
@daira

daira Jul 3, 2017

Contributor

Use CBlockIndex::nMedianTimeSpan / 2 instead of 5.

@str4d str4d force-pushed the str4d:block-download-completion branch from 92bea1d to 4d9c9e0 Jul 11, 2017

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2017

@daira comments addressed.

@daira

daira approved these changes Jul 11, 2017

Copy link
Contributor

left a comment

utACK. I'm not sure what our testing policy is for changes that obviously only affect non-essential output; do we need to test this @nathan-at-least ?

@daira daira added this to the 1.0.11 Release milestone Jul 25, 2017

@nathan-at-least
Copy link
Contributor

left a comment

Requested change: unit tests of arithmetic.

int netheight = medianHeight + ((GetTime() - tipmediantime) / averageSpacing);
// Round to nearest ten to reduce noise
netheight = ((netheight + 5) / 10) * 10;
int downloadPercent = height * 100 / netheight;

This comment has been minimized.

Copy link
@nathan-at-least

nathan-at-least Jul 25, 2017

Contributor

Imagine there were some bug in the arithmetic. Could we factor out the "sensor" code (which reads from blockchain state, ex IsInitialBlockDownload() and chainParams.Checkpoints, arithmetic functional code, and display code, and then unittest at least the arithmetic functional code?

Then if there were an arithmetic bug, that would imply either we missed a test specification, or the specification was buggy.

This comment has been minimized.

Copy link
@daira

daira Jul 25, 2017

Contributor

@str4d will write a unit test for this.

@daira daira assigned str4d and unassigned str4d Jul 25, 2017

@str4d str4d force-pushed the str4d:block-download-completion branch from 4d9c9e0 to 047aec1 Jul 26, 2017

@str4d

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2017

Refactored the code and added unit tests. I force-pushed to squash this into the existing commit.

@daira

daira approved these changes Jul 26, 2017

Copy link
Contributor

left a comment

ut(ACK+cov)

// We average the target spacing with the observed spacing to the last
// checkpoint, and use that to estimate the current network height.
int medianHeight = height - CBlockIndex::nMedianTimeSpan / 2;
double checkpointSpacing = (double (tipmediantime - timeLastCheckpoint)) / (medianHeight - heightLastCheckpoint);

This comment has been minimized.

Copy link
@ebfull

ebfull Jul 26, 2017

Contributor

Under what conditions could this potentially divide by zero?

This comment has been minimized.

Copy link
@nathan-at-least

nathan-at-least Jul 27, 2017

Contributor

Also, can checkpointSpacing become negative in pathological cases?

This comment has been minimized.

Copy link
@daira

daira Jul 27, 2017

Contributor

double division-by-zero is well-defined (for IEEE arithmetic) to produce infinity or -infinity. Dividing a double by an infinity gives zero.

This comment has been minimized.

Copy link
@daira

daira Jul 28, 2017

Contributor

Provided we have floating point non-stop mode enabled (via feholdexcept or fesetexceptflag). I assumed we did because we use infinities in the fee handling code.

This comment has been minimized.

Copy link
@daira

daira Jul 28, 2017

Contributor

I suggest avoiding the issue by using:

double checkpointSpacing = max(1.0d, (double (tipmediantime - timeLastCheckpoint)) / max(1.0d, double (medianHeight - heightLastCheckpoint)));

(and adding tests that exercise the clamping).

This comment has been minimized.

Copy link
@str4d

str4d Jul 28, 2017

Author Contributor

@ebfull raises a good point. I have addressed this in a different way: while the current height is <= the last checkpoint, estimate the checkpoint spacing using the interval [genesis, lastCheckpoint] instead of [lastCheckpoint, tip].

@nathan-at-least
Copy link
Contributor

left a comment

utACK.

There are some pathological cases and I'm not sure if they are reachable.

If not, an improvement would be to add precondition assertions to guard against undefined behavior (if that is indeed C++ divide by zero behavior). Otherwise test cases for them and error recovery would be good. I don't see either of those as requirements for merging this.

// We average the target spacing with the observed spacing to the last
// checkpoint, and use that to estimate the current network height.
int medianHeight = height - CBlockIndex::nMedianTimeSpan / 2;
double checkpointSpacing = (double (tipmediantime - timeLastCheckpoint)) / (medianHeight - heightLastCheckpoint);

This comment has been minimized.

Copy link
@nathan-at-least

nathan-at-least Jul 27, 2017

Contributor

Also, can checkpointSpacing become negative in pathological cases?

int medianHeight = height - CBlockIndex::nMedianTimeSpan / 2;
double checkpointSpacing = (double (tipmediantime - timeLastCheckpoint)) / (medianHeight - heightLastCheckpoint);
double averageSpacing = (targetSpacing + checkpointSpacing) / 2;
int netheight = medianHeight + ((GetTime() - tipmediantime) / averageSpacing);

This comment has been minimized.

Copy link
@nathan-at-least

nathan-at-least Jul 27, 2017

Contributor

Under what conditions could averageSpacing == 0? Three cases to consider:

  • targetSpacing == checkpointSpacing == 0
  • One is negative (which seem unexpected).
  • The other is negative.
Checkpoints::GetTotalBlocksEstimate(checkpointData),
checkpointData.nTimeLastCheckpoint,
chainParams.GetConsensus().nPowTargetSpacing);
}

This comment has been minimized.

Copy link
@nathan-at-least

nathan-at-least Jul 27, 2017

Contributor

I personally like factoring out the "I/O" into this outer function. What's your opinion @str4d?

This comment has been minimized.

Copy link
@daira

daira Jul 28, 2017

Contributor

I like it too.

This comment has been minimized.

Copy link
@str4d

str4d Jul 28, 2017

Author Contributor

Could you clarify what you mean? If you mean pulling the stdio calls from the parent into here, I would like to point out that having them in their current function makes aligning the UI significantly easier, as all the common calls are together (in this case, inside printStats()).

height, tipmediantime,
Checkpoints::GetTotalBlocksEstimate(checkpointData),
checkpointData.nTimeLastCheckpoint,
chainParams.GetConsensus().nPowTargetSpacing);

This comment has been minimized.

Copy link
@nathan-at-least

nathan-at-least Jul 27, 2017

Contributor

Answering my edge-case questions above:

  • nPowTargetSpacing is defined in one place as an int64_t and is our target block time and it would be quite unlikely we wouldn't notice changing this to negative.
  • I haven't determined what the pathological conditions for checkpointData.nTimeLastCheckpoint or Checkpoints::GetTotalBlockEstimate() would be.

I suspect the worst case behavior for those pathological conditions would be just incorrect display, although it sounds like division by 0 can be undefined behavior.

@daira

This comment has been minimized.

Copy link
Contributor

commented Jul 28, 2017

Note that we're behind schedule on the release and this is the last feature PR. It's in danger of getting kicked out if the edge case issues are not resolved today (Friday).

str4d added some commits Jul 28, 2017

Correct and extend EstimateNetHeightInner tests
Corrections are to the median block times, which were generated by subtracting
CBlockIndex::nMedianTimeSpan / 2 from the block height and then multiplying by
the target spacing. GetMedianTimePast() takes an array sorted by std::sort() and
returns element CBlockIndex::nMedianTimeSpan / 2, meaning that if
CBlockIndex::nMedianTimeSpan is odd (which it is), there is an out-by-one error
in the subtraction.
@daira

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2017

ut(ACK+cov)

@daira

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2017

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2017

📌 Commit 92bfde0 has been approved by daira

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2017

⌛️ Testing commit 92bfde0 with merge 10acd8a...

zkbot added a commit that referenced this pull request Jul 29, 2017

Auto merge of #2484 - str4d:block-download-completion, r=daira
Add block download progress to metrics UI
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2017

☀️ Test successful - pr-merge
Approved by: daira
Pushing 10acd8a to master...

@zkbot zkbot merged commit 92bfde0 into zcash:master Jul 29, 2017

1 check passed

homu Test successful
Details

@str4d str4d deleted the str4d:block-download-completion branch Aug 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.