feat(l1): add meta entries + muiltiget for faster receipt retrieval#6507
feat(l1): add meta entries + muiltiget for faster receipt retrieval#6507
Conversation
Arkenan
changed the title
Lines of code reportTotal lines added: Detailed view |
🤖 Claude Code ReviewNow I have all the information needed to write a comprehensive review. PR Review:
|
🤖 Kimi Code ReviewOverall Assessment: Critical Issues1. Potential panic on corrupted receipt data let gas_used = receipt.cumulative_gas_used - last_cumulative_gas_used;This subtraction can panic if Fix: Use let gas_used = receipt.cumulative_gas_used.saturating_sub(last_cumulative_gas_used);2. Missing validation between transaction and receipt counts let raw_receipts = storage.get_receipts_by_block_hash(block_hash, tx_count).await?;
// ...
for (index, (tx, receipt)) in body.transactions.iter().zip(raw_receipts.into_iter()).enumerate()If storage corruption causes Fix: Validate lengths match before zipping: if raw_receipts.len() != body.transactions.len() {
return Err(RpcErr::Internal("Receipt/transaction count mismatch".to_owned()));
}Minor Issues3. Race condition on metadata backfill let _ = storage.put_block_receipt_meta(block_hash, &meta).await;Concurrent requests for the same legacy block may trigger redundant backfill writes. While harmless, this wastes I/O. Suggestion: Consider logging backfill occurrences for monitoring, or document that idempotent writes are acceptable here. 4. Unnecessary block hash recalculation let block_hash = header.hash();If Positive Observations
SummaryThe PR is architecturally sound and improves performance significantly. Please address items 1 and 2 (unchecked subtraction and missing length validation) before merging to prevent potential panics or data corruption bugs in edge cases. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
Greptile SummaryThis PR adds a Confidence Score: 5/5Safe to merge; only P2 style findings remain. All remaining findings are P2: one is a stale doc comment (u32 vs u64 in tables.rs) and the other is a minor arithmetic inconsistency (plain subtraction vs saturating_sub in block.rs). Neither affects runtime correctness on valid block data. The core logic — MultiGet batching, meta encoding/decoding, lazy backfill — is correct and well-tested. crates/storage/api/tables.rs (doc comment), crates/networking/rpc/eth/block.rs (subtraction style)
|
| Filename | Overview |
|---|---|
| crates/common/types/receipt.rs | Adds ReceiptMeta struct and build_block_meta helper using saturating_sub/saturating_add; clean implementation with unit tests. |
| crates/storage/api/tables.rs | Adds BLOCK_RECEIPT_META column family constant; doc comment incorrectly says u32 when the actual encoding uses u64 (16 bytes/entry). |
| crates/storage/api/mod.rs | Adds multi_get to StorageReadView with a correct serial fallback default implementation. |
| crates/storage/backend/rocksdb.rs | Implements multi_get via batched_multi_get_cf with sorted_input=false; correct and idiomatic. |
| crates/storage/store.rs | Adds get_receipts_by_block_hash, get_block_receipt_meta, put_block_receipt_meta, and multi_read_async; encodes/decodes ReceiptMeta correctly; unit tests cover roundtrip and edge cases. |
| crates/networking/rpc/eth/block.rs | Refactors get_all_block_rpc_receipts to use get_receipts_by_block_hash; uses plain u64 subtraction where saturating_sub is used in the shared helper, creating a minor inconsistency. |
| crates/networking/rpc/eth/transaction.rs | Implements the O(1) fast path for eth_getTransactionReceipt with lazy backfill of the metadata CF on miss; non-fatal backfill errors are correctly swallowed. |
Sequence Diagram
sequenceDiagram
participant Client
participant RPC as eth_getTransactionReceipt
participant Store
participant RocksDB
Client->>RPC: tx_hash
RPC->>Store: get_transaction_location(tx_hash)
Store-->>RPC: block_hash, index
RPC->>Store: get_block_receipt_meta(block_hash)
alt Meta CF hit - new blocks
Store->>RocksDB: GET block_receipt_meta[block_hash]
RocksDB-->>Store: packed gas_used + log_offset x N
Store-->>RPC: Vec of ReceiptMeta
RPC->>Store: get_receipt_by_block_hash(block_hash, index)
RocksDB-->>RPC: single Receipt
else Meta CF miss - old blocks lazy backfill
Store-->>RPC: None
RPC->>Store: get_receipts_by_block_hash(block_hash, tx_count)
Store->>RocksDB: batched_multi_get_cf receipts keys 0 to N
RocksDB-->>Store: Vec of Receipt
Store-->>RPC: Vec of Receipt
RPC->>RPC: Receipt::build_block_meta
RPC->>Store: put_block_receipt_meta fire and forget
end
RPC->>RPC: build_rpc_receipt_single
RPC-->>Client: RpcReceipt
Prompt To Fix All With AI
This is a comment left during a code review.
Path: crates/storage/api/tables.rs
Line: 41
Comment:
**Incorrect field width in doc comment**
The comment says `u32` for both fields, but the implementation in `store.rs` packs two `u64` values (8 bytes each), giving `BLOCK_RECEIPT_META_ENTRY_LEN = 16`. The comment should read `u64` to match the actual encoding.
```suggestion
/// - Value: packed `[u64 gas_used_le | u64 log_index_offset_le]` * N_transactions
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: crates/networking/rpc/eth/block.rs
Line: 376
Comment:
**Arithmetic inconsistency with `build_block_meta`**
`get_all_block_rpc_receipts` uses plain subtraction here, which will panic in debug builds (and produce wrapping arithmetic in release) if `cumulative_gas_used` ever decreases across receipts. The shared helper `Receipt::build_block_meta` uses `saturating_sub` for exactly this reason — it's worth staying consistent here.
```suggestion
let gas_used = receipt.cumulative_gas_used.saturating_sub(last_cumulative_gas_used);
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Merge branch 'main' into improve-receipt..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /// Per-block receipt metadata column family: [`Vec<u8>`] => [`Vec<u8>`] | ||
| /// - Key: `block_hash.as_bytes()` (32 bytes) | ||
| /// - Value: packed `[u32 gas_used_le | u32 log_index_offset_le]` * N_transactions | ||
| /// |
There was a problem hiding this comment.
The comment says u32 for both fields, but the implementation in store.rs packs two u64 values (8 bytes each), giving BLOCK_RECEIPT_META_ENTRY_LEN = 16. The comment should read u64 to match the actual encoding.
| /// | |
| /// - Value: packed `[u64 gas_used_le | u64 log_index_offset_le]` * N_transactions |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/storage/api/tables.rs
Line: 41
Comment:
**Incorrect field width in doc comment**
The comment says `u32` for both fields, but the implementation in `store.rs` packs two `u64` values (8 bytes each), giving `BLOCK_RECEIPT_META_ENTRY_LEN = 16`. The comment should read `u64` to match the actual encoding.
```suggestion
/// - Value: packed `[u64 gas_used_le | u64 log_index_offset_le]` * N_transactions
```
How can I resolve this? If you propose a fix, please make it concise.| Some(receipt) => receipt, | ||
| _ => return Err(RpcErr::Internal("Could not get receipt".to_owned())), | ||
| }; | ||
| let gas_used = receipt.cumulative_gas_used - last_cumulative_gas_used; |
There was a problem hiding this comment.
build_block_meta
get_all_block_rpc_receipts uses plain subtraction here, which will panic in debug builds (and produce wrapping arithmetic in release) if cumulative_gas_used ever decreases across receipts. The shared helper Receipt::build_block_meta uses saturating_sub for exactly this reason — it's worth staying consistent here.
| let gas_used = receipt.cumulative_gas_used - last_cumulative_gas_used; | |
| let gas_used = receipt.cumulative_gas_used.saturating_sub(last_cumulative_gas_used); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/networking/rpc/eth/block.rs
Line: 376
Comment:
**Arithmetic inconsistency with `build_block_meta`**
`get_all_block_rpc_receipts` uses plain subtraction here, which will panic in debug builds (and produce wrapping arithmetic in release) if `cumulative_gas_used` ever decreases across receipts. The shared helper `Receipt::build_block_meta` uses `saturating_sub` for exactly this reason — it's worth staying consistent here.
```suggestion
let gas_used = receipt.cumulative_gas_used.saturating_sub(last_cumulative_gas_used);
```
How can I resolve this? If you propose a fix, please make it concise.
🤖 Codex Code Review
No other blocking correctness or EVM-consensus issues stood out in the diff. I could not run the targeted tests locally because Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
ElFantasma
left a comment
ElFantasma
left a comment
There was a problem hiding this comment.
Follow-up worth filing: eth_getBlockReceipts (get_all_block_rpc_receipts / get_all_block_receipts) doesn't use the meta CF — both still iterate from scratch. Could short-circuit via the meta CF for blocks that have it.
|
|
||
| /// Per-block receipt metadata column family: [`Vec<u8>`] => [`Vec<u8>`] | ||
| /// - Key: `block_hash.as_bytes()` (32 bytes) | ||
| /// - Value: packed `[u32 gas_used_le | u32 log_index_offset_le]` * N_transactions |
There was a problem hiding this comment.
Doc/impl mismatch: this says Value: packed [u32 gas_used_le | u32 log_index_offset_le], but the implementation uses u64::to_le_bytes() for both fields and BLOCK_RECEIPT_META_ENTRY_LEN = 16 (store.rs:148). Should read [u64 gas_used_le | u64 log_index_offset_le]. (Greptile flagged this.)
| Some(receipt) => receipt, | ||
| _ => return Err(RpcErr::Internal("Could not get receipt".to_owned())), | ||
| }; | ||
| let gas_used = receipt.cumulative_gas_used - last_cumulative_gas_used; |
There was a problem hiding this comment.
saturating_sub vs plain - inconsistency: Receipt::build_block_meta uses saturating_sub for gas_used; this path uses plain -. Same inputs flow through both paths post-backfill, so a hypothetical pathological cumulative-gas sequence saturates in one and panics in the other. Pre-existing rather than introduced here, but worth aligning while you're touching this code.
| let meta = Receipt::build_block_meta(&all); | ||
| // Backfill the CF so the next request takes the fast path. | ||
| // Errors here are non-fatal. | ||
| let _ = storage.put_block_receipt_meta(block_hash, &meta).await; |
There was a problem hiding this comment.
Silent error drop: if RocksDB returns a transient error here, we lose observability and the next request re-derives. tracing::warn! on Err is cheap and non-fatal.
| let all = storage | ||
| .get_receipts_by_block_hash(block_hash, tx_count) | ||
| .await?; | ||
| let meta = Receipt::build_block_meta(&all); |
There was a problem hiding this comment.
Concurrent-request race on lazy backfill: two concurrent GetTransactionReceiptRequests on a pre-meta block both reach this None arm, both compute the same vec, both put_block_receipt_meta. Idempotent so no correctness issue, but flagging for potential thundering-herd patterns on archival queries.
|
|
||
| let results = self | ||
| .db | ||
| .batched_multi_get_cf(&cf, keys.iter().map(|k| k.as_slice()), false); |
There was a problem hiding this comment.
sorted=false is correct for general callers, but in get_receipts_by_block_hash keys are constructed from (block_hash, 0..count) — already sorted by RLP encoding. Passing true lets RocksDB skip the internal sort. Tiny win; not blocking.
| /// Keys share a `block_hash` prefix so RocksDB's MultiGet coalesces them | ||
| /// into ~one SST-block read rather than the `n` independent reads a | ||
| /// `join_all` over `get_receipt_by_block_hash` would do. | ||
| pub async fn get_receipts_by_block_hash( |
There was a problem hiding this comment.
Returns Err if any receipt is missing — a partially-pruned block fails outright in GetTransactionReceiptRequest's None arm and never gets meta backfilled. Probably fine since partial-prune is a separate problem, but worth a thought if pruning lands on the L1 path.
2 participants
Motivation
We saw that the method to retrieve a single receipt was taking too much time (seconds). We traced this to the sequential get of all the receipts of a block to calculate the gas used and the log index.
Description
This PR adds two mechanisms to solve this: