From 6334192a957e2aec7a391ea412c5bf334072bd68 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Tue, 30 Jul 2024 17:36:57 +0300 Subject: [PATCH 1/3] mempool: remove Feer from Remove() It's not used and not needed for removal. Signed-off-by: Roman Khimov --- pkg/core/mempool/mem_pool.go | 10 +++++----- pkg/core/mempool/mem_pool_test.go | 10 +++++----- pkg/core/mempool/subscriptions_test.go | 2 +- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/pkg/core/mempool/mem_pool.go b/pkg/core/mempool/mem_pool.go index aecd774cd..5e36fd525 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 @@ -296,14 +296,14 @@ 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) mp.lock.Unlock() } // removeInternal is an internal unlocked representation of Remove. -func (mp *Pool) removeInternal(hash util.Uint256, feer Feer) { +func (mp *Pool) removeInternal(hash util.Uint256) { if tx, ok := mp.verifiedMap[hash]; ok { var num int delete(mp.verifiedMap, hash) diff --git a/pkg/core/mempool/mem_pool_test.go b/pkg/core/mempool/mem_pool_test.go index 430f4ea05..81c515ae9 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. @@ -204,7 +204,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 +379,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 +526,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 From a11e4337544833f98c78dd11791d8e7351782fd0 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Tue, 30 Jul 2024 17:42:10 +0300 Subject: [PATCH 2/3] mempool: move metrics out of removeInternal, simplify it Metrics should be updated once per action, currently removeInternal is used by Add and Remove, the first one updates them in the end anyway and remove should do the same. Signed-off-by: Roman Khimov --- pkg/core/mempool/mem_pool.go | 64 +++++++++++++++++++----------------- 1 file changed, 33 insertions(+), 31 deletions(-) diff --git a/pkg/core/mempool/mem_pool.go b/pkg/core/mempool/mem_pool.go index 5e36fd525..263baa665 100644 --- a/pkg/core/mempool/mem_pool.go +++ b/pkg/core/mempool/mem_pool.go @@ -299,44 +299,46 @@ func (mp *Pool) Add(t *transaction.Transaction, fee Feer, data ...any) error { func (mp *Pool) Remove(hash util.Uint256) { mp.lock.Lock() 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) { - 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, - } + tx, 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 } } - 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] + } + 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, + } } } From 5d1d7b104efa001c41a82d1e7c8c3b941bc1d040 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Tue, 30 Jul 2024 18:11:05 +0300 Subject: [PATCH 3/3] 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)))