Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/Makefile.qt.include
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,6 @@ BITCOIN_QT_H = \
qt/transactionrecord.h \
qt/transactiontablemodel.h \
qt/transactionview.h \
qt/util.h \
qt/utilitydialog.h \
qt/walletcontroller.h \
qt/walletframe.h \
Expand Down
1 change: 0 additions & 1 deletion src/interfaces/node.h
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,6 @@ class GOV
uint16_t m_evo{0};
};
virtual UniqueVoters getObjUniqueVoters(const CGovernanceObject& obj, vote_signal_enum_t vote_signal) = 0;
virtual bool getObjLocalValidity(const CGovernanceObject& obj, std::string& error, bool check_collateral) = 0;
virtual bool existsObj(const uint256& hash) = 0;
virtual bool isEnabled() = 0;
virtual bool processVoteAndRelay(const CGovernanceVote& vote, std::string& error) = 0;
Expand Down
8 changes: 0 additions & 8 deletions src/node/interfaces.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -265,14 +265,6 @@ class GOVImpl : public GOV
}
return false;
}
bool getObjLocalValidity(const CGovernanceObject& obj, std::string& error, bool check_collateral) override
{
if (context().govman != nullptr && context().chainman != nullptr && context().dmnman != nullptr) {
LOCK(cs_main);
return obj.IsValidLocally(context().dmnman->GetListAtChainTip(), *(context().chainman), error, check_collateral);
}
return false;
}
bool isEnabled() override
{
if (context().govman != nullptr) {
Expand Down
5 changes: 1 addition & 4 deletions src/qt/clientfeeds.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
#include <qt/clientmodel.h>
#include <qt/masternodemodel.h>
#include <qt/proposalmodel.h>
#include <qt/util.h>

#include <QDebug>
#include <QThread>
Expand Down Expand Up @@ -188,9 +187,7 @@ void MasternodeFeed::fetch()
nNextPayment = nextPaymentIt->second;
}
auto entry = std::make_shared<MasternodeEntry>(dmn, collateralStr, nNextPayment);
ret->m_by_protx[dmn->getProTxHash()] = entry.get();
ret->m_by_service[util::make_array(dmn->getNetInfoPrimary().GetKey())] = entry.get();
ret->m_entries.push_back(std::move(entry));
ret->m_entries.emplace_back(std::move(entry));
});

ret->m_valid = true;
Expand Down
6 changes: 2 additions & 4 deletions src/qt/clientfeeds.h
Original file line number Diff line number Diff line change
Expand Up @@ -151,10 +151,8 @@ class InstantSendFeed : public Feed<InstantSendData> {
struct MasternodeData {
bool m_valid{false};
int m_list_height{0};
interfaces::MnList::Counts m_counts{};
QHash<QByteArray, const MasternodeEntry*> m_by_service{};
std::vector<std::shared_ptr<MasternodeEntry>> m_entries{};
Uint256HashMap<const MasternodeEntry*> m_by_protx{};
interfaces::MnList::Counts m_counts;
std::vector<std::shared_ptr<MasternodeEntry>> m_entries;
Comment on lines 151 to +155
Copy link

@coderabbitai coderabbitai bot Feb 24, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

cat -n src/qt/clientfeeds.h | sed -n '145,160p'

Repository: dashpay/dash

Length of output: 489


🏁 Script executed:

rg -n -A5 'struct MasternodeData' src

Repository: dashpay/dash

Length of output: 694


🏁 Script executed:

rg -n 'MasternodeData\s*[({]' src --type=cpp --type=cc --type=h --type=hpp

Repository: dashpay/dash

Length of output: 82


🏁 Script executed:

rg -n 'interfaces::MnList::Counts' src --type=cpp --type=cc --type=h --type=hpp -B2 -A2

Repository: dashpay/dash

Length of output: 82


🏁 Script executed:

rg -n 'MasternodeData' src --type=cpp --type=h -B2 -A5 | head -80

Repository: dashpay/dash

Length of output: 3045


🏁 Script executed:

fd . src -name '*.h' -o -name '*.cpp' | xargs grep -l 'MnList::Counts' | head -5

Repository: dashpay/dash

Length of output: 286


🏁 Script executed:

rg -n 'namespace.*MnList\|struct.*Counts\|class.*Counts' src --type=cpp --type=h -A3

Repository: dashpay/dash

Length of output: 38


🏁 Script executed:

find src -name '*.h' -o -name '*.cpp' | xargs grep -l 'Counts\s*[{=:]' | head -10

Repository: dashpay/dash

Length of output: 158


🏁 Script executed:

rg 'struct Counts|class Counts' src --type=cpp --type=h -A10

Repository: dashpay/dash

Length of output: 4075


🏁 Script executed:

rg 'MnList::Counts' src --type=cpp --type=h -B3 -A3

Repository: dashpay/dash

Length of output: 383


🏁 Script executed:

rg -n 'm_counts' src/qt --type=cpp --type=h -B2 -A2

Repository: dashpay/dash

Length of output: 3934


🏁 Script executed:

cat -n src/qt/masternodelist.cpp | sed -n '200,225p'

Repository: dashpay/dash

Length of output: 1095


🏁 Script executed:

rg -n 'ret\.' src/qt/masternodelist.cpp | grep -A2 -B2 'MasternodeData ret'

Repository: dashpay/dash

Length of output: 38


🏁 Script executed:

cat -n src/interfaces/node.h | sed -n '1,50p' | grep -A20 'struct Counts'

Repository: dashpay/dash

Length of output: 38


🏁 Script executed:

rg -n 'struct Counts' src/interfaces/node.h -A12

Repository: dashpay/dash

Length of output: 576


Add initializer for consistency with similar structures in the same file.

Line 154 omits the {} initializer on m_counts. While the interfaces::MnList::Counts type safely zero-initializes all its members (defined in src/interfaces/node.h), other similar data structures in src/qt/clientfeeds.h (CreditPoolData at line 117 and InstantSendData at line 135) explicitly initialize their m_counts members with {}. For consistency and clarity:

Suggested fix
-    interfaces::MnList::Counts m_counts;
+    interfaces::MnList::Counts m_counts{};
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
struct MasternodeData {
bool m_valid{false};
int m_list_height{0};
interfaces::MnList::Counts m_counts{};
QHash<QByteArray, const MasternodeEntry*> m_by_service{};
std::vector<std::shared_ptr<MasternodeEntry>> m_entries{};
Uint256HashMap<const MasternodeEntry*> m_by_protx{};
interfaces::MnList::Counts m_counts;
std::vector<std::shared_ptr<MasternodeEntry>> m_entries;
struct MasternodeData {
bool m_valid{false};
int m_list_height{0};
interfaces::MnList::Counts m_counts{};
std::vector<std::shared_ptr<MasternodeEntry>> m_entries;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/qt/clientfeeds.h` around lines 151 - 155, MasternodeData's m_counts
member (type interfaces::MnList::Counts) lacks the {} initializer for
consistency with similar structs; update the MasternodeData definition to
initialize m_counts with {} (matching CreditPoolData and InstantSendData) so the
line declaring m_counts uses m_counts{} to explicitly value-initialize it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's intentionally removed. interfaces::MnList::Counts is a struct and have default initialized members. {} is not needed here. Moreover it's missleading initialization that makes code reader to assume that initialization by {} is required.

other similar data structures in src/qt/clientfeeds.h (CreditPoolData at line 117 and InstantSendData at line 135) explicitly initialize their m_counts members with {}. For consistency and clarity:

Quite contradicately, m_counts is almost the only member that initialized by this way.
For reference, the latest merged PR to bitcoin:
bitcoin@d907d65

        std::string block_start_name;

There's no enforced call of default constructor. It's default constructor.

More examples class BlockManager (randomly chosen to show a point):

    RecursiveMutex cs_LastBlockFile;
    std::vector<CBlockFileInfo> m_blockfile_info;                                                                                        
    int m_last_blockfile = 0;
    /** Global flag to indicate we should check to see if there are                                                                      
     *  block/undo files that should be deleted.  Set on startup                                                                         
     *  or if we allocate more file space when we're in prune mode                                                                       
     */
    bool m_check_for_pruning = false;
    
    /** Dirty block index entries. */
    std::set<CBlockIndex*> m_dirty_blockindex;                                                                                           
    
    /** Dirty block file entries. */
    std::set<int> m_dirty_fileinfo;
    
    /**
     * Map from external index name to oldest block that must not be pruned.                                                             
     *
     * @note Internally, only blocks at height (height_first - PRUNE_LOCK_BUFFER - 1) and                                                
     * below will be pruned, but callers should avoid assuming any particular buffer size.                                               
     */
    std::unordered_map<std::string, PruneLockInfo> m_prune_locks GUARDED_BY(::cs_main);

As you can see, trivial POD members, such as bool, int have initialized; any struct or class - do not.

Copy link

Choose a reason for hiding this comment

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

@knst, thank you for the detailed explanation! You're absolutely right—I appreciate the clarification on the coding convention. The distinction between explicitly initializing POD types (like bool and int) versus letting struct/class types use their default constructors makes sense and aligns with Bitcoin Core's style.

Your point about {} being potentially misleading is well taken—it can indeed suggest that explicit initialization is required when the default constructor already handles it properly. I'll keep this pattern in mind for future reviews.


✏️ Learnings added
Learnt from: knst
Repo: dashpay/dash PR: 7176
File: src/qt/clientfeeds.h:151-155
Timestamp: 2026-02-24T12:52:51.913Z
Learning: In Dash Core (following Bitcoin Core conventions), member variable initialization style differs by type: POD types (bool, int) should use explicit in-class initialization (e.g., `bool m_flag = false;`), while struct and class types should rely on their default constructors without `{}` (e.g., `interfaces::MnList::Counts m_counts;` not `m_counts{}`). Explicit `{}` for struct/class members can be misleading as it suggests initialization is required when the default constructor already handles it.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,evo,llmq,governance,coinjoin}/**/*.{cpp,h} : Use Dash-specific database implementations: CFlatDB for persistent storage (MasternodeMetaStore, GovernanceStore, SporkStore, NetFulfilledRequestStore) and CDBWrapper extensions for Evolution/DKG/InstantSend/Quorum/RecoveredSigs data

Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h} : Masternode lists must use immutable data structures (Immer library) for thread safety

Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/evo/evodb/**/*.{cpp,h} : Evolution Database (CEvoDb) must handle masternode snapshots, quorum state, governance objects with efficient differential updates for masternode lists

Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:41:22.457Z
Learning: Applies to src/{masternode,llmq,evo,coinjoin,governance}/**/*.{cpp,h} : Use unordered_lru_cache for efficient caching with LRU eviction in Dash-specific data structures

Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (`#6718`), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.

Learnt from: knst
Repo: dashpay/dash PR: 6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.801Z
Learning: In PR `#6692`, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR `#6789` to address it, consistent with avoiding scope creep in performance-focused PRs.

Learnt from: kwvg
Repo: dashpay/dash PR: 7068
File: src/qt/guiutil_font.h:68-77
Timestamp: 2025-12-22T15:42:48.595Z
Learning: In C++/Qt codebases, use fail-fast asserts in setters to enforce invariants (e.g., ensuring internal maps contain necessary keys). Prefer assert() for programmer errors that should be caught in development and debugging, rather than defensive runtime checks with fallbacks. This helps catch invariant violations early during development. Apply to header and source files where invariant-driven state mutations occur, especially in setters like SetWeightBold/SetWeightNormal that assume established relationships or preconditions.

};

class MasternodeFeed : public Feed<MasternodeData> {
Expand Down
5 changes: 5 additions & 0 deletions src/qt/guiutil.h
Original file line number Diff line number Diff line change
Expand Up @@ -531,6 +531,11 @@ namespace GUIUtil
return false;
}

template <typename T1>
inline QByteArray MakeQByteArray(const T1& data)
{
return QByteArray(reinterpret_cast<const char*>(data.data()), data.size());
}
} // namespace GUIUtil

#endif // BITCOIN_QT_GUIUTIL_H
14 changes: 8 additions & 6 deletions src/qt/proposallist.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -576,16 +576,19 @@ void ProposalList::voteForProposal(vote_outcome_enum_t outcome)
// Vote with each masternode
for (const auto& [proTxHash, votingKeyID] : votableMasternodes) {
// Find the masternode
const auto* dmn = [&]() -> const MasternodeEntry* {
if (const auto it = data_mn->m_by_protx.find(proTxHash); it != data_mn->m_by_protx.end() && !it->second->isBanned()) {
return it->second;
QString protx_hash{QString::fromStdString(proTxHash.ToString())};
const auto dmn = [&]() -> const std::shared_ptr<MasternodeEntry> {
for (const auto& mn : data_mn->m_entries) {
if (mn->proTxHash() == protx_hash) {
return mn->isBanned() ? nullptr : mn;
}
}
return nullptr;
}();

if (!dmn) {
nFailed++;
failedMessages.append(tr("Masternode %1 not found").arg(QString::fromStdString(proTxHash.ToString())));
failedMessages.append(tr("Masternode %1 not found").arg(protx_hash));
continue;
}

Expand All @@ -595,8 +598,7 @@ void ProposalList::voteForProposal(vote_outcome_enum_t outcome)
// Sign vote via wallet interface
if (!walletModel->wallet().signGovernanceVote(votingKeyID, vote)) {
nFailed++;
failedMessages.append(
tr("Failed to sign vote for masternode %1").arg(QString::fromStdString(proTxHash.ToString())));
failedMessages.append(tr("Failed to sign vote for masternode %1").arg(protx_hash));
continue;
}

Expand Down
9 changes: 4 additions & 5 deletions src/qt/rpcconsole.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
#include <qt/masternodemodel.h>
#include <qt/networkwidget.h>
#include <qt/peertablesortproxy.h>
#include <qt/util.h>
#include <qt/walletcontroller.h>
#include <qt/walletmodel.h>

Expand Down Expand Up @@ -1291,12 +1290,12 @@ void RPCConsole::updateDetailWidget()
}
ui->peerMappedAS->setText(stats->nodeStats.m_mapped_as != 0 ? QString::number(stats->nodeStats.m_mapped_as) : ts.na);

const auto addr_key{util::make_array(stats->nodeStats.addr.GetKey())};
const MasternodeEntry* dmn = [&]() -> const MasternodeEntry* {
const auto addr_key{GUIUtil::MakeQByteArray(stats->nodeStats.addr.GetKey())};
const std::shared_ptr<MasternodeEntry> dmn = [&]() -> const std::shared_ptr<MasternodeEntry> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider using MasternodeEntryList alias

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

std::shared_ptr<MasternodeEntry> dmn but it is not a list here; just a single item

if (m_feed_masternode) {
if (const auto data{m_feed_masternode->data()}; data) {
if (auto it = data->m_by_service.find(addr_key); it != data->m_by_service.end()) {
return it.value();
for (const auto& mn : data->m_entries) {
if (mn->serviceKey() == addr_key) return mn;
}
}
}
Expand Down
18 changes: 0 additions & 18 deletions src/qt/util.h

This file was deleted.

1 change: 0 additions & 1 deletion test/util/data/non-backported.txt
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ src/qt/proposalinfo.*
src/qt/proposallist.*
src/qt/proposalmodel.*
src/qt/proposalresume.*
src/qt/util.h
src/rpc/coinjoin.cpp
src/rpc/evo.cpp
src/rpc/evo_util.*
Expand Down
Loading