diff --git a/packages/beacon-node/src/api/impl/validator/index.ts b/packages/beacon-node/src/api/impl/validator/index.ts index 7a9ed8987771..a4f5dccbb561 100644 --- a/packages/beacon-node/src/api/impl/validator/index.ts +++ b/packages/beacon-node/src/api/impl/validator/index.ts @@ -323,10 +323,20 @@ export function getValidatorApi({ { skipHeadChecksAndUpdate, commonBlockBody, - }: Omit & { - skipHeadChecksAndUpdate?: boolean; - commonBlockBody?: CommonBlockBody; - } = {} + parentBlockRoot: inParentBlockRoot, + }: Omit & + ( + | { + skipHeadChecksAndUpdate: true; + commonBlockBody: CommonBlockBody; + parentBlockRoot: Root; + } + | { + skipHeadChecksAndUpdate?: false | undefined; + commonBlockBody?: undefined; + parentBlockRoot?: undefined; + } + ) = {} ): Promise { const version = config.getForkName(slot); if (!isForkExecution(version)) { @@ -344,6 +354,7 @@ export function getValidatorApi({ throw Error("Execution builder disabled"); } + let parentBlockRoot: Root; if (skipHeadChecksAndUpdate !== true) { notWhileSyncing(); await waitForSlot(slot); // Must never request for a future slot > currentSlot @@ -352,7 +363,9 @@ export function getValidatorApi({ // forkChoice.updateTime() might have already been called by the onSlot clock // handler, in which case this should just return. chain.forkChoice.updateTime(slot); - chain.recomputeForkChoiceHead(); + parentBlockRoot = fromHexString(chain.getProposerHead(slot).blockRoot); + } else { + parentBlockRoot = inParentBlockRoot; } let timer; @@ -360,6 +373,7 @@ export function getValidatorApi({ timer = metrics?.blockProductionTime.startTimer(); const {block, executionPayloadValue, consensusBlockValue} = await chain.produceBlindedBlock({ slot, + parentBlockRoot, randaoReveal, graffiti: toGraffitiBuffer(graffiti || ""), commonBlockBody, @@ -393,14 +407,21 @@ export function getValidatorApi({ strictFeeRecipientCheck, skipHeadChecksAndUpdate, commonBlockBody, - }: Omit & { - skipHeadChecksAndUpdate?: boolean; - commonBlockBody?: CommonBlockBody; - } = {} + parentBlockRoot: inParentBlockRoot, + }: Omit & + ( + | { + skipHeadChecksAndUpdate: true; + commonBlockBody: CommonBlockBody; + parentBlockRoot: Root; + } + | {skipHeadChecksAndUpdate?: false | undefined; commonBlockBody?: undefined; parentBlockRoot?: undefined} + ) = {} ): Promise { const source = ProducedBlockSource.engine; metrics?.blockProductionRequests.inc({source}); + let parentBlockRoot: Root; if (skipHeadChecksAndUpdate !== true) { notWhileSyncing(); await waitForSlot(slot); // Must never request for a future slot > currentSlot @@ -409,7 +430,9 @@ export function getValidatorApi({ // forkChoice.updateTime() might have already been called by the onSlot clock // handler, in which case this should just return. chain.forkChoice.updateTime(slot); - chain.recomputeForkChoiceHead(); + parentBlockRoot = fromHexString(chain.getProposerHead(slot).blockRoot); + } else { + parentBlockRoot = inParentBlockRoot; } let timer; @@ -417,6 +440,7 @@ export function getValidatorApi({ timer = metrics?.blockProductionTime.startTimer(); const {block, executionPayloadValue, consensusBlockValue, shouldOverrideBuilder} = await chain.produceBlock({ slot, + parentBlockRoot, randaoReveal, graffiti: toGraffitiBuffer(graffiti || ""), feeRecipient, @@ -528,13 +552,13 @@ export function getValidatorApi({ }; logger.verbose("Assembling block with produceEngineOrBuilderBlock", loggerContext); - const proposerHead = chain.getProposerHead(slot); + const parentBlockRoot = fromHexString(chain.getProposerHead(slot).blockRoot); const commonBlockBody = await chain.produceCommonBlockBody({ slot, + parentBlockRoot, randaoReveal, graffiti: toGraffitiBuffer(graffiti || ""), - proposerHead, }); logger.debug("Produced common block body", loggerContext); @@ -557,6 +581,7 @@ export function getValidatorApi({ // skip checking and recomputing head in these individual produce calls skipHeadChecksAndUpdate: true, commonBlockBody, + parentBlockRoot, }) : Promise.reject(new Error("Builder disabled")); @@ -567,6 +592,7 @@ export function getValidatorApi({ // skip checking and recomputing head in these individual produce calls skipHeadChecksAndUpdate: true, commonBlockBody, + parentBlockRoot, }).then((engineBlock) => { // Once the engine returns a block, in the event of either: // - suspected builder censorship diff --git a/packages/beacon-node/src/chain/chain.ts b/packages/beacon-node/src/chain/chain.ts index fca0144659e1..ecb3ba31abc1 100644 --- a/packages/beacon-node/src/chain/chain.ts +++ b/packages/beacon-node/src/chain/chain.ts @@ -483,22 +483,19 @@ export class BeaconChain implements IBeaconChain { } async produceCommonBlockBody(blockAttributes: BlockAttributes): Promise { - const {slot} = blockAttributes; - const head = blockAttributes.proposerHead ?? this.forkChoice.getHead(); + const {slot, parentBlockRoot} = blockAttributes; const state = await this.regen.getBlockSlotState( - head.blockRoot, + toHexString(parentBlockRoot), slot, {dontTransferCache: true}, RegenCaller.produceBlock ); - const parentBlockRoot = fromHexString(head.blockRoot); // TODO: To avoid breaking changes for metric define this attribute const blockType = BlockType.Full; return produceCommonBlockBody.call(this, blockType, state, { ...blockAttributes, - parentBlockRoot, parentSlot: slot - 1, }); } @@ -522,21 +519,26 @@ export class BeaconChain implements IBeaconChain { async produceBlockWrapper( blockType: T, - {randaoReveal, graffiti, slot, feeRecipient, commonBlockBody}: BlockAttributes & {commonBlockBody?: CommonBlockBody} + { + randaoReveal, + graffiti, + slot, + feeRecipient, + commonBlockBody, + parentBlockRoot, + }: BlockAttributes & {commonBlockBody?: CommonBlockBody} ): Promise<{ block: AssembledBlockType; executionPayloadValue: Wei; consensusBlockValue: Wei; shouldOverrideBuilder?: boolean; }> { - const head = this.forkChoice.getHead(); const state = await this.regen.getBlockSlotState( - head.blockRoot, + toHexString(parentBlockRoot), slot, {dontTransferCache: true}, RegenCaller.produceBlock ); - const parentBlockRoot = fromHexString(head.blockRoot); const proposerIndex = state.epochCtx.getBeaconProposer(slot); const proposerPubKey = state.epochCtx.index2pubkey[proposerIndex].toBytes(); diff --git a/packages/beacon-node/src/chain/forkChoice/index.ts b/packages/beacon-node/src/chain/forkChoice/index.ts index 2c4203a79b75..739bb7686097 100644 --- a/packages/beacon-node/src/chain/forkChoice/index.ts +++ b/packages/beacon-node/src/chain/forkChoice/index.ts @@ -7,7 +7,7 @@ import { ForkChoiceStore, ExecutionStatus, JustifiedBalancesGetter, - ForkChoiceOpts, + ForkChoiceOpts as RawForkChoiceOpts, } from "@lodestar/fork-choice"; import { CachedBeaconStateAllForks, @@ -22,7 +22,10 @@ import {ChainEventEmitter} from "../emitter.js"; import {ChainEvent} from "../emitter.js"; import {GENESIS_SLOT} from "../../constants/index.js"; -export type {ForkChoiceOpts}; +export type ForkChoiceOpts = RawForkChoiceOpts & { + // for testing only + forkchoiceConstructor?: typeof ForkChoice; +}; /** * Fork Choice extended with a ChainEventEmitter @@ -49,7 +52,11 @@ export function initializeForkChoice( const justifiedBalances = getEffectiveBalanceIncrementsZeroInactive(state); - return new ForkChoice( + // forkchoiceConstructor is only used for some test cases + // production code use ForkChoice constructor directly + const forkchoiceConstructor = opts.forkchoiceConstructor ?? ForkChoice; + + return new forkchoiceConstructor( config, new ForkChoiceStore( diff --git a/packages/beacon-node/src/chain/produceBlock/produceBlockBody.ts b/packages/beacon-node/src/chain/produceBlock/produceBlockBody.ts index d73afa24451c..fcef8452a21e 100644 --- a/packages/beacon-node/src/chain/produceBlock/produceBlockBody.ts +++ b/packages/beacon-node/src/chain/produceBlock/produceBlockBody.ts @@ -28,7 +28,6 @@ import {ChainForkConfig} from "@lodestar/config"; import {ForkSeq, ForkExecution, isForkExecution} from "@lodestar/params"; import {toHex, sleep, Logger} from "@lodestar/utils"; -import {ProtoBlock} from "@lodestar/fork-choice"; import type {BeaconChain} from "../chain.js"; import {PayloadId, IExecutionEngine, IExecutionBuilder, PayloadAttributes} from "../../execution/index.js"; import {ZERO_HASH, ZERO_HASH_HEX} from "../../constants/index.js"; @@ -65,8 +64,8 @@ export type BlockAttributes = { randaoReveal: BLSSignature; graffiti: Bytes32; slot: Slot; + parentBlockRoot: Root; feeRecipient?: string; - proposerHead?: ProtoBlock; }; export enum BlockType { @@ -97,7 +96,6 @@ export async function produceBlockBody( currentState: CachedBeaconStateAllForks, blockAttr: BlockAttributes & { parentSlot: Slot; - parentBlockRoot: Root; proposerIndex: ValidatorIndex; proposerPubKey: BLSPubkey; commonBlockBody?: CommonBlockBody; @@ -582,7 +580,6 @@ export async function produceCommonBlockBody( parentBlockRoot, }: BlockAttributes & { parentSlot: Slot; - parentBlockRoot: Root; } ): Promise { const stepsMetrics = diff --git a/packages/beacon-node/test/e2e/chain/proposerBoostReorg.test.ts b/packages/beacon-node/test/e2e/chain/proposerBoostReorg.test.ts new file mode 100644 index 000000000000..a2257bd7f1ca --- /dev/null +++ b/packages/beacon-node/test/e2e/chain/proposerBoostReorg.test.ts @@ -0,0 +1,133 @@ +import {describe, it, afterEach, expect} from "vitest"; +import {SLOTS_PER_EPOCH} from "@lodestar/params"; +import {TimestampFormatCode} from "@lodestar/logger"; +import {ChainConfig} from "@lodestar/config"; +import {RootHex, Slot} from "@lodestar/types"; +import {routes} from "@lodestar/api"; +import {toHexString} from "@lodestar/utils"; +import {LogLevel, TestLoggerOpts, testLogger} from "../../utils/logger.js"; +import {getDevBeaconNode} from "../../utils/node/beacon.js"; +import {TimelinessForkChoice} from "../../mocks/fork-choice/timeliness.js"; +import {getAndInitDevValidators} from "../../utils/node/validator.js"; +import {waitForEvent} from "../../utils/events/resolver.js"; +import {ReorgEventData} from "../../../src/chain/emitter.js"; + +describe( + "proposer boost reorg", + function () { + const validatorCount = 8; + const testParams: Pick = + { + // eslint-disable-next-line @typescript-eslint/naming-convention + SECONDS_PER_SLOT: 2, + // need this to make block `reorgSlot - 1` strong enough + // eslint-disable-next-line @typescript-eslint/naming-convention + REORG_PARENT_WEIGHT_THRESHOLD: 80, + // need this to make block `reorgSlot + 1` to become the head + // eslint-disable-next-line @typescript-eslint/naming-convention + PROPOSER_SCORE_BOOST: 120, + }; + + const afterEachCallbacks: (() => Promise | void)[] = []; + afterEach(async () => { + while (afterEachCallbacks.length > 0) { + const callback = afterEachCallbacks.pop(); + if (callback) await callback(); + } + }); + + const reorgSlot = 10; + const proposerBoostReorgEnabled = true; + /** + * reorgSlot + * / + * reorgSlot - 1 ------------ reorgSlot + 1 + */ + it(`should reorg a late block at slot ${reorgSlot}`, async () => { + // the node needs time to transpile/initialize bls worker threads + const genesisSlotsDelay = 7; + const genesisTime = Math.floor(Date.now() / 1000) + genesisSlotsDelay * testParams.SECONDS_PER_SLOT; + const testLoggerOpts: TestLoggerOpts = { + level: LogLevel.debug, + timestampFormat: { + format: TimestampFormatCode.EpochSlot, + genesisTime, + slotsPerEpoch: SLOTS_PER_EPOCH, + secondsPerSlot: testParams.SECONDS_PER_SLOT, + }, + }; + const logger = testLogger("BeaconNode", testLoggerOpts); + const bn = await getDevBeaconNode({ + params: testParams, + options: { + sync: {isSingleNode: true}, + network: {allowPublishToZeroPeers: true, mdns: true, useWorker: false}, + // run the first bn with ReorgedForkChoice, no nHistoricalStates flag so it does not have to reload + chain: { + blsVerifyAllMainThread: true, + forkchoiceConstructor: TimelinessForkChoice, + proposerBoostEnabled: true, + proposerBoostReorgEnabled, + }, + }, + validatorCount, + genesisTime, + logger, + }); + + (bn.chain.forkChoice as TimelinessForkChoice).lateSlot = reorgSlot; + afterEachCallbacks.push(async () => bn.close()); + const {validators} = await getAndInitDevValidators({ + node: bn, + logPrefix: "vc-0", + validatorsPerClient: validatorCount, + validatorClientCount: 1, + startIndex: 0, + useRestApi: false, + testLoggerOpts, + }); + afterEachCallbacks.push(() => Promise.all(validators.map((v) => v.close()))); + + const commonAncestor = await waitForEvent<{slot: Slot; block: RootHex}>( + bn.chain.emitter, + routes.events.EventType.head, + 240000, + ({slot}) => slot === reorgSlot - 1 + ); + // reorgSlot + // / + // commonAncestor ------------ newBlock + const commonAncestorRoot = commonAncestor.block; + const reorgBlockEventData = await waitForEvent<{slot: Slot; block: RootHex}>( + bn.chain.emitter, + routes.events.EventType.head, + 240000, + ({slot}) => slot === reorgSlot + ); + const reorgBlockRoot = reorgBlockEventData.block; + const [newBlockEventData, reorgEventData] = await Promise.all([ + waitForEvent<{slot: Slot; block: RootHex}>( + bn.chain.emitter, + routes.events.EventType.block, + 240000, + ({slot}) => slot === reorgSlot + 1 + ), + waitForEvent(bn.chain.emitter, routes.events.EventType.chainReorg, 240000), + ]); + expect(reorgEventData.slot).toEqual(reorgSlot + 1); + const newBlock = await bn.chain.getBlockByRoot(newBlockEventData.block); + if (newBlock == null) { + throw Error(`Block ${reorgSlot + 1} not found`); + } + expect(reorgEventData.oldHeadBlock).toEqual(reorgBlockRoot); + expect(reorgEventData.newHeadBlock).toEqual(newBlockEventData.block); + expect(reorgEventData.depth).toEqual(2); + expect(toHexString(newBlock?.block.message.parentRoot)).toEqual(commonAncestorRoot); + logger.info("New block", { + slot: newBlock.block.message.slot, + parentRoot: toHexString(newBlock.block.message.parentRoot), + }); + }); + }, + {timeout: 60000} +); diff --git a/packages/beacon-node/test/mocks/fork-choice/timeliness.ts b/packages/beacon-node/test/mocks/fork-choice/timeliness.ts new file mode 100644 index 000000000000..72b3ff66a084 --- /dev/null +++ b/packages/beacon-node/test/mocks/fork-choice/timeliness.ts @@ -0,0 +1,24 @@ +import {ForkChoice} from "@lodestar/fork-choice"; +import {Slot, allForks} from "@lodestar/types"; + +/** + * A specific forkchoice implementation to mark some blocks as timely or not. + */ +export class TimelinessForkChoice extends ForkChoice { + /** + * These need to be in the constructor, however we want to keep the constructor signature the same. + * So they are set after construction in the test instead. + */ + lateSlot: Slot | undefined; + + /** + * This is to mark the `lateSlot` as not timely. + */ + protected isBlockTimely(block: allForks.BeaconBlock, blockDelaySec: number): boolean { + if (block.slot === this.lateSlot) { + return false; + } + + return super.isBlockTimely(block, blockDelaySec); + } +} diff --git a/packages/beacon-node/test/perf/chain/produceBlock/produceBlockBody.test.ts b/packages/beacon-node/test/perf/chain/produceBlock/produceBlockBody.test.ts index 56132cedd790..4cbdd9cebcab 100644 --- a/packages/beacon-node/test/perf/chain/produceBlock/produceBlockBody.test.ts +++ b/packages/beacon-node/test/perf/chain/produceBlock/produceBlockBody.test.ts @@ -75,9 +75,9 @@ describe("produceBlockBody", () => { await produceBlockBody.call(chain, BlockType.Full, state, { parentSlot: slot, slot: slot + 1, + parentBlockRoot: fromHexString(head.blockRoot), graffiti: Buffer.alloc(32), randaoReveal: Buffer.alloc(96), - parentBlockRoot: fromHexString(head.blockRoot), proposerIndex, proposerPubKey, }); diff --git a/packages/beacon-node/test/unit/api/impl/validator/produceBlockV2.test.ts b/packages/beacon-node/test/unit/api/impl/validator/produceBlockV2.test.ts index deb148d34b5a..1ca952fdab70 100644 --- a/packages/beacon-node/test/unit/api/impl/validator/produceBlockV2.test.ts +++ b/packages/beacon-node/test/unit/api/impl/validator/produceBlockV2.test.ts @@ -1,4 +1,4 @@ -import {fromHexString} from "@chainsafe/ssz"; +import {fromHexString, toHexString} from "@chainsafe/ssz"; import {describe, it, expect, beforeEach, afterEach, vi} from "vitest"; import {ssz} from "@lodestar/types"; import {ProtoBlock} from "@lodestar/fork-choice"; @@ -43,9 +43,12 @@ describe("api/validator - produceBlockV2", function () { // Set the node's state to way back from current slot const slot = 100000; const randaoReveal = fullBlock.body.randaoReveal; + const parentBlockRoot = fullBlock.parentRoot; const graffiti = "a".repeat(32); const feeRecipient = "0xcccccccccccccccccccccccccccccccccccccccc"; + // mock whatever value as we also mock produceBlock below + modules.chain["getProposerHead"].mockReturnValue(generateProtoBlock({blockRoot: toHexString(parentBlockRoot)})); modules.chain.produceBlock.mockResolvedValue({ block: fullBlock, executionPayloadValue, @@ -58,6 +61,7 @@ describe("api/validator - produceBlockV2", function () { randaoReveal, graffiti: toGraffitiBuffer(graffiti), slot, + parentBlockRoot, feeRecipient, }); @@ -68,6 +72,7 @@ describe("api/validator - produceBlockV2", function () { randaoReveal, graffiti: toGraffitiBuffer(graffiti), slot, + parentBlockRoot, feeRecipient: undefined, }); }); @@ -81,7 +86,7 @@ describe("api/validator - produceBlockV2", function () { const feeRecipient = "0xccccccccccccccccccccccccccccccccccccccaa"; const headSlot = 0; - modules.forkChoice.getHead.mockReturnValue(generateProtoBlock({slot: headSlot})); + modules.chain["getProposerHead"].mockReturnValue(generateProtoBlock({slot: headSlot})); modules.chain["opPool"].getSlashingsAndExits.mockReturnValue([[], [], [], []]); modules.chain["aggregatedAttestationPool"].getAttestationsForBlock.mockReturnValue([]); diff --git a/packages/beacon-node/test/unit/api/impl/validator/produceBlockV3.test.ts b/packages/beacon-node/test/unit/api/impl/validator/produceBlockV3.test.ts index ba0267fc5810..587eb758d66c 100644 --- a/packages/beacon-node/test/unit/api/impl/validator/produceBlockV3.test.ts +++ b/packages/beacon-node/test/unit/api/impl/validator/produceBlockV3.test.ts @@ -1,8 +1,10 @@ import {describe, it, expect, beforeEach, afterEach, vi} from "vitest"; +import {toHexString} from "@chainsafe/ssz"; import {ssz} from "@lodestar/types"; import {SLOTS_PER_EPOCH} from "@lodestar/params"; import {routes} from "@lodestar/api"; import {createBeaconConfig, createChainForkConfig, defaultChainConfig} from "@lodestar/config"; +import {ProtoBlock} from "@lodestar/fork-choice"; import {ApiTestModules, getApiTestModules} from "../../../../utils/api.js"; import {SyncState} from "../../../../../src/sync/interface.js"; import {getValidatorApi} from "../../../../../src/api/impl/validator/index.js"; @@ -83,6 +85,8 @@ describe("api/validator - produceBlockV3", function () { vi.spyOn(modules.chain.clock, "currentSlot", "get").mockReturnValue(currentSlot); vi.spyOn(modules.sync, "state", "get").mockReturnValue(SyncState.Synced); + modules.chain.getProposerHead.mockReturnValue({blockRoot: toHexString(fullBlock.parentRoot)} as ProtoBlock); + if (enginePayloadValue !== null) { const commonBlockBody: CommonBlockBody = { attestations: fullBlock.body.attestations, diff --git a/packages/beacon-node/test/utils/logger.ts b/packages/beacon-node/test/utils/logger.ts index 1c1526514565..10f27565216f 100644 --- a/packages/beacon-node/test/utils/logger.ts +++ b/packages/beacon-node/test/utils/logger.ts @@ -21,6 +21,6 @@ export const testLogger = (module?: string, opts?: TestLoggerOpts): LoggerNode = opts.module = module; } const level = getEnvLogLevel(); - opts.level = level ?? LogLevel.info; + opts.level = level ?? opts.level ?? LogLevel.info; return getNodeLogger(opts); }; diff --git a/packages/fork-choice/src/forkChoice/forkChoice.ts b/packages/fork-choice/src/forkChoice/forkChoice.ts index 2b5f45b76fb6..93c5c7aaf76f 100644 --- a/packages/fork-choice/src/forkChoice/forkChoice.ts +++ b/packages/fork-choice/src/forkChoice/forkChoice.ts @@ -503,8 +503,7 @@ export class ForkChoice implements IForkChoice { // Assign proposer score boost if the block is timely // before attesting interval = before 1st interval - const isBeforeAttestingInterval = blockDelaySec < this.config.SECONDS_PER_SLOT / INTERVALS_PER_SLOT; - const isTimely = this.fcStore.currentSlot === slot && isBeforeAttestingInterval; + const isTimely = this.isBlockTimely(block, blockDelaySec); if ( this.opts?.proposerBoostEnabled && isTimely && @@ -1050,6 +1049,15 @@ export class ForkChoice implements IForkChoice { throw Error(`Not found dependent root for block slot ${block.slot}, epoch difference ${epochDifference}`); } + /** + * Return true if the block is timely for the current slot. + * Child class can overwrite this for testing purpose. + */ + protected isBlockTimely(block: allForks.BeaconBlock, blockDelaySec: number): boolean { + const isBeforeAttestingInterval = blockDelaySec < this.config.SECONDS_PER_SLOT / INTERVALS_PER_SLOT; + return this.fcStore.currentSlot === block.slot && isBeforeAttestingInterval; + } + private getPreMergeExecStatus(executionStatus: MaybeValidExecutionStatus): ExecutionStatus.PreMerge { if (executionStatus !== ExecutionStatus.PreMerge) throw Error(`Invalid pre-merge execution status: expected: ${ExecutionStatus.PreMerge}, got ${executionStatus}`);