Skip to content

Introduce separate payload availability deadline + make naturalSet/Map only write ordered - #10749

Merged
rolfyone merged 10 commits into
Consensys:masterfrom
zilm13:payload-availability
Jun 2, 2026
Merged

Introduce separate payload availability deadline + make naturalSet/Map only write ordered#10749
rolfyone merged 10 commits into
Consensys:masterfrom
zilm13:payload-availability

Conversation

@zilm13

@zilm13 zilm13 commented May 25, 2026

Copy link
Copy Markdown
Contributor

PR Description

Implementation of ethereum/consensus-specs#5212
Part of alpha.8 spec

Fixed Issue(s)

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

Note

High Risk
Changes Gloas payload attestation semantics and execution-payload import timing, which affect validator votes and fork-choice-related behavior at the payload deadline.

Overview
Implements Gloas alpha.8 timing for when an execution payload counts as present for payload attestation (payload_present), instead of treating any recently gossiped payload as present.

Spec & config: Adds PAYLOAD_DUE_BPS (7500) and get_payload_due_ms / Spec.getPayloadDueMillis(), alongside shared slot-time helpers isTimeReached and isBeforeTimeInSlot (also used by slot processing). Spec reference metadata is updated to drop temporary exceptions for these items.

Execution payload handling: ExecutionPayloadManager splits “recently seen” (dedup) from isExecutionPayloadAvailableForPayloadAttestation, which is true only if a validated payload was recorded strictly before the payload-due offset in the slot (earliest arrival wins; late duplicates do not flip the vote; IGNORE after an earlier ACCEPT can still mark availability). Pending payloads per block/builder keep multiple candidates with timestamps and bounded caches.

Validator API: createPayloadAttestationData uses the new availability check for payload_present.

Infrastructure: LimitedMap.createSynchronizedNatural / LimitedSet.createSynchronizedNatural evict on insert/update order, not LRU on read—used for the new execution-payload caches.

Reviewed by Cursor Bugbot for commit 4e120df. Bugbot is set up for automated code reviews on this repo. Configure here.

@zilm13 zilm13 mentioned this pull request May 25, 2026
17 tasks
@rolfyone

Copy link
Copy Markdown
Contributor

i wont claim this as my feedback, as i just asked codex but worth considering

Findings

[P1] ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/execution/DefaultExecutionPayloadManager.java:144 keeps only the earliest pending envelope before block-dependent validation has run. An early invalid envelope can occupy the pending slot for a BlockRootAndBuilderIndex; a later valid envelope for the same key is discarded, then only the invalid one is retried when the block imports. That can leave the payload unimported and payload_present=false. Keep pending candidates until one validates, or separate earliest arrival tracking from the payload chosen for validation.

[P2] ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/execution/DefaultExecutionPayloadManager.java:124 records availability only when validation returns ACCEPT. Since gossip validation runs asynchronously, a post-deadline duplicate can validate first and mark the payload as seen; an actually earlier valid copy then becomes IGNORE in ExecutionPayloadGossipValidator and never updates the availability cache. That can incorrectly vote payload_present=false even though a valid envelope arrived before get_payload_due_ms().

@zilm13

zilm13 commented May 26, 2026

Copy link
Copy Markdown
Contributor Author

@rolfyone what do you think without Codex on my changes to alternative of LRU map, which is called "natural" which is a part of this PR?
First time I've introduced it I've expected this behavior: last write puts item to the top (last to prune), write of the same item. again puts it to the top too. Reads doesn't matter at all. There are simple tests in PR for clarity. So when I introduced it before I've expected this exact behavior to be more appropriate for the things we need to contain some fresh (gossip stuff) rather than LRU which could be affected for example with RPC requests etc. It was a bit broken before (write of the same item didn't update its position) which is also fixed in this PR.

Also, addressed feedback. A lot of structures to hold everything but I didn't come to something simpler fast.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit 56239dc. Configure here.

private PendingExecutionPayloads(final Map<Bytes32, PendingExecutionPayload> payloads) {
final Map<Bytes32, PendingExecutionPayload> limitedPayloads =
LimitedMap.createSynchronizedNatural(UNVALIDATED_EXECUTION_PAYLOADS_CACHE_SIZE);
limitedPayloads.putAll(payloads);

@rolfyone rolfyone May 26, 2026

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.

probably worth a size sanity check here, just to ensure that the new limitedPayloads won't exceed the cache size - would be a good place to throw if that happens... this was basically what codex was concerned about, but it makes sense to sanity check given the 'map' coming in.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've added protection on putAll, I think it's better to prune rather than throw

@zilm13 zilm13 mentioned this pull request Jun 1, 2026
2 tasks
@rolfyone

rolfyone commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

did a scan, looked ok to me.... codex had one concern which seems like its worth investigating

[P2] ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/execution/DefaultExecutionPayloadManager.java:348 can still drop the only timely valid pending payload. PendingExecutionPayloads is bounded with write-order pruning, so once more than 64 candidates arrive for the same block root/builder index, the earliest candidate is evicted. Because these candidates are stored before block-dependent validation runs, a valid envelope seen before get_payload_due_ms() can be pushed out by later invalid envelopes; when the block imports, only the remaining invalid candidates are retried, and Teku may vote payload_present=false despite having seen a valid payload before the deadline. I’d preserve pre-deadline candidates until block validation rejects them, or use an eviction policy that does not replace earlier unvalidated candidates with later ones for this deadline-sensitive path.

@zilm13

zilm13 commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

@rolfyone
That's the end of my discussion with AI. My feeling is we shouldn't change this:

› so we could have 2 eviction policies, x and y
  1 x
  1 x
  1 x
  1 x y
  1 x y
  1   y
  1   y
  1   y
  [ block arrives ]
  both could be DoSed, but in normal world we expect payload to arrive later, so y is preferred, right?


• Yes. If x means “keep earliest candidates” and y means “keep latest candidates”, then y is the more natural policy for the expected honest flow.

  Reason: for builder/Gloas blocks, the useful payload is expected after the block/bid, so if the pending pool has to choose under pressure, keeping later candidates is more
  likely to preserve the real payload than keeping the first things seen. Earliest-retention can be DoSed by sending 64 bad candidates first, then the valid payload gets
  dropped.

  The reviewer’s concern is still real, but it is the opposite attack ordering: valid payload arrives timely, then bad later candidates evict it. Current latest-retention
  handles “bad first, valid later” better; earliest-retention handles “valid first, bad later” better. Both are bounded-cache DoS tradeoffs.

  The better compromise may be: keep latest candidates, but make pre-deadline candidates higher priority than post-deadline candidates. That preserves the normal “payload
  arrives later” preference while preventing post-deadline junk from evicting something that could still prove payload_present=true. It does not solve unlimited pre-deadline
  spam, but it is a cleaner fit than switching fully to earliest-retention.

@rolfyone rolfyone left a comment

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.

LGTM thanks for looking into the codex comment, sounds like we can live with it.

@rolfyone
rolfyone enabled auto-merge (squash) June 2, 2026 23:23
@rolfyone
rolfyone merged commit 57f5a11 into Consensys:master Jun 2, 2026
65 of 68 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Jun 2, 2026
@zilm13
zilm13 deleted the payload-availability branch June 3, 2026 08:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants