Implement weak subjectivity safety checks #7347
Conversation
|
hey @dapplion when you have a chance, could you take a look at what I have so far? The general flow is
If the user doesn't set a weak subjectivity checkpoint, what are we supposed to do here? |
eserilev
added
electra
| } | ||
|
|
||
| let Ok(ws_period) = finalized_state.compute_weak_subjectivity_period(&self.spec) else { | ||
| // TODO(ws) failed to calculate ws period, log and continue? |
There was a problem hiding this comment.
No, this should never fail propagate the error and abort start
| let Ok(finalized_block) = fork_choice.get_finalized_block() else { | ||
| panic!("TODO(ws)") | ||
| }; | ||
| if !store.is_within_weak_subjectivity_period( |
There was a problem hiding this comment.
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
consensus/types/src/beacon_state.rs
Outdated
| .safe_div(balance_churn_limit.safe_mul(200)?)?; | ||
| let weak_subjectivity_period = spec | ||
| .min_validator_withdrawability_delay | ||
| .safe_mul(epochs_for_validator_set_churn)?; |
There was a problem hiding this comment.
Where is this formula from? You should implement
There was a problem hiding this comment.
I'm using the updated forumla for electra
I think the calc is simpler now that top ups are part of activation churn
There was a problem hiding this comment.
Ah right, please link it
There was a problem hiding this comment.
Ah but we probably need to support the pre-electra calculation as well
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
|
| } | ||
| 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() |
There was a problem hiding this comment.
We should either summarize "the risks" or link to some resource of our own. Otherwise it's quite opaque to the users.
There was a problem hiding this comment.
i've summarized the risks and linked to a blog post by vitalik
eserilev
added
ready-for-review
|
Thanks for all the help Lion. I've made changes based on your feedback and added additional test coverage |
Issue Addressed
Closes #7273
Proposed Changes
ethereum/consensus-specs#4179