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: ignore already known errors in API #6321

Merged
merged 4 commits into from
Jan 17, 2024
Merged

fix: ignore already known errors in API #6321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9dc811a
Ignore known attestations submitted through API
nflaig Jan 17, 2024
c05b704
Ignore known sync committee aggregate
nflaig Jan 17, 2024
3e525b0
Add log contex to ignored aggregate attestations
nflaig Jan 17, 2024
95fdf09
Fix typo in submitPoolSyncCommitteeSignatures
nflaig Jan 17, 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
24 changes: 17 additions & 7 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 @@ -8,7 +8,12 @@ import {validateApiVoluntaryExit} from "../../../../chain/validation/voluntaryEx
import {validateApiBlsToExecutionChange} from "../../../../chain/validation/blsToExecutionChange.js";
import {validateApiSyncCommittee} from "../../../../chain/validation/syncCommittee.js";
import {ApiModules} from "../../types.js";
import {AttestationError, GossipAction, SyncCommitteeError} from "../../../../chain/errors/index.js";
import {
AttestationError,
AttestationErrorCode,
GossipAction,
SyncCommitteeError,
} from "../../../../chain/errors/index.js";
import {validateGossipFnRetryUnknownRoot} from "../../../../network/processor/gossipHandlers.js";

export function getBeaconPoolApi({
Expand Down Expand Up @@ -77,12 +82,17 @@ export function getBeaconPoolApi({
const sentPeers = await network.publishBeaconAttestation(attestation, subnet);
metrics?.onPoolSubmitUnaggregatedAttestation(seenTimestampSec, indexedAttestation, subnet, sentPeers);
} catch (e) {
const logCtx = {slot: attestation.data.slot, index: attestation.data.index};

if (e instanceof AttestationError && e.type.code === AttestationErrorCode.ATTESTATION_ALREADY_KNOWN) {
logger.debug("Ignoring known attestation", logCtx);
// Attestations might already be published by another node as part of a fallback setup or DVT cluster
// and can reach our node by gossip before the api. The error can be ignored and should not result in a 500 response.
return;
}

errors.push(e as Error);
logger.error(
`Error on submitPoolAttestations [${i}]`,
{slot: attestation.data.slot, index: attestation.data.index},
e as Error
);
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 Down Expand Up @@ -224,7 +234,7 @@ export function getBeaconPoolApi({
);

if (errors.length > 1) {
throw Error("Multiple errors on publishAggregateAndProofs\n" + errors.map((e) => e.message).join("\n"));
throw Error("Multiple errors on submitPoolSyncCommitteeSignatures\n" + errors.map((e) => e.message).join("\n"));
Copy link
Member Author

Choose a reason for hiding this comment

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

This just fixes a typo but it is worth highlighting that we don't need to handle already known errors in submitPoolSyncCommitteeSignatures as the api only verifies the signature

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

Lines 184 to 186 in d7ca290

// Verify signature only, all other data is very likely to be correct, since the `signature` object is created by this node.
// Worst case if `signature` is not valid, gossip peers will drop it and slightly downscore us.
await validateApiSyncCommittee(chain, state, signature);

} else if (errors.length === 1) {
throw errors[0];
}
Expand Down
46 changes: 28 additions & 18 deletions packages/beacon-node/src/api/impl/validator/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,13 @@ import {
} from "@lodestar/types";
import {ExecutionStatus} from "@lodestar/fork-choice";
import {toHex, racePromisesWithCutoff, RaceEvent} from "@lodestar/utils";
import {AttestationError, AttestationErrorCode, GossipAction, SyncCommitteeError} from "../../../chain/errors/index.js";
import {
AttestationError,
AttestationErrorCode,
GossipAction,
SyncCommitteeError,
SyncCommitteeErrorCode,
} from "../../../chain/errors/index.js";
import {validateApiAggregateAndProof} from "../../../chain/validation/index.js";
import {ZERO_HASH} from "../../../constants/index.js";
import {SyncState} from "../../../sync/index.js";
Expand Down Expand Up @@ -1073,20 +1079,18 @@ export function getValidatorApi({
const sentPeers = await network.publishBeaconAggregateAndProof(signedAggregateAndProof);
metrics?.onPoolSubmitAggregatedAttestation(seenTimestampSec, indexedAttestation, sentPeers);
} catch (e) {
const logCtx = {
slot: signedAggregateAndProof.message.aggregate.data.slot,
index: signedAggregateAndProof.message.aggregate.data.index,
};

if (e instanceof AttestationError && e.type.code === AttestationErrorCode.AGGREGATOR_ALREADY_KNOWN) {
logger.debug("Ignoring known signedAggregateAndProof");
logger.debug("Ignoring known signedAggregateAndProof", logCtx);
return; // Ok to submit the same aggregate twice
}

errors.push(e as Error);
logger.error(
`Error on publishAggregateAndProofs [${i}]`,
{
slot: signedAggregateAndProof.message.aggregate.data.slot,
index: signedAggregateAndProof.message.aggregate.data.index,
},
e as Error
);
logger.error(`Error on publishAggregateAndProofs [${i}]`, logCtx, e as Error);
if (e instanceof AttestationError && e.action === GossipAction.REJECT) {
chain.persistInvalidSszValue(ssz.phase0.SignedAggregateAndProof, signedAggregateAndProof, "api_reject");
}
Expand Down Expand Up @@ -1128,15 +1132,21 @@ export function getValidatorApi({
);
await network.publishContributionAndProof(contributionAndProof);
} catch (e) {
const logCtx = {
slot: contributionAndProof.message.contribution.slot,
subcommitteeIndex: contributionAndProof.message.contribution.subcommitteeIndex,
};

if (
e instanceof SyncCommitteeError &&
e.type.code === SyncCommitteeErrorCode.SYNC_COMMITTEE_AGGREGATOR_ALREADY_KNOWN
) {
logger.debug("Ignoring known contributionAndProof", logCtx);
Copy link
Member Author

@nflaig nflaig Jan 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This error was already ignored previously (added in #2598) but it got removed again by #2816 as the PR changed the error behavior.

In #4019 it was introduced again that the error is thrown during validation but the handling on the API was not re-added.

The error was recently observed (#6021 (comment)) and should be ignored again.

return; // Ok to submit the same aggregate twice
}

errors.push(e as Error);
logger.error(
`Error on publishContributionAndProofs [${i}]`,
{
slot: contributionAndProof.message.contribution.slot,
subcommitteeIndex: contributionAndProof.message.contribution.subcommitteeIndex,
},
e as Error
);
logger.error(`Error on publishContributionAndProofs [${i}]`, logCtx, e as Error);
if (e instanceof SyncCommitteeError && e.action === GossipAction.REJECT) {
chain.persistInvalidSszValue(ssz.altair.SignedContributionAndProof, contributionAndProof, "api_reject");
}
Expand Down
Loading