fix: bound DKG pending message queues across NodeId reconnects - #7524
fix: bound DKG pending message queues across NodeId reconnects#7524PastaPastaPasta wants to merge 1 commit into
Conversation
ee9c5e9 to
b988249
Compare
|
This pull request has conflicts, please rebase. |
b988249 to
12f74e3
Compare
PastaPastaPasta
changed the title
CDKGPendingMessages limited intake per NodeId, but NodeId is an ephemeral per-connection identifier. Reconnecting peers could therefore obtain a fresh quota while previously queued payloads remained resident; observer-mode handlers could retain that state indefinitely. Bound each message-type queue by actual serialized payload bytes derived from its quorum parameters while retaining the per-connection count limit. Reject duplicates and invalid sizes before applying capacity pressure, evict the oldest payload from the peer with the largest occupancy when necessary, and keep locally generated DKG messages exempt. Add a protocol-handler finalization hook that releases a disconnecting peer's unprocessed payloads, quota, and corresponding seen hashes. Cover byte accounting, reconnect bounds, duplicate replay, eviction, local messages, and disconnect cleanup with focused unit tests.
12f74e3 to
b26434d
Compare
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
|
|
⛔ Blockers found — Sonnet deferred (commit b26434d) |
![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: b26434df46
ℹ️ 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".
| for (auto it = pendingMessages.begin(); it != pendingMessages.end();) { | ||
| if (it->from == nodeId) { | ||
| // Free the content-hash slot too; otherwise a reconnecting attacker | ||
| // can grow seenMessages without bound even after payloads are dropped | ||
| // (especially in observer mode where Clear() never runs). The hash is | ||
| // stored alongside the payload, so no re-hashing happens here. | ||
| it = EraseEntry(it); |
There was a problem hiding this comment.
When a requested, structurally valid DKG message is queued for a later phase and its delivery peer disconnects before the worker pops it, this loop erases both the only retained payload and its hash. NetDKG::ProcessMessage authenticates the connection but does not require the delivery peer's identity to match the message's signed proTxHash, so that peer may merely be a relay and its disconnect does not invalidate the contribution, complaint, justification, or commitment. Normal connection churn can therefore make a valid message disappear and potentially prevent a DKG round from completing if no peer announces it again; release the connection quota on disconnect but leave the globally byte-bounded payload queued.
AGENTS.md reference: AGENTS.md:L164-L164
Useful? React with 👍 / 👎.
WalkthroughThis PR adds size-based limits and byte accounting to DKG pending message queues in CDKGPendingMessages. It rejects invalid or oversized peer messages, evicts the greediest peer's queued messages when capacity is exceeded, and tracks per-peer and total queued bytes. A new NetHandler::FinalizeNode virtual hook is called from PeerManagerImpl on peer disconnect and propagated through NetDKG to remove disconnected nodes' queued state. A new test suite validates the queue limits and cleanup behavior. Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 1
🧹 Nitpick comments (1)
src/net_processing.h (1)
133-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the
cs_mainlock contract.
PeerManagerImpl::FinalizeNodeinvokes this callback while it holdscs_main. State that implementations must not re-enterPeerManagerfrom this callback. This prevents lock re-entry in future handlers.As per coding guidelines, reserve comments for non-obvious invariants.
Proposed update
//! Called when a peer is fully disconnected. Handlers may free any //! per-NodeId state keyed on the ephemeral connection id. + //! Called with ::cs_main held. Implementations must not re-enter PeerManager. virtual void FinalizeNode(NodeId /*nodeid*/) {}🤖 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/net_processing.h` around lines 133 - 136, Update the documentation for the virtual FinalizeNode callback to state that it is invoked while cs_main is held and implementations must not re-enter PeerManager from the callback. Keep the existing disconnect and per-NodeId state guidance intact.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.cpp`:
- Around line 173-183: Update PopPendingMessages so hashes for permanently
popped payloads are removed from seenMessages, using the popped message’s
identifying hash before or as its queue entry is released. Preserve hash
tracking for messages that remain queued, and ensure the cleanup also covers
popped entries from disconnected nodes.
---
Nitpick comments:
In `@src/net_processing.h`:
- Around line 133-136: Update the documentation for the virtual FinalizeNode
callback to state that it is invoked while cs_main is held and implementations
must not re-enter PeerManager from the callback. Keep the existing disconnect
and per-NodeId state guidance intact.
🪄 Autofix
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 Plus
Run ID: 962eb816-05c8-438f-85ce-7f157c84d412
📒 Files selected for processing (8)
src/Makefile.test.includesrc/llmq/dkgsessionhandler.cppsrc/llmq/dkgsessionhandler.hsrc/llmq/net_dkg.cppsrc/llmq/net_dkg.hsrc/net_processing.cppsrc/net_processing.hsrc/test/llmq_dkg_pending_tests.cpp
| auto& front = pendingMessages.front(); | ||
| ret.emplace_back(front.from, std::move(front.msg)); | ||
| // Popped messages are handed to the DKG session; their content hash stays | ||
| // in seenMessages so AlreadyHave() keeps suppressing re-requests. | ||
| if (front.from >= 0) { | ||
| pendingBytes -= front.bytes; | ||
| } | ||
| if (auto qit = queuedBytesPerNode.find(front.from); qit != queuedBytesPerNode.end()) { | ||
| qit->second -= front.bytes; | ||
| if (qit->second == 0) queuedBytesPerNode.erase(qit); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find every release path for seenMessages and the callers of ClearPendingMessages.
rg -n 'seenMessages' src/llmq/
rg -n -C4 'ClearPendingMessages|HasSeen\(' src/llmq/Repository: dashpay/dash
Length of output: 5545
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== dkgsessionhandler.h outline =="
ast-grep outline src/llmq/dkgsessionhandler.h --view expanded | sed -n '1,220p'
echo "== dkgsessionhandler.cpp relevant ranges =="
cat -n src/llmq/dkgsessionhandler.cpp | sed -n '1,270p'
echo "== net_dkg relevant ranges around RemoveNode and round start =="
cat -n src/llmq/net_dkg.cpp | sed -n '530,585p'
cat -n src/llmq/net_dkg.cpp | sed -n '655,685p'Repository: dashpay/dash
Length of output: 15993
Pop seenMessages when messages leave the queue permanently.
PopPendingMessages moves each payload out while leaving its 32-byte hash in seenMessages. RemoveNode only deletes hashes for still-queued entries, so disconnected/reconnected peers can exhaust seenMessages over active phases with no payload-count cap per node. Add a release path for processed payload hashes; for popped payloads, either delete the hash or bound seenMessages independently of pendingBytes.
🤖 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.cpp` around lines 173 - 183, Update
PopPendingMessages so hashes for permanently popped payloads are removed from
seenMessages, using the popped message’s identifying hash before or as its queue
entry is released. Preserve hash tracking for messages that remain queued, and
ensure the cleanup also covers popped entries from disconnected nodes.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The byte accounting and eviction path is generally coherent, but both reported blockers are confirmed. Disconnect cleanup can discard valid relayed DKG messages, while hashes retained after dequeue escape all new memory bounds, so the PR does not yet safely achieve its liveness and reconnect-memory goals.
Validated blockers were found in the Codex precheck. Sonnet 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/dkgsessionhandler.cpp`:
- [BLOCKING] src/llmq/dkgsessionhandler.cpp:206-234: Preserve valid relayed messages after delivery-peer disconnects
`PendingMessage::from` is the NodeId of the connection that delivered the payload, not the quorum member identified by the message's signed `proTxHash`. `NetDKG::ProcessGetData` allows any active node that previously accepted an object to serve it, so the delivery peer may only be a relay. If that relay disconnects before the phase worker pops the message, `RemoveNode()` deletes the otherwise valid, self-contained payload. While it was queued, `AlreadyHave()` returned true, and `SendMessages()` may consequently have called `ForgetTxHash()` for other peers' announcements, so removing the payload does not guarantee another download. Keep queued payloads and their hashes after disconnect and release only the connection-scoped count quota; the new queue-wide byte cap already bounds retained payload memory until processing or eviction.
- [BLOCKING] src/llmq/dkgsessionhandler.cpp:167-184: Bound hashes retained after messages leave the byte-accounted queue
`PopPendingMessages()` removes a remote payload from `pendingBytes` and `queuedBytesPerNode` but deliberately leaves its hash in `seenMessages`. A later `RemoveNode()` then returns without touching that hash because the NodeId no longer has queued bytes, while also releasing the connection's count quota. An authenticated peer can repeatedly announce requested, unique, structurally valid messages that the active phase worker pops but rejects without a ban—for example, messages naming a known but non-current quorum base mapped to the same handler—and reconnect under fresh NodeIds. The payload-byte counter remains near zero while `seenMessages` grows without a parameter-derived bound until the handler's next DKG round calls `Clear()`. Track retained hashes under a separate bound, or remove rejected hashes after worker verification while relying on the session's accepted-object state for valid messages.
| void CDKGPendingMessages::RemoveNode(NodeId nodeId) | ||
| { | ||
| // Own/local enqueues use from=-1 and are not tied to a peer disconnect. | ||
| if (nodeId < 0) { | ||
| return; | ||
| } | ||
|
|
||
| LOCK(cs_messages); | ||
| messagesPerNode.erase(nodeId); | ||
|
|
||
| // Runs under ::cs_main (via PeerManagerImpl::FinalizeNode), so skip the list | ||
| // scan entirely for the overwhelmingly common case of a peer that never | ||
| // queued a DKG message. | ||
| if (queuedBytesPerNode.find(nodeId) == queuedBytesPerNode.end()) { | ||
| return; | ||
| } | ||
|
|
||
| for (auto it = pendingMessages.begin(); it != pendingMessages.end();) { | ||
| if (it->from == nodeId) { | ||
| // Free the content-hash slot too; otherwise a reconnecting attacker | ||
| // can grow seenMessages without bound even after payloads are dropped | ||
| // (especially in observer mode where Clear() never runs). The hash is | ||
| // stored alongside the payload, so no re-hashing happens here. | ||
| it = EraseEntry(it); | ||
| } else { | ||
| ++it; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Preserve valid relayed messages after delivery-peer disconnects
PendingMessage::from is the NodeId of the connection that delivered the payload, not the quorum member identified by the message's signed proTxHash. NetDKG::ProcessGetData allows any active node that previously accepted an object to serve it, so the delivery peer may only be a relay. If that relay disconnects before the phase worker pops the message, RemoveNode() deletes the otherwise valid, self-contained payload. While it was queued, AlreadyHave() returned true, and SendMessages() may consequently have called ForgetTxHash() for other peers' announcements, so removing the payload does not guarantee another download. Keep queued payloads and their hashes after disconnect and release only the connection-scoped count quota; the new queue-wide byte cap already bounds retained payload memory until processing or eviction.
| void CDKGPendingMessages::RemoveNode(NodeId nodeId) | |
| { | |
| // Own/local enqueues use from=-1 and are not tied to a peer disconnect. | |
| if (nodeId < 0) { | |
| return; | |
| } | |
| LOCK(cs_messages); | |
| messagesPerNode.erase(nodeId); | |
| // Runs under ::cs_main (via PeerManagerImpl::FinalizeNode), so skip the list | |
| // scan entirely for the overwhelmingly common case of a peer that never | |
| // queued a DKG message. | |
| if (queuedBytesPerNode.find(nodeId) == queuedBytesPerNode.end()) { | |
| return; | |
| } | |
| for (auto it = pendingMessages.begin(); it != pendingMessages.end();) { | |
| if (it->from == nodeId) { | |
| // Free the content-hash slot too; otherwise a reconnecting attacker | |
| // can grow seenMessages without bound even after payloads are dropped | |
| // (especially in observer mode where Clear() never runs). The hash is | |
| // stored alongside the payload, so no re-hashing happens here. | |
| it = EraseEntry(it); | |
| } else { | |
| ++it; | |
| } | |
| } | |
| } | |
| void CDKGPendingMessages::RemoveNode(NodeId nodeId) | |
| { | |
| // Own/local enqueues use from=-1 and are not tied to a peer disconnect. | |
| if (nodeId < 0) { | |
| return; | |
| } | |
| LOCK(cs_messages); | |
| // The delivery connection may only be relaying a message signed by another | |
| // quorum member. Release its connection-scoped quota, but retain the bounded | |
| // payload until normal processing or queue eviction removes it. | |
| messagesPerNode.erase(nodeId); | |
| } |
source: ['codex']
| @@ -50,7 +170,17 @@ std::list<CDKGPendingMessages::BinaryMessage> CDKGPendingMessages::PopPendingMes | |||
|
|
|||
| std::list<BinaryMessage> ret; | |||
| while (!pendingMessages.empty() && ret.size() < maxCount) { | |||
| ret.emplace_back(std::move(pendingMessages.front())); | |||
| auto& front = pendingMessages.front(); | |||
| ret.emplace_back(front.from, std::move(front.msg)); | |||
| // Popped messages are handed to the DKG session; their content hash stays | |||
| // in seenMessages so AlreadyHave() keeps suppressing re-requests. | |||
| if (front.from >= 0) { | |||
| pendingBytes -= front.bytes; | |||
| } | |||
| if (auto qit = queuedBytesPerNode.find(front.from); qit != queuedBytesPerNode.end()) { | |||
| qit->second -= front.bytes; | |||
| if (qit->second == 0) queuedBytesPerNode.erase(qit); | |||
| } | |||
| pendingMessages.pop_front(); | |||
There was a problem hiding this comment.
🔴 Blocking: Bound hashes retained after messages leave the byte-accounted queue
PopPendingMessages() removes a remote payload from pendingBytes and queuedBytesPerNode but deliberately leaves its hash in seenMessages. A later RemoveNode() then returns without touching that hash because the NodeId no longer has queued bytes, while also releasing the connection's count quota. An authenticated peer can repeatedly announce requested, unique, structurally valid messages that the active phase worker pops but rejects without a ban—for example, messages naming a known but non-current quorum base mapped to the same handler—and reconnect under fresh NodeIds. The payload-byte counter remains near zero while seenMessages grows without a parameter-derived bound until the handler's next DKG round calls Clear(). Track retained hashes under a separate bound, or remove rejected hashes after worker verification while relying on the session's accepted-object state for valid messages.
source: ['codex']
2 participants
Issue being fixed or feature implemented
v23.1.8 added param-derived limits for individual DKG messages, but aggregate pending-queue retention was still bounded only by
messagesPerNode[from] >= maxMessagesPerNode.NodeIdis an ephemeral per-connection identifier, and disconnecting a peer did not release its counter, queued payloads, or queuedseenMessageshashes.A peer could therefore reconnect under fresh
NodeIdvalues and repeatedly obtain a new quota while prior payloads remained resident. Observer-mode handlers are especially exposed because they enqueue DKG traffic but do not advance rounds and clear the queues.What was done?
develop, preserving the v23.1.8 per-message structural and size checks.How Has This Been Tested?
make -C src -j8 test/test_dash./src/test/test_dash --run_test=llmq_dkg_pending_tests(8 test cases)git diff upstream/develop...HEAD --checkThe focused tests cover per-node quotas, byte bounds across fresh
NodeIdvalues, oversized and empty payload rejection, duplicate replay behavior, byte-based eviction, local-message retention, and disconnect cleanup/accounting.Breaking Changes
None.
Checklist: