[MegaRedHand]

🗒️ Description / Motivation

With an execution client, a pre-gloas beacon import waited for engine_newPayload and then ran the state transition, so the two summed. The request is now built on the actor and sent from a spawned task, and the actor computes the transition meanwhile, so the import costs the larger of the two.

This is possible because the transition reads the engine's verdict in one place (process_execution_payload), and on_block maps every verdict except INVALIDATED to ExecutionEngine::valid(). A transition computed assuming a valid payload is therefore the one on_block would run for every verdict it imports.

What Changed

• fork_choice.rs: speculate_transition computes the block's transition against its parent's post-state with a valid() engine and returns it as a local value (SpeculativeTransition). on_block_speculative is on_block with an optional speculation: every check runs unchanged and in the same order; only the transition_block call is replaced, and only when the speculation was computed for this block root and parent root and the verdict is not INVALIDATED. on_block keeps its signature as a wrapper, so spec-test runners and the replay harness (no engine) are untouched.
• beacon_engine.rs: OwnedNewPayloadRequest (copies only the payload, never the block, so the request can move into a task) and ask_request.
• lib.rs (process_block): spawns the request, runs speculate_transition, awaits the task, then hands the speculation to on_block_speculative.
• import_timing.rs: the transition that overlapped the round trip is reported under stf, and engine reports only the wait left after it, so the rows still tile the end-to-end time. A discarded speculation leaves engine covering the overlap.
• metrics.rs: lean_engine_speculation_total{outcome} counts used and discarded attempts.
• docs/beacon_engine.md, docs/benchmarking.md, docs/metrics.md.

Correctness / Behavior Guarantees

Verdict	Speculative post-state
VALID, or SYNCING/ACCEPTED for an optimistic candidate	used
INVALIDATED	dropped; on_block recomputes with invalid(), the invalidation path is unchanged
no answer, or SYNCING/ACCEPTED for a non-candidate	dropped; the block is held as before
computed for another root or parent	dropped

• Nothing reaches the store, a non-content-addressed cache or the network until on_block_speculative accepts the speculation.
• A panicked or cancelled request task is treated as no verdict, like an exhausted retry ladder.
• Without an execution client, and for forks with no request to send (pre-merge, gloas), nothing overlaps and the path is the old one. The gloas envelope path (beacon_envelope.rs) is unchanged: it still runs the consensus checks before asking the execution client.
• The overlap is real on the production runtime: the chain actor runs on Backend::Thread, whose thread blocks on a multi-thread tokio Runtime, so the spawned request progresses on that runtime's workers while the actor computes.

Tests Added / Run

• New fork_choice tests: a matching speculation leaves the store exactly as importing without one; it is the transition on_block uses; a speculation for another root or parent is ignored; an INVALIDATED verdict ignores it and matches today's result; no speculation without a parent state.
• cargo fmt --all -- --check: clean
• make lint: clean
• cargo test --profile release-fast -p ethlambda-state-transition -p ethlambda-blockchain --lib --bins: state-transition 635 passed (15 ignored), blockchain 293 passed (43 ignored)
• Not run locally: make test (spec suites); CI covers them.
• Not yet run against a live execution client in isolation. It runs on ethlambda-5's mainnet node (nightly with this batch, paired with ethrex v30.0.1), where lean_engine_speculation_total{outcome} shows how often the speculation is used.

Related Issues / PRs

Part of a beacon import perf batch, each PR independent and based on main: #752, #753, #754. Overlaps: the STF phase timing PR also edits docs/benchmarking.md; #754 and the STF phase timing PR edit fork_choice.rs in different regions; most batch PRs edit docs/metrics.md.

✅ Verification Checklist

• Ran make fmt — clean
• Ran make lint (clippy with -D warnings) — clean
• Ran make test (test-consensus plus test-node, at release-fast) — not run locally (spec suites); unit tests of the touched crates pass

[github-actions]

🤖 Claude Code Review

Review of PR 755: run the beacon state transition while the engine verdict is in flight

The change is sound overall. The approach is conservative and the correctness argument is explicit. I found no blocking bugs.

What checks out

• on_block_speculative keeps every on_block check in its original order. The speculation only replaces the single transition_block call, so rejection behaviour is unchanged.
• The speculation is dropped for Invalidated, the one verdict whose engine (invalid()) differs from the valid() engine it assumed. It is also re-checked against (block_root, parent_root) inside on_block_speculative, so lib.rs cannot pass one that doesn't fit.
• SpeculativeTransition is opaque and nothing reaches the store before the checks, so dropping one costs only the work.
• The Held paths (no verdict, non-candidate optimistic) return before on_block. Nothing leaks and engine_start is left untouched, so the timing rows stay honest.
• A panicked or cancelled task is treated as "no verdict" and the block is held, which matches the optimistic-sync rule for an exhausted retry ladder.
• The new timing test shows the rows still tile the span. The test refactor into store_with_valid_child is a clean extraction.

Points to consider

1. Overlap depends on the runtime. lib.rs:1820-1843 calls tokio::spawn and then runs the STF synchronously on the actor's thread. If the actor runs on a current-thread runtime, or all workers are busy, the spawned task can't make progress during speculate_transition. The result is correct but gains nothing. Please confirm the runtime is multi-threaded. The lean_engine_speculation_total{outcome="used"} metric will not reveal a missing overlap. The engine timing row would, because it should shrink toward zero.
2. STF now runs before on_block's cheap rejections. Before this PR, a block failing an early on_block check (slot or finalization bounds, future block, and so on) never reached the STF. Now an execution-client deployment pays for the full STF on such blocks. Gossip validation mostly filters them first. Blocks arriving through BlockSource::Sync or by-root fetches are less filtered, so a peer feeding junk descendants costs one STF each. The doc comment already says "a block that then fails one of on_block's earlier checks still counts". A cheap pre-filter (slot and finalized-descendant) before the spawn would remove the exposure. It is acceptable as is if you judge the exposure low.
3. Payload clone. into_owned (beacon_engine.rs) clones the whole ExecutionPayload on every block, including all transactions, which can be megabytes. This is small next to the STF, but you could avoid it by moving the payload out of the block, or by sharing it through Arc, if the block type allows that.
4. Dead mismatch branch. The discarded_mismatch metric and the is_for pre-check in lib.rs cover a case the code says is "not expected from the actor". on_block_speculative already repeats the check. That is fine as defence in depth, but the metric label will probably always read zero.
5. Minor. verdict is a Result<Option<_>, String> whose error is stringified only to be logged. Keeping the error type, or logging at the match site, would be a little more idiomatic. It is not important.

Verdict: correct and well documented. I'd confirm Point 1 and decide on Point 2 before merging.

---

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

[github-actions]

🤖 Codex Code Review

Looks good overall. I did not find a consensus-safety or memory-safety bug in the speculative engine_newPayload overlap path, and the added tests cover the key invariants well.

• The strongest part of the change is the guard that only reuses speculation when both block_root and parent_root match and the verdict is not Invalidated; that preserves the old invalidation path and avoids contaminating fork choice with a transition run under the wrong engine assumption. See crates/blockchain/state_transition/src/beacon/fork_choice.rs:4828 and crates/blockchain/src/lib.rs:1912.
• The held-block correctness fix remains intact: on_block_speculative still guards on has_state rather than has_block, which is the right distinction for persisted-but-unimported blocks. See crates/blockchain/state_transition/src/beacon/fork_choice.rs:4907.
• Timing/reporting math also looks internally consistent: when speculation is used, engine_start is shifted to spec_end and the speculative span is added back into stf, so the rows still tile end-to-end time without double counting. The unit test at crates/blockchain/src/import_timing.rs:923 exercises that directly.

One minor point to consider:

• OwnedNewPayloadRequest always clones a full Deneb execution payload before spawning the task. That is functionally correct, but it can be a noticeable per-block allocation/copy on busy import paths. If profiling ever shows this path hot, it may be worth exploring an Arc-backed request object or moving request construction into the spawned task to reduce copying. Current code is still acceptable and safe. See crates/blockchain/src/beacon_engine.rs:189.

If you want, I can turn this into ready-to-post GitHub review comments.

---

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

[github-actions]

🤖 Kimi Code Review

I'll review this PR which introduces speculative state transitions that run concurrently with engine_newPayload round trips. This is a performance optimization for beacon block import with execution clients.

Overall Assessment

The design is sound: compute the state transition with ExecutionEngine::valid() while waiting for the execution client's verdict, then reuse if the verdict matches. The safety arguments in documentation are well-reasoned. However, I've identified several issues ranging from correctness concerns to code quality.

---

Critical Issues

1. Race condition in speculate_transition — store mutation between speculation and use

File: crates/blockchain/state_transition/src/beacon/fork_choice.rs, lines 4848–4877

pub fn speculate_transition(
    store: &Store,
    // ...
) -> Option<SpeculativeTransition> {
    // ...
    let parent_state = store.get_state(&parent_root).expect("get")?;
    // ...
    let (state, outcome) = transition_block(
        store,
        // ...
        &stf::ExecutionEngine::valid(),
        // ...
    );

The documentation states: "The chain actor handles one message at a time, so nothing mutates the store between this computation and on_block's own." This is an architectural invariant, not an enforced one. If this ever changes (e.g., async message handling, background tasks), the speculation could use stale cache state.

Recommendation: Add a debug-only assertion or store generation check in on_block_speculative to verify the store hasn't changed. At minimum, document this invariant more prominently in the code itself, not just in docs/beacon_engine.md.

---

2. expect("get") can panic on database errors

File: crates/blockchain/state_transition/src/beacon/fork_choice.rs, line 4859

let parent_state = store.get_state(&parent_root).expect("get")?;

This .expect("get") will panic if the store returns a database error, not just None. The ? handles None (parent not found), but expect aborts on Err.

Recommendation: Change to proper error handling:

let parent_state = store.get_state(&parent_root).ok().flatten()?;

Or if get_state returns Result<Option<...>>, propagate the error appropriately rather than panicking.

---

3. tokio::spawn without abort handle or timeout — potential task leak

File: crates/blockchain/src/lib.rs, lines 1826–1828

let task = tokio::spawn(async move {
    beacon_engine::ask_request(&client, request).await
});

If the chain actor is dropped or the block import is cancelled, this task continues running. The client is cloned (Arc presumably), so the connection pool stays alive. More critically, if ask_request hangs indefinitely due to an execution client bug, the task leaks.

Recommendation: Consider using tokio::time::timeout around the task or the ask_request call, or at minimum store the JoinHandle to abort on actor shutdown. The existing retry ladder in ask_request should handle most cases, but defense in depth matters for consensus code.

---

Security & Correctness Issues

4. SpeculativeTransition does not validate the block's state root

File: crates/blockchain/state_transition/src/beacon/fork_choice.rs, lines 4830–4841

The SpeculativeTransition stores state and outcome (which is Result<()>), but transition_block returns (BeaconState, Result<()>). The outcome here is the entire transition result — if this is Ok(()), the state transition passed all checks including the state root verification.

However, looking at on_block_speculative (line 5062+), the usable check only verifies is_for and !Invalidated. It does not check that speculation.outcome.is_ok() before using it.

Wait — re-reading: speculation.outcome is stored as Result<()>, but in the destructuring:

let (mut state, transition) = match usable {
    Some(speculation) => (speculation.state, speculation.outcome),
    None => transition_block(...),
};

Then transition? is called at line 5085. So a failed speculation would fail here. But this means we did run a failed transition speculatively, wasting work. More importantly, what if transition_block with valid() succeeds but invalid() would have caught something? The docs argue this can't happen, but...

Actually, re-checking: the ExecutionEngine only affects payload validation. The state root check is independent. So a speculation that fails state root would fail here and fall through to the same error path. This seems correct.

However: The poisoned test at line 7435 verifies this works. No issue here after deeper analysis.

---

5. Missing block_root validation in speculate_transition return value

File: crates/blockchain/state_transition/src/beacon/fork_choice.rs, lines 4850–4851

let block_root = signed_block.message_hash_tree_root();

This recomputes the block root. In on_block_speculative, the same computation happens. These should match, but if SignedBeaconBlock::message_hash_tree_root() has any side effects or depends on mutable state (it shouldn't), this could diverge. The is_for check catches this, but only after the fact.

Recommendation: Not a bug, but consider whether block_root should be computed once and passed to both functions, or if SpeculativeTransition should be constructed by on_block_speculative itself to ensure consistency. The current design is probably fine.

---

Performance Issues

6. Unnecessary clone of execution_payload in into_owned

File: crates/blockchain/src/beacon_engine.rs, lines 195–206

pub fn into_owned(self) -> OwnedNewPayloadRequest {
    OwnedNewPayloadRequest {
        execution_payload: self.execution_payload.clone(),
        versioned_hashes: self.versioned_hashes,
        parent_beacon_block_root: self.parent_beacon_block_root,
        execution_requests: self.execution_requests,
    }
}

The execution_payload is cloned, but versioned_hashes and execution_requests are moved. However, Bytes32 and Vec<Vec<u8>> may also be expensive. More importantly, into_owned takes self by value — why clone execution_payload instead of moving it? Because NewPayloadRequest borrows it.

Wait, re-reading: NewPayloadRequest<'_> has execution_payload: &'a containers::deneb::ExecutionPayload. So it must clone to own it. This is correct but expensive for large payloads (Deneb blobs).

Recommendation: Consider using Arc<containers::deneb::ExecutionPayload> or ensuring the payload can be cheaply shared. For now, document the cost. Not blocking.

---

7. CommitteeCache::default() in tests may hide real cache requirements

File: crates/blockchain/state_transition/src/beacon/fork_choice.rs, lines 7405, 7448, 7480

let committees = CommitteeCache::default();

Multiple test helpers use CommitteeCache::default(). If the real on_block or speculate_transition requires a populated cache for certain operations, these tests may pass incorrectly.

Recommendation: Verify that CommitteeCache::default() is sufficient for all test paths, or construct proper caches from the test state. The existing test on_block_imports_a_block_already_persisted_with_no_post_state was refactored to use store_with_valid_child — ensure the committee cache is still correct.

---

Rust Best Practices & Idiomatic Patterns

8. match on bool in usable filter — prefer if or matches!

File: crates/blockchain/state_transition/src/beacon/fork_choice.rs, lines 5062–5065

let usable = speculation.filter(|speculation| {
    speculation.is_for(block_root, parent_root)
        && !matches!(payload_validity, PayloadValidity::Invalidated { .. })
});

This is fine, but the !matches! with && is slightly hard to read. Consider:

let usable = speculation.filter(|spec| {
    spec.is_for(block_root, parent_root) && !payload_validity.is_invalidated()
});

If PayloadValidity doesn't have is_invalidated(), add it for clarity.

---

9. Option<(SpeculativeTransition, Instant, Instant)> tuple is unwieldy

File: crates/blockchain/src/lib.rs, lines 1811–1814

let mut speculation: Option<(
    fork_choice::SpeculativeTransition,
    Instant,
    Instant,
)> = None;

This tuple type is used once and immediately destructured. Consider a small struct:

struct SpeculationTiming {
    transition: SpeculativeTransition,
    start: Instant,
    end: Instant,
}

This improves readability and makes the metrics::inc_engine_speculation calls clearer.

---

10. verdict type annotation is unnecessary and complex

File: crates/blockchain/src/lib.rs, line 1815

let verdict: Result<Option<fork_choice::PayloadValidity>, String> =

This explicit type annotation is required because the match arms return different types that unify to String errors. However, String as an error type is unidiomatic — consider using EngineError or a dedicated enum.

Actually, looking closer: task.await returns Result<Result<PayloadValidity, EngineError>, JoinError>, and both error cases are converted with .to_string(). This loses structured error information.

Recommendation: Define a small enum or use EngineError throughout. Converting to String early prevents downstream matching and logging of specific error kinds.

---

11. if speculation.is_some() repeated — use is_some_and or map

File: crates/blockchain/src/lib.rs, lines 1869–1871, 1896–1898

if speculation.is_some() {
    metrics::inc_engine_speculation("discarded_held");
}

This appears twice. Consider extracting to a helper or using:

speculation.as_ref().map(|_| metrics::inc_engine_speculation("discarded_held"));

Though the if is clearer. Not a bug.

---

Memory Safety & Error Handling

12. tokio::spawn move closure captures client by clone, but what about request?

File: crates/blockchain/src/lib.rs, lines 1826–1828

let client = client.clone();
let task = tokio::spawn(async move {
    beacon_engine::ask_request(&client, request).await
});

client is cloned (presumably EngineClient is Arc-based or cheap). request is moved. This is correct.

But what if the actor is dropped while the task runs? The client clone keeps the connection alive. If EngineClient holds a reference to the actor, this could prevent cleanup. Verify EngineClient doesn't hold actor references.

---

13. task.await after speculate_transition — sequential, not truly parallel

File: crates/blockchain/src/lib.rs, lines 1826–1841

let task = tokio::spawn(async move { ... });  // spawn first
let spec_start = Instant::now();
let computed = fork_choice::speculate_transition(...);  // then compute
// ...
match task.await { ... }  // then await

This is correct: the spawn starts the network I/O, then we do CPU work, then await. The task runs on another thread/core. Good.

However, if speculate_transition is faster than the task setup, we might await immediately. Not a bug, but the overlap is maximized when speculate_transition is slow.

---

Documentation & Maintainability

14. docs/beacon_engine.md typo: "gloas" should be "gloss" or "gloas" is jargon?

File: docs/beacon_engine.md, line 114

Without an engine, and for forks that have no request to send (pre-merge, gloas)

"gloas" appears in code comments too (crates/blockchain/src/lib.rs:1810). If this is internal jargon, define it. If it's a typo for "gloss" or "gloas" (some fork name?), clarify.

---

15. Test no_speculation_without_a_parent_state mutates block — side effects?

File: crates/blockchain/state_transition/src/beacon/fork_choice.rs, lines 7512–7523

let SignedBeaconBlock::Phase0(inner) = &mut block else {
    unreachable!("phase0");
};
inner.message.parent_root = Root::repeat_byte(0xcd);

This mutates block after store_with_valid_child() returns it. If store_with_valid_child caches or shares the block, this could affect other tests. Currently safe since it's the last use, but fragile.

Recommendation: Clone before mutate, or have store_with_valid_child return an owned block that can be modified.

---

Minor Issues

16. OwnedNewPayloadRequest fields are pub but could be crate-private

File: crates/blockchain/src/beacon_engine.rs, lines 188–193

pub struct OwnedNewPayloadRequest {
    execution_payload: containers::deneb::ExecutionPayload,
    // ...
}

Fields have no visibility modifier, so they're pub in a pub struct. They could be pub(crate) since only ask_request uses them.

---

17. stf_spec_start/stf_spec_end naming is slightly confusing

File: crates/blockchain/src/import_timing.rs, lines 199–211

"spec" could mean "speculative" or "specification". Given the context it's "speculative", but consider stf_overlapped_start or stf_concurrent_start for clarity.

---

Tests Analysis

The tests are comprehensive and well-designed:

• a_matching_speculation_leaves_the_store_as_importing_without_one: Verifies equivalence for NotRequired, Validated, Optimistic. Good coverage.
• a_matching_speculation_is_the_transition_on_block_uses: Uses poisoned outcome to verify the speculation is actually used. Clever.
• a_speculation_for_another_root_or_parent_is_ignored: Tests both block_root and parent_root mismatch.
• an_invalidated_verdict_ignores_the_speculation_and_matches_today: Critical path for safety.

One gap: No test for PayloadValidity::Optimistic with is_optimistic_candidate_block returning false — the speculation discard path on non-candidate optimistic blocks. The metrics call exists (discarded_held) but no test verifies the speculation is dropped in this specific case.

---

Summary

Severity	Count	Categories
Critical	2	Race condition (architectural), panic on DB error
Security/Correctness	1	Task leak potential
Performance	2	Payload clone, cache usage in tests
Rust/Idiomatic	4	Error types, struct vs tuple, visibility, match readability
Minor	4	Typos, naming, test fragility, documentation

Must fix before merge:

1. expect("get") panic (Item 2)
2. Verify task cleanup on actor drop (Item 3)
3. Consider adding defensive check for store mutation (Item 1)

Should fix:
4. String error type in verdict (Item 10)
5. SpeculationTiming struct (Item 9)
6. Test gap for non-candidate optimistic discard

---

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

[MegaRedHand]

Pushed b95be16: the engine import phase now means only the wait for the verdict left once the overlapped state transition finished. It is a recorded 0 when there was no request (no execution client, pre-merge, gloas) and when the answer arrived before the transition finished; it is non-zero only when the execution client was slower than the transition, or gave no answer (the rest of the retry ladder, block held). The speculative transition is charged to stf whether it was used or discarded, so the rows still tile the end-to-end time.

The execution client's own time is a new histogram, lean_engine_new_payload_seconds{outcome} (valid, optimistic, invalidated, no_verdict), observed in ask_request from the request to the answer, retry ladder included.

Checks: cargo fmt, make lint clean; cargo test --profile release-fast -p ethlambda-blockchain --lib 295 passed (43 ignored), including two new import_timing tests (a held block keeps its speculation in stf; an engine with nothing to wait for is a recorded zero).

[MegaRedHand]

Pushed 088dfc2: lean_engine_new_payload_seconds now also times the gloas envelope's engine_newPayloadV5. Every newPayload request goes through one helper, timed_new_payload (ask_request, ask, and ask_envelope_status, which ask_envelope uses); an envelope refused before a request is sent (block mismatch) is not observed. Same labels, so one series covers both forks. make lint clean; cargo test --profile release-fast -p ethlambda-blockchain --lib 295 passed (43 ignored), the envelope tests going through the timed path.

[MegaRedHand]

Block import benchmark (offline replay of mainnet blocks, steady state)

The replay runs without an execution client, so there is no engine verdict for the STF to overlap with, and this PR's code path does not run there. The leg served as a noise control.

leg	STF per block	difference
main 012dc4f	157.7 ms
main + #755	157.1 ms	-0.6 ms (-0.4%), within the ±1.5% round-to-round noise

Measuring this PR needs a live node with an execution client.

Method

• Offline ethlambda benchmark import replay on ethlambda-5 (Ryzen 7 9700X), with the node stopped.
• 3 consecutive mainnet windows fetched from that node: 173 measured blocks (slots 15567200 to 15567385), each window after one unmeasured warm-up epoch. --block-delay 1000.
• One leg per PR: the PR head merged onto main 012dc4f. The BLS PRs (perf(crypto): sum validated public keys through blst's batched affine addition #737, perf(beacon): verify a block's signatures in one random-weight batch #738, perf(beacon): verify attestations against committee key sums #742, perf(crypto): resolve registry public keys through a validator-index table #743) are stacked, so each of those legs includes its parents. --warm-committee-sums on the legs that have the flag (perf(beacon): verify attestations against committee key sums #742, perf(crypto): resolve registry public keys through a validator-index table #743, nightly).
• 2 rounds over all legs, leg order rotated per window. Every run of a window imported the same block roots.
• Numbers are per-block STF means, paired against the reference leg on the same blocks, with a bootstrap 95% CI of the mean difference.
• Steady state: 9 of the 173 blocks are dropped from every leg, because their cost exists only in a fresh process with no gossip. They are the first measured block of each window (about 31k attesters whose keys the process has never validated) and, per epoch, the first block carrying that epoch's attestations (it builds the committee shuffling inside the timed span; a live node's gossip validation builds it first). Including them makes the legs with --warm-committee-sums look about 3 points better than they are, since the flag does both before the timer starts.
• Noise floor: legs whose code the replay cannot exercise move about ±1.5% between rounds.