fix(validator_store): sign the decided AggregatorCommittee worklist once per slot - #1229
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## unstable #1229 +/- ##
============================================
+ Coverage 65.42% 66.64% +1.21%
============================================
Files 164 166 +2
Lines 28075 28798 +723
============================================
+ Hits 18367 19191 +824
+ Misses 9708 9607 -101
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:
|
…nce per slot Both Boole post-consensus callbacks sized the committee partial-signature batch from the full decided value but each signed only its own object class, so any decided entry whose class the local Lighthouse did not request left the batch permanently under-filled and the operator's single post-consensus message was never sent. The whole duty then failed cluster-wide. Drive the signing from the decided value instead. The slot pipeline starts one execution per committee when it publishes that value at 2/3 slot, so exactly one message per (committee, slot) holds by construction rather than by locking, and the two Lighthouse callbacks become lookups that filter the shared outcome for the roots they asked for. The batch size is the number of entries actually submitted, so it is always reachable by local filtering. This reuses the crate's existing signing pipeline rather than duplicating it: SigningRequest and resolve now key results by (validator index, signing root) so one validator can hold five distinct roots, and collect_prepared_signatures shares the drain helper with the new path. collect_signature is unchanged. Refs sigp#1227
02c334a to
444be35
Compare
|
@claude, please take a look |
|
Claude finished @shane-moore's task in 11m 38s —— View job Review: PR #1229
The core fix is well-reasoned and the diagnosis is convincing: deriving the signing worklist from the decided value (filtered only by local share/liquidation/metadata) and sizing the committee batch from what this operator actually submits directly removes the "shared batch sized from the full union, filled by only one class" bug. Keying Two points, both non-blocking: 1. 2. Stale field doc ( Test coverage is strong for the module (both callback orders, dropped-future, five-root validator, missing/duplicate/conflicting/slot-mismatched decided entries). Neither point above is exercised by a test; #1 in particular would only surface at runtime. I did not run |
![claude-code-actions-sigp[bot]](https://avatars.githubusercontent.com/in/2108028?s=60&v=4){kind=small}
| for (&committee_id, decided_data) in &assignments.consensus_data_by_ssv_committee { | ||
| let store = Arc::clone(self); | ||
| let decided_data = Arc::clone(decided_data); | ||
| let execution = self.spawn_shared("aggregator_post_consensus", async move { | ||
| store | ||
| .run_aggregator_post_consensus(committee_id, slot, decided_data) | ||
| .await | ||
| }); | ||
| executions.insert((committee_id, slot), execution); | ||
| } |
There was a problem hiding this comment.
The "exactly one committee message per (committee, slot) by construction" invariant leans on this being the sole writer and on each (committee, slot) being written once. insert replaces any existing execution, so a second publish for the same slot (Phase 3 firing twice under clock catch-up/drift, a re-publish, etc.) spawns a fresh QBFT round and a second full set of detached signing tasks — the old tasks are already detached via spawn_handle and keep running, so you'd get two committee messages, defeating the "by construction" guarantee.
If a duplicate publish for the same slot is genuinely impossible, worth a one-line comment saying so. Otherwise consider making the registration idempotent so a re-publish is a no-op:
| for (&committee_id, decided_data) in &assignments.consensus_data_by_ssv_committee { | |
| let store = Arc::clone(self); | |
| let decided_data = Arc::clone(decided_data); | |
| let execution = self.spawn_shared("aggregator_post_consensus", async move { | |
| store | |
| .run_aggregator_post_consensus(committee_id, slot, decided_data) | |
| .await | |
| }); | |
| executions.insert((committee_id, slot), execution); | |
| } | |
| for (&committee_id, decided_data) in &assignments.consensus_data_by_ssv_committee { | |
| if let Entry::Vacant(slot_entry) = executions.entry((committee_id, slot)) { | |
| let store = Arc::clone(self); | |
| let decided_data = Arc::clone(decided_data); | |
| let execution = self.spawn_shared("aggregator_post_consensus", async move { | |
| store | |
| .run_aggregator_post_consensus(committee_id, slot, decided_data) | |
| .await | |
| }); | |
| slot_entry.insert(execution); | |
| } | |
| } |
| /// Once-only post-consensus signing executions for `AggregatorCommittee` duties (Boole+). | ||
| /// | ||
| /// One entry per `(committee, slot)`. The first Lighthouse callback (aggregate or | ||
| /// contribution class) spawns the execution as a detached task; concurrent and later | ||
| /// callbacks of either class join the same `Shared` future, so the complete decided | ||
| /// worklist is signed and batched exactly once no matter which callbacks fire, in what | ||
| /// order, or whether their futures are dropped. |
There was a problem hiding this comment.
This doc still describes the old callback-triggered design ("The first Lighthouse callback ... spawns the execution as a detached task; concurrent and later callbacks of either class join the same Shared future"). Per the PR itself, executions are now started deterministically in update_aggregation_assignments before the watch publish, and callbacks only look up results — they never spawn. Suggest updating so the invariant readers rely on matches the code:
| /// Once-only post-consensus signing executions for `AggregatorCommittee` duties (Boole+). | |
| /// | |
| /// One entry per `(committee, slot)`. The first Lighthouse callback (aggregate or | |
| /// contribution class) spawns the execution as a detached task; concurrent and later | |
| /// callbacks of either class join the same `Shared` future, so the complete decided | |
| /// worklist is signed and batched exactly once no matter which callbacks fire, in what | |
| /// order, or whether their futures are dropped. | |
| /// Once-only post-consensus signing executions for `AggregatorCommittee` duties (Boole+). | |
| /// | |
| /// One entry per `(committee, slot)`, started by `start_aggregator_post_consensus` from | |
| /// `update_aggregation_assignments` before the assignments are published. Lighthouse | |
| /// callbacks only look results up via the shared future; they never spawn work, so the | |
| /// complete decided worklist is signed and batched exactly once no matter which callbacks | |
| /// fire, in what order, or whether their futures are dropped. |
Registering an AggregatorCommittee execution overwrote any existing entry for the same (committee, slot). The slot pipeline publishes once per slot today, so this was not reachable, but it moved the exactly-once guarantee out of this module and into another module's timing loop. Overwriting would spawn a second QBFT round and a second set of detached signing tasks while the first set kept running, putting two post-consensus messages on the wire for one slot, which peers reject with a gossip penalty. Register only into a vacant entry so exactly-once holds here regardless of how often the pipeline publishes, and correct the field doc, which still described the superseded callback-triggered design. Raised in review of sigp#1229.
|
@claude, both points addressed in 09b0210. 1. Idempotent registration. Fixed. I traced the Phase 3 loop (`metadata_service.rs:334-352`): it sleeps `duration_to_next_slot + 2/3 slot` each iteration, so a same-slot double publish is not reachable today, and an overrun skips a slot rather than repeating one. But the objection holds regardless of reachability: Worth noting this was a regression I introduced: the earlier callback-triggered version of this code used 2. Stale field doc. Fixed, it now describes the pipeline-triggered design and states that callbacks only read the map. On the missing test coverage, which was the most useful part of the review: added |
|
Claude finished @shane-moore's task in 1m 8s —— View job Re-review: PR #1229 (commit 09b0210)
Both points are resolved correctly. Verified against the commit: 1. Idempotent registration — 2. Stale field doc — 3. Test coverage — Note: I confirmed all three by source inspection; I was not able to execute No further concerns. LGTM. |
|
Queued — the merge queue status continues in this comment ↓. |
Merge Queue Status
This pull request spent 30 minutes 59 seconds in the queue, including 28 minutes 34 seconds running CI. Required conditions to merge
|
3 participants
Problem, Evidence, and Context (Required)
At Boole, one QBFT decision can carry both attestation aggregates and sync contributions. Anchor
signed these from two separate Lighthouse callbacks. Each callback signed only its own object type.
But both sized the shared partial-signature batch from the full decided value.
If the decided value held an object type that the local Lighthouse never asked for, the batch never
filled. The operator then never sent its one post-consensus message. The duty failed for the whole
cluster, not just for that operator.
We first saw this on a mixed Anchor and go-ssv network. On ssv-mini it breaks about 1.2% of
post-fork
AggregatorCommitteeduties, so it is a steady loss on any mixed network.Closes #1227.
Change Overview (Required)
Anchor now builds the signing list from the decided value instead of from what Lighthouse asked
for. Local state (share, cluster status, metadata) only filters that list.
The slot pipeline starts one signing job per committee when it publishes the decided value. This is
the only place that starts a job, so one committee sends exactly one message per slot. The two
callbacks now only read the result.
The batch size is the number of entries Anchor actually submits, so local filtering can always
reach it.
Read
aggregator_post_consensus.rsfirst, then the trigger inupdate_aggregation_assignments,then the two callbacks.
The signature collector is untouched, and
collect_signatureis byte-identical to before, sopre-Boole paths cannot change.
Risks, Trade-offs, and Mitigations (Required)
Two behavior changes to be aware of:
AggregatorCommitteenow starts at 2/3 slot from the pipeline. Before, it started whenthe first callback arrived.
OTHER_ERRORinstead ofTIMEOUT. This matches the existingBeaconVote path, and it is visible on dashboards.
Blast radius is
validator_storeplus one deleted helper inssv_types.Anchor signs every decided entry that it holds a share for. go-ssv also filters by local duty and
signs fewer. Signing extra roots is safe here, because peers do not check completeness for this
duty, and one validator stays inside the 5-root cap.
Validation (Required)
66 unit tests, plus
make lint,make cargo-fmt-check, and a full releasemake test, all clean.Live A/B on ssv-mini with 2 Anchor and 2 go-ssv operators at a 3-of-4 threshold. Everything was
identical between the two arms except the Anchor build.
AggregatorCommitteedutiesEvery one of the 8 failures looks the same. Only the two go-ssv operators sent a post-consensus
message. Both Anchor operators sent nothing, which left 2 of 4 partials against a threshold of 3.
On this branch that never happened once.
We also restarted an operator after the fork, because this PR keeps the signing jobs in memory. The
node misses 4 slots and then recovers on its own, with no stuck batch.
Rollback (Required for behavior or runtime changes; optional otherwise)
Revert the commits. There are no config, CLI, database, or wire-format changes, and nothing new is
written to disk.
Blockers / Dependencies (Optional)
None.
🤖 Generated with Claude Code