refactor: avoid return by reference and prefer return by value #6955
Conversation
…e methods Updated methods in the CRecoveredSigsDb and CSigningManager classes to return std::optional types instead of using output parameters. This change simplifies error handling and improves code readability. Adjusted related logic in net_processing, signing, and RPC layers to accommodate the new return types.
…trieval Refactored several methods in the DKG session and block processor classes to return std::optional types instead of using output parameters. This change enhances error handling and code clarity. Adjusted related logic in net processing and chain lock handling to accommodate the new return types.
Updated methods in CSigSharesNodeState and CSigSharesManager to return std::optional types instead of using output parameters for session information retrieval. This refactor enhances error handling and improves code clarity, aligning with recent changes in the codebase.
|
WalkthroughThis pull request converts numerous getter APIs from a bool + out-parameter pattern to returning std::optional across multiple subsystems (chainlock, instantsend, LLMQ signing, recovered sigs DB, DKG session/manager/handler, signing shares, blockprocessor). It extracts several DKG-related types from llmq/dkgsession.h into a new llmq/dkgtypes.h and exposes that header via src/Makefile.am. Masternode GetLocalAddress was changed to return std::optional with additional address-finding fallbacks. net_processing GETDATA handlers and RPC quorums were updated to consume optionals. Sequence Diagram(s)sequenceDiagram
participant Caller
participant OldGet as GetXxx (old)
participant NewGet as GetXxx (new)
rect rgb(245, 245, 255)
Note over Caller,OldGet: Old pattern — bool + out param
Caller->>OldGet: GetXxx(hash, outParam)
alt Found
OldGet-->>Caller: true (outParam filled)
Note over Caller: check bool, use outParam
else Not found
OldGet-->>Caller: false
Note over Caller: skip / treat missing
end
end
rect rgb(245, 255, 245)
Note over Caller,NewGet: New pattern — std::optional<T>
Caller->>NewGet: GetXxx(hash)
alt Found
NewGet-->>Caller: optional<T> (has value)
Note over Caller: dereference optional and use value
else Not found
NewGet-->>Caller: std::nullopt
Note over Caller: handle absence
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/masternode/node.cpp (1)
239-260: Preserve the failure reason when GetLocalAddress() returns nulloptWith connected peers but no IPv4 candidate, we now return
std::nulloptwithout touchingm_error.InitInternal()then switches toSOME_ERROR, yetGetStatus()shows “Error. ” with an empty reason—this is a regression from the previous bool/out-param version, which populated the message before returning false. Please setm_error(and log once) right before the finalreturn std::nullopt;so operators keep the actionable guidance.- if (empty) { - m_error = "Can't detect valid external address. Please consider using the externalip configuration option if problem persists. Make sure to use IPv4 address only."; - LogPrintf("CActiveMasternodeManager::GetLocalAddress -- ERROR: %s\n", m_error); - return std::nullopt; - } + if (empty) { + m_error = "Can't detect valid external address. Please consider using the externalip configuration option if problem persists. Make sure to use IPv4 address only."; + LogPrintf("CActiveMasternodeManager::GetLocalAddress -- ERROR: %s\n", m_error); + } } if (!fFoundLocal) { + if (m_error.empty()) { + m_error = "Can't detect valid external address. Please consider using the externalip configuration option if problem persists. Make sure to use IPv4 address only."; + LogPrintf("CActiveMasternodeManager::GetLocalAddress -- ERROR: %s\n", m_error); + } return std::nullopt; }
🧹 Nitpick comments (2)
src/net_processing.cpp (1)
2825-2827: Optional-return usage is consistent; consider moving payloads to avoid copies.Dereferencing the optionals passes lvalues, which may copy sizable structs. Moving the contained value can hint rvalue usage during serialization. Optional change, keep scope focused.
Apply move on the contained value (repeat for similar lines):
- m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::QFCOMMITMENT, *opt_commitment)); + m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::QFCOMMITMENT, std::move(*opt_commitment))); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::QCONTRIB, *opt_contrib)); + m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::QCONTRIB, std::move(*opt_contrib))); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::QCOMPLAINT, *opt_complaint)); + m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::QCOMPLAINT, std::move(*opt_complaint))); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::QJUSTIFICATION, *opt_justification)); + m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::QJUSTIFICATION, std::move(*opt_justification))); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::QPCOMMITMENT, *opt_premature)); + m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::QPCOMMITMENT, std::move(*opt_premature))); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::QSIGREC, *opt_rec_sig)); + m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::QSIGREC, std::move(*opt_rec_sig))); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::CLSIG, *opt_clsig)); + m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::CLSIG, std::move(*opt_clsig))); - m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::ISDLOCK, *opt_islock)); + m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::ISDLOCK, std::move(*opt_islock)));Optional follow-up: a tiny helper to DRY this pattern can be introduced later. Based on learnings.
Also applies to: 2832-2834, 2839-2841, 2846-2848, 2853-2855, 2860-2862, 2867-2869, 2874-2876
src/chainlock/chainlock.h (1)
94-94: Add [[nodiscard]] to GetChainLockByHash; remove noexcept suggestion.The [[nodiscard]] attribute is a good polish to prevent ignored optional results. However,
noexceptis incorrect here—the function acquires a lock viaLOCK(cs)(src/sync.h:347), which usesDebugLockand can throw on lock contention or failure. Additionally,ChainLockSigcopy construction is not marked noexcept, so the function's return operation may also throw.Apply only the [[nodiscard]] part:
- std::optional<chainlock::ChainLockSig> GetChainLockByHash(const uint256& hash) const EXCLUSIVE_LOCKS_REQUIRED(!cs); + [[nodiscard]] std::optional<chainlock::ChainLockSig> GetChainLockByHash(const uint256& hash) const EXCLUSIVE_LOCKS_REQUIRED(!cs);Verification confirmed no legacy two-argument call sites exist; only one compatible call site found in src/net_processing.cpp:2867.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
src/Makefile.am(1 hunks)src/chainlock/chainlock.cpp(1 hunks)src/chainlock/chainlock.h(2 hunks)src/instantsend/instantsend.cpp(2 hunks)src/instantsend/instantsend.h(2 hunks)src/instantsend/signing.cpp(1 hunks)src/llmq/blockprocessor.cpp(1 hunks)src/llmq/blockprocessor.h(1 hunks)src/llmq/dkgsession.h(1 hunks)src/llmq/dkgsessionhandler.cpp(1 hunks)src/llmq/dkgsessionhandler.h(2 hunks)src/llmq/dkgsessionmgr.cpp(1 hunks)src/llmq/dkgsessionmgr.h(2 hunks)src/llmq/signing.cpp(8 hunks)src/llmq/signing.h(4 hunks)src/llmq/signing_shares.cpp(4 hunks)src/llmq/signing_shares.h(2 hunks)src/masternode/node.cpp(3 hunks)src/masternode/node.h(2 hunks)src/net_processing.cpp(1 hunks)src/rpc/quorums.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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/blockprocessor.hsrc/chainlock/chainlock.hsrc/llmq/dkgsession.hsrc/llmq/signing_shares.hsrc/masternode/node.hsrc/net_processing.cppsrc/llmq/dkgsessionhandler.cppsrc/instantsend/instantsend.hsrc/chainlock/chainlock.cppsrc/llmq/dkgsessionhandler.hsrc/instantsend/signing.cppsrc/rpc/quorums.cppsrc/llmq/dkgsessionmgr.cppsrc/llmq/blockprocessor.cppsrc/llmq/signing.cppsrc/masternode/node.cppsrc/llmq/dkgsessionmgr.hsrc/llmq/signing.hsrc/instantsend/instantsend.cppsrc/llmq/signing_shares.cpp
src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit inference engine (CLAUDE.md)
Masternode lists must use immutable data structures (Immer library) for thread safety
Files:
src/masternode/node.hsrc/masternode/node.cpp
🧠 Learnings (7)
📓 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/wallet/wallet.cpp:0-0
Timestamp: 2025-07-29T14:33:01.040Z
Learning: In refactoring PRs like #6761, kwvg acknowledges code safety improvements (like null pointer checks and unused parameter warnings) but prefers to defer them to follow-up PRs to maintain focus on the primary refactoring objectives, avoiding scope creep.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
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.
📚 Learning: 2025-07-23T09:30:34.631Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.631Z
Learning: Dash Core uses BITCOIN_ prefix for header guards as the standard convention, inherited from Bitcoin Core. Only a few BLS-specific files in src/bls/ use DASH_ prefix. The vast majority of files (385+) use BITCOIN_ prefix.
Applied to files:
src/Makefile.am
📚 Learning: 2025-07-20T18:42:49.794Z
Learnt from: CR
Repo: dashpay/dash PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/**/*.{cpp,h,cc,cxx,hpp} : Dash uses unordered_lru_cache for efficient caching with LRU eviction
Applied to files:
src/instantsend/instantsend.h
📚 Learning: 2025-10-21T11:09:34.688Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6849
File: src/governance/governance.cpp:1339-1343
Timestamp: 2025-10-21T11:09:34.688Z
Learning: In the Dash Core codebase, `CacheMap` (defined in src/cachemap.h) is internally thread-safe and uses its own `mutable CCriticalSection cs` to protect access to its members. Methods like `GetSize()`, `Insert()`, `Get()`, `HasKey()`, etc., can be called without holding external locks.
Applied to files:
src/instantsend/instantsend.h
📚 Learning: 2025-07-29T14:32:48.369Z
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.
Applied to files:
src/chainlock/chainlock.cpp
📚 Learning: 2025-10-02T18:29:54.756Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6840
File: src/net_processing.cpp:2882-2886
Timestamp: 2025-10-02T18:29:54.756Z
Learning: Across net_processing.cpp, once LLMQContext (m_llmq_ctx) is asserted non-null, its subcomponents (e.g., isman, qdkgsman, quorum_block_processor) are treated as initialized and used without extra null checks.
Applied to files:
src/llmq/dkgsessionhandler.h
📚 Learning: 2025-05-10T00:54:15.597Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6629
File: src/evo/deterministicmns.cpp:473-485
Timestamp: 2025-05-10T00:54:15.597Z
Learning: In the current implementation of `NetInfoEntry`, only `CService` is a valid type and `std::monostate` represents an invalid state. Entries that don't provide a valid `CService` through `GetAddrPort()` should be rejected with an exception, not silently skipped.
Applied to files:
src/masternode/node.cpp
🧬 Code graph analysis (13)
src/llmq/blockprocessor.h (1)
src/llmq/blockprocessor.cpp (2)
GetMineableCommitmentByHash(723-731)GetMineableCommitmentByHash(723-723)
src/chainlock/chainlock.h (2)
src/chainlock/clsig.h (1)
chainlock(15-38)src/chainlock/chainlock.cpp (2)
GetChainLockByHash(98-108)GetChainLockByHash(98-98)
src/llmq/signing_shares.h (1)
src/llmq/signing_shares.cpp (4)
GetSessionInfoByRecvId(155-169)GetSessionInfoByRecvId(155-155)GetSessionInfoByRecvId(1362-1366)GetSessionInfoByRecvId(1362-1362)
src/masternode/node.h (1)
src/masternode/node.cpp (2)
GetLocalAddress(218-260)GetLocalAddress(218-218)
src/net_processing.cpp (3)
src/instantsend/instantsend.cpp (1)
inv(422-422)src/coinjoin/server.cpp (1)
inv(375-375)src/llmq/dkgsession.cpp (4)
inv(301-301)inv(609-609)inv(820-820)inv(1175-1175)
src/llmq/dkgsessionhandler.cpp (1)
src/llmq/dkgsessionmgr.cpp (8)
GetContribution(206-221)GetContribution(206-206)GetComplaint(223-238)GetComplaint(223-223)GetJustification(240-255)GetJustification(240-240)GetPrematureCommitment(257-272)GetPrematureCommitment(257-257)
src/instantsend/instantsend.h (2)
src/instantsend/lock.h (1)
instantsend(18-41)src/instantsend/instantsend.cpp (2)
GetInstantSendLockByHash(840-862)GetInstantSendLockByHash(840-840)
src/chainlock/chainlock.cpp (1)
src/chainlock/chainlock.h (2)
cs(64-135)cs(65-132)
src/llmq/dkgsessionhandler.h (2)
src/llmq/dkgsessionmgr.cpp (8)
GetContribution(206-221)GetContribution(206-206)GetComplaint(223-238)GetComplaint(223-223)GetJustification(240-255)GetJustification(240-240)GetPrematureCommitment(257-272)GetPrematureCommitment(257-257)src/llmq/dkgsessionhandler.cpp (8)
GetContribution(630-638)GetContribution(630-630)GetComplaint(640-648)GetComplaint(640-640)GetJustification(650-658)GetJustification(650-650)GetPrematureCommitment(660-668)GetPrematureCommitment(660-660)
src/llmq/dkgsessionmgr.cpp (1)
src/llmq/dkgsessionhandler.cpp (8)
GetContribution(630-638)GetContribution(630-630)GetComplaint(640-648)GetComplaint(640-640)GetJustification(650-658)GetJustification(650-650)GetPrematureCommitment(660-668)GetPrematureCommitment(660-660)
src/masternode/node.cpp (2)
src/net.cpp (2)
GetLocalAddress(231-234)GetLocalAddress(231-231)src/masternode/node.h (1)
cs(48-87)
src/llmq/dkgsessionmgr.h (2)
src/llmq/dkgsessionmgr.cpp (8)
GetContribution(206-221)GetContribution(206-206)GetComplaint(223-238)GetComplaint(223-223)GetJustification(240-255)GetJustification(240-240)GetPrematureCommitment(257-272)GetPrematureCommitment(257-257)src/llmq/dkgsessionhandler.cpp (8)
GetContribution(630-638)GetContribution(630-630)GetComplaint(640-648)GetComplaint(640-640)GetJustification(650-658)GetJustification(650-650)GetPrematureCommitment(660-668)GetPrematureCommitment(660-660)
src/llmq/signing.h (1)
src/llmq/signing.cpp (18)
GetRecoveredSigByHash(118-127)GetRecoveredSigByHash(118-118)GetRecoveredSigById(129-132)GetRecoveredSigById(129-129)TruncateRecoveredSig(213-218)TruncateRecoveredSig(213-213)TruncateRecoveredSig(643-646)TruncateRecoveredSig(643-643)GetVoteForId(277-285)GetVoteForId(277-277)GetVoteForId(712-715)GetVoteForId(712-712)ReadRecoveredSig(100-116)ReadRecoveredSig(100-100)GetRecoveredSigForGetData(367-379)GetRecoveredSigForGetData(367-367)GetRecoveredSigForId(691-694)GetRecoveredSigForId(691-691)
⏰ 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: linux64_tsan-build / Build source
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (15)
src/llmq/signing_shares.h (2)
347-347: LGTM - Clean refactoring to std::optional.The method signature change from bool+out-parameter to
std::optional<SessionInfo>return is correct and aligns with the PR objectives.
475-475: LGTM - Consistent refactoring across the class.The manager-level method signature mirrors the node state method's refactoring pattern correctly.
src/llmq/signing_shares.cpp (6)
155-169: LGTM - Correct implementation of optional-return pattern.The function properly returns
std::nulloptwhen the session is not found and returns a populatedSessionInfoby value otherwise. The move semantics will handle this efficiently.
355-388: LGTM - Proper handling of optional return value.The code correctly uses the if-initializer pattern to check the optional and safely dereferences
sessionInfowith the->operator throughout. The separation between gettingSessionInfofor validation and retrieving the actualSession*to modify it is appropriate sinceSessionInfois a lightweight copy.
392-416: LGTM - Consistent with other callsites.The optional handling follows the same correct pattern as
ProcessMessageSigSharesInv.
420-469: LGTM - Correct handling with both reference and pointer access.The code properly dereferences the optional with
*sessionInfoon line 421 to pass by const reference, and uses the->operator for member access throughout. All usage is safe within the if-block scope.
906-925: LGTM - Proper optional value checks and dereferencing.The code consistently checks
has_value()before dereferencingprevMsgHashOptwith either*or->operators. The ternary expressions on lines 921 and 924 safely handle both the present and absent cases.
1362-1366: LGTM - Clean delegation with appropriate locking.The manager method correctly delegates to the node state method while holding the appropriate lock, and returns the optional directly.
src/instantsend/signing.cpp (1)
302-305: Optional-based conflict check looks correct. The migration cleanly guards the dereference before logging, matching the new std::optional API from CSigningManager::GetVoteForId.src/Makefile.am (1)
270-274: Header export update acknowledged. Including llmq/dkgtypes.h in the public set keeps the build in sync with the type relocation done in this refactor.src/masternode/node.h (1)
14-87: Return-type refactor looks good. Switching GetLocalAddress to std::optional keeps the locking contract intact and aligns with the new call sites.src/rpc/quorums.cpp (1)
735-741: Recovered-sig lookup handled safely. The std::optional flow preserves the previous error handling without risking invalid access.src/llmq/blockprocessor.cpp (1)
723-731: Optional return enables clearer callers. Returning std::optional here keeps locking semantics the same while avoiding mutable out parameters.src/llmq/dkgsession.h (1)
9-10: Include adjustment is appropriate. Pulling in llmq/dkgtypes.h pairs with the new location of the DKG message types referenced later in this header.src/chainlock/chainlock.h (1)
25-25: Include for std::optional looks good.
Updated the logging statement in CSigSharesManager to enhance readability by continuing the format string on a new line. This change maintains the existing logging functionality while improving code clarity.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
-
src/llmq/signing_shares.cpp(4 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.cpp
🧠 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/wallet/wallet.cpp:0-0
Timestamp: 2025-07-29T14:33:01.040Z
Learning: In refactoring PRs like #6761, kwvg acknowledges code safety improvements (like null pointer checks and unused parameter warnings) but prefers to defer them to follow-up PRs to maintain focus on the primary refactoring objectives, avoiding scope creep.
Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
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.
🪛 GitHub Actions: Clang Diff Format Check
src/llmq/signing_shares.cpp
[error] 368-368: Clang-format differences detected in formatting of the log message. Run clang-format (or the project's clang-format-diff workflow) to apply formatting changes. Command: git diff -U0 origin/develop --
[error] 450-451: Clang-format differences detected in the log message formatting. Run clang-format (or the project's clang-format-diff workflow) to apply formatting changes. Command: 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: win64-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (5)
src/llmq/signing_shares.cpp (5)
155-169: LGTM! Clean refactoring to std::optional return type.The function correctly returns
std::nulloptwhen the session is not found and properly constructs aSessionInfovalue when found. This eliminates the bool + out-parameter pattern and aligns with modern C++20 practices.
355-387: LGTM! Safe optional consumption pattern.The code correctly uses the if-with-initializer pattern to check and consume the optional. All dereferencing operations (
sessionInfo->) are safely within the scope where the optional is guaranteed to have a value. The distinction betweenSessionInfo(value snapshot) andSession*(mutable reference) is properly maintained.
391-415: LGTM! Consistent optional handling.The function follows the same safe pattern as
ProcessMessageSigSharesInv, correctly checking and consuming the optional return value. All accesses are guarded by the optional check.
906-927: LGTM! Safe optional handling with proper guard checks.The code correctly handles the optional returned by
db.GetVoteForId():
- Line 907 uses short-circuit evaluation to safely check and dereference
- Lines 911 and 916 dereference within the if block where the value is guaranteed
- Lines 921 and 924 use explicit
has_value()checks in ternary expressionsAll optional accesses are properly guarded.
1362-1366: LGTM! Clean forwarding function.The function correctly forwards the optional return value from the node state's
GetSessionInfoByRecvIdmethod.
| if (!sessionInfo->quorum->HasVerificationVector()) { | ||
| // TODO we should allow to ask other nodes for the quorum vvec if we missed it in the DKG | ||
| LogPrint(BCLog::LLMQ_SIGS, /* Continued */ | ||
| "CSigSharesManager::%s -- we don't have the quorum vvec for %s, not requesting sig shares. node=%d\n", | ||
| __func__, sessionInfo->quorumHash.ToString(), pfrom.GetId()); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Fix clang-format differences in log message.
The pipeline reports clang-format differences on line 370 (the LogPrint call). Please run clang-format to resolve these formatting issues.
Run the following to fix formatting:
#!/bin/bash
# Run clang-format on the file
git diff -U0 origin/develop -- src/llmq/signing_shares.cpp | ./contrib/devtools/clang-format-diff.py -p1🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 368-368: Clang-format differences detected in formatting of the log message. Run clang-format (or the project's clang-format-diff workflow) to apply formatting changes. Command: git diff -U0 origin/develop --
🤖 Prompt for AI Agents
In src/llmq/signing_shares.cpp around lines 368 to 374, the LogPrint call on
line 370 has clang-format style differences; fix by reformatting that LogPrint
invocation to match project style (wrap arguments/line breaks as clang-format
expects) and then run the provided clang-format-diff script to apply the
formatting changes: git diff -U0 origin/develop -- src/llmq/signing_shares.cpp |
./contrib/devtools/clang-format-diff.py -p1. Ensure no functional changes are
made, only whitespace/line-break adjustments so the file passes the CI
clang-format check.
| if (auto sessionInfo = GetSessionInfoByRecvId(pfrom.GetId(), batchedSigShares.sessionId)) { | ||
| if (bool ban{false}; !PreVerifyBatchedSigShares(m_mn_activeman, qman, *sessionInfo, batchedSigShares, ban)) { | ||
| return !ban; | ||
| } | ||
|
|
||
| if (bool ban{false}; !PreVerifyBatchedSigShares(m_mn_activeman, qman, sessionInfo, batchedSigShares, ban)) { | ||
| return !ban; | ||
| } | ||
| std::vector<CSigShare> sigSharesToProcess; | ||
| sigSharesToProcess.reserve(batchedSigShares.sigShares.size()); | ||
|
|
||
| std::vector<CSigShare> sigSharesToProcess; | ||
| sigSharesToProcess.reserve(batchedSigShares.sigShares.size()); | ||
| { | ||
| LOCK(cs); | ||
| auto& nodeState = nodeStates[pfrom.GetId()]; | ||
|
|
||
| { | ||
| LOCK(cs); | ||
| auto& nodeState = nodeStates[pfrom.GetId()]; | ||
| for (const auto& sigSharetmp : batchedSigShares.sigShares) { | ||
| CSigShare sigShare = RebuildSigShare(*sessionInfo, sigSharetmp); | ||
| nodeState.requestedSigShares.Erase(sigShare.GetKey()); | ||
|
|
||
| for (const auto& sigSharetmp : batchedSigShares.sigShares) { | ||
| CSigShare sigShare = RebuildSigShare(sessionInfo, sigSharetmp); | ||
| nodeState.requestedSigShares.Erase(sigShare.GetKey()); | ||
| // TODO track invalid sig shares received for PoSe? | ||
| // It's important to only skip seen *valid* sig shares here. If a node sends us a | ||
| // batch of mostly valid sig shares with a single invalid one and thus batched | ||
| // verification fails, we'd skip the valid ones in the future if received from other nodes | ||
| if (sigShares.Has(sigShare.GetKey())) { | ||
| continue; | ||
| } | ||
|
|
||
| // TODO track invalid sig shares received for PoSe? | ||
| // It's important to only skip seen *valid* sig shares here. If a node sends us a | ||
| // batch of mostly valid sig shares with a single invalid one and thus batched | ||
| // verification fails, we'd skip the valid ones in the future if received from other nodes | ||
| if (sigShares.Has(sigShare.GetKey())) { | ||
| continue; | ||
| } | ||
| // TODO for PoSe, we should consider propagating shares even if we already have a recovered sig | ||
| if (sigman.HasRecoveredSigForId(sigShare.getLlmqType(), sigShare.getId())) { | ||
| continue; | ||
| } | ||
|
|
||
| // TODO for PoSe, we should consider propagating shares even if we already have a recovered sig | ||
| if (sigman.HasRecoveredSigForId(sigShare.getLlmqType(), sigShare.getId())) { | ||
| continue; | ||
| sigSharesToProcess.emplace_back(sigShare); | ||
| } | ||
|
|
||
| sigSharesToProcess.emplace_back(sigShare); | ||
| } | ||
| } | ||
|
|
||
| LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- signHash=%s, shares=%d, new=%d, inv={%s}, node=%d\n", __func__, | ||
| sessionInfo.signHash.ToString(), batchedSigShares.sigShares.size(), sigSharesToProcess.size(), batchedSigShares.ToInvString(), pfrom.GetId()); | ||
| LogPrint(BCLog::LLMQ_SIGS, /* Continued */ | ||
| "CSigSharesManager::%s -- signHash=%s, shares=%d, new=%d, inv={%s}, node=%d\n", | ||
| __func__, sessionInfo->signHash.ToString(), batchedSigShares.sigShares.size(), | ||
| sigSharesToProcess.size(), batchedSigShares.ToInvString(), pfrom.GetId()); | ||
|
|
||
| if (sigSharesToProcess.empty()) { | ||
| return true; | ||
| } | ||
| if (sigSharesToProcess.empty()) { | ||
| return true; | ||
| } | ||
|
|
||
| LOCK(cs); | ||
| auto& nodeState = nodeStates[pfrom.GetId()]; | ||
| for (const auto& s : sigSharesToProcess) { | ||
| nodeState.pendingIncomingSigShares.Add(s.GetKey(), s); | ||
| LOCK(cs); | ||
| auto& nodeState = nodeStates[pfrom.GetId()]; | ||
| for (const auto& s : sigSharesToProcess) { | ||
| nodeState.pendingIncomingSigShares.Add(s.GetKey(), s); | ||
| } | ||
| return true; | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Fix clang-format differences in log message.
The refactoring logic is correct—the optional is safely consumed with proper checks and dereferencing. However, the pipeline reports clang-format differences on lines 452-455.
Please run clang-format on this section to resolve the formatting differences:
#!/bin/bash
# Run clang-format on the specific lines
git diff -U0 origin/develop -- src/llmq/signing_shares.cpp | ./contrib/devtools/clang-format-diff.py -p1🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 450-451: Clang-format differences detected in the log message formatting. Run clang-format (or the project's clang-format-diff workflow) to apply formatting changes. Command: git diff -U0 origin/develop --
🤖 Prompt for AI Agents
In src/llmq/signing_shares.cpp around lines 419 to 469, the LogPrint call
formatting was changed and causes clang-format differences on lines ~452-455;
run clang-format on this section (or reformat the LogPrint parameters back to
the project's style) so the multiline LogPrint matches the repository
clang-format rules — e.g., wrap the arguments and indentation exactly as other
LogPrint calls in the file or run the suggested clang-format-diff.py command to
automatically apply the correct formatting.
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/dkgtypes.h(1 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/dkgtypes.h
🧠 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: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/wallet/wallet.cpp:0-0
Timestamp: 2025-07-29T14:33:01.040Z
Learning: In refactoring PRs like #6761, kwvg acknowledges code safety improvements (like null pointer checks and unused parameter warnings) but prefers to defer them to follow-up PRs to maintain focus on the primary refactoring objectives, avoiding scope creep.
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/dkgtypes.h (2)
src/bls/bls.h (2)
CBLSSecretKey(273-277)CBLSSignature(346-349)src/llmq/signing_shares.h (1)
GetSignHash(75-85)
🪛 GitHub Actions: Clang Diff Format Check
src/llmq/dkgtypes.h
[error] 1-1: Clang format differences found in this file. Run clang-format-diff.py 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: linux64_multiprocess-build / Build source
- GitHub Check: mac-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: linux64_tsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
🔇 Additional comments (1)
src/llmq/dkgtypes.h (1)
1-193: Resolve the clang-format check failure.CI is currently failing with
clang-format-diffcomplaints on this file. Please run the project’s formatting script (e.g.contrib/devtools/clang-format-diff.py) over the touched ranges before re-pushing.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| if (!VerifySigSharesInv(sessionInfo.llmqType, inv)) { | ||
| return false; | ||
| } | ||
| if (auto sessionInfo = GetSessionInfoByRecvId(pfrom.GetId(), inv.sessionId)) { |
There was a problem hiding this comment.
I don't like increasing nested level here tbh. Consider changing this to
auto sessionInfo = GetSessionInfoByRecvId(pfrom.GetId(), inv.sessionId);
if (!sessionInfo) return true;
Same ProcessMessageGetSigShares and ProcessMessageBatchedSigShares.
| } | ||
| session->announced.Merge(inv); | ||
| session->knows.Merge(inv); | ||
| return true; |
There was a problem hiding this comment.
this line can be dropped, same for other functions
|
This pull request has conflicts, please rebase. |
2 participants
Issue being fixed or feature implemented
Return by value with std::optional over using return by reference
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.