revert: "fix: avoid improper dual way connection attempts" + tests

[knst]

Issue being fixed or feature implemented

#7255
Intermittent failure at feature_llmq_signing.py --spork21

File "feature_llmq_signing.py", line 111, in run_test
    wait_for_sigs(True, False, True, 15)
File "feature_llmq_signing.py", line 60, in wait_for_sigs
    self.wait_until(lambda: check_sigs(hasrecsigs, isconflicting1, isconflicting2), timeout = timeout)
AssertionError: Predicate '' not true after <timeout> seconds

What was done?

This reverts a57a811 from PR #6967.

relayMembers and connections in EnsureQuorumConnections are not interchangeable.

• connections: who this node should connect TO. For each pair (A, B) only the deterministic-outbound side is listed, so the pair results in one TCP connection.

• relayMembers: who this node should ask to push recovered sigs. For every already-connected MN in the set we send QSENDRECSIGS=true, and the peer then flips m_wants_recsigs=true on its side. For the handshake to happen in both directions,
  the set must list every other quorum member -- not just the outbound half.

After a57a811, only the outbound half is listed, so on the inbound half of each pair m_wants_recsigs stays false. RelayRecoveredSig only pushes QSIGREC to peers with m_wants_recsigs=true, so half of all proactive recovered-sig pushes are silently dropped.

This only triggers with spork21 on (IsAllMembersConnectedEnabled returns true). In this case both the path that uses the half-mesh outbound subset and the path that relies on proactive QSIGREC.

How Has This Been Tested?

This fixes the functional test feature_llmq_signing.py --spork21, which times out in wait_for_sigs.
It reduced amount of failures on my localhost from ~50% to almost 0.

Added new functional test in feature_llmq_signing.py for --spork21 case; which fails as expected without this patch.

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

[github-actions]

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

[thepastaclaw]

✅ Review complete (commit c1cdb75)

[coderabbitai]

Walkthrough

The EnsureQuorumConnections function in src/llmq/utils.cpp now separates relay member computation from outbound connection tracking. When the local node is a quorum member, relay membership is set to either all other quorum members (if all-members-connected mode is enabled) or a ring-based relay subset; when the node is not a member, relay members are set equal to the watch-connection-derived connections. The function now calls SetMasternodeQuorumRelayMembers(..., relayMembers) only when relayMembers is non-empty, instead of always using connections. In test/functional/feature_llmq_signing.py, the LLMQ signing test (with spork21 enabled) now polls peer statistics to assert transport-level symmetry of QSENDRECSIGS by verifying that both sent and received qsendrecsigs byte counters are observed for each masternode edge, failing if symmetry is not seen within the timeout.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ 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%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description check	✅ Passed	The pull request description clearly relates to the changeset, documenting a revert of a previous commit and explaining the bug fix with concrete reasoning about connections vs relay members.
Title check	✅ Passed	The title accurately describes the main change: reverting a previous fix commit and adding related tests.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[thepastaclaw]

Code Review

Clean revert restoring the distinction between outbound connections and full-mesh relayMembers in EnsureQuorumConnections, fixing intermittent feature_llmq_signing.py --spork21 failures. No correctness regressions identified. Two non-blocking suggestions about test coverage and documentation.

Reviewed commit: 0e33aa2

🟡 1 suggestion(s) | 💬 1 nitpick(s)

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/llmq/utils.cpp`:
- [SUGGESTION] lines 803-826: No regression guard against re-introducing the conflation bug
  This revert fixes an intermittent failure in `feature_llmq_signing.py --spork21` by restoring the relayMembers logic, but adds no targeted assertion that would catch the same regression in the future. The bug is silent — signatures still arrive eventually via fallback paths, just slowly enough to flake the test — so the existing functional test only catches it ~60% of the time per the commit message. A more direct check (e.g. that `m_wants_recsigs` is set on both sides of every quorum pair, or that the relay-members set is strictly a superset of the outbound-only connections set when spork21 is active) would prevent the same simplification from being merged again. The original PR #6967 was merged precisely because the existing tests didn't reliably catch the conflation.

[thepastaclaw]

💬 Nitpick: Document why connections and relayMembers must differ

The detailed rationale for keeping connections (outbound-only half-mesh) separate from relayMembers (every other quorum member, used to drive QSENDRECSIGS handshakes bidirectionally) lives only in the revert's commit message. The existing comment at lines 807-810 explains the all-members-connected branch but not the broader invariant. A short in-source comment near line 803 noting that the two sets are intentionally distinct — and why conflating them breaks recovered-sig propagation on the inbound side of each pair — would make the trap visible to future readers and reduce the chance of the same simplification being attempted again.

_{source: ['claude']}

[PastaPastaPasta]

utACK 0e33aa2

[thepastaclaw]

Code Review

Clean revert of a problematic change in EnsureQuorumConnections, restoring the correct distinction between outbound-only connections and full-mesh relayMembers. The new assert_qsendrecsigs_symmetric functional test provides a deterministic regression guard for the bidirectional QSENDRECSIGS handshake. No remaining correctness issues.

Reviewed commit: c1cdb75

[thepastaclaw]

No blockers from me on the current head; remaining comments are advisory suggestions/nits.

[UdjinM6]

utACK c1cdb75