feat: check gas limit in header received from builder #7336
+238
−5
Conversation
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
nflaig
force-pushed
the
nflaig/check-header-gas-limit
branch
from
587034f to
c0308f9
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #7336 +/- ##
=========================================
Coverage 48.76% 48.76%
=========================================
Files 601 601
Lines 40243 40243
Branches 2067 2067
=========================================
Hits 19626 19626
Misses 20579 20579
Partials 38 38 |
Performance Report✔️ no performance regression detected 🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
nflaig
force-pushed
the
nflaig/check-header-gas-limit
branch
from
c0308f9 to
d317aef
Compare
nflaig
commented
Jan 7, 2025
| gasLimit: number; | ||
| }; | ||
|
|
||
| export class ValidatorRegistrationCache { |
There was a problem hiding this comment.
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
g11tech
reviewed
Jan 9, 2025
| const parentGasLimit = (currentState as CachedBeaconStateBellatrix).latestExecutionPayloadHeader.gasLimit; | ||
| const expectedGasLimit = getExpectedGasLimit(parentGasLimit, targetGasLimit); | ||
|
|
||
| if (builderRes.header.gasLimit !== expectedGasLimit) { |
There was a problem hiding this comment.
i think many builders/relays might have bugs over here initially.
we should relax it and make sure its between [parent limit, exected limit] or [expected limit, parent limit] interval and then warn if its not equal to expected
There was a problem hiding this comment.
i think many builders/relays might have bugs over here initially.
Running this branch on Holesky right now but it's hard to validate if gas target is not moving
we should relax it and make sure its between [parent limit, exected limit] or [expected limit, parent limit]
Right so in this case we should definitely reject as the block would be invalid, we can relax for now and observe this for a while on mainnet, eventually we can enforce the strict check
g11tech
reviewed
Jan 9, 2025
| @@ -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.
is adding epoch to api standard spec now?
There was a problem hiding this comment.
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
g11tech
reviewed
Jan 9, 2025
| 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.
good to see upper/lower taken care of
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Motivation
As per spec the builder must adhere to the preferred gas limit of the validator
but currently we are not validating this anywhere and rely on honest builder behavior which is not great considering there are only two builder that produce most of the blocks.
Description
This PR adds a check the to builder flow that compares the expected gas limit against the target gas limit (ie. preferred gas limit of validator) and rejects the builder block in case there is a mismatch which will trigger the fallback to local block.
The expected gas limit is calculated based on this spec
I haven't found where this is defined but it's based on what other clients implement (eg. prysmaticlabs/prysm#14707 and Consensys/teku#8909) and makes sense considering gas limit checks in EIP-1559.