Skip to content

Conversation

@olegshmuelov
Copy link
Contributor

@olegshmuelov olegshmuelov commented Aug 4, 2025

ssvlabs/SIPs#61 implementation

@GalRogozinski
Copy link
Contributor

So far looks good @olegshmuelov

@github-actions
Copy link

github-actions bot commented Oct 9, 2025

This pull request has been marked as stale due to 60 days of inactivity.

@github-actions github-actions bot added the stale label Oct 9, 2025
# Conflicts:
#	ssv/committee.go
#	ssv/json_testutils.go
#	ssv/spectest/all_tests.go
#	ssv/spectest/generate/tests/partialsigcontainer.PartialSigContainerTest_PartialSigContainer_duplicate.json
#	ssv/spectest/generate/tests/partialsigcontainer.PartialSigContainerTest_PartialSigContainer_duplicate_quorum.json
#	ssv/spectest/generate/tests/partialsigcontainer.PartialSigContainerTest_PartialSigContainer_invalid.json
#	ssv/spectest/generate/tests/partialsigcontainer.PartialSigContainerTest_PartialSigContainer_one_signature.json
#	ssv/spectest/generate/tests/partialsigcontainer.PartialSigContainerTest_PartialSigContainer_quorum.json
#	ssv/spectest/generate/tests/tests.MultiMsgProcessingSpecTest_wrong_duty_pubkey.json
#	ssv/spectest/generate/tests/tests.MultiMsgProcessingSpecTest_wrong_duty_role.json
#	ssv/spectest/generate/tests/valcheck.MultiSpecTest_blinded_blocks.json
#	ssv/spectest/generate/tests/valcheck.SpecTest_attestation_valid_with_non_slashable_slot.json
#	ssv/spectest/generate/tests/valcheck.SpecTest_attestation_value_check_source_higher_than_target.json
#	ssv/spectest/generate/tests/valcheck.SpecTest_attestation_value_check_valid.json
#	ssv/spectest/generate/tests/valcheck.SpecTest_attestation_value_check_with_slashable_majority.json
#	ssv/spectest/generate/tests/valcheck.SpecTest_attestation_value_check_with_slashable_minority.json
#	ssv/spectest/generate/tests/valcheck.SpecTest_consensus_data_value_check_nil.json
#	types/consensus_data.go
#	types/consensus_data_encoding.go
#	types/generate.go
#	types/partial_sig_message_encoding.go
#	types/testingutils/beacon_node_aggregator.go
Copilot AI review requested due to automatic review settings November 7, 2025 13:57
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@github-actions github-actions bot removed the stale label Nov 8, 2025
# Conflicts:
#	ssv/spectest/tests/valcheck/valcheckattestations/unmatched_source_root.go
#	ssv/spectest/tests/valcheck/valcheckattestations/unmatched_target_root.go
#	ssv/value_check.go
@nkryuchkov
Copy link
Contributor

@greptileai please review it

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 2, 2025

Greptile Overview

Greptile Summary

This PR implements the Aggregator Committee feature, which consolidates aggregator and sync committee contribution duties into a single runner to optimize performance and reduce consensus rounds.

Key Changes

  • New AggregatorCommitteeRunner: Combines aggregator and sync committee contribution duties in a single runner that processes both types in parallel
  • New consensus data type: AggregatorCommitteeConsensusData stores both aggregator attestations and sync committee contributions
  • Committee support: Updated Committee struct to handle the new AggregatorCommitteeDuty type
  • Validation logic: Added specific validation for aggregator committee consensus data
  • Submission tracking: Proper tracking for both duty types with different granularity (aggregator: one per validator, sync committee: multiple per validator)

Implementation Quality

The implementation follows existing patterns in the codebase and includes comprehensive test coverage. The code properly handles edge cases like operators with divergent validator sets and validates consensus data before processing. The separation between aggregator duties (one submission per validator) and sync committee contributions (multiple submissions per validator by root) is correctly implemented.

Test Coverage

Extensive test files added including multi-committee spec tests with various validator counts (1, 2, 4 duties) and both attestation and sync committee scenarios. Test utilities properly generate consensus data for both pre-Electra and Electra+ versions.

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The implementation follows established patterns in the codebase, includes comprehensive test coverage, and properly handles edge cases. The code is well-structured with clear separation of concerns and appropriate error handling throughout.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
ssv/aggregator_committee.go 4/5 New aggregator committee runner that combines aggregator and sync committee contribution duties. Core logic appears sound with proper state management and submission tracking.
ssv/committee.go 5/5 Updated to support AggregatorCommitteeDuty type alongside existing CommitteeDuty. Logic correctly filters validators and creates runners.
types/consensus_data.go 5/5 Added AggregatorCommitteeConsensusData type with proper validation and encoding. Supports both aggregator and sync committee contribution data.
ssv/value_check.go 5/5 Added AggregatorCommitteeValueCheckF for validating aggregator committee consensus data. Simple validation ensures data is not empty.

Sequence Diagram

sequenceDiagram
    participant Committee
    participant AggregatorCommitteeRunner
    participant BeaconNode
    participant Network
    participant QBFT

    Committee->>AggregatorCommitteeRunner: StartNewDuty(duty, quorum)
    AggregatorCommitteeRunner->>AggregatorCommitteeRunner: Initialize submission tracking
    
    Note over AggregatorCommitteeRunner: Pre-Consensus Phase
    AggregatorCommitteeRunner->>AggregatorCommitteeRunner: executeDuty()
    loop For each validator duty
        alt Aggregator duty
            AggregatorCommitteeRunner->>AggregatorCommitteeRunner: Sign slot (selection proof)
        else SyncCommittee duty
            loop For each subcommittee index
                AggregatorCommitteeRunner->>AggregatorCommitteeRunner: Sign SyncAggregatorSelectionData
            end
        end
    end
    AggregatorCommitteeRunner->>Network: Broadcast partial signatures
    
    AggregatorCommitteeRunner->>AggregatorCommitteeRunner: ProcessPreConsensus()
    AggregatorCommitteeRunner->>AggregatorCommitteeRunner: Reconstruct selection proof signatures
    alt Is Aggregator
        AggregatorCommitteeRunner->>BeaconNode: GetAggregateAttestation()
        BeaconNode-->>AggregatorCommitteeRunner: Attestation
        AggregatorCommitteeRunner->>AggregatorCommitteeRunner: Add to consensus data
    end
    alt Is SyncCommittee Aggregator
        AggregatorCommitteeRunner->>BeaconNode: GetSyncCommitteeContribution()
        BeaconNode-->>AggregatorCommitteeRunner: Contribution
        AggregatorCommitteeRunner->>AggregatorCommitteeRunner: Add to consensus data (dedupe by subnet)
    end
    
    Note over AggregatorCommitteeRunner: Consensus Phase
    AggregatorCommitteeRunner->>QBFT: Start consensus with AggregatorCommitteeConsensusData
    QBFT->>Network: Exchange consensus messages
    QBFT-->>AggregatorCommitteeRunner: Consensus decided
    
    Note over AggregatorCommitteeRunner: Post-Consensus Phase
    AggregatorCommitteeRunner->>AggregatorCommitteeRunner: ProcessConsensus()
    loop For each aggregator in consensus data
        AggregatorCommitteeRunner->>AggregatorCommitteeRunner: Sign AggregateAndProof
    end
    loop For each contributor in consensus data
        AggregatorCommitteeRunner->>AggregatorCommitteeRunner: Sign ContributionAndProof
    end
    AggregatorCommitteeRunner->>Network: Broadcast post-consensus partial signatures
    
    AggregatorCommitteeRunner->>AggregatorCommitteeRunner: ProcessPostConsensus()
    AggregatorCommitteeRunner->>AggregatorCommitteeRunner: Reconstruct final signatures
    loop For each duty with quorum
        alt Aggregator duty
            AggregatorCommitteeRunner->>BeaconNode: SubmitSignedAggregateSelectionProof()
            AggregatorCommitteeRunner->>AggregatorCommitteeRunner: RecordSubmission(aggregator)
        else SyncCommittee duty
            AggregatorCommitteeRunner->>BeaconNode: SubmitSignedContributionAndProof()
            AggregatorCommitteeRunner->>AggregatorCommitteeRunner: RecordSubmission(contribution)
        end
    end
    AggregatorCommitteeRunner->>AggregatorCommitteeRunner: Check if all duties submitted
    AggregatorCommitteeRunner->>AggregatorCommitteeRunner: Mark as Finished
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

68 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@nkryuchkov nkryuchkov requested a review from y0sher December 2, 2025 13:08
* remove aggregatorCommittee from BeaconRole

* remove aggregator and scc roles

* remove agg and scc from ValidatorConsensusData

* generate ssz encoding

* align testingutils to remove reference to agg and scc alone

* add agg committee consensus data tests (and remove agg and scc from validator consensus data tests)

* generate types JSON tests

* drop agg and scc runners; fix agg committee runner issue

* align testingutils for agg committee tests

* value check tests

* preconsensus tests

* post consensus tests

* duties tests

* runner construction tests

* consensus tests

* happy flow test

* dutyexe tests

* add test docs; fix msg processing test

* add all tests

* generate JSON tests

* drop weird json tests in unintended directory

* add mixed agg+scc pre-consensus tests

* generate JSON tests

* add error code

* increase number of Contributors

* add  post-consensus mixed agg committee tests

* generate JSON tests

* add agg committee duty validation; add psgi msg sorting;

* add sorting and duty validation tests

* generate JSON tests

* fixed remaining mixed tests

* generate JSON tests

* fix lint (remove unused functions)

* change AggregatorCommitteeConsensusData to reduce duplicated data overhead

* align tests

* generate JSON tests

* fix maximum ssz sizes

* avoid in-place sorting

* add test docs

* remove unused test docs

* fix maximum-size tests

* generate JSON tests

* fix test: duty with diff slots

* add max size test for aggCommCD

* add size tests for phase0 and electra attestations; fix ssz max size for attestation in AggCommCD

* fix lint issues

* fix test dir (no multiple duty)

* fix versions data

* maximum duty possible test

* fix lint

* apply suggestions (remove sorting feature; remove unused errors; use subnet for contribution)

* clarify validator sync committee index usage

* change subnetID computation to avoid errors

* generate JSON tests

* tests for: invalid quorum; invalid quorum then valid quorum;

* generate JSON tests

* generate JSON tests with new error numbers

* remove deprecated partial signature types

* generate JSON tests

* generate SSZ files
* remove aggregatorCommittee from BeaconRole

* remove aggregator and scc roles

* remove agg and scc from ValidatorConsensusData

* generate ssz encoding

* align testingutils to remove reference to agg and scc alone

* add agg committee consensus data tests (and remove agg and scc from validator consensus data tests)

* generate types JSON tests

* drop agg and scc runners; fix agg committee runner issue

* align testingutils for agg committee tests

* value check tests

* preconsensus tests

* post consensus tests

* duties tests

* runner construction tests

* consensus tests

* happy flow test

* dutyexe tests

* add test docs; fix msg processing test

* add all tests

* generate JSON tests

* drop weird json tests in unintended directory

* add mixed agg+scc pre-consensus tests

* generate JSON tests

* add error code

* increase number of Contributors

* add  post-consensus mixed agg committee tests

* generate JSON tests

* add agg committee duty validation; add psgi msg sorting;

* add sorting and duty validation tests

* generate JSON tests

* fixed remaining mixed tests

* generate JSON tests

* fix lint (remove unused functions)

* change AggregatorCommitteeConsensusData to reduce duplicated data overhead

* align tests

* generate JSON tests

* fix maximum ssz sizes

* avoid in-place sorting

* add test docs

* remove unused test docs

* fix maximum-size tests

* generate JSON tests

* fix test: duty with diff slots

* add max size test for aggCommCD

* add size tests for phase0 and electra attestations; fix ssz max size for attestation in AggCommCD

* fix lint issues

* fix test dir (no multiple duty)

* fix versions data

* maximum duty possible test

* fix lint

* apply suggestions (remove sorting feature; remove unused errors; use subnet for contribution)

* clarify validator sync committee index usage

* change subnetID computation to avoid errors

* generate JSON tests

* tests for: invalid quorum; invalid quorum then valid quorum;

* generate JSON tests

* generate JSON tests with new error numbers

* rename ValidatorConsensusData -> ProposerConsensusData

* align tests

* generate JSON tests

* fix renaming on merge
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 8, 2026

Too many files changed for review.

4 similar comments
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 8, 2026

Too many files changed for review.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 8, 2026

Too many files changed for review.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 8, 2026

Too many files changed for review.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 8, 2026

Too many files changed for review.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 8, 2026

Too many files changed for review.

* remove aggregatorCommittee from BeaconRole

* remove aggregator and scc roles

* remove agg and scc from ValidatorConsensusData

* generate ssz encoding

* align testingutils to remove reference to agg and scc alone

* add agg committee consensus data tests (and remove agg and scc from validator consensus data tests)

* generate types JSON tests

* drop agg and scc runners; fix agg committee runner issue

* align testingutils for agg committee tests

* value check tests

* preconsensus tests

* post consensus tests

* duties tests

* runner construction tests

* consensus tests

* happy flow test

* dutyexe tests

* add test docs; fix msg processing test

* add all tests

* generate JSON tests

* drop weird json tests in unintended directory

* add mixed agg+scc pre-consensus tests

* generate JSON tests

* add error code

* increase number of Contributors

* add  post-consensus mixed agg committee tests

* generate JSON tests

* add agg committee duty validation; add psgi msg sorting;

* add sorting and duty validation tests

* generate JSON tests

* fixed remaining mixed tests

* generate JSON tests

* fix lint (remove unused functions)

* change AggregatorCommitteeConsensusData to reduce duplicated data overhead

* align tests

* generate JSON tests

* fix maximum ssz sizes

* avoid in-place sorting

* add test docs

* remove unused test docs

* fix maximum-size tests

* generate JSON tests

* fix test: duty with diff slots

* add max size test for aggCommCD

* add size tests for phase0 and electra attestations; fix ssz max size for attestation in AggCommCD

* fix lint issues

* fix test dir (no multiple duty)

* fix versions data

* maximum duty possible test

* fix lint

* apply suggestions (remove sorting feature; remove unused errors; use subnet for contribution)

* clarify validator sync committee index usage

* change subnetID computation to avoid errors

* generate JSON tests

* tests for: invalid quorum; invalid quorum then valid quorum;

* generate JSON tests

* generate JSON tests with new error numbers

* remove deprecated partial signature types

* generate JSON tests

* generate SSZ files

* fix committee to have agg and comm runners

* align testing utils. Fix Committee constructor to a common one, and align tests execution

* add test for comm + agg comm duties in the same slot

* generate JSON tests

* remove unused function

* add tests for error cases in committee

* add test for mixed duties for multiple slots

* remove unused parameter
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 8, 2026

Too many files changed for review.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants