fix: protect CoinJoinWalletManager::m_wallet_manager_map with Mutex, avoid data race, partial bitcoin#22868 by kwvg · Pull Request #6192 · dashpay/dash

kwvg · 2024-08-10T14:54:12Z

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)

src/coinjoin/client.cpp

src/rpc/coinjoin.cpp

… run multiprocess on CI d5a944d Revert: fix: missing changes from bitcoin#19267 - run multiprocess on CI (pasta) Pull request description: ## Issue being fixed or feature implemented See dashpay#6192 for a discussion around this: for now simply make CI happy again, by not running for multiprocessor (since this is new, and unstable / dev feature, and not something shipped). ## What was done? Revert commit which reintroduces multiprocessor CI while we investigate proper fix ## How Has This Been Tested? CI ## Breaking Changes ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ Top commit has no ACKs. Tree-SHA512: d878b02efc72019e09b09e2482e63dca276824b66272135e00ff340ba60b5b58e5f5ae3feff3b894f08958ca8fc6ce71f91c926f6aaf29a459d6230f129316f6

src/coinjoin/interfaces.cpp

src/wallet/wallet.cpp

src/dsnotificationinterface.cpp

src/coinjoin/client.cpp

src/rpc/coinjoin.cpp

…#22937 and ipc/process 540f687 fix: lock `::cs_main` before accessing `ChainstateManager::m_best_header` (Kittywhiskers Van Gogh) aafded6 fix: compilation error due to rebase error between bitcoin#22937 and ipc/process (Kittywhiskers Van Gogh) Pull request description: ## Issue being fixed or feature implemented CI failure for multiprocess ## What was done? It resolve compilation failure on develop (apply changes from 22937 to ipc/process) See alternate solutions: - #6192 - #6195 ## How Has This Been Tested? Run build locally: ``` make MULTIPROCESS=1 -j10 ./configure --prefix=$(pwd)/depends/x86_64-pc-linux-gnu --enable-suppress-external-warnings --enable-werror --enable-debug --enable-crash-hooks --enable-maintainer-mode --enable-stacktraces --enable-multi-process ``` ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: PastaPastaPasta: utACK 540f687 Tree-SHA512: 75b675e93763e8e53086a10b93516b8ec94418902f9be2de738517176cc835c0dad25c286426089a5327a9c13d1787b89debda2c6025cb598489204d7d5af2cf

UdjinM6 · 2024-08-12T19:11:27Z

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

src/coinjoin/client.h

PastaPastaPasta · 2024-08-20T19:52:55Z

src/coinjoin/client.h

should we use std::for_each here?

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);

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

@UdjinM6, thoughts?

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

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 · 2024-08-20T19:57:39Z

Can you squash these two commits?

UdjinM6 · 2024-08-25T18:17:20Z

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) {

Avoid TSan-reported data race ``` WARNING: ThreadSanitizer: data race (pid=374820) Read of size 8 at 0x7b140002ce10 by thread T12: #0 _M_ptr /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/unique_ptr.h:154:42 (dashd+0xb58e08) (BuildId: c3fdce9f7e778985a4fb0968ff4506d9ad24d408) #1 get /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/unique_ptr.h:361:21 (dashd+0xb58e08) #2 operator-> /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/unique_ptr.h:355:9 (dashd+0xb58e08) #3 CoinJoinWalletManager::DoMaintenance() /src/dash/src/coinjoin/client.cpp:1907:9 (dashd+0xb58e08) [...] Previous write of size 8 at 0x7b140002ce10 by thread T17 (mutexes: write M0): #0 operator new(unsigned long) <null> (dashd+0x162657) (BuildId: c3fdce9f7e778985a4fb0968ff4506d9ad24d408) #1 allocate /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/ext/new_allocator.h:114:27 (dashd+0xb772b4) (BuildId: c3fdce9f7e778985a4fb0968ff4506d9ad24d408) #2 allocate /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/alloc_traits.h:443:20 (dashd+0xb772b4) #3 _M_get_node /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/stl_tree.h:580:16 (dashd+0xb772b4) [...] dashpay#8 CoinJoinWalletManager::Add(CWallet&) /src/dash/src/coinjoin/client.cpp:1898:26 (dashd+0xb58c73) (BuildId: c3fdce9f7e778985a4fb0968ff4506d9ad24d408) [...] SUMMARY: ThreadSanitizer: data race [...] ```

excludes changes in `wallet/context.h` due to `::vpwallets` (and thus, `cs_wallets`) not being deglobalized yet

UdjinM6

LGTM (to be merged via merge commit)

light-ACK 87d775d

kwvg added this to the 21.2 milestone Aug 10, 2024

kwvg changed the title ~~fix: resolve compile failure in multiprocess build due to missing std::filesystem preparation changes~~ fix: resolve compile failure in multiprocess build due to missing std::filesystem preparation changes, data races Aug 10, 2024

kwvg force-pushed the 6143_fixup branch from 43bf359 to a43ab1c Compare August 10, 2024 20:15

kwvg requested review from PastaPastaPasta, UdjinM6 and knst August 10, 2024 21:21

kwvg marked this pull request as ready for review August 10, 2024 21:22

kwvg force-pushed the 6143_fixup branch 3 times, most recently from d492d1c to 9e50226 Compare August 11, 2024 08:34

PastaPastaPasta reviewed Aug 11, 2024

View reviewed changes

src/coinjoin/client.cpp Outdated Show resolved Hide resolved

kwvg force-pushed the 6143_fixup branch from 9e50226 to abf3a1d Compare August 11, 2024 08:44

kwvg requested a review from PastaPastaPasta August 11, 2024 08:44

PastaPastaPasta reviewed Aug 11, 2024

View reviewed changes

src/rpc/coinjoin.cpp Outdated Show resolved Hide resolved

PastaPastaPasta mentioned this pull request Aug 11, 2024

revert: fix: missing changes from bitcoin#19267 - run multiprocess on CI #6195

Closed

5 tasks

kwvg requested a review from PastaPastaPasta August 11, 2024 11:39