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

Fix is_syncing API reporting when head is synced optimistically #8889

Merged
merged 1 commit into from
Dec 6, 2024

Conversation

StefanBratanov
Copy link
Contributor

@StefanBratanov StefanBratanov commented Dec 5, 2024

PR Description

When we optimistically track head and sync distance is 0, technically we should report is_syncing: false even though we are waiting for the EL.

Fixed Issue(s)

fixes #8888

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

Copy link
Contributor

@mehdi-aouadi mehdi-aouadi left a comment

Choose a reason for hiding this comment

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

I'm a bit bothered by the logic difference between the API and the SyncStateTracker: the latter doesn't take the slots behind into consideration at all to mark the node IN_SYNC. This definitely fixes the issue we have though.
I'm wondering if it wouldn't be better to first check the SyncStateTracker to see if the node is IN_SYNC otherwise consider checking the slot behind...
Not a strong opinion here, just making sure we cover all the cases...

@StefanBratanov
Copy link
Contributor Author

I'm a bit bothered by the logic difference between the API and the SyncStateTracker: the latter doesn't take the slots behind into consideration at all to mark the node IN_SYNC. This definitely fixes the issue we have though. I'm wondering if it wouldn't be better to first check the SyncStateTracker to see if the node is IN_SYNC otherwise consider checking the slot behind... Not a strong opinion here, just making sure we cover all the cases...

I had a look at the SyncStateTracker and we have this notion that if we are AWAITING_EL (tracking optimistic head with sync_distance = 0) then we are still syncing and isInSync() returns false. This cascades to a few places in the code, specifically around the fact that when we are tracking optimistic head, we shouldn't perform any duties and also in onTick(). I think I would raise an issue instead explaining this and leave this change relatively simple only impacting the API code.

@StefanBratanov
Copy link
Contributor Author

Raised #8890 to track usages of isInSync

Copy link
Contributor

@mehdi-aouadi mehdi-aouadi left a comment

Choose a reason for hiding this comment

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

Let's fix the API and we'll review the SyncStateTracker in the issue you raised. Thanks!

@StefanBratanov StefanBratanov enabled auto-merge (squash) December 6, 2024 16:17
@StefanBratanov StefanBratanov merged commit 1b68e2b into Consensys:master Dec 6, 2024
17 checks passed
@StefanBratanov StefanBratanov deleted the fix_is_syncing_api branch December 12, 2024 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Teku reports "is_syncing": true when sync_distance ==0
2 participants