Unify assertions#11567
Conversation
|
Claude finished @flcl42's task in 9m 23s —— View job PR Review: Unify Assertions
SummaryThis PR removes Medium1. 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)
2. 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);
3. Custom 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 Low4. 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 5. 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));
6. Duplicated assertion helpers in two test files
These should be extracted to a shared test utility to reduce duplication. Non-issues (verified)
|
# 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
# Conflicts: # src/Nethermind/Nethermind.State.Flat.Test/Persistence/BloomFilter/BloomFilterTests.cs # src/Nethermind/Nethermind.State.Test/StateReaderTests.cs
|
@claude review deeply again |
|
Claude finished @flcl42's task in 11m 29s —— View job PR Review: Unify Assertions (Deep Review #4)
SummaryFourth 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 ✅
New helper
Medium —
|
# 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
…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>
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>
…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.
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>
…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>
) * 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>
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.
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.
Changes
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?