Skip to content

Commit de9a1db

Browse files
committed
Acquire cs_main lock before cs_wallet during wallet initialization
CWallet::MarkConflicted may acquire the cs_main lock after CWalletDB::LoadWallet acquires the cs_wallet lock during wallet initialization. (CWalletDB::LoadWallet calls ReadKeyValue which calls CWallet::LoadToWallet which calls CWallet::MarkConflicted). This is the opposite order that cs_main and cs_wallet locks are acquired in the rest of the code, and so leads to POTENTIAL DEADLOCK DETECTED errors if bitcoin is built with -DDEBUG_LOCKORDER. This commit changes CWallet::LoadWallet (which calls CWalletDB::LoadWallet) to acquire both locks in the standard order. It also fixes some tests that were acquiring wallet and main locks out of order and failed with the new locking in CWallet::LoadWallet. Error was reported by Luke Dashjr <luke-jr@utopios.org> in https://botbot.me/freenode/bitcoin-core-dev/msg/90244330/
1 parent 41496e2 commit de9a1db

File tree

2 files changed

+12
-8
lines changed

2 files changed

+12
-8
lines changed

src/wallet/test/wallet_tests.cpp

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -364,6 +364,12 @@ BOOST_AUTO_TEST_CASE(ApproximateBestSubset)
364364
empty_wallet();
365365
}
366366

367+
static void AddKey(CWallet& wallet, const CKey& key)
368+
{
369+
LOCK(wallet.cs_wallet);
370+
wallet.AddKeyPubKey(key, key.GetPubKey());
371+
}
372+
367373
BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)
368374
{
369375
LOCK(cs_main);
@@ -379,8 +385,7 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)
379385
// and new block files.
380386
{
381387
CWallet wallet;
382-
LOCK(wallet.cs_wallet);
383-
wallet.AddKeyPubKey(coinbaseKey, coinbaseKey.GetPubKey());
388+
AddKey(wallet, coinbaseKey);
384389
BOOST_CHECK_EQUAL(nullBlock, wallet.ScanForWalletTransactions(oldTip));
385390
BOOST_CHECK_EQUAL(wallet.GetImmatureBalance(), 100 * COIN);
386391
}
@@ -393,8 +398,7 @@ BOOST_FIXTURE_TEST_CASE(rescan, TestChain100Setup)
393398
// file.
394399
{
395400
CWallet wallet;
396-
LOCK(wallet.cs_wallet);
397-
wallet.AddKeyPubKey(coinbaseKey, coinbaseKey.GetPubKey());
401+
AddKey(wallet, coinbaseKey);
398402
BOOST_CHECK_EQUAL(oldTip, wallet.ScanForWalletTransactions(oldTip));
399403
BOOST_CHECK_EQUAL(wallet.GetImmatureBalance(), 50 * COIN);
400404
}
@@ -599,8 +603,7 @@ class ListCoinsTestingSetup : public TestChain100Setup
599603
wallet.reset(new CWallet(std::unique_ptr<CWalletDBWrapper>(new CWalletDBWrapper(&bitdb, "wallet_test.dat"))));
600604
bool firstRun;
601605
wallet->LoadWallet(firstRun);
602-
LOCK(wallet->cs_wallet);
603-
wallet->AddKeyPubKey(coinbaseKey, coinbaseKey.GetPubKey());
606+
AddKey(*wallet, coinbaseKey);
604607
wallet->ScanForWalletTransactions(chainActive.Genesis());
605608
}
606609

@@ -635,7 +638,7 @@ class ListCoinsTestingSetup : public TestChain100Setup
635638
BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup)
636639
{
637640
std::string coinbaseAddress = coinbaseKey.GetPubKey().GetID().ToString();
638-
LOCK(wallet->cs_wallet);
641+
LOCK2(cs_main, wallet->cs_wallet);
639642

640643
// Confirm ListCoins initially returns 1 coin grouped under coinbaseKey
641644
// address.

src/wallet/wallet.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3107,13 +3107,14 @@ CAmount CWallet::GetMinimumFee(unsigned int nTxBytes, const CCoinControl& coin_c
31073107

31083108
DBErrors CWallet::LoadWallet(bool& fFirstRunRet)
31093109
{
3110+
LOCK2(cs_main, cs_wallet);
3111+
31103112
fFirstRunRet = false;
31113113
DBErrors nLoadWalletRet = CWalletDB(*dbw,"cr+").LoadWallet(this);
31123114
if (nLoadWalletRet == DB_NEED_REWRITE)
31133115
{
31143116
if (dbw->Rewrite("\x04pool"))
31153117
{
3116-
LOCK(cs_wallet);
31173118
setInternalKeyPool.clear();
31183119
setExternalKeyPool.clear();
31193120
m_pool_key_to_index.clear();

0 commit comments

Comments
 (0)