Skip to content

fix: score peers requesting an unregistered LLMQType via QGETDATA - #7529

Open
PastaPastaPasta wants to merge 1 commit into
dashpay:developfrom
PastaPastaPasta:fix/qgetdata-invalid-llmqtype-score
Open

fix: score peers requesting an unregistered LLMQType via QGETDATA#7529
PastaPastaPasta wants to merge 1 commit into
dashpay:developfrom
PastaPastaPasta:fix/qgetdata-invalid-llmqtype-score

Conversation

@PastaPastaPasta

Copy link
Copy Markdown
Member

Issue being fixed or feature implemented

The QGETDATA handler validates the requested Consensus::LLMQType, but treats an unregistered one as a routine miss. sendQDATA groups QUORUM_TYPE_INVALID with QUORUM_BLOCK_NOT_FOUND, QUORUM_NOT_FOUND and MASTERNODE_IS_NO_MEMBER, all of which are scored only when the request limiter is also exceeded:

case (CQuorumDataRequest::Errors::QUORUM_TYPE_INVALID):
case (CQuorumDataRequest::Errors::QUORUM_BLOCK_NOT_FOUND):
case (CQuorumDataRequest::Errors::QUORUM_NOT_FOUND):
case (CQuorumDataRequest::Errors::MASTERNODE_IS_NO_MEMBER):
    misbehave = request_limit_exceeded;

That grouping is right for the others: a peer hits them innocently when it is ahead of us, or when we lack data it reasonably expected us to have. An unregistered LLMQ type is not in that category. No quorum of a type this chain does not register can exist, so there is nothing for a well-behaved peer to ask about, and every sibling handler already treats one as a protocol violation -- QSIGREC, the DKG messages and QFCOMMITMENT all score 100.

The practical exposure is small: QGETDATA already requires a ProRegTx-verified masternode or a qwatch connection, and each request passes through RegisterDataRequest, so the traffic is rate-limited even when unscored. This is a consistency fix, not a fix for a live attack.

What was done?

Send the QUORUM_TYPE_INVALID reply exactly as before, so a confused peer still learns why its request failed, then score the sender 100.

p2p_quorum_data.py exercises every error code on one connection. Its invalid-type case moves to the end of that block, because the peer is now dropped once it sends one; the test asserts the disconnect.

Spotted while reviewing #7516, which fixes an abort reachable through the same class of unvalidated LLMQType. This change is independent of it -- it does not depend on that PR and can land in either order.

How Has This Been Tested?

Built and run on macOS/arm64 against develop:

  • Full build clean.
  • p2p_quorum_data.py: passes.

Breaking Changes

A peer that requests quorum data for an LLMQ type this chain does not register is now scored to the ban threshold instead of being answered indefinitely. Only requests that were already answered with QUORUM_TYPE_INVALID are affected.

A new LLMQ type is introduced with a deployment, and no quorum of that type exists on chain before activation, so an honest peer has nothing to request until every node recognises the type. This should not misfire across a version boundary.

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

QGETDATA validated the type but grouped QUORUM_TYPE_INVALID with the misses a peer can hit innocently (QUORUM_BLOCK_NOT_FOUND, QUORUM_NOT_FOUND, MASTERNODE_IS_NO_MEMBER), which are only scored when the request limiter is also exceeded. Those can mean the peer is simply ahead of us; an unregistered type cannot, since no quorum of that type can exist on this chain. Every sibling handler already treats one as a protocol violation.

Send the error reply as before, then score 100. p2p_quorum_data.py moves the invalid-type case to the end of the error-code block, because the peer is now dropped once it sends one.
@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@PastaPastaPasta, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 28 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 100ed14a-5d5c-4b58-aec0-22f040e9d1ce

📥 Commits

Reviewing files that changed from the base of the PR and between c751ae4 and b05403e.

📒 Files selected for processing (2)
  • src/llmq/net_quorum.cpp
  • test/functional/p2p_quorum_data.py

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

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

@thepastaclaw

thepastaclaw commented Aug 3, 2026

Copy link
Copy Markdown

ℹ️ Review skipped (commit b05403e)
Last checked: 2026-08-03 02:50 UTC

@PastaPastaPasta PastaPastaPasta left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self utACK. this change makes sense to me; we should disconnect peers that send us purely invalid messages. If tests pass; merging this seems easy to me

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

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 these PRs merge first

This PR will likely need a rebase:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants