fix: keep sending ISDLOCK invs to non-MN peers with watchquorums by knst · Pull Request #7293 · dashpay/dash

knst · 2026-04-27T16:34:45Z

Issue being fixed or feature implemented

PR #6994 made masternodes skip ISDLOCK inv announcements to any peer with m_wants_recsigs set, on the premise that such peers can reconstruct the ISLOCK from the recsig.

It works for MN peers but it does not work for quorum observers running with -watchquorums: those nodes also opt in to recsigs via QSENDRECSIGS but they don't have a signer worker running, so they cannot reconstruct an ISDLOCK from a recsig and they still need the inv.

nodes[0] runs with -watchquorums and had progressively sent QSENDRECSIGS to all four MN peers; by the third call every MN saw nodes[0].m_wants_recsigs=true and skipped the inv to it.

What was done?

The ISDLOCK is skipped now only on the peer being verified masternode. Move the policy from PushInv (called for every inv type) to the three sites that actually relay MSG_ISDLOCK and have CNode in scope.

How Has This Been Tested?

Run functional test interface_dash_zmq.py multiple times.
This PR drops failure rate from 50% to 0.

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

PR dashpay#6994 made masternodes skip ISDLOCK inv announcements to any peer with m_wants_recsigs set, on the premise that such peers can reconstruct the ISLOCK from the recsig. It works for MN peers but it does not work for quorum observers running with -watchquorums: those nodes also opt in to recsigs via QSENDRECSIGS but they don't have a signer worker running, so they cannot reconstruct an ISDLOCK from a recsig and they still need the inv. nodes[0] runs with -watchquorums and had progressively sent QSENDRECSIGS to all four MN peers; by the third call every MN saw nodes[0].m_wants_recsigs=true and skipped the inv to it. This commit make the ISDLOCK skipped only on the peer being verified masternode. Move the policy from PushInv (called for every inv type) to the three sites that actually relay MSG_ISDLOCK and have CNode in scope.

chatgpt-codex-connector · 2026-04-27T16:34:51Z

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Repo admins can enable using credits for code reviews in their settings.

github-actions · 2026-04-27T16:35:25Z

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

thepastaclaw · 2026-04-27T16:36:05Z

✅ Review complete (commit f6bb02d)

coderabbitai · 2026-04-27T16:37:58Z

Walkthrough

The changes refactor inventory announcement filtering logic in net processing by introducing a new helper function PeerReconstructsISLockFromRecsig that checks if a peer both wants recsigs and is a verified masternode. Previously, PushInv suppressed MSG_ISDLOCK inventory announcements for all peers requesting recsigs. This blanket suppression is now removed and replaced with narrower conditional checks in RelayInvFiltered and SendMessages that use the new helper, alongside logging and control flow updates.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% 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
Description check	✅ Passed	The description is directly related to the changeset, explaining the issue fixed (overly broad suppression in PR `#6994`), the solution (narrower suppression only for verified masternodes), testing performed, and checklist items.
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.
Title check	✅ Passed	The title clearly summarizes the main fix: keeping ISDLOCK inventory announcements sent to non-masternode peers that want recsigs, directly addressing the core issue resolved in the PR.

_{✏️ 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

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

thepastaclaw

Code Review

Verified the targeted fix on net_processing.cpp: a new helper PeerReconstructsISLockFromRecsig narrows the ISDLOCK inv suppression to verified masternodes that opted into recsigs, restoring relay to non-MN watchquorums peers. No correctness regressions found. Two suggestions and one nitpick stand: a small heuristic gap, duplicated skip-and-log blocks across the two RelayInvFiltered overloads, and absence of a dedicated regression test for the watchquorums + recsigs scenario.

Reviewed commit: f6bb02d

🟡 2 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/net_processing.cpp`:
- [SUGGESTION] lines 1201-1203: Add a direct regression test for ISDLOCK relay to watchquorums peers
  The fix relies entirely on PeerReconstructsISLockFromRecsig to keep ISDLOCK invs flowing to non-masternode peers that opted into recsigs (e.g. nodes started with -watchquorums). The PR changes only src/net_processing.cpp and adds no test that pins this behavior down. The original regression in PR #6994 was masked because interface_dash_zmq.py covers the failure mode only indirectly (and was flaky enough to miss it ~50% of the time). A dedicated functional or unit assertion that a non-MN peer that has sent QSENDRECSIGS still receives MSG_ISDLOCK after all connected MNs have set m_wants_recsigs=true would prevent the next refactor from collapsing the three sites back into a PushInv-level filter and reintroducing the same bug.
- [SUGGESTION] lines 1196-1204: Helper assumes every verified-MN peer can reconstruct an ISDLOCK
  PeerReconstructsISLockFromRecsig treats `m_wants_recsigs && verified-MN` as sufficient to assume the peer will reconstruct an ISDLOCK locally from the recsig. That holds for masternodes that are members of the relevant InstantSend quorum running the signer worker, but it is not strictly enforced: a verified MN that is not in the active quorum (or is outside the recovery window) will still receive recsigs but lacks the share data needed to assemble the ISDLOCK, and under this code we will not send it the inv either. The behavior is still strictly safer than the pre-fix state and other MN peers will fill the gap, but the comment overstates the invariant. Tightening the comment to call this out as a best-effort optimization (or confirming the IS team is comfortable with the edge case) would prevent future readers from over-trusting the predicate.

thepastaclaw · 2026-04-28T08:18:54Z

+// Returns true when peer is a verified masternode that has opted in to receive recsigs.
+// Such peers participate in the signing flow that populates creatingInstantSendLocks, so
+// they can reconstruct an ISDLOCK locally from the recsig and don't need the ISDLOCK inv.
+// Non-MN peers (e.g. nodes running with -watchquorums) also opt in to recsigs via
+// QSENDRECSIGS but still need ISDLOCK invs because they don't run the signing flow.
+static bool PeerReconstructsISLockFromRecsig(const CNode& pnode, const Peer& peer)
+{
+    return peer.m_wants_recsigs && !pnode.GetVerifiedProRegTxHash().IsNull();
+}


🟡 Suggestion: Helper assumes every verified-MN peer can reconstruct an ISDLOCK

PeerReconstructsISLockFromRecsig treats m_wants_recsigs && verified-MN as sufficient to assume the peer will reconstruct an ISDLOCK locally from the recsig. That holds for masternodes that are members of the relevant InstantSend quorum running the signer worker, but it is not strictly enforced: a verified MN that is not in the active quorum (or is outside the recovery window) will still receive recsigs but lacks the share data needed to assemble the ISDLOCK, and under this code we will not send it the inv either. The behavior is still strictly safer than the pre-fix state and other MN peers will fill the gap, but the comment overstates the invariant. Tightening the comment to call this out as a best-effort optimization (or confirming the IS team is comfortable with the edge case) would prevent future readers from over-trusting the predicate.

_{source: ['claude']}

🤖 Fix this 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/net_processing.cpp`: - [SUGGESTION] lines 1196-1204: Helper assumes every verified-MN peer can reconstruct an ISDLOCK PeerReconstructsISLockFromRecsig treats `m_wants_recsigs && verified-MN` as sufficient to assume the peer will reconstruct an ISDLOCK locally from the recsig. That holds for masternodes that are members of the relevant InstantSend quorum running the signer worker, but it is not strictly enforced: a verified MN that is not in the active quorum (or is outside the recovery window) will still receive recsigs but lacks the share data needed to assemble the ISDLOCK, and under this code we will not send it the inv either. The behavior is still strictly safer than the pre-fix state and other MN peers will fill the gap, but the comment overstates the invariant. Tightening the comment to call this out as a best-effort optimization (or confirming the IS team is comfortable with the edge case) would prevent future readers from over-trusting the predicate.

thepastaclaw · 2026-04-28T08:18:55Z

@@ -2566,6 +2574,11 @@ void PeerManagerImpl::RelayInvFiltered(const CInv& inv, const uint256& relatedTx
                return;
            }
        } // LOCK(tx_relay->m_bloom_filter_mutex)
+        if (inv.type == MSG_ISDLOCK && PeerReconstructsISLockFromRecsig(*pnode, *peer)) {
+            LogPrint(BCLog::NET, "%s -- skipping ISDLOCK inv (peer wants recsigs): %s peer=%d\n",
+                     __func__, inv.ToString(), peer->m_id);
+            return;
+        }


💬 Nitpick: Duplicated ISDLOCK skip-and-log block in both RelayInvFiltered overloads

The four-line if (inv.type == MSG_ISDLOCK && PeerReconstructsISLockFromRecsig(*pnode, *peer)) { LogPrint(...); return; } is copy-pasted into both RelayInvFiltered overloads (2547-2551 and 2577-2581) and is the structural twin of the guard at 6364. Extracting a small helper such as ShouldSkipISDLockInv(pnode, peer, inv) would avoid the three sites drifting apart in future edits. Pure readability/maintainability nit, not a defect.

_{source: ['claude']}

PastaPastaPasta

utACK f6bb02d

knst added this to the 24 milestone Apr 27, 2026

knst added the backport-candidate-23.1.x label Apr 27, 2026

knst mentioned this pull request Apr 27, 2026

fix: reconstruct ISLOCK locally when recsig arrived first #7291

Closed

5 tasks

knst requested review from PastaPastaPasta and UdjinM6 April 27, 2026 18:20

knst changed the title ~~fix: keep sending ISDLOCK invs to non-MN peers that want recsigs~~ fix: keep sending ISDLOCK invs to non-MN peers with watchquorums Apr 27, 2026

thepastaclaw reviewed Apr 28, 2026

View reviewed changes

PastaPastaPasta approved these changes Apr 28, 2026

View reviewed changes

PastaPastaPasta merged commit 67683f3 into dashpay:develop Apr 28, 2026
46 of 48 checks passed

knst mentioned this pull request Apr 30, 2026

test: adds QSENDRECSIGS functional tests for watchquorum nodes #7301

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: keep sending ISDLOCK invs to non-MN peers with watchquorums#7293

fix: keep sending ISDLOCK invs to non-MN peers with watchquorums#7293
PastaPastaPasta merged 1 commit intodashpay:developfrom
knst:fix-6994-watch

knst commented Apr 27, 2026

Uh oh!

chatgpt-codex-connector Bot commented Apr 27, 2026

Uh oh!

github-actions Bot commented Apr 27, 2026

Uh oh!

thepastaclaw commented Apr 27, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Apr 27, 2026 •

edited

Loading

❌ Failed checks (1 warning)

Uh oh!

thepastaclaw left a comment

Uh oh!

Uh oh!

thepastaclaw Apr 28, 2026

Uh oh!

thepastaclaw Apr 28, 2026

Uh oh!

PastaPastaPasta left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

knst commented Apr 27, 2026

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

chatgpt-codex-connector Bot commented Apr 27, 2026

Uh oh!

github-actions Bot commented Apr 27, 2026

✅ No Merge Conflicts Detected

Uh oh!

thepastaclaw commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Estimated code review effort

❌ Failed checks (1 warning)

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

thepastaclaw Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

thepastaclaw Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thepastaclaw commented Apr 27, 2026 •

edited

Loading

coderabbitai Bot commented Apr 27, 2026 •

edited

Loading