Repository navigation
feat(rpc): serve validator/liveness - #643
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
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.
c41deeb to
d973505
Compare
…64-633-636-638-gloas-live #653 sits on the squashed #643, whose liveness change tmp already holds from the pre-squash branches, adapted to gloas and the shared attestation pool. Every conflict in the files only #643 touches (verdict.rs, pool.rs, store.rs) therefore keeps tmp's side. validator.rs keeps tmp's imports and takes #653's v2 proposer duties handler; proposer_duties still reads a gloas head's lookahead (#662). The new validators/{validator_id} handler reads the registry through tmp's iter_validators and balance accessors (validators() and balances() are gone here), and reports execution_optimistic through block_is_optimistic, as its siblings in states.rs do on tmp. docs/rpc.md keeps tmp's endpoint rows and both #653's v2 dependent root and #662's gloas sentence.
d973505 to
54adf35
Compare
…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.
54adf35 to
8b3cbc5
Compare
Squash of feat/beacon-gloas-live at d1ef2fe (merge-base d91c6e5). Adapted onto #626-#638 and main (#642-#677): - 13 tables (#628's BeaconUnrealizedJustifications beside the gloas two). - #626's event bus, test stand-ins and BlockAnnouncement carry the gloas sidecar and the two gloas chain-actor messages. - The Beacon API's optimism readers (syncing, fork, validator-by-id, sync duties, require_validated) go through block_is_optimistic. - #643's liveness recorder ignores envelopes and payload attestations.
…ient (#668) Squash of feat/beacon-sync-committee at 07f0357 (merge-base 55adf1b). Adapted onto #643-#667: gloas block production packs an empty sync aggregate, #643's liveness recorder ignores sync committee messages, and #638's per-validator settings reach the sync duties. The #642 commit this branch carries is main's already.
…r duties, for Lighthouse's validator client (lambdaclass#653) ## 🗒️ Description / Motivation Lighthouse's validator client (v8.2.3) can't run on ethlambda beacon, because it needs two endpoints this node doesn't serve: | Endpoint | Before | What Lighthouse's VC does on the 404 | |---|---|---| | `GET /eth/v1/beacon/states/{state_id}/validators/{validator_id}` | 404 (only `?id=` and `POST` served) | It resolves each key's index through this route. Without it, every validator is "inactive", so no attester, sync, liveness or proposer duty is ever requested | | `GET /eth/v2/validator/duties/proposer/{epoch}` | 404 (v1 served) | It asks for v2 only, with no fallback to v1, so "Failed to download proposer duties" every epoch and it never proposes | The first one blocks the end-to-end check lambdaclass#642 and lambdaclass#643 list as still to do: Lighthouse's validator client with `--enable-doppelganger-protection`. Its doppelganger service only calls `liveness` for validators it has indices for. Found on a kurtosis devnet with Lighthouse's validator client talking only to ethlambda beacon. The VC logged `Validator without index` for each of its 64 keys and `All validators inactive` every slot. ## What Changed - **`crates/net/rpc/src/beacon/states.rs`**: `GET /eth/v1/beacon/states/{state_id}/validators/{validator_id}`. It returns one registry entry as an object, by index or by pubkey: the same entry the list endpoint returns, without the array. - **`crates/net/rpc/src/beacon/validator.rs`**: `GET /eth/v2/validator/duties/proposer/{epoch}`. These are v1's duties with v2's `dependent_root`. v1 and v2 share one handler, parameterized by the dependent-root rule. - **`docs/rpc.md`**: both routes, and the v2 `dependent_root` rule. - **`tooling/kurtosis-validator/`**: - `ethlambda_validator.client: lighthouse` runs `lighthouse vc` in place of ethlambda's validator client, on the same derived keys and against the same beacon node list. - Lighthouse writes its definitions file into the validators directory, so it gets a writable copy of the keys. - `doppelganger: true` adds `--enable-doppelganger-protection`. - `lighthouse_extra_params` passes any other flags through. - `network_params_lighthouse_vc.yaml` runs it against ethlambda beacon alone, with doppelganger protection on. - **`bin/ethlambda/src/main.rs`**: `cargo fmt`. A merge of `beacon-chain-integration` left the module list out of order, which is what fails Lint on lambdaclass#642 and lambdaclass#643. ## Correctness / Behavior Guarantees - **`validators/{validator_id}`:** - `404` for a validator the state doesn't hold, `400` for a malformed id. - `state_id` resolves the same way as on every other state endpoint. - **v2 proposer duties:** - `dependent_root` is the block root at the slot before the **previous** epoch's first slot, since fulu fixes proposers an epoch ahead. Where that would underflow, it's the genesis block root. - `503` while syncing. - v1 is unchanged. ## Tests Added / Run - `validators/{validator_id}`: - `one_validator_by_pubkey_or_index_is_the_list_entry` - `one_unknown_validator_is_a_404` - `one_malformed_validator_id_is_a_400` - v2 proposer duties: - `v2_proposer_duties_are_v1_s` - `the_v2_dependent_root_is_the_block_before_the_previous_epoch` - `the_v2_dependent_root_is_genesis_where_it_would_underflow` - `v2_proposer_duties_answer_503_while_syncing` - `cargo test -p ethlambda-rpc --profile release-fast`: 136 passed, 0 failed. - **Kurtosis devnet, Lighthouse v8.2.3's validator client talking only to ethlambda beacon** (built from this branch). The setup: - 2× geth+lighthouse hold validators [0, 112); Lighthouse's VC holds [112, 128) with `--enable-doppelganger-protection`. With `network_params_lighthouse_vc.yaml`'s split, where the VC holds every key, the chain stops during the doppelganger wait (see below). - Lighthouse skips doppelganger for validators registered at genesis, so the VC was restarted at slot 40 to exercise it. | Check | Result | |---|---| | Index resolution (`validators/{validator_id}`) | ✅ all 16 resolved; before this PR: `Validator without index` for every key | | v2 proposer duties | ✅ `current_epoch_proposers: 7`; it proposed slots 3, 4, 29 and 30. `dependent_root` matches Lighthouse's BN for epochs 0 and 1 | | Attestations and aggregates through ethlambda | ✅ published | | `duties/sync` (lambdaclass#642) | ✅ `Validator in sync committee` for 112 and 114 | | Doppelganger via `liveness` (lambdaclass#643) | ✅ after the restart: `Listening for doppelgangers` (16), then `Found no doppelganger … further_checks_remaining: 0` and `Doppelganger detection complete: starting validator`. The validators unmuted at slot 128 (epoch 4) and attested at once. `liveness` answered `false` for the muted keys and `true` for a participant's | | Finality | ✅ justified 3, finalized 2 at epoch 4 | - **Found on the devnet, not fixed here:** - **Sync-committee endpoints missing.** The node doesn't serve `sync_committee_subscriptions`, `pool/sync_committees` or the contribution endpoints. Lighthouse logs `Unable to publish sync committee messages` and `No aggregate contribution found` (CRIT) every slot. This is the known gap lambdaclass#642 describes. - **Duties refused at each epoch boundary.** `duties/attester` and `duties/proposer` bound the requested epoch by the **head state's** epoch, not the clock's (`validator.rs`, from the validator API work). At an epoch's first slot, before its block arrives, the next epoch's duties are a 400 (`epoch is not within one epoch of the head state's`). Lighthouse retries next slot and gets them, but it logs an ERROR every epoch. Lighthouse's BN advances the state instead; its own VCs logged no such failure. - **Stall with the committed split.** `network_params_lighthouse_vc.yaml`'s split, where the VC holds all 128 keys, risks a stall: while doppelganger mutes the VC's validators, a run of more than 8 missed slots puts Lighthouse past its sync tolerance and every VC stops. An earlier run with a 64/64 split stopped at slot 55. - **Proposer refused at the slot boundary.** Lighthouse's VC asked to propose slot 29 a second time 3 ms before slot 30, once the block was already imported; ethlambda answered `400 slot is not after the head block`. Harmless: slashing protection would refuse the second signature too. ## Related Issues / PRs - Stacked on lambdaclass#643, which is stacked on lambdaclass#642. Retarget to `beacon-chain-integration` once those merge. - Unblocks the Lighthouse end-to-end check listed under "Not yet run" in lambdaclass#642 and lambdaclass#643. ## ✅ Verification Checklist - [x] Ran `make fmt` — clean - [x] Ran `make lint` (clippy with `-D warnings`) — clean - [ ] Ran `make test` (`test-consensus` plus `test-node`, at `release-fast`) — only `ethlambda-rpc` locally (136 passed), the only crate with code changes; the full suite runs in CI
🗒️ 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)