From c0aa75280c5bf7f3fd1a8116f2bfc98b3ffc413a Mon Sep 17 00:00:00 2001 From: Nazar Hussain Date: Mon, 24 Jul 2023 18:54:25 +0200 Subject: [PATCH 1/5] fix: contract eth_call bug and made some improvements (#5785) * Fix some regressions for prover * Fix package export assertion for nested conditional exports * Improve the exports for the package * Fix the http agent for https * Fix e2e tests --- packages/prover/package.json | 6 ++- packages/prover/src/browser/index.ts | 3 ++ packages/prover/src/constants.ts | 2 +- packages/prover/src/index.ts | 2 + packages/prover/src/index.web.ts | 2 - packages/prover/src/interfaces.ts | 1 + packages/prover/src/proof_provider/index.ts | 1 + packages/prover/src/utils/evm.ts | 15 +++++- packages/prover/src/utils/json_rpc.ts | 16 +++++-- packages/prover/src/utils/rpc.ts | 2 +- .../prover/src/verified_requests/eth_call.ts | 8 +++- .../src/verified_requests/eth_estimateGas.ts | 8 +++- .../src/verified_requests/eth_getBalance.ts | 8 +++- .../verified_requests/eth_getBlockByHash.ts | 8 +++- .../verified_requests/eth_getBlockByNumber.ts | 11 ++++- .../src/verified_requests/eth_getCode.ts | 13 +++-- .../eth_getTransactionCount.ts | 11 ++++- packages/prover/src/web3_proxy.ts | 3 +- .../test/e2e/web3_batch_request.test.ts | 3 +- packages/prover/test/mocks/request_handler.ts | 1 + .../unit/verified_requests/eth_call.test.ts | 5 +- .../verified_requests/eth_estimateGas.test.ts | 5 +- .../verified_requests/eth_getBalance.test.ts | 5 +- .../eth_getBlockByHash.test.ts | 9 ++-- .../eth_getBlockByNumber.test.ts | 18 +++++-- .../verified_requests/eth_getCode.test.ts | 5 +- .../eth_getTransactionCount.test.ts | 8 +++- .../prover/test/unit/web3_provider.test.ts | 4 +- scripts/assert_exports.mjs | 47 ++++++++++--------- 29 files changed, 158 insertions(+), 72 deletions(-) create mode 100644 packages/prover/src/browser/index.ts delete mode 100644 packages/prover/src/index.web.ts create mode 100644 packages/prover/src/proof_provider/index.ts diff --git a/packages/prover/package.json b/packages/prover/package.json index 171d0853da66..88d0d7d8a317 100644 --- a/packages/prover/package.json +++ b/packages/prover/package.json @@ -15,8 +15,10 @@ "type": "module", "exports": { ".": { - "import": "./lib/index.js", - "browser": "./lib/index.web.js" + "import": "./lib/index.js" + }, + "./browser": { + "import": "./lib/browser/index.js" } }, "bin": { diff --git a/packages/prover/src/browser/index.ts b/packages/prover/src/browser/index.ts new file mode 100644 index 000000000000..6bce2e428088 --- /dev/null +++ b/packages/prover/src/browser/index.ts @@ -0,0 +1,3 @@ +export * from "../interfaces.js"; +export * from "../proof_provider/index.js"; +export {createVerifiedExecutionProvider} from "../web3_provider.js"; diff --git a/packages/prover/src/constants.ts b/packages/prover/src/constants.ts index d9e4b5dbfa43..5a9eefd0f3ca 100644 --- a/packages/prover/src/constants.ts +++ b/packages/prover/src/constants.ts @@ -1,6 +1,6 @@ // https://github.com/ethereum/consensus-specs/blob/dev/specs/altair/light-client/p2p-interface.md#configuration export const MAX_REQUEST_LIGHT_CLIENT_UPDATES = 128; export const MAX_PAYLOAD_HISTORY = 32; -export const UNVERIFIED_RESPONSE_CODE = -33091; +export const VERIFICATION_FAILED_RESPONSE_CODE = -33091; export const ZERO_ADDRESS = "0x0000000000000000000000000000000000000000"; export const DEFAULT_PROXY_REQUEST_TIMEOUT = 3000; diff --git a/packages/prover/src/index.ts b/packages/prover/src/index.ts index 59390455dc2e..daf91784cecc 100644 --- a/packages/prover/src/index.ts +++ b/packages/prover/src/index.ts @@ -1,3 +1,5 @@ export * from "./interfaces.js"; +export * from "./proof_provider/index.js"; export {createVerifiedExecutionProvider} from "./web3_provider.js"; export {createVerifiedExecutionProxy} from "./web3_proxy.js"; +export {isVerificationFailedError} from "./utils/json_rpc.js"; diff --git a/packages/prover/src/index.web.ts b/packages/prover/src/index.web.ts deleted file mode 100644 index bdde6e5542d3..000000000000 --- a/packages/prover/src/index.web.ts +++ /dev/null @@ -1,2 +0,0 @@ -export * from "./interfaces.js"; -export {createVerifiedExecutionProvider} from "./web3_provider.js"; diff --git a/packages/prover/src/interfaces.ts b/packages/prover/src/interfaces.ts index 6d00a7c47e6d..30558f149596 100644 --- a/packages/prover/src/interfaces.ts +++ b/packages/prover/src/interfaces.ts @@ -5,6 +5,7 @@ import {ProofProvider} from "./proof_provider/proof_provider.js"; import {JsonRpcRequest, JsonRpcRequestOrBatch, JsonRpcResponse, JsonRpcResponseOrBatch} from "./types.js"; import {ELRpc} from "./utils/rpc.js"; +export {NetworkName} from "@lodestar/config/networks"; export enum LCTransport { Rest = "Rest", P2P = "P2P", diff --git a/packages/prover/src/proof_provider/index.ts b/packages/prover/src/proof_provider/index.ts new file mode 100644 index 000000000000..d64938f13e6e --- /dev/null +++ b/packages/prover/src/proof_provider/index.ts @@ -0,0 +1 @@ +export * from "./proof_provider.js"; diff --git a/packages/prover/src/utils/evm.ts b/packages/prover/src/utils/evm.ts index 7d602a4520bf..eb0296752ffe 100644 --- a/packages/prover/src/utils/evm.ts +++ b/packages/prover/src/utils/evm.ts @@ -84,8 +84,19 @@ export async function getVMWithState({ const batchRequests = []; for (const [address, storageKeys] of Object.entries(storageKeysMap)) { - batchRequests.push({jsonrpc: "2.0", method: "eth_getProof", params: [address, storageKeys, blockHashHex]}); - batchRequests.push({jsonrpc: "2.0", method: "eth_getCode", params: [address, blockHashHex]}); + batchRequests.push({ + jsonrpc: "2.0", + id: rpc.getRequestId(), + method: "eth_getProof", + params: [address, storageKeys, blockHashHex], + }); + + batchRequests.push({ + jsonrpc: "2.0", + id: rpc.getRequestId(), + method: "eth_getCode", + params: [address, blockHashHex], + }); } // If all responses are valid then we will have even number of responses diff --git a/packages/prover/src/utils/json_rpc.ts b/packages/prover/src/utils/json_rpc.ts index cde220b597d3..74727c198122 100644 --- a/packages/prover/src/utils/json_rpc.ts +++ b/packages/prover/src/utils/json_rpc.ts @@ -1,5 +1,5 @@ import {Logger} from "@lodestar/logger"; -import {UNVERIFIED_RESPONSE_CODE} from "../constants.js"; +import {VERIFICATION_FAILED_RESPONSE_CODE} from "../constants.js"; import { JsonRpcErrorPayload, JsonRpcNotificationPayload, @@ -44,18 +44,26 @@ export function getResponseForRequest( throw new Error("Either result or error must be defined."); } -export function getErrorResponseForUnverifiedRequest( +export function getVerificationFailedMessage(method: string): string { + return `verification for '${method}' request failed.`; +} + +export function isVerificationFailedError

(payload: JsonRpcResponseWithErrorPayload

): boolean { + return !isValidResponsePayload(payload) && payload.error.code === VERIFICATION_FAILED_RESPONSE_CODE; +} + +export function getErrorResponseForRequestWithFailedVerification( payload: JsonRpcRequest

, message: string, data?: D ): JsonRpcResponseWithErrorPayload { return isNullish(data) ? (getResponseForRequest(payload, undefined, { - code: UNVERIFIED_RESPONSE_CODE, + code: VERIFICATION_FAILED_RESPONSE_CODE, message, }) as JsonRpcResponseWithErrorPayload) : (getResponseForRequest(payload, undefined, { - code: UNVERIFIED_RESPONSE_CODE, + code: VERIFICATION_FAILED_RESPONSE_CODE, message, data, }) as JsonRpcResponseWithErrorPayload); diff --git a/packages/prover/src/utils/rpc.ts b/packages/prover/src/utils/rpc.ts index 5fb8078f228c..5feee3332c4a 100644 --- a/packages/prover/src/utils/rpc.ts +++ b/packages/prover/src/utils/rpc.ts @@ -98,7 +98,7 @@ export class ELRpc { } } - private getRequestId(): string { + getRequestId(): string { // TODO: Find better way to generate random id return (Math.random() * 10000).toFixed(0); } diff --git a/packages/prover/src/verified_requests/eth_call.ts b/packages/prover/src/verified_requests/eth_call.ts index 4b0ea115245c..b28bd222c568 100644 --- a/packages/prover/src/verified_requests/eth_call.ts +++ b/packages/prover/src/verified_requests/eth_call.ts @@ -2,7 +2,11 @@ import {ELVerifiedRequestHandler} from "../interfaces.js"; import {ELApiParams, ELApiReturn} from "../types.js"; import {bufferToHex} from "../utils/conversion.js"; import {createVM, executeVMCall, getVMWithState} from "../utils/evm.js"; -import {getResponseForRequest, getErrorResponseForUnverifiedRequest} from "../utils/json_rpc.js"; +import { + getResponseForRequest, + getErrorResponseForRequestWithFailedVerification, + getVerificationFailedMessage, +} from "../utils/json_rpc.js"; // eslint-disable-next-line @typescript-eslint/naming-convention export const eth_call: ELVerifiedRequestHandler = async ({ @@ -42,6 +46,6 @@ export const eth_call: ELVerifiedRequestHandler = async ({ @@ -19,5 +23,5 @@ export const eth_getBalance: ELVerifiedRequestHandler<[address: string, block?: } logger.error("Request could not be verified.", {method: payload.method, params: JSON.stringify(payload.params)}); - return getErrorResponseForUnverifiedRequest(payload, "eth_getBalance request can not be verified."); + return getErrorResponseForRequestWithFailedVerification(payload, getVerificationFailedMessage("eth_getBalance")); }; diff --git a/packages/prover/src/verified_requests/eth_getBlockByHash.ts b/packages/prover/src/verified_requests/eth_getBlockByHash.ts index 0c27a81729ad..cb5fa1711c0f 100644 --- a/packages/prover/src/verified_requests/eth_getBlockByHash.ts +++ b/packages/prover/src/verified_requests/eth_getBlockByHash.ts @@ -1,7 +1,11 @@ import {ELVerifiedRequestHandler} from "../interfaces.js"; import {ELBlock} from "../types.js"; import {verifyBlock} from "../utils/verification.js"; -import {getErrorResponseForUnverifiedRequest, getResponseForRequest} from "../utils/json_rpc.js"; +import { + getErrorResponseForRequestWithFailedVerification, + getResponseForRequest, + getVerificationFailedMessage, +} from "../utils/json_rpc.js"; // eslint-disable-next-line @typescript-eslint/naming-convention export const eth_getBlockByHash: ELVerifiedRequestHandler<[block: string, hydrated: boolean], ELBlock> = async ({ @@ -17,5 +21,5 @@ export const eth_getBlockByHash: ELVerifiedRequestHandler<[block: string, hydrat } logger.error("Request could not be verified.", {method: payload.method, params: JSON.stringify(payload.params)}); - return getErrorResponseForUnverifiedRequest(payload, "eth_getBlockByHash request can not be verified."); + return getErrorResponseForRequestWithFailedVerification(payload, getVerificationFailedMessage("eth_getBlockByHash")); }; diff --git a/packages/prover/src/verified_requests/eth_getBlockByNumber.ts b/packages/prover/src/verified_requests/eth_getBlockByNumber.ts index 64c00410945a..a08703881cc0 100644 --- a/packages/prover/src/verified_requests/eth_getBlockByNumber.ts +++ b/packages/prover/src/verified_requests/eth_getBlockByNumber.ts @@ -1,7 +1,11 @@ import {ELVerifiedRequestHandler} from "../interfaces.js"; import {ELBlock} from "../types.js"; import {verifyBlock} from "../utils/verification.js"; -import {getErrorResponseForUnverifiedRequest, getResponseForRequest} from "../utils/json_rpc.js"; +import { + getErrorResponseForRequestWithFailedVerification, + getResponseForRequest, + getVerificationFailedMessage, +} from "../utils/json_rpc.js"; // eslint-disable-next-line @typescript-eslint/naming-convention export const eth_getBlockByNumber: ELVerifiedRequestHandler< @@ -15,5 +19,8 @@ export const eth_getBlockByNumber: ELVerifiedRequestHandler< } logger.error("Request could not be verified.", {method: payload.method, params: JSON.stringify(payload.params)}); - return getErrorResponseForUnverifiedRequest(payload, "eth_getBlockByNumber request can not be verified."); + return getErrorResponseForRequestWithFailedVerification( + payload, + getVerificationFailedMessage("eth_getBlockByNumber") + ); }; diff --git a/packages/prover/src/verified_requests/eth_getCode.ts b/packages/prover/src/verified_requests/eth_getCode.ts index 9f64ae3627f3..9cb3362c4e50 100644 --- a/packages/prover/src/verified_requests/eth_getCode.ts +++ b/packages/prover/src/verified_requests/eth_getCode.ts @@ -1,6 +1,10 @@ import {ELVerifiedRequestHandler} from "../interfaces.js"; import {verifyAccount, verifyCode} from "../utils/verification.js"; -import {getErrorResponseForUnverifiedRequest, getResponseForRequest} from "../utils/json_rpc.js"; +import { + getErrorResponseForRequestWithFailedVerification, + getResponseForRequest, + getVerificationFailedMessage, +} from "../utils/json_rpc.js"; // eslint-disable-next-line @typescript-eslint/naming-convention export const eth_getCode: ELVerifiedRequestHandler<[address: string, block?: number | string], string> = async ({ @@ -23,7 +27,10 @@ export const eth_getCode: ELVerifiedRequestHandler<[address: string, block?: num if (!accountProof.valid) { logger.error("Request could not be verified.", {method: payload.method, params: JSON.stringify(payload.params)}); - return getErrorResponseForUnverifiedRequest(payload, "account for eth_getCode request can not be verified."); + return getErrorResponseForRequestWithFailedVerification( + payload, + "account for eth_getCode request can not be verified." + ); } const codeProof = await verifyCode({ @@ -40,5 +47,5 @@ export const eth_getCode: ELVerifiedRequestHandler<[address: string, block?: num } logger.error("Request could not be verified.", {method: payload.method, params: JSON.stringify(payload.params)}); - return getErrorResponseForUnverifiedRequest(payload, "eth_getCode request can not be verified."); + return getErrorResponseForRequestWithFailedVerification(payload, getVerificationFailedMessage("eth_getCode")); }; diff --git a/packages/prover/src/verified_requests/eth_getTransactionCount.ts b/packages/prover/src/verified_requests/eth_getTransactionCount.ts index 8a2de9c78179..4cafd9b2b271 100644 --- a/packages/prover/src/verified_requests/eth_getTransactionCount.ts +++ b/packages/prover/src/verified_requests/eth_getTransactionCount.ts @@ -1,6 +1,10 @@ import {ELVerifiedRequestHandler} from "../interfaces.js"; import {verifyAccount} from "../utils/verification.js"; -import {getResponseForRequest, getErrorResponseForUnverifiedRequest} from "../utils/json_rpc.js"; +import { + getResponseForRequest, + getErrorResponseForRequestWithFailedVerification, + getVerificationFailedMessage, +} from "../utils/json_rpc.js"; // eslint-disable-next-line @typescript-eslint/naming-convention export const eth_getTransactionCount: ELVerifiedRequestHandler< @@ -17,5 +21,8 @@ export const eth_getTransactionCount: ELVerifiedRequestHandler< } logger.error("Request could not be verified.", {method: payload.method, params: JSON.stringify(payload.params)}); - return getErrorResponseForUnverifiedRequest(payload, "eth_getTransactionCount request can not be verified."); + return getErrorResponseForRequestWithFailedVerification( + payload, + getVerificationFailedMessage("eth_getTransactionCount") + ); }; diff --git a/packages/prover/src/web3_proxy.ts b/packages/prover/src/web3_proxy.ts index 508aed45b86c..6aa44b314953 100644 --- a/packages/prover/src/web3_proxy.ts +++ b/packages/prover/src/web3_proxy.ts @@ -1,4 +1,5 @@ import http from "node:http"; +import https from "node:https"; import url from "node:url"; import httpProxy from "http-proxy"; import {getNodeLogger} from "@lodestar/logger/node"; @@ -78,7 +79,7 @@ export function createVerifiedExecutionProxy(opts: VerifiedProxyOptions): { const proxy = httpProxy.createProxy({ target: executionRpcUrl, ws: executionRpcUrl.startsWith("ws"), - agent: http.globalAgent, + agent: executionRpcUrl.startsWith("https") ? https.globalAgent : http.globalAgent, xfwd: true, ignorePath: true, changeOrigin: true, diff --git a/packages/prover/test/e2e/web3_batch_request.test.ts b/packages/prover/test/e2e/web3_batch_request.test.ts index 472c04274764..f7167a8054f3 100644 --- a/packages/prover/test/e2e/web3_batch_request.test.ts +++ b/packages/prover/test/e2e/web3_batch_request.test.ts @@ -4,6 +4,7 @@ import Web3 from "web3"; import {LCTransport} from "../../src/interfaces.js"; import {createVerifiedExecutionProvider} from "../../src/web3_provider.js"; import {rpcUrl, beaconUrl, config} from "../utils/e2e_env.js"; +import {getVerificationFailedMessage} from "../../src/utils/json_rpc.js"; describe("web3_batch_requests", function () { // Give some margin to sync light client @@ -94,7 +95,7 @@ describe("web3_batch_requests", function () { batch.execute(); await expect(successRequest).to.be.fulfilled; - await expect(errorRequest).to.be.rejectedWith("eth_getBlockByHash request can not be verified"); + await expect(errorRequest).to.be.rejectedWith(getVerificationFailedMessage("eth_getBlockByHash")); }); }); }); diff --git a/packages/prover/test/mocks/request_handler.ts b/packages/prover/test/mocks/request_handler.ts index ba71954200a4..9f65eee00363 100644 --- a/packages/prover/test/mocks/request_handler.ts +++ b/packages/prover/test/mocks/request_handler.ts @@ -117,6 +117,7 @@ export function generateReqHandlerOptionsMock( rpc: { request: sinon.stub(), batchRequest: sinon.stub(), + getRequestId: () => (Math.random() * 10000).toFixed(0), }, }; diff --git a/packages/prover/test/unit/verified_requests/eth_call.test.ts b/packages/prover/test/unit/verified_requests/eth_call.test.ts index 1c60879c8591..996ce0849e8a 100644 --- a/packages/prover/test/unit/verified_requests/eth_call.test.ts +++ b/packages/prover/test/unit/verified_requests/eth_call.test.ts @@ -3,11 +3,12 @@ import deepmerge from "deepmerge"; import {createForkConfig} from "@lodestar/config"; import {NetworkName, networksChainConfig} from "@lodestar/config/networks"; import {ELTransaction} from "../../../lib/types.js"; -import {UNVERIFIED_RESPONSE_CODE} from "../../../src/constants.js"; +import {VERIFICATION_FAILED_RESPONSE_CODE} from "../../../src/constants.js"; import {eth_call} from "../../../src/verified_requests/eth_call.js"; import ethCallCase1 from "../../fixtures/mainnet/eth_call.json" assert {type: "json"}; import {generateReqHandlerOptionsMock} from "../../mocks/request_handler.js"; import {JsonRpcRequest, JsonRpcResponseWithResultPayload} from "../../../src/types.js"; +import {getVerificationFailedMessage} from "../../../src/utils/json_rpc.js"; const testCases = [ethCallCase1]; @@ -60,7 +61,7 @@ describe("verified_requests / eth_call", () => { expect(response).to.eql({ jsonrpc: "2.0", id: testCase.request.id, - error: {code: UNVERIFIED_RESPONSE_CODE, message: "eth_call request can not be verified."}, + error: {code: VERIFICATION_FAILED_RESPONSE_CODE, message: getVerificationFailedMessage("eth_call")}, }); }); }); diff --git a/packages/prover/test/unit/verified_requests/eth_estimateGas.test.ts b/packages/prover/test/unit/verified_requests/eth_estimateGas.test.ts index dd87e8e466b6..ae7df1e8b7fb 100644 --- a/packages/prover/test/unit/verified_requests/eth_estimateGas.test.ts +++ b/packages/prover/test/unit/verified_requests/eth_estimateGas.test.ts @@ -3,12 +3,13 @@ import deepmerge from "deepmerge"; import {createForkConfig} from "@lodestar/config"; import {NetworkName, networksChainConfig} from "@lodestar/config/networks"; import {ELTransaction} from "../../../lib/types.js"; -import {UNVERIFIED_RESPONSE_CODE} from "../../../src/constants.js"; +import {VERIFICATION_FAILED_RESPONSE_CODE} from "../../../src/constants.js"; import {eth_estimateGas} from "../../../src/verified_requests/eth_estimateGas.js"; import ethEstimateGasCase1 from "../../fixtures/mainnet/eth_estimateGas_simple_transfer.json" assert {type: "json"}; import ethEstimateGasCase2 from "../../fixtures/mainnet/eth_estimateGas_contract_call.json" assert {type: "json"}; import {TestFixture, generateReqHandlerOptionsMock} from "../../mocks/request_handler.js"; import {JsonRpcRequest, JsonRpcResponseWithResultPayload} from "../../../src/types.js"; +import {getVerificationFailedMessage} from "../../../src/utils/json_rpc.js"; const testCases = [ethEstimateGasCase1, ethEstimateGasCase2] as TestFixture[]; @@ -62,7 +63,7 @@ describe("verified_requests / eth_estimateGas", () => { expect(response).to.eql({ jsonrpc: "2.0", id: testCase.request.id, - error: {code: UNVERIFIED_RESPONSE_CODE, message: "eth_estimateGas request can not be verified."}, + error: {code: VERIFICATION_FAILED_RESPONSE_CODE, message: getVerificationFailedMessage("eth_estimateGas")}, }); }); }); diff --git a/packages/prover/test/unit/verified_requests/eth_getBalance.test.ts b/packages/prover/test/unit/verified_requests/eth_getBalance.test.ts index cf8dd4e85c00..46b4edf77f16 100644 --- a/packages/prover/test/unit/verified_requests/eth_getBalance.test.ts +++ b/packages/prover/test/unit/verified_requests/eth_getBalance.test.ts @@ -2,11 +2,12 @@ import {expect} from "chai"; import deepmerge from "deepmerge"; import {createForkConfig} from "@lodestar/config"; import {NetworkName, networksChainConfig} from "@lodestar/config/networks"; -import {UNVERIFIED_RESPONSE_CODE} from "../../../src/constants.js"; +import {VERIFICATION_FAILED_RESPONSE_CODE} from "../../../src/constants.js"; import {eth_getBalance} from "../../../src/verified_requests/eth_getBalance.js"; import eth_getBalance_eoa from "../../fixtures/sepolia/eth_getBalance_eoa.json" assert {type: "json"}; import eth_getBalance_contract from "../../fixtures/sepolia/eth_getBalance_contract.json" assert {type: "json"}; import {generateReqHandlerOptionsMock} from "../../mocks/request_handler.js"; +import {getVerificationFailedMessage} from "../../../src/utils/json_rpc.js"; const testCases = [eth_getBalance_eoa, eth_getBalance_contract]; @@ -46,7 +47,7 @@ describe("verified_requests / eth_getBalance", () => { expect(response).to.eql({ jsonrpc: "2.0", id: data.request.id, - error: {code: UNVERIFIED_RESPONSE_CODE, message: "eth_getBalance request can not be verified."}, + error: {code: VERIFICATION_FAILED_RESPONSE_CODE, message: getVerificationFailedMessage("eth_getBalance")}, }); }); }); diff --git a/packages/prover/test/unit/verified_requests/eth_getBlockByHash.test.ts b/packages/prover/test/unit/verified_requests/eth_getBlockByHash.test.ts index 71c584a74f1d..a55f94903216 100644 --- a/packages/prover/test/unit/verified_requests/eth_getBlockByHash.test.ts +++ b/packages/prover/test/unit/verified_requests/eth_getBlockByHash.test.ts @@ -2,12 +2,13 @@ import {expect} from "chai"; import deepmerge from "deepmerge"; import {createForkConfig} from "@lodestar/config"; import {NetworkName, networksChainConfig} from "@lodestar/config/networks"; -import {UNVERIFIED_RESPONSE_CODE} from "../../../src/constants.js"; +import {VERIFICATION_FAILED_RESPONSE_CODE} from "../../../src/constants.js"; import {eth_getBlockByHash} from "../../../src/verified_requests/eth_getBlockByHash.js"; import eth_getBlock_with_contractCreation from "../../fixtures/sepolia/eth_getBlock_with_contractCreation.json" assert {type: "json"}; import eth_getBlock_with_no_accessList from "../../fixtures/sepolia/eth_getBlock_with_no_accessList.json" assert {type: "json"}; import {TestFixture, generateReqHandlerOptionsMock} from "../../mocks/request_handler.js"; import {ELBlock} from "../../../src/types.js"; +import {getVerificationFailedMessage} from "../../../src/utils/json_rpc.js"; const testCases = [eth_getBlock_with_no_accessList, eth_getBlock_with_contractCreation] as [ TestFixture, @@ -51,7 +52,7 @@ describe("verified_requests / eth_getBlockByHash", () => { expect(response).to.eql({ jsonrpc: "2.0", id: testCase.request.id, - error: {code: UNVERIFIED_RESPONSE_CODE, message: "eth_getBlockByHash request can not be verified."}, + error: {code: VERIFICATION_FAILED_RESPONSE_CODE, message: getVerificationFailedMessage("eth_getBlockByHash")}, }); }); @@ -74,7 +75,7 @@ describe("verified_requests / eth_getBlockByHash", () => { expect(response).to.eql({ jsonrpc: "2.0", id: testCase.request.id, - error: {code: UNVERIFIED_RESPONSE_CODE, message: "eth_getBlockByHash request can not be verified."}, + error: {code: VERIFICATION_FAILED_RESPONSE_CODE, message: getVerificationFailedMessage("eth_getBlockByHash")}, }); }); @@ -97,7 +98,7 @@ describe("verified_requests / eth_getBlockByHash", () => { expect(response).to.eql({ jsonrpc: "2.0", id: testCase.request.id, - error: {code: UNVERIFIED_RESPONSE_CODE, message: "eth_getBlockByHash request can not be verified."}, + error: {code: VERIFICATION_FAILED_RESPONSE_CODE, message: getVerificationFailedMessage("eth_getBlockByHash")}, }); }); }); diff --git a/packages/prover/test/unit/verified_requests/eth_getBlockByNumber.test.ts b/packages/prover/test/unit/verified_requests/eth_getBlockByNumber.test.ts index a4f1f9d7f869..506a115eba4d 100644 --- a/packages/prover/test/unit/verified_requests/eth_getBlockByNumber.test.ts +++ b/packages/prover/test/unit/verified_requests/eth_getBlockByNumber.test.ts @@ -2,12 +2,13 @@ import {expect} from "chai"; import deepmerge from "deepmerge"; import {createForkConfig} from "@lodestar/config"; import {NetworkName, networksChainConfig} from "@lodestar/config/networks"; -import {UNVERIFIED_RESPONSE_CODE} from "../../../src/constants.js"; +import {VERIFICATION_FAILED_RESPONSE_CODE} from "../../../src/constants.js"; import {ELBlock} from "../../../src/types.js"; import {eth_getBlockByNumber} from "../../../src/verified_requests/eth_getBlockByNumber.js"; import eth_getBlock_with_contractCreation from "../../fixtures/sepolia/eth_getBlock_with_contractCreation.json" assert {type: "json"}; import eth_getBlock_with_no_accessList from "../../fixtures/sepolia/eth_getBlock_with_no_accessList.json" assert {type: "json"}; import {TestFixture, generateReqHandlerOptionsMock} from "../../mocks/request_handler.js"; +import {getVerificationFailedMessage} from "../../../src/utils/json_rpc.js"; const testCases = [eth_getBlock_with_no_accessList, eth_getBlock_with_contractCreation] as [ TestFixture, @@ -51,7 +52,10 @@ describe("verified_requests / eth_getBlockByNumber", () => { expect(response).to.eql({ jsonrpc: "2.0", id: testCase.request.id, - error: {code: UNVERIFIED_RESPONSE_CODE, message: "eth_getBlockByNumber request can not be verified."}, + error: { + code: VERIFICATION_FAILED_RESPONSE_CODE, + message: getVerificationFailedMessage("eth_getBlockByNumber"), + }, }); }); @@ -74,7 +78,10 @@ describe("verified_requests / eth_getBlockByNumber", () => { expect(response).to.eql({ jsonrpc: "2.0", id: testCase.request.id, - error: {code: UNVERIFIED_RESPONSE_CODE, message: "eth_getBlockByNumber request can not be verified."}, + error: { + code: VERIFICATION_FAILED_RESPONSE_CODE, + message: getVerificationFailedMessage("eth_getBlockByNumber"), + }, }); }); @@ -100,7 +107,10 @@ describe("verified_requests / eth_getBlockByNumber", () => { expect(response).to.eql({ jsonrpc: "2.0", id: testCase.request.id, - error: {code: UNVERIFIED_RESPONSE_CODE, message: "eth_getBlockByNumber request can not be verified."}, + error: { + code: VERIFICATION_FAILED_RESPONSE_CODE, + message: getVerificationFailedMessage("eth_getBlockByNumber"), + }, }); }); }); diff --git a/packages/prover/test/unit/verified_requests/eth_getCode.test.ts b/packages/prover/test/unit/verified_requests/eth_getCode.test.ts index 584457dff781..c88f58a1cd2b 100644 --- a/packages/prover/test/unit/verified_requests/eth_getCode.test.ts +++ b/packages/prover/test/unit/verified_requests/eth_getCode.test.ts @@ -2,10 +2,11 @@ import {expect} from "chai"; import deepmerge from "deepmerge"; import {createForkConfig} from "@lodestar/config"; import {NetworkName, networksChainConfig} from "@lodestar/config/networks"; -import {UNVERIFIED_RESPONSE_CODE} from "../../../src/constants.js"; +import {VERIFICATION_FAILED_RESPONSE_CODE} from "../../../src/constants.js"; import {eth_getCode} from "../../../src/verified_requests/eth_getCode.js"; import ethGetCodeCase1 from "../../fixtures/sepolia/eth_getCode.json" assert {type: "json"}; import {generateReqHandlerOptionsMock} from "../../mocks/request_handler.js"; +import {getVerificationFailedMessage} from "../../../src/utils/json_rpc.js"; const testCases = [ethGetCodeCase1]; @@ -44,7 +45,7 @@ describe("verified_requests / eth_getCode", () => { expect(response).to.eql({ jsonrpc: "2.0", id: testCase.request.id, - error: {code: UNVERIFIED_RESPONSE_CODE, message: "eth_getCode request can not be verified."}, + error: {code: VERIFICATION_FAILED_RESPONSE_CODE, message: getVerificationFailedMessage("eth_getCode")}, }); }); }); diff --git a/packages/prover/test/unit/verified_requests/eth_getTransactionCount.test.ts b/packages/prover/test/unit/verified_requests/eth_getTransactionCount.test.ts index b6fd3dcecaad..03d491dc5b0c 100644 --- a/packages/prover/test/unit/verified_requests/eth_getTransactionCount.test.ts +++ b/packages/prover/test/unit/verified_requests/eth_getTransactionCount.test.ts @@ -2,10 +2,11 @@ import {expect} from "chai"; import deepmerge from "deepmerge"; import {createForkConfig} from "@lodestar/config"; import {NetworkName, networksChainConfig} from "@lodestar/config/networks"; -import {UNVERIFIED_RESPONSE_CODE} from "../../../src/constants.js"; +import {VERIFICATION_FAILED_RESPONSE_CODE} from "../../../src/constants.js"; import {eth_getTransactionCount} from "../../../src/verified_requests/eth_getTransactionCount.js"; import getTransactionCountCase1 from "../../fixtures/sepolia/eth_getTransactionCount.json" assert {type: "json"}; import {generateReqHandlerOptionsMock} from "../../mocks/request_handler.js"; +import {getVerificationFailedMessage} from "../../../src/utils/json_rpc.js"; const testCases = [getTransactionCountCase1]; @@ -46,7 +47,10 @@ describe("verified_requests / eth_getTransactionCount", () => { expect(response).to.eql({ jsonrpc: "2.0", id: testCase.request.id, - error: {code: UNVERIFIED_RESPONSE_CODE, message: "eth_getTransactionCount request can not be verified."}, + error: { + code: VERIFICATION_FAILED_RESPONSE_CODE, + message: getVerificationFailedMessage("eth_getTransactionCount"), + }, }); }); }); diff --git a/packages/prover/test/unit/web3_provider.test.ts b/packages/prover/test/unit/web3_provider.test.ts index 290dfce72f3f..e29188503b96 100644 --- a/packages/prover/test/unit/web3_provider.test.ts +++ b/packages/prover/test/unit/web3_provider.test.ts @@ -2,9 +2,7 @@ import {expect} from "chai"; import Web3 from "web3"; import {ethers} from "ethers"; import sinon from "sinon"; -import {LCTransport} from "../../src/interfaces.js"; -import {ProofProvider} from "../../src/proof_provider/proof_provider.js"; -import {createVerifiedExecutionProvider} from "../../src/web3_provider.js"; +import {createVerifiedExecutionProvider, ProofProvider, LCTransport} from "@lodestar/prover/browser"; import {ELRpc} from "../../src/utils/rpc.js"; describe("web3_provider", () => { diff --git a/scripts/assert_exports.mjs b/scripts/assert_exports.mjs index 1f52c909aad5..5110fbc79658 100644 --- a/scripts/assert_exports.mjs +++ b/scripts/assert_exports.mjs @@ -5,22 +5,13 @@ import path from "node:path"; // This script ensure that the referenced files exist const pkgsDirpath = path.resolve("./packages"); -const exportPaths = []; - -for (const pkgDirname of fs.readdirSync(pkgsDirpath)) { - const pkgDirpath = path.join(pkgsDirpath, pkgDirname); - const packageJSONPath = path.join(pkgDirpath, "package.json"); - if (!fs.existsSync(packageJSONPath)) { - throw Error(`No package.json found in ${pkgDirpath}`); - } - - const packageJSON = JSON.parse(fs.readFileSync(packageJSONPath, "utf8")); +function getExportPaths(pkgDirPath, pkgExports) { // { // "exports": "./lib/index.js", // } - if (typeof packageJSON.exports === "string") { - exportPaths.push(path.join(pkgDirpath, packageJSON.exports)); + if (typeof pkgExports === "string") { + return [pkgExports]; } // { @@ -29,18 +20,30 @@ for (const pkgDirname of fs.readdirSync(pkgsDirpath)) { // "import": "./lib/index.js" // }, // } - else if (typeof packageJSON.exports === "object") { - for (const [exportPath, exportObj] of Object.entries(packageJSON.exports)) { - if (!exportObj.import) { - throw Error(`package.json ${packageJSONPath} export ${exportPath} has not import`); - } - - exportPaths.push(path.join(pkgDirpath, exportObj.import)); + const exportPaths = []; + for (const [exportPath, nestedExportObj] of Object.entries(pkgExports)) { + if (typeof nestedExportObj === "object") { + exportPaths.push(...getExportPaths(pkgDirPath, nestedExportObj)); + } else if (typeof nestedExportObj === "string") { + exportPaths.push(nestedExportObj); } } + + return exportPaths; } -const missingExportPaths = exportPaths.filter((exportPath) => !fs.existsSync(exportPath)); -if (missingExportPaths.length > 0) { - throw Error(`export paths file(s) not found\n${missingExportPaths.join("\n")}`); +for (const pkgDirname of fs.readdirSync(pkgsDirpath)) { + const pkgDirpath = path.join(pkgsDirpath, pkgDirname); + const packageJSONPath = path.join(pkgDirpath, "package.json"); + if (!fs.existsSync(packageJSONPath)) { + throw Error(`No package.json found in ${pkgDirpath}`); + } + + const packageJSON = JSON.parse(fs.readFileSync(packageJSONPath, "utf8")); + const exportPaths = getExportPaths(pkgDirpath, packageJSON.exports); + const missingExportPaths = exportPaths.filter((exportPath) => !fs.existsSync(path.join(pkgDirpath, exportPath))); + + if (missingExportPaths.length > 0) { + throw Error(`export paths file(s) not found\n${missingExportPaths.join("\n")}`); + } } From 4720a5bf75036b180bc7b904d4cf3741a2a4c098 Mon Sep 17 00:00:00 2001 From: Matthew Keil Date: Mon, 24 Jul 2023 12:55:55 -0400 Subject: [PATCH 2/5] test(db): add check for gdu vs du (#5788) tes(db)t: add check for gdu vs du --- packages/db/test/unit/controller/level.test.ts | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/packages/db/test/unit/controller/level.test.ts b/packages/db/test/unit/controller/level.test.ts index 337274b5eb53..38d8274ebb22 100644 --- a/packages/db/test/unit/controller/level.test.ts +++ b/packages/db/test/unit/controller/level.test.ts @@ -1,4 +1,5 @@ import {execSync} from "node:child_process"; +import os from "node:os"; import {expect} from "chai"; import leveldown from "leveldown"; import all from "it-all"; @@ -135,9 +136,23 @@ describe("LevelDB controller", () => { expect(approxSize).gt(0, "approximateSize return not > 0"); }); + function getDuCommand(): string { + if (os.platform() === "darwin") { + try { + const res = execSync("gdu --help", {encoding: "utf8"}); + if (res?.startsWith("Usage: gdu ")) { + return "gdu"; + } + } catch { + /* no-op */ + } + } + return "du"; + } + function getDbSize(): number { // 116 ./.__testdb - const res = execSync(`du -bs ${dbLocation}`, {encoding: "utf8"}); + const res = execSync(`${getDuCommand()} -bs ${dbLocation}`, {encoding: "utf8"}); const match = res.match(/^(\d+)/); if (!match) throw Error(`Unknown du response \n${res}`); return parseInt(match[1]); From 2f28718a17416c68d401c808fb9d737571fd2a87 Mon Sep 17 00:00:00 2001 From: Nico Flaig Date: Mon, 24 Jul 2023 18:57:15 +0200 Subject: [PATCH 3/5] refactor: update distributed aggregation selection error logs (#5780) --- packages/validator/src/services/attestation.ts | 6 ++---- packages/validator/src/services/syncCommittee.ts | 6 ++---- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/packages/validator/src/services/attestation.ts b/packages/validator/src/services/attestation.ts index b5bd851760fd..f954a9d2d0d5 100644 --- a/packages/validator/src/services/attestation.ts +++ b/packages/validator/src/services/attestation.ts @@ -322,9 +322,7 @@ export class AttestationService { this.logger.debug("Submitting partial beacon committee selection proofs", {slot, count: partialSelections.length}); const res = await Promise.race([ - this.api.validator - .submitBeaconCommitteeSelections(partialSelections) - .catch((e) => this.logger.error("Error on submitBeaconCommitteeSelections", {slot}, e)), + this.api.validator.submitBeaconCommitteeSelections(partialSelections), // Exit attestation aggregation flow if there is no response after 1/3 of slot as // beacon node would likely not have enough time to prepare an aggregate attestation. // Note that the aggregations flow is not explicitly exited but rather will be skipped @@ -334,7 +332,7 @@ export class AttestationService { ]); if (!res) { - throw new Error("submitBeaconCommitteeSelections did not resolve after 1/3 of slot"); + throw new Error("Failed to receive combined selection proofs before 1/3 of slot"); } ApiError.assert(res, "Error receiving combined selection proofs"); diff --git a/packages/validator/src/services/syncCommittee.ts b/packages/validator/src/services/syncCommittee.ts index adbda3231697..9f104e0d7d4b 100644 --- a/packages/validator/src/services/syncCommittee.ts +++ b/packages/validator/src/services/syncCommittee.ts @@ -261,9 +261,7 @@ export class SyncCommitteeService { this.logger.debug("Submitting partial sync committee selection proofs", {slot, count: partialSelections.length}); const res = await Promise.race([ - this.api.validator - .submitSyncCommitteeSelections(partialSelections) - .catch((e) => this.logger.error("Error on submitSyncCommitteeSelections", {slot}, e)), + this.api.validator.submitSyncCommitteeSelections(partialSelections), // Exit sync committee contributions flow if there is no response after 2/3 of slot. // This is in contrast to attestations aggregations flow which is already exited at 1/3 of the slot // because for sync committee is not required to resubscribe to subnets as beacon node will assume @@ -275,7 +273,7 @@ export class SyncCommitteeService { ]); if (!res) { - throw new Error("submitSyncCommitteeSelections did not resolve after 2/3 of slot"); + throw new Error("Failed to receive combined selection proofs before 2/3 of slot"); } ApiError.assert(res, "Error receiving combined selection proofs"); From ae9f572c8e88472b28803733d353f492ff74bfc4 Mon Sep 17 00:00:00 2001 From: Nico Flaig Date: Mon, 24 Jul 2023 18:59:35 +0200 Subject: [PATCH 4/5] fix: gracefully terminate connections when closing http server (#5786) * Gracefully terminate connections when closing http server * Log unexpected errors when shutting down server * Fix code references in comments * More gracefully close eventstream api * Misc updates * Close idle connections on server * Update ts-expect-error comments --- .../beacon-node/src/api/rest/activeSockets.ts | 74 ++++++++++++++++--- packages/beacon-node/src/api/rest/base.ts | 25 ++----- .../beacon-node/src/metrics/server/http.ts | 8 +- packages/utils/src/index.ts | 1 + packages/utils/src/waitFor.ts | 53 +++++++++++++ packages/utils/test/unit/waitFor.test.ts | 37 ++++++++++ 6 files changed, 166 insertions(+), 32 deletions(-) create mode 100644 packages/utils/src/waitFor.ts create mode 100644 packages/utils/test/unit/waitFor.test.ts diff --git a/packages/beacon-node/src/api/rest/activeSockets.ts b/packages/beacon-node/src/api/rest/activeSockets.ts index b98db9b0fb9b..ba8a35c80119 100644 --- a/packages/beacon-node/src/api/rest/activeSockets.ts +++ b/packages/beacon-node/src/api/rest/activeSockets.ts @@ -1,5 +1,6 @@ import http, {Server} from "node:http"; import {Socket} from "node:net"; +import {waitFor} from "@lodestar/utils"; import {IGauge} from "../../metrics/index.js"; export type SocketMetrics = { @@ -8,17 +9,23 @@ export type SocketMetrics = { socketsBytesWritten: IGauge; }; +// Use relatively short timeout to speed up shutdown +const GRACEFUL_TERMINATION_TIMEOUT = 1_000; + /** - * From https://github.com/nodejs/node/blob/57bd715d527aba8dae56b975056961b0e429e91e/lib/_http_client.js#L363-L413 - * But exposes the count of sockets, and does not have a graceful period + * From https://github.com/gajus/http-terminator/blob/aabca4751552e983f8a59ba896b7fb58ce3b4087/src/factories/createInternalHttpTerminator.ts#L24-L61 + * But only handles HTTP sockets, exposes the count of sockets as metrics */ export class HttpActiveSocketsTracker { private sockets = new Set(); - private terminated = false; + private terminating = false; - constructor(server: Server, metrics: SocketMetrics | null) { + constructor( + private readonly server: Server, + metrics: SocketMetrics | null + ) { server.on("connection", (socket) => { - if (this.terminated) { + if (this.terminating) { socket.destroy(Error("Closing")); } else { this.sockets.add(socket); @@ -39,18 +46,65 @@ export class HttpActiveSocketsTracker { } } - destroyAll(): void { - this.terminated = true; + /** + * Wait for all connections to drain, forcefully terminate any open connections after timeout + * + * From https://github.com/gajus/http-terminator/blob/aabca4751552e983f8a59ba896b7fb58ce3b4087/src/factories/createInternalHttpTerminator.ts#L78-L165 + * But only handles HTTP sockets and does not close server, immediately closes eventstream API connections + */ + async terminate(): Promise { + if (this.terminating) return; + this.terminating = true; + + // Can speed up shutdown by a few milliseconds + this.server.closeIdleConnections(); + + // Inform new incoming requests on keep-alive connections that + // the connection will be closed after the current response + this.server.on("request", (_req, res) => { + if (!res.headersSent) { + res.setHeader("Connection", "close"); + } + }); for (const socket of this.sockets) { // This is the HTTP CONNECT request socket. - // @ts-expect-error Unclear if I am using wrong type or how else this should be handled. + // @ts-expect-error HTTP sockets have reference to server if (!(socket.server instanceof http.Server)) { continue; } - socket.destroy(Error("Closing")); - this.sockets.delete(socket); + // @ts-expect-error Internal property but only way to access response of socket + const res = socket._httpMessage as http.ServerResponse | undefined; + + if (res == null) { + // Immediately destroy sockets without an attached HTTP request + this.destroySocket(socket); + } else if (res.getHeader("Content-Type") === "text/event-stream") { + // eventstream API will never stop and must be forcefully closed + socket.end(); + } else if (!res.headersSent) { + // Inform existing keep-alive connections that they will be closed after the current response + res.setHeader("Connection", "close"); + } + } + + // Wait for all connections to drain, forcefully terminate after timeout + try { + await waitFor(() => this.sockets.size === 0, { + timeout: GRACEFUL_TERMINATION_TIMEOUT, + }); + } catch { + // Ignore timeout error + } finally { + for (const socket of this.sockets) { + this.destroySocket(socket); + } } } + + private destroySocket(socket: Socket): void { + socket.destroy(Error("Closing")); + this.sockets.delete(socket); + } } diff --git a/packages/beacon-node/src/api/rest/base.ts b/packages/beacon-node/src/api/rest/base.ts index fda16fffc8ce..c9c6e4bfb0ef 100644 --- a/packages/beacon-node/src/api/rest/base.ts +++ b/packages/beacon-node/src/api/rest/base.ts @@ -29,11 +29,6 @@ export type RestApiServerMetrics = SocketMetrics & { errors: IGauge<"operationId">; }; -enum Status { - Listening = "listening", - Closed = "closed", -} - /** * REST API powered by `fastify` server. */ @@ -42,8 +37,6 @@ export class RestApiServer { protected readonly logger: Logger; private readonly activeSockets: HttpActiveSocketsTracker; - private status = Status.Closed; - constructor( private readonly opts: RestApiServerOpts, modules: RestApiServerModules @@ -106,8 +99,7 @@ export class RestApiServer { server.addHook("onError", async (req, _res, err) => { // Don't log ErrorAborted errors, they happen on node shutdown and are not useful // Don't log NodeISSyncing errors, they happen very frequently while syncing and the validator polls duties - // Don't log eventstream aborted errors if server instance is being closed on node shutdown - if (err instanceof ErrorAborted || err instanceof NodeIsSyncing || this.status === Status.Closed) return; + if (err instanceof ErrorAborted || err instanceof NodeIsSyncing) return; const {operationId} = req.routeConfig as RouteConfig; @@ -127,9 +119,6 @@ export class RestApiServer { * Start the REST API server. */ async listen(): Promise { - if (this.status === Status.Listening) return; - this.status = Status.Listening; - try { const host = this.opts.address; const address = await this.server.listen({port: this.opts.port, host}); @@ -139,7 +128,6 @@ export class RestApiServer { } } catch (e) { this.logger.error("Error starting REST api server", this.opts, e as Error); - this.status = Status.Closed; throw e; } } @@ -148,17 +136,16 @@ export class RestApiServer { * Close the server instance and terminate all existing connections. */ async close(): Promise { - if (this.status === Status.Closed) return; - this.status = Status.Closed; - // In NodeJS land calling close() only causes new connections to be rejected. // Existing connections can prevent .close() from resolving for potentially forever. - // In Lodestar case when the BeaconNode wants to close we will just abruptly terminate - // all existing connections for a fast shutdown. + // In Lodestar case when the BeaconNode wants to close we will attempt to gracefully + // close all existing connections but forcefully terminate after timeout for a fast shutdown. // Inspired by https://github.com/gajus/http-terminator/ - this.activeSockets.destroyAll(); + await this.activeSockets.terminate(); await this.server.close(); + + this.logger.debug("REST API server closed"); } /** For child classes to override */ diff --git a/packages/beacon-node/src/metrics/server/http.ts b/packages/beacon-node/src/metrics/server/http.ts index ba17b743d112..b699471e07d5 100644 --- a/packages/beacon-node/src/metrics/server/http.ts +++ b/packages/beacon-node/src/metrics/server/http.ts @@ -90,10 +90,10 @@ export async function getHttpMetricsServer( async close(): Promise { // In NodeJS land calling close() only causes new connections to be rejected. // Existing connections can prevent .close() from resolving for potentially forever. - // In Lodestar case when the BeaconNode wants to close we will just abruptly terminate - // all existing connections for a fast shutdown. + // In Lodestar case when the BeaconNode wants to close we will attempt to gracefully + // close all existing connections but forcefully terminate after timeout for a fast shutdown. // Inspired by https://github.com/gajus/http-terminator/ - activeSockets.destroyAll(); + await activeSockets.terminate(); await new Promise((resolve, reject) => { server.close((err) => { @@ -101,6 +101,8 @@ export async function getHttpMetricsServer( else resolve(); }); }); + + logger.debug("Metrics HTTP server closed"); }, }; } diff --git a/packages/utils/src/index.ts b/packages/utils/src/index.ts index bcb0bf27109f..a09c615fbf2b 100644 --- a/packages/utils/src/index.ts +++ b/packages/utils/src/index.ts @@ -16,3 +16,4 @@ export * from "./timeout.js"; export {RecursivePartial, bnToNum} from "./types.js"; export * from "./verifyMerkleBranch.js"; export * from "./promise.js"; +export * from "./waitFor.js"; diff --git a/packages/utils/src/waitFor.ts b/packages/utils/src/waitFor.ts new file mode 100644 index 000000000000..91206267e6ca --- /dev/null +++ b/packages/utils/src/waitFor.ts @@ -0,0 +1,53 @@ +import {ErrorAborted, TimeoutError} from "./errors.js"; + +export type WaitForOpts = { + /** Time in milliseconds between checking condition */ + interval?: number; + /** Time in milliseconds to wait before throwing TimeoutError */ + timeout?: number; + /** Abort signal to stop waiting for condition by throwing ErrorAborted */ + signal?: AbortSignal; +}; + +/** + * Wait for a condition to be true + */ +export function waitFor(condition: () => boolean, opts: WaitForOpts = {}): Promise { + return new Promise((resolve, reject) => { + const {interval = 10, timeout = Infinity, signal} = opts; + + if (signal?.aborted) { + return reject(new ErrorAborted()); + } + + if (condition()) { + return resolve(); + } + + let onDone: () => void = () => {}; + + const timeoutId = setTimeout(() => { + onDone(); + reject(new TimeoutError()); + }, timeout); + + const intervalId = setInterval(() => { + if (condition()) { + onDone(); + resolve(); + } + }, interval); + + const onAbort = (): void => { + onDone(); + reject(new ErrorAborted()); + }; + if (signal) signal.addEventListener("abort", onAbort); + + onDone = () => { + clearTimeout(timeoutId); + clearInterval(intervalId); + if (signal) signal.removeEventListener("abort", onAbort); + }; + }); +} diff --git a/packages/utils/test/unit/waitFor.test.ts b/packages/utils/test/unit/waitFor.test.ts new file mode 100644 index 000000000000..1dd3dec766b7 --- /dev/null +++ b/packages/utils/test/unit/waitFor.test.ts @@ -0,0 +1,37 @@ +import "../setup.js"; +import {expect} from "chai"; +import {waitFor} from "../../src/waitFor.js"; +import {ErrorAborted, TimeoutError} from "../../src/errors.js"; + +describe("waitFor", () => { + const interval = 10; + const timeout = 20; + + it("Should resolve if condition is already true", async () => { + await expect(waitFor(() => true, {interval, timeout})).to.be.fulfilled; + }); + + it("Should resolve if condition becomes true within timeout", async () => { + let condition = false; + setTimeout(() => { + condition = true; + }, interval); + await waitFor(() => condition, {interval, timeout}); + }); + + it("Should reject with TimeoutError if condition does not become true within timeout", async () => { + await expect(waitFor(() => false, {interval, timeout})).to.be.rejectedWith(TimeoutError); + }); + + it("Should reject with ErrorAborted if aborted before condition becomes true", async () => { + const controller = new AbortController(); + setTimeout(() => controller.abort(), interval); + await expect(waitFor(() => false, {interval, timeout, signal: controller.signal})).to.be.rejectedWith(ErrorAborted); + }); + + it("Should reject with ErrorAborted if signal is already aborted", async () => { + const controller = new AbortController(); + controller.abort(); + await expect(waitFor(() => true, {interval, timeout, signal: controller.signal})).to.be.rejectedWith(ErrorAborted); + }); +}); From 7b5fc63e678c4d9010b8b6993a17a8db84667013 Mon Sep 17 00:00:00 2001 From: Nico Flaig Date: Mon, 24 Jul 2023 19:06:26 +0200 Subject: [PATCH 5/5] fix: eventstream invalid topic error (#5787) --- packages/api/src/beacon/server/events.ts | 11 +++++++++-- packages/api/src/beacon/server/index.ts | 5 ++++- packages/api/src/utils/server/errors.ts | 9 +++++++++ packages/api/src/utils/server/index.ts | 1 + packages/beacon-node/src/api/impl/errors.ts | 10 ++-------- packages/beacon-node/src/api/impl/events/index.ts | 7 ------- 6 files changed, 25 insertions(+), 18 deletions(-) create mode 100644 packages/api/src/utils/server/errors.ts diff --git a/packages/api/src/beacon/server/events.ts b/packages/api/src/beacon/server/events.ts index 3e45b104ecb5..f687f21f874b 100644 --- a/packages/api/src/beacon/server/events.ts +++ b/packages/api/src/beacon/server/events.ts @@ -1,7 +1,7 @@ import {ChainForkConfig} from "@lodestar/config"; import {ErrorAborted} from "@lodestar/utils"; -import {Api, ReqTypes, routesData, getEventSerdes} from "../routes/events.js"; -import {ServerRoutes} from "../../utils/server/index.js"; +import {Api, ReqTypes, routesData, getEventSerdes, eventTypes} from "../routes/events.js"; +import {ApiError, ServerRoutes} from "../../utils/server/index.js"; import {ServerApi} from "../../interfaces.js"; export function getRoutes(config: ChainForkConfig, api: ServerApi): ServerRoutes { @@ -15,6 +15,13 @@ export function getRoutes(config: ChainForkConfig, api: ServerApi): ServerR id: "eventstream", handler: async (req, res) => { + const validTopics = new Set(Object.values(eventTypes)); + for (const topic of req.query.topics) { + if (!validTopics.has(topic)) { + throw new ApiError(400, `Invalid topic: ${topic}`); + } + } + const controller = new AbortController(); try { diff --git a/packages/api/src/beacon/server/index.ts b/packages/api/src/beacon/server/index.ts index 4f90ed24dee7..6c1cc9c16a4b 100644 --- a/packages/api/src/beacon/server/index.ts +++ b/packages/api/src/beacon/server/index.ts @@ -1,6 +1,6 @@ import {ChainForkConfig} from "@lodestar/config"; import {Api} from "../routes/index.js"; -import {ServerInstance, ServerRoute, RouteConfig, registerRoute} from "../../utils/server/index.js"; +import {ApiError, ServerInstance, ServerRoute, RouteConfig, registerRoute} from "../../utils/server/index.js"; import {ServerApi} from "../../interfaces.js"; import * as beacon from "./beacon.js"; @@ -13,6 +13,9 @@ import * as node from "./node.js"; import * as proof from "./proof.js"; import * as validator from "./validator.js"; +// Re-export for usage in beacon-node +export {ApiError}; + // Re-export for convenience export {RouteConfig}; diff --git a/packages/api/src/utils/server/errors.ts b/packages/api/src/utils/server/errors.ts new file mode 100644 index 000000000000..ea075678f4f6 --- /dev/null +++ b/packages/api/src/utils/server/errors.ts @@ -0,0 +1,9 @@ +import {HttpErrorCodes} from "../client/httpStatusCode.js"; + +export class ApiError extends Error { + statusCode: HttpErrorCodes; + constructor(statusCode: HttpErrorCodes, message?: string) { + super(message); + this.statusCode = statusCode; + } +} diff --git a/packages/api/src/utils/server/index.ts b/packages/api/src/utils/server/index.ts index 5a0227a01916..2e17e9ee2a6f 100644 --- a/packages/api/src/utils/server/index.ts +++ b/packages/api/src/utils/server/index.ts @@ -1,3 +1,4 @@ export * from "./genericJsonServer.js"; export * from "./registerRoute.js"; +export * from "./errors.js"; export * from "./types.js"; diff --git a/packages/beacon-node/src/api/impl/errors.ts b/packages/beacon-node/src/api/impl/errors.ts index 7169b7138295..cc877de90a7a 100644 --- a/packages/beacon-node/src/api/impl/errors.ts +++ b/packages/beacon-node/src/api/impl/errors.ts @@ -1,12 +1,6 @@ -import {HttpErrorCodes} from "@lodestar/api"; +import {ApiError} from "@lodestar/api/beacon/server"; -export class ApiError extends Error { - statusCode: HttpErrorCodes; - constructor(statusCode: HttpErrorCodes, message?: string) { - super(message); - this.statusCode = statusCode; - } -} +export {ApiError}; export class StateNotFound extends ApiError { constructor() { diff --git a/packages/beacon-node/src/api/impl/events/index.ts b/packages/beacon-node/src/api/impl/events/index.ts index 18f9ff160b3a..9f75bc0a83c5 100644 --- a/packages/beacon-node/src/api/impl/events/index.ts +++ b/packages/beacon-node/src/api/impl/events/index.ts @@ -1,19 +1,12 @@ import {routes, ServerApi} from "@lodestar/api"; import {ApiModules} from "../types.js"; -import {ApiError} from "../errors.js"; export function getEventsApi({chain}: Pick): ServerApi { - const validTopics = new Set(Object.values(routes.events.eventTypes)); - return { async eventstream(topics, signal, onEvent) { const onAbortFns: (() => void)[] = []; for (const topic of topics) { - if (!validTopics.has(topic)) { - throw new ApiError(400, `Unknown topic ${topic}`); - } - // eslint-disable-next-line @typescript-eslint/no-explicit-any const handler = (data: any): void => { // TODO: What happens if this handler throws? Does it break the other chain.emitter listeners?