refactor: avoid return by reference and prefer return by value #6955
Conversation
|
WalkthroughThis PR converts many bool + out-parameter getters to std::optional return values across multiple subsystems (chainlock, instantsend, LLMQ signing & recovered sigs DB, DKG session/handler/manager, signing shares, blockprocessor, net_processing, RPC quorums, masternode). It extracts four DKG types into a new public header 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-parameter
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: 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 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)
🧰 Additional context used🧠 Learnings (11)📓 Common learnings📚 Learning: 2025-07-29T14:33:01.040ZApplied to files:
📚 Learning: 2025-06-09T16:43:20.996ZApplied to files:
📚 Learning: 2025-11-04T18:24:27.241ZApplied to files:
📚 Learning: 2025-10-05T20:38:28.457ZApplied to files:
📚 Learning: 2025-10-13T12:37:12.357ZApplied to files:
📚 Learning: 2025-07-29T14:32:48.369ZApplied to files:
📚 Learning: 2025-07-23T09:28:32.783ZApplied to files:
📚 Learning: 2025-08-11T17:16:36.654ZApplied to files:
📚 Learning: 2025-10-25T07:08:51.918ZApplied to files:
📚 Learning: 2025-07-17T15:48:29.418ZApplied to files:
⏰ 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)
🔇 Additional comments (1)
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.
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.
src/llmq/signing_shares.cpp
Outdated
| 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.
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.
src/llmq/signing_shares.cpp
Outdated
| 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
There was a problem hiding this comment.
I don't understand what you mean
|
This pull request has conflicts, please rebase. |
| #define BITCOIN_LLMQ_DKGSESSION_H | ||
|
|
||
| #include <llmq/commitment.h> | ||
| #include <llmq/dkgtypes.h> |
There was a problem hiding this comment.
nit: can probably drop
#include <bls/bls_ies.h>
and something else once dkgtypes.h is split
@coderabbitai, validate list of included headers for this PR
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what symbols are used from each include in dkgsession.h
echo "=== Analyzing includes in src/llmq/dkgsession.h ==="
echo
# Check for bls_ies.h usage
echo "1. Checking bls/bls_ies.h usage:"
rg -n "CBLS.*IES|IES.*Recipient" src/llmq/dkgsession.h
echo
echo "2. Checking util/underlying.h usage (ToUnderlying):"
rg -n "ToUnderlying|underlying" src/llmq/dkgsession.h
echo
echo "3. Checking batchedlogger.h usage:"
rg -n "CBatchedLogger" src/llmq/dkgsession.h
echo
echo "4. Checking saltedhasher.h usage:"
rg -n "Salted|Uint256HashSet" src/llmq/dkgsession.h
echo
echo "5. Checking what dkgtypes.h defines:"
cat src/llmq/dkgtypes.h | head -50
echo
echo "6. Checking if any types from dkgtypes.h are forward-declarable:"
rg -n "CDKG(Contribution|Complaint|Justification|PrematureCommitment)" src/llmq/dkgsession.h
echo
echo "7. Check includes in dkgtypes.h:"
rg "^#include" src/llmq/dkgtypes.h
Length of output: 3176
@PastaPastaPasta, @knst is correct! After analyzing the includes in src/llmq/dkgsession.h, I can confirm that the following includes are now redundant and can be removed:
-
#include <bls/bls_ies.h>- This can be dropped because:- It's used for
CBLSIESMultiRecipientObjects<CBLSSecretKey>(line 144) - However,
dkgtypes.halready includesbls/bls_ies.h - The type will be available transitively through
dkgtypes.h
- It's used for
-
#include <util/underlying.h>- This can be dropped because:- It's not directly used anywhere in
dkgsession.h dkgtypes.halready includes it- If needed in the future, it's available transitively
- It's not directly used anywhere in
Current necessary includes:
llmq/commitment.h- used for CFinalCommitmentllmq/dkgtypes.h- provides CDKGContribution, CDKGComplaint, CDKGJustification, CDKGPrematureCommitment (stored as full objects in std::map members, can't be forward-declared)batchedlogger.h- base class for CDKGLoggerbls/bls.h- BLS types (CBLSId, CBLSSecretKey, BLSVerificationVectorPtr, etc.)bls/bls_worker.h- CBLSWorker& memberevo/types.h- CDeterministicMN typessaltedhasher.h- Uint256HashSet typesync.h- Mutex, GUARDED_BYoptional- std::optional usageunordered_set- std::unordered_set
Would you like me to create an issue to track the removal of these redundant includes?
🧬 Code Graph Analysis Results
-
File: src/llmq/dkgsessionmgr.h
- Snippet (32-61)
- Content: Contributions cache key/entry types and their usage:
- ContributionsCacheKey (llmqType, quorumHash, proTxHash) with operator<
- ContributionsCacheEntry (entryTime, vvec, skContribution)
- Contributes a thread-safe cache for verified contributions and SK contributions, guarded by contributionsCacheCs
- Rationale: Helps understand how CDKGSessionManager caches verified contributions and SK shares for DKG sessions.
-
File: src/llmq/dkgsessionhandler.h
- Snippet (65-208)
- Content: CDKGPendingMessages class
- Manages per-node pending messages (contributions, complaints, justifications, premature commitments)
- Synchronization via cs_messages
- Methods to PushPendingMessage, PopPendingMessages, HasSeen, Misbehaving, Clear
- Template helper to PushPendingMessage and to PopAndDeserializeMessages
- Rationale: Explains how a quorum session queues and processes batched DKG messages before full verification.
-
File: src/llmq/dkgsession.h
- Snippet (not a separate file; reference for context)
- Content: CDKGLogger forward usage
- Rationale: Provides context for how logging is associated with a CDKGSession instance.
-
File: src/llmq/dkgsession.cpp
- Snippet (CDKGSession::CDKGSession constructor) - Line ~70-88
- Content: CDKGSession constructor initializes DKG session with parameters, blsWorker, managers, and session state
- Rationale: Shows how a DKG session is instantiated, which informs how the rest of the session lifecycle is expected to operate.
-
File: src/llmq/dkgsession.cpp
- Snippet (CDKGSession::Contribute) - Line ~157-178
- Content: Generating contributions via blsWorker and preparing to send
- Rationale: Core step of Phase 1; helps understand how local contributions are created.
-
File: src/llmq/dkgsession.cpp
- Snippet (CDKGSession::SendContributions) - Line ~180-230
- Content: Building, encrypting, signing and sending own contributions
- Rationale: Critical for understanding how a member’s contribution is prepared and distributed.
-
File: src/llmq/dkgsession.cpp
- Snippet (CDKGSession::PreVerifyMessage for CDKGContribution) - Line ~233-277
- Content: Validation logic for incoming contributions (quorum hash, member validation, vector lengths, verification)
- Rationale: Key for understanding pre-verification checks before deeper processing.
-
File: src/llmq/dkgsession.cpp
- Snippet (CDKGSession::ReceiveMessage for CDKGContribution) - Line ~280-360
- Content: Handling of received contributions, decryption path, verification queuing, and relay logic
- Rationale: Details on how contributions are accepted, decrypted, and propagated.
-
File: src/llmq/dkgsession.cpp
- Snippet (CDKGSession::PreVerifyMessage for CDKGComplaint) - Line ~550-590
- Content: Validation logic for complaints (quorum hash, member validity, bad/complaint counts)
- Rationale: Clarifies pre-verification rules for complaints.
-
File: src/llmq/dkgsession.cpp
- Snippet (CDKGSession::ReceiveMessage for CDKGComplaint) - Line ~592-651
- Content: Handling of received complaints, relay, and member state updates
- Rationale: Explains how complaints are recorded and relayed.
-
File: src/llmq/dkgsession.cpp
- Snippet (CDKGSession::PreVerifyMessage for CDKGJustification) - Line ~744-800
- Content: Validation logic for justifications (quorum hash, member, contributions, and counts)
- Rationale: Details pre-verification for justifications.
-
File: src/llmq/dkgsession.cpp
- Snippet (CDKGSession::ReceiveMessage for CDKGJustification) - Line ~802-892
- Content: Handling of justified shares, verification of contributions, and updating state
- Rationale: Explains how justifications are processed and how SK contributions are integrated.
-
File: src/llmq/dkgsession.cpp
- Snippet (CDKGSession::ReceiveMessage for CDKGPrematureCommitment) - Line ~1113-1188
- Content: Premature commitment handling, vector verification, and relay logic
- Rationale: Clarifies how premature commitments are validated and relayed.
-
File: src/llmq/dkgsession.cpp
- Snippet (CDKGSession::ReceiveMessage for CDKGPrematureCommitment) - Line ~1113-1113
- Content: Specific entry point for premature commitment receive
- Rationale: Quick anchor for premature commitment handling.
-
File: src/llmq/dkgsession.cpp
- Snippet (CDKGSession::FinalizeCommitments) - Line ~1190-1297
- Content: Aggregation of valid premature commitments into final quorums
- Rationale: Central to producing final commitments.
-
File: src/llmq/dkgsession.cpp
- Snippet (CDKGSession::FinalizeSingleCommitment) - Line ~1299-1355
- Content: Single-node quorum finalization path
- Rationale: Explains fallback/single-member signing scenario.
-
File: src/llmq/dkgsession.cpp
- Snippet (CDKGSession::MarkBadMember) - Line ~1366-1366
- Content: Marking a member as bad and logging status
- Rationale: Important for understanding how misbehaving members are tracked.
-
File: src/llmq/dkgsession.cpp
- Snippet (CDKGSession::GetMember) - Line ~1357-1357
- Content: Lookup member by proTxHash
- Rationale: Member lookup utility used throughout processing.
-
File: src/llmq/dkgsession.cpp
- Snippet (CDKGSession::ShouldSimulateError) - Line ~53-60
- Content: Determines whether to simulate DKG errors for test quorums
- Rationale: Relevant for tests and understanding error injection
-
File: src/llmq/dkgsession.cpp
- Snippet (SetSimulatedDKGErrorRate / GetSimulatedErrorRate) - Lines 41-51 and 47-47
- Content: Global helpers to configure/get simulated error rates
- Rationale: Context for how error rates are controlled in tests
-
File: src/llmq/signing_shares.h (contextual; not from dkgsession.h)
- Snippet (CBatchedSigShares and SigShareMap usage)
- Content: Structures used for batched signature shares
- Rationale: Helpful for understanding how sigShares are aggregated in DKG flows
-
File: src/llmq/params.h (contextual)
- Snippet (LLMQParams basics and available_llmqs)
- Content: Definitions of LLMQ types, parameters, and rotation behavior
- Rationale: Provides context for how DKG sessions are configured; relevant to interpretation of CDKGSession behavior
Note: If you want, I can extract and present any of these snippets in full from the provided references, or tailor a focused subset to a specific area of interest (e.g., contribution flow, verification flow, or finalization).
🧠 Learnings used
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.
| #include <bls/bls.h> | ||
| #include <bls/bls_worker.h> | ||
| #include <llmq/dkgsessionhandler.h> | ||
| #include <llmq/dkgtypes.h> |
There was a problem hiding this comment.
| #include <llmq/dkgtypes.h> |
Same here; forward declaration is enough for references and return's type
src/llmq/dkgtypes.h
Outdated
| #include <bls/bls.h> | ||
| #include <bls/bls_ies.h> | ||
| #include <consensus/params.h> | ||
| #include <hash.h> | ||
| #include <serialize.h> | ||
| #include <util/underlying.h> | ||
|
|
||
| #include <llmq/commitment.h> |
There was a problem hiding this comment.
| #include <bls/bls.h> | |
| #include <bls/bls_ies.h> | |
| #include <consensus/params.h> | |
| #include <hash.h> | |
| #include <serialize.h> | |
| #include <util/underlying.h> | |
| #include <llmq/commitment.h> | |
| #include <bls/bls.h> | |
| #include <bls/bls_ies.h> | |
| #include <consensus/params.h> | |
| #include <hash.h> | |
| #include <llmq/commitment.h> | |
| #include <serialize.h> | |
| #include <util/underlying.h> |
src/llmq/dkgtypes.h
Outdated
| } // namespace llmq | ||
|
|
||
| #endif // BITCOIN_LLMQ_DKGTYPES_H | ||
|
|
There was a problem hiding this comment.
nit: duplicated \n\n at the end
src/llmq/signing_shares.cpp
Outdated
| if (!VerifySigSharesInv(sessionInfo.llmqType, inv)) { | ||
| return false; | ||
| } | ||
| if (auto sessionInfo = GetSessionInfoByRecvId(pfrom.GetId(), inv.sessionId)) { |
There was a problem hiding this comment.
|
|
||
| llmq::CRecoveredSig recSig; | ||
| if (!llmq_ctx.sigman->GetRecoveredSigForId(llmqType, id, recSig)) { | ||
| auto recSigOpt = llmq_ctx.sigman->GetRecoveredSigForId(llmqType, id); |
|
This pull request has conflicts, please rebase. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/llmq/dkgtypes.h (1)
23-72: Consider removing redundant access specifier.Line 32 has a duplicate
public:label (the class members are already public from line 25). While not incorrect, removing it improves readability.Apply this diff to remove the redundant label:
CBLSSignature sig; -public: template<typename Stream> inline void SerializeWithoutSig(Stream& s) constNote: This same pattern appears in the other three classes in this file.
📜 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
🧠 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.
🧬 Code graph analysis (1)
src/llmq/dkgtypes.h (3)
src/bls/bls.h (2)
CBLSSecretKey(273-277)CBLSSignature(346-349)test/functional/test_framework/messages.py (1)
CBLSPublicKey(1577-1589)src/llmq/signing_shares.h (1)
GetSignHash(75-85)
⏰ 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). (7)
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (1)
src/llmq/dkgtypes.h (1)
1-21: LGTM! Header structure and includes are correct.The file properly includes all necessary headers, including
<memory>forstd::shared_ptrusage (addressing the previous critical issue). The header guard and namespace declarations follow conventions.
…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.
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.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
PastaPastaPasta
force-pushed
the
refac/return-value
branch
from
5d6c64c to
5b7c51e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/llmq/signing_shares.cpp (3)
355-377: Consider simplifying the early return per past review feedback.The optional handling is correct—the code checks for presence before dereferencing and all subsequent uses of
sessionInfo->are safe. However, the early return could be simplified to a one-liner to match the style suggestion from past reviews.Consider applying this diff to simplify the early return:
auto sessionInfo = GetSessionInfoByRecvId(pfrom.GetId(), inv.sessionId); - if (!sessionInfo) { - return true; - } + if (!sessionInfo) return true;Based on learnings
393-418: LGTM! Consider simplifying the early return.The optional handling is correct. Same style note as the previous function: the early return could be simplified to a one-liner.
Consider applying this diff:
auto sessionInfo = GetSessionInfoByRecvId(pfrom.GetId(), inv.sessionId); - if (!sessionInfo) { - return true; - } + if (!sessionInfo) return true;Based on learnings
423-474: LGTM! Consider simplifying the early return.The refactoring is correct. The optional is safely checked before dereferencing, and the value is properly passed to
PreVerifyBatchedSigSharesandRebuildSigShare. Same style note applies here.Consider applying this diff:
auto sessionInfo = GetSessionInfoByRecvId(pfrom.GetId(), batchedSigShares.sessionId); - if (!sessionInfo) { - return true; - } + if (!sessionInfo) return true;Based on learnings
src/llmq/dkgtypes.h (1)
8-187: New DKG type header looks like a straight extraction of existing behaviorThe four DKG structs (contribution, complaint, justification, premature commitment) are defined with the expected fields, serialization, and
GetSignHashlogic (including zeroed sigs andBuildCommitmentHashusage), and the header pulls in the right BLS/consensus/serialization dependencies. This aligns with the intent to centralize DKG types without altering their wire format or semantics.If you ever touch this again, a small, non-blocking improvement would be to document or assert the invariant that
CDKGContribution::vvecandcontributionsare non-null before use, just to make the assumptions explicit; no need to change that in this refactor.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
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/dkgtypes.h(1 hunks)src/llmq/signing.cpp(8 hunks)src/llmq/signing.h(4 hunks)src/llmq/signing_shares.cpp(8 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)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/Makefile.am
- src/llmq/dkgsessionmgr.h
- src/rpc/quorums.cpp
- src/instantsend/instantsend.h
- src/masternode/node.h
- src/llmq/dkgsessionmgr.cpp
🧰 Additional context used
🧠 Learnings (16)
📓 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-29T14:33:01.040Z
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.
Applied to files:
src/llmq/dkgsession.h
📚 Learning: 2025-06-09T16:43:20.996Z
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.
Applied to files:
src/llmq/dkgsession.hsrc/net_processing.cpp
📚 Learning: 2025-11-04T18:24:27.241Z
Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.
Applied to files:
src/llmq/dkgsession.h
📚 Learning: 2025-10-05T20:38:28.457Z
Learnt from: knst
Repo: dashpay/dash PR: 6871
File: contrib/guix/libexec/build.sh:358-360
Timestamp: 2025-10-05T20:38:28.457Z
Learning: In the Dash repository, when backporting code from Bitcoin Core, typos and minor issues in comments should be kept as-is to reduce merge conflicts in future backports, even if they remain unfixed in Bitcoin Core's master branch.
Applied to files:
src/llmq/dkgsession.hsrc/llmq/signing_shares.cpp
📚 Learning: 2025-10-13T12:37:12.357Z
Learnt from: knst
Repo: dashpay/dash PR: 6883
File: src/rpc/rawtransaction.cpp:1088-1125
Timestamp: 2025-10-13T12:37:12.357Z
Learning: In backport pull requests (especially from Bitcoin Core), treat "moved" or refactored code as out-of-scope for content-level review. Focus validation on verifying that code is moved correctly: no fields added, no fields removed, no fields reordered, and no unexpected changes beyond whitespace adjustments. Pre-existing issues in the upstream code should be preserved to maintain fidelity to the original implementation.
Applied to files:
src/llmq/dkgsession.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/llmq/dkgsession.hsrc/chainlock/chainlock.cpp
📚 Learning: 2025-07-23T09:28:32.783Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:15-250
Timestamp: 2025-07-23T09:28:32.783Z
Learning: In refactoring PRs like #6761, kwvg prefers to defer code formatting fixes to separate follow-up PRs when formatting is not the primary objective, to maintain focus on the structural changes and avoid scope creep.
Applied to files:
src/llmq/dkgsession.h
📚 Learning: 2025-08-11T17:16:36.654Z
Learnt from: PastaPastaPasta
Repo: dashpay/dash PR: 6804
File: src/qt/proposalwizard.cpp:40-42
Timestamp: 2025-08-11T17:16:36.654Z
Learning: In the Dash repository, when a PR adds new files that are not from Bitcoin backports, these files must be added to the list in test/util/data/non-backported.txt. This applies to newly created files like qt/proposalwizard.{h,cpp} and forms/proposalwizard.ui. Limited exemptions may exist for subtrees and similar cases.
Applied to files:
src/llmq/dkgsession.hsrc/llmq/signing_shares.cpp
📚 Learning: 2025-10-25T07:08:51.918Z
Learnt from: knst
Repo: dashpay/dash PR: 6916
File: src/univalue/include/univalue.h:81-88
Timestamp: 2025-10-25T07:08:51.918Z
Learning: For backport PRs from bitcoin/bitcoin, bitcoin-core/gui, etc., backported changes should match the original upstream PRs even if they appear strange, modify vendored code, or seem to violate coding guidelines. Still flag genuine issues like bugs, undefined behavior, crashes, compilation errors, or linter failures.
Applied to files:
src/llmq/dkgsession.h
📚 Learning: 2025-07-17T15:48:29.418Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6752
File: src/wallet/load.cpp:164-164
Timestamp: 2025-07-17T15:48:29.418Z
Learning: In Dash Core, kwvg prefers using assert() statements to document expected behavior in code paths, especially when the called function also has internal asserts. This is used as a defensive programming pattern rather than requiring conditional error handling.
Applied to files:
src/llmq/dkgsession.h
📚 Learning: 2025-02-14T15:15:58.165Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6529
File: src/rpc/governance.cpp:1074-1089
Timestamp: 2025-02-14T15:15:58.165Z
Learning: Code blocks marked with `// clang-format off` and `// clang-format on` directives should be excluded from clang-format suggestions as they are intentionally formatted manually for better readability.
Applied to files:
src/llmq/signing_shares.cpp
📚 Learning: 2025-07-09T15:05:36.250Z
Learnt from: kwvg
Repo: dashpay/dash PR: 6729
File: src/rpc/evo.cpp:1273-1273
Timestamp: 2025-07-09T15:05:36.250Z
Learning: When clang-format suggestions significantly harm readability (like splitting logical parameter groups across multiple lines), it's acceptable to use `// clang-format off` and `// clang-format on` directives to exclude the problematic section from automatic formatting, prioritizing code readability over strict line length compliance.
Applied to files:
src/llmq/signing_shares.cpp
📚 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/llmq/signing_shares.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/net_processing.cppsrc/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 (10)
src/llmq/blockprocessor.h (1)
src/llmq/blockprocessor.cpp (2)
GetMineableCommitmentByHash(723-731)GetMineableCommitmentByHash(723-723)
src/net_processing.cpp (2)
src/instantsend/net_instantsend.cpp (1)
inv(190-190)src/llmq/dkgsession.cpp (4)
inv(301-301)inv(609-609)inv(820-820)inv(1175-1175)
src/llmq/signing_shares.h (1)
src/llmq/signing_shares.cpp (4)
GetSessionInfoByRecvId(155-169)GetSessionInfoByRecvId(155-155)GetSessionInfoByRecvId(1385-1389)GetSessionInfoByRecvId(1385-1385)
src/llmq/dkgsessionhandler.h (3)
src/llmq/dkgtypes.h (2)
CDKGContribution(23-41)CDKGComplaint(86-87)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/masternode/node.cpp (2)
src/net.cpp (2)
GetLocalAddress(231-234)GetLocalAddress(231-231)src/masternode/node.h (1)
cs(48-87)
src/chainlock/chainlock.h (2)
src/chainlock/clsig.h (1)
chainlock(15-38)src/chainlock/chainlock.cpp (2)
GetChainLockByHash(97-107)GetChainLockByHash(97-97)
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/llmq/signing.h (1)
src/llmq/signing.cpp (14)
GetRecoveredSigByHash(118-127)GetRecoveredSigByHash(118-118)GetRecoveredSigById(129-132)GetRecoveredSigById(129-129)GetVoteForId(277-285)GetVoteForId(277-277)GetVoteForId(718-721)GetVoteForId(718-718)ReadRecoveredSig(100-116)ReadRecoveredSig(100-100)GetRecoveredSigForGetData(367-379)GetRecoveredSigForGetData(367-367)GetRecoveredSigForId(697-700)GetRecoveredSigForId(697-697)
src/chainlock/chainlock.cpp (1)
src/chainlock/chainlock.h (2)
cs(65-136)cs(66-133)
src/llmq/dkgtypes.h (3)
src/bls/bls.h (2)
CBLSSecretKey(273-277)CBLSSignature(346-349)test/functional/test_framework/messages.py (1)
CBLSPublicKey(1577-1589)src/llmq/signing_shares.h (1)
GetSignHash(76-86)
⏰ 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). (11)
- 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_nowallet-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: ClangFormat
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
🔇 Additional comments (20)
src/instantsend/signing.cpp (1)
302-309: LGTM! Clean optional-based API refactoring.The refactoring from bool+out-parameter to std::optional return is correctly implemented. The code properly checks that
otherTxHashOpthas a value before dereferencing it on lines 303 and 305.src/llmq/dkgsession.h (1)
9-9: Include is necessary for value semantics.The include of
llmq/dkgtypes.his required because the class storesCDKGContribution,CDKGComplaint,CDKGJustification, andCDKGPrematureCommitmentby value in member variables (lines 155-158). Forward declarations are insufficient when storing types by value in containers likestd::map.src/llmq/blockprocessor.cpp (1)
723-731: LGTM! Correct optional return implementation.The refactoring correctly returns
std::nulloptwhen the commitment is not found (line 728) and returns the commitment value directly when found (line 730). This is consistent with the updated method signature in the header.src/chainlock/chainlock.h (1)
26-26: LGTM! Header changes align with implementation.The addition of
<optional>include and the updated method signature forGetChainLockByHashare consistent with the implementation changes insrc/chainlock/chainlock.cpp(lines 97-107).Also applies to: 95-95
src/chainlock/chainlock.cpp (1)
97-107: LGTM! Clean optional return pattern.The implementation correctly returns
std::nulloptwhen the requested chain lock hash doesn't match (line 103) and returns thebestChainLockvalue directly when it does match (line 106). The pattern is consistent with the broader refactoring across the codebase.src/llmq/blockprocessor.h (1)
76-77: LGTM! Header signature matches implementation.The updated method signature correctly reflects the implementation in
src/llmq/blockprocessor.cpp(lines 723-731), returningstd::optional<CFinalCommitment>instead of using a bool return with an out-parameter.src/llmq/signing_shares.h (1)
348-348: LGTM! Consistent optional return pattern.Both
GetSessionInfoByRecvIdmethods correctly adopt the optional return pattern, removing the out-parameter approach. The implementations insrc/llmq/signing_shares.cpp(lines 154-168 and 1384-1388) correctly returnstd::nulloptwhen the session is not found and return theSessionInfovalue when found.Also applies to: 481-482
src/llmq/dkgsessionhandler.h (2)
10-10: Include is necessary for optional return types.The include of
llmq/dkgtypes.his required because the methods on lines 179-182 returnstd::optional<CDKGContribution>,std::optional<CDKGComplaint>, etc. by value. Returningstd::optional<T>by value requires the complete type definition ofT, asstd::optionalstores the value inline and needs to know its size. Forward declarations are insufficient for this use case.
179-182: LGTM! API refactoring is consistent.The four method signatures correctly adopt the optional return pattern, replacing the previous bool+out-parameter approach. The implementations in
src/llmq/dkgsessionhandler.cpp(lines 629-667) properly returnstd::nulloptwhen items are not found and return the values directly when found.src/masternode/node.cpp (2)
126-131: LGTM! Correct optional handling.The refactoring correctly handles the optional return value from
GetLocalAddress(). The code properly checks for the presence of a value, safely dereferences it when present, and handles the absence case by setting an error state.
218-260: LGTM! Refactoring correctly implemented.The conversion from bool + out-parameter to
std::optional<CService>is correctly implemented. All return paths properly return eitherstd::nullopton failure or the constructedCServiceon success. The fallback logic (primary method → loopback → peer iteration) is preserved correctly.src/llmq/signing_shares.cpp (3)
155-169: LGTM! Refactoring correctly implemented.The conversion from bool + out-parameter to
std::optional<SessionInfo>is correctly implemented. The function returnsstd::nulloptwhen the session is not found and returns the populatedSessionInfostruct otherwise.
929-948: LGTM! Proper optional handling with ternary operator.The code correctly handles the optional returned from
GetVoteForId. The ternary operators on lines 944 and 947 safely handle both the present and absent cases when logging.
1385-1389: LGTM! Simple wrapper correctly forwards the optional.The public wrapper correctly forwards to the node state's method and returns the optional result.
src/net_processing.cpp (1)
2886-2940: Optional-based GetData replies for quorum/CL/IS objects look correct and behavior-preservingEach branch now calls a
Get*that returnsstd::optional<T>, checks it withif (auto opt = ...), and only dereferences inside theif, settingpush = trueon success. When the optional is empty, control flow still falls through to the existingvNotFoundhandling, so semantics match the previous bool+out-param pattern, and lifetimes are safe since the value is copied intoPushMessage. No changes needed.
Based on learningssrc/llmq/dkgsessionhandler.cpp (1)
630-668: Optional-based DKG getters preserve prior semanticsThese four getters cleanly replace the old
bool + out-parampattern withstd::optional<T>, keep the existing locking (invCs) and lookup logic (including thevalidCommitmentsgate), and don’t alter copying behavior vs the previous implementation. Looks good and tightly scoped to the refactor intent.src/instantsend/instantsend.cpp (1)
620-642: InstantSend lock lookup refactor is behaviorally neutralSwitching
GetInstantSendLockByHashtostd::optional<InstantSendLock>mirrors the old success/failure semantics (including the “IS disabled” path), keeps the DB + pending-locks lookup order and locking intact, and just replaces the out-parameter copy with a value-return copy. No issues spotted.src/llmq/signing.h (1)
25-26: Recovered-sig/vote APIs now clearly model absence with std::optionalThe header updates (adding
<optional>and switching the variousGet*methods to returnstd::optionalinstead of using out-parameters) are consistent with the implementations insigning.cppand make the “not found / invalid” case explicit at the type level without changing behavior. This is a good, tightly scoped API improvement.Also applies to: 139-155, 196-196, 238-242
src/llmq/signing.cpp (2)
26-27: CRecoveredSigsDb optional helpers are a faithful refactorUsing
std::optionalinReadRecoveredSig,GetRecoveredSigByHash,GetRecoveredSigById, andGetVoteForIdcleanly replaces the priorbool + out-parampattern. Missing keys and deserialization failures both surface asstd::nullopt, which matches the old behavior, andRemoveRecoveredSig’s early-return onnulloptpreserves the previous “nothing to remove” handling. Cache updates and cleanup paths stay unchanged.Also applies to: 100-132, 171-178, 277-285
367-379: CSigningManager’s recovered-sig/vote lookups now use optionals consistently
GetRecoveredSigForGetData,ProcessRecoveredSig’s conflict handling, and the publicGetRecoveredSigForId/GetVoteForIdmethods now work withstd::optional, but retain the same semantics: only active-quorum sigs are returned for getdata, conflicts are still detected against any existing recovered sig, and callers can now rely on presence checks instead of implicit “success” booleans. The refactor is cohesive and doesn’t introduce behavioral changes.Also applies to: 612-632, 697-700, 718-721
|
This pull request has conflicts, please rebase. |
3 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.