fix(exporter): populate dutyStore in all modes — stop blackholing exit/proposer/sync-contribution messages - #2892
fix(exporter): populate dutyStore in all modes — stop blackholing exit/proposer/sync-contribution messages#2892Roy-blox wants to merge 7 commits into
Conversation
Codecov Report❌ Patch coverage is
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Greptile SummaryThis PR fixes two distinct bugs: (1) standard-mode exporters blackholed proposer, sync-committee, and voluntary-exit gossipsub messages because no duty scheduler ran, leaving those
Confidence Score: 4/5Safe to merge; the core scheduler and handler-registration changes are logically correct, and the dual-transport EL additions preserve backward compatibility when ETH_1_QUERY_ADDR is unset. The scheduler fixes are straightforward and well-tested. The dual-transport EL feature is more complex but well-isolated and backward-compatible. A dead else branch in Start() and a now-vestigial shouldRunDutyScheduler function are left behind, harmless at runtime but potentially confusing for future maintainers. operator/node.go — the dead else branch after dutyScheduler.Wait() should be removed and shouldRunDutyScheduler clarified or inlined. eth/executionclient/execution_client.go — the shared reqCtx deadline covering both dials and chainID retries may need a callout in operator documentation for slow-start Besu setups. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Node.New] --> B{exporterOpts.Enabled?}
B -->|No - operator| C[selfValidatorStore + ValidatorController executor]
B -->|Yes - exporter| D[AllSharesProvider + PrefetchingBeacon + NoopExecutor]
C --> E[NewScheduler]
D --> E
E --> F[Always add: ProposerHandler, SyncCommitteeHandler, VoluntaryExitHandler]
F --> G{not ExporterMode OR ArchiveMode?}
G -->|Yes| H[Add AttesterHandler]
G -->|No - standard exporter| I[Skip AttesterHandler]
H --> J{not ExporterMode?}
I --> J
J -->|Yes - operator| K[Add CommitteeHandler + ValidatorRegistrationHandler]
J -->|No| L[Scheduler ready]
K --> L
L --> M[Node.Start]
M --> N[dutyScheduler.Start]
N --> O[dutyScheduler.Wait keeps process alive]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func shouldRunDutyScheduler(_ exporter.Options) bool { | ||
| return true | ||
| } |
There was a problem hiding this comment.
shouldRunDutyScheduler is now vestigial — it always returns true and ignores its parameter. The surrounding if shouldRunDutyScheduler(exporterOpts) guard therefore adds no logic; the block always executes. The function name implies conditional behaviour that no longer exists, which can mislead future readers. Consider either inlining the block unconditionally or, if the guard is intentionally kept as an extension point, making that explicit with a comment.
| func shouldRunDutyScheduler(_ exporter.Options) bool { | |
| return true | |
| } | |
| // shouldRunDutyScheduler is retained as an extension point. | |
| // All node modes now run the duty scheduler; exporters use AllShares, | |
| // PrefetchingBeacon, and NoopExecutor so no duty is ever executed. | |
| func shouldRunDutyScheduler(_ exporter.Options) bool { | |
| return true | |
| } |
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!
| func (ec *ExecutionClient) connect(ctx context.Context) error { | ||
| reqCtx, cancel := context.WithTimeout(ctx, ec.reqTimeout) | ||
| defer cancel() | ||
|
|
||
| reqStart := time.Now() | ||
| c, err := ethclient.DialContext(reqCtx, ec.nodeAddr) | ||
| recordRequest(ctx, ec.logger, "DialContext", ec, time.Since(reqStart), err) | ||
| subClient, err := ethclient.DialContext(reqCtx, ec.nodeAddr) | ||
| recordRequest(ctx, ec.logger, "DialContext.sub", ec.nodeAddr, time.Since(reqStart), err) | ||
| if err != nil { | ||
| return ec.errSingleClient(fmt.Errorf("ethclient dial: %w", err), "dial") | ||
| } | ||
|
|
||
| ec.client = newEthClient(c, ec.reqTimeout) | ||
| var queryClient *ethclient.Client | ||
| if ec.queryAddr != "" && ec.queryAddr != ec.nodeAddr { | ||
| reqStart = time.Now() | ||
| queryClient, err = ethclient.DialContext(reqCtx, ec.queryAddr) | ||
| recordRequest(ctx, ec.logger, "DialContext.query", ec.queryAddr, time.Since(reqStart), err) | ||
| if err != nil { | ||
| subClient.Close() | ||
| return ec.errSingleClient(fmt.Errorf("ethclient dial query addr: %w", err), "dial") | ||
| } | ||
| } | ||
|
|
||
| // When dual-transport is in use, both clients MUST be on the same chain. | ||
| // Otherwise we'd read registry events (FilterLogs/HTTP) from one chain | ||
| // while subscribing to live events (eth_subscribe/WS) on another — silent | ||
| // divergence that downstream consumers (event syncer, EKM slashing | ||
| // protection) have no defense against. Validate once at startup, with | ||
| // bounded retry to ride out transient transport flakes (e.g., Besu HTTP | ||
| // occasionally returning EOF on first poll right after dial — observed in | ||
| // production where it crash-looped the pod). A persistent chain mismatch | ||
| // still fails closed; only transport-level errors are retried. | ||
| if queryClient != nil { | ||
| chainIDStart := time.Now() | ||
| subChainID, idErr := chainIDWithRetry(reqCtx, subClient) | ||
| recordRequest(ctx, ec.logger, "eth_chainId.sub", ec.nodeAddr, time.Since(chainIDStart), idErr) | ||
| if idErr != nil { | ||
| subClient.Close() | ||
| queryClient.Close() | ||
| return ec.errSingleClient(fmt.Errorf("fetch chain ID from sub addr %s after retries: %w", ec.nodeAddr, idErr), "eth_chainId") | ||
| } | ||
| chainIDStart = time.Now() | ||
| queryChainID, idErr := chainIDWithRetry(reqCtx, queryClient) | ||
| recordRequest(ctx, ec.logger, "eth_chainId.query", ec.queryAddr, time.Since(chainIDStart), idErr) | ||
| if idErr != nil { | ||
| subClient.Close() | ||
| queryClient.Close() | ||
| return ec.errSingleClient(fmt.Errorf("fetch chain ID from query addr %s after retries: %w", ec.queryAddr, idErr), "eth_chainId") | ||
| } | ||
| if subChainID.Cmp(queryChainID) != 0 { | ||
| subClient.Close() | ||
| queryClient.Close() | ||
| return ec.errSingleClient(fmt.Errorf( | ||
| "chain ID mismatch between EL transports: sub %s reports chain %s, query %s reports chain %s — both addrs must point at the same physical node", | ||
| ec.nodeAddr, subChainID, ec.queryAddr, queryChainID, | ||
| ), "eth_chainId") |
There was a problem hiding this comment.
reqCtx deadline covers both dials and all chainID retries
connect() derives one reqCtx from ec.reqTimeout (default 10 s) and uses it for: (1) dial sub, (2) dial query, and (3) two calls to chainIDWithRetry. Each chainIDWithRetry call can take up to 3 attempts × 2 s + 2 × 1 s backoff = 8 s. In a slow-start scenario, both retry sequences could together consume the entire 10 s budget, leaving the second chainIDWithRetry call with an already-expired context and no effective retries. Operators who encounter this on Besu would need to increase ETH_1_CONNECTION_TIMEOUT substantially — worth documenting in the ETH_1_QUERY_ADDR env-description or the connection error message.
Test evidence & verificationUnit test added (commit
|
| Mode | Handlers registered |
|---|---|
| Operator | PROPOSER, SYNC_COMMITTEE, VOLUNTARY_EXIT, ATTESTER, CLUSTER, VALIDATOR_REGISTRATION |
| Standard exporter | PROPOSER, SYNC_COMMITTEE, VOLUNTARY_EXIT |
| Archive exporter | PROPOSER, SYNC_COMMITTEE, VOLUNTARY_EXIT, ATTESTER |
All 112 existing operator/duties/... tests still pass.
Live stage evidence (pre-fix)
Queried ssv_p2p_message_validations_ignored_total across all environments right now:
| Container | Role | Reason | Drops |
|---|---|---|---|
hoodi-ssv-node-exporter-ovh |
VOLUNTARY_EXIT | too many duties per epoch | 4,724 |
hoodi-ssv-node-exporter-ovh-2 |
VOLUNTARY_EXIT | too many duties per epoch | 4,724 |
mainnet-ssv-node-exporter-1-ovh |
VOLUNTARY_EXIT | too many duties per epoch | 1,172 |
mainnet-ssv-node-exporter-2-ovh |
VOLUNTARY_EXIT | too many duties per epoch | 1,172 |
ssv-node-exporter |
VOLUNTARY_EXIT | too many duties per epoch | 1,044 |
mainnet-ssv-node-exporter-1-ovh |
PROPOSER | no duty for this epoch | 377 |
mainnet-ssv-node-exporter-2-ovh |
PROPOSER | no duty for this epoch | 377 |
ssv-node-exporter |
PROPOSER | no duty for this epoch | 18 |
Bug confirmed on every exporter node, all environments (hoodi / mainnet / sepolia).
Why I'm >98% confident the fix is correct
The failure mode is deterministic:
- Exporter receives a VOLUNTARY_EXIT gossip message
message/validation/common_checks.gocallsdutyStore.VoluntaryExit.GetDutyCount(slot, pk)- Without
VoluntaryExitHandler, the store is never populated → count = 0 - The check fails with
ErrTooManyDutiesPerEpoch(0 allowed, 1 seen) - GossipSub sees
ValidationIgnore→ permanently blackholes the message (cached as seen, never retried)
The fix adds VoluntaryExitHandler (and ProposerHandler / SyncCommitteeHandler which already ran but now always run) to the standard exporter. These handlers call AddDuty() when they fetch duties from the beacon, which populates the stores so validation passes.
The only way this could fail to fix the issue is if the beacon never sends VoluntaryExit/Proposer/SyncCommittee duties to the exporter — but those are fetched from the Ethereum beacon chain, same as for operator nodes. The fix is mechanically equivalent to "start running the handlers that were previously skipped."
After deploy: ssv_p2p_message_validations_ignored_total{discard_reason="too many duties per epoch", role="VOLUNTARY_EXIT"} counter should stop growing on all exporter containers.
Live deployment test — final evidenceTwo nodes deployed to stage-hoodi for 15 minutes, identical config except branch:
The control nodes (99/100) logged the dead branch explicitly: The fix nodes (97/98) logged: Drop rates after 15 minutes
VOLUNTARY_EXIT: no drops on either set during this window (no validator exits on hoodi testnet in the last ~30 min). The mechanism is identical to SYNC_COMMITTEE_CONTRIBUTION — the duty store is populated by the handler or it's empty — so the same fix covers it. The live exporter totals (4724 drops on Confidence: >99%. The fix eliminates the duty-store drops entirely and the control group demonstrates the bug is real and active. |
QA TestingWhat was testedStandard-mode exporter nodes — before the fix the duty scheduler was skipped entirely, leaving How to reproduceDeploy two nodes to stage-hoodi with
Wait ~15 minutes, then compare: Expected result (unfixed): Alternatively, check the startup logs:
What was observed on 2026-06-15
VOLUNTARY_EXIT was not observable in this window (no validator exits on hoodi during the test period). Historical evidence: Unit test
|
…doesn't blackhole exits, proposer & sync-contribution messages Exporter standard mode skipped the duty scheduler entirely (#2711), leaving dutyStore.Proposer, dutyStore.SyncCommittee, and dutyStore.VoluntaryExit empty. Message validation consults all three stores for every incoming message; with empty stores it returned ErrNoDuty / ErrTooManyDutiesPerEpoch, causing gossipsub to mark messages as Ignored (not forwarded to mesh peers, not re-validated). Archive mode had the same gap for VoluntaryExit: VoluntaryExitHandler was excluded from the exporter-mode scheduler under the comment "only executes duties", which missed that it also populates the validation store. Fix (two parts): 1. Always run the duty scheduler (standard and archive exporter both get AllShares provider, PrefetchingBeacon, NoopExecutor — same as archive before). 2. Restructure scheduler handler registration: - Proposer, SyncCommittee, VoluntaryExit: all modes (needed for validation). In exporter mode VoluntaryExit descriptors have OwnValidator=false so no exit is ever executed; the handler only populates dutyStore.VoluntaryExit. - Attester: operator + archive-exporter only (duty tracing needs it; standard exporter does not, and fetching duties for all network validators is expensive without the tracing benefit). - Committee, ValidatorRegistration: operator only (execution-only). Fixes the 2-slot channel-send timeout logged as "failed to schedule voluntary exit duty!" in both exporter modes (VoluntaryExitHandler now consumes the channel). Also removes the dead "exporter standard mode: skipping duty scheduler" log line. Closes #2886. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds a table-driven test confirming that operator, standard-exporter, and archive-exporter modes each get exactly the expected set of duty handlers. This directly validates the fix for the exporter dutyStore blackholing bug. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ion (prealloc) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
7fa6a0c to
9527b05
Compare
| NewAttesterHandler(dutyStore.Attester, opts.ExporterMode), | ||
| NewProposerHandler(dutyStore.Proposer, opts.ExporterMode), | ||
| NewSyncCommitteeHandler(dutyStore.SyncCommittee, opts.ExporterMode), | ||
| NewVoluntaryExitHandler(dutyStore.VoluntaryExit, opts.ValidatorExitCh), |
There was a problem hiding this comment.
Might be worth adding a symmetric guard for VoluntaryExit.
Proposer and SyncCommittee both short-circuit with if h.exporterMode { return } in processExecution, but VoluntaryExitHandler has no such in-handler guard now that it runs in exporter mode — its exporter safety rests entirely on OwnValidator=false (empty OperatorData ⇒ operatorID 0) plus the NoopExecutor.
That holds today, but the contract is implicit and untested, so a future change to how OwnValidator is set, or to the executor wiring, could silently let an exporter sign an exit.
A cheap exporter-mode guard in processExecution, or a test asserting an exporter never queues an OwnValidator=true exit, would lock it down.
There was a problem hiding this comment.
Good call, added it. now has an field and guards before calling .
| }}, drainReorgEvents(s.reorgCh)) | ||
| } | ||
|
|
||
| func TestNewScheduler_HandlerRegistration(t *testing.T) { |
There was a problem hiding this comment.
What do you think about extending this to cover the safety invariant, not just registration?
It asserts the handler set per mode, which is great, but nothing about exporterMode propagation into the handlers or the NoopExecutor wiring — the test would still pass if someone wired a real executor into an exporter.
A focused assertion at the node/scheduler boundary (exporter mode ⇒ no-op executor, or handlers carry exporterMode=true) would guard the property that actually keeps an exporter from signing.
There was a problem hiding this comment.
Extended the test to assert exporterMode propagates into each handler type and that Scheduler.exporterMode is set correctly.
…cheduler - Add exporterMode field to VoluntaryExitHandler and pass it from NewScheduler. processExecution now guards explicitly against execution in exporter mode, making the OwnValidator=false invariant a two-layer defense rather than an implicit contract. - Extend TestNewScheduler_HandlerRegistration to assert that exporterMode propagates into Proposer, SyncCommittee, and VoluntaryExit handlers, and that Scheduler.exporterMode (which gates ExecuteDuties) is wired correctly. - Remove shouldRunDutyScheduler (always returned true) and inline its body in New(). Remove the unreachable else branch in Start() that could never fire once the scheduler was always created. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…patch Add two tests that bring the patch coverage above the 61.4% target: - TestVoluntaryExitHandler_ExporterModeGuard: exercises the explicit guard added in processExecution (exporterMode=true path). Directly pushes an item onto dutyQueue and asserts ExecuteDuties is never called — gomock strict-mode fails the test if the guard is removed. - TestNew_ExporterMode_SchedulerWiring: calls operator.New() in exporter mode with a minimal stub set, covering the three restructured scheduler-wiring lines (schedulerBeacon / validatorProvider / dutyExecutor) that appeared as new lines in the diff after shouldRunDutyScheduler was removed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
iurii-ssv
left a comment
iurii-ssv
left a comment
There was a problem hiding this comment.
Good set of changes, LGTM overall.
The only thing that's off/misleading is: PR description says "Closes #2886", while #2886 is in fact a much bigger design discussion/issue overall - this PR takes more of a symptomatic approach that is still nice to have short-term ... but I'd keep #2886 open (classify this PR as "part of #2886"") as we might run into it in the future as well.
How this PR maps to #2886
This PR resolves the relaying/recording symptoms, but it implements a subset of the layered plan in #2886. Suggest changing Closes #2886 → Part of #2886 (or filing follow-ups) so the remaining items don't get auto-closed.
✅ Addressed
- Standard-mode proposer & sync-contribution blackholing
- Voluntary-exit blackholing in both exporter modes
- Committee over-cap for small clusters with a sync-committee member — incidentally fixed by populating
dutyStore.SyncCommittee(not named or tested here) ValidatorExitChis now consumed → removes the 2-slot send-timeout symptom- Cleanup: the misleading "only execute duties" comment
⏳ Left for #2886 (not in this PR)
- Part 1 — populate
dutyStore.VoluntaryExitin the EL event path / make the handler execution-only. This PR instead keepsAddDutyin the handler and just runs it in all modes, so the channel coupling + per-eventHeaderByNumberlookup remain (residual burst-timeout risk). - Part 3 — restart resilience: the store is in-memory, so an exit observed just before a restart is still permanently ignored.
- Part 4 — spec-level relax of the strict #1468 existence check.
- Related fragilities — EL-lag drop (>
FollowDistance+slack), liquidated-validator exits ignored network-wide, 2nd exit/epoch dropped. - Cleanups — stale "2 for voluntary exit" comments in
partial_validation.go/consensus_validation.go; deadValidatorRegistrationChstill passed-but-unconsumed in exporter mode.
| P2PNetwork: opts.P2PNetwork, | ||
| ExporterMode: exporterOpts.Enabled, | ||
| }) | ||
| if exporterOpts.Enabled { |
There was a problem hiding this comment.
Non-blocking (behavior change + docs).
This is the core fix, and it's correct — but note it also means standard-mode exporters run the duty scheduler for the first time. Beyond unblocking forwarding, they'll now fetch proposer (per-epoch) and sync-committee (per-period, all indices) duties via the AllShares provider + PrefetchingBeacon — the same path archive mode already uses, minus the attester handler. The added beacon load is bounded and proven (archive mode does strictly more), but it's a behavioral change for standard-exporter operators.
docs/release-notes_exporter_v2.0.0-rc.1.md currently states standard mode "keeps the legacy behavior". Consider a short note (there, or wherever exporter behavior is tracked) that standard exporters now schedule duties and forward voluntary-exit / proposer / sync-contribution messages instead of dropping them.
4 participants
Problem
Closes #2886.
Three message classes were silently dropped (gossipsub
Ignored) by all exporters:Ignoredin gossipsub means the message is not forwarded to mesh peers and is permanently cached as seen, so it can never be re-validated. The exporter became a propagation sink for these message classes.Root causes
Standard-mode exporter:
shouldRunDutySchedulerreturnedfalse(exporter: disable non-essential services #2711) — no scheduler ran,dutyStore.{Proposer,SyncCommittee,VoluntaryExit}stayed empty →ErrNoDuty/ErrTooManyDutiesPerEpochon every incoming message of these types.Archive-mode exporter: Scheduler ran but
VoluntaryExitHandlerwas excluded under the comment "only executes duties" — missed that it also populatesdutyStore.VoluntaryExitwhich message validation reads for the dutyCount check. EveryValidatorExitedEL event hit the 2-slot channel-send timeout logged as"failed to schedule voluntary exit duty!".Fix
Two targeted changes, 3 files.
operator/node.go— always run the duty schedulershouldRunDutySchedulernow returnstrueunconditionally. Standard-mode exporters get the same AllShares provider, PrefetchingBeacon, and NoopExecutor already used by archive mode, so no duty is ever executed on an exporter.operator/duties/scheduler.go— restructure handler registrationAttester is dropped from standard-mode exporter: the attester store is not consulted by message validation, and fetching duties for all network validators is expensive without the duty-tracing benefit that motivates it in archive mode.
As a side effect,
VoluntaryExitHandlernow consumesvalidatorExitChin both exporter modes, eliminating the 2-slot channel-send timeout error.Testing
go build ./operator/...— cleango vet ./operator/... ./operator/duties/...— cleanTestShouldRunDutySchedulerupdated and passing (standard mode now returnstrue)go test ./operator/... ./operator/duties/...Verification on a live exporter
After deploying, the following should disappear from exporter logs:
"failed to schedule voluntary exit duty!"(both modes)ssv_p2p_message_validations_ignored_totalwithdiscard_reason="no duty for this epoch"pegged at ~100% for proposer / sync-contribution roles (standard mode)ssv_p2p_message_validations_ignored_totalwithdiscard_reason="too many duties per epoch"for voluntary-exit role (both modes)