Remove mempool logic designed to maintain ancestor/descendant state
What changed, and why it matters
This commit removes old bookkeeping code in Bitcoin Core's transaction memory pool (mempool) that tracked the cumulative size, fees, and counts of related transactions (ancestors and descendants). The project is moving this responsibility to a newer component called txgraph. The removed functions are left as empty stubs, meaning they no longer do anything. On its own, this is a cleanup/refactoring change, not a direct security fix, but it alters internal accounting that other parts of the mempool rely on. Without seeing the txgraph replacement and the rest of the surrounding work, it is hard to tell whether any edge cases in fee/size accounting could be affected.
Treat this as a refactoring/cleanup commit that requires review in context with the txgraph implementation and related commits. Do not flag as a standalone security vulnerability. Verify that the new txgraph correctly reproduces all ancestor/descendant aggregate accounting, especially during reorgs, prioritisation, and cluster-limit eviction, and that switching from removeRecursive to removeUnchecked does not leave orphaned descendants or inconsistent state.
Security signals we found
Large removal of internal mempool accounting code without an accompanying visible replacement in the same commit
Removal of cached descendant traversal in UpdateTransactionsFromBlock, which previously maintained consistency during reorgs
Switch from removeRecursive to removeUnchecked for cluster-size-limit eviction, changing cleanup semantics
Ancestor/descendant aggregate fee/size/count updates are no longer performed by CTxMemPoolEntry
No mention of vulnerability, CVE, bug, or security impact in commit title/message
Evidence from the diff
The commit deletes or empties mempool ancestor/descendant state-maintenance logic: CTxMemPoolEntry::UpdateDescendantState and UpdateAncestorState become no-ops; CTxMemPool::UpdateForDescendants is removed; UpdateAncestorsOf and UpdateForRemoveFromMempool no longer propagate ancestor/descendant aggregate size/fee/count updates; UpdateTransactionsFromBlock no longer builds a descendant cache and now removes oversized-cluster transactions via removeUnchecked instead of removeRecursive. The change is framed as removing logic that is now handled by m_txgraph. The diff is large (-185 lines) but mechanical. There is no explicit bug fix, CVE, or security discussion in the commit message or diff.
Changed components
src/kernel/mempool_entry.hsrc/txmempool.cppsrc/txmempool.hCTxMemPool transaction accountingCTxMemPoolEntry state trackingmempool reorg handling (UpdateTransactionsFromBlock)mempool eviction/cluster size limitsInspect captured patch +6 / −185
diff --git a/src/kernel/mempool_entry.h b/src/kernel/mempool_entry.h
index 8be0a77e..e12047de 100644
--- a/src/kernel/mempool_entry.h
+++ b/src/kernel/mempool_entry.h
@@ -128,10 +128,6 @@ public:
size_t DynamicMemoryUsage() const { return nUsageSize; }
const LockPoints& GetLockPoints() const { return lockPoints; }
- // Adjusts the descendant state.
- void UpdateDescendantState(int32_t modifySize, CAmount modifyFee, int64_t modifyCount);
- // Adjusts the ancestor state
- void UpdateAncestorState(int32_t modifySize, CAmount modifyFee, int64_t modifyCount, int64_t modifySigOps);
// Updates the modified fees with descendants/ancestors.
void UpdateModifiedFee(CAmount fee_diff)
{
diff --git a/src/txmempool.cpp b/src/txmempool.cpp
index 960296e2..233e3421 100644
--- a/src/txmempool.cpp
+++ b/src/txmempool.cpp
@@ -54,70 +54,16 @@ bool TestLockPointValidity(CChain& active_chain, const LockPoints& lp)
return true;
}
-void CTxMemPool::UpdateForDescendants(txiter updateIt, cacheMap& cachedDescendants,
- const std::set<Txid>& setExclude)
-{
- CTxMemPoolEntry::Children stageEntries, descendants;
- stageEntries = updateIt->GetMemPoolChildrenConst();
-
- while (!stageEntries.empty()) {
- const CTxMemPoolEntry& descendant = *stageEntries.begin();
- descendants.insert(descendant);
- stageEntries.erase(descendant);
- const CTxMemPoolEntry::Children& children = descendant.GetMemPoolChildrenConst();
- for (const CTxMemPoolEntry& childEntry : children) {
- cacheMap::iterator cacheIt = cachedDescendants.find(mapTx.iterator_to(childEntry));
- if (cacheIt != cachedDescendants.end()) {
- // We've already calculated this one, just add the entries for this set
- // but don't traverse again.
- for (txiter cacheEntry : cacheIt->second) {
- descendants.insert(*cacheEntry);
- }
- } else if (!descendants.count(childEntry)) {
- // Schedule for later processing
- stageEntries.insert(childEntry);
- }
- }
- }
- // descendants now contains all in-mempool descendants of updateIt.
- // Update and add to cached descendant map
- int32_t modifySize = 0;
- CAmount modifyFee = 0;
- int64_t modifyCount = 0;
- for (const CTxMemPoolEntry& descendant : descendants) {
- if (!setExclude.count(descendant.GetTx().GetHash())) {
- modifySize += descendant.GetTxSize();
- modifyFee += descendant.GetModifiedFee();
- modifyCount++;
- cachedDescendants[updateIt].insert(mapTx.iterator_to(descendant));
- // Update ancestor state for each descendant
- mapTx.modify(mapTx.iterator_to(descendant), [=](CTxMemPoolEntry& e) {
- e.UpdateAncestorState(updateIt->GetTxSize(), updateIt->GetModifiedFee(), 1, updateIt->GetSigOpCost());
- });
- }
- }
- mapTx.modify(updateIt, [=](CTxMemPoolEntry& e) { e.UpdateDescendantState(modifySize, modifyFee, modifyCount); });
-}
-
void CTxMemPool::UpdateTransactionsFromBlock(const std::vector<Txid>& vHashesToUpdate)
{
AssertLockHeld(cs);
- // For each entry in vHashesToUpdate, store the set of in-mempool, but not
- // in-vHashesToUpdate transactions, so that we don't have to recalculate
- // descendants when we come across a previously seen entry.
- cacheMap mapMemPoolDescendantsToUpdate;
// Use a set for lookups into vHashesToUpdate (these entries are already
// accounted for in the state of their ancestors)
std::set<Txid> setAlreadyIncluded(vHashesToUpdate.begin(), vHashesToUpdate.end());
- std::set<Txid> descendants_to_remove;
-
// Iterate in reverse, so that whenever we are looking at a transaction
// we are sure that all in-mempool descendants have already been processed.
- // This maximizes the benefit of the descendant cache and guarantees that
- // CTxMemPoolEntry::m_children will be updated, an assumption made in
- // UpdateForDescendants.
for (const Txid& hash : vHashesToUpdate | std::views::reverse) {
// calculate children from mapNextTx
txiter it = mapTx.find(hash);
@@ -143,22 +89,13 @@ void CTxMemPool::UpdateTransactionsFromBlock(const std::vector<Txid>& vHashesToU
// block and transactions that were in the mempool to txgraph.
m_txgraph->AddDependency(/*parent=*/*it, /*child=*/*childIter);
}
- } // release epoch guard for UpdateForDescendants
- UpdateForDescendants(it, mapMemPoolDescendantsToUpdate, setAlreadyIncluded);
+ }
}
auto txs_to_remove = m_txgraph->Trim(); // Enforce cluster size limits.
for (auto txptr : txs_to_remove) {
const CTxMemPoolEntry& entry = *(static_cast<const CTxMemPoolEntry*>(txptr));
- descendants_to_remove.insert(entry.GetTx().GetHash());
- }
-
- for (const auto& txid : descendants_to_remove) {
- // This txid may have been removed already in a prior call to removeRecursive.
- // Therefore we ensure it is not yet removed already.
- if (const std::optional<txiter> txiter = GetIter(txid)) {
- removeRecursive((*txiter)->GetTx(), MemPoolRemovalReason::SIZELIMIT);
- }
+ removeUnchecked(mapTx.iterator_to(entry), MemPoolRemovalReason::SIZELIMIT);
}
}
@@ -265,33 +202,13 @@ CTxMemPool::setEntries CTxMemPool::AssumeCalculateMemPoolAncestors(
return std::move(result).value_or(CTxMemPool::setEntries{});
}
-void CTxMemPool::UpdateAncestorsOf(bool add, txiter it, setEntries &setAncestors)
+void CTxMemPool::UpdateAncestorsOf(bool add, txiter it)
{
const CTxMemPoolEntry::Parents& parents = it->GetMemPoolParentsConst();
// add or remove this tx as a child of each parent
for (const CTxMemPoolEntry& parent : parents) {
UpdateChild(mapTx.iterator_to(parent), it, add);
}
- const int32_t updateCount = (add ? 1 : -1);
- const int32_t updateSize{updateCount * it->GetTxSize()};
- const CAmount updateFee = updateCount * it->GetModifiedFee();
- for (txiter ancestorIt : setAncestors) {
- mapTx.modify(ancestorIt, [=](CTxMemPoolEntry& e) { e.UpdateDescendantState(updateSize, updateFee, updateCount); });
- }
-}
-
-void CTxMemPool::UpdateEntryForAncestors(txiter it, const setEntries &setAncestors)
-{
- int64_t updateCount = setAncestors.size();
- int64_t updateSize = 0;
- CAmount updateFee = 0;
- int64_t updateSigOpsCost = 0;
- for (txiter ancestorIt : setAncestors) {
- updateSize += ancestorIt->GetTxSize();
- updateFee += ancestorIt->GetModifiedFee();
- updateSigOpsCost += ancestorIt->GetSigOpCost();
- }
- mapTx.modify(it, [=](CTxMemPoolEntry& e){ e.UpdateAncestorState(updateSize, updateFee, updateCount, updateSigOpsCost); });
}
void CTxMemPool::UpdateChildrenForRemoval(txiter it)
@@ -304,52 +221,10 @@ void CTxMemPool::UpdateChildrenForRemoval(txiter it)
void CTxMemPool::UpdateForRemoveFromMempool(const setEntries &entriesToRemove, bool updateDescendants)
{
- // For each entry, walk back all ancestors and decrement size associated with this
- // transaction
- if (updateDescendants) {
- // updateDescendants should be true whenever we're not recursively
- // removing a tx and all its descendants, eg when a transaction is
- // confirmed in a block.
- // Here we only update statistics and not data in CTxMemPool::Parents
- // and CTxMemPoolEntry::Children (which we need to preserve until we're
- // finished with all operations that need to traverse the mempool).
- for (txiter removeIt : entriesToRemove) {
- setEntries setDescendants;
- CalculateDescendants(removeIt, setDescendants);
- setDescendants.erase(removeIt); // don't update state for self
- int32_t modifySize = -removeIt->GetTxSize();
- CAmount modifyFee = -removeIt->GetModifiedFee();
- int modifySigOps = -removeIt->GetSigOpCost();
- for (txiter dit : setDescendants) {
- mapTx.modify(dit, [=](CTxMemPoolEntry& e){ e.UpdateAncestorState(modifySize, modifyFee, -1, modifySigOps); });
- }
- }
- }
for (txiter removeIt : entriesToRemove) {
- const CTxMemPoolEntry &entry = *removeIt;
- // Since this is a tx that is already in the mempool, we can call CMPA
- // with fSearchForParents = false. If the mempool is in a consistent
- // state, then using true or false should both be correct, though false
- // should be a bit faster.
- // However, if we happen to be in the middle of processing a reorg, then
- // the mempool can be in an inconsistent state. In this case, the set
- // of ancestors reachable via GetMemPoolParents()/GetMemPoolChildren()
- // will be the same as the set of ancestors whose packages include this
- // transaction, because when we add a new transaction to the mempool in
- // addNewTransaction(), we assume it has no children, and in the case of a
- // reorg where that assumption is false, the in-mempool children aren't
- // linked to the in-block tx's until UpdateTransactionsFromBlock() is
- // called.
- // So if we're being called during a reorg, ie before
- // UpdateTransactionsFromBlock() has been called, then
- // GetMemPoolParents()/GetMemPoolChildren() will differ from the set of
- // mempool parents we'd calculate by searching, and it's important that
- // we use the cached notion of ancestor transactions as the set of
- // things to update for removal.
- auto ancestors{AssumeCalculateMemPoolAncestors(__func__, entry, Limits::NoLimits(), /*fSearchForParents=*/false)};
// Note that UpdateAncestorsOf severs the child links that point to
// removeIt in the entries for the parents of removeIt.
- UpdateAncestorsOf(false, removeIt, ancestors);
+ UpdateAncestorsOf(false, removeIt);
}
// After updating all the ancestor sizes, we can now sever the link between each
// transaction being removed and any mempool children (ie, update CTxMemPoolEntry::m_parents
@@ -359,15 +234,6 @@ void CTxMemPool::UpdateForRemoveFromMempool(const setEntries &entriesToRemove, b
}
}
-void CTxMemPoolEntry::UpdateDescendantState(int32_t modifySize, CAmount modifyFee, int64_t modifyCount)
-{
-}
-
-void CTxMemPoolEntry::UpdateAncestorState(int32_t modifySize, CAmount modifyFee, int64_t modifyCount, int64_t modifySigOps)
-{
-}
-
-//! Clamp option values and populate the error if options are not valid.
static CTxMemPool::Options&& Flatten(CTxMemPool::Options&& opts, bilingual_str& error)
{
opts.check_ratio = std::clamp<int>(opts.check_ratio, 0, 1'000'000);
@@ -470,8 +336,7 @@ void CTxMemPool::addNewTransaction(CTxMemPool::txiter newit, CTxMemPool::setEntr
for (const auto& pit : GetIterSet(setParentTransactions)) {
UpdateParent(newit, pit, true);
}
- UpdateAncestorsOf(true, newit, setAncestors);
- UpdateEntryForAncestors(newit, setAncestors);
+ UpdateAncestorsOf(true, newit);
nTransactionsUpdated++;
totalTxSize += entry.GetTxSize();
@@ -823,18 +688,6 @@ void CTxMemPool::PrioritiseTransaction(const Txid& hash, const CAmount& nFeeDelt
// transaction fee to be current modified fee + feedelta.
mapTx.modify(it, [&nFeeDelta](CTxMemPoolEntry& e) { e.UpdateModifiedFee(nFeeDelta); });
m_txgraph->SetTransactionFee(*it, it->GetModifiedFee());
- // Now update all ancestors' modified fees with descendants
- auto ancestors{AssumeCalculateMemPoolAncestors(__func__, *it, Limits::NoLimits(), /*fSearchForParents=*/false)};
- for (txiter ancestorIt : ancestors) {
- mapTx.modify(ancestorIt, [=](CTxMemPoolEntry& e){ e.UpdateDescendantState(0, nFeeDelta, 0);});
- }
- // Now update all descendants' modified fees with ancestors
- setEntries setDescendants;
- CalculateDescendants(it, setDescendants);
- setDescendants.erase(it);
- for (txiter descendantIt : setDescendants) {
- mapTx.modify(descendantIt, [=](CTxMemPoolEntry& e){ e.UpdateAncestorState(0, nFeeDelta, 0, 0); });
- }
++nTransactionsUpdated;
}
if (delta == 0) {
diff --git a/src/txmempool.h b/src/txmempool.h
index 9a4389c3..b5da8c4c 100644
--- a/src/txmempool.h
+++ b/src/txmempool.h
@@ -350,7 +350,6 @@ public:
*/
void check(const CCoinsViewCache& active_coins_tip, int64_t spendheight) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
-
void removeRecursive(const CTransaction& tx, MemPoolRemovalReason reason) EXCLUSIVE_LOCKS_REQUIRED(cs);
/** After reorg, filter the entries that would no longer be valid in the next block, and update
* the entries' cached LockPoints if needed. The mempool does not have any knowledge of
@@ -639,35 +638,8 @@ private:
*/
void RemoveStaged(setEntries& stage, bool updateDescendants, MemPoolRemovalReason reason) EXCLUSIVE_LOCKS_REQUIRED(cs);
- /** UpdateForDescendants is used by UpdateTransactionsFromBlock to update
- * the descendants for a single transaction that has been added to the
- * mempool but may have child transactions in the mempool, eg during a
- * chain reorg.
- *
- * @pre CTxMemPoolEntry::m_children is correct for the given tx and all
- * descendants.
- * @pre cachedDescendants is an accurate cache where each entry has all
- * descendants of the corresponding key, including those that should
- * be removed for violation of ancestor limits.
- * @post if updateIt has any non-excluded descendants, cachedDescendants has
- * a new cache line for updateIt.
- *
- * @param[in] updateIt the entry to update for its descendants
- * @param[in,out] cachedDescendants a cache where each line corresponds to all
- * descendants. It will be updated with the descendants of the transaction
- * being updated, so that future invocations don't need to walk the same
- * transaction again, if encountered in another transaction chain.
- * @param[in] setExclude the set of descendant transactions in the mempool
- * that must not be accounted for (because any descendants in setExclude
- * were added to the mempool after the transaction being updated and hence
- * their state is already reflected in the parent state).
- */
- void UpdateForDescendants(txiter updateIt, cacheMap& cachedDescendants,
- const std::set<Txid>& setExclude) EXCLUSIVE_LOCKS_REQUIRED(cs);
/** Update ancestors of hash to add/remove it as a descendant transaction. */
- void UpdateAncestorsOf(bool add, txiter hash, setEntries &setAncestors) EXCLUSIVE_LOCKS_REQUIRED(cs);
- /** Set ancestor state for an entry */
- void UpdateEntryForAncestors(txiter it, const setEntries &setAncestors) EXCLUSIVE_LOCKS_REQUIRED(cs);
+ void UpdateAncestorsOf(bool add, txiter hash) EXCLUSIVE_LOCKS_REQUIRED(cs);
/** For each transaction being removed, update ancestors and any direct children.
* If updateDescendants is true, then also update in-mempool descendants'
* ancestor state. */
Why this scored 25/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.