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

Init sync: conditional syncing to finalized slot #7999

Merged
merged 9 commits into from
Dec 3, 2020

Conversation

farazdagi
Copy link
Contributor

@farazdagi farazdagi commented Nov 30, 2020

What type of PR is this?

Other

What does this PR do? Why is it needed?

  • When node's head is already beyond the finalized slot, phase1 sync should be skipped (and phase2 -- sync to best-known non-finalized slot -- should commence).
  • This will also help with More robust init-sync on detached head block #7990 (init-sync will skip the 1st phase, and continue immediately into the 2nd, re-syncing immediately; otherwise, it stays in 1st phase until chain sufficiently advances -- we check that requested slot is lower than finalized one in 1st phase, and do not do so in 2nd).
  • This PR:
    • extracts phase1/phase2 sync code into the separate methods
    • adds additional check in phase1 code, to see if chain.HeadSlot() >= highestFinalizedSlot

Which issues(s) does this PR fix?

N/A

Other notes for review

@farazdagi farazdagi added the Sync Sync (regular, initial, checkpoint) related issues label Nov 30, 2020
@farazdagi farazdagi self-assigned this Nov 30, 2020
@rauljordan rauljordan changed the base branch from master to develop November 30, 2020 21:36
@farazdagi farazdagi marked this pull request as ready for review December 3, 2020 09:59
@farazdagi farazdagi requested a review from a team as a code owner December 3, 2020 09:59
@farazdagi farazdagi added Ready For Review A pull request ready for code review OK to merge labels Dec 3, 2020
highestFinalizedSlot, err := helpers.StartSlot(s.highestFinalizedEpoch() + 1)
if err != nil {
return err
}
// Set to the last slot of the finalized epoch.
highestFinalizedSlot--
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how do we handle missing slot in this case?

Copy link
Contributor Author

@farazdagi farazdagi Dec 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not missing slot, we have a finalized epoch add +1 to it, and take the start slot of the next to finalized epoch. But since this method should sync to finalized slot only, we need to get the last slot of the finalized epoch, not the first slot of the next epoch.

Note: since we are adding +1 to finalized epoch, then even if it finalized epoch is zero, we get 1st epoch (so, can substract w/o underflow)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although highestFinalizedEpoch cannot be zero (since we are using start slot of the next to finalized epoch), I still added check for zero, just in case some logic on how we first setting up the highestFinalizedSlot changes (and we will be open to underflow).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for clarifying

@prylabs-bulldozer prylabs-bulldozer bot merged commit 3092f75 into develop Dec 3, 2020
@delete-merged-branch delete-merged-branch bot deleted the init-sync-bypass-phase1-sync branch December 3, 2020 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review Sync Sync (regular, initial, checkpoint) related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants