feat(rpc): serve states/{id}/fork, config/deposit_contract and validator/duties/sync - #642
pablodeymo wants to merge 1 commit into
Conversation
🤖 Claude Code ReviewReview: PR 642 (sync duties,
|
🤖 Codex Code ReviewLooks good overall. The added endpoints are small, well-scoped, and the sync-duty implementation is efficient enough for the stated constraints. I did not spot consensus-critical bugs in fork choice, finalization, state transition, XMSS, or SSZ paths from this diff.
One small maintainability/perf note:
No security concerns jumped out from this PR. Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Kimi Code ReviewI'll review this PR systematically across correctness, security, performance, and Rust best practices. Overall AssessmentThis PR adds three Beacon API endpoints ( Detailed Findings1. Critical: Integer Overflow Risk in
|
| Priority | Item | File | Line |
|---|---|---|---|
| High | Overflow risk in period multiplication | validator.rs |
~1004 |
| Medium | Potential DoS via unbounded indices |
validator.rs |
72-76 |
| Medium | Inefficient eager HashMap construction | validator.rs |
95-100 |
| Low | Implicit Default::default() in tests |
validator.rs |
multiple |
| Low | hex::encode(pubkey.0) fragility |
validator.rs |
983 |
| Low | Missing execution_optimistic test |
states.rs |
457-489 |
The PR is generally well-crafted and ready for merge after addressing the overflow risk (Item 1) and considering the DoS limit (Item 2).
Automated review by Kimi (Moonshot AI) · kimi-k2.6 · custom prompt
MegaRedHand
left a comment
MegaRedHand
left a comment
There was a problem hiding this comment.
A few comments, nothing blocking. The period bound is the only behavior change I'd consider; the rest are small cleanups and tighter tests.
| let requested_period = compute_sync_committee_period(epoch); | ||
| let committee = if requested_period == head_period { | ||
| current | ||
| } else if requested_period == head_period + 1 { |
There was a problem hiding this comment.
Minor: the allowed periods are measured from the head state's slot, not the clock. When the head is still in period P but the clock is already in P+1 (an empty first slot of the new period, or a head a few slots behind), a request for P+2 is a valid "next period" request and gets a 400 here.
Lighthouse covers this by advancing a lagging head state to the current period before answering (duties_from_state_load in http_api/src/sync_committees.rs). Here that would mean advancing through fork_choice::checkpoint_state, as attestation_data already does, since P+2's committee only exists in the advanced state's next_sync_committee.
| for validator_index in indices { | ||
| let validator = state | ||
| .validator(validator_index) | ||
| .map_err(|_| ApiError::BadRequest("unknown validator index"))?; |
There was a problem hiding this comment.
Nit: a 400 for an index the state doesn't know matches Lighthouse, but attester_duties in this same file skips unknown indices and answers 200. Worth picking one behavior for both duty endpoints, so a validator client sending the same index set to both gets consistent answers?
| indices: &[String], | ||
| ) -> Result<serde_json::Value, ApiError> { | ||
| let epoch = parse_epoch(epoch)?; | ||
| let indices = indices |
There was a problem hiding this comment.
Nit: this parse block is the same as attester_duties', which collects into a HashSet and so also drops duplicates. Here ["3", "3"] returns validator 3's duty twice. A shared parse_indices helper next to parse_epoch would keep both in line.
|
|
||
| // One pass over the committee rather than one per requested validator: | ||
| // the committee stores pubkeys, so that is what a validator is matched by. | ||
| let mut positions: HashMap<BlsPubkey, Vec<String>> = HashMap::new(); |
There was a problem hiding this comment.
Nit: this allocates a String per seat and then clones the Vec for each duty. Storing Vec<u64> positions and serializing the field with serialize_with = "ethlambda_types::beacon::serde_helpers::quoted_u64_seq::serialize" (as the altair containers do) avoids the per-seat allocations; once the requested indices are deduplicated, remove instead of get + clone avoids the copy too.
| state; see `docs/spec_deviations.md`). A validator is matched by pubkey and | ||
| gets every seat it holds, since the committee is drawn with replacement; one | ||
| with no seat is left out. An unknown index is a `400`, and the endpoint is a | ||
| `503` while the node is syncing. This node serves no sync committee message |
There was a problem hiding this comment.
Minor: besides the message and contribution endpoints, POST /eth/v1/validator/sync_committee_subscriptions is also unserved. Once a validator client gets sync duties it starts calling it, so for a VC with a sync committee member the visible effect of this PR is more 404s (subscriptions per epoch, messages and contributions per slot) instead of one 404 on duties/sync. Worth listing the subscriptions gap here too.
|
|
||
| /// `GET /eth/v1/beacon/states/{state_id}/fork`: the `Fork` the state carries, | ||
| /// which is what a validator client builds its signing domains from. | ||
| async fn get_fork(Path(state_id): Path<String>, State(store): State<Store>) -> Response { |
There was a problem hiding this comment.
Nit: the load, error and execution_optimistic + finalized envelope here is the same as get_finality_checkpoints'. A small helper that takes a closure over the state (state_json(store, state_id, |state| data)) would keep the state sub-resources consistent as more are added.
| crate::json_response(serde_json::json!({ | ||
| "data": { | ||
| "chain_id": config.deposit_chain_id.to_string(), | ||
| "address": HexPrefixed(&config.deposit_contract_address).to_string(), |
There was a problem hiding this comment.
Nit: hex_string(config.deposit_contract_address), defined further down in this file, does the same thing.
| let response = app.oneshot(request).await.unwrap(); | ||
| let status = response.status(); | ||
| let body = response.into_body().collect().await.unwrap().to_bytes(); | ||
| (status, serde_json::from_slice(&body).unwrap_or_default()) |
There was a problem hiding this comment.
Test nit: unwrap_or_default() turns a non-JSON body into Null, and the 400 tests only check the status code, so they would still pass if a different 400 path fired (epoch parse, body rejection). Unwrapping the parse and asserting on json["message"] would pin each test to the check it names.
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn the_current_period_reads_the_current_committee() { |
There was a problem hiding this comment.
Test gap: every current-period test asks for exactly the head's epoch. A case for a later epoch in the same period (for example head_epoch + 1) would catch a regression to comparing epochs instead of periods.
…eposit_contract and POST /eth/v1/validator/duties/sync/{epoch} from ethlambda beacon, three of the four Beacon API endpoints validator clients call that were missing. fork returns the state's own Fork through the same state_id resolution as the other state endpoints, so a state root is the same 404. deposit_contract returns the Config's deposit chain id and contract address. duties/sync reads the head state's current_sync_committee for an epoch in the head's own sync committee period and next_sync_committee for the next one, matches each requested validator by pubkey and returns every seat it holds (the committee is drawn with replacement), leaves out validators with no seat, and answers 400 for an unknown index or any other period and 503 while syncing. An earlier period is refused rather than answered from a historical state, recorded in docs/spec_deviations.md. compute_sync_committee_period is added to the altair helpers as validator.md defines it.
a7f7f83 to
b252fbc
Compare
…poser-duties-v2 #642 was squashed onto the current beacon-chain-integration, so this merge ran against the old base and both sides re-added #642's work. Git kept the `duties/sync` test module twice; the second copy is dropped. In docs/rpc.md the duties paragraph keeps this branch's `dependent_root` wording.
This branch has not been deployed
2 participants
🗒️ Description / Motivation
Validator clients call Beacon API endpoints that ethlambda beacon didn't serve. This PR adds three of the four:
GET /eth/v1/beacon/states/{state_id}/fork(ValidatorRequiredApi): used for fork detection and signing domains. Prysm's validator client calls it in its doppelganger check.GET /eth/v1/config/deposit_contract: network sanity checks and tooling. Prysm's validator client needs it for voluntary exits and its keymanager UI.POST /eth/v1/validator/duties/sync/{epoch}(ValidatorRequiredApi): a validator client's sync committee service. Without it, Lighthouse and Prysm log a warning and skip sync duties.The fourth,
POST /eth/v1/validator/liveness/{epoch}, comes in a separate PR, since it's the only one that needs new shared state.What Changed
crates/net/rpc/src/beacon/states.rs:get_fork. It resolvesstate_idwith the sameload()as the other state endpoints and returnsstate.fork(), plusexecution_optimisticandfinalized.crates/net/rpc/src/beacon/config.rs:get_deposit_contract. It returnsConfig::deposit_chain_idanddeposit_contract_address, the same values/config/specreports.crates/net/rpc/src/beacon/validator.rs:post_sync_duties/sync_duties.crates/blockchain/state_transition/src/beacon/helpers/altair.rs:compute_sync_committee_period, as altair'svalidator.mddefines it.docs/rpc.md, a note on sync duties, and a newdocs/spec_deviations.mdentry.Correctness / Behavior Guarantees
forkduties/synccurrent_sync_committee; the next period readsnext_sync_committee.data, as the spec says.duties/attesterandduties/proposerdon't return 503; that's unchanged here.Tests Added / Run
fork:the_fork_is_the_one_the_state_carriesthe_finalized_states_fork_is_marked_finalizeda_fork_by_state_root_is_a_404deposit_contract:the_deposit_contract_is_mainnetsduties/sync, on a fixture whose current committee is validators 0–7 and next committee validators 8–15, so reading the wrong one shows up:the_current_period_reads_the_current_committee(also covers a validator in neither committee, and every seat listed)the_next_period_reads_the_next_committeethe_period_after_next_is_a_400an_earlier_period_is_a_400an_unknown_validator_is_a_400a_syncing_node_answers_503compute_sync_committee_period:a_sync_committee_period_spans_its_epochs_and_no_morecargo test -p ethlambda-types --lib --features preset-minimaland the same forethlambda-state-transition).Related Issues / PRs
✅ Verification Checklist
make fmt— cleanmake lint(clippy with-D warnings) — cleanmake test(test-consensusplustest-node, atrelease-fast) — all passing (1948 passed, 0 failed, 26 ignored)