feat: use memstore instead of block wise transient store #117
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request primarily focus on modifications to the handling of transient data within various components, particularly the Changes
Possibly related PRs
Suggested reviewers
Warning Rate limit exceeded@beer-1 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 9 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (11)
x/evm/state/context.go (2)
15-22: Consider adding error handling for memory store initializationThe
newMemStore()call might fail (e.g., due to resource constraints). Consider propagating any potential errors to the caller for proper error handling.-func NewContext(ctx sdk.Context) Context { +func NewContext(ctx sdk.Context) (Context, error) { - memStore, memStoreKey := newMemStore() + memStore, memStoreKey, err := newMemStore() + if err != nil { + return Context{}, fmt.Errorf("failed to create memory store: %w", err) + } return Context{ Context: ctx, memStore: memStore, memStoreKey: memStoreKey, - } + }, nil }
34-45: Add documentation for CacheContext methodThe implementation looks good, but please add documentation explaining:
- The purpose of caching both SDK context and memory store
- The behavior of the writeCache function
- When changes are actually committed
Add documentation like:
// CacheContext creates a cached copy of the context and returns it along with a // write function. The write function atomically commits all cached changes to both // the SDK context and memory store when called. If the write function is not called, // all cached changes are discarded. func (c Context) CacheContext() (cc Context, writeCache func()) {x/evm/state/keys.go (1)
41-42: Consider adding documentation about key space separationThe file manages two distinct key spaces (permanent and transient storage) with different prefix schemes. Consider adding a package-level comment to document this separation and the rationale.
Add this documentation at the package level:
package state import ( "encoding/binary" "github.com/ethereum/go-ethereum/common" ) +// This package manages two distinct key spaces: +// 1. Permanent storage: Uses string-based prefixes ("code", "codesize", "state") +// 2. Memory/transient storage: Uses single-byte prefixes (0x01-0x07) +// This separation ensures no key collisions between permanent and transient data. + var (x/evm/types/keys.go (1)
Based on the search results, I can now provide a final response:
The TStoreKey constant can be safely removed
The search results confirm that:
- TStoreKey is only referenced in test files (
common_test.gofiles)- The actual transient store functionality has been replaced with memstore collections as seen in
x/evm/state/statedb.go- No production code is using the transient store key
🔗 Analysis chain
Line range hint
8-11: Consider removing unused TStoreKey constantSince we're moving away from transient storage to memstore, the
TStoreKeyconstant might no longer be needed.Let's verify if this constant is still in use:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if TStoreKey is still being used # Search for TStoreKey usage echo "Checking TStoreKey usage..." rg "TStoreKey" # Search for transient store initialization patterns echo "Checking store initialization patterns..." ast-grep --pattern 'NewTransient$_($_, $TStoreKey)'Length of output: 3046
Script:
#!/bin/bash # Let's search specifically for TStoreKey usage echo "Checking TStoreKey usage..." rg "TStoreKey" # Also check for any transient store usage patterns echo "Checking transient store usage..." rg "TransientStore|GetTransient" # Check if there are any other files importing the types package and using transient storage echo "Checking imports of evm types..." rg -p "\".*x/evm/types\"" -A 5Length of output: 21032
x/evm/state/memdb.go (3)
14-16: Consider using a more specific constant nameThe constant
storeKeyis quite generic. Consider using a more specific name likememStoreKeyorevmStateKeyto better indicate its purpose and avoid potential naming conflicts.- storeKey = "stateDB" + memStoreKey = "evmStateDB"
18-27: Consider enhancing error handling and configurationWhile using panic for initialization errors is acceptable, there are several potential improvements:
- The error message could be more descriptive about what caused the failure
- Consider accepting configuration parameters (e.g., logger, metrics) instead of hardcoding them
-func newMemStore() (storetypes.MultiStore, storetypes.StoreKey) { +func newMemStore(logger log.Logger, metrics storemetrics.Metrics) (storetypes.MultiStore, storetypes.StoreKey) { memStoreKey := storetypes.NewMemoryStoreKey(storeKey) - memStore := store.NewCommitMultiStore(dbm.NewMemDB(), log.NewNopLogger(), storemetrics.NewNoOpMetrics()) + memStore := store.NewCommitMultiStore(dbm.NewMemDB(), logger, metrics) memStore.MountStoreWithDB(memStoreKey, storetypes.StoreTypeMemory, nil) if err := memStore.LoadLatestVersion(); err != nil { - panic(fmt.Sprintf("failed to initialize memory store: %v", err)) + panic(fmt.Sprintf("failed to initialize memory store: failed to load latest version: %v", err)) }
1-80: Consider performance and persistence implicationsWhile using a memory store instead of a transient store might help diagnose app hash crashes, consider:
- Memory usage implications for large datasets
- Data persistence requirements across node restarts
- Impact on state sync and snapshots
Would you like assistance in implementing monitoring or metrics to track memory usage?
x/evm/precompiles/erc20_registry/common_test.go (1)
Line range hint
28-33: Add documentation for the context conversion.The constructor correctly handles the conversion from
sdk.Contexttostate.Context. Consider adding a comment explaining the conversion for better maintainability.func NewMockStateDB(sdkCtx sdk.Context) *MockStateDB { + // Convert SDK context to state.Context for EVM state management ctx := state.NewContext(sdkCtx) return &MockStateDB{ ctx: ctx, initialCtx: ctx, } }x/evm/state/statedb.go (3)
Line range hint
643-656: Error handling inCommitfunction may lead to data inconsistencyIn the
Commitfunction, errors returned froms.vmStore.Clearands.accountKeeper.RemoveAccountare not handled explicitly within theWalkclosure. Since theWalkfunction continues even if an error occurs, this may lead to incomplete cleanup of destructed accounts.Modify the closure to handle errors properly and stop walking when an error occurs:
err := s.memStoreSelfDestruct.Walk(s.ctx, nil, func(key []byte) (stop bool, err error) { addr := common.BytesToAddress(key) // If ether was sent to account post-selfdestruct it is burnt. if bal := s.GetBalance(addr); bal.Sign() != 0 { s.SubBalance(addr, bal, tracing.BalanceDecreaseSelfdestructBurn) } - err = s.vmStore.Clear(s.ctx, new(collections.Range[[]byte]).Prefix(addr.Bytes())) + if err := s.vmStore.Clear(s.ctx, new(collections.Range[[]byte]).Prefix(addr.Bytes())); err != nil { + return true, err + } // remove cosmos account - s.accountKeeper.RemoveAccount(s.ctx, s.accountKeeper.GetAccount(s.ctx, sdk.AccAddress(addr.Bytes()))) + acc := s.accountKeeper.GetAccount(s.ctx, sdk.AccAddress(addr.Bytes())) + if acc != nil { + s.accountKeeper.RemoveAccount(s.ctx, acc) + } + return false, nil }) if err != nil { return err }This adjustment ensures that errors are not ignored and that the function exits gracefully if an error occurs during account cleanup.
Line range hint
217-223: Handling errors by panicking inAddAddressToAccessListThe function
AddAddressToAccessListusespanicto handle errors frommemStoreAccessList.Set. Reliance on panicking for error handling can lead to abrupt termination of the application, which is undesirable in production environments.Refactor the function to return an error instead of panicking:
-func (s *StateDB) AddAddressToAccessList(addr common.Address) { - err := s.memStoreAccessList.Set(s.ctx, addr.Bytes()) - if err != nil { - panic(err) - } +func (s *StateDB) AddAddressToAccessList(addr common.Address) error { + return s.memStoreAccessList.Set(s.ctx, addr.Bytes()) }Update the function's callers to handle the error appropriately. This approach enhances the robustness of the application by allowing controlled error propagation and handling.
Based on the code analysis and EIP-6780 requirements, I can now provide the final response:
Incomplete implementation of EIP-6780: Missing balance transfer to beneficiary
The current implementation of
Selfdestruct6780only checks if the contract was created in the same transaction and callsSelfDestructaccordingly. However, it's missing critical requirements from EIP-6780:
When called in the same transaction as creation (current implementation):
- Correctly deletes the account via
SelfDestruct- But missing: Transfer of balance to beneficiary before deletion
When called in different transactions:
- Correctly skips deletion
- But missing: Required transfer of entire balance to beneficiary
The implementation needs to be updated to handle balance transfers in both cases, as per EIP-6780 specification.
🔗 Analysis chain
Line range hint
519-523: Incomplete implementation of EIP-6780 inSelfdestruct6780The
Selfdestruct6780function only callsSelfDestructif the account was created in the same transaction. However, according to EIP-6780, additional considerations might be required for self-destruction behavior.Please verify that the implementation fully complies with EIP-6780 specifications.
Run the following script to check for compliance details:
Let me check the implementation details of
Selfdestruct6780andSelfDestructto verify compliance with EIP-6780.
Let me check if there's any balance transfer to beneficiary implementation, as that's a key requirement of EIP-6780.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `Selfdestruct6780` aligns with EIP-6780. # Fetch EIP-6780 details curl -s https://eips.ethereum.org/EIPS/eip-6780 | grep -i "selfdestruct" # Analyze the implementation for completeness # [Manual analysis required]Length of output: 8155
Script:
#!/bin/bash # Get implementation of Selfdestruct6780 and SelfDestruct ast-grep --pattern 'func (s $_) Selfdestruct6780($_) { $$$ }' echo "---" ast-grep --pattern 'func (s $_) SelfDestruct($_) { $$$ }' # Check for balance transfer implementation rg -A 10 "func.*SelfDestruct"Length of output: 6517
Script:
#!/bin/bash # Search for balance transfer related functions and their usage rg -A 5 "func.*(AddBalance|SubBalance|TransferBalance)" x/evm/state/ # Check if there's any beneficiary handling rg -B 3 -A 5 "beneficiary" x/evm/Length of output: 795
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (17)
app/ibc-hooks/common_test.go(0 hunks)app/ibc-hooks/receive_test.go(1 hunks)app/keepers/keepers.go(0 hunks)app/keepers/keys.go(1 hunks)x/bank/keeper/common_test.go(0 hunks)x/evm/keeper/common_test.go(0 hunks)x/evm/keeper/context.go(2 hunks)x/evm/keeper/keeper.go(0 hunks)x/evm/precompiles/cosmos/common_test.go(2 hunks)x/evm/precompiles/erc20_registry/common_test.go(2 hunks)x/evm/state/common_test.go(0 hunks)x/evm/state/context.go(1 hunks)x/evm/state/keys.go(1 hunks)x/evm/state/memdb.go(1 hunks)x/evm/state/snapshot.go(2 hunks)x/evm/state/statedb.go(17 hunks)x/evm/types/keys.go(1 hunks)
💤 Files with no reviewable changes (6)
- app/ibc-hooks/common_test.go
- app/keepers/keepers.go
- x/bank/keeper/common_test.go
- x/evm/keeper/common_test.go
- x/evm/keeper/keeper.go
- x/evm/state/common_test.go
🔇 Additional comments (24)
x/evm/state/snapshot.go (3)
Line range hint 8-13: Verify caching behavior and state isolation
The constructor now uses the custom Context's CacheContext() method. While the commit mechanism appears unchanged, we should ensure that:
- State changes remain isolated until explicitly committed
- The cache context provides the same atomicity guarantees as before
✅ Verification successful
Let's search for test cases and implementation patterns to verify the caching behavior and state isolation mechanisms.
Based on the search results, I can now generate the final response as we have sufficient evidence about the caching behavior:
Cache context implementation maintains proper state isolation
The implementation correctly preserves state isolation through the following mechanisms:
- The
CacheContext()method inx/evm/state/context.goproperly wraps both the SDK context and memstore with cache layers - The commit function is correctly captured and only executed when explicitly called
- Usage patterns across the codebase (in keepers, hooks, and tests) demonstrate consistent cache-then-commit patterns with proper error handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for tests verifying cache isolation
rg -A 5 'TestSnapshot.*Cache' --type go
rg -A 5 'TestContext.*Isolation' --type go
# Check for any existing issues with cache contexts
rg -l 'CacheContext.*[Bb]ug|[Ii]ssue|[Ff]ix' --type go
Length of output: 213
Script:
#!/bin/bash
# Search for snapshot tests and their implementation
rg -A 10 'func TestSnapshot' --type go
# Look for cache context usage patterns
rg -A 5 'CacheContext\(\)' --type go
# Check snapshot commit patterns
rg -A 5 'snapshot.*commit' --type go
# Look for any cache-related tests
rg -A 5 'func Test.*Cache' --type go
Length of output: 4629
21-21: Verify test coverage and mock usage
The Context() method's return type change affects mock testing scenarios. Ensure all test files have been updated to handle the new Context type correctly.
✅ Verification successful
The previous script had syntax errors. Let's try with a more comprehensive search to find any relevant usage patterns of the Context() method and related test files.
Based on the search results, I can now provide a final response:
Test coverage is properly maintained for Context() method
The codebase shows consistent handling of the Context() method change:
- Mock implementations in test files (e.g.,
x/evm/precompiles/erc20_registry/common_test.go,x/evm/precompiles/cosmos/common_test.go) have been updated to use the new Context type - The change is properly reflected in the interface definition (
x/evm/types/expected_keeper.go) - No stray references to the old context type usage were found in test files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for test files using this method
rg -l 'Context\(\).*mock' --type go
rg -l 'snapshot\.Context\(\)' --type go
# Check for any remaining references to sdk.Context in tests
rg 'sdk\.Context.*snapshot' --type go
Length of output: 213
Script:
#!/bin/bash
# Look for test files using Context() method
rg "Context\(\)" --type go -l
# Look for test files specifically
fd ".*_test\.go$" -x rg "Context\(\)" {}
# Check for any references to sdk.Context
rg "sdk\.Context" --type go
# Look for mock usage
rg "mock.*Context" --type go
Length of output: 17736
4-4: Verify thread-safety and isolation guarantees of the new Context type
The change from sdk.Context to a custom Context type is significant. While this aligns with the goal of eliminating block-wise transient storage, we should ensure the new type maintains proper state isolation.
✅ Verification successful
Based on the search results, I can now provide a conclusive response about the Context type change:
Custom Context type maintains proper state isolation and thread safety
The custom Context type in x/evm/state/context.go is a wrapper around sdk.Context that adds EVM-specific functionality:
- It embeds
sdk.Contextto inherit all its thread-safety properties - Adds memory store capabilities for EVM state management
- Provides clean separation between SDK and EVM-specific context needs
The change is safe and appropriate for the following reasons:
- Maintains compatibility with SDK context functionality
- Adds necessary EVM-specific storage without compromising thread safety
- Follows the pattern of extending rather than replacing core functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Context implementation to verify its capabilities
ast-grep --pattern 'type Context struct {
$$$
}'
# Look for any concurrent access patterns or locks
rg -l 'sync\.' --type go
rg -l 'mutex' --type go -i
Length of output: 213
Script:
#!/bin/bash
# Let's try a different approach to find the Context definition and usage
# Find the Context type definition
rg -A 5 "type Context" --type go
# Find files importing sdk.Context and our custom Context
rg "sdk\.Context|\"Context\"" --type go
# Look for any methods or functions using Context
ast-grep --pattern 'func $_($_Context$_) $_'
Length of output: 14675
x/evm/state/context.go (4)
8-13: LGTM! Well-structured Context definition
The Context struct is well-designed:
- Properly embeds sdk.Context for standard context functionality
- Adds memory store capabilities aligned with the PR objective of moving away from transient storage
- Clear and consistent field naming
24-32: LGTM! Clean and idiomatic method implementations
The WithSDKContext and WithMemStore methods follow the builder pattern correctly and maintain immutability.
1-45: Verify integration with existing state management
The move from transient store to memory store is a significant architectural change. Please ensure:
- Memory consumption is monitored and managed appropriately
- State consistency is maintained across all EVM operations
- Performance impact is measured, especially for large state changes
#!/bin/bash
# Search for transient store usage that might need migration
rg -l "TransientStore|TStore" --type go
# Look for existing state management patterns
ast-grep --pattern 'type $_ struct {
$$$
store $_ $$$
$$$
}'15-17: Verify the implementation of newMemStore()
The code calls newMemStore() but this function is not visible in the current file or imports. Please ensure this function is properly implemented and accessible.
✅ Verification successful
Let me try a different search pattern to locate the newMemStore function.
newMemStore() is properly implemented in x/evm/state/memdb.go
The function is correctly implemented in a separate file within the same package. It creates and initializes a memory store with the necessary configuration, returning both the store and its key as expected by the NewContext constructor.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for newMemStore implementation
ast-grep --pattern 'func newMemStore() ('
Length of output: 297
Script:
#!/bin/bash
# Search for newMemStore implementation with rg
rg -A 5 'func newMemStore\(\)' --type go
# Also search in the same file for any memory store related code
rg -A 5 'memStore|MemStore' x/evm/state/context.go
Length of output: 1393
x/evm/state/keys.go (1)
43-49: LGTM! Space-efficient prefix design
The new memstore prefixes are well-designed using single bytes (0x01-0x07), which is space-efficient and ensures no collisions with existing string-based prefixes. Each prefix has a clear purpose documented in the comments.
x/evm/types/keys.go (2)
23-23: Verify the migration from transient store to memstore
The removal of transient store prefixes aligns with the PR objective to use memstore instead of block-wise transient store. However, we should verify that all transient store operations have been properly migrated.
Let's verify the complete removal of transient store operations:
23-23: Verify VM state persistence strategy
With the removal of transient prefixes and the retention of only VMStorePrefix, we need to ensure that the VM state management strategy is properly implemented.
Let's verify the VM state handling:
#!/bin/bash
# Description: Check VM state handling implementation
# Check for VMStorePrefix usage
echo "Checking VMStorePrefix usage..."
rg -A 5 "VMStorePrefix"
# Check for state store operations in keeper
echo "Checking state store operations..."
ast-grep --pattern 'func ($k Keeper) $method($ctx $_) {
$store := $ctx.KVStore($k.storeKey)
$$$
}'x/evm/state/memdb.go (1)
63-71: Document iterator behavior and add validation
The iterator methods have important contracts about concurrent writes, but:
- The behavior when start > end is not handled
- The comment about cachekv.Store seems out of place in this context
Let's verify if this is the standard behavior in other stores:
Also applies to: 73-80
app/keepers/keys.go (2)
Line range hint 1-100: Consider cleaning up unused imports.
The evmtypes package is still imported but its TStoreKey is no longer used in this file. However, since this is part of a larger refactoring effort, let's verify if this import is still needed elsewhere in the file before removing it.
#!/bin/bash
# Check if evmtypes is used for anything else in this file
rg "evmtypes\." app/keepers/keys.go60-60: LGTM! Verify transient store removal impact.
The removal of evmtypes.TStoreKey from transient store keys aligns with the PR's objective to move away from block-wise transient storage. This is a significant architectural change.
Let's verify that no code still expects EVM data in the transient store:
✅ Verification successful
The previous search might have been too specific. Let's broaden our search to:
- Find all transient store usage
- Look for store access patterns that might indicate EVM transient store dependencies
The search results show EVM transient store keys are still present in test files and type definitions. Let's verify if there are any actual usages in the keeper implementation and check recent changes to understand the new storage mechanism.
Transient store removal is properly implemented with memory store replacement
The verification confirms that the EVM transient store removal is safe and properly implemented:
- The transient store keys are completely removed from the EVM module
- A new memory store implementation replaces the transient store functionality:
- All transient state is now managed via
memStoreinstate/memdb.go - The memory store provides the same capabilities but without persistence
- All transient state is now managed via
- The changes are comprehensive and consistent across the codebase:
- Keeper initialization updated to remove transient store dependency
- StateDB implementation modified to use memory store
- Test files updated accordingly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to EVM transient store usage
rg "TStoreKey|TransientStoreKey.*evm" -A 3
Length of output: 197
Script:
#!/bin/bash
# Let's search more broadly for any references to EVM transient store
rg "TransientStoreKey|TStoreKey" -A 3
# Also search for any EVM-related store access patterns
ast-grep --pattern 'store.Get($$$)'
Length of output: 3345
Script:
#!/bin/bash
# Let's check if there are any remaining EVM transient store usages in keeper implementations
rg "ctx.*Store.*TStore|TransientStore.*evm" -A 3
# Also check for any recent changes to EVM-related files that might indicate the new storage mechanism
git diff origin/main -- "x/evm/**"
Length of output: 31107
x/evm/precompiles/erc20_registry/common_test.go (2)
21-22: LGTM! Verify state.Context usage across the codebase.
The change from sdk.Context to state.Context is consistent with the PR's objective. Both context fields are properly updated.
#!/bin/bash
# Verify consistent usage of state.Context across the codebase
ast-grep --pattern 'type $_ struct {
$$$
ctx $ctx_type
$$$
}'71-74: LGTM! Verify StateDB interface compliance.
The context accessor methods correctly return the underlying sdk.Context from the state.Context wrapper, maintaining interface compatibility.
Also applies to: 79-79
x/evm/precompiles/cosmos/common_test.go (3)
Line range hint 34-39: LGTM! Clear and correct constructor implementation
The changes improve clarity by:
- Renaming the parameter to clearly distinguish between SDK and state contexts
- Properly initializing both context fields with the same state context
77-85: Consider using Context getter method if available
The implementation correctly returns the underlying SDK context, but directly accessing the Context field might bypass any necessary state management in the state.Context type. Consider using a getter method if one is available in the state.Context type.
#!/bin/bash
# Description: Check if state.Context has a getter method for the underlying SDK context
# Search for Context getter method in state.Context
ast-grep --pattern 'func ($receiver state.Context) $name() sdk.Context {
$$$
}'27-28: Verify context type consistency with production code
The change from sdk.Context to state.Context for both context fields aligns with the PR's objective. However, we should verify that this matches the production implementation.
✅ Verification successful
Based on the search results, I can now provide a definitive response:
Context type change is consistent with production code
The change from sdk.Context to state.Context in the test file aligns with the production implementation. The search results show that:
- The production
StateDBstruct inx/evm/state/statedb.gousesstate.Contextfor bothctxandinitialCtxfields - The mock implementation in the test file correctly mirrors this structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the context type change is consistent across the codebase
# Look for any remaining sdk.Context usage in StateDB implementations
# Search for StateDB implementations
ast-grep --pattern 'type $name struct {
$$$
ctx $type
$$$
}'
# Search for any remaining sdk.Context usage in state-related files
rg "sdk\.Context" "x/evm/state"
Length of output: 508
Script:
#!/bin/bash
# Let's try a different approach to verify the context type changes
# Search for StateDB struct definitions
ast-grep --pattern 'type StateDB struct {
$$$
}'
# Search for context-related fields in state-related files
rg "ctx\s+\w+\.Context" "x/evm"
# Search for any remaining sdk.Context usage in EVM-related files
rg "sdk\.Context" "x/evm"
Length of output: 43162
app/ibc-hooks/receive_test.go (2)
Line range hint 1-324: Test coverage looks comprehensive
The test suite thoroughly covers:
- ICS20 and ICS721 packet handling
- Success and failure scenarios
- ACL verification
- Contract state validation
- Token allowance checks
113-113: Verify the behavior of DenomToContractAddr remains consistent
The change to use evmtypes.DenomToContractAddr aligns with the PR's objective of moving away from block-wise transient store. Let's verify the implementation to ensure consistent behavior.
✅ Verification successful
Function behavior is consistent and properly implemented
The change to use evmtypes.DenomToContractAddr is correct and safe. The implementation in x/evm/types/denom.go shows that the function:
- Handles EVM denom prefix correctly
- Validates hex addresses
- Falls back to keeper for non-EVM denoms
- Maintains consistent behavior with proper error handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of DenomToContractAddr in evmtypes
# Check the implementation and its dependencies
ast-grep --pattern 'func DenomToContractAddr($ctx, $keeper, $denom) ($result, $err) {
$$$
}'
# Look for any related tests
rg -l "DenomToContractAddr.*Test"
Length of output: 132
Script:
#!/bin/bash
# Let's try a broader search to locate the DenomToContractAddr function and its usage
# Search for DenomToContractAddr function definition
rg -A 5 "func.*DenomToContractAddr"
# Search for any imports of evmtypes package
rg "\".*evmtypes\""
# Search for any usage of DenomToContractAddr
rg "DenomToContractAddr"
Length of output: 2769
Script:
#!/bin/bash
# Let's examine the implementation details and any recent changes
# Check the full implementation of DenomToContractAddr
cat x/evm/types/denom.go
# Look for any recent changes to this file
git log -p x/evm/types/denom.go
Length of output: 3013
x/evm/keeper/context.go (4)
30-31: Confirm the use of an infinite gas meter in NewStateDB
Using storetypes.NewInfiniteGasMeter() in NewStateDB may lead to uncontrolled gas consumption. Ensure that this is intentional and that gas is properly accounted for elsewhere to prevent potential Denial-of-Service attacks.
159-159: Preparation of SDK context appears appropriate
The call to prepareSDKContext(sdk.UnwrapSDKContext(ctx)) correctly unwraps the context for SDK usage. This ensures that the EVM operates with the proper context settings.
178-184: Verify initialization of EVM with nil StateDB
Initializing the EVM with nil as the StateDB parameter may lead to nil pointer dereferences if accessed before proper assignment. Confirm that the EVM instance does not interact with the StateDB until it is set later in the code.
195-199: Setting precompiles aligns with updated context and StateDB
Assigning precompiles after initializing the EVM's context and StateDB ensures compatibility with the current configuration. This approach appears correct and consistent with the overall code structure.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #117 +/- ##
==========================================
+ Coverage 40.56% 40.70% +0.13%
==========================================
Files 106 108 +2
Lines 9923 9963 +40
==========================================
+ Hits 4025 4055 +30
- Misses 5322 5330 +8
- Partials 576 578 +2
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
x/evm/keeper/fuzz_test.go (2)
83-83: Remove redundant error checkThere's an unnecessary error check here with no preceding error-generating operation.
- require.NoError(t, err)
74-87: Document assumptions about log ordering and data structureThe function makes several implicit assumptions:
- Logs are ordered by iteration index (used in line 84-85)
- Log data is exactly 64 bytes (checked in line 79)
Consider adding documentation to explain these requirements and their relationship to the contract's behavior.
+// assertLogs validates the event logs emitted by the counter contract. +// It expects: +// - Logs to be ordered by iteration index +// - Each log to contain exactly 64 bytes of data: +// - First 32 bytes: counter value before increment +// - Last 32 bytes: counter value after increment func assertLogs(t *testing.T, contractAddr common.Address, logs []types.Log) {x/evm/state/statedb_test.go (2)
91-98: LGTM! Consider adding intermediate snapshot assertions.The additional assertions effectively verify state consistency across multiple snapshots. This is particularly valuable for ensuring the reliability of the memstore implementation.
Consider adding assertions immediately after the first
Snapshot()call to verify the intermediate state, making the test progression more explicit:stateDB.Snapshot() stateDB.SubRefund(10) + // Verify intermediate snapshot state + require.Equal(t, uint64(90), stateDB.GetBalance(common.BytesToAddress(addr1)).Uint64()) + require.Equal(t, uint64(80), stateDB.GetRefund()) require.Equal(t, uint64(90), stateDB.GetBalance(common.BytesToAddress(addr1)).Uint64()) require.Equal(t, uint64(80), stateDB.GetRefund())
Line range hint
1-489: Consider adding app hash consistency tests.Given that this PR aims to address app hash crashes on the query node, it would be valuable to add specific test cases that verify app hash consistency across state operations, particularly during snapshot and revert operations.
Consider adding a new test function that:
- Performs a series of state modifications
- Takes snapshots at various points
- Verifies that the app hash remains consistent after reverting to different snapshots
- Ensures the app hash is correctly maintained when switching between transient and persistent storage
Example structure:
func Test_AppHashConsistency(t *testing.T) { sdkCtx, input := createDefaultTestInput(t) // Get initial app hash initialHash := input.EVMKeeper.GetAppHash(sdkCtx) // Perform state modifications and verify app hash _, evm, err := input.EVMKeeper.CreateEVM(sdkCtx, evmtypes.StdAddress, nil) require.NoError(t, err) stateDB := evm.StateDB.(*state.StateDB) // Take snapshot sid := stateDB.Snapshot() // Modify state // ... // Verify app hash after modifications modifiedHash := input.EVMKeeper.GetAppHash(sdkCtx) require.NotEqual(t, initialHash, modifiedHash) // Revert to snapshot stateDB.RevertToSnapshot(sid) // Verify app hash is restored finalHash := input.EVMKeeper.GetAppHash(sdkCtx) require.Equal(t, initialHash, finalHash) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
x/evm/contracts/counter/Counter.go(1 hunks)x/evm/contracts/counter/Counter.sol(1 hunks)x/evm/keeper/fuzz_test.go(1 hunks)x/evm/state/memstore.go(1 hunks)x/evm/state/statedb_test.go(1 hunks)
🔇 Additional comments (6)
x/evm/contracts/counter/Counter.sol (1)
17-23: Verify IBC testing coverage with the modified function
Since increase_for_fuzz is likely used in testing scenarios, ensure that the IBC callback functionality is still adequately tested with the new implementation, particularly for:
- Gas consumption patterns in cross-chain operations
- Interaction with IBC acknowledgments and timeouts
- Edge cases in recursive scenarios
x/evm/keeper/fuzz_test.go (1)
62-62: Verify concurrent behavior without transient store
The replacement of bloom filter with assertLogs aligns with the PR's objective. However, we should verify that removing the transient store doesn't impact concurrent operations.
✅ Verification successful
Let me gather more information about the concurrent test implementation to verify the impact.
Let me check the concurrent test implementation more thoroughly using ast-grep.
Based on the complete test implementation, I can now generate the final response:
Concurrent test implementation is correct and safe
The concurrent test implementation is properly designed and the removal of transient store/bloom filter doesn't impact its correctness because:
- Each goroutine uses its own cached context via
CacheContext(), ensuring isolation - The test properly validates logs through
assertLogswhich checks contract events - The final state verification is done separately for each cached context
- The synchronization using
WaitGroupensures all operations complete before verification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other concurrent tests that might be affected by transient store removal
# Search for concurrent test patterns
echo "Searching for other concurrent tests that might use transient store..."
rg -l "sync\.(WaitGroup|Mutex)" --type go
# Search for transient store usage in test files
echo "Checking for remaining transient store usage in tests..."
rg "TStoreKey|TransientStore" --type go
Length of output: 1830
Script:
#!/bin/bash
# Check the implementation of concurrent test and assertLogs
echo "Checking concurrent test implementation..."
rg -A 10 "func TestConcurrentCounter" x/evm/keeper/fuzz_test.go
echo "Checking assertLogs implementation..."
rg -A 5 "func assertLogs" x/evm/keeper/fuzz_test.go
echo "Checking if there are any remaining bloom filter references..."
rg "bloom|Bloom" x/evm/keeper/
Length of output: 726
Script:
#!/bin/bash
# Get the full implementation of TestConcurrentCounter using ast-grep
ast-grep --pattern 'func TestConcurrentCounter($$$) {
$$$
}'
# Get the full context of the test file
echo "Getting full test file content..."
cat x/evm/keeper/fuzz_test.go
Length of output: 3101
x/evm/contracts/counter/Counter.go (3)
Line range hint 1019-1067: LGTM: Event handling implementation is robust
The implementation of event handlers (CallbackReceived, Increased, RecursiveCalled) demonstrates good practices:
- Proper error handling
- Resource cleanup through subscription unsubscription
- Thread-safe event forwarding
Also applies to: 1068-1116, 1117-1165
35-35: Verify the updated contract binary
The contract binary has been updated while maintaining the same ABI. This suggests internal implementation changes, which aligns with the PR's objective of switching from block-wise transient store to memstore.
Line range hint 519-519: Review recursive implementation for potential issues
The recursive method implementation could lead to stack overflow or excessive gas consumption if not properly bounded. Consider adding appropriate guards and documentation about the maximum allowed recursion depth.
x/evm/state/memstore.go (1)
69-71: Verify: Ensure iterator methods enforce start < end as per contract
The comments for Iterator and ReverseIterator specify that "Start must be less than end, or the Iterator is invalid." The current implementations delegate directly to store.kvStore.Iterator without explicitly checking this condition.
Run the following script to verify if the underlying kvStore.Iterator methods enforce the start < end constraint:
If the underlying kvStore does not enforce this constraint, consider adding checks in your Iterator and ReverseIterator methods:
func (store coreKVStore) Iterator(start, end []byte) (store.Iterator, error) {
+ if bytes.Compare(start, end) >= 0 {
+ return nil, fmt.Errorf("invalid iterator range: start must be less than end")
+ }
return store.kvStore.Iterator(start, end), nil
}Also applies to: 78-80
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
x/evm/keeper/context.go(2 hunks)
🔇 Additional comments (4)
x/evm/keeper/context.go (4)
190-193:
Handle potential nil StateDB
As noted in a previous review, proper error handling after NewStateDB is crucial. The current implementation should be enhanced to handle nil StateDB cases.
Apply this diff as suggested in the previous review:
evm.StateDB, err = k.NewStateDB(ctx, evm, fee)
if err != nil {
return ctx, nil, err
}
+if evm.StateDB == nil {
+ return ctx, nil, fmt.Errorf("StateDB is nil after initialization")
+}167-168:
Reconsider the dummy block context pattern
Using a dummy block context for chain rules initialization is a risky pattern. This could lead to inconsistencies if the chain rules depend on block context state that's not properly initialized.
Consider refactoring to avoid the dummy context:
-// use dummy block context for chain rules in EVM creation
-dummyBlockContext, err := k.buildBlockContext(ctx, nil, fee)
+// build actual block context with proper initialization
+blockContext, err := k.buildBlockContext(ctx, nil, fee)Likely invalid or redundant comment.
30-31: Verify gas delegation implementation
The use of InfiniteGasMeter is acceptable since gas tracking is delegated to the EVM, preventing double gas charging. However, let's verify the gas consumption is properly tracked in the EVM calls.
✅ Verification successful
Gas consumption is properly tracked and delegated
The use of InfiniteGasMeter in NewStateDB is safe as the code shows proper gas tracking:
- EVM gas consumption is correctly tracked and charged in the SDK context after EVM execution (3 instances in
keeper/context.go) - Precompiles properly track and charge gas for their operations
- No double charging occurs since the infinite gas meter is only used during EVM execution, while actual consumption is tracked and charged afterward
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify gas consumption tracking in EVM calls
# Look for gas consumption patterns in the codebase
# Check for gas meter usage in EVM calls
rg -A 5 "GasMeter\(\).ConsumeGas"
# Check for potential gas-related issues
ast-grep --pattern 'sdkCtx.GasMeter().ConsumeGas($_, $_)'
Length of output: 8676
177-185:
Address the uncertainty in EVM initialization
The comment indicates uncertainty about correct EVM initialization with empty context and stateDB. This is a critical concern that needs to be addressed before merging.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
x/evm/state/statedb.go (5)
Line range hint
187-209: Improve error handling consistency in refund operations.The AddRefund and SubRefund methods have inconsistent error handling - some errors trigger panic while others are properly handled.
Consider standardizing error handling across these methods:
func (s *StateDB) AddRefund(gas uint64) { refund, err := s.memStoreRefund.Get(s.ctx) - if err != nil { - panic(err) - } + if err != nil { + return fmt.Errorf("failed to get refund: %w", err) + } err = s.memStoreRefund.Set(s.ctx, refund+gas) - if err != nil { - panic(err) - } + if err != nil { + return fmt.Errorf("failed to set refund: %w", err) + } + return nil }
Line range hint
534-554: Optimize transient state operations.The current implementation of SetTransientState and GetTransientState creates a new byte slice on every operation by concatenating address and key.
Consider using a more efficient key construction:
+func transientKey(addr common.Address, key common.Hash) []byte { + b := make([]byte, common.AddressLength+common.HashLength) + copy(b[:common.AddressLength], addr.Bytes()) + copy(b[common.AddressLength:], key.Bytes()) + return b +} func (s *StateDB) SetTransientState(addr common.Address, key, value common.Hash) { prev := s.GetTransientState(addr, key) if prev == value { return } - if err := s.memStoreVMStore.Set(s.ctx, append(addr.Bytes(), key.Bytes()...), value[:]); err != nil { + if err := s.memStoreVMStore.Set(s.ctx, transientKey(addr, key), value[:]); err != nil { panic(err) } }
Line range hint
690-704: Fix potential index out of bounds in Logs method.The current implementation assumes that log indices are sequential and within bounds, which might not be true if logs were added out of order or if some indices are missing.
Consider using a more robust approach:
func (s *StateDB) Logs() evmtypes.Logs { logSize, err := s.memStoreLogSize.Get(s.ctx) if err != nil { panic(err) } else if logSize == 0 { return []evmtypes.Log{} } - logs := make([]evmtypes.Log, logSize) + logs := make([]evmtypes.Log, 0, logSize) err = s.memStoreLogs.Walk(s.ctx, nil, func(key uint64, log evmtypes.Log) (stop bool, err error) { - logs[key] = log + logs = append(logs, log) return false, nil }) if err != nil { panic(err) } return logs }
Line range hint
642-662: Improve error handling in Commit method.The Commit method could potentially leave the state in an inconsistent state if an error occurs mid-operation.
Consider implementing atomic commits:
func (s *StateDB) Commit() error { + // Collect all operations first + var ops []func() error err := s.memStoreSelfDestruct.Walk(s.ctx, nil, func(key []byte) (stop bool, err error) { addr := common.BytesToAddress(key) + ops = append(ops, func() error { if bal := s.GetBalance(addr); bal.Sign() != 0 { s.SubBalance(addr, bal, tracing.BalanceDecreaseSelfdestructBurn) } - err = s.vmStore.Clear(s.ctx, new(collections.Range[[]byte]).Prefix(addr.Bytes())) + if err := s.vmStore.Clear(s.ctx, new(collections.Range[[]byte]).Prefix(addr.Bytes())); err != nil { + return fmt.Errorf("failed to clear storage: %w", err) + } s.accountKeeper.RemoveAccount(s.ctx, s.accountKeeper.GetAccount(s.ctx, sdk.AccAddress(addr.Bytes()))) - return false, err + return nil + }) + return false, nil }) if err != nil { return err } + // Execute all operations + for _, op := range ops { + if err := op(); err != nil { + return err + } + } + // commit all changes for i := range s.snaps { s.snaps[len(s.snaps)-i-1].Commit() } s.ctx = s.initialCtx return nil }
Line range hint
271-302: Consider adding event attributes for contract creation failure.The CreateContract method emits an event on success but doesn't emit any event on failure.
Consider adding failure events for better observability:
func (s *StateDB) CreateContract(contractAddr common.Address) { + defer func() { + if r := recover(); r != nil { + s.ctx.EventManager().EmitEvent(sdk.NewEvent( + evmtypes.EventTypeContractCreationFailed, + sdk.NewAttribute(evmtypes.AttributeKeyContract, contractAddr.Hex()), + sdk.NewAttribute(evmtypes.AttributeKeyError, fmt.Sprint(r)), + )) + panic(r) + } + }() // ... rest of the function }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
x/evm/state/statedb.go(16 hunks)
🔇 Additional comments (1)
x/evm/state/statedb.go (1)
592-595:
Add bounds check in ContextOfSnapshot.
The method doesn't validate the snapshot index, which could lead to a panic if an invalid index is provided.
Add boundary validation:
func (s *StateDB) ContextOfSnapshot(i int) sdk.Context {
if i == -1 {
return s.initialCtx.Context
}
+ if i < 0 || i >= len(s.snaps) {
+ panic(fmt.Sprintf("snapshot index out of range: %d", i))
+ }
return s.snaps[i].ctx.Context
}Likely invalid or redundant comment.
Description
I'm not sure why we're continuing to encounter app hash crashes on the query node. To identify the root cause, this PR removes the use of block-wise transient storage.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes