feat(l1): implement debug_preimage RPC endpoint#6699
Conversation
Add debug_preimage which looks up the preimage (original data) for a given keccak hash. Returns null since ethrex does not maintain a preimage store. The endpoint is registered for API compatibility. Part of #6572
|
Lines of code reportTotal lines added: Detailed view |
Call debug_preimage and assert it returns null, since ethrex does not maintain a keccak preimage store.
🤖 Claude Code ReviewHere is my review of PR #6699: PR Review:
|
| Area | Finding |
|---|---|
| Core implementation | Correct, idiomatic, well-tested |
| Dead code in test file | TestEnv + setup_single_transfer_block unused — likely triggers dead_code warnings |
| Test coverage | Good for the stub; minor gap on empty-params case |
| File naming | debug_trace_tests.rs is misleading for what this file currently tests |
The main actionable item is the dead code in the test file. Everything else is minor or cosmetic.
Automated review by Claude (Anthropic) · sonnet · custom prompt
🤖 Kimi Code ReviewOverall Assessment: The PR correctly implements a stub Minor Issues:
Security/Consensus Notes:
Code Quality:
Suggestion: If future plans include actual preimage storage, consider adding a Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
Greptile SummaryThis PR registers
Confidence Score: 4/5Safe to merge — the RPC change is a well-contained stub with no logic risk; the only concern is dead scaffolding code in the test file. The handler itself is trivially correct (parse, discard, return null), the endpoint registration follows the established pattern, and the unit tests cover the expected parse failures. The test file ships a noticeable amount of unused helper infrastructure that will generate compiler warnings and could mislead future contributors about what is actually tested. test/tests/rpc/debug_trace_tests.rs — contains unused scaffolding functions and a name that does not match its content.
|
| Filename | Overview |
|---|---|
| crates/networking/rpc/debug/preimage.rs | New stub RPC handler for debug_preimage; always returns null with proper parameter validation, well-documented rationale, and inline unit tests. |
| crates/networking/rpc/debug/mod.rs | Adds pub mod preimage declaration — one-line, low-risk change. |
| crates/networking/rpc/rpc.rs | Registers debug_preimage in the debug request dispatcher — straightforward, follows existing pattern. |
| test/tests/rpc/debug_trace_tests.rs | Contains a large amount of scaffolding (block-building helpers, TestEnv, transfer-tx utilities, unused constants) that is not exercised by the single preimage test; will produce dead-code warnings and the file name does not match its content. |
| test/tests/rpc/mod.rs | Registers the new debug_trace_tests module — trivial one-line change. |
Sequence Diagram
sequenceDiagram
participant Client
participant RPC as map_debug_requests (rpc.rs)
participant Handler as PreimageRequest (preimage.rs)
Client->>RPC: POST debug_preimage [hash]
RPC->>Handler: parse(params)
alt params missing or wrong count
Handler-->>RPC: Err(RpcErr::BadParams)
RPC-->>Client: JSON-RPC error
else valid H256 hash
Handler-->>RPC: "Ok(PreimageRequest { _hash })"
RPC->>Handler: handle(context)
Handler-->>RPC: Ok(Value::Null)
RPC-->>Client: "{ result: null }"
end
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
test/tests/rpc/debug_trace_tests.rs:65-160
**Unused scaffolding will produce dead-code warnings**
`build_block`, `create_transfer_tx`, `build_and_execute_block`, `setup_single_transfer_block`, `TestEnv`, and the constants `TEST_MAX_FEE_PER_GAS` / `TEST_GAS_LIMIT` are all defined here but referenced only from each other — none are called by the single actual test (`preimage`). Rust will emit `dead_code` warnings for every one of them. If this infrastructure is intended for future trace tests, it would be cleaner to land it alongside the tests that exercise it, or at least annotate it with `#[allow(dead_code)]` with a comment explaining the intent.
### Issue 2 of 2
test/tests/rpc/debug_trace_tests.rs:1
**File name doesn't reflect its content**
The file is named `debug_trace_tests.rs` but the only test in it (`preimage`) covers `debug_preimage`, not any tracing endpoint. Consider renaming it to `debug_preimage_tests.rs` (or a broader `debug_tests.rs` if trace tests will be added here later) to keep the module name self-documenting.
Reviews (1): Last reviewed commit: "test(rpc): add e2e integration test for ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| async fn build_block(store: &Store, blockchain: &Blockchain, parent_header: &BlockHeader) -> Block { | ||
| let args = BuildPayloadArgs { | ||
| parent: parent_header.hash(), | ||
| timestamp: parent_header.timestamp + 12, | ||
| fee_recipient: H160::zero(), | ||
| random: H256::zero(), | ||
| withdrawals: Some(Vec::new()), | ||
| beacon_root: Some(H256::zero()), | ||
| slot_number: None, | ||
| version: 1, | ||
| elasticity_multiplier: ELASTICITY_MULTIPLIER, | ||
| gas_ceil: DEFAULT_BUILDER_GAS_CEIL, | ||
| }; | ||
| let block = create_payload(&args, store, Bytes::new()).unwrap(); | ||
| let result = blockchain.build_payload(block).unwrap(); | ||
| result.payload | ||
| } | ||
|
|
||
| async fn create_transfer_tx( | ||
| chain_id: u64, | ||
| nonce: u64, | ||
| to: Address, | ||
| value: U256, | ||
| signer: &Signer, | ||
| ) -> Transaction { | ||
| let mut tx = Transaction::EIP1559Transaction(EIP1559Transaction { | ||
| chain_id, | ||
| nonce, | ||
| max_priority_fee_per_gas: 0, | ||
| max_fee_per_gas: TEST_MAX_FEE_PER_GAS, | ||
| gas_limit: TEST_GAS_LIMIT, | ||
| to: TxKind::Call(to), | ||
| value, | ||
| data: Bytes::new(), | ||
| ..Default::default() | ||
| }); | ||
| tx.sign_inplace(signer).await.unwrap(); | ||
| tx | ||
| } | ||
|
|
||
| async fn build_and_execute_block( | ||
| store: &Store, | ||
| blockchain: &Blockchain, | ||
| parent_header: &BlockHeader, | ||
| transactions: Vec<Transaction>, | ||
| ) -> Block { | ||
| for tx in &transactions { | ||
| blockchain | ||
| .add_transaction_to_pool(tx.clone()) | ||
| .await | ||
| .expect("tx should enter pool"); | ||
| } | ||
| let block = build_block(store, blockchain, parent_header).await; | ||
| assert_eq!(block.body.transactions.len(), transactions.len()); | ||
| blockchain.add_block(block.clone()).expect("block should be valid"); | ||
| store | ||
| .forkchoice_update(vec![], block.header.number, block.hash(), None, None) | ||
| .await | ||
| .unwrap(); | ||
| block | ||
| } | ||
|
|
||
| async fn rpc_call(store: &Store, method: &str, params: Vec<Value>) -> Value { | ||
| let body = json!({ | ||
| "jsonrpc": "2.0", | ||
| "method": method, | ||
| "params": params, | ||
| "id": 1 | ||
| }); | ||
| let request: RpcRequest = serde_json::from_value(body).expect("valid RPC request"); | ||
| let context = default_context_with_storage(store.clone()).await; | ||
| map_http_requests(&request, context) | ||
| .await | ||
| .expect("RPC call should succeed") | ||
| } | ||
|
|
||
| struct TestEnv { | ||
| store: Store, | ||
| block: Block, | ||
| tx_hash: H256, | ||
| } | ||
|
|
||
| async fn setup_single_transfer_block() -> TestEnv { | ||
| let sk = test_secret_key(); | ||
| let sender = sender_from_key(&sk); | ||
| let signer: Signer = LocalSigner::new(sk).into(); | ||
| let (store, chain_id) = setup_store(sender).await; | ||
| let blockchain = Blockchain::default_with_store(store.clone()); | ||
| let genesis_header = store.get_block_header(0).unwrap().unwrap(); | ||
| let recipient = Address::from_low_u64_be(0xAA); | ||
| let value = U256::from(1_000_000_000_000_000_000u64); | ||
| let tx = create_transfer_tx(chain_id, 0, recipient, value, &signer).await; | ||
| let tx_hash = tx.hash(); | ||
| let block = build_and_execute_block(&store, &blockchain, &genesis_header, vec![tx]).await; | ||
| TestEnv { store, block, tx_hash } | ||
| } |
There was a problem hiding this comment.
build_block, create_transfer_tx, build_and_execute_block, setup_single_transfer_block, TestEnv, and the constants TEST_MAX_FEE_PER_GAS / TEST_GAS_LIMIT are all defined here but referenced only from each other — none are called by the single actual test (preimage). Rust will emit dead_code warnings for every one of them. If this infrastructure is intended for future trace tests, it would be cleaner to land it alongside the tests that exercise it, or at least annotate it with #[allow(dead_code)] with a comment explaining the intent.
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/tests/rpc/debug_trace_tests.rs
Line: 65-160
Comment:
**Unused scaffolding will produce dead-code warnings**
`build_block`, `create_transfer_tx`, `build_and_execute_block`, `setup_single_transfer_block`, `TestEnv`, and the constants `TEST_MAX_FEE_PER_GAS` / `TEST_GAS_LIMIT` are all defined here but referenced only from each other — none are called by the single actual test (`preimage`). Rust will emit `dead_code` warnings for every one of them. If this infrastructure is intended for future trace tests, it would be cleaner to land it alongside the tests that exercise it, or at least annotate it with `#[allow(dead_code)]` with a comment explaining the intent.
How can I resolve this? If you propose a fix, please make it concise.| @@ -0,0 +1,178 @@ | |||
| use std::{fs::File, io::BufReader, path::PathBuf}; | |||
There was a problem hiding this comment.
The file is named debug_trace_tests.rs but the only test in it (preimage) covers debug_preimage, not any tracing endpoint. Consider renaming it to debug_preimage_tests.rs (or a broader debug_tests.rs if trace tests will be added here later) to keep the module name self-documenting.
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/tests/rpc/debug_trace_tests.rs
Line: 1
Comment:
**File name doesn't reflect its content**
The file is named `debug_trace_tests.rs` but the only test in it (`preimage`) covers `debug_preimage`, not any tracing endpoint. Consider renaming it to `debug_preimage_tests.rs` (or a broader `debug_tests.rs` if trace tests will be added here later) to keep the module name self-documenting.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
🤖 Codex Code Review
No EVM/gas-accounting/consensus-critical issues stood out in this diff beyond the API-semantics concern above. I couldn’t run the test suite in this environment: Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
1 participant
Summary
Adds the
debug_preimageRPC endpoint for keccak256 preimage lookup.This is a stub that always returns
null. A full implementation is not currently possible because ethrex does not maintain a keccak preimage store:debug_preimagemaps a keccak256 hash back to its original input (e.g. storage slot keys, address hashes used in the state trie).keccak256(input) → inputmapping during EVM execution, which has significant storage and performance cost.--cache.preimages).Returning
nullis geth-compatible behavior meaning "preimage not available for this hash". The endpoint is registered so that tooling that probes for it gets a well-formed response instead of amethod not founderror.Part of #6572
Test plan
debug_preimagewith any hash returnsnulltrace_preimage)