fix: purge pending recovered sigs in BanNode - #7563
Conversation
|
✅ Final review complete — no blockers (commit 249d088) |
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 👍 / 👎.
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 |
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 entirely: that predicate was a ~100ms one-shot and never reliably ran. Caps from dashpay#7402 remain the bound for peers that disconnect without misbehavior. Keep m_sig_manager.Cleanup() for DB age.
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
The eager purge in NetSigning::BanNode is racy: BanNode can run on a recsig/sig-share verification worker thread while the message handler is concurrently processing another QSIGREC from the same peer, so a fresh pending entry can be queued right after the purge and still burn BLS verification after the ban. FinalizeNode runs strictly after the peer's last ProcessMessages call (DeleteNode requires the node refcount to reach zero) and node ids are never reused, so purging there is final. It also reclaims the pending backlog of peers that disconnect without misbehaving instead of waiting for the drain loop to chew through it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ailures Routing the two raw recsig score-100 sites (invalid QSIGREC llmqType, bad BLS batch source) through BanNode also started calling CSigSharesManager::MarkAsBanned, which those sites never did before. For NoBan/manual peers MaybeDiscourageAndDisconnect clears the discourage flag but keeps the connection, while the sticky nodeState.banned flag suppresses all subsequent sig-share traffic from the peer (TryAddPendingIncomingSigShare, CollectSigSharesToRequest/Send/Announce) for the connection lifetime. Keep the misbehavior score and the pending-recsig purge on those paths, but reserve MarkAsBanned for sig-share protocol failures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![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.
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