fix(l1): distinguish a disabled RPC namespace from an unimplemented method

[ilitteri]

Motivation

A debug_* call against a stock node returns a bare Method not found, which reads as "ethrex does not implement this method". Tooling that probes the standard RPC surface keeps drawing that conclusion — the same failure shape already documented at the top of test/tests/rpc/missing_rpc_methods_tests.rs, where differential testing reported ethrex as lacking functionality it largely already had.

The cause is not the request: --http.api defaults to eth,net,web3, so on a stock node every debug_* request is refused by the namespace allowlist before the handler ever sees its params. Reproduced against a running node — the response is identical with valid params, with empty params, and with no params field at all:

request	response
debug_traceBlockByNumber ["latest"]	-32601 Method not found
debug_traceBlockByNumber []	-32601 Method not found
debug_chainConfig (no params field)	-32601 Method not found

Investigating it surfaced a second, unrelated defect: one handler panics on empty params and drops the connection.

Description

Distinguish "not served here" from "not implemented". Adds RpcErr::MethodNotServedHere { method, reason } for a method this build implements but this endpoint does not serve. The code stays -32601, which is correct per JSON-RPC 2.0 and matches geth's behaviour for a disabled module, so any client keying on the code is unaffected; only the message gains the reason. A genuinely unknown method still returns the bare MethodNotFound, so the two cases are now distinguishable.

debug_traceCall             -32601  Method not found: debug_traceCall (the 'debug' namespace is not
                                    enabled on this endpoint; add it to --http.api)
txpool_content              -32601  Method not found: txpool_content (the 'txpool' namespace is not
                                    enabled on this endpoint; add it to --http.api)
engine_forkchoiceUpdatedV3  -32601  Method not found: engine_forkchoiceUpdatedV3 (the 'engine'
                                    namespace is served on the authenticated RPC port
                                    (--authrpc.port), not on the public HTTP port)
bogus_method                -32601  Method not found: bogus_method

Applied to every sibling refusal, not just the HTTP allowlist: the authenticated port's non-engine/eth rejection, the WebSocket eth_subscribe allowlist guard, and the L2 dispatcher's eth and ethrex guards — the last naming --http.api.ethrex, which is the flag that actually controls it.

engine_* over HTTP is now checked before the allowlist. It previously fell through to the allowlist branch and was told to add engine to --http.api, a flag whose parser rejects engine outright; it now points at --authrpc.port.

RpcNamespace::as_prefix renders the CLI spelling (txpool, not Mempool) and has a round-trip test against from_prefix, so the advice cannot drift into naming something the CLI would refuse.

Fix the panic. debug_executionWitness bounded its params only from above (params.len() > 2) and then indexed params[0], so "params": [] panicked with an index-out-of-bounds inside the connection task — the caller got a dropped connection and no JSON-RPC response at all, and the node logged:

thread 'tokio-rt-worker' panicked at crates/networking/rpc/debug/execution_witness.rs:24:49:
index out of bounds: the len is 0 but the index is 0

Every sibling handler (eth_call, eth_estimateGas, eth_createAccessList, debug_traceCall, engine_forkchoiceUpdated*) already had the is_empty() guard; this one was missing it. Sweeping all 68 dispatched non-engine methods against [], [null], [null,null] and [null,null,null] confirms it was the only method in the whole surface that could be made to drop a connection, and that none can after the fix.

How to Test

# Panic regression — was an empty reply (curl exit 52), now a param error.
cargo run --bin ethrex -- --dev --http.api eth,net,web3,debug --datadir memory
curl -s -X POST -H 'content-type: application/json' \
  --data '{"jsonrpc":"2.0","id":1,"method":"debug_executionWitness","params":[]}' \
  http://localhost:8545

# Namespace message — run without --http.api to get the default allowlist.
cargo run --bin ethrex -- --dev --datadir memory
curl -s -X POST -H 'content-type: application/json' \
  --data '{"jsonrpc":"2.0","id":1,"method":"debug_traceCall","params":[]}' \
  http://localhost:8545

Unit coverage: cargo test -p ethrex-rpc -p ethrex-l2-rpc --lib and cargo test -p ethrex-test --test ethrex_tests.

Out of scope

Param errors return -32000 (RpcErr::BadParams) across the entire RPC surface where JSON-RPC 2.0 specifies -32602. The crate already has an InvalidParams variant at -32602 and the migration is happening spot by spot (#7236 did eth_getLogs); a blanket change touches every namespace and its test expectations, so it is left for its own PR. Related, also untouched: "params": {} returns -32000 "Invalid request body" with "id": "", where the spec wants -32600 with the request's id.

Checklist

• Updated STORE_SCHEMA_VERSION (crates/storage/lib.rs) if the PR includes breaking changes to the Store requiring a re-sync.

[github-actions]

⚠️ Known Issues — intentionally skipped tests

Source: docs/known_issues.md

rpc-compat log-bearing cases excluded

Where: KNOWN_EXCLUDED_TESTS in .github/scripts/check-hive-results.sh counts out
eight hive rpc-compat cases — the four eth_getLogs cases, eth_getBlockReceipts/get-block-receipts-latest,
and three eth_getTransactionReceipt cases. They are exactly the cases whose recorded
response contains at least one log object; every case with an empty log array still runs.
Note this leaves eth_getLogs with no rpc-compat coverage at all, since all four of its
cases are in the set.

Why: ethrex populates blockTimestamp on log objects, as geth, besu, nethermind, reth
and erigon all do. hive's rpc-compat compares responses byte-exactly (jsondiff.FullMatch;
the lenient checkJSONStructure path applies only to cases upstream marks speconly), and
the corpus is pinned to execution-apis d08382ae (2025-02-10), whose recordings predate the
field — it entered the schema in execution-apis#639 and the fixtures in #846 (2026-07-22).
So the extra key cannot match, and this is a property of the pin rather than of the response.

The pin cannot move, and this is not temporary. The pin sits one commit before
execution-apis#627, which moved the test chain to a pre-merge genesis: the current corpus has
~36 proof-of-work blocks before its terminal total difficulty. ethrex does not support
pre-merge chains and will not, so importing that chain.rlp fails at block 1 —
validate_block_header has no pre-London base-fee path. Every revision carrying
blockTimestamp in its fixtures also carries that chain, so there is no revision that
satisfies both. Nor can the corpus be patched locally: rpc-compat's Dockerfile clones
ethereum/execution-apis by hard-coded URL, so the branch buildarg cannot point at a fork.

Coverage: the field itself is pinned by
block_timestamp_is_on_the_log_and_not_on_the_receipt in
crates/networking/rpc/types/receipt.rs, which asserts it is present on each log and absent
from the receipt level.

Removal: delete the entries if ethrex ever gains pre-merge chain import, or if upstream
marks these cases speconly so they are type-checked instead of compared byte-for-byte.

---

The stateless schema id does not identify the encoding

Where: STATELESS_INPUT_SCHEMA_ID in crates/common/types/stateless_ssz.rs.

Upstream keeps the stateless input schema id at 0x1501
(fork_index 0x15 << 8 | revision 0x01) across incompatible body changes. Three
encodings have now shipped under it: tests-zkevm@v0.6.2, then #3248 + #3278,
then #3356, which moved state, codes and public_keys from SszList to
ProgressiveList. ethrex speaks the last one.

The consequence is that the 2-byte prefix cannot be used to detect a stale or
mismatched bundle. A wrong-dialect input is accepted by the id check and then
fails later — in SSZ decode, or on a root that does not match — rather than being
rejected up front for what it is. only_amsterdam_schema_id_decodes therefore
proves less than its name suggests.

Worth raising upstream: a revision field that does not move across a body change
provides no version negotiation at all.

---

ZisK guest program hash changes with the unsync_cell gate

Where: crates/common/types/block.rs, transaction.rs.

The gate on the single-threaded unsync_cell::OnceCell moved from
all(feature = "eip-8025", target_arch = "riscv64") to
all(feature = "zisk", target_arch = "riscv64") when the eip-8025 feature was removed.

The guest ELFs were previously built --features "<zkvm>-build-elf,ci", which never enabled
eip-8025, so they compiled the atomic once_cell variant. bin/zisk/Cargo.toml does enable
ethrex-common/zisk, so the ZisK guest now compiles the unsafe impl Sync cell instead.
That changes the ELF bytes and therefore the program hash and verification key.

This is intended (the guest is single-threaded, so the unsync cell is sound and cheaper), but it
is a VK change rather than a no-op refactor, and the diffstat presents it as a file rename
(eip8025_cell.rs → unsync_cell.rs). Anyone pinning a ZisK VK across this change must
re-register it. The stateless-validator crate now forwards ethrex-common/zisk from its own
zisk feature so the two ZisK guests do not disagree on the cell type.

---

Release signing key is an unprotected repository secret

Where: .github/workflows/tag_release.yaml.

MINISIGN_SECRET_KEY is a plain repository secret. There is no environment: on
finalize-release or dry-run-release-assets, and gh api repos/lambdaclass/ethrex/rulesets
shows only branch-targeted rulesets, so the github.ref_type == 'tag' condition is a workflow
check rather than an enforced boundary: anyone who can push a tag can reach the signing key.

This is a repository-settings change, not a code change, so it is recorded here rather than
fixed in the tree. Recommended:

1. Move MINISIGN_SECRET_KEY / MINISIGN_PASSWORD into a GitHub Environment with required
   reviewers, and add environment: to the two jobs that sign.
2. Add a ruleset targeting refs/tags/v* restricting who may create release tags.

Until then, the compromise of that key is silent and durable: signatures would still verify
against the committed .github/minisign.pub.

[github-actions]

🤖 Kimi Code Review

I'll review this PR diff focusing on correctness, security, performance, and Rust best practices.

Overall Assessment

This is a well-structured PR that improves RPC error messages by distinguishing "method not implemented" from "method exists but namespace is disabled on this endpoint." The changes are defensive and include good test coverage. I found a few issues to address.

---

Issues Found

1. Potential panic in ExecutionWitnessRequest::parse — bounds check is correct but fragile

File: crates/networking/rpc/debug/execution_witness.rs, lines 17-24

The fix for the empty params panic is correct, but the code still indexes params[0] and params[1] later without visible bounds checks. Looking at the parse method:

if params.is_empty() || params.len() > 2 {
    return Err(...);
}
// Later: params[0] and params[1] are used

The early return ensures params.len() >= 1, so params[0] is safe. However, params[1] is used conditionally (only when params.len() == 2). This is correct but relies on the reader verifying the control flow. Consider adding a small comment where params[1] is used, or restructuring to make the safety more obvious.

Minor suggestion: At line 24, the error message says "Expected one or two params" but the check also rejects empty params. The message is accurate (empty is not "one or two"), but could be clearer: "Expected 1 or 2 params, got 0".

2. Redundant Engine arm in match namespace — dead code with misleading comment

File: crates/networking/rpc/rpc.rs, lines 1358-1361

// Unreachable: the guard above already returned. Kept so the match stays
// exhaustive without a catch-all arm that would silently route a
// namespace added later.
RpcNamespace::Engine => Err(engine_not_on_http(&req.method)),

This is technically correct but suboptimal. In Rust, you can use unreachable!() for truly unreachable arms, which documents intent more clearly and allows the compiler to optimize:

RpcNamespace::Engine => unreachable!("engine namespace rejected before allowlist check"),

However, the current approach has a benefit: if someone refactors and removes the early guard, this arm still produces a correct error rather than panicking. The comment explains this tradeoff. This is acceptable as-is, but consider whether unreachable!() with a detailed message would better serve future maintainers.

3. Inconsistent error variant matching in test assertion

File: crates/networking/rpc/rpc.rs, lines 1752-1755

!matches!(
    result,
    Err(RpcErr::MethodNotFound(_) | RpcErr::MethodNotServedHere { .. })
),

This is correct and uses the modern Rust pattern syntax well. However, note that this test (default_allowlist_allows_expected_methods) is checking that methods on the default allowlist are not blocked. The match arm now accepts either variant as "blocked" — but MethodNotServedHere should never occur for these methods if the default allowlist is correct. This is defensive.

Question: Should this be stricter? If a default-allowed method returns MethodNotServedHere, that indicates a bug in the allowlist configuration or namespace detection. The current assertion would miss that. Consider:

// If you want to catch configuration bugs:
assert!(
    !matches!(result, Err(RpcErr::MethodNotFound(_))),
    "default allowlist should route {method}, got {result:?}"
);
// MethodNotServedHere here would indicate a bug and should fail the test

4. Missing Debug derive on RpcErr could affect error handling

File: crates/networking/rpc/utils.rs, lines 28-50

The RpcErr enum derives #[error(...)] from thiserror but I don't see #[derive(Debug)] in the visible diff. thiserror's Error derive also provides Debug, so this is fine. However, verify that RpcErr implements Debug since the tests use {:?} formatting extensively.

Verified: thiserror::Error derives Debug automatically. This is correct.

5. Potential information disclosure in error messages

File: crates/networking/rpc/rpc.rs, lines 1311-1328

The new error messages reveal:

• Which namespaces are available (--http.api)
• Which port serves which namespace (--authrpc.port)
• That a method exists (distinguishing "not served" from "not found")

This is intentional and improves UX, but consider security implications:

1. Method enumeration: An attacker can now distinguish "method doesn't exist" from "method exists but disabled." This aids reconnaissance.

2. Configuration disclosure: Error messages reveal CLI flags and port configuration.

Assessment: This is standard practice in Ethereum clients (Geth, Reth, etc. do similar). The JSON-RPC spec uses -32601 for both, but the message text varies. The information gain for attackers is minimal since namespace structure is public knowledge. Acceptable for this use case.

6. as_prefix returns &'static str — lifetime is correct but consider const

File: crates/networking/rpc/utils.rs, lines 308-320

pub fn as_prefix(self) -> &'static str {

This is correct and efficient. An alternative in modern Rust:

pub const fn as_prefix(self) -> &'static str {

Since this is a simple match with no runtime computation, const fn would allow compile-time evaluation in const contexts. Minor improvement:

pub const fn as_prefix(self) -> &'static str {
    match self {
        RpcNamespace::Engine => "engine",
        // ...
    }
}

7. Test database path collision risk

File: crates/networking/rpc/rpc.rs, lines 1815-1818 (in new test)

let mut storage = Store::new("temp.db", EngineType::InMemory).expect("Failed to create test DB");

Wait — EngineType::InMemory is used, but a path "temp.db" is still provided. Check if InMemory ignores the path or if this could create a file on disk. If tests run in parallel, "temp.db" could collide.

Looking at the existing code, other tests use similar patterns. If InMemory truly uses memory only, the path is irrelevant. Verify this assumption — if InMemory can fall back to disk or if the path is used for any on-disk temp files, use a unique temp directory:

let temp_dir = tempfile::tempdir().unwrap();
let db_path = temp_dir.path().join("test.db");
let mut storage = Store::new(&db_path.to_string_lossy(), EngineType::InMemory).unwrap();

8. RpcNamespace equality comparison in map_http_requests

File: crates/networking/rpc/rpc.rs, line 1344

if namespace == RpcNamespace::Engine {

This requires RpcNamespace: PartialEq. Verify this is derived or implemented. Looking at the enum definition (line 289+), it derives Clone, Copy, Debug, PartialEq, Eq, Hash. Correct.

---

Positive Observations

1. Good test coverage: New tests cover edge cases (empty params, missing params, too many params, namespace blocking, engine routing).

2. Defensive programming: The engine namespace check runs before the allowlist check, preventing misleading error messages.

3. Clear error messages: Operators get actionable guidance instead of generic "Method not found."

4. Round-trip test for namespace names: Prevents drift between CLI parser and error messages.

5. Proper use of thiserror: The new MethodNotServedHere variant uses structured fields rather than string formatting, enabling programmatic inspection.

---

Summary of Recommended Changes

Priority	File	Line	Change
Low	crates/networking/rpc/utils.rs	308	Make as_prefix a const fn
Low	crates/networking/rpc/rpc.rs	1358	Consider unreachable!() instead of dead error arm
Low	crates/networking/rpc/rpc.rs	1752	Consider stricter test assertion (don't accept MethodNotServedHere for allowed methods)
Verify	crates/networking/rpc/rpc.rs	1815	Confirm InMemory store doesn't use the path for temp files

No critical bugs or security vulnerabilities found. The PR is well-constructed and ready to merge with at most minor polish.

---

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

[github-actions]

🤖 Claude Code Review

I've completed a thorough static review of the diff. Compiling wasn't approved, but the changes are small, well-typed, and covered by new unit tests described in the PR. Here's my review.

Review: PR #7255 — distinguish disabled RPC namespace from unimplemented method

Summary

Two independent, well-scoped fixes: (1) a real panic/DoS bug in debug_executionWitness with empty params, and (2) a UX improvement that makes disabled-namespace responses distinguishable from truly-unknown methods while preserving the -32601 code for wire compatibility. Both are implemented cleanly and are well covered by new tests.

Correctness

• execution_witness.rs:20 — the params.is_empty() || params.len() > 2 guard correctly fixes the out-of-bounds panic on params[0] (line 27). This was a legitimate connection-dropping DoS on a public-facing RPC endpoint (any caller could crash the connection task with {"params": []}), so this is a good find and a correct fix.
• map_http_requests (rpc.rs:1358) — moving the Engine check before the allowlist lookup is correct: engine can never appear in allowed_namespaces from the CLI (rejected by the value parser), so the old code path was dead in practice and gave misleading advice ("add engine to --http.api") in the defensive/programmatic-construction case. The retained RpcNamespace::Engine => Err(engine_not_on_http(...)) arm is unreachable given the early return, but keeping it for match-exhaustiveness (rather than a catch-all) is a reasonable defensive choice — worth a brief unreachable!()-style comment call-out, which the code already has.
• map_authrpc_requests (rpc.rs:1373) — splitting Ok(_) (known-but-unserved namespace → MethodNotServedHere) from Err(rpc_err) (unparseable method → forwarded as-is) is correct and is actually a small behavior improvement: previously an Err from req.namespace() was silently discarded and replaced with a fresh MethodNotFound, which happened to carry the same data, so no observable regression.
• RpcNamespace::as_prefix() is verified as the exact inverse of from_prefix via a round-trip test (utils.rs tests) — good, this is exactly the kind of invariant that quietly rots otherwise.
• The -32601 code is preserved for MethodNotServedHere, matching the PR's stated compatibility goal (verified by the namespace_not_enabled_serializes_as_method_not_found test). This is consistent with geth's behavior for a disabled module.

Minor observations (non-blocking)

• rpc.rs:1662 (message string): "the 'debug' namespace is not enabled..." — message construction via format! + namespace.as_prefix() is correct for every namespace routed through namespace_not_enabled, including Testing, whose doc comment says it "must never be exposed on public-facing RPC APIs" — but the CLI does allow testing via --http.api (confirmed in cmd/ethrex/cli.rs tests), so the advice is technically accurate even if operationally discouraged. Not a bug, just worth being aware the same generic message applies uniformly.
• Slight duplication between the thiserror #[error(...)] string on MethodNotServedHere (utils.rs) and the manually-formatted message in the From<RpcErr> for RpcErrorMetadata impl — both say "Method not found: {method} ({reason})". This mirrors the pre-existing pattern for MethodNotFound, so it's consistent with the codebase's existing style rather than a new issue.
• get_error_kind (rpc.rs:405) correctly adds a static-string arm for the new variant, avoiding metrics cardinality blowup from the free-form reason field.

Test coverage

Good breadth: round-trip test for as_prefix/from_prefix, panic-regression tests for debug_executionWitness (empty/missing/valid/over-limit params), dispatcher-level tests for the HTTP allowlist, the authenticated port, the L2 eth/ethrex guards, and the engine-on-HTTP redirection message — each asserting both the error variant and that the message contains the expected flag/namespace name.

Verdict

No correctness, security, or consensus-relevant issues found. The panic fix is a genuine (low-severity but real) DoS fix and is correctly scoped; the namespace-message change is careful to preserve the JSON-RPC error code for backward compatibility. Good to merge as-is.

---

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

[github-actions]

🤖 Codex Code Review

1. crates/l2/networking/rpc/rpc.rs:371, crates/networking/rpc/rpc.rs:1325, crates/l2/networking/rpc/rpc.rs:81: engine_* requests on the L2 RPC path now inherit the L1 error "served on the authenticated RPC port (--authrpc.port)", but the L2 node explicitly does not start an Auth-RPC server. That turns a previously generic -32601 into incorrect operator guidance. The L2 dispatcher should override this case or use a transport-neutral message instead of delegating to the L1 engine_not_on_http() wording.

2. crates/networking/rpc/rpc.rs:1325, crates/networking/rpc/rpc.rs:877: the engine_not_on_http() message is also reused for WebSocket requests because the WS upgrade handler routes through map_http_requests(). A WS client hitting engine_* will be told the method is “not on the public HTTP port”, which is inaccurate on both merged and standalone WS listeners. This is low severity, but the new error text should be endpoint-aware if the goal is precise diagnostics.

Beyond that, the debug_executionWitness empty-params guard in crates/networking/rpc/debug/execution_witness.rs:13 looks correct and is a real robustness fix: it closes a panic path without affecting EVM/state logic or gas/accounting behavior.

I could not run the targeted Rust tests here because cargo/rustup attempted toolchain access and failed under the current filesystem/network restrictions.

---

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

[github-actions]

Lines of code report

Total lines added: 192
Total lines removed: 0
Total lines changed: 192

Detailed view

+---------------------------------------------------------+-------+------+
| File                                                    | Lines | Diff |
+---------------------------------------------------------+-------+------+
| ethrex/crates/l2/networking/rpc/rpc.rs                  | 410   | +10  |
+---------------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/debug/execution_witness.rs | 135   | +45  |
+---------------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/rpc.rs                     | 1708  | +90  |
+---------------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/utils.rs                   | 418   | +47  |
+---------------------------------------------------------+-------+------+