feat(qt): introduce framework for sourcing and applying data, use for {Masternode,Proposal}Lists #7146
feat(qt): introduce framework for sourcing and applying data, use for {Masternode,Proposal}Lists #7146kwvg wants to merge 9 commits intodashpay:developfrom
{Masternode,Proposal}Lists #7146Conversation
✅ 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:
WalkthroughThis PR adds a Qt client-side feed system (ClientFeeds, FeedBase, templated Feed, MasternodeFeed, ProposalFeed) and integrates it into ClientModel. Masternode and proposal UI, models, and related headers are refactored to consume MasternodeData / ProposalData from feeds. Ownership semantics change from unique_ptr to shared_ptr for entries and proposals. GovernanceList is replaced by ProposalList across UI and build visibility; build/moc/public-header lists updated accordingly. Feed scheduling runs on a worker thread with QTimer-driven fetches and dataReady signaling. Sequence Diagram(s)sequenceDiagram
participant UI as ProposalList / MasternodeList
participant CM as ClientModel
participant CF as ClientFeeds
participant Feed as MasternodeFeed / ProposalFeed
participant Timer as QTimer
participant Worker as WorkerThread
participant Data as FeedData
UI->>CM: setClientModel(...)
CM->>CF: create/register feeds, start()
CF->>Feed: registerFeed(feed)
CF->>Timer: create/start(baseline/throttle)
Timer->>CF: timeout
CF->>Worker: schedule(fetch task)
Worker->>Feed: fetch()
Feed->>Data: gather/compute data
Feed->>Worker: setData(shared_ptr)
Worker->>CF: notify dataReady
CF->>UI: emit dataReady
UI->>UI: updateMasternodeList / updateProposalList
alt syncing state change
UI->>CF: setSyncing(true/false)
CF->>Timer: adjust/backoff intervals
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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.
🧹 Nitpick comments (11)
test/lint/lint-circular-dependencies.py (1)
60-66: New circular dependency entries align with the refactored module structure.Seven new cycles added, all rooted in
qt/clientmodel↔qt/masternodelist/qt/proposallistand their transitive dependencies. These replace the priorqt/governancelist-based entries. Based on learnings, when refactoring changes module dependencies, update the allowlist entries to reflect the new dependency paths.As an architectural note: the number of cycles through
qt/clientmodelis growing. Consider whether a forward-declaration or interface-based decoupling could reduce this surface in a follow-up.src/qt/clientfeeds.h (1)
96-103:add()has no guard against being called afterstop().If
add()is called afterstop(), the feed will be registered (includingregisterFeedwhich creates a timer and connects it) butm_threadmay already be finished andm_workerset tonullptr. TheinvokeMethodinregisterFeed's lambda would then invoke on a null object.Consider adding an assert or early return:
🛡️ Suggested defensive guard
template<typename Source, typename... Args> Source* add(Args&&... args) { + assert(!m_stopped); auto source = std::make_unique<Source>(std::forward<Args>(args)...);Based on learnings, prefer assert() for programmer errors that should be caught in development, rather than defensive runtime checks with fallbacks.
src/qt/clientmodel.cpp (1)
84-89: Proposal feed refreshes on every block tip but not masternode feed.
m_feed_proposal->requestRefresh()is triggered on every non-headernumBlocksChanged, while the masternode feed only refreshes whenmasternodeListChangedfires. If the intent is that the masternode feed should also refresh periodically with blocks (e.g., to update "next payment height" or similar block-height-dependent data), this may be a gap. If the masternode list signal is sufficient, this is fine as-is.src/qt/walletview.h (1)
12-12: Consider forward-declaringProposalListinstead of including the full header.Only a
ProposalList*member is declared here (line 72). A forward declaration would reduce header coupling and compilation dependencies, consistent with the existing forward declarations in this file (e.g.,class ClientModel;,class OverviewPage;).♻️ Suggested change
-#include <qt/proposallist.h> +class ProposalList;(Move the
#includetowalletview.cppwhereProposalListmethods are actually called.)src/qt/clientfeeds.cpp (1)
46-76: Solid feed registration with proper thread marshalling and retry logic.The concurrency model is correct: timer fires on main thread → fetch dispatched to worker → completion marshalled back to main thread via
QTimer::singleShot. Them_in_progressatomic prevents overlapping fetches, andm_retry_pendingensures no updates are lost.One minor point:
util::ThreadRename("qt-clientfeed")on Line 58 is called on every fetch invocation. Since the worker thread is reused, this syscall is redundant after the first call.♻️ Move thread rename to thread start
Consider renaming the thread once at start time instead of on every fetch:
void ClientFeeds::start() { m_worker->moveToThread(m_thread); connect(m_thread, &QThread::finished, m_worker, &QObject::deleteLater); m_thread->start(); + QMetaObject::invokeMethod(m_worker, [] { util::ThreadRename("qt-clientfeed"); }); for (const auto& source : m_sources) {And remove it from
registerFeed:QMetaObject::invokeMethod(m_worker, [this, raw] { - util::ThreadRename("qt-clientfeed"); try {src/qt/masternodelist.cpp (3)
184-235: Feed fetch logic is correct; minor style nit on C-style cast.The fetch logic properly handles edge cases (shutdown, empty MN list, failed projected payees). Data is correctly shared via
std::make_shared<Data>().Line 208:
(int)iuses a C-style cast. Preferstatic_cast<int>(i)for consistency with the rest of the codebase (e.g., Line 203 already uses the more explicit style indirectly).♻️ Use C++ cast
- nextPayments.emplace(dmn->getProTxHash(), ret->m_list_height + (int)i + 1); + nextPayments.emplace(dmn->getProTxHash(), ret->m_list_height + static_cast<int>(i) + 1);
237-285:updateMasternodeList()re-fetches MN list for wallet ownership — intentional but worth documenting.Lines 271-283 call
clientModel->getMasternodeList()again (the feed already fetched it inMasternodeFeed::fetch()). This is necessary because the feed'sMasternodeDatacontains pre-builtMasternodeEntryobjects and doesn't expose raw DMN references needed forisSpendablekey/script checks. A brief comment explaining this would help future readers.📝 Suggested comment
+ // Re-fetch raw MN list from clientModel since MasternodeData contains + // pre-built UI entries without raw key/script data needed for ownership checks. const auto [dmn, pindex] = clientModel->getMasternodeList();
254-257: Field-by-field copy could be simplified.The copy from
feedtoretcopies each field individually. SinceMasternodeDatais a simple struct, a direct copy would be more concise and less error-prone if new fields are added later.♻️ Simplify to struct copy
- MasternodeFeed::Data ret; - ret.m_list_height = feed->m_list_height; - ret.m_entries = feed->m_entries; - ret.m_valid = feed->m_valid; + MasternodeFeed::Data ret = *feed;src/qt/proposallist.cpp (3)
130-146: No cleanup of prior feed connection ifsetClientModelis called more than once.If
setClientModelis ever called a second time with a differentClientModel*, the oldm_feedsignal connection (line 138) will remain active alongside the new one, potentially invokingupdateProposalListtwice per feed update — once from the stale feed and once from the new one.If
setClientModelis guaranteed to be called only once, this is fine. Otherwise, disconnect the previous feed before connecting the new one.Suggested guard
void ProposalList::setClientModel(ClientModel* model) { + if (m_feed) { + disconnect(m_feed, &ProposalFeed::dataReady, this, &ProposalList::updateProposalList); + m_feed = nullptr; + } this->clientModel = model; if (!clientModel) { return; }
282-289: Clearingm_proposalsdiscards all pre-fetched feed data forLocalsource — intentional but worth a brief inline comment.Line 284 clears the proposals vector that was just copied from the feed. For the
Localsource path this is expected (only wallet proposals should be shown), but a reader might wonder why the feed data was copied at all for this branch. A one-liner comment at line 263 (e.g., "Copy feed data as baseline; source-specific paths may override") would help.
632-671: Reentrancy guard uses a plainbool— adequate here but not exception-safe.
m_col_refreshis set totrueon line 639 and reset on line 671. If any code between those lines threw, the flag would remain set permanently, silently disabling future resizes. In practice, Qt widget calls don't throw, so this is fine. A scope-guard pattern (auto guard = qScopeGuard([&]{ m_col_refresh = false; });) would be slightly more robust but is not essential.
kwvg
force-pushed
the
client_feeds
branch
2 times, most recently
from
6f0d15e to
fd12e06
Compare
kwvg
requested review from
PastaPastaPasta,
UdjinM6 and
knst
and removed request for
UdjinM6
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/qt/proposalmodel.h (1)
1-1:⚠️ Potential issue | 🟡 MinorCI failure: clang-format differences detected.
The pipeline reports clang-format violations for this file. Run
./contrib/devtools/clang-format-diff.py -p1or apply clang-format to resolve.
🤖 Fix all issues with AI agents
In `@src/qt/clientmodel.cpp`:
- Around line 90-95: The masternode feed's next-payment heights become stale
because m_feed_masternode is only refreshed on masternodeListChanged while its
nNextPayment depends on current block height; update m_feed_masternode on each
new block like m_feed_proposal does by calling
m_feed_masternode->requestRefresh() inside the numBlocksChanged lambda (the same
lambda that sets m_feeds->setSyncing and calls
m_feed_proposal->requestRefresh()), so height-dependent calculations using
getMasternodeList() are re-evaluated per block.
In `@test/util/data/non-backported.txt`:
- Around line 31-38: Replace the broad file globs ending with ".*" (e.g.,
src/qt/clientfeeds.*, src/qt/descriptiondialog.*, src/qt/guiutil_font.*,
src/qt/masternodelist.*, src/qt/masternodelodel.*,
src/qt/mnemonicverificationdialog.*, src/qt/proposalcreate.*,
src/qt/proposallist.*) with C++-only patterns restricting to .cpp and .h (for
example src/qt/clientfeeds.{cpp,h}) so the list only matches C++ source and
header files used by clang-format; update each listed entry to the {cpp,h}
pattern.
🧹 Nitpick comments (5)
src/qt/masternodelist.cpp (2)
189-192: Shallow copy of feed data is intentional but worth noting.
ret.m_entries = feed->m_entriescopies theshared_ptrvector (bumping ref counts). This is correct for the feed pattern where the snapshot is shared, but it means the UI holds references to the same entry objects the feed produced. EnsureMasternodeEntryobjects are treated as immutable after construction (they appear to be, from the read-only accessor pattern).
206-218: Potential snapshot mismatch between feed data andgetMasternodeList().The masternode entries displayed come from
m_feed->data()(Line 178), but the owned-MN check at Line 206 callsclientModel->getMasternodeList()separately. These could reference different chain tips momentarily, causing a mismatch between displayed entries and ownership highlights. This self-corrects on the next feed cycle, but if strict consistency is needed, consider deriving owned_mns from the same snapshot the feed uses.src/qt/masternodelist.h (1)
34-35: Stale forward declaration ofQThread.With the removal of the old worker thread infrastructure,
QThreadis no longer referenced in this header. The forward declaration can be removed.🧹 Suggested cleanup
QT_BEGIN_NAMESPACE class QModelIndex; -class QThread; QT_END_NAMESPACEsrc/qt/proposallist.h (1)
64-64:std::atomic<bool>for single-threaded guard.
m_col_refreshis only accessed from the main thread (viashowEvent,resizeEvent, andsectionResizedsignal handler). A plainboolwould suffice, but this is harmless.src/qt/clientfeeds.cpp (1)
79-98: Variabledmnis shadowed three times in nested scopes.Line 64 binds
dmnfromgetMasternodeList(), Line 81 reusesdmnforprojectedPayees[i], and Line 85 usesdmnas the lambda parameter. While valid C++, this triple-shadowing is confusing and could hide bugs if the wrongdmnis accidentally referenced.♻️ Rename to avoid shadowing
- for (size_t i = 0; i < projectedPayees.size(); i++) { - const auto& dmn = projectedPayees[i]; - nextPayments.emplace(dmn->getProTxHash(), ret->m_list_height + (int)i + 1); + for (size_t i = 0; i < projectedPayees.size(); i++) { + const auto& payee = projectedPayees[i]; + nextPayments.emplace(payee->getProTxHash(), ret->m_list_height + (int)i + 1); } - dmn->forEachMN(/*only_valid=*/false, [&](const auto& dmn) { + dmn->forEachMN(/*only_valid=*/false, [&](const auto& mn) {
| connect(this, &ClientModel::numBlocksChanged, this, | ||
| [this](int, const QDateTime&, const QString&, double, bool header, SynchronizationState sync_state) { | ||
| if (header) return; | ||
| m_feeds->setSyncing(sync_state != SynchronizationState::POST_INIT); | ||
| if (m_feed_proposal) m_feed_proposal->requestRefresh(); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what triggers masternodeListChanged and if height-dependent data is handled
rg -n "masternodeListChanged" --type cpp --type-add 'header:*.h' --type header -C 2
echo ""
echo "=== MasternodeFeed fetch implementation ==="
rg -n "MasternodeFeed::fetch" --type cpp -A 30Repository: dashpay/dash
Length of output: 3422
🏁 Script executed:
#!/bin/bash
# Check getMasternodeList implementation to see if pindex is current or cached
echo "=== getMasternodeList implementation ==="
rg -n "getMasternodeList" --type cpp --type-add 'header:*.h' --type header -B 2 -A 10 | head -80
echo ""
echo "=== How nextPayments is used in MasternodeFeed ==="
rg -n "nextPayments" src/qt/clientfeeds.cpp -B 2 -A 2
echo ""
echo "=== Check if m_list_height or nextPayments update on block changes ==="
rg -n "m_list_height\|next.*payment" src/qt/clientfeeds.cpp -B 1 -A 1Repository: dashpay/dash
Length of output: 5814
🏁 Script executed:
#!/bin/bash
# Compare ProposalFeed::fetch to understand why it updates on every block
echo "=== ProposalFeed::fetch implementation ==="
rg -n "class ProposalFeed" --type cpp --type-add 'header:*.h' --type header -A 5
echo ""
echo "=== ProposalFeed::fetch method ==="
rg -n "void ProposalFeed::fetch" src/qt/clientfeeds.cpp -A 50 | head -80
echo ""
echo "=== Check where block changes trigger proposal refresh ==="
rg -n "m_feed_proposal.*requestRefresh\|numBlocksChanged" src/qt/clientmodel.cpp -B 2 -A 2Repository: dashpay/dash
Length of output: 3036
🏁 Script executed:
#!/bin/bash
# Search for where nNextPayment is used after being calculated
echo "=== Usage of nNextPayment in MasternodeFeed ==="
rg -n "nNextPayment" src/qt/clientfeeds.cpp -B 2 -A 10
echo ""
echo "=== Check how MasternodeFeed data is displayed ==="
rg -n "MasternodeFeed\|m_feed_masternode" src/qt/ -t cpp --type-add 'header:*.h' --type header | grep -v "Binary" | head -30
echo ""
echo "=== Check the Data structure to see what's stored ==="
rg -n "struct.*MasternodeData\|struct MasternodeFeed::Data" src/qt/clientfeeds.cpp -A 20 | head -60Repository: dashpay/dash
Length of output: 961
🏁 Script executed:
#!/bin/bash
# Search for MasternodeEntry class definition and usage
echo "=== MasternodeEntry class definition ==="
rg -n "struct MasternodeEntry\|class MasternodeEntry" --type cpp --type-add 'header:*.h' --type header -A 15 | head -80
echo ""
echo "=== Find MasternodeModel or display classes ==="
find src/qt -name "*.h" -o -name "*.cpp" | xargs grep -l "MasternodeEntry\|MasternodeFeed" | head -10
echo ""
echo "=== Check what calls getMasternodeList to see how it's used in UI ==="
rg -n "getMasternodeList\(\)" src/qt/ -B 3 -A 8 | head -100Repository: dashpay/dash
Length of output: 6762
Masternode feed next payment heights go stale between masternode list changes.
m_feed_masternode is only refreshed on masternodeListChanged (line 81), but the calculated nNextPayment values depend on the current block height. Since getMasternodeList() returns cached data that only updates when the masternode list changes, the next_payment_height displayed to users becomes increasingly inaccurate as new blocks arrive. This is inconsistent with m_feed_proposal, which refreshes on every block despite using the same cached masternode list, suggesting that height-dependent calculations require real-time updates.
🤖 Prompt for AI Agents
In `@src/qt/clientmodel.cpp` around lines 90 - 95, The masternode feed's
next-payment heights become stale because m_feed_masternode is only refreshed on
masternodeListChanged while its nNextPayment depends on current block height;
update m_feed_masternode on each new block like m_feed_proposal does by calling
m_feed_masternode->requestRefresh() inside the numBlocksChanged lambda (the same
lambda that sets m_feeds->setSyncing and calls
m_feed_proposal->requestRefresh()), so height-dependent calculations using
getMasternodeList() are re-evaluated per block.
| src/qt/clientfeeds.* | ||
| src/qt/descriptiondialog.* | ||
| src/qt/governancelist.* | ||
| src/qt/guiutil_font.* | ||
| src/qt/masternodelist.* | ||
| src/qt/masternodemodel.* | ||
| src/qt/mnemonicverificationdialog.* | ||
| src/qt/proposalcreate.* | ||
| src/qt/proposallist.* |
There was a problem hiding this comment.
Restrict new entries to .cpp/.h patterns.
This file should only list C++ sources/headers. Using .* can accidentally include other extensions if added later. Consider narrowing to {cpp,h} for the new entries.
✅ Suggested update
-src/qt/clientfeeds.*
+src/qt/clientfeeds.{cpp,h}
…
-src/qt/proposallist.*
+src/qt/proposallist.{cpp,h}Based on learnings: “test/util/data/non-backported.txt should only include C++ files (.cpp or .h) because it is used for running clang-format. Other file types … should not be added.”
🤖 Prompt for AI Agents
In `@test/util/data/non-backported.txt` around lines 31 - 38, Replace the broad
file globs ending with ".*" (e.g., src/qt/clientfeeds.*,
src/qt/descriptiondialog.*, src/qt/guiutil_font.*, src/qt/masternodelist.*,
src/qt/masternodelodel.*, src/qt/mnemonicverificationdialog.*,
src/qt/proposalcreate.*, src/qt/proposallist.*) with C++-only patterns
restricting to .cpp and .h (for example src/qt/clientfeeds.{cpp,h}) so the list
only matches C++ source and header files used by clang-format; update each
listed entry to the {cpp,h} pattern.
kwvg
force-pushed
the
client_feeds
branch
2 times, most recently
from
d353007 to
763e1c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/qt/clientfeeds.h`:
- Around line 137-166: The m_worker QObject is orphaned because deleteLater() is
queued to the worker thread's event loop and may never run once stop() calls
m_thread->quit() and m_thread->wait(); fix by ensuring deterministic cleanup:
either set m_worker->setParent(this) (ClientFeeds) when creating/assigning it so
it will be deleted with the ClientFeeds instance, or explicitly delete m_worker
after m_thread->wait() in stop() (e.g., if (m_worker) { m_worker->deleteLater();
delete m_worker; m_worker = nullptr; } or simply delete m_worker and set to
nullptr) and remove reliance on the QThread::finished -> deleteLater connection;
update start() and stop() code paths accordingly to use the chosen approach and
reference m_worker, start(), stop(), and the QThread::finished/deleteLater usage
when making the change.
🧹 Nitpick comments (5)
src/qt/clientfeeds.cpp (1)
55-99: Variable namedmnshadows outer scope on Line 78.Line 61 binds
dmnfrom the structured binding, then Line 78 declaresconst auto& dmn = projectedPayees[i]inside the loop, and Line 82's lambda parameter is also nameddmn. While legal C++, this triple-shadowing is confusing and could mask bugs during maintenance.🔧 Rename inner variables to avoid confusion
for (size_t i = 0; i < projectedPayees.size(); i++) { - const auto& dmn = projectedPayees[i]; - nextPayments.emplace(dmn->getProTxHash(), ret->m_list_height + (int)i + 1); + const auto& payee = projectedPayees[i]; + nextPayments.emplace(payee->getProTxHash(), ret->m_list_height + (int)i + 1); } - dmn->forEachMN(/*only_valid=*/false, [&](const auto& dmn) { + dmn->forEachMN(/*only_valid=*/false, [&](const auto& mn) {src/qt/proposallist.cpp (2)
295-313:showCreateProposalDialog: three separateconnectcalls toacceptedcould be consolidated.Lines 309-311 connect three separate slots to the same
QDialog::acceptedsignal. While functionally correct, a single lambda could handle all three actions.♻️ Optional consolidation
- connect(proposalCreate, &QDialog::accepted, this, [this] { if (m_feed) m_feed->requestForceRefresh(); }); - connect(proposalCreate, &QDialog::accepted, this, &ProposalList::updateProposalButtons); - connect(proposalCreate, &QDialog::accepted, this, &ProposalList::showResumeProposalDialog); + connect(proposalCreate, &QDialog::accepted, this, [this] { + if (m_feed) m_feed->requestForceRefresh(); + updateProposalButtons(); + showResumeProposalDialog(); + });
589-628:refreshColumnWidthsusesm_col_refreshas a reentrancy guard —std::atomicis overkill here.Since
refreshColumnWidthsis only invoked from the main thread (viashowEvent,resizeEvent, andsectionResizedsignal), a plainboolwould suffice. Theatomicadds unnecessary overhead for cross-core visibility guarantees that aren't needed.This is declared in the header (Line 64 of
proposallist.h). Minor nit — not worth changing if consistency with other patterns is preferred.src/qt/proposallist.h (1)
23-36: Remove unusedCDeterministicMNListforward declaration (line 23).The forward declaration is not referenced in this header or the corresponding
.cppfile and can be safely removed.src/qt/masternodelist.h (1)
20-35: Remove unusedQThreadforward declaration (line 34).The
QThreadforward declaration is not used anywhere in the class definition and can be removed. Additionally, the#include <QThread>in the implementation file may also be unnecessary.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@src/Makefile.qt.include`:
- Around line 247-248: The list in BITCOIN_QT_BASE_CPP is out of alphabetical
order: move the entries "qt/masternodemodel.cpp" and "qt/proposalmodel.cpp" from
their current location between bitcoinaddressvalidator.cpp and
bitcoinamountfield.cpp into their correct alphabetical positions — place
"qt/masternodemodel.cpp" near "modaloverlay.cpp" (M-section) and
"qt/proposalmodel.cpp" near "qvalidatedlineedit.cpp" (P/Q-section) so the entire
list remains sorted; update only the ordering in the Makefile fragment,
preserving existing whitespace and line endings.
In `@src/qt/clientfeeds.cpp`:
- Around line 109-141: ProposalFeed::fetch() uses a structured binding named dmn
from m_client_model.getMasternodeList(), which shadows/duplicates the naming
pattern criticized elsewhere; rename the binding (e.g., mn_list or mnList) and
update all subsequent uses (currently dmn->getValidWeightedMNsCount() and any
other dmn references) to the new name to match the MasternodeFeed::fetch()
rename for clarity and consistency.
- Around line 55-99: The fetch function currently shadows the outer variable
named dmn (from auto [dmn, pindex] = m_client_model.getMasternodeList()) with
inner uses named dmn in the projectedPayees loop and the forEachMN lambda;
rename one side to avoid shadowing (e.g. rename the structured binding dmn to
mnList or mnManager, or rename the lambda parameters to payee and mn) and update
all references in MasternodeFeed::fetch (uses in getProjectedMNPayees,
projectedPayees loop, nextPayments.emplace, and the forEachMN callback where
getCollateralOutpoint(), getProTxHash(), etc. are called) so identifiers are
unique and readable.
- Around line 187-198: ClientFeeds::start currently restarts every source timer
with m_timer->start(0), which forces an immediate timeout and makes prior
requestForceRefresh() calls redundant; change the logic to only start timers
that are not already active (check QTimer::isActive() on each source->m_timer
and call start() only when isActive() is false) so that pre-start
requestForceRefresh() results are preserved and you avoid forcing an immediate
0ms restart.
In `@src/qt/clientfeeds.h`:
- Around line 145-152: The add<Source>(...) template must only be called before
the feed system is started; add an assert at the top of add() to fail-fast when
called after start() (use the existing startup flag used by start()/m_started or
the lifecycle indicator checked by registerFeed/start). Modify the add()
implementation (referencing add, registerFeed, m_sources and the start()
lifecycle flag) to Q_ASSERT or assert(!startedFlag) so new feeds can't be added
once start() has run, keeping registerFeed and m_sources behavior unchanged.
- Around line 96-100: MasternodeData includes an m_valid flag used by
MasternodeList::updateMasternodeList() to schedule retries, while ProposalData
currently lacks this flag; add a brief inline comment inside the ProposalData
struct explaining why validity tracking is unnecessary (or that
ProposalFeed::fetch() only publishes complete data and handles failures via
early return) so future readers know the asymmetry is intentional and not a
missing field.
In `@src/qt/proposallist.h`:
- Around line 43-44: The doc comment above the ProposalList class is stale
(mentions "Governance Manager page widget" and the old class name
GovernanceList); update the comment to accurately describe ProposalList (e.g.,
"Proposal list page widget" or similar) so it matches the renamed class
ProposalList in proposallist.h and any related documentation references.
🧹 Nitpick comments (6)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@src/Makefile.qt.include`: - Around line 247-248: The list in BITCOIN_QT_BASE_CPP is out of alphabetical order: move the entries "qt/masternodemodel.cpp" and "qt/proposalmodel.cpp" from their current location between bitcoinaddressvalidator.cpp and bitcoinamountfield.cpp into their correct alphabetical positions — place "qt/masternodemodel.cpp" near "modaloverlay.cpp" (M-section) and "qt/proposalmodel.cpp" near "qvalidatedlineedit.cpp" (P/Q-section) so the entire list remains sorted; update only the ordering in the Makefile fragment, preserving existing whitespace and line endings. In `@src/qt/clientfeeds.cpp`: - Around line 109-141: ProposalFeed::fetch() uses a structured binding named dmn from m_client_model.getMasternodeList(), which shadows/duplicates the naming pattern criticized elsewhere; rename the binding (e.g., mn_list or mnList) and update all subsequent uses (currently dmn->getValidWeightedMNsCount() and any other dmn references) to the new name to match the MasternodeFeed::fetch() rename for clarity and consistency. - Around line 55-99: The fetch function currently shadows the outer variable named dmn (from auto [dmn, pindex] = m_client_model.getMasternodeList()) with inner uses named dmn in the projectedPayees loop and the forEachMN lambda; rename one side to avoid shadowing (e.g. rename the structured binding dmn to mnList or mnManager, or rename the lambda parameters to payee and mn) and update all references in MasternodeFeed::fetch (uses in getProjectedMNPayees, projectedPayees loop, nextPayments.emplace, and the forEachMN callback where getCollateralOutpoint(), getProTxHash(), etc. are called) so identifiers are unique and readable. - Around line 187-198: ClientFeeds::start currently restarts every source timer with m_timer->start(0), which forces an immediate timeout and makes prior requestForceRefresh() calls redundant; change the logic to only start timers that are not already active (check QTimer::isActive() on each source->m_timer and call start() only when isActive() is false) so that pre-start requestForceRefresh() results are preserved and you avoid forcing an immediate 0ms restart. In `@src/qt/clientfeeds.h`: - Around line 145-152: The add<Source>(...) template must only be called before the feed system is started; add an assert at the top of add() to fail-fast when called after start() (use the existing startup flag used by start()/m_started or the lifecycle indicator checked by registerFeed/start). Modify the add() implementation (referencing add, registerFeed, m_sources and the start() lifecycle flag) to Q_ASSERT or assert(!startedFlag) so new feeds can't be added once start() has run, keeping registerFeed and m_sources behavior unchanged. - Around line 96-100: MasternodeData includes an m_valid flag used by MasternodeList::updateMasternodeList() to schedule retries, while ProposalData currently lacks this flag; add a brief inline comment inside the ProposalData struct explaining why validity tracking is unnecessary (or that ProposalFeed::fetch() only publishes complete data and handles failures via early return) so future readers know the asymmetry is intentional and not a missing field.src/Makefile.qt.include (1)
247-248:masternodemodel.cppandproposalmodel.cppbreak alphabetical order inBITCOIN_QT_BASE_CPP.The rest of the list is sorted alphabetically, but these two entries are wedged between
bitcoinaddressvalidator.cppandbitcoinamountfield.cpp. They should be placed in their correct positions (masternodemodel.cppnearmodaloverlay.cpp,proposalmodel.cppnearqvalidatedlineedit.cpp).Proposed fix
BITCOIN_QT_BASE_CPP = \ qt/appearancewidget.cpp \ qt/bantablemodel.cpp \ qt/bitcoin.cpp \ qt/bitcoinaddressvalidator.cpp \ - qt/masternodemodel.cpp \ - qt/proposalmodel.cpp \ qt/bitcoinamountfield.cpp \ qt/bitcoingui.cpp \ qt/bitcoinunits.cpp \ qt/clientfeeds.cpp \ qt/clientmodel.cpp \ qt/csvmodelwriter.cpp \ qt/guiutil.cpp \ qt/guiutil_font.cpp \ qt/initexecutor.cpp \ qt/intro.cpp \ + qt/masternodemodel.cpp \ qt/modaloverlay.cpp \ qt/networkstyle.cpp \ qt/notificator.cpp \ qt/optionsdialog.cpp \ qt/optionsmodel.cpp \ qt/peertablemodel.cpp \ qt/peertablesortproxy.cpp \ + qt/proposalmodel.cpp \ qt/qvalidatedlineedit.cpp \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Makefile.qt.include` around lines 247 - 248, The list in BITCOIN_QT_BASE_CPP is out of alphabetical order: move the entries "qt/masternodemodel.cpp" and "qt/proposalmodel.cpp" from their current location between bitcoinaddressvalidator.cpp and bitcoinamountfield.cpp into their correct alphabetical positions — place "qt/masternodemodel.cpp" near "modaloverlay.cpp" (M-section) and "qt/proposalmodel.cpp" near "qvalidatedlineedit.cpp" (P/Q-section) so the entire list remains sorted; update only the ordering in the Makefile fragment, preserving existing whitespace and line endings.src/qt/clientfeeds.cpp (3)
109-141: Samedmnshadowing pattern inProposalFeed::fetch().Line 115 binds
dmnfrom structured binding, and line 124 usesdmn->getValidWeightedMNsCount(). While there's no lambda shadowing here, maintaining consistency with the suggested rename inMasternodeFeed::fetch()would improve clarity (e.g.,mn_list).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/clientfeeds.cpp` around lines 109 - 141, ProposalFeed::fetch() uses a structured binding named dmn from m_client_model.getMasternodeList(), which shadows/duplicates the naming pattern criticized elsewhere; rename the binding (e.g., mn_list or mnList) and update all subsequent uses (currently dmn->getValidWeightedMNsCount() and any other dmn references) to the new name to match the MasternodeFeed::fetch() rename for clarity and consistency.
55-99: Variable shadowing:dmnused for both the mn-list and the lambda parameter.On line 61,
dmnis bound to the masternode list from the structured binding. On lines 78 and 82,dmnis reused as the lambda parameter (projected payee andforEachMNcallback respectively). While this compiles, it shadows the outerdmnand harms readability.Suggested rename
- const auto [dmn, pindex] = m_client_model.getMasternodeList(); + const auto [mn_list, pindex] = m_client_model.getMasternodeList(); - if (!dmn || !pindex) { + if (!mn_list || !pindex) { return; } - auto projectedPayees = dmn->getProjectedMNPayees(pindex); - if (projectedPayees.empty() && dmn->getValidMNsCount() > 0) { + auto projectedPayees = mn_list->getProjectedMNPayees(pindex); + if (projectedPayees.empty() && mn_list->getValidMNsCount() > 0) { return; } auto ret = std::make_shared<Data>(); - ret->m_list_height = dmn->getHeight(); + ret->m_list_height = mn_list->getHeight(); ... - dmn->forEachMN(/*only_valid=*/false, [&](const auto& dmn) { + mn_list->forEachMN(/*only_valid=*/false, [&](const auto& dmn) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/clientfeeds.cpp` around lines 55 - 99, The fetch function currently shadows the outer variable named dmn (from auto [dmn, pindex] = m_client_model.getMasternodeList()) with inner uses named dmn in the projectedPayees loop and the forEachMN lambda; rename one side to avoid shadowing (e.g. rename the structured binding dmn to mnList or mnManager, or rename the lambda parameters to payee and mn) and update all references in MasternodeFeed::fetch (uses in getProjectedMNPayees, projectedPayees loop, nextPayments.emplace, and the forEachMN callback where getCollateralOutpoint(), getProTxHash(), etc. are called) so identifiers are unique and readable.
187-198:start()re-starts all timers at 0ms, making priorrequestForceRefresh()calls redundant.In
clientmodel.cpp,requestForceRefresh()is called on each feed right afteradd(), but thenstart()(line 193-196) restarts all timers with0msanyway. The pre-start()force-refresh calls are effectively no-ops since the timer gets reset. Not a bug, but the redundancy may be confusing to future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/clientfeeds.cpp` around lines 187 - 198, ClientFeeds::start currently restarts every source timer with m_timer->start(0), which forces an immediate timeout and makes prior requestForceRefresh() calls redundant; change the logic to only start timers that are not already active (check QTimer::isActive() on each source->m_timer and call start() only when isActive() is false) so that pre-start requestForceRefresh() results are preserved and you avoid forcing an immediate 0ms restart.src/qt/clientfeeds.h (2)
145-152:add()must only be called beforestart()— consider an assert.The
add<Source>()template creates a feed, registers it (creating a timer), and adds it tom_sources. If called afterstart(), the new feed's timer won't be initially started, and the feed won't participate in the worker thread lifecycle correctly. An assert would catch misuse during development.Suggested safeguard
template<typename Source, typename... Args> Source* add(Args&&... args) { + assert(!m_thread->isRunning() && "feeds must be added before start()"); auto source = std::make_unique<Source>(std::forward<Args>(args)...);Based on learnings, in C++/Qt codebases, use fail-fast asserts in setters to enforce invariants. Prefer assert() for programmer errors that should be caught in development and debugging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/clientfeeds.h` around lines 145 - 152, The add<Source>(...) template must only be called before the feed system is started; add an assert at the top of add() to fail-fast when called after start() (use the existing startup flag used by start()/m_started or the lifecycle indicator checked by registerFeed/start). Modify the add() implementation (referencing add, registerFeed, m_sources and the start() lifecycle flag) to Q_ASSERT or assert(!startedFlag) so new feeds can't be added once start() has run, keeping registerFeed and m_sources behavior unchanged.
96-100:MasternodeDatahasm_validbutProposalDatadoes not — intentional asymmetry?
MasternodeDatahas anm_validfield (defaulting tofalse) that is checked inMasternodeList::updateMasternodeList()to schedule retries.ProposalData(lines 117-122) lacks this field, soProposalFeed::fetch()either publishes complete data or publishes nothing (early return). The asymmetry is fine if intentional, but consider adding a brief comment onProposalDataexplaining why validity tracking isn't needed there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/clientfeeds.h` around lines 96 - 100, MasternodeData includes an m_valid flag used by MasternodeList::updateMasternodeList() to schedule retries, while ProposalData currently lacks this flag; add a brief inline comment inside the ProposalData struct explaining why validity tracking is unnecessary (or that ProposalFeed::fetch() only publishes complete data and handles failures via early return) so future readers know the asymmetry is intentional and not a missing field.
|
This pull request has conflicts, please rebase. |
1 similar comment
|
This pull request has conflicts, please rebase. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/qt/clientfeeds.cpp (1)
76-79: Variabledmnon line 78 shadows the structured binding on line 61.The inner
dmn(the projected payee) shadows the outerdmn(the masternode list). It works because the inner scope ends before line 82 uses the outer one, but this is confusing and fragile if the loop body is ever extended.♻️ Suggested rename
for (size_t i = 0; i < projectedPayees.size(); i++) { - const auto& dmn = projectedPayees[i]; - nextPayments.emplace(dmn->getProTxHash(), ret->m_list_height + (int)i + 1); + const auto& payee = projectedPayees[i]; + nextPayments.emplace(payee->getProTxHash(), ret->m_list_height + (int)i + 1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/clientfeeds.cpp` around lines 76 - 79, The inner loop variable dmn in the block that iterates projectedPayees shadows the outer structured-binding dmn; rename the inner variable to a clearer, non-conflicting name (e.g., projectedPayee or payee) in the for-loop that builds nextPayments so you still call projectedPayee->getProTxHash() and compute ret->m_list_height + (int)i + 1 without changing logic; update all uses inside that loop (the nextPayments.emplace call) to the new identifier to remove the shadowing and avoid future confusion.src/qt/clientfeeds.h (1)
94-100: Consider consolidatingMasternodeEntryListalias nearMasternodeEntry.The
MasternodeEntryListalias is defined here in the feed header. For discoverability, it may be better placed alongside theMasternodeEntryclass definition. However, this is a minor organizational preference.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/clientfeeds.h` around lines 94 - 100, Move the MasternodeEntryList type alias next to the MasternodeEntry class definition to improve discoverability: locate the current alias MasternodeEntryList (std::vector<std::shared_ptr<MasternodeEntry>>) and cut it from src/qt/clientfeeds.h where MasternodeData is declared, then paste it adjacent to the MasternodeEntry class/struct declaration so references (MasternodeEntryList, MasternodeEntry) are colocated; ensure MasternodeData continues to include m_entries of type MasternodeEntryList and update any includes or forward declarations if necessary.src/qt/proposallist.cpp (1)
308-312: Three separate connections fromQDialog::accepted— consider consolidating.Lines 309–311 connect three separate lambdas/slots to
proposalCreate'sacceptedsignal. While functionally correct (all will fire), a single slot that performs all three actions would be slightly cleaner and make the execution order explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/proposallist.cpp` around lines 308 - 312, Consolidate the three separate connections to QDialog::accepted into a single slot so the actions run in a defined order: replace the three connect calls that reference proposalCreate, QDialog::accepted, m_feed->requestForceRefresh(), ProposalList::updateProposalButtons, and ProposalList::showResumeProposalDialog with one connect that calls a single lambda or a new ProposalList method which (1) calls m_feed->requestForceRefresh() if m_feed, (2) calls updateProposalButtons(), and (3) calls showResumeProposalDialog(), then keep proposalCreate->show() as-is.src/qt/proposallist.h (1)
23-23: Remove unusedCDeterministicMNListforward declaration.The forward declaration at line 23 is not used in any member declarations, method signatures, or in the corresponding
.cppfile and can be safely removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/proposallist.h` at line 23, Remove the unused forward declaration of CDeterministicMNList in proposallist.h: locate the line declaring "class CDeterministicMNList;" and delete it since it is not referenced by any member declarations, method signatures, or in the corresponding .cpp; ensure no other code relies on that forward declaration (e.g., check for usages of CDeterministicMNList in proposallist.cpp) and run a build to verify no missing symbols.src/qt/proposalmodel.h (1)
95-95: Consolidate duplicateProposalstype alias to a single header.This type alias is defined identically in both
proposalmodel.h(line 95) andclientfeeds.h(line 115). Define it once in a shared header and include from both files to avoid maintenance issues if one definition diverges from the other.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/proposalmodel.h` at line 95, The Proposals type alias (using Proposals = std::vector<std::shared_ptr<Proposal>>;) is duplicated; remove the duplicate and centralize it: create a single shared header (e.g., a new proposals_types.h) that defines the Proposals alias and include that header from both proposalmodel.h and clientfeeds.h, then delete the local alias definitions in ProposalModel and ClientFeeds headers so both use the same centralized Proposals type; ensure the shared header forward-declares Proposal or includes the Proposal definition as needed to compile.src/qt/masternodelist.h (1)
34-34: Remove unusedQThreadforward declaration.The
QThreadforward declaration is no longer referenced in this header after the removal ofm_thread. The class has no members or methods that use it. Additionally, the#include <QThread>in masternodelist.cpp appears to be dead code and should also be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/masternodelist.h` at line 34, Remove the now-unused QThread forward declaration from the header: delete the line declaring "class QThread;" in masternodelist.h and also remove the corresponding "#include <QThread>" from masternodelist.cpp since m_thread was removed and no code references QThread; ensure no other symbols in MasternodeList or functions reference QThread before committing.
🤖 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/clientfeeds.h`:
- Around line 28-59: m_syncing is a plain bool causing a data race between
setSyncing() (main thread) and reads in requestRefresh() (worker thread); change
m_syncing to std::atomic<bool> in class FeedBase and update its initialization
and usage so setSyncing() writes atomically and reads in requestRefresh()/retry
handlers use the atomic value (leave method names setSyncing(),
requestRefresh(), and the FeedBase::m_syncing identifier intact so callers
remain unchanged).
---
Duplicate comments:
In `@src/qt/clientfeeds.h`:
- Around line 137-166: The stop() implementation must explicitly free the
heap-allocated m_worker because deleteLater() tied to QThread::finished may
never run after quit()+wait(); modify stop() to, after calling m_thread->quit()
and m_thread->wait(), check m_worker and delete it (delete m_worker; m_worker =
nullptr) and likewise delete or cleanup m_thread, ensuring no orphaned pointers
remain; reference m_worker, m_thread, and stop() when making this change.
In `@src/qt/clientmodel.cpp`:
- Around line 88-94: The masternode feed (m_feed_masternode) is not being
refreshed on block height updates so its nNextPayment values (computed in
MasternodeFeed::fetch()) grow stale; update the numBlocksChanged handler (the
same lambda that calls m_feed_proposal->requestRefresh()) to also call
m_feed_masternode->requestRefresh() when header is false so the masternode feed
is refreshed on each block change and keeps next-payment heights accurate.
Ensure you reference m_feed_masternode and MasternodeFeed::fetch() behavior when
adding the call and guard it the same way you guard the proposal refresh.
In `@src/qt/proposallist.h`:
- Around line 43-44: The doc comment above the ProposalList class is stale
("Governance Manager page widget"); update the comment for the ProposalList
class to accurately describe its current purpose and behavior (e.g., "Proposal
list view widget" or a brief summary of what ProposalList does), ensuring the
comment directly precedes the class declaration for ProposalList so future
reviewers see the correct description.
---
Nitpick comments:
In `@src/qt/clientfeeds.cpp`:
- Around line 76-79: The inner loop variable dmn in the block that iterates
projectedPayees shadows the outer structured-binding dmn; rename the inner
variable to a clearer, non-conflicting name (e.g., projectedPayee or payee) in
the for-loop that builds nextPayments so you still call
projectedPayee->getProTxHash() and compute ret->m_list_height + (int)i + 1
without changing logic; update all uses inside that loop (the
nextPayments.emplace call) to the new identifier to remove the shadowing and
avoid future confusion.
In `@src/qt/clientfeeds.h`:
- Around line 94-100: Move the MasternodeEntryList type alias next to the
MasternodeEntry class definition to improve discoverability: locate the current
alias MasternodeEntryList (std::vector<std::shared_ptr<MasternodeEntry>>) and
cut it from src/qt/clientfeeds.h where MasternodeData is declared, then paste it
adjacent to the MasternodeEntry class/struct declaration so references
(MasternodeEntryList, MasternodeEntry) are colocated; ensure MasternodeData
continues to include m_entries of type MasternodeEntryList and update any
includes or forward declarations if necessary.
In `@src/qt/masternodelist.h`:
- Line 34: Remove the now-unused QThread forward declaration from the header:
delete the line declaring "class QThread;" in masternodelist.h and also remove
the corresponding "#include <QThread>" from masternodelist.cpp since m_thread
was removed and no code references QThread; ensure no other symbols in
MasternodeList or functions reference QThread before committing.
In `@src/qt/proposallist.cpp`:
- Around line 308-312: Consolidate the three separate connections to
QDialog::accepted into a single slot so the actions run in a defined order:
replace the three connect calls that reference proposalCreate,
QDialog::accepted, m_feed->requestForceRefresh(),
ProposalList::updateProposalButtons, and ProposalList::showResumeProposalDialog
with one connect that calls a single lambda or a new ProposalList method which
(1) calls m_feed->requestForceRefresh() if m_feed, (2) calls
updateProposalButtons(), and (3) calls showResumeProposalDialog(), then keep
proposalCreate->show() as-is.
In `@src/qt/proposallist.h`:
- Line 23: Remove the unused forward declaration of CDeterministicMNList in
proposallist.h: locate the line declaring "class CDeterministicMNList;" and
delete it since it is not referenced by any member declarations, method
signatures, or in the corresponding .cpp; ensure no other code relies on that
forward declaration (e.g., check for usages of CDeterministicMNList in
proposallist.cpp) and run a build to verify no missing symbols.
In `@src/qt/proposalmodel.h`:
- Line 95: The Proposals type alias (using Proposals =
std::vector<std::shared_ptr<Proposal>>;) is duplicated; remove the duplicate and
centralize it: create a single shared header (e.g., a new proposals_types.h)
that defines the Proposals alias and include that header from both
proposalmodel.h and clientfeeds.h, then delete the local alias definitions in
ProposalModel and ClientFeeds headers so both use the same centralized Proposals
type; ensure the shared header forward-declares Proposal or includes the
Proposal definition as needed to compile.
kwvg
force-pushed
the
client_feeds
branch
2 times, most recently
from
f9fa02f to
b989d7b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/qt/proposallist.cpp (1)
480-575:⚠️ Potential issue | 🟠 MajorGuard against null
mnListbefore dereference.
getMasternodeList()can return a null list even whenpindexis non-null (as indicated by other call sites). The current code only checkspindexand then dereferencesmnList, which can crash.🐛 Proposed fix
- auto [mnList, pindex] = clientModel->getMasternodeList(); - if (!pindex) { + auto [mnList, pindex] = clientModel->getMasternodeList(); + if (!mnList || !pindex) { QMessageBox::warning(this, tr("Voting Failed"), tr("Unable to get masternode list. Please try again later.")); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/proposallist.cpp` around lines 480 - 575, The code calls clientModel->getMasternodeList() and only checks pindex but then dereferences mnList (mnList->getValidMN), which can be null; update the block after auto [mnList, pindex] to also check if (!mnList) and handle it the same as a missing pindex (QMessageBox::warning with "Unable to get masternode list. Please try again later." then return) so you never dereference a null mnList; reference getMasternodeList, mnList, pindex, and mnList->getValidMN when making the change.
🤖 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/proposallist.cpp`:
- Around line 5-23: The code uses std::max and std::clamp in
refreshColumnWidths() but doesn't include <algorithm>, which is brittle; fix by
adding a direct `#include` <algorithm> to the top of proposallist.cpp (near the
other standard includes) so that std::max and std::clamp are available; update
any include ordering if needed to keep style consistent and ensure
refreshColumnWidths() compiles across toolchains.
---
Outside diff comments:
In `@src/qt/proposallist.cpp`:
- Around line 480-575: The code calls clientModel->getMasternodeList() and only
checks pindex but then dereferences mnList (mnList->getValidMN), which can be
null; update the block after auto [mnList, pindex] to also check if (!mnList)
and handle it the same as a missing pindex (QMessageBox::warning with "Unable to
get masternode list. Please try again later." then return) so you never
dereference a null mnList; reference getMasternodeList, mnList, pindex, and
mnList->getValidMN when making the change.
---
Duplicate comments:
In `@src/qt/clientfeeds.cpp`:
- Around line 143-154: The destructor may double-delete m_worker because start()
can reparent it for deletion via deleteLater(); fix by transferring ownership
when starting: in ClientFeeds::start() call m_worker->setParent(nullptr) (or
otherwise remove this as parent) before moving it to the QThread and connecting
QThread::finished to m_worker->deleteLater(), and keep the explicit delete in
~ClientFeeds() so that if start() was never called the QObject parent will still
safely delete m_worker; refer to ClientFeeds, start(), stop(), m_worker, and
~ClientFeeds().
In `@src/qt/clientmodel.cpp`:
- Around line 78-80: The masternode next payment heights can go stale because
m_feed_masternode is only refreshed on masternodeListChanged; update the
numBlocksChanged handler (the lambda connected to ClientModel::numBlocksChanged
where ProposalFeed is refreshed) to also call
m_feed_masternode->requestRefresh() so the MasternodeFeed recalculates
height-dependent fields on every new block; ensure you reference the existing
m_feed_masternode and its requestRefresh() method inside that same
numBlocksChanged lambda.
|
Issues and Suggestions
clientfeeds.cpp:58 — util::ThreadRename("qt-clientfeed") is called inside the registerFeed
clientfeeds.h:97-103 — If add() were called after stop(), the feed would be registered but |
kwvg
force-pushed
the
client_feeds
branch
3 times, most recently
from
adaad00 to
7b927bd
Compare
5e5aaec fix: MN update notifications had old_list/new_list swapped (UdjinM6) Pull request description: ## Issue being fixed or feature implemented - UI: Masternode list tab displayed state at block X-1 while node was at block X - Net: mnauth disconnect handling could miss removals, keeping removed masternode peers connected longer than intended Noticed while reviewing #7146 ## What was done? ## How Has This Been Tested? Run qt, open masternode list tab, sort by last paid (desc). Open Info dialog to see block height, wait for another block after full sync and compare. ## 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 _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 5e5aaec kwvg: utACK 5e5aaec Tree-SHA512: 00cd0abd17a46e025f4419b8c06ca0ac5a5edbb07a372da75f1b40866b9b218116fa803f4f903b4010dd878aa05e6377d3ca7e8df99aa184d6b24a0f76a00325
Will be needed as we start consolidating non-wallet data sourcing into a central thread to prevent duplicated fetching. Review with `git log -p -n1 --color-moved=dimmed_zebra --ignore-space-change`.
The current system spins up threads for each wallet instance, this is not sustainable for clients that load a lot of wallets and is computationally expensive for redundant fetches
Same reasoning as previous commit and we consolidate two loops while we're at it. Review with `git log -p -n1 --color-moved=dimmed_zebra --ignore-space-change`.
3 participants
Additional Information
Dependency for feat(qt): UI refresh (4/n, introduce distinct widgets for Dash-specific reporting in debug window) #7118
The redesigned "Masternode" and "Governance" tabs (see dash#7110 and dash#7116) were migrated to a worker-based update mechanism with a773635 and others, this proved to deal with main thread contention and the debounce mechanism prevented frequent refreshes of infrequently updated data and was a step towards throttled event-driven updates.
This implementation though had a problematic side effect, a worker thread is spawned per tab per wallet and while per tab threads were by design, per wallet threads meant redundant work happening all at once fighting over the same locks, which cause noticeable performance degradation.
This pull request therefore introduces the concept of "feeds", that perform the expensive fetch operation that are either triggered by events (and debounced) or user feedback (and executed immediately) to ensure snappiness. The IBD throttling logic is respected.
The result is having a central worker thread fetching (
fetch()) and dispatching refresh signals (dataReady()) which are then used by the consumer (e.g.updateMasternodeList()) to fetch wallet-specific information (which is relatively inexpensive) and then apply those changes (e.g.setMasternodeList()).Note that masternodes and proposal views use different refresh intervals, masternode data uses 3 seconds and proposal data uses 10 seconds, scaled up to 30s and 60s respectively during IBD.
Breaking Changes
None expected.
Checklist