Merge bitcoin#34661: ipc mining: Prevent `Assertion m_node.chainman' failed`` errors on early startup

bbc8f1e ipc mining: Prevent ``Assertion `m_node.chainman' failed`` errors on early startup (Ryan Ofsky)
a7cabf9 init refactor: Only initialize node.notifications one time (Ryan Ofsky)
c8e332c init refactor: Remove node.init accesss in AppInitInterfaces (Ryan Ofsky)

Pull request description:

  This fixes ``Assertion `m_node.chainman' failed`` errors first reported bitcoin#33994 (comment) when IPC mining methods are called before ChainstateManager is loaded.

  The fix works by making the `Init.makeMining` method wait until chainstate data is loaded. It's probably the simplest possible fix but other alternatives like moving the wait to `Mining.createNewBlock` were discussed in the thread bitcoin#34661 (comment) and could be implemented later without changes to clients.

ACKs for top commit:
  Sjors:
    utACK bbc8f1e
  ismaelsadeeq:
    ACK bbc8f1e
  achow101:
    ACK bbc8f1e

Tree-SHA512: 3e2e4e28ccff364b2303efd06ce337a229c28609076638500acb29559f716a15ad99409c6970ce9ad91776d53e3f9d959f1bbbd144ea9a4a2fb578ddbf2da267

Author: achow101

src/init.cpp

@@ -1206,8 +1206,10 @@ bool AppInitLockDirectories()
 
 bool AppInitInterfaces(NodeContext& node)
 {
-    node.chain = node.init->makeChain();
-    node.mining = node.init->makeMining();
+    node.chain = interfaces::MakeChain(node);
+    // Specify wait_loaded=false so internal mining interface can be initialized
+    // on early startup and does not need to be tied to chainstate loading.
+    node.mining = interfaces::MakeMining(node, /*wait_loaded=*/false);
     return true;
 }
 
@@ -1304,16 +1306,12 @@ static ChainstateLoadResult InitAndLoadChainstate(
     const ArgsManager& args)
 {
     // This function may be called twice, so any dirty state must be reset.
-    node.notifications.reset(); // Drop state, such as a cached tip block
+    node.notifications->setChainstateLoaded(false); // Drop state, such as a cached tip block
     node.mempool.reset();
     node.chainman.reset(); // Drop state, such as an initialized m_block_tree_db
 
     const CChainParams& chainparams = Params();
 
-    Assert(!node.notifications); // Was reset above
-    node.notifications = std::make_unique<KernelNotifications>(Assert(node.shutdown_request), node.exit_status, *Assert(node.warnings));
-    ReadNotificationArgs(args, *node.notifications);
-
     CTxMemPool::Options mempool_opts{
         .check_ratio = chainparams.DefaultConsistencyChecks() ? 1 : 0,
         .signals = node.validation_signals.get(),
@@ -1414,6 +1412,7 @@ static ChainstateLoadResult InitAndLoadChainstate(
         std::tie(status, error) = catch_exceptions([&] { return VerifyLoadedChainstate(chainman, options); });
         if (status == node::ChainstateLoadStatus::SUCCESS) {
             LogInfo("Block index and chainstate loaded");
+            node.notifications->setChainstateLoaded(true);
         }
     }
     return {status, error};
@@ -1486,6 +1485,13 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     node.validation_signals = std::make_unique<ValidationSignals>(std::make_unique<SerialTaskRunner>(scheduler));
     auto& validation_signals = *node.validation_signals;
 
+    // Create KernelNotifications object. Important to do this early before
+    // calling ipc->listenAddress() below so makeMining and other IPC methods
+    // can use this.
+    assert(!node.notifications);
+    node.notifications = std::make_unique<KernelNotifications>(Assert(node.shutdown_request), node.exit_status, *Assert(node.warnings));
+    ReadNotificationArgs(args, *node.notifications);
+
     // Create client interfaces for wallets that are supposed to be loaded
     // according to -wallet and -disablewallet options. This only constructs
     // the interfaces, it doesn't load wallet data. Wallets actually get loaded

src/interfaces/mining.h

@@ -160,7 +160,11 @@ class Mining
 };
 
 //! Return implementation of Mining interface.
-std::unique_ptr<Mining> MakeMining(node::NodeContext& node);
+//!
+//! @param[in] wait_loaded waits for chainstate data to be loaded before
+//!                        returning. Used to prevent external clients from
+//!                        being able to crash the node during startup.
+std::unique_ptr<Mining> MakeMining(node::NodeContext& node, bool wait_loaded=true);
 
 } // namespace interfaces
 

src/node/interfaces.cpp

@@ -1017,5 +1017,17 @@ class MinerImpl : public Mining
 namespace interfaces {
 std::unique_ptr<Node> MakeNode(node::NodeContext& context) { return std::make_unique<node::NodeImpl>(context); }
 std::unique_ptr<Chain> MakeChain(node::NodeContext& context) { return std::make_unique<node::ChainImpl>(context); }
-std::unique_ptr<Mining> MakeMining(node::NodeContext& context) { return std::make_unique<node::MinerImpl>(context); }
+std::unique_ptr<Mining> MakeMining(node::NodeContext& context, bool wait_loaded)
+{
+    if (wait_loaded) {
+        node::KernelNotifications& kernel_notifications(*Assert(context.notifications));
+        util::SignalInterrupt& interrupt(*Assert(context.shutdown_signal));
+        WAIT_LOCK(kernel_notifications.m_tip_block_mutex, lock);
+        kernel_notifications.m_tip_block_cv.wait(lock, [&]() EXCLUSIVE_LOCKS_REQUIRED(kernel_notifications.m_tip_block_mutex) {
+            return kernel_notifications.m_state.chainstate_loaded || interrupt;
+        });
+        if (interrupt) return nullptr;
+    }
+    return std::make_unique<node::MinerImpl>(context);
+}
 } // namespace interfaces

src/node/kernel_notifications.cpp

@@ -53,7 +53,7 @@ kernel::InterruptResult KernelNotifications::blockTip(SynchronizationState state
     {
         LOCK(m_tip_block_mutex);
         Assume(index.GetBlockHash() != uint256::ZERO);
-        m_tip_block = index.GetBlockHash();
+        m_state.tip_block = index.GetBlockHash();
         m_tip_block_cv.notify_all();
     }
 
@@ -103,7 +103,7 @@ void KernelNotifications::fatalError(const bilingual_str& message)
 std::optional<uint256> KernelNotifications::TipBlock()
 {
     AssertLockHeld(m_tip_block_mutex);
-    return m_tip_block;
+    return m_state.tip_block;
 };
 
 

src/node/kernel_notifications.h

@@ -29,6 +29,18 @@ namespace node {
 class Warnings;
 static constexpr int DEFAULT_STOPATHEIGHT{0};
 
+//! State tracked by the KernelNotifications interface meant to be used by
+//! mining code, index code, RPCs, and other code sitting above the validation
+//! layer.
+//!
+//! Currently just tracks the chain tip, but could be used to hold other
+//! information in the future, like the last flushed block, pruning
+//! information, etc.
+struct KernelState {
+    bool chainstate_loaded{false};
+    std::optional<uint256> tip_block;
+};
+
 class KernelNotifications : public kernel::Notifications
 {
 public:
@@ -49,13 +61,21 @@ class KernelNotifications : public kernel::Notifications
 
     void fatalError(const bilingual_str& message) override;
 
+    void setChainstateLoaded(bool chainstate_loaded) EXCLUSIVE_LOCKS_REQUIRED(!m_tip_block_mutex) {
+        LOCK(m_tip_block_mutex);
+        if (!chainstate_loaded) m_state = {};
+        m_state.chainstate_loaded = chainstate_loaded;
+        m_tip_block_cv.notify_all();
+    }
+
     //! Block height after which blockTip notification will return Interrupted{}, if >0.
     int m_stop_at_height{DEFAULT_STOPATHEIGHT};
     //! Useful for tests, can be set to false to avoid shutdown on fatal error.
     bool m_shutdown_on_fatal_error{true};
 
     Mutex m_tip_block_mutex;
     std::condition_variable m_tip_block_cv GUARDED_BY(m_tip_block_mutex);
+    KernelState m_state GUARDED_BY(m_tip_block_mutex);
     //! The block for which the last blockTip notification was received.
     //! It's first set when the tip is connected during node initialization.
     //! Might be unset during an early shutdown.
@@ -65,8 +85,6 @@ class KernelNotifications : public kernel::Notifications
     const std::function<bool()>& m_shutdown_request;
     std::atomic<int>& m_exit_status;
     node::Warnings& m_warnings;
-
-    std::optional<uint256> m_tip_block GUARDED_BY(m_tip_block_mutex);
 };
 
 void ReadNotificationArgs(const ArgsManager& args, KernelNotifications& notifications);

src/test/miner_tests.cpp

@@ -68,7 +68,7 @@ struct MinerTestingSetup : public TestingSetup {
     }
     std::unique_ptr<Mining> MakeMining()
     {
-        return interfaces::MakeMining(m_node);
+        return interfaces::MakeMining(m_node, /*wait_loaded=*/false);
     }
 };
 } // namespace miner_tests

src/test/testnet4_miner_tests.cpp

@@ -22,7 +22,7 @@ namespace testnet4_miner_tests {
 struct Testnet4MinerTestingSetup : public Testnet4Setup {
     std::unique_ptr<Mining> MakeMining()
     {
-        return interfaces::MakeMining(m_node);
+        return interfaces::MakeMining(m_node, /*wait_loaded=*/false);
     }
 };
 } // namespace testnet4_miner_tests

test/functional/interface_ipc_mining.py

@@ -147,6 +147,34 @@ async def wait_for_tip():
 
         asyncio.run(capnp.run(async_routine()))
 
+    def run_early_startup_test(self):
+        """Make sure mining.createNewBlock safely returns on early startup as
+        soon as mining interface is available """
+        self.log.info("Running Mining interface early startup test")
+
+        node = self.nodes[0]
+        self.stop_node(node.index)
+        node.start()
+
+        async def async_routine():
+            while True:
+                try:
+                    ctx, mining = await self.make_mining_ctx()
+                    break
+                except (ConnectionRefusedError, FileNotFoundError):
+                    # Poll quickly to connect as soon as socket becomes
+                    # available but without using a lot of CPU
+                    await asyncio.sleep(0.005)
+
+            opts = self.capnp_modules['mining'].BlockCreateOptions()
+            await mining.createNewBlock(ctx, opts)
+
+        asyncio.run(capnp.run(async_routine()))
+
+        # Reconnect nodes so next tests are happy
+        node.wait_for_rpc_connection()
+        self.connect_nodes(1, 0)
+
     def run_block_template_test(self):
         """Test BlockTemplate interface methods."""
         self.log.info("Running BlockTemplate interface test")
@@ -374,6 +402,7 @@ def run_test(self):
         self.miniwallet = MiniWallet(self.nodes[0])
         self.default_block_create_options = self.capnp_modules['mining'].BlockCreateOptions()
         self.run_mining_interface_test()
+        self.run_early_startup_test()
         self.run_block_template_test()
         self.run_coinbase_and_submission_test()
         self.run_ipc_option_override_test()