[MegaRedHand]

Motivation

The first block of every epoch runs process_epoch inline on the chain actor (fork_choice::on_block, then stf::state_transition, then process_slots), followed by the post-epoch rehash the state-root check needs. In the import replay such blocks take ~1.5 s against ~0.58 s for an ordinary block (measured on the replay).

The transition depends only on the parent, which is known by the end of the previous epoch's last slot, so it can run ahead of time on a blocking worker, off the import path.

Design

Part	Behaviour
Trigger: head	An import leaves a block at the last slot of epoch E as head: precompute (E+1, head_root). Covers a late last-slot block.
Trigger: timer	Three quarters into the last slot of E, derived from the configured slot duration. The once-per-slot tick arms a delayed self-message on last slots only (send_after). Starts a worker only if none is running and the key is not cached. Covers a skipped last slot, where the head is an earlier block.
Gate	Beacon only. Skipped while the sync tracker says syncing, and for a head more than one slot behind the wall clock (the tracker updates once per slot, so a follower replaying history would otherwise precompute every epoch).
Work	spawn_blocking: clone the head state, process_slots to the first slot of E+1, flush pending writes, hash_tree_root so tree-node hashes are memoized, then message the state back. At most one worker in flight; deduped by (epoch, root) through the cache.
Store	CacheKey::CheckpointState { epoch: E+1, root: head_root }. checkpoint_state derives exactly that value for the checkpoint (the checkpoint block's post-state advanced to the epoch's first slot), so attestation targets for E+1 hit it too. A unit test asserts equal hash_tree_root.
Consume	on_block: when the parent's slot is before the block epoch's first slot and the block is at or after it, look up CheckpointState { block_epoch, parent_root }. A hit clones the entry, advances any remaining skipped slots, and applies the block with the new stf::apply_block (the part of state_transition after process_slots; process_slots rejects a target equal to the current slot, so a boundary block cannot go through state_transition).
Miss / race	Today's path, unchanged. A block that arrives while the worker runs imports inline; the late result is stored and unused by that block.

Forks: process_slots performs the fork upgrade at the boundary, so nothing here is fork-specific.

New metrics (documented in docs/metrics.md): lean_beacon_epoch_precompute_lookups_total{result} (epoch-crossing imports only), lean_beacon_epoch_precompute_seconds, lean_beacon_epoch_precompute_started_total{trigger}.

Not in this PR

• Progressive balances (O(1) justification, and a cheaper compute_pulled_up_tip).
• Holding the block when it races the worker (it computes inline instead, so no ordering dependency is added).
• Advancing the head every slot; only epoch boundaries are advanced.

Testing

• ethlambda-storage lib, ethlambda-state-transition lib, ethlambda-blockchain lib, beacon spec suite (mainnet), clippy and cargo check -p ethlambda: see counts below.
• New unit tests: resuming from a precomputed state gives the inline post-state (block on the boundary and with 1 and 3 skipped slots, compared with the block's committed state root); the lookup applies only to epoch-crossing imports; the worker's state equals checkpoint_state's for the same key; trigger decisions as a pure function (head, timer, syncing, stale head, in-flight, already cached) and the three-quarter delay.
• There is no actor-level test: the beacon actor's test harness drives handlers without a Context, so the trigger decision is tested as a pure function instead.
• Not measured: no benchmark was run for this PR.

Results: storage lib 138 passed (1 ignored); state-transition lib 327 passed (16 ignored); blockchain lib 217 passed; beacon spec suite (mainnet) 5734 passed, 4 failed (the 4 expected gossip matched_fixture_cases on the empty gossip dir), 152 ignored; clippy (workspace, and state-transition with beacon-spec-tests) clean; cargo check -p ethlambda clean.

[github-actions]

🤖 Claude Code Review

Review: PR 652, epoch-transition precompute

The design is sound and the correctness argument holds up. The cache key CheckpointState { epoch, root } is what checkpoint_state already derives, and a test asserts the two have the same hash_tree_root. The fallback to the inline path on a miss makes the precompute a pure speed trade. apply_block correctly handles a block sitting exactly on the boundary, where process_slots would reject an equal target slot. The cached entry is never mutated, since the import clones it. The decide function is pure and well tested.

I found no consensus-correctness bugs. A few robustness points:

1. Worker panic wedges the precompute permanently (epoch_precompute.rs:217-224, :236). epoch_precompute_in_flight is only cleared in the EpochPrecomputed handler. If advance_to_epoch_start panics inside spawn_blocking, no message is sent, in_flight stays Some, and decide returns None forever. Every later epoch silently falls back to the inline path, with no log. The beacon STF has several unreachable!/expect paths, such as the lean-state arms and SpecAssert. Fix: wrap the closure in catch_unwind and send Err. Alternatively, await the JoinHandle in a small spawned task and send the failure from there. A cheap backstop is to store the start Instant alongside the key and ignore an in_flight older than about one slot.

2. Stale timer after a delayed tick (epoch_precompute.rs:157-165). If the tick is delayed past the three-quarter point, delay saturates to 0 and the check fires immediately. That is fine for the current slot. After a long stall that catches up through older last slots, however, the timer can fire for a slot well behind the wall clock. The Timer arm of decide checks head_slot > slot but not freshness against current_slot, so it could precompute an epoch nobody needs. The syncing gate probably covers most of this. Adding current_slot > slot to the Timer guard (slot is the wall slot when the timer is armed) would make the gate symmetric with Head.

3. Cache eviction can discard the result before use (epoch_precompute.rs:241). The result lives in the shared state cache, which attestation-target checkpoint states also churn. The docs mention eviction as a possible cause of misses. It would be worth confirming the cache capacity comfortably holds the entry for the roughly 4 to 12 s between precompute and block arrival. If the cache is small, a miss would show up as lookups_total{result="miss"}.

4. Minor: repeated work per tick (lib.rs:1035-1036). maybe_start_epoch_precompute(Head) runs on every tick and does beacon_head() and a cached_state lookup before decide can reject. It is cheap, but you could short-circuit with the is_last_slot_of_epoch(head_slot) check first. That would skip the cache lookup on 31 of every 32 slots.

5. Nit: duplicated epoch math (epoch_precompute.rs:175-178 and :101-118). The candidate epoch is computed in maybe_start_epoch_precompute and again in decide. If the two drift apart, already_cached would be checked against the wrong key. Consider a candidate_epoch(trigger, head_slot) helper shared by both.

6. Nit: three_quarter_slot computes slot_duration_ms / 4 * 3, which drops up to 3 ms of rounding. That is harmless and not worth changing.

The tests cover the happy path, decide's branches, and resume-versus-inline equivalence for boundary and skipped-slot blocks. A test for the worker-failure path (Point 1) would be a good addition once that is handled.

Point 1 is the only one I'd want addressed before merge. The rest are optional.

---

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

[github-actions]

🤖 Codex Code Review

Looks mostly solid. The precompute path is well-contained, keeps cache residency out of consensus decisions, and the transition_block fallback preserves correctness.

• Potential correctness bug: stale worker results can overwrite a newer cached checkpoint state for the same key. In crates/blockchain/src/epoch_precompute.rs:231, the handler unconditionally clears epoch_precompute_in_flight, then caches msg.result at crates/blockchain/src/epoch_precompute.rs:236. If worker A is started for key (epoch, root), later worker B starts for a different key, and A finishes after B has become the current in-flight worker, A’s completion will still clear the in-flight marker and store its result. That opens two races:

  • a second worker can start while B is still running, violating the “at most one worker” invariant;
  • an older result can overwrite a newer cached state for the same CheckpointState key if the head rolls back/reorgs and later returns to the same root.
    Suggestion: only clear/store when self.epoch_precompute_in_flight == Some(msg.key), otherwise ignore the stale completion. See crates/blockchain/src/epoch_precompute.rs:231.

• Maintainability/readability: maybe_start_epoch_precompute computes candidate_epoch before calling decide, then decide recomputes the epoch from the trigger again. That duplication in crates/blockchain/src/epoch_precompute.rs:170 and crates/blockchain/src/epoch_precompute.rs:190 is easy to drift. Consider deriving already_cached after decide returns the actual key, or refactoring decide to accept less duplicated state.

• Performance: advance_to_epoch_start deliberately pre-hashes the state in crates/blockchain/state_transition/src/beacon/fork_choice.rs:904, which is sensible for the fast path. No obvious extra clone/hash churn beyond what the design intends.

• Consensus/security: I don’t see a fork-choice, attestation, justification/finalization, or SSZ correctness regression in the new transition_block/precomputed_epoch_state flow. The fallback remains the canonical stf::state_transition, and the cached path only substitutes an equivalent process_slots prefix at the epoch boundary (crates/blockchain/state_transition/src/beacon/fork_choice.rs:919 and crates/blockchain/state_transition/src/beacon/fork_choice.rs:974).

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 for epoch-transition precompute optimization in the beacon chain. Let me analyze the code carefully.

Overall Assessment

This is a well-structured optimization PR that precomputes epoch-boundary states to reduce import latency. The design is sound, but I've identified several issues ranging from minor correctness concerns to potential race conditions.

---

Critical Issues

1. Integer Overflow in three_quarter_slot (Line 147, epoch_precompute.rs)

pub(crate) fn three_quarter_slot(slot_duration_ms: u64) -> Duration {
    Duration::from_millis(slot_duration_ms / 4 * 3)
}

Problem: The multiplication slot_duration_ms / 4 * 3 can overflow if slot_duration_ms is large. While slot_duration_ms / 4 is safe, the subsequent * 3 could overflow for values > u64::MAX / 3 (~6.1 million seconds, unlikely for slot durations but a correctness issue).

Fix: Use checked arithmetic or reorder: slot_duration_ms.saturating_mul(3).saturating_div(4) or Duration::from_millis(slot_duration_ms / 4 * 3) → Duration::from_millis(slot_duration_ms.saturating_mul(3) / 4) with explicit comment.

Actually, re-reading: slot_duration_ms / 4 * 3 is evaluated left-to-right as (slot_duration_ms / 4) * 3. For typical values (12_000 ms) this is fine. But for defense in depth:

Duration::from_millis(slot_duration_ms.saturating_mul(3) / 4)

Or use checked_mul and panic/return default on overflow since this indicates a config error.

---

2. Race Condition: epoch_precompute_in_flight Not Cleared on Actor Restart (Lines 534-540, lib.rs)

epoch_precompute_in_flight: None,

This is set in multiple places (BlockChain::new, from_store, test constructors). However, if the BlockChainServer is reconstructed from store (restart), any previously in-flight precompute is lost. More critically, if a precompute worker completes after the actor has been dropped and recreated, the EpochPrecomputed message will arrive at a fresh actor with epoch_precompute_in_flight: None, which is harmless. But if the actor is restarted while a worker is running, the new actor won't know about it and could start a duplicate.

This is acceptable given the "at most one" is best-effort and decide() also checks already_cached. But document this explicitly.

---

3. ms_into_slot Can Return Negative, Cast to u64 Loses Information (Line 160-161, epoch_precompute.rs)

let into_slot = Duration::from_millis(self.ms_into_slot(slot).max(0) as u64);

Problem: ms_into_slot returns some signed type (presumably i64). If it's significantly negative (clock went backwards, or logic error), .max(0) clamps to 0, which is correct behavior. However, the as u64 cast from i64 is safe here due to .max(0).

Minor concern: What if ms_into_slot returns i64::MIN? The .max(0) handles it. This looks okay but verify ms_into_slot's return type is bounded.

---

Security / Consensus-Critical Issues

4. Precompute State Could Diverge from Canonical Path (Lines 95-101, fork_choice.rs)

pub fn advance_to_epoch_start(
    parent_state: &BeaconState,
    epoch: Epoch,
    config: &Config,
) -> Result<BeaconState> {
    let target_slot = compute_start_slot_at_epoch(epoch);
    let mut state = parent_state.clone();
    if state.slot() < target_slot {
        stf::process_slots(&mut state, target_slot, config)?;
    }
    state.apply_pending_mutations();
    let _ = state.hash_tree_root();
    Ok(state)
}

Critical observation: The comment says "A state already at or past the epoch's first slot comes back unchanged." But the condition is state.slot() < target_slot. If state.slot() > target_slot, it returns unchanged, which violates the invariant that the returned state should be at target_slot.

However, looking at callers: this is only called from the precompute worker with parent_state at a last-slot-of-epoch, so state.slot() should always be < target_slot. The guard is defensive.

More serious: The let _ = state.hash_tree_root(); discards the result. If hashing fails (e.g., due to malformed state after process_slots), this is silently ignored. The state is still cached with uncomputed/memoized hashes, which could cause issues when later cloned.

Fix:

state.hash_tree_root().map_err(|e| Error::HashTreeRoot(e))?;

Or if the hash is purely for memoization side effects and failure is impossible, document why let _ is safe.

---

5. transition_block Clones Precomputed State Without Verifying Hash Integrity (Lines 137-138, fork_choice.rs)

let mut state = (*precomputed).clone();

The precomputed state was hashed in advance_to_epoch_start, but cloning an Arc<BeaconState> and then (*precomputed).clone() does a deep clone. The memoized hashes in the tree should be preserved, but this depends on BeaconState::clone() implementation. If hash_tree_root() memoization is in an Arc<Mutex<_>> or similar, cloning should preserve it. Verify this assumption holds.

---

6. Metrics Counter Can Be Incremented for Non-Epoch-Crossing Blocks (Lines 1015-1018, fork_choice.rs)

fn precomputed_epoch_state(...) -> Option<Arc<BeaconState>> {
    // ...
    crate::metrics::inc_epoch_precompute_lookups(if cached.is_some() { "hit" } else { "miss" });
    cached
}

Wait, re-reading: the metric is only incremented when this function is called, which is only from transition_block, which is only called from on_block when... actually transition_block is always called. Let me re-check.

Looking at on_block (line 2544), transition_block is always called. Inside transition_block, precomputed_epoch_state is called, and it increments the metric. But the metric comment says "Counts a hit or a miss for every such import and for no other." The function returns None early for non-epoch-crossing, but the metric is incremented before the early return!

Wait no, re-reading lines 1009-1018:

fn precomputed_epoch_state(
    store: &Store,
    parent_slot: Slot,
    parent_root: Root,
    block_slot: Slot,
) -> Option<Arc<BeaconState>> {
    let epoch = compute_epoch_at_slot(block_slot);
    let epoch_start = compute_start_slot_at_epoch(epoch);
    if !(parent_slot < epoch_start && epoch_start <= block_slot) {
        return None;
    }
    let cached = store.cached_state(CacheKey::CheckpointState {
        epoch,
        root: parent_root,
    });
    crate::metrics::inc_epoch_precompute_lookups(if cached.is_some() { "hit" } else { "miss" });
    cached
}

Bug: The metric is not incremented for non-epoch-crossing blocks because of the early return. Good. But wait—transition_block calls precomputed_epoch_state unconditionally, so for non-epoch-crossing blocks, the function returns None immediately without touching metrics. For epoch-crossing blocks, it always increments hit or miss. This matches the documented behavior. ✓

---

Correctness Issues

7. decide Function: head_slot >= compute_start_slot_at_epoch(epoch) Check Is Redundant? (Lines 116-118, epoch_precompute.rs)

if inputs.head_slot >= compute_start_slot_at_epoch(epoch) {
    return None;
}

Given epoch = head_slot / SLOTS_PER_EPOCH + 1, compute_start_slot_at_epoch(epoch) is (head_slot / SLOTS_PER_EPOCH + 1) * SLOTS_PER_EPOCH. For head_slot = 2*SPE - 1 (last slot), this is 2*SPE. So head_slot >= 2*SPE is 2*SPE - 1 >= 2*SPE, false.

For head_slot = 2*SPE (first slot of epoch 2), epoch = 2 + 1 = 3, compute_start_slot_at_epoch(3) = 3*SPE. Check: 2*SPE >= 3*SPE, false.

Actually this seems like a sanity check that head_slot hasn't already reached or passed the target. But given how epoch is computed, head_slot is always in the previous epoch, so head_slot < compute_start_slot_at_epoch(epoch) always holds. This check is defensive but could indicate a logic error elsewhere if triggered.

Suggestion: Add a debug_assert! or comment explaining this is a defensive invariant.

---

8. Timer Trigger Uses slot Not head_slot for Epoch Calculation (Lines 108-113, epoch_precompute.rs)

Trigger::Timer { slot } => {
    // ...
    slot / preset::SLOTS_PER_EPOCH + 1
}

But in maybe_start_epoch_precompute (line 178-181):

let candidate_epoch = match trigger {
    Trigger::Head => head_slot / preset::SLOTS_PER_EPOCH + 1,
    Trigger::Timer { slot } => slot / preset::SLOTS_PER_EPOCH + 1,
};

The decide function recomputes epoch internally. The candidate_epoch in maybe_start_epoch_precompute is only used for already_cached check. This is correct but slightly redundant. Not a bug.

---

9. apply_block is Public But Documents Internal Use (Lines 147-163, stf/mod.rs)

/// [`state_transition`] from the point where `state` is already at the block's
/// own slot...
pub fn apply_block(...)

Making this pub exposes an internal API. The documentation says "For a caller that advanced the state itself" which is exactly the precompute use case. This is acceptable but consider pub(crate) unless external crates need it.

---

Performance Issues

10. tokio::task::spawn_blocking Without Handle Tracking (Line 228, epoch_precompute.rs)

tokio::task::spawn_blocking(move || {
    let result = {
        let _timing = stf_metrics::time_epoch_precompute();
        fork_choice::advance_to_epoch_start(&head_state, key.epoch, &config)
    };
    let result = result.map(Arc::new).map_err(|err| err.to_string());
    let _ = actor.send(EpochPrecomputed { key, result });
});

Problem: The spawned task is not tracked. If the actor is shut down, the task continues running (wasted work). More importantly, under heavy load, multiple spawn_blocking calls could exhaust the blocking thread pool.

Mitigation: The epoch_precompute_in_flight flag prevents multiple precomputes for the same key, but doesn't prevent queueing if the blocking pool is saturated. Given the "at most one" design, this is acceptable. Consider using spawn_blocking with a named task or tracking the JoinHandle for shutdown.

Also: let _ = actor.send(...) silently drops send failures. If the actor is shut down, the result is lost and epoch_precompute_in_flight is never cleared. However, the actor's Drop or restart would reset this. In practice, this is acceptable.

---

11. head_state is Cloned Twice (Lines 220-228, epoch_precompute.rs)

let Ok(Some(head_state)) = self.store.get_state(&head_root) else {
    return;
};
// ...
tokio::task::spawn_blocking(move || {
    let result = {
        // ...
        fork_choice::advance_to_epoch_start(&head_state, key.epoch, &config)
    };

head_state is cloned from the store (first clone), then advance_to_epoch_start does parent_state.clone() (second clone). The first clone happens on the actor thread; the second on the blocking thread. This is unavoidable given the API, but consider if get_state can return an Arc<BeaconState> to avoid the first clone.

Actually, looking at the type: get_state returns Result<Option<BeaconState>>, not Arc. So the first clone is required to move into the blocking task. The second clone in advance_to_epoch_start is also required since process_slots mutates. This is correct but expensive for large states.

---

Error Handling Issues

12. get_state Error Silently Ignored (Line 220, epoch_precompute.rs)

let Ok(Some(head_state)) = self.store.get_state(&head_root) else {
    return;
};

If get_state returns Err(_), the precompute is silently skipped. This is correct behavior (retry later), but consider logging at debug level to help diagnose store issues.

---

13. advance_to_epoch_start Error Converted to String (Line 229, epoch_precompute.rs)

let result = result.map(Arc::new).map_err(|err| err.to_string());

This loses error context and type information. The EpochPrecomputed result uses String for errors, which is fine for logging but prevents programmatic handling. Given the handler just logs, this is acceptable.

---

Code Quality / Maintainability

14. Inconsistent Naming: epoch_precompute_in_flight vs PrecomputeKey (Lines 543-545, lib.rs)

epoch_precompute_in_flight: Option<epoch_precompute::PrecomputeKey>,

The field name suggests a boolean or worker handle, but it stores the key. This is correct (stores what is in flight, not whether something is in flight), but slightly confusing. Consider epoch_precompute_key_in_flight or document.

---

15. Test last_slot_parent Uses expect Heavily (Lines 3940-3955, fork_choice.rs)

fn last_slot_parent() -> BeaconState {
    let mut state = with_signing_validators_at(crate::beacon::ForkName::Fulu, 64);
    let lookahead =
        crate::beacon::helpers::fulu::initialize_proposer_lookahead(&state).expect("lookahead");
    // ...
}

Multiple expect calls in test setup. Acceptable for tests, but if any fails, the panic message is just the string. Consider unwrap_or_else(|e| panic!("...: {e:?}")) for better diagnostics.

---

16. block_on Function Name Conflicts with tokio::runtime::Runtime::block_on (Line 3959, fork_choice.rs)

fn block_on(parent: &BeaconState, slot: Slot) -> (SignedBeaconBlock, Root) {

This shadows the well-known block_on from async runtimes. In a test module it's acceptable, but consider build_block_at or propose_block_at to avoid confusion.

---

Documentation Issues

17. Doc Comment on advance_to_epoch_start Claims "Already At or Past" (Lines 888-895, fork_choice.rs)

/// A state already at or past the epoch's first slot comes back unchanged, as
/// `checkpoint_state` returns it.

But the code only checks state.slot() < target_slot. If state.slot() > target_slot, it returns unchanged (not at target). The comment should say "already at the epoch's first slot" or the code should handle > target_slot by erroring or clamping.

Fix: Change comment to "A state already at the epoch's first slot comes back unchanged" since > target_slot shouldn't happen in correct usage.

---

Positive Acknowledgments

1. Excellent test coverage for decide logic with clear, isolated test cases covering all branches.
2. Metrics integration is thorough with appropriate labels and buckets.
3. The transition_block abstraction cleanly separates precomputed vs inline paths without duplicating apply_block logic.
4. Cache key reuse with CheckpointState is elegant and avoids cache fragmentation.
5. Defensive programming with multiple guard conditions (syncing, stale head, in-flight, already cached).
6. Documentation in beacon_stf.md and metrics.md is comprehensive.

---

Summary Table

Priority	Item	File	Line	Issue
High	4	fork_choice.rs	888-901	hash_tree_root() error silently ignored
Medium	1	epoch_precompute.rs	147	Potential overflow in three_quarter_slot
Medium	10	epoch_precompute.rs	228	spawn_blocking task not tracked
Medium	5	fork_choice.rs	137-138	Clone of precomputed state assumes hash memoization preserved
Low	17	fork_choice.rs	888-895	Doc comment overstates "at or past" behavior
Low	16	fork_choice.rs	3959	block_on name shadows tokio function
Low	12	epoch_precompute.rs	220	Store error silently ignored, consider debug log
Low	9	stf/mod.rs	147	apply_block is pub, consider pub(crate)

The PR is well-designed and the core logic is correct. The most important fix is Item 4 (handling hash_tree_root failure), followed by the overflow check in Item 1.

---

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

[ilitteri]

Medium: Timer-triggered precompute can replay an unbounded number of historical epochs.

The Timer arm at crates/blockchain/src/epoch_precompute.rs:105-112 checks only that the timer names an epoch's last slot and that head_slot <= slot. Unlike the Head arm, it never uses current_slot or otherwise bounds the head's lag.

During a network-wide stall, the sync tracker deliberately reports syncing = false once the freshest-known block is over eight slots old (crates/blockchain/src/sync_status.rs:98-100). A timer several epochs ahead of the head can therefore start advance_to_epoch_start, replaying every intervening slot and epoch on a blocking worker. If the stall continues, later epochs use new cache keys and repeat increasingly expensive work from the same old head. This also contradicts the general freshness gate documented in docs/beacon_stf.md:578.

Please bound Timer precompute to a head within the trigger's epoch, or another explicitly bounded lag that still supports skipped slots. Please also add a stale-Timer test; the existing stale-head test covers only Trigger::Head.