Merge pull request #6534

9493803 Revert "Revert "Assert on probable deadlocks if the second lock isnt try_lock"" (Cory Fields)
605433f locks: don't lock cs_wallet before making rpc calls (Cory Fields)

src/sync.cpp

@@ -33,20 +33,22 @@ void PrintLockContention(const char* pszName, const char* pszFile, int nLine)
 //
 
 struct CLockLocation {
-    CLockLocation(const char* pszName, const char* pszFile, int nLine)
+    CLockLocation(const char* pszName, const char* pszFile, int nLine, bool fTryIn)
     {
         mutexName = pszName;
         sourceFile = pszFile;
         sourceLine = nLine;
+        fTry = fTryIn;
     }
 
     std::string ToString() const
     {
-        return mutexName + "  " + sourceFile + ":" + itostr(sourceLine);
+        return mutexName + "  " + sourceFile + ":" + itostr(sourceLine) + (fTry ? " (TRY)" : "");
     }
 
     std::string MutexName() const { return mutexName; }
 
+    bool fTry;
 private:
     std::string mutexName;
     std::string sourceFile;
@@ -62,23 +64,52 @@ static boost::thread_specific_ptr<LockStack> lockstack;
 
 static void potential_deadlock_detected(const std::pair<void*, void*>& mismatch, const LockStack& s1, const LockStack& s2)
 {
+    // We attempt to not assert on probably-not deadlocks by assuming that
+    // a try lock will immediately have otherwise bailed if it had
+    // failed to get the lock
+    // We do this by, for the locks which triggered the potential deadlock,
+    // in either lockorder, checking that the second of the two which is locked
+    // is only a TRY_LOCK, ignoring locks if they are reentrant.
+    bool firstLocked = false;
+    bool secondLocked = false;
+    bool onlyMaybeDeadlock = false;
+
     LogPrintf("POTENTIAL DEADLOCK DETECTED\n");
     LogPrintf("Previous lock order was:\n");
     BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, s2) {
-        if (i.first == mismatch.first)
+        if (i.first == mismatch.first) {
             LogPrintf(" (1)");
-        if (i.first == mismatch.second)
+            if (!firstLocked && secondLocked && i.second.fTry)
+                onlyMaybeDeadlock = true;
+            firstLocked = true;
+        }
+        if (i.first == mismatch.second) {
             LogPrintf(" (2)");
+            if (!secondLocked && firstLocked && i.second.fTry)
+                onlyMaybeDeadlock = true;
+            secondLocked = true;
+        }
         LogPrintf(" %s\n", i.second.ToString());
     }
+    firstLocked = false;
+    secondLocked = false;
     LogPrintf("Current lock order is:\n");
     BOOST_FOREACH (const PAIRTYPE(void*, CLockLocation) & i, s1) {
-        if (i.first == mismatch.first)
+        if (i.first == mismatch.first) {
             LogPrintf(" (1)");
-        if (i.first == mismatch.second)
+            if (!firstLocked && secondLocked && i.second.fTry)
+                onlyMaybeDeadlock = true;
+            firstLocked = true;
+        }
+        if (i.first == mismatch.second) {
             LogPrintf(" (2)");
+            if (!secondLocked && firstLocked && i.second.fTry)
+                onlyMaybeDeadlock = true;
+            secondLocked = true;
+        }
         LogPrintf(" %s\n", i.second.ToString());
     }
+    assert(onlyMaybeDeadlock);
 }
 
 static void push_lock(void* c, const CLockLocation& locklocation, bool fTry)
@@ -101,10 +132,8 @@ static void push_lock(void* c, const CLockLocation& locklocation, bool fTry)
             lockorders[p1] = (*lockstack);
 
             std::pair<void*, void*> p2 = std::make_pair(c, i.first);
-            if (lockorders.count(p2)) {
+            if (lockorders.count(p2))
                 potential_deadlock_detected(p1, lockorders[p2], lockorders[p1]);
-                break;
-            }
         }
     }
     dd_mutex.unlock();
@@ -119,7 +148,7 @@ static void pop_lock()
 
 void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry)
 {
-    push_lock(cs, CLockLocation(pszName, pszFile, nLine), fTry);
+    push_lock(cs, CLockLocation(pszName, pszFile, nLine, fTry), fTry);
 }
 
 void LeaveCritical()

src/test/rpc_wallet_tests.cpp

@@ -27,8 +27,6 @@ BOOST_FIXTURE_TEST_SUITE(rpc_wallet_tests, TestingSetup)
 
 BOOST_AUTO_TEST_CASE(rpc_addmultisig)
 {
-    LOCK(pwalletMain->cs_wallet);
-
     rpcfn_type addmultisig = tableRPC["addmultisigaddress"]->actor;
 
     // old, 65-byte-long:
@@ -68,25 +66,28 @@ BOOST_AUTO_TEST_CASE(rpc_wallet)
 {
     // Test RPC calls for various wallet statistics
     UniValue r;
-
+    CPubKey demoPubkey;
-    LOCK2(cs_main, pwalletMain->cs_wallet);
+    CBitcoinAddress demoAddress;
-
-    CPubKey demoPubkey = pwalletMain->GenerateNewKey();
-    CBitcoinAddress demoAddress = CBitcoinAddress(CTxDestination(demoPubkey.GetID()));
     UniValue retValue;
     string strAccount = "walletDemoAccount";
-    string strPurpose = "receive";
+    CBitcoinAddress setaccountDemoAddress;
-    BOOST_CHECK_NO_THROW({ /*Initialize Wallet with an account */
+    {
-        CWalletDB walletdb(pwalletMain->strWalletFile);
+        LOCK(pwalletMain->cs_wallet);
-        CAccount account;
-        account.vchPubKey = demoPubkey;
-        pwalletMain->SetAddressBook(account.vchPubKey.GetID(), strAccount, strPurpose);
-        walletdb.WriteAccount(strAccount, account);
-    });
 
-    CPubKey setaccountDemoPubkey = pwalletMain->GenerateNewKey();
+        demoPubkey = pwalletMain->GenerateNewKey();
-    CBitcoinAddress setaccountDemoAddress = CBitcoinAddress(CTxDestination(setaccountDemoPubkey.GetID()));
+        demoAddress = CBitcoinAddress(CTxDestination(demoPubkey.GetID()));
+        string strPurpose = "receive";
+        BOOST_CHECK_NO_THROW({ /*Initialize Wallet with an account */
+            CWalletDB walletdb(pwalletMain->strWalletFile);
+            CAccount account;
+            account.vchPubKey = demoPubkey;
+            pwalletMain->SetAddressBook(account.vchPubKey.GetID(), strAccount, strPurpose);
+            walletdb.WriteAccount(strAccount, account);
+        });
 
+        CPubKey setaccountDemoPubkey = pwalletMain->GenerateNewKey();
+        setaccountDemoAddress = CBitcoinAddress(CTxDestination(setaccountDemoPubkey.GetID()));
+    }
     /*********************************
      * 			setaccount
      *********************************/