RevertCurrentBlock by sstanculeanu · Pull Request #7355 · multiversx/mx-chain-go

sstanculeanu · 2025-10-20T11:39:35Z

Reasoning behind the pull request

RevertCurrentBlock should update also the execution tracker
as for the blocks queue, AddOrReplace method should be able to replace all headers on top of a specific nonce

Proposed changes

updated blocksQeueu.AddOrReplace
added a new method executionResultsTracker.RemoveFromHash

Testing procedure

with feat branch

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?

codecov · 2025-10-20T11:53:18Z

Codecov Report

❌ Patch coverage is 94.48819% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.31%. Comparing base (129c05f) to head (857919f).
⚠️ Report is 32 commits behind head on feat/supernova-async-exec.

Files with missing lines	Patch %	Lines
...xecution/executionTrack/executionResultsTracker.go	80.95%	2 Missing and 2 partials ⚠️
process/asyncExecution/queue/blocksQueue.go	95.23%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@                      Coverage Diff                      @@
##           feat/supernova-async-exec    #7355      +/-   ##
=============================================================
+ Coverage                      77.28%   77.31%   +0.02%     
=============================================================
  Files                            867      867              
  Lines                         116704   116789      +85     
=============================================================
+ Hits                           90193    90292      +99     
+ Misses                         20559    20548      -11     
+ Partials                        5952     5949       -3

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

AdoAdoAdo · 2025-10-28T12:45:02Z

 }

+// RemoveFromHash will remove the execution result with the provided hash and all execution results with higher nonces
+func (ert *executionResultsTracker) RemoveFromHash(hash []byte) error {


can you check if it is possible to give also the Nonce?
at line 331 if we cannot find the hash, any on-top results would not be cleaned.

AdoAdoAdo · 2025-10-28T13:20:25Z

+	}
+	_, shouldIgnoreExecutionResultNonce := ert.noncesToRemoveOnAdd[executionResult.GetHeaderNonce()]
+	if shouldIgnoreExecutionResultNonce {
+		delete(ert.noncesToRemoveOnAdd, executionResult.GetHeaderNonce())


can we make sure we are considering all registered nonces to remove ?

Copilot

Pull Request Overview

This PR enhances the RevertCurrentBlock functionality to properly clean up the execution tracker and blocks queue when reverting blocks. The key changes ensure that when a block processing fails and needs to be reverted, both the execution results tracker and the blocks queue are updated to maintain consistency.

Key Changes:

Modified RevertCurrentBlock interface to accept a HeaderHandler parameter
Added RemoveFromNonce method to the execution results tracker
Added RemoveAtNonceAndHigher method and eviction subscriber support to the blocks queue
Integrated blocks queue eviction notifications with the execution results tracker

Reviewed Changes

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

Show a summary per file

File	Description
process/interface.go	Updated BlockProcessor and related interfaces with new method signatures
process/block/baseProcess.go	Implemented revertCurrentBlockV3 logic to clean up tracker and queue
process/asyncExecution/queue/blocksQueue.go	Added removal methods and eviction subscriber support
process/asyncExecution/executionTrack/executionResultsTracker.go	Added RemoveFromNonce and OnHeaderEvicted methods
factory/processing/*.go	Updated factory code to wire up eviction subscriptions
testscommon/*	Updated mocks to match new interfaces
integrationTests/*	Updated test initialization to register eviction subscribers

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

Copilot · 2025-10-28T13:23:50Z

+	ert.mutex.Lock()
+	defer ert.mutex.Unlock()
+
+	// first search thorough pending execution results and remove it if already processed


Corrected spelling of 'thorough' to 'through'.

Suggested change

// first search thorough pending execution results and remove it if already processed

// first search through pending execution results and remove it if already processed

Copilot · 2025-10-28T13:23:50Z

+		log.Error("revertCurrentBlockV3 RemoveFromNonce", "err", err)
+		return
+	}
+
+	err = bp.blocksQueue.RemoveAtNonceAndHigher(headerHandler.GetNonce())
+	if err != nil {
+		log.Error("revertCurrentBlockV3 RemoveAtNonceAndHigher", "err", err)


Error logging should include the header nonce being reverted to aid debugging. Add 'nonce' field to both log.Error calls.

Suggested change

log.Error("revertCurrentBlockV3 RemoveFromNonce", "err", err)

return

}

err = bp.blocksQueue.RemoveAtNonceAndHigher(headerHandler.GetNonce())

if err != nil {

log.Error("revertCurrentBlockV3 RemoveAtNonceAndHigher", "err", err)

log.Error("revertCurrentBlockV3 RemoveFromNonce", "err", err, "nonce", headerHandler.GetNonce())

return

}

err = bp.blocksQueue.RemoveAtNonceAndHigher(headerHandler.GetNonce())

if err != nil {

log.Error("revertCurrentBlockV3 RemoveAtNonceAndHigher", "err", err, "nonce", headerHandler.GetNonce())

Copilot · 2025-10-28T13:23:51Z

+	if indexToRemove == 0 {
+		// removing from the beginning, clear the entire queue
+		bq.headerBodyPairs = make([]HeaderBodyPair, 0)
+		if nonce > 0 {
+			bq.lastAddedNonce = nonce - 1
+			return nil
+		}
+
+		bq.lastAddedNonce = 0
+		return nil


When removing from the beginning (indexToRemove == 0), eviction notifications are not sent for the removed pairs. This is inconsistent with the behavior at line 211 where notifyEvictedPairs is called. Consider notifying subscribers before clearing the queue to maintain consistent behavior.

good catch, updated

AdoAdoAdo · 2025-10-28T16:16:36Z

+		return
+	}
+
+	err = bp.blocksQueue.RemoveAtNonceAndHigher(headerNonce)


is it still needed then to have the execution results tracker subscribed to the evictions from the blocks queue if we call remove on both here?

I think it should not be needed, or am I missing something?

as blocksQueue.AddOrReaplace is now able to replace an intermediat nonce and remove all the above ones, I think the subscriber mechanism can be kept

removed the call to executionResultsTracker from the above though, as it will be done internally through notification

AdoAdoAdo · 2025-10-28T16:17:38Z

+}
+
+// RegisterEvictionSubscriber registers a new eviction subscriber
+func (bq *blocksQueue) RegisterEvictionSubscriber(subscriber BlocksQueueEvictionSubscriber) {


is this still needed?

… queue

Copilot

Pull Request Overview

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

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

… the one requested to be removed)

AdoAdoAdo · 2025-10-31T12:14:53Z

+		return nil
+	}
+
+	bq.updateLastAddedNonceBasedOnRemovingNonce(nonce)


I don't think this should update, as you are also returning error.

AdoAdoAdo · 2025-10-31T12:39:03Z

-		ert.hashToRemoveOnAdd = hash
-		return nil
-	}
+	return ert.removePendingFromNonceUnprotected(nonce)


I think this does not finish the cleanup, as the lastExecutedResultHash is not set correctly.
Maybe give on RemoveFromNonce the previousHash if this blockNonce, so that it is set on lastExecutedResultHash?

AdoAdoAdo · 2025-10-31T12:40:02Z

 }

+// OnHeaderEvicted is a callback called when a header is removed from the execution queue
+func (ert *executionResultsTracker) OnHeaderEvicted(headerNonce uint64) {


I think this also needs to give the hash of the previous block (previous to the header with the nonce to be removed) so that it is set as the last

AdoAdoAdo · 2025-10-31T12:42:20Z

+	return pairsToBeRemoved, firstIndex
+}
+
+func (bq *blocksQueue) removeFromNonce(nonce uint64) {


this might need to receive the hash of the previous block, to notify the subscribers

AdoAdoAdo · 2025-10-31T12:42:59Z


+// RemoveAtNonceAndHigher removes the header-body pair at the specified nonce
+// and all pairs with higher nonces from the queue
+func (bq *blocksQueue) RemoveAtNonceAndHigher(nonce uint64) {


might need to get the prevHash as well.

AdoAdoAdo · 2025-10-31T13:33:41Z

The TODO will be taken on a different PR, which will add also a manager component, to handle the three subcomponents consistently.
Then the left comments should be solved as well.

Copilot

Pull Request Overview

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

Comments suppressed due to low confidence (1)

process/asyncExecution/headersExecutor.go:126

When AddExecutionResult fails, the error is logged but nil is returned. This means the processing is marked as successful even though the execution result was not saved. This should return the error to trigger retry logic in handleProcessError, otherwise the result will be lost permanently.

	err = he.executionTracker.AddExecutionResult(executionResult)
	if err != nil {
		log.Warn("headersExecutor.process add execution result failed", "err", err)
		return nil

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

sstanculeanu added 3 commits October 20, 2025 13:22

added RemoveFromHash on executionResultsTracker

9471a92

extended AddOrReplace method from blocksQueue

4aafee2

updated RevertCurrentBlock

4ef451e

sstanculeanu self-assigned this Oct 20, 2025

sstanculeanu added area:processing complexity:trivial impact:low interested:protocol type:improvement labels Oct 20, 2025

fixed RevertCurrentBlockCalled

e937949

Merge branch 'feat/supernova-async-exec' into revert-current-block

ae67e3d

ssd04 previously approved these changes Oct 21, 2025

View reviewed changes

Merge branch 'feat/supernova-async-exec' into revert-current-block

6ee0f61

AdoAdoAdo reviewed Oct 27, 2025

View reviewed changes

sstanculeanu and others added 2 commits October 27, 2025 17:57

Merge branch 'feat/supernova-async-exec' into revert-current-block

9b091f0

fixes after review

6aa6442

sstanculeanu dismissed ssd04’s stale review via 6aa6442 October 28, 2025 10:36

Merge branch 'feat/supernova-async-exec' into revert-current-block

c911522

AdoAdoAdo reviewed Oct 28, 2025

View reviewed changes

fixes after discussion

a0d029e

AdoAdoAdo requested a review from Copilot October 28, 2025 13:22

Copilot AI reviewed Oct 28, 2025

View reviewed changes

sstanculeanu and others added 3 commits October 28, 2025 15:30

Merge branch 'feat/supernova-async-exec' into revert-current-block

fd84259

fixes after copilot

5cafea7

second fix after copilot

c506617

miiu96 previously approved these changes Oct 28, 2025

View reviewed changes

AdoAdoAdo reviewed Oct 28, 2025

View reviewed changes

fixes after review, removed call that is not needed anymore

a118a4b

sstanculeanu dismissed miiu96’s stale review via a118a4b October 28, 2025 16:32

Merge branch 'feat/supernova-async-exec' into revert-current-block

bc5c594

ssd04 previously approved these changes Oct 29, 2025

View reviewed changes

AdoAdoAdo reviewed Oct 29, 2025

View reviewed changes

Comment thread process/asyncExecution/queue/blocksQueue.go Outdated

fixes after review: register headersExecutor for evicted headers from…

6a33236

… queue

sstanculeanu dismissed ssd04’s stale review via 6a33236 October 30, 2025 14:49

Merge branch 'feat/supernova-async-exec' into revert-current-block

a452c25

AdoAdoAdo requested a review from Copilot October 31, 2025 08:59

Copilot AI reviewed Oct 31, 2025

View reviewed changes

Comment thread process/asyncExecution/queue/blocksQueue.go Outdated

Comment thread process/asyncExecution/queue/blocksQueue.go Outdated

Comment thread process/asyncExecution/queue/blocksQueue.go Outdated

Comment thread process/asyncExecution/executionTrack/executionResultsTracker.go Outdated

sstanculeanu added 3 commits October 31, 2025 11:08

removed RemoveByHash

5706dfa

fixes after copilot review

f50a2b9

fixed linter

becb6b8

AdoAdoAdo reviewed Oct 31, 2025

View reviewed changes

Comment thread process/asyncExecution/queue/blocksQueue.go Outdated

sstanculeanu and others added 5 commits October 31, 2025 12:48

fixes after review, small cleanup

69ce704

fixed linter

ef5a6f1

fixed linter

c29762e

fixes after review, only notify for lowest nonce(the one replacing or…

1b27737

… the one requested to be removed)

Merge branch 'feat/supernova-async-exec' into revert-current-block

1851add

AdoAdoAdo reviewed Oct 31, 2025

View reviewed changes

sstanculeanu and others added 3 commits October 31, 2025 15:02

added extra todo

8a747c1

Merge branch 'feat/supernova-async-exec' into revert-current-block

e006e18

fix after review + updated todo

a7993d9

AdoAdoAdo approved these changes Oct 31, 2025

View reviewed changes

AdoAdoAdo requested a review from Copilot October 31, 2025 13:34

Copilot AI reviewed Oct 31, 2025

View reviewed changes

Comment thread process/asyncExecution/queue/blocksQueue.go

Comment thread process/block/shardblockProposal.go

miiu96 approved these changes Oct 31, 2025

View reviewed changes

ssd04 approved these changes Oct 31, 2025

View reviewed changes

Merge branch 'feat/supernova-async-exec' into revert-current-block

857919f

AdoAdoAdo merged commit b47470c into feat/supernova-async-exec Nov 1, 2025
11 checks passed

AdoAdoAdo deleted the revert-current-block branch November 1, 2025 11:52

	// first search thorough pending execution results and remove it if already processed
	// first search through pending execution results and remove it if already processed

Uh oh!

Conversation

sstanculeanu commented Oct 20, 2025

Reasoning behind the pull request

Proposed changes

Testing procedure

Pre-requisites

Uh oh!

codecov Bot commented Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AdoAdoAdo commented Oct 31, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

codecov Bot commented Oct 20, 2025 •

edited

Loading