Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: update multiple api errors to be spec compliant #7113

Merged
merged 10 commits into from
Oct 2, 2024
5 changes: 4 additions & 1 deletion packages/api/src/utils/client/response.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
32 changes: 13 additions & 19 deletions packages/beacon-node/src/api/impl/beacon/pool/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -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");
Expand All @@ -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);
}
},

Expand Down Expand Up @@ -168,7 +166,7 @@ export function getBeaconPoolApi({
},

async submitPoolBLSToExecutionChange({blsToExecutionChanges}) {
const errors: Error[] = [];
const failures: FailureList = [];

await Promise.all(
blsToExecutionChanges.map(async (blsToExecutionChange, i) => {
Expand All @@ -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},
Expand All @@ -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);
}
},

Expand All @@ -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) => {
Expand Down Expand Up @@ -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},
Expand All @@ -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);
}
},
};
Expand Down
13 changes: 13 additions & 0 deletions packages/beacon-node/src/api/impl/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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}[];
14 changes: 13 additions & 1 deletion packages/beacon-node/src/api/rest/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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)
*/
Expand Down Expand Up @@ -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;
Expand Down
24 changes: 20 additions & 4 deletions packages/cli/test/sim/endpoints.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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 () => {
Expand Down
Loading