Repository navigation
Conversation
BuildDiff walked the whole new list and looked every masternode up in the old one to find the few entries a block changes, and walked the old list once more whenever something was removed: two hash lookups per masternode, ~4000 per block, for a handful of changes. The new list is the old one plus those changes and the two share all other nodes, so immer::diff visits only the subtrees that differ. The added, removed and updated sets are the same; the early exit of the removal walk is no longer needed.
…do its job Both took the callback as std::function, so every per-block walk paid a type-erased call per masternode and the lambda could not be inlined into the loop. Take the callable as a template parameter.
ProcessSpecialTxsInBlock fetched the list at the previous block for the payment check, BuildNewListFromBlock fetched it again to rebuild from, and CDeterministicMNManager::ProcessBlock a third time to build the diff. Each fetch copies the list and runs the cache bookkeeping. Pass the one already fetched into the rebuild and into ProcessBlock.
FinishListForBlock selected the payee from the previous block's list to update its last-paid height, and IsBlockPayeeValid selected it again from the same list to check the coinbase, each a full walk (two before MN_RR). GetMasternodePayments also walked the list once more in GetCounts() just to see whether it is empty. RebuildListFromBlock hands the payee it selected to its caller, ProcessSpecialTxsInBlock records it in MNListUpdates next to the lists it was chosen from, and the payment check uses it. Block templates and the RPC select the payee themselves as before.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
|
|
✅ Final review complete — no blockers (commit 1602784) · triage: normal |
|
No actionable comments were generated in the recent review.
 |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 15.38% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 26 functions across 10 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly identifies the main performance changes: optimizing deterministic masternode list construction and payee validation. The typo in “payye” does not prevent understanding. Description check The description directly explains the performance goals, implemented changes, testing method, measured results, and related build fix. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Static verification of ca14f98 found no actionable correctness or consensus regressions in the list-diff, previous-list reuse, or cached-payee changes. Approval is withheld because mac-build / Build source fails check-stdlib-paths on an embedded libc++ construct_at.h SDK path; the corresponding base job passed, and changed C++ template instantiations could affect this check, so the failure cannot yet be dismissed as unrelated. Linux standard, ASan, and TSan tests and the multiprocess build remain pending; no local builds or tests were run.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The diff makes moderately cross-cutting optimizations to masternode-list diffing and cached payee propagation through block validation, but is not sufficiently large or intricate to meet the critical tier's combined criteria. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer
thepastaclaw
added
the
pastaclaw:commented
thepastaclaw
removed
the
pastaclaw:commented
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The complete PR range was verified at the exact head. The masternode-list diff, payee propagation, payment generation, incremental builder, and mining call-site changes preserve the existing semantics; no actionable correctness, consensus, security, or runtime issue was found. Builds, lint, and formatting checks reported success, while several source-test checks were still in progress in the supplied CI snapshot.
🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — This is a cross-cutting performance refactor across deterministic masternode-list construction and block payee validation, but it does not directly alter consensus rules, funds movement, cryptography, networking deserialization, or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity quota lookup failed),glm-5.3-flash(not used above high effort; tier asks max) - Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.
No unresolved findings remain from the prior review on this head.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
CI failed after approval — commit 1602784
The standing review approved this commit; this follow-up moves the verdict to COMMENT: checks that failed after the review may relate to this change, and the evidence does not rule it out.
Failing checks:
linux64_tsan-test / Test source(failure)
Why it is unclear: The failing feature_llmq_signing.py --spork21 test exercises broad block, masternode, and payment validation paths that this PR changes, so a behavioral interaction is plausible. However, the provided log tail contains no failure assertion, race report, or stack trace identifying a changed path; all other tests passed and the failure occurred during shutdown, so the evidence is insufficient to classify it as related.
Nothing was re-reviewed: the findings of the standing review are unchanged. The next push gets a full review; a re-run that passes does not restore the approval.
Review provenance
Source: CI-check lane gpt-6.1-sol (agent: triage); no reviewer or verifier lanes ran for this follow-up
- Verdict moved because CI failed on this commit after the review; failure logs read: 1 GitHub Actions job(s)
thepastaclaw
added
pastaclaw:commented
Issue being fixed or feature implemented
Significant part of block validation is building new lists of masternodes and validation of block's payee.
It's not waste, but it could be done more efficient way. This PR aim to optimize some bottlenecks.
What was done?
This checks are done in the main validation thread and not parallelized, so it should make overall sync ~5% faster for all post-dip3 block, for blocks between v20 and mn_rr even more.
Extra fix provided by pastapastapasta@:
This fixes the macOS check-stdlib-paths failure for #7934.
How Has This Been Tested?
Some blocks close to tip had been invalidated and re-considered after [10k blocks].
Breaking Changes
N/A
Checklist: