Skip to content

Commit b36dfad

Browse files
kwvgkwvg
committed
wallet: add missing cs_main locks implied by older logic, resolve inconsistent lock order
1 parent 99471e6 commit b36dfad

File tree

8 files changed

+52
-29
lines changed

8 files changed

+52
-29
lines changed

‎src/coinjoin/client.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1483,7 +1483,7 @@ bool CCoinJoinClientSession::MakeCollateralAmounts(const CompactTallyItem& tally
14831483

14841484
bool CCoinJoinClientSession::CreateCollateralTransaction(CMutableTransaction& txCollateral, std::string& strReason)
14851485
{
1486-
LOCK(mixingWallet.cs_wallet);
1486+
LOCK2(mixingWallet.cs_wallet, cs_main);
14871487

14881488
std::vector<COutput> vCoins;
14891489
CCoinControl coin_control;

‎src/coinjoin/util.cpp‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -271,8 +271,11 @@ bool CTransactionBuilder::Commit(bilingual_str& strResult)
271271
}
272272

273273
CTransactionRef tx;
274-
if (!pwallet->CreateTransaction(vecSend, tx, nFeeRet, nChangePosRet, strResult, coinControl)) {
275-
return false;
274+
{
275+
LOCK2(pwallet->cs_wallet, cs_main);
276+
if (!pwallet->CreateTransaction(vecSend, tx, nFeeRet, nChangePosRet, strResult, coinControl)) {
277+
return false;
278+
}
276279
}
277280

278281
CAmount nAmountLeft = GetAmountLeft();
@@ -306,7 +309,10 @@ bool CTransactionBuilder::Commit(bilingual_str& strResult)
306309
return false;
307310
}
308311

309-
pwallet->CommitTransaction(tx, {}, {});
312+
{
313+
LOCK2(pwallet->cs_wallet, cs_main);
314+
pwallet->CommitTransaction(tx, {}, {});
315+
}
310316

311317
fKeepKeys = true;
312318

‎src/interfaces/wallet.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -289,7 +289,7 @@ class WalletImpl : public Wallet
289289
WalletValueMap value_map,
290290
WalletOrderForm order_form) override
291291
{
292-
LOCK(m_wallet->cs_wallet);
292+
LOCK2(m_wallet->cs_wallet, cs_main);
293293
CReserveKey m_key(m_wallet.get());
294294
m_wallet->CommitTransaction(std::move(tx), std::move(value_map), std::move(order_form));
295295
}

‎src/qt/test/wallettests.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ void TestGUI(interfaces::Node& node)
121121
wallet->SetLastBlockProcessed(105, ::ChainActive().Tip()->GetBlockHash());
122122
}
123123
{
124-
LOCK(cs_main);
124+
LOCK2(wallet->cs_wallet, cs_main);
125125
WalletRescanReserver reserver(wallet.get());
126126
reserver.reserve();
127127
CWallet::ScanResult result = wallet->ScanForWalletTransactions(wallet->chain().getBlockHash(0), {} /* stop_block */, reserver, true /* fUpdate */);

‎src/rpc/governance.cpp‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -244,7 +244,10 @@ static UniValue gobject_prepare(const JSONRPCRequest& request)
244244
}
245245

246246
// -- send the tx to the network
247-
pwallet->CommitTransaction(tx, {}, {});
247+
{
248+
LOCK2(pwallet->cs_wallet, cs_main);
249+
pwallet->CommitTransaction(tx, {}, {});
250+
}
248251

249252
LogPrint(BCLog::GOBJECT, "gobject_prepare -- GetDataAsPlainString = %s, hash = %s, txid = %s\n",
250253
govobj.GetDataAsPlainString(), govobj.GetHash().ToString(), tx->GetHash().ToString());

‎src/wallet/test/coinjoin_tests.cpp‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -73,15 +73,14 @@ class CTransactionBuilderTestSetup : public TestChain100Setup
7373
wallet->LoadWallet(firstRun);
7474
AddWallet(wallet);
7575
{
76-
LOCK(wallet->cs_wallet);
76+
LOCK2(wallet->cs_wallet, cs_main);
7777
wallet->AddKeyPubKey(coinbaseKey, coinbaseKey.GetPubKey());
7878
wallet->SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash());
79+
WalletRescanReserver reserver(wallet.get());
80+
reserver.reserve();
81+
CWallet::ScanResult result = wallet->ScanForWalletTransactions(::ChainActive().Genesis()->GetBlockHash(), {} /* stop_block */, reserver, true /* fUpdate */);
82+
BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::SUCCESS);
7983
}
80-
WalletRescanReserver reserver(wallet.get());
81-
reserver.reserve();
82-
83-
CWallet::ScanResult result = wallet->ScanForWalletTransactions(::ChainActive().Genesis()->GetBlockHash(), {} /* stop_block */, reserver, true /* fUpdate */);
84-
BOOST_CHECK_EQUAL(result.status, CWallet::ScanResult::SUCCESS);
8584
}
8685

8786
~CTransactionBuilderTestSetup()
@@ -103,7 +102,7 @@ class CTransactionBuilderTestSetup : public TestChain100Setup
103102
blocktx = CMutableTransaction(*it->second.tx);
104103
}
105104
CreateAndProcessBlock({blocktx}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
106-
LOCK(wallet->cs_wallet);
105+
LOCK2(wallet->cs_wallet, cs_main);
107106
wallet->SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash());
108107
CWalletTx::Confirmation confirm(CWalletTx::Status::CONFIRMED, ::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash(), 1);
109108
it->second.m_confirm = confirm;
@@ -122,10 +121,11 @@ class CTransactionBuilderTestSetup : public TestChain100Setup
122121
BOOST_CHECK(destKey.GetReservedKey(pubKey, false));
123122
tallyItem.txdest = pubKey.GetID();
124123
for (CAmount nAmount : vecAmounts) {
124+
BOOST_CHECK(wallet->CreateTransaction({{GetScriptForDestination(tallyItem.txdest), nAmount, false}}, tx, nFeeRet, nChangePosRet, strError, coinControl));
125125
{
126-
BOOST_CHECK(wallet->CreateTransaction({{GetScriptForDestination(tallyItem.txdest), nAmount, false}}, tx, nFeeRet, nChangePosRet, strError, coinControl));
126+
LOCK2(wallet->cs_wallet, cs_main);
127+
wallet->CommitTransaction(tx, {}, {});
127128
}
128-
wallet->CommitTransaction(tx, {}, {});
129129
AddTxToChain(tx->GetHash());
130130
for (size_t n = 0; n < tx->vout.size(); ++n) {
131131
if (nChangePosRet != -1 && int(n) == nChangePosRet) {

‎src/wallet/test/wallet_tests.cpp‎

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,10 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
8787
}
8888

8989
// Prune the older block file.
90-
EnsureChainman(m_node).PruneOneBlockFile(oldTip->GetBlockPos().nFile);
90+
{
91+
LOCK(cs_main);
92+
EnsureChainman(m_node).PruneOneBlockFile(oldTip->GetBlockPos().nFile);
93+
}
9194
UnlinkPrunedFiles({oldTip->GetBlockPos().nFile});
9295

9396
// Verify ScanForWalletTransactions only picks transactions in the new block
@@ -110,7 +113,10 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
110113
}
111114

112115
// Prune the remaining block file.
113-
EnsureChainman(m_node).PruneOneBlockFile(newTip->GetBlockPos().nFile);
116+
{
117+
LOCK(cs_main);
118+
EnsureChainman(m_node).PruneOneBlockFile(newTip->GetBlockPos().nFile);
119+
}
114120
UnlinkPrunedFiles({newTip->GetBlockPos().nFile});
115121

116122
// Verify ScanForWalletTransactions scans no blocks.
@@ -145,7 +151,10 @@ BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup)
145151
LockAnnotation lock(::cs_main);
146152

147153
// Prune the older block file.
148-
EnsureChainman(m_node).PruneOneBlockFile(oldTip->GetBlockPos().nFile);
154+
{
155+
LOCK(cs_main);
156+
EnsureChainman(m_node).PruneOneBlockFile(oldTip->GetBlockPos().nFile);
157+
}
149158
UnlinkPrunedFiles({oldTip->GetBlockPos().nFile});
150159

151160
// Verify importmulti RPC returns failure for a key whose creation time is
@@ -397,10 +406,11 @@ class ListCoinsTestingSetup : public TestChain100Setup
397406
int changePos = -1;
398407
bilingual_str error;
399408
CCoinControl dummy;
409+
BOOST_CHECK(wallet->CreateTransaction({recipient}, tx, fee, changePos, error, dummy));
400410
{
401-
BOOST_CHECK(wallet->CreateTransaction({recipient}, tx, fee, changePos, error, dummy));
411+
LOCK2(wallet->cs_wallet, cs_main);
412+
wallet->CommitTransaction(tx, {}, {});
402413
}
403-
wallet->CommitTransaction(tx, {}, {});
404414
CMutableTransaction blocktx;
405415
{
406416
LOCK(wallet->cs_wallet);
@@ -611,7 +621,10 @@ class CreateTransactionTestSetup : public TestChain100Setup
611621
bilingual_str strError;
612622
CCoinControl coinControl;
613623
BOOST_CHECK(wallet->CreateTransaction(GetRecipients(vecEntries), tx, nFeeRet, nChangePosRet, strError, coinControl));
614-
wallet->CommitTransaction(tx, {}, {});
624+
{
625+
LOCK2(wallet->cs_wallet, cs_main);
626+
wallet->CommitTransaction(tx, {}, {});
627+
}
615628
CMutableTransaction blocktx;
616629
{
617630
LOCK(wallet->cs_wallet);
@@ -945,16 +958,16 @@ BOOST_FIXTURE_TEST_CASE(select_coins_grouped_by_addresses, ListCoinsTestingSetup
945958
int changePos = -1;
946959
bilingual_str error;
947960
CCoinControl dummy;
961+
BOOST_CHECK(wallet->CreateTransaction({CRecipient{GetScriptForRawPubKey({}), 2 * COIN, true /* subtract fee */}},
962+
tx1, fee, changePos, error, dummy));
963+
BOOST_CHECK(wallet->CreateTransaction({CRecipient{GetScriptForRawPubKey({}), 1 * COIN, true /* subtract fee */}},
964+
tx2, fee, changePos, error, dummy));
948965
{
949-
BOOST_CHECK(wallet->CreateTransaction({CRecipient{GetScriptForRawPubKey({}), 2 * COIN, true /* subtract fee */}},
950-
tx1, fee, changePos, error, dummy));
951-
BOOST_CHECK(wallet->CreateTransaction({CRecipient{GetScriptForRawPubKey({}), 1 * COIN, true /* subtract fee */}},
952-
tx2, fee, changePos, error, dummy));
966+
LOCK2(wallet->cs_wallet, cs_main);
967+
wallet->CommitTransaction(tx1, {}, {});
953968
}
954-
wallet->CommitTransaction(tx1, {}, {});
955969
BOOST_CHECK_EQUAL(wallet->GetAvailableBalance(), 0);
956970
CreateAndProcessBlock({CMutableTransaction(*tx2)}, GetScriptForRawPubKey({}));
957-
958971
{
959972
LOCK2(wallet->cs_wallet, cs_main);
960973
wallet->SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash());

‎src/wallet/wallet.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1527,6 +1527,7 @@ void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, c
15271527

15281528
void CWallet::SyncTransaction(const CTransactionRef& ptx, CWalletTx::Confirmation confirm, bool update_tx)
15291529
{
1530+
LOCK2(cs_wallet, cs_main);
15301531
if (!AddToWalletIfInvolvingMe(ptx, confirm, update_tx))
15311532
return; // Not one of ours
15321533

@@ -5631,7 +5632,7 @@ bool CWalletTx::IsLockedByInstantSend() const
56315632
bool CWalletTx::IsChainLocked() const
56325633
{
56335634
if (!fIsChainlocked) {
5634-
AssertLockHeld(cs_main);
5635+
LOCK(cs_main);
56355636
CBlockIndex* pIndex = LookupBlockIndex(m_confirm.hashBlock);
56365637
if (pIndex != nullptr) {
56375638
fIsChainlocked = llmq::chainLocksHandler->HasChainLock(pIndex->nHeight, m_confirm.hashBlock);

0 commit comments

Comments
 (0)