diff --git a/pkg/core/mempool/mem_pool.go b/pkg/core/mempool/mem_pool.go index aecd774cd..54bd31de4 100644 --- a/pkg/core/mempool/mem_pool.go +++ b/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,47 +285,60 @@ 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 { - var num int - delete(mp.verifiedMap, hash) - for num = range mp.verifiedTxes { - if hash.Equals(mp.verifiedTxes[num].txn.Hash()) { - break - } - } - itm := mp.verifiedTxes[num] - if num < len(mp.verifiedTxes)-1 { - mp.verifiedTxes = append(mp.verifiedTxes[:num], mp.verifiedTxes[num+1:]...) - } else if num == len(mp.verifiedTxes)-1 { - mp.verifiedTxes = mp.verifiedTxes[:num] - } - payer := itm.txn.Signers[mp.payerIndex].Account - senderFee := mp.fees[payer] - senderFee.feeSum.SubUint64(&senderFee.feeSum, uint64(tx.SystemFee+tx.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 { - delete(mp.oracleResp, attrs[0].Value.(*transaction.OracleResponse).ID) - } - if mp.subscriptionsOn.Load() { - mp.events <- mempoolevent.Event{ - Type: mempoolevent.TransactionRemoved, - Tx: itm.txn, - Data: itm.data, - } +// 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 + for num = range mp.verifiedTxes { + if hash.Equals(mp.verifiedTxes[num].txn.Hash()) { + break } } - if mp.updateMetricsCb != nil { - mp.updateMetricsCb(len(mp.verifiedTxes)) + itm := mp.verifiedTxes[num] + if num < len(mp.verifiedTxes)-1 { + mp.verifiedTxes = append(mp.verifiedTxes[:num], mp.verifiedTxes[num+1:]...) + } 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(itm.txn.SystemFee+itm.txn.NetworkFee)) + mp.fees[payer] = senderFee + // remove all conflicting hashes from mp.conflicts list + 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() { + mp.events <- mempoolevent.Event{ + Type: mempoolevent.TransactionRemoved, + Tx: itm.txn, + Data: itm.data, + } } } diff --git a/pkg/core/mempool/mem_pool_test.go b/pkg/core/mempool/mem_pool_test.go index 430f4ea05..e6abf6821 100644 --- a/pkg/core/mempool/mem_pool_test.go +++ b/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()]) diff --git a/pkg/core/mempool/subscriptions_test.go b/pkg/core/mempool/subscriptions_test.go index d476f20bc..a935cfec9 100644 --- a/pkg/core/mempool/subscriptions_test.go +++ b/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