feat: add evodb verify and repair RPC commands #6969
Conversation
Add overload that takes an explicit starting CDeterministicMNList instead of loading from GetListForBlock. This allows rebuilding masternode lists from trusted snapshots without depending on potentially corrupted diffs in the database. Key changes: - Add new overload taking prevList parameter (specialtxman.h) - Refactor original method to delegate to new overload (specialtxman.cpp) - Add assert to verify prevList is either empty or matches previous block - Skip collateral validation when using dummy coins view (historical blocks) Co-authored-by: Claude (Anthropic AI) <[email protected]>
Add method to compare two masternode lists for equality while ignoring non-deterministic members (nTotalRegisteredCount, internalId). Non-deterministic members can differ between nodes due to different sync histories but don't affect consensus validity. This method is useful for verifying that masternode list states match after applying diffs. Key features: - Compares blockHash, nHeight, and mnUniquePropertyMap - Compares map sizes (but not nTotalRegisteredCount) - Iterates through all masternodes comparing deterministic fields - Uses SerializeHash for pdmnState to avoid enumerating all fields Co-authored-by: Claude (Anthropic AI) <[email protected]>
Add two new RPC commands for verifying and repairing corrupted evodb diff records between snapshots stored every 576 blocks. evodb verify (read-only): - Verifies that applying stored diffs between snapshots produces correct results - Reports verification errors without modifying database - Defaults to full range from DIP0003 activation to chain tip evodb repair (writes to database): - First runs verification on all snapshot pairs - For failed pairs, recalculates diffs from actual blockchain data - Writes repaired diffs to database using efficient batching (16MB chunks) - Clears both diff and list caches to prevent serving stale data - Only commits repairs if recalculation verification passes Key implementation details: - Uses BuildNewListFromBlock overload with dummy coins view to avoid UTXO lookups - Breaks circular dependency by rebuilding from trusted snapshots, not corrupted diffs - Handles missing initial snapshot at DIP0003 (creates empty list) - Treats any other missing snapshot as critical error requiring reindex - Logs progress every 100 snapshot pairs to avoid spam - Separate error reporting for verification vs repair phases New public types: - CDeterministicMNManager::RecalcDiffsResult - result struct with detailed stats - Forward declarations for ChainstateManager and CSpecialTxProcessor Co-authored-by: Claude (Anthropic AI) <[email protected]>
Add detailed documentation for the new evodb verify and repair RPC commands, covering: - Command syntax and parameters - Return value specifications - Verification and repair process details - Usage guidelines and troubleshooting - Performance and implementation notes 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Add release notes for new hidden RPC commands: - evodb verify: read-only verification of evodb diffs - evodb repair: repair corrupted diffs from blockchain data 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThis PR adds hidden RPCs Sequence Diagram(s)sequenceDiagram
participant RPC as RPC command
participant Impl as evodb_verify_or_repair_impl
participant DMNMgr as CDeterministicMNManager
participant Collector as CollectSnapshotBlocks
participant Verifier as VerifySnapshotPair
participant Repairer as RepairSnapshotPair
participant Writer as WriteRepairedDiffs
participant DB as EvoDB
RPC->>Impl: call evodb verify/repair(start?, stop?)
Impl->>DMNMgr: RecalculateAndRepairDiffs(start, stop, build_callback, repair)
DMNMgr->>Collector: CollectSnapshotBlocks(start, stop)
Collector-->>DMNMgr: snapshot block list
loop each snapshot pair
DMNMgr->>Verifier: VerifySnapshotPair(from, to, fromSnapshot, toSnapshot)
rect rgb(230,240,255)
Note over Verifier: apply stored diffs sequentially and compare to target
end
Verifier-->>DMNMgr: ok / errors
alt failure AND repair mode
DMNMgr->>Repairer: RepairSnapshotPair(..., build_callback)
rect rgb(230,255,230)
Note over Repairer: replay blocks, rebuild lists, recalc diffs, validate
end
Repairer-->>DMNMgr: recalculated diffs
end
end
alt repair mode and diffs exist
DMNMgr->>Writer: WriteRepairedDiffs(recalculated_diffs)
Writer->>DB: batch write diffs (flush on size limit)
Writer->>DB: clear mnListDiffsCache & mnListsCache
Writer-->>DMNMgr: write status
end
DMNMgr-->>Impl: RecalcDiffsResult (counts, errors)
Impl-->>RPC: JSON response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 0
🧹 Nitpick comments (4)
src/rpc/evo.cpp (1)
1763-1839: Don't holdcs_mainfor the entire verify/repair runThe outer
LOCK(::cs_main)keeps the global validation lock held whileRecalculateAndRepairDiffstraverses every snapshot pair, reads blocks, and potentially writes batches back to LevelDB. The docs you added spell out that these commands are intentionally long-running and I/O heavy, so on mainnet this will freeze block validation, mempool admission, and RPCs for minutes or hours. Please confinecs_mainto short critical sections (fetching the start/stop indices, etc.) and reacquire it only within the callback right before callingBuildNewListFromBlock. That keeps the chain view stable without stalling the whole node.- LOCK(::cs_main); - - const CBlockIndex* start_index; - const CBlockIndex* stop_index; - - // Default to DIP0003 activation height if startBlock not specified - if (request.params[0].isNull()) { - const auto& consensus_params = Params().GetConsensus(); - start_index = chainman.ActiveChain()[consensus_params.DIP0003Height]; - if (!start_index) { - throw JSONRPCError(RPC_INTERNAL_ERROR, "Cannot find DIP0003 activation block"); - } - } else { - uint256 start_block_hash = ParseBlock(request.params[0], chainman, "startBlock"); - start_index = chainman.m_blockman.LookupBlockIndex(start_block_hash); - if (!start_index) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Start block not found"); - } - } - ... - if (request.params[1].isNull()) { - stop_index = chainman.ActiveChain().Tip(); - if (!stop_index) { - throw JSONRPCError(RPC_INTERNAL_ERROR, "Cannot find chain tip"); - } - } else { - uint256 stop_block_hash = ParseBlock(request.params[1], chainman, "stopBlock"); - stop_index = chainman.m_blockman.LookupBlockIndex(stop_block_hash); - if (!stop_index) { - throw JSONRPCError(RPC_INVALID_PARAMETER, "Stop block not found"); - } - } + const CBlockIndex* start_index; + const CBlockIndex* stop_index; + { + LOCK(::cs_main); + const auto& consensus_params = Params().GetConsensus(); + start_index = request.params[0].isNull() + ? chainman.ActiveChain()[consensus_params.DIP0003Height] + : chainman.m_blockman.LookupBlockIndex(ParseBlock(request.params[0], chainman, "startBlock")); + if (!start_index) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Start block not found"); + } + + stop_index = request.params[1].isNull() + ? chainman.ActiveChain().Tip() + : chainman.m_blockman.LookupBlockIndex(ParseBlock(request.params[1], chainman, "stopBlock")); + if (!stop_index) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Stop block not found"); + } + } ... - auto build_list_func = [&chain_helper](const CBlock& block, gsl::not_null<const CBlockIndex*> pindexPrev, - const CDeterministicMNList& prevList, const CCoinsViewCache& view, - bool debugLogs, BlockValidationState& state, - CDeterministicMNList& mnListRet) NO_THREAD_SAFETY_ANALYSIS -> bool { - return chain_helper.special_tx->BuildNewListFromBlock(block, pindexPrev, prevList, view, debugLogs, state, mnListRet); - }; + auto build_list_func = [&chain_helper](const CBlock& block, gsl::not_null<const CBlockIndex*> pindexPrev, + const CDeterministicMNList& prevList, const CCoinsViewCache& view, + bool debugLogs, BlockValidationState& state, + CDeterministicMNList& mnListRet) -> bool { + LOCK(::cs_main); + return chain_helper.special_tx->BuildNewListFromBlock(block, pindexPrev, prevList, view, debugLogs, state, mnListRet); + };src/evo/deterministicmns.h (1)
441-479: Consider verifying mnInternalIdMap consistency.The method compares
mnInternalIdMap.size()(Line 456) but doesn't verify the actual internal ID mappings are consistent with the MN entries being compared. WhileinternalIdis non-deterministic across nodes, within a single list the mapping should be consistent withmnMap.If the goal is to detect corruption (rather than just cross-node comparison), consider verifying that for each MN in
mnMap, the corresponding entry inmnInternalIdMappoints back to the correctproTxHash.Example verification:
// After line 458, add: // Verify internal ID mappings are consistent with mnMap for (const auto& [proTxHash, dmn] : mnMap) { auto mapped_hash = mnInternalIdMap.find(dmn->GetInternalId()); if (!mapped_hash || *mapped_hash != proTxHash) { return false; } }If this is only for cross-node comparison where internal IDs may legitimately differ, the current implementation is acceptable.
src/evo/deterministicmns.cpp (2)
1595-1607: Clarify the double assignment of result.start_height.Lines 1590 and 1606 both assign to
result.start_height. While this appears intentional (the original value is used in the log message at line 1603-1604), the logic is confusing.Consider restructuring:
// Clamp start height to DIP0003 activation (no snapshots/diffs exist before this) if (start_index->nHeight < consensus_params.DIP0003Height) { const int original_start_height = start_index->nHeight; start_index = stop_index->GetAncestor(consensus_params.DIP0003Height); if (!start_index) { result.verification_errors.push_back(strprintf("Stop height %d is below DIP0003 activation height %d", stop_index->nHeight, consensus_params.DIP0003Height)); return result; } LogPrintf("CDeterministicMNManager::%s -- Clamped start height from %d to DIP0003 activation height %d\n", __func__, original_start_height, consensus_params.DIP0003Height); // Set result to reflect the clamped start height result.start_height = start_index->nHeight; } else { result.start_height = start_index->nHeight; }This makes the intent clearer and avoids the double assignment.
1689-1693: Consider lock holding duration during I/O operations.Line 1692 calls
WriteRepairedDiffswhile holding thecslock. This method performs batched database writes (16MB batches, potentially multiple flush operations at lines 1903 and 1911 inWriteRepairedDiffs). Holdingcsduring I/O could block other operations that need access to masternode data.Consider whether the cache clearing (lines 1917-1920 in
WriteRepairedDiffs) is the only operation that truly requirescs, and if the database writes could be performed without it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
doc/evodb-verify-repair.md(1 hunks)doc/release-notes-6969.md(1 hunks)src/evo/deterministicmns.cpp(2 hunks)src/evo/deterministicmns.h(3 hunks)src/evo/specialtxman.cpp(2 hunks)src/evo/specialtxman.h(1 hunks)src/rpc/evo.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
doc/**
📄 CodeRabbit inference engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/release-notes-6969.mddoc/evodb-verify-repair.md
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/evo/deterministicmns.hsrc/evo/deterministicmns.cppsrc/evo/specialtxman.cppsrc/evo/specialtxman.hsrc/rpc/evo.cpp
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/deterministicmns.hsrc/evo/deterministicmns.cppsrc/evo/specialtxman.cppsrc/evo/specialtxman.h
🧠 Learnings (7)
📓 Common learnings
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.
📚 Learning: 2025-01-07T18:50:44.838Z
Learnt from: knst
Repo: dashpay/dash PR: 6511
File: src/evo/deterministicmns.cpp:1369-1373
Timestamp: 2025-01-07T18:50:44.838Z
Learning: The functions `MigrateDBIfNeeded` and `MigrateDBIfNeeded2` in `src/evo/deterministicmns.cpp` are temporary and will be removed in a future version. Refactoring suggestions for these functions should be avoided.
Applied to files:
src/evo/deterministicmns.hsrc/evo/deterministicmns.cpp
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
Applied to files:
src/evo/deterministicmns.hsrc/evo/deterministicmns.cpp
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/evo/specialtx.h : Special transactions use payload extensions defined in src/evo/specialtx.h
Applied to files:
src/evo/specialtxman.cppsrc/evo/specialtxman.h
📚 Learning: 2025-08-19T14:57:31.801Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Applied to files:
src/evo/specialtxman.cppsrc/evo/specialtxman.h
📚 Learning: 2025-07-09T15:02:26.899Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6729
File: src/evo/deterministicmns.cpp:1313-1316
Timestamp: 2025-07-09T15:02:26.899Z
Learning: In Dash's masternode transaction validation, `IsVersionChangeValid()` is only called by transaction types that update existing masternode entries (like `ProUpServTx`, `ProUpRegTx`, `ProUpRevTx`), not by `ProRegTx` which creates new entries. This means validation logic in `IsVersionChangeValid()` only applies to the subset of transaction types that actually call it, not all masternode transaction types.
Applied to files:
src/evo/specialtxman.cpp
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/rpc/evo.cpp
🧬 Code graph analysis (5)
src/evo/deterministicmns.h (3)
src/validation.cpp (1)
ChainstateManager(6051-6061)src/evo/specialtxman.h (2)
CSpecialTxProcessor(38-92)Consensus(28-28)src/evo/deterministicmns.cpp (12)
RecalculateAndRepairDiffs(1583-1696)RecalculateAndRepairDiffs(1583-1585)GetListForBlockInternal(773-865)GetListForBlockInternal(773-773)CollectSnapshotBlocks(1698-1748)CollectSnapshotBlocks(1698-1699)VerifySnapshotPair(1750-1801)VerifySnapshotPair(1750-1752)RepairSnapshotPair(1803-1877)RepairSnapshotPair(1803-1805)WriteRepairedDiffs(1879-1924)WriteRepairedDiffs(1879-1880)
src/evo/deterministicmns.cpp (3)
src/chainparams.cpp (2)
Params(1356-1359)Params(1356-1356)src/evo/evodb.h (1)
cs(65-69)src/evo/deterministicmns.h (2)
CDeterministicMNList(133-194)nHeight(148-181)
src/evo/specialtxman.cpp (1)
src/evo/deterministicmns.h (2)
CDeterministicMNList(133-194)nHeight(148-181)
src/evo/specialtxman.h (1)
src/evo/specialtxman.cpp (4)
BuildNewListFromBlock(174-181)BuildNewListFromBlock(174-176)BuildNewListFromBlock(183-511)BuildNewListFromBlock(183-186)
src/rpc/evo.cpp (1)
src/rpc/server_util.cpp (4)
EnsureAnyNodeContext(28-45)EnsureAnyNodeContext(28-28)EnsureChainman(87-93)EnsureChainman(87-87)
🪛 LanguageTool
doc/evodb-verify-repair.md
[style] ~84-~84: Consider placing the discourse marker ‘first’ at the beginning of the sentence for more clarity.
Context: ...ed ... ] } ``` Description: This command first runs verification on all snapshot pairs...
(SENT_START_FIRST_PREMIUM)
🪛 markdownlint-cli2 (0.18.1)
doc/evodb-verify-repair.md
3-3: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
8-8: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
57-57: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: mac-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (10)
src/evo/deterministicmns.h (5)
37-38: LGTM: Forward declarations reduce header coupling.The forward declarations for
ChainstateManagerandCSpecialTxProcessorappropriately reduce compilation dependencies.
723-731: LGTM: Result structure is well-designed.The
RecalcDiffsResultstruct appropriately captures all relevant outcomes including error details, making the API easy to use and debug.
733-741: LGTM: Function type alias is clear and well-defined.The
BuildListFromBlockFuncalias provides good abstraction for the callback pattern, making theRecalculateAndRepairDiffsmethod flexible and testable.
743-746: LGTM: Method declaration is correct.The
[[nodiscard]]attribute is appropriate since ignoring the result could miss critical errors. Lock annotations correctly specify that bothcsandcs_mainmust not be held by caller.
756-767: LGTM: Helper method declarations are well-structured.The private helper methods appropriately decompose the complex repair workflow. Lock requirements are clearly documented and consistent with the implementation pattern.
src/evo/deterministicmns.cpp (5)
28-28: LGTM: Required include for std::function.The
<functional>include is necessary for theBuildListFromBlockFunctype alias.
1698-1748: LGTM: Snapshot collection logic is correct.The implementation correctly handles:
- Walking backwards to find the starting snapshot
- Special case for irregular spacing at DIP0003 activation height
- Collecting all subsequent snapshots at regular intervals
The calculation at line 1728 correctly rounds up to the first regular snapshot period after DIP0003 activation.
1750-1801: LGTM: Verification logic is sound.The implementation correctly:
- Applies all stored diffs sequentially to build up from the source snapshot
- Uses
IsEqual(Line 1789) for comparison, appropriately ignoring non-deterministic fields- Includes exception handling for corrupt diffs
- Logs progress at reasonable intervals
1879-1924: LGTM: Batched write and cache clearing logic is correct.The implementation appropriately:
- Uses 16MB batches to manage memory usage (Line 1889)
- Flushes batches when threshold is reached
- Clears both
mnListDiffsCacheandmnListsCache(Lines 1918-1919), which is essential since cached lists were built from the old diffsThe cache clearing ensures subsequent reads fetch the repaired diffs from disk rather than stale cached data.
1829-1840: No issues found - the dummy coins view approach is properly designed and safe.The
GetCoin()calls inBuildNewListFromBlockare guarded byif (!view.GetBestBlock().IsNull())(line 255). A freshly created dummyCCoinsViewhas no best block, so the condition is false and collateral verification is safely skipped. This is intentional: historical blocks (processed during repair) were already validated and don't need UTXO re-verification. The code comments explicitly document this design.
| {"stopBlock", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The ending block height (defaults to current chain tip)."}, | ||
| }, | ||
| RPCResult{ | ||
| RPCResult::Type::OBJ, "", "", |
There was a problem hiding this comment.
nit: consider to create a common helper for both functions not for implementations only, but also to return RPCHelpMan; because they have big pieces of common code; and multiple duplicated strings
| } else { | ||
| // Any other missing snapshot is critical corruption beyond our repair capability | ||
| result.verification_errors.push_back(strprintf("CRITICAL: Snapshot missing at height %d. " | ||
| "This cannot be repaired by this tool - full reindex required.", from_index->nHeight)); | ||
| return result; | ||
| } | ||
| } | ||
|
|
||
| if (!has_to_snapshot) { | ||
| // Missing target snapshot is always critical - we cannot repair snapshots, only diffs | ||
| result.verification_errors.push_back(strprintf("CRITICAL: Snapshot missing at height %d. " | ||
| "This cannot be repaired by this tool - full reindex required.", to_index->nHeight)); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
nit: duplicated code and error message between different branches from/to;
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/evo/deterministicmns.h (1)
1-1: Fix formatting issues.The CI pipeline detected clang-format differences.
Run the following command to fix formatting:
./contrib/devtools/clang-format-diff.py -p1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/evo/deterministicmns.cpp(2 hunks)src/evo/deterministicmns.h(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/evo/deterministicmns.cpp
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/evo/deterministicmns.h
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/deterministicmns.h
🧠 Learnings (3)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
📚 Learning: 2025-01-07T18:50:44.838Z
Learnt from: knst
Repo: dashpay/dash PR: 6511
File: src/evo/deterministicmns.cpp:1369-1373
Timestamp: 2025-01-07T18:50:44.838Z
Learning: The functions `MigrateDBIfNeeded` and `MigrateDBIfNeeded2` in `src/evo/deterministicmns.cpp` are temporary and will be removed in a future version. Refactoring suggestions for these functions should be avoided.
Applied to files:
src/evo/deterministicmns.h
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
Applied to files:
src/evo/deterministicmns.h
🧬 Code graph analysis (1)
src/evo/deterministicmns.h (2)
src/evo/specialtxman.h (2)
CSpecialTxProcessor(38-92)Consensus(28-28)src/evo/deterministicmns.cpp (12)
RecalculateAndRepairDiffs(1583-1692)RecalculateAndRepairDiffs(1583-1585)GetListForBlockInternal(773-865)GetListForBlockInternal(773-773)CollectSnapshotBlocks(1694-1742)CollectSnapshotBlocks(1694-1695)VerifySnapshotPair(1744-1786)VerifySnapshotPair(1744-1746)RepairSnapshotPair(1788-1862)RepairSnapshotPair(1788-1790)WriteRepairedDiffs(1864-1910)WriteRepairedDiffs(1864-1865)
🪛 GitHub Actions: Clang Diff Format Check
src/evo/deterministicmns.h
[error] 1-1: Clang format differences found. Run './contrib/devtools/clang-format-diff.py -p1' to fix formatting in this file.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (4)
src/evo/deterministicmns.h (4)
37-38: LGTM!Forward declarations are appropriate for the new diff recalculation functionality.
441-479: LGTM!The
IsEqualmethod correctly implements deterministic comparison by excluding non-deterministic fields (nTotalRegisteredCount, internalId). UsingSerializeHashforpdmnStatecomparison (line 474) is a safe, deterministic approach that avoids enumerating all state fields, which is appropriate for this verification use case.
723-731: LGTM!Well-structured result type for aggregating verification and repair outcomes.
733-746: LGTM!The
BuildListFromBlockFunccallback type andRecalculateAndRepairDiffsmethod signature are well-designed. Usingstd::functionis appropriate for this infrequent operation, and the[[nodiscard]]attribute correctly enforces result checking.
UdjinM6
force-pushed
the
feat_evodb_verify_repair_rpc
branch
from
5a1b1e2 to
f3ee1d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/evo/deterministicmns.cpp(2 hunks)src/evo/deterministicmns.h(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/evo/deterministicmns.hsrc/evo/deterministicmns.cpp
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/deterministicmns.hsrc/evo/deterministicmns.cpp
🧠 Learnings (6)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
📚 Learning: 2025-01-07T18:50:44.838Z
Learnt from: knst
Repo: dashpay/dash PR: 6511
File: src/evo/deterministicmns.cpp:1369-1373
Timestamp: 2025-01-07T18:50:44.838Z
Learning: The functions `MigrateDBIfNeeded` and `MigrateDBIfNeeded2` in `src/evo/deterministicmns.cpp` are temporary and will be removed in a future version. Refactoring suggestions for these functions should be avoided.
Applied to files:
src/evo/deterministicmns.hsrc/evo/deterministicmns.cpp
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
Applied to files:
src/evo/deterministicmns.hsrc/evo/deterministicmns.cpp
📚 Learning: 2025-08-19T14:57:31.801Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Applied to files:
src/evo/deterministicmns.hsrc/evo/deterministicmns.cpp
📚 Learning: 2025-07-22T14:38:45.606Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6738
File: src/evo/smldiff.h:91-94
Timestamp: 2025-07-22T14:38:45.606Z
Learning: In the Dash codebase, EXCLUSIVE_LOCKS_REQUIRED annotations are Clang thread safety annotations that are enforced at compile time on supported platforms like macOS. If callers don't properly hold the required locks, the build will fail with compile-time errors, not runtime issues.
Applied to files:
src/evo/deterministicmns.h
📚 Learning: 2025-10-21T11:09:34.688Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6849
File: src/governance/governance.cpp:1339-1343
Timestamp: 2025-10-21T11:09:34.688Z
Learning: In the Dash Core codebase, `CacheMap` (defined in src/cachemap.h) is internally thread-safe and uses its own `mutable CCriticalSection cs` to protect access to its members. Methods like `GetSize()`, `Insert()`, `Get()`, `HasKey()`, etc., can be called without holding external locks.
Applied to files:
src/evo/deterministicmns.cpp
🧬 Code graph analysis (2)
src/evo/deterministicmns.h (3)
src/validation.cpp (1)
ChainstateManager(6051-6061)src/evo/specialtxman.h (2)
CSpecialTxProcessor(38-92)Consensus(28-28)src/node/chainstate.h (1)
Consensus(28-30)
src/evo/deterministicmns.cpp (3)
src/chainparams.cpp (2)
Params(1356-1359)Params(1356-1356)src/evo/deterministicmns.h (2)
CDeterministicMNList(133-194)nHeight(148-181)src/evo/evodb.h (1)
cs(65-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: arm-linux-build / Build source
…romBlock Rationale: - Short and clear - The "rebuild" prefix makes it obvious you're starting from an existing list - Distinguishes it well from BuildNewListFromBlock (which builds from database)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/rpc/evo.cpp (3)
1755-1816: Helper wiring into diff recalc/repair looks correct; be mindful of cs_main duration
evodb_verify_or_repair_implcorrectly:
- Resolves
start_index/stop_indexwith sane defaults (DIP0003 activation to tip) and enforcesstopHeight >= startHeight.- Holds
cs_mainwhile resolving chain indices and callingRecalculateAndRepairDiffs, which matches existing DMN/RPC patterns.- Wraps the new
CSpecialTxProcessor::RebuildListFromBlockviabuild_list_funcwith a matching signature andNO_THREAD_SAFETY_ANALYSIS, relying on the outerLOCK(::cs_main).Only operational note:
RecalculateAndRepairDiffscan span large ranges, so holdingcs_mainfor the whole call may block other chain operations on busy nodes. If that method internally breaks work into chunks wherecs_mainis not strictly required, consider relaxing the outer lock in the future (or acquiring it only around chain access inside the loop).If you’d like, I can sketch a follow-up that documents or narrows the locking expectations around
RecalculateAndRepairDiffsto avoid surprises if that code evolves.
1817-1841: Result marshalling matches the recalc/repair struct; consider future-proofing error payloadsThe conversion of
recalc_resultintoUniValueis straightforward and consistent with the described API:
- All scalar counters (
startHeight,stopHeight,diffsRecalculated,snapshotsVerified) are propagated.verification_errorsandrepair_errorsare emitted as simple string arrays, withrepairErrorsonly present in repair mode as documented.If you later decide to enrich error reporting (e.g., include block heights or snapshot indexes), this is a good place to switch from plain strings to small JSON objects without changing the top-level schema.
1843-1922: evodb verify/repair RPC contracts look coherent and aligned with the helperBoth
evodb_verifyandevodb_repair:
- Share consistent argument semantics (
startBlock,stopBlock, optional, clamped defaults) and call the common helper with the appropriaterepairflag.- Document result fields that match what
evodb_verify_or_repair_implreturns, including the absence ofrepairErrorsin verify-only mode and the meaning ofdiffsRecalculated.Nit-level only: the
RPCArg::Type::NUMforstartBlock/stopBlockmirrors existing uses ofParseBlockin this file, but the implementation does accept both heights and hashes. If you ever expose these commands (un-hide them), you might consider updating the arg type/help text to mention hashes explicitly for consistency withParseBlock’s error message.Please double-check that
RecalculateAndRepairDiffsindeed leavesdiffsRecalculated == 0whenrepair=false, matching the help text forevodb verify.src/evo/specialtxman.cpp (1)
183-196: RebuildListFromBlock precondition assertion is reasonable; small copy nit
RebuildListFromBlock’s assertion:assert(prevList == CDeterministicMNList() || prevList.GetBlockHash() == pindexPrev->GetBlockHash());is a good internal invariant: callers must either pass a default/empty list (initial state) or a list whose block hash matches the previous block. Given that
prevListis only constructed from snapshots orGetListForBlock, this should never be hit in normal operation and helps catch misuse early. This also aligns with the retrieved learning that deterministic MN list equality focuses on consensus-relevant fields, not non-deterministicinternalIdmappings. Based on learningsMinor nit: you can avoid one copy by skipping the
oldListtemporary and usingprevListdirectly as the source:- CDeterministicMNList oldList = prevList; - CDeterministicMNList newList = oldList; + CDeterministicMNList newList = prevList;since
oldListis only used to derivepayeeand to iterate MNs, both of which can be done fromprevList.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/evo/specialtxman.cpp(2 hunks)src/evo/specialtxman.h(1 hunks)src/rpc/evo.cpp(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/rpc/evo.cppsrc/evo/specialtxman.hsrc/evo/specialtxman.cpp
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/evo/specialtxman.hsrc/evo/specialtxman.cpp
🧠 Learnings (7)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6969
File: src/evo/deterministicmns.h:441-479
Timestamp: 2025-11-13T20:02:55.434Z
Learning: In `src/evo/deterministicmns.h`, the `internalId` field in `CDeterministicMN` and the `mnInternalIdMap` in `CDeterministicMNList` are non-deterministic and used only for internal bookkeeping and efficient lookups. Different nodes can assign different internalIds to the same masternode depending on their sync history. Methods like `IsEqual()` intentionally ignore internalId mappings and only compare consensus-critical deterministic fields (proTxHash, collateral, state, etc.).
📚 Learning: 2025-08-08T07:01:47.332Z
Learnt from: knst
Repo: dashpay/dash PR: 6805
File: src/wallet/rpc/wallet.cpp:357-357
Timestamp: 2025-08-08T07:01:47.332Z
Learning: In src/wallet/rpc/wallet.cpp, the upgradetohd RPC now returns a UniValue string message (RPCResult::Type::STR) instead of a boolean, including guidance about mnemonic backup and null-character passphrase handling; functional tests have been updated to assert returned strings in several cases.
Applied to files:
src/rpc/evo.cpp
📚 Learning: 2025-11-13T20:02:55.434Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6969
File: src/evo/deterministicmns.h:441-479
Timestamp: 2025-11-13T20:02:55.434Z
Learning: In `src/evo/deterministicmns.h`, the `internalId` field in `CDeterministicMN` and the `mnInternalIdMap` in `CDeterministicMNList` are non-deterministic and used only for internal bookkeeping and efficient lookups. Different nodes can assign different internalIds to the same masternode depending on their sync history. Methods like `IsEqual()` intentionally ignore internalId mappings and only compare consensus-critical deterministic fields (proTxHash, collateral, state, etc.).
Applied to files:
src/evo/specialtxman.hsrc/evo/specialtxman.cpp
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/evo/specialtx.h : Special transactions use payload extensions defined in src/evo/specialtx.h
Applied to files:
src/evo/specialtxman.hsrc/evo/specialtxman.cpp
📚 Learning: 2025-08-19T14:57:31.801Z
Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.
Applied to files:
src/evo/specialtxman.cpp
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
Applied to files:
src/evo/specialtxman.cpp
📚 Learning: 2025-07-09T15:02:26.899Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6729
File: src/evo/deterministicmns.cpp:1313-1316
Timestamp: 2025-07-09T15:02:26.899Z
Learning: In Dash's masternode transaction validation, `IsVersionChangeValid()` is only called by transaction types that update existing masternode entries (like `ProUpServTx`, `ProUpRegTx`, `ProUpRevTx`), not by `ProRegTx` which creates new entries. This means validation logic in `IsVersionChangeValid()` only applies to the subset of transaction types that actually call it, not all masternode transaction types.
Applied to files:
src/evo/specialtxman.cpp
🧬 Code graph analysis (3)
src/rpc/evo.cpp (1)
src/rpc/server_util.cpp (4)
EnsureAnyNodeContext(28-45)EnsureAnyNodeContext(28-28)EnsureChainman(87-93)EnsureChainman(87-87)
src/evo/specialtxman.h (1)
src/evo/specialtxman.cpp (2)
RebuildListFromBlock(183-511)RebuildListFromBlock(183-186)
src/evo/specialtxman.cpp (1)
src/evo/deterministicmns.h (2)
CDeterministicMNList(133-194)nHeight(148-181)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build container / Build container
🔇 Additional comments (4)
src/evo/specialtxman.h (1)
82-87: Header overload looks consistent and well-scopedThe new
RebuildListFromBlockoverload cleanly separates the “rebuild from explicit prev list” path from the DB-backedBuildNewListFromBlockand keeps the same locking contract oncs_main. Signature and parameter order match the cpp implementation and thebuild_list_funclambda inevodb_verify_or_repair_impl, so the public API surface here looks solid.src/rpc/evo.cpp (1)
2083-2093: Hidden RPC registration is correctly wiredRegistering
evodb_verifyandevodb_repairunder the"hidden"category matches the intent to keep these as expert-only maintenance commands while still making them callable viaevodb verify/repair. The placement alongside other evo RPCs inRegisterEvoRPCCommandsis consistent.src/evo/specialtxman.cpp (2)
174-181: BuildNewListFromBlock delegation keeps existing behavior while enabling reuseThe refactor to:
- fetch
oldListviam_dmnman.GetListForBlock(pindexPrev)and- delegate to
RebuildListFromBlockpreserves existing behavior for normal block processing while letting other callers (like evodb repair) inject a trusted
prevList. This avoids duplicating the list-update logic and keepsBuildNewListFromBlockas the canonical “from-chainstate” entry point.
252-263: Collateral check gating via view.GetBestBlock() – confirm view setup in evodb repair pathGuarding the external-collateral check with:
if (!view.GetBestBlock().IsNull()) { ... }matches the comment “complain about spent collaterals only when we process the tip” and avoids re-flagging already-validated ProRegTx collateral when working with historical or synthetic views (e.g., in evodb verify/repair). This is important to keep the rebuild/verification path from failing on legitimate collaterals that were later spent.
The correctness of this approach hinges on the callers:
- Using a non-null best block when validating at the tip in
ProcessSpecialTxsInBlock, and- Using a view with
GetBestBlock() == uint256()(or otherwise null-equivalent) when doing offline reconstruction inRecalculateAndRepairDiffs, so that collateral checks are skipped there.It would be good to explicitly ensure in
RecalculateAndRepairDiffsthat theCCoinsViewCachepassed tobuild_list_funchas its best block cleared before callingRebuildListFromBlock, to make this behavior robust against future changes in how the view is constructed. If you want, I can outline a small helper to create such a “validation-light” view.
There was a problem hiding this comment.
I just met real-case scenario to use build from this PR [testnet]:
> protx listdiff 1176911 1176913
....
pubKeyOperator": "000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
...
> evodb repair 1170000 1179999
{
"startHeight": 1170000,
"stopHeight": 1179999,
"diffsRecalculated": 576,
"snapshotsVerified": 16,
"verificationErrors": [
"Verification failed between snapshots at heights 1176768 and 1177344: Applied diffs do not match target snapshot"
],
"repairErrors": [
]
}
> protx listdiff 1176911 1176913
...
"pubKeyOperator": "8dfb2a7e5fc39cf220f1d5020e73a243ec9ba301a92af9b5a50d390471efc266341a168dd97b2312c5e92782b4432598"
...
Though, output of evodb repair is a bit messy; it doesn't show how much diffs have been actually fixed, instead it says "Verification failed between snapshots at heights 1176768 and 1177344: Applied diffs do not match target snapshot" and I assumed repair is failed. But actually it helped and fixed public key.
@UdjinM6 I think RPC output should be updated to make it more clear, especially if there any diffs have been fixed.
Also I suggest instead taking "the first snapshot after block X" take the closest snapshot "before"; and insted "the last snapshot before block Y" take the closest snapshot "after".
Because at first I tried to call "evodb repair 1176911 1176913" and nothing happened:
> evodb repair 1176911 1176913
{
"startHeight": 1176911,
"stopHeight": 1176913,
"diffsRecalculated": 0,
"snapshotsVerified": 0,
"verificationErrors": [
"Need at least 2 snapshots, found 1"
],
"repairErrors": [
]
}
3 participants
Issue being fixed or feature implemented
#6961 should fix the issue for new nodes migrating to v23. This PR aims to provide tools for nodes that updated before the fix implemented in #6961.
What was done?
Preparatory Refactoring (2 commits)
Needed for repair to rebuild from trusted snapshots.
Main Feature (1 commit)
Documentation (2 commits)
Key Points
How Has This Been Tested?
--noconnectto avoid altering blocks and chainstate), wait for migration to finish and stop the nodeevodb verify, should see a bunch of errors inverificationErrorsevodb repair, should see same errors inverificationErrorsand none inrepairErrors(can specify start/stop params from errors inverificationErrorsto speed things up a bit,verificationErrorsshould look accordingly)evodb verifyorevodb repairagain, should see no errors nowBreaking Changes
n/a
Checklist: