Repository navigation
fix(rpc): bound duties by the wall clock, advance the head for later epochs - #663
Conversation
🤖 Claude Code ReviewReview: PR 663, bound duties by the wall clock and advance the head for later epochsI read the diff only. I did not build it or run the tests. The change is well reasoned. Bounding by
Findings
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 |
🤖 Codex Code ReviewLooks good overall. The change is thoughtful about Beacon API timing semantics, moves expensive
One thing to tighten:
If you want, I can turn that into a tiny follow-up patch suggestion. Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Kimi Code ReviewI'll review this PR which extends validator duty endpoints to serve epochs beyond the head state's immediate lookahead, using Overall AssessmentThe 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 Issues1. Incorrect
|
| 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
…-64-633-636-638-gloas-live
…653 line #663 landed on tmp while #660 was being reverted here, and was written against #660's v2 helpers. The merge keeps #663's wall-clock bound, head advance and blocking thread, with #653's DependentRoot in place of #660's last_slot_before helpers and #653's 503-while-syncing v2 handler. The lookahead paragraph moves from the v1 handler's doc to proposer_duties, as #663 has it. The tests' get helper now layers a default SyncStatusController, as the production router always does: #663's v2 assertions went through routes() without it, which #653's v2 handler answers with a 500.
…tmp/bci-626-63-64-633-636-638-gloas-live
…epochs The attester duties endpoint refused any epoch more than one past the head state's. The Beacon API bounds it by the current epoch instead, and at the first slot of an epoch the head is still in the previous one until that slot's block arrives, so every validator client's next-epoch lookahead got a 400 at each epoch boundary (and for as long as the boundary slot stayed empty). Duties now accept any epoch up to one past the later of the wall clock's epoch and the head's. An epoch the head state cannot derive as it is (committees need a state at epoch - MIN_SEED_LOOKAHEAD) is read from the head advanced through fork choice's checkpoint-state cache, on a blocking thread, so the epoch processing neither stalls the runtime nor repeats for a second request. dependent_root still comes from the head's own chain. Proposer duties had the same head-relative limit on their lookahead window and get the same treatment. PTC duties already bounded by the clock and advance the head, so they are unchanged.
PTC duties bounded the epoch by the store's tick-driven clock, which still reads the previous epoch until the boundary slot's tick runs. A validator client asking for the next epoch's duties in the first milliseconds of an epoch (lighthouse does) got a 400. Share epoch_upper_bound with attester and proposer duties so all three bound by the wall clock.
2733334 to
a40d018
Compare
pablodeymo
left a comment
pablodeymo
left a comment
There was a problem hiding this comment.
Nice fix — bounding by the wall clock is what the Beacon API asks for, and it explains the 400s at every epoch boundary on the devnet. Checked:
- Upper bound
max(wall_epoch, head_epoch) + 1covers both a lagging head and a lagging clock. - Attester duties for
Efrom the head advanced to the start ofE - 1is sufficient (seed fromE - 2's mix, active set fixed an epoch ahead), and the test compares against the spec helpers on aprocess_slots-advanced state. - Proposer duties read from
proposer_lookaheadafter advancing toE - MIN_SEED_LOOKAHEAD, so it stays fulu+ only. dependent_rootstill comes from the head's chain, so the VC refetches once the boundary block lands.
One thing I'd like changed before merging: cap how far a duties request can advance the head.
With the head L epochs behind the clock (syncing or stalled node), a request for wall_epoch + 1 runs process_slots across L epoch transitions. At the ~0.7–0.9 s per mainnet epoch transition from #632/#633, a head 100 epochs behind is over a minute of CPU per request. The checkpoint_state cache doesn't help much while syncing, since the head root (part of the key) keeps moving, and any API caller can trigger it, not only our VC. The old head-relative bound made this impossible.
Suggestion: answer 503 when epoch > head_epoch + 2 — the node can't give correct duties that far from its head anyway, and it's the same "syncing" signal sync_duties and liveness already return. Note that gating on SyncStatusController alone wouldn't catch this, since it reports synced during a beacon catch-up (#636's "still open" #1).
if epoch > head_epoch + 2 {
return Err(ApiError::ServiceUnavailable("the head is too far behind the clock to serve duties"));
}Nit: in proposer_duties, the "outside the lookahead" case went from a 400 to an Internal 500. Unreachable with the new bounds, but if it ever fires a 500 is the wrong signal.
Approving once the cap is in.
|
Let's fix in another PR |
Main gained the gloas stack (#634, #635, #639, #662, #663). Adapted to it: - require_validated and the syncing endpoint's head check read optimism through block_is_optimistic, which knows a gloas block's payload link. - payload_attestation_data follows attestation data's rule: a 503 without an execution client, or when the voted block's payload is unvalidated. - The gloas validator-client tests serve through an idle execution client, and the optimistic-head test names the fork attestation_data now takes.
Main gained the gloas stack (#634, #635, #639, #662, #663). Adapted to it: - Gloas block production (produceBlockV4) appends the client versions to the graffiti the way fulu's does. - The validator client's gloas proposal and duties tests take their graffiti and fee recipient from ProposerSettings, and a validator's own graffiti is checked to reach a gloas block request.
Main gained the gloas stack (#634, #635, #639, #662, #663). Adapted to it: - The BLS fixture tests follow main's cryptography-specs layout, and main's Makefile (gossip vectors in the pinned tree) replaces the separate gossip tree this branch extended. - Operations under a gloas head are IGNOREd and a gloas-shaped attester slashing has no consumer; gloas operation vectors are not run. - Published columns and operations go out under the slot's own digest, and column subnets use compute_subnet_for_data_column_sidecar. - Gloas block production packs from the store's attestation pool, and a published gloas block gossips no sidecars (its columns follow the envelope). The payload attestation pool stays a shared handle.
Main gained the gloas stack (#634, #635, #639, #662, #663). Adapted to it: the write cursor also drives gloas's progressive registry. A cursor pass crosses a ProgressiveList's subtrees with global indices, and try_update_validators_with_balances and try_update_balances work on any BeaconState, so electra's effective-balance step and the reward deltas write through them too.
Main gained the gloas stack (#634, #635, #639, #662, #663). Adapted to it: gloas block processing (its attestations and sync aggregate) reads the total active balance from the same cache, which gloas block production passes in, and the cache's registry scan walks iter_validators so it covers gloas's progressive registry.
Main gained the gloas stack (#634, #635, #639, #662, #663). Adapted to it: gloas's progressive inactivity scores and ring buffers go through the same element accessors (inactivity_score_mut, InactivityScoresRef), and ProgressiveList is Buffered. Main's new tests write block_roots by index rather than through iter_mut, which a tree-backed vector has not got.
Main gained the gloas stack (#634, #635, #639, #662, #663). Adapted to it: - The key-layout bump is DB_VERSION 7, since main spends 6 on #634's gloas config keys. - A gloas fork-choice test stores a state whose header slot is its block's, since states are keyed by that slot, and the slot-peek test covers a gloas block, now the newest fork.
Main gained the gloas stack (#634, #635, #639, #662, #663). Adapted to it: gloas aggregates are checked under the schedule's attester domain too (is_valid_indexed_attestation_with_domain), main's attestation_for test helper signs under the schedule's domain, and the test store helpers are main's beacon_store_with_config.
## Motivation `GET /eth/v2/beacon/pool/attester_slashings` took its `version` and `Eth-Consensus-Version` from the head slot's fork. Right after a fork boundary whose first block has not arrived, the head is still on the old fork while beacon-APIs means the current (wall-clock) epoch's fork. Found while auditing for the head-versus-wall-clock bug class (#663, #670). ## Change - `version` and `Eth-Consensus-Version` come from `fork_at_epoch` of the wall clock's epoch. - No container conversion is needed: the pool holds electra-shaped slashings and their JSON is the same in every fork from electra on (only the SSZ bound on attesting indices differs; gloas adds no container), noted in the doc comment. - The other operation-pool GETs (`proposer_slashings`, `voluntary_exits`, `bls_to_execution_changes`) are not versioned by the head, so nothing else changes. - Test: head before a fork boundary, clock after it: the version is the clock's fork, not the head's. `cargo test -p ethlambda-rpc --lib --profile release-fast`, clippy and fmt clean.
|
Here's how the other clients handle the two points in the review. 1. How far a duties request can advance the head
2. Proposer epoch outside the lookaheadLighthouse, Prysm, Lodestar and Grandine answer 500. Teku answers 400; I inferred that from its error mapping and didn't trace the path. Nimbus has no such path. Why a head-relative cap is a problemA rule like The chain restarts only if a validator on another client proposes. On a network where every node applies this rule, it never restarts. Nimbus behaves exactly like this and says so in its source ( # TODO if everyone follows this logic, the network will not recover from a
# halt: nobody will be producing blocks because everone expects someone
# else to do itLodestar (after 1 epoch) and Grandine (after 32 slots, on its gated routes) also stop serving duties in a halt. Lighthouse, Prysm and Teku decide "syncing" from their peers rather than from the clock alone. They keep serving, so they are the clients that would restart the chain. |
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
max(wall-clock epoch, head epoch) + 1. The wall clock isnode::wall_slot, the clockattestation_dataalready uses; themaxkeeps a head ahead of a lagging clock from being refused, as PTC duties already do.checkpoint_statecache (the start ofepoch - 1for attester duties,epoch - MIN_SEED_LOOKAHEADfor proposer duties), so a repeated request does no epoch processing.dependent_rootis still read from the head's chain.spawn_blocking, like PTC duties.attestation_dataalready bound by the clock; nothing else bounds by the head's epoch.Tests
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.attester_duties_two_epochs_ahead_are_a_400becameattester_duties_past_the_clocks_next_epoch_are_a_400, andan_epoch_outside_the_lookahead_is_a_400becamean_epoch_past_the_clocks_next_is_a_400.cargo test --profile release-fast --lib: rpc 175, validator 269; fmt and clippy-D warningsclean.