feat: Add a provider to persist additional nodes to disk during full pruning and shutdown

[hudem1]

Needed for arbitrum's validators.

The blocks being validated might lag behind the tip of the chain and therefore:

• full pruning : when (online) full pruning occurs, we should keep some needed states for the validator to continue generating execution witnesses for blocks (behind the ones to be kept after full pruning) to send them to nitro which validates them. Otherwise regular full pruning will just keep very recent blocks and execution witness generation for arbitrum validators will throw some missing RLP nodes.
• shutdown: we also want to persist some additional states (the last validated state) on shutdown so that on restart, the validator has a recent available state and less states to reconstruct before being able to continue his validation job.

Types of changes

What types of changes does your code introduce?

• Bugfix (a non-breaking change that fixes an issue)
• New feature (a non-breaking change that adds functionality)
• Breaking change (a change that causes existing functionality not to work as expected)
• Optimization
• Refactoring
• Documentation update
• Build-related changes
• Other: Description

Testing

Requires testing

• Yes
• No

If yes, did you write tests?

• Yes
• No

Notes on testing

Optional. Remove if not applicable.

Documentation

Requires documentation update

• Yes
• No

If yes, link the PR to the docs update or the issue with the details labeled docs. Remove if not applicable.

Requires explanation in Release Notes

• Yes
• No

If yes, fill in the details here. Remove if not applicable.

Remarks

Optional. Remove if not applicable.

[svlachakis]

@claude review

[claude]

Claude finished @svlachakis's task in 1m 43s —— View job

---

Code Review

• Read changed files
• Read relevant rules
• Analyze and post review

---

The PR adds a hook (IAdditionalRootsProvider) for external components to persist additional trie nodes during full pruning and shutdown — e.g., to preserve an Arbitrum validator's active state range. The idea is sound and the implementation is mostly clean, but there are a few issues worth addressing.

---

Ordering issue in CopyTrie (FullPruner.cs:279)

AdditionalRootsProvider?.CopyAdditionalStatesToNodeStorage(...) is called before visitor.Finish() and before the cancellation check:

// line 279 — additional copy runs first
AdditionalRootsProvider?.CopyAdditionalStatesToNodeStorage(targetNodeStorage, baseBlock.Number);

// line 281 — cancellation is checked only afterwards
if (!cancellationToken.IsCancellationRequested)
{
    visitor.Finish();   // flushes buffered visitor writes
    ...
    pruning.Commit();
}

Two concerns here:

1. If cancellation is requested, the additional copy still runs and then pruning is aborted — wasted writes and potentially surprising behavior.
2. The additional copy runs before visitor.Finish() which flushes the visitor's buffered writes. Depending on the storage implementation, this could be benign, but it's a subtle coupling. Moving the call to inside the if (!cancellationToken.IsCancellationRequested) block, after visitor.Finish(), would be cleaner and correct.

Fix this →

---

Setter injection is an anti-pattern here

Both FullPruner.AdditionalRootsProvider and TrieStore.AdditionalRootsProvider are public settable properties. The project's DI patterns prefer constructor injection to avoid silent missing-wiring bugs and race conditions.

Since IAdditionalRootsProvider is optional (for Arbitrum only), the idiomatic approach would be to accept IAdditionalRootsProvider? as a constructor parameter (defaulting to null) in both FullPruner and TrieStore, and register the Arbitrum implementation in a module. Even if you want to keep it as a property for plugin flexibility, it should be noted that this bypasses the DI container entirely and the Arbitrum plugin must manually locate the FullPruner and TrieStore instances to set the property.

---

upToBlockNumberExclusive naming is misleading

The parameter name says "exclusive" but the XML doc says "notify the base block from which to keep states", and the call site passes baseBlock.Number — the actual state being pruned to, not an exclusive upper bound. Implementors of IAdditionalRootsProvider will be confused: does "exclusive" mean they should keep states for block < upToBlockNumberExclusive, or that upToBlockNumberExclusive is the base block and they should keep states at or above it?

The name should match the semantics. If it means "the pruning base block number (everything before this will be pruned)", name it pruningBaseBlockNumber or baseBlockNumber. Fix this →

---

Missing tests

The PR marks "Requires testing: No", but this is new behavior (hook invoked during full pruning and shutdown) with non-trivial sequencing. At minimum:

• A test verifying AdditionalRootsProvider.CopyAdditionalStatesToNodeStorage is called during CopyTrie with the correct block number.
• A test verifying it is not called if pruning is cancelled.
• A test verifying it is called during PersistOnShutdown.

FullPrunerTests.cs already tests the pruning pipeline and would be the natural home for these.

---

Minor: MainPruningTrieStoreFactory.FullPruner is only used to wire AdditionalRootsProvider

// PruningTrieStateFactory.cs:164
mainPruningTrieStoreFactory.FullPruner = pruner;

This exposes the FullPruner instance purely so the Arbitrum plugin can set AdditionalRootsProvider on it. If the DI approach is fixed (constructor injection), this property won't be needed.

---

Summary

#	Severity	Issue
1	Bug	AdditionalRootsProvider runs even when cancellation is requested; runs before visitor.Finish()
2	Medium	Setter injection bypasses DI; fragile if not wired
3	Medium	upToBlockNumberExclusive name contradicts documented semantics
4	Low	No tests for the new hook invocation
5	Low	FullPruner property on MainPruningTrieStoreFactory only exists to work around setter injection

[LukaszRozmej]

please target claude review issues

[LukaszRozmej]

Wouldn't it be simpler to provide to FullPrunner a decorator over IStateReader and just run the copy in the RunTreeVisitor?

It is a bit side-effecty, but the extension point is already there, no need to change anything on the Nethermind side if possible - everything goes into the plugin

[hudem1]

yes true, that looks cleaner overall.
I updated the PR, still requires very small change in nethermind side because full pruner gets the state reader after manually constructed and not from DI, as the state reader gets registered in DI right after PruningTrieStateFactory.Build() finishes.

I was also wondering about performances because CopyTree implies: FindCachedOrUnknown miss + LoadRlp + full RLP decode into TrieNode, whereas my initial solution directly stored the node's byte[], but actually i think i could come up with a solution where full pruning requires only the storage of 1-2 additional states rather than potentially many states as in the original solution. So, this solution's slight per-node overhead is negligible and doesn’t offset the cleaner design !

(And i could find an alternative for persisting additional states on shutdown fully in the plugin)

[LukaszRozmej]

It's ok, not sure if we can do better.
@asdacap any better ideas how to supply a decorator of IStateReader here?

[asdacap]

Can you show the implementation? And how does it effect FullPruner?

[hudem1]

Can you show the implementation? And how does it effect FullPruner?

@asdacap Here is how i use it: NethermindEth/nethermind-arbitrum#792

ValidatorStatePreservingStateReader is used as the FullPrunerStateReaderFactory, and uses a custom ITrieStore (ReconstructedStateTrieStore) to copy an additional block's state's trie nodes from