fix(net): clean recovered sig backlog after peer disconnect - #7483
Draft
thepastaclaw wants to merge 1 commit into
Draft
fix(net): clean recovered sig backlog after peer disconnect#7483thepastaclaw wants to merge 1 commit into
thepastaclaw wants to merge 1 commit into
Conversation
…nned
NetSigning::WorkThreadSigning sweeps CSigningManager's pending (not-yet-
verified) recovered sigs every CLEANUP_INTERVAL{5s} so a flood's backlog does
not outlive the peer that sent it. That sweep was keyed on PeerIsBanned(),
which made it effectively a no-op.
PeerIsBanned() -> IsBanned() reads Peer::m_should_discourage, a one-shot flag
set by Misbehaving() and cleared by MaybeDiscourageAndDisconnect() on the very
next SendMessages pass (~100ms) just before the peer is disconnected. Once the
peer is finalized the Peer is erased from m_peer_map, so GetPeerRef() returns
nullptr and IsBanned() reports false again. The predicate could therefore only
match if a 5s tick happened to land inside a ~100ms window, so in practice the
backlog survived and the stated goal was never achieved.
Key the sweep on "no longer connected" instead. Disconnection is a durable,
terminal state -- node ids are never reused (CConnman::GetNewNodeId only
increments) -- so whenever the tick lands, it is observed. This also reclaims
the queues of peers that simply went away without misbehaving, and mirrors how
CSigSharesManager::Cleanup() already prunes its per-node state by disconnection.
A connected-state predicate was chosen over a FinalizeNode hook to keep the
change minimal: it preserves the existing throttled-sweep shape, adds no
cross-subsystem lifetime or ordering coupling, and is directly unit-testable.
The new PeerIsConnected() also needs a strictly smaller lock footprint than
PeerIsBanned() (m_peer_mutex only, no cs_main or m_misbehavior_mutex).
NetSigning::RemoveBannedNodeStates() is deliberately left on PeerIsBanned():
it runs from WorkThreadCleaning() on a 100ms loop that matches the flag's
lifetime, and its subsystem has its own disconnected-node sweep.
Locally reconstructed sigs are unaffected: they use NodeId -1 and live in
pendingReconstructedRecoveredSigs, a separate map keyed by sig hash that
RemoveNodesIf() does not touch.
Adds a regression test asserting the flag has already gone false by the time a
sweep could run, while the connection-keyed predicate stays true once the peer
is finalized, plus direct coverage of PeerIsConnected() across FinalizeNode().
Author
|
✅ Final review complete — no blockers (commit ea71732) |
thepastaclaw
commented
Jul 25, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
Author
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The change correctly replaces the transient discouragement flag with durable peer-map membership when pruning pending recovered signatures. Peer lifecycle, locking, node-ID uniqueness, local reconstruction handling, and the focused regression tests all support the implementation; no in-scope correctness issues were found. GitHub does not permit self-approval, so the canonical APPROVE result is transported as a COMMENT review.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (completed)
thepastaclaw
changed the title
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
1 participant
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
The recovered-signature cleanup added in #7402 checks
PeerIsBanned(node_id)every five seconds. That query reads the peer's one-shotm_should_discourageflag, which is cleared during the nextSendMessagespass before disconnection completes. A cleanup tick therefore usually misses the short-lived state, and onceFinalizeNode()removes the peer,PeerIsBanned()returns false. Pending recovered signatures from the peer can consequently remain queued instead of being reclaimed after disconnection.What was done?
PeerManagerInternal::PeerIsConnected()using the peer map as the authoritative connection-lifecycle state.PeerIsConnected()lifecycle acrossFinalizeNode().The existing
RemoveBannedNodeStates()path for signature shares is intentionally unchanged because it runs in the fast message-processing loop while the discourage flag is observable.How Has This Been Tested?
./src/test/test_dash --run_test=denialofservice_tests/peer_gone_recovered_sig_cleanup_predicate./src/test/test_dash --run_test=net_peer_connection_tests/peer_is_connected_tracks_finalize_node./src/test/test_dash --run_test='!validation_chainstate_tests'(762 test cases)git diff --check 6d04c60ef36996f2d13662f806790de9fe732836..ea717326c77191cb1a1644a09a2cd39d60fed223shipThe predicate regression test was also verified to fail with the old
PeerIsBanned()condition and pass with the new disconnected-peer condition.Breaking Changes
None.
Checklist