feat: classify range sync batch processing fault#9667
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces fault localization for sync batch processing errors to avoid tearing down the entire sync chain on any error. It adds a classification mechanism (classifyProcessingFault) to determine if a processing failure is due to the current batch, a previous batch, or is ambiguous, allowing the node to re-download only the faulty batch(es). It also adds a retainForReprocessing state transition for batches, increases the maximum batch processing attempts, and gates peer penalization during teardown to finalized sync only. I have no feedback to provide as there are no review comments.
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.
Performance Report✔️ no performance regression detected Full benchmark results
|
| toRootHex(config.getForkTypes(blockN.message.slot).SignedBeaconBlock.hashTreeRoot(blockN)) | ||
| ); | ||
| } | ||
| export function hashBlocks(blocks: IBlockInput[], payloadEnvelopes: Map<Slot, PayloadEnvelopeInput> | null): RootHex { |
There was a problem hiding this comment.
hashing blocks + envlopes are cheap enough thanks to cachePermanentRootStruct option
the downside is we don't include signatures, haven't seen an issue of same block with different signatures for range sync on any networks yet
There was a problem hiding this comment.
The cheapness argument holds — blockRootHex is already cached, so this is strictly cheaper than the old SignedBeaconBlock.hashTreeRoot path.
On signatures, I'd frame the risk differently than "same block with different signatures". BLS signing is deterministic, so an honest peer serving the same block message always produces the same signature — you're right that this doesn't arise naturally, and no network would have shown it.
The case that does bite is adversarial: a peer serves the correct block message with a garbage signature. Since blockRootHex is the root of block.message, that attempt now gets the same id as the good one. It fails processing → lands in failedProcessingAttempts → the good attempt later wins with the same hash → attempt.hash !== attemptOk.hash is false at chain.ts:776 → the peer is never scored. The old signature-inclusive hash would have caught it, since a garbage signature changed the id.
That's cheap for a peer to do and now free of consequence at this layer, and it matters slightly more with MAX_BATCH_PROCESSING_ATTEMPTS going 0 → 3 (more free attempts before teardown). I don't think it's blocking — signature-corrupting peers may well get caught elsewhere first — but it might be worth re-wording the tradeoff note to say we no longer score peers that corrupt only the signature, rather than that same-block-different-signature hasn't been observed. Those are different claims, and the second one reads as "low risk" while the first is the one a peer can actually exploit.
|
Synced with this branch a couple of times on 
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #9667 +/- ##
============================================
- Coverage 52.49% 52.49% -0.01%
============================================
Files 848 848
Lines 60485 60455 -30
Branches 4466 4462 -4
============================================
- Hits 31754 31737 -17
+ Misses 28670 28657 -13
Partials 61 61 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a0251f476
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // PARENT_UNKNOWN/PARENT_PAYLOAD_UNKNOWN always fires on the batch's first not-yet-imported block. | ||
| const firstBlockSlot = err.signedBlock.message.slot; | ||
| // Anchored at the batch start => the missing parent is before this batch => previous-batch fault. | ||
| if (firstBlockSlot === batch.startSlot) return ProcessingFaultKind.PreviousBatch; |
There was a problem hiding this comment.
When this is the first queued batch in a sync chain and its first block fails with PARENT_UNKNOWN/PARENT_PAYLOAD_UNKNOWN, prevBatch is undefined but this still returns PreviousBatch. processBatch() then calls retainForReprocessing() and invalidates no earlier batch, so the same bad downloaded batch is retried without re-downloading or incrementing attempts; for a one-batch target, or until another download happens to retrigger processing, range sync can spin/stall instead of dropping the bad attempt. Treat the no-previous-batch case as this batch's fault/ambiguous rather than previous-batch fault.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — this is real, and I think it's blocking. Traced end to end:
classifyProcessingFault returns PreviousBatch whenever firstBlockSlot === batch.startSlot, without ever checking prevBatch. For the first batch of a sync chain prevBatch is undefined, and then:
batch.retainForReprocessing()→Processing→AwaitingProcessing, blocks retained, no attempt recorded — by design, its docstring says so explicitly.invalidatePreviousBatches()requirespendingBatch.startEpoch < batch.startEpoch, so it is a no-op — there is nothing earlier to invalidate.- The error branch ends at
triggerBatchDownloader()(chain.ts:755), nottriggerBatchProcessor(). getNextBatchToProcessreturns the firstAwaitingProcessingbatch — which is this same batch, holding the same bad blocks.
So each subsequent download success re-triggers the processor (chain.ts:646) → same batch → same PARENT_UNKNOWN → retainForReprocessing() again. failedProcessingAttempts never grows, so MAX_BATCH_PROCESSING_ATTEMPTS is never reached, the batch never re-downloads, and the chain never tears down.
One refinement to the report: it settles into a hard stall rather than a spin. includeNextBatch counts AwaitingProcessing toward batchesInBuffer, so once BATCH_BUFFER_SIZE(=10)+1 batches are buffered, includeNextBatch returns null → downloads stop → nothing re-triggers the processor → the chain sits in Syncing indefinitely with no progress, no peer report, and no error.
Worth flagging that this is a regression against the exact case the old constant documented: with MAX_BATCH_PROCESSING_ATTEMPTS = 0, processingError() threw MAX_PROCESSING_ATTEMPTS on the first failure → shouldReportPeerOnBatchError → chain dropped and peers reported. That's the #8147 remedy, and the first-batch path no longer reaches it.
Agree with the suggested fix. For the first batch there is no previous batch by definition, so an unknown parent can only mean the peer's segment doesn't chain onto our anchor:
if (firstBlockSlot === batch.startSlot) {
return prevBatch === undefined ? ProcessingFaultKind.ThisBatch : ProcessingFaultKind.PreviousBatch;
}I'd pick ThisBatch over Ambiguous — Ambiguous would additionally call invalidatePreviousBatches(), which is a no-op here anyway, so ThisBatch states the intent. Worth a unit test that a first batch failing PARENT_UNKNOWN at startSlot re-downloads and eventually throws.
| // gloas payloads, see hashBlocks) delivered bad data => penalize its peers | ||
| const attemptOk = batch.validationSuccess(); | ||
| for (const attempt of batch.failedProcessingAttempts) { | ||
| for (const attempt of [...batch.failedProcessingAttempts, ...batch.executionErrorAttempts]) { |
There was a problem hiding this comment.
Including executionErrorAttempts here can downscore peers after transient local EL failures: Batch.processingError() records EXECUTION_ENGINE_ERROR separately via onExecutionEngineError(), and teardown intentionally maps MAX_EXECUTION_ENGINE_ERROR_ATTEMPTS to no peer report, but a later successful retry with a different hash will now report the earlier peer even though UNAVAILABLE/ELERROR was not evidence of bad data. This can penalize honest peers when the execution engine recovers and the batch is retried; only score attempts that failed block validation, or distinguish actual invalid payloads from local EL errors.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, though I'd rank it below the P1. The strongest evidence is that the PR is internally inconsistent on this point: shouldReportPeerOnBatchError maps MAX_EXECUTION_ENGINE_ERROR_ATTEMPTS → null ("If peer should not be downscored, returns null") directly alongside MAX_PROCESSING_ATTEMPTS → LowToleranceError. So the teardown path already asserts EL errors are not the peer's fault, while advanceChain now scores exactly those attempts one screen up.
onExecutionEngineError only fires on BlockErrorCode.EXECUTION_ENGINE_ERROR, which is a local fault — the peer's data was never shown invalid, we just couldn't evaluate it.
The attempt.hash !== attemptOk.hash guard narrows this but doesn't close it. onExecutionEngineError wipes state to {blocks: [], payloadEnvelopes: null} → AwaitingDownload, so the retry generally comes from a different peer via the balancer — a differing hash is the common case here, not the rare one. Net effect: the EL flaps, and an honest peer that happened to be in flight takes a LowToleranceError ("SyncChainInvalidBatchOther").
The new hashBlocks widens it further: it now folds in payload envelopes and filters on hasPayloadEnvelope(), so two attempts with byte-identical blocks but different envelope availability produce different ids. An EL-error attempt with identical blocks can therefore still mismatch the winner and get scored.
Simplest fix is to drop ...batch.executionErrorAttempts from the loop — that's the same conclusion shouldReportPeerOnBatchError already reached. If the intent was to catch peers serving payloads the EL rejects, that's an invalid-payload signal rather than EXECUTION_ENGINE_ERROR, and is better distinguished inside onExecutionEngineError than at scoring time.
|
@lodekeeper please review |
lodekeeper
left a comment
lodekeeper
left a comment
There was a problem hiding this comment.
Reviewed on request. The fault-localization idea is a clear improvement over tearing the whole chain down on any processing error, and splitting the boundary PARENT_PAYLOAD_UNKNOWN from the new mid-segment NON_LINEAR_PAYLOAD_ROOTS in assertLinearChainSegment is the right shape — classifyProcessingFault leans entirely on that distinction being reliable, so it's good that it's now explicit in the type.
Requesting changes on one confirmed liveness bug. Details in-thread; summarising here plus one cross-cutting note.
Blocking — a first-batch PARENT_UNKNOWN stalls the sync chain permanently (r3600093261). classifyProcessingFault returns PreviousBatch whenever the failing block sits at batch.startSlot, without checking that a previous batch exists. For the first batch of a chain there is none, so retainForReprocessing() keeps the bad blocks without recording an attempt while invalidatePreviousBatches() no-ops. The batch then re-processes the same data on every downstream download, never re-downloads, never increments failedProcessingAttempts, and so never reaches MAX_BATCH_PROCESSING_ATTEMPTS. It ends in a hard stall once BATCH_BUFFER_SIZE+1 batches are buffered, since includeNextBatch counts AwaitingProcessing. This regresses the behaviour MAX_BATCH_PROCESSING_ATTEMPTS = 0 used to guarantee for #8147.
Non-blocking — peer scoring on EL errors (r3600093263). Codex is right, and the tell is internal: shouldReportPeerOnBatchError already maps MAX_EXECUTION_ENGINE_ERROR_ATTEMPTS → null, so scoring executionErrorAttempts in advanceChain contradicts the PR's own stance a few lines down.
Non-blocking — the head-sync safety argument in classifyProcessingFault's doc comment doesn't hold. It ends with:
Head-sync peers may be on different forks; the actions are still safe there (retries can converge onto the target fork), but peer penalization on teardown is gated to finalized sync.
Teardown penalization is not gated by syncType. The catch in sync() calls shouldReportPeerOnBatchError(e.type.code) and reports every peer in the peerset with no fork or syncType check. The only syncType === RangeSyncType.Finalized gate in this file is in the downloadByRange error path (chain.ts:561), which is a different path — and advanceChain's scoring loop isn't gated either. So during head sync, peers legitimately on another fork can still drive a batch to MAX_PROCESSING_ATTEMPTS and get the whole peerset LowToleranceError'd. Either drop that clause or actually add the gate; flagging it because that clause is what the head-sync safety argument rests on, so it shouldn't be load-bearing while untrue.
Doc nit — MAX_BATCH_PROCESSING_ATTEMPTS's new comment calls it a "Shared budget (via failedProcessingAttempts)", but it bounds two independent arrays: failedProcessingAttempts (via onProcessingError, reached from both processingError and validationError) and executionErrorAttempts (via onExecutionEngineError). It's two budgets of 3, not one shared budget of 3.
Nice work on the feat3 soak — that plus the new batches.test.ts cases is reassuring for the happy path. The first-batch case above is one a soak wouldn't surface unless a peer actually misbehaves on batch 0, which is why I'd want a unit test pinning it.
3 participants
Motivation
MAX_BATCH_PROCESSING_ATTEMPTS = 0in fix: handle bad response of 0 block from peer #8150Description
MAX_BATCH_PROCESSING_ATTEMPTSto 3 againprocessChainSegment()BlockError code of batchn, we classify if it's batchnerror, or batchn - 1or ambiguous and handle accordingly, seeclassifyProcessingFault()inbatches.tsutilNON_LINEARerrorclassifyProcessingFault()is to handle 0-block batch in fix: sync through empty epochs in range sync #9417 (through i want to make sure devnet sync works well first)Batch.retainForReprocessing()when the current batch itself is good, we retain and invalidate previous batchesAI Assistance Disclosure
cc @ensi321