From fb16303eab8c41a248f7bac11332ea3dc94fba65 Mon Sep 17 00:00:00 2001 From: g11tech Date: Tue, 9 Jan 2024 19:57:22 +0530 Subject: [PATCH] feat: reject builder blocks if engine indicates censorship (#6258) * feat: reject builder blocks if engine indicates censorship * apply feeback * fix tests * small refac and test fixes and selection test extenstion for override combinations * make typing tighter for blindedblock * apply feedbac * Fix typo --------- Co-authored-by: Nico Flaig --- .../src/api/impl/validator/index.ts | 35 ++++++++++--- packages/beacon-node/src/chain/chain.ts | 51 ++++++++++++------- packages/beacon-node/src/chain/interface.ts | 17 ++++--- .../chain/produceBlock/produceBlockBody.ts | 16 ++++-- .../beacon-node/src/execution/engine/http.ts | 7 ++- .../src/execution/engine/interface.ts | 7 ++- .../beacon-node/src/execution/engine/types.ts | 14 ++++- .../api/impl/validator/produceBlockV2.test.ts | 6 ++- .../api/impl/validator/produceBlockV3.test.ts | 44 +++++++++++----- 9 files changed, 143 insertions(+), 54 deletions(-) diff --git a/packages/beacon-node/src/api/impl/validator/index.ts b/packages/beacon-node/src/api/impl/validator/index.ts index 9f98c606d525..839c1df7c463 100644 --- a/packages/beacon-node/src/api/impl/validator/index.ts +++ b/packages/beacon-node/src/api/impl/validator/index.ts @@ -353,7 +353,7 @@ export function getValidatorApi({ strictFeeRecipientCheck, skipHeadChecksAndUpdate, }: Omit & {skipHeadChecksAndUpdate?: boolean} = {} - ): Promise { + ): Promise { const source = ProducedBlockSource.engine; metrics?.blockProductionRequests.inc({source}); @@ -371,7 +371,7 @@ export function getValidatorApi({ let timer; try { timer = metrics?.blockProductionTime.startTimer(); - const {block, executionPayloadValue, consensusBlockValue} = await chain.produceBlock({ + const {block, executionPayloadValue, consensusBlockValue, shouldOverrideBuilder} = await chain.produceBlock({ slot, randaoReveal, graffiti: toGraffitiBuffer(graffiti || ""), @@ -408,9 +408,10 @@ export function getValidatorApi({ version, executionPayloadValue, consensusBlockValue, + shouldOverrideBuilder, }; } else { - return {data: block, version, executionPayloadValue, consensusBlockValue}; + return {data: block, version, executionPayloadValue, consensusBlockValue, shouldOverrideBuilder}; } } finally { if (timer) timer({source}); @@ -504,7 +505,10 @@ export function getValidatorApi({ // reference index of promises in the race const promisesOrder = [ProducedBlockSource.builder, ProducedBlockSource.engine]; [blindedBlock, fullBlock] = await racePromisesWithCutoff< - routes.validator.ProduceBlockOrContentsRes | routes.validator.ProduceBlindedBlockRes | null + | ((routes.validator.ProduceBlockOrContentsRes | routes.validator.ProduceBlindedBlockRes) & { + shouldOverrideBuilder?: boolean; + }) + | null >( [blindedBlockPromise, fullBlockPromise], BLOCK_PRODUCTION_RACE_CUTOFF_MS, @@ -552,8 +556,20 @@ export function getValidatorApi({ const blockValueEngine = enginePayloadValue + gweiToWei(consensusBlockValueEngine); // Total block value is in wei let executionPayloadSource: ProducedBlockSource | null = null; + const shouldOverrideBuilder = fullBlock?.shouldOverrideBuilder ?? false; - if (fullBlock && blindedBlock) { + // handle the builder override case separately + if (shouldOverrideBuilder === true) { + executionPayloadSource = ProducedBlockSource.engine; + logger.info("Selected engine block as censorship suspected in builder blocks", { + // winston logger doesn't like bigint + enginePayloadValue: `${enginePayloadValue}`, + consensusBlockValueEngine: `${consensusBlockValueEngine}`, + blockValueEngine: `${blockValueEngine}`, + shouldOverrideBuilder, + slot, + }); + } else if (fullBlock && blindedBlock) { switch (builderSelection) { case routes.validator.BuilderSelection.MaxProfit: { if ( @@ -579,7 +595,7 @@ export function getValidatorApi({ executionPayloadSource = ProducedBlockSource.builder; } } - logger.verbose(`Selected executionPayloadSource=${executionPayloadSource} block`, { + logger.info(`Selected executionPayloadSource=${executionPayloadSource} block`, { builderSelection, // winston logger doesn't like bigint builderBoostFactor: `${builderBoostFactor}`, @@ -589,24 +605,27 @@ export function getValidatorApi({ consensusBlockValueBuilder: `${consensusBlockValueBuilder}`, blockValueEngine: `${blockValueEngine}`, blockValueBuilder: `${blockValueBuilder}`, + shouldOverrideBuilder, slot, }); } else if (fullBlock && !blindedBlock) { executionPayloadSource = ProducedBlockSource.engine; - logger.verbose("Selected engine block: no builder block produced", { + logger.info("Selected engine block: no builder block produced", { // winston logger doesn't like bigint enginePayloadValue: `${enginePayloadValue}`, consensusBlockValueEngine: `${consensusBlockValueEngine}`, blockValueEngine: `${blockValueEngine}`, + shouldOverrideBuilder, slot, }); } else if (blindedBlock && !fullBlock) { executionPayloadSource = ProducedBlockSource.builder; - logger.verbose("Selected builder block: no engine block produced", { + logger.info("Selected builder block: no engine block produced", { // winston logger doesn't like bigint builderPayloadValue: `${builderPayloadValue}`, consensusBlockValueBuilder: `${consensusBlockValueBuilder}`, blockValueBuilder: `${blockValueBuilder}`, + shouldOverrideBuilder, slot, }); } diff --git a/packages/beacon-node/src/chain/chain.ts b/packages/beacon-node/src/chain/chain.ts index aa44b5cf1cbc..acf6c02d98f3 100644 --- a/packages/beacon-node/src/chain/chain.ts +++ b/packages/beacon-node/src/chain/chain.ts @@ -472,22 +472,32 @@ export class BeaconChain implements IBeaconChain { return data && {block: data, executionOptimistic: false}; } - produceBlock( - blockAttributes: BlockAttributes - ): Promise<{block: allForks.BeaconBlock; executionPayloadValue: Wei; consensusBlockValue: Gwei}> { + produceBlock(blockAttributes: BlockAttributes): Promise<{ + block: allForks.BeaconBlock; + executionPayloadValue: Wei; + consensusBlockValue: Gwei; + shouldOverrideBuilder?: boolean; + }> { return this.produceBlockWrapper(BlockType.Full, blockAttributes); } - produceBlindedBlock( - blockAttributes: BlockAttributes - ): Promise<{block: allForks.BlindedBeaconBlock; executionPayloadValue: Wei; consensusBlockValue: Gwei}> { + produceBlindedBlock(blockAttributes: BlockAttributes): Promise<{ + block: allForks.BlindedBeaconBlock; + executionPayloadValue: Wei; + consensusBlockValue: Gwei; + }> { return this.produceBlockWrapper(BlockType.Blinded, blockAttributes); } async produceBlockWrapper( blockType: T, {randaoReveal, graffiti, slot, feeRecipient}: BlockAttributes - ): Promise<{block: AssembledBlockType; executionPayloadValue: Wei; consensusBlockValue: Gwei}> { + ): Promise<{ + block: AssembledBlockType; + executionPayloadValue: Wei; + consensusBlockValue: Gwei; + shouldOverrideBuilder?: boolean; + }> { const head = this.forkChoice.getHead(); const state = await this.regen.getBlockSlotState( head.blockRoot, @@ -499,16 +509,21 @@ export class BeaconChain implements IBeaconChain { const proposerIndex = state.epochCtx.getBeaconProposer(slot); const proposerPubKey = state.epochCtx.index2pubkey[proposerIndex].toBytes(); - const {body, blobs, executionPayloadValue} = await produceBlockBody.call(this, blockType, state, { - randaoReveal, - graffiti, - slot, - feeRecipient, - parentSlot: slot - 1, - parentBlockRoot, - proposerIndex, - proposerPubKey, - }); + const {body, blobs, executionPayloadValue, shouldOverrideBuilder} = await produceBlockBody.call( + this, + blockType, + state, + { + randaoReveal, + graffiti, + slot, + feeRecipient, + parentSlot: slot - 1, + parentBlockRoot, + proposerIndex, + proposerPubKey, + } + ); // The hashtree root computed here for debug log will get cached and hence won't introduce additional delays const bodyRoot = @@ -559,7 +574,7 @@ export class BeaconChain implements IBeaconChain { this.metrics?.blockProductionCaches.producedContentsCache.set(this.producedContentsCache.size); } - return {block, executionPayloadValue, consensusBlockValue: proposerReward}; + return {block, executionPayloadValue, consensusBlockValue: proposerReward, shouldOverrideBuilder}; } /** diff --git a/packages/beacon-node/src/chain/interface.ts b/packages/beacon-node/src/chain/interface.ts index 1365d8766ec7..fbeba12c87b4 100644 --- a/packages/beacon-node/src/chain/interface.ts +++ b/packages/beacon-node/src/chain/interface.ts @@ -155,12 +155,17 @@ export interface IBeaconChain { getContents(beaconBlock: deneb.BeaconBlock): deneb.Contents; - produceBlock( - blockAttributes: BlockAttributes - ): Promise<{block: allForks.BeaconBlock; executionPayloadValue: Wei; consensusBlockValue: Gwei}>; - produceBlindedBlock( - blockAttributes: BlockAttributes - ): Promise<{block: allForks.BlindedBeaconBlock; executionPayloadValue: Wei; consensusBlockValue: Gwei}>; + produceBlock(blockAttributes: BlockAttributes): Promise<{ + block: allForks.BeaconBlock; + executionPayloadValue: Wei; + consensusBlockValue: Gwei; + shouldOverrideBuilder?: boolean; + }>; + produceBlindedBlock(blockAttributes: BlockAttributes): Promise<{ + block: allForks.BlindedBeaconBlock; + executionPayloadValue: Wei; + consensusBlockValue: Gwei; + }>; /** Process a block until complete */ processBlock(block: BlockInput, opts?: ImportBlockOpts): Promise; diff --git a/packages/beacon-node/src/chain/produceBlock/produceBlockBody.ts b/packages/beacon-node/src/chain/produceBlock/produceBlockBody.ts index 3c2bec223eca..0b6ff7b1316b 100644 --- a/packages/beacon-node/src/chain/produceBlock/produceBlockBody.ts +++ b/packages/beacon-node/src/chain/produceBlock/produceBlockBody.ts @@ -109,12 +109,20 @@ export async function produceBlockBody( proposerIndex: ValidatorIndex; proposerPubKey: BLSPubkey; } -): Promise<{body: AssembledBodyType; blobs: BlobsResult; executionPayloadValue: Wei}> { +): Promise<{ + body: AssembledBodyType; + blobs: BlobsResult; + executionPayloadValue: Wei; + shouldOverrideBuilder?: boolean; +}> { // Type-safe for blobs variable. Translate 'null' value into 'preDeneb' enum // TODO: Not ideal, but better than just using null. // TODO: Does not guarantee that preDeneb enum goes with a preDeneb block let blobsResult: BlobsResult; let executionPayloadValue: Wei; + // even though shouldOverrideBuilder is relevant for the engine response, for simplicity of typing + // we just return it undefined for the builder which anyway doesn't get consumed downstream + let shouldOverrideBuilder: boolean | undefined; const fork = currentState.config.getForkName(blockSlot); const logMeta: Record = { @@ -295,9 +303,11 @@ export async function produceBlockBody( const engineRes = await this.executionEngine.getPayload(fork, payloadId); const {executionPayload, blobsBundle} = engineRes; + shouldOverrideBuilder = engineRes.shouldOverrideBuilder; + (blockBody as allForks.ExecutionBlockBody).executionPayload = executionPayload; executionPayloadValue = engineRes.executionPayloadValue; - Object.assign(logMeta, {transactions: executionPayload.transactions.length}); + Object.assign(logMeta, {transactions: executionPayload.transactions.length, shouldOverrideBuilder}); const fetchedTime = Date.now() / 1000 - computeTimeAtSlot(this.config, blockSlot, this.genesisTime); this.metrics?.blockPayload.payloadFetchedTime.observe({prepType}, fetchedTime); @@ -380,7 +390,7 @@ export async function produceBlockBody( Object.assign(logMeta, {executionPayloadValue}); this.logger.verbose("Produced beacon block body", logMeta); - return {body: blockBody as AssembledBodyType, blobs: blobsResult, executionPayloadValue}; + return {body: blockBody as AssembledBodyType, blobs: blobsResult, executionPayloadValue, shouldOverrideBuilder}; } /** diff --git a/packages/beacon-node/src/execution/engine/http.ts b/packages/beacon-node/src/execution/engine/http.ts index 70df97ba1e4a..91ceabaf2770 100644 --- a/packages/beacon-node/src/execution/engine/http.ts +++ b/packages/beacon-node/src/execution/engine/http.ts @@ -363,7 +363,12 @@ export class ExecutionEngineHttp implements IExecutionEngine { async getPayload( fork: ForkName, payloadId: PayloadId - ): Promise<{executionPayload: allForks.ExecutionPayload; executionPayloadValue: Wei; blobsBundle?: BlobsBundle}> { + ): Promise<{ + executionPayload: allForks.ExecutionPayload; + executionPayloadValue: Wei; + blobsBundle?: BlobsBundle; + shouldOverrideBuilder?: boolean; + }> { const method = ForkSeq[fork] >= ForkSeq.deneb ? "engine_getPayloadV3" diff --git a/packages/beacon-node/src/execution/engine/interface.ts b/packages/beacon-node/src/execution/engine/interface.ts index 9a7ee3963379..e5f612fc0965 100644 --- a/packages/beacon-node/src/execution/engine/interface.ts +++ b/packages/beacon-node/src/execution/engine/interface.ts @@ -136,7 +136,12 @@ export interface IExecutionEngine { getPayload( fork: ForkName, payloadId: PayloadId - ): Promise<{executionPayload: allForks.ExecutionPayload; executionPayloadValue: Wei; blobsBundle?: BlobsBundle}>; + ): Promise<{ + executionPayload: allForks.ExecutionPayload; + executionPayloadValue: Wei; + blobsBundle?: BlobsBundle; + shouldOverrideBuilder?: boolean; + }>; getPayloadBodiesByHash(blockHash: DATA[]): Promise<(ExecutionPayloadBody | null)[]>; diff --git a/packages/beacon-node/src/execution/engine/types.ts b/packages/beacon-node/src/execution/engine/types.ts index 4f24480e0b96..72a0100f7a51 100644 --- a/packages/beacon-node/src/execution/engine/types.ts +++ b/packages/beacon-node/src/execution/engine/types.ts @@ -107,6 +107,7 @@ type ExecutionPayloadRpcWithValue = { // even though CL tracks this as executionPayloadValue, EL returns this as blockValue blockValue: QUANTITY; blobsBundle?: BlobsBundleRpc; + shouldOverrideBuilder?: boolean; }; type ExecutionPayloadResponse = ExecutionPayloadRpc | ExecutionPayloadRpcWithValue; @@ -207,19 +208,28 @@ export function hasPayloadValue(response: ExecutionPayloadResponse): response is export function parseExecutionPayload( fork: ForkName, response: ExecutionPayloadResponse -): {executionPayload: allForks.ExecutionPayload; executionPayloadValue: Wei; blobsBundle?: BlobsBundle} { +): { + executionPayload: allForks.ExecutionPayload; + executionPayloadValue: Wei; + blobsBundle?: BlobsBundle; + shouldOverrideBuilder?: boolean; +} { let data: ExecutionPayloadRpc; let executionPayloadValue: Wei; let blobsBundle: BlobsBundle | undefined; + let shouldOverrideBuilder: boolean; + if (hasPayloadValue(response)) { executionPayloadValue = quantityToBigint(response.blockValue); data = response.executionPayload; blobsBundle = response.blobsBundle ? parseBlobsBundle(response.blobsBundle) : undefined; + shouldOverrideBuilder = response.shouldOverrideBuilder ?? false; } else { data = response; // Just set it to zero as default executionPayloadValue = BigInt(0); blobsBundle = undefined; + shouldOverrideBuilder = false; } const executionPayload = { @@ -269,7 +279,7 @@ export function parseExecutionPayload( (executionPayload as deneb.ExecutionPayload).excessBlobGas = quantityToBigint(excessBlobGas); } - return {executionPayload, executionPayloadValue, blobsBundle}; + return {executionPayload, executionPayloadValue, blobsBundle, shouldOverrideBuilder}; } export function serializePayloadAttributes(data: PayloadAttributes): PayloadAttributesRpc { 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 3e96f3b932c8..9ca426672efe 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 @@ -85,7 +85,11 @@ describe("api/validator - produceBlockV2", function () { const feeRecipient = "0xcccccccccccccccccccccccccccccccccccccccc"; const api = getValidatorApi(modules); - server.chainStub.produceBlock.mockResolvedValue({block: fullBlock, executionPayloadValue, consensusBlockValue}); + server.chainStub.produceBlock.mockResolvedValue({ + block: fullBlock, + executionPayloadValue, + consensusBlockValue, + }); // check if expectedFeeRecipient is passed to produceBlock await api.produceBlockV2(slot, randaoReveal, graffiti, {feeRecipient}); 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 83e1e7887510..3a87b709b741 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 @@ -41,23 +41,38 @@ describe("api/validator - produceBlockV3", function () { vi.clearAllMocks(); }); - const testCases: [routes.validator.BuilderSelection, number | null, number | null, number, string][] = [ - [routes.validator.BuilderSelection.MaxProfit, 1, 0, 0, "builder"], - [routes.validator.BuilderSelection.MaxProfit, 1, 2, 1, "engine"], - [routes.validator.BuilderSelection.MaxProfit, null, 0, 0, "engine"], - [routes.validator.BuilderSelection.MaxProfit, 0, null, 1, "builder"], - - [routes.validator.BuilderSelection.BuilderAlways, 1, 2, 0, "builder"], - [routes.validator.BuilderSelection.BuilderAlways, 1, 0, 1, "builder"], - [routes.validator.BuilderSelection.BuilderAlways, null, 0, 0, "engine"], - [routes.validator.BuilderSelection.BuilderAlways, 0, null, 1, "builder"], - - [routes.validator.BuilderSelection.BuilderOnly, 0, 2, 0, "builder"], - [routes.validator.BuilderSelection.ExecutionOnly, 2, 0, 1, "execution"], + const testCases: [routes.validator.BuilderSelection, number | null, number | null, number, boolean, string][] = [ + [routes.validator.BuilderSelection.MaxProfit, 1, 0, 0, false, "builder"], + [routes.validator.BuilderSelection.MaxProfit, 1, 2, 1, false, "engine"], + [routes.validator.BuilderSelection.MaxProfit, null, 0, 0, false, "engine"], + [routes.validator.BuilderSelection.MaxProfit, 0, null, 1, false, "builder"], + [routes.validator.BuilderSelection.MaxProfit, 0, null, 1, true, "builder"], + [routes.validator.BuilderSelection.MaxProfit, 1, 1, 1, true, "engine"], + [routes.validator.BuilderSelection.MaxProfit, 2, 1, 1, true, "engine"], + + [routes.validator.BuilderSelection.BuilderAlways, 1, 2, 0, false, "builder"], + [routes.validator.BuilderSelection.BuilderAlways, 1, 0, 1, false, "builder"], + [routes.validator.BuilderSelection.BuilderAlways, null, 0, 0, false, "engine"], + [routes.validator.BuilderSelection.BuilderAlways, 0, null, 1, false, "builder"], + [routes.validator.BuilderSelection.BuilderAlways, 0, 1, 1, true, "engine"], + [routes.validator.BuilderSelection.BuilderAlways, 1, 1, 1, true, "engine"], + [routes.validator.BuilderSelection.BuilderAlways, 1, null, 1, true, "builder"], + + [routes.validator.BuilderSelection.BuilderOnly, 0, 2, 0, false, "builder"], + [routes.validator.BuilderSelection.ExecutionOnly, 2, 0, 1, false, "engine"], + [routes.validator.BuilderSelection.BuilderOnly, 1, 1, 0, true, "builder"], + [routes.validator.BuilderSelection.ExecutionOnly, 1, 1, 1, true, "engine"], ]; testCases.forEach( - ([builderSelection, builderPayloadValue, enginePayloadValue, consensusBlockValue, finalSelection]) => { + ([ + builderSelection, + builderPayloadValue, + enginePayloadValue, + consensusBlockValue, + shouldOverrideBuilder, + finalSelection, + ]) => { it(`produceBlockV3 - ${finalSelection} produces block`, async () => { syncStub = server.syncStub; modules = { @@ -89,6 +104,7 @@ describe("api/validator - produceBlockV3", function () { block: fullBlock, executionPayloadValue: BigInt(enginePayloadValue), consensusBlockValue: BigInt(consensusBlockValue), + shouldOverrideBuilder, }); } else { chainStub.produceBlock.mockRejectedValue(Error("not produced"));