init: use std::thread for ThreadImport() #19197

nit: The AppInitMain() is a way large function. Could the scope of the import variable be limited by a block?

I think it might be safer to check if the thread is joinable before joining in case this path is reached after some initialization failure when the thread may not have started.

Thanks, I've added that.

I don't really like adding more and more globals to init. Ideally init should be a slim file with only calls to other modules to start/stop. Longer term ThreadImport should be moved to validation because it is purely validation related. Init shouldn't need to know how to load blocks from disk, etc... Note that LoadMempool is also in validation.

Moreover, with assumeutxo, thus multiple chainstates, I could imagine that a reindex/import can be done in the background chainstate for additional efficiency. Having init to deal with even more chainstate related logic would feel wrong.

What do you think about making this thread a member of the chainstate manager?

On the other hand this was a hidden global before, a part of threadGroup (which can hopefully go away soon!). I don't think this PR makes anything worse.

I think your longer-term solution makes sense, but I'm not sure it needs to be done here.

Agree that it doesn't make anything worse, but it could be slightly better with a trivial diff.

diff --git a/src/init.cpp b/src/init.cpp
index 8d9566edc3..984f6b2034 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -152,8 +152,6 @@ NODISCARD static bool CreatePidFile()
 
 static std::unique_ptr<ECCVerifyHandle> globalVerifyHandle;
 
-static std::thread g_load_block;
-
 static boost::thread_group threadGroup;
 
 void Interrupt(NodeContext& node)
@@ -218,7 +216,7 @@ void Shutdown(NodeContext& node)
     // After everything has been shut down, but before things get flushed, stop the
     // CScheduler/checkqueue, threadGroup 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();
     threadGroup.interrupt_all();
     threadGroup.join_all();
 
@@ -1844,7 +1842,7 @@ bool AppInitMain(const util::Ref& context, NodeContext& node)
         vImportFiles.push_back(strFile);
     }
 
-    g_load_block = std::thread(&TraceThread<std::function<void()>>, "loadblk", [=, &chainman]{ ThreadImport(chainman, vImportFiles); });
+    chainman.m_load_block = std::thread(&TraceThread<std::function<void()>>, "loadblk", [=, &chainman]{ ThreadImport(chainman, vImportFiles); });
 
     // Wait for genesis block to be processed
     {
diff --git a/src/validation.h b/src/validation.h
index e403bcb51a..9764a7aaca 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -783,6 +783,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);

Thanks. I'll take care of the suggestions, adding to chainman and thread renaming, in some followup changes. I just wanted to get it out of the Boost thread group here.