Skip to content

perf: check quorum activity before materializing the quorum on QSIGREC #7531

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
PastaPastaPasta wants to merge 1 commit into
base: develop
Choose a base branch
from
+19 −8
Open

perf: check quorum activity before materializing the quorum on QSIGREC #7531

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 19 additions & 8 deletions src/llmq/signing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,14 +366,14 @@ bool CSigningManager::GetRecoveredSigForGetData(const uint256& hash, CRecoveredS
void CSigningManager::VerifyAndProcessRecoveredSig(NodeId from, std::shared_ptr<CRecoveredSig> recoveredSig)
{
auto llmq_type = recoveredSig->getLlmqType();
auto quorum = qman.GetQuorum(llmq_type, recoveredSig->getQuorumHash());

if (!quorum) {
LogPrint(BCLog::LLMQ, "CSigningManager::%s -- quorum %s not found\n", __func__,
recoveredSig->getQuorumHash().ToString());
return;
}
if (!IsQuorumActive(llmq_type, qman, quorum->qc->quorumHash)) {
const uint256& quorum_hash = recoveredSig->getQuorumHash();

// 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.
Comment on lines +371 to +375

@coderabbitai coderabbitai Bot Aug 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 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.

Suggested change
// 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

if (!IsQuorumActive(llmq_type, qman, quorum_hash)) {
return;
}

Expand All @@ -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);
Comment on lines 371 to +388

@thepastaclaw thepastaclaw Aug 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💬 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.

Suggested change
// 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']

if (!quorum) {
// Reported active by ScanQuorums but no longer materializable (e.g. reorg).
// Not peer-controlled once the hash is restricted to the active set, so no score.
LogPrint(BCLog::LLMQ, "CSigningManager::%s -- quorum %s not found\n", __func__,
quorum_hash.ToString());
return;
}

LogPrint(BCLog::LLMQ, "CSigningManager::%s -- signHash=%s, id=%s, msgHash=%s, node=%d\n", __func__,
recoveredSig->buildSignHash().ToString(), recoveredSig->getId().ToString(), recoveredSig->getMsgHash().ToString(), from);

Expand Down
Loading