fix(net): clean recovered sig backlog after peer disconnect - #7483
fix(net): clean recovered sig backlog after peer disconnect#7483thepastaclaw wants to merge 2 commits into
Conversation
|
🔍 Review in progress — actively reviewing now (commit 6a2c939) |
thepastaclaw
left a comment
thepastaclaw
left a comment
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
…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().
ea71732 to
43cefa6
Compare
Remove standalone tests that copied cleanup predicates without exercising the production sweep. Reuse the existing peer discouragement flow to cover the transient ban flag and finalized peer lifecycle, and keep the production rationale concise. Co-Authored-By: Claude <noreply@anthropic.com>
|
Superseded by #7563, which keeps the Closing this PR in favor of that remake. 🤖 Posted autonomously by Claude on behalf of pasta. |
|
The Both the original failing run and the run at the final head pass all Qt test cases, then LeakSanitizer reports the same Qt dependency shutdown leak in The PR diff does not touch Qt or DBus, and the substantive PR was already superseded by #7563. I have left this branch untouched; maintainers can address the shared CI issue via #7561. |
2 participants
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.FinalizeNode().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?
make -C src test/test_dash./src/test/test_dash --run_test=denialofservice_tests/peer_discouragement./src/test/test_dash --run_test=denialofservice_tests./src/test/test_dash --run_test=net_peer_connection_teststest/lint/lint-whitespace.pygit diff --checkclang-format-diff.pyon the PR diffThe two original standalone tests were removed after verifying that both still passed when the production cleanup predicate was reverted. The retained lifecycle assertions fail when
PeerIsConnected()is replaced with the old transient banned-state behavior.Breaking Changes
None.
Checklist