perf(merge-sync): cache PosForwardHeaderProvider.GetBlockHeaders

[0xDevNinja]

Resolves #8573

Changes

• PosForwardHeaderProvider.GetBlockHeaders now fetches a 4 * maxHeader batch from ChainLevelHelper once, caches it, and serves subsequent calls by slicing the cached array.
• Cache is invalidated when IBeaconPivot.ProcessDestination (hash or number) changes, when BestKnownNumber advances past the cached end, or when the requested slice cannot be satisfied without going below the cached start.
• Pre-merge path (base.GetBlockHeaders) is unchanged; the LastResponseBatch mechanism in PowForwardHeaderProvider continues to drive that branch.
• ValidateSeals runs once per cache fill (on the freshly fetched array) rather than on every peer allocation.

Types of changes

What types of changes does your code introduce?

• Optimization

Testing

Requires testing

• Yes

If yes, did you write tests?

• Yes

Notes on testing

PosForwardHeaderProviderCacheTests (5 cases) covers: cache hit on identical inputs, invalidation when ProcessDestination.Hash changes, invalidation when BestKnownNumber advances past the cached range, slice advances with BestKnownNumber while inside the range, and skipLastN honored on cache hits. Existing ForwardHeaderProviderTests (37/38 pass, 1 pre-existing skip) continue to pass.

Documentation

Requires documentation update

• No

Requires explanation in Release Notes

• No

Remarks

Invalidation strategy follows the hint left in the issue: with no explicit signal for when ChainLevelHelper would return different headers, identity of BeaconPivot.ProcessDestination (set by NewPayload/ForkchoiceUpdated) plus BestKnownNumber advance covers the cases where the underlying chain-level data could have moved. Happy to swap to a more aggressive (e.g. BlockTree.NewHeadBlock) invalidation if reviewers prefer.

[LukaszRozmej]

Deep review

Verdict: request changes. The optimization is sound in concept, but the cache slicing has an off-by-one that drops the anchor block on every cache hit in the typical PoS-sync state (the walk-back case). The test suite doesn't catch it because the mock simulates a non-walk-back scenario that doesn't occur in real sync.

---

1. Critical: off-by-one in desiredStart drops the anchor block

TryServeFromCache computes:

long desiredStart = Math.Min(_blockTree.BestKnownNumber + 1, currentNumber);

The + 1 is wrong for the dominant PoS-sync state:

• During active beacon sync, beacon-only headers don't advance BestKnownNumber (BlockTree.cs only updates it when !beaconInsert). So BestKnownNumber == latest_canonical_block and BestKnownNumber + 1 is a beacon-info block.
• ChainLevelHelper.GetStartingPoint() starts at min(BestKnown+1, ProcessDestination.Number). When ProcessDestination > BestKnown (active sync) the block at BestKnown+1 is IsBeaconInfo, so the do/while walks back one level and returns (BestKnown, hash_of_BestKnown_block) — i.e. headers[0].Number == BestKnownNumber in the array GetNextHeaders produces.
• Same in the caught-up case (ProcessDestination == BestKnown → startingPoint = BestKnown, no walk-back, headers[0] = BestKnown).

So in both real-world states, the original chainLevelHelper.GetNextHeaders returns [block@BestKnown, block@BestKnown+1, …]. BlockDownloader relies on this: headers[0] is the already-known anchor used as parentHeader, and headers.Skip(1) is the to-download set (see AssembleRequest, AssembleSatisfiedEntries, and the ReceiptEdgeCase(…, headers[1].Number, …) call site).

With this PR, on the first call (cache miss) fresh starts at BestKnown as expected. On subsequent cache hits with the same BestKnownNumber, desiredStart = BestKnown+1, offset = 1, and the returned slice is [BestKnown+1, …, BestKnown+N] — the anchor at BestKnown is dropped. Downstream effects:

1. AssembleSatisfiedEntries iterates headers.Skip(1) starting at BestKnown+2, so the body for the block at BestKnown+1 (queued by the previous iteration's AssembleRequest) is never moved into a satisfied entry and never suggested — it lingers in _downloadRequests.
2. AssembleRequest then sets parentHeader = headers[0] = block@BestKnown+1, which itself is a beacon-only header. BlockTree.SuggestBlock(K+2) will likely accept (beacon header at K+1 exists), but BestKnownNumber then advances past K+1 with the canonical chain having a state-less hole at K+1. In process mode this stalls the processor; in fast sync it leaks the request and silently skips one body download per cache hit.
3. The previousStartingHeaderNumber == headers[0].Number guard in BlockDownloader.DoPrepareRequest flips one iteration earlier than before, mostly benign.

The new tests don't catch this because the chain-helper mock returns BuildSequentialHeaders(start: 1, count: 64) with BestKnownNumber == 0. That's a "no walk-back, headers start at BestKnown+1" shape — unreachable in real PoS sync. The test asserts second[0]!.Number.Should().Be(21) after advancing BestKnownNumber to 20, but in production the same call would have returned [20, 21, …, 35] (anchor at 20 included).

Fix. The cache must always present the anchor:

long desiredStart = Math.Min(_blockTree.BestKnownNumber, currentNumber);

(or equivalently offset = (int)(_blockTree.BestKnownNumber - cacheStart)), and add a regression test that mocks BuildSequentialHeaders(start: 0, count: 64) so the slice's first element is the anchor at BestKnownNumber, not the first new block.

---

2. ValidateSeals no longer revalidates the boundary on each slice

Original code called ValidateSeals on the returned slice, force-validating the last element and a random index (and any terminal-block transitions). This PR runs it once on the full fresh array, so cache hits get only the validations from the original fill. For PoS this is mostly fine (seal validation is effectively a no-op), but the forced check on suspected terminal blocks (headers[i+1].Difficulty == 0 transitions) is no longer guaranteed at the slice boundary. Likely benign post-merge, but call it out as an explicit trade-off, or rerun ValidateSeals(slice, cancellation) on cache hits (cheap for PoS, safer for the transition region).

---

3. skipLastN semantics drift

Original: chainLevelHelper.GetNextHeaders(maxHeader, long.MaxValue, skipLastN) — skipLastBlockCount trims from the tail of the available chain (ChainLevelHelper.cs:69-70, 147-155).

PR: always passes skipLastBlockCount: 0, then applies skipLastN as a count cap inside BuildSlice / TryServeFromCache. When fresh.Length >= maxHeader + skipLastN the two are equivalent, but in the small/end-of-chain regime where ChainLevelHelper returns fewer headers than fetchSize, the new code returns up to fresh.Length - skipLastN from the front, while the original truncated from the tail of the available chain. Worth either a clarifying comment or restoring the parameter pass-through on cache miss.

---

4. Invalidation strategy is too coarse

ProcessDestination identity + BestKnownNumber > cacheEnd will miss:

• Reorgs landing inside the cached range that don't change ProcessDestination (e.g. concurrent FCU keeping the same head but with a competing branch at lower height). Cached headers are then stale; ChainLevelHelper would re-read chain-level info.
• ProcessDestination rewinds: the hash check catches this, good — but the Number comparison uses long.MaxValue sentinel for null, masking a null transition. Verify intentional.

The PR remarks already flag this. I'd prefer subscribing to BlockTree.NewHeadBlock (BlockDownloader already does), nuking the cache on reorg, rather than relying on ProcessDestination identity alone.

---

5. Smaller things

• TryServeFromCache reads cache fields under _cacheLock but accesses cached[0].Number / cached[^1].Number outside the lock — safe today (arrays are immutable post-publish) but worth a comment.
• cached.Length == 0 is unreachable given UpdateCache guards on MinCachedHeaderBatchSize. Drop it.
• MinCachedHeaderBatchSize = 32 duplicates the same const in PowForwardHeaderProvider. Hoist to the base class.
• TryServeFromCache allocates a new BlockHeader[take] and the caller wraps it via ToPooledList(slice.Length) — two allocations per hit. Either populate ArrayPoolList directly or hand the array to ToPooledList(array, count).
• _cacheLock is redundant in practice (BlockDownloader._requestLock serializes callers), but fine as defense-in-depth — a one-liner comment would help.
• BlockHeader[]? vs BlockHeader?[]? mismatch forces slice! casts; clean up the nullable-element shape in the slice path for consistency.

---

Summary

#	Severity	Issue
1	Blocker	Off-by-one in desiredStart drops the anchor on cache hits in the walk-back path (typical PoS sync). Test mock hides it.
2	Medium	ValidateSeals no longer runs per slice — terminal-transition forced check may be skipped.
3	Low	skipLastN semantics drift from tail-trim to front-cap.
4	Medium	Cache invalidation misses reorgs that don't touch ProcessDestination; consider subscribing to BlockTree.NewHeadBlock.
5	Nit	Const duplication, double allocation on hit, dead branch, nullable-shape cleanup.

Fixing #1 and adding a walk-back regression test (mock start: 0 so the anchor is included) would unblock; #4 worth revisiting before merge.

[LukaszRozmej]

@claude review

[claude]

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

---

Code Review: perf(merge-sync): cache PosForwardHeaderProvider.GetBlockHeaders

• Gather context and read rules
• Read changed files (PosForwardHeaderProvider.cs, ChainLevelHelper.cs, PowForwardHeaderProvider.cs, test file)
• Analyze code correctness, performance, and test coverage
• Post full review

---

Summary

The caching concept is sound — avoiding repeated ChainLevelHelper round-trips by serving a 4× pre-fetched batch is a worthwhile optimization. However there is one critical blocker: an off-by-one that silently drops the anchor block on every single cache hit, paired with a test mock that was accidentally written to hide that exact bug.

---

Critical

1. Off-by-one in TryServeFromCache drops the anchor on every cache hit (line 98)

ChainLevelHelper.GetStartingPoint() always returns the last processed (non-beacon-info) block as the anchor. In the typical active-sync state the walk-back loop returns (BestKnownNumber, hash_of_BestKnownNumber), so fresh[0].Number == BestKnownNumber and the anchor is stored at index 0 in the cache.

On the first call (cache miss), BuildSlice correctly returns [block@BestKnown, block@BestKnown+1, …].
On subsequent calls (cache hit, unchanged BestKnownNumber):

desiredStart = BestKnownNumber + 1   // +1 is wrong
offset       = 1
slice        = [block@BestKnown+1, …]  // anchor dropped

BlockDownloader relies on headers[0] being the already-known anchor that seeds AssembleSatisfiedEntries. Dropping it means:

• The body for BestKnown+1 is never moved to a satisfied entry and lingers in _downloadRequests.
• parentHeader on the next AssembleRequest becomes a beacon-only header, potentially causing a state hole in process mode or a silent skipped body in fast-sync mode.

Fix: long desiredStart = Math.Min(_blockTree.BestKnownNumber, currentNumber);

2. Test mock hides the bug (line 36 of test file)

The mock returns BuildSequentialHeaders(start: 1, count: 64) with BestKnownNumber = 0.
This gives cacheStart = 1, desiredStart = BestKnownNumber + 1 = 1, offset = 0 — the only case where the off-by-one is invisible.

In production GetStartingPoint() returns (BestKnownNumber, …) so cacheStart == BestKnownNumber and offset would be 1 on the next hit. The mock should use start: 0 to match. The Cached_slice_advances_with_best_known_number assertion second[0]!.Number.Should().Be(21) also asserts the buggy behaviour — it should be 20 (the anchor at the new BestKnownNumber).

---

Medium

3. ValidateSeals not re-run per slice (line 72)

Running it once on the large fill means the forced last-element check and terminal-block transition detection are fixed at fill positions, not at the slice boundaries served on cache hits. Safe in pure PoS, but a latent risk in the merge-transition region. Consider calling ValidateSeals(slice, cancellation) on cache hits — it is a no-op post-merge and restores the original safety invariant.

4. Cache invalidation misses intra-range reorgs (noted in PR remarks)

ProcessDestination identity + BestKnownNumber > cacheEnd won't catch a reorg that lands inside the cached range without changing ProcessDestination (e.g. a competing FCU at a lower height than current ProcessDestination). BlockDownloader already subscribes to BlockTree.NewHeadBlock; subscribing there and evicting the cache on reorg would be the more robust approach.

---

Low / Nit

5. skipLastN semantics drift (line 64)
Passing skipLastBlockCount: 0 to the large fetch includes headers right up to the chain tip; the original forwarded skipLastN to ChainLevelHelper which trimmed from the tail of the available window. In the end-of-chain regime the two diverge and may serve headers too close to a finalization boundary.

6. MinCachedHeaderBatchSize = 32 duplicated from base class — hoist to PowForwardHeaderProvider or a shared constant.

7. cached.Length == 0 guard on line 91 is unreachable — UpdateCache only stores arrays of at least MinCachedHeaderBatchSize elements.

8. Double allocation on cache hit — new BlockHeader[take] in TryServeFromCache + ToPooledList(slice.Length) at the call site. Either populate ArrayPoolList directly inside TryServeFromCache, or expose an overload that accepts the pre-allocated array to avoid one copy.

9. _cacheLock comment — A one-liner noting it is defense-in-depth (callers are already serialized by BlockDownloader._requestLock) would help future readers understand why the lock exists given the serialized call pattern.

---

Verdict

Not mergeable — the off-by-one on desiredStart drops the anchor block on every cache hit in production PoS sync. The test suite does not exercise the real start == BestKnownNumber shape that ChainLevelHelper produces.

Fixing line 98 (+ 1 → no + 1) and updating the mock to start: 0 with the matching assertion change will unblock this. The medium findings (items 3–4) are worth addressing before merge; the rest are polish.

[claude]

Critical: off-by-one drops the anchor block on every cache hit

ChainLevelHelper.GetStartingPoint() always returns the last processed (non-beacon) block as the anchor. In the typical active-sync state where BestKnownNumber + 1 is a beacon-info block, the walk-back loop returns (BestKnownNumber, hash_of_BestKnownNumber), so fresh[0].Number == BestKnownNumber. The cache therefore stores the anchor at index 0.

On the first call (cache miss) BuildSlice correctly returns [block@BestKnown, …]. On subsequent calls (cache hit, same BestKnownNumber):

desiredStart = BestKnownNumber + 1   // ← wrong
offset       = 1
slice        = [block@BestKnown+1, …]  // anchor dropped

BlockDownloader relies on headers[0] being the already-known anchor that seeds AssembleSatisfiedEntries / AssembleRequest. Dropping it means the body for BestKnown+1 is never moved to a satisfied entry and lingers in _downloadRequests, and parentHeader becomes a beacon-only header on the next iteration.

Fix:

Suggested change

	long desiredStart = Math.Min(_blockTree.BestKnownNumber + 1, currentNumber);
	long desiredStart = Math.Min(_blockTree.BestKnownNumber, currentNumber);

[claude]

Test mock hides the off-by-one bug

The mock returns BuildSequentialHeaders(start: 1, count: 64) with BestKnownNumber = 0. This means desiredStart = Math.Min(0+1, 1000) = 1 = cacheStart, so offset = 0 — the exact case that avoids the off-by-one.

In production, ChainLevelHelper.GetStartingPoint() returns the anchor at BestKnownNumber, so fresh[0].Number == BestKnownNumber. The mock should match this:

Suggested change

	_chainLevelHelper.GetNextHeaders(default, default, default).ReturnsForAnyArgs(_ => BuildSequentialHeaders(start: 1, count: CachedBatchSize));
	_chainLevelHelper.GetNextHeaders(default, default, default).ReturnsForAnyArgs(_ => BuildSequentialHeaders(start: 0, count: CachedBatchSize));

With this change cacheStart = 0 = BestKnownNumber, so desiredStart = BestKnownNumber + 1 = 1 ≠ 0 and the off-by-one is exposed. The Cached_slice_advances_with_best_known_number assertion second[0]!.Number.Should().Be(21) would also need updating to Be(20) (the anchor at the new BestKnownNumber).

[claude]

Medium: skipLastN semantics differ from the original

The original code forwarded skipLastN directly to ChainLevelHelper as skipLastBlockCount, which appended skipLastN extra headers to the fetch window and then trimmed them from the tail — effectively ensuring skipLastN blocks at the tip of the available chain are not included.

With skipLastBlockCount: 0 here, the fetch retrieves fetchSize headers right up to the tip, caches them all, and then trims skipLastN at serve time from the end of the cached window. In the end-of-chain regime where ChainLevelHelper returns fewer than fetchSize headers (i.e. close to ProcessDestination), the two semantics diverge: the original would exclude the last skipLastN of the available chain; this version may include them if the slice happens to reach the end of the cache.

At minimum worth a comment; worst case it may serve headers too close to an in-progress finalization boundary.

[claude]

Medium: ValidateSeals no longer runs on each served slice

The original code called ValidateSeals on every returned batch. Post-merge seal validation is a no-op, but the method also contains the terminal-block transition check:

bool terminalBlock = !lastBlock && headers[i+1].Difficulty == 0 && headers[i].Difficulty != 0;
bool forceValidation = lastBlock || i == randomNumberForValidation || terminalBlock;

Running it once on the large fresh array means: (a) the forced last-element check and the random-index check are fixed at fill time, not per-slice; (b) terminal-block transitions at a slice boundary (not present in the original fetch) are never force-validated on cache hits. This is safe in pure PoS but is a latent risk in the merge-transition region. Consider calling ValidateSeals(slice, cancellation) on cache hits too — it's cheap post-merge and restores the original safety invariant.

[LukaszRozmej]

@claude re-review

[claude]

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

---

Re-review: perf(merge-sync): cache PosForwardHeaderProvider.GetBlockHeaders

• Load tools
• Read changed files (PosForwardHeaderProvider.cs, PosForwardHeaderProviderCacheTests.cs, ChainLevelHelper.cs)
• Verify prior findings against current code
• Post final re-review

---

The PR has not been updated since the initial deep review (May 15) and the first automated review (today, May 20). All previously identified findings remain open on the current code.

---

Status of Prior Findings

#	Severity	Finding	Status
1	Critical	Off-by-one in TryServeFromCache (desiredStart = BKN+1) drops anchor on cache hit in walk-back path	Still present — line 98 unchanged
2	Critical	Test mock uses start: 1 / BKN=0 (no-walk-back shape) and hides the bug	Still present — line 36 unchanged
3	Medium	ValidateSeals not re-run per slice; terminal-transition forced check missed at slice boundaries	Still present — line 72 unchanged
4	Medium	Cache invalidation misses intra-range reorgs that don't change ProcessDestination	Still present
5	Low	skipLastN semantics drift: original trimmed from chain tail; PR front-caps the cached window	Still present — line 64 unchanged
6	Nit	MinCachedHeaderBatchSize = 32 duplicated from PowForwardHeaderProvider (base class already has it at line 40)	Still present — line 33 unchanged
7	Nit	cached.Length == 0 branch on line 91 is unreachable (UpdateCache guards on MinCachedHeaderBatchSize)	Still present
8	Nit	Double allocation per cache hit (new BlockHeader[take] + ToPooledList)	Still present

---

Critical Analysis (re-confirmed against ChainLevelHelper.cs)

Reading ChainLevelHelper.GetStartingPoint() directly confirms the blocker. The method has two exit paths:

Path A — no walk-back (block at BKN+1 is already processed, !IsBeaconInfo):

// line 179-182
if (!beaconMainChainBlock.IsBeaconInfo)
    return (startingPoint, beaconMainChainBlock.BlockHash); // returns BKN+1

Here headers[0].Number == BKN+1. The +1 in line 98 is coincidentally correct.

Path B — walk-back (typical active beacon sync, BKN+1 is IsBeaconInfo):

// ChainLevelHelper.cs:207-208 (explicit comment):
// "Note: the starting point, points to the non-beacon info block.
//  MergeBlockDownloader does not download the first header so this is deliberate"
// Decrements startingPoint → returns (BKN, hash_of_BKN)

Here headers[0].Number == BKN. On every cache hit: desiredStart = BKN+1, offset = 1, anchor dropped.

The test only exercises Path A (start: 1, BKN=0 → cacheStart == BKN+1). Path B (the dominant production state) is not covered. Inline comments with exact fix suggestions have been posted on lines 98 and 36.

---

Verdict

Not mergeable. The critical off-by-one (finding #1) and the concealing test mock (finding #2) are both unaddressed. The medium findings (#3, #4) should also be resolved or explicitly acknowledged before merge.

[claude]

[Critical — still present] Off-by-one drops the anchor on every cache hit in typical PoS sync

ChainLevelHelper.GetStartingPoint() has two exit paths:

1. No walk-back (block at BestKnownNumber+1 is already processed): returns (BestKnownNumber+1, hash) → headers[0].Number == BestKnownNumber+1. The +1 here is correct.
2. Walk-back (typical active beacon sync — block at BestKnownNumber+1 is IsBeaconInfo): the do/while loop decrements startingPoint once to BestKnownNumber and returns (BestKnownNumber, hash) — confirmed by ChainLevelHelper.cs:207-208: "the starting point, points to the non-beacon info block. MergeBlockDownloader does not download the first header so this is deliberate". In this case headers[0].Number == BestKnownNumber and the +1 is wrong.

On a cache hit (same BestKnownNumber, walk-back path):

cacheStart    = BestKnownNumber      (from first call)
desiredStart  = BestKnownNumber + 1  (← off by one)
offset        = 1
slice         = [block@BKN+1, …]     (anchor at BKN dropped)

BlockDownloader.AssembleRequest relies on headers[0] as the parent anchor (parentHeader), iterating headers.Skip(1) for actual downloads. Dropping the anchor means the body for BKN+1 is never moved into a satisfied entry.

Suggested change

	long desiredStart = Math.Min(_blockTree.BestKnownNumber + 1, currentNumber);
	long desiredStart = Math.Min(_blockTree.BestKnownNumber, currentNumber);

[claude]

[Critical — still present] Test mock covers the wrong code path and hides the off-by-one

BuildSequentialHeaders(start: 1, count: CachedBatchSize) with BestKnownNumber = 0 models the no-walk-back path where headers[0].Number == BestKnownNumber+1 == 1. In this shape desiredStart = BKN+1 = 1 = cacheStart, so offset = 0 and the off-by-one is invisible.

Production uses the walk-back path (BestKnownNumber+1 is IsBeaconInfo) which returns headers[0].Number == BestKnownNumber == 0. The mock should match that:

Suggested change

	_chainLevelHelper.GetNextHeaders(default, default, default).ReturnsForAnyArgs(_ => BuildSequentialHeaders(start: 1, count: CachedBatchSize));
	_chainLevelHelper.GetNextHeaders(default, default, default).ReturnsForAnyArgs(_ => BuildSequentialHeaders(start: 0, count: CachedBatchSize));

With start: 0: cacheStart = 0 = BestKnownNumber. The buggy desiredStart = BKN+1 = 1 ≠ 0 → offset = 1 → anchor dropped. The test would then fail, exposing the bug. The Cached_slice_advances_with_best_known_number assertion on line 107 (second[0]!.Number.Should().Be(21)) would also need to change to Be(20) once the code fix is applied.