Estimate network chain tip height based on local node time and current best tip #3492
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jvff
force-pushed
the
estimate-network-chain-tip-height
branch
from
6ddb5ef to
f604225
Compare
Codecov Report
@@ Coverage Diff @@
## main #3492 +/- ##
==========================================
+ Coverage 78.34% 78.46% +0.12%
==========================================
Files 267 274 +7
Lines 31526 32032 +506
==========================================
+ Hits 24698 25134 +436
- Misses 6828 6898 +70 |
jvff
force-pushed
the
estimate-network-chain-tip-height
branch
from
f604225 to
dbfaf34
Compare
|
Coverage failed due to #3489 |
3 tasks
teor2345
previously approved these changes
Feb 9, 2022
There was a problem hiding this comment.
This looks great.
I wanted to check if the estimate was actually accurate when used in zebrad, so I added a quick progress log in PR #3495. (This will also help us diagnose full sync failures.)
I have some optional suggestions, but feel free to merge the PR as it is.
The documentation was exactly the same as the documentation from the trait.
Simulate a block being added to the chain with a random block time based on the previous block time and the target spacing time.
Store the block time so that it's ready for a future chain that allows obtaining the chain tip's block time.
Allow obtaining the bes chain tip's block time.
Prevent any data races by returning both values so that they refer to the same chain tip.
Returns all the target spacings defined for a network.
Isolate the code to calculate the height estimation in a new type, so that it's easier to understand and doesn't decrease the readability of the `chain_tip.rs` file.
This is more of an extension method than a trait method. It uses the `NetworkChainTipHeightEstimator` to actually perform the estimation, but obtains the initial information from the current best chain tip.
There was an extra closing bracket in the summary line.
Prepare to allow mocking the block time of the best tip as well as the block height.
Add a separate `watch` channel to send the best tip block times from a `MockChainTipSender` to a `MockChainTip`. The `best_tip_height_and_block_time` implementation will only return a value if there's a height and a block time value for the best tip.
Use Euclidean division to force the division result to round down instead of rounding towards zero. This fixes an off-by-one error when estimating a height that is lower than the current height, because the fractionary result was being discarded, and it should have forced the height to go one block back.
Detect situations that might cause the block height estimate to underflow, and return the genesis height instead.
The implementation of `chrono::Duration::num_seconds` adds one to the number of seconds if it's negative. This breaks the division calculation, so it has to be compensated for.
Generate pairs of block heights and check that it's possible to estimate the larger height from the smaller height and a displaced time difference.
jvff
force-pushed
the
estimate-network-chain-tip-height
branch
from
dbfaf34 to
9a5b634
Compare
teor2345
approved these changes
Feb 10, 2022
teor2345
added
P-Medium ⚡
A-diagnostics
22 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
In order to properly estimate if chain synchronization has finished, we need to calculate an estimate of the network's chain tip height. This prepares for the work to add a test for mainnet full chain synchronization.
Solution
The estimate is calculated based on the current local node's system time and the current best tip's height and block time. The time difference between the current best tip's block time and the current time is calculated, and then the number of remaining blocks is calculated using the target spacing between blocks. Because Zcash has more than one target spacing, all of them are considered for their respective block height ranges.
The API for estimation is provided as a default method in the
ChainTipmethod. The actual estimation code is in a separateNetworkChainTipEstimatortype in a separate module. I ended up deciding to separate the code to avoid making the actualChainTiptrait hard to read.This closes #3161.
Review
@teor2345 can take a look. I'd especially like to know if the solution I've implemented is too generic and complicated, and if I should change it to a simpler one.
Reviewer Checklist
Follow Up Work