Improve eth/70 checks

[flcl42]

Changes

• Pass receipts in batches up to soft limit
• Allow last receipt to fill the packet up to hard limit
• Split it if it does not fit

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 8m 12s —— View job

---

PR Review: Improve eth/70 checks

• Gather context and read changed files
• Load relevant agent rules
• Analyse implementation correctness and edge cases
• Post inline comments and summary

---

Summary

The PR substantially revises the eth/70 receipt handler to: (1) serve receipts in batches up to a soft limit, (2) allow the response to grow past the soft limit to deliver a complete single block (capped at a hard limit), and (3) split oversized single blocks across multiple requests. The implementation is well-structured, test coverage is comprehensive (~24 new test cases), and the validation logic in ValidateBlockReceipts is thorough.

Two medium-severity bugs were identified; everything else is low/informational.

---

Findings

Medium

#	Location	Issue
M1	Eth70ProtocolHandler.cs:127	checked((int)getReceiptsMessage.FirstBlockReceiptIndex) throws OverflowException (not SubprotocolException) for adversarial FirstBlockReceiptIndex > int.MaxValue. The background-task handler will still disconnect the peer, but with an opaque error message. A pre-cast bounds check is cleaner.
M2	Eth70ProtocolHandler.cs:166	lastBlockIncomplete is set to true when cancellationToken.IsCancellationRequested races with the last receipt being counted. This causes the requester to issue a follow-up request with FirstBlockReceiptIndex = receipts.Length, which the responder rejects → spurious disconnect. The `

Low

#	Location	Issue
L1	Eth70ProtocolHandler.cs:277	Redundant full re-validation (ValidateBlockReceipts) of the completed partial receipt array. Each chunk was already validated on receipt. Adds O(N) work and an extra ToArray() allocation for every block whose receipts span multiple pages.
L2	Eth70ProtocolHandler.cs:171	FulfillReceiptsRequest is synchronous but declared as Task<ReceiptsResponse> and uses Task.FromResult. Minor allocation overhead; can be simplified to a plain synchronous method.
L3	Eth70ProtocolHandler.cs:349	BuildRequest copies all remaining hashes into a new ArrayPoolList on every page — O(N²) total hash copies for heavily-paginated syncs. Low impact in practice but easy to improve.

---

Correctness walk-through highlights

• Batch soft-limit path (lines 126–131): correct — blocks are accumulated until responseSizeEstimate + remainingBlockSizeEstimate would exceed the soft limit.
• Hard-limit split path (lines 139–167): correct — when the response is empty and the first block exceeds the soft limit, receipts are included one-by-one up to min(blockSize, hardLimit).
• Requester paging logic (SendGetReceiptsWithPaging): state transitions (firstBlockReceiptIndex, partialReceipts, partialReceiptsGas) look correct across all page-boundary cases tested.
• ValidateBlockReceipts gas checks: the Math.Max(previousGasUsed, firstReceiptIndex * GasCostOf.Transaction) lower-bound trick is sound and handles continuation segments correctly.
• Early-exit on fewer blocks than requested (line 313–315): intentionally preserves eth/63 semantics — peers may return fewer blocks; callers handle it upstream.

[flcl42]

@claude review again

[claude]

Claude finished @flcl42's task in 7m 21s —— View job

---

PR Review: Improve eth/70 checks

• Gather context
• Load relevant agent rules
• Read changed files
• Analyse implementation correctness and edge cases
• Post inline comments and summary

---

Summary

All Medium findings from the previous review have been addressed in the "Review" commit (d8a657d). The implementation is now correct and well-tested.

---

Previous findings — status

#	Issue	Status
M1	checked cast throws OverflowException for FirstBlockReceiptIndex > int.MaxValue	Fixed — pre-cast bounds check at line 110; also covered by the new Should_disconnect_with_subprotocol_error_when_first_block_receipt_index_exceeds_int_max_value test
M2	Spurious lastBlockIncomplete = true when cancellation races with last receipt	Fixed — now simply startIndex + taken < receipts.Length (no || cancellationToken.IsCancellationRequested)
L1	Redundant full re-validation of completed partial receipt array	Fixed — re-validation call removed
L2	FulfillReceiptsRequest returns Task<ReceiptsResponse> despite being synchronous	Fixed — now returns ReceiptsResponse directly; call site uses ValueTask.FromResult
L3	BuildRequest copies all remaining hashes into a new list each page — O(N²) total	Still present (BuildRequest unchanged)

---

Fresh analysis

Correctness walk-through (re-verified):

• requestedStartIndex == receipts.Length for non-empty blocks (line 110 uses >, not >=): this case falls through line 110 (allowed), skips the empty-block branch (line 117), and is caught by line 138. Behaviour is correct — throws SubprotocolException("Invalid firstBlockReceiptIndex …") — but using >= at line 110 would make line 138 dead code and clarify the intent. Minor readability nit.
• Paging state machine (blockIndex, firstBlockReceiptIndex, partialReceipts, partialReceiptsGas): correct across all page-boundary cases. firstBlockReceiptIndex is always 0 when non-first blocks in a response are processed; partialReceiptsGas is always 0 when a new (non-continuation) block starts.
• ValidateBlockReceipts gas bounds: partialReceiptsGas = partialReceipts[^1].GasUsedTotal is read before AddRange, so it correctly represents the gas at end of the previous page. The Math.Max(previousGasUsed, firstReceiptIndex * GasCostOf.Transaction) lower-bound remains sound.
• Empty response short-circuit (lines 245–253): correctly detected before any per-receipt processing; the PartialContinuationComplete test scenario exercises this path with the expected "Peer returned no progress" message.
• Memory ownership: TxReceipt[] arrays are regular managed arrays; disposing the IOwnedReadOnlyList after the using block does not affect arrays already stored in aggregated. Safe.
• Cancellation at outer loop top (line 97–100): returns empty response with LastBlockIncomplete = false, causing the requester to break cleanly. Correct.
• CalculateLogsGas: checked arithmetic could theoretically throw OverflowException for pathological receipts, but gas-limit caps bound data lengths well within long range in practice.

No new bugs found.

---

Remaining low-priority item

L3 (BuildRequest, line ~392): copies all remaining blockHashes into a new ArrayPoolList on every page — O(N) copy per round-trip, O(N²) total for heavily-paginated syncs. In practice the batch size is capped by RunSizeAndLatencyRequestSizer, so the absolute copy count stays small; not a blocker.

---

Overall: the PR is ready to merge. All blocking issues are resolved; the only open item (L3) is a minor efficiency improvement that can be addressed in a follow-up.

[flcl42]

@claude review again

[claude]

Claude finished @flcl42's task in 7m 3s —— View job

---

PR Review: Improve eth/70 checks

• Gather context
• Load relevant agent rules
• Read changed files
• Analyse implementation correctness and edge cases
• Post inline comments and summary

---

Summary

This revision addresses all remaining items from the previous review, including the previously-open L3 (O(N²) hash copies in BuildRequest). The implementation is now correct, well-tested, and clean.

---

Previous findings — final status

#	Issue	Status
M1	OverflowException for FirstBlockReceiptIndex > int.MaxValue	Fixed (bounds check + new test)
M2	Spurious lastBlockIncomplete=true on cancellation race	Fixed
L1	Redundant full re-validation of completed partial receipts	Fixed
L2	Synchronous FulfillReceiptsRequest returning Task<ReceiptsResponse>	Fixed — now ReceiptsResponse directly; call site uses ValueTask.FromResult
L3	O(N²) hash copies in BuildRequest	Fixed — BuildRequest now uses zero-copy Slice() via the new SlicedOwnedReadOnlyList

---

L3 fix walk-through

The O(N²) fix is clean:

• SlicedOwnedReadOnlyList<T> added in IOwnedReadOnlyList.cs — a view over the original list, with a no-op Dispose() so the original owner remains responsible for cleanup.
• SlicedReadOnlyList._list / ._start promoted to protected so the derived class can implement AsSpan() correctly.
• BuildRequest now calls blockHashes.Slice(startIndex, ...) — zero allocations per page.
• GetReceipts makes one O(N) copy upfront (clampedHashes.ToPooledList()), then all subsequent pages use slices.
• HashesMessageSerializer converted from AsSpan() to index-based for loop — both approaches work with SlicedOwnedReadOnlyList (the slice correctly implements AsSpan()), but the for loop is explicit and unambiguous.
• New test Should_not_copy_remaining_hashes_when_building_paged_receipts_requests uses a CountingReadOnlyList to assert exactly N indexer reads for N hashes (one per hash, from the initial ToPooledList()).

---

New findings

Informational

CalculateLogsGas throws OverflowException instead of SubprotocolException on pathological data (Eth70ProtocolHandler.cs:509)

checked arithmetic throws OverflowException if a receipt somehow carries enough log topics/data bytes to overflow long. Gas limits make this impossible on any live network (block gas cap ~30M gas, log topic cost 375 gas → max ~80k topics per block). On a non-standard devnet or under fuzzing it could produce a generic disconnect rather than the structured "protocol violation" path. Noted as an optional hardening; not a blocker.

---

Correctness re-verification

• Soft-limit batch path (lines 148–156): correct — blocks added until adding the next would exceed soft limit.
• Hard-limit pass-through (lines 163–166): correct — single block that exceeds soft but fits hard limit is delivered whole without lastBlockIncomplete.
• Hard-limit split path (lines 169–199): correct — receipts added one-by-one until the next would exceed hard limit; lastBlockIncomplete = startIndex + taken < receipts.Length.
• taken == 0 guard (line 192): throws SubprotocolException instead of sending an empty block. Correct and necessary.
• ValidateBlockReceipts with previousGasUsed: the Math.Max(previousGasUsed, firstReceiptIndex * GasCostOf.Transaction) lower bound is sound for both fresh and continuation segments.
• partialReceiptsGas state machine: correctly set/reset across all page-boundary transitions; the pre-AddRange read at line 296 gives the gas at end of the previous page, which is the correct previousGasUsed for the new segment.
• Hard-limit response validation (line 266): new check rejects responses above the hard limit, closing an adversarial amplification gap.
• Memory ownership: SlicedOwnedReadOnlyList no-op Dispose() is correct by design — the backing ownedHashes (ArrayPoolList) is still alive when slices are in use and is disposed only when SendGetReceiptsWithPaging returns.
• Empty-block soft-limit check (lines 122–140): correctly defers to the responseReceiptsContentSize == 0 guard so at least one block is always included.

No new bugs found. The PR is ready to merge.

[flcl42]

@claude review once more

[claude]

Claude finished @flcl42's task in 5m 37s —— View job

---

PR Review: Improve eth/70 checks

• Gather context
• Load relevant agent rules
• Read changed files
• Analyse implementation correctness and edge cases
• Post inline comments and summary

---

Summary

This is the fourth pass over the PR. All Medium and Low findings from the previous three rounds have been addressed, including the previously-open informational about CalculateLogsGas overflowing (now handled in AddLogGas with a structured SubprotocolException, tested by Should_throw_subprotocol_exception_when_logs_gas_overflows).

---

Previous findings — final status

#	Issue	Status
M1	OverflowException for FirstBlockReceiptIndex > int.MaxValue	Fixed
M2	Spurious lastBlockIncomplete=true on cancellation race	Fixed
L1	Redundant full re-validation of completed partial receipts	Fixed
L2	Synchronous FulfillReceiptsRequest returning Task<ReceiptsResponse>	Fixed
L3	O(N²) hash copies in BuildRequest	Fixed — zero-copy Slice() via SlicedOwnedReadOnlyList
Info	CalculateLogsGas throws OverflowException instead of SubprotocolException	Fixed — now AddLogGas with checked/catch

---

Fresh analysis

Correctness re-verification (all clean):

• Two-step bound check (lines 115 + 143): requestedStartIndex > receipts.Length (line 115) is not dead code duplication — for empty blocks requestedStartIndex == 0 == receipts.Length is valid and must not throw; for non-empty blocks the == case is caught at line 143. Both paths are correct. The > vs >= asymmetry was noted in the previous review as a readability nit; the author has chosen to keep it as is, which is acceptable.

• isCompleteSegment calculation (!response.LastBlockIncomplete || !isLast): verified correct across all four boolean combinations — a non-last continuation block is always a complete segment; the last block is complete only when LastBlockIncomplete = false. ✓

• partialReceiptsGas read-before-AddRange (line 296): partialReceipts[^1].GasUsedTotal is read before AddRange, correctly capturing the gas at end of the previous page. ✓

• taken == 0 guard (line 192): throws SubprotocolException rather than sending a zero-receipt block, preventing infinite loops on pathological receipts. ✓

• Minimum partial response size check (line 276): size < SoftOutgoingMessageSizeLimit / 4 guards against peers sending arbitrarily tiny partial pages to waste requester bandwidth; the hard-limit split path always produces responses at least as large as the hard limit divided by some factor, so this doesn't false-trigger for legitimate oversized blocks. ✓

• SlicedOwnedReadOnlyList ownership: the backing ownedHashes (ArrayPoolList) is disposed by the using in GetReceipts; all slices are consumed within that scope; Dispose() no-op is correct. ✓

• Memory safety in catch path: txReceipts.Dispose() in the catch block of FulfillReceiptsRequest and aggregated.Dispose() + partialReceipts?.Dispose() in SendGetReceiptsWithPaging ensure no leaks on error paths. ✓

• Should_not_copy_remaining_hashes_when_building_paged_receipts_requests: CountingReadOnlyList wraps the inner enumerator (no indexer reads from enumeration), so IndexerReadCount == blockHashes.Count asserts exactly one pass through ToPooledList() with zero additional per-page copies. ✓

No new bugs found.

---

One minor observation (not a blocker)

Line 415 of Eth70ProtocolHandler.cs — BuildRequest creates a slice but GetReceiptsMessage70 takes IOwnedReadOnlyList<Hash256>

When GetReceiptsMessage70 is disposed (via using in SendGetReceiptsWithPaging), it calls Dispose() on the slice (no-op). The original ownedHashes is still alive. This is intentional and correct by design, but it means the message's Hashes field becomes a dangling view after ownedHashes is disposed. In this code that race cannot happen (the using GetReceiptsMessage70 scope is nested inside the using ArrayPoolList<Hash256> ownedHashes scope in GetReceipts). Consider a doc comment on GetReceiptsMessage70 clarifying the lifetime contract, but this is purely informational.

---

Overall: the PR is ready to merge. All blocking issues are resolved and the implementation is correct, well-tested, and clean.

[claude]

Claude finished @flcl42's task in 6m 33s —— View job

---

PR Review: Improve eth/70 checks

• Gather context
• Load relevant agent rules
• Read changed files and diff
• Analyse implementation correctness and edge cases
• Post findings

---

Summary

This revision introduces accurate RLP-based size computation (ReceiptMessageDecoder69.GetLength()), per-receipt log-gas validation in ValidateBlockReceipts, and hard-limit enforcement on the requester side. All previously identified issues are fully resolved.

---

Previous findings — final status

#	Issue	Status
M1	OverflowException for FirstBlockReceiptIndex > int.MaxValue	Fixed — pre-cast bounds check + test
M2	Spurious lastBlockIncomplete=true on cancellation race	Fixed
L1	Redundant full re-validation of completed partial receipts	Fixed
L2	Synchronous FulfillReceiptsRequest returning Task<ReceiptsResponse>	Fixed — returns ReceiptsResponse directly; call site uses ValueTask.FromResult
L3	O(N²) hash copies in BuildRequest	Fixed — zero-copy Slice() via SlicedOwnedReadOnlyList
INF	CalculateLogsGas throws OverflowException instead of SubprotocolException	Fixed — AddLogGas wraps overflow as SubprotocolException

---

New changes — correctness walkthrough

Accurate RLP size accounting (GetReceiptSize, GetBlockReceiptsSize, GetEth70ReceiptsResponseSize)

GetReceiptSize now calls ReceiptMessageDecoder69.GetLength(receipt, behaviors) — the actual on-wire byte count — which correctly varies by EIP-658 spec. The behaviors cache keyed on receipt.BlockNumber avoids a GetReceiptSpec call per receipt within the same block. GetEth70ReceiptsResponseSize accounts for the full RLP envelope: request-ID length + 1-byte bool + inner-sequence overhead. GetRlpSequenceSize saturates to ulong.MaxValue when content exceeds int.MaxValue, ensuring all limit comparisons stay consistent. All correct.

Hard-limit enforcement on receive side (line 266)

if (size > HardOutgoingReceiptsMessageSizeLimit)
    throw new SubprotocolException($"Received eth/70 receipts response above hard limit …");

Closes the adversarial amplification gap. Correct.

Per-receipt log-gas validation

ValidateBlockReceipts now checks logsGas + GasCostOf.Transaction > receiptGasUpperBound for every receipt, not just complete segments. This catches malformed continuations mid-stream (tested by Rejects partial continuation when receipt logs exceed remaining gas). The previousGasUsed parameter threads correctly across pages: read from partialReceipts[^1].GasUsedTotal before AddRange at line 296, then reset to 0 on block completion.

partialReceiptsGas state machine

Traced through all page-boundary combinations: fresh block → partial → complete, multi-page partial, continuation-within-batch. No stale-reads or reset-ordering bugs found. Line 296 reads partialReceipts[^1] before AddRange (capturing end-of-previous-page gas), and line 317/344 reset to 0 on completion. Correct in all paths.

SoftOutgoingMessageSizeLimit units: .MB → .MiB

Test assertion Is.EqualTo(2UL * 1024 * 1024) confirms 2 097 152 bytes — the binary-unit interpretation. Correct.

---

Informational (no action required)

requestedStartIndex > receipts.Length vs >= at line 115

Allowing == means line 143 (startIndex >= receipts.Length) serves as the dead-code catch for that case. Semantically correct; >= at line 115 would make line 143 unreachable and slightly clearer. Mentioned in the previous review as a readability nit only.

logsGas + GasCostOf.Transaction is unchecked (line 471)

AddLogGas already throws SubprotocolException on overflow, so logsGas can never reach long.MaxValue. The addition at line 471 is therefore safe under any real-network data. No action needed.

---

New test coverage

• Should_count_rlp_wrappers_when_splitting_near_hard_limit and Should_count_actual_serialized_receipts_bytes_near_hard_limit — verify size is computed from actual serialized bytes, not estimates
• Should_send_single_large_block_above_soft_limit_when_below_hard_limit — exercises the soft→hard pass-through path
• InvalidPartialContinuationCases — parametrized test for per-receipt log-gas and header-gas mismatch rejections
• Should_throw_subprotocol_exception_when_logs_gas_overflows — unit-tests AddLogGas directly
• Should_not_copy_remaining_hashes_when_building_paged_receipts_requests — verifies O(N) indexer reads via CountingReadOnlyList
• Default_size_limits_match_eth_protocol_limits — pinpoint test for the .MiB unit change

No new bugs found. The PR is ready to merge.

[LukaszRozmej]

This code feels complicated, can it be simplified?

[LukaszRozmej]

maybe this should be a struct? (then returned by it's type from Slice to avoid boxing)