refactor: delete extra ctors from managers, restore memory-only db for EvoDb and LLMQContext
#6923
Conversation
kwvg
changed the title
EvoDb and LLMQContext and other misc. fixesEvoDb and LLMQContext and other misc. fixes
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis pull request systematically refactors object lifecycle management and database configuration across the codebase. The changes delete copy constructors and copy assignment operators for numerous classes (CBLSWorker, CChainLocksHandler, ChainLockSigner, CDeterministicMNManager, CEvoDB, CInstantSendManager, and many others), enforcing non-copyable semantics. Concurrently, a new Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (10)
src/rpc/blockchain.cpp (1)
254-261: Doc key mismatch: “hash” vs returned “blockhash”.getbestchainlock help documents field “hash”, but the code returns “blockhash” at Line 278. Align to “blockhash” in help.
Apply this diff:
- {RPCResult::Type::STR_HEX, "hash", "The block hash hex-encoded"}, + {RPCResult::Type::STR_HEX, "blockhash", "The block hash hex-encoded"},src/llmq/signing.cpp (2)
265-266: Fix format specifier for func in LogPrint.func is a const char*, not an int; use %s.
- LogPrint(BCLog::LLMQ, "CRecoveredSigsDb::%d -- deleted %d entries\n", __func__, toDelete.size()); + LogPrint(BCLog::LLMQ, "CRecoveredSigsDb::%s -- deleted %d entries\n", __func__, toDelete.size());
330-331: Same logging format issue here.Use %s for func.
- LogPrint(BCLog::LLMQ, "CRecoveredSigsDb::%d -- deleted %d entries\n", __func__, cnt); + LogPrint(BCLog::LLMQ, "CRecoveredSigsDb::%s -- deleted %d entries\n", __func__, cnt);src/coinjoin/server.cpp (4)
100-114: Potential null dereference on meta infom_mn_metaman.GetMetaInfo(dmn->proTxHash) is used without a null check. If it can return nullptr, this dereferences blindly and will crash. Add a guard before calling GetLastDsq()/GetDsqThreshold().
Would you like a patch that safely handles a missing meta entry?
249-253: Fix log format specifier for size()Use %zu for size_t. Current "%s" is incorrect.
Apply this diff:
- LogPrint(BCLog::COINJOIN, "DSSIGNFINALTX -- vecTxIn.size() %s\n", vecTxIn.size()); + LogPrint(BCLog::COINJOIN, "DSSIGNFINALTX -- vecTxIn.size() %zu\n", vecTxIn.size());
283-289: Fix log format specifier for entriesentries is int; format uses %lu. Use %d (or cast to size_t and %zu).
- if (int entries = GetEntriesCount(); entries != 0) LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckPool -- entries count %lu\n", entries); + if (int entries = GetEntriesCount(); entries != 0) LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckPool -- entries count %d\n", entries);
845-852: Fix log format specifier for nDisconnectednDisconnected is unsigned int but logged as %llu. Use %u.
- LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- can't continue, %llu client(s) disconnected, nSessionID: %d nSessionDenom: %d (%s)\n", + LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- can't continue, %u client(s) disconnected, nSessionID: %d nSessionDenom: %d (%s)\n",src/coinjoin/client.cpp (3)
1041-1046: Fix potential underflow when computing nCountNotExcludedIf vecMasternodesUsed.size() > nCountEnabled, size_t subtraction underflows, skewing logs/branching.
- size_t nCountNotExcluded = nCountEnabled - vecMasternodesUsed.size(); + if (vecMasternodesUsed.size() >= nCountEnabled) { + WalletCJLogPrint(m_wallet, "CCoinJoinClientManager::%s -- no MNs left to choose\n", __func__); + return nullptr; + } + size_t nCountNotExcluded = nCountEnabled - vecMasternodesUsed.size();Also applies to: 1051-1053
713-717: Data races on cached block heights; make them atomic (and use load/store)nCachedBlockHeight and nCachedLastSuccessBlock are read/written across threads without a lock, causing races. Use std::atomic and load/store.
-void CCoinJoinClientManager::UpdatedSuccessBlock() -{ - nCachedLastSuccessBlock = nCachedBlockHeight; -} +void CCoinJoinClientManager::UpdatedSuccessBlock() +{ + nCachedLastSuccessBlock.store(nCachedBlockHeight.load()); +} @@ -void CCoinJoinClientManager::UpdatedBlockTip(const CBlockIndex* pindex) -{ - nCachedBlockHeight = pindex->nHeight; +void CCoinJoinClientManager::UpdatedBlockTip(const CBlockIndex* pindex) +{ + nCachedBlockHeight.store(pindex->nHeight); }Pair with the header change making these members std::atomic. Also update WaitForAnotherBlock() to use .load():
- return nCachedBlockHeight - nCachedLastSuccessBlock < nMinBlocksToWait; + return nCachedBlockHeight.load() - nCachedLastSuccessBlock.load() < nMinBlocksToWait;Also applies to: 1846-1848
1944-1949: Fix race in Flush(): use manager under the map lock or a shared handleGet(name) drops the lock and returns a raw pointer; concurrent Remove() can erase it, leading to UAF.
-void CoinJoinWalletManager::Flush(const std::string& name) -{ - auto clientman = Assert(Get(name)); - clientman->ResetPool(); - clientman->StopMixing(); -} +void CoinJoinWalletManager::Flush(const std::string& name) +{ + LOCK(cs_wallet_manager_map); + auto it = m_wallet_manager_map.find(name); + if (it == m_wallet_manager_map.end()) return; + it->second->ResetPool(); + it->second->StopMixing(); +}Alternative: store shared_ptr in m_wallet_manager_map and return a shared_ptr from Get() to extend lifetime during Flush().
🧹 Nitpick comments (25)
src/masternode/active/notificationinterface.h (1)
21-25: Consider explicitly deleting move operations for clarity.While the move constructor and move assignment operator are implicitly deleted when copy operations are deleted, explicitly deleting them would improve code clarity and consistency with the codebase-wide refactoring pattern.
Add explicit deletions after line 23:
ActiveNotificationInterface& operator=(const ActiveNotificationInterface&) = delete; +ActiveNotificationInterface(ActiveNotificationInterface&&) = delete; +ActiveNotificationInterface& operator=(ActiveNotificationInterface&&) = delete; explicit ActiveNotificationInterface(ActiveContext& active_ctx, CActiveMasternodeManager& mn_activeman);src/qt/governancelist.cpp (1)
495-501: LGTM: explicit named param improves clarity. Consider direct key ownership check.Instead of building a script and calling isSpendable(), prefer checking private key presence for the voting key ID (clearer and cheaper):
- mn_list.ForEachMN(/*onlyValid=*/true, [&](const auto& dmn) { - // Check if wallet owns the voting key using the same logic as RPC - const CScript script = GetScriptForDestination(PKHash(dmn.pdmnState->keyIDVoting)); - if (walletModel->wallet().isSpendable(script)) { + mn_list.ForEachMN(/*onlyValid=*/true, [&](const auto& dmn) { + // Wallet owns the private key for voting? + if (walletModel->wallet().havePrivKey(dmn.pdmnState->keyIDVoting)) { votableMasternodes[dmn.proTxHash] = dmn.pdmnState->keyIDVoting; } });If havePrivKey(...) isn’t available in your wallet interface, keep current code or switch to an equivalent IsMine/HaveKey call.
src/llmq/signing_shares.h (1)
419-421: Constructor deletions are good. Optionally delete move ops for explicitness.Implicit moves are already disabled by non-movable members, but making it explicit helps readability.
CSigSharesManager() = delete; CSigSharesManager(const CSigSharesManager&) = delete; CSigSharesManager& operator=(const CSigSharesManager&) = delete; + CSigSharesManager(CSigSharesManager&&) = delete; + CSigSharesManager& operator=(CSigSharesManager&&) = delete;src/governance/signing.h (1)
62-64: Good tightening of semantics. Optionally delete move ops too.Make move ctor/assignment explicitly deleted for clarity.
GovernanceSigner() = delete; GovernanceSigner(const GovernanceSigner&) = delete; GovernanceSigner& operator=(const GovernanceSigner&) = delete; + GovernanceSigner(GovernanceSigner&&) = delete; + GovernanceSigner& operator=(GovernanceSigner&&) = delete;src/coinjoin/coinjoin.h (1)
334-335: Out-of-line definitions verified; consider explicitly deleting move operationsCtor/dtor definitions exist in src/coinjoin/coinjoin.cpp (lines 125, 127, 401, 403), preventing ODR/link errors. ✓
CDSTXManager (src/coinjoin/coinjoin.h:375–379) already deletes copy operations but does not explicitly delete move ctor/assignment. Given the class contains
Mutex cs_mapdstxandstd::map mapDSTX, explicitly forbidding moves would prevent accidental move construction/assignment:class CDSTXManager { Mutex cs_mapdstx; std::map<uint256, CCoinJoinBroadcastTx> mapDSTX GUARDED_BY(cs_mapdstx); public: CDSTXManager(const CDSTXManager&) = delete; CDSTXManager& operator=(const CDSTXManager&) = delete; + CDSTXManager(CDSTXManager&&) = delete; + CDSTXManager& operator=(CDSTXManager&&) = delete; explicit CDSTXManager(); ~CDSTXManager();src/evo/deterministicmns.cpp (3)
337-341: DecreaseScores: iterating valid MNs only is appropriate.
Collecting CDeterministicMNCPtr and calling PoSeDecrease(*ptr) avoids races and is safe. Minor nit: rename loop var from proTxHash to dmnPtr for clarity.- for (const auto& proTxHash : toDecrease) { - PoSeDecrease(*proTxHash); + for (const auto& dmnPtr : toDecrease) { + PoSeDecrease(*dmnPtr); }
249-255: Order stability is deterministic and critical—add documentation to clarify.The result vector's order is critical for correctness: the RPC caller (src/rpc/masternode.cpp:309-311) uses array indices to compute sequential block heights (
int h = nChainTipHeight + 1 + i). Iteration is deterministic becausemnMapis animmer::map(immutable persistent map with defined iteration order). Document thatForEachMNSharediteration order is stable and why it matters to prevent future refactoring mistakes.
279-281: Add comment explaining why all MNs (including PoSe-banned) are included in SML.The current code intentionally includes all masternodes in the simplified list with their validity status. The
to_sml_entry()method sets each entry'sisValidfield to!state.IsBanned(), so PoSe-banned nodes are included but marked invalid. This is necessary for consensus-critical merkle root calculation. Add a comment clarifying this design to avoid future confusion about theonlyValid=falseparameter.src/llmq/debug.h (1)
104-107: Non-copyable semantics look good; drop redundantexpliciton default ctor.The deletions and dtor enforce clear ownership. The
expliciton a no-arg constructor has no effect; consider removing for clarity.Apply:
- explicit CDKGDebugManager(); + CDKGDebugManager();src/masternode/sync.h (1)
56-61: Make CMasternodeSync non-copyable — LGTM.Constructors/assignment deletions and dtor are appropriate for owning/synchronized state. The
expliciton a multi-arg ctor is redundant but harmless.src/node/chainstate.h (1)
96-97: Boolean parameter proliferation in LoadChainstate: consolidation recommended but not yet implementedThe concern raised in the original review remains valid. The current signature contains 9 scattered boolean parameters:
fPruneMode,is_addrindex_enabled,is_governance_enabled,is_spentindex_enabled,is_timeindex_enabled,is_txindex_enabled(index-related flags)block_tree_db_in_memory,coins_db_in_memory,dash_dbs_in_memory(database flags)This creates readability and maintainability concerns at call sites (src/init.cpp:1970 and src/test/util/setup_common.cpp:306), which currently pass each boolean individually. The suggested refactoring to group these via a
DbInitFlagsstruct is still a worthwhile improvement to reduce error-proneness, though not yet implemented.src/evo/evodb.cpp (1)
35-39: Prefer using db_params.cache_size (with fallback) instead of a hardcoded 64 MiB.Keeps configuration centralized and consistent across DBs; still allows a default for EvoDB.
Apply this diff if util::DbWrapperParams exposes cache_size:
-CEvoDB::CEvoDB(const util::DbWrapperParams& db_params) : - db{util::MakeDbWrapper({db_params.path / "evodb", /*cache_size=*/64 << 20, db_params.memory, db_params.wipe})}, +CEvoDB::CEvoDB(const util::DbWrapperParams& db_params) : + db{util::MakeDbWrapper({db_params.path / "evodb", + /*cache_size=*/ db_params.cache_size > 0 ? db_params.cache_size : (64 << 20), + db_params.memory, db_params.wipe})}, rootBatch{*db}, rootDBTransaction{*db, rootBatch}, curDBTransaction{rootDBTransaction, rootDBTransaction}Confirm the field name (cache_size) and intended default for EvoDB.
src/instantsend/instantsend.cpp (1)
90-104: Make Stop() resilient if Interrupt wasn’t called.Current assert can crash in mis-ordered shutdowns. Optionally make Stop() idempotent by triggering the interrupt.
void CInstantSendManager::Stop() { - if (auto signer = m_signer.load(std::memory_order_acquire); signer) { + if (auto signer = m_signer.load(std::memory_order_acquire); signer) { signer->Stop(); } - // make sure to call InterruptWorkerThread() first - if (!workInterrupt) { - assert(false); - } + // Be defensive: ensure the thread is interrupted before join + if (!workInterrupt) { + workInterrupt(); + } if (workThread.joinable()) { workThread.join(); } }If applied here, consider mirroring this in CSigningManager::StopWorkerThread for consistency.
src/llmq/signing.cpp (1)
30-33: Use db_params.cache_size (with default) instead of a fixed 8 MiB here too.Keeps DB cache configuration unified; pick a small default when unset to preserve footprint.
-CRecoveredSigsDb::CRecoveredSigsDb(const util::DbWrapperParams& db_params) : - db{util::MakeDbWrapper({db_params.path / "llmq" / "recsigdb", /*cache_size=*/8 << 20, db_params.memory, db_params.wipe})} +CRecoveredSigsDb::CRecoveredSigsDb(const util::DbWrapperParams& db_params) : + db{util::MakeDbWrapper({db_params.path / "llmq" / "recsigdb", + /*cache_size=*/ db_params.cache_size > 0 ? db_params.cache_size : (8 << 20), + db_params.memory, db_params.wipe})}Confirm the intended default cache for RecoveredSigs DB.
src/evo/evodb.h (2)
43-43: Ensure non-null invariant for db.db is a std::unique_ptr; methods dereference it unconditionally. Add a cheap invariant check to prevent accidental null deref.
// ... - CDBWrapper& GetRawDB() + CDBWrapper& GetRawDB() { - return *db; + assert(db != nullptr); + return *db; }
99-102: Make IsEmpty const and guard invariant.Minor API polish and safety.
- bool IsEmpty() { return db->IsEmpty(); } + [[nodiscard]] bool IsEmpty() const { assert(db != nullptr); return db->IsEmpty(); }Optionally add a threadsafety annotation for consistency:
- [[nodiscard]] bool IsEmpty() const { assert(db != nullptr); return db->IsEmpty(); } + [[nodiscard]] bool IsEmpty() const EXCLUSIVE_LOCKS_REQUIRED(!cs) { assert(db != nullptr); return db->IsEmpty(); }Also applies to: 111-111
src/instantsend/db.cpp (1)
37-37: Upgrade path is correct; minor nit: avoid magic number repetition.
- Wiping and recreating via util::MakeDbWrapper on version bump is fine.
- Consider a single constexpr for the 32 MiB cache to avoid drift.
- db{util::MakeDbWrapper({db_params.path / "llmq" / "isdb", /*cache_size=*/32 << 20, db_params.memory, db_params.wipe})} + static constexpr uint32_t IS_DB_CACHE_BYTES{32u << 20}; + db{util::MakeDbWrapper({db_params.path / "llmq" / "isdb", IS_DB_CACHE_BYTES, db_params.memory, db_params.wipe})} @@ - Upgrade({db_params.path / "llmq" / "isdb", /*cache_size=*/32 << 20, db_params.memory, /*wipe=*/true}); + Upgrade({db_params.path / "llmq" / "isdb", IS_DB_CACHE_BYTES, db_params.memory, /*wipe=*/true}); @@ void CInstantSendDb::Upgrade(const util::DbWrapperParams& db_params) - db = util::MakeDbWrapper(db_params); + db = util::MakeDbWrapper(db_params);Optional: If db_params.wipe can be true on entry, you’re effectively wiping twice (ctor + Upgrade). It’s harmless but can be avoided by documenting precedence or gating the ctor’s initial MakeDbWrapper when db_params.wipe is true.
Also applies to: 44-44
src/coinjoin/server.cpp (1)
351-361: Avoid treating lock contention as invalid transactionIf TRY_LOCK(::cs_main) fails, the code resets the session and signals ERR_INVALID_TX. This is a reliability footgun under contention. Prefer retry/schedule-later instead of hard-failing the mix.
Would you like a small refactor that returns early on lock contention and retries CommitFinalTransaction on the next maintenance tick? Based on learnings
src/evo/deterministicmns.h (2)
291-299: Consider templated overload to avoid std::function overheadstd::function introduces type‑erasure/alloc overhead in hot paths. Add a templated overload to keep zero‑overhead lambdas while preserving this API.
- void ForEachMN(bool onlyValid, std::function<void(const CDeterministicMN&)> cb) const + template <typename F> + void ForEachMN(bool onlyValid, F&& cb) const(Body unchanged; accept any invocable.)
307-314: Same for ForEachMNSharedProvide a templated overload to avoid std::function in tight loops.
- void ForEachMNShared(bool onlyValid, std::function<void(const CDeterministicMNCPtr&)> cb) const + template <typename F> + void ForEachMNShared(bool onlyValid, F&& cb) constsrc/test/util/setup_common.cpp (1)
238-254: Optional: reorder teardown for symmetryMinor: consider resetting managers/DBs before fs::remove_all(m_path_root) to avoid any accidental late file touches if the order changes later. Behavior is fine today (mem-only), this is just hygiene.
- LogInstance().DisconnectTestLogger(); - fs::remove_all(m_path_root); - gArgs.ClearArgs(); - ... - m_node.evodb.reset(); + LogInstance().DisconnectTestLogger(); + ... + m_node.evodb.reset(); + ... + fs::remove_all(m_path_root); + gArgs.ClearArgs();src/coinjoin/client.cpp (2)
160-171: Avoid storing references to std::unique_ptr; prefer raw/shared pointerHolding const std::unique_ptr<...>& as a member ties you to the owner’s storage and risks a dangling reference if the owner replaces its unique_ptr. Store a raw pointer (or shared_ptr if shared ownership is needed).
-CCoinJoinClientManager::CCoinJoinClientManager(..., - const std::unique_ptr<CCoinJoinClientQueueManager>& queueman) : +CCoinJoinClientManager::CCoinJoinClientManager(..., + CCoinJoinClientQueueManager* queueman) : ... - m_queueman{queueman} + m_queueman{queueman}Apply the same pattern to declarations and to CoinJoinWalletManager/CCoinJoinClientSession constructors and members.
1901-1917: Wallet manager ctor wiring is fine; see note on unique_ptr referencesCtor is fine. Consider the earlier remark about storing m_queueman as a raw/shared pointer to avoid referencing a unique_ptr.
src/coinjoin/client.h (2)
81-85: Prefer pointer over reference-to-unique_ptr for dependenciesHolding const std::unique_ptr<...>& as a member is brittle. Store CCoinJoinClientQueueManager* instead (or std::shared_ptr if shared lifetime is intended). Propagate through constructors.
- const std::unique_ptr<CCoinJoinClientQueueManager>& m_queueman; + CCoinJoinClientQueueManager* m_queueman;Apply similarly in CCoinJoinClientManager and CCoinJoinClientSession members/ctors.
93-109: Map traversal helpers: OK; minor const-correctness nitForAnyCJClientMan currently passes pair.second as a non-const unique_ptr&. Most lambdas treat it as const anyway. Consider const overloading or passing const std::unique_ptr<>& when mutation isn’t needed.
- bool ForAnyCJClientMan(Callable&& func) EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet_manager_map) + bool ForAnyCJClientMan(Callable&& func) EXCLUSIVE_LOCKS_REQUIRED(!cs_wallet_manager_map) { LOCK(cs_wallet_manager_map); - return ranges::any_of(m_wallet_manager_map, [&](auto& pair) { return func(pair.second); }); + return ranges::any_of(m_wallet_manager_map, [&](auto& pair) { + return func(static_cast<const std::unique_ptr<CCoinJoinClientManager>&>(pair.second)); + }); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (78)
contrib/containers/ci/ci-slim.Dockerfile(1 hunks)contrib/seeds/makeseeds.py(1 hunks)src/bls/bls.cpp(1 hunks)src/bls/bls.h(1 hunks)src/bls/bls_worker.h(1 hunks)src/chainlock/chainlock.h(1 hunks)src/chainlock/signing.h(1 hunks)src/coinjoin/client.cpp(4 hunks)src/coinjoin/client.h(3 hunks)src/coinjoin/coinjoin.cpp(2 hunks)src/coinjoin/coinjoin.h(2 hunks)src/coinjoin/server.cpp(1 hunks)src/coinjoin/server.h(1 hunks)src/dbwrapper.h(1 hunks)src/dsnotificationinterface.cpp(1 hunks)src/dsnotificationinterface.h(1 hunks)src/evo/chainhelper.h(1 hunks)src/evo/creditpool.cpp(1 hunks)src/evo/creditpool.h(1 hunks)src/evo/deterministicmns.cpp(7 hunks)src/evo/deterministicmns.h(3 hunks)src/evo/evodb.cpp(2 hunks)src/evo/evodb.h(5 hunks)src/evo/mnhftx.cpp(1 hunks)src/evo/mnhftx.h(1 hunks)src/evo/smldiff.cpp(2 hunks)src/evo/specialtxman.cpp(2 hunks)src/governance/governance.cpp(2 hunks)src/governance/governance.h(1 hunks)src/governance/signing.h(1 hunks)src/init.cpp(3 hunks)src/init/common.cpp(1 hunks)src/instantsend/db.cpp(1 hunks)src/instantsend/db.h(2 hunks)src/instantsend/instantsend.cpp(1 hunks)src/instantsend/instantsend.h(2 hunks)src/instantsend/signing.h(1 hunks)src/llmq/blockprocessor.cpp(2 hunks)src/llmq/blockprocessor.h(1 hunks)src/llmq/context.cpp(1 hunks)src/llmq/context.h(1 hunks)src/llmq/debug.cpp(1 hunks)src/llmq/debug.h(1 hunks)src/llmq/dkgsessionmgr.cpp(1 hunks)src/llmq/dkgsessionmgr.h(2 hunks)src/llmq/quorums.cpp(2 hunks)src/llmq/quorums.h(2 hunks)src/llmq/signing.cpp(2 hunks)src/llmq/signing.h(3 hunks)src/llmq/signing_shares.h(1 hunks)src/llmq/snapshot.cpp(1 hunks)src/llmq/snapshot.h(1 hunks)src/llmq/utils.cpp(2 hunks)src/masternode/active/context.h(1 hunks)src/masternode/active/notificationinterface.cpp(1 hunks)src/masternode/active/notificationinterface.h(1 hunks)src/masternode/meta.cpp(1 hunks)src/masternode/meta.h(1 hunks)src/masternode/node.cpp(2 hunks)src/masternode/node.h(1 hunks)src/masternode/sync.cpp(1 hunks)src/masternode/sync.h(1 hunks)src/netfulfilledman.cpp(1 hunks)src/netfulfilledman.h(1 hunks)src/node/chainstate.cpp(6 hunks)src/node/chainstate.h(4 hunks)src/qt/governancelist.cpp(1 hunks)src/qt/masternodelist.cpp(2 hunks)src/rpc/blockchain.cpp(1 hunks)src/rpc/evo.cpp(1 hunks)src/rpc/governance.cpp(1 hunks)src/rpc/masternode.cpp(7 hunks)src/spork.h(1 hunks)src/test/evo_deterministicmns_tests.cpp(1 hunks)src/test/util/setup_common.cpp(7 hunks)src/test/util/setup_common.h(1 hunks)src/test/validation_chainstatemanager_tests.cpp(2 hunks)test/functional/wallet_signrawtransactionwithwallet.py(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
PR: dashpay/dash#6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/node/chainstate.cppsrc/llmq/context.h
📚 Learning: 2025-01-02T21:50:00.967Z
Learnt from: kwvg
PR: dashpay/dash#6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
Applied to files:
src/llmq/context.h
📚 Learning: 2024-12-29T17:43:41.755Z
Learnt from: kwvg
PR: dashpay/dash#6504
File: src/llmq/quorums.cpp:224-224
Timestamp: 2024-12-29T17:43:41.755Z
Learning: The `CQuorumManager` is fully initialized by `LLMQContext`, addressing any concerns about the manager’s initialization sequence.
Applied to files:
src/llmq/context.h
📚 Learning: 2025-01-07T18:50:44.838Z
Learnt from: knst
PR: dashpay/dash#6511
File: src/evo/deterministicmns.cpp:1369-1373
Timestamp: 2025-01-07T18:50:44.838Z
Learning: The functions `MigrateDBIfNeeded` and `MigrateDBIfNeeded2` in `src/evo/deterministicmns.cpp` are temporary and will be removed in a future version. Refactoring suggestions for these functions should be avoided.
Applied to files:
src/evo/deterministicmns.cpp
🪛 GitHub Actions: Clang Diff Format Check
src/llmq/blockprocessor.cpp
[error] 1-1: Clang-format differences detected in src/llmq/blockprocessor.cpp.
src/evo/evodb.h
[error] 1-1: Clang-format differences detected in src/evo/evodb.h.
src/llmq/quorums.cpp
[error] 1-1: Clang-format differences detected in src/llmq/quorums.cpp.
src/dsnotificationinterface.cpp
[error] 1-1: Clang-format differences detected in src/dsnotificationinterface.cpp.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (107)
src/masternode/active/notificationinterface.cpp (1)
19-20: LGTM! Proper destructor pattern.Defaulting the destructor in the implementation file is the correct approach for a class with a virtual destructor and potential incomplete types in the header.
src/masternode/active/notificationinterface.h (2)
23-23: Good addition - enforces non-copyable semantics.Explicitly deleting the copy assignment operator is consistent with the deleted copy constructor and aligns with the PR's goal of enforcing stricter ownership semantics.
25-25: Proper virtual destructor declaration.The virtual destructor is appropriate for a class inheriting from
CValidationInterface, and defining it in the .cpp file (as seen in the implementation) allows for better encapsulation.src/evo/smldiff.cpp (2)
128-141: LGTM: Explicit parameter naming improves readability.The addition of
/*onlyValid=*/falsemakes the intent of the boolean parameter clear at the call site. This aligns with the broader refactoring to standardize callback invocations across the codebase.
143-148: LGTM: Consistent parameter naming.The explicit
/*onlyValid=*/falseparameter annotation is consistent with the previous call and improves code readability.src/masternode/sync.cpp (2)
17-23: LGTM: Modern initialization style.The switch from parentheses
()to brace initialization{}for member variables is a good modernization that prevents narrowing conversions and improves consistency with modern C++ practices.
25-26: LGTM: Explicit destructor declaration.Adding an explicit defaulted destructor improves code clarity and aligns with the broader refactoring effort to make object lifecycle management more explicit.
contrib/containers/ci/ci-slim.Dockerfile (1)
108-114: LGTM: Prevents group conflict errors.The addition of
getent group ${GROUP_ID} ||preventsgroupmodfrom failing when a group with the target GID already exists. This is a necessary safeguard for cross-platform compatibility, particularly when dealing with macOS vs. Linux group ID conflicts (e.g., staff vs. dialout).src/netfulfilledman.cpp (1)
15-20: LGTM: Improved code organization.Relocating the destructor implementation to immediately follow the constructor improves code readability and organization. The destructor logic remains unchanged.
src/llmq/debug.cpp (1)
112-113: LGTM: Explicit destructor declaration.Adding an explicit defaulted destructor for
CDKGDebugManagerimproves code clarity and is consistent with the broader refactoring effort to make object lifecycle management more explicit across the codebase.src/chainlock/signing.h (1)
66-68: LGTM: Enforces non-copyable, non-default-constructible semantics.Explicitly deleting the default constructor, copy constructor, and copy assignment operator correctly enforces that
ChainLockSignermust be constructed with its required dependencies and cannot be accidentally copied. This is appropriate for a class managing resources and state.src/llmq/utils.cpp (2)
147-159: LGTM: Explicit parameter naming and modern lambda style.The addition of
/*onlyValid=*/truemakes the boolean parameter intent clear. Usingautofor the lambda parameter type is more flexible and aligns with modern C++ practices.
490-496: LGTM: Consistent improvements.The explicit
/*onlyValid=*/falseparameter andautolambda parameter type are consistent with the changes at line 147 and the broader refactoring effort across the codebase.contrib/seeds/makeseeds.py (1)
209-209: Change is correct and verified against standardized protx RPC format.The change from
mn['state']['addresses'][0]tomn['state']['addresses']['core_p2p'][0]correctly adapts to the standardizedprotx list validRPC output format. Existing test files confirm this exact structure is the expected format:protx_info['state']['addresses']['core_p2p'][0]is used in multiple functional tests (feature_dip3_deterministicmns.py, rpc_quorum.py).src/instantsend/signing.h (1)
72-74: LGTM! Enforces non-copyable semantics.The deleted constructors and assignment operator correctly prevent default construction and copying of
InstantSendSignerinstances, enforcing unique ownership. This aligns with the PR-wide pattern of stricter ownership semantics.src/llmq/blockprocessor.h (1)
58-60: LGTM! Consistent ownership restrictions.The deleted constructors prevent default construction and copying of
CQuorumBlockProcessor, matching the ownership pattern applied consistently across the PR.src/evo/chainhelper.h (1)
39-41: LGTM! Enforces unique ownership.The deleted special member functions prevent unintended copying and default construction of
CChainstateHelper, consistent with the project-wide refactoring pattern.src/evo/mnhftx.h (1)
102-104: LGTM! Non-copyable semantics enforced.The deleted constructors prevent copying and default construction of
CMNHFManager, maintaining consistency with the ownership model applied throughout this PR.src/chainlock/chainlock.h (1)
73-75: LGTM! Consistent with ownership pattern.The deleted special members prevent copying and default construction of
CChainLocksHandler, aligning with the broader refactoring to enforce unique ownership semantics.src/evo/creditpool.h (1)
126-130: LGTM! Ownership restrictions and destructor externalization.The deleted constructors prevent copying and default construction. The destructor is moved from inline defaulted to a declaration, with the implementation provided in the source file (verified in
src/evo/creditpool.cpp:246). This pattern allows for proper separation of interface and implementation.src/evo/creditpool.cpp (1)
241-246: LGTM! Destructor implementation and uniform initialization.The constructor uses brace initialization (a stylistic improvement), and the defaulted destructor implementation matches the declaration in the header. No behavioral changes.
src/dbwrapper.h (1)
712-741: LGTM! Well-designed parameter struct and factory.The
DbWrapperParamsstruct andMakeDbWrapperfactory provide a clean, standardized approach to database initialization:
- Deleted default constructor enforces explicit configuration
- Const members ensure immutability after construction
- Two constructors provide flexibility: explicit cache size or default (1 MiB)
- Factory correctly handles memory-only databases by passing empty path
The default
cache_sizeof 1 MiB should be sufficient for most components, as larger caches are explicitly specified via the first constructor.src/bls/bls.h (1)
597-597: Verification complete: BLSInit() API change is safe.Both call sites (src/test/util/setup_common.cpp:187 and src/init/common.cpp:36) treat BLSInit() as plain function calls. No conditional checks or value assignments detected.
src/rpc/blockchain.cpp (1)
1465-1492: Let me verify the getbestchainlock mismatch mentioned in the scratchpad and check if there are any other concerns:Based on the verification results:
No issues found—softforks schema implementation and tests are aligned.
The OBJ_DYN layout and fields in the code (type, bip9, statistics, height, active, EHF fields) match the schema validated in
test/functional/rpc_blockchain.py(lines 204–247). Functional tests already reference the new structure correctly through helper functions likeget_bip9_details()andsoftfork_active(). No legacy assumptions or conflicts detected.src/masternode/meta.cpp (1)
18-22: Destructor relocation looks goodNo behavior change; persists cache only when valid. LGTM.
src/rpc/governance.cpp (1)
569-575: Const-correct ForEachMN usageExplicit onlyValid flag and const auto& tighten intent. LGTM.
src/qt/masternodelist.cpp (1)
179-185: UI list iteration const-ified; explicit onlyValid flagRead-only lambdas align with const correctness; clearer intent. LGTM.
Also applies to: 211-310
src/test/validation_chainstatemanager_tests.cpp (1)
54-55: All call sites verified as using the new signatureThe two test file calls at lines 54 and 87 both correctly use the updated
DashChainstateSetupsignature withllmq_dbs_in_memoryandllmq_dbs_wipeparameters. No remaining call sites with the old signature were found in the codebase.src/test/evo_deterministicmns_tests.cpp (1)
220-220: LGTM: Improved readability with explicit parameter naming.The use of
/*onlyValid=*/truemakes the intent clear at the call site, and theconst auto&lambda parameter is appropriately deduced.src/rpc/evo.cpp (1)
1469-1469: LGTM: Consistent with project-wide refactoring.The explicit parameter naming improves code clarity. The change aligns with the broader effort to standardize
ForEachMN/ForEachMNSharedusage across the codebase.src/llmq/blockprocessor.cpp (2)
50-53: Formatting: Clang-format differences detected.The brace initialization change is good, but please run clang-format to resolve the formatting differences flagged by CI.
73-76: LGTM: Proper cleanup in destructor.Explicitly calling
StopWorkerThreads()in the destructor ensures proper resource cleanup.src/llmq/snapshot.cpp (1)
339-345: LGTM: Clean constructor and destructor implementation.The brace initialization and defaulted destructor follow modern C++ best practices. The explicit constructor properly initializes the member variables.
src/dsnotificationinterface.cpp (2)
30-36: Formatting: Clang-format differences detected.The brace initialization change is good and consistent with the broader refactoring, but please run clang-format to resolve the formatting differences flagged by CI.
40-40: LGTM: Proper use of defaulted destructor.The defaulted destructor is appropriate when no custom cleanup is required.
src/evo/mnhftx.cpp (1)
48-48: LGTM: Consistent brace initialization.The change to brace initialization aligns with modern C++ practices and is consistent with the broader refactoring effort.
src/bls/bls.cpp (1)
427-432: LGTM: BLSInit() return type change is intentional.Changing from
booltovoidis appropriate since the function never reported errors. This aligns with the PR objective to move BLSInit() out of SanityChecks() since it doesn't provide error state.Based on learnings
src/masternode/node.cpp (2)
52-56: LGTM: Clean brace initialization.The brace initialization of
m_info{sk, sk.GetPublicKey()}is clear and follows modern C++ practices. The change is consistent with the broader refactoring effort.
64-64: LGTM: Appropriate use of defaulted destructor.The defaulted destructor is suitable here as no custom cleanup logic is required.
src/evo/specialtxman.cpp (2)
192-205: LGTM: Improved const-correctness and clarity.The explicit named parameter
/*onlyValid=*/falseand const lambda parameter improve code readability and prevent unintended modifications during iteration.
476-487: LGTM: Consistent iteration pattern.The changes here mirror the improvements at line 192, maintaining consistency across the codebase.
src/rpc/masternode.cpp (1)
581-708: LGTM: Explicit parameter naming improves clarity.Adding the explicit
/*onlyValid=*/comment makes the boolean parameter's purpose immediately clear to readers.test/functional/wallet_signrawtransactionwithwallet.py (2)
127-134: LGTM: Test correctly updated for early softfork activation.The assertion correctly verifies CSV is already active without needing additional block generation, aligning with the updated activation schedule mentioned in the PR.
157-167: LGTM: Aligned with upstream behavior.The changes correctly:
- Assert CLTV (bip65) is already active without additional blocks
- Reduce the locktime from 1000 to 100 to match upstream
As noted in the PR description, this aligns with upstream behavior after softfork activation adjustments.
src/init/common.cpp (1)
29-37: LGTM: Appropriate separation of concerns.Moving
BLSInit()fromSanityChecks()toSetGlobals()is correct since BLS initialization is setup rather than validation. The placement alongsideRandomInit()andECC_Start()is logical.src/test/util/setup_common.h (1)
87-93: LGTM: Clear parameter naming for test database configuration.The renamed parameters
llmq_dbs_in_memoryandllmq_dbs_wipeclearly express their purpose for controlling test database behavior, supporting the PR's goal of restoring memory-only databases for improved test performance.src/masternode/meta.h (1)
237-240: LGTM: Enforces appropriate ownership semantics.Deleting copy operations for
CMasternodeMetaManis correct given its unique ownership of the database handle. This prevents accidental copies and makes ownership explicit.src/bls/bls_worker.h (1)
52-55: LGTM: Correct resource management for thread pool.Making
CBLSWorkernon-copyable is essential since it owns a thread pool. The explicit constructor prevents unintended implicit conversions.src/coinjoin/coinjoin.cpp (2)
125-127: LGTM: Explicit special member declarations.Explicitly declaring defaulted constructors and destructors is good practice for manager classes, making the interface clear and facilitating future modifications if needed.
401-403: LGTM: Consistent with manager class pattern.The explicit defaulted special members for
CDSTXManagerfollow the same pattern asCCoinJoinBaseManagerabove, maintaining consistency.src/governance/governance.cpp (2)
79-84: Brace-init in member initializer list: LGTM.
Consistent with modern style; no functional change.
621-623: Verify whetherGetCurrentVotesshould include votes from banned/invalid masternodes.The RPC endpoint
gobject getcurrentvotesdocuments "Get only current (tallying) votes," suggesting votes should be from currently valid masternodes. However, the implementation usesonlyValid=falsewithout explanation, allowing votes from invalid masternodes to be included. Either setonlyValid=trueto match the documented semantics, or add a comment explaining why banned masternodes must be included.src/evo/deterministicmns.cpp (4)
193-202: onlyValid=true in GetMNPayee pre-scan: good.
Scopes to enabled MNs while checking Evo consecutive payouts.
209-214: Final selection over valid MNs: good.
Const-correct auto capture with shared ptrs is fine.
235-247: Projected payees: pre-fill Evo streak logic reads well.
No correctness issues spotted.
637-644: Add explicit ctor/dtor for CDeterministicMNManager: LGTM.
Solidify ownership; no side effects.src/spork.h (1)
236-239: Make CSporkManager non-copyable + explicit ctor: LGTM.
Aligns with move-only/ownership goals from prior refactors.
Based on learningssrc/netfulfilledman.h (1)
53-55: Enforce non-copyable CNetFulfilledRequestManager: LGTM.
Matches manager ownership patterns elsewhere.src/instantsend/db.h (2)
27-30: Forward declaration properly applied: util::DbWrapperParams is correctly forward-declared in db.h and instantsend.h.Verification confirms:
- db.cpp includes
<dbwrapper.h>(line 8) before using DbWrapperParams in the constructor and Upgrade method- instantsend.cpp passes db_params by const reference to the db member, which only requires a forward declaration—no full definition needed
- The actual parameter usage occurs in CInstantSendDb::CInstantSendDb (in db.cpp), which has the defining header included
84-84: All call sites properly updated; memory-only DB support verified.The refactor is complete and correct. CInstantSendDb constructor and Upgrade method both accept DbWrapperParams. The single instantiation site (llmq/context.cpp:35-36) properly threads db_params through CInstantSendManager to CInstantSendDb. No bool-based constructor calls remain in the codebase. Test infrastructure confirms memory-only DB capability (memory=true flag in DbWrapperParams).
src/init.cpp (3)
2358-2365: Const-ref lambda in ForEachMN avoids copies — LGTM.Clear
/*onlyValid=*/falsecomment improves readability; no functional risk.
1970-2006: Parameter order and call sites verified — LGTM.Both LoadChainstate call sites (src/init.cpp and src/test/util/setup_common.cpp) have been correctly updated with the new
data_dirparameter in position 15, maintaining consistent parameter order with the declaration. Thedash_dbs_in_memoryflag is properly wired in both locations.
1996-2000: Verification confirmed: Tests properly set dash_dbs_in_memory=true.Tests in
src/test/util/setup_common.cpp:335callLoadChainstatewithdash_dbs_in_memory=true, while production insrc/init.cpp:1999correctly usesdash_dbs_in_memory=false. The wiring is explicit and properly differentiated between test and production contexts.src/llmq/quorums.h (2)
41-43: Forward-declare util::DbWrapperParams — LGTM.Reduces header coupling and keeps includes light.
265-273: Implementation correctly wires DbWrapperParams and handles migration.Verified:
dbcreated at line 215–216 withdb_params(path, memory, wipe flags)MigrateOldQuorumDB(_evoDb)called at line 229 for data migration from old DB- All prior test/wipe flags mapped into
db_paramsstructure (no hardcoded legacy flags)- Pattern consistent across related managers (CEvoDB, CDKGSessionManager, CRecoveredSigsDb, CInstantSendDb)
src/governance/governance.h (1)
276-279: Governance manager: delete default/copy/assign — verified safe.Constructor pattern is correctly enforced across all instantiation sites. Two verified creation points both use explicit parameter passing via
std::make_unique, with no default construction or copy operations detected.src/coinjoin/server.h (1)
97-105: Destructor already defaulted out-of-line—pattern is correctThe destructor is already properly defined as
= default;in src/coinjoin/server.cpp (line 45). The current out-of-line approach is valid and consistent with the non-copyable manager pattern. The suggestion to inline it is stylistic; no changes required.src/masternode/node.h (1)
54-58: The destructor definition already exists out-of-line in node.cpp and is properly defaulted.The script confirms that
CActiveMasternodeManager::~CActiveMasternodeManager() = default;is defined insrc/masternode/node.cppat line 64. This is a valid, idiomatic C++ pattern—the declaration in the header stays as-is, and the out-of-line defaulted definition in the .cpp file provides ODR compliance and proper teardown semantics. No changes are needed.Likely an incorrect or invalid review comment.
src/llmq/snapshot.h (1)
226-231: Destructor is already properly defined as= default;in the implementation file—no changes needed.The destructor declaration in the header (
~CQuorumSnapshotManager();) paired with the definition insrc/llmq/snapshot.cpp:345(~CQuorumSnapshotManager() = default;) is a valid C++ pattern. There is no "missing symbol risk" since the destructor is already properly defined. The review comment's concern is not applicable here; the code is already correct as-is.Likely an incorrect or invalid review comment.
src/masternode/active/context.h (1)
45-49: Explicit construction + no copy/assign: LGTMAll const unique_ptr members (cj_server, gov_signer, shareman, ehf_sighandler, cl_signer, is_signer) and the const reference member (m_llmq_ctx) are properly initialized in the ctor's initializer list using std::make_unique and direct initialization. The constructor signature correctly matches the LLMQContext& parameter to the m_llmq_ctx member. Ownership semantics are sound.
src/node/chainstate.h (2)
31-33: ****The forward declaration of
namespace fsin chainstate.h is valid and creates no conflict. The review comment incorrectly states that fs is an alias likenamespace fs = std::filesystem;. In reality,fsis a namespace (defined in src/fs.h withusing namespace std::filesystem;). Forward-declaring a namespace is a standard C++ pattern that allows usingfs::pathin function signatures without including the full fs.h header. Multiple namespace declarations with the same name legally coexist and merge during compilation.Likely an incorrect or invalid review comment.
129-132: No issues found—wiring verifiedAll parameters are correctly threaded from DashChainstateSetup through LLMQContext to individual LLMQ managers (CQuorumManager, CSigningManager, etc.), and fReset/fReindex logic is preserved.
src/llmq/context.cpp (2)
21-37: Check member declaration order matches dependency usage in the initializer.qdkgsman depends on quorum_block_processor/qsnapman/bls_worker; qman depends on qdkgsman/qsnapman; sigman depends on qman; isman depends on clhandler/sigman/qman. Please confirm src/llmq/context.h declares these members in the same order to avoid unintended initialization-order bugs. If not, reorder declarations, not just the init list.
33-37: Good: unified db_params propagation into LLMQ managers.Passing util::DbWrapperParams through CSigningManager and CInstantSendManager aligns with move-only/ownership refactors and simplifies test configuration.
src/instantsend/instantsend.cpp (1)
61-63: LGTM: db initialization now driven by db_params.Matches the broader refactor and restores memory-only DB control via a single parameter object.
src/instantsend/instantsend.h (2)
35-36: Good forward declaration to avoid heavy includes.
95-101: Move-only and explicit construction looks good.Deleting default/copy/assign plus the db_params-based ctor aligns with ownership semantics.
src/evo/evodb.h (3)
12-15: Good: minimal dependency via forward declaration.Reduces header coupling; keeps build times down.
53-58: Non-copyable CEvoDB is appropriate.Deleting default/copy/assign matches ownership semantics; destructor presence suppresses implicit moves as well. LGTM.
1-1: No formatting changes required—file is already properly formatted and committed.The working tree is clean with no pending changes to this file. The copyright header and code formatting already match the codebase style, indicating the formatting issue has been resolved.
src/instantsend/db.cpp (1)
29-33: Constructor migration to DbWrapperParams looks good.Path scoping under data_dir/llmq/isdb and honoring memory/wipe flags are clear.
src/llmq/signing.h (3)
35-38: Good: forward declare DbWrapperParams to limit header dependencies.Keeps this header lightweight.
130-130: Constructor switch to DbWrapperParams is consistent.Keeps DB config centralized; ensure internal db is always initialized.
186-192: Signing manager ownership and ctor changes confirmed correct.Verification shows the explicit ctor with db_params is properly used at the single instantiation point in
src/llmq/context.cpp:33, whereCSigningManageris constructed viamake_uniquewith all three required parameters includingdb_params. The non-copyable interface is clean and enforced through deleted copy operations.src/node/chainstate.cpp (4)
77-78: CEvoDB now uses DbWrapperParams—LGTM.Wires data_dir and in-memory/wipe flags cleanly.
93-95: DashChainstateSetup: forwarding db params is correct.Using same wipe condition as reindex/reset is consistent.
233-236: Migration verified—all LLMQContext constructor calls properly updated.Verification confirms the (bool,bool) constructor signature has been removed entirely, and the single instantiation site at line 250–252 correctly wraps
llmq_dbs_in_memoryandllmq_dbs_wipeintoutil::DbWrapperParams. No legacy constructor patterns remain in the codebase.
51-51: API surface change verified: all LoadChainstate callers updated.Both call sites correctly pass the new
data_diranddash_dbs_in_memoryparameters:
src/init.cpp:1970passesargs.GetDataDirNet()anddash_dbs_in_memory=falsesrc/test/util/setup_common.cpp:306passesAssert(m_node.args)->GetDataDirNet()anddash_dbs_in_memory=truesrc/llmq/context.h (3)
31-35: Forward declare DbWrapperParams—good header hygiene.Prevents pulling db headers transitively.
40-40: Delete copy assignment to enforce unique ownership.Matches the design intent spelled out in comments.
41-45: Constructor migration to DbWrapperParams verified complete—all uses updated.Verification confirms the constructor signature in context.h (lines 41-44) is correctly aligned with its only instantiation at src/node/chainstate.cpp:250-252, which properly passes the new
util::DbWrapperParamsparameter. No old-style signatures remain in the codebase. The migration is comprehensive and ready.src/coinjoin/server.cpp (1)
26-46: Constructor-based DI looks solidExplicit dependency injection and defaulted dtor are clear and consistent with the PR direction.
src/evo/deterministicmns.h (1)
655-660: Lifecycle restrictions look goodDeleting default/copy/assign and adding an explicit ctor tightens ownership; aligns with PR goals.
src/llmq/dkgsessionmgr.cpp (2)
36-45: Good move to util::DbWrapperParams + MakeDbWrapperDB construction is centralized and consistent; pathing under "llmq/dkgdb" is clear.
298-345: Contributions fetch path is careful with locking and cachingCorrect lock scoping; reserves vectors up‑front; reads vvec + sk with clear cache key.
src/llmq/quorums.cpp (4)
210-231: Constructor refactor to DbWrapperParams is cleanFactory usage and subpath "llmq/quorumdb" look correct; migration call preserved.
232-235: Destructor calls Stop()Explicit stop in dtor prevents background work after teardown. Good.
686-701: Callsite update to new ForEachMN signature is correctConst‑ref lambda and onlyValid=true match the updated API.
1-1: Fix clang-format discrepancies in this fileCI reports formatting diffs. Please run clang-format with the repo’s style (e.g., script or: clang-format -i src/llmq/quorums.cpp) and re‑push.
src/llmq/dkgsessionmgr.h (2)
36-39: Forward declaration keeps header lightGood choice to avoid pulling dbwrapper.h into the public header.
84-92: Constructor signature change aligns with db_params refactorExplicit ctor + deleted copy/assign are appropriate.
src/test/util/setup_common.cpp (3)
128-136: DashChainstateSetup wrapper param forwarding looks correctPassing m_node.args->GetDataDirNet() plus llmq_dbs_in_memory/llmq_dbs_wipe is consistent with memory-only DB semantics. No issues.
219-225: EvoDB in-memory/wipe defaults: good alignment with test goalsConstructing CEvoDB with util::DbWrapperParams{..., /memory=/true, /wipe=/true} is appropriate for tests and speeds them up. LGTM.
320-336: LLMQ DB in-memory flags are correctly governed by DashChainstateSetupThe
dash_dbs_in_memoryparameter passed to LoadChainstate is properly propagated to control LLMQ database behavior. At chainstate.cpp:93–94, LoadChainstate passesdash_dbs_in_memoryas thellmq_dbs_in_memoryparameter to DashChainstateSetup, which then passes it to LLMQContext via util::DbWrapperParams. All LLMQ databases (dkgsessionmgr, signing, quorums, instantsend) use this parameter consistently. Wipe behavior is also unified: both evodb and LLMQ DBs use the samefReset || fReindexChainStatelogic.src/coinjoin/client.cpp (2)
41-50: Queue manager ctor is straightforwardDependency wiring via references looks good. No issues here.
153-159: Queue DoMaintenance minimal guard is fineEarly exits on sync/shutdown and calling CheckQueue() are reasonable. LGTM.
src/coinjoin/client.h (1)
228-234: Queue manager API deletions/ctors look goodDeleting default/copy/assign and adding explicit ctor/dtor is appropriate for ownership semantics. LGTM.
| CCoinJoinClientManager() = delete; | ||
| CCoinJoinClientManager(CCoinJoinClientManager const&) = delete; | ||
| CCoinJoinClientManager& operator=(CCoinJoinClientManager const&) = delete; | ||
|
|
||
| CCoinJoinClientManager(const CCoinJoinClientManager&) = delete; | ||
| CCoinJoinClientManager& operator=(const CCoinJoinClientManager&) = delete; | ||
| explicit 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) : | ||
| m_wallet(wallet), | ||
| m_dmnman(dmnman), | ||
| m_mn_metaman(mn_metaman), | ||
| m_mn_sync(mn_sync), | ||
| m_isman{isman}, | ||
| m_queueman(queueman) | ||
| { | ||
| } | ||
| const std::unique_ptr<CCoinJoinClientQueueManager>& queueman); | ||
| ~CCoinJoinClientManager(); | ||
|
|
There was a problem hiding this comment.
Make cached height fields atomic to avoid data races
These are updated/read from different threads without a shared lock. Make them std::atomic.
- int nCachedLastSuccessBlock{0};
+ std::atomic<int> nCachedLastSuccessBlock{0};
@@
- int nCachedBlockHeight{0};
+ std::atomic<int> nCachedBlockHeight{0};Pair with .cpp changes to use load()/store().
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/coinjoin/client.h around lines 278 to 286, change the cached height
member fields that are accessed from multiple threads to std::atomic<int> (add
#include <atomic> if not present) so reads/writes are atomic; then update the
corresponding .cpp to replace direct reads/writes with .load() and .store()
calls (or appropriate memory order if needed) wherever those cached height
fields are accessed, ensuring they are properly initialized and used without
surrounding shared locks.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR consolidates miscellaneous fixes and refactorings, including standardizing manager class constructors/destructors, restoring memory-only database support for EvoDb and LLMQContext, and correcting documentation inconsistencies from previous backports. Key architectural changes include replacing boolean constructor parameters with a structured util::DbWrapperParams for database initialization across multiple manager classes, enabling unit tests to run ~25% faster (100.5s to 75s). Additional fixes include correcting makeseeds.py to work with the new extended addresses structure, moving BLSInit() from sanity checks to global initialization, standardizing lambda signatures in ForEachMN* calls to use const auto&, and fixing Docker container compatibility for macOS users. These changes prepare the codebase for future refactoring while maintaining backward compatibility in production code.
Important Files Changed
| Filename | Score | Overview |
|---|---|---|
src/dbwrapper.h |
4/5 | Adds util::DbWrapperParams struct and MakeDbWrapper() factory function to standardize database wrapper construction and enable memory-only mode |
src/evo/evodb.h |
4/5 | Refactors CEvoDB to use std::unique_ptr<CDBWrapper> and accept DbWrapperParams for memory-only database support |
src/evo/evodb.cpp |
4/5 | Implements constructor to create database via MakeDbWrapper() and updates all db references to db-> for pointer dereference |
src/llmq/context.h |
4/5 | Changes constructor signature to accept DbWrapperParams instead of boolean flags for LLMQ database configuration |
src/llmq/context.cpp |
5/5 | Propagates DbWrapperParams to all LLMQ manager constructors for consistent memory-only database support |
src/node/chainstate.h |
5/5 | Adds data_dir and dash_dbs_in_memory parameters to support memory-only databases for Dash components |
src/node/chainstate.cpp |
5/5 | Implements memory-only database initialization for EvoDb and LLMQContext in chainstate setup |
src/test/util/setup_common.cpp |
5/5 | Moves Dash database initialization to BasicTestingSetup, enables memory-only mode for tests, and fixes teardown order |
src/instantsend/db.cpp |
2/5 | Updates constructor to use DbWrapperParams but hardcodes wipe=true in Upgrade(), potentially causing data loss |
src/evo/deterministicmns.h |
5/5 | Replaces templated ForEachMN* callbacks with explicit std::function signatures for type safety |
src/llmq/quorums.h |
5/5 | Changes constructor to accept DbWrapperParams and deletes copy/default constructors for CQuorumManager |
src/llmq/signing.h |
5/5 | Refactors constructors to use DbWrapperParams for CRecoveredSigsDb and CSigningManager |
src/instantsend/instantsend.h |
3/5 | Updates constructor signature to accept DbWrapperParams for flexible database configuration |
src/bls/bls.h |
5/5 | Changes BLSInit() return type from bool to void to correctly reflect no error state |
src/init/common.cpp |
5/5 | Moves BLSInit() from SanityChecks() to SetGlobals() for proper initialization ordering |
src/rpc/blockchain.cpp |
5/5 | Fixes softforks help text structure to correctly document dynamic object with softfork names as keys |
test/functional/wallet_signrawtransactionwithwallet.py |
5/5 | Removes unnecessary block generation, adds softfork activation assertions, adjusts locktime to 100 |
contrib/seeds/makeseeds.py |
5/5 | Updates to access Core P2P addresses from new extended addresses structure (addresses['core_p2p'][0]) |
contrib/containers/ci/ci-slim.Dockerfile |
5/5 | Conditionally skips group creation if GID exists to support macOS users with staff group GID20 |
| All manager class headers/implementations | 5/5 | Standardizes constructor/destructor patterns by deleting default/copy constructors and using explicit initialization |
Confidence score: 3/5
- This PR contains multiple unrelated changes bundled together which increases review complexity, though most individual changes are straightforward refactorings
- Score reduced due to a critical issue in
src/instantsend/db.cppwhereUpgrade()hardcodeswipe=true, potentially causing data loss in production, and concerns about the extensive signature changes to database initialization patterns that could have subtle initialization-order dependencies - Pay close attention to
src/instantsend/db.cpp(potential data loss), database initialization order acrosssrc/node/chainstate.cpp,src/evo/evodb.cpp, and all LLMQ context files, and verify that the memory-only database flag propagation is correct in all test fixtures
Context used:
- Context from
dashboard- CLAUDE.md (source)
78 files reviewed, 4 comments
src/dbwrapper.h
Outdated
| DbWrapperParams(const fs::path path, size_t cache_size, bool memory, bool wipe) : | ||
| path{path}, | ||
| cache_size{cache_size}, | ||
| memory{memory}, | ||
| wipe{wipe} | ||
| { | ||
| } |
There was a problem hiding this comment.
style: Constructor takes parameters by value and initializes const member variables directly with them, resulting in unnecessary copies. Consider const fs::path& for path parameter.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/dbwrapper.h
Line: 716:722
Comment:
**style:** Constructor takes parameters by value and initializes const member variables directly with them, resulting in unnecessary copies. Consider `const fs::path&` for `path` parameter.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/evo/deterministicmns.cpp (1)
1454-1472: Critical: migration can drop unconverted diffs (partial conversion + blanket erase)MigrateLegacyDiffs stops on first missing/invalid key (break), then later erases all remaining DB_LIST_DIFF_LEGACY entries. If any heights weren’t converted (e.g., prior partial migration or sparse legacy keys), this will silently delete data and make historical reconstruction impossible. Read failures also fall through to the erase phase.
Minimal safe fix (fail hard and avoid erasing on partial conversion):
@@ - for (auto current_height : irange::range(start_height, tip_index->nHeight + 1)) { + bool conversion_ok = true; + for (auto current_height : irange::range(start_height, tip_index->nHeight + 1)) { @@ - if (!pcursor->Valid() || !pcursor->GetKey(key) || std::get<0>(key) != DB_LIST_DIFF_LEGACY) { - break; + if (!pcursor->Valid() || !pcursor->GetKey(key) || std::get<0>(key) != DB_LIST_DIFF_LEGACY) { + LogPrintf("CDeterministicMNManager::%s -- Missing legacy diff at height=%d; aborting migration\n", __func__, current_height); + conversion_ok = false; + break; } @@ - if (!m_evoDb.GetRawDB().ReadDataStream(key, s)) { - break; + if (!m_evoDb.GetRawDB().ReadDataStream(key, s)) { + LogPrintf("CDeterministicMNManager::%s -- Failed to read legacy diff at height=%d; aborting migration\n", __func__, current_height); + conversion_ok = false; + break; } @@ - LogPrintf("CDeterministicMNManager::%s -- Writing new diffs, height=%d...\n", __func__, tip_index->nHeight); + if (!conversion_ok) { + // Do NOT erase legacy entries; leave DB untouched for a retry or manual intervention. + return false; + } + LogPrintf("CDeterministicMNManager::%s -- Writing new diffs, height=%d...\n", __func__, tip_index->nHeight); @@ - // Delete all dangling legacy format entries + // Delete all dangling legacy format entries (only after full successful conversion)Recommended robust approach (follow-up): iterate all DB_LIST_DIFF_LEGACY keys with a single cursor walk (Seek(start); Next()), collect (height, hash) pairs by looking up CBlockIndex for ordering, convert all, then erase—no height loop assumptions. I can draft this if you want.
Also applies to: 1540-1561
src/evo/evodb.h (1)
1-123: Fix clang-format violations.The pipeline indicates formatting issues in this file. Please run clang-format to ensure consistent code style.
Run the following command to fix formatting:
#!/bin/bash # Apply clang-format to the file git diff -U0 --no-color HEAD^ | clang-format-diff -p1 -isrc/llmq/quorums.cpp (1)
1-1: Run clang-format to fix formatting issues.The CI pipeline detected clang-format differences. Please run
clang-format-diff.py -p1or update the formatting to match the project style.#!/bin/bash # Run clang-format on the modified file clang-format -i src/llmq/quorums.cpp
🧹 Nitpick comments (6)
src/coinjoin/client.cpp (1)
1901-1921: Constructor properly initializes all dependencies.The member initialization list correctly captures all required services. Note that the explicit
.reset()calls in the destructor (lines 1918-1920) are unnecessary sinceunique_ptrmembers are automatically destroyed, but this doesn't cause any issues.If you prefer a simpler destructor, you can rely on automatic cleanup:
CoinJoinWalletManager::~CoinJoinWalletManager() { - LOCK(cs_wallet_manager_map); - for (auto& [wallet_name, cj_man] : m_wallet_manager_map) { - cj_man.reset(); - } }src/test/evo_deterministicmns_tests.cpp (1)
46-73: Nit: rename parameter ‘utoxs’ to ‘utxos’ for clarityJust a typo/readability fix; no functional change.
-static std::vector<COutPoint> SelectUTXOs(const CChain& active_chain, SimpleUTXOMap& utoxs, CAmount amount, CAmount& changeRet) +static std::vector<COutPoint> SelectUTXOs(const CChain& active_chain, SimpleUTXOMap& utxos, CAmount amount, CAmount& changeRet) { ... - while (!utoxs.empty()) { + while (!utxos.empty()) { ... - for (auto it = utoxs.begin(); it != utoxs.end(); ++it) { + for (auto it = utxos.begin(); it != utxos.end(); ++it) { ... - utoxs.erase(it); + utxos.erase(it);src/evo/deterministicmns.cpp (2)
343-345: Rename loop variable for clarityproTxHash holds a CDeterministicMNCPtr, not a hash; rename to dmnPtr for readability.
- for (const auto& proTxHash : toDecrease) { - PoSeDecrease(*proTxHash); + for (const auto& dmnPtr : toDecrease) { + PoSeDecrease(*dmnPtr);
118-124: Consider explicit std::ranges::find_if with#include <algorithm>for clarity; current code works via transitive include.The code currently works because
src/evo/dmnstate.h→src/bls/bls.h→src/util/ranges.h(which definesnamespace ranges { using namespace std::ranges; }). However, explicit includes improve readability and reduce reliance on transitive chains, making the code's dependencies clearer:#include <util/pointer.h> +#include <algorithm> ... - const auto it = ranges::find_if(mnMap, + const auto it = std::ranges::find_if(mnMap, [&pubKey](const auto& p){return p.second->pdmnState->pubKeyOperator.Get() == pubKey;});Note: This follows established project patterns (e.g.,
src/coinjoin/client.h,src/llmq/options.cpp) that useranges::via the same transitive chain, so this change is optional for consistency but not required for correctness.src/evo/evodb.cpp (1)
35-40: Don’t hard‑code 64 MiB; honor DbWrapperParams.cache_size.This reduces configurability and diverges from the new params pattern.
Apply:
-CEvoDB::CEvoDB(const util::DbWrapperParams& db_params) : - db{util::MakeDbWrapper({db_params.path / "evodb", /*cache_size=*/64 << 20, db_params.memory, db_params.wipe})}, +CEvoDB::CEvoDB(const util::DbWrapperParams& db_params) : + db{util::MakeDbWrapper(util::DbWrapperParams{ + db_params.path / "evodb", + db_params.cache_size, + db_params.memory, + db_params.wipe + })},src/dbwrapper.h (1)
713-735: Prefer explicit constructors to avoid accidental implicit conversions.Mark DbWrapperParams constructors explicit.
-struct DbWrapperParams +struct DbWrapperParams { DbWrapperParams() = delete; - DbWrapperParams(const fs::path path, size_t cache_size, bool memory, bool wipe) : + explicit DbWrapperParams(const fs::path path, size_t cache_size, bool memory, bool wipe) : path{path}, cache_size{cache_size}, memory{memory}, wipe{wipe} { } - DbWrapperParams(const fs::path path, bool memory, bool wipe) : + explicit DbWrapperParams(const fs::path path, bool memory, bool wipe) : path{path}, memory{memory}, wipe{wipe} { }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
contrib/containers/ci/ci-slim.Dockerfile(1 hunks)contrib/seeds/makeseeds.py(1 hunks)src/bls/bls.cpp(1 hunks)src/bls/bls.h(1 hunks)src/coinjoin/client.cpp(4 hunks)src/dbwrapper.h(1 hunks)src/dsnotificationinterface.cpp(1 hunks)src/dsnotificationinterface.h(1 hunks)src/evo/deterministicmns.cpp(7 hunks)src/evo/deterministicmns.h(3 hunks)src/evo/evodb.cpp(2 hunks)src/evo/evodb.h(5 hunks)src/evo/smldiff.cpp(2 hunks)src/evo/specialtxman.cpp(2 hunks)src/governance/governance.cpp(2 hunks)src/init.cpp(3 hunks)src/init/common.cpp(1 hunks)src/instantsend/db.cpp(1 hunks)src/instantsend/db.h(2 hunks)src/instantsend/instantsend.cpp(1 hunks)src/instantsend/instantsend.h(2 hunks)src/llmq/context.cpp(1 hunks)src/llmq/context.h(1 hunks)src/llmq/dkgsessionmgr.cpp(1 hunks)src/llmq/dkgsessionmgr.h(2 hunks)src/llmq/quorums.cpp(2 hunks)src/llmq/quorums.h(2 hunks)src/llmq/signing.cpp(2 hunks)src/llmq/signing.h(3 hunks)src/llmq/utils.cpp(2 hunks)src/masternode/active/notificationinterface.cpp(1 hunks)src/masternode/active/notificationinterface.h(1 hunks)src/node/chainstate.cpp(6 hunks)src/node/chainstate.h(4 hunks)src/qt/governancelist.cpp(1 hunks)src/qt/masternodelist.cpp(2 hunks)src/rpc/blockchain.cpp(1 hunks)src/rpc/evo.cpp(1 hunks)src/rpc/governance.cpp(1 hunks)src/rpc/masternode.cpp(7 hunks)src/test/evo_deterministicmns_tests.cpp(1 hunks)src/test/util/setup_common.cpp(7 hunks)src/test/util/setup_common.h(1 hunks)src/test/validation_chainstatemanager_tests.cpp(2 hunks)test/functional/wallet_signrawtransactionwithwallet.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (14)
- src/rpc/evo.cpp
- test/functional/wallet_signrawtransactionwithwallet.py
- src/instantsend/instantsend.h
- src/init/common.cpp
- src/llmq/utils.cpp
- src/evo/specialtxman.cpp
- src/bls/bls.cpp
- src/masternode/active/notificationinterface.cpp
- contrib/seeds/makeseeds.py
- src/qt/governancelist.cpp
- contrib/containers/ci/ci-slim.Dockerfile
- src/evo/deterministicmns.h
- src/qt/masternodelist.cpp
- src/evo/smldiff.cpp
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/node/chainstate.hsrc/node/chainstate.cppsrc/dsnotificationinterface.hsrc/rpc/governance.cppsrc/test/validation_chainstatemanager_tests.cppsrc/evo/evodb.hsrc/llmq/dkgsessionmgr.cppsrc/governance/governance.cppsrc/llmq/context.cppsrc/coinjoin/client.cppsrc/llmq/quorums.hsrc/llmq/context.hsrc/evo/deterministicmns.cppsrc/llmq/signing.cppsrc/dbwrapper.hsrc/llmq/quorums.cppsrc/init.cppsrc/dsnotificationinterface.cppsrc/bls/bls.hsrc/instantsend/db.hsrc/instantsend/instantsend.cppsrc/test/util/setup_common.hsrc/instantsend/db.cppsrc/rpc/masternode.cppsrc/evo/evodb.cppsrc/masternode/active/notificationinterface.hsrc/test/evo_deterministicmns_tests.cppsrc/llmq/dkgsessionmgr.hsrc/test/util/setup_common.cppsrc/llmq/signing.hsrc/rpc/blockchain.cpp
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/validation_chainstatemanager_tests.cppsrc/test/util/setup_common.hsrc/test/evo_deterministicmns_tests.cppsrc/test/util/setup_common.cpp
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/evodb.hsrc/evo/deterministicmns.cppsrc/evo/evodb.cppsrc/masternode/active/notificationinterface.h
🧠 Learnings (4)
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
PR: dashpay/dash#6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/node/chainstate.cppsrc/llmq/context.cppsrc/llmq/context.h
📚 Learning: 2025-01-02T21:50:00.967Z
Learnt from: kwvg
PR: dashpay/dash#6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
Applied to files:
src/llmq/context.h
📚 Learning: 2024-12-29T17:43:41.755Z
Learnt from: kwvg
PR: dashpay/dash#6504
File: src/llmq/quorums.cpp:224-224
Timestamp: 2024-12-29T17:43:41.755Z
Learning: The `CQuorumManager` is fully initialized by `LLMQContext`, addressing any concerns about the manager’s initialization sequence.
Applied to files:
src/llmq/context.h
📚 Learning: 2025-01-07T18:50:44.838Z
Learnt from: knst
PR: dashpay/dash#6511
File: src/evo/deterministicmns.cpp:1369-1373
Timestamp: 2025-01-07T18:50:44.838Z
Learning: The functions `MigrateDBIfNeeded` and `MigrateDBIfNeeded2` in `src/evo/deterministicmns.cpp` are temporary and will be removed in a future version. Refactoring suggestions for these functions should be avoided.
Applied to files:
src/evo/deterministicmns.cpp
🧬 Code graph analysis (20)
src/node/chainstate.h (1)
src/fs.h (1)
path(42-42)
src/dsnotificationinterface.h (1)
src/dsnotificationinterface.cpp (2)
CDSNotificationInterface(23-38)CDSNotificationInterface(40-40)
src/test/validation_chainstatemanager_tests.cpp (2)
src/node/chainstate.cpp (2)
DashChainstateSetup(220-261)DashChainstateSetup(220-236)src/test/util/setup_common.cpp (2)
DashChainstateSetup(126-136)DashChainstateSetup(126-130)
src/evo/evodb.h (2)
src/dbwrapper.h (2)
unique_ptr(737-740)CDBWrapper(214-534)src/evo/evodb.cpp (2)
CEvoDB(35-41)CEvoDB(43-43)
src/governance/governance.cpp (1)
src/script/descriptor.cpp (1)
nullopt(657-657)
src/coinjoin/client.cpp (3)
src/coinjoin/server.cpp (2)
DoMaintenance(899-907)DoMaintenance(899-899)src/coinjoin/coinjoin.cpp (2)
CheckQueue(135-150)CheckQueue(135-135)src/coinjoin/client.h (1)
CoinJoinWalletManager(73-108)
src/llmq/quorums.h (1)
src/llmq/quorums.cpp (2)
CQuorumManager(210-230)CQuorumManager(232-235)
src/llmq/context.h (1)
src/llmq/context.cpp (2)
LLMQContext(18-40)LLMQContext(42-44)
src/evo/deterministicmns.cpp (1)
src/evo/deterministicmns.h (2)
ForEachMNShared(307-556)ForEachMN(291-298)
src/llmq/signing.cpp (1)
src/llmq/signing.h (2)
CRecoveredSigsDb(119-285)CSigningManager(165-251)
src/dbwrapper.h (9)
src/evo/evodb.h (1)
util(12-14)src/instantsend/db.h (1)
util(27-29)src/instantsend/instantsend.h (1)
util(34-36)src/llmq/context.h (1)
util(32-34)src/llmq/dkgsessionmgr.h (1)
util(36-38)src/llmq/quorums.h (1)
util(40-42)src/llmq/signing.h (1)
util(35-37)src/fs.h (1)
path(42-42)src/dbwrapper.cpp (2)
CDBWrapper(129-183)CDBWrapper(185-197)
src/llmq/quorums.cpp (1)
src/llmq/utils.cpp (8)
InitQuorumsCache(962-968)InitQuorumsCache(962-962)InitQuorumsCache(969-970)InitQuorumsCache(971-972)InitQuorumsCache(973-978)InitQuorumsCache(979-980)InitQuorumsCache(981-982)InitQuorumsCache(983-984)
src/dsnotificationinterface.cpp (1)
src/dsnotificationinterface.h (1)
CDSNotificationInterface(18-58)
src/bls/bls.h (1)
src/bls/bls.cpp (2)
BLSInit(427-432)BLSInit(427-427)
src/instantsend/db.h (1)
src/instantsend/db.cpp (4)
Upgrade(37-51)Upgrade(37-37)CInstantSendDb(29-33)CInstantSendDb(35-35)
src/evo/evodb.cpp (1)
src/evo/evodb.h (1)
CEvoDB(38-76)
src/masternode/active/notificationinterface.h (1)
src/masternode/active/notificationinterface.cpp (2)
ActiveNotificationInterface(13-17)ActiveNotificationInterface(19-19)
src/llmq/dkgsessionmgr.h (2)
src/evo/evodb.h (1)
util(12-14)src/llmq/dkgsessionmgr.cpp (2)
CDKGSessionManager(32-61)CDKGSessionManager(63-63)
src/test/util/setup_common.cpp (1)
src/node/chainstate.cpp (2)
DashChainstateSetup(220-261)DashChainstateSetup(220-236)
src/llmq/signing.h (1)
src/llmq/signing.cpp (4)
CRecoveredSigsDb(30-33)CRecoveredSigsDb(35-35)CSigningManager(335-341)CSigningManager(343-343)
🪛 GitHub Actions: Clang Diff Format Check
src/evo/evodb.h
[error] 1-1: Clang-format differences found. Run clang-format-diff.py -p1 or update formatting to match style.
src/llmq/quorums.cpp
[error] 1-1: Clang-format differences found. Run clang-format-diff.py -p1 or update formatting to match style.
src/dsnotificationinterface.cpp
[error] 1-1: Clang-format differences found. Run clang-format-diff.py -p1 or update formatting to match style.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: linux64_ubsan-test / Test source
🔇 Additional comments (39)
src/masternode/active/notificationinterface.h (2)
23-23: LGTM! Completes non-copyable semantics.Deleting the copy-assignment operator is the correct approach since the copy constructor is already deleted. This follows the rule of five and ensures the class cannot be copied.
25-25: Acceptable destructor pattern aligning with broader refactoring.Moving the destructor definition to the .cpp file (while keeping the declaration in the header) is a common pattern for reducing header dependencies and maintaining consistency across the codebase. While not strictly necessary for this class (since members are references), it aligns with the PR's objective of tightening object lifetimes and ownership semantics.
src/bls/bls.h (1)
597-597: Signature change frombooltovoidis appropriate and all call sites are correctly updated.The change aligns with the fact that
SetSecureAllocator()has no error state. Verification confirms that both call sites insrc/test/util/setup_common.cpp:187andsrc/init/common.cpp:36correctly invokeBLSInit()without attempting to check a return value.src/rpc/blockchain.cpp (1)
1465-1491: LGTM! Documentation accurately reflects the actual output structure.The changes correctly update the help text to show that
softforksuses dynamic keys (deployment names) rather than a static structure. The nested hierarchy for BIP9 deployments is now properly documented with the optionalbip9object and its fields. All documented fields match the implementation inSoftForkDescPushBack().This aligns with the PR objective to fix help text discrepancies while keeping output unchanged.
src/coinjoin/client.cpp (4)
41-50: LGTM! Constructor and destructor follow RAII principles.The explicit constructor properly initializes all member references, and the defaulted destructor is appropriate since the class holds only non-owning references.
153-158: LGTM! DoMaintenance consolidation is clean.The guard conditions (sync status and shutdown check) are appropriate before calling
CheckQueue().
160-173: LGTM! Constructor properly initializes all dependencies.The constructor correctly takes non-owning references to all required managers and services. The defaulted destructor is appropriate.
1051-1052: LGTM! Lambda parameter change improves const-correctness.The change from explicit type to
const auto&is more concise while maintaining type safety and const-correctness, which aligns with the PR objectives.src/rpc/governance.cpp (1)
569-574: LGTM: Improved readability and const-correctness.The explicit
/*onlyValid=*/parameter naming andconst auto&lambda parameter type enhance code clarity and enforce immutability where appropriate.src/governance/governance.cpp (2)
79-85: LGTM: Consistent brace initialization.The switch from parentheses to brace initialization for member variables is stylistically consistent with modern C++ practices, though semantically equivalent here.
621-623: LGTM: Enhanced readability and const-correctness.The explicit
/*onlyValid=*/falseparameter naming andconst auto&lambda parameter improve code clarity and enforce immutability.src/test/util/setup_common.h (1)
90-92: LGTM: Clear parameter naming.The parameter rename from
fReset/fReindexChainStatetollmq_dbs_in_memory/llmq_dbs_wipebetter conveys their purpose and aligns with theDbWrapperParamsrefactor.src/test/util/setup_common.cpp (3)
219-224: LGTM: In-memory manager initialization.The new manager initialization (mn_metaman, netfulfilledman, sporkman, evodb, cpoolman, mnhf_manager) properly sets up in-memory databases for testing, improving test performance.
242-247: LGTM: Correct destruction order.The cleanup order in the destructor correctly reverses the construction order, preventing use-after-free issues.
320-335: LGTM: Updated chainstate loading.The LoadChainstate call now correctly passes
data_dirand in-memory flags, consistent with theDbWrapperParamsrefactor.src/test/validation_chainstatemanager_tests.cpp (2)
54-54: LGTM: Consistent with signature change.The updated call correctly uses the new parameter names and appropriate values for test setup.
87-87: LGTM: Consistent with signature change.The updated call correctly uses the new parameter names and appropriate values for test setup.
src/instantsend/db.h (2)
27-29: LGTM: Forward declaration for DbWrapperParams.Proper forward declaration for the new centralized DB configuration struct.
84-87: LGTM: Centralized DB configuration.The refactor to use
DbWrapperParamsconsolidates DB configuration parameters into a single struct, reducing parameter proliferation and improving maintainability.src/llmq/context.h (2)
32-34: LGTM: Forward declaration for DbWrapperParams.Proper forward declaration for the centralized DB configuration struct.
40-44: LGTM: Improved lifecycle management and API.The explicitly deleted copy assignment operator prevents accidental copies of this resource-managing class. The constructor signature update to use
DbWrapperParamsconsolidates DB configuration parameters, improving API clarity.Based on learnings
src/test/evo_deterministicmns_tests.cpp (1)
220-224: Const‑correct callback + explicit flag: LGTMUpdated ForEachMNShared usage and auto-typed lambda look correct. No behavior change.
src/dsnotificationinterface.cpp (4)
30-36: Ctor brace‑init: LGTMMember refs are correctly brace‑initialized in order.
40-40: Defaulted destructor: LGTMConsistent with header and avoids ODR/linker pitfalls.
1-1: No formatting issues detected—code is already compliant with clang-format standardsVerification confirms the repository has no staged or unstaged diffs, and the file
src/dsnotificationinterface.cppis properly formatted. Running the clang-format-diff script produced no output, indicating no formatting changes are needed. The copyright line and file structure conform to expected formatting standards.
44-46: No duplicate side effects detected; initialization pattern is correctThe code properly separates concerns:
SynchronousUpdatedBlockTipupdates only the deterministic masternode manager, whileUpdatedBlockTiphandles other components (sync, transactions, LLMQ, governance). WhenfInitialDownload=true,UpdatedBlockTipreturns early after updatingm_mn_sync, avoiding expensive work. Whenfalseat startup, both functions execute once each without overlap—no managers call back into handlers already invoked.src/dsnotificationinterface.h (1)
21-23: Ownership/copy semantics tightened: LGTMDefault ctor deleted; copy ops deleted; virtual dtor declared. Matches implementation and prevents unsafe default construction.
Also applies to: 31-31
src/node/chainstate.h (3)
116-133: Let me verify there are no other call sites that might have been missed:Based on my verification of the codebase, the new parameters (
data_dir,llmq_dbs_in_memory,llmq_dbs_wipe) have been properly propagated across all call sites:
- Main implementation call (src/node/chainstate.cpp:92-94): Correctly passes the three new parameters
- Wrapper function (src/test/util/setup_common.cpp:126-135): Properly extracts and forwards parameters from NodeContext
- Test callers (src/test/validation_chainstatemanager_tests.cpp:54, 87): Use the wrapper variant, which handles parameter forwarding
DashChainstateSetup: Parameter propagation verified
All callers have been updated. The main call site properly passes
data_dir,dash_dbs_in_memory, and the reset flag. The test wrapper abstracts this by extracting values fromNodeContextand forwarding them correctly.
82-114: ✓ Verified: All call sites updated with new parametersBoth LoadChainstate invocations have been correctly updated:
src/init.cpp:1970passesargs.GetDataDirNet()and/*dash_dbs_in_memory=*/falsesrc/test/util/setup_common.cpp:306passesAssert(m_node.args)->GetDataDirNet()and/*dash_dbs_in_memory=*/trueSignature changes are fully propagated. No issues found.
31-33: Forward declaration of fs::path is safe and properly implemented — verified that the custom fs::path wrapper class exists in src/fs.h, the LoadChainstate function signature correctly uses it as a reference parameter (line 96), and callers such as init.cpp properly include fs.h before use. The forward declaration is necessary and sufficient for the function signature in the header file.src/init.cpp (2)
2360-2363: Const-correct callback usage is good.Switch to named param and const auto& improves readability and safety.
1984-2000: Test callsite correctly sets dash_dbs_in_memory=true.Verification confirms src/test/util/setup_common.cpp:306 passes
/*dash_dbs_in_memory=*/truewhile production (src/init.cpp:1970) passesfalse. Test setup is correct for in-memory Evo/LLMQ DB performance; no action required.src/evo/evodb.cpp (1)
62-65: Dereferencing unique_ptr is correct here.Switch to db->WriteBatch(rootBatch) is appropriate after wrapping.
src/node/chainstate.cpp (2)
76-78: EvoDB wiring via DbWrapperParams looks correct.Reset and in‑memory flags are cleanly propagated.
92-95: Subpath uniqueness verified across all DB wrappers.All six DB wrapper instantiations across the codebase use distinct, collision-free subpaths:
blocks/index(txdb)evodb(evolution DB)llmq/recsigdb(recovered signatures)llmq/quorumdb(quorum data)llmq/dkgdb(DKG sessions)llmq/isdb(instant send)Each subdirectory is unique with proper hierarchical organization (evodb, blocks/, llmq/), confirming the review's concern has been addressed. The code structure ensures safe isolation of DB files per component.
src/instantsend/instantsend.cpp (1)
59-63: Constructor update aligns with the new DB params model.Looks good; relies on unique per‑component paths in the supplied DbWrapperParams.
src/llmq/quorums.cpp (3)
214-225: LGTM! Constructor refactored to support memory-only databases.The constructor now properly accepts
util::DbWrapperParamsand usesutil::MakeDbWrapperfor centralized DB creation. Thedb_params.memoryanddb_params.wipeflags are correctly propagated, enabling memory-only database mode that aligns with this PR's objective to restore memory-only databases for improved unit test performance.
232-235: LGTM! Formatting improved.Destructor reformatted from single-line to multi-line block with no functional change.
686-687: LGTM! Improved const-correctness and readability.The
ForEachMNcall now uses an explicit/*onlyValid=*/comment and aconst auto&lambda parameter, emphasizing const-correctness since the lambda only readsproTxHash. This aligns with the PR objective to "emphasize const-ness" in lambda signatures.
| {RPCResult::Type::BOOL, "automatic_pruning", /*optional=*/true, "whether automatic pruning is enabled (only present if pruning is enabled)"}, | ||
| {RPCResult::Type::NUM, "prune_target_size", /*optional=*/true, "the target size used by pruning (only present if automatic pruning is enabled)"}, | ||
| {RPCResult::Type::OBJ, "softforks", "status of softforks in progress", | ||
| {RPCResult::Type::OBJ_DYN, "softforks", "status of softforks in progress", |
There was a problem hiding this comment.
nit: rpc changes seems unrelated and good to go to own PR?
There was a problem hiding this comment.
Bundled here as change seemed too minor to have its own PR, meant for bitcoin#25308 but also valuable due to RPCResult validation.
|
|
||
| # Make sure CSV is active | ||
| self.generate(self.nodes[0], 500) | ||
| assert self.nodes[0].getblockchaininfo()['softforks']['csv']['active'] |
There was a problem hiding this comment.
test: drop extra generates from wallet_signrawtransactionwithwallet
not related to PR; should be its own PR.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/evo/deterministicmns.h (1)
8-29: Add missing include for std::function.This header uses std::function but doesn’t include . Add it to avoid build breaks and reliance on transitive includes.
#include <gsl/pointers.h> #include <immer/map.hpp> +#include <functional> #include <atomic> #include <limits> #include <numeric>src/coinjoin/client.cpp (1)
1036-1047: size_t underflow when computing nCountNotExcluded.If vecMasternodesUsed.size() > nCountEnabled, subtracting wraps and bypasses the early return.
Apply this diff:
- size_t nCountEnabled = mnList.GetValidMNsCount(); - size_t nCountNotExcluded = nCountEnabled - vecMasternodesUsed.size(); + const size_t nCountEnabled = mnList.GetValidMNsCount(); + const size_t nUsed = std::min(vecMasternodesUsed.size(), nCountEnabled); + const size_t nCountNotExcluded = nCountEnabled - nUsed; - WalletCJLogPrint(m_wallet, "CCoinJoinClientManager::%s -- %d enabled masternodes, %d masternodes to choose from\n", __func__, nCountEnabled, nCountNotExcluded); - if (nCountNotExcluded < 1) { + WalletCJLogPrint(m_wallet, "CCoinJoinClientManager::%s -- %zu enabled masternodes, %zu masternodes to choose from\n", __func__, nCountEnabled, nCountNotExcluded); + if (nCountNotExcluded == 0) { return nullptr; }
♻️ Duplicate comments (1)
src/rpc/blockchain.cpp (1)
1465-1492: Help schema aligns with dynamic softforks layout.Switch to OBJ_DYN with nested "bip9" details and conditional fields matches actual output. This improves RPCResult validation without behavioral change. LGTM.
🧹 Nitpick comments (7)
src/llmq/dkgsessionmgr.cpp (1)
36-45: DB initialization is unnecessary for non-MN/non-watcher nodes (lazy-init recommended).The review comment correctly identifies that
dbis opened unconditionally in the constructor initializer list (line 37), even though the constructor may early-return on line 48 for nodes that don't participate in DKG. Sincedbis already astd::unique_ptr<CDBWrapper>initialized to{nullptr}, and no members usedbduring their construction or before the early-return check, the suggested refactoring is valid and safe.Move DB initialization into the constructor body, conditioned on whether the node needs it:
-CDKGSessionManager::CDKGSessionManager(..., - const CSporkManager& sporkman, const util::DbWrapperParams& db_params) : - db{util::MakeDbWrapper({db_params.path / "llmq" / "dkgdb", /*cache_size=*/8 << 20, db_params.memory, db_params.wipe})}, - blsWorker{_blsWorker}, +CDKGSessionManager::CDKGSessionManager(..., + const CSporkManager& sporkman, const util::DbWrapperParams& db_params) : + db{nullptr}, + blsWorker{_blsWorker}, m_chainstate{chainstate}, m_dmnman{dmnman}, dkgDebugManager{_dkgDebugManager}, quorumBlockProcessor{_quorumBlockProcessor}, m_qsnapman{qsnapman}, spork_manager{sporkman} { + const bool needs_db = mn_activeman != nullptr || IsWatchQuorumsEnabled(); + if (needs_db) { + db = util::MakeDbWrapper({db_params.path / "llmq" / "dkgdb", /*cache_size=*/8 << 20, + db_params.memory, db_params.wipe}); + } if (mn_activeman == nullptr && !IsWatchQuorumsEnabled()) { // Regular nodes do not care about any DKG internals, bail out return; }src/instantsend/db.cpp (2)
29-33: Deduplicate DbWrapperParams construction in ctor to avoid drift.Build params once, reuse for db and upgrade; set wipe only for the upgrade params.
-CInstantSendDb::CInstantSendDb(const util::DbWrapperParams& db_params) : - db{util::MakeDbWrapper({db_params.path / "llmq" / "isdb", /*cache_size=*/32 << 20, db_params.memory, db_params.wipe})} -{ - Upgrade({db_params.path / "llmq" / "isdb", /*cache_size=*/32 << 20, db_params.memory, /*wipe=*/true}); +CInstantSendDb::CInstantSendDb(const util::DbWrapperParams& db_params) : + db{util::MakeDbWrapper({db_params.path / "llmq" / "isdb", /*cache_size=*/32 << 20, + db_params.memory, db_params.wipe})} +{ + auto upgrade_params = util::DbWrapperParams{db_params.path / "llmq" / "isdb", + /*cache_size=*/32 << 20, db_params.memory, /*wipe=*/true}; + Upgrade(upgrade_params); }
37-45: Clear caches if the DB is wiped/reinitialized.Upgrade() resets the DB when version changes. If this is ever called post-construction, in-memory caches may retain stale entries.
void CInstantSendDb::Upgrade(const util::DbWrapperParams& db_params) { LOCK(cs_db); int v{0}; if (!db->Read(DB_VERSION, v) || v < CInstantSendDb::CURRENT_VERSION) { // Wipe db db.reset(); - db = util::MakeDbWrapper(db_params); + db = util::MakeDbWrapper(db_params); + // Ensure caches don’t reference stale state after reinit + islockCache.clear(); + txidCache.clear(); + outpointCache.clear();Please confirm Upgrade() is only invoked during construction today; if not, the cache clears above (or equivalent) are needed.
src/evo/deterministicmns.h (1)
291-298: Avoid copying std::function in ForEachMN{,Shared}.Pass the callback by const reference to prevent an extra copy per call.
-void ForEachMN(bool onlyValid, std::function<void(const CDeterministicMN&)> cb) const +void ForEachMN(bool onlyValid, const std::function<void(const CDeterministicMN&)>& cb) const @@ -void ForEachMNShared(bool onlyValid, std::function<void(const CDeterministicMNCPtr&)> cb) const +void ForEachMNShared(bool onlyValid, const std::function<void(const CDeterministicMNCPtr&)>& cb) constAlso applies to: 307-314
src/node/chainstate.cpp (1)
87-91: Prefer std::make_unique for exception safety and clarity.Minor polish: use make_unique to construct CBlockTreeDB.
- pblocktree.reset(new CBlockTreeDB(nBlockTreeDBCache, block_tree_db_in_memory, fReset)); + pblocktree = std::make_unique<CBlockTreeDB>(nBlockTreeDBCache, block_tree_db_in_memory, fReset);src/coinjoin/client.cpp (2)
160-171: Avoid storing a reference to std::unique_ptr; store a raw/non‑owning pointer instead.Holding const std::unique_ptr<…>& as a member is brittle (dangling if owner moves/resets). Prefer a raw pointer or std::weak_ptr to the pointee.
Patch this cpp file:
-CCoinJoinClientManager::CCoinJoinClientManager(..., const std::unique_ptr<CCoinJoinClientQueueManager>& queueman) : +CCoinJoinClientManager::CCoinJoinClientManager(..., CCoinJoinClientQueueManager* queueman) : @@ - m_queueman{queueman} + m_queueman{queueman} @@ -CCoinJoinClientSession::CCoinJoinClientSession(..., const std::unique_ptr<CCoinJoinClientQueueManager>& queueman) : +CCoinJoinClientSession::CCoinJoinClientSession(..., CCoinJoinClientQueueManager* queueman) : @@ - m_queueman(queueman) + m_queueman(queueman) @@ -CoinJoinWalletManager::CoinJoinWalletManager(..., const std::unique_ptr<CCoinJoinClientQueueManager>& queueman) : +CoinJoinWalletManager::CoinJoinWalletManager(..., CCoinJoinClientQueueManager* queueman) : @@ - m_queueman{queueman} + m_queueman{queueman} @@ - std::make_unique<CCoinJoinClientManager>(wallet, m_dmnman, m_mn_metaman, m_mn_sync, - m_isman, m_queueman)); + std::make_unique<CCoinJoinClientManager>(wallet, 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, m_queueman); + deqSessions.emplace_back(m_wallet, *this, m_dmnman, m_mn_metaman, m_mn_sync, m_isman, m_queueman);And in the corresponding headers (shown here for clarity):
- explicit CoinJoinWalletManager(..., const std::unique_ptr<CCoinJoinClientQueueManager>& queueman); + explicit CoinJoinWalletManager(..., CCoinJoinClientQueueManager* queueman); - const std::unique_ptr<CCoinJoinClientQueueManager>& m_queueman; + CCoinJoinClientQueueManager* m_queueman{nullptr}; -class CCoinJoinClientManager { - CCoinJoinClientManager(..., const std::unique_ptr<CCoinJoinClientQueueManager>& queueman); - const std::unique_ptr<CCoinJoinClientQueueManager>& m_queueman; +class CCoinJoinClientManager { + CCoinJoinClientManager(..., CCoinJoinClientQueueManager* queueman); + CCoinJoinClientQueueManager* m_queueman{nullptr}; -class CCoinJoinClientSession { - CCoinJoinClientSession(..., const std::unique_ptr<CCoinJoinClientQueueManager>& queueman); - const std::unique_ptr<CCoinJoinClientQueueManager>& m_queueman; +class CCoinJoinClientSession { + CCoinJoinClientSession(..., CCoinJoinClientQueueManager* queueman); + CCoinJoinClientQueueManager* m_queueman{nullptr};Also applies to: 198-209, 1901-1912
1931-1938: Don’t run maintenance under the wallet map lock.Reduce lock contention and risk of lock inversion by copying pointers, releasing the lock, then iterating.
- LOCK(cs_wallet_manager_map); - for (auto& [_, clientman] : m_wallet_manager_map) { - clientman->DoMaintenance(m_chainman, connman, m_mempool); - } + std::vector<CCoinJoinClientManager*> clients; + { + LOCK(cs_wallet_manager_map); + clients.reserve(m_wallet_manager_map.size()); + for (auto& [_, clientman] : m_wallet_manager_map) clients.push_back(clientman.get()); + } + for (auto* clientman : clients) { + clientman->DoMaintenance(m_chainman, connman, m_mempool); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (41)
contrib/containers/ci/ci-slim.Dockerfile(1 hunks)contrib/seeds/makeseeds.py(1 hunks)src/bls/bls.cpp(1 hunks)src/bls/bls.h(1 hunks)src/coinjoin/client.cpp(4 hunks)src/dbwrapper.h(1 hunks)src/evo/deterministicmns.cpp(7 hunks)src/evo/deterministicmns.h(3 hunks)src/evo/evodb.cpp(2 hunks)src/evo/evodb.h(5 hunks)src/evo/smldiff.cpp(2 hunks)src/evo/specialtxman.cpp(2 hunks)src/governance/governance.cpp(2 hunks)src/init.cpp(3 hunks)src/init/common.cpp(1 hunks)src/instantsend/db.cpp(1 hunks)src/instantsend/db.h(2 hunks)src/instantsend/instantsend.cpp(1 hunks)src/instantsend/instantsend.h(2 hunks)src/llmq/context.cpp(1 hunks)src/llmq/context.h(1 hunks)src/llmq/dkgsessionmgr.cpp(1 hunks)src/llmq/dkgsessionmgr.h(2 hunks)src/llmq/quorums.cpp(2 hunks)src/llmq/quorums.h(2 hunks)src/llmq/signing.cpp(2 hunks)src/llmq/signing.h(3 hunks)src/llmq/utils.cpp(2 hunks)src/node/chainstate.cpp(6 hunks)src/node/chainstate.h(4 hunks)src/qt/governancelist.cpp(1 hunks)src/qt/masternodelist.cpp(2 hunks)src/rpc/blockchain.cpp(1 hunks)src/rpc/evo.cpp(1 hunks)src/rpc/governance.cpp(1 hunks)src/rpc/masternode.cpp(1 hunks)src/test/evo_deterministicmns_tests.cpp(1 hunks)src/test/util/setup_common.cpp(7 hunks)src/test/util/setup_common.h(1 hunks)src/test/validation_chainstatemanager_tests.cpp(2 hunks)test/functional/wallet_signrawtransactionwithwallet.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (19)
- src/instantsend/instantsend.cpp
- src/governance/governance.cpp
- src/test/evo_deterministicmns_tests.cpp
- src/node/chainstate.h
- src/rpc/evo.cpp
- src/rpc/masternode.cpp
- contrib/seeds/makeseeds.py
- src/llmq/signing.h
- src/test/util/setup_common.cpp
- src/evo/smldiff.cpp
- src/test/validation_chainstatemanager_tests.cpp
- src/evo/specialtxman.cpp
- src/qt/governancelist.cpp
- src/evo/evodb.h
- src/dbwrapper.h
- src/test/util/setup_common.h
- contrib/containers/ci/ci-slim.Dockerfile
- src/llmq/quorums.cpp
- test/functional/wallet_signrawtransactionwithwallet.py
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/init/common.cppsrc/llmq/context.hsrc/bls/bls.hsrc/llmq/quorums.hsrc/evo/deterministicmns.hsrc/bls/bls.cppsrc/instantsend/db.hsrc/qt/masternodelist.cppsrc/instantsend/instantsend.hsrc/llmq/context.cppsrc/instantsend/db.cppsrc/llmq/signing.cppsrc/evo/deterministicmns.cppsrc/llmq/utils.cppsrc/llmq/dkgsessionmgr.cppsrc/evo/evodb.cppsrc/rpc/governance.cppsrc/node/chainstate.cppsrc/init.cppsrc/llmq/dkgsessionmgr.hsrc/rpc/blockchain.cppsrc/coinjoin/client.cpp
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/deterministicmns.hsrc/evo/deterministicmns.cppsrc/evo/evodb.cpp
🧠 Learnings (4)
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
PR: dashpay/dash#6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/llmq/context.hsrc/llmq/context.cppsrc/node/chainstate.cpp
📚 Learning: 2025-01-02T21:50:00.967Z
Learnt from: kwvg
PR: dashpay/dash#6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
Applied to files:
src/llmq/context.h
📚 Learning: 2024-12-29T17:43:41.755Z
Learnt from: kwvg
PR: dashpay/dash#6504
File: src/llmq/quorums.cpp:224-224
Timestamp: 2024-12-29T17:43:41.755Z
Learning: The `CQuorumManager` is fully initialized by `LLMQContext`, addressing any concerns about the manager’s initialization sequence.
Applied to files:
src/llmq/context.h
📚 Learning: 2025-01-07T18:50:44.838Z
Learnt from: knst
PR: dashpay/dash#6511
File: src/evo/deterministicmns.cpp:1369-1373
Timestamp: 2025-01-07T18:50:44.838Z
Learning: The functions `MigrateDBIfNeeded` and `MigrateDBIfNeeded2` in `src/evo/deterministicmns.cpp` are temporary and will be removed in a future version. Refactoring suggestions for these functions should be avoided.
Applied to files:
src/evo/deterministicmns.hsrc/evo/deterministicmns.cpp
🧬 Code graph analysis (12)
src/init/common.cpp (1)
src/bls/bls.cpp (2)
BLSInit(427-432)BLSInit(427-427)
src/llmq/context.h (2)
src/evo/evodb.h (1)
util(12-14)src/llmq/context.cpp (2)
LLMQContext(18-40)LLMQContext(42-44)
src/bls/bls.h (1)
src/bls/bls.cpp (2)
BLSInit(427-432)BLSInit(427-427)
src/llmq/quorums.h (1)
src/llmq/quorums.cpp (2)
CQuorumManager(210-230)CQuorumManager(232-235)
src/evo/deterministicmns.h (1)
src/evo/deterministicmns.cpp (2)
CDeterministicMNManager(637-641)CDeterministicMNManager(643-643)
src/instantsend/db.h (1)
src/instantsend/db.cpp (4)
Upgrade(37-51)Upgrade(37-37)CInstantSendDb(29-33)CInstantSendDb(35-35)
src/instantsend/instantsend.h (1)
src/instantsend/instantsend.cpp (2)
CInstantSendManager(59-72)CInstantSendManager(74-74)
src/llmq/signing.cpp (1)
src/llmq/signing.h (2)
CRecoveredSigsDb(119-285)CSigningManager(165-251)
src/evo/deterministicmns.cpp (1)
src/evo/deterministicmns.h (2)
ForEachMNShared(307-556)ForEachMN(291-298)
src/evo/evodb.cpp (1)
src/evo/evodb.h (1)
CEvoDB(38-76)
src/llmq/dkgsessionmgr.h (2)
src/evo/evodb.h (1)
util(12-14)src/llmq/dkgsessionmgr.cpp (2)
CDKGSessionManager(32-61)CDKGSessionManager(63-63)
src/coinjoin/client.cpp (3)
src/coinjoin/server.cpp (2)
DoMaintenance(899-907)DoMaintenance(899-899)src/coinjoin/coinjoin.cpp (2)
CheckQueue(135-150)CheckQueue(135-135)src/coinjoin/client.h (1)
CoinJoinWalletManager(73-108)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build container / Build container
🔇 Additional comments (30)
src/bls/bls.h (1)
597-597: LGTM: BLSInit() return type simplified to void.The change from
booltovoidaligns with the underlyingSetSecureAllocator()function which is also void and doesn't perform sanity checks. This is consistent with moving the call fromSanityChecks()toSetGlobals().src/bls/bls.cpp (1)
427-432: LGTM: Implementation correctly updated to void return type.The function body remains unchanged and the void return type is consistent with the header declaration.
src/instantsend/db.h (1)
27-29: LGTM: Clean refactor to use DbWrapperParams.The forward declaration and updated signatures consolidate multiple boolean parameters into a single
DbWrapperParamsobject, improving API clarity and consistency with the broader codebase refactor.Also applies to: 84-87
src/rpc/governance.cpp (1)
569-574: LGTM: Named parameter improves readability.The explicit
/*onlyValid=*/comment clarifies the boolean parameter's purpose without changing behavior.src/qt/masternodelist.cpp (1)
179-185: LGTM: Named parameters improve readability.Both
ForEachMNcalls now have explicit/*onlyValid=*/comments that clarify the boolean parameter's purpose.Also applies to: 211-218
src/init/common.cpp (1)
29-37: LGTM: BLSInit() correctly moved to SetGlobals().Moving
BLSInit()fromSanityChecks()toSetGlobals()is appropriate sinceSetSecureAllocator()is a setup operation rather than a sanity check. The placement afterECC_Start()ensures proper initialization order.src/llmq/utils.cpp (1)
147-159: LGTM: Named parameters improve readability.Both
ForEachMNSharedcalls now have explicit/*onlyValid=*/comments that clarify the boolean parameter's purpose.Also applies to: 490-496
src/evo/deterministicmns.cpp (5)
193-213: LGTM: Named parameters and const-correct lambdas.The
ForEachMNSharedcalls now use explicit/*onlyValid=*/comments and maintain const-correctness withconst auto& dmnlambda parameters.
234-254: LGTM: Named parameters improve readability.Both
ForEachMNSharedcalls now have explicit/*onlyValid=*/comments that clarify the boolean parameter's purpose.
279-281: LGTM: Consistent use of named parameter.The
ForEachMNcall uses the explicit/*onlyValid=*/falsecomment for clarity.
336-341: LGTM: Named parameter improves readability.The
ForEachMNSharedcall uses the explicit/*onlyValid=*/truecomment for clarity.
637-643: LGTM: Explicit constructor and destructor added.The explicit constructor using member initializer list and defaulted destructor follow modern C++ best practices and are consistent with the broader refactoring to pass
CEvoDBandCMasternodeMetaManas constructor parameters.src/init.cpp (2)
2360-2363: Const-correct masternode iteration.Switch to named flag and const auto& dmn is good and safer for captures.
1968-2006: Verification complete: test harness correctly configures in-memory DBs.The codebase properly implements the plumbing:
- Production passes
/*dash_dbs_in_memory=*/falseto LoadChainstate, correctly using disk storage- Test setup creates CEvoDB with
/*memory=*/true- Tests calling DashChainstateSetup directly pass
/*llmq_dbs_in_memory=*/true- DashChainstateSetup properly forwards the memory flag through DbWrapperParams to LLMQ context initialization
The restored memory-only code path is properly enabled for all unit and functional tests.
src/llmq/context.h (2)
31-34: Lean header dependency.Forward-declaring util::DbWrapperParams here is the right call to avoid heavy includes.
40-44: Verification complete: all construction sites properly updatedThe review is accurate. Verified one direct construction site in
src/node/chainstate.cpp:250-252wherestd::make_unique<LLMQContext>is called with the newconst util::DbWrapperParams& db_paramsparameter constructed asutil::DbWrapperParams{data_dir, llmq_dbs_in_memory, llmq_dbs_wipe}. The parameter is correctly propagated through member initializers toqdkgsman,qman,sigman, andisman. All other usages retrieve the already-constructed context via accessor functions, not direct construction. The deletedoperator=is confirmed in place.src/instantsend/instantsend.h (2)
34-36: Forward-declared DbWrapperParams is fine.Keeps headers slim and avoids unnecessary rebuilds.
95-101: API change verified: all construction sites properly updated.The unique instantiation in
src/llmq/context.cpp(lines 35–36) correctly passes all required parameters including the newdb_paramsparameter to match the updated constructor signature. No outstanding issues.src/llmq/quorums.h (2)
40-42: Header-only dependency trim.Forward-decl util::DbWrapperParams here is appropriate.
265-273: Constructor signature confirmed correctly forwarded to all instantiation sites.The
CQuorumManagerconstructor receivingdb_paramsis properly instantiated withinLLMQContext's constructor at src/llmq/context.cpp:30-32 with the parameter correctly passed. The only instantiation site is in src/node/chainstate.cpp:250-252 whereLLMQContextis created with appropriateDbWrapperParams. No additional caller updates are needed.src/evo/deterministicmns.h (1)
655-660: Manager lifecycle tightening looks good.Deleting default/copy/assignment and making construction explicit with CEvoDB/MetaMan improves safety.
src/evo/evodb.cpp (2)
34-39: Ctor migration to util::MakeDbWrapper is correct.Initialization of rootBatch/rootDBTransaction/curDBTransaction from the wrapper is clean and safe. No issues spotted.
50-54: RollbackCurTransaction locking is correct.Clears the nested transaction under cs, matching CommitCurTransaction. LGTM.
src/llmq/signing.cpp (2)
30-33: RecoveredSigs DB wrapper migration looks good.Uses dedicated path and 8 MiB cache; aligns with other modules. LGTM.
335-343: CSigningManager now takes DbWrapperParams — OK.Constructor wiring preserves ordering; defaulted dtor fine.
src/llmq/context.cpp (1)
21-37: LLMQContext db_params propagation — verified and correct.The constructor signature and implementation in
src/llmq/context.cpp(lines 21-37) correctly propagatedb_paramsto all subcomponents. The single instantiation site atsrc/node/chainstate.cpp:250-252has been updated with the complete argument list, includingutil::DbWrapperParams{data_dir, llmq_dbs_in_memory, llmq_dbs_wipe}.src/node/chainstate.cpp (2)
51-51: Good migration to DbWrapperParams and memory-only toggles.Passing data_dir and dash_dbs_in_memory through DbWrapperParams for CEvoDB centralizes config and enables in‑memory mode cleanly. LGTM.
Also applies to: 66-66, 77-77
36-69: No issues found—all call sites correctly updated.Verification confirms that all call sites for
LoadChainstateandDashChainstateSetuphave been properly updated with the new parameters:
- LoadChainstate: Both call sites (src/init.cpp:1970 and src/test/util/setup_common.cpp:306) pass all 32 parameters, including the new
data_dirand*_dbs_in_memoryflags.- DashChainstateSetup (main): The call at src/node/chainstate.cpp:92 correctly passes all 17 parameters including
data_dirand the LLMQ flags.- DashChainstateSetup (wrapper): The test helper wrapper in src/test/util/setup_common.cpp properly forwards all parameters to the main version. Tests (src/test/validation_chainstatemanager_tests.cpp:54, 87) correctly use the NodeContext-friendly wrapper variant.
All parameters are aligned and no missing updates were detected across the codebase.
src/llmq/dkgsessionmgr.h (1)
36-39: Constructor tightening and DB param forward-declare look good.Noncopyable manager with explicit ctor taking util::DbWrapperParams is appropriate; forward-declare avoids heavy includes. Public StopThreads is a sensible complement to StartThreads. LGTM.
Also applies to: 84-92
src/coinjoin/client.cpp (1)
153-158: Queue maintenance hook is fine.DoMaintenance gating on sync/shutdown and calling CheckQueue matches server pattern. LGTM.
| DashChainstateSetup(chainman, govman, mn_metaman, mn_sync, sporkman, mn_activeman, chain_helper, cpoolman, | ||
| dmnman, evodb, mnhf_manager, llmq_ctx, mempool, fReset, fReindexChainState, | ||
| consensus_params); | ||
| dmnman, evodb, mnhf_manager, llmq_ctx, mempool, data_dir, dash_dbs_in_memory, | ||
| /*llmq_dbs_wipe=*/fReset || fReindexChainState, consensus_params); | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
src/dsnotificationinterface.cpp (1)
1-1: Address clang-format differences.The pipeline indicates clang-format differences in this file.
Please run clang-format on this file to resolve the formatting issue:
clang-format -i src/dsnotificationinterface.cppsrc/coinjoin/server.cpp (3)
249-253: Fix format specifier for size_tvecTxIn.size() is size_t; using %s is incorrect.
Apply:
- LogPrint(BCLog::COINJOIN, "DSSIGNFINALTX -- vecTxIn.size() %s\n", vecTxIn.size()); + LogPrint(BCLog::COINJOIN, "DSSIGNFINALTX -- vecTxIn.size() %zu\n", vecTxIn.size());
281-286: Fix format specifier mismatchentries is int; %lu expects unsigned long.
Apply:
- if (int entries = GetEntriesCount(); entries != 0) LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckPool -- entries count %lu\n", entries); + if (int entries = GetEntriesCount(); entries != 0) LogPrint(BCLog::COINJOIN, "CCoinJoinServer::CheckPool -- entries count %d\n", entries);
846-849: Fix format specifier for unsigned intnDisconnected is unsigned int; %llu expects unsigned long long.
Apply:
- LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- can't continue, %llu client(s) disconnected, nSessionID: %d nSessionDenom: %d (%s)\n", + LogPrint(BCLog::COINJOIN, "CCoinJoinServer::%s -- can't continue, %u client(s) disconnected, nSessionID: %d nSessionDenom: %d (%s)\n", __func__, nDisconnected, nSessionID, nSessionDenom, CoinJoin::DenominationToString(nSessionDenom));src/llmq/snapshot.cpp (1)
366-375: Do not bypass CEvoDB transactions; write via the current transaction.
GetRawDB().Write(...)skipsCEvoDB’s transactional path, risking inconsistent state and breaking commit/rollback guarantees. Write throughGetCurTransaction()(while holdingm_evoDb.cs) so snapshot persistence participates in the same atomic unit as other EvoDB updates.Apply:
void CQuorumSnapshotManager::StoreSnapshotForBlock(const Consensus::LLMQType llmqType, const CBlockIndex* pindex, const CQuorumSnapshot& snapshot) { auto snapshotHash = ::SerializeHash(std::make_pair(llmqType, pindex->GetBlockHash())); - // LOCK(::cs_main); - AssertLockNotHeld(m_evoDb.cs); - LOCK2(snapshotCacheCs, m_evoDb.cs); - m_evoDb.GetRawDB().Write(std::make_pair(DB_QUORUM_SNAPSHOT, snapshotHash), snapshot); - quorumSnapshotCache.insert(snapshotHash, snapshot); + LOCK2(snapshotCacheCs, m_evoDb.cs); + const auto key = std::make_pair(DB_QUORUM_SNAPSHOT, snapshotHash); + const bool ok = m_evoDb.GetCurTransaction().Write(key, snapshot); + assert(ok); + quorumSnapshotCache.insert(snapshotHash, snapshot); }src/llmq/signing.cpp (1)
759-763: Fix: missing namespace for ranges::any_of (compile error).Use std::ranges::any_of or std::any_of with iterators.
Apply this minimal fix:
- return ranges::any_of(quorums, [&quorumHash](const auto& q){ return q->qc->quorumHash == quorumHash; }); + return std::ranges::any_of(quorums, [&quorumHash](const auto& q){ return q->qc->quorumHash == quorumHash; });Alternatively:
- return ranges::any_of(quorums, [&quorumHash](const auto& q){ return q->qc->quorumHash == quorumHash; }); + return std::any_of(quorums.begin(), quorums.end(), + [&quorumHash](const auto& q){ return q->qc->quorumHash == quorumHash; });src/llmq/quorums.cpp (1)
1-1: Fix clang-format violations.The pipeline detected formatting differences in this file. Please run clang-format to address the formatting issues.
♻️ Duplicate comments (3)
src/node/chainstate.cpp (1)
233-236: Guard mempool pointer before use in DashChainstateSetup.
mempoolis dereferenced unconditionally below. Add a non‑fatal guard at function entry to prevent accidental null deref in tests or future call paths.void DashChainstateSetup(ChainstateManager& chainman, @@ - bool llmq_dbs_wipe, + bool llmq_dbs_wipe, const Consensus::Params& consensus_params) { + CHECK_NONFATAL(mempool != nullptr);Based on learnings
src/test/util/setup_common.cpp (1)
235-256: Explicit teardown order acknowledged.Manual resets enforce reverse-init destruction, consistent with prior rationale. No change requested.
src/coinjoin/client.h (1)
262-267: Address previous review: Make cached height fields atomic.A previous review flagged that
nCachedLastSuccessBlock(line 262) andnCachedBlockHeight(line 267) are accessed from multiple threads without locks and should bestd::atomic<int>. This concern has not been addressed in the current changes.Based on learnings
🧹 Nitpick comments (8)
src/llmq/debug.h (1)
104-107: LGTM! Correct non-copyable semantics for manager class.Deleting the copy constructor and copy assignment operator is the correct approach for a manager class with a
Mutexmember (line 100), which is inherently non-copyable. The destructor declaration (defaulted in the .cpp per relevant snippets) follows a valid pattern for controlling instantiation location.Move operations are implicitly deleted due to the declared destructor and deleted copy operations, which is the correct behavior for a class with a non-moveable mutex.
Optional: Explicitly delete move operations for maximum clarity.
For completeness, you could explicitly delete move operations to make the intent more obvious to readers:
CDKGDebugManager(const CDKGDebugManager&) = delete; CDKGDebugManager& operator=(const CDKGDebugManager&) = delete; +CDKGDebugManager(CDKGDebugManager&&) = delete; +CDKGDebugManager& operator=(CDKGDebugManager&&) = delete; CDKGDebugManager(); ~CDKGDebugManager();However, this is purely a stylistic choice—the current implementation is correct since move operations are already implicitly deleted.
src/dbwrapper.h (1)
716-722: Consider passing path by const reference.The constructor takes
pathby value, causing an unnecessary copy. Sincepathis directly used to initialize a const member, consider usingconst fs::path&for the parameter.Apply this diff:
- DbWrapperParams(const fs::path path, size_t cache_size, bool memory, bool wipe) : + DbWrapperParams(const fs::path& path, size_t cache_size, bool memory, bool wipe) : path{path},src/netfulfilledman.cpp (1)
21-29: Consider renaming LoadCache to reflect dual behavior.The method now either loads or stores based on the
load_cacheparameter (line 24). The nameLoadCacheis misleading whenload_cache=falsecauses it to store instead. Consider renaming to something likeInitializeCacheorLoadOrStoreCachefor clarity.src/evo/evodb.cpp (1)
34-38: Centralize EvoDB cache default.Hardcoded 64 MiB is fine for tests, but consider a named constant (e.g., EVO_DB_CACHE_BYTES) or deriving from a single config point to avoid divergence across components.
src/llmq/snapshot.cpp (1)
347-364: Avoid holding cache lock during DB I/O (reduce contention).Release
snapshotCacheCsbefore the disk read, then reacquire only to insert on hit. This keeps lock ordering (cache → db) while minimizing time under the cache lock.- LOCK(snapshotCacheCs); - // try using cache before reading from disk - if (quorumSnapshotCache.get(snapshotHash, snapshot)) { - return snapshot; - } - if (m_evoDb.Read(std::make_pair(DB_QUORUM_SNAPSHOT, snapshotHash), snapshot)) { - quorumSnapshotCache.insert(snapshotHash, snapshot); - return snapshot; - } + { + LOCK(snapshotCacheCs); + if (quorumSnapshotCache.get(snapshotHash, snapshot)) { + return snapshot; + } + } + if (m_evoDb.Read(std::make_pair(DB_QUORUM_SNAPSHOT, snapshotHash), snapshot)) { + LOCK(snapshotCacheCs); + quorumSnapshotCache.insert(snapshotHash, snapshot); + return snapshot; + }src/llmq/quorums.h (1)
265-273: Non-copyable (and effectively non-movable) CQuorumManager: confirm intent or make move deletion explicit.With a user-declared destructor and deleted copy members, move ctor/assign are implicitly not generated. If immovability is intentional, consider deleting moves explicitly to document it; otherwise, add a move ctor/assign.
Apply one of:
class CQuorumManager { public: CQuorumManager() = delete; CQuorumManager(const CQuorumManager&) = delete; CQuorumManager& operator=(const CQuorumManager&) = delete; + CQuorumManager(CQuorumManager&&) = delete; + CQuorumManager& operator=(CQuorumManager&&) = delete; explicit CQuorumManager(...);Please confirm immovability is intended for lifetime/ownership guarantees around the DB and worker threads.
src/llmq/signing.cpp (1)
30-33: Db params-driven DB construction LGTM; consider naming the cache size.The MakeDbWrapper call and path are correct. Consider replacing 8<<20 with a named constant to avoid magic numbers.
- db{util::MakeDbWrapper({db_params.path / "llmq" / "recsigdb", /*cache_size=*/8 << 20, db_params.memory, db_params.wipe})} + db{util::MakeDbWrapper({db_params.path / "llmq" / "recsigdb", + /*cache_size=*/REC_SIG_DB_CACHE_BYTES, + db_params.memory, db_params.wipe})}and define:
constexpr size_t REC_SIG_DB_CACHE_BYTES = 8u << 20;src/coinjoin/server.h (1)
97-105: Lifecycle tightened; constructor/destructor split LGTM.Publicly deleting default/copy/assign and moving the ctor body to .cpp improve clarity. Consider optionally deleting move ops for explicit immovability, mirroring the pattern used elsewhere.
- CCoinJoinServer() = delete; - CCoinJoinServer(const CCoinJoinServer&) = delete; - CCoinJoinServer& operator=(const CCoinJoinServer&) = delete; + CCoinJoinServer() = delete; + CCoinJoinServer(const CCoinJoinServer&) = delete; + CCoinJoinServer& operator=(const CCoinJoinServer&) = delete; + CCoinJoinServer(CCoinJoinServer&&) = delete; + CCoinJoinServer& operator=(CCoinJoinServer&&) = delete;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (78)
contrib/containers/ci/ci-slim.Dockerfile(1 hunks)contrib/seeds/makeseeds.py(1 hunks)src/bls/bls.cpp(1 hunks)src/bls/bls.h(1 hunks)src/bls/bls_worker.h(1 hunks)src/chainlock/chainlock.h(1 hunks)src/chainlock/signing.h(1 hunks)src/coinjoin/client.cpp(4 hunks)src/coinjoin/client.h(3 hunks)src/coinjoin/coinjoin.cpp(2 hunks)src/coinjoin/coinjoin.h(2 hunks)src/coinjoin/server.cpp(1 hunks)src/coinjoin/server.h(1 hunks)src/dbwrapper.h(1 hunks)src/dsnotificationinterface.cpp(1 hunks)src/dsnotificationinterface.h(1 hunks)src/evo/chainhelper.h(1 hunks)src/evo/creditpool.cpp(1 hunks)src/evo/creditpool.h(1 hunks)src/evo/deterministicmns.cpp(7 hunks)src/evo/deterministicmns.h(3 hunks)src/evo/evodb.cpp(2 hunks)src/evo/evodb.h(5 hunks)src/evo/mnhftx.cpp(1 hunks)src/evo/mnhftx.h(1 hunks)src/evo/smldiff.cpp(2 hunks)src/evo/specialtxman.cpp(2 hunks)src/governance/governance.cpp(2 hunks)src/governance/governance.h(1 hunks)src/governance/signing.h(1 hunks)src/init.cpp(3 hunks)src/init/common.cpp(1 hunks)src/instantsend/db.cpp(1 hunks)src/instantsend/db.h(2 hunks)src/instantsend/instantsend.cpp(1 hunks)src/instantsend/instantsend.h(2 hunks)src/instantsend/signing.h(1 hunks)src/llmq/blockprocessor.cpp(2 hunks)src/llmq/blockprocessor.h(1 hunks)src/llmq/context.cpp(1 hunks)src/llmq/context.h(1 hunks)src/llmq/debug.cpp(1 hunks)src/llmq/debug.h(1 hunks)src/llmq/dkgsessionmgr.cpp(1 hunks)src/llmq/dkgsessionmgr.h(2 hunks)src/llmq/quorums.cpp(2 hunks)src/llmq/quorums.h(2 hunks)src/llmq/signing.cpp(2 hunks)src/llmq/signing.h(3 hunks)src/llmq/signing_shares.h(1 hunks)src/llmq/snapshot.cpp(1 hunks)src/llmq/snapshot.h(1 hunks)src/llmq/utils.cpp(2 hunks)src/masternode/active/context.h(1 hunks)src/masternode/active/notificationinterface.cpp(1 hunks)src/masternode/active/notificationinterface.h(1 hunks)src/masternode/meta.cpp(1 hunks)src/masternode/meta.h(1 hunks)src/masternode/node.cpp(2 hunks)src/masternode/node.h(1 hunks)src/masternode/sync.cpp(1 hunks)src/masternode/sync.h(1 hunks)src/netfulfilledman.cpp(1 hunks)src/netfulfilledman.h(1 hunks)src/node/chainstate.cpp(6 hunks)src/node/chainstate.h(4 hunks)src/qt/governancelist.cpp(1 hunks)src/qt/masternodelist.cpp(2 hunks)src/rpc/blockchain.cpp(1 hunks)src/rpc/evo.cpp(1 hunks)src/rpc/governance.cpp(1 hunks)src/rpc/masternode.cpp(1 hunks)src/spork.h(1 hunks)src/test/evo_deterministicmns_tests.cpp(1 hunks)src/test/util/setup_common.cpp(7 hunks)src/test/util/setup_common.h(1 hunks)src/test/validation_chainstatemanager_tests.cpp(2 hunks)test/functional/wallet_signrawtransactionwithwallet.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/test/util/setup_common.h
🚧 Files skipped from review as they are similar to previous changes (26)
- src/evo/mnhftx.cpp
- src/rpc/evo.cpp
- src/init/common.cpp
- src/test/evo_deterministicmns_tests.cpp
- src/test/validation_chainstatemanager_tests.cpp
- src/masternode/active/notificationinterface.cpp
- src/rpc/masternode.cpp
- src/llmq/debug.cpp
- src/masternode/active/notificationinterface.h
- src/qt/masternodelist.cpp
- test/functional/wallet_signrawtransactionwithwallet.py
- src/llmq/context.h
- src/governance/governance.cpp
- src/chainlock/chainlock.h
- src/llmq/blockprocessor.cpp
- src/qt/governancelist.cpp
- src/coinjoin/coinjoin.cpp
- src/masternode/sync.cpp
- src/evo/evodb.h
- src/llmq/signing.h
- src/evo/deterministicmns.cpp
- src/llmq/utils.cpp
- src/instantsend/db.h
- contrib/seeds/makeseeds.py
- src/evo/deterministicmns.h
- src/instantsend/signing.h
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/evo/specialtxman.cppsrc/dsnotificationinterface.cppsrc/rpc/governance.cppsrc/coinjoin/server.cppsrc/masternode/sync.hsrc/bls/bls.hsrc/llmq/snapshot.hsrc/evo/mnhftx.hsrc/coinjoin/coinjoin.hsrc/dsnotificationinterface.hsrc/evo/smldiff.cppsrc/masternode/meta.cppsrc/instantsend/db.cppsrc/llmq/signing.cppsrc/evo/creditpool.hsrc/node/chainstate.hsrc/instantsend/instantsend.cppsrc/llmq/blockprocessor.hsrc/masternode/meta.hsrc/evo/creditpool.cppsrc/instantsend/instantsend.hsrc/coinjoin/server.hsrc/llmq/quorums.cppsrc/bls/bls_worker.hsrc/llmq/dkgsessionmgr.cppsrc/bls/bls.cppsrc/governance/signing.hsrc/llmq/debug.hsrc/dbwrapper.hsrc/evo/evodb.cppsrc/netfulfilledman.hsrc/llmq/context.cppsrc/rpc/blockchain.cppsrc/chainlock/signing.hsrc/coinjoin/client.hsrc/llmq/dkgsessionmgr.hsrc/coinjoin/client.cppsrc/evo/chainhelper.hsrc/governance/governance.hsrc/node/chainstate.cppsrc/llmq/snapshot.cppsrc/spork.hsrc/init.cppsrc/masternode/node.cppsrc/masternode/node.hsrc/llmq/signing_shares.hsrc/netfulfilledman.cppsrc/test/util/setup_common.cppsrc/llmq/quorums.hsrc/masternode/active/context.h
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/specialtxman.cppsrc/masternode/sync.hsrc/evo/mnhftx.hsrc/evo/smldiff.cppsrc/masternode/meta.cppsrc/evo/creditpool.hsrc/masternode/meta.hsrc/evo/creditpool.cppsrc/evo/evodb.cppsrc/evo/chainhelper.hsrc/masternode/node.cppsrc/masternode/node.hsrc/masternode/active/context.h
contrib/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the contrib directory (contributed scripts)
Files:
contrib/containers/ci/ci-slim.Dockerfile
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/util/setup_common.cpp
🧠 Learnings (1)
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
PR: dashpay/dash#6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/llmq/context.cppsrc/node/chainstate.cpp
🧬 Code graph analysis (33)
src/dsnotificationinterface.cpp (1)
src/dsnotificationinterface.h (1)
CDSNotificationInterface(18-58)
src/masternode/sync.h (1)
src/masternode/sync.cpp (2)
CMasternodeSync(17-23)CMasternodeSync(25-25)
src/bls/bls.h (1)
src/bls/bls.cpp (2)
BLSInit(427-432)BLSInit(427-427)
src/llmq/snapshot.h (1)
src/llmq/snapshot.cpp (2)
CQuorumSnapshotManager(339-343)CQuorumSnapshotManager(345-345)
src/evo/mnhftx.h (1)
src/evo/mnhftx.cpp (2)
CMNHFManager(47-52)CMNHFManager(54-58)
src/coinjoin/coinjoin.h (1)
src/coinjoin/coinjoin.cpp (4)
CCoinJoinBaseManager(125-125)CCoinJoinBaseManager(127-127)CDSTXManager(401-401)CDSTXManager(403-403)
src/dsnotificationinterface.h (1)
src/dsnotificationinterface.cpp (2)
CDSNotificationInterface(23-38)CDSNotificationInterface(40-40)
src/llmq/signing.cpp (1)
src/llmq/signing.h (2)
CRecoveredSigsDb(119-285)CSigningManager(165-251)
src/evo/creditpool.h (1)
src/evo/creditpool.cpp (2)
CCreditPoolManager(241-244)CCreditPoolManager(246-246)
src/node/chainstate.h (1)
src/fs.h (1)
path(42-42)
src/llmq/blockprocessor.h (1)
src/llmq/blockprocessor.cpp (2)
CQuorumBlockProcessor(48-71)CQuorumBlockProcessor(73-76)
src/masternode/meta.h (1)
src/masternode/meta.cpp (2)
CMasternodeMetaMan(13-16)CMasternodeMetaMan(18-22)
src/instantsend/instantsend.h (1)
src/instantsend/instantsend.cpp (2)
CInstantSendManager(59-72)CInstantSendManager(74-74)
src/coinjoin/server.h (1)
src/coinjoin/server.cpp (2)
CCoinJoinServer(26-43)CCoinJoinServer(45-45)
src/llmq/quorums.cpp (1)
src/llmq/utils.cpp (8)
InitQuorumsCache(962-968)InitQuorumsCache(962-962)InitQuorumsCache(969-970)InitQuorumsCache(971-972)InitQuorumsCache(973-978)InitQuorumsCache(979-980)InitQuorumsCache(981-982)InitQuorumsCache(983-984)
src/governance/signing.h (1)
src/governance/signing.cpp (2)
GovernanceSigner(24-34)GovernanceSigner(36-36)
src/llmq/debug.h (1)
src/llmq/debug.cpp (2)
CDKGDebugManager(110-110)CDKGDebugManager(112-112)
src/dbwrapper.h (9)
src/evo/evodb.h (1)
util(12-14)src/instantsend/db.h (1)
util(27-29)src/instantsend/instantsend.h (1)
util(34-36)src/llmq/context.h (1)
util(32-34)src/llmq/dkgsessionmgr.h (1)
util(36-38)src/llmq/quorums.h (1)
util(40-42)src/llmq/signing.h (1)
util(35-37)src/node/chainstate.h (1)
fs(31-33)src/dbwrapper.cpp (2)
CDBWrapper(129-183)CDBWrapper(185-197)
src/evo/evodb.cpp (1)
src/evo/evodb.h (1)
CEvoDB(38-76)
src/netfulfilledman.h (1)
src/netfulfilledman.cpp (2)
CNetFulfilledRequestManager(10-13)CNetFulfilledRequestManager(15-19)
src/chainlock/signing.h (1)
src/chainlock/signing.cpp (2)
ChainLockSigner(19-29)ChainLockSigner(31-31)
src/coinjoin/client.h (1)
src/coinjoin/client.cpp (6)
CoinJoinWalletManager(1901-1913)CoinJoinWalletManager(1915-1921)CCoinJoinClientQueueManager(41-48)CCoinJoinClientQueueManager(50-50)CCoinJoinClientManager(160-171)CCoinJoinClientManager(173-173)
src/llmq/dkgsessionmgr.h (1)
src/llmq/dkgsessionmgr.cpp (2)
CDKGSessionManager(32-61)CDKGSessionManager(63-63)
src/coinjoin/client.cpp (3)
src/coinjoin/server.cpp (2)
DoMaintenance(899-907)DoMaintenance(899-899)src/coinjoin/coinjoin.cpp (2)
CheckQueue(135-150)CheckQueue(135-135)src/coinjoin/client.h (1)
CoinJoinWalletManager(73-108)
src/evo/chainhelper.h (1)
src/evo/chainhelper.cpp (2)
CChainstateHelper(15-26)CChainstateHelper(28-28)
src/governance/governance.h (1)
src/governance/governance.cpp (2)
CGovernanceManager(70-86)CGovernanceManager(88-92)
src/llmq/snapshot.cpp (1)
src/llmq/snapshot.h (1)
CQuorumSnapshotManager(216-234)
src/spork.h (1)
src/spork.cpp (2)
CSporkManager(66-69)CSporkManager(71-75)
src/masternode/node.h (1)
src/masternode/node.cpp (2)
CActiveMasternodeManager(52-62)CActiveMasternodeManager(64-64)
src/llmq/signing_shares.h (1)
src/llmq/signing_shares.cpp (2)
CSigSharesManager(183-195)CSigSharesManager(197-197)
src/netfulfilledman.cpp (2)
src/netfulfilledman.h (1)
is_valid(50-69)src/wallet/sqlite.h (1)
m_db(126-128)
src/test/util/setup_common.cpp (1)
src/node/chainstate.cpp (2)
DashChainstateSetup(220-261)DashChainstateSetup(220-236)
src/llmq/quorums.h (1)
src/llmq/quorums.cpp (2)
CQuorumManager(210-230)CQuorumManager(232-235)
🪛 GitHub Actions: Clang Diff Format Check
src/dsnotificationinterface.cpp
[error] 1-1: Clang-format differences detected in this file.
src/llmq/quorums.cpp
[error] 1-1: Clang-format differences detected in this file.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build container / Build container
🔇 Additional comments (72)
contrib/containers/ci/ci-slim.Dockerfile (1)
109-109: Smart guard against existing GIDs — LGTM!The conditional check
getent group ${GROUP_ID} ||gracefully handles cases where the target GID already exists (common on macOS or in certain container environments), avoiding groupmod failures and the naming conflicts described in the PR. This approach is idempotent and aligns well with the macOS/Linux GID handling goal.src/bls/bls.h (1)
597-597: LGTM: Return type change accurately reflects function behavior.Changing
BLSInit()frombooltovoidis appropriate since the function only callsSetSecureAllocator()(which is void) and has no failure mode to report.src/bls/bls.cpp (2)
425-425: Good practice: #endif directive comments improve readability.Adding
// BUILD_BITCOIN_INTERNALcomments to#endifdirectives helps track preprocessor block boundaries, especially in files with multiple conditional compilation blocks.Also applies to: 431-431
427-432: LGTM: Implementation correctly matches the new signature.The function implementation is straightforward and correctly returns
voidsinceSetSecureAllocator()has no return value or failure mode to propagate.src/masternode/sync.h (1)
56-60: LGTM! Proper enforcement of non-copyable semantics and explicit lifetime management.These changes correctly enforce that
CMasternodeSyncmust be explicitly constructed with its dependencies and cannot be copied. This is appropriate because:
- The class holds reference members (
connman,m_netfulfilledman) that cannot be rebound after initialization- As a manager/synchronization component, copying would be semantically incorrect
- The explicit destructor declaration (defaulted in the .cpp) follows the rule of five
src/evo/specialtxman.cpp (2)
192-205: LGTM: Const-correctness improvement.The named argument
/*onlyValid=*/falseenhances readability, and theconst auto&lambda parameter correctly enforces that the iterated masternode is read-only within this iteration. The lambda only reads fromdmnand updates the new list vianewList.UpdateMN().
476-487: LGTM: Const-correctness improvement.The named argument and
const auto¶meter correctly enforce read-only iteration semantics. The lambda only reads masternode properties and updates the list vianewList.UpdateMN(), making the const qualifier appropriate.src/governance/governance.h (1)
276-278: LGTM! Correctly enforces non-copyable, non-default-constructible semantics.Deleting the default constructor, copy constructor, and copy assignment operator ensures
CGovernanceManagercan only be constructed via the explicit parameterized constructor. This prevents resource management issues and accidental copying of a manager class that holds aunique_ptrto the database and references to other subsystems.src/governance/signing.h (1)
62-64: LGTM! Correctly enforces non-copyable, non-default-constructible semantics.Deleting the default constructor, copy constructor, and copy assignment operator ensures
GovernanceSignercan only be constructed via the explicit parameterized constructor. This is essential because the class holds multiple member references that must be initialized at construction time and cannot be safely copied or default-constructed.src/evo/smldiff.cpp (2)
128-128: LGTM! Named parameter improves readability.The explicit
/*onlyValid=*/falsecomment makes the boolean argument self-documenting. The value is correct for building a complete diff that includes all masternodes.
143-143: LGTM! Consistent named parameter usage.The explicit
/*onlyValid=*/falsecomment is consistent with line 128 and correctly indicates that all masternodes should be checked when building the diff.src/rpc/governance.cpp (1)
569-569: LGTM! Named parameter clarifies voting logic.The explicit
/*onlyValid=*/truecomment makes it clear that only valid masternodes are considered for voting, which is the correct behavior for governance voting.src/masternode/node.h (1)
54-58: LGTM: Enforces proper lifetime management.Deleting the default constructor, copy constructor, and copy assignment operator is appropriate for this class, which holds references (
m_connman,m_dmnman) that cannot be rebound and containsconstmembers inm_info. The explicit destructor declaration with implementation in the .cpp file follows good practice for controlling destructor instantiation.Note: Move operations are implicitly deleted when copy operations are explicitly deleted per C++ rules, which is correct for this class design.
src/masternode/node.cpp (2)
52-54: LGTM: Brace initialization follows modern C++ style.The change from parentheses to brace initialization for
m_infois a stylistic improvement that aligns with modern C++20 conventions while maintaining identical semantics.
64-64: LGTM: Destructor correctly defined.The defaulted destructor implementation in the .cpp file matches the declaration in the header and ensures proper destruction semantics.
src/rpc/blockchain.cpp (1)
1465-1492: LGTM! Documentation accurately reflects the dynamic softfork structure.The help text correctly documents the restructured softfork output format. Verified that:
- OBJ_DYN usage is appropriate for dynamic deployment name keys
- All fields match the actual implementation in
SoftForkDescPushBackfunctions (lines 1371-1439)- Field types, descriptions, and optionality are accurate
- The distinction between "buried" and "bip9" deployment types is clear
The past review concern about PR scope was already addressed by the author as a minor alignment change beneficial for RPCResult validation.
src/masternode/meta.cpp (1)
18-22: LGTM! Destructor relocation improves code organization.Moving the destructor immediately after the constructor follows C++ conventions for grouping special member functions together, improving readability with no functional change.
src/spork.h (1)
236-237: LGTM! Non-copyable semantics are appropriate.Deleting copy operations is correct for
CSporkManagersince it manages aunique_ptr<db_type>member (line 216) and has destructor logic that persists state. Copying would cause resource management issues.src/bls/bls_worker.h (1)
52-53: LGTM! Non-copyable semantics are appropriate.Deleting copy operations is correct for
CBLSWorkersince it manages a thread pool (line 28) and worker threads. Copying such resources would be unsafe.src/evo/mnhftx.h (1)
102-104: LGTM! Non-copyable and non-default-constructible semantics are appropriate.The deletions are correct for
CMNHFManager:
- Default constructor deletion enforces explicit initialization with a required
CEvoDB&reference (line 92)- Copy operation deletions are appropriate for a manager class with reference members and singleton-like behavior
src/llmq/signing_shares.h (1)
419-421: LGTM! Non-copyable and non-default-constructible semantics are appropriate.The deletions are correct for
CSigSharesManager:
- Default constructor deletion enforces explicit initialization with required dependencies (lines 422-424)
- Copy operation deletions are appropriate for a manager with worker threads (line 381), mutexes, and reference members
src/llmq/snapshot.h (1)
226-230: LGTM! Non-copyable and non-default-constructible semantics are appropriate.The deletions and explicit constructor/destructor declarations are correct for
CQuorumSnapshotManager:
- Default constructor deletion enforces explicit initialization with a required
CEvoDB&reference (line 221)- Copy operation deletions are appropriate for a manager class with reference members
src/evo/chainhelper.h (1)
39-41: LGTM! Non-copyable and non-default-constructible semantics are appropriate.The deletions are correct for
CChainstateHelper:
- Default constructor deletion enforces explicit initialization with required dependencies (lines 42-47)
- Copy operation deletions are appropriate for a class with reference members (lines 35-36) and
unique_ptrmembers (lines 62-63)src/netfulfilledman.h (2)
53-54: LGTM! Non-copyable semantics are appropriate.Deleting copy operations is correct for
CNetFulfilledRequestManagersince it manages aunique_ptr<db_type>member (line 49) and has destructor logic that persists state.
55-55: The explicit keyword removal is intentional and poses minimal practical risk, but is inconsistent with the refactoring's intent.The git history confirms the
explicitkeyword was removed in commit ba194eb as part of refactoring to "delete extra ctors." While the web search confirms that removingexplicitfrom a zero-arg constructor theoretically enables implicit copy-initialization, the practical risk here is negated by:
- Deleted copy constructor (line 53) prevents copy-initialization entirely
- All instantiations use direct initialization via
std::make_unique<CNetFulfilledRequestManager>()in init.cpp and setup_common.cpp, which work identically with or withoutexplicit- No implicit constructions found in the codebase
However, removing
explicitis slightly inconsistent with the refactoring's purpose of explicitly controlling construction (by deleting copy operations). From a C++ best practices perspective, retainingexplicitwould be clearer about intent, even though it provides no functional difference here.src/dbwrapper.h (1)
737-740: Clarify the empty path for in-memory databases.The factory currently passes an empty string when
params.memoryis true. While this matches the existing behavior indevelop(as noted in past discussions), eachCDBWrappercreates its ownMemEnvso there's no cross-instance collision. However, passingparams.pathconsistently (relying on thememoryflag for in-memory behavior) would preserve namespace information and improve clarity.Based on past review discussions, this behavior is intentional and matches
develop. If you'd like to verify this is the desired approach or consider passingparams.pathin all cases, please confirm.src/evo/creditpool.cpp (1)
241-246: LGTM!The switch to brace initialization and the explicit defaulted destructor align with modern C++ practices and the broader lifecycle management improvements across the codebase.
src/dsnotificationinterface.cpp (2)
30-40: LGTM!The switch to brace initialization and explicit defaulted destructor align with the project-wide modernization and lifecycle management improvements.
42-46: Verify the addition of UpdatedBlockTip call during initialization.Line 45 now calls
UpdatedBlockTipin addition toSynchronousUpdatedBlockTip. This changes initialization behavior by invoking both the synchronous and asynchronous update paths.Please confirm this is intentional and that dependent subsystems (LLMQ, governance, etc.) expect both calls during initialization.
src/netfulfilledman.cpp (1)
15-19: LGTM!The destructor correctly persists state when valid, following RAII principles for proper resource cleanup.
src/llmq/blockprocessor.h (1)
58-63: LGTM!Deleting the default constructor and copy semantics prevents misuse and ensures
CQuorumBlockProcessoris constructed only with required dependencies. This aligns with the broader lifecycle management improvements across the codebase.src/masternode/meta.h (1)
237-240: LGTM!Deleting copy semantics while adding an explicit default constructor ensures
CMasternodeMetaManinstances are non-copyable and constructed explicitly. This prevents accidental copies and aligns with the project-wide lifecycle management improvements.src/coinjoin/coinjoin.h (2)
334-335: LGTM!The explicit constructor and virtual destructor properly document the lifecycle of this base class and enable correct polymorphic deletion.
375-378: LGTM!Deleting copy semantics and adding explicit constructor/destructor ensures proper lifecycle management and prevents accidental copies of
CDSTXManager.src/evo/creditpool.h (1)
126-130: LGTM!Deleting default and copy semantics ensures
CCreditPoolManageris constructed only with required dependencies and cannot be copied. The user-declared destructor (defaulted in the implementation) follows the Rule of Five and aligns with the broader lifecycle management improvements.src/instantsend/instantsend.cpp (1)
61-63: Ctor modernization LGTMPassing util::DbWrapperParams through and delegating DB init to CInstantSendDb is clean and consistent.
src/chainlock/signing.h (1)
66-68: Delete default/copy ctor: good callPrevents accidental misuse with reference members; destructor is defined in signing.cpp.
src/coinjoin/server.cpp (1)
26-46: New ctor wiring LGTMDependency-injected references look correct; defaulted dtor is fine.
src/dsnotificationinterface.h (1)
21-23: Non-default-constructible and non-copyable: goodMatches the reference-member design; dtor is virtual and defined in .cpp.
Also applies to: 31-31
src/instantsend/db.cpp (1)
29-33: DbWrapperParams adoption LGTMInitialization + conditional Upgrade preserve behavior and centralize DB config.
Also applies to: 37-45
src/instantsend/instantsend.h (1)
34-36: API shape and ctor change LGTMConstructor now cleanly consumes DbWrapperParams; deleted default/copy ctors are appropriate.
Also applies to: 95-101
src/llmq/dkgsessionmgr.cpp (1)
36-45: No action required—CDKGSessionHandler does not expect DbWrapperParams.The handler's constructor does not include
DbWrapperParamsand contains no references todb_paramsorDbWrapperParamsanywhere in its implementation. The manager owns its own database via theDbWrapperParamspassed to its constructor, while the handler receives the correct subset of parameters via theemplacecall. The code is correct as written.src/node/chainstate.h (1)
31-33: Forward-decl of fs::path is correct and safeThe forward declaration is properly designed. fs::path is used only as a const reference parameter in function signatures, and the full definition is transitively available in the implementation through validation.h and node/blockstorage.h (both include fs.h). No namespace aliasing conflicts exist—fs is a custom wrapper in fs.h. All call sites have been updated with the new data_dir parameter.
src/evo/evodb.cpp (2)
42-42: LGTM: defaulted destructor.No issues.
61-61: LGTM: use unique_ptr deref for WriteBatch.Switch to
db->WriteBatch(rootBatch)is correct.src/llmq/snapshot.cpp (1)
339-346: LGTM: ctor/dtor and cache wiring.Constructor wiring (
m_evoDb,quorumSnapshotCache{32}) and defaulted dtor look good.src/masternode/active/context.h (1)
45-49: LGTM: stricter lifecycle (deleted assignment + explicit ctor).Good step toward making construction explicit and preventing accidental copies.
src/node/chainstate.cpp (2)
51-52: LGTM: centralize Evo/LLMQ DB construction via DbWrapperParams.Parameter threading and EvoDB construction align with the in‑memory/wipe semantics.
Also applies to: 66-66, 76-79, 92-95
251-252: Related: also dereferenced here.
*mempoolis used to constructLLMQContext; the guard above covers this path.src/llmq/context.cpp (1)
24-37: Member declaration order is correct; no undefined behavior risk.The header declares members in dependency order:
qsnapman→quorum_block_processor→qdkgsman→qman→sigman→clhandler→isman. Each member is declared after all members it references in its initializer, ensuring safe initialization.src/llmq/quorums.h (1)
40-42: Forward declaration is appropriate here.Pointer-only use of util::DbWrapperParams justifies a forward declaration; avoids unnecessary header coupling.
src/init.cpp (2)
2361-2364: Const-correct ForEachMN capture: good.Passing each dmn as const ref prevents accidental mutation and aligns with intent.
1984-2001: All LoadChainstate callsites verified and properly updated.The two callsites (production: src/init.cpp:1970; tests: src/test/util/setup_common.cpp:306) both correctly pass
args.GetDataDirNet()anddash_dbs_in_memorywith appropriate values (false for production, true for tests). Parameter lists match the function signature across declaration and definition, and implementation correctly uses these parameters via DbWrapperParams for database initialization.src/test/util/setup_common.cpp (5)
126-136: Test wrapper now forwards in-memory flags: LGTM.Forwarding llmq_dbs_in_memory/llmq_dbs_wipe and data_dir keeps tests isolated and fast.
193-207: Chain interface and socket events initialization in tests: LGTM.Interfaces::MakeChain wiring and SEM parsing are appropriate for the test harness.
219-225: In-memory EvoDB with wipe=true for tests: good choice.This avoids disk I/O and stale state across runs.
291-296: Teardown sequence continues to be explicit: OK.Resetting scheduler-dependent globals and pointers avoids test flakiness.
320-336: The review comment's verification claim is incorrect.The review comment assumes
llmq_dbs_wipe=trueis set through the LoadChainstate-to-DashChainstateSetup path in tests. However, the actual implementation shows:
fReindexis initialized tofalse(src/node/blockstorage.cpp:28)- In TestChainSetup, LoadChainstate is called with
fReindex.load()as the fReset parameterllmq_dbs_wipeis computed asfReset || fReindexChainState(src/node/chainstate.cpp:94)- Both conditions default to
false, resulting inllmq_dbs_wipe=false, nottrueThe test code in setup_common.cpp (lines 320-336) is correctly configured with all DBs in-memory. The code snippet itself requires no changes.
Likely an incorrect or invalid review comment.
src/llmq/signing.cpp (1)
335-344: Constructor signature update verified; all call sites correctly updated.The only
CSigningManagerinstantiation is atsrc/llmq/context.cpp:33usingstd::make_unique<llmq::CSigningManager>(chainman.ActiveChainstate(), *qman, db_params), which correctly matches the new signature withDbWrapperParams. No lingeringfMemory/fWipepatterns in llmq code. API change is consistent and complete.src/llmq/quorums.cpp (3)
214-226: LGTM! Constructor refactored to use DbWrapperParams.The constructor signature has been successfully updated to accept
util::DbWrapperParamsinstead of separate unit test and wipe flags. The use ofutil::MakeDbWrapperand brace-initialization for all members aligns with the PR's goal of centralizing DB construction and improving object lifetime management.
232-235: LGTM! Destructor formatting updated.The destructor has been reformatted from single-line to multi-line while maintaining the same functionality (
Stop()is still invoked).
686-687: LGTM! Improved const-correctness in ForEachMN callback.The lambda parameter has been changed from
auto&toconst auto&and an explicit named parameter comment/*onlyValid=*/truehas been added. This improves const-correctness and code clarity, supporting future refactors as noted in the PR objectives.src/llmq/dkgsessionmgr.h (2)
36-38: LGTM! Forward declaration added for DbWrapperParams.The forward declaration is correctly placed and necessary for the constructor parameter type.
84-91: LGTM! Tightened object lifetime management.The deletion of default and copy constructors prevents accidental misuse, while the explicit constructor enforces proper initialization with
util::DbWrapperParams. This aligns with the PR's goal of centralizing DB construction and improving object lifetime management.src/coinjoin/client.cpp (5)
41-50: LGTM! CCoinJoinClientQueueManager constructor and destructor implemented.The constructor uses brace-initialization for member initialization and the destructor is properly defaulted. This aligns with the PR's goal of improved lifecycle management.
153-158: LGTM! DoMaintenance implementation added.The method properly checks sync status before calling
CheckQueue(), providing appropriate early exit conditions.
160-173: LGTM! CCoinJoinClientManager constructor and destructor implemented.The constructor uses brace-initialization for all members and the destructor is properly defaulted. This follows the pattern established throughout the PR for improved lifecycle management.
1051-1052: LGTM! Improved const-correctness in ForEachMNShared callback.The explicit named parameter comment
/*onlyValid=*/trueimproves code clarity, and the lambda parameter is appropriately const.
1901-1921: LGTM! CoinJoinWalletManager constructor and destructor implemented.The constructor uses brace-initialization for member initialization. The destructor properly locks the mutex and resets all managed pointers, ensuring clean shutdown of wallet managers.
src/coinjoin/client.h (3)
78-85: LGTM! CoinJoinWalletManager lifecycle management tightened.The deletion of default and copy constructors prevents accidental misuse, while the explicit constructor and destructor declarations enforce proper initialization and cleanup. This aligns with the PR's goal of improved object lifetime management.
228-233: LGTM! CCoinJoinClientQueueManager lifecycle management tightened.The deletion of default and copy constructors prevents accidental misuse, while the explicit constructor and destructor declarations enforce proper initialization and cleanup.
279-285: LGTM! CCoinJoinClientManager lifecycle management tightened.The deletion of copy constructor and copy assignment prevents accidental copying, and the destructor declaration enables proper cleanup.
kwvg
changed the title
EvoDb and LLMQContext and other misc. fixesEvoDb and LLMQContext
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/llmq/signing.cpp (1)
265-266: Logging format bug: %d used with func.tfm::format expects %s for const char*. Use %s for func.
-LogPrint(BCLog::LLMQ, "CRecoveredSigsDb::%d -- deleted %d entries\n", __func__, toDelete.size()); +LogPrint(BCLog::LLMQ, "CRecoveredSigsDb::%s -- deleted %d entries\n", __func__, toDelete.size()); -LogPrint(BCLog::LLMQ, "CRecoveredSigsDb::%d -- deleted %d entries\n", __func__, cnt); +LogPrint(BCLog::LLMQ, "CRecoveredSigsDb::%s -- deleted %d entries\n", __func__, cnt);Also applies to: 330-331
♻️ Duplicate comments (2)
src/node/chainstate.h (1)
96-97: Consolidate boolean flags into an options struct.Continuing to add booleans increases error risk at call sites. Prefer a small struct (e.g., ChainstateDbOptions) or reuse util::DbWrapperParams derivatives for related flags.
To spot fragile call sites:
#!/bin/bash rg -nP '\bLoadChainstate\s*\(' -C2 -- src rg -nP '\bDashChainstateSetup\s*\(' -C2 -- srcAlso applies to: 129-132
src/node/chainstate.cpp (1)
236-256: Guard mempool before dereference in DashChainstateSetup.mempool is a raw pointer and dereferenced below (ConnectManagers). Add a non‑fatal assert.
void DashChainstateSetup(..., CTxMemPool* mempool, const fs::path& data_dir, bool llmq_dbs_in_memory, bool llmq_dbs_wipe, const Consensus::Params& consensus_params) { + CHECK_NONFATAL(mempool != nullptr);
🧹 Nitpick comments (8)
src/evo/evodb.h (2)
99-103: Expose const overload for GetRawDB.Many callers only need read access. Add a const overload to improve const-correctness.
- CDBWrapper& GetRawDB() - { - return *db; - } + CDBWrapper& GetRawDB() { return *db; } + const CDBWrapper& GetRawDB() const { return *db; }
111-112: Make IsEmpty const and [[nodiscard]].It’s a pure query and should be usable on const instances; [[nodiscard]] helps avoid accidental drops of the result.
- bool IsEmpty() { return db->IsEmpty(); } + [[nodiscard]] bool IsEmpty() const { return db->IsEmpty(); }src/dbwrapper.h (1)
721-724: Guard against empty path even for in‑memory DBs.LevelDB MemEnv still namespaces by path string. Add a debug assert to avoid accidental collisions from empty paths.
static inline std::unique_ptr<CDBWrapper> MakeDbWrapper(const DbWrapperParams& params) { - return std::make_unique<CDBWrapper>(params.path, params.cache_size, params.memory, params.wipe); + assert(!params.path.empty()); // ensure unique namespace even in MemEnv + return std::make_unique<CDBWrapper>(params.path, params.cache_size, params.memory, params.wipe); }src/evo/evodb.cpp (1)
56-63: CommitRootTransaction uses db->WriteBatch; good—consider [[nodiscard]].Return value is already bool; optionally mark [[nodiscard]] on the declaration to avoid ignoring failures.
src/llmq/dkgsessionmgr.cpp (1)
393-405: CleanupOldContributions: iterate/remove pattern is fine; minor note on lock scope.You lock cs_main inside the LevelDB iteration. If heavy chains or slow disks appear, consider minimizing cs_main hold time by caching heights or batching keys before acquiring cs_main. Not urgent.
src/node/chainstate.cpp (1)
76-78: Redundant reset() before assignment.unique_ptr assignment resets old value automatically. Drop the explicit evodb.reset().
- evodb.reset(); - evodb = std::make_unique<CEvoDB>(util::DbWrapperParams{.path = data_dir, .memory = dash_dbs_in_memory, .wipe = fReset || fReindexChainState}); + evodb = std::make_unique<CEvoDB>(util::DbWrapperParams{.path = data_dir, .memory = dash_dbs_in_memory, .wipe = fReset || fReindexChainState});src/llmq/quorums.h (1)
265-273: Ctor signature aligned with unified DbWrapperParams.Looks correct. If you internally store/copy params, consider taking by value and move for symmetry with other managers.
Are you storing a copy of DbWrapperParams or just using it to build the DB wrapper?
src/init.cpp (1)
2359-2365: Const‑correct the masternode loop variable.The loop doesn’t mutate dmn; prefer const auto& to avoid accidental copies/changes.
- mnList.ForEachMN(false, [&](auto& dmn) { + mnList.ForEachMN(false, [&](const auto& dmn) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/dbwrapper.h(1 hunks)src/evo/evodb.cpp(2 hunks)src/evo/evodb.h(5 hunks)src/init.cpp(2 hunks)src/instantsend/db.cpp(1 hunks)src/instantsend/db.h(2 hunks)src/instantsend/instantsend.cpp(1 hunks)src/instantsend/instantsend.h(2 hunks)src/llmq/context.cpp(1 hunks)src/llmq/context.h(1 hunks)src/llmq/dkgsessionmgr.cpp(1 hunks)src/llmq/dkgsessionmgr.h(2 hunks)src/llmq/quorums.cpp(1 hunks)src/llmq/quorums.h(2 hunks)src/llmq/signing.cpp(2 hunks)src/llmq/signing.h(3 hunks)src/node/chainstate.cpp(6 hunks)src/node/chainstate.h(4 hunks)src/test/util/setup_common.cpp(7 hunks)src/test/util/setup_common.h(1 hunks)src/test/validation_chainstatemanager_tests.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/llmq/dkgsessionmgr.h
- src/test/validation_chainstatemanager_tests.cpp
- src/instantsend/db.h
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/test/util/setup_common.hsrc/llmq/quorums.hsrc/llmq/dkgsessionmgr.cppsrc/llmq/context.cppsrc/instantsend/db.cppsrc/llmq/context.hsrc/instantsend/instantsend.hsrc/node/chainstate.hsrc/evo/evodb.cppsrc/node/chainstate.cppsrc/llmq/signing.cppsrc/test/util/setup_common.cppsrc/llmq/quorums.cppsrc/evo/evodb.hsrc/llmq/signing.hsrc/dbwrapper.hsrc/instantsend/instantsend.cppsrc/init.cpp
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/util/setup_common.hsrc/test/util/setup_common.cpp
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/evodb.cppsrc/evo/evodb.h
🧠 Learnings (3)
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
PR: dashpay/dash#6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/llmq/context.cppsrc/llmq/context.hsrc/node/chainstate.cpp
📚 Learning: 2025-01-02T21:50:00.967Z
Learnt from: kwvg
PR: dashpay/dash#6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
Applied to files:
src/llmq/context.h
📚 Learning: 2024-12-29T17:43:41.755Z
Learnt from: kwvg
PR: dashpay/dash#6504
File: src/llmq/quorums.cpp:224-224
Timestamp: 2024-12-29T17:43:41.755Z
Learning: The `CQuorumManager` is fully initialized by `LLMQContext`, addressing any concerns about the manager’s initialization sequence.
Applied to files:
src/llmq/context.h
🧬 Code graph analysis (12)
src/llmq/quorums.h (1)
src/llmq/quorums.cpp (2)
CQuorumManager(210-230)CQuorumManager(232-235)
src/instantsend/db.cpp (1)
src/instantsend/db.h (4)
CInstantSendDb(32-163)cs_db(39-39)cs_db(41-162)CURRENT_VERSION(37-37)
src/llmq/context.h (1)
src/llmq/context.cpp (2)
LLMQContext(18-40)LLMQContext(42-44)
src/instantsend/instantsend.h (1)
src/instantsend/instantsend.cpp (2)
CInstantSendManager(59-72)CInstantSendManager(74-74)
src/node/chainstate.h (1)
src/fs.h (1)
path(42-42)
src/evo/evodb.cpp (1)
src/evo/evodb.h (1)
CEvoDB(38-76)
src/llmq/signing.cpp (1)
src/llmq/signing.h (2)
CRecoveredSigsDb(119-285)CSigningManager(165-251)
src/test/util/setup_common.cpp (1)
src/node/chainstate.cpp (2)
DashChainstateSetup(220-261)DashChainstateSetup(220-236)
src/llmq/quorums.cpp (1)
src/llmq/utils.cpp (8)
InitQuorumsCache(962-968)InitQuorumsCache(962-962)InitQuorumsCache(969-970)InitQuorumsCache(971-972)InitQuorumsCache(973-978)InitQuorumsCache(979-980)InitQuorumsCache(981-982)InitQuorumsCache(983-984)
src/evo/evodb.h (2)
src/dbwrapper.h (1)
CDBWrapper(214-534)src/evo/evodb.cpp (2)
CEvoDB(34-40)CEvoDB(42-42)
src/llmq/signing.h (1)
src/llmq/signing.cpp (4)
CRecoveredSigsDb(30-33)CRecoveredSigsDb(35-35)CSigningManager(335-341)CSigningManager(343-343)
src/dbwrapper.h (8)
src/evo/evodb.h (1)
util(12-14)src/instantsend/db.h (1)
util(27-29)src/instantsend/instantsend.h (1)
util(34-36)src/llmq/context.h (1)
util(32-34)src/llmq/dkgsessionmgr.h (1)
util(36-38)src/llmq/quorums.h (1)
util(40-42)src/llmq/signing.h (1)
util(35-37)src/dbwrapper.cpp (2)
CDBWrapper(129-183)CDBWrapper(185-197)
🪛 GitHub Actions: Clang Diff Format Check
src/llmq/quorums.cpp
[error] 1-1: Clang-format differences detected in this file. Run the formatting script to fix: differences shown by clang-format-diff.py.
src/evo/evodb.h
[error] 1-1: Clang-format differences detected in this file. Run the formatting script to fix: differences shown by clang-format-diff.py.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build container / Build container
🔇 Additional comments (26)
src/evo/evodb.h (4)
12-15: Forward-decl for util::DbWrapperParams is correct.
43-43: Good: switch to unique_ptr clarifies ownership.
53-57: Non-copyable CEvoDB—good call.Prevents accidental slicing/copies of transactional state.
1-123: No formatting issues found. Review comment is incorrect.The git working tree is clean with no uncommitted changes to
src/evo/evodb.h. Thegit diff -U0command produces no output, confirming there are no formatting differences to address. The original clang-format-diff script produced no output because there are no changes in the diff, not because of an error. The file is already properly formatted.src/node/chainstate.h (1)
31-33: Forward-declare fs::path—good to keep headers light.src/dbwrapper.h (1)
712-720: Centralizing DB params is a solid improvement.DbWrapperParams with a factory removes repetitive constructor calls and clarifies intent.
src/instantsend/instantsend.h (2)
34-36: Forward-decl for DbWrapperParams keeps header slim—nice.
95-101: Ctor migration to DbWrapperParams—verified all construction sites pass base path correctly.Confirmed only instantiation site in
src/node/chainstate.cpp:250-252correctly passesutil::DbWrapperParams{.path = data_dir, ...}(base path). CInstantSendDb constructor properly appends "llmq/isdb" to the path. Migration is sound.src/instantsend/db.cpp (2)
29-33: Constructor: consistent pathing and cache size; good.Using db_params.path / "llmq"/"isdb" with MakeDbWrapper and 32 MiB cache is clear and consistent.
37-45: Upgrade path preserves conditional wipe—LGTM.Version check gates the wipe/reinit; behavior matches prior code while using the new factory.
src/evo/evodb.cpp (1)
34-38: Ctor wiring via DbWrapperParams—LGTM.Consistent base path "evodb" and fixed 64 MiB cache; transactions initialized correctly.
src/llmq/dkgsessionmgr.cpp (1)
36-45: DB wiring via DbWrapperParams—LGTM.Scoped 8 MiB cache and consistent "llmq/dkgdb" pathing is clear.
src/llmq/context.cpp (1)
21-37: Param forwarding looks good; ensure callees don’t “move” from const&.Forwarding db_params to qdkgsman, qman, sigman, isman is correct. Please confirm those constructors either take DbWrapperParams by value (preferred) and move, or by const& without std::move; avoid std::move on a const& (ineffective). I’ve left concrete fixes in their files.
src/llmq/signing.h (1)
35-38: Good forward declaration to decouple headers.src/llmq/context.h (2)
31-34: Clean namespacing and forward-declare util::DbWrapperParams.
40-44: Making LLMQContext non-copyable is appropriate.Explicitly deleting assignment avoids accidental copies; ctor signature aligns with new db_params flow.
src/node/chainstate.cpp (2)
76-78: Good: centralizing EvoDB construction via DbWrapperParams.
92-95: Propagate DbWrapperParams into LLMQContext — nice unification.src/llmq/quorums.h (1)
40-43: Good forward declaration to avoid heavy includes.src/init.cpp (1)
1984-2000: Correctly passing data_dir and in‑memory flag into LoadChainstate.src/llmq/quorums.cpp (1)
232-235: LGTM!The destructor formatting change from single-line to multi-line is consistent with out-of-line definitions. No functional change.
src/test/util/setup_common.cpp (5)
126-136: LGTM!The parameter renaming from
fReset/fReindexChainStatetollmq_dbs_in_memory/llmq_dbs_wipesignificantly improves clarity. The addition ofnode.args->GetDataDirNet()properly supports theDbWrapperParamspattern introduced in this PR.
219-224: LGTM!The initialization order changes and use of
util::DbWrapperParamswith designated initializers (.path,.memory,.wipe) are correct. The updatedevodbinitialization properly supports memory-only databases for testing, aligning with the PR's performance improvement goals.Based on past review comments, the initialization order changes are intentional preparation for future refactoring.
238-253: LGTM!The destructor cleanup ordering is appropriate, resetting components in reverse initialization order. Based on past review discussion, this explicit ordering matches upstream patterns and ensures proper dependency cleanup.
290-295: LGTM!The destructor cleanup ordering properly reverses the initialization sequence, ensuring dependencies are torn down correctly.
320-320: LGTM!The addition of
Assert()wrapper andGetDataDirNet()call at line 320 properly provides the network-specific data directory. Thedash_dbs_in_memoryparameter at line 335 aligns with the broader refactoring to support memory-only databases for improved test performance.Also applies to: 334-335
| const CSporkManager& sporkman, const util::DbWrapperParams& db_params) : | ||
| db{util::MakeDbWrapper( | ||
| {db_params.path / "llmq" / "quorumdb", db_params.memory, db_params.wipe, /*cache_size=*/1 << 20})}, | ||
| blsWorker{_blsWorker}, | ||
| m_chainstate{chainstate}, | ||
| m_dmnman{dmnman}, | ||
| dkgManager{_dkgManager}, | ||
| quorumBlockProcessor{_quorumBlockProcessor}, | ||
| m_qsnapman{qsnapman}, | ||
| m_mn_activeman{mn_activeman}, | ||
| m_mn_sync{mn_sync}, | ||
| m_sporkman{sporkman} |
There was a problem hiding this comment.
@kwvg, thank you for resolving the clang-format issue! 🎉
| void DashChainstateSetup(ChainstateManager& chainman, | ||
| node::NodeContext& node, | ||
| bool fReset, | ||
| bool fReindexChainState, | ||
| bool llmq_dbs_in_memory, | ||
| bool llmq_dbs_wipe, | ||
| const Consensus::Params& consensus_params); |
There was a problem hiding this comment.
🧩 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 -- srcLength 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 -30Length 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.
| const CSporkManager& sporkman, const util::DbWrapperParams& db_params) : | ||
| db{util::MakeDbWrapper( | ||
| {db_params.path / "llmq" / "quorumdb", db_params.memory, db_params.wipe, /*cache_size=*/1 << 20})}, | ||
| blsWorker{_blsWorker}, | ||
| m_chainstate{chainstate}, | ||
| m_dmnman{dmnman}, | ||
| dkgManager{_dkgManager}, | ||
| quorumBlockProcessor{_quorumBlockProcessor}, | ||
| m_qsnapman{qsnapman}, | ||
| m_mn_activeman{mn_activeman}, | ||
| m_mn_sync{mn_sync}, | ||
| m_sporkman{sporkman} |
There was a problem hiding this comment.
@kwvg, thank you for resolving the clang-format issue! 🎉
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/llmq/dkgsessionmgr.cpp (1)
428-433: Fix printf specifiers for size_t in logs.cnt_all/cnt_old are size_t but logged with %lld. Use %zu or cast explicitly to avoid UB on some platforms.
Apply this diff:
- LogPrint(BCLog::LLMQ, "CDKGSessionManager::%s -- found %lld entries for llmq type %d\n", __func__, cnt_all, uint8_t(params.type)); + LogPrint(BCLog::LLMQ, "CDKGSessionManager::%s -- found %zu entries for llmq type %d\n", __func__, cnt_all, uint8_t(params.type)); @@ - LogPrint(BCLog::LLMQ, "CDKGSessionManager::%s -- removed %lld old entries for llmq type %d\n", __func__, cnt_old, uint8_t(params.type)); + LogPrint(BCLog::LLMQ, "CDKGSessionManager::%s -- removed %zu old entries for llmq type %d\n", __func__, cnt_old, uint8_t(params.type));src/llmq/signing.cpp (2)
265-266: Fix format specifiers: func uses %s; size_t uses %zu.Current line uses "%d" for func and size_t; switch to "%s" and "%zu".
Apply this diff:
- LogPrint(BCLog::LLMQ, "CRecoveredSigsDb::%d -- deleted %d entries\n", __func__, toDelete.size()); + LogPrint(BCLog::LLMQ, "CRecoveredSigsDb::%s -- deleted %zu entries\n", __func__, toDelete.size());
330-331: Same format-specifier bug in CleanupOldVotes.Use "%s" for func and "%zu" for cnt.
- LogPrint(BCLog::LLMQ, "CRecoveredSigsDb::%d -- deleted %d entries\n", __func__, cnt); + LogPrint(BCLog::LLMQ, "CRecoveredSigsDb::%s -- deleted %zu entries\n", __func__, cnt);
♻️ Duplicate comments (4)
src/node/chainstate.h (1)
96-97: Consolidate repeated boolean flags into an options structGroup related flags (prune/index toggles, in-memory/wipe) into a config struct to de-risk call sites and improve readability. Applicable to both
LoadChainstateandDashChainstateSetup.Also applies to: 111-112, 129-132
src/instantsend/db.cpp (1)
29-33: Note on Upgrade(wipe=true) call — resolution acknowledgedThe unconditional wipe=true passed here is used conditionally inside Upgrade() when version is outdated; behavior matches develop and is intentional. No action.
src/node/chainstate.cpp (1)
220-261: Guard mempool before dereference in DashChainstateSetup.mempool is a raw pointer and is dereferenced (Line 253). Add a non-fatal assert to avoid accidental null deref in tests or future paths.
Apply near function start:
void DashChainstateSetup(ChainstateManager& chainman, @@ const Consensus::Params& consensus_params) { + CHECK_NONFATAL(mempool != nullptr);Based on learnings.
src/llmq/quorums.cpp (1)
214-225: Fix clang-format differences for this constructor initializer list.Run project formatter (same as pipeline):
#!/bin/bash git diff -U0 HEAD^ | contrib/devtools/clang-format-diff.py -p1 -i
🧹 Nitpick comments (11)
src/instantsend/db.h (1)
27-29: DbWrapperParams adoption + locking: LGTM; tiny ctor consistency nitForward declaration and lock annotations look correct. Optional: for consistency with callers that pass a moved params object, consider taking
DbWrapperParamsby value here and moving internally (or drop the move at the call site).Apply (header side):
- explicit CInstantSendDb(const util::DbWrapperParams& db_params); + explicit CInstantSendDb(util::DbWrapperParams db_params);And update the definition in db.cpp accordingly:
- CInstantSendDb::CInstantSendDb(const util::DbWrapperParams& db_params) : + CInstantSendDb::CInstantSendDb(util::DbWrapperParams db_params) :Also applies to: 84-84, 87-87
src/test/util/setup_common.h (1)
90-92: Avoid boolean-parameter soup—group LLMQ DB flagsConsider a small options struct (e.g.,
LlmqDbOptions{bool in_memory; bool wipe;}) or reuseutil::DbWrapperParamsto carry these flags. Reduces call-site mistakes and improves readability.src/instantsend/instantsend.cpp (1)
61-63:std::movehere is effectively a no-op; prefer claritySince
CInstantSendDbtakesDbWrapperParamsby const&, the move won’t transfer anything. Either change that ctor to take by value (and move) or drop the move here.Minimal local tweak:
- const CMasternodeSync& mn_sync, util::DbWrapperParams db_params) : - db{std::move(db_params)}, + const CMasternodeSync& mn_sync, util::DbWrapperParams db_params) : + db{db_params},src/evo/evodb.h (2)
99-103: Return by reference from GetRawDB is fineConsider marking [[nodiscard]] on GetRawDB for misuse prevention, optional.
111-111: Thread-safety annotation for IsEmpty()Other methods acquire cs internally. For consistency, consider adding EXCLUSIVE_LOCKS_REQUIRED(!cs) and taking the lock inside, or document why IsEmpty() is safe without it.
src/instantsend/db.cpp (1)
43-45: Remove redundant reset() before reassigning unique_ptrunique_ptr assignment will destroy the old db automatically. Drop the explicit db.reset() to simplify.
- // Wipe db - db.reset(); - db = util::MakeDbWrapper(db_params); + // Wipe db + db = util::MakeDbWrapper(db_params);src/init.cpp (1)
2360-2364: Nit: prefer explicit const in lambda parameterForEachMN provides a const reference; make it explicit for readability.
- mnList.ForEachMN(false, [&](auto& dmn) { + mnList.ForEachMN(false, [&](const auto& dmn) {src/llmq/dkgsessionmgr.cpp (1)
37-37: Extract shared constant for LLMQ DB cache size.Hardcoding 8 << 20 here and in other LLMQ DBs diverges easily. Define a single constexpr (e.g., LLMQ_DB_CACHE_BYTES) and reuse.
src/llmq/signing.h (1)
130-131: Optional: take DbWrapperParams by value for uniformity.CRecoveredSigsDb currently takes const&. For consistency with CSigningManager and future-proofing, consider by-value + move in the impl.
- explicit CRecoveredSigsDb(const util::DbWrapperParams& db_params); + explicit CRecoveredSigsDb(util::DbWrapperParams db_params);src/llmq/signing.cpp (1)
555-556: Nit: use %zu for size_t in verification log.verifyCount and recSigsByNode.size() are size_t; "%d" risks truncation.
- LogPrint(BCLog::LLMQ, "CSigningManager::%s -- verified recovered sig(s). count=%d, vt=%d, nodes=%d\n", __func__, verifyCount, verifyTimer.count(), recSigsByNode.size()); + LogPrint(BCLog::LLMQ, "CSigningManager::%s -- verified recovered sig(s). count=%zu, vt=%d, nodes=%zu\n", __func__, verifyCount, verifyTimer.count(), recSigsByNode.size());src/llmq/context.h (1)
41-44: Updated constructor signature LGTM.Explicit and ends with const util::DbWrapperParams& as intended.
If not already documented, consider a brief comment clarifying that db_params can enable memory-only DBs for tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/dbwrapper.h(1 hunks)src/evo/evodb.cpp(2 hunks)src/evo/evodb.h(5 hunks)src/init.cpp(2 hunks)src/instantsend/db.cpp(1 hunks)src/instantsend/db.h(2 hunks)src/instantsend/instantsend.cpp(1 hunks)src/instantsend/instantsend.h(2 hunks)src/llmq/context.cpp(1 hunks)src/llmq/context.h(1 hunks)src/llmq/dkgsessionmgr.cpp(1 hunks)src/llmq/dkgsessionmgr.h(2 hunks)src/llmq/quorums.cpp(1 hunks)src/llmq/quorums.h(2 hunks)src/llmq/signing.cpp(2 hunks)src/llmq/signing.h(3 hunks)src/node/chainstate.cpp(6 hunks)src/node/chainstate.h(4 hunks)src/test/util/setup_common.cpp(7 hunks)src/test/util/setup_common.h(1 hunks)src/test/validation_chainstatemanager_tests.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/dbwrapper.h
- src/llmq/quorums.h
- src/test/validation_chainstatemanager_tests.cpp
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/test/util/setup_common.hsrc/node/chainstate.cppsrc/instantsend/db.hsrc/node/chainstate.hsrc/evo/evodb.cppsrc/llmq/dkgsessionmgr.cppsrc/llmq/signing.cppsrc/llmq/context.cppsrc/llmq/dkgsessionmgr.hsrc/instantsend/instantsend.cppsrc/init.cppsrc/instantsend/instantsend.hsrc/instantsend/db.cppsrc/test/util/setup_common.cppsrc/llmq/quorums.cppsrc/llmq/context.hsrc/llmq/signing.hsrc/evo/evodb.h
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/util/setup_common.hsrc/test/util/setup_common.cpp
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/evodb.cppsrc/evo/evodb.h
🧠 Learnings (3)
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
PR: dashpay/dash#6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/node/chainstate.cppsrc/llmq/context.h
📚 Learning: 2025-01-02T21:50:00.967Z
Learnt from: kwvg
PR: dashpay/dash#6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
Applied to files:
src/llmq/context.h
📚 Learning: 2024-12-29T17:43:41.755Z
Learnt from: kwvg
PR: dashpay/dash#6504
File: src/llmq/quorums.cpp:224-224
Timestamp: 2024-12-29T17:43:41.755Z
Learning: The `CQuorumManager` is fully initialized by `LLMQContext`, addressing any concerns about the manager’s initialization sequence.
Applied to files:
src/llmq/context.h
🧬 Code graph analysis (11)
src/instantsend/db.h (7)
src/evo/evodb.h (1)
util(12-14)src/instantsend/instantsend.h (1)
util(34-36)src/llmq/context.h (1)
util(32-34)src/llmq/dkgsessionmgr.h (1)
util(36-38)src/llmq/quorums.h (1)
util(40-42)src/llmq/signing.h (1)
util(35-37)src/instantsend/db.cpp (4)
Upgrade(37-51)Upgrade(37-37)CInstantSendDb(29-33)CInstantSendDb(35-35)
src/node/chainstate.h (1)
src/fs.h (1)
path(42-42)
src/evo/evodb.cpp (1)
src/evo/evodb.h (1)
CEvoDB(38-76)
src/llmq/signing.cpp (1)
src/llmq/signing.h (2)
CRecoveredSigsDb(119-285)CSigningManager(165-251)
src/llmq/dkgsessionmgr.h (1)
src/llmq/dkgsessionmgr.cpp (2)
CDKGSessionManager(32-61)CDKGSessionManager(63-63)
src/instantsend/instantsend.h (1)
src/instantsend/instantsend.cpp (2)
CInstantSendManager(59-72)CInstantSendManager(74-74)
src/test/util/setup_common.cpp (2)
src/node/chainstate.cpp (2)
DashChainstateSetup(220-261)DashChainstateSetup(220-236)src/node/interfaces.cpp (5)
chainman(576-578)chainman(1061-1063)chainman(1131-1134)MakeChain(1143-1143)MakeChain(1143-1143)
src/llmq/quorums.cpp (1)
src/llmq/utils.cpp (8)
InitQuorumsCache(962-968)InitQuorumsCache(962-962)InitQuorumsCache(969-970)InitQuorumsCache(971-972)InitQuorumsCache(973-978)InitQuorumsCache(979-980)InitQuorumsCache(981-982)InitQuorumsCache(983-984)
src/llmq/context.h (2)
src/evo/evodb.h (1)
util(12-14)src/llmq/context.cpp (2)
LLMQContext(18-40)LLMQContext(42-44)
src/llmq/signing.h (1)
src/llmq/signing.cpp (4)
CRecoveredSigsDb(30-33)CRecoveredSigsDb(35-35)CSigningManager(335-341)CSigningManager(343-343)
src/evo/evodb.h (2)
src/dbwrapper.h (1)
CDBWrapper(214-534)src/evo/evodb.cpp (2)
CEvoDB(34-40)CEvoDB(42-42)
🪛 GitHub Actions: Clang Diff Format Check
src/llmq/quorums.cpp
[error] 1-1: Clang-format differences found in src/llmq/quorums.cpp. Run the formatting tool to fix code style differences.
src/llmq/signing.h
[error] 1-1: Clang-format differences found in src/llmq/signing.h. Run the formatting tool to fix code style differences.
src/evo/evodb.h
[error] 1-1: Clang-format differences found in src/evo/evodb.h. Run the formatting tool to fix code style differences.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (20)
src/evo/evodb.cpp (1)
34-39: MakeDbWrapper migration and transaction wiring: LGTMConstructor, ownership, and commit path look correct; no behavior change concerns.
Also applies to: 42-42, 61-61
src/llmq/dkgsessionmgr.h (2)
84-91: Good: make manager non-copyable and require explicit constructionStronger ownership semantics; constructor using const util::DbWrapperParams& matches forward-decl use. LGTM.
94-96: Ensure StopThreads is idempotent and safe post-Interrupt()Please confirm StopThreads() can be called multiple times and after Interrupt() without UB, and that all worker threads check the interrupt and join. Add a brief comment if invariants apply.
src/evo/evodb.h (3)
43-44: Switch to unique_ptr looks goodMatches factory-based construction and clarifies ownership. No issues spotted.
53-58: API tightening: delete default/copy and add explicit params-ctorSolid improvement; destructor defaulted in .cpp keeps ABI tidy. LGTM.
1-122: Fix clang-format diffs to unblock CICI reports style diffs for this file. Please run the repo’s clang-format config on src/evo/evodb.h and commit the result.
src/init.cpp (1)
1984-2000: LGTM: propagate data-dir and in-memory DB flags to LoadChainstateThe added args.GetDataDirNet() and dash_dbs_in_memory=false parameters align with the new signature and production defaults.
src/llmq/dkgsessionmgr.cpp (1)
36-44: Ctor refactor to DbWrapperParams/MakeDbWrapper looks good.Correct propagation and brace-init; path layout llmq/dkgdb is consistent.
src/llmq/signing.h (2)
186-191: By-value DbWrapperParams enables move semantics.Header matches impl; avoids const-move anti-pattern.
1-200: Fix clang-format violations in this header.CI flagged style differences; please run clang-format for this file to unblock.
Suggested command:
- clang-format -i src/llmq/signing.h
src/node/chainstate.cpp (1)
76-78: CEvoDB via DbWrapperParams/MakeDbWrapper looks good.Clean switch to parameter object; wipe flag matches fReset||fReindexChainState.
src/llmq/signing.cpp (2)
30-33: LGTM on MakeDbWrapper usage for recsigdb.Correct path and in-memory/wipe propagation.
335-343: By-value DbWrapperParams ctor wiring is correct.Moves into db subobject as intended (uniform with other managers).
src/test/util/setup_common.cpp (2)
126-136: Test shim DashChainstateSetup signature passthrough looks good.Clean wrapper to pass data dir and LLMQ DB flags.
219-225: Good: tests use in-memory, wipe-on-start EvoDB.Keeps unit tests fast and isolated.
src/llmq/quorums.cpp (1)
232-235: Destructor change LGTM.Behavior unchanged; Stop() on destruction is appropriate and idempotent.
src/llmq/context.h (2)
31-34: Forward declaration LGTM.Keeps header light; avoids including dbwrapper.h here.
40-40: Delete assignment operator LGTM.Matches non-copyable ownership semantics of unique_ptr members.
src/llmq/context.cpp (2)
29-29: db_params propagation verified—all LLMQ component constructors properly accept and receive DbWrapperParams.Confirmed across CDKGSessionManager, CQuorumManager, CSigningManager, and CInstantSendManager. No legacy parameter patterns in LLMQ components.
21-22: Constructor API consolidation verified.The instantiation site in
src/node/chainstate.cpp:250–252correctly uses the newutil::DbWrapperParamssignature, and no legacy parameter remnants exist in the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/evo/evodb.h (1)
1-1: Address formatting differences.The pipeline detected Clang format differences. Please run the formatting script to apply the required changes.
♻️ Duplicate comments (1)
src/node/chainstate.cpp (1)
238-261: Guard mempool before dereferencemempool is a raw pointer and is dereferenced (ConnectManagers, passed by reference). Add a non-fatal guard at function start to avoid accidental null deref.
void DashChainstateSetup(ChainstateManager& chainman, @@ CTxMemPool* mempool, const fs::path& data_dir, bool llmq_dbs_in_memory, bool llmq_dbs_wipe, const Consensus::Params& consensus_params) { + CHECK_NONFATAL(mempool != nullptr);Based on learnings
🧹 Nitpick comments (1)
src/llmq/quorums.cpp (1)
1-3: Fix clang-format diffs to unblock CICI reports format drift. Please run the repo’s formatting script (e.g., contrib/devtools/clang-format-* or equivalent make target) on this file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/dbwrapper.h(1 hunks)src/evo/evodb.cpp(2 hunks)src/evo/evodb.h(5 hunks)src/init.cpp(2 hunks)src/instantsend/db.cpp(1 hunks)src/instantsend/db.h(2 hunks)src/instantsend/instantsend.cpp(1 hunks)src/instantsend/instantsend.h(2 hunks)src/llmq/context.cpp(1 hunks)src/llmq/context.h(1 hunks)src/llmq/dkgsessionmgr.cpp(1 hunks)src/llmq/dkgsessionmgr.h(2 hunks)src/llmq/quorums.cpp(1 hunks)src/llmq/quorums.h(2 hunks)src/llmq/signing.cpp(2 hunks)src/llmq/signing.h(3 hunks)src/node/chainstate.cpp(6 hunks)src/node/chainstate.h(4 hunks)src/test/util/setup_common.cpp(7 hunks)src/test/util/setup_common.h(1 hunks)src/test/validation_chainstatemanager_tests.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/test/validation_chainstatemanager_tests.cpp
- src/llmq/dkgsessionmgr.h
- src/node/chainstate.h
- src/dbwrapper.h
- src/init.cpp
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/instantsend/db.hsrc/llmq/quorums.hsrc/test/util/setup_common.cppsrc/test/util/setup_common.hsrc/llmq/quorums.cppsrc/llmq/signing.hsrc/node/chainstate.cppsrc/instantsend/instantsend.cppsrc/instantsend/db.cppsrc/llmq/context.cppsrc/evo/evodb.cppsrc/llmq/context.hsrc/llmq/signing.cppsrc/instantsend/instantsend.hsrc/llmq/dkgsessionmgr.cppsrc/evo/evodb.h
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/util/setup_common.cppsrc/test/util/setup_common.h
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/evodb.cppsrc/evo/evodb.h
🧠 Learnings (4)
📚 Learning: 2025-10-28T18:36:40.242Z
Learnt from: kwvg
PR: dashpay/dash#6923
File: src/test/util/setup_common.cpp:235-251
Timestamp: 2025-10-28T18:36:40.242Z
Learning: In `src/test/util/setup_common.cpp`, the `CEvoDB` instance in `BasicTestingSetup` is constructed with `.memory = true` flag (memory-only mode), so it does not create file handles on disk. This makes the destructor teardown order safe even if `fs::remove_all(m_path_root)` is called before `m_node.evodb.reset()`.
Applied to files:
src/test/util/setup_common.cppsrc/node/chainstate.cppsrc/evo/evodb.cpp
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
PR: dashpay/dash#6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/node/chainstate.cppsrc/llmq/context.h
📚 Learning: 2025-01-02T21:50:00.967Z
Learnt from: kwvg
PR: dashpay/dash#6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
Applied to files:
src/llmq/context.h
📚 Learning: 2024-12-29T17:43:41.755Z
Learnt from: kwvg
PR: dashpay/dash#6504
File: src/llmq/quorums.cpp:224-224
Timestamp: 2024-12-29T17:43:41.755Z
Learning: The `CQuorumManager` is fully initialized by `LLMQContext`, addressing any concerns about the manager’s initialization sequence.
Applied to files:
src/llmq/context.h
🧬 Code graph analysis (11)
src/instantsend/db.h (1)
src/instantsend/db.cpp (4)
Upgrade(37-51)Upgrade(37-37)CInstantSendDb(29-33)CInstantSendDb(35-35)
src/llmq/quorums.h (2)
src/evo/evodb.h (1)
util(12-14)src/llmq/quorums.cpp (2)
CQuorumManager(210-230)CQuorumManager(232-235)
src/test/util/setup_common.cpp (1)
src/node/chainstate.cpp (2)
DashChainstateSetup(220-261)DashChainstateSetup(220-236)
src/llmq/quorums.cpp (1)
src/llmq/utils.cpp (8)
InitQuorumsCache(962-968)InitQuorumsCache(962-962)InitQuorumsCache(969-970)InitQuorumsCache(971-972)InitQuorumsCache(973-978)InitQuorumsCache(979-980)InitQuorumsCache(981-982)InitQuorumsCache(983-984)
src/llmq/signing.h (1)
src/llmq/signing.cpp (4)
CRecoveredSigsDb(30-33)CRecoveredSigsDb(35-35)CSigningManager(335-341)CSigningManager(343-343)
src/instantsend/db.cpp (1)
src/instantsend/db.h (4)
CInstantSendDb(32-163)cs_db(39-39)cs_db(41-162)CURRENT_VERSION(37-37)
src/evo/evodb.cpp (1)
src/evo/evodb.h (1)
CEvoDB(38-76)
src/llmq/context.h (1)
src/llmq/context.cpp (2)
LLMQContext(18-40)LLMQContext(42-44)
src/llmq/signing.cpp (1)
src/llmq/signing.h (2)
CRecoveredSigsDb(119-285)CSigningManager(165-251)
src/instantsend/instantsend.h (2)
src/evo/evodb.h (1)
util(12-14)src/instantsend/instantsend.cpp (2)
CInstantSendManager(59-72)CInstantSendManager(74-74)
src/evo/evodb.h (1)
src/dbwrapper.h (1)
CDBWrapper(214-534)
🪛 GitHub Actions: Clang Diff Format Check
src/llmq/quorums.cpp
[error] 1-1: Clang format differences detected. Run the formatting script to apply changes.
src/evo/evodb.h
[error] 1-1: Clang format differences detected. Run the formatting script to apply changes.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (42)
src/llmq/signing.h (4)
35-37: LGTM!The forward declaration for
util::DbWrapperParamsis well-placed and reduces compilation dependencies by avoiding the need to include the fulldbwrapper.hheader.
130-131: LGTM!The constructor signature update to accept
util::DbWrapperParamsimproves maintainability by replacing scattered boolean flags with a structured configuration object.
186-188: LGTM!Deleting the default constructor, copy constructor, and copy-assignment operator is appropriate for a manager class that owns database resources and should not be copyable.
189-191: LGTM!The constructor signature is updated consistently with the broader refactor. The
explicitkeyword appropriately prevents implicit conversions, and past review feedback regarding move semantics has been addressed.src/instantsend/db.h (3)
27-29: LGTM!The forward declaration for
util::DbWrapperParamsis properly placed and reduces header dependencies.
84-84: LGTM!The
Upgrademethod signature is updated consistently with the constructor to acceptutil::DbWrapperParams.
87-88: LGTM!The constructor signature update centralizes DB configuration and prevents implicit conversions with the
explicitkeyword.src/test/util/setup_common.h (1)
88-92: LGTM!The updated function signature with
llmq_dbs_in_memoryandllmq_dbs_wipeparameters is clear and aligns with the broader DB configuration refactor.src/evo/evodb.cpp (3)
34-40: LGTM!The constructor properly uses
util::MakeDbWrapperto initialize the database with the correct path derivation and cache size. The brace initialization and member initialization order are correct.
42-42: LGTM!The defaulted destructor is appropriate for managing the
unique_ptr<CDBWrapper>member and ensures proper cleanup.
50-64: LGTM!The
RollbackCurTransactionmethod is properly implemented with locking, and the update todb->WriteBatch(line 61) is the correct syntax for accessing methods through theunique_ptr.src/instantsend/instantsend.cpp (1)
59-70: LGTM!The constructor properly initializes the
dbmember withdb_paramsand uses consistent brace initialization for other members. Past review feedback regarding move semantics has been addressed.src/instantsend/db.cpp (2)
29-33: LGTM!The constructor properly uses
util::MakeDbWrapperwith the correct path and cache size. TheUpgradecall withwipe=trueis conditional (only wipes if the database version is outdated, as checked on line 41), which is intentional behavior confirmed by past review discussions.
37-51: LGTM!The
Upgrademethod correctly checks the database version and only wipes/recreates the database when the version is outdated. The use ofutil::MakeDbWrapperfor recreation is consistent with the constructor.src/evo/evodb.h (5)
12-14: LGTM!The forward declaration for
util::DbWrapperParamsis properly placed and follows the same pattern used throughout the codebase.
43-43: LGTM!Changing to
std::unique_ptr<CDBWrapper>provides better control over the database wrapper's lifetime and enables proper move semantics.
53-57: LGTM!The deleted special members appropriately prevent copying and default construction. The explicit constructor and destructor declarations are consistent with the class's resource management responsibilities.
99-102: LGTM!The
GetRawDBmethod correctly dereferences theunique_ptrto return a reference to the underlyingCDBWrapper.
111-111: LGTM!The
IsEmptymethod correctly usesdb->IsEmpty()to access the method through theunique_ptr.src/llmq/dkgsessionmgr.cpp (1)
32-61: LGTM!The constructor properly integrates
util::DbWrapperParamsfor database initialization usingutil::MakeDbWrapperwith the correct path derivation. The brace initialization for member variables is consistent with modern C++ style, and the session handler creation logic remains intact.src/instantsend/instantsend.h (3)
34-36: Good: forward-declare DbWrapperParams in headerAvoids pulling heavy headers; safe since only used by reference.
95-97: Good: make manager non-default/copyablePrevents accidental slicing/copies; fits lifetime and resource ownership.
98-101: Ctor now takes DbWrapperParams by const& — correct for incomplete typesFixes the incomplete-type by-value hazard and avoids unnecessary copies; matches instantsend.cpp initializer.
src/node/chainstate.cpp (5)
76-79: Switch to DbWrapperParams for CEvoDB looks correctThreads path/memory/wipe coherently; aligns with memory-only test mode.
92-95: Wiring LLMQ setup with db_params is consistentPass-through of data_dir/memory/wipe into DashChainstateSetup is clear.
233-236: DashChainstateSetup signature extension is coherentAdds explicit data_dir and LLMQ DB controls; improves testability.
249-253: LLMQContext via DbWrapperParams — OKConsistent with the new pattern; memory-only and wipe flags are explicit.
51-67: All LoadChainstate and DashChainstateSetup call sites are correctly updatedVerification confirms both LoadChainstate calls (init.cpp, setup_common.cpp) and all DashChainstateSetup invocations include the new data_dir and in-memory database parameters. The test infrastructure properly forwards parameters through the wrapper variant.
src/llmq/quorums.h (2)
41-42: Good: forward-declare DbWrapperParamsKeeps header light; safe due to by-reference usage.
265-273: Ctor API tightened and specials deletedNon-default/copyable and db_params by const& are appropriate here.
src/llmq/signing.cpp (2)
30-33: RecoveredSigs DB uses DbWrapperParams correctlyProper path scoping and fixed cache size for this DB.
335-341: SigningManager ctor: clean pass-through of db_paramsBy-ref parameter and direct member init are appropriate.
src/llmq/quorums.cpp (2)
214-225: Ctor migrated to DbWrapperParams — aligned with designDB path namespaced under llmq/quorumdb; fixed cache size is intentional here.
232-235: Dtor calls Stop() — good for deterministic teardownEnsures worker pool and threads halt on destruction.
src/test/util/setup_common.cpp (5)
126-136: LGTM! Parameter naming improvement.The signature change from
fReset/fReindexChainStatetollmq_dbs_in_memory/llmq_dbs_wipeis more descriptive and aligns with theDbWrapperParamsrefactoring. The call site correctly passes the data directory path and memory flags.
219-224: Initialization order respects dependencies.The reordering ensures that
evodbis initialized beforecpoolmanandmnhf_manager, which depend on it. The use ofDbWrapperParamswith.memory = trueis consistent with the memory-only test setup noted in the learnings.As per the previous discussion, this reordering is preparation for upstream changes (bitcoin#24595).
235-253: Teardown order is correct.The destructor order is appropriate. Per the retrieved learning,
fs::remove_all(m_path_root)can safely be called before resettingm_node.evodbbecause EvoDb is constructed with.memory = true, so it doesn't create file handles on disk.[Based on learnings]
284-296: LGTM! Proper reverse-initialization order.The destructor resets components in reverse order of their initialization, which is the correct practice to ensure dependencies are torn down safely.
306-336: LGTM! Consistent with memory-only test databases.The
LoadChainstatecall now explicitly passes the data directory path and in-memory flags (coins_db_in_memory,dash_dbs_in_memory), which is consistent with the PR's goal of using memory-only databases for improved test performance.src/llmq/context.cpp (1)
18-40: LGTM! Clean parameter consolidation.The refactoring from individual
unit_tests/wipeboolean parameters to a singleDbWrapperParamsstruct simplifies the constructor signature and ensures consistent DB configuration across all LLMQ managers (qdkgsman,qman,sigman,isman).src/llmq/context.h (2)
31-34: LGTM! Proper forward declaration.The namespace reorganization correctly forward-declares
util::DbWrapperParamsto support the updated constructor signature.
38-44: LGTM! Good API safety improvements.The changes improve the API:
explicitkeyword on the constructor prevents unintended implicit conversions- Deleted copy assignment operator is consistent with the deleted copy constructor
- Updated signature with
DbWrapperParamssimplifies the interface
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/llmq/quorums.cpp (1)
858-861: Guard against null quorumVvec in QDATA path.Dereferencing
quorumVveccan null-deref if we requested only ENCRYPTED_CONTRIBUTIONS and don’t yet have the vvec. Add a null check (or useHasVerificationVector()).Apply:
- if (WITH_LOCK(pQuorum->cs_vvec_shShare, return pQuorum->quorumVvec->size() != size_t(pQuorum->params.threshold))) { + if (WITH_LOCK(pQuorum->cs_vvec_shShare, return !pQuorum->quorumVvec || + pQuorum->quorumVvec->size() != size_t(pQuorum->params.threshold))) { return errorHandler("No valid quorum verification vector available", 0); // Don't bump score because we asked for it }Alternative:
- if (WITH_LOCK(pQuorum->cs_vvec_shShare, return pQuorum->quorumVvec->size() != size_t(pQuorum->params.threshold))) { + if (!pQuorum->HasVerificationVector() || + WITH_LOCK(pQuorum->cs_vvec_shShare, return pQuorum->quorumVvec->size() != size_t(pQuorum->params.threshold))) { ... }
♻️ Duplicate comments (6)
src/llmq/quorums.cpp (1)
214-226: LGTM: DB init via MakeDbWrapper with tuned cache.Per earlier PR discussion, fixed 1 MiB here is intentional for quorum DB access patterns.
src/node/chainstate.cpp (1)
251-253: Guard mempool pointer before dereferenceAdd a non-fatal assert at function start to avoid accidental null deref in future refactors.
Apply:
void DashChainstateSetup(ChainstateManager& chainman, @@ -{ +{ + CHECK_NONFATAL(mempool != nullptr); @@ - llmq_ctx = std::make_unique<LLMQContext>(chainman, *dmnman, *evodb, mn_metaman, *mnhf_manager, sporkman, - *mempool, mn_activeman.get(), mn_sync, + 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 = llmq_dbs_in_memory, .wipe = llmq_dbs_wipe});src/instantsend/instantsend.cpp (1)
61-62: Ctor takes DbWrapperParams by const& (no move) — OK; consider consistency across managers.This fixes the prior move-on-const issue. If other managers take DbWrapperParams by value to enable move from temporaries, consider aligning for consistency; otherwise this is fine.
src/test/util/setup_common.h (1)
90-92: Reduce boolean-parameter soup; group LLMQ DB flags.Consider a small options struct (or reuse util::DbWrapperParams-derived config) for llmq_dbs_in_memory/llmq_dbs_wipe to improve readability and call-site safety.
src/node/chainstate.h (1)
96-112: Consolidate related booleans into an options struct.LoadChainstate/DashChainstateSetup signatures are lengthy and error-prone. Prefer a struct grouping the DB/mode flags (e.g., ChainstateOptions with addr/spent/time/tx indexes, prune, in-memory flags).
Also applies to: 129-132
src/llmq/dkgsessionmgr.h (1)
36-38: Public surface cleanup LGTM; consider by-value DbWrapperParams for move-friendly call sites.Forward-decl and deleted ctors are good. If you expect temporaries at call sites, taking DbWrapperParams by value enables efficient moves into MakeDbWrapper.
Also applies to: 84-92
🧹 Nitpick comments (8)
src/llmq/signing.h (1)
186-191: Optionally delete move specials to prevent accidental moves.Given reference members and background thread state elsewhere, consider also deleting move ctor/assignment to avoid surprising moves.
Apply:
class CSigningManager { public: CSigningManager() = delete; CSigningManager(const CSigningManager&) = delete; CSigningManager& operator=(const CSigningManager&) = delete; + CSigningManager(CSigningManager&&) = delete; + CSigningManager& operator=(CSigningManager&&) = delete;src/llmq/quorums.h (1)
265-273: Also consider deleting move operations.To avoid accidental moves with reference members and worker state, delete move ctor/assignment explicitly.
Apply:
class CQuorumManager { public: CQuorumManager() = delete; CQuorumManager(const CQuorumManager&) = delete; CQuorumManager& operator=(const CQuorumManager&) = delete; + CQuorumManager(CQuorumManager&&) = delete; + CQuorumManager& operator=(CQuorumManager&&) = delete; explicit CQuorumManager(..., const util::DbWrapperParams& db_params);src/node/chainstate.cpp (1)
271-279: Be defensive on mempool during teardownmempool is a raw pointer; wrap DisconnectManagers() with a null check to harden against future call-site changes.
Apply:
- mempool->DisconnectManagers(); + if (mempool) { + mempool->DisconnectManagers(); + }src/llmq/context.h (1)
31-45: Finalize non-movable semanticsCopy/assign are deleted; consider deleting moves too to prevent accidental moves of a heavy, threaded context.
Apply:
struct LLMQContext { public: LLMQContext() = delete; LLMQContext(const LLMQContext&) = delete; - LLMQContext& operator=(const LLMQContext&) = delete; + LLMQContext& operator=(const LLMQContext&) = delete; + LLMQContext(LLMQContext&&) = delete; + LLMQContext& operator=(LLMQContext&&) = delete;src/instantsend/db.cpp (1)
29-33: Good: conditional upgrade/wipe; avoid duplicated cache-size literals.Logic preserves existing behavior; suggest a single named constexpr (e.g., kISDBCacheBytes = 32 << 20) used in both places to prevent drift.
Also applies to: 37-45
src/init.cpp (1)
1985-1985: Nice inline docs on new params; consider a flag for dash_dbs_in_memory.Passing args.GetDataDirNet() and explicitly documenting dash_dbs_in_memory is clear. Optionally add a CLI/test arg to toggle dash_dbs_in_memory for benches/regtest.
Also applies to: 1999-2000
src/evo/evodb.cpp (1)
34-39: Ctor/dtor LGTM; consider naming the 64 MiB cache size.Implementation is sound. For maintainability, use a constexpr (e.g., kEvoDbCacheBytes = 64 << 20). Memory-only mode in tests aligns with the harness. Based on learnings.
Also applies to: 42-43
src/llmq/dkgsessionmgr.cpp (1)
36-45: DB init via MakeDbWrapper — good; tiny cleanup idea.Initialization is correct. Optionally add a quick guard for a null Tip() in CleanupOldContributions() before using m_chainstate.m_chain.Tip()->nHeight to be defensive on early start-up paths.
- if (pindexQuorum == nullptr || m_chainstate.m_chain.Tip()->nHeight - pindexQuorum->nHeight > params.max_store_depth()) { + const auto* tip = m_chainstate.m_chain.Tip(); + if (pindexQuorum == nullptr || tip == nullptr || + tip->nHeight - pindexQuorum->nHeight > params.max_store_depth()) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/dbwrapper.h(1 hunks)src/evo/evodb.cpp(2 hunks)src/evo/evodb.h(5 hunks)src/init.cpp(2 hunks)src/instantsend/db.cpp(1 hunks)src/instantsend/db.h(2 hunks)src/instantsend/instantsend.cpp(1 hunks)src/instantsend/instantsend.h(2 hunks)src/llmq/context.cpp(1 hunks)src/llmq/context.h(1 hunks)src/llmq/dkgsessionmgr.cpp(1 hunks)src/llmq/dkgsessionmgr.h(2 hunks)src/llmq/quorums.cpp(1 hunks)src/llmq/quorums.h(2 hunks)src/llmq/signing.cpp(2 hunks)src/llmq/signing.h(3 hunks)src/node/chainstate.cpp(6 hunks)src/node/chainstate.h(4 hunks)src/test/util/setup_common.cpp(7 hunks)src/test/util/setup_common.h(1 hunks)src/test/validation_chainstatemanager_tests.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/test/validation_chainstatemanager_tests.cpp
- src/dbwrapper.h
- src/instantsend/db.h
- src/instantsend/instantsend.h
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/instantsend/instantsend.cppsrc/node/chainstate.hsrc/test/util/setup_common.hsrc/llmq/quorums.hsrc/instantsend/db.cppsrc/llmq/signing.hsrc/init.cppsrc/llmq/quorums.cppsrc/llmq/signing.cppsrc/llmq/context.hsrc/llmq/context.cppsrc/llmq/dkgsessionmgr.hsrc/node/chainstate.cppsrc/test/util/setup_common.cppsrc/llmq/dkgsessionmgr.cppsrc/evo/evodb.hsrc/evo/evodb.cpp
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/util/setup_common.hsrc/test/util/setup_common.cpp
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/evodb.hsrc/evo/evodb.cpp
🧠 Learnings (4)
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
PR: dashpay/dash#6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/llmq/context.hsrc/llmq/context.cppsrc/node/chainstate.cpp
📚 Learning: 2025-01-02T21:50:00.967Z
Learnt from: kwvg
PR: dashpay/dash#6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
Applied to files:
src/llmq/context.h
📚 Learning: 2024-12-29T17:43:41.755Z
Learnt from: kwvg
PR: dashpay/dash#6504
File: src/llmq/quorums.cpp:224-224
Timestamp: 2024-12-29T17:43:41.755Z
Learning: The `CQuorumManager` is fully initialized by `LLMQContext`, addressing any concerns about the manager’s initialization sequence.
Applied to files:
src/llmq/context.h
📚 Learning: 2025-10-28T18:36:40.263Z
Learnt from: kwvg
PR: dashpay/dash#6923
File: src/test/util/setup_common.cpp:235-251
Timestamp: 2025-10-28T18:36:40.263Z
Learning: In `src/test/util/setup_common.cpp`, the `CEvoDB` instance in `BasicTestingSetup` is constructed with `.memory = true` flag (memory-only mode), so it does not create file handles on disk. This makes the destructor teardown order safe even if `fs::remove_all(m_path_root)` is called before `m_node.evodb.reset()`.
Applied to files:
src/node/chainstate.cppsrc/test/util/setup_common.cppsrc/evo/evodb.cpp
🧬 Code graph analysis (10)
src/node/chainstate.h (1)
src/fs.h (1)
path(42-42)
src/llmq/quorums.h (1)
src/llmq/quorums.cpp (2)
CQuorumManager(210-230)CQuorumManager(232-235)
src/llmq/signing.h (1)
src/llmq/signing.cpp (4)
CRecoveredSigsDb(30-33)CRecoveredSigsDb(35-35)CSigningManager(335-341)CSigningManager(343-343)
src/llmq/quorums.cpp (1)
src/llmq/utils.cpp (8)
InitQuorumsCache(962-968)InitQuorumsCache(962-962)InitQuorumsCache(969-970)InitQuorumsCache(971-972)InitQuorumsCache(973-978)InitQuorumsCache(979-980)InitQuorumsCache(981-982)InitQuorumsCache(983-984)
src/llmq/signing.cpp (1)
src/llmq/signing.h (2)
CRecoveredSigsDb(119-285)CSigningManager(165-251)
src/llmq/context.h (1)
src/llmq/context.cpp (2)
LLMQContext(18-40)LLMQContext(42-44)
src/llmq/dkgsessionmgr.h (1)
src/llmq/dkgsessionmgr.cpp (2)
CDKGSessionManager(32-61)CDKGSessionManager(63-63)
src/test/util/setup_common.cpp (1)
src/node/chainstate.cpp (2)
DashChainstateSetup(220-261)DashChainstateSetup(220-236)
src/evo/evodb.h (2)
src/dbwrapper.h (1)
CDBWrapper(214-534)src/evo/evodb.cpp (2)
CEvoDB(34-40)CEvoDB(42-42)
src/evo/evodb.cpp (1)
src/evo/evodb.h (1)
CEvoDB(38-76)
🔇 Additional comments (27)
src/llmq/signing.h (2)
35-37: LGTM: lean header via forward declaration.Keeps dependencies minimal while taking the param by reference.
130-131: LGTM: unify DB construction via DbWrapperParams.Constructor signature aligns with the repo-wide DB init pattern.
src/llmq/signing.cpp (3)
30-32: LGTM: DB wrapper via MakeDbWrapper (8 MiB cache).Matches the per-DB cache sizing approach for recsig DB.
335-339: LGTM: clean param forwarding without ineffective moves.Forwarding const& to CRecoveredSigsDb ctor is appropriate here.
343-344: LGTM: trivial defaulted destructor.No custom cleanup needed beyond members’ destructors.
src/llmq/quorums.h (1)
40-42: LGTM: forward declaration keeps header slim.Good to avoid pulling dbwrapper headers into a widely included file.
src/llmq/quorums.cpp (1)
232-235: LGTM: destructor stops background work.Ensures worker shutdown on destruction.
src/evo/evodb.h (5)
12-15: LGTM: forward declare util::DbWrapperParams.Keeps the header dependency-light.
43-44: LGTM: unique_ptr for DB ownership.Clear ownership; aligns with memory-only DB lifecycle.
53-58: LGTM: explicit ctor and deleted copy operations.Construction via DbWrapperParams standardizes DB setup.
101-102: LGTM: GetRawDB returns reference to underlying DB.Callers remain unchanged semantically.
111-112: LGTM: IsEmpty via pointer.Matches new ownership model.
src/test/util/setup_common.cpp (7)
128-130: Signature forwarding LGTMForwarder now takes llmq_dbs_in_memory/llmq_dbs_wipe; matches downstream API.
134-136: Correctly threads data_dir and in-memory flagsPassing GetDataDirNet(), llmq_dbs_in_memory, llmq_dbs_wipe is consistent with node::DashChainstateSetup.
193-193: No concerns on chain interface creationinterfaces::MakeChain(m_node) placement is fine.
219-225: EvoDB memory-only init is correct for testsCEvoDB constructed with {.memory = true, .wipe = true}. This avoids file handles and speeds tests. Safe and aligned with PR goals. Based on learnings
238-246: Teardown order is safe with memory-only EvoDBCalling fs::remove_all before evodb.reset() is OK here because EvoDB is memory-only in BasicTestingSetup. No file handle conflicts. Based on learnings
291-296: ChainTestingSetup dtor LGTMOrderly shutdown of components; no issues spotted.
320-336: LoadChainstate: in-memory flags wired correctlyPassing data dir net and setting block_tree_db_in_memory, coins_db_in_memory, and dash_dbs_in_memory to true matches the test intent to keep DBs in RAM.
src/node/chainstate.cpp (5)
51-52: LoadChainstate now accepts data_dir pathParameter addition is consistent with new DB setup flow.
66-67: dash_dbs_in_memory threaded into LoadChainstateMatches PR intent; no issues.
92-95: LLMQ setup call: params wiring LGTMdata_dir, in-memory, and wipe flags are forwarded as expected.
233-236: DashChainstateSetup signature update LGTMNew data_dir and in-memory/wipe parameters are clear and future-proof.
76-78: Review comment is incorrect; ignore the suggestion.CEvoDB's constructor already appends
/ "evodb"to the provided path parameter (seesrc/evo/evodb.cpp:35). Implementing the suggested change would result indata_dir / "evodb" / "evodb"instead of the intendeddata_dir / "evodb". Additionally, the cache size is already hardcoded at 64 MB within the CEvoDB constructor and cannot be configured at the callsite.Likely an incorrect or invalid review comment.
src/llmq/context.cpp (2)
18-22: Constructor switch to DbWrapperParams LGTMReplacing scattered booleans with db_params improves clarity and consistency.
27-37: Consistent propagation of db_params to LLMQ componentsAll dependent managers receive the same DbWrapperParams; good cohesion and single-source config.
src/node/chainstate.h (1)
30-33: Forward-decl fs::path is fine here.Only used as a parameter type; no need to include fs.h in this header.
|
This pull request has conflicts, please rebase. |
To harmonize with the rest of the `CDBWrapper` instantiations needed for the next commit.
Helps us reduce our gArgs usage as bitcoin-chainstate won't be having ArgsManager with it
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/dsnotificationinterface.cpp (1)
1-1: Fix clang-format violations.The pipeline reports clang-format differences in this file. Please run clang-format and commit the formatting changes.
src/llmq/snapshot.cpp (1)
366-375: Usem_evoDb.Write()instead ofGetRawDB().Write()to respect transaction buffering.The direct
GetRawDB().Write()call at line 373 bypasses CEvoDB's transaction buffer system. Writes viam_evoDb.Write()are buffered tocurDBTransaction, while raw DB writes persist immediately—risking partial updates if other EvoDB writes within the same transaction roll back.The codebase already handles similar quorum DB updates correctly:
blockprocessor.cppusesm_evoDb.Write()directly (lines 374, 377, 379) without explicitBeginTransaction()wrapping. Apply the same pattern:void CQuorumSnapshotManager::StoreSnapshotForBlock(const Consensus::LLMQType llmqType, const CBlockIndex* pindex, const CQuorumSnapshot& snapshot) { auto snapshotHash = ::SerializeHash(std::make_pair(llmqType, pindex->GetBlockHash())); // LOCK(::cs_main); AssertLockNotHeld(m_evoDb.cs); LOCK2(snapshotCacheCs, m_evoDb.cs); - m_evoDb.GetRawDB().Write(std::make_pair(DB_QUORUM_SNAPSHOT, snapshotHash), snapshot); + m_evoDb.Write(std::make_pair(DB_QUORUM_SNAPSHOT, snapshotHash), snapshot); quorumSnapshotCache.insert(snapshotHash, snapshot); }
♻️ Duplicate comments (5)
src/node/chainstate.h (1)
96-97: Group related flags into a params struct to reduce boolean-parameter soup.Define a ChainstateOptions (or reuse util::DbWrapperParams-like) to carry data_dir and feature flags instead of many bools; simplifies call sites and evolution.
Example sketch:
- std::optional<ChainstateLoadingError> LoadChainstate(..., const fs::path& data_dir, bool fPruneMode, bool is_addrindex_enabled, ..., bool dash_dbs_in_memory, ...); + struct ChainstateOptions { const fs::path& data_dir; bool fPruneMode; bool is_addrindex_enabled; bool is_governance_enabled; bool is_spentindex_enabled; bool is_timeindex_enabled; bool is_txindex_enabled; bool block_tree_db_in_memory; bool coins_db_in_memory; bool dash_dbs_in_memory; }; + std::optional<ChainstateLoadingError> LoadChainstate(..., const ChainstateOptions& opts, ...);Update DashChainstateSetup similarly.
Also applies to: 111-112, 129-132
src/llmq/signing.h (1)
186-191: Enable move semantics for DbWrapperParams in CSigningManager (header).Constructor still takes const&; pass by value and move into member to avoid copies.
Apply in header:
- explicit CSigningManager(const CChainState& chainstate, const CQuorumManager& _qman, - const util::DbWrapperParams& db_params); + explicit CSigningManager(const CChainState& chainstate, const CQuorumManager& _qman, + util::DbWrapperParams db_params);Then mirror in src/llmq/signing.cpp and initialize with std::move (see separate comment). To confirm current state:
#!/bin/bash ast-grep --pattern $'class CSigningManager {\n $$$\n explicit CSigningManager($_, $_,\n $_);\n $$$\n}' rg -nP --type=cpp 'CSigningManager::CSigningManager\(' -n -C1 src/llmq/signing.cppsrc/llmq/signing.cpp (1)
335-343: Realize move semantics for db_params in CSigningManager (implementation).Take DbWrapperParams by value and std::move into the member to avoid copies. Mirrors earlier agreement.
-CSigningManager::CSigningManager(const CChainState& chainstate, const CQuorumManager& _qman, - const util::DbWrapperParams& db_params) : - db{db_params}, +CSigningManager::CSigningManager(const CChainState& chainstate, const CQuorumManager& _qman, + util::DbWrapperParams db_params) : + db{std::move(db_params)}, m_chainstate{chainstate}, qman{_qman} { }src/llmq/quorums.cpp (1)
214-225: LGTM on scoped quorumdb creation; cache size choice acknowledged as intentional.src/node/chainstate.cpp (1)
233-236: Guard mempool before deref in DashChainstateSetup.mempool is a raw pointer; it’s dereferenced (ConnectManagers) without a check. Add a non‑fatal assert at function entry to avoid accidental null deref in tests/future paths.
Apply near the start of DashChainstateSetup:
void DashChainstateSetup(ChainstateManager& chainman, @@ - const Consensus::Params& consensus_params) + const Consensus::Params& consensus_params) { + CHECK_NONFATAL(mempool != nullptr);Based on learnings.
Also applies to: 251-253
🧹 Nitpick comments (12)
src/instantsend/signing.h (1)
72-74: Good: make InstantSendSigner non-copyable.Prevents accidental slicing and dangling refs; matches ownership model.
Consider also explicitly deleting move ctor/assignment to document immovability:
+ InstantSendSigner(InstantSendSigner&&) = delete; + InstantSendSigner& operator=(InstantSendSigner&&) = delete;src/masternode/active/notificationinterface.h (1)
23-25: Non-copyable interface and explicit dtor look good.Matches typical validation interface usage and avoids accidental copies.
Since the class is final and base likely has a virtual dtor, consider
~ActiveNotificationInterface() overridefor clarity.src/coinjoin/coinjoin.h (1)
375-379: Make CDSTXManager non-copyable with explicit lifetime—good.Aligns with internal mutex ownership.
Consider explicitly deleting move operations to avoid future surprises:
+ CDSTXManager(CDSTXManager&&) = delete; + CDSTXManager& operator=(CDSTXManager&&) = delete;src/masternode/active/context.h (1)
44-49: Explicit, non-assignable ActiveContext—good; consider bundling deps.The 13‑param ctor is hard to read and error‑prone. A small struct (ActiveContextDeps) would improve call‑site clarity and future evolution.
Also consider deleting moves explicitly:
+ ActiveContext(ActiveContext&&) = delete; + ActiveContext& operator=(ActiveContext&&) = delete;src/evo/deterministicmns.h (1)
657-661: Make non-movability explicit (delete move ops too).Copy is deleted; destructor is declared. To fully prevent accidental moves, explicitly delete move constructor/assignment as well.
class CDeterministicMNManager { @@ public: CDeterministicMNManager() = delete; CDeterministicMNManager(const CDeterministicMNManager&) = delete; CDeterministicMNManager& operator=(const CDeterministicMNManager&) = delete; + CDeterministicMNManager(CDeterministicMNManager&&) = delete; + CDeterministicMNManager& operator=(CDeterministicMNManager&&) = delete; explicit CDeterministicMNManager(CEvoDB& evoDb, CMasternodeMetaMan& mn_metaman); ~CDeterministicMNManager();src/coinjoin/server.h (1)
97-105: Also delete move operations to lock in ownership semantics.Mirror copy-deletes with move-deletes to avoid accidental moves of a heavy manager.
CCoinJoinServer() = delete; CCoinJoinServer(const CCoinJoinServer&) = delete; CCoinJoinServer& operator=(const CCoinJoinServer&) = delete; + CCoinJoinServer(CCoinJoinServer&&) = delete; + CCoinJoinServer& operator=(CCoinJoinServer&&) = delete;src/instantsend/instantsend.h (1)
95-101: Ctor takes DbWrapperParams by const& — LGTM.Fixes incomplete‑type by‑value issue and avoids copies. Consider also deleting move ops for consistency with copy‑deletes.
CInstantSendManager() = delete; CInstantSendManager(const CInstantSendManager&) = delete; CInstantSendManager& operator=(const CInstantSendManager&) = delete; + CInstantSendManager(CInstantSendManager&&) = delete; + CInstantSendManager& operator=(CInstantSendManager&&) = delete;src/llmq/quorums.cpp (2)
1046-1077: Clear batch after final WriteBatch to avoid redundant replays on next prefix.After writing the remaining operations, Clear the batch before the loop continues with the next prefix.
- db.WriteBatch(batch); + db.WriteBatch(batch); + batch.Clear();
1189-1196: Also Clear batch after WriteBatch in MigrateOldQuorumDB’s per-prefix flush.Prevents re-applying earlier batch entries on subsequent prefixes.
- db.WriteBatch(batch); + db.WriteBatch(batch); + batch.Clear();src/evo/evodb.h (1)
56-57: Optional: honor DbWrapperParams.cache_size for consistency.evodb.cpp currently hardcodes 64 MiB. Consider wiring through db_params.cache_size (with a sane default) for consistency with other DB users.
src/test/util/setup_common.cpp (1)
239-254: Destructor ordering note (non‑blocking).Since CEvoDB is memory‑only here, calling fs::remove_all before evodb.reset() is safe. If this ever flips to disk‑backed in tests, prefer resetting evodb before remove_all to avoid Windows flakiness.
Based on learnings.
src/llmq/context.cpp (1)
21-37: Optional: standardize cache sizing across sub‑DBs.If DbWrapperParams carries cache_size, consider threading it through instead of hardcoded sizes in subcomponents for easier tuning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (61)
src/bls/bls_worker.h(1 hunks)src/chainlock/chainlock.h(1 hunks)src/chainlock/signing.h(1 hunks)src/coinjoin/client.cpp(3 hunks)src/coinjoin/client.h(3 hunks)src/coinjoin/coinjoin.cpp(2 hunks)src/coinjoin/coinjoin.h(2 hunks)src/coinjoin/server.cpp(1 hunks)src/coinjoin/server.h(1 hunks)src/dbwrapper.h(1 hunks)src/dsnotificationinterface.cpp(1 hunks)src/dsnotificationinterface.h(1 hunks)src/evo/chainhelper.h(1 hunks)src/evo/creditpool.cpp(1 hunks)src/evo/creditpool.h(1 hunks)src/evo/deterministicmns.cpp(1 hunks)src/evo/deterministicmns.h(1 hunks)src/evo/evodb.cpp(2 hunks)src/evo/evodb.h(5 hunks)src/evo/mnhftx.cpp(1 hunks)src/evo/mnhftx.h(1 hunks)src/governance/governance.h(1 hunks)src/governance/signing.h(1 hunks)src/init.cpp(2 hunks)src/instantsend/db.cpp(1 hunks)src/instantsend/db.h(2 hunks)src/instantsend/instantsend.cpp(1 hunks)src/instantsend/instantsend.h(2 hunks)src/instantsend/signing.h(1 hunks)src/llmq/blockprocessor.cpp(2 hunks)src/llmq/blockprocessor.h(1 hunks)src/llmq/context.cpp(1 hunks)src/llmq/context.h(1 hunks)src/llmq/debug.cpp(1 hunks)src/llmq/debug.h(1 hunks)src/llmq/dkgsessionmgr.cpp(1 hunks)src/llmq/dkgsessionmgr.h(2 hunks)src/llmq/quorums.cpp(1 hunks)src/llmq/quorums.h(2 hunks)src/llmq/signing.cpp(2 hunks)src/llmq/signing.h(3 hunks)src/llmq/signing_shares.h(1 hunks)src/llmq/snapshot.cpp(1 hunks)src/llmq/snapshot.h(1 hunks)src/masternode/active/context.h(1 hunks)src/masternode/active/notificationinterface.cpp(1 hunks)src/masternode/active/notificationinterface.h(1 hunks)src/masternode/meta.cpp(1 hunks)src/masternode/meta.h(1 hunks)src/masternode/node.cpp(2 hunks)src/masternode/node.h(1 hunks)src/masternode/sync.cpp(1 hunks)src/masternode/sync.h(1 hunks)src/netfulfilledman.cpp(1 hunks)src/netfulfilledman.h(1 hunks)src/node/chainstate.cpp(6 hunks)src/node/chainstate.h(4 hunks)src/spork.h(1 hunks)src/test/util/setup_common.cpp(7 hunks)src/test/util/setup_common.h(1 hunks)src/test/validation_chainstatemanager_tests.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- src/governance/governance.h
- src/masternode/meta.h
- src/evo/creditpool.cpp
- src/llmq/blockprocessor.h
- src/llmq/dkgsessionmgr.cpp
- src/coinjoin/server.cpp
- src/evo/mnhftx.cpp
- src/evo/deterministicmns.cpp
- src/masternode/sync.cpp
- src/instantsend/db.h
- src/netfulfilledman.cpp
- src/masternode/node.cpp
- src/llmq/debug.h
- src/evo/chainhelper.h
- src/llmq/blockprocessor.cpp
- src/bls/bls_worker.h
- src/masternode/meta.cpp
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/instantsend/db.cppsrc/governance/signing.hsrc/masternode/sync.hsrc/chainlock/chainlock.hsrc/masternode/active/notificationinterface.hsrc/instantsend/instantsend.hsrc/dbwrapper.hsrc/chainlock/signing.hsrc/llmq/quorums.hsrc/test/util/setup_common.cppsrc/init.cppsrc/evo/mnhftx.hsrc/coinjoin/coinjoin.cppsrc/llmq/dkgsessionmgr.hsrc/masternode/node.hsrc/test/util/setup_common.hsrc/masternode/active/notificationinterface.cppsrc/llmq/signing.hsrc/dsnotificationinterface.hsrc/llmq/snapshot.hsrc/llmq/signing.cppsrc/llmq/debug.cppsrc/llmq/context.cppsrc/evo/creditpool.hsrc/llmq/quorums.cppsrc/coinjoin/coinjoin.hsrc/evo/evodb.hsrc/test/validation_chainstatemanager_tests.cppsrc/spork.hsrc/instantsend/signing.hsrc/llmq/signing_shares.hsrc/coinjoin/server.hsrc/node/chainstate.cppsrc/evo/evodb.cppsrc/llmq/context.hsrc/evo/deterministicmns.hsrc/instantsend/instantsend.cppsrc/masternode/active/context.hsrc/dsnotificationinterface.cppsrc/coinjoin/client.hsrc/netfulfilledman.hsrc/coinjoin/client.cppsrc/llmq/snapshot.cppsrc/node/chainstate.h
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/masternode/sync.hsrc/masternode/active/notificationinterface.hsrc/evo/mnhftx.hsrc/masternode/node.hsrc/masternode/active/notificationinterface.cppsrc/evo/creditpool.hsrc/evo/evodb.hsrc/evo/evodb.cppsrc/evo/deterministicmns.hsrc/masternode/active/context.h
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/util/setup_common.cppsrc/test/util/setup_common.hsrc/test/validation_chainstatemanager_tests.cpp
🧠 Learnings (4)
📚 Learning: 2025-10-28T18:36:40.263Z
Learnt from: kwvg
PR: dashpay/dash#6923
File: src/test/util/setup_common.cpp:235-251
Timestamp: 2025-10-28T18:36:40.263Z
Learning: In `src/test/util/setup_common.cpp`, the `CEvoDB` instance in `BasicTestingSetup` is constructed with `.memory = true` flag (memory-only mode), so it does not create file handles on disk. This makes the destructor teardown order safe even if `fs::remove_all(m_path_root)` is called before `m_node.evodb.reset()`.
Applied to files:
src/test/util/setup_common.cppsrc/node/chainstate.cppsrc/evo/evodb.cpp
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
PR: dashpay/dash#6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/llmq/context.cppsrc/node/chainstate.cppsrc/llmq/context.h
📚 Learning: 2025-01-02T21:50:00.967Z
Learnt from: kwvg
PR: dashpay/dash#6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
Applied to files:
src/llmq/context.h
📚 Learning: 2024-12-29T17:43:41.755Z
Learnt from: kwvg
PR: dashpay/dash#6504
File: src/llmq/quorums.cpp:224-224
Timestamp: 2024-12-29T17:43:41.755Z
Learning: The `CQuorumManager` is fully initialized by `LLMQContext`, addressing any concerns about the manager’s initialization sequence.
Applied to files:
src/llmq/context.h
🧬 Code graph analysis (36)
src/governance/signing.h (1)
src/governance/signing.cpp (2)
GovernanceSigner(24-34)GovernanceSigner(36-36)
src/masternode/sync.h (1)
src/masternode/sync.cpp (2)
CMasternodeSync(17-23)CMasternodeSync(25-25)
src/chainlock/chainlock.h (1)
src/chainlock/chainlock.cpp (2)
CChainLocksHandler(46-57)CChainLocksHandler(59-63)
src/masternode/active/notificationinterface.h (1)
src/masternode/active/notificationinterface.cpp (2)
ActiveNotificationInterface(13-17)ActiveNotificationInterface(19-19)
src/instantsend/instantsend.h (3)
src/evo/evodb.h (1)
util(12-14)src/instantsend/db.h (1)
util(27-29)src/instantsend/instantsend.cpp (2)
CInstantSendManager(59-72)CInstantSendManager(74-74)
src/dbwrapper.h (10)
src/evo/evodb.h (1)
util(12-14)src/instantsend/db.h (1)
util(27-29)src/instantsend/instantsend.h (1)
util(34-36)src/llmq/context.h (1)
util(32-34)src/llmq/dkgsessionmgr.h (1)
util(36-38)src/llmq/quorums.h (1)
util(40-42)src/llmq/signing.h (1)
util(35-37)src/node/chainstate.h (1)
fs(31-33)src/fs.h (1)
path(42-42)src/dbwrapper.cpp (2)
CDBWrapper(129-183)CDBWrapper(185-197)
src/chainlock/signing.h (1)
src/chainlock/signing.cpp (2)
ChainLockSigner(19-29)ChainLockSigner(31-31)
src/llmq/quorums.h (1)
src/llmq/quorums.cpp (2)
CQuorumManager(210-230)CQuorumManager(232-235)
src/test/util/setup_common.cpp (2)
src/node/chainstate.cpp (2)
DashChainstateSetup(220-261)DashChainstateSetup(220-236)src/node/interfaces.cpp (5)
chainman(576-578)chainman(1061-1063)chainman(1131-1134)MakeChain(1143-1143)MakeChain(1143-1143)
src/evo/mnhftx.h (1)
src/evo/mnhftx.cpp (2)
CMNHFManager(47-52)CMNHFManager(54-58)
src/coinjoin/coinjoin.cpp (1)
src/coinjoin/coinjoin.h (2)
CCoinJoinBaseManager(322-351)CDSTXManager(369-401)
src/llmq/dkgsessionmgr.h (1)
src/llmq/dkgsessionmgr.cpp (2)
CDKGSessionManager(32-61)CDKGSessionManager(63-63)
src/masternode/node.h (1)
src/masternode/node.cpp (2)
CActiveMasternodeManager(52-62)CActiveMasternodeManager(64-64)
src/llmq/signing.h (1)
src/llmq/signing.cpp (4)
CRecoveredSigsDb(30-33)CRecoveredSigsDb(35-35)CSigningManager(335-341)CSigningManager(343-343)
src/dsnotificationinterface.h (1)
src/dsnotificationinterface.cpp (2)
CDSNotificationInterface(23-38)CDSNotificationInterface(40-40)
src/llmq/snapshot.h (1)
src/llmq/snapshot.cpp (2)
CQuorumSnapshotManager(339-343)CQuorumSnapshotManager(345-345)
src/llmq/signing.cpp (1)
src/llmq/signing.h (2)
CRecoveredSigsDb(119-285)CSigningManager(165-251)
src/llmq/debug.cpp (1)
src/llmq/debug.h (1)
CDKGDebugManager(97-121)
src/evo/creditpool.h (1)
src/evo/creditpool.cpp (2)
CCreditPoolManager(241-244)CCreditPoolManager(246-246)
src/llmq/quorums.cpp (1)
src/llmq/utils.cpp (8)
InitQuorumsCache(962-968)InitQuorumsCache(962-962)InitQuorumsCache(969-970)InitQuorumsCache(971-972)InitQuorumsCache(973-978)InitQuorumsCache(979-980)InitQuorumsCache(981-982)InitQuorumsCache(983-984)
src/coinjoin/coinjoin.h (1)
src/coinjoin/coinjoin.cpp (4)
CCoinJoinBaseManager(125-125)CCoinJoinBaseManager(127-127)CDSTXManager(401-401)CDSTXManager(403-403)
src/evo/evodb.h (2)
src/dbwrapper.h (1)
CDBWrapper(214-534)src/evo/evodb.cpp (2)
CEvoDB(34-40)CEvoDB(42-42)
src/test/validation_chainstatemanager_tests.cpp (2)
src/node/chainstate.cpp (2)
DashChainstateSetup(220-261)DashChainstateSetup(220-236)src/test/util/setup_common.cpp (2)
DashChainstateSetup(126-136)DashChainstateSetup(126-130)
src/spork.h (1)
src/spork.cpp (2)
CSporkManager(66-69)CSporkManager(71-75)
src/instantsend/signing.h (1)
src/instantsend/signing.cpp (2)
InstantSendSigner(31-45)InstantSendSigner(47-47)
src/llmq/signing_shares.h (1)
src/llmq/signing_shares.cpp (2)
CSigSharesManager(183-195)CSigSharesManager(197-197)
src/coinjoin/server.h (1)
src/coinjoin/server.cpp (2)
CCoinJoinServer(26-43)CCoinJoinServer(45-45)
src/evo/evodb.cpp (1)
src/evo/evodb.h (1)
CEvoDB(38-76)
src/llmq/context.h (1)
src/llmq/context.cpp (2)
LLMQContext(18-40)LLMQContext(42-44)
src/evo/deterministicmns.h (1)
src/evo/deterministicmns.cpp (2)
CDeterministicMNManager(637-641)CDeterministicMNManager(643-643)
src/dsnotificationinterface.cpp (1)
src/dsnotificationinterface.h (1)
CDSNotificationInterface(18-58)
src/coinjoin/client.h (1)
src/coinjoin/client.cpp (6)
CoinJoinWalletManager(1903-1915)CoinJoinWalletManager(1917-1923)CCoinJoinClientQueueManager(41-48)CCoinJoinClientQueueManager(50-50)CCoinJoinClientManager(160-171)CCoinJoinClientManager(173-173)
src/netfulfilledman.h (1)
src/netfulfilledman.cpp (2)
CNetFulfilledRequestManager(10-13)CNetFulfilledRequestManager(15-19)
src/coinjoin/client.cpp (4)
src/netfulfilledman.cpp (2)
DoMaintenance(92-97)DoMaintenance(92-92)src/evo/deterministicmns.cpp (2)
DoMaintenance(1401-1408)DoMaintenance(1401-1401)src/coinjoin/server.cpp (2)
DoMaintenance(899-907)DoMaintenance(899-899)src/coinjoin/coinjoin.cpp (2)
CheckQueue(135-150)CheckQueue(135-135)
src/llmq/snapshot.cpp (1)
src/llmq/snapshot.h (1)
CQuorumSnapshotManager(216-234)
src/node/chainstate.h (1)
src/fs.h (1)
path(42-42)
🪛 GitHub Actions: Clang Diff Format Check
src/llmq/quorums.cpp
[error] 1-1: Clang-format differences detected in this file. Apply formatting changes and re-run the formatting check.
src/evo/evodb.h
[error] 1-1: Clang-format differences detected in this file. Apply formatting changes and re-run the formatting check.
src/dsnotificationinterface.cpp
[error] 1-1: Clang-format differences detected in this file. Apply formatting changes and re-run the formatting check.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (55)
src/dsnotificationinterface.cpp (3)
30-36: LGTM: Modern initialization style.The switch to brace initialization in the constructor initializer list aligns with modern C++ best practices.
40-40: LGTM: Destructor properly defaulted.The defaulted destructor correctly implements the declaration from the header.
42-46: Behavioral change: Added UpdatedBlockTip call.
InitializeCurrentBlockTipnow invokes bothSynchronousUpdatedBlockTipandUpdatedBlockTip, whereas previously it only called the synchronous version. This ensures both synchronous and asynchronous notification paths are properly initialized at startup.src/llmq/debug.cpp (1)
112-112: LGTM: Destructor properly defaulted.The defaulted destructor correctly implements the declaration from
src/llmq/debug.h.src/chainlock/chainlock.h (1)
73-75: LGTM: Appropriate non-copyable semantics.Deleting the default constructor, copy constructor, and copy assignment operator is appropriate for
CChainLocksHandler, which manages non-copyable resources includingstd::unique_ptrmembers and atomic state.src/evo/mnhftx.h (1)
102-104: LGTM: Enforces explicit construction and non-copyable semantics.Deleting the default constructor, copy constructor, and copy assignment operator is appropriate for
CMNHFManager, which manages global singleton state and complex internal state. This ensures the class can only be constructed via its explicit constructor with required dependencies.src/masternode/node.h (1)
54-58: LGTM: Enforces secure, explicit construction.Deleting special member functions and adding an explicit destructor is appropriate for
CActiveMasternodeManager, which manages sensitive BLS key material and connection references. This prevents accidental copies of key material and ensures proper lifecycle management.src/masternode/sync.h (1)
56-60: LGTM: Enforces explicit construction for state management.Deleting special member functions and adding a destructor is appropriate for
CMasternodeSync, which manages synchronization state and network connection references. This ensures proper dependency injection and prevents state duplication.src/llmq/signing_shares.h (1)
419-421: LGTM: Essential for thread-safe resource management.Deleting special member functions is critical for
CSigSharesManager, which manages worker threads, mutexes, and complex signing state. This prevents accidental copies of non-copyable threading primitives and ensures proper lifecycle management.src/llmq/snapshot.h (1)
226-230: LGTM: Enforces dependency injection pattern.The deleted special member functions combined with the explicit
CEvoDB&-accepting constructor enforce proper dependency injection forCQuorumSnapshotManager. This ensures the snapshot manager always has a valid database reference and prevents accidental copies of state.src/chainlock/signing.h (1)
66-71: LGTM! Non-copyable semantics enforced correctly.The deleted special members ensure
ChainLockSignermust be constructed with explicit dependencies and cannot be accidentally copied. The destructor is properly declared and defaulted in the implementation file.src/dsnotificationinterface.h (1)
21-31: LGTM! Resolved issue from previous review.The deleted default constructor prevents unsafe initialization of reference members, and the non-copyable semantics are appropriate for this validation interface. The destructor is properly declared and defaulted in the implementation file.
Based on past review comments.
src/governance/signing.h (1)
62-68: LGTM! Non-copyable signer enforces explicit construction.The deleted special members ensure
GovernanceSignercan only be constructed with explicit dependencies and cannot be accidentally copied, which is appropriate for a stateful component managing governance signatures.src/netfulfilledman.h (1)
53-56: LGTM! Non-copyable semantics align with unique_ptr member.The deleted copy operations are appropriate given the
unique_ptrmember. The removal ofexplicitfrom the default constructor allows more flexible usage while the constructor properly initializes the database pointer.src/instantsend/instantsend.cpp (1)
59-72: LGTM! Constructor properly refactored to use DbWrapperParams.The constructor signature has been updated to accept
DbWrapperParamsinstead of individual boolean flags, consolidating database configuration. The initialization is correct—passing by const reference without std::move is the appropriate approach here since thedbmember will copy-construct from the parameter.src/masternode/active/notificationinterface.cpp (1)
19-20: LGTM! Proper out-of-line destructor definition.Defining the destructor in the implementation file (even when defaulted) is good practice for classes with forward-declared member types, as it ensures proper destruction semantics and reduces header dependencies.
src/spork.h (1)
236-239: LGTM! Non-copyable semantics appropriate for manager.The deleted copy operations are correct given the
unique_ptrmember. The default constructor and destructor are properly declared and implemented in the source file, maintaining clean ownership semantics for the spork manager.src/test/validation_chainstatemanager_tests.cpp (2)
54-54: LGTM! Test properly configured for in-memory LLMQ databases.The updated parameters correctly configure LLMQ databases to run in-memory for faster test execution, which aligns with the PR's goal of restoring memory-only databases for testing.
87-87: LGTM! Consistent configuration for snapshot chainstate.The parameters are correctly applied to the second chainstate setup, maintaining consistency with the first call and enabling in-memory LLMQ databases for efficient snapshot testing.
src/coinjoin/coinjoin.cpp (1)
125-128: No-op lifetime definitions—LGTM.Defaulted ctor/dtor declarations align with header changes; no runtime impact.
Also applies to: 401-404
src/coinjoin/coinjoin.h (1)
334-336: Expose base manager ctor/dtor—OK.Matches definitions; keeps API explicit.
src/node/chainstate.h (1)
31-33: Forward declare fs::path—OK.By-reference usage makes this safe; avoids extra header coupling.
src/evo/creditpool.h (1)
126-131: Construction semantics verified—all sites compliant.All instantiation sites in the codebase correctly use the explicit constructor:
src/test/util/setup_common.cpp:223:std::make_unique<CCreditPoolManager>(*m_node.evodb)src/node/chainstate.cpp:243:std::make_unique<CCreditPoolManager>(*evodb)Deleted default and copy constructors prevent unintended construction patterns; destructor properly defaults in implementation. No default constructions or copy operations found.
src/init.cpp (1)
1984-1985: Wiring of data_dir and dash_dbs_in_memory parameters is correct and complete.All LoadChainstate call sites have been properly updated:
- src/init.cpp (line 1970): Main node passes
args.GetDataDirNet()and/*dash_dbs_in_memory=*/falsefor persistent databases ✓- src/test/util/setup_common.cpp (line 306): Test setup passes
Assert(m_node.args)->GetDataDirNet()and/*dash_dbs_in_memory=*/truefor in-memory databases ✓The optional suggestion to plumb a hidden arg (e.g.,
-dashdbsinmemory) for dev/regtest scenarios is valid for future enhancement but not required for correctness.src/evo/evodb.cpp (4)
34-39: Ctor wiring looks correct; unique in‑mem namespace preserved.Using util::MakeDbWrapper with db_params.path/"evodb" and memory flag is correct and avoids MemEnv namespace collisions. Cache=64MiB seems reasonable. Based on learnings.
42-42: Defaulted destructor — LGTM.No custom teardown needed; relies on member destructors.
50-54: Rollback via Clear() — LGTM.Consistent with CEvoDBScopedCommitter semantics; no external side effects.
56-64: Root commit path — LGTM.Asserting clean cur tx, committing root, WriteBatch, then clearing batch is correct.
src/llmq/snapshot.cpp (2)
339-346: Ctor/dtor — LGTM.Dependency injection and LRU size 32 are sensible defaults; defaulted dtor is fine.
347-364: Read path and cache usage — LGTM.Cache-then-DB with consistent keying and locking order looks correct.
src/dbwrapper.h (1)
712-725: Params + factory — LGTM.Always passing path (even in memory mode) avoids MemEnv namespace collisions; simple factory keeps call sites uniform.
src/instantsend/db.cpp (2)
29-33: Ctor path and cache — LGTM.Scoped path llmq/isdb with 32MiB cache aligns with other DBs; immediate conditional Upgrade call is appropriate.
37-51: Upgrade logic — LGTM.Version‑gated wipe and reinit via util::MakeDbWrapper, with fSync on version write, looks correct.
src/instantsend/instantsend.h (1)
34-36: Forward declaration — LGTM.Prevents header coupling; fine.
src/llmq/quorums.h (1)
265-273: LGTM: non-copyable CQuorumManager and db_params-based ctor improve ownership clarity.src/llmq/signing.cpp (1)
30-33: LGTM: DB path/params for CRecoveredSigsDb are coherent and scoped under llmq/recsigdb.src/llmq/quorums.cpp (1)
1-1: Manual verification required: clang-format tool unavailable in sandbox environment.Cannot verify the formatting issue directly since clang-format is not available in the sandbox. However, manual inspection of src/llmq/quorums.cpp shows the file already appears properly formatted with standard C++ style, and git status shows no uncommitted changes on the PR branch.
Please manually run
clang-format -i src/llmq/quorums.cpp && git diff -- src/llmq/quorums.cpplocally to confirm whether formatting corrections are actually needed.src/node/chainstate.cpp (1)
51-67: LGTM on threading data_dir and in‑memory flags.Passing data_dir and dash_dbs_in_memory through and constructing CEvoDB with util::DbWrapperParams is clean and aligns with the new API. Passing the base data_dir is correct since CEvoDB appends "evodb" internally.
Also applies to: 76-79
src/llmq/dkgsessionmgr.h (3)
36-39: Minimal forward declaration is fine.Avoids heavy includes in the header while keeping the type visible to users.
84-92: Non‑copyable semantics + explicit ctor look good.Deleting default/copy and taking util::DbWrapperParams cleans up ownership and configuration flow.
94-95: All concerns verified.StopThreads()has an out-of-line definition atsrc/llmq/dkgsessionmgr.cpp:72and is properly invoked inActiveContext::Stop()atsrc/masternode/active/context.cpp:65as part of the shutdown sequence. Clean stop is guaranteed.src/evo/evodb.h (2)
43-57: Ownership + API refactor is appropriate.Switching to std::unique_ptr, deleting copy ops, and exposing an explicit ctor with util::DbWrapperParams all look good. GetRawDB() returning a reference is fine given construction guarantees.
Also applies to: 99-102, 111-111
1-1: Review comment is not applicable — file is already properly formatted.The file
src/evo/evodb.hshows no formatting issues. Git status is clean with no uncommitted changes, and the visible content adheres to the clang-format configuration (C++20, 4-space indent, 120 column limit). No formatting action is required.Likely an incorrect or invalid review comment.
src/test/util/setup_common.cpp (2)
126-136: LGTM: test wiring matches new API.Forwarding data_dir, llmq in‑memory, and wipe flags keeps tests aligned with production wiring.
219-225: Good: CEvoDB memory‑only in BasicTestingSetup.Using .memory = true and .wipe = true avoids file handles and speeds tests. Matches prior discussion.
src/llmq/context.cpp (1)
21-37: LGTM: consolidated db_params propagation.Passing util::DbWrapperParams into DKG/Quorum/Signing/IS managers simplifies configuration and enables memory‑only mode consistently. Init/stop order remains correct.
src/llmq/context.h (1)
40-45: Public API changes look solid.Explicit ctor with util::DbWrapperParams and deleted assignment clarify lifecycle and configuration.
src/coinjoin/client.h (3)
78-85: LGTM! Well-structured dependency injection.The explicit constructor with deleted copy semantics properly enforces single-ownership semantics for this manager class. The dependency injection pattern clearly documents all required dependencies.
228-233: LGTM! Consistent refactoring pattern.The constructor refactoring maintains consistency with the other manager classes, properly enforcing non-copyable semantics and explicit dependency management.
276-282: LGTM! Proper lifecycle management.The constructor properly initializes all dependencies. The explicit constructor and deleted copy operations ensure clear ownership semantics.
src/coinjoin/client.cpp (5)
41-50: LGTM! Clean constructor implementation.The member initializer list properly initializes all reference members, and the defaulted destructor is appropriate since no special cleanup is required.
153-158: LGTM! Proper maintenance guard conditions.The implementation correctly guards queue maintenance with sync and shutdown checks, following the established pattern used in similar components (e.g., CCoinJoinServer::DoMaintenance).
160-173: LGTM! Proper initialization of all dependencies.The constructor correctly initializes all six reference members, and the defaulted destructor is appropriate for this class.
1053-1055: LGTM! Good documentation practice.Adding the
/*onlyValid=*/truecomment improves readability by documenting the purpose of the boolean parameter.
1903-1923: LGTM! Thread-safe destruction pattern.The constructor properly initializes all dependencies. The custom destructor correctly ensures thread-safe cleanup by explicitly resetting the managed
CCoinJoinClientManagerinstances while holding the lock. This guarantees that the managed objects are destroyed before the lock is released, preventing race conditions during cleanup.
| bool llmq_dbs_in_memory, | ||
| bool llmq_dbs_wipe, | ||
| const Consensus::Params& consensus_params); |
There was a problem hiding this comment.
🧩 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*,' srcLength 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.
4 participants
Additional Information
There is a mild performance improvement when restoring memory-only databases in unit tests
Build arguments:
CC=clang CXX=clang++ ./configure --prefix=$(pwd)/depends/arm64-apple-darwin25 --enable-werror --enable-debug --enable-crash-hooks --enable-stacktraces --enable-zmq --enable-suppress-external-warnings --disable-ccache --disable-hardeningdevelop (b82450a):
This PR (969e2c4):
Breaking Changes
None expected.
Checklist