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 22s —— View job Code Review: In-Process Geth IntegrationI've completed a comprehensive review of this PR, which introduces in-process Geth execution capabilities. This is an ambitious feature that simplifies deployment by embedding Geth directly in the node. While the implementation demonstrates solid understanding of both Geth internals and the Engine API, there are several critical issues that need attention before this can be considered production-ready. Critical Issues (Must Fix)1. Datastore Performance Bottlenecks 🔴 (HIGH PRIORITY)File: The datastore wrapper has serious performance issues that will impact production use: DeleteRange O(N) complexity (lines 49-71): 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)
// ...iterates through ALL keys in the datastoreThis queries all keys in the datastore and filters in memory. With millions of keys, this will be extremely slow. Geth uses range deletions heavily during state pruning and ancient data cleanup. Iterator inefficiency (lines 298-333): Impact: These will cause severe performance degradation during:
Recommendation: The
2. Missing Error Handling in Flag Parsing 🔴File: useGeth, _ := cmd.Flags().GetBool(evm.FlagEVMInProcessGeth) // Line 63
genesisPath, _ := cmd.Flags().GetString(evm.FlagEVMGenesisPath) // Line 275
rpcAddress, _ := cmd.Flags().GetString(evm.FlagEVMRPCAddress) // Line 290Errors from flag parsing are silently ignored. If flags are misconfigured (typo, wrong type), the program will proceed with default/zero values, causing confusing runtime failures. Impact:
Fix: useGeth, err := cmd.Flags().GetBool(evm.FlagEVMInProcessGeth)
if err != nil {
return nil, fmt.Errorf("failed to get '%s' flag: %w", evm.FlagEVMInProcessGeth, err)
}3. Transaction Pool Price Limit Set to 0
|
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.