Improve the error handling for execution error

ChainSafe · Jul 24, 2023 · ade32b2 · ade32b2
1 parent 7f19831
commit ade32b2
Show file tree

Hide file tree

Showing 2 changed files with 73 additions and 51 deletions.
diff --git a/packages/beacon-node/src/execution/engine/http.ts b/packages/beacon-node/src/execution/engine/http.ts
@@ -1,11 +1,9 @@
 import {Root, RootHex, allForks, Wei} from "@lodestar/types";
 import {SLOTS_PER_EPOCH, ForkName, ForkSeq} from "@lodestar/params";
 import {Logger} from "@lodestar/logger";
-import {isErrorAborted} from "@lodestar/utils";
-import {ErrorJsonRpcResponse, HttpRpcError} from "../../eth1/provider/jsonRpcHttpClient.js";
 import {IJsonRpcHttpClient, ReqOpts} from "../../eth1/provider/jsonRpcHttpClient.js";
 import {Metrics} from "../../metrics/index.js";
-import {JobItemQueue, isQueueErrorAborted} from "../../util/queue/index.js";
+import {JobItemQueue} from "../../util/queue/index.js";
 import {EPOCHS_PER_BATCH} from "../../sync/constants.js";
 import {numToQuantity} from "../../eth1/provider/utils.js";
 import {IJson, RpcPayload} from "../../eth1/interface.js";
@@ -131,9 +129,13 @@ export class ExecutionEngineHttp implements IExecutionEngine {
       this.updateEngineState(ExecutionEngineState.ONLINE);
       return res;
     } catch (err) {
-      if (!isErrorAborted(err)) {
-        this.updateEngineState(getExecutionEngineState({payloadError: err}));
+      const newState = getExecutionEngineState({payloadError: err, oldState: this.state});
+      if (newState !== undefined) {
+        this.updateEngineState(newState);
       }
+
+      // TODO: This is case where we can't determine the nature of error.
+      // We should throw only for such cases not for the which we knew the status
       throw err;
     }
   }
@@ -205,22 +207,11 @@ export class ExecutionEngineHttp implements IExecutionEngine {
       };
     }
 
-    const {status, latestValidHash, validationError} = await (
-      this.rpcFetchQueue.push(engineRequest) as Promise<EngineApiRpcReturnTypes[typeof method]>
-    )
-      // If there are errors by EL like connection refused, internal error, they need to be
-      // treated separate from being INVALID. For now, just pass the error upstream.
-      .catch((e: Error): EngineApiRpcReturnTypes[typeof method] => {
-        if (!isErrorAborted(e) && !isQueueErrorAborted(e)) {
-          this.updateEngineState(getExecutionEngineState({payloadError: e}));
-        }
-        if (e instanceof HttpRpcError || e instanceof ErrorJsonRpcResponse) {
-          return {status: ExecutePayloadStatus.ELERROR, latestValidHash: null, validationError: e.message};
-        } else {
-          return {status: ExecutePayloadStatus.UNAVAILABLE, latestValidHash: null, validationError: e.message};
-        }
-      });
-    this.updateEngineState(getExecutionEngineState({payloadStatus: status}));
+    const {status, latestValidHash, validationError} = await (this.rpcFetchQueue.push(engineRequest) as Promise<
+      EngineApiRpcReturnTypes[typeof method]
+    >);
+
+    this.updateEngineState(getExecutionEngineState({payloadStatus: status, oldState: this.state}));
 
     switch (status) {
       case ExecutePayloadStatus.VALID:
@@ -312,22 +303,12 @@ export class ExecutionEngineHttp implements IExecutionEngine {
       methodOpts: fcUReqOpts,
     }) as Promise<EngineApiRpcReturnTypes[typeof method]>;
 
-    const response = await request
-      // If there are errors by EL like connection refused, internal error, they need to be
-      // treated separate from being INVALID. For now, just pass the error upstream.
-      .catch((e: Error): EngineApiRpcReturnTypes[typeof method] => {
-        if (!isErrorAborted(e) && !isQueueErrorAborted(e)) {
-          this.updateEngineState(getExecutionEngineState({payloadError: e}));
-        }
-        throw e;
-      });
-
     const {
       payloadStatus: {status, latestValidHash: _latestValidHash, validationError},
       payloadId,
-    } = response;
+    } = await request;
 
-    this.updateEngineState(getExecutionEngineState({payloadStatus: status}));
+    this.updateEngineState(getExecutionEngineState({payloadStatus: status, oldState: this.state}));
 
     switch (status) {
       case ExecutePayloadStatus.VALID:
@@ -430,14 +411,6 @@ export class ExecutionEngineHttp implements IExecutionEngine {
 
     if (oldState === newState) return;
 
-    // The ONLINE is initial state and can reached from offline or auth failed error
-    if (
-      newState === ExecutionEngineState.ONLINE &&
-      !(oldState === ExecutionEngineState.OFFLINE || oldState === ExecutionEngineState.AUTH_FAILED)
-    ) {
-      return;
-    }
-
     switch (newState) {
       case ExecutionEngineState.ONLINE:
         this.logger.info("Execution client became online");

diff --git a/packages/beacon-node/src/execution/engine/utils.ts b/packages/beacon-node/src/execution/engine/utils.ts
@@ -1,5 +1,12 @@
+import {isErrorAborted} from "@lodestar/utils";
 import {IJson, RpcPayload} from "../../eth1/interface.js";
-import {IJsonRpcHttpClient, isFetchError} from "../../eth1/provider/jsonRpcHttpClient.js";
+import {
+  ErrorJsonRpcResponse,
+  HttpRpcError,
+  IJsonRpcHttpClient,
+  isFetchError,
+} from "../../eth1/provider/jsonRpcHttpClient.js";
+import {isQueueErrorAborted} from "../../util/queue/errors.js";
 import {ExecutePayloadStatus, ExecutionEngineState} from "./interface.js";
 
 export type JsonRpcBackend = {
@@ -32,12 +39,7 @@ export class ExecutionEngineMockJsonRpcClient implements IJsonRpcHttpClient {
 const fatalErrorCodes = ["ECONNREFUSED", "ENOTFOUND", "EAI_AGAIN"];
 const connectionErrorCodes = ["ECONNRESET", "ECONNABORTED"];
 
-export function getExecutionEngineState({
-  payloadError,
-  payloadStatus,
-}:
-  | {payloadStatus: ExecutePayloadStatus; payloadError?: never}
-  | {payloadStatus?: never; payloadError: unknown}): ExecutionEngineState {
+function getExecutionEngineStateForPayloadStatus(payloadStatus: ExecutePayloadStatus): ExecutionEngineState {
   switch (payloadStatus) {
     case ExecutePayloadStatus.ACCEPTED:
     case ExecutePayloadStatus.VALID:
@@ -52,6 +54,25 @@ export function getExecutionEngineState({
 
     case ExecutePayloadStatus.UNAVAILABLE:
       return ExecutionEngineState.OFFLINE;
+
+    // In case we can't determine the state, we assume it's online
+    // This assumption is better than considering offline, because the offline state may trigger some notifications
+    default:
+      return ExecutionEngineState.ONLINE;
+  }
+}
+
+function getExecutionEngineStateForPayloadError(
+  payloadError: unknown,
+  oldState: ExecutionEngineState
+): ExecutionEngineState | undefined {
+  if (isErrorAborted(payloadError) || isQueueErrorAborted(payloadError)) {
+    return oldState;
+  }
+
+  // Originally this case was handled with {status: ExecutePayloadStatus.ELERROR}
+  if (payloadError instanceof HttpRpcError || payloadError instanceof ErrorJsonRpcResponse) {
+    return ExecutionEngineState.SYNCING;
   }
 
   if (payloadError && isFetchError(payloadError) && fatalErrorCodes.includes(payloadError.code)) {
@@ -62,7 +83,35 @@ export function getExecutionEngineState({
     return ExecutionEngineState.AUTH_FAILED;
   }
 
-  // In case we can't determine the state, we assume it's online
-  // This assumption is better than considering offline, because the offline state may trigger some notifications
-  return ExecutionEngineState.ONLINE;
+  return undefined;
+}
+
+export function getExecutionEngineState<
+  S extends ExecutePayloadStatus | undefined,
+  E extends unknown | undefined,
+  R = S extends undefined ? ExecutionEngineState | undefined : ExecutionEngineState,
+>({
+  payloadError,
+  payloadStatus,
+  oldState,
+}:
+  | {payloadStatus: S; payloadError?: never; oldState: ExecutionEngineState}
+  | {payloadStatus?: never; payloadError: E; oldState: ExecutionEngineState}): R {
+  const newState =
+    payloadStatus === undefined
+      ? getExecutionEngineStateForPayloadError(payloadError, oldState)
+      : getExecutionEngineStateForPayloadStatus(payloadStatus);
+
+  // The `payloadError` is something based on which we can't determine the state of execution engine
+  if (newState === undefined) return undefined as R;
+
+  // The ONLINE is initial state and can reached from offline or auth failed error
+  if (
+    newState === ExecutionEngineState.ONLINE &&
+    !(oldState === ExecutionEngineState.OFFLINE || oldState === ExecutionEngineState.AUTH_FAILED)
+  ) {
+    return oldState as R;
+  }
+
+  return newState as R;
 }