feat(rpc): serve validator/liveness - #643
pablodeymo wants to merge 1 commit into
Conversation
🤖 Codex Code ReviewLooks good overall. The change is narrowly scoped, thread-safe, and doesn’t touch fork choice, state transition, justification/finalization, XMSS, or SSZ logic.
Minor note:
I don’t see correctness, security, or performance regressions in this PR. Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Claude Code ReviewReview of PR 643: Beacon API
|
🤖 Kimi Code ReviewI'll review this PR which implements the
|
| Severity | Count | Description |
|---|---|---|
| Critical | 0 | None found |
| High | 0 | None found |
| Medium | 0 | None found |
| Low/Info | 2 | Minor clarity improvements possible |
The PR is well-implemented and correct. The liveness tracking properly covers all paths (gossip aggregates, gossip attestations, block proposers, API submissions) with appropriate epoch windowing and memory bounds. The endpoint correctly combines on-chain participation with off-chain observation for doppelganger protection.
Optional suggestions:
-
File:
crates/storage/src/liveness.rs, line 78: Add explicitvalidator < MAX_TRACKED_INDEXcheck inis_livefor symmetry withrecord_all, or document the implicitfalsereturn. -
File:
crates/net/rpc/src/beacon/validator.rs, line 178: Considerepoch.saturating_add(1) < currentfor absolute safety, though not practically needed.
Automated review by Kimi (Moonshot AI) · kimi-k2.6 · custom prompt
…the endpoint a validator client's doppelganger protection calls before signing. A validator is live in an epoch if the head state credits it for that epoch (a non-zero participation byte, covering what blocks have already included) or if this node observed it act in that epoch, which covers what no block has included yet. The observations live in a new ObservedLiveness on the Store, one bitset per epoch for the newest three, because three places write them and all three already hold a Store clone: P2P when it accepts a gossip aggregate (its aggregator and every attester its signature verified) or subnet attestation, the chain actor when it imports a beacon block (its proposer, so range-synced and self-published blocks count), and the RPC for attestations and aggregates submitted through pool/attestations and aggregate_and_proofs, since gossip never delivers a node its own messages. The endpoint answers the store clock's previous, current and next epoch and is a 400 for any other epoch or an unknown index, and a 503 while syncing.
0d9c9e1 to
c41deeb
Compare
MegaRedHand
left a comment
MegaRedHand
left a comment
There was a problem hiding this comment.
Comments below. The two I'd look at first are the proposer write (only on a full import) and attestations included in blocks off the canonical chain; the rest are small.
| // API's liveness endpoint. Recorded here rather than where | ||
| // gossip accepts a block, so a block that arrived by range | ||
| // sync or through this node's own API counts too. | ||
| if self.store.chain() == Chain::Beacon { |
There was a problem hiding this comment.
Medium: the proposer is recorded only when the import ends in Imported. A gossip block whose proposer signature already verified (Accept), but that is then held for columns or the engine, or fails the STF, marks nobody, and record_liveness in verdict.rs skips Validated::Block explicitly. For doppelganger purposes a verified proposer signature is already evidence the key is in use; Lighthouse records observed_block_producers at gossip verification. Recording on Validated::Block Accept as well, and keeping this write for range-synced and API-published blocks, would cover both.
Also, this write has no test (the PR notes it). Moving it into a small helper with a unit test would keep a later refactor of this arm from dropping it unnoticed.
| let state_epoch = compute_epoch_at_slot(state.slot()); | ||
| // The head state's flags for `epoch`, if it keeps them: its own epoch's | ||
| // and the one before. `None` before altair, which keeps no flags. | ||
| let participation = state |
There was a problem hiding this comment.
Minor: included attestations only count through the canonical head's participation flags. Votes packed in an imported block on another fork, or included without earning a flag (wrong target, late source), are missed unless this node also saw them on a subnet it joined. process_block already resolves attesting_indices for every attestation of every imported block (fork_choice::on_block_attestation); recording them into ObservedLiveness there, once per block with record_all, would cover all imported blocks. Lighthouse does the same through observed_block_attesters, which its liveness check reads.
| for index in indices { | ||
| state | ||
| .validator(index) | ||
| .map_err(|_| ApiError::BadRequest("unknown validator index"))?; |
There was a problem hiding this comment.
Minor: one index outside the head's registry turns the whole batch into a 400. When Lighthouse's doppelganger service gets an error from the liveness call, it carries on with an empty response (doppelganger_service/src/lib.rs, beacon_node_liveness), so no validator in that request makes progress, and with this node as its only BN they all stay muted while the bad index is in the set. Lighthouse's BN answers false for any index. I see the PR explains choosing 400; flagging the batch-wide effect.
| /// the current epoch. See [`ethlambda_storage::ObservedLiveness`]. | ||
| /// | ||
| /// Answered for the store clock's previous, current and next epoch; the next | ||
| /// one is always `false`, and is accepted because a doppelganger check made |
There was a problem hiding this comment.
Nit: "the next one is always false" isn't guaranteed. Writers record by the message's target epoch using the wall clock, while this window follows the store clock, which only moves on the chain actor's tick. If the store clock lags at a boundary, the store's next epoch can already hold observations and answer true. That answer is correct, so only this comment and docs/rpc.md need adjusting.
| .map_err(|_| ApiError::BadRequest("invalid validator index"))?; | ||
|
|
||
| let current = get_current_store_epoch(store, &store.config()); | ||
| if epoch + 1 < current || epoch > current + 1 { |
There was a problem hiding this comment.
Nit: epoch + 1 overflows for epoch = u64::MAX (a panic under overflow checks; it wraps in release). epoch.saturating_add(1) < current avoids it. The bot review raised this too.
| .copied() | ||
| .unwrap_or(epoch) | ||
| .max(epoch); | ||
| let floor = newest.saturating_sub(RETAINED_EPOCHS - 1); |
There was a problem hiding this comment.
Nit (defensive): the floor follows the newest epoch ever recorded rather than a clock, so a single record for a far-future epoch would prune every epoch the endpoint can still be asked about. Today's writers all pass gossip or API time checks, so this isn't reachable, but refusing an epoch more than one past the current newest (or passing the clock in) would make it hold by construction.
| } | ||
| bits[word] |= 1 << (validator % 64); | ||
| } | ||
| epochs.retain(|&kept, _| kept >= floor); |
There was a problem hiding this comment.
Nit: retain (and the newest lookup) runs on every call, but the set of epochs only changes when a new key is inserted. Pruning only on the Entry::Vacant path would take it off the per-attestation path.
| /// | ||
| /// Every writer records an index that passed validation against a state, so | ||
| /// this is a guard rather than a limit anything should reach: it bounds one | ||
| /// epoch's bitset at 2 MiB whatever an index turns out to be. Mainnet has |
There was a problem hiding this comment.
Nit: these comments spell out what the constants evaluate to ("2 MiB" here, "the newest epoch and the two before it" above, and "the newest three epochs" in docs/rpc.md). If RETAINED_EPOCHS or MAX_TRACKED_INDEX changes, they go stale; referring to the constants by name avoids that.
| .and_then(|attesting_indices| { | ||
| // The aggregator and every attester its signature verified are | ||
| // live, as when P2P accepts a gossip aggregate. | ||
| let (epoch, _root) = aggregate.target(); |
There was a problem hiding this comment.
Nit: this is the same "an aggregate marks its aggregator and its attesters" rule as record_liveness in verdict.rs. A helper on ObservedLiveness, something like record_aggregate(aggregate, attesting_indices), would keep the two writers from drifting.
| /// validators it names as live for its target epoch; see | ||
| /// [`record_liveness`]. | ||
| fn forward(self, server: &P2PServer, received_at: Instant, outcome: Outcome) { | ||
| if outcome == Outcome::Accept { |
There was a problem hiding this comment.
Nit: forward now checks outcome == Outcome::Accept in three separate places (here, aggregate pooling, aggregator-subnet pooling). One if outcome == Outcome::Accept { ... } block holding all three side effects would keep the Accept-only rule in one place.
This branch has not been deployed
2 participants
🗒️ Description / Motivation
POST /eth/v1/validator/liveness/{epoch}is what a validator client's doppelganger protection calls before it signs anything: "did anyone see these validators act in epoch E?" If the node answers 404:This is the last of the four missing endpoints; the other three are in #642, which this PR is stacked on.
The Beacon API leaves the answer to the node's own view ("network, chain or API"), and clients differ:
is_liveWhat Changed
crates/storage/src/liveness.rs(new):ObservedLiveness, one bitset per epoch for the newest three.1 << 24, so one epoch is at most 2 MiB. Every writer passes a validated index anyway; mainnet is ~2.4M validators, about 300 KB.crates/storage/src/store.rs: anobserved_livenessfield and accessor, shared by everyStoreclone likecommittee_cache, because the set has three writers and a reader that all already hold aStore.crates/net/p2p/src/beacon/verdict.rs: onAccept, an aggregate marks its aggregator and every attester its signature verified; a subnet attestation marks its attester.crates/blockchain/src/lib.rs: a successfully imported beacon block marks its proposer. This is done at import, not at gossip acceptance, so range-synced blocks and blocks published through this node's API count too.crates/net/rpc/src/beacon/pool.rs: submissions throughpool/attestationsandaggregate_and_proofs, since gossip never delivers a node its own messages. The aggregate path now keeps the attesting indices thatstateful_checksalready returned.crates/net/rpc/src/beacon/validator.rs:post_liveness/liveness.docs/rpc.md: the route and the liveness semantics.Correctness / Behavior Guarantees
Accepted gossip and validated API submissions are recorded; anIgnore,RejectorOverloadedobject marks nobody.current_epoch_participation) and the one before (previous_epoch_participation). Before altair there are no flags, so only observations count.false, because a doppelganger check at an epoch boundary can land on it; Lighthouse accepts it too.false). An index outside the registry is a client bug, better reported than hidden.Mutexonce per accepted gossip object, to set bits. That includes every accepted attestation on the backbone subnets, which don't touch the attestation pool: roughly a couple of thousand short locks a slot on mainnet, against a reader that only runs when a validator client asks.Tests Added / Run
ObservedLiveness:a_recorded_validator_is_live_in_that_epoch_onlyrecord_all_sets_every_indexepochs_older_than_the_window_are_prunedan_epoch_below_the_window_is_not_recordedan_index_past_the_guard_is_ignoredobserved_liveness_is_shared_across_store_clonesa_participation_flag_makes_a_validator_live(current and previous epoch flags)an_observed_validator_is_live_without_a_flagthe_next_epoch_is_answered_and_nobody_is_live_in_itepochs_outside_the_window_are_a_400an_unknown_validator_is_a_400a_syncing_node_answers_503an_accepted_aggregate_marks_its_aggregator_and_attesters_livean_accepted_subnet_attestation_marks_its_attester_livean_object_that_was_not_accepted_marks_nobody_livea_published_attestation_marks_its_attester_livea_refused_attestation_marks_nobody_livea_published_aggregate_marks_its_aggregator_and_attesters_live: the aggregate is built on one node and submitted to a fresh one, so only the aggregate itself can have marked its attesters.--enable-doppelganger-protection. Its validators should come online after the doppelganger wait instead of staying muted.Related Issues / PRs
fork,deposit_contract,duties/sync); retarget tobeacon-chain-integrationonce feat(rpc): serve states/{id}/fork, config/deposit_contract and validator/duties/sync #642 merges.✅ Verification Checklist
make fmt— cleanmake lint(clippy with-D warnings) — cleanmake test(test-consensusplustest-node, atrelease-fast) — all passing (1966 passed, 0 failed, 26 ignored)