Put height into mined commitments and use it instead of the special h…

…andling of quorumVvecHash (dashpay#2501)

* Allow to skip sig verification for CFinalCommitment::Verify

* Add CFinalCommitmentTxPayload and CheckLLMQCommitment and use it

As described in dashpay/dips#31 (see discussion).

* Properly ban nodes for invalid commitments

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);
             }
         }
     }

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");

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;

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>
 
@@ -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<CDeterministicMNCPtr>& members) const
+bool CFinalCommitment::Verify(const std::vector<CDeterministicMNCPtr>& members, bool checkSigs) const
 {
     if (nVersion == 0 || nVersion > CURRENT_VERSION) {
         return false;
@@ -76,30 +79,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);
@@ -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;
+}
+
 }

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<CDeterministicMNCPtr>& members) const;
-    bool VerifyNull(int nHeight) const;
+    bool Verify(const std::vector<CDeterministicMNCPtr>& 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<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

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;