fix: protect CoinJoinWalletManager::m_wallet_manager_map with Mutex, avoid data race, partial bitcoin#22868

[kwvg]

Additional Information

This pull request aims to deal with regressions (build) spotted in develop after the merger of dash#6143, namely, a failure to build the multiprocess variant and two data races, one involving ChainstateManager::m_best_header and another involving CoinJoinWalletManager::m_wallet_manager_map.

• Fixes for the build failure and the first data race (b136742 and 9e7c685), have been spun off into dash#6199 and merged

• The second data race is being avoided by protected m_wallet_manager_map with a new RecursiveMutex, cs_wallet_manager_map (the contents of this PR). A version of these changes are available using a regular Mutex but prove far more cumbersome (source) when taking practicalities into account (comment). A Mutex version is now available courtesy of UdjinM6 (see patch), thanks!

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas (note: N/A)
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation (note: N/A)
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

[UdjinM6]

my bad, should've run linter locally 🙈 pls also check 2d6612a

[PastaPastaPasta]

should we use std::for_each here?

[kwvg]

We would not be able to do the below snippet because the function isn't taking in a pair but one only one half of it

std::for_each(m_wallet_manager_map.begin(), m_wallet_manager_map.end(), func);

[PastaPastaPasta]

could do something ```suggestion
ranges::for_each(m_wallet_manager_map, [&](auto [_, clientman]){func(clientman)});

[kwvg]

@UdjinM6, thoughts?

[UdjinM6]

can't use structured bindings there, would be more like 019a780 instead

[knst]

I don't like to use std::for_each if it's not super trivial.

ranges::for_each(m_wallet_manager_map, [&](auto [_, clientman]){func(clientman)});

it looks like extra more difficult to read, I disagree with this suggestment.

For-each has been a great option when there was no range-loop in C++! But in this particular case it looks difficult to read, difficult to understand. That's a case when 2 lines better than one!

using any_of below makes sense, but not here.

[PastaPastaPasta]

Can you squash these two commits?

[UdjinM6]

POTENTIAL DEADLOCK DETECTED
Previous lock order was:
(2) 'walletInstance->cs_wallet' in wallet/wallet.cpp:4952 (in thread 'qt-init')
(1) 'cs_wallet_manager_map' in coinjoin/client.cpp:1903 (in thread 'qt-init')
Current lock order is:
(1) 'cs_wallet_manager_map' in coinjoin/client.cpp:1913 (in thread 'scheduler')
'cs_deqsessions' in coinjoin/client.cpp:998 (in thread 'scheduler')
(2) 'm_wallet.cs_wallet' in coinjoin/client.cpp:804 (in thread 'scheduler')

can be fixed by backporting bitcoin#22868 or via a temporary fix

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index f648fc2ced..6a33585ede 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4949,8 +4949,6 @@ 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);
-
if (chain && !AttachChain(walletInstance, *chain, error, warnings)) {
return nullptr;
}
@@ -4959,6 +4957,8 @@ std::shared_ptr<CWallet> CWallet::Create(interfaces::Chain* chain, interfaces::C
coinjoin_loader->AddWallet(*walletInstance);
}
+ LOCK(walletInstance->cs_wallet);
+
{
LOCK(cs_wallets);
for (auto& load_wallet : g_load_wallet_fns) {

[UdjinM6]

LGTM (to be merged via merge commit)

light-ACK 87d775d