Skip to content

fix: dangling point to cj client #7259

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
knst wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

fix: dangling point to cj client #7259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9452854
fix: linkage error for non-wallet builds for further changes
knst Mar 29, 2026
f61af76
fix: regression test coinjoin_options_tests by enforcing call of `int…
knst Apr 16, 2026
66c8443
refactor: remove CoinJoinClientImpl wrapper and return bare pointer t…
knst Mar 27, 2026
721233d
fix: avoid dangling pointers to CJ clients
knst Mar 29, 2026
bfc3c02
refactor: remove duplicated methods from CJWalletManagerImpl to prefe…
knst Mar 29, 2026
d9a3c74
fix: call InitCoinJoin only wallet is added
knst Mar 30, 2026
646af49
fix: keep flag `coinjoin` enabled consistently with user's intention …
knst Apr 13, 2026
fbf9d09
refactor: cleanups for CoinJoin client interface
UdjinM6 Apr 19, 2026
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
View file
Open in desktop
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
View file
Open in desktop
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;
}
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;
}

30 changes: 17 additions & 13 deletions src/coinjoin/client.h
View file
Open in desktop
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 All @@ -178,15 +179,15 @@ class CCoinJoinClientManager
// Keep track of current block height
int nCachedBlockHeight{0};

int nCachedNumBlocks{std::numeric_limits<int>::max()}; // used for the overview screen
bool fCreateAutoBackups{true}; // builtin support for automatic backups

bool WaitForAnotherBlock() const;

// Make sure we have enough keys since last backup
bool CheckAutomaticBackup();

public:
int nCachedNumBlocks{std::numeric_limits<int>::max()}; // used for the overview screen
bool fCreateAutoBackups{true}; // builtin support for automatic backups

CCoinJoinClientManager() = delete;
CCoinJoinClientManager(const CCoinJoinClientManager&) = delete;
CCoinJoinClientManager& operator=(const CCoinJoinClientManager&) = delete;
Expand All @@ -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
View file
Open in desktop
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

@thepastaclaw thepastaclaw Apr 23, 2026

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 on lines 38 to 45
Copy link
Copy Markdown

@thepastaclaw thepastaclaw Apr 23, 2026

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, const 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
22 changes: 10 additions & 12 deletions src/coinjoin/walletman.cpp
View file
Open in desktop
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);
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 @@ -303,17 +306,12 @@ MessageProcessingResult CJWalletManagerImpl::ProcessDSQueue(NodeId from, CConnma
} // cs_ProcessDSQueue
return ret;
}
#endif // ENABLE_WALLET

std::unique_ptr<CJWalletManager> CJWalletManager::make(ChainstateManager& chainman, CDeterministicMNManager& dmnman,
CMasternodeMetaMan& mn_metaman, CTxMemPool& mempool,
const CMasternodeSync& mn_sync,
const llmq::CInstantSendManager& isman, bool relay_txes)
{
#ifdef ENABLE_WALLET
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;
#endif // ENABLE_WALLET
}
#endif // ENABLE_WALLET
5 changes: 4 additions & 1 deletion src/coinjoin/walletman.h
View file
Open in desktop
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
Loading
Loading