From 64306afa24addef9dbd4838397b1205185d4d6a2 Mon Sep 17 00:00:00 2001 From: Joe Clapis Date: Tue, 7 May 2024 13:38:18 -0400 Subject: [PATCH 1/9] Added more error checking to the API server response handler --- api/server/handle-response.go | 74 +++++++++++++++++------------------ api/server/queryless.go | 40 +++++++++++++++---- api/server/single-stage.go | 40 +++++++++++++++---- 3 files changed, 101 insertions(+), 53 deletions(-) diff --git a/api/server/handle-response.go b/api/server/handle-response.go index 33a1906..268f24d 100644 --- a/api/server/handle-response.go +++ b/api/server/handle-response.go @@ -20,108 +20,107 @@ const ( ) // Handle routes called with an invalid method -func HandleInvalidMethod(logger *slog.Logger, w http.ResponseWriter) { - writeResponse(w, logger, http.StatusMethodNotAllowed, "", nil, []byte{}) +func HandleInvalidMethod(logger *slog.Logger, w http.ResponseWriter) error { + return writeResponse(w, logger, http.StatusMethodNotAllowed, "", nil, []byte{}) } // Handles an error related to parsing the input parameters of a request -func HandleInputError(logger *slog.Logger, w http.ResponseWriter, err error) { +func HandleInputError(logger *slog.Logger, w http.ResponseWriter, err error) error { msg := err.Error() - writeResponse(w, logger, http.StatusBadRequest, "", err, formatError(msg)) + return writeResponse(w, logger, http.StatusBadRequest, "", err, formatError(msg)) } // The request couldn't complete because the node requires an address but one wasn't present -func HandleAddressNotPresent(logger *slog.Logger, w http.ResponseWriter, err error) { +func HandleAddressNotPresent(logger *slog.Logger, w http.ResponseWriter, err error) error { msg := fmt.Sprintf(addressNotPresentMessage, err.Error()) - writeResponse(w, logger, http.StatusUnprocessableEntity, "Address not present", err, formatError(msg)) + return writeResponse(w, logger, http.StatusUnprocessableEntity, "Address not present", err, formatError(msg)) } // The request couldn't complete because the node requires a wallet but one isn't present or useable -func HandleWalletNotReady(logger *slog.Logger, w http.ResponseWriter, err error) { +func HandleWalletNotReady(logger *slog.Logger, w http.ResponseWriter, err error) error { msg := fmt.Sprintf(walletNotReadyMessage, err.Error()) - writeResponse(w, logger, http.StatusUnprocessableEntity, "Wallet not ready", err, formatError(msg)) + return writeResponse(w, logger, http.StatusUnprocessableEntity, "Wallet not ready", err, formatError(msg)) } // The request couldn't complete because it's trying to create a resource that already exists, or use a resource that conflicts with what's requested -func HandleResourceConflict(logger *slog.Logger, w http.ResponseWriter, err error) { +func HandleResourceConflict(logger *slog.Logger, w http.ResponseWriter, err error) error { msg := fmt.Sprintf(resourceConflictMessage, err.Error()) - writeResponse(w, logger, http.StatusConflict, "Resource conflict", err, formatError(msg)) + return writeResponse(w, logger, http.StatusConflict, "Resource conflict", err, formatError(msg)) } // The request couldn't complete because it's trying to access a resource that didn't exist or couldn't be found -func HandleResourceNotFound(logger *slog.Logger, w http.ResponseWriter, err error) { +func HandleResourceNotFound(logger *slog.Logger, w http.ResponseWriter, err error) error { msg := fmt.Sprintf(resourceNotFoundMessage, err.Error()) - writeResponse(w, logger, http.StatusNotFound, "Resource not found", err, formatError(msg)) + return writeResponse(w, logger, http.StatusNotFound, "Resource not found", err, formatError(msg)) } // The request couldn't complete because the clients aren't synced yet -func HandleClientNotSynced(logger *slog.Logger, w http.ResponseWriter, err error) { +func HandleClientNotSynced(logger *slog.Logger, w http.ResponseWriter, err error) error { msg := clientsNotSyncedMessage - writeResponse(w, logger, http.StatusUnprocessableEntity, "Clients not synced", err, formatError(msg)) + return writeResponse(w, logger, http.StatusUnprocessableEntity, "Clients not synced", err, formatError(msg)) } // The request couldn't complete because the chain state is preventing the request (it will revert if submitted) -func HandleInvalidChainState(logger *slog.Logger, w http.ResponseWriter, err error) { +func HandleInvalidChainState(logger *slog.Logger, w http.ResponseWriter, err error) error { msg := fmt.Sprintf(invalidChainStateMessage, err.Error()) - writeResponse(w, logger, http.StatusUnprocessableEntity, "Invalid chain state", err, formatError(msg)) + return writeResponse(w, logger, http.StatusUnprocessableEntity, "Invalid chain state", err, formatError(msg)) } // The request couldn't complete because of a server error -func HandleServerError(logger *slog.Logger, w http.ResponseWriter, err error) { +func HandleServerError(logger *slog.Logger, w http.ResponseWriter, err error) error { msg := err.Error() - writeResponse(w, logger, http.StatusInternalServerError, "", err, formatError(msg)) + return writeResponse(w, logger, http.StatusInternalServerError, "", err, formatError(msg)) } // The request completed successfully -func HandleSuccess(logger *slog.Logger, w http.ResponseWriter, response any) { +func HandleSuccess(logger *slog.Logger, w http.ResponseWriter, response any) error { // Serialize the response bytes, err := json.Marshal(response) if err != nil { - HandleServerError(logger, w, fmt.Errorf("error serializing response: %w", err)) - return + return HandleServerError(logger, w, fmt.Errorf("error serializing response: %w", err)) } // Write it logger.Debug("Response body", slog.String(log.BodyKey, string(bytes))) - writeResponse(w, logger, http.StatusOK, "", nil, bytes) + return writeResponse(w, logger, http.StatusOK, "", nil, bytes) } // Handles an API response for a request that could not be completed -func HandleFailedResponse(logger *slog.Logger, w http.ResponseWriter, status types.ResponseStatus, err error) { +func HandleFailedResponse(logger *slog.Logger, w http.ResponseWriter, status types.ResponseStatus, err error) error { switch status { case types.ResponseStatus_InvalidArguments: - HandleInputError(logger, w, err) + return HandleInputError(logger, w, err) case types.ResponseStatus_AddressNotPresent: - HandleAddressNotPresent(logger, w, err) + return HandleAddressNotPresent(logger, w, err) case types.ResponseStatus_WalletNotReady: - HandleWalletNotReady(logger, w, err) + return HandleWalletNotReady(logger, w, err) case types.ResponseStatus_ResourceConflict: - HandleResourceConflict(logger, w, err) + return HandleResourceConflict(logger, w, err) case types.ResponseStatus_ResourceNotFound: - HandleResourceNotFound(logger, w, err) + return HandleResourceNotFound(logger, w, err) case types.ResponseStatus_ClientsNotSynced: - HandleClientNotSynced(logger, w, err) + return HandleClientNotSynced(logger, w, err) case types.ResponseStatus_InvalidChainState: - HandleInvalidChainState(logger, w, err) + return HandleInvalidChainState(logger, w, err) case types.ResponseStatus_Error: - HandleServerError(logger, w, err) + return HandleServerError(logger, w, err) default: - HandleServerError(logger, w, fmt.Errorf("unknown response status: %d", status)) + return HandleServerError(logger, w, fmt.Errorf("unknown response status: %d", status)) } } // Handles an API response -func HandleResponse(logger *slog.Logger, w http.ResponseWriter, status types.ResponseStatus, response any, err error) { +func HandleResponse(logger *slog.Logger, w http.ResponseWriter, status types.ResponseStatus, response any, err error) error { switch status { case types.ResponseStatus_Success: - HandleSuccess(logger, w, response) + return HandleSuccess(logger, w, response) default: - HandleFailedResponse(logger, w, status, err) + return HandleFailedResponse(logger, w, status, err) } } // Writes a response to an HTTP request back to the client and logs it -func writeResponse(w http.ResponseWriter, logger *slog.Logger, statusCode int, cause string, err error, message []byte) { +func writeResponse(w http.ResponseWriter, logger *slog.Logger, statusCode int, cause string, err error, message []byte) error { // Prep the log attributes codeMsg := fmt.Sprintf("%d %s", statusCode, http.StatusText(statusCode)) attrs := []any{ @@ -148,7 +147,8 @@ func writeResponse(w http.ResponseWriter, logger *slog.Logger, statusCode int, c // Write it to the client w.Header().Add("Content-Type", "application/json") w.WriteHeader(statusCode) - w.Write(message) + _, writeErr := w.Write(message) + return writeErr } // JSONifies an error for responding to requests diff --git a/api/server/queryless.go b/api/server/queryless.go index c6cef56..167f880 100644 --- a/api/server/queryless.go +++ b/api/server/queryless.go @@ -58,20 +58,29 @@ func RegisterQuerylessGet[ContextType IQuerylessCallContext[DataType], DataType // Check the method if r.Method != http.MethodGet { - HandleInvalidMethod(logger, w) + err := HandleInvalidMethod(logger, w) + if err != nil { + logger.Error("Error handling response", log.Err(err)) + } return } // Create the handler and deal with any input validation errors context, err := factory.Create(args) if err != nil { - HandleInputError(logger, w, err) + err = HandleInputError(logger, w, err) + if err != nil { + logger.Error("Error handling response", log.Err(err)) + } return } // Run the context's processing routine status, response, err := runQuerylessRoute[DataType](context, serviceProvider) - HandleResponse(logger, w, status, response, err) + err = HandleResponse(logger, w, status, response, err) + if err != nil { + logger.Error("Error handling response", log.Err(err)) + } }) } @@ -90,14 +99,20 @@ func RegisterQuerylessPost[ContextType IQuerylessCallContext[DataType], BodyType // Check the method if r.Method != http.MethodPost { - HandleInvalidMethod(logger, w) + err := HandleInvalidMethod(logger, w) + if err != nil { + logger.Error("Error handling response", log.Err(err)) + } return } // Read the body bodyBytes, err := io.ReadAll(r.Body) if err != nil { - HandleInputError(logger, w, fmt.Errorf("error reading request body: %w", err)) + err = HandleInputError(logger, w, fmt.Errorf("error reading request body: %w", err)) + if err != nil { + logger.Error("Error handling response", log.Err(err)) + } return } logger.Debug("Request body:", slog.String(log.BodyKey, string(bodyBytes))) @@ -106,20 +121,29 @@ func RegisterQuerylessPost[ContextType IQuerylessCallContext[DataType], BodyType var body BodyType err = json.Unmarshal(bodyBytes, &body) if err != nil { - HandleInputError(logger, w, fmt.Errorf("error deserializing request body: %w", err)) + err = HandleInputError(logger, w, fmt.Errorf("error deserializing request body: %w", err)) + if err != nil { + logger.Error("Error handling response", log.Err(err)) + } return } // Create the handler and deal with any input validation errors context, err := factory.Create(body) if err != nil { - HandleInputError(logger, w, err) + err = HandleInputError(logger, w, err) + if err != nil { + logger.Error("Error handling response", log.Err(err)) + } return } // Run the context's processing routine status, response, err := runQuerylessRoute[DataType](context, serviceProvider) - HandleResponse(logger, w, status, response, err) + err = HandleResponse(logger, w, status, response, err) + if err != nil { + logger.Error("Error handling response", log.Err(err)) + } }) } diff --git a/api/server/single-stage.go b/api/server/single-stage.go index e27c7d4..8738bd7 100644 --- a/api/server/single-stage.go +++ b/api/server/single-stage.go @@ -62,20 +62,29 @@ func RegisterSingleStageRoute[ContextType ISingleStageCallContext[DataType], Dat // Check the method if r.Method != http.MethodGet { - HandleInvalidMethod(logger, w) + err := HandleInvalidMethod(logger, w) + if err != nil { + logger.Error("Error handling response", log.Err(err)) + } return } // Create the handler and deal with any input validation errors context, err := factory.Create(args) if err != nil { - HandleInputError(logger, w, err) + err = HandleInputError(logger, w, err) + if err != nil { + logger.Error("Error handling response", log.Err(err)) + } return } // Run the context's processing routine status, response, err := runSingleStageRoute[DataType](context, serviceProvider) - HandleResponse(logger, w, status, response, err) + err = HandleResponse(logger, w, status, response, err) + if err != nil { + logger.Error("Error handling response", log.Err(err)) + } }) } @@ -94,14 +103,20 @@ func RegisterSingleStagePost[ContextType ISingleStageCallContext[DataType], Body // Check the method if r.Method != http.MethodPost { - HandleInvalidMethod(logger, w) + err := HandleInvalidMethod(logger, w) + if err != nil { + logger.Error("Error handling response", log.Err(err)) + } return } // Read the body bodyBytes, err := io.ReadAll(r.Body) if err != nil { - HandleInputError(logger, w, fmt.Errorf("error reading request body: %w", err)) + err = HandleInputError(logger, w, fmt.Errorf("error reading request body: %w", err)) + if err != nil { + logger.Error("Error handling response", log.Err(err)) + } return } logger.Debug("Body", slog.String(log.BodyKey, string(bodyBytes))) @@ -110,20 +125,29 @@ func RegisterSingleStagePost[ContextType ISingleStageCallContext[DataType], Body var body BodyType err = json.Unmarshal(bodyBytes, &body) if err != nil { - HandleInputError(logger, w, fmt.Errorf("error deserializing request body: %w", err)) + err = HandleInputError(logger, w, fmt.Errorf("error deserializing request body: %w", err)) + if err != nil { + logger.Error("Error handling response", log.Err(err)) + } return } // Create the handler and deal with any input validation errors context, err := factory.Create(body) if err != nil { - HandleInputError(logger, w, err) + err = HandleInputError(logger, w, err) + if err != nil { + logger.Error("Error handling response", log.Err(err)) + } return } // Run the context's processing routine status, response, err := runSingleStageRoute[DataType](context, serviceProvider) - HandleResponse(logger, w, status, response, err) + err = HandleResponse(logger, w, status, response, err) + if err != nil { + logger.Error("Error handling response", log.Err(err)) + } }) } From 1fb5c25b7b6e9fe26b061af0b0ea790250b85637 Mon Sep 17 00:00:00 2001 From: Joe Clapis Date: Tue, 7 May 2024 13:39:05 -0400 Subject: [PATCH 2/9] Pulled private methods out of IClientManager --- node/services/bn-manager.go | 4 ++-- node/services/ec-manager.go | 4 ++-- node/services/function-runners.go | 10 +++++----- node/services/manager.go | 8 ++++++-- 4 files changed, 15 insertions(+), 11 deletions(-) diff --git a/node/services/bn-manager.go b/node/services/bn-manager.go index ebcaae8..48a9f03 100644 --- a/node/services/bn-manager.go +++ b/node/services/bn-manager.go @@ -65,11 +65,11 @@ func (m *BeaconClientManager) GetClientTypeName() string { return "Beacon Node" } -func (m *BeaconClientManager) setPrimaryReady(ready bool) { +func (m *BeaconClientManager) SetPrimaryReady(ready bool) { m.primaryReady = ready } -func (m *BeaconClientManager) setFallbackReady(ready bool) { +func (m *BeaconClientManager) SetFallbackReady(ready bool) { m.fallbackReady = ready } diff --git a/node/services/ec-manager.go b/node/services/ec-manager.go index 35b2fec..ef73d6d 100644 --- a/node/services/ec-manager.go +++ b/node/services/ec-manager.go @@ -83,11 +83,11 @@ func (m *ExecutionClientManager) GetClientTypeName() string { return "Execution Client" } -func (m *ExecutionClientManager) setPrimaryReady(ready bool) { +func (m *ExecutionClientManager) SetPrimaryReady(ready bool) { m.primaryReady = ready } -func (m *ExecutionClientManager) setFallbackReady(ready bool) { +func (m *ExecutionClientManager) SetFallbackReady(ready bool) { m.fallbackReady = ready } diff --git a/node/services/function-runners.go b/node/services/function-runners.go index b724729..5db6330 100644 --- a/node/services/function-runners.go +++ b/node/services/function-runners.go @@ -18,7 +18,7 @@ type function2[ClientType any, ReturnType1 any, ReturnType2 any] func(ClientType // Attempts to run a function progressively through each client until one succeeds or they all fail. // Expects functions with 1 output and an error; for functions with other signatures, see the other runFunctionX functions. -func runFunction1[ClientType any, ReturnType any](m IClientManager[ClientType], ctx context.Context, function function1[ClientType, ReturnType]) (ReturnType, error) { +func runFunction1[ClientType any, ReturnType any](m iClientManagerImpl[ClientType], ctx context.Context, function function1[ClientType, ReturnType]) (ReturnType, error) { logger, _ := log.FromContext(ctx) var blank ReturnType typeName := m.GetClientTypeName() @@ -30,7 +30,7 @@ func runFunction1[ClientType any, ReturnType any](m IClientManager[ClientType], if err != nil { if isDisconnected(err) { // If it's disconnected, log it and try the fallback - m.setPrimaryReady(false) + m.SetPrimaryReady(false) if m.IsFallbackEnabled() { logger.Warn("Primary "+typeName+" client disconnected, using fallback...", log.Err(err)) return runFunction1[ClientType, ReturnType](m, ctx, function) @@ -53,7 +53,7 @@ func runFunction1[ClientType any, ReturnType any](m IClientManager[ClientType], if isDisconnected(err) { // If it's disconnected, log it and try the fallback logger.Warn("Fallback "+typeName+" disconnected", log.Err(err)) - m.setFallbackReady(false) + m.SetFallbackReady(false) return blank, fmt.Errorf("all " + typeName + "s failed") } @@ -68,7 +68,7 @@ func runFunction1[ClientType any, ReturnType any](m IClientManager[ClientType], } // Run a function with 0 outputs and an error -func runFunction0[ClientType any](m IClientManager[ClientType], ctx context.Context, function function0[ClientType]) error { +func runFunction0[ClientType any](m iClientManagerImpl[ClientType], ctx context.Context, function function0[ClientType]) error { _, err := runFunction1(m, ctx, func(client ClientType) (any, error) { return nil, function(client) }) @@ -76,7 +76,7 @@ func runFunction0[ClientType any](m IClientManager[ClientType], ctx context.Cont } // Run a function with 2 outputs and an error -func runFunction2[ClientType any, ReturnType1 any, ReturnType2 any](m IClientManager[ClientType], ctx context.Context, function function2[ClientType, ReturnType1, ReturnType2]) (ReturnType1, ReturnType2, error) { +func runFunction2[ClientType any, ReturnType1 any, ReturnType2 any](m iClientManagerImpl[ClientType], ctx context.Context, function function2[ClientType, ReturnType1, ReturnType2]) (ReturnType1, ReturnType2, error) { type out struct { arg1 ReturnType1 arg2 ReturnType2 diff --git a/node/services/manager.go b/node/services/manager.go index 19618f9..b57dfae 100644 --- a/node/services/manager.go +++ b/node/services/manager.go @@ -7,8 +7,12 @@ type IClientManager[ClientType any] interface { IsFallbackReady() bool IsFallbackEnabled() bool GetClientTypeName() string +} + +type iClientManagerImpl[ClientType any] interface { + IClientManager[ClientType] // Internal functions - setPrimaryReady(bool) - setFallbackReady(bool) + SetPrimaryReady(bool) + SetFallbackReady(bool) } From c6452ca00e19cc98d283747d1e34a54c8509bd70 Mon Sep 17 00:00:00 2001 From: Joe Clapis Date: Tue, 7 May 2024 13:40:38 -0400 Subject: [PATCH 3/9] Various minor linter cleanup --- beacon/client/std-http-client.go | 2 ++ config/utils.go | 4 ++-- node/validator/keystore/lodestar.go | 5 ++--- utils/input/validation.go | 6 ++---- 4 files changed, 8 insertions(+), 9 deletions(-) diff --git a/beacon/client/std-http-client.go b/beacon/client/std-http-client.go index eb5f1a3..4b546a1 100644 --- a/beacon/client/std-http-client.go +++ b/beacon/client/std-http-client.go @@ -653,6 +653,7 @@ func (c *StandardHttpClient) getFinalityCheckpoints(ctx context.Context, stateId } // Get fork +/* func (c *StandardHttpClient) getFork(ctx context.Context, stateId string) (ForkResponse, error) { responseBody, status, err := c.getRequest(ctx, fmt.Sprintf(RequestForkPath, stateId)) if err != nil { @@ -667,6 +668,7 @@ func (c *StandardHttpClient) getFork(ctx context.Context, stateId string) (ForkR } return fork, nil } +*/ // Get validators func (c *StandardHttpClient) getValidators(ctx context.Context, stateId string, pubkeys []string) (ValidatorsResponse, error) { diff --git a/config/utils.go b/config/utils.go index 2aa3e31..85ce553 100644 --- a/config/utils.go +++ b/config/utils.go @@ -42,14 +42,14 @@ func GetPortModes(warningOverride string) []*ParameterOption[RpcPortMode] { func GetExternalIP() (net.IP, error) { // Try IPv4 first ip4Consensus := externalip.DefaultConsensus(nil, nil) - ip4Consensus.UseIPProtocol(4) + _ = ip4Consensus.UseIPProtocol(4) if ip, err := ip4Consensus.ExternalIP(); err == nil { return ip, nil } // Try IPv6 as fallback ip6Consensus := externalip.DefaultConsensus(nil, nil) - ip6Consensus.UseIPProtocol(6) + _ = ip6Consensus.UseIPProtocol(6) return ip6Consensus.ExternalIP() } diff --git a/node/validator/keystore/lodestar.go b/node/validator/keystore/lodestar.go index 01ccf86..8cc16fd 100644 --- a/node/validator/keystore/lodestar.go +++ b/node/validator/keystore/lodestar.go @@ -2,7 +2,6 @@ package keystore import ( "fmt" - "io/ioutil" "os" "path/filepath" @@ -80,7 +79,7 @@ func (ks *LodestarKeystoreManager) StoreValidatorKey(key *eth2types.BLSPrivateKe } // Write secret to disk - if err := ioutil.WriteFile(secretFilePath, []byte(password), FileMode); err != nil { + if err := os.WriteFile(secretFilePath, []byte(password), FileMode); err != nil { return fmt.Errorf("error writing validator secret to disk: %w", err) } @@ -93,7 +92,7 @@ func (ks *LodestarKeystoreManager) StoreValidatorKey(key *eth2types.BLSPrivateKe } // Write key store to disk - if err := ioutil.WriteFile(keyFilePath, keyStoreBytes, FileMode); err != nil { + if err := os.WriteFile(keyFilePath, keyStoreBytes, FileMode); err != nil { return fmt.Errorf("error writing validator key to disk: %w", err) } return nil diff --git a/utils/input/validation.go b/utils/input/validation.go index c550c23..fa67d82 100644 --- a/utils/input/validation.go +++ b/utils/input/validation.go @@ -201,7 +201,7 @@ func ValidateWalletMnemonic(name, value string) (string, error) { // Validate a timezone location func ValidateTimezoneLocation(name, value string) (string, error) { - if !regexp.MustCompile("^([a-zA-Z_]{2,}\\/)+[a-zA-Z_]{2,}$").MatchString(value) { + if !regexp.MustCompile(`^([a-zA-Z_]{2,}\/)+[a-zA-Z_]{2,}$`).MatchString(value) { return "", fmt.Errorf("Invalid %s '%s' - must be in the format 'Country/City'", name, value) } return value, nil @@ -260,9 +260,7 @@ func ValidatePubkey(name, value string) (beacon.ValidatorPubkey, error) { // Validate a hex-encoded byte array func ValidateByteArray(name, value string) ([]byte, error) { // Remove a 0x prefix if present - if strings.HasPrefix(value, "0x") { - value = value[2:] - } + value = strings.TrimPrefix(value, "0x") // Try to parse the string (removing the prefix) bytes, err := hex.DecodeString(value) From ac96f93a88ece0bed9baeebdd018d313db43c7c9 Mon Sep 17 00:00:00 2001 From: Joe Clapis Date: Tue, 7 May 2024 14:59:58 -0400 Subject: [PATCH 4/9] Updated CI workflows (SN commit 607ef85) --- .github/workflows/commits.yml | 2 +- .github/workflows/lint.yml | 5 +++-- .github/workflows/unit-tests.yml | 4 ++-- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/.github/workflows/commits.yml b/.github/workflows/commits.yml index 1c9df19..fde759a 100644 --- a/.github/workflows/commits.yml +++ b/.github/workflows/commits.yml @@ -9,6 +9,6 @@ jobs: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - name: Block Fixup Commit Merge uses: 13rac1/block-fixup-merge-action@v2.0.0 diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 7f7a6e0..f6d964e 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -16,10 +16,11 @@ jobs: name: lint runs-on: ubuntu-latest steps: - - uses: actions/setup-go@v4 + - uses: actions/setup-go@v5 with: go-version: 1.21.8 - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 + - run: go mod download all - name: golangci-lint uses: golangci/golangci-lint-action@v4 with: diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index 1ee6cbc..49e4d29 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -12,8 +12,8 @@ jobs: build: runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3 - - uses: actions/setup-go@v4 + - uses: actions/checkout@v4 + - uses: actions/setup-go@v5 with: go-version: 1.21.8 - run: go test ./... From b3de9a23eea1e06ffe6cff4dffc83914b61bb958 Mon Sep 17 00:00:00 2001 From: Joe Clapis Date: Tue, 14 May 2024 15:31:56 -0400 Subject: [PATCH 5/9] Updated clients --- config/besu-config.go | 4 ++-- config/geth-config.go | 4 ++-- config/lodestar-bn-config.go | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/config/besu-config.go b/config/besu-config.go index 24bbadc..7d9af91 100644 --- a/config/besu-config.go +++ b/config/besu-config.go @@ -7,8 +7,8 @@ import ( // Constants const ( // Tags - besuTagTest string = "hyperledger/besu:24.3.3" - besuTagProd string = "hyperledger/besu:24.3.3" + besuTagTest string = "hyperledger/besu:24.5.1" + besuTagProd string = "hyperledger/besu:24.5.1" ) // Configuration for Besu diff --git a/config/geth-config.go b/config/geth-config.go index ad8dc5d..0323f73 100644 --- a/config/geth-config.go +++ b/config/geth-config.go @@ -10,8 +10,8 @@ import ( // Constants const ( // Tags - gethTagProd string = "ethereum/client-go:v1.14.0" - gethTagTest string = "ethereum/client-go:v1.14.0" + gethTagProd string = "ethereum/client-go:v1.14.3" + gethTagTest string = "ethereum/client-go:v1.14.3" ) // Configuration for Geth diff --git a/config/lodestar-bn-config.go b/config/lodestar-bn-config.go index 5c9ed1a..aa11f33 100644 --- a/config/lodestar-bn-config.go +++ b/config/lodestar-bn-config.go @@ -5,8 +5,8 @@ import ( ) const ( - lodestarBnTagTest string = "chainsafe/lodestar:v1.18.0" - lodestarBnTagProd string = "chainsafe/lodestar:v1.18.0" + lodestarBnTagTest string = "chainsafe/lodestar:v1.18.1" + lodestarBnTagProd string = "chainsafe/lodestar:v1.18.1" ) // Configuration for the Lodestar BN From d3843839ab005d763beb3409c24b9d30cd4231ff Mon Sep 17 00:00:00 2001 From: Joe Clapis Date: Wed, 15 May 2024 10:37:43 -0400 Subject: [PATCH 6/9] Broke reth's max peers into separate inbound and outbound settings --- config/ids/ids.go | 4 ++++ config/local-execution-config.go | 3 ++- config/reth-config.go | 34 +++++++++++++++++++++++--------- 3 files changed, 31 insertions(+), 10 deletions(-) diff --git a/config/ids/ids.go b/config/ids/ids.go index c3200c7..0248c14 100644 --- a/config/ids/ids.go +++ b/config/ids/ids.go @@ -100,6 +100,10 @@ const ( PrysmOpenRpcPortID string = "openRpcPort" PrysmRpcUrlID string = "prysmRpcUrl" + // Reth + RethMaxInboundPeersID string = "maxInboundPeers" + RethMaxOutboundPeersID string = "maxOutboundPeers" + // Teku TekuJvmHeapSizeID string = "jvmHeapSize" TekuArchiveModeID string = "archiveMode" diff --git a/config/local-execution-config.go b/config/local-execution-config.go index d0c7abc..facbbe8 100644 --- a/config/local-execution-config.go +++ b/config/local-execution-config.go @@ -200,6 +200,7 @@ func (cfg *LocalExecutionConfig) GetOpenApiPortMapping() string { } // Gets the max peers of the selected EC +// Note that Reth treats the max peer count specially func (cfg *LocalExecutionConfig) GetMaxPeers() uint16 { switch cfg.ExecutionClient.Value { case ExecutionClient_Geth: @@ -209,7 +210,7 @@ func (cfg *LocalExecutionConfig) GetMaxPeers() uint16 { case ExecutionClient_Besu: return cfg.Besu.MaxPeers.Value case ExecutionClient_Reth: - return cfg.Reth.MaxPeers.Value + return cfg.Reth.MaxInboundPeers.Value + cfg.Reth.MaxOutboundPeers.Value default: panic(fmt.Sprintf("Unknown Execution Client %s", string(cfg.ExecutionClient.Value))) } diff --git a/config/reth-config.go b/config/reth-config.go index 873511c..c3132a6 100644 --- a/config/reth-config.go +++ b/config/reth-config.go @@ -18,8 +18,11 @@ type RethConfig struct { // Size of Reth's Cache CacheSize Parameter[uint64] - // Max number of P2P peers to connect to - MaxPeers Parameter[uint16] + // Max number of P2P peers that can connect to this node + MaxInboundPeers Parameter[uint16] + + // Max number of P2P peers to this node can connect to + MaxOutboundPeers Parameter[uint16] // The Docker Hub tag for Reth ContainerTag Parameter[string] @@ -45,11 +48,23 @@ func NewRethConfig() *RethConfig { }, }, - MaxPeers: Parameter[uint16]{ + MaxInboundPeers: Parameter[uint16]{ + ParameterCommon: &ParameterCommon{ + ID: ids.RethMaxInboundPeersID, + Name: "Max Inbound Peers", + Description: "The maximum number of inbound peers that should be allowed to connect to Reth (peers that request to connect to your node). This can be lowered to improve performance on low-power systems or constrained networks. Inbound peers requires you to have properly forwarded ports. We recommend keeping the sum of this and max outbound peers at 12 or higher.", + AffectsContainers: []ContainerID{ContainerID_ExecutionClient}, + CanBeBlank: false, + OverwriteOnUpgrade: false, + }, + Default: map[Network]uint16{Network_All: calculateRethPeers()}, + }, + + MaxOutboundPeers: Parameter[uint16]{ ParameterCommon: &ParameterCommon{ - ID: ids.MaxPeersID, - Name: "Max Peers", - Description: "The maximum number of peers Reth should connect to. This can be lowered to improve performance on low-power systems or constrained networks. We recommend keeping it at 12 or higher.", + ID: ids.RethMaxOutboundPeersID, + Name: "Max Outbound Peers", + Description: "The maximum number of outbound peers that Reth can connect to (peers that your node requests to connect to). This can be lowered to improve performance on low-power systems or constrained networks. Outbound peers do not require proper port forwarding, but are slower to accumulate than inbound peers. We recommend keeping the sum of this and max outbound peers at 12 or higher.", AffectsContainers: []ContainerID{ContainerID_ExecutionClient}, CanBeBlank: false, OverwriteOnUpgrade: false, @@ -97,7 +112,8 @@ func (cfg *RethConfig) GetTitle() string { func (cfg *RethConfig) GetParameters() []IParameter { return []IParameter{ &cfg.CacheSize, - &cfg.MaxPeers, + &cfg.MaxInboundPeers, + &cfg.MaxOutboundPeers, &cfg.ContainerTag, &cfg.AdditionalFlags, } @@ -132,7 +148,7 @@ func calculateRethCache() uint64 { // Calculate the default number of Reth peers func calculateRethPeers() uint16 { if runtime.GOARCH == "arm64" { - return 25 + return 12 } - return 50 + return 25 } From bcc14830771afc6be45803a3f0118d8b21741e06 Mon Sep 17 00:00:00 2001 From: Joe Clapis Date: Wed, 15 May 2024 10:45:06 -0400 Subject: [PATCH 7/9] Switched network ID to chain ID on the EC manager --- api/types/status.go | 2 +- node/services/ec-manager.go | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/api/types/status.go b/api/types/status.go index 19f7556..d12fb3b 100644 --- a/api/types/status.go +++ b/api/types/status.go @@ -5,7 +5,7 @@ type ClientStatus struct { IsWorking bool `json:"isWorking"` IsSynced bool `json:"isSynced"` SyncProgress float64 `json:"syncProgress"` - NetworkId uint `json:"networkId"` + ChainId uint `json:"networkId"` Error string `json:"error"` } diff --git a/node/services/ec-manager.go b/node/services/ec-manager.go index ef73d6d..4bfac4f 100644 --- a/node/services/ec-manager.go +++ b/node/services/ec-manager.go @@ -274,11 +274,11 @@ func (m *ExecutionClientManager) CheckStatus(ctx context.Context) *apitypes.Clie if status.FallbackEnabled { status.FallbackClientStatus = checkEcStatus(ctx, m.fallbackEc) // Check if fallback is using the expected network - if status.FallbackClientStatus.Error == "" && status.FallbackClientStatus.NetworkId != m.expectedChainID { + if status.FallbackClientStatus.Error == "" && status.FallbackClientStatus.ChainId != m.expectedChainID { m.fallbackReady = false colorReset := "\033[0m" colorYellow := "\033[33m" - status.FallbackClientStatus.Error = fmt.Sprintf("The fallback client is using a different chain [%s%s%s, Chain ID %d] than what your node is configured for [%s, Chain ID %d]", colorYellow, getNetworkNameFromId(status.FallbackClientStatus.NetworkId), colorReset, status.FallbackClientStatus.NetworkId, getNetworkNameFromId(m.expectedChainID), m.expectedChainID) + status.FallbackClientStatus.Error = fmt.Sprintf("The fallback client is using a different chain [%s%s%s, Chain ID %d] than what your node is configured for [%s, Chain ID %d]", colorYellow, getNetworkNameFromId(status.FallbackClientStatus.ChainId), colorReset, status.FallbackClientStatus.ChainId, getNetworkNameFromId(m.expectedChainID), m.expectedChainID) return status } } @@ -303,8 +303,8 @@ func getNetworkNameFromId(networkId uint) string { func checkEcStatus(ctx context.Context, client *ethclient.Client) apitypes.ClientStatus { status := apitypes.ClientStatus{} - // Get the NetworkId - networkId, err := client.NetworkID(ctx) + // Get the Chain ID + chainId, err := client.ChainID(ctx) if err != nil { status.Error = fmt.Sprintf("Sync progress check failed with [%s]", err.Error()) status.IsSynced = false @@ -312,8 +312,8 @@ func checkEcStatus(ctx context.Context, client *ethclient.Client) apitypes.Clien return status } - if networkId != nil { - status.NetworkId = uint(networkId.Uint64()) + if chainId != nil { + status.ChainId = uint(chainId.Uint64()) } // Get the fallback's sync progress From 2d59cd4dc1c91b1baf19cf077903e304211d52e8 Mon Sep 17 00:00:00 2001 From: Joe Clapis Date: Wed, 15 May 2024 11:10:47 -0400 Subject: [PATCH 8/9] Added chain ID check flags to the BN and EC managers --- node/services/bn-manager.go | 62 +++++++++++++++++++------------ node/services/ec-manager.go | 61 ++++++++++++++---------------- node/services/service-provider.go | 2 +- 3 files changed, 67 insertions(+), 58 deletions(-) diff --git a/node/services/bn-manager.go b/node/services/bn-manager.go index 48a9f03..ee34cb0 100644 --- a/node/services/bn-manager.go +++ b/node/services/bn-manager.go @@ -17,11 +17,11 @@ type BeaconClientManager struct { fallbackBc beacon.IBeaconClient primaryReady bool fallbackReady bool - ignoreSyncCheck bool + expectedChainID uint } // Creates a new BeaconClientManager instance -func NewBeaconClientManager(primaryProvider string, fallbackProvider string, clientTimeout time.Duration) (*BeaconClientManager, error) { +func NewBeaconClientManager(primaryProvider string, fallbackProvider string, chainID uint, clientTimeout time.Duration) (*BeaconClientManager, error) { var primaryBc beacon.IBeaconClient var fallbackBc beacon.IBeaconClient primaryBc = client.NewStandardHttpClient(primaryProvider, clientTimeout) @@ -30,10 +30,11 @@ func NewBeaconClientManager(primaryProvider string, fallbackProvider string, cli } return &BeaconClientManager{ - primaryBc: primaryBc, - fallbackBc: fallbackBc, - primaryReady: true, - fallbackReady: fallbackBc != nil, + primaryBc: primaryBc, + fallbackBc: fallbackBc, + primaryReady: true, + fallbackReady: fallbackBc != nil, + expectedChainID: chainID, }, nil } @@ -215,42 +216,55 @@ func (m *BeaconClientManager) ChangeWithdrawalCredentials(ctx context.Context, v /// ================= // Get the status of the primary and fallback clients -func (m *BeaconClientManager) CheckStatus(ctx context.Context) *types.ClientManagerStatus { +func (m *BeaconClientManager) CheckStatus(ctx context.Context, checkChainIDs bool) *types.ClientManagerStatus { status := &types.ClientManagerStatus{ FallbackEnabled: m.fallbackBc != nil, } - // Ignore the sync check and just use the predefined settings if requested - if m.ignoreSyncCheck { - status.PrimaryClientStatus.IsWorking = m.primaryReady - status.PrimaryClientStatus.IsSynced = m.primaryReady - if status.FallbackEnabled { - status.FallbackClientStatus.IsWorking = m.fallbackReady - status.FallbackClientStatus.IsSynced = m.fallbackReady - } - return status - } - // Get the primary BC status - status.PrimaryClientStatus = checkBcStatus(ctx, m.primaryBc) + status.PrimaryClientStatus = checkBcStatus(ctx, m.primaryBc, checkChainIDs) + if checkChainIDs && status.PrimaryClientStatus.Error == "" && status.PrimaryClientStatus.ChainId != m.expectedChainID { + m.primaryReady = false + status.PrimaryClientStatus.Error = fmt.Sprintf("The primary client is using a different chain (%d) than what your node is configured for (%d)", status.PrimaryClientStatus.ChainId, m.expectedChainID) + } else { + // Flag if primary client is ready + m.primaryReady = (status.PrimaryClientStatus.IsWorking && status.PrimaryClientStatus.IsSynced) + } // Get the fallback BC status if applicable if status.FallbackEnabled { - status.FallbackClientStatus = checkBcStatus(ctx, m.fallbackBc) + status.FallbackClientStatus = checkBcStatus(ctx, m.fallbackBc, checkChainIDs) + // Check if fallback is using the expected network + if checkChainIDs && status.FallbackClientStatus.Error == "" && status.FallbackClientStatus.ChainId != m.expectedChainID { + m.fallbackReady = false + status.FallbackClientStatus.Error = fmt.Sprintf("The fallback client is using a different chain (%d) than what your node is configured for (%d)", status.FallbackClientStatus.ChainId, m.expectedChainID) + return status + } } - // Flag the ready clients - m.primaryReady = (status.PrimaryClientStatus.IsWorking && status.PrimaryClientStatus.IsSynced) m.fallbackReady = (status.FallbackEnabled && status.FallbackClientStatus.IsWorking && status.FallbackClientStatus.IsSynced) return status } // Check the client status -func checkBcStatus(ctx context.Context, client beacon.IBeaconClient) types.ClientStatus { +func checkBcStatus(ctx context.Context, client beacon.IBeaconClient, checkChainIDs bool) types.ClientStatus { status := types.ClientStatus{} - // Get the fallback's sync progress + if checkChainIDs { + // Get the Chain ID + contractInfo, err := client.GetEth2DepositContract(ctx) + if err != nil { + status.Error = fmt.Sprintf("Chain ID check failed with [%s]", err.Error()) + status.IsSynced = false + status.IsWorking = false + return status + } + + status.ChainId = uint(contractInfo.ChainID) + } + + // Get the client's sync progress syncStatus, err := client.GetSyncStatus(ctx) if err != nil { status.Error = fmt.Sprintf("Sync progress check failed with [%s]", err.Error()) diff --git a/node/services/ec-manager.go b/node/services/ec-manager.go index 4bfac4f..13c82cf 100644 --- a/node/services/ec-manager.go +++ b/node/services/ec-manager.go @@ -259,26 +259,30 @@ func (m *ExecutionClientManager) SyncProgress(ctx context.Context) (*ethereum.Sy /// ================= // Get the status of the primary and fallback clients -func (m *ExecutionClientManager) CheckStatus(ctx context.Context) *apitypes.ClientManagerStatus { +func (m *ExecutionClientManager) CheckStatus(ctx context.Context, checkChainIDs bool) *apitypes.ClientManagerStatus { status := &apitypes.ClientManagerStatus{ FallbackEnabled: m.fallbackEc != nil, } // Get the primary EC status - status.PrimaryClientStatus = checkEcStatus(ctx, m.primaryEc) - - // Flag if primary client is ready - m.primaryReady = (status.PrimaryClientStatus.IsWorking && status.PrimaryClientStatus.IsSynced) + status.PrimaryClientStatus = checkEcStatus(ctx, m.primaryEc, checkChainIDs) + + // Check if primary is using the expected network + if checkChainIDs && status.PrimaryClientStatus.Error == "" && status.PrimaryClientStatus.ChainId != m.expectedChainID { + m.primaryReady = false + status.PrimaryClientStatus.Error = fmt.Sprintf("The primary client is using a different chain (%d) than what your node is configured for (%d)", status.PrimaryClientStatus.ChainId, m.expectedChainID) + } else { + // Flag if primary client is ready + m.primaryReady = (status.PrimaryClientStatus.IsWorking && status.PrimaryClientStatus.IsSynced) + } // Get the fallback EC status if applicable if status.FallbackEnabled { - status.FallbackClientStatus = checkEcStatus(ctx, m.fallbackEc) + status.FallbackClientStatus = checkEcStatus(ctx, m.fallbackEc, checkChainIDs) // Check if fallback is using the expected network - if status.FallbackClientStatus.Error == "" && status.FallbackClientStatus.ChainId != m.expectedChainID { + if checkChainIDs && status.FallbackClientStatus.Error == "" && status.FallbackClientStatus.ChainId != m.expectedChainID { m.fallbackReady = false - colorReset := "\033[0m" - colorYellow := "\033[33m" - status.FallbackClientStatus.Error = fmt.Sprintf("The fallback client is using a different chain [%s%s%s, Chain ID %d] than what your node is configured for [%s, Chain ID %d]", colorYellow, getNetworkNameFromId(status.FallbackClientStatus.ChainId), colorReset, status.FallbackClientStatus.ChainId, getNetworkNameFromId(m.expectedChainID), m.expectedChainID) + status.FallbackClientStatus.Error = fmt.Sprintf("The fallback client is using a different chain (%d) than what your node is configured for (%d)", status.FallbackClientStatus.ChainId, m.expectedChainID) return status } } @@ -288,35 +292,26 @@ func (m *ExecutionClientManager) CheckStatus(ctx context.Context) *apitypes.Clie return status } -func getNetworkNameFromId(networkId uint) string { - switch networkId { - case 1: - return "Ethereum Mainnet" - case 17000: - return "Holesky Testnet" - default: - return "Unknown Network" - } -} - // Check the client status -func checkEcStatus(ctx context.Context, client *ethclient.Client) apitypes.ClientStatus { +func checkEcStatus(ctx context.Context, client *ethclient.Client, checkChainIDs bool) apitypes.ClientStatus { status := apitypes.ClientStatus{} - // Get the Chain ID - chainId, err := client.ChainID(ctx) - if err != nil { - status.Error = fmt.Sprintf("Sync progress check failed with [%s]", err.Error()) - status.IsSynced = false - status.IsWorking = false - return status - } + if checkChainIDs { + // Get the Chain ID + chainId, err := client.ChainID(ctx) + if err != nil { + status.Error = fmt.Sprintf("Chain ID check failed with [%s]", err.Error()) + status.IsSynced = false + status.IsWorking = false + return status + } - if chainId != nil { - status.ChainId = uint(chainId.Uint64()) + if chainId != nil { + status.ChainId = uint(chainId.Uint64()) + } } - // Get the fallback's sync progress + // Get the client's sync progress progress, err := client.SyncProgress(ctx) if err != nil { status.Error = fmt.Sprintf("Sync progress check failed with [%s]", err.Error()) diff --git a/node/services/service-provider.go b/node/services/service-provider.go index 9275562..0e304e4 100644 --- a/node/services/service-provider.go +++ b/node/services/service-provider.go @@ -73,7 +73,7 @@ func NewServiceProvider(cfg config.IConfig, clientTimeout time.Duration) (*Servi // Beacon manager primaryBnUrl, fallbackBnUrl := cfg.GetBeaconNodeUrls() - bcManager, err := NewBeaconClientManager(primaryBnUrl, fallbackBnUrl, clientTimeout) + bcManager, err := NewBeaconClientManager(primaryBnUrl, fallbackBnUrl, resources.ChainID, clientTimeout) if err != nil { return nil, fmt.Errorf("error creating Beacon client manager: %w", err) } From dd88806cef891a20b31c046b30650ca32775b6f4 Mon Sep 17 00:00:00 2001 From: Joe Clapis Date: Wed, 15 May 2024 11:37:51 -0400 Subject: [PATCH 9/9] Fixed a linter issue with committee pooling --- beacon/client/committees.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/beacon/client/committees.go b/beacon/client/committees.go index 3a22c40..e065c5c 100644 --- a/beacon/client/committees.go +++ b/beacon/client/committees.go @@ -18,16 +18,17 @@ type Committee struct { // substantially. var validatorSlicePool sync.Pool = sync.Pool{ New: func() any { - return make([]string, 0, 1024) + buffer := make([]string, 0, 1024) + return &buffer }, } func (c *Committee) UnmarshalJSON(body []byte) error { var committee map[string]*json.RawMessage - pooledSlice := validatorSlicePool.Get().([]string) + pooledSlice := validatorSlicePool.Get().(*[]string) - c.Validators = pooledSlice + c.Validators = *pooledSlice // Partially parse the json if err := json.Unmarshal(body, &committee); err != nil { @@ -70,6 +71,6 @@ func (c *CommitteesResponse) Release() { // Reset the slice length to 0 (capacity stays the same) committee.Validators = committee.Validators[:0] // Return the slice for reuse - validatorSlicePool.Put(committee.Validators) + validatorSlicePool.Put(&committee.Validators) } }