From b51d931ba7c7bc19bbc98481ed576d05000057c8 Mon Sep 17 00:00:00 2001 From: harkamal Date: Tue, 9 Jan 2024 16:24:55 +0530 Subject: [PATCH] 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"));