From 6bc2fe5551ad3ce60b447007feda5ac1d3676128 Mon Sep 17 00:00:00 2001
From: Alexander Block <ablock84@gmail.com>
Date: Mon, 26 Nov 2018 17:38:57 +0100
Subject: [PATCH 1/3] Allow to skip sig verification for
 CFinalCommitment::Verify

---
 src/llmq/quorums_blockprocessor.cpp |  4 ++--
 src/llmq/quorums_commitment.cpp     | 35 ++++++++++++++++-------------
 src/llmq/quorums_commitment.h       |  2 +-
 src/llmq/quorums_dummydkg.cpp       |  2 +-
 4 files changed, 23 insertions(+), 20 deletions(-)

diff --git a/src/llmq/quorums_blockprocessor.cpp b/src/llmq/quorums_blockprocessor.cpp
index c40c1a85c96d..f87754545d3d 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);
@@ -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");
     }
 
diff --git a/src/llmq/quorums_commitment.cpp b/src/llmq/quorums_commitment.cpp
index f8e0a6b51ab0..e81347d77570 100644
--- a/src/llmq/quorums_commitment.cpp
+++ b/src/llmq/quorums_commitment.cpp
@@ -24,7 +24,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<CDeterministicMNCPtr>& members) const
+bool CFinalCommitment::Verify(const std::vector<CDeterministicMNCPtr>& members, bool checkSigs) const
 {
     if (nVersion == 0 || nVersion > CURRENT_VERSION) {
         return false;
@@ -76,30 +76,33 @@ bool CFinalCommitment::Verify(const std::vector<CDeterministicMNCPtr>& 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<CBLSPublicKey> memberPubKeys;
-    for (size_t i = 0; i < members.size(); i++) {
-        if (!signers[i]) {
-            continue;
+        std::vector<CBLSPublicKey> 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);
diff --git a/src/llmq/quorums_commitment.h b/src/llmq/quorums_commitment.h
index bbf702b22bbf..749c3d0fd9a4 100644
--- a/src/llmq/quorums_commitment.h
+++ b/src/llmq/quorums_commitment.h
@@ -48,7 +48,7 @@ class CFinalCommitment
         return (int)std::count(validMembers.begin(), validMembers.end(), true);
     }
 
-    bool Verify(const std::vector<CDeterministicMNCPtr>& members) const;
+    bool Verify(const std::vector<CDeterministicMNCPtr>& members, bool checkSigs) const;
     bool VerifyNull(int nHeight) const;
     bool VerifySizes(const Consensus::LLMQParams& params) const;
 
diff --git a/src/llmq/quorums_dummydkg.cpp b/src/llmq/quorums_dummydkg.cpp
index 538cfa204a54..384720d9bae3 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;

From bfca0e02f72de809ebf7309d56155f43a7654525 Mon Sep 17 00:00:00 2001
From: Alexander Block <ablock84@gmail.com>
Date: Mon, 26 Nov 2018 17:43:58 +0100
Subject: [PATCH 2/3] Add CFinalCommitmentTxPayload and CheckLLMQCommitment and
 use it

As described in https://github.com/dashpay/dips/pull/31 (see discussion).
---
 src/evo/deterministicmns.cpp        |  6 ++--
 src/evo/specialtx.cpp               |  2 +-
 src/llmq/quorums_blockprocessor.cpp | 20 +++++------
 src/llmq/quorums_commitment.cpp     | 55 ++++++++++++++++++++++++++---
 src/llmq/quorums_commitment.h       | 27 +++++++++++++-
 5 files changed, 89 insertions(+), 21 deletions(-)

diff --git a/src/evo/deterministicmns.cpp b/src/evo/deterministicmns.cpp
index 8ccd785d0a0a..502f330b08dd 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 1b9956b3da87..7bd29ed51f69 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 f87754545d3d..cde3f84ccad0 100644
--- a/src/llmq/quorums_blockprocessor.cpp
+++ b/src/llmq/quorums_blockprocessor.cpp
@@ -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;
@@ -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 e81347d77570..428336948cfe 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 <univalue.h>
 
@@ -114,11 +117,6 @@ bool CFinalCommitment::VerifyNull() const
         return false;
     }
 
-    uint256 expectedQuorumVvecHash = ::SerializeHash(std::make_pair(quorumHash, nHeight));
-    if (quorumVvecHash != expectedQuorumVvecHash) {
-        return false;
-    }
-
     return true;
 }
 
@@ -146,4 +144,51 @@ 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)) {
+        LogPrintfFinalCommitment("invalid llmqType=%d\n", qcTx.commitment.llmqType);
+        return false;
+    }
+    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 false;
+    }
+
+    return true;
+}
+
 }
diff --git a/src/llmq/quorums_commitment.h b/src/llmq/quorums_commitment.h
index 749c3d0fd9a4..3eeea96779bb 100644
--- a/src/llmq/quorums_commitment.h
+++ b/src/llmq/quorums_commitment.h
@@ -49,7 +49,7 @@ class CFinalCommitment
     }
 
     bool Verify(const std::vector<CDeterministicMNCPtr>& members, bool checkSigs) const;
-    bool VerifyNull(int nHeight) 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<typename Stream, typename Operation>
+    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

From 9f21ee1f82c7b6980275ada0a95fc7c885d99d9e Mon Sep 17 00:00:00 2001
From: Alexander Block <ablock84@gmail.com>
Date: Mon, 26 Nov 2018 20:13:45 +0100
Subject: [PATCH 3/3] Properly ban nodes for invalid commitments

---
 src/llmq/quorums_commitment.cpp | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/llmq/quorums_commitment.cpp b/src/llmq/quorums_commitment.cpp
index 428336948cfe..dd668a5c2d97 100644
--- a/src/llmq/quorums_commitment.cpp
+++ b/src/llmq/quorums_commitment.cpp
@@ -171,8 +171,7 @@ bool CheckLLMQCommitment(const CTransaction& tx, const CBlockIndex* pindexPrev,
     }
 
     if (!Params().GetConsensus().llmqs.count((Consensus::LLMQType)qcTx.commitment.llmqType)) {
-        LogPrintfFinalCommitment("invalid llmqType=%d\n", qcTx.commitment.llmqType);
-        return false;
+        return state.DoS(100, false, REJECT_INVALID, "bad-qc-type");
     }
     const auto& params = Params().GetConsensus().llmqs.at((Consensus::LLMQType)qcTx.commitment.llmqType);
 
@@ -185,7 +184,7 @@ bool CheckLLMQCommitment(const CTransaction& tx, const CBlockIndex* pindexPrev,
 
     auto members = CLLMQUtils::GetAllQuorumMembers(params.type, qcTx.commitment.quorumHash);
     if (!qcTx.commitment.Verify(members, false)) {
-        return false;
+        return state.DoS(100, false, REJECT_INVALID, "bad-qc-invalid");
     }
 
     return true;
