feat: Migrate CDeterministicMNStateDiff to a new format with nVersion being the first bit to read
#6813
Conversation
|
WalkthroughBumps EvoDB diffs key from "dmn_D3" to "dmn_D4" and preserves legacy entries under DB_LIST_DIFF_LEGACY ("dmn_D3"). Adds CDeterministicMNStateDiffLegacy with ToNewFormat(), legacy-aware deserialization in CDeterministicMNListDiff (UnserializeLegacyFormat/UnserializeImpl) and HasChanges(). Adds migration API to CDeterministicMNManager (IsMigrationRequired, MigrateLegacyDiffs) performing batched writes, legacy-key erasure, DB compaction, commit, and cache clears. Calls migration during chainstate load (new ERROR_UPGRADING_EVO_DB) and updates init error text. Adds unit tests validating bit mappings and migration and a release-note entry. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🔭 Outside diff range comments (1)
src/evo/deterministicmns.h (1)
553-582: Bug: addedMNs is not cleared before deserialization; also missed pre-reserving capacitiesReusing the same CDeterministicMNListDiff instance will accumulate addedMNs across calls, causing stale/duplicated entries and incorrect HasChanges() results. Clear it and pre-reserve for performance.
Apply this diff to fix and optimize UnserializeImpl:
template <typename Stream> void UnserializeImpl(Stream& s, bool isLegacyFormat) { - updatedMNs.clear(); - removedMns.clear(); - - for (size_t to_read = ReadCompactSize(s); to_read > 0; --to_read) { - addedMNs.push_back(std::make_shared<CDeterministicMN>(deserialize, s)); - } - - for (size_t to_read = ReadCompactSize(s); to_read > 0; --to_read) { - uint64_t internalId = ReadVarInt<Stream, VarIntMode::DEFAULT, uint64_t>(s); - if (isLegacyFormat) { - // Use legacy deserializer for old format - CDeterministicMNStateDiffLegacy legacyDiff(deserialize, s); - // Convert to new format and store - updatedMNs.emplace(internalId, legacyDiff.ToNewFormat()); - } else { - // Use current deserializer for new format - updatedMNs.emplace(internalId, CDeterministicMNStateDiff(deserialize, s)); - } - } - - for (size_t to_read = ReadCompactSize(s); to_read > 0; --to_read) { - uint64_t internalId = ReadVarInt<Stream, VarIntMode::DEFAULT, uint64_t>(s); - removedMns.emplace(internalId); - } + // Reset all collections before reading + addedMNs.clear(); + updatedMNs.clear(); + removedMns.clear(); + + // addedMNs + const size_t added_count = ReadCompactSize(s); + addedMNs.reserve(added_count); + for (size_t i = 0; i < added_count; ++i) { + addedMNs.push_back(std::make_shared<CDeterministicMN>(deserialize, s)); + } + + // updatedMNs + const size_t updated_count = ReadCompactSize(s); + updatedMNs.reserve(updated_count); + for (size_t i = 0; i < updated_count; ++i) { + const uint64_t internalId = ReadVarInt<Stream, VarIntMode::DEFAULT, uint64_t>(s); + if (isLegacyFormat) { + CDeterministicMNStateDiffLegacy legacyDiff(deserialize, s); + updatedMNs.emplace(internalId, legacyDiff.ToNewFormat()); + } else { + updatedMNs.emplace(internalId, CDeterministicMNStateDiff(deserialize, s)); + } + } + + // removedMns + const size_t removed_count = ReadCompactSize(s); + for (size_t i = 0; i < removed_count; ++i) { + const uint64_t internalId = ReadVarInt<Stream, VarIntMode::DEFAULT, uint64_t>(s); + removedMns.emplace(internalId); + } }
🧹 Nitpick comments (6)
doc/release-nodes-6813.md (2)
4-4: Fix typo: "seemless" should be "seamless"-This release automatically handles migration from legacy masternode state diff format to a new one which should make future state diff upgrades seemless. No manual intervention required for node operators. This operation is irreversible and you'd have to reindex if you decide to downgrade to a previous version. #6813 +This release automatically handles migration from legacy masternode state diff format to a new one which should make future state diff upgrades seamless. No manual intervention required for node operators. This operation is irreversible and you'd have to reindex if you decide to downgrade to a previous version. #6813
1-4: Filename appears to have a typo: "release-nodes" should be "release-notes"The file is named
doc/release-nodes-6813.mdbut should likely bedoc/release-notes-6813.mdbased on common naming conventions for release documentation.src/evo/deterministicmns.cpp (1)
1428-1429: Consider more descriptive logging for migration progressThe log message "Will erase %d legacy diffs now" could be more informative by specifying these are database entries being removed as part of the migration cleanup.
- LogPrintf("CDeterministicMNManager::%s -- Will erase %d legacy diffs now\n", __func__, keys_to_erase.size()); + LogPrintf("CDeterministicMNManager::%s -- Erasing %d legacy database entries after successful migration\n", __func__, keys_to_erase.size());src/test/evo_deterministicmns_tests.cpp (1)
1046-1046: Fix comment to match actual field countThe comment says "ALL 18 fields" but the test actually validates 19 fields (including nVersion). Update the comment to reflect the correct count.
- // Test individual field mappings for ALL 18 fields + // Test individual field mappings for ALL 19 fieldssrc/evo/deterministicmns.h (2)
546-552: Clarify scope of UnserializeLegacyFormatConsider documenting that this is intended for on-disk migration only (not P2P). A brief comment reduces accidental misuse.
656-659: Migration API: consider enforcing result usage and documenting lockingAPIs look consistent with the .cpp implementations. Consider adding [[nodiscard]] to both to discourage ignoring their boolean results, and mention in comments that they lock ::cs_main internally.
- bool IsMigrationRequired() const EXCLUSIVE_LOCKS_REQUIRED(!cs); - bool MigrateLegacyDiffs() EXCLUSIVE_LOCKS_REQUIRED(!cs); + [[nodiscard]] bool IsMigrationRequired() const EXCLUSIVE_LOCKS_REQUIRED(!cs); + [[nodiscard]] bool MigrateLegacyDiffs() EXCLUSIVE_LOCKS_REQUIRED(!cs);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
-
doc/release-nodes-6813.md(1 hunks) -
src/evo/deterministicmns.cpp(2 hunks) -
src/evo/deterministicmns.h(4 hunks) -
src/evo/dmnstate.h(7 hunks) -
src/init.cpp(1 hunks) -
src/node/chainstate.cpp(1 hunks) -
src/node/chainstate.h(1 hunks) -
src/test/evo_deterministicmns_tests.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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/node/chainstate.cppsrc/node/chainstate.hsrc/evo/deterministicmns.cppsrc/init.cppsrc/test/evo_deterministicmns_tests.cppsrc/evo/deterministicmns.hsrc/evo/dmnstate.h
doc/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Unless specifically prompted, avoid making changes to the doc directory (documentation)
Files:
doc/release-nodes-6813.md
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.cppsrc/evo/deterministicmns.hsrc/evo/dmnstate.h
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/evo_deterministicmns_tests.cpp
🧠 Learnings (2)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#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
PR: dashpay/dash#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/node/chainstate.cppsrc/evo/deterministicmns.cppsrc/test/evo_deterministicmns_tests.cppsrc/evo/deterministicmns.h
🧬 Code Graph Analysis (4)
src/evo/deterministicmns.cpp (4)
src/evo/dmnstate.h (2)
get(184-184)get(315-315)src/txmempool.cpp (2)
get(1312-1319)get(1312-1312)src/llmq/quorums.cpp (4)
batch(1048-1048)s(173-173)s(188-188)s(1166-1166)src/llmq/dkgsessionmgr.cpp (2)
s(281-281)s(318-318)
src/test/evo_deterministicmns_tests.cpp (1)
src/evo/netinfo.cpp (2)
MakeNetInfo(166-170)MakeNetInfo(166-166)
src/evo/deterministicmns.h (2)
src/evo/deterministicmns.cpp (5)
s(1388-1388)IsMigrationRequired(1341-1364)IsMigrationRequired(1341-1341)MigrateLegacyDiffs(1366-1463)MigrateLegacyDiffs(1366-1366)src/evo/dmnstate.h (1)
CDeterministicMNStateDiff(153-185)
src/evo/dmnstate.h (2)
src/evo/netinfo.h (1)
- `` (217-219)
src/bls/bls.h (1)
CBLSLazyPublicKeyVersionWrapper(575-587)
🪛 GitHub Actions: Clang Diff Format Check
src/evo/dmnstate.h
[error] 186-318: Clang-format differences detected. Command 'git diff -U0 origin/develop --
⏰ 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). (7)
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (9)
src/evo/deterministicmns.cpp (3)
30-31: LGTM! Database key versioning for format migrationThe database key change from
dmn_D3todmn_D4appropriately versions the storage format, and preserving the legacy key enables backward compatibility during migration.
1341-1364: LGTM! Efficient migration check implementationThe function correctly checks for legacy format entries using a database iterator without loading unnecessary data into memory.
1396-1408: Clarify the conversion logic - comment suggests redundancyThe comment on lines 1401-1403 states that the conversion is already done by
UnserializeLegacyFormat()and thatlegacyDiff.updatedMNsalready contains properly convertedCDeterministicMNStateDiffobjects. However, the code then creates a newconvertedDiffobject and manually copies fields fromlegacyDiff.If the conversion is truly complete after deserialization, this copying appears redundant. If the copying is necessary, the comment should be updated to accurately reflect what's happening.
- // The conversion is already done by UnserializeLegacyFormat()! - // CDeterministicMNStateDiffLegacy.ToNewFormat() was called during deserialization - // So legacyDiff.updatedMNs already contains properly converted CDeterministicMNStateDiff objects - - // Simply copy the already-converted state diffs - for (const auto& [internalId, stateDiff] : legacyDiff.updatedMNs) { - convertedDiff.updatedMNs.emplace(internalId, stateDiff); - } + // Copy the converted diff to ensure proper structure + // UnserializeLegacyFormat() has already converted the state diffs to the new format + for (const auto& [internalId, stateDiff] : legacyDiff.updatedMNs) { + convertedDiff.updatedMNs.emplace(internalId, stateDiff); + }src/node/chainstate.h (1)
48-48: LGTM! Appropriate error enum additionThe new
ERROR_UPGRADING_EVO_DBenum value is correctly placed and follows the existing naming convention for chainstate loading errors.src/init.cpp (1)
2046-2048: LGTM! Consistent error handling implementationThe error case handling for
ERROR_UPGRADING_EVO_DBfollows the established pattern and provides a clear error message to users.src/node/chainstate.cpp (1)
195-198: Consider adding logging for migration statusWhile the migration logic is correct, it would be helpful to add logging to indicate when migration is being performed or skipped, similar to the logging in the migration functions themselves.
// Check if nVersion-first migration is needed and perform it if (dmnman->IsMigrationRequired() && !dmnman->MigrateLegacyDiffs()) { return ChainstateLoadingError::ERROR_UPGRADING_EVO_DB; }The migration functions already log their status internally, so this is acceptable as-is.
src/evo/dmnstate.h (1)
284-423: Formatting Verified: No Issues FoundRan clang-format diff against
src/evo/dmnstate.hand no inconsistencies were detected. The code is properly formatted.src/evo/deterministicmns.h (2)
542-544: Good: default Unserialize delegates to implementationClear separation via UnserializeImpl(s, false) is fine.
567-575: All legacy types and conversion APIs are present and visibleThe header
src/evo/deterministicmns.hincludesevo/dmnstate.h, which declaresCDeterministicMNStateDiffLegacyand itsToNewFormat()method. No further action is needed.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
src/evo/dmnstate.h (1)
234-253: FixNetInfoSerWrapperversion check to>= ProTxVersion::ExtAddrThe equality check limits netInfo serialization to exactly the current ExtAddr version, breaking forward compatibility with any future ProTx versions. Align this with other serialization sites (e.g. in providertx.h and simplifiedmns.h) by changing
==to>=.• File: src/evo/dmnstate.h
Replace in theField_netInfobranch:- READWRITE(NetInfoSerWrapper( - const_cast<std::shared_ptr<NetInfoInterface>&>(obj.state.netInfo), - obj.state.nVersion == ProTxVersion::ExtAddr)); + READWRITE(NetInfoSerWrapper( + const_cast<std::shared_ptr<NetInfoInterface>&>(obj.state.netInfo), + obj.state.nVersion >= ProTxVersion::ExtAddr));
🧹 Nitpick comments (1)
src/evo/dmnstate.h (1)
283-423: Comprehensive legacy compatibility implementation.The CDeterministicMNStateDiffLegacy class provides a complete backward compatibility layer:
✅ Strengths:
- Maintains original legacy bit positions correctly
- Proper field mapping from legacy to new format
- Handles special cases for pubKeyOperator and netInfo
- Uses non-extended addressing for legacy format (line 376)
⚠️ Minor concerns:
- The pubKeyOperator handling in lines 385-388 uses SER_READ macro which may not be thread-safe if called concurrently
- Consider adding bounds checking for field bit operations
The legacy compatibility implementation is solid and handles the migration requirements well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
-
doc/release-notes-6813.md(1 hunks) -
src/evo/deterministicmns.cpp(2 hunks) -
src/evo/deterministicmns.h(4 hunks) -
src/evo/dmnstate.h(7 hunks) -
src/init.cpp(1 hunks) -
src/node/chainstate.cpp(1 hunks) -
src/node/chainstate.h(1 hunks) -
src/test/evo_deterministicmns_tests.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- doc/release-notes-6813.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/init.cpp
- src/node/chainstate.h
- src/test/evo_deterministicmns_tests.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/node/chainstate.cppsrc/evo/deterministicmns.cppsrc/evo/deterministicmns.hsrc/evo/dmnstate.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.cppsrc/evo/deterministicmns.hsrc/evo/dmnstate.h
🧠 Learnings (2)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#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: knst
PR: dashpay/dash#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.
📚 Learning: 2025-01-07T18:50:44.838Z
Learnt from: knst
PR: dashpay/dash#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/node/chainstate.cppsrc/evo/deterministicmns.cppsrc/evo/deterministicmns.h
🧬 Code Graph Analysis (3)
src/evo/deterministicmns.cpp (4)
src/evo/dmnstate.h (2)
get(184-184)get(315-315)src/llmq/quorums.cpp (4)
batch(1048-1048)s(173-173)s(188-188)s(1166-1166)src/llmq/dkgsessionmgr.cpp (2)
s(281-281)s(318-318)src/llmq/debug.cpp (1)
s(127-127)
src/evo/deterministicmns.h (2)
src/evo/deterministicmns.cpp (5)
s(1388-1388)IsMigrationRequired(1341-1364)IsMigrationRequired(1341-1341)MigrateLegacyDiffs(1366-1463)MigrateLegacyDiffs(1366-1366)src/evo/dmnstate.h (1)
CDeterministicMNStateDiff(153-185)
src/evo/dmnstate.h (2)
src/evo/netinfo.h (1)
- `` (217-219)
src/bls/bls.h (1)
CBLSLazyPublicKeyVersionWrapper(575-587)
🪛 GitHub Actions: Clang Diff Format Check
src/evo/deterministicmns.cpp
[error] 1425-1426: Clang-format differences detected in src/evo/deterministicmns.cpp. Command used: git diff -U0 origin/develop --
src/evo/dmnstate.h
[error] 186-198: Clang-format differences detected in src/evo/dmnstate.h. Command used: git diff -U0 origin/develop --
⏰ 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). (7)
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
🔇 Additional comments (10)
src/evo/dmnstate.h (2)
38-38: LGTM!Adding friend access for the legacy class to access private state members during migration is appropriate.
189-189: LGTM!Moving nVersion to the first position in the members tuple correctly implements the nVersion-first serialization format.
Also applies to: 207-207
src/node/chainstate.cpp (1)
195-198: Well-integrated migration step.The migration integration is properly placed in the chainstate loading flow:
✅ Good practices:
- Runs after essential DB initialization (ForceSignalDBUpdate)
- Proper error handling with specific ERROR_UPGRADING_EVO_DB code
- Conditional execution only when migration is required
- Clean integration with existing error handling pattern
The placement ensures the migration happens at the right time during node startup.
src/evo/deterministicmns.h (4)
542-551: Clean API design for format compatibility.The template method approach with shared UnserializeImpl logic is well-designed:
✅ Good architecture:
- Clear separation between legacy and new format entry points
- Shared implementation reduces code duplication
- Template design works with different stream types
- Backward compatibility maintained through dedicated legacy method
554-582: Robust deserialization implementation.The UnserializeImpl method correctly handles both formats:
✅ Implementation highlights:
- Proper format-specific deserialization logic
- Legacy format conversion via CDeterministicMNStateDiffLegacy::ToNewFormat()
- Consistent handling of addedMNs and removedMns across formats
- Clear separation of legacy vs new format code paths
The implementation ensures seamless migration between formats.
585-588: LGTM!The HasChanges() method provides a clean utility for checking if a diff contains modifications. The implementation correctly checks all three change types.
656-658: Well-designed migration API.The migration API methods are properly designed:
✅ Good practices:
- Clear method names indicating purpose
- Appropriate const qualifier for IsMigrationRequired()
- Proper lock annotations for thread safety
- Clean separation between check and action methods
src/evo/deterministicmns.cpp (3)
30-31: Appropriate database key versioning strategy.The key versioning approach enables safe migration:
✅ Benefits:
- dmn_D4 for new nVersion-first format
- dmn_D3 preserved as legacy format identifier
- Allows coexistence during migration
- Clear separation between formats
1341-1364: Efficient migration detection.The IsMigrationRequired implementation is well-designed:
✅ Implementation strengths:
- Efficient check using database iterator (no need to load all data)
- Proper resource management with explicit pcursor.reset()
- Clear logging for operational visibility
- Early return when legacy entries found
1423-1463: Thorough migration cleanup with proper error handling.The migration cleanup phase is comprehensive:
✅ Complete cleanup process:
- Batch deletion of legacy entries with size monitoring
- Database compaction to reclaim space (line 1446)
- Proper error checking on commit operations (lines 1449-1452)
- Cache clearing to ensure new format usage (lines 1455-1457)
- Success metrics logging for operational tracking
⚠️ Minor observation:
The cache clearing happens under both cs_main and cs locks, which is correct for thread safety but worth noting the lock ordering.The migration implementation handles all aspects of the format transition properly.
UdjinM6
force-pushed
the
nver_first
branch
2 times, most recently
from
7ef8324 to
c5e0f90
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
src/test/evo_deterministicmns_tests.cpp (1)
26-30: Add missing header for std::set to fix compilation.std::set is used below (Line 1104) but isn’t included.
Apply this diff:
#include <boost/test/unit_test.hpp> #include <vector> +#include <set>src/evo/deterministicmns.h (1)
553-585: UnserializeImpl does not clear addedMNs; stale entries possible on reuse.updatedMNs and removedMns are cleared, but addedMNs is not. This risks appending to previous content if the same object is reused.
Apply this diff:
private: template <typename Stream> void UnserializeImpl(Stream& s, bool isLegacyFormat) { - updatedMNs.clear(); - removedMns.clear(); + addedMNs.clear(); + updatedMNs.clear(); + removedMns.clear();
♻️ Duplicate comments (2)
src/evo/dmnstate.h (2)
186-211: Fix clang-format issues in DMN_STATE_MEMBER block.CI flagged formatting differences here. Please run the repo’s clang-format to align with project style.
318-343: Fix clang-format issues in LEGACY_DMN_STATE_MEMBER block.CI flagged formatting differences here. Please run the repo’s clang-format to align with project style.
🧹 Nitpick comments (2)
src/test/evo_deterministicmns_tests.cpp (1)
1044-1143: Good coverage of legacy→new bit mapping; consider adding a negative test for stricter nVersion requirement.You validate mappings and a typical legacy→new conversion. Add a test ensuring the new-format serializer throws when pubKeyOperator or netInfo bits are set without nVersion, matching the new guard in CDeterministicMNStateDiff.
Example (can be placed near these tests):
BOOST_AUTO_TEST_CASE(state_diff_requires_version_when_pubkey_or_netinfo_set) { // Build an invalid new-format diff: pubKeyOperator but missing nVersion CDeterministicMNStateDiff bad; bad.fields = CDeterministicMNStateDiff::Field_pubKeyOperator; // no Field_nVersion bad.state.nVersion = ProTxVersion::BasicBLS; CDataStream ss(SER_DISK, CLIENT_VERSION); BOOST_CHECK_THROW(ss << bad, std::ios_base::failure); // Same for netInfo CDeterministicMNStateDiff bad2; bad2.fields = CDeterministicMNStateDiff::Field_netInfo; // no Field_nVersion bad2.state.nVersion = ProTxVersion::BasicBLS; BOOST_CHECK_THROW(ss << bad2, std::ios_base::failure); }If you’d like, I can add this test in a follow-up commit.
src/evo/deterministicmns.cpp (1)
1391-1394: Log and continue strategy on ReadDataStream failure during migration.Currently, a read failure breaks the loop without context. Consider logging the failure to aid operations, and decide whether to continue or abort explicitly.
Apply this diff:
- if (!m_evoDb.GetRawDB().ReadDataStream(k, s)) { - break; - } + if (!m_evoDb.GetRawDB().ReadDataStream(k, s)) { + LogPrintf("CDeterministicMNManager::%s -- Failed to read legacy diff value, aborting migration\n", __func__); + break; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
doc/release-notes-6813.md(1 hunks)src/evo/deterministicmns.cpp(2 hunks)src/evo/deterministicmns.h(4 hunks)src/evo/dmnstate.h(7 hunks)src/init.cpp(1 hunks)src/node/chainstate.cpp(1 hunks)src/node/chainstate.h(1 hunks)src/test/evo_deterministicmns_tests.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- doc/release-notes-6813.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/init.cpp
- src/node/chainstate.h
- src/node/chainstate.cpp
🧰 Additional context used
📓 Path-based instructions (3)
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/test/evo_deterministicmns_tests.cppsrc/evo/deterministicmns.cppsrc/evo/dmnstate.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.hsrc/evo/deterministicmns.cppsrc/evo/dmnstate.h
src/{test,wallet/test,qt/test}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Unit tests for C++ code should be placed in src/test/, src/wallet/test/, or src/qt/test/ and use Boost::Test or Qt 5 for GUI tests
Files:
src/test/evo_deterministicmns_tests.cpp
🧠 Learnings (3)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#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: knst
PR: dashpay/dash#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.
📚 Learning: 2025-01-07T18:50:44.838Z
Learnt from: knst
PR: dashpay/dash#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/test/evo_deterministicmns_tests.cppsrc/evo/deterministicmns.cpp
📚 Learning: 2025-05-10T00:54:15.597Z
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/evo/deterministicmns.cpp:473-485
Timestamp: 2025-05-10T00:54:15.597Z
Learning: In the current implementation of `NetInfoEntry`, only `CService` is a valid type and `std::monostate` represents an invalid state. Entries that don't provide a valid `CService` through `GetAddrPort()` should be rejected with an exception, not silently skipped.
Applied to files:
src/evo/dmnstate.h
🧬 Code Graph Analysis (3)
src/evo/deterministicmns.h (2)
src/evo/deterministicmns.cpp (5)
s(1390-1390)IsMigrationRequired(1341-1365)IsMigrationRequired(1341-1341)MigrateLegacyDiffs(1367-1466)MigrateLegacyDiffs(1367-1367)src/evo/dmnstate.h (2)
CDeterministicMNStateDiff(153-185)nodiscard(243-271)
src/test/evo_deterministicmns_tests.cpp (1)
src/evo/netinfo.cpp (2)
MakeNetInfo(166-170)MakeNetInfo(166-166)
src/evo/dmnstate.h (3)
src/evo/netinfo.h (1)
- `` (217-219)
src/bls/bls.h (1)
CBLSLazyPublicKeyVersionWrapper(575-587)src/evo/providertx.h (6)
platformNodeID(68-68)platformNodeID(134-134)platformP2PPort(69-69)platformP2PPort(135-135)platformHTTPPort(70-70)platformHTTPPort(136-136)
🪛 GitHub Actions: Clang Diff Format Check
src/evo/dmnstate.h
[error] 186-211: Clang-format differences found in DMN_STATE_MEMBER block. The clang-format-diff.py check failed with exit code 1. Run './contrib/devtools/clang-format-diff.py -p1' to fix formatting.
[error] 318-343: Clang-format differences found in LEGACY_DMN_STATE_MEMBER block. The clang-format-diff.py check failed with exit code 1. Run './contrib/devtools/clang-format-diff.py -p1' to fix formatting.
⏰ 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). (7)
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
🔇 Additional comments (4)
src/evo/deterministicmns.h (1)
546-552: Public legacy-aware Unserialize entry point looks good.Clear separation via UnserializeLegacyFormat and shared Impl improves maintainability while keeping the public API clean.
src/evo/deterministicmns.cpp (3)
29-32: DB key bump and legacy key introduction LGTM.dmn_D4 for new diffs and preserving dmn_D3 for legacy migration is consistent and safe.
1341-1365: Migration detection logic is sound and lock-aware.Scanning for DB_LIST_DIFF_LEGACY with cs_main held is appropriate.
1457-1465: Cache invalidation after migration LGTM.Clearing both list and diff caches under cs ensures consistent reloads with the new format.
…sticMNListDiff::Unserialize()` 8e0e63f fix: Reset all collections before reading in `CDeterministicMNListDiff::Unserialize()` (UdjinM6) Pull request description: ## Issue being fixed or feature implemented Fix the issue found by @coderabbitai while reviewing #6813 #6813 (review) ## What was done? ## How Has This Been Tested? ## Breaking Changes n/a ## Checklist: - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: kwvg: utACK 8e0e63f Tree-SHA512: 22fe37720a87052b450c33c0a51537058b776d103981506b3556e7be76c0a3390764d089bb16bac0722468754eadf77177fd8d39b873121a1f9fdbf7220710d0
|
This pull request has conflicts, please rebase. |
… field positions and conversion to new format
…`CDeterministicMNStateDiff`
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/evo/dmnstate.h (5)
157-176: Bit remap looks correct; fix formatting per pipelineThe new Field bit assignments correctly place nVersion at 0x0001 and shift subsequent fields as intended.
However, clang-format is failing on this file; please run clang-format as indicated by the pipeline.Run the formatter locally to match project style:
- clang-format -i src/evo/dmnstate.h
- Or use the repo’s helper script (if available) to format diffs only.
186-211: Resolve clang-format violations (members tuple region)CI reports clang-format differences for this region. Please reformat before merging.
Apply the project’s clang-format to this file; no logic changes expected.
318-343: Resolve clang-format violations (LEGACY_DMN_STATE_MEMBER region)CI reports formatting differences here as well. Please run clang-format on this file.
Re-run clang-format for the whole file to normalize macro indentation and line breaks.
261-264: Fix ExtAddr gating: use >= instead of == for netInfo in new formatFuture ProTx versions beyond ExtAddr must continue to use the extended-address serialization. Switch equality to greater-or-equal.
Apply this diff:
- READWRITE(NetInfoSerWrapper(const_cast<std::shared_ptr<NetInfoInterface>&>(obj.state.netInfo), - obj.state.nVersion == ProTxVersion::ExtAddr)); + READWRITE(NetInfoSerWrapper(const_cast<std::shared_ptr<NetInfoInterface>&>(obj.state.netInfo), + obj.state.nVersion >= ProTxVersion::ExtAddr));
359-389: Legacy deserializer must enforce nVersion presence when pubKeyOperator or netInfo are setThis mirrors the stricter validation in the new format and prevents ambiguous decoding of legacy diffs.
Apply this diff:
SERIALIZE_METHODS(CDeterministicMNStateDiffLegacy, obj) { READWRITE(VARINT(obj.fields)); + // pubKeyOperator and netInfo require nVersion in legacy diffs as well + if (((obj.fields & LegacyField_pubKeyOperator) || (obj.fields & LegacyField_netInfo)) && + !(obj.fields & LegacyField_nVersion)) { + throw std::ios_base::failure("Invalid legacy data, nVersion unset when pubKeyOperator or netInfo set"); + } + bool read_pubkey{false}; boost::hana::for_each(legacy_members, [&](auto&& member) { using BaseType = std::decay_t<decltype(member)>; if constexpr (BaseType::mask == LegacyField_pubKeyOperator) { if (obj.fields & member.mask) { SER_READ(obj, read_pubkey = true); READWRITE(CBLSLazyPublicKeyVersionWrapper(const_cast<CBLSLazyPublicKey&>(obj.state.pubKeyOperator), obj.state.nVersion == ProTxVersion::LegacyBLS)); }Optional follow-up: if we want to guarantee the correct read-time behavior for pubKeyOperator based on nVersion, we can pre-read nVersion when present before iterating fields. If you want that change, I can provide a minimal diff that avoids double-reading while keeping the hana iteration for other members.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
doc/release-notes-6813.md(1 hunks)src/evo/deterministicmns.cpp(2 hunks)src/evo/deterministicmns.h(4 hunks)src/evo/dmnstate.h(7 hunks)src/init.cpp(1 hunks)src/node/chainstate.cpp(1 hunks)src/node/chainstate.h(1 hunks)src/test/evo_deterministicmns_tests.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- doc/release-notes-6813.md
- src/node/chainstate.cpp
- src/init.cpp
- src/evo/deterministicmns.cpp
- src/node/chainstate.h
- src/test/evo_deterministicmns_tests.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.hsrc/evo/dmnstate.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.hsrc/evo/dmnstate.h
🧠 Learnings (4)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#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: knst
PR: dashpay/dash#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.
📚 Learning: 2025-01-07T18:50:44.838Z
Learnt from: knst
PR: dashpay/dash#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-05-10T00:54:15.597Z
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/evo/deterministicmns.cpp:473-485
Timestamp: 2025-05-10T00:54:15.597Z
Learning: In the current implementation of `NetInfoEntry`, only `CService` is a valid type and `std::monostate` represents an invalid state. Entries that don't provide a valid `CService` through `GetAddrPort()` should be rejected with an exception, not silently skipped.
Applied to files:
src/evo/dmnstate.h
📚 Learning: 2025-06-06T11:53:09.094Z
Learnt from: kwvg
PR: dashpay/dash#6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Applied to files:
src/evo/dmnstate.h
🧬 Code Graph Analysis (2)
src/evo/deterministicmns.h (2)
src/evo/deterministicmns.cpp (5)
s(1390-1390)IsMigrationRequired(1341-1365)IsMigrationRequired(1341-1341)MigrateLegacyDiffs(1367-1466)MigrateLegacyDiffs(1367-1367)src/evo/dmnstate.h (1)
CDeterministicMNStateDiff(153-185)
src/evo/dmnstate.h (2)
src/evo/netinfo.h (1)
- `` (217-219)
src/bls/bls.h (1)
CBLSLazyPublicKeyVersionWrapper(575-587)
🪛 GitHub Actions: Clang Diff Format Check
src/evo/dmnstate.h
[error] 186-211: Clang format differences found in src/evo/dmnstate.h. Run 'clang-format' or 'clang-format-diff.py -p1' to fix code style issues in this region.
[error] 318-343: Clang format differences found in src/evo/dmnstate.h (LEGACY_DMN_STATE_MEMBER region). Run 'clang-format' or 'clang-format-diff.py -p1' to fix code style issues.
⏰ 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). (4)
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (10)
src/evo/dmnstate.h (5)
38-38: Friendship for legacy diff is appropriateGranting friend access to CDeterministicMNStateDiffLegacy is reasonable to enable legacy-aware (de)serialization without exposing additional public API.
189-189: nVersion as the first member in members tuple — LGTMHaving nVersion first in the members tuple aligns with the new first-bit semantics and simplifies guards in diff construction/serialization.
234-238: Auto-including nVersion when pubKeyOperator or netInfo change — LGTMEnsuring Field_nVersion is present whenever pubKeyOperator or netInfo are included avoids ambiguous decoding at call sites.
249-253: Stricter validation on new format is correctThrowing on missing nVersion when pubKeyOperator or netInfo are set is the right behavior to fail fast on malformed data.
398-419: Legacy-to-new bit mapping — LGTMThe mapping covers all fields, including nVersion, and preserves semantics. Returning a CDeterministicMNStateDiff is the right integration point for downstream code.
src/evo/deterministicmns.h (5)
542-545: Wrapper Unserialize delegates to new-format path — LGTMDefaulting to the new format via UnserializeImpl(s, false) keeps wire format clear and centralizes logic.
546-552: Legacy-aware Unserialize is a clean API additionProviding UnserializeLegacyFormat(Stream&) is a sensible way to decode legacy diffs at the call site without contaminating the primary path.
555-580: Legacy conversion during deserialization — LGTM; minor comment nit retained
- Reading added/removed/updated collections is correct.
- Using CDeterministicMNStateDiffLegacy(deserialize, s) and converting via ToNewFormat() keeps the rest of the pipeline operating solely on the new format.
- The inline comment explaining why field additions don’t need migration while field order does is still relevant. Keeping it is fine.
588-592: HasChanges() helper — LGTMThis is a useful query for upstream logic and tests.
660-663: Migration hooks on manager — LGTM; ensure locking discipline in implementationAPI surface and annotations look good. Verify implementation acquires the expected locks and handles iterator invalidation around batch writes and compaction.
Would you like me to scan the implementation in src/evo/deterministicmns.cpp for:
- correct usage of DB_LIST_DIFF vs DB_LIST_DIFF_LEGACY keys,
- batched writes and erasures ordering,
- lock coverage around iterator usage and compaction calls?
I can provide a grep/ast-grep script to highlight these patterns if helpful.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/evo/dmnstate.h (3)
157-176: Fix clang-format issuesThe Field enum bit assignments are logically correct for the new nVersion-first format, but there are clang-format differences that need to be addressed.
Apply clang-format to fix the formatting issues:
#!/bin/bash # Fix clang-format issues in src/evo/dmnstate.h clang-format -i src/evo/dmnstate.h
261-264: Fix ExtAddr gating: use>=instead of==when (de)serializing netInfo in new formatFuture state versions above ExtAddr should still use the extended-address serialization. Using equality will silently serialize/deserialize netInfo incorrectly for versions beyond ExtAddr.
Apply this diff:
- READWRITE(NetInfoSerWrapper(const_cast<std::shared_ptr<NetInfoInterface>&>(obj.state.netInfo), - obj.state.nVersion == ProTxVersion::ExtAddr)); + READWRITE(NetInfoSerWrapper(const_cast<std::shared_ptr<NetInfoInterface>&>(obj.state.netInfo), + obj.state.nVersion >= ProTxVersion::ExtAddr));
318-343: Fix clang-format issues in legacy member definitionsThe legacy member definitions have formatting issues that need to be addressed.
Apply clang-format to fix the formatting issues:
#!/bin/bash # Fix clang-format issues in src/evo/dmnstate.h (legacy members section) clang-format -i src/evo/dmnstate.h
🧹 Nitpick comments (4)
src/evo/dmnstate.h (1)
356-380: Consider validating nVersion presence in legacy formatThe legacy deserializer correctly handles field deserialization with appropriate legacy flags for
pubKeyOperatorand non-extended addresses fornetInfo. However, consider adding validation similar to the new format to ensurenVersionis present whenpubKeyOperatorornetInfoare set.src/evo/deterministicmns.cpp (3)
1367-1511: Comprehensive migration implementation with good operational practicesThe migration logic demonstrates solid engineering:
✅ Strengths:
- Clear documentation of critical migration scope
- Efficient batch operations with size monitoring (1 << 24 bytes)
- Proper use of
UnserializeLegacyFormatfor legacy data- Conversion handled automatically via
ToNewFormat()during deserialization- Comprehensive logging for operational visibility
- Database compaction after migration for optimal performance
- Proper cache invalidation to ensure clean reload
Consider adding a progress indicator for long-running migrations. Since the migration can take 1.5-5 minutes on mainnet, periodic progress logs (e.g., every 1000 blocks) would help operators monitor the migration status.
1425-1426: Fix inconsistent field bit checkingThe condition checks are split across two lines but should be on the same line for consistency with the codebase style.
Apply this diff to improve readability:
- if ((stateDiff.fields & CDeterministicMNStateDiff::Field_pubKeyOperator) || - (stateDiff.fields & CDeterministicMNStateDiff::Field_netInfo)) { + if ((stateDiff.fields & CDeterministicMNStateDiff::Field_pubKeyOperator) || (stateDiff.fields & CDeterministicMNStateDiff::Field_netInfo)) {
1503-1505: Consider using WithLock pattern for cache clearingWhile the current implementation is correct, consider using the
WITH_LOCKmacro for consistency with modern C++ practices in the codebase.Apply this diff for consistency:
- LOCK(cs); - mnListsCache.clear(); - mnListDiffsCache.clear(); + WITH_LOCK(cs, { + mnListsCache.clear(); + mnListDiffsCache.clear(); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/evo/deterministicmns.cpp(2 hunks)src/evo/deterministicmns.h(4 hunks)src/evo/dmnstate.h(7 hunks)src/node/chainstate.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/node/chainstate.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.cppsrc/evo/deterministicmns.hsrc/evo/dmnstate.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.cppsrc/evo/deterministicmns.hsrc/evo/dmnstate.h
🧠 Learnings (4)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#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: knst
PR: dashpay/dash#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.
📚 Learning: 2025-01-07T18:50:44.838Z
Learnt from: knst
PR: dashpay/dash#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.cppsrc/evo/deterministicmns.h
📚 Learning: 2025-05-10T00:54:15.597Z
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/evo/deterministicmns.cpp:473-485
Timestamp: 2025-05-10T00:54:15.597Z
Learning: In the current implementation of `NetInfoEntry`, only `CService` is a valid type and `std::monostate` represents an invalid state. Entries that don't provide a valid `CService` through `GetAddrPort()` should be rejected with an exception, not silently skipped.
Applied to files:
src/evo/dmnstate.h
📚 Learning: 2025-06-06T11:53:09.094Z
Learnt from: kwvg
PR: dashpay/dash#6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Applied to files:
src/evo/dmnstate.h
🧬 Code Graph Analysis (2)
src/evo/deterministicmns.cpp (1)
src/evo/dmnstate.h (2)
get(184-184)get(315-315)
src/evo/deterministicmns.h (2)
src/evo/deterministicmns.cpp (5)
s(1400-1400)IsMigrationRequired(1341-1365)IsMigrationRequired(1341-1341)MigrateLegacyDiffs(1367-1511)MigrateLegacyDiffs(1367-1367)src/evo/dmnstate.h (1)
CDeterministicMNStateDiff(153-185)
🪛 GitHub Actions: Clang Diff Format Check
src/evo/dmnstate.h
[error] 186-211: Clang-format differences detected by clang-format-diff.py for src/evo/dmnstate.h. Apply formatting changes.
[error] 318-343: Clang-format differences detected by clang-format-diff.py for src/evo/dmnstate.h. Apply formatting changes.
🔇 Additional comments (11)
src/evo/deterministicmns.h (5)
542-544: LGTM! Clean delegation to UnserializeImplThe new pattern correctly delegates to the private implementation with the legacy format flag set to false, maintaining the default behavior while enabling the legacy format support.
546-551: LGTM! Well-structured legacy format supportThe public
UnserializeLegacyFormatmethod provides a clear interface for legacy deserialization, properly delegating to the private implementation with the legacy flag set to true.
553-580: LGTM! Robust implementation with proper legacy format handlingThe implementation elegantly handles both legacy and new formats through a single code path:
- Legacy format entries are deserialized via
CDeterministicMNStateDiffLegacyand converted usingToNewFormat()- New format entries use direct deserialization via
CDeterministicMNStateDiff- The conversion happens transparently during deserialization, which is efficient
588-592: LGTM! Useful utility method for detecting non-empty diffsThe
HasChanges()method provides a clean way to check if a diff contains any actual changes, which is used throughout the codebase for optimization purposes.
660-663: LGTM! Well-designed migration API with proper thread safetyThe migration methods are properly designed with:
- Clear const-correctness for
IsMigrationRequired()- Appropriate thread safety annotations (
EXCLUSIVE_LOCKS_REQUIRED)- Logical separation between detection and execution of migration
src/evo/dmnstate.h (4)
234-238: LGTM! Ensures nVersion is set when required fields are presentThe logic correctly enforces that
nVersionmust be included whenpubKeyOperatorornetInfofields are present, maintaining consistency with the serialization requirements.
249-252: LGTM! Critical validation for data integrityThe validation correctly rejects malformed data where
pubKeyOperatorornetInfoare set without the requirednVersionfield, preventing deserialization of invalid state diffs.
349-353: LGTM! Clean constructor for legacy deserializationThe constructor properly delegates to the serialization method for deserialization.
383-413: LGTM! Clean and efficient legacy-to-new format conversionThe
ToNewFormat()method efficiently maps legacy field bits to new positions using a macro-based approach, which is both maintainable and performant. The conversion preserves all state data correctly.src/evo/deterministicmns.cpp (2)
30-31: LGTM! Clear separation between new and legacy DB keysThe DB key versioning approach is clean - bumping
DB_LIST_DIFFto "dmn_D4" for the new format while preserving "dmn_D3" asDB_LIST_DIFF_LEGACYenables safe migration without data loss.
1341-1365: LGTM! Efficient migration detection logicThe implementation efficiently checks for legacy entries by:
- Using a database iterator to seek to the legacy key space
- Checking only the first entry (sufficient to determine if migration is needed)
- Providing clear logging for operational visibility
- Properly asserting the required lock
|
Should be working correctly now. It's slower because we reconstruct current mn list ( |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/evo/dmnstate.h (3)
157-176: Enum bit remap aligns with “nVersion-first” format; fix clang-format driftThe bit assignments are logically correct and place nVersion as the first bit. However, CI flagged clang-format differences for this file.
Run the project’s clang-format check locally and reformat this file:
#!/bin/bash set -euo pipefail # Show clang-format issues in this PR (mirrors CI script) git diff -U0 origin/develop -- $(git ls-files -- $(cat test/util/data/non-backported.txt)) | ./contrib/devtools/clang-format-diff.py -p1 # To autofix src/evo/dmnstate.h: clang-format -i src/evo/dmnstate.h git add src/evo/dmnstate.h
261-264: NetInfo ext-addr gating fixed (>= ExtAddr): matches existing usageThanks for switching to >=. This aligns with providertx and simplifiedmns codepaths and ensures forward compatibility with newer versions.
356-381: Enforce nVersion presence in legacy diffs when pubKeyOperator or netInfo bits are setCurrently the legacy deserializer accepts inputs that set pubKeyOperator or netInfo while omitting LegacyField_nVersion. Invalid inputs will only fail later when converting/serializing to the new format, which makes debugging harder and allows malformed legacy records to propagate further into the migration pipeline.
Add a fail-fast check in the legacy deserializer, consistent with the new format’s stricter validation.
Apply this diff:
SERIALIZE_METHODS(CDeterministicMNStateDiffLegacy, obj) { READWRITE(VARINT(obj.fields)); + // pubKeyOperator and netInfo require nVersion in legacy diffs as well + if (((obj.fields & LegacyField_pubKeyOperator) || (obj.fields & LegacyField_netInfo)) && + !(obj.fields & LegacyField_nVersion)) { + throw std::ios_base::failure("Invalid legacy data, nVersion unset when pubKeyOperator or netInfo set"); + } + boost::hana::for_each(legacy_members, [&](auto&& member) { using BaseType = std::decay_t<decltype(member)>; if constexpr (BaseType::mask == LegacyField_pubKeyOperator) { if (obj.fields & member.mask) { // We'll set proper scheme later in MigrateLegacyDiffs() READWRITE(CBLSLazyPublicKeyVersionWrapper(const_cast<CBLSLazyPublicKey&>(obj.state.pubKeyOperator), /*legacy=*/true));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
-
src/evo/dmnstate.h(7 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/dmnstate.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/dmnstate.h
🧠 Learnings (4)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#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: knst
PR: dashpay/dash#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.
📚 Learning: 2025-05-10T00:54:15.597Z
Learnt from: kwvg
PR: dashpay/dash#6629
File: src/evo/deterministicmns.cpp:473-485
Timestamp: 2025-05-10T00:54:15.597Z
Learning: In the current implementation of `NetInfoEntry`, only `CService` is a valid type and `std::monostate` represents an invalid state. Entries that don't provide a valid `CService` through `GetAddrPort()` should be rejected with an exception, not silently skipped.
Applied to files:
src/evo/dmnstate.h
📚 Learning: 2025-06-06T11:53:09.094Z
Learnt from: kwvg
PR: dashpay/dash#6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/*is_basic_scheme_active=*/true, /*is_extended_addr=*/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.
Applied to files:
src/evo/dmnstate.h
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
PR: dashpay/dash#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/dmnstate.h
🧬 Code Graph Analysis (1)
src/evo/dmnstate.h (3)
src/evo/netinfo.h (1)
- `` (217-219)
src/bls/bls.h (1)
CBLSLazyPublicKeyVersionWrapper(575-587)src/evo/providertx.h (6)
platformNodeID(68-68)platformNodeID(134-134)platformP2PPort(69-69)platformP2PPort(135-135)platformHTTPPort(70-70)platformHTTPPort(136-136)
🪛 GitHub Actions: Clang Diff Format Check
src/evo/dmnstate.h
[error] 1-1: Clang-format differences detected. Command failed: 'git diff -U0 origin/develop --
⏰ 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: arm-linux-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: Lint / Run linters
🔇 Additional comments (6)
src/evo/dmnstate.h (6)
38-38: Friendship for legacy diff: LGTMGranting CDeterministicMNStateDiffLegacy access to CDeterministicMNState internals is appropriate for legacy deserialization and migration.
188-208: Members tuple updated to match new bit order: LGTMPlacing nVersion first in the tuple ensures correct (de)serialization order and allows version-dependent reads downstream.
234-238: Auto-including nVersion when pubKeyOperator/netInfo change: LGTMThis guarantees schema-dependent members aren’t serialized without their governing version, preventing ambiguous decoding.
249-253: Strict validation added for new format: LGTMThrowing on pubKeyOperator/netInfo without nVersion is the right fail-fast behavior for the new wire format.
383-414: Legacy-to-new field bit mapping: LGTMBitfield remap and state copy look correct. Not synthesizing Field_nVersion preserves strictness and lets new-format validation catch malformed legacy inputs.
365-373: Confirmed: migration normalizes BLS scheme for legacy pubKeyOperatorMigrateLegacyDiffs explicitly normalizes the pubKeyOperator scheme after legacy deserialization, so non-legacy BLS keys from legacy diffs are not left misparsed.
- Legacy deserializer reads pubKeyOperator using legacy=true (temporary): src/evo/dmnstate.h (READWRITE(... /legacy=/true)) — around lines 362–366.
- Migration fixes the scheme once nVersion is known: src/evo/deterministicmns.cpp — legacyDiff.UnserializeLegacyFormat(...) and later stateDiff.state.pubKeyOperator.SetLegacy(stateDiff.state.nVersion == ProTxVersion::LegacyBLS) (around lines 1406–1432).
- SetLegacy exists on the lazy wrapper and controls the stored legacy flag: src/bls/bls.h (CBLSLazyWrapper::SetLegacy / CBLSLazyPublicKeyVersionWrapper) — ~lines 559–583.
- Tests exercise conversion of non-legacy keys in legacy diffs: src/test/evo_deterministicmns_tests.cpp (legacyDiff with BasicBLS → ToNewFormat()) — around lines 1121–1131.
Result: no change required.
| if (obj.fields & member.mask) { | ||
| // As nVersion is stored after netInfo, we use a magic word to determine the underlying implementation | ||
| // TODO: Implement this | ||
| READWRITE(NetInfoSerWrapper(const_cast<std::shared_ptr<NetInfoInterface>&>(obj.state.netInfo), | ||
| /*is_extended=*/false)); | ||
| obj.state.nVersion >= ProTxVersion::ExtAddr)); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
NetInfo must reject invalid entries (monostate) during (de)serialization
Per prior learnings, only CService is a valid NetInfo entry; std::monostate should be treated as invalid. Please verify NetInfoSerWrapper enforces this and throws on invalid inputs (both new and legacy paths). If it doesn’t, add a guard.
I can check the wrapper’s implementation to confirm behavior:
Also applies to: 369-373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate NetInfoSerWrapper and confirm it validates the underlying NetInfoInterface value
rg -nP -C3 'NetInfoSerWrapper\s*\('
rg -nP -C3 'class\s+NetInfoSerWrapper|struct\s+NetInfoSerWrapper'
rg -nP -C3 'GetAddrPort\s*\('Length of output: 14519
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the NetInfoSerWrapper implementation and nearby context
sed -n '1,240p' src/evo/netinfo.h
echo "----"
sed -n '1,340p' src/evo/netinfo.cppLength of output: 15025
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print the remainder of netinfo.h including NetInfoSerWrapper methods
sed -n '240,520p' src/evo/netinfo.hLength of output: 635
NetInfoSerWrapper must reject invalid (monostate) NetInfo entries during (de)serialization — fix required
I inspected the implementation: NetInfoSerWrapper::Unserialize simply constructs a MnNetInfo from the stream and does not validate its contents; Serialize only asserts on unexpected implementation type but does not validate the MnNetInfo contents. Therefore invalid/missing CService (std::monostate) is currently accepted silently on deserialize (and may be serialized silently), which matches the concern.
Files to change
- src/evo/netinfo.h — NetInfoSerWrapper::Serialize / NetInfoSerWrapper::Unserialize
- (informational) src/evo/netinfo.cpp — MnNetInfo::Unserialize (current behavior leads to an empty/invalid NetInfoEntry when the CService is invalid)
Suggested fix (high level)
- After constructing / before emitting MnNetInfo, validate and reject invalid entries. For example:
- In Serialize: ensure the dynamic_cast succeeds and the MnNetInfo is not empty and Validate() == NetInfoStatus::Success; if not, throw std::ios_base::failure.
- In Unserialize: after creating the MnNetInfo, if IsEmpty() or Validate() != NetInfoStatus::Success, throw std::ios_base::failure.
Minimal example change (replace bodies in netinfo.h):
- Serialize:
if (const auto ptr{std::dynamic_pointer_cast<MnNetInfo>(m_data)}) { if (ptr->IsEmpty() || ptr->Validate() != NetInfoStatus::Success) { throw std::ios_base::failure("invalid NetInfo entry"); } s << *ptr; } else { assert(false); } - Unserialize:
m_data = std::make_shared<MnNetInfo>(deserialize, s); if (!m_data || m_data->IsEmpty() || m_data->Validate() != NetInfoStatus::Success) { throw std::ios_base::failure("invalid NetInfo entry"); }
Reasoning: callers (and earlier NetInfoEntry::Unserialize) already catch/expect std::ios_base::failure for deserialization errors — throwing keeps behavior consistent and prevents silently accepting std::monostate entries.
🤖 Prompt for AI Agents
In src/evo/netinfo.h (Serialize/Unserialize implementations) the wrapper
currently accepts/produces monostate/invalid MnNetInfo silently; change
Serialize to dynamic_cast to MnNetInfo, assert on failure, then check
ptr->IsEmpty() or ptr->Validate() != NetInfoStatus::Success and throw
std::ios_base::failure("invalid NetInfo entry") if invalid before writing to the
stream; change Unserialize to construct the MnNetInfo into m_data (e.g.
make_shared<MnNetInfo>(...)) and after construction check m_data non-null,
!m_data->IsEmpty() and m_data->Validate() == NetInfoStatus::Success, throwing
std::ios_base::failure("invalid NetInfo entry") on failure so invalid/monostate
entries are rejected during (de)serialization.
Issue being fixed or feature implemented
nVersionis needed forpubKeyOperatorandnetInfodiffs. Let's fix bit order and streamline the logic instead of adding workarounds and magic numbers (#6666). Migration takes1m30s3-5 minutes on mainnet on my machine.What was done?
Changes:
CDeterministicMNStateDiffLegacyclass to maintain backward compatibility with existing data while enabling migration to the optimized formatnVersionfield from position0x40000to0x0001(first position) inCDeterministicMNStateDiffpubKeyOperatorandnetInfobits in the new serialization formatHow Has This Been Tested?
Breaking Changes
Needs reindex on downgrades
Checklist: