feat(rpc): add eth_capabilities method for routing capability discovery

[divi2806]

Closes #11260

Changes

• Add eth_capabilities RPC method to IEthRpcModule and EthRpcModule per ethereum/execution-apis#755
• Add EthCapabilitiesResult response model with CapabilityHead, CapabilityResource, and CapabilityDeleteStrategy types
• Inject ISyncConfig and IPruningConfig as optional constructor parameters into EthRpcModule and EthModuleFactory to derive historical data availability
• Response includes current head block (number + hash) and six resource descriptors: state, tx, logs, receipts, blocks, stateproofs
• oldestBlock and deleteStrategy are omitted (null) for disabled resources and archive nodes; deleteStrategy only appears for in-memory pruned nodes (type: "window")
• stateproofs is disabled on pruned nodes since eth_getProof requires full trie history

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

4 tests added in EthRpcModuleTests.Capabilities.cs:

• eth_capabilities_returns_all_resources_and_head — verifies all 6 resource fields and head block info are present
• eth_capabilities_archive_node_has_stateproofs_enabled_and_no_delete_strategy — archive node (PruningMode.None) serves state proofs, has no window delete strategy
• eth_capabilities_pruned_node_disables_stateproofs_and_reports_window — memory-pruned node disables stateproofs, reports deleteStrategy.type = "window" with correct retentionBlocks
• eth_capabilities_no_receipts_disables_tx_logs_receipts — when DownloadReceiptsInFastSync = false, tx, logs, and receipts are disabled with no oldestBlock

All 5 tests pass (total: 5 | failed: 0 | succeeded: 5).

Documentation

Requires documentation update

• Yes
• No

Requires explanation in Release Notes

• Yes
• No

Add eth_capabilities JSON-RPC method. Returns the node's current head block and historical data availability for state, tx, logs, receipts, blocks, and stateproofs resources. Enables RPC routers and load balancers to make routing decisions without active probing. Implements ethereum/execution-apis#755.

[LukaszRozmej]

@claude review

[claude]

Claude finished @LukaszRozmej's task in 5m 43s —— View job

---

Code Review

• Gather context and read changed files
• Read relevant rules files (coding-style, robustness, di-patterns)
• Review EthCapabilitiesResult.cs
• Review EthRpcModule.cs changes
• Review EthModuleFactory.cs changes
• Review IEthRpcModule.cs changes
• Review test file
• Post inline comments and final summary

---

Summary

The overall structure is clean — the DI wiring is correct (ConfigRegistrationSource will automatically resolve ISyncConfig/IPruningConfig in production), the model types are well-structured, and the camelCase serialization from EthereumJsonSerializer maps property names to the spec correctly.

Three correctness issues with the boundary calculations warrant attention before merge:

Severity	Finding
Medium	AncientReceiptsBarrierCalc always returns ≥ 1 via Math.Max(1, ...). For an archive node (PivotNumber = 0), tx/logs/receipts report oldestBlock = "0x1" instead of "0x0". Genesis receipts exist, the API says they don't. → inline
Medium	Blocks.Disabled = false and OldestBlock = "0x0" are reported even when the node hasn't downloaded any headers (e.g., freshly started). Callers will assume full block history is available when it isn't. → inline
Medium	PruningMode.Full-only (no memory component) computes state.OldestBlock = head.Number, implying only the tip's state is available. This is overly conservative — full pruning sweeps run periodically, not block-by-block. → inline
Low	_blockFinder as IBlockTree silently returns null if the finder isn't a block tree; lowestBlock falls back to 0. Works today but is a fragile assumption. → inline
Low	Missing blank line before eth_capabilities. → inline
Low	No test validates the serialized JSON key names — only in-memory properties are asserted. A regression on naming policy or a misplaced [JsonPropertyName] would go undetected. → inline

[LukaszRozmej]

@claude re-review

[claude]

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

---

Re-Review

• Read relevant rules files
• Read changed files
• Analyze correctness, security, DI patterns, and tests
• Post inline comments
• Post final summary

---

Summary

All three Medium and all three Low findings from the prior review have been addressed in commit ed261ee. Here is the status of each:

Prior Finding	Status
Medium AncientReceiptsBarrierCalc returns ≥ 1, wrong for archive nodes	✅ Fixed: (syncConfig?.PivotNumber ?? 0) == 0 ? 0 : .... Regression test added.
Medium Blocks.OldestBlock = "0x0" when node hasn't finished syncing	✅ Fixed: headersAvailable = blockTree?.BestSuggestedHeader is not null guards the field.
Medium PruningMode.Full reports misleading state.OldestBlock = head.Number	✅ Fixed: Full-only pruning now returns null for OldestBlock. Test added.
Low _blockFinder as IBlockTree silent null fallback	✅ Addressed: if blockTree is null, headersAvailable = false → Disabled = true, so the ?? 0 fallback is never exposed. Comment added.
Low Missing blank line before eth_capabilities	✅ Fixed.
Low No test for wire-format JSON keys	✅ Fixed: eth_capabilities_serializes_to_spec_compliant_json_key_names added.

The test suite grew from 4 to 8 tests, covering: archive, memory-pruned, full-pruned, no-receipts, archive-receipts-genesis, null-configs, JSON key names, and all-resources-present.

Two new observations (both Low, non-blocking):

• State.Disabled = false is hardcoded. On a freshly-started memory-pruned node with no chain head, this yields { disabled: false, oldestBlock: null }. Callers should tolerate null oldestBlock on a non-disabled resource — worth documenting the intent. → inline
• eth_capabilities in IEthRpcModule has no ExampleResponse unlike every other implemented method. Minor, affects auto-generated docs only. → inline

Verdict: ready to merge. No Medium or higher findings remain unresolved.

[LukaszRozmej]

one last thing - cleanup json type

[LukaszRozmej]

don't format in the method, keep it as long. Json serialization will format it without allocating strings.

[divi2806]

Done — OldestBlock is now long?. Removed all .ToHexString() calls from EthCapabilitiesProvider; NullableLongConverter (globally registered in EthereumJsonSerializer) handles hex encoding on the wire.

[LukaszRozmej]

readonly record struct? Or record class

[LukaszRozmej]

@claude from performance perspective, better to make it struct or keeping this one class?

[LukaszRozmej]

@claude which ones to make structs for optimal performance?

[claude]

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

---

Struct vs Class Analysis for EthCapabilitiesResult types

• Read EthCapabilitiesResult.cs
• Read EthCapabilitiesProvider.cs
• Analyze performance tradeoffs
• Post answer

---

Answer

Rule of thumb: a struct saves a heap allocation only when it's stored as a field in a class (it lives inline) or in a Nullable<T> slot. A struct that itself contains 6+ struct fields becomes too large to pass by value safely (guideline: keep structs ≤ 16–24 bytes).

Type	Recommendation	Reasoning
EthCapabilitiesResult	record class	If the nested types become structs, each CapabilityResource is ~40 bytes × 6 = ~240 bytes inline. A 256-byte struct is too large — copying it is worse than a single class allocation. It's the top-level container; one heap allocation is fine.
CapabilityHead	readonly record struct	Two string references = ~16 bytes. Stored inline in EthCapabilitiesResult, saves 1 heap allocation per call. Ideal struct size.
CapabilityResource	readonly record struct	Used 6× per result — saves 6 heap allocations per call. Bool + string ref + Nullable<CapabilityDeleteStrategy> lands at ~32–40 bytes, acceptable when stored as fields in a class (not passed around by value).
CapabilityDeleteStrategy	readonly record struct	String ref + long = ~16 bytes. Used as CapabilityDeleteStrategy? → becomes Nullable<CapabilityDeleteStrategy> inline in CapabilityResource — no heap allocation for the strategy object.

Net effect on allocations per eth_capabilities call:

• Before: result + head + 6× resource + 0–1× deleteStrategy = 8–9 allocations
• After structs on all nested types: result only = 1 allocation

One caveat on CapabilityDeleteStrategy?: When CapabilityDeleteStrategy is a struct, DeleteStrategy becomes Nullable<CapabilityDeleteStrategy>. [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] on a nullable value type works correctly in System.Text.Json — it omits the property when the nullable has no value.

Separate from the struct question — LukaszRozmej's inline comments also ask to keep Number as long and Hash as Hash256 (let the serializer format them, avoiding intermediate string allocations). That applies on top of these struct changes.

[divi2806]

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

Struct vs Class Analysis for EthCapabilitiesResult types

• Read EthCapabilitiesResult.cs
• Read EthCapabilitiesProvider.cs
• Analyze performance tradeoffs
• Post answer

Answer

Rule of thumb: a struct saves a heap allocation only when it's stored as a field in a class (it lives inline) or in a Nullable<T> slot. A struct that itself contains 6+ struct fields becomes too large to pass by value safely (guideline: keep structs ≤ 16–24 bytes).

Type Recommendation Reasoning
EthCapabilitiesResult record class If the nested types become structs, each CapabilityResource is ~40 bytes × 6 = ~240 bytes inline. A 256-byte struct is too large — copying it is worse than a single class allocation. It's the top-level container; one heap allocation is fine.
CapabilityHead readonly record struct Two string references = ~16 bytes. Stored inline in EthCapabilitiesResult, saves 1 heap allocation per call. Ideal struct size.
CapabilityResource readonly record struct Used 6× per result — saves 6 heap allocations per call. Bool + string ref + Nullable<CapabilityDeleteStrategy> lands at ~32–40 bytes, acceptable when stored as fields in a class (not passed around by value).
CapabilityDeleteStrategy readonly record struct String ref + long = ~16 bytes. Used as CapabilityDeleteStrategy? → becomes Nullable<CapabilityDeleteStrategy> inline in CapabilityResource — no heap allocation for the strategy object.
Net effect on allocations per eth_capabilities call:

• Before: result + head + 6× resource + 0–1× deleteStrategy = 8–9 allocations
• After structs on all nested types: result only = 1 allocation

One caveat on CapabilityDeleteStrategy?: When CapabilityDeleteStrategy is a struct, DeleteStrategy becomes Nullable<CapabilityDeleteStrategy>. [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] on a nullable value type works correctly in System.Text.Json — it omits the property when the nullable has no value.

Separate from the struct question — LukaszRozmej's inline comments also ask to keep Number as long and Hash as Hash256 (let the serializer format them, avoiding intermediate string allocations). That applies on top of these struct changes.

Changed to record class based on Claude Bot's analysis — the nested types (CapabilityHead, CapabilityResource, CapabilityDeleteStrategy) are all readonly record struct, so EthCapabilitiesResult contains ~240 bytes of inline struct data. Making the top-level container a struct would cause large copies on every pass-by-value. Keeping it as a record class gives 1 heap allocation instead of 8–9.

[LukaszRozmej]

@claude review

[claude]

Claude finished @LukaszRozmej's task in 8m 56s —— View job

---

Code Review (Pass 4)

• Read relevant rules files (coding-style, robustness, di-patterns, performance)
• Diff and read changed files (6 commits since pass 3)
• Review EthCapabilities.cs / EthCapabilitiesProvider.cs
• Verify all three prior Low findings addressed
• Check IBlockFinder, BlockTree, FullPruner, StateSyncRunner changes
• Check DI wiring (ISyncPointers, IHistoryConfig, IHistoryPruner)
• Check tests
• Post inline comments and final summary

---

Summary

This is the fourth review pass. All three Low observations from pass 3 have been addressed in the five intervening commits:

Prior Low Finding	Status
Receipts.OldestBlock can understate floor during mid-sync header download	✅ Fixed — ISyncPointers.LowestInsertedReceiptBlockNumber now tracks actual progress; comment documenting transient nature added
Window descriptor silently suppressed when static floor dominates	✅ Fixed — one-line comment added explaining the suppression rationale (commit dd4dc74)
state always reported as enabled during snap sync before pivot floor written	✅ Fixed — BuildState now returns Disabled when fastSyncing && blockTree.OldestStateBlock is null (commit 8a05579)

Architecture is clean. The refactoring in 6c38e79 improved several things: EthCapabilitiesProvider is now sealed, takes required ctor params (no nullable defaults), uses IReadOnlyBlockTree, extracts BuildState with the multi-line why-comment, inlines the Resource helper into three focused methods, and gates FullPruner.OldestStateBlock write behind the explicit bool return from CopyTrie. The BlockTreeOverlay correctly falls back to the base tree for OldestStateBlock reads, matching the pattern for Head/Finalized/Safe.

One nit (non-blocking):

The ExampleResponse hash value is 0x000...000 (Keccak.Zero). The commit 2fa4434 fixed the previous 0xabc (3-char, schema-invalid) value to a 64-char hash, which satisfies the schema pattern. But Keccak.Zero is a sentinel value unlikely to appear as a real block hash — a hex-encoded random-looking value would make the docs more illustrative.

Verdict: ready to merge. No Critical, High, or Medium findings remain. The DI wiring is correct — ISyncPointers is registered as a singleton in Synchronizer.cs, IHistoryConfig is covered by ConfigRegistrationSource, IHistoryPruner is registered in BlockTreeModule — all resolve cleanly when EthCapabilitiesProvider is constructed.

[svlachakis]

@claude what do you think about the following points?

🔴 Correctness — Stateproofs: state is wrong for flat storage

EthCapabilities.cs:
Stateproofs: state
The spec deliberately separates state from stateproofs because eth_getProof requires Merkle/trie nodes, which a node may not retain even when the flat state is
queryable. FlatWorldStateManager.GetOldestStateBlock() correctly returns null (no rolling window), so state ends up Available for a flat-storage node — but
Stateproofs is aliased to the same value, advertising proof capability that eth_getProof cannot satisfy. Routers will misroute proof requests to flat nodes.

Fix: add an IWorldStateManager.SupportsTrieProofs (or equivalent) and gate Stateproofs:
Stateproofs: worldStateManager.SupportsTrieProofs ? state : Disabled
WorldStateManager (trie-store backed) → true; FlatWorldStateManager → false.

🔴 Correctness — Tx/Logs alias Receipts ignores LogIndex

EthCapabilities.cs:30,34 (Tx => Receipts, Logs => Receipts).

In Nethermind, tx-by-hash lookup goes through IReceiptStorage, so the Tx alias is defensible. But eth_getLogs is increasingly served by Nethermind.Db.LogIndex,
and ILogIndexConfig is already in the DI graph (it's even injected into BoundedModulePoolTests constructors in this very PR). If LogIndex has a different
retention floor than receipts, logs.oldestBlock is wrong.

At minimum: a comment explaining the invariant being relied on, or — preferably — feeding the actual log-index lower bound into Logs instead of reusing Receipts.
Spec carves them out for exactly this reason.

---

🟡 Minor

• IEthRpcModule.cs:324 ExampleResponse uses 0x000…000 for the head hash (Keccak.Zero). Schema-valid but visually a sentinel; replace with a realistic-looking
  64-hex value for the auto-generated docs (the @claude bot noted this too).
• HistoryPruner.SlotsPerEpoch made public const purely to expose 32 to EthCapabilitiesProvider. Prefer a method on IHistoryPruner (e.g.
  GetRetentionBlocks(RetentionEpochs)) so the spec's epoch→blocks conversion lives next to the pruner's other logic and stays out of public API surface.
• EthCapabilities.cs comment claims JsonPropertyOrder "preserves the spec's canonical key ordering on the wire." The spec doesn't mandate JSON object key ordering
  — clarify the comment is for example-payload visual parity, not a wire requirement.
• No-head path returns ChainHead(0, Hash256.Zero) plus all-Disabled. Genesis always exists so this is essentially dead code, but emitting a zero-hash payload at
  all is misleading. Consider returning a 503-style RPC error instead.
• receipts barrier test gap: BuildResource uses Math.Max(AncientBodiesBarrier, AncientReceiptsBarrier) — but no scenario in CapabilitiesScenarios() exercises
  AncientReceiptsBarrier > AncientBodiesBarrier. Worth one extra test.

[claude]

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

---

Analysis of @svlachakis's Points

• Read relevant source files (EthCapabilities.cs, EthCapabilitiesProvider.cs, IWorldStateManager.cs, FlatWorldStateManager.cs, FlatReadOnlyTrieStore.cs, FlatStateReader.cs, ILogIndexConfig.cs, HistoryPruner.cs, IEthRpcModule.cs)
• Analyze 🔴 Stateproofs aliased to state for flat storage
• Analyze 🔴 Tx/Logs alias Receipts ignores LogIndex
• Analyze 🟡 Minor points

---

🔴 #1 — Stateproofs aliased to state for flat storage

The premise needs a small correction, but the conclusion is right.

FlatWorldStateManager does implement IReadOnlyTrieStore via FlatReadOnlyTrieStore, and FlatStateReader.RunTreeVisitor reconstructs the Patricia Trie from flat data using ReadOnlyStateTrieStoreAdapter. So flat storage can generate Merkle proofs — eth_getProof does not hard-fail on flat nodes. The concern that "flat nodes don't retain trie nodes" is technically accurate (they don't store trie nodes; they reconstruct them on demand), but doesn't make eth_getProof return errors.

However the correctness issue still holds in a different way:

1. HasRoot(Hash256 stateRoot, long blockNumber) on FlatReadOnlyTrieStore calls flatDbManager.HasStateForBlock(...) — meaning flat state snapshots must exist for the requested block. If flat storage prunes old snapshots, eth_getProof will throw InvalidOperationException("State at {baseBlock} not found") for pruned blocks. Currently FlatWorldStateManager.GetOldestStateBlock returns null, so stateOldest = blockTree.OldestStateBlock ?? 0, meaning the PR advertises stateproofs available from genesis even when old flat snapshots may have been pruned.

2. Proof reconstruction from flat data is O(accounts) — potentially an extremely expensive operation on large state. Advertising it as equivalent to trie-backed proof availability is misleading to router implementors who would send proof requests expecting fast responses.

The worldStateManager.SupportsTrieProofs suggestion is valid and implementable: WorldStateManager (trie-backed) → true, FlatWorldStateManager → false (or a third variant reflecting "expensive but works"). This is a real correctness gap for the use case described in the spec (routing proof requests to capable nodes).

Verdict: 🔴 Confirmed — worth fixing before merge. The Stateproofs: state alias is over-optimistic for flat nodes.

---

🔴 #2 — Tx/Logs alias Receipts ignores LogIndex

Valid concern, but less severe than framed.

ILogIndexConfig has no OldestBlock or retention floor concept — it only has Enabled plus operational knobs. The log index is built from receipts, so today its coverage equals receipts coverage. The scenarios where they diverge:

• Log index enabled after node startup: the index only covers blocks from when it was enabled, even though receipts exist for earlier blocks. eth_getLogs via the index would return empty results for earlier blocks. The receipt scanner fallback would still work, but a router that respects logs.oldestBlock and queries a range before the index started would get incorrect results from a node advertising logs.oldestBlock = receiptsBarrier.
• Log index disabled: eth_getLogs falls back to sequential receipt scan → the alias is fully correct.

The spec separates them for exactly this reason. The fix (add an ILogIndex interface with an OldestBlock property and feed it into BuildResource) is clean. A code comment documenting the invariant being relied on ("log index has no independent retention floor, index coverage equals receipt coverage") would at minimum make the assumption explicit.

Verdict: 🔴 Confirmed — at minimum a comment, preferably a proper floor query. The scenario of a node with log index enabled only from block N advertising logs.oldestBlock = 0 is a real misrepresentation.

---

🟡 Minor Points

ExampleResponse uses Keccak.Zero (0x000...000):
Confirmed nit. Previous review pass 4 also called it out. Keccak.Zero is a sentinel, not a real block hash. A realistic-looking pseudorandom 32-byte hex string (e.g. 0xd4e56740f876aef8c010b86a40d5f56745a118d0906a34e69aec8c0db1cb8fa3 — the actual genesis hash) is better for generated docs.

HistoryPruner.SlotsPerEpoch made public solely for EthCapabilitiesProvider:
Valid. SlotsPerEpoch = 32 is an Ethereum protocol constant and leaks implementation detail into the public API surface. The GetRetentionBlocks(long retentionEpochs) method suggestion is cleaner and keeps the epoch→blocks conversion co-located with the pruner. Fix this →

EthCapabilities.cs comment about JsonPropertyOrder:
Confirmed inaccuracy. The comment says "preserves the spec's canonical key ordering on the wire" — the JSON spec has no canonical object key ordering, and the ethereum/execution-apis spec doesn't require one either. The comment should say "matches spec example payload for visual consistency" or similar. Low friction fix.

No-head path emitting ChainHead(0, Hash256.Zero):
Partially valid. Hash256.Zero is 0x000...000, not the genesis block hash, so a router receiving this would have a block hash pointing to nothing. On a freshly initialized node that hasn't synced genesis yet, this situation is real (not dead code). A ResultWrapper.Fail with ErrorCodes.InternalError would be cleaner. However, the actual exposure window is extremely narrow (between node start and genesis block insertion), so pragmatically it's low priority.

Test gap — AncientReceiptsBarrier > AncientBodiesBarrier:
Confirmed gap. BuildResource uses Math.Max(AncientBodiesBarrier, AncientReceiptsBarrier), but no test scenario in CapabilitiesScenarios() exercises AncientReceiptsBarrier > AncientBodiesBarrier. Worth one extra TheoryData row with e.g. AncientBodiesBarrier = 1000, AncientReceiptsBarrier = 2000 to verify the Math.Max branch is exercised.

---

Summary

Point	Severity	Verdict
Stateproofs aliased to state for flat storage	🔴	Valid — flat nodes advertise proof capability that is either unreliable (pruned snapshots) or O(state-size) expensive
Tx/Logs alias Receipts ignores LogIndex	🔴	Valid — divergence possible when log index starts after genesis; at minimum needs a comment
ExampleResponse Keccak.Zero	🟡	Valid nit
SlotsPerEpoch public API leak	🟡	Valid — prefer GetRetentionBlocks() on IHistoryPruner
JsonPropertyOrder comment misleading	🟡	Valid — easy one-line fix
No-head path emits zero-hash	🟡	Valid in principle; practical exposure window is tiny
Test gap for receipts barrier > bodies barrier	🟡	Valid — worth adding one test row

[asdacap]

Please move to IWorldStateManager or IStateReader instead.

[LukaszRozmej]

@claude re-review

[claude]

Claude finished @LukaszRozmej's task in 5m 31s —— View job

---

Code Review (Pass 5)

• Read relevant rules files (coding-style, robustness, di-patterns, performance)
• Diff and read changed files since pass 4
• Assess @asdacap's CHANGES_REQUESTED: move OldestStateBlock off IBlockFinder
• Assess @svlachakis's 🔴 findings (Stateproofs + LogIndex)
• Check all prior Low observations and 🟡 minor points
• Post inline comment and final summary

---

Summary

Commit ac03884 addresses all outstanding review feedback from @asdacap and @svlachakis. Here is the complete status:

Finding	Source	Status
OldestStateBlock on IBlockFinder (wrong interface)	@asdacap CHANGES_REQUESTED	✅ Fixed — moved to IBlockTree (writer side) + IWorldStateManager (consumer). IOldestStateBlockStore narrow interface carries the write path without a Nethermind.Blockchain dep into Nethermind.State.
Stateproofs aliased to state for flat-storage nodes	@svlachakis 🔴	✅ Fixed — IWorldStateManager.SupportsTrieProofs added. WorldStateManager returns true, FlatWorldStateManager returns false. Provider gates: stateproofs = worldStateManager.SupportsTrieProofs ? state : Disabled.
Tx/Logs alias Receipts, ignores LogIndex retention	@svlachakis 🔴	✅ Acknowledged — XML doc on EthCapabilities explains the invariant: log index is built from receipts so coverage equals receipt coverage; if index started late, eth_getLogs falls back to the receipt scanner. Comment captures the assumption explicitly. Full implementation deferred.
SlotsPerEpoch public solely for EthCapabilitiesProvider	@svlachakis 🟡	✅ Fixed — GetRetentionBlocks(long) added to IHistoryPruner; SlotsPerEpoch is private const again.
JsonPropertyOrder comment claimed wire contract	@svlachakis 🟡	✅ Fixed — comment now reads "matches the spec's example-payload key order for visual parity — not a wire requirement".
ExampleResponse uses Keccak.Zero	@svlachakis 🟡 / pass 4 Low	✅ Fixed — now uses 0xd4e56740f876aef8c010b86a40d5f56745a118d0906a34e69aec8c0db1cb8fa3 (genesis hash).
Test gap: AncientReceiptsBarrier > AncientBodiesBarrier	@svlachakis 🟡	✅ Fixed — fast_sync_receipts_barrier_above_bodies_barrier scenario added.

Architecture is clean. The IOldestStateBlockStore narrow interface correctly prevents Nethermind.State from depending on Nethermind.Blockchain. BlockTreeModule.cs casts IBlockTree → IOldestStateBlockStore in DI; both WorldStateManager and FlatWorldStateManager take it as a constructor param. BlockTreeOverlay correctly falls back to _baseTree.OldestStateBlock on reads. BlockTree persists the value to the metadata DB and loads it on startup.

One remaining Low observation:

Severity	Finding
Low	No test exercises SupportsTrieProofs = false. Every scenario in CapabilitiesScenarios() and GetCaps hardcodes wsm.SupportsTrieProofs.Returns(true). The stateproofs = worldStateManager.SupportsTrieProofs ? state : Disabled branch added as the fix for the 🔴 flat-storage finding is never covered. A one-row addition to CapabilitiesScenarios with SupportsTrieProofs = false would close this. → inline

Verdict: ready to merge. No Critical, High, or Medium findings remain. The single Low observation is a test-coverage gap for the stateproofs flat-storage fix — worth adding before merge but not a blocker.

[claude]

Low — no test for SupportsTrieProofs = false (flat-storage stateproofs disabled)

The GetCaps helper hardcodes wsm.SupportsTrieProofs.Returns(true) for every scenario. The new Stateproofs: worldStateManager.SupportsTrieProofs ? state : Disabled path — the whole fix for @svlachakis's 🔴 finding — is never exercised by the test suite. A regression that swapped the conditional would go undetected.

Consider adding one scenario row where SupportsTrieProofs = false and asserting caps.Stateproofs == Disabled:

yield return new CapabilitiesScenario(
    Name: "flat_storage_disables_stateproofs",
    ExpectedState: Available, ExpectedStateproofs: Disabled,
    ExpectedReceipts: Available, ExpectedBlocks: Available)
{ SyncConfig = fullSync, SupportsTrieProofs = false };  // or pass as a flag

(You'd need to thread supportsTrieProofs through CapabilitiesScenario and GetCaps similarly to how HistoryPruner is passed.)

[LukaszRozmej]

@claude review

[claude]

Claude finished @LukaszRozmej's task in 5m 33s —— View job

---

Code Review (Pass 6)

• Read relevant rules files (coding-style, robustness, di-patterns)
• Read changed files and git log since pass 5 (1bb0bcb, 4106a1e, b499b2d, cfd6015)
• Assess @asdacap's latest CHANGES_REQUESTED (OldestStateBlock on IBlockFinder)
• Review StateMetadataValidator and tests
• Inspect OldestStateBlockStore null-delete behavior
• Post inline comments and final summary

---

Summary

This is pass 6. Since pass 5 (four commits were merged):

Commit	What changed
1bb0bcb	OldestStateBlock removed from IBlockFinder/IBlockTree; WorldStateManager + FlatWorldStateManager own persistence via OldestStateBlockStore. FullPruner/StateSyncRunner write to worldStateManager.OldestStateBlock. IOldestStateBlockStore glue interface dropped.
4106a1e	SupportsTrieProofs removed; Stateproofs: state alias restored with a comment explaining flat storage retains trie nodes in path-keyed columns and serves eth_getProof in O(trie-depth).
b499b2d	StateMetadataValidator.DiscardStaleFloors added — startup check that clears OldestStateBlock and BestPersistedState when the recorded state root is no longer present on disk. Wired via OnActivate<IWorldStateManager> in both WorldStateModule and FlatWorldStateModule.
cfd6015	StateMetadataValidator moved from Nethermind.Init to Nethermind.Blockchain for testability; 7 unit tests added covering all code paths.

All outstanding findings from prior rounds are resolved:

Prior Finding	Status
@asdacap CHANGES_REQUESTED: OldestStateBlock on IBlockFinder	✅ Fixed — moved to IWorldStateManager exclusively
Pass 5 Low: no test for SupportsTrieProofs = false	✅ N/A — SupportsTrieProofs removed entirely (4106a1e)
@svlachakis 🔴 stateproofs-vs-flat correctness	✅ Resolved — both backends serve trie proofs, comment documents reasoning

Architecture is clean. The OldestStateBlockStore ownership is well-placed in Nethermind.State with proper layering — IWorldStateManager exposes the property, FullPruner and StateSyncRunner write through it, and neither touches IBlockTree for state availability. The StateMetadataValidator startup guard is a sound defensive measure for the state-wipe scenario.

One new Low observation:

Severity

Finding

Low

OldestStateBlockStore setter doesn't delete the DB key when set to null. StateMetadataValidator correctly clears the in-memory value on each startup (functionally correct), but the DB key persists indefinitely — meaning after a state wipe the stale floor is reloaded and re-detected on every restart. BlockTree.BestPersistedState has the same pattern. Fix: add else metadataDb.Remove(MetadataDbKeys.OldestStateBlock). → inline

Verdict: ready to merge. No Critical, High, or Medium findings remain.

[claude]

Low — DB key is not deleted when set to null

IKeyValueStore.Remove(key) is defined as Set(key, null) — setting null deletes the entry in RocksDB. But the setter only writes when value.HasValue; when StateMetadataValidator calls worldStateManager.OldestStateBlock = null to clear a stale floor, the in-memory _value becomes null but the DB key persists. On the next startup the stale value is loaded again and the validator re-clears it each time.

BlockTree.BestPersistedState has the same pattern (no-delete-on-null), so this follows existing convention. Functionally correct: the validator catches the stale floor on every startup. Still worth fixing for consistency:

Suggested change

	metadataDb.Set(MetadataDbKeys.OldestStateBlock, Rlp.Encode(value.Value).Bytes);
	if (value.HasValue)
	metadataDb.Set(MetadataDbKeys.OldestStateBlock, Rlp.Encode(value.Value).Bytes);
	else
	metadataDb.Remove(MetadataDbKeys.OldestStateBlock);

[LukaszRozmej]

@claude re-review

[claude]

Claude finished @LukaszRozmej's task in 5m 13s —— View job

---

Code Review (Pass 7)

• Read relevant rules files (coding-style, robustness, di-patterns)
• Diff and read changed files since pass 6 (b9f1947, 675346c)
• Check pass 6 Low finding: OldestStateBlockStore null-delete behavior
• Verify all prior findings resolved
• Post inline comments and final summary

---

Summary

This is pass 7. Two commits landed since pass 6 (b9f1947 and 675346c). All findings from prior rounds are resolved, and the new commits introduce two additional improvements.

Pass 6 Low finding:

Prior Low	Status
OldestStateBlockStore setter doesn't delete DB key when set to null	✅ Fixed — b9f1947 adds metadataDb.Delete(MetadataDbKeys.OldestStateBlock) in the null branch. Uses the C# 14 extension Delete(long key) => db.Remove(...) which correctly removes the entry from RocksDB.

New changes in 675346c:

Change	Assessment
Block/Receipt resources now Disabled when fast-sync descending pointers are null (pre-first-batch)	✅ Correct fix — IsDescendingResourceDownloaded returns false when pointer is null with a non-zero pivot. Previously the config barrier would be reported as oldestBlock before any data was actually downloaded.
IWorldStateManager.GetOldestStateBlock(long) replaced by RetentionWindowBlocks { get; } property	✅ Cleaner API — separates the configured window size from the head - retention computation. BuildState now derives windowOldest itself; this also fixes retentionBlocks being rounded down to head when head < retention on a fresh chain.
OldestStateBlockStore reads/writes locked with System.Threading.Lock	✅ Correct fix — long? is not atomically read/written on 32-bit-wide slots; the RPC path reads concurrently with FullPruner and StateSyncRunner. Targets net10.0, so System.Threading.Lock is available. The early if (_value == value) return; guard is inside the lock, so it doesn't introduce a TOCTOU.
New test scenario fast_sync_before_first_batch_disables_blocks_and_receipts	✅ Covers the case where LowestInsertedBodyNumber = null with a non-zero pivot — all four resources correctly reported Disabled.

Verification of key logic paths:

• Pre-batch with state not finalised: fastSyncing && OldestStateBlock is null → Disabled; pointers null → blocks/receipts Disabled. ✅
• Post-sync with memory pruning dominating: windowOldest >= stateFloor → DeleteStrategy emitted with configured retention, not clamped to current head. ✅
• No-head path: ChainHead(0, Keccak.Zero) with all-Disabled returned. Schema-valid (the JSON schema test validates this case indirectly via the main scenario tests). ✅

Verdict: ready to merge. No Critical, High, or Medium findings remain. No new Low observations.