Skip to content

cl: fix Gloas checkpoint sync with external execution clients - #22683

Open
domiwei wants to merge 12 commits into
mainfrom
kewei/gloas-external-el-sync
Open

cl: fix Gloas checkpoint sync with external execution clients#22683
domiwei wants to merge 12 commits into
mainfrom
kewei/gloas-external-el-sync

Conversation

@domiwei

@domiwei domiwei commented Jul 23, 2026

Copy link
Copy Markdown
Member

Summary

Fix the original standalone-Caplin Gloas checkpoint-sync path when using an external execution client.

  • Preserve the skipped-slot checkpoint state root across fork-choice disk persistence.
  • Validate Gloas anchor and downloaded payloads through the external EL even when insertion is unavailable.
  • Encode an empty Engine API transaction list as an SSZ array instead of null.
  • Bound remote-EL and pending-envelope retry work, release the fork-choice mutex during NewPayload, and refresh identity before committing the result.
  • Terminate missing-root pending envelope retries once the claimed payload slot is before finalized history or implausibly ahead of the local clock, while retaining recent, data-unavailable, storage, and EL-retry work.
  • Preserve the authoritative finalized Gloas root across the production checkpoint restart artifact.
  • Bound payload-attestation validation memory by collapsing same-root state builds, admitting one full-state build at a time, and caching compact validation contexts.
  • Start PeerDAS only after data-column gossip topics are registered.

Scope

This PR has been reset to its original three behaviors plus the minimal lifecycle correction required to make external-EL validation safe, the narrow pending-envelope terminal rule required to stop finalized side-branch retries, the payload-attestation OOM fix, and the PeerDAS startup-order fix required by the Gloas devnet. The later downloader recovery, broader envelope durability/canonical-ingress lifecycle work, and live resolver work were removed from this diff and preserved in draft #23249.

The reset reduces the cumulative diff from 60 files / 10,904 changed lines to 31 files / 1,959 changed lines before the focused stale-envelope regression coverage. The additional files are safety dependencies of the original external-EL/checkpoint path plus bounded payload-attestation validation and ordered PeerDAS startup needed for stable devnet operation, not the later downloader/resolver feature set.

The pending-envelope rule uses the Gloas finalized epoch start as its lower bound. A next-slot envelope is retained only within MAXIMUM_GOSSIP_CLOCK_DISPARITY; farther future slots are rejected before slot-time conversion, avoiding attacker-controlled arithmetic aliases. Queue insertion remains atomic with the missing-block observation, while retries do not overwrite concurrent replacements. Gossip and local-self-build origins use the same lifecycle rule, and nil index-repair sentinels remain unchanged.

Validation

  • TDD regression: TestRetryPendingExecutionPayloadEnvelopesDropsMissingBlockOlderThanFinality failed before the production change because the stale entry remained queued, then passed after the terminal rule was implemented.
  • go test ./cl/phase1/forkchoice -count=1
  • go test ./cl/phase1/network/services -count=1
  • go test ./cl/phase1/stages -count=1
  • go test -race ./cl/phase1/forkchoice -run 'TestRetryPendingExecutionPayloadEnvelopes(KeepsConcurrentReplacement|DropsMissingBlockOlderThanFinality|DropsFarFutureSlot)' -count=1
  • go test -count=1 -timeout=15m ./cl/das ./cl/spectest/consensus_tests ./cmd/caplin/caplin1 ./cl/phase1/core/checkpoint_sync ./cl/phase1/execution_client ./cl/phase1/forkchoice ./cl/phase1/forkchoice/fork_graph ./cl/phase1/network/services ./cl/phase1/stages ./execution/engineapi/engine_types
  • go test -race -count=1 -timeout=20m ./cl/phase1/core/checkpoint_sync ./cl/phase1/execution_client ./cl/phase1/forkchoice ./cl/phase1/forkchoice/fork_graph ./cl/phase1/network/services ./cl/phase1/stages ./execution/engineapi/engine_types
  • Gloas Kurtosis: gloas-caplin-mixed and gloas-three-cl-mixed passed using glamsterdam-devnet-7 CL/VC images.
  • make lint repeatedly until clean
  • make erigon integration
  • git diff --check

This history rewrite is intentional so the original fix can be reviewed and merged independently.

@domiwei
domiwei force-pushed the kewei/gloas-external-el-sync branch 3 times, most recently from c4c3417 to 8fd2aaf Compare July 31, 2026 08:57
@domiwei
domiwei force-pushed the kewei/gloas-external-el-sync branch from 8fd2aaf to 2ffc4ae Compare July 31, 2026 09:12
@domiwei domiwei changed the title cl: fix Gloas checkpoint sync with external execution clients cl: stabilize Gloas sync with external execution clients Jul 31, 2026
@domiwei
domiwei changed the base branch from main to kewei/gloas-progressive-alpha12 July 31, 2026 09:13
@domiwei
domiwei force-pushed the kewei/gloas-external-el-sync branch 2 times, most recently from 7fa3c7e to 38844bf Compare July 31, 2026 22:25
@domiwei
domiwei force-pushed the kewei/gloas-progressive-alpha12 branch from 1b55705 to 0b0a9d8 Compare August 3, 2026 08:46
@domiwei
domiwei force-pushed the kewei/gloas-external-el-sync branch from 38844bf to 18701d4 Compare August 3, 2026 08:46
@domiwei
domiwei changed the base branch from kewei/gloas-progressive-alpha12 to main August 3, 2026 08:47
@domiwei
domiwei requested a review from Copilot August 3, 2026 10:20

Copilot AI 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.

Pull request overview

Stabilizes Caplin’s Gloas sync when paired with an external execution client by bounding/serializing expensive validation and recovery paths, improving envelope/data-availability handling, and reducing stall/OOM risk in fork-choice, networking, and stages.

Changes:

  • Introduce shared/ bounded Engine API NewPayload validation coordination (including releasing fork-choice mutex during external EL calls) and tighten payload-attestation validation to avoid redundant work.
  • Harden Gloas envelope fetching/validation (HTTP bounds, exact-root checks, malformed-candidate handling) and implement fair, bounded skipped-envelope recovery batching.
  • Adjust runtime sequencing (PeerDAS explicit Start) and observer aggregate verification logic.

Reviewed changes

Copilot reviewed 34 out of 35 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
cmd/caplin/caplin1/run.go Starts PeerDAS explicitly and updates constructor usage.
cl/spectest/consensus_tests/fork_choice.go Updates PeerDAS constructor usage in spectests.
cl/phase1/stages/stage_history_download.go Adds bounded, batched recovery for skipped envelopes with deadlines and exact-root validation.
cl/phase1/stages/stage_history_download_test.go Tests fairness/partial-success behavior for batched skipped-envelope recovery.
cl/phase1/stages/gloas_payload_test.go Updates/extends tests for shared payload validation coordinator and anchor validation behavior.
cl/phase1/stages/forward_sync.go Routes payload validation through shared coordinator; validates via execution client when present.
cl/phase1/stages/clstages.go Wires a shared payload validation coordinator into stage config (prefers forkchoice’s coordinator).
cl/phase1/stages/chain_tip_sync.go Switches gating from “retry local EL” to “validate with any execution client”; shares coordinator.
cl/phase1/network/services/payload_attestation_service.go Deduplicates in-flight per-validator attestation validations to limit duplicate work.
cl/phase1/network/services/payload_attestation_service_test.go Adds concurrency/serialization/retry tests for attestation validation deduplication.
cl/phase1/network/services/execution_payload_service.go Queues envelopes when PeerDAS column data is unavailable; improves pending processing semantics.
cl/phase1/network/services/execution_payload_service_test.go Adds tests for pending envelope behavior when data availability is delayed/fails.
cl/phase1/network/services/aggregate_and_proof_service.go Introduces shouldVerifyAggregate to handle observer behavior consistently.
cl/phase1/network/services/aggregate_and_proof_service_test.go Tests observer aggregate verification gating.
cl/phase1/network/beacon_downloader.go Filters incomplete blocks before processing and validates/fetches missing envelopes with bounded HTTP reads.
cl/phase1/network/backward_beacon_downloader.go Improves Gloas FULL/EMPTY inference via canonical lineage lookahead; bounds skipped-full tracking and HTTP reads; validates envelopes against blocks.
cl/phase1/network/backward_beacon_downloader_test.go Adds extensive coverage for lookahead, envelope validation/fallback, bounded reads, and skipped-full memory budget.
cl/phase1/forkchoice/payload_attestation_validation.go Adds cached, singleflight validation contexts for payload attestations to avoid repeated full-state work.
cl/phase1/forkchoice/payload_attestation_validation_test.go Tests caching/singleflight/bounds and signature/position validation.
cl/phase1/forkchoice/on_payload_attestation_message.go Refactors payload attestation verification to use the new cached validation context.
cl/phase1/forkchoice/on_execution_payload.go Releases fork-choice mutex around NewPayload, coalesces/bounds validations, and refreshes state after unlock.
cl/phase1/forkchoice/on_execution_payload_test.go Adds tests for mutex release, coalescing keys, bounded concurrency, cancellation, and panic recovery.
cl/phase1/forkchoice/gloas_weight_tree.go Reuses spare slice capacity safely (clearing newly exposed entries).
cl/phase1/forkchoice/gloas_weight_tree_test.go Tests spare-capacity reuse and clearing behavior.
cl/phase1/forkchoice/forkchoice.go Stores payload attestation context cache and shared payload validation coordinator in forkchoice.
cl/phase1/forkchoice/fork_graph/fork_graph_test.go Tests anchor state-root caching behavior across cases.
cl/phase1/forkchoice/fork_graph/fork_graph_disk.go Fixes anchor/previous state-root handling for Gloas and skipped-slot nuances.
cl/phase1/forkchoice/fork_graph/fork_graph_disk_fs.go Writes correct state root for skipped-slot states when dumping to disk.
cl/phase1/execution_client/payload_validation_coordinator.go New: bounds/coalesces distinct/same-key NewPayload calls to an execution client.
cl/phase1/execution_client/payload_validation_coordinator_test.go Tests coordinator concurrency bounds and panic behavior.
cl/phase1/execution_client/execution_client_engine.go Ensures Engine API payload transactions encodes as JSON array (not null).
cl/phase1/execution_client/execution_client_engine_test.go Tests JSON encoding of transactions as an array across forks.
cl/das/peer_das.go Adds explicit Start(ctx) to control when PeerDAS goroutines/subscriptions begin.
cl/das/peer_das_start_test.go Tests that PeerDAS subscribes only after Start and only once.
cl/das/mock_services/peer_das_mock.go Regenerates PeerDAS mock to include Start(ctx).
Files not reviewed (1)
  • cl/das/mock_services/peer_das_mock.go: Generated file

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread cl/phase1/network/beacon_downloader.go Outdated

Copilot AI 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.

Pull request overview

Copilot reviewed 35 out of 36 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • cl/das/mock_services/peer_das_mock.go: Generated file

Copilot AI 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.

Pull request overview

Copilot reviewed 35 out of 36 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • cl/das/mock_services/peer_das_mock.go: Generated file

@domiwei
domiwei force-pushed the kewei/gloas-external-el-sync branch from 2c715f2 to b85bb15 Compare August 3, 2026 23:20
@domiwei
domiwei requested a review from Copilot August 3, 2026 23:21

Copilot AI 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.

Pull request overview

Copilot reviewed 35 out of 36 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • cl/das/mock_services/peer_das_mock.go: Generated file

@domiwei
domiwei marked this pull request as ready for review August 3, 2026 23:57
@domiwei domiwei changed the title cl: stabilize Gloas sync with external execution clients cl: fix Gloas checkpoint sync with external execution clients Aug 13, 2026
@domiwei

domiwei commented Aug 13, 2026

Copy link
Copy Markdown
Member Author

@yperbasis I reset this PR to its original three-commit scope so it can receive a focused re-review:

  • preserve the skipped checkpoint state root
  • validate Gloas payloads with an external EL
  • encode empty Engine API transactions as an array

The later downloader, recovery, coordinator, payload-attestation, DA/PeerDAS, persistence, and resolver work—and the review discussion attached to those changes—has moved to stacked draft #23249.

The current diff is now 19 files / 1,045 changed lines instead of 60 files / 10,904 changed lines. The retained safety work is limited to this path: remote and pending retries are budgeted, NewPayload no longer holds the global fork-choice lock, and the authoritative finalized Gloas root survives the production checkpoint restart artifact. The rewritten head was rebuilt on the latest main and passed the affected normal/race suites, repeated lint runs, and make erigon integration.

The long status comment above describes the extracted branch and is superseded for this PR. Please review the new small diff when convenient.

@domiwei

domiwei commented Aug 13, 2026

Copy link
Copy Markdown
Member Author

@yperbasis I reset this PR to its original three-commit scope so it can receive a focused re-review:

  • preserve the skipped checkpoint state root
  • validate Gloas payloads with an external EL
  • encode empty Engine API transactions as an array
  • prevent payload-attestation validation from copying/replaying state per concurrent gossip message (the devnet OOM fix)

The later downloader, recovery, DA/PeerDAS, persistence/journal, canonical-ingress, and resolver work—and the review discussion attached to those changes—has moved to stacked draft #23249. The payload-attestation OOM cluster remains here because it is required for stable Gloas devnet operation.

The current diff is now 26 files / 1,855 changed lines instead of 60 files / 10,904 changed lines. The retained safety work is limited to this path: remote and pending retries are budgeted, NewPayload no longer holds the global fork-choice lock, the authoritative finalized Gloas root survives restart, and payload-attestation state-context construction is bounded and shared. The rewritten head passed the affected normal/race suites, repeated lint runs, make erigon integration, and both Gloas Kurtosis suites using glamsterdam-devnet-7 images.

The long status comment above describes the extracted branch and is superseded for this PR. Please review the new small diff when convenient.

@domiwei

domiwei commented Aug 13, 2026

Copy link
Copy Markdown
Member Author

@yperbasis @AskAlexSharov Review-feedback disposition after the scope reset:

Retained in this PR and addressed

  • OnBlock / external NewPayload consistency: pending-envelope application now happens after the block bookkeeping commit; the fork-choice mutex is released only around the EL call, state/block identity is refreshed after relock, duplicate completion is rechecked, failed work is retained and retried, and block-event optimism is emitted after the result is known. The reverse/concurrent and DB-index retry paths have focused tests.
  • External EL retry monopolization: pending work is capped at 32 attempts under one aggregate 2 s context, failures rotate fairly across both pending origins, cancellation and DB failures are retained, and no network call holds f.mu. The synchronous bound is intentional so completion is visible before later recovery/verification phases; it cannot expand to one 30 s timeout per queued item.
  • Forward validation for local versus external EL: validation is intentionally based on the presence of an execution client, while SupportInsertion() remains the separate insertion/flush capability. Insertion alone does not establish the NewPayload validation result needed by fork choice.
  • Skipped/legacy checkpoint identity: the fork-graph constructor establishes the authoritative previous state root before dumping. The production finalized checkpoint now persists a state-digest-bound root record durably; unsafe legacy same-slot Gloas local resumes fall back instead of inventing an identity. Zero-cache skipped-slot and production restart paths are covered.
  • Payload-attestation OOM: same-message service validation is serialized; state-context construction is singleflight per block root, globally limited to one full-state build, and cached as compact data with a 128-entry bound. Cancellation, invalid-to-valid competition, panic cleanup, cache publication, and vote races are covered.
  • PeerDAS topic registration: PeerDAS construction no longer subscribes before gossip topics exist. An idempotent Start(ctx) is invoked after RegisterGossipServices, with a regression test proving subscriptions begin only at that boundary.
  • Empty Engine API transactions: the required JSON field is always encoded as an array, including an empty one.

Removed from this PR, not dismissed

The backward/forward downloader rewrite, lookahead trust and EMPTY/FULL inference, HTTP/P2P recovery budgets, skipped-envelope persistence, live envelope resolver, envelope journal/prune/startup recovery, canonical-ingress hardening, and observer aggregate behavior are no longer in #22683. They are preserved in stacked draft #23249, which is intentionally not ready to merge. The related High/Medium feedback remains follow-up work there rather than being claimed as fixed by this PR.

This includes the inline findings about local failures banning peers, nil-engine stage blocking, repeated full-state copies, prune error/scan bounds, envelope-before-block handling, Gloas EMPTY skipping, stale lookahead trust, one-block forward progress, resolver hot-path copying, startup scratch artifacts, repeated SSZ hashing, and observer aggregate cost. I replied to each thread with its exact disposition and resolved it only because the referenced code is absent from the current diff.

Low/nit disposition

  • Duplicate block reads and envelope validation, dead downloader branches/helpers, response-body lifecycle, retry-sleep duplication, batch rehashing, and metric-placement nits belong to the extracted downloader/journal/coordinator work and are absent from this diff.
  • The lazy payload-validation coordinator path is also absent; the slim implementation uses explicit initialized admission state.
  • These were not ignored as “unnecessary”; they were excluded because their production code was excluded. They remain review input for the follow-up PRs if those clusters are reintroduced.

Current core diff: 31 files, +1756/-203 (1,959 changed lines). The exact head passed the PeerDAS regression and affected packages, lint twice, and make erigon integration; the previous core head passed both devnet-7 Gloas Kurtosis jobs, which will rerun for this update.

@domiwei

domiwei commented Aug 13, 2026

Copy link
Copy Markdown
Member Author

Scope correction: the small PeerDAS startup-order fix is now restored in #22683 as commit fc350dbed1.

The devnet warning was reproducible: NewPeerDas subscribed before RegisterGossipServices created the data-column topic handles, producing topic not found and potentially missing the initial archive/custody subscriptions. PeerDAS now has an idempotent Start(ctx) called after gossip registration; the focused regression and affected packages pass.

The core diff remains close to the requested size at 31 files / 1,959 changed lines. The larger DA retry/resolver lifecycle remains excluded: block state not found is already retained by the core pending-envelope path, while missing FULL envelopes are retried under the bounded chain-tip pass and actively refetched by root. No broader resolver/journal/downloader cluster was restored with this change.

Copilot AI 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.

Pull request overview

Copilot reviewed 30 out of 31 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • cl/das/mock_services/peer_das_mock.go: Generated file

Comment thread cl/phase1/forkchoice/fork_graph/fork_graph_disk.go

@yperbasis yperbasis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Combined review: deep automated review + Codex findings + the open Copilot thread, all verified against head fc350dbe.

High — fix before merge

  1. Anchor state-root selection breaks restartscl/phase1/forkchoice/fork_graph/fork_graph_disk.go:154. Two sides of one defect:

    • Legacy datadir (pre-PR finalized state: same-slot header root filled, no sidecar): RestoreFinalizedStateRoot returns nil (finalized_state_root.go:49 errors only when header.Root == 0), PeekPreviousStateRoot() == 0, so the constructor recomputes HashSSZ() over the Root-filled state → non-canonical root → the first transitionSlot poisons state_roots and every child block fails state-root verification. Reproduced independently three times (two probe tests, one Codex repro).
    • New-format file at a block slot (header.Root == 0, sidecar present): the anchorHeader.Root == 0 disjunct discards the restored sidecar root and recomputes, while anchorRoot (line 147) was already derived from that same sidecar root via BlockRoot()'s previousStateRoot preference — two sources of truth for the anchor, plus one wasted full-state merkleization per startup. This is the open Copilot thread (#22683 (comment)).

    One rewrite fixes both: trust a non-zero PeekPreviousStateRoot(); when absent, trust a non-zero same-slot anchorHeader.Root (legacy artifact); otherwise HashSSZ(). Note the 'legacy block slot' test pre-sets the cached root, which real legacy files never have — add a case without it.

  2. Admission-wait expiry persists envelopes the EL never executedcl/phase1/forkchoice/on_execution_payload.go:294. <-ctx.Done() while waiting for the cap-1 admission slot returns PayloadStatusNone, which is mapped to errELBehind, and applyEnvelopeCoordinated still processes, dumps, and queues the envelope → HasEnvelope = true with zero EL contact. Under the shared 2s retry budget (chain_tip_sync.go:697) one slow NewPayload makes this systematic, and PTC duty then answers PAYLOAD_PRESENT off HasEnvelope (epbs.go:219). Distinguish admission-wait cancellation (requeue, do not persist) from a real EL answer.

  3. Local ctx errors ban innocent peerscl/phase1/network/services/payload_attestation_service.go:234. beginValidation's duplicate-key wait and the validation-context build waits (payload_attestation_validation.go:100, :129) return raw ctx.Err(); the gossip manager REJECTs and bans the relaying peer for any non-ignore error (gossip_manager.go:185-196). Codex reproduced context deadline exceeded escaping this path. Map local cancellation/deadline errors to services.ErrIgnore.

  4. Failed pending envelopes are immortalcl/phase1/forkchoice/on_block.go:548. On apply error the entry is neither dequeued nor expired (base removed it before its single attempt); block-not-found re-Adds and the Contains+Get touches promote dead entries to MRU in the 128-cap LRU with no TTL → wasted GetState/BLS/EL work plus warn spam every cycle, forever. Also execution_payload_service.go:140-155 queues unknown-root envelopes before any builder-signature check, keyed by root, so junk can overwrite a genuine queued envelope. Dequeue-or-backoff on hard failure, add an expiry, verify the builder signature before queueing.

Medium — degraded-mode hazards

  1. SupportInsertion gate dropped in forward synccl/phase1/stages/forward_sync.go:213. Local ELs now pay a serialized per-envelope NewPayload (InsertBlock per envelope) while the same blocks are batch-inserted two lines below, plus a duplicate BLS verify per envelope. A spurious EL INVALID on a historical envelope wedges sync: children hard-fail on the cached Invalidated status and each retry bans a peer. Restore the distinction, or skip NewPayload when insertion is available.

  2. Admission-channel bypasses and shared budgetcl/phase1/stages/chain_tip_sync.go:518 and forward_sync.go:652 call NewPayload directly, so a retry can validate the same payload concurrently with a gossip validation — the duplication the admission channel exists to prevent. Gossip validation meanwhile waits in the admission with the deadline-free pubsub ctx, and RetryPendingExecutionPayloadEnvelopes can consume the whole 2s budget, starving the other two retry stages. Route the direct sites through the admission helper; give the gossip wait a short timeout mapping to ErrIgnore + requeue; per-stage budgets.

Low — cleanups

  1. cl/phase1/stages/forkchoice.go:309: sha256 over the full snappy-encoded state runs twice per save and twice per load — compute the digest once and pass it through.
  2. on_block.go:558: the completedByAnother equality check does a disk read plus HashSSZ of both envelopes; comparing eth2Roots.Get(blockRoot) against pending.Message.Payload.BlockHash (or the two signatures) is O(1).
  3. Duplication: the post-yield revalidation block is copy-pasted between applyEnvelopeCoordinated (:443-462) and applyLocalSelfBuildEnvelopeCoordinated (:659-675); replaceDurableFile + syncDirectory is the codebase's third atomic-write/fsync-dir copy (db/state/commitment_convert.go:963, common/dbg/experiments.go:337); RestoreFinalizedStateRoot re-derives the record format EncodeFinalizedStateRoot defines — verify by re-encoding instead.
  4. on_block.go:523: the Contains + discarded-result Get pairs read as dead code but are the retry-rotation mechanism (LRU recency touch); an explicit requeue structure or a named helper would prevent someone "fixing" them to Peek.
  5. cl/phase1/forkchoice/forkchoice.go:1101: DrainPendingELPayloads (unlimited) has only test callers and its len > limit branch is unreachable; point the tests at DrainPendingELPayloadsLimit and delete it.
  6. cl/phase1/forkchoice/CLAUDE.md:43: still maps on_execution_payload.go to applyEnvelopeLocked, renamed to applyEnvelopeCoordinated in this PR.

@domiwei

domiwei commented Aug 14, 2026

Copy link
Copy Markdown
Member Author

Review follow-up for 969925928d (cl: harden Gloas payload recovery and validation).

I went through the latest feedback and kept the changes within the Gloas payload validation/recovery scope. The actionable findings are addressed as follows:

  • Run the complete CL envelope transition validation before engine_newPayload, so a CL-invalid reveal cannot trigger EL work or invalidate the beacon header. Local self-build ingress now also rejects a nil payload before lookup, queueing, or EL access.
  • Make execution-payload index persistence idempotent and retry-safe. Same-root writes are coalesced, retries use the authoritative envelope persisted by forkchoice, root-only repair survives temporary disk-read failures, and cancellation/panic/transient storage paths do not report false success or discard repair ownership. Pending OnBlock writes use the same coordinator.
  • Bound payload-attestation validation work by protocol cardinality while preserving same-key coalescing and a retry candidate. Context construction is bounded across distinct roots, with cancellation and panic cleanup covered.
  • Recover and verify envelopes from the selected fork-choice head rather than the highest-seen side branch. The scans now use bounded cursors, retain partial/failed work, reset on reorgs, preserve progress across direct extensions, process the immediate frontier first, and deduplicate it within the 32-item cycle limit.
  • Rotate the first retry class across cycles so pending-envelope, pending-EL, and unverified-anchor work each periodically receive the full remaining two-second budget without increasing concurrency or the total cycle budget.
  • Restrict the finalized-state root sidecar behavior to Gloas and later, preserving the legacy pre-Gloas checkpoint path. Anchor restoration also handles legacy non-zero header roots without hashing the already-populated root field.

Validation completed:

  • make lint twice: clean (0 issues)
  • make erigon integration: passed
  • focused forkchoice, stages, network/services, checkpoint-sync, and fork-graph tests: passed
  • race-focused tests for the new singleflight, cancellation, retry, attestation admission, and cursor paths: passed
  • adversarial Standards/Spec, lifecycle/progress, and boundary reviews converged with no remaining in-scope actionable findings

I intentionally did not fold unrelated pre-existing behavior or broad refactors into this update.

@domiwei
domiwei requested review from AskAlexSharov and yperbasis and a balanced review from Copilot August 14, 2026 08:27

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI 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.

Pull request overview

Copilot reviewed 32 out of 33 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • cl/das/mock_services/peer_das_mock.go: Generated file

Comment thread cl/phase1/stages/chain_tip_sync.go

@yperbasis yperbasis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Requesting changes for two correctness issues:

  1. The selected Gloas head is not requested when its envelope is missing. recoverMissingEnvelopes scans only parents, and pollForEnvelope only checks local storage. If gossip misses the head envelope, FCU keeps the inherited execution hash until a child arrives. This is covered by the existing open thread: #22683 (comment)
  2. The payload-attestation validation bound can ignore a valid competing message before any candidate validates. See the inline comment.

The focused affected-package tests pass, but neither scenario is covered.

Comment thread cl/phase1/network/services/payload_attestation_service.go Outdated
@domiwei

domiwei commented Aug 15, 2026

Copy link
Copy Markdown
Member Author

Addressed the two latest review findings in c005feb.

Selected-head envelope recovery:

  • A missing selected Gloas head now gets one root-specific peer request during the existing 2s head wait.
  • The request is gated by the selected block actual version, so pre-Gloas heads are unchanged.
  • The ancestor recovery cursor remains independent, so a legitimate EMPTY head does not pin ancestor progress.
  • The stage no longer joins a late requester after the deadline, and a response arriving after cancellation is not applied.

Payload-attestation first-valid handling:

  • Removed the pre-signature leader/one-waiter slot that allowed forged candidates to displace a later valid message.
  • Full validation admission now applies cancellation-aware backpressure instead of immediately dropping the next candidate.
  • After PTC membership and BLS verification, fork choice atomically commits the first valid (slot, validator) and its votes under the vote lock, including candidates for different block roots.
  • The seen state resets forward by slot, rejects stale completions, and is bounded by the 512-member PTC. No candidate pool was added.

Deliberate scope boundary: hardening an internal requester that permanently violates context cancellation would require managed worker ownership, and strict cross-validator scheduling fairness would require pubsub/per-peer scheduler changes. The production requester honors context; validation work is bounded by the existing 512 admission plus libp2p validation queue/throttle. Those broader redesigns are not included here.

Validation:

  • go test ./cl/phase1/forkchoice ./cl/phase1/network/services ./cl/phase1/stages -count=1
  • focused go test -race across the new concurrency/recovery cases
  • make lint, repeated clean
  • make erigon integration
  • three independent Standards/spec, adversarial, and lifecycle review passes converged with no remaining in-scope findings.

@yperbasis yperbasis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

CI is red

@domiwei

domiwei commented Aug 17, 2026

Copy link
Copy Markdown
Member Author

Pushed a49b7c84c4 to this PR branch.

What changed:

  • Merged the latest main into the PR branch.
  • Fixed the merge-tree CI failure by migrating the removed db/kv/memdb test helper to db/kv/mdbx/mdbxtest.
  • Preserved the PR's execution-payload cancellation/error semantics while resolving the merge conflicts.
  • Made selected-head envelope requests retryable after error/empty completion, with per-root attempt tokens so stale A→B→A completions cannot release a newer claim.
  • Made bounded ancestor recovery advance past unavailable holes and revisit them on later full passes, so one unavailable envelope cannot permanently block deeper recovery.

TDD / validation:

  • Reproduced the original CI error before the test-helper migration, then confirmed the focused test turned green.
  • Added regression coverage for selected-head retry/release behavior and recovery cursor progress.
  • go test ./cl/phase1/forkchoice ./cl/phase1/network/services ./cl/phase1/stages -count=1
  • Focused stage race tests passed.
  • make erigon integration passed.
  • make lint reports no PR-related issues; the only local macOS diagnostic is the current-main Linux-only db/seg.residencyOnce field, which is used by the Linux implementation and is unrelated to this PR.
  • No test skips were added.

Two independent adversarial/lifecycle reviews converged on the changed recovery surface. GitHub checks are restarting from the new head.

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

Labels

Caplin Caplin: Consensus Layer, Beacon API Glamsterdam https://eips.ethereum.org/EIPS/eip-7773

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants