diff --git a/src/coinjoin/interfaces.cpp b/src/coinjoin/interfaces.cpp index 8dd645025561..4044b6b46eb5 100644 --- a/src/coinjoin/interfaces.cpp +++ b/src/coinjoin/interfaces.cpp @@ -13,6 +13,7 @@ #include #include +#include using node::NodeContext; using wallet::CWallet; @@ -22,55 +23,55 @@ namespace { class CoinJoinClientImpl : public interfaces::CoinJoin::Client { - CCoinJoinClientManager& m_clientman; + std::shared_ptr m_clientman; public: - explicit CoinJoinClientImpl(CCoinJoinClientManager& clientman) - : m_clientman(clientman) {} + explicit CoinJoinClientImpl(std::shared_ptr clientman) + : m_clientman(std::move(clientman)) {} void resetCachedBlocks() override { - m_clientman.nCachedNumBlocks = std::numeric_limits::max(); + m_clientman->nCachedNumBlocks = std::numeric_limits::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 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 GetClient(const std::string& name) override { auto clientman = manager().getClient(name); - return clientman ? std::make_unique(*clientman) : nullptr; + return clientman ? std::make_unique(std::move(clientman)) : nullptr; } NodeContext& m_node; diff --git a/src/coinjoin/walletman.cpp b/src/coinjoin/walletman.cpp index a54ac93795b4..adbff775bc9d 100644 --- a/src/coinjoin/walletman.cpp +++ b/src/coinjoin/walletman.cpp @@ -22,8 +22,10 @@ #include #endif // ENABLE_WALLET -#include +#include +#include #include +#include #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 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 getQueueFromHash(const uint256& hash) const override; @@ -70,18 +72,21 @@ class CJWalletManagerImpl final : public CJWalletManager mutable Mutex cs_ProcessDSQueue; mutable Mutex cs_wallet_manager_map; - std::map> 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> 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> GetClientManagersSnapshot() const EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet_manager_map); + template 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); } } @@ -89,8 +94,9 @@ class CJWalletManagerImpl final : public CJWalletManager template 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> CJWalletManagerImpl::GetClientManagersSnapshot() const +{ + LOCK(cs_wallet_manager_map); + std::vector> 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 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 CJWalletManagerImpl::getQueueFromHash(const uint256& hash) const @@ -172,23 +190,32 @@ std::vector CJWalletManagerImpl::getMixingMasternodes() void CJWalletManagerImpl::addWallet(const std::shared_ptr& wallet) { + const auto name{wallet->GetName()}; + auto clientman{std::make_shared(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(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 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) @@ -196,8 +223,7 @@ 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); } } diff --git a/src/coinjoin/walletman.h b/src/coinjoin/walletman.h index 720a4bdf2f81..6990175ea1d0 100644 --- a/src/coinjoin/walletman.h +++ b/src/coinjoin/walletman.h @@ -10,8 +10,11 @@ #include +#include #include +#include #include +#include 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 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 getQueueFromHash(const uint256& hash) const = 0; diff --git a/src/wallet/test/coinjoin_tests.cpp b/src/wallet/test/coinjoin_tests.cpp index 61f3c4d2ae0d..8bdaa36d36ef 100644 --- a/src/wallet/test/coinjoin_tests.cpp +++ b/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) diff --git a/test/functional/rpc_coinjoin.py b/test/functional/rpc_coinjoin.py index 578dc1b22c9d..257d12eae7bf 100755 --- a/test/functional/rpc_coinjoin.py +++ b/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