Move management of platformvm preferred block to executor.Manager (#…

…2292)

vms/platformvm/block/builder/builder.go

@@ -44,12 +44,6 @@ type Builder interface {
 	mempool.BlockTimer
 	Network
 
-	// set preferred block on top of which we'll build next
-	SetPreference(blockID ids.ID)
-
-	// get preferred block on top of which we'll build next
-	Preferred() (snowman.Block, error)
-
 	// AddUnverifiedTx verifier the tx before adding it to mempool
 	AddUnverifiedTx(tx *txs.Tx) error
 
@@ -70,9 +64,6 @@ type builder struct {
 	txExecutorBackend *txexecutor.Backend
 	blkManager        blockexecutor.Manager
 
-	// ID of the preferred block to build on top of
-	preferredBlockID ids.ID
-
 	// channel to send messages to the consensus engine
 	toEngine chan<- common.Message
 
@@ -110,19 +101,6 @@ func New(
 	return builder
 }
 
-func (b *builder) SetPreference(blockID ids.ID) {
-	if blockID == b.preferredBlockID {
-		// If the preference didn't change, then this is a noop
-		return
-	}
-	b.preferredBlockID = blockID
-	b.ResetBlockTimer()
-}
-
-func (b *builder) Preferred() (snowman.Block, error) {
-	return b.blkManager.GetBlock(b.preferredBlockID)
-}
-
 // AddUnverifiedTx verifies a transaction and attempts to add it to the mempool
 func (b *builder) AddUnverifiedTx(tx *txs.Tx) error {
 	if !b.txExecutorBackend.Bootstrapped.Get() {
@@ -138,7 +116,7 @@ func (b *builder) AddUnverifiedTx(tx *txs.Tx) error {
 
 	verifier := txexecutor.MempoolTxVerifier{
 		Backend:       b.txExecutorBackend,
-		ParentID:      b.preferredBlockID, // We want to build off of the preferred block
+		ParentID:      b.blkManager.Preferred(), // We want to build off of the preferred block
 		StateVersions: b.blkManager,
 		Tx:            tx,
 	}
@@ -186,11 +164,11 @@ func (b *builder) BuildBlock(context.Context) (snowman.Block, error) {
 // 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.
-	preferred, err := b.Preferred()
+	preferredID := b.blkManager.Preferred()
+	preferred, err := b.blkManager.GetBlock(preferredID)
 	if err != nil {
 		return nil, err
 	}
-	preferredID := preferred.ID()
 	nextHeight := preferred.Height() + 1
 	preferredState, ok := b.blkManager.GetState(preferredID)
 	if !ok {
@@ -266,11 +244,12 @@ func (b *builder) setNextBuildBlockTime() {
 	}
 
 	// Wake up when it's time to add/remove the next validator/delegator
-	preferredState, ok := b.blkManager.GetState(b.preferredBlockID)
+	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", b.preferredBlockID),
+			zap.Stringer("preferredID", preferredID),
 			zap.Stringer("lastAcceptedID", b.blkManager.LastAccepted()),
 		)
 		return
@@ -279,7 +258,7 @@ func (b *builder) setNextBuildBlockTime() {
 	nextStakerChangeTime, err := txexecutor.GetNextStakerChangeTime(preferredState)
 	if err != nil {
 		ctx.Log.Error("couldn't get next staker change time",
-			zap.Stringer("preferredID", b.preferredBlockID),
+			zap.Stringer("preferredID", preferredID),
 			zap.Stringer("lastAcceptedID", b.blkManager.LastAccepted()),
 			zap.Error(err),
 		)

vms/platformvm/block/builder/builder_test.go

@@ -110,7 +110,7 @@ func TestNoErrorOnUnexpectedSetPreferenceDuringBootstrapping(t *testing.T) {
 		require.NoError(t, shutdownEnvironment(env))
 	}()
 
-	env.Builder.SetPreference(ids.GenerateTestID()) // should not panic
+	require.False(t, env.blkManager.SetPreference(ids.GenerateTestID())) // should not panic
 }
 
 func TestGetNextStakerToReward(t *testing.T) {

vms/platformvm/block/builder/helpers_test.go

@@ -177,7 +177,7 @@ func newEnvironment(t *testing.T) *environment {
 		res.sender,
 	)
 
-	res.Builder.SetPreference(genesisID)
+	res.blkManager.SetPreference(genesisID)
 	addSubnet(t, res)
 
 	return res

vms/platformvm/block/executor/manager.go

@@ -21,6 +21,10 @@ type Manager interface {
 
 	// Returns the ID of the most recently accepted block.
 	LastAccepted() ids.ID
+
+	SetPreference(blkID ids.ID) (updated bool)
+	Preferred() ids.ID
+
 	GetBlock(blkID ids.ID) (snowman.Block, error)
 	GetStatelessBlock(blkID ids.ID) (block.Block, error)
 	NewBlock(block.Block) snowman.Block
@@ -33,9 +37,10 @@ func NewManager(
 	txExecutorBackend *executor.Backend,
 	validatorManager validators.Manager,
 ) Manager {
+	lastAccepted := s.GetLastAccepted()
 	backend := &backend{
 		Mempool:      mempool,
-		lastAccepted: s.GetLastAccepted(),
+		lastAccepted: lastAccepted,
 		state:        s,
 		ctx:          txExecutorBackend.Ctx,
 		blkIDToState: map[ids.ID]*blockState{},
@@ -57,6 +62,7 @@ func NewManager(
 			backend:         backend,
 			addTxsToMempool: !txExecutorBackend.Config.PartialSyncPrimaryNetwork,
 		},
+		preferred: lastAccepted,
 	}
 }
 
@@ -65,6 +71,8 @@ type manager struct {
 	verifier block.Visitor
 	acceptor block.Visitor
 	rejector block.Visitor
+
+	preferred ids.ID
 }
 
 func (m *manager) GetBlock(blkID ids.ID) (snowman.Block, error) {
@@ -85,3 +93,13 @@ func (m *manager) NewBlock(blk block.Block) snowman.Block {
 		Block:   blk,
 	}
 }
+
+func (m *manager) SetPreference(blockID ids.ID) (updated bool) {
+	updated = m.preferred == blockID
+	m.preferred = blockID
+	return updated
+}
+
+func (m *manager) Preferred() ids.ID {
+	return m.preferred
+}

vms/platformvm/service.go

@@ -1943,11 +1943,8 @@ func (s *Service) GetBlockchainStatus(r *http.Request, args *GetBlockchainStatus
 		return nil
 	}
 
-	preferredBlk, err := s.vm.Preferred()
-	if err != nil {
-		return fmt.Errorf("could not retrieve preferred block, err %w", err)
-	}
-	preferred, err := s.chainExists(ctx, preferredBlk.ID(), blockchainID)
+	preferredBlkID := s.vm.manager.Preferred()
+	preferred, err := s.chainExists(ctx, preferredBlkID, blockchainID)
 	if err != nil {
 		return fmt.Errorf("problem looking up blockchain: %w", err)
 	}
@@ -2246,12 +2243,7 @@ func (s *Service) GetTxStatus(_ *http.Request, args *GetTxStatusArgs, response *
 
 	// The status of this transaction is not in the database - check if the tx
 	// is in the preferred block's db. If so, return that it's processing.
-	prefBlk, err := s.vm.Preferred()
-	if err != nil {
-		return err
-	}
-
-	preferredID := prefBlk.ID()
+	preferredID := s.vm.manager.Preferred()
 	onAccept, ok := s.vm.manager.GetState(preferredID)
 	if !ok {
 		return fmt.Errorf("could not retrieve state for block %s", preferredID)

vms/platformvm/service_test.go

@@ -784,7 +784,8 @@ func TestGetBlock(t *testing.T) {
 			)
 			require.NoError(err)
 
-			preferred, err := service.vm.Builder.Preferred()
+			preferredID := service.vm.manager.Preferred()
+			preferred, err := service.vm.manager.GetBlock(preferredID)
 			require.NoError(err)
 
 			statelessBlock, err := block.NewBanffStandardBlock(

vms/platformvm/vm.go

@@ -393,7 +393,9 @@ func (vm *VM) LastAccepted(context.Context) (ids.ID, error) {
 
 // SetPreference sets the preferred block to be the one with ID [blkID]
 func (vm *VM) SetPreference(_ context.Context, blkID ids.ID) error {
-	vm.Builder.SetPreference(blkID)
+	if vm.manager.SetPreference(blkID) {
+		vm.Builder.ResetBlockTimer()
+	}
 	return nil
 }
 

vms/platformvm/vm_regression_test.go

@@ -410,11 +410,10 @@ func TestUnverifiedParentPanicRegression(t *testing.T) {
 	)
 	require.NoError(err)
 
-	preferred, err := vm.Builder.Preferred()
+	preferredID := vm.manager.Preferred()
+	preferred, err := vm.manager.GetBlock(preferredID)
 	require.NoError(err)
-
 	preferredChainTime := preferred.Timestamp()
-	preferredID := preferred.ID()
 	preferredHeight := preferred.Height()
 
 	statelessStandardBlk, err := block.NewBanffStandardBlock(
@@ -493,11 +492,10 @@ func TestRejectedStateRegressionInvalidValidatorTimestamp(t *testing.T) {
 	require.NoError(err)
 
 	// Create the standard block to add the new validator
-	preferred, err := vm.Builder.Preferred()
+	preferredID := vm.manager.Preferred()
+	preferred, err := vm.manager.GetBlock(preferredID)
 	require.NoError(err)
-
 	preferredChainTime := preferred.Timestamp()
-	preferredID := preferred.ID()
 	preferredHeight := preferred.Height()
 
 	statelessBlk, err := block.NewBanffStandardBlock(
@@ -706,11 +704,10 @@ func TestRejectedStateRegressionInvalidValidatorReward(t *testing.T) {
 	require.NoError(err)
 
 	// Create the standard block to add the first new validator
-	preferred, err := vm.Builder.Preferred()
+	preferredID := vm.manager.Preferred()
+	preferred, err := vm.manager.GetBlock(preferredID)
 	require.NoError(err)
-
 	preferredChainTime := preferred.Timestamp()
-	preferredID := preferred.ID()
 	preferredHeight := preferred.Height()
 
 	statelessAddValidatorStandardBlk0, err := block.NewBanffStandardBlock(
@@ -1044,11 +1041,10 @@ func TestValidatorSetAtCacheOverwriteRegression(t *testing.T) {
 	require.NoError(err)
 
 	// Create the standard block to add the first new validator
-	preferred, err := vm.Builder.Preferred()
+	preferredID := vm.manager.Preferred()
+	preferred, err := vm.manager.GetBlock(preferredID)
 	require.NoError(err)
-
 	preferredChainTime := preferred.Timestamp()
-	preferredID := preferred.ID()
 	preferredHeight := preferred.Height()
 
 	statelessStandardBlk, err := block.NewBanffStandardBlock(
@@ -1081,7 +1077,8 @@ func TestValidatorSetAtCacheOverwriteRegression(t *testing.T) {
 
 	// Create the standard block that moves the first new validator from the
 	// pending validator set into the current validator set.
-	preferred, err = vm.Builder.Preferred()
+	preferredID = vm.manager.Preferred()
+	preferred, err = vm.manager.GetBlock(preferredID)
 	require.NoError(err)
 	preferredID = preferred.ID()
 	preferredHeight = preferred.Height()