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)

[flcl42]

@LukaszRozmej

What part is the most disturbing? When this protocol was discussed what bothered me is that we could replace this gas accounting for incomplete receipts with receipt root proofs like we do for snap think, which seems simpler to me. But I guess you mean something different

@LukaszRozmej @benaadams

We need IOwnedReadOnlyList in GetReceiptsMessage70 which we not only send but also receive and need to clean up after the usage. Span do not work for such case. Dispose is called on parent for sure.

[benaadams]

We need IOwnedReadOnlyList in GetReceiptsMessage70 which we not only send but also receive and need to clean up after the usage. Span do not work for such case. Dispose is called on parent for sure.

Shouldn't implement Dispose and make it empty then?

[LukaszRozmej]

@claude re-review

[claude]

Claude finished @LukaszRozmej's task in 3m 27s —— View job

---

PR Re-Review: Improve eth/70 checks

• Gather context from previous reviews
• Load relevant agent rules
• Read changed files (current state)
• Analyse complexity, correctness, and reviewer concerns
• Post findings

---

Summary

This is the fifth pass. All previously identified Medium and Low bugs remain fixed. The re-review is specifically prompted by LukaszRozmej's complexity concern and the SlicedOwnedReadOnlyList design questions from benaadams.

---

Reviewer concerns — addressed

LukaszRozmej: "this code feels complicated, can it be simplified or better encapsulated?"

FulfillReceiptsRequest has five branches that each map to a distinct EIP-7975 scenario:

Branch	Condition
1	Empty block — include if soft-limit allows, else stop
2	Remaining block fits within soft limit — include whole
3	Response already has content, block overflows soft limit — stop
4	Block overflows soft limit but fits hard limit — include whole
5	Block overflows hard limit — split per-receipt

The complexity is inherent to the protocol. The validation side (ValidateBlockReceipts and its helpers) is already well-extracted. The main loop itself could be marginally flatter, but the current structure is readable given the sequential guard-clause shape. No functional issue — this is a design opinion.

benaadams: "Should the dispose be calling parent?" / "Otherwise just return the ReadOnlySpan?"

SlicedOwnedReadOnlyList.Dispose() is intentionally a no-op: the slice is a view; ownership of the backing ArrayPoolList<Hash256> stays with the using block in GetReceipts. Calling _list.Dispose() here would double-dispose the backing list. The no-op is correct. A ReadOnlySpan can't cross async/await boundaries, so the interface approach is necessary.

A one-line comment on the empty Dispose() would prevent the same question in future reviews (see Low #2 below).

LukaszRozmej: "maybe this should be a struct? (then returned by its type from Slice to avoid boxing)"

A struct would avoid boxing only if all call sites hold the concrete SlicedOwnedReadOnlyList<T> type. The hot path here is BuildRequest → GetReceiptsMessage70.Hashes (typed IOwnedReadOnlyList<Hash256>) — the struct would be boxed on construction of the message regardless. The allocation saved is one SlicedOwnedReadOnlyList instance per paged request, which is negligible relative to the allocation for the message itself. Not worth the API churn.

---

New findings

Low

L1 – Stale // ChatGPT generated comment in SlicedReadOnlyList.cs (SlicedReadOnlyList.cs:11)

SlicedReadOnlyList.cs is modified in this PR (the protected access-modifier change). The existing summary comment /// ChatGPT generated sliced read only list is inappropriate for production code and should be replaced with a brief description of the type's purpose. Fix this →

L2 – Empty Dispose() on SlicedOwnedReadOnlyList needs a clarifying comment (IOwnedReadOnlyList.cs:72)

Without documentation, the empty body looks like a mistake (as confirmed by benaadams' review). A one-line comment stating "the backing list is owned and disposed by the caller" would close this question. Fix this →

---

Correctness re-verification (clean)

• FulfillReceiptsRequest five-branch structure: all branches have been checked across empty blocks, soft-limit fits, hard-limit pass-through, and split. No regression. startIndex + taken < receipts.Length is correct for lastBlockIncomplete.
• SendGetReceiptsWithPaging state machine: blockIndex, firstBlockReceiptIndex, partialReceipts, partialReceiptsGas transitions verified across first page, continuation, and completion.
• ValidateBlockReceipts gas lower-bound: Math.Max(previousGasUsed, firstReceiptIndex * GasCostOf.Transaction) — correct for both fresh and continuation segments.
• partialReceiptsGas = partialReceipts[^1].GasUsedTotal read before AddRange — captures end-of-previous-page gas correctly.
• SlicedOwnedReadOnlyList lifetime: all slices used within using ArrayPoolList<Hash256> ownedHashes scope. No dangling-view risk in practice.
• Hard-limit enforcement on receive side (line 266): correctly rejects oversized responses before processing receipts.
• taken == 0 guard (line 192): throws SubprotocolException instead of sending a zero-receipt block; prevents infinite-loop on pathological single receipt exceeding hard limit.

No new bugs found.

---

Status of all tracked findings

#	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()
INF	CalculateLogsGas throws OverflowException	Fixed — AddLogGas wraps as SubprotocolException
New L1	SlicedReadOnlyList.cs "ChatGPT generated" comment	Open
New L2	SlicedOwnedReadOnlyList.Dispose() no-op lacks comment	Open

The two new Low items are style/documentation issues only — neither affects runtime correctness or security.

[benaadams]

@claude re-review

[claude]

Claude finished @benaadams's task in 6m 38s —— View job

---

PR Re-Review: Improve eth/70 checks

• Gather context from previous reviews
• Load relevant agent rules
• Read changed files (current state)
• Analyse Gas remake commit correctness and edge cases
• Post findings

---

Summary

This is the sixth pass. The new commit is d83296b ("Gas remake"), which replaces the per-receipt log-gas validation approach with RLP-size-based bounds and adds receipt-count/transaction-count cross-validation.

---

Previous open items — final status

#	Issue	Status
New L1 (5th review)	Stale "ChatGPT generated" comment in SlicedReadOnlyList.cs	Fixed — now /// A read-only view over a contiguous segment of an <see cref="IReadOnlyList{T}"/>.
New L2 (5th review)	Empty Dispose() on SlicedOwnedReadOnlyList lacks comment	Fixed — now // The slice does not own the backing list; the caller disposes the original list.

---

"Gas remake" commit — correctness walkthrough

The key change is replacing ValidateReceiptGasCoversLogs (logsGas ≤ receiptGasUsed − 21000) with two new size-based validations and a receipt-count check:

New ValidateReceiptSizeAgainstTransactionGasLimit (receiptRlpSize ≤ tx.GasLimit / 8)

• Uses tx.GasLimit (capacity) not tx.GasUsed (actual) — this is intentionally looser, but the EVM enforces valid blocks on-chain, and fabricated receipts would fail the receipts root check elsewhere. Correct and tested.

New ValidateTotalReceiptsSizeAgainstBlockGasLimit (cumulative receiptsRlpSize ≤ blockGasLimit / 8)

• Log data costs ≥ 8 gas/byte (GasCostOf.LogData = 8), so blockGasLimit / 8 bytes is a valid upper bound on total receipt payload. The receiptsContentSize accumulator correctly threads across pages via partialReceiptsContentSize. Correct.

New ValidateReceiptCount (firstReceiptIndex + deliveredCount vs transactions.Length)

• For partial segments (isCompleteSegment = false): only the > transactions.Length overflow check applies — continuation pages do not require the complete count to match yet. For complete segments: equality is enforced. Correct.

Spec flags IsEip7778Enabled / IsEip8037Enabled (Amsterdam fork)

• IsEip8037Enabled = true → both gas header bound and equality checks skipped. This is intentional for Amsterdam's zkVM/stateless execution where gas accounting differs.
• IsEip7778Enabled = true → equality check only is skipped (upper-bound still applied). The AmsterdamNoEip8037Instance in Forks/25_Amsterdam.cs confirms this is a deliberate, opt-in split.

partialReceiptsContentSize state machine: receiptsContentSize starts at previousReceiptsContentSize inside ValidateBlockReceipts, accumulates per-receipt, and the total is returned. The caller updates partialReceiptsContentSize from validationResult.ReceiptsContentSize and resets to 0 on block completion. Traced through all page-boundary cases — correct.

SyncServer.Find called upfront for all block hashes (SendGetReceiptsWithPaging:260): Full block bodies are fetched for every hash before paging begins, to populate blockTransactions for count/size validation. When the body is unavailable (null or IsBodyMissing), transactions = null and those validations are skipped. Functionally correct; the extra DB round-trips are an acceptable cost for richer validation.

GetReceiptSizeLimit(0) = 0: Any non-zero-size receipt for a zero-gas-limit transaction would throw. Transactions with GasLimit < 21000 are invalid and never in a block — not a reachable path.

checked(firstReceiptIndex + i) in ValidateBlockReceipts: Overflow impossible in practice — firstReceiptIndex is bounded by block receipt count, itself bounded by block gas limit.

Should_accept_when_logs_gas_exceeds_post_refund_gas_used: Correctly documents the intentional loosening — a receipt with log gas exceeding post-refund gas delta now passes (only size bounds apply).

No new bugs found.

---

Informational (no action required)

The removal of ValidateReceiptGasCoversLogs relaxes the per-receipt invariant: a peer could now send a receipt claiming minimal gas used but with large log data, as long as receiptRlpSize ≤ txGasLimit / 8. The old check bounded logs by the gas delta; the new check bounds the entire receipt by the gas budget. For valid on-chain blocks this is equivalent (EVM enforces it); for fabricated receipts the mismatch would be caught by the receipts root check. The design trade-off is explicitly tested and documented.

---

Status of all tracked findings

#	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
INF	CalculateLogsGas throws OverflowException	Removed (entire ValidateReceiptGasCoversLogs path replaced)
New L1 (5th)	Stale "ChatGPT generated" comment	Fixed
New L2 (5th)	Empty Dispose() lacks comment	Fixed

The PR is ready to merge. All blocking issues have been resolved; the remaining informational note is a design decision the author has clearly made intentionally.

[benaadams]

@claude re-review

[claude]

Claude finished @benaadams's task in 3m 42s —— View job

---

PR Re-Review: Improve eth/70 checks

• Gather context from previous reviews
• Load relevant agent rules
• Read changed files (new commit since 6th review)
• Analyse correctness and edge cases
• Post findings

---

Summary

This is the seventh pass. The new commit since the 6th review is 27a5423 ("Fix full sync"). All previously identified issues remain fixed. No new bugs found.

---

New commit — 27a5423 Fix full sync

What changed:

BlockDownloader.AssembleSatisfiedEntries and AssembleRequest now receive a shouldProcess parameter. Two guards were added:

1. AssembleSatisfiedEntries (line 386): if (!shouldProcess && !blockEntry.HasAccessList) break;
   — Blocks no longer need a downloaded access list to be considered "satisfied" when in full sync (processing) mode.

2. AssembleRequest (line 319): if (!shouldProcess && (requestContentType is null or BlocksRequestContentType.BlockAccessLists) && entry.NeedAccessListDownload)
   — Access list requests are not issued in processing mode.

Correctness analysis:

The fix is correct. Access lists are only required for stateless/archival execution (Insert mode), not for processing blocks through the EVM (Process mode). The original code blocked full sync whenever a block header had a non-null BlockAccessListHash (e.g., post-Amsterdam blocks), preventing any forward progress in processing mode.

Edge case — ReceiptEdgeCase flipping shouldProcess to false: When isFastSyncTransition = true and the full state is not yet available, ReceiptEdgeCase can flip shouldProcess from true to false. Both guards correctly use the post-flip shouldProcess value, so access list download/requirement is re-enabled when processing is turned off mid-loop. Correct.

New tests:

Test	What it verifies
Full_sync_processing_does_not_request_block_access_lists	PrepareRequest(Process) yields a body request only — no access list request — even for a header with BlockAccessListHash set
Full_sync_processing_satisfies_block_without_downloaded_block_access_list	Once the body arrives, the block entry is satisfied and suggested (BestSuggestedHeader.Number == 1) without waiting for an access list

Both tests use ConfigureBlockAccessListRequest which creates a header with BlockAccessListHash = TestItem.KeccakA — correctly exercising the case that would require an access list in archival mode. The "no second request" assertion relies on the stale-starting-header guard (line 162–168), which terminates the loop cleanly after the block is suggested. All paths are correct.

---

Status of all tracked findings

#	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()
INF	CalculateLogsGas throws OverflowException	Fixed — AddLogGas wraps as SubprotocolException
New L1 (5th)	Stale "ChatGPT generated" comment in SlicedReadOnlyList.cs	Fixed
New L2 (5th)	Empty Dispose() on SlicedOwnedReadOnlyList lacks comment	Fixed
New (7th)	Full sync blocked waiting for access lists — fix in BlockDownloader	Fixed — 27a5423

The PR is ready to merge. All blocking issues are resolved; the fix in 27a5423 is correct and well-tested.