Skip to content
Open
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
53 changes: 36 additions & 17 deletions src/llmq/signing_shares.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -768,8 +768,11 @@ void CSigSharesManager::TryRecoverSig(PeerManager& peerman, const CQuorumCPtr& q
return;
}

std::vector<CBLSSignature> sigSharesForRecovery;
// Collect lazy signatures (cheap copy) under lock, then materialize outside lock
std::vector<CBLSLazySignature> lazySignatures;
std::vector<CBLSId> idsForRecovery;
bool isSingleNode = false;

{
LOCK(cs);

Expand All @@ -788,28 +791,44 @@ void CSigSharesManager::TryRecoverSig(PeerManager& peerman, const CQuorumCPtr& q
return;
}
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);
}

auto rs = std::make_shared<CRecoveredSig>(quorum->params.type, quorum->qc->quorumHash, id, msgHash,
recoveredSig);
sigman.ProcessRecoveredSig(rs, peerman);
return; // end of single-quorum processing
// check if we can recover the final signature
if (lazySignatures.size() < size_t(quorum->params.threshold)) {
return;
}
}
} // 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 (isSingleNode) {
CBLSSignature recoveredSig = lazySignatures[0].Get();
auto rs = std::make_shared<CRecoveredSig>(quorum->params.type, quorum->qc->quorumHash, id, msgHash,
recoveredSig);
sigman.ProcessRecoveredSig(rs, peerman);
return; // end of single-quorum processing
}

// check if we can recover the final signature
if (sigSharesForRecovery.size() < size_t(quorum->params.threshold)) {
return;
}
// 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
Collaborator

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
Member Author

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
Member Author

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