backport: assumeutxo M2 — EvoDB multi-chainstate isolation and subsystem gating

[PastaPastaPasta]

Issue being fixed or feature implemented

M1 (#50) landed snapshot persistence but kept Dash's single-EvoDB
assumption. To support two live chainstates (background/normal +
snapshot) each with independent masternode/quorum/credit-pool state, EvoDB
needs stable per-chainstate identities and transaction overlays, and every
side-effect subsystem (MN list notifications, LLMQ signing, RPC, UI) needs
to bind validation to the calling chainstate. This is milestone 2/7 (design
docs D2/D3). The execution
plan and per-unit ledger (M2 section) are tracked outside the repo in a
private gist, not checked in: https://gist.github.com/PastaPastaPasta/aa52b1f89fb74a0566ba3b5e15afab6a

PR 2/7 in the stacked series — base is develop after M1 merged.

What was done?

• B2 — EvoDB multi-chainstate: shared single CEvoDB with per-chainstate
  best-block markers and flush transactions (Option A from the design doc:
  union converges, no merge needed at completion). First implementation was
  rejected on round 1 adversarial review (cross-context pending-read
  visibility broke crash atomicity, unstable role-keyed markers, undo-erase
  cross-chainstate deletion, tombstones defeated by fallback reads); reworked
  to own-overlay+disk-only reads, storage-identity-keyed markers/contexts, an
  undo-erase guard, and a downgrade flag. Two items were explicitly deferred
  to M3: marker promotion to the legacy b_b4 key at completion, and holistic
  base-state comparison (both left as TODOs in evodb.h).
• B3 — subsystem gating: masternode/quorum/credit-pool side effects gated
  by the calling chainstate and snapshot-validation state.
  Two review rounds found and fixed a critical seeded-commitment accounting
  bug and, more seriously, that MNHF/asset-unlock quorum resolution used
  active-chain semantics during background validation — fixed by threading
  the validating chainstate through CQuorumManager::HasQuorum/GetQuorum/ScanQuorums.
  Masternodes refuse DKG/signing on an unvalidated snapshot chainstate.
• Orchestrator-found fix: CEvoDB::WriteDerived byte-compares
  re-serialized values, so every block-derived payload type must serialize
  canonically. Two latent violations (CDeterministicMNListDiff::updatedMNs
  as an unordered_map, MNHF Signals as an unordered_map) caused
  BLOCK_CONSENSUS failures on reconsiderblock after restart — fixed by
  sorting on serialize / switching to std::map. As a result,
  feature_mnehf.py and feature_asset_locks.py are now permanent members
  of the milestone functional gate from M2 onward.

How Has This Been Tested?

• make check: exit 0, 0 failures.
• Functional subset on the final tree: feature_mnehf, feature_asset_locks,
  feature_dip3_deterministicmns ×2, feature_llmq_signing ×2,
  feature_llmq_rotation, rpc_quorum — all passed.
• Lint: circular-dependencies and whitespace PASS; python lint deferred to CI.
• 3 review rounds each for B2 and B3 (adversarial rework → crash-matrix
  verification → final adjudication); milestone gate PASS on 2026-07-11.

Breaking Changes

None externally. Internally, ActivateExistingSnapshot becomes fallible
(returns/propagates failure instead of the upstream reference-return shape) —
noted for future backports that touch this path.

Checklist:

• I have performed a self-review of my own code
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation (docs rewrite lands in M7/B8)
• I have assigned this pull request to a milestone

Summary by CodeRabbit

• New Features

  • Added independent management of normal and snapshot chain data.
  • Added status reporting for masternode and quorum participation during snapshot validation.
  • Improved chainlock cache retention and cleanup.

• Bug Fixes

  • Prevented quorum and masternode operations before snapshot validation completes.
  • Improved handling of unavailable block data and persistence inconsistencies.

• Tests

  • Expanded coverage for snapshots, database overlays, quorum processing, serialization, and cache pruning.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Changes

Core validation and chainstate changes

Layer / File(s)	Summary
Identity-scoped EvoDB and chainstate persistence src/evo/evodb.*, src/validation.*, src/node/chainstate.cpp, src/dbwrapper.h	EvoDB transactions, derived writes, best-block markers, snapshot activation, and chainstate notifications now use chainstate identities.
Chain-aware quorum and special-transaction processing src/llmq/*, src/evo/assetlocktx.*, src/evo/mnhftx.*, src/evo/specialtxman.*, src/evo/creditpool.cpp, src/evo/deterministicmns.cpp	Quorum lookup, commitment handling, special-transaction validation, and EvoDB persistence now receive explicit chain context and check write results.
Snapshot validation gating and unavailable block data src/active/*, src/llmq/signing_shares.*, src/llmq/snapshot.cpp, src/evo/smldiff.*, src/net_processing.cpp, src/rpc/masternode.cpp, src/rpc/quorums.cpp	Unvalidated snapshots block DKG and signing. Snapshot builders report unavailable block data. Network handlers avoid peer penalties for that error.
Cache thresholds and deterministic serialization src/chainlock/*, src/limitedmap.h, src/versionbits.h, src/evo/chainhelper.*, src/evo/deterministicmns.h, src/node/miner.cpp, src/rpc/blockchain.cpp	Caches now separate retained and prune-after sizes. Signal and masternode-list serialization use ordered data.
Validation, persistence, and cache tests src/test/*, src/Makefile.test.include	Tests cover EvoDB overlays, snapshot markers, chain-aware commitments, callback suppression, canonical serialization, cache pruning, and on-disk test databases.

Pull request description validation

Layer / File(s)	Summary
Description mention checker .github/workflows/check_pr_description_mentions.py, .github/workflows/semantic-pull-request.yml	CI checks pull request descriptions for username mentions while allowing email addresses.

Estimated code review effort: 5 (Critical) | ~120 minutes

Suggested reviewers: knst, udjinm6

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 14.44% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the PR's main changes: AssumeUTXO M2 backport, EvoDB multi-chainstate isolation, and subsystem gating.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch assumeutxo/m2-evodb-roles

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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/test/validation_chainstatemanager_tests.cpp (1)

463-500: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Reopen EvoDB to simulate an actual node restart.

SimulateNodeRestart replaces only the chainstate manager; the same CEvoDB object and its transaction contexts survive. Consequently, the persistence test at Lines 767-793 can pass using retained state rather than data reopened from disk. Close and recreate m_node.evodb with .wipe=false after tearing down its consumers.

🤖 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 463 - 500,
Update SimulateNodeRestart to close and recreate m_node.evodb with wipe=false
after tearing down all consumers that depend on it and before constructing the
replacement ChainstateManager. Ensure the recreated CEvoDB uses the existing
database contents while clearing the old transaction contexts, so persistence
tests exercise reopened disk state.

🤖 Prompt for all review comments with 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.

Inline comments:
In `@src/evo/smldiff.cpp`:
- Around line 185-189: Remove the BLOCK_HAVE_DATA requirement from the
baseBlockIndex ancestry/list-anchor validation in src/evo/smldiff.cpp (lines
185-189), while retaining data checks for the blockIndex that is actually read.
In src/llmq/snapshot.cpp, update lines 43, 89-90, and 123 so metadata-only
CBlockIndex and snapshot lookup paths do not require raw block data; keep such
checks only at actual disk-read sites.

In `@src/test/util/setup_common.cpp`:
- Line 247: Reset or otherwise destroy m_node.evodb in
BasicTestingSetup::~BasicTestingSetup before calling
fs::remove_all(m_path_root), ensuring the disk-backed EvoDB is closed while
preserving the existing in-memory behavior.

In `@src/test/validation_chainstatemanager_tests.cpp`:
- Line 818: Update the restart call in the validation-chainstate test to pass
flush_chainstates=false after erasing the snapshot marker, so the erased marker
remains absent when startup failure is checked. Ensure SimulateNodeRestart
supports this no-flush path with a real EvoDB reopen, while preserving the
existing restart behavior elsewhere.

In `@src/validation.cpp`:
- Around line 3564-3567: Extend the active-chainstate guards in
src/validation.cpp at lines 3564-3567 and 3669-3672 to also enclose the adjacent
uiInterface.NotifyBlockTip calls at lines 3570-3573 and 3674-3677. Keep both
SynchronousUpdatedBlockTip and UpdatedBlockTip notifications restricted to
ActiveChainstate, preventing all related UI tip notifications from background
chainstates.

In `@test/lint/lint-circular-dependencies.py`:
- Line 27: Remove the allowlisted cycle entry involving blockfilter and
interfaces/chain.h from the circular-dependency test. Refactor the dependency
path through validation, kernel/chain, and interfaces/chain.h so shared chain
metadata comes from a lower-level header or an interface rather than the
concrete interface-layer dependency, preserving the intended isolation boundary.

---

Outside diff comments:
In `@src/test/validation_chainstatemanager_tests.cpp`:
- Around line 463-500: Update SimulateNodeRestart to close and recreate
m_node.evodb with wipe=false after tearing down all consumers that depend on it
and before constructing the replacement ChainstateManager. Ensure the recreated
CEvoDB uses the existing database contents while clearing the old transaction
contexts, so persistence tests exercise reopened disk state.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

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

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9edf0b40-f213-4658-b878-17634348f43a

📥 Commits

Reviewing files that changed from the base of the PR and between f153015 and 6a803ac6acb2ac818b8d8cd8c6495b7f30b081db.

📒 Files selected for processing (51)

• src/Makefile.am
• src/Makefile.test.include
• src/active/context.cpp
• src/active/context.h
• src/active/dkgsessionhandler.cpp
• src/dbwrapper.h
• src/evo/assetlocktx.cpp
• src/evo/assetlocktx.h
• src/evo/chainhelper.cpp
• src/evo/chainhelper.h
• src/evo/creditpool.cpp
• src/evo/deterministicmns.cpp
• src/evo/deterministicmns.h
• src/evo/evodb.cpp
• src/evo/evodb.h
• src/evo/mnhftx.cpp
• src/evo/mnhftx.h
• src/evo/smldiff.cpp
• src/evo/smldiff.h
• src/evo/specialtxman.cpp
• src/evo/specialtxman.h
• src/interfaces/chain.h
• src/kernel/chain.cpp
• src/kernel/chain.h
• src/llmq/blockprocessor.cpp
• src/llmq/blockprocessor.h
• src/llmq/context.cpp
• src/llmq/quorumsman.cpp
• src/llmq/quorumsman.h
• src/llmq/signing_shares.cpp
• src/llmq/signing_shares.h
• src/llmq/snapshot.cpp
• src/net_processing.cpp
• src/node/chainstate.cpp
• src/node/interfaces.cpp
• src/node/miner.cpp
• src/rpc/blockchain.cpp
• src/rpc/masternode.cpp
• src/rpc/quorums.cpp
• src/test/evo_db_tests.cpp
• src/test/evo_deterministicmns_tests.cpp
• src/test/util/setup_common.cpp
• src/test/util/setup_common.h
• src/test/validation_chainstatemanager_tests.cpp
• src/validation.cpp
• src/validation.h
• src/versionbits.h
• src/wallet/test/fuzz/notifications.cpp
• src/wallet/wallet.cpp
• src/wallet/wallet.h
• test/lint/lint-circular-dependencies.py

[coderabbitai]

📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift

Break the new interface-layer cycle instead of allowlisting it.

The cycle routes implementation dependencies back through interfaces/chain.h, weakening the isolation boundary. Move the shared chain metadata contract to a lower-level header or replace the concrete dependency, then remove this exception.

As per coding guidelines, “Use interfaces in src/interfaces/ for codebase isolation and inter-process communication.” <coding_guidelines>

🤖 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/lint/lint-circular-dependencies.py` at line 27, Remove the allowlisted
cycle entry involving blockfilter and interfaces/chain.h from the
circular-dependency test. Refactor the dependency path through validation,
kernel/chain, and interfaces/chain.h so shared chain metadata comes from a
lower-level header or an interface rather than the concrete interface-layer
dependency, preserving the intended isolation boundary.

Source: Coding guidelines

[coderabbitai]

Actionable comments posted: 2

♻️ Duplicate comments (2)

src/test/validation_chainstatemanager_tests.cpp (1)

785-807: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Re-verify: does SimulateNodeRestart()'s default flush restore the marker this test just erased?

This test erases and fully commits the SNAPSHOT EvoDB marker (789-795), then calls this->SimulateNodeRestart(); with the default flush_chainstates=true, which flushes every chainstate — including the snapshot chainstate whose marker was just erased — before DetectSnapshotChainstate is checked. A sibling test (chainstatemanager_evodb_snapshot_only_flush_restart, 748-759) deliberately uses SimulateNodeRestart(/*flush_chainstates=*/false) to avoid exactly this kind of marker rewrite. This is the same scenario a past review flagged as defeating the missing-marker test via an unwanted flush; the resolution is marked "Addressed" but the visible code here still uses the default true, so it's worth confirming Chainstate::ForceFlushStateToDisk() doesn't unconditionally re-derive/re-write the EvoDB best-block marker (if it does, this test would pass for the wrong reason, or could become flaky).

#!/bin/bash
# Inspect ForceFlushStateToDisk to see whether it unconditionally writes the EvoDB best-block marker.
rg -n -B3 -A25 'void Chainstate::ForceFlushStateToDisk' src/validation.cpp
rg -n -B3 -A5 'WriteBestBlock' src/validation.cpp

🤖 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 785 - 807, Use
SimulateNodeRestart(/*flush_chainstates=*/false) in
chainstatemanager_snapshot_init_missing_evodb_marker so the erased and committed
SNAPSHOT EvoDB marker remains absent when DetectSnapshotChainstate runs; keep
the rest of the test unchanged.

src/validation.cpp (1)

3564-3573: 🎯 Functional Correctness | 🟠 Major

UI tip notification still not gated to the active chainstate.

GetMainSignals().SynchronousUpdatedBlockTip/UpdatedBlockTip are now correctly restricted to the active chainstate (Lines 3564-3567), but the adjacent uiInterface.NotifyBlockTip(...) a few lines below (3571-3573) is gated only by pindex_was_in_chain, not by this == &m_chainman.ActiveChainstate(). The same gap exists in MarkConflictingBlock at Lines 3669-3677. Every other refactored call site in this file (ConnectBlock, DisconnectBlock, ActivateBestChain) gates the ValidationInterface signals and the UI notification together in a single active-chainstate check — these two sites are the outliers. A background/snapshot-validation chainstate invalidating/marking-conflicting a block on its own chain would still surface a UI tip update for a chain that isn't the active one.

This was flagged in a prior review round and marked addressed, but the current code still shows the unfixed pattern.

🐛 Suggested fix (apply analogously at both sites)

         InvalidChainFound(to_mark_failed);
         if (this == &m_chainman.ActiveChainstate()) {
             GetMainSignals().SynchronousUpdatedBlockTip(m_chain.Tip(), nullptr, IsInitialBlockDownload());
             GetMainSignals().UpdatedBlockTip(m_chain.Tip(), nullptr, IsInitialBlockDownload());
+
+            // Only notify about a new block tip if the active chain was modified.
+            if (pindex_was_in_chain) {
+                uiInterface.NotifyBlockTip(GetSynchronizationState(IsInitialBlockDownload()), to_mark_failed->pprev);
+            }
         }
     }
-
-    // Only notify about a new block tip if the active chain was modified.
-    if (pindex_was_in_chain) {
-        uiInterface.NotifyBlockTip(GetSynchronizationState(IsInitialBlockDownload()), to_mark_failed->pprev);
-    }
     return true;

🤖 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/validation.cpp` around lines 3564 - 3573, Gate the
uiInterface.NotifyBlockTip call in both the shown block-tip handling path and
MarkConflictingBlock with the same this == &m_chainman.ActiveChainstate()
condition used for the corresponding ValidationInterface signals, while
preserving the existing pindex_was_in_chain check.

🧹 Nitpick comments (6)

src/evo/evodb.cpp (1)

96-123: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Non-NORMAL best-block key hardcodes uint8_t{1} regardless of which identity it is.

ReadBestBlock/WriteBestBlock route every identity other than NORMAL to the same literal key (EVODB_BEST_BLOCK, uint8_t{1}). Since every identity's root_transaction is layered over the same underlying CDBWrapper (db), this only avoids collisions today because exactly one non-NORMAL identity (SNAPSHOT) exists. If a third identity is introduced (the PR notes marker work continues into M3), two non-NORMAL identities would silently overwrite each other's best-block marker on the shared disk namespace.

Deriving the suffix from the identity itself removes the landmine cheaply:

♻️ Suggested fix

-    return transaction.Read(std::make_pair(EVODB_BEST_BLOCK, uint8_t{1}), hash);
+    return transaction.Read(std::make_pair(EVODB_BEST_BLOCK, static_cast<uint8_t>(identity)), hash);

and symmetrically in WriteBestBlock.

🤖 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/evodb.cpp` around lines 96 - 123, Update ReadBestBlock and
WriteBestBlock so the non-NORMAL best-block key derives its suffix from the
provided EvoDbIdentity instead of hardcoding uint8_t{1}. Preserve the existing
EVODB_BEST_BLOCK key for NORMAL and ensure distinct non-NORMAL identities use
distinct keys in the shared database namespace.

src/validation.h (1)

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

Consider marking the method as const.

If EvoDbInconsistencyMessage does not modify the state of the object, it is best practice to mark it as const.

♻️ Proposed refactor

-    std::string EvoDbInconsistencyMessage();
+    std::string EvoDbInconsistencyMessage() const;

🤖 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/validation.h` at line 542, Update EvoDbInconsistencyMessage to be
const-qualified if its implementation does not modify object state, and ensure
the corresponding definition matches the declaration.

src/test/block_reward_reallocation_tests.cpp (2)

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

Fix the typo in the parameter name.

The parameter name is misspelled as utoxs. Consider renaming it to utxos (and updating its usages within the methods) for clarity and consistency.

• src/test/block_reward_reallocation_tests.cpp#L54-L54: rename utoxs to utxos.
• src/test/block_reward_reallocation_tests.cpp#L84-L84: rename utoxs to utxos.

🤖 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/block_reward_reallocation_tests.cpp` at line 54, Rename the
SelectUTXOs parameter from utoxs to utxos and update all references within the
function. Apply the same spelling correction at
src/test/block_reward_reallocation_tests.cpp lines 54-54 and 84-84.

---

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

Initialize the change variable.

Although SelectUTXOs writes a value to this reference internally, explicitly initializing the variable at declaration improves safety and readability.

♻️ Proposed refactor

-    CAmount change;
+    CAmount change = 0;

🤖 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/block_reward_reallocation_tests.cpp` at line 86, Initialize the
CAmount variable change at its declaration before passing it to SelectUTXOs,
while preserving the existing reference usage and value assignment flow.

src/llmq/net_signing.cpp (1)

67-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the repeated log/ban/rethrow pattern.

The try/catch block (log rejection, BanNode, throw;) is duplicated across QSIGSHARE, QSIGSESANN, QSIGSHARESINV/QGETSIGSHARES, and QBSIGSHARES. A small templated helper would remove the duplication and keep future limit/message-type changes in one place.

♻️ Suggested helper

template <typename T>
static bool TryUnserializeCapped(CDataStream& vRecv, std::vector<T>& msgs, size_t max_size,
                                  std::string_view msg_type, NodeId peer_id,
                                  PeerManager& peer_manager, std::function<void(NodeId)> ban_node)
{
    try {
        if (!UnserializeVectorWithMaxSize(vRecv, msgs, max_size)) {
            throw std::ios_base::failure(strprintf("%s vector size too large", msg_type));
        }
        return true;
    } catch (const std::ios_base::failure& e) {
        LogPrint(BCLog::LLMQ_SIGS, "NetSigning::%s -- rejected %s from peer=%d: %s\n",
                 __func__, msg_type, peer_id, e.what());
        ban_node(peer_id);
        throw;
    }
}

🤖 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/llmq/net_signing.cpp` around lines 67 - 130, Extract the duplicated
deserialization error handling from NetSigning’s message branches into a
reusable templated helper near the relevant implementation, covering capped
vector deserialization, rejection logging, peer banning, and exception
rethrowing. Update the QSIGSHARE, QSIGSESANN, QSIGSHARESINV/QGETSIGSHARES, and
QBSIGSHARES paths to use the helper while preserving each branch’s existing size
limit and deserialization routine.

src/test/evo_deterministicmns_tests.cpp (1)

149-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

CreateProRegTxExternalCollateral funds a redundant collateral-sized output.

Since the collateral is supplied externally via collateralOutpoint, this helper doesn't need to fund another dmn_types::Regular.collat_amount output — the other update-tx helpers in this file fund with a nominal 1 * COIN for exactly this reason. Creating a second collateral-value output here is at best confusing and at worst a latent trap for any future caller that runs GetCollateralOutpoint on this function's output expecting it to identify a real collateral.

♻️ Suggested funding-amount fix

-    const auto spent = FundTransaction(chainman, tx, utxos, scriptPayout, dmn_types::Regular.collat_amount);
+    const auto spent = FundTransaction(chainman, tx, utxos, scriptPayout, 1 * 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/test/evo_deterministicmns_tests.cpp` around lines 149 - 173, Update
CreateProRegTxExternalCollateral so FundTransaction uses the nominal 1 * COIN
amount instead of dmn_types::Regular.collat_amount, while preserving the
externally supplied collateralOutpoint as the actual collateral reference.

🤖 Prompt for all review comments with 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.

Inline comments:
In `@src/active/context.cpp`:
- Around line 98-109: Move the nodeman->UpdatedBlockTip call in
CActiveMasternodeManager::UpdatedBlockTip below the
IsSnapshotActiveAndUnvalidated guard, so it is skipped while snapshot validation
is active. Preserve the existing m_snapshot_duty_blocked logging and state
transitions, and invoke UpdatedBlockTip only after validation has completed.

In `@src/test/evo_deterministicmns_tests.cpp`:
- Around line 1454-1471: Update
deterministic_mn_list_diff_serialization_is_canonical to test a serialization
behavior that specifically requires canonical key ordering, rather than only
comparing different insertion orders of unordered_map entries. Add a scenario
whose serialized output differs when key sorting is removed, and assert the
canonical result against an explicitly constructed expected ordering or byte
sequence.

---

Duplicate comments:
In `@src/test/validation_chainstatemanager_tests.cpp`:
- Around line 785-807: Use SimulateNodeRestart(/*flush_chainstates=*/false) in
chainstatemanager_snapshot_init_missing_evodb_marker so the erased and committed
SNAPSHOT EvoDB marker remains absent when DetectSnapshotChainstate runs; keep
the rest of the test unchanged.

In `@src/validation.cpp`:
- Around line 3564-3573: Gate the uiInterface.NotifyBlockTip call in both the
shown block-tip handling path and MarkConflictingBlock with the same this ==
&m_chainman.ActiveChainstate() condition used for the corresponding
ValidationInterface signals, while preserving the existing pindex_was_in_chain
check.

---

Nitpick comments:
In `@src/evo/evodb.cpp`:
- Around line 96-123: Update ReadBestBlock and WriteBestBlock so the non-NORMAL
best-block key derives its suffix from the provided EvoDbIdentity instead of
hardcoding uint8_t{1}. Preserve the existing EVODB_BEST_BLOCK key for NORMAL and
ensure distinct non-NORMAL identities use distinct keys in the shared database
namespace.

In `@src/llmq/net_signing.cpp`:
- Around line 67-130: Extract the duplicated deserialization error handling from
NetSigning’s message branches into a reusable templated helper near the relevant
implementation, covering capped vector deserialization, rejection logging, peer
banning, and exception rethrowing. Update the QSIGSHARE, QSIGSESANN,
QSIGSHARESINV/QGETSIGSHARES, and QBSIGSHARES paths to use the helper while
preserving each branch’s existing size limit and deserialization routine.

In `@src/test/block_reward_reallocation_tests.cpp`:
- Line 54: Rename the SelectUTXOs parameter from utoxs to utxos and update all
references within the function. Apply the same spelling correction at
src/test/block_reward_reallocation_tests.cpp lines 54-54 and 84-84.
- Line 86: Initialize the CAmount variable change at its declaration before
passing it to SelectUTXOs, while preserving the existing reference usage and
value assignment flow.

In `@src/test/evo_deterministicmns_tests.cpp`:
- Around line 149-173: Update CreateProRegTxExternalCollateral so
FundTransaction uses the nominal 1 * COIN amount instead of
dmn_types::Regular.collat_amount, while preserving the externally supplied
collateralOutpoint as the actual collateral reference.

In `@src/validation.h`:
- Line 542: Update EvoDbInconsistencyMessage to be const-qualified if its
implementation does not modify object state, and ensure the corresponding
definition matches the declaration.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

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

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 286d0ead-f9b0-42f1-8d60-585ac42c9b99

📥 Commits

Reviewing files that changed from the base of the PR and between 6a803ac6acb2ac818b8d8cd8c6495b7f30b081db and 1a95697.

📒 Files selected for processing (52)

• .github/workflows/handle_potential_conflicts.py
• .github/workflows/test_handle_potential_conflicts.py
• src/Makefile.test.include
• src/active/context.cpp
• src/active/context.h
• src/active/dkgsessionhandler.cpp
• src/dbwrapper.h
• src/evo/assetlocktx.cpp
• src/evo/assetlocktx.h
• src/evo/chainhelper.cpp
• src/evo/chainhelper.h
• src/evo/creditpool.cpp
• src/evo/deterministicmns.cpp
• src/evo/deterministicmns.h
• src/evo/evodb.cpp
• src/evo/evodb.h
• src/evo/mnhftx.cpp
• src/evo/mnhftx.h
• src/evo/smldiff.cpp
• src/evo/smldiff.h
• src/evo/specialtxman.cpp
• src/evo/specialtxman.h
• src/index/coinstatsindex.cpp
• src/index/coinstatsindex.h
• src/llmq/blockprocessor.cpp
• src/llmq/blockprocessor.h
• src/llmq/context.cpp
• src/llmq/net_signing.cpp
• src/llmq/net_signing.h
• src/llmq/quorumsman.cpp
• src/llmq/quorumsman.h
• src/llmq/signing_shares.cpp
• src/llmq/signing_shares.h
• src/llmq/snapshot.cpp
• src/net_processing.cpp
• src/node/chainstate.cpp
• src/node/miner.cpp
• src/rpc/blockchain.cpp
• src/rpc/masternode.cpp
• src/rpc/quorums.cpp
• src/test/block_reward_reallocation_tests.cpp
• src/test/evo_db_tests.cpp
• src/test/evo_deterministicmns_tests.cpp
• src/test/governance_inv_tests.cpp
• src/test/llmq_utils_tests.cpp
• src/test/util/setup_common.cpp
• src/test/util/setup_common.h
• src/test/validation_chainstatemanager_tests.cpp
• src/validation.cpp
• src/validation.h
• src/versionbits.h
• test/functional/feature_coinstatsindex.py

💤 Files with no reviewable changes (2)

• src/index/coinstatsindex.cpp
• test/functional/feature_coinstatsindex.py

🚧 Files skipped from review as they are similar to previous changes (27)

• src/llmq/context.cpp
• src/evo/chainhelper.cpp
• src/evo/deterministicmns.h
• src/evo/assetlocktx.h
• src/versionbits.h
• src/node/miner.cpp
• src/evo/creditpool.cpp
• src/rpc/quorums.cpp
• src/active/dkgsessionhandler.cpp
• src/node/chainstate.cpp
• src/rpc/masternode.cpp
• src/active/context.h
• src/evo/smldiff.cpp
• src/evo/mnhftx.h
• src/evo/smldiff.h
• src/llmq/signing_shares.cpp
• src/net_processing.cpp
• src/evo/specialtxman.h
• src/evo/assetlocktx.cpp
• src/llmq/quorumsman.h
• src/evo/mnhftx.cpp
• src/Makefile.test.include
• src/test/util/setup_common.h
• src/llmq/quorumsman.cpp
• src/llmq/blockprocessor.h
• src/evo/specialtxman.cpp
• src/evo/evodb.h

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with 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.

Inline comments:
In `@src/evo/evodb.h`:
- Around line 190-197: Update GetMemoryUsage() to acquire LOCK(cs) before
iterating transaction_contexts, and annotate transaction_contexts with
GUARDED_BY(cs) so accesses are statically protected. Preserve the existing
memory-usage calculation.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

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

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4ba18c9b-3d24-48ca-bcd5-424590fe282c

📥 Commits

Reviewing files that changed from the base of the PR and between 1a95697 and a3c4476.

📒 Files selected for processing (54)

• .github/workflows/build-depends.yml
• .github/workflows/build-src.yml
• .github/workflows/build.yml
• src/Makefile.test.include
• src/active/context.cpp
• src/active/context.h
• src/active/dkgsessionhandler.cpp
• src/coinjoin/coinjoin.cpp
• src/coinjoin/coinjoin.h
• src/coinjoin/server.cpp
• src/coinjoin/server.h
• src/dbwrapper.h
• src/evo/assetlocktx.cpp
• src/evo/assetlocktx.h
• src/evo/chainhelper.cpp
• src/evo/chainhelper.h
• src/evo/creditpool.cpp
• src/evo/deterministicmns.cpp
• src/evo/deterministicmns.h
• src/evo/evodb.cpp
• src/evo/evodb.h
• src/evo/mnhftx.cpp
• src/evo/mnhftx.h
• src/evo/smldiff.cpp
• src/evo/smldiff.h
• src/evo/specialtxman.cpp
• src/evo/specialtxman.h
• src/governance/governance.cpp
• src/governance/governance.h
• src/llmq/blockprocessor.cpp
• src/llmq/blockprocessor.h
• src/llmq/context.cpp
• src/llmq/quorumsman.cpp
• src/llmq/quorumsman.h
• src/llmq/signing_shares.cpp
• src/llmq/signing_shares.h
• src/llmq/snapshot.cpp
• src/net_processing.cpp
• src/node/chainstate.cpp
• src/node/miner.cpp
• src/rpc/blockchain.cpp
• src/rpc/masternode.cpp
• src/rpc/quorums.cpp
• src/test/bls_tests.cpp
• src/test/coinjoin_inouts_tests.cpp
• src/test/evo_db_tests.cpp
• src/test/evo_deterministicmns_tests.cpp
• src/test/governance_inv_tests.cpp
• src/test/util/setup_common.cpp
• src/test/util/setup_common.h
• src/test/validation_chainstatemanager_tests.cpp
• src/validation.cpp
• src/validation.h
• src/versionbits.h

💤 Files with no reviewable changes (2)

• src/governance/governance.h
• src/test/bls_tests.cpp

🚧 Files skipped from review as they are similar to previous changes (31)

• src/evo/smldiff.h
• src/versionbits.h
• src/evo/assetlocktx.h
• src/llmq/context.cpp
• src/dbwrapper.h
• src/rpc/masternode.cpp
• src/Makefile.test.include
• src/evo/specialtxman.h
• src/evo/mnhftx.h
• src/evo/chainhelper.cpp
• src/evo/creditpool.cpp
• src/evo/smldiff.cpp
• src/net_processing.cpp
• src/llmq/signing_shares.h
• src/evo/deterministicmns.cpp
• src/node/miner.cpp
• src/evo/chainhelper.h
• src/active/context.cpp
• src/validation.h
• src/test/util/setup_common.h
• src/llmq/blockprocessor.h
• src/evo/assetlocktx.cpp
• src/node/chainstate.cpp
• src/evo/specialtxman.cpp
• src/llmq/signing_shares.cpp
• src/llmq/quorumsman.h
• src/rpc/blockchain.cpp
• src/llmq/blockprocessor.cpp
• src/llmq/quorumsman.cpp
• src/validation.cpp
• src/test/evo_deterministicmns_tests.cpp