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(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 |
|
This pull request has merge conflicts. Could you please resolve them @eserilev? 🙏 |
mergify
bot
added
waiting-on-author
eserilev
added
ready-for-review
|
@mergify requeue |
Merge Queue StatusRule:
This pull request spent 29 minutes 56 seconds in the queue, including 28 minutes 1 second running CI. Required conditions to merge
ReasonPull request #7347 has been dequeued. The pull request could not be merged. This could be related to an activated branch protection or ruleset rule that prevents us from merging. (details: 1 review requesting changes and 1 approving review by reviewers with write access.) HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. |
|
@mergify requeue |
Merge Queue StatusRule:
This pull request spent 31 minutes 13 seconds in the queue, including 29 minutes 46 seconds running CI. Required conditions to merge
ReasonPull request #7347 has been dequeued. The pull request could not be merged. This could be related to an activated branch protection or ruleset rule that prevents us from merging. (details: 1 review requesting changes and 1 approving review by reviewers with write access.) HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. |
|
@mergify requeue |
Merge Queue StatusRule:
This pull request spent 31 minutes 3 seconds in the queue, including 28 minutes 53 seconds running CI. Required conditions to merge
|
3 participants
Issue Addressed
Closes #7273
Proposed Changes
ethereum/consensus-specs#4179