Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: reject builder blocks if engine indicates censorship #6258

Merged
merged 7 commits into from
Jan 9, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 26 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}`,
slot,
shouldOverrideBuilder,
g11tech marked this conversation as resolved.
Show resolved Hide resolved
g11tech marked this conversation as resolved.
Show resolved Hide resolved
});
} 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 @@ -590,19 +606,21 @@ export function getValidatorApi({
blockValueEngine: `${blockValueEngine}`,
blockValueBuilder: `${blockValueBuilder}`,
slot,
shouldOverrideBuilder,
});
} 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}`,
slot,
shouldOverrideBuilder,
g11tech marked this conversation as resolved.
Show resolved Hide resolved
});
} 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}`,
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 can also return is just as false for builder which will anyway be ignored by the caller
g11tech marked this conversation as resolved.
Show resolved Hide resolved
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
Original file line number Diff line number Diff line change
Expand Up @@ -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"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case we produce a builder block even though the execution client asked us to override builder, are we fine with this behavior, maybe we should at least log a warning that builders are suspected to censor transactions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will be changed in followup PR where builderselection will be removed on the beacon side (didn't want to mix it in this PR), so just ignore for now, followup on builderselection coming shortly

[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 = {
Expand Down Expand Up @@ -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"));
Expand Down
Loading