Skip to content

Don't punish the block peer when the envelope response is empty and is a different peeer#16779

Merged
terencechain merged 2 commits into
developfrom
skip-empty-envelopes-in-range-sync
May 13, 2026
Merged

Don't punish the block peer when the envelope response is empty and is a different peeer#16779
terencechain merged 2 commits into
developfrom
skip-empty-envelopes-in-range-sync

Conversation

@terencechain
Copy link
Copy Markdown
Collaborator

In validatePayloadBlockConsistency, when the envelope response is empty AND checkAllBlocksBuildOnEmpty fails (i.e., the block bids show a payload was revealed somewhere in the range), the old code:

  1. Set r.err = errors.Wrap(prysmsync.ErrInvalidFetchedData, …) → batch dropped by the FSM.
  2. blocks_queue.go:347-349 then unconditionally fired q.downscorePeer(response.blocksFrom, "invalidBlocks") against the block peer — even when a different peer was the one that returned the empty envelope
    response.
  3. The inline if r.blocksFrom == r.payloadsFrom { f.downscorePeer(...) } separately downscored the same peer.

Changes in this pr

  • Drop the r.err = … assignment in this branch. Without it, the queue-level invalidBlocks downscore (which keys on ErrInvalidFetchedData) no longer fires, and the batch passes through to processing.
  • Keep the inline same-peer downscore — folded into the same condition. If the same peer served both the blocks and an empty envelope response that contradicts its own bids, that peer is still penalized.
scenario before after
Blocks from peer A, empty envelopes from peer B batch dropped; A downscored invalidBlocks batch passes through; nobody downscored
Same peer served both blocks and empty envelopes batch dropped; peer downscored (inline + queue) batch passes through; peer downscored (inline only)

@terencechain terencechain changed the title Don't punish the block peer when the envelope response is empty Don't punish the block peer when the envelope response is empty and is a different peeer May 13, 2026
Copy link
Copy Markdown
Contributor

@potuz potuz left a comment

Choose a reason for hiding this comment

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

Thanks for catching my bug. We need to check if there's another consistency check though that will carry the same error, that is if the chains are not consistent but with some payloads, we need to make sure that we're not wrapping the error as here and penalizing the blocks provider unfairly.

@terencechain terencechain added this pull request to the merge queue May 13, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks May 13, 2026
@terencechain terencechain enabled auto-merge May 13, 2026 15:31
@terencechain terencechain added this pull request to the merge queue May 13, 2026
Merged via the queue into develop with commit 7218bd1 May 13, 2026
23 checks passed
@terencechain terencechain deleted the skip-empty-envelopes-in-range-sync branch May 13, 2026 17:18
pull Bot pushed a commit to Hawthorne001/prysm that referenced this pull request May 13, 2026
…ffchainLabs#16785)

Follow-up to OffchainLabs#16779. Same anti-pattern lived in the non-empty envelope
branch of `validatePayloadBlockConsistency`:

- When an envelope doesn't match a block, the old code set `r.err =
errors.Wrap(prysmsync.ErrInvalidFetchedData, ...)`.
- The inline downscore is correctly gated on `r.blocksFrom ==
r.payloadsFrom`, but `ErrInvalidFetchedData` propagates up to
`blocks_queue.go:347-349` which unconditionally fires
`q.downscorePeer(response.blocksFrom, "invalidBlocks")`, penalizing the
block peer for a bad envelope served by a different peer
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.

2 participants