From e190a396a174c5b618ec5b35c47261c3a48d9a8b Mon Sep 17 00:00:00 2001 From: lystopad Date: Sat, 22 Aug 2026 03:31:26 +0000 Subject: [PATCH] cl/beacon: report a syncing node as 503, not 500 (#23464) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #23462. A node with no head state — starting up, restarting, catching up — answered **500 Internal Server Error** on endpoints where `beacon-APIs` declares **503 CurrentlySyncing**. It is transiently unavailable, not faulty, and 500 reads as a node fault to operators and alerting. Same node, same cause, two different codes today: `POST /eth/v1/beacon/pool/attestations` already returns 503 (`pool.go` sets it explicitly), while the committee subscription endpoints return 500. That inconsistency is what makes this an oversight rather than a decision. ## Change Two paths fed the same wrong code. **The sentinel was discarded.** `AddAttestationSubscription` built a fresh error, so nothing downstream could classify it: ```go - return errors.New("head state not available") + return synced_data.ErrNotSynced ``` **The central mapper had no case for it.** `WrapEndpointError` already special-cases one sentinel, so this sits next to it — one change covering every endpoint that surfaces `ErrNotSynced` raw (`getAttesterDuties`, `getCommittees`, `PostEthV1BeaconPoolSyncCommittees`, `blockRootFromBlockId`, `PostEthV1ValidatorSyncCommitteeSubscriptions`): ```go if errors.Is(err, fork_graph.ErrStateNotFound) { return NewEndpointError(http.StatusNotFound, ErrorCantFindBeaconState) } +if errors.Is(err, synced_data.ErrNotSynced) { + return NewEndpointError(http.StatusServiceUnavailable, err) +} ``` Both subscription handlers now route through `WrapEndpointError` instead of hardcoding 500, and the syncing case no longer logs at `Error` — a node that is merely catching up should not fill the log with failures. Also in this PR: - **`WrapEndpointError` matched `*EndpointError` by value.** `errors.As` with a value target never matches the pointer `NewEndpointError` returns, so an explicit code reaching this helper collapsed to 500. `HandleEndpoint` checks the pointer form first, so **no endpoint hit this in practice** — it is latent, not live, and it is a trap for the direct callers this PR adds. Both forms now match. - **`ApiHandler.committeeSub` held the concrete `*CommitteeSubscribeMgmt`**, so the already-generated `MockCommitteeSubscribe` could not be injected. It now takes the existing `CommitteeSubscribe` interface. No caller changes — the concrete type satisfies it. - Removed a commented-out block in `subscription.go` that proposed this exact 503 check. ## Spec [`types/http.yaml`](https://github.com/ethereum/beacon-APIs/blob/master/types/http.yaml) — `CurrentlySyncing`: *"Beacon node is currently syncing, try again later."* Declared on two affected endpoints: [`beacon_committee_subscriptions.yaml`](https://github.com/ethereum/beacon-APIs/blob/master/apis/validator/beacon_committee_subscriptions.yaml) and [`duties/attester.yaml`](https://github.com/ethereum/beacon-APIs/blob/master/apis/validator/duties/attester.yaml). **Worth a reviewer's opinion:** the spec does *not* declare 503 for `states/{state_id}/committees`, `pool/sync_committees` or `sync_committee_subscriptions`, which the central mapping also changes. I went with consistency — the condition is equally reachable there, the error body shape is unchanged, and only strict response-code validation would notice. Happy to narrow it to the two spec-declared endpoints if you would rather stay literal. ## Scope This is a conformance and observability fix, not a high-availability one. Lighthouse does not branch on 503 anywhere in its fallback or publish path — it derives health from `/eth/v1/node/health` and `/eth/v1/node/syncing`, and both codes arrive as the same `RequestFailed(ServerMessage(..))`. Client routing behaviour will not change. ## Test TDD, and each guard was verified by mutation rather than by reading: | Mutation | Test that caught it | | --- | --- | | 503 mapping → 500 | `TestWrapEndpointErrorStatusCodes`, both subscription 503 tests | | sentinel → bare `errors.New` | `TestAddAttestationSubscriptionReportsNotSyncedWhileSyncing` | | pointer match removed | `TestWrapEndpointErrorStatusCodes/explicit_code_is_preserved` | The handler tests mock the subscription call, so they cannot catch a regression in `committee_subscription.go` — hence the separate package-level test for the sentinel. Coverage also pins the cases that must *not* change: an unrelated failure still returns 500, and a synced node still returns 200. `./cl/beacon/... ./cl/validator/...` green (18 packages). `make lintci` reports `0 issues`. (cherry picked from commit ef0a6ce7e64285fb6b6c20e3509478aba7b4b495) --- cl/beacon/beaconhttp/api.go | 17 ++- cl/beacon/beaconhttp/api_error_test.go | 47 +++++++ cl/beacon/handler/handler.go | 4 +- cl/beacon/handler/subscription.go | 16 +-- cl/beacon/handler/subscription_test.go | 128 ++++++++++++++++++ .../committee_subscription.go | 2 +- .../committee_subscription_test.go | 46 +++++++ 7 files changed, 244 insertions(+), 16 deletions(-) create mode 100644 cl/beacon/beaconhttp/api_error_test.go create mode 100644 cl/beacon/handler/subscription_test.go create mode 100644 cl/validator/committee_subscription/committee_subscription_test.go diff --git a/cl/beacon/beaconhttp/api.go b/cl/beacon/beaconhttp/api.go index 87167e4d69f..303ec987ac2 100644 --- a/cl/beacon/beaconhttp/api.go +++ b/cl/beacon/beaconhttp/api.go @@ -27,6 +27,7 @@ import ( "strconv" "strings" + "github.com/erigontech/erigon/cl/beacon/synced_data" "github.com/erigontech/erigon/cl/phase1/forkchoice/fork_graph" "github.com/erigontech/erigon/common/log/v3" "github.com/erigontech/erigon/common/ssz" @@ -47,13 +48,23 @@ var ErrorCantFindBeaconState = errors.New("Could not find beacon state") var ErrorSszNotSupported = errors.New("This endpoint does not support SSZ response") func WrapEndpointError(err error) *EndpointError { - e := &EndpointError{} - if errors.As(err, e) { - return e + // Handlers build these with NewEndpointError, so the pointer form is the one that carries a + // deliberate code; matching only the value form would silently turn it into a 500. + var byPointer *EndpointError + if errors.As(err, &byPointer) { + return byPointer + } + byValue := EndpointError{} + if errors.As(err, &byValue) { + return &byValue } if errors.Is(err, fork_graph.ErrStateNotFound) { return NewEndpointError(http.StatusNotFound, ErrorCantFindBeaconState) } + // A node without a head state is transiently unavailable, not faulty. + if errors.Is(err, synced_data.ErrNotSynced) { + return NewEndpointError(http.StatusServiceUnavailable, err) + } return NewEndpointError(http.StatusInternalServerError, err) } diff --git a/cl/beacon/beaconhttp/api_error_test.go b/cl/beacon/beaconhttp/api_error_test.go new file mode 100644 index 00000000000..b36c29a7422 --- /dev/null +++ b/cl/beacon/beaconhttp/api_error_test.go @@ -0,0 +1,47 @@ +// Copyright 2026 The Erigon Authors +// This file is part of Erigon. +// +// Erigon is free software: you can redistribute it and/or modify +// it under the terms of the GNU Lesser General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// Erigon is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Lesser General Public License for more details. +// +// You should have received a copy of the GNU Lesser General Public License +// along with Erigon. If not, see . + +package beaconhttp + +import ( + "errors" + "fmt" + "net/http" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/erigontech/erigon/cl/beacon/synced_data" + "github.com/erigontech/erigon/cl/phase1/forkchoice/fork_graph" +) + +func TestWrapEndpointErrorStatusCodes(t *testing.T) { + for _, tc := range []struct { + name string + err error + code int + }{ + {"not synced", synced_data.ErrNotSynced, http.StatusServiceUnavailable}, + {"not synced, wrapped", fmt.Errorf("attester duties: %w", synced_data.ErrNotSynced), http.StatusServiceUnavailable}, + {"state not found", fork_graph.ErrStateNotFound, http.StatusNotFound}, + {"anything else", errors.New("boom"), http.StatusInternalServerError}, + {"explicit code is preserved", NewEndpointError(http.StatusBadRequest, errors.New("bad")), http.StatusBadRequest}, + } { + t.Run(tc.name, func(t *testing.T) { + require.Equal(t, tc.code, WrapEndpointError(tc.err).Code) + }) + } +} diff --git a/cl/beacon/handler/handler.go b/cl/beacon/handler/handler.go index d0d04dd42f3..1d082e109cb 100644 --- a/cl/beacon/handler/handler.go +++ b/cl/beacon/handler/handler.go @@ -119,7 +119,7 @@ type ApiHandler struct { elClientVersion atomic.Pointer[engine_types.ClientVersionV1] // Cached execution client version for default graffiti. elClientVersionFetching atomic.Bool // Guards a single in-flight background elClientVersion fetch. syncMessagePool sync_contribution_pool.SyncContributionPool - committeeSub *committee_subscription.CommitteeSubscribeMgmt + committeeSub committee_subscription.CommitteeSubscribe attestationProducer attestation_producer.AttestationDataProducer slotWaitedForAttestationProduction *lru.Cache[uint64, struct{}] aggregatePool aggregation.AggregationPool @@ -177,7 +177,7 @@ func NewApiHandler( attestationProducer attestation_producer.AttestationDataProducer, engine execution_client.ExecutionEngine, syncMessagePool sync_contribution_pool.SyncContributionPool, - committeeSub *committee_subscription.CommitteeSubscribeMgmt, + committeeSub committee_subscription.CommitteeSubscribe, aggregatePool aggregation.AggregationPool, syncCommitteeMessagesService services.SyncCommitteeMessagesService, syncContributionAndProofs services.SyncContributionService, diff --git a/cl/beacon/handler/subscription.go b/cl/beacon/handler/subscription.go index cc7802d9509..eb8ef5a3490 100644 --- a/cl/beacon/handler/subscription.go +++ b/cl/beacon/handler/subscription.go @@ -26,6 +26,7 @@ import ( "time" "github.com/erigontech/erigon/cl/beacon/beaconhttp" + "github.com/erigontech/erigon/cl/beacon/synced_data" "github.com/erigontech/erigon/cl/cltypes" "github.com/erigontech/erigon/cl/gossip" "github.com/erigontech/erigon/cl/phase1/core/state" @@ -67,12 +68,6 @@ func (a *ApiHandler) PostEthV1ValidatorSyncCommitteeSubscriptions(w http.Respons syncnets = append(syncnets, uint64(i)) } } else { - // headState, cn := a.syncedData.HeadState() - // defer cn() - // if headState == nil { - // http.Error(w, "head state not available", http.StatusServiceUnavailable) - // return - // } if err := a.syncedData.ViewHeadState(func(headState *state.CachingBeaconState) error { syncnets, err = subnets.ComputeSubnetsForSyncCommittee(headState, subRequest.ValidatorIndex) if err != nil { @@ -80,10 +75,9 @@ func (a *ApiHandler) PostEthV1ValidatorSyncCommitteeSubscriptions(w http.Respons } return nil }); err != nil { - beaconhttp.NewEndpointError(http.StatusInternalServerError, err).WriteTo(w) + beaconhttp.WrapEndpointError(err).WriteTo(w) return } - //cn() } // subscribe to subnets @@ -118,8 +112,10 @@ func (a *ApiHandler) PostEthV1ValidatorBeaconCommitteeSubscription(w http.Respon } for _, sub := range req { if err := a.committeeSub.AddAttestationSubscription(context.Background(), sub); err != nil { - log.Error("failed to add attestation subscription", "err", err) - beaconhttp.NewEndpointError(http.StatusInternalServerError, err).WriteTo(w) + if !errors.Is(err, synced_data.ErrNotSynced) { + log.Error("failed to add attestation subscription", "err", err) + } + beaconhttp.WrapEndpointError(err).WriteTo(w) return } } diff --git a/cl/beacon/handler/subscription_test.go b/cl/beacon/handler/subscription_test.go new file mode 100644 index 00000000000..42cbef9a640 --- /dev/null +++ b/cl/beacon/handler/subscription_test.go @@ -0,0 +1,128 @@ +// Copyright 2026 The Erigon Authors +// This file is part of Erigon. +// +// Erigon is free software: you can redistribute it and/or modify +// it under the terms of the GNU Lesser General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// Erigon is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Lesser General Public License for more details. +// +// You should have received a copy of the GNU Lesser General Public License +// along with Erigon. If not, see . + +package handler + +import ( + "bytes" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + + "github.com/erigontech/erigon/cl/beacon/synced_data" + sync_mock_services "github.com/erigontech/erigon/cl/beacon/synced_data/mock_services" + "github.com/erigontech/erigon/cl/clparams" + "github.com/erigontech/erigon/cl/cltypes" + "github.com/erigontech/erigon/cl/validator/committee_subscription/mock_services" + "github.com/erigontech/erigon/common/log/v3" +) + +func postBeaconCommitteeSubscription(t *testing.T, handler *ApiHandler) int { + t.Helper() + + server := httptest.NewServer(handler.mux) + defer server.Close() + + body, err := json.Marshal([]*cltypes.BeaconCommitteeSubscription{{ + ValidatorIndex: 1, + CommitteeIndex: 0, + CommitteesAtSlot: 1, + Slot: 1, + IsAggregator: false, + }}) + require.NoError(t, err) + + req, err := http.NewRequestWithContext(t.Context(), http.MethodPost, + server.URL+"/eth/v1/validator/beacon_committee_subscriptions", bytes.NewBuffer(body)) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/json") + + resp, err := server.Client().Do(req) + require.NoError(t, err) + defer resp.Body.Close() + return resp.StatusCode +} + +// The beacon-APIs spec declares 503 CurrentlySyncing for this endpoint, so a node without a head +// state must not report the condition as an internal error. +func TestBeaconCommitteeSubscriptionIsUnavailableWhileSyncing(t *testing.T) { + ctrl := gomock.NewController(t) + _, _, _, _, _, handler, _, _, _, _ := setupTestingHandler(t, clparams.Phase0Version, log.Root(), false) + + committeeSub := mock_services.NewMockCommitteeSubscribe(ctrl) + committeeSub.EXPECT().AddAttestationSubscription(gomock.Any(), gomock.Any()). + Return(synced_data.ErrNotSynced).AnyTimes() + handler.committeeSub = committeeSub + + require.Equal(t, http.StatusServiceUnavailable, postBeaconCommitteeSubscription(t, handler)) +} + +func TestBeaconCommitteeSubscriptionReportsOtherFailuresAsInternalError(t *testing.T) { + ctrl := gomock.NewController(t) + _, _, _, _, _, handler, _, _, _, _ := setupTestingHandler(t, clparams.Phase0Version, log.Root(), false) + + committeeSub := mock_services.NewMockCommitteeSubscribe(ctrl) + committeeSub.EXPECT().AddAttestationSubscription(gomock.Any(), gomock.Any()). + Return(errors.New("subnet computation blew up")).AnyTimes() + handler.committeeSub = committeeSub + + require.Equal(t, http.StatusInternalServerError, postBeaconCommitteeSubscription(t, handler)) +} + +func TestBeaconCommitteeSubscriptionSucceedsWhenSynced(t *testing.T) { + ctrl := gomock.NewController(t) + _, _, _, _, _, handler, _, _, _, _ := setupTestingHandler(t, clparams.Phase0Version, log.Root(), false) + + committeeSub := mock_services.NewMockCommitteeSubscribe(ctrl) + committeeSub.EXPECT().AddAttestationSubscription(gomock.Any(), gomock.Any()). + Return(nil).AnyTimes() + handler.committeeSub = committeeSub + + require.Equal(t, http.StatusOK, postBeaconCommitteeSubscription(t, handler)) +} + +func TestSyncCommitteeSubscriptionIsUnavailableWhileSyncing(t *testing.T) { + _, _, _, _, _, handler, _, syncedDataMgr, _, _ := setupTestingHandler(t, clparams.Phase0Version, log.Root(), false) + syncedDataMgr.(*sync_mock_services.MockSyncedData).EXPECT().ViewHeadState(gomock.Any()). + Return(synced_data.ErrNotSynced).AnyTimes() + + server := httptest.NewServer(handler.mux) + defer server.Close() + + // Far enough ahead that the subscription has not expired, without overflowing the slot clock. + body, err := json.Marshal([]ValidatorSyncCommitteeSubscriptionsRequest{{ + ValidatorIndex: 1, + SyncCommitteeIndicies: []string{"0"}, + UntilEpoch: 1_000_000_000, + }}) + require.NoError(t, err) + + req, err := http.NewRequestWithContext(t.Context(), http.MethodPost, + server.URL+"/eth/v1/validator/sync_committee_subscriptions", bytes.NewBuffer(body)) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/json") + + resp, err := server.Client().Do(req) + require.NoError(t, err) + defer resp.Body.Close() + + require.Equal(t, http.StatusServiceUnavailable, resp.StatusCode) +} diff --git a/cl/validator/committee_subscription/committee_subscription.go b/cl/validator/committee_subscription/committee_subscription.go index b7ec4dbd88a..5b1a8049fd4 100644 --- a/cl/validator/committee_subscription/committee_subscription.go +++ b/cl/validator/committee_subscription/committee_subscription.go @@ -94,7 +94,7 @@ func (c *CommitteeSubscribeMgmt) AddAttestationSubscription(ctx context.Context, ) if c.syncedData.Syncing() { - return errors.New("head state not available") + return synced_data.ErrNotSynced } log.Trace("Add attestation subscription", "slot", slot, "committeeIndex", cIndex, "isAggregator", p.IsAggregator, "validatorIndex", p.ValidatorIndex) diff --git a/cl/validator/committee_subscription/committee_subscription_test.go b/cl/validator/committee_subscription/committee_subscription_test.go new file mode 100644 index 00000000000..3eb44ff1bb4 --- /dev/null +++ b/cl/validator/committee_subscription/committee_subscription_test.go @@ -0,0 +1,46 @@ +// Copyright 2026 The Erigon Authors +// This file is part of Erigon. +// +// Erigon is free software: you can redistribute it and/or modify +// it under the terms of the GNU Lesser General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// Erigon is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Lesser General Public License for more details. +// +// You should have received a copy of the GNU Lesser General Public License +// along with Erigon. If not, see . + +package committee_subscription + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/erigontech/erigon/cl/beacon/synced_data" + "github.com/erigontech/erigon/cl/clparams" + "github.com/erigontech/erigon/cl/cltypes" +) + +// The REST layer turns this into 503 CurrentlySyncing by matching the sentinel, so a bare error +// here would be reported as an internal failure instead. +func TestAddAttestationSubscriptionReportsNotSyncedWhileSyncing(t *testing.T) { + cfg := clparams.MainnetBeaconConfig + c := &CommitteeSubscribeMgmt{ + beaconConfig: &cfg, + syncedData: synced_data.NewSyncedDataManager(&cfg, true), + } + + err := c.AddAttestationSubscription(t.Context(), &cltypes.BeaconCommitteeSubscription{ + ValidatorIndex: 1, + CommitteeIndex: 0, + CommitteesAtSlot: 1, + Slot: 1, + }) + + require.ErrorIs(t, err, synced_data.ErrNotSynced) +}