fix: bound pending DKG message retention per proTxHash - #7583
Conversation
The DKG pending-message retention quota was keyed by NodeId, which resets on reconnect: a single masternode identity could retain an unbounded number of pending messages across reconnects. Key the quota by the sender's proTxHash instead, which survives reconnects and is pinned to registered masternode identities by the MNAuth gate (develop already rejects pushed DKG messages from peers without a verified proRegTxHash). The quota is cumulative for the round and not refunded on pop, so draining the queue does not regain retention slots. Also check the duplicate-hash set before charging the quota so resent hashes don't burn budget, and do not mark a quota-dropped hash as seen so another peer with remaining budget can re-deliver it. Locally produced messages (from=-1) are enqueued under this node's own proTxHash and charged like any other sender's. Extracted from the DKG intake redesign in dashpay#7557; the deserialize-once/framing changes there are deliberately not included.
|
🔍 Review in progress — actively reviewing now (commit cf85283) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DKGPeer
participant ProcessMessage
participant CDKGPendingMessages
participant DKGSession
DKGPeer->>ProcessMessage: send authenticated DKG message
ProcessMessage->>CDKGPendingMessages: enqueue payload with sender proTxHash
DKGSession->>CDKGPendingMessages: enqueue locally generated message with session proTxHash
CDKGPendingMessages->>CDKGPendingMessages: reject duplicate or over-quota hash
CDKGPendingMessages-->>DKGSession: retain accepted message
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 40-44: Update the sender_protx validation in the DKG session
handler to use a direct null check and return immediately for a null identity,
avoiding Assume/assertion_fail abort behavior. Add a unit test covering a null
sender_protx and confirming the request is dropped without aborting.
In `@src/test/llmq_dkg_tests.cpp`:
- Around line 109-114: Add a third sender with available quota in the
pending-message test, submit hash5 through that sender after confirming
protx_b’s quota rejection, and assert that hash5 is retained/enqueued. Update
the expected PopPendingMessages(5) count to include the successfully accepted
redelivery while preserving the existing HasSeen assertions for the rejected
attempt.
🪄 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: 89d4a6d7-efef-4f73-a157-b36a14eff395
📒 Files selected for processing (5)
src/llmq/dkgsessionhandler.cppsrc/llmq/dkgsessionhandler.hsrc/llmq/net_dkg.cppsrc/test/llmq_dkg_tests.cpptest/functional/feature_llmq_dkg_intake.py
PastaPastaPasta
left a comment
There was a problem hiding this comment.
LGTM; this simplifies the remaining / other PR
Unit tests: the quota is charged per proTxHash (surviving NodeId changes and not refunded on pop), duplicates are rejected before charging the quota, distinct proTxHashes have independent budgets, and locally produced messages share the quota path. Functional test: extend feature_llmq_dkg_intake.py with a late-message scenario proving that a masternode identity reconnecting under fresh NodeIds cannot retain more than maxMessagesPerProTx contributions, that quota drops are silent (banscore stays 0), that a distinct proTx keeps its own budget, and that round-start clearing discards retained messages without them ever reaching a worker. Ported from dashpay#7557, adapted to the pre-framing intake (well-formed zero-BLS payloads instead of BLS-invalid ones, since develop still deserializes a copy at intake).
5e46608 to
cf85283
Compare
Issue being fixed or feature implemented
The DKG pending-message retention quota in
CDKGPendingMessagesis keyed by NodeId, which resets on every reconnect. A single masternode identity can therefore retain an effectively unbounded number of pending DKG messages on a victim masternode by reconnecting between sends.This extracts the DoS-relevant part of #7557 so it can ship in the release; the deserialize-once/framing intake redesign remains in #7557 for post-RC.
What was done?
PopAndDeserializeMessagesand the framing/deserialize-once changes from perf: deserialize DKG messages once (framing-only intake) #7557 are intentionally not included.How Has This Been Tested?
llmq_dkg_tests: quota survives NodeId changes, quota is per-proTx, duplicates are rejected before charging the quota, own-message enqueue path shares the quota.feature_llmq_dkg_intake.pywith a late-message scenario: a reconnecting identity cannot exceed its budget across2*llmq_sizefresh NodeIds, over-quota drops are silent (banscore 0), a distinct proTx has its own budget, and round-start clearing discards retained messages without scoring.make -j5,test_dash --run_test=llmq_dkg_tests,test_runner.py feature_llmq_dkg_intake.py,lint-python,lint-whitespace.Breaking Changes
None. Honest masternodes send one message per type per round, far under the unchanged
size * 2budget; the only behavior change is that a reconnecting identity can no longer refill its retention quota.Checklist: