Move BlockCacheService forkchoice hashes and gossip policy to their proper owners

[asdacap]

Changes

A series of dependency-injection / ownership cleanups around BlockCacheService, the beacon sync strategy, and the block-gossip policy. No user-facing behavior change.

• Move FinalizedHash/HeadBlockHash out of IBlockCacheService into the beacon sync strategy. These CL-declared forkchoice hashes are now owned by BeaconSync: reads go through IBeaconSyncStrategy.GetFinalizedHash/GetHeadBlockHash, writes through a new IMergeSyncController.SetForkchoiceHashes. BlockCacheService reads them back from the strategy to decide which blocks to protect from pruning. Direct cache readers (MergeGossipPolicy, MergeFinalizedStateProvider, TaikoBeaconHeadAdvancer) now depend on IBeaconSyncStrategy.
• Invert cache clearing via an event, removing a circular dependency. Instead of BeaconSync reaching into IBlockCacheService.Clear() (which forced a Lazy<> to break the BeaconSync ↔ cache cycle), IBeaconSyncStrategy exposes a BeaconSyncStopped event that BlockCacheService subscribes to. The dependency is now one-way (cache → strategy); the cycle and the Lazy<> are gone.
• Move IGossipPolicy from the mutable INethermindApi.GossipPolicy property to Autofac DI. NetworkModule registers the Policy.FullGossip base; MergePluginModule and AuRaMergeModule decorate it with MergeGossipPolicy; Optimism/Taiko modules override it with ShouldNotGossip. The api property is removed (the container's FallbackToFieldFromApi guard forbids a service having both a registration and a settable api property), so the gossip policy is now a normal DI-resolved singleton like its existing consumers already expect.

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

Notes on testing

Added BlockCacheServiceTest.clears_cache_when_beacon_sync_stops for the new event-driven clear; updated BlockCacheServiceTest, MergeFinalizedStateProviderTests, MergePluginTests, and E2ESyncTests for the moved ownership/DI.

Verified locally (release):

• Full DI graph builds for every chain config (mainnet/Merge, Optimism, Taiko, Gnosis/Chiado AuRa-merge) — EthereumRunnerTests 271 pass / 2 env-skip.
• Nethermind.Merge.Plugin.Test, Nethermind.Merge.AuRa.Test, Nethermind.Optimism.Test, Nethermind.Taiko.Test, Nethermind.Network.Test, Nethermind.Synchronization.Test green (one pre-existing flaky block-production timing test in AuRa passes in isolation).

Documentation

Requires documentation update

• Yes
• No

Requires explanation in Release Notes

• Yes
• No

🤖 Generated with Claude Code

[claude]

Claude finished @asdacap's task in 6m 4s —— View job

---

PR Review: Move BlockCacheService forkchoice hashes and gossip policy to their proper owners

• Gather context and understand the PR
• Read relevant rule files (di-patterns, coding-style)
• Review changed files
• Analyze correctness, edge cases, and regressions
• Check DI patterns and wiring
• Evaluate test coverage
• Post findings

---

Summary

This is a clean dependency-ownership refactoring with no user-facing behavior change. The three independent cleanups are all logically correct. No critical or high findings.

---

DI wiring — verified correct

The gossip-policy layering is consistent across all chain configurations:

Chain	Base registration	Decorator / override	Resolved type
Mainnet/merge	NetworkModule → Policy.FullGossip	MergePluginModule decorates with MergeGossipPolicy	MergeGossipPolicy(Policy.FullGossip)
AuRa-merge	NetworkModule → Policy.FullGossip	AuRaMergeModule decorates with MergeGossipPolicy	MergeGossipPolicy(Policy.FullGossip)
Optimism	NetworkModule → Policy.FullGossip	OptimismModule overrides with ShouldNotGossip (last-wins)	ShouldNotGossip
Taiko	NetworkModule → Policy.FullGossip	TaikoModule overrides with ShouldNotGossip (last-wins)	ShouldNotGossip

AuRaMergePlugin overrides Module to return AuRaMergeModule (not MergePluginModule), so there is no double-decoration of MergeGossipPolicy on AuRa-merge chains. ✓

TestMergeModule uses MergePluginModule (which now includes the decorator) and relies on PseudoNetworkModule for the base IGossipPolicy(Policy.FullGossip) — both are present in all affected test containers. ✓

TaikoBeaconSync correctly forwards BeaconSyncStopped to the inner BeaconSync, and XdcBeaconSyncStrategy provides a correct no-op event implementation. ✓

Cache clearing inversion — correct

StopSyncing() fires BeaconSyncStopped only when !_isInBeaconModeControl, matching the old guard around _blockCacheService.Clear(). Semantics are preserved. Both BlockCacheService and BeaconSync are singletons so the event subscription established in the constructor lives for the node's full lifetime. ✓

Forkchoice hash ownership — correct

SetForkchoiceHashes is called in both paths in ForkchoiceUpdatedHandler (the fast-path at line 200 and the sync-start path at line 338), covering the same sites where the old direct property writes appeared. ✓

---

Findings

Low — double GetFinalizedHash() call in MergeGossipPolicy.ShouldDiscardBlocks

→ inline comment

Pre-existing pattern carried over from the old property double-read. Theoretically a TOCTOU (only conservative false-positives possible), but a local variable via is { } finalizedHash would be cleaner.

Low — _finalizedHash/_headBlockHash fields not volatile in BeaconSync

→ inline comment

Written by the engine-API thread (SetForkchoiceHashes) and read concurrently by network/sync threads. Not a regression from the unsynchronized { get; set; } properties on BlockCacheService, but now that ownership is consolidated, volatile would be the correct fix.

---

Verdict

	Count
Critical	0
High	0
Medium	0
Low	2

Both Low findings are pre-existing patterns (not introduced by this PR) and carry no safety risk. The PR is mergeable.

[claude]

Low — double method call / TOCTOU smell (pre-existing, carried over from old property access)

GetFinalizedHash() is called twice in the same expression. Theoretically, SetForkchoiceHashes on another thread could update the field between the two reads, producing an incoherent evaluation (e.g., first call returns a non-null non-zero value, then SetForkchoiceHashes(null, null) fires, second call returns null).

In practice the only observable effect is a transient false-positive on ShouldDiscardBlocks (we discard blocks we could have accepted), so this is conservative and safe. Still, a local variable would be cleaner:

Suggested change

	(_beaconSyncStrategy.GetFinalizedHash() is not null && _beaconSyncStrategy.GetFinalizedHash() != Keccak.Zero); /* _beaconSyncStrategy.GetFinalizedHash() is not null && _beaconSyncStrategy.GetFinalizedHash() != Keccak.Zero
	public bool ShouldDiscardBlocks => _poSSwitcher.TransitionFinished ||
	(_beaconSyncStrategy.GetFinalizedHash() is { } finalizedHash && finalizedHash != Keccak.Zero); /* _beaconSyncStrategy.GetFinalizedHash() is not null && _beaconSyncStrategy.GetFinalizedHash() != Keccak.Zero
	This condition was added for edge case situation.
	We started beacon sync, and we hadn't reached transition yet. If CL sent us non zero finalization hash, it would mean that network reached transition.
	However, in edge case situation (verified by merge hive tests), our node needs to be reorged to PoW again, so we can't add this condition _beaconSyncStrategy.GetFinalizedHash() != Keccak.Zero
	to PoSSwitcher.TransitionFinished. On the other hand, we don't want to receive any blocks from the network, so we want to discard blocks. */

[claude]

Low — hash fields are not volatile

_finalizedHash and _headBlockHash are written by SetForkchoiceHashes (engine API thread) and read concurrently by the gossip policy and cache-protection checks on network/sync threads. Without volatile (or Interlocked.Exchange) there's no cross-thread visibility guarantee; the JIT is permitted to cache the value in a register and miss subsequent writes.

This is not a regression — the original BlockCacheService { get; set; } properties were equally unsynchronized. But since ownership of these fields is now concentrated here, this is a good time to add volatile:

Suggested change

	private Hash256? _finalizedHash;
	private volatile Hash256? _finalizedHash;
	private volatile Hash256? _headBlockHash;