From ade32b258ba9d08fedb1ead67975b85dde57b316 Mon Sep 17 00:00:00 2001 From: Nazar Hussain Date: Mon, 24 Jul 2023 15:48:32 +0200 Subject: [PATCH] Improve the error handling for execution error --- .../beacon-node/src/execution/engine/http.ts | 55 ++++----------- .../beacon-node/src/execution/engine/utils.ts | 69 ++++++++++++++++--- 2 files changed, 73 insertions(+), 51 deletions(-) diff --git a/packages/beacon-node/src/execution/engine/http.ts b/packages/beacon-node/src/execution/engine/http.ts index ea89506c2fc4..b7c9e0a8d059 100644 --- 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 - ) - // 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; - 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 index 1566c1d59984..720abf4c456a 100644 --- 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; }