From 7659438a63ef162b4a4f942f86683ae6785f8162 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Mon, 19 Oct 2015 12:42:42 -0400 Subject: [PATCH 1/6] CTxMemPool::removeForBlock now uses RemoveStaged --- src/txmempool.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index eee6cbf855..01f6c97aea 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -564,8 +564,12 @@ void CTxMemPool::removeForBlock(const std::vector& vtx, unsigned i } BOOST_FOREACH(const CTransaction& tx, vtx) { - std::list dummy; - remove(tx, dummy, false); + txiter it = mapTx.find(tx.GetHash()); + if (it != mapTx.end()) { + setEntries stage; + stage.insert(it); + RemoveStaged(stage); + } removeConflicts(tx, conflicts); ClearPrioritisation(tx.GetHash()); } From 5de2baa138cda501038a4558bc169b2cfe5b7d6b Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Mon, 19 Oct 2015 12:43:38 -0400 Subject: [PATCH 2/6] Rename CTxMemPool::remove -> removeRecursive remove is no longer called non-recursively, so simplify the logic and eliminate an unnecessary parameter --- src/main.cpp | 2 +- src/test/mempool_tests.cpp | 20 ++++++++++---------- src/txmempool.cpp | 18 +++++++----------- src/txmempool.h | 2 +- 4 files changed, 19 insertions(+), 23 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 027a36394c..d5254806ad 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -2502,7 +2502,7 @@ bool static DisconnectTip(CValidationState& state, const Consensus::Params& cons list removed; CValidationState stateDummy; if (tx.IsCoinBase() || !AcceptToMemoryPool(mempool, stateDummy, tx, false, NULL, true)) { - mempool.remove(tx, removed, true); + mempool.removeRecursive(tx, removed); } else if (mempool.exists(tx.GetHash())) { vHashUpdate.push_back(tx.GetHash()); } diff --git a/src/test/mempool_tests.cpp b/src/test/mempool_tests.cpp index fa352ace8f..ebdf7dbaa2 100644 --- a/src/test/mempool_tests.cpp +++ b/src/test/mempool_tests.cpp @@ -57,12 +57,12 @@ BOOST_AUTO_TEST_CASE(MempoolRemoveTest) std::list removed; // Nothing in pool, remove should do nothing: - testPool.remove(txParent, removed, true); + testPool.removeRecursive(txParent, removed); BOOST_CHECK_EQUAL(removed.size(), 0); // Just the parent: testPool.addUnchecked(txParent.GetHash(), entry.FromTx(txParent)); - testPool.remove(txParent, removed, true); + testPool.removeRecursive(txParent, removed); BOOST_CHECK_EQUAL(removed.size(), 1); removed.clear(); @@ -74,16 +74,16 @@ BOOST_AUTO_TEST_CASE(MempoolRemoveTest) testPool.addUnchecked(txGrandChild[i].GetHash(), entry.FromTx(txGrandChild[i])); } // Remove Child[0], GrandChild[0] should be removed: - testPool.remove(txChild[0], removed, true); + testPool.removeRecursive(txChild[0], removed); BOOST_CHECK_EQUAL(removed.size(), 2); removed.clear(); // ... make sure grandchild and child are gone: - testPool.remove(txGrandChild[0], removed, true); + testPool.removeRecursive(txGrandChild[0], removed); BOOST_CHECK_EQUAL(removed.size(), 0); - testPool.remove(txChild[0], removed, true); + testPool.removeRecursive(txChild[0], removed); BOOST_CHECK_EQUAL(removed.size(), 0); // Remove parent, all children/grandchildren should go: - testPool.remove(txParent, removed, true); + testPool.removeRecursive(txParent, removed); BOOST_CHECK_EQUAL(removed.size(), 5); BOOST_CHECK_EQUAL(testPool.size(), 0); removed.clear(); @@ -96,7 +96,7 @@ BOOST_AUTO_TEST_CASE(MempoolRemoveTest) } // Now remove the parent, as might happen if a block-re-org occurs but the parent cannot be // put into the mempool (maybe because it is non-standard): - testPool.remove(txParent, removed, true); + testPool.removeRecursive(txParent, removed); BOOST_CHECK_EQUAL(removed.size(), 6); BOOST_CHECK_EQUAL(testPool.size(), 0); removed.clear(); @@ -281,11 +281,11 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest) // Now try removing tx10 and verify the sort order returns to normal std::list removed; - pool.remove(pool.mapTx.find(tx10.GetHash())->GetTx(), removed, true); + pool.removeRecursive(pool.mapTx.find(tx10.GetHash())->GetTx(), removed); CheckSort(pool, snapshotOrder); - pool.remove(pool.mapTx.find(tx9.GetHash())->GetTx(), removed, true); - pool.remove(pool.mapTx.find(tx8.GetHash())->GetTx(), removed, true); + pool.removeRecursive(pool.mapTx.find(tx9.GetHash())->GetTx(), removed); + pool.removeRecursive(pool.mapTx.find(tx8.GetHash())->GetTx(), removed); /* Now check the sort on the mining score index. * Final order should be: * diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 01f6c97aea..2d1b78bea2 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -461,7 +461,7 @@ void CTxMemPool::CalculateDescendants(txiter entryit, setEntries &setDescendants } } -void CTxMemPool::remove(const CTransaction &origTx, std::list& removed, bool fRecursive) +void CTxMemPool::removeRecursive(const CTransaction &origTx, std::list& removed) { // Remove transaction from memory pool { @@ -470,8 +470,8 @@ void CTxMemPool::remove(const CTransaction &origTx, std::list& rem txiter origit = mapTx.find(origTx.GetHash()); if (origit != mapTx.end()) { txToRemove.insert(origit); - } else if (fRecursive) { - // If recursively removing but origTx isn't in the mempool + } else { + // When recursively removing but origTx isn't in the mempool // be sure to remove any children that are in the pool. This can // happen during chain re-orgs if origTx isn't re-accepted into // the mempool for any reason. @@ -485,12 +485,8 @@ void CTxMemPool::remove(const CTransaction &origTx, std::list& rem } } setEntries setAllRemoves; - if (fRecursive) { - BOOST_FOREACH(txiter it, txToRemove) { - CalculateDescendants(it, setAllRemoves); - } - } else { - setAllRemoves.swap(txToRemove); + BOOST_FOREACH(txiter it, txToRemove) { + CalculateDescendants(it, setAllRemoves); } BOOST_FOREACH(txiter it, setAllRemoves) { removed.push_back(it->GetTx()); @@ -524,7 +520,7 @@ void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMem } BOOST_FOREACH(const CTransaction& tx, transactionsToRemove) { list removed; - remove(tx, removed, true); + removeRecursive(tx, removed); } } @@ -539,7 +535,7 @@ void CTxMemPool::removeConflicts(const CTransaction &tx, std::list const CTransaction &txConflict = *it->second.ptx; if (txConflict != tx) { - remove(txConflict, removed, true); + removeRecursive(txConflict, removed); ClearPrioritisation(txConflict.GetHash()); } } diff --git a/src/txmempool.h b/src/txmempool.h index 7a2a1ef432..da5a97649f 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -428,7 +428,7 @@ public: bool addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, bool fCurrentEstimate = true); bool addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, setEntries &setAncestors, bool fCurrentEstimate = true); - void remove(const CTransaction &tx, std::list& removed, bool fRecursive = false); + void removeRecursive(const CTransaction &tx, std::list& removed); void removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight, int flags); void removeConflicts(const CTransaction &tx, std::list& removed); void removeForBlock(const std::vector& vtx, unsigned int nBlockHeight, From 76a76321d2f36992178ddaaf4d023c5e33c14fbf Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Wed, 21 Oct 2015 10:18:24 -0400 Subject: [PATCH 3/6] Remove work limit in UpdateForDescendants() The work limit served to prevent the descendant walking algorithm from doing too much work by marking the parent transaction as dirty. However to implement ancestor tracking, it's not possible to similarly mark those descendant transactions as dirty without having to calculate them to begin with. This commit removes the work limit altogether. With appropriate chain limits (-limitdescendantcount) the concern about doing too much work inside this function should be mitigated. --- src/main.cpp | 14 ----------- src/txmempool.cpp | 59 +++++++++-------------------------------------- src/txmempool.h | 19 +-------------- 3 files changed, 12 insertions(+), 80 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index d5254806ad..7a69d96668 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -1194,20 +1194,6 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C // Save these to avoid repeated lookups setIterConflicting.insert(mi); - // If this entry is "dirty", then we don't have descendant - // state for this transaction, which means we probably have - // lots of in-mempool descendants. - // Don't allow replacements of dirty transactions, to ensure - // that we don't spend too much time walking descendants. - // This should be rare. - if (mi->IsDirty()) { - return state.DoS(0, false, - REJECT_NONSTANDARD, "too many potential replacements", false, - strprintf("too many potential replacements: rejecting replacement %s; cannot replace tx %s with untracked descendants", - hash.ToString(), - mi->GetTx().GetHash().ToString())); - } - // Don't allow the replacement to reduce the feerate of the // mempool. // diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 2d1b78bea2..693d42667b 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -64,21 +64,13 @@ void CTxMemPoolEntry::UpdateFeeDelta(int64_t newFeeDelta) // Update the given tx for any in-mempool descendants. // Assumes that setMemPoolChildren is correct for the given tx and all // descendants. -bool CTxMemPool::UpdateForDescendants(txiter updateIt, int maxDescendantsToVisit, cacheMap &cachedDescendants, const std::set &setExclude) +void CTxMemPool::UpdateForDescendants(txiter updateIt, cacheMap &cachedDescendants, const std::set &setExclude) { - // Track the number of entries (outside setExclude) that we'd need to visit - // (will bail out if it exceeds maxDescendantsToVisit) - int nChildrenToVisit = 0; - setEntries stageEntries, setAllDescendants; stageEntries = GetMemPoolChildren(updateIt); while (!stageEntries.empty()) { const txiter cit = *stageEntries.begin(); - if (cit->IsDirty()) { - // Don't consider any more children if any descendant is dirty - return false; - } setAllDescendants.insert(cit); stageEntries.erase(cit); const setEntries &setChildren = GetMemPoolChildren(cit); @@ -88,22 +80,11 @@ bool CTxMemPool::UpdateForDescendants(txiter updateIt, int maxDescendantsToVisit // We've already calculated this one, just add the entries for this set // but don't traverse again. BOOST_FOREACH(const txiter cacheEntry, cacheIt->second) { - // update visit count only for new child transactions - // (outside of setExclude and stageEntries) - if (setAllDescendants.insert(cacheEntry).second && - !setExclude.count(cacheEntry->GetTx().GetHash()) && - !stageEntries.count(cacheEntry)) { - nChildrenToVisit++; - } + setAllDescendants.insert(cacheEntry); } } else if (!setAllDescendants.count(childEntry)) { - // Schedule for later processing and update our visit count - if (stageEntries.insert(childEntry).second && !setExclude.count(childEntry->GetTx().GetHash())) { - nChildrenToVisit++; - } - } - if (nChildrenToVisit > maxDescendantsToVisit) { - return false; + // Schedule for later processing + stageEntries.insert(childEntry); } } } @@ -121,7 +102,6 @@ bool CTxMemPool::UpdateForDescendants(txiter updateIt, int maxDescendantsToVisit } } mapTx.modify(updateIt, update_descendant_state(modifySize, modifyFee, modifyCount)); - return true; } // vHashesToUpdate is the set of transaction hashes from a disconnected block @@ -167,10 +147,7 @@ void CTxMemPool::UpdateTransactionsFromBlock(const std::vector &vHashes UpdateParent(childIter, it, true); } } - if (!UpdateForDescendants(it, 100, mapMemPoolDescendantsToUpdate, setAlreadyIncluded)) { - // Mark as dirty if we can't do the calculation. - mapTx.modify(it, set_dirty()); - } + UpdateForDescendants(it, mapMemPoolDescendantsToUpdate, setAlreadyIncluded); } } @@ -301,22 +278,13 @@ void CTxMemPool::UpdateForRemoveFromMempool(const setEntries &entriesToRemove) } } -void CTxMemPoolEntry::SetDirty() -{ - nCountWithDescendants = 0; - nSizeWithDescendants = nTxSize; - nModFeesWithDescendants = GetModifiedFee(); -} - void CTxMemPoolEntry::UpdateState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount) { - if (!IsDirty()) { - nSizeWithDescendants += modifySize; - assert(int64_t(nSizeWithDescendants) > 0); - nModFeesWithDescendants += modifyFee; - nCountWithDescendants += modifyCount; - assert(int64_t(nCountWithDescendants) > 0); - } + nSizeWithDescendants += modifySize; + assert(int64_t(nSizeWithDescendants) > 0); + nModFeesWithDescendants += modifyFee; + nCountWithDescendants += modifyCount; + assert(int64_t(nCountWithDescendants) > 0); } CTxMemPool::CTxMemPool(const CFeeRate& _minReasonableRelayFee) : @@ -658,12 +626,7 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const assert(setChildrenCheck == GetMemPoolChildren(it)); // Also check to make sure size is greater than sum with immediate children. // just a sanity check, not definitive that this calc is correct... - if (!it->IsDirty()) { - assert(it->GetSizeWithDescendants() >= childSizes + it->GetTxSize()); - } else { - assert(it->GetSizeWithDescendants() == it->GetTxSize()); - assert(it->GetModFeesWithDescendants() == it->GetModifiedFee()); - } + assert(it->GetSizeWithDescendants() >= childSizes + it->GetTxSize()); if (fDependsWait) waitingOnDependants.push_back(&(*it)); diff --git a/src/txmempool.h b/src/txmempool.h index da5a97649f..b1b6645a06 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -108,13 +108,6 @@ public: // modified fees with descendants. void UpdateFeeDelta(int64_t feeDelta); - /** We can set the entry to be dirty if doing the full calculation of in- - * mempool descendants will be too expensive, which can potentially happen - * when re-adding transactions from a block back to the mempool. - */ - void SetDirty(); - bool IsDirty() const { return nCountWithDescendants == 0; } - uint64_t GetCountWithDescendants() const { return nCountWithDescendants; } uint64_t GetSizeWithDescendants() const { return nSizeWithDescendants; } CAmount GetModFeesWithDescendants() const { return nModFeesWithDescendants; } @@ -138,12 +131,6 @@ struct update_descendant_state int64_t modifyCount; }; -struct set_dirty -{ - void operator() (CTxMemPoolEntry &e) - { e.SetDirty(); } -}; - struct update_fee_delta { update_fee_delta(int64_t _feeDelta) : feeDelta(_feeDelta) { } @@ -555,15 +542,11 @@ private: * updated and hence their state is already reflected in the parent * state). * - * If updating an entry requires looking at more than maxDescendantsToVisit - * transactions, outside of the ones in setExclude, then give up. - * * cachedDescendants will be updated with the descendants of the transaction * being updated, so that future invocations don't need to walk the * same transaction again, if encountered in another transaction chain. */ - bool UpdateForDescendants(txiter updateIt, - int maxDescendantsToVisit, + void UpdateForDescendants(txiter updateIt, cacheMap &cachedDescendants, const std::set &setExclude); /** Update ancestors of hash to add/remove it as a descendant transaction. */ From 72abd2ce3c5ad8157d3a993693df1919a6ad79c3 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Mon, 19 Oct 2015 10:54:28 -0400 Subject: [PATCH 4/6] Add ancestor tracking to mempool This implements caching of ancestor state to each mempool entry, similar to descendant tracking, but also including caching sigops-with-ancestors (as that metric will be helpful to future code that implements better transaction selection in CreatenewBlock). --- src/main.cpp | 2 +- src/txmempool.cpp | 88 +++++++++++++++++++++++++++++++++++++++-------- src/txmempool.h | 49 ++++++++++++++++++++++---- 3 files changed, 117 insertions(+), 22 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 7a69d96668..2ae560e01b 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -1323,7 +1323,7 @@ bool AcceptToMemoryPoolWorker(CTxMemPool& pool, CValidationState& state, const C FormatMoney(nModifiedFees - nConflictingFees), (int)nSize - (int)nConflictingSize); } - pool.RemoveStaged(allConflicting); + pool.RemoveStaged(allConflicting, false); // Store transaction in memory pool.addUnchecked(hash, entry, setAncestors, !IsInitialBlockDownload()); diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 693d42667b..f7961a6b80 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -38,6 +38,11 @@ CTxMemPoolEntry::CTxMemPoolEntry(const CTransaction& _tx, const CAmount& _nFee, assert(inChainInputValue <= nValueIn); feeDelta = 0; + + nCountWithAncestors = 1; + nSizeWithAncestors = nTxSize; + nModFeesWithAncestors = nFee; + nSigOpCountWithAncestors = sigOpCount; } CTxMemPoolEntry::CTxMemPoolEntry(const CTxMemPoolEntry& other) @@ -58,6 +63,7 @@ CTxMemPoolEntry::GetPriority(unsigned int currentHeight) const void CTxMemPoolEntry::UpdateFeeDelta(int64_t newFeeDelta) { nModFeesWithDescendants += newFeeDelta - feeDelta; + nModFeesWithAncestors += newFeeDelta - feeDelta; feeDelta = newFeeDelta; } @@ -99,6 +105,8 @@ void CTxMemPool::UpdateForDescendants(txiter updateIt, cacheMap &cachedDescendan modifyFee += cit->GetModifiedFee(); modifyCount++; cachedDescendants[updateIt].insert(cit); + // Update ancestor state for each descendant + mapTx.modify(cit, update_ancestor_state(updateIt->GetTxSize(), updateIt->GetModifiedFee(), 1, updateIt->GetSigOpCount())); } } mapTx.modify(updateIt, update_descendant_state(modifySize, modifyFee, modifyCount)); @@ -108,6 +116,7 @@ void CTxMemPool::UpdateForDescendants(txiter updateIt, cacheMap &cachedDescendan // which has been re-added to the mempool. // for each entry, look for descendants that are outside hashesToUpdate, and // add fee/size information for such descendants to the parent. +// for each such descendant, also update the ancestor state to include the parent. void CTxMemPool::UpdateTransactionsFromBlock(const std::vector &vHashesToUpdate) { LOCK(cs); @@ -228,6 +237,20 @@ void CTxMemPool::UpdateAncestorsOf(bool add, txiter it, setEntries &setAncestors } } +void CTxMemPool::UpdateEntryForAncestors(txiter it, const setEntries &setAncestors) +{ + int64_t updateCount = setAncestors.size(); + int64_t updateSize = 0; + CAmount updateFee = 0; + int updateSigOps = 0; + BOOST_FOREACH(txiter ancestorIt, setAncestors) { + updateSize += ancestorIt->GetTxSize(); + updateFee += ancestorIt->GetModifiedFee(); + updateSigOps += ancestorIt->GetSigOpCount(); + } + mapTx.modify(it, update_ancestor_state(updateSize, updateFee, updateCount, updateSigOps)); +} + void CTxMemPool::UpdateChildrenForRemoval(txiter it) { const setEntries &setMemPoolChildren = GetMemPoolChildren(it); @@ -236,11 +259,30 @@ void CTxMemPool::UpdateChildrenForRemoval(txiter it) } } -void CTxMemPool::UpdateForRemoveFromMempool(const setEntries &entriesToRemove) +void CTxMemPool::UpdateForRemoveFromMempool(const setEntries &entriesToRemove, bool updateDescendants) { // For each entry, walk back all ancestors and decrement size associated with this // transaction const uint64_t nNoLimit = std::numeric_limits::max(); + if (updateDescendants) { + // updateDescendants should be true whenever we're not recursively + // removing a tx and all its descendants, eg when a transaction is + // confirmed in a block. + // Here we only update statistics and not data in mapLinks (which + // we need to preserve until we're finished with all operations that + // need to traverse the mempool). + BOOST_FOREACH(txiter removeIt, entriesToRemove) { + setEntries setDescendants; + CalculateDescendants(removeIt, setDescendants); + setDescendants.erase(removeIt); // don't update state for self + int64_t modifySize = -((int64_t)removeIt->GetTxSize()); + CAmount modifyFee = -removeIt->GetModifiedFee(); + int modifySigOps = -removeIt->GetSigOpCount(); + BOOST_FOREACH(txiter dit, setDescendants) { + mapTx.modify(dit, update_ancestor_state(modifySize, modifyFee, -1, modifySigOps)); + } + } + } BOOST_FOREACH(txiter removeIt, entriesToRemove) { setEntries setAncestors; const CTxMemPoolEntry &entry = *removeIt; @@ -264,10 +306,7 @@ void CTxMemPool::UpdateForRemoveFromMempool(const setEntries &entriesToRemove) // transactions as the set of things to update for removal. CalculateMemPoolAncestors(entry, setAncestors, nNoLimit, nNoLimit, nNoLimit, nNoLimit, dummy, false); // Note that UpdateAncestorsOf severs the child links that point to - // removeIt in the entries for the parents of removeIt. This is - // fine since we don't need to use the mempool children of any entries - // to walk back over our ancestors (but we do need the mempool - // parents!) + // removeIt in the entries for the parents of removeIt. UpdateAncestorsOf(false, removeIt, setAncestors); } // After updating all the ancestor sizes, we can now sever the link between each @@ -278,7 +317,7 @@ void CTxMemPool::UpdateForRemoveFromMempool(const setEntries &entriesToRemove) } } -void CTxMemPoolEntry::UpdateState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount) +void CTxMemPoolEntry::UpdateDescendantState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount) { nSizeWithDescendants += modifySize; assert(int64_t(nSizeWithDescendants) > 0); @@ -287,6 +326,17 @@ void CTxMemPoolEntry::UpdateState(int64_t modifySize, CAmount modifyFee, int64_t assert(int64_t(nCountWithDescendants) > 0); } +void CTxMemPoolEntry::UpdateAncestorState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount, int modifySigOps) +{ + nSizeWithAncestors += modifySize; + assert(int64_t(nSizeWithAncestors) > 0); + nModFeesWithAncestors += modifyFee; + nCountWithAncestors += modifyCount; + assert(int64_t(nCountWithAncestors) > 0); + nSigOpCountWithAncestors += modifySigOps; + assert(int(nSigOpCountWithAncestors) >= 0); +} + CTxMemPool::CTxMemPool(const CFeeRate& _minReasonableRelayFee) : nTransactionsUpdated(0) { @@ -377,6 +427,7 @@ bool CTxMemPool::addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, } } UpdateAncestorsOf(true, newit, setAncestors); + UpdateEntryForAncestors(newit, setAncestors); nTransactionsUpdated++; totalTxSize += entry.GetTxSize(); @@ -459,7 +510,7 @@ void CTxMemPool::removeRecursive(const CTransaction &origTx, std::listGetTx()); } - RemoveStaged(setAllRemoves); + RemoveStaged(setAllRemoves, false); } } @@ -532,7 +583,7 @@ void CTxMemPool::removeForBlock(const std::vector& vtx, unsigned i if (it != mapTx.end()) { setEntries stage; stage.insert(it); - RemoveStaged(stage); + RemoveStaged(stage, true); } removeConflicts(tx, conflicts); ClearPrioritisation(tx.GetHash()); @@ -590,6 +641,8 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const innerUsage += memusage::DynamicUsage(links.parents) + memusage::DynamicUsage(links.children); bool fDependsWait = false; setEntries setParentCheck; + int64_t parentSizes = 0; + unsigned int parentSigOpCount = 0; BOOST_FOREACH(const CTxIn &txin, tx.vin) { // Check that every mempool transaction's inputs refer to available coins, or other mempool tx's. indexed_transaction_set::const_iterator it2 = mapTx.find(txin.prevout.hash); @@ -597,7 +650,10 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const const CTransaction& tx2 = it2->GetTx(); assert(tx2.vout.size() > txin.prevout.n && !tx2.vout[txin.prevout.n].IsNull()); fDependsWait = true; - setParentCheck.insert(it2); + if (setParentCheck.insert(it2).second) { + parentSizes += it2->GetTxSize(); + parentSigOpCount += it2->GetSigOpCount(); + } } else { const CCoins* coins = pcoins->AccessCoins(txin.prevout.hash); assert(coins && coins->IsAvailable(txin.prevout.n)); @@ -610,17 +666,19 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const i++; } assert(setParentCheck == GetMemPoolParents(it)); + // Also check to make sure ancestor size/sigops are >= sum with immediate + // parents. + assert(it->GetSizeWithAncestors() >= parentSizes + it->GetTxSize()); + assert(it->GetSigOpCountWithAncestors() >= parentSigOpCount + it->GetSigOpCount()); // Check children against mapNextTx CTxMemPool::setEntries setChildrenCheck; std::map::const_iterator iter = mapNextTx.lower_bound(COutPoint(it->GetTx().GetHash(), 0)); int64_t childSizes = 0; - CAmount childModFee = 0; for (; iter != mapNextTx.end() && iter->first.hash == it->GetTx().GetHash(); ++iter) { txiter childit = mapTx.find(iter->second.ptx->GetHash()); assert(childit != mapTx.end()); // mapNextTx points to in-mempool transactions if (setChildrenCheck.insert(childit).second) { childSizes += childit->GetTxSize(); - childModFee += childit->GetModifiedFee(); } } assert(setChildrenCheck == GetMemPoolChildren(it)); @@ -812,9 +870,9 @@ size_t CTxMemPool::DynamicMemoryUsage() const { return memusage::MallocUsage(sizeof(CTxMemPoolEntry) + 12 * sizeof(void*)) * mapTx.size() + memusage::DynamicUsage(mapNextTx) + memusage::DynamicUsage(mapDeltas) + memusage::DynamicUsage(mapLinks) + cachedInnerUsage; } -void CTxMemPool::RemoveStaged(setEntries &stage) { +void CTxMemPool::RemoveStaged(setEntries &stage, bool updateDescendants) { AssertLockHeld(cs); - UpdateForRemoveFromMempool(stage); + UpdateForRemoveFromMempool(stage, updateDescendants); BOOST_FOREACH(const txiter& it, stage) { removeUnchecked(it); } @@ -832,7 +890,7 @@ int CTxMemPool::Expire(int64_t time) { BOOST_FOREACH(txiter removeit, toremove) { CalculateDescendants(removeit, stage); } - RemoveStaged(stage); + RemoveStaged(stage, false); return stage.size(); } @@ -941,7 +999,7 @@ void CTxMemPool::TrimToSize(size_t sizelimit, std::vector* pvNoSpendsRe BOOST_FOREACH(txiter it, stage) txn.push_back(it->GetTx()); } - RemoveStaged(stage); + RemoveStaged(stage, false); if (pvNoSpendsRemaining) { BOOST_FOREACH(const CTransaction& tx, txn) { BOOST_FOREACH(const CTxIn& txin, tx.vin) { diff --git a/src/txmempool.h b/src/txmempool.h index b1b6645a06..0db3d5bd27 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -80,6 +80,12 @@ private: uint64_t nSizeWithDescendants; //! ... and size CAmount nModFeesWithDescendants; //! ... and total fees (all including us) + // Analogous statistics for ancestor transactions + uint64_t nCountWithAncestors; + uint64_t nSizeWithAncestors; + CAmount nModFeesWithAncestors; + unsigned int nSigOpCountWithAncestors; + public: CTxMemPoolEntry(const CTransaction& _tx, const CAmount& _nFee, int64_t _nTime, double _entryPriority, unsigned int _entryHeight, @@ -103,7 +109,9 @@ public: size_t DynamicMemoryUsage() const { return nUsageSize; } // Adjusts the descendant state, if this entry is not dirty. - void UpdateState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount); + void UpdateDescendantState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount); + // Adjusts the ancestor state + void UpdateAncestorState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount, int modifySigOps); // Updates the fee delta used for mining priority score, and the // modified fees with descendants. void UpdateFeeDelta(int64_t feeDelta); @@ -113,6 +121,11 @@ public: CAmount GetModFeesWithDescendants() const { return nModFeesWithDescendants; } bool GetSpendsCoinbase() const { return spendsCoinbase; } + + uint64_t GetCountWithAncestors() const { return nCountWithAncestors; } + uint64_t GetSizeWithAncestors() const { return nSizeWithAncestors; } + CAmount GetModFeesWithAncestors() const { return nModFeesWithAncestors; } + unsigned int GetSigOpCountWithAncestors() const { return nSigOpCountWithAncestors; } }; // Helpers for modifying CTxMemPool::mapTx, which is a boost multi_index. @@ -123,7 +136,7 @@ struct update_descendant_state {} void operator() (CTxMemPoolEntry &e) - { e.UpdateState(modifySize, modifyFee, modifyCount); } + { e.UpdateDescendantState(modifySize, modifyFee, modifyCount); } private: int64_t modifySize; @@ -131,6 +144,22 @@ struct update_descendant_state int64_t modifyCount; }; +struct update_ancestor_state +{ + update_ancestor_state(int64_t _modifySize, CAmount _modifyFee, int64_t _modifyCount, int _modifySigOps) : + modifySize(_modifySize), modifyFee(_modifyFee), modifyCount(_modifyCount), modifySigOps(_modifySigOps) + {} + + void operator() (CTxMemPoolEntry &e) + { e.UpdateAncestorState(modifySize, modifyFee, modifyCount, modifySigOps); } + + private: + int64_t modifySize; + CAmount modifyFee; + int64_t modifyCount; + int modifySigOps; +}; + struct update_fee_delta { update_fee_delta(int64_t _feeDelta) : feeDelta(_feeDelta) { } @@ -440,8 +469,12 @@ public: public: /** Remove a set of transactions from the mempool. * If a transaction is in this set, then all in-mempool descendants must - * also be in the set.*/ - void RemoveStaged(setEntries &stage); + * also be in the set, unless this transaction is being removed for being + * in a block. + * Set updateDescendants to true when removing a tx that was in a block, so + * that any in-mempool descendants have their ancestor state updated. + */ + void RemoveStaged(setEntries &stage, bool updateDescendants); /** When adding transactions from a disconnected block back to the mempool, * new mempool entries may have children in the mempool (which is generally @@ -551,8 +584,12 @@ private: const std::set &setExclude); /** Update ancestors of hash to add/remove it as a descendant transaction. */ void UpdateAncestorsOf(bool add, txiter hash, setEntries &setAncestors); - /** For each transaction being removed, update ancestors and any direct children. */ - void UpdateForRemoveFromMempool(const setEntries &entriesToRemove); + /** Set ancestor state for an entry */ + void UpdateEntryForAncestors(txiter it, const setEntries &setAncestors); + /** For each transaction being removed, update ancestors and any direct children. + * If updateDescendants is true, then also update in-mempool descendants' + * ancestor state. */ + void UpdateForRemoveFromMempool(const setEntries &entriesToRemove, bool updateDescendants); /** Sever link between specified transaction and direct children. */ void UpdateChildrenForRemoval(txiter entry); From e2eeb5dda790cf301aa669704a25fb35f67400e7 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Mon, 19 Oct 2015 15:15:12 -0400 Subject: [PATCH 5/6] Add ancestor feerate index to mempool --- src/test/mempool_tests.cpp | 104 +++++++++++++++++++++++++++++++++++++ src/txmempool.cpp | 4 +- src/txmempool.h | 32 +++++++++++- 3 files changed, 137 insertions(+), 3 deletions(-) diff --git a/src/test/mempool_tests.cpp b/src/test/mempool_tests.cpp index ebdf7dbaa2..c8b43df26c 100644 --- a/src/test/mempool_tests.cpp +++ b/src/test/mempool_tests.cpp @@ -317,6 +317,110 @@ BOOST_AUTO_TEST_CASE(MempoolIndexingTest) CheckSort(pool, sortedOrder); } +BOOST_AUTO_TEST_CASE(MempoolAncestorIndexingTest) +{ + CTxMemPool pool(CFeeRate(0)); + TestMemPoolEntryHelper entry; + entry.hadNoDependencies = true; + + /* 3rd highest fee */ + CMutableTransaction tx1 = CMutableTransaction(); + tx1.vout.resize(1); + tx1.vout[0].scriptPubKey = CScript() << OP_11 << OP_EQUAL; + tx1.vout[0].nValue = 10 * COIN; + pool.addUnchecked(tx1.GetHash(), entry.Fee(10000LL).Priority(10.0).FromTx(tx1)); + + /* highest fee */ + CMutableTransaction tx2 = CMutableTransaction(); + tx2.vout.resize(1); + tx2.vout[0].scriptPubKey = CScript() << OP_11 << OP_EQUAL; + tx2.vout[0].nValue = 2 * COIN; + pool.addUnchecked(tx2.GetHash(), entry.Fee(20000LL).Priority(9.0).FromTx(tx2)); + uint64_t tx2Size = ::GetSerializeSize(tx2, SER_NETWORK, PROTOCOL_VERSION); + + /* lowest fee */ + CMutableTransaction tx3 = CMutableTransaction(); + tx3.vout.resize(1); + tx3.vout[0].scriptPubKey = CScript() << OP_11 << OP_EQUAL; + tx3.vout[0].nValue = 5 * COIN; + pool.addUnchecked(tx3.GetHash(), entry.Fee(0LL).Priority(100.0).FromTx(tx3)); + + /* 2nd highest fee */ + CMutableTransaction tx4 = CMutableTransaction(); + tx4.vout.resize(1); + tx4.vout[0].scriptPubKey = CScript() << OP_11 << OP_EQUAL; + tx4.vout[0].nValue = 6 * COIN; + pool.addUnchecked(tx4.GetHash(), entry.Fee(15000LL).Priority(1.0).FromTx(tx4)); + + /* equal fee rate to tx1, but newer */ + CMutableTransaction tx5 = CMutableTransaction(); + tx5.vout.resize(1); + tx5.vout[0].scriptPubKey = CScript() << OP_11 << OP_EQUAL; + tx5.vout[0].nValue = 11 * COIN; + pool.addUnchecked(tx5.GetHash(), entry.Fee(10000LL).FromTx(tx5)); + BOOST_CHECK_EQUAL(pool.size(), 5); + + std::vector sortedOrder; + sortedOrder.resize(5); + sortedOrder[0] = tx2.GetHash().ToString(); // 20000 + sortedOrder[1] = tx4.GetHash().ToString(); // 15000 + // tx1 and tx5 are both 10000 + // Ties are broken by hash, not timestamp, so determine which + // hash comes first. + if (tx1.GetHash() < tx5.GetHash()) { + sortedOrder[2] = tx1.GetHash().ToString(); + sortedOrder[3] = tx5.GetHash().ToString(); + } else { + sortedOrder[2] = tx5.GetHash().ToString(); + sortedOrder[3] = tx1.GetHash().ToString(); + } + sortedOrder[4] = tx3.GetHash().ToString(); // 0 + + CheckSort(pool, sortedOrder); + + /* low fee parent with high fee child */ + /* tx6 (0) -> tx7 (high) */ + CMutableTransaction tx6 = CMutableTransaction(); + tx6.vout.resize(1); + tx6.vout[0].scriptPubKey = CScript() << OP_11 << OP_EQUAL; + tx6.vout[0].nValue = 20 * COIN; + uint64_t tx6Size = ::GetSerializeSize(tx6, SER_NETWORK, PROTOCOL_VERSION); + + pool.addUnchecked(tx6.GetHash(), entry.Fee(0LL).FromTx(tx6)); + BOOST_CHECK_EQUAL(pool.size(), 6); + sortedOrder.push_back(tx6.GetHash().ToString()); + CheckSort(pool, sortedOrder); + + CMutableTransaction tx7 = CMutableTransaction(); + tx7.vin.resize(1); + tx7.vin[0].prevout = COutPoint(tx6.GetHash(), 0); + tx7.vin[0].scriptSig = CScript() << OP_11; + tx7.vout.resize(1); + tx7.vout[0].scriptPubKey = CScript() << OP_11 << OP_EQUAL; + tx7.vout[0].nValue = 10 * COIN; + uint64_t tx7Size = ::GetSerializeSize(tx7, SER_NETWORK, PROTOCOL_VERSION); + + /* set the fee to just below tx2's feerate when including ancestor */ + CAmount fee = (20000/tx2Size)*(tx7Size + tx6Size) - 1; + + //CTxMemPoolEntry entry7(tx7, fee, 2, 10.0, 1, true); + pool.addUnchecked(tx7.GetHash(), entry.Fee(fee).FromTx(tx7)); + BOOST_CHECK_EQUAL(pool.size(), 7); + sortedOrder.insert(sortedOrder.begin()+1, tx7.GetHash().ToString()); + CheckSort(pool, sortedOrder); + + /* after tx6 is mined, tx7 should move up in the sort */ + std::vector vtx; + vtx.push_back(tx6); + std::list dummy; + pool.removeForBlock(vtx, 1, dummy, false); + + sortedOrder.erase(sortedOrder.begin()+1); + sortedOrder.pop_back(); + sortedOrder.insert(sortedOrder.begin(), tx7.GetHash().ToString()); + CheckSort(pool, sortedOrder); +} + BOOST_AUTO_TEST_CASE(MempoolSizeLimitTest) { diff --git a/src/txmempool.cpp b/src/txmempool.cpp index f7961a6b80..48fbe56023 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -866,8 +866,8 @@ bool CCoinsViewMemPool::HaveCoins(const uint256 &txid) const { size_t CTxMemPool::DynamicMemoryUsage() const { LOCK(cs); - // Estimate the overhead of mapTx to be 12 pointers + an allocation, as no exact formula for boost::multi_index_contained is implemented. - return memusage::MallocUsage(sizeof(CTxMemPoolEntry) + 12 * sizeof(void*)) * mapTx.size() + memusage::DynamicUsage(mapNextTx) + memusage::DynamicUsage(mapDeltas) + memusage::DynamicUsage(mapLinks) + cachedInnerUsage; + // Estimate the overhead of mapTx to be 15 pointers + an allocation, as no exact formula for boost::multi_index_contained is implemented. + return memusage::MallocUsage(sizeof(CTxMemPoolEntry) + 15 * sizeof(void*)) * mapTx.size() + memusage::DynamicUsage(mapNextTx) + memusage::DynamicUsage(mapDeltas) + memusage::DynamicUsage(mapLinks) + cachedInnerUsage; } void CTxMemPool::RemoveStaged(setEntries &stage, bool updateDescendants) { diff --git a/src/txmempool.h b/src/txmempool.h index 0db3d5bd27..3f80a6ec24 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -244,10 +244,34 @@ public: } }; +class CompareTxMemPoolEntryByAncestorFee +{ +public: + bool operator()(const CTxMemPoolEntry& a, const CTxMemPoolEntry& b) + { + double aFees = a.GetModFeesWithAncestors(); + double aSize = a.GetSizeWithAncestors(); + + double bFees = b.GetModFeesWithAncestors(); + double bSize = b.GetSizeWithAncestors(); + + // Avoid division by rewriting (a/b > c/d) as (a*d > c*b). + double f1 = aFees * bSize; + double f2 = aSize * bFees; + + if (f1 == f2) { + return a.GetTx().GetHash() < b.GetTx().GetHash(); + } + + return f1 > f2; + } +}; + // Multi_index tag names struct descendant_score {}; struct entry_time {}; struct mining_score {}; +struct ancestor_score {}; class CBlockPolicyEstimator; @@ -380,12 +404,18 @@ public: boost::multi_index::tag, boost::multi_index::identity, CompareTxMemPoolEntryByEntryTime - >, + >, // sorted by score (for mining prioritization) boost::multi_index::ordered_unique< boost::multi_index::tag, boost::multi_index::identity, CompareTxMemPoolEntryByScore + >, + // sorted by fee rate with ancestors + boost::multi_index::ordered_non_unique< + boost::multi_index::tag, + boost::multi_index::identity, + CompareTxMemPoolEntryByAncestorFee > > > indexed_transaction_set; From ce019bf90fe89c1256a89c489795987ef0b8a18f Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Tue, 8 Mar 2016 15:49:26 -0500 Subject: [PATCH 6/6] Check all ancestor state in CTxMemPool::check() --- src/txmempool.cpp | 27 ++++++++++++++++++++++----- src/txmempool.h | 2 +- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 48fbe56023..ae851621c1 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -160,7 +160,7 @@ void CTxMemPool::UpdateTransactionsFromBlock(const std::vector &vHashes } } -bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry, setEntries &setAncestors, uint64_t limitAncestorCount, uint64_t limitAncestorSize, uint64_t limitDescendantCount, uint64_t limitDescendantSize, std::string &errString, bool fSearchForParents /* = true */) +bool CTxMemPool::CalculateMemPoolAncestors(const CTxMemPoolEntry &entry, setEntries &setAncestors, uint64_t limitAncestorCount, uint64_t limitAncestorSize, uint64_t limitDescendantCount, uint64_t limitDescendantSize, std::string &errString, bool fSearchForParents /* = true */) const { setEntries parentHashes; const CTransaction &tx = entry.GetTx(); @@ -666,10 +666,27 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const i++; } assert(setParentCheck == GetMemPoolParents(it)); - // Also check to make sure ancestor size/sigops are >= sum with immediate - // parents. - assert(it->GetSizeWithAncestors() >= parentSizes + it->GetTxSize()); - assert(it->GetSigOpCountWithAncestors() >= parentSigOpCount + it->GetSigOpCount()); + // Verify ancestor state is correct. + setEntries setAncestors; + uint64_t nNoLimit = std::numeric_limits::max(); + std::string dummy; + CalculateMemPoolAncestors(*it, setAncestors, nNoLimit, nNoLimit, nNoLimit, nNoLimit, dummy); + uint64_t nCountCheck = setAncestors.size() + 1; + uint64_t nSizeCheck = it->GetTxSize(); + CAmount nFeesCheck = it->GetModifiedFee(); + unsigned int nSigOpCheck = it->GetSigOpCount(); + + BOOST_FOREACH(txiter ancestorIt, setAncestors) { + nSizeCheck += ancestorIt->GetTxSize(); + nFeesCheck += ancestorIt->GetModifiedFee(); + nSigOpCheck += ancestorIt->GetSigOpCount(); + } + + assert(it->GetCountWithAncestors() == nCountCheck); + assert(it->GetSizeWithAncestors() == nSizeCheck); + assert(it->GetSigOpCountWithAncestors() == nSigOpCheck); + assert(it->GetModFeesWithAncestors() == nFeesCheck); + // Check children against mapNextTx CTxMemPool::setEntries setChildrenCheck; std::map::const_iterator iter = mapNextTx.lower_bound(COutPoint(it->GetTx().GetHash(), 0)); diff --git a/src/txmempool.h b/src/txmempool.h index 3f80a6ec24..a82d17c2f8 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -527,7 +527,7 @@ public: * fSearchForParents = whether to search a tx's vin for in-mempool parents, or * look up parents from mapLinks. Must be true for entries not in the mempool */ - bool CalculateMemPoolAncestors(const CTxMemPoolEntry &entry, setEntries &setAncestors, uint64_t limitAncestorCount, uint64_t limitAncestorSize, uint64_t limitDescendantCount, uint64_t limitDescendantSize, std::string &errString, bool fSearchForParents = true); + bool CalculateMemPoolAncestors(const CTxMemPoolEntry &entry, setEntries &setAncestors, uint64_t limitAncestorCount, uint64_t limitAncestorSize, uint64_t limitDescendantCount, uint64_t limitDescendantSize, std::string &errString, bool fSearchForParents = true) const; /** Populate setDescendants with all in-mempool descendants of hash. * Assumes that setDescendants includes all in-mempool descendants of anything