-
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 2 commits
3f5e23f
52ae129
0feb3c7
82535ff
8e3d984
9de8c0c
32c9d96
36e901d
36f7968
5b4c59a
f4edfb6
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 |
|---|---|---|
|
|
@@ -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,67 @@ 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
Contributor
Author
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. 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() | ||
|
|
||
| require.Error(t, exp.ValidatorTraces(rec, req)) | ||
|
iurii-ssv marked this conversation as resolved.
Outdated
|
||
| }) | ||
| } | ||
|
|
||
| // 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 { | ||
|
|
@@ -36,6 +43,14 @@ func (e *Exporter) ValidatorTracesCore(request *ValidatorTracesQuery) (*Validato | |
| for s := request.From; s <= request.To; s++ { | ||
| slot := phase0.Slot(s) | ||
| for _, role := range request.Roles { | ||
| if e.isCommitteeDutyAtSlot(role, slot) && len(indices) == 0 { | ||
|
iurii-ssv marked this conversation as resolved.
Outdated
|
||
| // 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())) | ||
|
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.
When an external request supplies a pre-Boole How this was verified: The request bounds have no range-size constraint, validation checks only
Contributor
Author
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. |
||
| continue | ||
| } | ||
|
|
||
| providerFunc := e.getValidatorDutiesForRoleAndSlot | ||
| if e.isCommitteeDutyAtSlot(role, slot) { | ||
| providerFunc = e.getValidatorCommitteeDutiesForRoleAndSlot | ||
|
|
@@ -66,11 +81,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
Contributor
Author
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. 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.