diff options
| -rw-r--r-- | src/txmempool.cpp | 32 | ||||
| -rw-r--r-- | src/txmempool.h | 68 | ||||
| -rw-r--r-- | src/validation.cpp | 4 | ||||
| -rwxr-xr-x | test/functional/mempool_updatefromblock.py | 2 |
4 files changed, 73 insertions, 33 deletions
diff --git a/src/txmempool.cpp b/src/txmempool.cpp index dc2769b81e..fb5652d0a0 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -116,10 +116,9 @@ size_t CTxMemPoolEntry::GetTxSize() const return GetVirtualTransactionSize(nTxWeight, sigOpCost); } -// 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<uint256> &setExclude) +void CTxMemPool::UpdateForDescendants(txiter updateIt, cacheMap& cachedDescendants, + const std::set<uint256>& setExclude, std::set<uint256>& descendants_to_remove, + uint64_t ancestor_size_limit, uint64_t ancestor_count_limit) { CTxMemPoolEntry::Children stageEntries, descendants; stageEntries = updateIt->GetMemPoolChildrenConst(); @@ -156,17 +155,18 @@ void CTxMemPool::UpdateForDescendants(txiter updateIt, cacheMap &cachedDescendan cachedDescendants[updateIt].insert(mapTx.iterator_to(descendant)); // Update ancestor state for each descendant mapTx.modify(mapTx.iterator_to(descendant), update_ancestor_state(updateIt->GetTxSize(), updateIt->GetModifiedFee(), 1, updateIt->GetSigOpCost())); + // 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().GetHash()); + } } } mapTx.modify(updateIt, update_descendant_state(modifySize, modifyFee, modifyCount)); } -// vHashesToUpdate 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 vHashesToUpdate, 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<uint256> &vHashesToUpdate) +void CTxMemPool::UpdateTransactionsFromBlock(const std::vector<uint256> &vHashesToUpdate, uint64_t ancestor_size_limit, uint64_t ancestor_count_limit) { AssertLockHeld(cs); // For each entry in vHashesToUpdate, store the set of in-mempool, but not @@ -178,6 +178,8 @@ void CTxMemPool::UpdateTransactionsFromBlock(const std::vector<uint256> &vHashes // accounted for in the state of their ancestors) std::set<uint256> setAlreadyIncluded(vHashesToUpdate.begin(), vHashesToUpdate.end()); + std::set<uint256> 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 @@ -207,7 +209,15 @@ void CTxMemPool::UpdateTransactionsFromBlock(const std::vector<uint256> &vHashes } } } // release epoch guard for UpdateForDescendants - UpdateForDescendants(it, mapMemPoolDescendantsToUpdate, setAlreadyIncluded); + UpdateForDescendants(it, mapMemPoolDescendantsToUpdate, 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> txiter = GetIter(txid)) { + removeRecursive((*txiter)->GetTx(), MemPoolRemovalReason::SIZELIMIT); + } } } diff --git a/src/txmempool.h b/src/txmempool.h index e025dafd91..e7e5a3c402 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -636,16 +636,25 @@ public: */ void RemoveStaged(setEntries& stage, bool updateDescendants, 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 vHashesToUpdate (excluding any - * child transactions present in vHashesToUpdate, which are already accounted - * for). Note: vHashesToUpdate 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 + * vHashesToUpdate (excluding any child transactions present in + * vHashesToUpdate, 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] vHashesToUpdate 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<uint256>& vHashesToUpdate) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main) LOCKS_EXCLUDED(m_epoch); + void UpdateTransactionsFromBlock(const std::vector<uint256>& vHashesToUpdate, + uint64_t ancestor_size_limit, uint64_t ancestor_count_limit) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main) LOCKS_EXCLUDED(m_epoch); /** Try to calculate all in-mempool ancestors of entry. * (these are all calculated including the tx itself) @@ -794,19 +803,38 @@ 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). + * 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<uint256> &setExclude) EXCLUSIVE_LOCKS_REQUIRED(cs); + void UpdateForDescendants(txiter updateIt, cacheMap& cachedDescendants, + const std::set<uint256>& setExclude, std::set<uint256>& 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. */ void UpdateAncestorsOf(bool add, txiter hash, setEntries &setAncestors) EXCLUSIVE_LOCKS_REQUIRED(cs); /** Set ancestor state for an entry */ diff --git a/src/validation.cpp b/src/validation.cpp index 72e0b6218e..1b1d01a4c2 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -347,7 +347,9 @@ void CChainState::MaybeUpdateMempoolForReorg( // 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. - m_mempool->UpdateTransactionsFromBlock(vHashUpdate); + 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; + m_mempool->UpdateTransactionsFromBlock(vHashUpdate, 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 coinbase output, mature. diff --git a/test/functional/mempool_updatefromblock.py b/test/functional/mempool_updatefromblock.py index 16c15e3f74..51de582ce0 100755 --- a/test/functional/mempool_updatefromblock.py +++ b/test/functional/mempool_updatefromblock.py @@ -17,7 +17,7 @@ from test_framework.util import assert_equal class MempoolUpdateFromBlockTest(BitcoinTestFramework): def set_test_params(self): self.num_nodes = 1 - self.extra_args = [['-limitdescendantsize=1000', '-limitancestorsize=1000']] + self.extra_args = [['-limitdescendantsize=1000', '-limitancestorsize=1000', '-limitancestorcount=100']] def skip_test_if_missing_module(self): self.skip_if_no_wallet() |