Skip to content

refactor: delete extra ctors from managers, restore memory-only db for EvoDb and LLMQContext #6923

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

Merged
PastaPastaPasta merged 11 commits into from
Nov 4, 2025
+442 −225
Merged

refactor: delete extra ctors from managers, restore memory-only db for EvoDb and LLMQContext #6923

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
88a2375
refactor: move ctor/dtor to source file, delete extra ctors for LLMQ
kwvg Oct 28, 2025
301bc70
refactor: move ctor/dtor to source file, delete extra ctors for CoinJoin
kwvg Oct 28, 2025
adb5117
refactor: delete extra ctors for chainlock, instantsend, governance
kwvg Oct 26, 2025
52f7160
refactor: move ctor/dtor to source file, delete extra ctors for evo
kwvg Oct 26, 2025
5163e85
refactor: move ctor/dtor to source file, delete extra ctors for mn
kwvg Oct 28, 2025
5dd2e99
refactor: delete extra ctors for netfulfilled, spork
kwvg Oct 28, 2025
556d0b8
refactor: move dtor to source file, delete extra ctors for notif int.
kwvg Oct 26, 2025
a7bec54
refactor: move EvoDb cache size to constructor, use `std::unique_ptr`
kwvg Oct 26, 2025
5b2e54a
refactor: introduce `util::DbWrapperParams` and helper to pull out args
kwvg Oct 29, 2025
8763104
refactor: clean up {c,d}tor for `TestingSetup`s
kwvg Oct 19, 2025
3c85563
fix: keep databases in memory if running unit tests
kwvg Oct 26, 2025
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
1 change: 1 addition & 0 deletions src/init.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1996,6 +1996,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
cache_sizes.coins,
/*block_tree_db_in_memory=*/false,
/*coins_db_in_memory=*/false,
/*dash_dbs_in_memory=*/false,

@knst knst Nov 3, 2025

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.

nit: so far as coins_db_in_memory and dash_dbs_in_memory are always equal, consider merging them to a new flag dbs_in_memory.
LoadChainstate has a bit too many arguments already.

Though, keep block_tree_db_in_memory as it is because it will be refactored later in bitcoin#30965

/*shutdown_requested=*/ShutdownRequested,
/*coins_error_cb=*/[]() {
uiInterface.ThreadSafeMessageBox(
Expand Down
13 changes: 7 additions & 6 deletions src/node/chainstate.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ std::optional<ChainstateLoadingError> LoadChainstate(bool fReset,
int64_t nCoinCacheUsage,
bool block_tree_db_in_memory,
bool coins_db_in_memory,
bool dash_dbs_in_memory,
std::function<bool()> shutdown_requested,
std::function<void()> coins_error_cb)
{
Expand All @@ -73,7 +74,7 @@ std::optional<ChainstateLoadingError> LoadChainstate(bool fReset,
LOCK(cs_main);

evodb.reset();
evodb = std::make_unique<CEvoDB>(util::DbWrapperParams{.path = data_dir, .memory = false, .wipe = fReset || fReindexChainState});
evodb = std::make_unique<CEvoDB>(util::DbWrapperParams{.path = data_dir, .memory = dash_dbs_in_memory, .wipe = fReset || fReindexChainState});
Comment thread
UdjinM6 marked this conversation as resolved.

mnhf_manager.reset();
mnhf_manager = std::make_unique<CMNHFManager>(*evodb);
Expand All @@ -89,8 +90,8 @@ std::optional<ChainstateLoadingError> LoadChainstate(bool fReset,
pblocktree.reset(new CBlockTreeDB(nBlockTreeDBCache, block_tree_db_in_memory, fReset));

DashChainstateSetup(chainman, govman, mn_metaman, mn_sync, sporkman, mn_activeman, chain_helper, cpoolman,
dmnman, evodb, mnhf_manager, llmq_ctx, mempool, data_dir, fReset, fReindexChainState,
consensus_params);
dmnman, evodb, mnhf_manager, llmq_ctx, mempool, data_dir, dash_dbs_in_memory,
/*llmq_dbs_wipe=*/fReset || fReindexChainState, consensus_params);

Comment on lines 92 to 95

@coderabbitai coderabbitai Bot Oct 27, 2025

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 | 🟠 Major

Guard mempool pointer before use in DashChainstateSetup.

mempool is a raw pointer; Setup dereferences it (ConnectManagers, passed into LLMQContext). Add a non‑fatal assert/early check to avoid accidental null deref in tests or future code paths.

Apply this diff near the start of DashChainstateSetup:

 void DashChainstateSetup(ChainstateManager& chainman,
@@
                          const Consensus::Params& consensus_params)
 {
+    CHECK_NONFATAL(mempool != nullptr);

Based on learnings.

Also applies to: 251-253

🤖 Prompt for AI Agents 
In src/node/chainstate.cpp around lines 92-95 (and similarly at 251-253), the
raw pointer mempool is passed into DashChainstateSetup where it may be
dereferenced; add a non-fatal guard at the start of DashChainstateSetup that
checks mempool != nullptr and if null logs an error/warning and returns early
(or skips mempool-dependent setup) to avoid accidental null dereference in tests
or future code paths.

if (fReset) {
pblocktree->WriteReindexing(true);
Expand Down Expand Up @@ -230,8 +231,8 @@ void DashChainstateSetup(ChainstateManager& chainman,
std::unique_ptr<LLMQContext>& llmq_ctx,
CTxMemPool* mempool,
const fs::path& data_dir,
bool fReset,
bool fReindexChainState,
bool llmq_dbs_in_memory,
bool llmq_dbs_wipe,
const Consensus::Params& consensus_params)
{
// Same logic as pblocktree
Expand All @@ -248,7 +249,7 @@ void DashChainstateSetup(ChainstateManager& chainman,
llmq_ctx.reset();
llmq_ctx = std::make_unique<LLMQContext>(chainman, *dmnman, *evodb, mn_metaman, *mnhf_manager, sporkman,
*mempool, mn_activeman.get(), mn_sync,
util::DbWrapperParams{.path = data_dir, .memory = false, .wipe = fReset || fReindexChainState});
util::DbWrapperParams{.path = data_dir, .memory = llmq_dbs_in_memory, .wipe = llmq_dbs_wipe});
mempool->ConnectManagers(dmnman.get(), llmq_ctx->isman.get());
// Enable CMNHFManager::{Process, Undo}Block
mnhf_manager->ConnectManagers(&chainman, llmq_ctx->qman.get());
Expand Down
5 changes: 3 additions & 2 deletions src/node/chainstate.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ std::optional<ChainstateLoadingError> LoadChainstate(bool fReset,
int64_t nCoinCacheUsage,
bool block_tree_db_in_memory,
bool coins_db_in_memory,
bool dash_dbs_in_memory,
std::function<bool()> shutdown_requested = nullptr,
std::function<void()> coins_error_cb = nullptr);

Expand All @@ -126,8 +127,8 @@ void DashChainstateSetup(ChainstateManager& chainman,
std::unique_ptr<LLMQContext>& llmq_ctx,
CTxMemPool* mempool,
const fs::path& data_dir,
bool fReset,
bool fReindexChainState,
bool llmq_dbs_in_memory,
bool llmq_dbs_wipe,
const Consensus::Params& consensus_params);

void DashChainstateSetupClose(std::unique_ptr<CChainstateHelper>& chain_helper,
Expand Down
9 changes: 5 additions & 4 deletions src/test/util/setup_common.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,14 +125,14 @@ std::ostream& operator<<(std::ostream& os, const uint256& num)

void DashChainstateSetup(ChainstateManager& chainman,
NodeContext& node,
bool fReset,
bool fReindexChainState,
bool llmq_dbs_in_memory,
bool llmq_dbs_wipe,
const Consensus::Params& consensus_params)
{
DashChainstateSetup(chainman, *Assert(node.govman.get()), *Assert(node.mn_metaman.get()), *Assert(node.mn_sync.get()),
*Assert(node.sporkman.get()), node.mn_activeman, node.chain_helper, node.cpoolman, node.dmnman,
node.evodb, node.mnhf_manager, node.llmq_ctx, Assert(node.mempool.get()), node.args->GetDataDirNet(),
fReset, fReindexChainState, consensus_params);
llmq_dbs_in_memory, llmq_dbs_wipe, consensus_params);
}

void DashChainstateSetupClose(NodeContext& node)
Expand Down Expand Up @@ -331,7 +331,8 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
m_cache_sizes.coins_db,
m_cache_sizes.coins,
/*block_tree_db_in_memory=*/true,
/*coins_db_in_memory=*/true);
/*coins_db_in_memory=*/true,
/*dash_dbs_in_memory=*/true);
assert(!maybe_load_error.has_value());

auto maybe_verify_error = VerifyLoadedChainstate(
Expand Down
4 changes: 2 additions & 2 deletions src/test/util/setup_common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,8 @@ static constexpr CAmount CENT{1000000};
/** Initialize Dash-specific components during chainstate initialization (NodeContext-friendly aliases) */
void DashChainstateSetup(ChainstateManager& chainman,
node::NodeContext& node,
bool fReset,
bool fReindexChainState,
bool llmq_dbs_in_memory,
bool llmq_dbs_wipe,

@knst knst Nov 3, 2025

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.

nit; it's a bit risky to change one bool flag to other with completely different meaning; but so far as it is DashChainstateSetup I think it's not a big deal.

const Consensus::Params& consensus_params);
Comment on lines 88 to 92

@coderabbitai coderabbitai Bot Oct 28, 2025

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

API change: new LLMQ DB flags—verify all call sites and consider a small options struct.

Adding llmq_dbs_in_memory and llmq_dbs_wipe is fine, but increases boolean-parameter soup. Consider grouping LLMQ DB options into a small struct to reduce call-site errors. Please also verify all test helpers/callers are updated.

Run:

🏁 Script executed:

#!/bin/bash
rg -nP '\bDashChainstateSetup\s*\(' -C2 -- src

Length of output: 3890


---

🏁 Script executed:

```shell
#!/bin/bash
# Find all DashChainstateSetup calls (including in comments/docs)
rg -n 'DashChainstateSetup' -- src | head -30

Length of output: 2179

🏁 Script executed:

#!/bin/bash
# Look for indirect callers (functions that might call DashChainstateSetup)
rg -nP 'LoadChainstate|InitCoinsCache' -C1 -- src/test | head -30

Length of output: 2063


---

🏁 Script executed:

```shell
#!/bin/bash
# View the production call site context in detail
sed -n '85,100p' src/node/chainstate.cpp

Length of output: 839

🏁 Script executed:

#!/bin/bash
# Verify no additional DashChainstateSetup calls exist in production code
rg -nP 'DashChainstateSetup\s*\(' -- src/node src/init --type cpp | grep -v 'void DashChainstateSetup' | grep -v '//'

Length of output: 228

Add inline comment to production call site; bool-parameter design acceptable with comments.

The production call at src/node/chainstate.cpp:92 passes dash_dbs_in_memory for the llmq_dbs_in_memory parameter without an inline comment, creating inconsistency with test code which properly documents both bool parameters. Add /*llmq_dbs_in_memory=*/ comment before dash_dbs_in_memory for clarity. While a struct wrapper could reduce parameter count, the current design with inline comments is workable given only 2 new params were added.

🤖 Prompt for AI Agents 
In src/test/util/setup_common.h around lines 88 to 92, the production call at
src/node/chainstate.cpp:92 omits an inline comment for the bool parameter
mapping; update the call site to annotate the boolean argument by adding
/*llmq_dbs_in_memory=*/ immediately before dash_dbs_in_memory (so it matches the
test-site style for documenting bool parameters); leave the function signature
unchanged.

Comment on lines +90 to 92

@coderabbitai coderabbitai Bot Oct 29, 2025

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

Audit all DashChainstateSetup call sites for new bool ordering.

API now takes (llmq_dbs_in_memory, llmq_dbs_wipe). Verify all prod/tests map args correctly and annotate at call sites to avoid boolean swaps.

Run to review usage across the tree:

🏁 Script executed:

#!/bin/bash
# Show all call sites with context
rg -nP --type=cpp -C2 '\bDashChainstateSetup\s*\(' src

# Quick sanity: ensure named inline comments present at prod call(s)
rg -nP --type=cpp -C1 'DashChainstateSetup\s*\([^)]*llmq_dbs_in_memory' src/node src

# List any legacy signature usage that might remain (2 bools often ambiguous)
rg -nP --type=cpp -C2 'DashChainstateSetup\s*\(\s*[^,]+,\s*[^,]+,\s*(true|false)\s*,\s*(true|false)\s*,' src

Length of output: 4431

Add inline comment for llmq_dbs_in_memory at production call site.

Verification confirms the new bool parameters are mapped correctly across all call sites, but the production call at src/node/chainstate.cpp:92–94 lacks the inline comment for llmq_dbs_in_memory. The parameter llmq_dbs_wipe is already annotated (/*llmq_dbs_wipe=*/), so the test call sites in src/test/validation_chainstatemanager_tests.cpp show the correct annotation pattern for both parameters. Update line 94 to add /*llmq_dbs_in_memory=*/ before dash_dbs_in_memory for consistency.

🤖 Prompt for AI Agents 
In src/node/chainstate.cpp around lines 92 to 94, the production call to the
function declared in src/test/util/setup_common.h is missing the inline comment
for the llmq_dbs_in_memory parameter; update line 94 to add the inline
annotation /*llmq_dbs_in_memory=*/ immediately before the dash_dbs_in_memory
argument (keeping the existing /*llmq_dbs_wipe=*/ annotation intact) so the
boolean arguments mirror the test callsite style.

void DashChainstateSetupClose(node::NodeContext& node);

Expand Down
4 changes: 2 additions & 2 deletions src/test/validation_chainstatemanager_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ BOOST_AUTO_TEST_CASE(chainstatemanager)
/*cache_size_bytes=*/1 << 23, /*in_memory=*/true, /*should_wipe=*/false);
WITH_LOCK(::cs_main, c1.InitCoinsCache(1 << 23));

DashChainstateSetup(manager, m_node, /*fReset=*/false, /*fReindexChainState=*/false, consensus_params);
DashChainstateSetup(manager, m_node, /*llmq_dbs_in_memory=*/true, /*llmq_dbs_wipe=*/false, consensus_params);

BOOST_CHECK(!manager.IsSnapshotActive());
BOOST_CHECK(WITH_LOCK(::cs_main, return !manager.IsSnapshotValidated()));
Expand Down Expand Up @@ -84,7 +84,7 @@ BOOST_AUTO_TEST_CASE(chainstatemanager)
);
chainstates.push_back(&c2);

DashChainstateSetup(manager, m_node, /*fReset=*/false, /*fReindexChainState=*/false, consensus_params);
DashChainstateSetup(manager, m_node, /*llmq_dbs_in_memory=*/true, /*llmq_dbs_wipe=*/false, consensus_params);

BOOST_CHECK_EQUAL(manager.SnapshotBlockhash().value(), snapshot_blockhash);

Expand Down
Loading