Fix boostrap from network #7564
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/supernova-async-exec #7564 +/- ##
=============================================================
- Coverage 77.73% 77.67% -0.06%
=============================================================
Files 875 874 -1
Lines 120400 120586 +186
=============================================================
+ Hits 93591 93667 +76
- Misses 20662 20757 +95
- Partials 6147 6162 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This pull request implements the ability to save intermediate headers to storage during bootstrap, which is a key feature for V3 headers (Supernova). The main changes involve synchronizing not just epoch start headers but also intermediate headers up to the last executed block, and storing them appropriately during the bootstrap process.
- Adds functionality to fetch headers from both pool and storage (not just pool) when needed during bootstrap sync
- Updates header retrieval logic across block processors to check storage in addition to pool for V3 headers
- Implements proper storage of intermediate headers and metadata during epoch start bootstrap for both meta and shard nodes
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| process/sync/baseSync.go | Added new methods getHeaderWithNonce, getMetaHeaderWithNonce, and getShardHeaderWithNonce to fetch headers from both pool and storage; updated prepareForSyncIfNeeded to use the new retrieval methods |
| process/block/shardblock.go | Updated calls to use new getHeaderFromHash method instead of the removed getHeaderFromHash function |
| process/block/metablock.go | Updated multiple methods to use the new getHeaderFromHash method; added debug logging for self-notarized headers |
| process/block/baseProcess.go | Added getHeaderFromHash method to baseProcessor that checks both pool and storage for V3 headers |
| process/block/common.go | Removed the standalone getHeaderFromHash function in favor of the method-based approach |
| process/asyncExecution/executionManager/executionManager.go | Updated to use getHeaderFromPoolOrStorage (which already existed) with a TODO comment about optimization |
| process/asyncExecution/executionManager/executionManager_test.go | Updated test to reflect the new behavior of fetching from storage when pool lookup fails |
| epochStart/bootstrap/startInEpochScheduled.go | Added logic to handle V3 headers differently in getRequiredHeaderByHash; added TODO about analyzing request requirements |
| epochStart/bootstrap/shardStorageHandler.go | Removed filtering that only saved epoch start headers; now saves intermediate headers as well |
| epochStart/bootstrap/process.go | Added multiple new methods for syncing intermediate headers including syncIntermediateMetaBlocksIfNeeded, syncEpochStartDataInfo, syncLastNotarizedMetaForEpochStartData, and requestBlocksUpToLastExecuted |
| epochStart/bootstrap/metaStorageHandler.go | Added methods to compute and store last self-notarized headers for all shards and meta; added saveEpochStartMetaHdrs to save all synchronized headers |
| epochStart/bootstrap/fromLocalStorage.go | Updated to use process.UnmarshalMetaHeader for proper unmarshaling of meta headers (supporting both V2 and V3) |
| epochStart/bootstrap/process_test.go | Updated and expanded tests to cover the new intermediate header syncing behavior; removed obsolete test case |
| process/sync/storageBootstrap/metaStorageBootstrapper.go | Removed blank line (minor formatting change) |
Comments suppressed due to low confidence (1)
epochStart/bootstrap/process.go:939
- Missing documentation for this function. Consider adding a comment describing what this function does, its parameters, and return values.
func (e *epochStartBootstrap) requestBlocksUpToLastExecuted(
syncedHeaders map[string]data.HeaderHandler,
header data.HeaderHandler,
shardID uint32,
) error {
if !header.IsHeaderV3() {
return nil
}
lastExecutionResult, err := common.GetLastBaseExecutionResultHandler(header)
if err != nil {
return err
}
baseHeaderNonce := header.GetNonce()
lastExecutedNonce := lastExecutionResult.GetHeaderNonce()
if lastExecutedNonce >= baseHeaderNonce {
return nil
}
headerHashToSync := header.GetPrevHash()
currentNonce := baseHeaderNonce
for currentNonce > lastExecutedNonce {
syncedHeader, err := e.syncOneHeader(headerHashToSync, shardID)
if err != nil {
return err
}
syncedHeaders[string(headerHashToSync)] = syncedHeader
headerHashToSync = syncedHeader.GetPrevHash()
currentNonce = syncedHeader.GetNonce()
}
return nil
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ssd04
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
epochStart/bootstrap/process.go
Outdated
| return nil | ||
| } | ||
|
|
||
| for currNonce > lastFinished.GetNonce() { |
There was a problem hiding this comment.
this looks very similar to the sync loop on requestIntermediateBlocksIfNeeded
can you extract in a func and call it in both places?
There was a problem hiding this comment.
refactored to reduce common code
| } | ||
|
|
||
| for _, epochStartData := range epochStartMeta.GetEpochStartHandler().GetLastFinalizedHeaderHandlers() { | ||
|
|
| return bootstrapStorage.BootstrapHeaderInfo{}, epochStart.ErrWrongTypeAssertion | ||
| } | ||
|
|
||
| if len(shardHeaderHandler.GetMetaBlockHashes()) <= 0 { |
There was a problem hiding this comment.
I think this should be == 0, as it cannot be negative
| }, nil | ||
| } | ||
|
|
||
| metaHash := shardHeaderHandler.GetMetaBlockHashes()[0] |
There was a problem hiding this comment.
shouldn't this be the last referenced metablock hash instead?
| }, nil | ||
| } | ||
|
|
||
| metaHash := shardHeaderHandler.GetMetaBlockHashes()[0] |
There was a problem hiding this comment.
as discussed, the metaHash should be the last referenced metablock on the shardHeader.LastExecutinResult
if no Meta referenced there, then search on the previous and so forth until one is found.
valid for all returned bootstrapStorage.BootstrapHeaderInfo in the context of supernova
process/block/baseProcess.go
Outdated
|
|
||
| func (bp *baseProcessor) getHeaderFromHash( | ||
| isHeaderV3 bool, | ||
| shardHeaderHash []byte, |
There was a problem hiding this comment.
rename this to headerHash?
as this could also apply to metablocks
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
epochStart/bootstrap/process.go
Outdated
| for { | ||
| numIncludedMetaBlocks := len(currentHdr.GetMetaBlockHashes()) | ||
|
|
||
| // if there are no included meta blocks, go to prev header | ||
| if numIncludedMetaBlocks == 0 { | ||
| header, err := fetchPrevHeader(syncedHeaders, currentHdr) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| currentHdr = header | ||
| continue | ||
| } | ||
|
|
||
| // if there are notarized meta headers, return last included meta header | ||
| metaHash := currentHdr.GetMetaBlockHashes()[numIncludedMetaBlocks-1] | ||
|
|
||
| return metaHash, nil | ||
| } |
There was a problem hiding this comment.
Potential infinite loop: the for loop in getLastReferencedMetaHash lacks a termination condition if all previous shard headers have no meta block hashes. The loop will continue calling fetchPrevHeader indefinitely until it either finds a header with meta block hashes or hits an error. Consider adding a maximum iteration count or checking if we've reached the genesis block to prevent an infinite loop in edge cases.
There was a problem hiding this comment.
added genesis check condition
| func fetchPrevHeader( | ||
| syncedHeaders map[string]data.HeaderHandler, | ||
| header data.HeaderHandler, | ||
| ) (data.ShardHeaderHandler, error) { | ||
| prevHash := header.GetPrevHash() | ||
| syncedHeader, ok := syncedHeaders[string(prevHash)] | ||
| if !ok { | ||
| return nil, epochStart.ErrMissingHeader | ||
| } | ||
|
|
||
| shardHeader, ok := syncedHeader.(data.ShardHeaderHandler) | ||
| if !ok { | ||
| return nil, epochStart.ErrWrongTypeAssertion | ||
| } | ||
|
|
||
| return shardHeader, nil | ||
| } |
There was a problem hiding this comment.
Duplicated function: The fetchPrevHeader function in metaStorageHandler.go (lines 235-251) is identical to the syncPrevShardHeaderHandler method in process.go (lines 854-871), except the latter also stores the header in syncedHeaders. Consider extracting this into a shared utility function to avoid code duplication and potential maintenance issues.
There was a problem hiding this comment.
mainly the similar part is the type assertion, otherwise they are quite different since the other one uses syncOneHeader which needs other things like request, data pool, we can have a function pointer for this call, but it would be very similar with what we already have now
sstanculeanu
merged commit e7e0cd1
into
feat/supernova-async-exec
Reasoning behind the pull request
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?