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

362ded410b Avoid using g_rpc_node global in wallet code (Russell Yanofsky)
8922d7f6b7 scripted-diff: Remove g_connman, g_banman globals (Russell Yanofsky)
e6f4f895d5 Pass NodeContext, ConnMan, BanMan references more places (Russell Yanofsky)
4d5448c76b MOVEONLY: Move NodeContext struct to node/context.h (Russell Yanofsky)
301bd41a2e 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 362ded410b

Tree-SHA512: 9ae6ff1e33423291d1e52056bac95e0874538390892a6e83c4c115b3c73155a8827c0191b46eb3d14e3b3f6c23ccb08095490880fbc3188026319c71739f7db2

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

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.

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

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.

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