Skip to content

Commit

Permalink
feat: reject builder blocks if engine indicates censorship (#6258)
Browse files Browse the repository at this point in the history
* 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 <nflaig@protonmail.com>
  • Loading branch information
g11tech and nflaig authored Jan 9, 2024
1 parent 55bc136 commit 0f377dd
Show file tree
Hide file tree
Showing 9 changed files with 143 additions and 54 deletions.
35 changes: 27 additions & 8 deletions packages/beacon-node/src/api/impl/validator/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ export function getValidatorApi({
strictFeeRecipientCheck,
skipHeadChecksAndUpdate,
}: Omit<routes.validator.ExtraProduceBlockOps, "builderSelection"> & {skipHeadChecksAndUpdate?: boolean} = {}
): Promise<routes.validator.ProduceBlockOrContentsRes> {
): Promise<routes.validator.ProduceBlockOrContentsRes & {shouldOverrideBuilder?: boolean}> {
const source = ProducedBlockSource.engine;
metrics?.blockProductionRequests.inc({source});

Expand All @@ -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 || ""),
Expand Down Expand Up @@ -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});
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 (
Expand All @@ -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}`,
Expand All @@ -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,
});
}
Expand Down
51 changes: 33 additions & 18 deletions packages/beacon-node/src/chain/chain.ts
Original file line number Diff line number Diff line change
Expand Up @@ -470,22 +470,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>(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>(BlockType.Blinded, blockAttributes);
}

async produceBlockWrapper<T extends BlockType>(
blockType: T,
{randaoReveal, graffiti, slot, feeRecipient}: BlockAttributes
): Promise<{block: AssembledBlockType<T>; executionPayloadValue: Wei; consensusBlockValue: Gwei}> {
): Promise<{
block: AssembledBlockType<T>;
executionPayloadValue: Wei;
consensusBlockValue: Gwei;
shouldOverrideBuilder?: boolean;
}> {
const head = this.forkChoice.getHead();
const state = await this.regen.getBlockSlotState(
head.blockRoot,
Expand All @@ -497,16 +507,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 =
Expand Down Expand Up @@ -557,7 +572,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};
}

/**
Expand Down
17 changes: 11 additions & 6 deletions packages/beacon-node/src/chain/interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -154,12 +154,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<void>;
Expand Down
16 changes: 13 additions & 3 deletions packages/beacon-node/src/chain/produceBlock/produceBlockBody.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,12 +109,20 @@ export async function produceBlockBody<T extends BlockType>(
proposerIndex: ValidatorIndex;
proposerPubKey: BLSPubkey;
}
): Promise<{body: AssembledBodyType<T>; blobs: BlobsResult; executionPayloadValue: Wei}> {
): Promise<{
body: AssembledBodyType<T>;
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<string, string | number | bigint> = {
Expand Down Expand Up @@ -295,9 +303,11 @@ export async function produceBlockBody<T extends BlockType>(

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);
Expand Down Expand Up @@ -380,7 +390,7 @@ export async function produceBlockBody<T extends BlockType>(
Object.assign(logMeta, {executionPayloadValue});
this.logger.verbose("Produced beacon block body", logMeta);

return {body: blockBody as AssembledBodyType<T>, blobs: blobsResult, executionPayloadValue};
return {body: blockBody as AssembledBodyType<T>, blobs: blobsResult, executionPayloadValue, shouldOverrideBuilder};
}

/**
Expand Down
7 changes: 6 additions & 1 deletion packages/beacon-node/src/execution/engine/http.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
7 changes: 6 additions & 1 deletion packages/beacon-node/src/execution/engine/interface.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)[]>;

Expand Down
14 changes: 12 additions & 2 deletions packages/beacon-node/src/execution/engine/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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 = {
Expand Down Expand Up @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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});
Expand Down
Loading

0 comments on commit 0f377dd

Please sign in to comment.