feat(evm): support in-process evm via geth #2986
Conversation
|
The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).
|
|
Claude finished @julienrbrt's task in 2m 50s —— View job Code Review CompleteI've performed a comprehensive review of the in-process Geth integration. Below are my findings organized by category, ranging from critical issues to suggestions for improvement. Critical Issues1. Missing Error Handling in Flag Parsing (apps/evm/cmd/run.go:63,273,288)Severity: HIGH The errors returned by
// Line 63 - CRITICAL: ignores error
useGeth, _ := cmd.Flags().GetBool(evm.FlagEVMInProcessGeth)
// Line 273 - CRITICAL: ignores error
genesisPath, _ := cmd.Flags().GetString(evm.FlagEVMGenesisPath)
// Line 288 - CRITICAL: ignores error
rpcAddress, _ := cmd.Flags().GetString(evm.FlagEVMRPCAddress)Recommended Fix: Always handle flag parsing errors: useGeth, err := cmd.Flags().GetBool(evm.FlagEVMInProcessGeth)
if err != nil {
return nil, fmt.Errorf("failed to get '%s' flag: %w", evm.FlagEVMInProcessGeth, err)
}
if useGeth {
genesisPath, err := cmd.Flags().GetString(evm.FlagEVMGenesisPath)
if err != nil {
return nil, fmt.Errorf("failed to get '%s' flag: %w", evm.FlagEVMGenesisPath, err)
}
if len(genesisPath) == 0 {
return nil, fmt.Errorf("genesis path must be provided when using in-process Geth")
}
// ...
}2. O(N) DeleteRange Performance Bottleneck (execution/evm/engine_geth_db.go:49-71, 174-202)Severity: HIGH The Problems:
Impact: This will cause severe performance degradation as the chain grows, potentially making the node unusable after accumulating significant state. Current Implementation: func (w *wrapper) DeleteRange(start []byte, end []byte) error {
// Query all keys - O(N) where N = total keys!
q := query.Query{KeysOnly: true}
results, err := w.ds.Query(context.Background(), q)
// ... iterates through ALL keys to find matching range
}Recommended Fix: Leverage range queries with prefix filtering if 3. Inefficient Iterator Implementation (execution/evm/engine_geth_db.go:299-333)Severity: MEDIUM-HIGH The iterator's Current Implementation: func (it *iteratorWrapper) Next() bool {
for {
result, ok := it.results.NextSync()
// ...
keyBytes := datastoreKeyToBytes(result.Entry.Key)
// Filters AFTER retrieval - inefficient!
if len(it.start) > 0 && bytes.Compare(keyBytes, it.start) < 0 {
continue // Skip keys before start
}
// ...
}
}Problem: If you need to iterate starting from key Recommended Fix: Seek directly to the start key using prefix queries or modify the query to include range bounds if High Priority Issues4. Transaction Pool priceLimit Set to Zero (execution/evm/engine_geth.go:215)Severity: MEDIUM The transaction pool is initialized with txPool, err := txpool.New(0, blockchain, []txpool.SubPool{legacyPool})Implications:
Recommendation:
5. Mutex Held During Network I/O (execution/evm/engine_geth.go:267, 443, 670)Severity: MEDIUM The func (g *gethEngineClient) ForkchoiceUpdated(ctx context.Context, ...) {
g.backend.mu.Lock()
defer g.backend.mu.Unlock() // Held during blockchain.GetBlockByHash()
headBlock := g.backend.blockchain.GetBlockByHash(fcState.HeadBlockHash) // Could be slow
// ... more operations under lock
}Problem: If blockchain operations involve disk I/O or are slow, this blocks all other callers unnecessarily. Recommendation: Minimize critical sections by releasing the mutex before expensive operations when safe to do so. 6. Missing Context Cancellation Checks (execution/evm/engine_geth.go:216-233, 277-323)Severity: MEDIUM Several operations don't check
Recommendation: Add context checks before expensive operations and provide cancellation mechanisms for background goroutines. Medium Priority Issues7. Unsafe Concurrent Access to Batch Operations (execution/evm/engine_geth_db.go:136-262)Severity: MEDIUM The type batchWrapper struct {
ds datastore.Batching
ops []batchOp
size int
mu sync.Mutex // Good - has mutex
}Concern: While the mutex protects the batch's internal state, the pattern of creating a batch, having multiple goroutines add to it, then writing is uncommon. Verify this usage pattern with Geth's batch semantics. Recommendation: Add clear documentation about thread-safety guarantees and validate against Geth's expected usage patterns. 8. Iterator Not Checking Closed State (execution/evm/engine_geth_db.go:342-362)Severity: LOW-MEDIUM
func (it *iteratorWrapper) Key() []byte {
it.mu.Lock()
defer it.mu.Unlock()
if !it.started || it.closed {
return nil // Good - checks closed
}
return datastoreKeyToBytes(it.current.Key) // But current may be stale
}Recommendation: Ensure 9. Hardcoded Timeouts in HTTP Server (execution/evm/engine_geth_rpc.go:117-121)Severity: LOW b.httpServer = &http.Server{
Handler: handler,
ReadTimeout: 30 * time.Second, // Hardcoded
WriteTimeout: 30 * time.Second, // Hardcoded
}Problem: 30 seconds may be too short for complex RPC calls (like debug traces) or too long for simple ones. Recommendation: Make timeouts configurable via flags or environment variables. 10. Wide-Open CORS Policy (execution/evm/engine_geth_rpc.go:135)Severity: LOW-MEDIUM (Security) w.Header().Set("Access-Control-Allow-Origin", "*") // Allows ANY originSecurity Impact: Allows any website to make requests to the RPC server. While this may be acceptable for local development, it's risky for production deployments. Recommendation: Make CORS origins configurable, default to localhost only, and document security implications. Code Quality & Architecture11. Missing Package-Level DocumentationThe new packages lack godoc comments explaining their purpose:
Recommendation: Add package comments following Go conventions: // Package evm provides an in-process Geth execution client that allows
// ev-node to embed a full Ethereum execution layer without requiring
// an external Geth or Reth process.12. Inconsistent Error WrappingSome errors use // Good:
return nil, fmt.Errorf("failed to create geth backend: %w", err)
// Inconsistent:
return fmt.Errorf("failed to setup genesis: %w", genesisErr) // genesisErr is already descriptiveRecommendation: Consistently use 13. Magic Numbers Without Constants (execution/evm/engine_geth.go:39-41)const (
baseFeeChangeDenominator = 8 // Good - has constant
)
// But elsewhere:
txPool, err := txpool.New(0, blockchain, []txpool.SubPool{legacyPool}) // 0 is magicRecommendation: Define named constants for magic numbers (price limit, timeouts, etc.) to improve readability and maintainability. Testing Concerns14. Limited Edge Case CoverageThe tests in Missing Test Cases:
Recommendation: Add negative test cases and stress tests, especially for the database adapter layer which has known performance concerns. 15. No Integration Tests for Database AdapterThe database wrapper ( Recommendation: Add thorough tests for:
Performance & Resource Efficiency16. Unbounded Payload Map Growth (execution/evm/engine_geth.go:52, 471)type GethBackend struct {
// ...
payloads map[engine.PayloadID]*payloadBuildState // Never pruned!
// ...
}
// Deletes payload after GetPayload
delete(g.backend.payloads, payloadID) // Good, but what if GetPayload never called?Problem: If Recommendation: Implement a TTL-based cleanup mechanism or maximum size limit for the payloads map. 17. No Resource Limits on Transaction Processing (execution/evm/engine_geth.go:548-598)The
Recommendation: Add safeguards:
18. Inefficient Transaction Filtering (execution/evm/engine_geth.go:549-605)for i, txBytes := range ps.transactions {
if i%100 == 0 { // Only checks context every 100 txs
if err := ctx.Err(); err != nil {
return nil, fmt.Errorf("context cancelled during tx execution: %w", err)
}
}
// ...
}Issues:
Recommendation: Implement smarter batching and use gas-aware early termination. Security Considerations19. Genesis Configuration Validation (execution/evm/engine_geth.go:162-170)The code auto-populates missing blob schedule configuration: if genesis.Config.BlobScheduleConfig == nil {
if genesis.Config.CancunTime != nil || genesis.Config.PragueTime != nil {
genesis.Config.BlobScheduleConfig = ¶ms.BlobScheduleConfig{
Cancun: params.DefaultCancunBlobConfig,
Prague: params.DefaultPragueBlobConfig,
}
logger.Debug().Msg("auto-populated blobSchedule config for Cancun/Prague forks")
}
}Concern: Silently modifying genesis configuration can lead to unexpected chain behavior and consensus issues. Recommendation: Make this explicit with a WARNING log level, or require the configuration to be provided correctly. 20. No Rate Limiting on RPC Endpoints (execution/evm/engine_geth_rpc.go)The RPC server has no rate limiting or request size limits, making it vulnerable to:
Recommendation: Implement rate limiting and request size limits for production deployments. Documentation21. Missing Configuration ExamplesThe PR adds new flags but provides no examples of how to use them:
Recommendation: Add:
22. Unclear Concurrency SemanticsSeveral types have concurrent access but documentation doesn't clarify:
Recommendation: Add clear concurrency documentation to exported types and methods. Positive ObservationsDespite the issues above, there are several well-implemented aspects:
Summary & RecommendationsThis PR represents a significant undertaking to embed Geth directly into ev-node. The implementation demonstrates good Go practices in many areas but has several critical issues that should be addressed before merging: Must Fix Before Merge:
Should Fix Before Merge: Can Address Post-Merge (with follow-up issues):
Given this is marked as a "friday fun project" not meant for merging, these findings can serve as a roadmap if/when this work is productionized. Todo List
|
Summary of ChangesHello @julienrbrt, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the EVM execution capabilities by introducing an in-process Geth client. This architectural shift allows the application to perform Ethereum Virtual Machine operations directly, bypassing the need for an external execution client via RPC. The change aims to streamline development, simplify deployment, and potentially improve performance by reducing inter-process communication overhead. It includes the necessary data storage adaptations and new configuration options to enable this embedded functionality. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature: the ability to run an in-process Geth instance as the execution client. This is a great addition that will simplify setup, testing, and deployment for users by removing the need for an external execution client process. The implementation is extensive, including a new ethdb wrapper for go-datastore and the core logic for the in-process Geth backend. My review focuses on some critical areas for improvement, particularly around error handling in flag parsing and potential performance bottlenecks in the new database wrapper. I've also included a comment on the transaction pool configuration for consideration.
apps/evm/cmd/run.go
Outdated
| useGeth, _ := cmd.Flags().GetBool(evm.FlagEVMInProcessGeth) | ||
| if useGeth { | ||
| genesisPath, _ := cmd.Flags().GetString(evm.FlagEVMGenesisPath) | ||
| if len(genesisPath) == 0 { | ||
| return nil, fmt.Errorf("genesis path must be provided when using in-process Geth") | ||
| } |
There was a problem hiding this comment.
The errors returned by cmd.Flags().GetBool() and cmd.Flags().GetString() are being ignored. This can lead to silent failures and unexpected behavior if the flags are not parsed correctly (e.g., due to a typo or incorrect type). The program might proceed with default values (e.g., an empty genesisPath), causing issues later on. It's crucial to handle these errors to make the command-line interface more robust.
| useGeth, _ := cmd.Flags().GetBool(evm.FlagEVMInProcessGeth) | |
| if useGeth { | |
| genesisPath, _ := cmd.Flags().GetString(evm.FlagEVMGenesisPath) | |
| if len(genesisPath) == 0 { | |
| return nil, fmt.Errorf("genesis path must be provided when using in-process Geth") | |
| } | |
| useGeth, err := cmd.Flags().GetBool(evm.FlagEVMInProcessGeth) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get '%s' flag: %w", evm.FlagEVMInProcessGeth, err) | |
| } | |
| if useGeth { | |
| genesisPath, err := cmd.Flags().GetString(evm.FlagEVMGenesisPath) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to get '%s' flag: %w", evm.FlagEVMGenesisPath, err) | |
| } | |
| if len(genesisPath) == 0 { | |
| return nil, fmt.Errorf("genesis path must be provided when using in-process Geth") | |
| } |
| // DeleteRange implements ethdb.KeyValueStore. | ||
| func (w *wrapper) DeleteRange(start []byte, end []byte) error { | ||
| // Query all keys and delete those in range | ||
| q := query.Query{KeysOnly: true} | ||
| results, err := w.ds.Query(context.Background(), q) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer results.Close() | ||
|
|
||
| for result := range results.Next() { | ||
| if result.Error != nil { | ||
| return result.Error | ||
| } | ||
| keyBytes := datastoreKeyToBytes(result.Entry.Key) | ||
| if bytes.Compare(keyBytes, start) >= 0 && bytes.Compare(keyBytes, end) < 0 { | ||
| if err := w.ds.Delete(context.Background(), datastore.NewKey(result.Entry.Key)); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The DeleteRange implementation queries all keys from the datastore and then iterates through them to find keys within the specified range. This approach has a time complexity of O(N), where N is the total number of keys in the datastore. This can be very inefficient and cause significant performance degradation, especially with a large database, as Geth may use range deletions frequently. A more efficient implementation would leverage the underlying database's ability to iterate over a specific range, which is typically O(M) where M is the number of keys in the range. While the go-datastore interface is limited, this implementation could become a serious performance bottleneck.
| // Check if key is >= start (if start is set) | ||
| if len(it.start) > 0 && bytes.Compare(keyBytes, it.start) < 0 { | ||
| continue | ||
| } |
There was a problem hiding this comment.
The iterator's Next method filters keys by comparing them to the start key after retrieving them. This means it may iterate over many keys before the start key, which is inefficient. A more performant iterator would seek to the start key directly. This implementation could lead to performance issues when iterating over large datasets with a start key set far from the beginning. Given that database iterators are a core part of Geth's performance, this could be a significant bottleneck.
| txPoolConfig.NoLocals = true | ||
|
|
||
| legacyPool := legacypool.New(txPoolConfig, blockchain) | ||
| txPool, err := txpool.New(0, blockchain, []txpool.SubPool{legacyPool}) |
There was a problem hiding this comment.
The transaction pool is initialized with a priceLimit of 0. This disables the minimum gas price enforcement in the transaction pool, which could potentially allow for spamming the mempool with zero-price transactions. While this might be intentional for a rollup environment where gas pricing is handled differently, it's important to ensure this is the desired behavior. Consider making this configurable if there are scenarios where a price limit is needed.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2986 +/- ##
=======================================
Coverage 58.98% 58.98%
=======================================
Files 103 103
Lines 9902 9902
=======================================
Hits 5841 5841
Misses 3434 3434
Partials 627 627
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Attempt to embed geth in evnode.
90% AI.
friday fun project. not meant to be merged at any time.