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

[kwvg]

Additional Information

• Depends on refactor(qt): UI refresh (1/n, runtime show/hide of "Governance" and "Masternode" tabs, split out proposal model, additional bounds checking, better QTextEdit styling) #7112
• Depends on feat(qt): UI refresh (2/n, update "Governance" Tab layout, lifecycle icons, description dialogs, resumable proposal creation) #7110
• Depends on feat(qt): introduce framework for sourcing and applying data, use for {Masternode,Proposal}Lists  #7146
• Dependency for feat(qt): UI refresh (5/n, add proposal information widget to information, donut chart for proposal allocation) #7159

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)

[github-actions]

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

[thephez]

@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]

This pull request has conflicts, please rebase.

[kwvg]

@coderabbitai review

[coderabbitai]

✅ 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]

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

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).

[coderabbitai]

⚠️ 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.

[coderabbitai]

⚠️ 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()).

[coderabbitai]

⚠️ 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.

[coderabbitai]

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 (openConfEditorAction → showConfEditor(), showBackupsAction → showBackups()) 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.

[UdjinM6]

Nice! Why introducing formatAmount() though? pls consider b583b52

[kwvg]

@UdjinM6 floorWithUnit is meant to respect the decimal granularity settings for account balances, in this case it's not beneficial to show a proposal is requesting 250.00000000 DASH even if it is valuable to show they have a balance of 1.55874563 DASH, for proposals and other global data, it makes the tooltips painful to read.

So this is a separate helper that explicitly allows for more aggressive fixed truncation.

[PastaPastaPasta]

utACK f85a459

[UdjinM6]

@UdjinM6 floorWithUnit is meant to respect the decimal granularity settings for account balances, in this case it's not beneficial to show a proposal is requesting 250.00000000 DASH even if it is valuable to show they have a balance of 1.55874563 DASH, for proposals and other global data, it makes the tooltips painful to read.

So this is a separate helper that explicitly allows for more aggressive fixed truncation.

Well, actually... "last block change" and "total locked" are stats. Why truncate them at all? Not a blocker though, can be tweaked later.

[UdjinM6]

light ACK f85a459

[knst]

m_feed_masternode->data() can be replaced; it will destroy all shared_ptr objects inside and dmn will be dangling pointer.

[knst]

this one [and probably not the only one] is backported function, why have it been moved?

[kwvg]

Because the layout itself was changed from flat layout to widgets, all the upstream-inherited changes needed to be isolated (as the General widget) from the Dash-specific changes to the former-flat layout (as the Network widget), it will introduce some backport friction but the feature improvements should compensate for it, upstream changes to the layout shouldn't cause too much conflict as they are mostly move-only