From 55f4cd4afcd6c63f612a513fcbe11d57159f09a5 Mon Sep 17 00:00:00 2001 From: Harikrishnan Shaji Date: Sun, 25 Feb 2024 22:17:23 +0530 Subject: [PATCH 1/6] Add new MultipleError and update submitPoolAttestations to use it --- .../beacon-node/src/api/impl/beacon/pool/index.ts | 11 +++++------ packages/beacon-node/src/api/impl/errors.ts | 14 ++++++++++++++ packages/beacon-node/src/api/rest/base.ts | 10 +++++++++- 3 files changed, 28 insertions(+), 7 deletions(-) diff --git a/packages/beacon-node/src/api/impl/beacon/pool/index.ts b/packages/beacon-node/src/api/impl/beacon/pool/index.ts index 09a66eba4d15..83f7a2c2e669 100644 --- a/packages/beacon-node/src/api/impl/beacon/pool/index.ts +++ b/packages/beacon-node/src/api/impl/beacon/pool/index.ts @@ -15,6 +15,7 @@ import { SyncCommitteeError, } from "../../../../chain/errors/index.js"; import {validateGossipFnRetryUnknownRoot} from "../../../../network/processor/gossipHandlers.js"; +import { MultipleError } from "../../errors.js"; export function getBeaconPoolApi({ chain, @@ -52,7 +53,7 @@ export function getBeaconPoolApi({ async submitPoolAttestations(attestations) { const seenTimestampSec = Date.now() / 1000; - const errors: Error[] = []; + const errors: {index: number; error: Error}[] = []; await Promise.all( attestations.map(async (attestation, i) => { @@ -91,7 +92,7 @@ export function getBeaconPoolApi({ return; } - errors.push(e as Error); + errors.push({index: i, error: e as Error}); logger.error(`Error on submitPoolAttestations [${i}]`, logCtx, e as Error); if (e instanceof AttestationError && e.action === GossipAction.REJECT) { chain.persistInvalidSszValue(ssz.phase0.Attestation, attestation, "api_reject"); @@ -100,10 +101,8 @@ export function getBeaconPoolApi({ }) ); - if (errors.length > 1) { - throw Error("Multiple errors on submitPoolAttestations\n" + errors.map((e) => e.message).join("\n")); - } else if (errors.length === 1) { - throw errors[0]; + if (errors.length > 0) { + throw new MultipleError("Some errors on submitPoolAttestations", errors); } }, diff --git a/packages/beacon-node/src/api/impl/errors.ts b/packages/beacon-node/src/api/impl/errors.ts index cc877de90a7a..005f539f2d53 100644 --- a/packages/beacon-node/src/api/impl/errors.ts +++ b/packages/beacon-node/src/api/impl/errors.ts @@ -35,3 +35,17 @@ export class OnlySupportedByDVT extends ApiError { super(501, "Only supported by distributed validator middleware clients"); } } + +export class MultipleError extends ApiError { + failures: {index: number; message: string}[]; + + constructor(message: string, errors: {index: number; error: Error}[]) { + super(400, message); + + const failures = []; + for (const {index, error} of errors) { + failures.push({index: index, message: error.message}); + } + this.failures = failures; + } +} diff --git a/packages/beacon-node/src/api/rest/base.ts b/packages/beacon-node/src/api/rest/base.ts index f14937a49fa4..4be2460bead3 100644 --- a/packages/beacon-node/src/api/rest/base.ts +++ b/packages/beacon-node/src/api/rest/base.ts @@ -5,7 +5,7 @@ import bearerAuthPlugin from "@fastify/bearer-auth"; import {RouteConfig} from "@lodestar/api/beacon/server"; import {ErrorAborted, Gauge, Histogram, Logger} from "@lodestar/utils"; import {isLocalhostIP} from "../../util/ip.js"; -import {ApiError, NodeIsSyncing} from "../impl/errors.js"; +import {ApiError, MultipleError, NodeIsSyncing} from "../impl/errors.js"; import {HttpActiveSocketsTracker, SocketMetrics} from "./activeSockets.js"; export type RestApiServerOpts = { @@ -66,6 +66,14 @@ export class RestApiServer { server.setErrorHandler((err, req, res) => { if (err.validation) { void res.status(400).send(err.validation); + } else if (err instanceof MultipleError) { + // api's returning multiple errors need to formatted in a certain way + const body = { + code: err.statusCode, + message: err.message, + failures: err.failures, + }; + void res.status(err.statusCode).send(body); } else { // Convert our custom ApiError into status code const statusCode = err instanceof ApiError ? err.statusCode : 500; From 16611c8d874a4641b8f772e83872eae798429536 Mon Sep 17 00:00:00 2001 From: Harikrishnan Shaji Date: Mon, 26 Feb 2024 14:20:56 +0530 Subject: [PATCH 2/6] Rename MultipleError to IndexedError + change error message in submitPoolAttestations --- packages/beacon-node/src/api/impl/beacon/pool/index.ts | 4 ++-- packages/beacon-node/src/api/impl/errors.ts | 2 +- packages/beacon-node/src/api/rest/base.ts | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/beacon-node/src/api/impl/beacon/pool/index.ts b/packages/beacon-node/src/api/impl/beacon/pool/index.ts index 83f7a2c2e669..b21a8f67fa1a 100644 --- a/packages/beacon-node/src/api/impl/beacon/pool/index.ts +++ b/packages/beacon-node/src/api/impl/beacon/pool/index.ts @@ -15,7 +15,7 @@ import { SyncCommitteeError, } from "../../../../chain/errors/index.js"; import {validateGossipFnRetryUnknownRoot} from "../../../../network/processor/gossipHandlers.js"; -import { MultipleError } from "../../errors.js"; +import {IndexedError} from "../../errors.js"; export function getBeaconPoolApi({ chain, @@ -102,7 +102,7 @@ export function getBeaconPoolApi({ ); if (errors.length > 0) { - throw new MultipleError("Some errors on submitPoolAttestations", errors); + throw new IndexedError("Some errors submitting attestations", errors); } }, diff --git a/packages/beacon-node/src/api/impl/errors.ts b/packages/beacon-node/src/api/impl/errors.ts index 005f539f2d53..13991470ccf8 100644 --- a/packages/beacon-node/src/api/impl/errors.ts +++ b/packages/beacon-node/src/api/impl/errors.ts @@ -36,7 +36,7 @@ export class OnlySupportedByDVT extends ApiError { } } -export class MultipleError extends ApiError { +export class IndexedError extends ApiError { failures: {index: number; message: string}[]; constructor(message: string, errors: {index: number; error: Error}[]) { diff --git a/packages/beacon-node/src/api/rest/base.ts b/packages/beacon-node/src/api/rest/base.ts index 4be2460bead3..c30d8ad22d13 100644 --- a/packages/beacon-node/src/api/rest/base.ts +++ b/packages/beacon-node/src/api/rest/base.ts @@ -5,7 +5,7 @@ import bearerAuthPlugin from "@fastify/bearer-auth"; import {RouteConfig} from "@lodestar/api/beacon/server"; import {ErrorAborted, Gauge, Histogram, Logger} from "@lodestar/utils"; import {isLocalhostIP} from "../../util/ip.js"; -import {ApiError, MultipleError, NodeIsSyncing} from "../impl/errors.js"; +import {ApiError, IndexedError, NodeIsSyncing} from "../impl/errors.js"; import {HttpActiveSocketsTracker, SocketMetrics} from "./activeSockets.js"; export type RestApiServerOpts = { @@ -66,8 +66,8 @@ export class RestApiServer { server.setErrorHandler((err, req, res) => { if (err.validation) { void res.status(400).send(err.validation); - } else if (err instanceof MultipleError) { - // api's returning multiple errors need to formatted in a certain way + } else if (err instanceof IndexedError) { + // api's returning IndexedError need to formatted in a certain way const body = { code: err.statusCode, message: err.message, From a86e85afe8708c9148dc068e0ac47237c56e7f99 Mon Sep 17 00:00:00 2001 From: Harikrishnan Shaji Date: Sat, 2 Mar 2024 19:15:10 +0530 Subject: [PATCH 3/6] Update getErrorMessage to handle failures key --- packages/api/src/utils/client/httpClient.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/api/src/utils/client/httpClient.ts b/packages/api/src/utils/client/httpClient.ts index 9cbadde1cca5..1ac7e81156c8 100644 --- a/packages/api/src/utils/client/httpClient.ts +++ b/packages/api/src/utils/client/httpClient.ts @@ -373,8 +373,8 @@ function isAbortedError(e: Error): boolean { function getErrorMessage(errBody: string): string { try { - const errJson = JSON.parse(errBody) as {message: string}; - if (errJson.message) { + const errJson = JSON.parse(errBody) as {message: string; failures?: {index: number; message: string}[]}; + if (errJson.message && !errJson.failures) { return errJson.message; } else { return errBody; From 0ad7c213f882eb6bf35fd149400d3aa3b70fcafe Mon Sep 17 00:00:00 2001 From: Harikrishnan Shaji Date: Sat, 2 Mar 2024 20:17:43 +0530 Subject: [PATCH 4/6] Update submitPoolBlsToExecutionChange to use IndexedError --- packages/beacon-node/src/api/impl/beacon/pool/index.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/packages/beacon-node/src/api/impl/beacon/pool/index.ts b/packages/beacon-node/src/api/impl/beacon/pool/index.ts index b21a8f67fa1a..ad9d6b17a7d7 100644 --- a/packages/beacon-node/src/api/impl/beacon/pool/index.ts +++ b/packages/beacon-node/src/api/impl/beacon/pool/index.ts @@ -126,7 +126,7 @@ export function getBeaconPoolApi({ }, async submitPoolBlsToExecutionChange(blsToExecutionChanges) { - const errors: Error[] = []; + const errors: {index: number; error: Error}[] = []; await Promise.all( blsToExecutionChanges.map(async (blsToExecutionChange, i) => { @@ -142,7 +142,7 @@ export function getBeaconPoolApi({ await network.publishBlsToExecutionChange(blsToExecutionChange); } } catch (e) { - errors.push(e as Error); + errors.push({index: i, error: e as Error}); logger.error( `Error on submitPoolBlsToExecutionChange [${i}]`, {validatorIndex: blsToExecutionChange.message.validatorIndex}, @@ -152,10 +152,8 @@ export function getBeaconPoolApi({ }) ); - if (errors.length > 1) { - throw Error("Multiple errors on submitPoolBlsToExecutionChange\n" + errors.map((e) => e.message).join("\n")); - } else if (errors.length === 1) { - throw errors[0]; + if (errors.length > 0) { + throw new IndexedError("Some errors submitting BLS to execution change", errors); } }, From e32fb509e132798bf2f5939ee822a84673504ac2 Mon Sep 17 00:00:00 2001 From: Harikrishnan Shaji Date: Sat, 2 Mar 2024 20:19:16 +0530 Subject: [PATCH 5/6] Update submitPoolSyncCommitteeSignatures to use IndexedError --- packages/beacon-node/src/api/impl/beacon/pool/index.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/packages/beacon-node/src/api/impl/beacon/pool/index.ts b/packages/beacon-node/src/api/impl/beacon/pool/index.ts index ad9d6b17a7d7..6fab3447bee2 100644 --- a/packages/beacon-node/src/api/impl/beacon/pool/index.ts +++ b/packages/beacon-node/src/api/impl/beacon/pool/index.ts @@ -177,7 +177,7 @@ export function getBeaconPoolApi({ // TODO: Fetch states at signature slots const state = chain.getHeadState(); - const errors: Error[] = []; + const errors: {index: number; error: Error}[] = []; await Promise.all( signatures.map(async (signature, i) => { @@ -217,7 +217,7 @@ export function getBeaconPoolApi({ return; } - errors.push(e as Error); + errors.push({index: i, error: e as Error}); logger.debug( `Error on submitPoolSyncCommitteeSignatures [${i}]`, {slot: signature.slot, validatorIndex: signature.validatorIndex}, @@ -230,10 +230,8 @@ export function getBeaconPoolApi({ }) ); - if (errors.length > 1) { - throw Error("Multiple errors on submitPoolSyncCommitteeSignatures\n" + errors.map((e) => e.message).join("\n")); - } else if (errors.length === 1) { - throw errors[0]; + if (errors.length > 0) { + throw new IndexedError("Some errors submitting sync committee signatures", errors); } }, }; From 0acfc71a3548a0869d83667f529e9b577b578052 Mon Sep 17 00:00:00 2001 From: Harikrishnan Shaji Date: Sun, 3 Mar 2024 14:22:00 +0530 Subject: [PATCH 6/6] Add e2e api tests checking correct handling of IndexedError --- .../api/impl/beacon/pool/endpoints.test.ts | 95 +++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 packages/beacon-node/test/e2e/api/impl/beacon/pool/endpoints.test.ts diff --git a/packages/beacon-node/test/e2e/api/impl/beacon/pool/endpoints.test.ts b/packages/beacon-node/test/e2e/api/impl/beacon/pool/endpoints.test.ts new file mode 100644 index 000000000000..0663f0d26aa9 --- /dev/null +++ b/packages/beacon-node/test/e2e/api/impl/beacon/pool/endpoints.test.ts @@ -0,0 +1,95 @@ +import {describe, beforeAll, afterAll, it, expect} from "vitest"; +import {createBeaconConfig} from "@lodestar/config"; +import {chainConfig as chainConfigDef} from "@lodestar/config/default"; +import {Api, getClient} from "@lodestar/api"; +import {ssz} from "@lodestar/types"; +import {LogLevel, testLogger} from "../../../../../utils/logger.js"; +import {getDevBeaconNode} from "../../../../../utils/node/beacon.js"; +import {BeaconNode} from "../../../../../../src/node/nodejs.js"; + +describe("beacon pool api", function () { + const restPort = 9596; + const config = createBeaconConfig(chainConfigDef, Buffer.alloc(32, 0xaa)); + const validatorCount = 512; + + let bn: BeaconNode; + let client: Api["beacon"]; + + beforeAll(async () => { + bn = await getDevBeaconNode({ + params: chainConfigDef, + options: { + sync: {isSingleNode: true}, + network: {allowPublishToZeroPeers: true}, + api: { + rest: { + enabled: true, + port: restPort, + }, + }, + chain: {blsVerifyAllMainThread: true}, + }, + validatorCount, + logger: testLogger("Node-A", {level: LogLevel.info}), + }); + client = getClient({baseUrl: `http://127.0.0.1:${restPort}`}, {config}).beacon; + }); + + afterAll(async () => { + await bn.close(); + }); + + describe("submitPoolAttestations", () => { + it("should return correctly formatted errors responses", async () => { + const attestations = [ssz.phase0.Attestation.defaultValue()]; + const res = await client.submitPoolAttestations(attestations); + + expect(res.ok).toBe(false); + expect(res.status).toBe(400); + + const expectedErrorBody = { + code: 400, + message: "Some errors submitting attestations", + failures: [{index: 0, message: "ATTESTATION_ERROR_NOT_EXACTLY_ONE_AGGREGATION_BIT_SET"}], + }; + const expectedErrorMessage = `Bad Request: ${JSON.stringify(expectedErrorBody)}`; + expect(res.error?.message).toEqual(expectedErrorMessage); + }); + }); + + describe("submitPoolBlsToExecutionChange", () => { + it("should return correctly formatted errors responses", async () => { + const blsToExecutionChanges = [ssz.capella.SignedBLSToExecutionChange.defaultValue()]; + const res = await client.submitPoolBlsToExecutionChange(blsToExecutionChanges); + + expect(res.ok).toBe(false); + expect(res.status).toBe(400); + + const expectedErrorBody = { + code: 400, + message: "Some errors submitting BLS to execution change", + failures: [{index: 0, message: "BLS_TO_EXECUTION_CHANGE_ERROR_INVALID"}], + }; + const expectedErrorMessage = `Bad Request: ${JSON.stringify(expectedErrorBody)}`; + expect(res.error?.message).toEqual(expectedErrorMessage); + }); + }); + + describe("submitPoolSyncCommitteeSignatures", () => { + it("should return correctly formatted errors responses", async () => { + const signatures = [ssz.altair.SyncCommitteeMessage.defaultValue()]; + const res = await client.submitPoolSyncCommitteeSignatures(signatures); + + expect(res.ok).toBe(false); + expect(res.status).toBe(400); + + const expectedErrorBody = { + code: 400, + message: "Some errors submitting sync committee signatures", + failures: [{index: 0, message: "Empty SyncCommitteeCache"}], + }; + const expectedErrorMessage = `Bad Request: ${JSON.stringify(expectedErrorBody)}`; + expect(res.error?.message).toEqual(expectedErrorMessage); + }); + }); +});