feat(validator_store): use decided votes for aggregation - #1128
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## epbs #1128 +/- ##
=======================================
Coverage ? 66.21%
=======================================
Files ? 163
Lines ? 29345
Branches ? 0
=======================================
Hits ? 19430
Misses ? 9915
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@claude, please take a look |
|
Claude encountered an error after 11m 49s —— View job Claude PR Review
|
|
@claude, please take a look |
|
Claude finished @shane-moore's task in 10m 5s —— View job Claude PR Review
Reviewed the two commits against I did not find any correctness or safety issues I'm confident are bugs. A few non-blocking observations below. Observations (non-blocking)1. New failure mode injected into the signing hot path — let decided_vote = result?;
voting_context.remember_decided_vote(committee_id, decided_vote.clone())?;After QBFT successfully decides, a 2. Repeated
3. Confirm Nothing here blocks merge from my perspective — the core isolation and dedup logic is correct and well-tested. |
|
@claude, for the three observations:
|
|
Claude finished @shane-moore's task in 1m 11s —— View job Responses confirmedI re-checked each point against the current branch ( 1. Fail-closed on 2. Repeated 3.
The conflict originates in Nothing outstanding from my side. All three observations were non-blocking and your responses resolve them. |
jnhsigmap
left a comment
There was a problem hiding this comment.
Approved b/c none of these are blocking but just some suggestions from claude and analysis 🙂
another one that came up was an integration test for build_consensus_data_for_all_committees to ensure that it feeds each committee's decided vote and not the seed into the fetch keys when the two committees differ.
| let mut aggregate_requests = HashSet::new(); | ||
| let mut sync_requests = HashSet::new(); | ||
|
|
||
| for ssv_committee_id in &ssv_committees { |
There was a problem hiding this comment.
This is a bit nit-picky but here build_consensus_data_for_all_committees creates and populates committee_votes in the first loop by iterating ssv_committees, then the second loop iterates over ssv_committees again and returns Err if no vote is found for a ssv_committee_id?
let vote = committee_votes
.get(&ssv_committee_id)
.ok_or_else(|| format!("Missing vote for committee {ssv_committee_id:?}"))?;
committee_votes contains all of the ssv_committee_id values that we want, so ok_or_else technically should never be reached. Could we loop over committee_votes directly here?:
let mut result = HashMap::with_capacity(committee_votes.len());
for (ssv_committee_id, vote) in &committee_votes {
let consensus_data = self.build_consensus_data_for_committee(
slot,
ssv_committee_id,
attesters_by_ssv_committee.get(ssv_committee_id),
sync_by_ssv_committee.get(ssv_committee_id),
vote,
&fetch_results,
)?;
if let Some(data) = consensus_data {
result.insert(*ssv_committee_id, Arc::new(data));
}
}
There was a problem hiding this comment.
Resolved in e62322f7: iterate committee_votes directly and remove the unreachable missing-vote branch.
| } | ||
|
|
||
| #[test] | ||
| fn voting_context_returns_committee_decision_or_seed() { |
There was a problem hiding this comment.
could be worth adding a doc comment like "Tests vote_for_committee lookup rule. Does a committee get its own decided vote back once one is recorded, and does an undecided committee fall back to the slot seed." to make it clear what is being tested?
There was a problem hiding this comment.
Resolved in adc3858c: document the committee decision lookup and seed-fallback behavior covered by the test.
| } | ||
|
|
||
| #[test] | ||
| fn build_consensus_data_uses_composite_results_and_preserves_wire_order() { |
There was a problem hiding this comment.
fair bit going on here, test itself is sound, just could do with an explanation of objectives.
Objectives:
- Foreign-result isolation: Results fetched under a different vote must never be consumed by a committee with
foreign_vote.- Wire ordering: De-duplicated/surviving aggregators/contributors and their index lists come out in the exact go-ssv-compatible order as per "The sorting order MUST match SSV-Go exactly for consensus compatibility."
There was a problem hiding this comment.
Resolved in 73159658: document the composite-key isolation and go-ssv-compatible wire-ordering objectives.
|
Queued — the merge queue status continues in this comment ↓. |
Merge Queue Status
This pull request spent 12 minutes 11 seconds in the queue, including 10 minutes 44 seconds running CI. Required conditions to merge
|
…ions epbs (sigp#1082/sigp#1103/sigp#1128) added `spec` and `forced_gloas_index` to the shared test HarnessOptions. Append `..Default::default()` to the two ProposerPreferences failure-test constructions, matching the sibling payload_attestation tests, so the suite compiles on the rebased base. Part of sigp#1063.

Problem, Evidence, and Context (Required)
Committee signing from #1103 uses the QBFT-decided vote, but aggregation still derived aggregate-attestation and sync-contribution requests from the local slot seed. Under Gloas, this can omit the decided
AttestationData.indexfrom the requested root or use a block root the committee did not sign, causing missed or mismatched aggregation.Closes #1113.
Related context:
Change Overview (Required)
The commits are ordered for review: the first establishes the decision handoff and conflict safety, and the second consumes those decisions in aggregation.
Design rationale and data flow
Before this change, aggregation could use the slot's locally selected seed vote for every SSV committee. The fetched aggregate-attestation results were effectively associated by beacon committee index, and sync contributions by subnet. That was sufficient while every SSV committee used the same block and attestation-data inputs.
After #1103, each SSV committee can complete QBFT with its own
SlotVote. In Gloas, that vote also contains the decidedAttestationData.index. Two committees can therefore request the same beacon committee index or sync subnet against different decided roots. Reusing a result based only on committee index or subnet can attach data fetched for vote A to the consensus data for vote B.The implementation separates decision retention from fetch-result deduplication:
VotingContextretains the first successful decision for each SSV committee. It is already slot-scoped, so decisions expire with the existing voting context and require no independent eviction policy. If a committee has not decided before aggregation runs, lookup falls back to the slot seed to preserve the previous partial-progress behavior.MetadataServiceresolves one vote for each SSV committee, then derives the exact Beacon API requests required by that committee.(attestation_data_root, beacon_committee_index)(block_root, sync_subnet_id)The composite keys are intentional. Keying results only by SSV committee would prevent useful sharing when committees agree and would still need another level for multiple beacon committees or sync subnets. Keying only by beacon committee index or subnet is unsafe once committee votes diverge. The complete request identity provides isolation while retaining cross-committee deduplication.
Risks, Trade-offs, and Mitigations (Required)
Validation (Required)
cargo test -p anchor_validator_store(57 passed)cargo clippy -p anchor_validator_store --tests -- -D warnings -D clippy::allow_attributescargo fmt --all -- --checkgit diff --check upstream/epbs...HEADNew tests cover attestation-first and sync-first decisions, fallback, idempotence, conflicting decisions, Gloas index binding, composite-key deduplication and isolation, partial results, and deterministic ordering.
Rollback (Required for behavior or runtime changes; optional otherwise)
Revert the two commits. The cache is in-memory and slot-scoped, with no database, configuration, or network migration.
Blockers / Dependencies (Optional)
#1103 is merged into
epbs. This branch is based on its merge commit,3b174388.