Browse Source

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.
pull/1/head
Gavin Andresen 9 years ago
parent
commit
ca4cf5cff6
  1. 6
      qa/rpc-tests/txnmall.sh
  2. 5
      src/sync.cpp
  3. 41
      src/wallet.cpp
  4. 2
      src/wallet.h

6
qa/rpc-tests/txnmall.sh

@ -8,6 +8,8 @@ if [ $# -lt 1 ]; then @@ -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) @@ -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=$!

5
src/sync.cpp

@ -140,8 +140,9 @@ void AssertLockHeldInternal(const char *pszName, const char* pszFile, int nLine, @@ -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 */

41
src/wallet.cpp

@ -194,7 +194,7 @@ void CWallet::SetBestChain(const CBlockLocator& loc) @@ -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, @@ -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() @@ -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 @@ -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());

2
src/wallet.h

@ -363,7 +363,7 @@ public: @@ -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<uint256> GetConflicts(const uint256& txid) const;

Loading…
Cancel
Save