Break on unknown receipts; ignore unknown blocks

[flcl42]

Fixes #11231

Changes

• Break on unknown receipts
• Ignore unknown blocks

Types of changes

What types of changes does your code introduce?

• Bugfix (a non-breaking change that fixes an issue)
• New feature (a non-breaking change that adds functionality)
• Breaking change (a change that causes existing functionality not to work as expected)
• Optimization
• Refactoring
• Documentation update
• Build-related changes
• Other: Description

Testing

Requires testing

• Yes
• No

If yes, did you write tests?

• Yes
• No

[claude]

Claude finished @flcl42's task in 9m 45s —— View job

---

PR Review — fix-unknown-block-eth-responses

• Gather context and read changed files
• Analyze correctness, edge cases, security, performance
• Post inline comments
• Final summary

---

Summary

This PR fixes two distinct server-side behaviours when handling sync requests for unknown blocks:

Area	Before	After
GetBlockBodies	Block block with no null guard → null body silently included (or NPE risk if type was non-nullable)	continue — unknown blocks skipped, shorter response
GetReceipts (eth63/69)	GetReceipts called for every hash regardless of knowledge	FindHeader guard → break at first unknown block
GetReceipts (eth70)	Same	Same FindHeader guard → break

The receipt fix is clean, correct, and important — serving receipts for blocks you don't have is a protocol error. The break semantics are right because receipts must be a contiguous sequence from the front of the requested range.

The body fix is functionally safe but carries a subtle semantic shift (see Medium finding below).

---

Findings

Medium

FulfillBlockBodiesRequest: continue changes from null-placeholder to sparse-list (line 336)

The old code effectively added a null BlockBody entry per unknown hash (serialised as Rlp.OfEmptyList), preserving positional alignment. BodiesSyncFeed.InsertBodies uses responses[i] positional matching, so a sparse response with a gap in the middle maps bodies to wrong block infos — triggering hasBreachedProtocol = true and false peer-breach reports.

In normal operation (fully-synced, non-pruned serving node) gaps never occur, so this is low-impact in practice. The behaviour is also consistent with Geth. A comment explaining the intentional continue (vs break) would prevent future confusion, and a test covering [known, unknown, known] would lock in the semantics.

Low

1. Extra FindHeader per hash in receipt serving (line 379) — doubles header lookups on the serving path; acceptable if headers are cache-resident.

2. Test coverage — unknown hash in middle of GetBlockBodies (V62 test) — [known, unknown, known] case not tested.

3. Test coverage — first hash unknown in GetReceipts (V69 test) — all tests start with a known block; immediate-break on first-unknown not covered.

---

What looks good

• Receipt break is the right call — serving receipts for unknown blocks was incorrect.
• V63/V69/V70 test-setup fixes (FindHeader default stub) are correct and minimal.
• Tests clearly document the intended server-side truncation behaviour.
• No security or consensus-correctness issues found.

[claude]

Claude finished @flcl42's task in 4m 43s —— View job

---

PR Review — fix-unknown-block-eth-responses

• Gather context and read rule files
• Read changed files
• Analyze correctness, edge cases, security, performance
• Post inline comments
• Final summary

---

Summary

This revision fully addresses all findings from the prior review. Here's the updated status:

Finding	Previous	Now
Medium — continue creates sparse body response, breaks positional matching in BodiesSyncFeed	Open	✅ Resolved — InsertBodies refactored with responseIndex; ShouldHandleSparseBodyResponseWithoutReportingBreach test validates it
Low — Double FindHeader lookup on receipt-serving hot path	Open	Still present, acceptable trade-off
Low — V62 missing [known, unknown, known] test	Open	✅ Added: Should_omit_unknown_hash_in_middle_from_get_block_bodies
Low — V69 missing first-hash-unknown test	Open	✅ Added: Should_return_empty_receipts_response_when_first_hash_is_unknown
(New) V70 unknown-block coverage missing	—	✅ Added: Should_stop_receipts_response_at_first_unknown_block_hash + Should_return_empty_receipts_response_when_first_hash_is_unknown

New Observations

Low — responseIndex not advancing on TryPrepareBlock failure (BodiesSyncFeed.cs:274)
Intentional for sparse responses (body for B retried against C), safe for genuinely corrupt bodies (breach caught at loop end). Comment explains the intent; clarified the implicit "corrupt body still triggers breach" path for future readers.

Low — Always.Valid validator in sparse-response test (BodiesSyncFeedTests.cs:172)
Test correctly validates index tracking and no-breach behavior, but Always.Valid means body-to-header cryptographic matching isn't exercised here. Acceptable for this layer.

What looks good

• Receipt break on unknown block is the correct protocol behavior — verified in eth63/69 and eth70, with tests for both [known, unknown] and [unknown, ...] cases.
• BodiesSyncFeed.InsertBodies refactoring is logically sound: responseIndex decouples response position from request position, handling sparse responses without false breach reports or wasted re-requests.
• The cancellationToken is correctly checked in eth63/69's outer loop (pre-existing gap in eth70 outer loop is out of scope).
• No security or consensus-correctness issues found.

---

No Critical, High, or unresolved Medium findings remain.

[claude]

Low — responseIndex does not advance on validation failure; trace-through for clarity

When TryPrepareBlock returns false, the same body at responses[responseIndex] is retried against the next blockInfo. This is intentional for sparse responses: if a server skips block B and returns [bodyA, bodyC] for [A, B, C], bodyC correctly fails for B and then succeeds for C.

One edge case worth documenting: a genuinely corrupt body also fails for every subsequent blockInfo, leaving responseIndex < responses.Length at the end — which triggers the breach report correctly. But during that loop, all remaining blockInfos are marked pending and re-requested from another peer, so correctness is preserved.

The comment already captures the intent; just ensuring the implied "breach is still caught at the end" path is understood by future maintainers.

[claude]

Note — Always.Valid makes this test pass for any body-to-header pairing

CreateBlockValidator() wraps Always.Valid, so TryPrepareBlock succeeds regardless of tx-root or uncle-hash matching. The sparse response [firstBlock.Body, thirdBlock.Body] for [A, B, C] is handled correctly here because responseIndex advances only on success, but the test cannot verify that thirdBlock.Body is validated against thirdBlock's header (it would pass even if swapped).

This is fine for integration-style coverage of the index tracking and breach-report path. The unit-level body-to-header matching is exercised by TryPrepareBlock's own tests.