Remove CTxMemPool::GetSortedDepthAndScore
What changed, and why it matters
This commit refactors how Bitcoin Core's mempool sorts transactions for internal consistency checks and information reporting. It replaces an older sorting method based on 'ancestor count' with a newer topology-aware method that relies on the recently introduced mempool graph/linearization code. The change is a code cleanup and architectural simplification, not a fix for an active security bug. A new assertion is added to ensure the mempool graph is not oversized when this sorting is used for the consistency check.
No immediate action required. Treat as routine refactoring. Reviewers should verify that GetSortedScoreWithTopology() correctly preserves topological ordering in all edge cases (especially around graph oversize conditions) and that the removed ancestor-count monotonicity check is adequately covered by the new graph invariants.
Security signals we found
Removal of ancestor-count-based mempool sorting in favor of graph-based topological ordering
Addition of assert(!m_txgraph->IsOversized(...)) in CTxMemPool::check
Removal of prev_ancestor_count monotonicity sanity check
Refactoring only; no direct input validation, network, or cryptographic changes
Evidence from the diff
CTxMemPool::GetSortedDepthAndScore() is removed and replaced by GetSortedScoreWithTopology(), which sorts entries using m_txgraph->CompareMainOrder() instead of DepthAndScoreComparator (which sorted by GetCountWithAncestors() then by feerate). Callers in check(), entryAll(), and infoAll() are updated. The consistency check (CTxMemPool::check) now asserts !m_txgraph->IsOversized(TxGraph::Level::MAIN) before iterating, and removes the prev_ancestor_count monotonicity assertion. CompareTxMemPoolEntryByScore is also removed from the header as it is no longer used here.
Changed components
src/txmempool.cppsrc/txmempool.hCTxMemPool::checkCTxMemPool::entryAllCTxMemPool::infoAllInspect captured patch +14 / −50
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index fa7da083..326ff79f 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -714,11 +714,12 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei
uint64_t checkTotal = 0;
CAmount check_total_fee{0};
uint64_t innerUsage = 0;
- uint64_t prev_ancestor_count{0};
+
+ assert(!m_txgraph->IsOversized(TxGraph::Level::MAIN));
CCoinsViewCache mempoolDuplicate(const_cast<CCoinsViewCache*>(&active_coins_tip));
- for (const auto& it : GetSortedDepthAndScore()) {
+ for (const auto& it : GetSortedScoreWithTopology()) {
checkTotal += it->GetTxSize();
check_total_fee += it->GetFee();
innerUsage += it->DynamicMemoryUsage();
@@ -733,9 +734,10 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei
assert(tx2.vout.size() > txin.prevout.n && !tx2.vout[txin.prevout.n].IsNull());
setParentCheck.insert(*it2);
}
- // We are iterating through the mempool entries sorted in order by ancestor count.
- // All parents must have been checked before their children and their coins added to
- // the mempoolDuplicate coins cache.
+ // We are iterating through the mempool entries sorted
+ // topologically and by mining score. All parents must have been
+ // checked before their children and their coins added to the
+ // mempoolDuplicate coins cache.
assert(mempoolDuplicate.HaveCoin(txin.prevout));
// Check whether its inputs are marked in mapNextTx.
auto it3 = mapNextTx.find(txin.prevout);
@@ -765,9 +767,6 @@ void CTxMemPool::check(const CCoinsViewCache& active_coins_tip, int64_t spendhei
assert(it->GetSizeWithAncestors() == nSizeCheck);
assert(it->GetSigOpCostWithAncestors() == nSigOpCheck);
assert(it->GetModFeesWithAncestors() == nFeesCheck);
- // Sanity check: we are walking in ascending ancestor count order.
- assert(prev_ancestor_count <= it->GetCountWithAncestors());
- prev_ancestor_count = it->GetCountWithAncestors();
// Check children against mapNextTx
CTxMemPoolEntry::Children setChildrenCheck;
@@ -819,23 +818,7 @@ bool CTxMemPool::CompareMiningScoreWithTopology(const Wtxid& hasha, const Wtxid&
return m_txgraph->CompareMainOrder(*i.value(), *j.value()) < 0;
}
-namespace {
-class DepthAndScoreComparator
-{
-public:
- bool operator()(const CTxMemPool::indexed_transaction_set::const_iterator& a, const CTxMemPool::indexed_transaction_set::const_iterator& b)
- {
- uint64_t counta = a->GetCountWithAncestors();
- uint64_t countb = b->GetCountWithAncestors();
- if (counta == countb) {
- return CompareTxMemPoolEntryByScore()(*a, *b);
- }
- return counta < countb;
- }
-};
-} // namespace
-
-std::vector<CTxMemPool::indexed_transaction_set::const_iterator> CTxMemPool::GetSortedDepthAndScore() const
+std::vector<CTxMemPool::indexed_transaction_set::const_iterator> CTxMemPool::GetSortedScoreWithTopology() const
{
std::vector<indexed_transaction_set::const_iterator> iters;
AssertLockHeld(cs);
@@ -845,7 +828,9 @@ std::vector<CTxMemPool::indexed_transaction_set::const_iterator> CTxMemPool::Get
for (indexed_transaction_set::iterator mi = mapTx.begin(); mi != mapTx.end(); ++mi) {
iters.push_back(mi);
}
- std::sort(iters.begin(), iters.end(), DepthAndScoreComparator());
+ std::sort(iters.begin(), iters.end(), [this](const auto& a, const auto& b) EXCLUSIVE_LOCKS_REQUIRED(cs) noexcept {
+ return m_txgraph->CompareMainOrder(*a, *b) < 0;
+ });
return iters;
}
@@ -855,7 +840,7 @@ std::vector<CTxMemPoolEntryRef> CTxMemPool::entryAll() const
std::vector<CTxMemPoolEntryRef> ret;
ret.reserve(mapTx.size());
- for (const auto& it : GetSortedDepthAndScore()) {
+ for (const auto& it : GetSortedScoreWithTopology()) {
ret.emplace_back(*it);
}
return ret;
@@ -864,7 +849,7 @@ std::vector<CTxMemPoolEntryRef> CTxMemPool::entryAll() const
std::vector<TxMempoolInfo> CTxMemPool::infoAll() const
{
LOCK(cs);
- auto iters = GetSortedDepthAndScore();
+ auto iters = GetSortedScoreWithTopology();
std::vector<TxMempoolInfo> ret;
ret.reserve(mapTx.size());
diff --git a/src/txmempool.h b/src/txmempool.h
index 232538ae..fb2a4669 100644
--- a/src/txmempool.h
+++ b/src/txmempool.h
@@ -90,27 +90,6 @@ struct mempoolentry_wtxid
}
};
-/** \class CompareTxMemPoolEntryByScore
- *
- * Sort by feerate of entry (fee/size) in descending order
- * This is only used for transaction relay, so we use GetFee()
- * instead of GetModifiedFee() to avoid leaking prioritization
- * information via the sort order.
- */
-class CompareTxMemPoolEntryByScore
-{
-public:
- bool operator()(const CTxMemPoolEntry& a, const CTxMemPoolEntry& b) const
- {
- FeeFrac f1(a.GetFee(), a.GetTxSize());
- FeeFrac f2(b.GetFee(), b.GetTxSize());
- if (FeeRateCompare(f1, f2) == 0) {
- return b.GetTx().GetHash() < a.GetTx().GetHash();
- }
- return f1 > f2;
- }
-};
-
class CompareTxMemPoolEntryByEntryTime
{
public:
@@ -313,7 +292,7 @@ private:
void UpdateParent(txiter entry, txiter parent, bool add) EXCLUSIVE_LOCKS_REQUIRED(cs);
void UpdateChild(txiter entry, txiter child, bool add) EXCLUSIVE_LOCKS_REQUIRED(cs);
- std::vector<indexed_transaction_set::const_iterator> GetSortedDepthAndScore() const EXCLUSIVE_LOCKS_REQUIRED(cs);
+ std::vector<indexed_transaction_set::const_iterator> GetSortedScoreWithTopology() const EXCLUSIVE_LOCKS_REQUIRED(cs);
/**
* Track locally submitted transactions to periodically retry initial broadcast.
Why this scored 16/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.