fix(qt): prevent overview page font double scaling, recalculate minimum width correctly, SERVICE and STATUS sorting, fix common types filtering#7147
Conversation
|
SERVICE and STATUS sortingSERVICE and STATUS sorting, fix common types filtering
|
No actionable comments were generated in the recent review. 🎉 WalkthroughChanges touch Qt UI and models: masternode status values now use signed semantics (positive = banned time, negative = active time) and the masternode proxy model gains a custom lessThan to lexicographically compare SERVICE byte arrays; TransactionFilterProxy type-bit logic is converted to constexpr with a new EXCLUDED_TYPES mask and COMMON_TYPES updated; overview page uses a compile-time default font size and no longer excludes DustReceive transactions; BitcoinGUI now calls updateWidth() when toggling wallet actions; a forward declaration changed from Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/qt/masternodelist.cpp`:
- Around line 48-51: The current filter uses the EditRole integer (status_value)
from sourceModel()->data(idx, Qt::EditRole) and only hides nodes when
status_value > 0, which lets banned nodes with unknown/invalid poseBanHeight()
(that return 0) slip through; update the filter logic in masternodelist.cpp to
explicitly check the model entry's ban state instead of relying solely on the
EditRole integer: fetch the underlying entry (use the same lookup path you use
elsewhere in this file to map idx to the masternode entry), call
entry->isBanned() or inspect entry->poseBanHeight() (std::optional) and treat
unknown/invalid values as banned when m_current_height > 0, ensuring the "Hide
banned" branch hides any entry where entry->isBanned() is true or
poseBanHeight() indicates a ban rather than relying on status_value > 0.
🧹 Nitpick comments (1)
src/qt/transactionfilterproxy.h (1)
16-18: Consider astatic_assertto guard against future enum overflow.
TransactionTypeToBitperforms1u << type, which is undefined behavior iftype >= 32. Currently the max enum value (DustReceive) is 14, so it's safe. However, if someone adds more transaction types in the future, there's no compile-time safety net.🛡️ Optional: add a compile-time guard
/** Helper function to convert transaction type enum to bit field */ -constexpr quint32 TransactionTypeToBit(int type) { return 1u << type; } +constexpr quint32 TransactionTypeToBit(int type) { return 1u << type; } +static_assert(TransactionRecord::DustReceive < 32, "Transaction type enum exceeds quint32 bit capacity");
| int status_value = sourceModel()->data(idx, Qt::EditRole).toInt(); | ||
| if (status_value > 0) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "poseBanHeight|pose_ban_height|PoSeBanHeight" --type=cpp --type=h -B2 -A2 | head -100Repository: dashpay/dash
Length of output: 6749
🏁 Script executed:
# Find masternodemodel.cpp and examine the data() function around line 294
fd "masternodemodel.cpp" --type fRepository: dashpay/dash
Length of output: 82
🏁 Script executed:
# Look for the STATUS column handling and ban logic
rg -n "STATUS.*EditRole|isBanned|ban_height" src/qt/ --type=cpp --type=h -B2 -A2 | head -150Repository: dashpay/dash
Length of output: 4689
Banned nodes with unknown ban height will escape the "hide banned" filter.
When m_current_height > 0 and a banned node has no valid poseBanHeight() (nullopt or invalid value), the EditRole returns 0 at line 294. Since the filter checks status_value > 0 to hide banned nodes, a value of 0 won't be filtered out.
This is a real edge case: if the node state JSON lacks or has invalid PoSeBanHeight data, a banned node will display even when "Hide banned" is checked. While the node displays correctly with a banned icon, the filter logic should be consistent. Consider checking entry->isBanned() directly for the filter instead of relying on the EditRole value, or use >= 0 with a different sentinel for unknown ban times.
🤖 Prompt for AI Agents
In `@src/qt/masternodelist.cpp` around lines 48 - 51, The current filter uses the
EditRole integer (status_value) from sourceModel()->data(idx, Qt::EditRole) and
only hides nodes when status_value > 0, which lets banned nodes with
unknown/invalid poseBanHeight() (that return 0) slip through; update the filter
logic in masternodelist.cpp to explicitly check the model entry's ban state
instead of relying solely on the EditRole integer: fetch the underlying entry
(use the same lookup path you use elsewhere in this file to map idx to the
masternode entry), call entry->isBanned() or inspect entry->poseBanHeight()
(std::optional) and treat unknown/invalid values as banned when m_current_height
> 0, ensuring the "Hide banned" branch hides any entry where entry->isBanned()
is true or poseBanHeight() indicates a ban rather than relying on status_value >
0.
… as Data Transactions 5c22389 qt: add support for reporting `OP_RETURN` payloads as Data Transactions (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Depends on #7147 | `develop` (06e761e) | This PR (97ae689) | | ---------------------- | -------------------- | |  |  | |  |  | ## 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: PastaPastaPasta: utACK 5c22389 UdjinM6: utACK 5c22389 Tree-SHA512: 37ade0aed7f64885268551cdab79c8c966508792d63e74c97a4f622b834cde74954d7c5e3b08b7ae35b38122270f148bff27935290de08644b0a72be81cb5127
9705b9a fix(qt): prevent banned masternodes from returning status=0 (UdjinM6) Pull request description: ## Issue being fixed or feature implemented Two code paths in `MasternodeModel::data()` could return `0` for banned nodes: - `ban_height` == `m_current_height` (banned this block) - `ban_height` unavailable (unknown ban time) `0` is also valid for an active node registered in the current block, so the "Hide banned" filter (`status > 0`) could silently fail to hide freshly-banned nodes. #7147 follow-up ## What was done? Fix: add `+1` offset to the banned-node calculation and change the fallback return from 0 to 1. The encoding contract is now unambiguous: - Banned → always ≥ 1 (age in blocks + 1) - Active → always ≤ 0 (0 = registered this block) Relative sort order among banned nodes is preserved. ## How Has This Been Tested? ## Breaking Changes n/a ## 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 - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone ACKs for top commit: kwvg: utACK 9705b9a Tree-SHA512: aa0aea49ad02ab23155dd5568baaac384bfcd30ff9fecbbd4ecd297c1b4f0207fd68399e858edde50daf36c6017b0bdd4d8a04be087e85d5337db3cab8079cfd
…g, recalculate minimum width correctly, `SERVICE` and `STATUS` sorting, fix common types filtering b568163 fix: calculate `COMMON_TYPES` programmatically, add dust to exclusions (Kittywhiskers Van Gogh) 269b73e fix: correct forward declaration to prevent ABI warning (Kittywhiskers Van Gogh) 2d99c57 fix: factor status age when sorting by active/ban status (Kittywhiskers Van Gogh) 087859e fix: restore `SERVICE` column comparison behavior lost in MVC refactor (Kittywhiskers Van Gogh) e673c9c fix: recalculate minimum width when buttons are enabled (Kittywhiskers Van Gogh) 31b305e fix: use constant base size in overview page to prevent double scaling (Kittywhiskers Van Gogh) Pull request description: ## Additional Information | Fix | develop (06e761e) | This PR (ec8ee9c) | | --- | -------------------- | -------------------- | | Correct common types filter behaviour to restore exclusion of CoinJoin mixing transactions |  |  | | Enforce minimum width correctly (and prevent double scaling, look at "Recent transactions" font size) |  |  | | Sorting by age (oldest active node) | <p align="center">Does not work</p> |  | | Sorting by service (IP address) | <p align="center">Does not work</p> |  | ## 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: light ACK b568163 Tree-SHA512: 7d0bdadc9d1da3b06108cb561b1296df2bbe108283eeb88b482ac1575d439c77eeae2c347790ccd1a1a9f551e19df69c50599d60b032181e957b358d66900bdc
…ayloads as Data Transactions 5c22389 qt: add support for reporting `OP_RETURN` payloads as Data Transactions (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Depends on dashpay#7147 | `develop` (06e761e) | This PR (97ae689) | | ---------------------- | -------------------- | |  |  | |  |  | ## 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: PastaPastaPasta: utACK 5c22389 UdjinM6: utACK 5c22389 Tree-SHA512: 37ade0aed7f64885268551cdab79c8c966508792d63e74c97a4f622b834cde74954d7c5e3b08b7ae35b38122270f148bff27935290de08644b0a72be81cb5127
…ayloads as Data Transactions 5c22389 qt: add support for reporting `OP_RETURN` payloads as Data Transactions (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Depends on dashpay#7147 | `develop` (06e761e) | This PR (97ae689) | | ---------------------- | -------------------- | |  |  | |  |  | ## 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: PastaPastaPasta: utACK 5c22389 UdjinM6: utACK 5c22389 Tree-SHA512: 37ade0aed7f64885268551cdab79c8c966508792d63e74c97a4f622b834cde74954d7c5e3b08b7ae35b38122270f148bff27935290de08644b0a72be81cb5127
…g status=0 9705b9a fix(qt): prevent banned masternodes from returning status=0 (UdjinM6) Pull request description: ## Issue being fixed or feature implemented Two code paths in `MasternodeModel::data()` could return `0` for banned nodes: - `ban_height` == `m_current_height` (banned this block) - `ban_height` unavailable (unknown ban time) `0` is also valid for an active node registered in the current block, so the "Hide banned" filter (`status > 0`) could silently fail to hide freshly-banned nodes. dashpay#7147 follow-up ## What was done? Fix: add `+1` offset to the banned-node calculation and change the fallback return from 0 to 1. The encoding contract is now unambiguous: - Banned → always ≥ 1 (age in blocks + 1) - Active → always ≤ 0 (0 = registered this block) Relative sort order among banned nodes is preserved. ## How Has This Been Tested? ## Breaking Changes n/a ## 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 - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone ACKs for top commit: kwvg: utACK 9705b9a Tree-SHA512: aa0aea49ad02ab23155dd5568baaac384bfcd30ff9fecbbd4ecd297c1b4f0207fd68399e858edde50daf36c6017b0bdd4d8a04be087e85d5337db3cab8079cfd
00f590d Merge #7180: qt: add Tahoe styled icons for macOS, runtime styling for each network type, update bundle icon, add mask-based tray icon, generation scripts (pasta) 60dda51 Merge #7176: perf: do linear lookup instead building 2 heavy Hash-Maps (pasta) df1ca87 Merge #7159: feat(qt): UI refresh (5/n, add proposal information widget to information, donut chart for proposal allocation) (pasta) 9061ad0 Merge #7118: feat(qt): UI refresh (4/n, introduce distinct widgets for Dash-specific reporting in debug window) (pasta) 64cc4f2 Merge #7160: feat(interfaces): consolidate masternode counts into one struct, expose chainlock, instantsend, credit pool, quorum statistics (pasta) 5d28a69 Merge #7157: fix(qt): prevent banned masternodes from returning status=0 (pasta) e0b7386 Merge #7146: feat(qt): introduce framework for sourcing and applying data, use for `{Masternode,Proposal}List`s (pasta) 8fd53cd Merge #7144: feat(qt): add support for reporting `OP_RETURN` payloads as Data Transactions (pasta) cc6f0bb Merge #7154: fix: MN update notifications had old_list/new_list swapped (pasta) 33f0138 Merge #7145: fix(qt): move labelError styling from proposalcreate.ui into general.css (pasta) 1bdbde6 Merge #7148: feat(qt): persist filter preferences in masternode list (pasta) 96bb601 Merge #7147: fix(qt): prevent overview page font double scaling, recalculate minimum width correctly, `SERVICE` and `STATUS` sorting, fix common types filtering (pasta) Pull request description: ## Backport PRs for v23.1.1 Cherry-picks the following 12 PRs (labeled `backport-candidate-23.1.x`) from `develop` onto `v23.1.x`, in merge order: | PR | Title | |---|---| | #7147 | fix(qt): prevent overview page font double scaling, recalculate minimum width correctly, `SERVICE` and `STATUS` sorting, fix common types filtering | | #7148 | feat(qt): persist filter preferences in masternode list | | #7145 | fix(qt): move labelError styling from proposalcreate.ui into general.css | | #7154 | fix: MN update notifications had old_list/new_list swapped | | #7144 | feat(qt): add support for reporting `OP_RETURN` payloads as Data Transactions | | #7146 | feat(qt): introduce framework for sourcing and applying data, use for `{Masternode,Proposal}List`s | | #7157 | fix(qt): prevent banned masternodes from returning status=0 | | #7160 | feat(interfaces): consolidate masternode counts into one struct, expose chainlock, instantsend, credit pool, quorum statistics | | #7118 | feat(qt): UI refresh (4/n, introduce distinct widgets for Dash-specific reporting in debug window) | | #7159 | feat(qt): UI refresh (5/n, add proposal information widget to information, donut chart for proposal allocation) | | #7176 | perf: do linear lookup instead building 2 heavy Hash-Maps | | #7180 | qt: add Tahoe styled icons for macOS, runtime styling for each network type, update bundle icon, add mask-based tray icon, generation scripts | All 12 cherry-picked cleanly (no conflicts). ## Notes - Used `git cherry-pick -m 1 <merge-sha>` for each (all were merge commits on develop) - Applied in chronological merge order to respect dependency chains - Version bump and release notes are separate steps per the release process ACKs for top commit: kwvg: utACK 00f590d UdjinM6: utACK 00f590d Tree-SHA512: 90d2a0660db8daa69b3e3b33a8a790fb0ea7d9a04656a2e27955575e76b6f4c9a379c435ef1c573ef6669c36cb6e205ba9701716d3dc303b01f19c719516b6d1
Additional Information
Does not work
Does not work
Breaking Changes
None expected.
Checklist