Skip to content

Validate bid gas limit against the payload at bid.parent_block_hash - #8941

Open
barnabasbusa wants to merge 1 commit into
status-im:unstablefrom
barnabasbusa:bbusa/bid-gas-limit-parent-hash
Open

Validate bid gas limit against the payload at bid.parent_block_hash#8941
barnabasbusa wants to merge 1 commit into
status-im:unstablefrom
barnabasbusa:bbusa/bid-gas-limit-parent-hash

Conversation

@barnabasbusa

Copy link
Copy Markdown
Contributor

What

validateExecutionPayloadBid derived parent_gas_limit from dag.headState.latest_execution_payload_bid.gas_limit — the head block's own bid — regardless of which payload the bid builds on. The spec keys the check on the payload the bid actually extends:

parent_gas_limit = seen.execution_payloads[bid.parent_block_hash].gas_limit

(https://github.com/ethereum/consensus-specs/blob/a670e0193446474bb1775ec94a93ca6fbaa11a89/specs/gloas/p2p-interface.md#L995-L1000)

A bid with parent_block_root = head and parent_block_hash = head_bid.parent_block_hash (head payload treated as withheld — permitted by is_bid_compatible_with_head when should_build_on_full is false) must be checked against the grandparent payload's gas limit. Whenever the gas limit is moving, that differs from the head bid's gas limit by one EIP-1559 step, so a spec-correct bid was dropped with gas limit not target-compatible — on gossip and via POST /eth/v1/beacon/execution_payload_bids (same path).

This PR resolves the block carrying the payload identified by bid.parent_block_hash using the PayloadAvailability already computed for the pool key (Timely → parent block, Withheld → its execution parent via a new loadExecutionParent), and uses that block's committed gas limit (loadExecutionGasLimit: the bid's gas_limit post-Gloas, the payload's pre-Gloas). Envelopes must match their bid's gas_limit, so the bid value is the payload value.

Why

Seen on glamsterdam-devnet-8 (gas limit ramping under EIP-8261). Slot 85165: payload at 85163 had gas limit 199411976, payload at 85164 (head) had 199606713, target 200000000. A builder's parent-empty bid carried 199606713 = 199411976 + 199411976/1024 − 1, correct for the payload it builds on; comparing against the head bid (199606713) instead makes it look like it failed to step toward the target.

Same class of bug was found in prysm (fixed in OffchainLabs/prysm#17405), lighthouse (sigp/lighthouse#9905) and grandine; lodestar and teku are correct. The spec gossip test vectors don't cover empty-head bids yet (all cases use the head's own payload as parent_block_hash), a vector PR is in progress.

Notes

  • loadExecutionParent walks ancestors loading execution hashes on demand (bounded by EXECUTION_PARENT_MAX_DEPTH), unlike executionParent which only inspects already-loaded hashes.
  • Builds nimbus_beacon_node locally; there is no existing unit-test harness for validateExecutionPayloadBid, happy to add one if you can point me at the preferred fixture.

validateExecutionPayloadBid derived parent_gas_limit from the head state's
latest_execution_payload_bid, i.e. the head block's own bid. The spec keys
parent_gas_limit on bid.parent_block_hash: a bid that builds on the parent's
parent payload (parent payload withheld) must be checked against that
payload's gas limit, which differs from the parent bid's gas limit whenever the
gas limit is moving. Such bids were dropped with "gas limit not
target-compatible", on gossip and via the publish REST endpoint.

Resolve the block carrying the payload identified by bid.parent_block_hash
(the parent block for Timely, its execution parent for Withheld) and use that
block's committed gas limit.
of PayloadAvailability.Timely:
parentBlck
of PayloadAvailability.Withheld:
dag.loadExecutionParent(parentBlck).valueOr:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have

proc executionParent*(
dag: ChainDAGRef, parentRef: BlockRef,
parentBlockHash: Eth2Digest): Opt[BlockRef] =

that loads hashes and searches for the execution parent.

@github-actions

Copy link
Copy Markdown

Unit Test Results

       12 files  ±0    2 888 suites  ±0   1h 4m 1s ⏱️ - 9m 17s
11 458 tests ±0    8 266 ✔️ ±0  3 192 💤 ±0  0 ±0 
57 188 runs  ±0  51 361 ✔️ ±0  5 827 💤 ±0  0 ±0 

Results for commit 31a4116. ± Comparison against base commit 7acd211.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants