refactor: segregate CreateSigShare for single member handling #7003
Conversation
|
WalkthroughThis change extracts single-member quorum handling into a new private method Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant CSigSharesManager
participant Quorum
participant KeyStore
participant Crypto
Caller->>CSigSharesManager: CreateSigShare(quorum, id, msgHash)
alt quorum has single member
CSigSharesManager->>CSigSharesManager: CreateSigShareForSingleMember(quorum, id, msgHash)
CSigSharesManager->>Quorum: determineMemberIndex(msgHash)
Quorum-->>CSigSharesManager: memberIndex
CSigSharesManager->>CSigSharesManager: construct CSigShare(memberIndex, id, msgHash)
CSigSharesManager->>KeyStore: getOperatorKey()
KeyStore-->>CSigSharesManager: operatorKey
CSigSharesManager->>Crypto: sign(operatorKey, sigShareData)
Crypto-->>CSigSharesManager: signature
CSigSharesManager->>Crypto: verify(signature, publicKey, sigShareData)
Crypto-->>CSigSharesManager: verificationResult
alt verificationResult == valid
CSigSharesManager->>CSigSharesManager: update share key, log success
CSigSharesManager-->>Caller: optional<CSigShare> (value)
else verificationResult == invalid
CSigSharesManager->>CSigSharesManager: log failure
CSigSharesManager-->>Caller: optional<CSigShare> (nullopt)
end
else quorum has multiple members
CSigSharesManager->>CSigSharesManager: existing multi-member signing flow (unchanged)
CSigSharesManager-->>Caller: optional<CSigShare> (value/nullopt)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-08-19T15:08:00.835ZApplied to files:
📚 Learning: 2025-07-15T14:53:04.819ZApplied to files:
🪛 GitHub Actions: Clang Diff Format Checksrc/llmq/signing_shares.cpp[error] 1-1: Clang format differences found in source file. Apply formatting changes (lint: clang-format). ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
🔇 Additional comments (3)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 0
🧹 Nitpick comments (1)
src/llmq/signing_shares.cpp (1)
1685-1726: LGTM! Delegation to single-member handler is correct.The refactoring successfully separates the single-member logic while preserving the existing multi-member implementation. The validation check (
IsValidMember) appropriately occurs before delegation.Optional: Minor efficiency improvement.
For the single-member path, both
activeMasterNodeProTxHash(line 1688) and the timer (line 1687) are retrieved in this function but then retrieved again insideCreateSigShareForSingleMember. Consider moving the single-member check earlier to avoid this duplication:std::optional<CSigShare> CSigSharesManager::CreateSigShare(const CQuorum& quorum, const uint256& id, const uint256& msgHash) const { - cxxtimer::Timer t(true); - auto activeMasterNodeProTxHash = m_mn_activeman.GetProTxHash(); - - if (!quorum.IsValidMember(activeMasterNodeProTxHash)) { + if (!quorum.IsValidMember(m_mn_activeman.GetProTxHash())) { return std::nullopt; } if (quorum.params.is_single_member()) { return CreateSigShareForSingleMember(quorum, id, msgHash); } + + cxxtimer::Timer t(true); + auto activeMasterNodeProTxHash = m_mn_activeman.GetProTxHash();However, this is a minor optimization and can be deferred, especially given this is a focused refactoring PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/llmq/signing_shares.cpp(1 hunks)src/llmq/signing_shares.h(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
🧬 Code graph analysis (1)
src/llmq/signing_shares.h (1)
src/llmq/signing_shares.cpp (2)
CreateSigShareForSingleMember(1648-1683)CreateSigShareForSingleMember(1648-1648)
🔇 Additional comments (2)
src/llmq/signing_shares.h (1)
455-456: LGTM! Clean refactoring to separate single-member logic.The new private helper declaration is well-structured and clearly communicates its purpose. The signature is consistent with the existing
CreateSigSharemethod, and the const-correctness is properly maintained.src/llmq/signing_shares.cpp (1)
1648-1683: LGTM! Single-member logic successfully extracted.The implementation correctly handles single-member quorum signature creation with appropriate error handling and logging. The TODO comment about using the QUORUM key instead of the OPERATOR key is preserved, maintaining awareness of the future improvement.
Co-authored-by: Konstantin Akimov <[email protected]>
3 participants
Issue being fixed or feature implemented
I keep reading CSigSharesManager::CreateSigShare looking for optimizations, and keep getting distracted by single member logic; let's move it into it's own function
What was done?
How Has This Been Tested?
Builds
Breaking Changes
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.