refactor: optimize AsyncSign by using std::move for quorum parameter #6906
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThis change refactors LLMQ quorum parameter handling by replacing pointer-based quorum parameters (CQuorumCPtr / CQuorumCPtr&) with value and reference types (CQuorum and const CQuorum&), and by moving ownership in select async paths (std::move(CQuorum)). Function signatures in CSigSharesManager, CQuorumManager, and RPC handlers were updated accordingly. All internal accesses were converted from pointer dereference syntax (quorum->) to direct member access (quorum. or quorum.qc->). Call sites were updated to pass dereferenced quorums or to move quorum objects into worker threads; control flow and business logic remain unchanged. Sequence Diagram(s)mermaid mermaid 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)
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 (4)
src/llmq/signing.cpp (1)
676-689: Quorum selection + local capture looks good; minor readability nit.The on-the-fly lambda returning CQuorumCPtr is fine and keeps scopes tight. Optional: make the type explicit (CQuorumCPtr quorum = …) for readability in this hot path.
src/llmq/quorums.h (1)
283-285: API changes align with move/reference strategy; add nodiscard to prevent ignored results.
- RequestQuorumData(const CQuorum&, uint16_t, …) and Start*Thread signatures are coherent with the move into worker lambdas.
- Recommend marking RequestQuorumData as [[nodiscard]]; its boolean is actionable and ignoring it can hide request throttling/failures.
- bool RequestQuorumData(CNode* pfrom, CConnman& connman, const CQuorum& quorum, uint16_t nDataMask, + [[nodiscard]] bool RequestQuorumData(CNode* pfrom, CConnman& connman, const CQuorum& quorum, uint16_t nDataMask, const uint256& proTxHash = uint256()) const;Also consider [[nodiscard]] for CreateSigShare in signing_shares.h (it already returns std::optional).
Also applies to: 312-317, 314-317
src/llmq/quorums.cpp (1)
676-699: Guard against modulo by zero on validMembers.size().nIndex % quorum.qc->validMembers.size() will UB if validMembers is empty (shouldn’t happen, but a defensive guard avoids rare crashes).
- return nIndex % quorum.qc->validMembers.size(); + const size_t nMembers = quorum.qc->validMembers.size(); + assert(nMembers != 0); + return nMembers ? (nIndex % nMembers) : 0;src/llmq/signing_shares.h (1)
436-440: Interfaces now mix moved shared_ptr and const references; looks coherent.
- AsyncSign(CQuorumCPtr) by value enables caller std::move and avoids extra atomic ops.
- ForceReAnnouncement/SelectMemberForRecovery taking const CQuorum& is appropriate.
Optional: mark CreateSigShare(...) [[nodiscard]] to prevent silent drops of failures.- std::optional<CSigShare> CreateSigShare(const CQuorum& quorum, const uint256& id, const uint256& msgHash) const; + [[nodiscard]] std::optional<CSigShare> CreateSigShare(const CQuorum& quorum, const uint256& id, const uint256& msgHash) const;Also applies to: 439-440, 443-444
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/llmq/quorums.cpp(10 hunks)src/llmq/quorums.h(2 hunks)src/llmq/signing.cpp(2 hunks)src/llmq/signing_shares.cpp(11 hunks)src/llmq/signing_shares.h(2 hunks)src/rpc/quorums.cpp(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/llmq/signing_shares.hsrc/llmq/signing.cppsrc/rpc/quorums.cppsrc/llmq/quorums.cppsrc/llmq/quorums.hsrc/llmq/signing_shares.cpp
🧬 Code graph analysis (4)
src/llmq/signing_shares.h (1)
src/llmq/signing_shares.cpp (12)
AsyncSign(1510-1514)AsyncSign(1510-1510)CreateSigShare(1540-1610)CreateSigShare(1540-1540)ForceReAnnouncement(1613-1637)ForceReAnnouncement(1613-1613)HandleNewRecoveredSig(1639-1644)HandleNewRecoveredSig(1639-1639)SelectMemberForRecovery(846-859)SelectMemberForRecovery(846-846)TryRecoverSig(764-844)TryRecoverSig(764-765)
src/rpc/quorums.cpp (1)
src/llmq/quorums.h (1)
nDataMask(102-102)
src/llmq/quorums.h (1)
src/llmq/quorums.cpp (8)
RequestQuorumData(478-519)RequestQuorumData(478-479)GetQuorumRecoveryStartOffset(676-699)GetQuorumRecoveryStartOffset(676-676)StartCachePopulatorThread(886-914)StartCachePopulatorThread(886-886)StartQuorumDataRecoveryThread(916-1037)StartQuorumDataRecoveryThread(916-917)
src/llmq/signing_shares.cpp (2)
src/llmq/signhash.cpp (1)
SignHash(14-22)src/llmq/signhash.h (1)
SignHash(24-46)
🪛 GitHub Actions: Clang Diff Format Check
src/llmq/signing_shares.h
[error] 1-1: Clang-format differences detected in src/llmq/signing_shares.h. Run the formatter to fix formatting mismatches.
src/rpc/quorums.cpp
[error] 1-1: Clang-format differences detected in src/rpc/quorums.cpp. Run the formatter to fix formatting mismatches.
src/llmq/quorums.cpp
[error] 1-1: Clang-format differences detected in src/llmq/quorums.cpp. Run the formatter to fix formatting mismatches.
src/llmq/signing_shares.cpp
[error] 1-1: Clang-format differences detected in src/llmq/signing_shares.cpp. Run the formatter to fix formatting mismatches.
⏰ 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). (10)
- GitHub Check: mac-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (10)
src/llmq/signing.cpp (1)
736-739: Correct: avoid extra atomic ops by moving the shared_ptr into AsyncSign.Passing a dereferenced quorum to ForceReAnnouncement and then moving the shared_ptr into AsyncSign is the right order and avoids post-move use. Nice micro‑opt.
Please double‑check no future edits access “quorum” after Line 739; a quick grep will prevent accidental use‑after‑move regressions.
src/rpc/quorums.cpp (2)
182-233: Consistent shift to reference semantics for CQuorum across RPC paths.
- BuildQuorumInfo now takes const CQuorum&; member access updates are correct.
- Callers dereference shared_ptrs appropriately: quorum_info, quorum_memberof, quorum_sign_helper, quorum_selectquorum, quorum_getdata.
All good and lifetime-safe since sources are CQuorumCPtr held locally.Given public RPC exposure, please smoke‑test:
- quorum info: with includeSkShare=false/true (if this node is a member).
- quorum sign … submit=false path (CreateSigShare).
- quorum getdata with both QUORUM_VERIFICATION_VECTOR and ENCRYPTED_CONTRIBUTIONS masks.
Also applies to: 268-269, 426-429, 476-477, 745-747, 833-833
1-1: Verify formatting changes against CI results.The sandbox environment does not have clang-format available to verify the formatting diffs. While the file exists and a
.clang-formatconfiguration is present, I cannot execute the formatter to confirm the claimed violations. Please runclang-format-16 -i src/rpc/quorums.cpplocally and review thegit diffoutput, or check the CI results to confirm the exact formatting issues before applying fixes.src/llmq/quorums.cpp (3)
478-519: RequestQuorumData refactor: parameter update is correct.Switching to const CQuorum& and building the request with the new nDataMask parameter looks good. Logging and request de‑dupe remain intact.
886-914: Good use of move-capture for pQuorum into worker lambdas.Moving CQuorumCPtr into StartCachePopulatorThread/StartQuorumDataRecoveryThread tasks eliminates extra atomic ops and lifetime issues. Access is pointer‑stable in the closures.
Please sanity‑run a node with -watchquorums and ensure recovery logs (“Start/Requested/Processed/Done”) appear as expected on rotation.
Also applies to: 916-937, 1009-1011
1-1: Run clang-format locally to fix formatting issues.Formatting issues were detected in src/llmq/quorums.cpp. Evidence of trailing whitespace was found during inspection. Since clang-format is unavailable in the verification environment, please run the formatter locally and verify the changes:
clang-format -i src/llmq/quorums.cpp git diff -- src/llmq/quorums.cppCommit the formatting changes to resolve the CI check.
src/llmq/signing_shares.cpp (3)
1510-1514: AsyncSign path: correct move into queue; CreateSigShare callsite aligns with refactor.
- Moving CQuorumCPtr into pendingSigns avoids extra refcount traffic.
- Dereferencing for CreateSigShare is consistent with the new signature.
Also applies to: 1521-1537
1540-1610: Refactor to const CQuorum& in signing logic is sound.Member access changes (params, qc, members) are correct; TryRecoverSig and ForceReAnnouncement build SignHash with dereferenced quorum consistently. Single‑node quorum handling remains intact.
Please verify recovery still triggers at threshold by:
- forcing a session to reach params.threshold and observing a recovered signature in logs.
Also applies to: 1613-1637, 760-767, 776-828, 801-810, 827-835
1-1: Manual verification required for clang-format diffs.The clang-format tool is not available in the sandbox environment to verify the formatting issues. While manual inspection of src/llmq/signing_shares.cpp shows the copyright header and includes are clean (no trailing whitespace detected), I cannot confirm what clang-format diffs CI originally detected. Please run
clang-format -i src/llmq/signing_shares.cpplocally and verify the file passes CI formatting checks.src/llmq/signing_shares.h (1)
1-1: Unable to verify clang-format violations in the sandbox (clang-format tool not available). The file inspection shows proper Unix line endings and no visible trailing whitespace on the copyright header. However, without executing clang-format locally, the actual formatting violations cannot be confirmed.Please run
clang-format -i src/llmq/signing_shares.handgit diff -- src/llmq/signing_shares.hlocally to verify whether formatting changes are actually needed.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
-
src/llmq/quorums.cpp(11 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/llmq/quorums.cpp
🪛 GitHub Actions: Clang Diff Format Check
src/llmq/quorums.cpp
[error] 1-1: Clang format differences found. Command failed: git diff -U0 origin/develop --
⏰ 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). (10)
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (5)
src/llmq/quorums.cpp (5)
478-494: Excellent refactor to eliminate unnecessary reference counting.Changing the parameter from
CQuorumCPtr(shared pointer) toconst CQuorum&(reference) is a good optimization since this function only reads from the quorum and doesn't need ownership semantics. This avoids atomic increment/decrement operations on the control block.
676-698: Good refactor to use const reference.Similar to
RequestQuorumData, this change eliminates unnecessary shared_ptr operations for a read-only function.
886-913: LGTM! This achieves the move optimization for async tasks.The changes here correctly implement the optimization described in the PR:
- Taking
CQuorumCPtrby value (line 886) enables the caller to move in- Capturing by move in the lambda (line 899) transfers ownership to the async task
- This avoids the atomic increment/decrement pair that would occur with const reference + copy
916-1037: LGTM! Consistent move optimization for async recovery.The changes mirror the pattern in
StartCachePopulatorThread:
- Pass-by-value parameter enables move semantics (line 916-917)
- Lambda move capture transfers ownership (line 927)
- Dereferences on lines 933 and 1009 are consistent with updated function signatures
The
&connmancapture by reference is safe since the worker pool is stopped in the destructor.
1-1: Address clang-format issues.The pipeline is reporting clang-format differences. Please run the clang-format tool to fix formatting issues before merging.
PastaPastaPasta
force-pushed
the
perf/std-move-quorum
branch
from
f075a12 to
0716c7a
Compare
|
This pull request has conflicts, please rebase. |
PastaPastaPasta
force-pushed
the
perf/std-move-quorum
branch
from
0716c7a to
221aa27
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/llmq/quorums.cpp (1)
1-1: Verify clang-format compliance.The pipeline indicates clang-format differences in this file. Please run
clang-format-diff.pyorclang-formatto fix formatting issues.src/rpc/quorums.cpp (1)
1-1: Verify clang-format compliance.The pipeline indicates clang-format differences in this file. Please run
clang-format-diff.pyorclang-formatto fix formatting issues.src/llmq/signing_shares.cpp (1)
1-1: Verify clang-format compliance.The pipeline indicates clang-format differences in this file. Please run
clang-format-diff.pyorclang-formatto fix formatting issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/llmq/quorums.cpp(12 hunks)src/llmq/quorums.h(2 hunks)src/llmq/signing_shares.cpp(13 hunks)src/llmq/signing_shares.h(2 hunks)src/rpc/quorums.cpp(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/rpc/quorums.cppsrc/llmq/signing_shares.hsrc/llmq/signing_shares.cppsrc/llmq/quorums.hsrc/llmq/quorums.cpp
🧬 Code graph analysis (4)
src/rpc/quorums.cpp (1)
src/llmq/signing_shares.cpp (2)
SelectMemberForRecovery(856-869)SelectMemberForRecovery(856-856)
src/llmq/signing_shares.h (1)
src/llmq/signing_shares.cpp (12)
AsyncSign(1602-1606)AsyncSign(1602-1602)CreateSigShare(1632-1700)CreateSigShare(1632-1632)ForceReAnnouncement(1703-1727)ForceReAnnouncement(1703-1703)HandleNewRecoveredSig(1729-1734)HandleNewRecoveredSig(1729-1729)SelectMemberForRecovery(856-869)SelectMemberForRecovery(856-856)TryRecoverSig(775-854)TryRecoverSig(775-775)
src/llmq/signing_shares.cpp (3)
src/llmq/signhash.cpp (1)
SignHash(14-22)src/llmq/signhash.h (1)
SignHash(24-46)src/llmq/signing_shares.h (1)
attempt(359-359)
src/llmq/quorums.h (1)
src/llmq/quorums.cpp (8)
RequestQuorumData(478-519)RequestQuorumData(478-479)GetQuorumRecoveryStartOffset(676-699)GetQuorumRecoveryStartOffset(676-676)StartCachePopulatorThread(886-914)StartCachePopulatorThread(886-886)StartQuorumDataRecoveryThread(916-1037)StartQuorumDataRecoveryThread(916-917)
🪛 GitHub Actions: Clang Diff Format Check
src/rpc/quorums.cpp
[error] 1-1: Clang-format differences found in src/rpc/quorums.cpp. Run clang-format-diff.py or clang-format to fix formatting.
src/llmq/signing_shares.h
[error] 1-1: Clang-format differences found in src/llmq/signing_shares.h. Run clang-format-diff.py or clang-format to fix formatting.
src/llmq/signing_shares.cpp
[error] 1-1: Clang-format differences found in src/llmq/signing_shares.cpp. Run clang-format-diff.py or clang-format to fix formatting.
src/llmq/quorums.cpp
[error] 1-1: Clang-format differences found in src/llmq/quorums.cpp. Run clang-format-diff.py or clang-format to fix formatting.
⏰ 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). (10)
- GitHub Check: arm-linux-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (13)
src/llmq/quorums.cpp (4)
260-260: LGTM! Move optimization properly implemented.The removal of
constfromvecQuorums(line 260) and the use of mutable reference in the loop (line 269) correctly enable the move optimization when callingStartQuorumDataRecoveryThread(connman, std::move(pQuorum), ...)at line 296. This avoids unnecessary shared_ptr atomic operations.Also applies to: 269-269, 296-296
478-519: LGTM! Signature change correctly implemented.The change from
CQuorumCPtr pQuorumtoconst CQuorum& quorumis correct for this read-only method. All accesses have been properly updated from pointer dereference (pQuorum->) to direct member access (quorum.).
886-914: LGTM! Move semantics correctly implemented in lambda.The signature change to
CQuorumCPtr pQuorum(removing const) at line 886 and the move into the lambda at line 899 (pQuorum = std::move(pQuorum)) correctly transfer ownership to the worker thread, avoiding unnecessary reference count operations.
916-1037: LGTM! Ownership transfer correctly implemented.The signature change to
CQuorumCPtr pQuorumat line 916 and the move into the lambda at line 927 properly transfer ownership. The dereferencing at line 1009 (RequestQuorumData(pNode, connman, *pQuorum, ...)) is correct since RequestQuorumData now expects a const reference.src/llmq/quorums.h (1)
283-284: LGTM! Signature changes align with implementation.The updated signatures correctly reflect the ownership semantics:
RequestQuorumDataandGetQuorumRecoveryStartOffsettakeconst CQuorum&for read-only accessStartCachePopulatorThreadandStartQuorumDataRecoveryThreadtakeCQuorumCPtrby value (not const) to enable move semantics in their implementationsAlso applies to: 312-316
src/rpc/quorums.cpp (2)
183-235: LGTM! Signature and access patterns correctly updated.The change from
llmq::CQuorumCPtrtoconst llmq::CQuorum& quorumis correct. All member accesses have been properly updated from pointer syntax (quorum->) to direct member access (quorum.), and nested accesses likequorum.qc->quorumHashare correctly formed.
274-274: LGTM! Call sites correctly dereference quorums.All call sites to
BuildQuorumInfo,CreateSigShare,SelectMemberForRecovery, andRequestQuorumDataproperly dereference theCQuorumCPtrto pass aconst CQuorum&as expected by the updated signatures.Also applies to: 470-470, 524-524, 818-818, 905-905
src/llmq/signing_shares.cpp (6)
1602-1606: LGTM! Move optimization correctly implemented.The signature change from
const CQuorumCPtr&toCQuorumCPtr quorum(by value) combined withstd::moveat line 1605 achieves the stated optimization goal. This avoids an extra shared_ptr copy when the caller passes ownership viastd::move, eliminating unnecessary atomic operations.
879-949: LGTM! Proper move semantics at call site.The call to
AsyncSign(std::move(quorum), id, msgHash)at line 946 correctly transfers ownership, avoiding any shared_ptr copy. The quorum is no longer used after this point, making the move safe and optimal.
775-854: LGTM! Signature change and access patterns correctly updated.The change from
const CQuorumCPtr&toconst CQuorum& quorumis correct for this read-only method. All accesses have been properly updated:
quorum->params→quorum.paramsquorum->qc->quorumHash→quorum.qc->quorumHashquorum->members→quorum.membersThe dereferencing at the call site (line 771:
TryRecoverSig(*quorum, ...)) is appropriate.
856-869: LGTM! SelectMemberForRecovery correctly updated.The signature change to
const CQuorum& quorumand updated access patterns are correct. The method properly uses direct member access (quorum.params,quorum.members), and call sites correctly dereference when needed.
1632-1700: LGTM! CreateSigShare correctly refactored.The signature change to
const CQuorum& quorumis appropriate for this read-only method. All member accesses have been correctly updated throughout the function, and the logic remains unchanged.
1703-1727: LGTM! ForceReAnnouncement correctly updated.The signature change to
const CQuorum& quorumand the access pattern update at line 1710 (quorum.qc->quorumHash) are correct for this read-only method.
| void AsyncSign(CQuorumCPtr quorum, const uint256& id, const uint256& msgHash) | ||
| EXCLUSIVE_LOCKS_REQUIRED(!cs_pendingSigns); | ||
| std::optional<CSigShare> CreateSigShare(const CQuorumCPtr& quorum, const uint256& id, const uint256& msgHash) const; | ||
| void ForceReAnnouncement(const CQuorumCPtr& quorum, Consensus::LLMQType llmqType, const uint256& id, const uint256& msgHash); | ||
| std::optional<CSigShare> CreateSigShare(const CQuorum& quorum, const uint256& id, const uint256& msgHash) const; | ||
| void ForceReAnnouncement(const CQuorum& quorum, Consensus::LLMQType llmqType, const uint256& id, const uint256& msgHash); |
There was a problem hiding this comment.
Verify clang-format compliance.
The pipeline indicates clang-format differences in this file. Please run clang-format-diff.py or clang-format to fix formatting issues.
Also applies to: 441-441, 472-472
🤖 Prompt for AI Agents
In src/llmq/signing_shares.h around lines 434-437 (also address formatting at
lines 441 and 472), the function declarations are not clang-format compliant;
run clang-format (or clang-format-diff.py) on this file and reformat these lines
so spacing, indentation, and line breaks match the project's clang-format style;
commit the formatted file so the pipeline no longer reports clang-format
differences.
|
This pull request has conflicts, please rebase. |
…o use CQuorum reference
PastaPastaPasta
force-pushed
the
perf/std-move-quorum
branch
from
221aa27 to
6b49730
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/llmq/quorums.cpp (1)
943-950: Bug: fQuorumDataRecoveryThreadRunning not reset on abort pathIf quorumThreadInterrupt is set before sync, the lambda returns without clearing pQuorum->fQuorumDataRecoveryThreadRunning, permanently preventing future recovery threads for this quorum.
Apply one of the fixes below.
Minimal fix:
- if (quorumThreadInterrupt) { - printLog("Aborted"); - return; - } + if (quorumThreadInterrupt) { + printLog("Aborted"); + pQuorum->fQuorumDataRecoveryThreadRunning = false; + return; + }RAII (preferred) to cover all early returns:
@@ - workerPool.push([&connman, pQuorum = std::move(pQuorum), pIndex, nDataMaskIn, this](int threadId) { + workerPool.push([&connman, pQuorum = std::move(pQuorum), pIndex, nDataMaskIn, this](int threadId) { + struct FlagGuard { + std::atomic<bool>& flag; + ~FlagGuard() { flag = false; } + } guard{pQuorum->fQuorumDataRecoveryThreadRunning}; @@ - pQuorum->fQuorumDataRecoveryThreadRunning = false; - printLog("Done"); + printLog("Done"); });This ensures the flag is cleared on all exit paths.
♻️ Duplicate comments (1)
src/llmq/signing_shares.h (1)
434-437: Verify clang-format compliance (duplicate of prior feedback)CI flags clang-format differences in this header. Please re-run the formatter.
#!/bin/bash git diff -U0 origin/develop -- src/llmq/signing_shares.h | ./contrib/devtools/clang-format-diff.py -p1 > diff_output.txt wc -c diff_output.txt
🧹 Nitpick comments (2)
src/llmq/quorums.h (1)
312-316: Take CQuorumCPtr by value to enable move; consider documenting move semanticsSignatures now allow std::move into worker lambdas. Add a brief comment noting that ownership is intentionally transferred to async tasks.
src/llmq/quorums.cpp (1)
676-699: Guard modulo by non-empty validMembersvalidMembers should always match the quorum size, but a defensive assert would document the assumption:
size_t CQuorumManager::GetQuorumRecoveryStartOffset(const CQuorum& quorum, const CBlockIndex* pIndex) const { assert(m_mn_activeman); + assert(!quorum.qc->validMembers.empty()); @@ - return nIndex % quorum.qc->validMembers.size(); + return nIndex % quorum.qc->validMembers.size(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
-
src/llmq/quorums.cpp(12 hunks) -
src/llmq/quorums.h(2 hunks) -
src/llmq/signing_shares.cpp(13 hunks) -
src/llmq/signing_shares.h(2 hunks) -
src/rpc/quorums.cpp(8 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/rpc/quorums.cppsrc/llmq/quorums.hsrc/llmq/quorums.cppsrc/llmq/signing_shares.hsrc/llmq/signing_shares.cpp
🧬 Code graph analysis (4)
src/rpc/quorums.cpp (1)
src/llmq/signing_shares.cpp (2)
SelectMemberForRecovery(856-869)SelectMemberForRecovery(856-856)
src/llmq/quorums.h (1)
src/llmq/quorums.cpp (8)
RequestQuorumData(478-519)RequestQuorumData(478-479)GetQuorumRecoveryStartOffset(676-699)GetQuorumRecoveryStartOffset(676-676)StartCachePopulatorThread(886-914)StartCachePopulatorThread(886-886)StartQuorumDataRecoveryThread(916-1037)StartQuorumDataRecoveryThread(916-917)
src/llmq/signing_shares.h (1)
src/llmq/signing_shares.cpp (10)
AsyncSign(1602-1606)AsyncSign(1602-1602)CreateSigShare(1632-1700)CreateSigShare(1632-1632)ForceReAnnouncement(1703-1727)ForceReAnnouncement(1703-1703)SelectMemberForRecovery(856-869)SelectMemberForRecovery(856-856)TryRecoverSig(775-854)TryRecoverSig(775-775)
src/llmq/signing_shares.cpp (2)
src/llmq/signhash.cpp (1)
SignHash(14-22)src/llmq/signhash.h (1)
SignHash(24-46)
🪛 GitHub Actions: Clang Diff Format Check
src/rpc/quorums.cpp
[error] 1-1: Clang format differences found. Run the clang-format-diff.py tool (as shown in logs) to fix formatting in this file. Command that produced the diff: git diff -U0 origin/develop --
src/llmq/quorums.cpp
[error] 1-1: Clang format differences found. Run the clang-format-diff.py tool (as shown in logs) to fix formatting in this file. Command that produced the diff: git diff -U0 origin/develop --
src/llmq/signing_shares.h
[error] 1-1: Clang format differences found. Run the clang-format-diff.py tool (as shown in logs) to fix formatting in this file. Command that produced the diff: git diff -U0 origin/develop --
src/llmq/signing_shares.cpp
[error] 1-1: Clang format differences found. Run the clang-format-diff.py tool (as shown in logs) to fix formatting in this file. Command that produced the diff: git diff -U0 origin/develop --
⏰ 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). (9)
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (11)
src/llmq/quorums.h (1)
283-285: API shift to const CQuorum& is goodPassing a const reference avoids shared_ptr inc/dec at call sites and makes ownership explicit. Callers updated elsewhere look consistent.
src/rpc/quorums.cpp (3)
182-235: BuildQuorumInfo: switch to const CQuorum& looks correctDirect field access (quorum.params, quorum.qc, quorum.members) is consistent with the type change. No behavior change.
274-274: Call sites updated to pass dereferenced quorumUsing *quorum for CreateSigShare/BuildQuorumInfo/RequestQuorumData aligns with new APIs. Good.
Also applies to: 470-471, 818-819, 905-906
1-1: Run clang-format locally and commit any formatting changesThe sandbox environment lacks clang-format and Python 3, preventing verification of formatting diffs here. Since CI has already reported formatting differences for this file, verify and fix them in your local environment:
- Run your project's clang-format step locally
- Commit any resulting changes to
src/rpc/quorums.cppsrc/llmq/quorums.cpp (3)
259-297: Enabling move: good use of non-const container and std::moveDropping const on vecQuorums and moving pQuorum into StartQuorumDataRecoveryThread avoids extra atomic inc/dec on shared_ptr. Correctly checks the running flag before moving.
If any downstream uses of vecQuorums assume non-null entries post-loop, they could encounter nullptr after move. A quick check shows no reuse here, but please confirm no later use in this scope.
886-914: Move-capture into worker lambdas is correctCapturing pQuorum by move in both cache populator and data recovery threads achieves the intended shared_ptr move semantics. Logging and access patterns remain unchanged.
Also applies to: 916-937, 1009-1016
1-1: The review comment is based on an incorrect premise and should be ignored.The git diff reveals semantic code changes (const qualifiers, move semantics, parameter type changes, dereferencing), not formatting issues. Running clang-format would not produce or address these changes—clang-format only handles whitespace and style. The actual modifications appear to be intentional improvements to move semantics and parameter passing.
Likely an incorrect or invalid review comment.
src/llmq/signing_shares.cpp (3)
1602-1606: AsyncSign now takes CQuorumCPtr by value and moves into queueThis removes refcount churn on enqueue; pendingSigns stores the moved shared_ptr. Good.
770-776: Ref semantics across signing path look consistent
- TryRecoverSig/CreateSigShare/ForceReAnnouncement now take const CQuorum&, with call sites updated (e.g., TryRecoverSig(*quorum), CreateSigShare(*pQuorum)).
- Recovery member selection updated to use reference type.
All consistent with the header changes.Also applies to: 786-787, 793-821, 837-845, 1632-1699, 1703-1727, 1131-1143
1-1: Unable to verify clang-format compliance in sandbox environmentThe verification script's Python 3 dependency and unavailable clang-format tool prevent definitive detection of formatting issues. The empty diff output suggests no issues, but this is inconclusive. Please manually run
clang-formatonsrc/llmq/signing_shares.cpplocally to verify formatting compliance before committing.src/llmq/signing_shares.h (1)
434-437: Verification confirms all signatures and call sites are alignedAsyncSign, CreateSigShare, and TryRecoverSig all have matching definitions and properly updated call sites. AsyncSign correctly uses std::move() for the by-value CQuorumCPtr parameter, and the const CQuorum& methods dereference pointer arguments appropriately. No inconsistencies detected.
2 participants
Issue being fixed or feature implemented
Using std::move here should save on the magnitude of ~10-150 ns depending on how contended the underlying shared_ptr is. This move becomes basically free, with no atomics etc being touched. Currently, we pass a const ref in, then copy it, resulting in 2 unneeded atomic operations (++ for the copy, -- for the destroy of the one we could've just moved in.
What was done?
use std::move
Also; prefer just using the reference to CQuorum where possible, as opposed to the shared_ptr
How Has This Been Tested?
compiles
Breaking Changes
Please describe any breaking changes your code introduces
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.