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

polygon/sync: initialise canonical chain builder correctly #12246

Merged
merged 7 commits into from
Oct 9, 2024

Conversation

taratorio
Copy link
Member

@taratorio taratorio commented Oct 8, 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 polygon/sync: block downloader to not re-insert blocks behind start #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)

@taratorio taratorio requested review from shohamc1 and mh0lt October 8, 2024 12:31
@taratorio taratorio merged commit ef5422a into main Oct 9, 2024
11 checks passed
@taratorio taratorio deleted the astrid-ccb-populate-last-waypoint-root branch October 9, 2024 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants