Evict non-validator replicated blocks from the block cache once finalized - #516
Draft
samliok wants to merge 1 commit into
Draft
Evict non-validator replicated blocks from the block cache once finalized#516samliok wants to merge 1 commit into
samliok wants to merge 1 commit into
Conversation
…finalized The non-validator's storage wrapped the raw storage instead of the block cache, so blocks cached at verification were never evicted at indexing. The cache grew with every replicated block and served finalized blocks without their finalization.
yacovm
reviewed
Aug 14, 2026
| epoch: epochNum, | ||
| Storage: i.Config.Storage, | ||
| epoch: epochNum, | ||
| // Index through the cache so blocks inserted upon verification are evicted once finalized. |
Collaborator
There was a problem hiding this comment.
// Index through the cache so blocks inserted upon verification are evicted once finalized.
This is a weird place to place this comment.
I think if we want to comment this, it should be above the struct description and not here.
yacovm
reviewed
Aug 14, 2026
|
|
||
| // TestInstanceNonValidatorEvictsFinalizedBlocks asserts that a non-validator serves replicated | ||
| // blocks with their finalization once they are finalized. Blocks are cached as unfinalized upon | ||
| // verification, so indexing must evict them (instance.go:161) or they are served without a |
Collaborator
There was a problem hiding this comment.
we should not reference line numbers in comments. There is no way line numbers won't change.
Collaborator
There was a problem hiding this comment.
This comment is for all line numbers going forward
yacovm
reviewed
Aug 14, 2026
| block, ok := storage2.blockAt(seq) | ||
| require.True(t, ok) | ||
| digest := (&ParsedBlock{StateMachineBlock: block}).BlockHeader().Digest | ||
| _, finalization, err := nonValidatorInstance.cs.Retrieve(seq, digest) |
Collaborator
There was a problem hiding this comment.
What does this prove? We could still be retrieving this from the storage right?
I think we can just empty the dependency of the storage of the non-validator and see that it can still retrieve blocks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was wrong
The non-validator's EpochAwareStorage wrapped the raw storage (instance.go:160) instead of the instance's CachedStorage, unlike the validator path (instance.go:487). Blocks replicated by a non-validator are cached as unfinalized when verified (adapters.go:86), and CachedStorage.Index (adapters.go:138) is the only place cache entries are deleted, so the non-validator's indexing never evicted them.
Consequence
While a node runs as a non-validator, the cache grows by one entry per replicated block and is never pruned, holding the entire replicated chain in memory. Retrieving one of these finalized blocks by digest returns it with a nil finalization, violating the documented cache invariant (adapters.go:119). The stale entries survive into the validator role after promotion until the first block is indexed there.
Fix
Wire the non-validator's EpochAwareStorage to the block cache, matching the validator path, so indexing a finalized block evicts it and everything older.
Verification
TestInstanceNonValidatorEvictsFinalizedBlocks (instance_test.go): failed before the fix (finalized block at seq 1 served without its finalization), passes after. TestInstanceNonValidatorBootstraps and TestInstanceMixedNodeType still pass.
Found while debugging TestInstanceNonValidatorBootstraps on fix/cachedstorage-seq-only-lookup, where the stale entries became reachable by finality-sensitive callers.
🤖 Generated with Claude Code