From f34eafab852dca7d937b75f907f079f2096f93fa Mon Sep 17 00:00:00 2001 From: Stephen Buttolph Date: Tue, 14 Mar 2023 15:28:42 -0400 Subject: [PATCH] Replace error strings with error objects in mempools (#2732) --- vms/avm/txs/mempool/mempool.go | 17 ++++++++------- vms/platformvm/blocks/builder/builder.go | 8 +++---- vms/platformvm/blocks/builder/builder_test.go | 17 ++++++++------- vms/platformvm/blocks/builder/network.go | 2 +- vms/platformvm/blocks/builder/network_test.go | 2 +- vms/platformvm/blocks/executor/verifier.go | 6 +++--- vms/platformvm/service.go | 6 +++--- vms/platformvm/txs/mempool/mempool.go | 21 ++++++++----------- vms/platformvm/txs/mempool/mock_mempool.go | 9 ++++---- vms/platformvm/vm_test.go | 4 ++-- 10 files changed, 46 insertions(+), 46 deletions(-) diff --git a/vms/avm/txs/mempool/mempool.go b/vms/avm/txs/mempool/mempool.go index 2f0eb91b8578..dfc0b87f4d7c 100644 --- a/vms/avm/txs/mempool/mempool.go +++ b/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 } diff --git a/vms/platformvm/blocks/builder/builder.go b/vms/platformvm/blocks/builder/builder.go index 2a2fdc8ca8f3..8fc2cf0a425f 100644 --- a/vms/platformvm/blocks/builder/builder.go +++ b/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), ) } } diff --git a/vms/platformvm/blocks/builder/builder_test.go b/vms/platformvm/blocks/builder/builder_test.go index 3eba24a97af5..49671f3fe87e 100644 --- a/vms/platformvm/blocks/builder/builder_test.go +++ b/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,14 +86,14 @@ 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 @@ -98,8 +101,8 @@ func TestPreviouslyDroppedTxsCanBeReAddedToMempool(t *testing.T) { 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) { diff --git a/vms/platformvm/blocks/builder/network.go b/vms/platformvm/blocks/builder/network.go index 37356bef8af1..428265c3e5c7 100644 --- a/vms/platformvm/blocks/builder/network.go +++ b/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 } diff --git a/vms/platformvm/blocks/builder/network_test.go b/vms/platformvm/blocks/builder/network_test.go index 38c5a611180e..5de7f6566565 100644 --- a/vms/platformvm/blocks/builder/network_test.go +++ b/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() diff --git a/vms/platformvm/blocks/executor/verifier.go b/vms/platformvm/blocks/executor/verifier.go index 9683353482a2..e5c160e0e0a8 100644 --- a/vms/platformvm/blocks/executor/verifier.go +++ b/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 diff --git a/vms/platformvm/service.go b/vms/platformvm/service.go index b2bd4f93f1bc..66aa2e8ebeb0 100644 --- a/vms/platformvm/service.go +++ b/vms/platformvm/service.go @@ -2149,8 +2149,8 @@ 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 @@ -2158,7 +2158,7 @@ func (s *Service) GetTxStatus(_ *http.Request, args *GetTxStatusArgs, response * // The tx was recently dropped because it was invalid. response.Status = status.Dropped - response.Reason = reason + response.Reason = reason.Error() return nil } diff --git a/vms/platformvm/txs/mempool/mempool.go b/vms/platformvm/txs/mempool/mempool.go index 8434e1c424c6..d101fef87931 100644 --- a/vms/platformvm/txs/mempool/mempool.go +++ b/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) { diff --git a/vms/platformvm/txs/mempool/mock_mempool.go b/vms/platformvm/txs/mempool/mock_mempool.go index 92cbc65cc7f3..f1e1cb9dcd91 100644 --- a/vms/platformvm/txs/mempool/mock_mempool.go +++ b/vms/platformvm/txs/mempool/mock_mempool.go @@ -91,12 +91,11 @@ func (mr *MockMempoolMockRecorder) Get(arg0 interface{}) *gomock.Call { } // GetDropReason mocks base method. -func (m *MockMempool) GetDropReason(arg0 ids.ID) (string, bool) { +func (m *MockMempool) GetDropReason(arg0 ids.ID) error { m.ctrl.T.Helper() ret := m.ctrl.Call(m, "GetDropReason", arg0) - ret0, _ := ret[0].(string) - ret1, _ := ret[1].(bool) - return ret0, ret1 + ret0, _ := ret[0].(error) + return ret0 } // GetDropReason indicates an expected call of GetDropReason. @@ -148,7 +147,7 @@ func (mr *MockMempoolMockRecorder) HasTxs() *gomock.Call { } // MarkDropped mocks base method. -func (m *MockMempool) MarkDropped(arg0 ids.ID, arg1 string) { +func (m *MockMempool) MarkDropped(arg0 ids.ID, arg1 error) { m.ctrl.T.Helper() m.ctrl.Call(m, "MarkDropped", arg0, arg1) } diff --git a/vms/platformvm/vm_test.go b/vms/platformvm/vm_test.go index 13b6515dc819..a084c5cb69f7 100644 --- a/vms/platformvm/vm_test.go +++ b/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