refactor: break multiple circular dependencies over evo/deterministicmns and evo/cbtx #6730
Conversation
WalkthroughThis set of changes refactors the management and validation of deterministic masternode lists and chainlocks by relocating key logic from the deterministic masternode manager to the special transaction processor. The methods for building new masternode lists from blocks and handling quorum commitments are removed from the deterministic masternode manager and implemented in the special transaction processor. Functions related to simplified masternode list Merkle root calculation and chainlock validation are removed from 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (9)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (5)
🧰 Additional context used🧠 Learnings (2)📓 Common learningssrc/evo/deterministicmns.cpp (5)⏰ 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). (5)
🔇 Additional comments (2)
✨ 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: 0
🧹 Nitpick comments (3)
src/node/miner.cpp (1)
122-179: Consider optimizing duplicate call to GetBestChainLock()At line 175,
chainlock_handler.GetBestChainLock().getSig()is called again instead of reusingbest_clsig.getSig(). This is a minor inefficiency that could be avoided.// Inserting our best CL bestCLHeightDiff = pindexPrev->nHeight - best_clsig.getHeight(); - bestCLSignature = chainlock_handler.GetBestChainLock().getSig(); + bestCLSignature = best_clsig.getSig();src/evo/cbtx.h (1)
66-68: Consider clarifying the comment about block processing requirement.The comment states "This can only be done after the block has been fully processed" but doesn't specify what "fully processed" means in this context. Consider being more explicit about the requirement.
-// This can only be done after the block has been fully processed, as otherwise we won't have the finished MN list +// This can only be done after the block has been fully processed (i.e., after BuildNewListFromBlock has been called), +// as otherwise we won't have the finished MN list required for validationsrc/evo/specialtxman.cpp (1)
424-427: Remove redundant condition in collateral check.The condition
dmn->collateralOutpoint == in.prevoutis redundant becauseGetMNByCollateral(in.prevout)already ensures this equality when it returns a non-null result.auto dmn = newList.GetMNByCollateral(in.prevout); - if (dmn && dmn->collateralOutpoint == in.prevout) { + if (dmn) { newList.RemoveMN(dmn->proTxHash);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/evo/cbtx.cpp(1 hunks)src/evo/cbtx.h(1 hunks)src/evo/deterministicmns.cpp(2 hunks)src/evo/deterministicmns.h(0 hunks)src/evo/dmnstate.cpp(0 hunks)src/evo/simplifiedmns.cpp(1 hunks)src/evo/simplifiedmns.h(1 hunks)src/evo/specialtxman.cpp(6 hunks)src/evo/specialtxman.h(2 hunks)src/node/miner.cpp(3 hunks)src/node/miner.h(0 hunks)src/test/util/setup_common.cpp(2 hunks)test/lint/lint-circular-dependencies.py(1 hunks)
💤 Files with no reviewable changes (3)
- src/evo/dmnstate.cpp
- src/node/miner.h
- src/evo/deterministicmns.h
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
🔇 Additional comments (13)
src/evo/simplifiedmns.h (1)
182-183: LGTM: Well-designed function signature for merkle root calculation.The function signature follows established validation patterns with proper error reporting via
BlockValidationStateand uses move semantics for the input list to optimize performance.test/lint/lint-circular-dependencies.py (1)
47-47: Expected addition of newly exposed circular dependency.This aligns with the PR objectives - the refactoring revealed a previously hidden circular dependency that was masked by the removed cycles. Adding it to the expected list is appropriate for this refactoring-focused PR.
src/evo/simplifiedmns.cpp (1)
406-448: Excellent implementation with thread safety and performance optimizations.The function demonstrates several best practices:
- Thread-safe caching with proper mutex protection
- Performance benchmarking for monitoring
- Validation of merkle tree mutations
- Proper exception handling with detailed error states
- Efficient use of move semantics for cache updates
The caching strategy will provide significant performance benefits for repeated calculations.
src/test/util/setup_common.cpp (1)
493-493: Correct update to use new special transaction manager interface.This change appropriately updates the test infrastructure to use the refactored
BuildNewListFromBlockmethod that was moved fromCDeterministicMNManagertoCSpecialTxProcessor, maintaining test functionality after the architectural changes.src/evo/specialtxman.h (2)
8-9: Good additions for new functionality.The new includes (
gsl/pointers.hfor parameter safety,optionalfor return values) and forward declaration properly support the newBuildNewListFromBlockmethod while minimizing compilation dependencies.Also applies to: 19-19
75-80: Well-designed method signature with clear documentation.The
BuildNewListFromBlockmethod signature demonstrates good practices:
- Uses
gsl::not_nullto express non-null parameter requirements- Proper thread safety annotation with
EXCLUSIVE_LOCKS_REQUIRED(cs_main)- Clear documentation about the block hash limitation
- Appropriate parameter types for the masternode list building functionality
src/node/miner.cpp (1)
288-314: LGTM! Clean refactoring of masternode list building.The changes properly move the masternode list building responsibility from
CDeterministicMNManagertoCSpecialTxProcessor, and the integration of the newCalcCbTxBestChainlockhelper function is well done.src/evo/cbtx.cpp (1)
45-60: LGTM! Clean separation of concerns.The simplification of
CheckCbTxMerkleRootsto only handle quorum merkle root validation is consistent with the refactoring goals. The removal of MN list and chainlock validation logic properly breaks the circular dependencies.src/evo/deterministicmns.cpp (1)
584-586: LGTM! Clean removal of circular dependencies.The simplification of the
ProcessBlocksignature and removal ofBuildNewListFromBlockandHandleQuorumCommitmentmethods successfully breaks the circular dependencies as intended.src/evo/specialtxman.cpp (4)
29-94: Well-implemented chainlock validation with effective caching.The
CheckCbTxBestChainlockfunction properly validates chainlock signatures with good performance optimizations through caching. The logic correctly handles all cases including null chainlocks and enforces proper chainlock progression rules.
156-171: LGTM! Proper handling of DKG participation failures.The function correctly punishes masternodes that fail DKG participation with an appropriate 66% penalty, which aligns with the comment about banning after 3 failures within a payment cycle.
521-527: Good addition of CCbTx requirement validation.The check ensures that a coinbase special transaction is present when DIP0003 is active and merkle root checks are enabled, which is essential for maintaining consensus rules.
583-615: LGTM! Clean integration of masternode list building.The integration of
BuildNewListFromBlockand merkle root calculation is well-implemented with proper error handling and performance logging.
knst
force-pushed
the
fix-circular-deterministicmn
branch
from
48431e5 to
3ee1a1b
Compare
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
fix-circular-deterministicmn
branch
from
2727a68 to
566ac0f
Compare
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
fix-circular-deterministicmn
branch
from
566ac0f to
d2fff7d
Compare
src/evo/deterministicmns.cpp
Outdated
| } | ||
|
|
||
| ======= | ||
| >>>>>>> a67cfae449 (refactor: remove dependency of evo/deterministicmns on llmq/utils and llmq/commitment) |
…state,deterministicmns}
knst
force-pushed
the
fix-circular-deterministicmn
branch
from
d2fff7d to
86c3dad
Compare
4 participants
Issue being fixed or feature implemented
Addressed some exception for circular dependencies.
What was done?
See each commit. Most changes here are just moving functions between files,
git diff --color-moved=zebramay help to review it.Though,
GetAllQuorumMembers()is now called byBuildNewListFromBlock()compared to current behavior in develop branch.How Has This Been Tested?
Removed these exception from linter:
New one appeared in the list because it is a longer circular dependency that always has been here but has been hidden by shorter loop:
core_io -> evo/cbtx -> evo/simplifiedmns -> core_io.Breaking Changes
N/A
Checklist: