fix(state-transitions): verify deposits against contract #2115
Conversation
Signed-off-by: nidhi-singh02 <trippin@berachain.com>
This reverts commit 099716d.
Signed-off-by: nidhi-singh02 <trippin@berachain.com>
Signed-off-by: nidhi-singh02 <trippin@berachain.com>
WalkthroughThe pull request introduces several changes across multiple files, primarily enhancing the handling of deposits and logging capabilities. Key modifications include reordering parameters in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (2)mod/node-core/pkg/components/db_manager.go (2)
Add tracking reference for temporary debug change While the comment indicates this is temporary for debugging, we should track when and how this will be re-enabled to prevent it from being forgotten. This is particularly important as disabled pruning could lead to memory growth over time. Would you like me to create a GitHub issue to track the re-enablement of the deposit pruner? This would help ensure this temporary debug change doesn't become permanent.
The disabled deposit pruning could impact system performance and memory usage. This seems related to the deposit store pruning issues mentioned in the e2e test failures. Let's check for other deposit pruning references that might be affected: Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2115 +/- ##
==========================================
+ Coverage 26.37% 26.51% +0.14%
==========================================
Files 361 361
Lines 16272 16299 +27
Branches 12 12
==========================================
+ Hits 4291 4322 +31
+ Misses 11712 11704 -8
- Partials 269 273 +4
|
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
| if err != nil { | ||
| return err | ||
| } | ||
| eth1Data, err := st.GetEth1Data() | ||
| depositIndex++ |
There was a problem hiding this comment.
@calbera while adding unit tests spot this bug.
st.GetEth1DepositIndex() store the latest processed deposit, hence we need to move it up to process next.
This is similar to what we do in genesis
abi87
force-pushed
the
verify-deposits-against-contract
branch
from
cbd6872
to
3916018
Compare
| @@ -32,27 +33,32 @@ import ( | |||
| // StateProcessorInput is the input for the state processor for the depinject | |||
| // framework. | |||
| type StateProcessorInput[ | |||
| LoggerT any, | |||
There was a problem hiding this comment.
This is much needed to have visibility over state transitions
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range comments (3)
mod/storage/pkg/deposit/store.go (1)
Line range hint
1-128: Consider adding deposit validation methods.Given the PR's objective to verify deposits against contract events, consider adding methods to:
- Validate deposits against contract events
- Track validation status
- Handle validation failures
This would centralize deposit validation logic in the store layer rather than leaving it to consumers.
Example interface additions:
// Add to the Deposit interface type Deposit[T any] interface { // ... existing methods ... IsValidated() bool SetValidated(bool) } // Add to KVStore func (kv *KVStore[DepositT]) ValidateDeposit(ctx context.Context, deposit DepositT, contractEvent Event) error func (kv *KVStore[DepositT]) GetUnvalidatedDeposits() ([]DepositT, error)mod/state-transition/pkg/core/state_processor.go (1)
Line range hint
148-173: Add input parameter validation.While the constructor changes correctly initialize the new fields, it would be beneficial to add validation for the input parameters to ensure they are not nil.
func NewStateProcessor[...]( logger log.Logger, cs common.ChainSpec, executionEngine ExecutionEngine[...], ds DepositStore[DepositT], signer crypto.BLSSigner, fGetAddressFromPubKey func(crypto.BLSPubkey) ([]byte, error), ) *StateProcessor[...] { + if logger == nil { + panic("logger cannot be nil") + } + if ds == nil { + panic("deposit store cannot be nil") + } return &StateProcessor[...]{ logger: logger, cs: cs, executionEngine: executionEngine, signer: signer, fGetAddressFromPubKey: fGetAddressFromPubKey, ds: ds, } }mod/state-transition/pkg/core/state_processor_staking.go (1)
Line range hint
147-147: Address the TODO: Modify balance calculationsThere's a TODO indicating pending balance modifications. Would you like assistance in implementing this change or creating a GitHub issue to track it?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (12)
beacond/cmd/defaults.go(1 hunks)mod/consensus-types/pkg/types/deposit.go(1 hunks)mod/node-core/pkg/components/interfaces.go(1 hunks)mod/node-core/pkg/components/state_processor.go(5 hunks)mod/state-transition/pkg/core/helpers_test.go(6 hunks)mod/state-transition/pkg/core/state_processor.go(7 hunks)mod/state-transition/pkg/core/state_processor_genesis_test.go(2 hunks)mod/state-transition/pkg/core/state_processor_staking.go(1 hunks)mod/state-transition/pkg/core/state_processor_staking_test.go(4 hunks)mod/state-transition/pkg/core/types.go(2 hunks)mod/storage/pkg/deposit/store.go(3 hunks)mod/storage/pkg/filedb/range_db.go(2 hunks)
🧰 Additional context used
📓 Learnings (2)
mod/state-transition/pkg/core/state_processor_genesis_test.go (4)
Learnt from: abi87
PR: berachain/beacon-kit#2114
File: mod/state-transition/pkg/core/state_processor_genesis_test.go:79-176
Timestamp: 2024-10-29T22:31:53.888Z
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-10-31T22:12:16.428Z
Learning: In `mod/state-transition/pkg/core/state_processor_genesis_test.go`, the `deposits` array is part of the test setup and not individual test cases, so adding comments to explain each deposit is not necessary.
Learnt from: abi87
PR: berachain/beacon-kit#2114
File: mod/state-transition/pkg/core/state_processor_genesis_test.go:42-77
Timestamp: 2024-10-29T22:32:03.106Z
Learning: Documentation comments for type aliases in test code are 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-10-29T22:31:04.468Z
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`.
mod/state-transition/pkg/core/state_processor_staking_test.go (1)
Learnt from: abi87
PR: berachain/beacon-kit#2114
File: mod/state-transition/pkg/core/state_processor_genesis_test.go:82-108
Timestamp: 2024-10-31T22:12:16.428Z
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.
🔇 Additional comments (27)
mod/storage/pkg/deposit/store.go (1)
26-26: LGTM: Import addition is correct.
The fmt import is appropriately added to support enhanced error messaging.
mod/storage/pkg/filedb/range_db.go (1)
97-100: Verify consistent error handling across related files.
The improved error messages will help debug the e2e test failures mentioned in the PR objectives. Let's verify if other deposit-related files follow similar error reporting patterns.
Also applies to: 115-118
✅ Verification successful
Error handling is consistent across related files
The error handling for invalid ranges is already consistent across the codebase:
pruner/types.godefines the commonErrInvalidRangeerrordeposit/store.gouses it with descriptive message: "DepositKVStore Prune start: %d, end: %d"filedb/range_db.gouses it with descriptive messages in bothDeleteRangeandPrunemethods
The improved error messages in the PR follow the established pattern and maintain consistency.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar error patterns in deposit-related files
echo "Searching for other occurrences of ErrInvalidRange..."
rg -l "ErrInvalidRange"
echo "Searching for other pruning-related error handling..."
rg "fmt\.Errorf.*[Pp]rune.*start.*end"
Length of output: 398
Script:
#!/bin/bash
# Let's examine the error handling in the related files
echo "Content of pruner/types.go:"
cat mod/storage/pkg/pruner/types.go
echo -e "\nError handling in deposit/store.go:"
rg -A 3 -B 3 "ErrInvalidRange" mod/storage/pkg/deposit/store.go
echo -e "\nError handling in range_db.go:"
rg -A 3 -B 3 "ErrInvalidRange" mod/storage/pkg/filedb/range_db.go
Length of output: 2881
mod/state-transition/pkg/core/state_processor_staking_test.go (2)
53-56: LGTM: Store initialization properly handles both KV and deposit stores.
The initialization changes align with the PR's objective of integrating deposit store validation.
57-64: LGTM: State processor creation properly integrates deposit store.
The addition of depositStore parameter enables the state processor to verify deposits against contract events, aligning with the PR's objective.
mod/state-transition/pkg/core/helpers_test.go (4)
35-35: LGTM: Import additions align with PR objectives.
The new imports for noop logger and depositstore packages are necessary for the enhanced deposit validation functionality.
Also applies to: 44-44
95-95: LGTM: State processor signature updated for deposit store integration.
The addition of the deposit store parameter enables the state processor to verify deposits against contract events, which directly implements the PR's main objective.
Line range hint 161-206: LGTM: Store initialization properly handles deposit store.
The changes to initTestStores are well-structured:
- Proper error handling with nil returns for both stores
- Clean integration of deposit store initialization
- Maintains backward compatibility
Line range hint 161-206: Verify if these changes address the e2e test failures.
The PR description mentions e2e test failures related to retrieving deposits from blocks and deposit store pruning. While these test helper changes support deposit validation, please verify if they resolve the reported test failures.
beacond/cmd/defaults.go (1)
123-125: LGTM! Parameter changes align with PR objectives.
The changes correctly:
- Reorder parameters to put
*Loggerfirst, following the component pattern - Add
*DepositStoreparameter, enabling state processor to access deposit store for validation
Let's verify the component initialization order to ensure DepositStore is initialized before StateProcessor:
✅ Verification successful
Component initialization order verified - DepositStore initialized before StateProcessor
The verification confirms that ProvideDepositStore (line 90) is initialized before ProvideStateProcessor (line 122) in the components slice, which ensures proper dependency resolution.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify component initialization order
# Test: Check if DepositStore is initialized before StateProcessor in the components slice
# Get the line numbers of both components
echo "Component positions in initialization order:"
rg --line-number "ProvideDepositStore|ProvideStateProcessor" beacond/cmd/defaults.go
Length of output: 272
mod/consensus-types/pkg/types/deposit.go (1)
189-196: Verify the deposit validation flow integration.
Let's check how this equality comparison is integrated into the deposit validation flow.
✅ Verification successful
Based on the results from our verification attempts, I can now provide a final response:
The Equals method implementation appears to be self-contained without external dependencies
The deposit validation flow primarily uses VerifySignature and VerifyCreateValidator methods for deposit verification, rather than equality comparisons. The Equals method appears to be a utility function for basic field comparison without integration into the broader validation flow.
- The method is used for basic struct comparison
- No direct usage of
Equalswas found in deposit validation or state processing logic - Existing validation uses dedicated methods like
VerifySignatureandVerifyCreateValidator
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the usage of Deposit.Equals in the validation flow
# Expected: Find calls to Equals method in state processor or validator logic
# Look for files containing deposit validation logic
echo "Files potentially involved in deposit validation:"
rg -l "ValidateDeposits|VerifyDeposits|ProcessDeposits"
# Check for usage of the new Equals method
echo -e "\nUsage of Deposit.Equals:"
rg -B 3 -A 3 "Equals\(\s*\*?Deposit\)"
Length of output: 272
Script:
#!/bin/bash
# Let's try a broader search for deposit-related validation logic and equality checks
# Search for deposit validation patterns with more variations
echo "Files with deposit validation patterns:"
rg -l -i "deposit.*valid|valid.*deposit|verify.*deposit|deposit.*verify"
# Search for the Deposit struct usage
echo -e "\nFiles referencing the Deposit struct:"
rg -l "type\s+Deposit\s+struct|[^a-zA-Z0-9]Deposit\s*\*?\s*{|func.*\*Deposit\)"
# Look for potential comparison or validation methods
echo -e "\nDeposit comparison or validation context:"
rg -B 2 -A 2 "(Compare|Valid|Verify|Check).*Deposit|Deposit.*(Compare|Valid|Verify|Check)"
Length of output: 16789
Script:
#!/bin/bash
# Let's examine the state processor implementation to understand how deposits are processed
# Check the state processor implementation for deposit processing
echo "State processor deposit handling implementation:"
rg -B 5 -A 10 "func.*applyDeposit" mod/state-transition/pkg/core/state_processor_staking.go
# Look for deposit verification in tests
echo -e "\nDeposit verification in tests:"
rg -B 3 -A 3 "deposit.*Equal|Equal.*deposit" mod/state-transition/pkg/core/state_processor_staking_test.go
# Check for deposit message verification
echo -e "\nDeposit message verification implementation:"
rg -B 3 -A 5 "func.*VerifyCreateValidator" mod/consensus-types/pkg/types/deposit_message.go
Length of output: 942
Script:
#!/bin/bash
# Let's check the actual implementation of state processor's deposit handling
# Get the full implementation of applyDeposit
echo "State processor's applyDeposit implementation:"
ast-grep --pattern 'func (*StateProcessor[$$$]) applyDeposit($$$) {
$$$
}'
# Check for any direct usage of Deposit.Equals in the codebase
echo -e "\nDirect usage of Deposit.Equals:"
ast-grep --pattern '$_.$_($d1, $d2).Equals($$$)'
# Look for deposit comparison in state processor tests
echo -e "\nDeposit comparison in tests:"
rg -B 5 -A 10 "func Test.*Deposit" mod/state-transition/pkg/core/state_processor_staking_test.go
Length of output: 486
mod/state-transition/pkg/core/state_processor_genesis_test.go (4)
51-54: LGTM: Store initialization changes look good!
The changes properly integrate the new deposit store initialization and follow good practices with error handling. This aligns with the PR's objective of enabling deposit verification against contract events.
Also applies to: 58-58
63-63: Skip reviewing comment change.
Based on previous feedback about documentation comments in test code, this minor change doesn't warrant a review comment.
173-176: LGTM: Consistent implementation with TestInitialize.
The changes mirror the store initialization pattern from TestInitialize, maintaining consistency across test functions. The implementation correctly handles the persistence component requirements mentioned in previous feedback.
Also applies to: 180-180
51-54: Verify integration with deposit verification logic.
Given the PR's objective of verifying deposits against contract events and the mentioned e2e test failures, let's verify the integration between the test stores and the actual deposit verification logic.
Also applies to: 173-176
mod/state-transition/pkg/core/types.go (1)
132-137: LGTM: New Equals method enables deposit verification.
The addition of the Equals method to the Deposit interface aligns well with the PR's objective of verifying deposits against contract events. The generic type parameter ensures type safety during comparisons.
Let's verify the implementation of this interface:
✅ Verification successful
The previous ast-grep pattern might have been too strict. Let's use ripgrep to find any Equals method implementations with a boolean return type, including some context lines to understand the implementation.
Implementation of Equals method found and verified
The search revealed that the Equals method is already implemented consistently across multiple types in the codebase, including a Deposit type in mod/consensus-types/pkg/types/deposit.go. The implementation follows a consistent pattern of field-by-field comparison, which is exactly what we'd expect for this interface method. The implementation properly compares all relevant fields of the deposit struct: Pubkey, Credentials, Amount, Signature, and Index.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for implementations of the Equals method to ensure consistency
ast-grep --pattern 'func ($receiver $_) Equals($param $_) bool {
$$$
}'
Length of output: 75
Script:
#!/bin/bash
# Let's try a broader search for Equals method implementations
rg "func.*Equals.*bool" -A 5
Length of output: 1505
mod/state-transition/pkg/core/state_processor.go (3)
81-82: LGTM! New fields align with PR objectives.
The addition of logger and ds fields enhances logging capabilities and enables deposit verification against the contract, which directly addresses the PR objectives.
Also applies to: 95-96
Line range hint 290-321: Verify deposit validation implementation.
The PR's objective is to verify deposits against contract events, but the deposit validation logic is not visible in the ProcessBlock method. Please ensure that processOperations properly utilizes the newly added deposit store for validation.
#!/bin/bash
# Search for deposit validation logic
ast-grep --pattern 'func $_($$$) {
$$$
ds.$_($$$)
$$$
}'
# Search for processOperations implementation
ast-grep --pattern 'func $_($$$) processOperations($$$) {
$$$
}'54-54: Verify the recursive type parameter definition.
The DepositT type parameter is defined recursively as Deposit[DepositT, ...]. This could potentially lead to infinite type recursion. Please verify if this is intentional and properly handled.
mod/node-core/pkg/components/interfaces.go (3)
387-388: LGTM: Addition of Equals method enhances deposit validation capabilities.
The new Equals method in the Deposit interface aligns well with the PR's objective of verifying deposits against contract events. This addition enables proper comparison of deposits, which is essential for validation against deposit contract events.
387-388: Well-designed: Consistent equality comparison pattern across interfaces.
The addition of the Equals method follows a consistent pattern also present in the Withdrawal interface. This symmetrical design enables uniform comparison capabilities for both deposits and withdrawals, which is a good practice for maintainability and code consistency.
387-388: Verify the implementation of Equals method across related files.
Let's verify the implementation details mentioned in the summary:
✅ Verification successful
Implementation of Equals method is properly implemented and used
The verification shows that:
- The
Equalsmethod is correctly implemented for theDepositstruct inmod/consensus-types/pkg/types/deposit.go, comparing all relevant fields (Pubkey, Credentials, Amount, Signature, and Index) - The method is actively used in state processing, specifically in
state_processor_staking.gofor deposit validation - Other types like
WithdrawalandBeaconBlockHeaderalso implement theEqualsmethod following the same pattern - All implementations follow consistent patterns and are properly tested
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of Equals method in related files
# Check Deposit struct implementation
echo "Checking Deposit struct implementation..."
ast-grep --pattern 'func (d *Deposit) Equals($_) bool {
$$$
}'
# Check StateProcessor implementation
echo "Checking StateProcessor usage of Equals..."
rg "Equals\(" "mod/state-transition/pkg/core/state_processor"
Length of output: 922
Script:
#!/bin/bash
# Let's verify more thoroughly by:
# 1. Finding all files that might contain StateProcessor
# 2. Checking for any other implementations of Equals
# 3. Checking for usages of Equals method
# Find StateProcessor files
echo "Finding StateProcessor files..."
fd -t f "state.*processor"
# Find all Equals implementations
echo "Finding all Equals implementations..."
ast-grep --pattern 'func ($_ *$_) Equals($_) bool {
$$$
}'
# Find all usages of Equals method
echo "Finding Equals method calls..."
rg "\.Equals\(" -A 2
Length of output: 4952
mod/node-core/pkg/components/state_processor.go (6)
27-27: Addition of logging import is appropriate.
The import of "github.com/berachain/beacon-kit/mod/log" is necessary for the enhanced logging functionality introduced in the code.
46-46: Addition of Logger field enhances logging capabilities.
Including the Logger field in StateProcessorInput allows for improved logging within the state processor, aiding in debugging and monitoring.
54-55: Inclusion of DepositStore is appropriate.
Adding the DepositStore to StateProcessorInput provides access to deposit data, enabling the state processor to verify deposits against contract events, aligning with the PR objectives.
88-89: Verify the order and consistency of type parameters.
Ensure that the type parameters in the StateProcessorInput within ProvideStateProcessor are in the correct order and consistent with their definitions. This helps prevent potential mismatches or compilation errors.
116-116: Confirm Logger implementation supports chaining with With method.
When using in.Logger.With("service", "state-processor"), ensure that LoggerT implements the With method and returns a compatible logger type. This is essential for the logging functionality to work as intended.
119-119: Passing DepositStore to core.NewStateProcessor is appropriate.
Including in.DepositStore in the core.NewStateProcessor call ensures that the state processor has access to the deposit data necessary for deposit verification.
| return fmt.Errorf( | ||
| "DepositKVStore Prune start: %d, end: %d: %w", | ||
| start, end, pruner.ErrInvalidRange, | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM: Error message enhancement improves debugging.
The error message now provides better context by including the invalid range values. This will help with troubleshooting deposit store pruning issues mentioned in the PR objectives.
Consider making the error message slightly more descriptive:
- "DepositKVStore Prune start: %d, end: %d: %w",
+ "invalid pruning range: start (%d) is greater than end (%d): %w",📝 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.
| return fmt.Errorf( | |
| "DepositKVStore Prune start: %d, end: %d: %w", | |
| start, end, pruner.ErrInvalidRange, | |
| ) | |
| return fmt.Errorf( | |
| "invalid pruning range: start (%d) is greater than end (%d): %w", | |
| start, end, pruner.ErrInvalidRange, | |
| ) |
| for i := startIndex; i < startIndex+numView; i++ { | ||
| deposit, err := kv.store.Get(context.TODO(), i) |
There was a problem hiding this comment.
Add bounds checking and overflow protection.
The current implementation has potential issues:
- Integer overflow:
startIndex + numViewcould overflow - No validation of input parameters
- Inefficient continuation after reaching the end of deposits
Consider applying this safer implementation:
- for i := startIndex; i < startIndex+numView; i++ {
+ // Prevent integer overflow
+ if numView > 0 && startIndex > math.MaxUint64-numView {
+ return nil, fmt.Errorf("index overflow: startIndex %d + numView %d", startIndex, numView)
+ }
+
+ endIndex := startIndex + numView
+ for i := startIndex; i < endIndex; i++ {Committable suggestion skipped: line range outside the PR's diff.
| return fmt.Errorf( | ||
| "RangeDB DeleteRange start: %d, end: %d: %w", | ||
| from, to, pruner.ErrInvalidRange, | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Enhanced error reporting for invalid ranges.
The improved error message now includes the actual range values, which will help diagnose deposit store pruning issues mentioned in the PR objectives.
Consider adding a unit test to verify the error message format:
func TestDeleteRange_InvalidRange(t *testing.T) {
db := NewRangeDB(NewDB(t.TempDir()))
err := db.DeleteRange(10, 5)
require.ErrorIs(t, err, pruner.ErrInvalidRange)
require.Contains(t, err.Error(), "start: 10, end: 5")
}| return fmt.Errorf( | ||
| "RangeDB Prune start: %d, end: %d: %w", | ||
| start, end, pruner.ErrInvalidRange, | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider clarifying the error message for adjusted ranges.
The error message shows the range values after start is adjusted by max(start, firstNonNilIndex). This could be confusing when debugging, as the reported start value might differ from the input value.
Consider this improvement to show both original and adjusted values:
-return fmt.Errorf(
- "RangeDB Prune start: %d, end: %d: %w",
- start, end, pruner.ErrInvalidRange,
-)
+return fmt.Errorf(
+ "RangeDB Prune original_start: %d, adjusted_start: %d, end: %d: %w",
+ max(start, db.firstNonNilIndex), start, end, pruner.ErrInvalidRange,
+)Committable suggestion skipped: line range outside the PR's diff.
| // make sure included deposit is already available in deposit store | ||
| require.NoError(t, depositStore.EnqueueDeposits(blkDeposits)) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider enhancing deposit verification error handling.
While the deposit verification step is good and aligns with PR objectives, consider adding more descriptive error messaging to help debug test failures.
- require.NoError(t, depositStore.EnqueueDeposits(blkDeposits))
+ err = depositStore.EnqueueDeposits(blkDeposits)
+ require.NoError(t, err, "failed to enqueue deposits in store before transition")📝 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.
| // make sure included deposit is already available in deposit store | |
| require.NoError(t, depositStore.EnqueueDeposits(blkDeposits)) | |
| // make sure included deposit is already available in deposit store | |
| err = depositStore.EnqueueDeposits(blkDeposits) | |
| require.NoError(t, err, "failed to enqueue deposits in store before transition") | |
| @@ -32,27 +33,32 @@ import ( | |||
| // StateProcessorInput is the input for the state processor for the depinject | |||
| // framework. | |||
| type StateProcessorInput[ | |||
| LoggerT any, | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider specifying a more specific constraint for LoggerT.
Using any as a type constraint for LoggerT may reduce type safety. Consider specifying a more precise interface that defines the required logging methods to enhance type safety and maintainability.
| } | ||
|
|
||
| // ProvideStateProcessor provides the state processor to the depinject | ||
| // framework. | ||
| func ProvideStateProcessor[ | ||
| LoggerT log.AdvancedLogger[LoggerT], |
There was a problem hiding this comment.
Ensure consistent type constraints for LoggerT.
In ProvideStateProcessor, LoggerT is constrained as log.AdvancedLogger[LoggerT], while in StateProcessorInput, it is specified as any. Consistency in type constraints enhances type safety. Consider updating StateProcessorInput to use the same constraint.
Apply this diff to update the constraint in StateProcessorInput:
type StateProcessorInput[
- LoggerT any,
+ LoggerT log.AdvancedLogger[LoggerT],Committable suggestion skipped: line range outside the PR's diff.
| // of deposits. | ||
| deposits := blk.GetBody().GetDeposits() | ||
| index, err := st.GetEth1DepositIndex() | ||
| // Verify that outstanding deposits matches those listed by contract |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix grammatical error in comment
Correct the comment for grammatical accuracy.
Apply this diff:
-// Verify that outstanding deposits matches those listed by contract
+// Verify that outstanding deposits match those listed by contract📝 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.
| // Verify that outstanding deposits matches those listed by contract | |
| // Verify that outstanding deposits match those listed by contract |
| sp.logger.Info( | ||
| "Expected deposit index from payload", depositIndex, | ||
| "deposits withdrawals length", len(deposits), | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clarify logging message for better readability
Improve the clarity of the logging message for better understanding.
Apply this diff to enhance the logging message:
- sp.logger.Info(
- "Expected deposit index from payload", depositIndex,
- "deposits withdrawals length", len(deposits),
- )
+ sp.logger.Info(
+ "Processing deposits starting from index", depositIndex,
+ "number of deposits in payload", len(deposits),
+ )📝 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.
| sp.logger.Info( | |
| "Expected deposit index from payload", depositIndex, | |
| "deposits withdrawals length", len(deposits), | |
| ) | |
| sp.logger.Info( | |
| "Processing deposits starting from index", depositIndex, | |
| "number of deposits in payload", len(deposits), | |
| ) |
| return fmt.Errorf("deposits mismatched, state: %#v, payload: %#v", | ||
| sd, deposits[i], | ||
| ) | ||
| } |
There was a problem hiding this comment.
Avoid logging sensitive data in error messages
Using %#v may expose sensitive internal data. Consider logging minimal identifying information to avoid potential information leakage.
Adjust the error message to include only essential details:
- return fmt.Errorf("deposits mismatched, state: %#v, payload: %#v",
- sd, deposits[i],
- )
+ return fmt.Errorf("deposit mismatch at index %d", 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.
| return fmt.Errorf("deposits mismatched, state: %#v, payload: %#v", | |
| sd, deposits[i], | |
| ) | |
| } | |
| return fmt.Errorf("deposit mismatch at index %d", i) | |
| } |
abi87
force-pushed
the
verify-deposits-against-contract
branch
from
3916018
to
6ce250d
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (9)
beacond/cmd/defaults.go(1 hunks)mod/node-core/pkg/components/interfaces.go(1 hunks)mod/node-core/pkg/components/state_processor.go(5 hunks)mod/state-transition/pkg/core/helpers_test.go(6 hunks)mod/state-transition/pkg/core/state_processor.go(7 hunks)mod/state-transition/pkg/core/state_processor_genesis_test.go(2 hunks)mod/state-transition/pkg/core/state_processor_staking.go(1 hunks)mod/state-transition/pkg/core/state_processor_staking_test.go(4 hunks)mod/state-transition/pkg/core/types.go(2 hunks)
🧰 Additional context used
📓 Learnings (1)
mod/state-transition/pkg/core/state_processor_genesis_test.go (4)
Learnt from: abi87
PR: berachain/beacon-kit#2114
File: mod/state-transition/pkg/core/state_processor_genesis_test.go:82-108
Timestamp: 2024-10-31T22:12:16.428Z
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:79-176
Timestamp: 2024-10-29T22:31:53.888Z
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:42-77
Timestamp: 2024-10-29T22:32:03.106Z
Learning: Documentation comments for type aliases in test code are 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-10-29T22:31:04.468Z
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`.
🔇 Additional comments (26)
mod/state-transition/pkg/core/state_processor_staking_test.go (4)
53-56: LGTM: Store initialization looks good.
The initialization of both kvStore and depositStore aligns with the PR objectives of enabling deposit verification. The error handling is appropriate.
57-64: LGTM: State processor creation properly includes deposit store.
The addition of depositStore parameter enables the state processor to verify deposits, which directly implements the PR objective.
143-145: Enhance error messaging for deposit verification.
The verification step is good, but as noted in a previous review, consider adding more descriptive error messaging to help debug test failures.
122-122: Verify deposit index calculation in production code.
While using len(genDeposits) as the index for new deposits makes sense in this test context, we should verify this matches the production behavior.
✅ Verification successful
Let me gather more context about the deposit index handling to verify this thoroughly.
Deposit index calculation is correctly implemented
The test implementation matches the production behavior:
- Production code maintains deposit index through
GetEth1DepositIndex()state calls - Test verifies that state's deposit index equals
len(genDeposits)after genesis - New deposits correctly use the current length as their index, which aligns with the incremental index tracking in production
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for deposit index calculation patterns in production code
rg -A 5 "deposit.*[Ii]ndex.*=.*" mod/state-transition/pkg/core/
Length of output: 1133
Script:
#!/bin/bash
# Search for deposit index related state management
ast-grep --pattern 'func $_($_ *State) GetEth1DepositIndex() (uint64, error) {
$$$
}'
# Also check the test file for any deposit index setup
rg -B 2 -A 2 "genDeposits" mod/state-transition/pkg/core/state_processor_staking_test.go
Length of output: 1250
beacond/cmd/defaults.go (1)
123-125: LGTM! Parameter changes align with PR objectives.
The changes to ProvideStateProcessor parameters are well-structured:
- Adding
*DepositStoreenables deposit validation against contract events - Moving
*Loggerto first position maintains consistency with other components
Let's verify the integration with related components:
✅ Verification successful
Let me try a different approach to verify the integration by searching for the actual function and its usages.
Let me try one final verification to check the actual implementation and dependencies:
Parameter changes in ProvideStateProcessor are correctly integrated
Based on the verification results:
- The
StateProcessorimplementation inmod/state-transition/pkg/core/state_processor.goshows bothloggerandds(DepositStore) as required dependencies in its constructor. - The provider function correctly passes these dependencies to the constructor in the right order:
Loggerfirst, followed by other parameters includingDepositStore. - The type definitions and interfaces show proper integration of both dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent usage of Logger and DepositStore across components
# Test 1: Check if other components follow similar Logger positioning
echo "Components with Logger as first parameter:"
ast-grep --pattern 'components.Provide$_[$$$, *Logger, $$$]'
# Test 2: Check components using both Logger and DepositStore
echo "Components using both Logger and DepositStore:"
ast-grep --pattern 'components.Provide$_[$$$, *Logger, $$$, *DepositStore, $$$]'
Length of output: 343
Script:
#!/bin/bash
# Search for ProvideStateProcessor function definition and usages
echo "=== Searching for ProvideStateProcessor definition ==="
rg "func ProvideStateProcessor" -A 5
echo -e "\n=== Searching for ProvideStateProcessor invocations ==="
rg "ProvideStateProcessor\(" -A 3
echo -e "\n=== Checking other components with similar parameter patterns ==="
# Look for similar provider functions that might have Logger and DepositStore
rg "func Provide.*\(.*Logger.*DepositStore.*\)" -A 3
Length of output: 1085
Script:
#!/bin/bash
# Check the state processor implementation for Logger and DepositStore usage
echo "=== Checking state processor implementation ==="
rg -A 10 "type StateProcessor\["
echo -e "\n=== Checking state processor constructor ==="
ast-grep --pattern 'func NewStateProcessor[
$$$
] (
$$$
) *StateProcessor[
$$$
]'
echo -e "\n=== Checking for DepositStore usage in state processor ==="
rg -C 3 "DepositStore" mod/node-core/pkg/components/state_processor.go
Length of output: 18468
mod/state-transition/pkg/core/state_processor_genesis_test.go (4)
51-54: LGTM: Proper initialization and error handling.
The initialization of stores and error handling follows Go best practices. The changes align with the PR objective of enabling deposit store access for validation.
58-58: LGTM: Consistent parameter addition.
The addition of the depositStore parameter is consistent with the initialization changes and aligns with the PR's goal of enabling deposit validation.
173-176: LGTM: Consistent test setup for Bartio network.
The changes maintain consistency with the regular initialization test while preserving Bartio-specific test logic.
Also applies to: 180-180
51-54: Verify test coverage for deposit store initialization.
Given the PR objectives mention e2e test failures related to deposit retrieval and store pruning, let's verify the test coverage:
Also applies to: 173-176
✅ Verification successful
Based on the search results, I can now generate the final response as we have comprehensive information about the test coverage and deposit store initialization:
Test coverage for deposit store initialization is adequate
The codebase shows comprehensive test coverage for deposit store initialization and related functionality:
-
initTestStores()is properly used across multiple test files:state_processor_genesis_test.gostate_processor_staking_test.gohelpers_test.go
-
The deposit store functionality is well-tested with:
- Unit tests for store operations
- Integration tests in state processor tests
- E2E tests covering deposit contract interactions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for deposit store initialization and related functions
# Test: Look for other test files that might need similar updates
fd -e go -e _test.go$ | xargs rg "initTestStores|depositStore"
# Test: Check for any remaining TODO or FIXME comments related to deposits
rg -i "TODO|FIXME|XXX" -g "*.go" -A 3 "deposit"
# Test: Look for e2e test files that might be affected
fd -e go -e _e2e_test.go$ | xargs rg "deposit"
Length of output: 51189
mod/state-transition/pkg/core/types.go (1)
132-137: LGTM! The Equals method addition enhances deposit validation.
The new method enables precise deposit comparison, which is crucial for verifying deposits against contract events. This aligns perfectly with the PR's objective of preventing fraudulent validators.
Let's verify the implementation of this method:
✅ Verification successful
The verification results show that the Equals method is already implemented consistently across different types in the codebase, including a proper implementation for Deposit type in mod/consensus-types/pkg/types/deposit.go. The implementation follows a consistent pattern of field-by-field comparison, which is exactly what we'd expect for deposit validation.
The Equals method addition is well-implemented and consistent with existing patterns
The implementation in mod/consensus-types/pkg/types/deposit.go thoroughly compares all relevant fields (Pubkey, Credentials, Amount, Signature, and Index), which aligns perfectly with the PR's objective of preventing fraudulent validators through comprehensive deposit validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Equals method implementation across deposit types
# Look for implementations of the Equals method to ensure consistency
ast-grep --pattern 'func ($RECV $_) Equals($_ $_) bool {
$$$
}'
Length of output: 1701
mod/state-transition/pkg/core/state_processor.go (4)
54-54: LGTM: Type parameter change enables deposit comparison.
The updated type parameter signature for DepositT now includes itself, which aligns with the PR's objective of enabling deposit verification against contract events.
81-82: LGTM: Logger field addition enhances observability.
The addition of the logger field will help provide better insights into the state processing activities, as mentioned in the PR objectives.
148-153: LGTM: Constructor properly handles new dependencies.
The constructor has been updated to accept and initialize both the logger and deposit store fields, maintaining a clean dependency injection pattern.
Also applies to: 168-173
Line range hint 1-500: Verify e2e test coverage for deposit verification.
While the implementation looks solid, the PR mentions existing e2e test failures related to deposit retrieval and store pruning. Given this is a security-critical change preventing fraudulent validators, we should ensure:
- The e2e test failures are addressed
- There are comprehensive tests covering the deposit verification logic
- Edge cases are tested (e.g., empty deposit lists, invalid deposits)
Let's check for relevant test coverage:
✅ Verification successful
Let me gather more information about the test coverage by searching for deposit verification implementation and tests:
Let me search specifically for e2e test failures and deposit verification logic:
Let me search for deposit verification tests and implementation:
Let me search for the e2e test failures and deposit verification tests:
Based on the gathered information, I can now provide a final response:
E2E test failures are unrelated to deposit verification implementation
The changes in this PR are focused on deposit store pruning and do not affect the core deposit verification logic. The e2e test failures mentioned are related to the Kurtosis test environment (marked with "TODO: FIX KURTOSIS BUG" comments) and not the deposit verification functionality. The deposit verification implementation in state_processor_staking.go remains unchanged and properly validates deposits through:
processDeposits- Verifies each deposit matches the local stateprocessDeposit- Handles individual deposit verification and index trackingapplyDeposit- Processes validated deposits into the state
The e2e test TestDepositRobustness comprehensively tests the deposit flow including:
- Deposit transaction creation and mining
- Balance verification
- Node synchronization checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files related to deposit verification
echo "Searching for deposit verification tests..."
rg -l "TestDeposit.*Verification|TestVerify.*Deposit" --type go
# Search for deposit-related test assertions
echo "Searching for deposit verification assertions..."
rg "assert.*Deposit|require.*Deposit" --type go
Length of output: 1867
Script:
#!/bin/bash
# Search for deposit verification implementation
echo "Searching for deposit verification implementation..."
rg -A 5 "func.*[Vv]erify.*[Dd]eposit|func.*[Pp]rocess.*[Dd]eposit" --type go
# Search for deposit-related test files
echo "Searching for deposit test files..."
fd -e go -x grep -l "Test.*Deposit" {}
# Search for deposit-related mocks and test utilities
echo "Searching for deposit test utilities..."
rg "Mock.*Deposit|fake.*Deposit|test.*Deposit" --type go
Length of output: 2702
Script:
#!/bin/bash
# Check e2e test content
echo "Checking e2e test content..."
cat testing/e2e/e2e_staking_test.go
# Search for deposit verification logic in state processor
echo "Searching for deposit verification in state processor..."
rg -B 5 -A 10 "func.*processOperations|func.*processDeposits" mod/state-transition/pkg/core/
# Look for recent test failures in git history
echo "Checking recent test failures..."
git log -p --grep="test.*fail" --grep="e2e.*fail" -i
Length of output: 132423
Script:
#!/bin/bash
# Search for deposit verification tests
echo "Searching for deposit verification tests..."
rg -A 10 "test.*Deposit.*Verification|verify.*deposit" testing/e2e/
# Search for deposit verification implementation
echo "Searching for deposit verification implementation..."
rg -A 10 "verify.*deposit|process.*deposit" mod/state-transition/pkg/core/
# Search for deposit store implementation
echo "Searching for deposit store implementation..."
rg -A 10 "type.*DepositStore|interface.*DepositStore" mod/
Length of output: 21106
Script:
#!/bin/bash
# Search for e2e test content
echo "Checking e2e test content..."
cat testing/e2e/e2e_staking_test.go
# Search for deposit verification in state processor tests
echo "Searching for deposit verification tests..."
rg -A 10 "Test.*Deposit|test.*deposit" mod/state-transition/pkg/core/
# Search for recent test failures in git history
echo "Checking recent test failures..."
git log -p --grep="test.*fail" --grep="e2e.*fail" -i
Length of output: 132374
mod/node-core/pkg/components/interfaces.go (2)
387-388: LGTM: Addition of Equals method aligns with deposit validation requirements.
The new Equals method in the Deposit interface enables comparison between deposit instances, which is crucial for verifying deposits against contract events. This addition directly supports the PR's objective of preventing fraudulent validators and protecting legitimate ones.
Line range hint 1-1000: LGTM: Consistent implementation of Equals method across interfaces.
The addition of the Equals method to both Deposit and Withdrawal interfaces maintains consistency in the codebase. This parallel implementation ensures a uniform approach to comparing both deposits and withdrawals.
mod/node-core/pkg/components/state_processor.go (1)
Line range hint 27-119: LGTM!
The enhancements to logging and deposit handling in the state processor are well-implemented. Adding the Logger and DepositStore fields to StateProcessorInput, along with updating the ProvideStateProcessor function, aligns with the PR objectives to verify deposits against the contract and improve logging visibility. The changes are appropriate and maintain consistency within the codebase.
mod/state-transition/pkg/core/helpers_test.go (9)
35-35: Imports 'noop' logger correctly added.
The import of "github.com/berachain/beacon-kit/mod/log/pkg/noop" allows the test code to use the noop logger, which is appropriate for testing purposes.
44-44: Imports 'depositstore' package appropriately.
The addition of the depositstore import is necessary for managing the deposit store within the tests.
95-95: Adds 'ds' parameter to 'createStateProcessor' function.
Including ds *depositstore.KVStore[*types.Deposit] as a parameter allows the state processor to access the deposit store, aligning with the PR's objective to verify deposits against the contract.
136-136: Consider using a real logger instead of a noop logger.
As previously noted, using noop.NewLogger[any]() may suppress important log outputs during testing. Consider using a real logger that can be configured for different environments to capture meaningful logs.
139-139: Passes deposit store to state processor correctly.
The deposit store ds is appropriately passed to core.NewStateProcessor, ensuring that deposit verification can occur as intended.
161-171: Renames and updates 'initStore' function appropriately.
The function initStore has been renamed to initTestStores, and its return values now include both the beacon database store and the deposit store. This change accurately reflects the function's purpose of initializing multiple test stores.
174-174: Handles error return values correctly after adding deposit store.
The error return now includes an additional nil to account for the deposit store, maintaining proper error handling.
190-190: Consistent error handling in store initialization.
Returns nil, nil, error consistently when failing to load the latest version, ensuring that both stores are accounted for in error scenarios.
195-206: Initializes both beacon and deposit stores successfully.
The updated return statement correctly initializes and returns both the beacon database store and the deposit store, fulfilling the requirements for deposit verification in tests.
| // TestTransitionUpdateValidators shows that when validator is | ||
| // updated (increasing amount), corrensponding balance is updated. |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Fix typo in test comment.
"corrensponding" should be "corresponding"
| kvStore, depositStore, err := initTestStores() | ||
| require.NoError(t, err) | ||
| beaconState := new(TestBeaconStateT).NewFromDB(kvStore, cs) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider implementing a store provider interface.
Since both the block builder and state-transition processor now require deposit store access, consider introducing a common interface or provider pattern to standardize this access. This would:
- Reduce duplication of store initialization logic
- Make it easier to modify store behavior in the future
- Simplify testing by allowing mock implementations
Also applies to: 58-58, 173-176, 180-180
| // DepositStore defines the interface for deposit storage. | ||
| type DepositStore[DepositT any] interface { | ||
| // GetDepositsByIndex returns `numView` expected deposits. | ||
| GetDepositsByIndex( | ||
| startIndex uint64, | ||
| numView uint64, | ||
| ) ([]DepositT, error) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding deposit pruning strategy.
While the interface looks good for deposit retrieval, consider adding methods for deposit pruning to prevent unbounded growth of the store. This is particularly important as the past review comments mention deposit store pruning process issues.
Consider adding these methods to the interface:
- A method to prune old deposits
- A method to get the current size/capacity
- A method to configure retention policy
This would help prevent potential memory issues and align with the mentioned deposit store pruning requirements.
| // ds allows checking payload deposits against the deposit contract | ||
| ds DepositStore[DepositT] |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance deposit store field documentation.
While the comment explains what the field does, it could be more descriptive about its purpose in preventing fraudulent validators and censoring legitimate ones.
Consider updating the comment to:
- // ds allows checking payload deposits against the deposit contract
+ // ds allows verifying payload deposits against the deposit contract events
+ // to prevent fraudulent validators and ensure legitimate validators aren't censored📝 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.
| // ds allows checking payload deposits against the deposit contract | |
| ds DepositStore[DepositT] | |
| // ds allows verifying payload deposits against the deposit contract events | |
| // to prevent fraudulent validators and ensure legitimate validators aren't censored | |
| ds DepositStore[DepositT] |
| // Verify that outstanding deposits matches those listed by contract | ||
| depositIndex, err := st.GetEth1DepositIndex() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| eth1Data, err := st.GetEth1Data() | ||
|
|
||
| stateDeposits, err := sp.ds.GetDepositsByIndex( | ||
| depositIndex+1, | ||
| sp.cs.MaxDepositsPerBlock(), | ||
| ) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| depositCount := min( | ||
| sp.cs.MaxDepositsPerBlock(), | ||
| eth1Data.GetDepositCount().Unwrap()-index, | ||
|
|
||
| deposits := blk.GetBody().GetDeposits() | ||
| sp.logger.Info( | ||
| "Expected deposit index from payload", depositIndex, | ||
| "deposits withdrawals length", len(deposits), | ||
| ) | ||
| _ = depositCount | ||
| // TODO: Update eth1data count and check this. | ||
| // if uint64(len(deposits)) != depositCount { | ||
| // return errors.New("deposit count mismatch") | ||
| // } | ||
|
|
||
| if len(stateDeposits) != len(deposits) { | ||
| return fmt.Errorf("deposits mismatched lengths, state: %d, payload: %d", | ||
| len(stateDeposits), | ||
| len(deposits), | ||
| ) | ||
| } | ||
|
|
||
| for i, sd := range stateDeposits { | ||
| if !sd.Equals(deposits[i]) { | ||
| return fmt.Errorf("deposits mismatched, state: %#v, payload: %#v", | ||
| sd, deposits[i], | ||
| ) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding unit tests for deposit verification logic
To ensure the correctness and reliability of the new deposit verification implemented in processOperations, it's advisable to add unit tests covering various scenarios. This will help catch potential issues early and maintain code quality over time.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (5)
mod/consensus-types/pkg/types/deposit.go(1 hunks)mod/node-core/pkg/components/interfaces.go(1 hunks)mod/node-core/pkg/components/state_processor.go(5 hunks)mod/state-transition/pkg/core/state_processor_staking.go(1 hunks)mod/state-transition/pkg/core/types.go(2 hunks)
🔇 Additional comments (9)
mod/node-core/pkg/components/state_processor.go (2)
75-75: LGTM! Consistent type parameter handling.
The addition of DepositStoreT type parameter and parameter reordering maintains type safety and enables proper type inference.
Also applies to: 90-90
Line range hint 41-55: LGTM! Type-safe deposit store integration.
The addition of DepositT type parameter and DepositStore field aligns with the PR objective to verify deposits against contract events. The type constraints ensure type safety for deposit operations.
Let's verify the deposit store implementation:
✅ Verification successful
The shell script results provide comprehensive information about the DepositStore interface and its usage across the codebase. Let me generate the final response:
Verified: DepositStore integration is properly implemented and consistently used
The DepositStore interface is well-defined in multiple modules with consistent signatures:
- Core interface in
mod/state-transition/pkg/core/types.go - Implementation provider in
mod/node-core/pkg/components/deposit_store.go - Consistent usage across state processor, validator service, and other components
- Type-safe generic constraints maintained throughout the dependency chain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify deposit store implementation and usage
# Look for deposit store interface definition and other implementations
ast-grep --pattern 'type DepositStore[T any] interface {
$$$
}'
# Look for deposit store usage patterns
rg -A 5 'DepositStore\['
Length of output: 12216
mod/consensus-types/pkg/types/deposit.go (3)
189-190: Add security context to the documentation.
Given this method's critical role in deposit validation, enhance the documentation to explain its security implications:
-// Equals returns true if the Deposit is equal to the other.
+// Equals performs a secure comparison between two Deposit instances.
+// This method is crucial for validating deposits against contract events
+// to prevent fraudulent validator creation and ensure legitimate validators
+// aren't censored. It compares all fields including cryptographic data
+// (Pubkey, Credentials, Signature) and metadata (Amount, Index).191-195:
Use constant-time comparison for cryptographic fields.
The current implementation uses direct comparison (==) for cryptographic fields, which could be vulnerable to timing attacks. Use subtle.ConstantTimeCompare for secure comparison of Pubkey, Credentials, and Signature.
Apply this diff to implement constant-time comparison:
+import "crypto/subtle"
func (d *Deposit) Equals(rhs *Deposit) bool {
- return d.Pubkey == rhs.Pubkey &&
- d.Credentials == rhs.Credentials &&
- d.Amount == rhs.Amount &&
- d.Signature == rhs.Signature &&
+ return subtle.ConstantTimeCompare(d.Pubkey[:], rhs.Pubkey[:]) == 1 &&
+ subtle.ConstantTimeCompare(d.Credentials[:], rhs.Credentials[:]) == 1 &&
+ d.Amount == rhs.Amount &&
+ subtle.ConstantTimeCompare(d.Signature[:], rhs.Signature[:]) == 1 &&
d.Index == rhs.Index
}This change is essential because:
- Prevents timing attacks that could exploit variations in comparison time
- Aligns with cryptographic best practices for handling sensitive data
- Critical for the PR's objective of preventing fraudulent validators
189-196: Verify the equality comparison usage in state transition.
The implementation looks functionally correct, but let's verify how it's being used in the state transition process to ensure it properly validates deposits against contract events.
✅ Verification successful
Based on the search results, I can now generate the final response as we have found the key usage patterns of the Deposit.Equals method in state transition validation.
The Equals implementation is correctly used in state transition validation
The implementation is verified to be properly used in state transition validation, specifically in mod/state-transition/pkg/core/state_processor_staking.go where it compares deposits from state against payload deposits:
if !sd.Equals(deposits[i]) {
return fmt.Errorf("deposits mismatched, state: %#v, payload: %#v",
sd, deposits[i],
)
}This validates that:
- The implementation compares all fields (pubkey, credentials, amount, signature, index)
- It's used to ensure deposit consistency between state and payload
- The comparison is part of the state transition validation logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of Deposit.Equals in state transition validation
# Expected: Find calls to Equals method in state transition code that compare
# deposits from payload against contract events
# Search for Deposit.Equals usage in state transition code
echo "Searching for Deposit.Equals usage in state transition..."
rg -p "Equals.*\*Deposit" ./mod/state-transition
# Search for deposit validation logic
echo "Searching for deposit validation logic..."
rg -p "validate.*deposit" ./mod/state-transition
Length of output: 319
Script:
#!/bin/bash
# Let's try a broader search to understand deposit validation patterns
# Search for any usage of Deposit type and validation
echo "Searching for Deposit type usage..."
rg -p "type.*Deposit" -A 5
# Search for deposit validation patterns with broader context
echo "Searching for deposit validation patterns..."
rg -p "deposit.*validate|validate.*deposit" -i -A 5
# Search for deposit comparison patterns
echo "Searching for deposit comparison patterns..."
rg -p "deposit.*equal|equal.*deposit" -i -A 5
# Search for files containing both deposit and validation
echo "Files containing both deposit and validation..."
rg -l "deposit" | xargs rg -l "validate"
Length of output: 43617
mod/state-transition/pkg/core/types.go (2)
132-137: LGTM: Clean addition of Equals method
The addition of the Equals method to the Deposit interface is well-designed and type-safe. This enhancement aligns with the PR objectives by enabling deposit verification against contract events.
155-162: 🛠️ Refactor suggestion
Consider enhancing DepositStore interface for better deposit management
While the GetDepositsByIndex method is well-designed for deposit verification, the interface could benefit from additional methods to ensure robust deposit management:
-
Methods for deposit lifecycle management:
type DepositStore[DepositT any] interface { // Existing method GetDepositsByIndex(startIndex uint64, numView uint64) ([]DepositT, error) // Suggested additions PruneDeposits(beforeIndex uint64) error GetDepositCount() (uint64, error) GetOldestDepositIndex() (uint64, error) }
-
Consider adding methods for deposit store configuration:
type DepositStoreConfig interface { GetRetentionPolicy() uint64 // Number of blocks to retain GetMaxDepositCount() uint64 // Maximum number of deposits to store }
This enhancement would help prevent unbounded growth and align with the deposit store pruning requirements mentioned in the PR objectives.
mod/node-core/pkg/components/interfaces.go (1)
386-387: LGTM: Addition of Equals method enhances deposit verification capabilities.
The addition of the Equals method to the Deposit interface is well-designed and aligns with the PR's objective of verifying deposits against contract events. The generic type parameter ensures type safety.
mod/state-transition/pkg/core/state_processor_staking.go (1)
43-77: Deposit verification logic implemented correctly
The implementation of deposit verification in processOperations strengthens the validation process by ensuring that deposits in the block match those recorded in the deposit store. The comparison logic and error handling are appropriately handled.
| in.DepositStore, | ||
| in.Signer, | ||
| crypto.GetAddressFromPubKey, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider enhancing logging for deposit verification.
The implementation correctly integrates the deposit store. Given the PR objectives mention improving logging capabilities, consider adding structured logging fields for deposit verification outcomes.
Add logging fields to track deposit verification:
- in.Logger.With("service", "state-processor"),
+ in.Logger.With(
+ "service", "state-processor",
+ "features", []string{"deposit-verification"},
+ ),Committable suggestion skipped: line range outside the PR's diff.
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:
Moreover it adds much needed logs to state processor (to be filled in time)
Build on top of #2114
Note:
and several like
Summary by CodeRabbit
Release Notes
New Features
DepositandWithdrawalinstances.DepositStoreinterface for managing deposits.Improvements
Bug Fixes
Tests
These updates enhance the overall functionality and robustness of the deposit management system.