Skip to content

[Merged by Bors] - Remove legacy max-skip-slots checks #4403

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

Closed
wants to merge 1 commit into from

Conversation

macladson
Copy link
Member

@macladson macladson commented Jun 15, 2023

Proposed Changes

Remove max-skip-slots checks when processing blocks.
This was legacy code which was previously used in the Medalla testnet to sync to the correct fork.
With the addition of checkpoint sync which allows us to sync to any arbitrary fork, this is no longer a necessary feature, so it has been removed for simplicity.

Additional Notes

The CLI flag and checks for attestation processing have been retained as it still may have uses in DoS protection.

@macladson macladson added code-quality backwards-incompat Backwards-incompatible API change labels Jun 15, 2023
@macladson macladson requested a review from michaelsproul June 15, 2023 06:01
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.

LGTM, I agree max-skip-slots is an inelegant way of dealing with the issue of many diverging chains like we saw on Medalla, and unlikely to be useful in future

@michaelsproul michaelsproul requested a review from paulhauner June 15, 2023 06:04
@michaelsproul
Copy link
Member

I would like a +1 from @paulhauner too, seeing as this is a change potentially affecting liveness/DoS/security

@macladson macladson added ready-for-review The code is ready for review and removed backwards-incompat Backwards-incompatible API change labels Jun 15, 2023
@macladson macladson force-pushed the remove-max-skip-slots branch from 3f4a402 to 84620bb Compare June 16, 2023 05:14
@macladson macladson requested a review from michaelsproul June 16, 2023 05:16
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.

Re-approved 👍

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Jun 19, 2023
@michaelsproul
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Jun 19, 2023
## Proposed Changes

Remove `max-skip-slots` checks when processing blocks.
This was legacy code which was previously used in the Medalla testnet to sync to the correct fork.
With the addition of checkpoint sync which allows us to sync to any arbitrary fork, this is no longer a necessary feature, so it has been removed for simplicity.

## Additional Notes
The CLI flag and checks for attestation processing have been retained as it still may have uses in DoS protection.
@bors
Copy link

bors bot commented Jun 19, 2023

Build failed:

@michaelsproul
Copy link
Member

flaky

bors retry

bors bot pushed a commit that referenced this pull request Jun 19, 2023
## Proposed Changes

Remove `max-skip-slots` checks when processing blocks.
This was legacy code which was previously used in the Medalla testnet to sync to the correct fork.
With the addition of checkpoint sync which allows us to sync to any arbitrary fork, this is no longer a necessary feature, so it has been removed for simplicity.

## Additional Notes
The CLI flag and checks for attestation processing have been retained as it still may have uses in DoS protection.
@bors
Copy link

bors bot commented Jun 19, 2023

Timed out.

@michaelsproul
Copy link
Member

looks like Windows ran in 3h11m 😩

lets try again

bors retry

bors bot pushed a commit that referenced this pull request Jun 20, 2023
## Proposed Changes

Remove `max-skip-slots` checks when processing blocks.
This was legacy code which was previously used in the Medalla testnet to sync to the correct fork.
With the addition of checkpoint sync which allows us to sync to any arbitrary fork, this is no longer a necessary feature, so it has been removed for simplicity.

## Additional Notes
The CLI flag and checks for attestation processing have been retained as it still may have uses in DoS protection.
@bors
Copy link

bors bot commented Jun 20, 2023

Pull request successfully merged into unstable.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot changed the title Remove legacy max-skip-slots checks [Merged by Bors] - Remove legacy max-skip-slots checks Jun 20, 2023
@bors bors bot closed this Jun 20, 2023
@macladson macladson deleted the remove-max-skip-slots branch June 21, 2023 00:32
ghost pushed a commit to oone-world/lighthouse that referenced this pull request Jul 13, 2023
## Proposed Changes

Remove `max-skip-slots` checks when processing blocks.
This was legacy code which was previously used in the Medalla testnet to sync to the correct fork.
With the addition of checkpoint sync which allows us to sync to any arbitrary fork, this is no longer a necessary feature, so it has been removed for simplicity.

## Additional Notes
The CLI flag and checks for attestation processing have been retained as it still may have uses in DoS protection.
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
Remove `max-skip-slots` checks when processing blocks.
This was legacy code which was previously used in the Medalla testnet to sync to the correct fork.
With the addition of checkpoint sync which allows us to sync to any arbitrary fork, this is no longer a necessary feature, so it has been removed for simplicity.

The CLI flag and checks for attestation processing have been retained as it still may have uses in DoS protection.
Woodpile37 pushed a commit to Woodpile37/lighthouse that referenced this pull request Jan 6, 2024
Remove `max-skip-slots` checks when processing blocks.
This was legacy code which was previously used in the Medalla testnet to sync to the correct fork.
With the addition of checkpoint sync which allows us to sync to any arbitrary fork, this is no longer a necessary feature, so it has been removed for simplicity.

The CLI flag and checks for attestation processing have been retained as it still may have uses in DoS protection.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-quality ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants