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 4 commits into
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughQGETDATA messages now support an optional requester-supplied error byte. NetQuorum rejects such errors and validates quorum types and block hashes before registering requests. Invalid types receive score 100, unknown blocks receive score 10, and repeated valid requests follow the existing rate limit with score 25. Unit and functional tests cover these paths and are included in the test binary. Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
⛔ Blockers found — Opus deferred (commit 1b7054d) |
![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.
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
| 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...?
|
This pull request has conflicts, please rebase. |
d8dc7d5 to
df6347b
Compare
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/test/llmq_qgetdata_tests.cpp`:
- Around line 126-146: Extend
BOOST_AUTO_TEST_CASE(qgetdata_rejects_attacker_supplied_error) with a separate
QGETDATA request using a zero-valued trailing error byte, processed by a fresh
peer initialized through m_node.peerman. Assert that this request also receives
a misbehavior score of 100, ensuring zero is rejected rather than treated as
Errors::NONE.
In `@test/functional/test_framework/messages.py`:
- Around line 2598-2604: Fix the hanging indentation in msg_qgetdata.__repr__ by
aligning the tuple arguments under the opening parenthesis of the return
expression, eliminating the E126 violation while preserving the formatting and
output.
🪄 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: e05710be-aa3d-4a3e-8edf-9a298d4a8919
📒 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
🚧 Files skipped from review as they are similar to previous changes (5)
- src/Makefile.test.include
- test/functional/p2p_quorum_data.py
- src/llmq/net_quorum.cpp
- src/llmq/quorumsman.cpp
- src/llmq/quorumsman.h
| BOOST_AUTO_TEST_CASE(qgetdata_rejects_attacker_supplied_error) | ||
| { | ||
| LOCK(NetEventsInterface::g_msgproc_mutex); | ||
|
|
||
| auto peer{MakePeer(/*id=*/1)}; | ||
| m_node.peerman->InitializeNode(*peer, NODE_NETWORK); | ||
| AssertMisbehaviorScore(*m_node.peerman, *peer, 0); | ||
|
|
||
| // Any known LLMQ type is fine; the nError check must fire before body work. | ||
| const uint256 quorum_hash{uint256S("0x11")}; | ||
| const uint256 protx_hash{uint256S("0x22")}; | ||
| auto stream = MakeQGetDataStream(Consensus::LLMQType::LLMQ_TEST, quorum_hash, | ||
| CQuorumDataRequest::QUORUM_VERIFICATION_VECTOR, protx_hash, | ||
| /*error_byte=*/static_cast<uint8_t>( | ||
| CQuorumDataRequest::Errors::ENCRYPTED_CONTRIBUTIONS_MISSING)); | ||
|
|
||
| m_net_quorum->ProcessMessage(*peer, NetMsgType::QGETDATA, stream); | ||
|
|
||
| // Pre-fix: score stays 0 (nError steers the ban decision; no score applied). | ||
| // Post-fix: a request we could never have produced is scored in full. | ||
| AssertMisbehaviorScore(*m_node.peerman, *peer, 100); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test a zero-valued trailing error byte.
This test covers only a nonzero nError value. An implementation that rejects only nError != NONE would pass this test but still accept a requester-supplied zero byte. Add a separate request with a zero-valued trailing byte, a fresh peer, and an expected misbehavior score of 100.
🤖 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/test/llmq_qgetdata_tests.cpp` around lines 126 - 146, Extend
BOOST_AUTO_TEST_CASE(qgetdata_rejects_attacker_supplied_error) with a separate
QGETDATA request using a zero-valued trailing error byte, processed by a fresh
peer initialized through m_node.peerman. Assert that this request also receives
a misbehavior score of 100, ensuring zero is rejected rather than treated as
Errors::NONE.
Source: Coding guidelines
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Both previously published commit-history suggestions are fixed by rewriting the PR as a single commit containing the final implementation and its regression tests; no prior finding remains carried forward. One genuinely new blocking issue exists in the assigned delta: the consolidated tri-state request-registration API was not handled correctly by the quorum getdata RPC caller, allowing duplicate requests to be sent and valid responses to trigger peer penalties.
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 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/rpc/quorums.cpp`:
- [BLOCKING] src/rpc/quorums.cpp:935: Adapt quorum getdata to the tri-state registration result
RegisterDataRequest now returns std::optional<bool>, but applying operator! checks whether the optional is engaged rather than negating its contained value. An unexpired duplicate returns std::optional<bool>{false}, so this condition permits the RPC to send another QGETDATA while the original request remains tracked. A second request with a different data mask or proTxHash then causes the responder's valid QDATA to be classified as Mismatch against the stale request; an identical request can be classified as AlreadyReceived after the first response is processed. In either case, the honest responder receives a misbehavior score of 10. Require the explicit successful state, matching NetQuorum::RequestQuorumData.
…rum on QSIGREC 5619e24 refactor: trim comment density in VerifyAndProcessRecoveredSig (pasta) dcdc0fa perf(llmq): check quorum activity before materializing the quorum on QSIGREC (pasta) Pull request description: ## Issue being fixed or feature implemented `CSigningManager::VerifyAndProcessRecoveredSig` called `qman.GetQuorum(...)` before `IsQuorumActive(...)`. Those two have very different costs. `IsQuorumActive` is bounded to the `keepOldConnections` most recent quorums at the tip, and that set is shared and cached across callers. `GetQuorum` takes the peer-supplied hash and can rebuild an arbitrary historical mined commitment on a cache miss — a deterministic masternode list replay plus member selection. An unsolicited `QSIGREC` naming an inactive quorum hash therefore forced the expensive path before the cheap gate had a chance to reject it. This was split out of [#7519](#7519), where it had been bundled with unrelated QGETDATA work. ## What was done? Swap the order so the cheap gate runs first. Once `IsQuorumActive` passes, the hash is one of the recent quorums `ScanQuorums` covers, so the subsequent `GetQuorum` is usually served from cache. `ScanQuorums` and `GetQuorum` keep separate LRUs, so a rebuild there is still possible, but only of a recent quorum — never of the arbitrary historical one a peer could otherwise name. A null quorum after that point is no longer peer-controlled — it means the quorum was reported active but is no longer materializable, e.g. after a reorg — so it is logged without a misbehaviour score. The caller (`NetSigning`) has already rejected unknown LLMQ types before this point, so the reordering does not widen what reaches `IsQuorumActive`. ## How Has This Been Tested? Compiles cleanly. This is a reordering of two existing checks with no behavioural change for valid input, so it is covered by the existing QSIGREC paths in the functional suite. Full validation is delegated to CI. ## Breaking Changes None. ## Checklist: - [x] I have performed a self-review of my own code - [x] 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 Top commit has no ACKs. Tree-SHA512: dd799dae655f6513ea2ba809e4b832c3f12b538d1121e5081b48f155d3601705b324f82f2725c8581155ae889be3ac20b2720111dbb747818c6eac630331539f
|
This pull request has conflicts, please rebase. |
df6347b to
c768aeb
Compare
…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, scored 100: honest senders never emit one, so such a request is not one we could ever have produced and there is no run of them worth tolerating. 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.
c768aeb to
f38eb77
Compare
Drop the essay-length rationale around the nError reject, pre-tracking checks, and budget exhaustion path. Keep the non-obvious invariants only.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The QGETDATA hardening is correct at the reviewed head, and the prior tri-state RPC caller defect is fixed by requiring an explicit successful registration result. Two non-blocking improvements remain: cover the zero-valued trailing error-byte case and squash the follow-up comment-only cleanup into the commit that introduced the comments.
Source: reviewer backends gpt-5.6-sol (general) and gpt-5.6-sol (dash-core-commit-history); final verifier backend gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
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/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 2 suggestion(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/test/llmq_qgetdata_tests.cpp`:
- [SUGGESTION] src/test/llmq_qgetdata_tests.cpp:126-146: Cover a zero-valued trailing error byte
The test only sends `ENCRYPTED_CONTRIBUTIONS_MISSING`, so it does not establish the protocol invariant that the presence of any trailing `nError` byte is invalid independently of its value. An implementation that rejects nonzero errors but accepts an explicitly serialized `Errors::NONE` byte would satisfy this test while violating the PR's stated behavior. Add a request carrying `static_cast<uint8_t>(CQuorumDataRequest::Errors::NONE)` through a fresh initialized peer and assert a misbehavior score of 100.
In `<commit:359bdcf0a9a>`:
- [SUGGESTION] <commit:359bdcf0a9a>:1: Fold the comment cleanup into the primary fix
Commit 359bdcf0a9a only shortens comments introduced immediately beforehand by f38eb770003 in the same file. Preserving both commits creates avoidable add-then-remove blame noise without providing an independently useful revision. Squash 359bdcf0a9a into f38eb770003 so the primary fix lands with its intended final comments.
| BOOST_AUTO_TEST_CASE(qgetdata_rejects_attacker_supplied_error) | ||
| { | ||
| LOCK(NetEventsInterface::g_msgproc_mutex); | ||
|
|
||
| auto peer{MakePeer(/*id=*/1)}; | ||
| m_node.peerman->InitializeNode(*peer, NODE_NETWORK); | ||
| AssertMisbehaviorScore(*m_node.peerman, *peer, 0); | ||
|
|
||
| // Any known LLMQ type is fine; the nError check must fire before body work. | ||
| const uint256 quorum_hash{uint256S("0x11")}; | ||
| const uint256 protx_hash{uint256S("0x22")}; | ||
| auto stream = MakeQGetDataStream(Consensus::LLMQType::LLMQ_TEST, quorum_hash, | ||
| CQuorumDataRequest::QUORUM_VERIFICATION_VECTOR, protx_hash, | ||
| /*error_byte=*/static_cast<uint8_t>( | ||
| CQuorumDataRequest::Errors::ENCRYPTED_CONTRIBUTIONS_MISSING)); | ||
|
|
||
| m_net_quorum->ProcessMessage(*peer, NetMsgType::QGETDATA, stream); | ||
|
|
||
| // Pre-fix: score stays 0 (nError steers the ban decision; no score applied). | ||
| // Post-fix: a request we could never have produced is scored in full. | ||
| AssertMisbehaviorScore(*m_node.peerman, *peer, 100); |
There was a problem hiding this comment.
🟡 Suggestion: Cover a zero-valued trailing error byte
The test only sends ENCRYPTED_CONTRIBUTIONS_MISSING, so it does not establish the protocol invariant that the presence of any trailing nError byte is invalid independently of its value. An implementation that rejects nonzero errors but accepts an explicitly serialized Errors::NONE byte would satisfy this test while violating the PR's stated behavior. Add a request carrying static_cast<uint8_t>(CQuorumDataRequest::Errors::NONE) through a fresh initialized peer and assert a misbehavior score of 100.
source: ['coderabbit']
Unknown-block QGETDATA is no longer a free probe: score +10 and still skip tracking so random hashes cannot grow mapQuorumDataRequests. Remove MAX_INBOUND_DATA_REQUESTS / m_inbound_request_counts and restore RegisterDataRequest to bool. Keep the existing per-key rate limit and QUORUM_NOT_FOUND scoring unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be44542d6f
ℹ️ 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".
| const CQuorumDataRequestKey key(pfrom.GetVerifiedProRegTxHash(), false, request.GetQuorumHash(), request.GetLLMQType()); | ||
| const bool request_limit_exceeded = !m_qman.RegisterDataRequest(key, request, /*add_expiry_bias=*/false); |
There was a problem hiding this comment.
An unauthenticated qwatch peer can still grow mapQuorumDataRequests without bound by sending each public, known block hash once with a valid LLMQ type. Every distinct hash passes the new lookup, creates a fresh key here, and receives QUORUM_NOT_FOUND without any score because it is not a repeat; the chain therefore supplies millions of usable keys, while cleanup is delayed for 360 seconds and does not run during IBD. Retain a hard per-peer/identity live-entry cap so validating that a hash exists does not merely replace arbitrary hashes with an enormous attacker-enumerable set.
AGENTS.md reference: AGENTS.md:L162-L180
Useful? React with 👍 / 👎.
Drop the vestigial first-request setup and cleanup-loop duplication from the nError functional case; assert unknown-block +10 in the existing error-code path; trim stale pre/post-fix comments.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b7054d493
ℹ️ 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".
| } | ||
| // Core only asks for known quorums; unknown hashes are abuse or a large fork. | ||
| sendQDATA(CQuorumDataRequest::Errors::QUORUM_BLOCK_NOT_FOUND, /*request_limit_exceeded=*/false); | ||
| m_peer_manager->PeerMisbehaving(pfrom.GetId(), 10, "unknown quorum block in QGETDATA"); |
There was a problem hiding this comment.
When the receiver is behind the requester or has not learned the requester's fork, this hash is legitimately unknown locally: RequestQuorumData sends the requester's locally known quorum-base hash without checking the recipient's height. Adding 10 points for every such recovery request means ten distinct valid quorum requests reach the discouragement threshold and can disconnect a healthy masternode solely because the peers' chain states differ. Keep this response unscored or distinguish abuse using bounded request tracking, and cover the chain-state-skew case.
AGENTS.md reference: AGENTS.md:L169-L180
Useful? React with 👍 / 👎.
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/test/llmq_qgetdata_tests.cpp`:
- Around line 164-169: Strengthen the QGETDATA test around the two distinct
unknown hashes by asserting after each ProcessMessage call that
CQuorumManager::IsDataRequestPending(peer->GetVerifiedProRegTxHash(), false,
quorum_hash, Consensus::LLMQType::LLMQ_TEST) returns false. Use each request’s
quorum hash and retain the existing misbehavior-score checks, making this a
targeted unit test for the invariant that unknown requests are not tracked.
🪄 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: 167eabbe-b3ba-40ca-92e2-5437079b0bf8
📒 Files selected for processing (4)
src/llmq/net_quorum.cppsrc/llmq/quorumsman.hsrc/test/llmq_qgetdata_tests.cpptest/functional/p2p_quorum_data.py
🚧 Files skipped from review as they are similar to previous changes (1)
- test/functional/p2p_quorum_data.py
| // Distinct unknown hashes keep scoring; they never enter the per-key map, so | ||
| // the classic rate limit cannot bound this path on its own. | ||
| auto stream2 = MakeQGetDataStream(Consensus::LLMQType::LLMQ_TEST, uint256S("0x55"), | ||
| CQuorumDataRequest::QUORUM_VERIFICATION_VECTOR, protx_hash); | ||
| m_net_quorum->ProcessMessage(*peer, NetMsgType::QGETDATA, stream2); | ||
| AssertMisbehaviorScore(*m_node.peerman, *peer, 20); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that unknown requests are not tracked.
The test sends two different hashes and checks only the cumulative score. It would still pass if both requests were inserted into mapQuorumDataRequests, because the QUORUM_BLOCK_NOT_FOUND response uses request_limit_exceeded=false. After each request, assert that CQuorumManager::IsDataRequestPending(peer->GetVerifiedProRegTxHash(), false, quorum_hash, Consensus::LLMQType::LLMQ_TEST) is false.
As per coding guidelines, add a targeted C++ unit test that proves the changed request-tracking invariant.
🤖 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/test/llmq_qgetdata_tests.cpp` around lines 164 - 169, Strengthen the
QGETDATA test around the two distinct unknown hashes by asserting after each
ProcessMessage call that
CQuorumManager::IsDataRequestPending(peer->GetVerifiedProRegTxHash(), false,
quorum_hash, Consensus::LLMQType::LLMQ_TEST) returns false. Use each request’s
quorum hash and retain the existing misbehavior-score checks, making this a
targeted unit test for the invariant that unknown requests are not tracked.
Source: Coding guidelines
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The requester-supplied nError rejection is correctly ordered, but the current redesign does not achieve the PR's stated request-tracking bound: distinct known block hashes still create effectively unbounded live entries. It also newly penalizes legitimate requests when the recipient has not learned the requester's quorum-base block, so changes are required before merge.
Source: reviewer backends gpt-5.6-sol (general), gpt-5.6-sol (dash-core-commit-history); final verifier backend gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus 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)
🔴 2 blocking | 🟡 3 suggestion(s)
2 additional finding(s) omitted (not in diff).
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
🤖 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/net_quorum.cpp`:
- [BLOCKING] src/llmq/net_quorum.cpp:131-132: Restore a hard cap for known-block QGETDATA keys
The block-index precheck proves only that the supplied hash names a locally indexed block, not that it is a quorum base. An unauthenticated qwatch peer can enumerate public historical block hashes, and every fresh `(null proRegTx, hash, valid type)` key is inserted here before `GetQuorum` returns `QUORUM_NOT_FOUND`. Because each key is new, the per-key rate limit never rejects or scores it. Entries remain for the expiry window and cleanup runs only from `UpdatedBlockTip`, which skips cleanup during IBD and while unsynced. The public chain therefore still supplies millions of attacker-selectable keys, contradicting the PR's stated goal of bounding tracking. Restore a hard live-entry cap, preferably per peer so unrelated qwatch connections do not share one attackable budget.
- [BLOCKING] src/llmq/net_quorum.cpp:123-127: Do not score blocks that are only unknown to the receiver
`RequestQuorumData` sends a quorum-base hash known to the requester without establishing that the recipient has learned that block. An authenticated masternode or legitimate qwatch peer can therefore request a valid quorum while the receiver is behind, resyncing, or unaware of the requester's fork. `LookupBlockIndex` then fails locally and this branch assigns 10 points despite a valid request; ten distinct requests reach the discouragement threshold. Keep `QUORUM_BLOCK_NOT_FOUND` unscored, as the PR description itself specifies, and address tracking-map abuse with a hard entry budget rather than treating chain-state skew as proof of misbehavior.
In `<commit:359bdcf0a9a>`:
- [SUGGESTION] <commit:359bdcf0a9a>:1: Fold the comment cleanup into the primary fix
Commit 359bdcf0a9a only shortens comments introduced immediately beforehand by f38eb770003 in the same file. Preserving both commits creates avoidable add-then-remove blame noise without providing an independently useful revision. Squash 359bdcf0a9a into f38eb770003 so the primary fix lands with its intended final comments.
In `src/test/llmq_qgetdata_tests.cpp`:
- [SUGGESTION] src/test/llmq_qgetdata_tests.cpp:156-169: Assert that unknown requests are not tracked
The test verifies only the cumulative misbehavior score. It would still pass if either unknown hash were inserted into `mapQuorumDataRequests`, because the response path explicitly supplies `request_limit_exceeded=false`. After each `ProcessMessage` call, directly assert that `IsDataRequestPending(peer->GetVerifiedProRegTxHash(), false, request_hash, Consensus::LLMQType::LLMQ_TEST)` is false so the central prevalidation-before-tracking invariant is covered.
- [SUGGESTION] src/test/llmq_qgetdata_tests.cpp:125-143: Cover a zero-valued trailing error byte
(existing thread: https://github.com/dashpay/dash/pull/7519#discussion_r3744382209)
The test sends only `ENCRYPTED_CONTRIBUTIONS_MISSING`, so it does not establish that the presence of any trailing `nError` byte is invalid independently of its value. An implementation that rejected only nonzero errors while accepting an explicitly serialized `Errors::NONE` byte would pass this test while violating the protocol invariant. Add a request carrying `static_cast<uint8_t>(CQuorumDataRequest::Errors::NONE)` through a fresh initialized peer and assert a misbehavior score of 100.
In `<commit:be44542d6f6>`:
- [SUGGESTION] <commit:be44542d6f6>:1: Fold the abandoned budget design into the primary fix
Commit f38eb770003 introduces the per-identity counter, inbound cap, tri-state registration API, caller adaptations, and cap-specific tests as a central part of its QGETDATA solution. Commit be44542d6f6 immediately removes that newly introduced design, restores the boolean API, and deletes its tests before any of it has shipped. Keeping both commits permanently records a large immediate reversal and leaves the primary commit describing a design abandoned by the next substantive commit. Fold the final design into f38eb770003 and reword that commit to describe the implementation that will actually remain.
| const CQuorumDataRequestKey key(pfrom.GetVerifiedProRegTxHash(), false, request.GetQuorumHash(), request.GetLLMQType()); | ||
| const bool request_limit_exceeded = !m_qman.RegisterDataRequest(key, request, /*add_expiry_bias=*/false); |
There was a problem hiding this comment.
🔴 Blocking: Restore a hard cap for known-block QGETDATA keys
The block-index precheck proves only that the supplied hash names a locally indexed block, not that it is a quorum base. An unauthenticated qwatch peer can enumerate public historical block hashes, and every fresh (null proRegTx, hash, valid type) key is inserted here before GetQuorum returns QUORUM_NOT_FOUND. Because each key is new, the per-key rate limit never rejects or scores it. Entries remain for the expiry window and cleanup runs only from UpdatedBlockTip, which skips cleanup during IBD and while unsynced. The public chain therefore still supplies millions of attacker-selectable keys, contradicting the PR's stated goal of bounding tracking. Restore a hard live-entry cap, preferably per peer so unrelated qwatch connections do not share one attackable budget.
source: ['codex']
| const CBlockIndex* pQuorumBaseBlockIndex = WITH_LOCK(::cs_main, return m_chainman.m_blockman.LookupBlockIndex(request.GetQuorumHash())); | ||
| if (pQuorumBaseBlockIndex == nullptr) { | ||
| if (sendQDATA(CQuorumDataRequest::Errors::QUORUM_BLOCK_NOT_FOUND, request_limit_exceeded)) { | ||
| m_peer_manager->PeerMisbehaving(pfrom.GetId(), 25, "request limit exceeded"); | ||
| } | ||
| // Core only asks for known quorums; unknown hashes are abuse or a large fork. | ||
| sendQDATA(CQuorumDataRequest::Errors::QUORUM_BLOCK_NOT_FOUND, /*request_limit_exceeded=*/false); | ||
| m_peer_manager->PeerMisbehaving(pfrom.GetId(), 10, "unknown quorum block in QGETDATA"); |
There was a problem hiding this comment.
🔴 Blocking: Do not score blocks that are only unknown to the receiver
RequestQuorumData sends a quorum-base hash known to the requester without establishing that the recipient has learned that block. An authenticated masternode or legitimate qwatch peer can therefore request a valid quorum while the receiver is behind, resyncing, or unaware of the requester's fork. LookupBlockIndex then fails locally and this branch assigns 10 points despite a valid request; ten distinct requests reach the discouragement threshold. Keep QUORUM_BLOCK_NOT_FOUND unscored, as the PR description itself specifies, and address tracking-map abuse with a hard entry budget rather than treating chain-state skew as proof of misbehavior.
| const CBlockIndex* pQuorumBaseBlockIndex = WITH_LOCK(::cs_main, return m_chainman.m_blockman.LookupBlockIndex(request.GetQuorumHash())); | |
| if (pQuorumBaseBlockIndex == nullptr) { | |
| if (sendQDATA(CQuorumDataRequest::Errors::QUORUM_BLOCK_NOT_FOUND, request_limit_exceeded)) { | |
| m_peer_manager->PeerMisbehaving(pfrom.GetId(), 25, "request limit exceeded"); | |
| } | |
| // Core only asks for known quorums; unknown hashes are abuse or a large fork. | |
| sendQDATA(CQuorumDataRequest::Errors::QUORUM_BLOCK_NOT_FOUND, /*request_limit_exceeded=*/false); | |
| m_peer_manager->PeerMisbehaving(pfrom.GetId(), 10, "unknown quorum block in QGETDATA"); | |
| if (pQuorumBaseBlockIndex == nullptr) { | |
| // The requester can legitimately be ahead of us or on another fork. | |
| sendQDATA(CQuorumDataRequest::Errors::QUORUM_BLOCK_NOT_FOUND, /*request_limit_exceeded=*/false); | |
| return; | |
| } |
source: ['codex']
| const uint256 quorum_hash{uint256S("0x33")}; | ||
| const uint256 protx_hash{uint256S("0x44")}; | ||
| auto stream = MakeQGetDataStream(Consensus::LLMQType::LLMQ_TEST, quorum_hash, | ||
| CQuorumDataRequest::QUORUM_VERIFICATION_VECTOR, protx_hash); | ||
|
|
||
| m_net_quorum->ProcessMessage(*peer, NetMsgType::QGETDATA, stream); | ||
| AssertMisbehaviorScore(*m_node.peerman, *peer, 10); | ||
|
|
||
| // Distinct unknown hashes keep scoring; they never enter the per-key map, so | ||
| // the classic rate limit cannot bound this path on its own. | ||
| auto stream2 = MakeQGetDataStream(Consensus::LLMQType::LLMQ_TEST, uint256S("0x55"), | ||
| CQuorumDataRequest::QUORUM_VERIFICATION_VECTOR, protx_hash); | ||
| m_net_quorum->ProcessMessage(*peer, NetMsgType::QGETDATA, stream2); | ||
| AssertMisbehaviorScore(*m_node.peerman, *peer, 20); |
There was a problem hiding this comment.
🟡 Suggestion: Assert that unknown requests are not tracked
The test verifies only the cumulative misbehavior score. It would still pass if either unknown hash were inserted into mapQuorumDataRequests, because the response path explicitly supplies request_limit_exceeded=false. After each ProcessMessage call, directly assert that IsDataRequestPending(peer->GetVerifiedProRegTxHash(), false, request_hash, Consensus::LLMQType::LLMQ_TEST) is false so the central prevalidation-before-tracking invariant is covered.
source: ['coderabbit']
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 100. Our own writers never emit the field, so a request that carries one is not a message we could ever have produced — there is no "might be legitimate" reading to tolerate a run of, which is what the smaller graduated scores are for. This matches the treatment fix: score peers requesting an unregistered LLMQType via QGETDATA #7529 just gave the unregistered-llmqTypecase.llmqTypeadded in fix: score peers requesting an unregistered LLMQType via QGETDATA #7529 is preserved in the new ordering; theQUORUM_BLOCK_NOT_FOUNDreply stays unscored, since a block we have not synced yet is legitimate for an honest peer.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. Exhausting it scores 25 like the existing request-limit path — unlike the two cases above, a peer can plausibly reach it by being noisy rather than malformed.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.p2p_quorum_data.pyadditionally asserts the poisoned request drops the connection outright.Rebased onto develop after #7529 merged (which touched the same
QUORUM_TYPE_INVALIDbranch). Built locally;test_dash --run_test=llmq_qgetdata_testspasses (4 cases) andp2p_quorum_data.pypasses. Full validation is delegated to CI.Breaking Changes
None.
nErroron an inbound request was never meaningful.Checklist: