exporter: fork-straddling trace ranges + /committee cardinality doc (#2968 items 1, 2) #2975
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2485,7 +2485,7 @@ func TestExporterValidatorTraces_ForkGating(t *testing.T) { | |
| } | ||
|
|
||
| // TestExporterValidatorTraces_ForkGating_ValidationSymmetric proves that validateValidatorRequest | ||
| // (via isCommitteeDutyAtSlot at the range's lower bound) mirrors the same fork-gated routing decision for aggregator-family | ||
| // roles: pre-Boole no pubkeys/indices are required, post-Boole they are (mirroring committee duties). | ||
| func TestExporterValidatorTraces_ForkGating_ValidationSymmetric(t *testing.T) { | ||
| tests := []struct { | ||
|
|
@@ -2567,9 +2567,9 @@ func TestExporterValidatorTraces_ForkGating_ValidationSymmetric(t *testing.T) { | |
|
|
||
| // TestExporterValidatorTraces_ForkGating_CrossForkRange proves the behavior of a slot range | ||
| // straddling the Boole fork boundary (from pre-Boole, to post-Boole): validation is evaluated | ||
| // at the range's lower bound, so unfiltered aggregator-family requests are accepted and served | ||
| // partially — post-fork slots are reported as non-fatal notes — and with indices provided each | ||
| // slot routes independently: validator path before the boundary, committee path from it onward. | ||
| func TestExporterValidatorTraces_ForkGating_CrossForkRange(t *testing.T) { | ||
| const booleEpoch = phase0.Epoch(5) | ||
| idx := phase0.ValidatorIndex(1) | ||
|
|
@@ -2613,7 +2613,7 @@ func TestExporterValidatorTraces_ForkGating_CrossForkRange(t *testing.T) { | |
| } | ||
|
|
||
| for _, role := range roles { | ||
| t.Run(role.name+" without filters returns a partial response with post-fork notes", func(t *testing.T) { | ||
| exp := newTestExporterForV2WithNetwork(newMockTraceStore(), newMockValidatorStore(), &netCfg) | ||
|
|
||
| req := httptest.NewRequest(http.MethodPost, "/traces/validator", buildJSONBody(t, map[string]any{ | ||
|
|
@@ -2624,7 +2624,20 @@ func TestExporterValidatorTraces_ForkGating_CrossForkRange(t *testing.T) { | |
| req.Header.Set("Content-Type", "application/json") | ||
| rec := httptest.NewRecorder() | ||
|
|
||
| // the pre-fork portion of the range legitimately yields zero traces (no | ||
| // mock data), so the post-fork "requires pubkeys/indices" notes must not | ||
| // be treated as a hard failure: expect 200 with empty data and the notes | ||
| // surfaced in Errors. | ||
| require.NoError(t, exp.ValidatorTraces(rec, req)) | ||
| require.Equal(t, http.StatusOK, rec.Code) | ||
|
|
||
| var resp ValidatorTracesResponse | ||
| require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &resp)) | ||
| require.Empty(t, resp.Data) | ||
| require.NotEmpty(t, resp.Errors) | ||
| for _, msg := range resp.Errors { | ||
| require.Contains(t, msg, "committee duty post-fork") | ||
| } | ||
| }) | ||
|
|
||
| t.Run(role.name+" with indices routes each slot by its own fork state", func(t *testing.T) { | ||
|
|
@@ -2671,6 +2684,75 @@ func TestExporterValidatorTraces_ForkGating_CrossForkRange(t *testing.T) { | |
| } | ||
| } | ||
|
|
||
| // TestExporterValidatorTraces_ForkGating_ZeroPreForkTraces covers the fix for a | ||
| // fork-straddling AGGREGATOR/SYNC_COMMITTEE_CONTRIBUTION request without | ||
| // pubkeys/indices whose pre-fork slots legitimately yield zero traces (e.g. | ||
| // sparse aggregator duties): the response must be 200 with empty traces and | ||
| // the post-fork notes surfaced, not a 500. A genuine error alongside those | ||
| // notes must still yield 500. | ||
| func TestExporterValidatorTraces_ForkGating_ZeroPreForkTraces(t *testing.T) { | ||
| const booleEpoch = phase0.Epoch(5) | ||
|
|
||
| ssvCopy := *networkconfig.TestNetwork.SSV | ||
| ssvCopy.Forks.Boole = booleEpoch | ||
| netCfg := *networkconfig.TestNetwork | ||
| netCfg.SSV = &ssvCopy | ||
|
|
||
| booleSlot := netCfg.FirstSlotAtEpoch(booleEpoch) | ||
| require.GreaterOrEqual(t, uint64(booleSlot), uint64(2), "boole fork slot too low for the range below") | ||
| from := uint64(booleSlot) - 2 // pre-Boole | ||
| to := uint64(booleSlot) + 2 // post-Boole | ||
|
|
||
| t.Run("only post-fork notes and no pre-fork traces -> 200 with empty data", func(t *testing.T) { | ||
| exp := newTestExporterForV2WithNetwork(newMockTraceStore(), newMockValidatorStore(), &netCfg) | ||
|
|
||
| req := httptest.NewRequest(http.MethodPost, "/traces/validator", buildJSONBody(t, map[string]any{ | ||
| "from": from, | ||
| "to": to, | ||
| "roles": []string{"AGGREGATOR"}, | ||
| })) | ||
| req.Header.Set("Content-Type", "application/json") | ||
| rec := httptest.NewRecorder() | ||
|
|
||
| require.NoError(t, exp.ValidatorTraces(rec, req)) | ||
| require.Equal(t, http.StatusOK, rec.Code) | ||
|
|
||
| var resp ValidatorTracesResponse | ||
| require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &resp)) | ||
| require.Empty(t, resp.Data) | ||
|
Contributor
There was a problem hiding this comment. Coverage gap: every 200 case in these fork tests asserts
Contributor
Author
There was a problem hiding this comment. Fixed in 5b4c59a. |
||
| require.NotEmpty(t, resp.Errors, "expected post-fork notes to surface") | ||
| for _, msg := range resp.Errors { | ||
| require.Contains(t, msg, "committee duty post-fork") | ||
| } | ||
| }) | ||
|
|
||
| t.Run("genuine error alongside notes still yields 500", func(t *testing.T) { | ||
| store := newMockTraceStore() | ||
| store.GetValidatorDutiesFunc = func(role spectypes.BeaconRole, slot phase0.Slot) ([]*traces.ValidatorDutyTrace, error) { | ||
| return nil, fmt.Errorf("forced error on GetValidatorDuties") | ||
| } | ||
| exp := newTestExporterForV2WithNetwork(store, newMockValidatorStore(), &netCfg) | ||
|
|
||
| req := httptest.NewRequest(http.MethodPost, "/traces/validator", buildJSONBody(t, map[string]any{ | ||
| "from": from, | ||
| "to": to, | ||
| "roles": []string{"AGGREGATOR"}, | ||
| })) | ||
| req.Header.Set("Content-Type", "application/json") | ||
| rec := httptest.NewRecorder() | ||
|
|
||
| err := exp.ValidatorTraces(rec, req) | ||
| require.Error(t, err) | ||
|
|
||
| var apiErr *api.ErrorResponse | ||
| require.ErrorAs(t, err, &apiErr) | ||
| require.Equal(t, http.StatusInternalServerError, apiErr.Code, | ||
| "a genuine store failure must not be masked by the post-fork note exemption") | ||
| require.Contains(t, apiErr.Message, "forced error on GetValidatorDuties", | ||
| "the genuine error, not a post-fork note, must surface to the caller") | ||
| }) | ||
| } | ||
|
|
||
| // mockValidatorStore is a simple in-memory ValidatorStore implementation for tests. | ||
| type mockValidatorStore struct { | ||
| byIndex map[phase0.ValidatorIndex]*ssvtypes.SSVShare | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| package exporter | ||
|
|
||
| import ( | ||
| "errors" | ||
| "fmt" | ||
| "slices" | ||
|
|
||
|
|
@@ -16,6 +17,12 @@ import ( | |
| ssvtypes "github.com/ssvlabs/ssv/protocol/v2/types" | ||
| ) | ||
|
|
||
| // ErrPostForkCommitteeDutyNote marks the non-fatal note appended when a | ||
| // fork-straddling request reaches a post-fork slot/role pair without | ||
| // pubkeys/indices. It lets callers (e.g. the HTTP layer) tell this expected, | ||
| // partial-coverage note apart from genuine processing failures. | ||
| var ErrPostForkCommitteeDutyNote = errors.New("committee duty post-fork requires pubkeys or indices") | ||
|
|
||
| // ValidatorTracesCore contains the core logic for ValidatorTraces without any HTTP concerns. | ||
| func (e *Exporter) ValidatorTracesCore(request *ValidatorTracesQuery) (*ValidatorTracesResult, *multierror.Error) { | ||
| if err := e.validateValidatorRequest(request); err != nil { | ||
|
|
@@ -33,11 +40,26 @@ func (e *Exporter) ValidatorTracesCore(request *ValidatorTracesQuery) (*Validato | |
| return nil, multierror.Append(nil, &ValidationError{Err: indicesErr}) | ||
| } | ||
|
|
||
| // request validation only gates on 'from': a window whose tail crosses | ||
| // Boole reaches post-fork slots without pubkeys/indices. Fork state is | ||
| // monotonic in slot, so that tail is one contiguous range per role — | ||
| // record where it starts and report it as a single non-fatal note per | ||
| // role below, rather than allocating one note per skipped slot. | ||
| postForkNoteFrom := map[spectypes.BeaconRole]phase0.Slot{} | ||
|
|
||
| for s := request.From; s <= request.To; s++ { | ||
| slot := phase0.Slot(s) | ||
| for _, role := range request.Roles { | ||
| isCommittee := e.isCommitteeDutyAtSlot(role, slot) | ||
| if isCommittee && len(indices) == 0 { | ||
| if _, ok := postForkNoteFrom[role]; !ok { | ||
| postForkNoteFrom[role] = slot | ||
| } | ||
| continue | ||
| } | ||
|
|
||
| providerFunc := e.getValidatorDutiesForRoleAndSlot | ||
| if isCommittee { | ||
| providerFunc = e.getValidatorCommitteeDutiesForRoleAndSlot | ||
| } | ||
|
|
||
|
|
@@ -47,6 +69,15 @@ func (e *Exporter) ValidatorTracesCore(request *ValidatorTracesQuery) (*Validato | |
| } | ||
| } | ||
|
|
||
| for _, role := range request.Roles { | ||
| noteFrom, ok := postForkNoteFrom[role] | ||
| if !ok { | ||
| 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())) | ||
|
Contributor
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
Contributor
Author
There was a problem hiding this comment. Good call. Fixed in 36e901d. |
||
| } | ||
|
|
||
| // by design, not found duties are expected and not considered as API errors | ||
| errs = filterOutDutyNotFoundErrors(errs) | ||
|
|
||
|
|
@@ -66,11 +97,12 @@ func (e *Exporter) validateValidatorRequest(request *ValidatorTracesQuery) error | |
| } | ||
|
|
||
| // either PubKeys or Indices are required for committee duty roles. | ||
| // Fork state is evaluated at the range's lower bound so that a window | ||
| // whose tail crosses Boole still serves its pre-fork portion; the | ||
| // post-fork tail is reported as a non-fatal note in the per-slot loop. | ||
| if len(request.PubKeys) == 0 && len(request.Indices) == 0 { | ||
| for _, role := range request.Roles { | ||
| if e.isCommitteeDutyAtSlot(role, phase0.Slot(request.From)) { | ||
|
Contributor
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 An unfiltered committee-duty request ( The DoS itself is pre-existing (already reachable via any filtered request, or an unfiltered
Contributor
Author
There was a problem hiding this comment. Fair point. The endpoint-wide range cap stays with #2986. |
||
| return fmt.Errorf("role %s is a committee duty, 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", role.String()) | ||
| } | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.