perf: check quorum activity before materializing the quorum on QSIGREC - #7531
perf: check quorum activity before materializing the quorum on QSIGREC#7531PastaPastaPasta wants to merge 1 commit into
Conversation
…QSIGREC VerifyAndProcessRecoveredSig called GetQuorum before IsQuorumActive. GetQuorum rebuilds arbitrary historical mined commitments (DMN list replay plus member selection) on a cache miss, whereas IsQuorumActive only scans the small cached set of recent quorums. An unsolicited QSIGREC naming an inactive quorum hash therefore forced the expensive path before the cheap gate could reject it. Swap the order so the cheap gate runs first. Once IsQuorumActive passes, the hash is one of the few quorums ScanQuorums just materialized, so the subsequent GetQuorum is a cache hit. A null quorum there is no longer peer-controlled, so it is logged without a misbehaviour score.
|
✅ Final review complete — no blockers (commit 87753a0) |
PastaPastaPasta
changed the title
Walkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
![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/signing.cpp`:
- Around line 371-375: Correct the rationale comment above the IsQuorumActive
gate: state that it is bounded to scanning/materializing only recent
keepOldConnections quorums, while GetQuorum may reconstruct the arbitrary
historical quorum supplied by the peer. Preserve the explanation that inactive
hashes must not trigger that historical reconstruction, and remove the
inaccurate claim that ScanQuorums only scans an existing cached set.
🪄 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: c8cb580d-63d3-4291-b192-2f3973380fc2
📒 Files selected for processing (1)
src/llmq/signing.cpp
| // 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. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the cache behavior in this rationale.
IsQuorumActive calls ScanQuorums. On a scan-cache miss, ScanQuorums materializes recent quorums through GetQuorum; it does not only scan an existing cache. State that the check is bounded to recent keepOldConnections quorums. This preserves the actual performance property: it avoids reconstruction of the arbitrary historical quorum supplied by the peer.
As per coding guidelines, reserve C++ comments for non-obvious rationale.
Proposed fix
- // 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.
+ // Check the bounded active quorum set first. IsQuorumActive considers recent
+ // quorums (keepOldConnections), although ScanQuorums can materialize that set
+ // on a cache miss. Do not let an unsolicited QSIGREC reconstruct an arbitrary
+ // historical quorum for an inactive hash.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 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. | |
| // Check the bounded active quorum set first. IsQuorumActive considers recent | |
| // quorums (keepOldConnections), although ScanQuorums can materialize that set | |
| // on a cache miss. Do not let an unsolicited QSIGREC reconstruct an arbitrary | |
| // historical quorum for an inactive hash. |
🤖 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/signing.cpp` around lines 371 - 375, Correct the rationale comment
above the IsQuorumActive gate: state that it is bounded to
scanning/materializing only recent keepOldConnections quorums, while GetQuorum
may reconstruct the arbitrary historical quorum supplied by the peer. Preserve
the explanation that inactive hashes must not trigger that historical
reconstruction, and remove the inaccurate claim that ScanQuorums only scans an
existing cached set.
Source: Coding guidelines
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
This PR reorders two checks in CSigningManager::VerifyAndProcessRecoveredSig so the cheap IsQuorumActive gate runs before the expensive GetQuorum lookup, preventing an unsolicited QSIGREC naming an inactive/historical quorum hash from forcing arbitrary historical quorum reconstruction. I verified the reordering is correct and safe: NetSigning::ProcessMessage already validates the llmq type before calling this function, and BuildQuorumFromCommitment unconditionally inserts into mapQuorumsCache on every build, so the subsequent GetQuorum call is very likely (though not unconditionally guaranteed under LRU eviction) a cache hit. The only issue found is a documentation nitpick: the new comments slightly overstate the caching guarantee of ScanQuorums/GetQuorum, since scanQuorumsCache and mapQuorumsCache are separate LRUs and a cache-hit return path in ScanQuorums does not always freshly repopulate mapQuorumsCache. This does not affect correctness of the optimization, only comment precision.
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 nitpick(s)
| @@ -383,6 +383,17 @@ void CSigningManager::VerifyAndProcessRecoveredSig(NodeId from, std::shared_ptr< | |||
| return; | |||
| } | |||
|
|
|||
| // Once IsQuorumActive has passed, quorumHash is one of the few quorums ScanQuorums | |||
| // just materialised and cached, so this is a cache hit rather than a rebuild. | |||
| auto quorum = qman.GetQuorum(llmq_type, quorum_hash); | |||
There was a problem hiding this comment.
💬 Nitpick: Comments overstate ScanQuorums/GetQuorum caching guarantee
The new comments state that IsQuorumActive "only scans the small cached set of recent quorums" and that once it passes, the subsequent GetQuorum call is "a cache hit rather than a rebuild." This isn't quite accurate: ScanQuorums (called by IsQuorumActive) does more than scan a cache — on a scanQuorumsCache miss it calls GetMinedCommitmentsUntilBlock/GetMinedCommitmentsIndexedUntilBlock and materializes up to keepOldConnections quorums via GetQuorum/BuildQuorumFromCommitment. Also, scanQuorumsCache and mapQuorumsCache are separate LRU caches (the former in quorumsman.h keyed by block hash and holding CQuorumCPtr vectors, the latter keyed by quorum hash and holding CQuorumPtr): when ScanQuorums returns a hit from scanQuorumsCache without touching mapQuorumsCache, the quorums it returns may have already aged out of mapQuorumsCache's LRU. The subsequent GetQuorum call would then rebuild via BuildQuorumFromCommitment rather than hitting the cache. This doesn't break the optimization's safety property — the rebuild in that fallback case is still bounded to one of the last few active quorums, not an attacker-chosen historical one — but the comments should describe a bounded-cost guarantee rather than an unconditional cache-hit guarantee.
| // Cheap gate first. IsQuorumActive bounds work to scanning/materializing the | |
| // keepOldConnections most recent quorums. GetQuorum, by contrast, can rebuild the | |
| // arbitrary historical quorum named by the peer on a cache miss, so reject | |
| // inactive hashes before allowing that work. Caller (NetSigning) has already | |
| // rejected unknown llmq types. | |
| if (!IsQuorumActive(llmq_type, qman, quorum_hash)) { | |
| return; | |
| } | |
| // It's important to only skip seen *valid* sig shares here. See comment for CBatchedSigShare | |
| // We don't receive recovered sigs in batches, but we do batched verification per node on these | |
| if (db.HasRecoveredSigForHash(recoveredSig->GetHash())) { | |
| return; | |
| } | |
| // Now that IsQuorumActive restricted quorum_hash to one of the last few active | |
| // quorums, this GetQuorum call is usually a cache hit, and in the rare miss case | |
| // is a cheap rebuild bounded to a recent quorum rather than an arbitrary one. | |
| auto quorum = qman.GetQuorum(llmq_type, quorum_hash); |
source: ['codex']
2 participants
Issue being fixed or feature implemented
CSigningManager::VerifyAndProcessRecoveredSigcalledqman.GetQuorum(...)beforeIsQuorumActive(...).Those two have very different costs.
IsQuorumActivescans only the small cached set of recent quorums, whileGetQuorumrebuilds arbitrary historical mined commitments on a cache miss — a deterministic masternode list replay plus member selection. An unsolicitedQSIGRECnaming 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, where it had been bundled with unrelated QGETDATA work.
What was done?
Swap the order so the cheap gate runs first. Once
IsQuorumActivepasses, the hash is one of the few quorumsScanQuorumsjust materialized and cached, so the subsequentGetQuorumis a cache hit rather than a rebuild.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 reachesIsQuorumActive.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: