Skip to content

Commit ca4cf5c

Browse files
gavinandresengavinandresen
committed
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.
1 parent 5c99323 commit ca4cf5c

File tree

4 files changed

+31
-23
lines changed

4 files changed

+31
-23
lines changed

‎qa/rpc-tests/txnmall.sh‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ if [ $# -lt 1 ]; then
88
exit 1
99
fi
1010

11+
set -f
12+
1113
BITCOIND=${1}/bitcoind
1214
CLI=${1}/bitcoin-cli
1315

@@ -23,13 +25,13 @@ D=$(mktemp -d test.XXXXX)
2325

2426
D1=${D}/node1
2527
CreateDataDir $D1 port=11000 rpcport=11001
26-
B1ARGS="-datadir=$D1 -debug"
28+
B1ARGS="-datadir=$D1"
2729
$BITCOIND $B1ARGS &
2830
B1PID=$!
2931

3032
D2=${D}/node2
3133
CreateDataDir $D2 port=11010 rpcport=11011
32-
B2ARGS="-datadir=$D2 -debug"
34+
B2ARGS="-datadir=$D2"
3335
$BITCOIND $B2ARGS &
3436
B2PID=$!
3537

‎src/sync.cpp‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -140,8 +140,9 @@ void AssertLockHeldInternal(const char *pszName, const char* pszFile, int nLine,
140140
{
141141
BOOST_FOREACH(const PAIRTYPE(void*, CLockLocation)&i, *lockstack)
142142
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();
145146
}
146147

147148
#endif /* DEBUG_LOCKORDER */

‎src/wallet.cpp‎

Lines changed: 23 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,7 @@ void CWallet::SetBestChain(const CBlockLocator& loc)
194194

195195
bool CWallet::SetMinVersion(enum WalletFeature nVersion, CWalletDB* pwalletdbIn, bool fExplicit)
196196
{
197-
AssertLockHeld(cs_wallet); // nWalletVersion
197+
LOCK(cs_wallet); // nWalletVersion
198198
if (nWalletVersion >= nVersion)
199199
return true;
200200

@@ -221,7 +221,7 @@ bool CWallet::SetMinVersion(enum WalletFeature nVersion, CWalletDB* pwalletdbIn,
221221

222222
bool CWallet::SetMaxVersion(int nVersion)
223223
{
224-
AssertLockHeld(cs_wallet); // nWalletVersion, nWalletMaxVersion
224+
LOCK(cs_wallet); // nWalletVersion, nWalletMaxVersion
225225
// cannot downgrade below current version
226226
if (nWalletVersion > nVersion)
227227
return false;
@@ -1623,14 +1623,17 @@ DBErrors CWallet::ZapWalletTx()
16231623

16241624
bool CWallet::SetAddressBook(const CTxDestination& address, const string& strName, const string& strPurpose)
16251625
{
1626-
AssertLockHeld(cs_wallet); // mapAddressBook
1627-
std::map<CTxDestination, CAddressBookData>::iterator mi = mapAddressBook.find(address);
1628-
mapAddressBook[address].name = strName;
1629-
if (!strPurpose.empty()) /* update purpose only if requested */
1630-
mapAddressBook[address].purpose = strPurpose;
1626+
bool fUpdated = false;
1627+
{
1628+
LOCK(cs_wallet); // mapAddressBook
1629+
std::map<CTxDestination, CAddressBookData>::iterator mi = mapAddressBook.find(address);
1630+
fUpdated = mi != mapAddressBook.end();
1631+
mapAddressBook[address].name = strName;
1632+
if (!strPurpose.empty()) /* update purpose only if requested */
1633+
mapAddressBook[address].purpose = strPurpose;
1634+
}
16311635
NotifyAddressBookChanged(this, address, strName, ::IsMine(*this, address),
1632-
mapAddressBook[address].purpose,
1633-
(mi == mapAddressBook.end()) ? CT_NEW : CT_UPDATED);
1636+
strPurpose, (fUpdated ? CT_UPDATED : CT_NEW) );
16341637
if (!fFileBacked)
16351638
return false;
16361639
if (!strPurpose.empty() && !CWalletDB(strWalletFile).WritePurpose(CBitcoinAddress(address).ToString(), strPurpose))
@@ -1640,21 +1643,23 @@ bool CWallet::SetAddressBook(const CTxDestination& address, const string& strNam
16401643

16411644
bool CWallet::DelAddressBook(const CTxDestination& address)
16421645
{
1643-
1644-
AssertLockHeld(cs_wallet); // mapAddressBook
1645-
1646-
if(fFileBacked)
16471646
{
1648-
// Delete destdata tuples associated with address
1649-
std::string strAddress = CBitcoinAddress(address).ToString();
1650-
BOOST_FOREACH(const PAIRTYPE(string, string) &item, mapAddressBook[address].destdata)
1647+
LOCK(cs_wallet); // mapAddressBook
1648+
1649+
if(fFileBacked)
16511650
{
1652-
CWalletDB(strWalletFile).EraseDestData(strAddress, item.first);
1651+
// Delete destdata tuples associated with address
1652+
std::string strAddress = CBitcoinAddress(address).ToString();
1653+
BOOST_FOREACH(const PAIRTYPE(string, string) &item, mapAddressBook[address].destdata)
1654+
{
1655+
CWalletDB(strWalletFile).EraseDestData(strAddress, item.first);
1656+
}
16531657
}
1658+
mapAddressBook.erase(address);
16541659
}
16551660

1656-
mapAddressBook.erase(address);
16571661
NotifyAddressBookChanged(this, address, "", ::IsMine(*this, address), "", CT_DELETED);
1662+
16581663
if (!fFileBacked)
16591664
return false;
16601665
CWalletDB(strWalletFile).ErasePurpose(CBitcoinAddress(address).ToString());

‎src/wallet.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -363,7 +363,7 @@ class CWallet : public CCryptoKeyStore, public CWalletInterface
363363
bool SetMaxVersion(int nVersion);
364364

365365
// get the current wallet format (the oldest client version guaranteed to understand this wallet)
366-
int GetVersion() { AssertLockHeld(cs_wallet); return nWalletVersion; }
366+
int GetVersion() { LOCK(cs_wallet); return nWalletVersion; }
367367

368368
// Get wallet transactions that conflict with given transaction (spend same outputs)
369369
std::set<uint256> GetConflicts(const uint256& txid) const;

0 commit comments

Comments
 (0)