feat: build AggregatorCommitteeConsensusData and post consensus handling by shane-moore · Pull Request #786 · sigp/anchor

shane-moore · 2026-01-26T04:13:07Z

Issue Addressed

Continues committee-based aggregation support for the Boole fork. This PR support #770

Proposed Changes

Build AggregatorCommitteeConsensusData in the validator store metadata service for QBFT consensus
Update produce_aggregate_and_proof to handle post boole committee based aggregation flow with post-consensus signing
Update produce_sync_aggregate_and_proof to handle post boole committee based sync aggregation flow with post-consensus signing
Fetch aggregated attestations and sync contributions from beacon nodes with parallel requests and timeouts
Implement sorting and filtering for consensus compatibility with SSV Go
Add metrics and logging for consensus data construction and pre-consensus validation

Additional Info

The consensus data follows SSV protocol requirements:

Aggregators sorted by validator index
Contributors sorted by signing root then validator index
1:1 correspondence between aggregators and fetched attestations
Handles partial fetch failures while maintaining consensus data integrity

dknopik

Nice! Have only skimmed the metadata_service, will continue tomorrow. But looking good!

dknopik · 2026-02-02T16:19:17Z

anchor/message_validator/src/message_counts.rs

    /// Returns an error if the message type exceeds its respective count limit.
+    ///
+    /// Note: `debug_context` is an optional tuple of (slot, operator_id, num_signatures_in_message)
+    /// used for debugging Issue 4 (pre-consensus InvalidPartialSignatureTypeCount).


I think this comment is outdated?

yep, outdated, removed!

dknopik · 2026-02-02T16:22:49Z

anchor/message_validator/src/message_counts.rs

+                    let validator_indices: Vec<_> = messages
+                        .messages
+                        .iter()
+                        .take(5)
+                        .map(|m| m.validator_index)
+                        .collect();
+                    warn!(
+                        slot = %messages.slot,
+                        kind = ?messages.kind,
+                        num_signatures = messages.messages.len(),
+                        ?validator_indices,
+                        "Pre-consensus message rejected (limit exceeded)"
+                    );
                    return Err(ValidationFailure::InvalidPartialSignatureTypeCount {


is this a relic of debugging and to be removed or is there a specific reason why this is added?

yep, helped debugging during ssv-mini testing. related error has since been resolved in ssv repo

removed it since should be good now

dknopik · 2026-02-02T16:23:33Z

anchor/validator_store/src/metrics.rs

+// ═══════════════════════════════════════════════════════════════════════════════
+// AggregatorCommittee metrics
+// ═══════════════════════════════════════════════════════════════════════════════


Thanks for adding metrics! <3

dknopik · 2026-02-02T16:34:52Z

anchor/validator_store/src/lib.rs

-                Attestation::Electra(_) => ForkName::Electra.into(),
-            };
+            // Fork-gated logic for Boole+ committee-based consensus
+            if self.fork_schedule.active_fork(signing_epoch) >= Fork::Boole {


This is a looooong if-else.

What do you think about moving the bodies into two function, e.g. produce_signed_aggregate_and_proof_boole and produce_signed_aggregate_and_proof_alan?

was thinking maybe easier to rip out the alan logic some point post fork with just the if/else. but pretty much just as easy to rip out the helper functions later as well and inline. agreed, refactored!

dknopik · 2026-02-02T16:41:14Z

anchor/validator_store/src/lib.rs

+                // Defensive check: validator should be in VotingAssignments since both this
+                // function call and VotingAssignments are derived from DutiesService. If not,
                // there's an inconsistency (e.g., stale cache after poll timeout) and we
-                // should not participate with a wrong `num_signatures_to_collect`.
+                // should not participate with a wrong num_signatures_to_collect.


Everywhere else in this file, you add the backticks, here you remove them. I am in favor of adding/keeping them :)

ha yah, let's keep them. they've been resurrected

dknopik · 2026-02-02T16:41:36Z

anchor/validator_store/src/lib.rs

+                // Defensive check: validator should be in VotingAssignments since both this
+                // function call and VotingAssignments are derived from DutiesService. If not,
                // there's an inconsistency (e.g., stale cache after poll timeout) and we
-                // should not participate with a wrong `num_signatures_to_collect`.
+                // should not participate with a wrong num_signatures_to_collect.


same, resurrected

dknopik · 2026-02-02T16:45:23Z

anchor/validator_store/src/lib.rs

-                contribution,
-                selection_proof,
-            };
+            if self.fork_schedule.active_fork(epoch) >= Fork::Boole {


Again, wdyt about moving the bodies into independent functions?

dknopik

Another small note, otherwise LGTM.

dknopik · 2026-02-03T12:27:40Z

anchor/validator_store/src/metadata_service.rs

 };

+/// Tuple of (`validator_index`, `pubkey`, `selection_proof`) for sync committee aggregators.
+type SyncAggregatorData = (u64, PublicKeyBytes, SyncSelectionProof);


At this point, a struct might be easier to understand. Matter of taste though.

sure sure! updated in 979dec4

petarjuki7 · 2026-02-03T12:54:45Z

Nice, LGTM! Agree with @dknopik latest comment

shane-moore · 2026-02-03T17:20:28Z

@claude can you do another check of this pr

claude-code-actions-sigp · 2026-02-03T17:20:52Z

Claude finished @shane-moore's task in 2m 25s —— View job

PR Review Complete ✓

I've conducted a comprehensive code review of the Boole fork aggregation implementation. The code is generally high-quality with good Rust practices, but I've identified several issues requiring attention.

Critical Issues (Must Fix Before Merge)

1. Missing Error Context - Production Debugging Risk

Location: anchor/validator_store/src/lib.rs:758, 1054

let validator_index = validator.index.ok_or(SpecificError::MissingIndex)?;

Issue: MissingIndex error lacks context (which validator, which slot). This makes production debugging very difficult.

Recommendation: Enhance the error type to include context:

let validator_index = validator.index.ok_or_else(|| {
    SpecificError::MissingValidatorIndex {
        validator_pubkey,
        slot,
    }
})?;

2. Potential Index Mismatch - Data Safety

Location: anchor/validator_store/src/lib.rs:800-805

let decided_aggregate_bytes = decided_data
    .aggregated_attestations
    .get(decided_aggregate_idx)

Issue: Logic assumes aggregator_committee_indexes.len() == aggregated_attestations.len(). While validation exists in consensus.rs:627-633, this function trusts that validation happened. If bypassed, could sign wrong attestations.

Recommendation: Add defensive assertion:

debug_assert_eq!(
    decided_data.aggregator_committee_indexes.len(),
    decided_data.aggregated_attestations.len(),
    "Decided data validation failed - index/attestation count mismatch"
);

3. Hard-coded Timeout Without Validation

Location: anchor/validator_store/src/metadata_service.rs:46

const BEACON_API_FETCH_TIMEOUT: Duration = Duration::from_secs(2);

Issue: 2-second timeout may be too aggressive for some networks. No validation that it leaves sufficient time for QBFT consensus (which needs to complete before end of 2/3 slot window).

Recommendation: Make configurable via CLI with validation, or add compile-time check ensuring timeout < remaining slot time.

Warnings (Should Fix)

4. Non-Deterministic Iteration Order - Consensus Risk

Location: anchor/validator_store/src/metadata_service.rs:481-485

let ssv_committees: HashSet<CommitteeId> = attesters_by_ssv_committee
    .keys()
    .chain(sync_by_ssv_committee.keys())
    .copied()
    .collect();

Issue: HashSet iteration order is non-deterministic. While consensus data within each committee is deterministic, processing order across committees could affect timing.

Recommendation: Use BTreeSet for deterministic ordering.

5. Potential Slot Race in Cache Access

Location: anchor/validator_store/src/metadata_service.rs:265-270

let voting_assignments = self
    .validator_store
    .get_voting_assignments(slot)
    .await

Issue: Phase 3 (2/3 slot) reads VotingAssignments while Phase 1 (next slot) may be updating. No explicit validation ensures Phase 3 reads data for the correct slot.

Recommendation: Add slot validation after cache read:

if voting_assignments.slot != slot {
    return Err(format!("Slot mismatch: expected {}, got {}", slot, voting_assignments.slot));
}

6. Missing Committee Index Bounds Check

Location: anchor/validator_store/src/metadata_service.rs:712

index: if fork_name < ForkName::Electra { committee_index } else { 0 },

Issue: committee_index from duties service is used without validating it's within beacon chain limits (0-63 for Mainnet).

Recommendation: Add validation before use.

7. Expensive Clone on Critical Path

Location: anchor/validator_store/src/lib.rs:739, 1029

(*our_consensus_data).clone(),

Issue: AggregatorCommitteeConsensusData contains heap-allocated VariableList and large SSZ-encoded attestations. Cloning happens at 2/3 slot timing (critical path).

Recommendation: Profile clone cost and consider storing Arc<AggregatorCommitteeConsensusData> to avoid clones.

Suggestions (Nice to Have)

8. Inefficient Metrics Logging

Location: anchor/validator_store/src/metadata_service.rs:799-814

for _ in 0..successful {
    metrics::inc_counter_vec(...)
}

Issue: Incrementing counter in a loop is inefficient.

Recommendation: Use bulk increment:

metrics::inc_counter_vec_by(&metrics::AGGREGATOR_COMMITTEE_FETCH_SUCCESS, &["aggregated_attestations", "success"], successful as u64);

9. Magic Numbers Without Constants

Location: anchor/validator_store/src/lib.rs:728, 1018

Duration::from_secs(self.spec.seconds_per_slot) * 2 / 3,

Issue: Fraction 2/3 repeated multiple times without explanation.

Recommendation: Define named constant explaining its significance.

10. Missing Early Return Optimization

Location: anchor/validator_store/src/metadata_service.rs:608

if aggregators.is_empty() && contributors.is_empty() {
    return Ok(None);
}

Issue: Check happens after expensive beacon API fetches. Should check before fetching.

11. Error Messages Lack Context

Location: anchor/validator_store/src/metadata_service.rs:734

Error messages in beacon API calls lack slot, committee_index, and endpoint context for debugging.

Strengths ✨

Excellent test coverage - Comprehensive unit tests for sorting, filtering, validation (consensus.rs:1168-1707, metadata_service.rs:991+)
Good use of Rust idioms - Pure helper functions, custom error types with thiserror, appropriate Arc usage
Well-documented sorting requirements - Clear comments explaining consensus compatibility with SSV Go
Proper async/await patterns - Good use of FuturesUnordered with timeout handling, no blocking calls
Defense-in-depth validation - Comprehensive validation logic mirrors SSV Go spec exactly

Summary

Overall Assessment: The code is high-quality with strong Rust practices. Critical issues primarily concern error context and defensive programming rather than fundamental safety bugs.

Priority Actions:

✅ Add context to error types (Issue Draft a Testing Framework #1)
✅ Add defensive assertions for data integrity (Issue Add basic repository structure #2)
✅ Validate/configure beacon API timeout (Issue Remove sigp-crates dependency #3)
✅ Add slot validation in cache reads (Issue Rename the project to "Anchor" #5)
🔍 Profile clone performance (Issue Auto compile and publish mdbook #7)

Impact: Addressing critical issues will significantly improve production debugging and operational safety. The implementation follows Anchor's CLAUDE.md guidelines and maintains high code quality standards.

shane-moore · 2026-02-03T18:09:01Z

@claude, I have reviewed your thoughts 1-11 and have found that none are actionable at this time.

claude-code-actions-sigp · 2026-02-03T18:09:20Z

Claude finished @shane-moore's task in 19s —— View job

PR Review Acknowledgment

Thank you for reviewing my feedback. I understand that none of the 11 items from my previous review are actionable at this time.

Summary

Items Reviewed: 11 total

3 Critical Issues
4 Warnings
4 Suggestions

Outcome: No changes will be implemented based on this review feedback.

Notes

The items I identified remain documented in the previous comment for future reference if circumstances change. The PR has been approved by other reviewers (dknopik, petarjuki7) and appears ready to merge from the team's perspective.

…vents

…om the poll channels

…le fork

dknopik · 2026-02-04T10:26:14Z

@shane-moore Claude's point 8 seems valid and easy

shane-moore · 2026-02-04T13:49:00Z

@shane-moore Claude's point 8 seems valid and easy

@dknopik, agreed! killed a couple loops actually which is nice 81062ba

dknopik

LGTM, thanks!

diegomrsantos · 2026-02-05T19:00:21Z

anchor/client/src/lib.rs

            executor.clone(),
        );

        let metadata_service = MetadataService::new(


I believe we could use a more descriptive name for MetadataService

MetadataService has grown into a “god service” (assignment caching + beacon fetches + consensus data construction + committee grouping). That’s too many responsibilities for one module. A cleaner split would keep this service focused on publishing duty inputs (VotingAssignments, VotingContext, AggregationAssignments) and move consensus-data fetching/building into a dedicated AggregatorConsensusDataBuilder (or similar) module.

Given that scope, MetadataService is misleading. After the split, I’d rename it to DutyInputPublisher to reflect its actual role.

diegomrsantos · 2026-02-05T19:01:13Z

anchor/common/ssv_types/src/consensus.rs

+    /// being split across multiple messages.
+    ///
+    /// Returns: count(aggregators) + count(contributors) filtered by committee membership
+    pub fn post_consensus_signature_count<F>(&self, is_in_committee: F) -> usize


Is that the right place for this function?

It might be better as validator‑store logic (or a helper) rather than in the shared ssv_types crate

diegomrsantos · 2026-02-05T19:27:44Z

anchor/validator_store/src/lib.rs

+
+    /// Boole+ path for `produce_signed_aggregate_and_proof`: committee-based consensus using
+    /// `AggregatorCommitteeConsensusData`.
+    #[allow(clippy::too_many_arguments)]


Ideally, we should avoid using this

dope suggestions! assigned myself a ticket to look into all this so that your suggestions don't fall through the cracks #807

diegomrsantos · 2026-02-10T03:04:10Z

anchor/validator_store/src/lib.rs

+        let completed = self
+            .qbft_manager
+            .decide_instance(
+                AggregatorCommitteeInstanceId {


Question: Is the shared QBFT instance between attestation aggregation and sync contribution intentional?

Both produce_signed_aggregate_and_proof_boole (line 734) and produce_signed_contribution_and_proof_boole (here) call decide_instance with the same AggregatorCommitteeInstanceId { committee, instance_height: slot }.

Since QbftManager::get_or_spawn_instance deduplicates by instance ID, only the first caller's Config/validator/start_data actually create the QBFT instance — the second caller silently joins the existing instance via the QbftInstance::Initialized branch and receives the same decided result.

This means:

Whichever of the two methods runs first (attestation agg or sync contribution) creates the instance; the other joins.

The second caller's timeout_mode, AggregatorCommitteeDataValidator, and our_consensus_data are silently discarded (only an on_completed callback is registered).

If these two paths ever need different configurations (e.g., different timeout behavior), it would break subtly.

Could you confirm this is the intended design? And if so, would it be worth adding a comment at both call sites (and updating decide_instance's doc which says "brand new qbft instance") to make this shared-instance pattern explicit?

hey, yep, all that's correct. works pretty much like how attestations and sync votes duties are rolled up into a single CommitteeInstanceId. In this case, it's aggregated attestations and sync committee contributions that will share a single AggregatorCommitteeInstanceId. and agree that adding the documentation is a great idea. will knock that out

shane-moore force-pushed the create-aggregator-committee-consensus-data branch from bc215d5 to 30a6e87 Compare January 26, 2026 17:05

dknopik added the v2.0.0 The release shipping the next network upgrade label Feb 2, 2026

shane-moore force-pushed the create-aggregator-committee-consensus-data branch 5 times, most recently from 0aa42c7 to 9c5c507 Compare February 2, 2026 16:04

shane-moore requested review from diegomrsantos, dknopik and petarjuki7 February 2, 2026 16:17

dknopik reviewed Feb 2, 2026

View reviewed changes

shane-moore force-pushed the create-aggregator-committee-consensus-data branch from f54bcf1 to c91352f Compare February 2, 2026 23:24

dknopik reviewed Feb 3, 2026

View reviewed changes

shane-moore force-pushed the create-aggregator-committee-consensus-data branch from 81a67cc to d1d94f8 Compare February 3, 2026 16:32

shane-moore added 11 commits February 3, 2026 18:11

chore: updates per claude review

4d2b23b

refactor: metadata service to cover more of the slot

ebf57c2

chore: metadata service waits for attesters event from duties service

296d31f

chore: update lighthouse dependencies to include new duties service e…

4058b82

…vents

chore: skip updating attester and sync voting assignments if error fr…

35fe1f0

…om the poll channels

chore: refactor polling of duties in metadata service

949d47b

chore: update lighthouse dependency

7b25302

refactor: remove duties lighthouse watch channel logic

6f9aff0

chore: update anchor to latest lighthouse package versions

b5c5f95

chore: updates per claude review

cbab55c

feat: agg committee pre consensus p2p and qbft validation

daec148

shane-moore added 11 commits February 3, 2026 18:11

chore: formatting

45f5c4e

chore: update anchor to latest lighthouse package versions

7f4a494

chore: updates per claude review

073d78a

feat: create agg committee consensus data

234815f

chore: produce aggregate attestations and sync contributions post-boo…

da70447

…le fork

fix: agg committee produce aggregates counting

2356d63

add logging for pre-consensus checks

a4202de

chore: linting

c5bae8a

refactor: produce aggregates helper functions

7899324

refactor: SyncAggregatorData in metadataservice from tuple to struct

07a2a3a

chore: update agg committee functions to support timeout_mode

e47fd0b

shane-moore force-pushed the create-aggregator-committee-consensus-data branch from f9d7148 to e47fd0b Compare February 3, 2026 23:58

refactor: use inc_counter_vec_by in agg attestation counts

81062ba

dknopik approved these changes Feb 4, 2026

View reviewed changes

dknopik added the ready-for-merge label Feb 4, 2026

mergify bot merged commit 589e6bc into sigp:unstable Feb 4, 2026
19 of 20 checks passed

diegomrsantos reviewed Feb 5, 2026

View reviewed changes

shane-moore mentioned this pull request Feb 5, 2026

metadata service refactor #807

Open

diegomrsantos reviewed Feb 10, 2026

View reviewed changes

shane-moore mentioned this pull request Feb 10, 2026

add agg committee consensus documentation #816

Open

Comments

Conversation

shane-moore commented Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue Addressed

Proposed Changes

Additional Info

Uh oh!

dknopik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shane-moore Feb 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shane-moore Feb 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dknopik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petarjuki7 commented Feb 3, 2026

Uh oh!

shane-moore commented Feb 3, 2026

Uh oh!

claude-code-actions-sigp bot commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review Complete ✓

Critical Issues (Must Fix Before Merge)

1. Missing Error Context - Production Debugging Risk

2. Potential Index Mismatch - Data Safety

3. Hard-coded Timeout Without Validation

Warnings (Should Fix)

4. Non-Deterministic Iteration Order - Consensus Risk

5. Potential Slot Race in Cache Access

6. Missing Committee Index Bounds Check

7. Expensive Clone on Critical Path

Suggestions (Nice to Have)

8. Inefficient Metrics Logging

9. Magic Numbers Without Constants

10. Missing Early Return Optimization

11. Error Messages Lack Context

Strengths ✨

Summary

Uh oh!

shane-moore commented Feb 3, 2026

Uh oh!

claude-code-actions-sigp bot commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Review Acknowledgment

Summary

Notes

Uh oh!

shane-moore commented Jan 26, 2026 •

edited

Loading

shane-moore Feb 2, 2026 •

edited

Loading

shane-moore Feb 2, 2026 •

edited

Loading

claude-code-actions-sigp bot commented Feb 3, 2026 •

edited

Loading

claude-code-actions-sigp bot commented Feb 3, 2026 •

edited

Loading

diegomrsantos Feb 5, 2026 •

edited

Loading