From ed671005654136b728768a423c8ac7f4e1ce637f Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Wed, 23 Apr 2014 08:05:05 +0200 Subject: [PATCH 1/3] Add required locks in tests Unit tests with DEBUG_LOCKORDER were running into assertions. --- src/test/rpc_wallet_tests.cpp | 2 +- src/test/script_P2SH_tests.cpp | 3 +++ src/test/transaction_tests.cpp | 1 + 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/test/rpc_wallet_tests.cpp b/src/test/rpc_wallet_tests.cpp index af34e496..eea249b1 100644 --- a/src/test/rpc_wallet_tests.cpp +++ b/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); diff --git a/src/test/script_P2SH_tests.cpp b/src/test/script_P2SH_tests.cpp index 03dbdde5..9b1290e0 100644 --- a/src/test/script_P2SH_tests.cpp +++ b/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: // scriptPubKey: HASH160 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; diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp index 2ebb652f..24647950 100644 --- a/src/test/transaction_tests.cpp +++ b/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); From 41106a50d2be9358ab19e75c1d6d075a773525b7 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Wed, 23 Apr 2014 08:40:48 +0200 Subject: [PATCH 2/3] qt: get required locks upfront in polling functions 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. --- src/qt/clientmodel.cpp | 6 ++++++ src/qt/transactiontablemodel.cpp | 21 ++++++++++++++------- src/qt/walletmodel.cpp | 25 ++++++++++++++----------- 3 files changed, 34 insertions(+), 18 deletions(-) diff --git a/src/qt/clientmodel.cpp b/src/qt/clientmodel.cpp index 28729664..3c0564c2 100644 --- a/src/qt/clientmodel.cpp +++ b/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(); diff --git a/src/qt/transactiontablemodel.cpp b/src/qt/transactiontablemodel.cpp index aaecf88c..df412650 100644 --- a/src/qt/transactiontablemodel.cpp +++ b/src/qt/transactiontablemodel.cpp @@ -24,7 +24,6 @@ #include #include #include -#include // Amount column is right-aligned it contains numbers static int column_alignments[] = { @@ -187,17 +186,25 @@ public: { 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::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::iterator mi = wallet->mapWallet.find(rec->hash); + + if(mi != wallet->mapWallet.end()) + { + rec->updateStatus(mi->second); + } } } return rec; diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 61f26107..37d82ec0 100644 --- a/src/qt/walletmodel.cpp +++ b/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(); From b39a07dc42ab6ba746a25206969fb81913146f1f Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Wed, 23 Apr 2014 08:55:24 +0200 Subject: [PATCH 3/3] Add missing AssertLockHeld in ConnectBlock --- src/main.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main.cpp b/src/main.cpp index 0bbe8337..a591168c 100644 --- a/src/main.cpp +++ b/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;