Enforce EIP-3675 uncle ban at the spec level

[benaadams]

Changes

• Pin MaximumUncleCount=0 for post-merge releases so the consensus rule is enforced at the fork gate (BlockValidator.ValidateUncles) rather than only through MergeHeaderValidator's UnclesHash==empty check, which depends on PoSSwitcher resolving IsPostMerge correctly.
• Paris.Apply now sets spec.MaximumUncleCount=0; all post-Paris NamedReleaseSpec forks inherit 0 (Shanghai through Amsterdam).
• ChainSpecBasedSpecProvider gates MaximumUncleCount via a new IsPostMergeRelease helper covering TTD=0 (PoS-from-genesis), TerminalPoWBlockNumber, and Shanghai-or-later by timestamp (the signal that lets TTD-driven chains like mainnet register post-merge at spec-construction time).
• BlockchainTestBase drops the IsPostMerge=(Difficulty==0) shortcut so CI exercises the real PoSSwitcher path and matches hive's production semantics; PoSSwitcher.GetBlockConsensusInfo already has the same difficulty==0 fallback when TotalDifficulty is null.
• Verified: UncleFromSideChain_Cancun/Prague still reject via the spec gate; Ethereum.Blockchain.Block.Test (1146), Legacy (2539), and Nethermind.Specs.Test (208) all pass.

Types of changes

What types of changes does your code introduce?

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

Testing

Requires testing

• Yes

If yes, did you write tests?

• Yes

[claude]

Claude finished @benaadams's task in 4m 54s —— View job

---

PR Review

• Gather context and read changed files
• Load coding rules and specs
• Analyze correctness and edge cases
• Review security and performance implications
• Post final review

---

Summary

This PR is a well-targeted bug fix that enforces the EIP-3675 uncle ban at the spec level, reducing reliance on MergeHeaderValidator's runtime PoSSwitcher check. The core changes are sound. One medium inconsistency exists but is explicitly acknowledged in the PR description with rationale.

---

Finding: Medium — ChainSpecBasedSpecProvider vs MainnetSpecProvider inconsistency in the Paris→Shanghai window

File: src/Nethermind/Nethermind.Specs/ChainSpecStyle/ChainSpecBasedSpecProvider.cs:350-353

For TTD-driven chains like mainnet (large TTD, no TerminalPoWBlockNumber), IsPostMergeRelease returns false for all block-number-indexed specs before Shanghai, because:

1. TerminalTotalDifficulty != UInt256.Zero → false
2. TerminalPoWBlockNumber is null → releaseStartBlock > long.MaxValue → false
3. Eip4895TransitionTimestamp (0x64373057) > genesis timestamp (0x55ba4215) → third branch false

So ChainSpecBasedSpecProvider returns MaximumUncleCount = 2 for any block-indexed lookup in the Paris–Shanghai window, while MainnetSpecProvider.GetSpec returns Paris.Instance (now with MaximumUncleCount = 0) for the same range.

The Mainnet_loads_properly test skips over this window — MainnetActivations jumps directly from GrayGlacierBlockNumber to ShanghaiActivation — so the inconsistency isn't caught.

Impact: In production, MergeHeaderValidator always enforces the uncle hash check for post-merge blocks via poSSwitcher.IsPostMerge, so this gap doesn't open a consensus vulnerability. But it means the two spec providers diverge for Paris-era block queries, which could matter in edge cases (e.g., direct BlockValidator.ValidateUncles calls without MergeHeaderValidator in the stack).

Acknowledged in PR description: "which depends on PoSSwitcher resolving IsPostMerge correctly" / "the signal that lets TTD-driven chains like mainnet register post-merge at spec-construction time" — the Shanghai proxy is the intentional design choice.

Suggestion: A (ForkActivation)(MainnetSpecProvider.ParisBlockNumber, null) test case in Mainnet_loads_properly would document the known divergence explicitly (or a separate unit test for IsPostMergeRelease covering this case).

---

Finding: Low — AuRa override of MaximumUncleCount

File: src/Nethermind/Nethermind.Consensus.AuRa/Config/AuRaChainSpecEngineParameters.cs:86

spec.MaximumUncleCount = (int)(startBlock >= (MaximumUncleCountTransition ?? long.MaxValue) ? MaximumUncleCount ?? 2 : 2);

ApplyToReleaseSpec runs after CreateReleaseSpec and unconditionally overwrites whatever IsPostMergeRelease set. If an AuRa chain were genuinely post-merge and had no MaximumUncleCountTransition configured, it would silently revert to 2.

In practice, Gnosis sets MaximumUncleCountTransition = 0 (uncle count = 0 from genesis), so this is safe today. The test already gates Gnosis on MaximumUncleCount comparison (line 660). No action strictly needed, but worth being aware of if a new AuRa+merge chain is added.

---

Finding: Low — Comment style in BlockchainTestBase.cs

File: src/Nethermind/Ethereum.Test.Base/BlockchainTestBase.cs:272

// IsPostMerge is set by MergeProcessingRecoveryStep / PoSSwitcher during the pipeline.

Per the repo's coding-style rule — "Code comments explain why, not what" — this comment describes the mechanism (what sets IsPostMerge) rather than the constraint (why we must not set it here). A tighter framing would be: // Setting IsPostMerge here would bypass PoSSwitcher and diverge from hive production semantics. Minor nit.

---

Positive observations

• Paris.Apply is the right place to set MaximumUncleCount = 0. It flows through NamedReleaseSpec.ReplayAncestors so all post-Paris NamedReleaseSpec singletons (Shanghai through Amsterdam) inherit it correctly.
• IsPostMergeRelease is correct for its three target cases: TTD=0 networks, TerminalPoWBlockNumber-pinned networks, and Shanghai-or-later timestamp forks.
• The TerminalPoWBlockNumber boundary (releaseStartBlock > TerminalPoWBlockNumber) aligns exactly with how MergeBlockNumber is set (TerminalPoWBlockNumber + 1), so there's no off-by-one.
• BlockchainTestBase.cs removal of the difficulty shortcut is correct — it stops CI from bypassing PoSSwitcher and masks the exact class of bug this PR is fixing.
• EIP-3675 references in comments are accurate.

---

Verdict: Mergeable. The medium finding is explicitly acknowledged in the PR description with sound rationale (MergeHeaderValidator as fallback). The fix correctly closes the hive/production gap for the most impactful cases.

[Copilot]

Pull request overview

Updates release specs to enforce EIP-3675’s “no uncles post-merge” rule via MaximumUncleCount = 0, aiming to make uncle rejection occur in BlockValidator.ValidateUncles (spec-gated) rather than relying primarily on merge-header checks and IsPostMerge resolution.

Changes:

• Set Paris.Apply to pin MaximumUncleCount = 0 so post-Paris forks inherit the uncle ban.
• Gate ReleaseSpec.MaximumUncleCount in ChainSpecBasedSpecProvider via a new IsPostMergeRelease(...) helper.
• Adjust blockchain tests to stop manually setting Header.IsPostMerge and instead exercise the real PoS switcher path.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/Nethermind/Nethermind.Specs/Forks/15_Paris.cs	Sets Paris fork to enforce MaximumUncleCount=0 (uncle ban).
src/Nethermind/Nethermind.Specs/ChainSpecStyle/ChainSpecBasedSpecProvider.cs	Adds post-merge detection to choose 0 vs 2 uncles in constructed ReleaseSpecs.
src/Nethermind/Ethereum.Test.Base/BlockchainTestBase.cs	Removes test-only IsPostMerge shortcut to align with production pipeline behavior.

---

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

[benaadams]

@claude re-review

[claude]

Claude finished @benaadams's task in 5m 15s —— View job

---

PR Re-Review

• Gather context and read changed files
• Load coding rules and analyze current state
• Assess Copilot's terminal PoW block concern
• Re-evaluate previous findings
• Post final review

---

Summary

The PR has been significantly revised since the first review. The Copilot concern about the terminal PoW block was valid and has been correctly addressed: the uncle ban was moved from Paris.Apply to Shanghai.Apply in commit 7004eb1. All three concerns from the prior review are now resolved. Only Low findings remain.

---

Resolved since last review

✅ Previous Medium — ChainSpecBasedSpecProvider vs MainnetSpecProvider inconsistency

Resolved. Previously, Paris.Apply set MaximumUncleCount=0, which meant MainnetSpecProvider.GetSpec returned 0 for the Paris-era (including the terminal PoW block) while ChainSpecBasedSpecProvider kept 2, diverging. Now Paris.Apply does not set MaximumUncleCount. Both providers return 2 for Paris-era blocks and 0 for Shanghai+. The inconsistency is gone.

✅ Copilot concern 1 — Paris uncle ban would reject a valid PoW block

Resolved correctly. MainnetSpecProvider.GetSpec returns Paris.Instance for block 15537393 (the terminal PoW block, which is still PoW and may have valid uncles) because the GrayGlacier range is < ParisBlockNumber (strictly less-than). Setting MaximumUncleCount=0 in Paris.Apply would have caused BlockValidator.ValidateUncles to reject that block. Moving the spec gate to Shanghai sidesteps this entirely.

✅ Previous Low — Comment style in BlockchainTestBase.cs

Addressed. The comment now explains the constraint: "Setting IsPostMerge here would bypass PoSSwitcher and hide divergences between CI and hive's production pipeline."

---

Remaining findings

Low — AuRa ApplyToReleaseSpec unconditionally overwrites MaximumUncleCount

File: src/Nethermind/Nethermind.Consensus.AuRa/Config/AuRaChainSpecEngineParameters.cs:86

spec.MaximumUncleCount = (int)(startBlock >= (MaximumUncleCountTransition ?? long.MaxValue) ? MaximumUncleCount ?? 2 : 2);

ApplyToReleaseSpec runs after CreateReleaseSpec, overwriting whatever IsPostMergeRelease set. For an AuRa chain that is post-merge but has no MaximumUncleCountTransition configured, this silently reverts to 2. Gnosis avoids this because it sets MaximumUncleCountTransition = 0 from genesis. The Mainnet_loads_properly test skips the MaximumUncleCount comparison for Gnosis. No regression in this PR — carried-forward from existing behavior.

Low — IsPostMergeRelease comment overstates TerminalPoWBlockNumber coverage

File: src/Nethermind/Nethermind.Specs/ChainSpecStyle/ChainSpecBasedSpecProvider.cs:348

The comment says "TerminalPoWBlockNumber covers chainspecs that pin the boundary explicitly." This is accurate only when the chain has additional post-merge block-number transitions beyond TerminalPoWBlockNumber. Since TerminalPoWBlockNumber is explicitly excluded from AddTransitions (line 44), it creates no spec boundary itself. A chain with only TerminalPoWBlockNumber and no later block-number forks (like test_spec.json: terminalPoWBlockNumber=0x64, no post-merge block transitions) will still get MaximumUncleCount=2 for post-merge specs — the branch never fires. The safety net (MergeHeaderValidator) still applies in this case, but the comment implies broader coverage than the code delivers. A more accurate version: "TerminalPoWBlockNumber works for chains that have post-merge block-number transitions, where releaseStartBlock > TerminalPoWBlockNumber."

Low — "off-by-one" terminology in Paris.cs comment is imprecise

File: src/Nethermind/Nethermind.Specs/Forks/15_Paris.cs:9

"due to an off-by-one in its block-range resolution" implies a bug. The < ParisBlockNumber boundary is intentional — ParisBlockNumber = 15537393 IS the terminal PoW block, and it's correct for it to fall under Paris. A cleaner phrasing: "MainnetSpecProvider maps block 15537393 (the terminal PoW block) to Paris.Instance — the GrayGlacier boundary is < ParisBlockNumber so block 15537393 is Paris — making Paris a mixed-era spec."

---

Positive observations

• Shanghai is the correct anchor for MaximumUncleCount=0. It's the first unambiguously post-merge boundary in MainnetSpecProvider.GetSpec (pattern { Timestamp: < CancunBlockTimestamp }), and Eip4895TransitionTimestamp in IsPostMergeRelease maps directly to it for TTD-driven chains like mainnet.
• Both spec providers are now consistent for mainnet: MaximumUncleCount=2 for the Paris window, 0 for Shanghai+.
• BlockchainTestBase.cs removal of the IsPostMerge shortcut is correct and the improved comment explains the exact class of bug this prevents.
• IsPostMergeRelease comment (lines 346–351) accurately describes the three-branch structure and explicitly acknowledges the Paris-Shanghai window limitation.

---

Verdict: Mergeable. All Critical/Medium/High findings are resolved. Remaining Low findings are documentation nits that don't affect correctness.

[benaadams]

@claude re-review

[claude]

Claude finished @benaadams's task in 1m 29s —— View job

---

PR Re-Review

• Gather context and read changed files
• Load coding rules
• Analyze correctness and edge cases
• Assess previous findings against current state
• Post final review

---

Summary

The latest commit (88d0e73 Tighten uncle-ban comments) addresses all three Low findings from the previous review. No Critical/High/Medium findings exist.

---

Resolved since last review

✅ Previous Low — "off-by-one" terminology in Paris.cs comment imprecise

Fixed. The comment no longer uses "off-by-one". It now correctly explains that GrayGlacier→Paris boundary is < ParisBlockNumber, so block 15537393 (terminal PoW block) falls under Paris.Instance, making Paris a mixed-era spec — and explains why pinning MaximumUncleCount=0 here would spec-reject a consensus-valid PoW block.

✅ Previous Low — IsPostMergeRelease comment overstated TerminalPoWBlockNumber coverage

Fixed. The comment (lines 346–353) now explicitly states: "TerminalPoWBlockNumber fires only when releaseStartBlock advances past it, which requires the chainspec to carry post-merge block-number transitions (TerminalPoWBlockNumber is excluded from AddTransitions above, so it creates no spec boundary of its own). Paris-era blocks on TTD-driven chains with no such later transitions are not detected here — runtime MergeHeaderValidator (UnclesHash == empty via PoSSwitcher) is the safety net." This is accurate and complete.

✅ Previous Low — BlockchainTestBase.cs comment style

Already resolved in prior iteration. Comment correctly explains the constraint: "Setting IsPostMerge here would bypass PoSSwitcher and hide divergences between CI and hive's production pipeline."

---

Remaining findings

Low — AuRa ApplyToReleaseSpec unconditionally overwrites MaximumUncleCount

File: src/Nethermind/Nethermind.Consensus.AuRa/Config/AuRaChainSpecEngineParameters.cs:86

Pre-existing issue, not introduced by this PR. ApplyToReleaseSpec runs after CreateReleaseSpec and overwrites whatever IsPostMergeRelease set. Safe today because Gnosis sets MaximumUncleCountTransition=0 from genesis. No regression here.

---

Positive observations

• All three comment concerns from the previous review are fully resolved in 88d0e73. The comments now explain why (the constraint, the safety net, the mixed-era nature) rather than what.
• Core logic unchanged — the Shanghai-as-anchor approach is correct and both spec providers remain consistent.
• IsPostMergeRelease three-branch structure with explicit safety-net documentation is now clear and accurate for all code readers.

---

Verdict: Mergeable. All Critical/High/Medium findings are resolved. The one remaining Low finding is pre-existing in AuRa and not introduced by this PR.