Skip to content

Conversation

@sstanculeanu
Copy link
Collaborator

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?

@sstanculeanu sstanculeanu self-assigned this Dec 24, 2025
AdoAdoAdo
AdoAdoAdo previously approved these changes Dec 29, 2025
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 implements an optimization to finish consensus early if a header for the current consensus round has already been received and committed. This prevents redundant consensus processing when the block has already been finalized through other means (e.g., sync).

Key Changes

  • Added early detection in initCurrentRound() to check if the current block header's round matches the ongoing consensus round
  • When detected, all consensus subrounds are marked as finished to skip unnecessary processing
  • Added defensive checks in receivedBlockBody() and receivedBlockHeader() to prevent processing when the subround is already finished

Reviewed changes

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

File Description
consensus/spos/bls/v2/subroundStartRound.go Adds import for bls constants and implements early consensus termination logic when header is already committed for current round
consensus/spos/bls/v2/subroundBlock.go Adds early return guards in message reception handlers to prevent processing when subround is already finished

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

}

currentHeader := sr.Blockchain().GetCurrentBlockHeader()
if !check.IfNil(currentHeader) && int64(currentHeader.GetRound()) == sr.RoundHandler().Index() {
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

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

The comparison between currentHeader.GetRound() (which returns uint64) and sr.RoundHandler().Index() (which returns int64) involves casting uint64 to int64. This can cause issues if the round number exceeds the maximum value of int64, leading to incorrect comparisons due to overflow. Consider comparing both values as uint64 to avoid potential overflow issues.

Copilot uses AI. Check for mistakes.
Comment on lines +135 to +145
currentHeader := sr.Blockchain().GetCurrentBlockHeader()
if !check.IfNil(currentHeader) && int64(currentHeader.GetRound()) == sr.RoundHandler().Index() {
log.Debug("initCurrentRound: header for current consensus already committed, setting all subrounds as finished")

sr.SetStatus(sr.Current(), spos.SsFinished)
sr.SetStatus(bls.SrBlock, spos.SsFinished)
sr.SetStatus(bls.SrSignature, spos.SsFinished)
sr.SetStatus(bls.SrEndRound, spos.SsFinished)

return true
}
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

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

This early exit logic for finishing consensus when a header is already committed lacks test coverage. The new behavior introduced in lines 135-145 should be tested to ensure it correctly handles the case where the current block header's round matches the consensus round, and that all subrounds are properly marked as finished. Consider adding a test case to verify this scenario.

Copilot uses AI. Check for mistakes.
Comment on lines +135 to +145
currentHeader := sr.Blockchain().GetCurrentBlockHeader()
if !check.IfNil(currentHeader) && int64(currentHeader.GetRound()) == sr.RoundHandler().Index() {
log.Debug("initCurrentRound: header for current consensus already committed, setting all subrounds as finished")

sr.SetStatus(sr.Current(), spos.SsFinished)
sr.SetStatus(bls.SrBlock, spos.SsFinished)
sr.SetStatus(bls.SrSignature, spos.SsFinished)
sr.SetStatus(bls.SrEndRound, spos.SsFinished)

return true
}
Copy link

Copilot AI Dec 30, 2025

Choose a reason for hiding this comment

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

There's a potential race condition in the early exit logic. Between checking if the current header's round matches the consensus round (line 136) and setting the subround statuses (lines 139-142), the blockchain state could change due to concurrent operations. If another goroutine commits a different block for the same round during this window, the consensus state might become inconsistent. Consider using synchronization mechanisms to ensure atomicity of this check-and-set operation, or verify that existing locks already protect this critical section.

Copilot uses AI. Check for mistakes.
ssd04
ssd04 previously approved these changes Dec 30, 2025
Base automatically changed from fix-multiple-headers-not-being-referenced to feat/supernova-async-exec December 30, 2025 08:28
@sstanculeanu sstanculeanu dismissed stale reviews from ssd04 and AdoAdoAdo December 30, 2025 08:28

The base branch was changed.

@AdoAdoAdo AdoAdoAdo merged commit fb93090 into feat/supernova-async-exec Dec 30, 2025
9 checks passed
@AdoAdoAdo AdoAdoAdo deleted the fix-round-canceled-when-header-received-early branch December 30, 2025 08:41
@codecov
Copy link

codecov bot commented Dec 30, 2025

Codecov Report

❌ Patch coverage is 41.66667% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.65%. Comparing base (75782c2) to head (e263fc6).
⚠️ Report is 5 commits behind head on feat/supernova-async-exec.

Files with missing lines Patch % Lines
consensus/spos/bls/v2/subroundStartRound.go 12.50% 6 Missing and 1 partial ⚠️
Additional details and impacted files
@@                      Coverage Diff                      @@
##           feat/supernova-async-exec    #7583      +/-   ##
=============================================================
- Coverage                      77.66%   77.65%   -0.02%     
=============================================================
  Files                            874      874              
  Lines                         120689   120701      +12     
=============================================================
- Hits                           93739    93733       -6     
- Misses                         20777    20788      +11     
- Partials                        6173     6180       +7     

☔ 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.

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.

4 participants