Skip to content

Commit eb30666

Browse files
sdaftuarsdaftuar
committed
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.
1 parent aeedd8a commit eb30666

File tree

4 files changed

+78
-46
lines changed

4 files changed

+78
-46
lines changed

‎qa/rpc-tests/mempool_packages.py‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,17 +64,41 @@ def run_test(self):
6464
for x in reversed(chain):
6565
assert_equal(mempool[x]['descendantcount'], descendant_count)
6666
descendant_fees += mempool[x]['fee']
67+
assert_equal(mempool[x]['modifiedfee'], mempool[x]['fee'])
6768
assert_equal(mempool[x]['descendantfees'], SATOSHIS*descendant_fees)
6869
descendant_size += mempool[x]['size']
6970
assert_equal(mempool[x]['descendantsize'], descendant_size)
7071
descendant_count += 1
7172

73+
# Check that descendant modified fees includes fee deltas from
74+
# prioritisetransaction
75+
self.nodes[0].prioritisetransaction(chain[-1], 0, 1000)
76+
mempool = self.nodes[0].getrawmempool(True)
77+
78+
descendant_fees = 0
79+
for x in reversed(chain):
80+
descendant_fees += mempool[x]['fee']
81+
assert_equal(mempool[x]['descendantfees'], SATOSHIS*descendant_fees+1000)
82+
7283
# Adding one more transaction on to the chain should fail.
7384
try:
7485
self.chain_transaction(self.nodes[0], txid, vout, value, fee, 1)
7586
except JSONRPCException as e:
7687
print "too-long-ancestor-chain successfully rejected"
7788

89+
# Check that prioritising a tx before it's added to the mempool works
90+
self.nodes[0].generate(1)
91+
self.nodes[0].prioritisetransaction(chain[-1], 0, 2000)
92+
self.nodes[0].invalidateblock(self.nodes[0].getbestblockhash())
93+
mempool = self.nodes[0].getrawmempool(True)
94+
95+
descendant_fees = 0
96+
for x in reversed(chain):
97+
descendant_fees += mempool[x]['fee']
98+
if (x == chain[-1]):
99+
assert_equal(mempool[x]['modifiedfee'], mempool[x]['fee']+satoshi_round(0.00002))
100+
assert_equal(mempool[x]['descendantfees'], SATOSHIS*descendant_fees+2000)
101+
78102
# TODO: check that node1's mempool is as expected
79103

80104
# TODO: test ancestor size limits

‎src/rpcblockchain.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -197,7 +197,7 @@ UniValue mempoolToJSON(bool fVerbose = false)
197197
info.push_back(Pair("currentpriority", e.GetPriority(chainActive.Height())));
198198
info.push_back(Pair("descendantcount", e.GetCountWithDescendants()));
199199
info.push_back(Pair("descendantsize", e.GetSizeWithDescendants()));
200-
info.push_back(Pair("descendantfees", e.GetFeesWithDescendants()));
200+
info.push_back(Pair("descendantfees", e.GetModFeesWithDescendants()));
201201
const CTransaction& tx = e.GetTx();
202202
set<string> setDepends;
203203
BOOST_FOREACH(const CTxIn& txin, tx.vin)
@@ -255,7 +255,7 @@ UniValue getrawmempool(const UniValue& params, bool fHelp)
255255
" \"currentpriority\" : n, (numeric) transaction priority now\n"
256256
" \"descendantcount\" : n, (numeric) number of in-mempool descendant transactions (including this one)\n"
257257
" \"descendantsize\" : n, (numeric) size of in-mempool descendants (including this one)\n"
258-
" \"descendantfees\" : n, (numeric) fees of in-mempool descendants (including this one)\n"
258+
" \"descendantfees\" : n, (numeric) modified fees (see above) of in-mempool descendants (including this one)\n"
259259
" \"depends\" : [ (array) unconfirmed transactions used as inputs for this transaction\n"
260260
" \"transactionid\", (string) parent transaction id\n"
261261
" ... ]\n"

‎src/txmempool.cpp‎

Lines changed: 30 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ CTxMemPoolEntry::CTxMemPoolEntry(const CTransaction& _tx, const CAmount& _nFee,
3333

3434
nCountWithDescendants = 1;
3535
nSizeWithDescendants = nTxSize;
36-
nFeesWithDescendants = nFee;
36+
nModFeesWithDescendants = nFee;
3737
CAmount nValueIn = tx.GetValueOut()+nFee;
3838
assert(inChainInputValue <= nValueIn);
3939

@@ -57,6 +57,7 @@ CTxMemPoolEntry::GetPriority(unsigned int currentHeight) const
5757

5858
void CTxMemPoolEntry::UpdateFeeDelta(int64_t newFeeDelta)
5959
{
60+
nModFeesWithDescendants += newFeeDelta - feeDelta;
6061
feeDelta = newFeeDelta;
6162
}
6263

@@ -114,7 +115,7 @@ bool CTxMemPool::UpdateForDescendants(txiter updateIt, int maxDescendantsToVisit
114115
BOOST_FOREACH(txiter cit, setAllDescendants) {
115116
if (!setExclude.count(cit->GetTx().GetHash())) {
116117
modifySize += cit->GetTxSize();
117-
modifyFee += cit->GetFee();
118+
modifyFee += cit->GetModifiedFee();
118119
modifyCount++;
119120
cachedDescendants[updateIt].insert(cit);
120121
}
@@ -244,7 +245,7 @@ void CTxMemPool::UpdateAncestorsOf(bool add, txiter it, setEntries &setAncestors
244245
}
245246
const int64_t updateCount = (add ? 1 : -1);
246247
const int64_t updateSize = updateCount * it->GetTxSize();
247-
const CAmount updateFee = updateCount * it->GetFee();
248+
const CAmount updateFee = updateCount * it->GetModifiedFee();
248249
BOOST_FOREACH(txiter ancestorIt, setAncestors) {
249250
mapTx.modify(ancestorIt, update_descendant_state(updateSize, updateFee, updateCount));
250251
}
@@ -304,16 +305,15 @@ void CTxMemPoolEntry::SetDirty()
304305
{
305306
nCountWithDescendants = 0;
306307
nSizeWithDescendants = nTxSize;
307-
nFeesWithDescendants = nFee;
308+
nModFeesWithDescendants = GetModifiedFee();
308309
}
309310

310311
void CTxMemPoolEntry::UpdateState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount)
311312
{
312313
if (!IsDirty()) {
313314
nSizeWithDescendants += modifySize;
314315
assert(int64_t(nSizeWithDescendants) > 0);
315-
nFeesWithDescendants += modifyFee;
316-
assert(nFeesWithDescendants >= 0);
316+
nModFeesWithDescendants += modifyFee;
317317
nCountWithDescendants += modifyCount;
318318
assert(int64_t(nCountWithDescendants) > 0);
319319
}
@@ -372,6 +372,17 @@ bool CTxMemPool::addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry,
372372
indexed_transaction_set::iterator newit = mapTx.insert(entry).first;
373373
mapLinks.insert(make_pair(newit, TxLinks()));
374374

375+
// Update transaction for any feeDelta created by PrioritiseTransaction
376+
// TODO: refactor so that the fee delta is calculated before inserting
377+
// into mapTx.
378+
std::map<uint256, std::pair<double, CAmount> >::const_iterator pos = mapDeltas.find(hash);
379+
if (pos != mapDeltas.end()) {
380+
const std::pair<double, CAmount> &deltas = pos->second;
381+
if (deltas.second) {
382+
mapTx.modify(newit, update_fee_delta(deltas.second));
383+
}
384+
}
385+
375386
// Update cachedInnerUsage to include contained transaction's usage.
376387
// (When we update the entry for in-mempool parents, memory usage will be
377388
// further updated.)
@@ -399,15 +410,6 @@ bool CTxMemPool::addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry,
399410
}
400411
UpdateAncestorsOf(true, newit, setAncestors);
401412

402-
// Update transaction's score for any feeDelta created by PrioritiseTransaction
403-
std::map<uint256, std::pair<double, CAmount> >::const_iterator pos = mapDeltas.find(hash);
404-
if (pos != mapDeltas.end()) {
405-
const std::pair<double, CAmount> &deltas = pos->second;
406-
if (deltas.second) {
407-
mapTx.modify(newit, update_fee_delta(deltas.second));
408-
}
409-
}
410-
411413
nTransactionsUpdated++;
412414
totalTxSize += entry.GetTxSize();
413415
minerPolicyEstimator->processTransaction(entry, fCurrentEstimate);
@@ -644,27 +646,24 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const
644646
CTxMemPool::setEntries setChildrenCheck;
645647
std::map<COutPoint, CInPoint>::const_iterator iter = mapNextTx.lower_bound(COutPoint(it->GetTx().GetHash(), 0));
646648
int64_t childSizes = 0;
647-
CAmount childFees = 0;
649+
CAmount childModFee = 0;
648650
for (; iter != mapNextTx.end() && iter->first.hash == it->GetTx().GetHash(); ++iter) {
649651
txiter childit = mapTx.find(iter->second.ptx->GetHash());
650652
assert(childit != mapTx.end()); // mapNextTx points to in-mempool transactions
651653
if (setChildrenCheck.insert(childit).second) {
652654
childSizes += childit->GetTxSize();
653-
childFees += childit->GetFee();
655+
childModFee += childit->GetModifiedFee();
654656
}
655657
}
656658
assert(setChildrenCheck == GetMemPoolChildren(it));
657-
// Also check to make sure size/fees is greater than sum with immediate children.
659+
// Also check to make sure size is greater than sum with immediate children.
658660
// just a sanity check, not definitive that this calc is correct...
659-
// also check that the size is less than the size of the entire mempool.
660661
if (!it->IsDirty()) {
661662
assert(it->GetSizeWithDescendants() >= childSizes + it->GetTxSize());
662-
assert(it->GetFeesWithDescendants() >= childFees + it->GetFee());
663663
} else {
664664
assert(it->GetSizeWithDescendants() == it->GetTxSize());
665-
assert(it->GetFeesWithDescendants() == it->GetFee());
665+
assert(it->GetModFeesWithDescendants() == it->GetModifiedFee());
666666
}
667-
assert(it->GetFeesWithDescendants() >= 0);
668667

669668
if (fDependsWait)
670669
waitingOnDependants.push_back(&(*it));
@@ -788,6 +787,14 @@ void CTxMemPool::PrioritiseTransaction(const uint256 hash, const string strHash,
788787
txiter it = mapTx.find(hash);
789788
if (it != mapTx.end()) {
790789
mapTx.modify(it, update_fee_delta(deltas.second));
790+
// Now update all ancestors' modified fees with descendants
791+
setEntries setAncestors;
792+
uint64_t nNoLimit = std::numeric_limits<uint64_t>::max();
793+
std::string dummy;
794+
CalculateMemPoolAncestors(*it, setAncestors, nNoLimit, nNoLimit, nNoLimit, nNoLimit, dummy, false);
795+
BOOST_FOREACH(txiter ancestorIt, setAncestors) {
796+
mapTx.modify(ancestorIt, update_descendant_state(0, nFeeDelta, 0));
797+
}
791798
}
792799
}
793800
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
956963
// "minimum reasonable fee rate" (ie some value under which we consider txn
957964
// to have 0 fee). This way, we don't allow txn to enter mempool with feerate
958965
// equal to txn which were removed with no block in between.
959-
CFeeRate removed(it->GetFeesWithDescendants(), it->GetSizeWithDescendants());
966+
CFeeRate removed(it->GetModFeesWithDescendants(), it->GetSizeWithDescendants());
960967
removed += minReasonableRelayFee;
961968
trackPackageRemoved(removed);
962969
maxFeeRateRemoved = std::max(maxFeeRateRemoved, removed);

‎src/txmempool.h‎

Lines changed: 22 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -44,12 +44,12 @@ class CTxMemPool;
4444
* ("descendant" transactions).
4545
*
4646
* When a new entry is added to the mempool, we update the descendant state
47-
* (nCountWithDescendants, nSizeWithDescendants, and nFeesWithDescendants) for
47+
* (nCountWithDescendants, nSizeWithDescendants, and nModFeesWithDescendants) for
4848
* all ancestors of the newly added transaction.
4949
*
5050
* If updating the descendant state is skipped, we can mark the entry as
51-
* "dirty", and set nSizeWithDescendants/nFeesWithDescendants to equal nTxSize/
52-
* nTxFee. (This can potentially happen during a reorg, where we limit the
51+
* "dirty", and set nSizeWithDescendants/nModFeesWithDescendants to equal nTxSize/
52+
* nFee+feeDelta. (This can potentially happen during a reorg, where we limit the
5353
* amount of work we're willing to do to avoid consuming too much CPU.)
5454
*
5555
*/
@@ -74,11 +74,11 @@ class CTxMemPoolEntry
7474
// Information about descendants of this transaction that are in the
7575
// mempool; if we remove this transaction we must remove all of these
7676
// descendants as well. if nCountWithDescendants is 0, treat this entry as
77-
// dirty, and nSizeWithDescendants and nFeesWithDescendants will not be
77+
// dirty, and nSizeWithDescendants and nModFeesWithDescendants will not be
7878
// correct.
7979
uint64_t nCountWithDescendants; //! number of descendant transactions
8080
uint64_t nSizeWithDescendants; //! ... and size
81-
CAmount nFeesWithDescendants; //! ... and total fees (all including us)
81+
CAmount nModFeesWithDescendants; //! ... and total fees (all including us)
8282

8383
public:
8484
CTxMemPoolEntry(const CTransaction& _tx, const CAmount& _nFee,
@@ -104,7 +104,8 @@ class CTxMemPoolEntry
104104

105105
// Adjusts the descendant state, if this entry is not dirty.
106106
void UpdateState(int64_t modifySize, CAmount modifyFee, int64_t modifyCount);
107-
// Updates the fee delta used for mining priority score
107+
// Updates the fee delta used for mining priority score, and the
108+
// modified fees with descendants.
108109
void UpdateFeeDelta(int64_t feeDelta);
109110

110111
/** We can set the entry to be dirty if doing the full calculation of in-
@@ -116,7 +117,7 @@ class CTxMemPoolEntry
116117

117118
uint64_t GetCountWithDescendants() const { return nCountWithDescendants; }
118119
uint64_t GetSizeWithDescendants() const { return nSizeWithDescendants; }
119-
CAmount GetFeesWithDescendants() const { return nFeesWithDescendants; }
120+
CAmount GetModFeesWithDescendants() const { return nModFeesWithDescendants; }
120121

121122
bool GetSpendsCoinbase() const { return spendsCoinbase; }
122123
};
@@ -163,39 +164,39 @@ struct mempoolentry_txid
163164
}
164165
};
165166

166-
/** \class CompareTxMemPoolEntryByFee
167+
/** \class CompareTxMemPoolEntryByDescendantScore
167168
*
168-
* Sort an entry by max(feerate of entry's tx, feerate with all descendants).
169+
* Sort an entry by max(score/size of entry's tx, score/size with all descendants).
169170
*/
170-
class CompareTxMemPoolEntryByFee
171+
class CompareTxMemPoolEntryByDescendantScore
171172
{
172173
public:
173174
bool operator()(const CTxMemPoolEntry& a, const CTxMemPoolEntry& b)
174175
{
175-
bool fUseADescendants = UseDescendantFeeRate(a);
176-
bool fUseBDescendants = UseDescendantFeeRate(b);
176+
bool fUseADescendants = UseDescendantScore(a);
177+
bool fUseBDescendants = UseDescendantScore(b);
177178

178-
double aFees = fUseADescendants ? a.GetFeesWithDescendants() : a.GetFee();
179+
double aModFee = fUseADescendants ? a.GetModFeesWithDescendants() : a.GetModifiedFee();
179180
double aSize = fUseADescendants ? a.GetSizeWithDescendants() : a.GetTxSize();
180181

181-
double bFees = fUseBDescendants ? b.GetFeesWithDescendants() : b.GetFee();
182+
double bModFee = fUseBDescendants ? b.GetModFeesWithDescendants() : b.GetModifiedFee();
182183
double bSize = fUseBDescendants ? b.GetSizeWithDescendants() : b.GetTxSize();
183184

184185
// Avoid division by rewriting (a/b > c/d) as (a*d > c*b).
185-
double f1 = aFees * bSize;
186-
double f2 = aSize * bFees;
186+
double f1 = aModFee * bSize;
187+
double f2 = aSize * bModFee;
187188

188189
if (f1 == f2) {
189190
return a.GetTime() >= b.GetTime();
190191
}
191192
return f1 < f2;
192193
}
193194

194-
// Calculate which feerate to use for an entry (avoiding division).
195-
bool UseDescendantFeeRate(const CTxMemPoolEntry &a)
195+
// Calculate which score to use for an entry (avoiding division).
196+
bool UseDescendantScore(const CTxMemPoolEntry &a)
196197
{
197-
double f1 = (double)a.GetFee() * a.GetSizeWithDescendants();
198-
double f2 = (double)a.GetFeesWithDescendants() * a.GetTxSize();
198+
double f1 = (double)a.GetModifiedFee() * a.GetSizeWithDescendants();
199+
double f2 = (double)a.GetModFeesWithDescendants() * a.GetTxSize();
199200
return f2 > f1;
200201
}
201202
};
@@ -350,7 +351,7 @@ class CTxMemPool
350351
// sorted by fee rate
351352
boost::multi_index::ordered_non_unique<
352353
boost::multi_index::identity<CTxMemPoolEntry>,
353-
CompareTxMemPoolEntryByFee
354+
CompareTxMemPoolEntryByDescendantScore
354355
>,
355356
// sorted by entry time
356357
boost::multi_index::ordered_non_unique<

0 commit comments

Comments
 (0)