From ba9f922beb358a3ea9e256df3c92c91a5b976d8c Mon Sep 17 00:00:00 2001 From: harkamal Date: Mon, 8 Jan 2024 16:44:20 +0530 Subject: [PATCH 1/7] feat: reject builder blocks if engine indicates censorship --- .../src/api/impl/validator/index.ts | 49 ++++++++++++----- packages/beacon-node/src/chain/chain.ts | 52 ++++++++++++------- packages/beacon-node/src/chain/interface.ts | 18 ++++--- .../chain/produceBlock/produceBlockBody.ts | 20 +++++-- .../beacon-node/src/execution/engine/http.ts | 7 ++- .../src/execution/engine/interface.ts | 7 ++- .../beacon-node/src/execution/engine/types.ts | 14 ++++- 7 files changed, 124 insertions(+), 43 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..fd1ea4d385ce 100644 --- a/packages/beacon-node/src/api/impl/validator/index.ts +++ b/packages/beacon-node/src/api/impl/validator/index.ts @@ -288,7 +288,7 @@ export function getValidatorApi({ { skipHeadChecksAndUpdate, }: Omit & {skipHeadChecksAndUpdate?: boolean} = {} - ): Promise { + ): Promise { const version = config.getForkName(slot); if (!isForkExecution(version)) { throw Error(`Invalid fork=${version} for produceBuilderBlindedBlock`); @@ -319,11 +319,12 @@ export function getValidatorApi({ let timer; try { timer = metrics?.blockProductionTime.startTimer(); - const {block, executionPayloadValue, consensusBlockValue} = await chain.produceBlindedBlock({ - slot, - randaoReveal, - graffiti: toGraffitiBuffer(graffiti || ""), - }); + const {block, executionPayloadValue, consensusBlockValue, shouldOverrideBuilder} = + await chain.produceBlindedBlock({ + slot, + randaoReveal, + graffiti: toGraffitiBuffer(graffiti || ""), + }); metrics?.blockProductionSuccess.inc({source}); metrics?.blockProductionNumAggregated.observe({source}, block.body.attestations.length); @@ -338,7 +339,7 @@ export function getValidatorApi({ void chain.persistBlock(block, "produced_builder_block"); } - return {data: block, version, executionPayloadValue, consensusBlockValue}; + return {data: block, version, executionPayloadValue, consensusBlockValue, shouldOverrideBuilder}; } finally { if (timer) timer({source}); } @@ -353,7 +354,7 @@ export function getValidatorApi({ strictFeeRecipientCheck, skipHeadChecksAndUpdate, }: Omit & {skipHeadChecksAndUpdate?: boolean} = {} - ): Promise { + ): Promise { const source = ProducedBlockSource.engine; metrics?.blockProductionRequests.inc({source}); @@ -371,7 +372,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 +409,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 +506,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 +557,26 @@ export function getValidatorApi({ const blockValueEngine = enginePayloadValue + gweiToWei(consensusBlockValueEngine); // Total block value is in wei let executionPayloadSource: ProducedBlockSource | null = null; + const shouldOverrideBuilder = fullBlock?.shouldOverrideBuilder ?? false; + + // handle the builder override case separately + if (shouldOverrideBuilder === true) { + // this is just to make typescript happy as shouldOverrideBuilder can be true only will valid + // full block response + if (fullBlock === null) { + throw Error("Invalid null fullBlock with builder override"); + } - if (fullBlock && blindedBlock) { + executionPayloadSource = ProducedBlockSource.engine; + logger.verbose("Selected engine block as censorship suspected in builder blocks", { + // winston logger doesn't like bigint + enginePayloadValue: `${enginePayloadValue}`, + consensusBlockValueEngine: `${consensusBlockValueEngine}`, + blockValueEngine: `${blockValueEngine}`, + slot, + shouldOverrideBuilder, + }); + } else if (fullBlock && blindedBlock) { switch (builderSelection) { case routes.validator.BuilderSelection.MaxProfit: { if ( @@ -590,6 +613,7 @@ export function getValidatorApi({ blockValueEngine: `${blockValueEngine}`, blockValueBuilder: `${blockValueBuilder}`, slot, + shouldOverrideBuilder, }); } else if (fullBlock && !blindedBlock) { executionPayloadSource = ProducedBlockSource.engine; @@ -599,6 +623,7 @@ export function getValidatorApi({ consensusBlockValueEngine: `${consensusBlockValueEngine}`, blockValueEngine: `${blockValueEngine}`, slot, + shouldOverrideBuilder, }); } else if (blindedBlock && !fullBlock) { executionPayloadSource = ProducedBlockSource.builder; diff --git a/packages/beacon-node/src/chain/chain.ts b/packages/beacon-node/src/chain/chain.ts index 456bfef70b0a..95ec2a848f7c 100644 --- a/packages/beacon-node/src/chain/chain.ts +++ b/packages/beacon-node/src/chain/chain.ts @@ -470,22 +470,33 @@ 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; + shouldOverrideBuilder: boolean; + }> { 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, @@ -497,16 +508,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 = @@ -557,7 +573,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 880a5e86071a..07a7d4060651 100644 --- a/packages/beacon-node/src/chain/interface.ts +++ b/packages/beacon-node/src/chain/interface.ts @@ -154,12 +154,18 @@ 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; + shouldOverrideBuilder: boolean; + }>; /** 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..83f12372cfb9 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 can also return is just as false for builder which will anyway be ignored by the caller + let shouldOverrideBuilder: boolean; const fork = currentState.config.getForkName(blockSlot); const logMeta: Record = { @@ -204,6 +212,7 @@ export async function produceBlockBody( if (blockType === BlockType.Blinded) { if (!this.executionBuilder) throw Error("Execution Builder not available"); + shouldOverrideBuilder = false; // This path will not be used in the production, but is here just for merge mock // tests because merge-mock requires an fcU to be issued prior to fetch payload @@ -280,6 +289,7 @@ export async function produceBlockBody( ssz.allForksExecution[fork].ExecutionPayload.defaultValue(); blobsResult = {type: BlobsResultType.preDeneb}; executionPayloadValue = BigInt(0); + shouldOverrideBuilder = false; } else { const {prepType, payloadId} = prepareRes; Object.assign(logMeta, {executionPayloadPrepType: prepType}); @@ -295,9 +305,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); @@ -347,6 +359,7 @@ export async function produceBlockBody( ssz.allForksExecution[fork].ExecutionPayload.defaultValue(); blobsResult = {type: BlobsResultType.preDeneb}; executionPayloadValue = BigInt(0); + shouldOverrideBuilder = false; } else { // since merge transition is complete, we need a valid payload even if with an // empty (transactions) one. defaultValue isn't gonna cut it! @@ -357,6 +370,7 @@ export async function produceBlockBody( } else { blobsResult = {type: BlobsResultType.preDeneb}; executionPayloadValue = BigInt(0); + shouldOverrideBuilder = false; } endExecutionPayload?.({ step: BlockProductionStep.executionPayload, @@ -380,7 +394,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..d9b5b81f3a7d 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..e6d1c8543414 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..0934b101a691 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 { From 97823254cf57aa5507c8920a4dadf700b2672eda Mon Sep 17 00:00:00 2001 From: harkamal Date: Mon, 8 Jan 2024 23:47:48 +0530 Subject: [PATCH 2/7] apply feeback --- packages/beacon-node/src/api/impl/validator/index.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/packages/beacon-node/src/api/impl/validator/index.ts b/packages/beacon-node/src/api/impl/validator/index.ts index fd1ea4d385ce..515df87cd9d9 100644 --- a/packages/beacon-node/src/api/impl/validator/index.ts +++ b/packages/beacon-node/src/api/impl/validator/index.ts @@ -561,12 +561,6 @@ export function getValidatorApi({ // handle the builder override case separately if (shouldOverrideBuilder === true) { - // this is just to make typescript happy as shouldOverrideBuilder can be true only will valid - // full block response - if (fullBlock === null) { - throw Error("Invalid null fullBlock with builder override"); - } - executionPayloadSource = ProducedBlockSource.engine; logger.verbose("Selected engine block as censorship suspected in builder blocks", { // winston logger doesn't like bigint From a2895619b57c702b8457e0660da314ed87259ae3 Mon Sep 17 00:00:00 2001 From: harkamal Date: Tue, 9 Jan 2024 01:56:52 +0530 Subject: [PATCH 3/7] fix tests --- .../test/unit/api/impl/validator/produceBlockV2.test.ts | 8 +++++++- .../test/unit/api/impl/validator/produceBlockV3.test.ts | 2 ++ 2 files changed, 9 insertions(+), 1 deletion(-) 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..ad9bfc882e14 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,12 @@ 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, + shouldOverrideBuilder: false, + }); // check if expectedFeeRecipient is passed to produceBlock await api.produceBlockV2(slot, randaoReveal, graffiti, {feeRecipient}); @@ -129,6 +134,7 @@ describe("api/validator - produceBlockV2", function () { executionEngineStub.getPayload.mockResolvedValue({ executionPayload: ssz.bellatrix.ExecutionPayload.defaultValue(), executionPayloadValue, + shouldOverrideBuilder: false, }); // use fee recipient passed in produceBlockBody call for payload gen in engine notifyForkchoiceUpdate 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..76372a1d9a1e 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 @@ -89,6 +89,7 @@ describe("api/validator - produceBlockV3", function () { block: fullBlock, executionPayloadValue: BigInt(enginePayloadValue), consensusBlockValue: BigInt(consensusBlockValue), + shouldOverrideBuilder: false, }); } else { chainStub.produceBlock.mockRejectedValue(Error("not produced")); @@ -99,6 +100,7 @@ describe("api/validator - produceBlockV3", function () { block: blindedBlock, executionPayloadValue: BigInt(builderPayloadValue), consensusBlockValue: BigInt(consensusBlockValue), + shouldOverrideBuilder: false, }); } else { chainStub.produceBlindedBlock.mockRejectedValue(Error("not produced")); From b51d931ba7c7bc19bbc98481ed576d05000057c8 Mon Sep 17 00:00:00 2001 From: harkamal Date: Tue, 9 Jan 2024 16:24:55 +0530 Subject: [PATCH 4/7] small refac and test fixes and selection test extenstion for override combinations --- .../src/api/impl/validator/index.ts | 27 ++++++----- packages/beacon-node/src/chain/chain.ts | 6 +-- packages/beacon-node/src/chain/interface.ts | 4 +- .../chain/produceBlock/produceBlockBody.ts | 8 +--- .../beacon-node/src/execution/engine/http.ts | 2 +- .../src/execution/engine/interface.ts | 2 +- .../beacon-node/src/execution/engine/types.ts | 2 +- .../api/impl/validator/produceBlockV2.test.ts | 2 - .../api/impl/validator/produceBlockV3.test.ts | 46 ++++++++++++------- 9 files changed, 53 insertions(+), 46 deletions(-) diff --git a/packages/beacon-node/src/api/impl/validator/index.ts b/packages/beacon-node/src/api/impl/validator/index.ts index 515df87cd9d9..dcce4222269b 100644 --- a/packages/beacon-node/src/api/impl/validator/index.ts +++ b/packages/beacon-node/src/api/impl/validator/index.ts @@ -288,7 +288,7 @@ export function getValidatorApi({ { skipHeadChecksAndUpdate, }: Omit & {skipHeadChecksAndUpdate?: boolean} = {} - ): Promise { + ): Promise { const version = config.getForkName(slot); if (!isForkExecution(version)) { throw Error(`Invalid fork=${version} for produceBuilderBlindedBlock`); @@ -319,12 +319,11 @@ export function getValidatorApi({ let timer; try { timer = metrics?.blockProductionTime.startTimer(); - const {block, executionPayloadValue, consensusBlockValue, shouldOverrideBuilder} = - await chain.produceBlindedBlock({ - slot, - randaoReveal, - graffiti: toGraffitiBuffer(graffiti || ""), - }); + const {block, executionPayloadValue, consensusBlockValue} = await chain.produceBlindedBlock({ + slot, + randaoReveal, + graffiti: toGraffitiBuffer(graffiti || ""), + }); metrics?.blockProductionSuccess.inc({source}); metrics?.blockProductionNumAggregated.observe({source}, block.body.attestations.length); @@ -339,7 +338,7 @@ export function getValidatorApi({ void chain.persistBlock(block, "produced_builder_block"); } - return {data: block, version, executionPayloadValue, consensusBlockValue, shouldOverrideBuilder}; + return {data: block, version, executionPayloadValue, consensusBlockValue}; } finally { if (timer) timer({source}); } @@ -354,7 +353,7 @@ export function getValidatorApi({ strictFeeRecipientCheck, skipHeadChecksAndUpdate, }: Omit & {skipHeadChecksAndUpdate?: boolean} = {} - ): Promise { + ): Promise { const source = ProducedBlockSource.engine; metrics?.blockProductionRequests.inc({source}); @@ -507,7 +506,7 @@ export function getValidatorApi({ const promisesOrder = [ProducedBlockSource.builder, ProducedBlockSource.engine]; [blindedBlock, fullBlock] = await racePromisesWithCutoff< | ((routes.validator.ProduceBlockOrContentsRes | routes.validator.ProduceBlindedBlockRes) & { - shouldOverrideBuilder: boolean; + shouldOverrideBuilder?: boolean; }) | null >( @@ -562,7 +561,7 @@ export function getValidatorApi({ // handle the builder override case separately if (shouldOverrideBuilder === true) { executionPayloadSource = ProducedBlockSource.engine; - logger.verbose("Selected engine block as censorship suspected in builder blocks", { + logger.info("Selected engine block as censorship suspected in builder blocks", { // winston logger doesn't like bigint enginePayloadValue: `${enginePayloadValue}`, consensusBlockValueEngine: `${consensusBlockValueEngine}`, @@ -596,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}`, @@ -611,7 +610,7 @@ export function getValidatorApi({ }); } 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}`, @@ -621,7 +620,7 @@ export function getValidatorApi({ }); } 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}`, diff --git a/packages/beacon-node/src/chain/chain.ts b/packages/beacon-node/src/chain/chain.ts index 95ec2a848f7c..f917f0a14d88 100644 --- a/packages/beacon-node/src/chain/chain.ts +++ b/packages/beacon-node/src/chain/chain.ts @@ -474,7 +474,7 @@ export class BeaconChain implements IBeaconChain { block: allForks.BeaconBlock; executionPayloadValue: Wei; consensusBlockValue: Gwei; - shouldOverrideBuilder: boolean; + shouldOverrideBuilder?: boolean; }> { return this.produceBlockWrapper(BlockType.Full, blockAttributes); } @@ -483,7 +483,7 @@ export class BeaconChain implements IBeaconChain { block: allForks.BlindedBeaconBlock; executionPayloadValue: Wei; consensusBlockValue: Gwei; - shouldOverrideBuilder: boolean; + shouldOverrideBuilder?: boolean; }> { return this.produceBlockWrapper(BlockType.Blinded, blockAttributes); } @@ -495,7 +495,7 @@ export class BeaconChain implements IBeaconChain { block: AssembledBlockType; executionPayloadValue: Wei; consensusBlockValue: Gwei; - shouldOverrideBuilder: boolean; + shouldOverrideBuilder?: boolean; }> { const head = this.forkChoice.getHead(); const state = await this.regen.getBlockSlotState( diff --git a/packages/beacon-node/src/chain/interface.ts b/packages/beacon-node/src/chain/interface.ts index 07a7d4060651..9556d3e39b16 100644 --- a/packages/beacon-node/src/chain/interface.ts +++ b/packages/beacon-node/src/chain/interface.ts @@ -158,13 +158,13 @@ export interface IBeaconChain { block: allForks.BeaconBlock; executionPayloadValue: Wei; consensusBlockValue: Gwei; - shouldOverrideBuilder: boolean; + shouldOverrideBuilder?: boolean; }>; produceBlindedBlock(blockAttributes: BlockAttributes): Promise<{ block: allForks.BlindedBeaconBlock; executionPayloadValue: Wei; consensusBlockValue: Gwei; - shouldOverrideBuilder: boolean; + shouldOverrideBuilder?: boolean; }>; /** Process a block until complete */ diff --git a/packages/beacon-node/src/chain/produceBlock/produceBlockBody.ts b/packages/beacon-node/src/chain/produceBlock/produceBlockBody.ts index 83f12372cfb9..783bae54fc4f 100644 --- a/packages/beacon-node/src/chain/produceBlock/produceBlockBody.ts +++ b/packages/beacon-node/src/chain/produceBlock/produceBlockBody.ts @@ -113,7 +113,7 @@ export async function produceBlockBody( body: AssembledBodyType; blobs: BlobsResult; executionPayloadValue: Wei; - shouldOverrideBuilder: boolean; + shouldOverrideBuilder?: boolean; }> { // Type-safe for blobs variable. Translate 'null' value into 'preDeneb' enum // TODO: Not ideal, but better than just using null. @@ -122,7 +122,7 @@ export async function produceBlockBody( let executionPayloadValue: Wei; // even though shouldOverrideBuilder is relevant for the engine response, for simplicity of typing // we can also return is just as false for builder which will anyway be ignored by the caller - let shouldOverrideBuilder: boolean; + let shouldOverrideBuilder: boolean | undefined; const fork = currentState.config.getForkName(blockSlot); const logMeta: Record = { @@ -212,7 +212,6 @@ export async function produceBlockBody( if (blockType === BlockType.Blinded) { if (!this.executionBuilder) throw Error("Execution Builder not available"); - shouldOverrideBuilder = false; // This path will not be used in the production, but is here just for merge mock // tests because merge-mock requires an fcU to be issued prior to fetch payload @@ -289,7 +288,6 @@ export async function produceBlockBody( ssz.allForksExecution[fork].ExecutionPayload.defaultValue(); blobsResult = {type: BlobsResultType.preDeneb}; executionPayloadValue = BigInt(0); - shouldOverrideBuilder = false; } else { const {prepType, payloadId} = prepareRes; Object.assign(logMeta, {executionPayloadPrepType: prepType}); @@ -359,7 +357,6 @@ export async function produceBlockBody( ssz.allForksExecution[fork].ExecutionPayload.defaultValue(); blobsResult = {type: BlobsResultType.preDeneb}; executionPayloadValue = BigInt(0); - shouldOverrideBuilder = false; } else { // since merge transition is complete, we need a valid payload even if with an // empty (transactions) one. defaultValue isn't gonna cut it! @@ -370,7 +367,6 @@ export async function produceBlockBody( } else { blobsResult = {type: BlobsResultType.preDeneb}; executionPayloadValue = BigInt(0); - shouldOverrideBuilder = false; } endExecutionPayload?.({ step: BlockProductionStep.executionPayload, diff --git a/packages/beacon-node/src/execution/engine/http.ts b/packages/beacon-node/src/execution/engine/http.ts index d9b5b81f3a7d..91ceabaf2770 100644 --- a/packages/beacon-node/src/execution/engine/http.ts +++ b/packages/beacon-node/src/execution/engine/http.ts @@ -367,7 +367,7 @@ export class ExecutionEngineHttp implements IExecutionEngine { executionPayload: allForks.ExecutionPayload; executionPayloadValue: Wei; blobsBundle?: BlobsBundle; - shouldOverrideBuilder: boolean; + shouldOverrideBuilder?: boolean; }> { const method = ForkSeq[fork] >= ForkSeq.deneb diff --git a/packages/beacon-node/src/execution/engine/interface.ts b/packages/beacon-node/src/execution/engine/interface.ts index e6d1c8543414..e5f612fc0965 100644 --- a/packages/beacon-node/src/execution/engine/interface.ts +++ b/packages/beacon-node/src/execution/engine/interface.ts @@ -140,7 +140,7 @@ export interface IExecutionEngine { executionPayload: allForks.ExecutionPayload; executionPayloadValue: Wei; blobsBundle?: BlobsBundle; - shouldOverrideBuilder: boolean; + 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 0934b101a691..72a0100f7a51 100644 --- a/packages/beacon-node/src/execution/engine/types.ts +++ b/packages/beacon-node/src/execution/engine/types.ts @@ -212,7 +212,7 @@ export function parseExecutionPayload( executionPayload: allForks.ExecutionPayload; executionPayloadValue: Wei; blobsBundle?: BlobsBundle; - shouldOverrideBuilder: boolean; + shouldOverrideBuilder?: boolean; } { let data: ExecutionPayloadRpc; let executionPayloadValue: Wei; 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 ad9bfc882e14..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 @@ -89,7 +89,6 @@ describe("api/validator - produceBlockV2", function () { block: fullBlock, executionPayloadValue, consensusBlockValue, - shouldOverrideBuilder: false, }); // check if expectedFeeRecipient is passed to produceBlock @@ -134,7 +133,6 @@ describe("api/validator - produceBlockV2", function () { executionEngineStub.getPayload.mockResolvedValue({ executionPayload: ssz.bellatrix.ExecutionPayload.defaultValue(), executionPayloadValue, - shouldOverrideBuilder: false, }); // use fee recipient passed in produceBlockBody call for payload gen in engine notifyForkchoiceUpdate 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 76372a1d9a1e..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,7 +104,7 @@ describe("api/validator - produceBlockV3", function () { block: fullBlock, executionPayloadValue: BigInt(enginePayloadValue), consensusBlockValue: BigInt(consensusBlockValue), - shouldOverrideBuilder: false, + shouldOverrideBuilder, }); } else { chainStub.produceBlock.mockRejectedValue(Error("not produced")); @@ -100,7 +115,6 @@ describe("api/validator - produceBlockV3", function () { block: blindedBlock, executionPayloadValue: BigInt(builderPayloadValue), consensusBlockValue: BigInt(consensusBlockValue), - shouldOverrideBuilder: false, }); } else { chainStub.produceBlindedBlock.mockRejectedValue(Error("not produced")); From d4379368964059c7ac4700b0d60524f34d09a7ef Mon Sep 17 00:00:00 2001 From: harkamal Date: Tue, 9 Jan 2024 18:05:17 +0530 Subject: [PATCH 5/7] make typing tighter for blindedblock --- packages/beacon-node/src/chain/chain.ts | 1 - packages/beacon-node/src/chain/interface.ts | 1 - 2 files changed, 2 deletions(-) diff --git a/packages/beacon-node/src/chain/chain.ts b/packages/beacon-node/src/chain/chain.ts index f917f0a14d88..ac2f97128c16 100644 --- a/packages/beacon-node/src/chain/chain.ts +++ b/packages/beacon-node/src/chain/chain.ts @@ -483,7 +483,6 @@ export class BeaconChain implements IBeaconChain { block: allForks.BlindedBeaconBlock; executionPayloadValue: Wei; consensusBlockValue: Gwei; - shouldOverrideBuilder?: boolean; }> { return this.produceBlockWrapper(BlockType.Blinded, blockAttributes); } diff --git a/packages/beacon-node/src/chain/interface.ts b/packages/beacon-node/src/chain/interface.ts index 9556d3e39b16..a2f7fba34093 100644 --- a/packages/beacon-node/src/chain/interface.ts +++ b/packages/beacon-node/src/chain/interface.ts @@ -164,7 +164,6 @@ export interface IBeaconChain { block: allForks.BlindedBeaconBlock; executionPayloadValue: Wei; consensusBlockValue: Gwei; - shouldOverrideBuilder?: boolean; }>; /** Process a block until complete */ From 7f017015dd85a98c54ddf91392f4dc964fe33c13 Mon Sep 17 00:00:00 2001 From: harkamal Date: Tue, 9 Jan 2024 19:19:26 +0530 Subject: [PATCH 6/7] apply feedbac --- packages/beacon-node/src/api/impl/validator/index.ts | 7 ++++--- .../beacon-node/src/chain/produceBlock/produceBlockBody.ts | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/beacon-node/src/api/impl/validator/index.ts b/packages/beacon-node/src/api/impl/validator/index.ts index dcce4222269b..839c1df7c463 100644 --- a/packages/beacon-node/src/api/impl/validator/index.ts +++ b/packages/beacon-node/src/api/impl/validator/index.ts @@ -566,8 +566,8 @@ export function getValidatorApi({ enginePayloadValue: `${enginePayloadValue}`, consensusBlockValueEngine: `${consensusBlockValueEngine}`, blockValueEngine: `${blockValueEngine}`, - slot, shouldOverrideBuilder, + slot, }); } else if (fullBlock && blindedBlock) { switch (builderSelection) { @@ -605,8 +605,8 @@ export function getValidatorApi({ consensusBlockValueBuilder: `${consensusBlockValueBuilder}`, blockValueEngine: `${blockValueEngine}`, blockValueBuilder: `${blockValueBuilder}`, - slot, shouldOverrideBuilder, + slot, }); } else if (fullBlock && !blindedBlock) { executionPayloadSource = ProducedBlockSource.engine; @@ -615,8 +615,8 @@ export function getValidatorApi({ enginePayloadValue: `${enginePayloadValue}`, consensusBlockValueEngine: `${consensusBlockValueEngine}`, blockValueEngine: `${blockValueEngine}`, - slot, shouldOverrideBuilder, + slot, }); } else if (blindedBlock && !fullBlock) { executionPayloadSource = ProducedBlockSource.builder; @@ -625,6 +625,7 @@ export function getValidatorApi({ builderPayloadValue: `${builderPayloadValue}`, consensusBlockValueBuilder: `${consensusBlockValueBuilder}`, blockValueBuilder: `${blockValueBuilder}`, + shouldOverrideBuilder, slot, }); } diff --git a/packages/beacon-node/src/chain/produceBlock/produceBlockBody.ts b/packages/beacon-node/src/chain/produceBlock/produceBlockBody.ts index 783bae54fc4f..bb7b93501cf6 100644 --- a/packages/beacon-node/src/chain/produceBlock/produceBlockBody.ts +++ b/packages/beacon-node/src/chain/produceBlock/produceBlockBody.ts @@ -121,7 +121,7 @@ export async function produceBlockBody( let blobsResult: BlobsResult; let executionPayloadValue: Wei; // even though shouldOverrideBuilder is relevant for the engine response, for simplicity of typing - // we can also return is just as false for builder which will anyway be ignored by the caller + // we just return it undefined for the builder which anyway doesn't gets consumed downstream let shouldOverrideBuilder: boolean | undefined; const fork = currentState.config.getForkName(blockSlot); From f9c89f427f7550e0ee0f15373bbbd7a7c1ad7676 Mon Sep 17 00:00:00 2001 From: Nico Flaig Date: Tue, 9 Jan 2024 14:59:12 +0100 Subject: [PATCH 7/7] Fix typo --- packages/beacon-node/src/chain/produceBlock/produceBlockBody.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/beacon-node/src/chain/produceBlock/produceBlockBody.ts b/packages/beacon-node/src/chain/produceBlock/produceBlockBody.ts index bb7b93501cf6..0b6ff7b1316b 100644 --- a/packages/beacon-node/src/chain/produceBlock/produceBlockBody.ts +++ b/packages/beacon-node/src/chain/produceBlock/produceBlockBody.ts @@ -121,7 +121,7 @@ export async function produceBlockBody( 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 gets consumed downstream + // 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);