cl/network, cl/rpc: fix blob history backfill boundary - #23138
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a blob-history backfill gap at the FrozenBlobs() boundary in Caplin’s CL networking, while tightening downloader retry/error handling and improving correctness of persisted execution payload envelope validation and Beacon API header responses.
Changes:
- Treat
FrozenBlobs()as an exclusive boundary so backfill covers the first unfrozen slot and doesn’t cross into frozen snapshot ranges. - Make blob backfill run with any available peer (not a fixed 16-peer gate) and return request/verification failures for bounded retries; ban peers on invalid protocol responses.
- Add validation for persisted execution payload envelopes against the fork graph + EL, and include
finalizedin Beacon header responses with tests.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| cl/phase1/network/blob_downloader.go | Adjusts blob backfill boundary/peer gating and hardens response validation/error propagation. |
| cl/phase1/network/blob_downloader_test.go | Adds focused tests for boundary handling, retries, persistence, and malformed responses. |
| cl/phase1/forkchoice/on_execution_payload.go | Validates persisted envelopes with the EL when needed to avoid silently trusting disk state. |
| cl/phase1/forkchoice/on_execution_payload_test.go | Adds tests covering persisted-envelope validation and EL status outcomes. |
| cl/beacon/handler/headers.go | Adds finalized computation to header responses. |
| cl/beacon/handler/headers_test.go | Tests finalized behavior across canonical/non-canonical and empty results. |
| cl/beacon/handler/blobs_test.go | Extends blob handler tests to ensure first-unfrozen slot reads from storage (not snapshots). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
af686cb to
472e47e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
cl/phase1/network/blob_downloader.go:400
recoverDenebBlobsnow returns errors for malformed/incomplete Deneb blob responses, but it never applies any peer penalty even thoughPeerAndSidecarsincludes the peer ID andBeaconRpcP2PsupportsBanPeer. This can cause retries to keep selecting the same bad peer (especially withRequestBlobsFrantically, which stops at the first non-empty response), and it also diverges from the PR description’s “protocol-appropriate ban behavior” for invalid responses.
if blobs == nil {
return errors.New("request blobs: empty result")
}
if len(blobs.Responses) != req.Len() {
return fmt.Errorf("incomplete blob response: received %d of %d", len(blobs.Responses), req.Len())
}
AskAlexSharov
left a comment
AskAlexSharov
left a comment
There was a problem hiding this comment.
Review: blob history backfill boundary
The boundary fix itself looks right. FrozenBlobs() is exclusive (its doc comment and TestCaplinWatermarks pin that), the reader uses slot < FrozenBlobs(), and the downloader now walks down to max(targetSlot, FrozenBlobs()) inclusive, so the orphaned slot is covered. The Content-Type fix in EndpointError.WriteTo is a real improvement, and it also corrects the SSZ error path that used to leave application/octet-stream on a JSON body.
The error-handling rewrite in recoverDenebBlobs and downloadOnce is where I see real problems. 15 findings, ordered by severity:
Blocking — cl/phase1/network/blob_downloader.go
- Any failed or timed-out Deneb request now aborts the whole pass, and the pass can never complete.
- The 64-slot archive overlap is far smaller than the non-frozen window, so gaps become permanent and blob antiquation stalls.
- Short responses are legal per spec, but are now rejected and their data discarded.
Should fix
- A Deneb failure also skips Fulu column recovery in the same batch.
- Fulu failures stay silent while Deneb failures are fatal.
- The peer gate drop to one peer makes the node hammer its only peer.
- 503 hides sidecars that are on disk (
cl/beacon/handler/blobs.go). BlobBackfillPendingignores the progress the downloader already tracks.- 503 is not a documented response for this endpoint, and the body does not match the spec schema.
?indices=with an empty value now returns 400 instead of 200.
Nits — triage as you like
- The pending gate is implemented twice, with different logic.
- New snapshot read and full body decode on the miss path.
- The new test starts an uncancellable goroutine per case.
- Duplicated and overflow-prone
denebSlotcomputation. - The whole
*stages.Cfgis threaded into the API handler.
Items 1, 2, 3 and 5 interact: each one on its own can leave backfillCompleted in the wrong state, which is what the new 503 gate reads.
|
Addressed the review in Items 1–3: recovery no longer turns a legal short response into permanent zero progress. Deneb responses are matched by Items 4–6: Deneb and Fulu recovery are both attempted and their errors are accounted for. Fulu completion is checked from the actual stored sidecars and can force a repair when stale metadata would make PeerDAS skip it. Blob ByRoot requests now have bounded backoff and a shared protocol-level maximum of two concurrent requests; forced recovery also coalesces same-root work and drops canceled queued work. Items 7–8: availability is evaluated for the requested indices/hashes and precise canonical coverage. Available requested data remains a 200; any expected requested member still pending is a retryable 503. Completion is trimmed on reorgs before the canonical DB commit becomes visible and published only after commit succeeds. Item 9: confirmed that these endpoint schemas do not enumerate 503. We are intentionally retaining it as an Erigon extension, documented in the PR body. The body uses the shared Item 10: retained 400 for an empty Items 11–15: shared the availability/storage policy, removed the full Additional adversarial rounds covered disjoint Deneb/Fulu retention ranges, same-slot and deeper reorgs, commit visibility, partial/corrupt persisted sets, snapshot/status TOCTOU in both directions, and cancellation/resource bounds. Validation: two complete |
ce84ed8 to
c1d94c9
Compare
domiwei
changed the title
domiwei
changed the title
|
Final scope split: #23138 now contains only the #22429 boundary/peer-startup fix plus the bounded retry and protocol concurrency safety required when running with a small peer set. The retryable 503 availability contract and its completion/reorg/recovery/storage state machinery moved to draft #23213. Closed draft #23198 is superseded. |
4c7bd9a to
6f68ce2
Compare
| continue | ||
| } | ||
| ctx, cancel := context.WithTimeout(b.ctx, b.columnBackfillTimeout) | ||
| err = forced.ForceScheduleRecover(ctx, block.GetSlot(), blockRoot, uint64(block.Block.Body.BlobKzgCommitments.Len())) |
There was a problem hiding this comment.
Nil-pointer panic on Gloas blocks. recoverFuluColumns runs for every block with Version() >= FuluVersion, which includes GloasVersion. NewBeaconBody only allocates body.BlobKzgCommitments when version < clparams.GloasVersion — for Gloas it stays nil and the commitments live in SignedExecutionPayloadBid.Message.BlobKzgCommitments. ListSSZ.Len() is len(l.list) on a pointer receiver, so block.Block.Body.BlobKzgCommitments.Len() dereferences nil and panics in the backfill goroutine.
Every other site in this file already uses the version-aware accessor (actualBlobSetComplete, collectIncompleteBlocks).
| err = forced.ForceScheduleRecover(ctx, block.GetSlot(), blockRoot, uint64(block.Block.Body.BlobKzgCommitments.Len())) | |
| err = forced.ForceScheduleRecover(ctx, block.GetSlot(), blockRoot, uint64(block.Block.Body.GetBlobKzgCommitments().Len())) |
| } | ||
| currentSlot -= step | ||
| } | ||
| if passErr != nil { |
There was a problem hiding this comment.
One permanently-unavailable blob now makes the whole backfill never complete — and the new 503 permanent.
Backfill used to be best-effort: recoverDenebBlobs/recoverFuluColumns logged and moved on. Now every failure is joined into passErr, and this early return skips addCompletedRanges(pendingRanges) entirely — even for ranges that were fully processed earlier in the same pass.
Concrete scenario: a Deneb block whose sidecars no peer still serves. recoverDenebBlobs loops until RequestBlobsFrantically hits its 15 s ErrTimeout and returns request blobs: .... completedRanges therefore never grows, BlobBackfillPending stays true for every slot in the range, and GetEthV1BeaconBlobSidecars returns 503 forever for all of them instead of the spec's 200 + empty list. Each 12 s pass also re-runs the same 15 s stall and logs at Error level.
Suggest: mark the ranges that did complete (or track per-slot failures) and keep unrecoverable blocks from blocking completion, rather than all-or-nothing.
| begin := time.Now() | ||
| log.Debug("[blobsRecover] recovering blobs", "slot", toRecover.slot, "blockRoot", toRecover.blockRoot) | ||
| ctx := context.Background() | ||
| ctx := recoveryCtx |
There was a problem hiding this comment.
Running the recovery under the caller's context can permanently drop custody columns.
This was context.Background() precisely because the tail of recover is not atomic: it writes the blob sidecars, then RemoveColumnSidecars(ctx, ...) deletes non-custody columns, then re-creates the custody columns with WriteColumnSidecars(ctx, ...). With recoveryCtx now being the requester's context — ForceScheduleRecover is called with a 30 s columnBackfillTimeout from recoverFuluColumns — cancellation between the remove and the write leaves the columns deleted and not rewritten, with no retry.
128-column RecoverMatrix + per-blob ComputeBlobKZGProof + per-column VerifyDataColumnSidecar* can easily exceed 30 s on mainnet, so this is reachable in normal operation. Keep the recovery on a detached context and use the request context only to decide whether to start it.
|
|
||
| // SendBeaconBlocksByRangeReq retrieves blocks range from beacon chain. | ||
| func (b *BeaconRpcP2P) SendBlobsSidecarByIdentifierReq(ctx context.Context, req *solid.ListSSZ[*cltypes.BlobIdentifier]) ([]*cltypes.BlobSidecar, string, error) { | ||
| b.blobSidecarRequestsOnce.Do(func() { |
There was a problem hiding this comment.
The shared 2-permit limiter lets history backfill starve live forward sync.
This semaphore is process-wide and unprioritised, capacity 2. RequestBlobsFrantically keeps up to maxConcurrentBlobRequests (also 2) in flight, and recoverDenebBlobs calls it back-to-back in a for remaining.Len() > 0 loop with no pause — so the backfill holds both permits essentially continuously.
downloadAndProcessEip4844DA (forward_sync.go) goes through the same call. Its two goroutines park on this select and never reach the sentinel; after 15 s RequestBlobsFrantically returns ErrTimeout having issued zero network requests, forward sync logs "Blob request timeout" and returns without progress. That stalls catch-up while a background repair pass runs.
At minimum the backfill and live-sync paths need separate budgets (or the backfill should yield its permit between rounds).
Also: the literal 2 duplicates maxConcurrentBlobRequests in cl/phase1/network/blobs.go, and the lazy sync.Once init would be simpler as a field set in NewBeaconRpcP2P.
| if commitments.Len() == 0 { | ||
| return true, nil | ||
| } | ||
| sidecars, complete, err := b.blobStorage.ReadBlobSidecars(b.ctx, block.GetSlot(), blockRoot) |
There was a problem hiding this comment.
Scan cost per slot goes from one DB lookup to a full blob read + decode + hash.
collectIncompleteBlocks used to decide completeness with KzgCommitmentsCount — a single BlockRootToKzgCommitments lookup. actualBlobSetComplete now calls ReadBlobSidecars, which opens and snappy-decodes every sidecar file for the block (each blob is 128 KiB), and then HashSSZ()es each sidecar header.
That runs for every Deneb+ slot on every 12 s pass until the range is marked complete — and per the passErr early-return above, one unrecoverable block means it never is, so an archive node re-reads and re-hashes its entire blob history every 12 s.
A cheap pre-filter (count check first, full verification only when the count matches and something else is suspect) would keep the added integrity check without the I/O.
| } | ||
|
|
||
| d.recoveringMutex.Lock() | ||
| if active := d.isRecovering[request.blockRoot]; active != nil { |
There was a problem hiding this comment.
Coalescing drops live-sync recovery requests when the forced backfill request wins the race.
TryScheduleRecover (gossip / live sync) enqueues with result == nil. If a forced backfill request for the same root is already active, this branch registers no waiter and returns — which is correct as long as the active recovery runs to completion. But the forced request owns a 30 s context (see recover(requestCtx, request) below), so it can abort partway; nothing then retries on behalf of the live-sync caller, and TryScheduleRecover already returned nil so its caller believes recovery is under way.
Related: finishBlobRecovery(root, err) fans the owner's error out to every waiter, including ones whose own context is healthy — a 30 s backfill timeout surfaces as a failure to unrelated callers.
| if err != nil || complete || blockRoot != canonicalRoot || a.caplinSnapshots == nil || slot >= a.caplinSnapshots.FrozenBlobs() { | ||
| return sidecars, complete, err | ||
| } | ||
| sidecars, err = a.caplinSnapshots.ReadBlobSidecars(slot) |
There was a problem hiding this comment.
The snapshot retry throws away the sidecars already read from storage. When FrozenBlobs() advances past slot between the entry check on line 172 and this point, but the snapshot happens to hold no sidecars for that slot, this returns nil, false — so a request that could have served the partial set from blobStoage now returns nothing.
Prefer keeping the storage result unless the snapshot read actually produced something:
| sidecars, err = a.caplinSnapshots.ReadBlobSidecars(slot) | |
| if frozen, err := a.caplinSnapshots.ReadBlobSidecars(slot); err != nil || len(frozen) != 0 { | |
| return frozen, len(frozen) != 0, err | |
| } | |
| return sidecars, complete, nil |
| // Append the current slot and root to the list of reconnection roots | ||
| reconnectionRoots = append(reconnectionRoots, canonicalEntry{currentSlot, currentRoot}) | ||
| } | ||
| commonAncestorSlot := uint64(0) |
There was a problem hiding this comment.
commonAncestorSlot silently becomes 0 on the newFoundSlot == nil break, wiping all backfill progress.
The walk above breaks when a parent root has no slot index (checkpoint-synced node, pruned index, first head after restart). currentRoot != currentCanonical at that point, so commonAncestorSlot stays 0, and commitCanonicalHead then calls InvalidateCompletionAbove(0) → trimCompletedRanges(0) drops every completed range. The next downloadOnce re-scans (and, per the actualBlobSetComplete cost, re-reads and re-hashes) the whole blob history.
Worth distinguishing "no common ancestor found" from "common ancestor is slot 0" so the caller can decide, rather than encoding both as 0.
| "sys", common.ByteCount(m.Sys)) | ||
|
|
||
| return tx.Commit() | ||
| return commitCanonicalHead(tx, cfg.blobDownloader, headSlot, headRoot, commonAncestorSlot) |
There was a problem hiding this comment.
Moving the head publication here means it no longer runs when postForkchoiceOperations fails. Previously SetHeadSlot(headSlot) was the first thing after the headState == nil guard inside postForkchoiceOperations, so it ran even when the later steps (ProduceAndCacheAttestationData, OnHeadStateWithBlockRoot, DumpBeaconStateOnDisk, saveFinalizedStateOnDiskIfNeeded) errored.
Now any of those errors returns before line 436, so the blob downloader's head freezes for as long as the failure persists — and BlobBackfillPending reports every slot above the stale head as pending, which is exactly the input to the new 503.
| return sidecars, len(sidecars) != 0, err | ||
| } | ||
|
|
||
| func (a *ApiHandler) readBlobSidecarsWithBackfillStatus(ctx context.Context, slot uint64, blockRoot, canonicalRoot common.Hash) ([]*cltypes.BlobSidecar, bool, bool, error) { |
There was a problem hiding this comment.
The re-read doubles the disk cost of the common case. When the block is genuinely incomplete and backfill is not pending (old block, blobs gone for good, or backfill disabled), this reads and snappy-decodes every sidecar file twice — up to ~1.5 MB of blob data per request on a 9-blob block — to close a TOCTOU window that only matters when FrozenBlobs() or the backfill status flips mid-request.
Cheaper equivalent: sample blobBackfillPending before the first read, and only re-read when the status changed between the two samples.
| return nil, beaconhttp.NewEndpointError(http.StatusBadRequest, err) | ||
| } | ||
| if _, duplicate := included[i]; duplicate { | ||
| return nil, beaconhttp.NewEndpointError(http.StatusBadRequest, fmt.Errorf("duplicate blob index %d", i)) |
There was a problem hiding this comment.
Rejecting duplicate indices (and duplicate versioned_hashes on line 347) with 400 is a new, non-standard restriction. The Beacon API schema for these query params does not forbid repeats, and StringListFromQueryParams flattens ?indices=0,0 and ?indices=0&indices=0 into the same list — so a client that de-duplicates on its own side, or that builds the list from a set of versioned hashes with a repeated blob, now gets a hard error where it previously got the blob.
The PR body documents the 503 as an intentional Erigon extension but says nothing about these 400s. Deduplicating silently (the map already does it) keeps the endpoint permissive; only clearly malformed input needs 400.
|
|
||
| if cfg.blobDownloader != nil { | ||
| cfg.blobDownloader.SetHeadSlot(cfg.startingSlot + 1) | ||
| cfg.blobDownloader.SetHead(cfg.startingSlot+1, common.Hash{}, 0) |
There was a problem hiding this comment.
Two problems with this call after the semantics change.
+1is left over fromSetHeadSlot, whose doc said "the slot we start downloading from (currentSlot + 1)".SetHeadis now documented as "the inclusive upper bound of the range to download", anddoForkchoiceRoutinepasses the head slot itself. PassingstartingSlot+1here makes the two producers disagree by one slot and shiftsepochRetentionFloor(head, ...).safeThrough = 0meanstrimCompletedRanges(0)discards every completed range. Combined withheadRoot = common.Hash{}, anydownloadOncepass in flight also fails theb.headRoot != headRootcheck at the end and throws its results away.
| headSlot := b.headSlot.Load() | ||
| headRoot := b.headRoot | ||
| desiredRanges := b.backfillRanges(headSlot) | ||
| if len(desiredRanges) == 0 { |
There was a problem hiding this comment.
This early return leaves backfillCompleted at its previous value (false). On a chain where Deneb is unscheduled or the head is still below the Deneb start, backfillRanges always returns nil, so run()'s warningTimer branch logs "Blob backfilling is not finished, some blobs might be unavailable" every 4 minutes forever. The old loop reached backfillCompleted.Store(true) in that case.
| if len(desiredRanges) == 0 { | |
| if len(desiredRanges) == 0 { | |
| b.mu.RUnlock() | |
| b.backfillCompleted.Store(true) | |
| return nil | |
| } |
| return batch, nil | ||
| } | ||
|
|
||
| func (b *denebRecoveryBatch) validate(req *solid.ListSSZ[*cltypes.BlobIdentifier], sidecars []*cltypes.BlobSidecar) (int, error) { |
There was a problem hiding this comment.
This re-implements blob_storage.VerifyAgainstIdentifiersAndInsertIntoTheBlobStore — identifier matching, VerifyCommitmentInclusionProof, VerifyBlobKZGProof, and the grouped WriteBlobSidecars — which the downloader called until this PR and which cl/phase1/stages/forward_sync.go:108 still calls.
Two divergent verification paths for the same wire data is a maintenance hazard: the batched VerifyBlobKZGProofBatch in the shared helper is also faster than the per-sidecar VerifyBlobKZGProof here. The improvements this version adds (order independence, partial responses, per-index retry) are exactly what the shared helper needs; better to extend it and keep one implementation.
| wantErr := errors.New("first unfrozen slot visited") | ||
| reader := &boundaryBlockReader{err: wantErr} | ||
| downloader := newBoundaryDownloader(t, firstUnfrozenSlot, firstUnfrozenSlot, firstUnfrozenSlot, 1, reader) | ||
|
|
There was a problem hiding this comment.
This new file duplicates helpers the same PR adds to blob_downloader_test.go in the same package: newBoundaryDownloader ≈ newBlobDownloaderForBoundaryTest, boundaryBlockReader ≈ recordingBlobBlockReader, boundaryPeerClient ≈ peerCountClient, boundarySnapshot ≈ frozenBlobSnapshot, boundarySyncedChecker ≈ syncedChecker. Five near-identical pairs, and the two constructors already disagree (requestBlobs is unset in newBoundaryDownloader, so any test that reaches recoverDenebBlobs through it nil-panics).
Appending these four tests to the existing blob_downloader_test.go removes the whole duplicate set.
Also on file hygiene: cl/phase1/network/blobs_test.go carries a truncated LGPL header (stops after "any later version") and cl/das/peer_das_recovery_test.go has none, while their sibling files in both packages have the full one.
6f68ce2 to
f3d0e04
Compare
domiwei
changed the title
Summary
FrozenBlobs()as the exclusive end of frozen snapshot coverage so history backfill processes the first unfrozen slotRoot cause
The snapshot reader owns slots strictly below
FrozenBlobs(), but the downloader stopped whencurrentSlot <= FrozenBlobs(). The exact boundary slot therefore belonged to neither snapshots nor history backfill. A separate 16-peer threshold could also prevent repair on otherwise connected nodes.Lowering the peer gate without bounding the existing 100ms retry loop could overload a node's only peer. Requests are therefore limited to two concurrent streams with exponential backoff, matching the Req/Resp concurrency bound.
Scope
This PR is limited to the #22429 data-coverage bug and the request safety required by the lower peer gate. Temporary API availability responses, completion tracking, reorg handling, and recovery/storage hardening are intentionally moved to follow-up #23213.
Validation
go test ./cl/phase1/network ./cl/rpc -count=1make lint(complete pass: 0 issues)Follow-up: #23213
Fixes #22429