Merge #12882: tests: Make test_bitcoin pass under ThreadSanitzer (clang). Fix lock-order-inversion (potential deadlock).

9fdf05d70c tests: Fix lock-order-inversion (potential deadlock) in DoS_tests. Reported by TSAN. (practicalswift)

Pull request description:

  Fix lock-order-inversion (potential deadlock) in `DoS_tests`. Reported by Clang's TSAN.

  Makes `src/test/test_bitcoin` pass also when compiled with TreadSanitizer (`./configure --with-sanitizers=thread` with `clang`).

Tree-SHA512: 41403bb7b6e26bdf1b830b5699e27c637d522bae1799d2a19ed4b68b21b2555438b42170d8b1189613beb32a69b76a65175d29a83f5f4e493896c3d0d94ae26d

2 files changed, 55 insertions, 23 deletions

diff --git a/src/net_processing.h b/src/net_processing.h
index b0b905d922..3bdb4785a2 100644
--- a/src/net_processing.h
+++ b/src/net_processing.h
@@ -77,7 +77,7 @@ public:
     * @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;
+    bool SendMessages(CNode* pto, std::atomic<bool>& interrupt) 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
index e5f914ba8a..bebbd6c464 100644
--- a/src/test/denialofservice_tests.cpp
+++ b/src/test/denialofservice_tests.cpp
@@ -66,25 +66,40 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
     dummyNode1.fSuccessfullyConnected = true;
 
     // This test requires that we have a chain with non-zero work.
-    LOCK(cs_main);
-    BOOST_CHECK(chainActive.Tip() != nullptr);
-    BOOST_CHECK(chainActive.Tip()->nChainWork > 0);
+    {
+        LOCK(cs_main);
+        BOOST_CHECK(chainActive.Tip() != nullptr);
+        BOOST_CHECK(chainActive.Tip()->nChainWork > 0);
+    }
 
     // Test starts here
-    LOCK(dummyNode1.cs_sendProcessing);
-    peerLogic->SendMessages(&dummyNode1, interruptDummy); // should result in getheaders
-    LOCK(dummyNode1.cs_vSend);
-    BOOST_CHECK(dummyNode1.vSendMsg.size() > 0);
-    dummyNode1.vSendMsg.clear();
+    {
+        LOCK2(cs_main, dummyNode1.cs_sendProcessing);
+        peerLogic->SendMessages(&dummyNode1, interruptDummy); // should result in getheaders
+    }
+    {
+        LOCK2(cs_main, dummyNode1.cs_vSend);
+        BOOST_CHECK(dummyNode1.vSendMsg.size() > 0);
+        dummyNode1.vSendMsg.clear();
+    }
 
     int64_t nStartTime = GetTime();
     // Wait 21 minutes
     SetMockTime(nStartTime+21*60);
-    peerLogic->SendMessages(&dummyNode1, interruptDummy); // should result in getheaders
-    BOOST_CHECK(dummyNode1.vSendMsg.size() > 0);
+    {
+        LOCK2(cs_main, dummyNode1.cs_sendProcessing);
+        peerLogic->SendMessages(&dummyNode1, interruptDummy); // should result in getheaders
+    }
+    {
+        LOCK2(cs_main, dummyNode1.cs_vSend);
+        BOOST_CHECK(dummyNode1.vSendMsg.size() > 0);
+    }
     // Wait 3 more minutes
     SetMockTime(nStartTime+24*60);
-    peerLogic->SendMessages(&dummyNode1, interruptDummy); // should result in disconnect
+    {
+        LOCK2(cs_main, dummyNode1.cs_sendProcessing);
+        peerLogic->SendMessages(&dummyNode1, interruptDummy); // should result in disconnect
+    }
     BOOST_CHECK(dummyNode1.fDisconnect == true);
     SetMockTime(0);
 
@@ -190,8 +205,10 @@ BOOST_AUTO_TEST_CASE(DoS_banning)
         LOCK(cs_main);
         Misbehaving(dummyNode1.GetId(), 100); // Should get banned
     }
-    LOCK(dummyNode1.cs_sendProcessing);
-    peerLogic->SendMessages(&dummyNode1, interruptDummy);
+    {
+        LOCK2(cs_main, dummyNode1.cs_sendProcessing);
+        peerLogic->SendMessages(&dummyNode1, interruptDummy);
+    }
     BOOST_CHECK(connman->IsBanned(addr1));
     BOOST_CHECK(!connman->IsBanned(ip(0xa0b0c001|0x0000ff00))); // Different IP, not banned
 
@@ -205,15 +222,20 @@ BOOST_AUTO_TEST_CASE(DoS_banning)
         LOCK(cs_main);
         Misbehaving(dummyNode2.GetId(), 50);
     }
-    LOCK(dummyNode2.cs_sendProcessing);
-    peerLogic->SendMessages(&dummyNode2, interruptDummy);
+    {
+        LOCK2(cs_main, dummyNode2.cs_sendProcessing);
+        peerLogic->SendMessages(&dummyNode2, interruptDummy);
+    }
     BOOST_CHECK(!connman->IsBanned(addr2)); // 2 not banned yet...
     BOOST_CHECK(connman->IsBanned(addr1));  // ... but 1 still should be
     {
         LOCK(cs_main);
         Misbehaving(dummyNode2.GetId(), 50);
     }
-    peerLogic->SendMessages(&dummyNode2, interruptDummy);
+    {
+        LOCK2(cs_main, dummyNode2.cs_sendProcessing);
+        peerLogic->SendMessages(&dummyNode2, interruptDummy);
+    }
     BOOST_CHECK(connman->IsBanned(addr2));
 
     bool dummy;
@@ -237,20 +259,28 @@ BOOST_AUTO_TEST_CASE(DoS_banscore)
         LOCK(cs_main);
         Misbehaving(dummyNode1.GetId(), 100);
     }
-    LOCK(dummyNode1.cs_sendProcessing);
-    peerLogic->SendMessages(&dummyNode1, interruptDummy);
+    {
+        LOCK2(cs_main, dummyNode1.cs_sendProcessing);
+        peerLogic->SendMessages(&dummyNode1, interruptDummy);
+    }
     BOOST_CHECK(!connman->IsBanned(addr1));
     {
         LOCK(cs_main);
         Misbehaving(dummyNode1.GetId(), 10);
     }
-    peerLogic->SendMessages(&dummyNode1, interruptDummy);
+    {
+        LOCK2(cs_main, dummyNode1.cs_sendProcessing);
+        peerLogic->SendMessages(&dummyNode1, interruptDummy);
+    }
     BOOST_CHECK(!connman->IsBanned(addr1));
     {
         LOCK(cs_main);
         Misbehaving(dummyNode1.GetId(), 1);
     }
-    peerLogic->SendMessages(&dummyNode1, interruptDummy);
+    {
+        LOCK2(cs_main, dummyNode1.cs_sendProcessing);
+        peerLogic->SendMessages(&dummyNode1, interruptDummy);
+    }
     BOOST_CHECK(connman->IsBanned(addr1));
     gArgs.ForceSetArg("-banscore", std::to_string(DEFAULT_BANSCORE_THRESHOLD));
 
@@ -277,8 +307,10 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)
         LOCK(cs_main);
         Misbehaving(dummyNode.GetId(), 100);
     }
-    LOCK(dummyNode.cs_sendProcessing);
-    peerLogic->SendMessages(&dummyNode, interruptDummy);
+    {
+        LOCK2(cs_main, dummyNode.cs_sendProcessing);
+        peerLogic->SendMessages(&dummyNode, interruptDummy);
+    }
     BOOST_CHECK(connman->IsBanned(addr));
 
     SetMockTime(nStartTime+60*60);