fix(net): bound and stop re-sorting getqrinfo base block hashes - #7629
fix(net): bound and stop re-sorting getqrinfo base block hashes#7629PastaPastaPasta wants to merge 2 commits into
Conversation
|
✅ Final review complete — no blockers (commit dea5c4b) · triage: critical · Phase 2 only (queue backlog) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review. WalkthroughThe change limits Priority: ⬆️ High Estimated code review effort: 3 (Moderate) | ~20 minutes Severity of issue fixed: High Merge Risk: ⚪ Minimal · up to Quorum rotation requests now reject oversized base-hash lists and preserve base-block selection behavior with sorted lookup. The supplied coverage supports the new limits, malformed-request handling, ordering, and fallback behavior, with no current merge-blocking risk identified. Sequence Diagram(s)sequenceDiagram
participant P2PPeer
participant NetworkHandler
participant CGetQuorumRotationInfo
participant SnapshotDiffConstruction
P2PPeer->>NetworkHandler: send getqrinfo
NetworkHandler->>CGetQuorumRotationInfo: deserialize baseBlockHashes
CGetQuorumRotationInfo-->>NetworkHandler: accept or reject the request
NetworkHandler->>SnapshotDiffConstruction: process valid request
SnapshotDiffConstruction-->>NetworkHandler: construct ordered snapshot diff
NetworkHandler-->>P2PPeer: return rotation information or penalize malformed request
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 |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The exact-head diff correctly bounds unauthenticated getqrinfo deserialization while retaining the existing wire encoding, and it preserves the sorted base-block invariant without repeated full-list sorts. Call-site and test inspection found no actionable correctness, compatibility, performance, or coverage issues.
Source: reviewers codex-general and codex-dash-core-commit-history (exact backend model IDs were not included in the supplied evidence); final verifier Claude (exact runtime model ID was not exposed). openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and is 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)
CGetQuorumRotationInfo::baseBlockHashes had no size limit, so the wire format accepted MAX_PROTOCOL_MESSAGE_LENGTH / sizeof(uint256) = 98304 entries from an unauthenticated peer in a single 3 MiB message. BuildQuorumRotationInfo() then looks every one of them up in the block index and walks the list once per constructed CSimplifiedMNListDiff while holding cs_main. Route it through LIMITED_VECTOR with a 4096 cap. Only the highest base at or below each diff target is ever used and a response builds at most ~101 diffs for llmq_60_75, so no request can usefully carry more than that. Shipping clients send far fewer: DashSync sends one base (DSQuorumRotationService.m), dashj at most six (QuorumRotationState.java), dash-spv at most one (sync/masternodes/manager.rs). LimitedVectorFormatter emits the ordinary vector wire format, so senders are unaffected. Catch the deserialization failure in the handler and score the peer, as FILTERLOAD, FILTERADD and SPORK already do, instead of letting the outer ProcessMessages catch drop it silently. Apply the same cap inside BuildQuorumRotationInfo() so the `quorum rotationinfo` RPC, which builds the request in memory, reports an error rather than doing unbounded work. Add msg_getqrinfo to the functional test framework and cover the at-limit, over-limit and count-only-prefix cases over a real P2P connection. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
282a149 to
303b64c
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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:
|
![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: 303b64c28d
ℹ️ 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".
| hash = baseBlock->GetBlockHash(); | ||
| } | ||
| return hash; | ||
| Assume(std::is_sorted(baseBlockIndexes.begin(), baseBlockIndexes.end(), CBlockIndexHeightOnlyComparator())); |
There was a problem hiding this comment.
For a valid request containing 4096 distinct active-chain hashes, BuildQuorumRotationInfo() calls this helper once per generated diff, and Assume evaluates its expression even in release builds. Consequently, each nominally logarithmic lookup first performs a linear std::is_sorted scan that repeatedly dereferences the entire cache-hostile block-index list while holding cs_main, preserving the k * n work this optimization is intended to remove. Verify sortedness once at the construction or mutation boundary instead of inside every lookup.
AGENTS.md reference: AGENTS.md:L32-L39
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. Assume evaluates its argument in release builds, so the is_sorted check reintroduced a linear scan per lookup. Removed in dea5c4b; the sorted precondition is enforced by construction (initial std::sort plus InsertBaseBlockSorted) and stated in the header comment.
🤖 Posted autonomously by Codex on behalf of pasta.
On the non-legacy construction path GetLastBaseBlockHash() sorted the entire base list on every call, and BuildQuorumRotationInfo() calls it once per constructed CSimplifiedMNListDiff: up to 3 * signingActiveQuorumCount snapshot bases plus the target cycles and the tip, so roughly 10-30 full sorts per request. Every comparison dereferences a CBlockIndex*, so this is cache-hostile work that scales as k * n log n with n chosen by the requesting peer. Make sortedness a precondition of GetLastBaseBlockHash() instead. The initial sort in BuildQuorumRotationInfo() already establishes it for both paths; the two non-legacy call sites that appended mid-construction now insert at std::upper_bound via InsertBaseBlockSorted(), which preserves the order at O(n) pointer moves rather than re-establishing it at O(n log n) index dereferences. With the list sorted, the lookup itself becomes a binary search. Both the sort and the insert use node::CBlockIndexHeightOnlyComparator so a single definition owns the ordering. The genesis fallback reads the precomputed consensus hash instead of re-serialising and hashing the genesis header per call. Output is unchanged. The legacy path never sorted inside GetLastBaseBlockHash() and never appends, so it is untouched. On the non-legacy path the list was already sorted and deduplicated before the first call, and every subsequent append was followed by a re-sort; inserting in position produces the same sequence. Equal heights can only mean the same active-chain block, so ties resolve to the same hash either way. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
303b64c to
dea5c4b
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
No actionable in-scope defects were found at dea5c4b. Source inspection confirms rejection before vector-element decoding, the shared RPC construction limit, and preservation of sortedness for binary-search lookups; the earlier thread's per-lookup sortedness scan is absent. The two commits are narrowly scoped and include relevant regression tests; diff whitespace checks passed and the worktree is clean, but the reported build and test executions were not independently rerun.
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
Review provenance
- Triage:
criticalbygpt-6-astra(effort low) — This changes peer-controlled network deserialization, peer punishment, and quorum-rotation response construction under cs_main, where mistakes could cause denial of service, disconnect legitimate peers, or return incorrect quorum synchronization data. - Phase 1 reviewers: not run (skipped for throughput: 21 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— dash-core-commit-history (completed, effort xhigh); agentphase2-reviewer
thepastaclaw
added
the
pastaclaw:approved
2 participants
Issue being fixed or feature implemented
CGetQuorumRotationInfo::baseBlockHashesis deserialized with no size limit, andGETQUORUMROTATIONINFOis served to any peer, with no rate limiting, while holdingcs_main. Two things compound:MAX_PROTOCOL_MESSAGE_LENGTH / sizeof(uint256)= 98304 base block hashes in a single 3 MiB message. Each one costs a block-index lookup and an active-chain check undercs_main.GetLastBaseBlockHash()sorts the entire base list on every call, andBuildQuorumRotationInfo()calls it once per constructedCSimplifiedMNListDiff. Forllmq_60_75that is up to3 * signingActiveQuorumCount(96) snapshot bases plus the target cycles and the tip, so roughly 10-30 full sorts per request. Every comparison dereferences aCBlockIndex*, so this is cache-hostile work that scales ask * n log nwithnchosen by the requesting peer.The requesting peer does have to supply genuine hashes, since the population loop returns early on the first hash that is not found or is not in the active chain, but on a chain with millions of blocks that costs an attacker nothing, and the peer picks the construction path by advertising its protocol version.
Found while re-verifying an audit finding against
develop; there is no open issue.What was done
Two independent changes, one commit each.
Bound the request.
baseBlockHashesnow deserializes throughLIMITED_VECTOR(..., MAX_BASE_BLOCK_HASHES)withMAX_BASE_BLOCK_HASHES = 4096.LimitedVectorFormatteremits the ordinary vector wire format, so nothing changes for senders. Only the highest base at or below each diff target is ever used, and a response builds at most ~101 diffs forllmq_60_75, so no request can usefully carry more than that. I checked what shipping clients actually send:DSQuorumRotationService.m,-getQRInfoForBlockHash:previousBlockHash:QuorumRotationState.java,getQuorumRotationInfoRequestdash-spvsync/masternodes/manager.rs,send_qrinfo_for_tipgetqrinfoSo 4096 is roughly 40x the theoretical maximum useful list and ~700x real usage, while cutting the attacker's worst case 24x.
The handler now catches the deserialization failure locally and scores the peer with
Misbehaving(100), the same shapeFILTERLOAD,FILTERADDandSPORKalready use, instead of letting the outerProcessMessages()catch drop the message with only aBCLog::NETline. An honest client that somehow exceeded the cap would otherwise hang with no signal on either side.BuildQuorumRotationInfo()also checks the cap itself soquorum rotationinfo, which builds the request in memory without a serialization round-trip, returns an RPC error instead of doing unbounded work.Stop re-sorting.
GetLastBaseBlockHash()no longer sorts; sortedness is now a documented precondition, and it takesSpan<const CBlockIndex* const>so it cannot mutate the caller's list. The two callers that appended mid-construction now useInsertBaseBlockSorted(), which inserts atstd::upper_boundinstead ofpush_back, preserving the ordering atO(n)pointer moves rather than re-establishing it atO(n log n)index dereferences. With the list guaranteed sorted, the lookup itself is a binary search. The initialstd::sortinBuildQuorumRotationInfo()is unchanged and still runs for both construction paths. Both the sort and the insert usenode::CBlockIndexHeightOnlyComparatorso one definition owns the ordering, and the genesis fallback reads the precomputed consensus hash instead of re-hashing the genesis header per call.Output is unchanged. The legacy path never sorted inside
GetLastBaseBlockHash()and never appends, so it is untouched. On the non-legacy path the list was already sorted and deduplicated before the first call, and every subsequent append was followed by a re-sort; inserting in position produces the same sequence. Equal heights can only mean the same active-chain block, so ties resolve to the same hash either way.This PR removes a secondary amplifier. The dominant cost of the handler, ~30
BuildSimplifiedMNListDiffcalls undercs_mainwith no per-peer rate limit, is unchanged and is a separate piece of work.How Has This Been Tested?
Built with
--enable-debug --enable-crash-hookson aarch64-apple-darwin.Unit tests,
src/test/llmq_snapshot_tests.cpp:get_quorum_rotation_info_base_block_hashes_limit_testis new: a request at exactlyMAX_BASE_BLOCK_HASHESround-trips, and one past it throwsstd::ios_base::failurewith the target vector still empty and the element bytes still unread, which is the "rejected before any element is decoded" property the DoS argument rests on.get_last_base_block_hash_repeated_base_blocks_testwas updated for the new contract: the case that fed deliberately unsorted input to exercise the internal sort is gone, and cases covering the genesis fallback for an empty list and for a list entirely above the target were added.Functional test,
test/functional/feature_llmq_rotation.py:quorum rotationinfoblock now also asserts that two base blocks at different heights produce identical output regardless of request order, and that the second base is not inert: it displaces the genesis fallback inmnListDiffAtHMinus3C.test_getqrinfo_base_block_hashes_limitdrivesgetqrinfoover a real P2P connection using a newmsg_getqrinfoframework message. A request at the limit is answered withqrinfoand noMisbehaving; one past the limit, and a bare CompactSize prefix with no element bytes at all, both logMisbehavingwithmalformed getqrinfo receivedand disconnect the peer. The RPC path with the same oversized list returnstoo many baseBlockHashes.llmq_snapshot_tests,coinjoin_inouts_tests,net_testsandfeature_llmq_rotation.pypass locally.lint-python.py,lint-whitespace.py,lint-circular-dependencies.py,lint-includes.pyandclang-format-diffare clean on the diff.Breaking Changes
None for any current client. A
getqrinfocarrying more than 4096 base block hashes is now rejected and the peer is scored as misbehaving; no known client sends more than six, and Dash Core itself never sends this message. Serialization is byte-identical, so senders are unaffected.doc/release-notes-7629.mddocuments the new limit.Checklist: