exporter: fork-straddling trace ranges + /committee cardinality doc (#2968 items 1, 2) - #2975
exporter: fork-straddling trace ranges + /committee cardinality doc (#2968 items 1, 2)#2975momosh-ssv wants to merge 7 commits into
Conversation
validateValidatorRequest evaluated the committee-duty fork gate at request.To, so a validator-traces window whose tail crossed the Boole fork was rejected wholesale - a dashboard polling a fixed window starts 400-ing the moment its tail crosses the fork, losing the still-servable pre-fork slots. Evaluate the gate at request.From instead, and report each post-fork committee-duty slot lacking pubkeys/indices as a non-fatal per-slot note (ErrPostForkCommitteeDutyNote) in the response Errors instead of silently returning nothing for it. The HTTP handler treats a response whose only errors are such notes as a partial 200 - without that, a straddling range with zero pre-fork traces would 500. Item 1 of #2968.
Without a roles filter the endpoint returns one trace per (slot, committeeID, role) - up to two rows per (slot, committeeID) post-fork, distinguished by the role field. Additive-field back-compat holds; cardinality back-compat does not, so state it in the OpenAPI description (regenerated, not hand-edited). Item 2 of #2968.
| if e.isCommitteeDutyAtSlot(role, slot) && len(indices) == 0 { | ||
| // request validation only gates on 'from': a window whose tail crosses | ||
| // Boole reaches here for its post-fork slots without pubkeys/indices, | ||
| // so report the gap as a non-fatal note instead of silently skipping it. | ||
| errs = multierror.Append(errs, fmt.Errorf("%w: slot %d: role %s is a committee duty post-fork, please provide either pubkeys or indices to filter the duty for a specific validators subset or use the /committee endpoint to query all the corresponding duties", ErrPostForkCommitteeDutyNote, slot, role.String())) |
There was a problem hiding this comment.
Unbounded post-fork note amplification
When an external request supplies a pre-Boole from, a very large post-Boole to, and an unfiltered committee-duty role, the lower-bound gate accepts it and this loop allocates one response error per post-fork slot and role, causing unbounded CPU, memory, and response growth; with to set to the maximum uint64 value, the inclusive counter wraps and never terminates. Enforce a bounded range or aggregate these notes instead of creating one per slot.
How this was verified: The request bounds have no range-size constraint, validation checks only request.From, and the inclusive loop appends a formatted error for every matching slot.
Greptile SummaryThis PR permits validator trace ranges to straddle the Boole fork and documents the committee endpoint’s role-based cardinality.
Confidence Score: 4/5The PR should not merge until the newly accepted fork-straddling path enforces a bounded slot range or avoids per-slot note amplification. A caller can select a pre-fork lower bound and an arbitrarily large post-fork upper bound, causing an unbounded slot loop and one allocated response error per post-fork slot and role; the maximum uint64 bound additionally prevents loop termination after counter wraparound. Files Needing Attention: exporter/validator.go
|
| Filename | Overview |
|---|---|
| exporter/validator.go | Changes fork gating and adds per-slot notes, but newly permits unbounded post-fork iteration and response amplification for straddling requests. |
| api/handlers/exporter/validator_http.go | Correctly distinguishes sentinel notes from genuine errors before allowing an empty partial response. |
| api/handlers/exporter/exporter_test.go | Adds HTTP coverage for note-only partial responses and mixed genuine-error behavior. |
| exporter/validator_test.go | Adds focused validation and core tests around the Boole fork boundary. |
| api/handlers/exporter/committee_http.go | Clarifies unfiltered committee response cardinality in the endpoint documentation. |
| docs/api/ssvnode.openapi.json | Regenerates the JSON OpenAPI description consistently with the handler annotation. |
| docs/api/ssvnode.openapi.yaml | Regenerates the YAML OpenAPI description consistently with the handler annotation. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A["Validator traces request"] --> B{"from is post-Boole committee duty?"}
B -->|Yes, no filters| C["Reject request"]
B -->|No| D["Iterate from through to"]
D --> E{"Slot is post-Boole committee duty?"}
E -->|Yes, no filters| F["Append non-fatal per-slot note"]
E -->|No| G["Read validator traces"]
F --> D
G --> D
D --> H{"Only notes and no traces?"}
H -->|Yes| I["HTTP 200 with notes"]
H -->|No genuine errors| J["HTTP 500"]
H -->|Traces available| K["HTTP 200 partial response"]
Reviews (1): Last reviewed commit: "api/exporter: document the /committee pe..." | Re-trigger Greptile
Codecov Report❌ Patch coverage is
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
A fork-straddling range without pubkeys/indices previously appended one non-fatal note per post-fork slot per role, growing the response linearly with the range size. Fork state is monotonic in slot, so the skipped tail is contiguous: record where it starts and emit a single note per role covering the whole post-fork range instead.
iurii-ssv
left a comment
There was a problem hiding this comment.
Check out the Greptile comment above + left two more minor nits.
ovidiu-ssv-labs
left a comment
There was a problem hiding this comment.
The core mechanism is sound — evaluating the fork gate at From and emitting per-slot non-fatal notes is the right shape, the sentinel/errors.Is design is clean, and the test-network cloning is genuinely leak-free. But the PR documents the smaller back-compat deviation (/committee cardinality) while leaving the larger one (/traces/validator 400 -> 200-with-empty-data) undocumented, the test guarding the new 500 escape hatch doesn't actually assert the status code, and /decideds remains fork-blind for the same two roles this PR just made fork-aware on /traces/validator. [verdict: with_fixes]
Finding 1 · [IMPORTANT] Document the /traces/validator 400 -> 200 contract change in the OpenAPI description — api/handlers/exporter/validator_http.go:15
The PR documents the smaller back-compat break and leaves the larger one undocumented.
Item 2 of this PR adds an OpenAPI note to /committee because "cardinality back-compat does not hold." That is the right instinct — but item 1 changes a status code, which is a strictly harder break for clients, and gets no doc change at all. validator_http.go:15 still reads:
// @Description Returns consensus, decided, and message traces for the requested validator duties.The behavior change, concretely. For roles=[AGGREGATOR] (or SYNC_COMMITTEE_CONTRIBUTION) with no pubkeys/indices:
| request window | before | after |
|---|---|---|
| fully pre-Boole | 200 + data | 200 + data (unchanged) |
| straddles Boole | 400 | 200, pre-fork data only, post-fork slots reported as strings in errors |
| fully post-Boole | 400 | 400 (unchanged) |
Why it matters. The 400 was a contract signal: "you must supply indices/pubkeys or this data is not retrievable." A client that treated non-2xx as "my query is wrong, fix it" now receives a 200 and, in the sparse-aggregator case that TestExporterValidatorTraces_ForkGating_ZeroPreForkTraces covers, data: [] with a populated errors array. A dashboard that only reads data will render an empty chart and report success — silently missing every post-fork aggregator duty in the window.
There is also an undocumented cliff: a window starting one slot before the fork returns 200-with-notes, while the same window shifted one slot forward returns 400. Nothing tells an API consumer that.
Additional wrinkle: ValidatorTracesResponse.Errors is a flat []string (validator_model.go:63). Post-fork notes and genuine store failures land in the same untyped array, so a client cannot programmatically distinguish "partial coverage, expected" from "the store is broken." The sentinel exists in Go (ErrPostForkCommitteeDutyNote) but is flattened to a string at the API boundary.
Suggested fix: Extend the godoc description and regenerate via make openapi, mirroring what item 2 did for /committee:
// @Description Returns consensus, decided, and message traces for the requested validator duties.
// @Description For AGGREGATOR and SYNC_COMMITTEE_CONTRIBUTION the fork state is evaluated at 'from': a range whose
// @Description 'from' is post-Boole and that supplies no 'pubkeys'/'indices' is rejected with 400. A range whose
// @Description 'from' is pre-Boole is accepted and served partially — the post-Boole slots are omitted from 'data'
// @Description and reported per slot in 'errors' with the text "committee duty post-fork". Such a response is a 200
// @Description even when 'data' is empty; clients must inspect 'errors' to detect partial coverage, and should
// @Description supply 'indices'/'pubkeys' or use /traces/committee to retrieve the post-fork portion.If you want the stronger fix, promote the notes out of the flat errors array into a typed field (e.g. "partial": [{"slot": N, "role": "AGGREGATOR", "reason": "post_fork_committee_duty"}]) so clients can branch on it without string matching. Given this is a new field, additive back-compat holds.
Finding 2 · [IMPORTANT] /decideds is still fork-blind for AGGREGATOR / SYNC_COMMITTEE_CONTRIBUTION — exporter/decided.go:41
Adjacent gap — pre-existing, not introduced here, but this PR makes it the last fork-blind routing site in the package.
After this PR, isCommitteeDutyAtSlot (exporter/validator.go:278) is the only Boole-aware routing point in exporter/.
TraceDecidedsCore routes on a hardcoded role switch with no slot/fork input:
// exporter/decided.go:41-46
switch role {
case spectypes.BNRoleAttester, spectypes.BNRoleSyncCommittee:
roleParticipantsIdx, roleErrs = e.getCommitteeDecidedsForRole(slot, indices, role)
default:
roleParticipantsIdx, roleErrs = e.getValidatorDecidedsForRole(slot, indices, role)
}Mechanism. Post-Boole, aggregator and sync-committee-contribution duties execute under the RoleAggregatorCommittee runner (protocol/v2/ssv/runner/aggregator_committee.go:86), and the observer stores their traces on the committee path (protocol/v2/ssv/validator/committee_observer.go:117 gates RoleCommittee || RoleAggregatorCommittee onto the committee-ID branch). So post-fork there is no ValidatorDutyTrace for BNRoleAggregator to find. Yet the default arm sends it to getValidatorDecidedsForRole -> GetAllValidatorDecideds(BNRoleAggregator, slot) -> c.store.GetValidatorDuties(role, slot) (exporter/dutytracer/store.go:421-424), which reads the validator-duty index and returns nothing.
The strongest evidence that this is a bug and not intended: the store layer is already built to serve these roles from the committee side. committeeRunnerRolesForBeaconRoles explicitly maps aggregator-family beacon roles to RoleAggregatorCommittee, and GetCommitteeDecideds defaults to []RunnerRole{RoleCommittee, RoleAggregatorCommittee}. There is even a dedicated test, TestCollector_GetCommitteeDecideds_RoleFiltering, asserting GetCommitteeDecideds(slot, index, spectypes.BNRoleAggregator) works. Nothing in production calls it — the decided.go:42 switch never routes BNRoleAggregator to the committee path.
Impact. Post-Boole, /v1/exporter/decideds with roles=AGGREGATOR or SYNC_COMMITTEE_CONTRIBUTION returns an empty participants list with a 200. Silent data loss — no error, no note, no 400. Worse than the /traces/validator case this PR just fixed, because there the user at least gets a note. Any duty-syncer or dashboard consuming decideds for these roles will show zero participation post-fork and conclude the operators stopped performing the duty.
Scope. This is outside items 1-2 of #2968 and not in this diff, so not blocking — but it is the natural item 3 of the same umbrella and should be filed before Boole activates, not after.
Suggested fix: Route the decideds switch through the same fork-aware predicate the traces path now uses, so there is exactly one place that knows about Boole:
for _, role := range request.Roles {
for s := request.From; s <= request.To; s++ {
slot := phase0.Slot(s)
var roleParticipantsIdx []dutytracer.ParticipantsRangeIndexEntry
var roleErrs *multierror.Error
if e.isCommitteeDutyAtSlot(role, slot) {
roleParticipantsIdx, roleErrs = e.getCommitteeDecidedsForRole(slot, indices, role)
} else {
roleParticipantsIdx, roleErrs = e.getValidatorDecidedsForRole(slot, indices, role)
}
...This is behavior-preserving pre-Boole (isCommitteeDutyAtSlot returns true unconditionally for ATTESTER/SYNC_COMMITTEE and false for the aggregator family pre-fork), and correct post-Boole. Note the loop nesting in decided.go is role-outer/slot-inner, the inverse of validator.go, so isCommitteeDutyAtSlot must be evaluated inside the slot loop as above. Add a straddling-range test mirroring TestValidatorTracesCore_StraddlingFork.
If you'd rather keep this PR tight, file it as item 3 of #2968 and land it separately — but before Boole activates on mainnet.
3 findings approved — 2 could not be inlined (target line not in this diff), 1 inlined below.
…error test The fork gate moved from the range's upper bound to the lower bound; two test doc comments still described the old behavior. Also assert the exact status code and surfaced message in the 'genuine error alongside notes' subtest, so a future misroute to the 400 branch can't pass silently (require.Error alone was satisfied by either branch).
A fork-straddling range without pubkeys/indices used to be rejected with 400; it now returns 200 with the pre-fork portion and per-role notes in 'errors'. That is a status-code contract change for clients, so spell it out in the OpenAPI description like the /committee cardinality note.
|
@ovidiu-ssv-labs re the two findings without inline threads: Finding 1: fixed in 32c9d96 — extended the /traces/validator OpenAPI description with the partial-coverage contract, mirroring the /committee note. One wording change vs. the suggestion: since 82535ff the notes are aggregated into one per role covering the post-fork tail, rather than one per slot. The typed Finding 2: agreed on all points — filed as #2987 (item 3 of the #2968 umbrella) so it lands before Boole activates. |
iurii-ssv
left a comment
There was a problem hiding this comment.
Couple more things to check out.
| continue | ||
| } | ||
| delete(postForkNoteFrom, role) // guard against duplicate roles in the request | ||
| errs = multierror.Append(errs, fmt.Errorf("%w: slots %d-%d: role %s is a committee duty post-fork, please provide either pubkeys or indices to filter the duty for a specific validators subset or use the /committee endpoint to query all the corresponding duties", ErrPostForkCommitteeDutyNote, noteFrom, request.To, role.String())) |
There was a problem hiding this comment.
[nit] The tail of this note ("please provide either pubkeys or indices to filter the duty ... or use the /committee endpoint ...") is duplicated verbatim from the 400 message in validateValidatorRequest (the role %s is a committee duty return just below). Consider hoisting the shared sentence into a package-level const so the note and the validation error can't drift apart.
|
|
||
| var resp ValidatorTracesResponse | ||
| require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &resp)) | ||
| require.Empty(t, resp.Data) |
There was a problem hiding this comment.
Coverage gap: every 200 case in these fork tests asserts require.Empty(t, resp.Data) (here, the CrossForkRange "without filters" subtest, and TestValidatorTracesCore_StraddlingFork). The shape that is never exercised is the actual partial-coverage contract - an unfiltered straddling range whose pre-fork slots return real traces and whose post-fork tail emits a note, i.e. resp.Data non-empty while resp.Errors carries the note. The "with indices" subtest hits the committee path but via the filtered route (which never emits a note) and only asserts routing. Worth one case that seeds pre-fork trace data so data + note coexist.
| if len(request.PubKeys) == 0 && len(request.Indices) == 0 { | ||
| for _, role := range request.Roles { | ||
| if e.isCommitteeDutyAtSlot(role, phase0.Slot(request.To)) { | ||
| if e.isCommitteeDutyAtSlot(role, phase0.Slot(request.From)) { |
There was a problem hiding this comment.
Follow-on to the note-amplification thread / #2986 — not re-raising the endpoint-wide range bound, but one nuance that disposition doesn't capture: moving this gate from request.To to request.From regresses a specific input class from a clean 400 into a hang.
An unfiltered committee-duty request (roles:[AGGREGATOR], no indices/pubkeys) with a pre-Boole from and to = math.MaxUint64 used to be rejected here with 400 (the old gate evaluated to, which is post-Boole). It's now accepted and falls into the inclusive for s := request.From; s <= request.To; s++ loop, where s++ wraps past MaxUint64 and s <= request.To stays true forever — an infinite loop pinning a goroutine at 100% CPU, with no ctx/timeout escape.
The DoS itself is pre-existing (already reachable via any filtered request, or an unfiltered PROPOSER, with to = MaxUint64), so this is low-severity and non-blocking — leaving the general fix to #2986 is fine. But since this PR is what removes the 400 that shielded the unfiltered path, a cheap local guard in validateValidatorRequest (reject to == math.MaxUint64, or to - from above a sane cap, next to the existing from > to check) would stop this PR from regressing the case while the endpoint-wide fix waits.
Items 1 and 2 of #2968, one commit each.
Item 1 —
validateValidatorRequestevaluated the committee-duty fork gate atrequest.To, so a validator-traces window whose tail crossed the Boole fork was rejected wholesale: a dashboard polling a fixed window starts 400-ing the moment its tail crosses the fork, losing the still-servable pre-fork slots. The gate now evaluates atrequest.From, and each post-fork committee-duty slot lacking pubkeys/indices is reported as a non-fatal per-slot note (ErrPostForkCommitteeDutyNote,errors.Is-able) in the responseErrorsinstead of being silently empty. The HTTP handler treats a response whose only errors are such notes as a partial 200 — review caught that without this, a straddling range with zero pre-fork traces (common: aggregator duties are sparse) would have 500'd.from-already-post-fork and unfiltered ATTESTER/SYNC_COMMITTEE requests reject exactly as before.Item 2 — docs-only: the
/committeeOpenAPI description now states that absent arolesfilter the endpoint returns one trace per (slot, committeeID, role) — up to two rows per (slot, committeeID) post-fork, distinguished by therolefield. Additive-field back-compat holds; cardinality back-compat does not, hence the note. Regenerated viamake openapi, diff limited to description text.Covered by fork-phase tests (per-test
TestNetworkcopies withForks.Boolepinned), including HTTP-level cases for the straddling 200-partial path and the genuine-error-still-500 path.