simplify aggregated payload(s) processing from block or from gossip#625
Merged
simplify aggregated payload(s) processing from block or from gossip#625
Conversation
Consolidate the two separate paths for storing aggregated attestation payloads into a single forkchoice.storeAggregatedPayload function. Previously, block attestations called storeAggregatedPayload (single validator_id, stored in latest_known_aggregated_payloads) while gossip attestations called onGossipAggregatedAttestation (stored in latest_new_aggregated_payloads with attestation tracker updates). Now storeAggregatedPayload accepts a validator_ids slice and an is_from_block bool. When is_from_block is true, payloads go into latest_known_aggregated_payloads; when false, they go into latest_new_aggregated_payloads and attestation trackers are updated (previously done by the removed onGossipAggregatedAttestation). chain.storeAggregatedPayloads now collects all validator ids per attestation and calls the unified function with is_from_block=true. chain.onGossipAggregatedAttestation now extracts validator ids and calls the unified function with is_from_block=false directly, removing the delegation to forkchoice.onGossipAggregatedAttestation. Remove forkchoice.onGossipAggregatedAttestation and its unlocked helper as they are no longer needed. https://claude.ai/code/session_01Wc3TmWgYtBTY6wmyLubq6R
Remove the storeAggregatedPayloads helper and store each attestation's aggregated payload directly inside the existing aggregated_attestations loop in onBlock, reusing the already-computed signature_proof and validator_indices. https://claude.ai/code/session_01Wc3TmWgYtBTY6wmyLubq6R
…station, always store signature Rename forkchoice's onGossipAttestation/onGossipAttestationUnlocked to onSignedAttestation/onSignedAttestationUnlocked and remove the store_signature bool parameter since signatures are always stored. Update all call sites in chain.zig, forkchoice.zig tests, and spectest/fork_choice_runner.zig accordingly. https://claude.ai/code/session_01Wc3TmWgYtBTY6wmyLubq6R
g11tech
changed the title
added 2 commits
March 4, 2026 14:10
anshalshukla
requested changes
Mar 4, 2026
| &signature_groups[index] | ||
| else | ||
| null; | ||
| // Get participant indices from the signature proof, length already validated |
Collaborator
There was a problem hiding this comment.
We have validated the length but we are not rejecting the block if there is a length mismatch, we should reject the blocks if the aggregated_attestations length don't match with signatures_groups
Member
There was a problem hiding this comment.
wouldn't verifyaignatures take care of it?
|
|
||
| // Internal unlocked version - assumes caller holds lock | ||
| fn onGossipAttestationUnlocked(self: *Self, signed_attestation: types.SignedAttestation, store_signature: bool) !void { | ||
| fn onSignedAttestationUnlocked(self: *Self, signed_attestation: types.SignedAttestation) !void { |
Collaborator
There was a problem hiding this comment.
I think we should have store_signature, and properly set is unlike hardcoded true like we were passing before. If the node is not aggregator, what purpose would gossip_signatures map serve? I think it only makes sense for aggregator nodes since only they can aggregate it.
Member
There was a problem hiding this comment.
you only subscribe to the subnet to listen to if you are an aggregator, can you verify if this is the case, if not create an issue for the same
g11tech
approved these changes
Mar 4, 2026
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
4 participants
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.
Summary
Refactored the aggregated signature proof storage mechanism to accept multiple validators at once, eliminating redundant per-validator operations and simplifying the API.
Key Changes
storeAggregatedPayloadAPI: Changed from storing proofs for a single validator to accepting a slice of validator IDs, reducing code duplication across call sitesis_from_blockparameter to distinguish between:latest_known_aggregated_payloads(immediately available)latest_new_aggregated_payloads(promoted via periodic ticks)onGossipAggregatedAttestationmethod: Consolidated gossip attestation handling into the unifiedstoreAggregatedPayloadfunctionchain.zig,fork_choice_runner.zig,stageAggregatedAttestation) to build validator ID arrays and call the unified methodImplementation Details
!is_from_block) to avoid unnecessary fork choice updates for block-sourced proofssignatures_mutexis always held for proof storage, whilemutexis only locked for gossip attestationshttps://claude.ai/code/session_01Wc3TmWgYtBTY6wmyLubq6R