Unbounded reorg memory usage #9027

#9014 brought to light in issue introduced in #7946 - when we do a reorg, we now keep the full contents of each block we connect in memory until the reorg is complete. Luckily, #9014 makes this a bit eaiser to fix - dont keep more than a few blocks around in shared_ptrs, just re-read them from disk after the reorg is complete when you need to call signalers. This may be complicated due to pruning, but it should be doable.

Note that this issue has never made it into a released version, so to avoid regression we should fix this before 0.14.

Defer this to 0.15 and remove 0.14 milestone according to current irc log.

This was partially addressed in #9208. However, the issue of keeping around all blocks connected in the ConnectTrace still exists. Note that, of course, this does not present an issue during normal connect since a ConnectTrace should only ever contain one block (as ABCS will return if it made progress).

#10286 will present yet another issue here. Because wallet callbacks will not force the ABC thread to pause, callbacks could pile up and even if blocks get freed from the ConnectTrace, they could still sit around in memory waiting for the callback to finish. Luckily since wallet also takes cs_main there is some ability to block the main thread from making progress, assuming locks will switch between threads and not always get taken on the same thread if there is contention, but hopefully eventually wallet can lose much of its cs_main-taking, which means wallet needs a way to pause validation if it is super, super huge and gets far behind.

Should likely be pushed to 16, as 10708 is not making 15.

Testing with bitcoin-cli invalidateblock 00000000000000000003618a9a50c352c58fa3a7ae6d0016d2720a9aff2c2db2, a 50k block re-org, shows that bitcoind memory usage peaks at ~ 10GB during this process on my system, although it cannot cause OOM:

$ grep ^VmPeak /proc/$(pidof bitcoind)/status

VmPeak: 10721532 kB

So it seems like this may still be somewhat present, despite previous comments indicating that it had gone away, and this comment indicating that #15402 had fixed it somewhat more concretely.

Perhaps we should close this one and keep #14289 open, although it would be nice to address the penultimate comment here first?

Edit: testing a day's-worth of blocks re-org (6 * 24) used 729MB

Testing

Did you test with or without a wallet?

Testing

Did you test with or without a wallet?

I tested this without a wallet, which IIUC should give me the "maximum" unbounded memory usage in this scenario?

I accidentally -reindex-ed my mainnet dev node earlier this morning as I was adding coinstatsindex to it. When that sorts itself out, I can re-test with a wallet loaded.

The invalidateblock RPC exercises a different code path. A simple way to reproduce is to force a development mainnet node to reorg a certain number of blocks at startup. For instance i was able to test this on top of aa76bd266df with the following clankerdiff:

<details>

<summary>Expand clankerdiff</summary>

diff --git a/src/validation.cpp b/src/validation.cpp
index 9d1e1e3563..6cb3c2b0df 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3190,6 +3190,8 @@ void Chainstate::PruneBlockIndexCandidates() {
     assert(!setBlockIndexCandidates.empty());
 }
 
+static bool g_reorg_done{false};
+
 /**
  * Try to make some progress towards making index_most_work the active block.
  * pblock is either nullptr or a pointer to a CBlock corresponding to index_most_work.
@@ -3203,6 +3205,10 @@ bool Chainstate::ActivateBestChainStep(BlockValidationState& state, CBlockIndex&
 
     const CBlockIndex* pindexOldTip = m_chain.Tip();
     const CBlockIndex* pindexFork = m_chain.FindFork(index_most_work);
+    if (const char* h{std::getenv("REORG_FROM")}; h && !g_reorg_done) {
+        g_reorg_done = true;
+        pindexFork = Assert(m_blockman.LookupBlockIndex(*Assert(uint256::FromHex(h))))->pprev;
+    }
 
     // Disconnect active blocks which are no longer in the best chain.
     bool fBlocksDisconnected = false;
@@ -3381,7 +3387,7 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr<
                 }
 
                 // Whether we have anything to do at all.
-                if (pindexMostWork == nullptr || pindexMostWork == m_chain.Tip()) {
+                if (pindexMostWork == nullptr || (pindexMostWork == m_chain.Tip() && (g_reorg_done || !std::getenv("REORG_FROM")))) {
                     break;
                 }
 

</details>

I used it to disconnect 3449 mainnet blocks and observed memory usage increasing by about 14GiB until the fork point, and continuing during re-connection (i stopped it before reconnection was complete as my machine was about to OOM).

At a few MiB of additional memory usage per disconnected block, this is mostly a theoretical issue, as if an attacker can bring that to any meaningful level of memory usage, we have much bigger problems anyways.