From 5d1d7b104efa001c41a82d1e7c8c3b941bc1d040 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Tue, 30 Jul 2024 18:11:05 +0300 Subject: [PATCH] mempool: properly remove fees when removing tx during Add Fixes #3488. Signed-off-by: Roman Khimov --- pkg/core/mempool/mem_pool.go | 36 +++++++++++++++---------------- pkg/core/mempool/mem_pool_test.go | 19 ++++++++++------ 2 files changed, 31 insertions(+), 24 deletions(-) diff --git a/pkg/core/mempool/mem_pool.go b/pkg/core/mempool/mem_pool.go index 263baa665..54bd31de4 100644 --- a/pkg/core/mempool/mem_pool.go +++ b/pkg/core/mempool/mem_pool.go @@ -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) } @@ -305,14 +294,15 @@ func (mp *Pool) Remove(hash util.Uint256) { mp.lock.Unlock() } -// removeInternal is an internal unlocked representation of Remove. +// 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) { - tx, ok := mp.verifiedMap[hash] + _, 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 @@ -324,13 +314,23 @@ func (mp *Pool) removeInternal(hash util.Uint256) { } 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() { diff --git a/pkg/core/mempool/mem_pool_test.go b/pkg/core/mempool/mem_pool_test.go index 81c515ae9..e6abf6821 100644 --- a/pkg/core/mempool/mem_pool_test.go +++ b/pkg/core/mempool/mem_pool_test.go @@ -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)))