perf(qt): reduce consequent calls of updateVotingCapability for each block#6835
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/qt/clientmodel.h (1)
56-78: Remove lingering CDeterministicMNListPtr usages in governance.h
The aliasusing CDeterministicMNListPtr = std::shared_ptr<CDeterministicMNList>;(line 40) and its fieldCDeterministicMNListPtr lastMNListForVotingKeys;(line 180) need updating now that the public typedef was removed—replace with the appropriate type or inline declaration.
🧹 Nitpick comments (6)
src/qt/clientmodel.h (1)
118-121: Prefer non-recursive mutex; annotate mnListTip; simplify locking pathUse a plain Mutex here and avoid re-entrant locking; also add thread-safety annotation for mnListTip. This makes misuse harder and TSAN hints clearer.
- mutable RecursiveMutex cs_mnlist; // protects mnListCached - std::unique_ptr<CDeterministicMNList> mnListCached GUARDED_BY(cs_mnlist){}; - const CBlockIndex* mnListTip{nullptr}; + mutable Mutex cs_mnlist; // protects mnListCached/mnListTip + std::unique_ptr<CDeterministicMNList> mnListCached GUARDED_BY(cs_mnlist){}; + const CBlockIndex* mnListTip GUARDED_BY(cs_mnlist){nullptr};Follow-up in clientmodel.cpp provided below to remove the nested LOCK.
src/qt/governancelist.cpp (2)
10-10: Drop heavy include if unusedchain.h seems unnecessary in this TU; types accessed via clientModel are opaque. Remove to cut compile time if nothing here needs it.
-#include <chain.h>
409-412: Trigger voting capability updates with proposal refresh (throttled)This achieves the PR goal of avoiding per-block recomputation. Consider, optionally, reusing the already-fetched masternode list from above to avoid a second copy inside updateVotingCapability.
src/qt/clientmodel.cpp (3)
102-111: Update tip even if list hash unchangedIf the BlockIndex pointer instance changes while the block hash stays the same (reindex/IBD), mnListTip should still be refreshed. Move the assignment before the equality short-circuit.
- LOCK(cs_mnlist); - if (mnListCached->GetBlockHash() == mnList.GetBlockHash()) { - return; - } - mnListCached = std::make_unique<CDeterministicMNList>(mnList); - mnListTip = tip; + LOCK(cs_mnlist); + mnListTip = tip; + if (mnListCached->GetBlockHash() == mnList.GetBlockHash()) { + return; + } + mnListCached = std::make_unique<CDeterministicMNList>(mnList);
119-125: Remove redundant nested LOCK and the need for RecursiveMutexrefreshMasternodeList locks cs_mnlist then calls setMasternodeList(), which also locks it. Drop the outer LOCK and keep all mutation inside setMasternodeList(). This enables using a plain Mutex (see header diff).
void ClientModel::refreshMasternodeList() { auto [mnList, tip] = m_node.evo().getListAtChainTip(); - - LOCK(cs_mnlist); - setMasternodeList(mnList, tip); + setMasternodeList(mnList, tip); }
113-117: Minor: assert non-null cache (defensive)Optional: add an assertion to document the invariant that mnListCached is initialized in the ctor.
- LOCK(cs_mnlist); - return {*mnListCached, mnListTip}; + LOCK(cs_mnlist); + assert(mnListCached != nullptr); + return {*mnListCached, mnListTip};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/qt/clientmodel.cpp(2 hunks)src/qt/clientmodel.h(1 hunks)src/qt/governancelist.cpp(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/qt/governancelist.cppsrc/qt/clientmodel.cppsrc/qt/clientmodel.h
🧠 Learnings (2)
📓 Common learnings
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
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety
Applied to files:
src/qt/clientmodel.cpp
🧬 Code graph analysis (2)
src/qt/clientmodel.cpp (2)
src/node/interfaces.cpp (9)
LOCK(533-537)LOCK(543-550)LOCK(551-558)LOCK(820-829)LOCK(863-867)LOCK(1047-1051)tip(96-104)tip(538-542)tip(559-567)src/qt/clientmodel.h (1)
cs_mnlist(119-119)
src/qt/clientmodel.h (1)
src/evo/deterministicmns.h (1)
CDeterministicMNList(127-188)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (3)
src/qt/governancelist.cpp (2)
367-368: Good: initialize data flow only once clientModel is setMoving the initial refresh behind the nullptr check is correct and prevents premature calls.
492-503: Robust iteration and state reset look goodClearing votableMasternodes before ForEachMN(true, ...) is correct; using wallet.isSpendable(script) matches RPC logic.
src/qt/clientmodel.cpp (1)
53-53: Switch to unique_ptr init: LGTMLocal ownership here is appropriate; copies are still returned by value to callers.
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
This makes sense as an optimisation for reindex but not for regular usage when block time is 15 times larger than GOVERNANCELIST_UPDATE_SECONDS (156 > 10). Can we use one logic while we are still syncing and then switch to another one once we are up to date?
Also, I don't like having all the unrelated refactoring here, let's move that to another PR.
NOTE_1: not every new block changes Deterministic MN List. And ability of vote is triggered even more rare, because it is relevant to only masternodes that you have private key. IMO, 10seconds is a good balance between wasting CPU resources and how long masternode owner is ready to wait once he added a private key to wallet or once is protx transaction is mined. Just to avoid possible misunderstanding... |
|
|
||
| void ClientModel::setMasternodeList(const CDeterministicMNList& mnList, const CBlockIndex* tip) | ||
| { | ||
| LOCK(cs_mnlinst); |
There was a problem hiding this comment.
nit: should be done in a separate commit
4 participants
Issue being fixed or feature implemented
Currently, there are 2 handlers for governance in Qt app:
updateProposalListandupdateVotingCapability.updateVotingCapabilityis calling for each block;updateProposalListis calling once in 10 seconds.Calling
updateVotingCapabilityfor each block is very inefficient because during reindex this handler takes uses one full CPU core on my laptop and make Qt app very irresponsible and lagging; it could take up to 10-60 seconds to see an input text in RPC console or react to click.What was done?
Call
updateVotingCapabilityfor each call ofupdateProposalList, no more often.Minor improvement: use unique_ptr instead shared_ptr in ClientModel: it returns a copy of object anyway currently instead shared_ptr.
How Has This Been Tested?
This PR makes
updateVotingCapabilitydisappear from the top of profiler.Breaking Changes
N/A
Checklist: