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 13 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
13 commits
Select commit Hold shift + click to select a range
689b4ec
refactor: remove passing pointer to unique_ptr to more clear sequence…
knst Mar 16, 2026
e2d8247
refactor: pass directly reference to CCoinJoinClientManager& instead …
knst Mar 16, 2026
3b1c2da
refactor: inline the middleman class CoinJoinWalletManager to CJWalle…
knst Mar 17, 2026
72229ce
refactor: inline class CCoinJoinClientQueueManager to CJWalletManager…
knst Mar 17, 2026
a0f39f2
refactor: removed CCoinJoinBaseManager from inheritance of CCoinJoinS…
knst Mar 17, 2026
6fae09a
refactor: remove inheritance from CCoinJoinBaseManager in src/test/co…
knst Mar 17, 2026
40cd08d
refactor: rename CCoinJoinBaseManager to CoinJoinQueueManager and mak…
knst Mar 17, 2026
eb2142d
fix: added missing thread annotation for CJWalletManagerImpl
knst Mar 27, 2026
52eb316
fix: linkage error for non-wallet builds for further changes
knst Mar 29, 2026
8669407
refactor: remove CoinJoinClientImpl wrapper and return bare pointer t…
knst Mar 27, 2026
9016181
fix: avoid dangling pointers to CJ clients
knst Mar 29, 2026
87f7b2e
refactor: remove duplicated methods from CJWalletManagerImpl to prefe…
knst Mar 29, 2026
92d0f34
fix: call InitCoinJoin only wallet is added
knst Mar 30, 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 contrib/auto_gdb/simple_class_obj.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
"CMasternodeBroadcast", "CMasternodePing",
"CMasternodeMan", "CDarksendQueue", "CDarkSendEntry",
"CTransaction", "CMutableTransaction", "CCoinJoinBaseSession",
"CCoinJoinBaseManager", "CCoinJoinClientSession",
"CoinJoinQueueManager", "CCoinJoinClientSession",
"CCoinJoinClientManager", "CCoinJoinServer", "CMasternodePayments",
"CMasternodePaymentVote", "CMasternodeBlockPayees",
"CMasternodePayee", "CInstantSend", "CTxLockRequest",
Expand Down
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 @@ -501,7 +501,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 @@ -658,6 +657,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
1 change: 0 additions & 1 deletion src/Makefile.test.include
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,6 @@ BITCOIN_TESTS =\
test/governance_validators_tests.cpp \
test/coinjoin_inouts_tests.cpp \
test/coinjoin_dstxmanager_tests.cpp \
test/coinjoin_basemanager_tests.cpp \
test/coinjoin_queue_tests.cpp \
test/hash_tests.cpp \
test/httpserver_tests.cpp \
Expand Down
219 changes: 27 additions & 192 deletions src/coinjoin/client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,122 +40,10 @@ using wallet::CoinType;
using wallet::CWallet;
using wallet::ReserveDestination;

CCoinJoinClientQueueManager::CCoinJoinClientQueueManager(CoinJoinWalletManager& walletman, CDeterministicMNManager& dmnman,
CMasternodeMetaMan& mn_metaman, const CMasternodeSync& mn_sync) :
m_walletman{walletman},
m_dmnman{dmnman},
m_mn_metaman{mn_metaman},
m_mn_sync{mn_sync}
{
}

CCoinJoinClientQueueManager::~CCoinJoinClientQueueManager() = default;

MessageProcessingResult CCoinJoinClientQueueManager::ProcessMessage(NodeId from, CConnman& connman,
std::string_view msg_type, CDataStream& vRecv)
{
if (msg_type != NetMsgType::DSQUEUE) {
return {};
}
if (!m_mn_sync.IsBlockchainSynced()) return {};

assert(m_mn_metaman.IsValid());

CCoinJoinQueue dsq;
vRecv >> dsq;

MessageProcessingResult ret{};
ret.m_to_erase = CInv{MSG_DSQ, dsq.GetHash()};

if (dsq.masternodeOutpoint.IsNull() && dsq.m_protxHash.IsNull()) {
ret.m_error = MisbehavingError{100};
return ret;
}

const auto tip_mn_list = m_dmnman.GetListAtChainTip();
if (dsq.masternodeOutpoint.IsNull()) {
if (auto dmn = tip_mn_list.GetValidMN(dsq.m_protxHash)) {
dsq.masternodeOutpoint = dmn->collateralOutpoint;
} else {
ret.m_error = MisbehavingError{10};
return ret;
}
}

{
LOCK(cs_ProcessDSQueue);

{
LOCK(cs_vecqueue);
// process every dsq only once
for (const auto &q: vecCoinJoinQueue) {
if (q == dsq) {
return ret;
}
if (q.fReady == dsq.fReady && q.masternodeOutpoint == dsq.masternodeOutpoint) {
// no way the same mn can send another dsq with the same readiness this soon
LogPrint(BCLog::COINJOIN, /* Continued */
"DSQUEUE -- Peer %d is sending WAY too many dsq messages for a masternode with collateral %s\n",
from, dsq.masternodeOutpoint.ToStringShort());
return ret;
}
}
} // cs_vecqueue

LogPrint(BCLog::COINJOIN, "DSQUEUE -- %s new\n", dsq.ToString());

if (dsq.IsTimeOutOfBounds()) return ret;

auto dmn = tip_mn_list.GetValidMNByCollateral(dsq.masternodeOutpoint);
if (!dmn) return ret;

if (dsq.m_protxHash.IsNull()) {
dsq.m_protxHash = dmn->proTxHash;
}

if (!dsq.CheckSignature(dmn->pdmnState->pubKeyOperator.Get())) {
ret.m_error = MisbehavingError{10};
return ret;
}

// if the queue is ready, submit if we can
if (dsq.fReady &&
m_walletman.ForAnyCJClientMan([&connman, &dmn](std::unique_ptr<CCoinJoinClientManager>& clientman) {
return clientman->TrySubmitDenominate(dmn->proTxHash, connman);
})) {
LogPrint(BCLog::COINJOIN, "DSQUEUE -- CoinJoin queue is ready, masternode=%s, queue=%s\n", dmn->proTxHash.ToString(), dsq.ToString());
return ret;
} else {
if (m_mn_metaman.IsMixingThresholdExceeded(dmn->proTxHash, tip_mn_list.GetCounts().enabled())) {
LogPrint(BCLog::COINJOIN, "DSQUEUE -- Masternode %s is sending too many dsq messages\n",
dmn->proTxHash.ToString());
return ret;
}
m_mn_metaman.AllowMixing(dmn->proTxHash);

LogPrint(BCLog::COINJOIN, "DSQUEUE -- new CoinJoin queue, masternode=%s, queue=%s\n", dmn->proTxHash.ToString(), dsq.ToString());

m_walletman.ForAnyCJClientMan([&dsq](const std::unique_ptr<CCoinJoinClientManager>& clientman) {
return clientman->MarkAlreadyJoinedQueueAsTried(dsq);
});

WITH_LOCK(cs_vecqueue, vecCoinJoinQueue.push_back(dsq));
}
} // cs_ProcessDSQueue
return ret;
}

void CCoinJoinClientQueueManager::DoMaintenance()
{
if (!m_mn_sync.IsBlockchainSynced() || ShutdownRequested()) return;

CheckQueue();
}

CCoinJoinClientManager::CCoinJoinClientManager(const std::shared_ptr<wallet::CWallet>& wallet,
CDeterministicMNManager& dmnman, CMasternodeMetaMan& mn_metaman,
const CMasternodeSync& mn_sync, const llmq::CInstantSendManager& isman,
const std::unique_ptr<CCoinJoinClientQueueManager>& queueman) :
CoinJoinQueueManager* queueman) :
m_wallet{wallet},
m_dmnman{dmnman},
m_mn_metaman{mn_metaman},
Expand All @@ -173,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 All @@ -192,15 +80,13 @@ void CCoinJoinClientManager::ProcessMessage(CNode& peer, CChainState& active_cha

CCoinJoinClientSession::CCoinJoinClientSession(const std::shared_ptr<CWallet>& wallet, CCoinJoinClientManager& clientman,
CDeterministicMNManager& dmnman, CMasternodeMetaMan& mn_metaman,
const CMasternodeSync& mn_sync, const llmq::CInstantSendManager& isman,
const std::unique_ptr<CCoinJoinClientQueueManager>& queueman) :
const CMasternodeSync& mn_sync, const llmq::CInstantSendManager& isman) :
m_wallet(wallet),
m_clientman(clientman),
m_dmnman(dmnman),
m_mn_metaman(mn_metaman),
m_mn_sync(mn_sync),
m_isman{isman},
m_queueman(queueman)
m_isman{isman}
{}

void CCoinJoinClientSession::ProcessMessage(CNode& peer, CChainState& active_chainstate, CConnman& connman, const CTxMemPool& mempool, std::string_view msg_type, CDataStream& vRecv)
Expand Down Expand Up @@ -251,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 on lines +140 to 152
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Mar 30, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Fix formatting to pass CI checks.

The CI pipeline reports clang-format errors for the method declarations/definitions at lines 140-152. The brace placement and method signatures need reformatting.

🔧 Suggested formatting fix 
-bool CCoinJoinClientManager::startMixing() {
+bool CCoinJoinClientManager::startMixing()
+{
     bool expected{false};
     return fMixing.compare_exchange_strong(expected, true);
 }

-void CCoinJoinClientManager::stopMixing() {
+void CCoinJoinClientManager::stopMixing()
+{
     fMixing = false;
 }
📝 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
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;
}
bool CCoinJoinClientManager::startMixing()
{
bool expected{false};
return fMixing.compare_exchange_strong(expected, true);
}
void CCoinJoinClientManager::stopMixing()
{
fMixing = false;
}
bool CCoinJoinClientManager::isMixing() const
{
return fMixing;
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/coinjoin/client.cpp` around lines 140 - 152, Reformat the three
CCoinJoinClientManager methods (CCoinJoinClientManager::startMixing,
::stopMixing, and ::isMixing) to match project clang-format style: place the
opening brace on the same line as the function signature, ensure spacing around
return types and parameters is consistent, and keep single-line bodies compact
(e.g., return fMixing; or fMixing = false; as appropriate); run clang-format or
apply the repo's style so these method definitions and use of the atomic fMixing
variable conform to CI formatting rules.

Expand All @@ -273,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 @@ -355,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 @@ -369,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 @@ -442,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 @@ -719,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 @@ -728,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 @@ -754,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 @@ -976,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 @@ -1007,7 +893,7 @@ bool CCoinJoinClientManager::DoAutomaticDenominating(ChainstateManager& chainman
AssertLockNotHeld(cs_deqsessions);
LOCK(cs_deqsessions);
if (int(deqSessions.size()) < CCoinJoinClientOptions::GetSessions()) {
deqSessions.emplace_back(m_wallet, *this, m_dmnman, m_mn_metaman, m_mn_sync, m_isman, m_queueman);
deqSessions.emplace_back(m_wallet, *this, m_dmnman, m_mn_metaman, m_mn_sync, m_isman);
}
for (auto& session : deqSessions) {
if (!CheckAutomaticBackup()) return false;
Expand Down Expand Up @@ -1075,14 +961,13 @@ static int WinnersToSkip()
bool CCoinJoinClientSession::JoinExistingQueue(CAmount nBalanceNeedsAnonymized, CConnman& connman)
{
if (!CCoinJoinClientOptions::IsEnabled()) return false;
if (m_queueman == nullptr) return false;

const auto mnList = m_dmnman.GetListAtChainTip();
const int nWeightedMnCount = mnList.GetCounts().m_valid_weighted;

// Look through the queues and see if anything matches
CCoinJoinQueue dsq;
while (m_queueman->GetQueueItemAndTry(dsq)) {
while (m_clientman.GetQueueItemAndTry(dsq)) {
auto dmn = mnList.GetValidMNByCollateral(dsq.masternodeOutpoint);

if (!dmn) {
Expand Down Expand Up @@ -1288,6 +1173,11 @@ bool CCoinJoinClientManager::MarkAlreadyJoinedQueueAsTried(CCoinJoinQueue& dsq)
return false;
}

bool CCoinJoinClientManager::GetQueueItemAndTry(CCoinJoinQueue& dsq) const
{
return m_queueman && m_queueman->GetQueueItemAndTry(dsq);
}

bool CCoinJoinClientSession::SubmitDenominate(CConnman& connman)
{
LOCK(m_wallet->cs_wallet);
Expand Down Expand Up @@ -1875,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 @@ -1891,61 +1781,6 @@ void CCoinJoinClientManager::GetJsonInfo(UniValue& obj) const
}
}
obj.pushKV("sessions", arrSessions);
return obj;
}

CoinJoinWalletManager::CoinJoinWalletManager(ChainstateManager& chainman, CDeterministicMNManager& dmnman,
CMasternodeMetaMan& mn_metaman, const CTxMemPool& mempool,
const CMasternodeSync& mn_sync, const llmq::CInstantSendManager& isman,
const std::unique_ptr<CCoinJoinClientQueueManager>& queueman) :
m_chainman{chainman},
m_dmnman{dmnman},
m_mn_metaman{mn_metaman},
m_mempool{mempool},
m_mn_sync{mn_sync},
m_isman{isman},
m_queueman{queueman}
{
}

CoinJoinWalletManager::~CoinJoinWalletManager()
{
LOCK(cs_wallet_manager_map);
for (auto& [wallet_name, cj_man] : m_wallet_manager_map) {
cj_man.reset();
}
}

void CoinJoinWalletManager::Add(const std::shared_ptr<CWallet>& wallet)
{
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));
}

void CoinJoinWalletManager::DoMaintenance(CConnman& connman)
{
LOCK(cs_wallet_manager_map);
for (auto& [_, clientman] : m_wallet_manager_map) {
clientman->DoMaintenance(m_chainman, connman, m_mempool);
}
}

void CoinJoinWalletManager::Remove(const std::string& name) {
LOCK(cs_wallet_manager_map);
m_wallet_manager_map.erase(name);
}

void CoinJoinWalletManager::Flush(const std::string& name)
{
auto clientman = Assert(Get(name));
clientman->ResetPool();
clientman->StopMixing();
}

CCoinJoinClientManager* CoinJoinWalletManager::Get(const std::string& name) const
{
LOCK(cs_wallet_manager_map);
auto it = m_wallet_manager_map.find(name);
return (it != m_wallet_manager_map.end()) ? it->second.get() : nullptr;
}
Loading
Loading