perf(l1): remove the per-block thread creation from the block pipeline

[ilitteri]

Motivation

On a chain of near-empty blocks, ethrex spends more time on the fixed cost of processing a block than on the block itself. Measured on Plataberget (the public Glamsterdam testnet, where most blocks currently carry no transactions), a block took 1.23 ms on average, and the largest single component was not work: 0.32 ms of it was the merkleizer's start delay, the gap between the start of the exec/merkle phase and that thread's first instruction. The pipeline created one OS thread for execution and another for merkleization on every block, and the calling thread then did nothing but wait for both.

That fixed cost is what a consensus client measures for engine_newPayload when blocks are empty, and it is the difference between clients on a quiet chain.

Description

The first two commits add measurement, because the earlier reading pointed at the wrong place. A first look suggested a large cost outside block execution, in the engine handler. Instrumenting it showed the opposite:

Step	cost
JSON parse of the request body	0.02 ms
JWT verification	0.01 ms
payload to block conversion	0.02 to 0.04 ms
pre-execution checks	0.01 ms

So the engine plumbing is not the cost; block execution is. The apparent gap came from averaging over duplicate newPayload calls, which the consensus client re-sends and which short-circuit in about 0.2 ms.

Two new INFO lines make this visible going forward: [METRIC] NEWPAYLOAD splits the V5 handler into decode, checks and execute, and [METRIC] ENGINE_RPC splits the authenticated RPC entry point into JSON parsing, auth and dispatch. They are emitted for engine_newPayload* only.

The remaining commits then remove the per-block thread creation:

• Execution runs on the calling thread instead of a spawned one, and the merkleizer is spawned before it rather than after.
• The merkleizer moves onto a persistent four-thread pool, built on first use in the same way as the merkleization pool and kept separate from it because the merkleizer opens a scope on that one. in_place_scope runs execution on the calling thread and returns only once the merkleizer has finished, so both closures keep their borrows, and catch_unwind on each preserves the previous behaviour of reporting a panic as an error rather than unwinding.
• The speculative warmer is no longer started for a block with no transactions. It has nothing to warm there beyond the few entries the system calls and withdrawals touch, which execution reads anyway. Warming only populates caches, so skipping it cannot change a block's result. The trie-node prefetcher already opted out below its own size threshold.

Results

Measured on a node following Plataberget, where blocks currently carry no transactions. Only slot-paced blocks are counted: after a restart a node executes a catch-up backlog whose blocks cost about half as much, and mixing them in hides the effect. Baseline is 609 blocks, the change is 123 blocks, from the node's own [METRIC] BLOCK line.

Phase	Before (mean)	After (mean)	Before (median)	After (median)
total	1.341 ms	1.166 ms	1.240 ms	1.160 ms
exec	0.553 ms	0.416 ms	0.510 ms	0.390 ms
merkle start delay	0.391 ms	0.304 ms	0.330 ms	0.290 ms
merkle drain	0.704 ms	0.668 ms	0.620 ms	0.630 ms
store	0.058 ms	0.058 ms	0.050 ms	0.050 ms

Block processing is about 13% cheaper on the mean and 6% on the median, execution about 24% cheaper, and the merkleizer's start delay about 22% cheaper.

The start delay does not disappear, because handing the task to a pool worker that may be parked still costs around 0.29 ms.

Merkleizing inline for transactionless blocks, to skip that wakeup entirely, was tried and reverted on the branch: it measured 1.244 ms against 1.166 ms over 85 and 123 blocks, a regression. The premise was wrong. Merkleization does not idle waiting for streamed updates on such a block; the BAL-synthesized path starts on the parent state as soon as it has the prepared updates, so about 0.11 ms of its work genuinely overlaps execution. The full merkle cost is visible across the two runs: 0.775 ms when serialised, against 0.668 ms remaining after execution on the concurrent path. The commit and its revert are both kept so the measurement is on the record.

The consensus client's own measurement of the same node moved from 2.233 ms to 2.027 ms across the two windows. That is the right direction, but the other two clients on the same network drifted by a comparable amount over the same period without any change, so the per-phase numbers above are the reliable signal and that one is not.

How to Test

cargo test -p ethrex-blockchain
cargo clippy -p ethrex-blockchain --no-deps -- -D warnings

On a node following a chain of empty blocks, the [METRIC] BLOCK line's start_delay field should fall and ps -L should show persistent block-pipeline-* threads with no per-block block_executor_* threads. Compare only slot-paced blocks: after a restart a node executes a catch-up backlog whose blocks are roughly half the cost of steady-state ones.

Checklist

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

[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. Three
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. 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]

🤖 Kimi Code Review

I'll review this PR which introduces a persistent thread pool for block pipeline execution and adds performance metrics to engine API handlers.

crates/blockchain/blockchain.rs

Thread Pool Design and Safety

Line 282-288, 576-586: The pipeline_pool is a rayon::ThreadPool with 4 threads, separate from the existing merkle_pool. The design rationale is sound—avoiding OS thread creation per block—but there's a potential issue:

• Resource leak concern: The OnceLock<Arc<rayon::ThreadPool>> pattern means the pool is never shut down. On node restart or Blockchain reconstruction, the old pool's threads may linger until process exit. rayon pools do have shutdown behavior, but verify this doesn't interact poorly with test harnesses that create many Blockchain instances.

Critical: in_place_scope and Panic Safety

Lines 1246-1281: The switch from std::thread::scope with spawned threads to rayon::in_place_scope with catch_unwind has several issues:

1. Panic safety with AssertUnwindSafe (Line 1254, 1259): The closures capture self (the Blockchain), block, bal, vm, and numerous other non-Send/Sync types. Marking them AssertUnwindSafe is unsound if any captured type has invariants that Drop relies on for safety. Specifically:

   • self contains Arc<TokioMutex<Vec<...>>> and other shared state
   • vm likely contains database connections or caches
   • If the merkleizer panics after partially modifying state that the Blockchain or Store expects to be consistent, the execution_closure running concurrently may observe corrupted state

2. The previous code used std::thread::JoinHandle::join() which naturally propagated panics. The new catch_unwind + mpsc::sync_channel approach changes semantics: panics are caught and converted to errors, but the scope continues executing. This is actually a behavior change—previously, a panic in either thread would abort the entire scope due to join().unwrap_or_else().

3. Line 1254: ps.spawn(move |_| { ... })—the move captures merkle_closure which captures self (by reference, since it's within &self method). This is fine for std::thread::scope borrows, but rayon::in_place_scope has different lifetime guarantees. Verify that in_place_scope blocks until all spawned tasks complete (it does), but the move + catch_unwind combination is suspicious.

Data Race Risk in execution_closure

Lines 1144-1205: The execution_closure is move but not Send, and runs on the calling thread. However, it captures bal (an Arc), block (by reference originally, now moved?), and calls vm.execute_block_pipeline. The original code spawned this in a new thread; now it runs on the calling thread while the merkleizer runs on the pool.

Wait—looking more carefully: the original code had execution_handle spawned via std::thread::Builder::new().spawn_scoped(s, ...). The new code runs execution_closure directly in the in_place_scope closure. This means execution no longer runs in parallel with the calling thread doing other work—it is the calling thread. This is the intended optimization, but verify that no caller relies on the old async behavior.

sync_channel Buffer Size

Line 1251: std::sync::mpsc::sync_channel::<MerkleResult>(1)—buffer size of 1 is fine since there's exactly one send, but recv() on line 1281 blocks indefinitely if the merkleizer panics before sending. The unwrap_or_else only handles RecvError (sender dropped), not a hung sender. This is equivalent to the old join() behavior, so acceptable.

Redundant Code

Lines 1193-1200: The comment about draining rx_for_merkle in the None branch is now dead code? No—wait, looking at line 1199-1200:

if let Some(rx) = rx_for_merkle {
    for _ in rx {}
}

This is inside the None => branch where rx_for_merkle is None by construction (from match rx_for_merkle). This code is unreachable and should be removed. It appears to be leftover from a refactor—previously rx_for_merkle may have been separately bound.

Actually, re-reading: rx_for_merkle is the match scrutinee, so in the None arm it is None. The if let Some(rx) = rx_for_merkle is indeed dead code. Remove lines 1193-1200.

crates/networking/rpc/engine/payload.rs

Metric Timing Precision

Lines 375-378, 403-417, 465-477: The timing measurement uses Instant::now() and elapsed() with floating-point math for millisecond conversion. Minor issues:

1. Line 472: checks_ms calculation: (ms(total_elapsed) - ms(decode_elapsed) - ms(execute_elapsed)).max(0.0)—this can go negative due to measurement overhead and floating-point error. The .max(0.0) handles it, but consider using saturating_sub on Duration directly before conversion to avoid the issue.

2. Line 472: The comment says "Whatever is neither payload decoding nor execution" but the calculation subtracts decode and execute from total. However, execute_elapsed includes handle_new_payload_v4 which may itself do significant work beyond pure execution. The metric naming could be misleading—checks_ms includes any async waiting in handle_new_payload_v4().await.

Missing Metric Guard

Lines 465-477: The metric logging uses info! unconditionally. Consider a #[cfg(feature = "metrics")] or similar gating, or at least ensure this doesn't flood logs in production sync. The engine_newPayload frequency during sync can be very high.

crates/networking/rpc/rpc.rs

Timing Overhead in Hot Path

Lines 1012-1035: JWT authentication and JSON parsing are now timed for every authrpc request. The Instant::now() calls are cheap but not free. More importantly:

Line 1024: let json_parse_elapsed = request_start.elapsed();—this measures from request start to after JSON parse, but request_start was taken before body_len computation. Minor: move body_len after or use a separate start.

Line 1031: let auth_start = Instant::now(); inside the hot path—this is fine but the if req.method.starts_with("engine_newPayload") guard on line 1069 means we only log for newPayload, yet we pay the timing cost for all requests. Consider moving the dispatch_start and related timing inside the conditional, or using a compile-time feature flag.

starts_with Matching

Line 1069: req.method.starts_with("engine_newPayload")—this matches engine_newPayloadV1 through V5, but also would match a hypothetical engine_newPayloadBatch or similar. Use a more precise match if the intent is only V1-V5:

matches!(req.method.as_str(), 
    "engine_newPayloadV1" | "engine_newPayloadV2" | 
    "engine_newPayloadV3" | "engine_newPayloadV4" | "engine_newPayloadV5")

General Observations

1. The pipeline_pool threads are named block-pipeline-{i} (line 583)—good for debugging.

2. No Drop implementation for Blockchain to eagerly shut down pools. With OnceLock, pools persist for the Blockchain lifetime. Acceptable for a long-lived node, but test suites may accumulate threads.

3. The merkle_pool and pipeline_pool are separate—the comment explains this is because "the merkleizer itself opens a scope on that one." Verify this doesn't cause deadlock: if the merkleizer (running on pipeline_pool) tries to spawn work on merkle_pool, and both pools have limited threads, deadlock is possible if all merkle_pool threads are blocked waiting for pipeline_pool threads that are waiting for merkle_pool. The comment suggests this is intentional and safe, but worth double-checking the call graph.

Summary of Required Changes

File	Line	Issue	Severity
blockchain.rs	1193-1200	Dead code: unreachable if let Some(rx) in None branch	Medium
blockchain.rs	1254, 1259	AssertUnwindSafe may be unsound; audit captured types	Critical
blockchain.rs	1251-1281	Verify catch_unwind + channel semantics match previous panic behavior	High
rpc.rs	1069	starts_with is imprecise; use exact method matching	Low
rpc.rs	1012-1035	Consider gating timing overhead to metrics feature	Low
payload.rs	472	Use saturating_sub on Duration before max(0.0)	Low

---

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

[github-actions]

🤖 Codex Code Review

Findings:

1. pipeline_pool reintroduces the short-lived-Blockchain thread explosion that merkle_pool was explicitly engineered around. The new per-instance OnceLock<Arc<rayon::ThreadPool>> is added in blockchain.rs, initialized lazily in blockchain.rs, and used on every block in blockchain.rs. But the test constructors still only special-case merkle_pool (blockchain.rs, blockchain.rs), even though the existing tests/documentation already call out that rayon workers outlive short-lived harness instances (merkle_pool_tests.rs). In EF/RPC-heavy test binaries, every fixture that imports a block will now spin up its own extra 4-thread pool again. I’d either share/seed pipeline_pool in for_test_harness_with_pool too, or disable this optimization in test constructors.

2. The new pool creation can still panic on the first imported block instead of failing at startup. build_pipeline_pool() uses .expect(...) in blockchain.rs, but only merkle_pool has a startup preinit path (blockchain.rs). That means thread-creation/resource failures for this new mandatory pool are now deferred into engine_newPayload / block import rather than being surfaced during node boot. This is an availability regression; the same fail-fast treatment as merkle_pool seems warranted.

3. The two new [METRIC] lines are unconditional info! logs, unlike the existing block perf logs which are gated by perf_logs_enabled (blockchain.rs). See payload.rs and rpc.rs. On a live CL-connected node this adds two extra hot-path log writes per engine_newPayload* even when perf logging is meant to be off, which affects both log volume and the very timings being measured. I’d gate them on context.blockchain.options.perf_logs_enabled or lower them to debug!.

I did not spot a consensus/EVM/RLP correctness regression in the execution-path refactor itself.

I couldn’t run cargo check in this environment because rustup failed to create temp files on the read-only sandbox.

---

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

[github-actions]

Lines of code report

Total lines added: 61
Total lines removed: 0
Total lines changed: 61

Detailed view

+------------------------------------------------+-------+------+
| File                                           | Lines | Diff |
+------------------------------------------------+-------+------+
| ethrex/crates/blockchain/blockchain.rs         | 3620  | +23  |
+------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/engine/payload.rs | 1456  | +18  |
+------------------------------------------------+-------+------+
| ethrex/crates/networking/rpc/rpc.rs            | 1733  | +20  |
+------------------------------------------------+-------+------+

[github-actions]

🤖 Claude Code Review

Review: perf(l1): remove per-block thread creation from the block pipeline

Overall, this is a well-reasoned, well-instrumented performance change. The reasoning about rayon::Scope/in_place_scope panic semantics is sound, and the refactor from std::thread::spawn + join() to catch_unwind + a persistent pool preserves the original error-on-panic behavior correctly. I verified the working tree already reflects this PR's final commit (f56c8b0), and reviewed the full diff.

Correctness / concurrency analysis (no bugs found)

• in_place_scope + ps.spawn pattern (blockchain.rs:~1270-1310): The merkleizer is enqueued via Scope::spawn (structured, panic-tracking) while execution runs inline via catch_unwind on the calling thread. Since merkle_closure's entire body is wrapped in catch_unwind before the unconditional merkle_tx.send(result), the only way a panic could escape into rayon's own scope-panic mechanism (which re-raises on the calling thread when in_place_scope returns) is if send itself panics — which mpsc::SyncSender::send doesn't do. This avoids a subtle failure mode where an uncaught panic would surface as an unhandled panic on the caller rather than a clean ChainError.
• Ordering: The merkleizer is now spawned onto the pool before execution starts inline, matching the stated goal (hide the pool hand-off latency behind execution) and preserving the required "channel exists ⇒ merkleizer must drain it" invariant from before.
• Borrow safety: max_queue_length_ref: &mut usize and parent_header_ref: &BlockHeader are moved into the pool-spawned closure exactly as before (previously moved into spawn_scoped); the compiler still enforces exclusive access, so there's no new aliasing/data-race risk versus the prior thread-based version.
• Warmer skip for empty-tx blocks (blockchain.rs:1019-1023, 1035): Confirmed downstream that warm_handle: None is already handled gracefully (.unwrap_or(Duration::ZERO) at blockchain.rs:1320-1328), and warming is genuinely best-effort/cache-only (debug! on failure, result discarded) — skipping it cannot affect block validity or the state root. Good call to also gate on block_has_transactions rather than only !collect_witness.
• panic = "deny" in crates/l2/Cargo.toml/crates/prover/Cargo.toml is a clippy lint, not a panic-strategy = "abort" profile setting — no workspace profile sets panic = "abort", so catch_unwind behaves as intended.

Minor observations (non-blocking)

1. crates/blockchain/blockchain.rs:586 — pipeline_pool is hardcoded to 4 threads with no CLI/config knob, unlike other tunables in this codebase (e.g. bal_prefetch_enabled, bal_parallel_exec_enabled). Given the justification ("headroom for overlapping blocks") is somewhat empirical, a future node operator on a machine with many concurrent payloads (e.g. heavy reorg activity) has no way to tune this. Not required for this PR's scope, but worth a follow-up if overlap becomes common.
2. Doc duplication — the rationale for pipeline_pool (start-delay numbers, "separate from merkle_pool because it opens a scope on that one") is repeated almost verbatim in both the field doc comment (blockchain.rs:282-289) and build_pipeline_pool's doc comment. Could consolidate to one location to avoid drift if the numbers change later.
3. [METRIC] NEWPAYLOAD / [METRIC] ENGINE_RPC logs are only emitted on the success path — if handle_new_payload_v4 or any earlier check returns Err/early-return (undecodable_bal, UnsupportedFork, WrongParam), the handler-level metric line is skipped. This is presumably intentional (these are perf metrics, not audit logs), but worth confirming it matches the intended use of these two new INFO lines for ongoing perf monitoring — a spike in early rejections wouldn't show up in [METRIC] NEWPAYLOAD at all.
4. checks_ms is a derived value (total - decode - execute, clamped at 0) rather than measured directly — fine for an approximate diagnostic metric, just noting it's not a direct measurement so small clock-resolution artifacts are expected and already handled via .max(0.0).
5. ENGINE_RPC metric only logs for RpcRequestWrapper::Single — a batched engine_newPayload call (unusual for consensus clients, but technically valid JSON-RPC) wouldn't be logged. Acceptable given real-world engine API usage is always single-request.

No security, gas-accounting, consensus-rule, or RLP-encoding concerns — this PR touches only pipeline scheduling/threading and adds observability, without altering execution, validation, or state-root computation logic (the diff in the execution/merkle closures is a pure move-into-closure refactor with no logic changes, confirmed line-by-line against the pre-PR version).

---

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

[github-actions]

Benchmark Block Execution Results Comparison Against Main

Command	Mean [s]	Min [s]	Max [s]	Relative
base	59.506 ± 0.373	59.008	59.898	1.03 ± 0.01
head	57.718 ± 0.179	57.455	57.937	1.00