[ManuelBilbao]

Depends on #7426.

Motivation

Each L2 connection imports the blocks it receives on its own actor. Duplicate inbound and outbound sessions to the sequencer aren't deduplicated, so a node with several connections runs several add_block_pipelines at once on tokio workers. Each merkleization parks 17 jobs on the blockchain's 17-thread merkle pool until its block is done, so concurrent imports split the pool between them and none can finish. The follower deadlocks mid-sync at 0% CPU, and because the imports block tokio workers, RPC and logging stop too.

Description

• Add L2BlockImporter, one per node, which replaces the per-connection queue and import.
• Connections submit the blocks they receive to its ordered queue and move on. A duplicate submission is rejected, so only the connection that queued a block re-broadcasts it.
• A single background task imports queued blocks in order from the local head on, one at a time, running each import on a blocking thread. Queued blocks at or below the head are dropped.
• Update the p2p_block_import tests to drive the importer, and add tests for duplicate submissions, dropping stale blocks, and importing blocks delivered by several connections.

Checklist

• Updated STORE_SCHEMA_VERSION (crates/storage/lib.rs) if the PR includes breaking changes to the Store requiring a re-sync.

[github-actions]

⚠️ Known Issues — intentionally skipped tests

Source: docs/known_issues.md

rpc-compat log-bearing cases excluded

Where: KNOWN_EXCLUDED_TESTS in .github/scripts/check-hive-results.sh counts out
eight hive rpc-compat cases — the four eth_getLogs cases, eth_getBlockReceipts/get-block-receipts-latest,
and three eth_getTransactionReceipt cases. They are exactly the cases whose recorded
response contains at least one log object; every case with an empty log array still runs.
Note this leaves eth_getLogs with no rpc-compat coverage at all, since all four of its
cases are in the set.

Why: ethrex populates blockTimestamp on log objects, as geth, besu, nethermind, reth
and erigon all do. hive's rpc-compat compares responses byte-exactly (jsondiff.FullMatch;
the lenient checkJSONStructure path applies only to cases upstream marks speconly), and
the corpus is pinned to execution-apis d08382ae (2025-02-10), whose recordings predate the
field — it entered the schema in execution-apis#639 and the fixtures in #846 (2026-07-22).
So the extra key cannot match, and this is a property of the pin rather than of the response.

The pin cannot move, and this is not temporary. The pin sits one commit before
execution-apis#627, which moved the test chain to a pre-merge genesis: the current corpus has
~36 proof-of-work blocks before its terminal total difficulty. ethrex does not support
pre-merge chains and will not, so importing that chain.rlp fails at block 1 —
validate_block_header has no pre-London base-fee path. Every revision carrying
blockTimestamp in its fixtures also carries that chain, so there is no revision that
satisfies both. Nor can the corpus be patched locally: rpc-compat's Dockerfile clones
ethereum/execution-apis by hard-coded URL, so the branch buildarg cannot point at a fork.

Coverage: the field itself is pinned by
block_timestamp_is_on_the_log_and_not_on_the_receipt in
crates/networking/rpc/types/receipt.rs, which asserts it is present on each log and absent
from the receipt level.

Removal: delete the entries if ethrex ever gains pre-merge chain import, or if upstream
marks these cases speconly so they are type-checked instead of compared byte-for-byte.

---

The stateless schema id does not identify the encoding

Where: STATELESS_INPUT_SCHEMA_ID in crates/common/types/stateless_ssz.rs.

Upstream keeps the stateless input schema id at 0x1501
(fork_index 0x15 << 8 | revision 0x01) across incompatible body changes. Four
encodings have now shipped under it: tests-zkevm@v0.6.2, then #3248 + #3278,
then #3356, which moved state, codes and public_keys from SszList to
ProgressiveList, then #3652 in tests-zkevm@v21.0.1, which removed
public_keys outright without bumping the revision. ethrex speaks the last one.

The consequence is that the 2-byte prefix cannot be used to detect a stale or
mismatched bundle. A wrong-dialect input is accepted by the id check and then
fails later — in SSZ decode, or on a root that does not match — rather than being
rejected up front for what it is. only_amsterdam_schema_id_decodes therefore
proves less than its name suggests.

Worth raising upstream: a revision field that does not move across a body change
provides no version negotiation at all.

---

ZisK guest program hash changes with the unsync_cell gate

Where: crates/common/types/block.rs, transaction.rs.

The gate on the single-threaded unsync_cell::OnceCell moved from
all(feature = "eip-8025", target_arch = "riscv64") to
all(feature = "zisk", target_arch = "riscv64") when the eip-8025 feature was removed.

The guest ELFs were previously built --features "<zkvm>-build-elf,ci", which never enabled
eip-8025, so they compiled the atomic once_cell variant. bin/zisk/Cargo.toml does enable
ethrex-common/zisk, so the ZisK guest now compiles the unsafe impl Sync cell instead.
That changes the ELF bytes and therefore the program hash and verification key.

This is intended (the guest is single-threaded, so the unsync cell is sound and cheaper), but it
is a VK change rather than a no-op refactor, and the diffstat presents it as a file rename
(eip8025_cell.rs → unsync_cell.rs). Anyone pinning a ZisK VK across this change must
re-register it. The stateless-validator crate now forwards ethrex-common/zisk from its own
zisk feature so the two ZisK guests do not disagree on the cell type.

---

Release signing key is an unprotected repository secret

Where: .github/workflows/tag_release.yaml.

MINISIGN_SECRET_KEY is a plain repository secret. There is no environment: on
finalize-release or dry-run-release-assets, and gh api repos/lambdaclass/ethrex/rulesets
shows only branch-targeted rulesets, so the github.ref_type == 'tag' condition is a workflow
check rather than an enforced boundary: anyone who can push a tag can reach the signing key.

This is a repository-settings change, not a code change, so it is recorded here rather than
fixed in the tree. Recommended:

1. Move MINISIGN_SECRET_KEY / MINISIGN_PASSWORD into a GitHub Environment with required
   reviewers, and add environment: to the two jobs that sign.
2. Add a ruleset targeting refs/tags/v* restricting who may create release tags.

Until then, the compromise of that key is silent and durable: signatures would still verify
against the committed .github/minisign.pub.

[github-actions]

🤖 Claude Code Review

Review: PR 7429, single node-wide L2 block importer

The design is sound. A single serialized importer fixes the merkle-pool deadlock, and running add_block_pipeline under spawn_blocking is the right way to keep tokio workers free. The mutex is never held across an .await, and recovering from a poisoned lock is justified. notify_one stores a permit, so a submit that lands during an import is not lost. Removing the per-connection state also removes the stale-next_block_to_add problem.

Items to consider, most important first:

1. The queue is unbounded and now shared by the whole node (block_importer.rs, submit).

   • Blocks with a number above the head+1 gap stay in the BTreeMap until the gap fills, and nothing caps the queue's size or how far ahead of the head a block can be. The old queue was per connection and was freed when the connection dropped. Now the memory outlives any peer.
   • The queue is also keyed by block number alone, so a peer that gets a bad or far-future block past should_process_new_block can hold slot N. Other connections' blocks for N are then rejected until it's imported or dropped.
   • Suggested fix: reject blocks more than some window above head+1, and/or cap the queue length. Please also confirm that should_process_new_block verifies the committer signature before submit. I only saw the diff context, not that code.

2. A failed import drops the block and stalls the rest of the queue (import_ready_blocks, take).

   • take removes block N before it is imported. If add_block_pipeline, apply_fork_choice or store_fee_config_by_block fails, N is gone and run only logs the error. Blocks N+1 and later then sit idle until some later submit notifies the importer, and that attempt hits the same gap.
   • A failure after add_block_pipeline succeeds but before store_fee_config_by_block is more subtle. The head has advanced, so the block is never retried, and its fee config is silently never stored. Because the head moved, take would skip over that block on the next pass.
   • Suggested fix: make the post-import steps idempotent or retried, or at least log loudly. Re-queueing the block on pre-import failure would also help, though a permanently invalid block would then loop.

3. Dedup only helps while a block is queued. Once a block is taken, is_queued and submit both succeed for the same number again.

   • take cleans up stale entries on the next pass, so this is bounded.
   • A duplicate that arrives mid-import is still re-broadcast by the connection that queued it. That is a small amplification, not a correctness problem.
   • Optionally, also reject number <= head in submit or should_process_new_block.

4. #[derive(Default)] on L2BlockImporter makes it easy to build an importer that has no running task. Only the tests do this. Consider #[cfg(test)] or pub(crate) for the default constructor, or note it in the docs.

5. import_ready_blocks is pub and called both by the loop and by tests. That's fine. The error type is still PeerConnectionError, which no longer fits an importer that no connection owns. A dedicated error type would be cleaner, but it's not blocking.

6. Minor:

   • info!("Added new block ...") is now correct because it uses block_number. The old code used a separate counter.
   • The get_block_body re-check was dropped. This is safe because there is a single importer and take filters by the head, assuming no other path writes L2 blocks concurrently.

7. Tests. Coverage is good: ordering, stale-drop, dedup, and a multi-connection concurrency test. The concurrency test only exercises the submit side, so it would not catch a failure-path stall like Item 2. Consider adding a test where an invalid block is queued ahead of valid ones.

No consensus, EVM or RLP changes are involved. Items 1 and 2 are worth addressing before merge. The rest are nits.

---

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

[github-actions]

🤖 Codex Code Review

Found one correctness issue in the new shared importer; otherwise the direction looks good.

• crates/networking/p2p/rlpx/l2/block_importer.rs:62 / crates/networking/p2p/rlpx/l2/block_importer.rs:81 / crates/networking/p2p/rlpx/connection/server.rs:879
  The new background loop only wakes on submit() notifications, and import_ready_blocks() stops as soon as the next expected height is missing. That creates a stall case: if block N+1 is queued first, the importer wakes, sees next_block = N, returns, and goes back to sleep. When block N is later imported by some other path (another peer, sync, backfill, etc.), nothing re-notifies the importer, so the already-queued N+1 can remain stuck indefinitely until some unrelated future block arrives. The old path called process_blocks_on_queue() after each broadcast tick, so queued successors were retried regularly. I’d suggest either draining in a loop that re-checks after head changes, or explicitly notifying/rerunning the importer after any successful block import from other paths.

• crates/networking/p2p/rlpx/l2/block_importer.rs:44
  Deduplicating queued blocks only by block number can discard a conflicting block at the same height before parent/hash validation. In normal operation the sequencer should be unique, but from a robustness/security perspective this lets the first peer to send any block at height H suppress later candidates at height H. If reorgs/forks at L2 are impossible by design, a short comment asserting that invariant would help; otherwise consider keying by hash or validating parent linkage before rejecting duplicates.

• crates/networking/p2p/rlpx/l2/block_importer.rs:137
  Using std::sync::Mutex in async code is acceptable here because the critical sections are tiny and never .await, but this path is shared across all L2 peers. If block fan-in gets high, this becomes a runtime-thread contention point. Not a blocker, just worth keeping an eye on.

Overall, centralizing imports to avoid overlapping add_block_pipeline calls looks like the right fix, and the new tests improve coverage for duplicate delivery and concurrent peers.

---

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

[github-actions]

🤖 Kimi Code Review

I'll review this PR which introduces a centralized L2 block importer to replace per-connection block queue processing. Let me analyze the changes carefully.

Overall Assessment

The PR addresses a real problem (deadlocks from concurrent block imports, duplicate imports across connections) with a sound architectural change. However, there are several issues ranging from correctness bugs to performance concerns.

---

Critical Issues

1. Race Condition in submit() — Duplicate Block Imports Possible

File: crates/networking/p2p/rlpx/l2/block_importer.rs, lines 44-53

pub fn submit(&self, block: Arc<Block>, fee_config: FeeConfig) -> bool {
    let number = block.header.number;
    let mut queue = self.queue();
    if queue.contains_key(&number) {
        return false;
    }
    queue.insert(number, QueuedBlock { block, fee_config });
    drop(queue);
    self.block_queued.notify_one();
    true
}

Problem: The contains_key check + insert is not atomic. Between releasing the lock and another thread acquiring it, a duplicate can be inserted. The return false path also fails to notify, which could stall the importer if all queued blocks were already processed and a "duplicate" arrives.

Fix: Use BTreeMap::entry or insert and check the return value atomically:

pub fn submit(&self, block: Arc<Block>, fee_config: FeeConfig) -> bool {
    let number = block.header.number;
    let mut queue = self.queue();
    let is_new = queue.insert(number, QueuedBlock { block, fee_config }).is_none();
    drop(queue);
    if is_new {
        self.block_queued.notify_one();
    }
    is_new
}

---

2. Lost Wake-up in run() Loop

File: crates/networking/p2p/rlpx/l2/block_importer.rs, lines 62-71

async fn run(self, storage: Store, blockchain: Arc<Blockchain>, store_rollup: StoreRollup) {
    loop {
        self.block_queued.notified().await;
        if let Err(err) = self
            .import_ready_blocks(&storage, &blockchain, &store_rollup)
            .await
        { ... }
    }
}

Problem: Classic lost wake-up. If notify_one() fires between import_ready_blocks completing and notified().await starting, the notification is lost and the importer stalls until the next block arrives.

Fix: Use a loop that re-checks the queue after waking, or use notify_waiters / a Semaphore. Minimal fix:

async fn run(self, storage: Store, blockchain: Arc<Blockchain>, store_rollup: StoreRollup) {
    loop {
        self.block_queued.notified().await;
        while let Err(err) = self
            .import_ready_blocks(&storage, &blockchain, &store_rollup)
            .await
        {
            error!(error=%err, "Failed to import blocks, retrying");
            tokio::time::sleep(Duration::from_millis(100)).await;
        }
        // After successful import, check if more blocks arrived while we were importing
        if !self.queue().is_empty() {
            continue; // skip the await, re-check immediately
        }
    }
}

Actually, a cleaner pattern: loop with notified() but ensure we always drain the queue:

async fn run(self, storage: Store, blockchain: Arc<Blockchain>, store_rollup: StoreRollup) {
    loop {
        self.block_queued.notified().await;
        // Drain all ready blocks, not just one batch
        while self.queue().first_key_value().is_some() {
            match self.import_ready_blocks(&storage, &blockchain, &store_rollup).await {
                Ok(()) => {},
                Err(err) => {
                    error!(error=%err, "Failed to import blocks");
                    break;
                }
            }
        }
    }
}

Wait — import_ready_blocks already loops internally. The issue is it returns Ok(()) when no next block is ready. So the outer loop should just re-await. But the lost wake-up remains: if a block is submitted during the final iteration of import_ready_blocks's inner loop, after it checks take(next_block) but before it returns, the notify_one fires while no one is waiting.

Better fix: Use tokio::sync::Semaphore or always re-check queue after notified() returns, with a timeout fallback:

async fn run(self, storage: Store, blockchain: Arc<Blockchain>, store_rollup: StoreRollup) {
    loop {
        // Check immediately, then wait for notification
        if self.queue().is_empty() {
            self.block_queued.notified().await;
        }
        if let Err(err) = self
            .import_ready_blocks(&storage, &blockchain, &store_rollup)
            .await
        {
            error!(error=%err, "Failed to import blocks received from L2 peers");
            tokio::time::sleep(Duration::from_millis(100)).await;
        }
    }
}

---

3. spawn_blocking with Arc::unwrap_or_clone — Potential Panic

File: crates/networking/p2p/rlpx/l2/block_importer.rs, lines 87-92

let block = Arc::unwrap_or_clone(block);
let importer = blockchain.clone();
tokio::task::spawn_blocking(move || importer.add_block_pipeline(block, None))

Problem: Arc::unwrap_or_clone was stabilized in Rust 1.82. Check your MSRV. More importantly, unwrap_or_clone will clone if there are other references — this could be expensive for a Block (deep clone of transactions, etc.).

Question: Who else holds Arc<Block>? The NewBlock message in the connection handler? If so, this clones the entire block. Given blocks can be large, this is a significant memory/CPU hit.

Also: spawn_blocking without limiting concurrency — if many blocks queue up, you could spawn unlimited blocking tasks. But since import_ready_blocks is called sequentially, only one runs at a time. Good.

---

4. Poison Error Handling Masks Bugs

File: crates/networking/p2p/rlpx/l2/block_importer.rs, lines 138-140

fn queue(&self) -> MutexGuard<'_, BTreeMap<u64, QueuedBlock>> {
    // The lock is only held for map operations, which leave it consistent even if one panics.
    self.queue.lock().unwrap_or_else(PoisonError::into_inner)
}

Problem: The comment is incorrect. BTreeMap operations can panic (e.g., during drop of values if QueuedBlock has a custom Drop that panics, or on allocation failure). More critically, if a panic occurs during insert, the map may be in an inconsistent state (though BTreeMap is generally panic-safe for single operations).

More serious: If the lock is poisoned, continuing as if nothing happened is dangerous. Another thread panicked while holding the lock — the queue state may be corrupted. Silently ignoring this could lead to consensus failures.

Fix: At minimum, log the poisoning. Better: propagate the error or abort the process. For blockchain code, I'd suggest:

fn queue(&self) -> MutexGuard<'_, BTreeMap<u64, QueuedBlock>> {
    match self.queue.lock() {
        Ok(guard) => guard,
        Err(poisoned) => {
            error!("Block importer queue lock poisoned! Previous thread panicked while holding lock.");
            poisoned.into_inner()
        }
    }
}

Actually, given the criticality, consider std::sync::RwLock or parking_lot::Mutex (which doesn't poison). But if staying with std, don't silently swallow poison.

---

Security / Consensus Issues

5. Missing Validation in import_ready_blocks

File: crates/networking/p2p/rlpx/l2/block_importer.rs, lines 82-100

The old code in l2_connection.rs had this check before importing:

// This check is necessary if a connection to another peer applied the block after we
// read the head.
if let Ok(Some(_)) = storage.get_block_body(next_block_to_add).await {
    next_block_to_add += 1;
    continue;
}

This check is gone in the new code. The PR's take() method drops blocks at or below head, but there's a race:

1. Thread A: storage.get_latest_block_number() returns 5
2. Thread B (another connection's import, or external RPC): imports block 6
3. Thread A: take(6) succeeds, imports block 6 again

This is a double-import bug. While add_block_pipeline might handle duplicates, relying on that is dangerous — it may have side effects (writing to DB, emitting events, updating caches).

Fix: Re-add the check:

let next_block = storage.get_latest_block_number()? + 1;
let Some(QueuedBlock { block, fee_config }) = self.take(next_block) else {
    return Ok(());
};
// Defensive: check if already imported (race with other importers or RPC)
if storage.get_block_body(next_block).await?.is_some() {
    continue; // already have it, loop to next
}

---

6. No Limit on Queue Size — DoS Vector

File: crates/networking/p2p/rlpx/l2/block_importer.rs

The BTreeMap queue is unbounded. A malicious or buggy peer can flood the node with future blocks, causing unbounded memory growth.

Fix: Add a max queue size, or max future block distance:

const MAX_QUEUED_BLOCKS: usize = 1024;
const MAX_FUTURE_BLOCK_DISTANCE: u64 = 256;

pub fn submit(&self, block: Arc<Block>, fee_config: FeeConfig) -> bool {
    let number = block.header.number;
    // ... existing checks ...
    
    let head = self.get_latest_block_number(); // need access to storage or cached value
    if number > head + MAX_FUTURE_BLOCK_DISTANCE {
        warn!("Block {} too far in future, rejecting", number);
        return false;
    }
    
    let mut queue = self.queue();
    if queue.len() >= MAX_QUEUED_BLOCKS {
        warn!("Block queue full, rejecting block {}", number);
        return false;
    }
    // ...
}

---

Performance Issues

7. take() Pops Stale Entries One-by-One — O(n²) in Pathological Case

File: crates/networking/p2p/rlpx/l2/block_importer.rs, lines 126-136

fn take(&self, number: u64) -> Option<QueuedBlock> {
    let mut queue = self.queue();
    while queue
        .first_key_value()
        .is_some_and(|(&first, _)| first < number)
    {
        queue.pop_first();
    }
    queue.remove(&number)
}

If the queue has many stale entries (e.g., after a long sync, thousands of old blocks), this loops one-by-one. The comment says "almost every queued block is above number" — but that's the steady state. During catch-up, or after a reorg, there could be many stale entries.

Fix: The comment's rationale for avoiding split_off is weak. split_off is O(log n) to find the split point, then O(1) to create the new tree. The old entries are dropped lazily. Benchmark, but split_off is likely better:

fn take(&self, number: u64) -> Option<QueuedBlock> {
    let mut queue = self.queue();
    // Drop everything before number
    let mut new_queue = queue.split_off(&number);
    // queue now contains [..number), new_queue contains [number..)
    // Drop old queue (stale entries)
    drop(queue);
    let result = new_queue.remove(&number);
    *self.queue.lock().unwrap() = new_queue; // or use proper lock handling
    result
}

Actually this is awkward with the lock API. Alternative: just use BTreeMap::retain or accept that split_off + reassign is cleaner. Given the lock is held anyway, split_off is better.

Wait — the current code holds the lock for the entire while loop. split_off would also hold the lock. The issue is returning the guard. Let me restructure:

fn take(&self, number: u64) -> Option<QueuedBlock> {
    let mut queue = self.queue();
    // Remove all entries < number in bulk
    let to_drop = queue.split_off(&number);
    // `queue` now has entries < number; `to_drop` has >= number
    // We want to keep >= number, so swap:
    let result = to_drop.get(&number).is_some(); // can't do this easily...

Hmm, the API is annoying. Current approach is fine if stale entries are rare. Add a metric or log warning if many entries are dropped:

fn take(&self, number: u64) -> Option<QueuedBlock> {
    let mut queue = self.queue();
    let mut dropped = 0;
    while queue
        .first_key_value()
        .is_some_and(|(&first, _)| first < number)
    {
        queue.pop_first();
        dropped += 1;
    }
    if dropped > 10 {
        warn!("Dropped {} stale queued blocks", dropped);
    }
    queue.remove(&number)
}

---

Code Quality Issues

8. L2BlockImporter::default() Creates Broken Instance

File: crates/networking/p2p/rlpx/l2/block_importer.rs, lines 33-37

#[derive(Debug, Clone, Default)]
pub struct L2BlockImporter {
    queue: Arc<Mutex<BTreeMap<u64, QueuedBlock>>>,
    block_queued: Arc<Notify>,
}

Default creates an importer with no background task. Calling submit on it will notify but nothing listens. Tests use this — they call import_ready_blocks manually, which works. But production code could accidentally use default() instead of spawn().

Fix: Remove Default derive, make constructor explicit:

impl L2BlockImporter {
    fn new() -> Self {
        Self {
            queue: Arc::new(Mutex::new(BTreeMap::new())),
            block_queued: Arc::new(Notify::new()),
        }
    }
    
    pub fn spawn(...) -> Self {
        let importer = Self::new();
        tokio::spawn(importer.clone().run(...));
        importer
    }
}

Tests can use L2BlockImporter::new() or a #[cfg(test)] constructor.

---

9. Test blocks_from_several_connections_are_imported is Flaky

File: test/tests/l2/p2p_block_import.rs, lines 96-117

#[tokio::test(flavor = "multi_thread", worker_threads = 4)]
async fn blocks_from_several_connections_are_imported() {
    // ...
    for _ in 0..4 {
        let (importer, blocks) = (importer.clone(), sequencer_chain.clone());
        tokio::spawn(async move {
            for block in &blocks {
                submit(&importer, block);
                tokio::task::yield_now().await;
            }
        });
    }
    tokio::time::timeout(Duration::from_secs(60), async {
        while store.get_latest_block_number().unwrap() < 10 {
            tokio::time::sleep(Duration::from_millis(20)).await;
        }
    })
    // ...
}

Problems:

• No synchronization on task completion — tasks may still be running after timeout
• yield_now().await after every submit may not actually interleave as intended; depends on scheduler
• If importer stalls (lost wake-up from Issue 2), test hangs 60s then fails — should investigate root cause

Fix: Join the spawned tasks, use a more deterministic interleaving:

let mut handles = vec![];
for _ in 0..4 {
    let (importer, blocks) = (importer.clone(), sequencer_chain.clone());
    handles.push(tokio::spawn(async move {
        for block in &blocks {
            submit(&importer, block);
            // Occasionally yield to increase interleaving
            if fastrand::u8(0..4) == 0 {
                tokio::task::yield_now().await;
            }
        }
    }));
}
// Wait for all submitters to finish
for handle in handles {
    handle.await.unwrap();
}
// Now wait for import with shorter timeout
tokio::time::timeout(Duration::from_secs(10), async {
    while store.get_latest_block_number().unwrap() < 10 {
        tokio::time::sleep(Duration::from_millis(10)).await;
    }
}).await.expect("blocks were not imported");

---

10. Removed process_blocks_on_queue Call in Server Loop

File: crates/networking/p2p/rlpx/connection/server.rs, lines 877-881

L2Cast::BlockBroadcast => {
    l2_connection::send_new_block(established_state).await
}

Previously this was:

let res = l2_connection::send_new_block(established_state).await;
res.and(l2_connection::process_blocks_on_queue(established_state).await)

The removal of process_blocks_on_queue is correct (now handled by background task), but the error handling changed: send_new_block errors no longer combined with import errors. This is fine, but verify that send_new_block errors shouldn't also trigger some cleanup.

---

Minor Issues

11. Unused Import Node in l2_connection.rs

The diff removes use crate::types::Node; — good cleanup. Verify no other uses remain.

12. FeeConfig Import Path Inconsistency

In block_importer.rs:

use ethrex_common::types::fee_config::FeeConfig;

In l2_connection.rs (old code, now removed):

fee_config: ethrex_common::types::fee_config::FeeConfig,

Consistent now, good.

13. Missing Debug for QueuedBlock — Actually Present

Never mind, #[derive(Debug)] is there.

---

Summary Table

Priority	Issue	Location	Fix
Critical	Race in submit()	block_importer.rs:44	Use insert return value atomically
Critical	Lost wake-up

---

Automated review by Kimi (Moonshot AI) · kimi-k2.6 · custom prompt

[github-actions]

Lines of code report

Total lines added: 121
Total lines removed: 95
Total lines changed: 216

Detailed view

+--------------------------------------------------------+-------+------+
| File                                                   | Lines | Diff |
+--------------------------------------------------------+-------+------+
| ethrex/cmd/ethrex/l2/initializers.rs                   | 554   | +8   |
+--------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/rlpx/connection/server.rs | 2472  | -1   |
+--------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/rlpx/l2/block_importer.rs | 112   | +112 |
+--------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/rlpx/l2/l2_connection.rs  | 513   | -94  |
+--------------------------------------------------------+-------+------+
| ethrex/crates/networking/p2p/rlpx/l2/mod.rs            | 9     | +1   |
+--------------------------------------------------------+-------+------+