fix: apply gossip clock disparity to execution payload bid slot validation - #9627
Conversation
…ation `validateExecutionPayloadBid` checked `bid.slot === currentSlot || bid.slot === currentSlot + 1` with no `MAXIMUM_GOSSIP_CLOCK_DISPARITY` allowance, so a bid arriving within the disparity window of a slot boundary was wrongly IGNORED (`INVALID_SLOT`). Replace the exact-slot check with the spec's `is_within_slot_range(state, bid.slot, 1, current_time_ms)` semantics: the current time must fall within `[start(bid.slot - 1), start(bid.slot + 1)]` extended by `+/- MAXIMUM_GOSSIP_CLOCK_DISPARITY` on both ends (i.e. the clock is in slot `bid.slot - 1` -- bid.slot is the next slot -- or `bid.slot` -- the current slot). Implemented with `clock.msFromSlot` because the disparity boundary is sub-slot (1 ms flips the outcome), which the slot-granular helpers (`currentSlotWithGossipDisparity` / `slotWithFutureTolerance`) cannot express. 🤖 Generated with AI assistance
There was a problem hiding this comment.
Code Review
This pull request updates the execution payload bid validation logic in executionPayloadBid.ts to account for the maximum gossip clock disparity on both ends of the slot range. It replaces the strict slot comparison with millisecond-precise checks relative to the slot boundaries. There are no review comments, and I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // [IGNORE] `bid.slot` is the current slot or the next slot, with a `MAXIMUM_GOSSIP_CLOCK_DISPARITY` | ||
| // allowance on both ends. Equivalent to the spec's `is_within_slot_range(state, bid.slot, 1, ...)`: | ||
| // the clock must be in slot `bid.slot - 1` (bid.slot is the next slot) or `bid.slot` (the current | ||
| // slot), i.e. now within `[start(bid.slot - 1), start(bid.slot + 1)] ± MAXIMUM_GOSSIP_CLOCK_DISPARITY`. | ||
| // Millisecond-precise on purpose: the disparity boundary is sub-slot, so slot-granular helpers | ||
| // (`currentSlotWithGossipDisparity` / `slotWithFutureTolerance`) cannot express it. | ||
| const maxGossipClockDisparityMs = chain.config.MAXIMUM_GOSSIP_CLOCK_DISPARITY; | ||
| if ( | ||
| chain.clock.msFromSlot(bid.slot - 1) < -maxGossipClockDisparityMs || | ||
| chain.clock.msFromSlot(bid.slot + 1) > maxGossipClockDisparityMs | ||
| ) { |
There was a problem hiding this comment.
can we make this a bit less verbose, especially the comment is unnecessary, can you also double check, I believe we have clock helpers for gossip disparity
There was a problem hiding this comment.
Done — trimmed to a single [IGNORE] line matching the neighbouring checks (82c3c44).
Double-checked the gossip-disparity helpers — none expresses this window. Both currentSlotWithGossipDisparity and isCurrentSlotGivenGossipDisparity only treat currentSlot + 1 as valid once we're within MAXIMUM_GOSSIP_CLOCK_DISPARITY of the next-slot boundary, whereas a next-slot bid has to be accepted for the whole current slot. slotWithFutureTolerance / slotWithPastTolerance are slot-granular, so they can't express the sub-slot boundary either. That's why I kept the ms-precise msFromSlot bounds for the current-or-next-slot window.
Address review: trim the multi-line comment on the bid.slot gossip-disparity check to a single spec-style line matching the neighbouring [IGNORE]/[REJECT] checks. No behaviour change; the ms-precise msFromSlot bounds stay, since no clock helper covers the current-or-next-slot window (isCurrentSlotGivenGossipDisparity only accepts the next slot within disparity of the boundary, not for the whole current slot). 🤖 Generated with AI assistance Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
| const currentSlot = chain.clock.currentSlot; | ||
| if (bid.slot !== currentSlot && bid.slot !== currentSlot + 1) { | ||
| // [IGNORE] `bid.slot` is the current or next slot, allowing for `MAXIMUM_GOSSIP_CLOCK_DISPARITY`. | ||
| const maxGossipClockDisparityMs = chain.config.MAXIMUM_GOSSIP_CLOCK_DISPARITY; |
There was a problem hiding this comment.
why do we need to use MAXIMUM_GOSSIP_CLOCK_DISPARITY like that, isn't there an existing clock method we can use for this case? also seems strange to use clock disparity for bid.slot + 1, that's a future slot, also cc @jtraglia it would be strange if your tests force us to do this
There was a problem hiding this comment.
The current spec for this does not mention clock disparity, but it should. Anything which deals with the system clock should factor in the clock disparity, sort of like this. I don't find it strange that the clock disparity is also considered when checking if it's the next slot.
There was a problem hiding this comment.
Fixed in f03631d8c2, using the existing isCurrentSlotGivenGossipDisparity — no inline ms-math, no bid.slot + 1:
if (
!chain.clock.isCurrentSlotGivenGossipDisparity(bid.slot) &&
!chain.clock.isCurrentSlotGivenGossipDisparity(bid.slot - 1)
) {
// IGNORE
}bid.slot is accepted when it's the current slot, or when bid.slot - 1 is the current slot (so bid.slot is the next slot) — both with gossip disparity.
On "isn't there a helper": currentSlotWithGossipDisparity / slotWithFutureTolerance don't fit here because they Math.floor to a slot number, so both sides of the sub-slot boundary collapse to the same value and can't tell 108500ms (valid) from 108501ms (ignore). isCurrentSlotGivenGossipDisparity compares ms directly, so it does distinguish it.
And on applying disparity to the next slot — @jtraglia confirmed above (#9627 (comment)) it should. Verified: all 32 gloas execution_payload_bid gossip reftests pass, including the lower/upper disparity cases.
There was a problem hiding this comment.
Thanks @jtraglia — that's exactly how we read it: clock disparity is the convention for anything comparing against the system clock, even where a specific check doesn't spell it out. We've kept it — the fix uses the existing isCurrentSlotGivenGossipDisparity helper, which applies MAXIMUM_GOSSIP_CLOCK_DISPARITY (bid.slot is the current slot, or bid.slot - 1 is the current slot so bid.slot is next).
Happy to open a small consensus-specs PR adding the explicit "(with a MAXIMUM_GOSSIP_CLOCK_DISPARITY allowance)" to the execution_payload_bid slot check in gloas/p2p-interface.md if you'd like — just let me know.
There was a problem hiding this comment.
Thanks but to be clear, the "current spec" there is the non-executable spec. The executable spec includes this clarification with the is_current_or_next_slot helper. So no need for you to open any new PRs.
There was a problem hiding this comment.
Perfect, thanks for the pointer — that matches what we shipped in f03631d8: isCurrentSlotGivenGossipDisparity(bid.slot) || isCurrentSlotGivenGossipDisparity(bid.slot - 1), i.e. the same is_current_or_next_slot semantics. Good to know it's already in the executable spec — no new specs PR from us then.
There was a problem hiding this comment.
Got it — thanks for the clarification! Makes sense, no PR needed then.
…slot check Address review (nflaig): replace the inline `msFromSlot ± MAXIMUM_GOSSIP_CLOCK_DISPARITY` math with the existing clock helper. `bid.slot` is the current or next slot iff it is the current slot with gossip disparity, or `bid.slot - 1` is (so `bid.slot` is next). Drops the odd `bid.slot + 1` term. The slot-granular helpers (`currentSlotWithGossipDisparity` / `slotWithFutureTolerance`) floor to a slot and collapse the sub-slot boundary, but `isCurrentSlotGivenGossipDisparity` compares ms directly, so it still distinguishes the disparity boundary (108500ms valid vs 108501ms ignore). Verified: all 32 gloas execution_payload_bid gossip reftests pass, incl. the lower/upper disparity cases. 🤖 Generated with AI assistance Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…line Per review (nflaig preferred less verbosity on this line): collapse the 2-line comment on the bid slot check to one, keeping the current-or-next-slot + `MAXIMUM_GOSSIP_CLOCK_DISPARITY` essence plus a brief `bid.slot - 1` hint. Comment-only; no behaviour change. 🤖 Generated with AI assistance Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…9633) **Motivation** The Gloas `proposer_preferences` gossip validation `[IGNORE] preferences.proposal_slot has not already passed` compares `proposal_slot` against the exact `current_slot`. A preference whose `proposal_slot` falls within `MAXIMUM_GOSSIP_CLOCK_DISPARITY` of the next-slot boundary is accepted as `valid` when it should be `ignore`d. The boundary is sub-slot (a 1 ms shift flips the outcome), so the integer `current_slot` comparison cannot express it. This is the same class of gap fixed for the `execution_payload_bid` slot check in #9627. **Description** Use `slotWithFutureTolerance(MAXIMUM_GOSSIP_CLOCK_DISPARITY / 1000)` as the `current_slot` reference, matching the existing convention in `verifyPropagationSlotRange` (`attestation.ts`). `MAXIMUM_GOSSIP_CLOCK_DISPARITY` is a global gossip allowance that applies even where the [spec](https://github.com/ethereum/consensus-specs/blob/master/specs/gloas/p2p-interface.md) states the check as a plain `preferences.proposal_slot > current_slot` (as confirmed for the analogous bid check). **Verification** Ran the Gloas networking gossip reference tests from [consensus-specs PR #5294](ethereum/consensus-specs#5294): - `gossip_proposer_preferences__ignore_slot_outside_disparity` → now `ignore` (was `valid`) - `gossip_proposer_preferences__valid_slot_at_disparity_edge` → still `valid` - `gossip_proposer_preferences__valid_at_lookahead_upper_edge` → still `valid` Full `gossip_proposer_preferences`: **13/13 minimal + 13/13 mainnet**, no regressions in the rest of the Gloas networking suite. 🤖 Generated with AI assistance --------- Co-authored-by: lodekeeper <lodekeeper@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
|
🎉 This PR is included in v1.45.0 🎉 |
Problem
The Gloas execution payload bid gossip validation checked the slot with an exact match and no
MAXIMUM_GOSSIP_CLOCK_DISPARITYallowance:Every other gossip slot check in Lodestar applies the disparity allowance (see
block.ts,blobSidecar.ts,attestation.ts). Without it, a bid that arrives withinMAXIMUM_GOSSIP_CLOCK_DISPARITYof a slot boundary is wronglyIGNOREd.Fix
Implement the spec's
is_within_slot_range(state, bid.slot, 1, current_time_ms)semantics: the current time must fall within[start(bid.slot - 1), start(bid.slot + 1)], extended by± MAXIMUM_GOSSIP_CLOCK_DISPARITYon both ends — i.e. the clock is in slotbid.slot - 1(bid.slot is the next slot) orbid.slot(the current slot).Implemented with
chain.clock.msFromSlot(...)because the disparity boundary is sub-slot: the passing/failing reftests are exactly 1 ms apart at the edge (e.g.95500valid vs95499ignore;108500valid vs108501ignore). The slot-granular helpers (currentSlotWithGossipDisparity/slotWithFutureTolerance) floor to integer slots and produce identical values on either side of that 1 ms boundary, so they cannot express this check — a millisecond-precise comparison is required.Testing
Found by running the consensus-specs #5294 Gloas networking reference tests (spec
v1.7.0-alpha.12). All fivegossip_execution_payload_bidslot cases now pass (minimal + mainnet):valid_slot_at_lower_disparity,valid_slot_at_upper_disparity— nowvalid(wereignore)ignore_slot_outside_lower_disparity,ignore_slot_outside_upper_disparity,ignore_slot_too_far_future— stillignore(no regression on the 1 ms-outside boundary)🤖 Generated with AI assistance