-
Notifications
You must be signed in to change notification settings - Fork 925
Add failing test for a sync race condition scenario #8039
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
base: unstable
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch and good test! Some minor comments
I had a look at the failing checkpoint sync tests (optionally triggered jobs), and I don't believe they are related to changes from this PR, i think what's happening is:
|
@dapplion thanks for the review! I've addressed most of your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, running this on devnet 3 i'm getting a stuck chain with 40 peers.
Need to investigate further
Thanks for raising this Pawan. Sorry i haven't managed to get to this today, will look into this tomorrow. |
@pawanjay176 im going to remove the clean ups from this PR so we can merge this, because the flaky sync test is really quite annoying 😅 |
…arge number of `AwaitingDownload` batches now this is a valid in-between state
This pull request has merge conflicts. Could you please resolve them @jimmygchen? 🙏 |
Issue Addressed
This bug was discovered when the CI kurtosis sync tests got stuck in
SyncTransition
for a long time:https://github.com/sigp/lighthouse/actions/runs/17632496975/job/50102884940
I've added a failing test that replicate the exact scenario, and made a fix for it.
NOTE: I haven't done any manual testing yet.