Skip to content

Comments

feat(qt): UI refresh (4/n, introduce distinct widgets for Dash-specific reporting in debug window)#7118

Open
kwvg wants to merge 19 commits intodashpay:developfrom
kwvg:info_refresh
Open

feat(qt): UI refresh (4/n, introduce distinct widgets for Dash-specific reporting in debug window)#7118
kwvg wants to merge 19 commits intodashpay:developfrom
kwvg:info_refresh

Conversation

@kwvg
Copy link
Collaborator

@kwvg kwvg commented Jan 28, 2026

Additional Information

v23.0.2 (cdc5a63) This PR (WIP code)
See above
Does not exist in this build
Does not exist in this build

Breaking Changes

None expected.

Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests (note: N/A)
  • I have made corresponding changes to the documentation (note: N/A)
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

@kwvg kwvg added this to the 23.1 milestone Jan 28, 2026
@github-actions
Copy link

github-actions bot commented Jan 28, 2026

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

@thephez
Copy link
Collaborator

thephez commented Jan 28, 2026

@kwvg would it be difficult to also add the age of the quorums in addition to their health? On testnet some of those quorums haven't formed for years if I'm not mistaken. Would be nice to know if quorums were "fresh" and when the next ones are expected to form.

@github-actions
Copy link

github-actions bot commented Feb 3, 2026

This pull request has conflicts, please rebase.

PastaPastaPasta added a commit that referenced this pull request Feb 12, 2026
…use `UniValue` instead of `QJsonObject`, consistency in tab hiding behavior, minor bugfixes

3329898 chore: remove redundant `std::nullopt` from default init (Kittywhiskers Van Gogh)
d9b141a qt: use use `QHash`/`QSet` instead of `std::{map,set}` (Kittywhiskers Van Gogh)
ce747c8 qt: add vote signing to wallet interface (Kittywhiskers Van Gogh)
66f2e80 fix: check for `llmq_ctx` before accessing `isman` (Kittywhiskers Van Gogh)
ec5fb20 fix: avoid double-free by clearing before dtor semaphore cleanup (Kittywhiskers Van Gogh)
3ee43d9 fix: switch back to overview page if tab disabled (Kittywhiskers Van Gogh)
c8ce943 fix: use UniValue instead of QJsonObject to construct proposal (Kittywhiskers Van Gogh)
abb9aa1 fix: use wider integer to avoid potential overflow scenarios (Kittywhiskers Van Gogh)
6b90182 fix: acknowledge `updateDIP3List()` fail states (Kittywhiskers Van Gogh)
14b494c fix: avoid potential reassignment of atomic `m_mn_list_changed` (Kittywhiskers Van Gogh)
3b68a2d fix: emit `dataChanged()` when the display unit changes. (Kittywhiskers Van Gogh)
8ab5c0c fix: don't include invisible/hidden tabs in width calculations (Kittywhiskers Van Gogh)
99fd118 fix: avoid precision loss, storing as `CAmount`, fix potential overflow (Kittywhiskers Van Gogh)
72ed0ed fix: remove unused `m_node` in `ProposalWizard` (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  Resolution of preexisting bugs in `develop`, review feedback from [dash#7112](#7112) and [dash#7116](#7116) and fixes for bugs discovered when working on [dash#7118](#7118).

  ## Breaking Changes

  None expected.

  ## Checklist

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)**
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK 3329898
  knst:
    utACK 3329898

Tree-SHA512: 5437fa59ae6cd7a7f3d6bfc4167f8562265c0395cd3a92bcbc7c1d97541e25cd373f6f0808d58a503855469875c33d5e08d8b2041fee2c2a001416653f8cca7f
@kwvg kwvg modified the milestones: 23.1, 24 Feb 15, 2026
@kwvg kwvg changed the title feat(qt): UI refresh (4/n, introduce distinct widgets for Dash-specific reporting in debug window, governance stats) feat(qt): UI refresh (4/n, introduce distinct widgets for Dash-specific reporting in debug window) Feb 19, 2026
@kwvg kwvg force-pushed the info_refresh branch 3 times, most recently from 2796e9c to 0827101 Compare February 20, 2026 08:49
PastaPastaPasta added a commit that referenced this pull request Feb 20, 2026
…data, use for `{Masternode,Proposal}List`s

3d64056 move-only: move definitions to clientfeeds.{cpp,h} to deal with circulars (Kittywhiskers Van Gogh)
4f56a5e lint: update circular dependencies allowlist (Kittywhiskers Van Gogh)
77a99ee qt: register masternode information as a feed, replace per-wallet thread (Kittywhiskers Van Gogh)
722ec6a refactor(qt): pull out proposal data sourcing to `MasternodeFeed` (Kittywhiskers Van Gogh)
a6c7ba3 qt: register proposal information as a feed, replace per-wallet thread (Kittywhiskers Van Gogh)
35eb3d2 feat(qt): introduce framework for sourcing and applying data (Kittywhiskers Van Gogh)
4e951df refactor(qt): pull out proposal data sourcing to `ProposalFeed` (Kittywhiskers Van Gogh)
7827ae1 move-only: src/qt/governancelist.{cpp,h} -> src/qt/proposallist.{cpp,h} (Kittywhiskers Van Gogh)
342f785 refactor: s/ProposalList/Proposals/g (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Dependency for #7118

  * The redesigned "Masternode" and "Governance" tabs (see [dash#7110](#7110) and [dash#7116](#7116)) were migrated to a worker-based update mechanism with a773635 and others, this proved to deal with main thread contention and the debounce mechanism prevented frequent refreshes of infrequently updated data and was a step towards throttled event-driven updates.
    * This implementation though had a problematic side effect, a worker thread is spawned **per tab per wallet** and while per tab threads were by design, per wallet threads meant redundant work happening all at once fighting over the same locks, which cause noticeable performance degradation.

    * This pull request therefore introduces the concept of "feeds", that perform the expensive fetch operation that are either triggered by events (and debounced) or user feedback (and executed immediately) to ensure snappiness. The IBD throttling logic is respected.

  * The result is having a central worker thread fetching (`fetch()`) and dispatching refresh signals (`dataReady()`) which are then used by the consumer (e.g. `updateMasternodeList()`) to fetch wallet-specific information (which is relatively inexpensive) and then *apply* those changes (e.g. `setMasternodeList()`).

    * Note that wallet-specific fetching is done on the main thread, to prevent thread explosion (even if they are doing now non-redundant work) as they'd still be per tab per wallet.

  * Note that masternodes and proposal views use different refresh intervals, masternode data uses 3 seconds and proposal data uses 10 seconds, scaled up to 30s and 60s respectively during IBD.

  ## Breaking Changes

  None expected.

  ## Checklist

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)**
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK 3d64056

Tree-SHA512: 2bd89418e9de80c10269fc723468141970a41e492baadf839824eb4c58cea43f65ab8b8cbeac1867e0dffd3c3bc5785ec59fab97bd1c3659fe46f7bf797eddcc
@kwvg kwvg marked this pull request as ready for review February 20, 2026 16:25
@kwvg
Copy link
Collaborator Author

kwvg commented Feb 20, 2026

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Feb 20, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai
Copy link

coderabbitai bot commented Feb 20, 2026

Walkthrough

Adds InstantSend UI notifications and wires them through the node UI interface; replaces direct masternode-list access with a feed-based Qt data model (ChainLockFeed, CreditPoolFeed, InstantSendFeed, QuorumFeed, MasternodeFeed) and updates ClientModel and multiple UI components to consume feeds; splits RPCConsole info view into two custom widgets (InformationWidget, NetworkWidget) with new .ui files and implementations; removes several MnList accessors from the node interface and exposes weighted counts; introduces GUI utilities (formatAmount, formatBlockDuration) and small Qt helpers/util.h; adds a debug-log open action.

Sequence Diagram(s)

sequenceDiagram
    participant Core as Core Signals
    participant NodeIF as Node Interface
    participant Client as ClientModel
    participant Feed as Data Feed
    participant Widget as UI Widget

    Core->>NodeIF: NotifyInstantSendChanged()
    NodeIF->>Client: emit instantSendChanged()
    Client->>Feed: requestRefresh()
    Feed->>Feed: fetch() (LLMQ / evo / MnList)
    Feed->>Client: dataChanged()
    Client->>Widget: forward data (via feed pointers)
    Widget->>Widget: handle*DataChanged()
    Widget->>UI: update display
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.56% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: introducing distinct widgets for Dash-specific reporting in the Qt debug window UI refresh.
Description check ✅ Passed The pull request description clearly relates to the changeset, describing UI refresh work introducing distinct widgets for Dash-specific reporting in the debug window, with supporting images and dependency documentation.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (9)
src/qt/proposalmodel.h (1)

62-64: Nit: constructor declaration still uses _govObj while the definition uses govObj.

The parameter was renamed in proposalmodel.cpp line 24 but the declaration here was left unchanged. This is harmless in C++ but creates a minor readability inconsistency.

🔧 Suggested alignment
-    explicit Proposal(ClientModel& client_model, const CGovernanceObject& _govObj,
+    explicit Proposal(ClientModel& client_model, const CGovernanceObject& govObj,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/qt/proposalmodel.h` around lines 62 - 64, The Proposal constructor
declaration uses the parameter name _govObj while the definition uses govObj;
update the declaration of Proposal(ClientModel& client_model, const
CGovernanceObject& _govObj, const interfaces::GOV::GovernanceInfo& govInfo, int
collateral_confs, bool is_broadcast) to use govObj instead of _govObj so the
declaration and definition match (ensure the symbol Proposal and its parameter
list are adjusted accordingly).
src/evo/chainhelper.h (1)

44-48: Encapsulation improvement with credit_pool_manager moving to private.

The new GetCreditPool() passthrough is a clean public API. However, the friend class node::BlockAssembler declaration (line 44) still allows BlockAssembler::CreateNewBlock (miner.cpp line 309) to access credit_pool_manager directly via *m_chain_helper.credit_pool_manager, while addPackageTxs (miner.cpp line 471) uses the new public GetCreditPool(). Consider migrating CreateNewBlock to the public API as well to remove the friend declaration entirely.

Also applies to: 68-74

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/evo/chainhelper.h` around lines 44 - 48, The friend declaration allows
node::BlockAssembler to access the now-private credit_pool_manager directly;
update BlockAssembler usages (notably BlockAssembler::CreateNewBlock and its
call sites in miner.cpp) to use the public passthrough GetCreditPool() on
m_chain_helper instead of dereferencing credit_pool_manager, or add a new public
ChainHelper method that exposes exactly the functionality CreateNewBlock needs
and call that from CreateNewBlock; then remove the friend class
node::BlockAssembler declaration and ensure addPackageTxs and CreateNewBlock
both use GetCreditPool()/the new public API rather than accessing
credit_pool_manager directly.
src/qt/proposallist.cpp (1)

253-261: Trailing semicolon after for-loop body.

Line 261 has }; — the semicolon after the closing brace of a range-for body is a null statement and harmless, but inconsistent with the rest of the codebase style.

Proposed fix
-    };
+    }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/qt/proposallist.cpp` around lines 253 - 261, The for-range loop over
data_mn->m_entries ends with a stray trailing semicolon (`};`) which is
stylistically inconsistent; remove the extraneous semicolon after the closing
brace of the loop body so the loop is `for (const auto& entry :
data_mn->m_entries) { ... }` and leave the loop body unchanged (identify the
loop by the use of entry->isBanned(),
GetScriptForDestination(PKHash(entry->keyIdVotingRaw())),
walletModel->wallet().isSpendable(...), and
votableMasternodes[entry->proTxHashRaw()] = entry->keyIdVotingRaw()).
src/qt/masternodemodel.h (1)

9-9: Consider forward-declaring and moving #include <interfaces/node.h> to the .cpp file if possible.

interfaces/node.h is a heavy header that pulls in many transitive includes (net.h, netbase.h, etc.). Since MnEntryCPtr is std::shared_ptr<const MnEntry>, you need the full definition for the inline raw accessors (lines 68-84). If those accessors were moved out-of-line to the .cpp, you could reduce the include to a forward declaration and keep the header lighter. That said, this is a minor build-time concern and the current approach is functional.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/qt/masternodemodel.h` at line 9, The header currently includes the heavy
interfaces/node.h for inline "raw accessors" (MnEntryCPtr related functions at
lines 68-84); instead remove the `#include` from masternodemodel.h,
forward-declare "class MnEntry;" and keep the MnEntryCPtr typedef/declaration,
then turn those inline raw accessor definitions in masternodemodel.h into
out-of-line declarations. Implement those accessor methods in
masternodemodel.cpp and add `#include` <interfaces/node.h> there so the full
MnEntry definition is available only in the .cpp, reducing header dependencies
and build impact.
src/qt/informationwidget.h (1)

36-42: Two private: sections can be merged.

Minor style nit: lines 36 and 40 both declare private: sections. These could be consolidated into a single section.

Proposed merge
-private:
-    /** Update UI with latest network info from model. */
-    void updateNetworkState();
-
 private:
+    /** Update UI with latest network info from model. */
+    void updateNetworkState();
+
     Ui::InformationWidget* ui;
     ClientModel* clientModel{nullptr};
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/qt/informationwidget.h` around lines 36 - 42, In the InformationWidget
class, merge the two private access specifiers by removing the redundant
"private:" line so that updateNetworkState(), ui (Ui::InformationWidget*), and
clientModel (ClientModel* clientModel{nullptr}) are all declared under a single
private section; keep the existing declarations (void updateNetworkState();
Ui::InformationWidget* ui; ClientModel* clientModel{nullptr};) and ensure
spacing/indentation matches surrounding style.
src/qt/forms/networkwidget.ui (1)

157-168: Inconsistent textFormat property across value labels.

Several value labels are missing <property name="textFormat"><enum>Qt::PlainText</enum></property> while most other value labels in this form have it set (e.g., labelCrLastBlock, labelCrLocked, labelISLocks, bestClHeight, etc.). The affected labels are: labelISPending, labelISWaiting, labelISUnprotected, masternodeCount, and evoCount.

Setting Qt::PlainText is a good defensive practice to prevent unintended rich-text rendering if the label text ever contains HTML-like characters.

Also applies to: 177-188, 197-208, 224-235, 244-255

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/qt/forms/networkwidget.ui` around lines 157 - 168, Add a missing
textFormat property set to Qt::PlainText for the value QLabel widgets that
currently lack it (labelISPending, labelISWaiting, labelISUnprotected,
masternodeCount, evoCount) so they match the other value labels (e.g.,
labelCrLastBlock, labelCrLocked, labelISLocks, bestClHeight); edit each
corresponding <widget class="QLabel" name="..."> block in the UI XML and insert
<property name="textFormat"><enum>Qt::PlainText</enum></property> to prevent
unintended rich-text rendering.
src/qt/clientfeeds.cpp (1)

192-195: Raw pointers aliasing into shared_ptr-managed entries: correct but fragile.

m_by_protx and m_by_service store raw pointers obtained via entry.get() before the shared_ptr is moved into m_entries. This is safe because the shared ownership transfers to m_entries (same Data struct), and Data is published as effectively const via setData. However, any future modification to m_entries (erase, clear, reorder) would silently invalidate these raw pointers.

Consider documenting this invariant or using indices/iterators into m_entries instead of raw pointers if the data structure evolves.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/qt/clientfeeds.cpp` around lines 192 - 195, The code stores raw pointers
from entry.get() into m_by_protx and m_by_service while ownership is held by
m_entries, which is fragile if m_entries is ever modified; update the maps to
store either shared_ptr<MasternodeEntry> (preferred) or stable indices instead
of raw pointers so the maps own/hold a reference independent of m_entries, i.e.,
change assignments to ret->m_by_protx[dmn->getProTxHash()] = entry and
ret->m_by_service[util::make_array(dmn->getNetInfoPrimary().GetKey())] = entry
(or store an index into m_entries and update lookup logic), and ensure
MasternodeEntry lifetime is managed consistently with setData and document the
invariant if opting to keep raw pointers.
src/node/interfaces.cpp (2)

453-465: Linear mempool scan for platform transfers under lock.

getPendingAssetUnlocks iterates the entire mempool to count IsPlatformTransfer() transactions. This is O(n) per call (every 3–30s). For typical Dash mempool sizes this is fine, but worth noting if mempool grows significantly or if the feed interval is tightened.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/node/interfaces.cpp` around lines 453 - 465, getPendingAssetUnlocks
currently does a linear iteration over context().mempool->mapTx under the
mempool lock to count transactions with IsPlatformTransfer(), causing O(n) work
every call; change this to avoid repeated full scans by using a maintained
counter or index: add/keep an atomic or guarded pendingPlatformTransferCount in
the mempool and update it inside the mempool’s insertion/removal handlers (where
transactions are added/removed) based on tx.IsPlatformTransfer(), then have
getPendingAssetUnlocks simply read that counter (or query a mempool index)
instead of iterating mapTx; reference functions/fields: getPendingAssetUnlocks,
context().mempool, mapTx, and IsPlatformTransfer to locate the change points.

417-434: getCreditPoolCounts calls GetCreditPool twice — once for tip, once for pprev.

This computes the credit pool diff by fetching the pool state at two consecutive block heights. If GetCreditPool involves non-trivial computation, this doubles the cost. Given the 3–30s feed throttle, this is acceptable, but consider caching or computing the diff incrementally if profiling shows it's expensive.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/node/interfaces.cpp` around lines 417 - 434, getCreditPoolCounts
currently calls chain_helper.GetCreditPool twice (once for pindexTip and again
for pindexTip->pprev) which duplicates work; change it to call GetCreditPool
once for the tip (store in a local like tipPool) and only call GetCreditPool for
pprev if pindexTip->pprev exists (store as prevPool) then compute ret.m_locked,
ret.m_limit from tipPool and ret.m_diff = tipPool.locked - prevPool.locked,
keeping the existing LOCK(::cs_main) and return path; update references in
getCreditPoolCounts to use these cached locals (tipPool, prevPool) instead of
repeated GetCreditPool calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/evo/deterministicmns.h`:
- Line 372: Fix the doc comment for the parameter nCount by removing the stray
double-quote after max(): change the text that currently reads '"nCount =
max()"" means' to '"nCount = max()" means' so the comment about using max() to
mean "all" is correctly punctuated; update the comment near the parameter
description for nCount in deterministicmns.h (the line describing nCount and
max()).
- Around line 145-154: Fix the stray double-quote in the doc comment that
follows the Counts/GetCounts logic: locate the comment that currently reads
`"nCount = max()"" means` (near the Counts struct / GetCounts explanation) and
change it to `"nCount = max()" means` so there is only a single closing quote;
ensure the surrounding comment text remains unchanged.

In `@src/node/interfaces.cpp`:
- Around line 466-495: The concern about quorum ordering is
incorrect—ScanQuorums returns newest-first so keeping quorums[0] for
newest_height in getQuorumStats() is correct; revert any attempted change that
indexed the last element and restore the current newest_height computation using
quorums[0]->m_quorum_base_block_index->nHeight in src/node/interfaces.cpp within
getQuorumStats(), and add a brief inline comment near that line clarifying
"ScanQuorums returns newest-first" to prevent future confusion; optionally, if
you want to address the minor lock scope note, add a TODO comment on the
LOCK(::cs_main) in getQuorumStats() suggesting profiling and possible lock
narrowing rather than changing behavior now.

In `@src/qt/util.h`:
- Around line 13-15: The QByteArray construction currently uses data.size()
(size_t) which narrows to int and can trigger warnings or truncate large
buffers; in the function that returns QByteArray(reinterpret_cast<const
char*>(data.data()), data.size()) check that data.size() <=
std::numeric_limits<int>::max() and handle the overflow case (e.g. log/throw or
return an error/empty QByteArray), then pass the size as
static_cast<int>(data.size()) to QByteArray; keep the reinterpret_cast<const
char*>(data.data()) and update this call site to avoid implicit narrowing and
potential truncation.

---

Nitpick comments:
In `@src/evo/chainhelper.h`:
- Around line 44-48: The friend declaration allows node::BlockAssembler to
access the now-private credit_pool_manager directly; update BlockAssembler
usages (notably BlockAssembler::CreateNewBlock and its call sites in miner.cpp)
to use the public passthrough GetCreditPool() on m_chain_helper instead of
dereferencing credit_pool_manager, or add a new public ChainHelper method that
exposes exactly the functionality CreateNewBlock needs and call that from
CreateNewBlock; then remove the friend class node::BlockAssembler declaration
and ensure addPackageTxs and CreateNewBlock both use GetCreditPool()/the new
public API rather than accessing credit_pool_manager directly.

In `@src/node/interfaces.cpp`:
- Around line 453-465: getPendingAssetUnlocks currently does a linear iteration
over context().mempool->mapTx under the mempool lock to count transactions with
IsPlatformTransfer(), causing O(n) work every call; change this to avoid
repeated full scans by using a maintained counter or index: add/keep an atomic
or guarded pendingPlatformTransferCount in the mempool and update it inside the
mempool’s insertion/removal handlers (where transactions are added/removed)
based on tx.IsPlatformTransfer(), then have getPendingAssetUnlocks simply read
that counter (or query a mempool index) instead of iterating mapTx; reference
functions/fields: getPendingAssetUnlocks, context().mempool, mapTx, and
IsPlatformTransfer to locate the change points.
- Around line 417-434: getCreditPoolCounts currently calls
chain_helper.GetCreditPool twice (once for pindexTip and again for
pindexTip->pprev) which duplicates work; change it to call GetCreditPool once
for the tip (store in a local like tipPool) and only call GetCreditPool for
pprev if pindexTip->pprev exists (store as prevPool) then compute ret.m_locked,
ret.m_limit from tipPool and ret.m_diff = tipPool.locked - prevPool.locked,
keeping the existing LOCK(::cs_main) and return path; update references in
getCreditPoolCounts to use these cached locals (tipPool, prevPool) instead of
repeated GetCreditPool calls.

In `@src/qt/clientfeeds.cpp`:
- Around line 192-195: The code stores raw pointers from entry.get() into
m_by_protx and m_by_service while ownership is held by m_entries, which is
fragile if m_entries is ever modified; update the maps to store either
shared_ptr<MasternodeEntry> (preferred) or stable indices instead of raw
pointers so the maps own/hold a reference independent of m_entries, i.e., change
assignments to ret->m_by_protx[dmn->getProTxHash()] = entry and
ret->m_by_service[util::make_array(dmn->getNetInfoPrimary().GetKey())] = entry
(or store an index into m_entries and update lookup logic), and ensure
MasternodeEntry lifetime is managed consistently with setData and document the
invariant if opting to keep raw pointers.

In `@src/qt/forms/networkwidget.ui`:
- Around line 157-168: Add a missing textFormat property set to Qt::PlainText
for the value QLabel widgets that currently lack it (labelISPending,
labelISWaiting, labelISUnprotected, masternodeCount, evoCount) so they match the
other value labels (e.g., labelCrLastBlock, labelCrLocked, labelISLocks,
bestClHeight); edit each corresponding <widget class="QLabel" name="..."> block
in the UI XML and insert <property
name="textFormat"><enum>Qt::PlainText</enum></property> to prevent unintended
rich-text rendering.

In `@src/qt/informationwidget.h`:
- Around line 36-42: In the InformationWidget class, merge the two private
access specifiers by removing the redundant "private:" line so that
updateNetworkState(), ui (Ui::InformationWidget*), and clientModel (ClientModel*
clientModel{nullptr}) are all declared under a single private section; keep the
existing declarations (void updateNetworkState(); Ui::InformationWidget* ui;
ClientModel* clientModel{nullptr};) and ensure spacing/indentation matches
surrounding style.

In `@src/qt/masternodemodel.h`:
- Line 9: The header currently includes the heavy interfaces/node.h for inline
"raw accessors" (MnEntryCPtr related functions at lines 68-84); instead remove
the `#include` from masternodemodel.h, forward-declare "class MnEntry;" and keep
the MnEntryCPtr typedef/declaration, then turn those inline raw accessor
definitions in masternodemodel.h into out-of-line declarations. Implement those
accessor methods in masternodemodel.cpp and add `#include` <interfaces/node.h>
there so the full MnEntry definition is available only in the .cpp, reducing
header dependencies and build impact.

In `@src/qt/proposallist.cpp`:
- Around line 253-261: The for-range loop over data_mn->m_entries ends with a
stray trailing semicolon (`};`) which is stylistically inconsistent; remove the
extraneous semicolon after the closing brace of the loop body so the loop is
`for (const auto& entry : data_mn->m_entries) { ... }` and leave the loop body
unchanged (identify the loop by the use of entry->isBanned(),
GetScriptForDestination(PKHash(entry->keyIdVotingRaw())),
walletModel->wallet().isSpendable(...), and
votableMasternodes[entry->proTxHashRaw()] = entry->keyIdVotingRaw()).

In `@src/qt/proposalmodel.h`:
- Around line 62-64: The Proposal constructor declaration uses the parameter
name _govObj while the definition uses govObj; update the declaration of
Proposal(ClientModel& client_model, const CGovernanceObject& _govObj, const
interfaces::GOV::GovernanceInfo& govInfo, int collateral_confs, bool
is_broadcast) to use govObj instead of _govObj so the declaration and definition
match (ensure the symbol Proposal and its parameter list are adjusted
accordingly).

Comment on lines 145 to 154
struct Counts {
size_t m_total_evo{0};
size_t m_total_mn{0};
size_t m_valid_evo{0};
size_t m_valid_mn{0};
size_t m_valid_weighted_count{0};

[[nodiscard]] size_t total() const { return m_total_mn + m_total_evo; }
[[nodiscard]] size_t enabled() const { return m_valid_mn + m_valid_evo; }
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# Check the full Counts struct definition in interfaces/node.h
rg -n -A 20 'struct Counts' --type=cpp --type-add 'cpp:*.h' src/interfaces/node.h

Repository: dashpay/dash

Length of output: 929


🏁 Script executed:

# Check line 372 area in deterministicmns.h for the typo
sed -n '368,376p' src/evo/deterministicmns.h

Repository: dashpay/dash

Length of output: 539


🏁 Script executed:

# Verify the GetCounts() implementation in deterministicmns.h
sed -n '267,289p' src/evo/deterministicmns.h

Repository: dashpay/dash

Length of output: 759


🏁 Script executed:

# Check if immer::map is used in the CDeterministicMNList class
rg -n 'immer::map|immer\|mnMap' src/evo/deterministicmns.h | head -20

Repository: dashpay/dash

Length of output: 821


Fix typo in doc comment: remove extra double-quote.

The Counts struct design is solid and correctly matches the interface definition in src/interfaces/node.h. The single-pass GetCounts() implementation is efficient and properly classifies masternodes by type and validity while accumulating voting weights.

Correct the doc comment on line 372 from:

"nCount = max()"" means

to:

"nCount = max()" means
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/evo/deterministicmns.h` around lines 145 - 154, Fix the stray
double-quote in the doc comment that follows the Counts/GetCounts logic: locate
the comment that currently reads `"nCount = max()"" means` (near the Counts
struct / GetCounts explanation) and change it to `"nCount = max()" means` so
there is only a single closing quote; ensure the surrounding comment text
remains unchanged.

* Calculates the projected MN payees for the next *count* blocks. The result is not guaranteed to be correct
* as PoSe banning might occur later
* @param nCount the number of payees to return. "nCount = max()"" means "all", use it to avoid calling GetValidWeightedMNsCount twice.
* @param nCount the number of payees to return. "nCount = max()"" means "all", use it to avoid calling GetCounts twice.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Minor typo: extra double-quote in doc comment.

The comment reads "nCount = max()"" means — there's a stray " after the closing parenthesis.

Proposed fix
-     * `@param` nCount the number of payees to return. "nCount = max()"" means "all", use it to avoid calling GetCounts twice.
+     * `@param` nCount the number of payees to return. "nCount = max()" means "all", use it to avoid calling GetCounts twice.
📝 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.

Suggested change
* @param nCount the number of payees to return. "nCount = max()"" means "all", use it to avoid calling GetCounts twice.
* `@param` nCount the number of payees to return. "nCount = max()" means "all", use it to avoid calling GetCounts twice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/evo/deterministicmns.h` at line 372, Fix the doc comment for the
parameter nCount by removing the stray double-quote after max(): change the text
that currently reads '"nCount = max()"" means' to '"nCount = max()" means' so
the comment about using max() to mean "all" is correctly punctuated; update the
comment near the parameter description for nCount in deterministicmns.h (the
line describing nCount and max()).

Comment on lines +13 to +15
{
return QByteArray(reinterpret_cast<const char*>(data.data()), data.size());
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

Qt 5 QByteArray constructor signature size parameter type

💡 Result:

In Qt 5 (including Qt 5.15), QByteArray constructor “size” parameters are of type int:

  • QByteArray(int size, char ch)
  • QByteArray(const char *data, int size = -1) (where size defaults to -1) [1]

Source: Qt 5.15 QByteArray class reference (constructor list) [1].


🏁 Script executed:

# Check the actual code in src/qt/util.h around lines 13-15
cat -n src/qt/util.h | head -20

Repository: dashpay/dash

Length of output: 662


🏁 Script executed:

# Check for existing namespace util definitions and make_array symbol usage
rg "namespace util" --type cpp --type h

Repository: dashpay/dash

Length of output: 2139


🏁 Script executed:

# Search for any existing make_array references
rg "make_array" --type cpp --type h

Repository: dashpay/dash

Length of output: 328


data.size() implicit narrowing to int for Qt 5's QByteArray.

Qt 5's QByteArray(const char*, int size) takes a signed int for size, but T1::size() on standard containers returns size_t. The implicit signed/unsigned narrowing is silent, will trip -Wconversion/-Wsign-conversion warnings, and truncates for buffers larger than INT_MAX.

🛡️ Proposed fix
-    return QByteArray(reinterpret_cast<const char*>(data.data()), data.size());
+    return QByteArray(reinterpret_cast<const char*>(data.data()), static_cast<int>(data.size()));
📝 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.

Suggested change
{
return QByteArray(reinterpret_cast<const char*>(data.data()), data.size());
}
{
return QByteArray(reinterpret_cast<const char*>(data.data()), static_cast<int>(data.size()));
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/qt/util.h` around lines 13 - 15, The QByteArray construction currently
uses data.size() (size_t) which narrows to int and can trigger warnings or
truncate large buffers; in the function that returns
QByteArray(reinterpret_cast<const char*>(data.data()), data.size()) check that
data.size() <= std::numeric_limits<int>::max() and handle the overflow case
(e.g. log/throw or return an error/empty QByteArray), then pass the size as
static_cast<int>(data.size()) to QByteArray; keep the reinterpret_cast<const
char*>(data.data()) and update this call site to avoid implicit narrowing and
potential truncation.

kwvg added 3 commits February 21, 2026 22:41
Applying our font causes drift to be introduced for some reason if baked
into the layout but this doesn't seem to be an issue if done
programmatically
@kwvg kwvg marked this pull request as ready for review February 21, 2026 17:13
@kwvg kwvg requested review from PastaPastaPasta, UdjinM6 and knst and removed request for PastaPastaPasta, UdjinM6 and knst February 21, 2026 17:13
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)
src/qt/masternodemodel.cpp (1)

1-1: ⚠️ Potential issue | 🟡 Minor

Pipeline failure: Clang-format differences detected.

The CI reports formatting issues in this file. Run clang-format-diff.py -p1 to identify and fix the specific formatting differences before merge.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/qt/masternodemodel.cpp` at line 1, CI flagged clang-format differences in
src/qt/masternodemodel.cpp; run the formatter (e.g., clang-format-diff.py -p1 or
your project's clang-format config) against that file, apply the suggested
edits, and commit the formatted changes so the file matches the repository's
style; verify by re-running the CI or clang-format-diff to ensure no remaining
differences in masternodemodel.cpp (look for formatting around the
MasternodeModel class and its methods to confirm all changes were applied).
src/qt/clientfeeds.h (1)

1-1: ⚠️ Potential issue | 🟡 Minor

Fix clang-format violations to unblock CI.

Run clang-format-diff.py -p1 on the diff to fix formatting.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/qt/clientfeeds.h` at line 1, Clang-format violations were detected in
src/qt/clientfeeds.h; run clang-format-diff.py -p1 on the current diff (or apply
your project's clang-format style to src/qt/clientfeeds.h) to fix
whitespace/formatting issues so CI passes, then stage the reformatted changes
and update the PR; ensure you target the header file and any related symbols
(clientfeeds.h / ClientFeeds) when reformatting.
src/qt/proposallist.cpp (1)

1-1: ⚠️ Potential issue | 🟡 Minor

Fix clang-format violations to unblock CI.

The Clang Diff Format Check pipeline is failing for this file. Run clang-format-diff.py -p1 on the diff and commit the resulting fixes.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/qt/proposallist.cpp` at line 1, Run clang-format on the changes in
src/qt/proposallist.cpp and commit the formatted output to fix CI: run the
project's recommended command (e.g. clang-format-diff.py -p1 on the current diff
or clang-format with the project's style) to apply the code style rules to
proposallist.cpp, stage the resulting changes, and push them; ensure the file
compiles after formatting and that no functional code is altered (only
whitespace/formatting adjustments).
src/qt/clientfeeds.cpp (1)

1-1: ⚠️ Potential issue | 🟡 Minor

Fix clang-format violations to unblock CI.

Run clang-format-diff.py -p1 on the diff to fix formatting.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/qt/clientfeeds.cpp` at line 1, The file has clang-format style
violations; run the formatter on the change and commit the result: generate a
clang-format patch for the diff (e.g., run clang-format-diff.py -p1 on the
current diff or run clang-format on the file containing the header "Copyright
(c) 2026 The Dash Core developers"), apply the formatted changes, and update the
PR so CI passes.
🧹 Nitpick comments (5)
src/qt/res/css/general.css (1)

1696-1706: CSS selectors correctly updated for new widget decomposition.

The selectors properly target the new InformationWidget and NetworkWidget IDs, covering the relevant section header labels. The shared AlignLeft | AlignBottom alignment and min-height: 25px provide consistent header styling across both widgets.

Minor nit: the comment on line 1703 still reads /* Margin between Network and Block Chain headers */, which is a carryover from the old single-widget layout. The rule now styles section headers across two distinct widgets (including CreditPool, InstantSend, Masternodes, Quorums). Consider updating the comment to reflect the broader scope.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/qt/res/css/general.css` around lines 1696 - 1706, Update the inline
comment to accurately reflect the new scope of the rule: change the outdated "/*
Margin between Network and Block Chain headers */" to a brief description that
mentions it applies to section header labels across both InformationWidget and
NetworkWidget (e.g., "/* Styling for section header labels in InformationWidget
and NetworkWidget */"); locate the rule targeting QWidget#InformationWidget
QLabel#labelNetwork, QLabel#label_10, QLabel#labelMempoolTitle and
QWidget#NetworkWidget QLabel#labelChainLocks, QLabel#labelCreditPool,
QLabel#labelInstantSend, QLabel#labelMasternodes, QLabel#labelQuorums and
replace only the comment text without altering the qproperty-alignment or
min-height properties.
src/qt/masternodemodel.h (1)

29-29: Consider adding an assert to enforce m_dmn is non-null in the constructor.

The new raw accessors (lines 68–72, 84) unconditionally dereference m_dmn. If a null MnEntryCPtr were ever passed, this would cause a null-pointer dereference. While this should be a programmer error (the MnEntry constructor requires a valid CDeterministicMNCPtr), an assert(m_dmn) in the MasternodeEntry constructor would catch it early in development.

Based on learnings, in C++/Qt codebases, prefer fail-fast asserts for programmer errors that should be caught in development and debugging, rather than defensive runtime checks.

The assert should be placed in the constructor body in masternodemodel.cpp. Ensure the constructor includes:

assert(m_dmn);

Also applies to: 55-55, 68-72, 84-84

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/qt/masternodemodel.h` at line 29, Add a fail-fast assert to ensure the
MnEntryCPtr is non-null by inserting assert(m_dmn); into the body of the
MasternodeEntry constructor implementation in masternodemodel.cpp (the
constructor that initializes the member interfaces::MnEntryCPtr m_dmn), so the
raw accessors that dereference m_dmn (used by methods referenced around the raw
accessors) cannot cause null-pointer dereference; replicate the same assert in
any other MasternodeEntry-like constructors/initializers that set m_dmn where
the review flagged (the other constructor/initializer sites referenced by the
comment).
src/qt/networkwidget.h (1)

1-2: Copyright header: Bitcoin Core copyright may be inappropriate for a new Dash-specific file.

This is a brand-new widget not derived from Bitcoin Core code. The 2011-2021 The Bitcoin Core developers copyright line seems misplaced. Other new Dash-specific files in this PR (like masternodemodel.cpp) use only the Dash Core copyright. Consider using only:

// Copyright (c) 2014-2026 The Dash Core developers
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/qt/networkwidget.h` around lines 1 - 2, Remove the incorrect Bitcoin Core
copyright line from src/qt/networkwidget.h and retain only the Dash Core
copyright; specifically delete the line containing "The Bitcoin Core developers"
so the header reads only the Dash copyright (i.e., ensure the top-of-file header
matches other new Dash-specific files like masternodemodel.cpp and contains only
"// Copyright (c) 2014-2026 The Dash Core developers").
src/qt/guiutil.cpp (1)

1234-1244: formatBlockDuration — consider guarding against non-positive spacing_seconds.

If spacing_seconds is 0 (or negative), the function will compute secs = 0 (or negative) for any positive blocks and return "0 minute(s)", which is misleading. In practice, spacing_seconds comes from Params().GetConsensus().nPowTargetSpacing and should always be positive, but a defensive check could prevent confusing output.

🛡️ Optional: guard against non-positive spacing
 QString formatBlockDuration(int blocks, int64_t spacing_seconds)
 {
-    if (blocks <= 0) return QObject::tr("now");
+    if (blocks <= 0 || spacing_seconds <= 0) return QObject::tr("now");
     const double secs = static_cast<double>(blocks) * static_cast<double>(spacing_seconds);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/qt/guiutil.cpp` around lines 1234 - 1244, The function
formatBlockDuration should guard against non-positive spacing_seconds to avoid
returning misleading "0 minute(s)"; add an early check in formatBlockDuration
for spacing_seconds <= 0 and return a clear fallback translation (e.g.
QObject::tr("unknown")) when that occurs, leaving the rest of the computation
(secs, MINUTE/HOUR/DAY/MONTH/YEAR and the existing QObject::tr("%n ..."))
unchanged; reference the spacing_seconds parameter and the formatBlockDuration
function to locate where to add the guard.
src/qt/clientfeeds.cpp (1)

156-193: dmn is shadowed three times in MasternodeFeed::fetch(), obscuring which object is in scope.

The outer structured binding dmn (line 156, a CDeterministicMNListPtr), the for-loop variable dmn (line 174, a projected-payee shared pointer), and the lambda parameter dmn (line 178, a per-MN shared pointer) all share the same name. The logic is correct, but the shadowing makes it easy to misread which dmn is referenced on any given line.

♻️ Suggested rename
-    const auto [dmn, pindex] = m_client_model.node().evo().getListAtChainTip();
+    const auto [mn_list, pindex] = m_client_model.node().evo().getListAtChainTip();
-    if (!dmn || !pindex) {
+    if (!mn_list || !pindex) {
         return;
     }
 
-    auto projectedPayees = dmn->getProjectedMNPayees(pindex);
-    if (projectedPayees.empty() && dmn->getCounts().enabled() > 0) {
+    auto projectedPayees = mn_list->getProjectedMNPayees(pindex);
+    if (projectedPayees.empty() && mn_list->getCounts().enabled() > 0) {
         return;
     }
 
     auto ret = std::make_shared<Data>();
-    ret->m_list_height = dmn->getHeight();
-    ret->m_counts = dmn->getCounts();
+    ret->m_list_height = mn_list->getHeight();
+    ret->m_counts = mn_list->getCounts();
 
     Uint256HashMap<int> nextPayments;
     for (size_t i = 0; i < projectedPayees.size(); i++) {
-        const auto& dmn = projectedPayees[i];
-        nextPayments.emplace(dmn->getProTxHash(), ...);
+        const auto& payee = projectedPayees[i];
+        nextPayments.emplace(payee->getProTxHash(), ...);
     }
 
-    dmn->forEachMN(/*only_valid=*/false, [&](const auto& dmn) {
+    mn_list->forEachMN(/*only_valid=*/false, [&](const auto& dmn) {
         ...
     });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/qt/clientfeeds.cpp` around lines 156 - 193, In MasternodeFeed::fetch()
the name dmn is shadowed by three different variables (the structured binding
from getListAtChainTip, the projectedPayees loop variable, and the lambda
parameter to forEachMN); rename them to distinct, descriptive identifiers:
change the outer CDeterministicMNListPtr binding (currently dmn) to something
like dmn_list or dmnPtr and update uses (getHeight(), getCounts(),
getProjectedMNPayees call); rename the loop variable over projectedPayees to
projected or projPayee (used when inserting into nextPayments); and rename the
lambda parameter in forEachMN to mn or mnEntry and update all references inside
the lambda (getCollateralOutpoint(), getProTxHash(), getNetInfoPrimary(), etc.)
so no identifier is shadowed. Ensure all modified references
(ret->m_list_height, nextPayments emplace, ret->m_by_protx, ret->m_by_service,
ret->m_entries) use the new names consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/instantsend/net_instantsend.cpp`:
- Line 8: The CI clang-format failure is caused by incorrect include ordering:
move or reorder the include lines so they follow the project's sorted include
convention (put <cxxtimer.hpp> before <node/interface_ui.h>, or run the
repository's clang-format configuration on src/instantsend/net_instantsend.cpp)
so the header ordering matches the project's style.

In `@src/qt/networkwidget.cpp`:
- Line 1: There are clang-format style violations in NetworkWidget
(networkwidget.cpp); run the repository's formatting script
(clang-format-diff.py -p1) or apply clang-format with the project's style to the
modified hunks in networkwidget.cpp (or run clang-format on the file) to fix
spacing/indentation and header comment formatting, then stage and commit the
updated formatting so CI passes.

---

Outside diff comments:
In `@src/qt/clientfeeds.cpp`:
- Line 1: The file has clang-format style violations; run the formatter on the
change and commit the result: generate a clang-format patch for the diff (e.g.,
run clang-format-diff.py -p1 on the current diff or run clang-format on the file
containing the header "Copyright (c) 2026 The Dash Core developers"), apply the
formatted changes, and update the PR so CI passes.

In `@src/qt/clientfeeds.h`:
- Line 1: Clang-format violations were detected in src/qt/clientfeeds.h; run
clang-format-diff.py -p1 on the current diff (or apply your project's
clang-format style to src/qt/clientfeeds.h) to fix whitespace/formatting issues
so CI passes, then stage the reformatted changes and update the PR; ensure you
target the header file and any related symbols (clientfeeds.h / ClientFeeds)
when reformatting.

In `@src/qt/masternodemodel.cpp`:
- Line 1: CI flagged clang-format differences in src/qt/masternodemodel.cpp; run
the formatter (e.g., clang-format-diff.py -p1 or your project's clang-format
config) against that file, apply the suggested edits, and commit the formatted
changes so the file matches the repository's style; verify by re-running the CI
or clang-format-diff to ensure no remaining differences in masternodemodel.cpp
(look for formatting around the MasternodeModel class and its methods to confirm
all changes were applied).

In `@src/qt/proposallist.cpp`:
- Line 1: Run clang-format on the changes in src/qt/proposallist.cpp and commit
the formatted output to fix CI: run the project's recommended command (e.g.
clang-format-diff.py -p1 on the current diff or clang-format with the project's
style) to apply the code style rules to proposallist.cpp, stage the resulting
changes, and push them; ensure the file compiles after formatting and that no
functional code is altered (only whitespace/formatting adjustments).

---

Duplicate comments:
In `@src/qt/informationwidget.cpp`:
- Around line 43-68: The setClientModel method overwrites clientModel without
disconnecting signals from any previous model; before assigning the new model
(and also when model is nullptr) call QObject::disconnect on the existing
clientModel to remove its connections to this widget (e.g.,
disconnect(clientModel, nullptr, this, nullptr) or explicit disconnects for
ClientModel::numConnectionsChanged, networkActiveChanged, mempoolSizeChanged) so
stale handlers don't fire; then assign clientModel = model and proceed to
connect the new model (or skip connections if model is nullptr).

---

Nitpick comments:
In `@src/qt/clientfeeds.cpp`:
- Around line 156-193: In MasternodeFeed::fetch() the name dmn is shadowed by
three different variables (the structured binding from getListAtChainTip, the
projectedPayees loop variable, and the lambda parameter to forEachMN); rename
them to distinct, descriptive identifiers: change the outer
CDeterministicMNListPtr binding (currently dmn) to something like dmn_list or
dmnPtr and update uses (getHeight(), getCounts(), getProjectedMNPayees call);
rename the loop variable over projectedPayees to projected or projPayee (used
when inserting into nextPayments); and rename the lambda parameter in forEachMN
to mn or mnEntry and update all references inside the lambda
(getCollateralOutpoint(), getProTxHash(), getNetInfoPrimary(), etc.) so no
identifier is shadowed. Ensure all modified references (ret->m_list_height,
nextPayments emplace, ret->m_by_protx, ret->m_by_service, ret->m_entries) use
the new names consistently.

In `@src/qt/guiutil.cpp`:
- Around line 1234-1244: The function formatBlockDuration should guard against
non-positive spacing_seconds to avoid returning misleading "0 minute(s)"; add an
early check in formatBlockDuration for spacing_seconds <= 0 and return a clear
fallback translation (e.g. QObject::tr("unknown")) when that occurs, leaving the
rest of the computation (secs, MINUTE/HOUR/DAY/MONTH/YEAR and the existing
QObject::tr("%n ...")) unchanged; reference the spacing_seconds parameter and
the formatBlockDuration function to locate where to add the guard.

In `@src/qt/masternodemodel.h`:
- Line 29: Add a fail-fast assert to ensure the MnEntryCPtr is non-null by
inserting assert(m_dmn); into the body of the MasternodeEntry constructor
implementation in masternodemodel.cpp (the constructor that initializes the
member interfaces::MnEntryCPtr m_dmn), so the raw accessors that dereference
m_dmn (used by methods referenced around the raw accessors) cannot cause
null-pointer dereference; replicate the same assert in any other
MasternodeEntry-like constructors/initializers that set m_dmn where the review
flagged (the other constructor/initializer sites referenced by the comment).

In `@src/qt/networkwidget.h`:
- Around line 1-2: Remove the incorrect Bitcoin Core copyright line from
src/qt/networkwidget.h and retain only the Dash Core copyright; specifically
delete the line containing "The Bitcoin Core developers" so the header reads
only the Dash copyright (i.e., ensure the top-of-file header matches other new
Dash-specific files like masternodemodel.cpp and contains only "// Copyright (c)
2014-2026 The Dash Core developers").

In `@src/qt/res/css/general.css`:
- Around line 1696-1706: Update the inline comment to accurately reflect the new
scope of the rule: change the outdated "/* Margin between Network and Block
Chain headers */" to a brief description that mentions it applies to section
header labels across both InformationWidget and NetworkWidget (e.g., "/* Styling
for section header labels in InformationWidget and NetworkWidget */"); locate
the rule targeting QWidget#InformationWidget QLabel#labelNetwork,
QLabel#label_10, QLabel#labelMempoolTitle and QWidget#NetworkWidget
QLabel#labelChainLocks, QLabel#labelCreditPool, QLabel#labelInstantSend,
QLabel#labelMasternodes, QLabel#labelQuorums and replace only the comment text
without altering the qproperty-alignment or min-height properties.

#include <instantsend/net_instantsend.h>

#include <bls/bls_batchverifier.h>
#include <node/interface_ui.h>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix clang-format issue flagged by CI.

The pipeline reports a clang-format difference in this file. The include ordering likely needs adjustment — <node/interface_ui.h> appears before <cxxtimer.hpp>, but the project's clang-format rules may expect a different sort order.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/instantsend/net_instantsend.cpp` at line 8, The CI clang-format failure
is caused by incorrect include ordering: move or reorder the include lines so
they follow the project's sorted include convention (put <cxxtimer.hpp> before
<node/interface_ui.h>, or run the repository's clang-format configuration on
src/instantsend/net_instantsend.cpp) so the header ordering matches the
project's style.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
src/qt/bitcoingui.cpp (1)

522-523: Consider adding a showDebugLog() wrapper for consistency.

Every other file-opening action (openConfEditorActionshowConfEditor(), showBackupsActionshowBackups()) routes through a thin member slot with this as the connection context. Connecting directly to the free function works (the action is a child of this, so lifetime is safe), but it deviates from the established pattern.

♻️ Suggested refactor

In bitcoingui.h (public Q_SLOTS section, near showConfEditor/showBackups):

+    /** Open the debug log file */
+    void showDebugLog();

In bitcoingui.cpp:

-    connect(openDebugLogAction, &QAction::triggered, GUIUtil::openDebugLogfile);
+    connect(openDebugLogAction, &QAction::triggered, this, &BitcoinGUI::showDebugLog);
+void BitcoinGUI::showDebugLog()
+{
+    GUIUtil::openDebugLogfile();
+}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/qt/bitcoingui.cpp` around lines 522 - 523, Add a thin member slot
showDebugLog() in the BitcoinGUI class and wire openDebugLogAction to that slot
instead of directly to GUIUtil::openDebugLogfile to match existing patterns
(showConfEditor, showBackups). Update the header to declare Q_SLOTS: void
showDebugLog(); and implement BitcoinGUI::showDebugLog() in bitcoingui.cpp to
call GUIUtil::openDebugLogfile(), preserving the same connection context (this)
when connecting openDebugLogAction to the new slot.
src/qt/clientmodel.cpp (1)

95-98: Redundant null guards for unconditionally-initialized feeds.

m_feed_creditpool, m_feed_masternode, and m_feed_quorum are always initialized (lines 75, 77, 81) — their null guards here are dead code. Only m_feed_proposal requires a guard (conditional on m_node.gov().isEnabled()).

♻️ Proposed cleanup
-            if (m_feed_creditpool) m_feed_creditpool->requestRefresh();
-            if (m_feed_masternode) m_feed_masternode->requestRefresh();
+            m_feed_creditpool->requestRefresh();
+            m_feed_masternode->requestRefresh();
             if (m_feed_proposal) m_feed_proposal->requestRefresh();
-            if (m_feed_quorum) m_feed_quorum->requestRefresh();
+            m_feed_quorum->requestRefresh();
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/qt/clientmodel.cpp` around lines 95 - 98, The null checks around
m_feed_creditpool, m_feed_masternode, and m_feed_quorum are redundant because
those members are unconditionally initialized (see their initializations at
m_feed_creditpool, m_feed_masternode, m_feed_quorum); remove the if (...) guards
and call their requestRefresh() directly, leaving the conditional guard only for
m_feed_proposal (which depends on m_node.gov().isEnabled()). Update the block
where requestRefresh() is invoked (the calls to
m_feed_creditpool->requestRefresh(), m_feed_masternode->requestRefresh(),
m_feed_proposal->requestRefresh(), m_feed_quorum->requestRefresh()) to eliminate
the dead null checks while preserving the existing conditional for
m_feed_proposal.
ℹ️ Review info

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 063d62d and f85a459.

📒 Files selected for processing (29)
  • src/Makefile.qt.include
  • src/instantsend/net_instantsend.cpp
  • src/interfaces/node.h
  • src/node/interface_ui.cpp
  • src/node/interface_ui.h
  • src/node/interfaces.cpp
  • src/qt/bitcoingui.cpp
  • src/qt/bitcoingui.h
  • src/qt/clientfeeds.cpp
  • src/qt/clientfeeds.h
  • src/qt/clientmodel.cpp
  • src/qt/clientmodel.h
  • src/qt/forms/debugwindow.ui
  • src/qt/forms/informationwidget.ui
  • src/qt/forms/networkwidget.ui
  • src/qt/guiutil.cpp
  • src/qt/guiutil.h
  • src/qt/informationwidget.cpp
  • src/qt/informationwidget.h
  • src/qt/masternodemodel.cpp
  • src/qt/networkwidget.cpp
  • src/qt/networkwidget.h
  • src/qt/res/css/dark.css
  • src/qt/res/css/general.css
  • src/qt/res/css/light.css
  • src/qt/res/css/traditional.css
  • src/qt/rpcconsole.cpp
  • src/qt/rpcconsole.h
  • test/util/data/non-backported.txt
🚧 Files skipped from review as they are similar to previous changes (10)
  • src/Makefile.qt.include
  • src/instantsend/net_instantsend.cpp
  • src/qt/forms/debugwindow.ui
  • src/qt/guiutil.h
  • src/qt/networkwidget.h
  • src/node/interface_ui.cpp
  • src/qt/forms/informationwidget.ui
  • test/util/data/non-backported.txt
  • src/qt/informationwidget.h
  • src/node/interface_ui.h
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/qt/bitcoingui.cpp`:
- Around line 1818-1831: The tooltip is awkward when remaining_blocks == 0
because GUIUtil::formatBlockDuration returns "now" and the code prefixes it with
"~"; update the tooltip construction in the else branch (where remaining_str is
used) to short-circuit when remaining_blocks == 0 (or remaining_str == "now")
and omit the leading tilde—e.g., detect remaining_blocks == 0 and set tooltip1
using tr("now (%1 blocks) left for voting") or similar, then otherwise keep the
existing "~%1 (%2 blocks) left for voting" behavior; adjust both tooltip
variants if needed and reference remaining_blocks, remaining_str, tooltip1 and
labelGovernanceCycleIcon.

---

Nitpick comments:
In `@src/qt/bitcoingui.cpp`:
- Around line 522-523: Add a thin member slot showDebugLog() in the BitcoinGUI
class and wire openDebugLogAction to that slot instead of directly to
GUIUtil::openDebugLogfile to match existing patterns (showConfEditor,
showBackups). Update the header to declare Q_SLOTS: void showDebugLog(); and
implement BitcoinGUI::showDebugLog() in bitcoingui.cpp to call
GUIUtil::openDebugLogfile(), preserving the same connection context (this) when
connecting openDebugLogAction to the new slot.

In `@src/qt/clientmodel.cpp`:
- Around line 95-98: The null checks around m_feed_creditpool,
m_feed_masternode, and m_feed_quorum are redundant because those members are
unconditionally initialized (see their initializations at m_feed_creditpool,
m_feed_masternode, m_feed_quorum); remove the if (...) guards and call their
requestRefresh() directly, leaving the conditional guard only for
m_feed_proposal (which depends on m_node.gov().isEnabled()). Update the block
where requestRefresh() is invoked (the calls to
m_feed_creditpool->requestRefresh(), m_feed_masternode->requestRefresh(),
m_feed_proposal->requestRefresh(), m_feed_quorum->requestRefresh()) to eliminate
the dead null checks while preserving the existing conditional for
m_feed_proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants