Merge #16839: Replace Connman and BanMan globals with NodeContext local

362ded410b8cb1104b7ef31ff8488fec4824a7d5 Avoid using g_rpc_node global in wallet code (Russell Yanofsky)
8922d7f6b751a3e6b3b9f6fb7961c442877fb65a scripted-diff: Remove g_connman, g_banman globals (Russell Yanofsky)
e6f4f895d5e42feaf7bfa5f41e80292aaa73cd7d Pass NodeContext, ConnMan, BanMan references more places (Russell Yanofsky)
4d5448c76b71c9d91399c31b043237091be2e5e7 MOVEONLY: Move NodeContext struct to node/context.h (Russell Yanofsky)
301bd41a2e6765b185bd55f4c541f9e27aeea29d scripted-diff: Rename InitInterfaces to NodeContext (Russell Yanofsky)

Pull request description:

  This change is mainly a naming / organization change intended to simplify #10102. It:

  - Renames struct InitInterfaces to struct NodeContext and moves it from
    src/init.h to src/node/context.h. This is a cosmetic change intended to make
    the point of the struct more obvious.

  - Gets rid of BanMan and ConnMan globals making them NodeContext members
    instead. Getting rid of these globals has been talked about in past as a way
    to implement testing and simulations. Making them NodeContext members is a
    way of keeping them accessible without the globals.

  - Splits g_rpc_interfaces global into g_rpc_node and g_rpc_chain globals. This
    better separates node and wallet rpc methods. Node RPC methods should have
    access NodeContext, while wallet RPC methods should only have indirect access
    to node functionality via interfaces::Chain.

  - Adds NodeContext& references to interfaces::Chain class and the
    interfaces::MakeChain() function. This is needed to access ConnMan and BanMan
    instances without the globals.

  - Gets rid of redundant Node and Chain instances in Qt tests. This is
    needed due to the previous MakeChain change, and also makes test setup a
    little more straightforward. More cleanup could be done in the future, but it
    will require deduplication of bitcoind, bitcoin-qt, and TestingSetup init
    code.

ACKs for top commit:
  laanwj:
    ACK 362ded410b8cb1104b7ef31ff8488fec4824a7d5

Tree-SHA512: 9ae6ff1e33423291d1e52056bac95e0874538390892a6e83c4c115b3c73155a8827c0191b46eb3d14e3b3f6c23ccb08095490880fbc3188026319c71739f7db2

5 files changed, 46 insertions, 29 deletions

diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp
index b2c20573fb..23099a7799 100644
--- a/src/interfaces/chain.cpp
+++ b/src/interfaces/chain.cpp
@@ -11,6 +11,7 @@
 #include <net.h>
 #include <net_processing.h>
 #include <node/coin.h>
+#include <node/context.h>
 #include <node/transaction.h>
 #include <policy/fees.h>
 #include <policy/policy.h>
@@ -238,6 +239,7 @@ public:
 class ChainImpl : public Chain
 {
 public:
+    explicit ChainImpl(NodeContext& node) : m_node(node) {}
     std::unique_ptr<Chain::Lock> lock(bool try_lock) override
     {
         auto result = MakeUnique<LockImpl>(::cs_main, "cs_main", __FILE__, __LINE__, try_lock);
@@ -286,7 +288,7 @@ public:
     }
     bool broadcastTransaction(const CTransactionRef& tx, std::string& err_string, const CAmount& max_tx_fee, bool relay) override
     {
-        const TransactionError err = BroadcastTransaction(tx, err_string, max_tx_fee, relay, /*wait_callback*/ false);
+        const TransactionError err = BroadcastTransaction(m_node, tx, err_string, max_tx_fee, relay, /*wait_callback*/ false);
         // Chain clients only care about failures to accept the tx to the mempool. Disregard non-mempool related failures.
         // Note: this will need to be updated if BroadcastTransactions() is updated to return other non-mempool failures
         // that Chain clients do not need to know about.
@@ -378,9 +380,10 @@ public:
             notifications.TransactionAddedToMempool(entry.GetSharedTx());
         }
     }
+    NodeContext& m_node;
 };
 } // namespace
 
-std::unique_ptr<Chain> MakeChain() { return MakeUnique<ChainImpl>(); }
+std::unique_ptr<Chain> MakeChain(NodeContext& node) { return MakeUnique<ChainImpl>(node); }
 
 } // namespace interfaces
diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h
index 73a78e21fb..3fe12088c5 100644
--- a/src/interfaces/chain.h
+++ b/src/interfaces/chain.h
@@ -24,6 +24,7 @@ class uint256;
 enum class RBFTransactionState;
 struct CBlockLocator;
 struct FeeCalculation;
+struct NodeContext;
 
 namespace interfaces {
 
@@ -291,7 +292,7 @@ public:
 };
 
 //! Return implementation of Chain interface.
-std::unique_ptr<Chain> MakeChain();
+std::unique_ptr<Chain> MakeChain(NodeContext& node);
 
 //! Return implementation of ChainClient interface for a wallet client. This
 //! function will be undefined in builds where ENABLE_WALLET is false.
diff --git a/src/interfaces/node.cpp b/src/interfaces/node.cpp
index 6577895d50..1877c92178 100644
--- a/src/interfaces/node.cpp
+++ b/src/interfaces/node.cpp
@@ -16,6 +16,7 @@
 #include <net_processing.h>
 #include <netaddress.h>
 #include <netbase.h>
+#include <node/context.h>
 #include <policy/feerate.h>
 #include <policy/fees.h>
 #include <policy/settings.h>
@@ -52,7 +53,6 @@ namespace {
 class NodeImpl : public Node
 {
 public:
-    NodeImpl() { m_interfaces.chain = MakeChain(); }
     void initError(const std::string& message) override { InitError(message); }
     bool parseParameters(int argc, const char* const argv[], std::string& error) override
     {
@@ -75,11 +75,15 @@ public:
         return AppInitBasicSetup() && AppInitParameterInteraction() && AppInitSanityChecks() &&
                AppInitLockDataDirectory();
     }
-    bool appInitMain() override { return AppInitMain(m_interfaces); }
+    bool appInitMain() override
+    {
+        m_context.chain = MakeChain(m_context);
+        return AppInitMain(m_context);
+    }
     void appShutdown() override
     {
-        Interrupt();
-        Shutdown(m_interfaces);
+        Interrupt(m_context);
+        Shutdown(m_context);
     }
     void startShutdown() override { StartShutdown(); }
     bool shutdownRequested() override { return ShutdownRequested(); }
@@ -96,15 +100,15 @@ public:
     bool getProxy(Network net, proxyType& proxy_info) override { return GetProxy(net, proxy_info); }
     size_t getNodeCount(CConnman::NumConnections flags) override
     {
-        return g_connman ? g_connman->GetNodeCount(flags) : 0;
+        return m_context.connman ? m_context.connman->GetNodeCount(flags) : 0;
     }
     bool getNodesStats(NodesStats& stats) override
     {
         stats.clear();
 
-        if (g_connman) {
+        if (m_context.connman) {
             std::vector<CNodeStats> stats_temp;
-            g_connman->GetNodeStats(stats_temp);
+            m_context.connman->GetNodeStats(stats_temp);
 
             stats.reserve(stats_temp.size());
             for (auto& node_stats_temp : stats_temp) {
@@ -125,44 +129,44 @@ public:
     }
     bool getBanned(banmap_t& banmap) override
     {
-        if (g_banman) {
-            g_banman->GetBanned(banmap);
+        if (m_context.banman) {
+            m_context.banman->GetBanned(banmap);
             return true;
         }
         return false;
     }
     bool ban(const CNetAddr& net_addr, BanReason reason, int64_t ban_time_offset) override
     {
-        if (g_banman) {
-            g_banman->Ban(net_addr, reason, ban_time_offset);
+        if (m_context.banman) {
+            m_context.banman->Ban(net_addr, reason, ban_time_offset);
             return true;
         }
         return false;
     }
     bool unban(const CSubNet& ip) override
     {
-        if (g_banman) {
-            g_banman->Unban(ip);
+        if (m_context.banman) {
+            m_context.banman->Unban(ip);
             return true;
         }
         return false;
     }
     bool disconnect(const CNetAddr& net_addr) override
     {
-        if (g_connman) {
-            return g_connman->DisconnectNode(net_addr);
+        if (m_context.connman) {
+            return m_context.connman->DisconnectNode(net_addr);
         }
         return false;
     }
     bool disconnect(NodeId id) override
     {
-        if (g_connman) {
-            return g_connman->DisconnectNode(id);
+        if (m_context.connman) {
+            return m_context.connman->DisconnectNode(id);
         }
         return false;
     }
-    int64_t getTotalBytesRecv() override { return g_connman ? g_connman->GetTotalBytesRecv() : 0; }
-    int64_t getTotalBytesSent() override { return g_connman ? g_connman->GetTotalBytesSent() : 0; }
+    int64_t getTotalBytesRecv() override { return m_context.connman ? m_context.connman->GetTotalBytesRecv() : 0; }
+    int64_t getTotalBytesSent() override { return m_context.connman ? m_context.connman->GetTotalBytesSent() : 0; }
     size_t getMempoolSize() override { return ::mempool.size(); }
     size_t getMempoolDynamicUsage() override { return ::mempool.DynamicMemoryUsage(); }
     bool getHeaderTip(int& height, int64_t& block_time) override
@@ -202,11 +206,11 @@ public:
     bool getImporting() override { return ::fImporting; }
     void setNetworkActive(bool active) override
     {
-        if (g_connman) {
-            g_connman->SetNetworkActive(active);
+        if (m_context.connman) {
+            m_context.connman->SetNetworkActive(active);
         }
     }
-    bool getNetworkActive() override { return g_connman && g_connman->GetNetworkActive(); }
+    bool getNetworkActive() override { return m_context.connman && m_context.connman->GetNetworkActive(); }
     CFeeRate estimateSmartFee(int num_blocks, bool conservative, int* returned_target = nullptr) override
     {
         FeeCalculation fee_calc;
@@ -255,12 +259,12 @@ public:
     }
     std::unique_ptr<Wallet> loadWallet(const std::string& name, std::string& error, std::vector<std::string>& warnings) override
     {
-        return MakeWallet(LoadWallet(*m_interfaces.chain, name, error, warnings));
+        return MakeWallet(LoadWallet(*m_context.chain, name, error, warnings));
     }
     WalletCreationStatus createWallet(const SecureString& passphrase, uint64_t wallet_creation_flags, const std::string& name, std::string& error, std::vector<std::string>& warnings, std::unique_ptr<Wallet>& result) override
     {
         std::shared_ptr<CWallet> wallet;
-        WalletCreationStatus status = CreateWallet(*m_interfaces.chain, passphrase, wallet_creation_flags, name, error, warnings, wallet);
+        WalletCreationStatus status = CreateWallet(*m_context.chain, passphrase, wallet_creation_flags, name, error, warnings, wallet);
         result = MakeWallet(wallet);
         return status;
     }
@@ -315,7 +319,8 @@ public:
                     /* verification progress is unused when a header was received */ 0);
             }));
     }
-    InitInterfaces m_interfaces;
+    NodeContext* context() override { return &m_context; }
+    NodeContext m_context;
 };
 
 } // namespace
diff --git a/src/interfaces/node.h b/src/interfaces/node.h
index 4ee467014c..c29037f2e3 100644
--- a/src/interfaces/node.h
+++ b/src/interfaces/node.h
@@ -28,6 +28,7 @@ class RPCTimerInterface;
 class UniValue;
 class proxyType;
 struct CNodeStateStats;
+struct NodeContext;
 enum class WalletCreationStatus;
 
 namespace interfaces {
@@ -254,6 +255,9 @@ public:
     using NotifyHeaderTipFn =
         std::function<void(bool initial_download, int height, int64_t block_time, double verification_progress)>;
     virtual std::unique_ptr<Handler> handleNotifyHeaderTip(NotifyHeaderTipFn fn) = 0;
+
+    //! Return pointer to internal chain interface, useful for testing.
+    virtual NodeContext* context() { return nullptr; }
 };
 
 //! Return implementation of Node interface.
diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp
index 6bccde1b8f..b6ede08b14 100644
--- a/src/interfaces/wallet.cpp
+++ b/src/interfaces/wallet.cpp
@@ -496,7 +496,11 @@ public:
         : m_chain(chain), m_wallet_filenames(std::move(wallet_filenames))
     {
     }
-    void registerRpcs() override { return RegisterWalletRPCCommands(m_chain, m_rpc_handlers); }
+    void registerRpcs() override
+    {
+        g_rpc_chain = &m_chain;
+        return RegisterWalletRPCCommands(m_chain, m_rpc_handlers);
+    }
     bool verify() override { return VerifyWallets(m_chain, m_wallet_filenames); }
     bool load() override { return LoadWallets(m_chain, m_wallet_filenames); }
     void start(CScheduler& scheduler) override { return StartWallets(scheduler); }