fix: batch pre-Boole post-consensus sync contributions

[shane-moore]

Problem, Evidence, and Context (Required)

• Before Boole, QBFT can decide multiple sync contributions for one validator, but Anchor publishes each post-consensus callback as a single-root type-0 envelope. go-ssv expects the complete decided root multiset and rejects these envelopes with wrong expected roots count.
• A prior mixed-client reproduction recorded 626 type-0 wrong-root-count rejections across 210 multi-root duties.
• fix: batch pre-Boole sync contribution proofs #1201 fixed the analogous type-3 producer path and provides the batch reused here.

Change Overview (Required)

• Prepare a canonical descriptor from the complete decided Contributions, sorted by subnet and root with identical entries retained as multiplicity.
• Reuse the eager Pending/Admitted single-validator batch for post-consensus messages, isolated from type 3 by phase and keyed by exact signing root.
• Keep the single-root path unchanged. Boole, wire types, inbound validation, and Lighthouse APIs are unchanged.
• Review validator_store preparation first, then signature_collector admission and retry behavior.

Risks, Trade-offs, and Mitigations (Required)

• The main risk is concurrent sibling callbacks publishing incomplete or duplicate envelopes. Focused tests cover admission, retry, injection, cleanup, stale work, impostor mode, and isolation boundaries.

Validation (Required)

• 30-minute mixed run with three Anchor nodes and one go-ssv node: 1,035 type-0 duties, including 195 multi-root duties and exact two-, three-, and four-root envelopes; zero root-count, root, signature, descriptor, quorum, restart, or OOM failures; type 3 remained unchanged.
• After fix: batch pre-Boole sync contribution proofs #1201 merged, this branch was replayed as one commit on current unstable. The only conflicts were in validator_store test scaffolding added by feat(validator_store): add configurable proposer delay for MEV bids #1213, so the harness constructor now takes both a proposer delay and a mock consensus decider. git range-diff confirms the production diff is unchanged from the validated candidate.
• On the replayed commit: git diff --check, make cargo-fmt-check, make lint, and release tests covering 56 validator_store, 33 signature_collector, and 57 message_validator cases.

Rollback (Required for behavior or runtime changes; optional otherwise)

• Revert this commit. There are no config, data, API, or wire-format migrations.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 99.11700% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.54%. Comparing base (82bac14) to head (21746a0).

Files with missing lines	Patch %	Lines
anchor/validator_store/src/lib.rs	97.33%	2 Missing ⚠️
...r/validator_store/src/testing/sync_contribution.rs	99.27%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #1208      +/-   ##
============================================
+ Coverage     65.42%   66.54%   +1.12%     
============================================
  Files           164      165       +1     
  Lines         28075    28450     +375     
============================================
+ Hits          18367    18933     +566     
+ Misses         9708     9517     -191     

Flag	Coverage Δ
rust	66.54% <99.11%> (+1.12%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[shane-moore]

@claude, please take a look

[claude-code-actions-sigp]

Claude finished @shane-moore's task in 12m 33s —— View job

---

Review: fix: batch pre-Boole post-consensus sync contributions

• Read the PR diff against origin/unstable
• Review validator_store contribution preparation
• Review signature_collector admission/retry behavior
• Review tests and edge cases

I reviewed the full production diff and the tests. This is a clean, well-structured change that reuses the type-3 batch machinery from #1201 for the type-0 (post-consensus) path. I found no blocking issues. Notes below.

What holds up well

• Phase isolation is correct. Keying single_validator_batches by (SingleValidatorBatchPhase, Slot, PublicKeyBytes) (signature_collector/src/lib.rs:150, :464) cleanly separates concurrent type-3 (ContributionProofs) and type-0 (PostConsensus) batches for the same validator/slot. contribution_proof_and_post_consensus_records_are_isolated covers this.
• Root-keyed dedup + multiplicity is sound. The injection loop (:546-559) dedups by signing_root and expands via repeat_n(message, multiplicity). Since a type-0 signing root is ContributionAndProof::signing_root, which encodes subcommittee_index, distinct subnets can never collapse to one root — so dropping the subnet_id check in the descriptor guard (:447) is safe and is what enables the "two distinct roots on one subnet" case.
• Concurrency admission is single-winner. The state mutex ensures exactly one sibling admits the full expanded envelope and the rest inject only their own root; concurrent_post_consensus_callbacks_produce_one_admitted_send plus the single_validator_batch_before_state_lock test hook exercise the race directly.
• Descriptor consistency across siblings. Sibling callbacks share one QBFT instance (same ProposerInstanceId height), so each independently-computed descriptor from the decided Contributions is byte-identical, satisfying the record.descriptor != descriptor guard (:507).
• Canonicalization is deterministic. prepare_decided_sync_contributions sorts by (subnet_id, signing_root) then collapses identical adjacent entries into multiplicity, giving retry-stable, order-independent envelopes. The permutation cases in decided_contribution_preparation_cases confirm this.
• sync_committee_collection_mode shared helper correctly falls back to SingleValidator when total multiplicity is 1, keeping the single-root path byte-for-byte unchanged for both callers.

Observations (non-blocking)

1. Lock held across sign_and_send (:585-587). The state guard is held during message_sender.sign_and_send. This is safe because the network sender's sign_and_send uses a non-blocking try_send (message_sender/src/network.rs:66), consistent with the SingleValidatorBatchRecord::state doc and the existing type-3 path. Worth keeping the doc comment's "admission-only, never await/blocking-send" invariant in mind for any future MessageSender impl.

2. Callback message selection is unchanged pre-existing behavior. prepare_decided_sync_contributions picks the callback message as the first decided entry matching callback_subnet (validator_store/src/lib.rs:2286). This mirrors the prior find(...subcommittee_index == ...), so it's not a regression. If two decided contributions ever shared one subnet with different block roots (a fork edge case), both same-subnet callbacks would resolve to the same message — but that matched the old behavior too, so out of scope here.

3. Test coverage is thorough — envelope shapes, retry after failed admission, construction failure retaining Pending, impostor empty-signature mode, stale-slot suppression, cross-phase isolation, and cleanup across both phases are all covered, including shared run_* bodies parameterized over both PartialSignatureKind values.

I was unable to run make lint / make test in this environment (the required cargo/git fetch commands need approval here). The PR description reports fmt/lint/tests passing on the replayed commit plus a 30-minute mixed go-ssv run with zero root-count failures; the diff is consistent with those claims.
• fix/pre-boole-post-consensus-sync-contribution-batching

[shane-moore]

@claude Verified your observations against the branch. Two corrections and one addition, no code changes from this review.

1. Observation 1's conclusion is right but the citation is wrong. message_sender/src/network.rs:66 is inside the closure that runs later on the processor. The non-blocking primitive is processor/src/senders.rs:66-68, where send_work_item calls tx.try_send, reached from network.rs:68-69. So under the record lock sign_and_send only enqueues, and a full queue returns Err(Error::Processor), which leaves the record Pending for a sibling callback to retry. The impostor sender performs no I/O at all.

2. The identical-descriptor property across siblings is structural rather than incidental. Sync contribution consensus runs one instance per validator and slot, ProposerInstanceId { validator, duty: SyncCommitteeAggregator, instance_height: slot }, so every subnet callback decodes the same decided value before building its descriptor.

3. Confirming the root-only descriptor guard in code, not just by argument: type 0 roots cover contribution.subcommittee_index through ContributionAndProof, and type 3 roots cover SyncAggregatorSelectionData { slot, subcommittee_index } at validator_store/src/lib.rs:557-565. Root identity is therefore injective per subnet in both phases.

[claude-code-actions-sigp]

Claude finished @shane-moore's task in 1m 6s —— View job

---

Re: verification notes

All three points check out against the branch. Confirmed each in code:

1. sign_and_send non-blocking path — corrected citation accepted.
message_sender/src/network.rs:66 is let sender = self.clone();, setup for the closure whose body (sign → SignedSSVMessage::new → do_send) runs later on the processor — not the send primitive. The actual enqueue is send_blocking → send_work_item at processor/src/senders.rs:68 (self.tx.try_send(item)), and the .map_err(Error::Processor) at network.rs:97 means a full queue returns Err(Error::Processor) synchronously, leaving the record Pending for a sibling to retry. So the "admission-only, never blocking-send" invariant holds under the record lock. Thanks for the precise pointer — my original :66 reference was to the wrong line.

Impostor path confirmed no-I/O: ImpostorMessageSender::sign_and_send (message_sender/src/impostor.rs:31-45) computes a subnet, emits a debug!, and returns Ok(()). No signing, no queue send.

2. Identical-descriptor property is structural — agreed.
ProposerInstanceId { validator, duty, instance_height } (qbft_manager/src/lib.rs:79-85) with duty = ValidatorDutyKind::SyncCommitteeAggregator (:93) means one consensus instance per validator per slot. Every subnet callback decodes the same decided value before building its descriptor, so byte-identical descriptors are guaranteed by construction rather than incidental. Stronger framing than my "share one QBFT instance" note — accepted.

3. Root injectivity per subnet confirmed in code.
Type 3 roots are SyncAggregatorSelectionData { slot, subcommittee_index } at validator_store/src/lib.rs:557-565 (compute_sync_selection_root). Type 0 roots cover subcommittee_index via ContributionAndProof. Both encode the subnet into the root, so root identity is injective per subnet in each phase — the root-only descriptor guard is safe without a separate subnet_id check.

No code changes requested or made. My original review's conclusions stand with these three refinements.
• fix/pre-boole-post-consensus-sync-contribution-batching