From b2d04479647af64ad7cf5ebfb6175251b2f6b72e Mon Sep 17 00:00:00 2001
From: stickies-v <stickies-v@protonmail.com>
Date: Wed, 18 Oct 2023 11:38:45 +0100
Subject: [PATCH] bugfix: correct DisconnectedBlockTransactions memory usage

With `queuedTx` owning the `CTransactionRef` shared ptrs, they (and
the managed objects) are entirely allocated on the heap. In
`DisconnectedBlockTransactions::DynamicMemoryUsage`, we account for
the 2 pointers that make up the shared_ptr, but not for the managed
object (CTransaction) or the control block.

Prior to this commit, by calculating the `RecursiveDynamicUsage` on
a `CTransaction` whenever modifying `cachedInnerUsage`, we account
for the dynamic usage of the `CTransaction`, i.e. the `vins` and
`vouts` vectors, but we do not account for the `CTransaction`
object itself, nor for the `CTransactionRef` control block.

This means prior to this commit, `DynamicMemoryUsage` underestimates
dynamic memory usage by not including the `CTransaction` objects and
the shared ptr control blocks.

Fix this by calculating `RecursiveDynamicUsage` on the
`CTransactionRef` instead of the `CTransaction` whenever modifying
`cachedInnerUsage`.
---
 src/kernel/disconnected_transactions.cpp | 6 +++---
 src/kernel/disconnected_transactions.h   | 3 +--
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/src/kernel/disconnected_transactions.cpp b/src/kernel/disconnected_transactions.cpp
index 9c3f39135e1..f865fed688c 100644
--- a/src/kernel/disconnected_transactions.cpp
+++ b/src/kernel/disconnected_transactions.cpp
@@ -34,7 +34,7 @@ std::vector<CTransactionRef> DisconnectedBlockTransactions::LimitMemoryUsage()
 
     while (!queuedTx.empty() && DynamicMemoryUsage() > m_max_mem_usage) {
         evicted.emplace_back(queuedTx.front());
-        cachedInnerUsage -= RecursiveDynamicUsage(*queuedTx.front());
+        cachedInnerUsage -= RecursiveDynamicUsage(queuedTx.front());
         iters_by_txid.erase(queuedTx.front()->GetHash());
         queuedTx.pop_front();
     }
@@ -53,7 +53,7 @@ size_t DisconnectedBlockTransactions::DynamicMemoryUsage() const
         auto it = queuedTx.insert(queuedTx.end(), *block_it);
         auto [_, inserted] = iters_by_txid.emplace((*block_it)->GetHash(), it);
         assert(inserted); // callers may never pass multiple transactions with the same txid
-        cachedInnerUsage += RecursiveDynamicUsage(**block_it);
+        cachedInnerUsage += RecursiveDynamicUsage(*block_it);
     }
     return LimitMemoryUsage();
 }
@@ -69,7 +69,7 @@ void DisconnectedBlockTransactions::removeForBlock(const std::vector<CTransactio
         if (iter != iters_by_txid.end()) {
             auto list_iter = iter->second;
             iters_by_txid.erase(iter);
-            cachedInnerUsage -= RecursiveDynamicUsage(**list_iter);
+            cachedInnerUsage -= RecursiveDynamicUsage(*list_iter);
             queuedTx.erase(list_iter);
         }
     }
diff --git a/src/kernel/disconnected_transactions.h b/src/kernel/disconnected_transactions.h
index 998be9b3da1..401ec435e6b 100644
--- a/src/kernel/disconnected_transactions.h
+++ b/src/kernel/disconnected_transactions.h
@@ -36,8 +36,7 @@ static const unsigned int MAX_DISCONNECTED_TX_POOL_BYTES{20'000'000};
  */
 class DisconnectedBlockTransactions {
 private:
-    /** Cached dynamic memory usage for the CTransactions (memory for the shared pointers is
-     * included in the container calculations). */
+    /** Cached dynamic memory usage for the `CTransactionRef`s */
     uint64_t cachedInnerUsage = 0;
     const size_t m_max_mem_usage;
     std::list<CTransactionRef> queuedTx;