-
Notifications
You must be signed in to change notification settings - Fork 810
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
Use sync_tolerance_epochs
flag to control the proposer prep routines
#7044
Conversation
9a4c4b8
to
06a5568
Compare
7aeb636
to
5804340
Compare
This pull request has merge conflicts. Could you please resolve them @jimmygchen? 🙏 |
Should merge this after (manually) merging this PR: |
5804340
to
b87319f
Compare
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; |
There was a problem hiding this comment.
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 tosync_tolerance_epochs
get_pre_payload_attributes
epochs from 2 tosync_tolerance_epochs
DEFAULT_SYNC_TOLERANCE_EPOCHS
for http block production from 8 tosync_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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Issue Addressed
Replace the
2 + 2 == 5
hacks fromholesky-rescue
and use the existingsync_tolerance_epochs
flag to control the proposer prep routines.