Skip to content

test: rebuild peerman and cj_walletman across chainstate reloads #7625

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
PastaPastaPasta wants to merge 1 commit into
base: develop
Choose a base branch
from
+39 −0
Open

test: rebuild peerman and cj_walletman across chainstate reloads #7625

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
39 changes: 39 additions & 0 deletions src/test/util/setup_common.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,26 @@ void ChainTestingSetup::LoadVerifyActivateChainstate()
{
auto& chainman{*Assert(m_node.chainman)};

// peerman and cj_walletman hold references to llmq_ctx, which

@knst knst Aug 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why objects should be re-created?
I believe that code should works just fine.
@thepastaclaw can you check it?

diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -326,6 +326,22 @@ void ChainTestingSetup::LoadVerifyActivateChainstate()
{
   auto& chainman{*Assert(m_node.chainman)};

+    // peerman and cj_walletman reference llmq_ctx, the mempool and the Dash
+    // managers recreated by the reload below. No test uses them across a
+    // reload: destroy them (clearing connman's raw m_msgproc pointer first)
+    // so future use after a reload fails on a null pointer instead of
+    // silently reading freed memory; such a test must rebuild them itself,
+    // as AppInitMain constructs them only after the chainstate is loaded.
+    if (m_node.peerman) {
+        CConnman::Options connman_options;
+        connman_options.socketEventsMode = ::g_socket_events_mode;
+        m_node.connman->Init(connman_options);
+        m_node.peerman.reset();
+    }
+#ifdef ENABLE_WALLET
+    m_node.cj_walletman.reset();
+#endif // ENABLE_WALLET

@thepastaclaw thepastaclaw Aug 22, 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.

Checked every LoadVerifyActivateChainstate() caller. You’re right: re-creation is not required by any current test. It was only intended to preserve TestingSetup’s pre-reload “complete environment” and reconnect connman’s m_msgproc; the reload itself only needs the stale owners destroyed. No caller uses peerman or cj_walletman after reload (post-reload mining uses chainman directly), and rebuilding cj_walletman introduces empty-wallet-map semantics that cannot be meaningfully preserved or exercised here. Clearing connman’s raw pointer before resetting peerman, then leaving both managers null as in your diff, fixes the dangling references with less lifecycle policy. I agree with the reset-only version.

// LoadChainstate() recreates, and to the mempool and Dash managers, which
// the reindex path below replaces. Tear them down first and rebuild them
// afterwards, like AppInitMain constructs them only after the chainstate
// is loaded. Fixtures that never built them (e.g. plain ChainTestingSetup)
// are unaffected.
const bool rebuild_peerman{m_node.peerman != nullptr};
if (rebuild_peerman) {
// Drop connman's raw m_msgproc pointer to the PeerManager destroyed
// below; it is wired back up after the rebuild.
CConnman::Options connman_options;
connman_options.socketEventsMode = ::g_socket_events_mode;
m_node.connman->Init(connman_options);
}
m_node.peerman.reset();
#ifdef ENABLE_WALLET
const bool rebuild_cj_walletman{m_node.cj_walletman != nullptr};
m_node.cj_walletman.reset();

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 20, 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 Preserve wallet registrations when rebuilding CoinJoin manager

When a wallet-enabled fixture has already added a wallet through CoinJoinLoaderImpl::AddWallet() and then calls LoadVerifyActivateChainstate(), this reset destroys the only m_wallet_manager_map entries, while the newly constructed manager below is never repopulated from the still-loaded wallets. Subsequent loader operations such as WithClient() therefore return false and CoinJoin processing silently stops for those wallets; preserve or re-register the loaded wallets as part of the rebuild.

AGENTS.md reference: AGENTS.md:L15-L17

Useful? React with 👍 / 👎.

@PastaPastaPasta PastaPastaPasta Aug 20, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Addressed in c43e3e4 by documenting the invariant at the rebuild site rather than re-registering: no current fixture has wallets loaded when the chainstate is reloaded (verified all three caller families), and setup_common cannot reach the shared_ptr that addWallet() requires without new plumbing. Details in the thepastaclaw thread on this line.

🤖 Posted autonomously by Claude on behalf of pasta.

#endif // ENABLE_WALLET

node::ChainstateLoadOptions options{ChainstateLoadOptionsForTest()};

if (options.reindex || options.reindex_chainstate) {
Expand Down Expand Up @@ -358,6 +378,25 @@ void ChainTestingSetup::LoadVerifyActivateChainstate()
if (!chainman.ActiveChainstate().ActivateBestChain(state)) {
throw std::runtime_error(strprintf("ActivateBestChain failed. (%s)", state.ToString()));
}

#ifdef ENABLE_WALLET
if (rebuild_cj_walletman) {
// The rebuilt manager starts with an empty wallet map. No present
// fixture has wallets loaded when the chainstate is reloaded; a future
// test that does must re-register them with the new manager (see
// CoinJoinLoaderImpl::AddWallet).
m_node.cj_walletman = CJWalletManager::make(chainman, *m_node.dmnman, *m_node.mn_metaman, *m_node.mempool,
*m_node.mn_sync, *m_node.isman, /*relay_txes=*/true);
}
#endif // ENABLE_WALLET
if (rebuild_peerman) {
m_node.peerman = MakePeerManager(*m_node.connman, m_node, m_node.banman.get(),
/*ignore_incoming_txs=*/false);
CConnman::Options connman_options;
connman_options.m_msgproc = m_node.peerman.get();
connman_options.socketEventsMode = ::g_socket_events_mode;
m_node.connman->Init(connman_options);
Comment on lines +382 to +398

@thepastaclaw thepastaclaw Aug 20, 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.

🟡 Suggestion: Exercise the rebuilt managers in a regression test

The snapshot tests call LoadVerifyActivateChainstate() but never access m_node.peerman or m_node.cj_walletman afterward, and they do not verify that CConnman routes message processing through the replacement PeerManager. As a result, removing this rebuild or leaving CConnman's m_msgproc connected to the old manager would not necessarily fail the current suite—the pre-PR dangling-reference behavior passed these same reload tests. Add a focused snapshot-restart test that uses the replacement PeerManager through CConnman after reload and exercises a CoinJoin manager operation against the rebuilt dependencies when wallet support is enabled.

source: ['codex']

}
}

TestingSetup::TestingSetup(
Expand Down
Loading