Wait for tx pool re-org when emitting pending transaction events #1323

yacovm · 2025-10-05T17:54:57Z

Why this should be merged

Currently whenever we ask the VM to wait for an event, it checks if the mempool has transactions in it, and if so, it proceeds to return a "pending transactions" event.

The mempool has or has not transactions in it according to the block that the VM is configured to build on top on.

When the VM is being told to change the preferred block to build the next block on, the mempool is asynchronously re-organized in the background according to the block preference change.

Since the mempool re-organization happens asynchronously and in the background, when a VM changes its preference and is immediately asked to build a new block, the mempool may still contain transactions in it, because the mempool re-organization is still being performed asynchronously in the background. This leads to a false positive, as the mempool may not have transactions in it after its re-organization, but the WaitForEvent API call may return that the mempool has transactions in it and would result in building a block that shouldn't have been built.

This PR fixes this false positive, as explained below.

How this works

When we set the preference in the VM or accept a block, we mark the block height to be pending.
In parallel, we subscribe to updates from the mempool which are sent only once the re-org corresponding to a block has finished. Upon such an update, we mark the block as the latest block.

When the VM checks whether to build a block, additionally to checking whether there are transactions in the mempool, we now also check whether the mempool is pending a re-org. A mempool is pending a re-org if the block number that is set when the block is accepted or the preference changes (the pending number) is different than the block number that is received in the latest subscription update from the mempool (the latest number).

How this was tested

Rename TestWaitForEvent from the PR to testWaitForEvent and create TestWaitForEvent as follows:

func TestWaitForEvent(t *testing.T) {
	for i := 0; i < 1000; i++ {
		t.Run(fmt.Sprintf("run-%d", i), func(t *testing.T) {
			t.Parallel()
			testWaitForEvent(t)
		})
	}
}

The test passes but miserably fails in master.

Need to be documented?

No

Need to update RELEASES.md?

No

yacovm · 2025-10-05T19:45:43Z

plugin/evm/vm.go

+		case <-vm.shutdownChan:
+			return
+		case event := <-events:
+			if event.Head == nil {


can event.Head be nil or event.Head.Number be nil?

yacovm · 2025-10-05T19:45:57Z

plugin/evm/wrapped_block.go

+
+	if b.ethBlock != nil {
+		vm.setPendingBlock(b.ethBlock.NumberU64())
+	} else {


can this happen?

no, we do check this in verification

ceyonur

Few comments, I think we should move most of the logic to builder.

ceyonur · 2025-10-07T16:32:48Z

plugin/evm/vm.go

+	blockNumLock    sync.RWMutex
+	pendingBlockNum uint64
+	latestBlockNum  uint64


should we just push these to builder?

ceyonur · 2025-10-07T16:33:49Z

plugin/evm/vm.go

+	events := make(chan core.NewTxPoolReorgEvent)
+	sub := vm.txPool.SubscribeNewReorgEvent(events)
+	vm.shutdownWg.Add(1)
+	go func() {
+		vm.subscribeToTxPoolEvents(events)
+		sub.Unsubscribe()
+		vm.shutdownWg.Done()
+	}()


can we use builder to subscribe this? I think generally block builder should be a separate entity from vm.

ceyonur · 2025-10-07T16:35:40Z

plugin/evm/vm.go

+	vm.blockNumLock.Lock()
+	defer vm.blockNumLock.Unlock()
+	vm.pendingBlockNum = blockNum
+}


again can be part of block builder

ceyonur · 2025-10-07T16:38:33Z

plugin/evm/vm.go

+	err = vm.blockChain.SetPreference(block.GetEthBlock())
+	if err == nil {
+		vm.setPendingBlock(block.GetEthBlock().NumberU64())
+	}
+	return err
 }


Suggested change

err = vm.blockChain.SetPreference(block.GetEthBlock())

if err == nil {

vm.setPendingBlock(block.GetEthBlock().NumberU64())

}

return err

}

err = vm.blockChain.SetPreference(block.GetEthBlock())

if err != nil {

return err

}

vm.setPendingBlock(block.GetEthBlock().NumberU64())

return err

}

ceyonur · 2025-10-07T16:41:35Z

plugin/evm/vm.go

+func (vm *VM) subscribeToTxPoolEvents(events <-chan core.NewTxPoolReorgEvent) {
+	for {
+		select {
+		case <-vm.shutdownChan:


can we use the ctx in onNormalOperationsStarted?

ceyonur · 2025-10-07T16:58:08Z

plugin/evm/wrapped_block.go

 		"height", b.Height(),
 	)
+
+	if b.ethBlock != nil {


why do we set this here? I think setting this in SetPreference should be enough?

Because we later call vm.blockchain.Accept() and then setPreference.

ceyonur · 2025-10-07T17:41:45Z

plugin/evm/vm.go

+	vm.builder = vm.NewBlockBuilder(vm.extensionConfig.ExtraMempool, func() bool {
+		vm.blockNumLock.RLock()
+		defer vm.blockNumLock.RUnlock()
+		return vm.pendingBlockNum != vm.latestBlockNum


I'm confused with this equation doesn't this mean

b.pendingPoolUpdate will return true if they're not equal and it will keep waiting?

Exactly, if they are not equal - it means there is a pending mempool re-org, so we need to keep waiting.

If they are equal, then there is no pending mempool re-org and it is safe to return the pending event.

ceyonur · 2025-10-07T17:44:05Z

plugin/evm/vm.go

+			vm.blockNumLock.Lock()
+			vm.latestBlockNum = event.Head.Number.Uint64()
+			vm.blockNumLock.Unlock()
+			vm.builder.signalCanBuild()


isn't it enough to check event.Head against vm.pendingBlockNumber and then signal/not signal the builder? why do we need vm.latestBlockNum?

We wait on the condition variable:

for !b.needToBuild() || b.pendingPoolUpdate() { if err := b.pendingSignal.Wait(ctx); err != nil { return time.Time{}, common.Hash{}, err } }

In case we either don't need to build a new block, or there is a pending pool update.
If we need to build a new block but there is a pending pool update, we need to wait on the condition variable.

If we don't check the pending pool update in case we need to build a block, then we won't wait on the condition variable and will immediately try to build a block and may suffer the false positive.

The signal is done to wake up and check again if the mempool re-org has finished, because without the signal we may wait on the condition variable and not wake up once the re-org has finished.

yacovm · 2025-10-07T19:32:58Z

Few comments, I think we should move most of the logic to builder.

Thanks for the quick review, addressed your comments!

Currently whenever we ask the VM to wait for an event, it checks if the mempool has transactions in it, and if so, it proceeds to return a "pending transactions" event. The mempool has or has not transactions in it according to the block that the VM is configured to build on top on. When the VM is being told to change the preferred block to build the next block on, the mempool is asynchronously re-organized in the background according to the block preference change. Since the mempool re-organization happens asynchronously and in the background, when a VM changes its preference and is immediately asked to build a new block, the mempool may still contain transactions in it, because the mempool re-organization is still being performed asynchronously in the background. This leads to a false positive, as the mempool may not have transactions in it after its re-organization, but the WaitForEvent API call may return that the mempool has transactions in it and would result in building a block that shouldn't have been built. This PR fixes this false positive, as explained below: When we set the preference in the VM or accept a block, we mark the block height to be pending. In parallel, we subscribe to updates from the mempool which are sent only once the re-org corresponding to a block has finished. Upon such an update, we mark the block as the latest block. When the VM checks whether to build a block, additionally to checking whether there are transactions in the mempool, we now also check whether the mempool is pending a re-org. A mempool is pending a re-org if the block number that is set when the block is accepted or the preference changes (the pending number) is different than the block number that is received in the latest subscription update from the mempool (the latest number).

Signed-off-by: Yacov Manevich <[email protected]>

yacovm requested a review from a team as a code owner October 5, 2025 17:54

yacovm marked this pull request as draft October 5, 2025 17:55

yacovm force-pushed the yacovm/1312 branch 2 times, most recently from da88c79 to 7bfe897 Compare October 5, 2025 19:25

yacovm marked this pull request as ready for review October 5, 2025 19:32

yacovm changed the title ~~Wait for tx pool event loop~~ Wait for tx pool re-org when emitting pending transaction events Oct 5, 2025

yacovm force-pushed the yacovm/1312 branch from 7bfe897 to 1b822eb Compare October 5, 2025 19:44

yacovm commented Oct 5, 2025

View reviewed changes

ceyonur reviewed Oct 7, 2025

View reviewed changes

yacovm added 3 commits October 8, 2025 14:37

Address nit in code review

ab5a8d9

Signed-off-by: Yacov Manevich <[email protected]>

Move pool re-org subscription to block builder

16c4c4b

Signed-off-by: Yacov Manevich <[email protected]>

yacovm force-pushed the yacovm/1312 branch from 993b600 to 16c4c4b Compare October 8, 2025 12:40

Wait for tx pool re-org when emitting pending transaction events #1323

Are you sure you want to change the base?

Wait for tx pool re-org when emitting pending transaction events #1323

Conversation

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

Uh oh!

Why this should be merged

How this works

How this was tested

Need to be documented?

Need to update RELEASES.md?

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!

ceyonur left a comment

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

yacovm commented Oct 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yacovm commented Oct 5, 2025 •

edited

Loading