fix: adjust condition to detect suitable pending masternodes

[UdjinM6]

Issue being fixed or feature implemented

Mixing is broken on develop, mixing wallet can't connect to masternodes.

This condition was incorrectly refactored in #6912, https://github.com/dashpay/dash/pull/6912/files#diff-00021eed586a482abdb09d6cdada1d90115abe988a91421851960e26658bed02R3454-R3458.

What was done?

see commit

How Has This Been Tested?

Mixing works again

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 change updates CConnman::ThreadOpenMasternodeConnections in src/net.cpp to broaden when a pending masternode connection is opened. FindNode is now called with fExcludeDisconnecting=false to include disconnecting entries. A pending masternode connection is opened when either (a) no existing node entry is found for the target primary address (nullptr), or (b) an entry exists that is not a masternode_connection and is not disconnecting. Existing behavior of logging and returning the masternode candidate when no masternode connection is present is preserved.

Sequence Diagram

sequenceDiagram
    participant ThreadOpenMasternodeConnections as Thread
    participant FindNode
    participant ConnectionLogic as ConnLogic

    rect rgb(230,245,255)
    Note over Thread,FindNode: Previous Behavior
    Thread->>FindNode: FindNode(primary_address, fExcludeDisconnecting=true)
    alt pnode != nullptr
        FindNode-->>Thread: node found
        Thread->>ConnLogic: if not masternode_connection -> attempt
        alt Not masternode_connection
            ConnLogic-->>Thread: Open pending masternode connection
        else Already masternode_connection
            ConnLogic-->>Thread: Skip
        end
    else pnode == nullptr
        FindNode-->>Thread: nullptr
        Thread->>ConnLogic: Skip attempt (no node entry)
    end
    end

    rect rgb(245,230,255)
    Note over Thread,FindNode: New Behavior
    Thread->>FindNode: FindNode(primary_address, fExcludeDisconnecting=false)
    alt pnode == nullptr
        FindNode-->>Thread: nullptr
        rect rgb(200,255,200)
        Note over ConnLogic: NEW: proceed when no node entry
        Thread->>ConnLogic: Open pending masternode connection
        end
    else pnode != nullptr
        FindNode-->>Thread: node found (may be disconnecting)
        Thread->>ConnLogic: if not masternode_connection AND not disconnecting -> attempt
        alt Not masternode_connection AND not disconnecting
            ConnLogic-->>Thread: Open pending masternode connection
        else masternode_connection OR disconnecting
            ConnLogic-->>Thread: Skip
        end
    end
    end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Files/areas to review:
  • src/net.cpp: exact conditional changes and the FindNode call arguments
  • Logging paths that report masternode candidate/no-connection
  • Any code interacting with node disconnecting state to ensure the new check is correct
  • Tests or assertions covering the new nullptr path and disconnecting-case handling

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: adjusting a condition for detecting suitable pending masternodes, which directly relates to the code modification in src/net.cpp.
Description check	✅ Passed	The description is related to the changeset, explaining that a mixing-related condition was incorrectly refactored in PR #6912 and has been fixed in this PR to restore masternode connectivity.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ 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 9cde8d4 and 96febc3.

📒 Files selected for processing (1)

• src/net.cpp (1 hunks)

🧰 Additional context used

🧠 Learnings (1)

📓 Common learnings

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.

Learnt from: UdjinM6
Repo: dashpay/dash PR: 6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.

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.

⏰ 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: arm-linux-build / Build source
• GitHub Check: linux64_sqlite-build / Build source
• GitHub Check: linux64_ubsan-build / Build source
• GitHub Check: linux64-build / Build source
• GitHub Check: linux64_fuzz-build / Build source
• GitHub Check: win64-build / Build source
• GitHub Check: linux64_nowallet-build / Build source
• GitHub Check: linux64_tsan-build / Build source

🔇 Additional comments (1)

src/net.cpp (1)

3457-3463: LGTM! Correctly fixes disconnecting node detection.

The fix properly addresses the mixing connection issue by:

1. Line 3457: Explicitly setting fExcludeDisconnecting=false ensures FindNode returns disconnecting nodes (rather than nullptr)
2. Line 3458: The condition !pnode->fDisconnect prevents connection attempts when a node is disconnecting

This avoids the race condition where the old code would return nullptr for disconnecting nodes, triggering a connection attempt to an address that still has a node in the process of disconnecting.

The logic now correctly opens a pending masternode connection only when:

• No existing node entry exists (pnode == nullptr), OR
• An existing node is neither a masternode connection nor disconnecting

---

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.}

[PastaPastaPasta]

utACK 96febc3