-
-
Notifications
You must be signed in to change notification settings - Fork 317
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
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
d317aef
feat: check gas limit in header received from builder
nflaig 3b4f655
Add constant for gas limit adjustment factor
nflaig 107694c
Relax header gas limit check to lower / upper bound
nflaig 500c07d
Include parent and target gas limit in warning log
nflaig 3f91ab5
Merge branch 'unstable' into nflaig/check-header-gas-limit
nflaig b6ae0bd
Merge branch 'unstable' into nflaig/check-header-gas-limit
nflaig d00450b
Add note on why validator pubkey used as key instead of index
nflaig File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
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 { | ||
/** | ||
* Map to track registrations by validator pubkey which is used here instead of | ||
* validator index as `bellatrix.ValidatorRegistrationV1` does not contain the index | ||
* and builder flow in general prefers to use pubkey over index. | ||
*/ | ||
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}); | ||
nflaig marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
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)); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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) { | ||
nflaig marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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
58
packages/beacon-node/test/unit/execution/builder/cache.test.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
66
packages/beacon-node/test/unit/execution/builder/utils.test.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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); | ||
}); | ||
}); | ||
}); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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