feat(qt): UI refresh (2/n, update "Governance" Tab layout, lifecycle icons, description dialogs, resumable proposal creation)#7110
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR replaces the single ProposalWizard flow with two dialogs: ProposalCreate (create/prepare proposals) and ProposalResume (resume/broadcast pending wallet proposals). It adds a Proposal and ProposalModel with richer metadata and voting counts, updates GovernanceList to support Active vs Local sources and wallet-backed proposals, extends node and wallet interfaces with governance queries (vote counts, existence, funded height, fundable hashes), and updates multiple Qt UI files, headers, moc entries, wallet implementations, CSS, and governance manager signatures. Sequence Diagram(s)sequenceDiagram
participant User
participant ProposalCreate as ProposalCreate (UI)
participant WalletModel as WalletModel (Wallet)
participant Node as Node (interfaces)
participant Governance as Governance (manager)
User->>ProposalCreate: Open dialog, enter fields
ProposalCreate->>Node: Request governance params (superblock, relay confs)
Node-->>ProposalCreate: Return params
ProposalCreate->>ProposalCreate: buildJsonAndHex() / validateFields()
User->>ProposalCreate: Confirm create
ProposalCreate->>WalletModel: Unlock & prepare transaction
WalletModel-->>ProposalCreate: Prepared tx / signing
ProposalCreate->>Governance: Submit proposal payload via Node/Gov API
Governance-->>ProposalCreate: Return proposal hash/status
ProposalCreate->>User: Show result (success/error)
sequenceDiagram
participant User
participant GovernanceList as GovernanceList (UI)
participant ProposalResume as ProposalResume (UI)
participant WalletModel as WalletModel (Wallet)
participant Node as Node (interfaces)
participant Governance as Governance (manager)
User->>GovernanceList: Select "Local" proposals / Open Resume
GovernanceList->>ProposalResume: Open with wallet proposals
ProposalResume->>Node: Query relay required confirmations
Node-->>ProposalResume: Relay confirmations threshold
loop polling
ProposalResume->>WalletModel: Query collateral confirmations for each proposal
WalletModel-->>ProposalResume: Return confirmation counts
alt confirmations met
ProposalResume->>User: Enable Broadcast button
end
end
User->>ProposalResume: Click Broadcast
ProposalResume->>Governance: Broadcast proposal object
Governance-->>ProposalResume: Success / Error
ProposalResume->>GovernanceList: Remove/refresh entry
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/qt/guiutil_font.cpp (1)
17-25: Run clang-format on this file (CI is failing).
The Clang Diff Format Check reports formatting differences in this file; please reformat before merge.src/interfaces/node.h (1)
20-26: Add missing<optional>include forstd::optional.
The file usesstd::optionalin the function signatures at lines 156–157 but doesn't include the required<optional>header, which will cause compilation failures.🛠️ Suggested fix
`#include` <functional> `#include` <memory> +#include <optional> `#include` <stddef.h> `#include` <stdint.h> `#include` <string>Also applies to: 156–157
src/qt/governancelist.cpp (1)
1-1: Address clang-format pipeline failure.The pipeline indicates clang-format differences in this file. Please run
clang-formatto fix formatting before merging.
🤖 Fix all issues with AI agents
In `@src/Makefile.qt.include`:
- Around line 34-36: The non-backported list is missing the new C++ proposal
sources: add entries for src/qt/proposaldesc.*, src/qt/proposaldescdialog.*, and
src/qt/proposalmodel.* to test/util/data/non-backported.txt (do not add any .ui
form files); note that src/qt/proposalwizard.* is already registered so no
change needed for it.
In `@src/node/interfaces.cpp`:
- Around line 310-316: The call to context().chainman->ActiveChain().Height()
inside getProposalFundedHeight is not protected by the cs_main lock; wrap the
ActiveChain().Height() access in LOCK(::cs_main) (as done in getNumBlocks() /
getLastBlockTime()) to prevent a race. Add a scoped LOCK(::cs_main) before
reading nTipHeight so the loop over context()->govman triggers uses a
thread-safe snapshot of the active chain height.
In `@src/qt/guiutil_font.h`:
- Around line 84-86: Run clang-format on src/qt/guiutil_font.h to fix CI
formatting failures; reformat the declarations/Getters (GetScaleSteps,
GetScaledFontSize, GetFont) and surrounding spacing/line breaks so they match
the project's clang-format rules, then stage the updated header (ensuring member
names m_scale_steps, m_font_scale, m_font remain unchanged) and push the
formatted file.
In `@src/qt/proposalmodel.cpp`:
- Around line 354-361: In ProposalModel::setVotingParams the dataChanged call
uses createIndex(rowCount(), Column::VOTING_STATUS) which can create an
out-of-range QModelIndex; fix by guarding for empty model and emitting
dataChanged only when rowCount() > 0, using createIndex(0,
Column::VOTING_STATUS) as the first index and createIndex(rowCount() - 1,
Column::VOTING_STATUS) as the last index so the range is valid; update the
conditional in setVotingParams to check rowCount() before calling Q_EMIT
dataChanged.
- Around line 364-366: The getProposalAt method can dereference m_data with an
invalid QModelIndex and crash; update ProposalModel::getProposalAt to validate
the index before accessing m_data by checking index.isValid() and that
index.row() is within the range 0 .. m_data.size()-1 (and optionally that
index.model() == this), and return nullptr for any invalid or out-of-range index
instead of indexing m_data directly.
- Around line 239-242: Replace the hard-coded English words with translatable
strings: use tr("Voting") and tr("Voted") for the status (from
proposal->status() / ProposalStatus::Voting) and use Qt's plural-aware tr() for
the block text by switching the message to a pluralized pattern such as tr("%1,
%n block(s) till superblock", "", blocks) (where blocks is from
proposal->blocksUntilSuperblock()) so translators can provide correct
singular/plural forms.
🧹 Nitpick comments (9)
src/qt/guiutil_font.cpp (1)
562-567: Preserve per‑widget base font size for registeredQTextEdits.
updateFonts()re-applies styling withDEFAULT_FONT_SIZEfor all registered text edits, which can override custom base sizes. If anyQTextEdituses a non-default base size, consider storing the base size at registration time and reusing it here.♻️ Possible approach
-std::map<QPointer<QTextEdit>, QString> mapTextEditStyleUpdates; +struct TextEditStyleUpdate { + QString html; + double base_point_size{0}; +}; +std::map<QPointer<QTextEdit>, TextEditStyleUpdate> mapTextEditStyleUpdates;- for (const auto& [widget, html] : mapTextEditStyleUpdates) { - setFontBodyHTML(widget, html, - getScaledFont(FontRegistry::DEFAULT_FONT_SIZE, /*bold=*/false), - getScaledFont(FontRegistry::DEFAULT_FONT_SIZE, /*bold=*/true)); + for (const auto& [widget, style] : mapTextEditStyleUpdates) { + const double base_size = style.base_point_size > 0 ? style.base_point_size + : FontRegistry::DEFAULT_FONT_SIZE; + setFontBodyHTML(widget, style.html, + getScaledFont(base_size, /*bold=*/false), + getScaledFont(base_size, /*bold=*/true)); }- mapTextEditStyleUpdates[widget] = html; + mapTextEditStyleUpdates[widget] = {html, widget->font().pointSizeF()};src/qt/proposaldescdialog.cpp (2)
5-13: Consider reordering includes for consistency.The include order could be improved for consistency with typical project conventions (own header first, then related headers, then Qt headers):
♻️ Suggested reordering
-#include <qt/forms/ui_proposaldescdialog.h> - -#include <qt/proposaldesc.h> `#include` <qt/proposaldescdialog.h> + +#include <qt/forms/ui_proposaldescdialog.h> +#include <qt/proposaldesc.h> `#include` <qt/proposalmodel.h> -#include <qt/guiutil_font.h> - `#include` <qt/guiutil.h> +#include <qt/guiutil_font.h>Also,
guiutil_font.happears unused since no direct font functions from that header are called in this file.
15-24: Consider adding a null check for the proposal pointer.The constructor dereferences
proposalwithout validating it. While callers may guarantee non-null, defensive programming would prevent potential crashes.🛡️ Suggested defensive check
ProposalDescDialog::ProposalDescDialog(const Proposal* proposal, BitcoinUnit unit, QWidget* parent) : QDialog(parent, GUIUtil::dialog_flags), ui(new Ui::ProposalDescDialog) { + assert(proposal != nullptr); ui->setupUi(this); setWindowTitle(tr("Details for %1").arg(proposal->title())); GUIUtil::registerWidget(ui->detailText, ProposalDesc::toHTML(proposal, unit)); GUIUtil::updateFonts(); GUIUtil::handleCloseWindowShortcut(this); }Based on learnings, fail-fast asserts are preferred for programmer errors that should be caught during development.
src/qt/proposaldesc.cpp (2)
14-34: Consider HTML-escaping hash values for consistency.While hash values are typically safe hex strings, applying
HtmlEscapetohash(),parentHash(), andcollateralHash()would provide defense-in-depth and maintain consistency with other fields.♻️ Suggested change
- strHTML += "<b>" + tr("Object Hash") + ":</b> " + proposal->hash() + "<br>"; - strHTML += "<b>" + tr("Parent Hash") + ":</b> " + proposal->parentHash() + "<br>"; + strHTML += "<b>" + tr("Object Hash") + ":</b> " + GUIUtil::HtmlEscape(proposal->hash()) + "<br>"; + strHTML += "<b>" + tr("Parent Hash") + ":</b> " + GUIUtil::HtmlEscape(proposal->parentHash()) + "<br>"; strHTML += "<b>" + tr("Collateral Date") + ":</b> " + GUIUtil::dateTimeStr(proposal->collateralDate()) + "<br>"; - strHTML += "<b>" + tr("Collateral Hash") + ":</b> " + proposal->collateralHash() + "<br>"; + strHTML += "<b>" + tr("Collateral Hash") + ":</b> " + GUIUtil::HtmlEscape(proposal->collateralHash()) + "<br>";
18-18: Minor: HTML structure could be more complete.The HTML starts with
<html>but lacks<body>tags. While Qt's QTextEdit typically handles incomplete HTML gracefully, a proper structure would be more robust.src/qt/forms/governancelist.ui (1)
47-67: Add a buddy to the filter label for keyboard focus.This improves keyboard navigation (Alt+key focuses the filter input).
♿ Suggested UI tweak
<widget class="QLabel" name="label_filter_2"> <property name="text"> <string>Filter List:</string> </property> + <property name="buddy"> + <cstring>filterLineEdit</cstring> + </property> </widget>src/qt/governancelist.h (1)
33-36: Include<cstdint>foruint8_t.Avoid relying on transitive includes for fixed-width integer types.
♻️ Proposed include
`#include` <QWidget> +#include <cstdint> + `#include` <map> `#include` <memory>src/qt/bitcoingui.cpp (1)
1470-1500: Hardcoded toolbar action indices are fragile.Lines 1478 and 1494 use
appToolBar->actions()[5]andappToolBar->actions()[6]respectively. This mirrors the existing pattern at line 1460 for CoinJoin (actions()[4]), but hardcoded indices are error-prone — if buttons are reordered or added, these will silently break.Consider storing action references when buttons are added, or adding a comment documenting the expected order. This is a maintenance concern rather than a blocking issue since it follows the existing pattern.
src/qt/proposalmodel.h (1)
27-31: Q_OBJECT macro placement is unconventional.The
Q_OBJECTmacro is placed in aprivate:section. While this compiles, the Qt convention is to place it at the very beginning of the class body (before any access specifier). This is a minor style nit.Suggested adjustment
class Proposal : public QObject { -private: Q_OBJECT +private: ClientModel* clientModel;
| double GetScaleSteps() const { return m_scale_steps; } | ||
| double GetScaledFontSize(int size) const { return std::round(size * (1 + (m_font_scale * m_scale_steps)) * 4) / 4.0; } | ||
| double GetScaledFontSize(double size) const { return std::round(size * (1 + (m_font_scale * m_scale_steps)) * 4) / 4.0; } | ||
| QString GetFont() const { return m_font; } |
There was a problem hiding this comment.
Run clang-format on this header (CI is failing).
The Clang Diff Format Check reports formatting differences here; please reformat before merge.
🤖 Prompt for AI Agents
In `@src/qt/guiutil_font.h` around lines 84 - 86, Run clang-format on
src/qt/guiutil_font.h to fix CI formatting failures; reformat the
declarations/Getters (GetScaleSteps, GetScaledFontSize, GetFont) and surrounding
spacing/line breaks so they match the project's clang-format rules, then stage
the updated header (ensuring member names m_scale_steps, m_font_scale, m_font
remain unchanged) and push the formatted file.
kwvg
force-pushed
the
gov_refresh
branch
3 times, most recently
from
fc8a95a to
4490d40
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/qt/proposalmodel.cpp`:
- Around line 30-51: The Proposal constructor leaves m_paymentAmount
uninitialized if JSON parsing fails or "payment_amount" is absent; initialize
m_paymentAmount to a safe default (e.g., 0.0) either in the Proposal member
initializer list or immediately before parsing in the
Proposal::Proposal(ClientModel*, const CGovernanceObject&, QObject*) constructor
so that m_paymentAmount always has a defined value for display/sorting; update
the constructor to set m_paymentAmount = 0.0 (or another appropriate default)
and keep the existing assignment from prop_data when payment_amount exists.
In `@src/qt/proposalmodel.h`:
- Around line 71-83: Add a destructor for the ProposalModel class to free owned
Proposal pointers in m_data: implement ProposalModel::~ProposalModel() to
iterate m_data, delete each Proposal*, and clear the list; this mirrors
ownership semantics already used in remove() and reconcile() and prevents leaks
by releasing remaining proposals when the model is destroyed.
In `@src/qt/proposalresume.cpp`:
- Around line 135-141: The broadcast button lambdas capture a stale numeric
index (size_t index = m_entries.size()) causing wrong or out-of-bounds lookups
when entries are removed; change the lambda to find the clicked button's current
position by searching m_entries for the sender button pointer
(entry.broadcast_btn) at runtime and call onBroadcast with that computed index,
and add `#include` <algorithm> at the top for std::find support; update the
connect call to use the button pointer via the lambda capture (e.g., capture the
QPushButton* or use sender()) and compute index =
std::distance(m_entries.begin(), std::find(...)) before calling onBroadcast.
♻️ Duplicate comments (5)
src/node/interfaces.cpp (2)
317-331: GuardActiveChain().Height()access withcs_mainlock.As previously noted,
context().chainman->ActiveChain().Height()requires thecs_mainlock for thread safety. This is consistent with other methods in this file that access the active chain (e.g.,getNumBlocks(),getLastBlockTime()).🔒 Suggested fix
std::optional<int32_t> getProposalFundedHeight(const uint256& proposal_hash) override { if (context().govman != nullptr && context().chainman != nullptr && context().dmnman != nullptr) { - const int32_t nTipHeight = context().chainman->ActiveChain().Height(); + int32_t nTipHeight; + { + LOCK(::cs_main); + nTipHeight = context().chainman->ActiveChain().Height(); + } for (const auto& trigger : context().govman->GetActiveTriggers()) {
301-302: GuardActiveHeight()access withcs_mainlock.
ctx.chainman->ActiveHeight()reads chain state that can change concurrently. This should be protected bycs_mainfor thread safety, consistent with other chain height accesses in this file (e.g.,getNumBlocks(),getLastBlockTime()).Note: Lines 309-314 have a similar pre-existing issue where
GetListAtChainTip()andActiveChain()are accessed outside a lock. Consider wrapping the entire block that accesses chain state.🔒 Suggested fix
if (ctx.chainman) { + LOCK(::cs_main); CSuperblock::GetNearestSuperblocksHeights(ctx.chainman->ActiveHeight(), info.lastsuperblock, info.nextsuperblock); }Or consolidate locking for all chain-dependent reads in this method.
src/qt/proposalmodel.cpp (2)
356-362: GuarddataChangedwhen the model is empty.
rowCount() - 1can be-1, producing an invalid QModelIndex range.🐛 Proposed fix
- Q_EMIT dataChanged(createIndex(0, Column::VOTING_STATUS), createIndex(rowCount() - 1, Column::VOTING_STATUS)); + if (rowCount() > 0) { + Q_EMIT dataChanged(createIndex(0, Column::VOTING_STATUS), + createIndex(rowCount() - 1, Column::VOTING_STATUS)); + }
366-370: Bounds check ingetProposalAtis off by one.
std::clamp(..., 0, m_data.count())allowsrow == count, which can access past the end.🐛 Proposed fix
- if (index.isValid() && index.row() == std::clamp(index.row(), 0, m_data.count())) { - return m_data[index.row()]; - } - return nullptr; + if (!index.isValid() || index.row() < 0 || index.row() >= m_data.count()) return nullptr; + return m_data[index.row()];src/Makefile.qt.include (1)
164-167: Verify non-backported.txt includes new proposal C++ files.Per previous review feedback, the following new C++ files should be added to
test/util/data/non-backported.txt:
src/qt/descriptiondialog.cppandsrc/qt/descriptiondialog.hsrc/qt/proposaldesc.cppandsrc/qt/proposaldesc.hsrc/qt/proposalmodel.cppandsrc/qt/proposalmodel.hsrc/qt/proposalresume.cppandsrc/qt/proposalresume.hNote:
src/qt/proposalwizard.*is already registered, and.uifiles should not be added per project guidelines.#!/bin/bash # Verify non-backported.txt includes the new proposal C++ files echo "=== Current entries in non-backported.txt ===" cat test/util/data/non-backported.txt | grep -E "(descriptiondialog|proposaldesc|proposalmodel|proposalresume)" || echo "No matching entries found" echo "" echo "=== Expected new entries ===" echo "src/qt/descriptiondialog.cpp" echo "src/qt/descriptiondialog.h" echo "src/qt/proposaldesc.cpp" echo "src/qt/proposaldesc.h" echo "src/qt/proposalmodel.cpp" echo "src/qt/proposalmodel.h" echo "src/qt/proposalresume.cpp" echo "src/qt/proposalresume.h"Also applies to: 268-279
🧹 Nitpick comments (6)
src/qt/proposalmodel.h (1)
42-49: Assert non‑null ClientModel inProposalconstruction.Most methods dereference
clientModel, so a fail‑fast assert makes the invariant explicit.♻️ Suggested assert
+#include <cassert> ... Proposal::Proposal(ClientModel* _clientModel, const CGovernanceObject& _govObj, QObject* parent) : QObject(parent), clientModel(_clientModel), govObj(_govObj) { + assert(clientModel); ... }Based on learnings, prefer a fail‑fast assert to enforce this invariant.
src/qt/descriptiondialog.cpp (1)
6-11: Consider reordering includes for consistency.The typical convention is to include the corresponding header file first (descriptiondialog.h), followed by other project headers, then third-party headers. Currently, the UI header is included first.
♻️ Suggested include order
-#include <qt/forms/ui_descriptiondialog.h> - `#include` <qt/descriptiondialog.h> + +#include <qt/forms/ui_descriptiondialog.h> `#include` <qt/guiutil_font.h> - `#include` <qt/guiutil.h>src/qt/optionsmodel.h (1)
163-164: Consider adding default initializers for safety.The new member variables
m_enable_masternodesandm_enable_governancelack default initializers. WhileInit()sets these values from QSettings before they're used, adding default initializers (e.g.,{false}) provides defense-in-depth against potential future code paths that might access these before initialization.Suggested change
- bool m_enable_masternodes; - bool m_enable_governance; + bool m_enable_masternodes{false}; + bool m_enable_governance{false};src/qt/bitcoingui.cpp (1)
1470-1499: Refactor to eliminate hard-coded toolbar action indices.The visibility methods use hard-coded indices to access toolbar actions:
- Line 1460:
appToolBar->actions()[4]for CoinJoin- Line 1478:
appToolBar->actions()[5]for Masternodes- Line 1494:
appToolBar->actions()[6]for GovernanceThis pattern is fragile. Store action pointers when buttons are added to the toolbar, or use a lookup method (e.g.,
QToolBar::widgetForAction()) instead of relying on index positions. If the toolbar layout changes, these indices will silently reference wrong actions.src/qt/guiutil_font.cpp (1)
212-243: Character-by-character iteration may be slow for large HTML documents.The
setFontBodyHTMLfunction iterates one character at a time through the entire document to apply formatting. For large documents, this could cause noticeable UI lag.Consider using
QTextBlockiteration combined withQTextBlock::iteratorfor fragment-level processing, which would be more efficient:QTextBlock block = widget->document()->begin(); while (block.isValid()) { for (auto it = block.begin(); !it.atEnd(); ++it) { QTextFragment fragment = it.fragment(); // Process fragment... } block = block.next(); }That said, for typical proposal descriptions this is likely acceptable.
src/qt/proposalwizard.cpp (1)
206-212: Consider more specific error handling for creation failures.The error handling shows a generic error message from
prepareProposal. It might be helpful to distinguish between different failure modes (e.g., insufficient funds, network issues, validation failures) for better user guidance.However, this is a minor UX improvement and the current implementation is functional.
kwvg
changed the title
QTextEdit styling, governance layout refresh)
kwvg
force-pushed
the
gov_refresh
branch
6 times, most recently
from
b135de6 to
3c01a6f
Compare
|
This pull request has conflicts, please rebase. |
…ernance" and "Masternode" tabs, split out proposal model, additional bounds checking, better QTextEdit styling) f9f7c6e refactor: repurpose `transactiondescdialog` as a generic desc. container (Kittywhiskers Van Gogh) 5f4a6c0 refactor: track "Masternodes" tab show/hide in `OptionsModel` (Kittywhiskers Van Gogh) 1994f8c qt: use show/hide for "Masternodes" tab to avoid client restart (Kittywhiskers Van Gogh) 6a7e6b0 refactor: track "Governance" tab show/hide in `OptionsModel` (Kittywhiskers Van Gogh) 64a2db6 qt: use show/hide for "Governance" tab to avoid client restart (Kittywhiskers Van Gogh) 762e5d3 qt: add styling support for QTextEdit `<b>` and `<h{1-3}>` elements (Kittywhiskers Van Gogh) 7a2f09d refactor: drop `QObject` inheritance, precalculate hash string (Kittywhiskers Van Gogh) fdd3193 fix: use `unique_ptr` and more bounds checking in `qt/proposalmodel` (Kittywhiskers Van Gogh) c9e0d94 move-only: split proposal model out to `src/qt/proposalmodel.{cpp,h}` (Kittywhiskers Van Gogh) 42f2345 refactor: add `pruneStaleEntities` helper to deduplicate pruning (Kittywhiskers Van Gogh) 72c4099 refactor: add `getScaledFont` helper, keep `getFont` internal (Kittywhiskers Van Gogh) 6d9b90b chore: enforce alphabetical sorting in `src/Makefile.qt.include` (Kittywhiskers Van Gogh) 83b9271 move-only: move `subFeeFromAmount` into `verticalLayout_2` (Kittywhiskers Van Gogh) bcfc0e3 refactor: improve CoinJoin tab nullptr checks, bail-out condition (Kittywhiskers Van Gogh) 7648792 refactor: drop `emitCoinJoinEnabledChanged()`, use `setOption()` instead (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependency for #7110 * Dependency for #7116 | `develop` (cc50446) | This PR (18d6ce6) | | ----------------------- | ------------------- | |  |  | |  |  | |  |  | ## 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 f9f7c6e Tree-SHA512: 6c219c8bc5b90c52bb030a6f05bcf9168ec51be0d72c23b38b5aba852666419949ebe4ee07c2c8ba70241655a542de24e2e31b6b0241517e183b7bf289373a37
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
This is closer to how we report details for transactions and more translation-friendly and easier to copy.
Visiting URLs could be dangerous, add a warning before we open a link in someone's browser.
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Dash Governance Tools' budget generator is a much more visually friendly design and we can model our wizard after it.
The JSON and payload hex means little for the vast majority of users, what matters is the error messages, so we responsively validate contents and drop an extra panel in the process.
The visual language should match other coin-spending activities, also warn that that you don't get the fee back.
The proposal creation process can be interrupted for reasons beyond our control, since submitting the actual proposal is different from spending the collateral, it should have its own page. Right now this removes the old code but doesn't create a new layout, we will do that in the next commit.
This replaces the remaining pages of the proposal wizard that we removed in the last commit. Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
You cannot create a proposal without an adequate balance, it's a bad idea to allow any kind of proposal manipulation without being synced.
- Fix display unit change not updating governance cycle tooltip by resetting height cache before calling updateGovernanceCycleIcon - Snapshot block height in Proposal to prevent status flickering during IBD when chain tip advances between UI repaints Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
We are no longer using the wizard format so the name is a misnomer, the
logic is also relatively simplified (as now half of it is in
`qt/proposalresume.{cpp,h}`)
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/qt/proposalmodel.h (1)
1-3:⚠️ Potential issue | 🟡 MinorRun clang-format on this file (CI failure).
The Clang Diff Format Check is failing forproposalmodel.h.src/qt/governancelist.cpp (1)
1-5:⚠️ Potential issue | 🟡 MinorRun clang-format to fix the CI failure.
The Clang Diff Format Check reports formatting differences in this file. Please re-run clang-format on the touched hunks.
src/qt/proposalmodel.cpp (1)
1-4:⚠️ Potential issue | 🟡 MinorRun clang-format to fix the CI failure.
The Clang Diff Format Check reports formatting differences in this file. Please re-run clang-format on the touched hunks.
src/qt/governancelist.h (1)
1-4:⚠️ Potential issue | 🟡 MinorRun clang-format to fix the CI failure.
The Clang Diff Format Check reports formatting differences in this file. Please re-run clang-format on the touched hunks.
🤖 Fix all issues with AI agents
In `@src/governance/governance.cpp`:
- Around line 535-554: Clang-format drift in
CGovernanceManager::GetAllNewerThan: reformat this function to match project
style by running clang-format (or clang-format-diff) on
src/governance/governance.cpp and committing the changes; ensure the
LOCK(cs_store) block, the ranged-for loops over mapObjects and
mapPostponedObjects, the Assert(govobj)->GetCreationTime() checks, and the
objs.push_back(*govobj) lines follow the repository's spacing/bracing
conventions so the CI formatting check passes.
In `@src/qt/governancelist.cpp`:
- Around line 244-322: The worker thread is calling GUI-thread-affine methods;
fix by gathering all needed GUI-thread data inside updateProposalList() before
invoking m_worker: call clientModel->getMasternodeList(),
clientModel->getAllGovernanceObjects(),
clientModel->node().gov().getGovernanceInfo()/getFundableProposalHashes(), and
any walletModel queries (getWalletProposals(), wallet().isSpendable(),
queryCollateralDepth()/existsObj()) on the GUI thread, copy their results into
plain structs/containers, then pass those copies into the lambda (or into
calcProposalList via a new overload that accepts the copies) so
calcProposalList()/the worker only operates on thread-safe copies when
constructing Proposal instances and populating ret (refer to updateProposalList,
m_worker, calcProposalList, getMasternodeList, getAllGovernanceObjects,
getWalletProposals, clientModel->node().gov(), walletModel and Proposal).
In `@src/qt/proposalcreate.cpp`:
- Around line 1-3: Run clang-format over src/qt/proposalcreate.cpp and commit
the formatted result to fix the CI Clang Diff Format Check failure: format the
whole file using the project's clang-format configuration (same style used by
CI), update only whitespace/formatting changes, and ensure the resulting file
passes the repository's clang-format check so CI no longer flags
proposalcreate.cpp.
In `@src/qt/proposalresume.h`:
- Around line 1-6: A new Qt UI class ProposalResume (files proposalresume.h and
proposalresume.cpp added under src/qt) must be recorded in the non-backported
list; open test/util/data/non-backported.txt and append the exact new filenames
(proposalresume.h and proposalresume.cpp) each on their own line so the test
harness recognizes they are non-backported, then commit the change alongside the
new UI files.
🧹 Nitpick comments (1)
contrib/devtools/gen_moon_icons.py (1)
111-122: Consider adding directory existence check.The script assumes
src/qt/res/iconsexists. While this is reasonable for a devtools script, adding a safety check could improve robustness when running from unusual states.🔧 Optional: Add directory creation
def main(): script_dir = os.path.dirname(os.path.abspath(__file__)) repo_root = os.path.join(script_dir, '..', '..') icon_dir = os.path.join(repo_root, 'src', 'qt', 'res', 'icons') + os.makedirs(icon_dir, exist_ok=True) for frame in range(8):
| void CGovernanceManager::GetAllNewerThan(std::vector<CGovernanceObject>& objs, int64_t nMoreThanTime, | ||
| bool include_postponed) const | ||
| { | ||
| LOCK(cs_store); | ||
|
|
||
| for (const auto& [_, govobj] : mapObjects) { | ||
| // IF THIS OBJECT IS OLDER THAN TIME, CONTINUE | ||
| if (Assert(govobj)->GetCreationTime() < nMoreThanTime) { | ||
| continue; | ||
| } | ||
|
|
||
| // ADD GOVERNANCE OBJECT TO LIST | ||
| objs.push_back(*govobj); | ||
| } | ||
|
|
||
| if (include_postponed) { | ||
| for (const auto& [_, govobj] : mapPostponedObjects) { | ||
| if (Assert(govobj)->GetCreationTime() < nMoreThanTime) { | ||
| continue; | ||
| } | ||
| objs.push_back(*govobj); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix clang-format drift in this hunk (CI failure).
The clang-format check reports differences in this file; please run clang-format/clang-format-diff for governance.cpp before merge.
🤖 Prompt for AI Agents
In `@src/governance/governance.cpp` around lines 535 - 554, Clang-format drift in
CGovernanceManager::GetAllNewerThan: reformat this function to match project
style by running clang-format (or clang-format-diff) on
src/governance/governance.cpp and committing the changes; ensure the
LOCK(cs_store) block, the ranged-for loops over mapObjects and
mapPostponedObjects, the Assert(govobj)->GetCreationTime() checks, and the
objs.push_back(*govobj) lines follow the repository's spacing/bracing
conventions so the CI formatting check passes.
| void GovernanceList::updateProposalList() | ||
| { | ||
| if (this->clientModel) { | ||
| // A proposal is considered passing if (YES votes - NO votes) >= (Total Weight of Masternodes / 10), | ||
| // count total valid (ENABLED) masternodes to determine passing threshold. | ||
| // Need to query number of masternodes here with access to clientModel. | ||
| const int nWeightedMnCount = clientModel->getMasternodeList().first->getValidWeightedMNsCount(); | ||
| const int nAbsVoteReq = std::max(Params().GetConsensus().nGovernanceMinQuorum, nWeightedMnCount / 10); | ||
| proposalModel->setVotingParams(nAbsVoteReq); | ||
| if (!clientModel || clientModel->node().shutdownRequested()) { | ||
| return; | ||
| } | ||
|
|
||
| if (m_in_progress.exchange(true)) { | ||
| // Already applying, re-arm for next attempt | ||
| handleProposalListChanged(/*force=*/false); | ||
| return; | ||
| } | ||
|
|
||
| QMetaObject::invokeMethod(m_worker, [this] { | ||
| auto result = std::make_shared<CalcProposalList>(calcProposalList()); | ||
| m_in_progress.store(false); | ||
| QTimer::singleShot(0, this, [this, result] { | ||
| setProposalList(std::move(*result)); | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| GovernanceList::CalcProposalList GovernanceList::calcProposalList() const | ||
| { | ||
| CalcProposalList ret; | ||
| if (!clientModel || clientModel->node().shutdownRequested()) { | ||
| return ret; | ||
| } | ||
|
|
||
| const auto [dmn, pindex] = clientModel->getMasternodeList(); | ||
| if (!dmn || !pindex) { | ||
| return ret; | ||
| } | ||
|
|
||
| // A proposal is considered passing if (YES votes - NO votes) >= (Total Weight of Masternodes / 10), | ||
| // count total valid (ENABLED) masternodes to determine passing threshold. | ||
| // Need to query number of masternodes here with access to clientModel. | ||
| const int nWeightedMnCount = dmn->getValidWeightedMNsCount(); | ||
| ret.m_abs_vote_req = std::max(Params().GetConsensus().nGovernanceMinQuorum, nWeightedMnCount / 10); | ||
| ret.m_gov_info = clientModel->node().gov().getGovernanceInfo(); | ||
| if (m_proposal_source == ProposalSource::Active) { | ||
| std::vector<CGovernanceObject> govObjList; | ||
| clientModel->getAllGovernanceObjects(govObjList); | ||
| ProposalList newProposals; | ||
| for (const auto& govObj : govObjList) { | ||
| if (govObj.GetObjectType() != GovernanceObject::PROPOSAL) { | ||
| continue; // Skip triggers. | ||
| } | ||
| newProposals.emplace_back(std::make_unique<Proposal>(this->clientModel, govObj)); | ||
| ret.m_proposals.emplace_back(std::make_unique<Proposal>(this->clientModel, govObj, ret.m_gov_info, ret.m_gov_info.requiredConfs, | ||
| /*is_broadcast=*/true)); | ||
| } | ||
| proposalModel->reconcile(std::move(newProposals)); | ||
|
|
||
| // Update voting capability if we now have both client and wallet models | ||
| if (walletModel) { | ||
| updateVotingCapability(); | ||
| // Include unrelayed wallet proposals (0 confs, not yet broadcast) | ||
| for (const auto& obj : getWalletProposals(/*pending=*/true)) { | ||
| CGovernanceObject govObj(obj.hashParent, obj.revision, obj.time, obj.collateralHash, obj.GetDataAsHexString()); | ||
| ret.m_proposals.emplace_back(std::make_unique<Proposal>(this->clientModel, govObj, ret.m_gov_info, queryCollateralDepth(obj.collateralHash), | ||
| /*is_broadcast=*/false)); | ||
| } | ||
| } else if (m_proposal_source == ProposalSource::Local) { | ||
| for (const auto& obj : getWalletProposals(/*pending=*/std::nullopt)) { | ||
| CGovernanceObject govObj(obj.hashParent, obj.revision, obj.time, obj.collateralHash, obj.GetDataAsHexString()); | ||
| ret.m_proposals.emplace_back(std::make_unique<Proposal>(this->clientModel, govObj, ret.m_gov_info, queryCollateralDepth(obj.collateralHash), | ||
| /*is_broadcast=*/clientModel->node().gov().existsObj(obj.GetHash()))); | ||
| } | ||
| } | ||
|
|
||
| // Schedule next update. | ||
| timer->start(GOVERNANCELIST_UPDATE_SECONDS * 1000); | ||
| auto fundable{clientModel->node().gov().getFundableProposalHashes()}; | ||
| ret.m_fundable_hashes = std::move(fundable.hashes); | ||
|
|
||
| // Discover voting capability if we now have both client and wallet models | ||
| if (walletModel) { | ||
| dmn->forEachMN(/*only_valid=*/true, [&](const auto& dmn) { | ||
| // Check if wallet owns the voting key using the same logic as RPC | ||
| const auto script = GetScriptForDestination(PKHash(dmn.getKeyIdVoting())); | ||
| if (walletModel->wallet().isSpendable(script)) { | ||
| ret.m_votable_masternodes[dmn.getProTxHash()] = dmn.getKeyIdVoting(); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| return ret; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -a 'clientmodel.h|walletmodel.h' src/qtRepository: dashpay/dash
Length of output: 135
🏁 Script executed:
rg -n "class ClientModel|class WalletModel" src/qt --max-count=5 -A 10Repository: dashpay/dash
Length of output: 17105
🏁 Script executed:
rg -n "getMasternodeList|getAllGovernanceObjects|isSpendable" src/qt -B 2 -A 5Repository: dashpay/dash
Length of output: 7609
🏁 Script executed:
cat -n src/qt/clientmodel.h | sed -n '60,80p'Repository: dashpay/dash
Length of output: 1193
🏁 Script executed:
rg -n "interfaces::Node|interfaces::Wallet" src/qt/clientmodel.h -A 3 | head -40Repository: dashpay/dash
Length of output: 703
🏁 Script executed:
fd -a 'node.h|wallet.h' src/interfaces | head -5Repository: dashpay/dash
Length of output: 139
🏁 Script executed:
cat -n src/interfaces/node.h | sed -n '1,100p'Repository: dashpay/dash
Length of output: 3607
🏁 Script executed:
rg -n "thread|Thread|THREAD" src/interfaces/node.h | head -20Repository: dashpay/dash
Length of output: 38
🏁 Script executed:
cat -n src/interfaces/node.h | sed -n '100,250p'Repository: dashpay/dash
Length of output: 6373
🏁 Script executed:
rg -n "gov()" src/interfaces/node.h -A 2 -B 2Repository: dashpay/dash
Length of output: 899
🏁 Script executed:
cat -n src/interfaces/wallet.h | sed -n '1,80p'Repository: dashpay/dash
Length of output: 2579
🏁 Script executed:
rg -n "isSpendable" src/interfaces/wallet.h -B 3 -A 3Repository: dashpay/dash
Length of output: 465
🏁 Script executed:
cat -n src/qt/governancelist.h | sed -n '1,150p'Repository: dashpay/dash
Length of output: 4255
🏁 Script executed:
rg -n "m_worker|QThread" src/qt/governancelist.h -B 2 -A 5Repository: dashpay/dash
Length of output: 682
🏁 Script executed:
cat -n src/qt/governancelist.cpp | sed -n '1,50p'Repository: dashpay/dash
Length of output: 1622
🏁 Script executed:
cat -n src/qt/governancelist.cpp | sed -n '50,150p'Repository: dashpay/dash
Length of output: 5157
🏁 Script executed:
rg -n "m_worker|m_thread" src/qt/governancelist.cpp -B 2 -A 5 | head -100Repository: dashpay/dash
Length of output: 1408
🏁 Script executed:
cat -n src/qt/governancelist.cpp | sed -n '240,330p'Repository: dashpay/dash
Length of output: 4822
🏁 Script executed:
rg -n "queryCollateralDepth|getWalletProposals" src/qt/governancelist.cpp -A 10 | head -60Repository: dashpay/dash
Length of output: 2892
Avoid cross-thread access to ClientModel/WalletModel.
updateProposalList() schedules calcProposalList() on m_worker (worker thread), but calcProposalList() directly calls QObject methods on clientModel and walletModel (both GUI-thread-affine). This includes calls to getMasternodeList(), getAllGovernanceObjects(), node().gov() methods, wallet().isSpendable(), wallet().getGovernanceObjects(), and wallet().tryGetTxStatus(). While some underlying data is protected by mutexes (e.g., cs_mnlist), QObject method calls themselves are not thread-safe off-thread. Capture the raw data you need on the GUI thread (masternode list, governance objects, wallet state) and pass copies into the worker thread, or verify that each accessed method is explicitly documented as thread-safe.
🤖 Prompt for AI Agents
In `@src/qt/governancelist.cpp` around lines 244 - 322, The worker thread is
calling GUI-thread-affine methods; fix by gathering all needed GUI-thread data
inside updateProposalList() before invoking m_worker: call
clientModel->getMasternodeList(), clientModel->getAllGovernanceObjects(),
clientModel->node().gov().getGovernanceInfo()/getFundableProposalHashes(), and
any walletModel queries (getWalletProposals(), wallet().isSpendable(),
queryCollateralDepth()/existsObj()) on the GUI thread, copy their results into
plain structs/containers, then pass those copies into the lambda (or into
calcProposalList via a new overload that accepts the copies) so
calcProposalList()/the worker only operates on thread-safe copies when
constructing Proposal instances and populating ret (refer to updateProposalList,
m_worker, calcProposalList, getMasternodeList, getAllGovernanceObjects,
getWalletProposals, clientModel->node().gov(), walletModel and Proposal).
| // Copyright (c) 2025 The Dash Core developers | ||
| // Distributed under the MIT software license, see the accompanying | ||
| // file COPYING or http://www.opensource.org/licenses/mit-license.php. |
There was a problem hiding this comment.
Run clang-format on this file (CI failure).
The Clang Diff Format Check is failing for proposalcreate.cpp.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 1-1: Clang format differences detected in proposalcreate.cpp. Run clang-format-diff.py or clang-format to format the file.
🤖 Prompt for AI Agents
In `@src/qt/proposalcreate.cpp` around lines 1 - 3, Run clang-format over
src/qt/proposalcreate.cpp and commit the formatted result to fix the CI Clang
Diff Format Check failure: format the whole file using the project's
clang-format configuration (same style used by CI), update only
whitespace/formatting changes, and ensure the resulting file passes the
repository's clang-format check so CI no longer flags proposalcreate.cpp.
| QMenu* proposalContextMenu{nullptr}; | ||
| QObject* m_worker{nullptr}; | ||
| QSortFilterProxyModel* proposalModelProxy{nullptr}; | ||
| QThread* m_thread{nullptr}; |
There was a problem hiding this comment.
if I understand implementation correctly, it means that 2 extra threads are running now for each open wallet? If you have 16 or 24 cores it maybe a good idea until the some point...
But even 96 cores are not enough if you have 200wallets opens, they will just eat all CPU
this worker should be just a single worker for all wallets; otherwise it's too big waste, preferable the same worker for both governance and masternode, so, it's friendly for laptops or old PC that have only 4 cores; not talking about run in VM which could have only 1-2 cores.
| proposalContextMenu{new QMenu(this)}, | ||
| m_worker(new QObject), | ||
| proposalModelProxy{new QSortFilterProxyModel(this)}, | ||
| m_thread{new QThread(this)}, |
There was a problem hiding this comment.
this thread doesn't have name; consider assigning it.
Without it threads are just:
2 Thread 0x726390bfe6c0 (LWP 2145554) "QThread" __syscall_cancel_arch () at ../sysdeps/unix/sysv/linux/x86_64/syscall_cancel.S:56
3 Thread 0x7263913ff6c0 (LWP 2145553) "QThread" __syscall_cancel_arch () at ../sysdeps/unix/sysv/linux/x86_64/syscall_cancel.S:56
4 Thread 0x72639a7fe6c0 (LWP 2145548) "QThread" __syscall_cancel_arch () at ../sysdeps/unix/sysv/linux/x86_64/syscall_cancel.S:56
5 Thread 0x72639afff6c0 (LWP 2145547) "QThread" __syscall_cancel_arch () at ../sysdeps/unix/sysv/linux/x86_64/syscall_cancel.S:56
6 Thread 0x7263ea7ff6c0 (LWP 2145537) "QThread" __syscall_cancel_arch () at ../sysdeps/unix/sysv/linux/x86_64/syscall_cancel.S:56
7 Thread 0x7263f57ff6c0 (LWP 2145536) "QThread" __syscall_cancel_arch () at ../sysdeps/unix/sysv/linux/x86_64/syscall_cancel.S:56
8 Thread 0x7263dbbff6c0 (LWP 2145534) "QThread" __syscall_cancel_arch () at ../sysdeps/unix/sysv/linux/x86_64/syscall_cancel.S:56
9 Thread 0x7263e1fff6c0 (LWP 2145533) "QThread" __syscall_cancel_arch () at ../sysdeps/unix/sysv/linux/x86_64/syscall_cancel.S:56
while other do have it:
12 Thread 0x7264a17fa6c0 (LWP 2145522) "d-qt-clientmodl" __syscall_cancel_arch () at ../sysdeps/unix/sysv/linux/x86_64/syscall_cancel.S:56
UPDATE: yes, I opened 10 wallets and got 20 more runnign threads, all with the name QThread.
4 participants
Additional Information
UniValueinstead ofQJsonObject, consistency in tab hiding behavior, minor bugfixes #7134Proposal Creation
Breaking Changes
None expected.
Checklist