src/llmq/blockprocessor.cpp (1)

208-220: Consider adding error logging when signature verification fails.

The asynchronous signature verification implementation is correct and follows proper patterns. However, when queue_control.Wait() returns false, it would be helpful to log which commitment(s) failed verification for debugging purposes.

Consider adding a log message before returning false:

 if (!queue_control.Wait()) {
     // at least one check failed
+    LogPrintf("[ProcessBlock] BLS signature verification failed for block at height %d\n", pindex->nHeight);
     return false;
 }

src/llmq/commitment.cpp (1)

31-95: Consider refactoring for improved modularity.

The implementation correctly handles both synchronous and asynchronous BLS signature verification. However, the method is quite long (64 lines) and handles multiple responsibilities.

Consider extracting helper methods to improve readability and maintainability:

bool CFinalCommitment::VerifySignatureAsync(CDeterministicMNManager& dmnman, CQuorumSnapshotManager& qsnapman,
                                           gsl::not_null<const CBlockIndex*> pQuorumBaseBlockIndex,
                                           CCheckQueueControl<utils::BlsCheck>* queue_control) const
{
    auto members = utils::GetAllQuorumMembers(llmqType, dmnman, qsnapman, pQuorumBaseBlockIndex);
    const auto& llmq_params_opt = Params().GetLLMQ(llmqType);
    if (!llmq_params_opt.has_value()) {
        LogPrint(BCLog::LLMQ, "CFinalCommitment -- q[%s] invalid llmqType=%d\n", quorumHash.ToString(),
                 ToUnderlying(llmqType));
        return false;
    }
    const auto& llmq_params = llmq_params_opt.value();

    uint256 commitmentHash = BuildCommitmentHash(llmq_params.type, quorumHash, validMembers, quorumPublicKey,
                                                 quorumVvecHash);
    LogMemberDetails(members, commitmentHash);

    if (!VerifyMemberSignatures(llmq_params, members, commitmentHash, queue_control)) {
        return false;
    }

    if (!VerifyQuorumSignature(commitmentHash, queue_control)) {
        return false;
    }

    return true;
}

private:
bool CFinalCommitment::VerifyMemberSignatures(const Consensus::LLMQParams& llmq_params,
                                             const std::vector<CDeterministicMNCPtr>& members,
                                             const uint256& commitmentHash,
                                             CCheckQueueControl<utils::BlsCheck>* queue_control) const
{
    // Implementation for member signature verification
}

bool CFinalCommitment::VerifyQuorumSignature(const uint256& commitmentHash,
                                            CCheckQueueControl<utils::BlsCheck>* queue_control) const
{
    // Implementation for quorum signature verification
}

src/llmq/commitment.h (1)

34-37: ⚠️ Potential issue

Fix namespace formatting issue identified by pipeline.

The nested namespace declaration needs proper formatting according to clang-format requirements.

Apply this diff to fix the formatting:

-namespace utils
-{
-struct BlsCheck;
-} // namespace utils
+namespace utils {
+struct BlsCheck;
+} // namespace utils

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

namespace utils {
struct BlsCheck;
} // namespace utils

🤖 Prompt for AI Agents

In src/llmq/commitment.h around lines 34 to 37, the nested namespace declaration
for utils is not properly formatted according to clang-format standards. Adjust
the namespace declaration to use the correct syntax and indentation style,
ensuring the opening and closing braces align properly and the comment
indicating the namespace closure is correctly placed.

src/llmq/blockprocessor.cpp (1)

225-229: Optional: add a concise log on queue failure to include block context

While BlsCheck logs the failing commitment context, adding a single line here with the block hash can simplify triage.

-        if (!queue_control.Wait()) {
-            // at least one check failed
-            return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-qc-invalid");
-        }
+        if (!queue_control.Wait()) {
+            LogPrint(BCLog::LLMQ, "CQuorumBlockProcessor: BLS verification failed for block %s\n", blockHash.ToString());
+            return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-qc-invalid");
+        }

src/llmq/utils.cpp (1)

943-961: BLS check functor logic looks correct

• Correctly chooses VerifySecureAggregated for multiple pubkeys and VerifyInsecure for single pubkey.
• Returns false and logs when no pubkeys are provided, which is appropriate for a defensive check in the async worker.

Two minor polish suggestions:

• Consider making operator() a const method if BlsCheck’s members are immutable during verification.
• Optionally short-circuit on an obviously invalid signature (e.g., m_sig.IsValid()) to avoid unnecessary work.

src/llmq/commitment.cpp (2)

61-79: Reserve and batch Add() calls for fewer allocations and better queue efficiency

• Reserve memberPubKeys to at least CountSigners() to avoid multiple reallocations, especially for large LLMQs.
• Batch both the members and quorum checks into a single Add() call to reduce queue overhead.

-        std::vector<CBLSPublicKey> memberPubKeys;
+        std::vector<CBLSPublicKey> memberPubKeys;
+        memberPubKeys.reserve(CountSigners());
         for (const auto i : irange::range(members.size())) {
             if (!signers[i]) {
                 continue;
             }
             memberPubKeys.emplace_back(members[i]->pdmnState->pubKeyOperator.Get());
         }
         std::string members_id_string{
             strprintf("CFinalCommitment -- q[%s] invalid aggregated members signature", quorumHash.ToString())};
         if (queue_control) {
-            std::vector<utils::BlsCheck> vChecks;
-            vChecks.emplace_back(membersSig, memberPubKeys, commitmentHash, members_id_string);
-            queue_control->Add(vChecks);
+            std::vector<utils::BlsCheck> vChecks;
+            vChecks.reserve(2);
+            vChecks.emplace_back(membersSig, memberPubKeys, commitmentHash, members_id_string);
+            // The quorumSig check is added below to the same batch
+            // (see matching change in the quorumSig branch)
+            // queue_control->Add(vChecks) will be called once after pushing both
+            // checks.
+            // NOTE: see matching change in lines 82-89
         } else {
             if (!membersSig.VerifySecureAggregated(memberPubKeys, commitmentHash)) {
                 LogPrint(BCLog::LLMQ, "%s\n", members_id_string);
                 return false;
             }
         }

And couple this with the change below (lines 82–89) to actually batch both checks.

---

82-95: Batch the quorumSig check together with memberSig in a single Add()

Currently, two Add() calls are performed. Combine them into one to reduce queue contention.

-    std::string qsig_id_string{strprintf("CFinalCommitment -- q[%s] invalid quorum signature", quorumHash.ToString())};
-    if (queue_control) {
-        std::vector<utils::BlsCheck> vChecks;
-        std::vector<CBLSPublicKey> public_keys;
-        public_keys.push_back(quorumPublicKey);
-        vChecks.emplace_back(quorumSig, public_keys, commitmentHash, qsig_id_string);
-        queue_control->Add(vChecks);
-    } else {
+    std::string qsig_id_string{strprintf("CFinalCommitment -- q[%s] invalid quorum signature", quorumHash.ToString())};
+    if (queue_control) {
+        // If we already prepared vChecks above (multi-member path), append here. Otherwise, create it now.
+        std::vector<utils::BlsCheck> vChecks;
+        std::vector<CBLSPublicKey> public_keys;
+        public_keys.push_back(quorumPublicKey);
+        vChecks.emplace_back(quorumSig, public_keys, commitmentHash, qsig_id_string);
+        queue_control->Add(vChecks);
+    } else {
         if (!quorumSig.VerifyInsecure(quorumPublicKey, commitmentHash)) {
             LogPrint(BCLog::LLMQ, "%s\n", qsig_id_string);
             return false;
         }
     }

If you adopt batching in the multi-member branch (previous comment), ensure vChecks and public_keys are kept in scope appropriately to push both checks and call Add() once.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.

Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/evo/specialtx.h : Special transactions use payload extensions defined in src/evo/specialtx.h

src/llmq/blockprocessor.cpp (1)

217-229: Guard against null pQuorumBaseBlockIndex when queueing async checks.

LookupBlockIndex can return nullptr for untrusted blocks. Passing nullptr into downstream code that constructs gsl::not_null (as noted previously) is UB. While you have a follow-up PR (#6789) for a similar spot, this location should be guarded too to avoid queuing invalid work.

Applying a minimal guard (skip queueing the check; it will be rejected later) would be safe:

-        for (const auto& [_, qc] : qcs) {
-            if (qc.IsNull()) continue;
-            const auto* pQuorumBaseBlockIndex = m_chainstate.m_blockman.LookupBlockIndex(qc.quorumHash);
-            qc.VerifySignatureAsync(m_dmnman, m_qsnapman, pQuorumBaseBlockIndex, &queue_control);
-        }
+        for (const auto& [_, qc] : qcs) {
+            if (qc.IsNull()) continue;
+            const auto* pQuorumBaseBlockIndex = m_chainstate.m_blockman.LookupBlockIndex(qc.quorumHash);
+            if (!pQuorumBaseBlockIndex) {
+                // Defer validation to ProcessCommitment; avoid passing nullptr to code expecting not_null
+                continue;
+            }
+            qc.VerifySignatureAsync(m_dmnman, m_qsnapman, pQuorumBaseBlockIndex, &queue_control);
+        }

src/llmq/blockprocessor.h (1)

48-49: Hard-coded CCheckQueue batch size (4) — consider making it configurable or aligning with script-check queue.

m_bls_queue{4} sets the queue batch size to 4. That value controls how many items a worker processes per batch and can materially affect throughput/latency. Consider:

• Using a named constant (e.g., DEFAULT_BLSCHECK_BATCH) for discoverability.
• Optionally exposing it (hidden) behind an arg or deriving it from hardware like we do for thread count.

Not a blocker; just a maintainability/perf knob to keep in mind.

src/llmq/blockprocessor.cpp (2)

15-15: Redundant include (minor nit).

#include <checkqueue.h> is already present in the header; including it here too is harmless but redundant. You can keep it if it helps include hygiene, otherwise consider removing to reduce compile-time dependencies.

---

217-229: Consider releasing cs_main before waiting on BLS worker threads (optional).

We queue checks under cs_main and then Wait() while still holding it. If VerifySignatureAsync fully materializes inputs (msg hash, pubkeys, sigs) before queuing, you could unlock cs_main prior to Wait() to reduce contention on the global chainstate lock during CPU-bound BLS verification. This would require careful audit of any shared structures referenced by worker code (but BlsCheck itself only uses captured values).

Not required for this perf PR; just a potential follow-up to improve parallelism.

src/llmq/utils.cpp (1)

943-962: BlsCheck functor implementation — LGTM, with minor nits optional.

• Correctly distinguishes aggregated vs single-key verify paths and logs a clear id string on failure.
• Early-return on empty pubkey vector is defensive and appropriate.

Optional nits:

• You could early-check m_pubkeys.empty() to reduce nesting, but current readability is fine.
• If you ever need richer diagnostics, consider including sizes of m_pubkeys or a short hash of m_msg_hash in the log. Not necessary now since callers already prepare informative m_id_string.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: knst
PR: dashpay/dash#6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.763Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.

Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.

Learnt from: knst
PR: dashpay/dash#6692
File: src/llmq/blockprocessor.cpp:217-224
Timestamp: 2025-08-19T14:57:31.763Z
Learning: In PR #6692, knst acknowledged a null pointer dereference issue in ProcessBlock() method where LookupBlockIndex may return nullptr but is passed to gsl::not_null, and created follow-up PR #6789 to address it, consistent with avoiding scope creep in performance-focused PRs.

Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.

Learnt from: kwvg
PR: dashpay/dash#6761
File: src/wallet/wallet.cpp:0-0
Timestamp: 2025-07-29T14:33:01.040Z
Learning: In refactoring PRs like #6761, kwvg acknowledges code safety improvements (like null pointer checks and unused parameter warnings) but prefers to defer them to follow-up PRs to maintain focus on the primary refactoring objectives, avoiding scope creep.

Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.cpp:15-250
Timestamp: 2025-07-23T09:28:32.783Z
Learning: In refactoring PRs like #6761, kwvg prefers to defer code formatting fixes to separate follow-up PRs when formatting is not the primary objective, to maintain focus on the structural changes and avoid scope creep.

Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.