Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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()
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()
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()
Comment on lines +542 to +544

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!

@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.

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