refactor: migrate ProcessMessage definitions to MessageProcessingResult, drop PeerMsgRet
#6820
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe patch removes src/util/expected.h and the PeerMsgRet alias, introduces MessageProcessingResult (m_inventory now vector), and changes many message-handling APIs to return MessageProcessingResult (often marked [[nodiscard]]). Numerous handlers switch CNode/PeerManager usage to NodeId and std::string_view for msg_type. net_processing centralizes post-processing via PostProcessMessage and updates RelayInv signatures to take const CInv&. net_types::NodeId was added. Build/dev tooling lists (lint/devtools) were updated to include the removed header in checks. No algorithmic control-flow beyond result-type and signature changes. Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (5)
src/llmq/blockprocessor.h (1)
8-18: Add missing include for std::string_view to prevent build issuesThis header declares a function using std::string_view but does not include <string_view>. Relying on transitive includes is brittle and can break consumers of this header.
Apply this diff to include the missing header:
#include <sync.h> #include <gsl/pointers.h> #include <optional> +#include <string_view>src/llmq/dkgsessionmgr.h (1)
8-15: Include <string_view> in this headerThe declaration uses std::string_view but this header doesn't include <string_view>. This can break compilation in TU(s) that include this header without bringing <string_view> themselves.
Apply this diff:
#include <llmq/dkgsessionhandler.h> #include <net_types.h> #include <map> #include <memory> +#include <string_view>src/llmq/quorums.h (1)
10-19: Add <string_view> include to match the new parameter typeThis header declares a function taking std::string_view but doesn't include the header. Add it to avoid relying on transitive includes.
Apply this diff:
#include <util/threadinterrupt.h> #include <unordered_lru_cache.h> #include <util/time.h> +#include <string_view>src/llmq/blockprocessor.cpp (1)
658-671: Bug: stale entry not erased when overriding minable commitment mappingWhen a better commitment arrives, you overwrite the map value first, then erase using the new hash, leaving the old entry in minableCommitments (memory leak and potential stale lookups). Save the old hash before overwriting and erase that.
Apply this diff:
- auto& insertedQuorumHash = itInserted->second; - const auto& oldFqc = minableCommitments.at(insertedQuorumHash); + auto& insertedQuorumHash = itInserted->second; + const auto old_hash = insertedQuorumHash; + const auto& oldFqc = minableCommitments.at(old_hash); if (fqc.CountSigners() > oldFqc.CountSigners()) { // new commitment has more signers, so override the known one - insertedQuorumHash = commitmentHash; - minableCommitments.erase(insertedQuorumHash); - minableCommitments.try_emplace(commitmentHash, fqc); + insertedQuorumHash = commitmentHash; // update mapping to new + minableCommitments.erase(old_hash); // remove old entry + minableCommitments.try_emplace(commitmentHash, fqc); // insert new return true; }src/instantsend/instantsend.h (1)
38-38: Remove duplicate NodeId alias to avoid redefinitionThis header already includes <net_types.h> (Line 8), which defines NodeId. Redeclaring it here risks redefinition errors or ODR issues and diverges from the single-source-of-truth pattern.
Apply this diff to remove the duplicate alias:
-using NodeId = int64_t;
🧹 Nitpick comments (24)
src/protocol.h (2)
599-601: Docstring reads singular but field is now a collectionNow that m_inventory is a vector, the doc should reflect plurality for clarity.
Apply this diff:
- //! @m_inventory will relay this inventory to connected peers + //! @m_inventory will relay these inventories to connected peersAdditionally (outside the shown lines), consider ensuring the header explicitly includes its STL dependencies to be self-contained:
- Add: #include (since std::optional is used)
- Add: #include (std::vector is used throughout this header)
// Near other STL includes at the top of this file #include <optional> #include <vector>
613-614: Constructor change is correct; consider an overload for batchingThe braced-init to seed a single-element vector is fine. For efficiency when upstreams want to relay multiple inventories at once, consider adding an explicit overload that accepts a vector and moves it into place.
Outside the shown lines, add:
MessageProcessingResult(std::vector<CInv> invs) : m_inventory(std::move(invs)) { }src/chainlock/signing.h (1)
33-33: Fix clang-format issues on this signature to unblock CIThe Clang Diff Format Check is failing on this file. Reflow the function declaration to conform to the project’s style (wrap parameters). Then run the project’s clang-format script.
Apply this diff to the declaration:
- [[nodiscard]] virtual MessageProcessingResult ProcessNewChainLock(NodeId from, const ChainLockSig& clsig, const uint256& hash) = 0; + [[nodiscard]] virtual MessageProcessingResult ProcessNewChainLock( + NodeId from, + const ChainLockSig& clsig, + const uint256& hash) = 0;After updating, run clang-format (or the repo’s helper) to ensure no further diffs remain.
src/llmq/signing.cpp (1)
445-446: Consider bounding per-peer pending queue to reduce DoS surfacependingRecoveredSigs[from] can grow unbounded if a peer floods well-formed-but-invalid messages that pass pre-checks. Consider a small per-peer cap with drop/trim behavior.
Apply a cap, e.g., 1024 per peer:
- pendingRecoveredSigs[from].emplace_back(recoveredSig); + auto& q = pendingRecoveredSigs[from]; + static constexpr size_t kMaxPerPeerPending = 1024; + if (q.size() < kMaxPerPeerPending) { + q.emplace_back(std::move(recoveredSig)); + } else { + // Optional: log once per period or penalize lightly + // LogPrint(BCLog::LLMQ, "... dropping excess pending recSig from peer=%d\n", from); + }src/evo/mnauth.h (1)
58-69: Fix clang-format diff failure by reflowing the multi-line declarationClang-format is flagging this section. Rewrap the parameters to match our style and unblock CI.
Apply this diff for the declaration:
- [[nodiscard]] static MessageProcessingResult ProcessMessage(CNode& peer, ServiceFlags node_services, CConnman& connman, CMasternodeMetaMan& mn_metaman, - const CActiveMasternodeManager* const mn_activeman, const CMasternodeSync& mn_sync, - const CDeterministicMNList& tip_mn_list, std::string_view msg_type, CDataStream& vRecv); + [[nodiscard]] static MessageProcessingResult ProcessMessage( + CNode& peer, + ServiceFlags node_services, + CConnman& connman, + CMasternodeMetaMan& mn_metaman, + const CActiveMasternodeManager* const mn_activeman, + const CMasternodeSync& mn_sync, + const CDeterministicMNList& tip_mn_list, + std::string_view msg_type, + CDataStream& vRecv);src/llmq/blockprocessor.cpp (1)
150-152: Correctly handle optional inventory from AddMineableCommitment (LGTM)Guarding push_back on the optional return prevents inserting a dummy CInv. Minor nit below for push_back.
You can avoid an extra copy:
- if (auto inv_opt = AddMineableCommitment(qc)) { - ret.m_inventory.push_back(inv_opt.value()); - } + if (const auto inv_opt = AddMineableCommitment(qc)) { + ret.m_inventory.emplace_back(*inv_opt); + }src/coinjoin/server.h (2)
89-91: Format multi-line method declaration to satisfy clang-formatCI reports clang-format differences in this region. Reflow the signature.
- [[nodiscard]] MessageProcessingResult ProcessDSQUEUE(NodeId from, CDataStream& vRecv) EXCLUSIVE_LOCKS_REQUIRED(!cs_vecqueue); + [[nodiscard]] MessageProcessingResult ProcessDSQUEUE( + NodeId from, CDataStream& vRecv) EXCLUSIVE_LOCKS_REQUIRED(!cs_vecqueue);
115-116: ProcessMessage return type migration looks goodSwitching to [[nodiscard]] MessageProcessingResult here matches the new PostProcessMessage flow.
To placate clang-format and match our wrapping style:
- [[nodiscard]] MessageProcessingResult ProcessMessage(CNode& pfrom, std::string_view msg_type, CDataStream& vRecv); + [[nodiscard]] MessageProcessingResult ProcessMessage( + CNode& pfrom, std::string_view msg_type, CDataStream& vRecv);src/spork.h (2)
260-261: ProcessMessage: migration to [[nodiscard]] MessageProcessingResult is correctSignature changes align with the new centralized result handling; PeerManager parameter removal matches spork.cpp usage.
Reflow to address the clang-format diff check:
- [[nodiscard]] MessageProcessingResult ProcessMessage(CNode& peer, CConnman& connman, std::string_view msg_type, CDataStream& vRecv); + [[nodiscard]] MessageProcessingResult ProcessMessage( + CNode& peer, CConnman& connman, std::string_view msg_type, CDataStream& vRecv);
268-269: ProcessSpork: NodeId-based input and [[nodiscard]] are appropriateThis matches the call site in spork.cpp and the general NodeId-based pattern.
Match our wrapping style for thread-safety annotations and long signatures:
- [[nodiscard]] MessageProcessingResult ProcessSpork(NodeId from, CDataStream& vRecv) EXCLUSIVE_LOCKS_REQUIRED(!cs); + [[nodiscard]] MessageProcessingResult ProcessSpork( + NodeId from, CDataStream& vRecv) EXCLUSIVE_LOCKS_REQUIRED(!cs);src/coinjoin/client.h (1)
253-260: Fix clang-format diff failure by wrapping the declarationCI flagged this region. Reflow the long signature to match our style.
- [[nodiscard]] MessageProcessingResult ProcessMessage(NodeId from, CConnman& connman, PeerManager& peerman, std::string_view msg_type, - CDataStream& vRecv) - EXCLUSIVE_LOCKS_REQUIRED(!cs_vecqueue); + [[nodiscard]] MessageProcessingResult ProcessMessage( + NodeId from, + CConnman& connman, + PeerManager& peerman, + std::string_view msg_type, + CDataStream& vRecv) EXCLUSIVE_LOCKS_REQUIRED(!cs_vecqueue);src/governance/governance.h (1)
274-278: Nit: add <string_view> include in the header (IWYU)This header uses std::string_view in function declarations but doesn’t directly include <string_view>. Consider adding it for better IWYU hygiene.
src/coinjoin/server.cpp (1)
183-187: Nit: assert peerman before use to mirror surrounding codeElsewhere in this file, Relay/Inv calls often guard with Assert(m_peerman). Consider doing the same here for consistency and clearer assumptions.
Apply this minor tweak:
- m_peerman->RelayDSQ(dsq); + Assert(m_peerman)->RelayDSQ(dsq);src/llmq/dkgsessionhandler.h (1)
66-90: Good API shift to MessageProcessingResult and NodeId; minor type-safety nit with invTypeThe migration away from PeerMsgRet and CNode* looks correct and consistent with net_processing patterns. One minor nit: invType is now uint32_t, while CInv typically takes an int for the type. Where invType is later used to construct CInv, consider an explicit cast to avoid potential narrowing surprises at call sites.
If you keep invType as uint32_t, ensure CInv construction uses an explicit cast at its uses:
- In src/llmq/dkgsessionhandler.cpp Line 72:
- ret.m_to_erase = CInv{invType, hash}; + ret.m_to_erase = CInv{static_cast<int>(invType), hash};src/llmq/dkgsessionhandler.cpp (1)
70-73: Be explicit about CInv type to avoid narrowing warningsret.m_to_erase uses invType (uint32_t) to construct CInv. While this currently works, making the conversion explicit improves readability and avoids potential narrowing pitfalls.
Apply:
- if (from != -1) { - ret.m_to_erase = CInv{invType, hash}; - } + if (from != -1) { + ret.m_to_erase = CInv{static_cast<int>(invType), hash}; + }src/governance/governance.cpp (1)
143-169: Uniform MessageProcessingResult usage looks correct; remove unreachable returnThe refactor in CGovernanceManager::ProcessMessage to return MessageProcessingResult is consistent and well-scoped. There’s a small unreachable return after both branches return in the MNGOVERNANCESYNC block.
Apply:
if (nProp == uint256()) { return SyncObjects(peer, connman); } else { return SyncSingleObjVotes(peer, nProp, filter, connman); } - - return {};src/llmq/dkgsessionmgr.cpp (6)
112-119: Add informative messages to MisbehavingError for observabilityReturning a score without a message makes triage harder downstream (net_processing logs will lack context).
Apply a message to the error:
- return MisbehavingError{10}; + return MisbehavingError{10, "QWATCH received by non-masternode"};
121-124: Also include a reason in misbehavior hereSame rationale as above: helps when reading logs/stats.
- return MisbehavingError{10}; + return MisbehavingError{10, "DKG message received by regular non-watching node"};
126-129: Empty payload should report why it’s invalidGood to reject, but attach a message to aid debugging.
- if (vRecv.empty()) { - return MisbehavingError{100}; - } + if (vRecv.empty()) { + return MisbehavingError{100, "empty DKG message payload"}; + }
130-136: Peek-then-rewind is correct; consider guarding rewinds with static_assertsThe read→rewind order is correct. As a small safety net for future refactors, consider guarding sizes with static_asserts to ensure type sizes don’t accidentally change.
Example:
- static_assert(sizeof(uint8_t) == 1, ...)
- static_assert(sizeof(uint256) == 32, ...)
137-143: Invalid llmqType: add reason to MisbehavingErrorYou already log it; include the reason in the error too for consistency with other handlers in this PR.
- return MisbehavingError{100}; + return MisbehavingError{100, "invalid llmqType"};
155-163: Unknown quorumHash returns low DoS score – good; add reason stringThis matches the “might be lagging” comment. Include the reason as a message.
- return MisbehavingError{10}; + return MisbehavingError{10, "unknown quorumHash (peer might be lagging)"};src/spork.cpp (1)
149-153: Attach reason strings to MisbehavingError for better diagnosticsAdd messages to the errors to aid logging and telemetry. Other modules in this PR include reason strings.
- ret.m_error = MisbehavingError{100}; + ret.m_error = MisbehavingError{100, "spork time too far in the future"}; @@ - ret.m_error = MisbehavingError{100}; + ret.m_error = MisbehavingError{100, "invalid spork signature"};Also applies to: 157-161
src/net_processing.cpp (1)
653-655: Central PostProcessMessage is correctly wired to the new result structure
- m_error, m_to_erase, m_transactions handling matches prior behavior.
- Looping over m_inventory and relaying each item is correct.
Optional: consider de-duplicating inventories within a single result before relaying to reduce redundant inv announcements if callers ever add duplicates.
Also applies to: 3498-3507
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (32)
contrib/devtools/copyright_header.py(0 hunks)src/Makefile.am(0 hunks)src/chainlock/signing.h(1 hunks)src/coinjoin/client.cpp(5 hunks)src/coinjoin/client.h(2 hunks)src/coinjoin/server.cpp(3 hunks)src/coinjoin/server.h(3 hunks)src/evo/mnauth.cpp(3 hunks)src/evo/mnauth.h(1 hunks)src/governance/governance.cpp(16 hunks)src/governance/governance.h(1 hunks)src/instantsend/instantsend.cpp(2 hunks)src/instantsend/instantsend.h(1 hunks)src/llmq/blockprocessor.cpp(1 hunks)src/llmq/blockprocessor.h(1 hunks)src/llmq/dkgsessionhandler.cpp(3 hunks)src/llmq/dkgsessionhandler.h(4 hunks)src/llmq/dkgsessionmgr.cpp(5 hunks)src/llmq/dkgsessionmgr.h(1 hunks)src/llmq/quorums.cpp(2 hunks)src/llmq/quorums.h(1 hunks)src/llmq/signing.cpp(1 hunks)src/llmq/signing.h(1 hunks)src/net_processing.cpp(8 hunks)src/net_processing.h(1 hunks)src/protocol.h(2 hunks)src/spork.cpp(3 hunks)src/spork.h(1 hunks)src/util/expected.h(0 hunks)test/lint/lint-include-guards.py(1 hunks)test/lint/lint-spelling.py(1 hunks)test/lint/lint-whitespace.py(0 hunks)
💤 Files with no reviewable changes (4)
- src/Makefile.am
- contrib/devtools/copyright_header.py
- test/lint/lint-whitespace.py
- src/util/expected.h
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#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.
🧬 Code Graph Analysis (25)
src/llmq/blockprocessor.h (3)
src/llmq/blockprocessor.cpp (2)
ProcessMessage(57-154)ProcessMessage(57-58)src/llmq/dkgsessionmgr.cpp (2)
ProcessMessage(95-187)ProcessMessage(95-96)src/llmq/quorums.cpp (2)
ProcessMessage(706-889)ProcessMessage(706-706)
src/coinjoin/server.h (2)
src/coinjoin/server.cpp (4)
ProcessDSQUEUE(112-189)ProcessDSQUEUE(112-112)ProcessMessage(26-41)ProcessMessage(26-26)src/coinjoin/client.cpp (6)
ProcessMessage(41-142)ProcessMessage(41-42)ProcessMessage(144-166)ProcessMessage(144-144)ProcessMessage(183-230)ProcessMessage(183-183)
src/chainlock/signing.h (1)
src/chainlock/chainlock.cpp (2)
ProcessNewChainLock(127-192)ProcessNewChainLock(127-128)
src/llmq/dkgsessionmgr.h (3)
src/net_processing.cpp (12)
ProcessMessage(3510-5294)ProcessMessage(3510-3515)pfrom(616-616)pfrom(639-640)pfrom(722-724)pfrom(730-730)pfrom(738-738)pfrom(741-741)pfrom(743-743)pfrom(745-745)pfrom(838-838)pfrom(1020-1020)src/llmq/dkgsessionmgr.cpp (2)
ProcessMessage(95-187)ProcessMessage(95-96)src/llmq/dkgsessionhandler.cpp (2)
ProcessMessage(155-168)ProcessMessage(155-155)
src/spork.h (2)
src/spork.cpp (4)
ProcessMessage(129-137)ProcessMessage(129-129)ProcessSpork(139-193)ProcessSpork(139-139)src/net_processing.cpp (25)
ProcessMessage(3510-5294)ProcessMessage(3510-3515)peer(668-668)peer(797-797)peer(902-902)peer(903-903)peer(1010-1010)peer(1018-1018)from(1023-1023)EXCLUSIVE_LOCKS_REQUIRED(643-643)EXCLUSIVE_LOCKS_REQUIRED(644-644)EXCLUSIVE_LOCKS_REQUIRED(646-646)EXCLUSIVE_LOCKS_REQUIRED(647-647)EXCLUSIVE_LOCKS_REQUIRED(651-651)EXCLUSIVE_LOCKS_REQUIRED(657-657)EXCLUSIVE_LOCKS_REQUIRED(660-660)EXCLUSIVE_LOCKS_REQUIRED(739-739)EXCLUSIVE_LOCKS_REQUIRED(759-759)EXCLUSIVE_LOCKS_REQUIRED(818-818)EXCLUSIVE_LOCKS_REQUIRED(820-820)EXCLUSIVE_LOCKS_REQUIRED(825-825)EXCLUSIVE_LOCKS_REQUIRED(827-827)EXCLUSIVE_LOCKS_REQUIRED(828-828)EXCLUSIVE_LOCKS_REQUIRED(836-836)EXCLUSIVE_LOCKS_REQUIRED(837-837)
src/llmq/quorums.h (1)
src/llmq/quorums.cpp (2)
ProcessMessage(706-889)ProcessMessage(706-706)
src/coinjoin/client.h (1)
src/coinjoin/client.cpp (6)
ProcessMessage(41-142)ProcessMessage(41-42)ProcessMessage(144-166)ProcessMessage(144-144)ProcessMessage(183-230)ProcessMessage(183-183)
src/llmq/signing.h (1)
src/llmq/signing.cpp (2)
ProcessMessage(407-447)ProcessMessage(407-407)
src/governance/governance.h (2)
src/governance/governance.cpp (6)
SyncSingleObjVotes(922-972)SyncSingleObjVotes(922-922)SyncObjects(974-1034)SyncObjects(974-974)ProcessMessage(143-283)ProcessMessage(143-143)src/spork.cpp (2)
ProcessMessage(129-137)ProcessMessage(129-129)
src/net_processing.h (1)
src/net_processing.cpp (14)
RelayInv(2345-2356)RelayInv(2345-2345)RelayInv(2358-2365)RelayInv(2358-2358)inv(630-630)inv(631-631)inv(632-632)inv(633-633)inv(928-928)inv(2307-2307)RelayInvFiltered(2395-2418)RelayInvFiltered(2395-2395)RelayInvFiltered(2420-2443)RelayInvFiltered(2420-2420)
src/llmq/quorums.cpp (3)
src/llmq/blockprocessor.cpp (2)
ProcessMessage(57-154)ProcessMessage(57-58)src/net_processing.cpp (13)
ProcessMessage(3510-5294)ProcessMessage(3510-3515)pfrom(616-616)pfrom(639-640)pfrom(722-724)pfrom(730-730)pfrom(738-738)pfrom(741-741)pfrom(743-743)pfrom(745-745)pfrom(838-838)pfrom(1020-1020)ret(654-654)src/llmq/dkgsessionmgr.cpp (2)
ProcessMessage(95-187)ProcessMessage(95-96)
src/protocol.h (2)
src/spork.cpp (1)
inv(461-461)src/net_processing.cpp (6)
inv(630-630)inv(631-631)inv(632-632)inv(633-633)inv(928-928)inv(2307-2307)
src/llmq/dkgsessionmgr.cpp (6)
src/llmq/dkgsessionhandler.cpp (2)
ProcessMessage(155-168)ProcessMessage(155-155)src/llmq/quorums.cpp (2)
ProcessMessage(706-889)ProcessMessage(706-706)src/llmq/commitment.h (1)
quorumIndex(47-47)src/llmq/dkgsession.h (1)
quorumIndex(296-296)src/evo/mnhftx.h (1)
quorumHash(35-35)src/evo/assetlocktx.h (1)
quorumHash(81-81)
src/llmq/dkgsessionhandler.cpp (3)
src/llmq/signing.cpp (3)
ret(31-31)ProcessMessage(407-447)ProcessMessage(407-407)src/llmq/blockprocessor.cpp (2)
ProcessMessage(57-154)ProcessMessage(57-58)src/llmq/dkgsessionmgr.cpp (2)
ProcessMessage(95-187)ProcessMessage(95-96)
src/evo/mnauth.h (3)
src/governance/governance.cpp (2)
ProcessMessage(143-283)ProcessMessage(143-143)src/evo/mnauth.cpp (2)
ProcessMessage(58-189)ProcessMessage(58-60)src/net_processing.cpp (8)
ProcessMessage(3510-5294)ProcessMessage(3510-3515)peer(668-668)peer(797-797)peer(902-902)peer(903-903)peer(1010-1010)peer(1018-1018)
src/governance/governance.cpp (3)
src/llmq/blockprocessor.cpp (2)
ProcessMessage(57-154)ProcessMessage(57-58)src/coinjoin/server.cpp (2)
ProcessMessage(26-41)ProcessMessage(26-26)src/net_processing.cpp (9)
ProcessMessage(3510-5294)ProcessMessage(3510-3515)peer(668-668)peer(797-797)peer(902-902)peer(903-903)peer(1010-1010)peer(1018-1018)ret(654-654)
src/instantsend/instantsend.h (1)
src/instantsend/instantsend.cpp (2)
ProcessMessage(109-172)ProcessMessage(109-109)
src/net_processing.cpp (4)
src/coinjoin/server.cpp (3)
inv(354-354)ProcessMessage(26-41)ProcessMessage(26-26)src/governance/governance.cpp (2)
ProcessMessage(143-283)ProcessMessage(143-143)src/evo/mnauth.cpp (2)
ProcessMessage(58-189)ProcessMessage(58-60)src/coinjoin/client.cpp (6)
ProcessMessage(41-142)ProcessMessage(41-42)ProcessMessage(144-166)ProcessMessage(144-144)ProcessMessage(183-230)ProcessMessage(183-183)
src/llmq/signing.cpp (4)
src/llmq/blockprocessor.cpp (2)
ProcessMessage(57-154)ProcessMessage(57-58)src/instantsend/instantsend.cpp (2)
ProcessMessage(109-172)ProcessMessage(109-109)src/llmq/dkgsessionmgr.cpp (2)
ProcessMessage(95-187)ProcessMessage(95-96)src/llmq/signing_shares.cpp (2)
ProcessMessage(219-298)ProcessMessage(219-220)
src/coinjoin/client.cpp (3)
src/llmq/blockprocessor.cpp (2)
ProcessMessage(57-154)ProcessMessage(57-58)src/spork.cpp (2)
ProcessMessage(129-137)ProcessMessage(129-129)src/llmq/dkgsessionmgr.cpp (2)
ProcessMessage(95-187)ProcessMessage(95-96)
src/spork.cpp (3)
src/llmq/blockprocessor.cpp (2)
ProcessMessage(57-154)ProcessMessage(57-58)src/coinjoin/server.cpp (2)
ProcessMessage(26-41)ProcessMessage(26-26)src/spork.h (1)
cs(230-340)
src/coinjoin/server.cpp (4)
src/llmq/blockprocessor.cpp (2)
ProcessMessage(57-154)ProcessMessage(57-58)src/coinjoin/client.cpp (6)
ProcessMessage(41-142)ProcessMessage(41-42)ProcessMessage(144-166)ProcessMessage(144-144)ProcessMessage(183-230)ProcessMessage(183-183)src/net_processing.cpp (10)
ProcessMessage(3510-5294)ProcessMessage(3510-3515)peer(668-668)peer(797-797)peer(902-902)peer(903-903)peer(1010-1010)peer(1018-1018)from(1023-1023)ret(654-654)src/llmq/quorums.cpp (2)
ProcessMessage(706-889)ProcessMessage(706-706)
src/evo/mnauth.cpp (2)
src/llmq/signing.cpp (2)
ProcessMessage(407-447)ProcessMessage(407-407)test/functional/p2p_quorum_data.py (1)
mnauth(91-98)
src/instantsend/instantsend.cpp (3)
src/net_processing.cpp (6)
ProcessMessage(3510-5294)ProcessMessage(3510-3515)from(1023-1023)ret(654-654)WITH_LOCK(3155-3155)WITH_LOCK(3177-3177)src/llmq/dkgsessionmgr.cpp (2)
ProcessMessage(95-187)ProcessMessage(95-96)src/llmq/signing.cpp (3)
ProcessMessage(407-447)ProcessMessage(407-407)ret(31-31)
src/llmq/dkgsessionhandler.h (1)
src/llmq/dkgsessionhandler.cpp (4)
PushPendingMessage(61-91)PushPendingMessage(61-61)ProcessMessage(155-168)ProcessMessage(155-155)
🪛 GitHub Actions: Clang Diff Format Check
src/coinjoin/server.h
[error] 87-94: Clang-format differences detected by clang-format-diff. Please run the formatting tool (clang-format or the provided script) to fix formatting in this file.
src/chainlock/signing.h
[error] 30-37: Clang-format differences detected by clang-format-diff. Please run the formatting tool (clang-format or the provided script) to fix formatting in this file.
src/spork.h
[error] 257-264: Clang-format differences detected by clang-format-diff. Please run the formatting tool (clang-format or the provided script) to fix formatting in this file.
src/llmq/quorums.h
[error] 275-282: Clang-format differences detected by clang-format-diff. Please run the formatting tool (clang-format or the provided script) to fix formatting in this file.
src/coinjoin/client.h
[error] 253-260: Clang-format differences detected by clang-format-diff. Please run the formatting tool (clang-format or the provided script) to fix formatting in this file.
src/governance/governance.h
[error] 271-282: Clang-format differences detected by clang-format-diff. Please run the formatting tool (clang-format or the provided script) to fix formatting in this file.
src/llmq/quorums.cpp
[error] 703-710: Clang-format differences detected by clang-format-diff. Please run the formatting tool (clang-format or the provided script) to fix formatting in this file.
src/evo/mnauth.h
[error] 58-69: Clang-format differences detected by clang-format-diff. Please run the formatting tool (clang-format or the provided script) to fix formatting in this file.
src/governance/governance.cpp
[error] 140-147: Clang-format differences detected by clang-format-diff. Please run the formatting tool (clang-format or the provided script) to fix formatting in this file.
src/llmq/signing.cpp
[error] 432-447: Clang-format differences detected by clang-format-diff. Please run the formatting tool (clang-format or the provided script) to fix formatting in this file.
src/coinjoin/client.cpp
[error] 84-91: Clang-format differences detected by clang-format-diff. Please run the formatting tool (clang-format or the provided script) to fix formatting in this file.
src/spork.cpp
[error] 126-133: Clang-format differences detected by clang-format-diff. Please run the formatting tool (clang-format or the provided script) to fix formatting in this file.
src/coinjoin/server.cpp
[error] 145-154: Clang-format differences detected by clang-format-diff. Please run the formatting tool (clang-format or the provided script) to fix formatting in this file.
src/evo/mnauth.cpp
[error] 55-65: Clang-format differences detected by clang-format-diff. Please run the formatting tool (clang-format or the provided script) to fix formatting in this file.
src/llmq/dkgsessionhandler.h
[error] 73-80: Clang-format differences detected by clang-format-diff. Please run the formatting tool (clang-format or the provided script) to fix formatting in this file.
⏰ 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: linux64_tsan-build / Build source
- GitHub Check: win64-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: arm-linux-build / Build source
🔇 Additional comments (47)
test/lint/lint-spelling.py (1)
15-15: Removal of obsolete exclude looks goodDropping src/util/expected.h from the codespell exclusion list is appropriate given the header’s removal. No other issues spotted.
test/lint/lint-include-guards.py (1)
33-33: Include-guards lint: exclusion removal is correctRemoving src/util/expected.h from EXCLUDE_FILES_WITH_PREFIX aligns with the header’s removal. The remaining exclusions are unchanged and consistent.
src/llmq/signing.cpp (3)
407-418: Good migration to MessageProcessingResult and NodeId; consistent with the new post-processing flowThe unified handling for QSIGREC with ret.m_to_erase set and early bailouts looks correct and aligns with the pattern used elsewhere (e.g., ISLOCK).
419-426: Correct ban signaling via MisbehavingError; safe no-penalty fallback when appropriatePropagating ban via MisbehavingError{100} only when PreVerifyRecoveredSig requests it is consistent. Returning the prepared ret (with m_to_erase) for non-ban cases avoids redundant processing.
432-447: Fix Clang-format issues to unblock CIThe formatting check is failing for this hunk. Please run the project's clang-format tooling to normalize this section.
CI failure reference: GitHub Actions Clang Diff Format Check reported differences on lines 432-447 in this file.
src/llmq/blockprocessor.h (1)
56-56: [[nodiscard]] on ProcessMessage is appropriateMarking ProcessMessage as [[nodiscard]] enforces handling of MessageProcessingResult by callers. Good addition.
src/llmq/dkgsessionmgr.h (2)
91-92: Signature update looks correct and aligns with net_processing usageSwitching to [[nodiscard]] MessageProcessingResult, removing PeerManager from the signature, and using std::string_view is consistent with the refactor and the call site in net_processing.
91-92: CDKGSessionManager::ProcessMessage migration verified — no remaining call sites or PeerMsgRet usage foundVerified evidence:
- src/llmq/dkgsessionmgr.h (lines 91–92): ProcessMessage signature updated to return MessageProcessingResult and accept (CNode&, bool is_masternode, std::string_view, CDataStream&).
- src/llmq/dkgsessionmgr.cpp (definition at line ~95) implements the updated signature.
- Call site updated: src/net_processing.cpp:5270 calls m_llmq_ctx->qdkgsman->ProcessMessage(pfrom, is_masternode, msg_type, vRecv).
- Search for the old typedef/name returned no hits: no occurrences of PeerMsgRet found.
Conclusion: All call sites and the old PeerMsgRet typedef appear migrated — no further changes required.
src/llmq/quorums.h (2)
278-279: Good: [[nodiscard]] and string_view on ProcessMessage standardize the interfaceThis matches the broader migration to MessageProcessingResult and encourages caller-side post-processing and error handling.
275-282: Resolve Clang-format diff complaintsCI flagged formatting differences for this hunk. Please run the repository’s clang-format script to reformat.
CI failure reference: Clang Diff Format Check reported differences on lines 275-282 in this file.
src/llmq/quorums.cpp (2)
706-889: Refactor to MessageProcessingResult with centralized error handling is sound
- errorHandler lambda returning MessageProcessingResult is a clean replacement for tl::unexpected paths.
- The sendQDATA helper correctly returns a MessageProcessingResult and encapsulates response sending logic while optionally penalizing on request limits.
- QGETDATA/QDATA branches maintain previous behavior while adopting the new result flow.
703-710: Fix Clang-format issues to satisfy CIFormatting differences were detected for this area. Please run clang-format as per the project guidelines.
CI failure reference: Clang Diff Format Check reported differences on lines 703-710 in this file.
src/evo/mnauth.h (1)
61-63: API migration to MessageProcessingResult is correct and consistentReturning [[nodiscard]] MessageProcessingResult here aligns with the project's new centralized post-processing pattern. Signature and includes look right.
src/instantsend/instantsend.h (1)
134-134: Good refactor: decoupled API and unified result typeSwitching to [[nodiscard]] MessageProcessingResult and NodeId cleanly decouples from CNode/PeerManager in the public path and aligns with the new post-processing model.
src/governance/governance.h (2)
274-278: LGTM: move to [[nodiscard]] MessageProcessingResult across GovernanceThe API surface now consistently returns MessageProcessingResult for sync and processing paths. Keeping PeerManager in ProcessMessage is fine given current call sites in governance.cpp.
274-278: Fix clang-format diffs flagged by CIClang Diff Format Check is failing for this file (Lines 271-282 per CI). Please run the project’s clang-format tooling to resolve.
You can run either:
- If you have git-clang-format: git clang-format HEAD~1
- Or the repo’s helper (if present): contrib/devtools/clang-format.sh
- Or plain clang-format: clang-format -i src/governance/governance.h
Then re-push to re-run CI.
src/net_processing.h (1)
101-112: Const-correctness on RelayInv/RelayInvFiltered looks goodAccepting CInv by const reference avoids unnecessary copies and reinforces intent that inv is read-only. Upstream implementation in net_processing.cpp matches.
src/evo/mnauth.cpp (3)
58-61: Adoption of MessageProcessingResult + early guard is correctReturning {} when msg_type != MNAUTH or MN list isn’t synced mirrors how other modules short-circuit. Signature and return-type migration is consistent with the project-wide refactor.
74-90: Direct MisbehavingError returns fit the new result modelReturning MisbehavingError{...} directly (duplicate mnauth, invalid services, empty proRegTxHash, invalid signature) aligns with the new conversion-to-MessageProcessingResult pattern used elsewhere.
58-61: Fix clang-format diffs flagged by CICI indicates clang-format differences (Lines 55-65). Please run the formatter to bring the signature/line-wrapping into compliance.
Commands (pick one):
- git clang-format HEAD~1
- contrib/devtools/clang-format.sh
- clang-format -i src/evo/mnauth.cpp
src/coinjoin/server.cpp (3)
26-41: ProcessMessage: good migration to MessageProcessingResult and NodeId for DSQUEUEReturning the module-specific DSQUEUE result up the stack via MessageProcessingResult is consistent with the new PostProcessMessage flow. Other message types continue to be handled internally as before.
112-121: Consistent result struct initialization for DSQUEUEInitializing ret and setting m_to_erase to MSG_DSQ dsq hash ensures request tracking/cleanup is deferred to the centralized post-processor. Matches the new pattern elsewhere.
145-154: Fix clang-format diffs flagged by CICI reported formatting differences for this region. After adjusting the specifier above, please run clang-format to address any remaining wrap/spacing issues.
Run:
- git clang-format HEAD~1
- or contrib/devtools/clang-format.sh
- or clang-format -i src/coinjoin/server.cpp
src/llmq/dkgsessionhandler.h (3)
85-90: Template PushPendingMessage correctly routes result via peerman.PostProcessMessageSerializing Message to CDataStream and forwarding MessageProcessingResult to peerman.PostProcessMessage is the right abstraction. No issues here.
73-80: Fix clang-format drift flagged by CIThe CI reports clang-format differences for this hunk. Please run clang-format (or the repo’s format script) to normalize formatting.
The pipeline shows: GitHub Actions: Clang Diff Format Check — lines 73-80 in this file. Running the project’s formatting tool locally should resolve it.
168-169: ProcessMessage signature update looks consistent[[nodiscard]] MessageProcessingResult ProcessMessage(NodeId, std::string_view, CDataStream&) aligns with the refactor and net_processing expectations.
src/llmq/dkgsessionhandler.cpp (1)
155-168: ProcessMessage refactor is correct and side-effect free in the net threadDeferring heavy work and queuing by returning MessageProcessingResult per type is aligned with the new PostProcessMessage flow. Looks good.
src/coinjoin/client.cpp (3)
41-49: Correct early-exit gating and msg_type check with structured resultShifting to MessageProcessingResult with an early gate on role/sync and msg_type is consistent with other modules. Good.
56-62: DSQ erasure signaling via ret.m_to_erase is correctSetting ret.m_to_erase to MSG_DSQ, dsq.GetHash() properly centralizes request erasure in net_processing. Good.
84-91: Resolve clang-format issues flagged by CICI reports clang-format differences for this region. After applying the format-specifier fix above, please run clang-format to satisfy the style check.
The pipeline shows: GitHub Actions: Clang Diff Format Check — lines 84-91 in this file. Running the repo’s format tool should clear it.
src/governance/governance.cpp (5)
178-238: Governance object handling: ret.m_to_erase and error-paths are consistentUsing ret.m_to_erase for MSG_GOVERNANCE_OBJECT and MisbehavingError for invalid cases matches the new pattern. Good.
247-280: Vote handling: structured erasure and penalties are correctm_to_erase set to MSG_GOVERNANCE_OBJECT_VOTE with selective MisbehavingError based on exception penalty is correct and keeps behavior centralized.
974-985: Returning MisbehavingError from SyncObjects is appropriate for repeated full sync requestsReturning MisbehavingError{20} on repeated fulfilled MNGOVERNANCESYNC is consistent with prior behavior. No change requested.
996-1034: Inventory accumulation into ret.m_inventory is correct; no inline pushesBuilding ret.m_inventory for MSG_GOVERNANCE_OBJECT and emitting SYNCSTATUSCOUNT via connman is consistent with the global post-processing pipeline.
140-147: Fix clang-format issues flagged by CICI detected clang-format differences in this region. Please run the project’s formatting tool to clear the formatting check.
The pipeline shows: GitHub Actions: Clang Diff Format Check — lines 140-147 in this file.
src/instantsend/instantsend.cpp (1)
109-172: Solid refactor to MessageProcessingResult; correct gating and centralized erasure/penalties
- Early-exit on feature flag and msg_type is right.
- ret.m_to_erase with MSG_ISDLOCK and computed hash is correct.
- MisbehavingError usage differentiates severity (1 for missing block, 100 for invalid) appropriately.
- Enqueuing by NodeId into pendingInstantSendLocks preserves prior logic with the new API.
Looks good.
src/llmq/dkgsessionmgr.cpp (3)
95-103: Nice API migration and early gating; aligns with MessageProcessingResult patternThe new signature and early-out on disabled DKG via spork look good and consistent with the PR-wide refactor.
104-111: Explicit message-type allowlist is a good guardrailRestricting handling to QCONTRIB/QCOMPLAINT/QJUSTIFICATION/QPCOMMITMENT/QWATCH prevents accidental parsing of unrelated payloads.
185-187: Good: NodeId-based dispatch to per-quorum handlerThis matches the new CDKGSessionHandler::ProcessMessage(NodeId, ...) interface and removes PeerManager coupling.
src/spork.cpp (4)
129-137: Clang-format check is failing; please run the formatterCI reports clang-format differences around this area. Please run the project’s clang-format script (or clang-format-diff) to fix styling before merging.
If you want me to generate the exact clang-format-diff patch for this hunk, let me know.
139-148: Result object initialization and to_erase use look correctInitializing ret and setting m_to_erase upfront aligns with the centralized post-processing flow in net_processing.
163-165: Logging context updated to NodeId — goodThis keeps consistency across modules after switching to NodeId-based processing/logging.
191-193: Inventory relay enqueued via ret.m_inventory — goodUsing ret.m_inventory to signal broadcast is consistent with the new PostProcessMessage handler.
src/net_processing.cpp (4)
630-634: Const-correctness for RelayInv/RelayInvFiltered is a solid improvementUpdating both the interface and implementation to take const CInv& (and const in related args) prevents accidental mutations and matches usage patterns.
Also applies to: 2345-2366, 2358-2366, 2395-2443
5259-5259: Call-site rewiring to PostProcessMessage looks coherent across subsystems
- CoinJoin queue/server, spork, governance, mnauth, llmq (blockprocessor, dkg, quorums, signing, instantsend) now consistently return MessageProcessingResult and are funneled through PostProcessMessage using NodeId.
- Mixed CNode& vs NodeId parameters match each module’s updated signatures.
No issues spotted; good cohesion with the refactor.
Also applies to: 5265-5266, 5268-5269, 5271-5272, 5274-5274, 5286-5286
2345-2356: RelayInv(minProtoVersion): behavior preserved; const and filtering correctThe lambda captures and GetPeerRef usage remain safe; pushing via PushInv keeps existing known-filter semantics.
2395-2443: RelayInvFiltered overloads: bloom-filter checks and minProto gating intactBoth relatedTx and relatedTxHash variants preserve prior behavior with improved constness. Good.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/coinjoin/server.cpp (1)
148-149: Correct NodeId printf specifier — LGTMThe format specifier for NodeId is now %d. This resolves the prior issue.
src/coinjoin/client.cpp (1)
86-90: Correct NodeId printf specifier — LGTMThe logging now uses %d for NodeId; consistent with the codebase.
src/llmq/dkgsessionmgr.cpp (1)
164-181: On adding descriptive error messages: acknowledged out of scopeAdding textual messages to
MisbehavingError{...}here was suggested previously but deemed out of scope for this refactor by the PR author. No action requested.
🧹 Nitpick comments (11)
src/llmq/dkgsessionhandler.h (1)
74-77: Apply clang-format to fix formatting issues.The pipeline indicates formatting issues in this section. Please apply the formatting changes.
Run clang-format on these lines to ensure consistent code formatting:
- explicit CDKGPendingMessages(size_t _maxMessagesPerNode, uint32_t _invType) : - invType(_invType), maxMessagesPerNode(_maxMessagesPerNode) {}; + explicit CDKGPendingMessages(size_t _maxMessagesPerNode, uint32_t _invType) + : invType(_invType), maxMessagesPerNode(_maxMessagesPerNode){};src/llmq/quorums.cpp (1)
706-712: Apply clang-format to fix formatting issues.The pipeline indicates formatting issues in the errorHandler lambda definition.
Apply the following formatting:
MessageProcessingResult CQuorumManager::ProcessMessage(CNode& pfrom, CConnman& connman, std::string_view msg_type, CDataStream& vRecv) { auto strFunc = __func__; - auto errorHandler = [&](const std::string& strError, int nScore = 10) -> MessageProcessingResult { + auto errorHandler = [&](const std::string& strError, int nScore = 10) + -> MessageProcessingResult { LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- %s: %s, from peer=%d\n", strFunc, msg_type, strError, pfrom.GetId()); if (nScore > 0) { return MisbehavingError{nScore};src/coinjoin/server.h (1)
86-93: Fix clang-format diffs flagged by CIGitHub Actions reports clang-format differences for this region. Please run clang-format (or git clang-format) to match project style so CI passes.
src/evo/mnauth.h (1)
58-69: Address clang-format issues in this blockCI flagged formatting differences for this region. Please apply clang-format to resolve.
src/coinjoin/client.h (1)
253-260: Resolve clang-format diffsCI reported clang-format differences for this hunk. Please run clang-format to satisfy the style check.
src/coinjoin/server.cpp (1)
112-189: DSQUEUE: clean migration to MessageProcessingResult; consider reducing TOCTOU on vecCoinJoinQueue
- Good use of ret.m_to_erase and consistent mapping of invalid cases to MisbehavingError (100 for malformed, 10 for unknown MN and sig failure).
- Logging uses NodeId with %d — correct.
- Minor optional: there’s a small time-of-check/time-of-use window between the initial scan under cs_vecqueue (TRY_LOCK at Lines 138-152) and the later push_back under a second TRY_LOCK (Lines 183-185). It’s benign, but could allow a brief race that admits a duplicate dsq if one slips in between checks. If you want to eliminate this, consider performing the “seen” check and the push while holding the same lock (under a single WITH_LOCK block), at the expense of a slightly longer critical section.
src/coinjoin/client.cpp (1)
74-141: Inline DSQUEUE handler is solid; consider extracting common validation into a helperThe DSQUEUE flow mirrors server-side logic (null checks, resolving proTx/outpoint, signature verification, rate limiting, queue updates, relay). To reduce duplication and future drift between client/server paths, consider extracting the shared validation and mapping to a small helper (e.g., parse/validate dsq + build ret) and keep module-specific actions (wallet hooks vs server queue state) separate.
src/llmq/dkgsessionhandler.cpp (1)
61-91: PushPendingMessage now returns MessageProcessingResult — good pattern; optional DoS scoring
- m_to_erase is correctly populated (guarded for from == -1).
- Optional: consider setting ret.m_error for “too many messages” (Line 77-81) and possibly for duplicates to centralize DoS scoring in net_processing. This would be a behavior change, so fine to defer.
src/governance/governance.cpp (1)
974-985: Nit: prefer a consistent style over directreturn MisbehavingError{...}Elsewhere in this file you populate
ret.m_errorand returnret. For consistency/readability, mirror that here instead of relying on implicit conversion.Apply within this block:
- if (m_netfulfilledman.HasFulfilledRequest(peer.addr, NetMsgType::MNGOVERNANCESYNC)) { - // Asking for the whole list multiple times in a short period of time is no good - LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s -- peer already asked me for the list\n", __func__); - return MisbehavingError{20}; - } + if (m_netfulfilledman.HasFulfilledRequest(peer.addr, NetMsgType::MNGOVERNANCESYNC)) { + // Asking for the whole list multiple times in a short period of time is no good + LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s -- peer already asked me for the list\n", __func__); + MessageProcessingResult ret{}; + ret.m_error = MisbehavingError{20}; + return ret; + }src/net_processing.cpp (2)
2358-2365: Remove redundant GetInvRelay() null-check
Peer::GetInvRelay()unconditionally returns the internalTxRelay*(it’s always constructed), so the null check is effectively dead code.Apply this minimal diff:
void PeerManagerImpl::RelayInv(const CInv& inv) { LOCK(m_peer_mutex); for (const auto& [_, peer] : m_peer_map) { - if (!peer->GetInvRelay()) continue; PushInv(*peer, inv); } }
3505-3507: Optional: batch relays to reduce repeated peer traversalLooping
result.m_inventoryand callingRelayInv()per item repeatedly traverses peers each time. If some handlers start returning multiple invs, consider a batched relay path to amortize map iteration (e.g., aRelayInvs(span<const CInv>)that pushes all items per-peer in one pass). Not critical given typical small inventory counts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (34)
contrib/devtools/copyright_header.py(0 hunks)src/Makefile.am(0 hunks)src/chainlock/chainlock.h(1 hunks)src/chainlock/signing.h(1 hunks)src/coinjoin/client.cpp(5 hunks)src/coinjoin/client.h(2 hunks)src/coinjoin/server.cpp(3 hunks)src/coinjoin/server.h(3 hunks)src/evo/mnauth.cpp(3 hunks)src/evo/mnauth.h(1 hunks)src/governance/governance.cpp(16 hunks)src/governance/governance.h(2 hunks)src/instantsend/instantsend.cpp(2 hunks)src/instantsend/instantsend.h(1 hunks)src/llmq/blockprocessor.cpp(1 hunks)src/llmq/blockprocessor.h(1 hunks)src/llmq/dkgsessionhandler.cpp(3 hunks)src/llmq/dkgsessionhandler.h(5 hunks)src/llmq/dkgsessionmgr.cpp(5 hunks)src/llmq/dkgsessionmgr.h(2 hunks)src/llmq/quorums.cpp(2 hunks)src/llmq/quorums.h(1 hunks)src/llmq/signing.cpp(1 hunks)src/llmq/signing.h(2 hunks)src/net_processing.cpp(8 hunks)src/net_processing.h(1 hunks)src/net_types.h(1 hunks)src/protocol.h(2 hunks)src/spork.cpp(3 hunks)src/spork.h(1 hunks)src/util/expected.h(0 hunks)test/lint/lint-include-guards.py(1 hunks)test/lint/lint-spelling.py(1 hunks)test/lint/lint-whitespace.py(0 hunks)
💤 Files with no reviewable changes (4)
- test/lint/lint-whitespace.py
- contrib/devtools/copyright_header.py
- src/Makefile.am
- src/util/expected.h
🚧 Files skipped from review as they are similar to previous changes (12)
- test/lint/lint-include-guards.py
- src/chainlock/signing.h
- src/llmq/quorums.h
- src/protocol.h
- src/llmq/blockprocessor.cpp
- src/llmq/dkgsessionmgr.h
- src/spork.h
- src/llmq/blockprocessor.h
- test/lint/lint-spelling.py
- src/governance/governance.h
- src/instantsend/instantsend.cpp
- src/llmq/signing.cpp
🧰 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/net_types.hsrc/chainlock/chainlock.hsrc/llmq/quorums.cppsrc/evo/mnauth.hsrc/coinjoin/server.hsrc/net_processing.hsrc/coinjoin/server.cppsrc/coinjoin/client.hsrc/llmq/dkgsessionmgr.cppsrc/llmq/signing.hsrc/llmq/dkgsessionhandler.cppsrc/governance/governance.cppsrc/instantsend/instantsend.hsrc/spork.cppsrc/net_processing.cppsrc/coinjoin/client.cppsrc/evo/mnauth.cppsrc/llmq/dkgsessionhandler.h
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/evo/mnauth.hsrc/evo/mnauth.cpp
🧠 Learnings (8)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#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.
📚 Learning: 2025-01-02T21:50:00.967Z
Learnt from: kwvg
PR: dashpay/dash#6504
File: src/llmq/context.cpp:42-43
Timestamp: 2025-01-02T21:50:00.967Z
Learning: LLMQContext manages concurrency for the `CInstantSendManager`. Previously, this was handled globally; now it's handled as a class member in `LLMQContext`, but the concurrency control remains consistent.
Applied to files:
src/chainlock/chainlock.hsrc/instantsend/instantsend.h
📚 Learning: 2025-02-06T14:34:30.466Z
Learnt from: kwvg
PR: dashpay/dash#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.
Applied to files:
src/coinjoin/server.cppsrc/llmq/dkgsessionmgr.cpp
📚 Learning: 2025-07-29T14:32:48.369Z
Learnt from: kwvg
PR: dashpay/dash#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/coinjoin/server.cpp
📚 Learning: 2025-01-14T09:07:12.446Z
Learnt from: kwvg
PR: dashpay/dash#6532
File: src/net.cpp:4329-4329
Timestamp: 2025-01-14T09:07:12.446Z
Learning: Keep suggestions focused on the scope of the current commit/PR. Avoid suggesting unrelated improvements that should be handled in separate PRs, even if technically valid.
Applied to files:
src/llmq/dkgsessionmgr.cpp
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
PR: dashpay/dash#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/dkgsessionmgr.cpp
📚 Learning: 2025-07-29T14:33:01.040Z
Learnt from: kwvg
PR: dashpay/dash#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/dkgsessionmgr.cpp
📚 Learning: 2025-07-23T09:28:32.783Z
Learnt from: kwvg
PR: dashpay/dash#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/dkgsessionmgr.cpp
🧬 Code Graph Analysis (16)
src/llmq/quorums.cpp (3)
src/llmq/blockprocessor.cpp (2)
ProcessMessage(57-154)ProcessMessage(57-58)src/net_processing.cpp (13)
ProcessMessage(3510-5294)ProcessMessage(3510-3515)pfrom(616-616)pfrom(639-640)pfrom(722-724)pfrom(730-730)pfrom(738-738)pfrom(741-741)pfrom(743-743)pfrom(745-745)pfrom(838-838)pfrom(1020-1020)ret(654-654)src/llmq/dkgsessionmgr.cpp (2)
ProcessMessage(95-187)ProcessMessage(95-96)
src/evo/mnauth.h (3)
src/governance/governance.cpp (2)
ProcessMessage(143-283)ProcessMessage(143-143)src/net_processing.cpp (8)
ProcessMessage(3510-5294)ProcessMessage(3510-3515)peer(668-668)peer(797-797)peer(902-902)peer(903-903)peer(1010-1010)peer(1018-1018)src/evo/mnauth.cpp (2)
ProcessMessage(58-189)ProcessMessage(58-60)
src/coinjoin/server.h (2)
src/coinjoin/server.cpp (4)
ProcessDSQUEUE(112-189)ProcessDSQUEUE(112-112)ProcessMessage(26-41)ProcessMessage(26-26)src/coinjoin/client.cpp (6)
ProcessMessage(41-142)ProcessMessage(41-42)ProcessMessage(144-166)ProcessMessage(144-144)ProcessMessage(183-230)ProcessMessage(183-183)
src/net_processing.h (1)
src/net_processing.cpp (14)
RelayInv(2345-2356)RelayInv(2345-2345)RelayInv(2358-2365)RelayInv(2358-2358)inv(630-630)inv(631-631)inv(632-632)inv(633-633)inv(928-928)inv(2307-2307)RelayInvFiltered(2395-2418)RelayInvFiltered(2395-2395)RelayInvFiltered(2420-2443)RelayInvFiltered(2420-2420)
src/coinjoin/server.cpp (4)
src/governance/governance.cpp (2)
ProcessMessage(143-283)ProcessMessage(143-143)src/llmq/blockprocessor.cpp (2)
ProcessMessage(57-154)ProcessMessage(57-58)src/coinjoin/client.cpp (6)
ProcessMessage(41-142)ProcessMessage(41-42)ProcessMessage(144-166)ProcessMessage(144-144)ProcessMessage(183-230)ProcessMessage(183-183)src/llmq/dkgsessionmgr.cpp (2)
ProcessMessage(95-187)ProcessMessage(95-96)
src/coinjoin/client.h (2)
src/coinjoin/server.cpp (2)
ProcessMessage(26-41)ProcessMessage(26-26)src/coinjoin/client.cpp (6)
ProcessMessage(41-142)ProcessMessage(41-42)ProcessMessage(144-166)ProcessMessage(144-144)ProcessMessage(183-230)ProcessMessage(183-183)
src/llmq/dkgsessionmgr.cpp (5)
src/llmq/dkgsessionhandler.cpp (2)
ProcessMessage(155-168)ProcessMessage(155-155)src/llmq/commitment.h (1)
quorumIndex(47-47)src/llmq/dkgsession.h (1)
quorumIndex(296-296)src/evo/mnhftx.h (1)
quorumHash(35-35)src/evo/assetlocktx.h (1)
quorumHash(81-81)
src/llmq/signing.h (2)
src/net_processing.cpp (3)
ProcessMessage(3510-5294)ProcessMessage(3510-3515)from(1023-1023)src/llmq/signing.cpp (2)
ProcessMessage(407-447)ProcessMessage(407-407)
src/llmq/dkgsessionhandler.cpp (3)
src/llmq/signing.cpp (3)
ret(31-31)ProcessMessage(407-447)ProcessMessage(407-407)src/llmq/blockprocessor.cpp (2)
ProcessMessage(57-154)ProcessMessage(57-58)src/llmq/dkgsessionmgr.cpp (2)
ProcessMessage(95-187)ProcessMessage(95-96)
src/governance/governance.cpp (3)
src/llmq/blockprocessor.cpp (2)
ProcessMessage(57-154)ProcessMessage(57-58)src/spork.cpp (2)
ProcessMessage(129-137)ProcessMessage(129-129)src/coinjoin/server.cpp (2)
ProcessMessage(26-41)ProcessMessage(26-26)
src/instantsend/instantsend.h (2)
src/net_processing.cpp (3)
ProcessMessage(3510-5294)ProcessMessage(3510-3515)from(1023-1023)src/instantsend/instantsend.cpp (2)
ProcessMessage(109-172)ProcessMessage(109-109)
src/spork.cpp (5)
src/governance/governance.cpp (2)
ProcessMessage(143-283)ProcessMessage(143-143)src/llmq/blockprocessor.cpp (2)
ProcessMessage(57-154)ProcessMessage(57-58)src/coinjoin/server.cpp (2)
ProcessMessage(26-41)ProcessMessage(26-26)src/net_processing.cpp (10)
ProcessMessage(3510-5294)ProcessMessage(3510-3515)peer(668-668)peer(797-797)peer(902-902)peer(903-903)peer(1010-1010)peer(1018-1018)from(1023-1023)ret(654-654)src/spork.h (1)
cs(230-340)
src/net_processing.cpp (5)
src/spork.cpp (3)
inv(461-461)ProcessMessage(129-137)ProcessMessage(129-129)src/coinjoin/server.cpp (3)
inv(354-354)ProcessMessage(26-41)ProcessMessage(26-26)src/instantsend/instantsend.cpp (3)
inv(419-419)ProcessMessage(109-172)ProcessMessage(109-109)src/evo/mnauth.cpp (2)
ProcessMessage(58-189)ProcessMessage(58-60)src/coinjoin/client.cpp (6)
ProcessMessage(41-142)ProcessMessage(41-42)ProcessMessage(144-166)ProcessMessage(144-144)ProcessMessage(183-230)ProcessMessage(183-183)
src/coinjoin/client.cpp (4)
src/governance/governance.cpp (2)
ProcessMessage(143-283)ProcessMessage(143-143)src/llmq/blockprocessor.cpp (2)
ProcessMessage(57-154)ProcessMessage(57-58)src/spork.cpp (2)
ProcessMessage(129-137)ProcessMessage(129-129)src/llmq/dkgsessionmgr.cpp (2)
ProcessMessage(95-187)ProcessMessage(95-96)
src/evo/mnauth.cpp (1)
src/net_processing.cpp (8)
ProcessMessage(3510-5294)ProcessMessage(3510-3515)peer(668-668)peer(797-797)peer(902-902)peer(903-903)peer(1010-1010)peer(1018-1018)
src/llmq/dkgsessionhandler.h (2)
src/llmq/dkgsessionmgr.cpp (3)
GUARDED_BY(99-99)ProcessMessage(95-187)ProcessMessage(95-96)src/llmq/dkgsessionhandler.cpp (4)
PushPendingMessage(61-91)PushPendingMessage(61-61)ProcessMessage(155-168)ProcessMessage(155-155)
🪛 GitHub Actions: Clang Diff Format Check
src/llmq/quorums.cpp
[error] 703-710: Clang-format differences detected by clang-format-diff.py. Apply formatting changes to this file.
src/evo/mnauth.h
[error] 58-69: Clang-format differences detected by clang-format-diff.py. Apply formatting changes to this file.
src/coinjoin/server.h
[error] 86-93: Clang-format differences detected by clang-format-diff.py. Apply formatting changes to this file.
src/coinjoin/server.cpp
[error] 145-154: Clang-format differences detected by clang-format-diff.py. Apply formatting changes to this file.
src/coinjoin/client.h
[error] 253-260: Clang-format differences detected by clang-format-diff.py. Apply formatting changes to this file.
src/llmq/signing.h
[error] 13-20: Clang-format differences detected by clang-format-diff.py. Apply formatting changes to this file.
src/governance/governance.cpp
[error] 140-147: Clang-format differences detected by clang-format-diff.py. Apply formatting changes to this file.
src/spork.cpp
[error] 126-133: Clang-format differences detected by clang-format-diff.py. Apply formatting changes to this file.
src/coinjoin/client.cpp
[error] 84-91: Clang-format differences detected by clang-format-diff.py. Apply formatting changes to this file.
src/evo/mnauth.cpp
[error] 55-65: Clang-format differences detected by clang-format-diff.py. Apply formatting changes to this file.
src/llmq/dkgsessionhandler.h
[error] 74-81: Clang-format differences detected by clang-format-diff.py. Apply formatting changes to this file.
⏰ 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). (3)
- GitHub Check: linux64_tsan-test / Test source
- GitHub Check: linux64_ubsan-test / Test source
- GitHub Check: linux64_multiprocess-build / Build source
🔇 Additional comments (44)
src/net_types.h (1)
43-43: LGTM! Centralized NodeId definition improves maintainability.The addition of
NodeIdtype alias innet_types.hestablishes a single source of truth for node identifiers across the codebase, replacing scattered local definitions. This is a good architectural improvement that reduces potential type inconsistencies.src/llmq/signing.h (2)
11-11: LGTM! Required includes added for new signature dependencies.The addition of
net_types.hforNodeIdandstring_viewfor the updatedProcessMessagesignature are correctly placed and necessary for the refactored API.Also applies to: 19-19
184-184: CSigningManager::ProcessMessage (NodeId) verified — no action required
- Implementation treats
fromas a NodeId (no dereference/field access), logs it, and indexespendingRecoveredSigs[from].
- src/llmq/signing.cpp:407–447 (definition)
- src/llmq/signing.h:184 (signature)
- Call site updated to pass NodeId:
- src/net_processing.cpp:5273:
m_llmq_ctx->sigman->ProcessMessage(pfrom.GetId(), msg_type, vRecv)- Searches performed: no
from->orfrom.usages in signing.cpp; no callers passing aCNode/pnodeto the signing manager.Conclusion: change correctly uses NodeId and callers have been updated.
src/llmq/dkgsessionhandler.h (2)
86-91: Good use of template for type-safe message pushing.The templated
PushPendingMessagemethod provides a clean, type-safe way to serialize and push messages through the post-processing pipeline. The integration withPeerManager::PostProcessMessagealigns well with the new architecture.
94-115: Well-designed template for deserializing messages.The
PopAndDeserializeMessagestemplate provides a robust way to deserialize messages with proper error handling. Setting the message pointer tonullptron deserialization failure is a good practice that allows callers to detect and handle errors gracefully.src/chainlock/chainlock.h (1)
10-10: LGTM! Proper migration to centralized NodeId definition.The include of
net_types.hreplaces the localNodeIddefinition, ensuring consistency with the centralized type definition introduced in this PR.src/llmq/quorums.cpp (1)
725-748: Good refactoring of error handling logic.The conversion of the sendQDATA lambda to return
MessageProcessingResultand use structured error handling withMisbehavingErroris a clean improvement over the previoustl::unexpectedapproach. The logic properly handles different error cases with appropriate scoring.src/coinjoin/server.h (3)
10-10: Good: central NodeId includeIncluding net_types.h removes the need for local NodeId aliases and avoids ODR pitfalls. Consistent with the PR’s intent.
88-92: API migration: DSQUEUE now NodeId + MessageProcessingResult — looks correct
- Switching to NodeId and returning [[nodiscard]] MessageProcessingResult aligns with the new processing model and central PostProcessMessage.
- Header includes (net_types.h, protocol.h) cover the new types.
No issues from the declaration side.
Please confirm all internal calls use peer.GetId() when invoking ProcessDSQUEUE. The implementation in src/coinjoin/server.cpp already does so; double-check no other direct calls exist.
114-115: API migrated to [[nodiscard]] MessageProcessingResult — consistent with net_processingReturning MessageProcessingResult and marking it [[nodiscard]] is consistent with the new flow where net_processing handles follow-up via PostProcessMessage.
Ensure all callers consume the return via PostProcessMessage to avoid [[nodiscard]] warnings (or -Werror CI failures) for ignored results.
src/evo/mnauth.h (1)
61-63: MNAUTH: switch to [[nodiscard]] MessageProcessingResult — OKThe signature change matches the project-wide migration. Includes already bring in protocol.h, so the type is available. No interface mismatch spotted here.
Confirm net_processing.cpp consumes the returned MessageProcessingResult through PostProcessMessage, as shown in the PR context.
src/coinjoin/client.h (2)
11-11: Good: use shared NodeIdIncluding net_types.h is the right way to use the canonical NodeId.
256-258: ClientQueueManager::ProcessMessage migrated to NodeId + [[nodiscard]] MessageProcessingResult — correct
- Signature aligns with server-side and net_processing usage.
- The EXCLUSIVE_LOCKS_REQUIRED(!cs_vecqueue) annotation is preserved.
Looks good.
Verify all call sites pass pfrom.GetId() as the first argument (e.g., in net_processing.cpp) and that the return value is forwarded to PostProcessMessage.
src/instantsend/instantsend.h (1)
132-132: InstantSend: ProcessMessage now NodeId + MessageProcessingResult — matches net_processing usageThe new signature mirrors the updated call pattern in net_processing and enforces result handling with [[nodiscard]]. Header already includes net_types.h and protocol.h.
Confirm all callers use PostProcessMessage with the returned result; otherwise [[nodiscard]] may trigger warnings.
src/net_processing.h (2)
101-105: Make RelayInv take const CInv& — good API tighteningSwitching to const references avoids accidental mutation and unnecessary copies. Implementation in net_processing.cpp reflects this. Ensure all overrides in tests/mocks were updated accordingly.
I recommend verifying no overrides still use non-const signatures. Search for “override” methods of RelayInv that accept “CInv &”.
110-111: Make RelayInvFiltered take const CInv& — consistent with aboveBoth overloads updated to const references, matching the implementation. Good consistency.
Double-check any custom PeerManager implementations in tests conform to the updated signature set to avoid vtable mismatches at link time.
src/coinjoin/server.cpp (2)
26-41: Adopted MessageProcessingResult in ProcessMessage — LGTMThe DSQUEUE path now returns a MessageProcessingResult and uses NodeId via peer.GetId(). This aligns with the refactor pattern used elsewhere.
145-154: Fix clang-format diff failure in this hunkCI reports clang-format differences for this range; the verification command you ran produced no output in this environment — please run the project formatter locally and commit the result.
- File: src/coinjoin/server.cpp — lines 145–154
- Hunk:
} if (q.fReady == dsq.fReady && q.masternodeOutpoint == dsq.masternodeOutpoint) { // no way the same mn can send another dsq with the same readiness this soon LogPrint(BCLog::COINJOIN, "DSQUEUE -- Peer %d is sending WAY too many dsq messages for a masternode with collateral %s\n", from, dsq.masternodeOutpoint.ToStringShort()); return ret; } } } // cs_vecqueue LogPrint(BCLog::COINJOIN, "DSQUEUE -- %s new\n", dsq.ToString());
- Run locally:
git diff -U0 | contrib/devtools/clang-format-diff.py -p1 -i -v
Commit any formatting changes the script reports.src/coinjoin/client.cpp (2)
41-49: ProcessMessage now returns MessageProcessingResult and filters DSQUEUE — LGTMSignature and early-return filtering match the new pattern across the codebase.
56-71: Result object and error signaling — LGTMret.m_to_erase is set, and malformed DSQUEUE cases map to MisbehavingError codes consistently with server and other subsystems.
src/llmq/dkgsessionhandler.cpp (1)
155-168: ProcessMessage routing to per-type pending queues — LGTMReturning the result of PushPendingMessage for each message type fits the new post-processing pipeline.
src/spork.cpp (3)
129-137: ProcessMessage returns MessageProcessingResult and passes NodeId — LGTMThe SPORK path now leverages ProcessSpork(peer.GetId(), vRecv) and returns a result for post-processing.
139-193: ProcessSpork: structured result, proper misbehavior mapping, and inventory relay — LGTM
- ret.m_to_erase set to MSG_SPORK/.
- MisbehavingError{100} for time skew and invalid signature; updates under cs; clears caches.
- ret.m_inventory populated to request relay via the caller.
126-133: Fix clang-format diff failure in src/spork.cpp (around lines 126–133)Please re-run the formatter to satisfy CI’s clang-format-diff check — I couldn't reproduce the check in the verification sandbox (clang-format/nl may be missing). Locally run:
git diff -U0 | contrib/devtools/clang-format-diff.py -p1 -i -v
Location to fix:
- src/spork.cpp — hunk near lines 126–133 (the MessageProcessingResult / ProcessMessage branch)
If the formatter makes changes, commit and push them. If CI still fails, paste the output of the clang-format-diff.py run here so I can re-check.
src/evo/mnauth.cpp (4)
58-68: ProcessMessage return-type migration and early guard — LGTMReturning MessageProcessingResult and guarding on msg_type/sync state are consistent with the broader refactor.
72-99: Error-paths unified to MisbehavingError — LGTMDuplicate MNAUTH, invalid services, empty proRegTxHash, and missing MN now return MisbehavingError with appropriate scores/messages, matching project conventions.
114-118: Signature verification failure returns MisbehavingError — LGTMA low-severity (10) score for verification failure aligns with giving lagging nodes a chance to catch up.
55-65: Apply clang-format to the CMNAuth::ProcessMessage hunk in src/evo/mnauth.cppCI reports formatting differences in this hunk. I attempted to run contrib/devtools/clang-format-diff.py here but clang-format is not available in this environment — please run the formatter and commit the result.
Files/locations to fix:
- src/evo/mnauth.cpp — CMNAuth::ProcessMessage (hunk starting around line ~55)
How to reproduce/fix:
- git diff -U0 | contrib/devtools/clang-format-diff.py -p1 -i -v
(or run clang-format -i on the file and commit)src/llmq/dkgsessionmgr.cpp (4)
95-97: MessageProcessingResult migration and NodeId-based dispatch look correctThe new signature and return type align with the PR goals. Passing
pfrom.GetId()into the handler matches the updatedCDKGSessionHandler::ProcessMessage(NodeId, ...)contract.
104-111: Early message-type gating is correct and keeps processing tightRestricting to DKG-related message types and returning empty for others avoids unnecessary work and matches the unified result pattern.
185-187: NodeId-based handoff to session handler LGTMHandoff via
dkgSessionHandlers.at(...).ProcessMessage(pfrom.GetId(), msg_type, vRecv)matches the updated handler API and preserves the read position via earlier rewinds.
112-116: Confirmed — implicit conversion from MisbehavingError to MessageProcessingResult existsMessageProcessingResult declares std::optional m_error and a converting constructor:
- src/protocol.h (≈lines 594–612): m_error at ~596, and
MessageProcessingResult(MisbehavingError error) : m_error(error) {}at ~609–611.Returning
MisbehavingError{...}in src/llmq/dkgsessionmgr.cpp is valid; no change required.src/governance/governance.cpp (7)
143-144: Return type migration to MessageProcessingResult is consistentThe governance message entrypoint now cleanly returns a
MessageProcessingResult, aligning with post-processing in net_processing.
161-169: MNGOVERNANCESYNC path correctly delegates and returns resultsForwarding to
SyncObjectsorSyncSingleObjVotesand returning their results centralizes post-processing as intended.
180-186: Consolidating actions into ret (erase + error) is correct
- Setting
ret.m_to_eraseearly ensures a pending request is cleared regardless of acceptance outcome.- Using
ret.m_error = MisbehavingError{20}for invalid objects preserves previous behavior via structured results.Also applies to: 188-195, 199-209, 222-238
247-280: Vote handling path mirrors the new pattern correctlyThe vote path accumulates
m_to_eraseand setsm_errorbased onexception.GetNodePenalty(). Relay behavior remains unchanged.
922-972: SyncSingleObjVotes: inventory accumulation and status reporting are correct
- Accumulating votes into
ret.m_inventorymatches the new unified return model.- SYNCSTATUSCOUNT is still pushed via connman, preserving observable behavior.
143-169: No change required — implicit conversion confirmedMessageProcessingResult has a non-explicit converting constructor:
- src/protocol.h:609-611 —
MessageProcessingResult(MisbehavingError error) : m_error(error) {}Returning a bare MisbehavingError will implicitly construct a MessageProcessingResult, so the mixed return patterns are valid.
140-147: Fix clang-format diffs on the ProcessMessage signatureCI reports clang-format differences here; clang-format was not available in the verification environment, so please run it locally or apply the suggested wrap below.
- File: src/governance/governance.cpp — signature around lines 143–147
Suggested change (wrap the long signature):
-MessageProcessingResult CGovernanceManager::ProcessMessage(CNode& peer, CConnman& connman, PeerManager& peerman, std::string_view msg_type, CDataStream& vRecv) +MessageProcessingResult CGovernanceManager::ProcessMessage( + CNode& peer, CConnman& connman, PeerManager& peerman, + std::string_view msg_type, CDataStream& vRecv)Auto-fix locally:
- clang-format -i src/governance/governance.cpp
- or: git diff -U0 | clang-format-diff.py -p1 -i
src/net_processing.cpp (5)
630-633: Const-correct RelayInv/RelayInvFiltered API looks goodSwitching to
const CInv&across RelayInv and RelayInvFiltered overloads tightens the API and avoids copies. Call sites here are updated accordingly.
2345-2356: RelayInv(minProtoVersion) implementation aligns with new signatureUses
ForEachNodeand pushes to eligible peers without copyingCInv. No functional concerns from the refactor.
2395-2418: RelayInvFiltered(relatedTx) respects bloom gating and versioningThe bloom and relay flags are checked under the correct mutex, and the function signature is const-correct. Looks solid.
2420-2443: RelayInvFiltered(relatedTxHash) path is consistent and correctVersion/permission checks precede bloom lookups; const-ref signatures avoid copies. No issues found.
5259-5287: Unified post-processing confirmed — no lingering PeerMsgRet / tl::expectedSearches show no occurrences of PeerMsgRet, tl::expected, or util/expected.h. MessageProcessingResult and PostProcessMessage are in use as intended and net_processing.cpp now funnels handler returns through PostProcessMessage(NodeId).
- Verified absence of legacy types/headers: no matches for "PeerMsgRet", "tl::expected", or "util/expected.h".
- Key locations:
- src/protocol.h — struct MessageProcessingResult (definition).
- src/net_processing.cpp — PeerManagerImpl::PostProcessMessage implementation (~line 3494) and the handler calls at lines ~5259–5287 (CLSIG handling and PostProcessMessage(pfrom.GetId()) usage).
- Reminder: vRecv is still passed to multiple handlers; each handler must only consume the stream when msg_type matches (unchanged contract).
| if (q.fReady == dsq.fReady && q.masternodeOutpoint == dsq.masternodeOutpoint) { | ||
| // no way the same mn can send another dsq with the same readiness this soon | ||
| LogPrint(BCLog::COINJOIN, /* Continued */ | ||
| "DSQUEUE -- Peer %s is sending WAY too many dsq messages for a masternode with collateral %s\n", | ||
| peer.GetLogString(), dsq.masternodeOutpoint.ToStringShort()); | ||
| return {}; | ||
| "DSQUEUE -- Peer %d is sending WAY too many dsq messages for a masternode with collateral %s\n", | ||
| from, dsq.masternodeOutpoint.ToStringShort()); | ||
| return ret; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix clang-format diff failure in this hunk
CI flagged style differences here. Please apply the formatter.
You can use:
🏁 Script executed:
#!/bin/bash
git diff -U0 | contrib/devtools/clang-format-diff.py -p1 -i -vLength of output: 64
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=src/coinjoin/client.cpp
echo "=== Git branch ==="
git rev-parse --abbrev-ref HEAD || true
echo
echo "=== Git remotes ==="
git remote -v || true
echo
echo "=== Last commit ==="
git log -1 --oneline || true
echo
if [ ! -f "$FILE" ]; then
echo "FILE_NOT_FOUND: $FILE"
exit 0
fi
echo "=== File snippet ($FILE) ==="
nl -ba "$FILE" | sed -n '1,140p'
echo
# Determine a reasonable base to diff against
BASE=""
for candidate in origin/main origin/master origin/HEAD HEAD~1; do
if git rev-parse --verify "$candidate" >/dev/null 2>&1; then
if git show "$candidate":"$FILE" >/dev/null 2>&1; then
BASE="$candidate"
break
fi
fi
done
if [ -z "$BASE" ]; then
echo "No upstream base found; will diff against HEAD~1"
BASE="HEAD~1"
fi
echo "=== Diff base chosen: $BASE ==="
git rev-parse --short "$BASE" || true
echo
echo "=== Generating diff for $FILE (unified, 0 context) ==="
git diff -U0 "$BASE".."HEAD" -- "$FILE" > /tmp/file.diff || true
if [ ! -s /tmp/file.diff ]; then
echo "Diff is empty (no changes to $FILE between $BASE and HEAD). Showing full diff of HEAD~1..HEAD for the file as fallback."
git diff -U0 HEAD~1..HEAD -- "$FILE" > /tmp/file.diff || true
fi
echo "=== Diff (first 200 lines) ==="
sed -n '1,200p' /tmp/file.diff || true
echo
echo "=== clang-format presence ==="
if command -v clang-format >/dev/null 2>&1; then
echo "clang-format found: $(command -v clang-format)"
else
echo "clang-format NOT found in PATH"
fi
echo
echo "=== Running contrib/devtools/clang-format-diff.py on the diff ==="
python3 contrib/devtools/clang-format-diff.py -p1 -i -v < /tmp/file.diff || echo "clang-format-diff.py exited with non-zero status"Length of output: 796
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=src/coinjoin/client.cpp
echo "=== Git branch ==="
git rev-parse --abbrev-ref HEAD || true
echo
echo "=== Last commit ==="
git log -1 --oneline || true
echo
if [ ! -f "$FILE" ]; then
echo "FILE_NOT_FOUND: $FILE"
exit 0
fi
echo "=== File snippet ($FILE) lines 1-200 ==="
awk '{printf("%5d: %s\n", NR, $0)}' "$FILE" | sed -n '1,200p'
echo
# Determine base for diff
BASE=""
for candidate in origin/main origin/master origin/HEAD HEAD~1; do
if git rev-parse --verify "$candidate" >/dev/null 2>&1; then
if git show "$candidate":"$FILE" >/dev/null 2>&1; then
BASE="$candidate"
break
fi
fi
done
if [ -z "$BASE" ]; then
echo "No upstream base found; using HEAD~1"
BASE="HEAD~1"
fi
echo "=== Diff base chosen: $BASE ==="
git rev-parse --short "$BASE" || true
echo
echo "=== Generating diff for $FILE (unified, 0 context) ==="
git diff -U0 "$BASE".."HEAD" -- "$FILE" > /tmp/file.diff || true
if [ ! -s /tmp/file.diff ]; then
echo "Diff empty against $BASE; trying HEAD~1..HEAD"
git diff -U0 HEAD~1..HEAD -- "$FILE" > /tmp/file.diff || true
fi
echo "=== Diff (first 200 lines) ==="
sed -n '1,200p' /tmp/file.diff || true
echo
echo "=== clang-format presence ==="
if command -v clang-format >/dev/null 2>&1; then
echo "clang-format found: $(command -v clang-format)"
else
echo "clang-format NOT found in PATH"
fi
echo
echo "=== Running contrib/devtools/clang-format-diff.py ==="
python3 contrib/devtools/clang-format-diff.py -p1 -i -v < /tmp/file.diff || echo "clang-format-diff.py exited with non-zero status"Length of output: 15308
Apply clang-format to src/coinjoin/client.cpp (hunk around lines 84–91)
CI flagged style differences in this hunk. I ran contrib/devtools/clang-format-diff.py but it failed here because clang-format is not available in the environment (FileNotFoundError). Please run the formatter and commit the result.
- File: src/coinjoin/client.cpp — hunk around lines 84–91 (DSQUEUE log block)
- Problematic snippet:
if (q.fReady == dsq.fReady && q.masternodeOutpoint == dsq.masternodeOutpoint) {
// no way the same mn can send another dsq with the same readiness this soon
LogPrint(BCLog::COINJOIN, /* Continued */
"DSQUEUE -- Peer %d is sending WAY too many dsq messages for a masternode with collateral %s\n",
from, dsq.masternodeOutpoint.ToStringShort());
return ret;
}
Suggested fix (run locally and commit the formatted changes):
- git diff -U0 | contrib/devtools/clang-format-diff.py -p1 -i -v
- or run clang-format directly: clang-format -i src/coinjoin/client.cpp
🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 84-91: Clang-format differences detected by clang-format-diff.py. Apply formatting changes to this file.
🤖 Prompt for AI Agents
In src/coinjoin/client.cpp around lines 84 to 91, the DSQUEUE LogPrint block is
misformatted per clang-format; run the project formatter on this file (e.g.
clang-format -i src/coinjoin/client.cpp or run git diff -U0 |
contrib/devtools/clang-format-diff.py -p1 -i -v) to reformat the hunk so it
matches the repo style, then stage and commit the resulting changes.
| } | ||
|
|
||
| PeerMsgRet CQuorumManager::ProcessMessage(CNode& pfrom, CConnman& connman, const std::string& msg_type, CDataStream& vRecv) | ||
| MessageProcessingResult CQuorumManager::ProcessMessage(CNode& pfrom, CConnman& connman, std::string_view msg_type, CDataStream& vRecv) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify all message handlers return MessageProcessingResult.
The signature change to return MessageProcessingResult and use std::string_view for msg_type aligns with the PR objectives. This is part of the broader migration away from PeerMsgRet.
Let's verify that all message processing functions have been consistently updated:
🏁 Script executed:
#!/bin/bash
# Check for any remaining PeerMsgRet usage
echo "Checking for any remaining PeerMsgRet usage:"
rg "PeerMsgRet" src/
echo -e "\nVerifying ProcessMessage signatures are consistent:"
rg "ProcessMessage.*CNode.*string_view.*CDataStream" src/llmq/ src/coinjoin/ src/governance/ src/spork/ --type cppLength of output: 2351
Update remaining ProcessMessage handlers to return MessageProcessingResult
Verification shows most handlers were updated, but some still return void and must be converted to MessageProcessingResult.
- src/coinjoin/client.h
- Declarations currently:
void ProcessMessage(CNode& peer, CChainState& active_chainstate, CConnman& connman, const CTxMemPool& mempool, std::string_view msg_type, CDataStream& vRecv);(also theEXCLUSIVE_LOCKS_REQUIRED(!cs_deqsessions)variant). Change these to returnMessageProcessingResult(and add [[nodiscard]] if applicable).
- Declarations currently:
- src/coinjoin/client.cpp
- Definitions currently:
void CCoinJoinClientManager::ProcessMessage(...)andvoid CCoinJoinClientSession::ProcessMessage(...). Change their return types toMessageProcessingResultand update all return paths to return an appropriate MessageProcessingResult value.
- Definitions currently:
Note: rg reported src/spork/ does not exist during the search — re-run a repo-wide grep after fixing the above to ensure no remaining void ProcessMessage(...) or PeerMsgRet usages remain.
PushInv takes a `const CInv&` and we don't expect to modify the payload
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
src/governance/governance.h (1)
19-26: Add a forward declaration for MessageProcessingResult to avoid relying on transitive includesThis header declares functions returning MessageProcessingResult but doesn’t include or forward-declare it. To keep the header self-contained and resilient to include-order changes, add a forward declaration near other forward declarations.
Apply this diff:
class CInv; class PeerManager; +struct MessageProcessingResult; + class CDeterministicMNManager;src/net_processing.cpp (1)
1-1: Replace heavy <net.h> includes in headers; fix a few inv_opt.value() copiesVerified: no remaining ProcessPeerMsgRet or util/expected / tl::expected usages. However the repo still contains many header files that #include <net.h> (pulling a heavy header into headers) and a few call-sites that use inv_opt.value() (possible unnecessary copies).
Files that should be reviewed/changed (examples from search):
- Headers including <net.h> (prefer <net_types.h> or forward-declare):
src/net_processing.h, src/spork.h, src/txorphanage.h, src/netmessagemaker.h, src/llmq/dkgsessionhandler.h, src/interfaces/node.h, src/node/txreconciliation.h, src/qt/guiutil.h, src/qt/peertablemodel.h, src/qt/bantablemodel.h- RelayInv declarations/definitions:
src/net_processing.h:101-102 (virtual void RelayInv(const CInv& ...)), src/net_processing.cpp (defs at ~2345/2358)- Call-sites to inspect for copies / rvalues:
src/llmq/blockprocessor.cpp:151 (ret.m_inventory.emplace_back(inv_opt.value()));
src/llmq/dkgsessionhandler.cpp:558,609 (peerman.RelayInv(inv_opt.value()));
also: src/net_processing.cpp:3506, src/spork.cpp:462, src/governance/vote.cpp:119, src/governance/object.cpp:619, src/coinjoin/server.cpp:355Suggested actions:
- Replace <net.h> in headers with <net_types.h> when only lightweight types (NodeId, banmap_t, etc.) are needed, or use forward declarations / minimal headers to avoid heavy includes.
- Replace inv_opt.value() with *inv_opt (binds to const CInv&) or move the value into containers (e.g., emplace_back(std::move(*inv_opt))) where semantically safe to avoid copies.
♻️ Duplicate comments (4)
src/coinjoin/server.cpp (1)
148-149: Confirm NodeId format specifier is integer (looks correct now)Using %d for NodeId in LogPrint is correct and avoids undefined behavior.
src/coinjoin/client.cpp (2)
41-47: Correct early-exit guard on msg_type (and ordering)Returning immediately for non-DSQUEUE messages is the cheapest and most likely path, matching the perf nit suggested earlier.
84-91: LogPrint specifier for NodeId fixedUsing %d for from is correct here. This resolves the previous format issue.
src/net_processing.cpp (1)
653-655: Doc nit: pluralize and clarify helper descriptionThe comment reads oddly. Suggest:
- /** Helper to process result of external handlers of message */ + /** Helper to process the results returned by external message handlers */
🧹 Nitpick comments (12)
src/llmq/quorums.cpp (2)
710-710: Nit: log formatting with std::string_viewPassing msg_type (std::string_view) to a "%s" formatter works through tinyformat, but being explicit prevents surprises across compilers. Consider converting to std::string for logging.
- LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- %s: %s, from peer=%d\n", strFunc, msg_type, strError, pfrom.GetId()); + LogPrint(BCLog::LLMQ, "CQuorumManager::%s -- %s: %s, from peer=%d\n", strFunc, std::string{msg_type}, strError, pfrom.GetId());
1-1: Apply clang-format (CI is red)Clang-format check is failing for this file. The verification script produced no output here, so I couldn't confirm automatically — please run the formatter and commit the result.
- src/llmq/quorums.cpp
# Format only the lines changed in the PR git diff -U0 | contrib/devtools/clang-format-diff.py -p1 -i -v # If that yields no output, run full-file format and commit: clang-format -i src/llmq/quorums.cpp git add src/llmq/quorums.cpp && git commit -m "style: apply clang-format to src/llmq/quorums.cpp"src/coinjoin/server.cpp (1)
1-1: Apply clang-format to src/coinjoin/server.cpp (CI failing)CI reports clang-format differences for this file; the verification script produced no output, so please run the formatter and commit.
- File: src/coinjoin/server.cpp
- Suggested commands:
- contrib/devtools/clang-format-diff.py -p1 -i -v
- clang-format -i src/coinjoin/server.cpp
- git add src/coinjoin/server.cpp && git commit -m "style: run clang-format" && git push
src/llmq/dkgsessionhandler.cpp (1)
1-1: Apply clang-format (CI is red)Clang-format check produced no output in the sandbox, so I couldn't verify formatting here. Please run the formatter locally and commit the changes.
- File to format: src/llmq/dkgsessionhandler.cpp
- Suggested command: git diff -U0 | contrib/devtools/clang-format-diff.py -p1 -i -v
src/coinjoin/client.cpp (1)
1-1: Apply clang-format (CI is red)clang-format-diff flagged this file — please apply the formatter and push the resulting changes. I ran the provided verification command in the sandbox but it produced no output; please run it locally and commit any fixes:
git diff -U0 | contrib/devtools/clang-format-diff.py -p1 -i -v
Files needing check:
- src/coinjoin/client.cpp (line 1 — header comment)
src/evo/mnauth.cpp (2)
112-113: Nit: load atomic nVersion for loggingpeer.nVersion is atomic; load it explicitly for logging to avoid type/formatting surprises.
- LogPrint(BCLog::NET_NETCONN, "CMNAuth::%s -- constructed signHash for nVersion %d, peer=%d\n", __func__, peer.nVersion, peer.GetId()); + LogPrint(BCLog::NET_NETCONN, "CMNAuth::%s -- constructed signHash for nVersion %d, peer=%d\n", __func__, peer.nVersion.load(), peer.GetId());
1-1: Apply clang-format (CI is failing)CI reports style errors but I couldn't reproduce here because
git diff -U0produced no output in the sandbox. Please run this locally and commit any formatting fixes:#!/bin/bash # Ensure you compare against the base branch, then run the clang-format diff helper: git fetch origin main git diff -U0 origin/main...HEAD | contrib/devtools/clang-format-diff.py -p1 -i -v # If that prints fixes, apply them (e.g. `clang-format -i`) and commit/push so CI can pass.src/llmq/dkgsessionhandler.h (3)
9-9: Header depends on net_processing.h; consider reducing coupling (optional)Including net_processing.h in a public header increases compile-time coupling. It’s only needed because the templated overload calls peerman.PostProcessMessage inline. Consider returning MessageProcessingResult from the template and letting the caller post-process, which would let this header avoid including net_processing.h entirely.
If you want, I can propose a minimal diff that changes the template to return MessageProcessingResult and removes the PeerManager dependency from this header.
85-91: Make Message const in the templated PushPendingMessageThe message is only serialized; it shouldn’t require non-const. This also broadens call-site usability.
- template <typename Message> - void PushPendingMessage(NodeId from, Message& msg, PeerManager& peerman) + template <typename Message> + void PushPendingMessage(NodeId from, const Message& msg, PeerManager& peerman) { CDataStream ds(SER_NETWORK, PROTOCOL_VERSION); ds << msg; peerman.PostProcessMessage(PushPendingMessage(from, ds), from); }
54-62: Nit: fix typo in comment (“its” vs “it's”)“Each message type has its own instance of this class.”
src/llmq/dkgsessionmgr.cpp (1)
126-136: Optional: guard against short payloads before deserializationYou read llmqType (1 byte) and quorumHash (32 bytes) and then Rewind. To avoid exceptions bubbling up from stream ops, consider checking that vRecv size is at least sizeof(uint8_t)+sizeof(uint256) before reading. Not critical, net_processing will catch exceptions.
src/net_processing.cpp (1)
3503-3507: Optional: deduplicate inventories before relaying (micro-optimization)If result.m_inventory can contain duplicates from upstream handlers, consider deduping to reduce PushInv churn. Minor and optional; PushInv’s known-filter mitigates this already.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (28)
contrib/devtools/copyright_header.py(0 hunks)src/Makefile.am(0 hunks)src/coinjoin/client.cpp(5 hunks)src/coinjoin/client.h(2 hunks)src/coinjoin/server.cpp(3 hunks)src/coinjoin/server.h(3 hunks)src/evo/mnauth.cpp(3 hunks)src/evo/mnauth.h(1 hunks)src/governance/governance.cpp(15 hunks)src/governance/governance.h(2 hunks)src/instantsend/instantsend.cpp(2 hunks)src/instantsend/instantsend.h(1 hunks)src/llmq/blockprocessor.cpp(1 hunks)src/llmq/dkgsessionhandler.cpp(3 hunks)src/llmq/dkgsessionhandler.h(5 hunks)src/llmq/dkgsessionmgr.cpp(5 hunks)src/llmq/dkgsessionmgr.h(2 hunks)src/llmq/quorums.cpp(2 hunks)src/llmq/quorums.h(1 hunks)src/net_processing.cpp(9 hunks)src/net_processing.h(1 hunks)src/protocol.h(2 hunks)src/spork.cpp(3 hunks)src/spork.h(1 hunks)src/util/expected.h(0 hunks)test/lint/lint-include-guards.py(1 hunks)test/lint/lint-spelling.py(1 hunks)test/lint/lint-whitespace.py(0 hunks)
💤 Files with no reviewable changes (4)
- test/lint/lint-whitespace.py
- src/Makefile.am
- contrib/devtools/copyright_header.py
- src/util/expected.h
🚧 Files skipped from review as they are similar to previous changes (15)
- test/lint/lint-spelling.py
- src/coinjoin/server.h
- src/spork.h
- src/protocol.h
- src/coinjoin/client.h
- src/llmq/quorums.h
- src/llmq/blockprocessor.cpp
- src/llmq/dkgsessionmgr.h
- src/evo/mnauth.h
- test/lint/lint-include-guards.py
- src/instantsend/instantsend.cpp
- src/governance/governance.cpp
- src/spork.cpp
- src/net_processing.h
- src/instantsend/instantsend.h
🧰 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/quorums.cppsrc/coinjoin/server.cppsrc/llmq/dkgsessionhandler.cppsrc/coinjoin/client.cppsrc/governance/governance.hsrc/llmq/dkgsessionmgr.cppsrc/llmq/dkgsessionhandler.hsrc/evo/mnauth.cppsrc/net_processing.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/evo/mnauth.cpp
🧠 Learnings (7)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#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.
📚 Learning: 2025-02-06T14:34:30.466Z
Learnt from: kwvg
PR: dashpay/dash#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.
Applied to files:
src/coinjoin/server.cppsrc/llmq/dkgsessionmgr.cpp
📚 Learning: 2025-07-29T14:32:48.369Z
Learnt from: kwvg
PR: dashpay/dash#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/coinjoin/server.cpp
📚 Learning: 2025-01-14T09:07:12.446Z
Learnt from: kwvg
PR: dashpay/dash#6532
File: src/net.cpp:4329-4329
Timestamp: 2025-01-14T09:07:12.446Z
Learning: Keep suggestions focused on the scope of the current commit/PR. Avoid suggesting unrelated improvements that should be handled in separate PRs, even if technically valid.
Applied to files:
src/llmq/dkgsessionmgr.cpp
📚 Learning: 2025-06-09T16:43:20.996Z
Learnt from: kwvg
PR: dashpay/dash#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/dkgsessionmgr.cpp
📚 Learning: 2025-07-29T14:33:01.040Z
Learnt from: kwvg
PR: dashpay/dash#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/dkgsessionmgr.cpp
📚 Learning: 2025-07-23T09:28:32.783Z
Learnt from: kwvg
PR: dashpay/dash#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/dkgsessionmgr.cpp
🧬 Code Graph Analysis (8)
src/llmq/quorums.cpp (4)
src/llmq/blockprocessor.cpp (2)
ProcessMessage(57-154)ProcessMessage(57-58)src/llmq/dkgsessionmgr.cpp (2)
ProcessMessage(95-187)ProcessMessage(95-96)src/net_processing.cpp (13)
ProcessMessage(3510-5294)ProcessMessage(3510-3515)pfrom(616-616)pfrom(639-640)pfrom(722-724)pfrom(730-730)pfrom(738-738)pfrom(741-741)pfrom(743-743)pfrom(745-745)pfrom(838-838)pfrom(1020-1020)ret(654-654)src/streams.h (3)
CDataStream(348-384)CDataStream(359-359)CDataStream(360-383)
src/coinjoin/server.cpp (3)
src/llmq/blockprocessor.cpp (2)
ProcessMessage(57-154)ProcessMessage(57-58)src/coinjoin/client.cpp (6)
ProcessMessage(41-142)ProcessMessage(41-42)ProcessMessage(144-166)ProcessMessage(144-144)ProcessMessage(183-230)ProcessMessage(183-183)src/llmq/quorums.cpp (2)
ProcessMessage(706-889)ProcessMessage(706-706)
src/llmq/dkgsessionhandler.cpp (5)
src/llmq/signing.cpp (3)
ret(31-31)ProcessMessage(407-447)ProcessMessage(407-407)src/llmq/blockprocessor.cpp (2)
ProcessMessage(57-154)ProcessMessage(57-58)src/instantsend/instantsend.cpp (2)
ProcessMessage(109-172)ProcessMessage(109-109)src/llmq/dkgsessionmgr.cpp (2)
ProcessMessage(95-187)ProcessMessage(95-96)src/llmq/quorums.cpp (2)
ProcessMessage(706-889)ProcessMessage(706-706)
src/coinjoin/client.cpp (3)
src/coinjoin/server.cpp (2)
ProcessMessage(26-41)ProcessMessage(26-26)src/llmq/dkgsessionmgr.cpp (2)
ProcessMessage(95-187)ProcessMessage(95-96)src/llmq/quorums.cpp (2)
ProcessMessage(706-889)ProcessMessage(706-706)
src/governance/governance.h (3)
src/governance/governance.cpp (6)
SyncSingleObjVotes(922-971)SyncSingleObjVotes(922-922)SyncObjects(973-1032)SyncObjects(973-973)ProcessMessage(143-283)ProcessMessage(143-143)src/net_processing.cpp (8)
peer(668-668)peer(797-797)peer(902-902)peer(903-903)peer(1010-1010)peer(1018-1018)ProcessMessage(3510-5294)ProcessMessage(3510-3515)src/llmq/blockprocessor.cpp (2)
ProcessMessage(57-154)ProcessMessage(57-58)
src/llmq/dkgsessionmgr.cpp (4)
src/llmq/dkgsessionhandler.cpp (2)
ProcessMessage(155-168)ProcessMessage(155-155)src/net_processing.cpp (12)
ProcessMessage(3510-5294)ProcessMessage(3510-3515)pfrom(616-616)pfrom(639-640)pfrom(722-724)pfrom(730-730)pfrom(738-738)pfrom(741-741)pfrom(743-743)pfrom(745-745)pfrom(838-838)pfrom(1020-1020)src/llmq/quorums.cpp (2)
ProcessMessage(706-889)ProcessMessage(706-706)src/llmq/signing.cpp (2)
ProcessMessage(407-447)ProcessMessage(407-407)
src/evo/mnauth.cpp (4)
src/spork.cpp (2)
ProcessMessage(129-137)ProcessMessage(129-129)src/coinjoin/server.cpp (2)
ProcessMessage(26-41)ProcessMessage(26-26)src/net_processing.cpp (8)
ProcessMessage(3510-5294)ProcessMessage(3510-3515)peer(668-668)peer(797-797)peer(902-902)peer(903-903)peer(1010-1010)peer(1018-1018)src/llmq/signing.cpp (2)
ProcessMessage(407-447)ProcessMessage(407-407)
src/net_processing.cpp (6)
src/spork.cpp (3)
inv(461-461)ProcessMessage(129-137)ProcessMessage(129-129)src/coinjoin/server.cpp (3)
inv(354-354)ProcessMessage(26-41)ProcessMessage(26-26)src/instantsend/instantsend.cpp (3)
inv(419-419)ProcessMessage(109-172)ProcessMessage(109-109)src/llmq/dkgsession.cpp (4)
inv(301-301)inv(605-605)inv(816-816)inv(1171-1171)src/governance/governance.cpp (2)
ProcessMessage(143-283)ProcessMessage(143-143)src/coinjoin/client.cpp (6)
ProcessMessage(41-142)ProcessMessage(41-42)ProcessMessage(144-166)ProcessMessage(144-144)ProcessMessage(183-230)ProcessMessage(183-183)
🪛 GitHub Actions: Clang Diff Format Check
src/llmq/quorums.cpp
[error] 1-1: Clang-format differences detected. Run 'contrib/devtools/clang-format-diff.py -p1' to apply formatting changes.
src/coinjoin/server.cpp
[error] 1-1: Clang-format differences detected. Run 'contrib/devtools/clang-format-diff.py -p1' to apply formatting changes.
src/coinjoin/client.cpp
[error] 1-1: Clang-format differences detected. Run 'contrib/devtools/clang-format-diff.py -p1' to apply formatting changes.
src/governance/governance.h
[error] 1-1: Clang-format differences detected. Run 'contrib/devtools/clang-format-diff.py -p1' to apply formatting changes.
src/llmq/dkgsessionhandler.h
[error] 1-1: Clang-format differences detected. Run 'contrib/devtools/clang-format-diff.py -p1' to apply formatting changes.
src/evo/mnauth.cpp
[error] 1-1: Clang-format differences detected. Run 'contrib/devtools/clang-format-diff.py -p1' to apply formatting changes.
⏰ 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_nowallet-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: win64-build / Build source
- 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 (27)
src/governance/governance.h (4)
17-17: Good addition: include for std::string_viewIncluding <string_view> is appropriate for the updated ProcessMessage signature.
275-276: Approve: MessageProcessingResult migration verified — no remaining PeerMsgRet usagesrg shows no occurrences of PeerMsgRet. SyncSingleObjVotes/SyncObjects now return MessageProcessingResult and their call sites propagate/return that value.
Key locations:
- src/governance/governance.h: declarations now [[nodiscard]] MessageProcessingResult (lines ~275–276)
- src/governance/governance.cpp: callers returning the result (lines ~163–165)
- src/governance/governance.cpp: definitions — SyncSingleObjVotes (~line 922) and SyncObjects (~line 973)
No fixes required.
278-279: ProcessMessage signature & MessageProcessingResult visibility verified — no action requiredprotocol.h defines MessageProcessingResult and governance.h includes <protocol.h>, so the return type is visible. governance.cpp implements the same signature and net_processing.cpp forwards its result to PostProcessMessage, so the change (std::string_view + [[nodiscard]]) is compatible. Quick scans found no BCLog(...) typos and IsBlockchainSynced usages are correct.
Checked locations:
- src/governance/governance.h: declaration (line ~278)
- src/governance/governance.cpp: implementation (line 143)
- src/protocol.h: struct MessageProcessingResult (line 594)
- src/net_processing.cpp: caller (PostProcessMessage(m_govman.ProcessMessage(...)) at ~line 5267)
275-279: Fix formatting: run clang-format locally and commit fixesI ran the suggested script but it failed because
clang-formatis not installed in the runner (FileNotFoundError). CI reported formatting differences — please run the project formatter locally and commit the changes.
- File to check: src/governance/governance.h (around lines 275–279)
Suggested steps:
- Install clang-format (e.g., apt/brew/choco).
- Apply formatting to the PR diff (recommended):
git diff -U0 origin/<base-branch>...HEAD | contrib/devtools/clang-format-diff.py -p1 -i
or format the file directly:clang-format -i src/governance/governance.h- Commit & push the formatting changes and re-run CI.
src/llmq/quorums.cpp (3)
706-716: Clean migration to MessageProcessingResult with consistent local error handlerThe signature and the local errorHandler lambda align with the new result plumbing. The pattern is consistent with other LLMQ components.
725-749: sendQDATA now returns a structured outcome; good use of non-punitive pathsReturning MessageProcessingResult from sendQDATA and gating punishment on request_limit_exceeded (except for missing-data cases) matches the intended behavior. Looks good.
832-887: QDATA receive path returns structured errors without over-penalizing; cache access is safe
- Using 0-score for expected mismatches (not requested, already received, not like requested) avoids unnecessary bans.
- The mapQuorumsCache access via LOCK(cs_map_quorums) is correct.
src/coinjoin/server.cpp (2)
26-41: Server ProcessMessage now returns MessageProcessingResult and defers DSQUEUE to NodeId-based handlerThe return-type migration is consistent, and delegating DSQUEUE to a NodeId-based function fits the new pattern used in net_processing.
112-189: DSQUEUE: structured outcomes, correct ret.m_to_erase, and careful scoring
- m_to_erase is set to MSG_DSQ + hash.
- Misbehavior scoring (100 for malformed IDs, 10 for signature/identity issues) tracks previous semantics without being overly punitive for rate-limits/time-bounds.
- TRY_LOCK fallbacks return the current ret, which is appropriate for deferring post-processing without losing to_erase.
src/llmq/dkgsessionhandler.cpp (2)
61-91: Good: PushPendingMessage returns MessageProcessingResult and tags m_to_erase
- m_to_erase = {invType, hash} when from != -1 ensures the caller can clean up object requests.
- Returning ret on all early exits preserves the to_erase instruction. Nice.
155-168: CDKGSessionHandler::ProcessMessage returns MessageProcessingResult and defers workEarly return for unknown msg_type and forwarding to pending queues via PushPendingMessage is appropriate and non-blocking.
src/coinjoin/client.cpp (2)
56-71: Structured DSQUEUE handling: ret.m_to_erase and misbehavior scoring are correct
- m_to_erase is set promptly.
- Using MisbehavingError{100} for invalid IDs and {10} for missing/invalid MN cases mirrors server-side semantics.
96-117: Locking and rate-limit behavior are sound
- The outer cs_ProcessDSQueue and inner cs_vecqueue locks keep queue mutation safe.
- Returning ret on timeout, missing dmn, and threshold checks avoids side effects while still instructing cleanup via to_erase.
src/evo/mnauth.cpp (3)
58-68: Guard and return-type migration look goodEarly exit on non-MNAUTH and unsynced state is correct given we can't validate without the latest MN list. The MessageProcessingResult return value aligns with the project-wide change.
72-90: Direct MisbehavingError returns simplify error pathsReplacing tl::unexpected and returning MisbehavingError directly makes control-flow clearer. The scoring per condition (duplicate, services, empty hash, invalid sig) is sensible.
114-118: Appropriate low-score on verification failureUsing MisbehavingError{10} when the MN can’t verify yet (race with list updates) matches the documented intent to allow nodes to catch up.
src/llmq/dkgsessionhandler.h (2)
79-83: LGTM: ProcessMessage/PushPendingMessage migration to MessageProcessingResultThe transition to [[nodiscard]] MessageProcessingResult looks consistent and aligns with centralized post-processing in net_processing.
Also applies to: 168-170
1-1: Clang-format differences flagged in CICI indicates clang-format drift. Please run:
contrib/devtools/clang-format-diff.py -p1src/llmq/dkgsessionmgr.cpp (3)
95-97: LGTM: Return type migrated to MessageProcessingResultThe handler’s signature and return value are correctly updated and propagated through the call chain.
112-120: QWATCH gating and misbehavior handling look correctEarly return for non-masternodes with MisbehavingError{10}, and setting qwatch for valid peers, matches expected behavior.
184-187: NodeId-based dispatch looks consistentUsing pfrom.GetId() and forwarding msg_type and vRecv down to the session handler matches the PR’s NodeId-centric flow.
src/net_processing.cpp (6)
630-634: LGTM: RelayInv/RelayInvFiltered signatures now take const refsThe updated const-ref interfaces reduce copies and are correctly reflected in overrides.
2345-2365: LGTM: RelayInv implementations match new signaturesBoth overloads correctly forward const CInv& to PushInv and respect capability checks.
2395-2443: LGTM: Filtered relay API uses const referencesThe Bloom filter checks and const-ref usage on related objects are appropriate.
3494-3508: LGTM: Centralized post-processing handles inventory vector
- Misbehavior applied via PeerManager
- Erase queued request guarded by cs_main
- Transactions and inventories relayed through existing paths
This matches the new MessageProcessingResult shape (vector inventory).
5259-5287: LGTM: Unified PostProcessMessage flow across subsystemsAll migrated handlers now return MessageProcessingResult and are funneled through PostProcessMessage with NodeId, removing ProcessPeerMsgRet and tl::expected usage.
5256-5274: Consistency check: mixed CNode& and NodeId usageYou pass NodeId for modules migrated to NodeId (e.g., llmq::sigman, isman) and CNode& for others (e.g., sporkman, qman) as required by their current interfaces. This looks consistent with the staged migration.
…er functions a02e843 refactor: privatise `RequestObject` and `EraseObjectRequest` (Kittywhiskers Van Gogh) f95b50a refactor: privatise `RelayInvFiltered` and `AskPeersForTransaction` (Kittywhiskers Van Gogh) c93ade5 refactor: reduce `PeerManager` usage in InstantSend worker functions (Kittywhiskers Van Gogh) 9b2c916 refactor: drop `is_masternode` from `PeerManager` interface functions (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Depends on #6815 * Depends on #6820 * While this pull request doesn't _entirely_ decouple `PeerManager` from `CInstantSendManager` as the worker thread isn't _reactive_ (i.e. triggered by `ProcessMessage()`), we can reduce its usage by folding all expected network activity into a `MessageProcessingResult` batch that can be consumed by `PostProcessMessage`. To achieve this, `MessageProcessingResult` has been extended to allow * Requesting peers for a transaction (by setting `m_request_tx`) * Relaying inventories to interested peer based on filter match (by setting `m_inv_filter`) This has the additional effect of removing external uses of `RelayInvFiltered()` and `AskPeersForTransaction()`, which can now be safely removed from the `PeerManager` interface as it is now brokered through `MessageProcessingResult`. * Note that while the old `RelayInvFiltered()` allowed specifying _any_ minimum protocol version, `PostProcessMessage()` will assume that `minProtoVersion` is `ISDLOCK_PROTO_VERSION`. This is acceptable as `RelayInvFiltered()` isn't used anywhere else and deterministic InstantSend locks were introduced in v0.18 (see [dash#4381](#4381)). If there is any foreseeable use needing to relay messages to even older versions of Dash Core, `m_inv_filter` can be modified to accommodate that. ## Breaking Changes None expected. ## Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: knst: utACK a02e843 UdjinM6: utACK a02e843 Tree-SHA512: 6a83277781faf996cbb5187d352971c3508191dbf84cd801e062e2cde2db09cf64696bf27b9790f2b278f53adb46c042e04ac5c4dbdebdbc1faba2b262e39930
4 participants
Additional Information
Dependency for refactor: reduce
PeerManagerusage in InstantSend worker functions #6821To avoid having to redeclare
NodeIdrepeatedly to avoid circular dependencies from includingnet.h(source), a redeclaration is made in the relatively lightweightnet_types.hthat as ofdevelop(fb87a5a), doesn't have includes outside the standard library (source).Redeclarations were flagged during review (see comment, comment).
As
tl::expectedwas introduced alongsidePeerMsgRetin dash#5782 and has not been used elsewhere, we can safely remove it as we've finished migrating all usage toMessageProcessingResult.Breaking Changes
None expected.
Checklist