feat(l1): implement debug_getBadBlocks RPC endpoint

[azteca1998]

Summary

Adds the debug_getBadBlocks RPC endpoint as specified in the execution-apis OpenRPC spec.

This is a stub that always returns []. A full implementation is not currently possible because ethrex lacks the required storage infrastructure:

• Geth's response requires the full block RLP, header, body, and a human-readable error reason per bad block entry.
• ethrex only stores (bad_block_hash → latest_valid_hash) in the INVALID_CHAINS table (used by the fork-choice handler). It does not persist the full block data or the validation error when a block is rejected.
• Implementing this properly would require:
  1. A new storage table / ring buffer for bad block metadata (full block + error string).
  2. Writes at every block rejection site (engine fork-choice, full sync, etc.).
  3. An eviction policy (geth keeps the last N entries).

Returning [] is spec-valid — it's the correct response for a node that has not encountered any bad blocks. The endpoint is registered so that tooling that probes for it (e.g. Blockscout, hive tests) gets a well-formed response instead of a method not found error.

Part of #6572

Test plan

• debug_getBadBlocks returns []
• Calling with unexpected params returns error
• e2e integration test (trace_get_bad_blocks)

[github-actions]

⚠️ Known Issues — intentionally skipped tests

Source: docs/known_issues.md

Known Issues

Tests intentionally excluded from CI. Source of truth for the Known
Issues section the L1 workflow appends to each ef-tests job summary
and posts as a sticky PR comment.

EF Tests — Stateless coverage narrowed to EIP-8025 optional-proofs

make -C tooling/ef_tests/blockchain test calls test-stateless-zkevm
instead of test-stateless. The zkevm@v0.3.3 fixtures are filled against
bal@v5.6.1, out of sync with current bal spec; the broad target trips ~549
fixtures. Re-broaden once the zkevm bundle is regenerated.

Why and resolution path

PR #6527 broadened
test-stateless to extract the entire for_amsterdam/ tree from the
zkevm bundle and run all of it under --features stateless; combined with
this branch's bal-devnet-7 semantics that scope produces ~549
GasUsedMismatch / ReceiptsRootMismatch /
BlockAccessListHashMismatch failures.

test-stateless-zkevm filters cargo to the eip8025_optional_proofs
suite, which still validates the stateless harness without the bal-version
mismatch.

Re-broaden by switching test: back to test-stateless in
tooling/ef_tests/blockchain/Makefile once the zkevm bundle is regenerated
against the current bal spec.

[github-actions]

🤖 Kimi Code Review

The implementation is correct and follows the existing patterns in the codebase. A few minor observations:

crates/networking/rpc/debug/get_bad_blocks.rs

1. Line 17: The handle method is async but performs no async operations. This is unavoidable given the RpcHandler trait signature, but consider using std::future::ready if the trait ever adds a non-async variant for simple getters.

2. Line 20: The comment accurately documents Geth's behavior and the rationale for returning an empty array. Consider adding a doc comment (///) to the GetBadBlocksRequest struct itself for consistency with other RPC handlers.

Correctness verification:

• Parameter validation correctly rejects non-empty param arrays while accepting null or [] (lines 9-14)
• Return type Value::Array(vec![]) matches Geth's expected response format for healthy nodes
• The endpoint is properly registered in map_debug_requests (rpc.rs:1075)

Security: No concerns. This is a read-only endpoint returning static data with no side effects.

The implementation appropriately acknowledges that ethrex doesn't currently track bad blocks (which would require maintaining a ring buffer of failed validations), and returning an empty array is the valid response per Geth's API specification.

---

Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

[greptile-apps]

Greptile Summary

This PR adds the debug_getBadBlocks RPC endpoint as a well-documented stub that always returns [], which is spec-valid while ethrex lacks the storage infrastructure to track rejected blocks.

• get_bad_blocks.rs: New RpcHandler implementation — correctly rejects non-empty params, returns an empty JSON array, and ships with focused unit tests covering the three parse paths and the handler response.
• rpc.rs: Routes debug_getBadBlocks in map_debug_requests and updates the doc-comment to include the new method alongside existing debug_chainConfig and debug_getBlockAccessList entries.
• debug_trace_tests.rs: Adds an integration test for the endpoint, but also includes a large block of helper functions (build_block, create_transfer_tx, build_and_execute_block, setup_single_transfer_block, TestEnv) that are unused by any test in this PR and will emit dead-code warnings.

Confidence Score: 5/5

Safe to merge — the change adds a spec-valid stub endpoint that has no side effects and cannot corrupt state.

The core implementation is a no-op handler (always returns []) with clear documentation of why a full implementation isn't yet possible. The routing, param validation, and unit tests are all correct. The only rough edge is unused scaffolding code in the new test file that will produce compiler warnings, but it does not affect runtime behaviour.

test/tests/rpc/debug_trace_tests.rs — contains dead helper functions that will produce compiler warnings.

Important Files Changed

Filename	Overview
crates/networking/rpc/debug/get_bad_blocks.rs	New stub handler for debug_getBadBlocks; correctly rejects params and returns an empty array; includes thorough unit tests.
crates/networking/rpc/debug/mod.rs	Adds get_bad_blocks module declaration; trivial one-line change, no issues.
crates/networking/rpc/rpc.rs	Registers GetBadBlocksRequest import and routes debug_getBadBlocks in the match arm; doc-comment updated to reflect new and existing methods.
test/tests/rpc/debug_trace_tests.rs	Integration test file for debug_getBadBlocks; contains unused helper functions (setup_single_transfer_block, build_block, create_transfer_tx, build_and_execute_block, TestEnv) that are dead code and will produce compiler warnings.
test/tests/rpc/mod.rs	Registers the new debug_trace_tests module; trivial change, no issues.

Sequence Diagram

sequenceDiagram
    participant Client
    participant map_http_requests
    participant map_debug_requests
    participant GetBadBlocksRequest

    Client->>map_http_requests: POST /rpc (debug_getBadBlocks)
    map_http_requests->>map_debug_requests: "route debug_* method"
    map_debug_requests->>GetBadBlocksRequest: call(req, context)
    GetBadBlocksRequest->>GetBadBlocksRequest: parse(params) — rejects non-empty params
    GetBadBlocksRequest->>GetBadBlocksRequest: handle() — returns []
    GetBadBlocksRequest-->>map_debug_requests: Ok(Value::Array([]))
    map_debug_requests-->>map_http_requests: Ok(Value::Array([]))
    map_http_requests-->>Client: "{"jsonrpc":"2.0","result":[],"id":1}"

Loading

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
test/tests/rpc/debug_trace_tests.rs:65-160
**Unused scaffolding generates dead-code warnings**

`build_block`, `create_transfer_tx`, `build_and_execute_block`, `setup_single_transfer_block`, the `TestEnv` struct, `TEST_MAX_FEE_PER_GAS`, and `TEST_GAS_LIMIT` are all defined but never called by any `#[test]` function in this file. The Rust compiler will emit `dead_code` warnings for each of them, and if the project treats warnings as errors in CI these will become build failures. If these helpers are pre-emptive scaffolding for upcoming trace tests, adding `#[allow(dead_code)]` at the top of the file (or moving them into a private module gated by that attribute) will silence the warnings until the tests arrive.

_{Reviews (2): Last reviewed commit: "fix(rpc): update map_debug_requests doc-..." | Re-trigger Greptile}

[github-actions]

🤖 Claude Code Review

Here is my review of PR #6693:

---

PR Review: feat(l1): implement debug_getBadBlocks RPC endpoint

Overview

Adds the debug_getBadBlocks RPC endpoint as specified in the execution-apis OpenRPC spec. The implementation is a correct stub that returns an empty array, matching the valid behavior of a healthy node that hasn't encountered bad blocks. Three files changed, zero deletions — clean and minimal.

---

Code Quality

Follows existing patterns exactly. The parse() method in get_bad_blocks.rs:8-18 is a verbatim copy of chain_config.rs's parse logic (same let-else guard, same error message format). This is good consistency.

Parameter validation logic (get_bad_blocks.rs:9-11):

if let Some(params) = params && !params.is_empty()

This accepts both None and Some([]) as valid "no params" — identical to ChainConfigRequest. This is intentional and matches how Geth handles parameterless methods.

The _context prefix on handle's parameter (get_bad_blocks.rs:20) is correct for now, but when this is extended to read from storage, it will need to drop the underscore.

---

Issues

1. Doc comment on map_debug_requests is stale (rpc.rs:1055-1060)

The doc comment lists all handled methods but omits debug_getBadBlocks. Since this function is public and the comment serves as an index, it should be updated:

/// - Block info: `debug_getBadBlocks`

2. No unit tests added

The PR description has a test plan checkboxes but no corresponding test code. Other no-param handlers like debug_chainConfig also lack dedicated unit tests in rpc.rs, so this is not strictly a regression, but given the test plan was written, it would be worth adding at minimum:

• A test confirming the handler returns [] when called with no params.
• A test confirming BadParams error when params are non-empty (the existing BadParams path is exercised by the test plan but not in a committed test).

3. Existing INVALID_CHAINS storage is not leveraged — and that's fine, but worth a comment

store.rs:1136-1143 already tracks bad block hashes via set_latest_valid_ancestor / INVALID_CHAINS. A future implementer might try to reuse this, but that table only stores (bad_block_hash → latest_valid_hash) — it does not store full block data. The spec requires each entry in the response to include the full block object and RLP. The existing comment in get_bad_blocks.rs:21-23 covers the intent well. Consider a small note pointing to the storage gap so a future implementer knows they'll need a separate ring buffer store, not just querying INVALID_CHAINS.

---

Spec Compliance

The execution-apis spec defines the return type as an array of objects with block (full block object) and hash fields. An empty array [] is a valid response and unambiguously correct for a node with no tracked bad blocks. No issues here.

---

Summary

The implementation is correct, minimal, and consistent with existing patterns. The two actionable items before merging:

• Update the map_debug_requests doc comment to include debug_getBadBlocks.
• Add the unit tests described in the test plan (or at least tick the boxes after manual verification).

Everything else is clean.

---

Automated review by Claude (Anthropic) · sonnet · custom prompt

[github-actions]

🤖 Codex Code Review

1. crates/networking/rpc/debug/get_bad_blocks.rs:21-24 returns [] unconditionally, which is a semantic correctness bug for a compatibility RPC. debug_getBadBlocks in Geth means “show me the bad blocks you observed,” not “return an empty list when the feature is unimplemented.” Ethrex already records invalid-chain information in crates/storage/store.rs:1131-1156 for Engine/ForkChoice handling, so the node can in fact encounter and remember invalid blocks. With the new dispatch in crates/networking/rpc/rpc.rs:1075, callers will now get a false “healthy” result even after bad payloads were seen. I’d either implement this from stored invalid-block metadata (or add a dedicated ring buffer with the required details), or leave the method unsupported instead of fabricating an empty success response.

No other correctness, security, or performance issues stood out in this diff. The change is mechanically small and follows the existing RPC handler pattern; the main problem is the misleading behavior of the new endpoint.

---

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

[github-actions]

Lines of code report

Total lines added: 51
Total lines removed: 0
Total lines changed: 51

Detailed view

+------------------------------------------------------+-------+------+
| File                                                 | Lines | Diff |
+------------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/debug/get_bad_blocks.rs | 48    | +48  |
+------------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/debug/mod.rs            | 5     | +1   |
+------------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/rpc.rs                  | 1303  | +2   |
+------------------------------------------------------+-------+------+

[github-actions]

🤖 Kimi Code Review

The implementation is correct and follows the established patterns in the codebase. Below are specific observations:

crates/networking/rpc/debug/get_bad_blocks.rs

Correctness: The stub implementation is appropriate. Per Geth's specification, returning an empty array is valid behavior for a healthy node that hasn't encountered bad blocks.

Code Quality:

• Line 9-12: The let-chains syntax (if let Some(params) = params && !params.is_empty()) is idiomatic modern Rust.
• Line 20: The underscore prefix on _context correctly indicates the parameter is intentionally unused.

test/tests/rpc/debug_trace_tests.rs

File Organization:

• Issue: The filename debug_trace_tests.rs suggests tracing-specific tests, but debug_getBadBlocks is not a tracing method. Consider renaming to debug_tests.rs or splitting into logical groupings.
• Unused Code: Lines 147-161 define setup_single_transfer_block() and the TestEnv struct, but they appear unused in the shown diff. If this is scaffolding for future tests, add a TODO comment; otherwise, remove dead code to keep the PR focused.

Test Efficiency:

• The get_bad_blocks test (lines 164-175) initializes a full genesis state and blockchain, but the endpoint returns a constant empty array regardless of context. While acceptable for integration testing, consider adding a lighter unit test in get_bad_blocks.rs that doesn't require blockchain setup, or document that this tests the RPC routing layer specifically.

Minor:

• Line 106: The database path "store.db" is misleading since EngineType::InMemory is used. Consider using "test.db" or an empty string for clarity.

Security & Consensus

No security issues identified. The endpoint is read-only, performs no state access, and has no consensus implications.

Summary

The PR is ready to merge pending resolution of the test file naming/organization issues. The core implementation correctly implements the Geth-compatible stub behavior.

---

Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

[github-actions]

🤖 Codex Code Review

1. crates/networking/rpc/debug/get_bad_blocks.rs:20-24
debug_getBadBlocks is exposed as a real method but always returns []. That is not just “unimplemented”; it reports “no bad blocks” even if ethrex has actually rejected invalid blocks during import. For debugging/fork-diagnosis tooling, that is a misleading success result. If bad-block tracking does not exist yet, I would prefer returning MethodNotFound or an explicit internal “not supported” error until the node maintains actual bad-block state.

2. test/tests/rpc/debug_trace_tests.rs:1-159
   The new integration test file pulls in a large amount of unused block-building/signing scaffolding for a method that only needs a minimal RPC context. That makes this test unnecessarily brittle and slower to compile: unrelated changes in payload construction, signing, or block execution can break debug_getBadBlocks coverage even though the RPC handler itself is trivial. This should be reduced to a focused test, ideally in a file named for the method under test.

No consensus/EVM/gas-accounting/security issues are introduced by the dispatcher wiring in [crates/networking/rpc/rpc.rs:1062-1077] itself.

I could not run the tests locally because cargo test failed in this environment when rustup tried to create temp files on a read-only filesystem.

---

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

[github-actions]

🤖 Claude Code Review

Now I have all the information needed to write the review.

PR Review: feat(l1): implement debug_getBadBlocks RPC endpoint

Summary

This is a well-reasoned stub implementation. The PR description is thorough, the rationale for returning [] is sound and spec-valid, and the code is small and focused. A few things worth addressing:

---

crates/networking/rpc/debug/get_bad_blocks.rs

The implementation is correct. The parse() let-chain idiom is idiomatic Rust. The comment explaining why [] is returned is a good addition — leave it.

One minor redundancy: in the tests module, use super::* already captures all names in scope from the parent module (because use crate::{..., RpcHandler} is in the parent). In Rust, glob-importing a child module brings in the parent's re-exports but not private use imports — so the explicit use crate::RpcHandler; on line 33 is actually necessary. No change needed.

---

test/tests/rpc/debug_trace_tests.rs

Several issues here:

1. Unnecessary async on build_block

build_block contains no .await expressions — it's synchronous code inside an async fn. This compiles fine but is misleading and forces every caller to .await it unnecessarily.

// line 58 — should be:
fn build_block(store: &Store, blockchain: &Blockchain, parent_header: &BlockHeader) -> Block {

2. Dead code — unused scaffolding will produce compiler warnings

TestEnv, setup_single_transfer_block, build_block, build_and_execute_block, and create_transfer_tx are all unused by any #[test] function in this file. The only actual test is get_bad_blocks, which calls setup_store directly. Rustc will emit dead_code warnings for all of these in the integration-test binary.

Options:

• Remove the scaffolding now and add it back when trace tests are written.
• Add #[allow(dead_code)] at the module level with a comment explaining these are forward scaffolding (not ideal — it suppresses all future dead-code warnings too).
• Write at least one placeholder test using the scaffolding to keep it alive.

The cleanest choice is to move the scaffolding into a future PR alongside the trace tests it supports.

3. Misleading file name

debug_trace_tests.rs implies tests for debug_traceTransaction / debug_traceBlockByNumber, but the only test here is for debug_getBadBlocks. Suggest renaming to debug_tests.rs, or placing the get_bad_blocks test in an existing debug test file if one exists.

4. Double blank line before get_bad_blocks (cosmetic)

Line 163 has two consecutive blank lines before the test function. Minor style nit.

---

crates/networking/rpc/rpc.rs

Doc-comment update is accurate and the routing addition is correct. No issues.

---

Ethereum / spec correctness

• Returning [] from debug_getBadBlocks is spec-valid. The execution-apis spec defines the return type as Array<BadBlock> with no requirement to have entries.
• Parameter rejection for non-empty params is consistent with how other zero-parameter debug methods are handled in this codebase.
• No gas accounting, trie, or consensus logic is touched — the surface area is intentionally minimal.

---

Overall

The core implementation (get_bad_blocks.rs, mod.rs, rpc.rs) is clean and ready to merge. The main ask before merging is addressing the dead-code scaffolding in debug_trace_tests.rs — either strip it out or write a tracking comment, since unused code in integration tests produces CI noise and can confuse future readers about what is actually tested.

---

Automated review by Claude (Anthropic) · sonnet · custom prompt