Skip to content

Commit

Permalink
wallet: avoid locking cs_wallet throughout Create's entire scope
Browse files Browse the repository at this point in the history
Implement portions of f13a22a (bitcoin#22868) to sidestep inconsistent
lock order by further scoping `cs_wallet` and exiting that scope before
locking `cs_wallet_manager_map`

```
POTENTIAL DEADLOCK DETECTED
  Previous lock order was:
    (2) 'walletInstance->cs_wallet' in wallet/wallet.cpp:4932 (in thread 'init')
    (1) 'm_walletman.cs_wallet_manager_map' in coinjoin/interfaces.cpp:72 (in thread 'init')
  Current lock order is:
    (1) 'm_walletman.cs_wallet_manager_map' in coinjoin/interfaces.cpp:72 (in thread 'init')
    (2) 'cs_wallet' in wallet/wallet.cpp:5530 (in thread 'init')

```

Additionally, don't bother to see if the wallet is locked when doing a
re-init of CoinJoin, just stop mixing (it'll restart anyway if
`-coinjoinautostart` is set) because we're working from wallet code and
it's very likely that calls to the CoinJoin interface will already hold
a `cs_wallet`.

Which is a problem because re-init'ing involves going through every
wallet and stopping the mix process. If we check to see if the wallet is
locked first, it involves locking `cs_wallet` briefly.

If we're already holding `cs_wallet`, the order is `cs_wallet` and then
`cs_wallet_manager_map` but if we *don't* hold any `cs_wallet`, then the
order will reverse because we still need to lock `cs_wallet` to see if
it's locked, which spells trouble for us.
  • Loading branch information
kwvg committed Aug 11, 2024
1 parent b89457d commit abf3a1d
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 11 deletions.
6 changes: 2 additions & 4 deletions src/wallet/init.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -211,10 +211,8 @@ void WalletInit::InitCoinJoinSettings(const CoinJoinWalletManager& cjwalletman)
bool fAutoStart = gArgs.GetBoolArg("-coinjoinautostart", DEFAULT_COINJOIN_AUTOSTART);
for (auto& pwallet : GetWallets()) {
auto manager = cjwalletman.Get(pwallet->GetName());
assert(manager != nullptr);
if (pwallet->IsLocked()) {
manager->StopMixing();
} else if (fAutoStart) {
Assert(manager)->StopMixing();
if (fAutoStart) {
manager->StartMixing();
}
}
Expand Down
6 changes: 1 addition & 5 deletions src/wallet/test/wallet_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1298,18 +1298,14 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
// deadlock during the sync and simulates a new block notification happening
// as soon as possible.
addtx_count = 0;
auto handler = HandleLoadWallet([&](std::unique_ptr<interfaces::Wallet> wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet->wallet()->cs_wallet, cs_wallets) {
auto handler = HandleLoadWallet([&](std::unique_ptr<interfaces::Wallet> wallet) {
BOOST_CHECK(rescan_completed);
m_coinbase_txns.push_back(CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
block_tx = TestSimpleSpend(*m_coinbase_txns[2], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey()));
m_coinbase_txns.push_back(CreateAndProcessBlock({block_tx}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
mempool_tx = TestSimpleSpend(*m_coinbase_txns[3], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey()));
BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, false, error));
LEAVE_CRITICAL_SECTION(cs_wallets);
LEAVE_CRITICAL_SECTION(wallet->wallet()->cs_wallet);
SyncWithValidationInterfaceQueue();
ENTER_CRITICAL_SECTION(wallet->wallet()->cs_wallet);
ENTER_CRITICAL_SECTION(cs_wallets);
});
wallet = TestLoadWallet(m_node);
BOOST_CHECK_EQUAL(addtx_count, 4);
Expand Down
6 changes: 4 additions & 2 deletions src/wallet/wallet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4929,6 +4929,7 @@ std::shared_ptr<CWallet> CWallet::Create(interfaces::Chain& chain, interfaces::C
// Try to top up keypool. No-op if the wallet is locked.
walletInstance->TopUpKeyPool();

{
LOCK(walletInstance->cs_wallet);

// Register wallet with validationinterface. It's done before rescan to avoid
Expand Down Expand Up @@ -5009,6 +5010,7 @@ std::shared_ptr<CWallet> CWallet::Create(interfaces::Chain& chain, interfaces::C
walletInstance->chainStateFlushed(chain.getTipLocator());
walletInstance->GetDatabase().IncrementUpdateCounter();
}
} // LOCK(walletInstance->cs_wallet)

coinjoin_loader.AddWallet(*walletInstance);

Expand All @@ -5019,9 +5021,9 @@ std::shared_ptr<CWallet> CWallet::Create(interfaces::Chain& chain, interfaces::C
}
}

walletInstance->SetBroadcastTransactions(gArgs.GetBoolArg("-walletbroadcast", DEFAULT_WALLETBROADCAST));

{
LOCK(walletInstance->cs_wallet);
walletInstance->SetBroadcastTransactions(gArgs.GetBoolArg("-walletbroadcast", DEFAULT_WALLETBROADCAST));
walletInstance->WalletLogPrintf("setExternalKeyPool.size() = %u\n", walletInstance->KeypoolCountExternalKeys());
walletInstance->WalletLogPrintf("GetKeyPoolSize() = %u\n", walletInstance->GetKeyPoolSize());
walletInstance->WalletLogPrintf("mapWallet.size() = %u\n", walletInstance->mapWallet.size());
Expand Down

0 comments on commit abf3a1d

Please sign in to comment.