diff --git a/src/txmempool.h b/src/txmempool.h --- a/src/txmempool.h +++ b/src/txmempool.h @@ -672,17 +672,26 @@ MemPoolRemovalReason reason) EXCLUSIVE_LOCKS_REQUIRED(cs); /** - * When adding transactions from a disconnected block back to the mempool, - * new mempool entries may have children in the mempool (which is generally - * not the case when otherwise adding transactions). - * UpdateTransactionsFromBlock() will find child transactions and update the - * descendant state for each transaction in txidsToUpdate (excluding any - * child transactions present in txidsToUpdate, which are already accounted - * for). - * Note: txidsToUpdate should be the set of transactions from the - * disconnected block that have been accepted back into the mempool. + * UpdateTransactionsFromBlock is called when adding transactions from a + * disconnected block back to the mempool, new mempool entries may have + * children in the mempool (which is generally not the case when otherwise + * adding transactions). + * @post updated descendant state for descendants of each transaction in + * txidsToUpdate (excluding any child transactions present in + * txidsToUpdate, which are already accounted for). Updated state + * includes add fee/size information for such descendants to the + * parent and updated ancestor state to include the parent. + * + * @param[in] txidsToUpdate The set of txids from the + * disconnected block that have been accepted back into the mempool. + * @param[in] ancestor_size_limit The maximum allowed size in virtual + * bytes of an entry and its ancestors + * @param[in] ancestor_count_limit The maximum allowed number of + * transactions including the entry and its ancestors. */ - void UpdateTransactionsFromBlock(const std::vector &txidsToUpdate) + void UpdateTransactionsFromBlock(const std::vector &txidsToUpdate, + uint64_t ancestor_size_limit, + uint64_t ancestor_count_limit) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main) LOCKS_EXCLUDED(m_epoch); /** @@ -853,20 +862,45 @@ private: /** - * 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. - * setExclude is 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). + * 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 CTxMemPool::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. + * @post descendants_to_remove has a new entry for any descendant which + * exceeded ancestor limits relative to updateIt. * - * cachedDescendants 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] 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). + * @param[out] descendants_to_remove Populated with the txids of entries + * that exceed ancestor limits. It's the responsibility of the caller + * to removeRecursive them. + * @param[in] ancestor_size_limit the max number of ancestral bytes allowed + * for any descendant + * @param[in] ancestor_count_limit the max number of ancestor transactions + * allowed for any descendant */ void UpdateForDescendants(txiter updateIt, cacheMap &cachedDescendants, - const std::set &setExclude) + const std::set &setExclude, + std::set &descendants_to_remove, + uint64_t ancestor_size_limit, + uint64_t ancestor_count_limit) EXCLUSIVE_LOCKS_REQUIRED(cs); /** * Update ancestors of hash to add/remove it as a descendant transaction. diff --git a/src/txmempool.cpp b/src/txmempool.cpp --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -122,12 +123,12 @@ lockPoints = lp; } -// Update the given tx for any in-mempool descendants. -// Assumes that CTxMemPool::m_children is correct for the given tx and all -// descendants. void CTxMemPool::UpdateForDescendants(txiter updateIt, cacheMap &cachedDescendants, - const std::set &setExclude) { + const std::set &setExclude, + std::set &descendants_to_remove, + uint64_t ancestor_size_limit, + uint64_t ancestor_count_limit) { CTxMemPoolEntry::Children stageEntries, descendants; stageEntries = updateIt->GetMemPoolChildrenConst(); @@ -170,6 +171,13 @@ update_ancestor_state(updateIt->GetTxSize(), updateIt->GetModifiedFee(), 1, updateIt->GetSigOpCount())); + // Don't directly remove the transaction here -- doing so would + // invalidate iterators in cachedDescendants. Mark it for removal + // by inserting into descendants_to_remove. + if (descendant.GetCountWithAncestors() > ancestor_count_limit || + descendant.GetSizeWithAncestors() > ancestor_size_limit) { + descendants_to_remove.insert(descendant.GetTx().GetId()); + } } } mapTx.modify(updateIt, @@ -177,13 +185,9 @@ modifySigOpCount)); } -// txidsToUpdate is the set of transaction hashes from a disconnected block -// which has been re-added to the mempool. For each entry, look for descendants -// that are outside txidsToUpdate, and add fee/size information for such -// descendants to the parent. For each such descendant, also update the ancestor -// state to include the parent. void CTxMemPool::UpdateTransactionsFromBlock( - const std::vector &txidsToUpdate) { + const std::vector &txidsToUpdate, uint64_t ancestor_size_limit, + uint64_t ancestor_count_limit) { AssertLockHeld(cs); // For each entry in txidsToUpdate, store the set of in-mempool, but not // in-txidsToUpdate transactions, so that we don't have to recalculate @@ -195,6 +199,8 @@ std::set setAlreadyIncluded(txidsToUpdate.begin(), txidsToUpdate.end()); + std::set 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 @@ -228,7 +234,17 @@ } } // release epoch guard for UpdateForDescendants UpdateForDescendants(it, mapMemPoolDescendantsToUpdate, - setAlreadyIncluded); + setAlreadyIncluded, descendants_to_remove, + ancestor_size_limit, ancestor_count_limit); + } + + 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 = GetIter(txid)) { + removeRecursive((*txiter)->GetTx(), + MemPoolRemovalReason::SIZELIMIT); + } } } @@ -1507,7 +1523,13 @@ // previously-confirmed transactions back to the mempool. // UpdateTransactionsFromBlock finds descendants of any transactions in the // disconnectpool that were added back and cleans up the mempool state. - pool.UpdateTransactionsFromBlock(txidsUpdate); + const uint64_t ancestor_count_limit = + gArgs.GetIntArg("-limitancestorcount", DEFAULT_ANCESTOR_LIMIT); + const uint64_t ancestor_size_limit = + gArgs.GetIntArg("-limitancestorsize", DEFAULT_ANCESTOR_SIZE_LIMIT) * + 1000; + pool.UpdateTransactionsFromBlock(txidsUpdate, ancestor_size_limit, + ancestor_count_limit); // Predicate to use for filtering transactions in removeForReorg. // Checks whether the transaction is still final and, if it spends a diff --git a/test/functional/mempool_updatefromblock.py b/test/functional/mempool_updatefromblock.py --- a/test/functional/mempool_updatefromblock.py +++ b/test/functional/mempool_updatefromblock.py @@ -18,7 +18,7 @@ def set_test_params(self): self.num_nodes = 1 self.extra_args = [ - ['-limitdescendantsize=5000', '-limitancestorsize=5000']] + ['-limitdescendantsize=5000', '-limitancestorsize=5000', '-limitancestorcount=200']] def skip_test_if_missing_module(self): self.skip_if_no_wallet()