fix(state-transitions): Make setting of eth1 deposit index backward compatible #2202
Conversation
WalkthroughThe pull request introduces several modifications across various files, primarily enhancing logging and error handling in deposit processing and state management. Key changes include the addition of logging statements in methods related to block building and state processing, as well as adjustments to error handling practices. The updates streamline the code by removing unused variables and improving the clarity of comments in test cases. Overall, the changes maintain the existing logic while improving the visibility and robustness of the deposit and validator management processes. Changes
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2202 +/- ##
==========================================
- Coverage 27.98% 27.94% -0.04%
==========================================
Files 366 366
Lines 16986 17032 +46
Branches 13 13
==========================================
+ Hits 4753 4760 +7
- Misses 11916 11956 +40
+ Partials 317 316 -1
|
| case sp.cs.DepositEth1ChainID() == spec.BoonetEth1ChainID && | ||
| slot < math.U64(spec.BoonetFork2Height): | ||
| // Boonet pre fork2 has a bug which makes DepositEth1ChainID point to | ||
| // next deposit index, not latest processed deposit index. | ||
| // We keep it for backward compatibility. | ||
| depositIndex++ |
There was a problem hiding this comment.
This can be removed as soon as fork activates
…deposit-index-backward-compatible
calbera
changed the title
abi87
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range comments (1)
mod/storage/pkg/deposit/store.go (1)
Line range hint
148-170: Fix the incorrect loop in thePrunemethodThe loop
for i := range endis incorrect becauseendis auint64, and you cannot range over an integer. This will cause a compilation error.Apply this diff to fix the loop:
-func (kv *KVStore[DepositT]) Prune(start, end uint64) error { - kv.logger.Debug( - "Prune request", - "start", start, - "end", end, - ) - if start > end { - return fmt.Errorf( - "DepositKVStore Prune start: %d, end: %d: %w", - start, end, pruner.ErrInvalidRange, - ) - } - - var ctx = context.TODO() - kv.mu.Lock() - defer kv.mu.Unlock() - for i := range end { - // This only errors if the key passed in cannot be encoded. - if err := kv.store.Remove(ctx, start+i); err != nil { - return errors.Wrapf(err, "failed to prune deposit %d", start+i) - } - } +func (kv *KVStore[DepositT]) Prune(start, end uint64) error { + kv.logger.Debug( + "Prune request", + "start", start, + "end", end, + ) + if start > end { + return fmt.Errorf( + "DepositKVStore Prune start: %d, end: %d: %w", + start, end, pruner.ErrInvalidRange, + ) + } + + var ctx = context.TODO() + kv.mu.Lock() + defer kv.mu.Unlock() + for i := start; i < end; i++ { + // This only errors if the key passed in cannot be encoded. + if err := kv.store.Remove(ctx, i); err != nil { + return errors.Wrapf(err, "failed to prune deposit %d", i) + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (9)
mod/beacon/validator/block_builder.go(1 hunks)mod/state-transition/pkg/core/core_test.go(0 hunks)mod/state-transition/pkg/core/state_processor.go(1 hunks)mod/state-transition/pkg/core/state_processor_genesis_test.go(2 hunks)mod/state-transition/pkg/core/state_processor_staking.go(3 hunks)mod/state-transition/pkg/core/state_processor_staking_test.go(21 hunks)mod/state-transition/pkg/core/state_processor_withdrawals.go(1 hunks)mod/state-transition/pkg/core/validation_deposits.go(1 hunks)mod/storage/pkg/deposit/store.go(7 hunks)
💤 Files with no reviewable changes (1)
- mod/state-transition/pkg/core/core_test.go
🧰 Additional context used
📓 Learnings (4)
mod/state-transition/pkg/core/state_processor_genesis_test.go (2)
Learnt from: abi87
PR: berachain/beacon-kit#2114
File: mod/state-transition/pkg/core/state_processor_genesis_test.go:82-108
Timestamp: 2024-11-12T11:12:56.774Z
Learning: In `mod/state-transition/pkg/core/state_processor_genesis_test.go`, the `deposits` array is part of the test setup and not individual test cases, so adding comments to explain each deposit is not necessary.
Learnt from: abi87
PR: berachain/beacon-kit#2114
File: mod/state-transition/pkg/core/state_processor_genesis_test.go:42-77
Timestamp: 2024-11-12T11:12:56.773Z
Learning: Documentation comments for type aliases in test code are not necessary.
mod/state-transition/pkg/core/validation_deposits.go (1)
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.
mod/storage/pkg/deposit/store.go (1)
Learnt from: abi87
PR: berachain/beacon-kit#2115
File: mod/storage/pkg/deposit/store.go:77-80
Timestamp: 2024-11-24T21:25:28.207Z
Learning: In the Go file `mod/storage/pkg/deposit/store.go`, within the `GetDepositsByIndex` function, the `depRange` parameter comes from validated parameters, ensuring that integer overflow is not possible when calculating `endIdx = startIndex + depRange`.
mod/state-transition/pkg/core/state_processor_staking_test.go (2)
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.
🔇 Additional comments (13)
mod/state-transition/pkg/core/state_processor_staking.go (3)
125-134: Balance increase and logging are correctly implemented
The validator's balance is correctly increased, and relevant information is logged appropriately.
333-342: New validator creation is handled correctly with proper logging
The addValidatorInternal function correctly adds a new validator, increases the balance, and logs the details effectively.
74-95: 🧹 Nitpick (assertive)
Ensure backward compatibility code is revisited after fork activation
The code adjusts depositIndex for the Bartio and Boonet chains to maintain backward compatibility due to known bugs. Remember to revisit and remove this conditional incrementing of depositIndex post-fork activation to prevent unnecessary adjustments.
mod/storage/pkg/deposit/store.go (1)
82-85: Logging level changed to Debug
Updating the logging level to Debug for GetDepositsByIndex helps reduce log verbosity and is appropriate for detailed debugging information.
mod/state-transition/pkg/core/validation_deposits.go (1)
146-148: Logging enhanced for deposit processing range
The added logging provides useful information about the range of deposits being processed, which aids in debugging and monitoring.
mod/state-transition/pkg/core/state_processor_genesis_test.go (2)
127-128: Clarified comments on deposit index handling for Betnet
The comment now clearly explains that on Betnet, the deposit index is set to the last accepted deposit, improving the understanding of the test expectations.
250-254: Clarified comments on deposit index handling for Bartio
The updated comment accurately reflects that on Bartio, the deposit index is off by one, aligning with the chain's specific behavior and enhancing test clarity.
mod/state-transition/pkg/core/state_processor_withdrawals.go (1)
276-281: Proper error handling added for setting the next withdrawal validator index
Adding error handling for st.SetNextWithdrawalValidatorIndex ensures that any issues setting the next index are caught and handled appropriately, improving robustness.
mod/state-transition/pkg/core/state_processor.go (2)
256-259: 🧹 Nitpick (assertive)
Consider reducing log level to Debug as previously suggested.
As noted in a previous review, this logging could be at Debug level since it's primarily for development/debugging purposes.
255-259: Verify the impact of deposit index adjustment.
The deposit index adjustment is specific to Boonet network and Fork2 height. This network-specific fix should be well documented and tested.
Run the following script to verify the deposit index adjustment implementation:
✅ Verification successful
The search results provide good context about the deposit index adjustment. Let me verify the implementation details further with additional scripts.
Based on the search results, I can now provide a verified response about the deposit index adjustment implementation:
The deposit index adjustment at BoonetFork2Height is a well-implemented fix for a known issue
The implementation is properly handled with:
- Clear boundary conditions checking slot against
BoonetFork2Height - Proper error handling and logging during the adjustment
- Consistent behavior across multiple components (state processor, validation, block builder)
- Test coverage for deposit index verification
- The fix decrements the deposit index by 1 to correct the off-by-one error inherited from Bartio
The adjustment is specifically designed to fix a pre-fork2 bug where DepositEth1ChainID was pointing to the next deposit index instead of the latest processed deposit index. The implementation includes proper validation skipping before the fork and appropriate handling after the fork activation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the deposit index adjustment implementation and its tests
# Test: Search for related test cases and documentation
rg -A 5 "BoonetFork2Height"
Length of output: 5062
Script:
#!/bin/bash
# Search for test cases specifically related to deposit index adjustment
rg -A 10 "GetEth1DepositIndex|SetEth1DepositIndex" mod/state-transition/pkg/core/
# Look for any documentation about this feature
fd README.md | xargs rg -i "deposit.*index|fork.*2"
Length of output: 11542
mod/beacon/validator/block_builder.go (1)
314-317: LGTM! Helpful logging addition.
The logging of deposit processing details (start_index and num_deposits) improves visibility and helps track deposit index adjustments.
mod/state-transition/pkg/core/state_processor_staking_test.go (2)
147-148: Improved epoch transition testing logic.
The loop condition for epoch transitions has been updated to be more precise:
-for i := 0; i < int(cs.SlotsPerEpoch()); i++ {
+currEpoch := cs.SlotToEpoch(blk.GetSlot())
+for currEpoch == cs.SlotToEpoch(blk.GetSlot()+1) {This change makes the tests more accurate by ensuring they run until the epoch actually changes.
Also applies to: 304-305, 690-692, 863-865, 1102-1104
44-44: Verify the impact of changing chain spec type from Boonet to Betnet.
The test file has been updated to use BetnetChainSpecType instead of BoonetChainSpecType. This change seems significant and needs verification.
Run the following script to understand the chain spec type usage:
Also applies to: 211-211, 596-596, 750-750, 932-932
✅ Verification successful
Let me gather more context about the chain spec types and their usage.
Let me check the chain spec implementations to understand the differences between these chain types.
The change from Boonet to Betnet chain spec type is intentional and correct
Based on the code analysis:
- Boonet has special cases and legacy behaviors (pre-fork2) that need to be maintained for backward compatibility
- Betnet is a newer chain spec that inherits from the base spec without the legacy behaviors
- The test file changes align with this transition, ensuring tests run against the newer, cleaner chain spec implementation
The change is part of moving away from the legacy Boonet implementation that had several special cases and bugs that needed workarounds (like deposit index handling, emergency minting, etc.).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the chain spec type usage across the codebase
# Test: Search for chain spec type usage
rg -A 3 "BoonetChainSpecType|BetnetChainSpecType"
Length of output: 3256
Script:
#!/bin/bash
# Check for chain spec type implementations and their differences
ast-grep --pattern 'func $_(ChainSpec) {
$$$
}'
# Also check for any references to these chain types in test files
rg -g "**/*_test.go" "setupChain.*ChainSpecType" -B 2 -A 2
Length of output: 4036
Script:
#!/bin/bash
# Look for chain spec implementations
rg -g "!*test.go" "func.*ChainSpec\(\)" -A 10
# Also check for any configuration differences
rg -g "!*test.go" "Betnet|Boonet" -B 2 -A 2
Length of output: 13624
| sp.logger.Info( | ||
| "Processed withdrawals", | ||
| "num_withdrawals", numWithdrawals, | ||
| "bera_inflation", float64( | ||
| payloadWithdrawals[0].GetAmount().Unwrap())/math.GweiPerWei, | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Ensure consistent terminology for inflation withdrawal in logging
Consider using consistent terminology in logging messages. The code references "EVM inflation," but the log key is "bera_inflation." Aligning these terms improves clarity.
A proposal to replace #2201
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Refactor