diff --git a/doc/release-notes.md b/doc/release-notes.md index 791240fe14ae..24c20e2e39d4 100644 --- a/doc/release-notes.md +++ b/doc/release-notes.md @@ -593,6 +593,12 @@ connections to it advertise: - transaction relay (`relay=1` in the `version` message); - compact block filters (`NODE_COMPACT_FILTERS`). (#7755) +A DKG member whose verification vector shares a coefficient with another +member's, as happens when one member copies another's contribution, is now +excluded from the commitment together with that member instead of the round +failing. Both receive the usual PoSe penalty for failed DKG participation. +(#8016) + ## Bitcoin Core backports This release continues the backporting of Bitcoin Core, adding well over 200 @@ -629,11 +635,9 @@ and RPC parameters. cache format version was bumped for this: on the first start after upgrading, the existing cache is discarded and governance objects and votes are synced again from the network. (#7831) -- This release includes the fixes shipped in Dash Core v23.1.9, including the - consensus rule that rejects transactions with a negative `nVersion` once DIP3 - is active, and the change that stopped masternodes from InstantSend-locking - provider transactions (#7858; software waiting for an islock on a provider - transaction should wait for a ChainLock or confirmations instead). +- Provider (masternode) transactions are no longer InstantSend-locked. + Software that waits for an InstantSend lock on a provider transaction should + wait for a ChainLock or for confirmations instead. (#7858) - Network-adjusted time is no longer used to validate block timestamps or to build block templates; both use the system clock. Keep the system clock accurate. `-maxtimeadjustment` now only affects Dash subsystems that still use @@ -720,6 +724,12 @@ and RPC parameters. policy (previously 83 bytes). This better reflects the actual afforded protections against CVE-2017-12842 and opens up additional use-cases of smaller transaction sizes. (#7666) +- Asset Lock transactions with more than 100 inputs or larger than 20480 bytes + are now non-standard (`assetlocktx-too-many-inputs`, `assetlocktx-too-big`), + because Dash Platform cannot process them and the locked funds could be + lost. They are not accepted into the mempool or relayed, but remain valid in + a block. Software that funds Platform should keep its Asset Locks within these + limits. (#7359) - Block template creation now checks credit pool limits across complete transaction packages. Because Asset Unlock limits are cumulative, a package may exceed the block's limit even though its transactions were accepted @@ -949,6 +959,9 @@ and RPC parameters. - Additional flags "in" and "out" have been added to `-whitelist` to control whether permissions apply to incoming connections and/or manual (default: incoming only). (#8005) +- `-allowprivatenet` no longer affects which masternode addresses are accepted. + On mainnet, testnet and devnets a masternode address in a private (RFC 1918) + range is rejected whether or not the option is set. (#7888) ### Removed options diff --git a/src/active/dkgsession.cpp b/src/active/dkgsession.cpp index 7ab5d2756b90..02f110fad2a4 100644 --- a/src/active/dkgsession.cpp +++ b/src/active/dkgsession.cpp @@ -17,6 +17,7 @@ #include #include +#include #include #include @@ -49,7 +50,23 @@ std::optional 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 svec(params.threshold); + vvecContribution = std::make_shared>(); + 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 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 memberIndexes; std::vector vvecs; std::vector 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 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 remainingMembers; + std::vector remainingVvecs; + for (const auto& m : members) { + if (!m->bad) { + remainingMembers.emplace_back(m->idx); + remainingVvecs.emplace_back(receivedVvecs[m->idx]); + } + } + std::vector 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 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(); diff --git a/src/bench/bls_dkg.cpp b/src/bench/bls_dkg.cpp index 7b60a5fe4f03..bdf9015be8b7 100644 --- a/src/bench/bls_dkg.cpp +++ b/src/bench/bls_dkg.cpp @@ -47,7 +47,8 @@ class DKG void VerifyContributionShares(size_t whoAmI, const std::set& 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]); diff --git a/src/bls/bls.cpp b/src/bls/bls.cpp index 8d5b1b83703c..96a188e2ae93 100644 --- a/src/bls/bls.cpp +++ b/src/bls/bls.cpp @@ -205,6 +205,13 @@ bool CBLSPublicKey::PublicKeyShare(Span 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()) { diff --git a/src/bls/bls.h b/src/bls/bls.h index ae2124cd38e1..82abc2c6de04 100644 --- a/src/bls/bls.h +++ b/src/bls/bls.h @@ -314,6 +314,8 @@ class CBLSPublicKey : public CBLSWrapper pks); + void MulInsecure(const CBLSSecretKey& factor); + bool PublicKeyShare(Span mpk, const CBLSId& id); bool DHKeyExchange(const CBLSSecretKey& sk, const CBLSPublicKey& pk); @@ -360,6 +362,7 @@ class CBLSSignature : public CBLSWrapper sigs, Span 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 pubKeys, Span hashes) const; diff --git a/src/bls/bls_batchverifier.h b/src/bls/bls_batchverifier.h index 528b527f0090..647d38e675e3 100644 --- a/src/bls/bls_batchverifier.h +++ b/src/bls/bls_batchverifier.h @@ -6,10 +6,26 @@ #define DASH_CRYPTO_BLS_BATCHVERIFIER_H #include +#include +#include +#include #include #include +//! 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 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 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; @@ -32,6 +51,8 @@ class CBLSBatchVerifier MessageMap messages; MessagesBySourceMap messagesBySource; + FastRandomContext rng; + public: std::set badSources; std::set 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(); diff --git a/src/bls/bls_worker.cpp b/src/bls/bls_worker.cpp index aeae68b1ed3b..2ebc5eade3a0 100644 --- a/src/bls/bls_worker.cpp +++ b/src/bls/bls_worker.cpp @@ -3,11 +3,15 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include +#include #include +#include #include #include +#include +#include #include #include #include @@ -392,13 +396,6 @@ struct ContributionVerifier : public std::enable_shared_from_this > aggDone; - // we can't directly update a vector in parallel // as vector is not thread safe (uses bitsets internally) // so we must use vector temporarily and concatenate/convert @@ -451,7 +448,6 @@ struct ContributionVerifier : public std::enable_shared_from_this>(0); batchState.start = i * batchSize; batchState.count = std::min(batchSize, vvecs.size() - batchState.start); batchState.verifyResults.assign(batchState.count, 0); @@ -483,32 +479,64 @@ struct ContributionVerifier : public std::enable_shared_from_thisshared_from_this()); - auto vvecAgg = std::make_shared>(vvecs.subspan(batchState.start, batchState.count), parallel, workerPool, [this, self, batchIdx] (const BLSVerificationVectorPtr& vvec) {HandleAggVvecDone(batchIdx, vvec);}); - auto skShareAgg = std::make_shared>(Span{skShares}.subspan(batchState.start, batchState.count), parallel, workerPool, [this, self, batchIdx] (const CBLSSecretKey& skShare) {HandleAggSkShareDone(batchIdx, skShare);}); - - vvecAgg->Start(); - skShareAgg->Start(); + auto f = [this, self, batchIdx](int threadId) { + auto& batchState = batchStates[batchIdx]; + if (VerifyWeightedBatch(batchState)) { + // whole batch is valid + batchState.verifyResults.assign(batchState.count, 1); + HandleVerifyDone(batchState.count); + } else { + // at least one entry in the batch is invalid, revert to per-contribution verification (but parallelized) + AsyncVerifyBatchOneByOne(batchIdx); + } + }; + PushOrDoWork(std::move(f)); } - void HandleAggVvecDone(size_t batchIdx, const BLSVerificationVectorPtr& vvec) + // Each contribution's vvec and the public key of its share are multiplied by the same secret random weight before + // they are aggregated, so that errors in the contributions of several dealers can't cancel out. 64-bit weights + // let a batch with an invalid contribution pass with a probability of at most 2^-64. + bool VerifyWeightedBatch(const BatchState& batchState) const { - auto& batchState = batchStates[batchIdx]; - batchState.vvec = vvec; - if (++(*batchState.aggDone) == 2) { - HandleAggDone(batchIdx); + FastRandomContext rng; + std::vector aggVvec; + std::vector pubKeys; + pubKeys.reserve(batchState.count); + for (size_t i = batchState.start; i < batchState.start + batchState.count; i++) { + const auto& vvec = vvecs[i]; + if (vvec == nullptr || vvec->empty() || !skShares[i].IsValid() || + (!aggVvec.empty() && vvec->size() != aggVvec.size())) { + return false; + } + const CBLSSecretKey weight{MakeBatchVerificationWeight(rng, /*numBytes=*/8)}; + CBLSPublicKey pubKey{skShares[i].GetPublicKey()}; + pubKey.MulInsecure(weight); + pubKeys.emplace_back(pubKey); + const bool first{aggVvec.empty()}; + for (size_t j = 0; j < vvec->size(); j++) { + CBLSPublicKey coefficient{(*vvec)[j]}; + if (!coefficient.IsValid()) { + return false; + } + coefficient.MulInsecure(weight); + if (first) { + aggVvec.emplace_back(coefficient); + } else { + aggVvec[j].AggregateInsecure(coefficient); + } + if (!aggVvec[j].IsValid()) { + // AggregateInsecure() asserts on an invalid operand, so leave this to the one-by-one fallback + return false; + } + } } - } - void HandleAggSkShareDone(size_t batchIdx, const CBLSSecretKey& skShare) - { - auto& batchState = batchStates[batchIdx]; - batchState.skShare = skShare; - if (++(*batchState.aggDone) == 2) { - HandleAggDone(batchIdx); + + CBLSPublicKey expected; + if (!expected.PublicKeyShare(aggVvec, forId)) { + return false; } + return expected == CBLSPublicKey::AggregateInsecure(pubKeys); } void HandleVerifyDone(size_t count) @@ -519,39 +547,6 @@ struct ContributionVerifier : public std::enable_shared_from_thisempty() || !batchState.skShare.IsValid()) { - // something went wrong while aggregating and there is nothing we can do now except mark the whole batch as failed - // this can only happen if inputs were invalid in some way - batchState.verifyResults.assign(batchState.count, 0); - HandleVerifyDone(batchState.count); - return; - } - - AsyncAggregatedVerifyBatch(batchIdx); - } - - void AsyncAggregatedVerifyBatch(size_t batchIdx) - { - auto self(this->shared_from_this()); - auto f = [this, self, batchIdx](int threadId) { - auto& batchState = batchStates[batchIdx]; - bool result = Verify(batchState.vvec, batchState.skShare); - if (result) { - // whole batch is valid - batchState.verifyResults.assign(batchState.count, 1); - HandleVerifyDone(batchState.count); - } else { - // at least one entry in the batch is invalid, revert to per-contribution verification (but parallelized) - AsyncVerifyBatchOneByOne(batchIdx); - } - }; - PushOrDoWork(std::move(f)); - } - void AsyncVerifyBatchOneByOne(size_t batchIdx) { size_t count = batchStates[batchIdx].count; @@ -619,13 +614,15 @@ BLSVerificationVectorPtr CBLSWorker::BuildQuorumVerificationVector(Span void AsyncAggregateHelper(ctpl::thread_pool& workerPool, Span vec, bool parallel, - std::function doneCallback) + std::function doneCallback, bool rejectDuplicates = true) { if (vec.empty()) { doneCallback(T()); return; } - if (!VerifyVectorHelper(vec)) { + const bool inputValid{rejectDuplicates ? VerifyVectorHelper(vec) + : std::all_of(vec.begin(), vec.end(), [](const T& item) { return item.IsValid(); })}; + if (!inputValid) { doneCallback(T()); return; } @@ -637,7 +634,10 @@ void AsyncAggregateHelper(ctpl::thread_pool& workerPool, Span vec, bool paral void CBLSWorker::AsyncAggregateSecretKeys(Span secKeys, bool parallel, std::function doneCallback) { - AsyncAggregateHelper(workerPool, secKeys, parallel, std::move(doneCallback)); + // These are the DKG shares dealt to one member. Two colluding dealers can deal it equal shares while their + // contributions stay valid and their vvecs share no coefficient, so rejecting duplicates here would let them keep + // chosen members from building their quorum secret key share. The sum is correct either way. + AsyncAggregateHelper(workerPool, secKeys, parallel, std::move(doneCallback), /*rejectDuplicates=*/false); } std::future CBLSWorker::AsyncAggregateSecretKeys(Span secKeys, bool parallel) @@ -804,6 +804,25 @@ bool CBLSWorker::VerifyVerificationVectors(Span vvecs) return true; } +std::set CBLSWorker::FindVerificationVectorsWithSharedCoefficients(Span vvecs) +{ + std::map owners; + std::set ret; + for (size_t i = 0; i < vvecs.size(); i++) { + if (vvecs[i] == nullptr) { + continue; + } + for (const auto& coefficient : *vvecs[i]) { + const auto [it, inserted] = owners.emplace(coefficient.GetHash(), i); + if (!inserted && it->second != i) { + ret.emplace(it->second); + ret.emplace(i); + } + } + } + return ret; +} + void CBLSWorker::AsyncSign(const CBLSSecretKey& secKey, const uint256& msgHash, const CBLSWorker::SignDoneCallback& doneCallback) { workerPool.push([secKey, msgHash, doneCallback](int threadId) { diff --git a/src/bls/bls_worker.h b/src/bls/bls_worker.h index 369058a9591a..74c7ffe67d9b 100644 --- a/src/bls/bls_worker.h +++ b/src/bls/bls_worker.h @@ -11,6 +11,7 @@ #include #include +#include #include // Low level BLS/DKG stuff. All very compute intensive and optimized for parallelization @@ -104,11 +105,13 @@ class CBLSWorker static constexpr size_t CONTRIBUTION_VERIFY_BATCH_SIZE{8}; // 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 - // a batch are aggregated (in parallel, see AsyncBuildQuorumVerificationVector and AsyncBuildSecretKeyShare). The - // result per batch is a single aggregated verification vector and a single aggregated contribution, which are then - // verified with VerifyContributionShare. If verification of the aggregated inputs is successful, the whole batch - // is marked as valid. If the batch verification fails, the individual entries are verified in a non-aggregated manner + // This is parallelized by performing batched verification. Each verification vector of a batch and the public key of + // its contribution are multiplied by the same secret random 64-bit weight, so that errors in the contributions of + // several dealers can't cancel out, and aggregated. The public key share recovered from the aggregated verification + // vector must match the aggregated public key. If it does, the whole batch is marked as valid. If the batch + // verification fails, the individual entries are verified in a non-aggregated manner + // The weighting is only sound for points that passed subgroup checks, so callers that may hold objects deserialized + // under the legacy scheme must pass aggregated=false void AsyncVerifyContributionShares(const CBLSId& forId, Span vvecs, Span skShares, bool parallel, bool aggregated, std::function&)> doneCallback); std::future > AsyncVerifyContributionShares(const CBLSId& forId, Span vvecs, Span skShares, @@ -121,6 +124,9 @@ class CBLSWorker // Simple verification of vectors. Checks x.IsValid() for every entry and checks for duplicate entries static bool VerifyVerificationVector(Span vvec); static bool VerifyVerificationVectors(Span vvecs); + //! Indexes of the vvecs sharing a coefficient with another vvec, which VerifyVerificationVectors() rejects. + //! Null vvecs are skipped. + static std::set FindVerificationVectorsWithSharedCoefficients(Span vvecs); // Internally batched signature signing and verification void AsyncSign(const CBLSSecretKey& secKey, const uint256& msgHash, const SignDoneCallback& doneCallback); diff --git a/src/governance/signing.cpp b/src/governance/signing.cpp index 1eea9b60dac6..ed8c3d0eeb98 100644 --- a/src/governance/signing.cpp +++ b/src/governance/signing.cpp @@ -178,10 +178,12 @@ std::optional GovernanceSigner::CreateGovernanceTrigger return std::make_optional(gov_sb); } -void GovernanceSigner::VoteGovernanceTriggers(const std::optional& trigger_opt) +void GovernanceSigner::VoteGovernanceTriggers(const std::optional& trigger_opt, int nHeight) { // only active masternodes can vote on triggers if (m_mn_activeman.GetProTxHash().IsNull()) return; + // Until we are synced we can't build a candidate, so we have nothing to compare triggers to + if (!m_mn_sync.IsSynced()) return; LOCK(::cs_main); @@ -240,6 +242,12 @@ void GovernanceSigner::VoteGovernanceTriggers(const std::optionalGetBlockHeight() - Params().GetConsensus().nSuperblockMaturityWindow) { + // We can't build a candidate for this trigger's superblock yet, so we have nothing to compare it to + LogPrint(BCLog::GOBJECT, "%s -- Not voting NO-FUNDING for trigger:%s, its maturity window has not started\n", + __func__, trigger_hash.ToString()); + continue; + } if (trigger_hash == votedFundingYesTriggerHash) { // Skip actual trigger LogPrint(BCLog::GOBJECT, "%s -- Not voting NO-FUNDING for trigger:%s, we voted yes for it already\n", @@ -300,7 +308,7 @@ void GovernanceSigner::UpdatedBlockTip(const CBlockIndex* pindex) { const auto sb_opt = CreateSuperblockCandidate(pindex->nHeight); const auto trigger_opt = CreateGovernanceTrigger(sb_opt); - VoteGovernanceTriggers(trigger_opt); + VoteGovernanceTriggers(trigger_opt, pindex->nHeight); CSuperblock_sptr pSuperblock; if (m_superblocks.GetBestSuperblock(m_dmnman.GetListAtChainTip(), pSuperblock, pindex->nHeight)) { ResetVotedFundingTrigger(); diff --git a/src/governance/signing.h b/src/governance/signing.h index b70f73df81fe..c58ef4357303 100644 --- a/src/governance/signing.h +++ b/src/governance/signing.h @@ -54,7 +54,7 @@ class GovernanceSigner std::optional CreateGovernanceTrigger(const std::optional& sb_opt); std::optional CreateSuperblockCandidate(int nHeight) const; void ResetVotedFundingTrigger(); - void VoteGovernanceTriggers(const std::optional& trigger_opt); + void VoteGovernanceTriggers(const std::optional& trigger_opt, int nHeight); }; #endif // BITCOIN_GOVERNANCE_SIGNING_H diff --git a/src/llmq/dkgmessages.h b/src/llmq/dkgmessages.h index 04a5592d827f..7080779abf2f 100644 --- a/src/llmq/dkgmessages.h +++ b/src/llmq/dkgmessages.h @@ -26,6 +26,8 @@ class CDKGContribution BLSVerificationVectorPtr vvec; std::shared_ptr> contributions; CBLSSignature sig; + //! Not serialized: whether vvec was deserialized under the legacy BLS scheme, which skips the subgroup check + bool vvecLegacyScheme{false}; public: template @@ -52,7 +54,11 @@ class CDKGContribution s >> llmqType; s >> quorumHash; s >> proTxHash; - s >> tmp1; + // Decode the whole vvec under one scheme, so that vvecLegacyScheme describes every point in it + vvecLegacyScheme = bls::bls_legacy_scheme.load(); + for (uint64_t i = 0, size = ReadCompactSize(s); i < size; i++) { + s >> CBLSPublicKeyVersionWrapper(tmp1.emplace_back(), vvecLegacyScheme); + } s >> tmp2; s >> sig; diff --git a/src/llmq/dkgsession.cpp b/src/llmq/dkgsession.cpp index 0df95d9fbd49..848afca9f1ee 100644 --- a/src/llmq/dkgsession.cpp +++ b/src/llmq/dkgsession.cpp @@ -94,6 +94,7 @@ bool CDKGSession::Init(const uint256& _myProTxHash, int _quorumIndex) members.resize(mns.size()); memberIds.resize(members.size()); receivedVvecs.resize(members.size()); + receivedVvecsLegacyScheme.resize(members.size()); receivedSkContributions.resize(members.size()); vecEncryptedContributions.resize(members.size()); @@ -221,6 +222,7 @@ std::optional CDKGSession::ReceiveMessage(const CDKGContribution& qc) if (!should_process) return inv; receivedVvecs[member->idx] = qc.vvec; + receivedVvecsLegacyScheme[member->idx] = qc.vvecLegacyScheme; int receivedCount = std::ranges::count_if(members, [](const auto& m) { return !m->contributions.empty(); }); diff --git a/src/llmq/dkgsession.h b/src/llmq/dkgsession.h index 6b29c3427b0e..e116e55bcd16 100644 --- a/src/llmq/dkgsession.h +++ b/src/llmq/dkgsession.h @@ -70,6 +70,7 @@ class DKGError { JUSTIFY_OMIT, JUSTIFY_LIE, CONTRIBUTION_INVALID, + CONTRIBUTION_CLONE, _COUNT }; static constexpr DKGError::type from_string(std::string_view in) { @@ -81,6 +82,7 @@ class DKGError { if (in == "justify-lie") return JUSTIFY_LIE; if (in == "justify-omit") return JUSTIFY_OMIT; if (in == "contribution-invalid") return CONTRIBUTION_INVALID; + if (in == "contribution-clone") return CONTRIBUTION_CLONE; return _COUNT; } }; @@ -144,6 +146,8 @@ class CDKGSession std::vector memberIds; std::vector receivedVvecs; + // whether each received vvec was deserialized under the legacy BLS scheme, see CDKGContribution::vvecLegacyScheme + std::vector receivedVvecsLegacyScheme; // these are not necessarily verified yet. Only trust in what was written to the DB std::vector receivedSkContributions; /// Contains the received unverified/encrypted DKG contributions diff --git a/src/llmq/net_dkg.cpp b/src/llmq/net_dkg.cpp index da1f2481a0b4..279c7dbfd40c 100644 --- a/src/llmq/net_dkg.cpp +++ b/src/llmq/net_dkg.cpp @@ -5,6 +5,7 @@ #include #include +#include #include #include #include @@ -223,6 +224,7 @@ std::unordered_set BatchVerifyMessageSigs(CDKGSession& session, std::unordered_set ret; bool revertToSingleVerification = false; + FastRandomContext rng; CBLSSignature aggSig; std::vector pubKeys; @@ -250,11 +252,22 @@ std::unordered_set BatchVerifyMessageSigs(CDKGSession& session, ret.emplace(nodeId); continue; } + CBLSPublicKey pubKey{member->dmn->pdmnState->pubKeyOperator.Get()}; + if (!pubKey.IsValid()) { + // should not happen as operator keys are validated on registration + ret.emplace(nodeId); + continue; + } + + const CBLSSecretKey weight{MakeBatchVerificationWeight(rng)}; + CBLSSignature weightedSig{msg->sig}; + weightedSig.MulInsecure(weight); + pubKey.MulInsecure(weight); if (first) { - aggSig = msg->sig; + aggSig = weightedSig; } else { - aggSig.AggregateInsecure(msg->sig); + aggSig.AggregateInsecure(weightedSig); } first = false; @@ -270,7 +283,7 @@ std::unordered_set BatchVerifyMessageSigs(CDKGSession& session, break; } - pubKeys.emplace_back(member->dmn->pdmnState->pubKeyOperator.Get()); + pubKeys.emplace_back(pubKey); messageHashes.emplace_back(msgHash); } if (!revertToSingleVerification) { diff --git a/src/test/bls_tests.cpp b/src/test/bls_tests.cpp index 3abcb14611f5..09176f7bb4e2 100644 --- a/src/test/bls_tests.cpp +++ b/src/test/bls_tests.cpp @@ -292,6 +292,30 @@ static void AddMessage(std::vector& vec, uint32_t sourceId, uint32_t ms vec.emplace_back(m); } +// Adds two wrong signatures whose errors cancel out when both are aggregated unweighted +static void AddCancellingMessages(std::vector& vec, uint32_t sourceId1, uint32_t msgId1, uint8_t msgHash1, + uint32_t sourceId2, uint32_t msgId2, uint8_t msgHash2, bool sameKey) +{ + const bool legacy_scheme = bls::bls_legacy_scheme.load(); + AddMessage(vec, sourceId1, msgId1, msgHash1, true); + AddMessage(vec, sourceId2, msgId2, msgHash2, true); + auto& m1 = vec[vec.size() - 2]; + auto& m2 = vec[vec.size() - 1]; + if (sameKey) { + m2.sk = m1.sk; + m2.pk = m1.pk; + m2.sig = m2.sk.Sign(m2.msgHash, legacy_scheme); + } + + CBLSSecretKey sk_error; + sk_error.MakeNewKey(); + const CBLSSignature error{sk_error.Sign(uint256::ONE, legacy_scheme)}; + m1.sig.AggregateInsecure(error); + m2.sig.SubInsecure(error); + m1.valid = false; + m2.valid = false; +} + static void Verify(std::vector& vec, bool secureVerification, bool perMessageFallback) { CBLSBatchVerifier batchVerifier(secureVerification, perMessageFallback); @@ -369,6 +393,18 @@ void FuncBatchVerifier(const bool legacy_scheme) // last message invalid from one source AddMessage(msgs, 1, 7, 1, false); Verify(msgs); + + msgs.clear(); + // errors cancelling out between distinct messages from one source + AddMessage(msgs, 1, 1, 1, true); + AddCancellingMessages(msgs, 2, 2, 2, 2, 3, 3, /*sameKey=*/false); + Verify(msgs); + + // errors cancelling out between distinct sources signing the same message with the same key + msgs.clear(); + AddMessage(msgs, 1, 1, 1, true); + AddCancellingMessages(msgs, 2, 2, 2, 3, 3, 2, /*sameKey=*/true); + Verify(msgs); } void FuncThresholdSignature(const bool legacy_scheme) @@ -636,6 +672,30 @@ void FuncVerifyContributionSharesHonest(const bool legacy_scheme) } } + // 3. With the defaults the DKG uses, only the dealers of wrong shares are blamed: + // - two colluding dealers in the same batch swap their shares: each one is wrong, but their sum is not, so the + // errors cancel in an unweighted aggregate; + // - a missing (invalid) share fails its batch and is blamed alone. + auto swapped = skContributions; + std::swap(swapped[1], swapped[2]); + auto missing = skContributions; + missing[5] = CBLSSecretKey{}; + std::vector, std::set>> cases{ + {swapped, {1, 2}}, + {missing, {5}}, + }; + for (auto& [shares, bad] : cases) { + for (const bool parallel : {true, false}) { + const auto result = worker.VerifyContributionShares(myId, vvecs, shares, parallel); + BOOST_REQUIRE_EQUAL(result.size(), QUORUM_SIZE); + for (const size_t i : util::irange(QUORUM_SIZE)) { + const bool expected{bad.count(i) == 0}; + BOOST_CHECK_MESSAGE(result[i] == expected, "index " << i << " expected " << expected << " got " + << result[i] << " (parallel=" << parallel << ")"); + } + } + } + worker.Stop(); } @@ -693,6 +753,23 @@ void FuncVerifyContributionSharesDuplicateCoefficient(const bool legacy_scheme) BOOST_CHECK(!res[0]); // the member that planted the duplicate is caught alone BOOST_CHECK(res[1]); // the honest member whose coefficient was copied is unaffected BOOST_CHECK(res[2]); // the other honest member is unaffected + BOOST_CHECK(CBLSWorker::FindVerificationVectorsWithSharedCoefficients(vvecs) == (std::set{0, 1})); + + // Dealer 0 copies dealer 1's whole contribution, vvec and share alike: every share verifies, + // but the quorum verification vector can't be built until both copies are dropped. + vvecs[0] = std::make_shared>(*vvecs[1]); + recvShares[0] = skShares[1][R]; + res = worker.VerifyContributionShares(recvId, vvecs, recvShares); + BOOST_REQUIRE_EQUAL(res.size(), 3U); + BOOST_CHECK(res[0] && res[1] && res[2]); + BOOST_CHECK(worker.BuildQuorumVerificationVector(vvecs) == nullptr); + BOOST_CHECK(CBLSWorker::FindVerificationVectorsWithSharedCoefficients(vvecs) == (std::set{0, 1})); + std::vector remaining{vvecs[2]}; + BOOST_CHECK(worker.BuildQuorumVerificationVector(remaining) != nullptr); + + // Distinct vvecs share nothing, and a missing vvec is skipped. + std::vector distinct{vvecs[1], nullptr, vvecs[2]}; + BOOST_CHECK(CBLSWorker::FindVerificationVectorsWithSharedCoefficients(distinct).empty()); worker.Stop(); } @@ -703,6 +780,61 @@ BOOST_AUTO_TEST_CASE(bls_verify_contribution_shares_duplicate_coefficient_tests) FuncVerifyContributionSharesDuplicateCoefficient(false); } +void FuncAggregateCollidingContributionShares(const bool legacy_scheme) +{ + bls::bls_legacy_scheme.store(legacy_scheme); + + CBLSWorker worker; + worker.Start(4); + + // The receiver's id is -1 mod the group order, so a degree-1 polynomial b0 + b1 * x takes the value b0 - b1 there + uint256 recvIdHash; + const auto minusOne{ParseHex("73eda753299d7d483339d80809a1d80553bda402fffe5bfeffffffff00000000")}; + std::copy(minusOne.begin(), minusOne.end(), recvIdHash.begin()); + const CBLSId recvId{recvIdHash}; + + // Dealer 0 is honest. Dealers 1 and 2 collude: their polynomials differ, but agree at the receiver's id + const auto makeKey = [] { CBLSSecretKey sk; sk.MakeNewKey(); return sk; }; + std::vector> polys{{makeKey(), makeKey()}, {makeKey(), makeKey()}, {}}; + CBLSSecretKey share1; + BOOST_REQUIRE(share1.SecretKeyShare(polys[1], recvId)); + const CBLSSecretKey b1{makeKey()}; + CBLSSecretKey b0{share1}; + b0.AggregateInsecure(b1); + polys[2] = {b0, b1}; + + std::vector vvecs; + std::vector recvShares; + for (auto& poly : polys) { + auto vvec = std::make_shared>(); + for (const auto& coefficient : poly) { + vvec->emplace_back(coefficient.GetPublicKey()); + } + vvecs.emplace_back(vvec); + BOOST_REQUIRE(recvShares.emplace_back().SecretKeyShare(poly, recvId)); + } + BOOST_REQUIRE(recvShares[1] == recvShares[2]); + + // Every contribution verifies and nothing in the vvecs collides + const std::vector ok{worker.VerifyContributionShares(recvId, vvecs, recvShares)}; + BOOST_CHECK(ok == std::vector(3, true)); + const BLSVerificationVectorPtr quorumVvec{worker.BuildQuorumVerificationVector(vvecs)}; + BOOST_REQUIRE(quorumVvec != nullptr); + + // so the receiver must still get the quorum secret key share matching the quorum vvec + const CBLSSecretKey skShare{worker.AggregateSecretKeys(recvShares)}; + BOOST_REQUIRE(skShare.IsValid()); + BOOST_CHECK(skShare.GetPublicKey() == CBLSWorker::BuildPubKeyShare(quorumVvec, recvId)); + + worker.Stop(); +} + +BOOST_AUTO_TEST_CASE(bls_aggregate_colliding_contribution_shares_tests) +{ + FuncAggregateCollidingContributionShares(true); + FuncAggregateCollidingContributionShares(false); +} + // A dummy BLS object that satisfies the minimal interface expected by CBLSLazyWrapper. class DummyBLS { diff --git a/src/test/llmq_dkg_tests.cpp b/src/test/llmq_dkg_tests.cpp index 127d9eaffaf2..6595cf457a02 100644 --- a/src/test/llmq_dkg_tests.cpp +++ b/src/test/llmq_dkg_tests.cpp @@ -2,6 +2,8 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include +#include #include #include #include @@ -120,4 +122,37 @@ BOOST_AUTO_TEST_CASE(pending_messages_quota_is_per_protx) BOOST_CHECK_EQUAL(pending.PopPendingMessages(6).size(), 5U); } +BOOST_AUTO_TEST_CASE(contribution_records_vvec_scheme) +{ + // A contribution remembers which BLS scheme its vvec was deserialized under, since only the basic scheme checks + // subgroup membership, which weighted contribution verification relies on + using namespace llmq; + + const bool legacy_scheme_before{bls::bls_legacy_scheme.load()}; + + uint256 seed{uint256S("0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef")}; + *seed.begin() = 0; // keep it below the group order + const CBLSSecretKey sk{Span{seed}}; + BOOST_REQUIRE(sk.IsValid()); + CDKGContribution qc; + qc.llmqType = Consensus::LLMQType::LLMQ_TEST; + qc.vvec = std::make_shared>(std::vector{sk.GetPublicKey()}); + qc.contributions = std::make_shared>(); + + for (const bool legacy : {true, false}) { + bls::bls_legacy_scheme.store(legacy); + CDataStream stream{SER_NETWORK, PROTOCOL_VERSION}; + stream << qc; + CDKGContribution received; + stream >> received; + BOOST_CHECK_EQUAL(received.vvecLegacyScheme, legacy); + BOOST_REQUIRE_EQUAL(received.vvec->size(), 1U); + BOOST_CHECK((*received.vvec)[0].IsValid()); + BOOST_CHECK((*received.vvec)[0] == (*qc.vvec)[0]); + BOOST_CHECK(stream.empty()); + } + + bls::bls_legacy_scheme.store(legacy_scheme_before); +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/llmq_utils_tests.cpp b/src/test/llmq_utils_tests.cpp index 31b10acb789c..16e2ed401590 100644 --- a/src/test/llmq_utils_tests.cpp +++ b/src/test/llmq_utils_tests.cpp @@ -275,9 +275,16 @@ BOOST_AUTO_TEST_CASE(sig_share_batch_verifier_judges_each_sender_by_its_own_shar sk_forger.MakeNewKey(); const CBLSSignature forged_sig{sk_forger.Sign(sig_share.GetSignHash(), legacy_scheme)}; + // Two wrong shares whose errors cancel out: (valid + forged) + (valid - forged) == 2 * valid + CBLSSignature plus_error_sig{valid_sig}; + plus_error_sig.AggregateInsecure(forged_sig); + CBLSSignature minus_error_sig{valid_sig}; + minus_error_sig.SubInsecure(forged_sig); + constexpr NodeId honest{1}; constexpr NodeId attacker{2}; constexpr NodeId other_honest{3}; + constexpr NodeId other_attacker{4}; struct Push { NodeId from; @@ -290,6 +297,8 @@ BOOST_AUTO_TEST_CASE(sig_share_batch_verifier_judges_each_sender_by_its_own_shar {{{attacker, forged_sig}, {honest, valid_sig}}, {attacker}}, // The same valid share relayed by two peers is not misbehavior. {{{honest, valid_sig}, {other_honest, valid_sig}}, {}}, + // Complementary errors from two peers must not pass as one valid aggregate. + {{{attacker, plus_error_sig}, {other_attacker, minus_error_sig}}, {attacker, other_attacker}}, }; for (const auto& [pushes, expected_bad_sources] : cases) { diff --git a/test/functional/feature_governance.py b/test/functional/feature_governance.py index f319fd180d9c..6a6ff64beade 100755 --- a/test/functional/feature_governance.py +++ b/test/functional/feature_governance.py @@ -390,6 +390,56 @@ def sync_gov(node): self.check_superblockbudget(True) self.check_superblock() + self.log.info("A matching trigger relayed before the maturity window must still be funded") + sb_block_height = 240 + window_start = sb_block_height - sb_maturity_window + # Blocks above came faster than 2.5 minutes apart, so get clear of the per-masternode trigger rate limit + self.bump_mocktime(3 * sb_cycle * 150, update_schedulers=False) + self.generate(self.nodes[0], window_start - 3 - self.nodes[0].getblockcount(), sync_fun=self.sync_blocks()) + + # The payee at window_start creates the trigger, so let it reach the window alone, then rewind it + payee_address = self.nodes[0].masternode("winners")[str(window_start + 1)] + early_idx = next(mn.nodeIdx for mn in self.mninfo + if self.nodes[0].protx("info", mn.proTxHash)["state"]["payoutAddress"] in payee_address) + early_node = self.nodes[early_idx] + self.isolate_node(early_idx) + fork_point = early_node.getblockcount() + 1 + for _ in range(3): + self.bump_mocktime(1) + self.generate(early_node, 1, sync_fun=self.no_op) + self.wait_until(lambda: have_trigger_for_height([early_node], sb_block_height), timeout=5) + early_trigger_hash = next(h for h, t in early_node.gobject("list", "valid", "triggers").items() + if json.loads(t["DataString"])["event_block_height"] == sb_block_height) + early_node.invalidateblock(early_node.getblockhash(fork_point)) + self.reconnect_isolated_node(early_idx, 0) + self.sync_blocks() + + def early_trigger_relayed(): + # Each bump runs the once-a-second governance relay flush + self.bump_mocktime(1) + return all(early_trigger_hash in node.gobject("list", "valid", "triggers") for node in self.nodes) + self.wait_until(early_trigger_relayed, timeout=10) + + self.log.info("Masternodes must not vote NO on it before the window opens") + for _ in range(2): + self.bump_mocktime(1) + self.generate(self.nodes[0], 1, sync_fun=self.sync_blocks()) + assert_equal(self.nodes[0].getblockcount(), window_start - 1) + for node in self.nodes: + # A masternode's own vote is visible on its own node without waiting for relay + node.syncwithvalidationinterfacequeue() + assert_equal(node.gobject("list", "valid", "triggers")[early_trigger_hash]["NoCount"], 0) + + self.log.info("Every masternode votes YES on it once the window opens") + self.bump_mocktime(1) + self.generate(self.nodes[0], 1, sync_fun=self.sync_blocks()) + self.wait_until(lambda: self.nodes[0].gobject("list", "valid", "triggers")[early_trigger_hash]["YesCount"] == self.mn_count, timeout=5) + n = sb_block_height - self.nodes[0].getblockcount() + self.bump_mocktime(n) + self.generate(self.nodes[0], n, sync_fun=self.sync_blocks()) + assert_equal(self.nodes[0].getblockcount(), sb_block_height) + self.check_superblock() + if __name__ == '__main__': DashGovernanceTest().main() diff --git a/test/functional/feature_llmq_simplepose.py b/test/functional/feature_llmq_simplepose.py index 4ad57c34c7e3..13b887e1a10b 100755 --- a/test/functional/feature_llmq_simplepose.py +++ b/test/functional/feature_llmq_simplepose.py @@ -60,6 +60,8 @@ def run_test(self): self.nodes[0].sporkupdate("SPORK_17_QUORUM_DKG_ENABLED", 0) self.wait_for_sporks_same() + self.test_cloned_contributions() + if not self.options.disable_spork23: # Lets isolate MNs one by one and verify that punishment/banning happens self.test_banning(self.isolate_mn, 2) @@ -108,6 +110,31 @@ def run_test(self): self.repair_masternodes(True) self.reset_probe_timeouts() + def test_cloned_contributions(self): + self.log.info("Two members dealing identical contributions are both excluded and the quorum still forms") + cloners = self.mninfo[:2] + others = self.mninfo[2:] + for mn in cloners: + mn.get_node(self).quorum('dkgsimerror', 'contribution-clone', 100) + qh = self.mine_quorum(mninfos_valid=others) + for mn in cloners: + mn.get_node(self).quorum('dkgsimerror', 'contribution-clone', 0) + + valid_members = {m['proTxHash'] for m in self.nodes[0].quorum('info', 100, qh)['members'] if m['valid']} + assert_equal(valid_members, {mn.proTxHash for mn in others}) + for mn in cloners: + assert check_punished(self.nodes[0], mn) + assert not check_banned(self.nodes[0], mn) + + self.log.info("Let the PoSe penalty of the cloners decay without running any DKG") + self.nodes[0].sporkupdate("SPORK_17_QUORUM_DKG_ENABLED", 4070908800) + self.wait_for_sporks_same() + self.generate(self.nodes[0], 66) + for mn in self.mninfo: + assert not check_punished(self.nodes[0], mn) + self.nodes[0].sporkupdate("SPORK_17_QUORUM_DKG_ENABLED", 0) + self.wait_for_sporks_same() + def isolate_mn(self, mn: MasternodeInfo): mn.get_node(self).setnetworkactive(False) self.wait_until(lambda: mn.get_node(self).getconnectioncount() == 0)