backport: assumeutxo M4 — evo snapshot format v3 and LLMQ reconstruction - #7579
backport: assumeutxo M4 — evo snapshot format v3 and LLMQ reconstruction#7579PastaPastaPasta wants to merge 23 commits into
Conversation
…hen renaming chainstates
Discard Dash snapshot lifecycle markers only after the invalid snapshot directory rename succeeds. If the rename fails, preserving the markers keeps the existing restart recovery state recognizable while the upstream rename error is propagated to the fatal shutdown message.
Keep Dash’s mock shutdown callback while asserting the expected fatal diagnostic. The default callback reaches StartShutdown(), whose unit-test guard aborts the process before this Dash test can complete.
a733dd7 Remove unused function `reliesOnAssumedValid` (Suhas Daftuar) d4a11ab Cache block index entry corresponding to assumeutxo snapshot base blockhash (Suhas Daftuar) 3556b85 Move CheckBlockIndex() from Chainstate to ChainstateManager (Suhas Daftuar) 0ce805b Documentation improvements for assumeutxo (Ryan Ofsky) 768690b Fix initialization of setBlockIndexCandidates when working with multiple chainstates (Suhas Daftuar) d43a1f1 Tighten requirements for adding elements to setBlockIndexCandidates (Suhas Daftuar) d0d40ea Move block-storage-related logic to ChainstateManager (Suhas Daftuar) 3cfc753 test: Clear block index flags when testing snapshots (Suhas Daftuar) 272fbc3 Update CheckBlockIndex invariants for chains based on an assumeutxo snapshot (Suhas Daftuar) 10c0571 Add wrapper for adding entries to a chainstate's block index candidates (Suhas Daftuar) 471da5f Move block-arrival information / preciousblock counters to ChainstateManager (Suhas Daftuar) 1cfc887 Remove CChain dependency in node/blockstorage (Suhas Daftuar) fe86a7c Explicitly track maximum block height stored in undo files (Suhas Daftuar) Pull request description: This PR proposes a clean up of the relationship between block storage and the chainstate objects, by moving the decision of whether to store a block on disk to something that is not chainstate-specific. Philosophically, the decision of whether to store a block on disk is related to validation rules that do not require any UTXO state; for anti-DoS reasons we were using some chainstate-specific heuristics, and those have been reworked here to achieve the proposed separation. This PR also fixes a bug in how a chainstate's `setBlockIndexCandidates` was being initialized; it should always have all the HAVE_DATA block index entries that have more work than the chain tip. During startup, we were not fully populating `setBlockIndexCandidates` in some scenarios involving multiple chainstates. Further, this PR establishes a concept that whenever we have 2 chainstates, that we always know the snapshotted chain's base block and the base block's hash must be an element of our block index. Given that, we can establish a new invariant that the background validation chainstate only needs to consider blocks leading to that snapshotted block entry as potential candidates for its tip. As a followup I would imagine that when writing net_processing logic to download blocks for the background chainstate, that we would use this concept to only download blocks towards the snapshotted entry as well. ACKs for top commit: achow101: ACK a733dd7 jamesob: reACK a733dd7 ([`jamesob/ackr/27746.5.sdaftuar.rework_validation_logic`](https://github.com/jamesob/bitcoin/tree/ackr/27746.5.sdaftuar.rework_validation_logic)) Sjors: Code review ACK a733dd7. ryanofsky: Code review ACK a733dd7. Just suggested changes since the last review. There are various small things that could be followed up on, but I think this is ready for merge. Tree-SHA512: 9ec17746f22b9c27082743ee581b8adceb2bd322fceafa507b428bdcc3ffb8b4c6601fc61cc7bb1161f890c3d38503e8b49474da7b5ab1b1f38bda7aa8668675
Preserve ChainLock candidate exclusions in the new admission wrapper and keep Dash background-notification and EvoDB fixtures consistent with the tightened multi-chainstate candidate invariants.
Peer-penalty exemption for unavailable history hinged on three files repeating one literal string that IsBlockDataUnavailableError() then substring-matched; rewording any copy would silently revert those paths to Misbehaving. Define the suffix once next to BlockDataUnavailableError and use it at every producer and in the matcher.
Both chainstates carried a live mempool pointer after snapshot activation, so background ConnectTip called removeForBlock and removeExpiredAssetUnlock with historical blocks and lower heights. Follow the bitcoin#27596 shape: only the active chainstate keeps the mempool. Runtime activation transfers it to the snapshot chainstate, restart activation clears it from the background chainstate, and the invalid-snapshot revert hands it back.
The background chainstate in this fixture is reset to genesis before activation, so the base MN list is not derivable; a capture at activation would fabricate an empty list and poison the shared list cache (thepastaclaw review finding).
|
⛔ Blockers found — Opus deferred (commit aadbe61) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08ac564912
ℹ️ 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".
| EraseHistoricalMNListMarkers(*db, batch); | ||
| batch.Erase(EVODB_SNAPSHOT_EVO_SECTION); | ||
| batch.Erase(EVODB_DUAL_CHAINSTATE); |
There was a problem hiding this comment.
When background validation rejects a loaded snapshot, this batch removes only lifecycle metadata, while PopulateAndValidateSnapshot() has already committed snapshot-provided DB_LIST_SNAPSHOT, quorum commitments/snapshots/modifiers, credit-pool, and MNHF records into the shared, unprefixed EvoDB namespace. After shutdown and restart, the restored NORMAL chainstate can consume those invalid records—for example, GetListForBlockInternal() checks DB_LIST_SNAPSHOT before reconstructing from NORMAL diffs—so the advertised fallback to independently validated state remains contaminated and can reproduce the mismatch or use rejected masternode/quorum state. The discard path must remove or replace every seeded record using the independently validated background state, or snapshot-derived records must be identity-isolated.
AGENTS.md reference: AGENTS.md:L162-L180
Useful? React with 👍 / 👎.
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
If these PRs merge firstThis PR will likely need a rebase:
|
WalkthroughThis PR adds Evo snapshot serialization, canonical validation, hashing, and chain-aware reconstruction. It loads UTXO and Evo state into a snapshot chainstate, seeds deterministic masternode and quorum data, and validates background state before promotion. It adds EvoDB snapshot markers, filesystem recovery, pruning protection, mismatch quarantine, and multi-chainstate candidate handling. The RPC now emits Evo snapshot metadata. Unit, integration, functional, fuzz, and recovery tests cover the new paths. Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant RPC
participant ChainstateManager
participant CEvoSnapshot
participant CEvoDB
RPC->>ChainstateManager: create UTXO snapshot
ChainstateManager->>CEvoSnapshot: build and validate Evo snapshot
ChainstateManager->>CEvoDB: persist snapshot state and markers
ChainstateManager-->>RPC: return snapshot hash and MN count
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/evo/evodb.cpp (1)
152-160: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winErase all snapshot lifecycle markers in
EraseSnapshotMarkers.The failure path can run after the
NORMALandSNAPSHOTmarker commits.EraseSnapshotMarkersdoes not removeEVODB_REQUIRED_WORK_MNLISTS, its per-blockEVODB_BACKGROUND_WORK_MNLIST_HASHentries, orEVODB_SNAPSHOT_EVO_SECTION, so stale metadata remains.🤖 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 152 - 160, Update CEvoDB::EraseSnapshotMarkers to erase every snapshot lifecycle marker, including EVODB_REQUIRED_WORK_MNLISTS, all per-block EVODB_BACKGROUND_WORK_MNLIST_HASH entries, and EVODB_SNAPSHOT_EVO_SECTION, alongside the existing markers. Use the established key/prefix range deletion mechanism for the per-block entries.
🧹 Nitpick comments (11)
src/llmq/blockprocessor.cpp (1)
703-712: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winInvalidate the memoized quorum-hash caches after seeding.
The comment states that this replays
ProcessCommitment's iteration index exactly.ProcessCommitmentalso callsDropQcHashesCache()after the writes (line 432), andSeedMinedCommitmentdoes not.
HasMinedCommitmentis already safe here, because the negative-caching path was removed for exactly this reason (lines 661-663). The remaining exposure ism_qc_hashes_lru, which is keyed on the quorum base-block hash and is not keyed on the mined commitment. A positive entry cached before seeding stays stale after seeding and would feed a wrong quorum merkle root throughGetQcHashes.Call
DropQcHashesCache()at the end ofSeedMinedCommitment. This also requires adding!m_qc_hashes_cache_mutexto the declaration insrc/llmq/blockprocessor.hline 132.🛡️ Proposed change
if (IsQuorumRotationEnabled(*llmq_params, quorum_base_index)) { m_evoDb.Write(BuildInversedHeightKeyIndexed(llmqType, mined_index->nHeight, int(commitment.quorumIndex)), quorum_base_index->nHeight); } else { m_evoDb.Write(BuildInversedHeightKey(llmqType, mined_index->nHeight), quorum_base_index->nHeight); } + // Only once this commitment's state change is complete; see ProcessCommitment. + DropQcHashesCache(); return true; }And in
src/llmq/blockprocessor.h:bool SeedMinedCommitment(Consensus::LLMQType llmqType, const uint256& quorum_hash, const CFinalCommitment& commitment, const uint256& mined_block_hash) - EXCLUSIVE_LOCKS_REQUIRED(::cs_main); + EXCLUSIVE_LOCKS_REQUIRED(::cs_main, !m_qc_hashes_cache_mutex);🤖 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/blockprocessor.cpp` around lines 703 - 712, Update SeedMinedCommitment to call DropQcHashesCache() after completing the commitment-seeding writes and before returning. Add the required !m_qc_hashes_cache_mutex declaration to the SeedMinedCommitment declaration in the class header, matching the cache-invalidation synchronization requirements.src/llmq/snapshot.cpp (1)
334-349: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winName the modifier key prefix as a constant.
"llmq_M3"appears as a literal inSeedQuorumModifierand again inGetSeededQuorumModifier. The file already declaresDB_QUORUM_SNAPSHOTat line 21 for the same purpose.If the two literals ever diverge, every read misses.
GetSeededQuorumModifierthen returnsstd::nullopt, andllmq::utils::GetHashModifier(src/llmq/utils.cpplines 118-132) silently falls back toCalculateHashModifier. On a snapshot node whose work block has no block data, that fallback is the case the seeding exists to prevent, and it produces no error.♻️ Proposed change
constexpr std::string_view DB_QUORUM_SNAPSHOT{"llmq_S"}; +constexpr std::string_view DB_QUORUM_MODIFIER{"llmq_M3"};- return m_evoDb.WriteDerived(std::make_tuple(std::string_view{"llmq_M3"}, llmq_type, work_block_hash), modifier); + return m_evoDb.WriteDerived(std::make_tuple(DB_QUORUM_MODIFIER, llmq_type, work_block_hash), modifier);- if (!m_evoDb.Read(std::make_tuple(std::string_view{"llmq_M3"}, llmq_type, work_block_hash), modifier)) { + if (!m_evoDb.Read(std::make_tuple(DB_QUORUM_MODIFIER, llmq_type, work_block_hash), modifier)) {Note that
DB_QUORUM_SNAPSHOTlives in an anonymous namespace at file scope, so move the new constant to the same scope or to thellmqnamespace as appropriate.🤖 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 334 - 349, Define a file-scope constant for the "llmq_M3" modifier key prefix alongside DB_QUORUM_SNAPSHOT, then update SeedQuorumModifier and GetSeededQuorumModifier to use that shared constant for both database keys. Ensure the existing read/write key structure remains unchanged.src/evo/snapshot.cpp (1)
37-74: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize the canonical comparators.
The ordering rules for
CMinedQuorumCommitment,CQuorumSnapshotEntry,CHistoricalMNListDiff,CQuorumModifier, andCQuorumSnapshotDatanow exist in three places:Sorted,IsStrictlySorted, and the sort lambdas inCQuorumSnapshotData::SerializeandCEvoSnapshot::Serializeinsrc/evo/snapshot.h(lines 476-483 and 516-523). The three copies agree today. If one copy drifts, the writer will produce snapshots that fail their ownrequire_canonical_ordercheck on load.Define one comparator per type in
src/evo/snapshot.hand use it from all sort and adjacent-find sites.IsStrictlySortedcan then bestd::ranges::is_sortedplus a uniqueness check driven by the same comparator.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/evo/snapshot.cpp` around lines 37 - 74, Centralize the ordering rules by defining one canonical comparator per supported type in snapshot.h for CMinedQuorumCommitment, CQuorumSnapshotEntry, CHistoricalMNListDiff, CQuorumModifier, and CQuorumSnapshotData, then replace the duplicated lambdas in Sorted, IsStrictlySorted, CQuorumSnapshotData::Serialize, and CEvoSnapshot::Serialize with those comparators. Implement IsStrictlySorted using std::ranges::is_sorted plus a uniqueness check based on the same comparator, preserving each type’s existing ordering and duplicate behavior.src/test/evo_snapshot_tests.cpp (2)
626-634: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant
ErasebeforeWrite.Line 629 erases
modifier_keyand Line 630 immediately writes a new value for the same key in the same transaction. TheWritealone produces the intended state. Drop theErasecall to make the intent explicit.♻️ Proposed simplification
const auto modifier_key{std::make_tuple(std::string_view{"llmq_M3"}, plain.type, plain_work->GetBlockHash())}; - m_node.evodb->Erase(modifier_key); m_node.evodb->Write(modifier_key, H(254));🤖 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_snapshot_tests.cpp` around lines 626 - 634, Remove the redundant m_node.evodb->Erase(modifier_key) call in the transaction setup before m_node.evodb->Write(modifier_key, H(254)); retain the write and the subsequent SnapshotStateMismatchError assertion unchanged.
328-331: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the manual
DIP0003Heightrestore.
ConsensusParamsRestorerat Line 328 already restores the completeConsensus::Paramswhen the test case leaves scope, including after a failedBOOST_REQUIRE. Theold_dip3_heightcapture at Line 329 and the manual restore at Line 375 duplicate that guarantee and only restore on the success path. Remove both to keep one restore mechanism.Also applies to: 375-375
🤖 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_snapshot_tests.cpp` around lines 328 - 331, Remove the unused old_dip3_height capture near ConsensusParamsRestorer and remove the corresponding manual DIP0003Height restoration later in the test. Rely solely on ConsensusParamsRestorer’s scope-based restoration of the complete consensus parameters, including failure paths.test/functional/test_runner.py (1)
357-357: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the entry to a slower runtime section.
feature_assumeutxo_dash.pyadds three evo masternodes and mines four quorum cycles. Its runtime is minutes, not seconds. The current position is inside the "Tests less than 30s" section. The file header asks for the longest tests first so parallel scheduling stays efficient. Move the entry into the "Tests less than 5m" or "less than 2m" section.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/functional/test_runner.py` at line 357, Move the feature_assumeutxo_dash.py entry out of the “Tests less than 30s” section in the test runner’s test-list configuration and place it in the appropriate slower-runtime section, preferably “Tests less than 5m” or “less than 2m,” while preserving the header’s longest-tests-first ordering.src/test/evo_db_tests.cpp (1)
287-289: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrect the comment or add the restart it describes.
The comment states that promotion is idempotent "across a restart after the synced batch lands". The test uses an in-memory database (
.memory = trueat Line 268) and never reopens it, so no restart occurs. Either reword the comment to describe repeated calls within one process, or switch the fixture to an on-disk database and reopen it, assnapshot_markers_can_be_discardeddoes.🤖 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_db_tests.cpp` around lines 287 - 289, The comment above the repeated PromoteSnapshotMarkers calls does not match the in-memory, single-process test setup. Update the comment to describe idempotency across repeated calls in the same process, or modify the fixture and test flow to use a persistent database and reopen it before the second call, following snapshot_markers_can_be_discarded.src/streams.h (1)
541-563: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that
size()returns the remaining bytes.The name
size()suggests the total file size. The function returnsst_size - position, which is the number of bytes after the current read position. Callers that use the value as a total allocation bound would be wrong after any read.Add a short doc comment, or rename to
remaining().♻️ Proposed doc comment
+ //! Number of bytes between the current position and the end of the file. size_t size() 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/streams.h` around lines 541 - 563, Document the existing AutoFile::size() method as returning the number of bytes remaining from the current file position, not the total file size. Add a concise doc comment directly above size() and leave its behavior unchanged.src/test/validation_chainstatemanager_tests.cpp (1)
1100-1111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the
dmn_S3key instead of duplicating the literal.The test hard-codes the database key
dmn_S3, which is a file-local constant insrc/evo/deterministicmns.cpp. If that constant is renamed, this test silently seeds an unused key. The mismatch would then come only fromWriteSnapshotBaseMNListHash, so the test would still pass but would no longer cover the poisoned-list path.Expose the constant from a header, or add a compile-time reference so a rename breaks the build.
🤖 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 1100 - 1111, Update the test setup around the database write in the validation-chain-state-manager test to reuse the shared DB_LIST_SNAPSHOT key, or add a compile-time reference to that symbol instead of hard-coding "dmn_S3". Ensure renaming the production key causes the test to fail compilation rather than silently seeding an unused key.src/evo/snapshot_load.cpp (1)
368-377: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the inner
indexvariable to avoid shadowing.Line 371 declares
const CBlockIndex* indexinside the loop. It shadows theCBlockIndex* indexdeclared at Line 299 and used at Line 333. The two variables hold unrelated values. A rename such aswork_indexremoves the ambiguity.♻️ Proposed rename
for (const auto& block_hash : required_work_blocks) { - const CBlockIndex* index{m_blockman.LookupBlockIndex(block_hash)}; - assert(index != nullptr); - if (m_ibd_chainstate->m_chain.Contains(index)) { + const CBlockIndex* work_index{m_blockman.LookupBlockIndex(block_hash)}; + assert(work_index != nullptr); + if (m_ibd_chainstate->m_chain.Contains(work_index)) { existing_background_work_hashes.emplace( - block_hash, evo::CanonicalMNListHash(background_dmnman.GetListForBlock(index))); + block_hash, evo::CanonicalMNListHash(background_dmnman.GetListForBlock(work_index))); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/evo/snapshot_load.cpp` around lines 368 - 377, Rename the loop-local CBlockIndex pointer currently named index in the required_work_blocks iteration to work_index, and update its uses in the assert, chain membership check, and GetListForBlock call; leave the outer index variable unchanged.src/validation.h (1)
584-591: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew snapshot-lifecycle state and entry points lack
cs_mainthread-safety annotations. Both sites add code that runs undercs_mainin practice but is not annotated, so Clang's thread-safety analyzer cannot verify the callers. Neighbouring snapshot members such asm_disabledandm_cached_snapshot_baseare alreadyGUARDED_BY(::cs_main).
src/validation.h#L584-L591: addGUARDED_BY(::cs_main)tom_required_background_mn_list_hashes, and addEXCLUSIVE_LOCKS_REQUIRED(::cs_main)toRecordBackgroundMNListHashandSetRequiredBackgroundMNListHashes.src/validation.cpp#L5697-L5719: declareHandleSnapshotStateMismatchwithEXCLUSIVE_LOCKS_REQUIRED(::cs_main)insrc/validation.hand replace theLOCK(::cs_main)at Line 5714 withAssertLockHeld(::cs_main).🤖 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` around lines 584 - 591, Annotate the snapshot lifecycle state and entry points for cs_main safety: in src/validation.h:584-591, add GUARDED_BY(::cs_main) to m_required_background_mn_list_hashes and EXCLUSIVE_LOCKS_REQUIRED(::cs_main) to RecordBackgroundMNListHash and SetRequiredBackgroundMNListHashes; also declare HandleSnapshotStateMismatch with EXCLUSIVE_LOCKS_REQUIRED(::cs_main) there. In src/validation.cpp:5697-5719, replace the nested LOCK(::cs_main) with AssertLockHeld(::cs_main).
🤖 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 `@doc/design/assumeutxo.md`:
- Around line 6-8: Update the RPC availability statement in assumeutxo
documentation: state that dumptxoutset is included in this PR and emits the Evo
section, while identifying loadtxoutset as the RPC still awaiting merge or
deferred work. Keep the existing snapshot-generation and loading context and
utility-script reference.
In `@src/evo/snapshot_load.cpp`:
- Around line 176-207: Update the evo_marker read handling in the
snapshot-loading function to record when reading the marker throws, using a
dedicated flag set in the ios_base::failure catch block. Preserve the immediate
rejection for DIP3-active bases, and for pre-DIP3 bases reject the file as
truncated before the evo_marker validation can report a bad marker.
In `@src/evo/snapshot.cpp`:
- Around line 233-255: Refactor CEvoSnapshot::Validate and its callers to avoid
repeating ReconstructHistoricalMNLists and CanonicalMNListHash computation:
reconstruct the historical lists once during snapshot validation and pass or
reuse the resulting std::map<uint256, CDeterministicMNList> in Unserialize,
GetEvoSnapshotHash, ValidateEvoSnapshotAgainstChain, and VerifyEvoSnapshotCbTx.
Remove the redundant reconstruction in ValidateEvoSnapshotAgainstChain while
preserving all existing validation results and error handling.
In `@src/llmq/snapshot.cpp`:
- Around line 320-325: Update StoreSnapshotForBlock so a failed
m_evoDb.WriteDerived uses the established EvoDB inconsistency handling: call
AbortNode and throw EvoDbInconsistencyError instead of std::runtime_error. Match
the handling used by CMNHFManager::AddToCache and
CQuorumBlockProcessor::ProcessCommitment, preserving the existing cache update
on successful writes.
In `@src/llmq/utils.cpp`:
- Around line 761-775: Update GetAllQuorumMembers so snapshot-backed
reconstruction clears both process-static quorum-member caches before
GetAllQuorumMembersInternal can return cached entries, or equivalently forces
reset_cache=true. Ensure this applies to both rotating and non-rotating quorum
paths before GetHashModifier validation, while preserving the existing
SnapshotStateMismatchError handling.
In `@src/node/chainstate.cpp`:
- Around line 39-64: Define shared constants for the invalid and to-delete
snapshot suffixes alongside SNAPSHOT_CHAINSTATE_SUFFIX in utxo_snapshot.h.
Replace the hardcoded "_INVALID" and "_todelete" strings in
Chainstate::InvalidateCoinsDBOnDisk and
ChainstateManager::ValidatedSnapshotCleanup, then derive all corresponding
directory names in RemoveSnapshotChainstateArtifacts and RecoverSnapshotCleanup
from SNAPSHOT_CHAINSTATE_SUFFIX and the new constants.
In `@src/util/ranges_set.h`:
- Around line 101-116: Reject the full-domain Range{0, 0} during bounded
deserialization in the range validation logic at src/util/ranges_set.h:101-116,
preserving the existing Size() and Contains() contracts; update
src/util/ranges_set.cpp:84-86 to ensure wrapped arithmetic cannot report this
range as empty, and add an encoded {0, 0} regression test in
src/test/util_tests.cpp:1429-1493 that verifies deserialization rejects it.
In `@src/validation.cpp`:
- Around line 5748-5757: Update the snapshot-to-background mempool handoff in
HandleSnapshotStateMismatch so MempoolMutex() remains held across ownership
transfer and through the failed ActivateBestChainStep cleanup path. Acquire the
snapshot mempool mutex before assigning m_snapshot_chainstate->m_mempool to
m_ibd_chainstate, or defer the transfer until ActivateBestChain unwinds;
preserve the existing chainstate shutdown assertions.
---
Outside diff comments:
In `@src/evo/evodb.cpp`:
- Around line 152-160: Update CEvoDB::EraseSnapshotMarkers to erase every
snapshot lifecycle marker, including EVODB_REQUIRED_WORK_MNLISTS, all per-block
EVODB_BACKGROUND_WORK_MNLIST_HASH entries, and EVODB_SNAPSHOT_EVO_SECTION,
alongside the existing markers. Use the established key/prefix range deletion
mechanism for the per-block entries.
---
Nitpick comments:
In `@src/evo/snapshot_load.cpp`:
- Around line 368-377: Rename the loop-local CBlockIndex pointer currently named
index in the required_work_blocks iteration to work_index, and update its uses
in the assert, chain membership check, and GetListForBlock call; leave the outer
index variable unchanged.
In `@src/evo/snapshot.cpp`:
- Around line 37-74: Centralize the ordering rules by defining one canonical
comparator per supported type in snapshot.h for CMinedQuorumCommitment,
CQuorumSnapshotEntry, CHistoricalMNListDiff, CQuorumModifier, and
CQuorumSnapshotData, then replace the duplicated lambdas in Sorted,
IsStrictlySorted, CQuorumSnapshotData::Serialize, and CEvoSnapshot::Serialize
with those comparators. Implement IsStrictlySorted using std::ranges::is_sorted
plus a uniqueness check based on the same comparator, preserving each type’s
existing ordering and duplicate behavior.
In `@src/llmq/blockprocessor.cpp`:
- Around line 703-712: Update SeedMinedCommitment to call DropQcHashesCache()
after completing the commitment-seeding writes and before returning. Add the
required !m_qc_hashes_cache_mutex declaration to the SeedMinedCommitment
declaration in the class header, matching the cache-invalidation synchronization
requirements.
In `@src/llmq/snapshot.cpp`:
- Around line 334-349: Define a file-scope constant for the "llmq_M3" modifier
key prefix alongside DB_QUORUM_SNAPSHOT, then update SeedQuorumModifier and
GetSeededQuorumModifier to use that shared constant for both database keys.
Ensure the existing read/write key structure remains unchanged.
In `@src/streams.h`:
- Around line 541-563: Document the existing AutoFile::size() method as
returning the number of bytes remaining from the current file position, not the
total file size. Add a concise doc comment directly above size() and leave its
behavior unchanged.
In `@src/test/evo_db_tests.cpp`:
- Around line 287-289: The comment above the repeated PromoteSnapshotMarkers
calls does not match the in-memory, single-process test setup. Update the
comment to describe idempotency across repeated calls in the same process, or
modify the fixture and test flow to use a persistent database and reopen it
before the second call, following snapshot_markers_can_be_discarded.
In `@src/test/evo_snapshot_tests.cpp`:
- Around line 626-634: Remove the redundant m_node.evodb->Erase(modifier_key)
call in the transaction setup before m_node.evodb->Write(modifier_key, H(254));
retain the write and the subsequent SnapshotStateMismatchError assertion
unchanged.
- Around line 328-331: Remove the unused old_dip3_height capture near
ConsensusParamsRestorer and remove the corresponding manual DIP0003Height
restoration later in the test. Rely solely on ConsensusParamsRestorer’s
scope-based restoration of the complete consensus parameters, including failure
paths.
In `@src/test/validation_chainstatemanager_tests.cpp`:
- Around line 1100-1111: Update the test setup around the database write in the
validation-chain-state-manager test to reuse the shared DB_LIST_SNAPSHOT key, or
add a compile-time reference to that symbol instead of hard-coding "dmn_S3".
Ensure renaming the production key causes the test to fail compilation rather
than silently seeding an unused key.
In `@src/validation.h`:
- Around line 584-591: Annotate the snapshot lifecycle state and entry points
for cs_main safety: in src/validation.h:584-591, add GUARDED_BY(::cs_main) to
m_required_background_mn_list_hashes and EXCLUSIVE_LOCKS_REQUIRED(::cs_main) to
RecordBackgroundMNListHash and SetRequiredBackgroundMNListHashes; also declare
HandleSnapshotStateMismatch with EXCLUSIVE_LOCKS_REQUIRED(::cs_main) there. In
src/validation.cpp:5697-5719, replace the nested LOCK(::cs_main) with
AssertLockHeld(::cs_main).
In `@test/functional/test_runner.py`:
- Line 357: Move the feature_assumeutxo_dash.py entry out of the “Tests less
than 30s” section in the test runner’s test-list configuration and place it in
the appropriate slower-runtime section, preferably “Tests less than 5m” or “less
than 2m,” while preserving the header’s longest-tests-first ordering.
🪄 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: 2586346a-3020-498e-9796-fc883f8043e5
📒 Files selected for processing (58)
doc/design/assumeutxo.mdsrc/Makefile.amsrc/Makefile.test.includesrc/bench/load_external.cppsrc/chain.hsrc/chainparams.cppsrc/chainparams.hsrc/evo/chainhelper.cppsrc/evo/chainhelper.hsrc/evo/creditpool.cppsrc/evo/creditpool.hsrc/evo/deterministicmns.cppsrc/evo/deterministicmns.hsrc/evo/evodb.cppsrc/evo/evodb.hsrc/evo/mnhftx.cppsrc/evo/mnhftx.hsrc/evo/smldiff.cppsrc/evo/snapshot.cppsrc/evo/snapshot.hsrc/evo/snapshot_load.cppsrc/evo/snapshot_types.hsrc/evo/specialtxman.cppsrc/init.cppsrc/llmq/blockprocessor.cppsrc/llmq/blockprocessor.hsrc/llmq/snapshot.cppsrc/llmq/snapshot.hsrc/llmq/utils.cppsrc/llmq/utils.hsrc/node/blockstorage.cppsrc/node/blockstorage.hsrc/node/chainstate.cppsrc/node/chainstate.hsrc/node/utxo_snapshot.cppsrc/rpc/blockchain.cppsrc/streams.hsrc/test/blockmanager_tests.cppsrc/test/coinstatsindex_tests.cppsrc/test/evo_db_tests.cppsrc/test/evo_netinfo_tests.cppsrc/test/evo_snapshot_tests.cppsrc/test/flatfile_tests.cppsrc/test/fuzz/load_external_block_file.cppsrc/test/util/chainstate.hsrc/test/util/setup_common.cppsrc/test/util_tests.cppsrc/test/validation_block_tests.cppsrc/test/validation_chainstate_tests.cppsrc/test/validation_chainstatemanager_tests.cppsrc/util/ranges_set.cppsrc/util/ranges_set.hsrc/validation.cppsrc/validation.htest/functional/feature_assumeutxo_dash.pytest/functional/feature_reindex.pytest/functional/rpc_dumptxoutset.pytest/functional/test_runner.py
| The RPC commands `dumptxoutset` and `loadtxoutset` (yet to be merged) are used to | ||
| respectively generate and load UTXO snapshots. The utility script | ||
| `./contrib/devtools/utxo_snapshot.sh` may be of use. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the RPC availability statement.
dumptxoutset is part of this PR and emits the Evo section. The current text says that it is not merged. State the current dumptxoutset status separately from the deferred loadtxoutset work.
Proposed documentation update
-The RPC commands `dumptxoutset` and `loadtxoutset` (yet to be merged) are used to
-respectively generate and load UTXO snapshots. The utility script
+The `dumptxoutset` RPC generates UTXO snapshots. `loadtxoutset` remains
+disabled until M5. The utility script
`./contrib/devtools/utxo_snapshot.sh` may be of use.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| The RPC commands `dumptxoutset` and `loadtxoutset` (yet to be merged) are used to | |
| respectively generate and load UTXO snapshots. The utility script | |
| `./contrib/devtools/utxo_snapshot.sh` may be of use. | |
| The `dumptxoutset` RPC generates UTXO snapshots. `loadtxoutset` remains | |
| disabled until M5. The utility script | |
| `./contrib/devtools/utxo_snapshot.sh` may be of use. |
🤖 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 `@doc/design/assumeutxo.md` around lines 6 - 8, Update the RPC availability
statement in assumeutxo documentation: state that dumptxoutset is included in
this PR and emits the Evo section, while identifying loadtxoutset as the RPC
still awaiting merge or deferred work. Keep the existing snapshot-generation and
loading context and utility-script reference.
| void CEvoSnapshot::Validate(bool require_canonical_order) const | ||
| { | ||
| if (version != EVO_SNAPSHOT_VERSION) throw std::ios_base::failure("unsupported evo snapshot version"); | ||
| if (base_block_hash.IsNull() || mn_list.GetBlockHash() != base_block_hash) { | ||
| throw std::ios_base::failure("evo snapshot base block mismatch"); | ||
| } | ||
| ValidateCanonicalMNInvariants(mn_list); | ||
| if (quorums.size() > Consensus::available_llmqs.size() || | ||
| historical_mn_list_diffs.size() > EvoSnapshotMaxHistoricalMNLists() || | ||
| quorum_modifiers.size() > EVO_SNAPSHOT_MAX_MODIFIERS || | ||
| mnhf_signals.size() > Consensus::MAX_VERSION_BITS_DEPLOYMENTS) { | ||
| throw std::ios_base::failure("oversized evo snapshot collection"); | ||
| } | ||
| if (require_canonical_order && (!IsStrictlySorted(quorums) || !IsStrictlySorted(historical_mn_list_diffs) || | ||
| !IsStrictlySorted(quorum_modifiers))) { | ||
| throw std::ios_base::failure("noncanonical evo snapshot top-level order"); | ||
| } | ||
|
|
||
| std::map<uint256, CDeterministicMNList> reconstructed; | ||
| std::string reconstruction_error; | ||
| if (!ReconstructHistoricalMNLists(*this, reconstructed, reconstruction_error)) { | ||
| throw std::ios_base::failure(reconstruction_error); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Reduce the repeated historical reconstruction on the load path.
Validate() calls ReconstructHistoricalMNLists, which replays every diff and computes CanonicalMNListHash for each resulting list. Validate() runs from CEvoSnapshot::Unserialize (src/evo/snapshot.h line 577), from GetEvoSnapshotHash (line 321), from ValidateEvoSnapshotAgainstChain (line 504), and from VerifyEvoSnapshotCbTx (line 666). ValidateEvoSnapshotAgainstChain then reconstructs the lists a second time at line 522.
A single snapshot load therefore replays and re-hashes the whole history about five times. Each replay is bounded by EVO_SNAPSHOT_MAX_MNS per list multiplied by EvoSnapshotMaxHistoricalMNLists(), and the input is attacker-supplied.
Reconstruct once and pass the resulting std::map<uint256, CDeterministicMNList> to the callers that need it, or memoize it on the snapshot object.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/evo/snapshot.cpp` around lines 233 - 255, Refactor CEvoSnapshot::Validate
and its callers to avoid repeating ReconstructHistoricalMNLists and
CanonicalMNListHash computation: reconstruct the historical lists once during
snapshot validation and pass or reuse the resulting std::map<uint256,
CDeterministicMNList> in Unserialize, GetEvoSnapshotHash,
ValidateEvoSnapshotAgainstChain, and VerifyEvoSnapshotCbTx. Remove the redundant
reconstruction in ValidateEvoSnapshotAgainstChain while preserving all existing
validation results and error handling.
| QuorumMembers GetAllQuorumMembers(Consensus::LLMQType llmqType, const UtilParameters& util_params, bool reset_cache) | ||
| { | ||
| try { | ||
| return GetAllQuorumMembersInternal(llmqType, util_params, reset_cache); | ||
| } catch (const evo::SnapshotStateMismatchError& e) { | ||
| // Outside block connection there is no EvoDB transaction to unwind, so | ||
| // P2P, RPC, DKG, and quorum-manager callers can immediately enter the | ||
| // controlled invalid-snapshot path. During block connect/disconnect, | ||
| // defer to the Chainstate boundary after its transaction rolls back. | ||
| if (const_cast<ChainstateManager&>(util_params.m_chainman).HandleSnapshotStateMismatch(e.what())) { | ||
| return {}; | ||
| } | ||
| throw; | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the quorum-member implementation before inspecting call sites.
ast-grep outline src/llmq/utils.cpp --items all
# Find all callers and determine whether snapshot activation forces cache reset.
rg -n --type=cpp -C 5 '\bGetAllQuorumMembers\s*\(' src test
rg -n --type=cpp -C 5 'reset_cache|mapQuorumMembers|mapIndexedQuorumMembers|ActivateSnapshot|SnapshotStateMismatch' src testRepository: dashpay/dash
Length of output: 47673
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- quorum cache implementation ---'
sed -n '100,145p' src/llmq/utils.cpp
sed -n '662,760p' src/llmq/utils.cpp
printf '%s\n' '--- snapshot activation ---'
sed -n '5550,5725p' src/validation.cpp
printf '%s\n' '--- snapshot and chainstate cache invalidation symbols ---'
rg -n --type=cpp -C 4 \
'InitQuorumsCache|mapQuorumMembers|mapIndexedQuorumMembers|Invalidate.*Cache|Clear.*Cache|ActivateSnapshot|m_active_chainstate' \
src/llmq src/evo src/validation.cpp src/validation.h
printf '%s\n' '--- snapshot quorum reconstruction tests and setup ---'
sed -n '470,645p' src/test/evo_snapshot_tests.cppRepository: dashpay/dash
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Evo snapshot loading around cache invalidation ---'
sed -n '430,495p' src/evo/snapshot_load.cpp
sed -n '520,610p' src/evo_snapshot.cpp 2>/dev/null || true
printf '%s\n' '--- every production GetAllQuorumMembers call and reset argument ---'
rg -n --type=cpp \
'GetAllQuorumMembers\s*\(' src \
| grep -v '^src/test/'
rg -n --type=cpp -C 2 \
'GetAllQuorumMembers\s*\([^;]*,\s*true\s*\)' src \
| grep -v '^src/test/' || true
printf '%s\n' '--- block processing paths after snapshot activation ---'
rg -n --type=cpp -C 6 \
'PreComputeQuorumMembers|ConnectBlock|ActivateSnapshot|LoadChainTip|PopulateAndValidateSnapshot' \
src/llmq/blockprocessor.cpp src/evo/snapshot_load.cpp src/validation.cppRepository: dashpay/dash
Length of output: 44182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Evo snapshot loading around cache invalidation ---'
sed -n '430,495p' src/evo/snapshot_load.cpp
printf '%s\n' '--- production GetAllQuorumMembers calls with reset=true ---'
rg -n --type=cpp -C 2 \
'GetAllQuorumMembers\s*\([^;]*,\s*true\s*\)' src \
| grep -v '^src/test/' || true
printf '%s\n' '--- block processing paths after snapshot activation ---'
rg -n --type=cpp -C 6 \
'PreComputeQuorumMembers|ActivateSnapshot|PopulateAndValidateSnapshot' \
src/llmq/blockprocessor.cpp src/evo/snapshot_load.cpp src/validation.cppRepository: dashpay/dash
Length of output: 11583
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
utils = Path("src/llmq/utils.cpp").read_text()
activation = Path("src/validation.cpp").read_text()
loader = Path("src/evo/snapshot_load.cpp").read_text()
blockprocessor = Path("src/llmq/blockprocessor.cpp").read_text()
start = utils.index("static QuorumMembers GetAllQuorumMembersInternal")
end = utils.index("QuorumMembers GetAllQuorumMembers(", start)
body = utils[start:end]
checks = {
"member cache uses block-hash keys":
"Uint256LruHashMap<QuorumMembers>" in body,
"rotating cache uses cycle-hash/index keys":
"unordered_lru_cache<std::pair<uint256, int>" in body,
"member cache returns before modifier validation":
body.index("mapQuorumMembers[llmqType].get") < body.index("GetHashModifier("),
"rotating cache returns before modifier validation":
body.index("mapIndexedQuorumMembers[llmqType].get") < body.index("GetHashModifier("),
"activation does not call quorum-member cache reset":
"GetAllQuorumMembers" not in activation[
activation.index("bool ChainstateManager::ActivateSnapshot"):
activation.index("bool ChainstateManager::HandleSnapshotStateMismatch")
],
"snapshot loader clears only subordinate caches":
"InvalidateListCacheForBlock(base_blockhash)" in loader
and "InvalidateSnapshotCacheForBlock(" in loader,
"block connection uses reset_cache=false":
"PreComputeQuorumMembers(m_dmnman, m_qsnapman, m_chainman, pindex, /*reset_cache=*/false)"
in blockprocessor,
"undo uses reset_cache=true":
"PreComputeQuorumMembers(m_dmnman, m_qsnapman, m_chainman, pindex, /*reset_cache=*/true)"
in blockprocessor,
}
for name, result in checks.items():
print(("PASS" if result else "FAIL") + ": " + name)
if not all(checks.values()):
raise SystemExit(1)
PYRepository: dashpay/dash
Length of output: 537
Reset quorum-member caches when activating an Evo snapshot.
GetAllQuorumMembersInternal() can return cached members before GetHashModifier() validates seeded modifiers. The process-static caches are keyed by block hashes, not chainstate or EvoDB identity. Clear both caches, or force reset_cache=true, before snapshot-backed reconstruction for rotating and non-rotating quorums.
🤖 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/utils.cpp` around lines 761 - 775, Update GetAllQuorumMembers so
snapshot-backed reconstruction clears both process-static quorum-member caches
before GetAllQuorumMembersInternal can return cached entries, or equivalently
forces reset_cache=true. Ensure this applies to both rotating and non-rotating
quorum paths before GetHashModifier validation, while preserving the existing
SnapshotStateMismatchError handling.
| const bool wrapped_max{range.end == 0}; | ||
| if (!wrapped_max && range.begin >= range.end) { | ||
| throw std::ios_base::failure("invalid empty CRangesSet range"); | ||
| } | ||
| // Equality is adjacent and must have been merged; less-than is | ||
| // overlapping or unordered. Both are noncanonical and could make | ||
| // Size() underflow. | ||
| if (have_previous && (previous_end == 0 || range.begin <= previous_end)) { | ||
| throw std::ios_base::failure("noncanonical CRangesSet ranges"); | ||
| } | ||
| if (wrapped_max && i + 1 != count) { | ||
| throw std::ios_base::failure("wrapped CRangesSet range must be last"); | ||
| } | ||
| previous_end = range.end; | ||
| have_previous = true; | ||
| decoded.emplace(range); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject or explicitly support the full-domain wrapped range.
Range{0, 0} passes deserialization and represents [0, 2^64). Size() then evaluates UINT64_MAX - 0 + 1 as uint64_t and returns zero. Contains() returns true for every value. This breaks the Size() and Contains() contract.
src/util/ranges_set.h#L101-L116: rejectRange{0, 0}during bounded deserialization, or change the cardinality API to represent2^64.src/util/ranges_set.cpp#L84-L86: prevent the wrapped arithmetic from reporting the full-domain range as empty.src/test/util_tests.cpp#L1429-L1493: add an encoded{0, 0}case that verifies the selected behavior.
As per coding guidelines, “For consensus, serialization ... changes, prefer small tests that prove the invariant being changed.”
📍 Affects 3 files
src/util/ranges_set.h#L101-L116(this comment)src/util/ranges_set.cpp#L84-L86src/test/util_tests.cpp#L1429-L1493
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/util/ranges_set.h` around lines 101 - 116, Reject the full-domain
Range{0, 0} during bounded deserialization in the range validation logic at
src/util/ranges_set.h:101-116, preserving the existing Size() and Contains()
contracts; update src/util/ranges_set.cpp:84-86 to ensure wrapped arithmetic
cannot report this range as empty, and add an encoded {0, 0} regression test in
src/test/util_tests.cpp:1429-1493 that verifies deserialization rejects it.
Source: Coding guidelines
| m_active_chainstate = m_ibd_chainstate.get(); | ||
| // The active snapshot owns the mempool. Hand it back before disabling the | ||
| // snapshot so the restored background chainstate remains internally | ||
| // consistent for the rest of the shutdown path. | ||
| m_ibd_chainstate->m_mempool = m_snapshot_chainstate->m_mempool; | ||
| m_snapshot_chainstate->m_mempool = nullptr; | ||
| m_snapshot_chainstate->m_disabled = true; | ||
| ReleaseSnapshotPruneLock(); | ||
| assert(!IsUsable(m_snapshot_chainstate.get())); | ||
| assert(IsUsable(m_ibd_chainstate.get())); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect MaybeUpdateMempoolForReorg and every call reachable after a failed background ConnectTip.
set -euo pipefail
ast-grep run --pattern 'void Chainstate::MaybeUpdateMempoolForReorg($$$) { $$$ }' --lang cpp src/validation.cpp
rg -nP -C 8 '\bMaybeUpdateMempoolForReorg\s*\(' src/validation.cpp
rg -nP -C 6 '\bMempoolMutex\s*\(' src/validation.cpp src/validation.hRepository: dashpay/dash
Length of output: 150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- validation.cpp locations ---'
rg -n -C 12 'HandleSnapshotStateMismatch|MaybeUpdateMempoolForReorg|ActivateBestChainStep|MempoolMutex|m_mempool' src/validation.cpp src/validation.h | head -n 1200
printf '%s\n' '--- relevant source ranges ---'
sed -n '3000,3420p' src/validation.cpp
sed -n '5680,5785p' src/validation.cpp
printf '%s\n' '--- declarations and call sites ---'
rg -n -C 8 'MaybeUpdateMempoolForReorg|HandleSnapshotStateMismatch|ActivateBestChainStep' src --glob '*.{cpp,h}'Repository: dashpay/dash
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- ConnectTip and ActivateBestChainStep ---'
sed -n '3025,3105p' src/validation.cpp
sed -n '3215,3305p' src/validation.cpp
sed -n '3360,3475p' src/validation.cpp
printf '%s\n' '--- snapshot mismatch implementation and callers ---'
rg -n -C 30 'HandleSnapshotStateMismatch|MaybeCompleteSnapshotValidation|ReleaseSnapshotPruneLock|m_active_chainstate|m_ibd_chainstate|m_snapshot_chainstate' src/validation.cpp | tail -n 500
printf '%s\n' '--- lock macro definitions and nullable-lock behavior ---'
rg -n -C 12 '`#define` LOCK|class?UniqueLock|UniqueLock|MempoolMutex\(\)' src sync | head -n 500Repository: dashpay/dash
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- lock headers and macros ---'
git ls-files | rg '(^|/)(sync|lock|thread).*\.(h|cpp)$|lock'
rg -n -C 15 '`#define` (LOCK|LOCK2|WITH_LOCK)|MakeUniqueLock|UniqueLock<|class UniqueLock|AssertLockHeld' src --glob '*.{h,cpp}' | head -n 1000
printf '%s\n' '--- tests for mismatch and shutdown handoff ---'
rg -n -C 20 'HandleSnapshotStateMismatch|SnapshotStateMismatch|invalid assumeutxo|m_mempool.*nullptr|GetMempool\(\)' src/test --glob '*.{cpp,h}' | head -n 1200
printf '%s\n' '--- exact declarations and construction paths ---'
sed -n '430,510p' src/validation.h
sed -n '1070,1100p' src/validation.h
rg -n -C 15 'MakeChainstate|m_ibd_chainstate\s*=|m_ibd_chainstate\)|new Chainstate|Chainstate\(' src/validation.cpp src/*.cpp src/*.h | head -n 1000Repository: dashpay/dash
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '175,325p' src/sync.h
printf '%s\n' '--- mismatch-specific tests ---'
rg -n -C 30 'HandleSnapshotStateMismatch|SnapshotStateMismatch|invalid assumeutxo|evo state mismatch' src/test --glob '*.{cpp,h}'
printf '%s\n' '--- snapshot completion handoff and mempool use ---'
sed -n '517,620p' src/evo/snapshot_load.cpp
rg -n -C 12 'm_mempool|MempoolMutex|MaybeUpdateMempoolForReorg' src/evo/snapshot_load.cpp src/test/validation_chainstatemanager_tests.cppRepository: dashpay/dash
Length of output: 19783
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
validation = Path("src/validation.cpp").read_text()
sync = Path("src/sync.h").read_text()
header = Path("src/validation.h").read_text()
def section(text, start, end):
a = text.index(start)
b = text.index(end, a)
return text[a:b]
activate = section(validation, "bool Chainstate::ActivateBestChainStep", "static SynchronizationState")
mismatch = section(validation, "bool ChainstateManager::HandleSnapshotStateMismatch", "Chainstate& ChainstateManager::ActiveChainstate")
mempool_mutex = section(header, "RecursiveMutex* MempoolMutex()", "private:")
assert "LOCK(MempoolMutex());" in section(validation, "bool Chainstate::ActivateBestChain", "bool Chainstate::PreciousBlock")
assert "if (!pmutexIn) return;" in sync
assert "m_ibd_chainstate->m_mempool = m_snapshot_chainstate->m_mempool;" in mismatch
assert "m_snapshot_chainstate->m_mempool = nullptr;" in mismatch
assert "MaybeUpdateMempoolForReorg(disconnectpool, false);" in activate
handoff = mismatch.index("m_ibd_chainstate->m_mempool =")
failure_cleanup = activate.index("MaybeUpdateMempoolForReorg(disconnectpool, false);")
assert handoff >= 0 and failure_cleanup >= 0
print("MempoolMutex() returns m_mempool ? &m_mempool->cs : nullptr:", "m_mempool ? &m_mempool->cs : nullptr" in mempool_mutex)
print("LOCK(nullptr) constructs without owning a lock:", "if (!pmutexIn) return;" in sync)
print("Mismatch hands off mempool before failed-step cleanup:", True)
print("ActivateBestChainStep performs failed-step cleanup after ConnectTip:", True)
PYRepository: dashpay/dash
Length of output: 403
Keep the mempool mutex held across the mempool handoff. LOCK(MempoolMutex()) is a no-op while the background chainstate has m_mempool == nullptr. After HandleSnapshotStateMismatch assigns the snapshot mempool to m_ibd_chainstate, the failed ActivateBestChainStep path calls MaybeUpdateMempoolForReorg without holding that mempool's mutex. Defer the handoff until ActivateBestChain unwinds, or acquire the snapshot mempool mutex before transferring ownership.
🤖 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 5748 - 5757, Update the
snapshot-to-background mempool handoff in HandleSnapshotStateMismatch so
MempoolMutex() remains held across ownership transfer and through the failed
ActivateBestChainStep cleanup path. Acquire the snapshot mempool mutex before
assigning m_snapshot_chainstate->m_mempool to m_ibd_chainstate, or defer the
transfer until ActivateBestChain unwinds; preserve the existing chainstate
shutdown assertions.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The snapshot format has strong structural bounds and focused tests, but five in-scope correctness issues remain: rejected snapshot data survives in shared EvoDB, stale quorum-member caches can bypass snapshot-backed reconstruction, quorum-snapshot write conflicts can become consensus rejections, the full-domain range encoding is accepted with contradictory cardinality, and the mismatch path transfers the mempool without preserving its lock. The repeated historical-list reconstruction, inaccurate RPC documentation, upstream snapshot-base cache bypass, and fixup-heavy commit series are lower-severity follow-ups.
Source: reviewer backends codex-general=gpt-5.6-sol, codex-dash-core-commit-history=gpt-5.6-sol, and codex-backport-reviewer=gpt-5.6-sol; final verifier backend codex-verifier=gpt-5.6-sol; CodeRabbit inline evidence was separately verified. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 5 blocking | 🟡 2 suggestion(s) | 💬 2 nitpick(s)
2 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/evo/evodb.cpp`:
- [BLOCKING] src/evo/evodb.cpp:261-277: Remove or isolate seeded evo state when discarding a snapshot
DiscardSnapshotMarkers() removes lifecycle metadata but not the snapshot-provided deterministic-MN snapshots, mined commitments and inverse-height indexes, rotation snapshots, quorum modifiers, credit-pool snapshot, or MNHF state committed under ordinary unprefixed EvoDB keys. EraseSnapshotMarkers() has the same problem if activation fails after seeding, such as when writing base_blockhash fails. After the invalid snapshot is quarantined and the process restarts, NORMAL reads can consume these records; CDeterministicMNManager::GetListForBlockInternal(), for example, checks DB_LIST_SNAPSHOT before reconstructing from NORMAL diffs. This defeats the promised fallback to independently validated state and can also make subsequent valid background derivations collide with rejected data. Snapshot-derived records need identity-isolated keys or a cleanup/promotion design that can atomically remove or replace every seeded record without deleting independently generated NORMAL state.
In `src/llmq/utils.cpp`:
- [BLOCKING] src/llmq/utils.cpp:681-687: Reset quorum-member caches when the active Evo state changes
The process-static rotating and non-rotating quorum-member caches are keyed only by block hashes, not by EvoDB identity or snapshot generation, and both return before GetHashModifier() validates a seeded modifier. Entries can have been computed before activation against the background state, including failed or incomplete reconstruction attempts for header-only quorum hashes, and then be reused after the snapshot seeds a different state. Clear both caches as part of snapshot activation and invalid-snapshot fallback, or include an Evo-state generation in their keys. Resetting only the deterministic-MN and quorum-snapshot subordinate caches does not invalidate these completed member lists.
In `src/llmq/snapshot.cpp`:
- [BLOCKING] src/llmq/snapshot.cpp:320-321: Do not translate quorum-snapshot DB conflicts into block invalidity
StoreSnapshotForBlock() throws a generic std::runtime_error when WriteDerived finds different bytes. This function runs while quorum rotation is reconstructed, and generic exception handlers in the block-processing path can translate the exception into a consensus-invalid block, unlike the EvoDbInconsistencyError handling used by the other derived-state writers. During a dual-chainstate run, the conflicting value can also be an untrusted snapshot-seeded rotation snapshot, in which case it should enter SnapshotStateMismatchError's controlled invalid-snapshot path rather than merely abort as generic local corruption. Distinguish that case and use EvoDbInconsistencyError plus AbortNode for ordinary local DB corruption; neither case may be reported as a consensus failure attributable to the block or its peer.
In `src/util/ranges_set.h`:
- [BLOCKING] src/util/ranges_set.h:101-116: Reject the unrepresentable full-domain range
The bounded decoder accepts Range{0, 0}. Under the wrapped half-open representation, Contains() then reports every uint64_t as present, while Size() evaluates UINT64_MAX - 0 + 1 in uint64_t and returns zero. A snapshot can therefore provide credit-pool indexes whose membership and cardinality disagree, and the seeded state can reject every later unlock despite appearing empty by Size(). Reject begin == 0 when end == 0, preserve wrapped ranges beginning above zero for sets containing UINT64_MAX, and add an encoded {0, 0} regression test.
In `src/validation.cpp`:
- [BLOCKING] src/validation.cpp:5748-5753: Preserve the mempool lock through invalid-snapshot cleanup
Background ActivateBestChain enters with LOCK(MempoolMutex()), but that lock is a no-op because the background chainstate has m_mempool == nullptr. HandleSnapshotStateMismatch() then transfers the active snapshot's mempool pointer to the background chainstate and returns to ActivateBestChainStep, whose failed ConnectTip path calls MaybeUpdateMempoolForReorg(). That function now operates on the transferred mempool without its mutex, violating its explicit lock precondition, triggering lock assertions in checked builds, and racing concurrent mempool users otherwise. Defer ownership transfer until ActivateBestChain has unwound, or restructure the failure path so the snapshot mempool mutex remains owned through MaybeUpdateMempoolForReorg and the rest of the failed-step cleanup.
- [NITPICK] src/validation.cpp:6024-6034: bitcoin#27746 snapshot-base cache is not used by production lookup
Upstream bitcoin#27746 routes ChainstateManager::GetSnapshotBaseBlock() through Chainstate::SnapshotBase(), making m_cached_snapshot_base effective. Dash originally bypassed that method because SnapshotBase asserted if the persisted base was absent during startup, but ef9f84ab087 changed SnapshotBase to return a nullable lookup and left this direct-lookup bypass and stale comment in place. Production callers therefore still repeat LookupBlockIndex(), while SnapshotBase is used only by tests. Route the manager lookup through the now-nullable cached method.
In `src/evo/snapshot.cpp`:
- [SUGGESTION] src/evo/snapshot.cpp:248-255: Reuse historical MN-list reconstruction during snapshot validation
CEvoSnapshot::Unserialize reconstructs and hashes the complete historical MN-list chain through Validate(). The activation path then repeats that work in ValidateEvoSnapshotAgainstChain, GetEvoSnapshotHash, VerifyEvoSnapshotCbTx, and the seeding step; ValidateEvoSnapshotAgainstChain itself reconstructs twice. Deferred completion repeats several of the same passes again. Each pass is bounded, but each historical list can contain up to 100,000 masternodes, so the constant-factor amplification is substantial for snapshot-controlled input. Produce a validated reconstruction context once per load/completion operation and pass its historical-list map to the chain, hash, CbTx, and seeding checks that need it, while retaining safe standalone wrappers for callers that do not have a context.
In `<commit-history>`:
- [SUGGESTION] <commit-history>:1: Fold M4 fixups into the commits that introduce their code
The retained feature revisions are not clean intermediate targets: 525599d fails circular-dependency lint, 3cba43b adds five more reported cycles, d217753 still leaves those five cycles, and only the final 08ac564 adaptation removes the remaining validation includes. Commits 75df67d and f324c1e likewise repair behavior and tests introduced by 525599d. Rebase these corrections into their introducing commits so each retained revision passes the dependency invariant and its new tests; split the independent AutoFile sizing work into a dedicated commit if it remains. Add durable rationale to the large consensus-sensitive feature commits and replace the opaque B6 subject with a lifecycle-focused description.
In `doc/design/assumeutxo.md`:
- [NITPICK] doc/design/assumeutxo.md:6-8: Correct the snapshot RPC availability statement
This PR extends the already available dumptxoutset RPC to emit the Evo section, while only loadtxoutset remains intentionally unexposed until M5. Saying both commands are yet to be merged contradicts the implementation and the PR description.
| bool CEvoDB::DiscardSnapshotMarkers() | ||
| { | ||
| LOCK(cs); | ||
| assert(!active_transaction.has_value()); | ||
| for (const auto& [_, context] : transaction_contexts) { | ||
| if (!context) continue; | ||
| assert(context->cur_transaction.IsClean()); | ||
| assert(context->root_transaction.IsClean()); | ||
| } | ||
|
|
||
| CDBBatch batch{*db}; | ||
| batch.Erase(std::make_pair(EVODB_BEST_BLOCK, uint8_t{1})); | ||
| batch.Erase(EVODB_SNAPSHOT_MNLIST_HASH); | ||
| batch.Erase(EVODB_BACKGROUND_MNLIST_HASH); | ||
| EraseHistoricalMNListMarkers(*db, batch); | ||
| batch.Erase(EVODB_SNAPSHOT_EVO_SECTION); | ||
| batch.Erase(EVODB_DUAL_CHAINSTATE); |
There was a problem hiding this comment.
🔴 Blocking: Remove or isolate seeded evo state when discarding a snapshot
DiscardSnapshotMarkers() removes lifecycle metadata but not the snapshot-provided deterministic-MN snapshots, mined commitments and inverse-height indexes, rotation snapshots, quorum modifiers, credit-pool snapshot, or MNHF state committed under ordinary unprefixed EvoDB keys. EraseSnapshotMarkers() has the same problem if activation fails after seeding, such as when writing base_blockhash fails. After the invalid snapshot is quarantined and the process restarts, NORMAL reads can consume these records; CDeterministicMNManager::GetListForBlockInternal(), for example, checks DB_LIST_SNAPSHOT before reconstructing from NORMAL diffs. This defeats the promised fallback to independently validated state and can also make subsequent valid background derivations collide with rejected data. Snapshot-derived records need identity-isolated keys or a cleanup/promotion design that can atomically remove or replace every seeded record without deleting independently generated NORMAL state.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Remove or isolate seeded evo state when discarding a snapshot no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| if (!m_evoDb.WriteDerived(std::make_pair(DB_QUORUM_SNAPSHOT, snapshotHash), snapshot)) { | ||
| throw std::runtime_error("EvoDB quorum snapshot payload mismatch"); |
There was a problem hiding this comment.
🔴 Blocking: Do not translate quorum-snapshot DB conflicts into block invalidity
StoreSnapshotForBlock() throws a generic std::runtime_error when WriteDerived finds different bytes. This function runs while quorum rotation is reconstructed, and generic exception handlers in the block-processing path can translate the exception into a consensus-invalid block, unlike the EvoDbInconsistencyError handling used by the other derived-state writers. During a dual-chainstate run, the conflicting value can also be an untrusted snapshot-seeded rotation snapshot, in which case it should enter SnapshotStateMismatchError's controlled invalid-snapshot path rather than merely abort as generic local corruption. Distinguish that case and use EvoDbInconsistencyError plus AbortNode for ordinary local DB corruption; neither case may be reported as a consensus failure attributable to the block or its peer.
source: ['coderabbit']
| const bool wrapped_max{range.end == 0}; | ||
| if (!wrapped_max && range.begin >= range.end) { | ||
| throw std::ios_base::failure("invalid empty CRangesSet range"); | ||
| } | ||
| // Equality is adjacent and must have been merged; less-than is | ||
| // overlapping or unordered. Both are noncanonical and could make | ||
| // Size() underflow. | ||
| if (have_previous && (previous_end == 0 || range.begin <= previous_end)) { | ||
| throw std::ios_base::failure("noncanonical CRangesSet ranges"); | ||
| } | ||
| if (wrapped_max && i + 1 != count) { | ||
| throw std::ios_base::failure("wrapped CRangesSet range must be last"); | ||
| } | ||
| previous_end = range.end; | ||
| have_previous = true; | ||
| decoded.emplace(range); |
There was a problem hiding this comment.
🔴 Blocking: Reject the unrepresentable full-domain range
The bounded decoder accepts Range{0, 0}. Under the wrapped half-open representation, Contains() then reports every uint64_t as present, while Size() evaluates UINT64_MAX - 0 + 1 in uint64_t and returns zero. A snapshot can therefore provide credit-pool indexes whose membership and cardinality disagree, and the seeded state can reject every later unlock despite appearing empty by Size(). Reject begin == 0 when end == 0, preserve wrapped ranges beginning above zero for sets containing UINT64_MAX, and add an encoded {0, 0} regression test.
source: ['coderabbit']
| m_active_chainstate = m_ibd_chainstate.get(); | ||
| // The active snapshot owns the mempool. Hand it back before disabling the | ||
| // snapshot so the restored background chainstate remains internally | ||
| // consistent for the rest of the shutdown path. | ||
| m_ibd_chainstate->m_mempool = m_snapshot_chainstate->m_mempool; | ||
| m_snapshot_chainstate->m_mempool = nullptr; |
There was a problem hiding this comment.
🔴 Blocking: Preserve the mempool lock through invalid-snapshot cleanup
Background ActivateBestChain enters with LOCK(MempoolMutex()), but that lock is a no-op because the background chainstate has m_mempool == nullptr. HandleSnapshotStateMismatch() then transfers the active snapshot's mempool pointer to the background chainstate and returns to ActivateBestChainStep, whose failed ConnectTip path calls MaybeUpdateMempoolForReorg(). That function now operates on the transferred mempool without its mutex, violating its explicit lock precondition, triggering lock assertions in checked builds, and racing concurrent mempool users otherwise. Defer ownership transfer until ActivateBestChain has unwound, or restructure the failure path so the snapshot mempool mutex remains owned through MaybeUpdateMempoolForReorg and the rest of the failed-step cleanup.
source: ['coderabbit']
| throw std::ios_base::failure("noncanonical evo snapshot top-level order"); | ||
| } | ||
|
|
||
| std::map<uint256, CDeterministicMNList> reconstructed; | ||
| std::string reconstruction_error; | ||
| if (!ReconstructHistoricalMNLists(*this, reconstructed, reconstruction_error)) { | ||
| throw std::ios_base::failure(reconstruction_error); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Reuse historical MN-list reconstruction during snapshot validation
CEvoSnapshot::Unserialize reconstructs and hashes the complete historical MN-list chain through Validate(). The activation path then repeats that work in ValidateEvoSnapshotAgainstChain, GetEvoSnapshotHash, VerifyEvoSnapshotCbTx, and the seeding step; ValidateEvoSnapshotAgainstChain itself reconstructs twice. Deferred completion repeats several of the same passes again. Each pass is bounded, but each historical list can contain up to 100,000 masternodes, so the constant-factor amplification is substantial for snapshot-controlled input. Produce a validated reconstruction context once per load/completion operation and pass its historical-list map to the chain, hash, CbTx, and seeding checks that need it, while retaining safe standalone wrappers for callers that do not have a context.
source: ['coderabbit']
| The RPC commands `dumptxoutset` and `loadtxoutset` (yet to be merged) are used to | ||
| respectively generate and load UTXO snapshots. The utility script | ||
| `./contrib/devtools/utxo_snapshot.sh` may be of use. |
There was a problem hiding this comment.
💬 Nitpick: Correct the snapshot RPC availability statement
This PR extends the already available dumptxoutset RPC to emit the Evo section, while only loadtxoutset remains intentionally unexposed until M5. Saying both commands are yet to be merged contradicts the implementation and the PR description.
| The RPC commands `dumptxoutset` and `loadtxoutset` (yet to be merged) are used to | |
| respectively generate and load UTXO snapshots. The utility script | |
| `./contrib/devtools/utxo_snapshot.sh` may be of use. | |
| The `dumptxoutset` RPC generates UTXO snapshots, including the Dash Evo section. | |
| `loadtxoutset` remains disabled until M5. The utility script | |
| `./contrib/devtools/utxo_snapshot.sh` may be of use. |
source: ['coderabbit']
| const CBlockIndex* ChainstateManager::GetSnapshotBaseBlock() const | ||
| { | ||
| // Deliberately bypass Chainstate::SnapshotBase(), which Asserts a missing | ||
| // base block out of existence: startup snapshot completion must be able to | ||
| // observe "base not in the block index" and fail with | ||
| // BASE_BLOCKHASH_MISMATCH instead of aborting the node. Callers that | ||
| // require existence Assert at the call site. | ||
| if (!m_active_chainstate || !m_active_chainstate->m_from_snapshot_blockhash) { | ||
| return nullptr; | ||
| } | ||
| return m_blockman.LookupBlockIndex(*m_active_chainstate->m_from_snapshot_blockhash); |
There was a problem hiding this comment.
💬 Nitpick: bitcoin#27746 snapshot-base cache is not used by production lookup
Upstream bitcoin#27746 routes ChainstateManager::GetSnapshotBaseBlock() through Chainstate::SnapshotBase(), making m_cached_snapshot_base effective. Dash originally bypassed that method because SnapshotBase asserted if the persisted base was absent during startup, but ef9f84a changed SnapshotBase to return a nullable lookup and left this direct-lookup bypass and stale comment in place. Production callers therefore still repeat LookupBlockIndex(), while SnapshotBase is used only by tests. Route the manager lookup through the now-nullable cached method.
| const CBlockIndex* ChainstateManager::GetSnapshotBaseBlock() const | |
| { | |
| // Deliberately bypass Chainstate::SnapshotBase(), which Asserts a missing | |
| // base block out of existence: startup snapshot completion must be able to | |
| // observe "base not in the block index" and fail with | |
| // BASE_BLOCKHASH_MISMATCH instead of aborting the node. Callers that | |
| // require existence Assert at the call site. | |
| if (!m_active_chainstate || !m_active_chainstate->m_from_snapshot_blockhash) { | |
| return nullptr; | |
| } | |
| return m_blockman.LookupBlockIndex(*m_active_chainstate->m_from_snapshot_blockhash); | |
| const CBlockIndex* ChainstateManager::GetSnapshotBaseBlock() const | |
| { | |
| return m_active_chainstate ? m_active_chainstate->SnapshotBase() : nullptr; | |
| } |
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — bitcoin#27746 snapshot-base cache is not used by production lookup no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
…abort GetSnapshotBaseBlock() bypassed Chainstate::SnapshotBase() so completion could observe a missing base and return BASE_BLOCKHASH_MISMATCH, but that branch was unreachable: LoadBlockIndex's candidate admission Asserts the base for the background chainstate before completion ever runs, so a missing base aborted the node anyway, and the bypass silently lost upstream's per-call caching (bitcoin d4a11ab). Restore the cached delegation, make SnapshotBase() non-asserting (synthetic unit fixtures activate a snapshot before its base is indexed), and detect the missing base explicitly in ChainstateManager::LoadBlockIndex() before any admission runs, failing with the standard reindex advice; -reindex already discards the snapshot chainstate and its EvoDB markers. Covered by a new test that wipes blocks/index under a persisted snapshot.
The crash-recovery state machine depends on every chainstate-directory rename and removal being followed by a DirectoryCommit of the parent, but the pattern was open-coded at six sites where the commit could silently be forgotten. Add RenameDurably/RemoveAllDurably next to DirectoryCommit and use them everywhere the snapshot lifecycle touches directories.
…idate admission The CheckBlockIndex doc comment referenced upstream's m_options.check_block_index, which Dash does not have; the gate is still the fCheckBlockIndex global. Also document why ResetBlockFailureFlags defers candidate admission to a pass over every usable chainstate instead of upstream's inline insert (review question in dashpay#7553).
… overlap EraseSnapshotMarkers' removal of the background MN-list hash key was untested, and the promote/discard overlap that RecoverSnapshotCleanup documents (invalid-rename crash with the SNAPSHOT marker at the background tip) relied on a comment alone. Pin both: the marker-rollback test now seeds and asserts the background key, and a new recovery test drives the overlap state through LoadVerifyActivateChainstate and asserts the promote branch lands in the same end state as a completed discard.
quorum_members_reconstruct_from_seeded_state_only forced DIP0003/v19/v20 active through const_cast on a TestChain100Setup chain that was mined with those forks inactive. The v20 quorum hash modifier then probed a pre-DIP3 coinbase for a chainlock and tripped GetTxPayload's payload-type assertion in --enable-debug builds. The abort longjmps past fixture teardown, so mocktime, gArgs, and worker threads leaked and poisoned every later case in the same test_dash process. Mine the fixture chain with the required activation heights already in force instead (new SnapshotActivationChainSetup with its own deterministic checkpoint), and restore const_cast-mutated consensus params through a scope guard so a failed assertion in one case can no longer leak mutated params into cases that run after it.
08ac564 to
aadbe61
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/util/fs_helpers.cpp (1)
164-174: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
RenameDurablycommits only the destination parent directory. The implementation and its documented contract both cover one directory, so a cross-directory rename is not fully durable.
src/util/fs_helpers.cpp#L164-L174: commit the source parent directory as well whensrc.parent_path()differs fromdest.parent_path().src/util/fs_helpers.h#L28-L35: update the comment to state that both parent directories are committed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/util/fs_helpers.cpp` around lines 164 - 174, The RenameDurably function currently commits only the destination directory; update it to also commit src.parent_path() when it differs from dest.parent_path(), while avoiding a duplicate commit for same-directory renames. Update the RenameDurably contract comment in src/util/fs_helpers.h lines 28-35 to state that both parent directories are committed.
🤖 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/test/evo_db_tests.cpp`:
- Around line 261-265: Update the background hash read assertion in the test
around ReadBackgroundMNListHash to use the BlockHash(40) key written earlier,
rather than the default-constructed hash left by the failed preceding reads.
Preserve the assertion that the lookup fails after erasure.
---
Nitpick comments:
In `@src/util/fs_helpers.cpp`:
- Around line 164-174: The RenameDurably function currently commits only the
destination directory; update it to also commit src.parent_path() when it
differs from dest.parent_path(), while avoiding a duplicate commit for
same-directory renames. Update the RenameDurably contract comment in
src/util/fs_helpers.h lines 28-35 to state that both parent directories are
committed.
🪄 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: a84c3392-e3c9-4844-b8ab-77936fc1dc8c
📒 Files selected for processing (9)
src/node/chainstate.cppsrc/test/evo_db_tests.cppsrc/test/util/setup_common.cppsrc/test/util/setup_common.hsrc/test/validation_chainstatemanager_tests.cppsrc/util/fs_helpers.cppsrc/util/fs_helpers.hsrc/validation.cppsrc/validation.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/node/chainstate.cpp
| uint256 hash; | ||
| uint256 hash2; | ||
| BOOST_CHECK(!reopened.ReadBestBlock(EvoDbIdentity::SNAPSHOT, hash)); | ||
| BOOST_CHECK(!reopened.ReadSnapshotBaseMNListHash(hash)); | ||
| BOOST_CHECK(!reopened.ReadBackgroundMNListHash(hash, hash2)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Read the background MN-list hash with the key that was written.
Line 242 writes the background hash under key BlockHash(40). Line 265 reads with hash, which is still the default-constructed uint256 because the two preceding reads failed. The check therefore queries a key that was never written and passes without testing the erasure.
💚 Proposed fix
uint256 hash;
uint256 hash2;
BOOST_CHECK(!reopened.ReadBestBlock(EvoDbIdentity::SNAPSHOT, hash));
BOOST_CHECK(!reopened.ReadSnapshotBaseMNListHash(hash));
- BOOST_CHECK(!reopened.ReadBackgroundMNListHash(hash, hash2));
+ BOOST_CHECK(!reopened.ReadBackgroundMNListHash(BlockHash(40), hash2));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint256 hash; | |
| uint256 hash2; | |
| BOOST_CHECK(!reopened.ReadBestBlock(EvoDbIdentity::SNAPSHOT, hash)); | |
| BOOST_CHECK(!reopened.ReadSnapshotBaseMNListHash(hash)); | |
| BOOST_CHECK(!reopened.ReadBackgroundMNListHash(hash, hash2)); | |
| uint256 hash; | |
| uint256 hash2; | |
| BOOST_CHECK(!reopened.ReadBestBlock(EvoDbIdentity::SNAPSHOT, hash)); | |
| BOOST_CHECK(!reopened.ReadSnapshotBaseMNListHash(hash)); | |
| BOOST_CHECK(!reopened.ReadBackgroundMNListHash(BlockHash(40), hash2)); |
🤖 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_db_tests.cpp` around lines 261 - 265, Update the background hash
read assertion in the test around ReadBackgroundMNListHash to use the
BlockHash(40) key written earlier, rather than the default-constructed hash left
by the failed preceding reads. Preserve the assertion that the lookup fails
after erasure.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aadbe61597
ℹ️ 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".
| if (const auto seeded{snapshot_manager->GetSeededQuorumModifier(llmq_params.type, work_index->GetBlockHash())}) { | ||
| if (WITH_LOCK(::cs_main, return (work_index->nStatus & BLOCK_HAVE_DATA) == 0;)) return *seeded; | ||
| const uint256 recomputed{CalculateHashModifier(llmq_params, consensus_params, cycle_index)}; | ||
| if (recomputed != *seeded) throw evo::SnapshotStateMismatchError("seeded quorum score modifier mismatch"); |
There was a problem hiding this comment.
When a seeded modifier disagrees after its work-block data becomes available during ConnectTip, this exception is thrown inside CSpecialTxProcessor::ProcessSpecialTxsInBlock, whose catch (const std::exception&) in src/evo/specialtxman.cpp converts it into BLOCK_CONSENSUS. Consequently, the outer snapshot-mismatch catch in ConnectTip never runs, and the node marks the honest connecting block failed instead of quarantining the invalid snapshot; explicitly rethrow this exception type through the special-transaction boundary.
AGENTS.md reference: AGENTS.md:L169-L169
Useful? React with 👍 / 👎.
| if ((work_index->nStatus & BLOCK_HAVE_DATA) != 0 && | ||
| seeded->second != llmq::utils::GetQuorumHashModifier(*params, consensus, quorum_index)) return false; |
There was a problem hiding this comment.
On a pruned snapshot node, the required work blocks lie below the snapshot base, while the new assumeutxo prune lock protects only the base. If those historical blocks are pruned after background connection but before completion, this condition silently skips recomputing their modifiers and returns success, so a wrong snapshot-provided modifier can survive background validation and determine incorrect LLMQ membership. Capture independently derived modifiers while the background chain connects these blocks, or include every required work block in the pruning protection.
AGENTS.md reference: AGENTS.md:L176-L177
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The snapshot format is structurally well bounded, but seven blocking correctness issues remain around shared seeded EvoDB state, stale quorum caches, mismatch propagation, range decoding, mempool locking, pruned modifier verification, and full credit-pool validation. The historical reconstruction and commit-series suggestions plus the documentation nitpick also remain; one lower-priority test nitpick was omitted by the 10-comment budget.
Source: reviewers codex-general=gpt-5.6-sol, codex-dash-core-commit-history=gpt-5.6-sol, and codex-backport-reviewer=gpt-5.6-sol; final verifier codex-verifier=gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed),gpt-5.6-sol— backport-reviewer (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 7 blocking | 🟡 1 suggestion(s)
2 additional finding(s) omitted (not in diff).
6 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/llmq/utils.cpp`:
- [BLOCKING] src/llmq/utils.cpp:662-727: Reset quorum-member caches when the active Evo state changes
The process-static rotating and non-rotating quorum-member caches are keyed only by block hashes, not by EvoDB identity or snapshot generation. Both cache-hit paths return before GetHashModifier() can validate a seeded modifier, so a member list computed against one chainstate can be reused by the other chainstate or after snapshot activation. Clearing only the deterministic-MN and rotation-snapshot subordinate caches does not invalidate these completed member lists. Include an Evo-state generation in the cache keys or clear both caches whenever snapshot state is activated, rejected, or promoted.
In `src/evo/snapshot.cpp`:
- [BLOCKING] src/evo/snapshot.cpp:624-631: Retain work-block data until seeded modifiers are verified
The completion check accepts each seeded modifier without comparison when its work block no longer has BLOCK_HAVE_DATA. The assumeutxo prune lock protects only blocks near the base through the ten-block buffer, while the required historical quorum work blocks include prior cycles far below that boundary. Those blocks can be automatically pruned after background connection but before completion; stale quorum-member cache hits can also bypass the earlier on-connect modifier check. Capture independently calculated modifiers while their work blocks are connected, or extend pruning protection to every required work block and fail completion whenever an independent comparison cannot be performed.
- [SUGGESTION] src/evo/snapshot.cpp:248-255: Reuse historical MN-list reconstruction during snapshot validation
(existing thread: https://github.com/dashpay/dash/pull/7579#discussion_r3760006407)
CEvoSnapshot::Unserialize reconstructs and hashes the complete historical MN-list chain through Validate(). Activation repeats that work in ValidateEvoSnapshotAgainstChain(), GetEvoSnapshotHash(), VerifyEvoSnapshotCbTx(), and seeding, and deferred completion performs several more passes. ValidateEvoSnapshotAgainstChain() also reconstructs again immediately after Validate(). Although each pass is bounded, each historical list can contain up to 100,000 masternodes, making this substantial repeated work on snapshot-controlled input. Build one validated reconstruction context per load or completion operation and pass its historical-list map to the hash, chain, CbTx, and seeding checks.
In `src/evo/snapshot_load.cpp`:
- [BLOCKING] src/evo/snapshot_load.cpp:651-667: Independently validate the complete credit-pool state
Snapshot completion validates only credit_pool.locked through the base CbTx balance. It never compares currentLimit, latelyUnlocked, or indexes with state independently derived by the background chainstate. Background block connection stops at the base after calculating only the base block's diff; it does not materialize GetCreditPool(base), and a later lookup would read the snapshot-seeded DB_CREDITPOOL_SNAPSHOT record before reconstructing anything. A snapshot can therefore pass completion with incorrect limits or unlock indexes and affect post-snapshot asset-unlock validation. Independently construct or capture the full background CCreditPool at the base and compare its canonical serialization before promotion.
In `<commit-history>`:
- [SUGGESTION] <commit-history>:1: Fold M4 fixups into the commits that introduce their code
The retained M4 series still depends on later commits to repair earlier feature revisions: 823da7e exists specifically to break dependencies introduced by 52c8f63 and 8698365, 789356a corrects snapshot-aware modifier handling added earlier in M4, and ba7e217 repairs the new snapshot tests. The final aadbe61 adaptation also combines M3 integration with independent bounded AutoFile sizing. Fold these corrections into the commits that introduce the affected code so retained revisions satisfy dependency and test invariants, and split unrelated stream-sizing work into a dedicated commit.
In `src/evo/evodb.cpp`:
- [BLOCKING] src/evo/evodb.cpp:261-281: Remove or isolate seeded evo state when discarding a snapshot
(existing thread: https://github.com/dashpay/dash/pull/7579#discussion_r3760006375)
DiscardSnapshotMarkers() removes lifecycle metadata but leaves the snapshot-provided deterministic-MN snapshots, mined commitments and inverse-height indexes, rotation snapshots, quorum modifiers, credit-pool snapshot, and MNHF state in the shared persistent key namespace. EvoDbIdentity separates pending transaction batches, not on-disk keys, so after quarantine and restart NORMAL reads can consume rejected records; GetListForBlockInternal(), for example, reads DB_LIST_SNAPSHOT before reconstructing from NORMAL diffs. The same shared records also interfere before completion: a poisoned safety commitment can make ProcessCommitment reject an honest block as bad-qc-dup, while differing MNHF state makes AddToCache abort instead of quarantining the snapshot. Isolate snapshot-derived keys by identity or implement atomic cleanup/promotion that covers every seeded record, and route conflicts with seeded state through SnapshotStateMismatchError rather than block invalidity or a restart loop.
In `src/llmq/snapshot.cpp`:
- [BLOCKING] src/llmq/snapshot.cpp:316-325: Do not translate quorum-snapshot DB conflicts into block invalidity
(existing thread: https://github.com/dashpay/dash/pull/7579#discussion_r3760006385)
StoreSnapshotForBlock() throws a generic std::runtime_error when WriteDerived finds different bytes. Rotation reconstruction can execute this during block processing, where generic special-transaction catches convert the exception into TX_CONSENSUS or BLOCK_CONSENSUS. The same boundaries also swallow SnapshotStateMismatchError raised when GetHashModifier() disproves a seeded modifier, preventing ConnectTip's quarantine handler from running. Distinguish conflicts with snapshot-seeded state from ordinary local corruption, throw SnapshotStateMismatchError for the former, use EvoDbInconsistencyError plus AbortNode for the latter, and explicitly rethrow SnapshotStateMismatchError through both special-transaction exception boundaries. Neither case should invalidate the connecting block or punish its peer.
In `src/util/ranges_set.h`:
- [BLOCKING] src/util/ranges_set.h:99-116: Reject the unrepresentable full-domain range
(existing thread: https://github.com/dashpay/dash/pull/7579#discussion_r3760006391)
UnserializeBounded() accepts Range{0, 0}. In the wrapped half-open representation, Contains() then reports every uint64_t value as present, while Size() computes UINT64_MAX - 0 + 1 in uint64_t and returns zero. Snapshot-controlled credit-pool indexes can therefore have contradictory membership and cardinality, causing every subsequent unlock index to appear duplicated while the set reports size zero. Reject begin == 0 when end == 0, preserve wrapped ranges beginning above zero, and add an encoded {0, 0} regression test.
In `src/validation.cpp`:
- [BLOCKING] src/validation.cpp:5772-5781: Preserve the mempool lock through invalid-snapshot cleanup
(existing thread: https://github.com/dashpay/dash/pull/7579#discussion_r3760006399)
Background ActivateBestChain enters with LOCK(MempoolMutex()), but that lock is a no-op because the background chainstate has no mempool. HandleSnapshotStateMismatch() then transfers the active snapshot's mempool to the background chainstate while control is still inside that activation. A failed ConnectTip path subsequently calls MaybeUpdateMempoolForReorg(), and a completion-time mismatch can reach the final mempool check, without holding the transferred mempool's mutex. This violates explicit lock preconditions and can trigger checked-lock assertions or races. Defer the ownership transfer until ActivateBestChain unwinds, or preserve the snapshot mempool mutex through the transfer and all remaining activation cleanup.
In `doc/design/assumeutxo.md`:
- [NITPICK] doc/design/assumeutxo.md:6-8: Correct the snapshot RPC availability statement
(existing thread: https://github.com/dashpay/dash/pull/7579#discussion_r3760006415)
This PR extends the already available dumptxoutset RPC to emit the Evo section, while only loadtxoutset remains intentionally unexposed until M5. Saying that both commands are yet to be merged contradicts the implementation and the PR description.
| const auto check_modifier = [&](const uint256& quorum_hash, const uint256& work_hash) EXCLUSIVE_LOCKS_REQUIRED(::cs_main) { | ||
| const auto seeded{seeded_modifiers.find(std::make_pair(data.llmq_type, work_hash))}; | ||
| const CBlockIndex* quorum_index{chainman.m_blockman.LookupBlockIndex(quorum_hash)}; | ||
| const CBlockIndex* work_index{chainman.m_blockman.LookupBlockIndex(work_hash)}; | ||
| if (seeded == seeded_modifiers.end() || quorum_index == nullptr || work_index == nullptr) return false; | ||
| if ((work_index->nStatus & BLOCK_HAVE_DATA) != 0 && | ||
| seeded->second != llmq::utils::GetQuorumHashModifier(*params, consensus, quorum_index)) return false; | ||
| return true; |
There was a problem hiding this comment.
🔴 Blocking: Retain work-block data until seeded modifiers are verified
The completion check accepts each seeded modifier without comparison when its work block no longer has BLOCK_HAVE_DATA. The assumeutxo prune lock protects only blocks near the base through the ten-block buffer, while the required historical quorum work blocks include prior cycles far below that boundary. Those blocks can be automatically pruned after background connection but before completion; stale quorum-member cache hits can also bypass the earlier on-connect modifier check. Capture independently calculated modifiers while their work blocks are connected, or extend pruning protection to every required work block and fail completion whenever an independent comparison cannot be performed.
source: ['codex']
| const auto cbtx{GetTxPayload<CCbTx>(*base_block.vtx[0])}; | ||
| std::map<uint256, CDeterministicMNList> reconstructed_history; | ||
| bool history_matches{evo::ReconstructHistoricalMNLists(retained_snapshot, reconstructed_history, evo_error)}; | ||
| if (history_matches) { | ||
| for (const auto& [block_hash, reconstructed_list] : reconstructed_history) { | ||
| uint256 background_hash; | ||
| if (!m_ibd_chainstate->m_evoDb.ReadBackgroundWorkMNListHash(block_hash, background_hash) || | ||
| background_hash != evo::CanonicalMNListHash(reconstructed_list)) { | ||
| evo_error = "missing or mismatched background historical MN-list capture"; | ||
| history_matches = false; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| if (!history_matches || | ||
| !evo::ValidateEvoSnapshotAgainstChain(retained_snapshot, *this, &index_new, evo_error) || | ||
| !cbtx || !evo::VerifyEvoSnapshotCbTx(retained_snapshot, *cbtx, evo_error)) { |
There was a problem hiding this comment.
🔴 Blocking: Independently validate the complete credit-pool state
Snapshot completion validates only credit_pool.locked through the base CbTx balance. It never compares currentLimit, latelyUnlocked, or indexes with state independently derived by the background chainstate. Background block connection stops at the base after calculating only the base block's diff; it does not materialize GetCreditPool(base), and a later lookup would read the snapshot-seeded DB_CREDITPOOL_SNAPSHOT record before reconstructing anything. A snapshot can therefore pass completion with incorrect limits or unlock indexes and affect post-snapshot asset-unlock validation. Independently construct or capture the full background CCreditPool at the base and compare its canonical serialization before promotion.
source: ['codex']
3 participants
Issue being fixed or feature implemented
Dash's AssumeUTXO implementation cannot stop at the upstream UTXO-only snapshot format. At a DIP3-active base, a node also needs deterministic masternode, LLMQ/rotation, credit-pool, and MNHF state that can be reconstructed immediately and independently checked when background validation reaches the base.
This is M4 of the AssumeUTXO series and is stacked on M3, #7553. Until #7553 merges, GitHub's
developcomparison includes both M3 and M4 because the M3 head lives in the contributor fork. After M3 merges, this PR will contain only the seven M4 commits.loadtxoutsetremains intentionally unexposed; the runtime activation and rebinding work belongs to M5.What was done?
dumptxoutsetto emit the evo section and snapshot activation to validate and seed the state under the snapshot EvoDB identity.evo/snapshot_load.cppand remove the new circular dependencies from validation.How Has This Been Tested?
make -j13make check -j13./src/test/test_dash --run_test=evo_snapshot_tests,flatfile_tests,validation_chainstatemanager_tests./src/test/test_dash --run_test=validation_chainstate_teststest/functional/test_runner.py -j3 feature_assumeutxo_dash.py rpc_dumptxoutset.py feature_reindex.pytest/functional/test_runner.py feature_dip3_deterministicmns.pytest/lint/lint-circular-dependencies.pytest/lint/lint-whitespace.pyThe aggregate lint runner still reports repository-wide pre-existing spelling and cppcheck findings unrelated to this stack; the targeted M4 linters above pass.
Breaking Changes
The snapshot evo section uses a new, versioned v3 format. AssumeUTXO has not yet been released in Dash, and the user-facing load RPC remains disabled until M5.
Checklist:
This pull request was created by Codex.