feat(l1): improve rlp import#6666
Conversation
|
edg-l
changed the title
Lines of code reportTotal lines added: Detailed view |
edg-l
force-pushed
the
feat/import-bench-bal-tooling
branch
from
d09b663 to
5c087ae
Compare
🤖 Kimi Code ReviewOverall Assessment: The PR introduces BAL (Block Access List) export/import functionality for benchmark tooling and replaces a fixed sleep with proper synchronization for persistence idle detection. The logic is generally sound, but there are blocking I/O issues in async contexts and missing validation that could lead to benchmarking with incorrect data. Critical Issues1. Blocking I/O in Async Context (cmd/ethrex/cli.rs)
Fix: Use Correctness & Validation2. Missing BAL Hash Validation (cmd/ethrex/cli.rs:1004) let bal = if block.header.block_access_list_hash.is_some() {
let b = preloaded_bals.as_ref().and_then(|bals| bals.get(bal_index));
// TODO: Verify b.hash() matches block.header.block_access_list_hash
bal_index += 1;
b
} else {
None
};Fix: Verify the hash matches before using the preloaded BAL, and return an error if validation fails. 3. Silent Index Mismatch (cmd/ethrex/cli.rs:1004) Fix: Return an explicit error if 4. Panic on Decode Error (cmd/ethrex/cli.rs:943-944) Fix: Use Code Quality5. Incorrect Error Message (crates/storage/store.rs:1434) Current: StoreError::Custom(format!("failed to read new trie layer notification: {e}"))Fix: Change "read" to "send". 6. Memory Usage for Large Chains (cmd/ethrex/cli.rs:983) Positive Aspects
Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
Greptile SummaryThis PR replaces the
Confidence Score: 3/5Safe to merge for single-file bench runs; directory-based imports with --with-bal will silently feed wrong BALs to every chain after the first The store.rs changes are clean and the ping/rendezvous mechanism is correctly implemented. The only issue lives in cli.rs: bal_index resets to zero at the start of each chain in the multi-chain loop, so any directory-based bench run using --with-bal will correlate BALs from chain 1 to blocks in chain 2+, producing silently incorrect parallel-execution inputs. The typical single-file workflow is unaffected, but the fix is a one-line variable hoist. cmd/ethrex/cli.rs — specifically the bal_index declaration and the out-of-bounds fallback in the block loop
|
| Filename | Overview |
|---|---|
| cmd/ethrex/cli.rs | Adds --export-bal/--with-bal flags and ping-based persistence wait to import-bench; bal_index resets per-chain causing BAL misalignment for directory imports |
| crates/storage/store.rs | Introduces TrieMessage enum and wait_for_persistence_idle; rendezvous-channel ping semantics are correct and the change is well-isolated from production paths |
Sequence Diagram
sequenceDiagram
participant Bench as import_blocks_bench
participant Pipeline as add_block_pipeline[_bal]
participant Store as Store (sync send)
participant Worker as trie-update worker thread
Bench->>Pipeline: execute block N
Pipeline->>Store: send TrieMessage::Update (rendezvous blocks)
Store-->>Worker: worker receives Update, begins Phase 1+2+3
Store-->>Pipeline: send returns (Phase 1 accepted)
Pipeline-->>Bench: returns Ok
Bench->>Store: wait_for_persistence_idle() spawn_blocking(tx.send(Ping))
Note over Store,Worker: Ping send blocks until worker loops back to recv()
Worker-->>Worker: Phase 2 disk write completes
Worker-->>Worker: Phase 3 in-memory removal completes
Worker-->>Store: worker calls recv() picks up Ping
Store-->>Bench: send returns persistence idle confirmed
Bench->>Pipeline: execute block N+1
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
cmd/ethrex/cli.rs:979-989
`bal_index` is declared inside the `for blocks in chains` loop, so it resets to `0` at the start of every chain. When the input is a directory (multiple chain files), chains 2, 3, … will each re-read BALs starting from the beginning of `preloaded_bals` instead of continuing from where the previous chain left off. This means BALs will be assigned to the wrong blocks for every chain after the first, silently producing incorrect parallel-execution inputs.
```suggestion
let mut exported_bals = Vec::new();
let mut total_blocks_imported = 0;
let mut bal_index = 0usize;
for blocks in chains {
let size = blocks.len();
let mut numbers_and_hashes = blocks
.iter()
.map(|b| (b.header.number, b.hash()))
.collect::<Vec<_>>();
// Execute block by block
let mut last_progress_log = Instant::now();
```
### Issue 2 of 2
cmd/ethrex/cli.rs:1020-1026
When `--with-bal` is active but `bal_index` has grown past the end of `preloaded_bals` (e.g. the BAL file was exported from a shorter run, or there is a miscount between runs), `bals.get(bal_index)` silently returns `None` and the block is executed on the sequential path. No warning is logged, so a user relying on this bench to test parallel execution would get no indication that the BAL-parallel path was never exercised.
```suggestion
let bal = if block.header.block_access_list_hash.is_some() {
let b = preloaded_bals.as_ref().and_then(|bals| {
let entry = bals.get(bal_index);
if entry.is_none() {
warn!(block = number, bal_index, "BAL file exhausted; falling back to sequential execution for this block");
}
entry
});
bal_index += 1;
b
} else {
None
};
```
Reviews (1): Last reviewed commit: "perf(l1): replace import-bench inter-blo..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -938,6 +986,7 @@ pub async fn import_blocks_bench( | |||
| .collect::<Vec<_>>(); | |||
| // Execute block by block | |||
| let mut last_progress_log = Instant::now(); | |||
| let mut bal_index = 0usize; | |||
There was a problem hiding this comment.
bal_index is declared inside the for blocks in chains loop, so it resets to 0 at the start of every chain. When the input is a directory (multiple chain files), chains 2, 3, … will each re-read BALs starting from the beginning of preloaded_bals instead of continuing from where the previous chain left off. This means BALs will be assigned to the wrong blocks for every chain after the first, silently producing incorrect parallel-execution inputs.
| let mut exported_bals = Vec::new(); | |
| let mut total_blocks_imported = 0; | |
| let mut bal_index = 0usize; | |
| for blocks in chains { | |
| let size = blocks.len(); | |
| let mut numbers_and_hashes = blocks | |
| .iter() | |
| .map(|b| (b.header.number, b.hash())) | |
| .collect::<Vec<_>>(); | |
| // Execute block by block | |
| let mut last_progress_log = Instant::now(); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: cmd/ethrex/cli.rs
Line: 979-989
Comment:
`bal_index` is declared inside the `for blocks in chains` loop, so it resets to `0` at the start of every chain. When the input is a directory (multiple chain files), chains 2, 3, … will each re-read BALs starting from the beginning of `preloaded_bals` instead of continuing from where the previous chain left off. This means BALs will be assigned to the wrong blocks for every chain after the first, silently producing incorrect parallel-execution inputs.
```suggestion
let mut exported_bals = Vec::new();
let mut total_blocks_imported = 0;
let mut bal_index = 0usize;
for blocks in chains {
let size = blocks.len();
let mut numbers_and_hashes = blocks
.iter()
.map(|b| (b.header.number, b.hash()))
.collect::<Vec<_>>();
// Execute block by block
let mut last_progress_log = Instant::now();
```
How can I resolve this? If you propose a fix, please make it concise.| let bal = if block.header.block_access_list_hash.is_some() { | ||
| let b = preloaded_bals.as_ref().and_then(|bals| bals.get(bal_index)); | ||
| bal_index += 1; | ||
| b | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
--with-bal is active but bal_index has grown past the end of preloaded_bals (e.g. the BAL file was exported from a shorter run, or there is a miscount between runs), bals.get(bal_index) silently returns None and the block is executed on the sequential path. No warning is logged, so a user relying on this bench to test parallel execution would get no indication that the BAL-parallel path was never exercised.
| let bal = if block.header.block_access_list_hash.is_some() { | |
| let b = preloaded_bals.as_ref().and_then(|bals| bals.get(bal_index)); | |
| bal_index += 1; | |
| b | |
| } else { | |
| None | |
| }; | |
| let bal = if block.header.block_access_list_hash.is_some() { | |
| let b = preloaded_bals.as_ref().and_then(|bals| { | |
| let entry = bals.get(bal_index); | |
| if entry.is_none() { | |
| warn!(block = number, bal_index, "BAL file exhausted; falling back to sequential execution for this block"); | |
| } | |
| entry | |
| }); | |
| bal_index += 1; | |
| b | |
| } else { | |
| None | |
| }; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: cmd/ethrex/cli.rs
Line: 1020-1026
Comment:
When `--with-bal` is active but `bal_index` has grown past the end of `preloaded_bals` (e.g. the BAL file was exported from a shorter run, or there is a miscount between runs), `bals.get(bal_index)` silently returns `None` and the block is executed on the sequential path. No warning is logged, so a user relying on this bench to test parallel execution would get no indication that the BAL-parallel path was never exercised.
```suggestion
let bal = if block.header.block_access_list_hash.is_some() {
let b = preloaded_bals.as_ref().and_then(|bals| {
let entry = bals.get(bal_index);
if entry.is_none() {
warn!(block = number, bal_index, "BAL file exhausted; falling back to sequential execution for this block");
}
entry
});
bal_index += 1;
b
} else {
None
};
```
How can I resolve this? If you propose a fix, please make it concise.
🤖 Claude Code ReviewI have enough to write a thorough review now. Review: feat(l1): improve rlp importOverall: The persistence-idle wait mechanism is the star of this PR — clean design, sound reasoning, properly implemented. The BAL round-trip plumbing is mostly correct for the primary single-chain use case but has a meaningful scope bug for directory imports, plus a few silent failure modes worth tightening.
|
| Severity | Item |
|---|---|
| Bug | bal_index resets to 0 per chain — multi-chain directory imports pair BALs incorrectly |
| Moderate | Silent BAL under-supply — no warning when BAL file has fewer entries than Amsterdam+ blocks |
| Low | Unused bal/bal_index computation in export branch — confusing, should be moved into else |
| Low | expect() panics rather than returning errors through the existing Result chain |
| Low | Blocking std::fs I/O directly on the async executor at startup/shutdown |
| Trivial | Stale error message text on TrieMessage::Update send |
Automated review by Claude (Anthropic) · sonnet · custom prompt
🤖 Codex Code ReviewFindings
Open Questions
Summary I did not run Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
edg-l
force-pushed
the
feat/import-bench-bal-tooling
branch
from
5c087ae to
87f5a24
Compare
edg-l
force-pushed
the
feat/import-bench-bal-tooling
branch
from
87f5a24 to
8192dd1
Compare
Add tooling to benchmark the BAL parallel execution path
(execute_block_pipeline) against sequential execution in a
reproducible way.
- cmd/ethrex/cli.rs: two new import-bench flags:
- --export-bal <FILE>: sequential pass via add_block_pipeline_bal,
writes a single concatenated RLP file with one BAL per block.
- --with-bal <FILE>: loads BALs from disk (in-memory) and routes
each block through add_block_pipeline with the pre-loaded BAL,
activating the parallel path. BALs are matched to Amsterdam+
blocks by header block_access_list_hash presence.
The two flags conflict, making the intent explicit. The
bootstrap/benchmark split runs the parallel path against
deterministic, pre-recorded BALs rather than re-deriving them, so
timings are comparable.
- crates/storage/store.rs: replace import-bench inter-block sleep
with explicit persistence-idle wait. The trie-update worker
channel is sync_channel(0) (rendezvous), so a successful send
proves the worker drained its previous iteration and returned to
recv(). Add TrieMessage::Ping (no-op) plus
Store::wait_for_persistence_idle() that sends one from
spawn_blocking; the import-bench loop calls it instead of sleeping
100ms. Removes the magic-number sleep and tightens the per-block
timer against the actual idle signal rather than a worst-case
Phase 2/3 estimate. Bench-tooling change only; no effect on
production paths.
edg-l
force-pushed
the
feat/import-bench-bal-tooling
branch
from
8192dd1 to
02a5663
Compare
bal_index was declared inside the `for blocks in chains` loop, so it reset to 0 at the start of every chain. When the input path is a directory the importer processes multiple chain files sequentially but `preloaded_bals` is a single flat list across all Amsterdam+ blocks, so chain 2+ silently consumed chain 1's BALs instead of continuing the cursor. Move the declaration above the outer loop so the cursor spans all chains. Single-chain imports are unaffected.
ElFantasma
left a comment
ElFantasma
left a comment
There was a problem hiding this comment.
Both modes batch-load everything into RAM. --export-bal accumulates exported_bals: Vec<BlockAccessList> across all blocks before writing; --with-bal does preloaded_bals: Vec<BlockAccessList> upfront. For a 200-block bench this is fine. For 100k+ blocks on a long chain, total memory could reach multiple GB. Worth a comment in the help text noting the in-memory bound, or a follow-up to stream both sides via length-prefixed framing.
- assert preloaded BAL count matches Amsterdam+ block count up front; mismatched files would otherwise fall through to sequential silently - include path in panic messages for BAL read/write/decode and chain path stat - document single-producer expectation on Store::wait_for_persistence_idle - note in-memory bound in --export-bal/--with-bal help text
4 participants
Summary
--export-bal <FILE>and--with-bal <FILE>flags toimport-bench(cmd/ethrex/cli.rs).--export-balruns sequential execution and writes one RLP-encodedBlockAccessListper Amsterdam+ block into a single file;--with-baldecodes that file and feeds BALs into the parallel execution path.tokio::time::sleep(500ms)between blocks inimport_blocks_benchwith an explicitStore::wait_for_persistence_idle()call. Uses a newTrieMessage::Pingover thesync_channel(0)rendezvous channel: a successful send proves the trie-update worker finished its Phase 2 disk write and returned torecv().Storemethod.Import speed improvement
The previous inter-block
sleep(500ms)is replaced by a ping-based wait that resolves in ~3-10ms (typical Phase 2/3 drain time).For a 200-block import, total inter-block waiting:
So ~99 seconds saved in pure waiting per 200 blocks. The per-block timer is also now accurate -- it no longer absorbs the previous block's Phase 2/3 background persistence cost.
Notes
Cherry-picked from two feature branches:
bal-benchmark-setup----export-bal/--with-balplumbingperf/bal-parallel-overhead-rebased-- persistence-idle waitIsolated here so they can ship as a focused tooling PR without dragging in unrelated work from either parent branch.
Test plan