fix(rpc): bound sync duties by the wall clock

[MegaRedHand]

Motivation

POST /eth/v1/validator/duties/sync/{epoch} answered only the head state's sync-committee period and the next one. At a period boundary whose first block is late or missing, the head is still in period P-1 while the wall clock is in P, so a validator client asking for period P+1 (the next one, which beacon-APIs allows) got a 400. Same bug class as #663 (duties) and #670 (liveness): the bound must follow the wall-clock current epoch, not the head.

Change

• The period is bounded by max(wall-clock period, head period) + 1.
• A period past the head's next one is read from the head advanced (fork choice's checkpoint_state, on a blocking thread) to the first epoch of the period before it, whose next_sync_committee answers it.
• Kept: 503 while syncing, 400 for unknown indices and for periods before the head's (the remaining deviation, rewritten in docs/spec_deviations.md).
• Tests: head in the last epoch of period 0 with the clock at the first slot of period 1: period 2 is served and equals the advanced state's committee (asserted to differ from the head's next committee), period 3 is a 400, period 1 is served from the head. the_period_after_next_is_a_400 asserted the old head-relative bound and now pins the clock to the head's period.

cargo test -p ethlambda-rpc --lib --profile release-fast, clippy and fmt clean.

[github-actions]

🤖 Kimi Code Review

I'll review this PR which extends sync committee duty serving to handle cases where the head lags behind the wall clock across a period boundary.

Overall Assessment

The PR correctly implements Beacon API semantics for sync duties: bounding requests by the wall clock rather than the head state, with state advancement via process_slots when needed. The core logic is sound, but there are several issues to address.

---

Issues Found

1. Critical: spawn_blocking closure captures by reference incorrectly (Line 106)

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

Problem: The move closure moves store, epoch, and indices into the blocking thread, but sync_duties takes &Store, &str, and &[String] — references to values owned by the closure. This compiles but the references outlive the closure body, which is unsound. However, since spawn_blocking runs the closure to completion before returning, this is technically safe in practice. More critically: store is an Arc<Store> or similar? Let me check — head(store?) suggests Store is passed by reference. If Store is not Clone or Send, this fails.

Actually, re-reading: store comes from Extension(store) in the handler. The type is likely Store (not Arc<Store>). The move moves store into the closure, but sync_duties only needs &Store. This is fine for spawn_blocking since the closure owns store and passes a reference to it.

However: If Store is large or not Send, this could be problematic. The spawn_blocking requires Send. Assuming Store: Send, this is okay.

2. Bug: state_for_epoch uses wrong checkpoint root (Lines 119-128)

fn state_for_epoch(
    store: &Store,
    head_root: H256,
    epoch: Epoch,
) -> Result<Arc<BeaconState>, ApiError> {
    let target = Checkpoint {
        epoch,
        root: head_root,  // ← BUG: wrong root for the target epoch
    };
    checkpoint_state(store, &target, &store.config())

Problem: The checkpoint's root should be the block root at the target epoch, not the head_root. Using head_root for a future epoch creates an invalid checkpoint — the root doesn't match the epoch. The checkpoint_state function likely expects a valid checkpoint (epoch, root) pair where the root is the block root at that epoch.

This will likely cause checkpoint_state to fail or return wrong state, since the cache lookup uses (epoch, root) as key and the root doesn't correspond to that epoch.

Fix: You need to get the block root at the target epoch first, or use a different API. If checkpoint_state is meant to advance the head state to a future slot, it should probably take (head_root, target_slot) separately, or compute the correct root.

Wait — let me re-read the comment: "The head's post-state advanced through empty slots to the first slot of epoch, from fork choice's checkpoint-state cache." This suggests checkpoint_state is a custom function that advances state, not a standard spec function.

Looking at the checkpoint structure: Checkpoint { epoch, root: head_root } — this is indeed odd. If checkpoint_state uses this to key its cache, the cache key is wrong for the target epoch. If it's just using head_root to find the base state and epoch as target, it might work internally.

Need clarification: What does checkpoint_state do with the root field? If it's used as cache key for the result, then concurrent requests for the same (head_root, target_epoch) would hit cache, which is correct. But if it's used to validate the checkpoint, this is wrong.

3. Performance: Blocking thread for potentially long process_slots (Line 106)

The comment says "seconds on a mainnet registry" for process_slots. spawn_blocking is appropriate, but the default blocking pool might be exhausted if many concurrent sync duty requests arrive.

Suggestion: Consider adding a semaphore or dedicated pool if this becomes a bottleneck. Not critical for now.

4. Error handling: map_err discards error details (Lines 126-127, 152)

.map_err(|_| ApiError::Internal("advancing the head state failed"))

Problem: The original error from checkpoint_state is lost. For debugging, at least log the original error:

.map_err(|e| {
    tracing::error!(?e, "checkpoint_state failed");
    ApiError::Internal("advancing the head state failed")
})

Similarly for other map_err(|_| ...) patterns.

5. Logic issue: Upper bound check allows clock_period + 1 but comment says "period after the current one" (Lines 148-152)

if requested_period < head_period || requested_period > head_period.max(clock_period) + 1 {

The condition > head_period.max(clock_period) + 1 allows clock_period + 1 when clock_period >= head_period. This matches the spec: "current period plus one". ✓

But when head_period > clock_period (head ahead of clock), it allows head_period + 1. This seems correct too — if head is ahead, we serve based on head.

However, the lower bound requested_period < head_period rejects periods before the head even if the clock is behind. This matches the documented deviation.

6. Test helper: Potential race condition in beacon_store_at_clock (Lines 470-476)

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;

Problem: Uses real system time in tests. The - 1 second offset means "slot began a second ago". If test execution is slow or system time jumps, genesis could be in the future, causing Store::init_beacon to fail or behave unexpectedly.

Suggestion: Use a deterministic time or mock clock for tests. At minimum, document this fragility.

7. Missing test: Head ahead of clock (Tests)

The tests cover:

• Head lagging clock (lagging_head)
• Period after next is 400
• Two periods past clock is 400
• Clock's own period from head

Missing: Head ahead of clock (e.g., head at period 2, clock at period 1). Should still serve head's period and next from head state without advancement.

8. Documentation inconsistency (docs/rpc.md, docs/spec_deviations.md)

The docs say "up to the wall clock's current period plus one" but the code uses head_period.max(clock_period) + 1. When head is ahead, this allows head_period + 1, which is correct but subtly different from "wall clock's current period plus one".

---

Minor Issues

9. Code style: first_epoch calculation (Line 164)

let first_epoch = (requested_period - 1) * preset::EPOCHS_PER_SYNC_COMMITTEE_PERIOD;

This computes the first epoch of the previous period, not the requested period. The comment says "first epoch of the period before it" — this is correct for getting next_sync_committee (which becomes current in the next period). But naming is confusing: first_epoch is actually first epoch of requested_period - 1.

Suggestion: Rename to advance_to_epoch or add clearer comment.

10. Clone in test (Line 1087)

let mut advanced = state.clone();

Cloning a full BeaconState is expensive. In tests this is fine, but for production the state_for_epoch returns Arc<BeaconState> — good.

---

Positive Notes

1. Correct use of spawn_blocking for CPU-intensive state transition
2. Good test coverage for the lagging head scenario
3. Proper Arc<BeaconState> return avoids unnecessary cloning in production
4. Clear documentation updates in both rpc.md and spec_deviations.md

---

Summary

Priority	Issue	Location
High	Verify checkpoint_state behavior with wrong root in Checkpoint	validator.rs:119-128
Medium	Add error logging before discarding errors	validator.rs:126-127, 152
Medium	Test fragility from real system time	lib.rs:470-476
Low	Add test for head ahead of clock	validator.rs tests
Low	Rename first_epoch for clarity	validator.rs:164

---

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

[github-actions]

🤖 Claude Code Review

Review: PR 671, sync duties bounded by the wall clock

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

The logic is sound. The period arithmetic checks out:

• Head's own period: for requested_period == head_period, state_for_epoch is skipped and state_period == head_period, so current is used.
• Next period: for head_period + 1, the head state is used and next is returned.
• Later period: for requested_period >= head_period + 2, the head is advanced to epoch (requested_period - 1) * EPOCHS_PER_SYNC_COMMITTEE_PERIOD. That state sits in period requested_period - 1, so its next_sync_committee is the requested one. The target epoch is always after the head's epoch, so process_slots never has to go backwards.
• Bound: head_period.max(clock_period) + 1 handles a head that is ahead of the clock.

The tests cover the interesting cases: the rotation-proof assertion on the committee, the clock's own period, and two periods past the clock.

Concerns

1. Unbounded blocking work per request (validator.rs, post_sync_duties).

   • The handler now calls spawn_blocking on every request. A cache miss in state_for_epoch runs process_slots for what the doc comment says is seconds on mainnet.
   • Concurrent requests for the same uncached checkpoint will each recompute it, so there is no single-flight. A validator client retrying, or several of them asking at a period boundary, can fill tokio's blocking pool (default 512 threads) with multi-second jobs. That starves other spawn_blocking users such as gossip validation.
   • Consider a semaphore or single-flight around the advance path, or confirm that checkpoint_state already dedups concurrent misses.
   • Also check how far behind the clock the head can be. requested_period is only bounded by clock_period + 1, so a head lagging several periods gives process_slots a multi-period advance. The Syncing gate probably prevents this, but that is worth confirming.

2. Errors are swallowed.

   • state_for_epoch uses map_err(|_| ...), and the Err(_) arm on the JoinError does the same. A panic or a failed advance becomes a bare 500 with nothing logged.
   • Per the repo's inspect_err idiom, add a warn!/error! with the underlying error, at least for the join error, since a panic there would otherwise be invisible.

3. Clock read twice, in two places. wall_slot(store) is read once inside sync_duties, while head(store) is read separately. The two can straddle a slot or period boundary. That is harmless here, since the check is conservative, but it is a small inconsistency.

Nits

• Test helper: beacon_store_at_clock computes now_secs - clock_slot * slot_secs - 1. This is fine in practice, but use saturating_sub or an expect so a large clock_slot fails with a clear message instead of an underflow panic. The -1 also leaves the test sensitive to a slot boundary if slot_secs is ever 1.
• docs/rpc.md: the edited paragraph has one overlong line (historical state; see ... A validator is matched by pubkey and). Rewrap it.
• docs/spec_deviations.md: the bullets below the edited heading (beyond this diff) still describe the old "current and next period" limitation. Check that they match the new behaviour.

Overall this is a good change. I would address Point 1 (blocking-pool exposure) and Point 2 (error logging) before merging.

---

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

[github-actions]

🤖 Codex Code Review

Found two correctness risks; the rest looks reasonable and the boundary tests are good.

• crates/net/rpc/src/beacon/validator.rs:153 — The upper-bound check uses head_period.max(clock_period) + 1, but the docstring and Beacon API semantics you describe are “wall clock current period plus one.” If the head is ahead of the wall clock, this now accepts requests beyond the API bound. Example: head_period = 10, clock_period = 8 allows period 11, but the allowed max should be 9. This is a consensus-adjacent correctness bug in duty serving; the bound should key off the wall clock only for the upper end, while still rejecting periods before the head.

• crates/net/rpc/src/lib.rs:477 — let genesis = now_secs - clock_slot * slot_secs - 1; can underflow in tests when clock_slot * slot_secs + 1 > now_secs (or in any future refactor that passes a large synthetic slot). In debug this panics; in release it wraps. Since this helper exists specifically for tests, that makes failures flaky and non-obvious. Use checked/saturating arithmetic or assert the precondition explicitly.

• crates/net/rpc/src/beacon/validator.rs:106-110 — Minor error-handling nit: collapsing all spawn_blocking join errors to a generic 500 loses whether the task panicked or was cancelled. Not a blocker, but logging the JoinError would help diagnose production failures in this hot path.

• crates/net/rpc/src/beacon/validator.rs:163-167 — The “advance from head across boundary” approach looks sound for sync-committee lookup, and using checkpoint_state avoids mutating fork-choice state directly. I don’t see a fork-choice / justification / SSZ / signature-safety regression in the diff itself.

---

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