fix!: making MnEhfTx to comply DIP-0023 (#5505)

## Issue being fixed or feature implemented
Current implementation of MnEhfTx is not matched with DIP-0023, this PR
fixes it. It is a prior work for
https://github.com/dashpay/dash/pull/5469

## What was done?
- requestID is fixed from `clsig{quorumHeight}` to `mnhf{versionBit}` +
fixes for signature validation properly
 - v20 is minimal height to accept MnEHF special transactions
- versionBit is not BLS version - removed unrelated wrong code and
validations
- TxMempool will accept MnEHF transaction even if inputs/outputs are
zeroes and no fee
- implemented python's serialization/deserialization of MnEHF
transactions for future using in functional tests
 

## How Has This Been Tested?
Run functional/unit tests. Beside that there's new functional test in
https://github.com/dashpay/dash/pull/5469 that actually test format of
transaction and signature validation - to be merged later.

## Breaking Changes
Payload of MnEhf tx is changed, related consensus rules are changed.


## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added or updated relevant unit/integration/functional/e2e
tests
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone

src/Makefile.test.include

@@ -98,6 +98,7 @@ BITCOIN_TESTS =\
   test/evo_assetlocks_tests.cpp \
   test/evo_deterministicmns_tests.cpp \
   test/evo_instantsend_tests.cpp \
+  test/evo_mnhf_tests.cpp \
   test/evo_simplifiedmns_tests.cpp \
   test/evo_trivialvalidation.cpp \
   test/evo_utils_tests.cpp \
@@ -146,7 +147,6 @@ BITCOIN_TESTS =\
   test/sigopcount_tests.cpp \
   test/skiplist_tests.cpp \
   test/sock_tests.cpp \
-  test/specialtx_tests.cpp \
   test/streams_tests.cpp \
   test/subsidy_tests.cpp \
   test/sync_tests.cpp \

src/consensus/tx_check.cpp

@@ -14,7 +14,7 @@ bool CheckTransaction(const CTransaction& tx, TxValidationState& state)
 {
     bool allowEmptyTxIn = false;
     bool allowEmptyTxOut = false;
-    if (tx.nType == TRANSACTION_QUORUM_COMMITMENT) {
+    if (tx.nType == TRANSACTION_QUORUM_COMMITMENT || tx.nType == TRANSACTION_MNHF_SIGNAL) {
         allowEmptyTxIn = true;
         allowEmptyTxOut = true;
     }

src/evo/mnhftx.cpp

@@ -13,29 +13,39 @@
 #include <chain.h>
 #include <chainparams.h>
 #include <validation.h>
+#include <versionbits.h>
 
 #include <string>
 
-extern const std::string CBLSIG_REQUESTID_PREFIX = "clsig";
+extern const std::string MNEHF_REQUESTID_PREFIX = "mnhf";
 
-bool MNHFTx::Verify(const CBlockIndex* pQuorumIndex) const
+bool MNHFTx::Verify(const CBlockIndex* pQuorumIndex, const uint256& msgHash, TxValidationState& state) const
 {
-    if (nVersion == 0 || nVersion > (llmq::utils::IsV19Active(pQuorumIndex) ? BASIC_BLS_VERSION : LEGACY_BLS_VERSION)) {
-        return false;
+    if (versionBit >= VERSIONBITS_NUM_BITS) {
+        return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-mnhf-nbit-out-of-bounds");
     }
 
     Consensus::LLMQType llmqType = Params().GetConsensus().llmqTypeMnhf;
     const auto& llmq_params_opt = llmq::GetLLMQParams(llmqType);
-    assert(llmq_params_opt.has_value());
+    if (!llmq_params_opt.has_value()) {
+        return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-mnhf-quorum-type");
+    }
     int signOffset{llmq_params_opt->dkgInterval};
 
-    const uint256 requestId = ::SerializeHash(std::make_pair(CBLSIG_REQUESTID_PREFIX, pQuorumIndex->nHeight));
-    return llmq::CSigningManager::VerifyRecoveredSig(llmqType, *llmq::quorumManager, pQuorumIndex->nHeight, requestId, pQuorumIndex->GetBlockHash(), sig, 0) ||
-           llmq::CSigningManager::VerifyRecoveredSig(llmqType, *llmq::quorumManager, pQuorumIndex->nHeight, requestId, pQuorumIndex->GetBlockHash(), sig, signOffset);
+    const uint256 requestId = ::SerializeHash(std::make_pair(MNEHF_REQUESTID_PREFIX, int64_t{versionBit}));
+
+    if (!llmq::CSigningManager::VerifyRecoveredSig(llmqType, *llmq::quorumManager, pQuorumIndex->nHeight + signOffset, requestId, msgHash, sig)) {
+        return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-mnhf-invalid");
+    }
+    return true;
 }
 
 bool CheckMNHFTx(const CTransaction& tx, const CBlockIndex* pindexPrev, TxValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
 {
+    if (tx.nVersion != 3 || tx.nType != TRANSACTION_MNHF_SIGNAL) {
+        return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-mnhf-type");
+    }
+
     MNHFTxPayload mnhfTx;
     if (!GetTxPayload(tx, mnhfTx)) {
         return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-mnhf-payload");
@@ -55,12 +65,16 @@ bool CheckMNHFTx(const CTransaction& tx, const CBlockIndex* pindexPrev, TxValida
         return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-mnhf-quorum-hash");
     }
 
-    if (!llmq::GetLLMQParams(Params().GetConsensus().llmqTypeMnhf).has_value()) {
-        return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-mnhf-type");
-    }
+    // Copy transaction except `quorumSig` field to calculate hash
+    CMutableTransaction tx_copy(tx);
+    auto payload_copy = mnhfTx;
+    payload_copy.signal.sig = CBLSSignature();
+    SetTxPayload(tx_copy, payload_copy);
+    uint256 msgHash = tx_copy.GetHash();
 
-    if (!mnhfTx.signal.Verify(pindexQuorum)) {
-        return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-mnhf-invalid");
+    if (!mnhfTx.signal.Verify(pindexQuorum, msgHash, state)) {
+        // set up inside Verify
+        return false;
     }
 
     return true;
@@ -68,7 +82,11 @@ bool CheckMNHFTx(const CTransaction& tx, const CBlockIndex* pindexPrev, TxValida
 
 std::string MNHFTx::ToString() const
 {
-    return strprintf("MNHFTx(nVersion=%d, quorumHash=%s, sig=%s)",
-                     nVersion, quorumHash.ToString(), sig.ToString());
+    return strprintf("MNHFTx(versionBit=%d, quorumHash=%s, sig=%s)",
+                     versionBit, quorumHash.ToString(), sig.ToString());
+}
+std::string MNHFTxPayload::ToString() const
+{
+    return strprintf("MNHFTxPayload(nVersion=%d, signal=%s)",
+                     nVersion, signal.ToString());
 }
-

src/evo/mnhftx.h

@@ -19,20 +19,17 @@ extern RecursiveMutex cs_main;
 class MNHFTx
 {
 public:
-    static constexpr uint16_t LEGACY_BLS_VERSION = 1;
-    static constexpr uint16_t BASIC_BLS_VERSION = 2;
-
-    uint16_t nVersion{LEGACY_BLS_VERSION};
+    uint8_t versionBit{0};
     uint256 quorumHash;
     CBLSSignature sig;
 
     MNHFTx() = default;
-    bool Verify(const CBlockIndex* pQuorumIndex) const;
+    bool Verify(const CBlockIndex* pQuorumIndex, const uint256& msgHash, TxValidationState& state) const;
 
     SERIALIZE_METHODS(MNHFTx, obj)
     {
-        READWRITE(obj.nVersion, obj.quorumHash);
-        READWRITE(CBLSSignatureVersionWrapper(const_cast<CBLSSignature&>(obj.sig), (obj.nVersion == LEGACY_BLS_VERSION)));
+        READWRITE(obj.versionBit, obj.quorumHash);
+        READWRITE(CBLSSignatureVersionWrapper(const_cast<CBLSSignature&>(obj.sig), /* fLegacy= */ false));
     }
 
     std::string ToString() const;
@@ -41,7 +38,7 @@ public:
     {
         obj.clear();
         obj.setObject();
-        obj.pushKV("version", (int)nVersion);
+        obj.pushKV("versionBit", (int)versionBit);
         obj.pushKV("quorumHash", quorumHash.ToString());
         obj.pushKV("sig", sig.ToString());
     }
@@ -53,7 +50,7 @@ public:
     static constexpr auto SPECIALTX_TYPE = TRANSACTION_MNHF_SIGNAL;
     static constexpr uint16_t CURRENT_VERSION = 1;
 
-    uint16_t nVersion{CURRENT_VERSION};
+    uint8_t nVersion{CURRENT_VERSION};
     MNHFTx signal;
 
     SERIALIZE_METHODS(MNHFTxPayload, obj)
@@ -61,6 +58,8 @@ public:
         READWRITE(obj.nVersion, obj.signal);
     }
 
+    std::string ToString() const;
+
     void ToJson(UniValue& obj) const
     {
         obj.setObject();

src/evo/specialtxman.cpp

@@ -45,7 +45,10 @@ bool CheckSpecialTx(const CTransaction& tx, const CBlockIndex* pindexPrev, const
         case TRANSACTION_QUORUM_COMMITMENT:
             return llmq::CheckLLMQCommitment(tx, pindexPrev, state);
         case TRANSACTION_MNHF_SIGNAL:
-            return pindexPrev->nHeight + 1 >= Params().GetConsensus().DIP0024Height && CheckMNHFTx(tx, pindexPrev, state);
+            if (!llmq::utils::IsV20Active(pindexPrev)) {
+                return state.Invalid(TxValidationResult::TX_CONSENSUS, "mnhf-before-v20");
+            }
+            return CheckMNHFTx(tx, pindexPrev, state);
         case TRANSACTION_ASSET_LOCK:
         case TRANSACTION_ASSET_UNLOCK:
             if (!llmq::utils::IsV20Active(pindexPrev)) {

src/test/evo_mnhf_tests.cpp

@@ -36,7 +36,7 @@ static CMutableTransaction CreateMNHFTx(const uint256& mnhfTxHash, const CBLSSig
 {
     MNHFTxPayload extraPayload;
     extraPayload.nVersion = 1;
-    extraPayload.signal.nVersion = versionBit;
+    extraPayload.signal.versionBit = versionBit;
     extraPayload.signal.quorumHash = mnhfTxHash;
     extraPayload.signal.sig = cblSig;
 
@@ -48,7 +48,7 @@ static CMutableTransaction CreateMNHFTx(const uint256& mnhfTxHash, const CBLSSig
     return tx;
 }
 
-BOOST_FIXTURE_TEST_SUITE(specialtx_tests, BasicTestingSetup)
+BOOST_FIXTURE_TEST_SUITE(evo_mnhf_tests, BasicTestingSetup)
 
 BOOST_AUTO_TEST_CASE(verify_mnhf_specialtx_tests)
 {

src/txmempool.cpp

@@ -457,6 +457,8 @@ void CTxMemPool::addUnchecked(const CTxMemPoolEntry &entry, setEntries &setAnces
         bool ok = GetTxPayload(tx, assetUnlockTx);
         assert(ok);
         mapAssetUnlockExpiry.insert({tx.GetHash(), assetUnlockTx.getHeightToExpiry()});
+    } else if (tx.nType == TRANSACTION_MNHF_SIGNAL) {
+        PrioritiseTransaction(tx.GetHash(), 0.1 * COIN);
     }
 }
 

test/functional/test_framework/messages.py

@@ -1176,6 +1176,45 @@ class CAssetUnlockTx:
             .format(self.version, self.index, self.fee, self.requestedHeight, self.quorumHash, self.quorumSig.hex())
 
 
+class CMnEhf:
+    __slots__ = ("version", "versionBit", "quorumHash", "quorumSig")
+
+    def __init__(self, version=None, versionBit=None, quorumHash = 0, quorumSig = None):
+        self.set_null()
+        if version is not None:
+            self.version = version
+        if versionBit is not None:
+            self.versionBit = versionBit
+        if quorumHash is not None:
+            self.quorumHash = quorumHash
+        if quorumSig is not None:
+            self.quorumSig = quorumSig
+
+    def set_null(self):
+        self.version = 0
+        self.versionBit = 0
+        self.quorumHash = 0
+        self.quorumSig = b'\x00' * 96
+
+    def deserialize(self, f):
+        self.version = struct.unpack("<B", f.read(1))[0]
+        self.versionBit = struct.unpack("<B", f.read(1))[0]
+        self.quorumHash = deser_uint256(f)
+        self.quorumSig = f.read(96)
+
+    def serialize(self):
+        r = b""
+        r += struct.pack("<B", self.version)
+        r += struct.pack("<B", self.versionBit)
+        r += ser_uint256(self.quorumHash)
+        r += self.quorumSig
+        return r
+
+    def __repr__(self):
+        return "CMnEhf(version={} versionBit={} quorumHash={:x} quorumSig={}" \
+            .format(self.version, self.versionBit, self.quorumHash, self.quorumSig.hex())
+
+
 class CSimplifiedMNListEntry:
     __slots__ = ("proRegTxHash", "confirmedHash", "service", "pubKeyOperator", "keyIDVoting", "isValid", "nVersion", "type", "platformHTTPPort", "platformNodeID")