feat(l1): implement FCUv4 targetGasLimit (execution-apis#796)#6773
Conversation
|
Lines of code reportTotal lines added: Detailed view |
edg-l
force-pushed
the
feat/fcu-v4-target-gas-limit
branch
from
e0799cf to
d3c2476
Compare
🤖 Kimi Code ReviewOverall this is a solid implementation of execution-apis#796 with good backward compatibility handling and comprehensive test coverage. A few minor points: Arithmetic safety in The delta calculation assumes let delta = parent_gas_limit / GAS_LIMIT_BOUND_DIVISOR - 1;While Ethereum protocol constraints ensure gas limits are well above 1024 in practice, consider using let delta = parent_gas_limit / GAS_LIMIT_BOUND_DIVISOR.saturating_sub(1);Similarly, Code clarity The comment in Tests The new test coverage is excellent:
Security/Consensus Correctly handles the consensus-critical requirement that payload IDs must be unique across different The permissive deserialization of No blocking issues found. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code ReviewFindings:
The payload-id fix itself looks correct: hashing both I could not run the targeted Rust tests in this environment because Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Greptile SummaryThis PR implements
Confidence Score: 4/5Safe to merge; the core FCUv4 gas-limit flow, serde handling, and ID-collision fix are all correct and well-tested. The only notable concern is that gas_ceil is now hashed unconditionally in BuildPayloadArgs::id() for every payload version (V1–V4), not just V4 where the CL-supplied value can actually vary. For V1/V2/V3 the value is always the static --builder.gas-limit config, so there is no collision risk, but the ID formula for those versions quietly changes relative to before this PR. crates/blockchain/payload.rs — the unconditional gas_ceil hashing change affects V1/V2/V3 payload ID output.
|
| Filename | Overview |
|---|---|
| crates/blockchain/payload.rs | Adds slot_number and gas_ceil to the Keccak payload ID hash; removes a TODO comment from calc_gas_limit. The gas_ceil field is added unconditionally for all payload versions (V1–V4), changing ID output even for non-V4 payloads. |
| crates/networking/rpc/engine/fork_choice.rs | Adds graceful fallback in build_payload_v4: CL-supplied target_gas_limit is preferred; absent field falls back to context.gas_ceil with a structured warn!. Comment in validate_attributes_v4 explains intentional leniency with a TODO for future hardening. |
| crates/networking/rpc/types/fork_choice.rs | Adds target_gas_limit: Option to PayloadAttributesV4 with #[serde(default, with = serde_utils::u64::hex_str_opt)]; handles absent, null, and hex-string inputs correctly. |
| test/tests/blockchain/payload_tests.rs | New unit tests cover BuildPayloadArgs::id() collision fix (distinct gas_ceil, distinct slot_number, stable on identical inputs) and calc_gas_limit step-up, step-down, and clamp behaviour. |
| test/tests/rpc/fork_choice_tests.rs | New integration tests: serde round-trips for present/absent/null targetGasLimit; end-to-end FCUv4 accepting present and absent gas limit on Amsterdam genesis; rejection of pre-Amsterdam timestamp. |
Sequence Diagram
sequenceDiagram
participant CL as Consensus Layer
participant RPC as engine_forkchoiceUpdatedV4
participant V as validate_attributes_v4
participant B as build_payload_v4
participant P as BuildPayloadArgs id
participant CP as create_payload
CL->>RPC: FCUv4(forkChoiceState, PayloadAttributesV4)
RPC->>V: validate_attributes_v4(attrs, head, ctx)
V-->>V: check amsterdam_activated
V-->>V: check withdrawals present
V-->>V: check parent_beacon_block_root present
V-->>RPC: Ok
RPC->>B: build_payload_v4(attrs, ctx, fcs)
alt targetGasLimit present
B-->>B: "gas_ceil = attrs.target_gas_limit"
else targetGasLimit absent
B-->>B: warn and fallback to context.gas_ceil
end
B->>P: BuildPayloadArgs with slot_number and gas_ceil
Note over P: hash includes slot_number and gas_ceil
P-->>B: payload_id
B->>CP: create_payload(args, storage, extra_data)
CP-->>B: Block with calc_gas_limit applied
B-->>RPC: payload_id
RPC-->>CL: payloadStatus VALID plus payloadId
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
crates/blockchain/payload.rs:123-126
`gas_ceil` is added to the hash unconditionally, affecting payload IDs for V1/V2/V3 payloads even though those versions do not carry a CL-supplied gas limit. The collision fix is only needed for V4, where two FCUs may legitimately differ only in `targetGasLimit`. For V1/V2/V3, `gas_ceil` is always drawn from static config (`context.gas_ceil`), so there are no duplicates to disambiguate — and the change alters the ID formula for those versions relative to what existed before this PR. Scoping the `gas_ceil` hash update to V4 (i.e. when `slot_number.is_some()`) keeps the fix targeted and avoids silently invalidating any existing tooling or test fixtures that compute V1/V2/V3 payload IDs off-node.
```suggestion
if let Some(slot_number) = self.slot_number {
hasher.update(slot_number.to_be_bytes());
// execution-apis#796: only include gas_ceil in the hash for V4
// (slot_number is the V4 discriminator). V1/V2/V3 always use the
// static --builder.gas-limit, so adding it there changes IDs
// without preventing any collision.
hasher.update(self.gas_ceil.to_be_bytes());
}
```
Reviews (1): Last reviewed commit: "test(l1): move FCUv4 targetGasLimit test..." | Re-trigger Greptile
🤖 Claude Code ReviewNow I have enough context for a thorough review. Review: feat(l1): implement FCUv4 targetGasLimit (execution-apis#796)SummaryThe implementation is correct and the transitional Medium
hasher.update(self.gas_ceil.to_be_bytes());This line runs for every call to In practice, payload IDs are ephemeral and the CL always uses the ID returned by the EL, never computing it independently — so this is unlikely to break production nodes. However it could break conformance test suites (e.g., Hive) that independently derive expected payload IDs for V1/V2/V3 calls. The fix is straightforward: guard by version or // Only include gas_ceil in the hash for V4+ where it is caller-supplied.
if self.version >= 4 {
hasher.update(self.gas_ceil.to_be_bytes());
}The Low
let target_field = match target_gas_limit {
Some(hex) => format!(",\n \"targetGasLimit\": \"{hex}\""),
None => String::new(),
};
let body = format!(r#"{{ ... {target_field} ... }}"#);The let mut attrs = serde_json::json!({
"timestamp": format!("{timestamp:#x}"),
// ... other fields ...
});
if let Some(hex) = target_gas_limit {
attrs["targetGasLimit"] = serde_json::Value::String(hex.to_string());
}No test for If a CL sends #[test]
fn gas_limit_respects_minimum_when_target_is_zero() {
assert_eq!(calc_gas_limit(30_000_000, 0), 30_000_000 - (30_000_000 / GAS_LIMIT_BOUND_DIVISOR - 1));
// or check it moves toward MIN_GAS_LIMIT
}
With Positive
Automated review by Claude (Anthropic) · sonnet · custom prompt |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if let Some(slot_number) = self.slot_number { | ||
| hasher.update(slot_number.to_be_bytes()); | ||
| } | ||
| hasher.update(self.gas_ceil.to_be_bytes()); |
There was a problem hiding this comment.
gas_ceil is added to the hash unconditionally, affecting payload IDs for V1/V2/V3 payloads even though those versions do not carry a CL-supplied gas limit. The collision fix is only needed for V4, where two FCUs may legitimately differ only in targetGasLimit. For V1/V2/V3, gas_ceil is always drawn from static config (context.gas_ceil), so there are no duplicates to disambiguate — and the change alters the ID formula for those versions relative to what existed before this PR. Scoping the gas_ceil hash update to V4 (i.e. when slot_number.is_some()) keeps the fix targeted and avoids silently invalidating any existing tooling or test fixtures that compute V1/V2/V3 payload IDs off-node.
| if let Some(slot_number) = self.slot_number { | |
| hasher.update(slot_number.to_be_bytes()); | |
| } | |
| hasher.update(self.gas_ceil.to_be_bytes()); | |
| if let Some(slot_number) = self.slot_number { | |
| hasher.update(slot_number.to_be_bytes()); | |
| // execution-apis#796: only include gas_ceil in the hash for V4 | |
| // (slot_number is the V4 discriminator). V1/V2/V3 always use the | |
| // static --builder.gas-limit, so adding it there changes IDs | |
| // without preventing any collision. | |
| hasher.update(self.gas_ceil.to_be_bytes()); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/payload.rs
Line: 123-126
Comment:
`gas_ceil` is added to the hash unconditionally, affecting payload IDs for V1/V2/V3 payloads even though those versions do not carry a CL-supplied gas limit. The collision fix is only needed for V4, where two FCUs may legitimately differ only in `targetGasLimit`. For V1/V2/V3, `gas_ceil` is always drawn from static config (`context.gas_ceil`), so there are no duplicates to disambiguate — and the change alters the ID formula for those versions relative to what existed before this PR. Scoping the `gas_ceil` hash update to V4 (i.e. when `slot_number.is_some()`) keeps the fix targeted and avoids silently invalidating any existing tooling or test fixtures that compute V1/V2/V3 payload IDs off-node.
```suggestion
if let Some(slot_number) = self.slot_number {
hasher.update(slot_number.to_be_bytes());
// execution-apis#796: only include gas_ceil in the hash for V4
// (slot_number is the V4 discriminator). V1/V2/V3 always use the
// static --builder.gas-limit, so adding it there changes IDs
// without preventing any collision.
hasher.update(self.gas_ceil.to_be_bytes());
}
```
How can I resolve this? If you propose a fix, please make it concise.
ElFantasma
left a comment
ElFantasma
left a comment
There was a problem hiding this comment.
Just one non-blocking observation
| hasher.update(slot_number.to_be_bytes()); | ||
| } | ||
| if self.version >= 4 { | ||
| hasher.update(self.gas_ceil.to_be_bytes()); |
There was a problem hiding this comment.
Asymmetry worth pinning down: gas_ceil hashing is explicitly gated on self.version >= 4, but slot_number hashing is gated implicitly via if let Some(slot_number). Today the two are equivalent because every V1-V3 construction site in fork_choice.rs sets slot_number: None (verified at line 461) — only the V4 path passes Some(attributes.slot_number) (line 563). So V1-V3 payload IDs are unchanged in practice.
But the contract is a soft invariant. If a future V5/V6/V7 path keeps setting slot_number: Some(_) (which is correct) AND someone adds slot to a V3 fork-choice call site by mistake (e.g. via a helper that defaults it), the V3 ID would silently start including the slot in the hash, breaking ID stability for any caller cached against the old IDs.
Two cheap lock-ins:
- Symmetric explicit gate:
if self.version >= 4 && let Some(slot_number) = self.slot_number { ... }so both V4+-only inputs are gated the same way and the version is the single source of truth. - Or add a regression test that V1-V3 IDs stay stable when
slot_numberis None (your existingpayload_id_stable_when_inputs_matchuses version=4; a parallel one for version=3 with slot_number=None would lock it in).
(1) is one extra &&; (2) is ~10 lines of test. Either works. Non-blocking — current code is correct.
edg-l
force-pushed
the
feat/fcu-v4-target-gas-limit
branch
from
257a31a to
8c72b36
Compare
…96) for glamsterdam-devnet-4 Extends PayloadAttributesV4 with the new required target_gas_limit field introduced by execution-apis#796 and consensus-specs#5235 (Gloas / upstream "Glamsterdam"). The CL is now the authoritative source of the per-block target gas limit on engine_forkchoiceUpdatedV4 instead of the EL reading it from the static --builder.gas-limit flag. validate_attributes_v4 strictly rejects FCUv4 requests that omit the field with RpcErr::InvalidPayloadAttributes, matching the spec contract. The paired CL on glamsterdam-devnet-4 ships consensus-specs#5235 and populates the field on every FCUv4 it sends. build_payload_v4 sets BuildPayloadArgs.gas_ceil directly from the attribute. The EIP-1559 1/1024 gas-limit envelope inside calc_gas_limit continues to clamp the value relative to the parent block. V1/V2/V3 and non-engine callers (sequencer, dev mode) keep using --builder.gas-limit. Also fixes a latent collision in BuildPayloadArgs::id(): slot_number was never hashed and gas_ceil now matters per-call, so two FCUv4s differing only in CL-supplied targetGasLimit could collide on payload_id. Both fields are now part of the Keccak input. Tests added: - payload-attribute parser round-trips for hex value present, field absent, and explicit null - validator accepts present, rejects absent (matches spec error message), and continues to reject pre-Amsterdam timestamps - BuildPayloadArgs::id() distinguishes different gas_ceil values and different slot_number values, and is stable under equal inputs - calc_gas_limit steps up/down toward target within one EIP-1559 1/1024 step and clamps when the target is within one step
execution-apis#796 makes targetGasLimit a required field on engine_forkchoiceUpdatedV4, but some Amsterdam devnets still run pre-#796 CLs that omit it. Relax validate_attributes_v4 to accept an absent field and fall back to --builder.gas-limit in build_payload_v4, warning so the config drift stays visible. Keeps EL rollout decoupled from CL rollout.
Move the payload-id / calc_gas_limit unit tests to test/tests/blockchain/payload_tests.rs and the PayloadAttributesV4 serde tests to test/tests/rpc/fork_choice_tests.rs (all public API). Rewrite the validate_attributes_v4 unit tests as end-to-end ForkChoiceUpdatedV4 handler tests driven through the public RPC entry point, so the private validator is no longer exposed for testing: accepts a present targetGasLimit, accepts an absent one (fallback), and rejects pre-Amsterdam timestamps.
Make target_gas_limit a required u64 on PayloadAttributesV4 (was Option with a --builder.gas-limit fallback). parse_v4 now rejects present-but- malformed V4 attributes (e.g. missing targetGasLimit) with InvalidPayloadAttributes instead of silently dropping them; build_payload_v4 uses the CL value directly. Drops the fallback + warn and the optionality that diverged from #796.
edg-l
force-pushed
the
feat/fcu-v4-target-gas-limit
branch
from
8c72b36 to
ded8b3e
Compare
…s-limit # Conflicts: # test/tests/rpc/fork_choice_tests.rs
…s-limit # Conflicts: # test/tests/rpc/fork_choice_tests.rs
5 participants
Adds
target_gas_limit: u64toPayloadAttributesV4intypes/fork_choice.rs.build_payload_v4inengine/fork_choice.rsuses the CL-supplied value asBuildPayloadArgs.gas_ceil; the EIP-1559 1/1024 envelope incalc_gas_limitstill clamps it relative to the parent.Per execution-apis#796 the field is required.
parse_v4rejects a present-but-malformed V4 attributes object (e.g. one missingtargetGasLimit) withInvalidPayloadAttributesrather than silently dropping it; an absent/null attributes parameter still yields no attributes (a plain forkchoice update). There is no--builder.gas-limitfallback: an Amsterdam CL that omitstargetGasLimitis non-compliant and is rejected, keeping the EL aligned with #796 for upcoming devnets.Also fixes a latent
BuildPayloadArgs::id()collision:slot_numberwas not hashed andgas_ceilis now caller-supplied, so two FCUv4 calls differing only intargetGasLimitorslotNumberwould produce the samepayload_id. Both fields are now part of the Keccak input.Tests cover:
PayloadAttributesV4serde (present parses; absent and explicitnullare rejected); FCUv4 e2e accepts a presenttargetGasLimit, rejects an absent one withInvalidPayloadAttributes, and still rejects pre-Amsterdam timestamps;BuildPayloadArgs::id()distinguishesgas_ceilandslot_number;calc_gas_limitstep-up, step-down, and clamp. First commit cherry-picked from ilitteri.