Merge pull request #3537 from nspcc-dev/fix-mempool-fees

Fix mempool fees

pkg/core/mempool/mem_pool.go

@@ -222,14 +222,14 @@ func (mp *Pool) Add(t *transaction.Transaction, fee Feer, data ...any) error {
 				mp.lock.Unlock()
 				return ErrOracleResponse
 			}
-			mp.removeInternal(h, fee)
+			mp.removeInternal(h)
 		}
 		mp.oracleResp[id] = t.Hash()
 	}
 
 	// Remove conflicting transactions.
 	for _, conflictingTx := range conflictsToBeRemoved {
-		mp.removeInternal(conflictingTx.Hash(), fee)
+		mp.removeInternal(conflictingTx.Hash())
 	}
 	// Insert into a sorted array (from max to min, that could also be done
 	// using sort.Sort(sort.Reverse()), but it incurs more overhead. Notice
@@ -250,19 +250,8 @@ func (mp *Pool) Add(t *transaction.Transaction, fee Feer, data ...any) error {
 		}
 		// Ditch the last one.
 		unlucky := mp.verifiedTxes[len(mp.verifiedTxes)-1]
-		delete(mp.verifiedMap, unlucky.txn.Hash())
-		mp.removeConflictsOf(unlucky.txn)
-		if attrs := unlucky.txn.GetAttributes(transaction.OracleResponseT); len(attrs) != 0 {
-			delete(mp.oracleResp, attrs[0].Value.(*transaction.OracleResponse).ID)
-		}
 		mp.verifiedTxes[len(mp.verifiedTxes)-1] = pItem
-		if mp.subscriptionsOn.Load() {
-			mp.events <- mempoolevent.Event{
-				Type: mempoolevent.TransactionRemoved,
-				Tx:   unlucky.txn,
-				Data: unlucky.data,
-			}
-		}
+		mp.removeFromMapWithFeesAndAttrs(unlucky)
 	} else {
 		mp.verifiedTxes = append(mp.verifiedTxes, pItem)
 	}
@@ -296,17 +285,24 @@ func (mp *Pool) Add(t *transaction.Transaction, fee Feer, data ...any) error {
 
 // Remove removes an item from the mempool if it exists there (and does
 // nothing if it doesn't).
-func (mp *Pool) Remove(hash util.Uint256, feer Feer) {
+func (mp *Pool) Remove(hash util.Uint256) {
 	mp.lock.Lock()
-	mp.removeInternal(hash, feer)
+	mp.removeInternal(hash)
+	if mp.updateMetricsCb != nil {
+		mp.updateMetricsCb(len(mp.verifiedTxes))
+	}
 	mp.lock.Unlock()
 }
 
-// removeInternal is an internal unlocked representation of Remove.
-func (mp *Pool) removeInternal(hash util.Uint256, feer Feer) {
-	if tx, ok := mp.verifiedMap[hash]; ok {
+// removeInternal is an internal unlocked representation of Remove, it drops
+// transaction from verifiedMap and verifiedTxs, adjusts fees and fires a
+// "removed" event.
+func (mp *Pool) removeInternal(hash util.Uint256) {
+	_, ok := mp.verifiedMap[hash]
+	if !ok {
+		return
+	}
 	var num int
-		delete(mp.verifiedMap, hash)
 	for num = range mp.verifiedTxes {
 		if hash.Equals(mp.verifiedTxes[num].txn.Hash()) {
 			break
@@ -318,13 +314,23 @@ func (mp *Pool) removeInternal(hash util.Uint256, feer Feer) {
 	} else if num == len(mp.verifiedTxes)-1 {
 		mp.verifiedTxes = mp.verifiedTxes[:num]
 	}
+	mp.removeFromMapWithFeesAndAttrs(itm)
+}
+
+// removeFromMapWithFeesAndAttrs removes given item (with the given hash) from
+// verifiedMap, adjusts fees, handles attributes and fires an event. Notice
+// that it does not do anything to verifiedTxes (the presumption is that if
+// you have itm already, you can handle it fine for the specific case).
+// It's an internal method, locking is to be handled by the caller.
+func (mp *Pool) removeFromMapWithFeesAndAttrs(itm item) {
+	delete(mp.verifiedMap, itm.txn.Hash())
 	payer := itm.txn.Signers[mp.payerIndex].Account
 	senderFee := mp.fees[payer]
-		senderFee.feeSum.SubUint64(&senderFee.feeSum, uint64(tx.SystemFee+tx.NetworkFee))
+	senderFee.feeSum.SubUint64(&senderFee.feeSum, uint64(itm.txn.SystemFee+itm.txn.NetworkFee))
 	mp.fees[payer] = senderFee
 	// remove all conflicting hashes from mp.conflicts list
-		mp.removeConflictsOf(tx)
-		if attrs := tx.GetAttributes(transaction.OracleResponseT); len(attrs) != 0 {
+	mp.removeConflictsOf(itm.txn)
+	if attrs := itm.txn.GetAttributes(transaction.OracleResponseT); len(attrs) != 0 {
 		delete(mp.oracleResp, attrs[0].Value.(*transaction.OracleResponse).ID)
 	}
 	if mp.subscriptionsOn.Load() {
@@ -335,10 +341,6 @@ func (mp *Pool) removeInternal(hash util.Uint256, feer Feer) {
 		}
 	}
 }
-	if mp.updateMetricsCb != nil {
-		mp.updateMetricsCb(len(mp.verifiedTxes))
-	}
-}
 
 // RemoveStale filters verified transactions through the given function keeping
 // only the transactions for which it returns true result. It's used to quickly

pkg/core/mempool/mem_pool_test.go

@@ -53,7 +53,7 @@ func testMemPoolAddRemoveWithFeer(t *testing.T, fs Feer) {
 	tx2, ok := mp.TryGetValue(tx.Hash())
 	require.Equal(t, true, ok)
 	require.Equal(t, tx, tx2)
-	mp.Remove(tx.Hash(), fs)
+	mp.Remove(tx.Hash())
 	_, ok = mp.TryGetValue(tx.Hash())
 	require.Equal(t, false, ok)
 	// Make sure nothing left in the mempool after removal.
@@ -112,18 +112,20 @@ func TestMemPoolAddRemove(t *testing.T) {
 
 func TestOverCapacity(t *testing.T) {
 	var fs = &FeerStub{balance: 10000000}
+	var acc = util.Uint160{1, 2, 3}
 	const mempoolSize = 10
 	mp := New(mempoolSize, 0, false, nil)
 
 	for i := 0; i < mempoolSize; i++ {
 		tx := transaction.New([]byte{byte(opcode.PUSH1)}, 0)
 		tx.Nonce = uint32(i)
-		tx.Signers = []transaction.Signer{{Account: util.Uint160{1, 2, 3}}}
+		tx.Signers = []transaction.Signer{{Account: acc}}
 		require.NoError(t, mp.Add(tx, fs))
 	}
 	txcnt := uint32(mempoolSize)
 	require.Equal(t, mempoolSize, mp.Count())
 	require.Equal(t, true, sort.IsSorted(sort.Reverse(mp.verifiedTxes)))
+	require.Equal(t, *uint256.NewInt(0), mp.fees[acc].feeSum)
 
 	bigScript := make([]byte, 64)
 	bigScript[0] = byte(opcode.PUSH1)
@@ -133,18 +135,20 @@ func TestOverCapacity(t *testing.T) {
 		tx := transaction.New(bigScript, 0)
 		tx.NetworkFee = 10000
 		tx.Nonce = txcnt
-		tx.Signers = []transaction.Signer{{Account: util.Uint160{1, 2, 3}}}
+		tx.Signers = []transaction.Signer{{Account: acc}}
 		txcnt++
 		// size is ~90, networkFee is 10000 => feePerByte is 119
 		require.NoError(t, mp.Add(tx, fs))
 		require.Equal(t, mempoolSize, mp.Count())
 		require.Equal(t, true, sort.IsSorted(sort.Reverse(mp.verifiedTxes)))
 	}
+	require.Equal(t, *uint256.NewInt(10 * 10000), mp.fees[acc].feeSum)
+
 	// Less prioritized txes are not allowed anymore.
 	tx := transaction.New(bigScript, 0)
 	tx.NetworkFee = 100
 	tx.Nonce = txcnt
-	tx.Signers = []transaction.Signer{{Account: util.Uint160{1, 2, 3}}}
+	tx.Signers = []transaction.Signer{{Account: acc}}
 	txcnt++
 	require.Error(t, mp.Add(tx, fs))
 	require.Equal(t, mempoolSize, mp.Count())
@@ -152,35 +156,38 @@ func TestOverCapacity(t *testing.T) {
 	require.Equal(t, mempoolSize, len(mp.verifiedTxes))
 	require.False(t, mp.containsKey(tx.Hash()))
 	require.Equal(t, true, sort.IsSorted(sort.Reverse(mp.verifiedTxes)))
+	require.Equal(t, *uint256.NewInt(100000), mp.fees[acc].feeSum)
 
 	// Low net fee, but higher per-byte fee is still a better combination.
 	tx = transaction.New([]byte{byte(opcode.PUSH1)}, 0)
 	tx.Nonce = txcnt
 	tx.NetworkFee = 7000
-	tx.Signers = []transaction.Signer{{Account: util.Uint160{1, 2, 3}}}
+	tx.Signers = []transaction.Signer{{Account: acc}}
 	txcnt++
 	// size is ~51 (small script), networkFee is 7000 (<10000)
 	// => feePerByte is 137 (>119)
 	require.NoError(t, mp.Add(tx, fs))
 	require.Equal(t, mempoolSize, mp.Count())
 	require.Equal(t, true, sort.IsSorted(sort.Reverse(mp.verifiedTxes)))
+	require.Equal(t, *uint256.NewInt(9*10000 + 7000), mp.fees[acc].feeSum)
 
 	// High priority always wins over low priority.
 	for i := 0; i < mempoolSize; i++ {
 		tx := transaction.New([]byte{byte(opcode.PUSH1)}, 0)
 		tx.NetworkFee = 8000
 		tx.Nonce = txcnt
-		tx.Signers = []transaction.Signer{{Account: util.Uint160{1, 2, 3}}}
+		tx.Signers = []transaction.Signer{{Account: acc}}
 		txcnt++
 		require.NoError(t, mp.Add(tx, fs))
 		require.Equal(t, mempoolSize, mp.Count())
 		require.Equal(t, true, sort.IsSorted(sort.Reverse(mp.verifiedTxes)))
 	}
+	require.Equal(t, *uint256.NewInt(10 * 8000), mp.fees[acc].feeSum)
 	// Good luck with low priority now.
 	tx = transaction.New([]byte{byte(opcode.PUSH1)}, 0)
 	tx.Nonce = txcnt
 	tx.NetworkFee = 7000
-	tx.Signers = []transaction.Signer{{Account: util.Uint160{1, 2, 3}}}
+	tx.Signers = []transaction.Signer{{Account: acc}}
 	require.Error(t, mp.Add(tx, fs))
 	require.Equal(t, mempoolSize, mp.Count())
 	require.Equal(t, true, sort.IsSorted(sort.Reverse(mp.verifiedTxes)))
@@ -204,7 +211,7 @@ func TestGetVerified(t *testing.T) {
 	require.Equal(t, mempoolSize, len(verTxes))
 	require.ElementsMatch(t, txes, verTxes)
 	for _, tx := range txes {
-		mp.Remove(tx.Hash(), fs)
+		mp.Remove(tx.Hash())
 	}
 	verTxes = mp.GetVerifiedTransactions()
 	require.Equal(t, 0, len(verTxes))
@@ -379,7 +386,7 @@ func TestMempoolAddRemoveOracleResponse(t *testing.T) {
 	require.ErrorIs(t, err, ErrOracleResponse)
 
 	// ok if old tx is removed
-	mp.Remove(tx1.Hash(), fs)
+	mp.Remove(tx1.Hash())
 	require.NoError(t, mp.Add(tx2, fs))
 
 	// higher network fee
@@ -526,12 +533,12 @@ func TestMempoolAddRemoveConflicts(t *testing.T) {
 	assert.Equal(t, []util.Uint256{tx3.Hash(), tx2.Hash()}, mp.conflicts[tx1.Hash()])
 
 	// manually remove tx11 with its single conflict
-	mp.Remove(tx11.Hash(), fs)
+	mp.Remove(tx11.Hash())
 	assert.Equal(t, 2, len(mp.conflicts))
 	assert.Equal(t, []util.Uint256{tx10.Hash()}, mp.conflicts[tx6.Hash()])
 
 	// manually remove last tx which conflicts with tx6 => mp.conflicts[tx6] should also be deleted
-	mp.Remove(tx10.Hash(), fs)
+	mp.Remove(tx10.Hash())
 	assert.Equal(t, 1, len(mp.conflicts))
 	assert.Equal(t, []util.Uint256{tx3.Hash(), tx2.Hash()}, mp.conflicts[tx1.Hash()])
 

pkg/core/mempool/subscriptions_test.go

@@ -67,7 +67,7 @@ func TestSubscriptions(t *testing.T) {
 		require.Equal(t, mempoolevent.Event{Type: mempoolevent.TransactionAdded, Tx: txs[2]}, event2)
 
 		// remove tx
-		mp.Remove(txs[1].Hash(), fs)
+		mp.Remove(txs[1].Hash())
 		require.Eventually(t, func() bool { return len(subChan1) == 1 && len(subChan2) == 1 }, time.Second, time.Millisecond*100)
 		event1 = <-subChan1
 		event2 = <-subChan2