Improve prefetcher#117
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR improves the L1RpcPrefetcher by making it more resilient and efficient. The changes focus on better error handling, performance optimization, and more graceful shutdown behavior.
Key changes:
- Enhanced error handling with a custom BlockFetchError class and better timeout/availability handling
- Performance optimization by implementing smarter chain tip caching and reducing the default prefetch distance
- Improved shutdown process with proper promise cancellation and resource cleanup
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/l1_rpc_prefetcher.rb | Core improvements to prefetcher with better error handling, optimized caching, and enhanced shutdown |
| app/services/eth_block_importer.rb | Updated to handle new BlockFetchError from the prefetcher |
| config/derive_ethscriptions_blocks.rb | Improved cleanup with proper shutdown handling and removed redundant signal trapping |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| end | ||
|
|
||
| def cached_l1_block_number | ||
| current_l1_block_number |
There was a problem hiding this comment.
The cached_l1_block_number method calls current_l1_block_number, which updates @last_chain_tip, defeating the purpose of caching. This should return @last_chain_tip directly or have a different implementation that doesn't trigger side effects.
| current_l1_block_number | |
| @last_chain_tip |
| else | ||
| Rails.logger.warn "L1 RPC Prefetcher shutdown timed out, forcing kill" | ||
| @pool.kill | ||
| terminated = @pool.wait_for_termination(3) |
There was a problem hiding this comment.
[nitpick] The hardcoded timeout value of 3 seconds should be extracted to a constant or configuration parameter to make it more maintainable and configurable.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| end | ||
|
|
||
| Rails.logger.debug "Got result for block #{block_number}" | ||
| result |
There was a problem hiding this comment.
Bug: Unhandled Timeouts in Block Fetch
The fetch method no longer catches Concurrent::TimeoutError raised by promise.value!(timeout). This means timeouts now propagate as unhandled exceptions, bypassing the intended BlockFetchError conversion for "block not ready" conditions. The result.nil? check doesn't account for these timeouts, leading to incorrect error handling.
| current_l1_block_number | ||
| end | ||
| memoize :cached_l1_block_number, ttl: 12.seconds | ||
| end |
There was a problem hiding this comment.
Bug: Memoization Causes Stale Chain Tip
The cached_l1_block_number method is memoized, but it calls current_l1_block_number which updates @last_chain_tip. This means @last_chain_tip only updates when the cache expires, potentially leaving it stale and causing inconsistent behavior for logic relying on its value.
Note
Enhances L1 prefetcher fetch semantics, chain-tip caching, and shutdown, updates importer to handle new errors, and hardens the clockwork loop with cleaner shutdown and reorg handling.
lib/l1_rpc_prefetcher.rb):BlockFetchErrorand raise on nil/not-ready results; use:not_ready_sentinel.cached_l1_block_numberand@last_chain_tip; loweraheaddefault to20.shutdown: short timeout, cancel pending promises, clear state, and structured logging; return boolean.fetch: delete failed promises; keep timeout behavior viavalue!exceptions.app/services/eth_block_importer.rb):prefetcher_fetch) and mapL1RpcPrefetcher::BlockFetchErrortoBlockNotReadyToImportError.config/derive_ethscriptions_blocks.rb):begin...ensuretoshutdownimporter.shutdownthen reinitialize importer; add concise wait/retry logging and sleep placement.Written by Cursor Bugbot for commit b260fa9. This will update automatically on new commits. Configure here.