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

Use sync_tolerance_epochs flag to control the proposer prep routines #7044

Merged

Conversation

jimmygchen
Copy link
Member

@jimmygchen jimmygchen commented Feb 26, 2025

Issue Addressed

Replace the 2 + 2 == 5 hacks from holesky-rescue and use the existing sync_tolerance_epochs flag to control the proposer prep routines.

@jimmygchen jimmygchen added the v7.0.0-beta.clean Clean release post Holesky rescue label Feb 26, 2025
@jimmygchen jimmygchen mentioned this pull request Feb 26, 2025
13 tasks
@jimmygchen jimmygchen force-pushed the proposer-prep-sync-threshold branch from 9a4c4b8 to 06a5568 Compare February 26, 2025 06:02
@michaelsproul michaelsproul force-pushed the proposer-prep-sync-threshold branch from 7aeb636 to 5804340 Compare February 26, 2025 07:50
@michaelsproul michaelsproul changed the base branch from holesky-rescue to release-v7.0.0 February 26, 2025 07:51
Copy link

mergify bot commented Feb 26, 2025

This pull request has merge conflicts. Could you please resolve them @jimmygchen? 🙏

@michaelsproul
Copy link
Member

Should merge this after (manually) merging this PR:

@michaelsproul michaelsproul force-pushed the proposer-prep-sync-threshold branch from 5804340 to b87319f Compare February 26, 2025 07:53
@michaelsproul michaelsproul changed the base branch from release-v7.0.0 to unstable February 26, 2025 08:44
@michaelsproul michaelsproul changed the base branch from unstable to release-v7.0.0 February 26, 2025 08:44
@michaelsproul michaelsproul added the ready-for-review The code is ready for review label Feb 26, 2025
@michaelsproul
Copy link
Member

Merged the original sync tolerance PR to unstable, so this now just contains the new changes.

@@ -16,6 +16,9 @@ pub const DEFAULT_PREPARE_PAYLOAD_LOOKAHEAD_FACTOR: u32 = 3;
/// Fraction of a slot lookahead for fork choice in the state advance timer (500ms on mainnet).
pub const FORK_CHOICE_LOOKAHEAD_FACTOR: u32 = 24;

/// Default sync tolerance epochs.
pub const DEFAULT_SYNC_TOLERANCE_EPOCHS: u64 = 16;
Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't really given enough thoughts to this, so this PR changes the following

  • PREPARE_PROPOSER_HISTORIC_EPOCHS from 4 to sync_tolerance_epochs
  • get_pre_payload_attributes epochs from 2 to sync_tolerance_epochs
  • DEFAULT_SYNC_TOLERANCE_EPOCHS for http block production from 8 to sync_tolerance_epochs

sync_tolerance_epochs is now defaulted to 16.

We discussed about DEFAULT_SYNC_TOLERANCE_EPOCHS a while ago but I forgot why it was 8.

@AgeManning had a comment here #6880 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

I think lets change this back to 8.

Now that we have a config flag for it, we have the option of getting people to set it in adverse chain conditions (sparse chain with legitimately very few blocks).

I'm worried that setting it to 16 might have contributed to the plethora of shitty sidechains we saw on Holesky. Whatever value we set here is essentially a bound on "we will build a shitty side chain during sync/stuckness of up to this length". I'd even be tempted to set the default lower, maybe 2 (as Age suggested originally) or 4.

Copy link
Member

Choose a reason for hiding this comment

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

As agreed on the call today, I've lowered the default to 2 in this commit: c7105d1.

In case of a sparse chain and liveness issues, we can advise validators to bump up the sync tolerance temporarily.

@michaelsproul michaelsproul added the under-review A reviewer has only partially completed a review. label Mar 5, 2025
@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review under-review A reviewer has only partially completed a review. labels Mar 6, 2025
@mergify mergify bot merged commit 09849e8 into sigp:release-v7.0.0 Mar 6, 2025
31 checks passed
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. v7.0.0-beta.clean Clean release post Holesky rescue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants