test: Remove duplicate NodeContext hacks

Qt tests currently are currently using two NodeContext structs at the same time, one in interfaces::NodeImpl::m_context, and the other in BasicTestingSetup::m_node, and the tests have hacks transferring state between them. Fix this by getting rid of the NodeImpl::m_context struct and making it a pointer. This way a common BitcoinApplication object can be used for all qt tests, but they can still have their own testing setups. Non-test code is changing but non-test behavior is still the same as before. Benefit of this PR is to being able to remove the existing "std::move(test.m_node.connman)" and mempool hacks. Motivation for this PR is to let qt wallet tests continue working when the ::vpwallets global variable is removed bitcoin#19101 and not have to add even more complicated hacks exposing duplicate WalletContext and WalletClient instances in addition to the duplicate NodeContext instances and having to manipulate them in the test setup.
ryanofsky · May 28, 2020 · 8569817 · 8569817
1 parent 55b4c65
commit 8569817
Show file tree

Hide file tree

Showing 7 changed files with 60 additions and 37 deletions.
diff --git a/src/interfaces/node.cpp b/src/interfaces/node.cpp
@@ -56,6 +56,7 @@ namespace {
 class NodeImpl : public Node
 {
 public:
+    NodeImpl(NodeContext* context) { setContext(context); }
     void initError(const std::string& message) override { InitError(Untranslated(message)); }
     bool parseParameters(int argc, const char* const argv[], std::string& error) override
     {
@@ -80,13 +81,13 @@ class NodeImpl : public Node
     }
     bool appInitMain() override
     {
-        m_context.chain = MakeChain(m_context);
-        return AppInitMain(m_context_ref, m_context);
+        m_context->chain = MakeChain(*m_context);
+        return AppInitMain(m_context_ref, *m_context);
     }
     void appShutdown() override
     {
-        Interrupt(m_context);
-        Shutdown(m_context);
+        Interrupt(*m_context);
+        Shutdown(*m_context);
     }
     void startShutdown() override { StartShutdown(); }
     bool shutdownRequested() override { return ShutdownRequested(); }
@@ -99,19 +100,19 @@ class NodeImpl : public Node
             StopMapPort();
         }
     }
-    void setupServerArgs() override { return SetupServerArgs(m_context); }
+    void setupServerArgs() override { return SetupServerArgs(*m_context); }
     bool getProxy(Network net, proxyType& proxy_info) override { return GetProxy(net, proxy_info); }
     size_t getNodeCount(CConnman::NumConnections flags) override
     {
-        return m_context.connman ? m_context.connman->GetNodeCount(flags) : 0;
+        return m_context->connman ? m_context->connman->GetNodeCount(flags) : 0;
     }
     bool getNodesStats(NodesStats& stats) override
     {
         stats.clear();
 
-        if (m_context.connman) {
+        if (m_context->connman) {
             std::vector<CNodeStats> stats_temp;
-            m_context.connman->GetNodeStats(stats_temp);
+            m_context->connman->GetNodeStats(stats_temp);
 
             stats.reserve(stats_temp.size());
             for (auto& node_stats_temp : stats_temp) {
@@ -132,46 +133,46 @@ class NodeImpl : public Node
     }
     bool getBanned(banmap_t& banmap) override
     {
-        if (m_context.banman) {
-            m_context.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 (m_context.banman) {
-            m_context.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 (m_context.banman) {
-            m_context.banman->Unban(ip);
+        if (m_context->banman) {
+            m_context->banman->Unban(ip);
             return true;
         }
         return false;
     }
     bool disconnectByAddress(const CNetAddr& net_addr) override
     {
-        if (m_context.connman) {
-            return m_context.connman->DisconnectNode(net_addr);
+        if (m_context->connman) {
+            return m_context->connman->DisconnectNode(net_addr);
         }
         return false;
     }
     bool disconnectById(NodeId id) override
     {
-        if (m_context.connman) {
-            return m_context.connman->DisconnectNode(id);
+        if (m_context->connman) {
+            return m_context->connman->DisconnectNode(id);
         }
         return false;
     }
-    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 m_context.mempool ? m_context.mempool->size() : 0; }
-    size_t getMempoolDynamicUsage() override { return m_context.mempool ? m_context.mempool->DynamicMemoryUsage() : 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 m_context->mempool ? m_context->mempool->size() : 0; }
+    size_t getMempoolDynamicUsage() override { return m_context->mempool ? m_context->mempool->DynamicMemoryUsage() : 0; }
     bool getHeaderTip(int& height, int64_t& block_time) override
     {
         LOCK(::cs_main);
@@ -209,11 +210,11 @@ class NodeImpl : public Node
     bool getImporting() override { return ::fImporting; }
     void setNetworkActive(bool active) override
     {
-        if (m_context.connman) {
-            m_context.connman->SetNetworkActive(active);
+        if (m_context->connman) {
+            m_context->connman->SetNetworkActive(active);
         }
     }
-    bool getNetworkActive() override { return m_context.connman && m_context.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,20 +256,20 @@ class NodeImpl : public Node
     std::vector<std::unique_ptr<Wallet>> getWallets() override
     {
         std::vector<std::unique_ptr<Wallet>> wallets;
-        for (auto& client : m_context.chain_clients) {
+        for (auto& client : m_context->chain_clients) {
             auto client_wallets = client->getWallets();
             std::move(client_wallets.begin(), client_wallets.end(), std::back_inserter(wallets));
         }
         return wallets;
     }
     std::unique_ptr<Wallet> loadWallet(const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings) override
     {
-        return MakeWallet(LoadWallet(*m_context.chain, name, error, warnings));
+        return MakeWallet(LoadWallet(*m_context->chain, name, error, warnings));
     }
     std::unique_ptr<Wallet> createWallet(const SecureString& passphrase, uint64_t wallet_creation_flags, const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings, WalletCreationStatus& status) override
     {
         std::shared_ptr<CWallet> wallet;
-        status = CreateWallet(*m_context.chain, passphrase, wallet_creation_flags, name, error, warnings, wallet);
+        status = CreateWallet(*m_context->chain, passphrase, wallet_creation_flags, name, error, warnings, wallet);
         return MakeWallet(wallet);
     }
     std::unique_ptr<Handler> handleInitMessage(InitMessageFn fn) override
@@ -322,13 +323,22 @@ class NodeImpl : public Node
                     /* verification progress is unused when a header was received */ 0);
             }));
     }
-    NodeContext* context() override { return &m_context; }
-    NodeContext m_context;
-    util::Ref m_context_ref{m_context};
+    NodeContext* context() override { return m_context; }
+    void setContext(NodeContext* context) override
+    {
+        m_context = context;
+        if (context) {
+            m_context_ref.Set(*context);
+        } else {
+            m_context_ref.Clear();
+        }
+    }
+    NodeContext* m_context{nullptr};
+    util::Ref m_context_ref;
 };
 
 } // namespace
 
-std::unique_ptr<Node> MakeNode() { return MakeUnique<NodeImpl>(); }
+std::unique_ptr<Node> MakeNode(NodeContext* context) { return MakeUnique<NodeImpl>(context); }
 
 } // namespace interfaces
diff --git a/src/interfaces/node.h b/src/interfaces/node.h
@@ -258,12 +258,14 @@ class Node
         std::function<void(SynchronizationState, 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.
+    //! Get and set internal node context. Useful for testing, but not
+    //! accessible across processes.
     virtual NodeContext* context() { return nullptr; }
+    virtual void setContext(NodeContext* context) { }
 };
 
 //! Return implementation of Node interface.
-std::unique_ptr<Node> MakeNode();
+std::unique_ptr<Node> MakeNode(NodeContext* context = nullptr);
 
 } // namespace interfaces
 

diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp
@@ -29,6 +29,7 @@
 
 #include <interfaces/handler.h>
 #include <interfaces/node.h>
+#include <node/context.h>
 #include <noui.h>
 #include <ui_interface.h>
 #include <uint256.h>
@@ -414,7 +415,8 @@ int GuiMain(int argc, char* argv[])
     SetupEnvironment();
     util::ThreadSetInternalName("main");
 
-    std::unique_ptr<interfaces::Node> node = interfaces::MakeNode();
+    NodeContext node_context;
+    std::unique_ptr<interfaces::Node> node = interfaces::MakeNode(&node_context);
 
     // Subscribe to global signals from core
     std::unique_ptr<interfaces::Handler> handler_message_box = node->handleMessageBox(noui_ThreadSafeMessageBox);

diff --git a/src/qt/bitcoin.h b/src/qt/bitcoin.h
@@ -90,6 +90,8 @@ class BitcoinApplication: public QApplication
     /// Setup platform style
     void setupPlatformStyle();
 
+    interfaces::Node& node() { return m_node; }
+
 public Q_SLOTS:
     void initializeResult(bool success);
     void shutdownResult();

diff --git a/src/qt/test/addressbooktests.cpp b/src/qt/test/addressbooktests.cpp
@@ -18,6 +18,7 @@
 #include <key.h>
 #include <key_io.h>
 #include <wallet/wallet.h>
+#include <walletinitinterface.h>
 
 #include <QApplication>
 #include <QTimer>
@@ -59,6 +60,8 @@ void EditAddressAndSubmit(
 void TestAddAddressesToSendBook(interfaces::Node& node)
 {
     TestChain100Setup test;
+    g_wallet_init_interface.Construct(test.m_node);
+    node.setContext(&test.m_node);
     std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(node.context()->chain.get(), WalletLocation(), WalletDatabase::CreateMock());
     wallet->SetupLegacyScriptPubKeyMan();
     bool firstRun;

diff --git a/src/qt/test/apptests.cpp b/src/qt/test/apptests.cpp
@@ -6,6 +6,7 @@
 
 #include <chainparams.h>
 #include <key.h>
+#include <interfaces/node.h>
 #include <qt/bitcoin.h>
 #include <qt/bitcoingui.h>
 #include <qt/networkstyle.h>
@@ -63,6 +64,7 @@ void AppTests::appTests()
 #endif
 
     BasicTestingSetup test{CBaseChainParams::REGTEST}; // Create a temp data directory to backup the gui settings to
+    m_app.node().setContext(&test.m_node);
     ECC_Stop(); // Already started by the common test setup, so stop it to avoid interference
     LogInstance().DisconnectTestLogger();
 

diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp
@@ -21,6 +21,7 @@
 #include <test/util/setup_common.h>
 #include <validation.h>
 #include <wallet/wallet.h>
+#include <walletinitinterface.h>
 #include <qt/overviewpage.h>
 #include <qt/receivecoinsdialog.h>
 #include <qt/recentrequeststablemodel.h>
@@ -138,8 +139,9 @@ void TestGUI(interfaces::Node& node)
     for (int i = 0; i < 5; ++i) {
         test.CreateAndProcessBlock({}, GetScriptForRawPubKey(test.coinbaseKey.GetPubKey()));
     }
-    node.context()->connman = std::move(test.m_node.connman);
-    node.context()->mempool = std::move(test.m_node.mempool);
+    test.m_node.chain = interfaces::MakeChain(test.m_node);
+    g_wallet_init_interface.Construct(test.m_node);
+    node.setContext(&test.m_node);
     std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(node.context()->chain.get(), WalletLocation(), WalletDatabase::CreateMock());
     bool firstRun;
     wallet->LoadWallet(firstRun);