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

d87784ac87 kernel: SanityChecks: Return an error struct (Carl Dong)
265d6393bf Move init::SanityCheck to kernel::SanityCheck (Carl Dong)
fed085a1a4 init: Initialize globals with kernel::Context's life (Carl Dong)
7d03feef81 kernel: Introduce empty and unused kernel::Context (Carl Dong)
eeb4fc20c5 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 d87784ac87
  vasild:
    ACK d87784ac87

Tree-SHA512: cd6b4923ea1865001b5f0caed9a4ff99c198d22bf74154d935dc09a47fda22ebe585ec912398cea69f722454ed1dbb4898faab5a2d02fb4c5e719c5c8d71a3f9

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 \

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();
 }

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");

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);
 
-    init::SetGlobals();
+    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
 
-    if (!init::SanityChecks()) {
         return InitError(strprintf(_("Initialization sanity check failed. %s is shutting down."), PACKAGE_NAME));
     }
 

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.

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);

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);

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;
+}
+
+}

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

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

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

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>

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;

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() &&
+        if (!AppInitBasicSetup(gArgs)) return false;
-               AppInitLockDataDirectory() && AppInitInterfaces(*m_context);
+        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
     {

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();
+    m_node.kernel = std::make_unique<kernel::Context>();
-    ECC_Start();
     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)

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; // keep as first member to be destructed last
-    node::NodeContext m_node;
 
     explicit BasicTestingSetup(const std::string& chainName = CBaseChainParams::MAIN, const std::vector<const char*>& extra_args = {});
     ~BasicTestingSetup();