Skip to content

fix: unknown sync to verify downloaded block before retain #10092

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

Draft
twoeths wants to merge 1 commit into
base: unstable
Choose a base branch
from
Draft

fix: unknown sync to verify downloaded block before retain #10092

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
43 changes: 41 additions & 2 deletions packages/beacon-node/src/sync/unknownBlock.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ import {
isPendingPayloadEnvelope,
isPendingPayloadInput,
} from "./types.js";
import {DownloadByRootError, downloadByRoot} from "./utils/downloadByRoot.js";
import {DownloadByRootError, downloadByRoot, getTerminalDownloadByRootError} from "./utils/downloadByRoot.js";
import {getAllDescendantBlocks, getUnknownAndAncestorBlocks} from "./utils/pendingBlocksTree.js";
import {getRateLimitedUntilMs} from "./utils/rateLimit.js";

Expand Down Expand Up @@ -829,6 +829,24 @@ export class BlockInputSync {
};
this.logger.verbose("Downloaded unknown block", logCtx2);

// NOT_LATER_THAN_PARENT propagated forward: a pending child that claimed this block as parent but
// does not have a later slot is provably invalid, remove it and its subtree.
for (const child of this.pendingBlocks.values()) {
if (
isPendingBlockInput(child) &&
child.blockInput.parentRootHex === pending.blockInput.blockRootHex &&
child.blockInput.slot <= blockSlot
) {
this.logger.debug("Pending block is not later than its downloaded parent", {
slot: child.blockInput.slot,
root: child.blockInput.blockRootHex,
parentSlot: blockSlot,
parentRoot: pending.blockInput.blockRootHex,
});
this.removeAndDownScoreAllDescendants(child, DroppedItemReason.invalidBlock);
}
}

if (parentInForkChoice) {
// If the direct parent is already in fork choice, let the block state machine decide if
// the next step is block import, parent payload download, or branch removal.
Expand Down Expand Up @@ -865,6 +883,20 @@ export class BlockInputSync {
this.onUnknownBlockRoot({rootHex: pending.blockInput.parentRootHex, source: BlockInputSource.byRoot});
}
} else {
const terminalError = getTerminalDownloadByRootError(res.err);
if (terminalError !== null) {
// penalize the serving peer, forwarding peers will be penalized inside removeAndDownScoreAllDescendants
this.network.reportPeer(terminalError.peerIdStr, terminalError.peerAction, terminalError.code);
this.logger.debug(
"Downloaded block by root is invalid",
{...logCtx, peer: prettyPrintPeerIdStr(terminalError.peerIdStr), peerAction: terminalError.peerAction},
res.err
);
this.metrics?.blockInputSync.downloadedBlocksError.inc();
this.removeAndDownScoreAllDescendants(block, DroppedItemReason.invalidBlock);
return;
}

if (res.err instanceof UnknownBlockRateLimitedError) {
const pendingBlock = this.pendingBlocks.get(rootHex);
if (pendingBlock) {
Expand Down Expand Up @@ -928,6 +960,9 @@ export class BlockInputSync {
// there could be finalized/head sync at the same time so we need to ignore if finalized
// see https://github.com/ChainSafe/lodestar/issues/5650
ignoreIfFinalized: true,
// proposer signature already checked in fetchAndValidateBlock() for downloaded blocks and in
// validateBeaconBlock() for gossip-retained blocks
validProposerSignature: true,
blsVerifyOnMainThread: true,
})
);
Expand Down Expand Up @@ -1532,6 +1567,10 @@ export class BlockInputSync {
if (e instanceof DownloadByRootError) {
const errorCode = e.type.code;
downloadByRootMetrics?.error.inc({code: errorCode, client: peerClient});
if (getTerminalDownloadByRootError(e) !== null) {
// block was downloaded with terminal error, we should not go with another loop
throw e;
}
excludedPeers.add(peerId);
} else if (e instanceof RequestError) {
// should look into req_resp metrics in this case
Expand Down Expand Up @@ -1629,7 +1668,7 @@ export class BlockInputSync {

for (const block of badPendingBlocks) {
//
// TODO(fulu): why is this commented out here?
// TODO(fulu): why is this commented out here? see https://github.com/ChainSafe/lodestar/issues/10091
//
// this.knownBadBlocks.add(block.blockRootHex);
// for (const peerIdStr of block.peerIdStrings) {
Expand Down
186 changes: 185 additions & 1 deletion packages/beacon-node/src/sync/utils/downloadByRoot.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,20 @@ import {
isForkPostFulu,
isForkPostGloas,
} from "@lodestar/params";
import {signedBlockToSignedHeader} from "@lodestar/state-transition";
import {BlobIndex, ColumnIndex, SignedBeaconBlock, Slot, deneb, fulu} from "@lodestar/types";
import {LodestarError, byteArrayEquals, fromHex, prettyPrintIndices, toHex, toRootHex} from "@lodestar/utils";
import {isBlockInputBlobs, isBlockInputColumns} from "../../chain/blocks/blockInput/blockInput.js";
import {BlockInputSource, IBlockInput} from "../../chain/blocks/blockInput/types.js";
import {PayloadEnvelopeInputSource} from "../../chain/blocks/payloadEnvelopeInput/index.js";
import {ChainEventEmitter} from "../../chain/emitter.js";
import {BlockGossipError} from "../../chain/errors/index.js";
import {IBeaconChain} from "../../chain/interface.js";
import {validateBlockBlobSidecars} from "../../chain/validation/blobSidecar.js";
import {verifyBlockProposerSignature} from "../../chain/validation/block.js";
import {validateFuluBlockDataColumnSidecars} from "../../chain/validation/dataColumnSidecar.js";
import {INetwork} from "../../network/interface.js";
import {PeerAction} from "../../network/peers/index.js";
import {PeerSyncMeta} from "../../network/peers/peersData.js";
import {prettyPrintPeerIdStr} from "../../network/util.js";
import {getBlobKzgCommitments} from "../../util/dataColumns.js";
Expand Down Expand Up @@ -243,6 +247,7 @@ export async function fetchByRoot({
} else {
block = await fetchAndValidateBlock({
config,
chain,
network,
peerIdStr,
blockRoot,
Expand Down Expand Up @@ -279,6 +284,7 @@ export async function fetchByRoot({
} else {
block = await fetchAndValidateBlock({
config,
chain,
network,
peerIdStr,
blockRoot,
Expand Down Expand Up @@ -326,10 +332,11 @@ export async function fetchByRoot({

export async function fetchAndValidateBlock({
config,
chain,
network,
peerIdStr,
blockRoot,
}: Omit<FetchByRootAndValidateBlockProps, "chain">): Promise<SignedBeaconBlock> {
}: FetchByRootAndValidateBlockProps): Promise<SignedBeaconBlock> {
const response = await network.sendBeaconBlocksByRoot(peerIdStr, [blockRoot]);
const block = response.at(0);
if (!block) {
Expand All @@ -351,6 +358,96 @@ export async function fetchAndValidateBlock({
"block does not match requested root"
);
}

// Chain is not null as passed by fetchBlockInput()
if (chain !== null) {
const blockSlot = block.message.slot;
const {proposerIndex} = block.message;
const rootHex = toRootHex(blockRoot);
const peer = prettyPrintPeerIdStr(peerIdStr);

const parentBlock = chain.forkChoice.getBlockHexDefaultStatus(toRootHex(block.message.parentRoot));

// [terminal] the block must be later than its parent, provable already when the parent is in fork choice
if (parentBlock !== null && parentBlock.slot >= blockSlot) {
throw new DownloadByRootError({
code: DownloadByRootErrorCode.NOT_LATER_THAN_PARENT,
peer,
peerIdStr,
slot: blockSlot,
blockRoot: rootHex,
parentSlot: parentBlock.slot,
});
}

// the common ancestor of the downloading chain and canonical chain should be at least the finalized slot and
// we should found it through forkchoice. If not, we should penalize all peers sending us this block chain
// 0 - 1 - ... - n - finalizedSlot
// \
// parent 1 - parent 2 - ... - unknownParent block
const finalizedSlot = chain.forkChoice.getFinalizedBlock().slot;
if (blockSlot <= finalizedSlot && parentBlock === null) {
throw new DownloadByRootError({
code: DownloadByRootErrorCode.WOULD_REVERT_FINALIZED_SLOT,
peer,
peerIdStr,
slot: blockSlot,
blockRoot: rootHex,
finalizedSlot,
});
}
const currentSlotWithDisparity = chain.clock.currentSlotWithGossipDisparity;
if (blockSlot > currentSlotWithDisparity) {
throw new DownloadByRootError({
code: DownloadByRootErrorCode.FUTURE_SLOT,
peer,
peerIdStr,
slot: blockSlot,
blockRoot: rootHex,
currentSlot: chain.clock.currentSlot,
});
}

if (proposerIndex >= chain.pubkeyCache.size) {
throw new DownloadByRootError({
code: DownloadByRootErrorCode.UNKNOWN_PROPOSER,
peer,
peerIdStr,
slot: blockSlot,
blockRoot: rootHex,
proposerIndex,
});
}

try {
await verifyBlockProposerSignature(chain, block, rootHex);
} catch (e) {
if (!(e instanceof BlockGossipError)) {
// BLS backend failure, not proof of an invalid block
throw e;
}
throw new DownloadByRootError({
code: DownloadByRootErrorCode.PROPOSAL_SIGNATURE_INVALID,
peer,
peerIdStr,
slot: blockSlot,
blockRoot: rootHex,
});
}

if (blockSlot > finalizedSlot) {
chain.seenBlockProposers.observeBlockRoot(
blockSlot,
proposerIndex,
rootHex,
signedBlockToSignedHeader(config, block)
);
if (!chain.seenBlockProposers.isKnown(blockSlot, proposerIndex)) {
chain.seenBlockProposers.add(blockSlot, proposerIndex, rootHex);
}
}
}

return block;
}

Expand Down Expand Up @@ -496,8 +593,56 @@ export enum DownloadByRootErrorCode {
MISSING_BLOCK_RESPONSE = "DOWNLOAD_BY_ROOT_ERROR_MISSING_BLOCK_RESPONSE",
MISSING_BLOB_RESPONSE = "DOWNLOAD_BY_ROOT_ERROR_MISSING_BLOB_RESPONSE",
MISSING_COLUMN_RESPONSE = "DOWNLOAD_BY_ROOT_ERROR_MISSING_COLUMN_RESPONSE",
WOULD_REVERT_FINALIZED_SLOT = "DOWNLOAD_BY_ROOT_ERROR_WOULD_REVERT_FINALIZED_SLOT",
FUTURE_SLOT = "DOWNLOAD_BY_ROOT_ERROR_FUTURE_SLOT",
NOT_LATER_THAN_PARENT = "DOWNLOAD_BY_ROOT_ERROR_NOT_LATER_THAN_PARENT",
UNKNOWN_PROPOSER = "DOWNLOAD_BY_ROOT_ERROR_UNKNOWN_PROPOSER",
PROPOSAL_SIGNATURE_INVALID = "DOWNLOAD_BY_ROOT_ERROR_PROPOSAL_SIGNATURE_INVALID",
Z = "DOWNLOAD_BY_ROOT_ERROR_Z",
}

type TerminalDownloadByRootErrorCode =
| DownloadByRootErrorCode.WOULD_REVERT_FINALIZED_SLOT
| DownloadByRootErrorCode.FUTURE_SLOT
| DownloadByRootErrorCode.NOT_LATER_THAN_PARENT
| DownloadByRootErrorCode.UNKNOWN_PROPOSER
| DownloadByRootErrorCode.PROPOSAL_SIGNATURE_INVALID;

/**
* PeerAction to report the serving peer with, per terminal error code. Terminal means we don't want to download
* the same block from another peer.
*
*/
const terminalErrorPeerAction: Record<TerminalDownloadByRootErrorCode, PeerAction> = {
[DownloadByRootErrorCode.WOULD_REVERT_FINALIZED_SLOT]: PeerAction.LowToleranceError,
[DownloadByRootErrorCode.FUTURE_SLOT]: PeerAction.LowToleranceError,
[DownloadByRootErrorCode.NOT_LATER_THAN_PARENT]: PeerAction.LowToleranceError,
[DownloadByRootErrorCode.UNKNOWN_PROPOSER]: PeerAction.Fatal,
[DownloadByRootErrorCode.PROPOSAL_SIGNATURE_INVALID]: PeerAction.Fatal,
};

export type TerminalDownloadByRootError = {
code: TerminalDownloadByRootErrorCode;
peerAction: PeerAction;
peerIdStr: PeerIdStr;
};

/** Returns null when the error is retryable with another peer */
export function getTerminalDownloadByRootError(e: unknown): TerminalDownloadByRootError | null {
if (!(e instanceof DownloadByRootError)) {
return null;
}
switch (e.type.code) {
case DownloadByRootErrorCode.WOULD_REVERT_FINALIZED_SLOT:
case DownloadByRootErrorCode.FUTURE_SLOT:
case DownloadByRootErrorCode.NOT_LATER_THAN_PARENT:
case DownloadByRootErrorCode.UNKNOWN_PROPOSER:
case DownloadByRootErrorCode.PROPOSAL_SIGNATURE_INVALID:
return {code: e.type.code, peerAction: terminalErrorPeerAction[e.type.code], peerIdStr: e.type.peerIdStr};
default:
return null;
}
}
export type DownloadByRootErrorType =
| {
code: DownloadByRootErrorCode.MISMATCH_BLOCK_ROOT;
Expand Down Expand Up @@ -550,6 +695,45 @@ export type DownloadByRootErrorType =
code: DownloadByRootErrorCode.MISSING_COLUMN_RESPONSE;
peer: string;
blockRoot: string;
}
| {
code: DownloadByRootErrorCode.WOULD_REVERT_FINALIZED_SLOT;
peer: string;
peerIdStr: PeerIdStr;
slot: Slot;
blockRoot: string;
finalizedSlot: Slot;
}
| {
code: DownloadByRootErrorCode.FUTURE_SLOT;
peer: string;
peerIdStr: PeerIdStr;
slot: Slot;
blockRoot: string;
currentSlot: Slot;
}
| {
code: DownloadByRootErrorCode.NOT_LATER_THAN_PARENT;
peer: string;
peerIdStr: PeerIdStr;
slot: Slot;
blockRoot: string;
parentSlot: Slot;
}
| {
code: DownloadByRootErrorCode.UNKNOWN_PROPOSER;
peer: string;
peerIdStr: PeerIdStr;
slot: Slot;
blockRoot: string;
proposerIndex: number;
}
| {
code: DownloadByRootErrorCode.PROPOSAL_SIGNATURE_INVALID;
peer: string;
peerIdStr: PeerIdStr;
slot: Slot;
blockRoot: string;
};

export class DownloadByRootError extends LodestarError<DownloadByRootErrorType> {}
Loading
Loading