Skip to content

feat(validator_store): publish PTC validator set on VotingAssignments - #1053

Closed
shane-moore wants to merge 3 commits into
sigp:epbsfrom
shane-moore:feat/ptc-voting-assignments
Closed

feat(validator_store): publish PTC validator set on VotingAssignments#1053
shane-moore wants to merge 3 commits into
sigp:epbsfrom
shane-moore:feat/ptc-voting-assignments

Conversation

@shane-moore

Copy link
Copy Markdown
Member

Problem, Evidence, and Context

Closes #1036. Next ePBS milestone item after PR #1033 (Role::PTCCommittee + message-validator handling), alongside PR #1047 (PayloadAttestationVote SSZ container).

Per SIP-94 §3 and the Gloas validator spec, each PTC-assigned validator signs PayloadAttestationData under DOMAIN_PTC_ATTESTER. In SSV, a cluster runs one QBFT instance per slot and each locally-assigned PTC validator contributes a partial signature on the decided data. The downstream sign_payload_attestation path (#1037) needs the per-cluster partial-signature batch size at signing time, mirroring how sign_committee_attestations uses voting_message_count_for_committee at `lib.rs:1534-1537` for the attestation batch.

This PR publishes that count input on `VotingAssignments` so #1037 can size its batch with the same closure-based filter pattern.

Change Overview

Additive change across four files, +122/-3. All work lives in `anchor/validator_store`.

  • `VotingAssignments::ptc_validators: Vec` — new field populated at slot start during Phase 1, mirroring `attesting_validators` exactly.
  • `ptc_signature_count_for_committee(F)` — count helper on `VotingAssignments`, closure-based filter matching sibling `voting_message_count_for_committee`. PTC stays in a separate helper because the QBFT instance is separate from attestation.
  • Fork-gated wire-up in `update_voting_assignments` — first `>= Fork::CStar` site in the codebase; mirrors the existing `>= Fork::Boole` precedent at `metadata_service.rs:501`. Pre-CStar slots get an empty vec.
  • Pure helper `build_ptc_validators` — intersects LH's epoch-cached `PtcMap` with the local validator set; filters liquidated clusters via `.is_ok()` on `get_validator_and_cluster` (same pattern as the existing Phase-2/3 builder at `metadata_service.rs:421-428`).
  • `METADATA_SERVICE_PTC_VALIDATORS` gauge — sibling of existing attester/sync gauges.
  • Empty-assignments counter tightened — now requires `attester_count == 0 && sync_count == 0 && ptc_count == 0` so a PTC-only slot post-CStar isn't miscounted as empty.

Reading order: start at `validator_store/src/lib.rs` (new field + count helper on `VotingAssignments`); then `metadata_service.rs` (fork-gated lookup + the pure helper in the helper bay); unit tests in the existing `#[cfg(test)] mod tests` block mirror `test_aggregators_sorted_by_validator_index`.

Intentionally unchanged:

Risks, Trade-offs, and Mitigations

Slot-start lookup cost. Adds ~30 O(1) hashmap reads per slot for a typical operator (1-10 PTC duties, three `database.state()` reads each via `get_validator_and_cluster`). Cheaper than the per-validator `SyncSubnetId::compute_subnets_for_sync_committee::(...)` already running in Phase 1. The alternative (separate 9s-fraction fetch phase) was rejected because PTC duties are epoch-stable and the source is an in-memory `PtcMap` read.

Flat vec vs per-cluster grouping. `ptc_validators` is flat across clusters, mirroring `attesting_validators`. The attestation precedent at `lib.rs:1534-1537` shows the flat-vec + closure-filter pattern is sufficient for per-cluster batch sizing; #1037 mirrors it. If a non-count consumer ever emerges, a refactor is one field-shape change with the call site as the regression test.

Empty-assignments counter behavior. Tightening to include `ptc_count == 0` makes the counter strictly more accurate — it fires only when no duties exist at the slot. Not noisier during early CStar rollout; the prior condition would have falsely counted PTC-only slots as empty post-fork.

Validation

  • `cargo test -p anchor_validator_store` — 36/36 pass (3 new PTC tests + 33 existing).
  • `make cargo-fmt && make cargo-fmt-check` — clean.
  • `make lint` — clean.
  • `cargo check --workspace` — clean.

New test coverage (pure-helper tests, no tokio runtime, no `MetadataService` instance):

  • `test_build_ptc_validators_empty_input` — empty input returns empty Vec.
  • `test_build_ptc_validators_all_local` — all-`true` predicate retains every duty with correct `u64 → usize` conversion.
  • `test_build_ptc_validators_filters_liquidated` — predicate returns false for one pubkey (simulating `ClusterLiquidated`); excluded duty is dropped.

Rollback

Additive change on a metadata-only struct. Revert removes the field, helper method, fork-gated lookup, pure helper, gauge, and tests. Pre-CStar behavior is unchanged (empty vec until fork activation), so revert pre-CStar is a no-op. Post-CStar revert removes the input #1037 reads, but #1037 has not landed yet, so no downstream impact.

Blockers / Dependencies

None for merge. Downstream consumer (milestone #4):

Additional Info / Next Steps

N/A

@shane-moore

Copy link
Copy Markdown
Member Author

@claude, please take a look

@claude-code-actions-sigp

Copy link
Copy Markdown

Claude Code is working…

I'll analyze this and get back to you.

View job run

@codecov-commenter

codecov-commenter commented May 22, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 76.82927% with 19 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (epbs@80d25de). Learn more about missing BASE report.

Files with missing lines Patch % Lines
anchor/validator_store/src/metadata_service.rs 75.43% 14 Missing ⚠️
anchor/validator_store/src/metrics.rs 0.00% 5 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             epbs    #1053   +/-   ##
=======================================
  Coverage        ?   62.08%           
=======================================
  Files           ?      156           
  Lines           ?    26317           
  Branches        ?        0           
=======================================
  Hits            ?    16340           
  Misses          ?     9977           
  Partials        ?        0           
Flag Coverage Δ
rust 62.08% <76.82%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@claude-code-actions-sigp

Copy link
Copy Markdown

Claude Code is working…

I'll analyze this and get back to you.

View job run

1 similar comment
@claude-code-actions-sigp

Copy link
Copy Markdown

Claude Code is working…

I'll analyze this and get back to you.

View job run

@shane-moore

Copy link
Copy Markdown
Member Author

@claude, please take a look

@claude-code-actions-sigp

claude-code-actions-sigp Bot commented May 25, 2026

Copy link
Copy Markdown

Claude finished @shane-moore's task in 3m 48s —— View job


Review

Verdict: LGTM with minor optional improvements. The change is tightly scoped, additive, and faithfully mirrors the existing Fork::Boole / sync-validator precedents.

  • Read changed files
  • Verify fork-gating, helper correctness, empty-assignments counter logic
  • Check tests against pure helper invariants
  • Post review feedback

What's good

  • Pattern fidelity. update_voting_assignments mirrors the sibling code at metadata_service.rs:517 (Fork::Boole-gated Phase-2 branch), and the closure-based filter on VotingAssignments matches voting_message_count_for_committee at lib.rs:1982. Future readers immediately see the precedent.
  • Tightened empty-assignments counter at metadata_service.rs:353. Before this PR, a PTC-only slot post-CStar would have been miscounted as empty. The added ptc_count == 0 clause makes the metric strictly more accurate; pre-fork the new term is always 0, so there's no behavior drift before activation.
  • Pure helper extraction (build_ptc_validators) gives the unit tests a tokio-free entrypoint and matches the Phase-2/3 builder shape at metadata_service.rs:421-428. Tests at metadata_service.rs:2285-2326 cover empty input, all-local, and one-liquidated. AAA, named, and minimal.
  • Test harness updates across lib.rs:3221, lib.rs:3387, lib.rs:3410, testing/common.rs:315 are consistent — no struct-literal sites missed.

Minor suggestions (optional)

  1. Visibility of build_ptc_validatorsmetadata_service.rs:1279 is pub but the only non-test caller is at metadata_service.rs:319. Consider pub(crate) (or just fn) to keep the public API of this crate trim. The neighboring filter_contributors_with_contributions at :1269 is pub for the same reason, so consistency cuts both ways — flagging as a thought, not a blocker. Fix this →

  2. Lock-acquisition fan-out. The closure at metadata_service.rs:319-323 calls get_validator_and_cluster(*pubkey), which calls self.database.state() — i.e. one lock acquisition per PTC duty. The aggregation path at :441 does the same, so this PR is consistent with precedent and the absolute counts are tiny, but it's worth a tracking note for a future operator running thousands of validators. Not a fix to do here.

  3. Coverage of the fork-gated branch. Codecov flags 14 uncovered lines in metadata_service.rs (76.8%), most of which I expect to be the new Phase-1 branch at :315-326 and :328-358. The pure helper is fully covered, but the wire-up itself isn't exercised end-to-end. Given how tightly this mirrors the Fork::Boole precedent (also without dedicated branch tests), I don't think this blocks merge — but if you want to harden, an integration test that drives update_voting_assignments across a CStar boundary (asserting ptc_validators empty pre-fork, populated post-fork) would close the gap.

Non-issues I checked

  • d.validator_index as usize (:1286) — matches the existing pattern at :276, :305, :640. Anchor targets 64-bit, so the cast is safe and consistent. No change needed.
  • get_validator_and_cluster(...).is_ok() swallows ClusterLiquidated and ValidatorClusterMismatch (with internal error! logging). Same pattern as :441. Consistent.
  • Duplicate handling in ptc_validators. It's a flat Vec. Lighthouse's PTC duty source is per-(validator, slot), so duplicates are not expected. Fine.
  • Closure borrow of self in :319-323 is sync and scoped to the build_ptc_validators call — no move needed, no lifetime issue.

Risk

Low. Pre-CStar paths are bit-for-bit unchanged (the new branch returns Vec::new() and the tightened counter clause is && ptc_count == 0 which is always-true pre-fork). Revert is a clean shape change with #1037 not yet landed.
· branch feat/ptc-voting-assignments

@shane-moore

shane-moore commented May 25, 2026

Copy link
Copy Markdown
Member Author

@claude, thanks. On each:

1. Keeping pub on build_ptc_validators to match the sibling pure helpers in this file (sort_aggregators_by_validator_index, sort_contributors_by_signing_root_then_validator_index, filter_aggregators_with_attestations, filter_contributors_with_contributions), all pub with zero out-of-crate callers, same as the parent struct's fields. C-STRUCT-PUBLIC is satisfied either way; no behavioral difference in a workspace-internal crate.

2. I don't think there's a real gap here. database.state() is a watch::Sender::borrow(), i.e. a nanosecond-scale read on the watch RwLock; 1-10 borrows per slot is microseconds. The "cache it" alternative is actively worse: holding a Ref<NetworkState> across the closure would block every send_modify/send_if_modified on the state channel (database/src/lib.rs:310,316), which the index-sync path uses. Serial cheap borrows are the lock-friendly shape. Attestation does this for hundreds of validators per slot without trouble; no flamegraph or metric has flagged any of the three sites.

3. No update_voting_assignments-level branch test exists for the Boole precedent at :517, and more broadly every test in metadata_service.rs is a pure-helper unit test. The orchestrator is untested end-to-end across the board, not just for fork branches. Adding scaffolding (mock DutiesService/Database/SlotClock plus a tokio runtime) belongs in its own coverage-investment PR that exercises the whole orchestrator (Phases 1/2/3 + Boole + CStar together), not as a one-off branch test here.

@claude-code-actions-sigp

claude-code-actions-sigp Bot commented May 25, 2026

Copy link
Copy Markdown

Claude finished @shane-moore's task in 1m 10s —— View job


Acknowledged — all three points stand

  • Verify sibling pure helpers visibility claim
  • Verify database.state() is watch::Sender::borrow()
  • Verify orchestrator-level test coverage shape in metadata_service.rs
  • Post response

1. pub consistency. Confirmed at metadata_service.rs:1237/1246/1259/1269/1279 — all five sibling pure helpers are pub with no out-of-crate callers. Keeping pub for shape parity is the right call; my suggestion was a wash and you flagged the right tradeoff.

2. database.state() cost. Confirmed at database/src/lib.rs:201-203: pub fn state(&self) -> Ref<'_, NetworkState> { self.state.borrow() } over watch::Sender<NetworkState> (:149). You're right that the per-call cost is a watch borrow, not a contended RwLock acquisition, and that holding a Ref across the closure would block send_modify/send_if_modified writers — the serial-cheap-borrows shape is genuinely the lock-friendly one here. Withdrawing the note.

3. Orchestrator-level coverage. Confirmed — every #[test] in metadata_service.rs (lines 1401-2326) is a sync pure-helper unit test; there is no orchestrator harness for update_voting_assignments and no precedent test for the Fork::Boole branch at :517 either. Agreed that scaffolding a mock DutiesService/Database/SlotClock + tokio runtime is a coverage-investment PR of its own that should exercise the whole orchestrator (Phases 1/2/3 + both fork branches together), not a one-off branch test bolted onto this PR.

No changes requested. PR stands as-is from my side.
· branch feat/ptc-voting-assignments

@shane-moore shane-moore closed this Jun 5, 2026
@shane-moore

Copy link
Copy Markdown
Member Author

closing for now since we're going to move to a non-qbft ptc approach

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants