eth/71

[flcl42]

Changes

• Add eth/71 support(historical BALs sync)
• BAL sync happens after we got headers and state, in parallel to bodies/receipts
• Rewire inheritance for messages of new protocols like eth/70 eth/71 (containing of message replaced with inheritance)

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

[LukaszRozmej]

merge back master please

[LukaszRozmej]

IMO fix: Low/Nit — BlocksRequest default field initializers - rest is good, will do manual review later

[LukaszRozmej]

Close, few things to polish

[LukaszRozmej]

@claude review

[claude]

Claude finished @LukaszRozmej's task in 4m 18s —— View job

---

PR Re-review: eth/71 (Re-review #6)

Tasks

• Load repo rules
• Check commits since last review (b7571a3, 03dd76a, 1aa43b1, 911b5d9)
• Verify LukaszRozmej's May 1 inline comments were addressed
• Deep-check key changed files
• Post findings

---

Resolved since LukaszRozmej's May 1 deep review ✅

All 21 inline comments from LukaszRozmej's May 1 review have been addressed across commits 1aa43b1, b7571a3, 03dd76a, and 911b5d9:

Finding	Status
ChainUtils.cs — Amsterdam => AmsterdamGnosis.Instance	Fixed — AmsterdamGnosis.cs added (NamedGnosisReleaseSpec<AmsterdamGnosis>(Amsterdam.Instance, ...)) and mapping updated.
IBlockAccessListStore.cs — ThrowXXX helper	Fixed — ThrowMissingBlockHash([DoesNotReturn, StackTraceHidden]) added.
MetadataDbKeys.cs:21 — WhenStarted suffix	Acknowledged — consistent with existing ReceiptsBarrierWhenStarted / BodiesBarrierWhenStarted.
ForkchoiceUpdatedHandler.cs — unrelated change	Removed.
GetReceiptsMessageSerializerTests.cs — TestCaseSource	Fixed — [TestCaseSource(nameof(InvalidPayloads))] pattern now used.
Eth70ProtocolHandlerTests.cs:120 — test dedup	Done — tests refactored (769 lines, down from 884).
Eth71ProtocolHandlerTests.cs:52 — test dedup	Done — tests refactored (269 lines, down from ~380).
Eth66MessageSerializer.cs — ThrowXXX helper	Fixed — ThrowUnexpectedTrailingData([DoesNotReturn, StackTraceHidden]).
GetReceiptsMessage70.cs:11 — inheritance justification	Acknowledged — fewer allocations + clarity; approved in prior review.
SyncServerTests.cs — dedup	Done (commit 03dd76a).
BlockDownloader.cs — name the nulls	Fixed — named arguments: Block: null, Receipts: null, PeerInfo: null, EncodedAccessList: null.
BlockDownloader.cs:528 — long block, helper method	Fixed — TryHandleBlockAccessListResponse extracted as a private helper.
BlocksSyncPeerAllocationStrategyFactory.cs:11 — make base extensible	Fixed — private BlockAccessListsPeerAllocationStrategy inner class extends FilterPeerAllocationStrategy via abstract Filter(PeerInfo).
BlocksRequest.cs — request.AllCounts	Fixed — AllCounts => BodiesRequests.Count + BlockAccessListsRequests.Count + ReceiptsRequests.Count.
SyncServer.cs — single return	Fixed — GetBlockAccessListRlp is now an expression-bodied member.
Synchronizer.cs — WasAnyModeFinished(params ReadOnlySpan)	Fixed — private static bool WasAnyModeFinished(SyncModeChangedEventArgs e, params ReadOnlySpan<SyncMode> modes) added.
SyncReport.cs — helper method per if	Fixed — LogProgressIfActive(SyncMode, SyncMode, ProgressLogger) helper.
SyncReport.cs — HasAllEnded(params ReadOnlySpan)	Fixed — private static bool HasAllEnded(params ReadOnlySpan<ProgressLogger> progressLoggers).
FastBlocksPeerAllocationStrategyFactory.cs — switch expression	Fixed — TransferSpeedType speedType = request switch { ... }.
BlockAccessListsSyncFeed.cs — single return	Fixed — IsValidAccessList uses single-return via bool isValid.
BlockAccessListsSyncFeed.cs — base Exception?	Addressed — catch (Exception ex) when (ex is not OperationCanceledException) retained; broad catch is correct here to guarantee MarkPending before re-throw.

---

Additional change verified ✅

IOwnedReadOnlyList<T>.Empty (1aa43b1): BlocksRequest fields now default to IOwnedReadOnlyList<T>.Empty instead of new ArrayPoolList<T>(0). The EmptyOwnedReadOnlyList singleton has a no-op Dispose(), so the existing BlocksRequest.Dispose() path is safe.

---

Verdict: mergeable. All Critical/High/Medium findings from all prior reviews are resolved, and all inline feedback from LukaszRozmej's May 1 deep review has been addressed. No new issues found in this pass.

[flcl42]

@claude check it again

[claude]

Claude finished @flcl42's task in 8m 29s —— View job

---

PR Re-review: eth/71 (Re-review #7)

Tasks

• Gather context and load repo rules
• Check recent commits since last review (a58dc4eb Review, 4f5e94c1 Migration, ab77c21c Fix build)
• Verify asdacap's review comments were addressed
• Deep review of BlockAccessListsMessageSerializer, SyncPointers, BlockDownloader
• Verify memory ownership and dispose correctness
• Post findings

---

Resolved since review #6 ✅

All three comments from asdacap's 2026-05-06 review have been addressed:

asdacap's Finding	Status
BlockDownloader.cs:328 — need bodiesOnly-style flag for BAL-only requests	Fixed — bool? bodiesOnly replaced with BlocksRequestContentType? enum (Bodies, BlockAccessLists, Receipts). Each type now gates the others via requestContentType is null or BlocksRequestContentType.X, correctly enforcing one-type-per-request.
BlockAccessListsMessage.cs — use RlpByteArrayList / DeferredRlpItemList.Builder	Fixed — message type changed from IOwnedReadOnlyList<byte[]?> to IByteArrayList. Deserializer uses NettyBufferMemoryOwner + RlpByteArrayList.DecodeList for zero-copy read. Server-side Handle(GetBlockAccessListsMessage) uses DeferredRlpItemList.Builder + Writer for zero-copy construction. SerializeInternal goes through IRlpWrapper.Write path (direct buffer write, no item-by-item re-encoding).
SyncPointers.cs — use metadata DB instead of raw BAL/blocks DB key	Fixed — both LowestInsertedBodyNumber and LowestInsertedBlockAccessListBlockNumber now stored under MetadataDbKeys.LowestInsertedBodyNumber (15) and MetadataDbKeys.LowestInsertedBlockAccessListBlockNumber (16). Migration for legacy LowestInsertedBodyNumber (stored as IDb[0] in blocks DB) is included and tested (MigrateLegacyLowestInsertedBodyNumber).

---

Memory ownership trace (verified correct)

Deserialization path (client):
NettyBufferMemoryOwner → RlpItemList (via DecodeList) → RlpByteArrayList → BlockAccessListsMessage.BlockAccessLists. After GetBlockAccessLists returns, caller owns the IByteArrayList. BlocksRequest.Dispose() / BlockAccessListsSyncBatch.Dispose() both call BlockAccessLists?.Dispose() / Response?.Dispose(), which chains through RlpByteArrayList.Dispose() → RlpItemList.Dispose() (idempotent via Interlocked.CompareExchange) → releases the Netty buffer. Spans passed to _blockAccessListStore.Insert(hash, span) and encodedAccessList.ToArray() are all consumed before the owning list is disposed. ✅

Serialization path (server):
DeferredRlpItemList.Builder.ToRlpItemList() transfers ownership (sets _entries = null; subsequent builder.Dispose() is a no-op). RlpByteArrayList backed by DeferredRlpItemList (owned ArrayPoolList<Entry> + ArrayPoolList<byte>). Written directly via IRlpWrapper.Write path → no extra copies. Disposed when BlockAccessListsMessage.Dispose() is called after the Netty frame is sent. ✅

Error paths in BlockAccessListsMessageSerializer.Deserialize:
If Rlp.GuardLimit / ctx.Check throw before memoryOwner = null (ownership transfer), catch block disposes both blockAccessLists and memoryOwner. NettyBufferMemoryOwner.Dispose() is idempotent (_disposed flag), so the double-dispose is safe. ✅

---

No new issues found

• BlocksRequestContentType enum covers all three types cleanly; the break conditions (Count >= requestSize) remain correct for each type.
• InsertBlockAccessLists hasBreachedProtocol flag (fixed in review Hive #4) remains correct.
• HasPivot pre-fork guard (fixed in review Patricia alt 2 #5) remains correct.
• All resource leaks from reviews Feature/networking #2–Hive #4 remain fixed.
• BlockAccessListsMessageSerializer.DeserializeInternal throwing NotSupportedException is an intentional guard (correct design — the base class's Deserialize(IByteBuffer) is overridden and the DeserializeInternal path is unreachable in normal operation).
• RLP security audit: count limits, position checks, and null-span handling all intact. ✅

---

Low / Nit (non-blocking)

• IByteArrayList.cs and RlpByteArrayList.cs carry // SPDX-FileCopyrightText: 2025 while the rest of the PR is 2026. Trivial inconsistency.
• No migration for LowestInsertedBlockAccessListBlockNumber from the old BAL DB key (_blockAccessListsDb.Set(Keccak.Zero, ...)) used in prior revisions of this branch. Since eth/71 was never on master, this only affects intermediate-build testers; production nodes are unaffected.

---

Verdict: mergeable. All Critical/High/Medium findings from all prior reviews are resolved. asdacap's three comments are all addressed. No new issues found in this pass.