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: check gas limit in header received from builder #7336

Open
wants to merge 6 commits into
base: unstable
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
2 changes: 1 addition & 1 deletion packages/beacon-node/src/api/impl/validator/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1524,7 +1524,7 @@ export function getValidatorApi(
);
});

await chain.executionBuilder.registerValidator(filteredRegistrations);
await chain.executionBuilder.registerValidator(currentEpoch, filteredRegistrations);

logger.debug("Forwarded validator registrations to connected builder", {
epoch: currentEpoch,
Expand Down
43 changes: 41 additions & 2 deletions packages/beacon-node/src/chain/produceBlock/produceBlockBody.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,17 @@ import {
ssz,
sszTypesFor,
} from "@lodestar/types";
import {Logger, sleep, toHex, toRootHex} from "@lodestar/utils";
import {Logger, sleep, toHex, toPubkeyHex, toRootHex} from "@lodestar/utils";
import {ZERO_HASH, ZERO_HASH_HEX} from "../../constants/index.js";
import {IEth1ForBlockProduction} from "../../eth1/index.js";
import {numToQuantity} from "../../eth1/provider/utils.js";
import {IExecutionBuilder, IExecutionEngine, PayloadAttributes, PayloadId} from "../../execution/index.js";
import {
IExecutionBuilder,
IExecutionEngine,
PayloadAttributes,
PayloadId,
getExpectedGasLimit,
} from "../../execution/index.js";
import {fromGraffitiBuffer} from "../../util/graffiti.js";
import type {BeaconChain} from "../chain.js";
import {CommonBlockBody} from "../interface.js";
Expand Down Expand Up @@ -223,6 +229,39 @@ export async function produceBlockBody<T extends BlockType>(
fetchedTime,
});

const targetGasLimit = this.executionBuilder.getValidatorRegistration(proposerPubKey)?.gasLimit;
if (!targetGasLimit) {
// This should only happen if cache was cleared due to restart of beacon node
this.logger.warn("Failed to get validator registration, could not check header gas limit", {
slot: blockSlot,
proposerIndex,
proposerPubKey: toPubkeyHex(proposerPubKey),
});
} else {
const headerGasLimit = builderRes.header.gasLimit;
const parentGasLimit = (currentState as CachedBeaconStateBellatrix).latestExecutionPayloadHeader.gasLimit;
const expectedGasLimit = getExpectedGasLimit(parentGasLimit, targetGasLimit);

const lowerBound = Math.min(parentGasLimit, expectedGasLimit);
const upperBound = Math.max(parentGasLimit, expectedGasLimit);

if (headerGasLimit < lowerBound || headerGasLimit > upperBound) {
throw Error(
`Header gas limit ${headerGasLimit} is outside of acceptable range [${lowerBound}, ${upperBound}]`
);
}

if (headerGasLimit !== expectedGasLimit) {
this.logger.warn("Header gas limit does not match expected value", {
slot: blockSlot,
headerGasLimit,
expectedGasLimit,
parentGasLimit,
targetGasLimit,
});
}
}

if (ForkSeq[fork] >= ForkSeq.deneb) {
const {blobKzgCommitments} = builderRes;
if (blobKzgCommitments === undefined) {
Expand Down
34 changes: 34 additions & 0 deletions packages/beacon-node/src/execution/builder/cache.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import {BLSPubkey, Epoch, bellatrix} from "@lodestar/types";
import {toPubkeyHex} from "@lodestar/utils";

const REGISTRATION_PRESERVE_EPOCHS = 2;

export type ValidatorRegistration = {
epoch: Epoch;
/** Preferred gas limit of validator */
gasLimit: number;
};

export class ValidatorRegistrationCache {
Copy link
Member Author

Choose a reason for hiding this comment

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

might be able to combine this with BeaconProposerCache in the future but this requires apis to be consolidated first as proposed in ethereum/beacon-APIs#435

private readonly registrationByValidatorPubkey: Map<string, ValidatorRegistration>;
constructor() {
this.registrationByValidatorPubkey = new Map();
}

add(epoch: Epoch, {pubkey, gasLimit}: bellatrix.ValidatorRegistrationV1): void {
this.registrationByValidatorPubkey.set(toPubkeyHex(pubkey), {epoch, gasLimit});
}

prune(epoch: Epoch): void {
for (const [pubkeyHex, registration] of this.registrationByValidatorPubkey.entries()) {
// We only retain an registrations for REGISTRATION_PRESERVE_EPOCHS epochs
if (registration.epoch + REGISTRATION_PRESERVE_EPOCHS < epoch) {
this.registrationByValidatorPubkey.delete(pubkeyHex);
}
}
}

get(pubkey: BLSPubkey): ValidatorRegistration | undefined {
return this.registrationByValidatorPubkey.get(toPubkeyHex(pubkey));
}
}
15 changes: 14 additions & 1 deletion packages/beacon-node/src/execution/builder/http.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {ForkExecution, SLOTS_PER_EPOCH} from "@lodestar/params";
import {parseExecutionPayloadAndBlobsBundle, reconstructFullBlockOrContents} from "@lodestar/state-transition";
import {
BLSPubkey,
Epoch,
ExecutionPayloadHeader,
Root,
SignedBeaconBlockOrContents,
Expand All @@ -19,6 +20,7 @@ import {
} from "@lodestar/types";
import {toPrintableUrl} from "@lodestar/utils";
import {Metrics} from "../../metrics/metrics.js";
import {ValidatorRegistration, ValidatorRegistrationCache} from "./cache.js";
import {IExecutionBuilder} from "./interface.js";

export type ExecutionBuilderHttpOpts = {
Expand Down Expand Up @@ -60,6 +62,7 @@ const BUILDER_PROPOSAL_DELAY_TOLERANCE = 1000;
export class ExecutionBuilderHttp implements IExecutionBuilder {
readonly api: BuilderApi;
readonly config: ChainForkConfig;
readonly registrations: ValidatorRegistrationCache;
readonly issueLocalFcUWithFeeRecipient?: string;
// Builder needs to be explicity enabled using updateStatus
status = false;
Expand Down Expand Up @@ -93,6 +96,7 @@ export class ExecutionBuilderHttp implements IExecutionBuilder {
);
logger?.info("External builder", {url: toPrintableUrl(baseUrl)});
this.config = config;
this.registrations = new ValidatorRegistrationCache();
this.issueLocalFcUWithFeeRecipient = opts.issueLocalFcUWithFeeRecipient;

/**
Expand Down Expand Up @@ -128,8 +132,17 @@ export class ExecutionBuilderHttp implements IExecutionBuilder {
}
}

async registerValidator(registrations: bellatrix.SignedValidatorRegistrationV1[]): Promise<void> {
async registerValidator(epoch: Epoch, registrations: bellatrix.SignedValidatorRegistrationV1[]): Promise<void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

is adding epoch to api standard spec now?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is an internal method, the api call happens on L136 which does not need the epoch, we only need it to track registrations in our internal cache to be able to prune old records (similar to what we do for proposer cache).

I opted for supplying the epoch by the caller of registerValidator but it could also just use clock.currentEpoch

(await this.api.registerValidator({registrations})).assertOk();

for (const registration of registrations) {
this.registrations.add(epoch, registration.message);
}
this.registrations.prune(epoch);
}

getValidatorRegistration(pubkey: BLSPubkey): ValidatorRegistration | undefined {
return this.registrations.get(pubkey);
}

async getHeader(
Expand Down
1 change: 1 addition & 0 deletions packages/beacon-node/src/execution/builder/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import {ChainForkConfig} from "@lodestar/config";
import {Logger} from "@lodestar/logger";
import {Metrics} from "../../metrics/metrics.js";
import {IExecutionBuilder} from "./interface.js";
export {getExpectedGasLimit} from "./utils.js";

import {ExecutionBuilderHttp, ExecutionBuilderHttpOpts, defaultExecutionBuilderHttpOpts} from "./http.js";

Expand Down
5 changes: 4 additions & 1 deletion packages/beacon-node/src/execution/builder/interface.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import {ForkExecution} from "@lodestar/params";
import {
BLSPubkey,
Epoch,
ExecutionPayloadHeader,
Root,
SignedBeaconBlockOrContents,
Expand All @@ -12,6 +13,7 @@ import {
deneb,
electra,
} from "@lodestar/types";
import {ValidatorRegistration} from "./cache.js";

export interface IExecutionBuilder {
/**
Expand All @@ -28,7 +30,8 @@ export interface IExecutionBuilder {

updateStatus(shouldEnable: boolean): void;
checkStatus(): Promise<void>;
registerValidator(registrations: bellatrix.SignedValidatorRegistrationV1[]): Promise<void>;
registerValidator(epoch: Epoch, registrations: bellatrix.SignedValidatorRegistrationV1[]): Promise<void>;
getValidatorRegistration(pubkey: BLSPubkey): ValidatorRegistration | undefined;
getHeader(
fork: ForkExecution,
slot: Slot,
Expand Down
19 changes: 19 additions & 0 deletions packages/beacon-node/src/execution/builder/utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/**
* From https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1559.md
*/
const gasLimitAdjustmentFactor = 1024;

/**
* Calculates expected gas limit based on parent gas limit and target gas limit
*/
export function getExpectedGasLimit(parentGasLimit: number, targetGasLimit: number): number {
const maxGasLimitDifference = Math.max(Math.floor(parentGasLimit / gasLimitAdjustmentFactor) - 1, 0);

if (targetGasLimit > parentGasLimit) {
Copy link
Contributor

Choose a reason for hiding this comment

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

good to see upper/lower taken care of

const gasDiff = targetGasLimit - parentGasLimit;
return parentGasLimit + Math.min(gasDiff, maxGasLimitDifference);
}

const gasDiff = parentGasLimit - targetGasLimit;
return parentGasLimit - Math.min(gasDiff, maxGasLimitDifference);
}
58 changes: 58 additions & 0 deletions packages/beacon-node/test/unit/execution/builder/cache.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import {ssz} from "@lodestar/types";
import {beforeEach, describe, expect, it} from "vitest";
import {ValidatorRegistrationCache} from "../../../../src/execution/builder/cache.js";

describe("ValidatorRegistrationCache", () => {
const gasLimit1 = 30000000;
const gasLimit2 = 36000000;

const validatorPubkey1 = new Uint8Array(48).fill(1);
const validatorPubkey2 = new Uint8Array(48).fill(2);

const validatorRegistration1 = ssz.bellatrix.ValidatorRegistrationV1.defaultValue();
validatorRegistration1.pubkey = validatorPubkey1;
validatorRegistration1.gasLimit = gasLimit1;

const validatorRegistration2 = ssz.bellatrix.ValidatorRegistrationV1.defaultValue();
validatorRegistration2.pubkey = validatorPubkey2;
validatorRegistration2.gasLimit = gasLimit2;

let cache: ValidatorRegistrationCache;

beforeEach(() => {
// max 2 items
cache = new ValidatorRegistrationCache();
cache.add(1, validatorRegistration1);
cache.add(3, validatorRegistration2);
});

it("get for registered validator", () => {
expect(cache.get(validatorPubkey1)?.gasLimit).toBe(gasLimit1);
});

it("get for unknown validator", () => {
const unknownValidatorPubkey = new Uint8Array(48).fill(3);
expect(cache.get(unknownValidatorPubkey)).toBe(undefined);
});

it("override and get latest", () => {
const newGasLimit = 60000000;
const registration = ssz.bellatrix.ValidatorRegistrationV1.defaultValue();
registration.pubkey = validatorPubkey1;
registration.gasLimit = newGasLimit;

cache.add(5, registration);

expect(cache.get(validatorPubkey1)?.gasLimit).toBe(newGasLimit);
});

it("prune", () => {
cache.prune(4);

// No registration as it has been pruned
expect(cache.get(validatorPubkey1)).toBe(undefined);

// Registration hasn't been pruned
expect(cache.get(validatorPubkey2)?.gasLimit).toBe(gasLimit2);
});
});
66 changes: 66 additions & 0 deletions packages/beacon-node/test/unit/execution/builder/utils.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import {describe, expect, it} from "vitest";
import {getExpectedGasLimit} from "../../../../src/execution/builder/utils.js";

describe("execution / builder / utils", () => {
describe("getExpectedGasLimit", () => {
const testCases: {
name: string;
parentGasLimit: number;
targetGasLimit: number;
expected: number;
}[] = [
{
name: "Increase within limit",
parentGasLimit: 30000000,
targetGasLimit: 30000100,
expected: 30000100,
},
{
name: "Increase exceeding limit",
parentGasLimit: 30000000,
targetGasLimit: 36000000,
expected: 30029295, // maxGasLimitDifference = (30000000 / 1024) - 1 = 29295
},
{
name: "Decrease within limit",
parentGasLimit: 30000000,
targetGasLimit: 29999990,
expected: 29999990,
},
{
name: "Decrease exceeding limit",
parentGasLimit: 36000000,
targetGasLimit: 30000000,
expected: 35964845, // maxGasLimitDifference = (36000000 / 1024) - 1 = 35155
},
{
name: "Target equals parent",
parentGasLimit: 30000000,
targetGasLimit: 30000000,
expected: 30000000, // No change
},
{
name: "Very small parent gas limit",
parentGasLimit: 1025,
targetGasLimit: 2000,
expected: 1025,
},
{
name: "Target far below parent but limited",
parentGasLimit: 30000000,
targetGasLimit: 10000000,
expected: 29970705, // maxGasLimitDifference = (30000000 / 1024) - 1 = 29295
},
{
name: "Parent gas limit underflows",
parentGasLimit: 1023,
targetGasLimit: 30000000,
expected: 1023,
},
];

it.each(testCases)("$name", ({parentGasLimit, targetGasLimit, expected}) => {
expect(getExpectedGasLimit(parentGasLimit, targetGasLimit)).toBe(expected);
});
});
});
Loading