From 465af48e835cf335c7d780b4483208ea400e9805 Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Tue, 7 Apr 2020 06:24:46 +0200 Subject: [PATCH 1/4] Improve logging in LLMQ sig handling --- src/bls/bls_batchverifier.h | 5 +++++ src/llmq/quorums_instantsend.cpp | 11 +++++++++++ src/llmq/quorums_signing_shares.cpp | 9 ++++++--- 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/src/bls/bls_batchverifier.h b/src/bls/bls_batchverifier.h index 5e7259794d0a..4b7173dd7e2f 100644 --- a/src/bls/bls_batchverifier.h +++ b/src/bls/bls_batchverifier.h @@ -63,6 +63,11 @@ class CBLSBatchVerifier messagesBySource.clear(); } + size_t GetUniqueSourceCount() const + { + return messagesBySource.size(); + } + void Verify() { std::map> byMessageHash; diff --git a/src/llmq/quorums_instantsend.cpp b/src/llmq/quorums_instantsend.cpp index 6e7a7bbf2c10..1273a12ebb62 100644 --- a/src/llmq/quorums_instantsend.cpp +++ b/src/llmq/quorums_instantsend.cpp @@ -19,6 +19,8 @@ #include #endif +#include + #include #include @@ -787,6 +789,8 @@ std::unordered_set CInstantSendManager::ProcessPendingInstantSendLocks( CBLSBatchVerifier batchVerifier(false, true, 8); std::unordered_map> recSigs; + size_t verifyCount = 0; + size_t alreadyVerified = 0; for (const auto& p : pend) { auto& hash = p.first; auto nodeId = p.second.first; @@ -805,6 +809,7 @@ std::unordered_set CInstantSendManager::ProcessPendingInstantSendLocks( // no need to verify an ISLOCK if we already have verified the recovered sig that belongs to it if (quorumSigningManager->HasRecoveredSig(llmqType, id, islock.txid)) { + alreadyVerified++; continue; } @@ -815,6 +820,7 @@ std::unordered_set CInstantSendManager::ProcessPendingInstantSendLocks( } uint256 signHash = CLLMQUtils::BuildSignHash(llmqType, quorum->qc.quorumHash, id, islock.txid); batchVerifier.PushMessage(nodeId, hash, signHash, islock.sig.Get(), quorum->qc.quorumPublicKey); + verifyCount++; // We can reconstruct the CRecoveredSig objects from the islock and pass it to the signing manager, which // avoids unnecessary double-verification of the signature. We however only do this when verification here @@ -832,7 +838,12 @@ std::unordered_set CInstantSendManager::ProcessPendingInstantSendLocks( } } + cxxtimer::Timer verifyTimer(true); batchVerifier.Verify(); + verifyTimer.stop(); + + LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- verified locks. count=%d, alreadyVerified=%d, vt=%d, nodes=%d\n", __func__, + verifyCount, alreadyVerified, verifyTimer.count(), batchVerifier.GetUniqueSourceCount()); std::unordered_set badISLocks; diff --git a/src/llmq/quorums_signing_shares.cpp b/src/llmq/quorums_signing_shares.cpp index 9a4d96c8ff75..d2eaa2a515cd 100644 --- a/src/llmq/quorums_signing_shares.cpp +++ b/src/llmq/quorums_signing_shares.cpp @@ -652,6 +652,7 @@ bool CSigSharesManager::ProcessPendingSigShares(CConnman& connman) // which are not craftable by individual entities, making the rogue public key attack impossible CBLSBatchVerifier batchVerifier(false, true); + cxxtimer::Timer prepareTimer(true); size_t verifyCount = 0; for (auto& p : sigSharesByNodes) { auto nodeId = p.first; @@ -684,12 +685,13 @@ bool CSigSharesManager::ProcessPendingSigShares(CConnman& connman) verifyCount++; } } + prepareTimer.stop(); cxxtimer::Timer verifyTimer(true); batchVerifier.Verify(); verifyTimer.stop(); - LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- verified sig shares. count=%d, vt=%d, nodes=%d\n", __func__, verifyCount, verifyTimer.count(), sigSharesByNodes.size()); + LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- verified sig shares. count=%d, pt=%d, vt=%d, nodes=%d\n", __func__, verifyCount, prepareTimer.count(), verifyTimer.count(), sigSharesByNodes.size()); for (auto& p : sigSharesByNodes) { auto nodeId = p.first; @@ -1036,10 +1038,11 @@ void CSigSharesManager::CollectSigSharesToSend(std::unordered_map= p.second.nextAttemptTime) { p.second.nextAttemptTime = curTime + SEND_FOR_RECOVERY_TIMEOUT; auto dmn = SelectMemberForRecovery(p.second.quorum, p.second.sigShare.id, p.second.attempt); - LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- sending to %s, signHash=%s\n", __func__, - dmn->proTxHash.ToString(), p.second.sigShare.GetSignHash().ToString()); p.second.attempt++; + LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- signHash=%s, sending to %s, attempt=%d\n", __func__, + p.second.sigShare.GetSignHash().ToString(), dmn->proTxHash.ToString(), p.second.attempt); + auto it = proTxToNode.find(dmn->proTxHash); if (it == proTxToNode.end()) { continue; From d856fd140756a1d36c788118a3fde6c09177df36 Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Tue, 7 Apr 2020 06:25:17 +0200 Subject: [PATCH 2/4] Use salted hasher for pendingInstantSendLocks --- src/llmq/quorums_instantsend.cpp | 2 +- src/llmq/quorums_instantsend.h | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/llmq/quorums_instantsend.cpp b/src/llmq/quorums_instantsend.cpp index 1273a12ebb62..755bd964a96f 100644 --- a/src/llmq/quorums_instantsend.cpp +++ b/src/llmq/quorums_instantsend.cpp @@ -782,7 +782,7 @@ bool CInstantSendManager::ProcessPendingInstantSendLocks() return true; } -std::unordered_set CInstantSendManager::ProcessPendingInstantSendLocks(int signHeight, const std::unordered_map>& pend, bool ban) +std::unordered_set CInstantSendManager::ProcessPendingInstantSendLocks(int signHeight, const std::unordered_map, StaticSaltedHasher>& pend, bool ban) { auto llmqType = Params().GetConsensus().llmqTypeInstantSend; diff --git a/src/llmq/quorums_instantsend.h b/src/llmq/quorums_instantsend.h index a28f7a41a36c..e3d7dce0ad8a 100644 --- a/src/llmq/quorums_instantsend.h +++ b/src/llmq/quorums_instantsend.h @@ -97,7 +97,7 @@ class CInstantSendManager : public CRecoveredSigsListener std::unordered_map txToCreatingInstantSendLocks; // Incoming and not verified yet - std::unordered_map> pendingInstantSendLocks; + std::unordered_map, StaticSaltedHasher> pendingInstantSendLocks; // TXs which are neither IS locked nor ChainLocked. We use this to determine for which TXs we need to retry IS locking // of child TXs @@ -137,7 +137,7 @@ class CInstantSendManager : public CRecoveredSigsListener void ProcessMessageInstantSendLock(CNode* pfrom, const CInstantSendLock& islock, CConnman& connman); bool PreVerifyInstantSendLock(NodeId nodeId, const CInstantSendLock& islock, bool& retBan); bool ProcessPendingInstantSendLocks(); - std::unordered_set ProcessPendingInstantSendLocks(int signHeight, const std::unordered_map>& pend, bool ban); + std::unordered_set ProcessPendingInstantSendLocks(int signHeight, const std::unordered_map, StaticSaltedHasher>& pend, bool ban); void ProcessInstantSendLock(NodeId from, const uint256& hash, const CInstantSendLock& islock); void UpdateWalletTransaction(const CTransactionRef& tx, const CInstantSendLock& islock); From 454fae3bda43818d0a7f1fc2bbfe9bade1905407 Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Tue, 7 Apr 2020 06:26:53 +0200 Subject: [PATCH 3/4] Only process 32 IS locks at a time in ProcessPendingInstantSendLocks --- src/llmq/quorums_instantsend.cpp | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/llmq/quorums_instantsend.cpp b/src/llmq/quorums_instantsend.cpp index 755bd964a96f..355650517058 100644 --- a/src/llmq/quorums_instantsend.cpp +++ b/src/llmq/quorums_instantsend.cpp @@ -731,7 +731,18 @@ bool CInstantSendManager::ProcessPendingInstantSendLocks() { LOCK(cs); - pend = std::move(pendingInstantSendLocks); + // only process a max 32 locks at a time to avoid duplicate verification of recovered signatures which have been + // verified by CSigningManager in parallel + const size_t maxCount = 32; + if (pendingInstantSendLocks.size() <= maxCount) { + pend = std::move(pendingInstantSendLocks); + } else { + while (pend.size() < maxCount) { + auto it = pendingInstantSendLocks.begin(); + pend.emplace(it->first, std::move(it->second)); + pendingInstantSendLocks.erase(it); + } + } } if (pend.empty()) { From f142fff881323f2ea8aa314bf49630be1a187e6c Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Tue, 7 Apr 2020 06:30:10 +0200 Subject: [PATCH 4/4] Skip verification of recovered sigs that were reconstructed in InstantSend Also don't request them via getdata --- src/llmq/quorums_signing.cpp | 33 ++++++++++++++++++++++++++------- src/llmq/quorums_signing.h | 2 +- 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/src/llmq/quorums_signing.cpp b/src/llmq/quorums_signing.cpp index 79c1b7fc1caf..1d28ee61f2ac 100644 --- a/src/llmq/quorums_signing.cpp +++ b/src/llmq/quorums_signing.cpp @@ -446,8 +446,17 @@ CSigningManager::CSigningManager(CDBWrapper& llmqDb, bool fMemory) : bool CSigningManager::AlreadyHave(const CInv& inv) { - LOCK(cs); - return inv.type == MSG_QUORUM_RECOVERED_SIG && db.HasRecoveredSigForHash(inv.hash); + if (inv.type != MSG_QUORUM_RECOVERED_SIG) { + return false; + } + { + LOCK(cs); + if (pendingReconstructedRecoveredSigs.count(inv.hash)) { + return true; + } + } + + return db.HasRecoveredSigForHash(inv.hash); } bool CSigningManager::GetRecoveredSigForGetData(const uint256& hash, CRecoveredSig& ret) @@ -492,6 +501,12 @@ void CSigningManager::ProcessMessageRecoveredSig(CNode* pfrom, const CRecoveredS CLLMQUtils::BuildSignHash(recoveredSig).ToString(), recoveredSig.id.ToString(), recoveredSig.msgHash.ToString(), pfrom->GetId()); LOCK(cs); + if (pendingReconstructedRecoveredSigs.count(recoveredSig.GetHash())) { + // no need to perform full verification + LogPrint(BCLog::LLMQ, "CSigningManager::%s -- already pending reconstructed sig, signHash=%s, id=%s, msgHash=%s, node=%d\n", __func__, + CLLMQUtils::BuildSignHash(recoveredSig).ToString(), recoveredSig.id.ToString(), recoveredSig.msgHash.ToString(), pfrom->GetId()); + return; + } pendingRecoveredSigs[pfrom->GetId()].emplace_back(recoveredSig); } @@ -587,13 +602,13 @@ void CSigningManager::CollectPendingRecoveredSigsToVerify( void CSigningManager::ProcessPendingReconstructedRecoveredSigs() { - decltype(pendingReconstructedRecoveredSigs) l; + decltype(pendingReconstructedRecoveredSigs) m; { LOCK(cs); - l = std::move(pendingReconstructedRecoveredSigs); + m = std::move(pendingReconstructedRecoveredSigs); } - for (auto& p : l) { - ProcessRecoveredSig(-1, p.first, p.second, *g_connman); + for (auto& p : m) { + ProcessRecoveredSig(-1, p.second.first, p.second.second, *g_connman); } } @@ -709,6 +724,8 @@ void CSigningManager::ProcessRecoveredSig(NodeId nodeId, const CRecoveredSig& re } db.WriteRecoveredSig(recoveredSig); + + pendingReconstructedRecoveredSigs.erase(recoveredSig.GetHash()); } CInv inv(MSG_QUORUM_RECOVERED_SIG, recoveredSig.GetHash()); @@ -726,7 +743,9 @@ void CSigningManager::ProcessRecoveredSig(NodeId nodeId, const CRecoveredSig& re void CSigningManager::PushReconstructedRecoveredSig(const llmq::CRecoveredSig& recoveredSig, const llmq::CQuorumCPtr& quorum) { LOCK(cs); - pendingReconstructedRecoveredSigs.emplace_back(recoveredSig, quorum); + pendingReconstructedRecoveredSigs.emplace(std::piecewise_construct, + std::forward_as_tuple(recoveredSig.GetHash()), + std::forward_as_tuple(recoveredSig, quorum)); } void CSigningManager::TruncateRecoveredSig(Consensus::LLMQType llmqType, const uint256& id) diff --git a/src/llmq/quorums_signing.h b/src/llmq/quorums_signing.h index 99331b13e55c..6ec050f48e6d 100644 --- a/src/llmq/quorums_signing.h +++ b/src/llmq/quorums_signing.h @@ -126,7 +126,7 @@ class CSigningManager // Incoming and not verified yet std::unordered_map> pendingRecoveredSigs; - std::list> pendingReconstructedRecoveredSigs; + std::unordered_map, StaticSaltedHasher> pendingReconstructedRecoveredSigs; // must be protected by cs FastRandomContext rnd;