feat(withdrawals): Fixed withdrawal for EVM inflation per block #2158
Conversation
WalkthroughThe pull request introduces significant updates across various modules, primarily focusing on the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lintlevel=error msg="Timeout exceeded: try increasing it by passing --timeout option" 🪧 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 #2158 +/- ##
==========================================
+ Coverage 26.28% 26.62% +0.33%
==========================================
Files 360 360
Lines 16243 16284 +41
Branches 12 12
==========================================
+ Hits 4270 4336 +66
+ Misses 11705 11679 -26
- Partials 268 269 +1
|
| if numWithdrawals == int(sp.cs.MaxWithdrawalsPerPayload()) { | ||
| // Next sweep starts after the latest withdrawal's validator index | ||
| nextValidatorIndex = | ||
| (expectedWithdrawals[len(expectedWithdrawals)-1].GetIndex() + 1) % | ||
| math.U64(totalValidators) |
There was a problem hiding this comment.
NOTE: fixing this according to spec (.GetIndex() is replaced by .GetValidatorIndex())
if len(expected_withdrawals) == MAX_WITHDRAWALS_PER_PAYLOAD:
# Next sweep starts after the latest withdrawal's validator index
next_validator_index = ValidatorIndex((expected_withdrawals[-1].validator_index + 1) % len(state.validators))![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 26
🧹 Outside diff range comments (2)
mod/config/pkg/spec/devnet.go (1)
Line range hint
37-40: Consider documenting possible error scenarios.Since the function now returns an error, it would be helpful to document the potential error cases that could be returned from
chain.NewChainSpec(). This helps API consumers better understand and handle error conditions.Add documentation like:
// DevnetChainSpec is the ChainSpec for the localnet. +// Returns an error if the chain specification validation fails or if required parameters are invalid. func DevnetChainSpec() (chain.Spec[mod/config/pkg/spec/testnet.go (1)
Line range hint
116-116: Implementation aligns with withdrawal specificationsThe configuration successfully supports the requirements:
- Maintains ETH 2.0 spec compliance with
MaxWithdrawalsPerPayload: 16- Enables EVM inflation as the first withdrawal through the new configuration values
- Implicitly allows for 15 validator withdrawals per block (16 total - 1 inflation)
This is a clean approach to implementing the requirements without modifying the core withdrawal limit.
Also applies to: 128-131
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (1)
mod/chain-spec/go.sumis excluded by!**/*.sum
📒 Files selected for processing (22)
mod/chain-spec/go.mod(1 hunks)mod/chain-spec/pkg/chain/chain_spec.go(3 hunks)mod/chain-spec/pkg/chain/data.go(1 hunks)mod/chain-spec/pkg/chain/errors.go(1 hunks)mod/chain-spec/pkg/chain/helpers_test.go(1 hunks)mod/config/pkg/spec/betnet.go(1 hunks)mod/config/pkg/spec/boonet.go(1 hunks)mod/config/pkg/spec/devnet.go(1 hunks)mod/config/pkg/spec/testnet.go(3 hunks)mod/da/pkg/store/pruner_test.go(1 hunks)mod/node-core/pkg/components/chain_spec.go(1 hunks)mod/node-core/pkg/components/dispatcher.go(1 hunks)mod/primitives/pkg/common/execution.go(2 hunks)mod/state-transition/pkg/core/core_test.go(2 hunks)mod/state-transition/pkg/core/errors.go(1 hunks)mod/state-transition/pkg/core/state/statedb.go(4 hunks)mod/state-transition/pkg/core/state_processor_genesis.go(1 hunks)mod/state-transition/pkg/core/state_processor_genesis_test.go(4 hunks)mod/state-transition/pkg/core/state_processor_staking.go(5 hunks)mod/state-transition/pkg/core/state_processor_staking_test.go(3 hunks)testing/e2e/config/defaults.go(1 hunks)testing/e2e/e2e_startup_test.go(2 hunks)
🧰 Additional context used
📓 Learnings (5)
mod/chain-spec/pkg/chain/data.go (1)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/chain-spec/pkg/chain/data.go:150-151
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/chain-spec/pkg/chain/data.go`, when adding new fields to the `SpecData` struct, follow the existing pattern for documentation, which may be minimal.
mod/state-transition/pkg/core/core_test.go (1)
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.
mod/state-transition/pkg/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.
mod/state-transition/pkg/core/state_processor_genesis_test.go (5)
Learnt from: abi87
PR: berachain/beacon-kit#2114
File: mod/state-transition/pkg/core/state_processor_genesis_test.go:79-176
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/state-transition/pkg/core/state_processor_genesis_test.go`, adding additional tests requires resetting the persistence component, which complicates the implementation.
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:145-175
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/state-transition/pkg/core/state_processor_genesis_test.go`, refactoring to extract assertion helpers will be revisited when adding unit tests for `Transition`.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking_test.go:416-416
Timestamp: 2024-11-12T11:12:56.774Z
Learning: In `mod/state-transition/pkg/core/state_processor_staking_test.go`, setting up the state via genesis is acceptable, and there is no need to set the state artificially in tests.
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/state_processor_staking_test.go (3)
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.
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.
🪛 golangci-lint
mod/state-transition/pkg/core/state_processor_staking.go
[high] 317-317: G115: integer overflow conversion uint64 -> int
(gosec)
🔇 Additional comments (40)
mod/chain-spec/go.mod (1)
6-6: 🧹 Nitpick (assertive)
Consider using a stable version for the errors module
The errors module is pinned to a pseudo-version (v0.0.0-20240610210054-bfdc14c4013c) which indicates it's tracking a specific commit. For better stability and versioning, consider using a tagged release version if available.
Let's check if there are any tagged releases:
mod/config/pkg/spec/betnet.go (2)
30-36: LGTM! Improved error handling following Go idioms.
The function signature change to include error return type follows Go's best practices for error handling and aligns with similar changes in other chain specification functions.
Line range hint 30-40: Verify the impact of this breaking change.
The function signature change from BetnetChainSpec() chain.Spec[...] to BetnetChainSpec() (chain.Spec[...], error) is a breaking change that requires updates in all calling code.
✅ Verification successful
Let me verify the error handling in the calling code with one more search to see the complete context of the usage.
No breaking change impact - error is already handled
The function signature change is safe as the only caller in mod/node-core/pkg/components/chain_spec.go already handles the error return through the err variable in the switch statement. All chain spec functions are handled consistently with error checking.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of BetnetChainSpec to ensure they handle the new error return.
# Search for direct function calls
rg -A 2 "BetnetChainSpec\(\)"
# Search for variable assignments that might need error handling
ast-grep --pattern 'spec := BetnetChainSpec()'
Length of output: 482
Script:
#!/bin/bash
# Get more context around the usage in chain_spec.go
rg -B 5 -A 5 "BetnetChainSpec\(\)" mod/node-core/pkg/components/chain_spec.go
Length of output: 386
mod/config/pkg/spec/boonet.go (2)
30-36: LGTM: Improved error handling in chain spec function.
The addition of error return type aligns with Go best practices and provides better error propagation capabilities.
Line range hint 30-40: Verify integration with withdrawal changes.
While the error handling changes look good, let's verify how this chain spec integrates with the withdrawal mechanism changes mentioned in the PR objectives.
✅ Verification successful
Based on the code analysis, I can now generate the final response:
Chain spec properly integrates with withdrawal mechanism
The BoonetChainSpec() inherits withdrawal-related configurations from BaseSpec() which includes:
- MaxWithdrawalsPerPayload set to 16 (mod/config/pkg/spec/testnet.go)
- MaxValidatorsPerWithdrawalsSweep set to 1<<14
- Proper integration with EVM inflation withdrawal mechanism
The withdrawal processing is fully implemented in the codebase with:
- Withdrawal validation in state processor
- Support for both full and partial withdrawals
- Proper withdrawal credentials handling
- Withdrawal index tracking
- Withdrawal sweep mechanism
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how this chain spec is used in the context of withdrawals
# Look for withdrawal-related configurations in chain specs
rg -A 5 "withdrawal" --type go
# Find usage of BoonetChainSpec in withdrawal processing
ast-grep --pattern 'BoonetChainSpec()' -A 5
# Look for withdrawal constants or configurations
rg -A 5 '(MaxWithdrawalsPerPayload|WithdrawalLimit)' --type go
Length of output: 135047
mod/config/pkg/spec/devnet.go (1)
30-36: LGTM! Good error handling improvement.
The addition of the error return type aligns with Go's idiomatic error handling practices and matches the pattern used in other chain specification functions.
mod/node-core/pkg/components/chain_spec.go (3)
38-38: LGTM: Improved error handling in function signature
The change to return an error alongside the chain spec follows Go best practices for error handling.
55-55: LGTM: Proper error propagation
The function correctly propagates errors from the chain spec implementations.
47-53: Verify chain spec implementations
Need to ensure that all chain spec implementations (DevnetChainSpec, BetnetChainSpec, BoonetChainSpec, TestnetChainSpec) properly handle the new EVM inflation parameters mentioned in the PR objectives.
✅ Verification successful
Let me generate additional scripts to check the chain spec implementations specifically:
The first script will help us find the chain spec implementations, and the second will show us the EVM inflation configurations in different chain specs. This will allow us to verify if all chain specs properly handle the EVM inflation parameters.
Let me try one more search to find all chain spec implementations and their configurations:
This will help us verify if all chain specs (Devnet, Betnet, Boonet, and Testnet) properly handle the EVM inflation parameters.
All chain specs properly handle EVM inflation parameters
The verification shows that all chain specs (Devnet, Betnet, Boonet, and Testnet) properly handle the EVM inflation parameters through a shared implementation in BaseSpec(). Each chain spec function (DevnetChainSpec, BetnetChainSpec, BoonetChainSpec, TestnetChainSpec) uses this base specification, which includes:
EVMInflationAddress: Set to "0x6942069420694206942069420694206942069420"EVMInflationPerBlock: Set to 10e9 (10 Gwei)
The implementation ensures consistent EVM inflation parameters across all chain specs, as they only override the DepositEth1ChainID while inheriting all other configurations from BaseSpec().
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for EVMInflation-related configurations in chain spec implementations
ast-grep --pattern 'func $_(ChainSpec|SpecData)() ($_, $_) {
$$$
EVMInflation$$$
$$$
}'
# Verify the withdrawal limit implementation
rg -A 5 "MaxWithdrawalsPerPayload|EVMInflation"
Length of output: 20166
Script:
#!/bin/bash
# Search for chain spec implementations
ast-grep --pattern 'func $_(ChainSpec)() (*chain.SpecData, error) {
$$$
}'
# Search for chain spec data in config files
rg -A 10 "EVMInflationAddress|EVMInflationPerBlock" mod/config/pkg/spec/
Length of output: 486
Script:
#!/bin/bash
# Find all chain spec implementation files
fd "spec.go" mod/config/pkg/spec/
# Show the content of these files to verify EVM inflation parameters
rg -l "ChainSpec" mod/config/pkg/spec/ | xargs cat
Length of output: 8647
testing/e2e/e2e_startup_test.go (2)
24-28: LGTM: Required imports added for new test functionality
The new imports are necessary and properly organized for handling big integers and Ethereum common types.
44-46: Verify interaction with withdrawal mechanism
While this test effectively verifies EVM inflation, consider adding test cases that verify the interaction between EVM inflation and validator withdrawals, especially considering the ETH 2.0 spec requirement of maximum 16 withdrawals per payload (15 validator withdrawals + 1 EVM inflation withdrawal).
mod/node-core/pkg/components/dispatcher.go (2)
60-62: LGTM! Improved code formatting.
The multi-line formatting enhances readability while maintaining the same functionality.
Line range hint 60-67: Verify withdrawal-related event handling.
Given that this PR implements changes to the withdrawal mechanism and EVM inflation, let's verify that all necessary events are properly registered in the dispatcher.
✅ Verification successful
Based on the extensive search results, I can now provide a final response regarding the withdrawal-related event handling verification:
All required withdrawal-related events are properly registered in the dispatcher
The code shows the dispatcher correctly registers all necessary withdrawal-related events:
FinalSidecarsReceived- For handling finalized blob sidecar dataFinalBeaconBlockReceived- For handling finalized beacon blocks that may contain withdrawalsFinalValidatorUpdatesProcessed- For handling finalized validator state updates including withdrawalsBeaconBlockFinalized- For handling block finalization that includes withdrawal processing
Additionally, the codebase has comprehensive withdrawal handling infrastructure:
- Proper withdrawal validation and processing in state transitions
- EVM inflation withdrawal handling as required first withdrawal in each block
- Withdrawal credentials and withdrawal index tracking
- Maximum withdrawal limits enforcement
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for withdrawal-related event definitions and their usage
# Check for withdrawal-related event definitions
echo "Checking for withdrawal event definitions..."
rg -A 2 "type.*Event.*Withdraw"
# Check for withdrawal event registrations in other dispatcher configurations
echo "Checking for withdrawal event registrations..."
ast-grep --pattern 'WithEvent[async.Event[$_]]($$$Withdraw$$$)'
# Check for EVM inflation related events
echo "Checking for EVM inflation related events..."
rg -A 2 "type.*Event.*Inflation"
Length of output: 435
Script:
#!/bin/bash
# Let's try a broader search to understand the event system and withdrawal-related code
# Search for all event type definitions to understand the event system
echo "Checking all event type definitions..."
rg "type.*Event" -A 2
# Search for withdrawal-related code patterns
echo "Checking withdrawal-related patterns..."
rg -i "withdraw" -A 3
# Search for event registrations in dispatcher
echo "Checking event registrations in dispatcher..."
rg "WithEvent\[" -A 1
# Search for inflation-related code
echo "Checking inflation-related code..."
rg -i "inflation" -A 3
Length of output: 430856
mod/da/pkg/store/pruner_test.go (1)
Line range hint 123-128: LGTM! Good simplification of event creation
The removal of explicit type parameters makes the code more concise while maintaining type safety through inference.
mod/config/pkg/spec/testnet.go (3)
32-38: LGTM: Improved error handling
The addition of error return type enhances robustness by allowing proper error propagation from chain spec creation.
62-66: LGTM: Cleaner constant declarations
The simplified syntax for Gwei constants improves readability while maintaining the correct values aligned with Ethereum specifications.
128-131: Verify EVM inflation configuration values
The new configuration introduces:
- A hardcoded inflation address:
0x6942069420694206942069420694206942069420 - A significant inflation rate of 10 ETH per block
Please verify:
- Is the inflation address intended to be hardcoded in testnet config?
- Is the inflation rate of 10 ETH per block the intended value?
- Should these values be documented in the project documentation?
✅ Verification successful
Let me gather more context about these values by checking the test configurations and related code.
Based on the comprehensive results, I can now provide a final response:
EVM inflation configuration values are well-documented and intentionally set
The values are properly documented and integrated into the system:
- The inflation address and rate are consistently used across testnet and e2e test configurations
- The system is designed to require an EVM inflation withdrawal as the first withdrawal in every block
- The configuration is well-documented in the codebase:
EVMInflationAddress: "the address on the EVM which will receive the inflation amount of native EVM balance through a withdrawal every block"EVMInflationPerBlock: "the amount of native EVM balance (in Gwei) to be minted to the EVMInflationAddress via a withdrawal every block"
- E2E tests verify that the inflation mechanism works correctly by checking balance increases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any documentation or references to these values
echo "Searching for documentation of EVM inflation values..."
rg -i "inflation.*per.*block|evm.*inflation|inflation.*address" --type md
# Check if these values are different in other chain configs
echo "Checking other chain configurations..."
fd -e go -e toml -e yaml | xargs rg "0x694206942069420694206942069420694206942|10e9.*inflation"
Length of output: 562
Script:
#!/bin/bash
# Check for any tests or code that might explain these values
echo "Checking for related test cases and code..."
rg -A 5 -B 5 "EVMInflationAddress|EVMInflationPerBlock"
# Check for any comments or documentation in Go files
echo "Checking for inline documentation..."
fd -e go | xargs rg -A 2 -B 2 "// .*[Ii]nflation|/\* .*[Ii]nflation"
# Check for any configuration structs or interfaces defining these parameters
echo "Checking for configuration definitions..."
ast-grep --pattern 'type $_ struct {
$$$
EVMInflation$_
$$$
}'
Length of output: 18709
mod/state-transition/pkg/core/errors.go (1)
84-92: LGTM! Well-defined error messages for EVM inflation withdrawals.
The new error variables ErrNoWithdrawals and ErrFirstWithdrawalNotEVMInflation are well-defined and directly support the PR objective of enforcing EVM inflation as the first withdrawal in each block.
mod/state-transition/pkg/core/state_processor_genesis.go (2)
147-148: LGTM! Error handling consolidation looks good.
The consolidation of error handling for SetNextWithdrawalValidatorIndex improves code readability while maintaining the same functionality.
146-149: Verify withdrawal indices initialization impact.
The initialization of both withdrawal indices to 0 is crucial for the new EVM inflation withdrawal mechanism. Let's verify this initialization aligns with the withdrawal processing implementation.
✅ Verification successful
Let me gather more information about the withdrawal processing implementation to verify the initialization impact.
Let me search for the withdrawal processing implementation one more time with a different pattern.
Based on the gathered information, I can now provide a final response:
Initialization of withdrawal indices to 0 is correctly implemented and necessary
The initialization aligns with the withdrawal processing implementation as evidenced by:
- The withdrawal processing logic in
state_processor_staking.goexpects these indices to be initialized and uses them to track withdrawal progress - The state processor increments these indices during withdrawal processing and wraps them around total validators
- The system enforces
MaxWithdrawalsPerPayloadconstraints using these indices - The indices are properly stored in the beacon state and database layer with appropriate getters/setters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the withdrawal processing implementation's assumptions about initial indices
# Check if the withdrawal processing code assumes these initial values
ast-grep --pattern 'func (sp *StateProcessor[$$$]) processWithdrawals($$$) {
$$$
nextWithdrawalIndex := $$$
nextWithdrawalValidatorIndex := $$$
$$$
}'
# Look for any validation of initial indices
rg -A 5 'SetNextWithdrawal(Index|ValidatorIndex)' --type go
Length of output: 6651
Script:
#!/bin/bash
# Search for withdrawal processing implementation and index usage
ast-grep --pattern 'func ($_ *StateProcessor) processWithdrawals($$$) {
$$$
}'
# Look for withdrawal index validation or requirements
rg -A 10 'GetNextWithdrawal(Index|ValidatorIndex)' --type go
# Search for any withdrawal-related tests that might verify initial state
rg -A 5 'TestProcess.*Withdrawal' --type go
Length of output: 20745
Script:
#!/bin/bash
# Search for withdrawal processing implementation in staking processor
rg -A 20 'processWithdrawals' --type go
# Look for withdrawal-related state transitions
ast-grep --pattern 'func ($_ *StateProcessor) process$$$Withdrawals($$$) {
$$$
}'
# Search for withdrawal-related constants or configuration
rg 'WithdrawalIndex|MaxWithdrawals' --type go
Length of output: 18042
mod/primitives/pkg/common/execution.go (1)
24-24: LGTM: Import addition is correct
The bytes package import is properly placed and utilized by the new Equals method.
mod/state-transition/pkg/core/state_processor_genesis_test.go (4)
28-28: LGTM: Import addition for components package
The new import supports the updated chain specification type usage in the tests.
37-38: LGTM: Improved test setup with setupChain
The test setup has been streamlined using setupChain, which aligns with previous feedback about state initialization preferences. This change improves test maintainability.
Also applies to: 131-132
86-101: LGTM: Comprehensive state validations
The state validations are thorough and well-structured, with consistent error handling. The checks cover all necessary scenarios including slot, fork, and validator indices.
Also applies to: 180-194
43-65: 🧹 Nitpick (assertive)
Consider documenting test scenarios
While the deposit initialization is now more systematic with sequential indices, consider adding a brief comment block explaining what each deposit test case is validating (e.g., max balance, partial balance, minimum balance scenarios).
Example documentation:
// Test cases:
// 1. Validator with max effective balance
// 2. Validator with half max effective balance
// 3. Validator with minimum effective balance
// 4. Validator with double max effective balance (should be capped)
// 5. Validator with below minimum balanceAlso applies to: 137-159
⛔ Skipped due to learnings
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.
mod/state-transition/pkg/core/core_test.go (3)
33-51: LGTM: Imports are well-organized and necessary.
The new imports are properly organized and align with the new functionality being added for chain setup and state processing.
152-162: 🧹 Nitpick (assertive)
Consider test isolation for parallel execution.
While the function is well-structured, setting environment variables via t.Setenv could cause issues if tests are run in parallel, as environment variables are process-wide.
Let's check if any tests are using t.Parallel():
Consider passing the chain spec type as a parameter instead of using an environment variable:
-func setupChain(t *testing.T, chainSpecType string) chain.Spec[
+func setupChainWithType(t *testing.T, chainSpecType string, useEnv bool) chain.Spec[
bytes.B4, math.U64, common.ExecutionAddress, math.U64, any,
] {
t.Helper()
- t.Setenv(components.ChainSpecTypeEnvVar, chainSpecType)
+ if useEnv {
+ t.Setenv(components.ChainSpecTypeEnvVar, chainSpecType)
+ }
cs, err := components.ProvideChainSpec()
require.NoError(t, err)
return cs
}235-238: Verify withdrawal limits configuration in chain spec.
Given that this PR implements ETH 2.0 withdrawal mechanism with specific limits (16 withdrawals per payload, first being EVM inflation), ensure that these limits are properly configured in the chain specification.
Let's check the chain spec configuration:
✅ Verification successful
Let me check the actual configuration values to ensure they match the requirements:
Chain spec properly supports withdrawal configuration
The chain specification correctly defines all required withdrawal-related configurations:
MaxWithdrawalsPerPayloadfield exists to limit withdrawals per payloadEVMInflationAddressandEVMInflationPerBlockfields handle EVM inflation withdrawals- Error handling ensures at least 2 withdrawals (1 for EVM inflation + 1 for validator)
The test configuration shows a minimal working setup with MaxWithdrawalsPerPayload: 2, which validates the error handling. While the actual production value of 16 would be set in deployment configs, the chain spec infrastructure fully supports the withdrawal mechanism requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for withdrawal limits configuration
# Look for withdrawal-related configuration in chain spec files
rg -A 5 'withdrawal|WithdrawalLimit|EVMInflation' 'mod/chain-spec/pkg/'
Length of output: 6168
Script:
#!/bin/bash
# Look for the actual configuration values in chain spec files
rg -l '\.ya?ml' 'mod/chain-spec/'
Length of output: 33
Script:
#!/bin/bash
# Try finding config files with different extensions and locations
fd -e yaml -e yml -e json . 'mod/chain-spec/'
# Also check for test files that might contain configuration
rg -A 5 'NewChainSpec|chainSpec.*Data' 'mod/chain-spec/pkg/'
Length of output: 1028
Script:
#!/bin/bash
# Check test files for withdrawal configuration values
rg -A 10 'MaxWithdrawalsPerPayload.*:|EVMInflation.*:' 'mod/chain-spec/pkg/'
# Also check for any test files containing withdrawal-related test cases
rg -A 5 'func.*Test.*Withdrawal' 'mod/chain-spec/pkg/'
Length of output: 2197
mod/chain-spec/pkg/chain/data.go (1)
147-147: LGTM: Comment style improvement
The change from "CometValues" to "Comet Values" improves consistency with other section headers in the file.
mod/chain-spec/pkg/chain/chain_spec.go (1)
504-518: Verify SpecData struct fields
The implementation looks correct. However, we should verify that the corresponding fields in the SpecData struct are properly defined.
✅ Verification successful
Fields are properly defined in SpecData struct
The verification confirms that both fields are correctly defined in the SpecData struct with matching types:
EVMInflationAddressis defined asExecutionAddressTtypeEVMInflationPerBlockis defined asuint64type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify SpecData struct definition in data.go
# Expected: Fields EVMInflationAddress and EVMInflationPerBlock should be present
# Search for SpecData struct definition
rg -A 5 "type SpecData\[" mod/chain-spec/pkg/chain/data.go
# Verify field definitions
rg "EVMInflationAddress\s+ExecutionAddressT" mod/chain-spec/pkg/chain/data.go
rg "EVMInflationPerBlock\s+uint64" mod/chain-spec/pkg/chain/data.go
Length of output: 481
mod/state-transition/pkg/core/state/statedb.go (6)
190-192: Modification from spec is well-documented
The added comments clearly explain that ExpectedWithdrawals has been modified to include a fixed withdrawal for EVM inflation. This enhances code readability and maintainability.
202-202: Variable declaration is appropriate
Declaring withdrawal WithdrawalT prepares for creating new withdrawal instances. This is correctly added for the subsequent logic.
205-207: EVM inflation withdrawal added correctly
Appending the EVM inflation withdrawal as the first entry ensures it is processed prior to validator withdrawals. This aligns with the intended functionality.
236-240: Withdrawal capping logic is correct
The added check effectively caps the number of withdrawals to MaxWithdrawalsPerPayload(), accounting for the EVM inflation withdrawal. This prevents processing more withdrawals than allowed per payload.
297-298: Validator index increment and wrap-around are correctly implemented
Incrementing validatorIndex and applying modulus with totalValidators ensures continuous cycling through validators without index overflow.
304-319:
Verify the use of zero indices in EVMInflationWithdrawal
Setting withdrawalIndex and validatorIndex to zero in EVMInflationWithdrawal may lead to conflicts if any part of the system relies on these indices being unique or mapping to actual validators. It's important to ensure that using zero does not cause unintended side effects.
Run the following script to check for dependencies that may be affected by zero indices:
mod/state-transition/pkg/core/state_processor_staking_test.go (4)
38-39: Good use of modular setup functions
The introduction of setupChain and setupState functions enhances test setup modularity and consistency across different test cases.
115-124: 🧹 Nitpick (assertive)
Consider simplifying balance assertions
In the TestTransitionUpdateValidators function, multiple assertions check the validator's balance and effective balance. Based on prior learnings, adding extensive balance checks may be unnecessary if the main functionality is already verified. Simplifying these assertions can improve test readability.
131-213: 🧹 Nitpick (assertive)
Avoid adding verbosity to substantial tests
The TestTransitionWithdrawals function is quite extensive. As learned previously, when a test becomes substantial in size, it's beneficial to avoid adding verbose code or comments that increase its length further. Consider splitting the test into smaller, focused tests or simplifying where possible to enhance readability and maintainability.
215-353: 🧹 Nitpick (assertive)
Refactor large test into smaller units
The TestTransitionMaxWithdrawals function is substantial and covers multiple scenarios within a single test. To improve clarity and maintainability, consider refactoring this test into smaller, focused tests that each verify a specific aspect of the withdrawal logic, such as:
- Testing the maximum withdrawals per payload constraint.
- Verifying the state's next withdrawal index incrementation.
- Checking individual validator balances post-withdrawal.
There was a problem hiding this comment.
Actionable comments posted: 24
🧹 Outside diff range comments (2)
mod/config/pkg/spec/betnet.go (1)
Line range hint
37-40: Consider adding EVM inflation configuration parametersSince this PR implements fixed EVM inflation withdrawals, consider adding configuration parameters for:
- The fixed withdrawal amount per block
- The predetermined withdrawal address (index 0)
This would make the configuration more explicit and maintainable.
func BetnetChainSpec() (chain.Spec[common.DomainType, math.Epoch, common.ExecutionAddress, math.Slot, any], error) { testnetSpec := BaseSpec() testnetSpec.DepositEth1ChainID = BetnetEth1ChainID + // Configure EVM inflation parameters + testnetSpec.EVMInflationWithdrawalAddress = common.ExecutionAddress{} // Set the predetermined address + testnetSpec.EVMInflationAmountPerBlock = uint64(1000) // Set the fixed amount per block return chain.NewChainSpec(testnetSpec) }mod/da/pkg/store/pruner_test.go (1)
Line range hint
123-129: LGTM: Event creation simplifiedThe removal of explicit type parameters is a good cleanup as they can be inferred from the
MockBeaconBlockparameter.Consider adding a comment explaining that the event types are inferred from the
MockBeaconBlockparameter for better code maintainability:+// Event types are inferred from MockBeaconBlock parameter event := async.NewEvent(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (1)
mod/chain-spec/go.sumis excluded by!**/*.sum
📒 Files selected for processing (22)
mod/chain-spec/go.mod(1 hunks)mod/chain-spec/pkg/chain/chain_spec.go(3 hunks)mod/chain-spec/pkg/chain/data.go(1 hunks)mod/chain-spec/pkg/chain/errors.go(1 hunks)mod/chain-spec/pkg/chain/helpers_test.go(1 hunks)mod/config/pkg/spec/betnet.go(1 hunks)mod/config/pkg/spec/boonet.go(1 hunks)mod/config/pkg/spec/devnet.go(1 hunks)mod/config/pkg/spec/testnet.go(3 hunks)mod/da/pkg/store/pruner_test.go(1 hunks)mod/node-core/pkg/components/chain_spec.go(1 hunks)mod/node-core/pkg/components/dispatcher.go(1 hunks)mod/primitives/pkg/common/execution.go(2 hunks)mod/state-transition/pkg/core/core_test.go(2 hunks)mod/state-transition/pkg/core/errors.go(1 hunks)mod/state-transition/pkg/core/state/statedb.go(4 hunks)mod/state-transition/pkg/core/state_processor_genesis.go(1 hunks)mod/state-transition/pkg/core/state_processor_genesis_test.go(4 hunks)mod/state-transition/pkg/core/state_processor_staking.go(5 hunks)mod/state-transition/pkg/core/state_processor_staking_test.go(3 hunks)testing/e2e/config/defaults.go(1 hunks)testing/e2e/e2e_startup_test.go(2 hunks)
🧰 Additional context used
📓 Learnings (5)
mod/chain-spec/pkg/chain/data.go (1)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/chain-spec/pkg/chain/data.go:150-151
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/chain-spec/pkg/chain/data.go`, when adding new fields to the `SpecData` struct, follow the existing pattern for documentation, which may be minimal.
mod/state-transition/pkg/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.
mod/state-transition/pkg/core/state_processor_genesis_test.go (5)
Learnt from: abi87
PR: berachain/beacon-kit#2114
File: mod/state-transition/pkg/core/state_processor_genesis_test.go:79-176
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/state-transition/pkg/core/state_processor_genesis_test.go`, adding additional tests requires resetting the persistence component, which complicates the implementation.
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:145-175
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/state-transition/pkg/core/state_processor_genesis_test.go`, refactoring to extract assertion helpers will be revisited when adding unit tests for `Transition`.
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.
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking_test.go:416-416
Timestamp: 2024-11-12T11:12:56.774Z
Learning: In `mod/state-transition/pkg/core/state_processor_staking_test.go`, setting up the state via genesis is acceptable, and there is no need to set the state artificially in tests.
mod/state-transition/pkg/core/state_processor_staking.go (1)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking.go:176-179
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/state-transition/pkg/core/state_processor_staking.go`, when a validator is marked as withdrawable, the withdrawals and evictions occur in the next epoch.
mod/state-transition/pkg/core/state_processor_staking_test.go (3)
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.
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.
🪛 golangci-lint
mod/state-transition/pkg/core/state_processor_staking.go
[high] 317-317: G115: integer overflow conversion uint64 -> int
(gosec)
🔇 Additional comments (42)
mod/chain-spec/go.mod (1)
6-24: Verify dependency versions for security concerns
Let's verify that all new dependencies are using secure versions and check for any known vulnerabilities.
✅ Verification successful
Dependencies are using secure versions
The security check reveals that all dependencies are using secure versions:
github.com/cockroachdb/errors v1.11.3- No known vulnerabilitiesgithub.com/getsentry/sentry-go v0.28.1- No known vulnerabilitiesgithub.com/gogo/protobuf v1.3.2- Using the patched version that fixes the high severity vulnerability (CVE reported for versions < 1.3.2)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for security advisories and latest versions of new dependencies
# Check for security advisories for cockroachdb/errors
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/cockroachdb/errors") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
# Check for security advisories for getsentry/sentry-go
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/getsentry/sentry-go") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
# Check for security advisories for gogo/protobuf
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/gogo/protobuf") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 1283
testing/e2e/config/defaults.go (1)
35-37: LGTM! Clear section structure and valid address format.
The EVM inflation section is well-organized and follows the file's existing pattern. The inflation address uses a valid Ethereum format suitable for testing purposes.
mod/chain-spec/pkg/chain/errors.go (1)
25-31: LGTM! Error handling implementation looks good.
The error variable is well-documented and correctly implements the requirement for minimum withdrawals per payload.
mod/config/pkg/spec/betnet.go (1)
30-36: Verify MaxWithdrawalsPerBlock configuration in BaseSpec
The PR requires MaxWithdrawalsPerBlock >= 2 to support both EVM inflation and validator withdrawals. Let's verify this configuration in the base specification.
✅ Verification successful
MaxWithdrawalsPerPayload is already configured correctly
The BaseSpec configuration already has MaxWithdrawalsPerPayload: 16 which is more than sufficient to support both EVM inflation and validator withdrawals (requirement >= 2). Additionally, the spec includes proper EVM inflation configuration with:
EVMInflationAddressset to a specific addressEVMInflationPerBlockset to 10e9
No changes are needed as the configuration already meets the requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check BaseSpec implementation for MaxWithdrawalsPerBlock configuration
# Expected: Should find MaxWithdrawalsPerBlock configuration with value >= 2
# Search for MaxWithdrawalsPerBlock configuration in spec files
rg -A 5 "MaxWithdrawalsPerBlock" ./mod/config/pkg/spec/
Length of output: 55
Script:
#!/bin/bash
# Let's try a broader search for withdrawal-related configurations
rg -i "withdraw" ./mod/config/pkg/spec/
# Also search for BaseSpec to understand the configuration structure
rg "BaseSpec" ./mod/config/pkg/spec/
# Search for any numeric constants that might be related to withdrawals
rg "\d+\s*=.*withdraw" ./mod/config/pkg/spec/ -i
Length of output: 580
Script:
#!/bin/bash
# Let's examine the BaseSpec implementation since it's used by betnet.go
cat ./mod/config/pkg/spec/testnet.go
# Also check the betnet implementation to see if it overrides any withdrawal configs
cat ./mod/config/pkg/spec/betnet.go
Length of output: 5691
mod/config/pkg/spec/boonet.go (2)
Line range hint 37-40: LGTM: Implementation properly propagates errors
The function correctly propagates potential errors from chain.NewChainSpec, which aligns with the broader error handling improvements.
30-36: Breaking change: Function signature updated to include error handling
The addition of error return improves robustness but requires updates to all callers of this function.
Let's verify the impact on consumers:
✅ Verification successful
All callers properly handle the error return - no action needed
The verification shows that the only caller of BoonetChainSpec() is in mod/node-core/pkg/components/chain_spec.go and it already handles the error return value correctly through the err variable. No interface definitions were found that need updating.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all direct usages of BoonetChainSpec to ensure they handle the new error return
# Search for function calls
rg -A 2 "BoonetChainSpec\(\)"
# Search for any potential interface definitions that might need updating
ast-grep --pattern 'interface {
$$$
BoonetChainSpec()
$$$
}'
Length of output: 486
mod/config/pkg/spec/devnet.go (2)
30-36: LGTM: Improved error handling in chain spec functions.
The function signature change to include error handling aligns with good Go practices and maintains consistency with other chain specification functions.
Line range hint 37-40: Verify BaseSpec withdrawal configuration.
Since this PR implements fixed EVM inflation withdrawals, we should verify that the BaseSpec includes the correct withdrawal configurations.
✅ Verification successful
Based on the results, I can see that BaseSpec() includes both the required withdrawal configurations:
- MaxWithdrawalsPerPayload is set to 16 (line 116)
- EVMInflationAddress is configured with a specific address (lines 128-130)
- EVMInflationPerBlock is set to 10e9 (line 131)
I can now generate the final response:
BaseSpec withdrawal configuration verified and properly configured
The BaseSpec includes all necessary withdrawal configurations for EVM inflation:
- MaxWithdrawalsPerPayload is set to 16, which is sufficient for handling withdrawals
- EVMInflationAddress is properly configured with a dedicated address
- EVMInflationPerBlock is set to 10e9
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify BaseSpec withdrawal configurations
# Expected: Find MaxWithdrawalsPerPayload >= 2 and EVM inflation address configuration
# Check BaseSpec implementation
ast-grep --pattern 'func BaseSpec() $_ {
$$$
}'
# Look for withdrawal-related constants and configurations
rg -l "MaxWithdrawalsPerPayload|MaxWithdrawalsPerBlock|EvmInflationAddress" --type go
Length of output: 6369
mod/node-core/pkg/components/chain_spec.go (1)
38-38: Verify all callers of ProvideChainSpec
The signature change to include error handling is good practice, but we need to ensure all callers are updated.
✅ Verification successful
All callers of ProvideChainSpec have been updated
Based on the search results, there is only one caller of ProvideChainSpec() in mod/state-transition/pkg/core/core_test.go and it already properly handles the error return value with require.NoError(t, err). No other callers were found in the codebase, including wire configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all callers of ProvideChainSpec to verify they handle the new error return
rg "ProvideChainSpec\(\)" -A 2
Length of output: 510
Script:
#!/bin/bash
# Let's check for any wire.go files that might be using this provider
rg "wire\.(Bind|Value|Provider)\(.*ProvideChainSpec" -A 2
# Also check for any other potential usages with different patterns
rg "chain.*:= .*ProvideChainSpec|spec.*:= .*ProvideChainSpec" -A 2
Length of output: 128
testing/e2e/e2e_startup_test.go (2)
24-28: LGTM! Required imports added for EVM inflation testing.
The new imports are necessary and properly organized for implementing the EVM inflation test.
44-46: Consider adding tests for withdrawal positioning.
While this test verifies the inflation amount, it doesn't validate that the EVM inflation withdrawal is correctly positioned as the first withdrawal in each block, as specified in the PR objectives. Consider adding test cases to verify:
- EVM inflation withdrawal is always at index 0
- MaxWithdrawalsPerBlock constraint is enforced
mod/node-core/pkg/components/dispatcher.go (1)
60-62: LGTM! Improved code formatting for better readability.
The multi-line formatting of the WithEvent call improves code readability while maintaining the same functionality.
mod/da/pkg/store/pruner_test.go (2)
119-119: LGTM: Proper error handling added
The error check is appropriately placed and uses the correct assertion method.
110-118: Verify minimum MaxWithdrawalsPerPayload value across test suites
The value of 2 for MaxWithdrawalsPerPayload aligns with the PR objectives (1 for EVM inflation + 1 for validator withdrawals). However, let's verify this minimum value is consistently enforced across other test files.
✅ Verification successful
Setting MaxWithdrawalsPerPayload to 2 is valid but differs from network defaults
The value of 2 for MaxWithdrawalsPerPayload in the test is valid as it meets the minimum requirement enforced by the codebase (must be > 1 per ErrZeroMaxWithdrawalsPerPayload). However, it's worth noting that:
- The network default is 16 (defined in
mod/primitives/pkg/constants/payload.go) - The test is specifically using the minimum value to test edge cases
- The validation in
chain_spec.goensures values <= 1 are rejected withErrZeroMaxWithdrawalsPerPayload
The test value is intentionally different from the network default to test the minimum viable configuration (1 for EVM inflation + 1 for validator withdrawals).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other test files that might be setting MaxWithdrawalsPerPayload
# to ensure consistent minimum value enforcement
rg -g '**/*_test.go' 'MaxWithdrawalsPerPayload.*[0-1]'
Length of output: 56
Script:
#!/bin/bash
# Let's broaden our search to find all MaxWithdrawalsPerPayload settings in test files
# and their values for a comprehensive check
rg -g '**/*_test.go' 'MaxWithdrawalsPerPayload\s*:.*\d+' -A 1
Length of output: 304
Script:
#!/bin/bash
# Also search for the ChainSpec initialization in test files
# to catch any indirect settings of MaxWithdrawalsPerPayload
ast-grep --pattern 'chain.NewChainSpec($$$)'
Length of output: 46
Script:
#!/bin/bash
# Let's also check the actual ChainSpec definition to understand
# if there are any constraints or default values
rg 'type\s+ChainSpec' -A 10
Length of output: 1498
Script:
#!/bin/bash
# Let's check the actual implementation or definition where MaxWithdrawalsPerPayload is defined
# to understand if there are any constraints or documentation about its minimum value
rg 'MaxWithdrawalsPerPayload' -B 2 -A 2
Length of output: 8902
Script:
#!/bin/bash
# Also check for any constants or variables that might be related to withdrawals per payload
# to ensure we're not missing any relevant configuration
rg -g '!*_test.go' '(const|var).*Withdrawal.*Payload'
Length of output: 436
mod/config/pkg/spec/testnet.go (3)
32-38: LGTM: Improved error handling
The addition of error return type enhances error propagation and aligns with robust error handling practices.
62-66: LGTM: Enhanced readability of Gwei constants
The use of scientific notation (e.g., 32e9 instead of explicit uint64 casts) improves readability while maintaining the same values.
128-131: 🧹 Nitpick (assertive)
Verify EVM inflation configuration
Several important considerations for the EVM inflation implementation:
-
The PR objectives specify that
MaxWithdrawalsPerBlockshould be at least 2 (one for EVM inflation, one for validator withdrawals), but this requirement isn't explicitly enforced in the configuration. -
The inflation amount is set to 10 Gwei per block (10e9), but there's no documentation explaining this specific value or its economic implications.
Let's verify the MaxWithdrawalsPerBlock configuration:
Consider:
- Adding a constant or validation for minimum
MaxWithdrawalsPerBlock - Adding documentation explaining the chosen inflation rate and its economic model
- Consider making the inflation rate configurable per network (testnet vs mainnet)
mod/state-transition/pkg/core/errors.go (1)
84-92: LGTM! Well-defined error variables for EVM inflation withdrawals.
The error variables ErrNoWithdrawals and ErrFirstWithdrawalNotEVMInflation are well-defined with clear messages that align with the PR objectives of implementing fixed EVM inflation withdrawals.
mod/state-transition/pkg/core/state_processor_genesis.go (2)
Line range hint 38-165: Verify withdrawal limits configuration
The PR requires MaxWithdrawalsPerBlock >= 2 to accommodate both EVM inflation and validator withdrawals. While this function initializes the withdrawal indices, we should ensure this requirement is enforced in the chain configuration.
Let's verify the withdrawal limits configuration:
#!/bin/bash
# Description: Verify the MaxWithdrawalsPerBlock configuration
# Look for the withdrawal limits configuration
rg -A 2 "MaxWithdrawalsPerBlock|MaxWithdrawalsPerPayload" --type go147-148: Verify initialization aligns with EVM inflation withdrawal requirements
Setting the next withdrawal validator index to 0 is crucial for the new EVM inflation withdrawal feature, as it ensures the first withdrawal slot is reserved for the fixed EVM inflation withdrawal in subsequent blocks.
Let's verify this initialization aligns with the withdrawal processing:
✅ Verification successful
Based on the search results from our previous queries, I can now generate a final response. The code shows that:
- The withdrawal validator index is used to track withdrawals in the system
- The first withdrawal in each block must be the EVM inflation withdrawal (from
ErrFirstWithdrawalNotEVMInflationerror) - The EVM inflation withdrawal is created with index 0 and validator index 0 (from
EVMInflationWithdrawal()implementation) - The chain spec enforces at least 2 withdrawals per payload (from
ErrZeroMaxWithdrawalsPerPayloaderror message)
Setting next withdrawal validator index to 0 is correct for genesis initialization
The initialization of the next withdrawal validator index to 0 in genesis is correct and necessary because:
- It reserves index 0 for the mandatory EVM inflation withdrawal that must be first in each block
- It ensures subsequent validator withdrawals start from index 1, maintaining proper separation between inflation and validator withdrawals
- It aligns with the chain specification requirement of having at least 2 withdrawal slots per block
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the withdrawal processing logic respects this initialization
# Look for the withdrawal processing code that handles the first withdrawal slot
ast-grep --pattern 'func $_($$$ BeaconStateT $$$) {
$$$
// Check if this is the first withdrawal
if $_ == 0 {
$$$
}
$$$
}'
Length of output: 6221
Script:
#!/bin/bash
# Let's search for withdrawal processing logic in a different way
# First, let's look for withdrawal-related functions and their index handling
rg -A 5 'func.*[Ww]ithdrawal.*Index' --type go
# Then, let's specifically look for the usage of this initialization value
rg -A 5 'GetNextWithdrawalValidatorIndex|SetNextWithdrawalValidatorIndex' --type go
# Finally, let's look for EVM inflation withdrawal related code
rg -A 5 'EVMInflation.*[Ww]ithdrawal' --type go
Length of output: 52199
Script:
#!/bin/bash
# Let's look for withdrawal processing logic to understand how validator indices are used
rg -A 10 'func.*ProcessWithdrawals' --type go
# Also check for any validation of withdrawal indices
rg -A 5 'ValidateWithdrawal|validateWithdrawal' --type go
Length of output: 107
mod/primitives/pkg/common/execution.go (2)
24-24: LGTM: Import addition is appropriate
The addition of the bytes package import is necessary for the new Equals method implementation.
101-104: LGTM: Secure and efficient implementation
The Equals method implementation is correct and secure:
- Uses constant-time comparison via
bytes.Equal - Properly compares all 20 bytes of the address
- No unnecessary allocations or conversions
Let's verify this method is properly used for EVM inflation address verification:
✅ Verification successful
LGTM: The Equals method is correctly used for EVM inflation address verification
The implementation is properly used in critical withdrawal processing:
- First withdrawal in each block must be the EVM inflation withdrawal
Equalsmethod is used to verify the withdrawal address matches the configured EVM inflation address- Constant-time comparison via
bytes.Equalensures secure address verification
The evidence from the codebase shows:
processWithdrawalsverifies first withdrawal usingEqualsto check againstEVMInflationAddress- Error
ErrFirstWithdrawalNotEVMInflationis returned if verification fails - Tests confirm first withdrawal is always reserved for EVM inflation
- Implementation follows security best practices with constant-time comparison
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Equals method usage in withdrawal processing
# Expected: Should find usage in withdrawal processing code checking the first withdrawal address
# Search for withdrawal processing code using the Equals method
ast-grep --pattern 'if $_.Equals($_) {
$$$
}'
# Backup: Search for direct references to the method
rg -l "Equals.*ExecutionAddress"
Length of output: 123
Script:
#!/bin/bash
# Let's search for any usage of ExecutionAddress type and its comparison
rg -A 5 "ExecutionAddress" --type go
# Also search for any test files that might verify this functionality
fd -e go -E vendor test | xargs rg -l "ExecutionAddress"
Length of output: 111968
Script:
#!/bin/bash
# Let's search for the usage of Equals method in withdrawal processing code
rg -A 5 "withdrawalAddress.*Equals" --type go
# Also search for any code that processes withdrawals and might use this comparison
rg -A 10 "EVMInflationAddress.*withdrawal" --type go
Length of output: 1545
Script:
#!/bin/bash
# Let's look for the withdrawal processing logic in the statedb
rg -B 5 -A 10 "func.*ProcessWithdrawals" --type go
# Also search for any withdrawal validation logic
rg -B 2 -A 5 "withdrawal.*validation|validate.*withdrawal" --type go
Length of output: 123
Script:
#!/bin/bash
# Let's search for withdrawal processing in state transition code
rg -B 5 -A 10 "withdrawal.*process|process.*withdrawal" --type go
# Also search for the first withdrawal check in state transition
rg -B 3 -A 7 "first withdrawal|withdrawal.*fixed" --type go
Length of output: 19412
mod/state-transition/pkg/core/state_processor_genesis_test.go (4)
28-28: LGTM: Import addition aligns with chain spec type usage
The addition of the components package import is necessary for accessing BetnetChainSpecType.
43-65: LGTM: Deposit data structure is well-organized
The deposit data changes:
- Use sequential pubkey values for better readability
- Calculate amounts correctly using chain spec parameters
- Maintain proper sequential indexing
Also applies to: 137-159
86-90: LGTM: Improved error handling in state verification
The changes enhance the robustness of state verification by:
- Adding proper error handling for state queries
- Maintaining consistent verification patterns
- Following Go best practices for error checking
Also applies to: 180-184
37-38: Verify the implementation of setup helper functions
The test now uses setupChain and setupState helper functions, which simplifies the setup. However, these helper functions are not visible in the current file.
Let's verify the implementation of these helper functions:
✅ Verification successful
Let me gather more context about these helper functions to understand their full implementation.
Let me get one final piece of context to see the complete implementation of setupState.
Based on the gathered information, I can now provide the final response:
Helper functions are properly implemented and used consistently
The helper functions are well-defined in core_test.go and used consistently across multiple test files:
setupChain: Creates a chain specification using environment variables and componentssetupState: Sets up the test state with mocked dependencies (execution engine, signer) and returns processor, state, and context
The simplification of test setup is a good improvement as these helpers are:
- Properly typed with generic parameters
- Used consistently across multiple test files in the package
- Include proper test helper marking with
t.Helper() - Handle mock setup in a centralized way
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for setupChain and setupState function implementations
rg -A 10 'func setup(Chain|State)\(' --type go
Length of output: 1537
Script:
#!/bin/bash
# Get the full implementation of setupState function with more context
rg -A 25 'func setupState\(' --type go
# Also check if these helpers are used in other test files
rg 'setupChain|setupState' --type go
Length of output: 2868
Script:
#!/bin/bash
# Get the complete implementation of setupState including the function body
rg -A 50 'func setupState\(' mod/state-transition/pkg/core/core_test.go
Length of output: 1185
mod/state-transition/pkg/core/core_test.go (2)
33-51: LGTM: Import changes are well-organized and necessary.
The new imports align with the PR objectives and support the implementation of EVM inflation withdrawals.
152-162: Verify parallel test safety.
The use of t.Setenv could affect parallel test execution. Consider using a more isolated approach or ensuring tests using this helper are not run in parallel.
✅ Verification successful
Let me verify if the environment variable is used in other test files to ensure there are no potential conflicts.
Environment variable usage in tests is safe
The environment variable setup is safe because:
t.Setenvis used, which provides test-local environment variables that are automatically cleaned up- No parallel tests are running in any of the affected test files
- The environment variable is only used in this specific test setup and the chain spec provider
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for t.Parallel() usage in test files that might use setupChain
# Search for test files using setupChain
rg -l "setupChain" --type go | while read -r file; do
echo "Checking $file for parallel test usage..."
rg "t\.Parallel\(\)" "$file" || echo "No parallel tests found in $file"
done
Length of output: 1424
Script:
#!/bin/bash
# Check for usage of the ChainSpecTypeEnvVar in test files
rg -A 2 "ChainSpecTypeEnvVar" --type go
# Also check the actual variable definition to understand its scope
rg "ChainSpecTypeEnvVar.*=" --type go
Length of output: 885
mod/chain-spec/pkg/chain/data.go (2)
147-151: LGTM! Clear section organization.
The addition of a new "Berachain Values" section follows the existing pattern of grouping related parameters.
152-157: Verify the integration with withdrawal processing.
The new fields are well-documented and align with the PR objectives. However, let's verify a few implementation details:
- Ensure the
EVMInflationAddressis properly validated - Confirm the withdrawal processing respects the minimum requirement of 2 withdrawals per block
Let's verify the implementation details:
✅ Verification successful
Based on the gathered information, I can now provide the final response:
The withdrawal processing implementation is properly integrated with necessary validations.
The codebase shows:
MaxWithdrawalsPerPayloadmust be greater than 1 to accommodate both EVM inflation and validator withdrawals (enforced in chain spec validation)- First withdrawal is explicitly validated to be the EVM inflation withdrawal (
ErrFirstWithdrawalNotEVMInflation) - Maximum withdrawals per payload is set to 16 by default, well above the minimum requirement
- Proper error handling and validation checks are in place throughout the withdrawal processing flow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration of EVM inflation with withdrawal processing
# Check for address validation
rg -A 5 "EVMInflationAddress.*validate"
# Check for MaxWithdrawalsPerBlock validation
rg -A 5 "MaxWithdrawalsPerBlock.*>= 2"
# Check withdrawal processing implementation
ast-grep --pattern 'func $_($$) {
$$$
EVMInflationAddress
$$$
}'
Length of output: 155
Script:
#!/bin/bash
# Let's try a broader search to understand the validation and processing
# Search for any validation related to EVMInflationAddress
rg -A 5 "EVMInflationAddress"
# Look for withdrawal processing logic
rg -A 10 "withdrawal.*process"
# Check for any validation functions in the chain spec
fd "validate.*go" --exec rg -A 5 "func.*validate"
# Look for any minimum withdrawal constraints
rg -A 5 "MinWithdrawals|MinimumWithdrawals"
# Search for withdrawal-related constants
rg "const.*Withdrawal"
Length of output: 8375
Script:
#!/bin/bash
# Let's check for withdrawal constraints and error handling
# Search for withdrawal-related constants and their values
rg -A 2 "MaxWithdrawalsPerPayload"
# Look for withdrawal validation errors
rg "ErrFirstWithdrawalNotEVMInflation|ErrInvalidWithdrawal"
# Check for withdrawal-related tests
fd ".*test.*go" --exec rg -A 5 "TestWithdrawal"
# Search for withdrawal validation in state processor
rg -A 10 "func.*validateWithdrawals"
Length of output: 9191
mod/state-transition/pkg/core/state_processor_staking_test.go (4)
26-30: LGTM: Import changes align with new test requirements
The new imports are necessary for handling chain specifications, types, and engine primitives in the added test functions.
91-94: LGTM: Correctly enforces EVM inflation withdrawal requirement
The test now properly includes the EVM inflation withdrawal as the first withdrawal in the block, which aligns with the PR's objective of implementing fixed EVM inflation per block.
183-193: LGTM: Comprehensive withdrawal test implementation
The test correctly validates:
- EVM inflation withdrawal as the first withdrawal
- Validator partial withdrawal functionality
- Balance updates after withdrawal processing
276-286: LGTM: Thorough validation of withdrawal limits
The test effectively validates:
- EVM inflation withdrawal doesn't prevent validator withdrawals
- Withdrawal processing works correctly across multiple blocks
- State updates are maintained correctly between blocks
Also applies to: 323-333
mod/state-transition/pkg/core/state_processor_staking.go (3)
228-230: Clarification on Function Modification
The added comment clearly notes that the function is modified from the spec to include a fixed withdrawal for EVM inflation. This is helpful for maintainers to understand the deviation from the original specification.
Line range hint 250-255: Ensuring Consistency in Withdrawal Counts
The code correctly adds a check to ensure that the number of expected withdrawals matches the number of payload withdrawals. This enhances consistency and prevents processing errors due to mismatched withdrawal counts.
259-271: Validation of EVM Inflation Withdrawal
The code appropriately validates that the first withdrawal is the EVM inflation withdrawal with the correct address and amount. This ensures protocol compliance and that the EVM inflation mechanism operates as intended.
mod/state-transition/pkg/core/state/statedb.go (3)
304-319: Confirm that using index 0 does not conflict with existing validators
The EVMInflationWithdrawal function sets both the withdrawal index and validator index to 0. Ensure that this does not conflict with any existing validator (typically, validator index 0 would be valid) and does not cause issues in systems that process withdrawals based on these indices.
Run the following script to check for validators with index 0:
#!/bin/bash
# Description: Check if validator index 0 is assigned to an actual validator.
# Expected: Validator index 0 exists; verify no conflicts arise.
# List validators with their indices
ast-grep --pattern 'Validator { index: 0, $$$ }'
# Search for usage of validator index 0 in the codebase
rg 'ValidatorIndex\(0\)'236-240:
Adjust withdrawal cap to account for EVM Inflation Withdrawal
Since the EVM Inflation Withdrawal is always added first, the validator withdrawals should be capped at MaxWithdrawalsPerPayload - 1 to prevent exceeding the maximum. Update the condition in the loop to reflect this adjustment.
Apply this diff to fix the capping logic:
- if uint64(len(withdrawals)) == s.cs.MaxWithdrawalsPerPayload() {
+ if uint64(len(withdrawals)) >= s.cs.MaxWithdrawalsPerPayload() {Likely invalid or redundant comment.
205-207:
Verify inclusion of EVM Inflation Withdrawal respects MaxWithdrawalsPerPayload
By appending the EVM Inflation Withdrawal as the first entry, the total number of withdrawals per payload increases by one. Ensure that this does not cause the total withdrawals to exceed MaxWithdrawalsPerPayload, potentially preventing validator withdrawals from being processed.
Run the following script to verify that the total number of withdrawals, including the EVM Inflation Withdrawal, does not exceed MaxWithdrawalsPerPayload:
✅ Verification successful
Let me search for more specific information about withdrawal handling.
Based on the search results, I can now provide a final response as there is sufficient evidence to verify the concern.
The EVM Inflation Withdrawal is safely handled within withdrawal limits
The codebase has multiple safeguards to ensure withdrawals don't exceed MaxWithdrawalsPerPayload:
- The chain spec validation requires
MaxWithdrawalsPerPayloadto be greater than 1 specifically to accommodate both the EVM inflation withdrawal and at least one validator withdrawal - The
ExpectedWithdrawalsmethod in statedb.go breaks the withdrawal collection whenlen(withdrawals) == MaxWithdrawalsPerPayload - Additional validation in state_processor_payload.go ensures the total withdrawals never exceed the limit
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify total withdrawals do not exceed MaxWithdrawalsPerPayload.
# Expected: Total withdrawals per payload should be less than or equal to MaxWithdrawalsPerPayload.
# Search for the capping condition in ExpectedWithdrawals function
rg 'len\(withdrawals\).*s\.cs\.MaxWithdrawalsPerPayload\(' -A 5
# Verify if the logic accounts for the additional EVM Inflation Withdrawal
Length of output: 196
Script:
#!/bin/bash
# Search for MaxWithdrawalsPerPayload definition and usage
rg "MaxWithdrawalsPerPayload" -A 5
# Search for functions that handle withdrawal limits
ast-grep --pattern 'func $_($_ *State) ExpectedWithdrawals() $_'
# Look for withdrawal length checks
rg "len\(withdrawals\)" -A 3
Length of output: 11503
mod/chain-spec/pkg/chain/chain_spec.go (2)
186-196: New EVM inflation methods added to Spec interface
The methods EVMInflationAddress() and EVMInflationPerBlock() are correctly added to the Spec interface with appropriate documentation.
504-518: Implementations of EVM inflation methods are correct
The methods EVMInflationAddress() and EVMInflationPerBlock() correctly return the corresponding fields from c.Data.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range comments (1)
mod/state-transition/pkg/core/core_test.go (1)
Line range hint
91-109: Consider adding documentation for the TestStateProcessorT type alias.While the type alias effectively reduces verbosity in function signatures, adding documentation would help explain its purpose and usage in tests.
Add a comment block explaining the type's role:
+// TestStateProcessorT is a type alias for the StateProcessor with all required generic parameters +// for testing. This alias helps reduce verbosity in test function signatures while maintaining +// type safety. TestStateProcessorT = core.StateProcessor[
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (6)
mod/chain-spec/pkg/chain/chain_spec.go(3 hunks)mod/chain-spec/pkg/chain/errors.go(1 hunks)mod/state-transition/pkg/core/core_test.go(4 hunks)mod/state-transition/pkg/core/state_processor_staking.go(5 hunks)mod/state-transition/pkg/core/state_processor_staking_test.go(3 hunks)testing/e2e/e2e_startup_test.go(2 hunks)
🧰 Additional context used
📓 Learnings (3)
mod/state-transition/pkg/core/core_test.go (1)
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.
mod/state-transition/pkg/core/state_processor_staking.go (1)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking.go:176-179
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/state-transition/pkg/core/state_processor_staking.go`, when a validator is marked as withdrawable, the withdrawals and evictions occur in the next epoch.
mod/state-transition/pkg/core/state_processor_staking_test.go (4)
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.
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#2119
File: mod/state-transition/pkg/core/state_processor_staking_test.go:416-416
Timestamp: 2024-11-12T11:12:56.774Z
Learning: In `mod/state-transition/pkg/core/state_processor_staking_test.go`, setting up the state via genesis is acceptable, and there is no need to set the state artificially in tests.
🔇 Additional comments (11)
mod/state-transition/pkg/core/state_processor_staking.go (4)
260-270: Verify error handling for EVM inflation withdrawal checks
The code correctly checks if the first withdrawal aligns with the expected EVM inflation withdrawal and returns ErrFirstWithdrawalNotEVMInflation if it does not match. Ensure that upstream functions or calling contexts appropriately handle this error to maintain the integrity of the withdrawal processing logic.
277-279: Avoid logging detailed withdrawal information in error messages
Using spew.Sdump to dump withdrawal objects in error messages can expose sensitive information and clutter logs, potentially leading to security concerns.
315-317: Prevent potential integer overflow by using consistent data types
Casting sp.cs.MaxWithdrawalsPerPayload() from uint64 to int may lead to an integer overflow if the value exceeds the maximum value of int. To ensure type safety and prevent potential issues, compare variables of consistent types.
228-230: 🧹 Nitpick (assertive)
Enhance the function documentation with detailed references
The added comment indicates that the function has been modified from the specification to allow a fixed withdrawal for EVM inflation as the first withdrawal. To improve clarity and maintainability, consider providing a reference to the specific section of the Ethereum consensus specifications or any relevant documentation that this modification aligns with. This will aid future developers in understanding the rationale behind the change.
⛔ Skipped due to learnings
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_staking.go:176-179
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `mod/state-transition/pkg/core/state_processor_staking.go`, when a validator is marked as withdrawable, the withdrawals and evictions occur in the next epoch.
Learnt from: abi87
PR: berachain/beacon-kit#2142
File: mod/state-transition/pkg/core/state_processor_staking.go:105-106
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In this project, detailed TODO comments may not be necessary; concise TODOs are acceptable.
mod/chain-spec/pkg/chain/errors.go (1)
25-32: Verify the usage of this error in validation logic
Since this is a renamed error variable, we should verify its usage across the codebase.
✅ Verification successful
Error variable is correctly used in validation logic and tests
The error variable ErrInsufficientMaxWithdrawalsPerPayload is properly integrated in the codebase:
- Used in
chain_spec.gofor validating thatMaxWithdrawalsPerPayloadis greater than 1 - The validation is critical as the codebase requires at least one withdrawal for EVM inflation and one for validator withdrawal per block
- The constant is consistently used across multiple components:
- Core state processor for withdrawal limits
- Payload validation in state transition
- Test cases properly verify the behavior with value of 2
- Default configuration in testnet spec sets it to 16
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usage of the error variable and related validation logic
# Search for current error usage
echo "Current error usage:"
rg "ErrInsufficientMaxWithdrawalsPerPayload"
# Search for validation logic related to withdrawals per payload
echo -e "\nValidation logic:"
rg -A 5 "MaxWithdrawalsPerPayload"
# Search for related test cases
echo -e "\nTest cases:"
fd -e test.go | xargs rg "MaxWithdrawalsPerPayload"
Length of output: 13014
testing/e2e/e2e_startup_test.go (1)
44-66: Verify EVM inflation withdrawal implementation aligns with PR objectives
The test validates the balance increases but doesn't verify that:
- The withdrawal is positioned at index 0 in the list of withdrawals
- The withdrawal is not processed by the CL StateProcessor
- The withdrawal doesn't affect validator withdrawals
✅ Verification successful
Based on the search results, I can see that the EVM inflation withdrawal implementation is handled in the state processor code. Let me search for more specific details about the withdrawal ordering and processing.
Based on the search results, I can see clear evidence in the state processor code that validates the EVM inflation withdrawal is at index 0 and has the correct properties. Let me verify the state processor skipping part.
Let me try one more search to find the withdrawal processing logic with different patterns.
Based on the search results, I can now provide a definitive response. The code shows clear evidence that:
- The EVM inflation withdrawal is enforced to be at index 0 in
state_processor_staking.go:
if !expectedWithdrawals[0].GetAddress().Equals(
sp.cs.EVMInflationAddress(),
) ||
expectedWithdrawals[0].GetAmount() != math.Gwei(
sp.cs.EVMInflationPerBlock(),
) {
return ErrFirstWithdrawalNotEVMInflation
}- The EVM inflation withdrawal is handled specially in
state/statedb.go:
// NOTE: This function is modified from the spec to allow a fixed withdrawal
// (as the first withdrawal) used for EVM inflation.
//
func (s *StateDB[...]) ExpectedWithdrawals() ([]WithdrawalT, error) {
// The first withdrawal is fixed to be the EVM inflation withdrawal.
withdrawals = append(withdrawals, s.EVMInflationWithdrawal())The test is correctly validating the EVM inflation functionality by checking the balance increases. The review comment's concerns about withdrawal positioning and processing are already handled by the codebase's design.
The EVM inflation withdrawal is properly implemented with the required constraints
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify EVM inflation withdrawal implementation details
# Check if withdrawal is at index 0 and handled by EL
ast-grep --pattern 'func $_(withdrawals []$_) {
$$$
if len(withdrawals) > 0 && withdrawals[0].$_ == $EVMInflationAddress {
$$$
}
$$$
}'
# Check if withdrawal is skipped by CL StateProcessor
ast-grep --pattern 'func $_(withdrawal $_) {
$$$
if withdrawal.$_ == $EVMInflationAddress {
return
}
$$$
}'
Length of output: 271
Script:
#!/bin/bash
# Let's try a broader search for EVM inflation related code
# Search for EVM inflation address handling
rg -A 5 "EVMInflationAddress"
# Search for withdrawal processing logic
rg -A 10 "func.*[Ww]ithdrawal"
# Search for state processor related code
rg -A 10 "StateProcessor|ProcessWithdrawals"
Length of output: 265167
Script:
#!/bin/bash
# Search for EVM inflation withdrawal processing logic
rg -A 10 "ErrFirstWithdrawalNotEVMInflation|expectedWithdrawals\[0\]"
# Search for state processor withdrawal handling
rg -A 10 "processWithdrawals.*BeaconStateT.*BeaconBlockBodyT"
# Search for withdrawal validation logic
rg -A 10 "validateWithdrawals|ValidateWithdrawals"
Length of output: 2589
Script:
#!/bin/bash
# Search for state processor withdrawal handling with more context
rg -B 5 -A 15 "processWithdrawals.*BeaconStateT"
# Search for withdrawal processing in state processor
rg -B 5 -A 15 "expectedWithdrawals, err := st.GetWithdrawals"
Length of output: 114
Script:
#!/bin/bash
# Search for withdrawal processing in state processor with different patterns
rg -B 5 -A 15 "func.*processWithdrawals"
# Search for withdrawal handling in state code
rg -B 5 -A 15 "GetWithdrawals|ExpectedWithdrawals"
# Search for withdrawal validation in state code
rg -B 5 -A 15 "validateWithdrawal|ValidateWithdrawal"
Length of output: 46631
mod/state-transition/pkg/core/state_processor_staking_test.go (2)
91-94: LGTM: Correctly enforces EVM inflation as first withdrawal
The implementation aligns with the PR objective by ensuring the first withdrawal is always reserved for EVM inflation.
131-353: LGTM: Comprehensive test coverage for EVM inflation withdrawals
The new test functions effectively validate:
- EVM inflation withdrawal as the mandatory first withdrawal
- Correct processing of validator withdrawals
- Minimum MaxWithdrawalsPerPayload requirement
- Withdrawal index progression across blocks
The implementation aligns perfectly with the PR objectives for implementing fixed EVM inflation withdrawals.
mod/chain-spec/pkg/chain/chain_spec.go (3)
189-196: LGTM on interface additions
The new methods are well-documented and correctly typed.
221-230: LGTM on constructor changes
The addition of error return and validation call is correct.
504-518: LGTM on implementation methods
The getter implementations are correct and straightforward.
| var ( | ||
| // ErrInsufficientMaxWithdrawalsPerPayload is returned when the max | ||
| // withdrawals per payload less than 2. Must allow at least one for the EVM | ||
| // inflation withdrawal, and at least one more for a validator withdrawal | ||
| // per block. | ||
| ErrInsufficientMaxWithdrawalsPerPayload = errors.New( | ||
| "max withdrawals per payload must be greater than 1") | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error definition to align with ETH 2.0 spec requirements
The error definition should be more explicit about the valid range and breakdown of withdrawals according to the ETH 2.0 specification.
Consider applying this improvement:
var (
+ // MinWithdrawalsPerPayload is the minimum required withdrawals per payload
+ // (1 for EVM inflation + 1 for validator withdrawals)
+ MinWithdrawalsPerPayload = 2
+
+ // MaxWithdrawalsPerPayload is the maximum allowed withdrawals per payload
+ // as defined by the ETH 2.0 specification
+ MaxWithdrawalsPerPayload = 16
+
- // ErrInsufficientMaxWithdrawalsPerPayload is returned when the max
- // withdrawals per payload less than 2. Must allow at least one for the EVM
- // inflation withdrawal, and at least one more for a validator withdrawal
- // per block.
- ErrInsufficientMaxWithdrawalsPerPayload = errors.New(
- "max withdrawals per payload must be greater than 1")
+ // ErrInvalidWithdrawalsPerPayload is returned when the withdrawals per payload
+ // is outside the valid range. According to ETH 2.0 spec, must be between 2-16:
+ // 1 for EVM inflation withdrawal and 1-15 for validator withdrawals per block.
+ ErrInvalidWithdrawalsPerPayload = errors.New(
+ "withdrawals per payload must be between 2 and 16 (1 EVM inflation + 1-15 validator withdrawals)")
)This change:
- Adds constants for the minimum and maximum values
- Renames the error to better reflect its purpose
- Updates the error message to clearly specify the valid range and breakdown
- Aligns with ETH 2.0 specification requirements
📝 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 ( | |
| // ErrInsufficientMaxWithdrawalsPerPayload is returned when the max | |
| // withdrawals per payload less than 2. Must allow at least one for the EVM | |
| // inflation withdrawal, and at least one more for a validator withdrawal | |
| // per block. | |
| ErrInsufficientMaxWithdrawalsPerPayload = errors.New( | |
| "max withdrawals per payload must be greater than 1") | |
| ) | |
| var ( | |
| // MinWithdrawalsPerPayload is the minimum required withdrawals per payload | |
| // (1 for EVM inflation + 1 for validator withdrawals) | |
| MinWithdrawalsPerPayload = 2 | |
| // MaxWithdrawalsPerPayload is the maximum allowed withdrawals per payload | |
| // as defined by the ETH 2.0 specification | |
| MaxWithdrawalsPerPayload = 16 | |
| // ErrInvalidWithdrawalsPerPayload is returned when the withdrawals per payload | |
| // is outside the valid range. According to ETH 2.0 spec, must be between 2-16: | |
| // 1 for EVM inflation withdrawal and 1-15 for validator withdrawals per block. | |
| ErrInvalidWithdrawalsPerPayload = errors.New( | |
| "withdrawals per payload must be between 2 and 16 (1 EVM inflation + 1-15 validator withdrawals)") | |
| ) |
|
|
||
| // Check over the next 10 EVM blocks, that after every block, the balance | ||
| // of the EVM inflation address increases by EVMInflationPerBlockWei. | ||
| for i := int64(0); i <= 10; i++ { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adjust loop range for accurate block count
The loop starts from 0, which means it's checking 11 blocks (0 to 10) instead of the intended 10 blocks. Consider adjusting the range.
-for i := int64(0); i <= 10; i++ {
+for i := int64(1); i <= 10; i++ {📝 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.
| for i := int64(0); i <= 10; i++ { | |
| for i := int64(1); i <= 10; i++ { |
| s.Require().Zero(balance.Cmp(new(big.Int).Mul( | ||
| evmInflationPerBlockWei, big.NewInt(i)), | ||
| )) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Improve balance comparison readability and error reporting
The Cmp() method returns 0 for equal values, but this isn't immediately clear. Consider using a more explicit comparison with a descriptive error message.
-s.Require().Zero(balance.Cmp(new(big.Int).Mul(
- evmInflationPerBlockWei, big.NewInt(i)),
-))
+expected := new(big.Int).Mul(evmInflationPerBlockWei, big.NewInt(i))
+s.Require().Equal(
+ expected, balance,
+ "Balance at block %d should be %s, got %s",
+ i, expected.String(), balance.String(),
+)📝 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.
| s.Require().Zero(balance.Cmp(new(big.Int).Mul( | |
| evmInflationPerBlockWei, big.NewInt(i)), | |
| )) | |
| expected := new(big.Int).Mul(evmInflationPerBlockWei, big.NewInt(i)) | |
| s.Require().Equal( | |
| expected, balance, | |
| "Balance at block %d should be %s, got %s", | |
| i, expected.String(), balance.String(), | |
| ) |
| func setupChain(t *testing.T, chainSpecType string) chain.Spec[ | ||
| bytes.B4, math.U64, common.ExecutionAddress, math.U64, any, | ||
| ] { | ||
| t.Helper() | ||
|
|
||
| t.Setenv(components.ChainSpecTypeEnvVar, chainSpecType) | ||
| cs, err := components.ProvideChainSpec() | ||
| require.NoError(t, err) | ||
|
|
||
| return cs | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance setupChain function with validation and documentation.
Consider these improvements:
- Add validation for the
chainSpecTypeparameter - Add documentation with example usage
+// setupChain creates a chain specification for testing based on the provided chain type.
+// Example usage:
+// cs := setupChain(t, "mainnet")
func setupChain(t *testing.T, chainSpecType string) chain.Spec[
bytes.B4, math.U64, common.ExecutionAddress, math.U64, any,
] {
t.Helper()
+ if chainSpecType == "" {
+ t.Fatal("chainSpecType cannot be empty")
+ }
+
t.Setenv(components.ChainSpecTypeEnvVar, chainSpecType)📝 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 setupChain(t *testing.T, chainSpecType string) chain.Spec[ | |
| bytes.B4, math.U64, common.ExecutionAddress, math.U64, any, | |
| ] { | |
| t.Helper() | |
| t.Setenv(components.ChainSpecTypeEnvVar, chainSpecType) | |
| cs, err := components.ProvideChainSpec() | |
| require.NoError(t, err) | |
| return cs | |
| } | |
| // setupChain creates a chain specification for testing based on the provided chain type. | |
| // Example usage: | |
| // cs := setupChain(t, "mainnet") | |
| func setupChain(t *testing.T, chainSpecType string) chain.Spec[ | |
| bytes.B4, math.U64, common.ExecutionAddress, math.U64, any, | |
| ] { | |
| t.Helper() | |
| if chainSpecType == "" { | |
| t.Fatal("chainSpecType cannot be empty") | |
| } | |
| t.Setenv(components.ChainSpecTypeEnvVar, chainSpecType) | |
| cs, err := components.ProvideChainSpec() | |
| require.NoError(t, err) | |
| return cs | |
| } |
| func setupState( | ||
| t *testing.T, cs chain.Spec[ | ||
| bytes.B4, math.U64, common.ExecutionAddress, math.U64, any, | ||
| ], | ||
| ) ( | ||
| *TestStateProcessorT, | ||
| *TestBeaconStateT, | ||
| *transition.Context, | ||
| ) { | ||
| t.Helper() | ||
|
|
||
| execEngine := mocks.NewExecutionEngine[ | ||
| *types.ExecutionPayload, | ||
| *types.ExecutionPayloadHeader, | ||
| engineprimitives.Withdrawals, | ||
| ](t) | ||
|
|
||
| mocksSigner := &cryptomocks.BLSSigner{} | ||
| mocksSigner.On( | ||
| "VerifySignature", | ||
| mock.Anything, mock.Anything, mock.Anything, | ||
| ).Return(nil) | ||
|
|
||
| dummyProposerAddr := []byte{0xff} | ||
|
|
||
| sp := core.NewStateProcessor[ | ||
| *types.BeaconBlock, | ||
| *types.BeaconBlockBody, | ||
| *types.BeaconBlockHeader, | ||
| *TestBeaconStateT, | ||
| *transition.Context, | ||
| *types.Deposit, | ||
| *types.Eth1Data, | ||
| *types.ExecutionPayload, | ||
| *types.ExecutionPayloadHeader, | ||
| *types.Fork, | ||
| *types.ForkData, | ||
| *TestKVStoreT, | ||
| *types.Validator, | ||
| types.Validators, | ||
| *engineprimitives.Withdrawal, | ||
| engineprimitives.Withdrawals, | ||
| types.WithdrawalCredentials, | ||
| ]( | ||
| noop.NewLogger[any](), | ||
| cs, | ||
| execEngine, | ||
| mocksSigner, | ||
| func(bytes.B48) ([]byte, error) { | ||
| return dummyProposerAddr, nil | ||
| }, | ||
| ) | ||
|
|
||
| kvStore, err := initStore() | ||
| require.NoError(t, err) | ||
| beaconState := new(TestBeaconStateT).NewFromDB(kvStore, cs) | ||
|
|
||
| ctx := &transition.Context{ | ||
| SkipPayloadVerification: true, | ||
| SkipValidateResult: true, | ||
| ProposerAddress: dummyProposerAddr, | ||
| } | ||
|
|
||
| return sp, beaconState, ctx | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance setupState for withdrawal testing scenarios.
Given this PR's focus on EVM inflation withdrawals, consider enhancing the setup function to better support withdrawal-related test cases.
Suggested improvements:
func setupState(
t *testing.T, cs chain.Spec[
bytes.B4, math.U64, common.ExecutionAddress, math.U64, any,
],
) (
*TestStateProcessorT,
*TestBeaconStateT,
*transition.Context,
) {
t.Helper()
execEngine := mocks.NewExecutionEngine[
*types.ExecutionPayload,
*types.ExecutionPayloadHeader,
engineprimitives.Withdrawals,
](t)
+ // Setup expectations for EVM inflation withdrawals
+ execEngine.On(
+ "ProcessWithdrawals",
+ mock.Anything,
+ mock.MatchedBy(func(w engineprimitives.Withdrawals) bool {
+ // Verify first withdrawal is EVM inflation
+ return len(w) > 0 && w[0].Address == cs.EVMInflationAddress()
+ }),
+ ).Return(nil)
mocksSigner := &cryptomocks.BLSSigner{}
mocksSigner.On(
"VerifySignature",
mock.Anything, mock.Anything, mock.Anything,
).Return(nil)
- dummyProposerAddr := []byte{0xff}
+ // Use a meaningful address for testing
+ proposerAddr := []byte("proposer_address_for_withdrawal_tests")
sp := core.NewStateProcessor[
// ... type parameters ...
](
noop.NewLogger[any](),
cs,
execEngine,
mocksSigner,
func(bytes.B48) ([]byte, error) {
- return dummyProposerAddr, nil
+ return proposerAddr, nil
},
)
kvStore, err := initStore()
require.NoError(t, err)
beaconState := new(TestBeaconStateT).NewFromDB(kvStore, cs)
ctx := &transition.Context{
SkipPayloadVerification: true,
SkipValidateResult: true,
- ProposerAddress: dummyProposerAddr,
+ ProposerAddress: proposerAddr,
+ // Add withdrawal-specific context flags if needed
+ WithdrawalsEnabled: true,
}
return sp, beaconState, ctx
}Committable suggestion skipped: line range outside the PR's diff.
| // Run the test. | ||
| vals, err := sp.Transition(ctx, st, blk) | ||
|
|
||
| // Check outputs and ensure withdrawals in payload is consistent with | ||
| // statedb expected withdrawals. | ||
| require.NoError(t, err) | ||
| require.Zero(t, vals) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding assertions for EVM inflation withdrawal
While the test verifies validator withdrawals correctly, it would be beneficial to add assertions verifying that the EVM inflation withdrawal was processed correctly, ensuring the inflation address received the expected amount.
| // validate ensures that the chain spec is valid, returning error if it is not. | ||
| func (c *chainSpec[ | ||
| DomainTypeT, EpochT, ExecutionAddressT, SlotT, CometBFTConfigT, | ||
| ]) validate() error { | ||
| if c.MaxWithdrawalsPerPayload() <= 1 { | ||
| return ErrInsufficientMaxWithdrawalsPerPayload | ||
| } | ||
|
|
||
| // TODO: Add more validation rules here. | ||
| return nil |
There was a problem hiding this comment.
Add validation for EVM inflation parameters
The validation method should ensure that EVM inflation parameters are properly set since they are critical for the block-by-block inflation mechanism.
Add these validations:
func (c *chainSpec[
DomainTypeT, EpochT, ExecutionAddressT, SlotT, CometBFTConfigT,
]) validate() error {
if c.MaxWithdrawalsPerPayload() <= 1 {
return ErrInsufficientMaxWithdrawalsPerPayload
}
+
+ // Validate EVM inflation parameters
+ if c.EVMInflationPerBlock() == 0 {
+ return fmt.Errorf("EVMInflationPerBlock must be greater than 0")
+ }
+
+ var zeroAddr ExecutionAddressT
+ if c.EVMInflationAddress() == zeroAddr {
+ return fmt.Errorf("EVMInflationAddress must not be zero")
+ }
// TODO: Add more validation rules here.
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.
| // validate ensures that the chain spec is valid, returning error if it is not. | |
| func (c *chainSpec[ | |
| DomainTypeT, EpochT, ExecutionAddressT, SlotT, CometBFTConfigT, | |
| ]) validate() error { | |
| if c.MaxWithdrawalsPerPayload() <= 1 { | |
| return ErrInsufficientMaxWithdrawalsPerPayload | |
| } | |
| // TODO: Add more validation rules here. | |
| return nil | |
| // validate ensures that the chain spec is valid, returning error if it is not. | |
| func (c *chainSpec[ | |
| DomainTypeT, EpochT, ExecutionAddressT, SlotT, CometBFTConfigT, | |
| ]) validate() error { | |
| if c.MaxWithdrawalsPerPayload() <= 1 { | |
| return ErrInsufficientMaxWithdrawalsPerPayload | |
| } | |
| // Validate EVM inflation parameters | |
| if c.EVMInflationPerBlock() == 0 { | |
| return fmt.Errorf("EVMInflationPerBlock must be greater than 0") | |
| } | |
| var zeroAddr ExecutionAddressT | |
| if c.EVMInflationAddress() == zeroAddr { | |
| return fmt.Errorf("EVMInflationAddress must not be zero") | |
| } | |
| // TODO: Add more validation rules here. | |
| return nil |
| if c.MaxWithdrawalsPerPayload() <= 1 { | ||
| return ErrInsufficientMaxWithdrawalsPerPayload | ||
| } |
| } | ||
|
|
||
| // TODO: Add more validation rules here. | ||
| return nil |
There was a problem hiding this comment.
rabbit is right, should we at least validate inflation parameters?
- Has
ExecutionAddressTits own validation - Can
EVMInflationPerBlockever be zero?
| EVMInflationAddress: common.NewExecutionAddressFromHex( | ||
| "0x6942069420694206942069420694206942069420", | ||
| ), | ||
| EVMInflationPerBlock: 10e9, |
There was a problem hiding this comment.
should we add a note about how these values are set?
| @@ -226,6 +233,11 @@ func (s *StateDB[ | |||
|
|
|||
| // Iterate through indices to find the next validators to withdraw. | |||
| for range bound { | |||
| // Cap the number of withdrawals to the maximum allowed per payload. | |||
There was a problem hiding this comment.
nit: we added already a withdrawals, so shouldn't the bound be smaller of 1 element?
Possibly ineffective, just raising this up
| @@ -226,6 +233,11 @@ func (s *StateDB[ | |||
|
|
|||
| // Iterate through indices to find the next validators to withdraw. | |||
| for range bound { | |||
| // Cap the number of withdrawals to the maximum allowed per payload. | |||
| if uint64(len(withdrawals)) == s.cs.MaxWithdrawalsPerPayload() { | |||
There was a problem hiding this comment.
now we need this first since MaxWithdrawalsPerPayload may be 1? If this value is at least 2 instead (can't recall if we validate this), we don't need to move it up I think
| kvStore, err := initStore() | ||
| require.NoError(t, err) | ||
| beaconState := new(TestBeaconStateT).NewFromDB(kvStore, cs) | ||
| cs := setupChain(t, "testnet") |
There was a problem hiding this comment.
nit: shouldn't we have a variable for "testnet", like components.BetnetChainSpecType above?
| func dummyProposerAddressVerifier(bytes.B48) ([]byte, error) { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
genesis should not really have a proposer but I guess we don't really care here, so I am in favour of the simplification
| if numWithdrawals == 0 { | ||
| return ErrNoWithdrawals | ||
| } |
There was a problem hiding this comment.
is this check needed? We build ExpectedWithdrawals with at least 1 withdrawal and we check the length above.
Consider dropping it
| if !expectedWithdrawals[0].GetAddress().Equals( | ||
| sp.cs.EVMInflationAddress(), | ||
| ) || | ||
| expectedWithdrawals[0].GetAmount() != math.Gwei( | ||
| sp.cs.EVMInflationPerBlock(), | ||
| ) { | ||
| return ErrFirstWithdrawalNotEVMInflation | ||
| } |
There was a problem hiding this comment.
nit: why don't we just use equals here? Looks like we build expected payload setting validator index to zero, but we do let withdrawals in block to set the validator differently. Granted we don't really care, I'd rather Expected and actual withdrawal to fully match.
| ErrNumWithdrawalsMismatch, | ||
| "withdrawals do not match expected %s, got %s", | ||
| spew.Sdump(wd), spew.Sdump(payloadWithdrawals[i]), | ||
| ErrWithdrawalMismatch, |
There was a problem hiding this comment.
is the range above correct? I would expect the indexes to be shifted all by one. Something like:
for i:=1; i <expectedWithdrawals; i++
In this way we also simplify the loop, since we don't need to test for i==0
Summary
MaxWithdrawalsPerBlockis at least 2 (to allow 1 for the inflation withdrawal and 1 for validator withdrawal)Side Effect:
MaxWithdrawalsPerPayload is 16 according the ETH 2.0 spec. Choosing to enforce the first withdrawal as EVM inflation withdrawal implies only 15 validator withdrawals per block are allowed at most.
Tests
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation