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.

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

feat: add kzg commitment length check when validating gossip blocks #7302

Merged
merged 4 commits into from
Dec 17, 2024
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
5 changes: 4 additions & 1 deletion packages/beacon-node/src/chain/errors/blockError.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<
Expand Down Expand Up @@ -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<BlockErrorType> {}

Expand Down
16 changes: 14 additions & 2 deletions packages/beacon-node/src/chain/validation/block.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {ChainForkConfig} from "@lodestar/config";
import {ForkName} from "@lodestar/params";
import {ForkName, isForkBlobs} from "@lodestar/params";
import {
computeStartSlotAtEpoch,
computeTimeAtSlot,
Expand All @@ -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";
Expand Down Expand Up @@ -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.
Expand Down
47 changes: 44 additions & 3 deletions packages/beacon-node/test/unit/chain/validation/block.test.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand All @@ -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;
Expand Down Expand Up @@ -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<ForkBlobs>).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);
});
});
Loading