fix: drop and score P2P objects the peer was never asked for#7484
fix: drop and score P2P objects the peer was never asked for#7484PastaPastaPasta wants to merge 3 commits into
Conversation
|
⛔ Blockers found — Sonnet deferred (commit d01d5f0) |
![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: 0ad2759288
ℹ️ 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".
| // since every distinct signature blob hashes differently, an unsolicited peer could | ||
| // otherwise repeat that free work indefinitely. Consume after the spork gate so a | ||
| // CLSIG dropped while ChainLocks are disabled does not burn a later retransmit. | ||
| if (!WITH_LOCK(::cs_main, return PeerConsumeObjectRequest(pfrom.GetId(), clsig_inv))) { |
There was a problem hiding this comment.
When a malicious peer sends an INV, waits for it to be processed, and then pushes the matching payload before receiving any GETDATA, this condition accepts it without scoring: PeerConsumeObjectRequest delegates to TxRequestTracker::ReceivedResponse, which returns true for every non-COMPLETED announcement, including a merely CANDIDATE one (src/txrequest.cpp:660-669). Because completing the sole candidate also removes it, the peer can repeat this with unlimited distinct stale CLSIGs and still churn the seen cache for free; the same bypass applies to the new DKG and QFCOMMITMENT gates. Check specifically for the REQUESTED state rather than the existence of any announcement, and update the test that currently treats INV alone as authorization.
AGENTS.md reference: AGENTS.md:L160-L161
Useful? React with 👍 / 👎.
|
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 (7)
🚧 Files skipped from review as they are similar to previous changes (6)
WalkthroughMessage handling now requires outstanding request consumption for quorum commitments, DKG payloads, and CLSIG messages. Unrequested objects are rejected and assigned a misbehavior score of 10. Consumed requests are no longer erased separately. Platform ban messages remain exempt from solicitation checks. New tests cover unsolicited rejection, accepted announced CLSIG messages, replay handling, logging, and banscore changes. Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Peer
participant PeerManagerImpl
participant PeerRequestTracker
participant LLMQHandler
Peer->>PeerManagerImpl: Send CLSIG, DKG, or quorum object
PeerManagerImpl->>PeerRequestTracker: Consume matching object request
PeerRequestTracker-->>PeerManagerImpl: Accepted or unrequested
PeerManagerImpl->>LLMQHandler: Process accepted object
PeerManagerImpl-->>Peer: Record misbehavior and drop unrequested object
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
🤖 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/msg_result.h`:
- Around line 17-29: Correct the documentation for
UNREQUESTED_OBJECT_MISBEHAVIOR_SCORE by removing PLATFORMBAN from the list of
GETDATA-only object types and clarifying that it is intentionally handled
without a solicitation gate in ProcessPlatformBanMessage. Keep the existing
rationale for the remaining object types unchanged.
🪄 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 Plus
Run ID: f2dd58bc-43e8-4791-a61f-233f79889532
📒 Files selected for processing (7)
src/llmq/blockprocessor.cppsrc/llmq/blockprocessor.hsrc/llmq/net_dkg.cppsrc/msg_result.hsrc/net_processing.cppsrc/test/llmq_chainlock_tests.cpptest/functional/feature_llmq_dkg_intake.py
A CLSIG is only ever sent in reply to a GETDATA, so a peer that neither announced it nor was asked for it is sending it unsolicited. Until now such a message was processed anyway, and ProcessNewChainLock returns without any penalty for a CLSIG at or below our best ChainLock -- the height check short-circuits before verification, deliberately, to avoid a verification DoS. Since every distinct signature blob hashes differently, the seen-cache dedup above it never catches a varied blob, so a peer could repeat that free work indefinitely: churning the bounded seen cache and competing with real LLMQ traffic on the quorum priority queue. Gate the message on PeerConsumeObjectRequest, the same authorization source already used for governance objects and votes, and score the peer on failure. Also record why PLATFORMBAN is deliberately left ungated: it has no local ingress, so Dash Platform's push is always the first hop. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
QCONTRIB/QCOMPLAINT/QJUSTIFICATION/QPCOMMITMENT travel inv -> getdata only (see NetDKG::ProcessGetData and RelayInvToParticipants), so a pushed one was never asked for. Such a message was previously retained in the per-phase pending queue until a worker got around to verifying its signature. Gate on PeerConsumeObjectRequest before the message reaches the queues, replacing the PeerEraseObjectRequest call that discarded the same answer. The check is placed last, after the MNAuth, size and structural checks, so the existing rejection paths and their scores are unchanged; feature_llmq_dkg_intake.py gains a case for a well-formed but unrequested QCONTRIB. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
QFCOMMITMENT is only ever sent in reply to a GETDATA (commitments are announced via AddMineableCommitment -> PeerRelayInv), so a pushed one was never asked for. Most of the checks in ProcessMessage reject without scoring the peer -- deliberately, since we may just be lagging behind or on another chain -- so an unsolicited sender could repeat the block lookups and mineable-commitment probes indefinitely at no cost. Gate on the request tracker before any of that work, replacing the m_to_erase round-trip that resolved to the same ReceivedResponse call. CQuorumBlockProcessor holds no PeerManagerInternal reference (net_processing already includes this header, so taking one would make the dependency circular), so the check is passed in as a predicate from the call site. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The three new solicitation gates use a tracker operation that accepts an INV-created candidate before any GETDATA is sent, so an attacker can still reach the protected CLSIG, DKG, and quorum-commitment processing paths without being scored. The current CLSIG test explicitly exercises this bypass. The PR also contains a contradictory PLATFORMBAN comment and a corrective commit that should be folded into the commits it amends.
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)
🔴 1 blocking | 🟡 1 suggestion(s) | 💬 1 nitpick(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 `src/net_processing.cpp`:
- [BLOCKING] src/net_processing.cpp:5564: An INV announcement bypasses all three unsolicited-object gates
`PeerConsumeObjectRequest()` delegates to `TxRequestTracker::ReceivedResponse()`, which returns true for every non-`COMPLETED` entry, including the `CANDIDATE` created immediately when an INV is processed. The transition to `REQUESTED` only occurs later when `SendMessages()` schedules GETDATA. A peer can therefore send `INV(hash)` followed by the matching payload before GETDATA and pass the gate. The CLSIG test demonstrates exactly this path: it calls `AnnounceInv()` and then sends the CLSIG without calling `SendMessages()`, yet expects the object to reach `ProcessNewChainLock()`. This is reachable through the production scheduler as well because sends for masternode connections are intentionally skipped between most message-processing iterations and emitted only every 100 ms. The same predicate is used by the DKG gate at `src/llmq/net_dkg.cpp:490` and the QFCOMMITMENT gate at `src/llmq/blockprocessor.cpp:80`. Completing a sole candidate removes it from the tracker, so the per-peer announcement cap does not prevent repeating INV-plus-payload pairs. Add a distinct tracker operation that succeeds only for `REQUESTED` entries and use it for these GETDATA-only object types, while preserving the existing announcement-or-request semantics for governance. Tests should reject an INV immediately followed by a payload and accept it only after `SendMessages()` has issued GETDATA.
In `<commit:0ad2759288e>`:
- [SUGGESTION] <commit:0ad2759288e>:1: Fold the corrective follow-up into its originating commits
Commit `0ad2759288e` corrects the CLSIG test introduced by `b8977068872` and adds locking-contract documentation for the callback introduced by `40353a8ccbd`. These are amendments to two separate logical changes rather than one standalone change. Fold the test hunk into the CLSIG commit and the header hunk into the quorum-commitment commit so each originating commit is complete and the final history does not retain review-fix noise.
| // since every distinct signature blob hashes differently, an unsolicited peer could | ||
| // otherwise repeat that free work indefinitely. Consume after the spork gate so a | ||
| // CLSIG dropped while ChainLocks are disabled does not burn a later retransmit. | ||
| if (!WITH_LOCK(::cs_main, return PeerConsumeObjectRequest(pfrom.GetId(), clsig_inv))) { |
There was a problem hiding this comment.
🔴 Blocking: An INV announcement bypasses all three unsolicited-object gates
PeerConsumeObjectRequest() delegates to TxRequestTracker::ReceivedResponse(), which returns true for every non-COMPLETED entry, including the CANDIDATE created immediately when an INV is processed. The transition to REQUESTED only occurs later when SendMessages() schedules GETDATA. A peer can therefore send INV(hash) followed by the matching payload before GETDATA and pass the gate. The CLSIG test demonstrates exactly this path: it calls AnnounceInv() and then sends the CLSIG without calling SendMessages(), yet expects the object to reach ProcessNewChainLock(). This is reachable through the production scheduler as well because sends for masternode connections are intentionally skipped between most message-processing iterations and emitted only every 100 ms. The same predicate is used by the DKG gate at src/llmq/net_dkg.cpp:490 and the QFCOMMITMENT gate at src/llmq/blockprocessor.cpp:80. Completing a sole candidate removes it from the tracker, so the per-peer announcement cap does not prevent repeating INV-plus-payload pairs. Add a distinct tracker operation that succeeds only for REQUESTED entries and use it for these GETDATA-only object types, while preserving the existing announcement-or-request semantics for governance. Tests should reject an INV immediately followed by a payload and accept it only after SendMessages() has issued GETDATA.
source: ['codex']
| * Several object types (CLSIG, QFCOMMITMENT, the DKG messages, PLATFORMBAN) are only ever sent in | ||
| * reply to a GETDATA, so a message with no matching announcement is unsolicited by definition and | ||
| * is dropped before it can cost us any work. The score is deliberately tiny because the per-object | ||
| * request expires after GetObjectInterval() -- as little as 5s for MSG_CLSIG -- after which an | ||
| * honest peer answering our GETDATA late is indistinguishable from one that was never asked. At | ||
| * this score a peer is only discouraged after DISCOURAGEMENT_THRESHOLD such messages, which bounds | ||
| * the abuse a single connection can sustain without penalising ordinary lag. |
There was a problem hiding this comment.
💬 Nitpick: Remove PLATFORMBAN from the GETDATA-only score documentation
This comment says PLATFORMBAN is only sent in response to GETDATA and is dropped when unsolicited, but ProcessPlatformBanMessage() explicitly documents that its originating first hop is unsolicited by design and deliberately has no solicitation gate. The constant is not used for PLATFORMBAN. Document that carve-out here so future maintenance does not incorrectly add a gate that rejects legitimate platform ban ingress.
| * Several object types (CLSIG, QFCOMMITMENT, the DKG messages, PLATFORMBAN) are only ever sent in | |
| * reply to a GETDATA, so a message with no matching announcement is unsolicited by definition and | |
| * is dropped before it can cost us any work. The score is deliberately tiny because the per-object | |
| * request expires after GetObjectInterval() -- as little as 5s for MSG_CLSIG -- after which an | |
| * honest peer answering our GETDATA late is indistinguishable from one that was never asked. At | |
| * this score a peer is only discouraged after DISCOURAGEMENT_THRESHOLD such messages, which bounds | |
| * the abuse a single connection can sustain without penalising ordinary lag. | |
| * CLSIG, QFCOMMITMENT, and the DKG messages are expected only in reply to GETDATA and are | |
| * dropped when their solicitation check fails. PLATFORMBAN is excluded because its originating | |
| * first hop is unsolicited by design; see ProcessPlatformBanMessage(). The score is deliberately |
source: ['coderabbit']
0ad2759 to
d01d5f0
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:
|
2 participants
Issue being fixed or feature implemented
Several P2P object types travel
inv→getdata→ object and are never pushed on their own. When such a message arrives that we never asked for, we process it anyway, and for some of them the rejection path costs the sender nothing at all.The clearest case is
CLSIG.ChainlockHandler::ProcessNewChainLockshort-circuits with no misbehaviour penalty for any CLSIG at or below our best ChainLock — the height check runs before BLS verification, deliberately, to avoid a verification DoS. Because every distinct signature blob hashes differently, the seen-cache dedup above it never catches a varied blob, so a peer can repeat that work indefinitely for free: churning the (bounded) 1024-entry seen cache and competing with real LLMQ traffic, sinceCLSIGsits on the quorum priority queue (IsQuorumPriorityMessage).QFCOMMITMENThas the same shape. Most of the checks inCQuorumBlockProcessor::ProcessMessagereturn without scoring the peer — deliberately, since we may just be lagging behind or on another chain — so an unsolicited sender can drive the block lookups and mineable-commitment probes repeatedly at no cost. The DKG messages are retained in the per-phase pending queues until a worker gets round to verifying their signatures.The net layer already tracks, per peer, whether an object was announced or requested, and
PeerConsumeObjectRequestis documented as "an atomic did-we-ask-this-peer check". It was already used exactly this way for governance objects and votes. These object types simply weren't using it — the underlyingReceivedResponsewas called and its return value discarded.What was done?
Gate three object types on
PeerConsumeObjectRequest, dropping and scoring anything the peer neither announced nor was asked for:CLSIG—net_processing.cpp. Consumed after the spork-19 gate, so a CLSIG dropped while ChainLocks are disabled does not burn a later retransmit.QCONTRIB/QCOMPLAINT/QJUSTIFICATION/QPCOMMITMENT—llmq/net_dkg.cpp. The check is placed last, after the MNAuth, size and structural checks, so every existing rejection path and its score is unchanged. Replaces the previousPeerEraseObjectRequestcall, which discarded the same answer.QFCOMMITMENT—llmq/blockprocessor.cpp. Runs before any of the lookups above.CQuorumBlockProcessorholds noPeerManagerInternalreference (net_processingalready includes this header, so taking one would make the dependency circular), so the check is passed in as a predicate from the call site. Replaces them_to_eraseround-trip, which resolved to the sameReceivedResponsecall.A shared
UNREQUESTED_OBJECT_MISBEHAVIOR_SCORE{10}lives inmsg_result.h, matching the existing "moderate" penalties nearby (mnauthfrom an unknown masternode, a DKG message from a non-verified peer, an invalid CLSIG). It is moderate rather than fatal because a REQUESTED announcement expires afterGetObjectInterval()— only 5s forMSG_CLSIG— and is marked COMPLETED, after whichPeerConsumeObjectRequestreturns false, so a peer answering our GETDATA very late looks the same as one that was never asked. At 10 that peer would need ten such late answers on a single connection to be discouraged, which for CLSIG means sustained multi-minute lateness against a ~2.5 minute arrival rate.Deliberately not gated
PLATFORMBANlooks like it belongs in this set — it is only ever pushed fromProcessGetDatain-tree — but it has no local ingress: no RPC, no internal producer. The originating Dash Platform node injects a ban by pushing the P2P message straight to a Dash Core peer, so the first hop is always unsolicited by design.p2p_platform_ban.pycovers this. A comment at the handler records the reasoning so the omission does not read as an oversight.The remaining inv-driven types were considered and left alone because each has a legitimate unsolicited-push path that would need a carve-out rather than a plain gate:
QSIGREC(proactive relay to peers that sentQSENDRECSIGS),MSG_DSQ(SENDDSQUEUE/WantsDSQ::ALL),ISDLOCK(pushed alongsideMERKLEBLOCKfor BIP37 clients), andSPORK(bulk push in reply toGETSPORKS).MSG_TX/MSG_DSTXkeep upstream Bitcoin semantics.How Has This Been Tested?
Built with autotools on macOS (aarch64-apple-darwin).
New coverage:
src/test/llmq_chainlock_tests.cpp—unrequested_clsig_is_dropped_and_scored. An unsolicited peer's CLSIG never reachesProcessNewChainLock, asserted via the handler'sAlreadyHave(i.e. the hash was never recorded in the seen cache). It is sent twice and charged for both: without the gate the second copy would hit the seen-cache dedup and cost nothing, so this is what distinguishes the gate from the pre-existing invalid-CLSIG penalty. A peer that announced the inv first gets through, and its score is asserted exactly, so the gate charging an authorised peer as well would fail the test.test/functional/feature_llmq_dkg_intake.py— a well-formedQCONTRIBthat passes every earlier check (verified sender, known quorum, size, structure) and is rejected purely for being unrequested.The new unit test was mutation-checked: disabling the CLSIG gate fails 3 of its assertions.
Full
make checkpasses. Functional tests run:feature_llmq_chainlocks.py,feature_llmq_signing.py(both variants),feature_llmq_rotation.py,feature_llmq_is_cl_conflicts.py,feature_llmq_dkgerrors.py,feature_llmq_dkg_intake.py,rpc_verifychainlock.py,p2p_platform_ban.py— all pass.lint-circular-dependencies.py,lint-python.pyandlint-whitespace.pyare clean.p2p_platform_ban.pyis what caught thePLATFORMBANcase: it was gated in an earlier revision of this branch and the test failed, which is what prompted checking for a local ingress.Breaking Changes
None for peers running Dash Core. All three message types have been inv/getdata-only since they were introduced (
CLSIGsince "Implement and enforce ChainLocks", 2019), so no released version pushes them unsolicited.Third-party software that pushes these messages directly rather than announcing them first will now be scored 10 per message and disconnected after ten of them. No such producer exists for these three types — that property is exactly what separates them from
PLATFORMBAN.Checklist:
🤖 Generated with Claude Code