Merge #22868: wallet: Call load handlers without cs_wallet locked

meshcollider · meshcollider · commit 200d97faf2ed · 2021-11-27T22:30:46.000+13:00
f13a22a wallet: Call load handlers without cs_wallet locked (João Barbosa) Pull request description: Don't have `cs_wallet` locked while calling each `context.wallet_load_fns`. A load handler can always lock `cs_wallet` if needed. The lock was added in 1c7e25d to satisfy TSAN. With 44c430f most of the code requiring the lock is in `CWallet::AttachChain`. A comment is added to warn about wallets_mutex and cs_wallet lock ordering. ACKs for top commit: meshcollider: re-utACK f13a22a ryanofsky: Code review ACK f13a22a. Only change since last review is adding a lock order comment jonatack: ACK f13a22a Tree-SHA512: d51976c3aae4bebc2d1997c88edff712d21fc5523801f5614062a10f826e164579973aeb1981bb1cbc243ecff6af3250362f544c02a79e5d135cbbca1704be62
diff --git a/src/wallet/context.h b/src/wallet/context.h
@@ -34,6 +34,8 @@ using LoadWalletFn = std::function<void(std::unique_ptr<interfaces::Wallet> wall
 struct WalletContext {
     interfaces::Chain* chain{nullptr};
     ArgsManager* args{nullptr}; // Currently a raw pointer because the memory is not managed by this struct
+    // It is unsafe to lock this after locking a CWallet::cs_wallet mutex because
+    // this could introduce inconsistent lock ordering and cause deadlocks.
     Mutex wallets_mutex;
     std::vector<std::shared_ptr<CWallet>> wallets GUARDED_BY(wallets_mutex);
     std::list<LoadWalletFn> wallet_load_fns GUARDED_BY(wallets_mutex);
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
@@ -779,18 +779,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(context, [&](std::unique_ptr<interfaces::Wallet> wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet->wallet()->cs_wallet, context.wallets_mutex) {
+    auto handler = HandleLoadWallet(context, [&](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(context.wallets_mutex);
-            LEAVE_CRITICAL_SECTION(wallet->wallet()->cs_wallet);
             SyncWithValidationInterfaceQueue();
-            ENTER_CRITICAL_SECTION(wallet->wallet()->cs_wallet);
-            ENTER_CRITICAL_SECTION(context.wallets_mutex);
         });
     wallet = TestLoadWallet(context);
     BOOST_CHECK_EQUAL(addtx_count, 4);
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
@@ -2763,8 +2763,6 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
     // Try to top up keypool. No-op if the wallet is locked.
     walletInstance->TopUpKeyPool();
 
-    LOCK(walletInstance->cs_wallet);
-
     if (chain && !AttachChain(walletInstance, *chain, rescan_required, error, warnings)) {
         return nullptr;
     }
@@ -2776,9 +2774,9 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri
         }
     }
 
-    walletInstance->SetBroadcastTransactions(args.GetBoolArg("-walletbroadcast", DEFAULT_WALLETBROADCAST));
-
     {
+        LOCK(walletInstance->cs_wallet);
+        walletInstance->SetBroadcastTransactions(args.GetBoolArg("-walletbroadcast", DEFAULT_WALLETBROADCAST));
         walletInstance->WalletLogPrintf("setKeyPool.size() = %u\n",      walletInstance->GetKeyPoolSize());
         walletInstance->WalletLogPrintf("mapWallet.size() = %u\n",       walletInstance->mapWallet.size());
         walletInstance->WalletLogPrintf("m_address_book.size() = %u\n",  walletInstance->m_address_book.size());

Original file line number	Diff line number	Diff line change
`@@ -2763,8 +2763,6 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri`
`2763`	`2763`	`// Try to top up keypool. No-op if the wallet is locked.`
`2764`	`2764`	`walletInstance->TopUpKeyPool();`
`2765`	`2765`
`2766`		`- LOCK(walletInstance->cs_wallet);`
`2767`		`-`
`2768`	`2766`	`if (chain && !AttachChain(walletInstance, *chain, rescan_required, error, warnings)) {`
`2769`	`2767`	`return nullptr;`
`2770`	`2768`	`}`
`@@ -2776,9 +2774,9 @@ std::shared_ptr<CWallet> CWallet::Create(WalletContext& context, const std::stri`
`2776`	`2774`	`}`
`2777`	`2775`	`}`
`2778`	`2776`
`2779`		`- walletInstance->SetBroadcastTransactions(args.GetBoolArg("-walletbroadcast", DEFAULT_WALLETBROADCAST));`
`2780`		`-`
`2781`	`2777`	`{`
	`2778`	`+ LOCK(walletInstance->cs_wallet);`
	`2779`	`+ walletInstance->SetBroadcastTransactions(args.GetBoolArg("-walletbroadcast", DEFAULT_WALLETBROADCAST));`
`2782`	`2780`	`walletInstance->WalletLogPrintf("setKeyPool.size() = %u\n", walletInstance->GetKeyPoolSize());`
`2783`	`2781`	`walletInstance->WalletLogPrintf("mapWallet.size() = %u\n", walletInstance->mapWallet.size());`
`2784`	`2782`	`walletInstance->WalletLogPrintf("m_address_book.size() = %u\n", walletInstance->m_address_book.size());`