fix: make multiple api errors spec compliant #6479

har777 · 2024-02-25T16:51:01Z

Motivation

Api's returning multiple errors are not formatted according to the spec. This PR resolves that.

Description

Add new IndexedError type to handle multiple errors
Update server to handle possible IndexedError
Update httpClient to handle possible IndexedError
Update submitPoolAttestations to use IndexedError
Update submitPoolBlsToExecutionChange to use IndexedError
Update submitPoolSyncCommitteeSignatures to use IndexedError
e2e tests checking response

codecov · 2024-02-25T17:01:20Z

Codecov Report

Merging #6479 (0acfc71) into unstable (06210ef) will increase coverage by 0.00%.
Report is 12 commits behind head on unstable.
The diff coverage is 100.00%.

Additional details and impacted files

@@            Coverage Diff            @@
##           unstable    #6479   +/-   ##
=========================================
  Coverage     61.72%   61.72%           
=========================================
  Files           555      555           
  Lines         58204    58206    +2     
  Branches       1839     1843    +4     
=========================================
+ Hits          35925    35928    +3     
+ Misses        22240    22239    -1     
  Partials         39       39

packages/beacon-node/src/api/impl/beacon/pool/index.ts

packages/beacon-node/src/api/impl/errors.ts

har777 · 2024-02-26T05:51:25Z

I'm thinking we can add something like an extra field to an Error object. And the handler checks and destructures the value of that key if it exists into the response object. That way if any error wants to return extra data, it can simply add an extra key to its Error object.

nflaig · 2024-02-26T07:00:03Z

I'm thinking we can add something like an extra field to an Error object.

I think more explicit solution is better as we don't have that many cases

ApiError
IndexedError
Ajv error (err.validation)
Any other Error

We could also consider handling errors thrown by ssz types e.g. JSON expected key ${jsonKey} is undefined in container.ts but unless ssz throws a specific validation error this will be hard to handle but definitely something we could do as well.

Also I noticed my suggestion to validate objects more strictly at the schema (ajv) level instead of just doing Schema.Object or Schema.ObjectArray is not ideal because we will be supporting ssz payloads (binary / bytes) for all APIs and in that case the validation has to be done by the ssz types.

…PoolAttestations

har777 · 2024-03-03T14:25:19Z

We could also consider handling errors thrown by ssz types e.g. JSON expected key ${jsonKey} is undefined in container.ts but unless ssz throws a specific validation error this will be hard to handle but definitely something we could do as well.

Yeah we should handle this in server.setErrorHandler. I think it's better to fix this + the error response keys together as part of tackling #6480 in a new PR.

nflaig · 2024-03-04T14:21:39Z

packages/beacon-node/src/api/impl/beacon/pool/index.ts

@@ -91,7 +92,7 @@ export function getBeaconPoolApi({
 return;
 }

- errors.push(e as Error);
+ errors.push({index: i, error: e as Error});


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

nflaig · 2024-03-04T14:27:31Z

packages/beacon-node/src/api/impl/beacon/pool/index.ts

- } else if (errors.length === 1) {
- throw errors[0];
+ if (errors.length > 0) {
+ throw new IndexedError("Some errors submitting attestations", errors);


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

nflaig · 2024-03-04T14:32:12Z

packages/beacon-node/src/api/impl/beacon/pool/index.ts

- } else if (errors.length === 1) {
- throw errors[0];
+ if (errors.length > 0) {
+ throw new IndexedError("Some errors submitting BLS to execution change", errors);


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

nflaig · 2024-03-04T14:34:02Z

packages/beacon-node/src/api/impl/errors.ts

+ super(400, message);
+
+ const failures = [];
+ for (const {index, error} of errors) {


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.

nflaig · 2024-03-04T14:47:53Z

packages/beacon-node/test/e2e/api/impl/beacon/pool/endpoints.test.ts

+ message: "Some errors submitting attestations",
+ failures: [{index: 0, message: "ATTESTATION_ERROR_NOT_EXACTLY_ONE_AGGREGATION_BIT_SET"}],
+ };
+ const expectedErrorMessage = `Bad Request: ${JSON.stringify(expectedErrorBody)}`;


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.

nflaig · 2024-03-04T14:49:20Z

packages/api/src/utils/client/httpClient.ts

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


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

nflaig · 2024-03-04T14:53:40Z

packages/beacon-node/src/api/rest/base.ts

@@ -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


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

nflaig · 2024-03-04T15:14:22Z

packages/beacon-node/test/e2e/api/impl/beacon/pool/endpoints.test.ts

+ });
+
+ describe("submitPoolAttestations", () => {
+ it("should return correctly formatted errors responses", async () => {


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

nflaig · 2024-03-04T15:16:28Z

packages/beacon-node/test/e2e/api/impl/beacon/pool/endpoints.test.ts

+ const expectedErrorBody = {
+ code: 400,
+ message: "Some errors submitting sync committee signatures",
+ failures: [{index: 0, message: "Empty SyncCommitteeCache"}],


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.

nflaig · 2024-03-04T15:18:19Z

packages/beacon-node/test/e2e/api/impl/beacon/pool/endpoints.test.ts

+
+ describe("submitPoolAttestations", () => {
+ it("should return correctly formatted errors responses", async () => {
+ const attestations = [ssz.phase0.Attestation.defaultValue()];


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

Add new MultipleError and update submitPoolAttestations to use it

55f4cd4

nflaig reviewed Feb 25, 2024

View reviewed changes

packages/beacon-node/src/api/impl/beacon/pool/index.ts Outdated Show resolved Hide resolved

packages/beacon-node/src/api/impl/errors.ts Outdated Show resolved Hide resolved

har777 added 5 commits February 26, 2024 14:20

Rename MultipleError to IndexedError + change error message in submit…

16611c8

…PoolAttestations

Update getErrorMessage to handle failures key

a86e85a

Update submitPoolBlsToExecutionChange to use IndexedError

0ad7c21

Update submitPoolSyncCommitteeSignatures to use IndexedError

e32fb50

Add e2e api tests checking correct handling of IndexedError

0acfc71

har777 marked this pull request as ready for review March 3, 2024 14:25

har777 requested a review from a team as a code owner March 3, 2024 14:25

nflaig reviewed Mar 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: make multiple api errors spec compliant #6479

fix: make multiple api errors spec compliant #6479

har777 commented Feb 25, 2024 •

edited

codecov bot commented Feb 25, 2024 •

edited

har777 commented Feb 26, 2024

nflaig commented Feb 26, 2024

har777 commented Mar 3, 2024

nflaig Mar 4, 2024

nflaig Mar 4, 2024

nflaig Mar 4, 2024

nflaig Mar 4, 2024

nflaig Mar 4, 2024

nflaig Mar 4, 2024

nflaig Mar 4, 2024

nflaig Mar 4, 2024

nflaig Mar 4, 2024

nflaig Mar 4, 2024

	errors.push({index: i, error: e as Error});
	failures.push({index: i, message: (e as Error).message});

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

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

fix: make multiple api errors spec compliant #6479

Are you sure you want to change the base?

fix: make multiple api errors spec compliant #6479

Conversation

har777 commented Feb 25, 2024 • edited

codecov bot commented Feb 25, 2024 • edited

Codecov Report

har777 commented Feb 26, 2024

nflaig commented Feb 26, 2024

har777 commented Mar 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

har777 commented Feb 25, 2024 •

edited

codecov bot commented Feb 25, 2024 •

edited