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 snapshot-aware chainstate handling with separate persistence for normal and snapshot data.
  • Masternode DKG participation and quorum signing are automatically paused until snapshot validation completes.
  • Added quorumParticipation to masternode status responses.
  • Added clearer snapshot and unavailable-block error reporting.

• Bug Fixes

  • Improved quorum, special transaction, deterministic masternode, and credit-pool processing across chainstates.
  • Prevented invalid or oversized signature-share messages from being processed.
  • Corrected coin statistics reporting by replacing the obsolete bip30 category with genesis_block.

• Tests

  • Expanded coverage for snapshot recovery, database consistency, quorum behavior, serialization, and network message limits.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR introduces identity-aware EvoDB transactions for normal and snapshot chainstates, threads explicit chain context through quorum and special-transaction processing, blocks DKG/signing during unvalidated snapshots, adds bounded signature decoding, improves block-data error handling, and expands unit and integration coverage.

Changes

Dual-chainstate validation and snapshot support

Layer / File(s)	Summary
Shared contracts and validation interfaces src/evo/..., src/llmq/..., src/validation.h, src/Makefile.test.include	Public APIs now expose chain or identity context, signal maps use ordered semantics, derived serialization is canonical, and the new EvoDB tests are built.
Identity-scoped EvoDB and chainstate persistence src/evo/evodb.*, src/validation.*, src/node/chainstate.cpp	Transactions, derived writes, best-block markers, commits, snapshot activation, and notifications are scoped to normal or snapshot chainstates.
Chain-aware quorum and special transactions src/llmq/blockprocessor.*, src/llmq/quorumsman.*, src/evo/specialtxman.*, src/evo/assetlocktx.*, src/evo/mnhftx.*	Quorum and special-transaction paths receive explicit chain context and use checked derived persistence.
Snapshot gating and bounded network handling src/active/*, src/llmq/signing_shares.*, src/llmq/snapshot.cpp, src/llmq/net_signing.*, src/net_processing.cpp, src/rpc/*	DKG and signing are blocked during unvalidated snapshots, unavailable block data is handled without peer penalties, and signature-share decoding is size-bounded.
Validation and transaction test coverage src/test/*, test/functional/feature_coinstatsindex.py	Tests cover EvoDB overlays, snapshot markers, chain-aware commitments, callback suppression, canonical serialization, offline signing, and bounded signature-share payloads.

Conflict comment state parsing

Layer / File(s)	Summary
Comment state rendering and parsing .github/workflows/handle_potential_conflicts.py, .github/workflows/test_handle_potential_conflicts.py	State extraction anchors on the appropriate marker, rendered state is appended after advisory text, and legacy marker ordering is tested.

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

Sequence Diagram(s)

sequenceDiagram
  participant Chainstate
  participant EvoDB
  participant QuorumBlockProcessor
  participant QuorumManager
  participant SigSharesManager
  Chainstate->>EvoDB: BeginTransaction(EvoDbIdentity)
  Chainstate->>QuorumBlockProcessor: ProcessBlock(chainstate)
  QuorumBlockProcessor->>QuorumManager: GetQuorum(..., chain)
  QuorumManager-->>QuorumBlockProcessor: chain-aware quorum
  Chainstate->>SigSharesManager: CreateSigShare()
  SigSharesManager-->>Chainstate: blocked or produced signature share
  Chainstate->>EvoDB: CommitRootTransaction(EvoDbIdentity)

Loading

Suggested reviewers: knst, udjinm6

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 13.77% 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 main change: backporting AssumeUTXO M2 with 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

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

📒 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 6a803ac 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]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
ast-grep outline src/active/masternode.cpp --items all --match 'UpdatedBlockTip' 2>/dev/null || \
fd -i masternode src/active --type f

Repository: PastaPastaPasta/dash

Length of output: 320

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== src/active/masternode.cpp outline =="
ast-grep outline src/active/masternode.cpp --view expanded || true

echo
echo "== UpdatedBlockTip implementation =="
sed -n '150,240p' src/active/masternode.cpp | cat -n

echo
echo "== UpdatedBlockTip call sites =="
rg -n "UpdatedBlockTip\(" src | cat -n

Repository: PastaPastaPasta/dash

Length of output: 18878

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

sed -n '175,260p' src/active/masternode.cpp | cat -n

Repository: PastaPastaPasta/dash

Length of output: 4476

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

grep -n "void CActiveMasternodeManager::UpdatedBlockTip" -A80 -B20 src/active/masternode.cpp

Repository: PastaPastaPasta/dash

Length of output: 4919

---

🌐 Web query:

"src/active/masternode.cpp CActiveMasternodeManager::UpdatedBlockTip Dash"

💡 Result:

In the Dash Core codebase, the function CActiveMasternodeManager::UpdatedBlockTip is a method responsible for handling notifications when the blockchain tip is updated [1]. It is part of the CActiveMasternodeManager class, which manages the operational state of a node acting as a masternode [1]. This method is inherited from CValidationInterface and serves as a listener that reacts to changes in the blockchain chainstate [1]. When a new block tip is processed, UpdatedBlockTip is triggered to verify and maintain the masternode's status, ensuring that the local node's information remains synchronized with the deterministic masternode list [2]. Key responsibilities of this method include: 1. Verification of Masternode Status: It checks if the masternode is still valid according to the deterministic masternode (DIP3) rules [2]. 2. State Transition Management: If the masternode information (such as the ProTx hash, operator key, or IP address) changes or if the masternode is removed from the list, the method updates the local active masternode state accordingly [2]. 3. Synchronization: It helps the local node react to events like operator key changes or network address updates, triggering a reset or update of the local masternode configuration when necessary [2]. While historically located in src/masternode/activemasternode.cpp, the CActiveMasternodeManager and related logic have been subject to architectural refactors, such as moving components into the src/active/ directory to support improved context management within the Dash Core node [3][4].

Citations:

• 1: https://dash-docs.github.io/en/doxygen/html/class_c_active_masternode_manager.html
• 2: https://dash-docs.github.io/en/doxygen/html/activemasternode_8cpp_source.html
• 3: refactor: extract CActiveMasternodeManager from LLMQContext (3/n, DKG session isolation, ActiveContext consolidation) dashpay/dash#7065
• 4: e9f8294

---

🌐 Web query:

"\"CActiveMasternodeManager::UpdatedBlockTip\" dash github"

💡 Result:

CActiveMasternodeManager::UpdatedBlockTip is a virtual method within the Dash Core codebase, defined in the CActiveMasternodeManager class [1][2]. It is a component of the node's validation interface, specifically implemented to handle notifications when the block chain tip is updated [1][2]. Key characteristics and functionality include: Implementation and Role: The method is an implementation of the CValidationInterface::UpdatedBlockTip interface [1]. It is called on a background thread whenever a new block tip is processed, allowing the active masternode manager to respond to changes in the blockchain [1]. Core Logic: When triggered, the method performs several checks to manage the state of the active masternode [3]: - It verifies if DIP3 (Deterministic Masternode List) enforcement is active for the new block height [3]. - It compares the masternode list before and after the block update to detect changes such as the masternode being removed from the list, changes to the operator key, or changes to the masternode's IP address [3]. - Depending on these checks, it updates the internal state of the active masternode (e.g., setting the state to MASTERNODE_REMOVED, MASTERNODE_OPERATOR_KEY_CHANGED, or MASTERNODE_PROTX_IP_CHANGED) and resets relevant identity information if necessary [3]. Technical Context: The method is defined in src/masternode/activemasternode.cpp and declared in src/masternode/activemasternode.h [3][2]. It is critical for ensuring that an active masternode remains synchronized and compliant with the network's deterministic masternode list as the chain progresses [3]. It is often part of broader architectural components, such as those involved in Distributed Key Generation (DKG) session management and masternode synchronization [4][5].

Citations:

• 1: https://dash-docs.github.io/en/doxygen/html/class_c_active_masternode_manager.html
• 2: https://dash-docs.github.io/en/doxygen/html/activemasternode_8h_source.html
• 3: https://dash-docs.github.io/en/doxygen/html/activemasternode_8cpp_source.html
• 4: refactor: extract CActiveMasternodeManager from LLMQContext (3/n, DKG session isolation, ActiveContext consolidation) dashpay/dash#7065
• 5: https://dash-docs.github.io/en/doxygen/html/masternode-sync_8cpp_source.html

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "== m_state and GetStateString uses =="
rg -n "\bm_state\b|GetStateString\(" src/active src/evo src/llmq src/governance src/masternode src/test | cat -n

echo
echo "== InitInternal / Init in src/active/masternode.cpp =="
sed -n '112,175p' src/active/masternode.cpp | cat -n

Repository: PastaPastaPasta/dash

Length of output: 11450

---

Gate nodeman->UpdatedBlockTip under the snapshot check
CActiveMasternodeManager::UpdatedBlockTip reads the DML for pindexNew/pindexNew->pprev and mutates m_state/m_protx_hash, so it depends on chain data from the current tip. Skip it until snapshot background validation completes, just like the other masternode duties.

🤖 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/active/context.cpp` around lines 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.

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Inspect the declared type of updatedMNs and its Serialize/Unserialize logic.
rg -n 'updatedMNs' src/evo/deterministicmns.h

Repository: PastaPastaPasta/dash

Length of output: 1008

---

Use a test that actually depends on canonicalization
updatedMNs is an std::unordered_map, and serialization already sorts the keys before writing them, so forward vs. reverse insertion produces identical bytes even without this change. Add a case that would fail if that ordering were removed.

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