src/util/vector.h (1)

72-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider using std::ranges::find_if for idiomatic C++20.

As per coding guidelines, C++20 is the target standard. You can leverage the <algorithm> library and std::ranges::find_if to make the implementation more idiomatic. Additionally, taking the predicate by forwarding reference (L&&) instead of const L can avoid unnecessary copies of the callable and support mutable lambdas if ever needed.

♻️ Proposed refactor

Make sure to include <algorithm> at the top of the file:

+#include <algorithm>

Then update the FindFirst function:

-template<typename V, typename L>
-inline std::optional<V> FindFirst(const std::vector<V>& vec, const L fnc)
-{
-    for (const auto& el : vec) {
-        if (fnc(el)) {
-            return el;
-        }
-    }
-    return std::nullopt;
-}
+template<typename V, typename L>
+inline std::optional<V> FindFirst(const std::vector<V>& vec, L&& fnc)
+{
+    auto it = std::ranges::find_if(vec, std::forward<L>(fnc));
+    return it != vec.end() ? std::make_optional(*it) : std::nullopt;
+}

🤖 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/util/vector.h` around lines 72 - 82, Update the FindFirst template to use
C++20 std::ranges::find_if, include the required <algorithm> header, and accept
the predicate as L&& so callable state is preserved without an unnecessary copy.
Return the matching element as an optional value, or std::nullopt when no
element matches.

Source: Coding guidelines

src/init.cpp (1)

2168-2171: 📐 Maintainability & Code Quality | 🔵 Trivial

Reminder: Address the TODO comment.

The TODO comment outlines pending work to recreate/rebind ActiveContext signers and NetInstantSend objects for runtime loadtxoutset when the ChainstateManager switches active chainstates.
Do you want me to track this task by opening a new issue, or provide assistance with implementing the rebinding logic?

🤖 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/init.cpp` around lines 2168 - 2171, Address the TODO in the runtime
loadtxoutset path by recreating or rebinding the Chainstate&-holding
ActiveContext signers and NetInstantSend after ChainstateManager switches active
chainstates. Preserve the existing startup detection and completion-time
ResetChainstates behavior, and ensure these objects reference the newly active
chainstate before runtime processing continues.

src/evo/deterministicmns.cpp (1)

376-387: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use %" PRIu64 " or %llu for 64-bit unsigned integers in strprintf.

Although tinyformat processes type-safety gracefully and often ignores the format character byte size, passing a uint64_t to a %d (signed 32-bit int) format specifier is technically incorrect and may trigger -Wformat warnings with modern compiler-level format checkers. Consider using %" PRIu64 " or %llu across these methods.

• src/evo/deterministicmns.cpp#L376-L387: update the %d format specifiers for id and p.first in ApplyDiff.
• src/evo/deterministicmns.cpp#L389-L415: update the %d format specifiers for id and p.first in ApplyDiffForSnapshot.

🤖 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/evo/deterministicmns.cpp` around lines 376 - 387, Update the format
specifiers for the uint64_t identifiers in ApplyDiff and ApplyDiffForSnapshot,
replacing each %d used for id and p.first with a 64-bit unsigned format such as
PRIu64 or %llu. Apply the change at src/evo/deterministicmns.cpp lines 376-387
and 389-415; no other behavior should change.

test/functional/test_framework/test_node.py (2)

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

Fix late-binding lint warning.

The linter flags that rpc is a loop variable captured in a lambda. While this is not functionally a bug here (since wait_until_helper_internal executes synchronously and the lambda doesn't outlive the iteration), you can bind it as a default argument to resolve the static analysis warning and prevent future fragility.

💡 Proposed fix

-                    wait_until_helper_internal(lambda: rpc.getmempoolinfo()['loaded'])
+                    wait_until_helper_internal(lambda rpc=rpc: rpc.getmempoolinfo()['loaded'])

🤖 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 `@test/functional/test_framework/test_node.py` at line 288, Update the lambda
passed to wait_until_helper_internal in the loop so the current rpc value is
bound through a default argument, eliminating late binding while preserving the
existing getmempoolinfo()['loaded'] check.

Source: Linters/SAST tools

---

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

Fix late-binding lint warning.

The linter flags that rpc is a loop variable captured in a lambda. While this is not functionally a bug here (since wait_until_helper_internal executes synchronously and the lambda doesn't outlive the iteration), you can bind it as a default argument to resolve the static analysis warning.

💡 Proposed fix

-                    wait_until_helper_internal(lambda: rpc.getmempoolinfo()['loaded'])
+                    wait_until_helper_internal(lambda rpc=rpc: rpc.getmempoolinfo()['loaded'])

🤖 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 `@test/functional/test_framework/test_node.py` at line 288, Update the lambda
passed to wait_until_helper_internal in the loop to bind the current rpc value
as a default argument, preventing late-binding lint warnings while preserving
the existing loaded-state check.

Source: Linters/SAST tools

src/kernel/coinstats.cpp (1)

92-100: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Avoid copying the Coin object and use a structured binding.

Inside the loop, Coin coin = it->second; creates an unnecessary copy of the Coin object (which includes a CTxOut containing a CScript vector). You can avoid this allocation and make the loop more readable by using a structured binding with a const auto& reference.

♻️ Proposed refactor

 template <typename T>
 static void ApplyHash(T& hash_obj, const uint256& hash, const std::map<uint32_t, Coin>& outputs)
 {
-    for (auto it = outputs.begin(); it != outputs.end(); ++it) {
-        COutPoint outpoint = COutPoint(hash, it->first);
-        Coin coin = it->second;
-        ApplyCoinHash(hash_obj, outpoint, coin);
+    for (const auto& [n, coin] : outputs) {
+        COutPoint outpoint(hash, n);
+        ApplyCoinHash(hash_obj, outpoint, coin);
     }
 }

🤖 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/kernel/coinstats.cpp` around lines 92 - 100, Update ApplyHash to iterate
over outputs using a const structured-binding reference, reusing the referenced
output index and Coin directly when constructing COutPoint and calling
ApplyCoinHash; remove the local Coin copy while preserving the existing hashing
behavior.

src/interfaces/chain.h (1)

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

Remove duplicate forward declaration.

The forward declaration for enum class RBFTransactionState is duplicated.

♻️ Proposed refactor

 enum class RBFTransactionState;
-enum class RBFTransactionState;

🤖 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/interfaces/chain.h` at line 34, Remove the duplicate enum class
RBFTransactionState forward declaration from the interface, retaining the
existing declaration elsewhere and leaving all dependent references unchanged.

Fix all unresolved CodeRabbit comments on this PR:

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

src/util/vector.h (1)

72-82: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider using std::ranges::find_if for idiomatic C++20.

As per coding guidelines, C++20 is the target standard. You can leverage the <algorithm> library and std::ranges::find_if to make the implementation more idiomatic. Additionally, taking the predicate by forwarding reference (L&&) instead of const L can avoid unnecessary copies of the callable and support mutable lambdas if ever needed.

♻️ Proposed refactor

Make sure to include <algorithm> at the top of the file:

+#include <algorithm>

Then update the FindFirst function:

-template<typename V, typename L>
-inline std::optional<V> FindFirst(const std::vector<V>& vec, const L fnc)
-{
-    for (const auto& el : vec) {
-        if (fnc(el)) {
-            return el;
-        }
-    }
-    return std::nullopt;
-}
+template<typename V, typename L>
+inline std::optional<V> FindFirst(const std::vector<V>& vec, L&& fnc)
+{
+    auto it = std::ranges::find_if(vec, std::forward<L>(fnc));
+    return it != vec.end() ? std::make_optional(*it) : std::nullopt;
+}

🤖 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/util/vector.h` around lines 72 - 82, Update the FindFirst template to use
C++20 std::ranges::find_if, include the required <algorithm> header, and accept
the predicate as L&& so callable state is preserved without an unnecessary copy.
Return the matching element as an optional value, or std::nullopt when no
element matches.

Source: Coding guidelines

src/init.cpp (1)

2168-2171: 📐 Maintainability & Code Quality | 🔵 Trivial

Reminder: Address the TODO comment.

The TODO comment outlines pending work to recreate/rebind ActiveContext signers and NetInstantSend objects for runtime loadtxoutset when the ChainstateManager switches active chainstates.
Do you want me to track this task by opening a new issue, or provide assistance with implementing the rebinding logic?

🤖 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/init.cpp` around lines 2168 - 2171, Address the TODO in the runtime
loadtxoutset path by recreating or rebinding the Chainstate&-holding
ActiveContext signers and NetInstantSend after ChainstateManager switches active
chainstates. Preserve the existing startup detection and completion-time
ResetChainstates behavior, and ensure these objects reference the newly active
chainstate before runtime processing continues.

src/evo/deterministicmns.cpp (1)

376-387: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use %" PRIu64 " or %llu for 64-bit unsigned integers in strprintf.

Although tinyformat processes type-safety gracefully and often ignores the format character byte size, passing a uint64_t to a %d (signed 32-bit int) format specifier is technically incorrect and may trigger -Wformat warnings with modern compiler-level format checkers. Consider using %" PRIu64 " or %llu across these methods.

• src/evo/deterministicmns.cpp#L376-L387: update the %d format specifiers for id and p.first in ApplyDiff.
• src/evo/deterministicmns.cpp#L389-L415: update the %d format specifiers for id and p.first in ApplyDiffForSnapshot.

🤖 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/evo/deterministicmns.cpp` around lines 376 - 387, Update the format
specifiers for the uint64_t identifiers in ApplyDiff and ApplyDiffForSnapshot,
replacing each %d used for id and p.first with a 64-bit unsigned format such as
PRIu64 or %llu. Apply the change at src/evo/deterministicmns.cpp lines 376-387
and 389-415; no other behavior should change.

test/functional/test_framework/test_node.py (2)

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

Fix late-binding lint warning.

The linter flags that rpc is a loop variable captured in a lambda. While this is not functionally a bug here (since wait_until_helper_internal executes synchronously and the lambda doesn't outlive the iteration), you can bind it as a default argument to resolve the static analysis warning and prevent future fragility.

💡 Proposed fix

-                    wait_until_helper_internal(lambda: rpc.getmempoolinfo()['loaded'])
+                    wait_until_helper_internal(lambda rpc=rpc: rpc.getmempoolinfo()['loaded'])

🤖 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 `@test/functional/test_framework/test_node.py` at line 288, Update the lambda
passed to wait_until_helper_internal in the loop so the current rpc value is
bound through a default argument, eliminating late binding while preserving the
existing getmempoolinfo()['loaded'] check.

Source: Linters/SAST tools

---

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

Fix late-binding lint warning.

The linter flags that rpc is a loop variable captured in a lambda. While this is not functionally a bug here (since wait_until_helper_internal executes synchronously and the lambda doesn't outlive the iteration), you can bind it as a default argument to resolve the static analysis warning.

💡 Proposed fix

-                    wait_until_helper_internal(lambda: rpc.getmempoolinfo()['loaded'])
+                    wait_until_helper_internal(lambda rpc=rpc: rpc.getmempoolinfo()['loaded'])

🤖 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 `@test/functional/test_framework/test_node.py` at line 288, Update the lambda
passed to wait_until_helper_internal in the loop to bind the current rpc value
as a default argument, preventing late-binding lint warnings while preserving
the existing loaded-state check.

Source: Linters/SAST tools

src/kernel/coinstats.cpp (1)

92-100: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Avoid copying the Coin object and use a structured binding.

Inside the loop, Coin coin = it->second; creates an unnecessary copy of the Coin object (which includes a CTxOut containing a CScript vector). You can avoid this allocation and make the loop more readable by using a structured binding with a const auto& reference.

♻️ Proposed refactor

 template <typename T>
 static void ApplyHash(T& hash_obj, const uint256& hash, const std::map<uint32_t, Coin>& outputs)
 {
-    for (auto it = outputs.begin(); it != outputs.end(); ++it) {
-        COutPoint outpoint = COutPoint(hash, it->first);
-        Coin coin = it->second;
-        ApplyCoinHash(hash_obj, outpoint, coin);
+    for (const auto& [n, coin] : outputs) {
+        COutPoint outpoint(hash, n);
+        ApplyCoinHash(hash_obj, outpoint, coin);
     }
 }

🤖 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/kernel/coinstats.cpp` around lines 92 - 100, Update ApplyHash to iterate
over outputs using a const structured-binding reference, reusing the referenced
output index and Coin directly when constructing COutPoint and calling
ApplyCoinHash; remove the local Coin copy while preserving the existing hashing
behavior.

src/interfaces/chain.h (1)

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

Remove duplicate forward declaration.

The forward declaration for enum class RBFTransactionState is duplicated.

♻️ Proposed refactor

 enum class RBFTransactionState;
-enum class RBFTransactionState;

🤖 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/interfaces/chain.h` at line 34, Remove the duplicate enum class
RBFTransactionState forward declaration from the interface, retaining the
existing declaration elsewhere and leaving all dependent references unchanged.

Fix all unresolved CodeRabbit comments on this PR:

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

src/evo/snapshot.cpp (1)

232-316: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf 'Repository root: '
pwd

echo '--- outline snapshot.cpp ---'
ast-grep outline src/evo/snapshot.cpp --view expanded || true

echo '--- line counts ---'
wc -l src/evo/snapshot.cpp src/evo/snapshot.h || true

echo '--- targeted ranges snapshot.cpp ---'
sed -n '300,520p' src/evo/snapshot.cpp

echo '--- targeted ranges snapshot.h ---'
sed -n '1,260p' src/evo/snapshot.h

Repository: PastaPastaPasta/dash

Length of output: 23253

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- search for base_block_hash comparisons and historical_hashes ---'
rg -n "base_block_hash|historical_hashes|required_work_hashes|ValidateEvoSnapshotAgainstChain|BuildEvoSnapshot|register_work_block" src/evo/snapshot.cpp src/evo/snapshot.h

echo '--- search for Validate() calls ---'
rg -n "\.Validate\(" src/evo/snapshot.cpp src/evo/snapshot.h src | head -n 100

Repository: PastaPastaPasta/dash

Length of output: 8699

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '232,316p' src/evo/snapshot.cpp
printf '\n---\n'
sed -n '340,500p' src/evo/snapshot.cpp
printf '\n---\n'
sed -n '580,640p' src/evo/snapshot.cpp
printf '\n---\n'
rg -n "Validate\\(true\\)|Validate\\(\\)" src/evo/snapshot.h src/evo/snapshot.cpp

Repository: PastaPastaPasta/dash

Length of output: 18340

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Look for comments/tests around evo snapshot construction and validation edge cases.
rg -n "snapshot.*base_block_hash|work_block_hash.*base_block_hash|historical.*base_block_hash|assumeutxo|non-rotated|V20|WORK_DIFF_DEPTH" src test contrib -g '!**/node_modules/**' || true

Repository: PastaPastaPasta/dash

Length of output: 50376

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- snapshot.cpp (240-520) ---'
sed -n '240,520p' src/evo/snapshot.cpp

echo '--- snapshot.cpp (580-635) ---'
sed -n '580,635p' src/evo/snapshot.cpp

echo '--- snapshot.h Validate declarations ---'
rg -n "Validate\\(" src/evo/snapshot.h

Repository: PastaPastaPasta/dash

Length of output: 19088

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- snapshot.h around serializer/unserializer ---'
sed -n '500,590p' src/evo/snapshot.h

echo '--- targeted tests around base/work hash edge cases ---'
sed -n '600,690p' src/test/evo_snapshot_tests.cpp

echo '--- any commentary around work/base equality in quorum utils ---'
sed -n '1110,1135p' src/rpc/quorums.cpp

Repository: PastaPastaPasta/dash

Length of output: 11338

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- reconstruction helper ---'
sed -n '190,230p' src/evo/snapshot.cpp

echo '--- test cases near line 653 and rotation/work hash behavior ---'
sed -n '640,760p' src/test/evo_snapshot_tests.cpp

echo '--- search for historical diff target mismatch tests ---'
rg -n "missing or extra historical|base_block_hash|work_block_hash = entry.quorum_base_block_hash|ValidateEvoSnapshotAgainstChain" src/test/evo_snapshot_tests.cpp

Repository: PastaPastaPasta/dash

Length of output: 11882

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

python3 - <<'PY'
from pathlib import Path
text = Path('src/evo/snapshot.cpp').read_text()

# Print the exact helper and the chain-validation historical comparison area.
for start, end, label in [
    (190, 230, 'reconstruct helper'),
    (530, 620, 'chain validation historical area'),
]:
    print(f'--- {label} ---')
    lines = text.splitlines()
    for i in range(start-1, min(end, len(lines))):
        print(f'{i+1}: {lines[i]}')
PY

Repository: PastaPastaPasta/dash

Length of output: 8126

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- evo snapshot tests around manual construction ---'
sed -n '620,700p' src/test/evo_snapshot_tests.cpp

echo '--- evo snapshot tests around ValidateEvoSnapshotAgainstChain historical comparison ---'
sed -n '760,860p' src/test/evo_snapshot_tests.cpp

Repository: PastaPastaPasta/dash

Length of output: 10301

---

Exclude base_block_hash before comparing historical work targets
CEvoSnapshot::Validate() should mirror ValidateEvoSnapshotAgainstChain() and erase base_block_hash from both sets before the historical_hashes != required_work_hashes check. BuildEvoSnapshot() can produce a work block at the snapshot base for non-rotated LLMQs before V20, and that snapshot will otherwise fail validation on build or load.

🤖 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/evo/snapshot.cpp` around lines 232 - 316, Update CEvoSnapshot::Validate()
so base_block_hash is removed from both historical_hashes and
required_work_hashes before comparing them. Keep the existing reconstruction and
required-target collection unchanged, and perform the exclusion immediately
before the historical_hashes != required_work_hashes check to match
ValidateEvoSnapshotAgainstChain().

src/evo/specialtxman.cpp (1)

783-784: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate the relevant implementation and call sites.
rg -n "CheckCbTxBestChainlock|best ChainLock|ChainLock" src/evo/specialtxman.cpp src -g '!**/node_modules/**'

# Show the surrounding context for the validator and the call site.
sed -n '80,150p' src/evo/specialtxman.cpp
printf '\n----\n'
sed -n '760,810p' src/evo/specialtxman.cpp

Repository: PastaPastaPasta/dash

Length of output: 50376

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '97,175p' src/evo/specialtxman.cpp
printf '\n----\n'
sed -n '770,800p' src/evo/specialtxman.cpp
printf '\n----\n'
sed -n '1,120p' src/chainlock/clsig.cpp

Repository: PastaPastaPasta/dash

Length of output: 7496

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the helper and the existing cbtx tests.
sed -n '1,140p' src/evo/specialtxman.cpp
printf '\n----\n'
sed -n '1,140p' src/test/evo_cbtx_tests.cpp
printf '\n----\n'
rg -n "GetNonNullCoinbaseChainlock|cached_chainlock|bestCLHeightDiff|bestCLSignature" src/evo src/test -g '!**/node_modules/**'

Repository: PastaPastaPasta/dash

Length of output: 14085

---

Don't short-circuit ChainLock validation on non-active chainstates.

CheckCbTxBestChainlock can return true from the height/signature fast path without checking that the ChainLock matches this block’s previous hash. A divergent chainstate can then accept the active chain’s best ChainLock instead of validating its own ancestor. Require the block-hash match here, or disable this shortcut off the active chain.

🤖 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/evo/specialtxman.cpp` around lines 783 - 784, Update the ChainLock
validation around CheckCbTxBestChainlock in the special transaction processing
path so non-active chainstates cannot accept a ChainLock from a divergent chain.
Require the ChainLock block hash to match the candidate block’s previous hash
before treating validation as successful, or disable the height/signature
shortcut whenever the chainstate is not active.

src/kernel/chain.h (1)

8-47: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

Remove duplicate declarations and prevent compiler errors.

The forward declarations for CBlock, CBlockIndex, interfaces::BlockInfo, and the function declaration kernel::MakeBlockInfo are duplicated. In C++, specifying a default argument for a parameter multiple times in the same scope (as done with const CBlock* data = nullptr on lines 18 and 45) will cause a compilation error.

Please remove the redundant declarations.

🐛 Proposed fix

 `#include`<iostream>
 
-class CBlock;
-class CBlockIndex;
-namespace interfaces {
-struct BlockInfo;
-} // namespace interfaces
-
-namespace kernel {
-//! Return data from block index.
-interfaces::BlockInfo MakeBlockInfo(const CBlockIndex* block_index, const CBlock* data = nullptr);
-
-} // namespace kernel
-
 class CBlock;
 class CBlockIndex;

📝 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.

`#include`<iostream>

class CBlock;
class CBlockIndex;
class CBlock;
class CBlockIndex;
class CBlockUndo;
class uint256;
namespace interfaces {
//! Block data sent with blockConnected and blockDisconnected notifications.
struct BlockInfo {
    const uint256& hash;
    const uint256* prev_hash{nullptr};
    int height{-1};
    int file_number{-1};
    unsigned data_pos{0};
    const CBlock* data{nullptr};
    const CBlockUndo* undo_data{nullptr};
    unsigned int chain_time_max{0};

    explicit BlockInfo(const uint256& block_hash) : hash(block_hash) {}
};
} // namespace interfaces

namespace kernel {
struct ChainstateRole;
//! Return data from block index.
interfaces::BlockInfo MakeBlockInfo(const CBlockIndex* block_index, const CBlock* data = nullptr);
std::ostream& operator<<(std::ostream& os, const ChainstateRole& role);
} // namespace kernel

🤖 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/kernel/chain.h` around lines 8 - 47, Remove the redundant forward
declarations of CBlock, CBlockIndex, and interfaces::BlockInfo, and keep only
one kernel::MakeBlockInfo declaration. Ensure the default nullptr argument for
MakeBlockInfo appears exactly once, while preserving the remaining BlockInfo
definition and ChainstateRole operator declaration.

src/net.h (1)

1506-1510: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
ast-grep outline src/net.h --match 'AddLocalServices|RemoveLocalServices' --view expanded
rg -nP -C3 '\b(AddLocalServices|RemoveLocalServices)\s*\(' src

Repository: PastaPastaPasta/dash

Length of output: 2054

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== src/net.h: around local-services helpers and member =="
sed -n '1490,1525p' src/net.h
echo
sed -n '1835,1865p' src/net.h

echo
echo "== search for lock annotations / related fields =="
rg -n "nLocalServices|EXCLUSIVE_LOCKS_REQUIRED|GUARDED_BY|LOCKS_EXCLUDED|Mutex|mutex" src/net.h src/net.cpp src/init.cpp src/rpc/blockchain.cpp

echo
echo "== search for all writes to nLocalServices =="
rg -n "nLocalServices\s*([=|&]|\.store|\.fetch|compare_exchange|load)" src

Repository: PastaPastaPasta/dash

Length of output: 41172

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== src/init.cpp around nLocalServices mutations =="
sed -n '1075,1115p' src/init.cpp
echo
sed -n '1188,1220p' src/init.cpp
echo
sed -n '1390,1415p' src/init.cpp
echo
sed -n '2355,2375p' src/init.cpp
echo
sed -n '2605,2630p' src/init.cpp

echo
echo "== src/rpc/blockchain.cpp around AddLocalServices/RemoveLocalServices =="
sed -n '3148,3165p' src/rpc/blockchain.cpp

echo
echo "== src/net.cpp GetLocalServices implementation and any locks nearby =="
sed -n '4660,4690p' src/net.cpp

Repository: PastaPastaPasta/dash

Length of output: 9546

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n "using ServiceFlags|typedef.*ServiceFlags|enum class ServiceFlags|enum ServiceFlags" src

Repository: PastaPastaPasta/dash

Length of output: 306

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n -C3 "GetLocalServices\(" src/net.cpp src/net.h src/init.cpp src/rpc/blockchain.cpp src/net_processing.cpp

Repository: PastaPastaPasta/dash

Length of output: 4462

---

Make local-service updates atomic RMWs. AddLocalServices/RemoveLocalServices still do separate load/store operations on std::atomic<ServiceFlags>, so concurrent updates can drop bits and advertise the wrong handshake services. Use fetch_or/fetch_and or a CAS loop here.

🤖 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/net.h` around lines 1506 - 1510, Update AddLocalServices and
RemoveLocalServices to perform atomic read-modify-write operations on
nLocalServices using fetch_or and fetch_and (or equivalent CAS loops),
preserving the existing bitwise add/remove semantics without separate loads and
stores.

Source: Coding guidelines

src/rpc/blockchain.cpp (1)

2870-2871: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

Reject rollback targets outside the active chain before dereferencing invalidate_index.

ParseHashOrHeight can return a block on a stale fork. ActiveChain().Next(target_index) then returns null, and Line 2871 dereferences it, allowing an RPC request to crash the node.

Proposed guard

 invalidate_index = WITH_LOCK(::cs_main, return node.chainman->ActiveChain().Next(target_index));
+if (!invalidate_index) {
+    throw JSONRPCError(
+        RPC_INVALID_PARAMETER,
+        "Rollback target must be an ancestor of the active chain tip.");
+}
 InvalidateBlock(*node.chainman, invalidate_index->GetBlockHash());

📝 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.

        invalidate_index = WITH_LOCK(::cs_main, return node.chainman->ActiveChain().Next(target_index));
        if (!invalidate_index) {
            throw JSONRPCError(
                RPC_INVALID_PARAMETER,
                "Rollback target must be an ancestor of the active chain tip.");
        }
        InvalidateBlock(*node.chainman, invalidate_index->GetBlockHash());

🤖 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/blockchain.cpp` around lines 2870 - 2871, Validate the result of
ActiveChain().Next(target_index) in the rollback handling flow before calling
GetBlockHash. If invalidate_index is null because target_index is outside the
active chain, reject the RPC request using the existing error-reporting path;
only invoke InvalidateBlock after confirming a valid active-chain block.

src/test/validation_chainstatemanager_tests.cpp (1)

1085-1108: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Global Consensus::Params mutation isn't restored if an assertion in between fails — risk of cascading unrelated test failures.

mutable_consensus.DIP0003Height is set to 1 on the process-wide Params().GetConsensus() singleton, and only restored by an unconditional statement placed after MaybeValidateSnapshot(...) (Line 1107). Between the mutation and the restore sits BOOST_REQUIRE(m_node.evodb->CommitRootTransaction(...)) (Line 1103) and the MaybeValidateSnapshot call itself — if either throws or the REQUIRE fails, Boost.Test aborts the test case and the restore is skipped, permanently corrupting DIP0003Height for every subsequent test case in the same binary run.

🛡️ Proposed fix: restore via RAII regardless of how the test case exits

     Consensus::Params& mutable_consensus{
         const_cast<Consensus::Params&>(Params().GetConsensus())};
     const int old_dip3_height{mutable_consensus.DIP0003Height};
     mutable_consensus.DIP0003Height = 1;
+    struct DIP3HeightGuard {
+        Consensus::Params& params;
+        int original_height;
+        ~DIP3HeightGuard() { params.DIP0003Height = original_height; }
+    } dip3_height_guard{mutable_consensus, old_dip3_height};
 
     {
         auto tx = m_node.evodb->BeginTransaction(EvoDbIdentity::SNAPSHOT);
@@
     const auto result = WITH_LOCK(::cs_main,
         return chainman.MaybeValidateSnapshot(*validation_chainstate, *snapshot_chainstate, [](bilingual_str) {}));
-    mutable_consensus.DIP0003Height = old_dip3_height;
     BOOST_CHECK_EQUAL(result, SnapshotCompletionResult::EVO_STATE_MISMATCH);

📝 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.

    m_node.dmnman->SetListForBlockForTesting(incorrect_base_list);

    // The regtest assumeutxo fixture predates DIP3. Temporarily make DIP3 active
    // so the legacy completion lookup would consume the poisoned shared cache.
    Consensus::Params& mutable_consensus{
        const_cast<Consensus::Params&>(Params().GetConsensus())};
    const int old_dip3_height{mutable_consensus.DIP0003Height};
    mutable_consensus.DIP0003Height = 1;
    struct DIP3HeightGuard {
        Consensus::Params& params;
        int original_height;
        ~DIP3HeightGuard() { params.DIP0003Height = original_height; }
    } dip3_height_guard{mutable_consensus, old_dip3_height};

    {
        auto tx = m_node.evodb->BeginTransaction(EvoDbIdentity::SNAPSHOT);
        // Seed an incorrect full base list, not merely an incorrect comparison hash.
        m_node.evodb->Write(
            std::make_pair(std::string{"dmn_S3"}, base_index->GetBlockHash()),
            incorrect_base_list);
        m_node.evodb->WriteSnapshotBaseMNListHash(incorrect_hash);
        tx->Commit();
    }
    BOOST_REQUIRE(m_node.evodb->CommitRootTransaction(EvoDbIdentity::SNAPSHOT, /*sync=*/true));

    const auto result = WITH_LOCK(::cs_main,
        return chainman.MaybeValidateSnapshot(*validation_chainstate, *snapshot_chainstate, [](bilingual_str) {}));
    BOOST_CHECK_EQUAL(result, SnapshotCompletionResult::EVO_STATE_MISMATCH);

🤖 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/validation_chainstatemanager_tests.cpp` around lines 1085 - 1108,
Restore the process-wide Consensus::Params::DIP0003Height mutation with an RAII
guard immediately after saving the original value, so restoration occurs even
when BOOST_REQUIRE or MaybeValidateSnapshot exits the test early. Update the
setup around mutable_consensus and remove the later manual restore, preserving
the existing temporary value and validation behavior.

src/wallet/rpc/backup.cpp (1)

1955-1970: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Correct the descriptor rescan error wording.

Use “descriptor creation” instead of “key creation,” and fix “could potentially caused” to “could potentially be caused.”

Proposed fix

-                            "of key creation, and could contain transactions pertaining to the desc. As a "
-                            "result, transactions and coins using this desc may not appear in the wallet.",
+                            "of descriptor creation, and could contain transactions pertaining to the descriptor. As a "
+                            "result, transactions and coins using this descriptor may not appear in the wallet.",
...
-                        error_msg += strprintf(" This error could potentially caused by data corruption. If "
+                        error_msg += strprintf(" This error could potentially be caused by data corruption. If "

📝 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.

                    std::string error_msg{strprintf("Rescan failed for descriptor with timestamp %d. There "
                            "was an error reading a block from time %d, which is after or within %d seconds "
                            "of descriptor creation, and could contain transactions pertaining to the descriptor. As a "
                            "result, transactions and coins using this descriptor may not appear in the wallet.",
                            GetImportTimestamp(request, now), scanned_time - TIMESTAMP_WINDOW - 1, TIMESTAMP_WINDOW)};
                    if (pwallet->chain().havePruned()) {
                        error_msg += strprintf(" This error could be caused by pruning or data corruption "
                                "(see dashd log for details) and could be dealt with by downloading and "
                                "rescanning the relevant blocks (see -reindex option and rescanblockchain RPC).");
                    } else if (pwallet->chain().hasAssumedValidChain()) {
                        error_msg += strprintf(" This error is likely caused by an in-progress assumeutxo "
                                "background sync. Check logs or getchainstates RPC for assumeutxo background "
                                "sync progress and try again later.");
                    } else {
                        error_msg += strprintf(" This error could potentially be caused by data corruption. If "
                                "the issue persists you may want to reindex (see -reindex option).");

🤖 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/wallet/rpc/backup.cpp` around lines 1955 - 1970, Update the error message
constructed in the descriptor rescan failure path to say “descriptor creation”
instead of “key creation,” and correct the grammar to “could potentially be
caused.” Keep the surrounding error handling and message content unchanged.

test/functional/test_framework/messages.py (1)

119-136: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Rename ambiguous parameter l in ser_varint.

Both Ruff and Flake8 flag l as an ambiguous variable name (E741) at Lines 119 and 125, which will likely fail lint CI. The varint encode/decode logic itself is correct (verified against the referenced serialize_tests.cpp:varint_bitpatterns vectors).

🔧 Proposed fix

-def ser_varint(l):
+def ser_varint(n):
     r = b""
     while True:
-        r = bytes([(l & 0x7f) | (0x80 if len(r) > 0 else 0x00)]) + r
-        if l <= 0x7f:
+        r = bytes([(n & 0x7f) | (0x80 if len(r) > 0 else 0x00)]) + r
+        if n <= 0x7f:
             return r
-        l = (l >> 7) - 1
+        n = (n >> 7) - 1

📝 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.

def ser_varint(n):
    r = b""
    while True:
        r = bytes([(n & 0x7f) | (0x80 if len(r) > 0 else 0x00)]) + r
        if n <= 0x7f:
            return r
        n = (n >> 7) - 1


def deser_varint(f):
    n = 0
    while True:
        dat = f.read(1)[0]
        n = (n << 7) | (dat & 0x7f)
        if (dat & 0x80) > 0:
            n += 1
        else:
            return n

🧰 Tools

🪛 Flake8 (7.3.0)

[error] 119-119: ambiguous variable name 'l'

(E741)

---

[error] 125-125: ambiguous variable name 'l'

(E741)

🪛 Ruff (0.15.21)

[error] 119-119: Ambiguous variable name: l

(E741)

---

[error] 125-125: Ambiguous variable name: l

(E741)

🤖 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 `@test/functional/test_framework/messages.py` around lines 119 - 136, Rename
the ambiguous parameter l in ser_varint to a descriptive name such as value, and
update all references to it within that function while preserving the existing
varint encoding logic.

Source: Linters/SAST tools