fix: bound QGETDATA request tracking and reject requester-supplied nError - #7519
fix: bound QGETDATA request tracking and reject requester-supplied nError#7519PastaPastaPasta wants to merge 1 commit into
Conversation
WalkthroughQGETDATA messages now carry an optional response error byte. NetQuorum rejects attacker-supplied errors and validates quorum data before request registration. CQuorumManager applies a 64-entry live-request limit per peer identity and tracks cleanup. Local registration checks now require a successful optional result. Unit and functional tests cover error scoring, repeated requests, capacity limits, identity isolation, and both cleanup modes. Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Peer
participant NetQuorum
participant CQuorumManager
Peer->>NetQuorum: Send QGETDATA
NetQuorum->>NetQuorum: Validate error, quorum type, and block hash
NetQuorum->>CQuorumManager: RegisterDataRequest
CQuorumManager-->>NetQuorum: Return registration result
NetQuorum-->>Peer: Return error or quorum data response
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 |
|
🔍 Review in progress — actively reviewing now (commit d8dc7d5) |
![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: 9461b8b97a
ℹ️ 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".
| // Cheap gates first. IsQuorumActive only scans the small cached set of recent | ||
| // quorums (keepOldConnections). GetQuorum, by contrast, rebuilds arbitrary | ||
| // historical mined commitments (DMN list replay + member selection) on a cache | ||
| // miss — do not let an unsolicited QSIGREC force that work for inactive hashes. | ||
| // Caller (NetSigning) has already rejected unknown llmq types. | ||
| if (!IsQuorumActive(llmq_type, qman, quorum_hash)) { |
There was a problem hiding this comment.
This reorders recovered-signature validation in VerifyAndProcessRecoveredSig, but the commit is scoped and tested as a QGETDATA fix. If the QGETDATA mitigation later needs to be cherry-picked or reverted, this independent QSIGREC behavior change must travel with it despite having no dedicated regression test, making a sensitive LLMQ signing-path change harder to validate and maintain. Move this hunk to a separate commit with focused coverage.
AGENTS.md reference: AGENTS.md:L13-L14
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The final implementation correctly rejects requester-supplied QGETDATA errors, bounds inbound tracking entries per identity, and validates cheap fields before expensive quorum work; the relevant call sites and tests support the intended behavior. No code-correctness blocker remains, but two commit-history issues should be cleaned up so the preserved stack stays bisectable and avoids add-then-remove blame noise.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (completed),claude-sonnet-5— general (completed)
🟡 2 suggestion(s)
2 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 `<commit:044055168c2>`:
- [SUGGESTION] <commit:044055168c2>:1: Keep the regression-test commit green
Commit 044055168c2 deliberately precedes the implementation while adding unit and functional tests that require a misbehavior score of 10. At that revision, src/llmq/net_quorum.cpp has no early nError rejection, so the new unit test observes 0 and fails exactly as the commit message and PR description acknowledge; the functional case likewise waits for a score the handler cannot produce. This leaves a knowingly red permanent revision and an avoidable bisect trap. Squash these tests into bed4f81137a, or place the test commit after the fix, so every preserved commit has internally consistent implementation and test expectations.
In `<commit:bed4f81137a>`:
- [SUGGESTION] <commit:bed4f81137a>:1: Fold transient corrections into the commit that introduced them
Commit bed4f81137a introduces two PeerMisbehaving branches whose sendQDATA calls hardcode request_limit_exceeded=false, making those branches unreachable, and duplicates the recovered-signature deduplication/backpressure block before and after GetQuorum. Commit 9461b8b97a1 immediately removes those exact branches and folds the duplicated block back into one, explicitly describing them as dead guards and unnecessary duplication. Because neither intermediate construct was intended to survive and the commits have not shipped independently, fix up those corrective hunks into bed4f81137a. The durable per-identity cap and its tests can remain in 9461b8b97a1 as a separate hardening change.
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:
If these PRs merge firstThis PR will likely need a rebase:
|
…rror CQuorumDataRequest deserializes an optional trailing nError byte. It is a response-only field that writers skip when undefined, but the QGETDATA handler read it back and passed it to sendQDATA, where QUORUM_VERIFICATION_VECTOR_MISSING and ENCRYPTED_CONTRIBUTIONS_MISSING deliberately skip the request-limit misbehaviour score. A requester supplying its own error byte therefore evaded the score-25 ban while still forcing verification-vector serialization and a LevelDB read on every repeat. Reject inbound requests carrying an error instead; honest senders never emit one. Separately, RegisterDataRequest ran before validation, so every QGETDATA inserted a mapQuorumDataRequests entry keyed on an attacker-chosen quorumHash. A fresh hash is never already pending, so the rate limit never fired and never bounded the map, and entries are reaped only from CleanupExpiredDataRequests via UpdatedBlockTip (skipped during IBD or when unsynced). Move registration after validation and add a per-identity cap so a new key consumes tracking budget; re-requests of an existing key still fall through to the rate limit. Reachability is broader than the masternode-only gate suggests: qwatch is set by a bare QWATCH message from any peer, so one connection from an unauthenticated peer was enough to grow the map without bound.
9461b8b to
d8dc7d5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8dc7d5efe
ℹ️ 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".
| //! Returns nullopt when a peer-initiated request would exceed that identity's tracking | ||
| //! budget, true when the entry was created or refreshed, and false when an unexpired entry | ||
| //! already exists (the rate limit applies). | ||
| std::optional<bool> RegisterDataRequest(const CQuorumDataRequestKey& key, const CQuorumDataRequest& request, |
There was a problem hiding this comment.
When quorum getdata is invoked again for the same peer and quorum before the request expires, this method now returns std::optional<bool>{false}. The unchanged caller in src/rpc/quorums.cpp:935 applies ! to the optional itself, which tests whether it is engaged rather than its contained value, so it proceeds to send the duplicate QGETDATA and reports success. The responder then treats that duplicate as rate-limit abuse and scores this node; repeated RPC calls can ultimately disconnect it. Update that caller to inspect the contained boolean just as RequestQuorumData now does.
Useful? React with 👍 / 👎.
![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/llmq/quorumsman.h`:
- Around line 151-157: Update the RPC flow around RegisterDataRequest to require
an explicit std::optional<bool>{true} result before calling PushMessage.
Preserve the existing failure return path, ensuring nullopt and false both
prevent sending QGETDATA.
🪄 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: ff92a1d2-4129-4e1a-b9f5-dce119ac58f9
📒 Files selected for processing (7)
src/Makefile.test.includesrc/llmq/net_quorum.cppsrc/llmq/quorumsman.cppsrc/llmq/quorumsman.hsrc/test/llmq_qgetdata_tests.cpptest/functional/p2p_quorum_data.pytest/functional/test_framework/messages.py
| //! Request tracking for QGETDATA/QDATA — used by NetQuorum and RPC. | ||
| //! Returns nullopt when a peer-initiated request would exceed that identity's tracking | ||
| //! budget, true when the entry was created or refreshed, and false when an unexpired entry | ||
| //! already exists (the rate limit applies). | ||
| std::optional<bool> RegisterDataRequest(const CQuorumDataRequestKey& key, const CQuorumDataRequest& request, | ||
| bool add_expiry_bias = true) const | ||
| EXCLUSIVE_LOCKS_REQUIRED(!cs_data_requests); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find all callers of CQuorumManager::RegisterDataRequest to confirm they handle std::optional<bool>.
rg -nP -C5 '\bRegisterDataRequest\s*\(' --type=cppRepository: dashpay/dash
Length of output: 150
🏁 Script executed:
#!/bin/bash
set -u
echo "Tracked files matching quorumsman.h name:"
git ls-files | rg '(^|/)quorumsman\.h$|src/llmq/net_quorum\.cpp$|src/llmq/.*quorum.*data.*|quorums.*data|quorum.*data' || true
echo
echo "Search all tracked files for RegisterDataRequest and CQuorumDataRequestKey:"
rg -n -C4 '\bRegisterDataRequest\s*\(|\bCQuorumDataRequestKey\b' || true
echo
echo "Locate RPC files mentioning QGETDATA/QDATA or CQuorumDataRequest:"
rg -n -C3 '\b(QGETDATA|QDATA|CQuorumDataRequest|quorumdata)\b' --glob '*.{cpp,h,json,py,js}' . || true
echo
echo "Current CQuorumManager declaration context if available:"
file="$(git ls-files | rg '(^|/)quorumsman\.h$' | head -n1)"
if [ -n "$file" ]; then
wc -l "$file"
sed -n '1,260p' "$file" | nl -ba
fiRepository: dashpay/dash
Length of output: 350
🏁 Script executed:
#!/bin/bash
set -u
echo "Exact identifier search:"
python3 - <<'PY'
from pathlib import Path
for p in Path('.').rglob('*'):
if p.is_file() and not any(part in {'.git','build','vendor','node_modules'} for part in p.parts):
try:
s=p.read_text(errors='ignore')
except Exception:
continue
if 'RegisterDataRequest' in s or 'CQuorumDataRequestKey' in s:
print(p)
for i,line in enumerate(s.splitlines(),1):
if 'RegisterDataRequest' in line or 'CQuorumDataRequestKey' in line:
print(f" {i}: {line}")
PY
echo
echo "NetQuorum source relevant functions if present:"
sed -n '1,520p' src/llmq/net_quorum.cpp | nl -ba | sed -n '110,170p;328,365p'
echo
echo "Any QGETDATA/QDATA/CQuorumDataRequest mentions:"
python3 - <<'PY'
from pathlib import Path
terms = ['QGETDATA','QDATA','CQuorumDataRequest','quorumdata']
for p in Path('.').rglob('*'):
if p.is_file() and p.suffix in {'.cpp','.h','.py','.json',''}:
try:
s=p.read_text(errors='ignore').splitlines()
except Exception:
continue
hits=[(i,line) for i,line in enumerate(s,1) if any(t in line for t in terms)]
if hits:
print(p)
for i,line in hits[:40]:
print(f" {i}: {line[:220]}")
PYRepository: dashpay/dash
Length of output: 15154
Fix the RPC call to use std::optional<bool> semantics.
src/rpc/quorums.cpp:935 still treats RegisterDataRequest as returning bool: if (!llmq_ctx.qman->RegisterDataRequest(key, request)) return false;. This misinterprets the nullopt case and can send a QGETDATA message while the request budget is exceeded.
Use the NetQuorum RPC pattern with RegisterDataRequest(key, request) == std::optional<bool>{true} before PushMessage(...).
🤖 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/quorumsman.h` around lines 151 - 157, Update the RPC flow around
RegisterDataRequest to require an explicit std::optional<bool>{true} result
before calling PushMessage. Preserve the existing failure return path, ensuring
nullopt and false both prevent sending QGETDATA.
| if (request.GetError() != CQuorumDataRequest::Errors::UNDEFINED) { | ||
| m_peer_manager->PeerMisbehaving(pfrom.GetId(), 10, "qgetdata with error field"); | ||
| return; | ||
| } |
There was a problem hiding this comment.
hmm; why only 10...?
2 participants
Issue being fixed or feature implemented
Two defects in the
QGETDATAbranch ofNetQuorum::ProcessMessage.1. Requester-supplied
nErrorsuppressed the ban.CQuorumDataRequest's serialisation reads an optional trailingnErrorbyte. It is a response-only field — writers skip it when undefined — but the request handler read it back and passed it tosendQDATA. In that switch,QUORUM_VERIFICATION_VECTOR_MISSINGandENCRYPTED_CONTRIBUTIONS_MISSINGdeliberately skip the "request limit exceeded" misbehaviour score, so a requester that supplied its own error byte evaded the score-25 ban while still forcing verification-vector serialisation and a LevelDB read on every repeat.2. Registration happened before validation.
RegisterDataRequestwas called before any validation, so everyQGETDATAinserted amapQuorumDataRequestsentry keyed on an attacker-chosenquorumHash. A fresh hash is never "already pending", so the rate limit never fired and never bounded the map. Entries live 300+60 s and are reaped only fromCleanupExpiredDataRequestsviaUpdatedBlockTip, which is skipped during IBD or when unsynced.Reachability is broader than it first appears: the handler requires the victim to be a masternode and the peer to be either MNAuth-verified or
qwatch, butqwatchis set by a bareQWATCHmessage from any peer. A single TCP connection from an unauthenticated peer is therefore enough to grow the map without bound.What was done?
QGETDATAcarrying a non-undefinednError, scored the same as the sibling malformed-QGETDATA case directly above it.MAX_INBOUND_DATA_REQUESTSlive entries. The reordering alone does not bound the map: the key still contains aquorumHashconstrained only to some block in our index, so each fresh hash is by construction never "already pending" and the rate limit never engages. The cap is what actually bounds it.The budget check lives inside the existing
RegisterDataRequestrather than in a parallelRegisterInboundDataRequest. An earlier revision of this PR added a near-verbatim copy of that function, which is the same bug guarded twice; the single function now returnsstd::optional<bool>—nulloptfor budget exhausted,truefor created or refreshed,falsefor the existing rate-limit result.Also removed from this PR: an unrelated QSIGREC reordering in
src/llmq/signing.cpp, now filed separately as #7531, and a hunk that rewrote theret_errcomputation into a behaviourally identical form.Known remaining gap: all
qwatchpeers share the null-proRegTx budget, so one attacker can deny that budget to every legitimate watch peer for up to 360 s — previously this was only a soft score. Because the budget is released only byCleanupExpiredDataRequests, a stalled tip or IBD extends that. Splitting the budget per peer rather than per identity is left as follow-up.How Has This Been Tested?
src/test/llmq_qgetdata_tests.cppcovers thenErrorbypass, the per-identity budget exhausting and turning into a misbehaviour score rather than response work, and that a separate identity keeps its own budget so one peer cannot starve another.Built locally and
test_dash --run_test=llmq_qgetdata_testspasses (4 cases). Full validation is delegated to CI.Breaking Changes
None.
nErroron an inbound request was never meaningful.Checklist: