vms/platformvm: Remove double block building logic

vms/platformvm/block/builder/builder.go

@@ -13,7 +13,6 @@ import (
 
 	"github.com/ava-labs/avalanchego/ids"
 	"github.com/ava-labs/avalanchego/snow/consensus/snowman"
-	"github.com/ava-labs/avalanchego/snow/engine/common"
 	"github.com/ava-labs/avalanchego/utils/timer"
 	"github.com/ava-labs/avalanchego/utils/timer/mockable"
 	"github.com/ava-labs/avalanchego/utils/units"
@@ -40,10 +39,12 @@ var (
 
 type Builder interface {
 	mempool.Mempool
-	mempool.BlockTimer
 
-	// BuildBlock is called on timer clock to attempt to create
-	// next block
+	// ResetBlockTimer schedules a timer to notify the consensus engine once a
+	// block needs to be built to process a staker change.
+	ResetBlockTimer()
+
+	// BuildBlock can be called to attempt to create a new block
 	BuildBlock(context.Context) (snowman.Block, error)
 
 	// Shutdown cleanly shuts Builder down
@@ -58,28 +59,24 @@ type builder struct {
 	txExecutorBackend *txexecutor.Backend
 	blkManager        blockexecutor.Manager
 
-	// channel to send messages to the consensus engine
-	toEngine chan<- common.Message
-
-	// This timer goes off when it is time for the next validator to add/leave
-	// the validator set. When it goes off ResetTimer() is called, potentially
-	// triggering creation of a new block.
-	timer *timer.Timer
+	// This timer goes off when it is time for the next staker to add/leave
+	// the staking set. When it goes off, [setNextBlockBuildTime()] is called,
+	// potentially triggering creation of a new block.
+	timer                *timer.Timer
+	nextStakerChangeTime time.Time
 }
 
 func New(
 	mempool mempool.Mempool,
 	txBuilder txbuilder.Builder,
 	txExecutorBackend *txexecutor.Backend,
 	blkManager blockexecutor.Manager,
-	toEngine chan<- common.Message,
 ) Builder {
 	builder := &builder{
 		Mempool:           mempool,
 		txBuilder:         txBuilder,
 		txExecutorBackend: txExecutorBackend,
 		blkManager:        blkManager,
-		toEngine:          toEngine,
 	}
 
 	builder.timer = timer.NewTimer(builder.setNextBuildBlockTime)
@@ -94,28 +91,14 @@ func New(
 func (b *builder) BuildBlock(context.Context) (snowman.Block, error) {
 	b.Mempool.DisableAdding()
 	defer func() {
+		b.Mempool.RequestBuildBlock(false /*=emptyBlockPermitted*/)
 		b.Mempool.EnableAdding()
 		b.ResetBlockTimer()
 	}()
 
 	ctx := b.txExecutorBackend.Ctx
 	ctx.Log.Debug("starting to attempt to build a block")
 
-	statelessBlk, err := b.buildBlock()
-	if err != nil {
-		return nil, err
-	}
-
-	// Remove selected txs from mempool now that we are returning the block to
-	// the consensus engine.
-	txs := statelessBlk.Txs()
-	b.Mempool.Remove(txs)
-	return b.blkManager.NewBlock(statelessBlk), nil
-}
-
-// Returns the block we want to build and issue.
-// Only modifies state to remove expired proposal txs.
-func (b *builder) buildBlock() (block.Block, error) {
 	// Get the block to build on top of and retrieve the new block's context.
 	preferredID := b.blkManager.Preferred()
 	preferred, err := b.blkManager.GetBlock(preferredID)
@@ -150,14 +133,23 @@ func (b *builder) buildBlock() (block.Block, error) {
 	}
 	// [timestamp] = min(max(now, parentTime), nextStakerChangeTime)
 
-	return buildBlock(
+	statelessBlk, err := buildBlock(
 		b,
 		preferredID,
 		nextHeight,
 		timestamp,
 		timeWasCapped,
 		preferredState,
 	)
+	if err != nil {
+		return nil, err
+	}
+
+	// Remove selected txs from mempool now that we are returning the block to
+	// the consensus engine.
+	txs := statelessBlk.Txs()
+	b.Mempool.Remove(txs)
+	return b.blkManager.NewBlock(statelessBlk), nil
 }
 
 func (b *builder) Shutdown() {
@@ -190,10 +182,10 @@ func (b *builder) setNextBuildBlockTime() {
 		return
 	}
 
-	if _, err := b.buildBlock(); err == nil {
-		// We can build a block now
-		b.notifyBlockReady()
-		return
+	now := b.txExecutorBackend.Clk.Time()
+	if !b.nextStakerChangeTime.After(now) {
+		// Block needs to be issued to advance time.
+		b.Mempool.RequestBuildBlock(true /*=emptyBlockPermitted*/)
 	}
 
 	// Wake up when it's time to add/remove the next validator/delegator
@@ -218,27 +210,17 @@ func (b *builder) setNextBuildBlockTime() {
 		return
 	}
 
-	now := b.txExecutorBackend.Clk.Time()
 	waitTime := nextStakerChangeTime.Sub(now)
 	ctx.Log.Debug("setting next scheduled event",
 		zap.Time("nextEventTime", nextStakerChangeTime),
 		zap.Duration("timeUntil", waitTime),
 	)
 
 	// Wake up when it's time to add/remove the next validator
+	b.nextStakerChangeTime = nextStakerChangeTime
 	b.timer.SetTimeoutIn(waitTime)
 }
 
-// notifyBlockReady tells the consensus engine that a new block is ready to be
-// created
-func (b *builder) notifyBlockReady() {
-	select {
-	case b.toEngine <- common.PendingTxs:
-	default:
-		b.txExecutorBackend.Ctx.Log.Debug("dropping message to consensus engine")
-	}
-}
-
 // [timestamp] is min(max(now, parent timestamp), next staker change time)
 func buildBlock(
 	builder *builder,

vms/platformvm/block/builder/helpers_test.go

@@ -169,7 +169,7 @@ func newEnvironment(t *testing.T) *environment {
 	metrics, err := metrics.New("", registerer)
 	require.NoError(err)
 
-	res.mempool, err = mempool.New("mempool", registerer, res)
+	res.mempool, err = mempool.New("mempool", registerer, nil)
 	require.NoError(err)
 
 	res.blkManager = blockexecutor.NewManager(
@@ -193,7 +193,6 @@ func newEnvironment(t *testing.T) *environment {
 		res.txBuilder,
 		&res.backend,
 		res.blkManager,
-		nil, // toEngine,
 	)
 
 	res.blkManager.SetPreference(genesisID)

vms/platformvm/block/executor/helpers_test.go

@@ -63,8 +63,6 @@ const (
 )
 
 var (
-	_ mempool.BlockTimer = (*environment)(nil)
-
 	defaultMinStakingDuration = 24 * time.Hour
 	defaultMaxStakingDuration = 365 * 24 * time.Hour
 	defaultGenesisTime        = time.Date(1997, 1, 1, 0, 0, 0, 0, time.UTC)
@@ -131,10 +129,6 @@ type environment struct {
 	backend        *executor.Backend
 }
 
-func (*environment) ResetBlockTimer() {
-	// dummy call, do nothing for now
-}
-
 func newEnvironment(t *testing.T, ctrl *gomock.Controller) *environment {
 	res := &environment{
 		isBootstrapped: &utils.Atomic[bool]{},
@@ -199,7 +193,7 @@ func newEnvironment(t *testing.T, ctrl *gomock.Controller) *environment {
 	metrics := metrics.Noop
 
 	var err error
-	res.mempool, err = mempool.New("mempool", registerer, res)
+	res.mempool, err = mempool.New("mempool", registerer, nil)
 	if err != nil {
 		panic(fmt.Errorf("failed to create mempool: %w", err))
 	}

vms/platformvm/block/executor/rejector.go

@@ -82,5 +82,7 @@ func (r *rejector) rejectBlock(b block.Block, blockType string) error {
 		}
 	}
 
+	r.Mempool.RequestBuildBlock(false /*=emptyBlockPermitted*/)
+
 	return nil
 }

vms/platformvm/block/executor/rejector_test.go

@@ -142,6 +142,8 @@ func TestRejectBlock(t *testing.T) {
 				mempool.EXPECT().Add(tx).Return(nil).Times(1)
 			}
 
+			mempool.EXPECT().RequestBuildBlock(false).Times(1)
+
 			require.NoError(tt.rejectFunc(rejector, blk))
 			// Make sure block and its parent are removed from the state map.
 			require.NotContains(rejector.blkIDToState, blk.ID())

vms/platformvm/network/network.go

@@ -181,6 +181,8 @@ func (n *network) issueTx(tx *txs.Tx) error {
 		return err
 	}
 
+	n.mempool.RequestBuildBlock(false /*=emptyBlockPermitted*/)
+
 	return nil
 }
 

vms/platformvm/network/network_test.go

@@ -284,6 +284,7 @@ func TestNetworkIssueTx(t *testing.T) {
 				mempool := mempool.NewMockMempool(ctrl)
 				mempool.EXPECT().Has(gomock.Any()).Return(false)
 				mempool.EXPECT().Add(gomock.Any()).Return(nil)
+				mempool.EXPECT().RequestBuildBlock(false)
 				return mempool
 			},
 			managerFunc: func(ctrl *gomock.Controller) executor.Manager {

vms/platformvm/txs/mempool/mempool.go

@@ -12,6 +12,7 @@ import (
 
 	"github.com/ava-labs/avalanchego/cache"
 	"github.com/ava-labs/avalanchego/ids"
+	"github.com/ava-labs/avalanchego/snow/engine/common"
 	"github.com/ava-labs/avalanchego/utils/linkedhashmap"
 	"github.com/ava-labs/avalanchego/utils/set"
 	"github.com/ava-labs/avalanchego/utils/units"
@@ -43,13 +44,6 @@ var (
 	errCantIssueRewardValidatorTx = errors.New("can not issue a reward validator tx")
 )
 
-type BlockTimer interface {
-	// ResetBlockTimer schedules a timer to notify the consensus engine once
-	// there is a block ready to be built. If a block is ready to be built when
-	// this function is called, the engine will be notified directly.
-	ResetBlockTimer()
-}
-
 type Mempool interface {
 	// we may want to be able to stop valid transactions
 	// from entering the mempool, e.g. during blocks creation
@@ -75,6 +69,13 @@ type Mempool interface {
 	// TODO: Remove once [StartTime] field is ignored in staker txs
 	DropExpiredStakerTxs(minStartTime time.Time) []ids.ID
 
+	// RequestBuildBlock notifies the consensus engine that a block should be
+	// built. If [emptyBlockPermitted] is true, the notification will be sent
+	// regardless of whether there are no transactions in the mempool. If not,
+	// a notification will only be sent if there is at least one transaction in
+	// the mempool.
+	RequestBuildBlock(emptyBlockPermitted bool)
+
 	// Note: dropped txs are added to droppedTxIDs but are not evicted from
 	// unissued decision/staker txs. This allows previously dropped txs to be
 	// possibly reissued.
@@ -100,13 +101,13 @@ type mempool struct {
 
 	consumedUTXOs set.Set[ids.ID]
 
-	blkTimer BlockTimer
+	toEngine chan<- common.Message
 }
 
 func New(
 	namespace string,
 	registerer prometheus.Registerer,
-	blkTimer BlockTimer,
+	toEngine chan<- common.Message,
 ) (Mempool, error) {
 	bytesAvailableMetric := prometheus.NewGauge(prometheus.GaugeOpts{
 		Namespace: namespace,
@@ -137,7 +138,7 @@ func New(
 		droppedTxIDs:  &cache.LRU[ids.ID, error]{Size: droppedTxIDsCacheSize},
 		consumedUTXOs: set.NewSet[ids.ID](initialConsumedUTXOsSize),
 		dropIncoming:  false, // enable tx adding by default
-		blkTimer:      blkTimer,
+		toEngine:      toEngine,
 	}, nil
 }
 
@@ -202,7 +203,6 @@ func (m *mempool) Add(tx *txs.Tx) error {
 	// An explicitly added tx must not be marked as dropped.
 	m.droppedTxIDs.Evict(txID)
 
-	m.blkTimer.ResetBlockTimer()
 	return nil
 }
 
@@ -259,6 +259,17 @@ func (m *mempool) GetDropReason(txID ids.ID) error {
 	return err
 }
 
+func (m *mempool) RequestBuildBlock(emptyBlockPermitted bool) {
+	if !emptyBlockPermitted && !m.HasTxs() {
+		return
+	}
+
+	select {
+	case m.toEngine <- common.PendingTxs:
+	default:
+	}
+}
+
 // Drops all [txs.Staker] transactions whose [StartTime] is before
 // [minStartTime] from [mempool]. The dropped tx ids are returned.
 //

vms/platformvm/txs/mempool/mempool_test.go

@@ -20,23 +20,15 @@ import (
 	"github.com/ava-labs/avalanchego/vms/secp256k1fx"
 )
 
-var (
-	_ BlockTimer = (*noopBlkTimer)(nil)
-
-	preFundedKeys = secp256k1.TestKeys()
-)
-
-type noopBlkTimer struct{}
-
-func (*noopBlkTimer) ResetBlockTimer() {}
+var preFundedKeys = secp256k1.TestKeys()
 
 // shows that valid tx is not added to mempool if this would exceed its maximum
 // size
 func TestBlockBuilderMaxMempoolSizeHandling(t *testing.T) {
 	require := require.New(t)
 
 	registerer := prometheus.NewRegistry()
-	mpool, err := New("mempool", registerer, &noopBlkTimer{})
+	mpool, err := New("mempool", registerer, nil)
 	require.NoError(err)
 
 	decisionTxs, err := createTestDecisionTxs(1)
@@ -60,7 +52,7 @@ func TestDecisionTxsInMempool(t *testing.T) {
 	require := require.New(t)
 
 	registerer := prometheus.NewRegistry()
-	mpool, err := New("mempool", registerer, &noopBlkTimer{})
+	mpool, err := New("mempool", registerer, nil)
 	require.NoError(err)
 
 	decisionTxs, err := createTestDecisionTxs(2)
@@ -112,7 +104,7 @@ func TestProposalTxsInMempool(t *testing.T) {
 	require := require.New(t)
 
 	registerer := prometheus.NewRegistry()
-	mpool, err := New("mempool", registerer, &noopBlkTimer{})
+	mpool, err := New("mempool", registerer, nil)
 	require.NoError(err)
 
 	// The proposal txs are ordered by decreasing start time. This means after
@@ -245,7 +237,7 @@ func TestDropExpiredStakerTxs(t *testing.T) {
 	require := require.New(t)
 
 	registerer := prometheus.NewRegistry()
-	mempool, err := New("mempool", registerer, &noopBlkTimer{})
+	mempool, err := New("mempool", registerer, nil)
 	require.NoError(err)
 
 	tx1, err := generateAddValidatorTx(10, 20)