Glamsterdam - Engine API Changes

[svlachakis]

Fixes Closes Resolves

• engine: EL must support reorg to head's ancestor ethereum/execution-apis#770
• Allow zero safeBlockHash and finalizedBlockHash after finalization ethereum/execution-apis#760
• engine: Restrict no-reorg to the prefix of known finalized ethereum/execution-apis#786

Should get merged after #11354

Changes

Implement Engine API FCU spec updates

Implements two Engine API spec changes: the FCU ancestor reorg depth limit and the zero-hash finalized/safe block behavior.

FCU ancestor reorg depth limit

Previously, FCU to any canonical ancestor was silently skipped. The new spec narrows this: a skip is only allowed when the ancestor is more than 32 blocks behind the current head. At ≤32 blocks, the client MUST execute the reorg.

BlockTreeExtensions: IsOnMainChainBehindHead now requires headNumber - blockNumber > 32 (was blockNumber < headNumber). The depth limit only applies to canonical ancestors — the IsMainChain guard ensures blocks on different branches always trigger a full reorg regardless of depth.

ForkchoiceUpdatedHandler: The handler calls IsOnMainChainBehindHead in two distinct places with different semantics. The unprocessed-block shortcut (line 170 — "is this block already in the canonical chain?") must not have a depth limit, so it was changed to call IsOnMainChainBehindOrEqualHead directly. The post-EnsureNewHead check (line 249 — "should we skip a reorg?") retains the virtual method with the 32-block limit.

Zero hash behavior (execution-apis PR #760)

When finalizedBlockHash or safeBlockHash is zero, the client must use the latest known value rather than overwriting the stored hash with zero. Both ForkChoiceUpdated call sites now resolve zero hashes via ResolveZeroHash before persisting to DB. Zero with no known previous value stays zero.

The -38002 condition (step 6) already correctly required a non-zero hash via IsInconsistent.

Tests

7 new tests in EngineModuleTests.V1.cs covering all spec branches: within/beyond/at the depth limit, different branch, zero hashes no-error, non-zero unknown hash error, and zero-hash preservation of known finalized state.

Test coverage — all cases:

Spec requirement	Test
Ancestor ≤32 blocks → MUST reorg	WhenHeadIsAncestorWithinDepthLimit_ReorgsToAncestor
Ancestor >32 blocks → MAY skip	WhenHeadIsAncestorBeyondDepthLimit_SkipsUpdate
Ancestor exactly 32 blocks → MUST reorg (boundary)	WhenHeadIsAncestorAtExactDepthLimit_ReorgsToAncestor
Different branch → always reorg, no depth limit	WhenHeadIsOnDifferentBranch_ReorgsRegardlessOfDepth
Zero safe/finalized → no -38002	WhenZeroFinalizedAndSafeHash_ReturnsValidWithoutError
Non-zero unknown hash → -38002	WhenNonZeroUnknownFinalizedHash_ReturnsInvalidForkchoiceState
Zero finalized → preserve last known finalized	WhenZeroFinalizedHash_PreservesKnownFinalizedHash

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

[svlachakis]

@claude review please

[claude]

Claude finished @svlachakis's task in 4m 44s —— View job

---

PR Review — Glamsterdam Engine API Changes

• Gathering context
• Reading changed files
• Reading relevant rules
• Analyzing spec compliance
• Writing review feedback

---

Summary

The implementation is correct overall — the two spec changes from execution-apis PRs #770 (ancestor reorg depth limit) and #760 (zero-hash preservation) are handled properly. Tests cover the key boundary cases. A few issues are worth addressing before merging.

---

Issues

1. Misleading method name IsOnMainChainBehindHead

BlockTreeExtensions.cs:16

The method was renamed in semantics but not in name. Previously it meant "is on main chain and anywhere behind head"; it now means "is on main chain and more than 32 blocks behind head" — i.e., skip-eligible. The old name is now actively misleading for future readers.

// current — confusing: name says "behind head" but logic is "32+ blocks behind head"
public static bool IsOnMainChainBehindHead(this IBlockTree blockTree, Block block) =>
    (blockTree.Head?.Number ?? 0) - block.Number > AncestorReorgDepthLimit && blockTree.IsMainChain(block.Header);

Suggested rename: IsAncestorOnMainChainBeyondReorgDepthLimit (or similar). The virtual method on ForkchoiceUpdatedHandler and the Taiko override should be renamed consistently.

Fix this →

---

2. Test duplication — should use [TestCase]

EngineModuleTests.V1.cs:567–630

Per the test-infrastructure rules: "Do not duplicate test methods that differ only in parameters — use [TestCase] to parameterize."

WhenHeadIsAncestorWithinDepthLimit_ReorgsToAncestor and WhenHeadIsAncestorAtExactDepthLimit_ReorgsToAncestor have identical structure and identical assertions — they both assert that a reorg happens. The only difference is chain length (3 vs 33 blocks) and the target (1 block behind vs 32 blocks behind). These two should be collapsed into one [TestCase]:

// Within limit: chain length 3, FCU to block 1 → depth = 2
// At exact limit: chain length 33, FCU to block 1 → depth = 32
[TestCase(3, Description = "2 blocks behind — within limit")]
[TestCase(33, Description = "exactly 32 blocks behind — boundary")]
public async Task forkchoiceUpdatedV1_WhenHeadIsAncestorWithinOrAtDepthLimit_ReorgsToAncestor(int chainLength) { ... }

Fix this →

---

3. Missing test: zero-hash preservation when payload attributes are present

EngineModuleTests.V1.cs:692

WhenZeroFinalizedHash_PreservesKnownFinalizedHash only tests the no-payload path (ForkChoiceUpdated from ApplyForkchoiceUpdate). Because Handle calls StartBuildingPayload next (which also calls _blockTree.ForkChoiceUpdated with ResolveZeroHash), the payload-building path should also be exercised to ensure both call sites behave consistently. A variant with PayloadAttributes non-null would complete coverage.

---

4. Minor: ForkchoiceStateV1 constructor order vs test comments

EngineModuleTests.V1.cs:703

The constructor is ForkchoiceStateV1(headBlockHash, finalizedBlockHash, safeBlockHash) (finalized before safe). Several new tests correctly use this order, but the variable names in WhenNonZeroUnknownFinalizedHash (fcuWithUnknownFinalized) and the comment on line 719 are clear. This is fine — just a note for awareness.

---

Correctness Verification

Spec requirement	Implementation	Verdict
Ancestor ≤32 blocks → MUST reorg	> AncestorReorgDepthLimit (strict) → NOT skipped at depth 32	✓
Ancestor >32 blocks → MAY skip	> 32 → skipped	✓
Non-canonical branch → always reorg	IsMainChain guard in IsOnMainChainBehindHead short-circuits	✓
Unprocessed block check → no depth limit	Uses IsOnMainChainBehindOrEqualHead (not depth-limited)	✓
Zero safe/finalized → preserve last known	ResolveZeroHash(hash, _blockTree.FinalizedHash/SafeHash) in both ForkChoiceUpdated call sites	✓
Zero with no prior → stays zero	knownHash ?? Keccak.Zero	✓
Non-zero unknown finalized → -38002	Pre-existing IsInconsistent check unmodified	✓
Taiko always reorgs (no depth limit)	TaikoForkchoiceUpdatedHandler.IsOnMainChainBehindHead always returns true	✓

The boundary arithmetic is correct: headNumber - block.Number > 32 evaluates to false at depth 32 (triggers reorg) and true at depth 33 (allows skip). Both ForkChoiceUpdated call sites (ApplyForkchoiceUpdate L289 and StartBuildingPayload L339) are updated consistently — the double write is idempotent and pre-existing.

---

Non-blocking notes

• The ResolveZeroHash helper is clean and easy to follow.
• Taiko's override design (virtual IsOnMainChainBehindHead) is a good, minimal extension point.
• The Taiko test at L128 (IsOnMainChainBehindHead_Override_PreventsAncestorReorgSkip) manually mocks IBlockTree — this is a pre-existing pattern in that file but worth noting for future cleanups.

[LukaszRozmej]

@claude review

[claude]

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

---

PR Review — Glamsterdam Engine API Changes

• Gathering context
• Reading changed files
• Reading relevant rules
• Analyzing spec compliance
• Writing review feedback

---

Summary

The implementation is correct. Both spec changes — the 32-block ancestor reorg depth limit (execution-apis #770) and zero-hash preservation (execution-apis #760) — are implemented accurately. Previous review feedback was well applied: method renamed, tests parameterized, and payload-path test added. A few things remain worth noting.

---

Correctness Verification

Spec requirement	Implementation	Verdict
Ancestor ≤32 blocks → MUST reorg	> AncestorReorgDepthLimit (strict) → not skipped at depth 32	✓
Ancestor >32 blocks → MAY skip	head - block.Number > 32 → skipped	✓
Non-canonical branch → always reorg	IsMainChain guard in IsAncestorOnMainChainBeyondReorgDepthLimit	✓
Unprocessed block → no depth limit	Line 170 uses IsOnMainChainBehindOrEqualHead directly	✓
Zero safe/finalized → preserve last known	ResolveZeroHash(hash, _blockTree.FinalizedHash/SafeHash) in both call sites	✓
Zero with no prior → stays zero	knownHash ?? Keccak.Zero	✓
Non-zero unknown finalized → -38002	Pre-existing IsInconsistent check; zero-hash path correctly bypassed	✓
Taiko always reorgs	Override always returns true	✓

---

Issues

1. Confusing return semantics on the virtual method

ForkchoiceUpdatedHandler.cs:94–106

The virtual method is named IsAncestorOnMainChainBeyondReorgDepthLimit — a reader expects true to mean "yes, it is beyond the limit." In reality the method returns false when the block IS beyond the limit (setting errorResult to signal a skip) and true when processing should continue. The [NotNullWhen(false)] annotation documents this contract, but the inverted return value is unintuitive.

The callsite amplifies this: if (!IsAncestorOnMainChainBeyondReorgDepthLimit(...)) return result; requires a double-negation to parse. Since this is a virtual method that external plugins (Taiko) override, clear semantics matter.

Consider renaming to reflect the actual boolean meaning — e.g. ShouldProceedWithReorg (returns true → proceed, false → skip with out result), or document the convention via a comment.

Fix this →

---

2. _poSSwitcher.ForkchoiceUpdated receives unresolved finalizedBlockHash

ForkchoiceUpdatedHandler.cs:284

_poSSwitcher.ForkchoiceUpdated(newHeadBlock.Header, finalizedBlockHash);

This passes the original (possibly-zero) finalizedBlockHash from forkchoiceState, not the ResolveZeroHash-resolved value. In practice this is currently benign because PoSSwitcher.ForkchoiceUpdated already guards if (finalizedHash != Keccak.Zero) — so a zero hash is silently ignored there anyway. But it's inconsistent with the zero-hash preservation semantics applied two lines later at the ForkChoiceUpdated call. Should use the resolved hash for consistency:

Hash256 resolvedFinalized = ResolveZeroHash(forkchoiceState.FinalizedBlockHash, _blockTree.FinalizedHash);
_poSSwitcher.ForkchoiceUpdated(newHeadBlock.Header, resolvedFinalized);
// ...
_blockTree.ForkChoiceUpdated(resolvedFinalized, ResolveZeroHash(forkchoiceState.SafeBlockHash, _blockTree.SafeHash));

Fix this →

---

3. Fragile genesis parent construction in test

EngineModuleTests.V1.cs — WhenHeadIsOnDifferentBranch_ReorgsRegardlessOfDepth

The test manually constructs genesisAsParent as an ExecutionPayload copying only some fields from genesis:

ExecutionPayload genesisAsParent = new ExecutionPayload
{
    BlockNumber = genesis.Number,
    BlockHash = genesis.Hash!,
    StateRoot = genesis.StateRoot!,
    // ...
};

Fields like ExtraData, MixHash, Difficulty, Nonce, LogsBloom are omitted, which means genesisAsParent may not pass strict payload validation in the future if the test environment's genesis requires them. A less fragile approach would be to retrieve the genesis ExecutionPayload using a newPayloadV1 round-trip or ExecutionPayload.FromBlock(genesis) (if available), rather than manually assembling it.

---

Non-blocking notes

• ResolveZeroHash is clean and self-documenting. Good helper.
• The AncestorReorgDepthLimit = 32 constant being public on BlockTreeExtensions is useful for tests and plugins.
• The Taiko test uses direct mocks rather than TestBlockchain — this is consistent with the pre-existing pattern in TaikoEngineApiTests.cs and acceptable for testing a specific override behavior in isolation.
• EnsureNewHead is called before the depth-limit check (line 243 vs 250). For within-limit ancestors this builds the branch and proceeds to UpdateMainChain correctly. For beyond-limit ancestors it builds the branch but then returns early — the branch-building is harmless (no state mutation). The ordering is correct.

[svlachakis]

@claude review again please

[claude]

Claude finished @svlachakis's task in 4m 37s —— View job

---

PR Review — Glamsterdam Engine API Changes

• Gathering context
• Reading changed files
• Reading relevant rules
• Analyzing spec compliance
• Writing review feedback

---

Summary

All issues raised in the two previous review rounds have been addressed. The implementation is correct and clean. One minor inconsistency remains that is worth fixing before merging.

---

Addressed From Previous Reviews

Previous Issue	Status
Misleading IsOnMainChainBehindHead name	✅ Renamed to ShouldProceedWithReorg (clear semantics, [NotNullWhen(false)])
Inverted return semantics on virtual method	✅ Now named ShouldProceedWithReorg — true = proceed, false = skip with out result
_poSSwitcher.ForkchoiceUpdated received unresolved hash	✅ Now calls ResolveZeroHash(finalizedBlockHash, _blockTree.FinalizedHash)
Fragile genesis parent in WhenHeadIsOnDifferentBranch test	✅ Now uses CreateParentBlockRequestOnHead
Test duplication	✅ Merged into [TestCase(3, ...)] / [TestCase(33, ...)]
Missing payload-attributes test for zero-hash path	✅ Added WhenZeroFinalizedHash_PreservesKnownFinalizedHash_WithPayloadAttributes

---

Remaining Issue

Source inconsistency in _blockTree.ForkChoiceUpdated within ApplyForkchoiceUpdate

ForkchoiceUpdatedHandler.cs:284,289

_poSSwitcher.ForkchoiceUpdated (line 284) correctly uses the local variable finalizedBlockHash:

_poSSwitcher.ForkchoiceUpdated(newHeadBlock.Header, ResolveZeroHash(finalizedBlockHash, _blockTree.FinalizedHash));

But _blockTree.ForkChoiceUpdated (line 289) uses the original forkchoiceState.FinalizedBlockHash:

_blockTree.ForkChoiceUpdated(
    ResolveZeroHash(forkchoiceState.FinalizedBlockHash, _blockTree.FinalizedHash),
    ResolveZeroHash(forkchoiceState.SafeBlockHash, _blockTree.SafeHash));

The local variables finalizedBlockHash and safeBlockHash are passed by ref to ValidateBlockHash. In the base class, ValidateBlockHash never modifies the ref, so both sources are identical. But TaikoForkchoiceUpdatedHandler.ValidateBlockHash does modify the ref — it zeroes it out for unknown non-zero hashes (blockHash = Keccak.Zero). This means in Taiko:

• An unknown non-zero finalizedBlockHash becomes Keccak.Zero after ValidateBlockHash
• _poSSwitcher.ForkchoiceUpdated sees ResolveZeroHash(Keccak.Zero, ...) → resolves to last-known finalized (good)
• _blockTree.ForkChoiceUpdated sees ResolveZeroHash(originalNonZeroUnknown, ...) → returns the unknown non-zero hash as-is (potentially stores an invalid hash)

The two call sites should draw from the same resolved source. The fix is straightforward:

// In ApplyForkchoiceUpdate, compute once:
Hash256 resolvedFinalizedHash = ResolveZeroHash(finalizedBlockHash, _blockTree.FinalizedHash);
Hash256 resolvedSafeHash = ResolveZeroHash(safeBlockHash, _blockTree.SafeHash);

if (shouldUpdateHead)
{
    _poSSwitcher.ForkchoiceUpdated(newHeadBlock.Header, resolvedFinalizedHash);
    if (_logger.IsInfo) ...
}

_blockTree.ForkChoiceUpdated(resolvedFinalizedHash, resolvedSafeHash);

Note: StartBuildingPayload must still use forkchoiceState.* directly (the local vars are scoped to ApplyForkchoiceUpdate), but since zero-hash is the primary case and it's handled by ResolveZeroHash, the behavior there is correct.

Fix this →

---

Correctness Verification

Spec requirement	Implementation	Verdict
Ancestor ≤32 blocks → MUST reorg	> AncestorReorgDepthLimit (strict) — depth 32 is not skipped	✓
Ancestor >32 blocks → MAY skip	> 32 → ShouldProceedWithReorg returns false	✓
Non-canonical branch → always reorg	IsMainChain guard in IsAncestorOnMainChainBeyondReorgDepthLimit	✓
Unprocessed block → no depth limit	Line 170 uses IsOnMainChainBehindOrEqualHead directly	✓
Zero safe/finalized → preserve last known	ResolveZeroHash(hash, _blockTree.FinalizedHash/SafeHash) in both ForkChoiceUpdated call sites	✓
Zero with no prior → stays zero	knownHash ?? Keccak.Zero	✓
Non-zero unknown finalized → -38002	IsInconsistent check; zero-hash path correctly bypassed	✓
Taiko always reorgs	Override always returns true	✓

---

Non-Blocking Notes

• EnsureNewHead is called before ShouldProceedWithReorg at line 243 vs 250. For skip-eligible ancestors (>32 blocks), TryGetBranch traverses the tree but the result is discarded. No correctness issue — pure read, no state mutation — just a minor wasted traversal.
• AncestorReorgDepthLimit = 32 as a public constant on BlockTreeExtensions is the right call for testability and Taiko reuse.
• The ShouldProceedWithReorg naming and [NotNullWhen(false)] annotation make the virtual contract clear for future overriders.

[svlachakis]

@claude review again

[claude]

Claude finished @svlachakis's task in 7m 10s —— View job

---

PR Review — Glamsterdam Engine API Changes (latest round, post spec-update #786)

• Gathering context
• Reading changed files
• Reading relevant rules
• Analyzing spec compliance (execution-apis aggresive dispose on timers #765 #770, Neth-Neth sync fails on receipt roots #760, Make Packets to operate with zero-allocating network stack #786)
• Verifying test coverage and test names
• Writing review feedback

---

Summary

All previous issues are resolved. The spec compliance for execution-apis #760, #770, and #786 is correct. However, this round introduced a self-inconsistency between two changes made in the same PR: BaseEngineModuleTests.cs was changed to stop setting finalized in ProduceBranchV1(setHead: true), but a new test added in this same PR (WhenHeadIsAncestorOfFinalizedBlock_SkipsUpdate) still assumes the helper sets finalized. This will cause that test to fail.

---

Critical Issue

WhenHeadIsAncestorOfFinalizedBlock_SkipsUpdate is broken by BaseEngineModuleTests.cs

EngineModuleTests.V1.cs:603 — the test calls ProduceBranchV1(34, setHead: true) and immediately asserts:

chain.BlockTree.FinalizedHash.Should().Be(b34Hash, "precondition: b34 is finalized after setHead:true branch");

But BaseEngineModuleTests.cs was changed (in this same PR) to use Keccak.Zero for finalized/safe in ProduceBranchV1:

// BaseEngineModuleTests.cs (this PR's change)
ForkchoiceStateV1 forkchoiceStateV1 = new(newHead, Keccak.Zero, Keccak.Zero); // was: new(newHead, newHead, newHead)

After this change, chain.BlockTree.FinalizedHash is null/zero after the branch, not b34Hash. The precondition assertion will throw, and even if skipped, ShouldProceedWithReorg would permit the reorg (depth 33 < PruningBoundary 64), causing the final HeadHash.Should().Be(b34Hash) to also fail.

Fix: Add an explicit FCU that sets finalized before the skip test, and update the stale comment at line 597:

IReadOnlyList<ExecutionPayload> branch = await ProduceBranchV1(rpc, chain, 34, ..., setHead: true);
// ProduceBranchV1 no longer sets finalized — do it explicitly:
await rpc.engine_forkchoiceUpdatedV1(new ForkchoiceStateV1(b34Hash, b34Hash, b34Hash));
chain.BlockTree.FinalizedHash.Should().Be(b34Hash, "precondition: b34 is finalized");

Fix this →

---

Spec Compliance Verification

Spec requirement (PR #786)	Implementation	Verdict
MAY skip when head is valid ancestor of latest known finalized	newHeadHeader.Number <= knownFinalizedHeader.Number && IsMainChain(newHeadHeader)	✓
MUST return -38006 when reorg depth > client limit	reorgDepth > _maxReorgDepth → TooDeepReorg	✓
Client limit tied to pruning state	_maxReorgDepth = pruningConfig.PruningBoundary (default 64)	✓
-38006 code added to MergeErrorCodes	TooDeepReorg = -38006	✓
Finalized header not found → safe fallback	Logs warning, falls through to depth check	✓
Zero safe/finalized → preserve last known (PR #760)	ResolveZeroHash in both ApplyForkchoiceUpdate call sites	✓
Non-zero unknown finalized → -38002	Pre-existing ValidateBlockHash / finalizationErrorMsg path	✓
Taiko always reorgs	ShouldProceedWithReorg override always returns true	✓

Test Coverage Verification

Spec requirement	Test	Status
Ancestor within PruningBoundary → MUST reorg	WhenHeadIsCanonicalAncestorWithinPruningBoundary_ReorgsToAncestor [TestCase(3), TestCase(33)]	✓
Ancestor of finalized → MAY skip	WhenHeadIsAncestorOfFinalizedBlock_SkipsUpdate	⚠️ broken (see above)
Reorg depth > PruningBoundary → -38006	WhenReorgDepthExceedsPruningBoundary_ReturnsTooDeepReorg	✓
Different branch → always reorg	WhenHeadIsOnDifferentBranch_ReorgsRegardlessOfDepth	✓
Zero safe/finalized → no -38002	WhenZeroFinalizedAndSafeHash_ReturnsValidWithoutError	✓
Non-zero unknown finalized → -38002	WhenNonZeroUnknownFinalizedHash_ReturnsInvalidForkchoiceState	✓
Zero finalized preserves last known	WhenZeroFinalizedHash_PreservesKnownFinalizedHash + _WithPayloadAttributes	✓
Taiko bypasses -38006	ShouldProceedWithReorg_Override_BypassesTooDeepReorgError	✓

FindMainChainAncestorNumber Correctness

• Main-chain block: returns block.Number directly → depth = head.Number - block.Number (exact)
• Side-chain block: walks parents to find common ancestor → conservative depth (correct for state availability)
• Null parent (pruned ancestry): returns 0 (genesis) → maximizes reported depth (safe fallback)

Non-blocking Notes

• BaseEngineModuleTests.cs change is a correct design decision in isolation (helpers should not silently imply finality semantics). The bug is that the new test wasn't updated to match.
• _maxReorgDepth comment referencing execution-apis/pull/786 is good.
• [NotNullWhen(false)] on ShouldProceedWithReorg correctly documents the out-parameter contract.
• <= in newHeadHeader.Number <= knownFinalizedHeader.Number correctly includes FCU to the finalized block itself (equivalent to an ancestor).

---

[claude]

Critical — broken test: precondition and main assertion will both fail

ProduceBranchV1(setHead: true) was changed in this same PR (BaseEngineModuleTests.cs) to use Keccak.Zero for finalized/safe:

// BaseEngineModuleTests.cs — after this PR's change:
ForkchoiceStateV1 forkchoiceStateV1 = new(newHead, Keccak.Zero, Keccak.Zero);

So after ProduceBranchV1(34, setHead: true), chain.BlockTree.FinalizedHash is null/Keccak.Zero, not b34Hash. This assertion will throw:

chain.BlockTree.FinalizedHash.Should().Be(b34Hash, "precondition: b34 is finalized after setHead:true branch");

And even if the precondition were skipped, ShouldProceedWithReorg with zero finalized would fall through to the depth check: reorgDepth = 34 - 1 = 33 < PruningBoundary(64) → reorg proceeds → head changes to b1 → the assertion at line 616 (HeadHash.Should().Be(b34Hash)) also fails.

The fix is to explicitly finalize b34 before testing the MAY-skip rule:

// After ProduceBranchV1, explicitly set finalized to b34:
await rpc.engine_forkchoiceUpdatedV1(new ForkchoiceStateV1(b34Hash, b34Hash, b34Hash));
chain.BlockTree.FinalizedHash.Should().Be(b34Hash, "precondition: b34 is finalized");

Also update the stale comment at line 597–600 which says "ProduceBranchV1 with setHead:true finalizes each block via FCU" — this is no longer true after the BaseEngineModuleTests.cs change.

[claude]

High — silent breaking change to all setHead: true callers

Changing the FCU from new(newHead, newHead, newHead) to new(newHead, Keccak.Zero, Keccak.Zero) is a reasonable design decision (helpers shouldn't imply semantics they don't own), but it is a breaking change for any caller that relied on finalized being set.

The only explicit regression is the new WhenHeadIsAncestorOfFinalizedBlock_SkipsUpdate test in this same PR, which has an inconsistency: it calls ProduceBranchV1(setHead: true) and then asserts chain.BlockTree.FinalizedHash.Should().Be(b34Hash). With this change that assertion will fail.

Before landing, verify there are no other pre-existing callers in EngineModuleTests.Synchronization.cs (which has 4+ setHead: true usages) that rely on the finalized hash being advanced. A quick grep for FinalizedHash assertions near ProduceBranchV1(..., true) would confirm.

[svlachakis]

@claude review again

[claude]

Claude finished @svlachakis's task in 7m 15s —— View job

---

PR Review — Glamsterdam Engine API Changes (post spec-update #786, latest round)

• Gathering context
• Reading changed files
• Reading relevant rules
• Analyzing spec compliance (execution-apis aggresive dispose on timers #765 #770, Neth-Neth sync fails on receipt roots #760, Make Packets to operate with zero-allocating network stack #786)
• Verifying test coverage and correctness
• Writing review feedback

---

Summary

All issues from the five previous review rounds have been resolved. The implementation correctly handles all three spec changes, DI wiring is sound, and tests are accurate and well-named.

---

Resolved From Previous Reviews

Previous Issue	Status
Broken WhenHeadIsAncestorOfFinalizedBlock_SkipsUpdate (used zero finalized after BaseEngineModuleTests.cs change)	✅ Explicit FCU with (b34Hash, b34Hash, b34Hash) now sets finalized before the MAY-skip assertion
Tests referencing old PR #770 "32-block limit" semantics	✅ Renamed to WhenHeadIsCanonicalAncestorWithinPruningBoundary_ReorgsToAncestor, WhenReorgDepthExceedsPruningBoundary_ReturnsTooDeepReorg, etc. with updated comments
Missing -38006 integration test	✅ WhenReorgDepthExceedsPruningBoundary_ReturnsTooDeepReorg: 66 blocks, FCU to H=1 → depth 65 > 64
Missing explicit MAY-skip test	✅ WhenHeadIsAncestorOfFinalizedBlock_SkipsUpdate now explicitly tests this path
< vs <= in finalized ancestor check	✅ newHeadHeader.Number <= knownFinalizedHeader.Number — FCU to the finalized block itself is also eligible for skip
_maxReorgDepth undocumented	✅ Comment with execution-apis/pull/786 reference added

---

Spec Compliance Verification

Spec requirement	Implementation	Verdict
MAY skip when head is valid ancestor of latest known finalized	newHeadHeader.Number <= knownFinalizedHeader.Number && IsMainChain(newHeadHeader)	✓
MUST return -38006 when reorg depth > client limit	reorgDepth = Head.Number - FindMainChainAncestorNumber(newHead) > _maxReorgDepth	✓
Client limit tied to pruning state	_maxReorgDepth = pruningConfig.PruningBoundary (default 64 via ConfigRegistrationSource → PruningConfig)	✓
-38006 error code defined	MergeErrorCodes.TooDeepReorg = -38006	✓
Finalized header not found → safe fallback	Logs warn, falls through to depth check	✓
Zero safe/finalized → preserve last known (PR #760)	resolvedFinalizedHash/resolvedSafeHash from local vars in both ApplyForkchoiceUpdate call sites	✓
Zero with no prior → stays zero	knownHash ?? Keccak.Zero	✓
Non-zero unknown finalized → -38002	Pre-existing ValidateBlockHash / finalizationErrorMsg path	✓
Taiko always reorgs	ShouldProceedWithReorg override always returns true; pruningConfig passed through to base ctor	✓

Test Coverage Verification

Spec requirement	Test	Correct?
Canonical ancestor within PruningBoundary → MUST reorg	[TestCase(3)]/[TestCase(33)] on WhenHeadIsCanonicalAncestorWithinPruningBoundary_ReorgsToAncestor	✓ (depth 2/32 < 64)
Ancestor of finalized → MAY skip	WhenHeadIsAncestorOfFinalizedBlock_SkipsUpdate	✓ (explicit finalize, MAY-skip fires)
Reorg depth > PruningBoundary → -38006	WhenReorgDepthExceedsPruningBoundary_ReturnsTooDeepReorg	✓ (depth 65 > 64)
Different branch → always reorg	WhenHeadIsOnDifferentBranch_ReorgsRegardlessOfDepth	✓ (side block, depth 34 < 64)
Zero safe/finalized → no -38002	WhenZeroFinalizedAndSafeHash_ReturnsValidWithoutError	✓
Zero finalized preserves last known	WhenZeroFinalizedHash_PreservesKnownFinalizedHash + _WithPayloadAttributes	✓
Non-zero unknown → -38002	WhenNonZeroUnknownFinalizedHash_ReturnsInvalidForkchoiceState	✓
Taiko bypasses -38006	ShouldProceedWithReorg_Override_BypassesTooDeepReorgError	✓

DI Wiring Verification

IPruningConfig is auto-resolved via ConfigRegistrationSource (registered in NethermindModule, which PseudoNethermindModule inherits). It calls configProvider.GetConfig(typeof(IPruningConfig)), which instantiates PruningConfig with PruningBoundary = (int)Reorganization.MaxDepth = 64. The MergeTestBlockchain DI chain correctly injects this into ForkchoiceUpdatedHandler. ✓

blocks is not null Guard Correctness

The guard at line 268 (if (blocks is not null && !ShouldProceedWithReorg(...))) correctly skips the reorg depth check for same-head FCUs. When EnsureNewHead returns null (head unchanged), there is no reorg, so neither the MAY-skip nor -38006 should fire. WhenZeroFinalizedAndSafeHash_ReturnsValidWithoutError exercises this path correctly. ✓

Non-blocking Notes

• StartBuildingPayload still uses forkchoiceState.* originals: In Taiko + payload-attributes + unknown-non-zero-finalized edge case, the second ForkChoiceUpdated call overwrites the resolved hash. This is acknowledged as pre-existing, not introduced by this PR.
• XML doc comments on IsOnMainChainBehindOrEqualHead, IsAncestorOnMainChainBeyondReorgDepthLimit in BlockTreeExtensions.cs are still missing (Low, pre-existing from earlier reviews). Fine to address in a follow-up.
• WhenReorgDepthExceedsPruningBoundary_ReturnsTooDeepReorg relies on the default PruningBoundary = 64 implicitly. Acceptable for an integration test using the real DI stack.

---

The implementation is correct, tests are accurate, and the conflict resolution is clean. This PR is ready to merge.

[svlachakis]

@claude review again, very deeply based on the spec.

[claude]

Claude finished @svlachakis's task in 9m 44s —— View job

---

PR Review — Glamsterdam Engine API Changes (Deep Spec Review)

• Gathering context and loading spec PRs
• Reading all changed files in depth
• Reading agent rules (coding-style, robustness, test-infrastructure)
• Deep spec compliance verification (execution-apis Neth-Neth sync fails on receipt roots #760, aggresive dispose on timers #765 #770, Make Packets to operate with zero-allocating network stack #786)
• Verifying test correctness and boundary arithmetic
• Writing review feedback

---

Summary

All issues from prior review rounds have been resolved. After a deep spec-level re-read against execution-apis PRs #760, #770, and #786, the implementation is correct and spec-compliant. A few new low-severity issues are noted below; none block merging.

---

Spec Compliance — Full Verification

Execution-apis PR #786 — FCU ancestor reorg depth limit

Requirement	Code location	Verdict
MAY skip when headBlockHash is a valid ancestor of the latest known finalized block	ShouldProceedWithReorg L86: newHeadHeader.Number <= knownFinalizedHeader.Number && IsMainChain(newHeadHeader)	✓
MUST return -38006 when reorg depth > client limit	ShouldProceedWithReorg L94-99: (_blockTree.Head?.Number ?? 0) - FindMainChainAncestorNumber(newHead) > _maxReorgDepth	✓
Client limit tied to pruning state	_maxReorgDepth = pruningConfig.PruningBoundary (default 64 via PruningConfig; floor enforced at 64 by PruningTrieStateFactory.AdviseConfig)	✓
Non-canonical block → never eligible for MAY-skip	IsMainChain guard in the MAY-skip condition short-circuits for fork blocks	✓
Unprocessed-block fast path → no depth limit	L188: uses IsOnMainChainBehindOrEqualHead directly, no virtual dispatch	✓
Finalized header pruned → safe fallback	Warn log + fall-through to depth check (conservative)	✓
-38006 added to MergeErrorCodes	TooDeepReorg = -38006	✓

Execution-apis PR #760 — Zero-hash finalized/safe preservation

Requirement	Code location	Verdict
Zero finalized/safe → use last known, not zero	resolvedFinalizedHash/resolvedSafeHash in ApplyForkchoiceUpdate L294-295	✓
Both ForkChoiceUpdated call sites covered	ApplyForkchoiceUpdate L303 and StartBuildingPayload L351-353	✓
Zero with no prior → stays zero	ResolveZeroHash: knownHash ?? Keccak.Zero	✓
Non-zero unknown finalized → -38002 (not affected by zero-hash fix)	finalizationErrorMsg check at L240-243; zero-hash path bypasses ValidateBlockHash via skipZeroHash = true	✓

Taiko-specific

Requirement	Code location	Verdict
Taiko always reorgs — override bypasses MAY-skip and -38006	TaikoForkchoiceUpdatedHandler.ShouldProceedWithReorg: always returns true	✓
Taiko relaxed bounds for finalized/safe	TaikoForkchoiceUpdatedHandler.RejectIfInconsistent: passes lowerBound=0 to base	✓
Taiko: unknown non-zero hash → silently zeroed via ValidateBlockHash	Taiko override zeros blockHash on unknown; resolvedFinalizedHash in ApplyForkchoiceUpdate then resolves via last known	✓

---

Boundary Arithmetic Verification

[TestCase(33)] — "32 blocks behind head" boundary:
ProduceBranchV1(33) produces blocks H=1…H=33. FCU first sets head to H=33, then FCU to H=1 (b1). Depth = 33−1 = 32. 32 > 64 = false → no -38006, reorg proceeds. ✓

WhenReorgDepthExceedsPruningBoundary — 66 blocks:
FCU sets head to H=66, then FCU to H=1. FindMainChainAncestorNumber(H=1) = 1 (it's on main chain). Depth = 66−1 = 65. 65 > 64 → -38006. ✓

WhenHeadIsAncestorOfFinalizedBlock_SkipsUpdate:
After explicit FCU (b34, b34, b34), FinalizedHash = b34 (H=34). FCU to b1 (H=1): 1 <= 34 && IsMainChain(b1) → MAY-skip fires. Returns Valid(null, b1Hash), head stays at H=34. ✓

WhenHeadIsOnDifferentBranch:
Side block off genesis has common ancestor = genesis (H=0). FindMainChainAncestorNumber(sideBlock): IsMainChain(sideBlock) = false → walk to parent (genesis, H=0) → IsMainChain(genesis) = true → returns 0. Depth = 34 − 0 = 34 < 64 → no -38006, reorg proceeds. ✓

---

New Findings

Low — _maxReorgDepth captured at construction, not read on-call

ForkchoiceUpdatedHandler.cs:62

private readonly int _maxReorgDepth = pruningConfig.PruningBoundary;

PruningTrieStateFactory.AdviseConfig (via MainPruningTrieStoreFactory.ctor) mutates pruningConfig.PruningBoundary — bumping it to SnapServingMaxDepth (128) when snap serving is enabled, enforcing a floor of 64. This mutation happens during node startup, before the first Engine API call. Because ForkchoiceUpdatedHandler is a DI singleton resolved lazily (first FCU call), it always captures the post-AdviseConfig value in practice.

However, this relies on initialization ordering: if any code path resolves ForkchoiceUpdatedHandler before MainPruningTrieStoreFactory, _maxReorgDepth would be stale. The simpler fix is to keep a reference to the full config and read the boundary on each call:

private readonly IPruningConfig _pruningConfig = pruningConfig;
// In ShouldProceedWithReorg:
if (reorgDepth > _pruningConfig.PruningBoundary) { ... }

Not a current bug, but fragile.

Fix this →

---

Low — Missing XML doc on public IsOnMainChainBehindOrEqualHead

BlockTreeExtensions.cs:11

Per coding-style.md: "Use documentation comments with proper structure (<summary>, <param>, <returns>) for all public APIs." IsOnMainChainBehindOrEqualHead is public with no <summary>.

(Previously flagged in rounds 4-5 as "Low, can be addressed in follow-up".)

---

Low — MergeErrorCodes.TooDeepReorg missing spec URL

MergeErrorCodes.cs:38

Per coding-style.md: "Non-obvious consensus rules or algorithms must reference the EIP number or Yellow Paper section." The XML doc describes the error but doesn't link to execution-apis/pull/786.

/// <summary>
/// Reorg depth exceeds the client-specific limitation.
/// See: https://github.com/ethereum/execution-apis/pull/786
/// </summary>
public const int TooDeepReorg = -38006;

(Previously flagged in rounds 4-5 as "Low, can be addressed in follow-up".)

---

Info — EnsureNewHead wasted traversal for skip-eligible FCUs

For canonical ancestors that qualify for the MAY-skip (spec point 2), TryGetBranch in EnsureNewHead traverses the branch from newHeadHeader to the main chain before ShouldProceedWithReorg fires and skips. This work is discarded. No correctness impact.

The MAY-skip condition (FinalizedHash lookup + IsMainChain) does not require the branch — it could in theory be checked before EnsureNewHead. Not worth refactoring now, but noting for future hot-path optimization.

---

Acknowledged Pre-existing Issues (Not Regressions)

• Taiko + payload-attributes + unknown-non-zero finalized: After ApplyForkchoiceUpdate correctly stores resolvedFinalizedHash, StartBuildingPayload reads forkchoiceState.FinalizedBlockHash (original non-zero unknown) and ResolveZeroHash passes it through, overwriting the resolved value. Final stored state is identical to pre-PR behavior (unknown hash stored). Not a regression; root cause is Taiko's silent-zero-for-unknown contract in ValidateBlockHash vs StartBuildingPayload using the original forkchoiceState.

• UpdateMainChain before RejectIfInconsistent: Pre-existing ordering (head updated before finalized/safe validated). Not introduced by this PR.

---

Correctness Verification — DI Wiring

_maxReorgDepth captures pruningConfig.PruningBoundary at ForkchoiceUpdatedHandler construction. MainPruningTrieStoreFactory.ctor (registered as AddSingleton, resolved via IReadOnlyTrieStore during startup) calls AdviseConfig which mutates PruningBoundary. Since IReadOnlyTrieStore is resolved during node initialization — well before any Engine API call triggers ForkchoiceUpdatedHandler's lazy construction — the captured value is always the post-AdviseConfig value. ✓

The test WhenReorgDepthExceedsPruningBoundary_ReturnsTooDeepReorg disables snap serving (ISyncConfig.SnapServingEnabled = false) to keep PruningBoundary at 64 (preventing AdviseConfig from bumping it to 128). With depth 65 > 64, -38006 fires. ✓

---

Summary Table

Spec requirement	Status
MAY skip → ancestor of finalized	✓ Correct
MUST -38006 → depth > pruning boundary	✓ Correct
Boundary = PruningConfig.PruningBoundary (default 64)	✓ Correct
Zero safe/finalized → preserve last known (PR #760)	✓ Both call sites
Non-zero unknown → -38002	✓ Unchanged
Taiko bypass	✓ Override always returns true
All 8 tests correct and well-named	✓

The implementation is correct. All previous Critical/High/Medium findings have been resolved. Remaining issues are Low severity, previously acknowledged.

[svlachakis]

@claude review again, not all these comments are needed

[claude]

Claude finished @svlachakis's task in 3m 15s —— View job

---

PR Review — Glamsterdam Engine API Changes

• Gathering context
• Reading current state of changed files
• Analyzing spec compliance and open issues
• Writing focused review

---

Summary

All Critical, High, and Medium issues from the previous review rounds have been resolved. The implementation is correct and spec-compliant. Only one Low-severity item remains outstanding.

---

Spec Compliance

Requirement	Implementation	Verdict
MAY skip when head is canonical ancestor of latest known finalized block	ShouldProceedWithReorg L86: newHeadHeader.Number <= knownFinalizedHeader.Number && IsMainChain(newHeadHeader)	✓
MUST return -38006 when reorg depth > client limit	reorgDepth > _pruningConfig.PruningBoundary	✓
Client limit tied to pruning state	_pruningConfig.PruningBoundary read on each call (not captured at construction)	✓
Zero safe/finalized → preserve last known	resolvedFinalizedHash/resolvedSafeHash from ResolveZeroHash in both ApplyForkchoiceUpdate call sites	✓
Zero with no prior → stays zero	knownHash ?? Keccak.Zero	✓
Non-zero unknown finalized → -38002	Pre-existing ValidateBlockHash/finalizationErrorMsg path	✓
Taiko always reorgs, bypasses -38006	ShouldProceedWithReorg override always returns true	✓

Test Coverage

Test	Spec requirement covered
WhenHeadIsCanonicalAncestorWithinPruningBoundary_ReorgsToAncestor [TestCase(3)]/[TestCase(33)]	Reorg at depth 2 and 32, both < PruningBoundary
WhenHeadIsAncestorOfFinalizedBlock_SkipsUpdate	PR #786 point 2 MAY-skip: b1 is canonical ancestor of finalized b34
WhenReorgDepthExceedsPruningBoundary_ReturnsTooDeepReorg	PR #786 point 6: depth 65 > 64 → -38006
WhenHeadIsOnDifferentBranch_ReorgsRegardlessOfDepth	Non-canonical block always reorgs
WhenZeroFinalizedAndSafeHash_ReturnsValidWithoutError	No -38002 for zero hashes
WhenZeroFinalizedHash_PreservesKnownFinalizedHash + _WithPayloadAttributes	Both ForkChoiceUpdated call sites preserve last known
WhenNonZeroUnknownFinalizedHash_ReturnsInvalidForkchoiceState	-38002 for non-zero unknown
ShouldProceedWithReorg_Override_BypassesTooDeepReorgError (Taiko)	Override bypasses -38006

---

Remaining Issue

Low — MergeErrorCodes.TooDeepReorg missing spec reference

coding-style.md requires a spec reference for non-obvious consensus rules. This has been flagged in previous rounds and is the only remaining item.

    /// <summary>
    /// Reorg depth exceeds the client-specific limitation.
    /// See: https://github.com/ethereum/execution-apis/pull/786
    /// </summary>
    public const int TooDeepReorg = -38006;

This is the only outstanding comment. Everything else is resolved. The PR is ready to merge.

[Marchhill]

unclear comment, probably can be removed

[Marchhill]

remove "spec point"

[Marchhill]

use FindFinalizedHeader?

[Marchhill]

return false straight away if head is null

[Marchhill]

add comment to explain why this is needed

[Marchhill]

add comment to explain why this is needed

[Marchhill]

remove "spec point 2"

[Marchhill]

maybe just have a function to calculate reorg depth directly

[Marchhill]

comment seems incorrect

[Marchhill]

closing in favour of #11388