mempool: optimization of removeForBlock + general optimizations

Summary
---

As discussed in issue Bitcoin-ABC#161, the performance of `removeForBlock` in particular
is a critical path that deserves optimization.

This MR significantly improves the performance of `removeForBlock` from 0% to
as much as 100% or more, depending on the exact mempool layout in question.

The changes introduced also happen to improve the general performance of the
mempool as well.

Minor RPC Change
---

- As a result of these optimizations, the mempool-related RPC calls that
  describe a mempool entry's `spentby` (list of mempool txs that spend a
  particular mempool tx) have a different sort order now. Previously, this list
  would be sorted by txid. Now, it is sorted topologically.
- `getmempooldescendants` and `getmempoolancestors` now also no longer returns a
  list that is sorted by `TxId` but is rather topologically sorted (by
  `entryId`).

Test Plan
---

- `ninja all check-all`

Run these benches on master versus this MR commit:

- `ninja bench_bitcoin && src/bench/bench_bitcoin -filter='(RemoveForBlock.*)|(MempoolAcc.*)|(Reorg.*)|(Generat.*)|(Evict.*)'`

doc/release-notes.md

@@ -41,7 +41,15 @@ Bitcoin Cash Node version 23.0.1 is now available from:
 
 ## Low-level RPC changes
 
-...
+The `getmempoolancestors` and `getmempooldescendants` RPC methods now
+return a list of transactions that are sorted topologically (with parents
+coming before children). Previously they were sorted by transaction id.
+
+Mempool entries from the verbose versions of: `getrawmempool`, `getmempoolentry`,
+`getmempoolancestors`, and `getmempooldescendants` which contain a `spentby`
+key now have the transactions in the `spentby` list sorted topologically (with
+parents coming before children). Previously this list was sorted by transaction
+id.
 
 ## User interface changes
 

src/memusage.h

@@ -82,6 +82,11 @@ inline constexpr size_t IncrementalDynamicUsage(const std::set<X, Y> &) {
     return MallocUsage(sizeof(stl_tree_node<X>));
 }
 
+template <typename X, typename Y>
+inline constexpr size_t IncrementalDynamicUsage(const std::set<X, Y> *) {
+    return MallocUsage(sizeof(stl_tree_node<X>));
+}
+
 template <typename X, typename Y, typename Z>
 inline size_t DynamicUsage(const std::map<X, Y, Z> &m) {
     return MallocUsage(sizeof(stl_tree_node<std::pair<const X, Y>>)) * m.size();

src/txmempool.cpp

@@ -6,6 +6,7 @@
 #include <txmempool.h>
 
 #include <algorithm/contains.h>
+#include <algorithm/erase_if.h>
 #include <chain.h>
 #include <chainparams.h> // for GetConsensus.
 #include <clientversion.h>
@@ -333,9 +334,11 @@ void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason) {
 
     totalTxSize -= it->GetTxSize();
     cachedInnerUsage -= it->DynamicMemoryUsage();
-    cachedInnerUsage -= memusage::DynamicUsage(mapLinks[it].parents) +
-                        memusage::DynamicUsage(mapLinks[it].children);
-    mapLinks.erase(it);
+    if (const auto linksiter = mapLinks.find(it); linksiter != mapLinks.end()) {
+        cachedInnerUsage -= memusage::DynamicUsage(linksiter->second.parents) +
+                            memusage::DynamicUsage(linksiter->second.children);
+        mapLinks.erase(linksiter);
+    }
     mapTx.erase(it);
     nTransactionsUpdated++;
 }
@@ -419,22 +422,33 @@ void CTxMemPool::removeConflicts(const CTransaction &tx) {
  * fee estimator.
  */
 void CTxMemPool::removeForBlock(const std::vector<CTransactionRef> &vtx) {
+    LOCK(cs);
+
+    if (mapTx.empty() && mapDeltas.empty()) {
+        // fast-path for IBD and/or when mempool is empty; there is no need to
+        // do any of the set-up work below which eats precious cycles.
+        return;
+    }
+
     DisconnectedBlockTransactions disconnectpool;
     disconnectpool.addForBlock(vtx);
 
-    LOCK(cs);
-
     // iterate in topological order (parents before children)
     for (const CTransactionRef &tx : reverse_iterate(disconnectpool.GetQueuedTx().get<insertion_order>())) {
         const txiter it = mapTx.find(tx->GetId());
         if (it != mapTx.end()) {
             setEntries stage;
             stage.insert(it);
             RemoveStaged(stage, MemPoolRemovalReason::BLOCK);
+        } else {
+            removeConflicts(*tx);
         }
-        removeConflicts(*tx);
-        ClearPrioritisation(tx->GetId());
     }
+    // clear prioritisations (mapDeltas); optmized for the common case where
+    // mapDeltas is empty or much smaller than block.vtx
+    algo::erase_if(mapDeltas, [&disconnectpool](const auto &kv) {
+        return algo::contains(disconnectpool.GetQueuedTx(), kv.first);
+    });
 
     lastRollingFeeUpdate = GetTime();
     blockSinceLastRollingFeeBump = true;
@@ -829,21 +843,23 @@ void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry) {
     return addUnchecked(entry, setAncestors);
 }
 
+// NB: The pointer type is only used for template overload selection and never dereferenced so this is safe.
+inline constexpr size_t setEntriesIncrementalUsage =
+        memusage::IncrementalDynamicUsage(static_cast<CTxMemPool::setEntries *>(nullptr));
+
 void CTxMemPool::UpdateChild(txiter entry, txiter child, bool add) {
-    setEntries s;
     if (add && mapLinks[entry].children.insert(child).second) {
-        cachedInnerUsage += memusage::IncrementalDynamicUsage(s);
+        cachedInnerUsage += setEntriesIncrementalUsage;
     } else if (!add && mapLinks[entry].children.erase(child)) {
-        cachedInnerUsage -= memusage::IncrementalDynamicUsage(s);
+        cachedInnerUsage -= setEntriesIncrementalUsage;
     }
 }
 
 void CTxMemPool::UpdateParent(txiter entry, txiter parent, bool add) {
-    setEntries s;
     if (add && mapLinks[entry].parents.insert(parent).second) {
-        cachedInnerUsage += memusage::IncrementalDynamicUsage(s);
+        cachedInnerUsage += setEntriesIncrementalUsage;
     } else if (!add && mapLinks[entry].parents.erase(parent)) {
-        cachedInnerUsage -= memusage::IncrementalDynamicUsage(s);
+        cachedInnerUsage -= setEntriesIncrementalUsage;
     }
 }
 
@@ -1226,8 +1242,7 @@ void CTxMemPool::SetIsLoaded(bool loaded) {
 /** Maximum bytes for transactions to store for processing during reorg */
 static const size_t MAX_DISCONNECTED_TX_POOL_SIZE = 20 * DEFAULT_EXCESSIVE_BLOCK_SIZE;
 
-void DisconnectedBlockTransactions::addForBlock(
-    const std::vector<CTransactionRef> &vtx) {
+void DisconnectedBlockTransactions::addForBlock(const std::vector<CTransactionRef> &vtx) {
     for (const auto &tx : reverse_iterate(vtx)) {
         // If we already added it, just skip.
         auto it = queuedTx.find(tx->GetId());
@@ -1239,7 +1254,7 @@ void DisconnectedBlockTransactions::addForBlock(
         addTransaction(tx);
 
         // Fill in the set of parents.
-        std::unordered_set<TxId, SaltedTxIdHasher> parents;
+        std::set<TxId> parents;
         for (const CTxIn &in : tx->vin) {
             parents.insert(in.prevout.GetTxId());
         }
@@ -1248,8 +1263,7 @@ void DisconnectedBlockTransactions::addForBlock(
         // if we already know of the parent of the current transaction. If so,
         // we remove them from the set and then add them back.
         while (parents.size() > 0) {
-            std::unordered_set<TxId, SaltedTxIdHasher> worklist(
-                std::move(parents));
+            std::set<TxId> worklist(std::move(parents));
             parents.clear();
 
             for (const TxId &txid : worklist) {

src/txmempool.h

@@ -132,7 +132,9 @@ class CTxMemPoolEntry {
                     bool spendsCoinbase, int64_t _sigOpCount, LockPoints lp);
 
     uint64_t GetEntryId() const { return entryId; }
-    //! This should only be set by addUnchecked() before entry insertion into mempool
+    //! This should only be set exactly once by addUnchecked() before entry insertion into the mempool.
+    //! In other words, it may not be mutated for an instance whose storage is in CTxMemPool::mapTx, otherwise mempool
+    //! invariants will be violated.
     void SetEntryId(uint64_t eid) { entryId = eid; }
 
     const CTransaction &GetTx() const { return *this->tx; }
@@ -436,12 +438,12 @@ class CTxMemPool {
     //! All tx hashes/entries in mapTx, in random order
     std::vector<std::pair<TxHash, txiter>> vTxHashes;
 
-    struct CompareIteratorById {
+    struct CompareIteratorByEntryId {
         bool operator()(const txiter &a, const txiter &b) const {
-            return a->GetTx().GetId() < b->GetTx().GetId();
+            return a->GetEntryId() < b->GetEntryId();
         }
     };
-    typedef std::set<txiter, CompareIteratorById> setEntries;
+    using setEntries = std::set<txiter, CompareIteratorByEntryId>;
 
     const setEntries &GetMemPoolParents(txiter entry) const
         EXCLUSIVE_LOCKS_REQUIRED(cs);
@@ -538,7 +540,7 @@ class CTxMemPool {
         setEntries children;
     };
 
-    typedef std::map<txiter, TxLinks, CompareIteratorById> txlinksMap;
+    using txlinksMap = std::map<txiter, TxLinks, CompareIteratorByEntryId>;
     txlinksMap mapLinks;
 
     void UpdateParent(txiter entry, txiter parent, bool add);