Skip to content

Unify assertions#11567

Merged
flcl42 merged 34 commits into
masterfrom
assertions
May 27, 2026
Merged

Unify assertions#11567
flcl42 merged 34 commits into
masterfrom
assertions

Conversation

@flcl42

@flcl42 flcl42 commented May 12, 2026

Copy link
Copy Markdown
Contributor

Changes

  • Unify assertions

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

@claude

claude Bot commented May 12, 2026

Copy link
Copy Markdown
Contributor

Claude finished @flcl42's task in 9m 23s —— View job


PR Review: Unify Assertions

  • Gather context and understand the PR scope
  • Analyze assertion pattern changes across files
  • Review for correctness issues in assertion migrations
  • Check for potential regressions
  • Post findings

Summary

This PR removes FluentAssertions, FluentAssertions.Json, and Shouldly entirely from the codebase (6,267 Should() calls replaced with NUnit-native assertions across 100 files). The migration is comprehensive and well-structured, but a few subtle semantic differences between the old and new assertion APIs introduce low-to-medium risk.


Medium

1. Is.EquivalentToIs.EqualTo for timed async collection polls (ContractDataStoreTests.cs)

ContractDataStoreTests.cs, lines ~112, ~149, ~172, ~195

// Before (order-independent):
Is.EquivalentTo(expected.Cast<object>()).After(200, 20)

// After (order-dependent):
Is.EqualTo(expected.Cast<object>()).After(200, 20)

Is.EquivalentTo treats the collection as a set (any order), while Is.EqualTo on IEnumerable is order-dependent. In timed assertions (.After(200, 20) polling), this could produce flaky tests if the ContractDataStore doesn't guarantee a deterministic ordering when updated asynchronously. Confirm that ordering is guaranteed or revert to Is.EquivalentTo.

Fix this →


2. NotThrow<InvalidOperationException>Throws.Nothing semantic change

ContractBasedValidatorTests.cs, line ~521, LogFinderTests.cs, line ~153

// Before: Only fails if InvalidOperationException is thrown
preProcess.Should().NotThrow<InvalidOperationException>(test.TestName);

// After: Fails for ANY exception
Assert.That(preProcess, Throws.Nothing);

Should().NotThrow<T>() in FluentAssertions only fails if the specific exception type is thrown — other exceptions pass through. Throws.Nothing fails for any exception whatsoever. This is semantically stricter and is generally the correct behavior, but it could surface previously-hidden other exceptions from these methods. Confirm no other exceptions are expected from the tested code paths.


3. Custom ContainsSubtree array comparison uses subset (unordered) semantics

AbiDefinitionParserTests.cs, lines ~32–68

// New implementation - for JArray, it searches for each expected item ANYWHERE in actual:
foreach (JToken expectedItem in expectedArray)
{
    bool found = false;
    foreach (JToken actualItem in actualArray)  // subset search, not positional
    {
        if (ContainsSubtree(actualItem, expectedItem)) { found = true; break; }
    }
    if (!found) return false;
}

The original FluentAssertions.Json's ContainSubtree also uses subset/unordered semantics for arrays, so this likely matches. However, the ABI JSON arrays (function params, event inputs) are positionally significant — two ABI definitions could produce the same set of array elements in different positions, and this test would still pass. Recommend adding a position-aware comparison if array ordering matters for ABI correctness.


Low

4. AssertBlocksEquivalent checks only Header.Hash instead of full header

BlockStoreTests.cs, lines ~170–175

// Before: full structural comparison (all BlockHeader fields)
retrieved.Header.Should().BeEquivalentTo(block.Header);

// After: only hash checked
Assert.That(retrieved.Header.Hash, Is.EqualTo(block.Header.Hash));

Fields not included in hash computation (e.g. cached values, mutable metadata) would silently differ. This is a weaker assertion. For round-trip serialization tests, the hash check covers all consensus-critical fields, but non-hashed fields (like EncodedSize, which was already excluded) are unverified.


5. Transaction collection equality relies on reference identity

TransactionSelectorTests.cs, line ~476, PermissionTxComparerTests.cs, line ~278

// Transaction does not implement Equals; this works only because
// the same object instances appear in both collections:
Assert.That(selectedTransactions, Is.EqualTo(testCase.ExpectedSelectedTransactions));

Transaction has no IEquatable<Transaction> implementation, so Is.EqualTo on a collection of transactions falls back to reference (object.Equals) comparison. This accidentally works because the same instances are used in both actual and expected. Anyone adding a test case with newly-constructed expected transactions would see a silent false-negative. Consider using EqualToTransaction checks in a loop for clarity.


6. Duplicated assertion helpers in two test files

AssertDestinationsEquivalent + ComparableDestination (private record) appear in both:

  • ContractDataStoreTests.cs
  • TxPriorityContractTests.cs

These should be extracted to a shared test utility to reduce duplication.


Non-issues (verified)

  • EqualToTransaction is a void extension that asserts internally — calling it without capturing a return value is correct.
  • AssertReceiptEquivalent correctly omits Error (matching the original Excluding(su => su.Error) option).
  • MergeTestAssertions.AssertJsonEquivalent (JSON string comparison) is appropriate for ExecutionPayload DTOs that were previously compared with IgnoringCyclicReferences().
  • Memory<byte> / ReadOnlyMemory<byte> comparisons are handled via .ToArray() in EqualToTransaction, correctly replacing the deleted global AssertionsSetup.
  • Throws.TypeOf<T>() vs. Should().Throw<T>() (exact type vs. subtype): unlikely to matter in practice since argument null checks throw exactly ArgumentNullException.

Comment thread src/Nethermind/Nethermind.AuRa.Test/Contract/ContractDataStoreTests.cs Outdated
Comment thread src/Nethermind/Nethermind.AuRa.Test/Validators/ContractBasedValidatorTests.cs Outdated
Comment thread src/Nethermind/Nethermind.Blockchain.Test/TransactionSelectorTests.cs Outdated
Comment thread src/Nethermind/Nethermind.Abi.Test/Json/AbiDefinitionParserTests.cs
@github-actions github-actions Bot added the tools label May 12, 2026
flcl42 added 5 commits May 12, 2026 16:19
# Conflicts:
#	src/Nethermind/Nethermind.Config.Test/ConfigProviderTests.cs
# Conflicts:
#	src/Nethermind/Nethermind.Abi.Test/AbiTests.cs
#	src/Nethermind/Nethermind.BalRecorder.Test/BalRecorderE2ETests.cs
#	src/Nethermind/Nethermind.BalRecorder.Test/RecordedBalStoreTests.cs
#	src/Nethermind/Nethermind.Blockchain.Test/Consensus/NullSignerTests.cs
#	src/Nethermind/Nethermind.Blockchain.Test/Consensus/SignerTests.cs
#	src/Nethermind/Nethermind.Core.Test/Crypto/SignatureTests.cs
#	src/Nethermind/Nethermind.Core.Test/Encoding/AccessListDecoderTests.cs
#	src/Nethermind/Nethermind.Core.Test/Encoding/AuthorizationTupleDecoderTests.cs
#	src/Nethermind/Nethermind.Core.Test/Encoding/HeaderDecoderTests.cs
#	src/Nethermind/Nethermind.Core.Test/Encoding/LogEntryDecoderTests.cs
#	src/Nethermind/Nethermind.Evm.Test/Eip7928Tests.cs
#	src/Nethermind/Nethermind.Evm.Test/Tracing/GasEstimationTests.cs
#	src/Nethermind/Nethermind.JsonRpc.Test/Modules/DebugRpcModuleTests.cs
#	src/Nethermind/Nethermind.JsonRpc.Test/Modules/Eth/EthRpcModuleTests.EthCall.cs
#	src/Nethermind/Nethermind.Merge.Plugin.Test/EngineModuleTests.PayloadProduction.cs
#	src/Nethermind/Nethermind.Merge.Plugin.Test/EngineModuleTests.Synchronization.cs
#	src/Nethermind/Nethermind.Merge.Plugin.Test/EngineModuleTests.V1.cs
#	src/Nethermind/Nethermind.Merge.Plugin.Test/EngineModuleTests.V5.cs
#	src/Nethermind/Nethermind.Merge.Plugin.Test/SszRest/SszMultiSegmentDecodeTests.cs
#	src/Nethermind/Nethermind.State.Test/TracedAccessWorldStateTests.cs
#	src/Nethermind/Nethermind.Taiko.Test/TaikoExecutionPayloadTests.cs
#	src/Nethermind/Nethermind.TxPool.Test/Collections/SortedPoolTests.cs
#	src/Nethermind/Nethermind.TxPool.Test/TxPoolTests.cs
#	src/Nethermind/Nethermind.Xdc.Test/ExtraConsensusDataDecoderTests.cs
#	src/Nethermind/Nethermind.Xdc.Test/ModuleTests/SyncInfoDecoderTests.cs
#	src/Nethermind/Nethermind.Xdc.Test/QuorumCertificateDecoderTests.cs
#	src/Nethermind/Nethermind.Xdc.Test/SnapshotDecoderTests.cs
#	src/Nethermind/Nethermind.Xdc.Test/SubnetSnapshotDecoderTests.cs
#	src/Nethermind/Nethermind.Xdc.Test/TimeoutCertificateDecoderTests.cs
#	src/Nethermind/Nethermind.Xdc.Test/VoteDecoderTests.cs
#	src/Nethermind/Nethermind.Xdc.Test/XdcSubnetHeaderDecoderTests.cs
flcl42 added 2 commits May 25, 2026 14:11
# Conflicts:
#	src/Nethermind/Nethermind.State.Flat.Test/Persistence/BloomFilter/BloomFilterTests.cs
#	src/Nethermind/Nethermind.State.Test/StateReaderTests.cs
@flcl42

flcl42 commented May 25, 2026

Copy link
Copy Markdown
Contributor Author

@claude review deeply again

@claude

claude Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor

Claude finished @flcl42's task in 11m 29s —— View job


PR Review: Unify Assertions (Deep Review #4)

  • Load rules and gather context
  • Check outstanding issues from previous reviews (C1, C2-1/2/3, C3-Feature/jsonrpc #1/Feature/networking #2)
  • Deep audit the current branch state for each issue
  • Check for any new issues not caught in prior passes
  • Post findings

Summary

Fourth pass. All critical and high issues from prior reviews are resolved. Two lower-priority items remain, one of which (M2) was previously raised by @LukaszRozmej and is still unaddressed.


Previously raised issues — all resolved ✅

Issue Resolution
C1: Throws.TypeOf<OperationCanceledException> in ForwardHeaderProviderTests ✅ Changed to Throws.InstanceOf
C2-1: Withdrawals comparison without Equals (PayloadDecoderTests) UsingPropertiesComparer<Withdrawal> added
C2-2: Transaction reference-equality trap (TxPoolTests.Blobs) ✅ Changed to AssertTransactionsEquivalent
C2-3: TopicsFilter abstract type comparison (FilterStoreTests) Using<TopicsFilter>(TopicsFiltersEqual) with type + field checks
C2-4: Dictionary.Keys ordering sensitivity (MultiValidatorTests) Is.EquivalentTo (order-independent)
C3: Throws.Nothing replacing typed NotThrow<T> (ContractBasedValidatorTests, Eth67, ReadOnlyBlockTree, etc.) ✅ New Assert.DoesNotThrow<T> C# 14 extension in ExceptionAssertExtensions.cs; correctly replicates FA semantics and is tested
EraE production code changes (BeaconApiRootsProvider, BlocksRootContext, Validator) ✅ Reverted in a3a5e21
Copyright year changes in 21 files ✅ Reverted

New helper ExceptionAssertExtensions.DoesNotThrow<T> (commit bf39d34): The C# 14 extension(Assert) idiom is correct, semantics match FA (NotThrow<T> swallows non-T exceptions — confirmed by 4 explicit tests in ExceptionAssertExtensionsTests.cs). All 6 call sites reference Nethermind.Core.Test via ProjectReference. ✅

UsingTransactionComparer exclusions (C5): MaxPriorityFeePerGas and ValueRef are both ref readonly properties backed by the same fields as GasPrice and Value. Excluding them hides no data — the underlying values are covered by the non-ref counterparts. ✅


Medium — UsingReceiptComparer silent-drop on unknown names

src/Nethermind/Nethermind.Core.Test/Encoding/ReceiptAssertionExtensions.cs (inline comment posted)

options = excludedProperty switch
{
    nameof(TxReceipt.Error) => options.Excluding(static receipt => receipt.Error),
    _ => options   // ← silently drops any unrecognised name
};

Only nameof(TxReceipt.Error) is recognised. Any other name is dropped with no diagnostic. Currently all three callers (PersistentReceiptStorageTests.cs lines 104, 106, 145) pass nameof(TxReceipt.Error), so no active regression. But the API is fragile: a future typo or a new property name silently becomes a no-op exclusion. Add _ => throw new ArgumentException(...) or extend the switch with the full set of supported names.

Fix this →


Low — Throws.TypeOf<OperationCanceledException> inconsistent with codebase

src/Nethermind/Nethermind.Evm.Test/Tracing/CancellationTracerTests.cs:27 (file not in diff, so inline comment could not be posted):

Assert.That(() => tracer.ReportActionError(EvmExceptionType.None),
    Throws.TypeOf<OperationCanceledException>().After(100, 10));

ThrowIfCancellationRequested() throws exactly OperationCanceledException, so TypeOf is technically correct here. But all 5 other OperationCanceledException assertions in the codebase (ForwardHeaderProviderTests, RateLimiterTests, BuildBlocksOnlyWhenNotProcessingTests, EngineModuleTests.V5) use Throws.InstanceOf. Consider Throws.InstanceOf<OperationCanceledException>().After(100, 10) for consistency.


Non-issues verified in this pass

  • ExceptionAssertExtensions.DoesNotThrow<T> swallows non-T exceptions: This is intentional — it replicates FA's NotThrow<T>() semantics exactly, as documented by NotThrow_passes_when_different_exception_is_thrown test.
  • Branch 1 commit behind master: 54ed9a2 (May 25 15:48) added ABI depth validation in AbiType.cs, AbiTypeConverterDepthTests.cs, and a refactor in ForkchoiceUpdatedHandler.cs. None of these touch the assertions being migrated. A standard GitHub merge preserves all these changes from master. No assertions-specific regression.
  • tools/StatsAnalyzer/.../Builder.cs whitespace change: Harmless noise, same as in prior reviews.
  • tests.props path fix ($(MSBuildThisFileDirectory)): Legitimate portability improvement (noted in prior reviews as acceptable).
  • RlpTrieNodeEncodingBenchmark.cs uses NUnit: Acceptable; the benchmark project was already linked via tests.props and the FluentAssertions import was the only thing removed.

flcl42 and others added 8 commits May 26, 2026 12:20
# Conflicts:
#	src/Nethermind/Nethermind.Blockchain.Test/Tracing/GethStyle/GethLikeTxDirectStreamingTracerHexTests.cs
#	src/Nethermind/Nethermind.Core.Test/Json/ByteArrayConverterTests.cs
#	src/Nethermind/Nethermind.EraE.Test/Proofs/BeaconApiRootsProviderTests.cs
#	src/Nethermind/Nethermind.JsonRpc.Test/JsonRpcLocalStatsTests.cs
#	src/Nethermind/Nethermind.JsonRpc.Test/JsonRpcProcessorTests.cs
#	src/Nethermind/Nethermind.JsonRpc.Test/JsonRpcSocketsClientTests.cs
#	src/Nethermind/Nethermind.JsonRpc.Test/Modules/AdminModuleTests.cs
#	src/Nethermind/Nethermind.JsonRpc.Test/Modules/DebugRpcModuleTests.ExecutionWitness.cs
#	src/Nethermind/Nethermind.JsonRpc.Test/Modules/DebugRpcModuleTests.cs
#	src/Nethermind/Nethermind.JsonRpc.Test/Modules/Eth/EthRpcModuleTests.cs
#	src/Nethermind/Nethermind.JsonRpc.Test/Modules/Eth/Simulate/EthSimulateTestsBlocksAndTransactions.cs
#	src/Nethermind/Nethermind.JsonRpc.Test/Modules/RpcModuleProviderTests.cs
#	src/Nethermind/Nethermind.JsonRpc.Test/Modules/Subscribe/TransactionReceiptsSubscriptionTests.cs
#	src/Nethermind/Nethermind.JsonRpc.Test/Modules/SubscribeModuleTests.cs
#	src/Nethermind/Nethermind.JsonRpc.Test/RpcTest.cs
#	src/Nethermind/Nethermind.Merge.Plugin.Test/EngineModuleTests.V3.cs
#	src/Nethermind/Nethermind.Merge.Plugin.Test/EngineModuleTests.V5.cs
#	src/Nethermind/Nethermind.Merge.Plugin.Test/TestingRpcModuleTests.cs
# Conflicts:
#	src/Nethermind/Nethermind.Blockchain.Test/Filters/AddressFilterTests.cs
#	src/Nethermind/Nethermind.Blockchain.Test/Filters/FilterManagerTests.cs
#	src/Nethermind/Nethermind.Blockchain.Test/Filters/FilterStoreTests.cs
#	src/Nethermind/Nethermind.Blockchain.Test/Filters/LogFilterTests.cs
#	src/Nethermind/Nethermind.JsonRpc.Test/Modules/TraceRpcModuleTests.cs
#	src/Nethermind/Nethermind.JsonRpc.TraceStore.Test/DbPersistingBlockTracerTests.cs
#	src/Nethermind/Nethermind.Synchronization.Test/OldStyleFullSynchronizerTests.cs
LukaszRozmej and others added 3 commits May 27, 2026 07:11
…m master

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
# Conflicts:
#	src/Nethermind/Nethermind.Blockchain.Test/BlockTreeTests.cs
#	src/Nethermind/Nethermind.Network.Discovery.Test/DiscoveryManagerTests.cs
#	src/Nethermind/Nethermind.Network.Discovery.Test/DiscoveryPersistenceManagerTests.cs
#	src/Nethermind/Nethermind.Network.Discovery.Test/NodeLifecycleManagerTests.cs
#	src/Nethermind/Nethermind.Network.Discovery.Test/RoutingTable/NodeBucketItemTests.cs
#	src/Nethermind/Nethermind.Network.Discovery.Test/RoutingTable/NodeBucketTests.cs
Tests added on master use FluentAssertions, which this branch removes.
Convert them to the unified NUnit Assert.That style so the merge builds.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@flcl42 flcl42 merged commit ba5a917 into master May 27, 2026
548 checks passed
@flcl42 flcl42 deleted the assertions branch May 27, 2026 13:41
Marchhill added a commit that referenced this pull request May 27, 2026
Master removed the FluentAssertions package in #11567 ("Unify
assertions"); the FOCIL test files added in this branch still used
`.Should()` and broke the test-project builds after the merge.

Translate each call site to the NUnit `Assert.That(...)` style already
used elsewhere in these projects. No behavioural change; the assertion
semantics line up (Should().Be → Is.EqualTo, NotBeNull → Is.Not.Null,
BeEmpty → Is.Empty, HaveCount → Has.Count.EqualTo, ContainSingle →
Has.Count.EqualTo(1) + indexed check, etc).

Files touched:
- Nethermind.Consensus.Test: InclusionListTxSourceTests,
  InclusionListBlockProducerTxSourceFactoryTests,
  PayloadAttributesPayloadIdTests
- Nethermind.Merge.Plugin.Test: EngineModuleTests.Bogota,
  InclusionListBuilderTests
- Nethermind.Specs.Test: MainnetSpecProviderTests (single Bogota_eips
  call site left over from before the merge)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
LukaszRozmej added a commit to divi2806/nethermind that referenced this pull request May 28, 2026
…ions

Master removed FluentAssertions (NethermindEth#11567 "Unify assertions"); the merged
StateBoundaryStoreTests still used .Should() via Core.Test's transitive
package reference and no longer builds. Convert to Assert.That to match
the unified style.
LukaszRozmej added a commit to 0xDevNinja/nethermind that referenced this pull request May 28, 2026
Master removed FluentAssertions in NethermindEth#11567; this PR's new
PosForwardHeaderProviderCacheTests still used Should() and failed to
compile against the updated test props. Convert to the unified
Assert.That style.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
LukaszRozmej added a commit that referenced this pull request May 29, 2026
…ry (#11438)

* Add eth_capabilities RPC method and related data structures

This commit introduces the `eth_capabilities` method to the Eth RPC module, which returns the node's historical data availability and head block information. It includes the new `EthCapabilitiesResult` class and associated classes (`CapabilityHead`, `CapabilityResource`, and `CapabilityDeleteStrategy`) to structure the response. Additionally, tests for the new functionality have been added to ensure correct behavior across various configurations, including archive and pruned nodes.

* Enhance eth_capabilities method to handle pruning modes and improve state reporting

This commit updates the `eth_capabilities` method in the Eth RPC module to correctly report the availability of blocks and receipts based on the configured pruning mode. It ensures that the `OldestBlock` field is set to null for full pruning scenarios, preventing misleading information. Additionally, tests have been added to verify the behavior of the method across different configurations, including archive nodes and various pruning settings, ensuring compliance with the expected JSON structure.

* Add EthCapabilitiesProvider class and refactor eth_capabilities method

This commit introduces the `EthCapabilitiesProvider` class, which encapsulates the logic for retrieving the capabilities of the Ethereum node, including block and receipt availability based on the current sync and pruning configurations. The `eth_capabilities` method in the `EthRpcModule` is refactored to utilize this new provider, streamlining the code and improving maintainability. Additionally, the interface for the `eth_capabilities` method is updated to include an example response for better clarity in usage.

* Refactor EthCapabilitiesResult and EthCapabilitiesProvider to use long and Hash256 types

This commit updates the `EthCapabilitiesResult` and `EthCapabilitiesProvider` classes to replace string representations of block numbers and hashes with their appropriate types: `long` for block numbers and `Hash256` for block hashes. This change enhances type safety and consistency across the codebase. Additionally, the related tests have been updated to reflect these changes, ensuring accurate validation of capabilities reporting.

* refactor: constructor-based capability records and dedup capability tests

- Convert EthCapabilitiesResult, CapabilityHead, CapabilityResource, and
  CapabilityDeleteStrategy to primary-constructor records; preserve conditional
  JSON omission via [property: JsonIgnore(...)] attribute targeting.
- Update EthCapabilitiesProvider to construct records via constructors instead
  of object initializers.
- De-duplicate EthRpcModuleTests.Capabilities: merge the two default-build
  smoke tests; consolidate archive/full-pruning/no-receipts cases under a
  TestCaseSource that asserts full CapabilityResource equality.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor: introduce IEthCapabilitiesProvider, register via DI, rename types

- Add IEthCapabilitiesProvider interface; register via Autofac (singleton).
- EthRpcModule now takes IEthCapabilitiesProvider instead of constructing it
  from IBlockFinder + sync/pruning configs; OptimismEthRpcModule and the
  factories pass the injected instance through.
- Rename EthCapabilitiesResult -> EthCapabilities, CapabilityHead -> ChainHead,
  CapabilityResource -> ResourceAvailability, CapabilityDeleteStrategy ->
  DeleteStrategy. File renamed accordingly.
- Replace eth_capabilities key-set test with a JSON-schema-based test using
  NJsonSchema (added to JsonRpc.Test). Schema mirrors ethereum/execution-apis#755
  and enforces additionalProperties=false plus required fields and hex patterns.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* feat(eth_capabilities): account for history pruning; alias Tx/Logs to Receipts

- EthCapabilitiesProvider now consumes IHistoryConfig and IHistoryPruner.
  Blocks/Tx/Logs/Receipts.OldestBlock is lifted to historyPruner.OldestBlockHeader
  when history pruning is active, and Rolling mode produces a window DeleteStrategy
  with retention = RetentionEpochs * 32 blocks.
- EthCapabilities.Tx and .Logs become computed properties returning Receipts —
  the three resources share storage and pruning policy in Nethermind today.
  JsonPropertyOrder preserves the spec's canonical key order on the wire; the
  fields can be decoupled later if a future spec change requires it.
- Add Nethermind.History project reference to JsonRpc.
- Fold the three new history-pruning tests into the existing TestCaseSource;
  Blocks is now part of the parametric assertion.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(eth_capabilities): align with spec — integer retentionBlocks; share Resource helper

Reviewed against ethereum/execution-apis#755 schema and test fixtures:

- DeleteStrategy.RetentionBlocks must be a JSON integer per spec
  ("retentionBlocks":90000), but Nethermind's default long converter writes
  hex. Apply [JsonConverter(typeof(LongRawJsonConverter))] to override.
- Tighten the JSON-schema test: retentionBlocks is integer; deleteStrategy.type
  is enum ["window"] (the spec's only variant — others may be added via oneOf).
- Extract a single Resource(enabled, oldestBlock, deleteStrategy?) helper used
  for all four resources. Enforces the spec invariant "disabled: true ⇒ no
  other fields" in one place.
- Promote HistoryPruner.SlotsPerEpoch from private to public; reference it
  from EthCapabilitiesProvider so the constant has a single source of truth.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix: regenerate packages.lock.json after JsonRpc->History reference

CI runs `dotnet restore --locked-mode`; adding the Nethermind.History
project reference to Nethermind.JsonRpc changed the dep graph, which the
locked-mode restore detected as inconsistent with packages.lock.json.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(eth_capabilities): wire state availability through IWorldStateManager

Pruning config only describes the trie-based store; flat-DB state has its own
retention model. Move the state-availability report behind IWorldStateManager
so each implementation reports its own semantics, and address the test review
by collapsing the standalone memory-pruned and json-schema fixtures into a
single mock-based scenario suite plus one end-to-end JSON schema test.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(eth_capabilities): support state proofs within trie pruning window

Trie nodes resolve by hash, so eth_getProof works wherever State works —
not only on archive nodes. Memory/Hybrid/Full trie modes now report
StateProofsSupported = true; only flat-DB (no by-hash lookup) keeps it off.
Stateproofs OldestBlock and DeleteStrategy now mirror State so callers see
the same retention bounds. Comments trimmed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(eth_capabilities): drop StateProofsSupported; flat supports proofs

Flat-DB serves state proofs via RunTreeVisitor over ReadOnlySnapshotBundle —
HashServer is only the snap-server hook, not the proof path. With both
production managers always supporting proofs, the flag is dead. Stateproofs
now mirrors State.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* feat(eth_capabilities): persist OldestStateBlock metadata

Adds an absolute floor for historical state availability, recorded in the
metadata DB and exposed via IBlockTree.OldestStateBlock. It's set:
- after fast/snap sync completes (= pivot block), via a new
  RecordOldestStateBlockOnStateSyncFinished hook on ITreeSync.SyncCompleted;
- after full pruning completes (= copied state's block), in FullPruner.

EthCapabilitiesProvider combines this floor with the rolling window
retention from IWorldStateManager, so a fast-synced archive node now
correctly reports OldestBlock = pivot, and a full-pruned node reports
OldestBlock = last successful prune target.

The Archive flag on StateAvailability is dropped — the floor + window are
sufficient to express all retention shapes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(eth_capabilities): inline StateAvailability as GetOldestStateBlock(head)

Drops the StateAvailability struct (only one field left) and exposes the
manager's contribution as a method returning the oldest block given the
chain head. Mirrors IBlockTree.OldestStateBlock naming. Trie pruning manager
returns max(0, head - PruningBoundary) for memory/hybrid, null otherwise;
flat returns null (its retention is via the IBlockTree.OldestStateBlock
floor). The provider combines this with the floor and derives DeleteStrategy
from the actual span retained.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix: regenerate packages.lock.json with Nethermind.History reference

The post-merge lock file was missing the JsonRpc → History dependency added
on this branch, causing dotnet restore --locked-mode to fail in CI.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(eth_capabilities): use raw barriers; honor body availability; handle no-head

Use min(pivot, AncientBodiesBarrier) and min(pivot, max(bodiesBarrier,
receiptsBarrier)) directly. Drops AncientReceiptsBarrierCalc's Math.Max(1, ...)
clamp, fixing the off-by-one for archive (PivotNumber=0) and fast-sync with
default barriers. Receipts now correctly report oldestBlock=0 from genesis
when nothing is gated.

Blocks resource was reporting LowestInsertedHeader for body availability.
A fast-synced node with non-zero AncientBodiesBarrier or DownloadBodiesInFastSync
=false has headers but no bodies below the cap, so Blocks must reflect bodies:
- lowestBlock floors to the bodies barrier
- enabled = headersAvailable && bodiesSynced
- receiptsSynced now also requires bodiesSynced

Suppress state.deleteStrategy when a higher floor (post-fast-sync pivot, full-
prune mark) dominates the rolling window. Keeps oldestBlock and head-retentionBlocks
internally consistent.

Return all-disabled with head=(0, Keccak.Zero) when blockTree.Head is null
(node warming up, no chain reconstructed yet).

Tests: 4 new scenarios cover fast-sync default barriers, ancient bodies barrier,
disabled bodies, and the no-head path; floor-dominated case now asserts the
suppressed strategy.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(eth_capabilities): use a valid 64-char hash in ExampleResponse

The previous example response contained "0xabc" as the head hash, which is
3 hex chars instead of the 64 the spec schema requires. The ExampleResponse
is documentation-only, but a malformed example breaks any tooling that
parses or validates the auto-generated docs.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(eth_capabilities): gate body/receipt download flags on fast-sync state

DownloadBodiesInFastSync and DownloadReceiptsInFastSync only affect data
availability when the node actually fast-synced. A full-sync node downloads
bodies and receipts as part of forward processing regardless of the flags.
Treating them as authoritative previously caused full-sync nodes that happened
to leave the flags off to falsely report Blocks/Receipts as disabled.

The flags now only disable the resources when FastSync || SnapSync is on.
Tests updated: the receipts/bodies-disabled scenarios are now expressed as
fast-sync configs (which actually exercise the flags), and two new scenarios
pin the full-sync-ignores-the-flag behavior.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(eth_capabilities): note window-vs-floor selection rationale

One-liner explaining why DeleteStrategy is suppressed when the static floor
dominates the rolling window — addresses the relevant claude[bot] review
finding. The two transient-during-sync findings (genesis-state advertised
pre-pivot, Receipts.OldestBlock < Blocks.OldestBlock during header download)
self-resolve once sync completes and don't need code commentary.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(eth_capabilities): track actual sync progress mid-sync

Use ISyncPointers.LowestInserted{Body,Receipt}Number so Blocks.OldestBlock
and Receipts.OldestBlock reflect what the node currently retains, not just
the eventual post-sync barrier. During fast sync the pointers track above
the barrier; once sync completes they collapse onto it.

State and Stateproofs are gated on OldestStateBlock when fast-sync is
configured — until StateSyncRunner finalises and writes the pivot floor we
report State as disabled rather than advertising historical state from
genesis.

Two new scenarios cover the mid-progress and pre-state-finalised cases.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(eth_capabilities): apply review fixes

Provider:
- Required ctor params (drop nullable defaults; tests pass Substitute.For)
- IReadOnlyBlockTree instead of IBlockTree (read-only intent)
- sealed
- Drop misleading head.Hash ?? Keccak.Zero (BlockTree invariant guarantees non-null)
- Inline Resource helper into BuildState/BuildResource/BuildWindow
- Suppress window descriptor when retentionBlocks <= 0 (genesis edge)
- Hoist Disabled to a static field
- Extract BuildState to absorb the multi-line "why" comment

BlockTree:
- OldestStateBlock setter no-op when value unchanged

BlockTreeOverlay:
- OldestStateBlock falls back to base tree (matches Head/Finalized/Safe pattern)

FullPruner:
- CopyTrie returns bool committed; OldestStateBlock write gated on the explicit
  return rather than re-reading the cancellation token

Tests:
- Rename test method to method_scenario shape
- New: BlockTreeTests.OldestStateBlock_round_trips_through_metadata_db

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(eth_capabilities): address review feedback from asdacap & stavros

- Move OldestStateBlock off IBlockFinder: now declared on IBlockTree
  (writer side) and IWorldStateManager (external consumers). BlockTree
  implements a narrow IOldestStateBlockStore so WorldStateManager can
  delegate without taking a Nethermind.Blockchain dependency.
- Gate Stateproofs separately from State via IWorldStateManager.SupportsTrieProofs:
  trie-backed managers return true, FlatWorldStateManager returns false to
  avoid advertising O(state-size) or snapshot-limited proof reconstruction.
- Encapsulate the epoch->blocks conversion behind IHistoryPruner.GetRetentionBlocks;
  SlotsPerEpoch becomes private const again.
- Clarify XML doc: Tx/Logs aliasing Receipts (with LogIndex caveat),
  JsonPropertyOrder is example-payload parity, not a wire requirement.
- Replace Keccak.Zero in ExampleResponse with a realistic 32-byte hash.
- Add CapabilitiesScenario row covering AncientReceiptsBarrier > AncientBodiesBarrier.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(eth_capabilities): own OldestStateBlock persistence in WorldStateManager

- Move the absolute oldest-state-block floor out of BlockTree entirely.
  WorldStateManager now owns the metadata-DB persistence (same key) via a
  small OldestStateBlockStore helper; FlatWorldStateManager does the same.
  IBlockTree/IBlockFinder no longer expose OldestStateBlock.
- Route FullPruner and StateSyncRunner writes through
  IWorldStateManager.OldestStateBlock (proper layering).
- FullPrunerFactory.Create now takes IWorldStateManager (not IStateReader)
  so the pruner can write the floor without going through the block tree.
- Make CopyTrie synchronous (it has no async work; was already returning
  Task.FromResult).
- Reformat multi-arg EthCapabilitiesProvider constructor calls one-per-line.
- Drop IOldestStateBlockStore glue interface no longer needed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(eth_capabilities): alias stateproofs to state again

Flat storage retains trie nodes in path-keyed columns (see
BaseTriePersistence: StateNodesTop, StateNodes, StorageNodes,
FallbackNodes) and serves eth_getProof in O(trie-depth) for any block
where the flat snapshot is present — same complexity as trie storage.
The previous SupportsTrieProofs gate was based on a wrong assumption.
Drop the property; stateproofs availability matches state availability
for both Nethermind backends.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* feat(eth_capabilities): discard stale state-availability markers at startup

Adds a one-shot consistency check at world-state-manager activation:
- If OldestStateBlock points at a block whose state root is not present
  in the state DB, clear it.
- Same for IBlockTree.BestPersistedState.

This handles the scenario where a user wipes the state directory without
touching MetadataDb / BlockInfoDb — without the check, eth_capabilities
would over-report state availability until sync rewrites the floor.

The check uses GlobalStateReader.HasStateForBlock so it's meaningful
(verifies the root, not just "is the DB empty"). Wired via
OnActivate<IWorldStateManager> in both WorldStateModule and
FlatWorldStateModule.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(state-metadata): cover stale-floor discard at startup

Move StateMetadataValidator from Nethermind.Init to Nethermind.Blockchain
(where IBlockTree lives) so it can be tested in Nethermind.Blockchain.Test.
Use explicit BlockTreeLookupOptions overload so NSubstitute can intercept
the FindHeader call without going through the default interface method.

Tests cover:
- OldestStateBlock cleared when state root missing
- OldestStateBlock kept when state root present
- OldestStateBlock kept when header unknown (defensive)
- No-op when OldestStateBlock is null
- BestPersistedState clear/keep symmetric to OldestStateBlock
- Markers independent (one stale, other ok)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(eth_capabilities): delete metadata key when OldestStateBlock is cleared

- Address claude-bot review (pass 6): OldestStateBlockStore setter now
  deletes the metadata DB entry on null, so a stale floor isn't reloaded
  on every restart after StateMetadataValidator clears it.
- Remove unused `using Nethermind.Blockchain.Find;` from
  StateMetadataValidator and its tests — BlockTreeLookupOptions lives in
  Nethermind.Blockchain. Fixes IDE0005 lint failures.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(eth_capabilities): guard descending-pointer pre-insert state and stabilize retentionBlocks

- Block/Receipt resources are now Disabled while fast sync is configured to download but the
  descending pointer is still null. Reporting the barrier (eventual floor) as oldestBlock
  before any insertion over-claimed availability.
- Replace IWorldStateManager.GetOldestStateBlock(head) with RetentionWindowBlocks. Provider
  derives windowOldest itself and emits the configured retention so retentionBlocks stays
  correct when head < retention (fresh chain) instead of rounding down to head.
- Lock OldestStateBlockStore reads/writes: long? is two fields, the RPC path reads concurrently
  with sync runners and the full pruner.
- Add a regression-scenario test for the pre-batch fast-sync window and update existing
  scenarios to set descending pointers consistent with their post-sync framing.

* refactor(eth_capabilities): narrow OldestStateBlockStore deps, dedup tests

- Promote OldestStateBlockStore to a DI singleton (registered in BlockTreeModule
  via IDbProvider.MetadataDb). FullPruner, StateSyncRunner, EthCapabilitiesProvider,
  StateMetadataValidator now depend on the narrow store instead of IWorldStateManager.
  Removes OldestStateBlock from IWorldStateManager.
- Drop IDbProvider from FlatWorldStateManager (the store is injected directly).
- Extract MetadataLongStore reusable base class taking IDb + key; OldestStateBlockStore
  becomes a thin sealed wrapper hardcoding MetadataDbKeys.OldestStateBlock.
- IFullPrunerFactory.Create signature: IWorldStateManager → IStateReader (mirrors
  what FullPruner actually needs); factory pulls the store via DI.
- IsDescendingResourceDownloaded as single expression.
- Drop the stateproofs-aliases-state comment.
- Deduplicate StateMetadataValidatorTests with [TestCase] parameterization
  across (Marker × initial × StateAt × shouldClear); test count: 9 (8 cases + 1
  independent-markers).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(eth_capabilities): IStateBoundary on IWorldStateManager; per-backend state-DB-resident storage

Addresses @asdacap review on `fabd507183`:
- Re-add `OldestStateBlock` (+ `RetentionWindowBlocks`) on a new narrow
  `IStateBoundary` interface; `IWorldStateManager : IStateBoundary`.
- Trie `WorldStateManager`: floor persists in `dbProvider.StateDb` under
  `Keccak("OldestStateBlock")` (collision-free vs 32-byte hash keys and
  HalfPath section bytes 0/1/2).
- `FlatWorldStateManager`: floor persists in `FlatDbColumns.Metadata` under
  the same keccak key, alongside the existing `CurrentState`/`Layout` keys.
- Co-locating the floor with state nodes means wiping the state directory
  drops the floor automatically — no startup validator needed.
- Drop `StateMetadataValidator` (+ tests), `OldestStateBlockStore`,
  `MetadataLongStore`, `MetadataDbKeys.OldestStateBlock` (the metadata-DB
  storage was never deployed), and the duplicated `OnActivate` validator
  wiring in both world-state modules.
- `FullPruner` / `StateSyncRunner` / `EthCapabilitiesProvider` depend on
  the narrow `IStateBoundary` (auto-resolved via `IWorldStateManager`).
- `IFullPrunerFactory.Create` signature returns to `IWorldStateManager`.
- Capabilities test mock simplifies to a single `IStateBoundary` substitute.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor: move IStateBoundary into its own file

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(eth_capabilities): expose IWorldStateManager as IStateBoundary in DI

EthCapabilitiesProvider depends on IStateBoundary, but no module
registered that service — Autofac does not auto-resolve base interfaces
for factory-registered singletons. Add an explicit Map so the manager
is resolvable through both contracts. Surfaced by
RpcModuleProviderTests.ModuleFactory_FromDI_IsLazy.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(eth_capabilities): rename CopyTrie to TryCopyTrie

Now that the method returns bool indicating whether the prune committed,
the Try-prefix matches the standard .NET convention and makes the
caller's `if (TryCopyTrie(...))` branch self-documenting.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(full-pruning): account for OldestStateBlock metadata in subset check

FullPruner now records the state-availability floor in the new state DB
on a successful prune, so the post-prune snapshot legitimately contains
one entry that's absent from the pre-prune snapshot. Verify the boundary
key was written (closing a coverage gap for the FullPruner → IStateBoundary
write), then exclude it before asserting the rest of the contents are a
subset of the pre-prune values.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(eth_capabilities): reject backward OldestStateBlock writes

The floor is meant to be monotonically non-decreasing: state sync writes
the pivot, then full pruning writes the (later) copied block. The setter
now rejects backward non-null writes so a stale caller can't regress the
reported availability. Null reset remains allowed for explicit recovery
(e.g. wiping a corrupt state DB).

Also adds StateBoundaryStoreTests covering initial-null read, round-trip
persistence, idempotent re-set, the monotonic guard, and the null-reset
escape hatch.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(history-pruner): cover GetRetentionBlocks contract

Locks in the epochs→blocks conversion (×32) so future refactors can't
silently change the slot-per-epoch constant without test coverage.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(eth_capabilities): persist before caching in StateBoundaryStore

If the kv write threw after _value had already been overwritten, the
in-memory snapshot would diverge from disk until the next forward write
or process restart. Swap the order so a failed persistence leaves both
caches at the previous value.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(eth_capabilities): split IStateBoundary into reader and writer

EthCapabilitiesProvider only needs to read the floor; StateSyncRunner and
FullPruner are the only legitimate writers. Splitting the contract makes
the write surface explicit instead of leaking through every consumer of
IWorldStateManager, which still implements both halves. Also promotes
StateBoundaryStore.OldestStateBlockKey to internal so the full-pruning
disk test can reference the key constant directly rather than recomputing
the keccak inline.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(eth_capabilities): trim redundant comments, dedup boundary tests

- Drop inline comments restating what IStateBoundary's XML doc already
  says (co-location, "no rolling window for flat", etc.).
- Compress EthCapabilities docs by moving the Tx/Logs aliasing rationale
  into <remarks> and tightening to one sentence.
- Drop the implementation-detail tail of the RetentionBlocks param doc.
- Collapse the three single-action StateBoundaryStore tests
  (forward/backward/equal) into one [TestCase]-parameterized test.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(eth_capabilities): address asdacap review on boundary plumbing

- IStateBoundaryWriter is trie-specific: it's no longer on IWorldStateManager.
  Only the trie WorldStateManager implements it; FlatWorldStateManager exposes
  only the reader half.
- Move the OldestStateBlock write from StateSyncRunner (wrong layer — sync
  runner orchestrates rounds and shouldn't know about the floor) to
  PatriciaTreeSyncStore.FinalizeSync, which is the trie-specific
  finalization hook that already has the pivot header.
- FlatWorldStateManager no longer owns a StateBoundaryStore. Flat state
  tracking lives in PersistenceManager already, so OldestStateBlock just
  reads CurrentPersistedStateId.BlockNumber from there. No setter on flat.
- DI: WorldStateModule keeps the IStateBoundary map (works for both
  backends since IWorldStateManager : IStateBoundary). IStateBoundaryWriter
  moves to PruningTrieStoreModule, mapped from the trie factory output so
  it stays unresolved when flat is the active backend.
- FullPruner's worldStateManager argument is cast to IStateBoundaryWriter
  at the FullPrunerFactory boundary (safe — full pruning is trie-only).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs(state-boundary): correct StateBoundaryStore doc after flat split

The store now serves only the trie backend — flat reads from
PersistenceManager directly — so the dual-backend wording was misleading.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(state-boundary): convert StateBoundaryStoreTests to NUnit assertions

Master removed FluentAssertions (#11567 "Unify assertions"); the merged
StateBoundaryStoreTests still used .Should() via Core.Test's transitive
package reference and no longer builds. Convert to Assert.That to match
the unified style.

* fix(eth_capabilities): address review feedback from svlachakis

- Restore #11811 KBucketTree.cs early-return guards and Trace level reverted by stale rebase
- Make FullPruner OldestStateBlock advance atomic with DB swap: write the marker before pruning.Commit() so FullPruningDb mirrors it into the cloning DB
- Drop unsafe (IStateBoundaryWriter) cast in FullPrunerFactory by injecting IStateBoundaryWriter from DI
- Log backward OldestStateBlock writes at Warn instead of dropping them silently
- Restructure BuildState with single early-return to remove the redundant retention null check
- Restore ReadOnlyBlockTree.cs blank line removed in rebase
- Rename Eth_capabilities_* tests to match the project's PascalCase prefix convention

* fix(eth_capabilities): restore IStateBoundaryWriter cast in FullPrunerFactory

Injecting IStateBoundaryWriter created a DI cycle: PruningTrieStateFactory
→ FullPrunerFactory → IStateBoundaryWriter (mapped from
PruningTrieStateFactoryOutput, which is built by PruningTrieStateFactory).
Revert to the runtime cast — it is safe in practice because Create only
runs for trie storage, which is always WorldStateManager.

---------

Co-authored-by: lukasz.rozmej <lukasz.rozmej@gmail.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
LukaszRozmej added a commit that referenced this pull request Jun 2, 2026
)

* perf(merge-sync): cache PosForwardHeaderProvider.GetBlockHeaders

Fetches a 4x batch from ChainLevelHelper, slices per-call, and serves
from cache while BeaconPivot.ProcessDestination is unchanged and the
desired start sits inside the cached range. Closes #8573.

* fix(merge-sync): preserve anchor block on PosForwardHeaderProvider cache hit

`ChainLevelHelper.GetStartingPoint` returns the anchor at `BestKnownNumber`
in the active beacon-sync walk-back path, so the cache stores the anchor at
index 0. Using `BestKnownNumber + 1` as the serve start dropped that anchor
on every cache hit, breaking `BlockDownloader.AssembleRequest` which relies
on `headers[0]` as the parent header.

Additional fixes:
- Forward `skipLastN` to `ChainLevelHelper` and only cache full-sized
  batches, restoring the original chain-tip exclusion semantics.
- Re-run `ValidateSeals` on each served slice so terminal-block and
  random-index checks are not frozen at fill time.

Tests updated so the mock models the walk-back path (`start: 0` matching
`BestKnownNumber = 0`); the assertion `second[0].Number == 20` now exercises
the anchor inclusion that previously slipped through the off-by-one.

* perf(merge-sync): address PR #11617 review nits

- Hoist MinCachedHeaderBatchSize from PosForwardHeaderProvider to
  PowForwardHeaderProvider (protected const), removing the duplicate.
- Drop unreachable `cached.Length == 0` guard in TryServeFromCache;
  UpdateCache only stores arrays >= MinCachedHeaderBatchSize, so the
  null check alone suffices.
- Eliminate double allocation on cache hit by making TryServeFromCache
  populate an ArrayPoolList directly and return it, instead of
  allocating BlockHeader[] then a second time via ToPooledList.

* fix(merge-sync): dispose slice on validation throw, invalidate cache on intra-range reorg

- Wrap the per-slice `ValidateSeals` call in try/catch so the `ArrayPoolList`
  is returned to the pool on cancellation/seal-validator exceptions instead
  of leaking the underlying buffer.
- Subscribe to `BlockTree.OnUpdateMainChain` and drop the cache whenever the
  first block of a main-chain update has a different hash than the cached
  header at the same number — catches reorgs that don't change
  `BeaconPivot.ProcessDestination`. Required converting the primary
  constructor to an explicit one and implementing `IDisposable` so the
  subscription is released for non-singleton consumers (tests).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(merge-sync): dedup, dispose owned lists, strengthen SkipLastN, cover reorg invalidation

- Dispose every returned `IOwnedReadOnlyList` via `using` so the pooled
  buffers are returned to `ArrayPool.Shared`.
- Extract `Get`, `RaiseMainChainUpdate`, and `AssertChainLevelCalls` helpers
  to remove the per-test boilerplate around the common cache-hit/miss pattern.
- `SkipLastN_is_honored_on_cache_hit`: request `CachedBatchSize` headers so
  the `min(available, maxHeader)` front-cap actually exposes `skipLastN`
  (previous `Requested = 16` made the test pass even if `skipLastN` was
  silently dropped) and assert exact count plus tail header number.
- Add `Cache_is_invalidated_on_reorg_within_cached_range` and
  `Cache_survives_main_chain_update_that_matches_cached_hash` to cover the
  new `OnUpdateMainChain` invalidation path.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* test(merge-sync): convert FluentAssertions to NUnit Assert.That

Master removed FluentAssertions in #11567; this PR's new
PosForwardHeaderProviderCacheTests still used Should() and failed to
compile against the updated test props. Convert to the unified
Assert.That style.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(merge-sync): widen reorg invalidation, key cache on skipLastN

- `BlockTreeOnUpdateMainChain` now scans the event for any block in the
  cached range instead of inspecting only `Blocks[0]`. `UpdateMainChain`
  can fire in either ascending or descending order, and reorgs may start
  below `cacheStart` and only extend into the cache from there — the old
  shape returned early in both situations and left stale headers in the
  cache.
- Track `skipLastN` per cache entry and invalidate when it changes; the
  cached array is the result of `ChainLevelHelper.GetNextHeaders` with a
  specific `skipLastBlockCount`, so reusing it for a different
  `skipLastN` would double-trim from the tail.
- Drop the dead `skipLastN` parameter from the cache-miss slice builder
  (it was always called with 0) and inline the slice.
- Replace the per-element copy loop with `ArrayPoolList.AddRange(span)`.
- Drop the redundant `headers.Length < MinCachedHeaderBatchSize` guard
  in `UpdateCache` — the single caller already gates on `fresh.Length
  >= fetchSize` where `fetchSize >= MinCachedHeaderBatchSize`.
- Dedup tests via a small `ExpectCalls` helper; add tests for the new
  invalidation paths (skipLastN change, ascending-order reorg starting
  below cache, descending-order reorg with top above cache).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(merge-sync): drop unused Nethermind.Core.Extensions using

`ToPooledList` was the only call site that needed it; the inlined
cache-miss slice no longer uses extension methods from that namespace.
Triggers IDE0005 in CI lint job.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(merge-sync): tighten PosForwardHeaderProvider cache contract

- Replace IDisposable with internal UnsubscribeForTest(); the singleton
  is never disposed in production, the subscription is intentionally
  permanent, and IDisposable falsely advertises managed lifetime.
- Move the IBeaconPivot.ProcessDestination read inside _cacheLock so the
  cache snapshot and the pivot comparison happen in one critical section
  (closes the TOCTOU window between snapshot release and pivot read).
- Switch both ArrayPoolList allocations to the ReadOnlySpan<T> ctor;
  rents the buffer and copies in one step instead of new() + AddRange().
- Add a precondition message on the reorg-cache test assertion.
- One-line note on the BlockTreeOnUpdateMainChain early return:
  first in-range block's hash commits to all earlier ones, no need to
  scan further (the early return is otherwise easy to misread).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: lukasz.rozmej <lukasz.rozmej@gmail.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: stavrosvl7 <stavrosvl7@gmail.com>
LukaszRozmej added a commit to peter941221/nethermind-peter that referenced this pull request Jun 9, 2026
Master removed the FluentAssertions.Json reference from JsonRpc.Test
(see PR NethermindEth#11567 "Unify assertions"), so the new .Should() calls in
this branch break the merge build. Convert them to Assert.That to
match the rest of the file post-merge.
LukaszRozmej added a commit to peter941221/nethermind-peter that referenced this pull request Jun 9, 2026
Master removed the FluentAssertions.Json reference from JsonRpc.Test
(see PR NethermindEth#11567 "Unify assertions"), so the new .Should() calls in
this branch break the merge build. Convert them to Assert.That to
match the rest of the file post-merge.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants