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

[LukaszRozmej]

@claude review

[claude]

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

---

Code Review (Pass 9)

• Read relevant rules files (coding-style, robustness, di-patterns)
• Diff and read changed files since pass 8 (8 commits)
• Assess @asdacap's CHANGES_REQUESTED (hide state boundary behind IWorldStateManager)
• Review IStateBoundary, StateBoundaryStore, WorldStateManager, FlatWorldStateManager
• Review DI wiring in WorldStateModule, FlatWorldStateModule, PruningTrieStoreModule
• Review EthCapabilitiesProvider, FullPruner, StateSyncRunner changes
• Review tests (StateBoundaryStoreTests, FullPruningDiskTest, HistoryPrunerTests)
• Final summary

---

Summary

This is pass 9. Eight commits landed since pass 8, addressing all outstanding @asdacap CHANGES_REQUESTED. Here is the complete status:

Prior Request	Status
"Hide behind IWorldStateManager please. Implement separately for flatdb." (×3)	✅ Resolved — IStateBoundary interface added; IWorldStateManager : IStateBoundary; each backend stores the floor in its own co-located DB
"What?" re FlatWorldStateModule (duplicate/stale OnActivate with OldestStateBlockStore)	✅ Removed — module cleaned to 37 lines; OldestStateBlockStore gone entirely
"Two time?" re WorldStateModule (OnActivate<IWorldStateManager> calling StateMetadataValidator)	✅ Removed — WorldStateModule is now a simple 37-line mapping stub

Architecture is clean. The key design choices:

• IStateBoundary lives in Nethermind.State (correct assembly — both trie and flat need it)
• WorldStateManager uses StateBoundaryStore(dbProvider.StateDb) — floor stored in the trie state DB, so wiping state automatically wipes the floor
• FlatWorldStateManager uses StateBoundaryStore(flatDb.GetColumnDb(FlatDbColumns.Metadata)) — floor stored in the flat metadata column, co-located with flat state
• WorldStateModule registers .Map<IStateBoundary, IWorldStateManager>(m => m) so any component requesting IStateBoundary gets the active world state manager
• EthCapabilitiesProvider, FullPruner, and StateSyncRunner all take IStateBoundary directly — no coupling to config or concrete type
• StateMetadataValidator and OldestStateBlockStore are fully deleted with no dangling references

StateBoundaryStore correctness verified (StateBoundaryStoreTests):

• Thread-safe via System.Threading.Lock
• Persists to DB before updating cache (commit 900df40) — no divergence on a thrown write
• Monotonic non-decreasing invariant enforced on set (commit 31d320d) — stale callers can't regress the floor
• Null reset allowed for explicit recovery
• Key Keccak.Compute("OldestStateBlock") is collision-free with both hash-keyed and HalfPath-keyed trie nodes

FullPruningDiskTest (commit d44237c) now verifies that after a successful prune the OldestStateBlock key is present in the new state DB, and correctly excludes that metadata entry from the pre/post subset check.

No new findings. All prior Low observations are resolved. No Critical, High, or Medium findings remain.

Verdict: ready to merge.

[claude]

Claude finished @LukaszRozmej's task in 4m 36s —— View job

---

Code Review (Pass 9)

• Read relevant rules files (coding-style, robustness, di-patterns)
• Read changed files and git log since pass 8
• Check @asdacap's outstanding CHANGES_REQUESTED (May 14)
• Review new changes
• Post inline comments and final summary

---

Summary

This is pass 9. All @asdacap CHANGES_REQUESTED from May 14 have been addressed in the 11 commits since pass 8.

@asdacap May 14 findings — resolved:

Finding	Status
"Hide behind IWorldStateManager please" (OldestStateBlockStore exposed as a standalone DI singleton)	✅ Fixed — IStateBoundary/IStateBoundaryWriter are now on IWorldStateManager; EthCapabilitiesProvider depends only on IStateBoundary. DI maps IStateBoundary → IWorldStateManager via WorldStateModule.
"Implement separately for flatdb"	✅ Fixed — FlatWorldStateManager holds its own StateBoundaryStore(flatDb.GetColumnDb(FlatDbColumns.Metadata)), co-located with the flat DB. Wiping the flat DB wipes the floor automatically.
"Two time?" (double StateMetadataValidator registration)	✅ N/A — StateMetadataValidator was eliminated entirely. Co-location of OldestStateBlock with the state DB makes it unnecessary: wipe-state-wipes-floor, so no startup recovery pass is needed.
"What?" (FlatWorldStateModule registering validator against IWorldStateManager lifecycle)	✅ N/A — same reason above.

Architecture is clean:

• IStateBoundary (read) / IStateBoundaryWriter (write) split interfaces keep each consumer to its minimum privilege.
• WorldStateManager persists via StateBoundaryStore(dbProvider.StateDb) — state DB is the storage.
• FlatWorldStateManager persists via StateBoundaryStore(flatDb.GetColumnDb(FlatDbColumns.Metadata)) — flat DB metadata column.
• StateBoundaryStore is thread-safe (System.Threading.Lock), persists before caching, and rejects backward non-null writes.
• FullPruner and StateSyncRunner both take IStateBoundaryWriter and advance the floor after completing their respective operations.
• DI singleton wiring via WorldStateModule.Map<IStateBoundary, IWorldStateManager> is correct — both trie and flat backends satisfy the interface.

All prior findings (passes 1–8, @asdacap × 2, @svlachakis × 2) remain resolved. No new Critical, High, or Medium findings.

Verdict: ready to merge.

[asdacap]

Not the righ place.

[asdacap]

Wrong. There is no store for flat, it just check the IPersistence directly.

[asdacap]

No mutation with flat. Flat already have the the state information written in PersistenceManager.

[asdacap]

No IStateBoundaryWriter. That is clearly trie store specific.

[asdacap]

Writer is triestore specific. Hide it. Flat already handle the state tracking. Only the reader is needed with flat.

[svlachakis]

@claude please check carefully this review whether it's valid or not

🔴 Critical — must fix before merge

Stale rebase reverts two merged PRs. The diff against origin/master silently undoes:

• KAD/discv4 debug logs #11811 (Log KAD report as a trace) — Nethermind.Network.Discovery/Kademlia/KBucketTree.cs removes the if (!_logger.IsDebug) return;
  early-return guard and flips _logger.Trace(...) → _logger.Info(...) in LogTreeStructure. Reverts the allocation guard and log-level fix.
• EIP-7778 gas fix #11794 (EIP-7778 gas fix) — Nethermind.Evm/TransactionProcessing/TransactionProcessor.cs and Nethermind.Evm.Test/Eip7778Tests.cs revert to
  pre-refund block gas tracking (drops _blockCumulativeReceiptGas).
• Plus a stray blank-line diff in Nethermind.Blockchain/ReadOnlyBlockTree.cs:37.

Fix: rebase onto current origin/master and drop the reversions.

🟠 High

1. FullPruner durability ordering (FullPruner.cs:188-194). Sequence is TryCopyTrie (which calls pruning.Commit() swapping the cloned DB) → then
   _stateBoundary.OldestStateBlock = stateToCopy. Kill the process in between and the new DB is live without the marker → eth_capabilities.state
   reports oldestBlock: 0 (archive-looking) even though state below is gone. Fix: write the marker into the cloning DB before Commit(), so the
   marker becomes atomic with the swap.
2. StateBoundaryStore.OldestStateBlock.get takes a lock on every call (StateBoundaryStore.cs:29-33). Reads dominate (every eth_capabilities),
   writes are rare (post-finalize-sync, post-full-prune). Fix: Volatile.Read on a long with long.MinValue sentinel for null; keep
   Interlocked.CompareExchange on the writer side for the monotonicity check.

🟡 Medium

3. (IStateBoundaryWriter)worldStateManager cast in FullPrunerFactory.cs:62 — FlatWorldStateManager doesn't implement the writer; the cast would
   InvalidCastException if wiring ever changed. Fix: inject IStateBoundaryWriter separately from DI; drop the cast.
4. Silent rejection of backward boundary writes (StateBoundaryStore.cs:39). If a reorg or bug ever drives a backward floor write, it's dropped
   with no log → invisible regression. Fix: keep dropping the write, but log at Warn/Debug with requested-vs-current values.
5. Redundant null check in EthCapabilitiesProvider.cs:73-76 — retention is { } cfg in the conjunction is provably true whenever windowOldest is
   { } w is true (windowOldest is non-null IFF retention is non-null per the line above). Restructure with a single early-return on retention is
   not { } cfg.

🟢 Low

6. Test naming pattern (EthRpcModuleTests.Capabilities.cs:289, 311, 326 and StateBoundaryStoreTests.cs:14, 18, 24, 36, 47). Mixed sentence-case
   and lower-snake_case violates the project's {What}{When}{Then} convention — e.g. eth_capabilities_no_head_disables_all_resources should be
   EthCapabilities_WhenHeadIsNull_DisablesAllResources.
7. Pre-existing typo _readaOnlyCodeCb in WorldStateManager.cs:19 — since the PR edits this class, cheap to ride along.

[claude]

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

---

Review Validation

• Read rules files and gather context
• Validate each finding against actual code
• Post final summary

---

Findings

🔴 Critical — "Stale rebase reverts #11811 and #11794"

#11811 (KBucketTree.cs): CONFIRMED VALID

The diff against origin/master confirms the PR branch has regressed two methods:

LogTreeStructure (KBucketTree.cs:405):

• master: if (!_logger.IsTrace) return; early-exit + _logger.Trace(...)
• PR branch: no guard + _logger.Info(...) (line 409)

LogTreeStatistics (KBucketTree.cs:369):

• master: if (!_logger.IsDebug) return; at top — skips entire tree traversal when debug is off
• PR branch: no early-exit, traversal always runs, only the _logger.Debug(...) call is guarded at the end

Impact assessment: LogDebugInfo() is called in Kademlia.cs inside if (_logger.IsDebug), so LogTreeStatistics missing the early-return guard has zero practical cost when debug is off. However, LogTreeStructure is more serious: when debug is on, it now (1) always runs the full recursive StringBuilder traversal and (2) emits the result at Info level — permanently visible output rather than Trace-only. This is a genuine regression but the caller guard means this only fires when debug logging is active, so "Critical" is an overstatement; Medium is more appropriate.

The rebase stale-merge explanation is correct: b955b50 Merge remote-tracking branch 'origin/master' must have resolved a conflict by keeping the PR's older KBucketTree.cs version.

#11794 (TransactionProcessor.cs): INCORRECT — reviewer has the direction backwards

origin/master line 681: long gasUsedForAllowance = _parallel ? 0 : header.GasUsed;  // old code, no _blockCumulativeReceiptGas
PR branch   line 84:  private long _blockCumulativeReceiptGas;                      // NEW — present only in PR
PR branch   line 689: { IsEip7778Enabled: true } => _blockCumulativeReceiptGas,     // NEW — present only in PR

The PR branch adds _blockCumulativeReceiptGas and the EIP-7778 spec-switch — it does not drop them. origin/master uses the older header.GasUsed path. The Eip7778Tests.cs change (test renamed from _admission_uses_pre_refund_block_gas_when_eip7778_enabled → _allowance_uses_post_refund_receipt_gas_when_eip7778_enabled) is consistent with implementing post-refund receipt tracking. This claim is a false positive — the direction of the diff was read incorrectly.

ReadOnlyBlockTree.cs blank line: CONFIRMED, BENIGN

The PR removes one blank line (line 37 in master). Trivial, but real.

---

🟠 High #1 — FullPruner durability ordering: VALID

FullPruner.cs:282 calls pruning.Commit() (DB swap) inside TryCopyTrie. Only after TryCopyTrie returns does _stateBoundary.OldestStateBlock = stateToCopy execute (FullPruner.cs:186). A crash in the window between the two operations leaves the new pruned DB live without the floor marker. On restart, OldestStateBlock is null and eth_capabilities would report state from genesis even though historical state below the pruning boundary is gone — routing the wrong queries to this node.

The suggested fix (write the marker into the cloning DB before Commit()) is the correct approach, making the marker atomic with the DB swap.

---

🟠 High #2 — StateBoundaryStore lock on every read: OVERSTATED

StateBoundaryStore.cs:32: lock (_lock) return _value;

The lock is correct: long? (Nullable<long>) is two words on 64-bit and is not atomically readable without synchronization. The reviewer's suggested Volatile.Read + sentinel is an optimization, but eth_capabilities is not a hot path — it is called by routing infrastructure occasionally, not on every block or transaction. The uncontested System.Threading.Lock acquisition overhead is in the nanoseconds range and is not a performance concern here. Low severity at most.

---

🟡 Medium #3 — Unsafe cast (IStateBoundaryWriter)worldStateManager in FullPrunerFactory.cs:63: VALID concern, SAFE IN PRACTICE

FlatWorldStateManager does not implement IStateBoundaryWriter. However, FullPrunerFactory.Create immediately returns null on line 43 when !pruningConfig.Mode.IsFull(), and full pruning never applies to flat state — so FlatWorldStateManager can never reach line 63. The cast is safe under current wiring, but the fragility is real: any future change that routes flat state through this path would throw InvalidCastException silently instead of a compile-time error. Injecting IStateBoundaryWriter directly is the right long-term fix but Low severity.

---

🟡 Medium #4 — Silent rejection of backward boundary writes: VALID, LOW

StateBoundaryStore.cs:40:

if (value.HasValue && _value.HasValue && value.Value < _value.Value) return;

Backward writes are silently dropped with no log. Adding a _logger.Warn (or even Debug) for the rejected value would aid diagnosis if a reorg or a bug produces a backward floor. The monotonicity invariant itself is correct behavior. Low severity — the reviewer's Medium is overstated.

---

🟡 Medium #5 — Redundant null check in EthCapabilitiesProvider.cs:72–74: VALID, LOW

EthCapabilitiesProvider.cs:68:

long? windowOldest = retention is { } r ? Math.Max(0L, head.Number - r) : (long?)null;

windowOldest is non-null if and only if retention is non-null. So retention is { } cfg in the compound condition on line 72 is provably always true when windowOldest is { } w is true — the check is redundant. This is a code-quality issue, not a functional bug. Low (the reviewer's Medium is overstated).

---

🟢 Low #6 — Test naming convention: VALID

The new tests use eth_capabilities_no_head_disables_all_resources (all-lowercase snake_case). The rest of the Nethermind.State.Test and Nethermind.JsonRpc.Test codebase consistently uses PascalCase_With_Underscores for test names (e.g., Reads_null_when_key_absent, Round_trips_value_through_kv_store in the same PR's StateBoundaryStoreTests.cs; BalanceOp_RecordsBalanceChange, NonceOp_RecordsNonceChange nearby). The reviewer's suggested convention (EthCapabilities_WhenHeadIsNull_DisablesAllResources) is in line with the codebase norm. Valid Low.

---

🟢 Low #7 — _readaOnlyCodeCb typo in WorldStateManager.cs:21: CONFIRMED VALID

Line 21: private readonly ReadOnlyDb _readaOnlyCodeCb; — missing the 'd' in _readOnly. The fix is cheap (rename all usages), but per AGENTS.md ("keep changes minimal and focused — don't touch unrelated code"), fixing a pre-existing typo in a PR whose focus is eth_capabilities adds noise to the diff. Worth a follow-up but not blocking.

---

Summary

Claim	Verdict
🔴 KBucketTree.cs _logger.Info / missing guard	VALID — regression, but Medium (not Critical); LogDebugInfo only called inside if (_logger.IsDebug)
🔴 TransactionProcessor.cs drops _blockCumulativeReceiptGas	INCORRECT — PR adds it; master is the one without it
🟠 High #1 FullPruner durability ordering	VALID High
🟠 High #2 StateBoundaryStore lock on reads	Overstated — should be Low; lock is correct, path is not hot
🟡 Medium #3 FullPrunerFactory unsafe cast	Valid concern, safe in practice — Low
🟡 Medium #4 Silent backward write rejection	Valid — Low
🟡 Medium #5 Redundant null check	Valid — Low
🟢 Low #6 Test naming	Valid
🟢 Low #7 Typo _readaOnlyCodeCb	Valid

The two actionable items before merge are: (1) fix the KBucketTree.cs regression (_logger.Info → _logger.Trace + if (!_logger.IsTrace) return; guard); (2) address the FullPruner durability gap by writing the floor marker into the cloning DB before Commit(). The TransactionProcessor.cs concern in the review is a false alarm.

[LukaszRozmej]

@claude re-review

[claude]

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

---

Code Review (Pass 10)

• Read relevant rules files (coding-style, robustness, di-patterns)
• Read git log and diff since pass 9 (commits 0ab6a0c, d14e7dc)
• Verify @svlachakis findings: KBucketTree regression and FullPruner durability ordering
• Check all other outstanding findings
• Post inline comments and final summary

---

Summary

This is pass 10. Two commits landed since pass 9:

Commit	What changed
0ab6a0c	StateBoundaryStoreTests converted from FluentAssertions (removed in #11567) to Assert.That NUnit assertions — build fix only
d14e7dc	Addresses all @svlachakis findings; see table below

All @svlachakis findings — resolved:

Finding	Verdict from pass 9 review	Status
🔴 KBucketTree.cs regression (#11811): _logger.Info / missing if (!_logger.IsTrace) return; guard	Valid Medium	✅ Fixed — LogTreeStatistics gets if (!_logger.IsDebug) return; at top; LogTreeStructure gets if (!_logger.IsTrace) return; and _logger.Info → _logger.Trace
🔴 TransactionProcessor drops _blockCumulativeReceiptGas (#11794)	False positive — PR adds it, master is without it	N/A
🟠 High #1 FullPruner durability ordering	Valid High	✅ Fixed — _stateBoundary.OldestStateBlock = stateToCopy now executes before pruning.Commit(). FullPruningDb mirrors all writes to the cloning DB during pruning (lines 89–102 of FullPruningDb.cs), so the floor is in both DBs before the swap. A crash between write and commit leaves the live DB with a conservative floor (over-reports pruning) rather than an absent floor (under-reports it — the dangerous direction).
🟠 High #2 StateBoundaryStore lock on reads	Overstated; Low — long? is not atomically readable; lock is correct; path is not hot	Not changed — correct as-is
🟡 Medium #3 Unsafe (IStateBoundaryWriter) cast in FullPrunerFactory	Valid; Low — safe under current wiring but fragile	✅ Fixed — IStateBoundaryWriter is now an explicit constructor parameter resolved by DI (PruningTrieStoreModule.cs:95); cast removed from factory
🟡 Medium #4 Silent backward OldestStateBlock write rejection	Valid; Low	✅ Fixed — StateBoundaryStore.cs:44 now logs at Warn with requested-vs-current values
🟡 Medium #5 Redundant null check in EthCapabilitiesProvider	Valid; Low	✅ Fixed — BuildState uses a single early-return if (stateBoundary.RetentionWindowBlocks is not { } retention) return ..., removing the redundant conjunction
🟢 Low #6 Test naming convention	Valid	✅ Fixed — tests renamed from eth_capabilities_* to Eth_capabilities_* PascalCase prefix
🟢 Low #7 _readaOnlyCodeCb typo in WorldStateManager.cs	Valid; pre-existing	Not fixed — pre-existing typo unrelated to this feature; per AGENTS.md, keep changes minimal and focused

Architecture is sound. The DI wiring in PruningTrieStoreModule maps IStateBoundaryWriter → PruningTrieStateFactoryOutput.WorldStateManager, which is safe because WorldStateManager : IStateBoundaryWriter. FlatWorldStateManager does not implement IStateBoundaryWriter, but IFullPrunerFactory is trie-specific and PruningTrieStoreModule is only loaded for the trie path — no exposure on flat.

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