Merge #13615: net: Remove unused interrupt from SendMessages

laanwj · laanwj · commit 172f984f598f · 2018-07-09T16:50:09.000+02:00
4b6ab02 Remove unused argument to ProcessGetBlockData(...) (practicalswift) c469ecf net: Remove unused interrupt from SendMessages (fanquake) Pull request description: Discussed very briefly with cfields. Includes 65b4400 from #13554 as it's a similar refactor. Tree-SHA512: 45cd64208a5c8164242db74e6687e9344ea592bab5e7f9ba8e1bb449057fc908ec9d8b8523748a68426e4a4304e3388a138cd834698b39837b2149b72beefdc9
diff --git a/src/net.cpp b/src/net.cpp
@@ -2042,7 +2042,7 @@ void CConnman::ThreadMessageHandler()
             // Send messages
             {
                 LOCK(pnode->cs_sendProcessing);
-                m_msgproc->SendMessages(pnode, flagInterruptMsgProc);
+                m_msgproc->SendMessages(pnode);
             }
 
             if (flagInterruptMsgProc)
diff --git a/src/net.h b/src/net.h
@@ -466,7 +466,7 @@ class NetEventsInterface
 {
 public:
     virtual bool ProcessMessages(CNode* pnode, std::atomic<bool>& interrupt) = 0;
-    virtual bool SendMessages(CNode* pnode, std::atomic<bool>& interrupt) = 0;
+    virtual bool SendMessages(CNode* pnode) = 0;
     virtual void InitializeNode(CNode* pnode) = 0;
     virtual void FinalizeNode(NodeId id, bool& update_connection_time) = 0;
 
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
@@ -1070,7 +1070,7 @@ static void RelayAddress(const CAddress& addr, bool fReachable, CConnman* connma
     connman->ForEachNodeThen(std::move(sortfunc), std::move(pushfunc));
 }
 
-void static ProcessGetBlockData(CNode* pfrom, const CChainParams& chainparams, const CInv& inv, CConnman* connman, const std::atomic<bool>& interruptMsgProc)
+void static ProcessGetBlockData(CNode* pfrom, const CChainParams& chainparams, const CInv& inv, CConnman* connman)
 {
     bool send = false;
     std::shared_ptr<const CBlock> a_recent_block;
@@ -1274,7 +1274,7 @@ void static ProcessGetData(CNode* pfrom, const CChainParams& chainparams, CConnm
         const CInv &inv = *it;
         if (inv.type == MSG_BLOCK || inv.type == MSG_FILTERED_BLOCK || inv.type == MSG_CMPCT_BLOCK || inv.type == MSG_WITNESS_BLOCK) {
             it++;
-            ProcessGetBlockData(pfrom, chainparams, inv, connman, interruptMsgProc);
+            ProcessGetBlockData(pfrom, chainparams, inv, connman);
         }
     }
 
@@ -3218,7 +3218,7 @@ class CompareInvMempoolOrder
     }
 };
 
-bool PeerLogicValidation::SendMessages(CNode* pto, std::atomic<bool>& interruptMsgProc)
+bool PeerLogicValidation::SendMessages(CNode* pto)
 {
     const Consensus::Params& consensusParams = Params().GetConsensus();
     {
diff --git a/src/net_processing.h b/src/net_processing.h
@@ -68,16 +68,20 @@ class PeerLogicValidation final : public CValidationInterface, public NetEventsI
     void InitializeNode(CNode* pnode) override;
     /** Handle removal of a peer by updating various state and removing it from mapNodeState */
     void FinalizeNode(NodeId nodeid, bool& fUpdateConnectionTime) override;
-    /** Process protocol messages received from a given node */
+    /**
+    * Process protocol messages received from a given node
+    *
+    * @param[in]   pfrom           The node which we have received messages from.
+    * @param[in]   interrupt       Interrupt condition for processing threads
+    */
     bool ProcessMessages(CNode* pfrom, std::atomic<bool>& interrupt) override;
     /**
     * Send queued protocol messages to be sent to a give node.
     *
     * @param[in]   pto             The node which we are sending messages to.
-    * @param[in]   interrupt       Interrupt condition for processing threads
     * @return                      True if there is more work to be done
     */
-    bool SendMessages(CNode* pto, std::atomic<bool>& interrupt) override EXCLUSIVE_LOCKS_REQUIRED(pto->cs_sendProcessing);
+    bool SendMessages(CNode* pto) override EXCLUSIVE_LOCKS_REQUIRED(pto->cs_sendProcessing);
 
     /** Consider evicting an outbound peer based on the amount of time they've been behind our tip */
     void ConsiderEviction(CNode *pto, int64_t time_in_seconds);
diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp
@@ -54,7 +54,6 @@ BOOST_FIXTURE_TEST_SUITE(denialofservice_tests, TestingSetup)
 // work.
 BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
 {
-    std::atomic<bool> interruptDummy(false);
 
     // Mock an outbound peer
     CAddress addr1(ip(0xa0b0c001), NODE_NONE);
@@ -75,7 +74,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
     // Test starts here
     {
         LOCK2(cs_main, dummyNode1.cs_sendProcessing);
-        peerLogic->SendMessages(&dummyNode1, interruptDummy); // should result in getheaders
+        peerLogic->SendMessages(&dummyNode1); // should result in getheaders
     }
     {
         LOCK2(cs_main, dummyNode1.cs_vSend);
@@ -88,7 +87,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
     SetMockTime(nStartTime+21*60);
     {
         LOCK2(cs_main, dummyNode1.cs_sendProcessing);
-        peerLogic->SendMessages(&dummyNode1, interruptDummy); // should result in getheaders
+        peerLogic->SendMessages(&dummyNode1); // should result in getheaders
     }
     {
         LOCK2(cs_main, dummyNode1.cs_vSend);
@@ -98,7 +97,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
     SetMockTime(nStartTime+24*60);
     {
         LOCK2(cs_main, dummyNode1.cs_sendProcessing);
-        peerLogic->SendMessages(&dummyNode1, interruptDummy); // should result in disconnect
+        peerLogic->SendMessages(&dummyNode1); // should result in disconnect
     }
     BOOST_CHECK(dummyNode1.fDisconnect == true);
     SetMockTime(0);
@@ -192,7 +191,6 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management)
 
 BOOST_AUTO_TEST_CASE(DoS_banning)
 {
-    std::atomic<bool> interruptDummy(false);
 
     connman->ClearBanned();
     CAddress addr1(ip(0xa0b0c001), NODE_NONE);
@@ -207,7 +205,7 @@ BOOST_AUTO_TEST_CASE(DoS_banning)
     }
     {
         LOCK2(cs_main, dummyNode1.cs_sendProcessing);
-        peerLogic->SendMessages(&dummyNode1, interruptDummy);
+        peerLogic->SendMessages(&dummyNode1);
     }
     BOOST_CHECK(connman->IsBanned(addr1));
     BOOST_CHECK(!connman->IsBanned(ip(0xa0b0c001|0x0000ff00))); // Different IP, not banned
@@ -224,7 +222,7 @@ BOOST_AUTO_TEST_CASE(DoS_banning)
     }
     {
         LOCK2(cs_main, dummyNode2.cs_sendProcessing);
-        peerLogic->SendMessages(&dummyNode2, interruptDummy);
+        peerLogic->SendMessages(&dummyNode2);
     }
     BOOST_CHECK(!connman->IsBanned(addr2)); // 2 not banned yet...
     BOOST_CHECK(connman->IsBanned(addr1));  // ... but 1 still should be
@@ -234,7 +232,7 @@ BOOST_AUTO_TEST_CASE(DoS_banning)
     }
     {
         LOCK2(cs_main, dummyNode2.cs_sendProcessing);
-        peerLogic->SendMessages(&dummyNode2, interruptDummy);
+        peerLogic->SendMessages(&dummyNode2);
     }
     BOOST_CHECK(connman->IsBanned(addr2));
 
@@ -245,7 +243,6 @@ BOOST_AUTO_TEST_CASE(DoS_banning)
 
 BOOST_AUTO_TEST_CASE(DoS_banscore)
 {
-    std::atomic<bool> interruptDummy(false);
 
     connman->ClearBanned();
     gArgs.ForceSetArg("-banscore", "111"); // because 11 is my favorite number
@@ -261,7 +258,7 @@ BOOST_AUTO_TEST_CASE(DoS_banscore)
     }
     {
         LOCK2(cs_main, dummyNode1.cs_sendProcessing);
-        peerLogic->SendMessages(&dummyNode1, interruptDummy);
+        peerLogic->SendMessages(&dummyNode1);
     }
     BOOST_CHECK(!connman->IsBanned(addr1));
     {
@@ -270,7 +267,7 @@ BOOST_AUTO_TEST_CASE(DoS_banscore)
     }
     {
         LOCK2(cs_main, dummyNode1.cs_sendProcessing);
-        peerLogic->SendMessages(&dummyNode1, interruptDummy);
+        peerLogic->SendMessages(&dummyNode1);
     }
     BOOST_CHECK(!connman->IsBanned(addr1));
     {
@@ -279,7 +276,7 @@ BOOST_AUTO_TEST_CASE(DoS_banscore)
     }
     {
         LOCK2(cs_main, dummyNode1.cs_sendProcessing);
-        peerLogic->SendMessages(&dummyNode1, interruptDummy);
+        peerLogic->SendMessages(&dummyNode1);
     }
     BOOST_CHECK(connman->IsBanned(addr1));
     gArgs.ForceSetArg("-banscore", std::to_string(DEFAULT_BANSCORE_THRESHOLD));
@@ -290,7 +287,6 @@ BOOST_AUTO_TEST_CASE(DoS_banscore)
 
 BOOST_AUTO_TEST_CASE(DoS_bantime)
 {
-    std::atomic<bool> interruptDummy(false);
 
     connman->ClearBanned();
     int64_t nStartTime = GetTime();
@@ -309,7 +305,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)
     }
     {
         LOCK2(cs_main, dummyNode.cs_sendProcessing);
-        peerLogic->SendMessages(&dummyNode, interruptDummy);
+        peerLogic->SendMessages(&dummyNode);
     }
     BOOST_CHECK(connman->IsBanned(addr));
 

Original file line number	Diff line number	Diff line change
`@@ -2042,7 +2042,7 @@ void CConnman::ThreadMessageHandler()`
`2042`	`2042`	`// Send messages`
`2043`	`2043`	`{`
`2044`	`2044`	`LOCK(pnode->cs_sendProcessing);`
`2045`		`- m_msgproc->SendMessages(pnode, flagInterruptMsgProc);`
	`2045`	`+ m_msgproc->SendMessages(pnode);`
`2046`	`2046`	`}`
`2047`	`2047`
`2048`	`2048`	`if (flagInterruptMsgProc)`
Original file line number	Diff line number	Diff line change
`@@ -466,7 +466,7 @@ class NetEventsInterface`
`466`	`466`	`{`
`467`	`467`	`public:`
`468`	`468`	`virtual bool ProcessMessages(CNode* pnode, std::atomic<bool>& interrupt) = 0;`
`469`		`- virtual bool SendMessages(CNode* pnode, std::atomic<bool>& interrupt) = 0;`
	`469`	`+ virtual bool SendMessages(CNode* pnode) = 0;`
`470`	`470`	`virtual void InitializeNode(CNode* pnode) = 0;`
`471`	`471`	`virtual void FinalizeNode(NodeId id, bool& update_connection_time) = 0;`
`472`	`472`
Original file line number	Diff line number	Diff line change
`@@ -1070,7 +1070,7 @@ static void RelayAddress(const CAddress& addr, bool fReachable, CConnman* connma`
`1070`	`1070`	`connman->ForEachNodeThen(std::move(sortfunc), std::move(pushfunc));`
`1071`	`1071`	`}`
`1072`	`1072`
`1073`		`-void static ProcessGetBlockData(CNode* pfrom, const CChainParams& chainparams, const CInv& inv, CConnman* connman, const std::atomic<bool>& interruptMsgProc)`
	`1073`	`+void static ProcessGetBlockData(CNode* pfrom, const CChainParams& chainparams, const CInv& inv, CConnman* connman)`
`1074`	`1074`	`{`
`1075`	`1075`	`bool send = false;`
`1076`	`1076`	`std::shared_ptr<const CBlock> a_recent_block;`
`@@ -1274,7 +1274,7 @@ void static ProcessGetData(CNode* pfrom, const CChainParams& chainparams, CConnm`
`1274`	`1274`	`const CInv &inv = *it;`
`1275`	`1275`	`if (inv.type == MSG_BLOCK \|\| inv.type == MSG_FILTERED_BLOCK \|\| inv.type == MSG_CMPCT_BLOCK \|\| inv.type == MSG_WITNESS_BLOCK) {`
`1276`	`1276`	`it++;`
`1277`		`- ProcessGetBlockData(pfrom, chainparams, inv, connman, interruptMsgProc);`
	`1277`	`+ ProcessGetBlockData(pfrom, chainparams, inv, connman);`
`1278`	`1278`	`}`
`1279`	`1279`	`}`
`1280`	`1280`
`@@ -3218,7 +3218,7 @@ class CompareInvMempoolOrder`
`3218`	`3218`	`}`
`3219`	`3219`	`};`
`3220`	`3220`
`3221`		`-bool PeerLogicValidation::SendMessages(CNode* pto, std::atomic<bool>& interruptMsgProc)`
	`3221`	`+bool PeerLogicValidation::SendMessages(CNode* pto)`
`3222`	`3222`	`{`
`3223`	`3223`	`const Consensus::Params& consensusParams = Params().GetConsensus();`
`3224`	`3224`	`{`
Original file line number	Diff line number	Diff line change
`@@ -54,7 +54,6 @@ BOOST_FIXTURE_TEST_SUITE(denialofservice_tests, TestingSetup)`
`54`	`54`	`// work.`
`55`	`55`	`BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)`
`56`	`56`	`{`
`57`		`- std::atomic<bool> interruptDummy(false);`
`58`	`57`
`59`	`58`	`// Mock an outbound peer`
`60`	`59`	`CAddress addr1(ip(0xa0b0c001), NODE_NONE);`
`@@ -75,7 +74,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)`
`75`	`74`	`// Test starts here`
`76`	`75`	`{`
`77`	`76`	`LOCK2(cs_main, dummyNode1.cs_sendProcessing);`
`78`		`- peerLogic->SendMessages(&dummyNode1, interruptDummy); // should result in getheaders`
	`77`	`+ peerLogic->SendMessages(&dummyNode1); // should result in getheaders`
`79`	`78`	`}`
`80`	`79`	`{`
`81`	`80`	`LOCK2(cs_main, dummyNode1.cs_vSend);`
`@@ -88,7 +87,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)`
`88`	`87`	`SetMockTime(nStartTime+21*60);`
`89`	`88`	`{`
`90`	`89`	`LOCK2(cs_main, dummyNode1.cs_sendProcessing);`
`91`		`- peerLogic->SendMessages(&dummyNode1, interruptDummy); // should result in getheaders`
	`90`	`+ peerLogic->SendMessages(&dummyNode1); // should result in getheaders`
`92`	`91`	`}`
`93`	`92`	`{`
`94`	`93`	`LOCK2(cs_main, dummyNode1.cs_vSend);`
`@@ -98,7 +97,7 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)`
`98`	`97`	`SetMockTime(nStartTime+24*60);`
`99`	`98`	`{`
`100`	`99`	`LOCK2(cs_main, dummyNode1.cs_sendProcessing);`
`101`		`- peerLogic->SendMessages(&dummyNode1, interruptDummy); // should result in disconnect`
	`100`	`+ peerLogic->SendMessages(&dummyNode1); // should result in disconnect`
`102`	`101`	`}`
`103`	`102`	`BOOST_CHECK(dummyNode1.fDisconnect == true);`
`104`	`103`	`SetMockTime(0);`
`@@ -192,7 +191,6 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management)`
`192`	`191`
`193`	`192`	`BOOST_AUTO_TEST_CASE(DoS_banning)`
`194`	`193`	`{`
`195`		`- std::atomic<bool> interruptDummy(false);`
`196`	`194`
`197`	`195`	`connman->ClearBanned();`
`198`	`196`	`CAddress addr1(ip(0xa0b0c001), NODE_NONE);`
`@@ -207,7 +205,7 @@ BOOST_AUTO_TEST_CASE(DoS_banning)`
`207`	`205`	`}`
`208`	`206`	`{`
`209`	`207`	`LOCK2(cs_main, dummyNode1.cs_sendProcessing);`
`210`		`- peerLogic->SendMessages(&dummyNode1, interruptDummy);`
	`208`	`+ peerLogic->SendMessages(&dummyNode1);`
`211`	`209`	`}`
`212`	`210`	`BOOST_CHECK(connman->IsBanned(addr1));`
`213`	`211`	`BOOST_CHECK(!connman->IsBanned(ip(0xa0b0c001\|0x0000ff00))); // Different IP, not banned`
`@@ -224,7 +222,7 @@ BOOST_AUTO_TEST_CASE(DoS_banning)`
`224`	`222`	`}`
`225`	`223`	`{`
`226`	`224`	`LOCK2(cs_main, dummyNode2.cs_sendProcessing);`
`227`		`- peerLogic->SendMessages(&dummyNode2, interruptDummy);`
	`225`	`+ peerLogic->SendMessages(&dummyNode2);`
`228`	`226`	`}`
`229`	`227`	`BOOST_CHECK(!connman->IsBanned(addr2)); // 2 not banned yet...`
`230`	`228`	`BOOST_CHECK(connman->IsBanned(addr1)); // ... but 1 still should be`
`@@ -234,7 +232,7 @@ BOOST_AUTO_TEST_CASE(DoS_banning)`
`234`	`232`	`}`
`235`	`233`	`{`
`236`	`234`	`LOCK2(cs_main, dummyNode2.cs_sendProcessing);`
`237`		`- peerLogic->SendMessages(&dummyNode2, interruptDummy);`
	`235`	`+ peerLogic->SendMessages(&dummyNode2);`
`238`	`236`	`}`
`239`	`237`	`BOOST_CHECK(connman->IsBanned(addr2));`
`240`	`238`
`@@ -245,7 +243,6 @@ BOOST_AUTO_TEST_CASE(DoS_banning)`
`245`	`243`
`246`	`244`	`BOOST_AUTO_TEST_CASE(DoS_banscore)`
`247`	`245`	`{`
`248`		`- std::atomic<bool> interruptDummy(false);`
`249`	`246`
`250`	`247`	`connman->ClearBanned();`
`251`	`248`	`gArgs.ForceSetArg("-banscore", "111"); // because 11 is my favorite number`
`@@ -261,7 +258,7 @@ BOOST_AUTO_TEST_CASE(DoS_banscore)`
`261`	`258`	`}`
`262`	`259`	`{`
`263`	`260`	`LOCK2(cs_main, dummyNode1.cs_sendProcessing);`
`264`		`- peerLogic->SendMessages(&dummyNode1, interruptDummy);`
	`261`	`+ peerLogic->SendMessages(&dummyNode1);`
`265`	`262`	`}`
`266`	`263`	`BOOST_CHECK(!connman->IsBanned(addr1));`
`267`	`264`	`{`
`@@ -270,7 +267,7 @@ BOOST_AUTO_TEST_CASE(DoS_banscore)`
`270`	`267`	`}`
`271`	`268`	`{`
`272`	`269`	`LOCK2(cs_main, dummyNode1.cs_sendProcessing);`
`273`		`- peerLogic->SendMessages(&dummyNode1, interruptDummy);`
	`270`	`+ peerLogic->SendMessages(&dummyNode1);`
`274`	`271`	`}`
`275`	`272`	`BOOST_CHECK(!connman->IsBanned(addr1));`
`276`	`273`	`{`
`@@ -279,7 +276,7 @@ BOOST_AUTO_TEST_CASE(DoS_banscore)`
`279`	`276`	`}`
`280`	`277`	`{`
`281`	`278`	`LOCK2(cs_main, dummyNode1.cs_sendProcessing);`
`282`		`- peerLogic->SendMessages(&dummyNode1, interruptDummy);`
	`279`	`+ peerLogic->SendMessages(&dummyNode1);`
`283`	`280`	`}`
`284`	`281`	`BOOST_CHECK(connman->IsBanned(addr1));`
`285`	`282`	`gArgs.ForceSetArg("-banscore", std::to_string(DEFAULT_BANSCORE_THRESHOLD));`
`@@ -290,7 +287,6 @@ BOOST_AUTO_TEST_CASE(DoS_banscore)`
`290`	`287`
`291`	`288`	`BOOST_AUTO_TEST_CASE(DoS_bantime)`
`292`	`289`	`{`
`293`		`- std::atomic<bool> interruptDummy(false);`
`294`	`290`
`295`	`291`	`connman->ClearBanned();`
`296`	`292`	`int64_t nStartTime = GetTime();`
`@@ -309,7 +305,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)`
`309`	`305`	`}`
`310`	`306`	`{`
`311`	`307`	`LOCK2(cs_main, dummyNode.cs_sendProcessing);`
`312`		`- peerLogic->SendMessages(&dummyNode, interruptDummy);`
	`308`	`+ peerLogic->SendMessages(&dummyNode);`
`313`	`309`	`}`
`314`	`310`	`BOOST_CHECK(connman->IsBanned(addr));`
`315`	`311`