From 7cd296b0466f75136cbf544aaec5ce000c89a8ed Mon Sep 17 00:00:00 2001 From: NC <17676176+ensi321@users.noreply.github.com> Date: Mon, 16 Dec 2024 19:21:22 -0800 Subject: [PATCH] feat: add kzg commitment length check when validating gossip blocks (#7302) --- .../src/chain/errors/blockError.ts | 5 +- .../beacon-node/src/chain/validation/block.ts | 16 ++++++- .../test/unit/chain/validation/block.test.ts | 47 +++++++++++++++++-- 3 files changed, 62 insertions(+), 6 deletions(-) diff --git a/packages/beacon-node/src/chain/errors/blockError.ts b/packages/beacon-node/src/chain/errors/blockError.ts index bc5dd6c33956..1dc5b08ccc20 100644 --- a/packages/beacon-node/src/chain/errors/blockError.ts +++ b/packages/beacon-node/src/chain/errors/blockError.ts @@ -64,6 +64,8 @@ export enum BlockErrorCode { TOO_MANY_SKIPPED_SLOTS = "TOO_MANY_SKIPPED_SLOTS", /** The blobs are unavailable */ DATA_UNAVAILABLE = "BLOCK_ERROR_DATA_UNAVAILABLE", + /** Block contains too many kzg commitments */ + TOO_MANY_KZG_COMMITMENTS = "BLOCK_ERROR_TOO_MANY_KZG_COMMITMENTS", } type ExecutionErrorStatus = Exclude< @@ -105,7 +107,8 @@ export type BlockErrorType = | {code: BlockErrorCode.SAME_PARENT_HASH; blockHash: RootHex} | {code: BlockErrorCode.TRANSACTIONS_TOO_BIG; size: number; max: number} | {code: BlockErrorCode.EXECUTION_ENGINE_ERROR; execStatus: ExecutionErrorStatus; errorMessage: string} - | {code: BlockErrorCode.DATA_UNAVAILABLE}; + | {code: BlockErrorCode.DATA_UNAVAILABLE} + | {code: BlockErrorCode.TOO_MANY_KZG_COMMITMENTS; blobKzgCommitmentsLen: number; commitmentLimit: number}; export class BlockGossipError extends GossipActionError {} diff --git a/packages/beacon-node/src/chain/validation/block.ts b/packages/beacon-node/src/chain/validation/block.ts index 1b5251f77809..b2623aa4f79d 100644 --- a/packages/beacon-node/src/chain/validation/block.ts +++ b/packages/beacon-node/src/chain/validation/block.ts @@ -1,5 +1,5 @@ import {ChainForkConfig} from "@lodestar/config"; -import {ForkName} from "@lodestar/params"; +import {ForkName, isForkBlobs} from "@lodestar/params"; import { computeStartSlotAtEpoch, computeTimeAtSlot, @@ -8,7 +8,7 @@ import { isExecutionEnabled, isExecutionStateType, } from "@lodestar/state-transition"; -import {SignedBeaconBlock} from "@lodestar/types"; +import {SignedBeaconBlock, deneb} from "@lodestar/types"; import {sleep, toRootHex} from "@lodestar/utils"; import {MAXIMUM_GOSSIP_CLOCK_DISPARITY} from "../../constants/index.js"; import {BlockErrorCode, BlockGossipError, GossipAction} from "../errors/index.js"; @@ -110,6 +110,18 @@ export async function validateGossipBlock( }); } + // [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 (isForkBlobs(fork)) { + const blobKzgCommitmentsLen = (block as deneb.BeaconBlock).body.blobKzgCommitments.length; + if (blobKzgCommitmentsLen > chain.config.MAX_BLOBS_PER_BLOCK) { + throw new BlockGossipError(GossipAction.REJECT, { + code: BlockErrorCode.TOO_MANY_KZG_COMMITMENTS, + blobKzgCommitmentsLen, + commitmentLimit: chain.config.MAX_BLOBS_PER_BLOCK, + }); + } + } + // use getPreState to reload state if needed. It also checks for whether the current finalized checkpoint is an ancestor of the block. // As a result, we throw an IGNORE (whereas the spec says we should REJECT for this scenario). // this is something we should change this in the future to make the code airtight to the spec. diff --git a/packages/beacon-node/test/unit/chain/validation/block.test.ts b/packages/beacon-node/test/unit/chain/validation/block.test.ts index 4b236181b038..5e3faf794453 100644 --- a/packages/beacon-node/test/unit/chain/validation/block.test.ts +++ b/packages/beacon-node/test/unit/chain/validation/block.test.ts @@ -1,6 +1,6 @@ import {config} from "@lodestar/config/default"; import {ProtoBlock} from "@lodestar/fork-choice"; -import {ForkName} from "@lodestar/params"; +import {ForkBlobs, ForkName} from "@lodestar/params"; import {SignedBeaconBlock, ssz} from "@lodestar/types"; import {Mock, Mocked, beforeEach, describe, it, vi} from "vitest"; import {BlockErrorCode} from "../../../../src/chain/errors/index.js"; @@ -20,12 +20,15 @@ describe("gossip block validation", () => { let job: SignedBeaconBlock; const proposerIndex = 0; const clockSlot = 32; - const block = ssz.phase0.BeaconBlock.defaultValue(); + const block = ssz.deneb.BeaconBlock.defaultValue(); block.slot = clockSlot; const signature = EMPTY_SIGNATURE; const maxSkipSlots = 10; beforeEach(() => { + // Fill up with kzg commitments + block.body.blobKzgCommitments = Array.from({length: config.MAX_BLOBS_PER_BLOCK}, () => new Uint8Array([0])); + chain = getMockedBeaconChain(); vi.spyOn(chain.clock, "currentSlotWithGossipDisparity", "get").mockReturnValue(clockSlot); forkChoice = chain.forkChoice; @@ -184,9 +187,47 @@ describe("gossip block validation", () => { regen.getPreState.mockResolvedValue(state); // BLS signature verifier returns valid verifySignature.mockResolvedValue(true); - // Force proposer shuffling cache to return wrong value + // Force proposer shuffling cache to return correct value vi.spyOn(state.epochCtx, "getBeaconProposer").mockReturnValue(proposerIndex); await validateGossipBlock(config, chain, job, ForkName.phase0); }); + + it("deneb - TOO_MANY_KZG_COMMITMENTS", async () => { + // Return not known for proposed block + forkChoice.getBlockHex.mockReturnValueOnce(null); + // Returned parent block is latter than proposed block + forkChoice.getBlockHex.mockReturnValueOnce({slot: clockSlot - 1} as ProtoBlock); + // Regen returns some state + const state = generateCachedState(); + regen.getPreState.mockResolvedValue(state); + // BLS signature verifier returns valid + verifySignature.mockResolvedValue(true); + // Force proposer shuffling cache to return correct value + vi.spyOn(state.epochCtx, "getBeaconProposer").mockReturnValue(proposerIndex + 1); + // Add one extra kzg commitment in the block so it goes over the limit + (job as SignedBeaconBlock).message.body.blobKzgCommitments.push(new Uint8Array([0])); + + await expectRejectedWithLodestarError( + validateGossipBlock(config, chain, job, ForkName.deneb), + BlockErrorCode.TOO_MANY_KZG_COMMITMENTS + ); + }); + + it("deneb - valid", async () => { + // Return not known for proposed block + forkChoice.getBlockHex.mockReturnValueOnce(null); + // Returned parent block is latter than proposed block + forkChoice.getBlockHex.mockReturnValueOnce({slot: clockSlot - 1} as ProtoBlock); + // Regen returns some state + const state = generateCachedState(); + regen.getPreState.mockResolvedValue(state); + // BLS signature verifier returns valid + verifySignature.mockResolvedValue(true); + // Force proposer shuffling cache to return correct value + vi.spyOn(state.epochCtx, "getBeaconProposer").mockReturnValue(proposerIndex); + // Keep number of kzg commitments as is so it stays within the limit + + await validateGossipBlock(config, chain, job, ForkName.deneb); + }); });