fix: offline error message when node is shutting down (#5797)

* Improve the error handling for execution error

* Add transition unit tests cases

* Fix fetch error usage

* Update the error test cases

* Update the comments

* Update code with feedback

* Add brief information to logging

* Fix the types

* Fix linter error

* Update variable and type names

packages/beacon-node/src/chain/blocks/verifyBlocksExecutionPayloads.ts

@@ -25,7 +25,7 @@ import {IExecutionEngine} from "../../execution/engine/interface.js";
 import {BlockError, BlockErrorCode} from "../errors/index.js";
 import {IClock} from "../../util/clock.js";
 import {BlockProcessOpts} from "../options.js";
-import {ExecutePayloadStatus} from "../../execution/engine/interface.js";
+import {ExecutionPayloadStatus} from "../../execution/engine/interface.js";
 import {IEth1ForBlockProduction} from "../../eth1/index.js";
 import {Metrics} from "../../metrics/metrics.js";
 import {ImportBlockOpts} from "./types.js";
@@ -304,13 +304,13 @@ export async function verifyBlockExecutionPayload(
   chain.metrics?.engineNotifyNewPayloadResult.inc({result: execResult.status});
 
   switch (execResult.status) {
-    case ExecutePayloadStatus.VALID: {
+    case ExecutionPayloadStatus.VALID: {
       const executionStatus: ExecutionStatus.Valid = ExecutionStatus.Valid;
       const lvhResponse = {executionStatus, latestValidExecHash: execResult.latestValidHash};
       return {executionStatus, lvhResponse, execError: null};
     }
 
-    case ExecutePayloadStatus.INVALID: {
+    case ExecutionPayloadStatus.INVALID: {
       const executionStatus: ExecutionStatus.Invalid = ExecutionStatus.Invalid;
       const lvhResponse = {
         executionStatus,
@@ -326,15 +326,15 @@ export async function verifyBlockExecutionPayload(
     }
 
     // Accepted and Syncing have the same treatment, as final validation of block is pending
-    case ExecutePayloadStatus.ACCEPTED:
-    case ExecutePayloadStatus.SYNCING: {
+    case ExecutionPayloadStatus.ACCEPTED:
+    case ExecutionPayloadStatus.SYNCING: {
       // Check if the entire segment was deemed safe or, this block specifically itself if not in
       // the safeSlotsToImportOptimistically window of current slot, then we can import else
       // we need to throw and not import his block
       if (!isOptimisticallySafe && block.message.slot + opts.safeSlotsToImportOptimistically >= currentSlot) {
         const execError = new BlockError(block, {
           code: BlockErrorCode.EXECUTION_ENGINE_ERROR,
-          execStatus: ExecutePayloadStatus.UNSAFE_OPTIMISTIC_STATUS,
+          execStatus: ExecutionPayloadStatus.UNSAFE_OPTIMISTIC_STATUS,
           errorMessage: `not safe to import ${execResult.status} payload within ${opts.safeSlotsToImportOptimistically} of currentSlot`,
         });
         return {executionStatus: null, execError} as VerifyBlockExecutionResponse;
@@ -360,9 +360,9 @@ export async function verifyBlockExecutionPayload(
     // back. But for now, lets assume other mechanisms like unknown parent block of a future
     // child block will cause it to replay
 
-    case ExecutePayloadStatus.INVALID_BLOCK_HASH:
-    case ExecutePayloadStatus.ELERROR:
-    case ExecutePayloadStatus.UNAVAILABLE: {
+    case ExecutionPayloadStatus.INVALID_BLOCK_HASH:
+    case ExecutionPayloadStatus.ELERROR:
+    case ExecutionPayloadStatus.UNAVAILABLE: {
       const execError = new BlockError(block, {
         code: BlockErrorCode.EXECUTION_ENGINE_ERROR,
         execStatus: execResult.status,

packages/beacon-node/src/chain/errors/blockError.ts

@@ -2,7 +2,7 @@ import {toHexString} from "@chainsafe/ssz";
 import {allForks, RootHex, Slot, ValidatorIndex} from "@lodestar/types";
 import {LodestarError} from "@lodestar/utils";
 import {CachedBeaconStateAllForks} from "@lodestar/state-transition";
-import {ExecutePayloadStatus} from "../../execution/engine/interface.js";
+import {ExecutionPayloadStatus} from "../../execution/engine/interface.js";
 import {QueueErrorCode} from "../../util/queue/index.js";
 import {GossipActionError} from "./gossipValidation.js";
 
@@ -66,8 +66,8 @@ export enum BlockErrorCode {
 }
 
 type ExecutionErrorStatus = Exclude<
-  ExecutePayloadStatus,
-  ExecutePayloadStatus.VALID | ExecutePayloadStatus.ACCEPTED | ExecutePayloadStatus.SYNCING
+  ExecutionPayloadStatus,
+  ExecutionPayloadStatus.VALID | ExecutionPayloadStatus.ACCEPTED | ExecutionPayloadStatus.SYNCING
 >;
 
 export type BlockErrorType =

packages/beacon-node/src/execution/engine/http.ts

@@ -1,16 +1,19 @@
 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 {
+  ErrorJsonRpcResponse,
+  HttpRpcError,
+  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";
 import {
-  ExecutePayloadStatus,
+  ExecutionPayloadStatus,
   ExecutePayloadResponse,
   IExecutionEngine,
   PayloadId,
@@ -131,9 +134,12 @@ export class ExecutionEngineHttp implements IExecutionEngine {
       this.updateEngineState(ExecutionEngineState.ONLINE);
       return res;
     } catch (err) {
-      if (!isErrorAborted(err)) {
-        this.updateEngineState(getExecutionEngineState({payloadError: err}));
-      }
+      this.updateEngineState(getExecutionEngineState({payloadError: err, oldState: this.state}));
+
+      /*
+       * TODO: For some error cases as abort, we may not want to escalate the error to the caller
+       * But for now the higher level code handles such cases so we can just rethrow the error
+       */
       throw err;
     }
   }
@@ -207,39 +213,34 @@ 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}));
+    ).catch((e: Error) => {
+      if (e instanceof HttpRpcError || e instanceof ErrorJsonRpcResponse) {
+        return {status: ExecutionPayloadStatus.ELERROR, latestValidHash: null, validationError: e.message};
+      } else {
+        return {status: ExecutionPayloadStatus.UNAVAILABLE, latestValidHash: null, validationError: e.message};
+      }
+    });
+
+    this.updateEngineState(getExecutionEngineState({payloadStatus: status, oldState: this.state}));
 
     switch (status) {
-      case ExecutePayloadStatus.VALID:
+      case ExecutionPayloadStatus.VALID:
         return {status, latestValidHash: latestValidHash ?? "0x0", validationError: null};
 
-      case ExecutePayloadStatus.INVALID:
+      case ExecutionPayloadStatus.INVALID:
         // As per latest specs if latestValidHash can be null and it would mean only
         // invalidate this block
         return {status, latestValidHash, validationError};
 
-      case ExecutePayloadStatus.SYNCING:
-      case ExecutePayloadStatus.ACCEPTED:
+      case ExecutionPayloadStatus.SYNCING:
+      case ExecutionPayloadStatus.ACCEPTED:
         return {status, latestValidHash: null, validationError: null};
 
-      case ExecutePayloadStatus.INVALID_BLOCK_HASH:
+      case ExecutionPayloadStatus.INVALID_BLOCK_HASH:
         return {status, latestValidHash: null, validationError: validationError ?? "Malformed block"};
 
-      case ExecutePayloadStatus.UNAVAILABLE:
-      case ExecutePayloadStatus.ELERROR:
+      case ExecutionPayloadStatus.UNAVAILABLE:
+      case ExecutionPayloadStatus.ELERROR:
         return {
           status,
           latestValidHash: null,
@@ -248,7 +249,7 @@ export class ExecutionEngineHttp implements IExecutionEngine {
 
       default:
         return {
-          status: ExecutePayloadStatus.ELERROR,
+          status: ExecutionPayloadStatus.ELERROR,
           latestValidHash: null,
           validationError: `Invalid EL status on executePayload: ${status}`,
         };
@@ -312,25 +313,15 @@ 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:
+      case ExecutionPayloadStatus.VALID:
         // if payloadAttributes are provided, a valid payloadId is expected
         if (payloadAttributesRpc) {
           if (!payloadId || payloadId === "0x") {
@@ -342,15 +333,15 @@ export class ExecutionEngineHttp implements IExecutionEngine {
         }
         return payloadId !== "0x" ? payloadId : null;
 
-      case ExecutePayloadStatus.SYNCING:
+      case ExecutionPayloadStatus.SYNCING:
         // Throw error on syncing if requested to produce a block, else silently ignore
         if (payloadAttributes) {
           throw Error("Execution Layer Syncing");
         } else {
           return null;
         }
 
-      case ExecutePayloadStatus.INVALID:
+      case ExecutionPayloadStatus.INVALID:
         throw Error(
           `Invalid ${payloadAttributes ? "prepare payload" : "forkchoice request"}, validationError=${
             validationError ?? ""
@@ -430,29 +421,21 @@ 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");
+        this.logger.info("Execution client became online", {oldState, newState});
         break;
       case ExecutionEngineState.OFFLINE:
-        this.logger.error("Execution client went offline");
+        this.logger.error("Execution client went offline", {oldState, newState});
         break;
       case ExecutionEngineState.SYNCED:
-        this.logger.info("Execution client is synced");
+        this.logger.info("Execution client is synced", {oldState, newState});
         break;
       case ExecutionEngineState.SYNCING:
-        this.logger.warn("Execution client is syncing");
+        this.logger.warn("Execution client is syncing", {oldState, newState});
         break;
       case ExecutionEngineState.AUTH_FAILED:
-        this.logger.error("Execution client authentication failed");
+        this.logger.error("Execution client authentication failed", {oldState, newState});
         break;
     }
 

packages/beacon-node/src/execution/engine/interface.ts

@@ -8,7 +8,7 @@ import {ExecutionPayloadBody} from "./types.js";
 
 export {PayloadIdCache, PayloadId, WithdrawalV1};
 
-export enum ExecutePayloadStatus {
+export enum ExecutionPayloadStatus {
   /** given payload is valid */
   VALID = "VALID",
   /** given payload is invalid */
@@ -39,19 +39,26 @@ export enum ExecutionEngineState {
 }
 
 export type ExecutePayloadResponse =
-  | {status: ExecutePayloadStatus.SYNCING | ExecutePayloadStatus.ACCEPTED; latestValidHash: null; validationError: null}
-  | {status: ExecutePayloadStatus.VALID; latestValidHash: RootHex; validationError: null}
-  | {status: ExecutePayloadStatus.INVALID; latestValidHash: RootHex | null; validationError: string | null}
   | {
-      status: ExecutePayloadStatus.INVALID_BLOCK_HASH | ExecutePayloadStatus.ELERROR | ExecutePayloadStatus.UNAVAILABLE;
+      status: ExecutionPayloadStatus.SYNCING | ExecutionPayloadStatus.ACCEPTED;
+      latestValidHash: null;
+      validationError: null;
+    }
+  | {status: ExecutionPayloadStatus.VALID; latestValidHash: RootHex; validationError: null}
+  | {status: ExecutionPayloadStatus.INVALID; latestValidHash: RootHex | null; validationError: string | null}
+  | {
+      status:
+        | ExecutionPayloadStatus.INVALID_BLOCK_HASH
+        | ExecutionPayloadStatus.ELERROR
+        | ExecutionPayloadStatus.UNAVAILABLE;
       latestValidHash: null;
       validationError: string;
     };
 
 export type ForkChoiceUpdateStatus =
-  | ExecutePayloadStatus.VALID
-  | ExecutePayloadStatus.INVALID
-  | ExecutePayloadStatus.SYNCING;
+  | ExecutionPayloadStatus.VALID
+  | ExecutionPayloadStatus.INVALID
+  | ExecutionPayloadStatus.SYNCING;
 
 export type PayloadAttributes = {
   timestamp: number;

packages/beacon-node/src/execution/engine/mock.ts

@@ -24,7 +24,7 @@ import {
   BlobsBundleRpc,
   ExecutionPayloadBodyRpc,
 } from "./types.js";
-import {ExecutePayloadStatus, PayloadIdCache} from "./interface.js";
+import {ExecutionPayloadStatus, PayloadIdCache} from "./interface.js";
 import {JsonRpcBackend} from "./utils.js";
 
 const INTEROP_GAS_LIMIT = 30e6;
@@ -171,7 +171,7 @@ export class ExecutionEngineMockBackend implements JsonRpcBackend {
     if (!this.validBlocks.has(parentHash)) {
       // if requisite data for the payload's acceptance or validation is missing
       // return {status: SYNCING, latestValidHash: null, validationError: null}
-      return {status: ExecutePayloadStatus.SYNCING, latestValidHash: null, validationError: null};
+      return {status: ExecutionPayloadStatus.SYNCING, latestValidHash: null, validationError: null};
     }
 
     // 4. Client software MAY NOT validate the payload if the payload doesn't belong to the canonical chain.
@@ -190,7 +190,7 @@ export class ExecutionEngineMockBackend implements JsonRpcBackend {
     // IF the payload has been fully validated while processing the call
     // RETURN payload status from the Payload validation process
     // If validation succeeds, the response MUST contain {status: VALID, latestValidHash: payload.blockHash}
-    return {status: ExecutePayloadStatus.VALID, latestValidHash: blockHash, validationError: null};
+    return {status: ExecutionPayloadStatus.VALID, latestValidHash: blockHash, validationError: null};
   }
 
   /**
@@ -246,7 +246,7 @@ export class ExecutionEngineMockBackend implements JsonRpcBackend {
       //
       // > TODO: Implement
       return {
-        payloadStatus: {status: ExecutePayloadStatus.SYNCING, latestValidHash: null, validationError: null},
+        payloadStatus: {status: ExecutionPayloadStatus.SYNCING, latestValidHash: null, validationError: null},
         payloadId: null,
       };
     }
@@ -337,7 +337,7 @@ export class ExecutionEngineMockBackend implements JsonRpcBackend {
       // IF the payload is deemed VALID and the build process has begun
       // {payloadStatus: {status: VALID, latestValidHash: forkchoiceState.headBlockHash, validationError: null}, payloadId: buildProcessId}
       return {
-        payloadStatus: {status: ExecutePayloadStatus.VALID, latestValidHash: null, validationError: null},
+        payloadStatus: {status: ExecutionPayloadStatus.VALID, latestValidHash: null, validationError: null},
         payloadId: String(payloadId as number),
       };
     }
@@ -347,7 +347,7 @@ export class ExecutionEngineMockBackend implements JsonRpcBackend {
       // IF the payload is deemed VALID and a build process hasn't been started
       // {payloadStatus: {status: VALID, latestValidHash: forkchoiceState.headBlockHash, validationError: null}, payloadId: null}
       return {
-        payloadStatus: {status: ExecutePayloadStatus.VALID, latestValidHash: null, validationError: null},
+        payloadStatus: {status: ExecutionPayloadStatus.VALID, latestValidHash: null, validationError: null},
         payloadId: null,
       };
     }

packages/beacon-node/src/execution/engine/types.ts

@@ -16,7 +16,7 @@ import {
   QUANTITY,
   quantityToBigint,
 } from "../../eth1/provider/utils.js";
-import {ExecutePayloadStatus, BlobsBundle, PayloadAttributes, VersionedHashes} from "./interface.js";
+import {ExecutionPayloadStatus, BlobsBundle, PayloadAttributes, VersionedHashes} from "./interface.js";
 import {WithdrawalV1} from "./payloadIdCache.js";
 
 /* eslint-disable @typescript-eslint/naming-convention */
@@ -65,7 +65,7 @@ export type EngineApiRpcParamTypes = {
 };
 
 export type PayloadStatus = {
-  status: ExecutePayloadStatus;
+  status: ExecutionPayloadStatus;
   latestValidHash: DATA | null;
   validationError: string | null;
 };