From 8c29259f226bddf9d21503b687d80c1625a6f48d Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 14 Apr 2021 01:03:42 -0400 Subject: [PATCH 01/10] avoid deprecated C headers --- src/bls/bls.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/bls/bls.cpp b/src/bls/bls.cpp index df5fe5caf0ee..388914084305 100644 --- a/src/bls/bls.cpp +++ b/src/bls/bls.cpp @@ -11,8 +11,8 @@ #include #endif -#include -#include +#include +#include static std::unique_ptr pSchemeLegacy(new bls::LegacySchemeMPL); static std::unique_ptr pScheme(new bls::BasicSchemeMPL); From 1d53c95c3ab4b23bc869bd15348731af5276c4be Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 14 Apr 2021 01:03:55 -0400 Subject: [PATCH 02/10] remove unused header --- src/bls/bls.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/bls/bls.cpp b/src/bls/bls.cpp index 388914084305..62262e37dd46 100644 --- a/src/bls/bls.cpp +++ b/src/bls/bls.cpp @@ -5,7 +5,6 @@ #include #include -#include #ifndef BUILD_BITCOIN_INTERNAL #include From be38dfcb5fd071fdbc84ed5bf78450a3ca3366cf Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 14 Apr 2021 01:06:39 -0400 Subject: [PATCH 03/10] typos --- src/bls/bls_worker.cpp | 4 ++-- src/bls/bls_worker.h | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/bls/bls_worker.cpp b/src/bls/bls_worker.cpp index b292dc2adacc..4d9f4be73654 100644 --- a/src/bls/bls_worker.cpp +++ b/src/bls/bls_worker.cpp @@ -404,13 +404,13 @@ struct ContributionVerifier { BLSVerificationVectorPtr vvec; CBLSSecretKey skShare; - // starts with 0 and is incremented if either vvec or skShare aggregation finishs. If it reaches 2, we know + // starts with 0 and is incremented if either vvec or skShare aggregation finishes. If it reaches 2, we know // that aggregation for this batch is fully done. We can then start verification. std::unique_ptr > aggDone; // we can't directly update a vector in parallel // as vector is not thread safe (uses bitsets internally) - // so we must use vector temporarely and concatenate/convert + // so we must use vector temporarily and concatenate/convert // each batch result into a final vector std::vector verifyResults; }; diff --git a/src/bls/bls_worker.h b/src/bls/bls_worker.h index b8cc8c677b45..24ecc2f7f1ef 100644 --- a/src/bls/bls_worker.h +++ b/src/bls/bls_worker.h @@ -69,7 +69,7 @@ class CBLSWorker // The result is in the following form: // [ a1+a2+a3+a4, b1+b2+b3+b4, c1+c2+c3+c4, d1+d2+d3+d4] // Multiple things can be parallelized here. For example, all 4 entries in the result vector can be calculated in parallel - // Also, each individual vector can be split into multiple batches and aggregating the batches can also be paralellized. + // Also, each individual vector can be split into multiple batches and aggregating the batches can also be parallelized. void AsyncBuildQuorumVerificationVector(const std::vector& vvecs, size_t start, size_t count, bool parallel, std::function doneCallback); @@ -82,7 +82,7 @@ class CBLSWorker // Inputs are in the following form: // [a, b, c, d], // The result is simply a+b+c+d - // Aggregation is paralellized by splitting up the input vector into multiple batches and then aggregating the individual batch results + // Aggregation is parallelized by splitting up the input vector into multiple batches and then aggregating the individual batch results void AsyncAggregateSecretKeys(const BLSSecretKeyVector& secKeys, size_t start, size_t count, bool parallel, std::function doneCallback); @@ -123,7 +123,7 @@ class CBLSWorker std::future AsyncVerifyContributionShare(const CBLSId& forId, const BLSVerificationVectorPtr& vvec, const CBLSSecretKey& skContribution); - // Non paralellized verification of a single contribution + // Non parallelized verification of a single contribution bool VerifyContributionShare(const CBLSId& forId, const BLSVerificationVectorPtr& vvec, const CBLSSecretKey& skContribution); // Simple verification of vectors. Checks x.IsValid() for every entry and checks for duplicate entries From 085a1799edd7425d090df8f1102cb0f5f219665b Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 14 Apr 2021 01:07:59 -0400 Subject: [PATCH 04/10] unused include --- src/bls/bls_worker.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/bls/bls_worker.h b/src/bls/bls_worker.h index 24ecc2f7f1ef..f1002165dccd 100644 --- a/src/bls/bls_worker.h +++ b/src/bls/bls_worker.h @@ -12,8 +12,6 @@ #include #include -#include - // Low level BLS/DKG stuff. All very compute intensive and optimized for parallelization // The worker tries to parallelize as much as possible and utilizes a few properties of BLS aggregation to speed up things // For example, public key vectors can be aggregated in parallel if they are split into batches and the batched aggregations are From 512b03a2fcab2cdca6a12a96b7dcafff3d2a99ab Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 14 Apr 2021 01:12:26 -0400 Subject: [PATCH 05/10] use static / const where possible --- src/bls/bls_worker.cpp | 2 +- src/bls/bls_worker.h | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/bls/bls_worker.cpp b/src/bls/bls_worker.cpp index 4d9f4be73654..68dfeb110568 100644 --- a/src/bls/bls_worker.cpp +++ b/src/bls/bls_worker.cpp @@ -575,7 +575,7 @@ struct ContributionVerifier { } } - bool Verify(const BLSVerificationVectorPtr& vvec, const CBLSSecretKey& skShare) + bool Verify(const BLSVerificationVectorPtr& vvec, const CBLSSecretKey& skShare) const { CBLSPublicKey pk1; if (!pk1.PublicKeyShare(*vvec, forId)) { diff --git a/src/bls/bls_worker.h b/src/bls/bls_worker.h index f1002165dccd..c6d924623778 100644 --- a/src/bls/bls_worker.h +++ b/src/bls/bls_worker.h @@ -104,7 +104,7 @@ class CBLSWorker // Calculate public key share from public key vector and id. Not parallelized - CBLSPublicKey BuildPubKeyShare(const BLSVerificationVectorPtr& vvec, const CBLSId& id); + static CBLSPublicKey BuildPubKeyShare(const BLSVerificationVectorPtr& vvec, const CBLSId& id); // The following functions verify multiple verification vectors and contributions for the same id // This is parallelized by performing batched verification. The verification vectors and the contributions of @@ -122,13 +122,13 @@ class CBLSWorker std::future AsyncVerifyContributionShare(const CBLSId& forId, const BLSVerificationVectorPtr& vvec, const CBLSSecretKey& skContribution); // Non parallelized verification of a single contribution - bool VerifyContributionShare(const CBLSId& forId, const BLSVerificationVectorPtr& vvec, const CBLSSecretKey& skContribution); + static bool VerifyContributionShare(const CBLSId& forId, const BLSVerificationVectorPtr& vvec, const CBLSSecretKey& skContribution); // Simple verification of vectors. Checks x.IsValid() for every entry and checks for duplicate entries - bool VerifyVerificationVector(const BLSVerificationVector& vvec, size_t start = 0, size_t count = 0); - bool VerifyVerificationVectors(const std::vector& vvecs, size_t start = 0, size_t count = 0); - bool VerifySecretKeyVector(const BLSSecretKeyVector& secKeys, size_t start = 0, size_t count = 0); - bool VerifySignatureVector(const BLSSignatureVector& sigs, size_t start = 0, size_t count = 0); + static bool VerifyVerificationVector(const BLSVerificationVector& vvec, size_t start = 0, size_t count = 0); + static bool VerifyVerificationVectors(const std::vector& vvecs, size_t start = 0, size_t count = 0); + static bool VerifySecretKeyVector(const BLSSecretKeyVector& secKeys, size_t start = 0, size_t count = 0); + static bool VerifySignatureVector(const BLSSignatureVector& sigs, size_t start = 0, size_t count = 0); // Internally batched signature signing and verification void AsyncSign(const CBLSSecretKey& secKey, const uint256& msgHash, SignDoneCallback doneCallback); From 4b23772956fc3a02faa99874454a7e45d4d7c26d Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 14 Apr 2021 01:13:48 -0400 Subject: [PATCH 06/10] make parameter const ref where possible --- src/bls/bls_worker.cpp | 4 ++-- src/bls/bls_worker.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/bls/bls_worker.cpp b/src/bls/bls_worker.cpp index 68dfeb110568..2c265949817d 100644 --- a/src/bls/bls_worker.cpp +++ b/src/bls/bls_worker.cpp @@ -830,7 +830,7 @@ bool CBLSWorker::VerifySignatureVector(const BLSSignatureVector& sigs, size_t st return VerifyVectorHelper(sigs, start, count); } -void CBLSWorker::AsyncSign(const CBLSSecretKey& secKey, const uint256& msgHash, CBLSWorker::SignDoneCallback doneCallback) +void CBLSWorker::AsyncSign(const CBLSSecretKey& secKey, const uint256& msgHash, const CBLSWorker::SignDoneCallback& doneCallback) { workerPool.push([secKey, msgHash, doneCallback](int threadId) { doneCallback(secKey.Sign(msgHash)); @@ -890,7 +890,7 @@ bool CBLSWorker::IsAsyncVerifyInProgress() // sigVerifyMutex must be held while calling void CBLSWorker::PushSigVerifyBatch() { - auto f = [this](int threadId, std::shared_ptr > _jobs) { + auto f = [this](int threadId, const std::shared_ptr >& _jobs) { auto& jobs = *_jobs; if (jobs.size() == 1) { auto& job = jobs[0]; diff --git a/src/bls/bls_worker.h b/src/bls/bls_worker.h index c6d924623778..a54bc09a9386 100644 --- a/src/bls/bls_worker.h +++ b/src/bls/bls_worker.h @@ -131,7 +131,7 @@ class CBLSWorker static bool VerifySignatureVector(const BLSSignatureVector& sigs, size_t start = 0, size_t count = 0); // Internally batched signature signing and verification - void AsyncSign(const CBLSSecretKey& secKey, const uint256& msgHash, SignDoneCallback doneCallback); + void AsyncSign(const CBLSSecretKey& secKey, const uint256& msgHash, const SignDoneCallback& doneCallback); std::future AsyncSign(const CBLSSecretKey& secKey, const uint256& msgHash); void AsyncVerifySig(const CBLSSignature& sig, const CBLSPublicKey& pubKey, const uint256& msgHash, SigVerifyDoneCallback doneCallback, CancelCond cancelCond = [] { return false; }); std::future AsyncVerifySig(const CBLSSignature& sig, const CBLSPublicKey& pubKey, const uint256& msgHash, CancelCond cancelCond = [] { return false; }); From 0e4e2be7d73502842f66b10da860250d36fc1a97 Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 14 Apr 2021 01:20:32 -0400 Subject: [PATCH 07/10] Clang-Tidy: Parameter is passed by value and only copied once; consider moving it to avoid unnecessary copies --- src/bls/bls_worker.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/bls/bls_worker.cpp b/src/bls/bls_worker.cpp index 2c265949817d..5f6b3dc49337 100644 --- a/src/bls/bls_worker.cpp +++ b/src/bls/bls_worker.cpp @@ -656,7 +656,7 @@ void CBLSWorker::AsyncAggregateSecretKeys(const BLSSecretKeyVector& secKeys, size_t start, size_t count, bool parallel, std::function doneCallback) { - AsyncAggregateHelper(workerPool, secKeys, start, count, parallel, doneCallback); + AsyncAggregateHelper(workerPool, secKeys, start, count, parallel, std::move(doneCallback)); } std::future CBLSWorker::AsyncAggregateSecretKeys(const BLSSecretKeyVector& secKeys, @@ -677,7 +677,7 @@ void CBLSWorker::AsyncAggregatePublicKeys(const BLSPublicKeyVector& pubKeys, size_t start, size_t count, bool parallel, std::function doneCallback) { - AsyncAggregateHelper(workerPool, pubKeys, start, count, parallel, doneCallback); + AsyncAggregateHelper(workerPool, pubKeys, start, count, parallel, std::move(doneCallback)); } std::future CBLSWorker::AsyncAggregatePublicKeys(const BLSPublicKeyVector& pubKeys, @@ -698,7 +698,7 @@ void CBLSWorker::AsyncAggregateSigs(const BLSSignatureVector& sigs, size_t start, size_t count, bool parallel, std::function doneCallback) { - AsyncAggregateHelper(workerPool, sigs, start, count, parallel, doneCallback); + AsyncAggregateHelper(workerPool, sigs, start, count, parallel, std::move(doneCallback)); } std::future CBLSWorker::AsyncAggregateSigs(const BLSSignatureVector& sigs, @@ -877,7 +877,7 @@ void CBLSWorker::AsyncVerifySig(const CBLSSignature& sig, const CBLSPublicKey& p std::future CBLSWorker::AsyncVerifySig(const CBLSSignature& sig, const CBLSPublicKey& pubKey, const uint256& msgHash, CancelCond cancelCond) { auto p = BuildFutureDoneCallback2(); - AsyncVerifySig(sig, pubKey, msgHash, std::move(p.first), cancelCond); + AsyncVerifySig(sig, pubKey, msgHash, std::move(p.first), std::move(cancelCond)); return std::move(p.second); } From e680bd1d8f1e7c9400656da8465d09e8c27cd64c Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 14 Apr 2021 01:22:06 -0400 Subject: [PATCH 08/10] Clang-Tidy: Passing result of std::move() as a const reference argument; no move will actually happen --- src/bls/bls_worker.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bls/bls_worker.cpp b/src/bls/bls_worker.cpp index 5f6b3dc49337..37cbbaa52163 100644 --- a/src/bls/bls_worker.cpp +++ b/src/bls/bls_worker.cpp @@ -840,7 +840,7 @@ void CBLSWorker::AsyncSign(const CBLSSecretKey& secKey, const uint256& msgHash, std::future CBLSWorker::AsyncSign(const CBLSSecretKey& secKey, const uint256& msgHash) { auto p = BuildFutureDoneCallback(); - AsyncSign(secKey, msgHash, std::move(p.first)); + AsyncSign(secKey, msgHash, p.first); return std::move(p.second); } From 027d4d701b655b686ea515253e8be5aec88f526e Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 14 Apr 2021 01:23:28 -0400 Subject: [PATCH 09/10] Clang-Tidy: Forwarding reference passed to std::move(), which may unexpectedly cause lvalues to be moved; use std::forward() instead --- src/bls/bls_worker.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bls/bls_worker.cpp b/src/bls/bls_worker.cpp index 37cbbaa52163..29440e832f1d 100644 --- a/src/bls/bls_worker.cpp +++ b/src/bls/bls_worker.cpp @@ -590,7 +590,7 @@ struct ContributionVerifier { void PushOrDoWork(Callable&& f) { if (parallel) { - workerPool.push(std::move(f)); + workerPool.push(std::forward(f)); } else { f(0); } From 820e0a399595bd35a2b44df1b0b9953afe1c0cf7 Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 14 Apr 2021 01:24:57 -0400 Subject: [PATCH 10/10] access statically --- src/bls/bls_worker.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bls/bls_worker.h b/src/bls/bls_worker.h index a54bc09a9386..6b46ae93ceeb 100644 --- a/src/bls/bls_worker.h +++ b/src/bls/bls_worker.h @@ -174,7 +174,7 @@ class CBLSWorkerCache CBLSPublicKey BuildPubKeyShare(const uint256& cacheKey, const BLSVerificationVectorPtr& vvec, const CBLSId& id) { return GetOrBuild(cacheKey, publicKeyShareCache, [&]() { - return worker.BuildPubKeyShare(vvec, id); + return CBLSWorker::BuildPubKeyShare(vvec, id); }); }