test: Remove duplicate NodeContext hacks

ryanofsky · ryanofsky · commit edc316020e82 · 2020-07-13T04:34:27.000-04:00
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.

Motivation for this PR is to be able to remove the
"std::move(test.m_node.connman)" and mempool hacks for swapping individual
NodeContext members in Qt tests, because followup PR #19099 adds yet another
member (wallet_client) that needs to be swapped. After this change, the whole
NodeContext struct can be swapped instead of individual members, so the
workarounds are less fragile and invasive.
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 bilingual_str& message) override { InitError(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
     {
@@ -107,19 +108,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) {
@@ -140,46 +141,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, int64_t ban_time_offset) override
     {
-        if (m_context.banman) {
-            m_context.banman->Ban(net_addr, ban_time_offset);
+        if (m_context->banman) {
+            m_context->banman->Ban(net_addr, 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);
@@ -222,11 +223,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;
@@ -268,20 +269,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
@@ -335,13 +336,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
@@ -263,12 +263,14 @@ class Node
         std::function<void(SynchronizationState, interfaces::BlockTip tip, 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);
 
 //! Block tip (could be a header or not, depends on the subscribed signal).
 struct BlockTip {
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 <uint256.h>
 #include <util/system.h>
@@ -430,7 +431,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/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,7 @@ void EditAddressAndSubmit(
 void TestAddAddressesToSendBook(interfaces::Node& node)
 {
     TestChain100Setup test;
+    node.setContext(&test.m_node);
     std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(node.context()->chain.get(), WalletLocation(), CreateMockWalletDatabase());
     wallet->SetupLegacyScriptPubKeyMan();
     bool firstRun;
diff --git a/src/qt/test/test_main.cpp b/src/qt/test/test_main.cpp
@@ -52,7 +52,8 @@ int main(int argc, char* argv[])
         BasicTestingSetup dummy{CBaseChainParams::REGTEST};
     }
 
-    std::unique_ptr<interfaces::Node> node = interfaces::MakeNode();
+    NodeContext node_context;
+    std::unique_ptr<interfaces::Node> node = interfaces::MakeNode(&node_context);
 
     bool fInvalid = false;
 
diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp
@@ -138,8 +138,7 @@ 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);
+    node.setContext(&test.m_node);
     std::shared_ptr<CWallet> wallet = std::make_shared<CWallet>(node.context()->chain.get(), WalletLocation(), CreateMockWalletDatabase());
     bool firstRun;
     wallet->LoadWallet(firstRun);
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
@@ -11,6 +11,7 @@
 #include <consensus/validation.h>
 #include <crypto/sha256.h>
 #include <init.h>
+#include <interfaces/chain.h>
 #include <miner.h>
 #include <net.h>
 #include <net_processing.h>
@@ -32,6 +33,7 @@
 #include <util/vector.h>
 #include <validation.h>
 #include <validationinterface.h>
+#include <walletinitinterface.h>
 
 #include <functional>
 
@@ -104,6 +106,8 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::ve
     SetupNetworking();
     InitSignatureCache();
     InitScriptExecutionCache();
+    m_node.chain = interfaces::MakeChain(m_node);
+    g_wallet_init_interface.Construct(m_node);
     fCheckBlockIndex = true;
     static bool noui_connected = false;
     if (!noui_connected) {

Original file line number	Diff line number	Diff line change
`@@ -56,6 +56,7 @@ namespace {`
`56`	`56`	`class NodeImpl : public Node`
`57`	`57`	`{`
`58`	`58`	`public:`
	`59`	`+ NodeImpl(NodeContext* context) { setContext(context); }`
`59`	`60`	`void initError(const bilingual_str& message) override { InitError(message); }`
`60`	`61`	`bool parseParameters(int argc, const char* const argv[], std::string& error) override`
`61`	`62`	`{`
`@@ -80,13 +81,13 @@ class NodeImpl : public Node`
`80`	`81`	`}`
`81`	`82`	`bool appInitMain() override`
`82`	`83`	`{`
`83`		`- m_context.chain = MakeChain(m_context);`
`84`		`- return AppInitMain(m_context_ref, m_context);`
	`84`	`+ m_context->chain = MakeChain(*m_context);`
	`85`	`+ return AppInitMain(m_context_ref, *m_context);`
`85`	`86`	`}`
`86`	`87`	`void appShutdown() override`
`87`	`88`	`{`
`88`		`- Interrupt(m_context);`
`89`		`- Shutdown(m_context);`
	`89`	`+ Interrupt(*m_context);`
	`90`	`+ Shutdown(*m_context);`
`90`	`91`	`}`
`91`	`92`	`void startShutdown() override`
`92`	`93`	`{`
`@@ -107,19 +108,19 @@ class NodeImpl : public Node`
`107`	`108`	`StopMapPort();`
`108`	`109`	`}`
`109`	`110`	`}`
`110`		`- void setupServerArgs() override { return SetupServerArgs(m_context); }`
	`111`	`+ void setupServerArgs() override { return SetupServerArgs(*m_context); }`
`111`	`112`	`bool getProxy(Network net, proxyType& proxy_info) override { return GetProxy(net, proxy_info); }`
`112`	`113`	`size_t getNodeCount(CConnman::NumConnections flags) override`
`113`	`114`	`{`
`114`		`- return m_context.connman ? m_context.connman->GetNodeCount(flags) : 0;`
	`115`	`+ return m_context->connman ? m_context->connman->GetNodeCount(flags) : 0;`
`115`	`116`	`}`
`116`	`117`	`bool getNodesStats(NodesStats& stats) override`
`117`	`118`	`{`
`118`	`119`	`stats.clear();`
`119`	`120`
`120`		`- if (m_context.connman) {`
	`121`	`+ if (m_context->connman) {`
`121`	`122`	`std::vector<CNodeStats> stats_temp;`
`122`		`- m_context.connman->GetNodeStats(stats_temp);`
	`123`	`+ m_context->connman->GetNodeStats(stats_temp);`
`123`	`124`
`124`	`125`	`stats.reserve(stats_temp.size());`
`125`	`126`	`for (auto& node_stats_temp : stats_temp) {`
`@@ -140,46 +141,46 @@ class NodeImpl : public Node`
`140`	`141`	`}`
`141`	`142`	`bool getBanned(banmap_t& banmap) override`
`142`	`143`	`{`
`143`		`- if (m_context.banman) {`
`144`		`- m_context.banman->GetBanned(banmap);`
	`144`	`+ if (m_context->banman) {`
	`145`	`+ m_context->banman->GetBanned(banmap);`
`145`	`146`	`return true;`
`146`	`147`	`}`
`147`	`148`	`return false;`
`148`	`149`	`}`
`149`	`150`	`bool ban(const CNetAddr& net_addr, int64_t ban_time_offset) override`
`150`	`151`	`{`
`151`		`- if (m_context.banman) {`
`152`		`- m_context.banman->Ban(net_addr, ban_time_offset);`
	`152`	`+ if (m_context->banman) {`
	`153`	`+ m_context->banman->Ban(net_addr, ban_time_offset);`
`153`	`154`	`return true;`
`154`	`155`	`}`
`155`	`156`	`return false;`
`156`	`157`	`}`
`157`	`158`	`bool unban(const CSubNet& ip) override`
`158`	`159`	`{`
`159`		`- if (m_context.banman) {`
`160`		`- m_context.banman->Unban(ip);`
	`160`	`+ if (m_context->banman) {`
	`161`	`+ m_context->banman->Unban(ip);`
`161`	`162`	`return true;`
`162`	`163`	`}`
`163`	`164`	`return false;`
`164`	`165`	`}`
`165`	`166`	`bool disconnectByAddress(const CNetAddr& net_addr) override`
`166`	`167`	`{`
`167`		`- if (m_context.connman) {`
`168`		`- return m_context.connman->DisconnectNode(net_addr);`
	`168`	`+ if (m_context->connman) {`
	`169`	`+ return m_context->connman->DisconnectNode(net_addr);`
`169`	`170`	`}`
`170`	`171`	`return false;`
`171`	`172`	`}`
`172`	`173`	`bool disconnectById(NodeId id) override`
`173`	`174`	`{`
`174`		`- if (m_context.connman) {`
`175`		`- return m_context.connman->DisconnectNode(id);`
	`175`	`+ if (m_context->connman) {`
	`176`	`+ return m_context->connman->DisconnectNode(id);`
`176`	`177`	`}`
`177`	`178`	`return false;`
`178`	`179`	`}`
`179`		`- int64_t getTotalBytesRecv() override { return m_context.connman ? m_context.connman->GetTotalBytesRecv() : 0; }`
`180`		`- int64_t getTotalBytesSent() override { return m_context.connman ? m_context.connman->GetTotalBytesSent() : 0; }`
`181`		`- size_t getMempoolSize() override { return m_context.mempool ? m_context.mempool->size() : 0; }`
`182`		`- size_t getMempoolDynamicUsage() override { return m_context.mempool ? m_context.mempool->DynamicMemoryUsage() : 0; }`
	`180`	`+ int64_t getTotalBytesRecv() override { return m_context->connman ? m_context->connman->GetTotalBytesRecv() : 0; }`
	`181`	`+ int64_t getTotalBytesSent() override { return m_context->connman ? m_context->connman->GetTotalBytesSent() : 0; }`
	`182`	`+ size_t getMempoolSize() override { return m_context->mempool ? m_context->mempool->size() : 0; }`
	`183`	`+ size_t getMempoolDynamicUsage() override { return m_context->mempool ? m_context->mempool->DynamicMemoryUsage() : 0; }`
`183`	`184`	`bool getHeaderTip(int& height, int64_t& block_time) override`
`184`	`185`	`{`
`185`	`186`	`LOCK(::cs_main);`
`@@ -222,11 +223,11 @@ class NodeImpl : public Node`
`222`	`223`	`bool getImporting() override { return ::fImporting; }`
`223`	`224`	`void setNetworkActive(bool active) override`
`224`	`225`	`{`
`225`		`- if (m_context.connman) {`
`226`		`- m_context.connman->SetNetworkActive(active);`
	`226`	`+ if (m_context->connman) {`
	`227`	`+ m_context->connman->SetNetworkActive(active);`
`227`	`228`	`}`
`228`	`229`	`}`
`229`		`- bool getNetworkActive() override { return m_context.connman && m_context.connman->GetNetworkActive(); }`
	`230`	`+ bool getNetworkActive() override { return m_context->connman && m_context->connman->GetNetworkActive(); }`
`230`	`231`	`CFeeRate estimateSmartFee(int num_blocks, bool conservative, int* returned_target = nullptr) override`
`231`	`232`	`{`
`232`	`233`	`FeeCalculation fee_calc;`
`@@ -268,20 +269,20 @@ class NodeImpl : public Node`
`268`	`269`	`std::vector<std::unique_ptr<Wallet>> getWallets() override`
`269`	`270`	`{`
`270`	`271`	`std::vector<std::unique_ptr<Wallet>> wallets;`
`271`		`- for (auto& client : m_context.chain_clients) {`
	`272`	`+ for (auto& client : m_context->chain_clients) {`
`272`	`273`	`auto client_wallets = client->getWallets();`
`273`	`274`	`std::move(client_wallets.begin(), client_wallets.end(), std::back_inserter(wallets));`
`274`	`275`	`}`
`275`	`276`	`return wallets;`
`276`	`277`	`}`
`277`	`278`	`std::unique_ptr<Wallet> loadWallet(const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings) override`
`278`	`279`	`{`
`279`		`- return MakeWallet(LoadWallet(*m_context.chain, name, error, warnings));`
	`280`	`+ return MakeWallet(LoadWallet(*m_context->chain, name, error, warnings));`
`280`	`281`	`}`
`281`	`282`	`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`
`282`	`283`	`{`
`283`	`284`	`std::shared_ptr<CWallet> wallet;`
`284`		`- status = CreateWallet(*m_context.chain, passphrase, wallet_creation_flags, name, error, warnings, wallet);`
	`285`	`+ status = CreateWallet(*m_context->chain, passphrase, wallet_creation_flags, name, error, warnings, wallet);`
`285`	`286`	`return MakeWallet(wallet);`
`286`	`287`	`}`
`287`	`288`	`std::unique_ptr<Handler> handleInitMessage(InitMessageFn fn) override`
`@@ -335,13 +336,22 @@ class NodeImpl : public Node`
`335`	`336`	`/* verification progress is unused when a header was received */ 0);`
`336`	`337`	`}));`
`337`	`338`	`}`
`338`		`- NodeContext* context() override { return &m_context; }`
`339`		`- NodeContext m_context;`
`340`		`- util::Ref m_context_ref{m_context};`
	`339`	`+ NodeContext* context() override { return m_context; }`
	`340`	`+ void setContext(NodeContext* context) override`
	`341`	`+ {`
	`342`	`+ m_context = context;`
	`343`	`+ if (context) {`
	`344`	`+ m_context_ref.Set(*context);`
	`345`	`+ } else {`
	`346`	`+ m_context_ref.Clear();`
	`347`	`+ }`
	`348`	`+ }`
	`349`	`+ NodeContext* m_context{nullptr};`
	`350`	`+ util::Ref m_context_ref;`
`341`	`351`	`};`
`342`	`352`
`343`	`353`	`} // namespace`
`344`	`354`
`345`		`-std::unique_ptr<Node> MakeNode() { return MakeUnique<NodeImpl>(); }`
	`355`	`+std::unique_ptr<Node> MakeNode(NodeContext* context) { return MakeUnique<NodeImpl>(context); }`
`346`	`356`
`347`	`357`	`} // namespace interfaces`
Original file line number	Diff line number	Diff line change
`@@ -52,7 +52,8 @@ int main(int argc, char* argv[])`
`52`	`52`	`BasicTestingSetup dummy{CBaseChainParams::REGTEST};`
`53`	`53`	`}`
`54`	`54`
`55`		`- std::unique_ptr<interfaces::Node> node = interfaces::MakeNode();`
	`55`	`+ NodeContext node_context;`
	`56`	`+ std::unique_ptr<interfaces::Node> node = interfaces::MakeNode(&node_context);`
`56`	`57`
`57`	`58`	`bool fInvalid = false;`
`58`	`59`