fix(runner): conclude benign ErrNoValidDutiesToExecute as not_required, not failed - #2988
fix(runner): conclude benign ErrNoValidDutiesToExecute as not_required, not failed#2988momosh-ssv wants to merge 7 commits into
Conversation
…d, not failed A committee or aggregator-committee duty that reaches post-consensus quorum but leaves this operator with no beacon objects to submit is a normal "nothing to do" terminal (divergent validator sets across the committee's operators), yet both runners classified it as a failed outcome — emitting a spurious "duty failed" warning and a false ssv.runner.duty.outcome=failed data point, while the queue simultaneously treated the same sentinel as a benign terminal drop. Conclude the branch as not_required instead, in both runners: - AggregatorCommitteeRunner: direct swap of markDutyFailed for markDutyNotRequired at the len(beaconObjects) == 0 branch. - CommitteeRunner: markDutyNotRequired before the sentinel return; the deferred markDutyFailed becomes a no-op via concludeDuty idempotency. - CommitteeRunner.ProcessConsensus: the zero-valid-duties sentinel had no marker at all (false "stuck"); conclude it as not_required too. The sentinel is still returned in every case, preserving committee_queue's terminal-drop handling. Genuine terminal failures (expected-roots errors, terminal BLS reconstruction, submit failures) remain classified failed. Closes #2903
Codecov Report✅ All modified and coverable lines are covered by tests. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Greptile SummaryThis PR changes zero-work committee-runner terminals from failed or unconcluded to not_required while preserving ErrNoValidDutiesToExecute for queue termination.
Confidence Score: 4/5The PR should not merge until the committee post-consensus path distinguishes benign absence of duties from failures that prevent every beacon object from being constructed. CommitteeRunner can swallow all per-validator construction or beacon-data errors into an empty object set, and the changed branch then records that missed submission as a successful not_required outcome. Files Needing Attention: protocol/v2/ssv/runner/committee.go
|
| Filename | Overview |
|---|---|
| protocol/v2/ssv/runner/committee.go | Adds not_required conclusions to zero-duty and empty-object paths, but the latter also masks cases where all object-building operations fail. |
| protocol/v2/ssv/runner/aggregator_committee.go | Reclassifies a genuinely empty aggregator/contributor assignment as not_required while retaining propagated derivation errors as failures. |
| protocol/v2/ssv/runner/committee_postconsensus_classification_test.go | Covers guard-invalidated empty-object and zero-duty outcomes but not an empty result caused by object-construction or beacon-data failures. |
| protocol/v2/ssv/runner/aggregator_committee_test.go | Verifies that decided data with no aggregator or contributor assignment concludes not_required and preserves the queue sentinel. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Consensus decides] --> B{Valid local duties?}
B -->|No| C[Conclude not_required]
B -->|Yes| D[Collect post-consensus quorum]
D --> E[Build beacon objects]
E --> F{Objects produced?}
F -->|Yes| G[Submit to beacon node]
F -->|No| C
C --> H[Return ErrNoValidDutiesToExecute]
H --> I[Committee queue terminates runner]
Reviews (1): Last reviewed commit: "fix(runner): conclude benign ErrNoValidD..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // the deferred markDutyFailed becomes a no-op. The sentinel still tells committee_queue to | ||
| // drop the message and terminate the runner. | ||
| r.markDutyNotRequired() |
There was a problem hiding this comment.
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!
The committee_queue case that catches ErrNoValidDutiesToExecute logged "❗ could not handle message, dropping message and terminating committee-runner" at Error level and marked the trace span as Error — the same false alarm as the failed outcome classification, fired by the same benign trigger. The runner now concludes the duty as not_required on this sentinel, so the drop is a correct completion: log it at Debug with neutral wording and set the span status to Ok. The drop-and- terminate behavior is unchanged.
| // 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() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
ovidiu-ssv-labs
left a comment
ovidiu-ssv-labs
left a comment
There was a problem hiding this comment.
The core idea is right and the CommitteeRunner consensus-phase fix (false 'stuck') is a clear win, but the AggregatorCommitteeRunner half reclassifies a state that is unreachable under spec-valid decided data — an invariant violation — as benign, removing its only alarm. Separately, the shutdown path can now emit a spurious not_required, the new terminals skip the duty-duration bookkeeping every sibling not_required site performs, and the change is not 'observability only' as described (markDutyNotRequired sets State.Succeeded). Greptile's P1 on committee.go:544 is confirmed still open at head. [verdict: with_fixes]
Finding 4 · [MINOR] Change is not "observability only": markDutyNotRequired also flips State.Succeeded — protocol/v2/ssv/runner/runner.go:643
The PR description states this change has "no impact on consensus or on what gets submitted... observability only." That's not quite accurate: markDutyNotRequired sets b.State.Succeeded = true, unlike the replaced markDutyFailed (which doesn't) and unlike the previous ProcessConsensus site (which set nothing). All three converted sites now flip real runner state: hasDutySucceeded() gates ValidatePreConsensusMsg/ValidatePostConsensusMsg (now short-circuit instead of processing); hasDutyRunning() and friends feed ConsumeQueue's per-iteration state; and the flag is serialized into GetStateRoot(), which spec tests compare directly.
Verified blast radius: spec tests pass at head (go test -tags blst_enabled ./protocol/v2/ssv/spectest, no fixture reaches these branches, so no root moves) — but make full-test excludes spectest as a separate target, so the PR's claim that "the full runner and validator suites pass" would not have caught a spectest regression here. No message-gating bug is reachable today, but only because ConsumeQueue returns entirely on this sentinel — an implicit containment that becomes load-bearing if that behavior is ever softened.
| // 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() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| // 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() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| // 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() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
…uilt, instead of not_required An empty post-consensus beacon-objects map was concluded not_required unconditionally, but expectedPostConsensusRootsAndBeaconObjects swallows per-validator construction / domain-data / signing-root failures with a debug log, so an all-validators failure produced the same empty map and recorded a missed submission as a benign completion. Collect the per-validator errors and surface them when nothing could be built (partial failures keep the debug-and-continue behavior), aligning with the sibling AggregatorCommitteeRunner which surfaces these errors. The empty-map not_required terminal is now guaranteed benign (guard-invalid skips only).
…inal loud (failed) Reverts the not_required reclassification for this runner. Unlike the sibling CommitteeRunner (whose beacon objects come from local state, so divergent validator sets legitimately empty them on one operator), this runner builds beaconObjects purely from the decided value with every error surfaced — an empty map means the decided data had zero aggregators and zero contributors, which AggregatorCommitteeConsensusData.Validate() rejects before a value can be stored as decided. Reaching the branch is an invariant violation (bypassed or regressed decided-value validation), a consensus-integrity signal that must stay a warning, not become a silent benign outcome. The sentinel stays wrapped in the returned error so committee_queue still drops the message and terminates the runner.
… phase is cancelled The zero-duties branch in CommitteeRunner.ProcessConsensus also fires on context cancellation: the duty feeder and workers bail out on ctx.Err() before incrementing any counter, so a node shutdown mid-signing recorded a spurious not_required outcome for a duty that was abandoned, not completed. Re-check ctx.Err() before concluding, mirroring markDutyFailed's context.Canceled filter — cancellation is never an outcome.
… bookkeeping and a completion log Every pre-existing markDutyNotRequired site closes the duty flow (EndDutyFlow + recordTotalDutyDuration) and logs an Info completion line; the two new committee terminals called the marker alone, skewing outcome counts against the duration histogram and leaving no operator-visible evidence of the early termination at default log level (the queue's own line is Debug).
|
Re Finding 4: fair catch — markDutyNotRequired does set State.Succeeded, so "observability only" overstated it. I've rewritten the PR description to drop that claim and describe the state effect and the ConsumeQueue containment explicitly, and this round was validated against the spectest suite as well (passes), since full-test doesn't run it. Findings 1–3 are addressed in cba5938, b0ef6a9 and 82e1c78. |
|
I don't see this as something showing up in the spec, so I guess it's ok to do our own extra err handling. see ref: https://github.com/ssvlabs/ssv-spec/blob/main/ssv/aggregator_committee.go Fyi @GalRogozinski @MatheusFranco99 but I don't really understand how this line is possible, and need some more explanation - |
Yep, agree, ok to add err handling here. But I also don't quite understand the line. Line I thought:
Would that be it? Operators skip part of the proposals for validators they don't know but Idk what is the condition for when it doesn't know any of them... |
|
@y0sher @MatheusFranco99 backing that sentence with the concrete code paths. The empty terminal is reachable in two ways, both benign:
Both are per operator because the decided value is only the @MatheusFranco99 the zero known validators case cannot reach this branch: One scope note: since cba5938 the The PR description now states these two triggers explicitly. Quick follow-up after some internal back and forth on the "this is normal" wording. Better said: it's rare, but when it happens nothing actually went wrong. Rare because it takes every validator in the operator's local duty set getting invalidated mid duty, and there are only two ways that happens: the share was removed (committee.go#L94-L101) or a fresher duty took over (committee_guard.go#L36-L44). Nothing gets missed either way: removed validators shouldn't be submitted for, superseded duties are covered by the newer run, and the rest of the committee still forms quorum without us, same as if we were offline. One more idea that came up: an operator briefly on the wrong fork with no duty for the slot. That can't reach this branch, the runner just never starts (committee.go#L252-L255). And with a partial duty set it submits whatever it has (committee.go#L1053-L1056). Since c92bad9 this isn't just an argument, the code enforces it: if the map is empty because building objects failed, we error out and conclude failed (committee.go#L1126-L1128). Only guard invalidations can leave it empty and benign. Updated the PR description to say "rare but expected" instead of "this is normal". |
5 participants
What this fixes
Fixes #2903.
A committee can reach consensus while one of its operators ends up with nothing to submit. It's rare, but when it happens it's the node doing the right thing, not a bug. The decided value is only the
BeaconVote, and each operator submits for its own local duty set, re-checked against its localCommitteeDutyGuardafter deciding. There are exactly two ways every validator in that set can drop out mid duty: the share was removed or liquidated (committee.go#L94-L101), or a fresher duty for the same validator took over (committee_guard.go#L36-L44). Either way nothing is missed: removed validators shouldn't be submitted for, superseded duties are covered by the newer run, and peers that still hold the validator submit as usual. An operator with no duties for the slot never even starts the runner, so it can't land here. The node handled all this correctly, but reported it as a failure: operators saw a⚠️ duty failedwarning and afaileddata point in the duty outcome metric, for something that wasn't a failure at all.This PR makes the node report that situation for what it is: a duty that completed correctly with nothing to submit (
not_required).What changed
Duty outcome classification (committee runner):
not_requiredbefore the error-handling defer can mark itfailed(recording an outcome twice is safe — only the first one counts).not_requiredas well.Guards added in review (round 1):
not_requiredclassification only applies when the empty result is provably benign:expectedPostConsensusRootsAndBeaconObjectsnow surfaces per-validator construction / domain-data / signing-root errors when nothing could be built, so an all-construction-failure empty map concludesfailedinstead of masking a missed submission. Partial failures keep the old behavior (log and continue, so one validator's failure never blocks the others' submissions).ctx.Err()before concluding: a shutdown mid-signing also reaches it with zero counts, and an abandoned duty must record no outcome at all (mirroringmarkDutyFailed'scontext.Canceledfilter).not_requiredterminals close the duty flow (EndDutyFlow+recordTotalDutyDuration) and log an Info completion line, matching every pre-existingnot_requiredsite.Aggregator-committee runner — deliberately NOT reclassified (round 1):
The first version of this PR converted its empty-beacon-objects terminal to
not_requiredtoo. That was wrong and is reverted: unlike the committee runner, its beacon objects are built purely from the decided value with every error surfaced, and a decided value with zero aggregators and zero contributors is rejected byAggregatorCommitteeConsensusData.Validate()before it can ever be stored as decided. Reaching that branch is an invariant violation — a consensus-integrity signal — so it staysfailed, now with an explicit invariant-violation error.The log line in the message queue:
When the benign situation occurs, the queue also printed an error-level log line (
❗ could not handle message...) and marked the trace as an error. It's now a debug-level line with neutral wording, and the trace is marked OK. Operator-visible evidence moves to the runner's own Info completion line (benign cases) or the outcome watcher's warn (failure cases).What did NOT change
failed— beacon submission errors, signature reconstruction failures, all-construction-failure empty results, and the aggregator-committee invariant violation above.Note on runner state (raised in review):
markDutyNotRequiredsetsState.Succeeded = true, like every pre-existingnot_requiredsite — so the converted terminals now flip real runner state where they previously didn't (hasDutySucceeded()gates message validation, and the flag serializes intoGetStateRoot()). No reachable behavior change today:ConsumeQueueterminates the runner on the sentinel before another message can be processed, and the spec test suite passes unchanged — but that containment is load-bearing if the queue's terminal-drop behavior is ever softened.Why
not_requiredand not a new labelThe issue asked for a decision here.
not_requiredalready exists and means exactly this ("completed correctly with nothing to submit") — it's what the aggregator runner uses when a validator turns out not to be an aggregator. Reusing it avoids adding a new metric label for the same idea.Testing
Six regression tests: the three original ones (benign post-consensus empty map, consensus-phase zero duties, and the aggregator-committee empty-decided-data terminal — now pinning
failed), plus three from review round 1 (all-construction-failure empty map concludesfailed, cancelled context concludes nothing, and the aggregator-committee invariant stays loud). Each fix was verified against the code it guards. The fullrunnerandvalidatorsuites and thespectestsuite (blst_enabled) pass.