polygon/sync: block downloader to not re-insert blocks behind start #11929
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
mh0lt
approved these changes
Sep 9, 2024
taratorio
added a commit
that referenced
this pull request
Oct 9, 2024
This PR captures several todos on my list of improving things related to Milestone hash mismatches that I noticed while testing Astrid on the tip: - after restarts, when we initialise the canonical chain builder, we need to set the root to the last finalised waypoint header and connect the latest known tip to it. Previously, we set the root to the latest known tip which may not be the finalised one. This caused unnecessary unwinds on startup (with potentially incorrect unwind num for the bridge.Unwind) and although we auto recover from it it is still better to fix it properly as it may cause unexpected behaviour further down the line - harden the milestone verifier to check for correct header connectivity via ParentHash and Number and also that the number of headers match with the number of headers in the Milestone because the Milestone is simply just the last header hash so a malicious peer may trick us easily - revert #11929 which, after taking a deeper look, seems to be an inaccurate interpretation of the error (prune of TD happens only for the blocks T-150_000). I believe this error may have been caused by something else, which may have been fixed by now (after numerous fixes to Astrid related to the parent td errors) - but if not it will be better to troubleshoot more thoroughly if the error ever arises again. In addition, this change is dangerous as it means we may falsely think we have all blocks for a given waypoint inserted in the DB, however that may not be true on restarts in the middle of waypoints, because the blocks inserted before the start may not be the right ones that are part of the waypoint (due to the tip not yet being finalised)
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.
Run into an issue since we started pruning total difficulty.
It happened for checkpoint 9703. Our start block was in the middle of the checkpoint range which meant we have to fetch all the 8k blocks in this checkpoint to verify the checkpoint root hash when receiving blocks from the peer.
The current logic will attempt to insert all these 8k blocks and it will fail with a missing parent td error because we only keep the last 1000 parent td records.
This PR fixes this by enhancing the block downloader to not re-insert blocks behind the
startblock. This solves the parent td error and also is saving some unnecessary inserts on the first waypoint processing on startup.