Replace error strings with error objects in mempools (ava-labs#2732)

vms/avm/txs/mempool/mempool.go

@@ -58,8 +58,8 @@ type Mempool interface {
 
 	// Note: Dropped txs are added to droppedTxIDs but not evicted from
 	// unissued. This allows previously dropped txs to be possibly reissued.
-	MarkDropped(txID ids.ID, reason string)
-	GetDropReason(txID ids.ID) (string, bool)
+	MarkDropped(txID ids.ID, reason error)
+	GetDropReason(txID ids.ID) error
 }
 
 type mempool struct {
@@ -72,8 +72,8 @@ type mempool struct {
 	toEngine chan<- common.Message
 
 	// Key: Tx ID
-	// Value: String representation of the verification error
-	droppedTxIDs *cache.LRU[ids.ID, string]
+	// Value: Verification error
+	droppedTxIDs *cache.LRU[ids.ID, error]
 
 	consumedUTXOs set.Set[ids.ID]
 }
@@ -108,7 +108,7 @@ func New(
 		unissuedTxs:          linkedhashmap.New[ids.ID, *txs.Tx](),
 		numTxs:               numTxsMetric,
 		toEngine:             toEngine,
-		droppedTxIDs:         &cache.LRU[ids.ID, string]{Size: droppedTxIDsCacheSize},
+		droppedTxIDs:         &cache.LRU[ids.ID, error]{Size: droppedTxIDsCacheSize},
 		consumedUTXOs:        set.NewSet[ids.ID](initialConsumedUTXOsSize),
 	}, nil
 }
@@ -209,10 +209,11 @@ func (m *mempool) RequestBuildBlock() {
 	}
 }
 
-func (m *mempool) MarkDropped(txID ids.ID, reason string) {
+func (m *mempool) MarkDropped(txID ids.ID, reason error) {
 	m.droppedTxIDs.Put(txID, reason)
 }
 
-func (m *mempool) GetDropReason(txID ids.ID) (string, bool) {
-	return m.droppedTxIDs.Get(txID)
+func (m *mempool) GetDropReason(txID ids.ID) error {
+	err, _ := m.droppedTxIDs.Get(txID)
+	return err
 }

vms/platformvm/blocks/builder/builder.go

@@ -143,7 +143,7 @@ func (b *builder) AddUnverifiedTx(tx *txs.Tx) error {
 		Tx:            tx,
 	}
 	if err := tx.Unsigned.Visit(&verifier); err != nil {
-		b.MarkDropped(txID, err.Error())
+		b.MarkDropped(txID, err)
 		return err
 	}
 
@@ -255,17 +255,17 @@ func (b *builder) dropExpiredStakerTxs(timestamp time.Time) {
 		}
 
 		txID := tx.ID()
-		errMsg := fmt.Sprintf(
+		err := fmt.Errorf(
 			"synchrony bound (%s) is later than staker start time (%s)",
 			minStartTime,
 			startTime,
 		)
 
 		b.Mempool.Remove([]*txs.Tx{tx})
-		b.Mempool.MarkDropped(txID, errMsg) // cache tx as dropped
+		b.Mempool.MarkDropped(txID, err) // cache tx as dropped
 		b.txExecutorBackend.Ctx.Log.Debug("dropping tx",
-			zap.String("reason", errMsg),
 			zap.Stringer("txID", txID),
+			zap.Error(err),
 		)
 	}
 }

vms/platformvm/blocks/builder/builder_test.go

@@ -5,6 +5,7 @@ package builder
 
 import (
 	"context"
+	"errors"
 	"testing"
 	"time"
 
@@ -30,6 +31,8 @@ import (
 	txexecutor "github.com/ava-labs/avalanchego/vms/platformvm/txs/executor"
 )
 
+var errTestingDropped = errors.New("testing dropped")
+
 // shows that a locally generated CreateChainTx can be added to mempool and then
 // removed by inclusion in a block
 func TestBlockBuilderAddLocalTx(t *testing.T) {
@@ -83,23 +86,23 @@ func TestPreviouslyDroppedTxsCanBeReAddedToMempool(t *testing.T) {
 	// A tx simply added to mempool is obviously not marked as dropped
 	require.NoError(env.mempool.Add(tx))
 	require.True(env.mempool.Has(txID))
-	_, isDropped := env.mempool.GetDropReason(txID)
-	require.False(isDropped)
+	reason := env.mempool.GetDropReason(txID)
+	require.NoError(reason)
 
 	// When a tx is marked as dropped, it is still available to allow re-issuance
-	env.mempool.MarkDropped(txID, "dropped for testing")
+	env.mempool.MarkDropped(txID, errTestingDropped)
 	require.True(env.mempool.Has(txID)) // still available
-	_, isDropped = env.mempool.GetDropReason(txID)
-	require.True(isDropped)
+	reason = env.mempool.GetDropReason(txID)
+	require.ErrorIs(reason, errTestingDropped)
 
 	// A previously dropped tx, popped then re-added to mempool,
 	// is not dropped anymore
 	env.mempool.Remove([]*txs.Tx{tx})
 	require.NoError(env.mempool.Add(tx))
 
 	require.True(env.mempool.Has(txID))
-	_, isDropped = env.mempool.GetDropReason(txID)
-	require.False(isDropped)
+	reason = env.mempool.GetDropReason(txID)
+	require.NoError(reason)
 }
 
 func TestNoErrorOnUnexpectedSetPreferenceDuringBootstrapping(t *testing.T) {

vms/platformvm/blocks/builder/network.go

@@ -132,7 +132,7 @@ func (n *network) AppGossip(_ context.Context, nodeID ids.NodeID, msgBytes []byt
 	n.ctx.Lock.Lock()
 	defer n.ctx.Lock.Unlock()
 
-	if _, dropped := n.blkBuilder.GetDropReason(txID); dropped {
+	if reason := n.blkBuilder.GetDropReason(txID); reason != nil {
 		// If the tx is being dropped - just ignore it
 		return nil
 	}

vms/platformvm/blocks/builder/network_test.go

@@ -93,7 +93,7 @@ func TestMempoolInvalidGossipedTxIsNotAddedToMempool(t *testing.T) {
 	// create a tx and mark as invalid
 	tx := getValidTx(env.txBuilder, t)
 	txID := tx.ID()
-	env.Builder.MarkDropped(txID, "dropped for testing")
+	env.Builder.MarkDropped(txID, errTestingDropped)
 
 	// show that the invalid tx is not requested
 	nodeID := ids.GenerateTestNodeID()

vms/platformvm/blocks/executor/verifier.go

@@ -196,7 +196,7 @@ func (v *verifier) ApricotAtomicBlock(b *blocks.ApricotAtomicBlock) error {
 
 	if err := b.Tx.Unsigned.Visit(&atomicExecutor); err != nil {
 		txID := b.Tx.ID()
-		v.MarkDropped(txID, err.Error()) // cache tx as dropped
+		v.MarkDropped(txID, err) // cache tx as dropped
 		return fmt.Errorf("tx %s failed semantic verification: %w", txID, err)
 	}
 
@@ -364,7 +364,7 @@ func (v *verifier) proposalBlock(
 
 	if err := b.Tx.Unsigned.Visit(&txExecutor); err != nil {
 		txID := b.Tx.ID()
-		v.MarkDropped(txID, err.Error()) // cache tx as dropped
+		v.MarkDropped(txID, err) // cache tx as dropped
 		return err
 	}
 
@@ -411,7 +411,7 @@ func (v *verifier) standardBlock(
 		}
 		if err := tx.Unsigned.Visit(&txExecutor); err != nil {
 			txID := tx.ID()
-			v.MarkDropped(txID, err.Error()) // cache tx as dropped
+			v.MarkDropped(txID, err) // cache tx as dropped
 			return err
 		}
 		// ensure it doesn't overlap with current input batch

vms/platformvm/service.go

@@ -2149,16 +2149,16 @@ func (s *Service) GetTxStatus(_ *http.Request, args *GetTxStatusArgs, response *
 
 	// Note: we check if tx is dropped only after having looked for it
 	// in the database and the mempool, because dropped txs may be re-issued.
-	reason, dropped := s.vm.Builder.GetDropReason(args.TxID)
-	if !dropped {
+	reason := s.vm.Builder.GetDropReason(args.TxID)
+	if reason == nil {
 		// The tx isn't being tracked by the node.
 		response.Status = status.Unknown
 		return nil
 	}
 
 	// The tx was recently dropped because it was invalid.
 	response.Status = status.Dropped
-	response.Reason = reason
+	response.Reason = reason.Error()
 	return nil
 }
 

vms/platformvm/txs/mempool/mempool.go

@@ -73,8 +73,8 @@ type Mempool interface {
 	// not evicted from unissued decision/staker txs.
 	// This allows previously dropped txs to be possibly
 	// reissued.
-	MarkDropped(txID ids.ID, reason string)
-	GetDropReason(txID ids.ID) (string, bool)
+	MarkDropped(txID ids.ID, reason error)
+	GetDropReason(txID ids.ID) error
 }
 
 // Transactions from clients that have not yet been put into blocks and added to
@@ -90,8 +90,8 @@ type mempool struct {
 	unissuedStakerTxs   txheap.Heap
 
 	// Key: Tx ID
-	// Value: String repr. of the verification error
-	droppedTxIDs *cache.LRU[ids.ID, string]
+	// Value: Verification error
+	droppedTxIDs *cache.LRU[ids.ID, error]
 
 	consumedUTXOs set.Set[ids.ID]
 
@@ -136,7 +136,7 @@ func NewMempool(
 		bytesAvailable:       maxMempoolSize,
 		unissuedDecisionTxs:  unissuedDecisionTxs,
 		unissuedStakerTxs:    unissuedStakerTxs,
-		droppedTxIDs:         &cache.LRU[ids.ID, string]{Size: droppedTxIDsCacheSize},
+		droppedTxIDs:         &cache.LRU[ids.ID, error]{Size: droppedTxIDsCacheSize},
 		consumedUTXOs:        set.NewSet[ids.ID](initialConsumedUTXOsSize),
 		dropIncoming:         false, // enable tx adding by default
 		blkTimer:             blkTimer,
@@ -275,16 +275,13 @@ func (m *mempool) PeekStakerTx() *txs.Tx {
 	return m.unissuedStakerTxs.Peek()
 }
 
-func (m *mempool) MarkDropped(txID ids.ID, reason string) {
+func (m *mempool) MarkDropped(txID ids.ID, reason error) {
 	m.droppedTxIDs.Put(txID, reason)
 }
 
-func (m *mempool) GetDropReason(txID ids.ID) (string, bool) {
-	reason, exist := m.droppedTxIDs.Get(txID)
-	if !exist {
-		return "", false
-	}
-	return reason, true
+func (m *mempool) GetDropReason(txID ids.ID) error {
+	err, _ := m.droppedTxIDs.Get(txID)
+	return err
 }
 
 func (m *mempool) register(tx *txs.Tx) {

vms/platformvm/vm_test.go

@@ -659,8 +659,8 @@ func TestInvalidAddValidatorCommit(t *testing.T) {
 	require.Error(err)
 
 	txID := statelessBlk.Txs()[0].ID()
-	_, dropped := vm.Builder.GetDropReason(txID)
-	require.True(dropped)
+	reason := vm.Builder.GetDropReason(txID)
+	require.Error(reason)
 }
 
 // Reject attempt to add validator to primary network