fix(llmq): purge pending recovered sigs in BanNode - #7563
Conversation
|
⛔ Blockers found — Opus deferred (commit bb11a28) |
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 CSigningManager
participant ShareManager
Peer->>CSigningManager: Send invalid recovered-signature message
CSigningManager->>CSigningManager: Call BanNode(peer)
CSigningManager->>CSigningManager: Remove pending unverified signatures
CSigningManager->>ShareManager: Update share-manager ban state
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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.
| 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.
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