fix(l1): honor targetGasLimit in testing_buildBlockV1 - #7123
Open
MysticRyuujin wants to merge 2 commits into
Open
fix(l1): honor targetGasLimit in testing_buildBlockV1#7123MysticRyuujin wants to merge 2 commits into
MysticRyuujin wants to merge 2 commits into
Conversation
Greptile SummaryThe PR teaches
Confidence Score: 4/5The PR appears safe to merge, with a non-blocking fork-gating inconsistency for pre-Amsterdam fixture requests. The intended post-Amsterdam targetGasLimit path is implemented and covered, but a caller can also supply the field before activation and override the configured gas ceiling where the corresponding Engine path would reject it. Files Needing Attention: crates/networking/rpc/testing.rs
|
| Filename | Overview |
|---|---|
| crates/networking/rpc/testing.rs | Correctly forwards targetGasLimit into payload construction and tests the main path, but does not gate the Amsterdam-specific field by fork activation. |
Prompt To Fix All With AI
### Issue 1
crates/networking/rpc/testing.rs:164
**Gate target gas by Amsterdam**
For a pre-Amsterdam timestamp, `target_gas_limit` still replaces `context.gas_ceil`, so fixture generation derives the block gas limit from a fork-inactive attribute even though this handler gates `slotNumber` and the Engine V4 path rejects such attributes.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(l1): honor targetGasLimit in testing..." | Re-trigger Greptile
This file contains hidden or 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
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.
testing_buildBlockV1readslotNumberfrom the raw payload attributes but droppedtargetGasLimit. It always passed the node's static gas ceiling toBuildPayloadArgs, so post-Amsterdam attributes could not steer the built block's gas limit. The engine path already uses the attribute:build_payload_v4setsgas_ceilfromattributes.target_gas_limit.This change reads
targetGasLimitfrom the attributes object, the same way the handler readsslotNumber, and uses it as the gas ceiling when present. When the field is absent, the handler keeps the node's static ceiling, so pre-Amsterdam calls do not change.Adds a parse test and a handler test. The handler test passes a
targetGasLimitequal to the parent's gas limit and asserts the built block keeps that gas limit; it fails without the fix.go-ethereum had the same bug and fixed it in ethereum/go-ethereum#35501. See also ethereum/execution-apis#857 and ethereum/execution-apis#862.