From ca4cf5cff6fb60c9769b62acce2e3a8fcd0e7aae Mon Sep 17 00:00:00 2001 From: Gavin Andresen Date: Tue, 18 Feb 2014 12:11:46 -0500 Subject: [PATCH] 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. --- qa/rpc-tests/txnmall.sh | 6 ++++-- src/sync.cpp | 5 +++-- src/wallet.cpp | 41 +++++++++++++++++++++++------------------ src/wallet.h | 2 +- 4 files changed, 31 insertions(+), 23 deletions(-) diff --git a/qa/rpc-tests/txnmall.sh b/qa/rpc-tests/txnmall.sh index 6bf92fce4..06e4f7102 100755 --- 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 index 8f713807f..e624a9ee8 100644 --- 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 index 823c96949..82f71d24a 100644 --- a/src/wallet.cpp +++ b/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::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::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()); diff --git a/src/wallet.h b/src/wallet.h index 95fb0ee9d..eb192f1ca 100644 --- a/src/wallet.h +++ b/src/wallet.h @@ -363,7 +363,7 @@ public: 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 GetConflicts(const uint256& txid) const;