fix: bound pending recovered sig queue to prevent remote OOM - #7402
Merged
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
✅ Review complete (commit 5c10539) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR adds backpressure limits to the pending recovered-signature queue in Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant WorkThreadSigning
participant PeerManager
participant CSigningManager
loop periodic cleanup
WorkThreadSigning->>CSigningManager: Cleanup()
WorkThreadSigning->>PeerManager: PeerIsBanned(node_id)
WorkThreadSigning->>CSigningManager: RemoveNodesIf(banned predicate)
end
CSigningManager->>CSigningManager: VerifyAndProcessRecoveredSig()
alt caps exceeded
CSigningManager-->>CSigningManager: log and drop recovered sig
else within caps
CSigningManager->>CSigningManager: enqueue recovered sig
end
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/llmq/signing.cpp (2)
462-472: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame
std::erase_ifopportunity here.Mirrors the loop in
RemoveNodesIf; can be simplified the same way.As per coding guidelines, "Dash Core implementation must be written in C++20, requiring at least Clang 16 or GCC 11.1" (`src/**/*.{cpp,h,hpp,cc}`).♻️ Proposed refactor
- for (auto it = pendingRecoveredSigs.begin(); it != pendingRecoveredSigs.end();) { - if (it->second.empty()) { - it = pendingRecoveredSigs.erase(it); - } else { - ++it; - } - } + std::erase_if(pendingRecoveredSigs, [](const auto& entry) { return entry.second.empty(); });🤖 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 462 - 472, The manual erase loop over pendingRecoveredSigs should be refactored to use std::erase_if, matching the simpler pattern used in RemoveNodesIf. Update the cleanup logic in signing.cpp so the pruning of empty entries is expressed with std::erase_if on pendingRecoveredSigs, keeping the same behavior while reducing boilerplate and aligning with the C++20 style guidelines.Source: Coding guidelines
419-429: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
std::erase_iffor this cleanup. Replace the manual iterator loop withstd::erase_if(pendingRecoveredSigs, ...)to keep the code shorter and less error-prone.🤖 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 419 - 429, Replace the manual iterator cleanup in CSigningManager::RemoveNodesIf with std::erase_if on pendingRecoveredSigs, using the existing predicate against the map key. Keep the cs_pending lock in place and preserve the same removal behavior while simplifying the implementation and avoiding the explicit erase/advance loop.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.
Inline comments:
In `@src/llmq/dkgsessionhandler.h`:
- Around line 80-89: In CDKGPendingMessages, the duplicate check in the message
handling flow should happen before consuming the per-node quota. Update the
logic around messagesPerNode[from] and seenMessages.emplace(hash) so duplicate
hashes are dropped first, and only increment messagesPerNode[from] after a
message is confirmed new and will be queued. Keep the existing behavior in the
same method, but reorder the checks to avoid counting retransmits against the
peer.
In `@src/llmq/net_dkg.cpp`:
- Around line 108-113: The deserialization path in the DKG message handling
currently accepts payloads even when unread trailing bytes remain after vRecv >>
*msg, which can cause duplicate suppression to miss semantically identical
messages. Update the receive/parse logic in the DKG message handler that calls
vRecv >> *msg and then ValidateDKGMessageStructure so it verifies the stream was
fully consumed after deserialization, and reject the message by returning
nullptr if any bytes remain or parsing leaves extra data.
---
Nitpick comments:
In `@src/llmq/signing.cpp`:
- Around line 462-472: The manual erase loop over pendingRecoveredSigs should be
refactored to use std::erase_if, matching the simpler pattern used in
RemoveNodesIf. Update the cleanup logic in signing.cpp so the pruning of empty
entries is expressed with std::erase_if on pendingRecoveredSigs, keeping the
same behavior while reducing boilerplate and aligning with the C++20 style
guidelines.
- Around line 419-429: Replace the manual iterator cleanup in
CSigningManager::RemoveNodesIf with std::erase_if on pendingRecoveredSigs, using
the existing predicate against the map key. Keep the cs_pending lock in place
and preserve the same removal behavior while simplifying the implementation and
avoiding the explicit erase/advance loop.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 191c2890-110d-4b9b-bee7-8d22efff1e0f
📥 Commits
Reviewing files that changed from the base of the PR and between 59d36a9 and b75b568d7794eafdc10e6a0d24ffc564dea1604a.
📒 Files selected for processing (6)
src/llmq/dkgsessionhandler.cppsrc/llmq/dkgsessionhandler.hsrc/llmq/net_dkg.cppsrc/llmq/net_signing.cppsrc/llmq/signing.cppsrc/llmq/signing.h
💤 Files with no reviewable changes (1)
- src/llmq/dkgsessionhandler.cpp
Comment on lines
+80
to
+89
| if (messagesPerNode[from] >= maxMessagesPerNode) { | ||
| // TODO ban? | ||
| LogPrint(BCLog::LLMQ_DKG, "CDKGPendingMessages::%s -- too many messages, peer=%d\n", __func__, from); | ||
| return; | ||
| } | ||
| messagesPerNode[from]++; | ||
|
|
||
| std::vector<std::pair<NodeId, std::shared_ptr<Message>>> ret; | ||
| ret.reserve(binaryMessages.size()); | ||
| for (const auto& bm : binaryMessages) { | ||
| auto msg = std::make_shared<Message>(); | ||
| try { | ||
| *bm.second >> *msg; | ||
| } catch (...) { | ||
| msg = nullptr; | ||
| } | ||
| ret.emplace_back(std::make_pair(bm.first, std::move(msg))); | ||
| if (!seenMessages.emplace(hash).second) { | ||
| LogPrint(BCLog::LLMQ_DKG, "CDKGPendingMessages::%s -- already seen %s, peer=%d\n", __func__, hash.ToString(), from); | ||
| return; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check duplicates before consuming the per-node quota.
Line 85 increments messagesPerNode[from] before Line 87 drops duplicate hashes, so duplicate retransmits permanently consume that peer’s quota until Clear() even though nothing was queued.
Proposed fix
- if (messagesPerNode[from] >= maxMessagesPerNode) {
- // TODO ban?
- LogPrint(BCLog::LLMQ_DKG, "CDKGPendingMessages::%s -- too many messages, peer=%d\n", __func__, from);
- return;
- }
- messagesPerNode[from]++;
-
if (!seenMessages.emplace(hash).second) {
LogPrint(BCLog::LLMQ_DKG, "CDKGPendingMessages::%s -- already seen %s, peer=%d\n", __func__, hash.ToString(), from);
return;
}
+
+ if (messagesPerNode[from] >= maxMessagesPerNode) {
+ // TODO ban?
+ seenMessages.erase(hash);
+ LogPrint(BCLog::LLMQ_DKG, "CDKGPendingMessages::%s -- too many messages, peer=%d\n", __func__, from);
+ return;
+ }
+ messagesPerNode[from]++;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (messagesPerNode[from] >= maxMessagesPerNode) { | |
| // TODO ban? | |
| LogPrint(BCLog::LLMQ_DKG, "CDKGPendingMessages::%s -- too many messages, peer=%d\n", __func__, from); | |
| return; | |
| } | |
| messagesPerNode[from]++; | |
| std::vector<std::pair<NodeId, std::shared_ptr<Message>>> ret; | |
| ret.reserve(binaryMessages.size()); | |
| for (const auto& bm : binaryMessages) { | |
| auto msg = std::make_shared<Message>(); | |
| try { | |
| *bm.second >> *msg; | |
| } catch (...) { | |
| msg = nullptr; | |
| } | |
| ret.emplace_back(std::make_pair(bm.first, std::move(msg))); | |
| if (!seenMessages.emplace(hash).second) { | |
| LogPrint(BCLog::LLMQ_DKG, "CDKGPendingMessages::%s -- already seen %s, peer=%d\n", __func__, hash.ToString(), from); | |
| return; | |
| if (!seenMessages.emplace(hash).second) { | |
| LogPrint(BCLog::LLMQ_DKG, "CDKGPendingMessages::%s -- already seen %s, peer=%d\n", __func__, hash.ToString(), from); | |
| return; | |
| } | |
| if (messagesPerNode[from] >= maxMessagesPerNode) { | |
| // TODO ban? | |
| seenMessages.erase(hash); | |
| LogPrint(BCLog::LLMQ_DKG, "CDKGPendingMessages::%s -- too many messages, peer=%d\n", __func__, from); | |
| return; | |
| } | |
| messagesPerNode[from]++; |
🤖 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/dkgsessionhandler.h` around lines 80 - 89, In CDKGPendingMessages,
the duplicate check in the message handling flow should happen before consuming
the per-node quota. Update the logic around messagesPerNode[from] and
seenMessages.emplace(hash) so duplicate hashes are dropped first, and only
increment messagesPerNode[from] after a message is confirmed new and will be
queued. Keep the existing behavior in the same method, but reorder the checks to
avoid counting retransmits against the peer.
Comment on lines
+108
to
+113
| try { | ||
| CDataStream s(vRecv); // copy; deserialization does not advance the caller's stream | ||
| if (msg_type == NetMsgType::QCONTRIB) { | ||
| CDKGContribution qc; | ||
| s >> qc; | ||
| return qc.vvec != nullptr && qc.vvec->size() == threshold && | ||
| qc.contributions != nullptr && qc.contributions->blobs.size() <= size; | ||
| } else if (msg_type == NetMsgType::QCOMPLAINT) { | ||
| CDKGComplaint qc; | ||
| s >> qc; | ||
| return qc.badMembers.size() == qc.complainForMembers.size() && | ||
| qc.badMembers.size() <= size; | ||
| } else if (msg_type == NetMsgType::QJUSTIFICATION) { | ||
| CDKGJustification qj; | ||
| s >> qj; | ||
| return qj.contributions.size() <= size; | ||
| } else if (msg_type == NetMsgType::QPCOMMITMENT) { | ||
| CDKGPrematureCommitment qc; | ||
| s >> qc; | ||
| return qc.validMembers.size() <= size; | ||
| } | ||
| return false; | ||
| vRecv >> *msg; | ||
| } catch (const std::exception&) { | ||
| return false; | ||
| return nullptr; | ||
| } | ||
| if (!ValidateDKGMessageStructure(*msg, size, threshold)) return nullptr; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject trailing bytes after deserialization.
vRecv >> *msg can leave unread bytes that are still included in the wire hash. Accepting those payloads lets semantically identical messages bypass duplicate suppression with different trailing garbage.
Proposed fix
try {
vRecv >> *msg;
} catch (const std::exception&) {
return nullptr;
}
+ if (!vRecv.empty()) return nullptr;
if (!ValidateDKGMessageStructure(*msg, size, threshold)) return nullptr;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try { | |
| CDataStream s(vRecv); // copy; deserialization does not advance the caller's stream | |
| if (msg_type == NetMsgType::QCONTRIB) { | |
| CDKGContribution qc; | |
| s >> qc; | |
| return qc.vvec != nullptr && qc.vvec->size() == threshold && | |
| qc.contributions != nullptr && qc.contributions->blobs.size() <= size; | |
| } else if (msg_type == NetMsgType::QCOMPLAINT) { | |
| CDKGComplaint qc; | |
| s >> qc; | |
| return qc.badMembers.size() == qc.complainForMembers.size() && | |
| qc.badMembers.size() <= size; | |
| } else if (msg_type == NetMsgType::QJUSTIFICATION) { | |
| CDKGJustification qj; | |
| s >> qj; | |
| return qj.contributions.size() <= size; | |
| } else if (msg_type == NetMsgType::QPCOMMITMENT) { | |
| CDKGPrematureCommitment qc; | |
| s >> qc; | |
| return qc.validMembers.size() <= size; | |
| } | |
| return false; | |
| vRecv >> *msg; | |
| } catch (const std::exception&) { | |
| return false; | |
| return nullptr; | |
| } | |
| if (!ValidateDKGMessageStructure(*msg, size, threshold)) return nullptr; | |
| try { | |
| vRecv >> *msg; | |
| } catch (const std::exception&) { | |
| return nullptr; | |
| } | |
| if (!vRecv.empty()) return nullptr; | |
| if (!ValidateDKGMessageStructure(*msg, size, threshold)) return nullptr; |
🤖 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/net_dkg.cpp` around lines 108 - 113, The deserialization path in the
DKG message handling currently accepts payloads even when unread trailing bytes
remain after vRecv >> *msg, which can cause duplicate suppression to miss
semantically identical messages. Update the receive/parse logic in the DKG
message handler that calls vRecv >> *msg and then ValidateDKGMessageStructure so
it verifies the stream was fully consumed after deserialization, and reject the
message by returning nullptr if any bytes remain or parsing leaves extra data.
QSIGREC messages were deserialized and appended to CSigningManager::pendingRecoveredSigs after only cheap gating checks (quorum exists/active, dedup by hash). Verification is deferred to a single worker thread draining ~32 unique sign-hash sessions per cycle, while ingestion is unbounded and crypto-free on the network threads. The queue had no size cap and was never purged for banned/disconnected peers (RemoveBannedNodeStates only cleans the separate sig-shares subsystem), so a peer could enqueue faster than the queue drains and grow memory until the node is OOM-killed — a remote, unauthenticated DoS, worst against masternodes. Bound the queue with per-node (1000) and global (10000) caps, dropping over-cap messages silently (an honest peer can legitimately outrun the single-threaded drain during a burst, so no misbehaviour is scored). Add CSigningManager::RemoveNodesIf to purge banned peers' queues, invoked from WorkThreadSigning's periodic cleanup. Prune drained (empty) node entries during collection so the global-cap scan stays cheap and disconnected nodes' queues are reclaimed once drained. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
5 tasks
PastaPastaPasta
force-pushed
the
fix/qsigrec-oom
branch
from
July 2, 2026 15:47
b75b568 to
95e160d
Compare
![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: 95e160d20f
ℹ️ 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".
| // Drop pending recovered sigs queued by banned peers so a flood's backlog does not | ||
| // persist after the peer is banned (RemoveBannedNodeStates only cleans the sig-shares | ||
| // subsystem, not m_sig_manager's pending recovered sigs). | ||
| m_sig_manager.RemoveNodesIf([this](NodeId node_id) { return m_peer_manager->PeerIsBanned(node_id); }); |
There was a problem hiding this comment.
When a flooding peer disconnects before this 5s cleanup runs, this predicate never matches because PeerIsBanned delegates to PeerManagerImpl::IsBanned, which returns false once GetPeerRef(node_id) is null after FinalizeNode removes the peer. Those disconnected peers' pendingRecoveredSigs entries still count toward MAX_PENDING_RECSIGS_TOTAL, so an attacker can fill the new global cap with stale node IDs and cause subsequent honest QSIGRECs to be dropped until the expensive verifier drains them.
Useful? React with 👍 / 👎.
thepastaclaw
reviewed
Jul 2, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Two well-scoped LLMQ networking changes: a DoS fix bounding CSigningManager::pendingRecoveredSigs with per-node/global caps plus pruning of drained entries, and a refactor templating CDKGPendingMessages on message type so DKG payloads deserialize exactly once on the p2p thread while preserving inventory-hash bytes and misbehaviour semantics. Both changes look correct. One convergent perf suggestion carried forward.
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5[high] (general, failed), opus (dash-core-commit-history), claude-sonnet-5 (dash-core-commit-history, failed), gpt-5.5[high] (dash-core-commit-history, failed); verifier: opus; specialists: dash-core-commit-history
🟡 1 suggestion(s)
🤖 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/signing.cpp`:
- [SUGGESTION] src/llmq/signing.cpp:400-408: Global pending-queue cap is recomputed by full scan on every incoming QSIGREC
VerifyAndProcessRecoveredSig sums list.size() over every entry in pendingRecoveredSigs under cs_pending on each admission (lines 400-403). The pruning added in CollectPendingRecoveredSigsToVerify (lines 465-471) keeps the map bounded to nodes with in-flight sigs, so this is O(#active peers with pending) rather than O(everyone-ever) — small in normal operation. However, this path runs hottest exactly during the burst/flood scenario the PR is defending against, when many peers can be near their per-node cap and every new message forces another scan under the shared lock also needed by the drain thread. A running counter incremented in the emplace_back on line 416 and decremented at the erase points (drain in CollectPendingRecoveredSigsToVerify line 458, prune loop lines 465-471, and RemoveNodesIf lines 422-428) would make the global check O(1) with no behavioral change. Not a correctness issue — take it or leave it.
Inline submission via scripts/review_poster.py hit GitHub HTTP 422, so I posted the same verified review as a body-only COMMENT.
thepastaclaw
approved these changes
Jul 2, 2026
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Single carried-forward suggestion from the prior review: the global pending-queue cap in VerifyAndProcessRecoveredSig is computed by full scan on every incoming QSIGREC. Still valid at head 95e160d — code unchanged from b75b568d at src/llmq/signing.cpp:400-408. The latest delta only dropped an unrelated DKG refactor commit; no new signing-side findings. No blockers.
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5[high] (general, failed), opus (dash-core-commit-history), claude-sonnet-5 (dash-core-commit-history), gpt-5.5[high] (dash-core-commit-history, failed); verifier: opus; specialists: dash-core-commit-history
🟡 1 suggestion(s)
🤖 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/signing.cpp`:
- [SUGGESTION] src/llmq/signing.cpp:400-408: Global pending-queue cap is recomputed by full scan on every incoming QSIGREC
VerifyAndProcessRecoveredSig sums list.size() across every entry in pendingRecoveredSigs under cs_pending on each admission. Thanks to the empty-entry pruning added in CollectPendingRecoveredSigsToVerify (lines 465-471), the map is bounded to nodes with in-flight sigs, so in practice this is O(#peers-with-pending) rather than O(everyone-ever) — small in normal operation and not a correctness issue. However, this scan runs hottest exactly during the burst/flood scenario this PR defends against, when many peers can be near their per-node cap and every new QSIGREC forces another O(N) walk under a lock the drain thread and other network threads also need. Maintaining a running counter incremented at the emplace_back on line 416 and decremented at each erase point (drain in CollectPendingRecoveredSigsToVerify line 458, the prune loop on lines 465-471, and RemoveNodesIf lines 422-428) would make the global check O(1) with no behavioral change. Take it or leave it.
Comment on lines
+400
to
+408
| size_t total_pending{0}; | ||
| for (const auto& node_entry : pendingRecoveredSigs) { | ||
| total_pending += node_entry.second.size(); | ||
| } | ||
| if (total_pending >= MAX_PENDING_RECSIGS_TOTAL) { | ||
| LogPrint(BCLog::LLMQ, "CSigningManager::%s -- global pending recovered sigs cap reached (%d), dropping sig from node=%d\n", | ||
| __func__, MAX_PENDING_RECSIGS_TOTAL, from); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Global pending-queue cap is recomputed by full scan on every incoming QSIGREC
VerifyAndProcessRecoveredSig sums list.size() across every entry in pendingRecoveredSigs under cs_pending on each admission. Thanks to the empty-entry pruning added in CollectPendingRecoveredSigsToVerify (lines 465-471), the map is bounded to nodes with in-flight sigs, so in practice this is O(#peers-with-pending) rather than O(everyone-ever) — small in normal operation and not a correctness issue. However, this scan runs hottest exactly during the burst/flood scenario this PR defends against, when many peers can be near their per-node cap and every new QSIGREC forces another O(N) walk under a lock the drain thread and other network threads also need. Maintaining a running counter incremented at the emplace_back on line 416 and decremented at each erase point (drain in CollectPendingRecoveredSigsToVerify line 458, the prune loop on lines 465-471, and RemoveNodesIf lines 422-428) would make the global check O(1) with no behavioral change. Take it or leave it.
source: ['claude', 'codex']
There was a problem hiding this comment.
Resolved in 5c10539 — Global pending-queue cap is recomputed by full scan on every incoming QSIGREC 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.
UdjinM6
previously approved these changes
Jul 3, 2026
… check Maintain a running pendingRecoveredSigsCount alongside pendingRecoveredSigs instead of summing list sizes across the whole map on every incoming QSIGREC message. The scan added by the previous commit runs under cs_pending on the network thread and gets more expensive as the number of distinct peers with pending entries grows, exactly during the flood scenario the cap defends against.
thepastaclaw
approved these changes
Jul 3, 2026
There was a problem hiding this comment.
Code Review
Prior finding prior-1 (O(n) global-cap scan on every QSIGREC) is FIXED by commit 5c10539: verified pendingRecoveredSigsCount is correctly maintained at all mutation sites — increment at signing.cpp:413 paired with emplace_back, decrement at signing.cpp:421 in RemoveNodesIf (subtracts list size before erase), and erasedCount tracking at signing.cpp:458 with single subtraction at line 461 in the drain loop. The empty-entry prune loop (signing.cpp:466-472) correctly does not touch the counter since empty lists contribute zero. Counter is GUARDED_BY(cs_pending) with zero-initialization (signing.h:184). No new in-scope defects in the delta or cumulative PR.
Source: reviewers: opus (general), claude-sonnet-5 (general), gpt-5.5 (general), opus (dash-core-commit-history), claude-sonnet-5 (dash-core-commit-history), gpt-5.5 (dash-core-commit-history); verifier: opus; specialists: dash-core-commit-history
UdjinM6
approved these changes
Jul 7, 2026
5 tasks
thepastaclaw
pushed a commit
to thepastaclaw/dash
that referenced
this pull request
Jul 22, 2026
… remote OOM 5c10539 perf: avoid O(n) map scan on every recovered sig in pending-queue cap check (pasta) 95e160d fix: bound pending recovered sig queue to prevent remote OOM (pasta) Pull request description: ## Issue Incoming `QSIGREC` (recovered signature) P2P messages are deserialized and appended to the per-node queue `CSigningManager::pendingRecoveredSigs` after only cheap gating checks (quorum exists/active, dedup by full hash). Actual verification is expensive (BLS) and is deferred to a single worker thread that drains only ~32 unique sign-hash sessions per cycle, whereas ingestion is crypto-free and happens on the network threads in parallel across connections. Two problems compound: - **No size bound.** `pendingRecoveredSigs` had no per-node or global cap, and the dedup check keys on the full message hash (which includes the signature), so it does not constrain a peer sending many distinct messages. Ingestion structurally outruns the drain. - **No cleanup on ban/disconnect.** `RemoveBannedNodeStates` only cleans the separate sig-shares subsystem (`CSigSharesManager::nodeStates`); nothing purged `pendingRecoveredSigs` for banned or disconnected peers. Net effect: a peer (or several) can grow the queue faster than it drains, increasing RSS until the node is OOM-killed — a remote, unauthenticated memory-exhaustion DoS. Highest impact against masternodes, which accept many inbound connections and run the LLMQ signing behind ChainLocks / InstantSend. ## Fix - **Bound the queue** in `VerifyAndProcessRecoveredSig` with a per-node cap (`MAX_PENDING_RECSIGS_PER_NODE = 1000`) and a global cap (`MAX_PENDING_RECSIGS_TOTAL = 10000`). Over-cap messages are dropped **silently, without a misbehaviour score** — verification is single-threaded, so an honest peer can legitimately outrun the drain during a burst, and this is queue-depth backpressure rather than a protocol violation (contrast the per-message structural caps for sig-shares, which do ban). - **Purge banned peers' queues** via a new `CSigningManager::RemoveNodesIf(predicate)` (mirroring `CSigSharesManager::RemoveNodesIf`), invoked from `WorkThreadSigning`'s periodic cleanup using `PeerIsBanned`. - **Prune drained (empty) node entries** during collection, so the global-cap scan stays cheap and disconnected nodes' queues are reclaimed once drained without waiting for a ban. With these caps the attacker-controllable pending queue is bounded to roughly **8 MB** worst case (10,000 entries × ~750 bytes resident per entry; the in-memory `CRecoveredSig` is 696 bytes vs. 193 on the wire, dominated by `CBLSLazySignature`). Previously it was unbounded. ## Testing - `make check` targets build clean; full `dashd` builds and links. - `feature_llmq_signing.py` passes — normal recovered-sig relay across masternodes is unaffected by the caps. - `lint-whitespace`, `lint-includes`, `lint-circular-dependencies` clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) ACKs for top commit: UdjinM6: utACK 5c10539 Tree-SHA512: c51f115c26d03a5174b22e311f0a385f776d7b67578735b3bf342b504b3996448e0d34ebe03a9cc28e2cdf52a3edf1088213300ce64b69ec80c90de6d806a19d
thepastaclaw
pushed a commit
to thepastaclaw/dash
that referenced
this pull request
Jul 22, 2026
… remote OOM 5c10539 perf: avoid O(n) map scan on every recovered sig in pending-queue cap check (pasta) 95e160d fix: bound pending recovered sig queue to prevent remote OOM (pasta) Pull request description: ## Issue Incoming `QSIGREC` (recovered signature) P2P messages are deserialized and appended to the per-node queue `CSigningManager::pendingRecoveredSigs` after only cheap gating checks (quorum exists/active, dedup by full hash). Actual verification is expensive (BLS) and is deferred to a single worker thread that drains only ~32 unique sign-hash sessions per cycle, whereas ingestion is crypto-free and happens on the network threads in parallel across connections. Two problems compound: - **No size bound.** `pendingRecoveredSigs` had no per-node or global cap, and the dedup check keys on the full message hash (which includes the signature), so it does not constrain a peer sending many distinct messages. Ingestion structurally outruns the drain. - **No cleanup on ban/disconnect.** `RemoveBannedNodeStates` only cleans the separate sig-shares subsystem (`CSigSharesManager::nodeStates`); nothing purged `pendingRecoveredSigs` for banned or disconnected peers. Net effect: a peer (or several) can grow the queue faster than it drains, increasing RSS until the node is OOM-killed — a remote, unauthenticated memory-exhaustion DoS. Highest impact against masternodes, which accept many inbound connections and run the LLMQ signing behind ChainLocks / InstantSend. ## Fix - **Bound the queue** in `VerifyAndProcessRecoveredSig` with a per-node cap (`MAX_PENDING_RECSIGS_PER_NODE = 1000`) and a global cap (`MAX_PENDING_RECSIGS_TOTAL = 10000`). Over-cap messages are dropped **silently, without a misbehaviour score** — verification is single-threaded, so an honest peer can legitimately outrun the drain during a burst, and this is queue-depth backpressure rather than a protocol violation (contrast the per-message structural caps for sig-shares, which do ban). - **Purge banned peers' queues** via a new `CSigningManager::RemoveNodesIf(predicate)` (mirroring `CSigSharesManager::RemoveNodesIf`), invoked from `WorkThreadSigning`'s periodic cleanup using `PeerIsBanned`. - **Prune drained (empty) node entries** during collection, so the global-cap scan stays cheap and disconnected nodes' queues are reclaimed once drained without waiting for a ban. With these caps the attacker-controllable pending queue is bounded to roughly **8 MB** worst case (10,000 entries × ~750 bytes resident per entry; the in-memory `CRecoveredSig` is 696 bytes vs. 193 on the wire, dominated by `CBLSLazySignature`). Previously it was unbounded. ## Testing - `make check` targets build clean; full `dashd` builds and links. - `feature_llmq_signing.py` passes — normal recovered-sig relay across masternodes is unaffected by the caps. - `lint-whitespace`, `lint-includes`, `lint-circular-dependencies` clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) ACKs for top commit: UdjinM6: utACK 5c10539 Tree-SHA512: c51f115c26d03a5174b22e311f0a385f776d7b67578735b3bf342b504b3996448e0d34ebe03a9cc28e2cdf52a3edf1088213300ce64b69ec80c90de6d806a19d
5 tasks
PastaPastaPasta
added a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jul 29, 2026
… remote OOM Backport of dashpay#7402 (upstream merge 976e15a, cherry-picked with -m1). v23.1.x adaptation: the new RemoveNodesIf call site uses PeerManagerInternal::PeerIsBanned, which develop got from the dashpay#7115 net/consensus separation refactor (not on this branch). The three-line accessor (pure-virtual declaration in PeerManagerInternal, override declaration and trivial forwarder to the pre-existing IsBanned in PeerManagerImpl) is ported here verbatim from develop so the dashpay#7402 hunks apply unchanged. Everything else is unchanged from upstream. (cherry picked from commit 976e15ad0114d475db0dc51ae125da4be6bea01a)
PastaPastaPasta
added a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jul 29, 2026
… remote OOM Backport of dashpay#7402 (upstream merge 976e15a, cherry-picked with -m1). v23.1.x adaptation: the new RemoveNodesIf call site uses PeerManagerInternal::PeerIsBanned, which develop got from the dashpay#7115 net/consensus separation refactor (not on this branch). The three-line accessor (pure-virtual declaration in PeerManagerInternal, override declaration and trivial forwarder to the pre-existing IsBanned in PeerManagerImpl) is ported here verbatim from develop so the dashpay#7402 hunks apply unchanged. Everything else is unchanged from upstream. (cherry picked from commit 976e15ad0114d475db0dc51ae125da4be6bea01a)
PastaPastaPasta
added a commit
that referenced
this pull request
Jul 30, 2026
24920a0 chore: prepare v23.1.8 release (pasta) 2194248 Merge #7348: fix: penalize oversized notfound messages (pasta) f5c72c3 Merge #7347: fix: punish invalid dstx messages (pasta) 550caf7 Merge #7465: fix(qt): handle pixel-sized fonts when scaling widgets (pasta) e203710 Merge #7419: fix(net): bound CoinJoin message vector intake (pasta) 5f5b960 Merge #7418: fix(net): bound signing message vector intake (Pasta) 7cc2cca Merge #7450: test: make governance vote fixtures wire-valid (Pasta) f011c80 Merge #7440: fix(net): bound governance vote signature deserialization (Pasta) 4b4d96a Merge #7442: fix(net): authorize governance inv responses via the net-layer per-peer request tracker (Pasta) f855b13 Merge #7444: fix(net): bound bloom message vectors before allocation (Pasta) 5b5c6fb Merge #7415: fix: bound pending sig share queue (Pasta) 9bbe808 Merge #7416: fix(net): bound quorum data response vectors (Pasta) da42f50 Merge #7424: fix: bound ChainLock seen cache (Pasta) 5b310df Merge #7438: fix: bound SPORK signature deserialization (Pasta) e118d0c Merge #7259: fix: dangling point to cj client (Pasta) 9921621 Merge #7439: refactor: add bounded vector deserialization (Pasta) 89bdf7c Merge #7414: fix(net): throttle per-object governance vote sync requests (Pasta) 44c396d Merge #7402: fix: bound pending recovered sig queue to prevent remote OOM (Pasta) 05cfe27 Merge #7351: fix: limit signing share sessions per peer (pasta) 3ef3a5b Merge #7408: fix: bound DKG contribution blob intake (pasta) 0ea6532 Merge #7387: test: migrate governance inv cache coverage to unit tests (Pasta) 8ffdf7f Merge #7398: backport: compact block relay hardening (bitcoin#26898, bitcoin#27626, bitcoin#27743, bitcoin#26969, bitcoin#29412, bitcoin#32646, bitcoin#33296) (Pasta) 2915142 backport: bitcoin#27608 - p2p: Avoid prematurely clearing download state for other peers (PastaClaw) 90b5473 Merge #7396: fix: run of circular-dependencies with python3.15 (Pasta) b003cdc Merge #7395: ci: update GitHub Actions pins for Node 24 (pasta) 97c3dd1 Merge #7394: fix: stabilize par help text in manpages (pasta) 8f8616b Merge #7372: backport: bitcoin#32693: depends: fix cmake compatibility error for freetype (pasta) 48f72be Merge #7360: fix: empty platformP2PPort deprecated field in protx listdiff results (pasta) a8cccff Merge #7298: fix(qt): keep PoSe score visible when hiding banned masternodes (pasta) Pull request description: Release PR for Dash Core v23.1.8, a patch release on top of v23.1.7. Fast-forwards from `v23.1.x` (currently at `chore: prepare v23.1.7 release`), 29 commits, no merge commits, no conflicts. ## Contents Backports of PRs already reviewed and merged on `develop`: `#7259` `#7347` `#7348` `#7351` `#7298` `#7360` `#7372` `#7387` `#7394` `#7395` `#7396` `#7398` `#7402` `#7408` `#7414` `#7415` `#7416` `#7418` `#7419` `#7424` `#7438` `#7439` `#7440` `#7442` `#7444` `#7450` `#7465` Plus `backport: bitcoin#27608`, a single commit taken from Dash #7237 because #7398's compact-block hardening depends on it. The rest of that v0.26 batch is intentionally not included on v23.1.x. The commit is byte-identical to its reviewed counterpart inside #7237. And release preparation: version bump, regenerated man pages, release notes, archived 23.1.7 notes. ## Note for reviewers: this branch was rebuilt An earlier revision of this PR was discarded and the branch rebuilt from scratch. Review comments on the previous revision point at commits that no longer exist, though the feedback itself was carried over (see below). The reason: several commits titled `Merge #NNNN` in the earlier revision contained substantial code that exists nowhere upstream — apparently written from a description of each PR rather than ported from its diff. For example, `feature_llmq_simplepose.py` is byte-identical between v23.1.7 and `develop`, yet the earlier `Merge #7408` rewrote 66 lines of it; `test/functional/p2p_governance_invs.py` does not exist on `develop` at all, yet had grown from 62 to 148 lines. That mislabeling matters because a commit titled `Merge #NNNN` invites less scrutiny, not more. It also had consequences: the earlier revision was **missing #7440 entirely**, and contained eleven consecutive commits that did not compile (code written against newer upstream APIs this branch does not have — `Misbehaving(Peer&)`, and `PeerIsBanned` used five commits before it was declared). Every commit on this branch has now been diffed against its upstream merge commit. Where a backport differs, it is because v23.1.x predates an upstream refactor and the change had to be applied to the pre-refactor file — for example #7418 and #7438 patch `signing_shares.cpp` / `spork.cpp` where upstream patches `net_signing.cpp` / `net_processing.cpp`. ## Dropped from this branch - **#7350** (`net: don't lock cs_main while reading blocks`) — dropped on review feedback. It is a 110-line lock-structure refactor of `ProcessGetBlockData` with no measured benefit, and it would add avoidable churn to the eventual master→develop merge-back. Nothing on this branch depends on it: #7398's compact-block work precedes it, and the remaining 14 commits replay with zero conflicts once it is removed. Thanks @knst. ## Added after the initial review pass - **#7351** (`fix: limit signing share sessions per peer`) — cherry-picked as a single commit and placed before #7402, matching upstream's merge order. The include block additionally carries `<ranges>`: upstream's diff adds only `<algorithm>` because develop already had it, whereas v23.1.x did not and the backported `GetSessionCount()` / `GetAnnouncementSessionCount()` use `std::ranges::count_if`. - **#7465** (`fix(qt): handle pixel-sized fonts when scaling widgets`) — cherry-picked from the five upstream commits. `optiontests.cpp` additionally includes `qt/guiutil_font.h`, because `fontsLoaded()` and `updateFonts()` are declared there on v23.1.x while develop declares them in `qt/guiutil.h`, which is all the upstream test includes. Two further backports were added later and applied without any adaptation -- their diffs are byte-for-byte identical to upstream: - **#7347** (`fix: punish invalid dstx messages`) - **#7348** (`fix: penalize oversized notfound messages`) ## Adaptations worth flagging - **#7360** — upstream gates `platformP2PPort` / `platformHTTPPort` in `protx listdiff` behind `IsServiceDeprecatedRPCEnabled()`. On 23.x those deprecated fields are deliberately not enforced through gating (see `bbcd9d543e6`), so shipping the gate as-is would silently drop two fields that v23.1.7 always returned. Changed to `if (true)` with a comment, per review feedback, keeping the block aligned with `develop`. The substantive fix from #7360 — reading the live port from `netInfo` instead of the always-zero scalar — is retained. - **#7415** — the pending-map caps (`MAX_PENDING_SIG_SHARES_PER_NODE`, `MAX_PENDING_SIG_SHARES_TOTAL`) are backported. The additional bound upstream places on batches awaiting verification is not, because it guards a condition that does not exist here: upstream's dispatcher pushes one task per batch inside an inner loop, whereas v23.1.x pushes a single looping worker per 10 ms tick. There is no unbounded task queue to bound. - **Man pages** — regenerated without the `lock` debug category, which only exists under `DEBUG_LOCKCONTENTION` and so is absent from release binaries. Thanks @UdjinM6 for catching this. ## Known CI failure macOS jobs are expected to fail. `actions/upload-artifact@v6` rejects filenames containing `:`, and the Xcode SDK ships Perl man pages with `::` in the name. A release-branch-only workaround existed on the earlier revision but was dropped as it corresponds to no upstream PR. This is accepted for this release. ## Testing - Every commit through #7465 compiles individually (verified for 27 of the 29; the three additions below were verified at the tip) — verified individually, not just at the tip. - Full build clean; no new warnings. - Unit tests pass. - Functional tests pass: `feature_llmq_signing` (both variants), `feature_llmq_chainlocks`, `feature_llmq_dkgerrors`, `feature_llmq_is_cl_conflicts`, `p2p_instantsend`, `feature_dip3_deterministicmns` (both wallet types), `rpc_coinjoin`. - Qt unit tests pass (32 cases, run under the `cocoa` platform plugin so the pixel-sized font regression from #7465 actually executes rather than self-skipping). - Lint: one pre-existing `lint-cppcheck-dash` failure, identical on v23.1.7, in files this branch does not touch. Top commit has no ACKs. Tree-SHA512: 0fa469c9a33820aa85fbb8b90c5877409d09490f746f1300b05ceda470a600765f900bec42e5aad5d90a2d46b44e28c20e44dc4a8fe719553a072298069eaec4
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
3 participants
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
Incoming
QSIGREC(recovered signature) P2P messages are deserialized and appended to the per-node queueCSigningManager::pendingRecoveredSigsafter only cheap gating checks (quorum exists/active, dedup by full hash). Actual verification is expensive (BLS) and is deferred to a single worker thread that drains only ~32 unique sign-hash sessions per cycle, whereas ingestion is crypto-free and happens on the network threads in parallel across connections.Two problems compound:
pendingRecoveredSigshad no per-node or global cap, and the dedup check keys on the full message hash (which includes the signature), so it does not constrain a peer sending many distinct messages. Ingestion structurally outruns the drain.RemoveBannedNodeStatesonly cleans the separate sig-shares subsystem (CSigSharesManager::nodeStates); nothing purgedpendingRecoveredSigsfor banned or disconnected peers.Net effect: a peer (or several) can grow the queue faster than it drains, increasing RSS until the node is OOM-killed — a remote, unauthenticated memory-exhaustion DoS. Highest impact against masternodes, which accept many inbound connections and run the LLMQ signing behind ChainLocks / InstantSend.
Fix
VerifyAndProcessRecoveredSigwith a per-node cap (MAX_PENDING_RECSIGS_PER_NODE = 1000) and a global cap (MAX_PENDING_RECSIGS_TOTAL = 10000). Over-cap messages are dropped silently, without a misbehaviour score — verification is single-threaded, so an honest peer can legitimately outrun the drain during a burst, and this is queue-depth backpressure rather than a protocol violation (contrast the per-message structural caps for sig-shares, which do ban).CSigningManager::RemoveNodesIf(predicate)(mirroringCSigSharesManager::RemoveNodesIf), invoked fromWorkThreadSigning's periodic cleanup usingPeerIsBanned.With these caps the attacker-controllable pending queue is bounded to roughly 8 MB worst case (10,000 entries × ~750 bytes resident per entry; the in-memory
CRecoveredSigis 696 bytes vs. 193 on the wire, dominated byCBLSLazySignature). Previously it was unbounded.Testing
make checktargets build clean; fulldashdbuilds and links.feature_llmq_signing.pypasses — normal recovered-sig relay across masternodes is unaffected by the caps.lint-whitespace,lint-includes,lint-circular-dependenciesclean.🤖 Generated with Claude Code