feat(l1): implement debug_getBadBlocks with bad-block tracking#6948
feat(l1): implement debug_getBadBlocks with bad-block tracking#6948edg-l wants to merge 1 commit into
Conversation
|
🤖 Kimi Code ReviewOverall this is a solid implementation of Issues to address: 1. Race condition in // Current (racy):
let mut bad_blocks = self.get_bad_blocks().await?; // Read
bad_blocks.push(block); // Modify
// ... other thread modifies here ...
self.write_async(..., bad_blocks.encode_to_vec()).await // Write (overwrites other thread)Since this is a debug feature with low concurrency, you may accept this risk, but document it with a 2. Potential panic/error on let rpc_block = RpcBlock::build(block.header, block.body, hash, true)?;For invalid blocks, transaction receipts may not exist in storage (execution failed). If 3. Unnecessary clone on happy path let bad_block_candidate = block.clone();While the comment notes this is "negligible", for a 30M gas block with full blobs, this could be ~100MB+ of memory copying on every Minor suggestions:
Security check:
Verdict: Fix the Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code Review
I did not run the test suite: Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Lines of code reportTotal lines added: Detailed view |
🤖 Claude Code ReviewThat's fine, no cleanup needed since the fetch never succeeded. Here's the review. Review:
|
Greptile SummaryThis PR implements
Confidence Score: 4/5The change is well-scoped and additive; the only correctness concern is a read-modify-write in add_bad_block that can drop one entry under concurrent invalid payloads, which is tolerable for a debug list. The core engine path and RPC handler are correct. The add_bad_block storage function uses an unguarded read-modify-write pattern that can cause a bad block entry to be silently lost when two newPayload failures race — negligible impact for a debug endpoint but worth addressing before this is relied on for diagnostics. crates/storage/store.rs — the add_bad_block read-modify-write and the redundant number+hash dedup predicate.
|
| Filename | Overview |
|---|---|
| crates/networking/rpc/debug/bad_blocks.rs | New RPC handler for debug_getBadBlocks; correctly reads stored bad blocks and serializes them to the geth BadBlockArgs shape with hash, full-transaction RPC block, and 0x-hex RLP. |
| crates/networking/rpc/engine/payload.rs | Clones the block before add_block consumes it and records it on the two INVALID decision points (InvalidBlock, EvmError); ParentNotFound and ParentStateNotFound (SYNCING) are correctly left unrecorded. |
| crates/storage/store.rs | Adds add_bad_block / get_bad_blocks with RLP-encoded bounded list; has an unguarded read-modify-write and a redundant number+hash dedup predicate, but is otherwise correct. |
| crates/storage/api/tables.rs | Adds BAD_BLOCKS column family and bumps the TABLES array from 20 to 21 entries. |
| test/tests/storage/store_tests.rs | Adds test_bad_blocks covering the empty default, descending-order sort, hash dedup, and 16-entry cap with oldest eviction; runs under both in-memory and RocksDB engines. |
| crates/networking/rpc/rpc.rs | Routes debug_getBadBlocks to the new GetBadBlocksRequest handler in the existing debug namespace. |
| crates/networking/rpc/debug/mod.rs | Exposes the new bad_blocks module. |
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
crates/storage/store.rs:1325-1344
**Unguarded read-modify-write on concurrent `newPayload` calls**
`add_bad_block` reads the list, mutates it in memory, and writes it back with no atomic protection. If two concurrent `newPayload` requests both fail (e.g. two different peers submit different invalid blocks at the same time), the second writer overwrites the first writer's appended entry and one bad block is silently lost from the list. For a bounded debug list this is low-impact, but worth acknowledging or documenting as a known limitation.
### Issue 2 of 2
crates/storage/store.rs:1329-1332
The dedup check combines `block_number` and `block_hash`, but block number is already embedded in the block header that the hash is derived from — two blocks with identical hashes necessarily have identical numbers. Deduplicating on `block_hash` alone is sufficient and clearer.
```suggestion
if bad_blocks.iter().any(|b| b.hash() == block_hash) {
```
Reviews (1): Last reviewed commit: "feat(l1): implement debug_getBadBlocks R..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| pub async fn add_bad_block(&self, block: Block) -> Result<(), StoreError> { | ||
| let mut bad_blocks = self.get_bad_blocks().await?; | ||
| let block_number = block.header.number; | ||
| let block_hash = block.hash(); | ||
| if bad_blocks | ||
| .iter() | ||
| .any(|b| b.header.number == block_number && b.hash() == block_hash) | ||
| { | ||
| return Ok(()); | ||
| } | ||
| bad_blocks.push(block); | ||
| bad_blocks.sort_by(|a, b| b.header.number.cmp(&a.header.number)); | ||
| bad_blocks.truncate(MAX_BAD_BLOCKS); | ||
| self.write_async( | ||
| BAD_BLOCKS, | ||
| BAD_BLOCKS_KEY.to_vec(), | ||
| bad_blocks.encode_to_vec(), | ||
| ) | ||
| .await | ||
| } |
There was a problem hiding this comment.
newPayload calls
add_bad_block reads the list, mutates it in memory, and writes it back with no atomic protection. If two concurrent newPayload requests both fail (e.g. two different peers submit different invalid blocks at the same time), the second writer overwrites the first writer's appended entry and one bad block is silently lost from the list. For a bounded debug list this is low-impact, but worth acknowledging or documenting as a known limitation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/storage/store.rs
Line: 1325-1344
Comment:
**Unguarded read-modify-write on concurrent `newPayload` calls**
`add_bad_block` reads the list, mutates it in memory, and writes it back with no atomic protection. If two concurrent `newPayload` requests both fail (e.g. two different peers submit different invalid blocks at the same time), the second writer overwrites the first writer's appended entry and one bad block is silently lost from the list. For a bounded debug list this is low-impact, but worth acknowledging or documenting as a known limitation.
How can I resolve this? If you propose a fix, please make it concise.| if bad_blocks | ||
| .iter() | ||
| .any(|b| b.header.number == block_number && b.hash() == block_hash) | ||
| { |
There was a problem hiding this comment.
block_number and block_hash, but block number is already embedded in the block header that the hash is derived from — two blocks with identical hashes necessarily have identical numbers. Deduplicating on block_hash alone is sufficient and clearer.
| if bad_blocks | |
| .iter() | |
| .any(|b| b.header.number == block_number && b.hash() == block_hash) | |
| { | |
| if bad_blocks.iter().any(|b| b.hash() == block_hash) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/storage/store.rs
Line: 1329-1332
Comment:
The dedup check combines `block_number` and `block_hash`, but block number is already embedded in the block header that the hash is derived from — two blocks with identical hashes necessarily have identical numbers. Deduplicating on `block_hash` alone is sufficient and clearer.
```suggestion
if bad_blocks.iter().any(|b| b.hash() == block_hash) {
```
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!
| /// `debug_getBadBlocks`. The list is bounded to [`MAX_BAD_BLOCKS`] entries, | ||
| /// kept sorted by descending block number, with the oldest dropped once the | ||
| /// bound is exceeded. Duplicate `(number, hash)` entries are ignored. | ||
| pub async fn add_bad_block(&self, block: Block) -> Result<(), StoreError> { |
There was a problem hiding this comment.
Nit: non-atomic so it could lose an entry
In practice it won't happen (bad blocks are rare, simultaneously executing two blocks is also rare even on fast networks like bsc).
| } | ||
| bad_blocks.push(block); | ||
| bad_blocks.sort_by(|a, b| b.header.number.cmp(&a.header.number)); | ||
| bad_blocks.truncate(MAX_BAD_BLOCKS); |
There was a problem hiding this comment.
Eviction semantic worth double-checking against geth. This truncates by descending block number (keeps the 16 highest-numbered bad blocks), whereas geth's bad-block cache is an LRU keyed by insertion time (returns "most recently seen"). Divergence only shows up when a bad block at a lower number arrives after the 16 slots are full — e.g., a losing-fork bad block reaching us after a deep reorg. Under this code it's silently dropped and never appears in debug_getBadBlocks; under geth it enters at the head of the LRU. If tooling that consumes the endpoint assumes "most recent by clock" semantics, the mismatch could confuse it. Might be intentional (highest-number is arguably a cleaner proxy for "newest fork"); worth pinning either the semantic choice in a doc comment or the divergence-vs-geth in the PR description.
3 participants
Description
Implements
debug_getBadBlockswith real bad-block tracking, superseding #6693 (which returned a hardcoded empty array as a placeholder).Blocks rejected during
engine_newPayloadexecution are now persisted and returned by the RPC method, matching geth'sBadBlockArgsresponse shape.Changes
Storage (
ethrex-storage)BAD_BLOCKScolumn family storing a single-keyed, RLP-encodedVec<Block>list.Store::add_bad_block— dedups on(number, hash), sorts by descending block number, and caps the list at 16 entries (oldest evicted).Store::get_bad_blocks— returns the tracked list (empty when unset).Population (
engine/payload.rs)newPayloadINVALID decision points (InvalidBlock,EvmError), alongside the existingset_latest_valid_ancestorcall. These already excludeParentNotFound/ParentStateNotFound, which are SYNCING rather than bad.RPC (
debug/bad_blocks.rs)GetBadBlocksRequestreturns[{ hash, block, rlp }]per entry: the full block JSON (viaRpcBlock) plus the0x-hex block RLP. Routed asdebug_getBadBlocksunder the existingdebugnamespace allowlist.Testing
test/tests/storage/store_tests.rs::test_bad_blockscovers the empty default, descending ordering,(number, hash)dedup, and the 16-entry cap with oldest eviction (runs under both in-memory and rocksdb engines).Scope note
Bad-block recording is wired into the
engine_newPayloadpath — the canonical "bad blocks seen on the network" source per the execution-apis spec. The full-sync batch/pipeline import paths also reject blocks but would require per-block clones on hot loops to record; left out of scope for this change.Closes #6693