Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 0 additions & 4 deletions packages/beacon-node/src/chain/errors/blockError.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,6 @@ export enum BlockErrorCode {
TRANSACTIONS_TOO_BIG = "BLOCK_ERROR_TRANSACTIONS_TOO_BIG",
/** Execution engine is unavailable, syncing, or api call errored. Peers must not be downscored on this code */
EXECUTION_ENGINE_ERROR = "BLOCK_ERROR_EXECUTION_ERROR",
/** The attestation head block is too far behind the attestation slot, causing many skip slots.
This is deemed a DoS risk */
TOO_MANY_SKIPPED_SLOTS = "TOO_MANY_SKIPPED_SLOTS",
/** The blobs are unavailable */
DATA_UNAVAILABLE = "BLOCK_ERROR_DATA_UNAVAILABLE",
/** Block contains too many kzg commitments */
Expand All @@ -89,7 +86,6 @@ export type BlockErrorType =
| {code: BlockErrorCode.FUTURE_SLOT; blockSlot: Slot; currentSlot: Slot}
| {code: BlockErrorCode.STATE_ROOT_MISMATCH}
| {code: BlockErrorCode.GENESIS_BLOCK}
| {code: BlockErrorCode.TOO_MANY_SKIPPED_SLOTS; parentSlot: Slot; blockSlot: Slot}
| {code: BlockErrorCode.WOULD_REVERT_FINALIZED_SLOT; blockSlot: Slot; finalizedSlot: Slot}
| {code: BlockErrorCode.ALREADY_KNOWN; root: RootHex}
| {code: BlockErrorCode.REPEAT_PROPOSAL; proposerIndex: ValidatorIndex}
Expand Down
28 changes: 12 additions & 16 deletions packages/beacon-node/src/chain/validation/block.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,17 @@ import {BlockErrorCode, BlockGossipError, GossipAction} from "../errors/index.js
import {IBeaconChain} from "../interface.js";
import {RegenCaller} from "../regen/index.js";

export type GossipBlockValidationResult = {
/** Number of skipped slots between the block and its parent (blockSlot - parentSlot - 1) */
skippedSlots: number;
};

export async function validateGossipBlock(
config: ChainForkConfig,
chain: IBeaconChain,
signedBlock: SignedBeaconBlock,
fork: ForkName
): Promise<void> {
): Promise<GossipBlockValidationResult> {
const block = signedBlock.message;
const blockSlot = block.slot;
const blockEpoch = computeEpochAtSlot(blockSlot);
Expand Down Expand Up @@ -109,21 +114,6 @@ export async function validateGossipBlock(
}
}

// [IGNORE] The attestation head block is too far behind the attestation slot, causing many skip slots.
// This is deemed a DoS risk because we need to get the proposerShuffling. To get the shuffling we have
// to do a bunch of epoch transitions, the longer the distance between the parent and block,
// the more we have to do. epochTransitions are expensive ~750ms, so we must limit how many a
// single bad block can trigger
// Note: Ensure this check is done before calling chain.regen.getBlockSlotStat as this is the function that does various epoch transitions.
// Note: This validation check is not part of the spec.
if (chain.opts.maxSkipSlots != null && parentBlock.slot + chain.opts.maxSkipSlots < blockSlot) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

worth pointing out that this is 32 by default, so with slotImportTolerance being 32 by default, this is very unlikely to happen as we won't be subscribed to gossip if our head is 32 slots behind clock slot, although it can eg. for a block on a different branch

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good point — that's another reason the gate was effectively dead code for our own canonical chain: gossip subscription is already gated by slotImportTolerance = SLOTS_PER_EPOCH = 32, so a block where parent.slot + 32 < block.slot under our canonical head would already be filtered before reaching validateGossipBlock. The different-branch case you flagged is the residual scenario — a peer gossips a block whose parent is on a side fork older than head - slotImportTolerance. The new lodestar_gossip_block_skipped_slots histogram should surface that distribution from mainnet; if a non-trivial tail shows up from competing-branch gossip, that data tells us whether a non-gating mitigation is warranted (peer budget, per-shuffle work cap, etc.).

throw new BlockGossipError(GossipAction.IGNORE, {
code: BlockErrorCode.TOO_MANY_SKIPPED_SLOTS,
parentSlot: parentBlock.slot,
blockSlot,
});
}

// [REJECT] The block is from a higher slot than its parent.
if (parentBlock.slot >= blockSlot) {
throw new BlockGossipError(GossipAction.REJECT, {
Expand All @@ -133,6 +123,10 @@ export async function validateGossipBlock(
});
}

// Number of skipped slots between block and parent (non-spec). Previously this gated blocks via
// maxSkipSlots; now the caller only observes it so legitimate post-skip blocks are no longer ignored.
const skippedSlots = blockSlot - parentBlock.slot - 1;

// [REJECT] The length of KZG commitments is less than or equal to the limitation defined in Consensus Layer -- i.e. validate that len(body.signed_beacon_block.message.blob_kzg_commitments) <= MAX_BLOBS_PER_BLOCK
if (isForkPostDeneb(fork) && !isForkPostGloas(fork)) {
const blobKzgCommitmentsLen = (block as deneb.BeaconBlock).body.blobKzgCommitments.length;
Expand Down Expand Up @@ -247,4 +241,6 @@ export async function validateGossipBlock(
}

chain.seenBlockProposers.add(blockSlot, proposerIndex);

return {skippedSlots};
}
6 changes: 6 additions & 0 deletions packages/beacon-node/src/metrics/metrics/lodestar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -861,6 +861,12 @@ export function createLodestarMetrics(
labelNames: ["numBlobs"],
}),

skippedSlots: register.histogram({
name: "lodestar_gossip_block_skipped_slots",
help: "Number of skipped slots between a gossip block and its parent (blockSlot - parentSlot - 1)",
buckets: [0, 1, 2, 4, 8, 16, 32],
}),

processBlockErrors: register.gauge<{error: BlockErrorCode | "NOT_BLOCK_ERROR"}>({
name: "lodestar_gossip_block_process_block_errors",
help: "Count of errors, by error type, while processing blocks",
Expand Down
11 changes: 9 additions & 2 deletions packages/beacon-node/src/network/processor/gossipHandlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ function getSequentialHandlers(modules: ValidatorFnsModules, options: GossipHand
peerIdStr,
});
try {
await validateGossipBlock(config, chain, signedBlock, fork);
const {skippedSlots} = await validateGossipBlock(config, chain, signedBlock, fork);

if (isForkPostGloas(fork)) {
chain.seenPayloadEnvelopeInputCache.add({
Expand All @@ -205,8 +205,15 @@ function getSequentialHandlers(modules: ValidatorFnsModules, options: GossipHand

metrics?.gossipBlock.gossipValidation.recvToValidation.observe(recvToValidation);
metrics?.gossipBlock.gossipValidation.validationTime.observe(validationTime);
metrics?.gossipBlock.skippedSlots.observe(skippedSlots);

logger.debug("Validated gossip block", {...blockInputMeta, ...logCtx, recvToValidation, validationTime});
logger.debug("Validated gossip block", {
...blockInputMeta,
...logCtx,
recvToValidation,
validationTime,
skippedSlots,
});

chain.emitter.emit(routes.events.EventType.blockGossip, {slot, block: blockRootHex});

Expand Down
15 changes: 1 addition & 14 deletions packages/beacon-node/test/unit/chain/validation/block.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ describe("gossip block validation", () => {
const block = ssz.deneb.BeaconBlock.defaultValue();
block.slot = clockSlot;
const signature = EMPTY_SIGNATURE;
const maxSkipSlots = 10;
const denebConfig = createChainForkConfig({
...configDef,
ALTAIR_FORK_EPOCH: 0,
Expand All @@ -43,7 +42,7 @@ describe("gossip block validation", () => {
chain.forkChoice = forkChoice;
regen = chain.regen;

(chain as any).opts = {maxSkipSlots};
(chain as any).opts = {};

verifySignature = chain.bls.verifySignatureSets;
verifySignature.mockResolvedValue(true);
Expand Down Expand Up @@ -119,18 +118,6 @@ describe("gossip block validation", () => {
);
});

it("TOO_MANY_SKIPPED_SLOTS", async () => {
// Return not known for proposed block
forkChoice.getBlockHexDefaultStatus.mockReturnValueOnce(null);
// Return parent block with 1 slot way back than maxSkipSlots
forkChoice.getBlockHexDefaultStatus.mockReturnValueOnce({slot: block.slot - (maxSkipSlots + 1)} as ProtoBlock);

await expectRejectedWithLodestarError(
validateGossipBlock(config, chain, job, ForkName.phase0),
BlockErrorCode.TOO_MANY_SKIPPED_SLOTS
);
});

it("NOT_LATER_THAN_PARENT", async () => {
// Return not known for proposed block
forkChoice.getBlockHexDefaultStatus.mockReturnValueOnce(null);
Expand Down
Loading