*: sync contributions per subcommittee#4602
Open
KaloyanTanev wants to merge 2 commits into
Open
Conversation
|
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a sync-committee aggregation correctness issue where validators that are aggregators for multiple sync subcommittees in the same slot would have their data collide when keyed only by (duty, pubkey), causing all but one subcommittee’s selection/contribution to be dropped. The PR extends the partial/aggregated signature keying to include a core.SubcommitteeIndex for sync-committee aggregator duties, and updates sync-contribution unsigned data to support multiple contributions per validator (version-gated for backwards compatibility).
Changes:
- Add
core.SubcommitteeIndexand thread it throughValidatorAPI,Fetcher,ParSigDB,AggSigDB, and wiring/tracing so sync-committee aggregator signatures are disambiguated per subcommittee. - Change
DutySyncContributionunsigned data to support pluralcore.SyncContributions(JSON array), while remaining tolerant of the legacy singlecore.SyncContributionwire form. - Add info-sync based gating (>=
v1.11) to only emit the plural encoding once a threshold of peers supports it; expand tests to cover both encodings and multi-subcommittee behavior.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| app/app.go | Wires infosync into fetcher to gate plural SyncContributions encoding when available. |
| core/aggsigdb/memory.go | Keys aggregated signatures by (duty, pubkey, subcommIdx) and updates Await API accordingly. |
| core/aggsigdb/memory_internal_test.go | Updates AggSigDB tests for the new Await signature (subcommittee index). |
| core/aggsigdb/memory_test.go | Updates AggSigDB tests for the new Await signature (subcommittee index). |
| core/aggsigdb/memory_v2.go | Updates MemDBV2 to store/await aggregated signatures with subcommittee keying. |
| core/aggsigdb/memory_v2_internal_test.go | Updates MemDBV2 internal tests for the new Await signature (subcommittee index). |
| core/dutydb/memory.go | Stores plural/single sync contributions, persisting each contribution entry keyed by slot/subcommittee/root. |
| core/dutydb/memory_test.go | Updates sync contribution DutyDB tests for the plural unsigned-data form and revised error text. |
| core/fetcher/fetcher.go | Fetches multiple per-subcommittee contributions per validator, with version-gated plural vs legacy single encoding. |
| core/fetcher/fetcher_test.go | Expands fetcher tests to cover plural encoding and a backwards-compatible single-contribution mode. |
| core/infosync/infosync.go | Adds quorum-based gating signal (SyncContributionsSupported) driven by cluster-agreed versions. |
| core/infosync/infosync_internal_test.go | Adds tests for version gating logic and slot-based enabled/disabled transitions. |
| core/interfaces.go | Extends AggSigDB / Fetcher / ValidatorAPI interfaces to include SubcommitteeIndex for sync duties. |
| core/parsigdb/memory.go | Keys partial signatures by (duty, pubkey, subcommIdx) using signed payload to derive the subcommittee index. |
| core/proto_test.go | Updates proto round-trip tests and adds compatibility coverage for plural vs single sync contribution forms. |
| core/signeddata.go | Adds SyncSubcommitteeIndex helper and IsSyncSubcommitteeDuty support to derive subcommittee keying from signed payloads. |
| core/signeddata_test.go | Adds unit test coverage for SyncSubcommitteeIndex. |
| core/ssz_test.go | Updates unsigned proto marshalling test to use SyncContributions for DutySyncContribution. |
| core/tracing.go | Threads SubcommitteeIndex through tracing wrapper for AggSigDB Await. |
| core/types.go | Introduces SubcommitteeIndex type and IsSyncSubcommitteeDuty helper. |
| core/unsigneddata.go | Introduces SyncContributions unsigned type and makes DutySyncContribution decoder accept plural or legacy single forms. |
| core/validatorapi/validatorapi.go | Groups sync selections/contributions by (slot, subcommittee) to avoid pubkey collisions and queries AggSigDB with subcommittee index. |
| core/validatorapi/validatorapi_test.go | Updates validator API tests to validate per-subcommittee behavior and new AggSigDB await signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+562
to
+572
| commSize, ok := eth2Resp.Data["SYNC_COMMITTEE_SIZE"].(uint64) | ||
| if !ok { | ||
| return 0, errors.New("invalid SYNC_COMMITTEE_SIZE") | ||
| } | ||
|
|
||
| subnetCount, ok := eth2Resp.Data["SYNC_COMMITTEE_SUBNET_COUNT"].(uint64) | ||
| if !ok || subnetCount == 0 { | ||
| return 0, errors.New("invalid SYNC_COMMITTEE_SUBNET_COUNT") | ||
| } | ||
|
|
||
| return commSize / subnetCount, nil |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4602 +/- ##
==========================================
+ Coverage 57.58% 57.63% +0.04%
==========================================
Files 246 246
Lines 33675 33811 +136
==========================================
+ Hits 19392 19486 +94
- Misses 11845 11873 +28
- Partials 2438 2452 +14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix distributed validators only aggregating one sync committee subcommittee per slot. A validator in multiple subcommittees produces a distinct selection and contribution-and-proof per subcommittee, but these were keyed by
(duty, pubkey)alone and collided, dropping all but one.Add
core.SubcommitteeIndexto the partial-signature key (ParSigDB,AggSigDB,ValidatorAPI) and make theDutySyncContributionunsigned data a pluralSyncContributions, one per subcommittee.Since older nodes can't decode the plural form, it is version-gated via info-sync: the fetcher emits it only once a threshold of peers advertise >= v1.11, else falls back to the single-contribution wire format.
category: bug
ticket: none