From faf843c07f99f91603e08ea858f972516f1d669a Mon Sep 17 00:00:00 2001
From: MarcoFalke <falke.marco@gmail.com>
Date: Fri, 2 Apr 2021 20:38:28 +0200
Subject: [PATCH 1/6] refactor: Move load block thread into ChainstateManager

---
 src/init.cpp     | 6 ++----
 src/validation.h | 2 ++
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/init.cpp b/src/init.cpp
index 17b216573f..7e963e8190 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -155,8 +155,6 @@ static fs::path GetPidFile(const ArgsManager& args)
 
 static std::unique_ptr<ECCVerifyHandle> globalVerifyHandle;
 
-static std::thread g_load_block;
-
 void Interrupt(NodeContext& node)
 {
     InterruptHTTPServer();
@@ -220,7 +218,7 @@ void Shutdown(NodeContext& node)
     // After everything has been shut down, but before things get flushed, stop the
     // CScheduler/checkqueue, scheduler and load block thread.
     if (node.scheduler) node.scheduler->stop();
-    if (g_load_block.joinable()) g_load_block.join();
+    if (node.chainman && node.chainman->m_load_block.joinable()) node.chainman->m_load_block.join();
     StopScriptCheckWorkerThreads();
 
     // After the threads that potentially access these pointers have been stopped,
@@ -1880,7 +1878,7 @@ bool AppInitMain(const std::any& context, NodeContext& node, interfaces::BlockAn
         vImportFiles.push_back(strFile);
     }
 
-    g_load_block = std::thread(&TraceThread<std::function<void()>>, "loadblk", [=, &chainman, &args] {
+    chainman.m_load_block = std::thread(&TraceThread<std::function<void()>>, "loadblk", [=, &chainman, &args] {
         ThreadImport(chainman, vImportFiles, args);
     });
 
diff --git a/src/validation.h b/src/validation.h
index 21e63947fa..3f7e967ec7 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -35,6 +35,7 @@
 #include <set>
 #include <stdint.h>
 #include <string>
+#include <thread>
 #include <utility>
 #include <vector>
 
@@ -869,6 +870,7 @@ private:
     friend CChain& ChainActive();
 
 public:
+    std::thread m_load_block;
     //! A single BlockManager instance is shared across each constructed
     //! chainstate to avoid duplicating block metadata.
     BlockManager m_blockman GUARDED_BY(::cs_main);

From fa413f07a14744e7d7f7746e861aabd9cf938f61 Mon Sep 17 00:00:00 2001
From: MarcoFalke <falke.marco@gmail.com>
Date: Fri, 2 Apr 2021 19:17:00 +0200
Subject: [PATCH 2/6] move-only: Move ThreadImport to blockstorage

Can be reviewed with the git options
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
---
 src/Makefile.am           |  2 +
 src/init.cpp              | 89 +-----------------------------------
 src/node/blockstorage.cpp | 96 +++++++++++++++++++++++++++++++++++++++
 src/node/blockstorage.h   | 19 ++++++++
 4 files changed, 118 insertions(+), 88 deletions(-)
 create mode 100644 src/node/blockstorage.cpp
 create mode 100644 src/node/blockstorage.h

diff --git a/src/Makefile.am b/src/Makefile.am
index 4e09c86ebd..f0da4937e5 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -174,6 +174,7 @@ BITCOIN_CORE_H = \
   netaddress.h \
   netbase.h \
   netmessagemaker.h \
+  node/blockstorage.h \
   node/coin.h \
   node/coinstats.h \
   node/context.h \
@@ -324,6 +325,7 @@ libbitcoin_server_a_SOURCES = \
   miner.cpp \
   net.cpp \
   net_processing.cpp \
+  node/blockstorage.cpp \
   node/coin.cpp \
   node/coinstats.cpp \
   node/context.cpp \
diff --git a/src/init.cpp b/src/init.cpp
index 7e963e8190..c7e8699d6b 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -16,7 +16,6 @@
 #include <chain.h>
 #include <chainparams.h>
 #include <compat/sanity.h>
-#include <consensus/validation.h>
 #include <fs.h>
 #include <hash.h>
 #include <httprpc.h>
@@ -32,6 +31,7 @@
 #include <net_permissions.h>
 #include <net_processing.h>
 #include <netbase.h>
+#include <node/blockstorage.h>
 #include <node/context.h>
 #include <node/ui_interface.h>
 #include <policy/feerate.h>
@@ -61,7 +61,6 @@
 #include <util/threadnames.h>
 #include <util/translation.h>
 #include <validation.h>
-
 #include <validationinterface.h>
 #include <walletinitinterface.h>
 
@@ -90,7 +89,6 @@
 
 static const bool DEFAULT_PROXYRANDOMIZE = true;
 static const bool DEFAULT_REST_ENABLE = false;
-static const bool DEFAULT_STOPAFTERBLOCKIMPORT = false;
 
 #ifdef WIN32
 // Win32 LevelDB doesn't use filedescriptors, and the ones used for
@@ -625,20 +623,6 @@ static void BlockNotifyGenesisWait(const CBlockIndex* pBlockIndex)
     }
 }
 
-struct CImportingNow
-{
-    CImportingNow() {
-        assert(fImporting == false);
-        fImporting = true;
-    }
-
-    ~CImportingNow() {
-        assert(fImporting == true);
-        fImporting = false;
-    }
-};
-
-
 // If we're using -prune with -reindex, then delete block files that will be ignored by the
 // reindex.  Since reindexing works by starting at block file 0 and looping until a blockfile
 // is missing, do the same here to delete any later block files after a gap.  Also delete all
@@ -691,77 +675,6 @@ static void StartupNotify(const ArgsManager& args)
 }
 #endif
 
-static void ThreadImport(ChainstateManager& chainman, std::vector<fs::path> vImportFiles, const ArgsManager& args)
-{
-    const CChainParams& chainparams = Params();
-    ScheduleBatchPriority();
-
-    {
-    CImportingNow imp;
-
-    // -reindex
-    if (fReindex) {
-        int nFile = 0;
-        while (true) {
-            FlatFilePos pos(nFile, 0);
-            if (!fs::exists(GetBlockPosFilename(pos)))
-                break; // No block files left to reindex
-            FILE *file = OpenBlockFile(pos, true);
-            if (!file)
-                break; // This error is logged in OpenBlockFile
-            LogPrintf("Reindexing block file blk%05u.dat...\n", (unsigned int)nFile);
-            ::ChainstateActive().LoadExternalBlockFile(chainparams, file, &pos);
-            if (ShutdownRequested()) {
-                LogPrintf("Shutdown requested. Exit %s\n", __func__);
-                return;
-            }
-            nFile++;
-        }
-        pblocktree->WriteReindexing(false);
-        fReindex = false;
-        LogPrintf("Reindexing finished\n");
-        // To avoid ending up in a situation without genesis block, re-try initializing (no-op if reindexing worked):
-        ::ChainstateActive().LoadGenesisBlock(chainparams);
-    }
-
-    // -loadblock=
-    for (const fs::path& path : vImportFiles) {
-        FILE *file = fsbridge::fopen(path, "rb");
-        if (file) {
-            LogPrintf("Importing blocks file %s...\n", path.string());
-            ::ChainstateActive().LoadExternalBlockFile(chainparams, file);
-            if (ShutdownRequested()) {
-                LogPrintf("Shutdown requested. Exit %s\n", __func__);
-                return;
-            }
-        } else {
-            LogPrintf("Warning: Could not open blocks file %s\n", path.string());
-        }
-    }
-
-    // scan for better chains in the block chain database, that are not yet connected in the active best chain
-
-    // We can't hold cs_main during ActivateBestChain even though we're accessing
-    // the chainman unique_ptrs since ABC requires us not to be holding cs_main, so retrieve
-    // the relevant pointers before the ABC call.
-    for (CChainState* chainstate : WITH_LOCK(::cs_main, return chainman.GetAll())) {
-        BlockValidationState state;
-        if (!chainstate->ActivateBestChain(state, chainparams, nullptr)) {
-            LogPrintf("Failed to connect best block (%s)\n", state.ToString());
-            StartShutdown();
-            return;
-        }
-    }
-
-    if (args.GetBoolArg("-stopafterblockimport", DEFAULT_STOPAFTERBLOCKIMPORT)) {
-        LogPrintf("Stopping after block import\n");
-        StartShutdown();
-        return;
-    }
-    } // End scope of CImportingNow
-    chainman.ActiveChainstate().LoadMempool(args);
-}
-
 /** Sanity checks
  *  Ensure that Bitcoin is running in a usable environment with all
  *  necessary library support.
diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
new file mode 100644
index 0000000000..235dbb1693
--- /dev/null
+++ b/src/node/blockstorage.cpp
@@ -0,0 +1,96 @@
+// Copyright (c) 2011-2021 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <blockstorage.h>
+
+#include <chainparams.h>
+#include <fs.h>
+#include <shutdown.h>
+#include <util/system.h>
+#include <validation.h>
+
+struct CImportingNow {
+    CImportingNow()
+    {
+        assert(fImporting == false);
+        fImporting = true;
+    }
+
+    ~CImportingNow()
+    {
+        assert(fImporting == true);
+        fImporting = false;
+    }
+};
+
+void ThreadImport(ChainstateManager& chainman, std::vector<fs::path> vImportFiles, const ArgsManager& args)
+{
+    const CChainParams& chainparams = Params();
+    ScheduleBatchPriority();
+
+    {
+        CImportingNow imp;
+
+        // -reindex
+        if (fReindex) {
+            int nFile = 0;
+            while (true) {
+                FlatFilePos pos(nFile, 0);
+                if (!fs::exists(GetBlockPosFilename(pos)))
+                    break; // No block files left to reindex
+                FILE* file = OpenBlockFile(pos, true);
+                if (!file)
+                    break; // This error is logged in OpenBlockFile
+                LogPrintf("Reindexing block file blk%05u.dat...\n", (unsigned int)nFile);
+                ::ChainstateActive().LoadExternalBlockFile(chainparams, file, &pos);
+                if (ShutdownRequested()) {
+                    LogPrintf("Shutdown requested. Exit %s\n", __func__);
+                    return;
+                }
+                nFile++;
+            }
+            pblocktree->WriteReindexing(false);
+            fReindex = false;
+            LogPrintf("Reindexing finished\n");
+            // To avoid ending up in a situation without genesis block, re-try initializing (no-op if reindexing worked):
+            ::ChainstateActive().LoadGenesisBlock(chainparams);
+        }
+
+        // -loadblock=
+        for (const fs::path& path : vImportFiles) {
+            FILE* file = fsbridge::fopen(path, "rb");
+            if (file) {
+                LogPrintf("Importing blocks file %s...\n", path.string());
+                ::ChainstateActive().LoadExternalBlockFile(chainparams, file);
+                if (ShutdownRequested()) {
+                    LogPrintf("Shutdown requested. Exit %s\n", __func__);
+                    return;
+                }
+            } else {
+                LogPrintf("Warning: Could not open blocks file %s\n", path.string());
+            }
+        }
+
+        // scan for better chains in the block chain database, that are not yet connected in the active best chain
+
+        // We can't hold cs_main during ActivateBestChain even though we're accessing
+        // the chainman unique_ptrs since ABC requires us not to be holding cs_main, so retrieve
+        // the relevant pointers before the ABC call.
+        for (CChainState* chainstate : WITH_LOCK(::cs_main, return chainman.GetAll())) {
+            BlockValidationState state;
+            if (!chainstate->ActivateBestChain(state, chainparams, nullptr)) {
+                LogPrintf("Failed to connect best block (%s)\n", state.ToString());
+                StartShutdown();
+                return;
+            }
+        }
+
+        if (args.GetBoolArg("-stopafterblockimport", DEFAULT_STOPAFTERBLOCKIMPORT)) {
+            LogPrintf("Stopping after block import\n");
+            StartShutdown();
+            return;
+        }
+    } // End scope of CImportingNow
+    chainman.ActiveChainstate().LoadMempool(args);
+}
diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h
new file mode 100644
index 0000000000..d81b39f9f9
--- /dev/null
+++ b/src/node/blockstorage.h
@@ -0,0 +1,19 @@
+// Copyright (c) 2011-2021 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_NODE_BLOCKSTORAGE_H
+#define BITCOIN_NODE_BLOCKSTORAGE_H
+
+#include <vector>
+
+#include <fs.h>
+
+class ArgsManager;
+class ChainstateManager;
+
+static constexpr bool DEFAULT_STOPAFTERBLOCKIMPORT{false};
+
+void ThreadImport(ChainstateManager& chainman, std::vector<fs::path> vImportFiles, const ArgsManager& args);
+
+#endif // BITCOIN_NODE_BLOCKSTORAGE_H

From fa91b2b2b3447a3645e7958c7dc4e1946a69cb9c Mon Sep 17 00:00:00 2001
From: MarcoFalke <falke.marco@gmail.com>
Date: Fri, 2 Apr 2021 20:49:01 +0200
Subject: [PATCH 3/6] move-only: Move AbortNode to shutdown

Can be reviewed with the git option
--color-moved=dimmed-zebra
---
 src/shutdown.cpp   | 14 ++++++++++++++
 src/shutdown.h     |  5 +++++
 src/validation.cpp | 13 -------------
 3 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/src/shutdown.cpp b/src/shutdown.cpp
index 2fc195e2d1..35faf3c412 100644
--- a/src/shutdown.cpp
+++ b/src/shutdown.cpp
@@ -6,7 +6,9 @@
 #include <shutdown.h>
 
 #include <logging.h>
+#include <node/ui_interface.h>
 #include <util/tokenpipe.h>
+#include <warnings.h>
 
 #include <config/bitcoin-config.h>
 
@@ -16,6 +18,18 @@
 #include <condition_variable>
 #endif
 
+bool AbortNode(const std::string& strMessage, bilingual_str user_message)
+{
+    SetMiscWarning(Untranslated(strMessage));
+    LogPrintf("*** %s\n", strMessage);
+    if (user_message.empty()) {
+        user_message = _("A fatal internal error occurred, see debug.log for details");
+    }
+    AbortError(user_message);
+    StartShutdown();
+    return false;
+}
+
 static std::atomic<bool> fRequestShutdown(false);
 #ifdef WIN32
 /** On windows it is possible to simply use a condition variable. */
diff --git a/src/shutdown.h b/src/shutdown.h
index b2fbdb8cfb..ff56c6bd87 100644
--- a/src/shutdown.h
+++ b/src/shutdown.h
@@ -6,6 +6,11 @@
 #ifndef BITCOIN_SHUTDOWN_H
 #define BITCOIN_SHUTDOWN_H
 
+#include <util/translation.h> // For bilingual_str
+
+/** Abort with a message */
+bool AbortNode(const std::string& strMessage, bilingual_str user_message = bilingual_str{});
+
 /** Initialize shutdown state. This must be called before using either StartShutdown(),
  * AbortShutdown() or WaitForShutdown(). Calling ShutdownRequested() is always safe.
  */
diff --git a/src/validation.cpp b/src/validation.cpp
index 19363c0efb..6f184780c8 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1636,19 +1636,6 @@ bool UndoReadFromDisk(CBlockUndo& blockundo, const CBlockIndex* pindex)
     return true;
 }
 
-/** Abort with a message */
-static bool AbortNode(const std::string& strMessage, bilingual_str user_message = bilingual_str())
-{
-    SetMiscWarning(Untranslated(strMessage));
-    LogPrintf("*** %s\n", strMessage);
-    if (user_message.empty()) {
-        user_message = _("A fatal internal error occurred, see debug.log for details");
-    }
-    AbortError(user_message);
-    StartShutdown();
-    return false;
-}
-
 static bool AbortNode(BlockValidationState& state, const std::string& strMessage, const bilingual_str& userMessage = bilingual_str())
 {
     AbortNode(strMessage, userMessage);

From fa0c7d9ad24d3c9515d3f9c136af4071cbd79055 Mon Sep 17 00:00:00 2001
From: MarcoFalke <falke.marco@gmail.com>
Date: Fri, 2 Apr 2021 20:42:05 +0200
Subject: [PATCH 4/6] move-only: Move *Disk functions to blockstorage

Can be reviewed with the git options
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
---
 src/index/base.cpp                      |   3 +-
 src/index/blockfilterindex.cpp          |   2 +-
 src/net_processing.cpp                  |   1 +
 src/node/blockstorage.cpp               | 142 +++++++++++++++++++++++-
 src/node/blockstorage.h                 |  21 ++++
 src/node/interfaces.cpp                 |   4 +-
 src/rest.cpp                            |   1 +
 src/rpc/blockchain.cpp                  |   1 +
 src/rpc/rawtransaction.cpp              |   1 +
 src/test/util/blockfilter.cpp           |   1 +
 src/validation.cpp                      | 140 +----------------------
 src/validation.h                        |   9 --
 src/zmq/zmqpublishnotifier.cpp          |   3 +-
 test/lint/lint-circular-dependencies.sh |   4 +-
 14 files changed, 181 insertions(+), 152 deletions(-)

diff --git a/src/index/base.cpp b/src/index/base.cpp
index 25644c3b41..9e637c9c6f 100644
--- a/src/index/base.cpp
+++ b/src/index/base.cpp
@@ -4,12 +4,13 @@
 
 #include <chainparams.h>
 #include <index/base.h>
+#include <node/blockstorage.h>
 #include <node/ui_interface.h>
 #include <shutdown.h>
 #include <tinyformat.h>
 #include <util/system.h>
 #include <util/translation.h>
-#include <validation.h>
+#include <validation.h> // For g_chainman
 #include <warnings.h>
 
 constexpr char DB_BEST_BLOCK = 'B';
diff --git a/src/index/blockfilterindex.cpp b/src/index/blockfilterindex.cpp
index 32271fb7ab..154d7a7027 100644
--- a/src/index/blockfilterindex.cpp
+++ b/src/index/blockfilterindex.cpp
@@ -6,8 +6,8 @@
 
 #include <dbwrapper.h>
 #include <index/blockfilterindex.h>
+#include <node/blockstorage.h>
 #include <util/system.h>
-#include <validation.h>
 
 /* The index database stores three items for each block: the disk location of the encoded filter,
  * its dSHA256 hash, and the header. Those belonging to blocks on the active chain are indexed by
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 4108de2c8a..61faf7e1ee 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -16,6 +16,7 @@
 #include <merkleblock.h>
 #include <netbase.h>
 #include <netmessagemaker.h>
+#include <node/blockstorage.h>
 #include <policy/fees.h>
 #include <policy/policy.h>
 #include <primitives/block.h>
diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
index 235dbb1693..de54bf1f42 100644
--- a/src/node/blockstorage.cpp
+++ b/src/node/blockstorage.cpp
@@ -2,14 +2,154 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
-#include <blockstorage.h>
+#include <node/blockstorage.h>
 
+#include <chain.h>
 #include <chainparams.h>
+#include <flatfile.h>
 #include <fs.h>
+#include <pow.h>
 #include <shutdown.h>
+#include <signet.h>
+#include <streams.h>
 #include <util/system.h>
 #include <validation.h>
 
+// From validation. TODO move here
+bool FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigned int nHeight, CChain& active_chain, uint64_t nTime, bool fKnown = false);
+
+static bool WriteBlockToDisk(const CBlock& block, FlatFilePos& pos, const CMessageHeader::MessageStartChars& messageStart)
+{
+    // Open history file to append
+    CAutoFile fileout(OpenBlockFile(pos), SER_DISK, CLIENT_VERSION);
+    if (fileout.IsNull())
+        return error("WriteBlockToDisk: OpenBlockFile failed");
+
+    // Write index header
+    unsigned int nSize = GetSerializeSize(block, fileout.GetVersion());
+    fileout << messageStart << nSize;
+
+    // Write block
+    long fileOutPos = ftell(fileout.Get());
+    if (fileOutPos < 0)
+        return error("WriteBlockToDisk: ftell failed");
+    pos.nPos = (unsigned int)fileOutPos;
+    fileout << block;
+
+    return true;
+}
+
+bool ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos, const Consensus::Params& consensusParams)
+{
+    block.SetNull();
+
+    // Open history file to read
+    CAutoFile filein(OpenBlockFile(pos, true), SER_DISK, CLIENT_VERSION);
+    if (filein.IsNull())
+        return error("ReadBlockFromDisk: OpenBlockFile failed for %s", pos.ToString());
+
+    // Read block
+    try {
+        filein >> block;
+    }
+    catch (const std::exception& e) {
+        return error("%s: Deserialize or I/O error - %s at %s", __func__, e.what(), pos.ToString());
+    }
+
+    // Check the header
+    if (!CheckProofOfWork(block.GetHash(), block.nBits, consensusParams))
+        return error("ReadBlockFromDisk: Errors in block header at %s", pos.ToString());
+
+    // Signet only: check block solution
+    if (consensusParams.signet_blocks && !CheckSignetBlockSolution(block, consensusParams)) {
+        return error("ReadBlockFromDisk: Errors in block solution at %s", pos.ToString());
+    }
+
+    return true;
+}
+
+bool ReadBlockFromDisk(CBlock& block, const CBlockIndex* pindex, const Consensus::Params& consensusParams)
+{
+    FlatFilePos blockPos;
+    {
+        LOCK(cs_main);
+        blockPos = pindex->GetBlockPos();
+    }
+
+    if (!ReadBlockFromDisk(block, blockPos, consensusParams))
+        return false;
+    if (block.GetHash() != pindex->GetBlockHash())
+        return error("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s",
+                     pindex->ToString(), pindex->GetBlockPos().ToString());
+    return true;
+}
+
+bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const FlatFilePos& pos, const CMessageHeader::MessageStartChars& message_start)
+{
+    FlatFilePos hpos = pos;
+    hpos.nPos -= 8; // Seek back 8 bytes for meta header
+    CAutoFile filein(OpenBlockFile(hpos, true), SER_DISK, CLIENT_VERSION);
+    if (filein.IsNull()) {
+        return error("%s: OpenBlockFile failed for %s", __func__, pos.ToString());
+    }
+
+    try {
+        CMessageHeader::MessageStartChars blk_start;
+        unsigned int blk_size;
+
+        filein >> blk_start >> blk_size;
+
+        if (memcmp(blk_start, message_start, CMessageHeader::MESSAGE_START_SIZE)) {
+            return error("%s: Block magic mismatch for %s: %s versus expected %s", __func__, pos.ToString(),
+                         HexStr(blk_start),
+                         HexStr(message_start));
+        }
+
+        if (blk_size > MAX_SIZE) {
+            return error("%s: Block data is larger than maximum deserialization size for %s: %s versus %s", __func__, pos.ToString(),
+                         blk_size, MAX_SIZE);
+        }
+
+        block.resize(blk_size); // Zeroing of memory is intentional here
+        filein.read((char*)block.data(), blk_size);
+    } catch (const std::exception& e) {
+        return error("%s: Read from block file failed: %s for %s", __func__, e.what(), pos.ToString());
+    }
+
+    return true;
+}
+
+bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const CBlockIndex* pindex, const CMessageHeader::MessageStartChars& message_start)
+{
+    FlatFilePos block_pos;
+    {
+        LOCK(cs_main);
+        block_pos = pindex->GetBlockPos();
+    }
+
+    return ReadRawBlockFromDisk(block, block_pos, message_start);
+}
+
+/** Store block on disk. If dbp is non-nullptr, the file is known to already reside on disk */
+FlatFilePos SaveBlockToDisk(const CBlock& block, int nHeight, CChain& active_chain, const CChainParams& chainparams, const FlatFilePos* dbp)
+{
+    unsigned int nBlockSize = ::GetSerializeSize(block, CLIENT_VERSION);
+    FlatFilePos blockPos;
+    if (dbp != nullptr)
+        blockPos = *dbp;
+    if (!FindBlockPos(blockPos, nBlockSize + 8, nHeight, active_chain, block.GetBlockTime(), dbp != nullptr)) {
+        error("%s: FindBlockPos failed", __func__);
+        return FlatFilePos();
+    }
+    if (dbp == nullptr) {
+        if (!WriteBlockToDisk(block, blockPos, chainparams.MessageStart())) {
+            AbortNode("Failed to write block");
+            return FlatFilePos();
+        }
+    }
+    return blockPos;
+}
+
 struct CImportingNow {
     CImportingNow()
     {
diff --git a/src/node/blockstorage.h b/src/node/blockstorage.h
index d81b39f9f9..3b546f0719 100644
--- a/src/node/blockstorage.h
+++ b/src/node/blockstorage.h
@@ -5,15 +5,36 @@
 #ifndef BITCOIN_NODE_BLOCKSTORAGE_H
 #define BITCOIN_NODE_BLOCKSTORAGE_H
 
+#include <cstdint>
 #include <vector>
 
 #include <fs.h>
+#include <protocol.h> // For CMessageHeader::MessageStartChars
 
 class ArgsManager;
+class CBlock;
+class CBlockIndex;
+class CBlockUndo;
+class CChain;
+class CChainParams;
 class ChainstateManager;
+struct FlatFilePos;
+namespace Consensus {
+struct Params;
+}
 
 static constexpr bool DEFAULT_STOPAFTERBLOCKIMPORT{false};
 
+/** Functions for disk access for blocks */
+bool ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos, const Consensus::Params& consensusParams);
+bool ReadBlockFromDisk(CBlock& block, const CBlockIndex* pindex, const Consensus::Params& consensusParams);
+bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const FlatFilePos& pos, const CMessageHeader::MessageStartChars& message_start);
+bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const CBlockIndex* pindex, const CMessageHeader::MessageStartChars& message_start);
+
+bool UndoReadFromDisk(CBlockUndo& blockundo, const CBlockIndex* pindex);
+
+FlatFilePos SaveBlockToDisk(const CBlock& block, int nHeight, CChain& active_chain, const CChainParams& chainparams, const FlatFilePos* dbp);
+
 void ThreadImport(ChainstateManager& chainman, std::vector<fs::path> vImportFiles, const ArgsManager& args);
 
 #endif // BITCOIN_NODE_BLOCKSTORAGE_H
diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
index 7ad02f81dc..264bec58f9 100644
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -4,7 +4,6 @@
 
 #include <addrdb.h>
 #include <banman.h>
-#include <boost/signals2/signal.hpp>
 #include <chain.h>
 #include <chainparams.h>
 #include <init.h>
@@ -17,6 +16,7 @@
 #include <net_processing.h>
 #include <netaddress.h>
 #include <netbase.h>
+#include <node/blockstorage.h>
 #include <node/coin.h>
 #include <node/context.h>
 #include <node/transaction.h>
@@ -53,6 +53,8 @@
 #include <optional>
 #include <utility>
 
+#include <boost/signals2/signal.hpp>
+
 using interfaces::BlockTip;
 using interfaces::Chain;
 using interfaces::FoundBlock;
diff --git a/src/rest.cpp b/src/rest.cpp
index aa97470ca7..1398de19e3 100644
--- a/src/rest.cpp
+++ b/src/rest.cpp
@@ -8,6 +8,7 @@
 #include <core_io.h>
 #include <httpserver.h>
 #include <index/txindex.h>
+#include <node/blockstorage.h>
 #include <node/context.h>
 #include <primitives/block.h>
 #include <primitives/transaction.h>
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index e1501d7254..183fa50228 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -14,6 +14,7 @@
 #include <core_io.h>
 #include <hash.h>
 #include <index/blockfilterindex.h>
+#include <node/blockstorage.h>
 #include <node/coinstats.h>
 #include <node/context.h>
 #include <node/utxo_snapshot.h>
diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
index 7932bd2915..527e9be87c 100644
--- a/src/rpc/rawtransaction.cpp
+++ b/src/rpc/rawtransaction.cpp
@@ -10,6 +10,7 @@
 #include <index/txindex.h>
 #include <key_io.h>
 #include <merkleblock.h>
+#include <node/blockstorage.h>
 #include <node/coin.h>
 #include <node/context.h>
 #include <node/psbt.h>
diff --git a/src/test/util/blockfilter.cpp b/src/test/util/blockfilter.cpp
index bccff5e5a6..b8ab9d2344 100644
--- a/src/test/util/blockfilter.cpp
+++ b/src/test/util/blockfilter.cpp
@@ -5,6 +5,7 @@
 #include <test/util/blockfilter.h>
 
 #include <chainparams.h>
+#include <node/blockstorage.h>
 #include <validation.h>
 
 
diff --git a/src/validation.cpp b/src/validation.cpp
index 6f184780c8..652282920e 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -21,6 +21,7 @@
 #include <index/txindex.h>
 #include <logging.h>
 #include <logging/timer.h>
+#include <node/blockstorage.h>
 #include <node/coinstats.h>
 #include <node/ui_interface.h>
 #include <policy/policy.h>
@@ -1148,123 +1149,6 @@ CTransactionRef GetTransaction(const CBlockIndex* const block_index, const CTxMe
     return nullptr;
 }
 
-//////////////////////////////////////////////////////////////////////////////
-//
-// CBlock and CBlockIndex
-//
-
-static bool WriteBlockToDisk(const CBlock& block, FlatFilePos& pos, const CMessageHeader::MessageStartChars& messageStart)
-{
-    // Open history file to append
-    CAutoFile fileout(OpenBlockFile(pos), SER_DISK, CLIENT_VERSION);
-    if (fileout.IsNull())
-        return error("WriteBlockToDisk: OpenBlockFile failed");
-
-    // Write index header
-    unsigned int nSize = GetSerializeSize(block, fileout.GetVersion());
-    fileout << messageStart << nSize;
-
-    // Write block
-    long fileOutPos = ftell(fileout.Get());
-    if (fileOutPos < 0)
-        return error("WriteBlockToDisk: ftell failed");
-    pos.nPos = (unsigned int)fileOutPos;
-    fileout << block;
-
-    return true;
-}
-
-bool ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos, const Consensus::Params& consensusParams)
-{
-    block.SetNull();
-
-    // Open history file to read
-    CAutoFile filein(OpenBlockFile(pos, true), SER_DISK, CLIENT_VERSION);
-    if (filein.IsNull())
-        return error("ReadBlockFromDisk: OpenBlockFile failed for %s", pos.ToString());
-
-    // Read block
-    try {
-        filein >> block;
-    }
-    catch (const std::exception& e) {
-        return error("%s: Deserialize or I/O error - %s at %s", __func__, e.what(), pos.ToString());
-    }
-
-    // Check the header
-    if (!CheckProofOfWork(block.GetHash(), block.nBits, consensusParams))
-        return error("ReadBlockFromDisk: Errors in block header at %s", pos.ToString());
-
-    // Signet only: check block solution
-    if (consensusParams.signet_blocks && !CheckSignetBlockSolution(block, consensusParams)) {
-        return error("ReadBlockFromDisk: Errors in block solution at %s", pos.ToString());
-    }
-
-    return true;
-}
-
-bool ReadBlockFromDisk(CBlock& block, const CBlockIndex* pindex, const Consensus::Params& consensusParams)
-{
-    FlatFilePos blockPos;
-    {
-        LOCK(cs_main);
-        blockPos = pindex->GetBlockPos();
-    }
-
-    if (!ReadBlockFromDisk(block, blockPos, consensusParams))
-        return false;
-    if (block.GetHash() != pindex->GetBlockHash())
-        return error("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s",
-                pindex->ToString(), pindex->GetBlockPos().ToString());
-    return true;
-}
-
-bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const FlatFilePos& pos, const CMessageHeader::MessageStartChars& message_start)
-{
-    FlatFilePos hpos = pos;
-    hpos.nPos -= 8; // Seek back 8 bytes for meta header
-    CAutoFile filein(OpenBlockFile(hpos, true), SER_DISK, CLIENT_VERSION);
-    if (filein.IsNull()) {
-        return error("%s: OpenBlockFile failed for %s", __func__, pos.ToString());
-    }
-
-    try {
-        CMessageHeader::MessageStartChars blk_start;
-        unsigned int blk_size;
-
-        filein >> blk_start >> blk_size;
-
-        if (memcmp(blk_start, message_start, CMessageHeader::MESSAGE_START_SIZE)) {
-            return error("%s: Block magic mismatch for %s: %s versus expected %s", __func__, pos.ToString(),
-                    HexStr(blk_start),
-                    HexStr(message_start));
-        }
-
-        if (blk_size > MAX_SIZE) {
-            return error("%s: Block data is larger than maximum deserialization size for %s: %s versus %s", __func__, pos.ToString(),
-                    blk_size, MAX_SIZE);
-        }
-
-        block.resize(blk_size); // Zeroing of memory is intentional here
-        filein.read((char*)block.data(), blk_size);
-    } catch(const std::exception& e) {
-        return error("%s: Read from block file failed: %s for %s", __func__, e.what(), pos.ToString());
-    }
-
-    return true;
-}
-
-bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const CBlockIndex* pindex, const CMessageHeader::MessageStartChars& message_start)
-{
-    FlatFilePos block_pos;
-    {
-        LOCK(cs_main);
-        block_pos = pindex->GetBlockPos();
-    }
-
-    return ReadRawBlockFromDisk(block, block_pos, message_start);
-}
-
 CAmount GetBlockSubsidy(int nHeight, const Consensus::Params& consensusParams)
 {
     int halvings = nHeight / consensusParams.nSubsidyHalvingInterval;
@@ -3218,7 +3102,8 @@ void CChainState::ReceivedBlockTransactions(const CBlock& block, CBlockIndex* pi
     }
 }
 
-static bool FindBlockPos(FlatFilePos &pos, unsigned int nAddSize, unsigned int nHeight, CChain& active_chain, uint64_t nTime, bool fKnown = false)
+// TODO move to blockstorage
+bool FindBlockPos(FlatFilePos& pos, unsigned int nAddSize, unsigned int nHeight, CChain& active_chain, uint64_t nTime, bool fKnown = false)
 {
     LOCK(cs_LastBlockFile);
 
@@ -3695,25 +3580,6 @@ bool ChainstateManager::ProcessNewBlockHeaders(const std::vector<CBlockHeader>&
     return true;
 }
 
-/** Store block on disk. If dbp is non-nullptr, the file is known to already reside on disk */
-static FlatFilePos SaveBlockToDisk(const CBlock& block, int nHeight, CChain& active_chain, const CChainParams& chainparams, const FlatFilePos* dbp) {
-    unsigned int nBlockSize = ::GetSerializeSize(block, CLIENT_VERSION);
-    FlatFilePos blockPos;
-    if (dbp != nullptr)
-        blockPos = *dbp;
-    if (!FindBlockPos(blockPos, nBlockSize+8, nHeight, active_chain, block.GetBlockTime(), dbp != nullptr)) {
-        error("%s: FindBlockPos failed", __func__);
-        return FlatFilePos();
-    }
-    if (dbp == nullptr) {
-        if (!WriteBlockToDisk(block, blockPos, chainparams.MessageStart())) {
-            AbortNode("Failed to write block");
-            return FlatFilePos();
-        }
-    }
-    return blockPos;
-}
-
 /** Store block on disk. If dbp is non-nullptr, the file is known to already reside on disk */
 bool CChainState::AcceptBlock(const std::shared_ptr<const CBlock>& pblock, BlockValidationState& state, const CChainParams& chainparams, CBlockIndex** ppindex, bool fRequested, const FlatFilePos* dbp, bool* fNewBlock)
 {
diff --git a/src/validation.h b/src/validation.h
index 3f7e967ec7..1eb010889d 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -300,15 +300,6 @@ public:
 /** Initializes the script-execution cache */
 void InitScriptExecutionCache();
 
-
-/** Functions for disk access for blocks */
-bool ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos, const Consensus::Params& consensusParams);
-bool ReadBlockFromDisk(CBlock& block, const CBlockIndex* pindex, const Consensus::Params& consensusParams);
-bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const FlatFilePos& pos, const CMessageHeader::MessageStartChars& message_start);
-bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const CBlockIndex* pindex, const CMessageHeader::MessageStartChars& message_start);
-
-bool UndoReadFromDisk(CBlockUndo& blockundo, const CBlockIndex* pindex);
-
 /** Functions for validating blocks and updating the block tree */
 
 /** Context-independent validity checks */
diff --git a/src/zmq/zmqpublishnotifier.cpp b/src/zmq/zmqpublishnotifier.cpp
index 168ba841c8..25afa94d0f 100644
--- a/src/zmq/zmqpublishnotifier.cpp
+++ b/src/zmq/zmqpublishnotifier.cpp
@@ -6,10 +6,11 @@
 
 #include <chain.h>
 #include <chainparams.h>
+#include <node/blockstorage.h>
 #include <rpc/server.h>
 #include <streams.h>
 #include <util/system.h>
-#include <validation.h>
+#include <validation.h> // For cs_main
 #include <zmq/zmqutil.h>
 
 #include <zmq.h>
diff --git a/test/lint/lint-circular-dependencies.sh b/test/lint/lint-circular-dependencies.sh
index 0b15f99448..ad2333a808 100755
--- a/test/lint/lint-circular-dependencies.sh
+++ b/test/lint/lint-circular-dependencies.sh
@@ -11,7 +11,9 @@ export LC_ALL=C
 EXPECTED_CIRCULAR_DEPENDENCIES=(
     "chainparamsbase -> util/system -> chainparamsbase"
     "index/txindex -> validation -> index/txindex"
-    "index/blockfilterindex -> validation -> index/blockfilterindex"
+    "node/blockstorage -> validation -> node/blockstorage"
+    "index/blockfilterindex -> node/blockstorage -> validation -> index/blockfilterindex"
+    "index/base -> validation -> index/blockfilterindex -> index/base"
     "policy/fees -> txmempool -> policy/fees"
     "qt/addresstablemodel -> qt/walletmodel -> qt/addresstablemodel"
     "qt/bitcoingui -> qt/walletframe -> qt/bitcoingui"

From fa121b628d51bb0e25eb3fbd716881fa55527dc7 Mon Sep 17 00:00:00 2001
From: MarcoFalke <falke.marco@gmail.com>
Date: Fri, 2 Apr 2021 19:27:59 +0200
Subject: [PATCH 5/6] blockstorage: [refactor] Use chainman reference where
 possible

Also, add missing { } for style.

Can be reviewed with `--word-diff-regex=.`
---
 src/node/blockstorage.cpp | 36 ++++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 14 deletions(-)

diff --git a/src/node/blockstorage.cpp b/src/node/blockstorage.cpp
index de54bf1f42..daed6605e8 100644
--- a/src/node/blockstorage.cpp
+++ b/src/node/blockstorage.cpp
@@ -22,8 +22,9 @@ static bool WriteBlockToDisk(const CBlock& block, FlatFilePos& pos, const CMessa
 {
     // Open history file to append
     CAutoFile fileout(OpenBlockFile(pos), SER_DISK, CLIENT_VERSION);
-    if (fileout.IsNull())
+    if (fileout.IsNull()) {
         return error("WriteBlockToDisk: OpenBlockFile failed");
+    }
 
     // Write index header
     unsigned int nSize = GetSerializeSize(block, fileout.GetVersion());
@@ -31,8 +32,9 @@ static bool WriteBlockToDisk(const CBlock& block, FlatFilePos& pos, const CMessa
 
     // Write block
     long fileOutPos = ftell(fileout.Get());
-    if (fileOutPos < 0)
+    if (fileOutPos < 0) {
         return error("WriteBlockToDisk: ftell failed");
+    }
     pos.nPos = (unsigned int)fileOutPos;
     fileout << block;
 
@@ -45,20 +47,21 @@ bool ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos, const Consensus::P
 
     // Open history file to read
     CAutoFile filein(OpenBlockFile(pos, true), SER_DISK, CLIENT_VERSION);
-    if (filein.IsNull())
+    if (filein.IsNull()) {
         return error("ReadBlockFromDisk: OpenBlockFile failed for %s", pos.ToString());
+    }
 
     // Read block
     try {
         filein >> block;
-    }
-    catch (const std::exception& e) {
+    } catch (const std::exception& e) {
         return error("%s: Deserialize or I/O error - %s at %s", __func__, e.what(), pos.ToString());
     }
 
     // Check the header
-    if (!CheckProofOfWork(block.GetHash(), block.nBits, consensusParams))
+    if (!CheckProofOfWork(block.GetHash(), block.nBits, consensusParams)) {
         return error("ReadBlockFromDisk: Errors in block header at %s", pos.ToString());
+    }
 
     // Signet only: check block solution
     if (consensusParams.signet_blocks && !CheckSignetBlockSolution(block, consensusParams)) {
@@ -76,11 +79,13 @@ bool ReadBlockFromDisk(CBlock& block, const CBlockIndex* pindex, const Consensus
         blockPos = pindex->GetBlockPos();
     }
 
-    if (!ReadBlockFromDisk(block, blockPos, consensusParams))
+    if (!ReadBlockFromDisk(block, blockPos, consensusParams)) {
         return false;
-    if (block.GetHash() != pindex->GetBlockHash())
+    }
+    if (block.GetHash() != pindex->GetBlockHash()) {
         return error("ReadBlockFromDisk(CBlock&, CBlockIndex*): GetHash() doesn't match index for %s at %s",
                      pindex->ToString(), pindex->GetBlockPos().ToString());
+    }
     return true;
 }
 
@@ -135,8 +140,9 @@ FlatFilePos SaveBlockToDisk(const CBlock& block, int nHeight, CChain& active_cha
 {
     unsigned int nBlockSize = ::GetSerializeSize(block, CLIENT_VERSION);
     FlatFilePos blockPos;
-    if (dbp != nullptr)
+    if (dbp != nullptr) {
         blockPos = *dbp;
+    }
     if (!FindBlockPos(blockPos, nBlockSize + 8, nHeight, active_chain, block.GetBlockTime(), dbp != nullptr)) {
         error("%s: FindBlockPos failed", __func__);
         return FlatFilePos();
@@ -177,13 +183,15 @@ void ThreadImport(ChainstateManager& chainman, std::vector<fs::path> vImportFile
             int nFile = 0;
             while (true) {
                 FlatFilePos pos(nFile, 0);
-                if (!fs::exists(GetBlockPosFilename(pos)))
+                if (!fs::exists(GetBlockPosFilename(pos))) {
                     break; // No block files left to reindex
+                }
                 FILE* file = OpenBlockFile(pos, true);
-                if (!file)
+                if (!file) {
                     break; // This error is logged in OpenBlockFile
+                }
                 LogPrintf("Reindexing block file blk%05u.dat...\n", (unsigned int)nFile);
-                ::ChainstateActive().LoadExternalBlockFile(chainparams, file, &pos);
+                chainman.ActiveChainstate().LoadExternalBlockFile(chainparams, file, &pos);
                 if (ShutdownRequested()) {
                     LogPrintf("Shutdown requested. Exit %s\n", __func__);
                     return;
@@ -194,7 +202,7 @@ void ThreadImport(ChainstateManager& chainman, std::vector<fs::path> vImportFile
             fReindex = false;
             LogPrintf("Reindexing finished\n");
             // To avoid ending up in a situation without genesis block, re-try initializing (no-op if reindexing worked):
-            ::ChainstateActive().LoadGenesisBlock(chainparams);
+            chainman.ActiveChainstate().LoadGenesisBlock(chainparams);
         }
 
         // -loadblock=
@@ -202,7 +210,7 @@ void ThreadImport(ChainstateManager& chainman, std::vector<fs::path> vImportFile
             FILE* file = fsbridge::fopen(path, "rb");
             if (file) {
                 LogPrintf("Importing blocks file %s...\n", path.string());
-                ::ChainstateActive().LoadExternalBlockFile(chainparams, file);
+                chainman.ActiveChainstate().LoadExternalBlockFile(chainparams, file);
                 if (ShutdownRequested()) {
                     LogPrintf("Shutdown requested. Exit %s\n", __func__);
                     return;

From fadcd3f78e1dd1acd7a774f8fad68dc471ff9e1f Mon Sep 17 00:00:00 2001
From: MarcoFalke <falke.marco@gmail.com>
Date: Sun, 4 Apr 2021 18:19:09 +0200
Subject: [PATCH 6/6] doc: Remove irrelevant link to GitHub

The doc nicely explains why the directory exists and it is
irrelevant when it was introduced. Even if it was relevant,
it could be trivially found out via `git log ./src/node/ | tail`
without visiting GitHub
---
 src/node/README.md | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/node/README.md b/src/node/README.md
index e99a717534..ab5979594d 100644
--- a/src/node/README.md
+++ b/src/node/README.md
@@ -15,8 +15,7 @@ As a rule of thumb, code in one of the [`src/node/`](./),
 calling code in the other directories directly, and only invoke it indirectly
 through the more limited [`src/interfaces/`](../interfaces/) classes.
 
-The [`src/node/`](./) directory is a new directory introduced in
-[#14978](https://github.com/bitcoin/bitcoin/pull/14978) and at the moment is
+This directory is at the moment
 sparsely populated. Eventually more substantial files like
 [`src/validation.cpp`](../validation.cpp) and
 [`src/txmempool.cpp`](../txmempool.cpp) might be moved there.