src/test/evo_deterministicmns_tests.cpp (1)

261-264: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the repeated tip-accessor lambdas into a shared helper.

tip_index/tip_height/tip_hash/sync_dmn_tip are redefined nearly identically in ~10 functions in this file. A single helper (e.g. a small struct or free function taking chainman/dmnman by reference) would remove the duplication and centralize the locking pattern for future changes.

♻️ Example helper

struct TipAccessors {
    ChainstateManager& chainman;
    CDeterministicMNManager& dmnman;
    const CBlockIndex* tip_index() const { return WITH_LOCK(::cs_main, return chainman.ActiveChain().Tip()); }
    int tip_height() const { return WITH_LOCK(::cs_main, return chainman.ActiveChain().Height()); }
    uint256 tip_hash() const { return WITH_LOCK(::cs_main, return chainman.ActiveChain().Tip()->GetBlockHash()); }
    void sync_dmn_tip() const { dmnman.UpdatedBlockTip(tip_index()); }
};

Also applies to: 299-301, 440-441, 539-540, 598-599, 706-708, 941-942, 989-991, 1180-1182, 1329-1332

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/test/evo_deterministicmns_tests.cpp` around lines 261 - 264, Introduce
one shared helper for the repeated tip accessors, such as a TipAccessors struct
holding ChainstateManager and CDeterministicMNManager references, with methods
for tip_index, tip_height, tip_hash, and sync_dmn_tip that preserve the existing
locking and update behavior. Replace the local lambdas in all affected test
functions with this helper and update their call sites accordingly.

src/dbwrapper.h (1)

338-344: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider making StoragePath() const.

It only reads m_is_memory/m_path, so marking it const lets it be called through a const CDBWrapper& and better expresses intent.

♻️ Proposed change

-    //! `@returns` filesystem path to the on-disk data.
-    std::optional<fs::path> StoragePath() {
+    //! `@returns` filesystem path to the on-disk data.
+    std::optional<fs::path> StoragePath() const {
         if (m_is_memory) {
             return {};
         }
         return m_path;
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/dbwrapper.h` around lines 338 - 344, Mark the CDBWrapper::StoragePath()
method const, since it only reads m_is_memory and m_path. Preserve its existing
return behavior for memory-backed and on-disk storage.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/test/evo_deterministicmns_tests.cpp (1)

261-264: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the repeated tip-accessor lambdas into a shared helper.

tip_index/tip_height/tip_hash/sync_dmn_tip are redefined nearly identically in ~10 functions in this file. A single helper (e.g. a small struct or free function taking chainman/dmnman by reference) would remove the duplication and centralize the locking pattern for future changes.

♻️ Example helper

struct TipAccessors {
    ChainstateManager& chainman;
    CDeterministicMNManager& dmnman;
    const CBlockIndex* tip_index() const { return WITH_LOCK(::cs_main, return chainman.ActiveChain().Tip()); }
    int tip_height() const { return WITH_LOCK(::cs_main, return chainman.ActiveChain().Height()); }
    uint256 tip_hash() const { return WITH_LOCK(::cs_main, return chainman.ActiveChain().Tip()->GetBlockHash()); }
    void sync_dmn_tip() const { dmnman.UpdatedBlockTip(tip_index()); }
};

Also applies to: 299-301, 440-441, 539-540, 598-599, 706-708, 941-942, 989-991, 1180-1182, 1329-1332

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/test/evo_deterministicmns_tests.cpp` around lines 261 - 264, Introduce
one shared helper for the repeated tip accessors, such as a TipAccessors struct
holding ChainstateManager and CDeterministicMNManager references, with methods
for tip_index, tip_height, tip_hash, and sync_dmn_tip that preserve the existing
locking and update behavior. Replace the local lambdas in all affected test
functions with this helper and update their call sites accordingly.

src/dbwrapper.h (1)

338-344: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider making StoragePath() const.

It only reads m_is_memory/m_path, so marking it const lets it be called through a const CDBWrapper& and better expresses intent.

♻️ Proposed change

-    //! `@returns` filesystem path to the on-disk data.
-    std::optional<fs::path> StoragePath() {
+    //! `@returns` filesystem path to the on-disk data.
+    std::optional<fs::path> StoragePath() const {
         if (m_is_memory) {
             return {};
         }
         return m_path;
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/dbwrapper.h` around lines 338 - 344, Mark the CDBWrapper::StoragePath()
method const, since it only reads m_is_memory and m_path. Preserve its existing
return behavior for memory-backed and on-disk storage.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/coinjoin/interfaces.cpp (1)

35-35: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Default value mismatch for -enablecoinjoin between help text and runtime default.

The constructor sets CCoinJoinClientOptions::SetEnabled(gArgs.GetBoolArg("-enablecoinjoin", true)) with a default of true, but the help text in src/wallet/init.cpp line 107 documents the default as 0 (disabled):

argsman.AddArg("-enablecoinjoin", strprintf("Enable use of CoinJoin for funds stored in this wallet (0-1, default: %u)", 0), ...)

Users who omit -enablecoinjoin will have CoinJoin enabled at runtime, contradicting the documented default. Either change the runtime default to false to match the help text, or update the help text to reflect default: 1.

🔧 Proposed fix (align runtime default to help text)

-        CCoinJoinClientOptions::SetEnabled(gArgs.GetBoolArg("-enablecoinjoin", true));
+        CCoinJoinClientOptions::SetEnabled(gArgs.GetBoolArg("-enablecoinjoin", false));

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

        CCoinJoinClientOptions::SetEnabled(gArgs.GetBoolArg("-enablecoinjoin", false));

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/coinjoin/interfaces.cpp` at line 35, Align the runtime default in
CCoinJoinClientOptions initialization with the documented disabled default by
changing the fallback passed to gArgs.GetBoolArg for -enablecoinjoin from true
to false. Leave the existing help text and option behavior unchanged otherwise.

src/node/utxo_snapshot.h (1)

9-16: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -n '`#include`' src/validation.h | rg -i 'utxo_snapshot'
rg -n '`#include`' src/node/utxo_snapshot.h
rg -n 'class Chainstate' src/node/utxo_snapshot.h src/validation.h
rg -n 'extern RecursiveMutex cs_main' -A1 src/node/blockstorage.h src/node/utxo_snapshot.h

Repository: PastaPastaPasta/dash

Length of output: 664

---

🏁 Script executed:

#!/bin/bash
sed -n '1,120p' src/node/utxo_snapshot.h
printf '\n--- validation.h excerpt ---\n'
sed -n '1,120p' src/validation.h
printf '\n--- references in utxo_snapshot.h ---\n'
rg -n 'Chainstate|SnapshotMetadata|cs_main|WriteSnapshotBaseBlockhash|ReadSnapshotBaseBlockhash' src/node/utxo_snapshot.h src/validation.h

Repository: PastaPastaPasta/dash

Length of output: 19311

---

Drop <validation.h> here. WriteSnapshotBaseBlockhash only needs a forward declaration of Chainstate, and the extra extern RecursiveMutex cs_main; is redundant with the declaration already pulled in by validation.h. If you keep the local declaration, add the same NOLINT(readability-redundant-declaration) used in src/node/blockstorage.h.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/node/utxo_snapshot.h` around lines 9 - 16, Remove the unnecessary
<validation.h> include from the header and provide only the forward declaration
of Chainstate required by WriteSnapshotBaseBlockhash. Remove the redundant local
cs_main declaration, or retain it only with the established
NOLINT(readability-redundant-declaration) annotation matching
src/node/blockstorage.h.

src/rpc/coinjoin.cpp (1)

281-286: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Inconsistent error handling for WithClient return value in salt RPCs.

The coinjoinsalt generate and coinjoinsalt set handlers ignore the bool return of WithClient. If the wallet is not found by the loader (returns false), the isMixing() check is silently bypassed and salt generation proceeds without verifying mixing is inactive. All other CoinJoin RPC handlers in this file use CHECK_NONFATAL to assert the wallet is found.

If the wallet was already validated by GetWalletForJSONRPCRequest, the WithClient call should always succeed — but the defensive gap means a race or unexpected loader state would allow salt changes during active mixing rather than failing safely.

Consider either asserting the return value (matching the other handlers) or returning an RPC error when WithClient returns false.

🔧 Suggested fix for `coinjoinsalt generate`

     const NodeContext& node = EnsureAnyNodeContext(request.context);
-    if (node.coinjoin_loader != nullptr) {
-        node.coinjoin_loader->WithClient(wallet->GetName(), [&](auto& client) {
+    CHECK_NONFATAL(CHECK_NONFATAL(node.coinjoin_loader)->WithClient(wallet->GetName(), [&](auto& client) {
         if (client.isMixing()) {
             throw JSONRPCError(RPC_WALLET_ERROR,
                                strprintf("Wallet \"%s\" is currently mixing, cannot change salt!", str_wallet));
         }
-    });
-    }
+    }));

Also applies to: 384-389

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/rpc/coinjoin.cpp` around lines 281 - 286, Update the salt validation
callbacks in both the `coinjoinsalt generate` and `coinjoinsalt set` handlers to
check the boolean returned by `node.coinjoin_loader->WithClient`. Assert the
wallet lookup succeeds using the file’s established `CHECK_NONFATAL` pattern, so
a failed lookup cannot bypass the `client.isMixing()` guard.