test: Split CDeterministicMNStateDiffLegacy ser/deser, improve tests #6960
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThis change adds explicit Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (2)src/**/*.{cpp,h,cc,cxx,hpp}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-06-06T11:53:09.094ZApplied to files:
📚 Learning: 2025-02-06T14:34:30.466ZApplied to files:
🧬 Code graph analysis (1)src/evo/dmnstate.h (1)
🪛 GitHub Actions: Clang Diff Format Checksrc/evo/dmnstate.h[error] 357-384: Clang format differences detected. Run 'clang-format-diff.py -p1' or 'clang-format' 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)
🔇 Additional comments (1)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
-
src/evo/dmnstate.h(1 hunks) -
src/test/evo_deterministicmns_tests.cpp(1 hunks)
🧰 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/dmnstate.hsrc/test/evo_deterministicmns_tests.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/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
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: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.
📚 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/test/evo_deterministicmns_tests.cpp
📚 Learning: 2025-06-06T11:53:09.094Z
Learnt from: kwvg
Repo: dashpay/dash PR: 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/test/evo_deterministicmns_tests.cpp
🧬 Code graph analysis (2)
src/evo/dmnstate.h (1)
src/bls/bls.h (1)
CBLSLazyPublicKeyVersionWrapper(575-587)
src/test/evo_deterministicmns_tests.cpp (1)
src/evo/netinfo.cpp (2)
MakeNetInfo(273-280)MakeNetInfo(273-273)
🪛 GitHub Actions: Clang Diff Format Check
src/evo/dmnstate.h
[error] 357-383: Clang-format differences found. 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: mac-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (4)
src/test/evo_deterministicmns_tests.cpp (2)
1131-1142: LGTM: Comprehensive legacy diff setup for migration testing.The test correctly initializes a legacy diff with real BLS keys, ban state, and multiple fields to validate the serialization roundtrip and format conversion. The explicit checks ensure pubKeyOperator starts as non-legacy and ban state is properly initialized.
1150-1166: LGTM: Thorough validation of legacy-to-new format conversion.The assertions correctly verify:
- Original pubKeyOperator scheme is preserved before deserialization
- Deserialized legacy format forces pubKeyOperator to legacy scheme as expected
- All fields (nVersion, ban height, pubKeyOperator) are correctly mapped and preserved during conversion
- pubKeyOperator scheme can be toggled post-conversion
This comprehensively validates the migration path.
src/evo/dmnstate.h (2)
386-409: LGTM: Correct legacy format deserialization.The
Unserializemethod correctly handles legacy format:
- Forces pubKeyOperator to legacy BLS scheme (line 396), as documented in the comment "We'll set proper scheme later in MigrateLegacyDiffs()"
- Uses non-extended addresses for netInfo (line 401), consistent with legacy format constraints
- Appropriately omits nVersion validation that exists in the new format, since legacy format had nVersion as the last field (0x40000), not first
412-442: LGTM: Clean field bit mapping for legacy-to-new conversion.The
ToNewFormatmethod correctly:
- Copies the state including pubKeyOperator with its legacy scheme flag
- Maps all 19 field bits from legacy positions to new positions
- Uses macros to avoid repetition while maintaining clarity
The conversion preserves all state data while translating field bitmasks to the new layout.
…ffLegacy, only deser We don't use ser for anything (yet)
Improvements: - Compares against original values instead of hardcoded constants - Verifies ban height round-trips correctly - Verifies BLS key value is identical after round-trip (using Get() to force initialization) - Verifies string representation matches
UdjinM6
force-pushed
the
fix_CDeterministicMNStateDiffLegacy_deser
branch
from
bd8bbcf to
4c8ae8e
Compare
1 participant
Issue being fixed or feature implemented
Current implementation is too limited for proper testing.
What was done?
Split CDeterministicMNStateDiffLegacy ser/deser. Improve tests to confirm we can indeed deser
pubKeyOperatoras legacy BLS and set correct scheme later viaSetLegacy(). Also check a few additional things while at it.How Has This Been Tested?
Run tests
Breaking Changes
n/a
Checklist: