Make import faster#105
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR improves import performance by implementing L1 RPC prefetching, replacing HTTParty with persistent HTTP connections, and adding comprehensive profiling capabilities.
- Adds concurrent L1 RPC prefetching with configurable thread pools to fetch blocks ahead of time
- Replaces HTTParty with Net::HTTP::Persistent for better connection reuse and performance
- Implements detailed import profiling with timing statistics for bottleneck identification
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/l1_rpc_prefetcher.rb | New concurrent prefetcher for L1 blocks, receipts, and API data with thread pool management |
| lib/import_profiler.rb | New singleton profiler for timing import operations with thread-safe statistics collection |
| lib/eth_rpc_client.rb | Replaced HTTParty with Net::HTTP::Persistent for connection reuse |
| lib/block_validator.rb | Added concurrent storage verification and profiling integration |
| config/derive_ethscriptions_blocks.rb | Added graceful signal handling for shutdown |
| app/services/geth_driver.rb | Added profiling instrumentation for engine API calls |
| app/services/eth_block_importer.rb | Integrated prefetcher and profiler, removed old RPC result caching |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| # Only enqueue a reasonable number at once to avoid overwhelming the promise system | ||
| # We'll enqueue more as blocks are consumed | ||
| max_to_enqueue = [@threads * 20, 100].min # At most 20x thread count or 100 |
There was a problem hiding this comment.
The magic numbers 20 and 100 should be defined as constants or made configurable through environment variables to improve maintainability.
| @incomplete_actual = true | ||
| binding.irb if ENV['DEBUG'] == '1' | ||
| next | ||
| raise "No receipts returned for L2 block #{block_hash}" |
There was a problem hiding this comment.
This changes error handling behavior from continuing with next block to stopping execution entirely. The raise statement will terminate the loop instead of the previous next behavior, which could break existing error handling expectations.
| @incomplete_actual = true | ||
| binding.irb if ENV['DEBUG'] == '1' | ||
| next | ||
| raise "Failed to get receipts for block #{block_hash}: #{e.message}" |
There was a problem hiding this comment.
This changes error handling behavior from continuing with next block to stopping execution entirely. The raise statement will terminate the loop instead of the previous next behavior, which could break existing error handling expectations.
| raise "Failed to get receipts for block #{block_hash}: #{e.message}" | |
| next |
| @prefetcher = L1RpcPrefetcher.new( | ||
| ethereum_client: @ethereum_client, | ||
| ahead: ENV.fetch('L1_PREFETCH_FORWARD', Rails.env.test? ? 5 : 20).to_i, | ||
| threads: ENV.fetch('L1_PREFETCH_THREADS', Rails.env.test? ? 2 : 2).to_i |
There was a problem hiding this comment.
Line 40 uses the same value (2) for both test and non-test environments. This should likely be different values, such as a higher thread count for production.
| threads: ENV.fetch('L1_PREFETCH_THREADS', Rails.env.test? ? 2 : 2).to_i | |
| threads: ENV.fetch('L1_PREFETCH_THREADS', Rails.env.test? ? 2 : 10).to_i |
No description provided.