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

Block beacon header sync until global pivot is updated #7614

Merged
merged 11 commits into from
Oct 22, 2024

Conversation

marcindsobczak
Copy link
Contributor

Fixes #6304

Changes

  • block a start of beacon header sync until global pivot is updated

Beacon header sync can be initialized only when global pivot is already set, otherwise it might result in conflicting pivots and a deadlock

Types of changes

What types of changes does your code introduce?

  • Bugfix (a non-breaking change that fixes an issue)
  • New feature (a non-breaking change that adds functionality)
  • Breaking change (a change that causes existing functionality not to work as expected)
  • Optimization
  • Refactoring
  • Documentation update
  • Build-related changes
  • Other: Description

@marcindsobczak marcindsobczak marked this pull request as ready for review October 16, 2024 14:00
@kamilchodola
Copy link
Contributor

While fixing that did you managed to find a proper reproduction for that? For me it is quite random and did not noticed what exactly I need to do to make it fully reproducible.

@marcindsobczak
Copy link
Contributor Author

@kamilchodola I didn't reproduced an issue, but I reproduced the root cause of an issue - setting beacon pivot and starting beacon header sync before setting global pivot. It is still working fine most of the time, but rarely pivots are conflicting and it is breaking the node - I didn't reproduced that part. This PR is fixing the root cause, so we will not have this issue anymore

@MarekM25
Copy link
Contributor

@kamilchodola @marcindsobczak While fixing the block production issue, I encountered a few syncing issues with beacon headers. I think this can happen when your CL is not close to the head and needs to catch up to the tip of the chain.

Copy link
Contributor

@MarekM25 MarekM25 left a comment

Choose a reason for hiding this comment

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

PR looks good, but @kamilchodola it will require some testing for sure. I guess a good idea is to restart CL (lighthouse)/EL a few times and trigger a new beacon sync in this way.

@marcindsobczak think about if it can be covered somehow by integreation tests

@marcindsobczak marcindsobczak merged commit 7588e22 into master Oct 22, 2024
73 checks passed
@marcindsobczak marcindsobczak deleted the fix/pivots branch October 22, 2024 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node stuck with sync when PivotUpdator changed block for second time based on FCU
4 participants