Skip to content
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -502,7 +502,6 @@ libbitcoin_node_a_SOURCES = \
chainlock/signing.cpp \
coinjoin/coinjoin.cpp \
coinjoin/server.cpp \
coinjoin/walletman.cpp \
consensus/tx_verify.cpp \
dbwrapper.cpp \
deploymentstatus.cpp \
Expand Down Expand Up @@ -659,6 +658,7 @@ libbitcoin_wallet_a_SOURCES = \
coinjoin/client.cpp \
coinjoin/interfaces.cpp \
coinjoin/util.cpp \
coinjoin/walletman.cpp \
wallet/bip39.cpp \
wallet/coinjoin.cpp \
wallet/coincontrol.cpp \
Expand Down
33 changes: 17 additions & 16 deletions src/coinjoin/client.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ void CCoinJoinClientManager::ProcessMessage(CNode& peer, CChainState& active_cha
if (!m_mn_sync.IsBlockchainSynced()) return;

if (!CheckDiskSpace(gArgs.GetDataDirNet())) {
ResetPool();
StopMixing();
resetPool();
stopMixing();
WalletCJLogPrint(m_wallet, "CCoinJoinClientManager::ProcessMessage -- Not enough disk space, disabling CoinJoin.\n");
return;
}
Expand Down Expand Up @@ -137,16 +137,16 @@ void CCoinJoinClientSession::ProcessMessage(CNode& peer, CChainState& active_cha
}
}

bool CCoinJoinClientManager::StartMixing() {
bool CCoinJoinClientManager::startMixing() {
bool expected{false};
return fMixing.compare_exchange_strong(expected, true);
}

void CCoinJoinClientManager::StopMixing() {
void CCoinJoinClientManager::stopMixing() {
fMixing = false;
}

bool CCoinJoinClientManager::IsMixing() const
bool CCoinJoinClientManager::isMixing() const
{
return fMixing;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Expand All @@ -159,7 +159,7 @@ void CCoinJoinClientSession::ResetPool()
WITH_LOCK(cs_coinjoin, SetNull());
}

void CCoinJoinClientManager::ResetPool()
void CCoinJoinClientManager::resetPool()
{
nCachedLastSuccessBlock = 0;
AssertLockNotHeld(cs_deqsessions);
Expand Down Expand Up @@ -241,7 +241,7 @@ bilingual_str CCoinJoinClientSession::GetStatus(bool fWaitForBlock) const
}
}

std::vector<std::string> CCoinJoinClientManager::GetStatuses() const
std::vector<std::string> CCoinJoinClientManager::getSessionStatuses() const
{
AssertLockNotHeld(cs_deqsessions);

Expand All @@ -255,7 +255,7 @@ std::vector<std::string> CCoinJoinClientManager::GetStatuses() const
return ret;
}

std::string CCoinJoinClientManager::GetSessionDenoms()
std::string CCoinJoinClientManager::getSessionDenoms() const
{
std::string strSessionDenoms;

Expand Down Expand Up @@ -328,7 +328,7 @@ void CCoinJoinClientManager::CheckTimeout()
{
AssertLockNotHeld(cs_deqsessions);

if (!CCoinJoinClientOptions::IsEnabled() || !IsMixing()) return;
if (!CCoinJoinClientOptions::IsEnabled() || !isMixing()) return;

LOCK(cs_deqsessions);
for (auto& session : deqSessions) {
Expand Down Expand Up @@ -605,7 +605,7 @@ bool CCoinJoinClientManager::WaitForAnotherBlock() const

bool CCoinJoinClientManager::CheckAutomaticBackup()
{
if (!CCoinJoinClientOptions::IsEnabled() || !IsMixing()) return false;
if (!CCoinJoinClientOptions::IsEnabled() || !isMixing()) return false;

// We don't need auto-backups for descriptor wallets
if (!m_wallet->IsLegacy()) return true;
Expand All @@ -614,7 +614,7 @@ bool CCoinJoinClientManager::CheckAutomaticBackup()
case 0:
strAutoDenomResult = _("Automatic backups disabled") + Untranslated(", ") + _("no mixing available.");
WalletCJLogPrint(m_wallet, "CCoinJoinClientManager::CheckAutomaticBackup -- %s\n", strAutoDenomResult.original);
StopMixing();
stopMixing();
m_wallet->nKeysLeftSinceAutoBackup = 0; // no backup, no "keys since last backup"
return false;
case -1:
Expand All @@ -640,7 +640,7 @@ bool CCoinJoinClientManager::CheckAutomaticBackup()
m_wallet->nKeysLeftSinceAutoBackup);
WalletCJLogPrint(m_wallet, "CCoinJoinClientManager::CheckAutomaticBackup -- %s\n", strAutoDenomResult.original);
// It's getting really dangerous, stop mixing
StopMixing();
stopMixing();
return false;
} else if (m_wallet->nKeysLeftSinceAutoBackup < COINJOIN_KEYS_THRESHOLD_WARNING) {
// Low number of keys left, but it's still more or less safe to continue
Expand Down Expand Up @@ -862,7 +862,7 @@ bool CCoinJoinClientSession::DoAutomaticDenominating(ChainstateManager& chainman
bool CCoinJoinClientManager::DoAutomaticDenominating(ChainstateManager& chainman, CConnman& connman,
const CTxMemPool& mempool, bool fDryRun)
{
if (!CCoinJoinClientOptions::IsEnabled() || !IsMixing()) return false;
if (!CCoinJoinClientOptions::IsEnabled() || !isMixing()) return false;

if (!m_mn_sync.IsBlockchainSynced()) {
strAutoDenomResult = _("Can't mix while sync in progress.");
Expand Down Expand Up @@ -1765,10 +1765,10 @@ void CCoinJoinClientSession::GetJsonInfo(UniValue& obj) const
obj.pushKV("entries_count", GetEntriesCount());
}

void CCoinJoinClientManager::GetJsonInfo(UniValue& obj) const
UniValue CCoinJoinClientManager::getJsonInfo() const
{
assert(obj.isObject());
obj.pushKV("running", IsMixing());
UniValue obj(UniValue::VOBJ);
obj.pushKV("running", isMixing());

UniValue arrSessions(UniValue::VARR);
AssertLockNotHeld(cs_deqsessions);
Expand All @@ -1781,5 +1781,6 @@ void CCoinJoinClientManager::GetJsonInfo(UniValue& obj) const
}
}
obj.pushKV("sessions", arrSessions);
return obj;
}

24 changes: 14 additions & 10 deletions src/coinjoin/client.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include <coinjoin/coinjoin.h>
#include <coinjoin/util.h>
#include <evo/types.h>
#include <interfaces/coinjoin.h>
#include <util/translation.h>

#include <atomic>
Expand Down Expand Up @@ -154,7 +155,7 @@ class CCoinJoinClientSession : public CCoinJoinBaseSession

/** Used to keep track of current status of mixing pool
*/
class CCoinJoinClientManager
class CCoinJoinClientManager : public interfaces::CoinJoin::Client
{
private:
const std::shared_ptr<wallet::CWallet> m_wallet;
Expand Down Expand Up @@ -197,14 +198,6 @@ class CCoinJoinClientManager

void ProcessMessage(CNode& peer, CChainState& active_chainstate, CConnman& connman, const CTxMemPool& mempool, std::string_view msg_type, CDataStream& vRecv) EXCLUSIVE_LOCKS_REQUIRED(!cs_deqsessions);

bool StartMixing();
void StopMixing();
bool IsMixing() const;
void ResetPool() EXCLUSIVE_LOCKS_REQUIRED(!cs_deqsessions);

std::vector<std::string> GetStatuses() const EXCLUSIVE_LOCKS_REQUIRED(!cs_deqsessions);
std::string GetSessionDenoms() EXCLUSIVE_LOCKS_REQUIRED(!cs_deqsessions);

bool GetMixingMasternodesInfo(std::vector<CDeterministicMNCPtr>& vecDmnsRet) const EXCLUSIVE_LOCKS_REQUIRED(!cs_deqsessions);

/// Passively run mixing in the background according to the configuration in settings
Expand All @@ -229,7 +222,18 @@ class CCoinJoinClientManager
void DoMaintenance(ChainstateManager& chainman, CConnman& connman, const CTxMemPool& mempool)
EXCLUSIVE_LOCKS_REQUIRED(!cs_deqsessions);

void GetJsonInfo(UniValue& obj) const EXCLUSIVE_LOCKS_REQUIRED(!cs_deqsessions);
// interfaces::CoinJoin::Client overrides
void resetCachedBlocks() override { nCachedNumBlocks = std::numeric_limits<int>::max(); }
int getCachedBlocks() const override { return nCachedNumBlocks; }
void setCachedBlocks(int nCachedBlocks) override { nCachedNumBlocks = nCachedBlocks; }
void disableAutobackups() override { fCreateAutoBackups = false; }
void resetPool() override EXCLUSIVE_LOCKS_REQUIRED(!cs_deqsessions);
UniValue getJsonInfo() const override EXCLUSIVE_LOCKS_REQUIRED(!cs_deqsessions);
std::vector<std::string> getSessionStatuses() const override EXCLUSIVE_LOCKS_REQUIRED(!cs_deqsessions);
std::string getSessionDenoms() const override EXCLUSIVE_LOCKS_REQUIRED(!cs_deqsessions);
bool isMixing() const override;
bool startMixing() override;
void stopMixing() override;
};

#endif // BITCOIN_COINJOIN_CLIENT_H
73 changes: 7 additions & 66 deletions src/coinjoin/interfaces.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,60 +20,6 @@ using wallet::CWallet;
namespace coinjoin {
namespace {

class CoinJoinClientImpl : public interfaces::CoinJoin::Client
{
CCoinJoinClientManager& m_clientman;

public:
explicit CoinJoinClientImpl(CCoinJoinClientManager& clientman)
: m_clientman(clientman) {}

void resetCachedBlocks() override
{
m_clientman.nCachedNumBlocks = std::numeric_limits<int>::max();
}
void resetPool() override
{
m_clientman.ResetPool();
}
void disableAutobackups() override
{
m_clientman.fCreateAutoBackups = false;
}
int getCachedBlocks() override
{
return m_clientman.nCachedNumBlocks;
}
void getJsonInfo(UniValue& obj) override
{
return m_clientman.GetJsonInfo(obj);
}
std::string getSessionDenoms() override
{
return m_clientman.GetSessionDenoms();
}
std::vector<std::string> getSessionStatuses() override
{
return m_clientman.GetStatuses();
}
void setCachedBlocks(int nCachedBlocks) override
{
m_clientman.nCachedNumBlocks = nCachedBlocks;
}
bool isMixing() override
{
return m_clientman.IsMixing();
}
bool startMixing() override
{
return m_clientman.StartMixing();
}
void stopMixing() override
{
m_clientman.StopMixing();
}
};

class CoinJoinLoaderImpl : public interfaces::CoinJoin::Loader
{
private:
Expand All @@ -82,37 +28,32 @@ class CoinJoinLoaderImpl : public interfaces::CoinJoin::Loader
return *Assert(m_node.cj_walletman);
}

interfaces::WalletLoader& wallet_loader()
{
return *Assert(m_node.wallet_loader);
}

public:
explicit CoinJoinLoaderImpl(NodeContext& node) :
m_node(node)
{
// Enablement will be re-evaluated when a wallet is added or removed
CCoinJoinClientOptions::SetEnabled(false);
CCoinJoinClientOptions::SetEnabled(gArgs.GetBoolArg("-enablecoinjoin", true));
}
Comment on lines 32 to 36
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💬 Nitpick: CoinJoinLoaderImpl constructor reads gArgs and flips global SetEnabled

CoinJoinLoaderImpl now calls CCoinJoinClientOptions::SetEnabled(gArgs.GetBoolArg("-enablecoinjoin", true)) unconditionally at construction. Previous behaviour initialised the flag to false and only flipped it once a wallet was added (and back off on last-wallet removal). Per the commit message this is intentional, but: (1) for non-wallet builds the loader is still constructed via MakeCoinJoinLoader, so this now flips a global to true even when no client will exist (benign but easy to miss); (2) reading gArgs from a constructor ties unit tests to global-state ordering (coinjoin_options_tests now must ForceSetArg("-enablecoinjoin", "0") before building the loader, src/wallet/test/coinjoin_tests.cpp:30). Consider passing the enabled value explicitly into the constructor to decouple from gArgs.

source: ['claude']


void AddWallet(const std::shared_ptr<CWallet>& wallet) override
{
manager().addWallet(wallet);
g_wallet_init_interface.InitCoinJoinSettings(*this, wallet_loader());
if (!CCoinJoinClientOptions::IsEnabled()) return;
manager().doForClient(wallet->GetName(), [](CCoinJoinClientManager& mgr) {
g_wallet_init_interface.InitCoinJoinSettings(mgr);
});
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment on lines 38 to 45
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: AddWallet releases cs_wallet_manager_map between insert and doForClient

manager().addWallet(wallet) acquires and releases cs_wallet_manager_map, then manager().doForClient(wallet->GetName(), ...) re-acquires it. A concurrent removeWallet() racing between the two calls would cause doForClient to return false and silently skip InitCoinJoinSettings for the just-added wallet. No dangling pointer results (the map stays consistent) but it defeats the spirit of the PR, which is to keep wallet access within a single critical section. Consider an atomic helper on CJWalletManager (e.g. addWalletAnd(name, func)) or invoke InitCoinJoinSettings from inside addWallet while the lock is held.

source: ['claude']

🤖 Fix this with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/coinjoin/interfaces.cpp`:
- [SUGGESTION] lines 38-45: `AddWallet` releases `cs_wallet_manager_map` between insert and `doForClient`
  `manager().addWallet(wallet)` acquires and releases `cs_wallet_manager_map`, then `manager().doForClient(wallet->GetName(), ...)` re-acquires it. A concurrent `removeWallet()` racing between the two calls would cause `doForClient` to return false and silently skip `InitCoinJoinSettings` for the just-added wallet. No dangling pointer results (the map stays consistent) but it defeats the spirit of the PR, which is to keep wallet access within a single critical section. Consider an atomic helper on `CJWalletManager` (e.g. `addWalletAnd(name, func)`) or invoke `InitCoinJoinSettings` from inside `addWallet` while the lock is held.

void RemoveWallet(const std::string& name) override
{
manager().removeWallet(name);
g_wallet_init_interface.InitCoinJoinSettings(*this, wallet_loader());
}
void FlushWallet(const std::string& name) override
{
manager().flushWallet(name);
}
std::unique_ptr<interfaces::CoinJoin::Client> GetClient(const std::string& name) override
bool WithClient(const std::string& name, std::function<void(interfaces::CoinJoin::Client&)> func) override
{
auto clientman = manager().getClient(name);
return clientman ? std::make_unique<CoinJoinClientImpl>(*clientman) : nullptr;
return manager().doForClient(name, [&](CCoinJoinClientManager& mgr) { func(mgr); });
}

NodeContext& m_node;
Expand Down
17 changes: 10 additions & 7 deletions src/coinjoin/walletman.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,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);
bool doForClient(const std::string& name, const std::function<void(CCoinJoinClientManager&)>& func) 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;
Expand Down Expand Up @@ -140,11 +140,13 @@ bool CJWalletManagerImpl::hasQueue(const uint256& hash) const
return false;
}

CCoinJoinClientManager* CJWalletManagerImpl::getClient(const std::string& name)
bool CJWalletManagerImpl::doForClient(const std::string& name, const std::function<void(CCoinJoinClientManager&)>& func)
{
LOCK(cs_wallet_manager_map);
auto it = m_wallet_manager_map.find(name);
return (it != m_wallet_manager_map.end()) ? it->second.get() : nullptr;
if (it == m_wallet_manager_map.end()) return false;
func(*it->second);
Comment on lines +145 to +148
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Do not invoke client callbacks while holding manager mutex

doForClient() now keeps cs_wallet_manager_map locked while running arbitrary callbacks, and several new callbacks call client.resetPool() (for example coinjoin reset in src/rpc/coinjoin.cpp and flushWallet() in this file), which takes wallet locks via CCoinJoinClientSession::ResetPool(). At the same time, wallet code can take the opposite order (cs_wallet then CoinJoin manager lock) through CWallet::NewKeyPoolCallback() calling WithClient() in src/wallet/wallet.cpp, creating a lock-order inversion that can deadlock under concurrent RPC/UI keypool operations and CoinJoin reset/flush paths.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not, the whole point of PR is doing it under cs_wallet_manager_map mutex, otherwhise wallet maybe destroyed

return true;
}

std::optional<CCoinJoinQueue> CJWalletManagerImpl::getQueueFromHash(const uint256& hash) const
Expand Down Expand Up @@ -180,9 +182,10 @@ void CJWalletManagerImpl::addWallet(const std::shared_ptr<wallet::CWallet>& wall

void CJWalletManagerImpl::flushWallet(const std::string& name)
{
auto* clientman = Assert(getClient(name));
clientman->ResetPool();
clientman->StopMixing();
doForClient(name, [](CCoinJoinClientManager& clientman) {
clientman.resetPool();
clientman.stopMixing();
});
}

void CJWalletManagerImpl::removeWallet(const std::string& name)
Expand Down Expand Up @@ -314,6 +317,6 @@ std::unique_ptr<CJWalletManager> CJWalletManager::make(ChainstateManager& chainm
return std::make_unique<CJWalletManagerImpl>(chainman, dmnman, mn_metaman, mempool, mn_sync, isman, relay_txes);
#else
// Cannot be constructed if wallet support isn't built
return nullptr;
assert(false);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail
nl -ba src/coinjoin/walletman.cpp | sed -n '311,322p'

Repository: dashpay/dash

Length of output: 98


🏁 Script executed:

sed -n '311,322p' src/coinjoin/walletman.cpp

Repository: dashpay/dash

Length of output: 729


Return after assert(false) in the non-wallet factory path.

In release builds, assert(false) compiles out, so this branch falls off the end of a non-void function. This breaks -nowallet builds and is undefined behavior.

🔧 Minimal fix
 `#else`
     // Cannot be constructed if wallet support isn't built
     assert(false);
+    return nullptr;
 `#endif` // ENABLE_WALLET
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
#else
// Cannot be constructed if wallet support isn't built
return nullptr;
assert(false);
`#else`
// Cannot be constructed if wallet support isn't built
assert(false);
return nullptr;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/coinjoin/walletman.cpp` around lines 318 - 320, The non-wallet factory
branch in src/coinjoin/walletman.cpp currently does assert(false) and then falls
off the end of a non-void function in release builds; update the non-wallet path
(the factory function in walletman.cpp that constructs/returns a wallet object
when wallet support is disabled) to return an explicit safe default (e.g.,
return nullptr) after the assert(false) so the function has a defined return
value in non-debug builds; ensure the returned value matches the function's
return type (use nullptr for pointer returns or an appropriate sentinel value)
and keep the assert for debug-time diagnostics.

#endif // ENABLE_WALLET
}
5 changes: 4 additions & 1 deletion src/coinjoin/walletman.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

#include <validationinterface.h>

#include <functional>
#include <optional>
#include <string_view>

Expand Down Expand Up @@ -47,7 +48,9 @@ class CJWalletManager : public CValidationInterface

public:
virtual bool hasQueue(const uint256& hash) const = 0;
virtual CCoinJoinClientManager* getClient(const std::string& name) = 0;
//! Execute func under the wallet manager lock for the client identified by name.
//! Returns true if the client was found and func was called, false otherwise.
virtual bool doForClient(const std::string& name, const std::function<void(CCoinJoinClientManager&)>& func) = 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;
Expand Down
2 changes: 1 addition & 1 deletion src/dummywallet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class DummyWalletInit : public WalletInitInterface {

// Dash Specific WalletInitInterface InitCoinJoinSettings
void AutoLockMasternodeCollaterals(interfaces::WalletLoader& wallet_loader) const override {}
void InitCoinJoinSettings(interfaces::CoinJoin::Loader& coinjoin_loader, interfaces::WalletLoader& wallet_loader) const override {}
void InitCoinJoinSettings(CCoinJoinClientManager& mgr) const override {}
void InitAutoBackup() const override {}
};

Expand Down
2 changes: 2 additions & 0 deletions src/init.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2224,8 +2224,10 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
} else {
assert(!node.cj_walletman);
// Can return nullptr if built without wallet support, must check before use
#ifdef ENABLE_WALLET
node.cj_walletman = CJWalletManager::make(chainman, *node.dmnman, *node.mn_metaman, *node.mempool, *node.mn_sync,
*node.llmq_ctx->isman, !ignores_incoming_txs);
#endif
}

if (node.cj_walletman) {
Expand Down
Loading
Loading