Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions src/init.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -529,6 +529,8 @@ void SetupServerArgs(ArgsManager& argsman)
argsman.AddArg("-minimumchainwork=<hex>", strprintf("Minimum work assumed to exist on a valid chain in hex (default: %s, testnet: %s)", defaultChainParams->GetConsensus().nMinimumChainWork.GetHex(), testnetChainParams->GetConsensus().nMinimumChainWork.GetHex()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::OPTIONS);
argsman.AddArg("-par=<n>", strprintf("Set the number of script verification threads (%u to %d, 0 = auto, <0 = leave that many cores free, default: %d)",
-GetNumCores(), MAX_SCRIPTCHECK_THREADS, DEFAULT_SCRIPTCHECK_THREADS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
argsman.AddArg("-parbls=<n>", strprintf("Set the number of BLS verification threads (%u to %d, 0 = auto, <0 = leave that many cores free, default: %d)",
-GetNumCores(), llmq::MAX_BLSCHECK_THREADS, llmq::DEFAULT_BLSCHECK_THREADS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
argsman.AddArg("-persistmempool", strprintf("Whether to save the mempool on shutdown and load on restart (default: %u)", DEFAULT_PERSIST_MEMPOOL), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
argsman.AddArg("-pid=<file>", strprintf("Specify pid file. Relative paths will be prefixed by a net-specific datadir location. (default: %s)", BITCOIN_PID_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
argsman.AddArg("-prune=<n>", strprintf("Reduce storage requirements by enabling pruning (deleting) of old blocks. This allows the pruneblockchain RPC to be called to delete specific blocks, and enables automatic pruning of old blocks if a target size in MiB is provided. This mode is incompatible with -txindex, -rescan and -disablegovernance=false. "
Expand Down
39 changes: 36 additions & 3 deletions src/llmq/blockprocessor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

#include <chain.h>
#include <chainparams.h>
#include <checkqueue.h>
#include <consensus/params.h>
#include <consensus/validation.h>
#include <deploymentstatus.h>
Expand Down Expand Up @@ -52,8 +53,25 @@ CQuorumBlockProcessor::CQuorumBlockProcessor(CChainState& chainstate, CDetermini
m_qsnapman(qsnapman)
{
utils::InitQuorumsCache(mapHasMinedCommitmentCache);

int bls_threads = gArgs.GetIntArg("-parbls", DEFAULT_BLSCHECK_THREADS);
if (bls_threads <= 0) {
// -parbls=0 means autodetect (number of cores - 1 validator threads)
// -parbls=-n means "leave n cores free" (number of cores - n - 1 validator threads)
bls_threads += GetNumCores();
}
// Subtract 1 because the main thread counts towards the par threads
bls_threads = std::max(bls_threads - 1, 0);

// Number of script-checking threads <= MAX_BLSCHECK_THREADS
bls_threads = std::min(bls_threads, MAX_BLSCHECK_THREADS);

LogPrintf("BLS verification uses %d additional threads\n", bls_threads);
m_bls_queue.StartWorkerThreads(bls_threads);
}

CQuorumBlockProcessor::~CQuorumBlockProcessor() { m_bls_queue.StopWorkerThreads(); }

MessageProcessingResult CQuorumBlockProcessor::ProcessMessage(const CNode& peer, std::string_view msg_type,
CDataStream& vRecv)
{
Expand Down Expand Up @@ -196,8 +214,21 @@ bool CQuorumBlockProcessor::ProcessBlock(const CBlock& block, gsl::not_null<cons
}
}

if (fBLSChecks) {
CCheckQueueControl<utils::BlsCheck> queue_control(&m_bls_queue);
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);
}
Comment on lines 219 to 223

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think I recall in the early days of testing and benchmarking BLS, we found it to be more efficient to aggregate then verify, rather than verify asynchronously or something like that. Did you investigate if this was possible?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

it maybe faster, but I can't be 100% sure that I won't introduce security issue and my solution will be safe enough


Comment on lines 217 to 224

@coderabbitai coderabbitai Bot Aug 19, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

Potential null dereference: LookupBlockIndex may return nullptr but is passed to gsl::not_null

In ProcessBlock() you call VerifySignatureAsync with pQuorumBaseBlockIndex constructed from LookupBlockIndex(qc.quorumHash). If LookupBlockIndex returns nullptr, constructing gsl::not_null inside VerifySignatureAsync is UB/assert-failing. This can happen before structural verification when processing untrusted blocks.

Skip queueing the check when the index isn’t found (it will be rejected later in ProcessCommitment), or invalidate the block immediately.

-        for (const auto& [_, qc] : qcs) {
+        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);
+            if (!pQuorumBaseBlockIndex) {
+                // Defer to ProcessCommitment() for full validation; avoid passing nullptr to gsl::not_null
+                continue;
+            }
+            qc.VerifySignatureAsync(m_dmnman, m_qsnapman, pQuorumBaseBlockIndex, &queue_control);
         }

Alternatively (stricter): return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-qc-quorum-hash") here.

📝 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
if (fBLSChecks) {
CCheckQueueControl<utils::BlsCheck> queue_control(&m_bls_queue);
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);
}
if (fBLSChecks) {
CCheckQueueControl<utils::BlsCheck> queue_control(&m_bls_queue);
for (const auto& [_, qc] : qcs) {
if (qc.IsNull()) continue;
const auto* pQuorumBaseBlockIndex = m_chainstate.m_blockman.LookupBlockIndex(qc.quorumHash);
if (!pQuorumBaseBlockIndex) {
// Defer to ProcessCommitment() for full validation; avoid passing nullptr to gsl::not_null
continue;
}
qc.VerifySignatureAsync(m_dmnman, m_qsnapman, pQuorumBaseBlockIndex, &queue_control);
}
🤖 Prompt for AI Agents
In src/llmq/blockprocessor.cpp around lines 217 to 224,
LookupBlockIndex(qc.quorumHash) may return nullptr which will lead to undefined
behavior when VerifySignatureAsync expects a gsl::not_null; add a nullptr check
for pQuorumBaseBlockIndex before calling VerifySignatureAsync and handle it by
either skipping queuing the check (continue the loop) or return
state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-qc-quorum-hash") to
immediately reject the block; ensure no gsl::not_null is constructed from a
nullptr in either path.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

addressed in follow-up #6789, it's copy-paste code and out-of-scope to fix

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@knst, thank you for the quick response and for being proactive in creating PR #6789 to address this issue. I understand that keeping the current PR focused on the BLS signature validation performance improvements makes sense to avoid scope creep, especially when dealing with existing code patterns.

Your approach of handling this in a separate follow-up PR is consistent with good engineering practices for maintaining focused changes.


✏️ Learnings added
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.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
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.

if (!queue_control.Wait()) {
// at least one check failed
return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-qc-invalid");
}
Comment on lines 225 to 228

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
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");
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What is that log? Seems very non-clear / non-detailed

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

See a caller, which create an object of BlsCheck:

        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);
        } else {
            if (!membersSig.VerifySecureAggregated(memberPubKeys, commitmentHash)) {
                LogPrint(BCLog::LLMQ, "%s\n", members_id_string);
                return false;
            }
        }
(and 2nd below)

So, there will be CFinalCommitment -- q[%s] invalid aggregated members signature logged or "CFinalCommitment -- q[%s] invalid quorum signature"

}
for (const auto& [_, qc] : qcs) {
if (!ProcessCommitment(pindex->nHeight, blockHash, qc, state, fJustCheck, fBLSChecks)) {
if (!ProcessCommitment(pindex->nHeight, blockHash, qc, state, fJustCheck)) {
LogPrintf("[ProcessBlock] failed h[%d] llmqType[%d] version[%d] quorumIndex[%d] quorumHash[%s]\n", pindex->nHeight, ToUnderlying(qc.llmqType), qc.nVersion, qc.quorumIndex, qc.quorumHash.ToString());
return false;
}
Expand Down Expand Up @@ -237,7 +268,8 @@ static bool IsMiningPhase(const Consensus::LLMQParams& llmqParams, const CChain&
return nHeight >= quorumCycleMiningStartHeight && nHeight <= quorumCycleMiningEndHeight;
}

bool CQuorumBlockProcessor::ProcessCommitment(int nHeight, const uint256& blockHash, const CFinalCommitment& qc, BlockValidationState& state, bool fJustCheck, bool fBLSChecks)
bool CQuorumBlockProcessor::ProcessCommitment(int nHeight, const uint256& blockHash, const CFinalCommitment& qc,
BlockValidationState& state, bool fJustCheck)
{
AssertLockHeld(::cs_main);

Expand Down Expand Up @@ -303,7 +335,8 @@ bool CQuorumBlockProcessor::ProcessCommitment(int nHeight, const uint256& blockH

const auto* pQuorumBaseBlockIndex = m_chainstate.m_blockman.LookupBlockIndex(qc.quorumHash);

if (!qc.Verify(m_dmnman, m_qsnapman, pQuorumBaseBlockIndex, /*checkSigs=*/fBLSChecks)) {
// we don't validate signatures here; they already validated on previous step
if (!qc.Verify(m_dmnman, m_qsnapman, pQuorumBaseBlockIndex, /*checksigs=*/false)) {
LogPrint(BCLog::LLMQ, /* Continued */
"%s -- height=%d, type=%d, quorumIndex=%d, quorumHash=%s, signers=%s, validMembers=%d, "
"quorumPublicKey=%s qc verify failed.\n",
Expand Down
10 changes: 9 additions & 1 deletion src/llmq/blockprocessor.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@

#include <unordered_lru_cache.h>

#include <bls/bls.h>
#include <checkqueue.h>
#include <llmq/params.h>
#include <llmq/utils.h>
#include <protocol.h>
#include <saltedhasher.h>
#include <sync.h>
Expand All @@ -19,6 +22,7 @@
class BlockValidationState;
class CBlock;
class CBlockIndex;
class CBLSSignature;
class CChain;
class CChainState;
class CDataStream;
Expand All @@ -41,6 +45,8 @@ class CQuorumBlockProcessor
CEvoDB& m_evoDb;
CQuorumSnapshotManager& m_qsnapman;

CCheckQueue<utils::BlsCheck> m_bls_queue{4};

mutable Mutex minableCommitmentsCs;
std::map<std::pair<Consensus::LLMQType, uint256>, uint256> minableCommitmentsByQuorum GUARDED_BY(minableCommitmentsCs);
std::map<uint256, CFinalCommitment> minableCommitments GUARDED_BY(minableCommitmentsCs);
Expand All @@ -50,6 +56,7 @@ class CQuorumBlockProcessor
public:
explicit CQuorumBlockProcessor(CChainState& chainstate, CDeterministicMNManager& dmnman, CEvoDB& evoDb,
CQuorumSnapshotManager& qsnapman);
~CQuorumBlockProcessor();

[[nodiscard]] MessageProcessingResult ProcessMessage(const CNode& peer, std::string_view msg_type, CDataStream& vRecv);

Expand All @@ -75,7 +82,8 @@ class CQuorumBlockProcessor
std::optional<const CBlockIndex*> GetLastMinedCommitmentsByQuorumIndexUntilBlock(Consensus::LLMQType llmqType, const CBlockIndex* pindex, int quorumIndex, size_t cycle) const;
private:
static bool GetCommitmentsFromBlock(const CBlock& block, gsl::not_null<const CBlockIndex*> pindex, std::multimap<Consensus::LLMQType, CFinalCommitment>& ret, BlockValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
bool ProcessCommitment(int nHeight, const uint256& blockHash, const CFinalCommitment& qc, BlockValidationState& state, bool fJustCheck, bool fBLSChecks) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
bool ProcessCommitment(int nHeight, const uint256& blockHash, const CFinalCommitment& qc,
BlockValidationState& state, bool fJustCheck) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
size_t GetNumCommitmentsRequired(const Consensus::LLMQParams& llmqParams, int nHeight) const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
static uint256 GetQuorumBlockHash(const Consensus::LLMQParams& llmqParams, const CChain& active_chain, int nHeight, int quorumIndex) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
};
Expand Down
103 changes: 70 additions & 33 deletions src/llmq/commitment.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,14 @@
#include <evo/specialtx.h>

#include <chainparams.h>
#include <checkqueue.h>
#include <consensus/validation.h>
#include <deploymentstatus.h>
#include <llmq/options.h>
#include <llmq/utils.h>
#include <logging.h>
#include <validation.h>
#include <util/underlying.h>
#include <validation.h>

namespace llmq
{
Expand All @@ -27,6 +28,73 @@ CFinalCommitment::CFinalCommitment(const Consensus::LLMQParams& params, const ui
{
}

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);
if (LogAcceptDebug(BCLog::LLMQ)) {
std::stringstream ss3;
for (const auto& mn : members) {
ss3 << mn->proTxHash.ToString().substr(0, 4) << " | ";
}
LogPrint(BCLog::LLMQ, "CFinalCommitment::%s members[%s] quorumPublicKey[%s] commitmentHash[%s]\n", __func__,
ss3.str(), quorumPublicKey.ToString(), commitmentHash.ToString());
}
if (llmq_params.size == 1) {
LogPrintf("pubkey operator: %s\n", members[0]->pdmnState->pubKeyOperator.Get().ToString());
if (!membersSig.VerifyInsecure(members[0]->pdmnState->pubKeyOperator.Get(), commitmentHash)) {
LogPrint(BCLog::LLMQ, "CFinalCommitment -- q[%s] invalid member signature\n", quorumHash.ToString());
return false;
}
} else {
Comment on lines 54 to 60

@coderabbitai coderabbitai Bot Aug 19, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Guard against out-of-bounds access and remove noisy log

  • When llmq_params.size == 1, the code indexes members[0] without checking members is non-empty. In ProcessBlock(), VerifySignatureAsync is called before structural validation, so a malformed commitment could yield an empty members list here and cause OOB access.
  • LogPrintf of the operator public key on every single-member verify is noisy and can clutter logs.

Apply this diff to make the path safe and quiet:

-    if (llmq_params.size == 1) {
-        LogPrintf("pubkey operator: %s\n", members[0]->pdmnState->pubKeyOperator.Get().ToString());
-        if (!membersSig.VerifyInsecure(members[0]->pdmnState->pubKeyOperator.Get(), commitmentHash)) {
+    if (llmq_params.size == 1) {
+        if (members.empty()) {
+            LogPrint(BCLog::LLMQ, "CFinalCommitment -- q[%s] missing quorum members for size=1\n", quorumHash.ToString());
+            return false;
+        }
+        if (!membersSig.VerifyInsecure(members[0]->pdmnState->pubKeyOperator.Get(), commitmentHash)) {
             LogPrint(BCLog::LLMQ, "CFinalCommitment -- q[%s] invalid member signature\n", quorumHash.ToString());
             return false;
         }
     } else {
📝 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
if (llmq_params.size == 1) {
LogPrintf("pubkey operator: %s\n", members[0]->pdmnState->pubKeyOperator.Get().ToString());
if (!membersSig.VerifyInsecure(members[0]->pdmnState->pubKeyOperator.Get(), commitmentHash)) {
LogPrint(BCLog::LLMQ, "CFinalCommitment -- q[%s] invalid member signature\n", quorumHash.ToString());
return false;
}
} else {
if (llmq_params.size == 1) {
if (members.empty()) {
LogPrint(BCLog::LLMQ, "CFinalCommitment -- q[%s] missing quorum members for size=1\n", quorumHash.ToString());
return false;
}
if (!membersSig.VerifyInsecure(members[0]->pdmnState->pubKeyOperator.Get(), commitmentHash)) {
LogPrint(BCLog::LLMQ, "CFinalCommitment -- q[%s] invalid member signature\n", quorumHash.ToString());
return false;
}
} else {
🤖 Prompt for AI Agents
In src/llmq/commitment.cpp around lines 54 to 60, the branch for
llmq_params.size == 1 accesses members[0] without ensuring members is non-empty
and emits a noisy LogPrintf; add a guard that checks members.empty() (or
members.size() < 1) and return false if so to prevent out-of-bounds access, and
remove the LogPrintf of the operator public key so only the signature
verification remains (keep the membersSig.VerifyInsecure call and its error
path).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I can't think of case when members.empty(). Anyway, llmq_params.size == 1 is used for regtest only, won't cause any public-net failure

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@knst, thank you for the context! You're absolutely right that if llmq_params.size == 1 is regtest-only, the practical risk is minimal since it's a controlled testing environment.

Given your domain expertise, I trust your assessment about when members.empty() could occur. The bounds check might be defensive programming overkill for this specific case, especially in a performance-focused PR.

However, what about the LogPrintf removal? That seems like it could still create noisy logs during regtest runs, regardless of the bounds issue. Would you consider removing just that line to keep the logs cleaner?

I understand if you prefer to keep the scope focused on the main performance improvements rather than these edge cases.

🐰💭 Contemplating the balance between defensive programming and practical risk assessment...


✏️ Learnings added
Learnt from: knst
PR: dashpay/dash#6692
File: src/llmq/commitment.cpp:54-60
Timestamp: 2025-08-19T15:08:00.791Z
Learning: In Dash Core, llmq_params.size == 1 is used only for regtest environments, not on public networks, which significantly reduces the risk profile of code paths specific to single-member quorums.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
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.

std::vector<CBLSPublicKey> memberPubKeys;
for (const auto i : irange::range(members.size())) {
if (!signers[i]) {
continue;
}
memberPubKeys.emplace_back(members[i]->pdmnState->pubKeyOperator.Get());
}
Comment on lines 61 to 67

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we should probably reserve here; we know that for a qc to be valid, we need at least 80% (llmq_50_60 has minsize 40). As such, we can pretty safely reserve the entire size. For the LLMQ_400 this may reduce the number of allocations from ~10-ish reallocations to just a single one.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Please notice, that this code is not new,but moved in commit 219f223e983bb50d4e39e771c35a6625544cfb6b

I may implement this optimization in follow-up PR

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);
} else {
if (!membersSig.VerifySecureAggregated(memberPubKeys, commitmentHash)) {
LogPrint(BCLog::LLMQ, "%s\n", members_id_string);
return false;
}
}
}
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 {
if (!quorumSig.VerifyInsecure(quorumPublicKey, commitmentHash)) {
LogPrint(BCLog::LLMQ, "%s\n", qsig_id_string);
return false;
}
}
return true;
}


bool CFinalCommitment::Verify(CDeterministicMNManager& dmnman, CQuorumSnapshotManager& qsnapman,
gsl::not_null<const CBlockIndex*> pQuorumBaseBlockIndex, bool checkSigs) const
{
Expand Down Expand Up @@ -106,38 +174,7 @@ bool CFinalCommitment::Verify(CDeterministicMNManager& dmnman, CQuorumSnapshotMa

// sigs are only checked when the block is processed
if (checkSigs) {
uint256 commitmentHash = BuildCommitmentHash(llmq_params.type, quorumHash, validMembers, quorumPublicKey, quorumVvecHash);
if (LogAcceptDebug(BCLog::LLMQ)) {
std::stringstream ss3;
for (const auto &mn: members) {
ss3 << mn->proTxHash.ToString().substr(0, 4) << " | ";
}
LogPrint(BCLog::LLMQ, "CFinalCommitment::%s members[%s] quorumPublicKey[%s] commitmentHash[%s]\n",
__func__, ss3.str(), quorumPublicKey.ToString(), commitmentHash.ToString());
}
if (llmq_params.size == 1) {
LogPrintf("pubkey operator: %s\n", members[0]->pdmnState->pubKeyOperator.Get().ToString());
if (!membersSig.VerifyInsecure(members[0]->pdmnState->pubKeyOperator.Get(), commitmentHash)) {
LogPrint(BCLog::LLMQ, "CFinalCommitment -- q[%s] invalid member signature\n", quorumHash.ToString());
return false;
}
} else {
std::vector<CBLSPublicKey> memberPubKeys;
for (const auto i : irange::range(members.size())) {
if (!signers[i]) {
continue;
}
memberPubKeys.emplace_back(members[i]->pdmnState->pubKeyOperator.Get());
}

if (!membersSig.VerifySecureAggregated(memberPubKeys, commitmentHash)) {
LogPrint(BCLog::LLMQ, "CFinalCommitment -- q[%s] invalid aggregated members signature\n",
quorumHash.ToString());
return false;
}
}
if (!quorumSig.VerifyInsecure(quorumPublicKey, commitmentHash)) {
LogPrint(BCLog::LLMQ, "CFinalCommitment -- q[%s] invalid quorum signature\n", quorumHash.ToString());
if (!VerifySignatureAsync(dmnman, qsnapman, pQuorumBaseBlockIndex, nullptr)) {
return false;
}
}
Expand Down
8 changes: 8 additions & 0 deletions src/llmq/commitment.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,16 @@ class CBlockIndex;
class CDeterministicMNManager;
class ChainstateManager;
class TxValidationState;
template <typename T>
class CCheckQueueControl;

namespace llmq
{
class CQuorumSnapshotManager;

namespace utils {
struct BlsCheck;
} // namespace utils
// This message is an aggregation of all received premature commitments and only valid if
// enough (>=threshold) premature commitments were aggregated
// This is mined on-chain as part of TRANSACTION_QUORUM_COMMITMENT
Expand Down Expand Up @@ -67,6 +72,9 @@ class CFinalCommitment
return int(std::count(validMembers.begin(), validMembers.end(), true));
}

bool VerifySignatureAsync(CDeterministicMNManager& dmnman, CQuorumSnapshotManager& qsnapman,
gsl::not_null<const CBlockIndex*> pQuorumBaseBlockIndex,
CCheckQueueControl<utils::BlsCheck>* queue_control) const;
bool Verify(CDeterministicMNManager& dmnman, CQuorumSnapshotManager& qsnapman,
gsl::not_null<const CBlockIndex*> pQuorumBaseBlockIndex, bool checkSigs) const;
bool VerifyNull() const;
Expand Down
5 changes: 5 additions & 0 deletions src/llmq/options.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@ enum class QvvecSyncMode {
OnlyIfTypeMember = 1,
};

/** Maximum number of dedicated script-checking threads allowed */
static const int MAX_BLSCHECK_THREADS = 33;
/** -parbls default (number of bls-checking threads, 0 = auto) */
static const int DEFAULT_BLSCHECK_THREADS = 0;

static constexpr bool DEFAULT_ENABLE_QUORUM_DATA_RECOVERY{true};

// If true, we will connect to all new quorums and watch their communication
Expand Down
20 changes: 20 additions & 0 deletions src/llmq/utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -940,6 +940,26 @@ void AddQuorumProbeConnections(const Consensus::LLMQParams& llmqParams, CConnman
}
}

bool BlsCheck::operator()()
{
if (m_pubkeys.size() > 1) {
if (!m_sig.VerifySecureAggregated(m_pubkeys, m_msg_hash)) {
LogPrint(BCLog::LLMQ, "%s\n", m_id_string);
return false;
}
} else if (m_pubkeys.size() == 1) {
if (!m_sig.VerifyInsecure(m_pubkeys.back(), m_msg_hash)) {
LogPrint(BCLog::LLMQ, "%s\n", m_id_string);
return false;
}
} else {
// we should not get there ever
LogPrint(BCLog::LLMQ, "%s - no public keys are provided\n", m_id_string);
return false;
}
return true;
}

template <typename CacheType>
void InitQuorumsCache(CacheType& cache, bool limit_by_connections)
{
Expand Down
29 changes: 29 additions & 0 deletions src/llmq/utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#ifndef BITCOIN_LLMQ_UTILS_H
#define BITCOIN_LLMQ_UTILS_H

#include <bls/bls.h>
#include <gsl/pointers.h>
#include <llmq/params.h>
#include <saltedhasher.h>
Expand Down Expand Up @@ -56,8 +57,36 @@ void AddQuorumProbeConnections(const Consensus::LLMQParams& llmqParams, CConnman
const CSporkManager& sporkman, const CDeterministicMNList& tip_mn_list,
gsl::not_null<const CBlockIndex*> pQuorumBaseBlockIndex, const uint256& myProTxHash);

struct BlsCheck {
CBLSSignature m_sig;
std::vector<CBLSPublicKey> m_pubkeys;
uint256 m_msg_hash;
std::string m_id_string;

BlsCheck() = default;

BlsCheck(CBLSSignature sig, std::vector<CBLSPublicKey> pubkeys, uint256 msg_hash, std::string id_string) :
m_sig(sig),
m_pubkeys(pubkeys),
m_msg_hash(msg_hash),
m_id_string(id_string)
{
}

void swap(BlsCheck& obj)
{
std::swap(m_sig, obj.m_sig);
std::swap(m_pubkeys, obj.m_pubkeys);
std::swap(m_msg_hash, obj.m_msg_hash);
std::swap(m_id_string, obj.m_id_string);
}

bool operator()();
};

template <typename CacheType>
void InitQuorumsCache(CacheType& cache, bool limit_by_connections = true);

} // namespace utils
} // namespace llmq

Expand Down
Loading