Merge pull request #4085

b39a07d Add missing AssertLockHeld in ConnectBlock (Wladimir J. van der Laan)
41106a5 qt: get required locks upfront in polling functions (Wladimir J. van der Laan)
ed67100 Add required locks in tests (Wladimir J. van der Laan)

Author: laanwj

src/main.cpp

@@ -1727,6 +1727,7 @@ void ThreadScriptCheck() {
 
 bool ConnectBlock(CBlock& block, CValidationState& state, CBlockIndex* pindex, CCoinsViewCache& view, bool fJustCheck)
 {
+    AssertLockHeld(cs_main);
     // Check it again in case a previous version let a bad block in
     if (!CheckBlock(block, state, !fJustCheck, !fJustCheck))
         return false;

src/qt/clientmodel.cpp

@@ -92,6 +92,12 @@ double ClientModel::getVerificationProgress() const
 
 void ClientModel::updateTimer()
 {
+    // Get required lock upfront. This avoids the GUI from getting stuck on
+    // periodical polls if the core is holding the locks for a longer time -
+    // for example, during a wallet rescan.
+    TRY_LOCK(cs_main, lockMain);
+    if(!lockMain)
+        return;
     // Some quantities (such as number of blocks) change so fast that we don't want to be notified for each change.
     // Periodically check and update with a timer.
     int newNumBlocks = getNumBlocks();

src/qt/transactiontablemodel.cpp

@@ -24,7 +24,6 @@
 #include <QDebug>
 #include <QIcon>
 #include <QList>
-#include <QTimer>
 
 // Amount column is right-aligned it contains numbers
 static int column_alignments[] = {
@@ -187,17 +186,25 @@ class TransactionTablePriv
         {
             TransactionRecord *rec = &cachedWallet[idx];
 
+            // Get required locks upfront. This avoids the GUI from getting
+            // stuck if the core is holding the locks for a longer time - for
+            // example, during a wallet rescan.
+            //
             // If a status update is needed (blocks came in since last check),
             //  update the status of this transaction from the wallet. Otherwise,
             // simply re-use the cached status.
-            LOCK2(cs_main, wallet->cs_wallet);
-            if(rec->statusUpdateNeeded())
+            TRY_LOCK(cs_main, lockMain);
+            if(lockMain)
             {
-                std::map<uint256, CWalletTx>::iterator mi = wallet->mapWallet.find(rec->hash);
-
-                if(mi != wallet->mapWallet.end())
+                TRY_LOCK(wallet->cs_wallet, lockWallet);
+                if(lockWallet && rec->statusUpdateNeeded())
                 {
-                    rec->updateStatus(mi->second);
+                    std::map<uint256, CWalletTx>::iterator mi = wallet->mapWallet.find(rec->hash);
+
+                    if(mi != wallet->mapWallet.end())
+                    {
+                        rec->updateStatus(mi->second);
+                    }
                 }
             }
             return rec;

src/qt/walletmodel.cpp

@@ -98,18 +98,21 @@ void WalletModel::updateStatus()
 
 void WalletModel::pollBalanceChanged()
 {
-    bool heightChanged = false;
-    {
-        LOCK(cs_main);
-        if(chainActive.Height() != cachedNumBlocks)
-        {
-            // Balance and number of transactions might have changed
-            cachedNumBlocks = chainActive.Height();
-            heightChanged = true;
-        }
-    }
-    if(heightChanged)
+    // Get required locks upfront. This avoids the GUI from getting stuck on
+    // periodical polls if the core is holding the locks for a longer time -
+    // for example, during a wallet rescan.
+    TRY_LOCK(cs_main, lockMain);
+    if(!lockMain)
+        return;
+    TRY_LOCK(wallet->cs_wallet, lockWallet);
+    if(!lockWallet)
+        return;
+
+    if(chainActive.Height() != cachedNumBlocks)
     {
+        // Balance and number of transactions might have changed
+        cachedNumBlocks = chainActive.Height();
+
         checkBalanceChanged();
         if(transactionTableModel)
             transactionTableModel->updateConfirmations();

src/test/rpc_wallet_tests.cpp

@@ -65,7 +65,7 @@ BOOST_AUTO_TEST_CASE(rpc_wallet)
     // Test RPC calls for various wallet statistics
     Value r;
 
-    LOCK(pwalletMain->cs_wallet);
+    LOCK2(cs_main, pwalletMain->cs_wallet);
 
     BOOST_CHECK_NO_THROW(CallRPC("listunspent"));
     BOOST_CHECK_THROW(CallRPC("listunspent string"), runtime_error);

src/test/script_P2SH_tests.cpp

@@ -50,6 +50,7 @@ BOOST_AUTO_TEST_SUITE(script_P2SH_tests)
 
 BOOST_AUTO_TEST_CASE(sign)
 {
+    LOCK(cs_main);
     // Pay-to-script-hash looks like this:
     // scriptSig:    <sig> <sig...> <serialized_script>
     // scriptPubKey: HASH160 <hash> EQUAL
@@ -147,6 +148,7 @@ BOOST_AUTO_TEST_CASE(norecurse)
 
 BOOST_AUTO_TEST_CASE(set)
 {
+    LOCK(cs_main);
     // Test the CScript::Set* methods
     CBasicKeyStore keystore;
     CKey key[4];
@@ -250,6 +252,7 @@ BOOST_AUTO_TEST_CASE(switchover)
 
 BOOST_AUTO_TEST_CASE(AreInputsStandard)
 {
+    LOCK(cs_main);
     CCoinsView coinsDummy;
     CCoinsViewCache coins(coinsDummy);
     CBasicKeyStore keystore;

src/test/transaction_tests.cpp

@@ -254,6 +254,7 @@ BOOST_AUTO_TEST_CASE(test_Get)
 
 BOOST_AUTO_TEST_CASE(test_IsStandard)
 {
+    LOCK(cs_main);
     CBasicKeyStore keystore;
     CCoinsView coinsDummy;
     CCoinsViewCache coins(coinsDummy);