Skip to content

Conversation

@AdoAdoAdo
Copy link
Contributor

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:

  • was the PR targeted to the correct branch?
  • if this is a larger feature that probably needs more than one PR, is there a feat branch created?
  • if this is a feat branch merging, do all satellite projects have a proper tag inside go.mod?

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes synchronized state handling by refactoring the logic that determines the starting point for synchronization. The key improvement is the introduction of a check for more recent execution results beyond the last notarized one, ensuring the sync process starts from the most up-to-date execution state. Additionally, the PR completes the execution result data structures by adding the missing HeaderEpoch field.

  • Refactored synchronization preparation logic into a dedicated method that checks both notarized and more recent execution results
  • Added HeaderEpoch field to execution result structures for complete header information
  • Updated test configuration constant to allow more blocks during transaction execution

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
process/sync/baseSync.go Extracted execution result header nonce retrieval into a new method that checks for both notarized and more recent execution results; fixed import ordering
process/common.go Added HeaderEpoch field to ExecutionResultInfo and MetaExecutionResultInfo creation
common/common.go Added HeaderEpoch field when creating execution results from previous headers
integrationTests/chainSimulator/staking/common.go Increased MaxNumOfBlockToGenerateWhenExecutingTx from 7 to 10; fixed import ordering and comment spacing

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +1134 to +1138
func (boot *baseBootstrap) getExecutionResultHeaderNonceForSyncStart(
syncingNonce uint64,
currentHeader data.HeaderHandler,
currentHeaderHash []byte,
) (uint64, error) {
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new function getExecutionResultHeaderNonceForSyncStart lacks documentation. Consider adding a comment that explains its purpose, parameters, and return value. For example, it should explain that it retrieves the header nonce of the most recent execution result available, either from the last notarized result or from a more recent execution result if available.

Copilot uses AI. Check for mistakes.
Comment on lines +1173 to +1183
lastExecutionResultNonce := lastNotarizedExecutedHeader.GetNonce()

// in case there is a more recent execution result available, use it
lastExecutionResult := boot.chainHandler.GetLastExecutionResult()
if !check.IfNil(lastExecutionResult) && lastExecutionResult.GetHeaderNonce() > lastExecutionResultNonce {
lastExecutionResultNonce = lastExecutionResult.GetHeaderNonce()
}

log.Debug("getExecutionResultHeaderNonceForSyncStart", "lastExecutionResultNonce", lastExecutionResultNonce)

return lastExecutionResultNonce, nil
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable name lastExecutionResultNonce is ambiguous because it starts as the nonce from the notarized execution result but may be updated to a more recent execution result. Consider renaming to something clearer like executionResultHeaderNonce or documenting what this variable represents after line 1178.

Suggested change
lastExecutionResultNonce := lastNotarizedExecutedHeader.GetNonce()
// in case there is a more recent execution result available, use it
lastExecutionResult := boot.chainHandler.GetLastExecutionResult()
if !check.IfNil(lastExecutionResult) && lastExecutionResult.GetHeaderNonce() > lastExecutionResultNonce {
lastExecutionResultNonce = lastExecutionResult.GetHeaderNonce()
}
log.Debug("getExecutionResultHeaderNonceForSyncStart", "lastExecutionResultNonce", lastExecutionResultNonce)
return lastExecutionResultNonce, nil
// executionResultHeaderNonce starts from the last notarized executed header
// and may be updated to a more recent execution result header nonce if available.
executionResultHeaderNonce := lastNotarizedExecutedHeader.GetNonce()
// in case there is a more recent execution result available, use it
lastExecutionResult := boot.chainHandler.GetLastExecutionResult()
if !check.IfNil(lastExecutionResult) && lastExecutionResult.GetHeaderNonce() > executionResultHeaderNonce {
executionResultHeaderNonce = lastExecutionResult.GetHeaderNonce()
}
log.Debug("getExecutionResultHeaderNonceForSyncStart", "executionResultHeaderNonce", executionResultHeaderNonce)
return executionResultHeaderNonce, nil

Copilot uses AI. Check for mistakes.
ssd04
ssd04 previously approved these changes Dec 22, 2025
@codecov
Copy link

codecov bot commented Dec 22, 2025

Codecov Report

❌ Patch coverage is 92.59259% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.73%. Comparing base (9f6edb1) to head (410de51).
⚠️ Report is 4 commits behind head on feat/supernova-async-exec.

Files with missing lines Patch % Lines
process/sync/baseSync.go 88.57% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@                    Coverage Diff                     @@
##           feat/supernova-async-exec    #7579   +/-   ##
==========================================================
  Coverage                      77.72%   77.73%           
==========================================================
  Files                            875      875           
  Lines                         120388   120400   +12     
==========================================================
+ Hits                           93576    93591   +15     
+ Misses                         20666    20662    -4     
- Partials                        6146     6147    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

lastExecutionResultNonce := lastNotarizedExecutedHeader.GetNonce()

// in case there is a more recent execution result available, use it
lastExecutionResult := boot.chainHandler.GetLastExecutionResult()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any chance that this happens right before we call txPool.OnExecutedBlock on L1167? would that be an ambiguous case where tx pool has last executed different than what this method returns?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that can happen.

The onExecute needs to be done with the latest notarized execution result not with the last available execution result. The last notarized execution result cannot change for the node if the node is not committing a new block that has passed consensus, either on the consensus flow, or on the sync flow.

This here is preceding the sync flow.

@sasurobert sasurobert self-requested a review December 23, 2025 07:33
sasurobert
sasurobert previously approved these changes Dec 23, 2025
@AdoAdoAdo AdoAdoAdo dismissed stale reviews from sasurobert and ssd04 via b3ba86d December 23, 2025 10:54
return haveTime
}

err := r.blockDataRequester.IsDataPreparedForProcessing(stepHaveTime(checkMissingDataStep))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i do not think you need this line. Inside the for, it would be enough.

@sstanculeanu sstanculeanu merged commit c43b7ec into feat/supernova-async-exec Dec 23, 2025
11 checks passed
@sstanculeanu sstanculeanu deleted the fixes-synchronized-state branch December 23, 2025 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants