fix: correctly detect and skip disconnecting pending quorum masternodes

[UdjinM6]

Issue being fixed or feature implemented

Similar to #6983, incorrect refactoring in #6912

What was done?

Correctly detect and skip disconnecting pending quorum masternodes

How Has This Been Tested?

Run tests

Breaking Changes

n/a

Checklist:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

[github-actions]

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

[coderabbitai]

Walkthrough

The FindNodeMutable() method in CConnman gains a second boolean parameter fExcludeDisconnecting to control whether disconnecting nodes are excluded. Call sites were updated to pass /*fExcludeDisconnecting=*/false where appropriate. The selection logic was changed to skip nodes when either the node is a masternode connection or pnode->fDisconnect is true.

Sequence Diagram(s)

sequenceDiagram
    participant Caller
    participant CConnman
    participant PNodeList as "pnode (iterable)"

    Caller->>CConnman: FindNodeMutable(addr, fExcludeDisconnecting=false)
    Note right of CConnman: iterate over connected nodes
    CConnman->>PNodeList: for each pnode
    alt pnode is masternode connection OR pnode->fDisconnect == true
        Note right of PNodeList `#f9f0c1`: SKIP node (new combined guard)
        PNodeList-->>CConnman: continue
    else match addr
        PNodeList-->>CConnman: return pnode
    end
    CConnman-->>Caller: pnode or nullptr

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Check all call sites of FindNodeMutable for correct argument usage and compile impact.
• Inspect src/net.cpp changes: iteration/skip logic combining masternode check and pnode->fDisconnect.
• Verify comments/documentation reflect the parameter semantics.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'fix: correctly detect and skip disconnecting pending quorum masternodes' is directly and specifically related to the main changeset, which modifies FindNodeMutable to properly detect and skip disconnecting nodes.
Description check	✅ Passed	The description clearly relates to the changeset by referencing the incorrect refactoring in #6912 and explaining the fix involves correctly detecting and skipping disconnecting pending quorum masternodes.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 42809cf and 5ec4651.

📒 Files selected for processing (1)

• src/net.cpp (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• src/net.cpp

⏰ 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_tsan-build / Build source
• GitHub Check: mac-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: arm-linux-build / Build source
• GitHub Check: linux64_nowallet-build / Build source
• GitHub Check: win64-build / Build source
• GitHub Check: Lint / Run linters

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between caf5010 and 42809cf.

📒 Files selected for processing (1)

• src/net.cpp (1 hunks)

🧰 Additional context used

🧠 Learnings (1)

📓 Common learnings

Learnt from: UdjinM6
Repo: dashpay/dash PR: 6933
File: src/llmq/utils.cpp:284-298
Timestamp: 2025-11-04T18:24:27.241Z
Learning: In consensus-critical code (such as quorum formation, block validation, or deployment activation logic), do not suggest changes to the logic itself even if the implementation appears theoretically incorrect or off-by-one. Consensus rules, once deployed on the Dash network, must be preserved exactly to avoid network forks. Refactoring PRs should maintain perfect behavioral equivalence. Only suggest logic changes if explicitly accompanied by a DIP (Dash Improvement Proposal) or if the maintainer indicates the consensus rule needs to be changed with appropriate activation logic.

Learnt from: kwvg
Repo: dashpay/dash PR: 6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.

Learnt from: kwvg
Repo: dashpay/dash PR: 6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.

Learnt from: kwvg
Repo: dashpay/dash PR: 6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.

🔇 Additional comments (1)

src/net.cpp (1)

3385-3389: LGTM - Correctly detects and skips disconnecting nodes.

The fix appropriately addresses the issue from PR #6912 by:

• Calling FindNodeMutable with fExcludeDisconnecting=false to retrieve nodes even if they're disconnecting
• Checking both m_masternode_connection and fDisconnect to skip nodes that are either already masternode connections or in a disconnecting state

This prevents attempting to establish quorum connections to nodes that are disconnecting, which was the root cause of issue #6983.

[PastaPastaPasta]

utACK 5ec4651