Repository navigation
Gate churn-limit helpers by fork - #9880
NikhilSharmaWe wants to merge 6 commits into
Conversation
Signed-off-by: Nikhil Sharma <nikhilsharma230303@gmail.com>
michaelsproul
added
code-quality
consensus
chong-he
left a comment
chong-he
left a comment
There was a problem hiding this comment.
Looks pretty good, some comments below. I thought it would also be great to have a few tests covering the *_churn_limit functions and their applicability to different forks to prevent regression.
| let activation_churn_limit = state.get_activation_churn_limit(spec)?; | ||
| let finalized_checkpoint = state.finalized_checkpoint(); | ||
| let fork_name = state.fork_name_unchecked(); | ||
| let (validator_churn_limit, activation_churn_limit) = if !fork_name.electra_enabled() { |
There was a problem hiding this comment.
Does the activation_churn_limit needs to be an Option? It looks like it can just be u64, which is simpler. Also maybe can move the let activation_churn_limit to a bit after this just before it's used
There was a problem hiding this comment.
Done, now a u64, computed just before use (before the state split)
| /// Pre-Electra validator-count exit churn. | ||
| validator_churn_limit: Option<u64>, | ||
| /// Electra+ Gwei exit churn. | ||
| exit_churn_limit: Option<u64>, |
There was a problem hiding this comment.
Now that we have 2 types of churn limits, and they are both Option, which cause a bit of ambiguity. Maybe we can define an enum:
enum ExitChurnLimit {
// Pre-Electra, number of validators per epoch
ValidatorCount(u64),
/// Electra onwards: Gwei per epoch.
Balance(u64),
}so that it is explicitly clear that we refer to the number of validators that can exit or the balance/Gwei that can exit per epoch
There was a problem hiding this comment.
Replaced the two Option with an ExitChurnLimit enum.
| pub fn get_validator_churn_limit(&self, spec: &ChainSpec) -> Result<u64, BeaconStateError> { | ||
| if self.fork_name_unchecked().electra_enabled() { |
There was a problem hiding this comment.
Suggestion: there are so many *_churn_limit functions now, and it will be helpful to have a comment at the top of functions as to where the fork they applies (although one can deduced from the function itself, but it is a nice to have at the comment for clarity).
For example, this is Pre-Electra only, others can be Electra-onwards, Electra/Fulu, or Gloas-onwards etc
There was a problem hiding this comment.
added the comments
chong-he
added
waiting-on-author
Signed-off-by: Nikhil Sharma <nikhilsharma230303@gmail.com>
added a fork-gate test for the |
chong-he
added
ready-for-review
This branch has not been deployed
3 participants
Issue Addressed
#9218
Proposed Changes
BeaconStateso pre-Electra validator-count functions error if called after Electra.get_validator_activation_churn_limit.get_activation_churn_limitis now the Gloas spec function (Gwei).