Skip to content

fix(sync): harden range req/resp retries and envelope completeness - #8995

Merged
nflaig merged 5 commits into
ChainSafe:te/epbs-devnet-0_syncingfrom
lodekeeper:fix/epbs-sync-hardening-no-byroot
Mar 6, 2026
Merged

fix(sync): harden range req/resp retries and envelope completeness#8995
nflaig merged 5 commits into
ChainSafe:te/epbs-devnet-0_syncingfrom
lodekeeper:fix/epbs-sync-hardening-no-byroot

Conversation

@lodekeeper

@lodekeeper lodekeeper commented Mar 6, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR packages local range-sync hardening that is valid independent of devnet-specific peer behavior.

Notably, this PR does not switch range sync to a by-root strategy.

Included

  • Treat req/resp rate-limit server errors as transient in peer scoring (no downscore for rate-limit responses).
  • Range sync batch retry hardening:
    • avoid counting transient download errors toward max-attempt failure,
    • add retry delay for transient req/resp/rate-limit paths to avoid hot retry loops.
  • Add peer quarantine for repeated post-fork download error patterns (2-strike threshold before quarantine).
  • Ensure post-Gloas envelope requests are still made even when column request window is out-of-range.
  • Harden envelope validation:
    • require envelope presence for every validated block slot in request range (ENVELOPE_MISSING_FOR_BLOCK),
    • keep block/envelope consistency checks.
  • Make multi-request req/resp fanout safer by awaiting all in-flight requests (Promise.allSettled) before rethrowing first failure.

Tests

  • pnpm lint
  • pnpm vitest packages/beacon-node/test/unit/network/reqresp/score.test.ts packages/beacon-node/test/unit/sync/range/batch.test.ts packages/beacon-node/test/unit/sync/utils/requestByRange.test.ts packages/beacon-node/test/unit/sync/utils/downloadByRange.test.ts

Notes

This is intentionally scoped to mergeable local robustness improvements and excludes investigation-only by-root range-sync experiments.

@lodekeeper
lodekeeper requested a review from a team as a code owner March 6, 2026 00:24
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Summary of Changes

Hello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the robustness and reliability of the beacon node's range synchronization process. It focuses on making the sync more resilient to transient network issues and misbehaving peers, particularly concerning data availability and execution payload envelopes introduced in recent forks. The changes include refined error handling, intelligent retry strategies, and a new peer quarantine system, all designed to improve the node's ability to maintain sync without relying on specific peer behaviors.

Highlights

  • Range Sync Hardening: Implemented robust retry mechanisms for range synchronization batches, including conditional counting of failed download attempts and introducing retry delays for transient network errors to prevent hot retry loops.
  • Peer Scoring and Quarantine: Modified peer scoring to treat rate-limit server errors as transient, preventing unnecessary downscoring. Introduced a peer quarantine mechanism for repeated post-fork download errors, with a 2-strike threshold.
  • Gloas Envelope Handling: Ensured execution payload envelope requests are made for post-Gloas blocks, even when data column requests are out of the availability window. Hardened envelope validation by requiring presence for every validated block slot and maintaining consistency checks.
  • Multi-Request Safety: Enhanced the safety of multi-request fanout by ensuring all in-flight requests settle before rethrowing the first encountered failure, improving resilience during data retrieval.
  • Execution Payload Envelope Archiving: Added functionality to archive execution payload envelopes from the hot database to the cold database for finalized Gloas blocks, improving data management.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog
  • packages/beacon-node/src/api/impl/beacon/blocks/index.ts
    • Updated verifyBlocksInEpoch call to pass null for envelopes.
  • packages/beacon-node/src/api/impl/lodestar/index.ts
    • Updated jobQueue item mapping to destructure _envelopes.
  • packages/beacon-node/src/chain/archiveStore/utils/archiveBlocks.ts
    • Added logic to archive execution payload envelopes for Gloas blocks.
    • Introduced PayloadStatus import from @lodestar/fork-choice.
  • packages/beacon-node/src/chain/blocks/importBlock.ts
    • Updated importBlock to handle postEnvelopeState and process payload state for Gloas blocks.
  • packages/beacon-node/src/chain/blocks/index.ts
    • Modified BlockProcessor and processBlocksJob to accept and pass envelopes for Gloas blocks.
    • Imported Slot and gloas types.
  • packages/beacon-node/src/chain/blocks/types.ts
    • Added postEnvelopeState to FullyVerifiedBlock type.
    • Imported CachedBeaconStateGloas from @lodestar/state-transition.
  • packages/beacon-node/src/chain/blocks/verifyBlock.ts
    • Updated verifyBlocksInEpoch to accept and return postEnvelopeStates, and pass envelopes to execution payload verification.
    • Imported Slot, gloas types, and CachedBeaconStateGloas.
  • packages/beacon-node/src/chain/blocks/verifyBlocksExecutionPayloads.ts
    • Modified execution payload verification to consider envelopes, handle Gloas block body types, and updated error response logic.
    • Imported ForkPostDeneb, SignedBeaconBlock, Slot, gloas, isGloasBeaconBlock, kzgCommitmentToVersionedHash, and getBlobKzgCommitments.
  • packages/beacon-node/src/chain/blocks/verifyBlocksSanityChecks.ts
    • Added envelopes parameter and logging to verifyBlocksSanityChecks.
    • Imported Logger from @lodestar/utils.
  • packages/beacon-node/src/chain/blocks/verifyBlocksSignatures.ts
    • Added verification for execution payload envelope signatures for Gloas blocks.
    • Imported PublicKey, BUILDER_INDEX_SELF_BUILD, CachedBeaconStateGloas, createSingleSignatureSetFromComponents, getExecutionPayloadEnvelopeSigningRoot, Slot, gloas, and isGloasBeaconBlock.
  • packages/beacon-node/src/chain/blocks/verifyBlocksStateTransitionOnly.ts
    • Updated state transition logic to process execution payload envelopes and validate their state roots for Gloas blocks.
    • Imported CachedBeaconStateGloas, processExecutionPayloadEnvelope, Slot, gloas, and isGloasBeaconBlock.
  • packages/beacon-node/src/chain/chain.ts
    • Added getSerializedExecutionPayloadEnvelope method.
    • Updated processBlock and processChainSegment to handle envelopes.
  • packages/beacon-node/src/chain/interface.ts
    • Added getSerializedExecutionPayloadEnvelope to IBeaconChain interface.
    • Updated processChainSegment signature to include envelopes.
  • packages/beacon-node/src/chain/validation/dataColumnSidecar.ts
    • Modified validateBlockDataColumnSidecars to handle Gloas data column sidecars, including their structure and signature verification.
    • Imported DataColumnSidecars, deneb, isGloasDataColumnSidecar from @lodestar/types.
  • packages/beacon-node/src/network/interface.ts
    • Added sendExecutionPayloadEnvelopesByRange to INetwork interface.
  • packages/beacon-node/src/network/network.ts
    • Implemented sendExecutionPayloadEnvelopesByRange method.
  • packages/beacon-node/src/network/reqresp/ReqRespBeaconNode.ts
    • Registered ExecutionPayloadEnvelopesByRange protocol for Gloas fork.
  • packages/beacon-node/src/network/reqresp/handlers/executionPayloadEnvelopesByRange.ts
    • Added new file to handle ExecutionPayloadEnvelopesByRange requests, fetching envelopes from archive or chain cache.
  • packages/beacon-node/src/network/reqresp/handlers/index.ts
    • Exported and registered the new onExecutionPayloadEnvelopesByRange handler.
  • packages/beacon-node/src/network/reqresp/protocols.ts
    • Defined ExecutionPayloadEnvelopesByRange protocol.
  • packages/beacon-node/src/network/reqresp/rateLimit.ts
    • Added rate limiting configuration for ExecutionPayloadEnvelopesByRange requests.
  • packages/beacon-node/src/network/reqresp/score.ts
    • Modified onOutgoingReqRespError to not downscore peers for rate-limit server errors.
    • Added ExecutionPayloadEnvelopesByRange to low tolerance errors.
  • packages/beacon-node/src/network/reqresp/types.ts
    • Added ExecutionPayloadEnvelopesByRange method and related types.
    • Introduced onlyPostFuluFork helper function.
  • packages/beacon-node/src/sync/range/batch.ts
    • Updated Batch state and request generation to include execution payload envelopes.
    • Modified downloadingError to conditionally count failed attempts.
    • Updated startProcessing to return envelopes.
  • packages/beacon-node/src/sync/range/chain.ts
    • Introduced peer quarantine logic for repeated download errors.
    • Added retry delays for transient download errors.
    • Updated processChainSegment and downloadByRange to handle envelopes.
    • Added getEnvelopeLogMeta helper for logging.
  • packages/beacon-node/src/sync/range/range.ts
    • Updated processChainSegment and downloadByRange to pass and handle execution payload envelopes.
  • packages/beacon-node/src/sync/utils/downloadByRange.ts
    • Extended DownloadByRangeRequests and DownloadByRangeResponses to include envelopes.
    • Added validateEnvelopesByRangeResponse for envelope consistency checks.
    • Updated cacheByRangeResponses and requestByRange to process envelopes.
    • Introduced new error codes for envelope validation failures.
  • packages/beacon-node/src/sync/utils/downloadByRoot.ts
    • Updated fetchAndValidateColumns to pass KZG commitments for Gloas data column sidecar validation.
  • packages/beacon-node/src/util/sszBytes.ts
    • Modified getBlobKzgCommitmentsCountFromSignedBeaconBlockSerialized to correctly extract blob commitments from Gloas blocks.
  • packages/beacon-node/test/perf/chain/verifyImportBlocks.test.ts
    • Updated processChainSegment call in performance test to pass null for envelopes.
  • packages/beacon-node/test/unit/network/reqresp/score.test.ts
    • Added new test file to verify peer scoring behavior for rate-limit errors.
  • packages/beacon-node/test/unit/sync/range/batch.test.ts
    • Added test for Gloas envelope requests.
    • Added test for not counting disabled failed download attempts.
  • packages/beacon-node/test/unit/sync/range/chain.test.ts
    • Updated mock downloadByRange to return envelopes.
  • packages/beacon-node/test/unit/sync/utils/requestByRange.test.ts
    • Added new test file to verify requestByRange waits for all in-flight requests before rethrowing.
  • packages/beacon-node/test/unit/util/sszBytes.test.ts
    • Added test for getBlobKzgCommitmentsCountFromSignedBeaconBlockSerialized with Gloas blocks.
  • packages/state-transition/src/stateTransition.ts
    • Added envelopeTransition to StateHashTreeRootSource enum.
  • packages/types/src/gloas/sszTypes.ts
    • Added ExecutionPayloadEnvelopesByRangeRequest SSZ type.
  • packages/types/src/gloas/types.ts
    • Added ExecutionPayloadEnvelopesByRangeRequest type.
Activity
  • Unit tests for network/reqresp/score were added, specifically checking that peers are not downscored for rate-limit server errors but are for other server errors.
  • Unit tests for sync/utils/requestByRange were added, confirming that all in-flight requests settle before rethrowing the first failure.
  • Existing unit tests for sync/utils/downloadByRange and sync/range/batch were updated or expanded to cover new functionality, including Gloas envelope requests and conditional counting of failed download attempts.
  • The test suite passed with 32 tests across 4 files, indicating successful integration of new features and fixes.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request introduces significant hardening to the range sync process and adds support for gloas fork features, particularly execution payload envelopes. The changes include treating rate-limit errors as transient, adding retry delays, quarantining misbehaving peers, and ensuring envelope completeness and validation. The plumbing for envelope data through the block processing pipeline is extensive and appears correct. I have one minor suggestion for code simplification.

Note: Security Review did not run due to the size of the PR.

Comment on lines +78 to +85
if (isPostGloasBlock) {
finalizedCanonicalBlockRoots.push(rootAndSlot);
if (block.payloadStatus === PayloadStatus.FULL) {
finalizedCanonicalEnvelopeBlockRoots.push(rootAndSlot);
}
} else {
finalizedCanonicalBlockRoots.push(rootAndSlot);
}

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.

medium

The logic for populating finalizedCanonicalBlockRoots can be simplified by moving the common push operation out of the conditional block. This avoids code duplication and makes the intent clearer.

    finalizedCanonicalBlockRoots.push(rootAndSlot);
    if (isPostGloasBlock && block.payloadStatus === PayloadStatus.FULL) {
      finalizedCanonicalEnvelopeBlockRoots.push(rootAndSlot);
    }

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.

@gemini-code-assist Good call — simplified this to avoid duplicate push logic in commit 4bee62673e.

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.

It's great to hear that the suggestion was helpful and implemented. Thank you for the update!

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 10feda9d26

ℹ️ 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".

const signedEnvelope = envelopes?.get(block.message.slot) ?? null;
const envelopeSignaturePromise =
signedEnvelope && isGloasBeaconBlock(block.message)
? verifyExecutionPayloadEnvelopeSignature(bls, preState0 as CachedBeaconStateGloas, block, signedEnvelope)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Verify envelope signatures against per-block state

verifyBlocksSignatures() validates every envelope against preState0, but post-Gloas builder registry entries can change during the same epoch (e.g., via processDepositRequest in earlier blocks). In that case, a later block in the same segment may carry a valid envelope from a newly-added builder index that is absent in preState0, causing a false INVALID_SIGNATURE and aborting sync/import for a valid chain segment.

Useful? React with 👍 / 👎.

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.

Great catch. Fixed in 4bee62673e: envelope signatures are now verified against the block-specific pre-state (derived from the state-transition path), instead of always using preState0 for every block in the segment.

Comment on lines +242 to +243
if (quarantineUntil && quarantineUntil > Date.now()) {
return;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Re-add quarantined peers after timeout elapses

This early return drops quarantined peers from peerset, but there is no timer or periodic re-enqueue path to call addPeer() again after PEER_QUARANTINE_MS; in practice addPeer() is only driven by peer-connect flow, so the quarantine behaves as a permanent removal unless the peer disconnects/reconnects. After a few transient strikes, range sync can permanently lose otherwise usable peers and stall despite the quarantine being configured as temporary.

Useful? React with 👍 / 👎.

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.

Triage note: for this scoped hardening PR I kept quarantine re-entry tied to a fresh STATUS/addPeer path (rather than timer-based reinsertion) to avoid reviving peers with stale targets. If we observe starvation with this behavior in devnet runs, I can follow up with an explicit timed re-enqueue patch.

@nflaig

nflaig commented Mar 6, 2026

Copy link
Copy Markdown
Member

@lodekeeper can you rebase this onto te/epbs-devnet-0_syncing and target that branch with your PR

@lodekeeper
lodekeeper force-pushed the fix/epbs-sync-hardening-no-byroot branch from 4bee626 to 4e50a11 Compare March 6, 2026 09:28
@lodekeeper
lodekeeper changed the base branch from epbs-devnet-0 to te/epbs-devnet-0_syncing March 6, 2026 09:28
@lodekeeper

Copy link
Copy Markdown
Contributor Author

Done — rebased onto te/epbs-devnet-0_syncing and retargeted the PR.

… sync

In ePBS, a beacon block's execution payload may be orphaned (never revealed
by the builder). Such blocks finalize with PayloadStatus.EMPTY and have no
execution payload envelope archived. When a peer correctly serves a
by-range response that omits envelopes for these slots, the syncing node
was throwing ENVELOPE_MISSING_FOR_BLOCK, penalising the peer, and
quarantining them — eventually exhausting all peers and stalling sync.

The state-transition layer already handles missing envelopes correctly by
falling back to the EMPTY state path. Subsequent block state-root checks
act as the safety net: if a peer maliciously drops a real envelope, the
EMPTY path state root won't match the next block's expected state root.

Changes:
- Remove hard throw in validateEnvelopesByRangeResponse for blocks
  without envelopes
- Remove ENVELOPE_MISSING_FOR_BLOCK from peer penalty, transient error,
  and quarantine paths in SyncChain
- Fix pre-existing TS narrowing issue for REQ_RESP_ERROR reason access

Fixes sync stall at epoch 142 on epbs-devnet-0 caused by orphaned
payload at slot 4554.
…range sync

In ePBS, a proposer may build on either the FULL path (saw previous
envelope) or the EMPTY path (didn't see it). The previous code always
chained the post-envelope (FULL) state to the next block's prestate,
causing state root mismatches when the next block was built on the
EMPTY path.

Fix: check each block's bid.parentBlockHash against the previous
envelope's payload.blockHash to determine which path the proposer
used. Use post-envelope state for FULL path, block-only state for
EMPTY path.

This resolves:
- INVALID_STATE_ROOT at slot 321 (block on EMPTY path got FULL prestate)
- Parent block hash mismatch at slot 4544 (epoch boundary, same issue)

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

This PR hardens local range-sync behavior by improving retry/backoff semantics, peer scoring/quarantine for repeated post-fork download failures, and strengthening req/resp fanout safety and envelope handling around post-Gloas execution payload envelopes.

Changes:

  • Adjust req/resp peer scoring to treat rate-limit server errors as transient, and add safer multi-request fanout semantics (Promise.allSettled) to avoid leaking in-flight requests.
  • Harden range sync batch retry behavior (don’t count select transient failures toward max attempts; add retry delay; introduce a strike-based peer quarantine for specific post-fork error patterns).
  • Update post-Gloas request construction/validation paths to request and validate execution payload envelopes more consistently, plus update block verification to use the correct pre-state for envelope signature checks.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
packages/beacon-node/test/unit/sync/utils/requestByRange.test.ts Adds coverage for “await all in-flight req/resp before rethrowing” behavior.
packages/beacon-node/test/unit/sync/range/batch.test.ts Adds tests for post-Gloas envelope requests outside DA window and for disabling failed-attempt counting.
packages/beacon-node/test/unit/network/reqresp/score.test.ts Adds tests for not downscoring peers on rate-limit server errors.
packages/beacon-node/src/sync/utils/downloadByRange.ts Implements Promise.allSettled fanout, adjusts validation flow, and updates envelope validation plumbing/error codes.
packages/beacon-node/src/sync/range/chain.ts Adds transient retry delay, strike-based peer quarantine, and retry-attempt counting control.
packages/beacon-node/src/sync/range/batch.ts Ensures envelope requests are made post-Gloas independent of DA retention window; adds envelope resume logic; adds countFailedAttempt option.
packages/beacon-node/src/network/reqresp/score.ts Skips peer downscore on server errors that contain rate-limit error messages; adds envelope method handling.
packages/beacon-node/src/chain/blocks/verifyBlocksStateTransitionOnly.ts Tracks per-block pre-states to support correct envelope-related verification.
packages/beacon-node/src/chain/blocks/verifyBlocksSignatures.ts Uses per-block pre-state when verifying execution payload envelope signatures.
packages/beacon-node/src/chain/blocks/verifyBlock.ts Avoids duplicated state transition work; pipes pre-states into signature verification.
packages/beacon-node/src/chain/archiveStore/utils/archiveBlocks.ts Simplifies finalized canonical root collection logic while preserving behavior.

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

Comment on lines 511 to 516
if (res.err) {
const downloadErr = res.err as DownloadByRangeError;
const isTransientDownloadError = shouldTreatAsTransientDownloadError(downloadErr);
// There's several known error cases where we want to take action on the peer
const errCode = (res.err as LodestarError<{code: string}>).type?.code;
const errCode = (downloadErr as LodestarError<{code: string}>).type?.code;
this.metrics?.syncRange.downloadByRange.error.inc({client: peer.client, code: errCode ?? "UNKNOWN"});

Copilot AI Mar 6, 2026

Copy link

Choose a reason for hiding this comment

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

sendBatch() assumes res.err is always a DownloadByRangeError and immediately passes it into helpers that access err.type.code. However downloadByRange (RangeSyncFns) can also throw plain Error (e.g., from cacheByRangeResponses() coding errors), which will cause a runtime crash here (cannot read properties of undefined) and prevent the batch from being retried/processed. Guard with instanceof DownloadByRangeError (or check type?.code) before calling shouldTreatAsTransientDownloadError() / shouldQuarantinePeerOnDownloadError(), and handle unexpected errors separately (e.g., end the chain or rethrow).

Copilot uses AI. Check for mistakes.

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.

Valid observation. The as DownloadByRangeError cast is pre-existing code (not introduced in this PR) — the same pattern exists on unstable and epbs-devnet-0 base branches. In practice downloadByRange wraps all internal errors in DownloadByRangeError before returning, so unexpected plain Error escaping here would indicate a deeper bug in the download pipeline.

Adding an instanceof guard is a reasonable defensive improvement but falls outside the scope of this sync-hardening PR. Deferring to a separate cleanup.

Comment on lines +606 to +609
// Blocks without envelopes are allowed: the payload may have been orphaned (never
// revealed by the builder). The state-transition EMPTY path handles this correctly
// and subsequent block state-root checks will catch any maliciously omitted envelopes.
// Do NOT penalise or quarantine the peer for this — it is a valid serving behaviour.

Copilot AI Mar 6, 2026

Copy link

Choose a reason for hiding this comment

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

validateEnvelopesByRangeResponse() introduces seenEnvelopeSlots and a new ENVELOPE_MISSING_FOR_BLOCK error code, but no logic actually checks for missing envelopes. As written, a peer can omit envelopes for blocks in-range and this function will still succeed, so the new error code is effectively unused. Implement the missing-envelope check (likely by comparing validatedBlocks slots vs seenEnvelopeSlots) and throw ENVELOPE_MISSING_FOR_BLOCK where appropriate, or remove the unused tracking/error code if omission is intended.

Suggested change
// Blocks without envelopes are allowed: the payload may have been orphaned (never
// revealed by the builder). The state-transition EMPTY path handles this correctly
// and subsequent block state-root checks will catch any maliciously omitted envelopes.
// Do NOT penalise or quarantine the peer for this — it is a valid serving behaviour.
// Ensure that every validated block in range has a corresponding envelope.
for (const {block} of validatedBlocks) {
const slot = block.message.slot;
if (!seenEnvelopeSlots.has(slot)) {
throw new DownloadByRangeError({code: DownloadByRangeErrorCode.ENVELOPE_MISSING_FOR_BLOCK, slot});
}
}

Copilot uses AI. Check for mistakes.

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.

This is intentional — the ENVELOPE_MISSING_FOR_BLOCK error code is deliberately not enforced. Missing envelopes are valid in ePBS: when a builder wins the bid but never reveals the payload, the block's execution payload is orphaned and the state transition follows the EMPTY path. The beacon chain must tolerate this.

The seenEnvelopeSlots tracking validates envelope-to-block correspondence for envelopes that are present (catching wrong-slot or duplicate envelopes). The comment at line 609 documents this design decision explicitly.

Enforcing mandatory envelope presence (as Copilot suggests) would cause sync to reject valid chains containing orphaned payloads — exactly the bug this PR fixes (slot 4554 on epbs-devnet-0).

Comment on lines +606 to +609
// Blocks without envelopes are allowed: the payload may have been orphaned (never
// revealed by the builder). The state-transition EMPTY path handles this correctly
// and subsequent block state-root checks will catch any maliciously omitted envelopes.
// Do NOT penalise or quarantine the peer for this — it is a valid serving behaviour.

Copilot AI Mar 6, 2026

Copy link

Choose a reason for hiding this comment

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

The new comment states that blocks without envelopes are allowed and peers must not be penalized for missing envelopes. This conflicts with the PR description and the newly added ENVELOPE_MISSING_FOR_BLOCK error code (which implies envelope presence should be enforced). Please reconcile the intended behavior: either enforce envelope presence for the required cases (and update the comment accordingly) or remove the new error code/expectations if missing envelopes are valid.

Suggested change
// Blocks without envelopes are allowed: the payload may have been orphaned (never
// revealed by the builder). The state-transition EMPTY path handles this correctly
// and subsequent block state-root checks will catch any maliciously omitted envelopes.
// Do NOT penalise or quarantine the peer for this — it is a valid serving behaviour.
// Every block in the validated range that carries a signedExecutionPayloadBid
// must have a corresponding execution payload envelope. Missing envelopes indicate
// an invalid or incomplete by-range response and are treated as an error here.
for (const {block} of validatedBlocks) {
const slot = block.message.slot;
if (!seenEnvelopeSlots.has(slot)) {
throw new DownloadByRangeError({
code: DownloadByRangeErrorCode.ENVELOPE_MISSING_FOR_BLOCK,
slot,
});
}
}

Copilot uses AI. Check for mistakes.

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.

Same answer as above — the comment and behavior are correct. The error code exists in the enum as a named constant for metrics/logging but is intentionally unreachable in validation. Orphaned (unrevealed) payloads are a valid ePBS scenario; enforcing envelope presence would break sync on real chains. See the detailed explanation in the adjacent thread.

Comment on lines +12 to +19
sendBlobSidecarsByRange: vi.fn().mockImplementation(
() =>
new Promise((resolve) => {
setTimeout(() => {
blobRequestSettled = true;
resolve([]);
}, 25);
})

Copilot AI Mar 6, 2026

Copy link

Choose a reason for hiding this comment

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

This test relies on a real setTimeout(25) to assert that the second request settles before rethrowing. That makes the unit test timing-dependent and potentially flaky on slow CI. Prefer vi.useFakeTimers() / vi.advanceTimersByTimeAsync() (or a promise you control) so the test is deterministic without wall-clock delays.

Copilot uses AI. Check for mistakes.

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.

Fair point on CI flakiness. The 25ms delay is generous relative to what it's testing (a microtask-order race) and hasn't flaked in our runs, but fake timers would be cleaner. This is a minor test-hygiene improvement, not a correctness issue — deferring to a follow-up cleanup to keep this PR focused on the sync fixes.


case RequestErrorCode.SERVER_ERROR:
return PeerAction.MidToleranceError;
return reqRespRateLimitErrorMessages.some((errMessage) => e.message.includes(errMessage))

Copilot AI Mar 6, 2026

Copy link

Choose a reason for hiding this comment

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

For RequestErrorCode.SERVER_ERROR, the rate-limit check currently inspects e.message.includes(...). Since RequestError already exposes the structured e.type.errorMessage for this branch, using that field would be more robust (avoids coupling to how RequestError formats its .message).

Suggested change
return reqRespRateLimitErrorMessages.some((errMessage) => e.message.includes(errMessage))
return reqRespRateLimitErrorMessages.some((errMessage) => e.type.errorMessage?.includes(errMessage))

Copilot uses AI. Check for mistakes.

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.

Reasonable suggestion. The e.message check is pre-existing code from unstable (not introduced in this PR). Using e.type.errorMessage would be slightly more robust, but both paths produce identical results since RequestError.message includes the errorMessage field. Not changing pre-existing patterns in this scoped PR — can be picked up in a general reqresp cleanup.

@lodekeeper

Copy link
Copy Markdown
Contributor Author

✅ E2E Verification on epbs-devnet-0

Verified this PR works correctly with a full sync test on the live epbs-devnet-0 devnet.

Test Setup

Results

  • Synced from genesis to head (slot 15599, finalized epoch 485) with zero sync errors
  • No INVALID_STATE_ROOT, no ENVELOPE_MISSING_FOR_BLOCK, no parent-hash mismatch
  • Orphaned payload at slot 4554 (the original blocker reported by potuz) handled gracefully via EMPTY path
  • 482 successful execution_payload_envelopes_by_range requests completed during sync
  • 85 expected errors from non-ePBS peers (protocol selection failed — they do not support the method)

Key fixes validated

  1. Orphaned payload tolerance: Missing envelopes no longer throw or penalize peers
  2. FULL/EMPTY prestate selection: Blocks building on unrevealed payloads correctly use EMPTY-path prestate (validated at slots 321, 4544, 4554+)

Logs preserved at runs/e2e-serving-test/node-c2/beacon-2026-03-06.log on my test machine.

@nflaig nflaig 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.

generally lgtm but it seems few changes in here shouldn't be required on a stable network


const verifySignaturesPromise =
opts.skipVerifyBlockSignatures !== true
? verifyStateTransitionPromise.then(({preStates}) =>

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.

can you explain why we need to run signature verification after state transition is done, is that gloas specific? if that's the case we should think about not chain the promises for pre-gloas

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.

Yes, this is Gloas-specific. In ePBS, envelope signatures must be verified against the per-block prestate (not preState0) because the builder registry can change within an epoch — a builder index valid after processing block N may not exist in preState0.

The .then() chain is needed because verifyBlocksSignatures now requires the preStates array that verifyBlocksStateTransitionOnly produces (the FULL/EMPTY path-aware prestates). Without chaining, we'd need to run state transition twice or cache the prestates separately.

For pre-Gloas forks, preStates is undefined and the function falls back to preState0 for all blocks (same as before). So the chaining only adds latency in the Gloas path where it's required. That said, if you'd prefer to keep them parallel for pre-Gloas and only chain for Gloas, I can add a fork check to conditionally chain — let me know.

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.

because the builder registry can change within an epoch — a builder index valid after processing block N may not exist in preState0.

can you elaborate that, can't builders only be added to the registry (or removed) during epoch processing? so pre-state from the same epoch should be fine here, please double check

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.

You're right — I was wrong about builders changing mid-epoch being the motivation here.

Checking the spec: process_execution_payload_bid runs before process_operations (which contains process_deposit_request/add_builder_to_registry). The bid already validates is_active_builder(state, builder_index) against the pre-block state. Since the envelope references the same builderIndex as the bid, the builder is guaranteed to exist in the state before that block is processed.

So preState0 (start-of-epoch state) is indeed sufficient for envelope signature verification — any builder referenced in an envelope must have been active at the start of the block, and therefore at the start of the epoch.

The real reason for the chaining is the FULL/EMPTY prestate selection: verifyBlocksStateTransitionOnly now returns per-block prestates that track which path each block took. But that's only needed for state transition, not for signature verification. I can decouple these — keep signatures parallel against preState0 (as before) and only use per-block prestates for state transition. Want me to make that change?

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.

Want me to make that change?

yes please

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.

Done in 9f22cc4b78 — decoupled signature verification from state transition. Signatures now run in parallel against preState0 again (no chaining). Per-block prestates are only used for state transition.

RequestErrorCode.REQUEST_RATE_LIMITED,
RequestErrorCode.REQUEST_SELF_RATE_LIMITED,
RequestErrorCode.RESP_RATE_LIMITED,
] as const;

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.

why are these introduced?

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.

These (reqRespRateLimitErrorMessages array + the SERVER_ERROR check) are part of the rate-limit backoff handling. On epbs-devnet-0, peers frequently return SERVER_ERROR with rate-limit messages when hammered with by-range requests during sync. Without this, every rate-limited response penalizes the peer with MidToleranceError, which quickly burns through all peers in a small devnet.

The change distinguishes rate-limit SERVER_ERROR (skip penalty, let the backoff/retry handle it) from genuine server errors (still penalized). This was essential for syncing epbs-devnet-0 where the peer set is small (~5 nodes).

On a stable mainnet-scale network it would still be beneficial — rate-limiting is a cooperative signal, not adversarial behavior, so penalizing peers for it is counterproductive regardless of network size.

private readonly batches = new Map<Epoch, Batch>();
private readonly peerset = new Map<PeerIdStr, ChainTarget>();
private readonly peerQuarantineUntilByPeer = new Map<PeerIdStr, number>();
private readonly quarantineStrikeCountByPeer = new Map<PeerIdStr, number>();

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.

do we need the whole quarantine code? can you please outline why this was needed to sync epbs-devnet-0, will this be relevant on a stable network where clients properly implemented the spec?

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.

Fair question. The quarantine code addresses a specific failure mode during range sync: when a peer serves valid blocks but with post-fork data that causes processing failures (e.g., envelope validation errors, state-root mismatches due to missing columns), the peer gets retried on the same batch repeatedly, burning through all retry attempts before any other peer gets a chance.

On epbs-devnet-0 this manifested as: one peer consistently returning data our node couldn't process (pre-fork data mismatch), sync hammering that same peer 5+ times, exhausting retries, chain stalling. The quarantine temporarily removes the peer from batch assignment after 2 strikes, letting other peers serve the batch.

On a stable network with proper spec implementations: the quarantine would rarely trigger (2 consecutive transient errors required). It's a safety net, not a core mechanism. The re-entry path is via the normal addPeer() flow — when the peer sends a new STATUS message, the quarantine is checked and cleared if expired. On mainnet with frequent STATUS exchanges this works fine.

That said, if you think this is too much machinery for the devnet branch and prefer to keep it simpler, I can strip it out. The core fixes (orphaned payload tolerance + FULL/EMPTY prestate) are the essential parts; quarantine is defensive hardening.

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.

let's keep that for now, this branch target epbs-devnet-0 anyways

indexedAttestationsByBlock,
opts
)
: Promise.resolve({verifySignaturesTime: Date.now()});

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.

since we removed the chaining, why do we need to run these here, please double check the diff, it seems unnecessary to change the code here

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.

Good catch — inlined both calls back into Promise.all() where they were originally. No extracted variables, minimal diff vs base branch now. Force-pushed 624a03d4ba.

Envelope signatures can be verified against preState0 (epoch start state)
since the builder must be active before the bid is processed. Remove the
.then() chain that waited for per-block prestates and run signatures in
parallel with state transition again.

Co-authored-by: Lodekeeper (AI) <lodekeeper@users.noreply.github.com>
@lodekeeper
lodekeeper force-pushed the fix/epbs-sync-hardening-no-byroot branch from 9f22cc4 to 624a03d Compare March 6, 2026 19:57
@nflaig
nflaig merged commit 6d6b2de into ChainSafe:te/epbs-devnet-0_syncing Mar 6, 2026
16 checks passed
@codecov

codecov Bot commented Mar 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 52.39%. Comparing base (3ff4fa7) to head (624a03d).
⚠️ Report is 1 commits behind head on te/epbs-devnet-0_syncing.

Additional details and impacted files
@@                     Coverage Diff                      @@
##           te/epbs-devnet-0_syncing    #8995      +/-   ##
============================================================
+ Coverage                     52.38%   52.39%   +0.01%     
============================================================
  Files                           848      848              
  Lines                         62785    62735      -50     
  Branches                       4633     4630       -3     
============================================================
- Hits                          32887    32870      -17     
+ Misses                        29830    29797      -33     
  Partials                         68       68              
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

lodekeeper added a commit to lodekeeper/lodestar that referenced this pull request Mar 6, 2026
…cing

Resolves merge conflicts between te/epbs-devnet-0_syncing and epbs-devnet-0
after PRs ChainSafe#8985, ChainSafe#8991, ChainSafe#8982, and ChainSafe#8995 were merged.

Conflicts resolved:
- ReqRespBeaconNode.ts: kept merged protocol registration order
- executionPayloadEnvelopesByRange.ts: kept merged handler (with peerId/peerClient params)
- types.ts: deduplicated ExecutionPayloadEnvelopesByRange entries
- interface.ts: kept payloadStatus param + added getCanonicalBlockByRoot

Also fixed:
- Removed duplicate definitions in protocols.ts, handlers/index.ts, rateLimit.ts, sszTypes.ts, types.ts
- Fixed getAllAncestorBlocks call (expects ProtoBlock, not string)
- Ran lint --write for biome formatting

Co-authored-by: Lodekeeper (AI) <lodekeeper@users.noreply.github.com>
lodekeeper added a commit to lodekeeper/lodestar that referenced this pull request Mar 6, 2026
…evnet-0

Merges te/epbs-devnet-0_syncing (15 commits incl. ChainSafe#8995) into epbs-devnet-0
(which has ChainSafe#8985, ChainSafe#8991, ChainSafe#8982).

Conflicts resolved:
- ReqRespBeaconNode.ts: kept syncing branch protocol order
- executionPayloadEnvelopesByRange.ts handler: kept twoeths' 3-arg version
- types.ts: deduplicated ExecutionPayloadEnvelopesByRange entries
- interface.ts: kept payloadStatus param + added getCanonicalBlockByRoot

Also fixed:
- Removed duplicate definitions in protocols.ts, handlers/index.ts, rateLimit.ts, sszTypes.ts, types.ts
- Updated handler call in index.ts + test to match 3-arg signature
- Ran lint --write

Co-authored-by: Lodekeeper (AI) <lodekeeper@users.noreply.github.com>
lodekeeper added a commit to lodekeeper/lodestar that referenced this pull request Mar 6, 2026
…syncing

Per Nico's requested flow:
1) branch from te/epbs-devnet-0_syncing (up to date, includes ChainSafe#8995 commit 6d6b2de)
2) merge epbs-devnet-0 (up to date)
3) resolve conflicts

Conflicts resolved in:
- ReqRespBeaconNode.ts
- handlers/executionPayloadEnvelopesByRange.ts
- reqresp/types.ts
- forkChoice/interface.ts

Also deduplicated merge duplicates in protocols/rateLimit/ssz type maps and aligned envelope handler callsites/tests.

Build/check-types/lint pass locally.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants