feat(l1): implement debug_traceBlockByHash RPC endpoint

[azteca1998]

Summary

• Adds debug_traceBlockByHash which traces all transactions in a block identified by its hash
• Extracts shared block tracing logic into a trace_block helper used by both traceBlockByNumber and traceBlockByHash, eliminating code duplication
• Supports all existing tracers: callTracer, prestateTracer, opcodeTracer

Closes part of #6572

Test plan

• Verify debug_traceBlockByHash returns same results as debug_traceBlockByNumber for the same block
• Test with each tracer type (callTracer, prestateTracer, opcodeTracer)
• Test with invalid/unknown block hash returns proper error

[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 PR correctly implements debug_traceBlockByHash by refactoring the tracing logic into a shared trace_block helper. This reduces code duplication and follows DRY principles effectively.

Code Quality & Correctness:

1. Good refactoring: Extracting trace_block (lines 226-332 in the new file) to share logic between TraceBlockByNumberRequest and TraceBlockByHashRequest is clean and maintainable.

2. Import grouping (tracing.rs, line 13): The import statement can be slightly cleaned up for readability:

   // Current:
   use crate::{rpc::RpcApiContext, rpc::RpcHandler, types::block_identifier::BlockIdentifier, utils::RpcErr};
   
   // Suggested:
   use crate::{
       rpc::{RpcApiContext, RpcHandler},
       types::block_identifier::BlockIdentifier,
       utils::RpcErr,
   };

3. Parameter validation (tracing.rs, lines 374-376): The validation logic correctly handles optional tracer config:

   if params.len() != 1 && params.len() != 2 {
       return Err(RpcErr::BadParams("Expected 1 or 2 params".to_owned()));
   };

   This properly enforces the method signature.

Security & Robustness:

4. Error propagation: The use of ? in trace_block properly propagates serde_json::Error to RpcErr. Ensure that RpcErr implements From<serde_json::Error> (which appears to be the case given existing usage).

5. Block lookup (tracing.rs, lines 394-398): Correctly handles the case where the block hash doesn't exist:

   .ok_or(RpcErr::Internal("Block not Found".to_string()))?;

Performance:

6. Memory efficiency: The trace_block function takes trace_config by reference (&TraceConfig), avoiding unnecessary cloning of the config struct across the async boundary. This is appropriate since TraceConfig is only used for reading configuration values.

Overall:
The implementation is correct and follows existing patterns in the codebase. The refactoring successfully eliminates ~100 lines of duplicated tracing logic while maintaining identical behavior for debug_traceBlockByNumber and adding the new debug_traceBlockByHash endpoint.

---

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

[github-actions]

🤖 Claude Code Review

Here is the full review for PR #6692:

---

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

Overview

This PR adds the debug_traceBlockByHash JSON-RPC endpoint and refactors the existing debug_traceBlockByNumber implementation by extracting a shared trace_block async helper. The result is a net reduction of ~110 lines with the two endpoints sharing all tracing logic.

---

Code Quality

Positive observations:

• The refactoring is clean and well-motivated. Extracting trace_block eliminates a large block of copy-pasted match arms, and the new helper is correctly scoped as a private module function.
• Borrow conventions in trace_block(block, &self.trace_config, &context) are consistent — block is moved (matching existing trace_block_* call patterns), while config and context are borrowed.
• TraceBlockByHashRequest::parse correctly mirrors the parameter-count validation pattern of TraceBlockByNumberRequest (1 required + 1 optional config param).
• Error propagation with ? is used consistently; no silent swallowing of errors.

---

Issues and Suggestions

1. "Block not Found" error should use RpcErr::InvalidParams or a block-specific error, not RpcErr::Internal

In tracing.rs (handle for TraceBlockByHashRequest, ~line 398):

.ok_or(RpcErr::Internal("Block not Found".to_string()))?;

Same pattern exists in TraceBlockByNumberRequest. An unknown hash supplied by the caller is not an internal error — it should map to something like RpcErr::InvalidParams or a dedicated RpcErr::BlockNotFound. Using Internal here causes the JSON-RPC response to carry a 500-level semantic for what is really a 404/InvalidParams condition. This makes it harder for callers to distinguish a bug from a bad request. Worth fixing here and in the existing handler.

2. Missing #[derive(Deserialize, Default)] consistency with TraceBlockByNumberRequest

The new struct (line ~31 in tracing.rs):

pub struct TraceBlockByHashRequest {
    block_hash: H256,
    trace_config: TraceConfig,
}

TraceBlockByNumberRequest carries #[derive(Deserialize)] (visible in the context of the original file). While the parse() approach means serde is not strictly needed, the inconsistency could confuse future readers. At minimum a comment explaining why there is no Default (a hash is mandatory) would be useful.

3. emit variable is constructed but cfg is moved into trace_block_opcodes

In the OpcodeTracer arm of trace_block (~line 292–307):

let emit = StructLoggerEmit {
    mem_size: cfg.enable_memory,
    return_data: cfg.enable_return_data,
    refund: false,
};
let opcode_traces = context.blockchain.trace_block_opcodes(block, reexec, timeout, cfg).await ...

cfg is moved into trace_block_opcodes, so emit must be built first — this is correct. However emit is then used only inside the closure. The code is safe but the ordering is non-obvious. A short comment or structuring emit closer to its use site would improve readability.

4. No test coverage added

The PR description lists test-plan items that are all unchecked. There are no automated tests (unit or integration) validating that:

• debug_traceBlockByHash returns the same result as debug_traceBlockByNumber for the same block.
• An unknown hash returns a proper error.
• All three tracer variants work correctly via the hash path.

Given that the shared trace_block helper now powers both endpoints, a regression in trace_block would silently break both. Even a single integration test comparing hash-based and number-based traces for a known fixture block would significantly improve confidence.

5. context.storage.get_block_by_hash — confirm this returns the canonical block

let block = context.storage.get_block_by_hash(self.block_hash).await?

Ethereum clients can have non-canonical blocks in storage (uncle/ommer blocks, or reorged blocks that were stored before being pruned). debug_traceBlockByHash in geth only traces blocks on the canonical chain. If get_block_by_hash can return non-canonical blocks, executing them against the canonical state trie would yield incorrect or misleading results, and could panic/error due to missing parent state. Worth verifying that get_block_by_hash is canonical-only, or adding a canonicality check.

---

Minor Nits

• The inline comment on TraceBlockByNumberRequest::handle that previously said "geth-RPC shape expected by debug_traceBlockByNumber consumers" was updated to the generic form — good catch.
• No unused imports or dead code introduced.

---

Summary

The refactoring goal is achieved cleanly, and the new endpoint follows all existing patterns. The two items worth addressing before merge are: the RpcErr::Internal misuse for a missing block (semantic correctness) and the canonicality question for get_block_by_hash (potential correctness/security issue). Adding at least a basic integration test would be valuable given the shared code path. Everything else is minor.

---

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

[github-actions]

🤖 Codex Code Review

No correctness or security findings in the RPC implementation itself. The new debug_traceBlockByHash handler in tracing.rs delegates into the same shared trace_block path used by debug_traceBlockByNumber, so EVM execution, gas accounting, and tracer serialization stay aligned with the existing block-trace behavior. The route wiring in rpc.rs is also straightforward.

Residual gaps:

1. Test coverage is missing for the new RPC surface. The new dispatch entry is in rpc.rs and the new parser/handler starts at tracing.rs, but I didn’t find coverage for routing, param validation, or parity with debug_traceBlockByNumber.
2. The bundled REPL is out of sync: tooling/repl/src/commands/debug.rs still exposes traceBlockByNumber only, so debug_traceBlockByHash is not reachable there unless that omission is intentional.

I couldn’t run a full compile/test pass in this environment because cargo attempted to write through rustup to a read-only filesystem.

---

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

[github-actions]

Lines of code report

Total lines added: 195
Total lines removed: 0
Total lines changed: 195

Detailed view

+-----------------------------------------+-------+------+
| File                                    | Lines | Diff |
+-----------------------------------------+-------+------+
| ethrex/crates/networking/rpc/rpc.rs     | 1302  | +1   |
+-----------------------------------------+-------+------+
| ethrex/crates/networking/rpc/tracing.rs | 509   | +194 |
+-----------------------------------------+-------+------+

[greptile-apps]

Greptile Summary

This PR adds debug_traceBlockByHash, which traces all transactions in a block identified by its hash, and eliminates the duplicate tracing logic that previously lived in TraceBlockByNumberRequest::handle by extracting it into a shared trace_block async helper. As a side-effect, the block-not-found error in TraceBlockByNumberRequest is improved from RpcErr::Internal to RpcErr::BadParams.

• New endpoint (tracing.rs, rpc.rs): TraceBlockByHashRequest parses a 32-byte block hash plus an optional TraceConfig, resolves the block via storage.get_block_by_hash, and delegates to the new trace_block helper — which already supports callTracer, prestateTracer, and opcodeTracer.
• Refactor (tracing.rs): ~120 lines of duplicated match logic are replaced by the trace_block free function shared between the two TraceBlockBy* handlers.
• REPL + tests (debug.rs, debug_trace_tests.rs): A new traceBlockByHash REPL command is registered, and an integration test builds a real block with a transfer transaction and verifies the callTracer response shape end-to-end.

Confidence Score: 5/5

The change is straightforward — new endpoint wired into an existing router, shared helper extracted from already-working code, and block-not-found errors made more accurate. No storage writes, no auth changes, no new async state.

The refactoring is mechanical and the logic is well-covered by the existing test suite for traceBlockByNumber plus the new integration test. The only finding is a cosmetic description mismatch in the REPL help text that has no runtime impact.

No files require special attention.

Important Files Changed

Filename	Overview
crates/networking/rpc/tracing.rs	Introduces TraceBlockByHashRequest and refactors shared block tracing into a trace_block helper; also improves block-not-found error from RpcErr::Internal to RpcErr::BadParams in TraceBlockByNumberRequest.
crates/networking/rpc/rpc.rs	Wires debug_traceBlockByHash into the debug request router alongside existing debug endpoints.
tooling/repl/src/commands/debug.rs	Adds traceBlockByHash REPL command using HASH_OPTIONS, but the shared HASH_OPTIONS param description says "Transaction hash" — misleading when applied to traceBlockByHash which accepts a block hash.
test/tests/rpc/debug_trace_tests.rs	New integration test for debug_traceBlockByHash that builds a real block with a transfer transaction and validates the callTracer response shape end-to-end.
test/tests/rpc/mod.rs	Adds debug_trace_tests module to the test suite.

Sequence Diagram

sequenceDiagram
    participant C as Client
    participant R as RPC Router
    participant H as TraceBlockByHashRequest
    participant S as Storage
    participant T as trace_block()
    participant BC as Blockchain

    C->>R: debug_traceBlockByHash(hash, config?)
    R->>H: parse(params)
    H-->>R: TraceBlockByHashRequest
    R->>H: handle(context)
    H->>S: get_block_by_hash(hash)
    S-->>H: Block (or None → BadParams)
    H->>T: trace_block(block, config, context)
    alt callTracer
        T->>BC: trace_block_calls(...)
        BC-->>T: "Vec<(H256, CallTrace)>"
    else prestateTracer
        T->>BC: trace_block_prestate(...)
        BC-->>T: "Vec<(H256, PrestateResult)>"
    else opcodeTracer
        T->>BC: trace_block_opcodes(...)
        BC-->>T: "Vec<(H256, OpcodeTrace)>"
    end
    T-->>H: serde_json::Value
    H-->>C: JSON response

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
tooling/repl/src/commands/debug.rs:19-26
The `HASH_OPTIONS` constant is shared between `traceTransaction` and `traceBlockByHash`, but its first parameter description says "Transaction hash". When the REPL renders help for `traceBlockByHash`, users will see "Transaction hash" for a parameter that actually expects a block hash. Consider splitting this into separate constants so each command can have an accurate description.

```suggestion
const TX_HASH_OPTIONS: &[ParamDef] = &[
    ParamDef {
        name: "hash",
        param_type: ParamType::Hash,
        required: true,
        default_value: None,
        description: "Transaction hash",
    },
```

_{Reviews (2): Last reviewed commit: "style: run cargo fmt on test and source ..." | Re-trigger Greptile}

[github-actions]

🤖 Kimi Code Review

The PR implements debug_traceBlockByHash by refactoring shared logic into a trace_block() helper and adds comprehensive tests. Overall, this is a clean, well-structured change.

Code Quality & Correctness

1. Good refactoring: Extracting trace_block() (lines 221-335) eliminates duplication between TraceBlockByNumberRequest and TraceBlockByHashRequest. The logic for handling CallTracer, PrestateTracer, and OpcodeTracer is now centralized and consistent.

2. Correct error categorization: Changing "Block not Found" errors from RpcErr::Internal to RpcErr::BadParams (lines 364, 369, 419) is semantically correct—a missing block due to user-supplied parameters is a client error (4xx), not a server error (5xx).

3. Parameter validation: TraceBlockByHashRequest::parse correctly validates parameter count (lines 384-387) and handles optional TraceConfig (lines 388-393), matching the behavior of TraceBlockByNumberRequest.

Testing

The new unit tests (lines 434-621) cover parsing edge cases for all three request types, and the integration test (debug_trace_tests.rs) validates the full round-trip. This is excellent coverage for RPC handlers.

Minor Observations

• Line 243-250: The CallTracer branch assumes each transaction produces at least one frame (trace.into_iter().next().ok_or(...)). This is safe for valid transactions (every tx has a top-level call), but the error message "Empty call trace" will correctly catch any anomalous behavior from the underlying tracer.

• Line 1078: The new RPC method is correctly registered in map_debug_requests.

Conclusion

The code is correct, follows Rust best practices, and improves maintainability through DRY principles. The error handling fix and test coverage are particularly good additions. No blocking issues found.

---

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

[github-actions]

🤖 Codex Code Review

No correctness, security, or consensus-risk findings in the implementation itself. debug_traceBlockByHash is a thin storage lookup plus reuse of the existing block-tracing path, and the refactor in crates/networking/rpc/tracing.rs appears behavior-preserving for debug_traceBlockByNumber.

Minor items:

• Low: crates/networking/rpc/rpc.rs still documents the debug tracing methods as debug_traceTransaction and debug_traceBlockByNumber only. Add debug_traceBlockByHash so the method inventory does not drift.
• Low: test/tests/rpc/debug_trace_tests.rs and debug_trace_tests.rs capture tx_hash in the fixture but never assert that the returned txHash matches it. As written, a regression that reorders traces or associates the wrong hash to a trace would still pass.

I could not run the tests in this environment: cargo test failed because rustup could not create temp files on the read-only filesystem. Static review aside, the patch looks good.

---

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

[github-actions]

🤖 Claude Code Review

Here is the review:

---

PR Review: feat(l1): implement debug_traceBlockByHash (#6692)

Overview

This PR adds the debug_traceBlockByHash RPC endpoint by extracting a shared trace_block helper from the existing debug_traceBlockByNumber implementation, eliminating significant code duplication. It also upgrades error classification for block-not-found cases and adds parse-level unit tests plus an integration test.

---

Correctness

Error classification fix is good. Changing RpcErr::Internal("Block not Found") to RpcErr::BadParams("Block not Found") in TraceBlockByNumberRequest::handle (lines 367 and 372) is semantically correct — a missing block is a client-side bad input, not a server-side failure. The same classification is used consistently in TraceBlockByHashRequest::handle (line 405). This aligns with the pattern in eth/block.rs:211.

get_block_by_hash returns any known block, not just canonical. TraceBlockByHashRequest calls storage.get_block_by_hash(self.block_hash) directly. If the hash belongs to an uncle or a reorged-out block that was stored, this would trace a non-canonical block. Geth's implementation typically checks if the block is on the canonical chain and falls back to re-executing from a checkpoint when it isn't. Whether this is a concern depends on whether the store retains non-canonical blocks — worth documenting or protecting against with a canonicality check.

Doc comment at rpc.rs:1059 is stale. The comment only lists debug_traceTransaction and debug_traceBlockByNumber; the new method should be added:

/// - Tracing: `debug_traceTransaction`, `debug_traceBlockByNumber`, `debug_traceBlockByHash`

---

Code Quality

Refactoring is clean. The extracted trace_block(block, config, context) helper is correct — all three tracer branches are identical to what was there before. The function is correctly kept private.

TraceBlockByHashRequest struct missing derives. Unlike TraceBlockByNumberRequest and TraceTransactionRequest, the new struct has no #[derive(...)] attribute at all. Looking at the other request structs these aren't needed either (parse is manual), so this is consistent — just noting it isn't an oversight.

Repeated "Block not Found" string. The same string literal appears in three places (lines 367, 372, 405). A module-level constant would be cleaner:

const ERR_BLOCK_NOT_FOUND: &str = "Block not Found";

Pre-existing typo in comment. Line 17: re-excute → re-execute. Not introduced here, but easy to fix in passing.

---

Test Coverage

Unit tests are thorough for parsing. Good coverage of happy paths, missing params, too-many params, default tracer, and config deserialization for all three request types. PrestateTracerConfig::validate tests are a nice addition.

Integration test covers the basic happy path. The trace_block_by_hash test in debug_trace_tests.rs verifies the response shape for a callTracer result.

Missing: cross-validation with traceBlockByNumber. The PR test plan explicitly lists this as a goal: "Verify debug_traceBlockByHash returns same results as debug_traceBlockByNumber for the same block." This test is not implemented. It's a straightforward addition — call both endpoints with the same block, assert the responses are equal.

Missing: non-default tracers in integration test. The test only exercises callTracer (the default). A test with prestateTracer or opcodeTracer would improve confidence that the dispatch path works end-to-end for all tracer types, not just parse-level.

Missing: multi-transaction block. Only a single-transaction block is tested. The iteration logic in trace_block (especially the .next() unwrap for callTracer) would benefit from a 2+ transaction block test.

---

Summary

The core implementation is correct and the refactoring is a clear improvement. The main gaps are:

1. No cross-validation test (traceBlockByHash result == traceBlockByNumber result for same block), which was explicitly called out in the test plan.
2. The stale doc comment in rpc.rs:1059.
3. Potential canonicality concern with get_block_by_hash — worth either a note or a guard depending on storage semantics.

---

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