fix!: efficient build mnlistdiffs in rotation info #6587
Conversation
…ighest known base block
WalkthroughThe changes introduce modifications to the ✨ Finishing Touches
🪧 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: 0
🧹 Nitpick comments (1)
src/llmq/snapshot.cpp (1)
304-304: Run clang-format.
The pipeline indicates formatting issues around line 304. Please run clang-format locally and commit the results to address the style check failure.🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 304-304: Clang format differences found. Please run 'clang-format' to format the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/llmq/snapshot.cpp(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Clang Diff Format Check
src/llmq/snapshot.cpp
[error] 304-304: Clang format differences found. Please run 'clang-format' to format the code.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
🔇 Additional comments (9)
src/llmq/snapshot.cpp (9)
245-245: No concerns on updatingbaseBlockIndexesfor H-4C.
This addition correctly stores the H-4C work block index for subsequent diffs, aligning with the approach of incrementally building MN list diffs from older to newer blocks.
303-303: No concerns on updatingbaseBlockIndexesfor needed blocks.
This push operation ensures the newly resolved work block index is tracked for future calls toBuildSimplifiedMNListDiff.
307-309: Properly builds the MN list diff at H-3C.
This call leveragesGetLastBaseBlockHashto ensure the correct base hash is used, then returns early on error. The logic is consistent with the new progressive approach.
310-310: Appending H-3C block index to the base list.
AddingpWorkBlockHMinus3CIndextobaseBlockIndexesensures future diffs include the latest known block.
312-314: Constructing the diff at H-2C.
The workflow is consistent: build the diff, check for errors, and proceed only when successful.
315-315: ExtendingbaseBlockIndexeswith H-2C.
This matches the incremental approach for building subsequent diffs.
317-319: Building the diff at H-C.
Same pattern: retrieving the last valid base hash, constructing the diff, and bubbling up errors if any.
320-320: Appending H-C work block index.
Ensures the new base index is available for final diff generation with the tip.
322-324: Final diff from the last base block to the tip.
UsingbaseBlockIndexes.back()as the starting point ensures the tip diff is built upon the most recent known base block index.
ogabrielides
changed the title
ogabrielides
changed the title
|
Looks like I missed some clang-format suggestions 🙈 pls add 927a8db |
Can you please clarify this? |
@PastaPastaPasta Sure, imagine the below scenario. Before this PR, the construction of MN list diff use the following base heights. With this PR: It is breaking change if Client doesn't apply diff 90->100 first in order to construct MN list at 100 (it wasn't know at request time) and beeing able to process the 100->125. |
ogabrielides
changed the title
|
How are we activating these breaking change? Was mobile made aware of this change and confirmed it won't actually break for them? Preferably this would be in a new protocol version and we'd maintain the old functionality if this is breaking. |
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6587.542ac804. A new comment will be made when the image is pushed. |
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6587.542ac804. The image should be on dockerhub soon. |
|
Guix Automation has began to build this PR tagged as v23.0.0-devpr6587.0fba0c5a. A new comment will be made when the image is pushed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
src/net_processing.cpp (1)
5203-5203: Fix typo in variable name and remove extra semicolonThe variable name has a typo ("leagcy" instead of "legacy") and there's an extra semicolon at the end of the line.
- bool use_leagcy_construction = pfrom.GetCommonVersion() < EFFICIENT_QRINFO_VERSION;; + bool use_legacy_construction = pfrom.GetCommonVersion() < EFFICIENT_QRINFO_VERSION;src/rpc/quorums.cpp (1)
863-863: Clarify the hardcodedfalseparameter usage.
Passingfalseforuse_legacy_constructionmay be correct if we want to ensure the newly introduced “efficient” path is always used in the RPC scenario. However, if there is a future need to support both legacy and non-legacy paths at the RPC layer (e.g., for backward compatibility or testing different client behaviors), consider making this a configurable parameter.src/llmq/snapshot.cpp (6)
115-118: Consider legacy logic naming.
SortingbaseBlockIndexesonly whenuse_legacy_constructionis true might be confusing, as “legacy” typically implies an older, perhaps simpler approach. Verify that “legacy” indeed requires sorting here, or rename for clarity (e.g.sort_base_blocks_in_legacy_mode) if it helps future maintainability.
156-157: Check for negative ancestor height.
Obtaining an ancestor athBlockIndex->nHeight - workDiffcan fail ifhBlockIndex->nHeight < workDiff. The code checksif (!pWorkBlockHIndex) {...}but consider also logging or clarifying for debugging if the height is unexpectedly too small.
213-218: Inspect repeated calls toBuildSimplifiedMNListDiff.
These lines repeat the pattern of building diffs for various “H minus C” blocks whenuse_legacy_constructionis true. If other code paths do the same for the non-legacy mode, consider extracting a helper function to reduce duplication and potential mistakes.
246-253: Parallel logic for H-3C.
Similar to the previous blocks, ensure that H-3C logic (both snapshot retrieval and the resulting MN list diff) is tested in a scenario where H-3C is near the chain genesis or the chain tip. Corner cases here might differ from H-2C or H-C.
340-350: Potential repeated sorting of baseBlockIndexes.
Each new block requires a call toGetLastBaseBlockHash, which can internally sort the indexes again if!use_legacy_construction. If there are many calls in a loop, consider caching or sorting only once to improve performance.
392-400: Multiple sorts inGetLastBaseBlockHash.
SortingbaseBlockIndexeshere can be costly if repeated. If it’s commonly invoked in quick succession, consider sorting once up front or using a data structure that preserves sorted order. Alternatively, only re-sort if the list is modified.
📜 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
🧬 Code Definitions (2)
src/net_processing.cpp (2)
src/llmq/snapshot.h (1)
BuildQuorumRotationInfo(210-213)src/llmq/snapshot.cpp (2)
BuildQuorumRotationInfo(87-390)BuildQuorumRotationInfo(87-90)
src/llmq/snapshot.cpp (1)
src/llmq/snapshot.h (1)
GetLastBaseBlockHash(215-216)
🔇 Additional comments (8)
src/version.h (1)
64-65:❓ Verification inconclusive
The versioning change correctly identifies a protocol behavior modification.
The addition of the
EFFICIENT_QRINFO_VERSIONconstant appropriately follows the versioning pattern in the file and clearly documents the protocol change related to QRINFO behavior. This version increment is essential since it introduces a breaking change in how MNListDiffs are constructed (from oldest to newest block rather than using the highest baseBlockHash).Since this introduces a breaking change for SPV clients, please ensure that:
- Proper documentation is updated to reflect this behavior change
- Mobile wallet teams and other SPV client maintainers are notified about this change
🌐 Web query:
What is the typical notification process for breaking protocol changes in Dash?💡 Result:
The notification process for breaking protocol changes in Dash involves a structured approach to ensure network participants are informed and prepared for mandatory upgrades. Key aspects include:
Protocol Upgrade Announcements
- Public release notes detail technical specifications and required actions for node operators[2][5].
- Coordinated timelines specify exact activation blocks (e.g., Dash Core v22.0.0 hard fork at block height 2,201,472)[5].
- Exchange partnerships ensure third-party platforms like Binance halt services during upgrades and notify users[5].
Network Participant Requirements
- Mandatory node upgrades with warnings about incompatibility for outdated versions[2].
- Extended lead times providing ≥6 months notice for significant changes like BIP324 encryption implementation[2][5].
Communication Channels
- Developer documentation with version-specific migration guides[1][4].
- Decentralized governance involving masternodes in proposal voting for protocol changes[10][16].
- Community alerts through official blogs and trusted infrastructure providers like NOWNodes[2].
For critical updates like 2024's Asset Unlock Transactions fork, Dash combines these elements with encrypted P2P messaging enhancements (via ZMQ notifications)[4] and experimental feature flags to phase in changes gradually[2]. The process emphasizes backward compatibility where possible, though hard forks require explicit network-wide coordination[5].
Citations:
- 1: https://docs.dash.org/projects/core/en/stable/docs/reference/p2p-network-protocol-versions.html
- 2: https://nownodes.io/blog/update-dash-core-version-22-0-0/
- 3: https://docs.dapr.io/operations/support/breaking-changes-and-deprecations/
- 4: https://docs.dash.org/projects/core/en/19.0.0/docs/api/zmq.html
- 5: https://crypto.news/binance-will-support-dash-network-upgrade-and-hard-fork/
- 6: https://dashif.org/docs/Report%20on%20Low%20Latency%20DASH.pdf
- 7: https://www.bitdegree.org/crypto/dash-cryptocurrency
- 8: https://en.wikipedia.org/wiki/Dash_(cryptocurrency)
- 9: https://docs.dash.org/projects/platform/en/stable/docs/explanations/platform-protocol.html
- 10: https://github.com/dashpay/dash/wiki/whitepaper
- 11: https://edge.app/blog/market-updates/dash-development-update-dash-evolution/
- 12: https://docs.dash.org/projects/core/en/stable/docs/api/remote-procedure-calls-raw-transactions.html
- 13: https://iq.wiki/wiki/dash
- 14: https://www.dash.org/roadmap/
- 15: https://dashif.org/Ingest/
- 16: https://coinflip.tech/blog/cryptocurrency-explained-dash
- 17: https://cina.gmu.edu/publications/dash-cryptocurrency-deanonymization/
- 18: https://www.omnipod.com/sites/default/files/2021-04/Omnipod-DASH_User-Guide_English.pdf
- 19: https://empirica.io/blog/everything-one-needs-to-know-about-dash/
- 20: https://coinmarketcap.com/currencies/dash/
Protocol Change Notification & Documentation Update
The addition of
EFFICIENT_QRINFO_VERSIONinsrc/version.hcorrectly reflects the protocol behavior modification—specifically, the change in MNListDiff construction order for QRINFO. Given that this is a breaking change for SPV clients, please ensure that:
- Documentation Update: The technical documentation and migration guides are updated to clearly describe the new behavior.
- Release Notes: Public release notes are issued in line with Dash’s structured notification process, detailing the change and the activation timeline.
- Stakeholder Notification: Mobile wallet teams and other SPV client maintainers are formally notified, following Dash’s established communication channels (e.g., developer documentation updates, community alerts, and coordinated timelines).
src/net_processing.cpp (1)
5203-5204: Implementation of version-dependent efficient quorum rotation infoThe code now conditionally uses a more efficient method for building quorum rotation info when communicating with newer clients, while maintaining backward compatibility with older ones. This approach ensures that older clients continue to work with the legacy construction method, while newer clients benefit from the improved efficiency.
Per the PR description, this change allows for constructing diffs from oldest to newest blocks, which is more efficient than the previous method. SPV clients need to be aware of this change as they should apply the diffs in the order from oldest to newest.
src/llmq/snapshot.cpp (5)
90-90: Validate all call sites for new function signature.
Introducing thebool use_legacy_constructionparameter ensures flexibility for two different construction modes. Please confirm that every caller sets it correctly (e.g., in net_processing, rpc, tests) to avoid accidental behavior changes.
126-131: Confirm usage of the “highest baseblock” assumption.
Whenuse_legacy_constructionis enabled, the code always builds the MN list diff against the last (highest) base block index. Ensure that this approach aligns with any downstream logic expecting progressive base heights rather than only the max height when in legacy mode.
162-168: Ensure consistent usage ofGetLastBaseBlockHash.
Here, we build a diff forpWorkBlockHIndexonly in legacy mode. Verify if modern mode must also produce diffs earlier for the same block; otherwise the resulting data might differ drastically between modes.
229-235: Maintain consistency with pWorkBlockHMinus2CIndex usage.
This code again uses the same approach for H-2C as with H-C. Watch out for subtle off-by-one or edge cases if the chain tip is below this height. Also confirm that block ordering is consistent with the newly introduced sorting logic.
279-290: Evaluate the extraShare path for H-4C.
Whenrequest.extraShareis set, the code extends to H-4C and builds the diff accordingly. If the chain is incomplete or near genesis, ensure that we handle the scenario gracefully. Also, confirm that the new logic andlegacysorting remain coherent when “extraShare” is toggled on.src/llmq/snapshot.h (1)
213-217: Confirm alignment of function signatures with usage.
BothBuildQuorumRotationInfoandGetLastBaseBlockHashnow acceptbool use_legacy_construction. Verify that all headers and implementations match, and that no external references are forgotten. If these are part of any public API or used in functional tests, confirm that those calls also pass the correct parameter.
| if (!use_legacy_construction) { | ||
| if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman, | ||
| GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHMinus3CIndex, | ||
| use_legacy_construction), | ||
| pWorkBlockHMinus3CIndex->GetBlockHash(), response.mnListDiffAtHMinus3C, errorRet)) { | ||
| return false; | ||
| } | ||
| baseBlockIndexes.push_back(pWorkBlockHMinus3CIndex); | ||
|
|
||
| if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman, | ||
| GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHMinus2CIndex, | ||
| use_legacy_construction), | ||
| pWorkBlockHMinus2CIndex->GetBlockHash(), response.mnListDiffAtHMinus2C, errorRet)) { | ||
| return false; | ||
| } | ||
| baseBlockIndexes.push_back(pWorkBlockHMinus2CIndex); | ||
|
|
||
| if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman, | ||
| GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHMinusCIndex, use_legacy_construction), | ||
| pWorkBlockHMinusCIndex->GetBlockHash(), response.mnListDiffAtHMinusC, errorRet)) { | ||
| return false; | ||
| } | ||
| baseBlockIndexes.push_back(pWorkBlockHMinusCIndex); | ||
|
|
||
| if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman, | ||
| GetLastBaseBlockHash(baseBlockIndexes, pWorkBlockHIndex, use_legacy_construction), | ||
| pWorkBlockHIndex->GetBlockHash(), response.mnListDiffH, errorRet)) { | ||
| return false; | ||
| } | ||
| baseBlockIndexes.push_back(pWorkBlockHIndex); | ||
|
|
||
| if (!BuildSimplifiedMNListDiff(dmnman, chainman, qblockman, qman, | ||
| GetLastBaseBlockHash(baseBlockIndexes, tipBlockIndex, use_legacy_construction), | ||
| tipBlockIndex->GetBlockHash(), response.mnListDiffTip, errorRet)) { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check for duplication between legacy and non-legacy paths.
These lines add a large else-branch to handle non-legacy mode, effectively re-building diffs in ascending block order. Much of it appears to replicate the logic also present in the legacy approach. Removing or unifying shared steps can simplify future maintenance.
|
Guix Automation has completed; a release should be present here: https://github.com/dashpay/dash-dev-branches/releases/tag/v23.0.0-devpr6587.0fba0c5a. The image should be on dockerhub soon. |
| llmq::CQuorumRotationInfo quorumRotationInfoRet; | ||
| std::string strError; | ||
| if (BuildQuorumRotationInfo(*m_dmnman, *m_llmq_ctx->qsnapman, m_chainman, *m_llmq_ctx->qman, *m_llmq_ctx->quorum_block_processor, cmd, quorumRotationInfoRet, strError)) { | ||
| bool use_leagcy_construction = pfrom.GetCommonVersion() < EFFICIENT_QRINFO_VERSION;; |
There was a problem hiding this comment.
nit: s/use_leagcy_construction/use_legacy_construction/
| return false; | ||
| } | ||
| baseBlockIndexes.push_back(pWorkBlockHMinus3CIndex); | ||
| if (!use_legacy_construction) { |
There was a problem hiding this comment.
maybe for simplicity better to invert this condition? if legacy -> return true.
shorter diff, easier to read, and review.
| static const int INCREASE_MAX_HEADERS2_VERSION = 70235; | ||
|
|
||
| //! Behavior of QRINFO is changed in this protocol version | ||
| static const int EFFICIENT_QRINFO_VERSION = 70236; |
There was a problem hiding this comment.
should update PROTOCOL_VERSION also
-static const int PROTOCOL_VERSION = 70235;
+static const int PROTOCOL_VERSION = 70236;
|
Super-seeded by #6622 |
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 #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
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
5 participants
Issue being fixed or feature implemented
On
GETQUORUMROTATIONINFOP2P message andquorum rotationinfoRPC the response was constructed inefficiently.On every MNListDiff construction, the diff was constructed using the highest baseBlockHash from request.baseBlockHashes or genesis if empty.
What was done?
With this PR, the Diffs are constructed from the oldest block to the newest. Everytime a diff is constructed, we assume the target height of the diff is know to the client for the construction of the next diff.
How Has This Been Tested?
feature_llmq_rotation.py --nocleanup, dash-qt --regtest --datadir= and then
quorum rotationinfo {tip}In the response
baseBlockHashwere progressive.Breaking Changes
If SPV clients don't apply the Diffs from the oldest to the newest then yes.
Checklist: