fix: drop and score P2P objects the peer was never asked for

[PastaPastaPasta]

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::ProcessNewChainLock short-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, since CLSIG sits on the quorum priority queue (IsQuorumPriorityMessage).

QFCOMMITMENT has the same shape. Most of the checks in CQuorumBlockProcessor::ProcessMessage return 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, what we asked for, and governance objects and votes already authorise incoming payloads against it. These three object types simply weren't doing so — the underlying ReceivedResponse was called and its return value discarded.

What was done?

Authorise three object types against the request tracker, dropping and scoring anything we have no in-flight GETDATA for.

The existing PeerConsumeObjectRequest is too weak for this. It resolves to TxRequestTracker::ReceivedResponse, which succeeds for any non-COMPLETED announcement — including the CANDIDATE created the moment an INV is processed. The transition to REQUESTED only happens later, when SendMessages schedules the GETDATA. A peer could therefore send INV(hash) immediately followed by the payload and authorise itself, which defeats the gate entirely; completing a sole candidate also erases it, so the per-peer announcement cap does not bound the repetition. This is reachable in production, not just in tests: sends for masternode connections are deliberately batched at ~100 ms intervals.

So this adds TxRequestTracker::ReceivedRequestedResponse, which succeeds only in the REQUESTED state, exposed as PeerConsumeGetDataResponse. On failure it leaves the announcement untouched, so a premature payload is rejected without suppressing the legitimate GETDATA that follows. Governance keeps the existing announced-or-requested semantics via PeerConsumeObjectRequest, which is unchanged.

The three call sites:

• CLSIG — net_processing.cpp. Authorised 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 previous PeerEraseObjectRequest call, which discarded the answer.
• QFCOMMITMENT — llmq/blockprocessor.cpp. Runs before any of the lookups above — the opposite placement from the DKG site, deliberately. There the pre-existing checks are cheap (MNAuth, size, structure) and gating last preserves their heavier penalties; here the pre-existing checks are the expensive part — block-index lookups and mineable-commitment probes under ::cs_main — so an unsolicited sender must be turned away before reaching them. (The gate takes ::cs_main too, but only for a tracker probe: a brief, bounded hold rather than chain-state traversal on the sender's behalf.) This knowingly softens the penalty for unsolicited garbage from 100 to 10 — such a peer now survives ten messages instead of one — but each of those messages is now a deserialize-hash-drop instead of chain lookups, and anything we actually requested still faces the full existing penalties. 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. Replaces the m_to_erase round-trip, which resolved to the weaker ReceivedResponse.

A shared UNREQUESTED_OBJECT_MISBEHAVIOR_SCORE{10} lives in msg_result.h, matching the existing "moderate" penalties nearby (mnauth from an unknown masternode, a DKG message from a non-verified peer, an invalid CLSIG).

Late and superseded answers are not misbehaviour

(The four commits addressing this are authored by UdjinM6, from review.)

A strict in-flight-only gate conflates an unsolicited push with an honest answer to a GETDATA of ours whose tracker entry is simply gone. Two routine paths remove it: expiry — a REQUESTED announcement past GetObjectInterval() (only 5s for MSG_CLSIG) is completed, and deleted outright when it was the sole announcement for its hash, the common case; and ForgetTxHash — accepting the object from any source (a ChainLock we signed ourselves, or one submitted over RPC) erases every peer's announcement, stranding an in-flight request. Neither is misbehaviour, but both would have been scored 10 — and since the score does not decay within a connection, a run of them discourages the peer's address. On the long-lived masternode connections these object types travel over, that is the wrong outcome.

So SendMessages additionally records, per peer, the GETDATA-only objects we actually sent a GETDATA for, and PeerConsumeGetDataResponse now reports REQUESTED, LATE or UNREQUESTED; only UNREQUESTED is scored, and LATE is processed as before. Only we ever write to that record, so a peer still cannot authorise its own payload by announcing it first. The record is bounded on three axes, each closing an abuse: consumption — an entry is erased by the answer it authorises, so one GETDATA buys exactly one accepted object and the induced payload cannot be replayed; time — entries age out after two request intervals of the requested type, so an unanswered request cannot be banked and redeemed later; type — both the match and the window come from the type we requested, never from the answer, so announcing a hash as MSG_CLSIG and answering with a DKG message is not authorised. The 256-entry per-peer LRU cap is only a memory ceiling: early eviction costs the grace for the oldest requests, never correctness.

The gated types are now named once in IsGetDataOnlyObject, together with why the other inv-driven types are excluded.

What the gate does not close

For the DKG messages and QFCOMMITMENT the gate is the substance of the fix: those payloads were retained in pending queues or driving block lookups before any cheap check could reject them, and requiring a GETDATA bounds that work outright.

For CLSIG the picture is narrower than the motivation above may suggest. The gate closes the push path, but a peer can still get itself solicited: announce the hash, and on non-masternode connections SendMessages runs after every processed message, so the GETDATA goes out almost immediately (masternode connections batch sends at ~100 ms, same outcome slightly later). A stale solicited CLSIG — at or below our best ChainLock — is then still processed and still costs the sender nothing, since ProcessNewChainLock's height check deliberately exits without penalty. So for the stale-CLSIG churn scenario specifically, the gate adds one INV and one GETDATA round-trip per blob rather than eliminating the free work; what it does guarantee is that nothing reaches the handler without our request, and that each request pays for exactly one payload. Closing the remainder means scoring stale solicited CLSIGs or rate-limiting per-peer CLSIG requests — both touch honest relay timing (a peer lagging behind us is normal) and are left as a follow-up.

Deliberately not gated

PLATFORMBAN looks like it belongs in this set — it is only ever pushed from ProcessGetData in-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.py covers 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 sent QSENDRECSIGS), MSG_DSQ (SENDDSQUEUE / WantsDSQ::ALL), ISDLOCK (pushed alongside MERKLEBLOCK for BIP37 clients), and SPORK (bulk push in reply to GETSPORKS). MSG_TX / MSG_DSTX keep 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 reaches ProcessNewChainLock, asserted via the handler's AlreadyHave (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 whose GETDATA is in flight gets through, and its score is asserted exactly, so the gate charging an authorised peer as well would fail the test.
• src/test/net_tests.cpp — peer_getdata_response_requires_an_inflight_request. A bare announcement is rejected and leaves the candidate intact; the same inv authorises exactly once after SendMessages has issued the GETDATA.
• src/test/llmq_chainlock_tests.cpp also covers the INV-then-payload race directly: announcing and immediately pushing the CLSIG is scored, and the identical payload is only accepted after SendMessages runs. It also gains an end-to-end replay: the same CLSIG, accepted once, is scored when sent again.
• test/functional/feature_llmq_dkg_intake.py — a well-formed QCONTRIB that passes every earlier check (verified sender, known quorum, size, structure) and is rejected purely for being unrequested.
• src/test/net_tests.cpp — four cases pinning the late-answer grace and its bounds: the expiry and ForgetTxHash paths each yield LATE for the first answer (asserting the tracker record is gone entirely, not merely COMPLETED) and UNREQUESTED for a second copy; a hash announced as MSG_CLSIG and answered as MSG_QUORUM_CONTRIB inside the window is not authorised; and an answer one second past the two-interval boundary is UNREQUESTED. The boundary cases are expressed in the CLSIG request-interval constants so they sit exactly on the edge — relaxing <= to < fails them.
• src/test/fuzz/txrequest.cpp — ReceivedRequestedResponse is modelled in the harness as a new command, asserting against the naive model that the call completes an announcement precisely when it was REQUESTED and leaves everything else, notably an INV-created CANDIDATE, untouched.

Both unit tests were mutation-checked: disabling the CLSIG gate fails 3 assertions, and relaxing ReceivedRequestedResponse back to the ReceivedResponse semantics fails 4.

Full make check passes. 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.py and lint-whitespace.py are clean.

Re-verified at the current head, i.e. with the four review follow-up commits included: full make check, feature_llmq_chainlocks.py, feature_llmq_dkg_intake.py and p2p_platform_ban.py all pass, the fuzz harness TU compiles, and the three lints above are clean.

p2p_platform_ban.py is what caught the PLATFORMBAN case: 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 (CLSIG since "Implement and enforce ChainLocks", 2019), so no released version pushes them unsolicited.

Third-party software that pushes these messages rather than waiting for our GETDATA 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:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone

🤖 Generated with Claude Code

[thepastaclaw]

🕓 Ready for review — 9 ahead in queue (commit 5c35e96)
Queue position: 10/12 · 2 reviews active
ETA: start ~06:39 UTC · complete ~07:00 UTC (median 20m across 30 recent reviews; 2 slots)
Queued 4h 11m ago · Last checked: 2026-07-29 05:00 UTC

[chatgpt-codex-connector]

💡 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".

[chatgpt-codex-connector]

Require an actual GETDATA before accepting object payloads

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 👍 / 👎.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 7ad7bb2d-685f-426b-8a8e-dc807469b95f

📥 Commits

Reviewing files that changed from the base of the PR and between d2228d5 and 5c35e96.

📒 Files selected for processing (5)

• src/Makefile.test.include
• src/test/llmq_blockprocessor_tests.cpp
• src/test/llmq_chainlock_tests.cpp
• src/test/util/net.cpp
• src/test/util/net.h

💤 Files with no reviewable changes (1)

• src/test/llmq_chainlock_tests.cpp

---

Walkthrough

Message handling now requires outstanding GETDATA consumption for quorum commitments, DKG payloads, and CLSIG messages. Unrequested objects are rejected and assigned a misbehavior score of 10. Recent request tracking distinguishes requested, late, and unrequested responses, while PlatformBan remains exempt. Tests cover request timing, unsolicited rejection, accepted CLSIG messages, replay handling, and DKG banscore changes.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Peer
  participant PeerManagerImpl
  participant TxRequestTracker
  participant LLMQHandler
  Peer->>PeerManagerImpl: Send CLSIG, DKG, or quorum object
  PeerManagerImpl->>TxRequestTracker: Consume matching GETDATA request
  TxRequestTracker-->>PeerManagerImpl: REQUESTED, LATE, or UNREQUESTED
  PeerManagerImpl->>LLMQHandler: Process accepted object
  PeerManagerImpl-->>Peer: Score and drop unrequested object

Loading

Possibly related PRs

• dashpay/dash#7314: Both changes modify the DKG message intake path with GETDATA authorization handling.

Suggested reviewers: udjinm6

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: rejecting and scoring unsolicited P2P objects.
Description check	✅ Passed	The description directly matches the implemented gating, scoring, and test coverage changes.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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

📥 Commits

Reviewing files that changed from the base of the PR and between 6d04c60 and 0ad2759.

📒 Files selected for processing (7)

• src/llmq/blockprocessor.cpp
• src/llmq/blockprocessor.h
• src/llmq/net_dkg.cpp
• src/msg_result.h
• src/net_processing.cpp
• src/test/llmq_chainlock_tests.cpp
• test/functional/feature_llmq_dkg_intake.py

[thepastaclaw]

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.

[thepastaclaw]

🔴 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']}

[thepastaclaw]

Resolved in this update — An INV announcement bypasses all three unsolicited-object gates 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.}

[thepastaclaw]

💬 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.

Suggested change

	* 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']}

[thepastaclaw]

Resolved in this update — Remove PLATFORMBAN from the GETDATA-only score documentation 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.}

[github-actions]

Potential PR merge conflicts

This 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 first

These open PRs will likely need a rebase:

• #7401: perf: deserialize DKG messages once Changed files: src/Makefile.test.include, src/llmq/net_dkg.cpp, test/functional/feature_llmq_dkg_intake.py.

If these PRs merge first

This PR will likely need a rebase:

• #7401: perf: deserialize DKG messages once Changed files: src/llmq/net_dkg.cpp, test/functional/feature_llmq_dkg_intake.py.

[thepastaclaw]

Final validation — Codex + Sonnet

This PR fixes a real DoS gap: three GETDATA-only P2P object types (CLSIG, QFCOMMITMENT, DKG push messages) were previously processed without confirming an in-flight request existed. The current head resolves the prior-review blocker by introducing TxRequestTracker::ReceivedRequestedResponse, which strictly requires State::REQUESTED and correctly rejects a bare INV-created CANDIDATE, closing the INV-then-payload bypass confirmed present at the prior reviewed SHA (d01d5f0). Two new Sonnet-only findings remain in scope: the QFCOMMITMENT gate lacks a dedicated regression test unlike its CLSIG/DKG siblings, and net_dkg.cpp uses UNREQUESTED_OBJECT_MISBEHAVIOR_SCORE via a transitive include rather than a direct one.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — dash-core-commit-history (completed)
• Verifier: claude-sonnet-5 — final-verifier
• Sonnet reviewers: claude-sonnet-5 — general (completed), claude-sonnet-5 — dash-core-commit-history (completed)

🟡 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/llmq/blockprocessor.cpp`:
- [SUGGESTION] src/llmq/blockprocessor.cpp:74-85: No dedicated test exercises the new QFCOMMITMENT solicitation gate
  The CLSIG gate got a focused unit test (unrequested_clsig_is_dropped_and_scored, including the INV-then-payload race) and the DKG gate got both a unit test (peer_getdata_response_requires_an_inflight_request) and a functional test (test_unrequested_rejected in feature_llmq_dkg_intake.py). The third call site — CQuorumBlockProcessor::ProcessMessage's new consume_request check at blockprocessor.cpp:82 — has no equivalent. A search of src/test/ and test/functional/ confirms no test constructs a CQuorumBlockProcessor and calls ProcessMessage with an unrequested QFCOMMITMENT, and no functional test sends a raw QFCOMMITMENT the node never asked for. net_tests.cpp's peer_getdata_response_requires_an_inflight_request exercises the shared tracker primitive via MSG_SPORK, which gives confidence in the primitive itself but not that the predicate is wired correctly at this call site (e.g. firing before rather than after the block/mineable-commitment lookups it's meant to gate, as the commit message claims). Given this touches consensus-adjacent llmq code, a small addition — a synthetic unrequested CFinalCommitment with a predicate returning false, asserting the MisbehavingError and that no lookups occurred — would close this gap.

[thepastaclaw]

🟡 Suggestion: No dedicated test exercises the new QFCOMMITMENT solicitation gate

The CLSIG gate got a focused unit test (unrequested_clsig_is_dropped_and_scored, including the INV-then-payload race) and the DKG gate got both a unit test (peer_getdata_response_requires_an_inflight_request) and a functional test (test_unrequested_rejected in feature_llmq_dkg_intake.py). The third call site — CQuorumBlockProcessor::ProcessMessage's new consume_request check at blockprocessor.cpp:82 — has no equivalent. A search of src/test/ and test/functional/ confirms no test constructs a CQuorumBlockProcessor and calls ProcessMessage with an unrequested QFCOMMITMENT, and no functional test sends a raw QFCOMMITMENT the node never asked for. net_tests.cpp's peer_getdata_response_requires_an_inflight_request exercises the shared tracker primitive via MSG_SPORK, which gives confidence in the primitive itself but not that the predicate is wired correctly at this call site (e.g. firing before rather than after the block/mineable-commitment lookups it's meant to gate, as the commit message claims). Given this touches consensus-adjacent llmq code, a small addition — a synthetic unrequested CFinalCommitment with a predicate returning false, asserting the MisbehavingError and that no lookups occurred — would close this gap.

_{source: ['claude']}

[UdjinM6]

pls see https://github.com/UdjinM6/dash/commits/claude/pr7484-followup/

[PastaPastaPasta]

pls see https://github.com/UdjinM6/dash/commits/claude/pr7484-followup/

Good catch!