fix: coinjoin wallet manager lock order

src/coinjoin/interfaces.cpp

@@ -13,6 +13,7 @@
 
 #include <memory>
 #include <string>
+#include <utility>
 
 using node::NodeContext;
 using wallet::CWallet;
@@ -22,55 +23,55 @@ namespace {
 
 class CoinJoinClientImpl : public interfaces::CoinJoin::Client
 {
-    CCoinJoinClientManager& m_clientman;
+    std::shared_ptr<CCoinJoinClientManager> m_clientman;
 
 public:
-    explicit CoinJoinClientImpl(CCoinJoinClientManager& clientman)
-        : m_clientman(clientman) {}
+    explicit CoinJoinClientImpl(std::shared_ptr<CCoinJoinClientManager> clientman)
+        : m_clientman(std::move(clientman)) {}
 
     void resetCachedBlocks() override
     {
-        m_clientman.nCachedNumBlocks = std::numeric_limits<int>::max();
+        m_clientman->nCachedNumBlocks = std::numeric_limits<int>::max();
     }
     void resetPool() override
     {
-        m_clientman.ResetPool();
+        m_clientman->ResetPool();
     }
     void disableAutobackups() override
     {
-        m_clientman.fCreateAutoBackups = false;
+        m_clientman->fCreateAutoBackups = false;
     }
     int getCachedBlocks() override
     {
-        return m_clientman.nCachedNumBlocks;
+        return m_clientman->nCachedNumBlocks;
     }
     void getJsonInfo(UniValue& obj) override
     {
-        return m_clientman.GetJsonInfo(obj);
+        return m_clientman->GetJsonInfo(obj);
     }
     std::string getSessionDenoms() override
     {
-        return m_clientman.GetSessionDenoms();
+        return m_clientman->GetSessionDenoms();
     }
     std::vector<std::string> getSessionStatuses() override
     {
-        return m_clientman.GetStatuses();
+        return m_clientman->GetStatuses();
     }
     void setCachedBlocks(int nCachedBlocks) override
     {
-       m_clientman.nCachedNumBlocks = nCachedBlocks;
+        m_clientman->nCachedNumBlocks = nCachedBlocks;
     }
     bool isMixing() override
     {
-        return m_clientman.IsMixing();
+        return m_clientman->IsMixing();
     }
     bool startMixing() override
     {
-        return m_clientman.StartMixing();
+        return m_clientman->StartMixing();
     }
     void stopMixing() override
     {
-        m_clientman.StopMixing();
+        m_clientman->StopMixing();
     }
 };
 
@@ -112,7 +113,7 @@ class CoinJoinLoaderImpl : public interfaces::CoinJoin::Loader
     std::unique_ptr<interfaces::CoinJoin::Client> GetClient(const std::string& name) override
     {
         auto clientman = manager().getClient(name);
-        return clientman ? std::make_unique<CoinJoinClientImpl>(*clientman) : nullptr;
+        return clientman ? std::make_unique<CoinJoinClientImpl>(std::move(clientman)) : nullptr;
     }
 
     NodeContext& m_node;

src/coinjoin/walletman.cpp

@@ -22,8 +22,10 @@
 #include <coinjoin/client.h>
 #endif // ENABLE_WALLET
 
-#include <memory>
+#include <algorithm>
+#include <map>
 #include <ranges>
+#include <vector>
 
 #ifdef ENABLE_WALLET
 class CJWalletManagerImpl final : public CJWalletManager
@@ -39,7 +41,7 @@ class CJWalletManagerImpl final : public CJWalletManager
 
 public:
     bool hasQueue(const uint256& hash) const override;
-    CCoinJoinClientManager* getClient(const std::string& name) override EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet_manager_map);
+    std::shared_ptr<CCoinJoinClientManager> getClient(const std::string& name) override EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet_manager_map);
     MessageProcessingResult processMessage(CNode& peer, CChainState& chainstate, CConnman& connman, CTxMemPool& mempool,
                                            std::string_view msg_type, CDataStream& vRecv) override EXCLUSIVE_LOCKS_REQUIRED(!cs_ProcessDSQueue, !cs_wallet_manager_map);
     std::optional<CCoinJoinQueue> getQueueFromHash(const uint256& hash) const override;
@@ -70,27 +72,31 @@ class CJWalletManagerImpl final : public CJWalletManager
     mutable Mutex cs_ProcessDSQueue;
 
     mutable Mutex cs_wallet_manager_map;
-    std::map<const std::string, std::unique_ptr<CCoinJoinClientManager>> m_wallet_manager_map GUARDED_BY(cs_wallet_manager_map);
+    // The map lock only protects the index. Managers are shared so snapshots and
+    // interface clients can finish without running manager or wallet code under it.
+    std::map<const std::string, std::shared_ptr<CCoinJoinClientManager>> m_wallet_manager_map GUARDED_BY(cs_wallet_manager_map);
 
     void DoMaintenance(CConnman& connman) EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet_manager_map);
 
     [[nodiscard]] MessageProcessingResult ProcessDSQueue(NodeId from, CConnman& connman, std::string_view msg_type,
                                                          CDataStream& vRecv) EXCLUSIVE_LOCKS_REQUIRED(!cs_ProcessDSQueue, !cs_wallet_manager_map);
 
+    std::vector<std::shared_ptr<CCoinJoinClientManager>> GetClientManagersSnapshot() const EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet_manager_map);
+
     template <typename Callable>
     void ForEachCJClientMan(Callable&& func) EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet_manager_map)
     {
-        LOCK(cs_wallet_manager_map);
-        for (auto& [_, clientman] : m_wallet_manager_map) {
+        for (const auto& clientman : GetClientManagersSnapshot()) {
             func(*clientman);
         }
     }
 
     template <typename Callable>
     bool ForAnyCJClientMan(Callable&& func) EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet_manager_map)
     {
-        LOCK(cs_wallet_manager_map);
-        return std::ranges::any_of(m_wallet_manager_map, [&](auto& pair) { return func(*pair.second); });
+        return std::ranges::any_of(GetClientManagersSnapshot(), [&](const auto& clientman) {
+            return func(*clientman);
+        });
     }
 };
 
@@ -111,9 +117,10 @@ CJWalletManagerImpl::CJWalletManagerImpl(ChainstateManager& chainman, CDetermini
 
 CJWalletManagerImpl::~CJWalletManagerImpl()
 {
-    LOCK(cs_wallet_manager_map);
-    for (auto& [_, clientman] : m_wallet_manager_map) {
-        clientman.reset();
+    decltype(m_wallet_manager_map) clientmans;
+    {
+        LOCK(cs_wallet_manager_map);
+        clientmans.swap(m_wallet_manager_map);
     }
 }
 
@@ -140,11 +147,22 @@ bool CJWalletManagerImpl::hasQueue(const uint256& hash) const
     return false;
 }
 
-CCoinJoinClientManager* CJWalletManagerImpl::getClient(const std::string& name)
+std::vector<std::shared_ptr<CCoinJoinClientManager>> CJWalletManagerImpl::GetClientManagersSnapshot() const
+{
+    LOCK(cs_wallet_manager_map);
+    std::vector<std::shared_ptr<CCoinJoinClientManager>> ret;
+    ret.reserve(m_wallet_manager_map.size());
+    for (const auto& [_, clientman] : m_wallet_manager_map) {
+        ret.emplace_back(clientman);
+    }
+    return ret;
+}
+
+std::shared_ptr<CCoinJoinClientManager> CJWalletManagerImpl::getClient(const std::string& name)
 {
     LOCK(cs_wallet_manager_map);
     auto it = m_wallet_manager_map.find(name);
-    return (it != m_wallet_manager_map.end()) ? it->second.get() : nullptr;
+    return (it != m_wallet_manager_map.end()) ? it->second : nullptr;
 }
 
 std::optional<CCoinJoinQueue> CJWalletManagerImpl::getQueueFromHash(const uint256& hash) const
@@ -172,32 +190,40 @@ std::vector<CDeterministicMNCPtr> CJWalletManagerImpl::getMixingMasternodes()
 
 void CJWalletManagerImpl::addWallet(const std::shared_ptr<wallet::CWallet>& wallet)
 {
+    const auto name{wallet->GetName()};
+    auto clientman{std::make_shared<CCoinJoinClientManager>(wallet, m_dmnman, m_mn_metaman, m_mn_sync, m_isman,
+                                                            m_queueman.get())};
+
     LOCK(cs_wallet_manager_map);
-    m_wallet_manager_map.try_emplace(wallet->GetName(),
-                                     std::make_unique<CCoinJoinClientManager>(wallet, m_dmnman, m_mn_metaman, m_mn_sync,
-                                                                              m_isman, m_queueman.get()));
+    m_wallet_manager_map.try_emplace(name, std::move(clientman));
 }
 
 void CJWalletManagerImpl::flushWallet(const std::string& name)
 {
-    auto* clientman = Assert(getClient(name));
+    auto clientman = Assert(getClient(name));
     clientman->ResetPool();
     clientman->StopMixing();
 }
 
 void CJWalletManagerImpl::removeWallet(const std::string& name)
 {
-    LOCK(cs_wallet_manager_map);
-    m_wallet_manager_map.erase(name);
+    std::shared_ptr<CCoinJoinClientManager> clientman;
+    {
+        LOCK(cs_wallet_manager_map);
+        auto it = m_wallet_manager_map.find(name);
+        if (it != m_wallet_manager_map.end()) {
+            clientman = std::move(it->second);
+            m_wallet_manager_map.erase(it);
+        }
+    }
 }
 
 void CJWalletManagerImpl::DoMaintenance(CConnman& connman)
 {
     if (m_queueman && m_mn_sync.IsBlockchainSynced() && !ShutdownRequested()) {
         m_queueman->CheckQueue();
     }
-    LOCK(cs_wallet_manager_map);
-    for (auto& [_, clientman] : m_wallet_manager_map) {
+    for (const auto& clientman : GetClientManagersSnapshot()) {
         clientman->DoMaintenance(m_chainman, connman, m_mempool);
     }
 }

src/coinjoin/walletman.h

@@ -10,8 +10,11 @@
 
 #include <validationinterface.h>
 
+#include <memory>
 #include <optional>
+#include <string>
 #include <string_view>
+#include <vector>
 
 class CBlockIndex;
 class CChainState;
@@ -47,7 +50,7 @@ class CJWalletManager : public CValidationInterface
 
 public:
     virtual bool hasQueue(const uint256& hash) const = 0;
-    virtual CCoinJoinClientManager* getClient(const std::string& name) = 0;
+    virtual std::shared_ptr<CCoinJoinClientManager> getClient(const std::string& name) = 0;
     virtual MessageProcessingResult processMessage(CNode& peer, CChainState& chainstate, CConnman& connman,
                                                    CTxMemPool& mempool, std::string_view msg_type, CDataStream& vRecv) = 0;
     virtual std::optional<CCoinJoinQueue> getQueueFromHash(const uint256& hash) const = 0;

src/wallet/test/coinjoin_tests.cpp

@@ -221,13 +221,24 @@ class CTransactionBuilderTestSetup : public TestChain100Setup
 
 BOOST_FIXTURE_TEST_CASE(coinjoin_manager_start_stop_tests, CTransactionBuilderTestSetup)
 {
-    auto& cj_man = *Assert(m_node.cj_walletman->getClient(""));
-    BOOST_CHECK_EQUAL(cj_man.IsMixing(), false);
-    BOOST_CHECK_EQUAL(cj_man.StartMixing(), true);
-    BOOST_CHECK_EQUAL(cj_man.IsMixing(), true);
-    BOOST_CHECK_EQUAL(cj_man.StartMixing(), false);
-    cj_man.StopMixing();
-    BOOST_CHECK_EQUAL(cj_man.IsMixing(), false);
+    auto cj_man = Assert(m_node.cj_walletman->getClient(""));
+    BOOST_CHECK_EQUAL(cj_man->IsMixing(), false);
+    BOOST_CHECK_EQUAL(cj_man->StartMixing(), true);
+    BOOST_CHECK_EQUAL(cj_man->IsMixing(), true);
+    BOOST_CHECK_EQUAL(cj_man->StartMixing(), false);
+    cj_man->StopMixing();
+    BOOST_CHECK_EQUAL(cj_man->IsMixing(), false);
+}
+
+BOOST_FIXTURE_TEST_CASE(coinjoin_newkeypool_stops_mixing_without_lock_inversion, CTransactionBuilderTestSetup)
+{
+    auto cj_man = Assert(m_node.cj_walletman->getClient(""));
+    BOOST_REQUIRE(cj_man->StartMixing());
+    {
+        LOCK(wallet->cs_wallet);
+        BOOST_REQUIRE(wallet->GetLegacyScriptPubKeyMan()->NewKeyPool());
+    }
+    BOOST_CHECK_EQUAL(cj_man->IsMixing(), false);
 }
 
 BOOST_FIXTURE_TEST_CASE(CTransactionBuilderTest, CTransactionBuilderTestSetup)

test/functional/rpc_coinjoin.py

@@ -50,6 +50,14 @@ def run_test(self):
         self.test_coinjoinsalt(w1)
         w1.unloadwallet()
 
+        if not self.options.descriptors:
+            node.createwallet(wallet_name='w_keypool', blank=False, disable_private_keys=False)
+            w_keypool = node.get_wallet_rpc('w_keypool')
+            self.test_newkeypool_stops_mixing(w_keypool)
+            w_keypool.unloadwallet()
+        else:
+            self.log.info('Skip "newkeypool" regression for descriptor wallets')
+
         node.createwallet(wallet_name='w2', blank=True, disable_private_keys=True)
         w2 = node.get_wallet_rpc('w2')
         self.test_coinjoinsalt_disabled(w2)
@@ -85,6 +93,13 @@ def test_coinjoin_start_stop(self, node):
         # Reset mix session
         assert_equal(node.coinjoin('reset'), "Mixing was reset")
 
+    def test_newkeypool_stops_mixing(self, node):
+        self.log.info('"newkeypool" should stop mixing')
+        node.coinjoin('start')
+        assert_equal(node.getcoinjoininfo()['running'], True)
+        node.newkeypool()
+        assert_equal(node.getcoinjoininfo()['running'], False)
+
     def test_setcoinjoinamount(self, node):
         self.log.info('"setcoinjoinamount" should update mixing target')
         # Test normal and large values