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.

Sign up for GitHub

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

Jump to bottom

fix: make multiple api errors spec compliant #6479

Closed
wants to merge 6 commits into from
Closed

fix: make multiple api errors spec compliant #6479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
55f4cd4
Add new MultipleError and update submitPoolAttestations to use it
har777 Feb 25, 2024
16611c8
Rename MultipleError to IndexedError + change error message in submit…
har777 Feb 26, 2024
a86e85a
Update getErrorMessage to handle failures key
har777 Mar 2, 2024
0ad7c21
Update submitPoolBlsToExecutionChange to use IndexedError
har777 Mar 2, 2024
e32fb50
Update submitPoolSyncCommitteeSignatures to use IndexedError
har777 Mar 2, 2024
0acfc71
Add e2e api tests checking correct handling of IndexedError
har777 Mar 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/api/src/utils/client/httpClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}[]};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably add a type to the API package to make sure we notice when the error format ever changes between server / client

if (errJson.message && !errJson.failures) {
return errJson.message;
} else {
return errBody;
Expand Down
31 changes: 13 additions & 18 deletions packages/beacon-node/src/api/impl/beacon/pool/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
SyncCommitteeError,
} from "../../../../chain/errors/index.js";
import {validateGossipFnRetryUnknownRoot} from "../../../../network/processor/gossipHandlers.js";
import {IndexedError} from "../../errors.js";

export function getBeaconPoolApi({
chain,
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -91,7 +92,7 @@ export function getBeaconPoolApi({
return;
}

errors.push(e as Error);
errors.push({index: i, error: e as Error});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is not a plain array of errors anymore, we could consider just tracking failures here directly as we log the errors (incl. stack trace) separately anyways

Suggested change
errors.push({index: i, error: 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 @@ -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 IndexedError("Some errors submitting attestations", errors);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error message is not really correct, there was an error(s) processing / validating the attestations, not submitting them.

Regarding the "some" I would probably just leave it out as per spec if one attestation failed validation it is already considered an error

If one or more attestations fail validation the node MUST return a 400 error with details of which attestations have failed, and why.

We could use the term "validating" or "processing" but since we do additional things like publishing the attestation to the network which can fail as well I think "processing" is better

Suggested change
throw new IndexedError("Some errors submitting attestations", errors);
throw new IndexedError("Error processing attestations", failures);

}
},

Expand All @@ -127,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) => {
Expand All @@ -143,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},
Expand All @@ -153,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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be plural to match other error messages, looks like in ethereum/beacon-APIs#279 it was missed to change the operationId to plural as well when it was changed to a batch API

Suggested change
throw new IndexedError("Some errors submitting BLS to execution change", errors);
throw new IndexedError("Some errors submitting BLS to execution changes", errors);

}
},

Expand All @@ -180,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) => {
Expand Down Expand Up @@ -220,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},
Expand All @@ -233,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);
}
},
};
Expand Down
14 changes: 14 additions & 0 deletions packages/beacon-node/src/api/impl/errors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,17 @@ export class OnlySupportedByDVT extends ApiError {
super(501, "Only supported by distributed validator middleware clients");
}
}

export class IndexedError 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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As noted in other comment, we could just pass failures in as is to avoid this conversion from array of errors to correct failures format.

Slight UX improvement we should consider is sorting the failures by index as order is not guaranteed due to the async processing.

failures.push({index: index, message: error.message});
}
this.failures = failures;
}
}
10 changes: 9 additions & 1 deletion packages/beacon-node/src/api/rest/base.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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, IndexedError, NodeIsSyncing} from "../impl/errors.js";
import {HttpActiveSocketsTracker, SocketMetrics} from "./activeSockets.js";

export type RestApiServerOpts = {
Expand Down Expand Up @@ -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 IndexedError) {
// api's returning IndexedError need to formatted in a certain way
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is applicable to any error just that we don't do it for other error responses (yet)

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;
Expand Down
95 changes: 95 additions & 0 deletions packages/beacon-node/test/e2e/api/impl/beacon/pool/endpoints.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be more descriptive, maybe something like?

Suggested change
it("should return correctly formatted errors responses", async () => {
it("should return an error with details about each failed attestation", async () => {

const attestations = [ssz.phase0.Attestation.defaultValue()];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be better to test with at least > 2 items, or have two tests cases, single item and multiple items

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)}`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to come up with a human readable format for the error message, would have to give this a try myself with > 10 and see how it reads in the logs but I'd imagine having the stringified json body as message is not that great.

Could just update getErrorMessage to result in the same error formatting as previously when multiple errors were added in #2595. Or even better come up with a new way of formatting, but I would really have to see different format to give good suggestions.

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"}],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests will likely break in the future, I am not sure it is the best approach to check for the exact messages here. What we really wanna make sure is that the format is correct and that we return a error for each failure.

};
const expectedErrorMessage = `Bad Request: ${JSON.stringify(expectedErrorBody)}`;
expect(res.error?.message).toEqual(expectedErrorMessage);
});
});
});
Loading