Merge pull request #3704 from gavinandresen/wallet_lock_fixes

gavinandresen · gavinandresen · commit a16ad1c0f465 · 2014-02-24T14:39:23.000-05:00
Wallet locking fixes for -DDEBUG_LOCKORDER
diff --git a/qa/rpc-tests/txnmall.sh b/qa/rpc-tests/txnmall.sh
@@ -8,6 +8,8 @@ if [ $# -lt 1 ]; then
         exit 1
 fi
 
+set -f
+
 BITCOIND=${1}/bitcoind
 CLI=${1}/bitcoin-cli
 
@@ -23,13 +25,13 @@ D=$(mktemp -d test.XXXXX)
 
 D1=${D}/node1
 CreateDataDir $D1 port=11000 rpcport=11001
-B1ARGS="-datadir=$D1 -debug"
+B1ARGS="-datadir=$D1"
 $BITCOIND $B1ARGS &
 B1PID=$!
 
 D2=${D}/node2
 CreateDataDir $D2 port=11010 rpcport=11011
-B2ARGS="-datadir=$D2 -debug"
+B2ARGS="-datadir=$D2"
 $BITCOIND $B2ARGS &
 B2PID=$!
 
diff --git a/src/sync.cpp b/src/sync.cpp
@@ -140,8 +140,9 @@ void AssertLockHeldInternal(const char *pszName, const char* pszFile, int nLine,
 {
     BOOST_FOREACH(const PAIRTYPE(void*, CLockLocation)&i, *lockstack)
         if (i.first == cs) return;
-    LogPrintf("Lock %s not held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld());
-    assert(0);
+    fprintf(stderr, "Assertion failed: lock %s not held in %s:%i; locks held:\n%s",
+            pszName, pszFile, nLine, LocksHeld().c_str());
+    abort();
 }
 
 #endif /* DEBUG_LOCKORDER */
diff --git a/src/wallet.cpp b/src/wallet.cpp
@@ -192,7 +192,7 @@ void CWallet::SetBestChain(const CBlockLocator& loc)
 
 bool CWallet::SetMinVersion(enum WalletFeature nVersion, CWalletDB* pwalletdbIn, bool fExplicit)
 {
-    AssertLockHeld(cs_wallet); // nWalletVersion
+    LOCK(cs_wallet); // nWalletVersion
     if (nWalletVersion >= nVersion)
         return true;
 
@@ -219,7 +219,7 @@ bool CWallet::SetMinVersion(enum WalletFeature nVersion, CWalletDB* pwalletdbIn,
 
 bool CWallet::SetMaxVersion(int nVersion)
 {
-    AssertLockHeld(cs_wallet); // nWalletVersion, nWalletMaxVersion
+    LOCK(cs_wallet); // nWalletVersion, nWalletMaxVersion
     // cannot downgrade below current version
     if (nWalletVersion > nVersion)
         return false;
@@ -1621,14 +1621,17 @@ DBErrors CWallet::ZapWalletTx()
 
 bool CWallet::SetAddressBook(const CTxDestination& address, const string& strName, const string& strPurpose)
 {
-    AssertLockHeld(cs_wallet); // mapAddressBook
-    std::map<CTxDestination, CAddressBookData>::iterator mi = mapAddressBook.find(address);
-    mapAddressBook[address].name = strName;
-    if (!strPurpose.empty()) /* update purpose only if requested */
-        mapAddressBook[address].purpose = strPurpose;
+    bool fUpdated = false;
+    {
+        LOCK(cs_wallet); // mapAddressBook
+        std::map<CTxDestination, CAddressBookData>::iterator mi = mapAddressBook.find(address);
+        fUpdated = mi != mapAddressBook.end();
+        mapAddressBook[address].name = strName;
+        if (!strPurpose.empty()) /* update purpose only if requested */
+            mapAddressBook[address].purpose = strPurpose;
+    }
     NotifyAddressBookChanged(this, address, strName, ::IsMine(*this, address),
-            mapAddressBook[address].purpose,
-            (mi == mapAddressBook.end()) ?  CT_NEW : CT_UPDATED);
+                             strPurpose, (fUpdated ? CT_UPDATED : CT_NEW) );
     if (!fFileBacked)
         return false;
     if (!strPurpose.empty() && !CWalletDB(strWalletFile).WritePurpose(CBitcoinAddress(address).ToString(), strPurpose))
@@ -1638,21 +1641,23 @@ bool CWallet::SetAddressBook(const CTxDestination& address, const string& strNam
 
 bool CWallet::DelAddressBook(const CTxDestination& address)
 {
-
-    AssertLockHeld(cs_wallet); // mapAddressBook
-
-    if(fFileBacked)
     {
-        // Delete destdata tuples associated with address
-        std::string strAddress = CBitcoinAddress(address).ToString();
-        BOOST_FOREACH(const PAIRTYPE(string, string) &item, mapAddressBook[address].destdata)
+        LOCK(cs_wallet); // mapAddressBook
+
+        if(fFileBacked)
         {
-            CWalletDB(strWalletFile).EraseDestData(strAddress, item.first);
+            // Delete destdata tuples associated with address
+            std::string strAddress = CBitcoinAddress(address).ToString();
+            BOOST_FOREACH(const PAIRTYPE(string, string) &item, mapAddressBook[address].destdata)
+            {
+                CWalletDB(strWalletFile).EraseDestData(strAddress, item.first);
+            }
         }
+        mapAddressBook.erase(address);
     }
 
-    mapAddressBook.erase(address);
     NotifyAddressBookChanged(this, address, "", ::IsMine(*this, address), "", CT_DELETED);
+
     if (!fFileBacked)
         return false;
     CWalletDB(strWalletFile).ErasePurpose(CBitcoinAddress(address).ToString());
diff --git a/src/wallet.h b/src/wallet.h
@@ -363,7 +363,7 @@ class CWallet : public CCryptoKeyStore, public CWalletInterface
     bool SetMaxVersion(int nVersion);
 
     // get the current wallet format (the oldest client version guaranteed to understand this wallet)
-    int GetVersion() { AssertLockHeld(cs_wallet); return nWalletVersion; }
+    int GetVersion() { LOCK(cs_wallet); return nWalletVersion; }
 
     // Get wallet transactions that conflict with given transaction (spend same outputs)
     std::set<uint256> GetConflicts(const uint256& txid) const;

Original file line number	Diff line number	Diff line change
`@@ -140,8 +140,9 @@ void AssertLockHeldInternal(const char pszName, const char pszFile, int nLine,`
`140`	`140`	`{`
`141`	`141`	`BOOST_FOREACH(const PAIRTYPE(void, CLockLocation)&i, lockstack)`
`142`	`142`	`if (i.first == cs) return;`
`143`		`- LogPrintf("Lock %s not held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld());`
`144`		`- assert(0);`
	`143`	`+ fprintf(stderr, "Assertion failed: lock %s not held in %s:%i; locks held:\n%s",`
	`144`	`+ pszName, pszFile, nLine, LocksHeld().c_str());`
	`145`	`+ abort();`
`145`	`146`	`}`
`146`	`147`
`147`	`148`	`#endif /* DEBUG_LOCKORDER */`