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

[Merged by Bors] - Run per-slot fork choice at a further distance from the head #3487

Closed
wants to merge 3 commits into from

Conversation

paulhauner
Copy link
Member

Issue Addressed

NA

Proposed Changes

Run fork choice when the head is 256 slots from the wall-clock slot, rather than 4.

The reason we don't always run FC is so that it doesn't slow us down during sync. As the comments state, setting the value to 256 means that we'd only have one interrupting fork-choice call if we were syncing at 20 slots/sec.

Additional Info

NA

@paulhauner paulhauner added ready-for-review The code is ready for review v3.0.0 🐼 Required for the v3.0.0 release labels Aug 19, 2022
@paulhauner paulhauner changed the title Increase fc sync period Run per-slot fork choice at a further distance from the head Aug 19, 2022
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Looks good

As discussed we can hone this behaviour in a future PR by some combination of:

  • Running fork choice before block proposal when we time out waiting for it to run on its own
  • Using the maximum slot of any known head rather than the slot of the canonical head
  • Adding a minimum fork choice run frequency (at least every N minutes), etc
  • Measuring sync status via head slot advancement speed, and not running fork choice when the head is already advancing quickly (i.e. sync is happening).

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Aug 19, 2022
@paulhauner
Copy link
Member Author

bors r+

bors bot pushed a commit that referenced this pull request Aug 19, 2022
## Issue Addressed

NA

## Proposed Changes

Run fork choice when the head is 256 slots from the wall-clock slot, rather than 4.

The reason we don't *always* run FC is so that it doesn't slow us down during sync. As the comments state, setting the value to 256 means that we'd only have one interrupting fork-choice call if we were syncing at 20 slots/sec.

## Additional Info

NA
@bors bors bot changed the title Run per-slot fork choice at a further distance from the head [Merged by Bors] - Run per-slot fork choice at a further distance from the head Aug 19, 2022
@bors bors bot closed this Aug 19, 2022
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
## Issue Addressed

NA

## Proposed Changes

Run fork choice when the head is 256 slots from the wall-clock slot, rather than 4.

The reason we don't *always* run FC is so that it doesn't slow us down during sync. As the comments state, setting the value to 256 means that we'd only have one interrupting fork-choice call if we were syncing at 20 slots/sec.

## Additional Info

NA
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to merge. v3.0.0 🐼 Required for the v3.0.0 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants