Wallet locking fixes for -DDEBUG_LOCKORDER

Compiling with -DDEBUG_LOCKORDER and running the qa/rpc-test/ regression
tests uncovered a couple of wallet methods that should (but didn't)
acquire the cs_wallet mutext.

I also changed the AssertLockHeld() routine print to stderr and
abort, instead of printing to debug.log and then assert()'ing.
It is annoying to look in debug.log to find out which
AssertLockHeld is failing.

Author: gavinandresen

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=$!
 

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 */

src/wallet.cpp

@@ -194,7 +194,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;
 
@@ -221,7 +221,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;
@@ -1623,14 +1623,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))
@@ -1640,21 +1643,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());

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;