Skip to content

test: cover CoinJoin client restart handling #7420

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
thepastaclaw wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

test: cover CoinJoin client restart handling #7420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 14 additions & 4 deletions src/coinjoin/server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1436,10 +1436,20 @@ void CCoinJoinServer::GetJsonInfo(UniValue& obj) const
{
obj.clear();
obj.setObject();
obj.pushKV("queue_size", m_queueman.GetQueueSize());
obj.pushKV("denomination", ValueFromAmount(CoinJoin::DenominationToAmount(nSessionDenom)));
obj.pushKV("state", GetStateString());
obj.pushKV("entries_count", GetEntriesCount());
std::string state;
int entries_count;
size_t collaterals_count;
{
LOCK(cs_coinjoin);
state = GetStateString();
entries_count = GetEntriesCountLocked();
collaterals_count = m_session_collaterals.size();

@chatgpt-codex-connector chatgpt-codex-connector Bot Oct 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Snapshot collateral count with its session state

When a server session is reset or replaced while GetJsonInfo() is executing, this value is captured under cs_coinjoin but the state paired with it is read only after that lock is released, so the RPC can return an old session's count alongside IDLE or a replacement session's QUEUE state. isolate_joiner_in_active_session() treats collaterals_count >= 2 plus that separately sampled state as proof that the current queue accepted both clients; under this interleaving it can isolate the joiner with fewer participants and then time out. Snapshot the count and state together, ideally with the session identity.

AGENTS.md reference: AGENTS.md:L136-L137

Useful? React with 👍 / 👎.

}
obj.pushKV("queue_size", m_queueman.GetQueueSize());
obj.pushKV("denomination", ValueFromAmount(CoinJoin::DenominationToAmount(nSessionDenom)));
obj.pushKV("state", state);
obj.pushKV("entries_count", entries_count);
obj.pushKV("collaterals_count", collaterals_count);
}

bool CCoinJoinServer::AlreadyHave(const CInv& inv)
Expand Down
1 change: 1 addition & 0 deletions src/rpc/coinjoin.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,6 +448,7 @@ static RPCHelpMan getcoinjoininfo()
{RPCResult::Type::NUM, "denomination", "The denomination of the mixing session in " + CURRENCY_UNIT + ""},
{RPCResult::Type::STR_HEX, "state", "Current state of the mixing session"},
{RPCResult::Type::NUM, "entries_count", "The number of entries in the mixing session"},
{RPCResult::Type::NUM, "collaterals_count", "The number of accepted collaterals in the mixing session"},
}},
RPCResult{"for non-masternodes without wallet support",
RPCResult::Type::OBJ, "", /*optional=*/false, "", {}},
Expand Down
46 changes: 46 additions & 0 deletions src/wallet/test/coinjoin_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@

#include <boost/test/unit_test.hpp>

#include <chrono>
#include <future>
#include <thread>

namespace wallet {
BOOST_FIXTURE_TEST_SUITE(coinjoin_tests, BasicTestingSetup)

Expand Down Expand Up @@ -394,6 +398,48 @@ BOOST_FIXTURE_TEST_CASE(coinjoin_pending_observation_reload_tests, CTransactionB
SetMockTime(0);
}

BOOST_FIXTURE_TEST_CASE(coinjoin_wallet_manager_remove_during_client_access_tests, CTransactionBuilderTestSetup)
{
std::promise<void> client_entered;
auto client_entered_future{client_entered.get_future()};
std::promise<void> release_client;
auto release_client_future{release_client.get_future()};
std::promise<void> remover_started;
auto remover_started_future{remover_started.get_future()};
bool client_found{false};

std::thread client_thread([&] {
client_found = m_node.cj_walletman->doForClient(wallet->GetName(), [&](CCoinJoinClientManager&) {
client_entered.set_value();
release_client_future.wait();
});
});

client_entered_future.wait();
auto remove_future{std::async(std::launch::async, [&] {
// Signal after this worker is scheduled, immediately before it
// contends for the manager-map lock.
remover_started.set_value();
m_node.cj_walletman->removeWallet(wallet->GetName());
})};
remover_started_future.wait();
Comment on lines +419 to +425

@thepastaclaw thepastaclaw Oct 19, 2026

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.

🟡 Suggestion: Synchronize the remover before timing its completion

The remover thread fulfills remover_started before it calls removeWallet(). The waiting thread can therefore be descheduled after setting the promise and before attempting to acquire cs_wallet_manager_map; the 100 ms timeout then passes even though removal never contended with the client callback. Add a synchronization point inside the removal attempt, or use a deterministic test hook/barrier that confirms the remover is actually waiting on the manager lock before checking for timeout.

source: muse-spark-1.3-contributor (phase1-reviewer: general); gpt-6.1-sol (phase2-reviewer: general, dash-core-commit-history)

@thepastaclaw thepastaclaw Oct 25, 2026

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.

I agree the promise is before the mutex acquisition in the strict sense, but I am leaving this as a focused black-box lock-order test rather than adding a production-only contention hook to CJWalletManager. The worker is launched with std::launch::async, signals from the worker immediately before removeWallet(), and the callback remains blocked until the timeout assertion completes; after release, the test waits for removal and verifies the client callback completed before restoring the manager. The new commit also folds the corrective history and makes the functional-session interruption deterministic.


// doForClient holds the manager-map lock while the callback uses the
// client, so removal must wait until the callback has released it. The
// remover-started barrier ensures this checks a running contender rather
// than an async task that has not been scheduled yet.
BOOST_CHECK_EQUAL(remove_future.wait_for(std::chrono::milliseconds{100}), std::future_status::timeout);

@chatgpt-codex-connector chatgpt-codex-connector Bot Oct 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Synchronize the remover before timing its completion

The fresh unit-test barrier added after the earlier RPC-race review holds a known doForClient callback, but it still launches removeWallet and immediately treats this 100 ms timeout as proof that removal blocked. When checking the pre-fix lock-free implementation on a loaded runner, the async thread can remain unscheduled for that entire window and run only after release_client, so every assertion passes without any removal overlapping the acquired client access; synchronize a point inside the remover after it has actually begun contending for the manager-map lock before testing that it cannot complete.

AGENTS.md reference: AGENTS.md:L136-L137

Useful? React with 👍 / 👎.

@thepastaclaw thepastaclaw Oct 19, 2026

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.

Addressed in fbecdeb: the remover now signals from its worker immediately before calling removeWallet(), and the test waits for that signal before checking the timeout. This ensures the async worker has been scheduled and is entering the lock-contending operation rather than remaining unscheduled during the assertion window.


release_client.set_value();
client_thread.join();
BOOST_CHECK(client_found);
BOOST_CHECK_EQUAL(remove_future.wait_for(std::chrono::seconds{1}), std::future_status::ready);
remove_future.get();

// Restore the fixture's manager for its teardown.
m_node.cj_walletman->addWallet(wallet);
}

BOOST_FIXTURE_TEST_CASE(coinjoin_pending_observation_unreadable_tests, CTransactionBuilderTestSetup)
{
// 0.100001 DASH, a valid CoinJoin denomination
Expand Down
1 change: 1 addition & 0 deletions test/functional/test_runner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@
# Longest test should go first, to favor running tests in parallel
'feature_llmq_data_recovery.py',
# vv Tests less than 5m vv
'wallet_coinjoin_mixing.py',
'feature_fee_estimation.py',
'feature_block.py', # NOTE: needs dash_hash to pass
# vv Tests less than 2m vv
Expand Down
Loading
Loading