From c01370e5de96d9b617a9f6a0918d24c950982e0a Mon Sep 17 00:00:00 2001 From: pasta Date: Mon, 3 Aug 2026 16:56:35 -0500 Subject: [PATCH] refactor: drop redundant CConnman argument from NetHandler::ProcessGetData Every NetHandler that overrides ProcessGetData already holds its own CConnman reference, and in a running node there is exactly one CConnman (node.connman), which is also what PeerManagerImpl passes as m_connman. The parameter was a mechanical artifact of a56c10698ee, which moved these branches out of PeerManagerImpl::ProcessGetData and promoted the m_connman local to a parameter to keep the hunk a 1:1 move. In CCoinJoinServer the parameter additionally shadowed the class member of the same name. Drop the parameter from the virtual and have each handler use its own member. msgMaker stays a parameter: it carries the peer's common version and is genuinely per-peer. NetDKG keeps its CConnman in the active-mode-only ActiveDKG bundle and replies through m_active->connman, which is where it is actually used. That makes the existing m_active null check guard the replies rather than merely short-circuit them, so the comment above it is updated to say so; the class doc already documents ProcessGetData as active-mode only. No behavior change. --- src/coinjoin/server.cpp | 2 +- src/coinjoin/server.h | 2 +- src/governance/net_governance.cpp | 6 +++--- src/governance/net_governance.h | 2 +- src/llmq/net_dkg.cpp | 16 ++++++++-------- src/llmq/net_dkg.h | 2 +- src/net_processing.cpp | 2 +- src/net_processing.h | 2 +- 8 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/coinjoin/server.cpp b/src/coinjoin/server.cpp index 6f7494043f0e..6d9a3ba16578 100644 --- a/src/coinjoin/server.cpp +++ b/src/coinjoin/server.cpp @@ -1004,7 +1004,7 @@ bool CCoinJoinServer::AlreadyHave(const CInv& inv) return (inv.type == MSG_DSQ) ? m_queueman.HasQueue(inv.hash) : false; } -bool CCoinJoinServer::ProcessGetData(CNode& pfrom, const CInv& inv, CConnman& connman, const CNetMsgMaker& msgMaker) +bool CCoinJoinServer::ProcessGetData(CNode& pfrom, const CInv& inv, const CNetMsgMaker& msgMaker) { if (inv.type != MSG_DSQ) return false; diff --git a/src/coinjoin/server.h b/src/coinjoin/server.h index a04eb4993bd4..6e148871b9d0 100644 --- a/src/coinjoin/server.h +++ b/src/coinjoin/server.h @@ -111,7 +111,7 @@ class CCoinJoinServer : public CCoinJoinBaseSession, public NetHandler ~CCoinJoinServer(); void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv) override; - bool ProcessGetData(CNode& pfrom, const CInv& inv, CConnman& connman, const CNetMsgMaker& msgMaker) override; + bool ProcessGetData(CNode& pfrom, const CInv& inv, const CNetMsgMaker& msgMaker) override; bool AlreadyHave(const CInv& inv) override; void Schedule(CScheduler& scheduler) override; diff --git a/src/governance/net_governance.cpp b/src/governance/net_governance.cpp index 44465e4d5bf6..8516d092748b 100644 --- a/src/governance/net_governance.cpp +++ b/src/governance/net_governance.cpp @@ -286,14 +286,14 @@ bool NetGovernance::AlreadyHave(const CInv& inv) return !m_gov_manager.ConfirmInventoryRequest(inv); } -bool NetGovernance::ProcessGetData(CNode& pfrom, const CInv& inv, CConnman& connman, const CNetMsgMaker& msgMaker) +bool NetGovernance::ProcessGetData(CNode& pfrom, const CInv& inv, const CNetMsgMaker& msgMaker) { if (inv.type == MSG_GOVERNANCE_OBJECT) { if (!m_gov_manager.HaveObjectForHash(inv.hash)) return false; CDataStream ss(SER_NETWORK, pfrom.GetCommonVersion()); ss.reserve(1000); if (!m_gov_manager.SerializeObjectForHash(inv.hash, ss)) return false; - connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::MNGOVERNANCEOBJECT, ss)); + m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::MNGOVERNANCEOBJECT, ss)); return true; } if (inv.type == MSG_GOVERNANCE_OBJECT_VOTE) { @@ -301,7 +301,7 @@ bool NetGovernance::ProcessGetData(CNode& pfrom, const CInv& inv, CConnman& conn CDataStream ss(SER_NETWORK, pfrom.GetCommonVersion()); ss.reserve(1000); if (!m_gov_manager.SerializeVoteForHash(inv.hash, ss)) return false; - connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::MNGOVERNANCEOBJECTVOTE, ss)); + m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::MNGOVERNANCEOBJECTVOTE, ss)); return true; } return false; diff --git a/src/governance/net_governance.h b/src/governance/net_governance.h index 5eaf4155b01a..c6975af44f5c 100644 --- a/src/governance/net_governance.h +++ b/src/governance/net_governance.h @@ -28,7 +28,7 @@ class NetGovernance final : public NetHandler void ProcessMessage(CNode& peer, const std::string& msg_type, CDataStream& vRecv) override; bool AlreadyHave(const CInv& inv) override; - bool ProcessGetData(CNode& pfrom, const CInv& inv, CConnman& connman, const CNetMsgMaker& msgMaker) override; + bool ProcessGetData(CNode& pfrom, const CInv& inv, const CNetMsgMaker& msgMaker) override; private: CGovernanceManager& m_gov_manager; diff --git a/src/llmq/net_dkg.cpp b/src/llmq/net_dkg.cpp index 8659b3be6530..5eddc10c1a9d 100644 --- a/src/llmq/net_dkg.cpp +++ b/src/llmq/net_dkg.cpp @@ -557,18 +557,18 @@ bool NetDKG::AlreadyHave(const CInv& inv) return false; } -bool NetDKG::ProcessGetData(CNode& pfrom, const CInv& inv, CConnman& connman, const CNetMsgMaker& msgMaker) +bool NetDKG::ProcessGetData(CNode& pfrom, const CInv& inv, const CNetMsgMaker& msgMaker) { - // Default implementations of GetContribution and the other virtual methods - // return false in observer mode; m_active is only an early exit and does - // not affect logic. + // Observer mode holds no CConnman, so this null check guards the replies + // below rather than merely short-circuiting them. It costs no coverage: the + // Get* calls return false by construction in observer mode. if (m_active == nullptr) return false; switch (inv.type) { case MSG_QUORUM_CONTRIB: { CDKGContribution o; if (m_qdkgsman.GetContribution(inv.hash, o)) { - connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::QCONTRIB, o)); + m_active->connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::QCONTRIB, o)); return true; } return false; @@ -576,7 +576,7 @@ bool NetDKG::ProcessGetData(CNode& pfrom, const CInv& inv, CConnman& connman, co case MSG_QUORUM_COMPLAINT: { CDKGComplaint o; if (m_qdkgsman.GetComplaint(inv.hash, o)) { - connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::QCOMPLAINT, o)); + m_active->connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::QCOMPLAINT, o)); return true; } return false; @@ -584,7 +584,7 @@ bool NetDKG::ProcessGetData(CNode& pfrom, const CInv& inv, CConnman& connman, co case MSG_QUORUM_JUSTIFICATION: { CDKGJustification o; if (m_qdkgsman.GetJustification(inv.hash, o)) { - connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::QJUSTIFICATION, o)); + m_active->connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::QJUSTIFICATION, o)); return true; } return false; @@ -592,7 +592,7 @@ bool NetDKG::ProcessGetData(CNode& pfrom, const CInv& inv, CConnman& connman, co case MSG_QUORUM_PREMATURE_COMMITMENT: { CDKGPrematureCommitment o; if (m_qdkgsman.GetPrematureCommitment(inv.hash, o)) { - connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::QPCOMMITMENT, o)); + m_active->connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::QPCOMMITMENT, o)); return true; } return false; diff --git a/src/llmq/net_dkg.h b/src/llmq/net_dkg.h index 2b1d6988878a..81019392400e 100644 --- a/src/llmq/net_dkg.h +++ b/src/llmq/net_dkg.h @@ -69,7 +69,7 @@ class NetDKG final : public NetHandler void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv) override EXCLUSIVE_LOCKS_REQUIRED(!cs_indexed_quorums_cache); bool AlreadyHave(const CInv& inv) override; - bool ProcessGetData(CNode& pfrom, const CInv& inv, CConnman& connman, const CNetMsgMaker& msgMaker) override; + bool ProcessGetData(CNode& pfrom, const CInv& inv, const CNetMsgMaker& msgMaker) override; /** * Drives one phase-handler thread per ActiveDKGSessionHandler in active mode; * no-op in observer mode (no curSession to drive). diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 9e5df50aeece..97554d4ecfde 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2979,7 +2979,7 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic } for (auto& handler : m_handlers) { if (!push) { - push = handler->ProcessGetData(pfrom, inv, m_connman, msgMaker); + push = handler->ProcessGetData(pfrom, inv, msgMaker); } } diff --git a/src/net_processing.h b/src/net_processing.h index 70e136f43f82..761da8001c07 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -136,7 +136,7 @@ class NetHandler virtual bool AlreadyHave(const CInv& inv) { return false; } // It should return true, if there's data has been pushed - virtual bool ProcessGetData(CNode& pfrom, const CInv& inv, CConnman& connman, const CNetMsgMaker& msgMaker) { return false; } + virtual bool ProcessGetData(CNode& pfrom, const CInv& inv, const CNetMsgMaker& msgMaker) { return false; } protected: PeerManagerInternal* m_peer_manager; };