feat(l1): implement debug_getBadBlocks with bad-block tracking#6949
feat(l1): implement debug_getBadBlocks with bad-block tracking#6949edg-l wants to merge 1 commit into
Conversation
|
🤖 Kimi Code ReviewReview Summary The PR implements 1. Race Condition in Bad Block Storage (Critical)File: The let mut bad_blocks = self.get_bad_blocks().await?; // Read
// ... modify list ...
self.write_async(BAD_BLOCKS, ...).await // WriteIf two concurrent Recommendation: Add a mutex to serialize updates: // In Store struct definition:
bad_blocks_lock: tokio::sync::Mutex<()>,
// In add_bad_block:
let _guard = self.bad_blocks_lock.lock().await;
let mut bad_blocks = self.get_bad_blocks().await?;
// ... rest of logicAlternatively, document this as a known limitation for the debug endpoint, though explicit synchronization is preferred for correctness. 2. Unnecessary Clone on Success Path (Minor)File: The block is cloned unconditionally before execution to preserve it for potential error handling. While the comment notes this is negligible compared to execution, you can avoid the clone in the success case by restructuring: // Instead of cloning upfront:
match add_block(context, block.clone(), ...).await {
Err(e) => { /* use block */ }
Ok(_) => { /* wasted clone */ }
}
// Consider refactoring add_block to take &Block if possible,
// or accept the clone given it's debug-only and rareGiven 3. Error Handling GranularityFile: If let rpc_block = match RpcBlock::build(block.header, block.body, hash, true) {
Ok(b) => b,
Err(e) => {
tracing::warn!(%hash, "Failed to build RPC block for bad block: {}", e);
continue;
}
};4. Storage Key Constant VisibilityFile:
5. Test Coverage (Good)The test in
VerdictApprove with changes requested - Please address the race condition in The EVM integration points look correct: bad blocks are only recorded for validation failures ( Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
|
Closing — pushing the commit directly to glamsterdam-devnet-6 instead. |
🤖 Codex Code Review
Outside of those points, the change looks straightforward. I did not find EVM/gas-accounting or consensus-rule regressions in the touched paths. Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Lines of code reportTotal lines added: Detailed view |
🤖 Claude Code ReviewReview:
|
Greptile SummaryThis PR implements
Confidence Score: 3/5The block-tracking and routing logic is correct, but two defects in the storage and handler layers should be addressed before merging. The non-atomic read-modify-write in
|
| Filename | Overview |
|---|---|
| crates/storage/store.rs | Adds add_bad_block and get_bad_blocks using a single-key RLP-encoded list; the read-modify-write in add_bad_block is not atomic and can silently drop entries under concurrent newPayload calls. |
| crates/networking/rpc/debug/bad_blocks.rs | New handler for debug_getBadBlocks; a single RpcBlock::build failure in the loop aborts the entire response, hiding all other stored bad blocks from the caller. |
| crates/networking/rpc/engine/payload.rs | Adds bad_block_candidate clone before add_block and calls add_bad_block in the two INVALID error branches (InvalidBlock, EvmError); SYNCING and internal-error branches are correctly excluded. |
| crates/storage/api/tables.rs | Adds BAD_BLOCKS column-family constant and bumps the TABLES array size from 20 to 21; straightforward and correct. |
| crates/networking/rpc/rpc.rs | Routes debug_getBadBlocks to GetBadBlocksRequest::call; no issues. |
| test/tests/storage/store_tests.rs | Adds test_bad_blocks covering empty store, out-of-order insertion, deduplication, and the 16-entry eviction bound; good coverage of the happy path. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant CL as Consensus Layer
participant RPC as engine_newPayload
participant Chain as add_block
participant Store as BAD_BLOCKS table
CL->>RPC: engine_newPayload(block)
RPC->>RPC: clone block as bad_block_candidate
RPC->>Chain: add_block(block)
alt InvalidBlock or EvmError
Chain-->>RPC: Err INVALID
RPC->>Store: add_bad_block(bad_block_candidate)
Note over Store: read, dedup, push, sort desc, truncate 16, write
RPC-->>CL: PayloadStatus INVALID
else ParentNotFound or ParentStateNotFound
Chain-->>RPC: Err SYNCING
RPC-->>CL: PayloadStatus SYNCING
else success
Chain-->>RPC: Ok witness
RPC-->>CL: PayloadStatus VALID
end
CL->>RPC: debug_getBadBlocks
RPC->>Store: get_bad_blocks
Store-->>RPC: Vec of Block
loop each Block
RPC->>RPC: hash + encode_to_vec + RpcBlock build
end
RPC-->>CL: array of hash block rlp
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant CL as Consensus Layer
participant RPC as engine_newPayload
participant Chain as add_block
participant Store as BAD_BLOCKS table
CL->>RPC: engine_newPayload(block)
RPC->>RPC: clone block as bad_block_candidate
RPC->>Chain: add_block(block)
alt InvalidBlock or EvmError
Chain-->>RPC: Err INVALID
RPC->>Store: add_bad_block(bad_block_candidate)
Note over Store: read, dedup, push, sort desc, truncate 16, write
RPC-->>CL: PayloadStatus INVALID
else ParentNotFound or ParentStateNotFound
Chain-->>RPC: Err SYNCING
RPC-->>CL: PayloadStatus SYNCING
else success
Chain-->>RPC: Ok witness
RPC-->>CL: PayloadStatus VALID
end
CL->>RPC: debug_getBadBlocks
RPC->>Store: get_bad_blocks
Store-->>RPC: Vec of Block
loop each Block
RPC->>RPC: hash + encode_to_vec + RpcBlock build
end
RPC-->>CL: array of hash block rlp
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
crates/storage/store.rs:1327-1346
**Non-atomic read-modify-write race condition**
`add_bad_block` reads the current list, mutates it in memory, then writes it back — all without any mutual exclusion. If two `engine_newPayload` calls arrive concurrently for different invalid blocks, both tasks can read the same initial list, produce diverging in-memory mutations, and the last `write_async` wins, silently dropping one entry. The scenario is uncommon in practice (CLs usually serialise newPayload) but is a real data-loss path for any concurrent caller. A tokio `Mutex` guarding the read-modify-write cycle (or wrapping the entire operation in a storage-level transaction if the backend supports it) would close the window.
### Issue 2 of 3
crates/networking/rpc/debug/bad_blocks.rs:29-37
**Single failing bad block silences the entire endpoint**
`RpcBlock::build(…)?` propagates any conversion error with `?`, aborting the loop immediately. If even one stored bad block produces an `RpcBlock` build failure (e.g., because of a field that `RpcBlock` cannot represent, or a future schema change), the whole `debug_getBadBlocks` response becomes an internal error and all other stored bad blocks are invisible to the caller. Consider skipping the failing entry with a `warn!` log (matching geth's best-effort approach) so a single corrupt entry cannot break the endpoint for all callers.
### Issue 3 of 3
crates/storage/store.rs:1331-1333
The deduplication check calls `b.hash()` (a keccak256 over the full block header) on every element of the stored list on each `add_bad_block` invocation. For a list capped at 16 entries this is acceptable, but the work is avoidable: the caller already has `block_hash` in scope — storing it alongside the block (e.g., as a `(Block, H256)` tuple) would let you dedup with a simple equality comparison.
```suggestion
// Consider storing the hash alongside each block to avoid re-hashing here.
if bad_blocks
.iter()
.any(|b| b.header.number == block_number && 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.
add_bad_block reads the current list, mutates it in memory, then writes it back — all without any mutual exclusion. If two engine_newPayload calls arrive concurrently for different invalid blocks, both tasks can read the same initial list, produce diverging in-memory mutations, and the last write_async wins, silently dropping one entry. The scenario is uncommon in practice (CLs usually serialise newPayload) but is a real data-loss path for any concurrent caller. A tokio Mutex guarding the read-modify-write cycle (or wrapping the entire operation in a storage-level transaction if the backend supports it) would close the window.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/storage/store.rs
Line: 1327-1346
Comment:
**Non-atomic read-modify-write race condition**
`add_bad_block` reads the current list, mutates it in memory, then writes it back — all without any mutual exclusion. If two `engine_newPayload` calls arrive concurrently for different invalid blocks, both tasks can read the same initial list, produce diverging in-memory mutations, and the last `write_async` wins, silently dropping one entry. The scenario is uncommon in practice (CLs usually serialise newPayload) but is a real data-loss path for any concurrent caller. A tokio `Mutex` guarding the read-modify-write cycle (or wrapping the entire operation in a storage-level transaction if the backend supports it) would close the window.
How can I resolve this? If you propose a fix, please make it concise.| for block in bad_blocks { | ||
| let hash = block.hash(); | ||
| let rlp = format!("0x{}", hex::encode(block.encode_to_vec())); | ||
| let rpc_block = RpcBlock::build(block.header, block.body, hash, true)?; | ||
| results.push(BadBlock { | ||
| hash, | ||
| block: rpc_block, | ||
| rlp, | ||
| }); |
There was a problem hiding this comment.
RpcBlock::build(…)? propagates any conversion error with ?, aborting the loop immediately. If even one stored bad block produces an RpcBlock build failure (e.g., because of a field that RpcBlock cannot represent, or a future schema change), the whole debug_getBadBlocks response becomes an internal error and all other stored bad blocks are invisible to the caller. Consider skipping the failing entry with a warn! log (matching geth's best-effort approach) so a single corrupt entry cannot break the endpoint for all callers.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/networking/rpc/debug/bad_blocks.rs
Line: 29-37
Comment:
**Single failing bad block silences the entire endpoint**
`RpcBlock::build(…)?` propagates any conversion error with `?`, aborting the loop immediately. If even one stored bad block produces an `RpcBlock` build failure (e.g., because of a field that `RpcBlock` cannot represent, or a future schema change), the whole `debug_getBadBlocks` response becomes an internal error and all other stored bad blocks are invisible to the caller. Consider skipping the failing entry with a `warn!` log (matching geth's best-effort approach) so a single corrupt entry cannot break the endpoint for all callers.
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.
b.hash() (a keccak256 over the full block header) on every element of the stored list on each add_bad_block invocation. For a list capped at 16 entries this is acceptable, but the work is avoidable: the caller already has block_hash in scope — storing it alongside the block (e.g., as a (Block, H256) tuple) would let you dedup with a simple equality comparison.
| if bad_blocks | |
| .iter() | |
| .any(|b| b.header.number == block_number && b.hash() == block_hash) | |
| // Consider storing the hash alongside each block to avoid re-hashing here. | |
| if bad_blocks | |
| .iter() | |
| .any(|b| b.header.number == block_number && b.hash() == block_hash) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/storage/store.rs
Line: 1331-1333
Comment:
The deduplication check calls `b.hash()` (a keccak256 over the full block header) on every element of the stored list on each `add_bad_block` invocation. For a list capped at 16 entries this is acceptable, but the work is avoidable: the caller already has `block_hash` in scope — storing it alongside the block (e.g., as a `(Block, H256)` tuple) would let you dedup with a simple equality comparison.
```suggestion
// Consider storing the hash alongside each block to avoid re-hashing here.
if bad_blocks
.iter()
.any(|b| b.header.number == block_number && 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!
1 participant
Cherry-pick of #6948 onto
glamsterdam-devnet-6.Implements
debug_getBadBlockswith real bad-block tracking: blocks rejected duringengine_newPayloadare persisted in a newBAD_BLOCKSstorage table (deduped on(number, hash), sorted descending, capped at 16) and returned as[{ hash, block, rlp }]matching geth's shape.Cherry-picked cleanly (auto-merge on
rpc.rs/store.rs); builds and thetest_bad_blocksstorage test passes on this branch.See #6948 for full details.