perf: do linear lookup instead building 2 heavy Hash-Maps#7176
perf: do linear lookup instead building 2 heavy Hash-Maps#7176knst wants to merge 4 commits intodashpay:developfrom
Conversation
…-Maps which used almost never It also fixes a bug with possible UB and dangling pointer to masternode if list has been re-created
|
WalkthroughThis pull request removes src/qt/util.h and its template make_array, removes qt/util.h from the public BITCOIN_QT_H header list, and adds GUIUtil::MakeQByteArray in src/qt/guiutil.h. It deletes GOV::getObjLocalValidity from interfaces and its GOVImpl implementation. MasternodeData no longer contains m_by_service and m_by_protx; callers (clientfeeds, proposallist, rpcconsole) were updated to use linear iteration over m_entries and to use std::shared_ptr and the new QByteArray helper. Sequence Diagram(s)(Skipped — changes are refactoring and API removal without a new multi-component sequential control flow.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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: 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)
552-557:⚠️ Potential issue | 🟡 MinorFix clang-format diff in the failedMessages block
CI flagged formatting differences for Lines 552-559. Please run clang-format (or the repo formatting script) on this block.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/proposallist.cpp` around lines 552 - 557, The failedMessages.append block in proposallist.cpp (inside the "Sign vote via wallet interface" branch where walletModel->wallet().signGovernanceVote is called) is misformatted; run the project's clang-format or formatting script on that block (the failedMessages.append(...) and surrounding braces/continue) so indentation, line breaks and brace placement match the repo style and resolve the CI clang-format diff.
🧹 Nitpick comments (1)
src/qt/guiutil.h (1)
534-538: Add safeguards toMakeQByteArraytemplate to prevent misuse with non-byte containersThe template accepts any type with
.data()and.size(), butQByteArrayinterprets the size as a byte count. While the current call site inrpcconsole.cppsafely passesstd::vector<unsigned char>(where element count equals byte count), adding a static assertion and explicit narrowing cast would prevent future misuse with containers likestd::vector<int>.♻️ Suggested hardening
template <typename T1> inline QByteArray MakeQByteArray(const T1& data) { - return QByteArray(reinterpret_cast<const char*>(data.data()), data.size()); + static_assert(sizeof(*data.data()) == 1, "MakeQByteArray expects byte-sized elements"); + return QByteArray(reinterpret_cast<const char*>(data.data()), + static_cast<int>(data.size())); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/guiutil.h` around lines 534 - 538, MakeQByteArray currently accepts any type with .data() and .size(), but QByteArray expects a byte count; add a compile-time guard and an explicit narrowing cast to avoid misuse with non-byte element types. In MakeQByteArray add a static_assert that sizeof(typename T1::value_type) == 1 (or that value_type is unsigned char/char/uint8_t) to ensure element size is one byte, and change the size argument to QByteArray to use an explicit static_cast<int>(data.size()) so you don't pass a size_t directly; keep the reinterpret_cast<const char*>(data.data()) for the pointer.
🤖 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 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.
---
Outside diff comments:
In `@src/qt/proposallist.cpp`:
- Around line 552-557: The failedMessages.append block in proposallist.cpp
(inside the "Sign vote via wallet interface" branch where
walletModel->wallet().signGovernanceVote is called) is misformatted; run the
project's clang-format or formatting script on that block (the
failedMessages.append(...) and surrounding braces/continue) so indentation, line
breaks and brace placement match the repo style and resolve the CI clang-format
diff.
---
Nitpick comments:
In `@src/qt/guiutil.h`:
- Around line 534-538: MakeQByteArray currently accepts any type with .data()
and .size(), but QByteArray expects a byte count; add a compile-time guard and
an explicit narrowing cast to avoid misuse with non-byte element types. In
MakeQByteArray add a static_assert that sizeof(typename T1::value_type) == 1 (or
that value_type is unsigned char/char/uint8_t) to ensure element size is one
byte, and change the size argument to QByteArray to use an explicit
static_cast<int>(data.size()) so you don't pass a size_t directly; keep the
reinterpret_cast<const char*>(data.data()) for the pointer.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/Makefile.qt.includesrc/interfaces/node.hsrc/node/interfaces.cppsrc/qt/clientfeeds.cppsrc/qt/clientfeeds.hsrc/qt/guiutil.hsrc/qt/proposallist.cppsrc/qt/rpcconsole.cppsrc/qt/util.h
💤 Files with no reviewable changes (4)
- src/interfaces/node.h
- src/qt/util.h
- src/Makefile.qt.include
- src/node/interfaces.cpp
| 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; |
There was a problem hiding this comment.
🧩 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' srcRepository: dashpay/dash
Length of output: 694
🏁 Script executed:
rg -n 'MasternodeData\s*[({]' src --type=cpp --type=cc --type=h --type=hppRepository: dashpay/dash
Length of output: 82
🏁 Script executed:
rg -n 'interfaces::MnList::Counts' src --type=cpp --type=cc --type=h --type=hpp -B2 -A2Repository: dashpay/dash
Length of output: 82
🏁 Script executed:
rg -n 'MasternodeData' src --type=cpp --type=h -B2 -A5 | head -80Repository: dashpay/dash
Length of output: 3045
🏁 Script executed:
fd . src -name '*.h' -o -name '*.cpp' | xargs grep -l 'MnList::Counts' | head -5Repository: dashpay/dash
Length of output: 286
🏁 Script executed:
rg -n 'namespace.*MnList\|struct.*Counts\|class.*Counts' src --type=cpp --type=h -A3Repository: dashpay/dash
Length of output: 38
🏁 Script executed:
find src -name '*.h' -o -name '*.cpp' | xargs grep -l 'Counts\s*[{=:]' | head -10Repository: dashpay/dash
Length of output: 158
🏁 Script executed:
rg 'struct Counts|class Counts' src --type=cpp --type=h -A10Repository: dashpay/dash
Length of output: 4075
🏁 Script executed:
rg 'MnList::Counts' src --type=cpp --type=h -B3 -A3Repository: dashpay/dash
Length of output: 383
🏁 Script executed:
rg -n 'm_counts' src/qt --type=cpp --type=h -B2 -A2Repository: 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 -A12Repository: 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.
| 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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
| 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> { |
There was a problem hiding this comment.
Consider using MasternodeEntryList alias
There was a problem hiding this comment.
std::shared_ptr<MasternodeEntry> dmn but it is not a list here; just a single item
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/qt/proposallist.cpp (1)
535-543: Compare raw proTxHash values in the scan to avoid QString conversions.You already have
proTxHashas auint256, so comparingmn->proTxHashRaw()keeps the lookup binary and avoids per-entry string work. This preserves behavior while shaving overhead in the new linear scan.♻️ Suggested refactor
- 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; - }(); + const auto dmn = [&]() -> const std::shared_ptr<MasternodeEntry> { + for (const auto& mn : data_mn->m_entries) { + if (mn->proTxHashRaw() == proTxHash) { + return mn->isBanned() ? nullptr : mn; + } + } + return nullptr; + }(); + const QString protx_hash{QString::fromStdString(proTxHash.ToString())};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/proposallist.cpp` around lines 535 - 543, The current scan converts proTxHash to QString (protx_hash) and compares with mn->proTxHash(), which causes per-entry string allocations; instead compare the raw uint256 values directly by using proTxHash against mn->proTxHashRaw() in the lambda that builds dmn over data_mn->m_entries and keep the isBanned() check unchanged (return nullptr when banned). Replace the string comparison (mn->proTxHash() == protx_hash) with a binary comparison (mn->proTxHashRaw() == proTxHash) to avoid QString conversion overhead.
🤖 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 555-558: The clang-format failure is due to the single-line
chained call to failedMessages.append with tr(...).arg(...); reformat the block
around walletModel->wallet().signGovernanceVote so the append call is split and
aligned: call failedMessages.append( on one line, put tr("Failed to sign vote
for masternode %1").arg(protx_hash) on the next indented line, then close the
append call with a closing parenthesis and semicolon on its own line; keep the
surrounding nFailed++ and the signGovernanceVote check
(walletModel->wallet().signGovernanceVote, nFailed, failedMessages.append,
tr(...).arg(protx_hash)) unchanged.
---
Nitpick comments:
In `@src/qt/proposallist.cpp`:
- Around line 535-543: The current scan converts proTxHash to QString
(protx_hash) and compares with mn->proTxHash(), which causes per-entry string
allocations; instead compare the raw uint256 values directly by using proTxHash
against mn->proTxHashRaw() in the lambda that builds dmn over data_mn->m_entries
and keep the isBanned() check unchanged (return nullptr when banned). Replace
the string comparison (mn->proTxHash() == protx_hash) with a binary comparison
(mn->proTxHashRaw() == proTxHash) to avoid QString conversion overhead.
| if (!walletModel->wallet().signGovernanceVote(votingKeyID, vote)) { | ||
| nFailed++; | ||
| failedMessages.append( | ||
| tr("Failed to sign vote for masternode %1").arg(QString::fromStdString(proTxHash.ToString()))); | ||
| tr("Failed to sign vote for masternode %1").arg(protx_hash)); |
There was a problem hiding this comment.
Fix clang-format failure in this block (CI is red).
The pipeline reports a clang-format diff here. Align the call to the formatter output.
🧹 Formatting fix
- failedMessages.append(
- tr("Failed to sign vote for masternode %1").arg(protx_hash));
+ failedMessages.append(tr("Failed to sign vote for masternode %1").arg(protx_hash));📝 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.
| if (!walletModel->wallet().signGovernanceVote(votingKeyID, vote)) { | |
| nFailed++; | |
| failedMessages.append( | |
| tr("Failed to sign vote for masternode %1").arg(QString::fromStdString(proTxHash.ToString()))); | |
| tr("Failed to sign vote for masternode %1").arg(protx_hash)); | |
| if (!walletModel->wallet().signGovernanceVote(votingKeyID, vote)) { | |
| nFailed++; | |
| failedMessages.append(tr("Failed to sign vote for masternode %1").arg(protx_hash)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/qt/proposallist.cpp` around lines 555 - 558, The clang-format failure is
due to the single-line chained call to failedMessages.append with
tr(...).arg(...); reformat the block around
walletModel->wallet().signGovernanceVote so the append call is split and
aligned: call failedMessages.append( on one line, put tr("Failed to sign vote
for masternode %1").arg(protx_hash) on the next indented line, then close the
append call with a closing parenthesis and semicolon on its own line; keep the
surrounding nFailed++ and the signGovernanceVote check
(walletModel->wallet().signGovernanceVote, nFailed, failedMessages.append,
tr(...).arg(protx_hash)) unchanged.
3 participants
Issue being fixed or feature implemented
Currently, for each update of masternode list 3 list are generated: list of shared_pointers, hash-map by service, hash-map by protx.
Creating these hash-maps creates overhead for every user for every masternode update. It causes excessive battery use, excessive RAM usage while these lookups happens in rare conditions (only for masternode owners during voting; only when "Peer Detail" is open).
It's better to do linear lookup once over all masternode in the specific scenario, rather than build these heavy objects for every update of list.
Lookup during voting may looks scary due to potential O(N^2). Though, it's not N^2, but N*M (where M is amount of masternodes that belongs to single Owner, M<<N). Secondly, N is relatively small and voting is not assumed to be rapid-fast action (milliseconds); it's okay if user will wait extra 10ms in the worst case scenario for each its vote.
What was done?
Code is updated to do linear lookup instead building heavy hash-maps for every update.
Current implementation (in develop) has a bug with dangling pointer and potentially could cause UB: #7118 (comment)
Fixed by this PR
How Has This Been Tested?
N/A
Breaking Changes
N/A
Checklist: