bump geth to v1.14.11 #99
Conversation
WalkthroughThe changes in this pull request involve updates to the Changes
Possibly related PRs
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumintegration-tests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go.mod(2 hunks)integration-tests/go.mod(4 hunks)
🔇 Additional comments (4)
integration-tests/go.mod (3)
11-11: Review major version bumps for breaking changes
Several initia packages have been updated to new major/minor versions:
- initia: v0.5.6 -> v0.6.0 (major)
- kvindexer: v0.1.8 -> v0.1.9 (minor)
- movevm: v0.5.0 -> v0.6.0 (major)
Major version bumps (v0.5.x -> v0.6.0) might introduce breaking changes. Please ensure:
- All breaking changes have been addressed
- Integration tests have been updated accordingly
#!/bin/bash
# Description: Look for potential breaking changes in the changelog or release notes
# Expected: Find information about breaking changes in v0.6.0
# Check for CHANGELOG.md or similar files
fd -t f "CHANGELOG.md|BREAKING_CHANGES.md" --exec cat {}Also applies to: 149-154
206-206: Verify blst update compatibility
The blst cryptographic library has been updated from v0.3.11 to v0.3.13. While this is a minor version bump, cryptographic libraries require careful verification.
#!/bin/bash
# Description: Check for any security-related changes or known issues
# Expected: Find information about changes between v0.3.11 and v0.3.13
# Check for security-related commits or issues
gh api repos/supranational/blst/releases --jq '.[] | select(.tag_name=="v0.3.13" or .tag_name=="v0.3.12") | .body'274-274: Verify geth version alignment with v1.14.11
The PR objective is to bump geth to v1.14.11, but the replacement directive points to a specific commit hash. Let's verify if this commit corresponds to the intended version.
✅ Verification successful
The previous script didn't return the expected commit message. Let me try a different approach to verify the geth version alignment.
Let me try one more approach to verify the version information.
✅ The go-ethereum replacement correctly aligns with v1.14.11
The commit f8482395518d in initia-labs/evm repository is a merge commit that explicitly includes the tag 'v1.14.11' as shown in the commit message "Merge tag 'v1.14.11'". This confirms that the replacement directive in go.mod is correctly pointing to a commit that incorporates geth v1.14.11.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if the commit hash corresponds to v1.14.11 tag
# Expected: The commit should be based on geth v1.14.11
# Check the commit message and tags
gh api repos/initia-labs/evm/commits/f8482395518d --jq '.commit.message'
Length of output: 453
Script:
#!/bin/bash
# Let's check the repository content at that specific commit
gh api repos/initia-labs/evm/contents?ref=f8482395518d --jq '.[].name'
# Also check if there's any tag information
gh api repos/initia-labs/evm/tags --jq '.[].name'
# Let's also look at the go.mod file at that commit to see what version it's based on
gh api repos/initia-labs/evm/contents/go.mod?ref=f8482395518d --jq '.content' | base64 -d
Length of output: 737
Script:
#!/bin/bash
# Let's try to get the raw content of go.mod without using base64
gh api -H "Accept: application/vnd.github.raw+json" repos/initia-labs/evm/contents/go.mod?ref=f8482395518d
# Also check the commit directly
gh api repos/initia-labs/evm/commits/f8482395518d --jq '.commit.message, .parents[].sha'
Length of output: 7287
go.mod (1)
227-227: LGTM: blst dependency update
The update of github.com/supranational/blst to v0.3.13 aligns with the geth update.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #99 +/- ##
==========================================
- Coverage 28.34% 28.31% -0.03%
==========================================
Files 126 126
Lines 14022 14014 -8
==========================================
- Hits 3974 3968 -6
+ Misses 9487 9485 -2
Partials 561 561
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
x/evm/precompiles/erc20_registry/contract.go (2)
Line range hint
46-159: Consider refactoring method handlers for better maintainability.While the implementation is functionally correct, the
ExtendedRunmethod is quite long and handles multiple concerns. Consider extracting the method handlers into separate functions to improve readability and maintainability.Example refactoring:
func (e *ERC20RegistryPrecompile) ExtendedRun(caller vm.ContractRef, input []byte, suppliedGas uint64, readOnly bool) (resBz []byte, usedGas uint64, err error) { // ... snapshot and context setup ... switch method.Name { case METHOD_REGISTER: - ctx.GasMeter().ConsumeGas(REGISTER_GAS, "register_erc20") - if readOnly { - return nil, ctx.GasMeter().GasConsumedToLimit(), types.ErrNonReadOnlyMethod.Wrap(method.Name) - } - if err := e.k.Register(ctx, caller.Address()); err != nil { - return nil, ctx.GasMeter().GasConsumedToLimit(), types.ErrPrecompileFailed.Wrap(err.Error()) - } - resBz, err = method.Outputs.Pack(true) + resBz, err = e.handleRegister(ctx, method, caller, readOnly) case METHOD_REGISTER_FROM_FACTORY: - // ... similar pattern ... + resBz, err = e.handleRegisterFromFactory(ctx, method, caller, readOnly, args) // ... other cases ... } usedGas = ctx.GasMeter().GasConsumedToLimit() return resBz, usedGas, err } +func (e *ERC20RegistryPrecompile) handleRegister(ctx types.Context, method *abi.Method, caller vm.ContractRef, readOnly bool) ([]byte, error) { + ctx.GasMeter().ConsumeGas(REGISTER_GAS, "register_erc20") + if readOnly { + return nil, types.ErrNonReadOnlyMethod.Wrap(method.Name) + } + if err := e.k.Register(ctx, caller.Address()); err != nil { + return nil, types.ErrPrecompileFailed.Wrap(err.Error()) + } + return method.Outputs.Pack(true) +}
Line range hint
161-169: Add documentation explaining the gas calculation strategy.While the implementations are correct, it would be helpful to add comments explaining:
- Why
RequiredGasreturns 0 (because gas calculation is handled inExtendedRun)- Why
Runis not supported (becauseExtendedRunprovides necessary context for gas metering and state management)x/evm/keeper/keeper.go (1)
81-83: LGTM! Good architectural improvements.The changes improve the integration with go-ethereum by:
- Using
vm.PrecompiledContractsdirectly instead of a custom type- Adding
precompileAddrsfor efficient address lookupsThis aligns well with the geth v1.14.11 upgrade and follows EVM implementation best practices.
Consider adding a method to validate that
precompileAddrsstays in sync withprecompilesto prevent potential inconsistencies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumintegration-tests/go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
go.mod(2 hunks)integration-tests/go.mod(4 hunks)x/evm/keeper/context.go(4 hunks)x/evm/keeper/keeper.go(3 hunks)x/evm/keeper/precompiles.go(2 hunks)x/evm/precompiles/cosmos/contract.go(5 hunks)x/evm/precompiles/cosmos/contract_test.go(9 hunks)x/evm/precompiles/erc20_registry/contract.go(3 hunks)x/evm/precompiles/erc20_registry/contract_test.go(1 hunks)x/evm/types/address.go(0 hunks)x/evm/types/expected_keeper.go(1 hunks)
💤 Files with no reviewable changes (1)
- x/evm/types/address.go
🚧 Files skipped from review as they are similar to previous changes (2)
- go.mod
- integration-tests/go.mod
🔇 Additional comments (20)
x/evm/types/expected_keeper.go (1)
93-94: LGTM! Improved interface naming and separation of concerns.
The renaming from WithStateDB to SetStateDB better follows Go naming conventions for setter methods. The removal of the vm.PrecompiledContract return type suggests a good separation of concerns, where state management is now independent of contract creation.
Let's verify the impact of this breaking change on existing implementations:
x/evm/precompiles/erc20_registry/contract.go (3)
15-17: LGTM: Appropriate use of pointer receivers for interface implementations.
The change to pointer receivers is correct as the ERC20RegistryPrecompile needs to maintain state across method calls.
25-31: LGTM: Constructor properly returns pointer and handles errors.
The constructor follows Go best practices by returning a pointer and properly handling errors by returning nil on failure.
34-36: LGTM: Clean implementation of state setter.
The SetStateDB method correctly uses a pointer receiver to modify the contract's state.
x/evm/precompiles/erc20_registry/contract_test.go (2)
79-79: LGTM: Simplified state setting approach
The change from WithStateDB to SetStateDB aligns with better practices by:
- Using a more direct and clear method name
- Avoiding unnecessary type assertions
- Maintaining the same functionality with simpler code
Line range hint 1-180: Verify test coverage with updated geth version
The test suite comprehensively covers key scenarios including:
- ERC20 registration
- Factory-based registration
- Store registration
- Gas limit checks
- Read-only operation validation
However, given the geth version update, let's verify there are no new edge cases to test.
x/evm/keeper/keeper.go (1)
160-162: Verify precompile initialization in loadPrecompiles()
The initialization looks good, but we should verify the loadPrecompiles() implementation to ensure:
- Both
precompilesandprecompileAddrsare properly populated - Thread safety is maintained if precompiles can be modified post-initialization
Let's check the loadPrecompiles implementation:
✅ Verification successful
Let me gather more context about the precompiles implementation to verify both concerns.
Precompile initialization verified and secure
The implementation in loadPrecompiles() properly handles both concerns:
-
Both fields are correctly populated:
precompilesis initialized with active precompiled contracts and custom precompilesprecompileAddrsis populated with corresponding addresses usingslices.Clone
-
Thread safety is not a concern as:
- The precompiles are only set once during initialization
- No subsequent modifications to
precompileswere found in the codebase - The implementation follows a clear initialization pattern without concurrent access
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for loadPrecompiles implementation
rg -A 10 "func.*loadPrecompiles"
Length of output: 640
Script:
#!/bin/bash
# Get more context around loadPrecompiles implementation
rg -A 30 "func.*loadPrecompiles"
# Search for any mutex/sync usage in the keeper
rg "sync\." "x/evm/keeper/"
# Search for any modifications to precompiles after initialization
ast-grep --pattern 'k.precompiles = $_'
Length of output: 2005
x/evm/precompiles/cosmos/contract.go (5)
25-27: LGTM! Consistent interface implementations with pointer receivers
The change to pointer receivers for interface implementations is appropriate since the methods need to modify the struct's state (e.g., SetStateDB).
Line range hint 52-67: LGTM! Constructor properly returns pointer and handles errors
The constructor changes align with Go best practices:
- Returns a pointer to
CosmosPrecompile - Proper error handling by returning
nilon failure - Complete initialization of all struct fields
Line range hint 70-86: LGTM! Methods properly implemented with pointer receivers
The implementation is correct and follows best practices:
- Proper use of pointer receivers for state modification
- Thorough error handling in
originAddress - Clear separation of concerns between state management and address handling
352-354: LGTM! Run method properly indicates exclusive ExtendedRun usage
The implementation correctly indicates that this precompile works exclusively with ExtendedRun, preventing misuse of the standard Run method.
347-349:
Potential DoS vector: RequiredGas returns 0
Returning 0 from RequiredGas means no gas cost for precompile execution, which could potentially be exploited for DoS attacks. Consider implementing proper gas calculation based on input size and operation complexity.
x/evm/keeper/context.go (2)
242-242: LGTM: Consistent updates to Prepare calls
The addition of k.precompileAddrs parameter to Prepare calls is consistently applied across all EVM operations (StaticCall, Call, Create), aligning with geth v1.14.11's API changes.
Also applies to: 280-280, 374-374
196-197: LGTM: Precompile handling updated for geth v1.14.11
The change from precompiles.toMap(stateDB) to k.Precompiles(stateDB) aligns with the geth v1.14.11 update.
Let's verify the thread safety of the new precompiles implementation:
x/evm/precompiles/cosmos/contract_test.go (2)
70-70: LGTM! Consistent implementation of SetStateDB across all test functions.
The changes consistently replace WithStateDB with SetStateDB across all test functions, maintaining uniform behavior throughout the test suite.
Also applies to: 116-116, 162-162, 193-193, 224-224, 297-297, 403-403, 450-450, 492-492
70-70: Verify thread safety implications of mutable state.
The change from WithStateDB to SetStateDB indicates a shift from immutable to mutable state management. While this is fine for the test environment, we should verify that the production code properly handles concurrent access to the state.
Also applies to: 116-116, 162-162, 193-193, 224-224, 297-297, 403-403, 450-450, 492-492
x/evm/keeper/precompiles.go (4)
38-39: Validate the use of an empty sdk.Context{}
On line 38, the code initializes chainConfig using an empty sdk.Context{}:
chainConfig := types.DefaultChainConfig(sdk.Context{})Ensure that using an empty context does not lead to unintended behavior. If DefaultChainConfig relies on context values (like block height, chain ID, etc.), passing an empty context might result in incorrect configurations.
Would you like assistance in verifying whether sdk.Context{} provides all necessary values for DefaultChainConfig?
41-44: Confirm correct integration of custom precompiles
Lines 41-44 add custom precompiles to the existing precompiled contracts:
precompiles := vm.ActivePrecompiledContracts(rules)
precompiles[types.CosmosPrecompileAddress] = cosmosPrecompile
precompiles[types.ERC20RegistryPrecompileAddress] = erc20RegistryPrecompile
k.precompiles = precompilesEnsure that the custom precompiles (cosmosPrecompile and erc20RegistryPrecompile) are fully compatible with the expected interface and behavior of precompiled contracts in the EVM. Improper integration may lead to execution errors or unexpected behavior during contract calls.
46-48: Verify the correctness of precompileAddrs order
The code clones the active precompile addresses and appends custom addresses:
precompileAddrs := slices.Clone(vm.ActivePrecompiles(rules))
precompileAddrs = append(precompileAddrs, types.CosmosPrecompileAddress, types.ERC20RegistryPrecompileAddress)
k.precompileAddrs = precompileAddrsWhile appending addresses, ensure that the order of addresses in precompileAddrs does not affect the initialization or execution of precompiles. If order is significant in any context where precompileAddrs is used, consider inserting the custom addresses at appropriate positions.
5-5:
Ensure compatibility with the slices package
The import statement on line 5:
"slices"introduces the use of the slices package from the Go standard library, which was added in Go 1.21. Please verify that the project's Go version has been updated to at least 1.21 to support this package. Using a newer Go version without updating the project settings may lead to build errors.
Run the following script to confirm the Go version specified in the project:
✅ Verification successful
Go version is compatible with slices package
The project uses Go 1.22.5, which fully supports the slices package introduced in Go 1.21. The import statement is safe to use.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the Go version specified in go.mod files.
# Test: Extract the Go version from go.mod. Expect: Version >= 1.21.
grep "^go " go.mod
Length of output: 30
Description
Diff between initia-labs/evm<>go-ethereum
=> ethereum/go-ethereum@v1.14.11...initia-labs:evm:main
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
New Features
Bug Fixes
Refactor
CosmosPrecompileandERC20RegistryPrecompilestructures.Chores