fix: skip ISDLOCK inv announcements for peers requesting recsigs#6994
Merged
PastaPastaPasta merged 1 commit intodashpay:developfrom Nov 21, 2025
Merged
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
WalkthroughThe changes modify inventory propagation logic in the peer-to-peer message handling to conditionally skip ISDLOCK announcements for peers that request recovery signatures (recsigs). In Sequence Diagram(s)sequenceDiagram
participant Node
participant PushInv
participant SendMessages
participant Peer
Node->>SendMessages: Process outgoing inventory
SendMessages->>SendMessages: Check: Does peer want recsigs?
alt Peer wants recsigs
SendMessages->>SendMessages: Skip ISDLOCK inv
Note over SendMessages: Guard prevents enqueue
else Peer does not want recsigs
SendMessages->>PushInv: Add to inventory queue
PushInv->>Peer: Send ISDLOCK announcement
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-09-09T21:36:11.833ZApplied to files:
🧬 Code graph analysis (1)src/net_processing.cpp (3)
⏰ 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)
🔇 Additional comments (2)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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. Comment |
UdjinM6
approved these changes
Nov 21, 2025
kwvg
approved these changes
Nov 21, 2025
knst
added a commit
to knst/dash
that referenced
this pull request
Apr 26, 2026
PR dashpay#6994 stopped sending ISDLOCK invs to peers requesting recsigs on the premise that they can reconstruct the islock from the recsig. That reconstruction has a pre-existing race that become critical now. It causes intermittent failure in interface_zmq_dash.py - wait_for_instantlock timing out. The previous reconstruction path checked HasRecoveredSigForId before emplacing into creatingInstantSendLocks, which leaves a window: thread A: ProcessRecoveredSig writes recsig to db, listener fires HandleNewRecoveredSig, finds creating empty, no-ops thread B: TrySignInstantSendLock saw HasRecoveredSigForId=false, now emplaces and calls AsyncSignIfMember It causes recsig to be in in db but no one ever builds the islock: - db.HasRecoveredSigForHash blocks any subsequent listener firing for that recsig - TrySignInstantSendLock has already passed its check Before dashpay#6994 this was masked because the peer would still receive the ISDLOCK inv from another node, but on develop the peer no longer receives an ISDLOCK inv from peers as a fallback. Reorder to emplace first, then check: 1. Emplace into creatingInstantSendLocks. Any listener that fires after this point sees the entry and reconstructs via the existing HandleNewInstantSendLockRecoveredSig path. 2. Then check HasRecoveredSigForId. If already in db, find+erase the entry; if find misses, the listener already handled it. 3. Only call AsyncSignIfMember when no recsig exists yet.
This was referenced Apr 26, 2026
PastaPastaPasta
pushed a commit
that referenced
this pull request
Apr 28, 2026
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. 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.
PastaPastaPasta
added a commit
that referenced
this pull request
Apr 28, 2026
…hquorums f6bb02d fix: keep sending ISDLOCK invs to non-MN peers that want recsigs (Konstantin Akimov) Pull request description: ## 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: - [x] 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 - [x] I have assigned this pull request to a milestone ACKs for top commit: PastaPastaPasta: utACK f6bb02d Tree-SHA512: 14be2cfaad6dd0cb359bedca9e65e176b3d52de109f1c9e606892ee284f8079860d01117d538efb19bcf695e5da0cae747f08dcb98c209624ca28d6cf7d1e34d
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Masternode to masternode connections which share recovered signatures, should not share isdlocks; as these are higher level messages produced for users /consumers, which these masternodes can reconstruct from the recsig. To avoid wasted bandwidth and processing, we should skip sending isdlock invs to peers that we send recSigs to.
What was done?
Don't send inv for isdlocks for peers who request recsigs.
How Has This Been Tested?
Breaking Changes
None
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.