From 73d904009dc25ddfe5d6c4a91a13673c8f5cf87a Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Thu, 29 Oct 2015 22:49:00 -0400 Subject: [PATCH] Improve RBF replacement criteria Fix the calculation of conflicting size/conflicting fees. --- src/main.cpp | 59 +++++++++++++++++++++++++++++++++++++++---------- src/txmempool.h | 9 ++++---- 2 files changed, 52 insertions(+), 16 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 6e238f552..79d4c91b7 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -1004,18 +1004,39 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa // than the ones it replaces. CAmount nConflictingFees = 0; size_t nConflictingSize = 0; + uint64_t nConflictingCount = 0; + CTxMemPool::setEntries allConflicting; if (setConflicts.size()) { LOCK(pool.cs); CFeeRate newFeeRate(nFees, nSize); set setConflictsParents; - BOOST_FOREACH(const uint256 hashConflicting, setConflicts) + const int maxDescendantsToVisit = 100; + CTxMemPool::setEntries setIterConflicting; + BOOST_FOREACH(const uint256 &hashConflicting, setConflicts) { CTxMemPool::txiter mi = pool.mapTx.find(hashConflicting); if (mi == pool.mapTx.end()) continue; + // 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, + error("AcceptToMemoryPool: rejecting replacement %s; cannot replace tx %s with untracked descendants", + hash.ToString(), + mi->GetTx().GetHash().ToString()), + REJECT_NONSTANDARD, "too many potential replacements"); + } + // Don't allow the replacement to reduce the feerate of the // mempool. // @@ -1048,12 +1069,28 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa setConflictsParents.insert(txin.prevout.hash); } - // For efficiency we simply sum up the pre-calculated - // fees/size-with-descendants values from the mempool package - // tracking; this does mean the pathological case of diamond tx - // graphs will be overcounted. - nConflictingFees += mi->GetFeesWithDescendants(); - nConflictingSize += mi->GetSizeWithDescendants(); + nConflictingCount += mi->GetCountWithDescendants(); + } + // This potentially overestimates the number of actual descendants + // but we just want to be conservative to avoid doing too much + // work. + if (nConflictingCount <= maxDescendantsToVisit) { + // If not too many to replace, then calculate the set of + // transactions that would have to be evicted + BOOST_FOREACH(CTxMemPool::txiter it, setIterConflicting) { + pool.CalculateDescendants(it, allConflicting); + } + BOOST_FOREACH(CTxMemPool::txiter it, allConflicting) { + nConflictingFees += it->GetFee(); + nConflictingSize += it->GetTxSize(); + } + } else { + return state.DoS(0, + error("AcceptToMemoryPool: rejecting replacement %s; too many potential replacements (%d > %d)\n", + hash.ToString(), + nConflictingCount, + maxDescendantsToVisit), + REJECT_NONSTANDARD, "too many potential replacements"); } for (unsigned int j = 0; j < tx.vin.size(); j++) @@ -1119,17 +1156,15 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa } // Remove conflicting transactions from the mempool - list ltxConflicted; - pool.removeConflicts(tx, ltxConflicted); - - BOOST_FOREACH(const CTransaction &txConflicted, ltxConflicted) + BOOST_FOREACH(const CTxMemPool::txiter it, allConflicting) { LogPrint("mempool", "replacing tx %s with %s for %s BTC additional fees, %d delta bytes\n", - txConflicted.GetHash().ToString(), + it->GetTx().GetHash().ToString(), hash.ToString(), FormatMoney(nFees - nConflictingFees), (int)nSize - (int)nConflictingSize); } + pool.RemoveStaged(allConflicting); // Store transaction in memory pool.addUnchecked(hash, entry, setAncestors, !IsInitialBlockDownload()); diff --git a/src/txmempool.h b/src/txmempool.h index 7b5843a8d..3d8ac435f 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -420,6 +420,11 @@ public: */ 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); + /** Populate setDescendants with all in-mempool descendants of hash. + * Assumes that setDescendants includes all in-mempool descendants of anything + * already in it. */ + void CalculateDescendants(txiter it, setEntries &setDescendants); + /** The minimum fee to get into the mempool, which may itself not be enough * for larger-sized transactions. * The minReasonableRelayFee constructor arg is used to bound the time it @@ -493,10 +498,6 @@ private: void UpdateForRemoveFromMempool(const setEntries &entriesToRemove); /** Sever link between specified transaction and direct children. */ void UpdateChildrenForRemoval(txiter entry); - /** Populate setDescendants with all in-mempool descendants of hash. - * Assumes that setDescendants includes all in-mempool descendants of anything - * already in it. */ - void CalculateDescendants(txiter it, setEntries &setDescendants); /** Before calling removeUnchecked for a given transaction, * UpdateForRemoveFromMempool must be called on the entire (dependent) set