fix(state-transitions): verify deposits against contract #2115
Conversation
Signed-off-by: nidhi-singh02 <[email protected]>
This reverts commit 099716d.
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
WalkthroughThe pull request introduces several modifications primarily focused on enhancing deposit handling within the codebase. Key changes include updates to function signatures for deposit-related methods, the addition of new validation methods for deposits, and improvements in logging and error handling. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant StateProcessor
participant DepositStore
participant Logger
User->>StateProcessor: Request to process deposits
StateProcessor->>Logger: Log processing start
StateProcessor->>DepositStore: Retrieve deposits
DepositStore-->>StateProcessor: Return deposits
StateProcessor->>StateProcessor: Validate deposits
StateProcessor->>Logger: Log validation results
StateProcessor-->>User: Return processing results
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
abi87
force-pushed
the
verify-deposits-against-contract
branch
from
d28ec4c to
fb9d020
Compare
abi87
force-pushed
the
verify-deposits-against-contract
branch
from
fb9d020 to
6a191d1
Compare
abi87
force-pushed
the
verify-deposits-against-contract
branch
from
9c43988 to
64d19e5
Compare
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Cal Bera <[email protected]>
| return nil | ||
|
|
||
| default: | ||
| if _, err := st.GetEth1DepositIndex(); err == nil { |
There was a problem hiding this comment.
what error is exactly returned here? Should we explicity check that the error contains "not found" ?
There was a problem hiding this comment.
Can't do now without importing cosmos sdk module here, which we really don't want. I wrote a comment about this but maybe got lost in updates. Let me find it
There was a problem hiding this comment.
For the very first deposit, where Eth1DepositIndex has not been set yet, we have a ErrNotFound kind of error. This is fine (we haven't even set a single deposit) but it should only happen for the very first deposit (so the check on deposit index)
On the other hand, I agree that we should check the kind of error we receive, and we should behave like that only for ErrNotFound and not others but:
- ErrNotFound is a cosmosSDK error and we don't want that dependency here (discussed with Dev at some point)
- We do this in another place (see
applyDepositand how we do create validators - I have an old PR that start fixing it (chore(storage): cleanup
NotFounderrors #2065) and I am willing to refresh that and resubmit for review as soon as this in in
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Cal Bera <[email protected]>
| if start > end { | ||
| return pruner.ErrInvalidRange | ||
| return fmt.Errorf( |
There was a problem hiding this comment.
nit: generally all use of fmt.Errorf can be nicely replaced by errors.Wrapf
|
|
||
| for i, sd := range localDeposits { | ||
| // Deposit indices should be contiguous. | ||
| if sd.GetIndex().Unwrap() != expectedStartIdx+uint64(i) { |
There was a problem hiding this comment.
order should always be maintained. From spec:
def process_deposit(state: BeaconState, deposit: Deposit) -> None:
...
# Deposits must be processed in order
state.eth1_deposit_index += 1
apply_deposit(
state=state,
pubkey=deposit.data.pubkey,
withdrawal_credentials=deposit.data.withdrawal_credentials,
amount=deposit.data.amount,
signature=deposit.data.signature,
)There was a problem hiding this comment.
These checks may be redundant, since these data come from our EVM state and I think they are guaranteed to be in-order. Keeping this for the time being, we will revisit all once merkle root checks are in
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range comments (1)
mod/node-api/backend/mocks/deposit_store.mock.go (1)
Line range hint
18-40: Enhance error handling in EnqueueDeposits mockGiven the PR's focus on preventing fraudulent validators, consider enhancing the mock to support testing of validation failure scenarios.
Consider adding specific error types or constants that can be returned by the mock to simulate various validation failure scenarios:
// EnqueueDeposits provides a mock function with given fields: deposits func (_m *DepositStore[DepositT]) EnqueueDeposits(deposits []DepositT) error { + // TODO: Add common error types for validation failures + // var ( + // ErrInvalidDeposit = errors.New("invalid deposit") + // ErrDuplicateDeposit = errors.New("duplicate deposit") + // ) ret := _m.Called(deposits)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (26)
mod/engine-primitives/pkg/engine-primitives/mocks/blobs_bundle.mock.go(1 hunks)mod/engine-primitives/pkg/engine-primitives/mocks/built_execution_payload_env.mock.go(1 hunks)mod/engine-primitives/pkg/engine-primitives/mocks/payload_attributer.mock.go(1 hunks)mod/node-api/backend/mocks/availability_store.mock.go(1 hunks)mod/node-api/backend/mocks/beacon_block_header.mock.go(1 hunks)mod/node-api/backend/mocks/beacon_state.mock.go(1 hunks)mod/node-api/backend/mocks/block_store.mock.go(1 hunks)mod/node-api/backend/mocks/deposit_store.mock.go(1 hunks)mod/node-api/backend/mocks/node.mock.go(1 hunks)mod/node-api/backend/mocks/state_processor.mock.go(1 hunks)mod/node-api/backend/mocks/storage_backend.mock.go(1 hunks)mod/node-api/backend/mocks/validator.mock.go(1 hunks)mod/node-api/backend/mocks/withdrawal.mock.go(1 hunks)mod/node-api/backend/mocks/withdrawal_credentials.mock.go(1 hunks)mod/node-core/pkg/services/registry/mocks/basic.mock.go(1 hunks)mod/node-core/pkg/services/registry/mocks/dispatcher.mock.go(1 hunks)mod/node-core/pkg/services/registry/mocks/registry_option.mock.go(1 hunks)mod/primitives/pkg/crypto/mocks/bls_signer.mock.go(1 hunks)mod/state-transition/pkg/core/deposits_validation.go(1 hunks)mod/state-transition/pkg/core/errors.go(1 hunks)mod/state-transition/pkg/core/mocks/execution_engine.mock.go(1 hunks)mod/storage/pkg/interfaces/mocks/db.mock.go(1 hunks)mod/storage/pkg/pruner/mocks/beacon_block.mock.go(1 hunks)mod/storage/pkg/pruner/mocks/block_event.mock.go(1 hunks)mod/storage/pkg/pruner/mocks/prunable.mock.go(1 hunks)mod/storage/pkg/pruner/mocks/pruner.mock.go(1 hunks)
🧰 Additional context used
📓 Learnings (1)
mod/state-transition/pkg/core/deposits_validation.go (2)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_genesis.go:109-118
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `InitializePreminedBeaconStateFromEth1` in `mod/state-transition/pkg/core/state_processor_genesis.go`, we enforce that there is a single deposit per validator in the genesis format; therefore, handling multiple deposits per validator is not necessary.
Learnt from: abi87
PR: berachain/beacon-kit#2115
File: mod/state-transition/pkg/core/deposits_validation.go:61-61
Timestamp: 2024-11-26T23:37:11.522Z
Learning: In `mod/state-transition/pkg/core/deposits_validation.go`, it's acceptable to use `math.U64(i)` to convert the loop index `i` from `int` to `uint64` when validating deposits.
🪛 golangci-lint (1.62.2)
mod/state-transition/pkg/core/deposits_validation.go
[high] 68-68: G115: integer overflow conversion int -> uint64
(gosec)
[high] 139-139: G115: integer overflow conversion int -> uint64
(gosec)
[high] 143-143: G115: integer overflow conversion int -> uint64
(gosec)
🔇 Additional comments (57)
mod/storage/pkg/pruner/mocks/beacon_block.mock.go (2)
1-1: Verify mockery version upgrade impact.
The mockery version bump from v2.49.0 to v2.49.1 appears safe, but let's verify the changelog for any breaking changes.
✅ Verification successful
Mockery v2.49.1 upgrade is safe to proceed
The changelog for v2.49.1 shows only minor changes:
- Bug fix for type aliases to structs
- Documentation improvements
- Addition of deprecation warning for
with-expecter - No breaking changes that would affect mock functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check mockery's changelog for version 2.49.1
# Expected: No breaking changes in patch version
gh api repos/vektra/mockery/releases/tags/v2.49.1 --jq '.body' || \
echo "Failed to fetch release notes. Please verify manually at https://github.com/vektra/mockery/releases/tag/v2.49.1"
Length of output: 781
Line range hint 1-70: Verify BeaconBlock mock usage in pruner tests.
Given the PR objectives mention deposit pruner issues, let's verify how this mock is being used in the pruner tests to ensure it adequately supports the test scenarios.
✅ Verification successful
The search results show that the BeaconBlock mock is being used appropriately in mod/storage/pkg/pruner/pruner_test.go. Let me verify the test implementation to ensure it adequately tests the pruning functionality.
Let me verify the complete test implementation and mock usage.
BeaconBlock mock implementation is correctly used in pruner tests
The mock implementation and its usage in tests is appropriate and well-structured:
- The mock correctly implements the required
GetSlotmethod - Tests cover various pruning scenarios (single index, multiple indexes, no indexes)
- The mock is properly verified with assertions on both number of calls and correct indices
- The implementation aligns with the pruner's interface requirements and usage patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usage of BeaconBlock mock in pruner tests
# Expected: Test files showing how the mock is used for pruning logic
# Search for test files using this mock
rg -l "mocks\\.BeaconBlock" --type go --glob "*_test.go"
# Search for GetSlot usage in pruner tests
rg "GetSlot" --type go --glob "*_test.go" -C 5
Length of output: 11036
Script:
#!/bin/bash
# Get the full pruner test implementation to verify the test coverage
rg -A 50 "^func TestPruner" mod/storage/pkg/pruner/pruner_test.go
Length of output: 1392
Script:
#!/bin/bash
# Get the complete test file content to see the full test implementation
cat mod/storage/pkg/pruner/pruner_test.go
# Also check the actual pruner implementation to verify the mock matches the interface requirements
cat mod/storage/pkg/pruner/pruner.go
Length of output: 6859
mod/node-core/pkg/services/registry/mocks/dispatcher.mock.go (2)
1-1: LGTM: Mockery version update
The update from mockery v2.49.0 to v2.49.1 is a minor version change that maintains backward compatibility.
Line range hint 1-71: Verify mockery version consistency across the codebase
Let's ensure all mock files are using the same mockery version to maintain consistency.
✅ Verification successful
All mock files consistently use mockery v2.49.1
The verification shows that all 24 mock files in the codebase are using the same mockery version (v2.49.1), indicating proper version consistency across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check mockery version across all mock files
# Expected: All files should use v2.49.1
# Search for mockery version declarations in mock files
echo "Checking mockery versions in mock files:"
rg -l "mockery v\d+\.\d+\.\d+" --type go | while read -r file; do
echo -n "$file: "
head -n 1 "$file" | grep -o "mockery v[0-9.]*"
done
# Count unique versions
echo -e "\nUnique versions found:"
rg "mockery v\d+\.\d+\.\d+" --type go | grep -o "mockery v[0-9.]*" | sort | uniq -c
Length of output: 6350
mod/storage/pkg/pruner/mocks/prunable.mock.go (2)
Line range hint 1-70: Verify mock implementation compatibility
The mock implementation looks correct and includes proper error handling, type-safe expectations, and cleanup. However, we should verify there are no breaking changes in mockery v2.49.1 that could affect the tests.
✅ Verification successful
Mock implementation verified and consistent
The verification shows:
- All mock files are generated using the same mockery version (v2.49.1)
- The mock implementation correctly matches the original Prunable interface from
mod/storage/pkg/pruner/types.go - The mock includes all required methods with correct signatures (Prune with uint64 start/end parameters)
- Error handling and type-safe expectations are properly implemented
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify mock implementation compatibility
# Find all mockery-generated files to check version consistency
rg -l "Code generated by mockery" --type go
# Look for any test failures that might be related to mocks
rg -i "mock.*fail|fail.*mock" --type go --type txt
Length of output: 1404
Script:
#!/bin/bash
# Let's verify the mock implementation compatibility by:
# 1. Checking if all mock files use the same mockery version
# 2. Looking for the interface definition that this mock implements
# Check mockery versions across all mock files
rg "mockery v\d+\.\d+\.\d+" --type go
# Find the original Prunable interface definition
rg "type\s+Prunable\s+interface" --type go -A 5
Length of output: 2961
1-1: Verify mockery version update impact on pruner issues
Given that the PR objectives mention deposit pruner issues, let's verify if this mockery version update addresses any known issues or introduces relevant changes.
✅ Verification successful
Mockery update v2.49.1 is safe to proceed
The mockery v2.49.1 update is a minor patch that primarily includes documentation improvements and bug fixes for type aliases to structs. None of the changes affect the Prunable interface implementation or its mock generation. The mock is used in pruner and manager tests, and the interface remains stable across the codebase.
- The Prunable interface in
mod/storage/pkg/pruner/types.gois simple and well-defined - The mock is used in test files:
pruner_test.goandmanager_test.go - The mockery update doesn't introduce any breaking changes that would affect the mock's functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for relevant changes in mockery v2.49.1 and verify mock usage
# Check mockery changelog for relevant changes
gh api repos/vektra/mockery/releases/tags/v2.49.1 --jq '.body'
# Find test files using this mock to assess impact
rg -l "NewPrunable|mock.Prunable" --type go
# Look for any pruner-related test failures or issues
rg -i "pruner.*fail|fail.*pruner" --type go
Length of output: 919
Script:
#!/bin/bash
# Let's check for any test files that use this mock and verify the interface implementation
rg -A 5 "type Prunable interface" --type go
# Check for any recent changes or issues related to the pruner implementation
rg -l "Prune\(" --type go
# Look for test files specifically
fd ".*_test.go$" | xargs rg -l "Prunable"
Length of output: 937
mod/node-core/pkg/services/registry/mocks/registry_option.mock.go (1)
1-1: LGTM! Mockery version bump looks good.
This is an auto-generated file, and the update from v2.49.0 to v2.49.1 is a minor version change that doesn't affect the mock's functionality.
Let's verify the mockery version is consistently updated across the project:
✅ Verification successful
Mockery version v2.49.1 is consistently used across all mock files
All generated mock files in the codebase are using mockery v2.49.1, confirming that the version bump has been applied consistently. While we couldn't find the mockery configuration file, the uniform version across all 24 mock files indicates proper version management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check mockery version across all mock files and config
# Expect: All files should use v2.49.1
# Check mockery version in all mock files
echo "Checking mockery version in mock files:"
rg -l "mockery v2\." | xargs head -n 1
# Check mockery version in config files
echo -e "\nChecking mockery version in config:"
fd -g ".mockery.yaml" -X cat
Length of output: 3198
mod/node-api/backend/mocks/node.mock.go (3)
Line range hint 1-70: LGTM! Remember this is generated code.
The mock implementation looks correct and complete. As this is generated code:
- Don't modify it manually
- If changes are needed, update the source interface and regenerate the mock
- Ensure all team members use the same mockery version (v2.49.1) for consistent generation
Line range hint 1-70: Verify mock coverage for deposit validation.
Given the PR's focus on deposit validation, we should ensure this mock provides sufficient coverage for testing the new functionality.
✅ Verification successful
Let me analyze the mock coverage for deposit validation based on the search results.
The mock provides sufficient coverage for deposit validation functionality
The mock implementation in node.mock.go provides complete coverage for deposit validation through:
- The generic
Node[ContextT]mock struct that matches the interface requirements - The
CreateQueryContextmethod which is essential for creating the context needed for deposit validation - Comprehensive mock expectations handling through:
Node_Expecterstruct for setting up expectationsNode_CreateQueryContext_Callfor type-safe call verification- Full parameter matching and return value handling
The mock implementation follows best practices by:
- Using mockery's auto-generation capabilities
- Providing type-safe generics support
- Including proper error handling and panic conditions
- Supporting both simple returns and complex callback scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if the Node interface has methods related to deposit validation
# Search for Node interface definition
ast-grep --pattern 'type Node interface {
$$$
}'
# Search for deposit-related methods that might need mocking
rg -i "deposit|validator" --type go
Length of output: 109407
1-1: Verify mockery version upgrade impact.
The file was regenerated using mockery v2.49.1 (upgraded from v2.49.0). While this is a minor version bump, it's important to verify that the upgrade doesn't introduce any breaking changes.
✅ Verification successful
Mockery version upgrade is safe to proceed.
The version bump from v2.49.0 to v2.49.1 is a patch version update, which by semantic versioning standards only includes backwards-compatible bug fixes. Additionally, the repository is currently using an older version (v2.43.2) as seen in go.mod, so both v2.49.0 and v2.49.1 are newer versions that maintain compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes between mockery versions
# Check if there are any other mock files that need to be regenerated
fd -e go -e mock.go$ | while read -r file; do
grep -l "mockery v2.49.0" "$file" || true
done
# Look for mockery version in go.mod
rg "github.com/vektra/mockery" || true
Length of output: 56215
mod/node-api/backend/mocks/withdrawal.mock.go (3)
Line range hint 13-89: Implementation looks good!
The generated mock implementation follows best practices:
- Proper handling of generic type parameter T
- Correct argument type assertions in mock calls
- Appropriate panic handling for missing return values
- Clean test cleanup implementation
1-1:
Regenerate mock to implement missing Equals method
The mock needs to be regenerated to implement the Equals(T) bool method that was added to the Withdrawal interface in mod/node-core/pkg/components/interfaces.go.
Let's verify the interface definition:
1-1: Verify mockery version compatibility
The mock was generated with mockery v2.49.1. While this is a minor version upgrade, we should verify that it's compatible with the project's build system and other dependencies.
✅ Verification successful
Mock files are consistently using the same mockery version
All mock files in the codebase are generated using mockery v2.49.1, showing consistent version usage across the project. This indicates that the mock generation is standardized and there are no version conflicts or compatibility issues to be concerned about.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check mockery version constraints in build files
# Expected: Find mockery version specifications in go.mod or similar files
# Search for mockery in go.mod
rg -A 3 'mockery' go.mod
# Search for mockery in other potential build files
fd -e yaml -e yml -e toml | xargs rg -l 'mockery'
Length of output: 121
Script:
#!/bin/bash
# Let's try to find any mockery-related configuration files or references
# Search for mockery config files
fd -t f "mockery|.mockery"
# Search for any files mentioning mockery version
rg -i "mockery.*version|version.*mockery"
# Look for go files that might have mockery version info in comments
rg -t go "//.*mockery.*v[0-9]"
# Check if there are other mock files to compare versions
fd -e go -e yaml | grep -i "mock" | xargs rg -l "mockery.*v[0-9]"
Length of output: 4007
mod/node-api/backend/mocks/withdrawal_credentials.mock.go (2)
1-1: Auto-generated mock implementation looks good.
This is an auto-generated mock file that follows best practices for mocking and includes proper error handling, type assertions, and helper methods for testing.
1-1: Verify mockery version upgrade impact.
The mockery version has been bumped from v2.49.0 to v2.49.1. While this is a minor version change, we should verify that all mock implementations are consistent across the codebase.
✅ Verification successful
All mock files are consistent with mockery v2.49.1
The verification shows that all mock files across the codebase are generated using mockery v2.49.1 and contain the proper "DO NOT EDIT" header. The version upgrade from v2.49.0 to v2.49.1 has been consistently applied across all mock implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any inconsistencies in mockery versions across mock files
# and verify that all mocks are properly generated with the same version
# Check for any mock files with different mockery versions
echo "Checking for inconsistent mockery versions:"
rg -l "Code generated by mockery" | while read -r file; do
echo -n "$file: "
head -n 1 "$file" | grep -o "mockery v[0-9.]*"
done
# Verify all mock files are properly generated
echo -e "\nVerifying mock file headers:"
rg -l "Code generated by mockery" | while read -r file; do
if ! head -n 1 "$file" | grep -q "DO NOT EDIT"; then
echo "Warning: Missing 'DO NOT EDIT' in $file"
fi
done
Length of output: 8831
mod/node-core/pkg/services/registry/mocks/basic.mock.go (2)
Line range hint 2-109: Auto-generated mock file - no manual changes needed
This is an auto-generated mock file. The implementation looks correct and includes all necessary mock functionality for the Basic interface. No manual modifications are required.
1-1: Verify mockery version upgrade impact
The mockery version has been upgraded from v2.49.0 to v2.49.1. While this is a patch version upgrade and should be backward compatible, it's good practice to verify that all tests using these mocks still pass successfully.
✅ Verification successful
Mockery version upgrade is safe to proceed
The mock is actively used in the registry tests (registry_test.go) for verifying service registration and startup functionality. The change from v2.49.0 to v2.49.1 is a patch version update, and the generated mock maintains the same interface implementation and functionality. The tests are well-structured and use the mock appropriately for both service registration and startup verification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any breaking changes in mockery v2.49.1
# and verify mock usage in tests
# Check for mockery usage in test files
rg -l "NewBasic|Basic_" --type go --type-add 'go:*.go' | grep "_test.go"
Length of output: 72
Script:
#!/bin/bash
# Let's first check if the Basic interface mock is actually used anywhere in the codebase
rg -l "Basic|NewBasic" --type go
# Also check the git diff to understand the exact changes in this mock file
git diff HEAD~1 mod/node-core/pkg/services/registry/mocks/basic.mock.go
# Let's also check the interface this mock is implementing
fd "basic.go" --type f --exec cat {}
Length of output: 3089
Script:
#!/bin/bash
# Let's check the actual Basic interface definition in the registry package
cat mod/node-core/pkg/services/registry/registry.go
# And check if there are any test files using this mock
rg -l "mocks.Basic" --type go
Length of output: 3929
Script:
#!/bin/bash
# Let's check the test file that uses this mock
cat mod/node-core/pkg/services/registry/registry_test.go
# Also check the mockery changelog for v2.49.1 to understand the changes
curl -s https://raw.githubusercontent.com/vektra/mockery/v2.49.1/CHANGELOG.md || echo "Failed to fetch changelog"
Length of output: 2886
mod/storage/pkg/pruner/mocks/pruner.mock.go (2)
1-1: LGTM on the mockery version bump.
The update from v2.49.0 to v2.49.1 is a patch version change, which typically includes bug fixes and is backward compatible.
Line range hint 1-92: Verify if this mock implementation is related to the pruner issues.
Given that the PR objectives mention deposit pruner issues causing errors, let's verify if this mock implementation is being used in the failing test scenarios.
✅ Verification successful
Let me gather more information about the pruner implementation and its usage to better understand the context.
Let me check the actual pruner implementation and test files to understand if there are any issues.
Based on the comprehensive analysis of the pruner implementation, tests, and its usage in deposit and availability stores, I can now provide the final response:
The mock implementation is correctly aligned with the pruner interface and not related to any issues.
The mock implementation in pruner.mock.go is an auto-generated file that correctly implements the Pruner interface defined in types.go. The implementation is being used properly in tests, and the pruner functionality is working as expected:
- The pruner interface is simple and well-defined with just two methods:
Name()andStart(ctx). - The test implementation shows successful pruning scenarios with proper error handling.
- Both deposit and availability stores use the pruner correctly through dependency injection.
- The TODO comment found in availability store is about a potential enhancement to the provider pattern, not an issue with the pruner itself.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usage of this mock in test files and any related errors
# Find test files using this mock
echo "Test files using Pruner mock:"
rg -l "mocks\\.Pruner" --type go --type-add "go:*.go" | grep "_test.go"
# Look for test failures related to pruner
echo -e "\nTest failures mentioning pruner:"
rg -i "fail.*pruner|error.*pruner" --type go --type-add "go:*.go" | grep "_test.go"
# Check if there are any TODO/FIXME comments related to pruner
echo -e "\nTODO/FIXME comments related to pruner:"
rg "TODO|FIXME|XXX|HACK|BUG" --type go --type-add "go:*.go" | grep -i "pruner"
Length of output: 599
Script:
#!/bin/bash
# Search for the actual pruner implementation and its usage
echo "Pruner interface and implementation:"
rg -A 5 "type Pruner" --type go
echo -e "\nPruner usage in tests:"
rg -A 5 "func Test.*Pruner" --type go
echo -e "\nPruner related errors or error handling:"
rg "Error.*[Pp]runer|[Pp]runer.*[Ee]rror" --type go
echo -e "\nFiles containing pruner package imports:"
rg -l "\".*pruner\"" --type go
Length of output: 3307
Script:
#!/bin/bash
# Check the pruner implementation and test files
echo "Pruner test implementation:"
cat mod/storage/pkg/pruner/pruner_test.go
echo -e "\nDeposit store pruner implementation:"
cat mod/node-core/pkg/components/deposit_store.go
echo -e "\nAvailability store pruner implementation:"
cat mod/node-core/pkg/components/availability_store.go
Length of output: 12063
mod/node-api/backend/mocks/state_processor.mock.go (2)
Line range hint 1-89: Mock implementation aligns with PR objectives
The StateProcessor mock correctly provides the structure needed for testing the new deposit verification functionality:
- It maintains type safety with the generic
BeaconStateT - The
ProcessSlotsmethod returnsValidatorUpdateswhich will be crucial for testing deposit-related state transitions - The mock's error handling capabilities will help test various deposit verification scenarios
✅ Verification successful
Mock implementation correctly supports deposit verification testing
The mock implementation is verified to be properly aligned with the codebase's needs:
- The StateProcessor interface and its implementation handle deposits as shown in
state_processor_staking_test.go - The mock correctly supports the generic type parameters including
DepositTand returnsValidatorUpdates - The mock is used in staking-related tests where deposits are processed and verified
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the mock is being used in deposit verification tests
rg -A 5 "NewStateProcessor.*BeaconState"
# Check for any deposit verification test files
fd -t f "deposit.*test\.go$" | xargs rg "StateProcessor"
Length of output: 587
Script:
#!/bin/bash
# Let's check for any test files that might be using this mock
rg -l "StateProcessor" $(fd -t f ".*_test\.go$")
# Also check for the actual interface implementation to understand usage
ast-grep --pattern 'type StateProcessor interface {
$$$
}'
Length of output: 4881
Script:
#!/bin/bash
# Let's examine the state processor tests in the state-transition module
rg -A 10 "StateProcessor" "mod/state-transition/pkg/core/state_processor_staking_test.go"
# Also check the interface definition
rg -A 10 "type StateProcessor" "mod/primitives/pkg/transition/"
Length of output: 316
Script:
#!/bin/bash
# Let's find the interface definition and its usage
fd -t f "state_processor\.go$" | xargs rg -A 10 "type StateProcessor"
# Check for deposit-related functionality in state processor tests
rg -B 5 -A 10 "deposit" "mod/state-transition/pkg/core/state_processor_staking_test.go"
Length of output: 3949
1-1: Verify mockery version compatibility
The mock has been regenerated with mockery v2.49.1 (upgraded from v2.49.0). While this is a patch version update and should be backward compatible, ensure that:
- All developers are using compatible mockery versions
- The mock generation command is documented in the project
mod/state-transition/pkg/core/errors.go (4)
33-35: LGTM: Clear and consistent error definition.
The error definition is well-documented and follows the established pattern.
37-39: LGTM: Well-defined error for deposit validation.
This error definition aligns perfectly with the PR's objective of validating deposits against contract events.
41-43: Fix typo in error documentation.
The error comment contains a typo: "corrispondent" should be "corresponding".
37-47: Verify completeness of deposit validation errors.
Let's ensure these error definitions cover all possible validation scenarios mentioned in the PR objectives.
✅ Verification successful
Based on the examination of the deposit validation code, I can now provide a final response about the completeness of the error definitions.
Error definitions for deposit validation are complete and well-structured
The current set of error definitions adequately covers all validation scenarios as evidenced by the validation logic in deposits_validation.go:
ErrDepositsLengthMismatch- Used when validating deposit counts between local and payload depositsErrDepositMismatch- Used for content verification between local and payload depositsErrDepositIndexOutOfOrder- Used to ensure deposit indices are contiguousErrExceedsBlockDepositLimit- Used when block exceeds deposit limit (found in state processor)
The validation logic shows these errors are used in both genesis and non-genesis deposit validation paths, with proper error wrapping to provide additional context. No additional error definitions are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any other deposit-related error cases in the codebase
# that might need to be defined here.
# Search for deposit-related error handling
rg -i "deposit.*err|err.*deposit" --type go
# Search for validation checks in the deposit validation logic
ast-grep --pattern 'if $_ {
return $_, $$$
}'
Length of output: 100644
Script:
#!/bin/bash
# Let's check the deposit validation logic to verify error coverage
# Search for deposit validation functions and their error handling
rg -A 10 "func.*validate.*deposit" --type go
# Search for deposit-related error definitions in other files
rg -l "ErrDeposit.*=.*errors\.New" --type go
# Search for deposit validation checks in the state processor
ast-grep --pattern 'if $_ {
return ErrDeposit$_, $$$
}'
Length of output: 272
Script:
#!/bin/bash
# Let's examine the deposit validation implementation and error handling
# Check the deposit validation implementation
rg -A 20 "validateGenesisDeposits|validateNonGenesisDeposits" --type go
# Look at the actual validation checks in the deposits_validation.go file
cat mod/state-transition/pkg/core/deposits_validation.go
# Check for any deposit validation tests that might reveal error scenarios
rg -A 10 "Test.*Deposit.*Valid" --type go
Length of output: 12669
mod/state-transition/pkg/core/mocks/execution_engine.mock.go (2)
Line range hint 1-89: LGTM! Mock implementation aligns with PR objectives.
The ExecutionEngine mock implementation includes proper handling of VerifyAndNotifyNewPayload, which will be crucial for testing the new deposit verification logic mentioned in the PR objectives. The mock maintains type safety through generics and includes proper error handling.
1-1: Verify mockery version upgrade impact.
The mockery version has been bumped from v2.49.0 to v2.49.1. While this is a patch version update and should be safe, let's verify there are no breaking changes.
✅ Verification successful
Mockery v2.49.1 update is safe to proceed
The version bump from v2.49.0 to v2.49.1 is confirmed safe based on:
- The changelog shows only minor fixes and documentation updates, with no breaking changes
- The fix for type aliases to structs (3cef7fd) improves reliability
- Multiple mock files in the codebase are consistently using mockery v2.49.x, indicating proper version alignment
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes in mockery v2.49.1
# Check the mockery changelog or release notes
gh api repos/vektra/mockery/releases/tags/v2.49.1 --jq .body
# Verify that other mockery-generated files in the codebase are consistent
fd -e go -x grep -l "mockery v2.49" {}
Length of output: 2178
mod/engine-primitives/pkg/engine-primitives/mocks/blobs_bundle.mock.go (3)
1-1: LGTM: Auto-generated mock file.
This is an auto-generated mock file by mockery v2.49.1. The mock implementation follows the standard mockery patterns and correctly implements the BlobsBundle interface.
Line range hint 1-150: Verify mock usage in deposit validation tests.
Given that the PR objectives focus on deposit validation and state transitions, we should verify how this mock is utilized in the related test cases.
1-1: 🧹 Nitpick (assertive)
Consider documenting mockery version in go.mod.
The mockery version upgrade from v2.49.0 to v2.49.1 should be reflected in the project's dependencies.
mod/engine-primitives/pkg/engine-primitives/mocks/payload_attributer.mock.go (2)
1-1: LGTM! Automated version bump.
The change from mockery v2.49.0 to v2.49.1 is an automated update that doesn't affect the mock's functionality.
Line range hint 1-140: Verify if deposit-related methods are needed.
Given that this PR focuses on deposit validation and verification against contract events, verify if this mock interface should include any deposit-related methods to support the new functionality.
mod/node-api/backend/mocks/availability_store.mock.go (2)
Line range hint 13-16: Mock implementation aligns with deposit verification requirements.
The generic mock implementation with IsDataAvailable and Persist methods provides the necessary functionality for testing deposit verification scenarios. The type parameters allow for type-safe testing with actual deposit and block body types.
Also applies to: 24-45, 54-89
1-1: Generated file - version upgrade noted.
This is an auto-generated mock file using mockery v2.49.1. The version upgrade from v2.49.0 is a minor change and should not introduce breaking changes.
Let's verify the mockery version consistency across other generated files:
✅ Verification successful
Mockery version is consistent across all generated files
All mock files in the codebase are generated using mockery v2.49.1, confirming consistent versioning across the entire project. No version mismatches were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check mockery version across all generated mock files
# Expected: All files should use the same version v2.49.1
# Search for mockery version declarations in mock files
echo "Checking mockery versions in mock files:"
rg -l "mockery v\d+\.\d+\.\d+" | xargs awk '/mockery v[0-9]+\.[0-9]+\.[0-9]+/ {print FILENAME ":" $0}'
Length of output: 2725
mod/primitives/pkg/crypto/mocks/bls_signer.mock.go (1)
1-1: LGTM! Auto-generated mock file updated.
This is an auto-generated mock file updated to use mockery v2.49.1. The mock implementation includes all necessary methods with proper error handling and follows testing best practices.
Let's verify if this version update is consistent across other mock files:
✅ Verification successful
All mock files consistently use mockery v2.49.1
The verification confirms that all mock files in the codebase are generated using mockery v2.49.1, showing consistent versioning across the entire project. This indicates proper maintenance of mock files and adherence to project standards.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check mockery version consistency across mock files
# Expected: All mock files should use the same mockery version
# Search for mockery version declarations in mock files
rg -l "mockery v[0-9]+\.[0-9]+\.[0-9]+" | xargs awk '/mockery v[0-9]+\.[0-9]+\.[0-9]+/ {print FILENAME ":" $0}'
Length of output: 2643
mod/storage/pkg/interfaces/mocks/db.mock.go (2)
1-1: Verify if mockery version upgrade was intentional.
The mockery version has been bumped from v2.49.0 to v2.49.1. While this is a minor version update and should maintain backward compatibility, please confirm if this upgrade was intentional.
1-1: Check mockery version consistency.
Let's verify that all mock files are using the same mockery version to maintain consistency.
✅ Verification successful
All mock files are using mockery v2.49.1 consistently
The verification shows that all mock files in the codebase are consistently using mockery v2.49.1. There are no version mismatches found across any of the mock files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check mockery version consistency across all mock files
# Search for mockery version declarations in mock files
echo "Checking mockery versions in mock files:"
rg -n "mockery v[0-9]+\.[0-9]+\.[0-9]+" --type go
Length of output: 2720
mod/node-api/backend/mocks/deposit_store.mock.go (3)
1-1: LGTM: Mock generation version update
The update from mockery v2.49.0 to v2.49.1 is a minor version bump that shouldn't introduce breaking changes.
Line range hint 97-134: Review pruning implementation given known issues
The PR objectives mention issues with the deposit pruner. While this is a mock implementation, we should verify that the Prune method's signature and behavior align with the actual requirements.
✅ Verification successful
Let me gather more context about the pruning implementation to verify the alignment.
Let me check one more aspect to understand the Prunable interface and its relationship with the deposit store.
Mock implementation aligns with interface requirements
The mock's Prune implementation correctly matches the interface requirements:
- The signature
Prune(start uint64, end uint64) errormatches the Prunable interface - The method documentation in
node-api/backend/types.goconfirms it prunes the deposit store of [start, end) - The implementation includes proper error handling for missing return values
The mock is consistent with both the Prunable interface and the DepositStore interface requirements, making it suitable for testing pruning functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for pruning-related issues and test coverage
# Search for pruning-related tests
rg -A 5 "TestPrune" --type go
# Look for pruning-related error patterns
rg "prune.*(?i)error" --type go
Length of output: 1013
Script:
#!/bin/bash
# Check the actual pruning interface and implementation
ast-grep --pattern 'type Prunable[T any] interface {
$$$
}'
# Look for the actual deposit store implementation
ast-grep --pattern 'func ($_ *DepositStore) Prune($$$) error {
$$$
}'
# Check pruner implementation
rg -A 10 "type Pruner\[" --type go
Length of output: 1139
Script:
#!/bin/bash
# Check the Prunable interface definition and any implementations
rg -A 5 "type Prunable" --type go
# Look for the actual deposit store type definition and its relationship with Prunable
rg -A 5 "type DepositStore\[" --type go
Length of output: 3184
Line range hint 71-96: Verify the mock's GetDepositsByIndex implementation against contract events
Given the PR's objective to verify deposits against contract events, ensure this mock method accurately represents the behavior needed for deposit validation testing.
✅ Verification successful
Based on the search results, I can now provide a final response:
The mock implementation correctly matches the interface contract and actual implementation
The mock's GetDepositsByIndex implementation aligns with both:
- The interface definition in
mod/node-api/backend/types.gowhich specifiesGetDepositsByIndex(startIndex uint64, numView uint64) ([]DepositT, error) - The actual implementation in
mod/storage/pkg/deposit/store.gowhich handles deposit retrieval by index range
The mock provides the necessary flexibility for testing through its return value handling and run-time behavior configuration, which is sufficient for deposit validation testing scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the actual implementation's signature matches the mock
# and check for any deposit validation logic in tests
# Check the interface definition
rg -A 5 "type DepositStore interface"
# Look for deposit validation tests
rg -l "GetDepositsByIndex.*contract" --type go
Length of output: 88
Script:
#!/bin/bash
# Let's try different approaches to gather the necessary information
# 1. Search for interface or struct definitions related to deposits
rg -A 10 "type Deposit" --type go
# 2. Look for the actual implementation of GetDepositsByIndex
ast-grep --pattern 'func $_($_, $_) GetDepositsByIndex($_, $_) ([]$_, error)'
# 3. Search for test files containing deposit-related tests
fd -e go -E '*_test.go' --exec rg -l "deposit" {}
# 4. Look for mock usage in tests
rg -l "mock.*DepositStore" --type go
Length of output: 17178
Script:
#!/bin/bash
# Let's look for deposit validation logic and contract event verification
# 1. Search for deposit validation implementation
rg -A 10 "func.*GetDepositsByIndex" --type go
# 2. Look for contract event verification logic
rg -A 5 "VerifyDeposit" --type go
# 3. Check deposit validation tests
rg -A 10 "TestGetDepositsByIndex" --type go
# 4. Look for deposit contract verification
rg -A 5 "verifyDepositContract" --type go
Length of output: 4148
mod/storage/pkg/pruner/mocks/block_event.mock.go (2)
1-1: LGTM: Mockery version update is appropriate.
The update from v2.49.0 to v2.49.1 is a patch version increment, indicating backward-compatible bug fixes. The generated code structure remains consistent with best practices.
Line range hint 1-180: Verify mock's compatibility with deposit validation requirements.
The mock implementation appears complete for basic block event testing. However, given this PR's focus on deposit validation, verify that this mock provides sufficient functionality for testing the new deposit validation scenarios.
✅ Verification successful
Mock implementation is compatible with deposit validation requirements
The mock correctly implements all required methods of the BlockEvent interface. From the search results, we can see that:
-
The mock in
mod/storage/pkg/pruner/mocks/block_event.mock.goimplements all methods required by the interface:Is(async.EventID) boolData() BeaconBlockTContext() context.ContextID() async.EventID
-
The deposit package's BlockEvent interface in
mod/execution/pkg/deposit/types.gorequires exactly the same methods, making this mock fully compatible with deposit validation requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the mock is used in deposit validation tests
# and if there are any new interface requirements.
# Look for test files using this mock in deposit validation
rg -l "NewBlockEvent" | grep -i "deposit.*test"
# Check for any new methods added to the original BlockEvent interface
# that might need to be mocked
ast-grep --pattern 'type BlockEvent interface {
$$$
}'
Length of output: 106
Script:
#!/bin/bash
# Let's try a different approach to verify the mock's compatibility
# First, find the actual BlockEvent interface definition
rg -A 10 "type BlockEvent\[" --type go
# Then check for any deposit-related test files that might use BlockEvent
rg -l "BlockEvent" | grep -i "test"
# Also look for any deposit validation related code that might use BlockEvent
rg -l "BlockEvent" | grep -i "deposit"
Length of output: 3359
mod/node-api/backend/mocks/block_store.mock.go (1)
1-1: LGTM: Minor mockery version update.
The update from mockery v2.49.0 to v2.49.1 is a patch version change, suggesting backward compatibility is maintained.
Let's verify the BlockStore interface implementation is complete:
✅ Verification successful
Let me verify the mock implementation against the interface definition with one more check:
Let me try a different approach to verify the mock implementation:
Let me try one final verification approach:
Mock implementation verified and complete
The mock correctly implements all three methods from the BlockStore interface with matching signatures:
- GetParentSlotByTimestamp(timestamp math.U64) (math.Slot, error)
- GetSlotByBlockRoot(root common.Root) (math.Slot, error)
- GetSlotByStateRoot(root common.Root) (math.Slot, error)
The mockery version update from v2.49.0 to v2.49.1 maintains the same interface contract and implementation structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify BlockStore interface definition matches the mock
# Search for the BlockStore interface definition
ast-grep --pattern 'type BlockStore[BeaconBlockT any] interface {
$$$
}'
Length of output: 774
Script:
#!/bin/bash
# Check if the mock implements all required methods
ast-grep --pattern 'func (m *BlockStore[BeaconBlockT]) $_($$$) ($$$) {
$$$
}'
Length of output: 81
Script:
#!/bin/bash
# Search for method implementations in the mock file
rg "func \(m \*MockBlockStore" mod/node-api/backend/mocks/block_store.mock.go -A 2
Length of output: 84
Script:
#!/bin/bash
# Display the content of the mock file to verify implementations
cat mod/node-api/backend/mocks/block_store.mock.go
Length of output: 6661
mod/node-api/backend/mocks/validator.mock.go (2)
1-1: LGTM: Mockery version update.
The update from mockery v2.49.0 to v2.49.1 is a minor version bump with no functional changes to the generated code.
1-1: Verify consistent mockery version across all generated files.
Since this is an auto-generated file, we should ensure all mock files are generated with the same version of mockery.
✅ Verification successful
All mock files are consistently using mockery v2.49.1
The verification shows that all 24 mock files across different modules are generated using the same version of mockery (v2.49.1). There are no version inconsistencies to address.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for inconsistent mockery versions across generated files
# Search for mockery version declarations
echo "Checking mockery versions in generated files..."
rg -n "Code generated by mockery v" --type go
# Count unique versions
echo -e "\nUnique versions found:"
rg "Code generated by mockery v" --type go | sort | uniq -c
Length of output: 5575
mod/engine-primitives/pkg/engine-primitives/mocks/built_execution_payload_env.mock.go (2)
1-1: LGTM on mockery version upgrade.
The update from mockery v2.49.0 to v2.49.1 is a minor version change and should not introduce breaking changes.
Line range hint 1-180: Verify if deposit-related mock methods are needed.
Given that this PR focuses on deposit validation against contract events, it seems that this mock interface might need additional methods to support the new deposit validation functionality. The current interface only provides methods for blobs, payload, value, and builder override.
Let's verify if there are any deposit-related interfaces that this mock should implement:
mod/node-api/backend/mocks/storage_backend.mock.go (2)
1-1: LGTM: Mockery version update
The update from mockery v2.49.0 to v2.49.1 is a minor version change. The generated mock implementation remains functionally equivalent and maintains all necessary methods for deposit store access and state transitions.
Line range hint 1-180: Verify mock usage in state transition tests
The mock implementation provides all necessary methods to support the PR's objective of validating deposits against contract events. Ensure that the state transition tests properly utilize these mocks, especially the DepositStore() method.
✅ Verification successful
Let me gather more information about the test usage of this mock by searching for test files that might be using the mock indirectly.
Let me check the content of the state transition test files to see how deposits are being tested.
Based on the test files examined, I can now generate the final response:
StorageBackend mock is properly utilized in state transition tests
The mock implementation is correctly used in the state transition tests, particularly in genesis state initialization tests. The tests properly validate deposit handling through:
- Initialization tests that verify deposit processing with different amounts and validator states
- Both regular network and Bartio-specific test cases that validate deposit store functionality
- Comprehensive validation of deposit-related state changes and balances
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of StorageBackend mock in state transition tests
# Expected: Find test files that use this mock for deposit validation
# Search for test files using StorageBackend mock
echo "Test files using StorageBackend mock:"
rg -l "NewStorageBackend.*testing.T" --type go
# Search for deposit validation tests
echo -e "\nDeposit validation test cases:"
rg -B 2 -A 2 "DepositStore\(\)" --type go
Length of output: 3143
Script:
#!/bin/bash
# Search for test files that might use the StorageBackend mock
echo "Test files potentially using StorageBackend mock:"
rg -l "storage_backend.*mock" --type go
# Search for test files containing deposit validation logic
echo -e "\nTest files with deposit validation:"
rg -l "deposit.*test" --type go
# Look for actual test implementations using the mock
echo -e "\nTest implementations using StorageBackend mock:"
rg -B 3 -A 3 "mock\.StorageBackend" --type go
Length of output: 625
Script:
#!/bin/bash
# Check state transition test files for deposit validation
echo "Content of state processor genesis test:"
cat mod/state-transition/pkg/core/state_processor_genesis_test.go
echo -e "\nContent of helpers test:"
cat mod/state-transition/pkg/core/helpers_test.go
# Look for mock setup in these tests
echo -e "\nMock setup in test files:"
rg -B 5 -A 5 "func Test.*" mod/state-transition/pkg/core/state_processor_genesis_test.go mod/state-transition/pkg/core/helpers_test.go
Length of output: 17653
mod/node-api/backend/mocks/beacon_block_header.mock.go (4)
Line range hint 13-15: LGTM: Generic mock implementation supports flexible testing
The generic implementation with BeaconBlockHeaderT allows for flexible testing scenarios, which is crucial for validating different block header implementations in the context of deposit verification.
Also applies to: 16-18
1-1: Verify mockery version compatibility with the codebase
The mock generator version has been updated from v2.49.0 to v2.49.1. While this is a minor version bump, we should ensure compatibility with other mocks in the codebase.
✅ Verification successful
Mockery version is consistent across the codebase
All mock files in the codebase are using mockery v2.49.1, indicating perfect version consistency. No compatibility issues are expected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check mockery version consistency across the codebase
# Look for other mockery version declarations to ensure consistency
# Search for mockery version declarations in mock files
rg -l "mockery v[0-9]+\.[0-9]+\.[0-9]+" --type go
# Check if there are any version mismatches
rg "mockery v[0-9]+\.[0-9]+\.[0-9]+" --type go | sort | uniq -c
Length of output: 4145
Line range hint 249-253: Ensure state root validation in deposit verification
The SetStateRoot and GetStateRoot methods are essential for maintaining block state consistency during deposit verification. Make sure test cases cover scenarios where state changes occur due to deposit processing.
Also applies to: 254-258, 259-263, 264-268
Line range hint 32-38: Verify mock's integration with deposit validation flow
The GetBodyRoot implementation and its expecter methods are crucial for the PR's objective of verifying deposits against contract events. Ensure that test cases utilizing this mock properly validate the deposit list within the block body.
Consider documenting the mock's role in deposit validation tests to help other developers understand the testing strategy.
Also applies to: 39-43, 44-48, 49-53, 54-58
mod/node-api/backend/mocks/beacon_state.mock.go (3)
Line range hint 16-24: LGTM: Generated mock implementation is well-structured
The generated code demonstrates good practices:
- Proper use of generics for type safety
- Comprehensive error handling
- Clean test cleanup through mock expectations
Line range hint 25-1000: LGTM: Mock implementation supports PR objectives
The mock provides all necessary methods to support the PR's goal of enhancing deposit validation and state transition processing:
- Comprehensive validator management methods
- State access and modification capabilities
- Methods for handling deposits and transitions
1-1: Verify mockery version upgrade impact
The upgrade from mockery v2.49.0 to v2.49.1 is a patch version change. While this should only contain backwards compatible bug fixes, it's good practice to verify the changes.
✅ Verification successful
Mockery v2.49.1 upgrade is safe to proceed
The changelog for mockery v2.49.1 shows only minor changes including:
- Bug fix for type aliases to structs
- Documentation improvements
- Deprecation warning for
with-expecterfeature - Internal tooling fixes
None of these changes introduce breaking changes or affect the generated mock functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check mockery changelog for any breaking changes or important fixes in v2.49.1
# Get the mockery releases information
gh api repos/vektra/mockery/releases | jq -r '.[] | select(.tag_name == "v2.49.1") | .body'
Length of output: 812
mod/state-transition/pkg/core/deposits_validation.go (1)
68-68: Confirm integer conversion is safe and intentional
The use of math.U64(i) to convert the loop index from int to uint64 may raise concerns about integer overflow. Since this conversion has been previously discussed and accepted, ensure that this usage is intentional and safe.
Refer to the learning:
In
mod/state-transition/pkg/core/deposits_validation.go, it's acceptable to usemath.U64(i)to convert the loop indexifrominttouint64when validating deposits.
Also applies to: 139-139, 143-143
🧰 Tools
🪛 golangci-lint (1.62.2)
[high] 68-68: G115: integer overflow conversion int -> uint64
(gosec)
| @@ -12,7 +12,7 @@ generate: ## generate all the code | |||
| (cd $$module && \ | |||
| GETH_PKG_INCLUDE=$(GETH_PKG_INCLUDE) go generate ./...) || exit 1; \ | |||
| done | |||
| @go run github.com/vektra/mockery/v2@latest | |||
There was a problem hiding this comment.
anchored mockery to avoid frequent updates. We can revisit say once per month
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
mod/state-transition/pkg/core/deposits_validation.go(1 hunks)
🧰 Additional context used
📓 Learnings (1)
mod/state-transition/pkg/core/deposits_validation.go (2)
Learnt from: abi87
PR: berachain/beacon-kit#2119
File: mod/state-transition/pkg/core/state_processor_genesis.go:109-118
Timestamp: 2024-11-12T11:12:56.773Z
Learning: In `InitializePreminedBeaconStateFromEth1` in `mod/state-transition/pkg/core/state_processor_genesis.go`, we enforce that there is a single deposit per validator in the genesis format; therefore, handling multiple deposits per validator is not necessary.
Learnt from: abi87
PR: berachain/beacon-kit#2115
File: mod/state-transition/pkg/core/deposits_validation.go:61-61
Timestamp: 2024-11-26T23:37:11.522Z
Learning: In `mod/state-transition/pkg/core/deposits_validation.go`, it's acceptable to use `math.U64(i)` to convert the loop index `i` from `int` to `uint64` when validating deposits.
🪛 golangci-lint (1.62.2)
mod/state-transition/pkg/core/deposits_validation.go
[high] 68-68: G115: integer overflow conversion int -> uint64
(gosec)
[high] 140-140: G115: integer overflow conversion int -> uint64
(gosec)
🔇 Additional comments (2)
mod/state-transition/pkg/core/deposits_validation.go (2)
124-128: 🧹 Nitpick (assertive)
Improve log message clarity
The log message could be more descriptive to aid in debugging.
Apply this diff to improve the log message:
sp.logger.Info(
- "processOperations",
+ "Validating non-genesis deposits",
"Expected deposit start index", expectedStartIdx,
- "Expected deposits length", len(localDeposits),
+ "Number of deposits from contract", len(localDeposits),
)Likely invalid or redundant comment.
39-48: 🧹 Nitpick (assertive)
Combine similar chain ID cases to reduce duplication
Both BartioChainID and BoonetEth1ChainID cases skip validation for backward compatibility. Consider combining them to reduce code duplication.
Apply this diff to combine the cases:
switch {
- case sp.cs.DepositEth1ChainID() == spec.BartioChainID:
- // Bartio does not properly validate deposits index
- // We skip checks for backward compatibility
- return nil
-
- case sp.cs.DepositEth1ChainID() == spec.BoonetEth1ChainID:
- // Boonet inherited the bug from Bartio and it may have added some
- // validators before we activate the fork. So we skip validation
- // before fork activation
+ case sp.cs.DepositEth1ChainID() == spec.BartioChainID ||
+ sp.cs.DepositEth1ChainID() == spec.BoonetEth1ChainID:
+ // Skip validation for Bartio and Boonet chains
+ // as they do not properly validate deposit indices
+ // for backward compatibility
return nilLikely invalid or redundant comment.
| func (sp *StateProcessor[ | ||
| _, _, _, BeaconStateT, _, DepositT, | ||
| _, _, _, _, _, _, _, _, _, _, _, | ||
| ]) validateNonGenesisDeposits( | ||
| st BeaconStateT, | ||
| deposits []DepositT, | ||
| ) error { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add function documentation
Please add a documentation comment explaining the purpose of this function, its parameters, and return values.
Add this documentation above the function:
+// validateNonGenesisDeposits validates non-genesis deposits by comparing them with
+// deposits from the deposit contract. For non-Bartio/Boonet chains (or post-fork),
+// it ensures:
+// 1. Number of deposits matches the contract
+// 2. Deposit indices are contiguous
+// 3. Deposits match exactly with contract deposits
+// Parameters:
+// - st: The beacon state
+// - deposits: List of deposits to validate
+// Returns an error if validation fails
func (sp *StateProcessor[📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (sp *StateProcessor[ | |
| _, _, _, BeaconStateT, _, DepositT, | |
| _, _, _, _, _, _, _, _, _, _, _, | |
| ]) validateNonGenesisDeposits( | |
| st BeaconStateT, | |
| deposits []DepositT, | |
| ) error { | |
| // validateNonGenesisDeposits validates non-genesis deposits by comparing them with | |
| // deposits from the deposit contract. For non-Bartio/Boonet chains (or post-fork), | |
| // it ensures: | |
| // 1. Number of deposits matches the contract | |
| // 2. Deposit indices are contiguous | |
| // 3. Deposits match exactly with contract deposits | |
| // Parameters: | |
| // - st: The beacon state | |
| // - deposits: List of deposits to validate | |
| // Returns an error if validation fails | |
| func (sp *StateProcessor[ | |
| _, _, _, BeaconStateT, _, DepositT, | |
| _, _, _, _, _, _, _, _, _, _, _, | |
| ]) validateNonGenesisDeposits( | |
| st BeaconStateT, | |
| deposits []DepositT, | |
| ) error { |
| func (sp *StateProcessor[ | ||
| _, _, _, BeaconStateT, _, DepositT, | ||
| _, _, _, _, _, _, _, _, _, _, _, | ||
| ]) validateGenesisDeposits( | ||
| st BeaconStateT, | ||
| deposits []DepositT, | ||
| ) error { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add function documentation
Please add a documentation comment explaining the purpose of this function, its parameters, and return values. This will help other developers understand the validation rules for genesis deposits.
Add this documentation above the function:
+// validateGenesisDeposits validates the deposits provided at genesis according to chain-specific rules.
+// For non-Bartio/Boonet chains, it ensures:
+// 1. No Eth1DepositIndex exists before the first genesis deposit
+// 2. At least one validator exists in genesis
+// 3. Deposit indices are contiguous
+// Parameters:
+// - st: The beacon state
+// - deposits: List of deposits to validate
+// Returns an error if validation fails
func (sp *StateProcessor[📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (sp *StateProcessor[ | |
| _, _, _, BeaconStateT, _, DepositT, | |
| _, _, _, _, _, _, _, _, _, _, _, | |
| ]) validateGenesisDeposits( | |
| st BeaconStateT, | |
| deposits []DepositT, | |
| ) error { | |
| // validateGenesisDeposits validates the deposits provided at genesis according to chain-specific rules. | |
| // For non-Bartio/Boonet chains, it ensures: | |
| // 1. No Eth1DepositIndex exists before the first genesis deposit | |
| // 2. At least one validator exists in genesis | |
| // 3. Deposit indices are contiguous | |
| // Parameters: | |
| // - st: The beacon state | |
| // - deposits: List of deposits to validate | |
| // Returns an error if validation fails | |
| func (sp *StateProcessor[ | |
| _, _, _, BeaconStateT, _, DepositT, | |
| _, _, _, _, _, _, _, _, _, _, _, | |
| ]) validateGenesisDeposits( | |
| st BeaconStateT, | |
| deposits []DepositT, | |
| ) error { |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
mod/beacon/validator/block_builder.go(2 hunks)
🔇 Additional comments (2)
mod/beacon/validator/block_builder.go (2)
29-29: LGTM: Import addition is necessary for chain-specific constants
The new import provides access to chain-specific constants used in deposit validation logic.
Line range hint 305-311: Verify deposit store implementation
The deposit store interaction needs verification to ensure it properly handles the modified deposit index logic.
Let's verify the deposit store implementation:
✅ Verification successful
Deposit store implementation is properly handling index-based retrieval
The implementation in mod/storage/pkg/deposit/store.go shows that the GetDepositsByIndex method correctly:
- Uses thread-safe access with read locks
- Retrieves deposits sequentially from the given start index
- Properly handles the range limit via
MaxDepositsPerBlock - Returns early if a deposit is not found (ErrNotFound)
- Includes comprehensive logging for debugging
- Uses proper error handling and propagation
The implementation aligns with the usage in block_builder.go and properly handles the deposit index logic with no issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for GetDepositsByIndex implementation
ast-grep --pattern 'func $_($$) GetDepositsByIndex($_, $_) $$ {
$$$
}'
# Search for tests covering this functionality
rg "TestGetDepositsByIndex|TestDepositStore" -l
Length of output: 124
Script:
#!/bin/bash
# Let's try a broader search for deposit store implementations and tests
# Search for deposit store interface and implementation
rg -l "type.*DepositStore"
# Search for GetDepositsByIndex method
rg "GetDepositsByIndex" -A 5
# Search for deposit-related tests with a broader pattern
rg -l "Test.*Deposit"
Length of output: 9012
Script:
#!/bin/bash
# Let's examine the implementation and tests in more detail
# Check the KVStore implementation
cat mod/storage/pkg/deposit/store.go
# Look for deposit store tests
rg -A 10 "func Test.*Deposit.*Store" mod/storage/pkg/deposit/
Length of output: 4732
Deposit list in execution payload body should be validated against the deposit contracts events to avoid creation of fake validators or censorship of real validators.
To do that this PR:
Eth1DepositIndexto duly point to the latest stored deposit indexWhat this PR does not (yet)
(start, end)indexes to prune with(start, range))-32001disappearNote:
and several like
Summary by CodeRabbit
Release Notes
New Features
Equalsmethod for comparing deposit instances.DepositStoreinterface for managing deposit data.Improvements
Configuration Changes
Documentation