Merge bitcoin/bitcoin#25571: refactor: Make mapBlocksUnknownParent local, and rename it

dd065dae9fcebd6806ff67703ffa8128e80b97cc refactor: Make mapBlocksUnknownParent local, and rename it (Hennadii Stepanov)

Pull request description:

  This PR is a second attempt at #19594. This PR has two motivations:

  - Improve code hygiene by eliminating a global variable, `mapBlocksUnknownParent`
  - Fix fuzz test OOM when running too long ([see #19594 comment](https://github.com/bitcoin/bitcoin/pull/19594#issuecomment-958801638))

  A minor added advantage is to release `mapBlocksUnknownParent` memory when the reindexing phase is done. The current situation is somewhat similar to a memory leak because this map exists unused for the remaining lifetime of the process. It's true that this map should be empty of data elements after use, but its internal metadata (indexing structures, etc.) can have non-trivial size because there can be many thousands of simultaneous elements in this map.

  This PR helps our efforts to reduce the use of global variables. This variable isn't just global, it's hidden inside a function (it looks like a local variable but has the `static` attribute).

  This global variable exists because the `-reindex` processing code calls `LoadExternalBlockFile()` multiple times (once for each block file), but that function must preserve some state between calls (the `mapBlocksUnknownParent` map). This PR fixes this by allocating this map as a local variable in the caller's scope and passing it in on each call. When reindexing completes, the map goes out of scope and is deallocated.

  I tested this manually by reindexing on mainnet and signet. Also, the existing `feature_reindex.py` functional test passes.

ACKs for top commit:
  mzumsande:
    re-ACK dd065dae9fcebd6806ff67703ffa8128e80b97cc
  theStack:
    re-ACK dd065dae9fcebd6806ff67703ffa8128e80b97cc
  shaavan:
    reACK dd065dae9fcebd6806ff67703ffa8128e80b97cc

Tree-SHA512: 9cd20e44d2fa1096dd405bc107bc065ea8f904f5b3f63080341b08d8cf57b790df565f58815c2f331377d044d5306708b4bf6bdfc5ef8d0ed85d8e97d744732c

1 files changed, 5 insertions, 1 deletions

diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
index bac05f6be2..601d0bdf58 100644
--- a/src/node/blockstorage.cpp
+++ b/src/node/blockstorage.cpp
@@ -21,6 +21,7 @@
 #include <util/system.h>
 #include <validation.h>
 
+#include <map>
 #include <unordered_map>
 
 namespace node {
@@ -834,6 +835,9 @@ void ThreadImport(ChainstateManager& chainman, std::vector<fs::path> vImportFile
         // -reindex
         if (fReindex) {
             int nFile = 0;
+            // Map of disk positions for blocks with unknown parent (only used for reindex);
+            // parent hash -> child disk position, multiple children can have the same parent.
+            std::multimap<uint256, FlatFilePos> blocks_with_unknown_parent;
             while (true) {
                 FlatFilePos pos(nFile, 0);
                 if (!fs::exists(GetBlockPosFilename(pos))) {
@@ -844,7 +848,7 @@ void ThreadImport(ChainstateManager& chainman, std::vector<fs::path> vImportFile
                     break; // This error is logged in OpenBlockFile
                 }
                 LogPrintf("Reindexing block file blk%05u.dat...\n", (unsigned int)nFile);
-                chainman.ActiveChainstate().LoadExternalBlockFile(file, &pos);
+                chainman.ActiveChainstate().LoadExternalBlockFile(file, &pos, &blocks_with_unknown_parent);
                 if (ShutdownRequested()) {
                     LogPrintf("Shutdown requested. Exit %s\n", __func__);
                     return;