refactor: drop redundant CSigningManager argument from AsyncSignIfMember - #7539
Conversation
CSigSharesManager has held a CSigningManager& member since a352456, and there is exactly one CSigningManager in a node (llmq_ctx->sigman). ActiveContext hands that same reference to CSigSharesManager and to every caller, so all five call sites already passed the object the member points at. The parameter was added by 0052fca, which moved AsyncSignIfMember off CSigningManager onto CSigSharesManager. Before the move the method took the opposite manager (CSigSharesManager& shareman); afterwards that argument became 'this' and a sigman argument was added to reach back for the db. The member was already declared 34 lines above the new signature in the same header. The parameter shadowed that member, so the single sigman.GetDb() in the body silently used the argument while the surrounding code reaches for qman and m_chainman as members. Dropping the parameter leaves the body unchanged; sigman now resolves to the member. No behavior change.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Walkthrough
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 7c05e56) |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The refactor consistently removes the redundant CSigningManager parameter from the declaration, definition, and all five call sites. CSigSharesManager already stores the same signing-manager instance supplied through ActiveContext, so sigman.GetDb() now resolves to the existing member without changing behavior; no stale calls, unused variables, 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(completed after 3 unparseable attempt(s)); verifier=codex/final-verifier=gpt-5.6-sol(completed, Sonnet-verifier fallback); 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 (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.
Issue being fixed or feature implemented
CSigSharesManager::AsyncSignIfMembertakes aCSigningManager&argument it does not need. The class has held aCSigningManager& sigmanmember since a352456 (#4988), declared 34 lines above the method in the same header, and the parameter shadows it.There is exactly one
CSigningManagerin a node —node.llmq_ctx->sigman.init.cppthreads it intoActiveContext, which hands the same reference toCSigSharesManager's member and to each caller's own member, so every call site already passes the object the member points at:chainlock/signing.cpp:177m_sigmaninstantsend/signing.cpp:356,:415m_sigmanllmq/ehf_signals.cpp:82sigmanrpc/quorums.cpp:533*llmq_ctx.sigmanThe parameter comes from 0052fca ("refactor: move
AsyncSignIfMember()toCSigSharesManager"). Before that move the method lived onCSigningManagerand took the opposite manager,CSigSharesManager& shareman. After the move that argument becamethis, and asigmanargument was added to reach back for the recovered-sigs db — a mirror-image swap that is correct in isolation, but that nobody reconciled with the member the destination class already had.Consequence today: the single
sigman.GetDb()in the body resolves to the argument, while the surrounding code in the same function reaches forqmanandm_chainmanas members.What was done?
Dropped the
CSigningManager& sigmanparameter from the declaration, the definition, and all five call sites.The body is untouched —
sigmannow resolves to the member, which is the same object it already received. Parameter-list continuation lines keep their alignment because the opening paren does not move.How Has This Been Tested?
Not built or run locally — reviewed by inspection. Verified by grep that no declaration, definition, or call site still passes a
CSigningManagertoAsyncSignIfMember, and that removing the argument leaves no caller-side variable unused (llmq_ctxinrpc/quorums.cppis still used a few lines below). Relying on CI for the compile and test run.Worth noting for reviewers: this class of shadowing is invisible to our warning set. Dash configures
-Wshadow-field, which only fires when a field shadows a base class field; a parameter shadowing a member of the same class needs-Wshadow-all(or GCC's-Wshadow).Breaking Changes
None.
Checklist: