From f54799bf78e7f7c1c82132fd0d4b8f1aa534d138 Mon Sep 17 00:00:00 2001 From: Nico Flaig Date: Wed, 2 Oct 2024 17:11:06 +0100 Subject: [PATCH] fix: update multiple api errors to be spec compliant (#7113) * fix: update HTTP error response format to be spec compliant * Fix error assertions * Remove group tag from keymanager option * Fix body has already been read * Assert deepEqual * fix: update multiple api errors to be spec compliant * Reorder test cases * Update res numbering * Use strict deep equal --- packages/api/src/utils/client/response.ts | 5 ++- .../src/api/impl/beacon/pool/index.ts | 32 ++++++++----------- packages/beacon-node/src/api/impl/errors.ts | 13 ++++++++ packages/beacon-node/src/api/rest/base.ts | 14 +++++++- packages/cli/test/sim/endpoints.test.ts | 24 +++++++++++--- 5 files changed, 63 insertions(+), 25 deletions(-) diff --git a/packages/api/src/utils/client/response.ts b/packages/api/src/utils/client/response.ts index ed008273588a..fdcb2afda943 100644 --- a/packages/api/src/utils/client/response.ts +++ b/packages/api/src/utils/client/response.ts @@ -189,8 +189,11 @@ export class ApiResponse extends Response { private getErrorMessage(): string { const errBody = this.resolvedErrorBody(); try { - const errJson = JSON.parse(errBody) as {message?: string}; + const errJson = JSON.parse(errBody) as {message?: string; failures?: {message: string}[]}; if (errJson.message) { + if (errJson.failures) { + return `${errJson.message}\n` + errJson.failures.map((e) => e.message).join("\n"); + } return errJson.message; } else { return errBody; 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 398238aa2508..e01b172f1e72 100644 --- a/packages/beacon-node/src/api/impl/beacon/pool/index.ts +++ b/packages/beacon-node/src/api/impl/beacon/pool/index.ts @@ -16,7 +16,7 @@ import { SyncCommitteeError, } from "../../../../chain/errors/index.js"; import {validateGossipFnRetryUnknownRoot} from "../../../../network/processor/gossipHandlers.js"; -import {ApiError} from "../../errors.js"; +import {ApiError, FailureList, IndexedError} from "../../errors.js"; export function getBeaconPoolApi({ chain, @@ -88,7 +88,7 @@ export function getBeaconPoolApi({ async submitPoolAttestationsV2({signedAttestations}) { const seenTimestampSec = Date.now() / 1000; - const errors: Error[] = []; + const failures: FailureList = []; await Promise.all( signedAttestations.map(async (attestation, i) => { @@ -127,7 +127,7 @@ export function getBeaconPoolApi({ return; } - errors.push(e as Error); + failures.push({index: i, message: (e as Error).message}); 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"); @@ -136,10 +136,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 (failures.length > 0) { + throw new IndexedError("Error processing attestations", failures); } }, @@ -168,7 +166,7 @@ export function getBeaconPoolApi({ }, async submitPoolBLSToExecutionChange({blsToExecutionChanges}) { - const errors: Error[] = []; + const failures: FailureList = []; await Promise.all( blsToExecutionChanges.map(async (blsToExecutionChange, i) => { @@ -184,7 +182,7 @@ export function getBeaconPoolApi({ await network.publishBlsToExecutionChange(blsToExecutionChange); } } catch (e) { - errors.push(e as Error); + failures.push({index: i, message: (e as Error).message}); logger.error( `Error on submitPoolBLSToExecutionChange [${i}]`, {validatorIndex: blsToExecutionChange.message.validatorIndex}, @@ -194,10 +192,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 (failures.length > 0) { + throw new IndexedError("Error processing BLS to execution changes", failures); } }, @@ -221,7 +217,7 @@ export function getBeaconPoolApi({ // TODO: Fetch states at signature slots const state = chain.getHeadState(); - const errors: Error[] = []; + const failures: FailureList = []; await Promise.all( signatures.map(async (signature, i) => { @@ -261,7 +257,7 @@ export function getBeaconPoolApi({ return; } - errors.push(e as Error); + failures.push({index: i, message: (e as Error).message}); logger.debug( `Error on submitPoolSyncCommitteeSignatures [${i}]`, {slot: signature.slot, validatorIndex: signature.validatorIndex}, @@ -274,10 +270,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 (failures.length > 0) { + throw new IndexedError("Error processing sync committee signatures", failures); } }, }; diff --git a/packages/beacon-node/src/api/impl/errors.ts b/packages/beacon-node/src/api/impl/errors.ts index 848691f7cf6d..609f40f83a12 100644 --- a/packages/beacon-node/src/api/impl/errors.ts +++ b/packages/beacon-node/src/api/impl/errors.ts @@ -35,3 +35,16 @@ export class OnlySupportedByDVT extends ApiError { super(501, "Only supported by distributed validator middleware clients"); } } + +// Error thrown when processing multiple items failed - https://github.com/ethereum/beacon-APIs/blob/e7f7d70423b0abfe9d9f33b701be2ec03e44eb02/types/http.yaml#L175 +export class IndexedError extends ApiError { + failures: FailureList; + + constructor(message: string, failures: FailureList) { + super(400, message); + + this.failures = failures.sort((a, b) => a.index - b.index); + } +} + +export type FailureList = {index: number; message: string}[]; diff --git a/packages/beacon-node/src/api/rest/base.ts b/packages/beacon-node/src/api/rest/base.ts index e8ea85f67af5..10318b1b3ba7 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 {addSszContentTypeParser} from "@lodestar/api/server"; import {ErrorAborted, Gauge, Histogram, Logger} from "@lodestar/utils"; import {isLocalhostIP} from "../../util/ip.js"; -import {ApiError, NodeIsSyncing} from "../impl/errors.js"; +import {ApiError, FailureList, IndexedError, NodeIsSyncing} from "../impl/errors.js"; import {HttpActiveSocketsTracker, SocketMetrics} from "./activeSockets.js"; export type RestApiServerOpts = { @@ -41,6 +41,10 @@ type ErrorResponse = { stacktraces?: string[]; }; +type IndexedErrorResponse = ErrorResponse & { + failures?: FailureList; +}; + /** * Error code used by Fastify if media type is not supported (415) */ @@ -97,6 +101,14 @@ export class RestApiServer { stacktraces, }; void res.status(400).send(payload); + } else if (err instanceof IndexedError) { + const payload: IndexedErrorResponse = { + code: err.statusCode, + message: err.message, + failures: err.failures, + stacktraces, + }; + void res.status(err.statusCode).send(payload); } else { // Convert our custom ApiError into status code const statusCode = err instanceof ApiError ? err.statusCode : 500; diff --git a/packages/cli/test/sim/endpoints.test.ts b/packages/cli/test/sim/endpoints.test.ts index b276ad4787c6..6a119fc219d7 100644 --- a/packages/cli/test/sim/endpoints.test.ts +++ b/packages/cli/test/sim/endpoints.test.ts @@ -3,6 +3,7 @@ import path from "node:path"; import assert from "node:assert"; import {toHexString} from "@chainsafe/ssz"; import {routes, fetch} from "@lodestar/api"; +import {ssz} from "@lodestar/types"; import {Simulation} from "../utils/crucible/simulation.js"; import {BeaconClient, ExecutionClient} from "../utils/crucible/interfaces.js"; import {defineSimTestConfig, logFilesDir} from "../utils/crucible/utils/index.js"; @@ -108,15 +109,30 @@ await env.tracker.assert( await env.tracker.assert("should return HTTP error responses in a spec compliant format", async () => { // ApiError with status 400 is thrown by handler const res1 = await node.api.beacon.getStateValidator({stateId: "current", validatorId: 1}); - assert.deepEqual(JSON.parse(await res1.errorBody()), {code: 400, message: "Invalid block id 'current'"}); + assert.deepStrictEqual(JSON.parse(await res1.errorBody()), {code: 400, message: "Invalid block id 'current'"}); // JSON schema validation failed const res2 = await node.api.beacon.getPoolAttestationsV2({slot: "current" as unknown as number, committeeIndex: 123}); - assert.deepEqual(JSON.parse(await res2.errorBody()), {code: 400, message: "slot must be integer"}); + assert.deepStrictEqual(JSON.parse(await res2.errorBody()), {code: 400, message: "slot must be integer"}); + + // Error processing multiple items + const signedAttestations = Array.from({length: 3}, () => ssz.phase0.Attestation.defaultValue()); + const res3 = await node.api.beacon.submitPoolAttestationsV2({signedAttestations}); + const errBody = JSON.parse(await res3.errorBody()) as {code: number; message: string; failures: unknown[]}; + assert.equal(errBody.code, 400); + assert.equal(errBody.message, "Error processing attestations"); + assert.equal(errBody.failures.length, signedAttestations.length); + assert.deepStrictEqual(errBody.failures[0], { + index: 0, + message: "ATTESTATION_ERROR_NOT_EXACTLY_ONE_AGGREGATION_BIT_SET", + }); // Route does not exist - const res3 = await fetch(`${node.restPublicUrl}/not/implemented/route`); - assert.deepEqual(JSON.parse(await res3.text()), {code: 404, message: "Route GET:/not/implemented/route not found"}); + const res4 = await fetch(`${node.restPublicUrl}/not/implemented/route`); + assert.deepStrictEqual(JSON.parse(await res4.text()), { + code: 404, + message: "Route GET:/not/implemented/route not found", + }); }); await env.tracker.assert("BN Not Synced", async () => {