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 sync committee when syncing #8192

Merged
merged 8 commits into from
Apr 15, 2024

Conversation

tbenr
Copy link
Contributor

@tbenr tbenr commented Apr 12, 2024

fixes #8005

The aggregation fix is non-controversial while the sync message is not that clean, but it is the easiest implementation I came up with so far.

Essentially we will stop producing sync committee message if the head we have is older than 32 slot, which means:
1- we are connected to a BN which is falling out of sync.
2- the chain is not production a block for more than entire epoch, we have bigger problems.

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

@StefanBratanov StefanBratanov left a comment

Choose a reason for hiding this comment

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

LGTM

@tbenr
Copy link
Contributor Author

tbenr commented Apr 12, 2024

@rolfyone I'm not a fan of the solution for the sync committee. The aggregate is fine.

dumping some messages wrote in chat:

essentially VC tracks the head, creates the message, and publish to the BN

so this is the only duty that is not created by the BN
so the creation can not be directly associated to a syncing state
to do a similar thing on VC we should have a ChainHeadTracker that becomes aware of the syncing state of the BN we are currently connected to

we could subscribe to sync event stream
but i think there are a lot of edge cases in case of failovers

@rolfyone
Copy link
Contributor

so this is the only duty that is not created by the BN
so the creation can not be directly associated to a syncing state
to do a similar thing on VC we should have a ChainHeadTracker that becomes aware of the syncing state of the BN we are currently connected to

ultimately its 1 persistent duty for sync committees which is different to other dutes.. that's why the VC is doing more... I think it's practical overall, but it's definitely different to all the others...

@tbenr
Copy link
Contributor Author

tbenr commented Apr 15, 2024

ultimately its 1 persistent duty for sync committees which is different to other dutes.. that's why the VC is doing more... I think it's practical overall, but it's definitely different to all the others...

I think It is fine yes. We could be wrong only if the chain is not producing blocks for an entire epoch. In that case i don't think there would be enough block space to include them all.

@tbenr tbenr enabled auto-merge (squash) April 15, 2024 10:00
@tbenr tbenr merged commit e9ef718 into Consensys:master Apr 15, 2024
15 of 16 checks passed
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.

We should not create and publish sync committee if node is syncing
3 participants