fix: purge pending recovered sigs in BanNode - #7563
Conversation
|
✅ Final review complete — no blockers (commit 2aac78f) |
2be660a to
38d9364
Compare
PastaPastaPasta
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64194d7716
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| if (!Params().GetLLMQ(recoveredSig->getLlmqType()).has_value()) { | ||
| m_peer_manager->PeerMisbehaving(pfrom.GetId(), 100); | ||
| BanNode(pfrom.GetId()); |
There was a problem hiding this comment.
When a peer with NoBan permission and an existing sig-share node state sends this malformed QSIGREC, BanNode() now also calls MarkAsBanned(). If SendMessages() clears the transient m_should_discourage flag before the cleaning thread observes it, the peer remains connected while its node state stays permanently marked banned, causing TryAddPendingIncomingSigShare() to discard all later valid QSIGSHAREs from that trusted connection. Use a recovered-sig-only purge here rather than the broader sig-share ban path.
AGENTS.md reference: AGENTS.md:L169-L169
Useful? React with 👍 / 👎.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughInvalid LLMQ types and invalid recovered signatures now use Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Peer
participant NetSigning
participant CSigningManager
participant ShareManager
Peer->>NetSigning: Send invalid recovered-signature message
NetSigning->>NetSigning: Call BanNode(peer)
NetSigning->>CSigningManager: RemoveNode(peer)
CSigningManager-->>NetSigning: Remove pending unverified signatures
NetSigning->>ShareManager: Conditionally mark shares banned
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
38d9364 to
bb11a28
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The eager purge is not durable: concurrent QSIGREC admission can repopulate the peer's queue after RemoveNode(), leaving the single recovered-signature worker to verify the residual backlog after disconnection. The newly routed recovered-signature failures also mark the separate sig-share state as banned, which can permanently disable valid sig-share traffic on NoBan and manual connections.
Source: reviewer backends: gpt-5.6-sol (general) and gpt-5.6-sol (dash-core-commit-history); final verifier backend: gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
🤖 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/net_signing.cpp`:
- [BLOCKING] src/llmq/net_signing.cpp:312-314: Prevent recovered signatures from being requeued after the eager purge
RemoveNode() only erases entries present while it holds cs_pending; it does not prevent a later VerifyAndProcessRecoveredSig() call for the same NodeId from inserting another entry. ProcessPendingRecoveredSigs() invokes BanNode() on the recovered-signature worker while the message handler can already be processing another QSIGREC. If removal wins the lock first, that in-flight message queues a residual signature afterward. Masternode connections can admit additional messages because their SendMessages() pass—and therefore MaybeDiscourageAndDisconnect()—is limited to the 100 ms cadence in CConnman::ThreadMessageHandler(). With the periodic sweep removed, these residual entries must be drained and BLS-verified after the peer is disconnected, defeating the stated cleanup guarantee. Fence further admission for the NodeId or perform another purge at a durable peer-disconnection/finalization point.
- [BLOCKING] src/llmq/net_signing.cpp:56-57: Do not permanently disable sig shares for a recovered-signature failure
Routing this recovered-signature-only failure through BanNode() now calls CSigSharesManager::MarkAsBanned(), as does the bad recovered-signature batch path at line 267. If the peer has an existing sig-share node state, MarkAsBanned() sets its banned flag. For NoBan and manual connections, MaybeDiscourageAndDisconnect() clears m_should_discourage but intentionally keeps the connection alive. If that happens before RemoveBannedNodeStates() observes the transient flag, the node state is never removed and remains banned for the connection's lifetime. TryAddPendingIncomingSigShare(), CollectSigSharesToRequest(), CollectSigSharesToSend(), and CollectSigSharesToAnnounce() then suppress subsequent valid sig-share traffic. Keep the recovered-signature score and queue purge separate from MarkAsBanned(), reserving the latter for sig-share protocol failures.
| // Drop any not-yet-verified recovered sigs still queued for this peer so a flood's backlog | ||
| // does not keep burning the single recsig worker after we have already decided to ban. | ||
| m_sig_manager.RemoveNode(nodeId); |
There was a problem hiding this comment.
🔴 Blocking: Prevent recovered signatures from being requeued after the eager purge
RemoveNode() only erases entries present while it holds cs_pending; it does not prevent a later VerifyAndProcessRecoveredSig() call for the same NodeId from inserting another entry. ProcessPendingRecoveredSigs() invokes BanNode() on the recovered-signature worker while the message handler can already be processing another QSIGREC. If removal wins the lock first, that in-flight message queues a residual signature afterward. Masternode connections can admit additional messages because their SendMessages() pass—and therefore MaybeDiscourageAndDisconnect()—is limited to the 100 ms cadence in CConnman::ThreadMessageHandler(). With the periodic sweep removed, these residual entries must be drained and BLS-verified after the peer is disconnected, defeating the stated cleanup guarantee. Fence further admission for the NodeId or perform another purge at a durable peer-disconnection/finalization point.
source: ['codex']
There was a problem hiding this comment.
1. Requeue after RemoveNode — real race, not treating as blocking
Agreed that RemoveNode is a one-shot erase under cs_pending and does not fence later VerifyAndProcessRecoveredSig inserts for the same NodeId. Concurrent msg-thread admission after a worker-path ban can re-queue residual entries.
Why I’m not expanding the PR for it:
- Memory stays bounded by the fix: bound pending recovered sig queue to prevent remote OOM #7402 caps (
MAX_PENDING_RECSIGS_PER_NODE/TOTAL). - For normal peers the window is short: score → next
SendMessages/MaybeDiscourageAndDisconnect→ disconnect → no moreProcessMessages. - The old 5s
PeerIsBannedsweep was not a durable fence either (m_should_discourageis ~100ms and false again after finalize), so restoring a periodic sweep would not honestly close this. - An admission blacklist in
CSigningManagerwould work but duplicates peer lifecycle state inside LLMQ; a proper fence belongs on disconnect/FinalizeNode(or similar) if we want that later, not as a second ban bit next to the queue.
So the PR guarantee is intentional: drop what is already queued at ban time, eagerly. Residual post-ban admission is accepted residual under BanNode-only + caps, not a regression of unbounded backlog.
There was a problem hiding this comment.
Resolved in 249d088 — Prevent recovered signatures from being requeued after the eager purge no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| if (!Params().GetLLMQ(recoveredSig->getLlmqType()).has_value()) { | ||
| m_peer_manager->PeerMisbehaving(pfrom.GetId(), 100); | ||
| BanNode(pfrom.GetId()); |
There was a problem hiding this comment.
🔴 Blocking: Do not permanently disable sig shares for a recovered-signature failure
Routing this recovered-signature-only failure through BanNode() now calls CSigSharesManager::MarkAsBanned(), as does the bad recovered-signature batch path at line 267. If the peer has an existing sig-share node state, MarkAsBanned() sets its banned flag. For NoBan and manual connections, MaybeDiscourageAndDisconnect() clears m_should_discourage but intentionally keeps the connection alive. If that happens before RemoveBannedNodeStates() observes the transient flag, the node state is never removed and remains banned for the connection's lifetime. TryAddPendingIncomingSigShare(), CollectSigSharesToRequest(), CollectSigSharesToSend(), and CollectSigSharesToAnnounce() then suppress subsequent valid sig-share traffic. Keep the recovered-signature score and queue purge separate from MarkAsBanned(), reserving the latter for sig-share protocol failures.
source: ['codex']
There was a problem hiding this comment.
2. Recsig failure → MarkAsBanned — declining for this PR
Yes, routing recsig score-100 paths through full BanNode now also runs MarkAsBanned, which can sticky-mute share traffic if the peer stays connected (NoBan/manual keep the connection after clearing m_should_discourage, so RemoveBannedNodeStates may never erase the node state).
I’m not splitting that out here:
- NoBan/manual punishment edge cases are out of scope for this change.
- For ordinary peers we disconnect on discourage; share
nodeStatescleanup then follows existing disconnect reclaim inCSigSharesManager::Cleanup. - Pre-existing share-path failures already used full
BanNode(misbehave +MarkAsBanned). Treating invalid recovered sigs the same is deliberate: one NetSigning score-100 choke point.
If we later want recsig-only score without muting shares, that can be a small follow-up that misbehaves + RemoveNode without MarkAsBanned on those two sites only.
Happy to revisit either point if we want a follow-up for disconnect-time reclaim or a recsig/share ban split.
There was a problem hiding this comment.
Resolved in 249d088 — Do not permanently disable sig shares for a recovered-signature failure no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
PastaPastaPasta
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/llmq/signing.cpp (1)
421-429: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftAdd a targeted test for the cleanup invariant.
Test that
RemoveNoderemoves only the selected peer’s queue, decrementspendingRecoveredSigsCountby the exact queue size, preserves another peer’s queue andpendingReconstructedRecoveredSigs, and remains safe for missing or repeated node IDs.As per coding guidelines, “Choose and add targeted C++ unit tests for changed behavior, preferably in existing test files unless a new file is clearly justified.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/llmq/signing.cpp` around lines 421 - 429, Add a focused C++ unit test for CSigningManager::RemoveNode covering removal of one peer’s queue, exact pendingRecoveredSigsCount decrement, preservation of another peer’s queue and pendingReconstructedRecoveredSigs, and safe no-op behavior for missing and repeated node IDs. Place it in the existing signing-manager test suite and use the manager’s observable state or established test accessors.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/llmq/signing.cpp`:
- Around line 421-429: Add a focused C++ unit test for
CSigningManager::RemoveNode covering removal of one peer’s queue, exact
pendingRecoveredSigsCount decrement, preservation of another peer’s queue and
pendingReconstructedRecoveredSigs, and safe no-op behavior for missing and
repeated node IDs. Place it in the existing signing-manager test suite and use
the manager’s observable state or established test accessors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e000918e-90c5-45b2-a1e0-79311838dd8d
📒 Files selected for processing (5)
src/llmq/net_signing.cppsrc/llmq/net_signing.hsrc/llmq/signing.cppsrc/llmq/signing.hsrc/net_processing.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/llmq/net_signing.cpp
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The current head resolves both previously verified blockers: finalization now performs a durable recovered-signature purge after message processing has ceased, and recovered-signature-only failures no longer mark the separate sig-share state as banned. No remaining correctness defects were found, but the two corrective follow-up commits should be squashed into the original change so the branch does not preserve known-bad intermediate states.
Source: reviewer backends: gpt-5.6-sol (general) and gpt-5.6-sol (dash-core-commit-history); final verifier backend: gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 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 `<commit:21fa89b>`:
- [SUGGESTION] <commit:21fa89b>:1: Squash the corrective follow-ups into the original purge commit
Commits 21fa89b029e and 249d088c8ec explicitly repair defects introduced by bb11a289290: the first closes the post-purge requeue race, while the second prevents recovered-signature failures from incorrectly applying the sticky sig-share ban. Keeping the three commits separately leaves known-bad intermediate states in history and matches the fixup-commit pattern that CONTRIBUTING.md asks authors to squash. Fold both follow-ups into bb11a289290 and update the resulting commit message to describe the finalization purge and recovered-signature-only ban handling.
249d088 to
6200969
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6200969edb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // eagerly, but a QSIGREC processed concurrently with a ban issued from a verification | ||
| // worker thread can re-queue entries behind the purge. This runs strictly after the | ||
| // peer's last ProcessMessages call and node ids are never reused, so it is final. | ||
| m_llmq_ctx->sigman->RemoveNode(nodeid); |
There was a problem hiding this comment.
When an ordinary peer sends a valid QSIGREC and disconnects before the signing worker drains its queue (the worker can sleep for 100 ms), FinalizeNode() unconditionally erases that peer's pending signatures even though no ban occurred. The recovered signature is then never verified or processed, potentially delaying ChainLock or InstantSend propagation until another peer relays it; restrict this final purge to peers actually handled by the ban path rather than every disconnect.
AGENTS.md reference: AGENTS.md:L169-L170
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The ban-time purge and recovered-signature-only handling fix the original stale-backlog and sticky sig-share problems, but the finalization fallback now also drops valid pending recovered signatures from ordinary disconnects. Restrict finalization-time cleanup to peers that crossed the discouragement threshold, and squash the two corrective follow-ups into the original commit so each commit remains hygienic.
Source: reviewer backend model gpt-5.6-sol; final verifier backend model gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 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`:
- [BLOCKING] src/net_processing.cpp:1791-1797: Do not discard valid recovered signatures on ordinary disconnects
`FinalizeNode()` runs for every disconnected peer, so this unconditional purge is not limited to peers handled by `NetSigning::BanNode()`. A peer can send a valid `QSIGREC` and disconnect before the signing worker drains its bounded queue; this removes the signature without verification or persistence. The receive path has already completed that peer's object announcement in `NetSigning::ProcessMessage()`, and proactive delivery may have no tracked announcement at all, so a sole-source signature is unavailable until another peer happens to announce or relay it. This can delay ChainLock or InstantSend processing. Preserve the durable second purge for peers that crossed the discouragement threshold, while allowing ordinary disconnected peers' queues to drain as they did before this PR.
In `<commit:36692867033>`:
- [SUGGESTION] <commit:36692867033>:1: Squash the corrective follow-ups into the original purge commit
Commits 36692867033 and 6200969edb5 explicitly repair defects introduced by 65c6cb375d9: the first adds finalization-time cleanup to close the post-purge requeue race, while the second prevents recovered-signature-only failures from applying the sticky sig-share ban. Keeping all three separately preserves known-bad intermediate states. CONTRIBUTING.md requires individual commits to remain free of regressions and specifically identifies repeated corrective changes as fixup commits that may need squashing. Fold both follow-ups into 65c6cb375d9 and update the resulting commit message to describe the finalization cleanup and recovered-signature-only ban handling.
| if (m_llmq_ctx) { | ||
| // Drop this peer's not-yet-verified pending recovered sigs. NetSigning::BanNode purges | ||
| // eagerly, but a QSIGREC processed concurrently with a ban issued from a verification | ||
| // worker thread can re-queue entries behind the purge. This runs strictly after the | ||
| // peer's last ProcessMessages call and node ids are never reused, so it is final. | ||
| m_llmq_ctx->sigman->RemoveNode(nodeid); | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Do not discard valid recovered signatures on ordinary disconnects
FinalizeNode() runs for every disconnected peer, so this unconditional purge is not limited to peers handled by NetSigning::BanNode(). A peer can send a valid QSIGREC and disconnect before the signing worker drains its bounded queue; this removes the signature without verification or persistence. The receive path has already completed that peer's object announcement in NetSigning::ProcessMessage(), and proactive delivery may have no tracked announcement at all, so a sole-source signature is unavailable until another peer happens to announce or relay it. This can delay ChainLock or InstantSend processing. Preserve the durable second purge for peers that crossed the discouragement threshold, while allowing ordinary disconnected peers' queues to drain as they did before this PR.
| if (m_llmq_ctx) { | |
| // Drop this peer's not-yet-verified pending recovered sigs. NetSigning::BanNode purges | |
| // eagerly, but a QSIGREC processed concurrently with a ban issued from a verification | |
| // worker thread can re-queue entries behind the purge. This runs strictly after the | |
| // peer's last ProcessMessages call and node ids are never reused, so it is final. | |
| m_llmq_ctx->sigman->RemoveNode(nodeid); | |
| } | |
| if (m_llmq_ctx && misbehavior >= DISCOURAGEMENT_THRESHOLD) { | |
| // Drop this banned peer's not-yet-verified pending recovered sigs. NetSigning::BanNode | |
| // purges eagerly, but a QSIGREC processed concurrently with a ban issued from a | |
| // verification worker thread can re-queue entries behind the purge. This runs strictly | |
| // after the peer's last ProcessMessages call and node ids are never reused, so it is final. | |
| m_llmq_ctx->sigman->RemoveNode(nodeid); | |
| } |
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Do not discard valid recovered signatures on ordinary disconnects no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
Widen BanNode to drop CSigningManager::pendingRecoveredSigs for the banned node, and route the two raw recsig PeerMisbehaving(100) sites (invalid QSIGREC llmqType, bad BLS batch source) through BanNode so every NetSigning score-100 path purges eagerly like shares already do via MarkAsBanned. Replace the unused-outside-BanNode RemoveNodesIf predicate API with a direct RemoveNode(NodeId), and remove the 5s PeerIsBanned pending-recsig sweep: that predicate was a ~100ms one-shot and never reliably ran. Caps from dashpay#7402 remain the bound for peers that disconnect without misbehavior. The eager purge alone is racy: BanNode can run on a verification worker thread while the message handler is still processing a QSIGREC from the same peer, re-queueing an entry behind the purge. Back it up in FinalizeNode, which runs strictly after the peer's last ProcessMessages call (DeleteNode requires the node refcount to reach zero) and node ids are never reused, so that purge is final. It is gated on misbehavior >= DISCOURAGEMENT_THRESHOLD - BanNode always scores exactly 100 - so peers that disconnect without misbehaving keep their queue and drain as before; their pending sigs may be the only copy we ever receive. The recovered-sig-only failure paths pass mark_shares_banned=false so they do not set the sticky CSigSharesManager nodeState.banned flag: NoBan/manual peers survive the misbehavior score with the connection intact, and marking them would mute their sig-share traffic for the connection lifetime. MarkAsBanned stays reserved for sig-share protocol failures, as before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
6200969 to
2aac78f
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The current squashed commit correctly purges pending recovered signatures at NetSigning ban sites, avoids applying the sticky sig-share ban for recovered-signature-only failures, and limits finalization cleanup to peers that reached the discouragement threshold. Both prior findings are fixed, and no remaining in-scope correctness issues were found.
Source: reviewer backend model gpt-5.6-sol; final verifier backend model gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
2 participants
Issue being fixed or feature implemented
PR #7402 bounded
CSigningManager::pendingRecoveredSigsand added a 5s cleanup that drops pending recovered sigs for peers matchingPeerIsBanned(). That predicate is effectively a no-op:m_should_discourageis a ~100ms one-shot cleared on the nextSendMessagespass, and afterFinalizeNodethe peer is gone soPeerIsBannedis false again. Flood backlog from a banned peer can therefore keep burning the single recsig worker even after we have already decided to ban.Sig-shares already purge eagerly in
BanNode→MarkAsBanned. Recovered-sig ban sites were inconsistent: invalidQSIGRECllmqType and bad BLS batch sources used rawPeerMisbehaving(100)without dropping the pending queue, andBanNodeitself never touchedpendingRecoveredSigs.This supersedes #7483. Rather than keying a periodic sweep on banned/connected state, reclaim is eager on the ban choke point.
What was done?
NetSigning::BanNodeto drop that node'spendingRecoveredSigs.RemoveNodesIfpredicate API with a directCSigningManager::RemoveNode(NodeId).PeerMisbehaving(100)sites (invalidQSIGRECllmqType; bad BLS after batch verify) throughBanNodeso every NetSigning score-100 path purges eagerly.PeerIsBannedpending-recsig sweep entirely. Keptm_sig_manager.Cleanup()for DB age.Caps from #7402 remain the bound for peers that disconnect without misbehavior. Shares'
RemoveBannedNodeStates()(100msPeerIsBannedpoll) is intentionally unchanged. Silent over-cap drops inVerifyAndProcessRecoveredSigremain silent. Local reconstruction (nodeId == -1) is still skipped byBanNodeand is not touched byRemoveNode.How Has This Been Tested?
git diff --checkBanNodeBreaking Changes
None.
Checklist