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

Open
wants to merge 6 commits into
base: unstable
Choose a base branch
from
Open

fix: make multiple api errors spec compliant #6479

Show file tree
Hide file tree
Changes from 1 commit
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
11 changes: 5 additions & 6 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 { MultipleError } 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 MultipleError("Some errors on submitPoolAttestations", errors);
har777 marked this conversation as resolved.
Show resolved Hide resolved
}
},

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 MultipleError extends ApiError {
har777 marked this conversation as resolved.
Show resolved Hide resolved
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, MultipleError, 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 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;
Expand Down
Loading