Skip to content
9 changes: 5 additions & 4 deletions protocol/v2/ssv/runner/aggregator_committee.go
Original file line number Diff line number Diff line change
Expand Up @@ -887,10 +887,11 @@ func (r *AggregatorCommitteeRunner) ProcessPostConsensus(
return fmt.Errorf("could not get expected post consensus roots and beacon objects: %w", err)
}
if len(beaconObjects) == 0 {
// Empty post-quorum (all beacon objects failed to build) is terminal and non-recoverable:
// committee_queue drops the message and terminates the runner on this error. Classify as
// failed (matching CommitteeRunner) rather than leaving the watcher to report a false stuck.
r.markDutyFailed(ErrNoValidDutiesToExecute)
// Benign terminal: consensus reached but this operator has nothing to submit (no aggregators
// or contributors assigned to it in the decided data). Conclude as not_required (matching
// CommitteeRunner) — neither a false "stuck" nor a spurious "failed". The sentinel still
// tells committee_queue to drop the message and terminate the runner.
r.markDutyNotRequired()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Finding 1 · [IMPORTANT] Don't reclassify AggregatorCommitteeRunner's empty-objects branch as benign — there it is an invariant violation, not a per-operator no-op

CommitteeRunner's "nothing to submit" rationale doesn't transfer to AggregatorCommitteeRunner. CommitteeRunner builds beaconObjects from LOCAL state (local DutyGuard, local construction), so divergent validator sets across operators genuinely produce an empty map on one operator only — benign. AggregatorCommitteeRunner instead builds beaconObjects purely from the DECIDED consensus data, byte-identical for every operator, surfacing every error rather than skipping any. So len(beaconObjects)==0 here can only happen if the decided value had zero Aggregators and zero Contributors — a state AggregatorCommitteeConsensusData.Validate() explicitly rejects, and validateDecidedConsensusData enforces before storing it as decided.

Reaching this branch therefore means the decided-value check was bypassed or regressed, QBFT decided on a value that never passed CheckValue, or there's a spec-level encode/decode mismatch — a consensus-integrity signal, not routine idle time. Before this PR it surfaced as a loud failure; after, it's a silent not_required + debug log. The node goes quiet on a state that should be structurally impossible.

Telling corroboration: the PR's own regression test for this branch can't reach it through the runner's normal flow — it hand-installs State.DecidedValue, bypassing CheckValue/Validate() to force the condition.

@momosh-ssv momosh-ssv Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in cba5938.
You're right, this one doesn't transfer: reverted the branch to failed with an explicit invariant-violation error (the sentinel stays wrapped so the queue still drops the message and terminates the runner), and the regression test now pins the failed outcome and documents why it has to hand-install the decided value.

return ErrNoValidDutiesToExecute
}

Expand Down
40 changes: 40 additions & 0 deletions protocol/v2/ssv/runner/aggregator_committee_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,46 @@ func TestAggregatorCommitteeRunnerProcessPostConsensus_MarksFailedOnSubmitError(
require.False(t, env.runner.State.Succeeded, "a failed duty must not be marked succeeded")
}

// TestAggregatorCommitteeRunnerProcessPostConsensus_MarksNotRequiredOnNoBeaconObjects is the
// regression test for #2903: a post-consensus quorum where the decided data leaves this operator
// with no beacon objects to submit is a benign terminal and must conclude not_required — not failed
// (the previous behavior, surfacing as a spurious "⚠️ duty failed") — while still surfacing the
// sentinel for the queue's terminal-drop handling. The decided value is swapped after consensus for
// one with no aggregators or contributors to model the empty-objects terminal.
func TestAggregatorCommitteeRunnerProcessPostConsensus_MarksNotRequiredOnNoBeaconObjects(t *testing.T) {
ctx := t.Context()
const version = spec.DataVersionElectra

base := protocoltesting.NewTestingBeaconNodeWrapped().(*protocoltesting.BeaconNodeWrapped)
env := newAggregatorCommitteeRunnerEnv(t, []int{1}, base)
duty := spectestingutils.TestingAggregatorCommitteeDutyForValidators([]int{1}, []int{}, version)

concluded := env.startAndFeedThroughConsensus(t, ctx, duty, version)

emptyDecided := &spectypes.AggregatorCommitteeConsensusData{Version: version}
encoded, err := emptyDecided.Encode()
require.NoError(t, err)
env.runner.State.DecidedValue = encoded

var postConsensusErr error
for _, psig := range postConsensusMsgsFromFixture(duty, env.keySetMap, version) {
if err := env.runner.ProcessPostConsensus(ctx, env.logger, psig); err != nil {
postConsensusErr = err
}
}

require.ErrorIs(t, postConsensusErr, ErrNoValidDutiesToExecute, "the benign sentinel must surface to the queue")

select {
case c := <-concluded:
require.Equal(t, dutyOutcomeNotRequired, c.outcome, "no beacon objects to submit must conclude not_required, not failed")
require.NoError(t, c.reason)
default:
t.Fatal("expected a not_required duty conclusion, got none")
}
require.True(t, env.runner.State.Succeeded, "not_required is a correct completion")
}

// TestAggregatorCommitteeRunnerProcessPostConsensus_DoesNotMarkFailedOnInvalidSigs asserts that the
// recoverable reconstruct-invalid-signatures case is NOT concluded failed: the root can later re-cross
// quorum on a subsequent message, so concluding here would mask a duty that still completes.
Expand Down
13 changes: 13 additions & 0 deletions protocol/v2/ssv/runner/committee.go
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,10 @@ listener:
)

if totalAttestations == 0 && totalSyncCommittee == 0 {
// Benign terminal: the committee decided but this operator ended up with zero valid duties to
// sign. Conclude as not_required so the watcher doesn't report a false "stuck"; the sentinel
// still tells committee_queue to drop the message and terminate the runner.
r.markDutyNotRequired()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Finding 2 · [MINOR] Guard the consensus-phase not_required against context cancellation (shutdown now records a benign outcome)

In ProcessConsensus, the zero-duties branch triggers not only when every duty is filtered by the DutyGuard, but also when the context is cancelled — neither the feeder nor the workers increment counters on cancellation, and nothing re-checks ctx.Err() before concluding not_required. A cancellation observed here means the parent context (node/committee shutdown) was cancelled.

This matters because the codebase deliberately treats cancellation as "not attempted", never as an outcome: markDutyFailed explicitly filters context.Canceled, with a comment noting a cancelled duty was abandoned, not attempted-and-failed. markDutyNotRequired has no equivalent filter, so a shutdown mid-signing now produces a positive duty.outcome=not_required data point claiming the duty completed correctly — even though whether it lands at all is non-deterministic (watchDutyOutcome picks pseudo-randomly between the conclusion and ctx.Done() when both fire together).

On a rolling restart across a large fleet, this shows up as a burst of spurious not_required outcomes for committee duties that were simply abandoned mid-flight — exactly the kind of noise this PR is trying to remove from the metric.

@momosh-ssv momosh-ssv Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in b0ef6a9.
The zero-counts branch now re-checks ctx.Err() and returns the cancellation without concluding any outcome, mirroring markDutyFailed's context.Canceled filter.

Added a cancelled-context regression test.

return ErrNoValidDutiesToExecute
}

Expand Down Expand Up @@ -514,6 +518,9 @@ func (r *CommitteeRunner) ProcessPostConsensus(ctx context.Context, logger *zap.
// are tagged recoverableReconstructError and must not be recorded as failed.
// Shutdown (context cancellation) needs no special-casing — markDutyFailed drops a context.Canceled
// reason, so a submission aborted by shutdown isn't recorded as a failure.
// The benign no-beacon-objects sentinel (ErrNoValidDutiesToExecute) pre-concludes the duty as
// not_required before returning, which makes this deferred markDutyFailed a no-op (concludeDuty
// is idempotent) — it must not be recorded as failed either.
defer func() {
if err != nil && !isRecoverableReconstructError(err) {
r.markDutyFailed(err)
Expand All @@ -529,6 +536,12 @@ func (r *CommitteeRunner) ProcessPostConsensus(ctx context.Context, logger *zap.
return fmt.Errorf("could not get expected post consensus roots and beacon objects: %w", err)
}
if len(beaconObjects) == 0 {
// Benign terminal: the committee reached consensus but this operator has no beacon objects to
// submit (e.g. divergent validator sets across the committee's operators). Conclude as
// not_required — not failed — before returning the sentinel; concludeDuty is idempotent, so
// the deferred markDutyFailed becomes a no-op. The sentinel still tells committee_queue to
// drop the message and terminate the runner.
r.markDutyNotRequired()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Empty objects mask construction failures

When every validator is skipped because duty validation, object construction, domain-data retrieval, or signing-root computation fails, this branch records the empty result as successful not_required, suppressing the failed outcome and warning for a missed submission.

Knowledge Base Used: Protocol v2 Duty Runners and QBFT Consensus

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in c92bad9.

@iurii-ssv iurii-ssv Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Building on the existing P1 by Greptile from above (empty objects masking construction failures) rather than repeating it — two clarifications that should help decide the fix:

A template already exists in the sibling runner. AggregatorCommitteeRunner hits the same empty-objects terminal but deliberately surfaces construction/domain errors instead of swallowing them, so a genuine failure stays failed rather than becoming not_required — see the note at aggregator_committee.go#L1441-L1445. Here, expectedPostConsensusRootsAndBeaconObjects instead does logger.Debug(...); continue on every construct / DomainData / signing-root failure, so the two siblings disagree on what an empty map means. Returning an error when a validator is skipped for a non-benign reason (vs. guard-invalid) would realign them and preserve failed.

Severity is likely below P1 in practice. DomainData for this domain/epoch is already fetched successfully during consensus-phase signing (signBeaconObject) and is normally cached, so an all-validators failure surfacing only at post-consensus is an unlikely path. The guard-invalid case (divergent validator sets — the actual #2903 trigger) is the dominant real cause of an empty map.

Scope note: only this post-consensus branch is affected. The consensus-phase sibling change at L381-L387 is unambiguous — totalAttesterDuties is incremented before signing and signing errors return early via errCh, so a zero count there can only mean guard-invalid, never a swallowed failure.

@momosh-ssv momosh-ssv Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in c92bad9.
Went with the realignment you suggested, scoped to the empty-map terminal: expectedPostConsensusRootsAndBeaconObjects now collects the construct/DomainData/signing-root errors and surfaces them when nothing could be built, so that empty result concludes failed via the defer, while partial failures keep the per-validator debug-and-continue behavior (one validator's failure shouldn't block the others' submissions).

A guard-invalid-only empty map stays not_required. Added a regression test with DomainData failing after consensus to pin the all-failure path.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Finding 3 · [MINOR] New not_required terminals skip EndDutyFlow/recordTotalDutyDuration and leave no operator-visible completion line

Every pre-existing markDutyNotRequired() call site pairs the conclusion with duty-flow bookkeeping (EndDutyFlow + recordTotalDutyDuration) and usually an Info completion log. The three new sites this PR adds (committee.go:385, committee.go:544, aggregator_committee.go:894) call the marker alone.

Two consequences. First, metric skew: ssv.runner.duty.outcome{not_required} increments with no matching total-duty-duration sample, and the duty-flow measurement is left open — any dashboard reconciling outcome counts against the duration histogram shows a permanent, unexplained gap. Second, zero operator-visible evidence: before this PR the operator saw duty failed plus an error-level queue log; after, the metric is benign, the runner itself logs nothing, and a related change in this PR downgrades the queue's own log line to Debug. A committee runner terminating early for the slot becomes invisible at default log level — which matters independently of whether the classification itself is correct, since it's the operator's only handle if that classification turns out wrong.

@momosh-ssv momosh-ssv Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 82e1c78.
Both committee sites now close the duty flow (EndDutyFlow + recordTotalDutyDuration) and log an Info completion line, matching the pre-existing not_required sites.

The aggregator-committee site went back to failed in cba5938, so operator visibility there comes from the outcome watcher's warn.

return ErrNoValidDutiesToExecute
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,3 +122,85 @@ func TestCommitteeRunnerProcessPostConsensus_RecoverableInvalidSigsThenSucceeds(
t.Fatal("expected a succeeded duty conclusion after recovery, got none")
}
}

// invalidateDutiesInGuard marks every validator duty of the committee duty invalid in the guard
// stub, so expectedPostConsensusRootsAndBeaconObjects (and the ProcessConsensus signing loop) skips
// them all.
func invalidateDutiesInGuard(guard *committeeDutyGuardStub, duty *spectypes.CommitteeDuty) {
guard.validErrs = make(map[string]error)
for _, vd := range duty.ValidatorDuties {
key := guard.validKey(vd.Type, spectypes.ValidatorPK(vd.PubKey), vd.DutySlot())
guard.validErrs[key] = errors.New("duty no longer valid")
}
}

// TestCommitteeRunnerProcessPostConsensus_MarksNotRequiredOnNoBeaconObjects is the regression test
// for #2903: a post-consensus quorum where this operator ends up with no beacon objects to submit
// (e.g. divergent validator sets across the committee's operators — modeled here by invalidating
// the duties in the guard after consensus) is a benign terminal. It must conclude not_required —
// not failed (the previous behavior, surfacing as a spurious "⚠️ duty failed") and not a silent
// stall — while still surfacing the sentinel for the queue's terminal-drop handling.
func TestCommitteeRunnerProcessPostConsensus_MarksNotRequiredOnNoBeaconObjects(t *testing.T) {
guard := &committeeDutyGuardStub{}
env := newCommitteeRunnerEnv(t, []int{1}, guard, &doppelgangerStub{})
duty := spectestingutils.TestingCommitteeDuty([]int{1}, nil, spec.DataVersionElectra)

env.startAndDecideCommitteeDuty(t, duty)
concluded := observeConclusion(env)

invalidateDutiesInGuard(guard, duty)

var postConsensusErr error
for id := spectypes.OperatorID(1); id <= 3; id++ {
msg := spectestingutils.PostConsensusCommitteeMsgForDuty(duty, env.keySetMap, id)
if err := env.runner.ProcessPostConsensus(context.Background(), env.logger, msg); err != nil {
postConsensusErr = err
}
}

require.ErrorIs(t, postConsensusErr, ErrNoValidDutiesToExecute, "the benign sentinel must surface to the queue")

select {
case c := <-concluded:
require.Equal(t, dutyOutcomeNotRequired, c.outcome, "no beacon objects to submit must conclude not_required, not failed")
require.NoError(t, c.reason)
default:
t.Fatal("expected a not_required duty conclusion, got none")
}
require.True(t, env.runner.State.Succeeded, "not_required is a correct completion")
require.Empty(t, env.beacon.GetBroadcastedRoots(), "nothing should have been submitted")
}

// TestCommitteeRunnerProcessConsensus_MarksNotRequiredOnNoValidDuties covers the consensus-phase
// sibling of the #2903 sentinel: a committee that decides while this operator has zero valid duties
// to sign (all invalidated in the guard before consensus) previously concluded via no marker at
// all, surfacing as a false "stuck". It must conclude not_required and surface the sentinel.
func TestCommitteeRunnerProcessConsensus_MarksNotRequiredOnNoValidDuties(t *testing.T) {
guard := &committeeDutyGuardStub{}
env := newCommitteeRunnerEnv(t, []int{1}, guard, &doppelgangerStub{})
duty := spectestingutils.TestingCommitteeDuty([]int{1}, nil, spec.DataVersionElectra)

ctx := t.Context()
require.NoError(t, env.runner.StartNewDuty(ctx, env.logger, duty, env.sampleKey.Threshold))
concluded := observeConclusion(env)

invalidateDutiesInGuard(guard, duty)

var consensusErr error
for _, msg := range spectestingutils.CommitteeInputForDuty(duty, duty.Slot, env.keySetMap, false) {
if err := env.runner.ProcessConsensus(ctx, env.logger, msg); err != nil {
consensusErr = err
}
}

require.ErrorIs(t, consensusErr, ErrNoValidDutiesToExecute, "the benign sentinel must surface to the queue")

select {
case c := <-concluded:
require.Equal(t, dutyOutcomeNotRequired, c.outcome, "deciding with zero valid duties must conclude not_required")
require.NoError(t, c.reason)
default:
t.Fatal("expected a not_required duty conclusion, got none")
}
require.True(t, env.runner.State.Succeeded, "not_required is a correct completion")
}
9 changes: 6 additions & 3 deletions protocol/v2/ssv/validator/committee_queue.go
Original file line number Diff line number Diff line change
Expand Up @@ -228,13 +228,16 @@ func (c *Committee) ConsumeQueue(
const couldNotHandleMsgLogPrefix = "could not handle message, "
switch {
case errors.Is(err, runner.ErrNoValidDutiesToExecute):
const droppingMsgDueToNoValidDutiesToExecuteEvent = "❗ " + couldNotHandleMsgLogPrefix + "dropping message and terminating committee-runner"
msgLogger.Error(droppingMsgDueToNoValidDutiesToExecuteEvent, zap.Error(err))
// Benign terminal, not a handling failure: the committee decided but this operator has
// no duties to execute (the runner already concluded the duty as not_required), so the
// message is dropped and the runner terminated without error-level noise.
const droppingMsgDueToNoValidDutiesToExecuteEvent = "no valid duties to execute, dropping message and terminating committee-runner"
msgLogger.Debug(droppingMsgDueToNoValidDutiesToExecuteEvent, zap.Error(err))
msgState.span.AddEvent(droppingMsgDueToNoValidDutiesToExecuteEvent, trace.WithAttributes(
attribute.String("drop_reason", err.Error()),
attribute.Int64("attempt", currentAttempt),
))
msgState.span.SetStatus(codes.Error, droppingMsgDueToNoValidDutiesToExecuteEvent)
msgState.span.SetStatus(codes.Ok, "")
msgState.span.End()
msgStates.Delete(msgKey)
return
Expand Down