Improve RBF replacement criteria

Fix the calculation of conflicting size/conflicting fees.
This commit is contained in:
Suhas Daftuar 2015-10-29 22:49:00 -04:00 committed by Peter Todd
parent b272ecfdb3
commit 73d904009d
2 changed files with 52 additions and 16 deletions

View File

@ -1004,18 +1004,39 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa
// than the ones it replaces. // than the ones it replaces.
CAmount nConflictingFees = 0; CAmount nConflictingFees = 0;
size_t nConflictingSize = 0; size_t nConflictingSize = 0;
uint64_t nConflictingCount = 0;
CTxMemPool::setEntries allConflicting;
if (setConflicts.size()) if (setConflicts.size())
{ {
LOCK(pool.cs); LOCK(pool.cs);
CFeeRate newFeeRate(nFees, nSize); CFeeRate newFeeRate(nFees, nSize);
set<uint256> setConflictsParents; set<uint256> 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); CTxMemPool::txiter mi = pool.mapTx.find(hashConflicting);
if (mi == pool.mapTx.end()) if (mi == pool.mapTx.end())
continue; 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 // Don't allow the replacement to reduce the feerate of the
// mempool. // mempool.
// //
@ -1048,12 +1069,28 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa
setConflictsParents.insert(txin.prevout.hash); setConflictsParents.insert(txin.prevout.hash);
} }
// For efficiency we simply sum up the pre-calculated nConflictingCount += mi->GetCountWithDescendants();
// fees/size-with-descendants values from the mempool package }
// tracking; this does mean the pathological case of diamond tx // This potentially overestimates the number of actual descendants
// graphs will be overcounted. // but we just want to be conservative to avoid doing too much
nConflictingFees += mi->GetFeesWithDescendants(); // work.
nConflictingSize += mi->GetSizeWithDescendants(); 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++) 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 // Remove conflicting transactions from the mempool
list<CTransaction> ltxConflicted; BOOST_FOREACH(const CTxMemPool::txiter it, allConflicting)
pool.removeConflicts(tx, ltxConflicted);
BOOST_FOREACH(const CTransaction &txConflicted, ltxConflicted)
{ {
LogPrint("mempool", "replacing tx %s with %s for %s BTC additional fees, %d delta bytes\n", 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(), hash.ToString(),
FormatMoney(nFees - nConflictingFees), FormatMoney(nFees - nConflictingFees),
(int)nSize - (int)nConflictingSize); (int)nSize - (int)nConflictingSize);
} }
pool.RemoveStaged(allConflicting);
// Store transaction in memory // Store transaction in memory
pool.addUnchecked(hash, entry, setAncestors, !IsInitialBlockDownload()); pool.addUnchecked(hash, entry, setAncestors, !IsInitialBlockDownload());

View File

@ -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); 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 /** The minimum fee to get into the mempool, which may itself not be enough
* for larger-sized transactions. * for larger-sized transactions.
* The minReasonableRelayFee constructor arg is used to bound the time it * The minReasonableRelayFee constructor arg is used to bound the time it
@ -493,10 +498,6 @@ private:
void UpdateForRemoveFromMempool(const setEntries &entriesToRemove); void UpdateForRemoveFromMempool(const setEntries &entriesToRemove);
/** Sever link between specified transaction and direct children. */ /** Sever link between specified transaction and direct children. */
void UpdateChildrenForRemoval(txiter entry); 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, /** Before calling removeUnchecked for a given transaction,
* UpdateForRemoveFromMempool must be called on the entire (dependent) set * UpdateForRemoveFromMempool must be called on the entire (dependent) set