-
-
Notifications
You must be signed in to change notification settings - Fork 315
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
base: unstable
Are you sure you want to change the base?
Changes from all commits
d317aef
3b4f655
107694c
500c07d
3f91ab5
b6ae0bd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 { | ||
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)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
|
@@ -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 = { | ||
|
@@ -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; | ||
|
@@ -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; | ||
|
||
/** | ||
|
@@ -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> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is adding epoch to api standard spec now? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
(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( | ||
|
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} |
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); | ||
}); | ||
}); |
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); | ||
}); | ||
}); | ||
}); |
There was a problem hiding this comment.
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