perf: using asynchronous worker to validate BLS signatures in quorum commitments #6692
Conversation
WalkthroughThe changes add asynchronous BLS signature verification to quorum commitment processing by introducing a 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
⏰ Context from checks skipped due to timeout of 90000ms (10)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (2)
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 }
🛑 Comments failed to post (1)
src/llmq/commitment.h (1)
34-37:
⚠️ Potential issueFix 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.
|
just to be sure that CCheckQueue works at all (and not just return true) I modified code a bit and it failed as expected: diff --git a/src/llmq/commitment.cpp b/src/llmq/commitment.cpp
index c2cbe9b35c..b242b4c988 100644
--- a/src/llmq/commitment.cpp
+++ b/src/llmq/commitment.cpp
@@ -69,7 +69,12 @@ bool CFinalCommitment::VerifySignatureAsync(CDeterministicMNManager& dmnman, CQu
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);
+ static int counter{0};
+ if (++counter == 42) {
+ vChecks.emplace_back(quorumSig, memberPubKeys, commitmentHash, members_id_string);
+ } else {
+ vChecks.emplace_back(membersSig, memberPubKeys, commitmentHash, members_id_string);
+ }
queue_control->Add(vChecks);
} else {
if (!membersSig.VerifySecureAggregated(memberPubKeys, commitmentHash)) {error: state is expected to don't be set, same issue on |
| @@ -52,8 +53,18 @@ CQuorumBlockProcessor::CQuorumBlockProcessor(CChainState& chainstate, CDetermini | |||
| m_qsnapman(qsnapman) | |||
| { | |||
| utils::InitQuorumsCache(mapHasMinedCommitmentCache); | |||
|
|
|||
| int qc_threads = gArgs.GetIntArg("-par", DEFAULT_SCRIPTCHECK_THREADS); | |||
| // -par=-n means "leave n cores free" (number of cores - n - 1 validator threads) | ||
| qc_threads += GetNumCores(); | ||
| } | ||
| m_bls_queue.StartWorkerThreads(qc_threads); |
There was a problem hiding this comment.
The original script check logic has some additional limits
// Subtract 1 because the main thread counts towards the par threads
script_threads = std::max(script_threads - 1, 0);
// Number of script-checking threads <= MAX_SCRIPTCHECK_THREADS
script_threads = std::min(script_threads, MAX_SCRIPTCHECK_THREADS);
Shouldn't we have smth like that here too?
Issue being fixed or feature implemented
During blocks validation, quorum commitments are processed in single thread.
While some blocks have up to 32 commitments (blocks which have rotation quorums commitment), each quorum commitment have hundreds public keys to validate and 2 signature (quorum signature and public key signature). It takes up to 30% in total indexing time and up to 1 second for heavy blocks.
What was done?
CCheckQueuewhich is used for validation ECDSA signatures is used now for validation of BLS signatures in quorum commitments.Quorum signature and members signatures are validated simultaneously now which makes performance improvement even for blocks which has only 1 commitment.
How Has This Been Tested?
Invalidated + reconsidered 15k blocks (~1 months worth)
This PR makes validation of Quorum Commitment 3.5x times faster; overall indexing 25% faster on my 12 cores environment.
PR:
develop:
Breaking Changes
N/A
Checklist: