-
Notifications
You must be signed in to change notification settings - Fork 150
exporter: fork-straddling trace ranges + /committee cardinality doc (#2968 items 1, 2) #2975
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: stage
Are you sure you want to change the base?
Changes from all commits
3f5e23f
52ae129
0feb3c7
82535ff
8e3d984
9de8c0c
32c9d96
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
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 upper bound) mirrors the same fork-gated routing decision for aggregator-family | ||
| // (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 upper bound, so aggregator-family roles require pubkeys/indices, and with | ||
| // indices provided each slot routes independently — validator path before the boundary, | ||
| // committee path from it onward. | ||
| // 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 requires pubkeys/indices", func(t *testing.T) { | ||
| 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() | ||
|
|
||
| require.Error(t, exp.ValidatorTraces(rec, req)) | ||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Coverage gap: every 200 case in these fork tests asserts |
||
| 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 e.isCommitteeDutyAtSlot(role, slot) { | ||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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 |
||
| } | ||
|
|
||
| // 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 upper bound: if any slot in | ||
| // [from, to] is post-Boole, the 'to' slot is too. | ||
| // 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.To)) { | ||
| if e.isCommitteeDutyAtSlot(role, phase0.Slot(request.From)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| 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.