fix: adjust condition to detect suitable pending masternodes #6983

UdjinM6 · 2025-11-16T15:57:58Z

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 · 2025-11-16T15:58:29Z

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

coderabbitai · 2025-11-16T16:01:18Z

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

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 9cde8d4b024822acae3bb596947912dfc498373f and 96febc30c8b626b1b7a58c80f2ab2534e90c5228.

📒 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:

Line 3457: Explicitly setting fExcludeDisconnecting=false ensures FindNode returns disconnecting nodes (rather than nullptr)

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

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

PastaPastaPasta

utACK 96febc30c8b626b1b7a58c80f2ab2534e90c5228

knst

LGTM b9310be

…rum masternodes 0fce90a fix: correctly detect and skip disconnecting pending quorum masternodes (UdjinM6) Pull request description: ## 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)_ ACKs for top commit: kwvg: utACK 0fce90a Tree-SHA512: b92b2bf0d08bc814835a29333895c1328b0a12c6b7d612e980b59895d80f6c17ed9c228f65580cb10c0e1f1fd66304093a8224761525302782a3f19bae2a63f3

UdjinM6 added this to the 23.1 milestone Nov 16, 2025

UdjinM6 force-pushed the fix_pending_masternodes_connection branch 2 times, most recently from b6193ef to 96febc3 Compare November 16, 2025 16:09

UdjinM6 mentioned this pull request Nov 16, 2025

fix: correctly detect and skip disconnecting pending quorum masternodes #6984

Merged

5 tasks

UdjinM6 requested review from PastaPastaPasta and knst November 18, 2025 07:50

PastaPastaPasta approved these changes Nov 19, 2025

View reviewed changes

fix: adjust condition to detect suitable pending masternodes

b9310be

UdjinM6 force-pushed the fix_pending_masternodes_connection branch from 96febc3 to b9310be Compare November 23, 2025 08:42

knst approved these changes Nov 24, 2025

View reviewed changes

PastaPastaPasta merged commit 73db539 into dashpay:develop Nov 24, 2025
32 of 33 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: adjust condition to detect suitable pending masternodes #6983

fix: adjust condition to detect suitable pending masternodes #6983

Uh oh!

UdjinM6 commented Nov 16, 2025

Uh oh!

github-actions bot commented Nov 16, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Nov 16, 2025 •

edited

Loading

Uh oh!

PastaPastaPasta left a comment

Uh oh!

knst left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fix: adjust condition to detect suitable pending masternodes #6983

fix: adjust condition to detect suitable pending masternodes #6983

Uh oh!

Conversation

UdjinM6 commented Nov 16, 2025

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

github-actions bot commented Nov 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ No Merge Conflicts Detected

Uh oh!

coderabbitai bot commented Nov 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Sequence Diagram

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Nov 16, 2025 •

edited

Loading

coderabbitai bot commented Nov 16, 2025 •

edited

Loading