Trie prunning async trigger#7800
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/supernova-async-exec #7800 +/- ##
=============================================================
- Coverage 77.54% 77.54% -0.01%
=============================================================
Files 882 882
Lines 123683 123726 +43
=============================================================
+ Hits 95908 95938 +30
- Misses 21429 21442 +13
Partials 6346 6346 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces an async trigger for trie pruning (especially for header v3), wiring it through the async headers execution flow and updating the relevant interfaces/mocks/stubs accordingly.
Changes:
- Add
PruneTrieAsyncHeader(header data.HeaderHandler)to block processor interfaces and implement it in the base processor. - Stop triggering shard header v3 pruning from
shardProcessor.updateStateand trigger pruning fromasyncExecution/headersExecutor. - Add
process.GetHeaderWithNonce(...)helper to fetch headers by nonce for both shard and metachain paths.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| testscommon/processMocks/blockProcessorStub.go | Extend async-execution stub with PruneTrieAsyncHeader. |
| testscommon/blockProcessorStub.go | Extend test stub with PruneTrieAsyncHeader. |
| process/interface.go | Add PruneTrieAsyncHeader to main process.BlockProcessor interface. |
| process/common.go | Add GetHeaderWithNonce wrapper that dispatches to meta vs shard retrieval. |
| process/block/shardblock_test.go | Update pruning test to pass a header (not exec results slice). |
| process/block/shardblock.go | Remove sync v3 pruning from updateState; change v3 pruning API to accept a header. |
| process/block/metablock.go | Rename/simplify v3 pruning path and fetch previous meta header on-demand. |
| process/block/interface.go | Extend internal blockProcessor interface with pruneTrieHeaderV3(header). |
| process/block/export_test.go | Update exported test helper PruneTrieHeaderV3 signature; add test-only getters/setters for last pruned nonce. |
| process/block/baseProcess_test.go | Add unit tests for PruneTrieAsyncHeader. |
| process/block/baseProcess.go | Add async pruning logic with intermediate nonce handling + last-pruned nonce tracking. |
| process/asyncExecution/interface.go | Add PruneTrieAsyncHeader to async execution’s BlockProcessor interface. |
| process/asyncExecution/headersExecutor.go | Trigger async trie pruning after execution result is persisted/tracked. |
| integrationTests/mock/blockProcessorMock.go | Extend integration mock with PruneTrieAsyncHeader. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| header3 := &block.HeaderV3{ | ||
| Nonce: 10, | ||
| } | ||
| _ = header1.SetExecutionResultsHandlers(executionResultsHandlers) |
There was a problem hiding this comment.
In this test, execution results are set on header1 (nonce 8), but the method under test is called with header3 (nonce 10). This leaves header3 without execution results handlers, so PruneTrieAsyncHeader(header3) may not trigger pruning as expected and the assertions can become incorrect/flaky. Set the execution results handlers on header3 (or call pruning on the header you populated).
| _ = header1.SetExecutionResultsHandlers(executionResultsHandlers) | |
| _ = header3.SetExecutionResultsHandlers(executionResultsHandlers) |
| // TODO: analyse based on current header to be committed, not last committed header | ||
| prevHeaderHash := sp.getCurrentBlockHeader().GetPrevHash() | ||
| prevHeader, err := process.GetShardHeader(prevHeaderHash, sp.dataPool.Headers(), sp.marshalizer, sp.store) | ||
| if err != nil { |
There was a problem hiding this comment.
getPreviousExecutionResult() uses sp.getCurrentBlockHeader().GetPrevHash() to find the previous header. With the new async pruning flow (including pruning of intermediate nonces), pruneTrieHeaderV3() can be called for headers that are not the current committed header, so this will fetch the wrong previous header and compute incorrect previous root hashes for pruning. Pass the header being pruned (or its PrevHash) into getPreviousExecutionResult() and use header.GetPrevHash() instead.
| } | ||
|
|
||
| // prune trie for the provided header | ||
| bp.blockProcessor.pruneTrieHeaderV3(header) | ||
| bp.lastPrunedHeaderNonce = header.GetNonce() |
There was a problem hiding this comment.
If pruning an intermediate nonce fails (e.g. header not found and the loop continues), lastPrunedHeaderNonce is still advanced to the provided header’s nonce at the end. This permanently skips the failed intermediate nonce(s) and they will never be retried in later calls, which can lead to missing trie pruning. Consider only advancing lastPrunedHeaderNonce up to the last successfully pruned nonce, or track/retry failures instead of skipping them permanently.
| // should be available in pool, since trie prunning is triggered from | ||
| // execution flow; if there are no included blocks from execution flow | ||
| // (and not prunning triggerd) headers will not be removed from pool | ||
| intermHeader, _, err := process.GetHeaderWithNonce( |
There was a problem hiding this comment.
Spelling: several occurrences of "prunning" / "triggerd" in these comments (and the warn log below in this function) should be "pruning" / "triggered" for consistency and searchability.
| sp.pruneTrieLegacy(headers) | ||
| } else { | ||
| sp.pruneTrieHeaderV3(currentHeader.GetExecutionResultsHandlers()) | ||
| // for header v3, trie prunning is triggered in async mode from headers executor |
There was a problem hiding this comment.
Spelling: "trie prunning" should be "trie pruning".
| // for header v3, trie prunning is triggered in async mode from headers executor | |
| // for header v3, trie pruning is triggered in async mode from headers executor |
| // GetHeaderWithNonce tries to get the header by nonce from pool first and if not found, searches for it through storer | ||
| func GetHeaderWithNonce( | ||
| nonce uint64, | ||
| shardId uint32, | ||
| headersCacher dataRetriever.HeadersPool, | ||
| marshalizer marshal.Marshalizer, | ||
| storageService dataRetriever.StorageService, | ||
| uint64Converter typeConverters.Uint64ByteSliceConverter, | ||
| ) (data.HeaderHandler, []byte, error) { | ||
| if shardId == core.MetachainShardId { | ||
| return GetMetaHeaderWithNonce( | ||
| nonce, | ||
| headersCacher, | ||
| marshalizer, | ||
| storageService, | ||
| uint64Converter, | ||
| ) | ||
| } | ||
|
|
||
| return GetShardHeaderWithNonce( | ||
| nonce, | ||
| shardId, | ||
| headersCacher, | ||
| marshalizer, | ||
| storageService, | ||
| uint64Converter, | ||
| ) | ||
| } |
There was a problem hiding this comment.
GetHeaderWithNonce is a new public helper that adds branching behavior (meta vs shard) but currently has no direct unit tests. Since process/common_test.go already covers the underlying meta/shard helpers, consider adding a small test table for GetHeaderWithNonce to ensure the metachain branch and shard branch dispatch correctly.
1714b93
process/block/metablock.go
Outdated
| ) | ||
| } else { | ||
| mp.pruneTriesHeaderV3(metaBlock, prevMetaBlock) | ||
| mp.pruneTrieHeaderV3(metaBlock) |
There was a problem hiding this comment.
is this still supposed to be here?
| // should be available in pool, since trie prunning is triggered from | ||
| // execution flow; if there are no included blocks from execution flow | ||
| // (and not prunning triggerd) headers will not be removed from pool | ||
| intermHeader, _, err := process.GetHeaderWithNonce( |
There was a problem hiding this comment.
can you get the headers by hash? (revert traversing and then go forward
| ) | ||
| if err != nil { | ||
| log.Warn("failed to get intermediate header for prunning", "error", err) | ||
| continue |
There was a problem hiding this comment.
maybe in case of error here, should we just cancel prune for this one?
but then that header might have had more execution results
maybe simpler is just to clean up the eviction wait list and then it acts like on reset.
| return nil, err | ||
| } | ||
|
|
||
| if prevHeader.IsHeaderV3() { |
There was a problem hiding this comment.
this doesn't look ok when called repeatedly in the loop from func (sp *shardProcessor) pruneTrieHeaderV3
it always defaults to the same prevHeader, or if in the meantime the consensus advanced to a newer one.
Reasoning behind the pull request
Proposed changes
Testing procedure
Pre-requisites
Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:
featbranch created?featbranch merging, do all satellite projects have a proper tag insidego.mod?