perf: use cache for SML #6738
perf: use cache for SML #6738
Conversation
WalkthroughThis change set refactors and modularizes the handling of simplified masternode list diffs (SML diffs) in the codebase. It introduces new files Estimated code review effort4 (60–120 minutes) 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 2
🧹 Nitpick comments (4)
src/llmq/snapshot.cpp (1)
7-10: Drop redundant include to avoid double parsing
evo/smldiff.halready transitively includesevo/simplifiedmns.h. Keeping both headers here brings no benefit and slightly slows down compilation.-#include <evo/simplifiedmns.h> -#include <evo/smldiff.h> +#include <evo/smldiff.h> // smldiff pulls in simplifiedmns when neededsrc/evo/deterministicmns.cpp (1)
633-633: Fix typo in comment.- newList.to_sml(); // to fullfill cache of SML + newList.to_sml(); // to fulfill cache of SMLsrc/evo/smldiff.cpp (2)
133-139: Consider optimizing the comparison logicCreating temporary
CSimplifiedMNListEntryobjects for comparison may be inefficient, especially when processing large masternode lists. Additionally, the condition spanning lines 135-137 is complex and could be more readable.Consider extracting the comparison logic into a helper function:
- CSimplifiedMNListEntry sme1{toPtr.to_sml_entry()}; - CSimplifiedMNListEntry sme2(fromPtr->to_sml_entry()); - if ((sme1 != sme2) || (extended && (sme1.scriptPayout != sme2.scriptPayout || - sme1.scriptOperatorPayout != sme2.scriptOperatorPayout))) { - diffRet.mnList.push_back(std::move(sme1)); - } + auto needsUpdate = [&extended](const auto& from, const auto& to) { + if (from.to_sml_entry() != to.to_sml_entry()) return true; + if (!extended) return false; + const auto& fromEntry = from.to_sml_entry(); + const auto& toEntry = to.to_sml_entry(); + return fromEntry.scriptPayout != toEntry.scriptPayout || + fromEntry.scriptOperatorPayout != toEntry.scriptOperatorPayout; + }; + if (needsUpdate(*fromPtr, toPtr)) { + diffRet.mnList.push_back(toPtr.to_sml_entry()); + }
180-180: Fix typo in error message- errorRet = strprintf("base block %s is higher then block %s", baseBlockHash.ToString(), blockHash.ToString()); + errorRet = strprintf("base block %s is higher than block %s", baseBlockHash.ToString(), blockHash.ToString());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/Makefile.am(2 hunks)src/evo/cbtx.cpp(4 hunks)src/evo/cbtx.h(2 hunks)src/evo/core_write.cpp(2 hunks)src/evo/deterministicmns.cpp(9 hunks)src/evo/deterministicmns.h(5 hunks)src/evo/simplifiedmns.cpp(2 hunks)src/evo/simplifiedmns.h(3 hunks)src/evo/smldiff.cpp(1 hunks)src/evo/smldiff.h(1 hunks)src/evo/specialtxman.cpp(1 hunks)src/llmq/snapshot.cpp(1 hunks)src/llmq/snapshot.h(1 hunks)src/net_processing.cpp(1 hunks)src/node/miner.cpp(1 hunks)src/rpc/evo.cpp(1 hunks)src/test/evo_simplifiedmns_tests.cpp(3 hunks)src/test/util/setup_common.cpp(1 hunks)test/lint/lint-circular-dependencies.py(2 hunks)
🧰 Additional context used
🧠 Learnings (19)
📓 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: kwvg
PR: dashpay/dash#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: 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.
src/llmq/snapshot.cpp (1)
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.
src/test/util/setup_common.cpp (3)
Learnt from: kwvg
PR: dashpay/dash#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: 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.
src/node/miner.cpp (2)
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: kwvg
PR: dashpay/dash#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.
src/llmq/snapshot.h (1)
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.
src/rpc/evo.cpp (1)
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.
src/evo/specialtxman.cpp (2)
Learnt from: kwvg
PR: dashpay/dash#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: 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.
src/evo/core_write.cpp (2)
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.
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.
src/evo/cbtx.h (3)
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.
Learnt from: kwvg
PR: dashpay/dash#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: 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.
src/Makefile.am (2)
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.
src/test/evo_simplifiedmns_tests.cpp (2)
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.
Learnt from: kwvg
PR: dashpay/dash#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.
src/evo/deterministicmns.h (1)
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.
src/evo/smldiff.h (1)
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.
src/net_processing.cpp (1)
undefined
<retrieved_learning>
Learnt from: knst
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.
</retrieved_learning>
src/evo/simplifiedmns.h (5)
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.
Learnt from: kwvg
PR: dashpay/dash#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: 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: kwvg
PR: dashpay/dash#6504
File: src/llmq/quorums.cpp:224-224
Timestamp: 2024-12-29T17:43:41.755Z
Learning: The `CQuorumManager` is fully initialized by `LLMQContext`, addressing any concerns about the manager’s initialization sequence.
Learnt from: kwvg
PR: dashpay/dash#6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
src/evo/cbtx.cpp (3)
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.
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: kwvg
PR: dashpay/dash#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.
src/evo/smldiff.cpp (1)
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.
src/evo/simplifiedmns.cpp (2)
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.
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.
src/evo/deterministicmns.cpp (2)
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.
Learnt from: knst
PR: dashpay/dash#6658
File: src/evo/creditpool.cpp:177-185
Timestamp: 2025-05-05T12:45:44.781Z
Learning: The GetAncestor() function in CBlockIndex safely handles negative heights by returning nullptr rather than asserting, making it safe to call with potentially negative values.
🧬 Code Graph Analysis (3)
src/node/miner.cpp (1)
src/evo/cbtx.cpp (2)
CalcCbTxMerkleRootMNList(92-136)CalcCbTxMerkleRootMNList(92-92)
src/evo/specialtxman.cpp (1)
src/evo/cbtx.cpp (2)
CheckCbTxMerkleRoots(50-90)CheckCbTxMerkleRoots(50-52)
src/evo/core_write.cpp (8)
src/evo/mnhftx.h (2)
nodiscard(50-57)obj(52-52)src/llmq/commitment.h (3)
nodiscard(74-80)nodiscard(122-138)obj(124-124)src/evo/deterministicmns.cpp (3)
ToJson(54-73)ToJson(54-54)obj(56-56)src/evo/dmnstate.cpp (4)
ToJson(38-67)ToJson(38-38)ToJson(69-132)ToJson(69-69)src/evo/simplifiedmns.h (1)
extended(90-90)src/evo/smldiff.h (1)
extended(88-88)src/util/underlying.h (1)
ToUnderlying(10-10)src/core_write.cpp (2)
EncodeHexTx(153-158)EncodeHexTx(153-153)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build container / Build container
- GitHub Check: Build slim container / Build container
🔇 Additional comments (32)
src/llmq/snapshot.h (1)
8-8: Include update is correctThe header now needs the diff definitions that live in
evo/smldiff.h, and the switch looks good.
No further action required.src/net_processing.cpp (1)
62-65: Confirm new header fully covers previous dependencies
<evo/smldiff.h>replaces<evo/simplifiedmns.h>. Please double-check that every symbol fromsimplifiedmns.hused innet_processing.cpp(e.g.CSimplifiedMNList,ToJson, diff builders, etc.) is now exported bysmldiff.h; otherwise the compile will break silently at link or include time.src/test/util/setup_common.cpp (1)
509-509: LGTM! Interface update correctly implemented.The change from constructing a temporary
CSimplifiedMNList(mn_list)to passingstd::move(mn_list)directly aligns with the function signature update and supports the performance improvements from the caching mechanism.src/evo/specialtxman.cpp (1)
196-196: LGTM! Function call updated to match new interface.The change to pass
mn_listdirectly instead of constructing a temporaryCSimplifiedMNListcorrectly aligns with the updatedCheckCbTxMerkleRootsfunction signature that now accepts aconst CDeterministicMNList¶meter.src/node/miner.cpp (1)
235-235: LGTM! Move semantics correctly applied for interface update.The change to use
std::move(mn_list)instead of constructing a temporaryCSimplifiedMNListis correct. It aligns with the updated function signature and supports the performance improvements from the internal caching mechanism.src/Makefile.am (1)
198-198: LGTM! New smldiff module correctly integrated into build system.The addition of
evo/smldiff.hto the core headers andevo/smldiff.cppto the node sources properly integrates the new simplified masternode list diff module into the build system.Also applies to: 470-470
src/test/evo_simplifiedmns_tests.cpp (3)
19-35: LGTM! Ownership model change is correctly implemented.The conversion to
std::unique_ptris properly handled with correct memory management and ownership transfer.
56-58: Pointer dereference is correct.The arrow operator correctly accesses the
CalcHash()method through the unique_ptr.
63-63: Proper move semantics for ownership transfer.The
std::movecorrectly transfers ownership of the vector of unique pointers to theCSimplifiedMNListconstructor.test/lint/lint-circular-dependencies.py (1)
37-37: Circular dependency updates correctly reflect module reorganization.The changes from
evo/simplifiedmnstoevo/smldiffaccurately represent the refactoring where diff functionality was moved to a new module.Also applies to: 40-40, 51-51, 56-56
src/evo/cbtx.h (2)
17-17: Forward declaration is appropriate.Using a forward declaration instead of a full include is good practice for reducing compilation dependencies.
69-72: Interface change aligns with caching architecture.The change from
CSimplifiedMNListtoCDeterministicMNListenables internal caching and avoids unnecessary conversions. The const reference ensures efficiency and prevents unintended modifications.src/evo/core_write.cpp (3)
11-12: Include statements are properly ordered.The includes for the simplified MN list modules are correctly placed with other evo module includes.
149-175: Well-structured JSON serialization for CSimplifiedMNListEntry.The implementation correctly serializes all fields, properly handles Evo-specific attributes, and uses the extended parameter appropriately for optional payout address information.
177-238: Comprehensive JSON serialization for CSimplifiedMNListDiff.The implementation correctly handles all diff components including MN lists, quorum changes, and chainlock signatures. The pre-DIP0003 compatibility consideration (line 219) is appropriately handled by not asserting the transaction type.
src/evo/deterministicmns.h (5)
33-34: Forward declarations are appropriate.Using forward declarations for the simplified MN list classes reduces compilation dependencies.
110-110: New conversion method follows established patterns.The
to_sml_entry()method provides a clean interface for converting deterministic MNs to simplified entries.
177-182: Verify thread safety of the mutable cache.The
m_cached_smlis mutable and can be modified in const methods liketo_sml(). IfCDeterministicMNListinstances are accessed from multiple threads, this could lead to data races.Please ensure that access to
CDeterministicMNListinstances is properly synchronized or document that they are not meant to be accessed concurrently.
218-218: Cache invalidation in deserialization is correct.Resetting the cache when deserializing ensures no stale data persists.
345-348: Cache invalidation logic in mutating methods is correctly implementedVerified that:
AddMNalways resetsm_cached_smltonullptrRemoveMNalways resetsm_cached_smltonullptrUpdateMNresetsm_cached_smltonullptrwhenever the simplified‐list entry actually changesNo further invalidation is needed.
src/evo/deterministicmns.cpp (5)
41-47: LGTM!The conversion from
CDeterministicMNtoCSimplifiedMNListEntrycorrectly extracts all necessary fields from the DMN state.
267-280: Well-implemented lazy caching mechanism.The
to_sml()method efficiently converts the deterministic MN list to a simplified MN list with proper caching. Good use ofshared_ptrandreserve()for performance optimization.
467-467: Correct cache invalidation.Properly clears the cached simplified MN list when adding a new masternode.
538-540: Smart cache invalidation optimization.Excellent optimization that only clears the cache when the simplified MN list entry actually changes, avoiding unnecessary cache rebuilds.
1109-1112: Good defensive programming.The guard against duplicate cache insertions prevents unnecessary memory usage and ensures cache consistency.
src/evo/cbtx.cpp (2)
51-52: Improved API design.The change from rvalue reference to const reference makes the API more flexible and avoids unnecessary object construction.
92-136: Well-implemented caching with shared pointers.The refactored caching mechanism correctly uses shared pointers for efficiency. The optimization to check pointer equality before value equality (line 107) is a nice touch that can save expensive comparisons.
src/evo/smldiff.h (1)
35-89: Well-designed P2P message classes.The
CGetSimplifiedMNListDiffandCSimplifiedMNListDiffclasses are properly structured with appropriate serialization methods and version handling. Good use of SERIALIZE_METHODS macro.src/evo/simplifiedmns.h (1)
16-17: Good modernization with unique_ptr usage.The refactoring to use
unique_ptrfor memory management and explicit constructor parameters improves the design by:
- Decoupling
CSimplifiedMNListEntryfromCDeterministicMN- Using RAII for automatic memory management
- Enabling move semantics for efficiency
Also applies to: 38-41, 101-101
src/evo/simplifiedmns.cpp (1)
16-34: Clean implementation with proper move semantics.Both constructors are well-implemented:
CSimplifiedMNListEntryconstructor properly initializes all members using the member initializer listCSimplifiedMNListconstructor efficiently takes ownership viastd::moveand maintains the sorted orderAlso applies to: 64-71
src/evo/smldiff.cpp (2)
89-90: No underflow risk: GetAncestor already guards negative heightsThe
GetAncestor(int height)implementation insrc/chain.cppchecks forheight < 0(andheight > nHeight) and returnsnullptr, so passing a negative value is safe and won’t underflow. No additional validation is needed.
51-51: No truncation risk: LLMQType is defined as uint8_tThe explicit cast to
uint8_ton line 51 is safe, sinceConsensus::LLMQTypeis declared as anenum class … : uint8_t, and all its values (0–255) fit within that range. You can ignore the truncation concern.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/evo/smldiff.cpp (1)
180-180: Grammar error in error message.There's a grammatical error in the error message: "higher then" should be "higher than".
- errorRet = strprintf("base block %s is higher then block %s", baseBlockHash.ToString(), blockHash.ToString()); + errorRet = strprintf("base block %s is higher than block %s", baseBlockHash.ToString(), blockHash.ToString());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/Makefile.am(2 hunks)src/evo/cbtx.cpp(4 hunks)src/evo/cbtx.h(2 hunks)src/evo/core_write.cpp(2 hunks)src/evo/deterministicmns.cpp(9 hunks)src/evo/deterministicmns.h(5 hunks)src/evo/simplifiedmns.cpp(2 hunks)src/evo/simplifiedmns.h(3 hunks)src/evo/smldiff.cpp(1 hunks)src/evo/smldiff.h(1 hunks)src/evo/specialtxman.cpp(1 hunks)src/llmq/snapshot.cpp(1 hunks)src/llmq/snapshot.h(1 hunks)src/net_processing.cpp(1 hunks)src/node/miner.cpp(1 hunks)src/rpc/evo.cpp(1 hunks)src/test/evo_simplifiedmns_tests.cpp(3 hunks)src/test/util/setup_common.cpp(1 hunks)test/lint/lint-circular-dependencies.py(2 hunks)
✅ Files skipped from review due to trivial changes (5)
- src/llmq/snapshot.cpp
- src/llmq/snapshot.h
- src/Makefile.am
- src/rpc/evo.cpp
- test/lint/lint-circular-dependencies.py
🚧 Files skipped from review as they are similar to previous changes (10)
- src/evo/specialtxman.cpp
- src/node/miner.cpp
- src/test/evo_simplifiedmns_tests.cpp
- src/test/util/setup_common.cpp
- src/net_processing.cpp
- src/evo/cbtx.h
- src/evo/core_write.cpp
- src/evo/deterministicmns.h
- src/evo/deterministicmns.cpp
- src/evo/cbtx.cpp
🧰 Additional context used
🧠 Learnings (5)
📓 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: kwvg
PR: dashpay/dash#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: 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.
src/evo/simplifiedmns.cpp (2)
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.
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.
src/evo/smldiff.h (1)
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.
src/evo/simplifiedmns.h (5)
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.
Learnt from: kwvg
PR: dashpay/dash#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: 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: kwvg
PR: dashpay/dash#6504
File: src/llmq/quorums.cpp:224-224
Timestamp: 2024-12-29T17:43:41.755Z
Learning: The `CQuorumManager` is fully initialized by `LLMQContext`, addressing any concerns about the manager’s initialization sequence.
Learnt from: kwvg
PR: dashpay/dash#6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
src/evo/smldiff.cpp (1)
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.
🧬 Code Graph Analysis (1)
src/evo/smldiff.h (4)
src/rpc/evo.cpp (3)
obj(1707-1707)EXCLUSIVE_LOCKS_REQUIRED(1573-1585)EXCLUSIVE_LOCKS_REQUIRED(1628-1644)src/evo/smldiff.cpp (2)
CSimplifiedMNListDiff(26-26)CSimplifiedMNListDiff(28-28)src/evo/simplifiedmns.h (1)
extended(90-90)src/net_processing.cpp (14)
EXCLUSIVE_LOCKS_REQUIRED(645-645)EXCLUSIVE_LOCKS_REQUIRED(646-646)EXCLUSIVE_LOCKS_REQUIRED(648-648)EXCLUSIVE_LOCKS_REQUIRED(649-649)EXCLUSIVE_LOCKS_REQUIRED(653-653)EXCLUSIVE_LOCKS_REQUIRED(660-660)EXCLUSIVE_LOCKS_REQUIRED(663-663)EXCLUSIVE_LOCKS_REQUIRED(713-713)EXCLUSIVE_LOCKS_REQUIRED(733-733)EXCLUSIVE_LOCKS_REQUIRED(753-753)EXCLUSIVE_LOCKS_REQUIRED(812-812)EXCLUSIVE_LOCKS_REQUIRED(814-814)EXCLUSIVE_LOCKS_REQUIRED(819-819)EXCLUSIVE_LOCKS_REQUIRED(821-821)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (10)
src/evo/simplifiedmns.cpp (2)
16-34: Constructor refactoring looks well-implemented.The change from taking a
CDeterministicMNreference to explicit parameters supports the new architecture whereCSimplifiedMNListEntryobjects are created via theto_sml_entry()method on deterministic MN objects. This decouples the simplified MN list from the deterministic MN list implementation details.
64-71: Updated constructor correctly handles unique pointer ownership.The constructor now properly takes ownership of the unique pointer vector and sorts entries by proRegTxHash. The move semantics are correctly applied to avoid unnecessary copying.
src/evo/simplifiedmns.h (2)
38-41: Constructor signature correctly updated.The explicit parameter constructor matches the implementation and supports the new conversion pattern from deterministic MN objects via
to_sml_entry()method.
101-101: Constructor properly handles unique pointer ownership.The move constructor for the vector of unique pointers is correctly declared to support the new ownership model where entries are managed via unique pointers.
src/evo/smldiff.h (3)
5-6: Header guards are correctly positioned.The header guards are properly placed at the beginning of the file before any includes, resolving the issue from the previous review.
91-94: Function declaration properly annotated with thread safety requirements.The
BuildSimplifiedMNListDifffunction correctly specifiesEXCLUSIVE_LOCKS_REQUIRED(::cs_main)which ensures callers must hold the main chain lock before calling this function that accesses blockchain state.
64-79: Serialization method correctly handles protocol version compatibility.The serialization logic properly handles different protocol versions and maintains backward compatibility with version ordering changes. The conditional serialization based on network flags and version checks follows the established patterns in the codebase.
src/evo/smldiff.cpp (3)
88-90: Potential integer underflow in block height calculation.The calculation
quorum->m_quorum_base_block_index->nHeight - quorum->qc->quorumIndex - 8could potentially underflow if the result becomes negative, leading to incorrect ancestor lookup.Consider adding a bounds check to ensure the calculation doesn't underflow:
+ if (quorum->m_quorum_base_block_index->nHeight < quorum->qc->quorumIndex + 8) { + LogPrintf("%s: ERROR! Invalid block height calculation for quorum %s\n", __func__, e.quorumHash.ToString()); + return false; + } const CBlockIndex* pWorkBaseBlockIndex = blockIndex->GetAncestor(quorum->m_quorum_base_block_index->nHeight - quorum->qc->quorumIndex - 8);
121-150: BuildSimplifiedDiff implementation is efficient and correct.The function efficiently compares two deterministic MN lists using the new
to_sml_entry()conversion method. The extended comparison logic properly handles script differences when the extended flag is set. This implementation supports the caching optimization goals by avoiding unnecessary conversions.
152-223: Main diff building function is well-structured.The
BuildSimplifiedMNListDifffunction properly orchestrates the diff building process with comprehensive validation, error handling, and proper lock assertions. The conditional inclusion of quorum chainlock info based on V20 deployment status is correctly implemented.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/evo/smldiff.cpp (2)
30-67: Add logging for better error diagnosis in BuildQuorumsDiff.The function correctly builds quorum diffs, but when
qc == nullptron line 59, it returns false without logging the reason. This could make debugging difficult.if (qc == nullptr) { + LogPrintf("%s: failed to get mined commitment for llmqType=%d, hash=%s\n", __func__, + ToUnderlying(llmqType), hash.ToString()); return false; }
152-223: Consider refactoring the main orchestration function.The
BuildSimplifiedMNListDifffunction is quite long and handles multiple responsibilities. While the logic is correct, it could benefit from being broken down into smaller, more focused functions for better maintainability.Consider extracting the validation logic (lines 160-182) into a separate function like
ValidateBlockIndicesto improve readability and testability.bool ValidateBlockIndices(const ChainstateManager& chainman, const uint256& baseBlockHash, const uint256& blockHash, const CBlockIndex*& baseBlockIndex, const CBlockIndex*& blockIndex, std::string& errorRet) { // Extract validation logic here }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/evo/cbtx.cpp(4 hunks)src/evo/deterministicmns.cpp(9 hunks)src/evo/deterministicmns.h(5 hunks)src/evo/simplifiedmns.cpp(2 hunks)src/evo/simplifiedmns.h(3 hunks)src/evo/smldiff.cpp(1 hunks)src/test/evo_simplifiedmns_tests.cpp(3 hunks)test/lint/lint-circular-dependencies.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/test/evo_simplifiedmns_tests.cpp
- src/evo/cbtx.cpp
- test/lint/lint-circular-dependencies.py
- src/evo/deterministicmns.h
- src/evo/deterministicmns.cpp
🧰 Additional context used
🧠 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: kwvg
PR: dashpay/dash#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: 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.
src/evo/smldiff.cpp (1)
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.
src/evo/simplifiedmns.h (4)
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.
Learnt from: kwvg
PR: dashpay/dash#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: 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: kwvg
PR: dashpay/dash#6504
File: src/llmq/quorums.cpp:224-224
Timestamp: 2024-12-29T17:43:41.755Z
Learning: The `CQuorumManager` is fully initialized by `LLMQContext`, addressing any concerns about the manager’s initialization sequence.
src/evo/simplifiedmns.cpp (2)
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.
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.
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: mac-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- 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
🔇 Additional comments (10)
src/evo/simplifiedmns.h (4)
16-17: New includes added for updated functionality.The addition of
#include <memory>and#include <vector>is necessary for the new constructor that usesstd::unique_ptrand move semantics. This is a clean addition that supports the performance improvements.
19-19: Forward declaration for UniValue added.The forward declaration of
UniValueis correctly added to support theToJsonmethod declaration on line 90.
38-41: Constructor signature decoupled from CDeterministicMN.The constructor change from taking a
CDeterministicMNreference to explicit parameters is a good design improvement. This decouples theCSimplifiedMNListEntryfrom theCDeterministicMNclass, making the code more modular and aligned with the caching strategy mentioned in the PR objectives.
101-101: Move constructor with unique_ptr for performance.The constructor now takes
std::vector<std::unique_ptr<CSimplifiedMNListEntry>>&&instead of copying entries by value. This is an excellent performance optimization that aligns with the PR's performance goals, reducing unnecessary copying and memory allocations.src/evo/simplifiedmns.cpp (3)
7-8: Updated includes for remaining functionality.The new includes (
clientversion.h,consensus/merkle.h,version.h) are necessary for the remaining functionality after the diff-related code was moved tosmldiff.cpp. The includes are minimal and appropriate.Also applies to: 14-14
16-34: Constructor implementation with explicit parameters.The constructor implementation correctly initializes all member variables with the explicit parameters. This implementation is clean and maintains the same functionality while decoupling from
CDeterministicMN.
64-71: Efficient move constructor implementation.The constructor efficiently moves the vector of unique pointers and sorts them in-place. The use of
std::moveavoids unnecessary copying, and the sorting logic correctly compares theproRegTxHashvalues through the unique pointers.src/evo/smldiff.cpp (3)
69-119: Complex but well-structured quorum chainlock logic.The
BuildQuorumChainlockInfofunction handles complex logic for grouping quorums and managing chainlock signatures. The implementation correctly handles edge cases like missing quorums and merges duplicate signatures. The use ofstd::multimapandstd::setoperations is appropriate for the task.
121-150: Efficient diff building with proper conversion.The
BuildSimplifiedDifffunction efficiently compares two deterministic masternode lists using the newto_sml_entry()conversion method. The logic correctly identifies additions, modifications, and deletions. The extended comparison for payout scripts is properly handled.
205-211: Verify block reading performance impact.The
ReadBlockFromDiskoperation is necessary for extracting the coinbase transaction, but it's a potentially expensive I/O operation. Ensure this aligns with the performance goals of the PR.Since this is a performance-focused PR, please verify that the block reading doesn't negate the performance gains from the caching mechanism. The TODO comment on line 205 suggests storing the coinbase TX in CBlockIndex might be a future optimization.
|
PastaPastaPasta@fc87b86 PastaPastaPasta@a922815 Please evaluate |
|
@PastaPastaPasta C++20 supports automic over shared_ptr Such as should have exactly same effect. Concerns? |
!!!!! xcode! so sad. Thanks @PastaPastaPasta for your implementation with mutex, I am gonna take it as it is. |
|
This pull request has conflicts, please rebase. |
Force-pushed to resolve conflicts over |
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
perf-sml
branch
2 times, most recently
from
96a7119 to
0fe1973
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/evo/smldiff.h (2)
47-47: Consider using a scoped enum for version constants.The
CURRENT_VERSIONconstant usesuint16_twhich is fine, but using a scoped enum would provide better type safety and prevent accidental misuse.-static constexpr uint16_t CURRENT_VERSION = 1; +enum class Version : uint16_t { + CURRENT = 1 +}; +static constexpr uint16_t CURRENT_VERSION = static_cast<uint16_t>(Version::CURRENT);
62-62: Verify the choice of std::map for quorumsCLSigs.Using
std::mapforquorumsCLSigsmay not be optimal for performance. Consider ifstd::unordered_mapwould be more appropriate given that this is likely used for lookups rather than ordered iteration.-std::map<CBLSSignature, std::set<uint16_t>> quorumsCLSigs; +std::unordered_map<CBLSSignature, std::set<uint16_t>> quorumsCLSigs;Note: This change would require ensuring
CBLSSignaturehas a proper hash function.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src/Makefile.am(2 hunks)src/evo/cbtx.h(1 hunks)src/evo/core_write.cpp(2 hunks)src/evo/deterministicmns.cpp(9 hunks)src/evo/deterministicmns.h(6 hunks)src/evo/simplifiedmns.cpp(4 hunks)src/evo/simplifiedmns.h(3 hunks)src/evo/smldiff.cpp(1 hunks)src/evo/smldiff.h(1 hunks)src/evo/specialtxman.cpp(1 hunks)src/llmq/snapshot.cpp(1 hunks)src/llmq/snapshot.h(1 hunks)src/net_processing.cpp(1 hunks)src/node/miner.cpp(1 hunks)src/rpc/evo.cpp(1 hunks)src/test/evo_simplifiedmns_tests.cpp(3 hunks)src/test/util/setup_common.cpp(1 hunks)test/lint/lint-circular-dependencies.py(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/llmq/snapshot.h
- src/llmq/snapshot.cpp
- src/rpc/evo.cpp
🚧 Files skipped from review as they are similar to previous changes (12)
- src/evo/cbtx.h
- src/test/util/setup_common.cpp
- src/evo/specialtxman.cpp
- src/Makefile.am
- test/lint/lint-circular-dependencies.py
- src/node/miner.cpp
- src/test/evo_simplifiedmns_tests.cpp
- src/net_processing.cpp
- src/evo/core_write.cpp
- src/evo/deterministicmns.h
- src/evo/smldiff.cpp
- src/evo/deterministicmns.cpp
🧰 Additional context used
📓 Path-based instructions (2)
`src/**/*.{cpp,h,cc,cxx,hpp}`: Dash Core codebase must be written in C++20 and r...
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core 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
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
src/evo/smldiff.hsrc/evo/simplifiedmns.hsrc/evo/simplifiedmns.cpp
`src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}`: Masternode lists must use immutable data structures (Immer library) for thread safety
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}: Masternode lists must use immutable data structures (Immer library) for thread safety
📄 Source: CodeRabbit Inference Engine (CLAUDE.md)
List of files the instruction was applied to:
src/evo/smldiff.hsrc/evo/simplifiedmns.hsrc/evo/simplifiedmns.cpp
🧠 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: kwvg
PR: dashpay/dash#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: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.331Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
src/evo/smldiff.h (3)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.331Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
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.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.331Z
Learning: Applies to src/evo/specialtx.h : Special transactions must use payload extensions as implemented in src/evo/specialtx.h
src/evo/simplifiedmns.h (6)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.331Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
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.
Learnt from: kwvg
PR: dashpay/dash#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: 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: kwvg
PR: dashpay/dash#6504
File: src/llmq/quorums.cpp:224-224
Timestamp: 2024-12-29T17:43:41.755Z
Learning: The `CQuorumManager` is fully initialized by `LLMQContext`, addressing any concerns about the manager’s initialization sequence.
Learnt from: kwvg
PR: dashpay/dash#6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
src/evo/simplifiedmns.cpp (5)
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.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.331Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
Learnt from: kwvg
PR: dashpay/dash#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: 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: 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.
🧬 Code Graph Analysis (1)
src/evo/simplifiedmns.cpp (5)
src/evo/simplifiedmns.h (4)
CSimplifiedMNListEntry(38-38)CSimplifiedMNListEntry(39-43)CSimplifiedMNList(101-101)CSimplifiedMNList(104-104)src/validation.h (11)
state(187-189)state(187-187)state(653-656)state(680-682)state(693-693)state(700-700)state(705-705)state(710-710)state(757-757)state(758-758)state(767-767)src/evo/deterministicmns.h (5)
GUARDED_BY(158-158)GUARDED_BY(604-604)GUARDED_BY(605-605)LOCK(161-165)LOCK(623-626)src/evo/specialtxman.cpp (2)
GUARDED_BY(37-37)GUARDED_BY(38-38)src/evo/cbtx.cpp (3)
GUARDED_BY(76-76)GUARDED_BY(77-77)GUARDED_BY(78-78)
🔇 Additional comments (10)
src/evo/smldiff.h (2)
64-79: Approve the version-dependent serialization logic.The serialization method correctly handles different protocol versions and network serialization requirements. The version checks are well-structured and maintain backward compatibility.
91-94: Good use of exclusive locks annotation.The function signature correctly uses
EXCLUSIVE_LOCKS_REQUIRED(::cs_main)which ensures thread safety when accessing blockchain state.src/evo/simplifiedmns.h (3)
39-43: Constructor parameter list is well-structured.The new constructor signature is comprehensive and allows for explicit initialization of all member variables. This is better than the previous approach of extracting values from a CDeterministicMN object.
104-104: Good use of move semantics in constructor.The constructor using rvalue reference (
std::vector<std::unique_ptr<CSimplifiedMNListEntry>>&&) is appropriate for efficient transfer of ownership and avoids unnecessary copying.
110-111: Shared pointer parameter enables efficient caching.Changing the function signature to accept
std::shared_ptr<const CSimplifiedMNList>is excellent for enabling the caching mechanism described in the PR objectives. This allows multiple consumers to share the same cached SML instance without copying.src/evo/simplifiedmns.cpp (5)
19-37: Constructor implementation is correct and well-structured.The new constructor implementation properly initializes all member variables using the member initializer list, which is efficient and follows best practices.
68-75: Move semantics and sorting logic are well-implemented.The constructor correctly uses
std::moveto transfer ownership of the vector and sorts the entries by proRegTxHash for consistent ordering. The lambda comparator properly dereferences the unique_ptr for comparison.
107-109: Thread safety of static cache variables is well-handled.The static cache variables are properly protected by the
cached_mutexand use appropriate thread-safe types. The initialization ofcached_smlwithstd::make_shared<const CSimplifiedMNList>()ensures a valid initial state.
129-129: Cache update logic is correct.The cache update
cached_sml = smlcorrectly stores the shared_ptr for future comparisons, enabling efficient cache hits when the same SML instance is used again.
98-99: Function signature change supports the caching strategy.The change from rvalue reference to shared_ptr parameter aligns perfectly with the caching mechanism and enables efficient sharing of SML instances across multiple callers.
| @@ -107,80 +99,15 @@ class CSimplifiedMNList | |||
| std::vector<std::unique_ptr<CSimplifiedMNListEntry>> mnList; | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider compliance with immutable data structures requirement.
The coding guidelines specify that files in src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} must use immutable data structures (Immer library) for thread safety. The current implementation uses std::vector<std::unique_ptr<CSimplifiedMNListEntry>> which is mutable.
Based on the retrieved learning about immutable data structures requirement, please verify whether the current approach of using std::vector<std::unique_ptr<CSimplifiedMNListEntry>> meets the thread safety requirements, or if it should be replaced with an immutable data structure from the Immer library.
🏁 Script executed:
#!/bin/bash
# Check if Immer library is used elsewhere in the codebase for similar data structures
rg -A 5 -B 5 "immer::" src/evo/ src/masternode/Length of output: 3356
Replace mutable MN list with Immer immutable vector
Per our thread-safety guidelines for src/evo/, masternode lists must use Immer immutable containers instead of std::vector. In src/evo/simplifiedmns.h at line 99, the declaration:
std::vector<std::unique_ptr<CSimplifiedMNListEntry>> mnList;should be changed to use an Immer vector (and shared, const-qualified pointers for immutability), for example:
- #include <vector>
+ #include <immer/vector.hpp>
- std::vector<std::unique_ptr<CSimplifiedMNListEntry>> mnList;
+ immer::vector<std::shared_ptr<const CSimplifiedMNListEntry>> mnList;• Add #include <immer/vector.hpp> at the top of the file.
• Adjust construction and access sites accordingly to call Immer’s persistent APIs.
🤖 Prompt for AI Agents
In src/evo/simplifiedmns.h at line 99, replace the mutable std::vector of
unique_ptr with an Immer immutable vector holding shared, const-qualified
pointers to CSimplifiedMNListEntry. Add #include <immer/vector.hpp> at the top
of the file. Update all code that constructs, modifies, or accesses mnList to
use Immer's persistent vector APIs and shared_ptr<const CSimplifiedMNListEntry>
to ensure thread-safe immutability.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
mnList is never changed once CSimplifiedmnlist is initialized. It's also old code, std::vector always has been here
There was a problem hiding this comment.
I'd add here const, but drserialization will not work then. So, I keep it with no changes
src/evo/simplifiedmns.cpp
Outdated
| static Mutex cached_mutex; | ||
| static CSimplifiedMNList smlCached GUARDED_BY(cached_mutex); | ||
| static std::shared_ptr<const CSimplifiedMNList> cached_sml GUARDED_BY(cached_mutex){ | ||
| std::make_shared<const CSimplifiedMNList>()}; | ||
| static uint256 merkleRootCached GUARDED_BY(cached_mutex); | ||
| static bool mutatedCached GUARDED_BY(cached_mutex){false}; | ||
|
|
||
| LOCK(cached_mutex); | ||
| if (sml == smlCached) { | ||
| if (sml == cached_sml || *sml == *cached_sml) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential issue with cache comparison logic.
The cache comparison on line 113 has two conditions: sml == cached_sml || *sml == *cached_sml. The first condition (pointer equality) is efficient, but the second condition (value equality) could be expensive as it requires deep comparison of the entire simplified masternode list. This might negate some of the performance benefits of caching.
Consider if the expensive deep comparison *sml == *cached_sml is necessary. If the shared_ptr approach is working correctly, pointer equality should be sufficient in most cases. If deep comparison is needed, consider adding a cheaper hash-based comparison first:
-if (sml == cached_sml || *sml == *cached_sml) {
+if (sml == cached_sml) {
merkleRootRet = merkleRootCached;
if (mutatedCached) {
return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "mutated-cached-calc-cb-mnmerkleroot");
}
return true;
}Or if value comparison is essential, add a hash check:
-if (sml == cached_sml || *sml == *cached_sml) {
+if (sml == cached_sml ||
+ (sml->CalcMerkleRoot() == cached_sml->CalcMerkleRoot() && *sml == *cached_sml)) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static Mutex cached_mutex; | |
| static CSimplifiedMNList smlCached GUARDED_BY(cached_mutex); | |
| static std::shared_ptr<const CSimplifiedMNList> cached_sml GUARDED_BY(cached_mutex){ | |
| std::make_shared<const CSimplifiedMNList>()}; | |
| static uint256 merkleRootCached GUARDED_BY(cached_mutex); | |
| static bool mutatedCached GUARDED_BY(cached_mutex){false}; | |
| LOCK(cached_mutex); | |
| if (sml == smlCached) { | |
| if (sml == cached_sml || *sml == *cached_sml) { | |
| LOCK(cached_mutex); | |
| - if (sml == cached_sml || *sml == *cached_sml) { | |
| + if (sml == cached_sml || | |
| + (sml->CalcMerkleRoot() == cached_sml->CalcMerkleRoot() && *sml == *cached_sml)) { |
🤖 Prompt for AI Agents
In src/evo/simplifiedmns.cpp around lines 106 to 113, the cache comparison uses
both pointer equality and a deep value equality check, which can be expensive
and reduce caching performance. To fix this, remove the deep comparison *sml ==
*cached_sml or replace it with a cheaper hash-based comparison before doing a
full deep comparison. This ensures the cache check remains efficient by relying
primarily on pointer equality or a quick hash check to avoid costly deep
comparisons.
force-pushed due to lint failure |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/evo/simplifiedmns.cpp (1)
99-143: Cache comparison logic needs optimization.The cache comparison on line 114 has both pointer equality and expensive deep comparison. This addresses a previous review comment but the deep comparison
*sml == *cached_smlcould still be expensive and negate caching benefits.Consider removing the expensive deep comparison and relying primarily on pointer equality, or add a cheaper hash-based comparison first:
- if (sml == cached_sml || *sml == *cached_sml) { + if (sml == cached_sml) { merkleRootRet = merkleRootCached; if (mutatedCached) { return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "mutated-cached-calc-cb-mnmerkleroot"); } return true; }If value comparison is essential, add a hash check first:
- if (sml == cached_sml || *sml == *cached_sml) { + if (sml == cached_sml || + (sml->CalcMerkleRoot() == cached_sml->CalcMerkleRoot() && *sml == *cached_sml)) {
🧹 Nitpick comments (1)
src/evo/smldiff.h (1)
8-18: Include optimization opportunity.Several includes may not be directly used in this header file. Consider moving non-essential includes to the implementation file (
smldiff.cpp) to reduce compilation dependencies.For example, includes like
<clientversion.h>,<key_io.h>,<logging.h>,<util/time.h>, and<version.h>are likely only needed in the implementation file.-#include <bls/bls.h> -#include <evo/dmn_types.h> -#include <evo/netinfo.h> -#include <evo/providertx.h> -#include <merkleblock.h> -#include <netaddress.h> -#include <pubkey.h> -#include <sync.h> -#include <threadsafety.h> +#include <bls/bls.h> +#include <evo/dmn_types.h> +#include <merkleblock.h> +#include <sync.h> +#include <threadsafety.h>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/Makefile.am(2 hunks)src/evo/cbtx.h(1 hunks)src/evo/core_write.cpp(2 hunks)src/evo/deterministicmns.cpp(9 hunks)src/evo/deterministicmns.h(6 hunks)src/evo/simplifiedmns.cpp(4 hunks)src/evo/simplifiedmns.h(3 hunks)src/evo/smldiff.cpp(1 hunks)src/evo/smldiff.h(1 hunks)src/evo/specialtxman.cpp(1 hunks)src/llmq/snapshot.cpp(1 hunks)src/llmq/snapshot.h(1 hunks)src/net_processing.cpp(1 hunks)src/node/miner.cpp(1 hunks)src/rpc/evo.cpp(1 hunks)src/test/evo_deterministicmns_tests.cpp(4 hunks)src/test/evo_simplifiedmns_tests.cpp(3 hunks)src/test/util/setup_common.cpp(1 hunks)test/lint/lint-circular-dependencies.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/llmq/snapshot.h
- src/llmq/snapshot.cpp
🚧 Files skipped from review as they are similar to previous changes (15)
- src/rpc/evo.cpp
- src/node/miner.cpp
- src/Makefile.am
- src/evo/specialtxman.cpp
- src/test/util/setup_common.cpp
- src/evo/cbtx.h
- src/net_processing.cpp
- test/lint/lint-circular-dependencies.py
- src/test/evo_simplifiedmns_tests.cpp
- src/evo/deterministicmns.cpp
- src/evo/core_write.cpp
- src/test/evo_deterministicmns_tests.cpp
- src/evo/deterministicmns.h
- src/evo/smldiff.cpp
- src/evo/simplifiedmns.h
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
🧠 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: kwvg
PR: dashpay/dash#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: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
src/evo/smldiff.h (3)
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
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.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/evo/specialtx.h : Special transactions must use payload extensions as implemented in src/evo/specialtx.h
src/evo/simplifiedmns.cpp (4)
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.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-10T16:26:00.371Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
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: kwvg
PR: dashpay/dash#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.
🧬 Code Graph Analysis (1)
src/evo/simplifiedmns.cpp (4)
src/evo/simplifiedmns.h (4)
CSimplifiedMNListEntry(39-39)CSimplifiedMNListEntry(40-44)CSimplifiedMNList(102-102)CSimplifiedMNList(105-105)src/validation.h (11)
state(187-189)state(187-187)state(653-656)state(680-682)state(693-693)state(700-700)state(705-705)state(710-710)state(757-757)state(758-758)state(767-767)src/evo/deterministicmns.h (5)
GUARDED_BY(158-158)GUARDED_BY(604-604)GUARDED_BY(605-605)LOCK(161-165)LOCK(623-626)src/evo/cbtx.cpp (3)
GUARDED_BY(76-76)GUARDED_BY(77-77)GUARDED_BY(78-78)
⏰ 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). (6)
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (6)
src/evo/smldiff.h (2)
91-94: Thread safety annotation is appropriate.The
EXCLUSIVE_LOCKS_REQUIRED(::cs_main)annotation correctly indicates that this function requires the main chain lock, which is essential for blockchain-related operations.
64-79: Add tests for CSimplifiedMNListDiff serialization branchingI didn’t find any existing unit tests for
CSimplifiedMNListDiffin the codebase, nor coverage of the version-dependent serialization paths (MNLISTDIFF_VERSION_ORDERandMNLISTDIFF_CHAINLOCKS_PROTO_VERSION). Please add or verify tests that:
- Cover the branch where
nVersionis written before the main fields (s.GetVersion() >= MNLISTDIFF_VERSION_ORDER).- Cover the alternate branch where
nVersionis written after the Merkle tree but before further fields (s.GetVersion() >= BLS_SCHEME_PROTO_VERSION && < MNLISTDIFF_VERSION_ORDER).- Cover the conditional serialization of
quorumsCLSigs(s.GetVersion() >= MNLISTDIFF_CHAINLOCKS_PROTO_VERSION).- Perform round-trip serialize→deserialize checks for each protocol version boundary.
File under review:
• src/evo/smldiff.h lines 64–79src/evo/simplifiedmns.cpp (4)
20-39: Constructor refactoring improves modularity.The change from accepting a
CDeterministicMNreference to explicit parameters is a good architectural improvement. This reduces coupling between the simplified and deterministic masternode representations.
69-76: Constructor change to unique_ptr vector is correct.The modification to accept
std::vector<std::unique_ptr<CSimplifiedMNListEntry>>&&aligns with modern C++ practices and provides clear ownership semantics.
108-109: Thread safety concern with static cache variables.The static cache variables are protected by a mutex, but the
gsl::not_nullinitialization pattern may not be thread-safe during static initialization.Please verify that the static initialization of
cached_smlis thread-safe. Consider usingstd::call_onceor a thread-safe singleton pattern:static std::once_flag init_flag; static gsl::not_null<std::shared_ptr<const CSimplifiedMNList>> cached_sml GUARDED_BY(cached_mutex); static uint256 merkleRootCached GUARDED_BY(cached_mutex); static bool mutatedCached GUARDED_BY(cached_mutex){false}; std::call_once(init_flag, []() { cached_sml = std::make_shared<const CSimplifiedMNList>(); });
7-18: I need to inspect howsrc/evo/simplifiedmns.cppdefines and uses standard containers for masternode lists to determine if they’re a thread-safety concern. Let me retrieve anystd::vector,std::map, or similar usages in that file.
|
This pull request has conflicts, please rebase. |
… evo/smldiff This commit is to prevent multiple circular dependencies over llmq/ module because evo/simplifiedmns depends on llmq/blockprocessor, llmq/commitment, llmq/quorums, but they includes evo/deterministicmns. Once evo/deterministicmns knows about simplifiedmns it causes 10 more loops over llmq/* and evo/deterministicmns
Shared_ptr to SML is preserved between multiple CDeterministicMNList and it helps to avoid building SML from a scratch; also it helps to avoid comparision of SML item-by-item.
Add private helper methods InvalidateSMLCache() and InvalidateSMLCacheIfChanged() to centralize cache invalidation logic and reduce code duplication. This improves maintainability by: - Centralizing cache invalidation logic in dedicated methods - Reducing code duplication across AddMN, RemoveMN, UpdateMN, and Unserialize - Making cache invalidation patterns consistent and easier to maintain - Providing both unconditional and conditional invalidation helpers The conditional invalidation method preserves the optimization in UpdateMN where cache is only invalidated if the SML entry actually changed.
Add extensive unit tests to verify the SML caching functionality: - Test basic cache functionality and cache hits - Test cache invalidation on AddMN operations - Test cache invalidation on RemoveMN operations - Test conditional cache invalidation on UpdateMN operations - Test cache behavior with copy constructor and assignment operator These tests ensure the cache works correctly and efficiently, validating: - Cache is properly shared when no changes occur - Cache is invalidated when MN list changes - Cache preserves optimization in UpdateMN when SML entry doesn't change - Cache is properly copied/shared during object copying This provides comprehensive coverage of the caching mechanism's correctness.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/evo/simplifiedmns.h (1)
100-100: Note: Immutable data structures guideline acknowledged.While the coding guidelines specify using Immer library for thread safety in
src/{masternode,evo}/**, the author has clarified thatmnListis never modified after initialization and represents legacy code. Given the performance focus of this PR and the author's explanation, this approach is acceptable for now.src/evo/simplifiedmns.cpp (1)
114-114: Consider the performance impact of deep comparison in cache logic.The cache comparison
sml == cached_sml || *sml == *cached_smlincludes a potentially expensive deep comparison. While pointer equality is fast, the value equality check could negate caching benefits when objects differ by pointer but are identical in content.Consider if the deep comparison is necessary given the shared_ptr caching approach, or add a cheaper hash-based check first.
- if (sml == cached_sml || *sml == *cached_sml) { + if (sml == cached_sml) { merkleRootRet = merkleRootCached; if (mutatedCached) { return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "mutated-cached-calc-cb-mnmerkleroot"); } return true; }
🧹 Nitpick comments (1)
src/evo/smldiff.h (1)
64-79: Review the complex serialization version logic.The serialization method has intricate version-dependent logic that could be error-prone. The version field
nVersionis written at different points depending on protocol versions, which may lead to deserialization issues if not handled carefully.Consider documenting the serialization format evolution or adding validation to ensure the version field is consistently handled across different protocol versions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/Makefile.am(2 hunks)src/evo/cbtx.h(1 hunks)src/evo/core_write.cpp(2 hunks)src/evo/deterministicmns.cpp(9 hunks)src/evo/deterministicmns.h(6 hunks)src/evo/simplifiedmns.cpp(4 hunks)src/evo/simplifiedmns.h(3 hunks)src/evo/smldiff.cpp(1 hunks)src/evo/smldiff.h(1 hunks)src/evo/specialtxman.cpp(1 hunks)src/llmq/snapshot.cpp(1 hunks)src/llmq/snapshot.h(1 hunks)src/net_processing.cpp(1 hunks)src/node/miner.cpp(1 hunks)src/rpc/evo.cpp(1 hunks)src/test/evo_deterministicmns_tests.cpp(4 hunks)src/test/evo_simplifiedmns_tests.cpp(3 hunks)src/test/util/setup_common.cpp(1 hunks)test/lint/lint-circular-dependencies.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/llmq/snapshot.h
- src/llmq/snapshot.cpp
🚧 Files skipped from review as they are similar to previous changes (14)
- src/evo/cbtx.h
- src/node/miner.cpp
- src/test/util/setup_common.cpp
- src/Makefile.am
- src/evo/specialtxman.cpp
- src/test/evo_simplifiedmns_tests.cpp
- test/lint/lint-circular-dependencies.py
- src/net_processing.cpp
- src/rpc/evo.cpp
- src/evo/core_write.cpp
- src/evo/deterministicmns.cpp
- src/evo/deterministicmns.h
- src/test/evo_deterministicmns_tests.cpp
- src/evo/smldiff.cpp
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
src/{masternode,evo}/**
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
🧠 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: kwvg
PR: dashpay/dash#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: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T15:51:47.798Z
Learning: Applies to src/{masternode,evo}/** : Masternode lists must use immutable data structures (Immer library) for thread safety
src/evo/simplifiedmns.cpp (4)
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.
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: kwvg
PR: dashpay/dash#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: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T15:51:47.798Z
Learning: Applies to src/{masternode,evo}/** : Masternode lists must use immutable data structures (Immer library) for thread safety
src/evo/simplifiedmns.h (4)
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.
Learnt from: kwvg
PR: dashpay/dash#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: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T15:51:47.798Z
Learning: Applies to src/{masternode,evo}/** : Masternode lists must use immutable data structures (Immer library) for thread safety
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.
src/evo/smldiff.h (2)
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.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T15:51:47.798Z
Learning: Applies to src/evo/specialtx.h : Special transactions must use payload extensions as defined in src/evo/specialtx.h
🧬 Code Graph Analysis (2)
src/evo/simplifiedmns.cpp (1)
src/evo/simplifiedmns.h (1)
CSimplifiedMNList(97-109)
src/evo/simplifiedmns.h (2)
src/univalue/include/univalue.h (1)
UniValue(17-21)src/evo/simplifiedmns.cpp (8)
CSimplifiedMNListEntry(20-39)CSimplifiedMNList(69-76)CalcMerkleRoot(78-86)CalcMerkleRoot(78-78)rhs(88-97)rhs(88-88)CalcCbTxMerkleRootMNList(99-143)CalcCbTxMerkleRootMNList(99-100)
⏰ 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). (9)
- GitHub Check: linux64_tsan-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: linux64-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (7)
src/evo/simplifiedmns.h (3)
40-44: Approve the explicit constructor parameters.The constructor change from taking a
CDeterministicMNreference to explicit parameters improves modularity and reduces coupling between classes. This is a good design improvement.
105-105: Approve the use of rvalue reference for move semantics.The constructor taking
std::vector<std::unique_ptr<CSimplifiedMNListEntry>>&& smlEntriescorrectly uses move semantics to avoid unnecessary copying, which is good for performance.
111-112: Approve the use of gsl::not_null for improved safety.The function signature change to use
gsl::not_null<std::shared_ptr<const CSimplifiedMNList>>improves code safety by making the non-null requirement explicit at the type level.src/evo/simplifiedmns.cpp (4)
20-39: Approve the constructor implementation.The constructor correctly initializes all member variables with the provided parameters. The implementation is straightforward and matches the header declaration.
69-76: Approve the move semantics implementation.The constructor correctly uses
std::moveto transfer ownership of the vector elements, avoiding unnecessary copying. The sorting logic remains the same and is correct.
107-111: Approve the thread-safe caching implementation.The static caching variables are properly protected with mutex (
GUARDED_BY(cached_mutex)) and the use ofgsl::not_nullensures type safety. The initialization with a default empty list is appropriate.
99-100: Approve the function signature modernization.The updated function signature using
gsl::not_null<std::shared_ptr<const CSimplifiedMNList>>improves type safety and makes the caching mechanism explicit through the shared_ptr interface.
| bool BuildSimplifiedMNListDiff(CDeterministicMNManager& dmnman, const ChainstateManager& chainman, | ||
| const llmq::CQuorumBlockProcessor& qblockman, const llmq::CQuorumManager& qman, | ||
| const uint256& baseBlockHash, const uint256& blockHash, CSimplifiedMNListDiff& mnListDiffRet, | ||
| std::string& errorRet, bool extended = false) EXCLUSIVE_LOCKS_REQUIRED(::cs_main); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify thread safety of the BuildSimplifiedMNListDiff function.
The function signature correctly requires EXCLUSIVE_LOCKS_REQUIRED(::cs_main), which is good for thread safety. However, ensure that all callers properly acquire this lock before calling this function.
🏁 Script executed:
#!/bin/bash
# Description: Verify that all callers of BuildSimplifiedMNListDiff properly acquire cs_main lock
# Search for calls to BuildSimplifiedMNListDiff and check surrounding context
rg -A 10 -B 5 "BuildSimplifiedMNListDiff"Length of output: 14388
#!/bin/bash
set -e
echo "=== RPC getmnlistdiff context ==="
rg -n -C5 "getmnlistdiff" src/rpc/evo.cpp || true
echo
echo "=== RPC LOCK/assert usage ==="
rg -n "LOCK(" -n src/rpc/evo.cpp || true
rg -n "AssertLockHeld" -n src/rpc/evo.cpp || true
echo
echo "=== Snapshot BuildSimplifiedMNListDiff context ==="
rg -n -C5 "BuildSimplifiedMNListDiff" src/llmq/snapshot.cpp || true
echo
echo "=== Snapshot LOCK/assert usage ==="
rg -n "LOCK(" -n src/llmq/snapshot.cpp || true
rg -n "AssertLockHeld" -n src/llmq/snapshot.cpp || true#!/bin/bash
set -e
echo "=== rpc/evo.cpp: BuildSimplifiedMNListDiff calls ==="
rg -n -C5 "BuildSimplifiedMNListDiff" src/rpc/evo.cpp || true
echo
echo "=== rpc/evo.cpp: LOCK or AssertLockHeld ==="
rg -n "LOCK(cs_main)" src/rpc/evo.cpp || true
rg -n "AssertLockHeld" src/rpc/evo.cpp || true
echo
echo "=== llmq/snapshot.cpp: BuildSimplifiedMNListDiff calls ==="
rg -n -C5 "BuildSimplifiedMNListDiff" src/llmq/snapshot.cpp || true
echo
echo "=== llmq/snapshot.cpp: LOCK or AssertLockHeld ==="
rg -n "LOCK(cs_main)" src/llmq/snapshot.cpp || true
rg -n "AssertLockHeld" src/llmq/snapshot.cpp || trueEnsure cs_main is held in all callers of BuildSimplifiedMNListDiff
The signature enforces EXCLUSIVE_LOCKS_REQUIRED(::cs_main), but the following call sites don’t acquire or assert cs_main:
• src/rpc/evo.cpp
– Before invoking BuildSimplifiedMNListDiff, wrap in LOCK(cs_main) or add AssertLockHeld(::cs_main).
• src/llmq/snapshot.cpp
– In every branch (legacy and non-legacy), surround BuildSimplifiedMNListDiff calls with LOCK(cs_main) or add AssertLockHeld(::cs_main) at function entry.
Without this, chainman.ActiveChain() and LookupBlockIndex() may race. Add the missing locks/asserts to satisfy the EXCLUSIVE_LOCKS_REQUIRED contract.
🤖 Prompt for AI Agents
In src/rpc/evo.cpp and src/llmq/snapshot.cpp around the lines where
BuildSimplifiedMNListDiff is called, the function requires exclusive ownership
of the cs_main lock as indicated by EXCLUSIVE_LOCKS_REQUIRED(::cs_main). To fix
this, ensure that each call to BuildSimplifiedMNListDiff is either wrapped
inside a LOCK(cs_main) block or that the function containing the call asserts
the lock is held at entry using AssertLockHeld(::cs_main). This will prevent
race conditions on chainman.ActiveChain() and LookupBlockIndex() and satisfy the
locking contract.
There was a problem hiding this comment.
EXCLUSIVE_LOCKS_REQUIRED is enforced at compile time, on builds that support it like macOS. If this comment was true, this build would fail
There was a problem hiding this comment.
@PastaPastaPasta You're absolutely right, and thank you for the correction! I was mistaken in my analysis. EXCLUSIVE_LOCKS_REQUIRED is indeed a Clang thread safety annotation that's enforced at compile time on supported builds like macOS, not a runtime requirement that needs manual verification.
If there were actual locking violations with the BuildSimplifiedMNListDiff calls, the build would fail on those platforms with compile-time errors. Since the PR builds successfully, the locking is correct.
I appreciate you pointing this out - it's an important distinction between compile-time thread safety enforcement and runtime assertions.
(\ /)
( ._. )
o_(")(") *learning noted*
✏️ Learnings added
Learnt from: PastaPastaPasta
PR: dashpay/dash#6738
File: src/evo/smldiff.h:91-94
Timestamp: 2025-07-22T14:38:45.574Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
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
Learnt from: CR
PR: dashpay/dash#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
Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{crc32c,dashbls,gsl,immer,leveldb,minisketch,secp256k1,univalue}/** : Do not make changes under any circumstances to vendored dependencies in src/crc32c, src/dashbls, src/gsl, src/immer, src/leveldb, src/minisketch, src/secp256k1, src/univalue
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: kwvg
PR: dashpay/dash#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.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/evo/deterministicmns.cpp (1)
543-546: Good optimization logic, but thread safety concern remains.The conditional cache invalidation is a smart optimization that only clears the cache when the SML entry actually changes. However, this also uses mutex-based synchronization instead of the required immutable data structures (Immer library) per coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/evo/deterministicmns.cpp(10 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.{cpp,h,cc,cxx,hpp}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
src/{masternode,evo}/**
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
🧠 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: kwvg
PR: dashpay/dash#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: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T15:51:47.798Z
Learning: Applies to src/{masternode,evo}/** : Masternode lists must use immutable data structures (Immer library) for thread safety
src/evo/deterministicmns.cpp (4)
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.
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T15:51:47.798Z
Learning: Applies to src/{masternode,evo}/** : Masternode lists must use immutable data structures (Immer library) for thread safety
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-18T15:51:47.798Z
Learning: Applies to src/evo/specialtx.h : Special transactions must use payload extensions as defined in src/evo/specialtx.h
Learnt from: knst
PR: dashpay/dash#6658
File: src/evo/creditpool.cpp:177-185
Timestamp: 2025-05-05T12:45:44.781Z
Learning: The GetAncestor() function in CBlockIndex safely handles negative heights by returning nullptr rather than asserting, making it safe to call with potentially negative values.
⏰ 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). (9)
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: win64-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: linux64_ubsan-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (4)
src/evo/deterministicmns.cpp (4)
40-46: LGTM! Well-implemented conversion method.The method correctly extracts all necessary fields from the deterministic masternode state to create a simplified masternode list entry. The const correctness and field mapping are appropriate.
471-471: LGTM! Correct cache invalidation placement.The cache invalidation calls are appropriately placed in
AddMNandRemoveMNmethods, ensuring the SML cache is cleared when the masternode list changes.Also applies to: 618-618
638-638: Good practice to pre-populate cache.Calling
to_sml()before entering the critical section is a good optimization that reduces the time spent holding locks.
788-789: LGTM! Good optimization to avoid redundant calls.Using a local variable
snapshot_hashto store the result ofGetBlockHash()avoids redundant function calls and improves readability.Also applies to: 799-799
| gsl::not_null<std::shared_ptr<const CSimplifiedMNList>> CDeterministicMNList::to_sml() const | ||
| { | ||
| LOCK(m_cached_sml_mutex); | ||
| if (!m_cached_sml) { | ||
| std::vector<std::unique_ptr<CSimplifiedMNListEntry>> sml_entries; | ||
| sml_entries.reserve(mnMap.size()); | ||
|
|
||
| ForEachMN(false, [&sml_entries](auto& dmn) { | ||
| sml_entries.emplace_back(std::make_unique<CSimplifiedMNListEntry>(dmn.to_sml_entry())); | ||
| }); | ||
| m_cached_sml = std::make_shared<CSimplifiedMNList>(std::move(sml_entries)); | ||
| } | ||
|
|
||
| return m_cached_sml; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using immutable data structures for thread safety.
According to the coding guidelines, masternode lists must use immutable data structures (Immer library) for thread safety. The current implementation uses std::mutex for synchronization, which may not align with the required approach.
Consider refactoring to use Immer's immutable data structures instead of mutex-based synchronization to comply with the coding guidelines for the src/{masternode,evo}/** modules.
🤖 Prompt for AI Agents
In src/evo/deterministicmns.cpp around lines 270 to 284, the current to_sml()
method uses a mutex to protect the cached masternode list, but the coding
guidelines require using Immer immutable data structures for thread safety
instead of mutexes. Refactor the caching mechanism to store and update the
masternode list using Immer's immutable containers, eliminating the need for
explicit locking and ensuring thread-safe access according to the project's
standards.
|
LGTM, pls check clang-format suggestions https://github.com/dashpay/dash/actions/runs/16388888870/job/46312212753?pr=6738 |
Issue being fixed or feature implemented
During block validation, each block requires SML (Simplified MN List) to be constructed from scratch: convert ~3000 DeterministicMN + DMNState objects to CSimplifiedMNListEntry; sort them by protx and compare with previously calculated SML.
In case if SML is changed, Dash Core re-calculates Merkle Root hash. On practice, changes in SML list happens only once in tens (or even hundreds) of blocks.
But it is a heavy process which takes up to 20% of CPU-time during block validation.
What was done?
Instead of re-calculation SML list for each block, let's try to keep this calculation cached and reset this cache every time when list of masternodes is actually changed.
The main idea of this cache is to add a
shared_ptr<CSimplifiedMNList>to each objectCDeterministicMNList. Its value is set whenCDeterministicMNManager::ProcessBlockis called and can be copied every time, when new list is re-created.Because
ProcessBlockis called consequently if and only if a new block is connected we don't waste creation of SML when we don't need it.This cache to SML is invalidated every time, when
AddMNorRemoveMNis called. CallsUpdateMNdo not reset this cache if produced CSimplifiedMNListEntry is not changed (see implementation for details).Though, indirect calls of AddMN/RemoveMN/UpdateMN (such as calls happened inside ApplyDiff) also invalidates cache.
Side notes about implementation
This PR inverts dependency of
evo/simplifiedmns -> evo/deterministicmnstoevo/deterministicmns -> evo/simplifiedmns.This change caused explosion in amount of new circular dependencies of
llmq/* <-> evo/determnisticmns; to prevent it a new moduleevo/smldiffhas been introduced.Also
CSimplifiedMNListDiffs::ToJsonandCSimplifiedMNList::ToJsonhas been moved toevo/core_write.How Has This Been Tested?
Tested by invalidation + reconsider 7000 blocks (~2 weeks of blocks).
This PR speeds up blocks validation for 15% compare to current develop.
Drastically improved performance
CalcCbTxMerkleRootMNList(which is part ofCheckCbTxMerkleRoots) andCSimplifiedMNList::CSimplifiedMNListis replaced byCDeterministicMNList::to_smlwhich is almost instant (see perf screenshots).PR times:
develop times:
Breaking Changes
N/A
Checklist: