feat(state-transition): make validators epochs handling close to Eth2.0 specs #2226
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes in this pull request involve significant modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Validator
participant StateProcessor
User->>Validator: Check eligibility for activation
Validator->>Validator: IsEligibleForActivation(finalizedEpoch)
Validator-->>User: Return eligibility status
User->>StateProcessor: Process epoch
StateProcessor->>StateProcessor: processRegistryUpdates(st)
StateProcessor-->>User: Update validator states
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
abi87
force-pushed
the
introduce_validators_epochs
branch
from
d31475d to
7409de1
Compare
abi87
force-pushed
the
introduce_validators_epochs
branch
from
5865607 to
51a12a0
Compare
abi87
force-pushed
the
introduce_validators_epochs
branch
from
158afbb to
763ed85
Compare
| // TODO: consider moving this to BeaconState directly | ||
| func (*StateProcessor[ | ||
| _, _, _, _, _, _, _, _, _, _, _, _, ValidatorT, _, _, _, _, | ||
| ]) lowestStakeVal(currentVals []ValidatorT) ( |
| // validator set would be. | ||
| func (sp *StateProcessor[ | ||
| _, _, _, BeaconStateT, _, _, _, _, _, _, _, _, ValidatorT, _, _, _, _, | ||
| ]) nextEpochValidatorSet(st BeaconStateT) ([]ValidatorT, error) { |
There was a problem hiding this comment.
moved to state-transition/core/state_processor_validators.go, as it should be
| @@ -492,40 +474,6 @@ func (sp *StateProcessor[ | |||
| return st.SetLatestBlockHeader(lbh) | |||
| } | |||
|
|
|||
| func (sp *StateProcessor[ | |||
| _, _, _, BeaconStateT, _, _, _, _, _, _, _, _, _, _, _, _, _, | |||
| ]) hollowProcessRewardsAndPenalties(st BeaconStateT) error { | |||
There was a problem hiding this comment.
moved to its own file to be out of the way
abi87
force-pushed
the
introduce_validators_epochs
branch
from
bd3c44f to
7f8df44
Compare
|
|
||
| // valSetMu protects valSetByEpoch from concurrent accesses | ||
| valSetMu sync.RWMutex | ||
|
|
||
| // valSetByEpoch tracks the set of validators active at the latest epochs. | ||
| // This is useful to optimize validators set updates. | ||
| // Note: Transition may be called multiple times on different, | ||
| // non/finalized blocks, so at some point valSetByEpoch may contain | ||
| // informations from blocks not finalized. This should be fine as long | ||
| // as a block is finalized eventually, and its changes will be the last | ||
| // ones. | ||
| // We prune the map to preserve only current and previous epoch | ||
| valSetByEpoch map[math.Epoch][]ValidatorT |
There was a problem hiding this comment.
not needed anymore, once validators epochs are duly handled
| switch { | ||
| case sp.cs.DepositEth1ChainID() == spec.BartioChainID: | ||
| if err = sp.hollowProcessRewardsAndPenalties(st); err != nil { | ||
| return nil, err | ||
| } | ||
| case sp.cs.DepositEth1ChainID() == spec.BoonetEth1ChainID && | ||
| slot < math.U64(spec.BoonetFork3Height): | ||
| // We cannot simply drop hollowProcessRewardsAndPenalties because | ||
| // appHash accounts for the list of operations carried out | ||
| // over the state even if the operations does not affect the state | ||
| // (rewards and penalties are always zero at this stage of beaconKit) | ||
| if err = sp.hollowProcessRewardsAndPenalties(st); err != nil { | ||
| return nil, err | ||
| } | ||
| default: | ||
| // no real need to perform hollowProcessRewardsAndPenalties |
There was a problem hiding this comment.
moved into processRewardsAndPenalties to have fork switches less in the way of mostly read code
|
@coderabbitai: review |
|
@abi87: I will review the changes. ✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (10)
consensus-types/types/validator.go(3 hunks)state-transition/core/state_processor.go(2 hunks)state-transition/core/state_processor_genesis.go(4 hunks)state-transition/core/state_processor_genesis_test.go(3 hunks)state-transition/core/state_processor_rewards_penalties.go(1 hunks)state-transition/core/state_processor_slashing.go(2 hunks)state-transition/core/state_processor_staking.go(2 hunks)state-transition/core/state_processor_staking_test.go(19 hunks)state-transition/core/state_processor_validators.go(2 hunks)state-transition/core/types.go(1 hunks)
🧰 Additional context used
📓 Learnings (4)
state-transition/core/state_processor_staking.go (3)
Learnt from: abi87
PR: berachain/beacon-kit#2142
File: mod/state-transition/pkg/core/state_processor_committee.go:48-50
Timestamp: 2024-11-12T11:12:56.774Z
Learning: In `mod/state-transition/pkg/core/state_processor_committee.go`, the TODO comment about optimizing the process to send only updated validators to consensus is already tracked and does not require creating a new issue.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_validators.go:40-43
Timestamp: 2024-11-29T12:00:20.998Z
Learning: The developer prefers not to add specific error handling for validator cap scenarios in the `processValidatorsSetUpdates` function.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking.go:202-207
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/state-transition/pkg/core/state_processor_staking.go`, within the `addValidatorToRegistry` function, when a new validator's effective balance is less than or equal to the smallest current validator's effective balance, it's acceptable to mark the new validator as withdrawable and add it to the registry.
state-transition/core/state_processor_validators.go (1)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_validators.go:40-43
Timestamp: 2024-11-29T12:00:20.998Z
Learning: The developer prefers not to add specific error handling for validator cap scenarios in the `processValidatorsSetUpdates` function.
state-transition/core/state_processor_genesis.go (2)
Learnt from: abi87
PR: berachain/beacon-kit#2142
File: mod/state-transition/pkg/core/state_processor_genesis.go:46-159
Timestamp: 2024-11-12T11:12:56.773Z
Learning: When reviewing the `InitializePreminedBeaconStateFromEth1` function in `mod/state-transition/pkg/core/state_processor_genesis.go`, additional documentation of the initialization sequence is not necessary as per the user's preference.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_genesis.go:109-118
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `InitializePreminedBeaconStateFromEth1` in `mod/state-transition/pkg/core/state_processor_genesis.go`, we enforce that there is a single deposit per validator in the genesis format; therefore, handling multiple deposits per validator is not necessary.
state-transition/core/state_processor_staking_test.go (4)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_genesis.go:109-118
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `InitializePreminedBeaconStateFromEth1` in `mod/state-transition/pkg/core/state_processor_genesis.go`, we enforce that there is a single deposit per validator in the genesis format; therefore, handling multiple deposits per validator is not necessary.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking_test.go:424-440
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In the `TestTransitionHittingValidatorsCap` function in `mod/state-transition/pkg/core/state_processor_staking_test.go`, adding additional assertions to verify validator set size after initialization is unnecessary.
Learnt from: abi87
PR: berachain/beacon-kit#2142
File: mod/state-transition/pkg/core/state_processor_staking_test.go:154-168
Timestamp: 2024-11-12T11:12:56.774Z
Learning: In the `TestTransitionUpdateValidators` function in `mod/state-transition/pkg/core/state_processor_staking_test.go`, adding additional assertions for balance relationships is not necessary.
Learnt from: abi87
PR: berachain/beacon-kit#2142
File: mod/state-transition/pkg/core/state_processor_staking_test.go:175-197
Timestamp: 2024-11-12T11:12:56.774Z
Learning: In Go test files within the `mod/state-transition/pkg/core/` directory, when a test is already substantial in size, avoid adding verbose code or comments that increase its length further.
🔇 Additional comments (15)
state-transition/core/state_processor_genesis.go (2)
109-111: Confirm Proper Activation of Validators at Genesis
The addition of sp.processGenesisActivation(st) enhances the genesis state initialization by activating validators appropriately. Ensure that this method sets activation and eligibility epochs in compliance with the Eth2.0 specifications.
164-196: Verify Activation Epoch Values in processGenesisActivation
In the processGenesisActivation method, activation eligibility and activation epochs are set to 0. Verify that setting these epochs to 0 aligns with the protocol specifications and does not introduce unintended behavior.
Run the following script to check for protocol compliance regarding activation epochs:
✅ Verification successful
Based on the search results, I can now provide a definitive response about the activation epoch values in the genesis state:
Setting activation epochs to 0 in genesis state is correct
The codebase evidence confirms that setting both ActivationEligibilityEpoch and ActivationEpoch to 0 for genesis validators is the correct implementation:
- Test cases in
state_processor_genesis_test.goexplicitly verify that genesis validators have both epochs set to 0 - The validator lifecycle implementation shows that a validator is considered active when
ActivationEpoch <= currentEpoch < ExitEpoch - Setting these values to 0 ensures genesis validators are active from the very first epoch, which is the expected behavior for initial validators
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that setting activation epochs to 0 is compliant with the specifications.
# Test: Search for protocol documentation or code comments related to activation epochs.
# Expected Result: Confirmation that 0 is the correct value for initial activation epochs.
rg "activation.?epoch" docs/ specs/ comments/ code/
Length of output: 196
Script:
#!/bin/bash
# Let's search for activation epoch related code in the entire codebase
rg -i "activation.?epoch|genesis.?activation" -A 3 -B 3
# Also search for any genesis state initialization patterns
ast-grep --pattern 'func $$$Genesis$$$($$$) {
$$$
}'
Length of output: 27959
state-transition/core/state_processor_staking.go (1)
215-215: Ensure Balance Increase Matches Deposit Amount
When increasing the validator's balance, ensure that the amount added matches the deposit amount to maintain consistency.
state-transition/core/state_processor.go (2)
Line range hint 367-403: Ensure Correct Processing Order in processEpoch
The updates in processEpoch correctly enforce the validator set cap after processing rewards, penalties, and registry updates. This ensures that all validator states are up to date before applying the cap.
375-377: Implement Actual Rewards and Penalties Processing
The call to sp.processRewardsAndPenalties(st) suggests that rewards and penalties are now being processed. Ensure that this method is properly implemented and aligns with the protocol specifications.
Run the following script to check for the implementation of processRewardsAndPenalties:
state-transition/core/state_processor_slashing.go (2)
37-41: LGTM! Clear documentation of implementation rationale.
The comment effectively explains why this seemingly no-op function is maintained, highlighting its importance for state consistency through appHash.
59-60: Verify the epoch calculation's correctness.
The epoch calculation and slashing vector index update appear correct, but let's verify there are no other dependencies on this operation.
✅ Verification successful
Let me analyze the slashing vector operations further by checking for any additional dependencies.
Let me analyze the results to provide a comprehensive verification of the epoch calculation and its dependencies.
Let me try one more verification to check the slashing-related state transitions.
Epoch calculation and slashing vector index update are correctly implemented
Based on the verification results:
- The epoch calculation
(sp.cs.SlotToEpoch(slot).Unwrap() + 1) % sp.cs.EpochsPerSlashingsVector()follows the same pattern as other vector index calculations in the codebase - The slashing vector is properly bounded by
EpochsPerSlashingsVector(set to 8 in base config) - The
UpdateSlashingAtIndexoperation is consistently implemented across the state interfaces - No conflicting or inconsistent usage patterns were found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other references to slashing vector operations
rg -A 3 "EpochsPerSlashingsVector|UpdateSlashingAtIndex"
Length of output: 3044
Script:
#!/bin/bash
# Search for any references to slashing calculations or index manipulations
ast-grep --pattern 'SlotToEpoch($$$).Unwrap() % $$$'
# Also check for any direct slashing vector access patterns
ast-grep --pattern 'GetSlashingAtIndex($$$)'
Length of output: 101
Script:
#!/bin/bash
# Search for slashing-related state transitions and calculations
rg -A 5 "func.*[Ss]lashing"
# Also check for epoch calculations in state transitions
rg -A 3 "SlotToEpoch.*Unwrap"
Length of output: 15794
state-transition/core/state_processor_rewards_penalties.go (1)
37-41: LGTM! Clear documentation of implementation rationale.
The comment effectively explains why this seemingly no-op function is maintained, highlighting its importance for state consistency through appHash.
state-transition/core/state_processor_genesis_test.go (3)
148-154: LGTM! Clear validator retrieval and verification.
The sequence of operations (get index by pubkey, then get validator by index) is correct and includes proper error handling.
159-160: LGTM! Proper activation epoch verification for non-Bartio validators.
The checks correctly verify that non-Bartio validators have their activation epochs set to 0, aligning with the PR's objective of improving validator epochs handling.
288-297: LGTM! Proper activation epoch verification for Bartio validators.
The checks correctly verify that Bartio validators have their activation epochs set to FarFutureEpoch, with clear formatting and proper constant usage.
state-transition/core/types.go (1)
245-248: LGTM! Interface changes align with Eth2.0 specifications.
The new methods enhance the Validator interface with proper eligibility checks and epoch management capabilities, following the Ethereum 2.0 specifications for validator state transitions.
Also applies to: 258-265
consensus-types/types/validator.go (2)
258-260: LGTM! Improved eligibility check logic.
The modification to use a threshold parameter instead of maxEffectiveBalance provides more flexibility in determining validator eligibility, while maintaining the core functionality.
320-342: LGTM! Clean implementation of epoch management methods.
The getter and setter implementations follow Go best practices and provide proper encapsulation of validator state.
state-transition/core/state_processor_staking_test.go (1)
1086-1121: LGTM! Well-structured helper function.
The moveToEndOfEpoch helper effectively reduces code duplication and provides clear epoch transition logic with proper error handling.
| @@ -1211,3 +1082,40 @@ func generateTestPK(t *testing.T, rndSeed int) (bytes.B48, int) { | |||
| rndSeed++ | |||
| return key, rndSeed | |||
| } | |||
|
|
|||
| func moveToEndOfEpoch( | |||
There was a problem hiding this comment.
minor test consolidation
| func (sp *StateProcessor[ | ||
| _, _, _, BeaconStateT, _, _, _, _, _, _, _, _, ValidatorT, _, _, _, _, | ||
| ]) processValidatorsSetUpdates( |
There was a problem hiding this comment.
this is basically being replace by the following processing:
- at the beginning of
processEpochwe not the currently active validator set - we carry out all Eth like transitions
- at the end of
processEpochwe pick again the active validator set (which will be active next epoch) and compute the diff to send back to consensus
| if val.GetEffectiveBalance() < minEffectiveBalance { | ||
| continue | ||
| } |
There was a problem hiding this comment.
just a sanity check for myself: this case should only happen if we include a genesis deposit under min stake? In which case we don't set the activation epoch
There was a problem hiding this comment.
This is correct. I synced the first 2000 blocks and had no issue, so we should be good
processRegistryUpdate, moving validators cap handling thereSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests