Acquire cs_main lock before cs_wallet during wallet initialization

ryanofsky · MarcoFalke · commit 2cb720ae6122 · 2017-10-03T18:35:39.000+02:00
CWallet::MarkConflicted may acquire the cs_main lock after CWalletDB::LoadWallet acquires the cs_wallet lock during wallet initialization. (CWalletDB::LoadWallet calls ReadKeyValue which calls CWallet::LoadToWallet which calls CWallet::MarkConflicted). This is the opposite order that cs_main and cs_wallet locks are acquired in the rest of the code, and so leads to POTENTIAL DEADLOCK DETECTED errors if bitcoin is built with -DDEBUG_LOCKORDER. This commit changes CWallet::LoadWallet (which calls CWalletDB::LoadWallet) to acquire both locks in the standard order. It also fixes some tests that were acquiring wallet and main locks out of order and failed with the new locking in CWallet::LoadWallet. Error was reported by Luke Dashjr <luke-jr@utopios.org> in https://botbot.me/freenode/bitcoin-core-dev/msg/90244330/ Github-Pull: #11126 Rebased-From: de9a1db
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
@@ -364,6 +364,12 @@ BOOST_AUTO_TEST_CASE(ApproximateBestSubset)
     empty_wallet();
 }
 
+static void AddKey(CWallet& wallet, const CKey& key)
+{
+    LOCK(wallet.cs_wallet);
+    wallet.AddKeyPubKey(key, key.GetPubKey());
+}
+
 BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)
 {
     LOCK(cs_main);
@@ -379,8 +385,7 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)
     // and new block files.
     {
         CWallet wallet;
-        LOCK(wallet.cs_wallet);
-        wallet.AddKeyPubKey(coinbaseKey, coinbaseKey.GetPubKey());
+        AddKey(wallet, coinbaseKey);
         BOOST_CHECK_EQUAL(nullBlock, wallet.ScanForWalletTransactions(oldTip));
         BOOST_CHECK_EQUAL(wallet.GetImmatureBalance(), 100 * COIN);
     }
@@ -393,8 +398,7 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)
     // file.
     {
         CWallet wallet;
-        LOCK(wallet.cs_wallet);
-        wallet.AddKeyPubKey(coinbaseKey, coinbaseKey.GetPubKey());
+        AddKey(wallet, coinbaseKey);
         BOOST_CHECK_EQUAL(oldTip, wallet.ScanForWalletTransactions(oldTip));
         BOOST_CHECK_EQUAL(wallet.GetImmatureBalance(), 50 * COIN);
     }
@@ -599,8 +603,7 @@ class ListCoinsTestingSetup : public TestChain100Setup
         wallet.reset(new CWallet(std::unique_ptr<CWalletDBWrapper>(new CWalletDBWrapper(&bitdb, "wallet_test.dat"))));
         bool firstRun;
         wallet->LoadWallet(firstRun);
-        LOCK(wallet->cs_wallet);
-        wallet->AddKeyPubKey(coinbaseKey, coinbaseKey.GetPubKey());
+        AddKey(*wallet, coinbaseKey);
         wallet->ScanForWalletTransactions(chainActive.Genesis());
     }
 
@@ -635,7 +638,7 @@ class ListCoinsTestingSetup : public TestChain100Setup
 BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup)
 {
     std::string coinbaseAddress = coinbaseKey.GetPubKey().GetID().ToString();
-    LOCK(wallet->cs_wallet);
+    LOCK2(cs_main, wallet->cs_wallet);
 
     // Confirm ListCoins initially returns 1 coin grouped under coinbaseKey
     // address.
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
@@ -3101,13 +3101,14 @@ CAmount CWallet::GetMinimumFee(unsigned int nTxBytes, const CCoinControl& coin_c
 
 DBErrors CWallet::LoadWallet(bool& fFirstRunRet)
 {
+    LOCK2(cs_main, cs_wallet);
+
     fFirstRunRet = false;
     DBErrors nLoadWalletRet = CWalletDB(*dbw,"cr+").LoadWallet(this);
     if (nLoadWalletRet == DB_NEED_REWRITE)
     {
         if (dbw->Rewrite("\x04pool"))
         {
-            LOCK(cs_wallet);
             setInternalKeyPool.clear();
             setExternalKeyPool.clear();
             m_pool_key_to_index.clear();

Original file line number	Diff line number	Diff line change
`@@ -3101,13 +3101,14 @@ CAmount CWallet::GetMinimumFee(unsigned int nTxBytes, const CCoinControl& coin_c`
`3101`	`3101`
`3102`	`3102`	`DBErrors CWallet::LoadWallet(bool& fFirstRunRet)`
`3103`	`3103`	`{`
	`3104`	`+ LOCK2(cs_main, cs_wallet);`
	`3105`	`+`
`3104`	`3106`	`fFirstRunRet = false;`
`3105`	`3107`	`DBErrors nLoadWalletRet = CWalletDB(*dbw,"cr+").LoadWallet(this);`
`3106`	`3108`	`if (nLoadWalletRet == DB_NEED_REWRITE)`
`3107`	`3109`	`{`
`3108`	`3110`	`if (dbw->Rewrite("\x04pool"))`
`3109`	`3111`	`{`
`3110`		`- LOCK(cs_wallet);`
`3111`	`3112`	`setInternalKeyPool.clear();`
`3112`	`3113`	`setExternalKeyPool.clear();`
`3113`	`3114`	`m_pool_key_to_index.clear();`