Skip to content

perf: improve signing latency by collecting lazy signatures under lock #6911

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
+34 −19
Open

perf: improve signing latency by collecting lazy signatures under lock #6911

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
53 changes: 34 additions & 19 deletions src/llmq/signing_shares.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -592,8 +592,11 @@ std::shared_ptr<CRecoveredSig> CSigSharesManager::TryRecoverSig(const CQuorum& q
return nullptr;
}

std::vector<CBLSSignature> sigSharesForRecovery;
// Collect lazy signatures (cheap copy) under lock, then materialize outside lock
Copy link
Copy Markdown

@thepastaclaw thepastaclaw Mar 17, 2026

Choose a reason for hiding this comment

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

💬 Nitpick: "cheap copy" is imprecise — state what it avoids

CBLSLazyWrapper's copy constructor acquires a mutex on the source and deep-copies a 96-byte array plus potentially a full bls::G2Element (if already materialized). It IS much cheaper than Get() on the hot path (unmaterialized objects skip BLS point decompression), but "cheap" without qualification could mislead a reader into thinking it's trivial. The same comment pattern appears at lines 620 and 633.

💡 Suggested change
Suggested change
// Collect lazy signatures (cheap copy) under lock, then materialize outside lock
// Collect lazy signatures (avoids BLS deserialization) under lock, then materialize outside lock

source: ['claude']

std::vector<CBLSLazySignature> lazySignatures;
std::vector<CBLSId> idsForRecovery;
bool isSingleNode = false;
Copy link
Copy Markdown

@thepastaclaw thepastaclaw Mar 17, 2026

Choose a reason for hiding this comment

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

💬 Nitpick: isSingleNode should be isSingleMember for consistency

The codebase consistently uses 'member' terminology: is_single_member() (params.h:124), getQuorumMember(), quorum.members. The new variable isSingleNode breaks this convention. The pre-existing log strings ("single-node signature") use 'node' but those predate this PR; the new variable should align with the API it guards (is_single_member()).

💡 Suggested change
Suggested change
bool isSingleNode = false;
bool isSingleMember = false;
// ... (also update line 622: isSingleMember = true; and line 644: if (isSingleMember))

source: ['claude']

Copy link
Copy Markdown

@UdjinM6 UdjinM6 Apr 9, 2026

Choose a reason for hiding this comment

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

Suggested change
bool isSingleNode = false;


{
LOCK(cs);

Expand All @@ -603,7 +606,6 @@ std::shared_ptr<CRecoveredSig> CSigSharesManager::TryRecoverSig(const CQuorum& q
return nullptr;
}

std::shared_ptr<CRecoveredSig> singleMemberRecoveredSig;
if (quorum.params.is_single_member()) {
if (sigSharesForSignHash->empty()) {
LogPrint(BCLog::LLMQ_SIGS, /* Continued */
Expand All @@ -613,29 +615,42 @@ std::shared_ptr<CRecoveredSig> CSigSharesManager::TryRecoverSig(const CQuorum& q
return nullptr;
}
const auto& sigShare = sigSharesForSignHash->begin()->second;
CBLSSignature recoveredSig = sigShare.sigShare.Get();
// Copy the lazy signature (cheap), materialize later outside lock
lazySignatures.emplace_back(sigShare.sigShare);
isSingleNode = true;
LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- recover single-node signature. id=%s, msgHash=%s\n",
__func__, id.ToString(), msgHash.ToString());
} else {
// Collect lazy signatures and IDs under lock (cheap operations)
lazySignatures.reserve((size_t) quorum.params.threshold);
idsForRecovery.reserve((size_t) quorum.params.threshold);
for (auto it = sigSharesForSignHash->begin();
it != sigSharesForSignHash->end() && lazySignatures.size() < size_t(quorum.params.threshold);
++it) {
const auto& sigShare = it->second;
lazySignatures.emplace_back(sigShare.sigShare); // Cheap copy of lazy wrapper
idsForRecovery.emplace_back(quorum.members[sigShare.getQuorumMember()]->proTxHash);
}

singleMemberRecoveredSig = std::make_shared<CRecoveredSig>(quorum.params.type, quorum.qc->quorumHash, id, msgHash,
recoveredSig);
// check if we can recover the final signature
if (lazySignatures.size() < size_t(quorum.params.threshold)) {
return nullptr;
}
}
} // Release lock before expensive materialization

sigSharesForRecovery.reserve((size_t) quorum.params.threshold);
idsForRecovery.reserve((size_t) quorum.params.threshold);
for (auto it = sigSharesForSignHash->begin(); it != sigSharesForSignHash->end() && sigSharesForRecovery.size() < size_t(quorum.params.threshold); ++it) {
const auto& sigShare = it->second;
sigSharesForRecovery.emplace_back(sigShare.sigShare.Get());
idsForRecovery.emplace_back(quorum.members[sigShare.getQuorumMember()]->proTxHash);
}
// Materialize signatures outside the critical section (expensive BLS operations)
if (quorum.params.is_single_member()) {
CBLSSignature recoveredSig = lazySignatures[0].Get();
return std::make_shared<CRecoveredSig>(quorum.params.type, quorum.qc->quorumHash, id, msgHash,
recoveredSig);
}

// check if we can recover the final signature
if (sigSharesForRecovery.size() < size_t(quorum.params.threshold)) {
return nullptr;
}
if (quorum.params.is_single_member()) {
return singleMemberRecoveredSig; // end of single-quorum processing
}
// Multi-node case: materialize all signatures
std::vector<CBLSSignature> sigSharesForRecovery;
sigSharesForRecovery.reserve(lazySignatures.size());
for (const auto& lazySig : lazySignatures) {
sigSharesForRecovery.emplace_back(lazySig.Get()); // Expensive, but outside lock
Copy link
Copy Markdown
Collaborator

@knst knst Oct 22, 2025

Choose a reason for hiding this comment

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

Is copy of bls signature indeed that expensive?

Maybe there's a bug in our bls's wrapper which calls "is-valid" or "serialize&deserialize" for each constructor copy?

Just copy of 500bytes are not that expensive; I think we should improve performance of our bls-wrapper rather than do this refactoring... Do I miss anything?

Copy link
Copy Markdown
Member Author

@PastaPastaPasta PastaPastaPasta Oct 22, 2025

Choose a reason for hiding this comment

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

It's not a copy; look into the implementation of .Get()

Copy link
Copy Markdown
Member Author

@PastaPastaPasta PastaPastaPasta Oct 22, 2025

Choose a reason for hiding this comment

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

See:

dash/src/bls/bls.h

Lines 488 to 508 in a488c8d

const BLSObject& Get() const
{
std::unique_lock<std::mutex> l(mutex);
static BLSObject invalidObj;
if (!bufValid && !objInitialized) {
return invalidObj;
}
if (!objInitialized) {
obj.SetBytes(vecBytes, bufLegacyScheme);
if (!obj.IsValid()) {
bufValid = false;
return invalidObj;
}
if (!obj.CheckMalleable(vecBytes, bufLegacyScheme)) {
bufValid = false;
return invalidObj;
}
objInitialized = true;
}
return obj;
}

I'd love ways to increase performance here, but it is quite expensive as is.

}

// now recover it
Expand Down
Loading