refactor: nuke circular dependencies over validationinterface #6736
Conversation
WalkthroughThis set of changes updates notification interfaces and related method signatures across the codebase to include explicit string identifiers for events and to standardize parameter ownership semantics. Notification methods for governance objects, governance votes, chain locks, and recovered signatures now accept an additional string parameter representing the event's unique identifier, replacing previous internal string generation. Additionally, the deterministic masternode list parameter in governance vote notifications is changed from a const reference to a shared pointer, affecting method signatures and calls in multiple classes and modules, including ZMQ notifier interfaces. Unused header includes are removed, and several expected circular dependencies are eliminated from the linting script. No changes are made to core logic, error handling, or control flow. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (14)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (12)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
✨ 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 (4)
src/dsnotificationinterface.h (1)
5-7: Include-guard prefix still reads “BITCOIN”
While outside the immediate scope of this PR, consider renaming the guard toDASH_DSNOTIFICATIONINTERFACE_H(or similar) for long-term consistency with the rest of the Dash-specific headers.src/llmq/chainlocks.cpp (1)
500-501: Interface update correctly applied
NotifyChainLock(…, clsig, clsig->ToString())matches the new signature.Minor nit:
clsig->ToString()is calculated multiple times in this method; caching it once (e.g.const std::string clsigStr = clsig->ToString();) would avoid a few redundant string constructions, though the impact is negligible.src/llmq/signing.cpp (1)
647-648: Call site aligned with new signal signature
NotifyRecoveredSig(recoveredSig, recoveredSig->GetHash().ToString())compiles with the refactored interface.As with the previous file, you could cache the hash string once earlier in the method to avoid repeating
.ToString()if this path becomes hot.src/governance/object.cpp (1)
165-165: Fix formatting and approve notification interface update.The functional changes are correct - converting to shared pointers and adding the vote hash identifier aligns with the notification interface refactor. However, there's a clang formatting issue that needs to be addressed.
Please run clang-format to fix the formatting issue:
- GetMainSignals().NotifyGovernanceVote(std::make_shared<CDeterministicMNList>(tip_mn_list), std::make_shared<const CGovernanceVote>(vote), vote.GetHash().ToString()); + GetMainSignals().NotifyGovernanceVote(std::make_shared<CDeterministicMNList>(tip_mn_list), + std::make_shared<const CGovernanceVote>(vote), + vote.GetHash().ToString());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
src/dsnotificationinterface.h(1 hunks)src/governance/governance.cpp(1 hunks)src/governance/object.cpp(1 hunks)src/llmq/chainlocks.cpp(1 hunks)src/llmq/signing.cpp(1 hunks)src/net.cpp(0 hunks)src/validationinterface.cpp(2 hunks)src/validationinterface.h(2 hunks)src/zmq/zmqabstractnotifier.cpp(1 hunks)src/zmq/zmqabstractnotifier.h(1 hunks)src/zmq/zmqnotificationinterface.cpp(1 hunks)src/zmq/zmqnotificationinterface.h(1 hunks)src/zmq/zmqpublishnotifier.cpp(2 hunks)src/zmq/zmqpublishnotifier.h(2 hunks)test/lint/lint-circular-dependencies.py(0 hunks)
💤 Files with no reviewable changes (2)
- src/net.cpp
- test/lint/lint-circular-dependencies.py
🧰 Additional context used
🧠 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.
src/dsnotificationinterface.h (1)
Learnt from: kwvg
PR: dashpay/dash#6530
File: src/validation.cpp:4514-4516
Timestamp: 2025-01-14T08:40:00.994Z
Learning: In Dash Core, error() is used (instead of state.Invalid()) to report critical failures that require immediate user intervention, particularly for database/disk errors and data corruption scenarios. This is an established pattern seen in validation.cpp.
🧬 Code Graph Analysis (5)
src/llmq/chainlocks.cpp (2)
src/validationinterface.cpp (2)
GetMainSignals(116-119)GetMainSignals(116-116)src/validationinterface.h (1)
GetMainSignals(247-247)
src/zmq/zmqabstractnotifier.cpp (3)
src/validationinterface.cpp (2)
NotifyGovernanceVote(286-291)NotifyGovernanceVote(286-286)src/zmq/zmqnotificationinterface.cpp (2)
NotifyGovernanceVote(223-228)NotifyGovernanceVote(223-223)src/zmq/zmqpublishnotifier.cpp (4)
NotifyGovernanceVote(265-273)NotifyGovernanceVote(265-265)NotifyGovernanceVote(436-443)NotifyGovernanceVote(436-436)
src/governance/governance.cpp (2)
src/validationinterface.cpp (2)
GetMainSignals(116-119)GetMainSignals(116-116)src/validationinterface.h (1)
GetMainSignals(247-247)
src/zmq/zmqpublishnotifier.h (3)
src/validationinterface.h (3)
tip_mn_list(169-169)tip_mn_list(169-169)tip_mn_list(237-237)src/zmq/zmqabstractnotifier.h (1)
tip_mn_list(75-75)src/zmq/zmqnotificationinterface.h (1)
tip_mn_list(36-36)
src/validationinterface.h (3)
src/zmq/zmqabstractnotifier.h (8)
tip_mn_list(75-75)pindex(62-62)pindex(64-64)pindex(66-66)pindex(73-73)object(76-76)currentTx(77-77)sig(78-78)src/zmq/zmqnotificationinterface.h (5)
tip_mn_list(36-36)pindex(34-34)object(37-37)currentTx(38-38)sig(39-39)src/zmq/zmqpublishnotifier.h (14)
tip_mn_list(66-66)tip_mn_list(135-135)pindex(42-42)pindex(48-48)pindex(90-90)pindex(96-96)pindex(102-102)pindex(114-114)pindex(115-115)object(72-72)object(141-141)currentTx(78-78)currentTx(147-147)sig(153-153)
🪛 GitHub Actions: Clang Diff Format Check
src/governance/object.cpp
[error] 162-163: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix the formatting issues.
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_multiprocess-build / Build source
- GitHub Check: arm-linux-build / Build source
🔇 Additional comments (15)
src/dsnotificationinterface.h (1)
1-1: Updated copyright notice acknowledged
Change is correct and consistent with Dash Core’s licensing header style.src/governance/governance.cpp (1)
361-361: LGTM: Correctly adds governance object hash identifier to notification.The addition of
nHash.ToString()as the second parameter toNotifyGovernanceObjectis consistent with the broader refactor to include string identifiers in notification methods for improved traceability.src/zmq/zmqabstractnotifier.cpp (1)
56-59: LGTM: Parameter type conversion aligns with shared ownership pattern.The change from
const CDeterministicMNList&tostd::shared_ptr<CDeterministicMNList>&is consistent with the broader refactor to use shared pointers for the deterministic masternode list in notification interfaces.src/zmq/zmqnotificationinterface.h (1)
36-36: LGTM: Interface signature correctly updated for shared pointer parameter.The parameter type change from
const CDeterministicMNList&tostd::shared_ptr<CDeterministicMNList>&is consistent with the notification interface refactor and maintains compatibility with the implementation.src/zmq/zmqabstractnotifier.h (1)
75-75: LGTM: Virtual method signature correctly updated for shared pointer parameter.The parameter type change from
const CDeterministicMNList&tostd::shared_ptr<CDeterministicMNList>&ensures all derived ZMQ notifiers use consistent shared pointer parameters in theirNotifyGovernanceVoteimplementations.src/zmq/zmqnotificationinterface.cpp (1)
223-228: LGTM: Parameter type change aligns with dependency refactoring.The change from
const CDeterministicMNList&tostd::shared_ptr<CDeterministicMNList>is consistent with the broader refactoring to eliminate circular dependencies. The shared ownership semantics provide better lifetime management and the lambda implementation correctly forwards the parameters to the notifiers.src/zmq/zmqpublishnotifier.h (1)
66-66: LGTM: Method signatures updated consistently.Both
CZMQPublishHashGovernanceVoteNotifierandCZMQPublishRawGovernanceVoteNotifiermethod signatures are properly updated to usestd::shared_ptr<CDeterministicMNList>, maintaining consistency with the base class interface changes and supporting the circular dependency elimination goals.Also applies to: 135-135
src/zmq/zmqpublishnotifier.cpp (2)
265-273: LGTM: Method signature updated correctly.The
CZMQPublishHashGovernanceVoteNotifier::NotifyGovernanceVotemethod signature is properly updated to usestd::shared_ptr<CDeterministicMNList>, and the implementation logic remains unchanged and correct.
436-443: LGTM: Proper shared_ptr usage in implementation.The method signature and implementation are correctly updated. The shared_ptr is properly dereferenced with
*tip_mn_listwhen passed tovote->ToString(), which expects a reference to the underlying object.src/validationinterface.cpp (4)
277-284: LGTM: Enhanced interface with explicit ID parameter.The addition of the
const std::string& idparameter toNotifyChainLockallows callers to provide explicit identifiers, reducing internal dependencies. The logging correctly uses the passedidparameter instead of generating it internally, which supports the circular dependency elimination goals.
286-291: LGTM: Consistent interface updates for governance notifications.Both the parameter type change to
std::shared_ptr<CDeterministicMNList>and the addition of theidparameter are well-implemented. The logging uses the providedidstring, eliminating the need for internal ID generation and supporting dependency reduction.
293-298: LGTM: Governance object notification updated consistently.The addition of the
idparameter and corresponding logging update follows the same pattern as other notification methods, maintaining consistency across the interface.
309-315: LGTM: Recovered signature notification enhanced appropriately.The
NotifyRecoveredSigmethod is consistently updated with theidparameter, and the logging properly utilizes the provided identifier string.src/validationinterface.h (2)
169-169: LGTM: Virtual interface updated for consistency.The change from
const CDeterministicMNList&tostd::shared_ptr<CDeterministicMNList>in the virtualNotifyGovernanceVotemethod ensures all derived classes use consistent parameter types and supports improved lifetime management for circular dependency elimination.
236-240: LGTM: Main signals interface enhanced with explicit identifiers.The addition of
const std::string& idparameters toNotifyChainLock,NotifyGovernanceVote,NotifyGovernanceObject, andNotifyRecoveredSigmethods, along with the shared_ptr change for the governance vote method, creates a consistent interface that reduces internal dependencies and supports the circular dependency elimination objectives.
knst
force-pushed
the
refactor-circular-dsnotificationinterface
branch
from
b773967 to
e66d2cf
Compare
| fDirtyCache = true; | ||
| // SEND NOTIFICATION TO SCRIPT/ZMQ | ||
| GetMainSignals().NotifyGovernanceVote(tip_mn_list, std::make_shared<const CGovernanceVote>(vote)); | ||
| GetMainSignals().NotifyGovernanceVote(std::make_shared<CDeterministicMNList>(tip_mn_list), |
There was a problem hiding this comment.
Not a huge fan of this but existing practice so it's probably alright
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
refactor-circular-dsnotificationinterface
branch
from
e66d2cf to
24fed4a
Compare
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
refactor-circular-dsnotificationinterface
branch
from
24fed4a to
27899d2
Compare
These circular dependencies exists only because excessive logging here. Prepared extra debug info outside of validationinterface
|
This pull request has conflicts, please rebase. |
knst
force-pushed
the
refactor-circular-dsnotificationinterface
branch
from
27899d2 to
29f45ce
Compare
PastaPastaPasta
force-pushed
the
refactor-circular-dsnotificationinterface
branch
from
29f45ce to
e875c42
Compare
|
Rebased on latest develop |
4 participants
What was done?
See commits
How Has This Been Tested?
By running
test/lint/lint-circular-dependencies.pyBreaking Changes
N/A
Checklist: