fix: cycleHash should represent a cycle starting block of the signing quorum
#6608
Conversation
WalkthroughThe pull request introduces significant modifications to the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (3)
✨ 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/instantsend.cpp (1)
680-682: Consider safer handling ofllmq_params_optinstead ofassert
Currently, relying onassert(llmq_params_opt)could cause a crash in production ifparams.llmqTypeDIP0024InstantSendwere ever invalid in a release build (whereNDEBUGmight disable asserts). A more robust approach might be to return early with an error log ifllmq_params_optis not set, rather than asserting.if (!llmq_params_opt) { - assert(llmq_params_opt); + LogPrint(BCLog::INSTANTSEND, + "CInstantSendManager::%s -- missing LLMQ params for type=%d\n", __func__, (int)llmqType); + return; } const auto& llmq_params = llmq_params_opt.value();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/llmq/instantsend.cpp(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (3)
src/llmq/instantsend.cpp (3)
684-689: Good early return upon missing quorum
The immediate return with a log message is clear and prevents further processing when quorum selection fails. This behavior helps avoid null pointer issues later in the function.
690-692: Verify safety ofGetAncestorcall
This code calculates the DKG cycle height and usesGetAncestor(cycle_height)to retrieve a block index. While it’s typically valid for well-formed blocks, consider guarding against a null return in case of an unusual or partial chain scenario, for robustness.
703-703: Appropriate inclusion of the quorum base block hash
Passingquorum->m_quorum_base_block_index->GetBlockHash()toAsyncSignIfMemberaligns with the new quorum-centric logic. Make sure all internal usages of this additional parameter handle it carefully, especially if the base block transforms or changes in future expansions.
|
I think we should tie this to a new protocol version. Do you agree? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
doc/release-notes-6608.md (2)
4-4: Improve Punctuation and Clarity in the DescriptionThe description on line 4 is a bit hard to read due to missing commas and an omitted article. Consider revising it for clarity. For example, inserting a comma after "Dash Core" and adding "the" before "P2P protocol version" would improve readability.
Diff suggestion:-* `cycleHash` field in `isdlock` message will now represent a DKG cycle starting block of the signing quorum instead of a DKG cycle starting block corresponding to the current chain height. While this is fully backwards compatible with older versions of Dash Core other implementations might not be expecting this so P2P protocol version was bumped to 70236. +* `cycleHash` field in `isdlock` message will now represent a DKG cycle starting block of the signing quorum instead of a DKG cycle starting block corresponding to the current chain height. While this is fully backwards compatible with older versions of Dash Core, other implementations might not be expecting this, so the P2P protocol version was bumped to 70236.🧰 Tools
🪛 LanguageTool
[uncategorized] ~4-~4: A comma might be missing here.
Context: ... compatible with older versions of Dash Core other implementations might not be expe...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~4-~4: A comma might be missing here.
Context: ... implementations might not be expecting this so P2P protocol version was bumped to 7...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~4-~4: You might be missing the article “the” here.
Context: ...ntations might not be expecting this so P2P protocol version was bumped to 70236.(AI_EN_LECTOR_MISSING_DETERMINER_THE)
5-5: Remove Unintended Stray TextIt appears that line 5 contains an unintended stray "5". If this text is not part of the intended content, please remove it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
doc/release-notes-6608.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
doc/release-notes-6608.md
[uncategorized] ~4-~4: A comma might be missing here.
Context: ... compatible with older versions of Dash Core other implementations might not be expe...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~4-~4: A comma might be missing here.
Context: ... implementations might not be expecting this so P2P protocol version was bumped to 7...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~4-~4: You might be missing the article “the” here.
Context: ...ntations might not be expecting this so P2P protocol version was bumped to 70236.
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: linux64_ubsan-test / Test source
- GitHub Check: linux64_sqlite-test / Test source
- GitHub Check: linux64-test / Test source
- GitHub Check: linux64_multiprocess-build / Build source
🔇 Additional comments (1)
doc/release-notes-6608.md (1)
1-2: Section Header Looks GoodThe header "P2P Changes" and its underline are clear and appropriately formatted.
|
Shouldn't we actually change the logic based on the protocol version? |
|
rebased simply to make GH CI happy, no code changes |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/llmq/instantsend.cpp (1)
681-689: Consider adding a comment explaining the rationale for this change.While the code is well-structured, adding a comment explaining why the
cycleHashis now based on the quorum's base block rather than the current chain height would improve maintainability and help future developers understand the reasoning behind this approach.const auto& llmq_params_opt = Params().GetLLMQ(llmqType); assert(llmq_params_opt); +// Select the quorum for signing and use its base block to calculate cycleHash +// This ensures cycleHash represents a cycle starting block of the signing quorum +// rather than using current chain height which might be from a different cycle const auto quorum = SelectQuorumForSigning(llmq_params_opt.value(), m_chainstate.m_chain, qman, id);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
doc/release-notes-6608.md(1 hunks)src/llmq/instantsend.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- doc/release-notes-6608.md
🔇 Additional comments (3)
src/llmq/instantsend.cpp (3)
681-693: Improved logic for calculatingcycleHashusing signing quorum's base block.The new implementation correctly resolves the issue mentioned in the PR objectives by using the quorum's base block index to calculate
cycleHashrather than the current chain height. This ensures that thecycleHashrepresents a cycle starting block of the signing quorum, preventing confusion when DKG is ongoing.The code also includes proper error handling with early return if quorum selection fails.
704-704: Updated method signature forAsyncSignIfMemberto include quorum's base block hash.The method call has been properly updated to include the quorum's base block hash as an additional parameter, which aligns with the improved method for calculating
cycleHash.
681-693:❓ Verification inconclusive
Verify protocol version update is properly handled.
The PR indicates this change requires a protocol version update (70235 -> 70236). Ensure all code paths properly handle both the old and new behavior based on protocol version for backward compatibility.
🏁 Script executed:
#!/bin/bash # Check if the code handles the protocol version update properly # Look for ISDLOCK_CYCLEHASH_UPDATE_VERSION constant grep -r "ISDLOCK_CYCLEHASH_UPDATE_VERSION" --include="*.h" --include="*.cpp" . # Check for version checks related to the cycleHash calculation grep -r "cycleHash" --include="*.cpp" . | grep -i "version"Length of output: 211
Protocol Version Handling in CycleHash Calculation
The PR updates the protocol version constant (ISDLOCK_CYCLEHASH_UPDATE_VERSION at 70236), but our search shows no explicit version-check condition around the cycleHash calculation in the shown code. Please verify that:
- The new cycleHash computation is gated by a protocol version check (or that older nodes/processes follow an alternative code path) to ensure proper backward compatibility.
- Other related code paths correctly differentiate between protocol versions 70235 and 70236.
|
This pull request has conflicts, please rebase. |
…g block of the signing quorum d150a65 docs: add release notes (UdjinM6) 85c6b58 feat: bump PROTOCOL_VERSION/MIN_MASTERNODE_PROTO_VERSION to 70237 (UdjinM6) c20dfd8 fix: `cycleHash` should represent a cycle starting block of the signing quorum (UdjinM6) Pull request description: ## Issue being fixed or feature implemented Currently `cycleHash` is calculated based on the current chain height we are signing `isdlock` at. This however can result in `cycleHash` to be a block hash of a new cycle where DKG is still in progress which can be confusing to verifiers. ## What was done? Select a signing quorum first and then calculate `cycleHash` based on the quorum's base block index instead of using the current chain height. ## How Has This Been Tested? Running a testnet MN for some time. It's creating new `isdlock`s and processing `isdlock`s from other nodes with no issues. ## Breaking Changes None, `cycleHash` is simply going to be more accurate now. ## 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 ACKs for top commit: PastaPastaPasta: utACK d150a65 knst: utACK d150a65 Tree-SHA512: ff7e8a9f37aded9ac330185a34db0eef346c6a3f4a6e22f11a6081ff074a789d260fbdf9efc1d8590d4013ecd95a273e07126dfcb5a7f6d6ebebdb45a543f053
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
3 participants
Issue being fixed or feature implemented
Currently
cycleHashis calculated based on the current chain height we are signingisdlockat. This however can result incycleHashto be a block hash of a new cycle where DKG is still in progress which can be confusing to verifiers.What was done?
Select a signing quorum first and then calculate
cycleHashbased on the quorum's base block index instead of using the current chain height.How Has This Been Tested?
Running a testnet MN for some time. It's creating new
isdlocks and processingisdlocks from other nodes with no issues.Breaking Changes
None,
cycleHashis simply going to be more accurate now.Checklist: