Refactor EthRpcClient and L1RpcPrefetcher #70

RogerPodacter · 2026-01-09T15:26:32Z

Upgraded connection_pool gem to version 3.0.2.
Refactored EthRpcClient to include JWT support and improved retry configuration.
Enhanced L1RpcPrefetcher to prevent prefetching beyond the chain tip and added error handling for block fetch failures.
Updated EthBlockImporter to utilize new EthRpcClient methods for better clarity and performance.
Adjusted tests to cover new behaviors in L1RpcPrefetcher.

Note

Modernizes RPC and prefetch layers and makes block import more resilient and observable.

Refactors EthRpcClient to use persistent connections with Net::HTTP::Persistent, configurable retries, optional JWT auth, unique request IDs, and explicit handling of execution reverted; adds l1, l1_prefetch, l2, and l2_engine helpers
Reworks L1RpcPrefetcher with chain-tip aware prefetching, fetch timeout, clearer error surface (BlockFetchError), promise cleanup/cancel, stats, and graceful shutdown; memoizes chain tip reads
Updates EthBlockImporter to use EthRpcClient.l1/l1_prefetch, pre-warm next block in non-test, map prefetch errors to BlockNotReadyToImportError, prune/remove extra prefetch calls, and add shutdown
Clockwork (derive_facet_blocks.rb): run every 2s, wrap loop in begin/ensure to call shutdown, and shutdown on reorg before reinit
Tests: expand L1RpcPrefetcher specs for not-ready, tip bounds, and shutdown; minor simulator tweak to use EthRpcClient.l1
Deps/infra: upgrade connection_pool and net-http-persistent; bump facet-node image to v2.0.2

^{Written by Cursor Bugbot for commit c198990. This will update automatically on new commits. Configure here.}

- Upgraded connection_pool gem to version 3.0.2. - Refactored EthRpcClient to include JWT support and improved retry configuration. - Enhanced L1RpcPrefetcher to prevent prefetching beyond the chain tip and added error handling for block fetch failures. - Updated EthBlockImporter to utilize new EthRpcClient methods for better clarity and performance. - Adjusted tests to cover new behaviors in L1RpcPrefetcher.

Copilot

Pull request overview

This PR refactors the EthRpcClient and L1RpcPrefetcher to improve performance, reliability, and maintainability. The changes include upgrading the connection_pool gem, adding JWT support for authenticated endpoints, implementing configurable retry logic, and enhancing the prefetcher to prevent fetching beyond the chain tip.

Key changes:

Added JWT authentication support with optional configuration for authenticated RPC endpoints
Replaced HTTParty with Net::HTTP::Persistent for better connection pooling and performance
Enhanced L1RpcPrefetcher with chain tip detection to avoid prefetching unavailable blocks
Improved error handling with specific exception types (BlockFetchError, ExecutionRevertedError)

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
Gemfile.lock	Upgraded connection_pool from 2.4.1 to 3.0.2 and net-http-persistent to 4.0.8 for improved connection management
lib/eth_rpc_client.rb	Major refactoring: added JWT support, configurable retry logic, HTTP persistent connections, and specialized client factories (l1, l1_prefetch, l2, l2_engine)
lib/l1_rpc_prefetcher.rb	Enhanced with chain tip prevention logic, improved shutdown handling, simplified fetch_job without Retriable wrapper, and better error handling with BlockFetchError
app/services/eth_block_importer.rb	Updated to use EthRpcClient.l1 singleton, added shutdown call in derive_facet_blocks.rb ensure block, and improved error handling for BlockFetchError
spec/l1_rpc_prefetcher_spec.rb	Added tests for new BlockFetchError behavior and chain tip prevention logic
lib/fct_mint_simulator.rb	Updated to use EthRpcClient.l1 singleton instead of direct initialization
config/derive_facet_blocks.rb	Added ensure block to properly shutdown importer and shutdown call on reorg detection

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-09T15:33:32Z

app/services/eth_block_importer.rb

    set_eth_block_starting_points
    populate_facet_block_cache

    @prefetcher = L1RpcPrefetcher.new(ethereum_client: @ethereum_client)


The prefetcher is initialized and ensure_prefetched is called in the constructor outside of test environment. However, there's no guarantee that the EthRpcClient.l1 singleton has the same configuration as what the prefetcher needs. Consider using EthRpcClient.l1_prefetch for the prefetcher initialization to ensure it uses the appropriate retry configuration for prefetching operations.

Suggested change

@prefetcher = L1RpcPrefetcher.new(ethereum_client: @ethereum_client)

@prefetcher = L1RpcPrefetcher.new(ethereum_client: EthRpcClient.l1_prefetch)

Copilot · 2026-01-09T15:33:32Z

spec/l1_rpc_prefetcher_spec.rb

+      prefetcher.ensure_prefetched(1)
+      sleep(0.1)  # Let promises start
+
+      promises = prefetcher.instance_variable_get(:@promises)


In the test setup, get_block_number is stubbed to return 100 for the initialization. However, in the new test "does not prefetch beyond chain tip", it's changed to return 5. This creates a problematic scenario where the prefetcher was initialized with @last_chain_tip = 100 but then the stub changes to return 5. The test should either create a new prefetcher instance with the correct initial value, or the test needs to account for this initialization issue.

Suggested change

prefetcher.ensure_prefetched(1)

sleep(0.1) # Let promises start

promises = prefetcher.instance_variable_get(:@promises)

local_prefetcher = described_class.new(ethereum_client: ethereum_client, ahead: 5, threads: 2)

local_prefetcher.ensure_prefetched(1)

sleep(0.1) # Let promises start

promises = local_prefetcher.instance_variable_get(:@promises)

Copilot · 2026-01-09T15:33:33Z

lib/eth_rpc_client.rb

+    @http = Net::HTTP::Persistent.new(
+      name: "eth_rpc_#{@uri.host}:#{@uri.port}",
+      pool_size: 100
+    )


The HTTP client pool is initialized once with pool_size: 100, but the prefetcher creates multiple threads (configurable, default 2) that will all share the same client instance. While Net::HTTP::Persistent is thread-safe, consider whether a pool size of 100 is appropriate for the expected concurrent usage patterns. The pool size should align with the expected number of concurrent requests across all use cases.

Copilot · 2026-01-09T15:33:33Z

lib/eth_rpc_client.rb

+    uri = URI(base_url)
+    request = Net::HTTP::Post.new(uri)
+    request.body = data.to_json
+    headers.each { |key, value| request[key] = value }
+
+    response = @http.request(uri, request)


The send_http_request_simple method creates a new URI object on every request instead of reusing the @uri instance variable that was already created during initialization. This is inefficient and should use the existing @uri variable.

Suggested change

uri = URI(base_url)

request = Net::HTTP::Post.new(uri)

request.body = data.to_json

headers.each { |key, value| request[key] = value }

response = @http.request(uri, request)

request = Net::HTTP::Post.new(@uri)

request.body = data.to_json

headers.each { |key, value| request[key] = value }

response = @http.request(@uri, request)

Copilot · 2026-01-09T15:33:33Z

spec/l1_rpc_prefetcher_spec.rb

+      allow(ethereum_client).to receive(:get_transaction_receipts).and_return(receipts_data)
+
+      prefetcher.ensure_prefetched(1)
+      sleep(0.1)  # Let promises start


Using sleep in tests to wait for asynchronous operations is fragile and can lead to flaky tests. Consider using a more robust approach such as waiting for a specific condition with a timeout, or using RSpec's eventually matcher if available.

Copilot · 2026-01-09T15:33:34Z

lib/l1_rpc_prefetcher.rb

+  def current_l1_block_number
+    @last_chain_tip = @eth.get_block_number
+  end
+
+  def cached_l1_block_number
+    current_l1_block_number


The memoization of cached_l1_block_number could lead to stale data. The method calls current_l1_block_number which updates @last_chain_tip, but the memoized result will return the same block number for 12 seconds even though @last_chain_tip is being updated. This creates inconsistency between the memoized value and the instance variable. Consider either removing the memoization or ensuring the side effect of updating @last_chain_tip doesn't happen within the memoized method.

Suggested change

def current_l1_block_number

@last_chain_tip = @eth.get_block_number

end

def cached_l1_block_number

current_l1_block_number

def fetch_l1_block_number

@eth.get_block_number

end

def current_l1_block_number

@last_chain_tip = fetch_l1_block_number

end

def cached_l1_block_number

fetch_l1_block_number

Copilot · 2026-01-09T15:33:34Z

lib/l1_rpc_prefetcher.rb

-        @promises.delete(block_number)
-      end
+      result = promise.value!(@fetch_timeout)
+    rescue => e


The rescue block catches all exceptions with a bare rescue. This is too broad and could mask unexpected errors. Consider catching specific exception types that are expected (e.g., Concurrent::Error, StandardError) to allow critical system errors to propagate properly.

Suggested change

rescue => e

rescue StandardError => e

Copilot · 2026-01-09T15:33:34Z

lib/l1_rpc_prefetcher.rb

+    distance_from_last_tip = @last_chain_tip - from_block
+    latest = if distance_from_last_tip > 10
+               cached_l1_block_number
+             else
+               current_l1_block_number
+             end


The logic for determining when to fetch the current vs cached block number could lead to incorrect behavior. The distance_from_last_tip calculation (line 23) uses @last_chain_tip which may be stale, and if the distance is greater than 10, it uses the cached value. However, if the cached value is also stale (up to 12 seconds old), the prefetcher could avoid fetching the latest block number even when it should. Consider checking the age of @last_chain_tip or the cached value before deciding whether to use it.

Copilot · 2026-01-09T15:33:35Z

lib/l1_rpc_prefetcher.rb

-        Rails.logger.error "Prefetch failed for block #{block_number}: #{e.message}"
-        # Clean up failed promise so it can be retried
+        Rails.logger.error "[PREFETCH] Block #{block_number}: #{e.class} - #{e.message}"
        @promises.delete(block_number)


The rescue block in the promise deletes the promise from the map before re-raising the error. However, this creates a race condition where other threads might try to access this promise between deletion and re-raise. Additionally, the error is re-raised within the promise's rescue handler which could lead to unhandled rejections. Consider whether the deletion should happen here or in the fetch method after detecting a rejected promise.

Suggested change

@promises.delete(block_number)

Copilot · 2026-01-09T15:33:35Z

lib/eth_rpc_client.rb

+  memoize :jwt, ttl: 55
+


The TTL for JWT memoization is set to 55 seconds, but JWT tokens typically have a claim verification window. If the receiving server checks the 'iat' (issued at) timestamp and expects it to be recent (e.g., within 60 seconds), a token issued at the start of the memoization period could be rejected near the end. Additionally, the JWT is called every time headers are built, which happens on every request. Consider whether memoization is beneficial here or if it adds unnecessary complexity for minimal performance gain.

Suggested change

memoize :jwt, ttl: 55

lib/eth_rpc_client.rb

cursor · 2026-01-09T15:38:44Z

lib/l1_rpc_prefetcher.rb

+      message = result.nil? ?
+        "Block #{block_number} fetch timed out after #{@fetch_timeout}s" :
+        "Block #{block_number} not yet available on L1"
+      raise BlockFetchError.new(message)


Promise not deleted on timeout causes stale promise reuse

Medium Severity

The comment states value! returns nil on timeout, but Concurrent::Promise#value! actually raises Concurrent::TimeoutError. The old code explicitly caught Concurrent::TimeoutError and deleted the promise from @promises. The new code catches all exceptions via rescue => e and raises BlockFetchError without deleting the promise. This means on timeout, subsequent fetch attempts reuse the same potentially-stuck promise instead of creating a fresh one. The result.nil? check and its timeout message are dead code since value! never returns nil on timeout.

- Updated EthBlockImporter to use EthRpcClient's L1 prefetch method for improved clarity. - Changed the block import frequency in derive_facet_blocks from 6 seconds to 2 seconds for more responsive processing. - Minor adjustments in EthRpcClient to streamline HTTP request handling.

RogerPodacter requested a review from Copilot January 9, 2026 15:26

Copilot started reviewing on behalf of RogerPodacter January 9, 2026 15:27 View session

Copilot AI reviewed Jan 9, 2026

View reviewed changes

cursor bot reviewed Jan 9, 2026

View reviewed changes

RogerPodacter added 2 commits January 9, 2026 10:58

Update Docker Compose to use facet-node version 2.0.2

c198990

RogerPodacter merged commit eef2187 into main Jan 9, 2026
3 checks passed

RogerPodacter deleted the improve_prefetching branch January 9, 2026 16:44

	@prefetcher = L1RpcPrefetcher.new(ethereum_client: @ethereum_client)
	@prefetcher = L1RpcPrefetcher.new(ethereum_client: EthRpcClient.l1_prefetch)

Refactor EthRpcClient and L1RpcPrefetcher #70

Refactor EthRpcClient and L1RpcPrefetcher #70

Uh oh!

Conversation

RogerPodacter commented Jan 9, 2026 • edited by cursor bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cursor bot Jan 9, 2026

Choose a reason for hiding this comment

Promise not deleted on timeout causes stale promise reuse

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

RogerPodacter commented Jan 9, 2026 •

edited by cursor bot

Loading