Move network implementation to separate package (#2296)

Signed-off-by: Stephen Buttolph <[email protected]>
Co-authored-by: Stephen Buttolph <[email protected]>

vms/platformvm/block/builder/helpers_test.go

@@ -40,6 +40,7 @@ import (
 	"github.com/ava-labs/avalanchego/vms/platformvm/config"
 	"github.com/ava-labs/avalanchego/vms/platformvm/fx"
 	"github.com/ava-labs/avalanchego/vms/platformvm/metrics"
+	"github.com/ava-labs/avalanchego/vms/platformvm/network"
 	"github.com/ava-labs/avalanchego/vms/platformvm/reward"
 	"github.com/ava-labs/avalanchego/vms/platformvm/state"
 	"github.com/ava-labs/avalanchego/vms/platformvm/status"
@@ -97,7 +98,7 @@ type environment struct {
 	Builder
 	blkManager blockexecutor.Manager
 	mempool    mempool.Mempool
-	network    Network
+	network    network.Network
 	sender     *common.SenderTest
 
 	isBootstrapped *utils.Atomic[bool]
@@ -179,7 +180,7 @@ func newEnvironment(t *testing.T) *environment {
 		pvalidators.TestManager,
 	)
 
-	res.network = NewNetwork(
+	res.network = network.New(
 		res.backend.Ctx,
 		res.blkManager,
 		res.mempool,
@@ -437,3 +438,17 @@ func shutdownEnvironment(env *environment) error {
 		env.baseDB.Close(),
 	)
 }
+
+func getValidTx(txBuilder txbuilder.Builder, t *testing.T) *txs.Tx {
+	tx, err := txBuilder.NewCreateChainTx(
+		testSubnet1.ID(),
+		nil,
+		constants.AVMID,
+		nil,
+		"chain name",
+		[]*secp256k1.PrivateKey{testSubnet1ControlKeys[0], testSubnet1ControlKeys[1]},
+		ids.ShortEmpty,
+	)
+	require.NoError(t, err)
+	return tx
+}

vms/platformvm/network/main_test.go

@@ -0,0 +1,14 @@
+// Copyright (C) 2019-2023, Ava Labs, Inc. All rights reserved.
+// See the file LICENSE for licensing terms.
+
+package network
+
+import (
+	"testing"
+
+	"go.uber.org/goleak"
+)
+
+func TestMain(m *testing.M) {
+	goleak.VerifyTestMain(m)
+}

vms/platformvm/block/builder/network.go → vms/platformvm/network/network.go

@@ -1,9 +1,7 @@
 // Copyright (C) 2019-2023, Ava Labs, Inc. All rights reserved.
 // See the file LICENSE for licensing terms.
 
-// TODO: consider moving the network implementation to a separate package
-
-package builder
+package network
 
 import (
 	"context"
@@ -34,7 +32,7 @@ type Network interface {
 	// it to the mempool, and gossips it to the network.
 	//
 	// Invariant: Assumes the context lock is held.
-	IssueTx(ctx context.Context, tx *txs.Tx) error
+	IssueTx(context.Context, *txs.Tx) error
 }
 
 type network struct {
@@ -52,7 +50,7 @@ type network struct {
 	recentTxs     *cache.LRU[ids.ID, struct{}]
 }
 
-func NewNetwork(
+func New(
 	ctx *snow.Context,
 	manager executor.Manager,
 	mempool mempool.Mempool,
@@ -109,60 +107,80 @@ func (n *network) AppGossip(ctx context.Context, nodeID ids.NodeID, msgBytes []b
 		)
 		return nil
 	}
-
 	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
 	}
-
-	// add to mempool
-	if err := n.IssueTx(ctx, tx); err != nil {
-		n.ctx.Log.Debug("tx failed verification",
-			zap.Stringer("nodeID", nodeID),
-			zap.Error(err),
-		)
+	if err := n.issueTx(tx); err == nil {
+		n.gossipTx(ctx, txID, msgBytes)
 	}
 	return nil
 }
 
 func (n *network) IssueTx(ctx context.Context, tx *txs.Tx) error {
+	if err := n.issueTx(tx); err != nil {
+		return err
+	}
+
+	txBytes := tx.Bytes()
+	msg := &message.Tx{
+		Tx: txBytes,
+	}
+	msgBytes, err := message.Build(msg)
+	if err != nil {
+		return err
+	}
+
+	txID := tx.ID()
+	n.gossipTx(ctx, txID, msgBytes)
+	return nil
+}
+
+// returns nil if the tx is in the mempool
+func (n *network) issueTx(tx *txs.Tx) error {
 	txID := tx.ID()
 	if n.mempool.Has(txID) {
-		// If the transaction is already in the mempool - then it looks the same
-		// as if it was successfully added
+		// The tx is already in the mempool
 		return nil
 	}
 
+	// Verify the tx at the currently preferred state
 	if err := n.manager.VerifyTx(tx); err != nil {
+		n.ctx.Log.Debug("tx failed verification",
+			zap.Stringer("txID", txID),
+			zap.Error(err),
+		)
+
 		n.mempool.MarkDropped(txID, err)
 		return err
 	}
 
 	// If we are partially syncing the Primary Network, we should not be
 	// maintaining the transaction mempool locally.
-	if !n.partialSyncPrimaryNetwork {
-		if err := n.mempool.Add(tx); err != nil {
-			return err
-		}
+	if n.partialSyncPrimaryNetwork {
+		return nil
 	}
 
-	txBytes := tx.Bytes()
-	msg := &message.Tx{
-		Tx: txBytes,
-	}
-	msgBytes, err := message.Build(msg)
-	if err != nil {
+	if err := n.mempool.Add(tx); err != nil {
+		n.ctx.Log.Debug("tx failed to be added to the mempool",
+			zap.Stringer("txID", txID),
+			zap.Error(err),
+		)
+
+		n.mempool.MarkDropped(txID, err)
 		return err
 	}
 
-	n.gossipTx(ctx, txID, msgBytes)
 	return nil
 }