From a48b1ba7e2b1f3728ea0da63997199da11b664b9 Mon Sep 17 00:00:00 2001 From: Eirik Ogilvie-Wigley Date: Fri, 27 Sep 2019 14:03:23 -0600 Subject: [PATCH] Rebuild weighted list on removal and fix size calculation --- qa/rpc-tests/mempool_limit.py | 19 ++++++++++--- src/gtest/test_mempoollimit.cpp | 47 +++++++++++++++------------------ src/main.cpp | 11 +++----- src/mempoollimit.cpp | 37 ++++++++++++++++++-------- src/mempoollimit.h | 10 ++++--- src/txmempool.cpp | 24 ++++++++--------- src/txmempool.h | 4 +-- 7 files changed, 88 insertions(+), 64 deletions(-) diff --git a/qa/rpc-tests/mempool_limit.py b/qa/rpc-tests/mempool_limit.py index b58cf9f36..52dd4419d 100755 --- a/qa/rpc-tests/mempool_limit.py +++ b/qa/rpc-tests/mempool_limit.py @@ -80,10 +80,23 @@ class MempoolLimit(BitcoinTestFramework): print("Mempool for node {}: {}".format(i, mempool)) assert_equal(2, len(mempool), "node {}".format(i)) - # self.nodes[0].generate(1) - # self.sync_all() + self.nodes[3].generate(1) + self.sync_all() - print("Success") + # The mempool sizes should be reset + print("Checking mempool size reset after block mined...") + taddr4 = self.nodes[0].getnewaddress() + self.nodes[0].sendtoaddress(taddr4, 9.999) + self.nodes[0].sendtoaddress(taddr4, 9.999) + self.sync_all() + + for i in range(0, 4): + mempool = self.nodes[i].getrawmempool() + print("Mempool for node {}: {}".format(i, mempool)) + assert_equal(2, len(mempool), "node {}".format(i)) + + self.nodes[3].generate(1) + self.sync_all() if __name__ == '__main__': diff --git a/src/gtest/test_mempoollimit.cpp b/src/gtest/test_mempoollimit.cpp index 84d0902a0..8c6f7040a 100644 --- a/src/gtest/test_mempoollimit.cpp +++ b/src/gtest/test_mempoollimit.cpp @@ -67,11 +67,11 @@ TEST(MempoolLimitTests, WeightedTransactionList_CheckSizeAfterDropping) list.add(WeightedTxInfo(TX_ID2, MIN_TX_COST, MIN_TX_COST)); EXPECT_EQ(8000, list.getTotalCost()); EXPECT_EQ(8000, list.getTotalLowFeePenaltyCost()); - EXPECT_FALSE(list.maybeDropRandom().is_initialized()); + EXPECT_FALSE(list.maybeDropRandom(true).is_initialized()); list.add(WeightedTxInfo(TX_ID3, MIN_TX_COST, MIN_TX_COST + LOW_FEE_PENALTY)); EXPECT_EQ(12000, list.getTotalCost()); EXPECT_EQ(12000 + LOW_FEE_PENALTY, list.getTotalLowFeePenaltyCost()); - boost::optional drop = list.maybeDropRandom(); + boost::optional drop = list.maybeDropRandom(true); ASSERT_TRUE(drop.is_initialized()); uint256 txid = drop.get().txId; std::cerr << "Trial " << trialNum++ << ": dropped " << txid.ToString() << std::endl; @@ -90,19 +90,15 @@ TEST(MempoolLimitTests, WeightedTXInfo_FromTx) auto consensusParams = RegtestActivateSapling(); auto sk = libzcash::SaplingSpendingKey::random(); - auto expsk = sk.expanded_spending_key(); - auto fvk = sk.full_viewing_key(); - auto pa = sk.default_address(); - - auto testNote = GetTestSaplingNote(pa, 50000); + auto testNote = GetTestSaplingNote(sk.default_address(), 50000); // Default fee { auto builder = TransactionBuilder(consensusParams, 1); - builder.AddSaplingSpend(expsk, testNote.note, testNote.tree.root(), testNote.tree.witness()); - builder.AddSaplingOutput(fvk.ovk, pa, 25000, {}); + builder.AddSaplingSpend(sk.expanded_spending_key(), testNote.note, testNote.tree.root(), testNote.tree.witness()); + builder.AddSaplingOutput(sk.full_viewing_key().ovk, sk.default_address(), 25000, {}); - WeightedTxInfo info = WeightedTxInfo::from(builder.Build().GetTxOrThrow()); + WeightedTxInfo info = WeightedTxInfo::from(builder.Build().GetTxOrThrow(), 10000); EXPECT_EQ(MIN_TX_COST, info.cost); EXPECT_EQ(MIN_TX_COST, info.lowFeePenaltyCost); } @@ -110,30 +106,31 @@ TEST(MempoolLimitTests, WeightedTXInfo_FromTx) // Lower than standard fee { auto builder = TransactionBuilder(consensusParams, 1); - builder.AddSaplingSpend(expsk, testNote.note, testNote.tree.root(), testNote.tree.witness()); - builder.AddSaplingOutput(fvk.ovk, pa, 25000, {}); - builder.SetFee(1); + builder.AddSaplingSpend(sk.expanded_spending_key(), testNote.note, testNote.tree.root(), testNote.tree.witness()); + builder.AddSaplingOutput(sk.full_viewing_key().ovk, sk.default_address(), 25000, {}); + builder.SetFee(9999); - WeightedTxInfo info = WeightedTxInfo::from(builder.Build().GetTxOrThrow()); + WeightedTxInfo info = WeightedTxInfo::from(builder.Build().GetTxOrThrow(), 9999); EXPECT_EQ(MIN_TX_COST, info.cost); EXPECT_EQ(MIN_TX_COST + LOW_FEE_PENALTY, info.lowFeePenaltyCost); } // Larger Tx { - auto testNote2 = GetTestSaplingNote(pa, 50000); - auto testNote3 = GetTestSaplingNote(pa, 50000); - auto testNote4 = GetTestSaplingNote(pa, 50000); auto builder = TransactionBuilder(consensusParams, 1); - builder.AddSaplingSpend(expsk, testNote.note, testNote.tree.root(), testNote.tree.witness()); - builder.AddSaplingSpend(expsk, testNote2.note, testNote2.tree.root(), testNote2.tree.witness()); - builder.AddSaplingSpend(expsk, testNote3.note, testNote3.tree.root(), testNote3.tree.witness()); - builder.AddSaplingSpend(expsk, testNote4.note, testNote4.tree.root(), testNote4.tree.witness()); - builder.AddSaplingOutput(fvk.ovk, pa, 25000, {}); + builder.AddSaplingSpend(sk.expanded_spending_key(), testNote.note, testNote.tree.root(), testNote.tree.witness()); + builder.AddSaplingOutput(sk.full_viewing_key().ovk, sk.default_address(), 5000, {}); + builder.AddSaplingOutput(sk.full_viewing_key().ovk, sk.default_address(), 5000, {}); + builder.AddSaplingOutput(sk.full_viewing_key().ovk, sk.default_address(), 5000, {}); + builder.AddSaplingOutput(sk.full_viewing_key().ovk, sk.default_address(), 5000, {}); - WeightedTxInfo info = WeightedTxInfo::from(builder.Build().GetTxOrThrow()); - EXPECT_EQ(MIN_TX_COST, info.cost); - EXPECT_EQ(MIN_TX_COST, info.lowFeePenaltyCost); + auto result = builder.Build(); + if (result.IsError()) { + std::cerr << result.GetError() << std::endl; + } + WeightedTxInfo info = WeightedTxInfo::from(result.GetTxOrThrow(), 10000); + EXPECT_EQ(5124, info.cost); + EXPECT_EQ(5124, info.lowFeePenaltyCost); } RegtestDeactivateSapling(); diff --git a/src/main.cpp b/src/main.cpp index b7bcd5980..4ec428bf4 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -1637,15 +1637,12 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa pool.addSpentIndex(entry, view); } - // In normal circumstances the following should be empty + // In normal circumstances the following should be boost::none + boost::optional maybeDropTxId; list removed; - std::vector evictedTxIds = pool.ensureSizeLimit(); - for (const uint256& txId : evictedTxIds) { - CTransaction toRemove = pool.mapTx.find(txId)->GetTx(); - pool.remove(toRemove, removed, true); + while ((maybeDropTxId = pool.ensureSizeLimit()).is_initialized()) { + pool.remove(pool.mapTx.find(maybeDropTxId.get())->GetTx(), removed, true); } - - // TODO rebuild list here and when a block is made } } diff --git a/src/mempoollimit.cpp b/src/mempoollimit.cpp index 0bc16720a..2f89f4e9e 100644 --- a/src/mempoollimit.cpp +++ b/src/mempoollimit.cpp @@ -5,9 +5,11 @@ #include "core_memusage.h" #include "mempoollimit.h" #include "random.h" +#include "serialize.h" #include "timedata.h" +#include "version.h" -const int64_t DEFAULT_FEE = 10000; +const CAmount DEFAULT_FEE = 10000; void RecentlyEvictedList::pruneList() { @@ -42,12 +44,16 @@ bool RecentlyEvictedList::contains(const uint256& txId) } -int64_t WeightedTransactionList::getTotalCost() +void WeightedTransactionList::clear() { + weightedTxInfos.clear(); +} + +int64_t WeightedTransactionList::getTotalCost() const { return weightedTxInfos.empty() ? 0 : weightedTxInfos.back().cost; } -int64_t WeightedTransactionList::getTotalLowFeePenaltyCost() +int64_t WeightedTransactionList::getTotalLowFeePenaltyCost() const { return weightedTxInfos.empty() ? 0 : weightedTxInfos.back().lowFeePenaltyCost; } @@ -65,7 +71,7 @@ void WeightedTransactionList::add(WeightedTxInfo weightedTxInfo) } } -boost::optional WeightedTransactionList::maybeDropRandom() +boost::optional WeightedTransactionList::maybeDropRandom(bool rebuildList) { int64_t totalCost = getTotalCost(); if (totalCost <= maxTotalCost) { @@ -81,22 +87,31 @@ boost::optional WeightedTransactionList::maybeDropRandom() if (i > 0) { drop.minusEquals(weightedTxInfos[i - 1]); } - while (++i < weightedTxInfos.size()) { - WeightedTxInfo nextTx = weightedTxInfos[i]; - nextTx.minusEquals(drop); - weightedTxInfos[i - 1] = nextTx; + if (rebuildList) { + while (++i < weightedTxInfos.size()) { + WeightedTxInfo nextTx = weightedTxInfos[i]; + nextTx.minusEquals(drop); + weightedTxInfos[i - 1] = nextTx; + } + weightedTxInfos.pop_back(); } - weightedTxInfos.pop_back(); LogPrint("mempool", "Evicting transaction (txid=%s, cost=%d, penaltyCost=%d)\n", drop.txId.ToString(), drop.cost, drop.lowFeePenaltyCost); return drop; } -WeightedTxInfo WeightedTxInfo::from(const CTransaction& tx) +// These are also defined in rpcwallet.cpp +#define JOINSPLIT_SIZE GetSerializeSize(JSDescription(), SER_NETWORK, PROTOCOL_VERSION) +#define OUTPUTDESCRIPTION_SIZE GetSerializeSize(OutputDescription(), SER_NETWORK, PROTOCOL_VERSION) +#define SPENDDESCRIPTION_SIZE GetSerializeSize(SpendDescription(), SER_NETWORK, PROTOCOL_VERSION) + +WeightedTxInfo WeightedTxInfo::from(const CTransaction& tx, const CAmount& fee) { size_t memUsage = RecursiveDynamicUsage(tx); + memUsage += tx.vJoinSplit.size() * JOINSPLIT_SIZE; + memUsage += tx.vShieldedOutput.size() * OUTPUTDESCRIPTION_SIZE; + memUsage += tx.vShieldedSpend.size() * SPENDDESCRIPTION_SIZE; int64_t cost = std::max(memUsage, MIN_TX_COST); int64_t lowFeePenaltyCost = cost; - int64_t fee = DEFAULT_FEE; if (fee < DEFAULT_FEE) { lowFeePenaltyCost += LOW_FEE_PENALTY; } diff --git a/src/mempoollimit.h b/src/mempoollimit.h index 96d528f7c..bc36b3aa9 100644 --- a/src/mempoollimit.h +++ b/src/mempoollimit.h @@ -51,11 +51,13 @@ class WeightedTransactionList public: WeightedTransactionList(int64_t maxTotalCost_) : maxTotalCost(maxTotalCost_) {} - int64_t getTotalCost(); - int64_t getTotalLowFeePenaltyCost(); + void clear(); + + int64_t getTotalCost() const; + int64_t getTotalLowFeePenaltyCost() const; void add(WeightedTxInfo weightedTxInfo); - boost::optional maybeDropRandom(); + boost::optional maybeDropRandom(bool rebuildList); }; @@ -67,7 +69,7 @@ struct WeightedTxInfo { WeightedTxInfo(uint256 txId_, uint64_t cost_, uint64_t lowFeePenaltyCost_) : txId(txId_), cost(cost_), lowFeePenaltyCost(lowFeePenaltyCost_) {} - static WeightedTxInfo from(const CTransaction& tx); + static WeightedTxInfo from(const CTransaction& tx, const CAmount& fee); void plusEquals(const WeightedTxInfo& other); void minusEquals(const WeightedTxInfo& other); diff --git a/src/txmempool.cpp b/src/txmempool.cpp index ae4ebb707..29fddc042 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -103,7 +103,7 @@ bool CTxMemPool::addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, // all the appropriate checks. LOCK(cs); if (weightedTxList) { - weightedTxList->add(WeightedTxInfo::from(entry.GetTx())); + weightedTxList->add(WeightedTxInfo::from(entry.GetTx(), entry.GetFee())); } mapTx.insert(entry); const CTransaction& tx = mapTx.find(hash)->GetTx(); @@ -291,6 +291,12 @@ void CTxMemPool::remove(const CTransaction &origTx, std::list& rem removeAddressIndex(hash); if (fSpentIndex) removeSpentIndex(hash); + } + if (weightedTxList) { + weightedTxList->clear(); + for (const CTxMemPoolEntry& e : mapTx) { + weightedTxList->add(WeightedTxInfo::from(e.GetTx(), e.GetFee())); + } } } } @@ -819,16 +825,10 @@ bool CTxMemPool::isRecentlyEvicted(const uint256& txId) { return recentlyEvicted->contains(txId); } -std::vector CTxMemPool::ensureSizeLimit() { - std::vector evicted; - if (!weightedTxList || !recentlyEvicted) { - return evicted; +boost::optional CTxMemPool::ensureSizeLimit() { + auto maybeDrop = weightedTxList->maybeDropRandom(false); + if (maybeDrop) { + return maybeDrop.get().txId; } - boost::optional txToDrop; - while ((txToDrop = weightedTxList->maybeDropRandom()).is_initialized()) { - uint256 txId = txToDrop->txId; - recentlyEvicted->add(txId); - evicted.push_back(txId); - } - return evicted; + return boost::none; } diff --git a/src/txmempool.h b/src/txmempool.h index 279b1d0a6..ac9eeacd5 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -267,8 +267,8 @@ public: void setMempoolCostLimit(int64_t totalCostLimit, int64_t evictionMemorySeconds); // Returns true if a transaction has been recently evicted bool isRecentlyEvicted(const uint256& txId); - // Returns a list of txids which have been evicted - std::vector ensureSizeLimit(); + // Returns a txid if a transaction is evicted from the mempool + boost::optional ensureSizeLimit(); }; /**