feat: assumeutxo M2 — EvoDB multi-chainstate isolation and subsystem gating - #7456
Conversation
|
See https://gist.github.com/PastaPastaPasta/c5108775f7b126f88856cde20d7a8556 for ai generated commit by commit explanation. |
06b5c6f to
1a95697
Compare
|
This pull request has conflicts, please rebase. |
1a95697 to
a3c4476
Compare
a3c4476 to
26c8718
Compare
|
This pull request has conflicts, please rebase. |
26c8718 to
b6edcf8
Compare
11a31da to
5f3a876
Compare
|
that's not my final review feedback, I am not determined yet how exactly improve or change this PR or keep it as it is
There are several components that are logically part of chainstate, but initialized separately and behave differently.
@PastaPastaPasta what is your thoughts on And one more things: you have no plans to make evodb a part of assume-utxo, right? =================
-evodb = std::make_unique<CEvoDB>(db_params);
+m_evodb{std::make_unique<CEvoDB>(db_params, snapshot_base ? "evodb" + std::string{node::SNAPSHOT_CHAINSTATE_SUFFIX} : "evodb")}, |
|
This pull request has conflicts, please rebase. |
5f3a876 to
d47d9b8
Compare
|
This pull request has conflicts, please rebase. |
d47d9b8 to
7fabb1d
Compare
7fabb1d to
bdd5626
Compare
|
@knst Fable's raw response: This is based on the context of the full WIP assumeutxo implementation. I think it answers your questions well. |
|
🔍 Review in progress — actively reviewing now (commit 066fca8) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bdd5626c20
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Pass the validating chainstate through special transaction and quorum commitment processing instead of borrowing the active chainstate. Interpret mined-commitment records and quorum resolution relative to the caller's chain. The cached values remain reusable, but chain membership is reevaluated across reorgs and chainstates while public non-validation callers retain active-chain semantics. This prevents snapshot-seeded records from suppressing commitments or satisfying MNHF and asset-unlock quorum lookups during background validation. Add dual-chainstate coverage for a commitment seeded at a block not yet contained by the background chain, including HasQuorum and GetQuorum cache-order checks.
Emit block, tip, deterministic masternode-list, UI, and flush notifications only for the active chainstate. In particular, suppressing background ChainStateFlushed prevents a background locator from regressing wallet best-block state. Keep BlockChecked ungated because its subscribers are mining/block-submit and peer validation/relay accounting; it does not reach CMNAuth. Document all 21 B3 call-site dispositions and extend the dual-chainstate test with validation-interface and UI counters.
Check local block-data availability before building masternode-list diffs and quorum rotation info. Treat failures caused by pruning or an unvalidated snapshot base like pruned getdata: log and silently drop the plausible request without increasing the peer's misbehavior score. Malformed and implausible requests retain the pre-existing penalties.
Disable DKG participation and quorum signing until snapshot background validation completes. Enforce the refusal at CreateSigShare, the actual share-production boundary, so direct RPC, async, and queued signing paths cannot bypass it. The quorum sign RPC now returns a clear JSON-RPC error for both submit modes, and masternode status exposes the disabled participation state. Add unit coverage for the shared production-gate predicate across snapshot activation.
A WriteDerived failure means independently derived block data disagrees with the copy already recorded in EvoDB. That is local state corruption (or a cross-chainstate divergence bug), never evidence about the block being processed. Previously the mismatch surfaced as BLOCK_CONSENSUS: the block was persistently marked BLOCK_FAILED_VALID (surviving restart and forking the node off the network) and the relaying peer was handed a 100-point misbehavior score via BlockChecked, which background validation also triggers. Instead, follow the existing EvoDbInconsistencyMessage convention: request node shutdown via AbortNode and fail validation with M_ERROR, which neither marks the block invalid nor punishes peers. The credit-pool and MNHF sites abort at the throw site because miner and RPC callers never pass through a validation-state catch; a typed EvoDbInconsistencyError lets the four block-path catch blocks that would otherwise swallow it into BLOCK_CONSENSUS reclassify it as M_ERROR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ethrow CMNHFManager::GetForBlock re-throws internal ProcessBlock failures as a plain runtime_error, which would let a downstream generic catch misreport an EvoDB mismatch (M_ERROR) as a consensus failure. Re-throw typed when the validation state carries M_ERROR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reads and writes outside a BeginTransaction scope previously always bound to the NORMAL identity. While a snapshot chainstate is active, transaction-less consumers (RPC, mempool, miner, P2P serving) could not see snapshot-chain records still pending in the SNAPSHOT root overlay, and transaction-less writes dirtied the wrong overlay. CEvoDB now tracks a default identity which snapshot activation (ActivateSnapshot, ActivateExistingSnapshot) sets to SNAPSHOT and ResetChainstates resets; snapshot completion must reset it to NORMAL when marker promotion lands. Also make the GetListForBlockInternal fallback loud: a missing list diff for a DIP3-active block is pending-elsewhere or corrupt data, never the pre-DIP3 genesis of the masternode list, so throw instead of silently caching an empty list and clobbering m_initial_snapshot_index. The thrown message carries the IsBlockDataUnavailableError sentinel and BuildSimplifiedMNListDiff converts it into a serve failure, so peers requesting such history are not penalized. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
GetListForBlock can now throw when list data is unavailable (missing diff for a DIP3-active block). CActiveMasternodeManager::UpdatedBlockTip and GovernanceSigner's trigger creation run on the scheduler thread, where an uncaught exception terminates the node; catch it there and skip the update instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The scheduler-thread guards added for snapshot nodes caught every std::exception, but the only recoverable condition is a masternode list diff that is not on this node yet. CDeterministicMNList::ApplyDiff raises a plain std::runtime_error for missing removals or updates, duplicate masternodes, and duplicate unique properties -- local EvoDB corruption, which was then logged as benign unavailable history while the active masternode stayed READY. Give the unavailable-history condition its own BlockDataUnavailableError type and catch only that. Everything else propagates exactly as it did before these guards existed. BuildSimplifiedMNListDiff is narrowed the same way, so local corruption no longer turns into a peer-facing error string.
bls::bls_legacy_scheme is process-wide, but the correct value is a property of the block being processed. ConnectBlock only saved the current value and committed a successful transition; it never established the scheme for the chainstate it was validating. ProcessSpecialTxsInBlock switches legacy->basic when crossing V19 forward and never the other way, so with an active post-V19 snapshot the background chainstate validated pre-V19 blocks under the basic scheme, decoding historical BLS-serialized quorum and special-transaction data with the wrong one. A background disconnect across V19 could conversely commit legacy and leave active-chain consumers there. Enter ConnectBlock under the scheme the block's parent left behind, and commit it out of ConnectTip/DisconnectTip only from the active chainstate.
GetCurrentIdentity() resolved to the process-wide active_transaction, so while the background chainstate held a NORMAL transaction open every concurrent transaction-less read was redirected to NORMAL instead of the active snapshot's default identity. CQuorumManager::GetQuorum releases cs_main before BuildQuorumFromCommitment calls GetMinedCommitment: if background validation started in between, snapshot-only records still pending in the SNAPSHOT overlay dropped out of that lookup and the active node intermittently reported a missing quorum. An open transaction is the identity of one validation execution context, not of the process, so resolve it only for the thread that began it.
…away
Two ways a persisted snapshot chainstate could outlive itself:
PopulateAndValidateSnapshot commits the SNAPSHOT best-block and dual-chainstate markers as its last step. If activation was then abandoned -- WriteSnapshotBaseBlockhash on an unwritable datadir, say -- only the coins directory was removed. The node went back to a single chainstate carrying a permanent dual-chainstate marker, which turns supported legacy missing-diff bootstrapping into unavailable history, and a stale SNAPSHOT marker that a future snapshot directory could satisfy ActivateExistingSnapshot with. Erase both when activation is rolled back.
LoadChainstate constructs CEvoDB with wipe=true for -reindex and -reindex-chainstate, erasing the SNAPSHOT marker while chainstate_snapshot and its base-blockhash file stay on disk. DetectSnapshotChainstate then found the directory and ActivateExistingSnapshot rejected it on the marker just erased, so startup failed with advice ('reindex is required') the user had already followed -- on every retry. Discard the persisted snapshot chainstate's on-disk artifacts alongside the EvoDB wipe. This is the Dash-shaped equivalent of the reindex-time cleanup in bitcoin#27596/bitcoin#29726; it runs before any chainstate has coins views, so no mempool transfer or leveldb-lock dance is needed.
993d53b to
066fca8
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 066fca8b4f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| nodeman->UpdatedBlockTip(pindexNew, pindexFork, fInitialDownload); | ||
|
|
||
| if (m_chainman.IsSnapshotActiveAndUnvalidated()) { |
There was a problem hiding this comment.
Move snapshot gate before masternode tip update
This guard runs after nodeman->UpdatedBlockTip(), so an active masternode that is not yet READY during an unvalidated snapshot can enter CActiveMasternodeManager::UpdatedBlockTip()'s Init(pindexNew) path before the gate is checked; InitInternal() calls GetListForBlock(pindexNew) without the new BlockDataUnavailableError catch, and that uncaught exception is on the scheduler validation callback path. In the same missing-history snapshot scenario this gate is meant to suppress, the node can terminate before DKG/signing is disabled, so check IsSnapshotActiveAndUnvalidated() before calling nodeman or catch the unavailable-list exception in the init path as well.
AGENTS.md reference: AGENTS.md:L168-L170
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/llmq/snapshot.cpp (1)
22-29: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd coverage for unavailability without peer penalization.
GETQUORUMROTATIONINFOalready treatsis not available (pruned or below an unvalidated snapshot base)as local unavailability, butp2p_quorum_data.pyshould include a pruned/unvalidated-snapshot request to cover this peer-sparing path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/llmq/snapshot.cpp` around lines 22 - 29, Extend the GETQUORUMROTATIONINFO coverage in p2p_quorum_data.py with a request for block data that is unavailable because it is pruned or below an unvalidated snapshot base. Assert that this local-unavailability response does not penalize the peer, preserving the existing handling for the error text produced by CheckBlockDataAvailable.
🤖 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.cpp`:
- Around line 67-74: Update CEvoDB::BeginTransaction so GetContext(identity) and
CEvoDBScopedCommitter construction complete before assigning active_transaction
and active_transaction_thread; only publish transaction state after both
fallible operations succeed, while preserving the existing lock and returned
committer behavior.
In `@src/validation.cpp`:
- Around line 3594-3597: Require this == &m_chainman.ActiveChainstate() in the
UI tip notification conditions for both invalidation handling at
src/validation.cpp lines 3594-3597 and conflicting-chain handling at lines
3699-3702, alongside the existing pindex_was_in_chain checks; leave the
main-signal notifications unchanged.
- Line 2011: Initialize the required pre-disconnect BLS scheme before
UndoSpecialTxsInBlock in src/validation.cpp:2011, while preserving the scheme
transition committed by DisconnectTip; also initialize the pre-connect scheme
before ProcessSpecialTxsInBlock during replay in src/validation.cpp:4655 and
restore the active-chainstate scheme afterward. Add coverage for interrupted
post-V19 snapshot replay and CVerifyDB validation.
---
Nitpick comments:
In `@src/llmq/snapshot.cpp`:
- Around line 22-29: Extend the GETQUORUMROTATIONINFO coverage in
p2p_quorum_data.py with a request for block data that is unavailable because it
is pruned or below an unvalidated snapshot base. Assert that this
local-unavailability response does not penalize the peer, preserving the
existing handling for the error text produced by CheckBlockDataAvailable.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 37a9ab73-7488-4e6f-9838-ceae31e6e69a
📒 Files selected for processing (45)
src/Makefile.test.includesrc/active/context.cppsrc/active/context.hsrc/active/dkgsessionhandler.cppsrc/active/masternode.cppsrc/dbwrapper.hsrc/evo/assetlocktx.cppsrc/evo/assetlocktx.hsrc/evo/chainhelper.cppsrc/evo/chainhelper.hsrc/evo/creditpool.cppsrc/evo/deterministicmns.cppsrc/evo/deterministicmns.hsrc/evo/evodb.cppsrc/evo/evodb.hsrc/evo/mnhftx.cppsrc/evo/mnhftx.hsrc/evo/smldiff.cppsrc/evo/smldiff.hsrc/evo/specialtxman.cppsrc/evo/specialtxman.hsrc/governance/signing.cppsrc/llmq/blockprocessor.cppsrc/llmq/blockprocessor.hsrc/llmq/quorumsman.cppsrc/llmq/quorumsman.hsrc/llmq/signing_shares.cppsrc/llmq/signing_shares.hsrc/llmq/snapshot.cppsrc/net_processing.cppsrc/node/chainstate.cppsrc/node/miner.cppsrc/rpc/blockchain.cppsrc/rpc/masternode.cppsrc/rpc/quorums.cppsrc/test/evo_cbtx_tests.cppsrc/test/evo_db_tests.cppsrc/test/evo_deterministicmns_tests.cppsrc/test/util/setup_common.cppsrc/test/util/setup_common.hsrc/test/validation_chainstate_tests.cppsrc/test/validation_chainstatemanager_tests.cppsrc/validation.cppsrc/validation.hsrc/versionbits.h
🚧 Files skipped from review as they are similar to previous changes (36)
- src/versionbits.h
- src/Makefile.test.include
- src/llmq/signing_shares.h
- src/test/evo_deterministicmns_tests.cpp
- src/net_processing.cpp
- src/rpc/masternode.cpp
- src/rpc/quorums.cpp
- src/active/context.cpp
- src/node/miner.cpp
- src/llmq/quorumsman.h
- src/dbwrapper.h
- src/governance/signing.cpp
- src/llmq/signing_shares.cpp
- src/rpc/blockchain.cpp
- src/evo/smldiff.cpp
- src/evo/chainhelper.cpp
- src/test/evo_cbtx_tests.cpp
- src/evo/chainhelper.h
- src/evo/creditpool.cpp
- src/evo/assetlocktx.h
- src/evo/mnhftx.cpp
- src/evo/specialtxman.h
- src/active/dkgsessionhandler.cpp
- src/llmq/blockprocessor.h
- src/active/masternode.cpp
- src/evo/deterministicmns.cpp
- src/evo/smldiff.h
- src/test/util/setup_common.cpp
- src/evo/specialtxman.cpp
- src/active/context.h
- src/llmq/quorumsman.cpp
- src/evo/assetlocktx.cpp
- src/test/validation_chainstatemanager_tests.cpp
- src/llmq/blockprocessor.cpp
- src/evo/evodb.h
- src/evo/mnhftx.h
| std::unique_ptr<CEvoDBScopedCommitter> CEvoDB::BeginTransaction(EvoDbIdentity identity) | ||
| { | ||
| LOCK(cs); | ||
| assert(!active_transaction.has_value()); | ||
| active_transaction = identity; | ||
| active_transaction_thread = std::this_thread::get_id(); | ||
| GetContext(identity); | ||
| return std::make_unique<CEvoDBScopedCommitter>(*this, identity); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate evodb files =="
fd -a 'evodb\.(cpp|h)$|EvoDb' . || true
echo "== evodb outline =="
ast-grep outline src/evo/evodb.cpp --view expanded || true
echo "== relevant evodb.cpp sections =="
wc -l src/evo/evodb.cpp src/evo/evodb.h
sed -n '1,180p' src/evo/evodb.cpp
echo "== evodb.h =="
sed -n '1,240p' src/evo/evodb.h
echo "== calls to BeginTransaction and active_transaction =="
rg -n "BeginTransaction|active_transaction|active_transaction_thread|CEvoDBScopedCommitter|GetContext|Start|Commit|Rollback" src test contrib -S || trueRepository: dashpay/dash
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== evodb.cpp relevant lines =="
wc -l src/evo/evodb.cpp src/evo/evodb.h
sed -n '1,130p' src/evo/evodb.cpp
echo
sed -n '1,220p' src/evo/evodb.h
echo
echo "== focused BeginTransaction/assert usages =="
rg -n "active_transaction|active_transaction_thread|BeginTransaction|GetContext\\(|Commit|Rollback|scoped" src/evo/evodb.* src/test/evo_db_tests.cpp src -g '*.cpp' -g '*.h' -S
echo
echo "== evodb tests relevant sections =="
sed -n '1,120p' src/test/evo_db_tests.cpp
sed -n '120,280p' src/test/evo_db_tests.cppRepository: dashpay/dash
Length of output: 50369
Set transaction state after fallible construction.
GetContext(identity) and std::make_unique<CEvoDBScopedCommitter> can throw after active_transaction is set. The failed call leaves state without a committer to clear it, so the next BeginTransaction asserts and an in-party transaction may resolve with the stale identity if the caller reuses its thread. Build the context and committer before assigning active_transaction and active_transaction_thread.
🤖 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 67 - 74, Update CEvoDB::BeginTransaction so
GetContext(identity) and CEvoDBScopedCommitter construction complete before
assigning active_transaction and active_transaction_thread; only publish
transaction state after both fallible operations succeed, while preserving the
existing lock and returned committer behavior.
|
|
||
| std::optional<MNListUpdates> mnlist_updates_opt{std::nullopt}; | ||
| if (!m_chain_helper->special_tx->UndoSpecialTxsInBlock(block, pindex, mnlist_updates_opt)) { | ||
| if (!m_chain_helper->special_tx->UndoSpecialTxsInBlock(*this, block, pindex, mnlist_updates_opt)) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Initialize the BLS scheme in direct replay and verification paths.
ReplayBlocks() and CVerifyDB call DisconnectBlock() directly. ReplayBlocks() also calls RollforwardBlock() directly. These paths bypass the guards in DisconnectTip() and ConnectBlock(). A post-V19 operation can then process BLS data with the scheme left by another chainstate.
src/validation.cpp#L2011-L2011: establish the required pre-disconnect scheme for directDisconnectBlock()callers while preserving the transition thatDisconnectTip()commits.src/validation.cpp#L4655-L4655: establish the required pre-connect scheme beforeProcessSpecialTxsInBlock()during replay, then restore the active-chainstate scheme after replay.
Add coverage for interrupted post-V19 snapshot replay and CVerifyDB validation.
📍 Affects 1 file
src/validation.cpp#L2011-L2011(this comment)src/validation.cpp#L4655-L4655
🤖 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` at line 2011, Initialize the required pre-disconnect BLS
scheme before UndoSpecialTxsInBlock in src/validation.cpp:2011, while preserving
the scheme transition committed by DisconnectTip; also initialize the
pre-connect scheme before ProcessSpecialTxsInBlock during replay in
src/validation.cpp:4655 and restore the active-chainstate scheme afterward. Add
coverage for interrupted post-V19 snapshot replay and CVerifyDB validation.
| if (this == &m_chainman.ActiveChainstate()) { | ||
| GetMainSignals().SynchronousUpdatedBlockTip(m_chain.Tip(), nullptr, IsInitialBlockDownload()); | ||
| GetMainSignals().UpdatedBlockTip(m_chain.Tip(), nullptr, IsInitialBlockDownload()); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Suppress UI tip notifications for background chainstate changes.
GetMainSignals() now checks the active chainstate. The nearby uiInterface.NotifyBlockTip() calls only check pindex_was_in_chain. A background invalidation or chainlock conflict can therefore report its tip as a UI tip update.
src/validation.cpp#L3594-L3597: require the active-chainstate check in the invalidation UI notification condition.src/validation.cpp#L3699-L3702: require the active-chainstate check in the conflicting-chain UI notification condition.
📍 Affects 1 file
src/validation.cpp#L3594-L3597(this comment)src/validation.cpp#L3699-L3702
🤖 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 3594 - 3597, Require this ==
&m_chainman.ActiveChainstate() in the UI tip notification conditions for both
invalidation handling at src/validation.cpp lines 3594-3597 and
conflicting-chain handling at lines 3699-3702, alongside the existing
pindex_was_in_chain checks; leave the main-signal notifications unchanged.
knst
left a comment
There was a problem hiding this comment.
LGTM 066fca8
I'd probably have some comments for code in this PR when I will see assumeutxo M3....assumeutxo M7 but for now I am happy with Fable's responses here: #7456 (comment)
| CDeterministicMNState::GetJsonHelp(/*key=*/"dmnState", /*optional=*/true), | ||
| {RPCResult::Type::STR, "state", "Masternode state (human-readable string)"}, | ||
| {RPCResult::Type::STR, "status", "Masternode status (human-readable string, based on current state)"}, | ||
| {RPCResult::Type::BOOL, "quorumParticipation", "Whether DKG participation and quorum signing are enabled"}, |
There was a problem hiding this comment.
I think the name of quorumPparticipation is a bit missleading ; can't figure out better naming.
@thepastaclaw any ideas?
…ompletion and snapshot promotion c28ddc8 test: cover background MN-hash erase and the promote/discard recovery overlap (pasta) a8b6790 docs: fix stale check_block_index reference and explain deferred candidate admission (pasta) d16440b refactor: centralize durable rename/remove for the snapshot lifecycle (pasta) 413afb1 fix: report a missing snapshot base block as a startup error, not an abort (pasta) 4b1294c test: assert cold snapshot activation captures no base MN-list marker (pasta) cedba66 docs: note the cross-chainstate duplicate-commitment corner (pasta) ab3b671 validation: move the mempool to the snapshot chainstate on activation (pasta) 44bd797 refactor: share the block-data-unavailable sentinel as a named constant (pasta) 2ef1222 backport: adapt bitcoin#27746 for Dash (Pasta) 43efc47 Merge bitcoin#27746: Rework validation logic for assumeutxo (Ryan Ofsky) d1ffdd6 Merge bitcoin#28050: test: make assumeUTXO test capture the expected fatal error (Pasta) 3da9105 Merge bitcoin#27862: validation: Stricter assumeutxo error handling when renaming chainstates (Pasta) 9cfc65a backport: adapt Dash EvoDB completion path for bitcoin#25740 (Pasta) 483b3d7 Merge bitcoin#25740: assumeutxo: background validation completion (Pasta) Pull request description: ## Issue being fixed or feature implemented [M1 (dashpay#7451)](dashpay#7451) added AssumeUTXO snapshot persistence and [M2 (dashpay#7456)](dashpay#7456) gave the snapshot and background chainstates independent EvoDB identities, markers, and chain-aware Dash validation. What was still missing is the end of the lifecycle: nothing ever *completed* background validation, so a snapshot-backed node stayed in the dual-chainstate state (with DKG participation and quorum signing disabled) forever. This is milestone 3 of the AssumeUTXO series: background validation completion. When the background chainstate reaches the snapshot base block, the node now verifies the background-derived state against the snapshot, disables the background chainstate, and on the next restart promotes the snapshot chainstate (coins directory and EvoDB markers) to the normal single-chainstate layout. ## What was done? Upstream backports (kept 1:1 where practical, Dash adaptations in separate commits): - **bitcoin#25740** — assumeutxo: background validation completion. Adds `ChainstateManager::MaybeCompleteSnapshotValidation()` (UTXO-set hash comparison against `m_assumeutxo_data` when the background tip reaches the base block) and `ValidatedSnapshotCleanup()` (restart-time promotion of `chainstate_snapshot` over `chainstate`), with `SnapshotCompletionResult` reporting and the `chainstate_snapshot_INVALID` quarantine path. - **bitcoin#27862** — stricter error handling when renaming chainstates (abort instead of continuing after a failed rename). - **bitcoin#28050** — test: capture the expected fatal error during assumeutxo completion failure. - **bitcoin#27746** — rework validation logic for assumeutxo: `Chainstate::m_disabled` replaces ad-hoc usability checks, `CompleteChainstateInitialization()` split out of `LoadChainstate()` so chainstates can be reinitialized after cleanup, `LoadExternalBlockFile` moved to `ChainstateManager`, per-blockfile undo tracking without the active-chain reference, and `BLOCK_ASSUMED_VALID` documentation/semantics updates. Dash-specific completion path: - **EvoDB marker promotion.** `CEvoDB::PromoteSnapshotMarkers()` atomically (single synced batch) moves the SNAPSHOT best-block marker to the legacy NORMAL key and removes all dual-chainstate metadata; `DiscardSnapshotMarkers()` does the same for a rejected snapshot while preserving NORMAL state. Both reset the transaction-less default identity to NORMAL, closing the `TODO(assumeutxo)` markers left in M2. - **Base-state comparison.** Snapshot activation records a canonical hash of the deterministic masternode list at the base block (`EVODB_SNAPSHOT_MNLIST_HASH`); the background chainstate independently records the list hash it derives when it connects the base block (`EVODB_BACKGROUND_MNLIST_HASH`). Completion compares them (in addition to the upstream UTXO-set hash) and fails with `SnapshotCompletionResult::EVO_STATE_MISMATCH` on divergence. This is the first installment of the holistic base-state comparison M2 deferred; extending it to the CbTx `merkleRootMNList`/`merkleRootQuorums` and credit-pool commitments is called out as a TODO for the `loadtxoutset` milestone, where the snapshot payload gains Dash state. Until then this comparison is a **corruption tripwire, not an independent check**: in the only case both markers exist (activation with the background tip already at the base) they are written from a single derivation, and a cold-start activation captures neither and skips the comparison. Divergence therefore only signals on-disk damage to the marker pair; the independent comparison arrives when the snapshot payload carries the base MN list. - **Crash recovery.** `ValidatedSnapshotCleanup()` performs two directory renames plus a marker promotion, each individually durable. `RecoverSnapshotCleanup()` (run at startup before chainstate detection) classifies every interruption point — first rename done, both renames done with markers pending, promotion durable but deletion pending, invalid-snapshot rename done with marker discard pending — and either rolls back, finishes the promotion, or fails with a precise error instead of the generic reindex advice. - **Lifecycle correctness fixes discovered while wiring the above:** `EraseSnapshotMarkers()` (the abandoned-activation rollback from M2) now also erases the new MN-list-hash markers; snapshot activation moves the mempool to the snapshot chainstate and restart activation clears it from the background chainstate (the bitcoin#27596 shape), so background block connects can no longer call `removeForBlock`/`removeExpiredAssetUnlock` against mempool state built on the snapshot tip; the invalid-snapshot revert hands the mempool back. Review follow-ups from the M2 merge applied here: - The peer-penalty exemption for unavailable history no longer depends on three files repeating one literal string: the sentinel is a named constant (`BLOCK_DATA_UNAVAILABLE_SUFFIX`) shared by every producer and the matcher. - The background MN-list hash is computed only for the snapshot base block (activation captures it directly when the background tip is already at the base). The initial implementation hashed the full deterministic MN list on every block connect, which would have been a measurable IBD regression on every node. - A comment documents the cross-chainstate duplicate-commitment corner in `CQuorumBlockProcessor::ProcessCommitment`. Review follow-ups from the dashpay#7553 review round: - A snapshot base block missing from the on-disk block index is now reported by `ChainstateManager::LoadBlockIndex()` as a normal startup failure (recoverable via the standard reindex advice), instead of aborting in candidate admission; `GetSnapshotBaseBlock()` regains upstream's cached `SnapshotBase()` delegation (bitcoin d4a11ab) that the initial adaptation had dropped. Note the related deliberate deviation: `MaybeCompleteSnapshotValidation()` converts one upstream hard assert into a `SKIPPED` return for synthetic in-memory unit fixtures, discriminated by `CoinsDB().StoragePath()` being empty. - Every snapshot-lifecycle directory rename/removal now goes through new `RenameDurably()`/`RemoveAllDurably()` helpers (`fs::rename`/`fs::remove_all` + `DirectoryCommit`), so the crash-recovery invariant is enforced by the helper rather than by remembering a follow-up call at six sites. - New tests: a missing-base startup failure, the promote/discard overlap state in `RecoverSnapshotCleanup` (must land in the same end state as a completed discard), and background-MN-hash coverage in the marker rollback test. With completion wired, the M2 duty gate resolves end-to-end: `IsSnapshotActiveAndUnvalidated()` becomes false at completion, so DKG participation and quorum signing re-enable without a restart, and the `masternode status` clause clears. ## How Has This Been Tested? - Rebased onto develop immediately after the M2 merge (`ab65592f85d`); every conflict was resolved against M2's final review round (thread-scoped EvoDB transactions, `EraseSnapshotMarkers`, reindex-time snapshot discard, fallible `DetectSnapshotChainstate`, BLS scheme establishment). The merged M2 test `chainstate_connectblock_bls_scheme` is adapted in the bitcoin#27746 commit for `AcceptBlock` moving to `ChainstateManager`. - Full clean build (autotools, `--enable-debug`), then the complete `test_dash` suite passes ("No errors detected"), including targeted reruns of `evo_db_tests`, `validation_chainstatemanager_tests`, `validation_chainstate_tests`, `evo_deterministicmns_tests`, `evo_mnhf_tests`, `evo_assetlocks_tests`, `evo_cbtx_tests`, `blockmanager_tests`, `coinstatsindex_tests`, and `validation_block_tests`. - New coverage: `snapshot_marker_promotion_and_discard` (promotion/discard idempotency across restarts), the extended abandoned-activation marker rollback test, `chainstatemanager_snapshot_completion` and `_hash_mismatch` (upstream-shaped), an `EVO_STATE_MISMATCH` completion case, four crash-recovery tests that each reproduce a distinct `ValidatedSnapshotCleanup` interruption point on disk and drive it through `LoadVerifyActivateChainstate()`, and mempool-ownership assertions at both activation paths. - An independent review pass traced the highest-risk interactions end to end: the `ConnectTip` → `MaybeCompleteSnapshotValidation` EvoDB transaction lifecycle (the scoped committer closes before completion runs, so the single-open-transaction invariant holds), BLS-scheme guard nesting across connect/disconnect, all four mempool handoff transitions, and the recovery state machine. Its two "correct but implicit" findings are addressed in the final commit (at-rest raw reads for the lifecycle markers; a comment documenting the deliberate promote/discard overlap in `RecoverSnapshotCleanup`). - `lint-circular-dependencies`, `lint-python`, and `git diff --check` are clean. ## Breaking Changes None released. The dual-chainstate on-disk state introduced in M2 (unreleased) gains two lifecycle marker keys (`b_dcs_mn`, `b_dcs_bg_mn`); nodes that never load a snapshot never write any of them. `ChainstateLoadStatus::FAILURE_FATAL` is a new internal failure class treated like `FAILURE_INCOMPATIBLE_DB` at init. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation (`doc/design/assumeutxo.md` updated for the implemented lifecycle; the user-facing AssumeUTXO documentation lands with `loadtxoutset`) - [ ] I have assigned this pull request to a milestone Top commit has no ACKs. Tree-SHA512: 0a5b28e1d1301a9c7a3bd0d825e2890408baf6d93ef697c8569913f680540122af277f3860386416bdbb5df5d6ee087af8f1f9af8780bd3d7a5967a420fa4896
Issue being fixed or feature implemented
M1 (#7451) added AssumeUTXO snapshot persistence, but Dash stores deterministic masternode, quorum, MNHF, and credit-pool state in a shared EvoDB. Running the snapshot and background chainstates concurrently therefore requires independent EvoDB transaction state and markers, chain-aware Dash validation, and protection against emitting or signing from the wrong chainstate.
This is milestone 2 of the AssumeUTXO series. It supplies the Dash-specific multi-chainstate foundation required by the later background-completion and
loadtxoutsetmilestones.What was done?
WriteDerivedfor immutable block-derived records. Independently derived values must serialize identically, including values pending in the other chainstate's overlay.BlockCheckedremains ungated because it reports validation results rather than active-tip changes.Review follow-ups (appended commits):
WriteDerivedmismatch is local EvoDB corruption, never evidence about the block. It now aborts the node withM_ERROR(matching the existingEvoDbInconsistencyMessageconvention) instead of marking the blockBLOCK_CONSENSUS-invalid and penalizing the relaying peer. A typedEvoDbInconsistencyErrorpreserves that classification through the catch blocks on the miner, RPC, and MNHF-recomputation paths.BeginTransactionscope previously always bound to the NORMAL identity, so transaction-less consumers (RPC, mempool, miner, P2P serving) could not see snapshot-chain records pending in the SNAPSHOT overlay.CEvoDBnow tracks a default identity that snapshot activation sets to SNAPSHOT andResetChainstatesresets; the background-completion milestone must reset it to NORMAL at marker promotion (TODO noted in code).GetListForBlockInternalno longer fabricates an empty "initial snapshot" masternode list when a diff for a DIP3-active block is missing; it throws instead. The message deliberately carries theIsBlockDataUnavailableErrorsentinel and is deliberately a plainruntime_errorrather thanEvoDbInconsistencyError: at that layer a missing diff can be benign (pending in the other chainstate's unflushed overlay, e.g. while serving historicalmnlistdiff), so it is reported as unavailable history without penalizing the requesting peer, and only definite mismatches abort the node. Scheduler-thread consumers (CActiveMasternodeManager::UpdatedBlockTip, governance trigger creation) catch it and skip the update, since an uncaught exception there would terminate the node.Second review round (appended commits):
bls::bls_legacy_schemeis process-wide, but the correct value belongs to the block being validated.ConnectBlockonly saved and committed the flag; it never established it, andProcessSpecialTxsInBlockswitches legacy to basic only when crossing V19 forward. With an active post-V19 snapshot, the background chainstate therefore validated pre-V19 blocks under the basic scheme, and a background disconnect across V19 could commit legacy onto the active chain's consumers.ConnectBlocknow enters under the scheme the block's parent left behind, andConnectTip/DisconnectTipcommit it only from the active chainstate.std::exception, so the plainruntime_errors thatCDeterministicMNList::ApplyDiffraises for missing removals or updates, duplicate masternodes, and duplicate unique properties were logged as benign unavailable history while the active masternode stayed READY. The unavailable-history condition now has its ownBlockDataUnavailableErrortype and is the only thing those guards (andBuildSimplifiedMNListDiff) catch; everything else propagates exactly as before the guards existed.CEvoDB::GetCurrentIdentity()resolved to a process-wideactive_transaction, so an open background NORMAL transaction redirected every concurrent transaction-less read away from the active snapshot's overlay. An open transaction is one validation execution context, not a process mode, so it now resolves only for the thread that began it.PopulateAndValidateSnapshotcommits the SNAPSHOT best-block and dual-chainstate markers as its last step, so abandoning activation afterwards (e.g.WriteSnapshotBaseBlockhashon an unwritable datadir) left a single-chainstate node carrying both; and-reindex/-reindex-chainstatewiped the shared EvoDB, erasing the SNAPSHOT marker whilechainstate_snapshotstayed on disk, so startup failed on the erased marker with advice the user had already followed. Markers are now rolled back on abandoned activation, and the persisted snapshot chainstate's on-disk artifacts are discarded alongside the EvoDB wipe -- the Dash-shaped equivalent of the reindex-time cleanup in assumeutxo (2) bitcoin/bitcoin#27596/assumeutxo: Fix -reindex before snapshot was validated bitcoin/bitcoin#29726, which runs before any chainstate has coins views and so needs no mempool transfer or leveldb-lock dance.The batch contains the original eight focused commits, a rebase fixture adaptation after the txindex removal, and eight review follow-up commits. The partial Bitcoin Core
BlockInfoandChainstateRoleprerequisites were intentionally moved to the laterloadtxoutsetmilestone where their APIs are first consumed.How Has This Been Tested?
transaction_less_access_uses_default_identitycovering default-identity resolution of transaction-less reads and writes. Known gap: the DIP3-active missing-diff throw has no end-to-end unit test because regtest activates DIP3 at height 432, above the unit-test chain heights; it is covered by the serving-path catch and review.make checkpassed and the following functional subset passed:feature_mnehf.py,feature_asset_locks.py,feature_dip3_deterministicmns.py(both modes),feature_llmq_signing.py(both modes),feature_llmq_rotation.py, andrpc_quorum.py.evo_db_tests,evo_deterministicmns_tests,evo_mnhf_tests,evo_assetlocks_tests, andvalidation_chainstatemanager_testspass (20 cases), andlint-circular-dependenciesis clean.test_dashsuite passes (815 cases). Each new regression test was negative-controlled --chainstate_connectblock_bls_schemefails on both halves of the BLS fix independently,open_transaction_does_not_capture_other_threadsfails without the thread scoping, andchainstatemanager_snapshot_discarded_on_reindexreproduces the unrecoverable reindex before the fix.git diff-tree --check.Breaking Changes
No released-interface changes. Internally,
ActivateExistingSnapshotbecomes fallible so startup can reject a snapshot chainstate whose EvoDB marker is missing. Two RPC-visible additions while a snapshot is active and unvalidated:masternode statusgains aquorumParticipationfield and appends a "DKG participation and quorum signing disabled" clause tostatus, andquorum signfails with an explanatory error.AbstractEHFManager::Signalschanged fromstd::unordered_maptostd::map(canonical serialization); the on-disk encoding is unchanged, but iteration order of EHF signals ingetblockchaininfooutput is now sorted.Checklist: