From 19a5676280370148492cf59a5103584cf37893ac Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Thu, 19 Dec 2013 09:09:51 +0100 Subject: [PATCH 1/3] Use mutex pointer instead of name for AssertLockHeld This makes it useable for non-global locks such as the wallet and keystore locks. --- src/main.cpp | 2 +- src/sync.cpp | 6 +++--- src/sync.h | 5 +++-- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index d130e9705..174c99345 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -2238,7 +2238,7 @@ void PushGetBlocks(CNode* pnode, CBlockIndex* pindexBegin, uint256 hashEnd) bool ProcessBlock(CValidationState &state, CNode* pfrom, CBlock* pblock, CDiskBlockPos *dbp) { - AssertLockHeld("cs_main"); + AssertLockHeld(cs_main); // Check for duplicate uint256 hash = pblock->GetHash(); diff --git a/src/sync.cpp b/src/sync.cpp index 9a20c87f8..b57d8c3c0 100644 --- a/src/sync.cpp +++ b/src/sync.cpp @@ -136,11 +136,11 @@ std::string LocksHeld() return result; } -void AssertLockHeld(std::string strName) +void AssertLockHeldInternal(const char *pszName, const char* pszFile, int nLine, void *cs) { BOOST_FOREACH(const PAIRTYPE(void*, CLockLocation)&i, *lockstack) - if (i.second.MutexName() == strName) return; - LogPrintf("Lock %s not held; locks held:\n%s", strName.c_str(), LocksHeld().c_str()); + if (i.first == cs) return; + LogPrintf("Lock %s not held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld().c_str()); assert(0); } diff --git a/src/sync.h b/src/sync.h index c50abf81b..077ed59b8 100644 --- a/src/sync.h +++ b/src/sync.h @@ -88,12 +88,13 @@ typedef AnnotatedMixin CWaitableCriticalSection; void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false); void LeaveCritical(); std::string LocksHeld(); -void AssertLockHeld(std::string strName); +void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void *cs); #else void static inline EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false) {} void static inline LeaveCritical() {} -void static inline AssertLockHeld(std::string) {} +void static inline AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void *cs) {} #endif +#define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, &cs) #ifdef DEBUG_LOCKCONTENTION void PrintLockContention(const char* pszName, const char* pszFile, int nLine); From 956916806a932b3502d8a1add410fd393c005df0 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Thu, 12 Dec 2013 08:07:59 +0100 Subject: [PATCH 2/3] Document cs_wallet lock and add AssertLockHeld Add locking assertions to wallet to all methods that access internal fields and do not aquire the cs_wallet mutex. --- src/wallet.cpp | 18 ++++++++++++++++++ src/wallet.h | 13 ++++++++++--- 2 files changed, 28 insertions(+), 3 deletions(-) diff --git a/src/wallet.cpp b/src/wallet.cpp index 3c3d890e2..6601575d8 100644 --- a/src/wallet.cpp +++ b/src/wallet.cpp @@ -33,6 +33,7 @@ struct CompareValueOnly CPubKey CWallet::GenerateNewKey() { + AssertLockHeld(cs_wallet); // mapKeyMetadata bool fCompressed = CanSupportFeature(FEATURE_COMPRPUBKEY); // default to compressed public keys if we want 0.6.0 wallets RandAddSeedPerfmon(); @@ -58,6 +59,7 @@ CPubKey CWallet::GenerateNewKey() bool CWallet::AddKeyPubKey(const CKey& secret, const CPubKey &pubkey) { + AssertLockHeld(cs_wallet); // mapKeyMetadata if (!CCryptoKeyStore::AddKeyPubKey(secret, pubkey)) return false; if (!fFileBacked) @@ -93,6 +95,7 @@ bool CWallet::AddCryptedKey(const CPubKey &vchPubKey, bool CWallet::LoadKeyMetadata(const CPubKey &pubkey, const CKeyMetadata &meta) { + AssertLockHeld(cs_wallet); // mapKeyMetadata if (meta.nCreateTime && (!nTimeFirstKey || meta.nCreateTime < nTimeFirstKey)) nTimeFirstKey = meta.nCreateTime; @@ -200,6 +203,7 @@ public: bool CWallet::SetMinVersion(enum WalletFeature nVersion, CWalletDB* pwalletdbIn, bool fExplicit) { + AssertLockHeld(cs_wallet); // nWalletVersion if (nWalletVersion >= nVersion) return true; @@ -233,6 +237,7 @@ bool CWallet::SetMinVersion(enum WalletFeature nVersion, CWalletDB* pwalletdbIn, bool CWallet::SetMaxVersion(int nVersion) { + AssertLockHeld(cs_wallet); // nWalletVersion, nWalletMaxVersion // cannot downgrade below current version if (nWalletVersion > nVersion) return false; @@ -325,6 +330,7 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase) int64_t CWallet::IncOrderPosNext(CWalletDB *pwalletdb) { + AssertLockHeld(cs_wallet); // nOrderPosNext int64_t nRet = nOrderPosNext++; if (pwalletdb) { pwalletdb->WriteOrderPosNext(nOrderPosNext); @@ -336,6 +342,7 @@ int64_t CWallet::IncOrderPosNext(CWalletDB *pwalletdb) CWallet::TxItems CWallet::OrderedTxItems(std::list& acentries, std::string strAccount) { + AssertLockHeld(cs_wallet); // mapWallet CWalletDB walletdb(strWalletFile); // First: get all CWalletTx and CAccountingEntry into a sorted-by-order multimap. @@ -1482,6 +1489,7 @@ string CWallet::SendMoneyToDestination(const CTxDestination& address, int64_t nV DBErrors CWallet::LoadWallet(bool& fFirstRunRet) { + AssertLockHeld(cs_wallet); // setKeyPool if (!fFileBacked) return DB_LOAD_OK; fFirstRunRet = false; @@ -1507,6 +1515,7 @@ DBErrors CWallet::LoadWallet(bool& fFirstRunRet) 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 */ @@ -1523,6 +1532,7 @@ bool CWallet::SetAddressBook(const CTxDestination& address, const string& strNam bool CWallet::DelAddressBook(const CTxDestination& address) { + AssertLockHeld(cs_wallet); // mapAddressBook mapAddressBook.erase(address); NotifyAddressBookChanged(this, address, "", ::IsMine(*this, address), "", CT_DELETED); if (!fFileBacked) @@ -1736,6 +1746,7 @@ std::map CWallet::GetAddressBalances() set< set > CWallet::GetAddressGroupings() { + AssertLockHeld(cs_wallet); // mapWallet set< set > groupings; set grouping; @@ -1828,6 +1839,7 @@ set< set > CWallet::GetAddressGroupings() set CWallet::GetAccountAddresses(string strAccount) const { + AssertLockHeld(cs_wallet); // mapWallet set result; BOOST_FOREACH(const PAIRTYPE(CTxDestination, CAddressBookData)& item, mapAddressBook) { @@ -1909,21 +1921,25 @@ void CWallet::UpdatedTransaction(const uint256 &hashTx) void CWallet::LockCoin(COutPoint& output) { + AssertLockHeld(cs_wallet); // setLockedCoins setLockedCoins.insert(output); } void CWallet::UnlockCoin(COutPoint& output) { + AssertLockHeld(cs_wallet); // setLockedCoins setLockedCoins.erase(output); } void CWallet::UnlockAllCoins() { + AssertLockHeld(cs_wallet); // setLockedCoins setLockedCoins.clear(); } bool CWallet::IsLockedCoin(uint256 hash, unsigned int n) const { + AssertLockHeld(cs_wallet); // setLockedCoins COutPoint outpt(hash, n); return (setLockedCoins.count(outpt) > 0); @@ -1931,6 +1947,7 @@ bool CWallet::IsLockedCoin(uint256 hash, unsigned int n) const void CWallet::ListLockedCoins(std::vector& vOutpts) { + AssertLockHeld(cs_wallet); // setLockedCoins for (std::set::iterator it = setLockedCoins.begin(); it != setLockedCoins.end(); it++) { COutPoint outpt = (*it); @@ -1939,6 +1956,7 @@ void CWallet::ListLockedCoins(std::vector& vOutpts) } void CWallet::GetKeyBirthTimes(std::map &mapKeyBirth) const { + AssertLockHeld(cs_wallet); // mapKeyMetadata mapKeyBirth.clear(); // get birth times for keys with metadata diff --git a/src/wallet.h b/src/wallet.h index 8e879c5e1..feb5cbf0c 100644 --- a/src/wallet.h +++ b/src/wallet.h @@ -102,6 +102,11 @@ private: int64_t nLastResend; public: + /// Main wallet lock. + /// This lock protects all the fields added by CWallet + /// except for: + /// fFileBacked (immutable after instantiation) + /// strWalletFile (immutable after instantiation) mutable CCriticalSection cs_wallet; bool fFileBacked; @@ -151,10 +156,11 @@ public: int64_t nTimeFirstKey; // check whether we are allowed to upgrade (or already support) to the named feature - bool CanSupportFeature(enum WalletFeature wf) { return nWalletMaxVersion >= wf; } + bool CanSupportFeature(enum WalletFeature wf) { AssertLockHeld(cs_wallet); return nWalletMaxVersion >= wf; } void AvailableCoins(std::vector& vCoins, bool fOnlyConfirmed=true, const CCoinControl *coinControl = NULL) const; bool SelectCoinsMinConf(int64_t nTargetValue, int nConfMine, int nConfTheirs, std::vector vCoins, std::set >& setCoinsRet, int64_t& nValueRet) const; + bool IsLockedCoin(uint256 hash, unsigned int n) const; void LockCoin(COutPoint& output); void UnlockCoin(COutPoint& output); @@ -171,7 +177,7 @@ public: // Load metadata (used by LoadWallet) bool LoadKeyMetadata(const CPubKey &pubkey, const CKeyMetadata &metadata); - bool LoadMinVersion(int nVersion) { nWalletVersion = nVersion; nWalletMaxVersion = std::max(nWalletMaxVersion, nVersion); return true; } + bool LoadMinVersion(int nVersion) { AssertLockHeld(cs_wallet); nWalletVersion = nVersion; nWalletMaxVersion = std::max(nWalletMaxVersion, nVersion); return true; } // Adds an encrypted key to the store, and saves it to disk. bool AddCryptedKey(const CPubKey &vchPubKey, const std::vector &vchCryptedSecret); @@ -320,6 +326,7 @@ public: unsigned int GetKeyPoolSize() { + AssertLockHeld(cs_wallet); // setKeyPool return setKeyPool.size(); } @@ -332,7 +339,7 @@ public: bool SetMaxVersion(int nVersion); // get the current wallet format (the oldest client version guaranteed to understand this wallet) - int GetVersion() { return nWalletVersion; } + int GetVersion() { AssertLockHeld(cs_wallet); return nWalletVersion; } /** Address book entry changed. * @note called with lock cs_wallet held. From 012ca1c9e4f21e0414f965cb812ecb1f2ddb4f67 Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Thu, 19 Dec 2013 09:58:06 +0100 Subject: [PATCH 3/3] LoadWallet: acquire cs_wallet mutex before clearing setKeyPool Make the function mutex-aware, to prevent having to lock cs_wallet at the call site in Init. --- src/wallet.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet.cpp b/src/wallet.cpp index 6601575d8..cfaa0d6b9 100644 --- a/src/wallet.cpp +++ b/src/wallet.cpp @@ -1489,7 +1489,6 @@ string CWallet::SendMoneyToDestination(const CTxDestination& address, int64_t nV DBErrors CWallet::LoadWallet(bool& fFirstRunRet) { - AssertLockHeld(cs_wallet); // setKeyPool if (!fFileBacked) return DB_LOAD_OK; fFirstRunRet = false; @@ -1498,6 +1497,7 @@ DBErrors CWallet::LoadWallet(bool& fFirstRunRet) { if (CDB::Rewrite(strWalletFile, "\x04pool")) { + LOCK(cs_wallet); setKeyPool.clear(); // Note: can't top-up keypool here, because wallet is locked. // User will be prompted to unlock wallet the next operation