fix: efficient build mnlistdiffs in rotation info #6622
Conversation
WalkthroughThis pull request updates several functions related to quorum rotation information by introducing a new boolean parameter, 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
src/net_processing.cpp (1)
5203-5203: Fix double semicolon typoThere's an extra semicolon at the end of this statement.
- bool use_legacy_construction = pfrom.GetCommonVersion() < EFFICIENT_QRINFO_VERSION;; + bool use_legacy_construction = pfrom.GetCommonVersion() < EFFICIENT_QRINFO_VERSION;🧰 Tools
🪛 GitHub Actions: Check Potential Conflicts
[error] Conflicts detected with multiple pull requests.
src/llmq/snapshot.cpp (7)
90-90: Add documentation for newly introduced parameter.
It would be helpful to add a brief comment explaining the purpose ofuse_legacy_construction, clarifying when or why it should be set to true/false.
163-168: Avoid repeated code blocks for building MN list diffs.
This block largely duplicates the approach seen in otherif (use_legacy_construction)sections. Centralizing or abstracting the repeated "BuildSimplifiedMNListDiff" calls could reduce code duplication and improve maintainability.
229-235: Reduce repeated try-catch usage pattern.
Again, the same diff-building logic. Combining repeated calls with a small helper function, e.g.,BuildAndAssignMNListDiff(...), can minimize code repetition and simplify future changes.
279-283: Validate consistent usage ofpWorkBlockHMinus4CIndex.
When buildingmnListDiffAtHMinus4C, ensure the code path is tested whenextraShareis enabled, particularly in conjunction with different settings foruse_legacy_construction.
340-343: Keep error messages consistent.
WhenBuildSimplifiedMNListDifffails, the error is returned immediately. Verify that each failure path setserrorRetconsistently.
346-348: Consolidate control flow.
These conditional blocks create a pattern: if!use_legacy_construction, build a diff and then push back tobaseBlockIndexes. Repeated code blocks might benefit from a helper method that streamlines the “build-and-push” sequence while capturing the repeated error checks.
392-394: Add documentation for new parameter.
In the function signature forGetLastBaseBlockHash, explicitly document howuse_legacy_constructionmodifies the chosen base block hash.src/llmq/snapshot.h (1)
213-217: Include parameter rationale in header docs.
For bothBuildQuorumRotationInfoandGetLastBaseBlockHash, briefly describe the meaning ofbool use_legacy_constructionin the header. This fosters clarity for callers and future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/llmq/snapshot.cpp(8 hunks)src/llmq/snapshot.h(1 hunks)src/net_processing.cpp(1 hunks)src/rpc/quorums.cpp(1 hunks)src/version.h(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Check Potential Conflicts
src/rpc/quorums.cpp
[error] Conflicts detected with multiple pull requests.
src/net_processing.cpp
[error] Conflicts detected with multiple pull requests.
🔇 Additional comments (7)
src/version.h (1)
64-65: LGTM: New protocol version constant for efficient QRINFOThe addition of this new protocol version constant follows the existing pattern and is properly documented. This constant will be used to determine when to use the new efficient construction for building MN list differences in rotation info.
src/rpc/quorums.cpp (1)
862-863:✅ Verification successful
New parameter added to BuildQuorumRotationInfo
The function call has been updated to include the new boolean parameter
falseto use the efficient construction method for building quorum rotation information. This aligns with the PR's objective to improve efficiency when building mnlistdiffs in rotation info.
🏁 Script executed:
#!/bin/bash # Check for other calls to BuildQuorumRotationInfo in the codebase to verify consistent parameter usage # Find all calls to BuildQuorumRotationInfo echo "Finding all calls to BuildQuorumRotationInfo function:" rg "BuildQuorumRotationInfo\(" --type cppLength of output: 752
Confirmed: Efficient Quorum Rotation Construction Implementation
The updated call in
src/rpc/quorums.cppnow includes the new boolean parameter explicitly set tofalseto enable the efficient construction method for quorum rotation info. Verification shows that while other files likesrc/net_processing.cpppass a flag (e.g.,use_legacy_construction), the use offalsehere aligns with the PR's objective to optimize the building of mnlistdiffs for rotation info.
- File:
src/rpc/quorums.cpp(lines 862-863)
- Updated call correctly passes
falsefor efficient construction.- Other references: The pattern is consistent with related calls seen in
src/net_processing.cpp.🧰 Tools
🪛 GitHub Actions: Check Potential Conflicts
[error] Conflicts detected with multiple pull requests.
src/net_processing.cpp (1)
5203-5204: Good backward compatibility approach for efficient quorum rotation buildsThe new parameter
use_legacy_constructioncorrectly determines the construction method based on peer version, ensuring compatibility with older nodes while enabling optimizations for newer ones. This aligns with the PR objective of making the mnlistdiffs build more efficient while maintaining backward compatibility.🧰 Tools
🪛 GitHub Actions: Check Potential Conflicts
[error] Conflicts detected with multiple pull requests.
src/llmq/snapshot.cpp (4)
126-131: Ensure code consistency for legacy vs. new approach.
Whenuse_legacy_constructionis true, you build the MN list diff in this block. Double-check that the logic for the new approach (in lines 352-388) yields an identical final state or otherwise meets backward-compatibility requirements.
213-218: Clarify necessity of multipleBuildSimplifiedMNListDiffchecks.
Foruse_legacy_construction, you build and store multiple diffs under slightly different conditions. If any block fails, you return immediately. Ensure that skipping subsequent builds on failure is intentional and verified through testing.
247-253: Assess performance impact of partial vs. final approach.
Ifuse_legacy_constructionis true, you handle these sub-snapshots earlier. Compare the overall performance with the later single pass approach used whenuse_legacy_constructionis false.
286-288: Double-check size growth ofbaseBlockIndexes.
Here, you push back a new block index only ifuse_legacy_constructionis false. Repeatedly calling the function or building multiple diffs could grow this vector. Confirm whether this is intentional and does not cause unintended sorting states.
| if (!use_legacy_construction) { | ||
| std::sort(baseBlockIndexes.begin(), baseBlockIndexes.end(), | ||
| [](const CBlockIndex* a, const CBlockIndex* b) { return a->nHeight < b->nHeight; }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unify sorting strategy.
Currently, we sort here only if !use_legacy_construction, while lines 115-118 sort only if use_legacy_construction. Evaluate whether it’s possible or preferable to sort unconditionally at a single point to reduce confusion.
…ighest known base block
PastaPastaPasta
force-pushed
the
fix/perf_qr_rotation
branch
from
fcf95bf to
606719a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/llmq/snapshot.cpp (3)
352-388: Add a comment explaining the reversed construction orderThe non-legacy approach builds the MN list diffs in reverse order (from oldest to newest) compared to the legacy approach. This is an important algorithmic change that should be documented for clarity.
if (!use_legacy_construction) { + // Build MN list diffs in reverse chronological order (from oldest to newest) + // This is more efficient as each diff becomes a base for the next one if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman, GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHMinus3CIndex, use_legacy_construction),
286-288: Document why baseBlockIndexes is updated differentlyThe non-legacy approach updates
baseBlockIndexesafter building each MN list diff, while the legacy approach doesn't. This behavior difference should be documented to explain why this is necessary.if (!use_legacy_construction) { + // In the non-legacy approach, we add each block to baseBlockIndexes after using it + // This creates a growing set of potential base blocks for subsequent diff calculations baseBlockIndexes.push_back(pWorkBlockHMinus4CIndex); }Also applies to: 346-348
87-90: Document the performance benefits of the new approachSince this PR is titled "fix: efficient build mnlistdiffs in rotation info", it would be valuable to add a comment explaining how the non-legacy construction approach improves performance over the legacy approach.
Add a comment near the function definition or where the non-legacy path starts, explaining the performance improvements, like:
bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotManager& qsnapman, const ChainstateManager& chainman, const CQuorumManager& qman, const CQuorumBlockProcessor& qblockman, const CGetQuorumRotationInfo& request, + // The non-legacy construction approach builds diffs in chronological order + // and reuses previous results, reducing redundant calculations bool use_legacy_construction, CQuorumRotationInfo& response, std::string& errorRet)Also applies to: 126-132, 352-388
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/llmq/snapshot.cpp(8 hunks)src/llmq/snapshot.h(1 hunks)src/net_processing.cpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/net_processing.cpp
- src/llmq/snapshot.h
🔇 Additional comments (3)
src/llmq/snapshot.cpp (3)
115-118: Consider adding a comment explaining the sorting strategy differenceThe function sorts
baseBlockIndexeswhenuse_legacy_constructionis true, whileGetLastBaseBlockHash(lines 396-399) sorts when it's false. These opposing conditions could be confusing for future maintainers.Add a comment like:
if (use_legacy_construction) { + // Legacy approach sorts blocks at the beginning for deterministic traversal std::sort(baseBlockIndexes.begin(), baseBlockIndexes.end(), [](const CBlockIndex* a, const CBlockIndex* b) { return a->nHeight < b->nHeight; }); }
126-132: Consider refactoring to reduce code duplicationThe code now has two distinct paths for building MN list diffs: the legacy approach (with multiple conditional blocks) and the new approach (lines 352-388). This duplicates a lot of logic which could lead to maintenance challenges if the core functionality needs to be updated in the future.
Consider creating a helper function that can build the diffs in either order based on the
use_legacy_constructionflag.Also applies to: 162-169, 213-219, 229-236, 246-253, 352-388
392-399: Consider adding a comment explaining the sorting strategy differenceThe sorting condition here is the opposite of the one in lines 115-118. To avoid confusion, add a comment explaining why the sorting is done differently in the non-legacy approach.
if (!use_legacy_construction) { + // Non-legacy approach sorts blocks just before using them for finding the last base block + // This enables the incremental building of baseBlockIndexes in BuildQuorumRotationInfo std::sort(baseBlockIndexes.begin(), baseBlockIndexes.end(), [](const CBlockIndex* a, const CBlockIndex* b) { return a->nHeight < b->nHeight; }); }
| bool BuildQuorumRotationInfo(CDeterministicMNManager& dmnman, CQuorumSnapshotManager& qsnapman, | ||
| const ChainstateManager& chainman, const CQuorumManager& qman, | ||
| const CQuorumBlockProcessor& qblockman, const CGetQuorumRotationInfo& request, | ||
| CQuorumRotationInfo& response, std::string& errorRet) | ||
| bool use_legacy_construction, CQuorumRotationInfo& response, std::string& errorRet) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure this API change is documented in the project
The function signature has been updated to include the new parameter bool use_legacy_construction. This is a significant change that affects how quorum rotation information is built. Make sure this change is properly documented and that callers of this function are updated accordingly.
🏁 Script executed:
#!/bin/bash
# Check all callers of BuildQuorumRotationInfo to ensure they've been updated
rg "BuildQuorumRotationInfo\s*\(" --type cpp -A 3 -B 3Length of output: 2565
API Change: Update Documentation for New Parameter
The updated function signature in src/llmq/snapshot.cpp and its corresponding header now includes the new parameter bool use_legacy_construction. Verification shows that the callers in both src/net_processing.cpp and src/rpc/quorums.cpp have been updated accordingly.
Please ensure that this significant API change is also reflected in the project documentation (e.g., API docs, changelog, or README) so that downstream users are aware of the new behavior.
UdjinM6
dismissed
their stale review
oh, wait. there is no PROTOCOL_VERSION related changes yet
|
see #6587 (comment) |
|
Done |
03c15a3 fix: actually bump protocol version (pasta) 606719a nit: s/use_leagcy_construction/use_legacy_construction/ (pasta) 7432879 fmt: apply clang-format suggestions (pasta) bdfd597 feat: only use efficient qrinfo construction for new shared protocol version (pasta) 71112dc refactor: more clang-format (UdjinM6) ff16e68 refactor: clang-format (UdjinM6) 1f16cf4 fix: sort indexes in GetLastBaseBlockHash (UdjinM6) 810ecd8 fix: don't use `baseBlockIndexes.back()` for the tip (UdjinM6) ec78465 fix: move BuildSimplifiedMNListDiff for block h (UdjinM6) 2a12ff6 bump guix (Konstantin Akimov) e8bbfd2 fix(perf): build mnlistdiffs in rotation info using dynamically the highest known base block (Odysseas Gabrielides) Pull request description: ## Issue being fixed or feature implemented See dashpay#6587 for history ## Breaking Changes This should remain backwards compatible ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] 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 - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 03c15a3 Tree-SHA512: d378b519fe614f047b031509b0ec764160ae8674f7417ef571ce7e6fd98d6c94966e6c94681e7bb9c66ac334ce3640492948aae6bb45737dfdf480f6f58b4472
03c15a3 fix: actually bump protocol version (pasta) 606719a nit: s/use_leagcy_construction/use_legacy_construction/ (pasta) 7432879 fmt: apply clang-format suggestions (pasta) bdfd597 feat: only use efficient qrinfo construction for new shared protocol version (pasta) 71112dc refactor: more clang-format (UdjinM6) ff16e68 refactor: clang-format (UdjinM6) 1f16cf4 fix: sort indexes in GetLastBaseBlockHash (UdjinM6) 810ecd8 fix: don't use `baseBlockIndexes.back()` for the tip (UdjinM6) ec78465 fix: move BuildSimplifiedMNListDiff for block h (UdjinM6) 2a12ff6 bump guix (Konstantin Akimov) e8bbfd2 fix(perf): build mnlistdiffs in rotation info using dynamically the highest known base block (Odysseas Gabrielides) Pull request description: ## Issue being fixed or feature implemented See dashpay#6587 for history ## Breaking Changes This should remain backwards compatible ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] 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 - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 03c15a3 Tree-SHA512: d378b519fe614f047b031509b0ec764160ae8674f7417ef571ce7e6fd98d6c94966e6c94681e7bb9c66ac334ce3640492948aae6bb45737dfdf480f6f58b4472
8b4ab03 fix: suppress MIN_MASTERNODE_PROTO_VERSION bump in 6608 (pasta) aca04d1 chore: bump build to 22.1.2 (pasta) d9d8c24 docs: add release notes for 22.1.2 (pasta) fb45240 Merge #6608: fix: `cycleHash` should represent a cycle starting block of the signing quorum (pasta) 9d1498c Merge #6625: fix: adjust quorum rotation data results in some edge cases, add tests (pasta) dfc1119 Merge #6622: fix: efficient build mnlistdiffs in rotation info (pasta) 6fd626b Merge #6586: fix: revert deployment images back to Ubuntu 22.04 LTS (`jammy`), pin QEMU to avoid segfault (pasta) affa9d1 Merge #6599: fix: follow-up #6546 to bump copyright year in COPYING and debian's package (pasta) f6163a2 Merge #6593: fix: resolve potential deadlock in coinjoin_tests (pasta) 243e0ab Merge #6585: fix: Do not assert special tx type for cbtx in simplified mn list diff output (pasta) 497f95c Merge #6581: perf: speedup of CBLSLazyPublicKey::operator== when comparing to the default / null object; speedup CDeterministicMNList::AddMN by avoiding check to IsValid when a nullcheck is sufficient (pasta) Pull request description: ## Issue being fixed or feature implemented Backports for a new version, v22.1.2 ## What was done? See release notes ## How Has This Been Tested? ## Breaking Changes None ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] 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 - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 8b4ab03 knst: utACK 8b4ab03 Tree-SHA512: 9f2f65e315940197cc2b75a6b0a858d624256cbe668272ff6dfa216eceda1ba9338484d47afc569202b3b5cc75b4dcb825209efe3a3d3ccec57c741f75c40577
8b4ab03 fix: suppress MIN_MASTERNODE_PROTO_VERSION bump in 6608 (pasta) aca04d1 chore: bump build to 22.1.2 (pasta) d9d8c24 docs: add release notes for 22.1.2 (pasta) fb45240 Merge #6608: fix: `cycleHash` should represent a cycle starting block of the signing quorum (pasta) 9d1498c Merge #6625: fix: adjust quorum rotation data results in some edge cases, add tests (pasta) dfc1119 Merge #6622: fix: efficient build mnlistdiffs in rotation info (pasta) 6fd626b Merge #6586: fix: revert deployment images back to Ubuntu 22.04 LTS (`jammy`), pin QEMU to avoid segfault (pasta) affa9d1 Merge #6599: fix: follow-up #6546 to bump copyright year in COPYING and debian's package (pasta) f6163a2 Merge #6593: fix: resolve potential deadlock in coinjoin_tests (pasta) 243e0ab Merge #6585: fix: Do not assert special tx type for cbtx in simplified mn list diff output (pasta) 497f95c Merge #6581: perf: speedup of CBLSLazyPublicKey::operator== when comparing to the default / null object; speedup CDeterministicMNList::AddMN by avoiding check to IsValid when a nullcheck is sufficient (pasta) 4298d73 chore: bump to 22.1.1 (pasta) fc65a16 chore: release notes for 22.1.1 (pasta) 38762f7 Merge #6574: fix: ReconnectionInfo should also store Dash-specific flags (pasta) 580b74c Merge #6566: fix(qt): avoid leaking balance and CJ info in GUI when in discreet mode (pasta) Pull request description: ## Issue being fixed or feature implemented ## What was done? ff-ed `master` 21.1.0 -> 21.1.2, merging it back into `develop` now ## How Has This Been Tested? ## Breaking Changes ## 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 73fa780; gonna merge as this changes docs only Tree-SHA512: f04fe461edc05c38771d684bd5d60327076251b8004723027276b104989ea6d84f7f77cce31f310f058e81f2c25411ab5a15b563cd3db66091ccaf33da459e3c
5 participants
Issue being fixed or feature implemented
See #6587 for history
Breaking Changes
This should remain backwards compatible
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.