Fix mempool limiting for PrioritiseTransaction

Redo the feerate index to be based on mining score, rather than fee.

Update mempool_packages.py to test prioritisetransaction's effect on
package scores.
This commit is contained in:
Suhas Daftuar 2015-11-19 11:18:28 -05:00 committed by Suhas Daftuar
parent aeedd8a53b
commit eb306664e7
4 changed files with 78 additions and 46 deletions

View File

@ -64,17 +64,41 @@ class MempoolPackagesTest(BitcoinTestFramework):
for x in reversed(chain): for x in reversed(chain):
assert_equal(mempool[x]['descendantcount'], descendant_count) assert_equal(mempool[x]['descendantcount'], descendant_count)
descendant_fees += mempool[x]['fee'] descendant_fees += mempool[x]['fee']
assert_equal(mempool[x]['modifiedfee'], mempool[x]['fee'])
assert_equal(mempool[x]['descendantfees'], SATOSHIS*descendant_fees) assert_equal(mempool[x]['descendantfees'], SATOSHIS*descendant_fees)
descendant_size += mempool[x]['size'] descendant_size += mempool[x]['size']
assert_equal(mempool[x]['descendantsize'], descendant_size) assert_equal(mempool[x]['descendantsize'], descendant_size)
descendant_count += 1 descendant_count += 1
# Check that descendant modified fees includes fee deltas from
# prioritisetransaction
self.nodes[0].prioritisetransaction(chain[-1], 0, 1000)
mempool = self.nodes[0].getrawmempool(True)
descendant_fees = 0
for x in reversed(chain):
descendant_fees += mempool[x]['fee']
assert_equal(mempool[x]['descendantfees'], SATOSHIS*descendant_fees+1000)
# Adding one more transaction on to the chain should fail. # Adding one more transaction on to the chain should fail.
try: try:
self.chain_transaction(self.nodes[0], txid, vout, value, fee, 1) self.chain_transaction(self.nodes[0], txid, vout, value, fee, 1)
except JSONRPCException as e: except JSONRPCException as e:
print "too-long-ancestor-chain successfully rejected" print "too-long-ancestor-chain successfully rejected"
# Check that prioritising a tx before it's added to the mempool works
self.nodes[0].generate(1)
self.nodes[0].prioritisetransaction(chain[-1], 0, 2000)
self.nodes[0].invalidateblock(self.nodes[0].getbestblockhash())
mempool = self.nodes[0].getrawmempool(True)
descendant_fees = 0
for x in reversed(chain):
descendant_fees += mempool[x]['fee']
if (x == chain[-1]):
assert_equal(mempool[x]['modifiedfee'], mempool[x]['fee']+satoshi_round(0.00002))
assert_equal(mempool[x]['descendantfees'], SATOSHIS*descendant_fees+2000)
# TODO: check that node1's mempool is as expected # TODO: check that node1's mempool is as expected
# TODO: test ancestor size limits # TODO: test ancestor size limits

View File

@ -197,7 +197,7 @@ UniValue mempoolToJSON(bool fVerbose = false)
info.push_back(Pair("currentpriority", e.GetPriority(chainActive.Height()))); info.push_back(Pair("currentpriority", e.GetPriority(chainActive.Height())));
info.push_back(Pair("descendantcount", e.GetCountWithDescendants())); info.push_back(Pair("descendantcount", e.GetCountWithDescendants()));
info.push_back(Pair("descendantsize", e.GetSizeWithDescendants())); info.push_back(Pair("descendantsize", e.GetSizeWithDescendants()));
info.push_back(Pair("descendantfees", e.GetFeesWithDescendants())); info.push_back(Pair("descendantfees", e.GetModFeesWithDescendants()));
const CTransaction& tx = e.GetTx(); const CTransaction& tx = e.GetTx();
set<string> setDepends; set<string> setDepends;
BOOST_FOREACH(const CTxIn& txin, tx.vin) BOOST_FOREACH(const CTxIn& txin, tx.vin)
@ -255,7 +255,7 @@ UniValue getrawmempool(const UniValue& params, bool fHelp)
" \"currentpriority\" : n, (numeric) transaction priority now\n" " \"currentpriority\" : n, (numeric) transaction priority now\n"
" \"descendantcount\" : n, (numeric) number of in-mempool descendant transactions (including this one)\n" " \"descendantcount\" : n, (numeric) number of in-mempool descendant transactions (including this one)\n"
" \"descendantsize\" : n, (numeric) size of in-mempool descendants (including this one)\n" " \"descendantsize\" : n, (numeric) size of in-mempool descendants (including this one)\n"
" \"descendantfees\" : n, (numeric) fees of in-mempool descendants (including this one)\n" " \"descendantfees\" : n, (numeric) modified fees (see above) of in-mempool descendants (including this one)\n"
" \"depends\" : [ (array) unconfirmed transactions used as inputs for this transaction\n" " \"depends\" : [ (array) unconfirmed transactions used as inputs for this transaction\n"
" \"transactionid\", (string) parent transaction id\n" " \"transactionid\", (string) parent transaction id\n"
" ... ]\n" " ... ]\n"

View File

@ -33,7 +33,7 @@ CTxMemPoolEntry::CTxMemPoolEntry(const CTransaction& _tx, const CAmount& _nFee,
nCountWithDescendants = 1; nCountWithDescendants = 1;
nSizeWithDescendants = nTxSize; nSizeWithDescendants = nTxSize;
nFeesWithDescendants = nFee; nModFeesWithDescendants = nFee;
CAmount nValueIn = tx.GetValueOut()+nFee; CAmount nValueIn = tx.GetValueOut()+nFee;
assert(inChainInputValue <= nValueIn); assert(inChainInputValue <= nValueIn);
@ -57,6 +57,7 @@ CTxMemPoolEntry::GetPriority(unsigned int currentHeight) const
void CTxMemPoolEntry::UpdateFeeDelta(int64_t newFeeDelta) void CTxMemPoolEntry::UpdateFeeDelta(int64_t newFeeDelta)
{ {
nModFeesWithDescendants += newFeeDelta - feeDelta;
feeDelta = newFeeDelta; feeDelta = newFeeDelta;
} }
@ -114,7 +115,7 @@ bool CTxMemPool::UpdateForDescendants(txiter updateIt, int maxDescendantsToVisit
BOOST_FOREACH(txiter cit, setAllDescendants) { BOOST_FOREACH(txiter cit, setAllDescendants) {
if (!setExclude.count(cit->GetTx().GetHash())) { if (!setExclude.count(cit->GetTx().GetHash())) {
modifySize += cit->GetTxSize(); modifySize += cit->GetTxSize();
modifyFee += cit->GetFee(); modifyFee += cit->GetModifiedFee();
modifyCount++; modifyCount++;
cachedDescendants[updateIt].insert(cit); cachedDescendants[updateIt].insert(cit);
} }
@ -244,7 +245,7 @@ void CTxMemPool::UpdateAncestorsOf(bool add, txiter it, setEntries &setAncestors
} }
const int64_t updateCount = (add ? 1 : -1); const int64_t updateCount = (add ? 1 : -1);
const int64_t updateSize = updateCount * it->GetTxSize(); const int64_t updateSize = updateCount * it->GetTxSize();
const CAmount updateFee = updateCount * it->GetFee(); const CAmount updateFee = updateCount * it->GetModifiedFee();
BOOST_FOREACH(txiter ancestorIt, setAncestors) { BOOST_FOREACH(txiter ancestorIt, setAncestors) {
mapTx.modify(ancestorIt, update_descendant_state(updateSize, updateFee, updateCount)); mapTx.modify(ancestorIt, update_descendant_state(updateSize, updateFee, updateCount));
} }
@ -304,7 +305,7 @@ void CTxMemPoolEntry::SetDirty()
{ {
nCountWithDescendants = 0; nCountWithDescendants = 0;
nSizeWithDescendants = nTxSize; nSizeWithDescendants = nTxSize;
nFeesWithDescendants = nFee; nModFeesWithDescendants = GetModifiedFee();
} }
void CTxMemPoolEntry::UpdateState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount) void CTxMemPoolEntry::UpdateState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount)
@ -312,8 +313,7 @@ void CTxMemPoolEntry::UpdateState(int64_t modifySize, CAmount modifyFee, int64_t
if (!IsDirty()) { if (!IsDirty()) {
nSizeWithDescendants += modifySize; nSizeWithDescendants += modifySize;
assert(int64_t(nSizeWithDescendants) > 0); assert(int64_t(nSizeWithDescendants) > 0);
nFeesWithDescendants += modifyFee; nModFeesWithDescendants += modifyFee;
assert(nFeesWithDescendants >= 0);
nCountWithDescendants += modifyCount; nCountWithDescendants += modifyCount;
assert(int64_t(nCountWithDescendants) > 0); assert(int64_t(nCountWithDescendants) > 0);
} }
@ -372,6 +372,17 @@ bool CTxMemPool::addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry,
indexed_transaction_set::iterator newit = mapTx.insert(entry).first; indexed_transaction_set::iterator newit = mapTx.insert(entry).first;
mapLinks.insert(make_pair(newit, TxLinks())); mapLinks.insert(make_pair(newit, TxLinks()));
// Update transaction for any feeDelta created by PrioritiseTransaction
// TODO: refactor so that the fee delta is calculated before inserting
// into mapTx.
std::map<uint256, std::pair<double, CAmount> >::const_iterator pos = mapDeltas.find(hash);
if (pos != mapDeltas.end()) {
const std::pair<double, CAmount> &deltas = pos->second;
if (deltas.second) {
mapTx.modify(newit, update_fee_delta(deltas.second));
}
}
// Update cachedInnerUsage to include contained transaction's usage. // Update cachedInnerUsage to include contained transaction's usage.
// (When we update the entry for in-mempool parents, memory usage will be // (When we update the entry for in-mempool parents, memory usage will be
// further updated.) // further updated.)
@ -399,15 +410,6 @@ bool CTxMemPool::addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry,
} }
UpdateAncestorsOf(true, newit, setAncestors); UpdateAncestorsOf(true, newit, setAncestors);
// Update transaction's score for any feeDelta created by PrioritiseTransaction
std::map<uint256, std::pair<double, CAmount> >::const_iterator pos = mapDeltas.find(hash);
if (pos != mapDeltas.end()) {
const std::pair<double, CAmount> &deltas = pos->second;
if (deltas.second) {
mapTx.modify(newit, update_fee_delta(deltas.second));
}
}
nTransactionsUpdated++; nTransactionsUpdated++;
totalTxSize += entry.GetTxSize(); totalTxSize += entry.GetTxSize();
minerPolicyEstimator->processTransaction(entry, fCurrentEstimate); minerPolicyEstimator->processTransaction(entry, fCurrentEstimate);
@ -644,27 +646,24 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const
CTxMemPool::setEntries setChildrenCheck; CTxMemPool::setEntries setChildrenCheck;
std::map<COutPoint, CInPoint>::const_iterator iter = mapNextTx.lower_bound(COutPoint(it->GetTx().GetHash(), 0)); std::map<COutPoint, CInPoint>::const_iterator iter = mapNextTx.lower_bound(COutPoint(it->GetTx().GetHash(), 0));
int64_t childSizes = 0; int64_t childSizes = 0;
CAmount childFees = 0; CAmount childModFee = 0;
for (; iter != mapNextTx.end() && iter->first.hash == it->GetTx().GetHash(); ++iter) { for (; iter != mapNextTx.end() && iter->first.hash == it->GetTx().GetHash(); ++iter) {
txiter childit = mapTx.find(iter->second.ptx->GetHash()); txiter childit = mapTx.find(iter->second.ptx->GetHash());
assert(childit != mapTx.end()); // mapNextTx points to in-mempool transactions assert(childit != mapTx.end()); // mapNextTx points to in-mempool transactions
if (setChildrenCheck.insert(childit).second) { if (setChildrenCheck.insert(childit).second) {
childSizes += childit->GetTxSize(); childSizes += childit->GetTxSize();
childFees += childit->GetFee(); childModFee += childit->GetModifiedFee();
} }
} }
assert(setChildrenCheck == GetMemPoolChildren(it)); assert(setChildrenCheck == GetMemPoolChildren(it));
// Also check to make sure size/fees is greater than sum with immediate children. // Also check to make sure size is greater than sum with immediate children.
// just a sanity check, not definitive that this calc is correct... // just a sanity check, not definitive that this calc is correct...
// also check that the size is less than the size of the entire mempool.
if (!it->IsDirty()) { if (!it->IsDirty()) {
assert(it->GetSizeWithDescendants() >= childSizes + it->GetTxSize()); assert(it->GetSizeWithDescendants() >= childSizes + it->GetTxSize());
assert(it->GetFeesWithDescendants() >= childFees + it->GetFee());
} else { } else {
assert(it->GetSizeWithDescendants() == it->GetTxSize()); assert(it->GetSizeWithDescendants() == it->GetTxSize());
assert(it->GetFeesWithDescendants() == it->GetFee()); assert(it->GetModFeesWithDescendants() == it->GetModifiedFee());
} }
assert(it->GetFeesWithDescendants() >= 0);
if (fDependsWait) if (fDependsWait)
waitingOnDependants.push_back(&(*it)); waitingOnDependants.push_back(&(*it));
@ -788,6 +787,14 @@ void CTxMemPool::PrioritiseTransaction(const uint256 hash, const string strHash,
txiter it = mapTx.find(hash); txiter it = mapTx.find(hash);
if (it != mapTx.end()) { if (it != mapTx.end()) {
mapTx.modify(it, update_fee_delta(deltas.second)); mapTx.modify(it, update_fee_delta(deltas.second));
// Now update all ancestors' modified fees with descendants
setEntries setAncestors;
uint64_t nNoLimit = std::numeric_limits<uint64_t>::max();
std::string dummy;
CalculateMemPoolAncestors(*it, setAncestors, nNoLimit, nNoLimit, nNoLimit, nNoLimit, dummy, false);
BOOST_FOREACH(txiter ancestorIt, setAncestors) {
mapTx.modify(ancestorIt, update_descendant_state(0, nFeeDelta, 0));
}
} }
} }
LogPrintf("PrioritiseTransaction: %s priority += %f, fee += %d\n", strHash, dPriorityDelta, FormatMoney(nFeeDelta)); LogPrintf("PrioritiseTransaction: %s priority += %f, fee += %d\n", strHash, dPriorityDelta, FormatMoney(nFeeDelta));
@ -956,7 +963,7 @@ void CTxMemPool::TrimToSize(size_t sizelimit, std::vector<uint256>* pvNoSpendsRe
// "minimum reasonable fee rate" (ie some value under which we consider txn // "minimum reasonable fee rate" (ie some value under which we consider txn
// to have 0 fee). This way, we don't allow txn to enter mempool with feerate // to have 0 fee). This way, we don't allow txn to enter mempool with feerate
// equal to txn which were removed with no block in between. // equal to txn which were removed with no block in between.
CFeeRate removed(it->GetFeesWithDescendants(), it->GetSizeWithDescendants()); CFeeRate removed(it->GetModFeesWithDescendants(), it->GetSizeWithDescendants());
removed += minReasonableRelayFee; removed += minReasonableRelayFee;
trackPackageRemoved(removed); trackPackageRemoved(removed);
maxFeeRateRemoved = std::max(maxFeeRateRemoved, removed); maxFeeRateRemoved = std::max(maxFeeRateRemoved, removed);

View File

@ -44,12 +44,12 @@ class CTxMemPool;
* ("descendant" transactions). * ("descendant" transactions).
* *
* When a new entry is added to the mempool, we update the descendant state * When a new entry is added to the mempool, we update the descendant state
* (nCountWithDescendants, nSizeWithDescendants, and nFeesWithDescendants) for * (nCountWithDescendants, nSizeWithDescendants, and nModFeesWithDescendants) for
* all ancestors of the newly added transaction. * all ancestors of the newly added transaction.
* *
* If updating the descendant state is skipped, we can mark the entry as * If updating the descendant state is skipped, we can mark the entry as
* "dirty", and set nSizeWithDescendants/nFeesWithDescendants to equal nTxSize/ * "dirty", and set nSizeWithDescendants/nModFeesWithDescendants to equal nTxSize/
* nTxFee. (This can potentially happen during a reorg, where we limit the * nFee+feeDelta. (This can potentially happen during a reorg, where we limit the
* amount of work we're willing to do to avoid consuming too much CPU.) * amount of work we're willing to do to avoid consuming too much CPU.)
* *
*/ */
@ -74,11 +74,11 @@ private:
// Information about descendants of this transaction that are in the // Information about descendants of this transaction that are in the
// mempool; if we remove this transaction we must remove all of these // mempool; if we remove this transaction we must remove all of these
// descendants as well. if nCountWithDescendants is 0, treat this entry as // descendants as well. if nCountWithDescendants is 0, treat this entry as
// dirty, and nSizeWithDescendants and nFeesWithDescendants will not be // dirty, and nSizeWithDescendants and nModFeesWithDescendants will not be
// correct. // correct.
uint64_t nCountWithDescendants; //! number of descendant transactions uint64_t nCountWithDescendants; //! number of descendant transactions
uint64_t nSizeWithDescendants; //! ... and size uint64_t nSizeWithDescendants; //! ... and size
CAmount nFeesWithDescendants; //! ... and total fees (all including us) CAmount nModFeesWithDescendants; //! ... and total fees (all including us)
public: public:
CTxMemPoolEntry(const CTransaction& _tx, const CAmount& _nFee, CTxMemPoolEntry(const CTransaction& _tx, const CAmount& _nFee,
@ -104,7 +104,8 @@ public:
// Adjusts the descendant state, if this entry is not dirty. // Adjusts the descendant state, if this entry is not dirty.
void UpdateState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount); void UpdateState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount);
// Updates the fee delta used for mining priority score // Updates the fee delta used for mining priority score, and the
// modified fees with descendants.
void UpdateFeeDelta(int64_t feeDelta); void UpdateFeeDelta(int64_t feeDelta);
/** We can set the entry to be dirty if doing the full calculation of in- /** We can set the entry to be dirty if doing the full calculation of in-
@ -116,7 +117,7 @@ public:
uint64_t GetCountWithDescendants() const { return nCountWithDescendants; } uint64_t GetCountWithDescendants() const { return nCountWithDescendants; }
uint64_t GetSizeWithDescendants() const { return nSizeWithDescendants; } uint64_t GetSizeWithDescendants() const { return nSizeWithDescendants; }
CAmount GetFeesWithDescendants() const { return nFeesWithDescendants; } CAmount GetModFeesWithDescendants() const { return nModFeesWithDescendants; }
bool GetSpendsCoinbase() const { return spendsCoinbase; } bool GetSpendsCoinbase() const { return spendsCoinbase; }
}; };
@ -163,27 +164,27 @@ struct mempoolentry_txid
} }
}; };
/** \class CompareTxMemPoolEntryByFee /** \class CompareTxMemPoolEntryByDescendantScore
* *
* Sort an entry by max(feerate of entry's tx, feerate with all descendants). * Sort an entry by max(score/size of entry's tx, score/size with all descendants).
*/ */
class CompareTxMemPoolEntryByFee class CompareTxMemPoolEntryByDescendantScore
{ {
public: public:
bool operator()(const CTxMemPoolEntry& a, const CTxMemPoolEntry& b) bool operator()(const CTxMemPoolEntry& a, const CTxMemPoolEntry& b)
{ {
bool fUseADescendants = UseDescendantFeeRate(a); bool fUseADescendants = UseDescendantScore(a);
bool fUseBDescendants = UseDescendantFeeRate(b); bool fUseBDescendants = UseDescendantScore(b);
double aFees = fUseADescendants ? a.GetFeesWithDescendants() : a.GetFee(); double aModFee = fUseADescendants ? a.GetModFeesWithDescendants() : a.GetModifiedFee();
double aSize = fUseADescendants ? a.GetSizeWithDescendants() : a.GetTxSize(); double aSize = fUseADescendants ? a.GetSizeWithDescendants() : a.GetTxSize();
double bFees = fUseBDescendants ? b.GetFeesWithDescendants() : b.GetFee(); double bModFee = fUseBDescendants ? b.GetModFeesWithDescendants() : b.GetModifiedFee();
double bSize = fUseBDescendants ? b.GetSizeWithDescendants() : b.GetTxSize(); double bSize = fUseBDescendants ? b.GetSizeWithDescendants() : b.GetTxSize();
// Avoid division by rewriting (a/b > c/d) as (a*d > c*b). // Avoid division by rewriting (a/b > c/d) as (a*d > c*b).
double f1 = aFees * bSize; double f1 = aModFee * bSize;
double f2 = aSize * bFees; double f2 = aSize * bModFee;
if (f1 == f2) { if (f1 == f2) {
return a.GetTime() >= b.GetTime(); return a.GetTime() >= b.GetTime();
@ -191,11 +192,11 @@ public:
return f1 < f2; return f1 < f2;
} }
// Calculate which feerate to use for an entry (avoiding division). // Calculate which score to use for an entry (avoiding division).
bool UseDescendantFeeRate(const CTxMemPoolEntry &a) bool UseDescendantScore(const CTxMemPoolEntry &a)
{ {
double f1 = (double)a.GetFee() * a.GetSizeWithDescendants(); double f1 = (double)a.GetModifiedFee() * a.GetSizeWithDescendants();
double f2 = (double)a.GetFeesWithDescendants() * a.GetTxSize(); double f2 = (double)a.GetModFeesWithDescendants() * a.GetTxSize();
return f2 > f1; return f2 > f1;
} }
}; };
@ -350,7 +351,7 @@ public:
// sorted by fee rate // sorted by fee rate
boost::multi_index::ordered_non_unique< boost::multi_index::ordered_non_unique<
boost::multi_index::identity<CTxMemPoolEntry>, boost::multi_index::identity<CTxMemPoolEntry>,
CompareTxMemPoolEntryByFee CompareTxMemPoolEntryByDescendantScore
>, >,
// sorted by entry time // sorted by entry time
boost::multi_index::ordered_non_unique< boost::multi_index::ordered_non_unique<