From 812834dc5fed83cf4d99994c9c503acccf5024e8 Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Tue, 27 Nov 2018 08:04:08 +0100 Subject: [PATCH] Put height into mined commitments and use it instead of the special handling of quorumVvecHash (#2501) * Allow to skip sig verification for CFinalCommitment::Verify * Add CFinalCommitmentTxPayload and CheckLLMQCommitment and use it As described in https://github.com/dashpay/dips/pull/31 (see discussion). * Properly ban nodes for invalid commitments --- src/evo/deterministicmns.cpp | 6 +- src/evo/specialtx.cpp | 2 +- src/llmq/quorums_blockprocessor.cpp | 24 ++++---- src/llmq/quorums_commitment.cpp | 89 ++++++++++++++++++++++------- src/llmq/quorums_commitment.h | 29 +++++++++- src/llmq/quorums_dummydkg.cpp | 2 +- 6 files changed, 111 insertions(+), 41 deletions(-) diff --git a/src/evo/deterministicmns.cpp b/src/evo/deterministicmns.cpp index 8ccd785d0a0a1..502f330b08dd4 100644 --- a/src/evo/deterministicmns.cpp +++ b/src/evo/deterministicmns.cpp @@ -674,12 +674,12 @@ bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, const C LogPrintf("CDeterministicMNManager::%s -- MN %s revoked operator key at height %d: %s\n", __func__, proTx.proTxHash.ToString(), nHeight, proTx.ToString()); } else if (tx.nType == TRANSACTION_QUORUM_COMMITMENT) { - llmq::CFinalCommitment qc; + llmq::CFinalCommitmentTxPayload qc; if (!GetTxPayload(tx, qc)) { assert(false); // this should have been handled already } - if (!qc.IsNull()) { - HandleQuorumCommitment(qc, newList); + if (!qc.commitment.IsNull()) { + HandleQuorumCommitment(qc.commitment, newList); } } } diff --git a/src/evo/specialtx.cpp b/src/evo/specialtx.cpp index 1b9956b3da874..7bd29ed51f69d 100644 --- a/src/evo/specialtx.cpp +++ b/src/evo/specialtx.cpp @@ -38,7 +38,7 @@ bool CheckSpecialTx(const CTransaction& tx, const CBlockIndex* pindexPrev, CVali case TRANSACTION_COINBASE: return CheckCbTx(tx, pindexPrev, state); case TRANSACTION_QUORUM_COMMITMENT: - return true; // can't really check much here. checks are done in ProcessBlock + return llmq::CheckLLMQCommitment(tx, pindexPrev, state); } return state.DoS(10, false, REJECT_INVALID, "bad-tx-type-check"); diff --git a/src/llmq/quorums_blockprocessor.cpp b/src/llmq/quorums_blockprocessor.cpp index c40c1a85c96d6..cde3f84ccad04 100644 --- a/src/llmq/quorums_blockprocessor.cpp +++ b/src/llmq/quorums_blockprocessor.cpp @@ -90,7 +90,7 @@ void CQuorumBlockProcessor::ProcessMessage(CNode* pfrom, const std::string& strC auto members = CLLMQUtils::GetAllQuorumMembers(type, qc.quorumHash); - if (!qc.Verify(members)) { + if (!qc.Verify(members, true)) { LOCK(cs_main); LogPrintf("CQuorumBlockProcessor::%s -- commitment for quorum %s:%d is not valid, peer=%d\n", __func__, qc.quorumHash.ToString(), qc.llmqType, pfrom->id); @@ -167,7 +167,7 @@ bool CQuorumBlockProcessor::ProcessCommitment(const CBlockIndex* pindexPrev, con } if (qc.IsNull()) { - if (!qc.VerifyNull(pindexPrev->nHeight + 1)) { + if (!qc.VerifyNull()) { return state.DoS(100, false, REJECT_INVALID, "bad-qc-invalid-null"); } return true; @@ -185,7 +185,7 @@ bool CQuorumBlockProcessor::ProcessCommitment(const CBlockIndex* pindexPrev, con auto members = CLLMQUtils::GetAllQuorumMembers(params.type, quorumHash); - if (!qc.Verify(members)) { + if (!qc.Verify(members, true)) { return state.DoS(100, false, REJECT_INVALID, "bad-qc-invalid"); } @@ -236,21 +236,18 @@ bool CQuorumBlockProcessor::GetCommitmentsFromBlock(const CBlock& block, const C for (const auto& tx : block.vtx) { if (tx->nType == TRANSACTION_QUORUM_COMMITMENT) { - CFinalCommitment qc; + CFinalCommitmentTxPayload qc; if (!GetTxPayload(*tx, qc)) { + // should not happen as it was verified before processing the block return state.DoS(100, false, REJECT_INVALID, "bad-tx-payload"); } - if (!consensus.llmqs.count((Consensus::LLMQType)qc.llmqType)) { - return state.DoS(100, false, REJECT_INVALID, "bad-qc-type"); - } - // only allow one commitment per type and per block - if (ret.count((Consensus::LLMQType)qc.llmqType)) { + if (ret.count((Consensus::LLMQType)qc.commitment.llmqType)) { return state.DoS(100, false, REJECT_INVALID, "bad-qc-dup"); } - ret.emplace((Consensus::LLMQType)qc.llmqType, std::move(qc)); + ret.emplace((Consensus::LLMQType)qc.commitment.llmqType, std::move(qc.commitment)); } } @@ -406,7 +403,6 @@ bool CQuorumBlockProcessor::GetMinableCommitment(Consensus::LLMQType llmqType, c if (it == minableCommitmentsByQuorum.end()) { // null commitment required ret = CFinalCommitment(Params().GetConsensus().llmqs.at(llmqType), quorumHash); - ret.quorumVvecHash = ::SerializeHash(std::make_pair(quorumHash, nHeight)); return true; } @@ -419,11 +415,13 @@ bool CQuorumBlockProcessor::GetMinableCommitmentTx(Consensus::LLMQType llmqType, { AssertLockHeld(cs_main); - CFinalCommitment qc; - if (!GetMinableCommitment(llmqType, pindexPrev, qc)) { + CFinalCommitmentTxPayload qc; + if (!GetMinableCommitment(llmqType, pindexPrev, qc.commitment)) { return false; } + qc.nHeight = pindexPrev->nHeight + 1; + CMutableTransaction tx; tx.nVersion = 3; tx.nType = TRANSACTION_QUORUM_COMMITMENT; diff --git a/src/llmq/quorums_commitment.cpp b/src/llmq/quorums_commitment.cpp index f8e0a6b51ab07..dd668a5c2d97c 100644 --- a/src/llmq/quorums_commitment.cpp +++ b/src/llmq/quorums_commitment.cpp @@ -6,6 +6,9 @@ #include "quorums_utils.h" #include "chainparams.h" +#include "validation.h" + +#include "evo/specialtx.h" #include @@ -24,7 +27,7 @@ CFinalCommitment::CFinalCommitment(const Consensus::LLMQParams& params, const ui LogPrintStr(strprintf("CFinalCommitment::%s -- %s", __func__, tinyformat::format(__VA_ARGS__))); \ } while(0) -bool CFinalCommitment::Verify(const std::vector& members) const +bool CFinalCommitment::Verify(const std::vector& members, bool checkSigs) const { if (nVersion == 0 || nVersion > CURRENT_VERSION) { return false; @@ -76,30 +79,33 @@ bool CFinalCommitment::Verify(const std::vector& members) } } - uint256 commitmentHash = CLLMQUtils::BuildCommitmentHash((uint8_t)params.type, quorumHash, validMembers, quorumPublicKey, quorumVvecHash); + // sigs are only checked when the block is processed + if (checkSigs) { + uint256 commitmentHash = CLLMQUtils::BuildCommitmentHash((uint8_t)params.type, quorumHash, validMembers, quorumPublicKey, quorumVvecHash); - std::vector memberPubKeys; - for (size_t i = 0; i < members.size(); i++) { - if (!signers[i]) { - continue; + std::vector memberPubKeys; + for (size_t i = 0; i < members.size(); i++) { + if (!signers[i]) { + continue; + } + memberPubKeys.emplace_back(members[i]->pdmnState->pubKeyOperator); } - memberPubKeys.emplace_back(members[i]->pdmnState->pubKeyOperator); - } - if (!membersSig.VerifySecureAggregated(memberPubKeys, commitmentHash)) { - LogPrintfFinalCommitment("invalid aggregated members signature\n"); - return false; - } + if (!membersSig.VerifySecureAggregated(memberPubKeys, commitmentHash)) { + LogPrintfFinalCommitment("invalid aggregated members signature\n"); + return false; + } - if (!quorumSig.VerifyInsecure(quorumPublicKey, commitmentHash)) { - LogPrintfFinalCommitment("invalid quorum signature\n"); - return false; + if (!quorumSig.VerifyInsecure(quorumPublicKey, commitmentHash)) { + LogPrintfFinalCommitment("invalid quorum signature\n"); + return false; + } } return true; } -bool CFinalCommitment::VerifyNull(int nHeight) const +bool CFinalCommitment::VerifyNull() const { if (!Params().GetConsensus().llmqs.count((Consensus::LLMQType)llmqType)) { LogPrintfFinalCommitment("invalid llmqType=%d\n", llmqType); @@ -111,11 +117,6 @@ bool CFinalCommitment::VerifyNull(int nHeight) const return false; } - uint256 expectedQuorumVvecHash = ::SerializeHash(std::make_pair(quorumHash, nHeight)); - if (quorumVvecHash != expectedQuorumVvecHash) { - return false; - } - return true; } @@ -143,4 +144,50 @@ void CFinalCommitment::ToJson(UniValue& obj) const obj.push_back(Pair("quorumPublicKey", quorumPublicKey.ToString())); } +bool CheckLLMQCommitment(const CTransaction& tx, const CBlockIndex* pindexPrev, CValidationState& state) +{ + CFinalCommitmentTxPayload qcTx; + if (!GetTxPayload(tx, qcTx)) { + return state.DoS(100, false, REJECT_INVALID, "bad-qc-payload"); + } + + if (qcTx.nVersion == 0 || qcTx.nVersion > CFinalCommitmentTxPayload::CURRENT_VERSION) { + return state.DoS(100, false, REJECT_INVALID, "bad-qc-version"); + } + + if (qcTx.nHeight != pindexPrev->nHeight + 1) { + return state.DoS(100, false, REJECT_INVALID, "bad-qc-height"); + } + + if (!mapBlockIndex.count(qcTx.commitment.quorumHash)) { + return state.DoS(100, false, REJECT_INVALID, "bad-qc-quorum-hash"); + } + + const CBlockIndex* pindexQuorum = mapBlockIndex[qcTx.commitment.quorumHash]; + + if (pindexQuorum != pindexPrev->GetAncestor(pindexQuorum->nHeight)) { + // not part of active chain + return state.DoS(100, false, REJECT_INVALID, "bad-qc-quorum-hash"); + } + + if (!Params().GetConsensus().llmqs.count((Consensus::LLMQType)qcTx.commitment.llmqType)) { + return state.DoS(100, false, REJECT_INVALID, "bad-qc-type"); + } + const auto& params = Params().GetConsensus().llmqs.at((Consensus::LLMQType)qcTx.commitment.llmqType); + + if (qcTx.commitment.IsNull()) { + if (!qcTx.commitment.VerifyNull()) { + return state.DoS(100, false, REJECT_INVALID, "bad-qc-invalid-null"); + } + return true; + } + + auto members = CLLMQUtils::GetAllQuorumMembers(params.type, qcTx.commitment.quorumHash); + if (!qcTx.commitment.Verify(members, false)) { + return state.DoS(100, false, REJECT_INVALID, "bad-qc-invalid"); + } + + return true; +} + } diff --git a/src/llmq/quorums_commitment.h b/src/llmq/quorums_commitment.h index bbf702b22bbf4..3eeea96779bb7 100644 --- a/src/llmq/quorums_commitment.h +++ b/src/llmq/quorums_commitment.h @@ -48,8 +48,8 @@ class CFinalCommitment return (int)std::count(validMembers.begin(), validMembers.end(), true); } - bool Verify(const std::vector& members) const; - bool VerifyNull(int nHeight) const; + bool Verify(const std::vector& members, bool checkSigs) const; + bool VerifyNull() const; bool VerifySizes(const Consensus::LLMQParams& params) const; void ToJson(UniValue& obj) const; @@ -79,6 +79,7 @@ class CFinalCommitment return false; } if (quorumPublicKey.IsValid() || + !quorumVvecHash.IsNull() || membersSig.IsValid() || quorumSig.IsValid()) { return false; @@ -87,6 +88,30 @@ class CFinalCommitment } }; +class CFinalCommitmentTxPayload +{ +public: + static const uint16_t CURRENT_VERSION = 1; + +public: + uint16_t nVersion{CURRENT_VERSION}; + uint32_t nHeight{(uint32_t)-1}; + CFinalCommitment commitment; + +public: + ADD_SERIALIZE_METHODS + + template + inline void SerializationOp(Stream& s, Operation ser_action) + { + READWRITE(nVersion); + READWRITE(nHeight); + READWRITE(commitment); + } +}; + +bool CheckLLMQCommitment(const CTransaction& tx, const CBlockIndex* pindexPrev, CValidationState& state); + } #endif //DASH_QUORUMS_COMMITMENT_H diff --git a/src/llmq/quorums_dummydkg.cpp b/src/llmq/quorums_dummydkg.cpp index 538cfa204a542..384720d9bae31 100644 --- a/src/llmq/quorums_dummydkg.cpp +++ b/src/llmq/quorums_dummydkg.cpp @@ -345,7 +345,7 @@ void CDummyDKG::CreateFinalCommitment(Consensus::LLMQType llmqType, const CBlock } fqc.membersSig = CBLSSignature::AggregateSecure(memberSigs, memberPubKeys, commitmentHash); - if (!fqc.Verify(members)) { + if (!fqc.Verify(members, true)) { LogPrintf("CDummyDKG::%s -- self created final commitment is invalid for quorum %s:%d, validMembers=%d, signers=%d\n", __func__, fqc.quorumHash.ToString(), fqc.llmqType, fqc.CountValidMembers(), fqc.CountSigners()); continue;