⚠️ Known Issues — intentionally skipped tests

Source: docs/known_issues.md

Known Issues

Tests intentionally excluded from CI. Source of truth for the Known
Issues section the L1 workflow appends to each ef-tests job summary
and posts as a sticky PR comment.

EF Tests — Stateless coverage narrowed to EIP-8025 optional-proofs

make -C tooling/ef_tests/blockchain test calls test-stateless-zkevm
instead of test-stateless. The zkevm@v0.3.3 fixtures are filled against
bal@v5.6.1, out of sync with current bal spec; the broad target trips ~549
fixtures. Re-broaden once the zkevm bundle is regenerated.

Lines of code report

Total lines added: 261
Total lines removed: 0
Total lines changed: 261

+-------------------------------------------------+-------+------+
| File                                            | Lines | Diff |
+-------------------------------------------------+-------+------+
| ethrex/crates/blockchain/blockchain.rs          | 2551  | +5   |
+-------------------------------------------------+-------+------+
| ethrex/crates/common/types/block_access_list.rs | 1409  | +246 |
+-------------------------------------------------+-------+------+
| ethrex/crates/common/types/mod.rs               | 31    | +1   |
+-------------------------------------------------+-------+------+
| ethrex/crates/vm/backends/levm/mod.rs           | 2396  | +9   |
+-------------------------------------------------+-------+------+

Benchmark Block Execution Results Comparison Against Main

🤖 Codex Code Review

Findings

1. High: blockchain.rs:478 builds optimistic_witness with only address and added_storage. That vector is later fed into blockchain.rs:1943, but the replay logic in store.rs:1869 only mutates nonce/balance/code hash when update.info is present and only removes accounts when update.removed is set. On the BAL path, cached witnesses will therefore miss balance-only, nonce-only, code-only, account-creation, and removal transitions when --precompute-witnesses is enabled. I would keep the synthesized BAL map for merkleization, but build witness updates from the prestate-aware bal_to_account_updates path (or another full AccountUpdate builder), not from this storage-only skeleton.

2. Low: blockchain.rs:924 uses par_iter() directly, which runs on Rayon’s global pool, while the existing streaming merkle path is explicitly bounded by blockchain.rs:699 self.merkle_pool. The new BAL prewarm can therefore spill extra work onto the global pool and oversubscribe CPU/DB readers on large BALs, which is the opposite of what you want on engine_newPayload. Running that prewarm inside self.merkle_pool.install(...) or folding it into the scoped workers would keep resource usage predictable.

No other consensus/gas-accounting problems stood out in the touched logic. I did not run the test suite; this is a static review of the diff.

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

🤖 Claude Code Review

Now I have a complete picture of the PR. Let me write the review.

PR Review: perf(l1): BAL optimistic merkleization on validation path

Overall: The design is sound and the implementation is careful. Decoupling synthesis from execution is a legitimate optimization, the invariants are well-maintained, and the 13 unit tests cover the critical edge cases. A few items below deserve attention before merging.

Correctness

EIP-161 normalization — correct. Stage C at blockchain.rs:1123-1130 already handles the removed: false hardcode safely:

// EIP-161: remove empty accounts (zero nonce, zero balance, empty code, empty storage)
if account_state != AccountState::default() {
    state_trie.insert(...)?;
} else {
    state_trie.remove(path)?;
}

A selfdestructed account (BAL records balance = 0, no nonce/code change) will be compared against AccountState::default() using pre-state nonce/code. If it was a fresh same-tx creation, the empty check removes it; if it had pre-existing nonce/code, the trie keeps the entry with balance=0. This matches the streaming path behavior as claimed.

Witness correctness — correct. generate_witness_from_account_updates at blockchain.rs:1579-1588 only reads account_update.address and account_update.added_storage.keys() — it does not consume info, code, or removed. The synthesized witness (address + added_storage) is sufficient and matches the actual usage.

Channel invariant — correct. The invariant optimistic_updates.is_some() ↔ rx_for_merkle.is_none() is established cleanly via bal.map(...) vs if bal.is_some(). Both expect() calls are logically justified.

Issues

1. duration_since may panic in the metrics path (blockchain.rs:2150-2153)

let merkle_start_delay_ms = merkle_start_instant
    .duration_since(exec_merkle_start)   // panics if exec_merkle_start > merkle_start_instant
    .as_secs_f64()
    * 1000.0;

Instant::duration_since panics on some platforms if the argument is later than self. Although thread scheduling virtually guarantees merkle_start_instant >= exec_merkle_start, a panic inside the logging function would crash a validator node over a metric. Compare with blockchain.rs:2093 which already uses saturating_duration_since for overlap:

.saturating_duration_since(exec_end_instant)

Use saturating_duration_since here as well for consistency and safety.

2. Pre-warm opens one state_trie per account (blockchain.rs, the new parallel pre-warm block)

accounts
    .par_iter()
    .try_for_each(|(hashed_address, _)| -> Result<(), StoreError> {
        let state_trie = self.storage.open_state_trie(parent_state_root)?;  // one per item
        let _ = state_trie.get(hashed_address.as_bytes())?;
        Ok(())
    })?;

For a block with, say, 400 touched accounts, this creates 400 trie handles and immediately discards them. Stage B opens exactly 16 (one per worker). If open_state_trie does anything heavier than wrapping a pointer — e.g. looking up the root node in RocksDB — this loop creates gratuitous work that may partially negate the warm-up benefit. Consider par_chunks or a per-thread trie via rayon's local storage to amortize the open across all accounts a given thread processes.

3. Redundant address field in BalSynthesisItem (block_access_list.rs:1594)

pub struct BalSynthesisItem {
    pub address: Address,    // duplicated — always equals the FxHashMap key
    ...
}

The field is set during synthesis but never read from the struct; callers always use the map key addr. Removing it would avoid the duplication and make the struct's role clearer (it's a field-delta bag, not a self-describing entity). Not a bug, but worth cleaning up.

Minor notes

• The debug_assert! + immediate continue guard for empty slot_changes is the right pattern; the test at synthesize_skips_when_slot_changes_empty correctly annotates the dual debug/release behaviour with #[cfg_attr(debug_assertions, should_panic(...))].

• The greedy bin-packing weight comment (weight >= 1 invariant) is still accurate for the new synthesis path since removed / removed_storage are gone and weight is purely max(1, added_storage.len()).

• The [Instant; 7] type alias in BlockExecutionPipelineResult vs [Instant; 8] in log_block_execution_pipeline_info (which appends stored_instant at the call site) is intentional and consistent.

Automated review by Claude (Anthropic) · sonnet · custom prompt

Greptile Summary

This PR decouples BAL-merkleization from EVM execution on the engine_newPayload validation path by synthesizing per-field state deltas from the input BlockAccessList before thread::scope, then running merkle stages B/C/D in parallel with EVM execution. The correctness gate (validate_block_access_list_hash) remains post-execution; a hash mismatch discards the optimistic result via ? propagation.

• BalSynthesisItem replaces the old Option<AccountInfo> blob with per-field optionals, preventing zero-fabrication for unchanged fields.
• handle_merkleization_bal is replaced by handle_merkleization_bal_from_updates, eliminating Stage A (channel drain); the EVM skips bal_to_account_updates + send when merkleizer = None on the validation path.
• Parallel state-trie pre-warm and hashed-key-sorted storage inserts are added to improve cache locality for Stage B/C.

Confidence Score: 4/5

Safe to merge for the validation path; correctness is gated by the existing post-execution BAL hash check and the state-root comparison against the block header, so an incorrect optimistic trie results in block rejection rather than silent state corruption.

The redesign is mechanically sound: per-field optionals avoid zero-fabrication for unchanged account fields, EIP-161 normalization in Stage C correctly handles account deletion without needing an explicit removed flag, and generate_witness_from_account_updates only reads address plus added_storage.keys() so the stripped optimistic_witness is complete for its purpose. Minor structural issues remain: BalSynthesisItem.address is stored but never read after construction, BalStateWorkItem.removed is an always-false dead branch, and the parallel pre-warm opens one state_trie handle per account rather than per thread.

The pre-warm block in handle_merkleization_bal_from_updates (blockchain.rs) and the BalSynthesisItem struct definition (block_access_list.rs) deserve a second look.

Sequence Diagram

sequenceDiagram
    participant Main as Main Thread
    participant Exec as EVM Exec Thread
    participant Merkle as Merkleizer Thread
    participant Warm as Warmer Thread

    Note over Main: exec_merkle_start captured
    Main->>Main: synthesize_bal_updates(bal)
    Main->>Main: build optimistic_witness
    Note over Main: thread::scope entered

    par Parallel threads
        Main->>Exec: "spawn merkleizer=None on BAL path"
        Main->>Merkle: "spawn prepared=optimistic_updates"
        Main->>Warm: spawn
    end

    Note over Merkle: merkle_start_instant captured
    Merkle->>Merkle: par_iter pre-warm open_state_trie per account
    Merkle->>Merkle: Stage B parallel storage root computation
    Merkle->>Merkle: Stage C per-shard state trie updates EIP-161
    Merkle->>Merkle: Stage D finalize root state_trie_hash
    Note over Merkle: merkle_end_instant captured

    Exec->>Exec: execute_block_parallel skip bal_to_account_updates
    Exec->>Exec: validate_block_access_list_hash
    Note over Exec: exec_end_instant captured

    Main->>Main: accumulated_updates optimistic_witness OR streaming_witness
    Main->>Main: verify state_trie_hash matches block header
    Note over Main: exec_merkle_end_instant captured

Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 3
crates/common/types/block_access_list.rs:1600-1608
The `address` field duplicates the map key in `FxHashMap<Address, BalSynthesisItem>`. In `handle_merkleization_bal_from_updates` the key `addr` is always used instead of `item.address`, and in `optimistic_witness` `*addr` is used too — so this field is written but never read. Removing it saves 20 bytes per account in the synthesized map.

```suggestion
#[derive(Debug, Clone, Default)]
pub struct BalSynthesisItem {
    pub balance: Option<U256>,
    pub nonce: Option<u64>,
    pub code_hash: Option<H256>,
    pub code: Option<Code>,
    pub added_storage: FxHashMap<H256, U256>,
}
```

### Issue 2 of 3
crates/blockchain/blockchain.rs:319-326
On the BAL synthesis path `removed` is hardcoded to `false` and `handle_merkleization_bal_from_updates` never sets it. The `if item.removed { account_state = AccountState::default(); }` branch in the Stage C worker is therefore unreachable on this path. Dropping the field from the struct would make this invariant visible and avoid confusion for future readers.

```suggestion
struct BalStateWorkItem {
    hashed_address: H256,
    nonce: Option<u64>,
    balance: Option<U256>,
    code_hash: Option<H256>,
    /// Pre-computed storage root from Stage B, or None to keep existing.
    storage_root: Option<H256>,
```

### Issue 3 of 3
crates/blockchain/blockchain.rs:921-932
**Pre-warm opens one `state_trie` handle per account**

The rayon closure calls `open_state_trie(parent_state_root)` for each of the N accounts, creating N independent trie handles before Stage B opens another 16 (one per worker). If `open_state_trie` acquires any internal lock, allocates a per-handle arena, or does RocksDB snapshot work, N parallel opens could serialise or spike allocation far beyond what Stage B already requires. Worth verifying that `open_state_trie` is a lightweight read-view wrapper, or considering a single trie opened per rayon thread rather than per item.

_{Reviews (1): Last reviewed commit: "perf(l1): BAL optimistic merkleization o..." | Re-trigger Greptile}

Pushed fixes for review comments in e7aaec8:

• dropped unused BalSynthesisItem.address
• dropped dead BalStateWorkItem.removed branch
• pre-warm now chunks accounts across workers (one trie open per worker, matching Stage B/C) instead of one open per account
• removed the conflicting debug_assert! + .expect() on empty slot_changes; defensive continue stays, test updated
• replaced the sequential-path merkleizer.expect(...) with a typed EvmError
• gated execute_block_pipeline's header_bal branch on is_amsterdam
• when the BAL validation path produces no BAL, return a ChainError instead of skipping the hash check