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

@@ -189,8 +189,11 @@ export class ApiResponse<E extends Endpoint> 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;

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);
       }
     },
   };

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}[];

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;

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 () => {