backport: v23.1.10 fixes for v24.0.x

src/active/dkgsession.cpp

@@ -17,6 +17,7 @@
 
 #include <chain.h>
 #include <deploymentstatus.h>
+#include <hash.h>
 #include <validation.h>
 
 #include <cxxtimer.hpp>
@@ -49,7 +50,23 @@ std::optional<CDKGContribution> ActiveDKGSession::Contribute()
 
     cxxtimer::Timer t1(true);
     logger.Batch("generating contributions");
-    if (!blsWorker.GenerateContributions(params.threshold, memberIds, vvecContribution, m_sk_contributions)) {
+    if (ShouldSimulateError(DKGError::type::CONTRIBUTION_CLONE)) {
+        // Derive the contribution from the quorum alone, so every member simulating this deals the same
+        // contribution, as if it had copied another member's
+        logger.Batch("dealing a cloned contribution");
+        std::vector<CBLSSecretKey> svec(params.threshold);
+        vvecContribution = std::make_shared<std::vector<CBLSPublicKey>>();
+        for (const auto i : util::irange(svec.size())) {
+            uint256 seed{::SerializeHash(std::make_pair(m_quorum_base_block_index->GetBlockHash(), uint32_t(i)))};
+            *seed.begin() = 0; // keep it below the group order
+            svec[i] = CBLSSecretKey{Span{seed}};
+            vvecContribution->emplace_back(svec[i].GetPublicKey());
+        }
+        m_sk_contributions.resize(memberIds.size());
+        for (const auto i : util::irange(memberIds.size())) {
+            m_sk_contributions[i].SecretKeyShare(svec, memberIds[i]);
+        }
+    } else if (!blsWorker.GenerateContributions(params.threshold, memberIds, vvecContribution, m_sk_contributions)) {
         // this should never happen actually
         logger.Batch("GenerateContributions failed");
         return std::nullopt;
@@ -121,9 +138,9 @@ std::optional<CDKGContribution> ActiveDKGSession::SendContributions()
 }
 
 // Verifies all pending secret key contributions in one batch
-// This is done by aggregating the verification vectors belonging to the secret key contributions
+// This is done by aggregating randomly weighted verification vectors belonging to the secret key contributions
 // The resulting aggregated vvec is then used to recover a public key share
-// The public key share must match the public key belonging to the aggregated secret key contributions
+// The public key share must match the equally weighted public keys of the secret key contributions
 // See CBLSWorker::VerifyContributionShares for more details.
 void ActiveDKGSession::VerifyPendingContributions()
 {
@@ -140,6 +157,7 @@ void ActiveDKGSession::VerifyPendingContributions()
     std::vector<size_t> memberIndexes;
     std::vector<BLSVerificationVectorPtr> vvecs;
     std::vector<CBLSSecretKey> skContributions;
+    bool anyLegacyVvec{false};
 
     for (const auto& idx : pendingContributionVerifications) {
         const auto& m = members[idx];
@@ -148,6 +166,7 @@ void ActiveDKGSession::VerifyPendingContributions()
         }
         memberIndexes.emplace_back(idx);
         vvecs.emplace_back(receivedVvecs[idx]);
+        anyLegacyVvec |= receivedVvecsLegacyScheme[idx];
         skContributions.emplace_back(receivedSkContributions[idx]);
         // Write here to definitely store one contribution for each member no matter if
         // our share is valid or not, could be that others are still correct
@@ -165,7 +184,9 @@ void ActiveDKGSession::VerifyPendingContributions()
         return;
     }
 
-    auto result = blsWorker.VerifyContributionShares(myId, vvecs, skContributions);
+    // Weighted aggregation needs points that passed subgroup checks, which legacy-scheme deserialization skips
+    auto result = blsWorker.VerifyContributionShares(myId, vvecs, skContributions, /*parallel=*/true,
+                                                     /*aggregated=*/!anyLegacyVvec);
     if (result.size() != memberIndexes.size()) {
         logger.Batch("VerifyContributionShares returned result of size %d but size %d was expected, something is wrong", result.size(), memberIndexes.size());
         return;
@@ -453,7 +474,25 @@ std::optional<CDKGPrematureCommitment> ActiveDKGSession::VerifyAndCommit()
         }
     }
 
-    if (!badMembers.empty() || !openComplaintMembers.empty()) {
+    // A member can copy another member's whole contribution: every share verifies, but the duplicated
+    // coefficients make the quorum verification vector impossible to build, so no commitment could be made.
+    // The copy can't be told from the original, so drop both; members that agree on the remaining members
+    // and hold the same contributions drop the same ones and still agree on the commitment.
+    std::vector<size_t> remainingMembers;
+    std::vector<BLSVerificationVectorPtr> remainingVvecs;
+    for (const auto& m : members) {
+        if (!m->bad) {
+            remainingMembers.emplace_back(m->idx);
+            remainingVvecs.emplace_back(receivedVvecs[m->idx]);
+        }
+    }
+    std::vector<size_t> sharedCoefficientMembers;
+    for (const auto i : CBLSWorker::FindVerificationVectorsWithSharedCoefficients(remainingVvecs)) {
+        MarkBadMember(remainingMembers[i]);
+        sharedCoefficientMembers.emplace_back(remainingMembers[i]);
+    }
+
+    if (!badMembers.empty() || !openComplaintMembers.empty() || !sharedCoefficientMembers.empty()) {
         logger.Batch("verification result:");
     }
     if (!badMembers.empty()) {
@@ -468,6 +507,12 @@ std::optional<CDKGPrematureCommitment> ActiveDKGSession::VerifyAndCommit()
             logger.Batch("    %s", members[idx]->dmn->proTxHash.ToString());
         }
     }
+    if (!sharedCoefficientMembers.empty()) {
+        logger.Batch("  members sharing verification vector coefficients and now marked as bad:");
+        for (const auto& idx : sharedCoefficientMembers) {
+            logger.Batch("    %s", members[idx]->dmn->proTxHash.ToString());
+        }
+    }
 
     logger.Flush();
 

src/bench/bls_dkg.cpp

@@ -47,7 +47,8 @@ class DKG
 
     void VerifyContributionShares(size_t whoAmI, const std::set<size_t>& invalidIndexes, bool aggregated)
     {
-        auto result = blsWorker.VerifyContributionShares(members[whoAmI].id, receivedVvecs, receivedSkShares, aggregated);
+        auto result = blsWorker.VerifyContributionShares(members[whoAmI].id, receivedVvecs, receivedSkShares,
+                                                         /*parallel=*/true, aggregated);
         for (const size_t i : util::irange(receivedVvecs.size())) {
             if (invalidIndexes.count(i)) {
                 assert(!result[i]);

src/bls/bls.cpp

@@ -205,6 +205,13 @@ bool CBLSPublicKey::PublicKeyShare(Span<CBLSPublicKey> mpk, const CBLSId& _id)
     return true;
 }
 
+void CBLSPublicKey::MulInsecure(const CBLSSecretKey& factor)
+{
+    assert(IsValid() && factor.IsValid());
+    impl = factor.impl * impl;
+    cachedHash.SetNull();
+}
+
 bool CBLSPublicKey::DHKeyExchange(const CBLSSecretKey& sk, const CBLSPublicKey& pk)
 {
     fValid = false;
@@ -293,6 +300,13 @@ void CBLSSignature::SubInsecure(const CBLSSignature& o)
     cachedHash.SetNull();
 }
 
+void CBLSSignature::MulInsecure(const CBLSSecretKey& factor)
+{
+    assert(IsValid() && factor.IsValid());
+    impl = factor.impl * impl;
+    cachedHash.SetNull();
+}
+
 bool CBLSSignature::VerifyInsecure(const CBLSPublicKey& pubKey, const uint256& hash, const bool specificLegacyScheme) const
 {
     if (!IsValid() || !pubKey.IsValid()) {

src/bls/bls.h

@@ -314,6 +314,8 @@ class CBLSPublicKey : public CBLSWrapper<bls::G1Element, BLS_CURVE_PUBKEY_SIZE,
     void AggregateInsecure(const CBLSPublicKey& o);
     static CBLSPublicKey AggregateInsecure(Span<CBLSPublicKey> pks);
 
+    void MulInsecure(const CBLSSecretKey& factor);
+
     bool PublicKeyShare(Span<CBLSPublicKey> mpk, const CBLSId& id);
     bool DHKeyExchange(const CBLSSecretKey& sk, const CBLSPublicKey& pk);
 
@@ -360,6 +362,7 @@ class CBLSSignature : public CBLSWrapper<bls::G2Element, BLS_CURVE_SIG_SIZE, CBL
     static CBLSSignature AggregateSecure(Span<CBLSSignature> sigs, Span<CBLSPublicKey> pks, const uint256& hash);
 
     void SubInsecure(const CBLSSignature& o);
+    void MulInsecure(const CBLSSecretKey& factor);
     [[nodiscard]] bool VerifyInsecure(const CBLSPublicKey& pubKey, const uint256& hash, const bool specificLegacyScheme) const;
     [[nodiscard]] bool VerifyInsecure(const CBLSPublicKey& pubKey, const uint256& hash) const;
     [[nodiscard]] bool VerifyInsecureAggregated(Span<CBLSPublicKey> pubKeys, Span<uint256> hashes) const;

src/bls/bls_batchverifier.h

@@ -6,10 +6,26 @@
 #define DASH_CRYPTO_BLS_BATCHVERIFIER_H
 
 #include <bls/bls.h>
+#include <random.h>
 
+#include <algorithm>
+#include <array>
 #include <map>
 #include <vector>
 
+//! A secret random nonzero scalar of numBytes bytes (at most 16, so well below the group order). Weighting each
+//! signature and its public key by one makes errors in several signatures unable to cancel out in an aggregate.
+inline CBLSSecretKey MakeBatchVerificationWeight(FastRandomContext& rng, size_t numBytes = 16)
+{
+    assert(numBytes > 0 && numBytes <= 16);
+    std::array<uint8_t, BLS_CURVE_SECKEY_SIZE> bytes{};
+    do {
+        const uint256 rand{rng.rand256()};
+        std::copy(rand.begin(), rand.begin() + numBytes, bytes.end() - numBytes);
+    } while (std::all_of(bytes.begin(), bytes.end(), [](uint8_t b) { return b == 0; }));
+    return CBLSSecretKey{bytes};
+}
+
 template<typename SourceId, typename MessageId>
 class CBLSBatchVerifier
 {
@@ -19,6 +35,9 @@ class CBLSBatchVerifier
         uint256 msgHash;
         CBLSSignature sig;
         CBLSPublicKey pubKey;
+        //! sig and pubKey multiplied by the same MakeBatchVerificationWeight()
+        CBLSSignature weightedSig;
+        CBLSPublicKey weightedPubKey;
     };
 
     using MessageMap = std::map<MessageId, Message>;
@@ -32,6 +51,8 @@ class CBLSBatchVerifier
     MessageMap messages;
     MessagesBySourceMap messagesBySource;
 
+    FastRandomContext rng;
+
 public:
     std::set<SourceId> badSources;
     std::set<MessageId> badMessages;
@@ -48,7 +69,12 @@ class CBLSBatchVerifier
     {
         assert(sig.IsValid() && pubKey.IsValid());
 
-        auto it = messages.emplace(msgId, Message{msgId, msgHash, sig, pubKey}).first;
+        auto [it, inserted] = messages.emplace(msgId, Message{msgId, msgHash, sig, pubKey, sig, pubKey});
+        if (inserted) {
+            const CBLSSecretKey weight{MakeBatchVerificationWeight(rng)};
+            it->second.weightedSig.MulInsecure(weight);
+            it->second.weightedPubKey.MulInsecure(weight);
+        }
         messagesBySource[sourceId].emplace_back(it);
 
         if (subBatchSize != 0 && messages.size() >= subBatchSize) {
@@ -155,8 +181,8 @@ class CBLSBatchVerifier
                     continue;
                 }
 
-                sigsToAggregate.push_back(msg.sig);
-                pubKeysToAggregate.push_back(msg.pubKey);
+                sigsToAggregate.push_back(msg.weightedSig);
+                pubKeysToAggregate.push_back(msg.weightedPubKey);
             }
 
             CBLSPublicKey aggPubKey = CBLSPublicKey::AggregateInsecure(pubKeysToAggregate);
@@ -210,8 +236,8 @@ class CBLSBatchVerifier
 
             if (dups.emplace(msg.msgId).second) {
                 msgHashes.emplace_back(msgHash);
-                pubKeys.emplace_back(msg.pubKey);
-                sigsToAggregate.push_back(msg.sig);
+                pubKeys.emplace_back(msg.weightedPubKey);
+                sigsToAggregate.push_back(msg.weightedSig);
             }
 
             messageIts.pop_back();