Add TxVerifier interface to network

vms/platformvm/block/builder/builder_test.go

@@ -52,7 +52,9 @@ func TestBuildBlockBasic(t *testing.T) {
 	txID := tx.ID()
 
 	// Issue the transaction
+	env.ctx.Lock.Unlock()
 	require.NoError(env.network.IssueTx(context.Background(), tx))
+	env.ctx.Lock.Lock()
 	_, ok := env.mempool.Get(txID)
 	require.True(ok)
 
@@ -125,7 +127,9 @@ func TestBuildBlockShouldReward(t *testing.T) {
 	txID := tx.ID()
 
 	// Issue the transaction
+	env.ctx.Lock.Unlock()
 	require.NoError(env.network.IssueTx(context.Background(), tx))
+	env.ctx.Lock.Lock()
 	_, ok := env.mempool.Get(txID)
 	require.True(ok)
 
@@ -249,7 +253,9 @@ func TestBuildBlockForceAdvanceTime(t *testing.T) {
 	txID := tx.ID()
 
 	// Issue the transaction
+	env.ctx.Lock.Unlock()
 	require.NoError(env.network.IssueTx(context.Background(), tx))
+	env.ctx.Lock.Lock()
 	_, ok := env.mempool.Get(txID)
 	require.True(ok)
 
@@ -471,6 +477,7 @@ func TestPreviouslyDroppedTxsCanBeReAddedToMempool(t *testing.T) {
 	env := newEnvironment(t)
 	env.ctx.Lock.Lock()
 	defer func() {
+		env.ctx.Lock.Lock()
 		require.NoError(shutdownEnvironment(env))
 		env.ctx.Lock.Unlock()
 	}()
@@ -500,6 +507,7 @@ func TestPreviouslyDroppedTxsCanBeReAddedToMempool(t *testing.T) {
 	require.ErrorIs(reason, errTestingDropped)
 
 	// Issue the transaction
+	env.ctx.Lock.Unlock()
 	require.NoError(env.network.IssueTx(context.Background(), tx))
 	_, ok := env.mempool.Get(txID)
 	require.True(ok)

vms/platformvm/block/builder/helpers_test.go

@@ -181,9 +181,10 @@ func newEnvironment(t *testing.T) *environment {
 		pvalidators.TestManager,
 	)
 
+	txVerifier := network.NewLockedTxVerifier(&res.ctx.Lock, res.blkManager)
 	res.network = network.New(
 		res.backend.Ctx,
-		res.blkManager,
+		txVerifier,
 		res.mempool,
 		res.backend.Config.PartialSyncPrimaryNetwork,
 		res.sender,

vms/platformvm/network/network.go

@@ -14,7 +14,6 @@ import (
 	"github.com/ava-labs/avalanchego/snow"
 	"github.com/ava-labs/avalanchego/snow/engine/common"
 	"github.com/ava-labs/avalanchego/vms/components/message"
-	"github.com/ava-labs/avalanchego/vms/platformvm/block/executor"
 	"github.com/ava-labs/avalanchego/vms/platformvm/txs"
 	"github.com/ava-labs/avalanchego/vms/platformvm/txs/mempool"
 )
@@ -30,8 +29,6 @@ type Network interface {
 
 	// IssueTx verifies the transaction at the currently preferred state, adds
 	// it to the mempool, and gossips it to the network.
-	//
-	// Invariant: Assumes the context lock is held.
 	IssueTx(context.Context, *txs.Tx) error
 }
 
@@ -40,7 +37,7 @@ type network struct {
 	common.AppHandler
 
 	ctx                       *snow.Context
-	manager                   executor.Manager
+	txVerifier                TxVerifier
 	mempool                   mempool.Mempool
 	partialSyncPrimaryNetwork bool
 	appSender                 common.AppSender
@@ -52,7 +49,7 @@ type network struct {
 
 func New(
 	ctx *snow.Context,
-	manager executor.Manager,
+	txVerifier TxVerifier,
 	mempool mempool.Mempool,
 	partialSyncPrimaryNetwork bool,
 	appSender common.AppSender,
@@ -61,7 +58,7 @@ func New(
 		AppHandler: common.NewNoOpAppHandler(ctx.Log),
 
 		ctx:                       ctx,
-		manager:                   manager,
+		txVerifier:                txVerifier,
 		mempool:                   mempool,
 		partialSyncPrimaryNetwork: partialSyncPrimaryNetwork,
 		appSender:                 appSender,
@@ -109,14 +106,6 @@ func (n *network) AppGossip(ctx context.Context, nodeID ids.NodeID, msgBytes []b
 	}
 	txID := tx.ID()
 
-	// We need to grab the context lock here to avoid racy behavior with
-	// transaction verification + mempool modifications.
-	//
-	// Invariant: tx should not be referenced again without the context lock
-	// held to avoid any data races.
-	n.ctx.Lock.Lock()
-	defer n.ctx.Lock.Unlock()
-
 	if reason := n.mempool.GetDropReason(txID); reason != nil {
 		// If the tx is being dropped - just ignore it
 		return nil
@@ -155,7 +144,11 @@ func (n *network) issueTx(tx *txs.Tx) error {
 	}
 
 	// Verify the tx at the currently preferred state
-	if err := n.manager.VerifyTx(tx); err != nil {
+	//
+	// We need to grab the context lock here to avoid racy behavior with
+	// transaction verification + mempool modifications.
+	err := n.txVerifier.VerifyTx(tx)
+	if err != nil {
 		n.ctx.Log.Debug("tx failed verification",
 			zap.Stringer("txID", txID),
 			zap.Error(err),

vms/platformvm/network/network_test.go

@@ -18,13 +18,22 @@ import (
 	"github.com/ava-labs/avalanchego/utils/logging"
 	"github.com/ava-labs/avalanchego/vms/components/avax"
 	"github.com/ava-labs/avalanchego/vms/components/message"
-	"github.com/ava-labs/avalanchego/vms/platformvm/block/executor"
 	"github.com/ava-labs/avalanchego/vms/platformvm/txs"
 	"github.com/ava-labs/avalanchego/vms/platformvm/txs/mempool"
 )
 
 var errTest = errors.New("test error")
 
+var _ TxVerifier = (*testTxVerifier)(nil)
+
+type testTxVerifier struct {
+	err error
+}
+
+func (t testTxVerifier) VerifyTx(*txs.Tx) error {
+	return t.err
+}
+
 func TestNetworkAppGossip(t *testing.T) {
 	testTx := &txs.Tx{
 		Unsigned: &txs.BaseTx{
@@ -159,7 +168,7 @@ func TestNetworkAppGossip(t *testing.T) {
 				&snow.Context{
 					Log: logging.NoLog{},
 				},
-				executor.NewMockManager(ctrl), // Manager is unused in this test
+				testTxVerifier{},
 				tt.mempoolFunc(ctrl),
 				tt.partialSyncPrimaryNetwork,
 				tt.appSenderFunc(ctrl),
@@ -175,7 +184,7 @@ func TestNetworkIssueTx(t *testing.T) {
 	type test struct {
 		name                      string
 		mempoolFunc               func(*gomock.Controller) mempool.Mempool
-		managerFunc               func(*gomock.Controller) executor.Manager
+		txVerifier                testTxVerifier
 		partialSyncPrimaryNetwork bool
 		appSenderFunc             func(*gomock.Controller) common.AppSender
 		expectedErr               error
@@ -189,10 +198,6 @@ func TestNetworkIssueTx(t *testing.T) {
 				mempool.EXPECT().Get(gomock.Any()).Return(tx, true)
 				return mempool
 			},
-			managerFunc: func(ctrl *gomock.Controller) executor.Manager {
-				// Unused in this test
-				return executor.NewMockManager(ctrl)
-			},
 			appSenderFunc: func(ctrl *gomock.Controller) common.AppSender {
 				// Should gossip the tx
 				appSender := common.NewMockSender(ctrl)
@@ -209,11 +214,7 @@ func TestNetworkIssueTx(t *testing.T) {
 				mempool.EXPECT().MarkDropped(gomock.Any(), gomock.Any())
 				return mempool
 			},
-			managerFunc: func(ctrl *gomock.Controller) executor.Manager {
-				manager := executor.NewMockManager(ctrl)
-				manager.EXPECT().VerifyTx(gomock.Any()).Return(errTest)
-				return manager
-			},
+			txVerifier: testTxVerifier{err: errTest},
 			appSenderFunc: func(ctrl *gomock.Controller) common.AppSender {
 				// Shouldn't gossip the tx
 				return common.NewMockSender(ctrl)
@@ -228,11 +229,7 @@ func TestNetworkIssueTx(t *testing.T) {
 				mempool.EXPECT().MarkDropped(gomock.Any(), gomock.Any())
 				return mempool
 			},
-			managerFunc: func(ctrl *gomock.Controller) executor.Manager {
-				manager := executor.NewMockManager(ctrl)
-				manager.EXPECT().VerifyTx(gomock.Any()).Return(errTest)
-				return manager
-			},
+			txVerifier: testTxVerifier{err: errTest},
 			appSenderFunc: func(ctrl *gomock.Controller) common.AppSender {
 				// Shouldn't gossip the tx
 				return common.NewMockSender(ctrl)
@@ -248,11 +245,6 @@ func TestNetworkIssueTx(t *testing.T) {
 				mempool.EXPECT().MarkDropped(gomock.Any(), errTest)
 				return mempool
 			},
-			managerFunc: func(ctrl *gomock.Controller) executor.Manager {
-				manager := executor.NewMockManager(ctrl)
-				manager.EXPECT().VerifyTx(gomock.Any()).Return(nil)
-				return manager
-			},
 			appSenderFunc: func(ctrl *gomock.Controller) common.AppSender {
 				// Shouldn't gossip the tx
 				return common.NewMockSender(ctrl)
@@ -266,11 +258,6 @@ func TestNetworkIssueTx(t *testing.T) {
 				mempool.EXPECT().Get(gomock.Any()).Return(nil, false)
 				return mempool
 			},
-			managerFunc: func(ctrl *gomock.Controller) executor.Manager {
-				manager := executor.NewMockManager(ctrl)
-				manager.EXPECT().VerifyTx(gomock.Any()).Return(nil)
-				return manager
-			},
 			partialSyncPrimaryNetwork: true,
 			appSenderFunc: func(ctrl *gomock.Controller) common.AppSender {
 				// Should gossip the tx
@@ -289,11 +276,6 @@ func TestNetworkIssueTx(t *testing.T) {
 				mempool.EXPECT().RequestBuildBlock(false)
 				return mempool
 			},
-			managerFunc: func(ctrl *gomock.Controller) executor.Manager {
-				manager := executor.NewMockManager(ctrl)
-				manager.EXPECT().VerifyTx(gomock.Any()).Return(nil)
-				return manager
-			},
 			appSenderFunc: func(ctrl *gomock.Controller) common.AppSender {
 				// Should gossip the tx
 				appSender := common.NewMockSender(ctrl)
@@ -313,7 +295,7 @@ func TestNetworkIssueTx(t *testing.T) {
 				&snow.Context{
 					Log: logging.NoLog{},
 				},
-				tt.managerFunc(ctrl),
+				tt.txVerifier,
 				tt.mempoolFunc(ctrl),
 				tt.partialSyncPrimaryNetwork,
 				tt.appSenderFunc(ctrl),
@@ -334,7 +316,7 @@ func TestNetworkGossipTx(t *testing.T) {
 		&snow.Context{
 			Log: logging.NoLog{},
 		},
-		executor.NewMockManager(ctrl),
+		testTxVerifier{},
 		mempool.NewMockMempool(ctrl),
 		false,
 		appSender,

vms/platformvm/network/tx_verifier.go

@@ -0,0 +1,36 @@
+// Copyright (C) 2019-2023, Ava Labs, Inc. All rights reserved.
+// See the file LICENSE for licensing terms.
+
+package network
+
+import (
+	"sync"
+
+	"github.com/ava-labs/avalanchego/vms/platformvm/txs"
+)
+
+var _ TxVerifier = (*LockedTxVerifier)(nil)
+
+type TxVerifier interface {
+	// VerifyTx verifies that the transaction should be issued into the mempool.
+	VerifyTx(tx *txs.Tx) error
+}
+
+type LockedTxVerifier struct {
+	lock       sync.Locker
+	txVerifier TxVerifier
+}
+
+func (l *LockedTxVerifier) VerifyTx(tx *txs.Tx) error {
+	l.lock.Lock()
+	defer l.lock.Unlock()
+
+	return l.txVerifier.VerifyTx(tx)
+}
+
+func NewLockedTxVerifier(lock sync.Locker, txVerifier TxVerifier) *LockedTxVerifier {
+	return &LockedTxVerifier{
+		lock:       lock,
+		txVerifier: txVerifier,
+	}
+}