Merge bitcoin/bitcoin#25065: [kernel 2c/n] Introduce `kernel::Context`, encapsulate global init/teardown

d87784ac87364fc977bbf9769c8bdb72dea8cbf9 kernel: SanityChecks: Return an error struct (Carl Dong)
265d6393bf9ef52e7ef7de97ca9c031da82a5ad1 Move init::SanityCheck to kernel::SanityCheck (Carl Dong)
fed085a1a4cd2787202752b6a0d98e42dce97f09 init: Initialize globals with kernel::Context's life (Carl Dong)
7d03feef8156ef37a4efa01dc591467bc7d957bf kernel: Introduce empty and unused kernel::Context (Carl Dong)
eeb4fc20c578b1e428a92d64cc9f8f903a677580 test: Use Set/UnsetGlobals in BasicTestingSetup (Carl Dong)

Pull request description:

  The full `init/common.cpp` is dependent on things like ArgsManager (which we wish to remove from libbitcoinkernel in the future) and sanity checks. These aren't necessary for libbitcoinkernel so we only extract the portion that is necessary (namely `init::{Set,Unset}Globals()`.

ACKs for top commit:
  theuni:
    ACK d87784ac87364fc977bbf9769c8bdb72dea8cbf9
  vasild:
    ACK d87784ac87364fc977bbf9769c8bdb72dea8cbf9

Tree-SHA512: cd6b4923ea1865001b5f0caed9a4ff99c198d22bf74154d935dc09a47fda22ebe585ec912398cea69f722454ed1dbb4898faab5a2d02fb4c5e719c5c8d71a3f9

16 files changed, 180 insertions, 65 deletions

diff --git a/src/Makefile.am b/src/Makefile.am
index d80bd79d53..89329f5f69 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -171,7 +171,9 @@ BITCOIN_CORE_H = \
   interfaces/node.h \
   interfaces/wallet.h \
   kernel/chainstatemanager_opts.h \
+  kernel/checks.h \
   kernel/coinstats.h \
+  kernel/context.h \
   key.h \
   key_io.h \
   logging.h \
@@ -356,7 +358,9 @@ libbitcoin_node_a_SOURCES = \
   index/coinstatsindex.cpp \
   index/txindex.cpp \
   init.cpp \
+  kernel/checks.cpp \
   kernel/coinstats.cpp \
+  kernel/context.cpp \
   mapport.cpp \
   net.cpp \
   netgroup.cpp \
@@ -866,8 +870,9 @@ libbitcoinkernel_la_SOURCES = \
   flatfile.cpp \
   fs.cpp \
   hash.cpp \
-  init/common.cpp \
+  kernel/checks.cpp \
   kernel/coinstats.cpp \
+  kernel/context.cpp \
   key.cpp \
   logging.cpp \
   node/blockstorage.cpp \
diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp
index 99aa23fb06..1817aa1a53 100644
--- a/src/bitcoin-chainstate.cpp
+++ b/src/bitcoin-chainstate.cpp
@@ -11,10 +11,12 @@
 //
 // It is part of the libbitcoinkernel project.
 
+#include <kernel/checks.h>
+#include <kernel/context.h>
+
 #include <chainparams.h>
 #include <consensus/validation.h>
 #include <core_io.h>
-#include <init/common.h>
 #include <node/blockstorage.h>
 #include <node/chainstate.h>
 #include <scheduler.h>
@@ -24,6 +26,7 @@
 #include <validation.h>
 #include <validationinterface.h>
 
+#include <cassert>
 #include <filesystem>
 #include <functional>
 #include <iosfwd>
@@ -49,7 +52,11 @@ int main(int argc, char* argv[])
     SelectParams(CBaseChainParams::MAIN);
     const CChainParams& chainparams = Params();
 
-    init::SetGlobals(); // ECC_Start, etc.
+    kernel::Context kernel_context{};
+    // We can't use a goto here, but we can use an assert since none of the
+    // things instantiated so far requires running the epilogue to be torn down
+    // properly
+    assert(!kernel::SanityChecks(kernel_context).has_value());
 
     // Necessary for CheckInputScripts (eventually called by ProcessNewBlock),
     // which will try the script cache first and fall back to actually
@@ -254,6 +261,4 @@ epilogue:
         }
     }
     GetMainSignals().UnregisterBackgroundSignalScheduler();
-
-    init::UnsetGlobals();
 }
diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp
index bc063faed1..92e73d7c2a 100644
--- a/src/bitcoind.cpp
+++ b/src/bitcoind.cpp
@@ -188,11 +188,14 @@ static bool AppInit(NodeContext& node, int argc, char* argv[])
             // InitError will have been called with detailed error, which ends up on console
             return false;
         }
-        if (!AppInitSanityChecks())
+
+        node.kernel = std::make_unique<kernel::Context>();
+        if (!AppInitSanityChecks(*node.kernel))
         {
             // InitError will have been called with detailed error, which ends up on console
             return false;
         }
+
         if (args.GetBoolArg("-daemon", DEFAULT_DAEMON) || args.GetBoolArg("-daemonwait", DEFAULT_DAEMONWAIT)) {
 #if HAVE_DECL_FORK
             tfm::format(std::cout, PACKAGE_NAME " starting\n");
diff --git a/src/init.cpp b/src/init.cpp
index 045808cc71..d0fd6074b1 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -9,6 +9,8 @@
 
 #include <init.h>
 
+#include <kernel/checks.h>
+
 #include <addrman.h>
 #include <banman.h>
 #include <blockfilter.h>
@@ -304,7 +306,7 @@ void Shutdown(NodeContext& node)
     node.chain_clients.clear();
     UnregisterAllValidationInterfaces();
     GetMainSignals().UnregisterBackgroundSignalScheduler();
-    init::UnsetGlobals();
+    node.kernel.reset();
     node.mempool.reset();
     node.fee_estimator.reset();
     node.chainman.reset();
@@ -1089,13 +1091,24 @@ static bool LockDataDirectory(bool probeOnly)
     return true;
 }
 
-bool AppInitSanityChecks()
+bool AppInitSanityChecks(const kernel::Context& kernel)
 {
     // ********************************************************* Step 4: sanity checks
+    auto maybe_error = kernel::SanityChecks(kernel);
+
+    if (maybe_error.has_value()) {
+        switch (maybe_error.value()) {
+        case kernel::SanityCheckError::ERROR_ECC:
+            InitError(Untranslated("Elliptic curve cryptography sanity check failure. Aborting."));
+            break;
+        case kernel::SanityCheckError::ERROR_RANDOM:
+            InitError(Untranslated("OS cryptographic RNG sanity check failure. Aborting."));
+            break;
+        case kernel::SanityCheckError::ERROR_CHRONO:
+            InitError(Untranslated("Clock epoch mismatch. Aborting."));
+            break;
+        } // no default case, so the compiler can warn about missing cases
 
-    init::SetGlobals();
-
-    if (!init::SanityChecks()) {
         return InitError(strprintf(_("Initialization sanity check failed. %s is shutting down."), PACKAGE_NAME));
     }
 
diff --git a/src/init.h b/src/init.h
index 1e22771dc2..e8e6a55eba 100644
--- a/src/init.h
+++ b/src/init.h
@@ -19,6 +19,9 @@ class ArgsManager;
 namespace interfaces {
 struct BlockAndHeaderTipInfo;
 }
+namespace kernel {
+struct Context;
+}
 namespace node {
 struct NodeContext;
 } // namespace node
@@ -47,7 +50,7 @@ bool AppInitParameterInteraction(const ArgsManager& args, bool use_syscall_sandb
  * @note This can be done before daemonization. Do not call Shutdown() if this function fails.
  * @pre Parameters should be parsed and config file should be read, AppInitParameterInteraction should have been called.
  */
-bool AppInitSanityChecks();
+bool AppInitSanityChecks(const kernel::Context& kernel);
 /**
  * Lock bitcoin core data directory.
  * @note This should only be done after daemonization. Do not call Shutdown() if this function fails.
diff --git a/src/init/common.cpp b/src/init/common.cpp
index 788abb9821..d4e45454d2 100644
--- a/src/init/common.cpp
+++ b/src/init/common.cpp
@@ -7,58 +7,19 @@
 #endif
 
 #include <clientversion.h>
-#include <crypto/sha256.h>
 #include <fs.h>
-#include <key.h>
 #include <logging.h>
 #include <node/ui_interface.h>
-#include <pubkey.h>
-#include <random.h>
 #include <tinyformat.h>
 #include <util/system.h>
 #include <util/time.h>
 #include <util/translation.h>
 
 #include <algorithm>
-#include <memory>
 #include <string>
 #include <vector>
 
-static std::unique_ptr<ECCVerifyHandle> globalVerifyHandle;
-
 namespace init {
-void SetGlobals()
-{
-    std::string sha256_algo = SHA256AutoDetect();
-    LogPrintf("Using the '%s' SHA256 implementation\n", sha256_algo);
-    RandomInit();
-    ECC_Start();
-    globalVerifyHandle.reset(new ECCVerifyHandle());
-}
-
-void UnsetGlobals()
-{
-    globalVerifyHandle.reset();
-    ECC_Stop();
-}
-
-bool SanityChecks()
-{
-    if (!ECC_InitSanityCheck()) {
-        return InitError(Untranslated("Elliptic curve cryptography sanity check failure. Aborting."));
-    }
-
-    if (!Random_SanityCheck()) {
-        return InitError(Untranslated("OS cryptographic RNG sanity check failure. Aborting."));
-    }
-
-    if (!ChronoSanityCheck()) {
-        return InitError(Untranslated("Clock epoch mismatch. Aborting."));
-    }
-
-    return true;
-}
-
 void AddLoggingArgs(ArgsManager& argsman)
 {
     argsman.AddArg("-debuglogfile=<file>", strprintf("Specify location of debug log file. Relative paths will be prefixed by a net-specific datadir location. (-nodebuglogfile to disable; default: %s)", DEFAULT_DEBUGLOGFILE), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
diff --git a/src/init/common.h b/src/init/common.h
index fc4bc1b280..2c7f485908 100644
--- a/src/init/common.h
+++ b/src/init/common.h
@@ -11,13 +11,6 @@
 class ArgsManager;
 
 namespace init {
-void SetGlobals();
-void UnsetGlobals();
-/**
- *  Ensure a usable environment with all
- *  necessary library support.
- */
-bool SanityChecks();
 void AddLoggingArgs(ArgsManager& args);
 void SetLoggingOptions(const ArgsManager& args);
 void SetLoggingCategories(const ArgsManager& args);
diff --git a/src/kernel/checks.cpp b/src/kernel/checks.cpp
new file mode 100644
index 0000000000..2a1dd3bfa2
--- /dev/null
+++ b/src/kernel/checks.cpp
@@ -0,0 +1,30 @@
+// Copyright (c) 2022 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 <kernel/checks.h>
+
+#include <key.h>
+#include <random.h>
+#include <util/time.h>
+
+namespace kernel {
+
+std::optional<SanityCheckError> SanityChecks(const Context&)
+{
+    if (!ECC_InitSanityCheck()) {
+        return SanityCheckError::ERROR_ECC;
+    }
+
+    if (!Random_SanityCheck()) {
+        return SanityCheckError::ERROR_RANDOM;
+    }
+
+    if (!ChronoSanityCheck()) {
+        return SanityCheckError::ERROR_CHRONO;
+    }
+
+    return std::nullopt;
+}
+
+}
diff --git a/src/kernel/checks.h b/src/kernel/checks.h
new file mode 100644
index 0000000000..80b207f607
--- /dev/null
+++ b/src/kernel/checks.h
@@ -0,0 +1,27 @@
+// Copyright (c) 2022 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_KERNEL_CHECKS_H
+#define BITCOIN_KERNEL_CHECKS_H
+
+#include <optional>
+
+namespace kernel {
+
+struct Context;
+
+enum class SanityCheckError {
+    ERROR_ECC,
+    ERROR_RANDOM,
+    ERROR_CHRONO,
+};
+
+/**
+ *  Ensure a usable environment with all necessary library support.
+ */
+std::optional<SanityCheckError> SanityChecks(const Context&);
+
+}
+
+#endif // BITCOIN_KERNEL_CHECKS_H
diff --git a/src/kernel/context.cpp b/src/kernel/context.cpp
new file mode 100644
index 0000000000..15413c1840
--- /dev/null
+++ b/src/kernel/context.cpp
@@ -0,0 +1,33 @@
+// Copyright (c) 2022 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 <kernel/context.h>
+
+#include <crypto/sha256.h>
+#include <key.h>
+#include <logging.h>
+#include <pubkey.h>
+#include <random.h>
+
+#include <string>
+
+
+namespace kernel {
+
+Context::Context()
+{
+    std::string sha256_algo = SHA256AutoDetect();
+    LogPrintf("Using the '%s' SHA256 implementation\n", sha256_algo);
+    RandomInit();
+    ECC_Start();
+    ecc_verify_handle.reset(new ECCVerifyHandle());
+}
+
+Context::~Context()
+{
+    ecc_verify_handle.reset();
+    ECC_Stop();
+}
+
+} // namespace kernel
diff --git a/src/kernel/context.h b/src/kernel/context.h
new file mode 100644
index 0000000000..0a08511564
--- /dev/null
+++ b/src/kernel/context.h
@@ -0,0 +1,31 @@
+// Copyright (c) 2022 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_KERNEL_CONTEXT_H
+#define BITCOIN_KERNEL_CONTEXT_H
+
+#include <memory>
+
+class ECCVerifyHandle;
+
+namespace kernel {
+//! Context struct holding the kernel library's logically global state, and
+//! passed to external libbitcoin_kernel functions which need access to this
+//! state. The kernel libary API is a work in progress, so state organization
+//! and member list will evolve over time.
+//!
+//! State stored directly in this struct should be simple. More complex state
+//! should be stored to std::unique_ptr members pointing to opaque types.
+struct Context {
+    std::unique_ptr<ECCVerifyHandle> ecc_verify_handle;
+
+    //! Declare default constructor and destructor that are not inline, so code
+    //! instantiating the kernel::Context struct doesn't need to #include class
+    //! definitions for all the unique_ptr members.
+    Context();
+    ~Context();
+};
+} // namespace kernel
+
+#endif // BITCOIN_KERNEL_CONTEXT_H
diff --git a/src/node/context.cpp b/src/node/context.cpp
index 4787efa1de..d80b8ca7a7 100644
--- a/src/node/context.cpp
+++ b/src/node/context.cpp
@@ -7,6 +7,7 @@
 #include <addrman.h>
 #include <banman.h>
 #include <interfaces/chain.h>
+#include <kernel/context.h>
 #include <net.h>
 #include <net_processing.h>
 #include <netgroup.h>
diff --git a/src/node/context.h b/src/node/context.h
index 91ba456219..31be308787 100644
--- a/src/node/context.h
+++ b/src/node/context.h
@@ -5,6 +5,8 @@
 #ifndef BITCOIN_NODE_CONTEXT_H
 #define BITCOIN_NODE_CONTEXT_H
 
+#include <kernel/context.h>
+
 #include <cassert>
 #include <functional>
 #include <memory>
@@ -39,6 +41,8 @@ namespace node {
 //! any member functions. It should just be a collection of references that can
 //! be used without pulling in unwanted dependencies or functionality.
 struct NodeContext {
+    //! libbitcoin_kernel context
+    std::unique_ptr<kernel::Context> kernel;
     //! Init interface for initializing current process and connecting to other processes.
     interfaces::Init* init{nullptr};
     std::unique_ptr<AddrMan> addrman;
diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
index 4810ae1f68..7752fb0f65 100644
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -90,8 +90,16 @@ public:
     uint32_t getLogCategories() override { return LogInstance().GetCategoryMask(); }
     bool baseInitialize() override
     {
-        return AppInitBasicSetup(gArgs) && AppInitParameterInteraction(gArgs, /*use_syscall_sandbox=*/false) && AppInitSanityChecks() &&
-               AppInitLockDataDirectory() && AppInitInterfaces(*m_context);
+        if (!AppInitBasicSetup(gArgs)) return false;
+        if (!AppInitParameterInteraction(gArgs, /*use_syscall_sandbox=*/false)) return false;
+
+        m_context->kernel = std::make_unique<kernel::Context>();
+        if (!AppInitSanityChecks(*m_context->kernel)) return false;
+
+        if (!AppInitLockDataDirectory()) return false;
+        if (!AppInitInterfaces(*m_context)) return false;
+
+        return true;
     }
     bool appInitMain(interfaces::BlockAndHeaderTipInfo* tip_info) override
     {
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index 01f41ccf82..8f03481f72 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -12,6 +12,7 @@
 #include <consensus/validation.h>
 #include <crypto/sha256.h>
 #include <init.h>
+#include <init/common.h>
 #include <interfaces/chain.h>
 #include <net.h>
 #include <net_processing.h>
@@ -126,8 +127,7 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::ve
     InitLogging(*m_node.args);
     AppInitParameterInteraction(*m_node.args);
     LogInstance().StartLogging();
-    SHA256AutoDetect();
-    ECC_Start();
+    m_node.kernel = std::make_unique<kernel::Context>();
     SetupEnvironment();
     SetupNetworking();
     InitSignatureCache();
@@ -147,7 +147,6 @@ BasicTestingSetup::~BasicTestingSetup()
     LogInstance().DisconnectTestLogger();
     fs::remove_all(m_path_root);
     gArgs.ClearArgs();
-    ECC_Stop();
 }
 
 ChainTestingSetup::ChainTestingSetup(const std::string& chainName, const std::vector<const char*>& extra_args)
diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index 4270eb18c6..37407bcb92 100644
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -81,8 +81,7 @@ static constexpr CAmount CENT{1000000};
  * This just configures logging, data dir and chain parameters.
  */
 struct BasicTestingSetup {
-    ECCVerifyHandle globalVerifyHandle;
-    node::NodeContext m_node;
+    node::NodeContext m_node; // keep as first member to be destructed last
 
     explicit BasicTestingSetup(const std::string& chainName = CBaseChainParams::MAIN, const std::vector<const char*>& extra_args = {});
     ~BasicTestingSetup();