refactor: drop redundant CConnman argument from NetHandler::ProcessGetData - #7538
Conversation
|
Warning Review limit reached
Next review available in: 3 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Final review complete — no blockers (commit c01370e) |
…tData 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 a56c106, 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.
91016d8 to
c01370e
Compare
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
LGTM; waiting for CI to be happy, if this builds it's ok
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The refactor consistently removes the redundant CConnman parameter from NetHandler::ProcessGetData, all three overrides, and the sole dispatch call site. Each handler uses the same existing CConnman reference supplied during runtime construction, while observer-mode NetDKG still exits safely before sending replies; no stale signatures, behavioral regressions, or commit-history concerns were found.
Source: reviewers codex/general=gpt-5.6-sol(completed); codex/dash-core-commit-history=gpt-5.6-sol(completed); claude/general=claude-sonnet-5(completed); claude/dash-core-commit-history=claude-sonnet-5(failed); claude/dash-core-commit-history=claude-sonnet-5(failed); claude/dash-core-commit-history=claude-sonnet-5(completed); verifier=codex/final-verifier=gpt-5.6-sol(completed) fallback_for_sonnet_verifier=true; coordinator=openclaw-agent/cliproxy/gpt-5.6-sol(orchestration-only).
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (completed)
The shadowMember re-enable left dangling parameter names after dashpay#7538/dashpay#7539 dropped those parameters (connman_in / signing_manager), which broke the build. Finish the ProcessVoteAndRelay cleanup by removing the now-unused CConnman plumbing from GovernanceSigner, governance RPC helpers, and node interfaces. Cap cppcheck jobs and treat abnormal cppcheck exits as failures so CI analysis cannot pass vacuous when the child process dies.
The shadowMember re-enable left dangling parameter names after dashpay#7538/dashpay#7539 dropped those parameters (connman_in / signing_manager), which broke the build. Finish the ProcessVoteAndRelay cleanup by removing the now-unused CConnman plumbing from GovernanceSigner, governance RPC helpers, and node interfaces. Cap cppcheck jobs and treat abnormal cppcheck exits as failures so CI analysis cannot pass vacuous when the child process dies.
2 participants
Issue being fixed or feature implemented
NetHandler::ProcessGetDatatakes aCConnman&argument that no implementation actually needs. Every handler that overrides it already holds its ownCConnman&:CCoinJoinServer::connmanNetGovernance::m_connmanNetDKG::ActiveDKG::connmanIn a running node there is exactly one
CConnman—node.connmanininit.cpp— and that is the same objectPeerManagerImplpasses asm_connmanat the call site, so the argument was always identical to the member.The parameter is a mechanical artifact of a56c106 ("refactor: drop dependency of PeerManager on CoinJoinServer"), which moved these branches out of
PeerManagerImpl::ProcessGetData. The moved lines readm_connman.PushMessage(...), som_connmanwas promoted to a parameter to keep the hunk a 1:1 move. Nobody reconciled it with the members the handlers already had.In
CCoinJoinServer::ProcessGetDatathe parameter additionally shadowed the class member of the same name, so the body silently used the argument while every other method in the file uses the member.What was done?
Dropped
CConnman& connmanfrom theNetHandler::ProcessGetDatavirtual and from all three overrides; each handler now replies through theCConnmanit already holds.msgMakerstays a parameter — it isCNetMsgMaker(pfrom.GetCommonVersion()), genuinely per-peer state the handler cannot reconstruct on its own.NetDKGkeeps itsCConnmanwhere it already lived, in the active-mode-onlyActiveDKGbundle, and replies viam_active->connman. One consequence worth noting for review: this makes the existingm_active == nullptrearly return guard the replies rather than merely short-circuit them, so the comment above it is updated to say so. That is not a behavior change — the class doc already documentsProcessGetDataas active-mode only, because the underlyingGet*calls return false by construction in observer mode.No behavior change: same object, same call, in every case.
How Has This Been Tested?
Not built or run locally — this branch was reviewed by inspection. Verified by grep that no
ProcessGetDatadeclaration, definition, or call site still carries aCConnmanargument. Relying on CI for the compile and test run.Breaking Changes
None.
NetHandleris an internal interface with no external implementors.Checklist: