Skip to content

Conversation

@eserilev
Copy link
Member

Issue Addressed

Closes #7273

Proposed Changes

ethereum/consensus-specs#4179

@eserilev
Copy link
Member Author

hey @dapplion when you have a chance, could you take a look at what I have so far?

The general flow is

  • user sets the weak subjectivity period checkpoint via the existing wss-checkpoint flag
  • on startup, we make the weak subjectivity calculation, using the user provided ws checkpoint, the head snapshot and the most recent finalized checkpoint

If the user doesn't set a weak subjectivity checkpoint, what are we supposed to do here?

@eserilev eserilev added electra Required for the Electra/Prague fork enhancement New feature or request work-in-progress PR is a work-in-progress labels Apr 22, 2025
let Ok(finalized_block) = fork_choice.get_finalized_block() else {
panic!("TODO(ws)")
};
if !store.is_within_weak_subjectivity_period(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just use the head_snapshot.beacon_state to compute the weak subjectivity period. You don't need to fetch the head state's finalize state. For WS sync, the head state = the weak subjectivity checkpoint state. You don't need to fetch any more data:

  • compute ws_period from head state
  • get current epoch from clock
  • check current_epoch <= head_state.epoch + ws_period

No need to involve the store here

.safe_div(balance_churn_limit.safe_mul(200)?)?;
let weak_subjectivity_period = spec
.min_validator_withdrawability_delay
.safe_mul(epochs_for_validator_set_churn)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member Author

@eserilev eserilev Apr 23, 2025

Choose a reason for hiding this comment

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

I'm using the updated forumla for electra

https://github.com/ethereum/consensus-specs/blob/dev/specs/electra/weak-subjectivity.md#modified-compute_weak_subjectivity_period

I think the calc is simpler now that top ups are part of activation churn

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah right, please link it

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah but we probably need to support the pre-electra calculation as well

@dapplion
Copy link
Collaborator

If the user doesn't set a weak subjectivity checkpoint, what are we supposed to do here?

We should assert that the initial state is within the weak subjectivity period on any type of start.

We should also offer a flag such that

  • If flag set and starting from outside weak subjectivity period: big warn log, continue start
  • If flag not set and starting from outside weak subjectivity period: error, abort start

}
return Err(
"The current head state is outside the weak subjectivity period. It is highly recommended to purge your db and \
checkpoint sync. Alternatively you can accept the risks and ignore this error with the --ignore-ws-check flag.".to_string()
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should either summarize "the risks" or link to some resource of our own. Otherwise it's quite opaque to the users.

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've summarized the risks and linked to a blog post by vitalik

@eserilev eserilev added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Apr 24, 2025
@eserilev
Copy link
Member Author

Thanks for all the help Lion. I've made changes based on your feedback and added additional test coverage

Copy link
Collaborator

@dapplion dapplion left a comment

Choose a reason for hiding this comment

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

Looks great now!

@eserilev eserilev added the v8.1.0 Post-Fulu release label Oct 14, 2025
@mergify
Copy link

mergify bot commented Oct 14, 2025

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

@mergify mergify bot added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Oct 14, 2025
@eserilev eserilev added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Oct 14, 2025
@michaelsproul michaelsproul self-requested a review December 2, 2025 00:20
@eserilev eserilev added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Dec 2, 2025
@mergify
Copy link

mergify bot commented Dec 2, 2025

Some required checks have failed. Could you please take a look @eserilev? 🙏

@mergify mergify bot added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Dec 2, 2025
@michaelsproul
Copy link
Member

There are some tests failures. Maybe we could use a small non-zero constant pre-Electra to smooth over this issue, or we could update these tests to only run post-Electra.

@eserilev
Copy link
Member Author

eserilev commented Dec 3, 2025

Yeah I'm going to fix theses tests to run post Electra

@eserilev
Copy link
Member Author

eserilev commented Dec 3, 2025

InvalidPayloadRig wasnt using the fork_from_env stuff when constructing spec so we weren't actually running related tests against RECENT_FORKS in some cases.

revert_minority_fork_on_resume was an annoying test to try to get right when setting the WS calculation to 0 pre-electra. Setting the calculation to a min of 5 fixes the test.

basic sim and fallback sim were also failing and I havent had a chance to look at why. I've ramped up the pre electra ws calc to 256 to fix this (5 was too small of a number apparently).

@eserilev eserilev added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Dec 3, 2025
@mergify
Copy link

mergify bot commented Jan 14, 2026

Some required checks have failed. Could you please take a look @eserilev? 🙏

@mergify mergify bot added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Jan 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

electra Required for the Electra/Prague fork enhancement New feature or request hardening v8.1.0 Post-Fulu release waiting-on-author The reviewer has suggested changes and awaits thier implementation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants