fix(state-transition): restore state sync over boonet #2219
Conversation
WalkthroughThe changes in this pull request modify the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2219 +/- ##
==========================================
- Coverage 29.85% 29.75% -0.10%
==========================================
Files 350 350
Lines 15871 15908 +37
Branches 13 13
==========================================
- Hits 4738 4734 -4
- Misses 10825 10865 +40
- Partials 308 309 +1
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range comments (1)
state-transition/core/state_processor_staking.go (1)
Line range hint
124-125: Improve error handling in validator lookup.The current implementation doesn't distinguish between
ErrNotFound(expected for new validators) and other errors (potential system issues). This could mask serious problems.Consider implementing explicit error handling:
idx, err := st.ValidatorIndexByPubkey(dep.GetPubkey()) if err != nil { - // If the validator does not exist, we add the validator. - // TODO: improve error handling by distinguishing - // ErrNotFound from other kind of errors - return sp.createValidator(st, dep) + if errors.Is(err, ErrNotFound) { + return sp.createValidator(st, dep) + } + return errors.Wrap(err, "failed to lookup validator") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
state-transition/core/state_processor_staking.go(1 hunks)
🔇 Additional comments (1)
state-transition/core/state_processor_staking.go (1)
87-87: LGTM, but verify genesis slot handling.
The additional check slot != 0 for Boonet chain is correct and aligns with fixing state sync. This prevents incorrect deposit index increments at genesis while maintaining backward compatibility for non-zero slots.
Let's verify the genesis slot handling across the codebase:
✅ Verification successful
Based on the search results, I can now make a definitive assessment of the genesis slot handling across the codebase. The search revealed comprehensive and consistent special handling of slot 0 (genesis) across multiple components:
Special genesis slot handling verified across codebase
The additional check slot != 0 in the Boonet chain condition is consistent with the codebase's overall genesis slot handling pattern. Key evidence:
-
State processor has proper genesis handling:
state_processor_staking.go: Empty root at genesis, special handling for validators rootstate_processor_validators.go: Special epoch handling for genesis slotstate_processor_genesis.go: Dedicated genesis state initialization
-
Storage layer validates genesis:
store.go: Explicit check preventing parent slot lookup for genesisbeacondb: Special genesis validators root storage
-
Validation layer has genesis-specific logic:
validation_deposits.go: Special validation rules for genesis deposits- Explicit checks for deposit index at genesis
The change is safe and properly maintains backward compatibility while fixing state sync by preventing deposit index increment at genesis slot.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for special genesis slot (0) handling in state processing
# Expected: Find other places where slot 0 has special handling to ensure consistency
# Search for slot zero checks in state processing
rg -A 3 "slot\s*(==|!=)\s*0"
# Search for genesis-related handling
rg -A 3 "genesis"
Length of output: 66398
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
config/spec/special_cases.go(2 hunks)state-transition/core/state_processor.go(3 hunks)
🔇 Additional comments (1)
config/spec/special_cases.go (1)
23-24: Addition of 'math' import and 'BoonetFork3Height' constant
The import of the "math" package and the definition of BoonetFork3Height using math.MaxUint64 are appropriate for handling numerical constants.
Also applies to: 40-40
| func (sp *StateProcessor[ | ||
| _, _, _, BeaconStateT, _, _, _, _, _, _, _, _, _, _, _, _, _, | ||
| ]) hollowProcessRewardsAndPenalties(st BeaconStateT) error { | ||
| slot, err := st.GetSlot() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if sp.cs.SlotToEpoch(slot) == math.U64(constants.GenesisEpoch) { | ||
| return nil | ||
| } | ||
|
|
||
| // this has been simplified to make clear that | ||
| // we are not really doing anything here | ||
| valCount, err := st.GetTotalValidators() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| for i := range valCount { | ||
| // Increase the balance of the validator. | ||
| if err = st.IncreaseBalance(math.ValidatorIndex(i), 0); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Decrease the balance of the validator. | ||
| if err = st.DecreaseBalance(math.ValidatorIndex(i), 0); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix the incorrect loop iteration over validators in 'hollowProcessRewardsAndPenalties'
The loop for i := range valCount is incorrect because valCount is a numeric value representing the total number of validators, not a slice or array. Using range on an integer will result in a compilation error.
Apply this diff to fix the loop:
func (sp *StateProcessor[
_, _, _, BeaconStateT, _, _, _, _, _, _, _, _, _, _, _, _, _,
]) hollowProcessRewardsAndPenalties(st BeaconStateT) error {
slot, err := st.GetSlot()
if err != nil {
return err
}
if sp.cs.SlotToEpoch(slot) == math.U64(constants.GenesisEpoch) {
return nil
}
// this has been simplified to make clear that
// we are not really doing anything here
valCount, err := st.GetTotalValidators()
if err != nil {
return err
}
- for i := range valCount {
+ for i := uint64(0); i < valCount; i++ {
// Increase the balance of the validator.
if err = st.IncreaseBalance(math.ValidatorIndex(i), 0); err != nil {
return err
}
// Decrease the balance of the validator.
if err = st.DecreaseBalance(math.ValidatorIndex(i), 0); err != nil {
return err
}
}
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (sp *StateProcessor[ | |
| _, _, _, BeaconStateT, _, _, _, _, _, _, _, _, _, _, _, _, _, | |
| ]) hollowProcessRewardsAndPenalties(st BeaconStateT) error { | |
| slot, err := st.GetSlot() | |
| if err != nil { | |
| return err | |
| } | |
| if sp.cs.SlotToEpoch(slot) == math.U64(constants.GenesisEpoch) { | |
| return nil | |
| } | |
| // this has been simplified to make clear that | |
| // we are not really doing anything here | |
| valCount, err := st.GetTotalValidators() | |
| if err != nil { | |
| return err | |
| } | |
| for i := range valCount { | |
| // Increase the balance of the validator. | |
| if err = st.IncreaseBalance(math.ValidatorIndex(i), 0); err != nil { | |
| return err | |
| } | |
| // Decrease the balance of the validator. | |
| if err = st.DecreaseBalance(math.ValidatorIndex(i), 0); err != nil { | |
| return err | |
| } | |
| } | |
| func (sp *StateProcessor[ | |
| _, _, _, BeaconStateT, _, _, _, _, _, _, _, _, _, _, _, _, _, | |
| ]) hollowProcessRewardsAndPenalties(st BeaconStateT) error { | |
| slot, err := st.GetSlot() | |
| if err != nil { | |
| return err | |
| } | |
| if sp.cs.SlotToEpoch(slot) == math.U64(constants.GenesisEpoch) { | |
| return nil | |
| } | |
| // this has been simplified to make clear that | |
| // we are not really doing anything here | |
| valCount, err := st.GetTotalValidators() | |
| if err != nil { | |
| return err | |
| } | |
| for i := uint64(0); i < valCount; i++ { | |
| // Increase the balance of the validator. | |
| if err = st.IncreaseBalance(math.ValidatorIndex(i), 0); err != nil { | |
| return err | |
| } | |
| // Decrease the balance of the validator. | |
| if err = st.DecreaseBalance(math.ValidatorIndex(i), 0); err != nil { | |
| return err | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
state-transition/core/state_processor.go(3 hunks)
🔇 Additional comments (5)
state-transition/core/state_processor.go (5)
33-33: Import statement addition is appropriate
The addition of the constants package is necessary for accessing constant values like GenesisEpoch.
378-381: Slot retrieval and error handling are correct
The retrieval of slot and the handling of potential errors are properly implemented.
383-400: Switch statement logic is correctly structured
The switch cases handle different DepositEth1ChainID conditions appropriately, calling hollowProcessRewardsAndPenalties when necessary.
401-407: Error handling in epoch processing functions is sound
The sequential calls to processEffectiveBalanceUpdates, processSlashingsReset, and processRandaoMixesReset with proper error checks ensure robustness.
495-525:
Fix the incorrect loop iteration in 'hollowProcessRewardsAndPenalties'
The loop for i := range valCount is incorrect because valCount is a numeric value representing the total number of validators, not a slice or array. Using range on an integer will result in a compilation error.
Apply this diff to correct the loop:
- for i := range valCount {
+ for i := uint64(0); i < valCount; i++ {| func (sp *StateProcessor[ | ||
| _, _, _, BeaconStateT, _, _, _, _, _, _, _, _, _, _, _, _, _, | ||
| ]) hollowProcessRewardsAndPenalties(st BeaconStateT) error { | ||
| slot, err := st.GetSlot() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if sp.cs.SlotToEpoch(slot) == math.U64(constants.GenesisEpoch) { | ||
| return nil | ||
| } | ||
|
|
||
| // this has been simplified to make clear that | ||
| // we are not really doing anything here | ||
| valCount, err := st.GetTotalValidators() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| for i := range valCount { | ||
| // Increase the balance of the validator. | ||
| if err = st.IncreaseBalance(math.ValidatorIndex(i), 0); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Decrease the balance of the validator. | ||
| if err = st.DecreaseBalance(math.ValidatorIndex(i), 0); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding unit tests for 'hollowProcessRewardsAndPenalties'
Adding unit tests for the new hollowProcessRewardsAndPenalties function will ensure its correctness across different scenarios.
| @@ -84,7 +84,7 @@ func (sp *StateProcessor[ | |||
| // We keep it for backward compatibility. | |||
| depositIndex++ | |||
| case sp.cs.DepositEth1ChainID() == spec.BoonetEth1ChainID && | |||
| slot < math.U64(spec.BoonetFork2Height): | |||
| slot != 0 && slot < math.U64(spec.BoonetFork2Height): | |||
There was a problem hiding this comment.
genesis indexes were set right, so we don't correct them.
| @@ -470,6 +492,40 @@ func (sp *StateProcessor[ | |||
| return st.SetLatestBlockHeader(lbh) | |||
| } | |||
|
|
|||
| func (sp *StateProcessor[ | |||
| _, _, _, BeaconStateT, _, _, _, _, _, _, _, _, _, _, _, _, _, | |||
| ]) hollowProcessRewardsAndPenalties(st BeaconStateT) error { | |||
There was a problem hiding this comment.
would just add a comment above this function explaining it came from getAttestationDeltas which was just returning 0s for rewards and 0s for penalties.
Sync is currently broken on Boonet. Bysecting I found:
The latest PR breaks sync because if mistakenly changes the indexes for genesis deposits, and only those
These indexes are part of the beacon state that is hashed into blocks. So state sync was broken because:
AppHashreported by the block does not match theAppHashgenerated by the local state.The fix is to preserve the way we set indexes over genesis
MOREOVER
We need to properly drop uselass BeaconState ops to preserve appHash