fix(legacy-rpc): eth_getLogs by blockHash returns empty array for local blocks with no matching events#263
Conversation
…al blocks with no matching events Previously, querying eth_getLogs with a blockHash filter would forward to legacy if the local result was empty. This caused "block not found" errors when the block existed locally but had no matching events, since legacy doesn't hold those blocks. Fix: check block existence via eth_getBlockByHash before deciding routing. - Block found locally → return local result directly (even if logs are []) - Block not found locally → forward to legacy Also adds unit tests for all three routing branches and an e2e regression test covering both the empty-logs case and a block with ERC20 Transfer events.
|
@claude review this |
XLayer-Bot
left a comment
There was a problem hiding this comment.
The fix is correct and well-targeted. Root cause analysis is accurate: the old is_result_empty check could not distinguish "empty logs for a local block" from "block unknown locally", causing unnecessary forwards to legacy that returned "block not found". Checking block existence first via eth_getBlockByHash cleanly resolves the ambiguity.
What I checked
- Routing logic for all three
BlockHashbranches (block found + empty logs, block found + events, block not found) call_eth_get_block_by_hashreturn type semantics:Ok(Some(_))correctly signals "block exists locally" — all valid Ethereum blocks carry a parseablenumberfield, so using it as an existence proxy is sound- Hash validation coverage —
parse_eth_get_logs_paramsvalidates viais_valid_32_bytes_stringbefore producingGetLogsParams::BlockHash, so no invalid hash can reachcall_eth_get_block_by_hash - Unit tests cover all three routing branches with a clean mock; e2e test covers the regression case and ERC20 log retrieval
- The
.omcgitignore entry is unrelated to the fix; fine to include but could be a separate commit
One minor note left inline.
…n getLogs blockHash path Separate Ok(None) and Err(_) arms so that any unexpected error from call_eth_get_block_by_hash is observable via a debug log rather than silently forwarded to legacy. The Err path is unreachable in practice since the block hash is already validated upstream, but this makes surprises visible.
|
@claude pls review again |
XLayer-Bot
left a comment
There was a problem hiding this comment.
The fix looks correct and complete. The previous concern about the silent arm has been addressed — the branch now logs the unexpected error before forwarding to legacy. The routing logic is semantically cleaner: block existence is checked first, and the empty-log case for a locally-known block is handled correctly. The three new unit tests cover all branches (block found with empty logs, block found with logs, block not found), and the e2e test adds regression coverage against a live node. workflow run
…ding (#262) * feat(legacy-rpc): add cutoff block override flag and refactor config construction Add `--rpc.legacy-cutoff-override` CLI flag to allow overriding the legacy RPC cutoff block height derived from genesis. This is intended for e2e testing where the devnet needs a specific cutoff block to exercise legacy routing logic. Refactor LegacyRpcRouterConfig construction into `LegacyRpcArgs::to_legacy_rpc_config()` to encapsulate the config building logic alongside the args definition. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test(legacy-rpc): add e2e tests for legacy RPC routing middleware Add comprehensive e2e test coverage for the LegacyRpcRouterLayer: - eth_getLogs blockHash filter with logs (baseline) - eth_getLogs blockHash filter with empty result (regression test for the bug where [] was incorrectly forwarded to legacy) - eth_getLogs range routing: pure local, pure legacy, hybrid split+merge - eth_getBlockByNumber forwarding for blocks below cutoff - Precondition helper asserting cutoff block is minted and empty Add DEVNET_GENESIS_BLOCK and LEGACY_CUTOFF_BLOCK_HEIGHT constants aligned with devnet config (--rpc.legacy-cutoff-override=8593925). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(legacy-rpc): use idiomatic if-let for clippy and remove unused import Replace `is_some() + unwrap()` with `if let Some(cutoff)` to satisfy clippy::unnecessary_unwrap. Remove unused LegacyRpcRouterConfig import from main.rs since config construction moved to LegacyRpcArgs. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test(legacy-rpc): add local routing assertion at cutoff boundary Extend test_legacy_get_block_by_number to also verify that a block at the cutoff height is served locally, testing both halves of the routing boundary (below → legacy, at/above → local). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test(legacy-rpc): revert redundant local routing assertion Remove the at-cutoff local routing assertion — local block retrieval is already covered by the existing test_eth_block_rpc::EthGetBlockByNumber test. Keep only the below-cutoff forwarding test which is the new behavior being validated. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor(legacy-rpc): switch legacy endpoint to L1 geth and remove cutoff override Legacy RPC now forwards to L1 geth instead of a second L2 node. The cutoff defaults to the L2 genesis block (8593921), so any block below genesis is forwarded to L1. - Remove --rpc.legacy-cutoff-override flag and to_legacy_rpc_config() - Remove LEGACY_CUTOFF_BLOCK_HEIGHT constant (cutoff = genesis) - Rewrite e2e tests to query L1 height and use actual L1 blocks for forwarding validation - Add precondition: L1 height must be below L2 genesis 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(legacy-rpc): restore inline config construction after merge Restore LegacyRpcRouterConfig inline construction in main.rs after merge with jimmyshi/fix-legacy-rpc-getLog reverted the to_legacy_rpc_config() refactor. Also apply rustfmt fixes from merge. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The previous test queried block 0x1 which doesn't exist on L2 (genesis is 8593921). With legacy RPC enabled, this forwarded to L1 which returned "0x" instead of null. Fix by querying the latest block with an out-of-range tx index (0xFFFF) instead. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@claude review this pr |
This comment was marked as resolved.
This comment was marked as resolved.
XLayer-Bot
left a comment
There was a problem hiding this comment.
The fix is correct and complete.
Root cause & fix: The old BlockHash path checked is_result_empty on the eth_getLogs result to decide routing — forwarding to legacy whenever logs were []. A local block with zero matching events returns [], so it was incorrectly forwarded to a legacy endpoint that does not hold local blocks, producing a spurious "block not found" error. The new path checks block existence via eth_getBlockByHash first: if the block is local, return the local logs result unconditionally (even []); if the block is not found locally, forward to legacy.
Code review:
- The
is_result_emptyimport is correctly removed fromget_logs.rsand the function still exists and is used inservice.rsfor other routing paths. call_eth_get_block_by_hashcallsself.inner.call(…)whereself.inneris the raw RPC handler, not anotherLegacyRpcRouterService— no recursive routing risk.- The
Err(_)arm is correctly unreachable (hash already validated byis_valid_32_bytes_stringinparse_eth_get_logs_params), and it falls back to legacy with adebug!log — good defensive coding. - Unit tests cover all three routing branches. The "forwards to legacy" test correctly verifies the forward path by relying on a connection failure to the unreachable endpoint
127.0.0.1:1. - E2E regression test covers both the empty-logs and non-empty-logs local cases.
- Flashblocks test fix (
0x1→ current block with out-of-range index) is correct.
Both previously raised threads have been properly addressed.
|
🔧 CI running — workflow run |
|
✅ XLayer Reth CI passed for commit
|
…al blocks with no matching events (#263) * fix(legacy-rpc): eth_getLogs by blockHash returns empty array for local blocks with no matching events Previously, querying eth_getLogs with a blockHash filter would forward to legacy if the local result was empty. This caused "block not found" errors when the block existed locally but had no matching events, since legacy doesn't hold those blocks. Fix: check block existence via eth_getBlockByHash before deciding routing. - Block found locally → return local result directly (even if logs are []) - Block not found locally → forward to legacy Also adds unit tests for all three routing branches and an e2e regression test covering both the empty-logs case and a block with ERC20 Transfer events. * fix(legacy-rpc): log unexpected Err from call_eth_get_block_by_hash in getLogs blockHash path Separate Ok(None) and Err(_) arms so that any unexpected error from call_eth_get_block_by_hash is observable via a debug log rather than silently forwarded to legacy. The Err path is unreachable in practice since the block hash is already validated upstream, but this makes surprises visible. * test(legacy-rpc): add e2e test coverage support for legacy RPC forwarding (#262) * feat(legacy-rpc): add cutoff block override flag and refactor config construction Add `--rpc.legacy-cutoff-override` CLI flag to allow overriding the legacy RPC cutoff block height derived from genesis. This is intended for e2e testing where the devnet needs a specific cutoff block to exercise legacy routing logic. Refactor LegacyRpcRouterConfig construction into `LegacyRpcArgs::to_legacy_rpc_config()` to encapsulate the config building logic alongside the args definition. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test(legacy-rpc): add e2e tests for legacy RPC routing middleware Add comprehensive e2e test coverage for the LegacyRpcRouterLayer: - eth_getLogs blockHash filter with logs (baseline) - eth_getLogs blockHash filter with empty result (regression test for the bug where [] was incorrectly forwarded to legacy) - eth_getLogs range routing: pure local, pure legacy, hybrid split+merge - eth_getBlockByNumber forwarding for blocks below cutoff - Precondition helper asserting cutoff block is minted and empty Add DEVNET_GENESIS_BLOCK and LEGACY_CUTOFF_BLOCK_HEIGHT constants aligned with devnet config (--rpc.legacy-cutoff-override=8593925). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(legacy-rpc): use idiomatic if-let for clippy and remove unused import Replace `is_some() + unwrap()` with `if let Some(cutoff)` to satisfy clippy::unnecessary_unwrap. Remove unused LegacyRpcRouterConfig import from main.rs since config construction moved to LegacyRpcArgs. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test(legacy-rpc): add local routing assertion at cutoff boundary Extend test_legacy_get_block_by_number to also verify that a block at the cutoff height is served locally, testing both halves of the routing boundary (below → legacy, at/above → local). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test(legacy-rpc): revert redundant local routing assertion Remove the at-cutoff local routing assertion — local block retrieval is already covered by the existing test_eth_block_rpc::EthGetBlockByNumber test. Keep only the below-cutoff forwarding test which is the new behavior being validated. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor(legacy-rpc): switch legacy endpoint to L1 geth and remove cutoff override Legacy RPC now forwards to L1 geth instead of a second L2 node. The cutoff defaults to the L2 genesis block (8593921), so any block below genesis is forwarded to L1. - Remove --rpc.legacy-cutoff-override flag and to_legacy_rpc_config() - Remove LEGACY_CUTOFF_BLOCK_HEIGHT constant (cutoff = genesis) - Rewrite e2e tests to query L1 height and use actual L1 blocks for forwarding validation - Add precondition: L1 height must be below L2 genesis 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(legacy-rpc): restore inline config construction after merge Restore LegacyRpcRouterConfig inline construction in main.rs after merge with jimmyshi/fix-legacy-rpc-getLog reverted the to_legacy_rpc_config() refactor. Also apply rustfmt fixes from merge. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix(test): use existing block for raw tx index negative test The previous test queried block 0x1 which doesn't exist on L2 (genesis is 8593921). With legacy RPC enabled, this forwarded to L1 which returned "0x" instead of null. Fix by querying the latest block with an out-of-range tx index (0xFFFF) instead. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Niven <sieniven@gmail.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Previously, querying eth_getLogs with a blockHash filter would forward to legacy if the local result was empty. This caused "block not found" errors when the block existed locally but had no matching events, since legacy doesn't hold those blocks.
Fix: check block existence via eth_getBlockByHash before deciding routing.
Also adds unit tests for all three routing branches and an e2e regression test covering both the empty-logs case and a block with ERC20 Transfer events.