Updated blockchainBridge hasStateForBlock to use config instead of st… #9457

mrzeszutko · 2025-10-14T11:43:39Z

…ate. Relates to #9408

Changes

Updated BlockChainBridge. HasStateForBlock to check the configuration and not call the state at all. The result is calculated based on the node type: archive node vs. full node, the information if we finish the initial sync and pruning boundary.
To cater for the race condition, we return false at the pruning boundary (So we return that we don't have state when we are at the pruning boundary) - this is a safety margin

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

Documentation

Remarks

There were no changes done to the snap serving. 128 is hardcoded there so there should be no impact

…ate. Relates to #9408

src/Nethermind/Nethermind.Facade/IBlockchainBridge.cs

src/Nethermind/Nethermind.Init/PruningTrieStateFactory.cs

src/Nethermind/Nethermind.Facade/BlockchainBridge.cs

src/Nethermind/Nethermind.Init/PruningTrieStateFactory.cs

…k numbers

src/Nethermind/Nethermind.Facade/BlockchainBridge.cs

LukaszRozmej · 2025-10-17T10:42:34Z

src/Nethermind/Nethermind.Trie/Pruning/ITrieStore.cs

+
+        /// <summary>
+        /// Gets the block number of the last persisted block.
+        /// Used to track the oldest available state after full pruning operations.
+        /// </summary>
+        long LastPersistedBlockNumber { get; }


Are you sure this is the OLDEST persisted block number or just LAST (newest) persisted block number?
Not entirely sure if it solves what we expect it does.
@asdacap can you advise?

I removed this

LukaszRozmej · 2025-10-17T10:45:40Z

src/Nethermind/Nethermind.Facade/BlockchainBridge.cs

+            {
+                // Archive mode: but state only available from sync pivot onwards
+                // If node was snap/fast synced, state before pivot was never downloaded
+                long syncPivotBlock = blockTree.SyncPivot.BlockNumber;


Are we sure BlockTree.SyncPivot is reliable data here?
Is it 0 for full archive? Is it 1st block with state for archive with snap sync?
@asdacap can you advise?

SyncPivot moves nowadays. So no.

I removed SyncPivot dependency

LukaszRozmej · 2025-10-17T10:46:52Z

src/Nethermind/Nethermind.Facade/BlockchainBridge.cs

+                // 3. Current pruning window (head - boundary)
+                long lastPersistedBlock = pruningTrieStore.LastPersistedBlockNumber;
+                long syncPivotBlock = blockTree.SyncPivot.BlockNumber;
+                long pruningWindowStart = headNumber - pruningConfig.PruningBoundary + 1; // +1 for safety margin


this calculation is used in both if and else path, let's name it as local method? Or do it before if and name correctly.

not relevant anymore after my last changes

asdacap · 2025-10-17T10:53:50Z

I think the fundamental issue is that 'HasStateForBlockcheck inIWorldStateandIStateReader` is not correct. I was kinda not correct as the existance of the root node does not necessarily mean that the whole trie is available. Its also weird that the root node is not pruned randomly.

asdacap · 2025-10-17T11:07:10Z

Also, checkout LastNStateRootTracker.

LukaszRozmej · 2025-10-17T11:14:47Z

I think the fundamental issue is that 'HasStateForBlockcheck inIWorldStateandIStateReader` is not correct. I was kinda not correct as the existance of the root node does not necessarily mean that the whole trie is available. Its also weird that the root node is not pruned randomly.

Why the existance of root node doesn't imply the existence of the trie?
That was basics of Trie in the past that we only persisted parent node if all it's children were persisted.

asdacap · 2025-10-17T12:00:57Z

Its children might get removed during in memory pruning. Thats why. Some of the nodes may not get removed due to memory limitation for tracking.

LukaszRozmej · 2025-10-17T12:23:08Z

Its children might get removed during in memory pruning. Thats why. Some of the nodes may not get removed due to memory limitation for tracking.

Sure but that is deviation from previous assumptions that were commonly used (like in RPC) and to be honest poses a potential for hard to track bugs (though might be necessary for better pruning)

mrzeszutko · 2025-10-20T11:31:32Z

I think the fundamental issue is that 'HasStateForBlockcheck inIWorldStateandIStateReader` is not correct. I was kinda not correct as the existance of the root node does not necessarily mean that the whole trie is available. Its also weird that the root node is not pruned randomly.

@asdacap is this relevant for this PR? For RPC calls we don't want to reference state, just base the response on config (e.g. pruning boundary etc)

LukaszRozmej · 2025-10-20T11:44:16Z

src/Nethermind/Nethermind.Facade/BlockchainBridge.cs

+                // For archive nodes that were snap synced, check the lowest inserted header
+                // We use LowestInsertedHeader (not SyncPivot) because SyncPivot moves forward over time
+                // as finalized blocks progress, while LowestInsertedHeader remains at the original starting point
+                long? lowestInsertedBlockNumber = blockTree.LowestInsertedHeader?.Number;


This is false, we always have all headers.
We will probably always have all block bodies too.

LukaszRozmej · 2025-10-20T11:44:57Z

src/Nethermind/Nethermind.Facade/BlockchainBridge.cs

+            // Pruning nodes: calculate oldest available state based on pruning boundary
+            // This is conservative but correct for all pruning node configurations:
+            // - Memory pruning: respects the configured boundary
+            // - Full pruning: also uses pruning boundary (persisted state within this window)


I think it doesn't work for full pruning only like this

asdacap · 2025-10-21T01:00:09Z

just base the response on config (e.g. pruning boundary etc)

The RPC consult the world state if a state is available or not. The world state is faulty. IMO this check is important especially for triestore as during memory pruning (around 1 to 2 second on mainnet probably every 30 minute), nodes are concurrently removed, so it definitely need to check config like what you did. However, think carefully, now it uses pruning boundary, later its gonna respect finalized block, later again its gonna have special archive node that only have last 1 mil block or something, do you really want to have all that logic in BlockchainBridge? Consider putting it in IWorldStateManager or IStateReader instead. Except for the sync check part. I guess that make sense here.

asdacap · 2025-10-21T03:14:33Z

src/Nethermind/Nethermind.Init/Modules/WorldStateModule.cs


            .Map<IWorldStateManager, PruningTrieStateFactoryOutput>((o) => o.WorldStateManager)
            .Map<IStateReader, IWorldStateManager>((m) => m.GlobalStateReader)
+            .Map<IPruningTrieStore, MainPruningTrieStoreFactory>((f) => f.PruningTrieStore)


I think you dont need this anymore. Please dont use IPruningTrieStore. It will make migrating to other world state layout harder.

asdacap · 2025-10-21T03:17:33Z

I think you can simply check for IBlockTree.BestPersistedState, which is kinda the same as TrieStore.LastPersistedBlockNumber. It should already have pruning boundary considered. Except for archive mode, then just dont check.

LukaszRozmej · 2025-10-21T06:56:50Z

just base the response on config (e.g. pruning boundary etc)
The RPC consult the world state if a state is available or not. The world state is faulty. IMO this check is important especially for triestore as during memory pruning (around 1 to 2 second on mainnet probably every 30 minute), nodes are concurrently removed, so it definitely need to check config like what you did. However, think carefully, now it uses pruning boundary, later its gonna respect finalized block, later again its gonna have special archive node that only have last 1 mil block or something, do you really want to have all that logic in BlockchainBridge? Consider putting it in IWorldStateManager or IStateReader instead. Except for the sync check part. I guess that make sense here.

I would consider putting it in IPruningStrategy

MarekM25 · 2025-10-23T08:44:42Z

src/Nethermind/Nethermind.Facade/BlockchainBridge.cs

+
+            // Conservative: don't claim we have state during state sync
+            // This prevents race conditions during the sync process for all node types
+            if (ethSyncingInfo.SyncMode.HaveNotSyncedStateYet())


This can be incorrect in edge case - archive node, starting from scratch, archive node should always have state

Updated blockchainBridge hasStateForBlock to use config instead of st…

9ca6a20

…ate. Relates to #9408

mrzeszutko requested review from LukaszRozmej, Marchhill and MarekM25 October 14, 2025 11:44

LukaszRozmej requested changes Oct 14, 2025

View reviewed changes

mrzeszutko added 3 commits October 14, 2025 16:52

Fixing failing tests

accaf8a

Changing IsSyncing to state sync check for pruning enabled

faf1519

Fixing comment - moving details into remarks section

15947c4

tanishqjasoria requested changes Oct 15, 2025

View reviewed changes

src/Nethermind/Nethermind.Init/PruningTrieStateFactory.cs Outdated Show resolved Hide resolved

mrzeszutko added 4 commits October 15, 2025 12:37

Merge branch 'master' into feature/config-based-state-check-for-rpc

5289012

Merge branch 'master' into feature/config-based-state-check-for-rpc

c34092c

Fixing the logic for the archive node - no sync state check just bloc…

6355451

…k numbers

Removing automatic pruning boundary change

eab55ec

mrzeszutko marked this pull request as ready for review October 16, 2025 14:49

mrzeszutko requested review from LukaszRozmej, asdacap and tanishqjasoria October 16, 2025 14:49

LukaszRozmej reviewed Oct 16, 2025

View reviewed changes

src/Nethermind/Nethermind.Facade/BlockchainBridge.cs Outdated Show resolved Hide resolved

src/Nethermind/Nethermind.Facade/BlockchainBridge.cs Outdated Show resolved Hide resolved

mrzeszutko added 3 commits October 17, 2025 09:40

Removing nullability from HasStateForBlock

cbd823d

Handling additional edge cases

2952565

Fixing tests

1006a42

mrzeszutko requested a review from LukaszRozmej October 17, 2025 10:43

LukaszRozmej reviewed Oct 17, 2025

View reviewed changes

Removing SyncPivot dependency

4d4a52e

mrzeszutko requested a review from LukaszRozmej October 20, 2025 11:31

LukaszRozmej requested changes Oct 20, 2025

View reviewed changes

asdacap reviewed Oct 21, 2025

View reviewed changes

LukaszRozmej marked this pull request as draft October 22, 2025 21:18

MarekM25 reviewed Oct 23, 2025

View reviewed changes

Updated blockchainBridge hasStateForBlock to use config instead of st… #9457

Are you sure you want to change the base?

Updated blockchainBridge hasStateForBlock to use config instead of st… #9457

Uh oh!

Conversation

mrzeszutko commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Types of changes

What types of changes does your code introduce?

Testing

Requires testing

If yes, did you write tests?

Documentation

Remarks

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asdacap commented Oct 17, 2025

Uh oh!

asdacap commented Oct 17, 2025

Uh oh!

LukaszRozmej commented Oct 17, 2025

Uh oh!

asdacap commented Oct 17, 2025

Uh oh!

LukaszRozmej commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrzeszutko commented Oct 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asdacap commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asdacap commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LukaszRozmej commented Oct 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mrzeszutko commented Oct 14, 2025 •

edited

Loading

LukaszRozmej commented Oct 17, 2025 •

edited

Loading

asdacap commented Oct 21, 2025 •

edited

Loading

asdacap commented Oct 21, 2025 •

edited

Loading