feat(interfaces): consolidate masternode counts into one struct, expose chainlock, instantsend, credit pool, quorum statistics#7160
Conversation
This is a preparatory commit needed to switch existing masternode list code to the newly created masternode feed.
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
| m_hash_collateral{QString::fromStdString(govObj.GetCollateralHash().ToString())}, | ||
| m_hash_object{QString::fromStdString(govObj.GetHash().ToString())}, | ||
| m_hash_parent{QString::fromStdString(govObj.Object().hashParent.ToString())}, | ||
| m_json{QString::fromStdString(govObj.GetInnerJson().write(2))}, |
There was a problem hiding this comment.
@PastaPastaPasta, the write(2) method is called on a UniValue object (returned by GetInnerJson()). The write method serializes the UniValue object to a JSON string, and the parameter 2 specifies the indentation level for pretty-printing.
So write(2) means: format the JSON output with 2-space indentation, making it more human-readable. If you used write(0) or write(), it would produce compact JSON without extra whitespace.
|
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 replaces many granular masternode and instant-send count getters with aggregated Counts structs and GetCounts()/getCounts() accessors across CDeterministicMNList, CInstantSendManager, interfaces::MnList, and related callers. MnEntry ownership changes from unique_ptr to shared_ptr, and forEach/get/projected-payees callbacks/signatures are updated to use the new pointer type. The LLMQ/node interfaces gain new state-probe methods (chainlock, credit-pool counts, instant-send counts, pending asset unlocks, quorum stats). CChainstateHelper adds a GetCreditPool passthrough and call sites now use it. Call sites, logging, RPCs, and Qt UI code were updated to consume the new accessors and types. Sequence Diagram(s)sequenceDiagram
participant UI as UI/Qt
participant NodeAPI as interfaces::Node
participant MnList as CDeterministicMNList
participant LLMQ as LLMQ/InstantSendMgr
participant ChainHelper as CChainstateHelper
UI->>NodeAPI: request masternode list / stats
NodeAPI->>MnList: getCounts()
MnList-->>NodeAPI: Counts { total, enabled, m_valid_weighted, ... }
NodeAPI-->>UI: masternode data + Counts
UI->>NodeAPI: request instant-send stats
NodeAPI->>LLMQ: getInstantSendCounts()
LLMQ-->>NodeAPI: InstantSendCounts { m_verified, m_unverified, ... }
NodeAPI-->>UI: instant-send stats
UI->>NodeAPI: request credit-pool info
NodeAPI->>ChainHelper: GetCreditPool(pindex)
ChainHelper-->>NodeAPI: CCreditPool
NodeAPI-->>UI: credit-pool data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/qt/masternodemodel.cpp (1)
28-38:⚠️ Potential issue | 🟡 MinorFix clang-format diff reported by CI at Line 38.
The Clang Diff Format Check reports formatting differences in this file; please run clang-format and update the patch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/qt/masternodemodel.cpp` around lines 28 - 38, Clang-format flagged formatting issues in the JoinArray function; run clang-format on src/qt/masternodemodel.cpp (or apply your project's clang-format style) and update the patch so JoinArray's spacing/line breaks match the formatter (ensure consistent brace placement, spacing around operators, and the final return line formatting for "return list.isEmpty() ? std::nullopt : std::optional<QString>(list.join(\", \"));" ), then stage the reformatted file and update the PR.
🧹 Nitpick comments (2)
src/evo/deterministicmns.h (1)
267-289: AddGetTotalCount()to avoid unnecessary iteration when only total count is needed.
GetCounts()iterates the entiremnMap(O(N)) butCalcMaxPoSePenalty()andDecreaseScores()only needtotal(), which equalsmnMap.size()— available in O(1). A simple[[nodiscard]] size_t GetTotalCount() const { return mnMap.size(); }would eliminate the scan for these call sites. At current mainnet MN counts this has negligible impact, but it's a cleaner design.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/evo/deterministicmns.h` around lines 267 - 289, Add a new [[nodiscard]] size_t GetTotalCount() const { return mnMap.size(); } method to provide O(1) access to the total masternode count; update call sites that only need the total (e.g., CalcMaxPoSePenalty and DecreaseScores) to use GetTotalCount() instead of calling GetCounts() to avoid iterating mnMap, leaving GetCounts() unchanged for callers that need the full breakdown; locate mnMap and GetCounts() in deterministicmns.h to add the new accessor near them.src/interfaces/node.h (1)
186-215: New LLMQ data structures provide a well-scoped interface for UI consumption.
ChainLockInfo,CreditPoolCounts,InstantSendCounts, andQuorumInfoeach expose only the data needed by downstream consumers (feeds/UI) without leaking internal LLMQ implementation details. Verified no external consumers depend on any removed method—the only usage found is insrc/qt/clientmodel.cpp, which correctly uses the newgetInstantSendCounts()to access them_verifiedcount.One note:
QuorumInfo::m_healthasdoubleis reasonable, but it lacks a doc comment explaining the expected range/semantics for UI consumers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/interfaces/node.h` around lines 186 - 215, QuorumInfo::m_health lacks documentation about its expected range and semantics for UI consumers; add a clear doc comment above the QuorumInfo struct (or directly above m_health) describing what m_health represents (e.g., fraction/percentage), its numeric range (e.g., 0.0–1.0 or 0–100), units, and how to interpret edge values and NaN/unset cases so consumers using getQuorumStats() know how to display it consistently.
🤖 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/evo/deterministicmns.cpp`:
- Around line 711-714: The two weighted gauges currently both emit
counts.m_valid_weighted_count; update the ::g_stats_client->gauge call for
"masternodes.weighted_count" to emit the intended value (either counts.total()
if you meant the unfiltered total analogous to "masternodes.count", or keep
counts.m_valid_weighted_count but add a clarifying comment that
"masternodes.weighted_count" is a legacy alias for the valid weighted count).
Locate the calls to ::g_stats_client->gauge in deterministicmns.cpp (the lines
using "masternodes.weighted_count" and "masternodes.weighted_enabled") and
either change the first to counts.total() or add a single-line comment above it
explaining why both weighted metrics use counts.m_valid_weighted_count.
- Around line 693-694: Fix the clang-format indentation on the LogPrintf
continuation line: in the LogPrintf call inside deterministicmns.cpp (the one
using __func__, nHeight, newList.GetCounts().total()), align the second line so
its arguments start under the column immediately after the opening parenthesis
of LogPrintf( (i.e., indent the continuation line to match the first argument
column) to satisfy the project's clang-format rules.
In `@src/qt/masternodelist.cpp`:
- Around line 226-229: Reformat the affected block in masternodelist.cpp to
satisfy clang-format: run clang-format on the region around the call to
clientModel->getMasternodeList() and the subsequent lambda passed to
dmn->forEachMN (the lines referencing getMasternodeList, forEachMN, setOutpts
and the fMyMasternode assignment) and re-commit the updated formatting so the
clang diff check passes.
In `@src/rpc/governance.cpp`:
- Around line 984-987: The long fundingthreshold pushKV line violates
clang-format; break the expression into a short intermediate variable and then
call pushKV—e.g., store
CHECK_NONFATAL(node.dmnman)->GetListAtChainTip().GetCounts() in a local (e.g.,
auto counts = ...GetCounts()) and then use obj.pushKV("fundingthreshold",
counts.m_valid_weighted_count / 10); this keeps the call chain short and
satisfies the formatter while referencing CHECK_NONFATAL, node.dmnman,
GetListAtChainTip, GetCounts, and m_valid_weighted_count.
---
Outside diff comments:
In `@src/qt/masternodemodel.cpp`:
- Around line 28-38: Clang-format flagged formatting issues in the JoinArray
function; run clang-format on src/qt/masternodemodel.cpp (or apply your
project's clang-format style) and update the patch so JoinArray's spacing/line
breaks match the formatter (ensure consistent brace placement, spacing around
operators, and the final return line formatting for "return list.isEmpty() ?
std::nullopt : std::optional<QString>(list.join(\", \"));" ), then stage the
reformatted file and update the PR.
---
Nitpick comments:
In `@src/evo/deterministicmns.h`:
- Around line 267-289: Add a new [[nodiscard]] size_t GetTotalCount() const {
return mnMap.size(); } method to provide O(1) access to the total masternode
count; update call sites that only need the total (e.g., CalcMaxPoSePenalty and
DecreaseScores) to use GetTotalCount() instead of calling GetCounts() to avoid
iterating mnMap, leaving GetCounts() unchanged for callers that need the full
breakdown; locate mnMap and GetCounts() in deterministicmns.h to add the new
accessor near them.
In `@src/interfaces/node.h`:
- Around line 186-215: QuorumInfo::m_health lacks documentation about its
expected range and semantics for UI consumers; add a clear doc comment above the
QuorumInfo struct (or directly above m_health) describing what m_health
represents (e.g., fraction/percentage), its numeric range (e.g., 0.0–1.0 or
0–100), units, and how to interpret edge values and NaN/unset cases so consumers
using getQuorumStats() know how to display it consistently.
| LogPrintf("CDeterministicMNManager::%s -- Wrote snapshot. nHeight=%d, mapCurMNs.allMNsCount=%d\n", | ||
| __func__, nHeight, newList.GetAllMNsCount()); | ||
| __func__, nHeight, newList.GetCounts().total()); |
There was a problem hiding this comment.
CI-blocking clang-format failure — fix indentation on the continuation line.
The pipeline reports formatting differences at this change. The continuation argument list in the LogPrintf call likely needs to be aligned with the first argument (column after the opening () per the project's clang-format rules.
🔧 Proposed formatting fix
- LogPrintf("CDeterministicMNManager::%s -- Wrote snapshot. nHeight=%d, mapCurMNs.allMNsCount=%d\n",
- __func__, nHeight, newList.GetCounts().total());
+ LogPrintf("CDeterministicMNManager::%s -- Wrote snapshot. nHeight=%d, mapCurMNs.allMNsCount=%d\n",
+ __func__, nHeight, newList.GetCounts().total());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/evo/deterministicmns.cpp` around lines 693 - 694, Fix the clang-format
indentation on the LogPrintf continuation line: in the LogPrintf call inside
deterministicmns.cpp (the one using __func__, nHeight,
newList.GetCounts().total()), align the second line so its arguments start under
the column immediately after the opening parenthesis of LogPrintf( (i.e., indent
the continuation line to match the first argument column) to satisfy the
project's clang-format rules.
| const auto [dmn, pindex] = clientModel->getMasternodeList(); | ||
| if (dmn && pindex) { | ||
| dmn->forEachMN(/*only_valid=*/false, [&](const auto& dmn) { | ||
| bool fMyMasternode = setOutpts.count(dmn.getCollateralOutpoint()) || | ||
| walletModel->wallet().isSpendable(PKHash(dmn.getKeyIdOwner())) || | ||
| walletModel->wallet().isSpendable(PKHash(dmn.getKeyIdVoting())) || | ||
| walletModel->wallet().isSpendable(dmn.getScriptPayout()) || | ||
| walletModel->wallet().isSpendable(dmn.getScriptOperatorPayout()); | ||
| bool fMyMasternode = setOutpts.count(dmn->getCollateralOutpoint()) || |
There was a problem hiding this comment.
Fix clang-format diff reported by CI at Line 227.
The Clang Diff Format Check is failing for this file; please re-run clang-format on the affected block and re-commit.
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 227-227: Clang-format differences detected. Apply formatting changes as shown in diff_output.txt. Command failed: git diff -U0 origin/develop --
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/qt/masternodelist.cpp` around lines 226 - 229, Reformat the affected
block in masternodelist.cpp to satisfy clang-format: run clang-format on the
region around the call to clientModel->getMasternodeList() and the subsequent
lambda passed to dmn->forEachMN (the lines referencing getMasternodeList,
forEachMN, setOutpts and the fMyMasternode assignment) and re-commit the updated
formatting so the clang diff check passes.
| obj.pushKV("lastsuperblock", nLastSuperblock); | ||
| obj.pushKV("nextsuperblock", nNextSuperblock); | ||
| obj.pushKV("fundingthreshold", CHECK_NONFATAL(node.dmnman)->GetListAtChainTip().GetValidWeightedMNsCount() / 10); | ||
| obj.pushKV("fundingthreshold", CHECK_NONFATAL(node.dmnman)->GetListAtChainTip().GetCounts().m_valid_weighted_count / 10); | ||
| obj.pushKV("governancebudget", ValueFromAmount(CSuperblock::GetPaymentsLimit(chainman.ActiveChain(), nNextSuperblock))); |
There was a problem hiding this comment.
Fix clang-format violation on the fundingthreshold line.
CI reports a clang-format diff here; wrap the long line to satisfy the formatter.
🧹 Suggested formatting fix
- obj.pushKV("fundingthreshold", CHECK_NONFATAL(node.dmnman)->GetListAtChainTip().GetCounts().m_valid_weighted_count / 10);
+ obj.pushKV("fundingthreshold",
+ CHECK_NONFATAL(node.dmnman)->GetListAtChainTip().GetCounts().m_valid_weighted_count / 10);📝 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.
| obj.pushKV("lastsuperblock", nLastSuperblock); | |
| obj.pushKV("nextsuperblock", nNextSuperblock); | |
| obj.pushKV("fundingthreshold", CHECK_NONFATAL(node.dmnman)->GetListAtChainTip().GetValidWeightedMNsCount() / 10); | |
| obj.pushKV("fundingthreshold", CHECK_NONFATAL(node.dmnman)->GetListAtChainTip().GetCounts().m_valid_weighted_count / 10); | |
| obj.pushKV("governancebudget", ValueFromAmount(CSuperblock::GetPaymentsLimit(chainman.ActiveChain(), nNextSuperblock))); | |
| obj.pushKV("lastsuperblock", nLastSuperblock); | |
| obj.pushKV("nextsuperblock", nNextSuperblock); | |
| obj.pushKV("fundingthreshold", | |
| CHECK_NONFATAL(node.dmnman)->GetListAtChainTip().GetCounts().m_valid_weighted_count / 10); | |
| obj.pushKV("governancebudget", ValueFromAmount(CSuperblock::GetPaymentsLimit(chainman.ActiveChain(), nNextSuperblock))); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/rpc/governance.cpp` around lines 984 - 987, The long fundingthreshold
pushKV line violates clang-format; break the expression into a short
intermediate variable and then call pushKV—e.g., store
CHECK_NONFATAL(node.dmnman)->GetListAtChainTip().GetCounts() in a local (e.g.,
auto counts = ...GetCounts()) and then use obj.pushKV("fundingthreshold",
counts.m_valid_weighted_count / 10); this keeps the call chain short and
satisfies the formatter while referencing CHECK_NONFATAL, node.dmnman,
GetListAtChainTip, GetCounts, and m_valid_weighted_count.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/evo/chainhelper.h (1)
73-74:GetCreditPoolshould beconst-qualified if feasible.All other passthrough methods in this class (
HasConflictingChainLock,HasChainLock,GetBestChainLockHeight) areconst.GetCreditPoolis non-const becauseCCreditPoolManager::GetCreditPoolmutates internal cache. This is fine as-is, but worth noting the inconsistency is driven by the underlying API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/evo/chainhelper.h` around lines 73 - 74, GetCreditPool is declared non-const while other passthroughs (HasConflictingChainLock, HasChainLock, GetBestChainLockHeight) are const; if CCreditPoolManager::GetCreditPool can be made callable from a const context, change CCreditPool GetCreditPool(const CBlockIndex* const pindex) to a const member to match the others; if CCreditPoolManager::GetCreditPool currently mutates an internal cache, either add a const overload or make the cache mutable (or provide a const accessor that returns the cached value) so CCreditPoolManager::GetCreditPool can be invoked from a const method and then mark the wrapper GetCreditPool as const to preserve logical constness and API consistency.src/interfaces/node.h (2)
102-112:Countsstruct looks good; consider a[[nodiscard]]ongetCounts()itselfThe struct design is clean.
total_weighted()is intentionally absent (weighted counts accessed directly). One small improvement: thegetCounts()pure-virtual method at line 113 could also carry[[nodiscard]]to propagate the hint to all override call sites.💡 Proposed addition
- virtual Counts getCounts() const = 0; + [[nodiscard]] virtual Counts getCounts() const = 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/interfaces/node.h` around lines 102 - 112, Add the [[nodiscard]] attribute to the pure-virtual getCounts() method so callers and overrides receive the compiler hint to use the returned Counts; locate the declaration of getCounts() (the method that returns Counts) and annotate it with [[nodiscard]] to propagate the warning to all overrides and call sites, leaving the Counts struct unchanged.
193-197:CreditPoolCountsname is a misnomer — fields areCAmountmonetary values, not counts
m_diff,m_limit, andm_lockedare allCAmount(int64_t, denominated in satoshis). Calling the structCreditPoolCountsimplies integer counts similar toInstantSendCounts, which could confuse future readers. ConsiderCreditPoolAmountsorCreditPoolInfo.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/interfaces/node.h` around lines 193 - 197, The struct name CreditPoolCounts is misleading because its members m_diff, m_limit, and m_locked are CAmount monetary values; rename the struct to a clearer name (e.g., CreditPoolAmounts or CreditPoolInfo) and update all references/usages accordingly (declarations, function signatures, serialization/deserialization, and any documentation) while keeping the existing member names and types (m_diff, m_limit, m_locked : CAmount) unchanged to preserve ABI/behavior.src/node/interfaces.cpp (2)
465-472: Prefer designated initializers forInstantSendCountsconstructionThe positional brace-init
{counts.m_verified, counts.m_unverified, counts.m_awaiting_tx, counts.m_unprotected_tx}silently maps by field position. IfInstantSendCountsstruct members are ever reordered, this will silently produce wrong values without a compile error.♻️ Proposed refactor
- return {counts.m_verified, counts.m_unverified, counts.m_awaiting_tx, counts.m_unprotected_tx}; + return { + .m_verified = counts.m_verified, + .m_unverified = counts.m_unverified, + .m_awaiting_tx = counts.m_awaiting_tx, + .m_unprotected_tx = counts.m_unprotected_tx, + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/node/interfaces.cpp` around lines 465 - 472, In getInstantSendCounts(), replace the positional brace-init of InstantSendCounts with a designated-initializer form so fields are assigned by name (e.g., InstantSendCounts{ .m_verified = counts.m_verified, .m_unverified = counts.m_unverified, .m_awaiting_tx = counts.m_awaiting_tx, .m_unprotected_tx = counts.m_unprotected_tx } using the exact member names of InstantSendCounts) to avoid silent field-mapping errors if the struct is reordered; locate this change in the getInstantSendCounts() method where counts is fetched from context().llmq_ctx->isman->GetCounts() and keep the existing return {} fallback unchanged.
218-227:getListAtChainTip— verify copy path formake_shared<MnListImpl>(mnList)
mnListis a localMnListImplvalue.make_shared<MnListImpl>(mnList)invokes the compiler-generated copy constructor (since no user-provided copy ctor exists), which copies the heldCDeterministicMNListby value. With Immer-backed immutable lists this is cheap (structural sharing), but it is still an extra allocation + copy that could be avoided by constructing in-place:💡 Optional: avoid the intermediate local and the extra copy
- MnListImpl mnList = context().dmnman->GetListForBlock(tip); - if (!mnList.getBlockHash().IsNull()) { - mnList.setContext(m_context); - return {std::make_shared<MnListImpl>(mnList), tip}; - } + auto mnListPtr = std::make_shared<MnListImpl>(context().dmnman->GetListForBlock(tip)); + if (!mnListPtr->getBlockHash().IsNull()) { + mnListPtr->setContext(m_context); + return {mnListPtr, tip}; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/node/interfaces.cpp` around lines 218 - 227, The code currently creates a local MnListImpl (mnList) and then does std::make_shared<MnListImpl>(mnList), which forces an extra copy; instead construct the shared object in-place from the GetListForBlock result to avoid the intermediate copy — e.g. call std::make_shared<MnListImpl>(context().dmnman->GetListForBlock(tip)) (or std::make_shared<MnListImpl>(std::move(mnList)) if you keep the local and want to enable move) within getListAtChainTip so the MnListImpl is constructed directly into the shared_ptr (referencing MnListImpl, getListAtChainTip, GetListForBlock, and the std::make_shared<MnListImpl> call).
🤖 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/node/interfaces.cpp`:
- Line 311: The assignment to GovernanceInfo::fundingthreshold narrows size_t to
int (info.fundingthreshold =
context().dmnman->GetListAtChainTip().GetCounts().m_valid_weighted / 10) and can
trigger -Wconversion or overflow; change the assignment to safely clamp and cast
the value: compute size_t val =
context().dmnman->GetListAtChainTip().GetCounts().m_valid_weighted / 10, then
set info.fundingthreshold = static_cast<int>(std::min<size_t>(val,
static_cast<size_t>(INT_MAX))); (ensure <algorithm> and <climits> are included)
so the conversion is explicit and bounded.
- Around line 473-482: getPendingAssetUnlocks returns size_t but uses
std::ranges::count_if which yields a signed range_difference_t; avoid implicit
signed-to-unsigned narrowing by explicitly casting the result to size_t. In the
getPendingAssetUnlocks method (using context().mempool,
LOCK(context().mempool->cs) and ranges::count_if over context().mempool->mapTx)
wrap the count_if result with static_cast<size_t>(...) (or assign to a ptrdiff_t
and then cast) before returning so the intent is explicit and -Wconversion is
satisfied.
- Around line 497-505: newest_height can be zero when no quorum/base block
exists which makes expiry_height compute to a misleading positive value; change
the expiry_height computation to be a sentinel (e.g. -1) when newest_height ==
0: compute expiry_height as (newest_height > 0) ? newest_height +
llmq_params->signingActiveQuorumCount * llmq_params->dkgInterval : -1, and use
that value when constructing the QuorumInfo; also avoid positional fragility of
the aggregate initialization by switching the QuorumInfo creation to either a
constructor or C++20 designated initializers (e.g. explicitly set m_name,
m_count, m_health, m_use_rotation, m_max_store_depth, m_newest_height,
m_expiry_height) so fields are not order-dependent.
---
Duplicate comments:
In `@src/evo/deterministicmns.cpp`:
- Around line 691-694: The LogPrintf continuation line in
CDeterministicMNManager (after calls to m_evoDb.Write and mnListsCache.emplace)
is misaligned and failing clang-format; reformat the LogPrintf call so its
continuation string and arguments are aligned per project style (e.g., break
before the format string or align the closing parenthesis with the call), then
run the project's formatting script or clang-format to ensure the LogPrintf
invocation and its continuation line conform to the repository's formatting
rules.
In `@src/rpc/governance.cpp`:
- Line 986: The long chained expression used in pushKV should be broken into a
descriptive intermediate variable to satisfy clang-format and improve
readability: retrieve the masternode list via
CHECK_NONFATAL(node.dmnman)->GetListAtChainTip(), call GetCounts() on that list,
store m_valid_weighted (or the divided-by-10 result) into a local variable
(e.g., valid_weighted or funding_threshold) and then pass that variable to
obj.pushKV("fundingthreshold", ...); update the code around the pushKV call to
use these intermediate symbols (CHECK_NONFATAL, GetListAtChainTip, GetCounts,
m_valid_weighted) accordingly.
---
Nitpick comments:
In `@src/evo/chainhelper.h`:
- Around line 73-74: GetCreditPool is declared non-const while other
passthroughs (HasConflictingChainLock, HasChainLock, GetBestChainLockHeight) are
const; if CCreditPoolManager::GetCreditPool can be made callable from a const
context, change CCreditPool GetCreditPool(const CBlockIndex* const pindex) to a
const member to match the others; if CCreditPoolManager::GetCreditPool currently
mutates an internal cache, either add a const overload or make the cache mutable
(or provide a const accessor that returns the cached value) so
CCreditPoolManager::GetCreditPool can be invoked from a const method and then
mark the wrapper GetCreditPool as const to preserve logical constness and API
consistency.
In `@src/interfaces/node.h`:
- Around line 102-112: Add the [[nodiscard]] attribute to the pure-virtual
getCounts() method so callers and overrides receive the compiler hint to use the
returned Counts; locate the declaration of getCounts() (the method that returns
Counts) and annotate it with [[nodiscard]] to propagate the warning to all
overrides and call sites, leaving the Counts struct unchanged.
- Around line 193-197: The struct name CreditPoolCounts is misleading because
its members m_diff, m_limit, and m_locked are CAmount monetary values; rename
the struct to a clearer name (e.g., CreditPoolAmounts or CreditPoolInfo) and
update all references/usages accordingly (declarations, function signatures,
serialization/deserialization, and any documentation) while keeping the existing
member names and types (m_diff, m_limit, m_locked : CAmount) unchanged to
preserve ABI/behavior.
In `@src/node/interfaces.cpp`:
- Around line 465-472: In getInstantSendCounts(), replace the positional
brace-init of InstantSendCounts with a designated-initializer form so fields are
assigned by name (e.g., InstantSendCounts{ .m_verified = counts.m_verified,
.m_unverified = counts.m_unverified, .m_awaiting_tx = counts.m_awaiting_tx,
.m_unprotected_tx = counts.m_unprotected_tx } using the exact member names of
InstantSendCounts) to avoid silent field-mapping errors if the struct is
reordered; locate this change in the getInstantSendCounts() method where counts
is fetched from context().llmq_ctx->isman->GetCounts() and keep the existing
return {} fallback unchanged.
- Around line 218-227: The code currently creates a local MnListImpl (mnList)
and then does std::make_shared<MnListImpl>(mnList), which forces an extra copy;
instead construct the shared object in-place from the GetListForBlock result to
avoid the intermediate copy — e.g. call
std::make_shared<MnListImpl>(context().dmnman->GetListForBlock(tip)) (or
std::make_shared<MnListImpl>(std::move(mnList)) if you keep the local and want
to enable move) within getListAtChainTip so the MnListImpl is constructed
directly into the shared_ptr (referencing MnListImpl, getListAtChainTip,
GetListForBlock, and the std::make_shared<MnListImpl> call).
All information can be computed at once and selectively discarded so we can avoid potential drift between list changes by making sure direct calls still guarantee that all counts are from the same snapshot
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/interfaces/node.h (1)
187-216: Nit: inconsistent block-height types across the new structs.
ChainLockInfo::m_heightisint32_t(Line 188), whileQuorumInfo::m_newest_heightandm_expiry_height(Lines 213–214) areint. On all target platforms these are the same width, so there's no functional issue, but using a consistent type across this interface surface would be cleaner.Not blocking — just a consistency nit for the public API.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/interfaces/node.h` around lines 187 - 216, The public API uses mixed integer types for block heights; make them consistent by switching QuorumInfo::m_newest_height and QuorumInfo::m_expiry_height from int to int32_t (matching ChainLockInfo::m_height), and update any related uses/signatures that assume those fields to use int32_t as well so the interface types are uniform (refer to ChainLockInfo::m_height and QuorumInfo::m_newest_height/m_expiry_height when locating the fields to change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/evo/deterministicmns.cpp`:
- Around line 693-694: Clang-format wants the LogPrintf continuation aligned
with the first argument column; in CDeterministicMNManager (the LogPrintf call
using __func__, nHeight, newList.GetCounts().total()) re-indent the second line
so its start lines up under the first argument of LogPrintf (i.e., align the
continuation with the column of "__func__") rather than the current indentation;
update that single LogPrintf invocation formatting accordingly.
---
Nitpick comments:
In `@src/interfaces/node.h`:
- Around line 187-216: The public API uses mixed integer types for block
heights; make them consistent by switching QuorumInfo::m_newest_height and
QuorumInfo::m_expiry_height from int to int32_t (matching
ChainLockInfo::m_height), and update any related uses/signatures that assume
those fields to use int32_t as well so the interface types are uniform (refer to
ChainLockInfo::m_height and QuorumInfo::m_newest_height/m_expiry_height when
locating the fields to change).
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/interfaces/node.h (1)
187-217: LGTM — new LLMQ structs are well-designed.One documentation note:
ChainLockInfo::m_heightdefaults to0, making it the sentinel for "no best chainlock". Consider a brief comment on this field so consumers know to treatm_height == 0(orm_hash.IsNull()) as the "no chainlock" indicator, since genesis height is also 0.📝 Proposed documentation improvement
struct ChainLockInfo { - int32_t m_height{0}; + int32_t m_height{0}; //!< 0 when no best chainlock is available int64_t m_block_time{0}; uint256 m_hash{}; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/interfaces/node.h` around lines 187 - 217, Add an inline comment to the ChainLockInfo struct clarifying that m_height == 0 is used as the sentinel meaning "no best chainlock" (and that callers may also check m_hash.IsNull()) so consumers won't confuse it with genesis height; update the comment near the m_height and/or m_hash members inside ChainLockInfo to state this explicitly and mention the intended sentinel semantics.src/node/interfaces.cpp (2)
448-452: Consider caching thepindex->pprevcredit pool to improve readability
GetCreditPool(pindex->pprev)is called inline for a single field assignment. Naming the result makes the diff computation self-documenting and avoids the second call in caseGetCreditPoolever becomes non-trivial.♻️ Proposed refactor
auto& chain_helper{context().chainman->ActiveChainstate().ChainHelper()}; const auto pool{chain_helper.GetCreditPool(pindex)}; + const auto prev_pool{chain_helper.GetCreditPool(pindex->pprev)}; ret.m_locked = pool.locked; ret.m_limit = pool.currentLimit; - ret.m_diff = pool.locked - chain_helper.GetCreditPool(pindex->pprev).locked; + ret.m_diff = pool.locked - prev_pool.locked;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/node/interfaces.cpp` around lines 448 - 452, Cache the previous-block credit pool before computing the diff to avoid the extra GetCreditPool call and make the calculation self-documenting: call chain_helper.GetCreditPool(pindex->pprev) once, store it in a descriptive variable (e.g., prev_pool or prevPool), then set ret.m_diff = pool.locked - prev_pool.locked; leave the existing assignments to ret.m_locked and ret.m_limit using the already-obtained pool.
509-514: Defensive guard forq->qcingetQuorumStats()
CQuorumrepresents "a quorum which was mined on-chain (through a quorum commitment)", soq->qcshould always be non-null for results fromScanQuorums. However, no null check is present beforeq->qc->CountValidMembers()at line 512.This pattern is consistent across the entire codebase: all
ScanQuorumsconsumers insrc/llmq/signing.cpp,src/rpc/quorums.cpp,src/evo/assetlocktx.cpp, and elsewhere accessq->qcdirectly without defensive checks. If the invariant is ever violated (e.g., partially-constructed quorum in a test/edge scenario), this crashes. Adding a guard here would require similar changes across the codebase for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/node/interfaces.cpp` around lines 509 - 514, The loop in getQuorumStats() uses q->qc->CountValidMembers() without guarding q->qc; add a defensive null-check before calling CountValidMembers() (e.g., if q->qc is null, treat numValidMembers as 0 or skip this quorum) to avoid dereferencing a null pointer when iterating over quorums; locate the loop over quorums (variable quorums and elements q) and update the calculation of numValidMembers to only call q->qc->CountValidMembers() when q->qc is non-null, preserving existing health accumulation semantics otherwise.src/instantsend/instantsend.h (1)
187-195:GetInstantSendLockCount()is redundant withGetCounts().m_verifiedBoth methods delegate to
db.GetInstantSendLockCount(). Only two call sites exist (RPC insrc/rpc/mempool.cpp:696and stats insrc/init.cpp:920), making it feasible to consolidate the public API around the newGetCounts()accessor in a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/instantsend/instantsend.h` around lines 187 - 195, GetInstantSendLockCount() is redundant with GetCounts().m_verified because both forward to db.GetInstantSendLockCount(); remove the GetInstantSendLockCount() declaration and replace its two call sites to use GetCounts().m_verified instead (RPC at the mempool RPC and the stats/init code), preserving the same lock/annotation expectations (EXCLUSIVE_LOCKS_REQUIRED on GetCounts) and ensuring any callers include the required locks or annotations when reading m_verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/instantsend/instantsend.h`:
- Around line 187-195: GetInstantSendLockCount() is redundant with
GetCounts().m_verified because both forward to db.GetInstantSendLockCount();
remove the GetInstantSendLockCount() declaration and replace its two call sites
to use GetCounts().m_verified instead (RPC at the mempool RPC and the stats/init
code), preserving the same lock/annotation expectations
(EXCLUSIVE_LOCKS_REQUIRED on GetCounts) and ensuring any callers include the
required locks or annotations when reading m_verified.
In `@src/interfaces/node.h`:
- Around line 187-217: Add an inline comment to the ChainLockInfo struct
clarifying that m_height == 0 is used as the sentinel meaning "no best
chainlock" (and that callers may also check m_hash.IsNull()) so consumers won't
confuse it with genesis height; update the comment near the m_height and/or
m_hash members inside ChainLockInfo to state this explicitly and mention the
intended sentinel semantics.
In `@src/node/interfaces.cpp`:
- Around line 448-452: Cache the previous-block credit pool before computing the
diff to avoid the extra GetCreditPool call and make the calculation
self-documenting: call chain_helper.GetCreditPool(pindex->pprev) once, store it
in a descriptive variable (e.g., prev_pool or prevPool), then set ret.m_diff =
pool.locked - prev_pool.locked; leave the existing assignments to ret.m_locked
and ret.m_limit using the already-obtained pool.
- Around line 509-514: The loop in getQuorumStats() uses
q->qc->CountValidMembers() without guarding q->qc; add a defensive null-check
before calling CountValidMembers() (e.g., if q->qc is null, treat
numValidMembers as 0 or skip this quorum) to avoid dereferencing a null pointer
when iterating over quorums; locate the loop over quorums (variable quorums and
elements q) and update the calculation of numValidMembers to only call
q->qc->CountValidMembers() when q->qc is non-null, preserving existing health
accumulation semantics otherwise.
3 participants
Additional Information
Spun off from dash#7118 for ease of review, exposes new interface methods for usage in feeds as part of UI refresh.
Breaking Changes
None expected.
Checklist