vms/platformvm: Remove double block building logic

vms/platformvm/block/builder/builder.go

@@ -40,13 +40,11 @@ var (
 type Builder interface {
 	mempool.Mempool
 
-	// 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 schedules a timer to notify the consensus engine once a
+	// block needs to be built to process a staker change.
 	ResetBlockTimer()
 
-	// BuildBlock is called on timer clock to attempt to create
-	// next block
+	// BuildBlock can be called to attempt to create a new block
 	BuildBlock(context.Context) (snowman.Block, error)
 
 	// Shutdown cleanly shuts Builder down
@@ -61,10 +59,11 @@ type builder struct {
 	txExecutorBackend *txexecutor.Backend
 	blkManager        blockexecutor.Manager
 
-	// 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, [maybeAdvanceTime()] is called,
+	// potentially triggering creation of a new block.
+	timer                *timer.Timer
+	nextStakerChangeTime time.Time
 }
 
 func New(
@@ -80,7 +79,7 @@ func New(
 		blkManager:        blkManager,
 	}
 
-	builder.timer = timer.NewTimer(builder.setNextBuildBlockTime)
+	builder.timer = timer.NewTimer(builder.maybeAdvanceTime)
 
 	go txExecutorBackend.Ctx.Log.RecoverAndPanic(builder.timer.Dispatch)
 	return builder
@@ -93,27 +92,12 @@ func (b *builder) BuildBlock(context.Context) (snowman.Block, error) {
 	b.Mempool.DisableAdding()
 	defer func() {
 		b.Mempool.EnableAdding()
-		b.ResetBlockTimer()
+		b.Mempool.RequestBuildBlock(false /*=emptyBlockPermitted*/)
 	}()
 
 	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)
@@ -126,7 +110,8 @@ func (b *builder) buildBlock() (block.Block, error) {
 		return nil, fmt.Errorf("%w: %s", state.ErrMissingParentState, preferredID)
 	}
 
-	timestamp := b.txExecutorBackend.Clk.Time()
+	now := b.txExecutorBackend.Clk.Time()
+	timestamp := now
 	if parentTime := preferred.Timestamp(); parentTime.After(timestamp) {
 		timestamp = parentTime
 	}
@@ -137,6 +122,16 @@ func (b *builder) buildBlock() (block.Block, error) {
 		return nil, fmt.Errorf("could not calculate next staker change time: %w", err)
 	}
 
+	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)
+
 	// timeWasCapped means that [timestamp] was reduced to
 	// [nextStakerChangeTime]. It is used as a flag for [buildApricotBlock] to
 	// be willing to issue an advanceTimeTx. It is also used as a flag for
@@ -148,14 +143,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() {
@@ -173,7 +177,7 @@ func (b *builder) ResetBlockTimer() {
 	b.timer.SetTimeoutIn(0)
 }
 
-func (b *builder) setNextBuildBlockTime() {
+func (b *builder) maybeAdvanceTime() {
 	ctx := b.txExecutorBackend.Ctx
 
 	// Grabbing the lock here enforces that this function is not called mid-way
@@ -188,43 +192,14 @@ func (b *builder) setNextBuildBlockTime() {
 		return
 	}
 
-	if _, err := b.buildBlock(); err == nil {
-		// We can build a block now
-		b.Mempool.RequestBuildBlock(true /*=emptyBlockPermitted*/)
-		return
-	}
-
-	// Wake up when it's time to add/remove the next validator/delegator
-	preferredID := b.blkManager.Preferred()
-	preferredState, ok := b.blkManager.GetState(preferredID)
-	if !ok {
-		// The preferred block should always be a decision block
-		ctx.Log.Error("couldn't get preferred block state",
-			zap.Stringer("preferredID", preferredID),
-			zap.Stringer("lastAcceptedID", b.blkManager.LastAccepted()),
-		)
-		return
-	}
-
-	nextStakerChangeTime, err := txexecutor.GetNextStakerChangeTime(preferredState)
-	if err != nil {
-		ctx.Log.Error("couldn't get next staker change time",
-			zap.Stringer("preferredID", preferredID),
-			zap.Stringer("lastAcceptedID", b.blkManager.LastAccepted()),
-			zap.Error(err),
-		)
+	now := b.txExecutorBackend.Clk.Time()
+	if b.nextStakerChangeTime.After(now) {
+		// [nextStakerChangeTime] is in the future, no need to advance time.
 		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.timer.SetTimeoutIn(waitTime)
+	// Block needs to be issued to advance time.
+	b.Mempool.RequestBuildBlock(true /*=emptyBlockPermitted*/)
 }
 
 // [timestamp] is min(max(now, parent timestamp), next staker change time)

vms/platformvm/block/executor/rejector.go

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

vms/platformvm/network/network.go

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