fix(merge): allow lower finalized FCUs and add Paris engine pyspec coverage

[benaadams]

Changes

This fixes a regression introduced after #11187 in engine_forkchoiceUpdated.

The ancestry validation from #11185 remains in place, but the handler no longer rejects a valid FCU just because the new finalizedBlockHash is lower than the previously accepted finalized level. We still enforce request-local ordering and ancestry:

• finalized must be an ancestor of head
• safe must be an ancestor of head
• safe >= finalized
• neither safe nor finalized can be above head

This matches the Hive failures seen in:

• In-Order Consecutive Payload Execution (Paris, Cancun)
• Valid NewPayload->ForkchoiceUpdated on Syncing Client (Paris, Cancun)

Regression coverage added:

• V1 test for lower finalized on repeated FCU
• V1 test for lower finalized while building payloads
• V3/Cancun test for lower finalized while building payloads

This PR also adds ParisEngineBlockchainTests to the Pyspec engine test registry so Paris engine fixtures run in CI as
well.

Types of changes

What types of changes does your code introduce?

• Bugfix (a non-breaking change that fixes an issue)
• Other: Moar tests

Testing

Requires testing

• Yes

If yes, did you write tests?

• Yes

[claude]

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

---

PR Review: Fix Fcu hive, add Paris CI tests

• Gather context and read changed files
• Read relevant rules files
• Analyze ForkchoiceUpdatedHandler.cs changes
• Analyze test changes
• Post review findings

---

Summary

The core change — removing the cross-request finalized monotonicity check — is correct per the Engine API spec. The Paris spec only requires finalizedBlockHash to be an ancestor of headBlockHash within the same request; there is no cross-call ordering requirement. The old prevFinalizedLevel guard was a Nethermind-specific invariant that caused valid CL FCU calls to be incorrectly rejected in Hive tests. The comment in ForkchoiceUpdatedHandler.cs explains the rationale clearly.

Adding ParisEngineBlockchainTests is appropriate — Paris is the first post-merge fork and was missing from the engine API pyspec test fixtures.

---

Findings

Medium

• Tests don't cover the actual cross-call regression (see inline comment on EngineModuleTests.V1.cs:1531). Tests named *_lower_finalized_than_previous* don't establish a higher finalized first, so they would have passed even before this PR. The real regression to guard against is: send FCU with finalized=block[2] → accepted, then send FCU with finalized=block[1] → must also be accepted. Without this two-call sequence, the tests don't catch a reintroduction of the monotonicity check.

Low

• ManualBlockFinalizationManager hash/level inconsistency (see inline comment on ForkchoiceUpdatedHandler.cs:259). LastFinalizedHash gets updated to the lower block while LastFinalizedBlockLevel stays at the historical max via Math.Max. This was a latent bug protected by the old guard; now that lower finalized blocks can reach MarkFinalized, it's exposed. Not blocking, but worth a follow-up issue.

• Test naming gap: ForkChoiceUpdatedV3_should_allow_lower_finalized_than_previous_when_building_payload implies a prior higher finalized exists, but none is set. Minor, but the test only proves a non-maximal finalized is accepted at call-time, not the cross-call invariant.

---

Verdict

Behaviorally correct, spec-compliant fix. The missing two-step test coverage is the main gap before this can be considered fully regression-tested.

[benaadams]

@claude re-review

[claude]

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

---

Re-Review: Fix Fcu hive, add Paris CI tests

• Read changed files
• Read relevant rules
• Analyze ForkchoiceUpdatedHandler changes
• Analyze test coverage
• Analyze ManualFinalizationManager changes
• Post findings

---

Both findings from the previous review have been addressed.

Previous findings — resolved

Medium (resolved): Tests now cover the actual cross-call regression.

The renamed test forkchoiceUpdated_accepts_lower_finalized_than_previous_but_rejects_safe_before_finalized (V1.cs:1514) now correctly does the two-step sequence:

1. FCU with finalized=blocks[2] → Valid, verifies LastFinalizedHash/LastFinalizedBlockLevel == block[2]
2. FCU with finalized=blocks[1] (lower) → Valid, verifies state updated to block[1]

This directly regresses the monotonicity guard that was removed. The V1 + V3 payload-building variants round it out.

Low (resolved): ManualFinalizationManager hash/level inconsistency fixed.

ManualFinalizationManager.MarkFinalized now assigns LastFinalizedBlockLevel = finalizedBlock.Number directly (removing Math.Max), keeping LastFinalizedHash and LastFinalizedBlockLevel in sync when a lower finalized block is accepted. The IBlockFinalizationManager doc was updated to match. The only subscriber of BlocksFinalized that uses the numeric level outside AuRa is ProcessedTransactionsDbCleaner, which already guards against regression with its own monotonic _lastFinalizedBlock check (ProcessedTransactionsDbCleaner.cs:33).

---

Verdict

No remaining issues. Core logic is correct and spec-compliant. Test coverage now genuinely exercises the regression. Safe to merge.

[claude]

Claude finished @benaadams's task in 2m 39s —— View job

---

PR Review: fix(merge): allow lower finalized FCUs and add Paris engine pyspec coverage

• Gather context and read changed files
• Read relevant rules files
• Analyze ForkchoiceUpdatedHandler.cs changes
• Analyze ManualFinalizationManager.cs fix
• Analyze test changes
• Post review findings

---

Summary

This is a clean, spec-compliant fix. All issues from the previous review cycle have been resolved.

Core change (ForkchoiceUpdatedHandler.cs): Removing prevFinalizedLevel as the lower bound for RejectIfInconsistent is correct. The Engine API spec (Paris, Shanghai) does not require cross-call finalized monotonicity. The old Math.Max-based guard in ManualBlockFinalizationManager was a Nethermind-specific invariant that caused valid CL FCU calls to be incorrectly rejected.

ManualBlockFinalizationManager: LastFinalizedHash and LastFinalizedBlockLevel are now kept in sync. The IBlockFinalizationManager.IsFinalized(long level) semantics correctly reflect the current CL-reported finalized level; the only non-AuRa consumer (ProcessedTransactionsDbCleaner) has its own monotonic _lastFinalizedBlock guard at line 33, so it correctly ignores lower finalized events. The doc update on IBlockFinalizationManager accurately reflects the new non-monotonic contract.

Test coverage: The three new tests (V1 cross-call + V1 payload-building + V3 payload-building) genuinely exercise the regression. forkchoiceUpdated_accepts_lower_finalized_than_previous_but_rejects_safe_before_finalized correctly does the two-step sequence (establish finalized=block[2], then verify finalized=block[1] is also accepted) and asserts both LastFinalizedHash and LastFinalizedBlockLevel are updated. The sibling-branch rejection test is unchanged and still validates ancestry correctness.

ParisEngineBlockchainTests: Appropriate addition — Paris is the first post-merge fork and was missing from the engine pyspec fixtures.

---

Findings

No new findings. No unresolved issues.

---

Verdict

No Critical, High, or Medium issues. Safe to merge.

[Copilot]

Pull request overview

Adjusts engine_forkchoiceUpdated handling to accept request-local finalized/safe markers even when the finalized block regresses relative to a previously accepted FCU, aligning Nethermind with Engine API request-local consistency rules and fixing Hive regressions. Also expands test coverage (including pyspec engine fixtures) to prevent recurrence.

Changes:

• Stop enforcing cross-request finalized monotonicity in ForkchoiceUpdatedHandler (keep request-local bounds + ancestry checks).
• Update manual finalization tracking to reflect the current finalized marker (not a monotonic max).
• Add regression tests for “lower finalized” scenarios and register Paris engine pyspec fixtures for CI.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/Nethermind/Nethermind.Merge.Plugin/Handlers/ForkchoiceUpdatedHandler.cs	Removes cross-request finalized monotonicity rejection; keeps per-FCU ordering/ancestry validation.
src/Nethermind/Nethermind.Merge.Plugin.Test/EngineModuleTests.V3.cs	Adds V3 regression test covering lower-finalized FCU while building payloads.
src/Nethermind/Nethermind.Merge.Plugin.Test/EngineModuleTests.V1.cs	Updates/extends V1 coverage for lower-finalized acceptance + payload-building scenario.
src/Nethermind/Nethermind.Blockchain/ManualFinalizationManager.cs	Makes LastFinalizedBlockLevel reflect the current finalized marker (can decrease).
src/Nethermind/Nethermind.Blockchain/IBlockFinalizationManager.cs	Updates documentation to match “current finalized level” semantics.
src/Nethermind/Ethereum.Blockchain.Pyspec.Test/Tests.cs	Registers ParisEngineBlockchainTests so Paris engine fixtures run in CI.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[LukaszRozmej]

@claude can new added tests in this PR be TestCase/TestCaseSource with themselves or existing tests?

[claude]

Claude finished @LukaszRozmej's task in 1m 24s —— View job

---

Test parameterization analysis

• Gather context
• Read new test methods in V1.cs and V3.cs
• Check existing TestCase patterns
• Post findings

---

Can the new tests use [TestCase] / [TestCaseSource]?

Short answer: limited opportunity, and merging them adds more complexity than it saves.

The three new tests

Test	File	What it tests
forkchoiceUpdated_accepts_lower_finalized_than_previous_but_rejects_safe_before_finalized	V1.cs	Cross-call lower-finalized acceptance + request-local safe-before-finalized rejection
forkchoiceUpdatedV1_should_allow_lower_finalized_than_previous_when_building_payload	V1.cs	Lower finalized accepted when payload-building (V1)
ForkChoiceUpdatedV3_should_allow_lower_finalized_than_previous_when_building_payload	V3.cs	Lower finalized accepted when payload-building (V3/Cancun)

The two payload-building tests (V1 + V3)

These test the same logical scenario at different fork versions. A [TestCase(bool cancun)] could technically merge them, but the setup diverges substantially:

• V3 needs CreateBlockchain(releaseSpec: Cancun.Instance, ...) + AddNewBlockV3 helper; V1 uses ProduceBranchV1.
• V3 PayloadAttributes has two extra fields (Withdrawals, ParentBeaconBlockRoot).
• V1 calls engine_forkchoiceUpdatedV1; V3 calls engine_forkchoiceUpdatedV3.

The existing codebase pattern keeps V1 and V3 in separate partial-class files precisely because of this divergence. Parameterizing them would mean either duplicating the setup inside a switch or extracting a factory — more noise than three clear, readable test methods.

The first V1 test

forkchoiceUpdated_accepts_lower_finalized_than_previous_but_rejects_safe_before_finalized combines two assertions (lower accepted + safe-before-finalized rejected) in a single flow. There's no natural boolean dimension to break it into a [TestCase] without splitting the two behaviors into separate tests, which is a style choice rather than a correctness concern.

Comparison with existing pattern

The [TestCase(bool cachedSafe)] on forkchoiceUpdated_rejects_repeated_hash_when_head_on_sibling_branch works well because the two cases differ in exactly one boolean with otherwise identical setup and assertion shape. The new tests don't have that clean symmetry.

---

Conclusion: The new tests are fine as standalone [Test] methods. There's no clear win from parameterizing them — the tests are short, their names are descriptive, and merging V1/V3 would require non-trivial indirection that the current three-test split avoids cleanly.