fix(state-transition): fix deposit index upon genesis processing #2116
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe pull request introduces several changes across multiple files, focusing on enhancing mock generation, testing capabilities, and state processing in the blockchain environment. Key updates include the addition of a new package in the Changes
Assessment against linked issues
Possibly related PRs
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 @@
## state-transition-add-UTs #2116 +/- ##
============================================================
+ Coverage 24.75% 24.81% +0.06%
============================================================
Files 357 357
Lines 16047 16057 +10
Branches 12 12
============================================================
+ Hits 3972 3985 +13
+ Misses 11879 11878 -1
+ Partials 196 194 -2
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 7
🧹 Outside diff range comments (2)
mod/state-transition/pkg/core/state_processor_genesis.go (1)
Line range hint
50-96: Consider documenting the genesis state handling designThe refactoring moves deposit index handling to processDeposit while using a flag to differentiate genesis processing. This is a good separation of concerns, but consider adding documentation to explain:
- Why the deposit index handling is special during genesis
- How processDeposit behaves differently during genesis
- The relationship between these changes and issue fix(eth1): deposit index is incorrect #1538
This will help future maintainers understand the design decisions.
mod/state-transition/pkg/core/state_processor.go (1)
Line range hint
171-177: Consider explicit initialization of processingGenesis.While the default
falsevalue is correct, consider explicitly initializingprocessingGenesisin the constructor for better clarity:return &StateProcessor[ BeaconBlockT, BeaconBlockBodyT, BeaconBlockHeaderT, BeaconStateT, ContextT, DepositT, Eth1DataT, ExecutionPayloadT, ExecutionPayloadHeaderT, ForkT, ForkDataT, KVStoreT, ValidatorT, ValidatorsT, WithdrawalT, WithdrawalsT, WithdrawalCredentialsT, ]{ cs: cs, executionEngine: executionEngine, signer: signer, + processingGenesis: false, }
🛑 Comments failed to post (7)
mod/state-transition/pkg/core/helpers_test.go (2)
43-78: 🧹 Nitpick (assertive)
Add documentation for the test type aliases.
These type aliases are complex and their purpose isn't immediately clear. Consider adding documentation comments explaining:
- The purpose of each type alias
- How they relate to the production types they're based on
- Example usage in tests
96-138: 🧹 Nitpick (assertive)
Improve readability of function signature using type alias.
The function signature is quite verbose due to the generic type parameters. Consider using the
TestKVStoreTtype alias that was defined earlier to simplify it:-func initTestStore() ( - *beacondb.KVStore[ - *types.BeaconBlockHeader, - *types.Eth1Data, - *types.ExecutionPayloadHeader, - *types.Fork, - *types.Validator, - types.Validators, - ], error) { +func initTestStore() (*TestKVStoreT, error) {Committable suggestion was skipped due to low confidence.
mod/state-transition/pkg/core/mocks/execution_engine.mock.go (1)
74-86: 🧹 Nitpick (assertive)
Consider adding nil check for testing interface.
While the implementation is correct, consider adding a defensive nil check for the testing interface to prevent potential panics.
func NewExecutionEngine[ExecutionPayloadT core.ExecutionPayload[ExecutionPayloadT, ExecutionPayloadHeaderT, WithdrawalsT], ExecutionPayloadHeaderT any, WithdrawalsT core.Withdrawals](t interface { mock.TestingT Cleanup(func()) }) *ExecutionEngine[ExecutionPayloadT, ExecutionPayloadHeaderT, WithdrawalsT] { + if t == nil { + panic("testing interface cannot be nil") + } mock := &ExecutionEngine[ExecutionPayloadT, ExecutionPayloadHeaderT, WithdrawalsT]{} mock.Mock.Test(t)📝 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.// NewExecutionEngine creates a new instance of ExecutionEngine. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. // The first argument is typically a *testing.T value. func NewExecutionEngine[ExecutionPayloadT core.ExecutionPayload[ExecutionPayloadT, ExecutionPayloadHeaderT, WithdrawalsT], ExecutionPayloadHeaderT any, WithdrawalsT core.Withdrawals](t interface { mock.TestingT Cleanup(func()) }) *ExecutionEngine[ExecutionPayloadT, ExecutionPayloadHeaderT, WithdrawalsT] { if t == nil { panic("testing interface cannot be nil") } mock := &ExecutionEngine[ExecutionPayloadT, ExecutionPayloadHeaderT, WithdrawalsT]{} mock.Mock.Test(t) t.Cleanup(func() { mock.AssertExpectations(t) }) return mock }mod/state-transition/pkg/core/state_processor_genesis_test.go (2)
81-97: 🧹 Nitpick (assertive)
Consider adding edge cases to test deposits.
The current test cases cover basic scenarios with different deposit amounts. Consider adding edge cases such as:
- Maximum number of validators
- Zero amount deposits
- Duplicate public keys
102-106:
⚠️ Potential issueStrengthen mock expectations for signature verification.
The current mock setup accepts any arguments and always returns nil. Consider:
- Verifying the actual signature parameters being passed
- Testing failure scenarios where signature verification fails
// Example of stronger mock expectations mocksSigner.On( "VerifySignature", mock.MatchedBy(func(msg []byte) bool { // Add specific message validation return len(msg) > 0 }), mock.MatchedBy(func(sig []byte) bool { // Add specific signature validation return len(sig) == 96 }), mock.Anything, ).Return(nil)mod/state-transition/pkg/core/state_processor.go (1)
88-91: 🧹 Nitpick (assertive)
Enhance field documentation for better context.
The field addition aligns with the PR objective. Consider enhancing the comment to better explain the specific issue it addresses:
- // processingGenesis allows initializing correctly - // eth1 deposit index upon genesis + // processingGenesis tracks whether we are processing genesis state to ensure + // eth1 deposit index starts at zero and does not account for genesis validators.📝 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.// processingGenesis tracks whether we are processing genesis state to ensure // eth1 deposit index starts at zero and does not account for genesis validators. processingGenesis boolmod/state-transition/pkg/core/state_processor_staking.go (1)
88-100: 🛠️ Refactor suggestion
Use
errors.Wrapfinstead offmt.Errorffor consistent error handlingThe code uses
fmt.Errorfto wrap errors, but elsewhere in the codebase,errors.Wrapfis used. For consistency, consider replacingfmt.Errorfwitherrors.Wrapf.Apply this diff to make the change:
- return fmt.Errorf("failed retrieving eth1 deposit index: %w", err) + return errors.Wrapf(err, "failed retrieving eth1 deposit index")📝 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.var nextDepositIndex uint64 switch depositIndex, err := st.GetEth1DepositIndex(); { case err == nil: nextDepositIndex = depositIndex + 1 case sp.processingGenesis && err != nil: // Eth1DepositIndex may have not been set yet nextDepositIndex = 0 default: // Failed retrieving Eth1DepositIndex outside genesis is an error return errors.Wrapf(err, "failed retrieving eth1 deposit index") } if err := st.SetEth1DepositIndex(nextDepositIndex); err != nil { ``` Note: The suggestion assumes that the `errors` package is already imported in the file. If it's not, an additional import statement would need to be added to the file's import block: ```go import "github.com/pkg/errors"
| // check that validator index is duly set | ||
| latestValIdx, err := beaconState.GetEth1DepositIndex() | ||
| require.NoError(t, err) | ||
| require.Equal(t, uint64(len(deposits)-1), latestValIdx) |
There was a problem hiding this comment.
deposits should be zero-indexed, not 1-indexed as they currently are
Fixes #1538
The more general cleanup to drop Eth1Data is deferred to a future PR
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Documentation