fix(rpc): bound duties by the wall clock, advance the head for later epochs

[MegaRedHand]

Motivation

On the gloas devnet (#662), the validator client's next-epoch attester-duty request got a 400 ("epoch is not within one epoch of the head state's") at every epoch boundary. At the first slot of epoch N+1 the head is still in epoch N until that slot's block arrives, and the validator client asks for epoch N+2 right then. The node bounded the epoch by the head state; the Beacon API bounds it by the current epoch (up to current_epoch + 1). Other validator clients ask for next-epoch duties early in the epoch too, so a late or missed boundary block hits them the same way.

Changes

• Attester and proposer duties accept any epoch up to max(wall-clock epoch, head epoch) + 1. The wall clock is node::wall_slot, the clock attestation_data already uses; the max keeps a head ahead of a lagging clock from being refused, as PTC duties already do.
• An epoch the head state cannot derive is computed from the head advanced through fork choice's checkpoint_state cache (the start of epoch - 1 for attester duties, epoch - MIN_SEED_LOOKAHEAD for proposer duties), so a repeated request does no epoch processing.
• dependent_root is still read from the head's chain.
• Both duty handlers run on spawn_blocking, like PTC duties.
• Audited: PTC duties and attestation_data already bound by the clock; nothing else bounds by the head's epoch.

Tests

• New: head in epoch N with the clock at the start of N+1 serves N+2 (committees and proposers equal to the spec helpers on a process_slots-advanced state; dependent root is the head); two past the clock is a 400; proposer duties before the head's epoch are a 400.
• Two tests asserted exactly the old head-relative bound and now pin the clock-relative one: attester_duties_two_epochs_ahead_are_a_400 became attester_duties_past_the_clocks_next_epoch_are_a_400, and an_epoch_outside_the_lookahead_is_a_400 became an_epoch_past_the_clocks_next_is_a_400.
• cargo test --profile release-fast --lib: rpc 175, validator 269; fmt and clippy -D warnings clean.

[github-actions]

🤖 Claude Code Review

Review: PR 663, bound duties by the wall clock and advance the head for later epochs

I read the diff only. I did not build it or run the tests.

The change is well reasoned. Bounding by max(clock_epoch, head_epoch) + 1 matches the Beacon API. The advance targets are correct for MIN_SEED_LOOKAHEAD = 1:

• Attester duties advance to epoch - 1, which is at least head_epoch + 1, so process_slots always moves forward.
• Proposer duties advance to epoch - MIN_SEED_LOOKAHEAD, so the lookahead window covers the requested epoch.

dependent_root is still read from the head's real chain, and the new tests check the advanced committees and proposers against process_slots directly.

Findings

1. Unbounded advance while the node is syncing (validator.rs, epoch_upper_bound and state_for_epoch).

   • The upper bound follows the wall clock, and the head is not part of the cost limit. A node whose head is N epochs behind the clock now accepts clock_epoch + 1.
   • That makes state_for_epoch run process_slots across all N epochs, at seconds per epoch on a mainnet registry.
   • Before this PR the reach was capped at head+1, so a lagging node never did this.
   • This endpoint is normally trusted-local, but a validator client pointed at a syncing node will trigger it every slot, because the head root changes and the checkpoint cache key (epoch, head_root) misses.
   • Consider refusing with 503 or "node is syncing" when the head is more than about 1–2 epochs behind the clock. SyncStatusController is already available. Alternatively, cap the advance distance.

2. Concurrent misses are not deduplicated (validator.rs, the spawn_blocking calls in get_proposer_duties and post_attester_duties).

   • The doc comment says the epoch processing does not "repeat for a second request". That only holds after the first request finishes.
   • N simultaneous requests for the same uncached (epoch, head_root) each run the full process_slots on separate blocking threads.
   • Tokio's blocking pool allows up to 512 threads, and each of these holds a large state copy. Memory and CPU use could spike at an epoch boundary, when several VCs ask at once.
   • A per-key single-flight, or a semaphore around state_for_epoch, would fix it.

3. Error context is lost (validator.rs, state_for_epoch).

   • .map_err(|_| ApiError::Internal(...)) drops the underlying error.
   • Use inspect_err(|err| warn!(%err, ...)) before mapping, per the repo's error-handling idiom, so a failed advance can be diagnosed.

4. Minor: a spec-valid request becomes a 500 (proposer_duties).

   • The final ok_or(ApiError::Internal("epoch is outside the state's lookahead")) is unreachable if the earlier guards are right. That is fine as an invariant check.
   • It would be safer as a debug_assert! plus the internal error. A pre-fulu or other proposer_lookahead-less state is handled by the existing early return, so this is not a new problem.

5. Minor: epoch - 1 in the test (an_epoch_before_the_head_is_a_400) underflows if fulu_state ever sits at epoch 0. A saturating_sub or an assert on the fixture would make that explicit.

Verdict

The logic is correct and the test coverage is good. I would address Item 1 before merge, since it adds a new expensive path that a lagging node can hit repeatedly. Item 2 is worth fixing at the same time.

---

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

[github-actions]

🤖 Codex Code Review

Looks good overall. The change is thoughtful about Beacon API timing semantics, moves expensive process_slots work off the async executor, and adds good coverage around epoch-boundary behavior.

• No correctness bug jumped out in the new proposer/attester duty flow. The use of checkpoint_state(...) to derive an advanced state from the current head is aligned with the consensus assumptions for committee/proposer derivation in crates/net/rpc/src/beacon/validator.rs:180 and crates/net/rpc/src/beacon/validator.rs:389.
• Performance is improved in the right place: both duty endpoints now offload potentially heavy state advancement to spawn_blocking, which avoids stalling the async runtime under mainnet-sized registries in crates/net/rpc/src/beacon/validator.rs:249 and crates/net/rpc/src/beacon/validator.rs:362.
• Security-wise, I don’t see a new memory-safety risk or consensus-integrity issue in this diff. The code stays in safe Rust, preserves error-to-HTTP mapping, and does not relax any validation boundary.

One thing to tighten:

• Potential overflow in epoch-bound checks. epoch_upper_bound does head_epoch.max(clock_epoch) + 1 in crates/net/rpc/src/beacon/validator.rs:170, and attester lower-bound logic does epoch + 1 < head_epoch in crates/net/rpc/src/beacon/validator.rs:381. Since Epoch is u64, both can overflow in debug builds and wrap in release. That is unrealistic in practice, but this is consensus-adjacent infrastructure and worth making total with saturating_add(1) or equivalent checked logic.

If you want, I can turn that into a tiny follow-up patch suggestion.

---

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

[github-actions]

🤖 Kimi Code Review

I'll review this PR which extends validator duty endpoints to serve epochs beyond the head state's immediate lookahead, using process_slots on a blocking thread when necessary.

Overall Assessment

The PR is well-structured and addresses a real consensus-layer need (serving next-epoch duties at epoch boundaries). However, there are several correctness, safety, and maintainability issues.

---

Critical Issues

1. Incorrect checkpoint_state target for proposer duties (crates/net/rpc/src/beacon/validator.rs:265-268)

let state = if epoch > head_epoch + preset::MIN_SEED_LOOKAHEAD {
    state_for_epoch(store, head_root, epoch - preset::MIN_SEED_LOOKAHEAD)?
} else {
    head_state.clone()
};

The proposer lookahead in Fulu covers epoch through epoch + MIN_SEED_LOOKAHEAD. To read proposers for epoch, you need a state whose lookahead covers epoch, meaning state_epoch such that state_epoch + MIN_SEED_LOOKAHEAD >= epoch. The minimal such state_epoch is epoch - MIN_SEED_LOOKAHEAD. However, process_slots advances to the start slot of that epoch, and the lookahead is computed from the post-state of that slot. The checkpoint target uses epoch as the checkpoint epoch, but the root is head_root—this creates a checkpoint with the head's root but a future epoch, which is semantically incorrect for fork choice.

The checkpoint_state function likely expects (root, epoch) pairs that are actual justified/finalized checkpoints. Passing a synthetic checkpoint with head_root and a future epoch may:

• Miss the cache every time (since this checkpoint never appears in chain history)
• Or worse, collide with a real checkpoint if head_root later gets justified at that epoch

Fix: Use state_for_epoch with epoch - MIN_SEED_LOOKAHEAD but ensure the checkpoint is constructed correctly, or bypass checkpoint_state and call process_slots directly with proper caching.

2. state_for_epoch error mapping discards information (crates/net/rpc/src/beacon/validator.rs:248-251)

fn state_for_epoch(...) -> Result<Arc<BeaconState>, ApiError> {
    let target = Checkpoint { epoch, root: head_root };
    checkpoint_state(store, &target, &store.config())
        .map_err(|_| ApiError::Internal("advancing the head state failed"))
}

Mapping any error to a generic internal error loses debugging information. checkpoint_state can fail for multiple reasons (invalid root, DB error, state transition failure). At minimum, log the original error.

3. dependent_root uses wrong state for advanced cases (crates/net/rpc/src/beacon/validator.rs:308-309)

let dependent_root =
    block_root_at_or_before(&head_state, head_root, first_slot.saturating_sub(1))?;

For proposer duties when epoch > head_epoch + MIN_SEED_LOOKAHEAD, the dependent_root is computed against head_state (the unadvanced head), but the duties themselves come from state (the advanced one). Per the v1 API spec, dependent_root should be the block at slot compute_start_slot_at_epoch(epoch) - 1 on the same chain that produced the duties. Since the advanced state descends from head_root, this is correct for the chain, but if head_state and state diverge in historical blocks (they shouldn't, since it's just slot advancement), this could be inconsistent.

Actually, re-reading: this is correct because block_root_at_or_before walks back from head_root using the head state's block roots. The advanced state doesn't change history. This is fine.

Wait—no. block_root_at_or_before takes &state for its block_roots field access. For head_state, this accesses head_state.block_roots. For a state advanced 2+ epochs, the relevant block root might have rotated out of the array if SLOTS_PER_HISTORICAL_ROOT is exceeded. But first_slot.saturating_sub(1) is at most ~1 epoch back from the advanced state's slot, and the head is at most a few epochs behind, so this is safe in practice.

4. Attester duties state_for_epoch off-by-one in comment vs. code (crates/net/rpc/src/beacon/validator.rs:387-392)

Comment says "advanced to the start of the epoch before epoch", code does epoch - 1. This is correct because attester committees for epoch depend on the seed computed at the end of epoch - 1, which requires state through epoch - 1. But verify: MAX_SEED_LOOKAHEAD is 4, committees are fixed at start of epoch - 1. Advancing to epoch - 1 is correct.

---

Security & Consensus Issues

5. No validation that advanced state matches head chain (crates/net/rpc/src/beacon/validator.rs:248)

checkpoint_state(store, &target, &store.config())

If head_root is not in the canonical chain (reorg happened between head() call and checkpoint_state call), this could advance a stale state. The head() function returns the current fork choice head, but there's no atomicity guarantee across the two calls.

Mitigation: The window is small, but in async code with spawn_blocking, the head could change. The store is Clone/Send, so it's a snapshot, but the fork choice head could advance. If head_root is no longer the head when checkpoint_state runs, the cache lookup might fail or return stale data.

Actually, Store appears to be an Arc-like handle, so store.clone() shares state. The head could change. checkpoint_state with a non-head root might:

• Find it in cache if it was a recent head
• Or fail to find it and try to reconstruct, potentially hitting DB

This is a TOCTOU race: head() and checkpoint_state don't execute atomically. If a reorg occurs, head_root might not be available.

Fix: Accept that this can happen and return an appropriate error, or re-check the head after state_for_epoch and retry.

6. spawn_blocking with Store clone (crates/net/rpc/src/beacon/validator.rs:241-242)

let computed = tokio::task::spawn_blocking(move || proposer_duties(&store, &epoch)).await;

Store is cloned into the blocking task. If Store contains locks or channels, this could cause contention. More importantly, spawn_blocking without a dedicated pool or limit can exhaust threads under load. Each duty request spawns a new blocking task; with many validator clients, this could create unbounded threads.

Fix: Use a bounded thread pool or semaphore for CPU-intensive consensus operations.

---

Performance Issues

7. process_slots on every cache miss without pre-warming (crates/net/rpc/src/beacon/validator.rs:248)

The checkpoint_state cache only helps for repeated requests. At epoch boundaries, every validator client will request next-epoch duties simultaneously, causing thundering herd on process_slots. Mainnet state processing is seconds; with 1000s of validators, this serializes or duplicates work.

Suggestion: Pre-compute or share in-progress advancement tasks. Consider a tokio::sync::OnceCell or deduplication map for (head_root, target_epoch) -> JoinHandle<Arc<BeaconState>>.

8. head_state.clone() for non-advanced case (crates/net/rpc/src/beacon/validator.rs:267, 391)

BeaconState is large (MBs on mainnet). Cloning it per request is expensive. The original code already did this, but now it's more prominent with the branching.

Note: If head_state is already Arc<BeaconState>, clone() is cheap. Verify the type: head returns Arc<BeaconState>, so .clone() on Arc is cheap. This is fine.

Wait—head_state is destructured from head() which returns Arc<BeaconState>. Then head_state.clone() clones the Arc. Correct.

---

Rust Best Practices

9. duties_response could use Result::flatten or and_then (crates/net/rpc/src/beacon/validator.rs:254-262)

fn duties_response(
    computed: Result<Result<serde_json::Value, ApiError>, tokio::task::JoinError>,
) -> Response {
    match computed {
        Ok(Ok(body)) => crate::json_response(body),
        Ok(Err(err)) => err.into_response(),
        Err(_) => ApiError::Internal("computing the duties failed").into_response(),
    }
}

The Result<Result<...>, JoinError> type is awkward. Consider:

fn duties_response(
    computed: Result<Result<serde_json::Value, ApiError>, tokio::task::JoinError>,
) -> Response {
    match computed {
        Ok(inner) => match inner {
            Ok(body) => crate::json_response(body),
            Err(err) => err.into_response(),
        },
        Err(_) => ApiError::Internal("computing the duties failed").into_response(),
    }
}

Or use match computed.flatten() if ApiError: From<JoinError> or similar. Current code is acceptable but nested Ok(Ok(...)) is slightly unidiomatic.

10. epoch_upper_bound naming and semantics (crates/net/rpc/src/beacon/validator.rs:165-168)

fn epoch_upper_bound(store: &Store, head_epoch: Epoch) -> Epoch {
    let clock_epoch = compute_epoch_at_slot(crate::beacon::node::wall_slot(store));
    head_epoch.max(clock_epoch) + 1
}

The function returns the inclusive upper bound (duties served up to this epoch). Name suggests it's a bound for comparison; epoch > epoch_upper_bound is correct. But "upper bound" usually means exclusive. Consider max_servable_epoch or document that it's inclusive.

11. Missing Send bound check on spawn_blocking closure

The closure move || proposer_duties(&store, &epoch) captures &epoch which is a String. The closure must be FnOnce() -> T + Send + 'static. String is Send, Store presumably is. This is likely fine but verify Store: Send + 'static.

---

Testing Issues

12. Test boundary_store doesn't test proposer duties at exact boundary (crates/net/rpc/src/beacon/validator.rs:744-751)

fn boundary_store() -> (Store, BeaconState, Epoch) {
    let state = fulu_state();
    let head_epoch = compute_epoch_at_slot(state.slot());
    let clock_slot = compute_start_slot_at_epoch(head_epoch + 1);
    // ...
}

This sets clock to first slot of head_epoch + 1. The upper bound is max(head_epoch, head_epoch+1) + 1 = head_epoch + 2. Tests verify head_epoch + 2 works, head_epoch + 3 fails. Good.

But missing: what if clock is exactly at boundary and head is at head_epoch (last slot)? Then epoch_upper_bound = head_epoch + 1, so head_epoch + 2 should fail. Test this edge case.

13. beacon_store_at_clock panics on clock skew (crates/net/rpc/src/lib.rs:635-642)

let now_secs = std::time::SystemTime::now()
    .duration_since(std::time::UNIX_EPOCH)
    .expect("the clock is after the epoch")
    .as_secs();
let genesis = now_secs - clock_slot * slot_secs - 1;

If clock_slot * slot_secs > now_secs, genesis underflows (panic in debug, wrap in release). This is test-only code, but expect message is wrong ("the clock is after the epoch"—should be "clock is after Unix epoch"). Also, if clock_slot is large, this panics.

Fix: Use saturating_sub or assert with clear message.

---

Documentation

14. Comment typo (crates/net/rpc/src/beacon/validator.rs:236)

/// Read from the `proposer_lookahead` fulu introduced and gloas keeps, which the state keeps for its own

"fulu introduced and gloas keeps" is garbled. Should be "Fulu introduced and Gloas keeps" or rephrase.

---

Summary Table

Item	Severity	File	Line	Issue
1	High	validator.rs	265-268	Synthetic checkpoint with future epoch may break checkpoint_state cache semantics
5	High	validator.rs	241-242, 248	TOCTOU race between head() and checkpoint_state
6	Medium	validator.rs	241-242	Unbounded spawn_blocking tasks
7	Medium	validator.rs	248	Thundering herd on process_slots at epoch boundaries
2	Medium	validator.rs	248-251	Error information loss
13	Low	lib.rs	635-642	Test helper panics on bad input
14	Trivial	validator.rs	236	Comment typo

The core logic for epoch bounds and state advancement is correct, but the interaction with checkpoint_state cache semantics and potential races need careful attention.

---

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