diff --git a/api/handlers/exporter/committee_http.go b/api/handlers/exporter/committee_http.go index 5ac2f97a5c..bfb64f3519 100644 --- a/api/handlers/exporter/committee_http.go +++ b/api/handlers/exporter/committee_http.go @@ -10,7 +10,7 @@ import ( // CommitteeTraces godoc // @Summary Retrieve committee duty traces -// @Description Returns consensus and post-consensus traces for requested committees. +// @Description Returns consensus and post-consensus traces for requested committees. Without a 'roles' filter, the response contains one trace per (slot, committeeID, role) - up to two rows per (slot, committeeID), distinguished by the 'role' field. // @Tags Exporter // @Accept json // @Produce json diff --git a/api/handlers/exporter/exporter_test.go b/api/handlers/exporter/exporter_test.go index 991197eeaf..11d4064c78 100644 --- a/api/handlers/exporter/exporter_test.go +++ b/api/handlers/exporter/exporter_test.go @@ -625,6 +625,16 @@ func newTestExporterForV2WithNetwork(traceStore *mockTraceStore, validators stor return NewExporter(zap.NewNop(), nil, traceStore, validators, netCfg) } +// booleNetwork returns a clone of TestNetwork with the Boole fork pinned at the +// given epoch. The SSV config is copied too so the shared TestNetwork is never mutated. +func booleNetwork(epoch phase0.Epoch) *networkconfig.Network { + ssvCopy := *networkconfig.TestNetwork.SSV + ssvCopy.Forks.Boole = epoch + netCfg := *networkconfig.TestNetwork + netCfg.SSV = &ssvCopy + return &netCfg +} + func buildJSONBody(t *testing.T, payload map[string]any) *strings.Reader { t.Helper() b, err := json.Marshal(payload) @@ -2450,12 +2460,7 @@ func TestExporterValidatorTraces_ForkGating(t *testing.T) { return &traces.CommitteeDutyTrace{Slot: s, CommitteeID: id}, nil } - ssvCopy := *networkconfig.TestNetwork.SSV - ssvCopy.Forks.Boole = tt.booleEpoch - netCfg := *networkconfig.TestNetwork - netCfg.SSV = &ssvCopy - - exp := newTestExporterForV2WithNetwork(store, validatorStore, &netCfg) + exp := newTestExporterForV2WithNetwork(store, validatorStore, booleNetwork(tt.booleEpoch)) req := httptest.NewRequest(http.MethodPost, "/traces/validator", buildJSONBody(t, map[string]any{ "from": uint64(slot), @@ -2485,7 +2490,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 { @@ -2539,12 +2544,7 @@ func TestExporterValidatorTraces_ForkGating_ValidationSymmetric(t *testing.T) { } validatorStore := newMockValidatorStore() - ssvCopy := *networkconfig.TestNetwork.SSV - ssvCopy.Forks.Boole = tt.booleEpoch - netCfg := *networkconfig.TestNetwork - netCfg.SSV = &ssvCopy - - exp := newTestExporterForV2WithNetwork(store, validatorStore, &netCfg) + exp := newTestExporterForV2WithNetwork(store, validatorStore, booleNetwork(tt.booleEpoch)) req := httptest.NewRequest(http.MethodPost, "/traces/validator", buildJSONBody(t, map[string]any{ "from": uint64(100), @@ -2567,20 +2567,16 @@ 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) var committeeID spectypes.CommitteeID committeeID[0] = 7 - ssvCopy := *networkconfig.TestNetwork.SSV - ssvCopy.Forks.Boole = booleEpoch - netCfg := *networkconfig.TestNetwork - netCfg.SSV = &ssvCopy - + netCfg := booleNetwork(booleEpoch) booleSlot := netCfg.FirstSlotAtEpoch(booleEpoch) require.GreaterOrEqual(t, uint64(booleSlot), uint64(10), "boole fork slot too low for the range below") from := uint64(booleSlot) - 10 // pre-Boole @@ -2613,8 +2609,8 @@ func TestExporterValidatorTraces_ForkGating_CrossForkRange(t *testing.T) { } for _, role := range roles { - t.Run(role.name+" without filters requires pubkeys/indices", func(t *testing.T) { - exp := newTestExporterForV2WithNetwork(newMockTraceStore(), newMockValidatorStore(), &netCfg) + 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{ "from": from, @@ -2624,7 +2620,58 @@ 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+" without filters serves pre-fork data alongside post-fork notes", func(t *testing.T) { + store := newMockTraceStore() + // the unfiltered pre-fork path reads GetValidatorDuties per slot; + // post-fork slots are skipped with a note and must never reach it. + store.GetValidatorDutiesFunc = func(r spectypes.BeaconRole, slot phase0.Slot) ([]*traces.ValidatorDutyTrace, error) { + require.Less(t, uint64(slot), uint64(booleSlot), "unfiltered validator path used at post-Boole slot") + return []*traces.ValidatorDutyTrace{{Slot: slot, Role: r, Validator: idx}}, nil + } + + exp := newTestExporterForV2WithNetwork(store, newMockValidatorStore(), netCfg) + + req := httptest.NewRequest(http.MethodPost, "/traces/validator", buildJSONBody(t, map[string]any{ + "from": from, + "to": to, + "roles": []string{role.name}, + })) + 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)) + + // the partial-coverage contract: real pre-fork traces in Data while + // the post-fork tail is reported as a note in Errors, in one response. + require.Len(t, resp.Data, int(uint64(booleSlot)-from), "expected one trace per pre-fork slot") + for _, item := range resp.Data { + assert.Less(t, uint64(item.Slot), uint64(booleSlot), "post-fork slot leaked into data") + assert.Equal(t, role.name, item.Role) + } + require.NotEmpty(t, resp.Errors, "expected the post-fork note to surface alongside data") + 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) { @@ -2645,7 +2692,7 @@ func TestExporterValidatorTraces_ForkGating_CrossForkRange(t *testing.T) { return role.signerDataBuilder(s), nil } - exp := newTestExporterForV2WithNetwork(store, newMockValidatorStore(), &netCfg) + exp := newTestExporterForV2WithNetwork(store, newMockValidatorStore(), netCfg) req := httptest.NewRequest(http.MethodPost, "/traces/validator", buildJSONBody(t, map[string]any{ "from": from, @@ -2671,6 +2718,71 @@ 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) + + netCfg := booleNetwork(booleEpoch) + 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) + 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 diff --git a/api/handlers/exporter/validator_http.go b/api/handlers/exporter/validator_http.go index 000d76aeab..b3113f4d47 100644 --- a/api/handlers/exporter/validator_http.go +++ b/api/handlers/exporter/validator_http.go @@ -1,14 +1,24 @@ package exporter import ( + "errors" "net/http" + "github.com/hashicorp/go-multierror" + "github.com/ssvlabs/ssv/api" + exportercore "github.com/ssvlabs/ssv/exporter" ) // ValidatorTraces godoc // @Summary Retrieve validator duty traces // @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, while a range whose +// @Description 'from' is pre-Boole is accepted and served partially — post-Boole slots are omitted from 'data' and +// @Description reported as one note per role in 'errors' with the text "committee duty post-fork". Such a response is +// @Description a 200 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. // @Tags Exporter // @Accept json // @Produce json @@ -39,8 +49,10 @@ func (e *Exporter) ValidatorTraces(w http.ResponseWriter, r *http.Request) error return toApiError(e.logger, r, "validator_traces", http.StatusBadRequest, request, underlyingValidationError(errs)) } - // if we don't have a single valid result and we have at least one meaningful error, return an error - if len(result.Traces) == 0 && errs.ErrorOrNil() != nil { + // if we don't have a single valid result and we have at least one meaningful error, return an error. + // post-fork committee-duty notes are expected on fork-straddling ranges whose pre-fork slots + // yield no traces (e.g. sparse aggregator duties), so they don't count as a hard failure here. + if len(result.Traces) == 0 && errs.ErrorOrNil() != nil && !onlyPostForkCommitteeDutyNotes(errs) { return toApiError(e.logger, r, "validator_traces", http.StatusInternalServerError, request, errs.ErrorOrNil()) } @@ -48,3 +60,18 @@ func (e *Exporter) ValidatorTraces(w http.ResponseWriter, r *http.Request) error response := toValidatorTraceResponse(result, errs) return api.Render(w, r, response) } + +// onlyPostForkCommitteeDutyNotes reports whether every error in errs is (or wraps) +// exportercore.ErrPostForkCommitteeDutyNote, i.e. the errors are non-fatal notes +// rather than genuine processing failures. +func onlyPostForkCommitteeDutyNotes(errs *multierror.Error) bool { + if errs.ErrorOrNil() == nil { + return false + } + for _, err := range errs.Errors { + if !errors.Is(err, exportercore.ErrPostForkCommitteeDutyNote) { + return false + } + } + return true +} diff --git a/docs/api/ssvnode.openapi.json b/docs/api/ssvnode.openapi.json index 7f978c997f..252f1f9826 100644 --- a/docs/api/ssvnode.openapi.json +++ b/docs/api/ssvnode.openapi.json @@ -229,7 +229,7 @@ }, "/v1/exporter/traces/committee": { "get": { - "description": "Returns consensus and post-consensus traces for requested committees.", + "description": "Returns consensus and post-consensus traces for requested committees. Without a 'roles' filter, the response contains one trace per (slot, committeeID, role) - up to two rows per (slot, committeeID), distinguished by the 'role' field.", "consumes": [ "application/json" ], @@ -318,7 +318,7 @@ } }, "post": { - "description": "Returns consensus and post-consensus traces for requested committees.", + "description": "Returns consensus and post-consensus traces for requested committees. Without a 'roles' filter, the response contains one trace per (slot, committeeID, role) - up to two rows per (slot, committeeID), distinguished by the 'role' field.", "consumes": [ "application/json" ], @@ -409,7 +409,7 @@ }, "/v1/exporter/traces/validator": { "get": { - "description": "Returns consensus, decided, and message traces for the requested validator duties.", + "description": "Returns consensus, decided, and message traces for the requested validator duties.\nFor AGGREGATOR and SYNC_COMMITTEE_CONTRIBUTION the fork state is evaluated at 'from': a range whose\n'from' is post-Boole and that supplies no 'pubkeys'/'indices' is rejected with 400, while a range whose\n'from' is pre-Boole is accepted and served partially — post-Boole slots are omitted from 'data' and\nreported as one note per role in 'errors' with the text \"committee duty post-fork\". Such a response is\na 200 even when 'data' is empty; clients must inspect 'errors' to detect partial coverage, and should\nsupply 'indices'/'pubkeys' or use /traces/committee to retrieve the post-fork portion.", "consumes": [ "application/json" ], @@ -514,7 +514,7 @@ } }, "post": { - "description": "Returns consensus, decided, and message traces for the requested validator duties.", + "description": "Returns consensus, decided, and message traces for the requested validator duties.\nFor AGGREGATOR and SYNC_COMMITTEE_CONTRIBUTION the fork state is evaluated at 'from': a range whose\n'from' is post-Boole and that supplies no 'pubkeys'/'indices' is rejected with 400, while a range whose\n'from' is pre-Boole is accepted and served partially — post-Boole slots are omitted from 'data' and\nreported as one note per role in 'errors' with the text \"committee duty post-fork\". Such a response is\na 200 even when 'data' is empty; clients must inspect 'errors' to detect partial coverage, and should\nsupply 'indices'/'pubkeys' or use /traces/committee to retrieve the post-fork portion.", "consumes": [ "application/json" ], diff --git a/docs/api/ssvnode.openapi.yaml b/docs/api/ssvnode.openapi.yaml index 63727a86db..bd94f95e31 100644 --- a/docs/api/ssvnode.openapi.yaml +++ b/docs/api/ssvnode.openapi.yaml @@ -742,6 +742,9 @@ paths: consumes: - application/json description: Returns consensus and post-consensus traces for requested committees. + Without a 'roles' filter, the response contains one trace per (slot, committeeID, + role) - up to two rows per (slot, committeeID), distinguished by the 'role' + field. parameters: - collectionFormat: csv description: CommitteeIDs is a comma-separated list of committee IDs (hex, @@ -805,6 +808,9 @@ paths: consumes: - application/json description: Returns consensus and post-consensus traces for requested committees. + Without a 'roles' filter, the response contains one trace per (slot, committeeID, + role) - up to two rows per (slot, committeeID), distinguished by the 'role' + field. parameters: - collectionFormat: csv description: CommitteeIDs is a comma-separated list of committee IDs (hex, @@ -868,8 +874,14 @@ paths: get: consumes: - application/json - description: Returns consensus, decided, and message traces for the requested - validator duties. + description: |- + Returns consensus, decided, and message traces for the requested validator duties. + For AGGREGATOR and SYNC_COMMITTEE_CONTRIBUTION the fork state is evaluated at 'from': a range whose + 'from' is post-Boole and that supplies no 'pubkeys'/'indices' is rejected with 400, while a range whose + 'from' is pre-Boole is accepted and served partially — post-Boole slots are omitted from 'data' and + reported as one note per role in 'errors' with the text "committee duty post-fork". Such a response is + a 200 even when 'data' is empty; clients must inspect 'errors' to detect partial coverage, and should + supply 'indices'/'pubkeys' or use /traces/committee to retrieve the post-fork portion. parameters: - description: From is the starting slot (inclusive). example: 123456 @@ -944,8 +956,14 @@ paths: post: consumes: - application/json - description: Returns consensus, decided, and message traces for the requested - validator duties. + description: |- + Returns consensus, decided, and message traces for the requested validator duties. + For AGGREGATOR and SYNC_COMMITTEE_CONTRIBUTION the fork state is evaluated at 'from': a range whose + 'from' is post-Boole and that supplies no 'pubkeys'/'indices' is rejected with 400, while a range whose + 'from' is pre-Boole is accepted and served partially — post-Boole slots are omitted from 'data' and + reported as one note per role in 'errors' with the text "committee duty post-fork". Such a response is + a 200 even when 'data' is empty; clients must inspect 'errors' to detect partial coverage, and should + supply 'indices'/'pubkeys' or use /traces/committee to retrieve the post-fork portion. parameters: - description: From is the starting slot (inclusive). example: 123456 diff --git a/exporter/committee.go b/exporter/committee.go index c4c51d7fbf..111aeeeb51 100644 --- a/exporter/committee.go +++ b/exporter/committee.go @@ -1,8 +1,6 @@ package exporter import ( - "fmt" - "github.com/attestantio/go-eth2-client/spec/phase0" "github.com/hashicorp/go-multierror" "go.uber.org/zap" @@ -40,10 +38,7 @@ func (e *Exporter) CommitteeTracesCore(request *CommitteeTracesQuery) (*Committe } func validateCommitteeRequest(request *CommitteeTracesQuery) error { - if request.From > request.To { - return fmt.Errorf("'from' must be less than or equal to 'to'") - } - return nil + return validateSlotRange(request.From, request.To) } func (e *Exporter) getCommitteeDutiesForSlot(slot phase0.Slot, committeeIDs []spectypes.CommitteeID, roles ...spectypes.RunnerRole) ([]*traces.CommitteeDutyTrace, error) { diff --git a/exporter/decided.go b/exporter/decided.go index 1760987ada..67dfc8ea98 100644 --- a/exporter/decided.go +++ b/exporter/decided.go @@ -119,8 +119,8 @@ func (e *Exporter) DecidedsCore(request *DecidedsQuery) (*TraceDecidedsResult, e } func validateDecidedRequest(request *DecidedsQuery) error { - if request.From > request.To { - return fmt.Errorf("'from' must be less than or equal to 'to'") + if err := validateSlotRange(request.From, request.To); err != nil { + return err } if len(request.Roles) == 0 { diff --git a/exporter/validation.go b/exporter/validation.go index fdda0ed4bd..f8efcf1976 100644 --- a/exporter/validation.go +++ b/exporter/validation.go @@ -1,5 +1,25 @@ package exporter +import ( + "fmt" + "math" +) + +// validateSlotRange checks the inclusive [from, to] slot window shared by +// every exporter range endpoint. The per-slot loops are inclusive of 'to', +// so the maximum uint64 value would wrap the counter and never terminate. +// An endpoint-wide range-size bound is tracked in #2986; this only rejects +// an inverted window and the guaranteed hang. +func validateSlotRange(from, to uint64) error { + if from > to { + return fmt.Errorf("'from' must be less than or equal to 'to'") + } + if to == math.MaxUint64 { + return fmt.Errorf("'to' must be less than %d", uint64(math.MaxUint64)) + } + return nil +} + // ValidationError wraps an underlying error to indicate that a request is semantically invalid. // It allows callers to distinguish validation errors from processing errors using errors.As. type ValidationError struct { diff --git a/exporter/validation_test.go b/exporter/validation_test.go new file mode 100644 index 0000000000..e97ac1b85a --- /dev/null +++ b/exporter/validation_test.go @@ -0,0 +1,52 @@ +package exporter + +import ( + "math" + "testing" + + "github.com/stretchr/testify/require" + + spectypes "github.com/ssvlabs/ssv-spec/types" +) + +func TestValidateSlotRange(t *testing.T) { + tests := []struct { + name string + from uint64 + to uint64 + wantErr string + }{ + {name: "single slot", from: 7, to: 7}, + {name: "ascending range", from: 1, to: 100}, + {name: "largest terminating 'to'", from: 1, to: math.MaxUint64 - 1}, + {name: "from greater than to", from: 10, to: 5, wantErr: "'from' must be less than or equal to 'to'"}, + {name: "'to' of max uint64 would wrap the inclusive loop", from: 1, to: math.MaxUint64, wantErr: "'to' must be less than"}, + {name: "'from' and 'to' both max uint64", from: math.MaxUint64, to: math.MaxUint64, wantErr: "'to' must be less than"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateSlotRange(tt.from, tt.to) + if tt.wantErr == "" { + require.NoError(t, err) + return + } + require.ErrorContains(t, err, tt.wantErr) + }) + } +} + +// The committee and decideds endpoints run the same inclusive per-slot loop as +// /traces/validator, so their validators must share the max-uint64 guard. +func TestValidateCommitteeRequest_SlotRange(t *testing.T) { + require.NoError(t, validateCommitteeRequest(&CommitteeTracesQuery{From: 1, To: 2})) + require.ErrorContains(t, validateCommitteeRequest(&CommitteeTracesQuery{From: 2, To: 1}), "'from' must be less than or equal to 'to'") + require.ErrorContains(t, validateCommitteeRequest(&CommitteeTracesQuery{From: 1, To: math.MaxUint64}), "'to' must be less than") +} + +func TestValidateDecidedRequest_SlotRange(t *testing.T) { + roles := []spectypes.BeaconRole{spectypes.BNRoleProposer} + require.NoError(t, validateDecidedRequest(&DecidedsQuery{From: 1, To: 2, Roles: roles})) + require.ErrorContains(t, validateDecidedRequest(&DecidedsQuery{From: 2, To: 1, Roles: roles}), "'from' must be less than or equal to 'to'") + require.ErrorContains(t, validateDecidedRequest(&DecidedsQuery{From: 1, To: math.MaxUint64, Roles: roles}), "'to' must be less than") +} diff --git a/exporter/validator.go b/exporter/validator.go index 8d73858e01..8e71df9072 100644 --- a/exporter/validator.go +++ b/exporter/validator.go @@ -1,6 +1,7 @@ package exporter import ( + "errors" "fmt" "slices" @@ -16,6 +17,17 @@ 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") + +// committeeDutyFilterHint is the actionable tail shared by the committee-duty +// validation error and the post-fork partial-coverage note, hoisted so the two +// messages cannot drift apart. +const committeeDutyFilterHint = "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" + // 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 +45,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 +74,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, %s", ErrPostForkCommitteeDutyNote, noteFrom, request.To, role.String(), committeeDutyFilterHint)) + } + // by design, not found duties are expected and not considered as API errors errs = filterOutDutyNotFoundErrors(errs) @@ -57,8 +93,8 @@ func (e *Exporter) ValidatorTracesCore(request *ValidatorTracesQuery) (*Validato } func (e *Exporter) validateValidatorRequest(request *ValidatorTracesQuery) error { - if request.From > request.To { - return fmt.Errorf("'from' must be less than or equal to 'to'") + if err := validateSlotRange(request.From, request.To); err != nil { + return err } if len(request.Roles) == 0 { @@ -66,12 +102,13 @@ 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)) { - 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()) + if e.isCommitteeDutyAtSlot(role, phase0.Slot(request.From)) { + return fmt.Errorf("role %s is a committee duty, %s", role.String(), committeeDutyFilterHint) } } } diff --git a/exporter/validator_test.go b/exporter/validator_test.go index af59da94e4..9297ec2b5a 100644 --- a/exporter/validator_test.go +++ b/exporter/validator_test.go @@ -1,6 +1,8 @@ package exporter import ( + "fmt" + "math" "testing" "github.com/attestantio/go-eth2-client/spec/phase0" @@ -10,6 +12,7 @@ import ( spectypes "github.com/ssvlabs/ssv-spec/types" + "github.com/ssvlabs/ssv/exporter/rolemask" estore "github.com/ssvlabs/ssv/exporter/store" "github.com/ssvlabs/ssv/exporter/traces" "github.com/ssvlabs/ssv/networkconfig" @@ -36,6 +39,18 @@ func postBooleNetwork() *networkconfig.Network { return &netCfg } +// straddlingNetwork returns a *networkconfig.Network clone whose Boole fork +// activates one epoch after "now", so a slot range that starts before the +// fork epoch and ends after it genuinely straddles the fork boundary. It +// never mutates the shared TestNetwork. +func straddlingNetwork() *networkconfig.Network { + ssvCopy := *networkconfig.TestNetwork.SSV + ssvCopy.Forks.Boole = networkconfig.TestNetwork.EstimatedCurrentEpoch() + 1 + netCfg := *networkconfig.TestNetwork + netCfg.SSV = &ssvCopy + return &netCfg +} + func TestIsCommitteeDutyAtSlot(t *testing.T) { preFork := preBooleNetwork() postFork := postBooleNetwork() @@ -73,9 +88,11 @@ func TestIsCommitteeDutyAtSlot(t *testing.T) { func TestValidateValidatorRequest(t *testing.T) { preFork := preBooleNetwork() postFork := postBooleNetwork() + straddle := straddlingNetwork() preForkSlot := uint64(preFork.FirstSlotAtEpoch(1)) postForkSlot := uint64(postFork.FirstSlotAtEpoch(1)) + straddleBoundarySlot := uint64(straddle.FirstSlotAtEpoch(straddle.SSV.Forks.Boole)) tests := []struct { name string @@ -162,7 +179,7 @@ func TestValidateValidatorRequest(t *testing.T) { wantErr: true, }, { - name: "range straddling the fork resolves on the 'to' bound: pre-fork 'to' is accepted", + name: "range straddling the fork resolves on the 'from' bound: pre-fork 'to' is accepted", netCfg: preFork, request: &ValidatorTracesQuery{ From: 1, @@ -172,7 +189,7 @@ func TestValidateValidatorRequest(t *testing.T) { wantErr: false, }, { - name: "range straddling the fork resolves on the 'to' bound: post-fork 'to' is rejected", + name: "range straddling the fork resolves on the 'from' bound: post-fork 'to' is rejected", netCfg: postFork, request: &ValidatorTracesQuery{ From: 1, @@ -181,6 +198,40 @@ func TestValidateValidatorRequest(t *testing.T) { }, wantErr: true, }, + { + name: "range genuinely straddling the fork boundary is accepted (gate evaluated at 'from', not 'to')", + netCfg: straddle, + request: &ValidatorTracesQuery{ + From: straddleBoundarySlot - 1, + To: straddleBoundarySlot + 10, + Roles: []spectypes.BeaconRole{spectypes.BNRoleAggregator}, + }, + wantErr: false, + }, + { + name: "range whose 'from' is already post-fork is rejected even if narrower than 'to'", + netCfg: straddle, + request: &ValidatorTracesQuery{ + From: straddleBoundarySlot, + To: straddleBoundarySlot + 10, + Roles: []spectypes.BeaconRole{spectypes.BNRoleAggregator}, + }, + wantErr: true, + }, + { + // the inclusive per-slot loop would wrap its uint64 counter at the + // maximum 'to' and never terminate; the lower-bound fork gate no + // longer rejects this shape for unfiltered committee-duty roles, + // so validation must. + name: "'to' of max uint64 is rejected regardless of role or filters", + netCfg: preFork, + request: &ValidatorTracesQuery{ + From: 1, + To: math.MaxUint64, + Roles: []spectypes.BeaconRole{spectypes.BNRoleProposer}, + }, + wantErr: true, + }, } for _, tt := range tests { @@ -196,6 +247,53 @@ func TestValidateValidatorRequest(t *testing.T) { } } +// mockCoreTraceStore is a minimal dutyTraceStore implementation for exercising +// ValidatorTracesCore end-to-end over a fork-straddling slot range. +type mockCoreTraceStore struct { + dutyTraceStore +} + +func (m *mockCoreTraceStore) GetValidatorDuties(_ spectypes.BeaconRole, _ phase0.Slot) ([]*traces.ValidatorDutyTrace, error) { + return nil, nil +} + +func (m *mockCoreTraceStore) GetScheduled(_ phase0.Slot) (map[phase0.ValidatorIndex]rolemask.Mask, error) { + return map[phase0.ValidatorIndex]rolemask.Mask{}, nil +} + +func TestValidatorTracesCore_StraddlingFork(t *testing.T) { + straddle := straddlingNetwork() + boundarySlot := straddle.FirstSlotAtEpoch(straddle.SSV.Forks.Boole) + + e := &Exporter{ + traceStore: &mockCoreTraceStore{}, + logger: zap.NewNop(), + networkConfig: straddle, + } + + request := &ValidatorTracesQuery{ + From: uint64(boundarySlot) - 1, + To: uint64(boundarySlot) + 1, + Roles: []spectypes.BeaconRole{spectypes.BNRoleAggregator}, + } + + result, errs := e.ValidatorTracesCore(request) + require.NotNil(t, result) + + // no *ValidationError: the request is accepted despite its tail crossing Boole. + for _, err := range errs.Errors { + var valErr *ValidationError + assert.NotErrorAs(t, err, &valErr) + } + + // the two post-fork slots (boundarySlot, boundarySlot+1) are reported as a + // single aggregated non-fatal note per role since no pubkeys/indices were + // supplied to filter the now-committee-backed AGGREGATOR duty. + require.Len(t, errs.Errors, 1) + assert.Contains(t, errs.Errors[0].Error(), fmt.Sprintf("slots %d-%d", boundarySlot, boundarySlot+1)) + assert.Contains(t, errs.Errors[0].Error(), "committee duty") +} + // mockValidatorTraceStore is a minimal dutyTraceStore implementation for // exercising getValidatorCommitteeDutiesForRoleAndSlot's signer-bucket gating. type mockValidatorTraceStore struct {