From ed86942600ed7f4511611ac14c124eaf0a6d7bc6 Mon Sep 17 00:00:00 2001 From: Samuel Laferriere Date: Fri, 25 Oct 2024 17:23:36 +0100 Subject: [PATCH 01/11] feat(failover): return 503 to batcher when eigenda is down chore: go mod tidy to generate go.mod feat: dealing with new eigenda-client grpc errors + ErrorFailover convention comment: fix typo feat(handlers): postShared returns 429 when disperser rate limited client flag(eigenda): rename RetriesBeforeFailover -> PutRetries reviewer correctly pointed out that retrying was more general than only for failovers lint: nolint exhaustive switch check for Put case --- flags/eigendaflags/cli.go | 11 +++++++ go.mod | 3 +- go.sum | 2 ++ server/config.go | 7 +++-- server/errors.go | 22 +++++++++++++ server/handlers.go | 14 ++++++--- server/load_store.go | 1 + store/generated_key/eigenda/eigenda.go | 43 +++++++++++++++++++++++++- 8 files changed, 93 insertions(+), 10 deletions(-) diff --git a/flags/eigendaflags/cli.go b/flags/eigendaflags/cli.go index 9a13c772..eb87ffd0 100644 --- a/flags/eigendaflags/cli.go +++ b/flags/eigendaflags/cli.go @@ -27,6 +27,8 @@ var ( ConfirmationDepthFlagName = withFlagPrefix("confirmation-depth") EthRPCURLFlagName = withFlagPrefix("eth-rpc") SvcManagerAddrFlagName = withFlagPrefix("svc-manager-addr") + // Flags that are proxy specific, and not used by the eigenda-client + PutRetriesFlagName = withFlagPrefix("put-retries") ) func withFlagPrefix(s string) string { @@ -137,6 +139,15 @@ func CLIFlags(envPrefix, category string) []cli.Flag { Category: category, Required: true, }, + // Flags that are proxy specific, and not used by the eigenda-client + // TODO: should we move this to a more specific category, like EIGENDA_STORE? + &cli.UintFlag{ + Name: PutRetriesFlagName, + Usage: "Number of times to retry blob dispersals.", + Value: 3, + EnvVars: []string{withEnvPrefix(envPrefix, "PUT_RETRIES")}, + Category: category, + }, } } diff --git a/go.mod b/go.mod index 38e89c39..3f261ef8 100644 --- a/go.mod +++ b/go.mod @@ -6,6 +6,7 @@ toolchain go1.22.0 require ( github.com/Layr-Labs/eigenda v0.8.5-0.20241031144746-e2ead56a306d + github.com/avast/retry-go/v4 v4.6.0 github.com/consensys/gnark-crypto v0.12.1 github.com/ethereum-optimism/optimism v1.9.4-0.20240927020138-a9c7f349d10b github.com/ethereum/go-ethereum v1.14.11 @@ -20,6 +21,7 @@ require ( github.com/testcontainers/testcontainers-go/modules/redis v0.33.0 github.com/urfave/cli/v2 v2.27.4 golang.org/x/exp v0.0.0-20240808152545-0cdaa3abc0fa + google.golang.org/grpc v1.64.1 ) require ( @@ -283,7 +285,6 @@ require ( golang.org/x/time v0.6.0 // indirect golang.org/x/tools v0.24.0 // indirect google.golang.org/genproto/googleapis/rpc v0.0.0-20240318140521-94a12d6c2237 // indirect - google.golang.org/grpc v1.64.1 // indirect google.golang.org/protobuf v1.34.2 // indirect gopkg.in/natefinch/lumberjack.v2 v2.2.1 // indirect gopkg.in/yaml.v2 v2.4.0 // indirect diff --git a/go.sum b/go.sum index 8e602d79..b636e0b5 100644 --- a/go.sum +++ b/go.sum @@ -45,6 +45,8 @@ github.com/andybalholm/brotli v1.1.0/go.mod h1:sms7XGricyQI9K10gOSf56VKKWS4oLer5 github.com/anmitsu/go-shlex v0.0.0-20161002113705-648efa622239/go.mod h1:2FmKhYUyUczH0OGQWaF5ceTx0UBShxjsH6f8oGKYe2c= github.com/armon/go-metrics v0.4.1 h1:hR91U9KYmb6bLBYLQjyM+3j+rcd/UhE+G78SFnF8gJA= github.com/armon/go-metrics v0.4.1/go.mod h1:E6amYzXo6aW1tqzoZGT755KkbgrJsSdpwZ+3JqfkOG4= +github.com/avast/retry-go/v4 v4.6.0 h1:K9xNA+KeB8HHc2aWFuLb25Offp+0iVRXEvFx8IinRJA= +github.com/avast/retry-go/v4 v4.6.0/go.mod h1:gvWlPhBVsvBbLkVGDg/KwvBv0bEkCOLRRSHKIr2PyOE= github.com/aws/aws-sdk-go-v2 v1.26.1 h1:5554eUqIYVWpU0YmeeYZ0wU64H2VLBs8TlhRB2L+EkA= github.com/aws/aws-sdk-go-v2 v1.26.1/go.mod h1:ffIFB97e2yNsv4aTSGkqtHnppsIJzw7G7BReUZ3jCXM= github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream v1.6.1 h1:gTK2uhtAPtFcdRRJilZPx8uJLL2J85xK11nKtWL0wfU= diff --git a/server/config.go b/server/config.go index e695755d..75a9866c 100644 --- a/server/config.go +++ b/server/config.go @@ -19,6 +19,7 @@ type Config struct { MemstoreConfig memstore.Config StorageConfig store.Config VerifierConfig verify.Config + PutRetries uint MemstoreEnabled bool } @@ -28,11 +29,11 @@ func ReadConfig(ctx *cli.Context) Config { edaClientConfig := eigendaflags.ReadConfig(ctx) return Config{ EdaClientConfig: edaClientConfig, - MemstoreConfig: memstore.ReadConfig(ctx), - StorageConfig: store.ReadConfig(ctx), VerifierConfig: verify.ReadConfig(ctx, edaClientConfig), - + PutRetries: ctx.Uint(eigendaflags.PutRetriesFlagName), MemstoreEnabled: ctx.Bool(memstore.EnabledFlagName), + MemstoreConfig: memstore.ReadConfig(ctx), + StorageConfig: store.ReadConfig(ctx), } } diff --git a/server/errors.go b/server/errors.go index 77ff759b..a3435497 100644 --- a/server/errors.go +++ b/server/errors.go @@ -1,9 +1,13 @@ package server import ( + "errors" "fmt" "github.com/Layr-Labs/eigenda-proxy/commitments" + "github.com/Layr-Labs/eigenda-proxy/store" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) // MetaError includes both an error and commitment metadata @@ -22,3 +26,21 @@ func (me MetaError) Error() string { func (me MetaError) Unwrap() error { return me.Err } + +func is400(err error) bool { + // proxy requests are super simple (clients basically only pass bytes), so the only 400 possible + // is passing a blob that's too big. + // + // Any 400s returned by the disperser are due to formatting bugs in proxy code, for eg. badly + // IFFT'ing or encoding the blob, so we shouldn't return a 400 to the client. + // See https://github.com/Layr-Labs/eigenda/blob/bee55ed9207f16153c3fd8ebf73c219e68685def/api/errors.go#L22 + // for the 400s returned by the disperser server (currently only INVALID_ARGUMENT). + return errors.Is(err, store.ErrProxyOversizedBlob) +} + +func is429(err error) bool { + // grpc RESOURCE_EXHAUSTED is returned by the disperser server when the client has sent too many requests + // in a short period of time. This is a client-side issue, so we should return the 429 to the client. + st, isGRPCError := status.FromError(err) + return isGRPCError && st.Code() == codes.ResourceExhausted +} diff --git a/server/handlers.go b/server/handlers.go index 60469a99..f38ca817 100644 --- a/server/handlers.go +++ b/server/handlers.go @@ -9,7 +9,7 @@ import ( "net/http" "github.com/Layr-Labs/eigenda-proxy/commitments" - "github.com/Layr-Labs/eigenda-proxy/common" + "github.com/Layr-Labs/eigenda/api" "github.com/gorilla/mux" ) @@ -181,11 +181,15 @@ func (svr *Server) handlePostShared(w http.ResponseWriter, r *http.Request, comm Err: fmt.Errorf("put request failed with commitment %v (commitment mode %v): %w", comm, meta.Mode, err), Meta: meta, } - if errors.Is(err, common.ErrEigenDAOversizedBlob) || errors.Is(err, common.ErrProxyOversizedBlob) { - // we add here any error that should be returned as a 400 instead of a 500. - // currently only includes oversized blob requests + switch { + case is400(err): http.Error(w, err.Error(), http.StatusBadRequest) - } else { + case is429(err): + http.Error(w, err.Error(), http.StatusTooManyRequests) + case errors.Is(err, &api.ErrorFailover{}): + // this tells the caller (batcher) to failover to ethda b/c eigenda is temporarily down + http.Error(w, err.Error(), http.StatusServiceUnavailable) + default: http.Error(w, err.Error(), http.StatusInternalServerError) } return err diff --git a/server/load_store.go b/server/load_store.go index e79145bf..cb75a6ae 100644 --- a/server/load_store.go +++ b/server/load_store.go @@ -112,6 +112,7 @@ func LoadStoreManager(ctx context.Context, cfg CLIConfig, log log.Logger, m metr MaxBlobSizeBytes: cfg.EigenDAConfig.MemstoreConfig.MaxBlobSizeBytes, EthConfirmationDepth: cfg.EigenDAConfig.VerifierConfig.EthConfirmationDepth, StatusQueryTimeout: cfg.EigenDAConfig.EdaClientConfig.StatusQueryTimeout, + PutRetries: cfg.EigenDAConfig.PutRetries, }, ) } diff --git a/store/generated_key/eigenda/eigenda.go b/store/generated_key/eigenda/eigenda.go index 70b972d4..b67ce1d2 100644 --- a/store/generated_key/eigenda/eigenda.go +++ b/store/generated_key/eigenda/eigenda.go @@ -8,8 +8,13 @@ import ( "github.com/Layr-Labs/eigenda-proxy/common" "github.com/Layr-Labs/eigenda-proxy/verify" "github.com/Layr-Labs/eigenda/api/clients" + "github.com/Layr-Labs/eigenda/api/grpc/disperser" + + "github.com/avast/retry-go/v4" "github.com/ethereum/go-ethereum/log" "github.com/ethereum/go-ethereum/rlp" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) type StoreConfig struct { @@ -20,6 +25,9 @@ type StoreConfig struct { // total duration time that client waits for blob to confirm StatusQueryTimeout time.Duration + + // number of times to retry eigenda blob dispersals + PutRetries uint } // Store does storage interactions and verifications for blobs with DA. @@ -70,7 +78,40 @@ func (e Store) Put(ctx context.Context, value []byte) ([]byte, error) { return nil, fmt.Errorf("%w: blob length %d, max blob size %d", common.ErrProxyOversizedBlob, len(value), e.cfg.MaxBlobSizeBytes) } - blobInfo, err := e.client.PutBlob(ctx, value) + // We attempt to disperse the blob to EigenDA up to 3 times, unless we get a 400 error on any attempt. + blobInfo, err := retry.DoWithData( + func() (*disperser.BlobInfo, error) { + return e.client.PutBlob(ctx, value) + }, + retry.RetryIf(func(err error) bool { + st, isGRPCError := status.FromError(err) + if !isGRPCError { + // api.ErrorFailover is returned, so we should retry + return true + } + //nolint:exhaustive // we only care about a few grpc error codes + switch st.Code() { + case codes.InvalidArgument: + // we don't retry 400 errors because there is no point, + // we are passing invalid data + return false + case codes.ResourceExhausted: + // we retry on 429s because *can* mean we are being rate limited + // we sleep 1 second... very arbitrarily, because we don't have more info. + // grpc error itself should return a backoff time, + // see https://github.com/Layr-Labs/eigenda/issues/845 for more details + time.Sleep(1 * time.Second) + return true + default: + return true + } + }), + // only return the last error. If it is an api.ErrorFailover, then the handler will convert + // it to an http 503 to signify to the client (batcher) to failover to ethda + // b/c eigenda is temporarily down. + retry.LastErrorOnly(true), + retry.Attempts(e.cfg.PutRetries), + ) if err != nil { // TODO: we will want to filter for errors here and return a 503 when needed // ie when dispersal itself failed, or that we timed out waiting for batch to land onchain From 85fd571f6e1623c550a316e5ba273cbc898d951c Mon Sep 17 00:00:00 2001 From: Samuel Laferriere Date: Thu, 31 Oct 2024 16:52:50 +0000 Subject: [PATCH 02/11] flag(eigenda-client): add cli flag for new config ConfirmationTimeout --- flags/eigendaflags/cli.go | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/flags/eigendaflags/cli.go b/flags/eigendaflags/cli.go index eb87ffd0..30937401 100644 --- a/flags/eigendaflags/cli.go +++ b/flags/eigendaflags/cli.go @@ -15,10 +15,11 @@ import ( var ( DisperserRPCFlagName = withFlagPrefix("disperser-rpc") + ResponseTimeoutFlagName = withFlagPrefix("response-timeout") + ConfirmationTimeoutFlagName = withFlagPrefix("confirmation-timeout") StatusQueryRetryIntervalFlagName = withFlagPrefix("status-query-retry-interval") StatusQueryTimeoutFlagName = withFlagPrefix("status-query-timeout") DisableTLSFlagName = withFlagPrefix("disable-tls") - ResponseTimeoutFlagName = withFlagPrefix("response-timeout") CustomQuorumIDsFlagName = withFlagPrefix("custom-quorum-ids") SignerPrivateKeyHexFlagName = withFlagPrefix("signer-private-key-hex") PutBlobEncodingVersionFlagName = withFlagPrefix("put-blob-encoding-version") @@ -48,6 +49,26 @@ func CLIFlags(envPrefix, category string) []cli.Flag { EnvVars: []string{withEnvPrefix(envPrefix, "DISPERSER_RPC")}, Category: category, }, + &cli.DurationFlag{ + Name: ResponseTimeoutFlagName, + Usage: "Total time to wait for a response from the EigenDA disperser. Default is 60 seconds.", + Value: 60 * time.Second, + EnvVars: []string{withEnvPrefix(envPrefix, "RESPONSE_TIMEOUT")}, + Category: category, + }, + &cli.DurationFlag{ + Name: ConfirmationTimeoutFlagName, + Usage: `The total amount of time that the client will spend waiting for EigenDA + to "confirm" (include onchain) a blob after it has been dispersed. Note that + we stick to "confirm" here but this really means InclusionTimeout, + not confirmation in the sense of confirmation depth. + + If ConfirmationTimeout time passes and the blob is not yet confirmed, + the client will return an api.ErrorFailover to let the caller failover to EthDA.`, + Value: 15 * time.Minute, + EnvVars: []string{withEnvPrefix(envPrefix, "CONFIRMATION_TIMEOUT")}, + Category: category, + }, &cli.DurationFlag{ Name: StatusQueryTimeoutFlagName, Usage: "Duration to wait for a blob to finalize after being sent for dispersal. Default is 30 minutes.", @@ -69,13 +90,6 @@ func CLIFlags(envPrefix, category string) []cli.Flag { EnvVars: []string{withEnvPrefix(envPrefix, "GRPC_DISABLE_TLS")}, Category: category, }, - &cli.DurationFlag{ - Name: ResponseTimeoutFlagName, - Usage: "Total time to wait for a response from the EigenDA disperser. Default is 60 seconds.", - Value: 60 * time.Second, - EnvVars: []string{withEnvPrefix(envPrefix, "RESPONSE_TIMEOUT")}, - Category: category, - }, &cli.UintSliceFlag{ Name: CustomQuorumIDsFlagName, Usage: "Custom quorum IDs for writing blobs. Should not include default quorums 0 or 1.", @@ -155,10 +169,11 @@ func ReadConfig(ctx *cli.Context) clients.EigenDAClientConfig { waitForFinalization, confirmationDepth := parseConfirmationFlag(ctx.String(ConfirmationDepthFlagName)) return clients.EigenDAClientConfig{ RPC: ctx.String(DisperserRPCFlagName), + ResponseTimeout: ctx.Duration(ResponseTimeoutFlagName), + ConfirmationTimeout: ctx.Duration(ConfirmationTimeoutFlagName), StatusQueryRetryInterval: ctx.Duration(StatusQueryRetryIntervalFlagName), StatusQueryTimeout: ctx.Duration(StatusQueryTimeoutFlagName), DisableTLS: ctx.Bool(DisableTLSFlagName), - ResponseTimeout: ctx.Duration(ResponseTimeoutFlagName), CustomQuorumIDs: ctx.UintSlice(CustomQuorumIDsFlagName), SignerPrivateKeyHex: ctx.String(SignerPrivateKeyHexFlagName), PutBlobEncodingVersion: codecs.BlobEncodingVersion(ctx.Uint(PutBlobEncodingVersionFlagName)), From 9cfdfa3971034dec0b31bead3578b43bd72b38ee Mon Sep 17 00:00:00 2001 From: Samuel Laferriere Date: Fri, 1 Nov 2024 15:34:07 +0000 Subject: [PATCH 03/11] tests(handlers): rename servers_test.go -> handlers_test.go + some small refactors --- .vscode/settings.json | 1 + server/{server_test.go => handlers_test.go} | 37 ++++++++++++--------- 2 files changed, 22 insertions(+), 16 deletions(-) rename server/{server_test.go => handlers_test.go} (70%) diff --git a/.vscode/settings.json b/.vscode/settings.json index 736563a9..8aff5c4a 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -10,6 +10,7 @@ "go.testFlags": [ "-test.parallel", "4", + // Comment the following 2 lines to run unit tests. "-deploy-config", "../.devnet/devnetL1.json" ] diff --git a/server/server_test.go b/server/handlers_test.go similarity index 70% rename from server/server_test.go rename to server/handlers_test.go index 3fc2c93c..05b844b9 100644 --- a/server/server_test.go +++ b/server/handlers_test.go @@ -1,5 +1,8 @@ package server +// The tests in this file test not only the handlers but also the middlewares, +// because server.registerRoutes(r) registers the handlers wrapped with middlewares. + import ( "bytes" "fmt" @@ -27,11 +30,7 @@ const ( func TestHandleOPCommitments(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() - - mockRouter := mocks.NewMockIManager(ctrl) - - m := metrics.NewMetrics("default") - server := NewServer("localhost", 8080, mockRouter, log.New(), m) + mockStorageMgr := mocks.NewMockIManager(ctrl) tests := []struct { name string @@ -44,7 +43,7 @@ func TestHandleOPCommitments(t *testing.T) { name: "Failure - OP Keccak256 Internal Server Error", url: fmt.Sprintf("/get/0x00%s", testCommitStr), mockBehavior: func() { - mockRouter.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("internal error")) + mockStorageMgr.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("internal error")) }, expectedCode: http.StatusInternalServerError, expectedBody: "", @@ -53,7 +52,7 @@ func TestHandleOPCommitments(t *testing.T) { name: "Success - OP Keccak256", url: fmt.Sprintf("/get/0x00%s", testCommitStr), mockBehavior: func() { - mockRouter.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return([]byte(testCommitStr), nil) + mockStorageMgr.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return([]byte(testCommitStr), nil) }, expectedCode: http.StatusOK, expectedBody: testCommitStr, @@ -62,7 +61,7 @@ func TestHandleOPCommitments(t *testing.T) { name: "Failure - OP Alt-DA Internal Server Error", url: fmt.Sprintf("/get/0x010000%s", testCommitStr), mockBehavior: func() { - mockRouter.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("internal error")) + mockStorageMgr.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("internal error")) }, expectedCode: http.StatusInternalServerError, expectedBody: "", @@ -71,7 +70,7 @@ func TestHandleOPCommitments(t *testing.T) { name: "Success - OP Alt-DA", url: fmt.Sprintf("/get/0x010000%s", testCommitStr), mockBehavior: func() { - mockRouter.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return([]byte(testCommitStr), nil) + mockStorageMgr.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).Return([]byte(testCommitStr), nil) }, expectedCode: http.StatusOK, expectedBody: testCommitStr, @@ -88,6 +87,10 @@ func TestHandleOPCommitments(t *testing.T) { // To add the vars to the context, // we need to create a router through which we can pass the request. r := mux.NewRouter() + // enable this logger to help debug tests + // logger := log.NewLogger(log.NewTerminalHandler(os.Stderr, true)).With("test_name", t.Name()) + noopLogger := log.NewLogger(log.DiscardHandler()) + server := NewServer("localhost", 0, mockStorageMgr, noopLogger, metrics.NoopMetrics) server.registerRoutes(r) r.ServeHTTP(rec, req) @@ -103,9 +106,7 @@ func TestHandleOPCommitments(t *testing.T) { func TestHandlerPut(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() - - mockRouter := mocks.NewMockIManager(ctrl) - server := NewServer("localhost", 8080, mockRouter, log.New(), metrics.NoopMetrics) + mockStorageMgr := mocks.NewMockIManager(ctrl) tests := []struct { name string @@ -121,7 +122,7 @@ func TestHandlerPut(t *testing.T) { url: "/put", body: []byte("some data that will trigger an internal error"), mockBehavior: func() { - mockRouter.EXPECT().Put(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("internal error")) + mockStorageMgr.EXPECT().Put(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("internal error")) }, expectedCode: http.StatusInternalServerError, expectedBody: "", @@ -132,7 +133,7 @@ func TestHandlerPut(t *testing.T) { url: "/put", body: []byte("some data that will successfully be written to EigenDA"), mockBehavior: func() { - mockRouter.EXPECT().Put(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return([]byte(testCommitStr), nil) + mockStorageMgr.EXPECT().Put(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return([]byte(testCommitStr), nil) }, expectedCode: http.StatusOK, expectedBody: opGenericPrefixStr + testCommitStr, @@ -143,7 +144,7 @@ func TestHandlerPut(t *testing.T) { url: fmt.Sprintf("/put/0x00%s", testCommitStr), body: []byte("some data that will successfully be written to EigenDA"), mockBehavior: func() { - mockRouter.EXPECT().Put(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return([]byte(testCommitStr), nil) + mockStorageMgr.EXPECT().Put(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return([]byte(testCommitStr), nil) }, expectedCode: http.StatusOK, expectedBody: "", @@ -154,7 +155,7 @@ func TestHandlerPut(t *testing.T) { url: "/put?commitment_mode=simple", body: []byte("some data that will successfully be written to EigenDA"), mockBehavior: func() { - mockRouter.EXPECT().Put(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return([]byte(testCommitStr), nil) + mockStorageMgr.EXPECT().Put(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return([]byte(testCommitStr), nil) }, expectedCode: http.StatusOK, expectedBody: simpleCommitmentPrefix + testCommitStr, @@ -172,6 +173,10 @@ func TestHandlerPut(t *testing.T) { // To add the vars to the context, // we need to create a router through which we can pass the request. r := mux.NewRouter() + // enable this logger to help debug tests + // logger := log.NewLogger(log.NewTerminalHandler(os.Stderr, true)).With("test_name", t.Name()) + noopLogger := log.NewLogger(log.DiscardHandler()) + server := NewServer("localhost", 0, mockStorageMgr, noopLogger, metrics.NoopMetrics) server.registerRoutes(r) r.ServeHTTP(rec, req) From 63d018be1cf1490e73d1523964c1b33c421c3a9f Mon Sep 17 00:00:00 2001 From: Samuel Laferriere Date: Fri, 1 Nov 2024 15:46:31 +0000 Subject: [PATCH 04/11] tests(handlers): add PUT failure tests for all modes --- server/handlers_test.go | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/server/handlers_test.go b/server/handlers_test.go index 05b844b9..8c1683ab 100644 --- a/server/handlers_test.go +++ b/server/handlers_test.go @@ -27,7 +27,7 @@ const ( testCommitStr = "9a7d4f1c3e5b8a09d1c0fa4b3f8e1d7c6b29f1e6d8c4a7b3c2d4e5f6a7b8c9d0" ) -func TestHandleOPCommitments(t *testing.T) { +func TestHandlerGet(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() mockStorageMgr := mocks.NewMockIManager(ctrl) @@ -95,9 +95,11 @@ func TestHandleOPCommitments(t *testing.T) { r.ServeHTTP(rec, req) require.Equal(t, tt.expectedCode, rec.Code) - // We don't test for bodies because it's a specific error message - // that contains a lot of information - // require.Equal(t, tt.expectedBody, rec.Body.String()) + // We only test for bodies for 200s because error messages contain a lot of information + // that isn't very important to test (plus its annoying to always change if error msg changes slightly). + if tt.expectedCode == http.StatusOK { + require.Equal(t, tt.expectedBody, rec.Body.String()) + } }) } @@ -139,6 +141,17 @@ func TestHandlerPut(t *testing.T) { expectedBody: opGenericPrefixStr + testCommitStr, expectError: false, }, + { + name: "Failure OP Mode Keccak256 - InternalServerError", + url: fmt.Sprintf("/put/0x00%s", testCommitStr), + body: []byte("some data that will trigger an internal error"), + mockBehavior: func() { + mockStorageMgr.EXPECT().Put(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("internal error")) + }, + expectedCode: http.StatusInternalServerError, + expectedBody: "", + expectError: true, + }, { name: "Success OP Mode Keccak256", url: fmt.Sprintf("/put/0x00%s", testCommitStr), @@ -150,6 +163,17 @@ func TestHandlerPut(t *testing.T) { expectedBody: "", expectError: false, }, + { + name: "Failure Simple Commitment Mode - InternalServerError", + url: "/put?commitment_mode=simple", + body: []byte("some data that will trigger an internal error"), + mockBehavior: func() { + mockStorageMgr.EXPECT().Put(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("internal error")) + }, + expectedCode: http.StatusInternalServerError, + expectedBody: "", + expectError: true, + }, { name: "Success Simple Commitment Mode", url: "/put?commitment_mode=simple", From 89077239e785fd3f5535e41830e923a2dbea2e23 Mon Sep 17 00:00:00 2001 From: Samuel Laferriere Date: Fri, 1 Nov 2024 21:36:20 +0000 Subject: [PATCH 05/11] test(handlers): remove unneeded expectedError in TestHandlerPut --- server/handlers_test.go | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/server/handlers_test.go b/server/handlers_test.go index 8c1683ab..336d6f73 100644 --- a/server/handlers_test.go +++ b/server/handlers_test.go @@ -117,7 +117,6 @@ func TestHandlerPut(t *testing.T) { mockBehavior func() expectedCode int expectedBody string - expectError bool }{ { name: "Failure OP Mode Alt-DA - InternalServerError", @@ -128,7 +127,6 @@ func TestHandlerPut(t *testing.T) { }, expectedCode: http.StatusInternalServerError, expectedBody: "", - expectError: true, }, { name: "Success OP Mode Alt-DA", @@ -139,7 +137,6 @@ func TestHandlerPut(t *testing.T) { }, expectedCode: http.StatusOK, expectedBody: opGenericPrefixStr + testCommitStr, - expectError: false, }, { name: "Failure OP Mode Keccak256 - InternalServerError", @@ -150,7 +147,6 @@ func TestHandlerPut(t *testing.T) { }, expectedCode: http.StatusInternalServerError, expectedBody: "", - expectError: true, }, { name: "Success OP Mode Keccak256", @@ -161,7 +157,6 @@ func TestHandlerPut(t *testing.T) { }, expectedCode: http.StatusOK, expectedBody: "", - expectError: false, }, { name: "Failure Simple Commitment Mode - InternalServerError", @@ -172,7 +167,6 @@ func TestHandlerPut(t *testing.T) { }, expectedCode: http.StatusInternalServerError, expectedBody: "", - expectError: true, }, { name: "Success Simple Commitment Mode", @@ -183,7 +177,6 @@ func TestHandlerPut(t *testing.T) { }, expectedCode: http.StatusOK, expectedBody: simpleCommitmentPrefix + testCommitStr, - expectError: false, }, } @@ -205,12 +198,10 @@ func TestHandlerPut(t *testing.T) { r.ServeHTTP(rec, req) require.Equal(t, tt.expectedCode, rec.Code) - if !tt.expectError && tt.expectedBody != "" { - require.Equal(t, []byte(tt.expectedBody), rec.Body.Bytes()) - } - - if !tt.expectError && tt.expectedBody == "" { - require.Equal(t, []byte(nil), rec.Body.Bytes()) + // We only test for bodies for 200s because error messages contain a lot of information + // that isn't very important to test (plus its annoying to always change if error msg changes slightly). + if tt.expectedCode == http.StatusOK { + require.Equal(t, tt.expectedBody, rec.Body.String()) } }) } From 9958470b56f4d82f6503df13bbb43df042f0a82c Mon Sep 17 00:00:00 2001 From: Samuel Laferriere Date: Fri, 1 Nov 2024 21:40:50 +0000 Subject: [PATCH 06/11] dep: update eigenda to master head (contains ErrorFailover fix) --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 3f261ef8..5774c02c 100644 --- a/go.mod +++ b/go.mod @@ -5,7 +5,7 @@ go 1.22 toolchain go1.22.0 require ( - github.com/Layr-Labs/eigenda v0.8.5-0.20241031144746-e2ead56a306d + github.com/Layr-Labs/eigenda v0.8.5-rc.0.0.20241101212705-fa8776ae648c github.com/avast/retry-go/v4 v4.6.0 github.com/consensys/gnark-crypto v0.12.1 github.com/ethereum-optimism/optimism v1.9.4-0.20240927020138-a9c7f349d10b diff --git a/go.sum b/go.sum index b636e0b5..6f4c239a 100644 --- a/go.sum +++ b/go.sum @@ -22,8 +22,8 @@ github.com/BurntSushi/toml v1.4.0/go.mod h1:ukJfTF/6rtPPRCnwkur4qwRxa8vTRFBF0uk2 github.com/DataDog/datadog-go v3.2.0+incompatible/go.mod h1:LButxg5PwREeZtORoXG3tL4fMGNddJ+vMq1mwgfaqoQ= github.com/DataDog/zstd v1.5.6-0.20230824185856-869dae002e5e h1:ZIWapoIRN1VqT8GR8jAwb1Ie9GyehWjVcGh32Y2MznE= github.com/DataDog/zstd v1.5.6-0.20230824185856-869dae002e5e/go.mod h1:g4AWEaM3yOg3HYfnJ3YIawPnVdXJh9QME85blwSAmyw= -github.com/Layr-Labs/eigenda v0.8.5-0.20241031144746-e2ead56a306d h1:2JtVArkLjW61kilkvvLyFHXBMp0ClF8PYCAxWqRnoDQ= -github.com/Layr-Labs/eigenda v0.8.5-0.20241031144746-e2ead56a306d/go.mod h1:sqUNf9Ak+EfAX82jDxrb4QbT/g3DViWD3b7YIk36skk= +github.com/Layr-Labs/eigenda v0.8.5-rc.0.0.20241101212705-fa8776ae648c h1:TuvZlhWSrwpG6EPl+xjOo5UCp2QcVGl+EOY+BalqOXg= +github.com/Layr-Labs/eigenda v0.8.5-rc.0.0.20241101212705-fa8776ae648c/go.mod h1:sqUNf9Ak+EfAX82jDxrb4QbT/g3DViWD3b7YIk36skk= github.com/Layr-Labs/eigensdk-go v0.1.7-0.20240507215523-7e4891d5099a h1:L/UsJFw9M31FD/WgXTPFB0oxbq9Cu4Urea1xWPMQS7Y= github.com/Layr-Labs/eigensdk-go v0.1.7-0.20240507215523-7e4891d5099a/go.mod h1:OF9lmS/57MKxS0xpSpX0qHZl0SKkDRpvJIvsGvMN1y8= github.com/Microsoft/go-winio v0.6.2 h1:F2VQgta7ecxGYO8k3ZZz3RS8fVIXVxONVUPlNERoyfY= From 8a8fcd6020967ce0d960adfb41c6f590989de56f Mon Sep 17 00:00:00 2001 From: Samuel Laferriere Date: Fri, 1 Nov 2024 22:08:54 +0000 Subject: [PATCH 07/11] tests(handlers): add tests for error types (including failover) --- server/handlers_test.go | 122 ++++++++++++++++++++++++++++++---------- 1 file changed, 91 insertions(+), 31 deletions(-) diff --git a/server/handlers_test.go b/server/handlers_test.go index 336d6f73..42f985ee 100644 --- a/server/handlers_test.go +++ b/server/handlers_test.go @@ -8,14 +8,20 @@ import ( "fmt" "net/http" "net/http/httptest" + "os" + "strings" "testing" "github.com/Layr-Labs/eigenda-proxy/metrics" "github.com/Layr-Labs/eigenda-proxy/mocks" + "github.com/Layr-Labs/eigenda-proxy/store" + "github.com/Layr-Labs/eigenda/api" "github.com/ethereum/go-ethereum/log" "github.com/golang/mock/gomock" "github.com/gorilla/mux" "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) const ( @@ -105,7 +111,7 @@ func TestHandlerGet(t *testing.T) { } } -func TestHandlerPut(t *testing.T) { +func TestHandlerPutSuccess(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() mockStorageMgr := mocks.NewMockIManager(ctrl) @@ -118,16 +124,6 @@ func TestHandlerPut(t *testing.T) { expectedCode int expectedBody string }{ - { - name: "Failure OP Mode Alt-DA - InternalServerError", - url: "/put", - body: []byte("some data that will trigger an internal error"), - mockBehavior: func() { - mockStorageMgr.EXPECT().Put(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("internal error")) - }, - expectedCode: http.StatusInternalServerError, - expectedBody: "", - }, { name: "Success OP Mode Alt-DA", url: "/put", @@ -138,16 +134,6 @@ func TestHandlerPut(t *testing.T) { expectedCode: http.StatusOK, expectedBody: opGenericPrefixStr + testCommitStr, }, - { - name: "Failure OP Mode Keccak256 - InternalServerError", - url: fmt.Sprintf("/put/0x00%s", testCommitStr), - body: []byte("some data that will trigger an internal error"), - mockBehavior: func() { - mockStorageMgr.EXPECT().Put(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("internal error")) - }, - expectedCode: http.StatusInternalServerError, - expectedBody: "", - }, { name: "Success OP Mode Keccak256", url: fmt.Sprintf("/put/0x00%s", testCommitStr), @@ -158,16 +144,6 @@ func TestHandlerPut(t *testing.T) { expectedCode: http.StatusOK, expectedBody: "", }, - { - name: "Failure Simple Commitment Mode - InternalServerError", - url: "/put?commitment_mode=simple", - body: []byte("some data that will trigger an internal error"), - mockBehavior: func() { - mockStorageMgr.EXPECT().Put(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("internal error")) - }, - expectedCode: http.StatusInternalServerError, - expectedBody: "", - }, { name: "Success Simple Commitment Mode", url: "/put?commitment_mode=simple", @@ -206,3 +182,87 @@ func TestHandlerPut(t *testing.T) { }) } } + +func TestHandlerPutErrors(t *testing.T) { + // Each test is run against all 3 different modes. + modes := []struct { + name string + url string + }{ + { + name: "OP Mode Alt-DA", + url: "/put", + }, + { + name: "OP Mode Keccak256", + url: fmt.Sprintf("/put/0x00%s", testCommitStr), + }, + { + name: "Simple Commitment Mode", + url: "/put?commitment_mode=simple", + }, + } + + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mockStorageMgr := mocks.NewMockIManager(ctrl) + + tests := []struct { + name string + mockStorageMgrPutReturnedErr error + expectedHTTPCode int + }{ + { + // we only test OK status here. Returned commitment is checked in TestHandlerPut + name: "Success - 200", + mockStorageMgrPutReturnedErr: nil, + expectedHTTPCode: http.StatusOK, + }, + { + name: "Failure - InternalServerError 500", + mockStorageMgrPutReturnedErr: fmt.Errorf("internal error"), + expectedHTTPCode: http.StatusInternalServerError, + }, + { + // if /put results in ErrorFailover (returned by eigenda-client), we should return 503 + name: "Failure - Failover 503", + mockStorageMgrPutReturnedErr: &api.ErrorFailover{}, + expectedHTTPCode: http.StatusServiceUnavailable, + }, + { + name: "Failure - TooManyRequests 429", + mockStorageMgrPutReturnedErr: status.Errorf(codes.ResourceExhausted, "too many requests"), + expectedHTTPCode: http.StatusTooManyRequests, + }, + { + // only 400s are due to oversized blobs right now + name: "Failure - BadRequest 400", + mockStorageMgrPutReturnedErr: store.ErrProxyOversizedBlob, + expectedHTTPCode: http.StatusBadRequest, + }, + } + + for _, tt := range tests { + for _, mode := range modes { + t.Run(tt.name+" / "+mode.name, func(t *testing.T) { + mockStorageMgr.EXPECT().Put(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, tt.mockStorageMgrPutReturnedErr) + + req := httptest.NewRequest(http.MethodPost, mode.url, strings.NewReader("optional body to be sent to eigenda")) + rec := httptest.NewRecorder() + + // To add the vars to the context, + // we need to create a router through which we can pass the request. + r := mux.NewRouter() + // enable this logger to help debug tests + logger := log.NewLogger(log.NewTerminalHandler(os.Stdout, true)).With("test_name", t.Name()) + // noopLogger := log.NewLogger(log.DiscardHandler()) + server := NewServer("localhost", 0, mockStorageMgr, logger, metrics.NoopMetrics) + server.registerRoutes(r) + r.ServeHTTP(rec, req) + + require.Equal(t, tt.expectedHTTPCode, rec.Code) + }) + } + } + +} From 574a7628c53d2a14935dca5ee2ec3cf93b8b33d7 Mon Sep 17 00:00:00 2001 From: Samuel Laferriere Date: Fri, 1 Nov 2024 22:20:46 +0000 Subject: [PATCH 08/11] fix: errors after rebase --- common/store.go | 3 +-- server/errors.go | 4 ++-- server/handlers_test.go | 4 ++-- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/common/store.go b/common/store.go index 62e4660f..2d508538 100644 --- a/common/store.go +++ b/common/store.go @@ -19,8 +19,7 @@ const ( ) var ( - ErrProxyOversizedBlob = fmt.Errorf("encoded blob is larger than max blob size") - ErrEigenDAOversizedBlob = fmt.Errorf("blob size cannot exceed") + ErrProxyOversizedBlob = fmt.Errorf("encoded blob is larger than max blob size") ) func (b BackendType) String() string { diff --git a/server/errors.go b/server/errors.go index a3435497..cba6e10e 100644 --- a/server/errors.go +++ b/server/errors.go @@ -5,7 +5,7 @@ import ( "fmt" "github.com/Layr-Labs/eigenda-proxy/commitments" - "github.com/Layr-Labs/eigenda-proxy/store" + "github.com/Layr-Labs/eigenda-proxy/common" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) @@ -35,7 +35,7 @@ func is400(err error) bool { // IFFT'ing or encoding the blob, so we shouldn't return a 400 to the client. // See https://github.com/Layr-Labs/eigenda/blob/bee55ed9207f16153c3fd8ebf73c219e68685def/api/errors.go#L22 // for the 400s returned by the disperser server (currently only INVALID_ARGUMENT). - return errors.Is(err, store.ErrProxyOversizedBlob) + return errors.Is(err, common.ErrProxyOversizedBlob) } func is429(err error) bool { diff --git a/server/handlers_test.go b/server/handlers_test.go index 42f985ee..97958826 100644 --- a/server/handlers_test.go +++ b/server/handlers_test.go @@ -12,9 +12,9 @@ import ( "strings" "testing" + "github.com/Layr-Labs/eigenda-proxy/common" "github.com/Layr-Labs/eigenda-proxy/metrics" "github.com/Layr-Labs/eigenda-proxy/mocks" - "github.com/Layr-Labs/eigenda-proxy/store" "github.com/Layr-Labs/eigenda/api" "github.com/ethereum/go-ethereum/log" "github.com/golang/mock/gomock" @@ -237,7 +237,7 @@ func TestHandlerPutErrors(t *testing.T) { { // only 400s are due to oversized blobs right now name: "Failure - BadRequest 400", - mockStorageMgrPutReturnedErr: store.ErrProxyOversizedBlob, + mockStorageMgrPutReturnedErr: common.ErrProxyOversizedBlob, expectedHTTPCode: http.StatusBadRequest, }, } From 0c9fd978c15da8c5e18ad25daf65988e01e0c270 Mon Sep 17 00:00:00 2001 From: Samuel Laferriere Date: Mon, 4 Nov 2024 12:03:33 +0000 Subject: [PATCH 09/11] flags: clearer usage string for eigenda-client ResponseTimeoutFlag --- flags/eigendaflags/cli.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flags/eigendaflags/cli.go b/flags/eigendaflags/cli.go index 30937401..f186834c 100644 --- a/flags/eigendaflags/cli.go +++ b/flags/eigendaflags/cli.go @@ -51,7 +51,7 @@ func CLIFlags(envPrefix, category string) []cli.Flag { }, &cli.DurationFlag{ Name: ResponseTimeoutFlagName, - Usage: "Total time to wait for a response from the EigenDA disperser. Default is 60 seconds.", + Usage: "Flag used to configure the underlying disperser-client. Total time to wait for the disperseBlob call to return or disperseAuthenticatedBlob stream to finish and close.", Value: 60 * time.Second, EnvVars: []string{withEnvPrefix(envPrefix, "RESPONSE_TIMEOUT")}, Category: category, From b40f2be5d2b6060898af8475af4c11e4fcfd32db Mon Sep 17 00:00:00 2001 From: Samuel Laferriere Date: Thu, 7 Nov 2024 18:43:08 +0400 Subject: [PATCH 10/11] style: define is503 function to follow isABC pattern --- server/errors.go | 7 +++++++ server/handlers.go | 3 +-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/server/errors.go b/server/errors.go index cba6e10e..e078a88c 100644 --- a/server/errors.go +++ b/server/errors.go @@ -6,6 +6,7 @@ import ( "github.com/Layr-Labs/eigenda-proxy/commitments" "github.com/Layr-Labs/eigenda-proxy/common" + "github.com/Layr-Labs/eigenda/api" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) @@ -44,3 +45,9 @@ func is429(err error) bool { st, isGRPCError := status.FromError(err) return isGRPCError && st.Code() == codes.ResourceExhausted } + +// 503 is returned to tell the caller (batcher) to failover to ethda b/c eigenda is temporarily down +func is503(err error) bool { + // TODO: would be cleaner to define a sentinel error in eigenda-core and use that instead + return errors.Is(err, &api.ErrorFailover{}) +} \ No newline at end of file diff --git a/server/handlers.go b/server/handlers.go index f38ca817..8cb4e27c 100644 --- a/server/handlers.go +++ b/server/handlers.go @@ -9,7 +9,6 @@ import ( "net/http" "github.com/Layr-Labs/eigenda-proxy/commitments" - "github.com/Layr-Labs/eigenda/api" "github.com/gorilla/mux" ) @@ -186,7 +185,7 @@ func (svr *Server) handlePostShared(w http.ResponseWriter, r *http.Request, comm http.Error(w, err.Error(), http.StatusBadRequest) case is429(err): http.Error(w, err.Error(), http.StatusTooManyRequests) - case errors.Is(err, &api.ErrorFailover{}): + case is503(err): // this tells the caller (batcher) to failover to ethda b/c eigenda is temporarily down http.Error(w, err.Error(), http.StatusServiceUnavailable) default: From d50655702703e6dd4ef560ba7212f4fa1ed6d6c9 Mon Sep 17 00:00:00 2001 From: Samuel Laferriere Date: Thu, 7 Nov 2024 18:52:40 +0400 Subject: [PATCH 11/11] style: make lint --- server/errors.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/errors.go b/server/errors.go index e078a88c..c53fdf7d 100644 --- a/server/errors.go +++ b/server/errors.go @@ -50,4 +50,4 @@ func is429(err error) bool { func is503(err error) bool { // TODO: would be cleaner to define a sentinel error in eigenda-core and use that instead return errors.Is(err, &api.ErrorFailover{}) -} \ No newline at end of file +}