Merge bitcoin/bitcoin#27125: refactor, kernel: Decouple ArgsManager from blockstorage

5ff63a0 refactor, blockstorage: Replace stopafterblockimport arg (TheCharlatan)
18e5ba7 refactor, blockstorage: Replace blocksdir arg (TheCharlatan)
02a0899 refactor, BlockManager: Replace fastprune from arg with options (TheCharlatan)
a498d69 refactor/iwyu: Complete includes for blockmanager_args (TheCharlatan)
f0bb102 refactor: Move functions to BlockManager methods (TheCharlatan)
cfbb212 zmq: Pass lambda to zmq's ZMQPublishRawBlockNotifier (TheCharlatan)
8ed4ff8 refactor: Declare g_zmq_notification_interface as unique_ptr (TheCharlatan)

Pull request description:

  The libbitcoin_kernel library should not rely on the `ArgsManager`, but rather use option structs that can be passed to the various classes it uses. This PR removes reliance on the `ArgsManager` from the `blockstorage.*` files. Like similar prior work, it uses the options struct in the `BlockManager` that can be populated with `ArgsManager` values.

  Some related prior work: bitcoin/bitcoin#26889 bitcoin/bitcoin#25862 bitcoin/bitcoin#25527 bitcoin/bitcoin#25487

  Related PR removing blockstorage globals: bitcoin/bitcoin#25781

ACKs for top commit:
  ryanofsky:
    Code review ACK 5ff63a0. Since last ACK just added std::move and fixed commit title. Sorry for the noise!
  mzumsande:
    Code Review ACK 5ff63a0

Tree-SHA512: 4bde8fd140a40b97eca923e9016d85dcea6fad6fd199731f158376294af59c3e8b163a0725aa47b4be3519b61828044e0a042deea005e0c28de21d8b6c3e1ea7

Author: fanquake

ci/test/06_script_b.sh

@@ -52,6 +52,7 @@ if [ "${RUN_TIDY}" = "true" ]; then
            src/dbwrapper.cpp \
            src/init \
            src/kernel \
+           src/node/blockmanager_args.cpp \
            src/node/chainstate.cpp \
            src/node/chainstatemanager_args.cpp \
            src/node/mempool_args.cpp \

src/bitcoin-chainstate.cpp

@@ -89,6 +89,7 @@ int main(int argc, char* argv[])
     };
     const node::BlockManager::Options blockman_opts{
         .chainparams = chainman_opts.chainparams,
+        .blocks_dir = gArgs.GetBlocksDirPath(),
     };
     ChainstateManager chainman{chainman_opts, blockman_opts};
 

src/index/base.cpp

@@ -23,8 +23,6 @@
 #include <string>
 #include <utility>
 
-using node::ReadBlockFromDisk;
-
 constexpr uint8_t DB_BEST_BLOCK{'B'};
 
 constexpr auto SYNC_LOG_INTERVAL{30s};
@@ -159,8 +157,6 @@ void BaseIndex::ThreadSync()
     SetSyscallSandboxPolicy(SyscallSandboxPolicy::TX_INDEX);
     const CBlockIndex* pindex = m_best_block_index.load();
     if (!m_synced) {
-        auto& consensus_params = Params().GetConsensus();
-
         std::chrono::steady_clock::time_point last_log_time{0s};
         std::chrono::steady_clock::time_point last_locator_write_time{0s};
         while (true) {
@@ -207,7 +203,7 @@ void BaseIndex::ThreadSync()
 
             CBlock block;
             interfaces::BlockInfo block_info = kernel::MakeBlockInfo(pindex);
-            if (!ReadBlockFromDisk(block, pindex, consensus_params)) {
+            if (!m_chainstate->m_blockman.ReadBlockFromDisk(block, *pindex)) {
                 FatalError("%s: Failed to read block %s from disk",
                            __func__, pindex->GetBlockHash().ToString());
                 return;

src/index/blockfilterindex.cpp

@@ -12,8 +12,6 @@
 #include <util/fs_helpers.h>
 #include <validation.h>
 
-using node::UndoReadFromDisk;
-
 /* 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
  * height, and those belonging to blocks that have been reorganized out of the active chain are
@@ -223,7 +221,7 @@ bool BlockFilterIndex::CustomAppend(const interfaces::BlockInfo& block)
         // pindex variable gives indexing code access to node internals. It
         // will be removed in upcoming commit
         const CBlockIndex* pindex = WITH_LOCK(cs_main, return m_chainstate->m_blockman.LookupBlockIndex(block.hash));
-        if (!UndoReadFromDisk(block_undo, pindex)) {
+        if (!m_chainstate->m_blockman.UndoReadFromDisk(block_undo, *pindex)) {
             return false;
         }
 

src/index/coinstatsindex.cpp

@@ -19,9 +19,6 @@ using kernel::CCoinsStats;
 using kernel::GetBogoSize;
 using kernel::TxOutSer;
 
-using node::ReadBlockFromDisk;
-using node::UndoReadFromDisk;
-
 static constexpr uint8_t DB_BLOCK_HASH{'s'};
 static constexpr uint8_t DB_BLOCK_HEIGHT{'t'};
 static constexpr uint8_t DB_MUHASH{'M'};
@@ -125,7 +122,7 @@ bool CoinStatsIndex::CustomAppend(const interfaces::BlockInfo& block)
         // pindex variable gives indexing code access to node internals. It
         // will be removed in upcoming commit
         const CBlockIndex* pindex = WITH_LOCK(cs_main, return m_chainstate->m_blockman.LookupBlockIndex(block.hash));
-        if (!UndoReadFromDisk(block_undo, pindex)) {
+        if (!m_chainstate->m_blockman.UndoReadFromDisk(block_undo, *pindex)) {
             return false;
         }
 
@@ -282,12 +279,11 @@ bool CoinStatsIndex::CustomRewind(const interfaces::BlockKey& current_tip, const
         LOCK(cs_main);
         const CBlockIndex* iter_tip{m_chainstate->m_blockman.LookupBlockIndex(current_tip.hash)};
         const CBlockIndex* new_tip_index{m_chainstate->m_blockman.LookupBlockIndex(new_tip.hash)};
-        const auto& consensus_params{Params().GetConsensus()};
 
         do {
             CBlock block;
 
-            if (!ReadBlockFromDisk(block, iter_tip, consensus_params)) {
+            if (!m_chainstate->m_blockman.ReadBlockFromDisk(block, *iter_tip)) {
                 return error("%s: Failed to read block %s from disk",
                              __func__, iter_tip->GetBlockHash().ToString());
             }
@@ -409,7 +405,7 @@ bool CoinStatsIndex::ReverseBlock(const CBlock& block, const CBlockIndex* pindex
 
     // Ignore genesis block
     if (pindex->nHeight > 0) {
-        if (!UndoReadFromDisk(block_undo, pindex)) {
+        if (!m_chainstate->m_blockman.UndoReadFromDisk(block_undo, *pindex)) {
             return false;
         }
 

src/index/txindex.cpp

@@ -10,8 +10,6 @@
 #include <node/blockstorage.h>
 #include <validation.h>
 
-using node::OpenBlockFile;
-
 constexpr uint8_t DB_TXINDEX{'t'};
 
 std::unique_ptr<TxIndex> g_txindex;
@@ -80,7 +78,7 @@ bool TxIndex::FindTx(const uint256& tx_hash, uint256& block_hash, CTransactionRe
         return false;
     }
 
-    CAutoFile file(OpenBlockFile(postx, true), SER_DISK, CLIENT_VERSION);
+    CAutoFile file(m_chainstate->m_blockman.OpenBlockFile(postx, true), SER_DISK, CLIENT_VERSION);
     if (file.IsNull()) {
         return error("%s: OpenBlockFile failed", __func__);
     }

src/init.cpp

@@ -112,22 +112,23 @@
 #include <zmq/zmqrpc.h>
 #endif
 
+using kernel::DEFAULT_STOPAFTERBLOCKIMPORT;
 using kernel::DumpMempool;
 using kernel::ValidationCacheSizes;
 
 using node::ApplyArgsManOptions;
+using node::BlockManager;
 using node::CacheSizes;
 using node::CalculateCacheSizes;
 using node::DEFAULT_PERSIST_MEMPOOL;
 using node::DEFAULT_PRINTPRIORITY;
-using node::DEFAULT_STOPAFTERBLOCKIMPORT;
+using node::fReindex;
 using node::LoadChainstate;
 using node::MempoolPath;
-using node::ShouldPersistMempool;
 using node::NodeContext;
+using node::ShouldPersistMempool;
 using node::ThreadImport;
 using node::VerifyLoadedChainstate;
-using node::fReindex;
 
 static constexpr bool DEFAULT_PROXYRANDOMIZE{true};
 static constexpr bool DEFAULT_REST_ENABLE{false};
@@ -328,9 +329,8 @@ void Shutdown(NodeContext& node)
 
 #if ENABLE_ZMQ
     if (g_zmq_notification_interface) {
-        UnregisterValidationInterface(g_zmq_notification_interface);
-        delete g_zmq_notification_interface;
-        g_zmq_notification_interface = nullptr;
+        UnregisterValidationInterface(g_zmq_notification_interface.get());
+        g_zmq_notification_interface.reset();
     }
 #endif
 
@@ -1038,8 +1038,9 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb
         if (const auto error{ApplyArgsManOptions(args, chainman_opts_dummy)}) {
             return InitError(*error);
         }
-        node::BlockManager::Options blockman_opts_dummy{
+        BlockManager::Options blockman_opts_dummy{
             .chainparams = chainman_opts_dummy.chainparams,
+            .blocks_dir = args.GetBlocksDirPath(),
         };
         if (const auto error{ApplyArgsManOptions(args, blockman_opts_dummy)}) {
             return InitError(*error);
@@ -1425,10 +1426,14 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     }
 
 #if ENABLE_ZMQ
-    g_zmq_notification_interface = CZMQNotificationInterface::Create();
+    g_zmq_notification_interface = CZMQNotificationInterface::Create(
+        [&chainman = node.chainman](CBlock& block, const CBlockIndex& index) {
+            assert(chainman);
+            return chainman->m_blockman.ReadBlockFromDisk(block, index);
+        });
 
     if (g_zmq_notification_interface) {
-        RegisterValidationInterface(g_zmq_notification_interface);
+        RegisterValidationInterface(g_zmq_notification_interface.get());
     }
 #endif
 
@@ -1443,8 +1448,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     };
     Assert(!ApplyArgsManOptions(args, chainman_opts)); // no error can happen, already checked in AppInitParameterInteraction
 
-    node::BlockManager::Options blockman_opts{
+    BlockManager::Options blockman_opts{
         .chainparams = chainman_opts.chainparams,
+        .blocks_dir = args.GetBlocksDirPath(),
     };
     Assert(!ApplyArgsManOptions(args, blockman_opts)); // no error can happen, already checked in AppInitParameterInteraction
 
@@ -1674,7 +1680,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     }
 
     chainman.m_load_block = std::thread(&util::TraceThread, "loadblk", [=, &chainman, &args] {
-        ThreadImport(chainman, vImportFiles, args, ShouldPersistMempool(args) ? MempoolPath(args) : fs::path{});
+        ThreadImport(chainman, vImportFiles, ShouldPersistMempool(args) ? MempoolPath(args) : fs::path{});
     });
 
     // Wait for genesis block to be processed

src/kernel/blockmanager_opts.h

@@ -5,17 +5,26 @@
 #ifndef BITCOIN_KERNEL_BLOCKMANAGER_OPTS_H
 #define BITCOIN_KERNEL_BLOCKMANAGER_OPTS_H
 
+#include <util/fs.h>
+
+#include <cstdint>
+
 class CChainParams;
 
 namespace kernel {
 
+static constexpr bool DEFAULT_STOPAFTERBLOCKIMPORT{false};
+
 /**
  * An options struct for `BlockManager`, more ergonomically referred to as
  * `BlockManager::Options` due to the using-declaration in `BlockManager`.
  */
 struct BlockManagerOpts {
     const CChainParams& chainparams;
     uint64_t prune_target{0};
+    bool fast_prune{false};
+    bool stop_after_block_import{DEFAULT_STOPAFTERBLOCKIMPORT};
+    const fs::path blocks_dir;
 };
 
 } // namespace kernel

src/net_processing.cpp

@@ -51,9 +51,6 @@
 #include <optional>
 #include <typeinfo>
 
-using node::ReadBlockFromDisk;
-using node::ReadRawBlockFromDisk;
-
 /** How long to cache transactions in mapRelay for normal relay */
 static constexpr auto RELAY_TX_CACHE_TIME = 15min;
 /** How long a transaction has to be in the mempool before it can unconditionally be relayed (even when not in mapRelay). */
@@ -2189,15 +2186,15 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv&
         // Fast-path: in this case it is possible to serve the block directly from disk,
         // as the network format matches the format on disk
         std::vector<uint8_t> block_data;
-        if (!ReadRawBlockFromDisk(block_data, pindex->GetBlockPos(), m_chainparams.MessageStart())) {
+        if (!m_chainman.m_blockman.ReadRawBlockFromDisk(block_data, pindex->GetBlockPos(), m_chainparams.MessageStart())) {
             assert(!"cannot load block from disk");
         }
         m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::BLOCK, Span{block_data}));
         // Don't set pblock as we've sent the block
     } else {
         // Send block from disk
         std::shared_ptr<CBlock> pblockRead = std::make_shared<CBlock>();
-        if (!ReadBlockFromDisk(*pblockRead, pindex, m_chainparams.GetConsensus())) {
+        if (!m_chainman.m_blockman.ReadBlockFromDisk(*pblockRead, *pindex)) {
             assert(!"cannot load block from disk");
         }
         pblock = pblockRead;
@@ -3889,7 +3886,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
 
             if (pindex->nHeight >= m_chainman.ActiveChain().Height() - MAX_BLOCKTXN_DEPTH) {
                 CBlock block;
-                bool ret = ReadBlockFromDisk(block, pindex, m_chainparams.GetConsensus());
+                const bool ret{m_chainman.m_blockman.ReadBlockFromDisk(block, *pindex)};
                 assert(ret);
 
                 SendBlockTransactions(pfrom, *peer, block, req);
@@ -5546,7 +5543,7 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
                         m_connman.PushMessage(pto, std::move(cached_cmpctblock_msg.value()));
                     } else {
                         CBlock block;
-                        bool ret = ReadBlockFromDisk(block, pBestIndex, consensusParams);
+                        const bool ret{m_chainman.m_blockman.ReadBlockFromDisk(block, *pBestIndex)};
                         assert(ret);
                         CBlockHeaderAndShortTxIDs cmpctblock{block};
                         m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::CMPCTBLOCK, cmpctblock));

src/node/blockmanager_args.cpp

@@ -5,8 +5,14 @@
 #include <node/blockmanager_args.h>
 
 #include <common/args.h>
+#include <node/blockstorage.h>
+#include <tinyformat.h>
+#include <util/translation.h>
 #include <validation.h>
 
+#include <cstdint>
+#include <optional>
+
 namespace node {
 std::optional<bilingual_str> ApplyArgsManOptions(const ArgsManager& args, BlockManager::Options& opts)
 {
@@ -25,6 +31,9 @@ std::optional<bilingual_str> ApplyArgsManOptions(const ArgsManager& args, BlockM
     }
     opts.prune_target = nPruneTarget;
 
+    if (auto value{args.GetBoolArg("-fastprune")}) opts.fast_prune = *value;
+    if (auto value{args.GetBoolArg("-stopafterblockimport")}) opts.stop_after_block_import = *value;
+
     return std::nullopt;
 }
 } // namespace node