fix: now EHF transactions expires after nExpiryEHF blocks

src/chainparams.cpp

@@ -192,6 +192,7 @@ public:
         consensus.DIP0024Height = 1737792; // 0000000000000001342be9c0b75ad40c276beaad91616423c4d9cb101b3db438
         consensus.V19Height = 1899072; // 0000000000000015e32e73052d663626327004c81c5c22cb8b42c361015c0eae
         consensus.MinBIP9WarningHeight = 1899072 + 2016; // V19 activation height + miner confirmation window
+        consensus.nExpireEHF = 576 * 365; // one year
         consensus.powLimit = uint256S("00000fffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"); // ~uint256(0) >> 20
         consensus.nPowTargetTimespan = 24 * 60 * 60; // Dash: 1 day
         consensus.nPowTargetSpacing = 2.5 * 60; // Dash: 2.5 minutes
@@ -389,6 +390,7 @@ public:
         consensus.DIP0024Height = 769700; // 0000008d84e4efd890ae95c70a7a6126a70a80e5c19e4cb264a5b3469aeef172
         consensus.V19Height = 850100; // 000004728b8ff2a16b9d4eebb0fd61eeffadc9c7fe4b0ec0b5a739869401ab5b
         consensus.MinBIP9WarningHeight = 850100 + 2016;  // v19 activation height + miner confirmation window
+        consensus.nExpireEHF = 576 * 365; // one year
         consensus.powLimit = uint256S("00000fffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"); // ~uint256(0) >> 20
         consensus.nPowTargetTimespan = 24 * 60 * 60; // Dash: 1 day
         consensus.nPowTargetSpacing = 2.5 * 60; // Dash: 2.5 minutes
@@ -560,6 +562,7 @@ public:
         consensus.DIP0024Height = 300;
         consensus.V19Height = 300;
         consensus.MinBIP9WarningHeight = 300 + 2016; // v19 activation height + miner confirmation window
+        consensus.nExpireEHF = 576 * 365; // one year
         consensus.powLimit = uint256S("7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"); // ~uint256(0) >> 1
         consensus.nPowTargetTimespan = 24 * 60 * 60; // Dash: 1 day
         consensus.nPowTargetSpacing = 2.5 * 60; // Dash: 2.5 minutes
@@ -798,6 +801,7 @@ public:
         consensus.DIP0024Height = 900;
         consensus.V19Height = 900;
         consensus.MinBIP9WarningHeight = 0;
+        consensus.nExpireEHF = 576; // one day for RegTest
         consensus.powLimit = uint256S("7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"); // ~uint256(0) >> 1
         consensus.nPowTargetTimespan = 24 * 60 * 60; // Dash: 1 day
         consensus.nPowTargetSpacing = 2.5 * 60; // Dash: 2.5 minutes

src/consensus/params.h

@@ -112,6 +112,8 @@ struct Params {
     /** Don't warn about unknown BIP 9 activations below this height.
      * This prevents us from warning about the CSV and DIP activations. */
     int MinBIP9WarningHeight;
+    /** this value is used to expire signals of EHF */
+    int nExpireEHF;
     /**
      * Minimum blocks including miner confirmation of the total of nMinerConfirmationWindow blocks in a retargeting period,
      * (nPowTargetTimespan / nPowTargetSpacing) which is also used for BIP9 deployments.

src/evo/mnhftx.cpp

@@ -22,7 +22,22 @@
 extern const std::string MNEHF_REQUESTID_PREFIX = "mnhf";
 static const std::string DB_SIGNALS = "mnhf_s";
 
-bool MNHFTx::Verify(const CBlockIndex* pQuorumIndex, const uint256& msgHash, TxValidationState& state) const
+CMNHFManager::Signals CMNHFManager::GetSignalsStage(const CBlockIndex* const pindexPrev)
+{
+    Signals signals = GetFromCache(pindexPrev);
+    const int height = pindexPrev->nHeight + 1;
+    for (auto it = signals.begin(); it != signals.end(); ) {
+         if (height > it->second + Params().GetConsensus().nExpireEHF) {
+            LogPrintf("CMNHFManager::GetSignalsStage: mnhf signal bit=%d height:%d is expired at height=%d\n", it->first, it->second, height);
+            it = signals.erase(it);
+         } else {
+             ++it;
+         }
+    }
+    return signals;
+}
+
+bool MNHFTx::Verify(const CBlockIndex* const pQuorumIndex, const uint256& msgHash, TxValidationState& state) const
 {
     if (versionBit >= VERSIONBITS_NUM_BITS) {
         return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-mnhf-nbit-out-of-bounds");
@@ -75,18 +90,33 @@ bool CheckMNHFTx(const CTransaction& tx, const CBlockIndex* pindexPrev, TxValida
     SetTxPayload(tx_copy, payload_copy);
     uint256 msgHash = tx_copy.GetHash();
 
+
     if (!mnhfTx.signal.Verify(pindexQuorum, msgHash, state)) {
         // set up inside Verify
         return false;
     }
 
-    if (!Params().UpdateMNActivationParam(mnhfTx.signal.versionBit, pindexPrev->nHeight, pindexPrev->GetMedianTimePast(), true /* fJustCheck */)) {
+    if (!Params().UpdateMNActivationParam(mnhfTx.signal.versionBit, pindexPrev->nHeight, pindexPrev->GetMedianTimePast(), /* fJustCheck= */ true )) {
         return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-mnhf-non-ehf");
     }
 
     return true;
 }
 
+std::optional<uint8_t> extractEHFSignal(const CTransaction& tx)
+{
+    if (tx.nVersion != 3 || tx.nType != TRANSACTION_MNHF_SIGNAL) {
+        // only interested in special TXs 'TRANSACTION_MNHF_SIGNAL'
+        return std::nullopt;
+    }
+
+    MNHFTxPayload mnhfTx;
+    if (!GetTxPayload(tx, mnhfTx)) {
+        return std::nullopt;
+    }
+    return mnhfTx.signal.versionBit;
+}
+
 static bool extractSignals(const CBlock& block, const CBlockIndex* const pindex, std::vector<uint8_t>& signals_to_process, BlockValidationState& state)
 {
     AssertLockHeld(cs_main);
@@ -116,7 +146,7 @@ static bool extractSignals(const CBlock& block, const CBlockIndex* const pindex,
     std::sort(signals_to_process.begin(), signals_to_process.end());
     const auto it = std::unique(signals_to_process.begin(), signals_to_process.end());
     if (std::distance(signals_to_process.begin(), it) != signals_to_process.size()) {
-        return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-mnhf-duplicates");
+        return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-mnhf-duplicates-in-block");
     }
 
     return true;
@@ -130,19 +160,19 @@ bool CMNHFManager::ProcessBlock(const CBlock& block, const CBlockIndex* const pi
             // state is set inside extractSignals
             return false;
         }
+        Signals signals = GetSignalsStage(pindex->pprev);
         if (new_signals.empty()) {
             if (!fJustCheck) {
-                AddToCache(GetFromCache(pindex->pprev), pindex);
+                AddToCache(signals, pindex);
             }
             return true;
         }
 
-        Signals signals = GetFromCache(pindex->pprev);
         int mined_height = pindex->nHeight;
 
         // Extra validation of signals to be sure that it can succeed
         for (const auto& versionBit : new_signals) {
-            LogPrintf("%s: add mnhf bit=%d block:%s number of known signals:%lld\n", __func__, versionBit, pindex->GetBlockHash().ToString(), signals.size());
+            LogPrintf("CMNHFManager::ProcessBlock: add mnhf bit=%d block:%s number of known signals:%lld\n", versionBit, pindex->GetBlockHash().ToString(), signals.size());
             if (signals.find(versionBit) != signals.end()) {
                 return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-mnhf-duplicate");
             }
@@ -168,7 +198,7 @@ bool CMNHFManager::ProcessBlock(const CBlock& block, const CBlockIndex* const pi
         AddToCache(signals, pindex);
         return true;
     } catch (const std::exception& e) {
-        LogPrintf("%s -- failed: %s\n", __func__, e.what());
+        LogPrintf("CMNHFManager::ProcessBlock -- failed: %s\n", e.what());
         return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "failed-proc-mnhf-inblock");
     }
 }
@@ -204,24 +234,26 @@ void CMNHFManager::UpdateChainParams(const CBlockIndex* const pindex, const CBlo
     LogPrintf("%s: update chain params %s -> %s\n", __func__, pindexOld ? pindexOld->GetBlockHash().ToString() : "", pindex ? pindex->GetBlockHash().ToString() : "");
     Signals signals_old{GetFromCache(pindexOld)};
     for (const auto& signal: signals_old) {
-        uint8_t versionBit = signal.first;
+        const uint8_t versionBit = signal.first;
+
         assert(versionBit < VERSIONBITS_NUM_BITS);
 
         LogPrintf("%s: unload mnhf bit=%d block:%s number of known signals:%lld\n", __func__, versionBit, pindex->GetBlockHash().ToString(), signals_old.size());
 
-        bool update_ret = Params().UpdateMNActivationParam(versionBit, 0, pindex->GetMedianTimePast(), false);
+        bool update_ret = Params().UpdateMNActivationParam(versionBit, 0, pindex->GetMedianTimePast(), /* fJustCheck= */ false);
         assert(update_ret);
     }
 
     Signals signals{GetFromCache(pindex)};
     for (const auto& signal: signals) {
-        uint8_t versionBit = signal.first;
+        const uint8_t versionBit = signal.first;
-        int value = signal.second;
+        const int value = signal.second;
+
         assert(versionBit < VERSIONBITS_NUM_BITS);
 
         LogPrintf("%s: load mnhf bit=%d block:%s number of known signals:%lld\n", __func__, versionBit, pindex->GetBlockHash().ToString(), signals.size());
 
-        bool update_ret = Params().UpdateMNActivationParam(versionBit, value, pindex->GetMedianTimePast(), false);
+        bool update_ret = Params().UpdateMNActivationParam(versionBit, value, pindex->GetMedianTimePast(), /* fJustCheck= */ false);
         assert(update_ret);
     }
 }
@@ -240,9 +272,9 @@ CMNHFManager::Signals CMNHFManager::GetFromCache(const CBlockIndex* const pindex
     }
     if (VersionBitsState(pindex->pprev, Params().GetConsensus(), Consensus::DEPLOYMENT_V20, versionbitscache) != ThresholdState::ACTIVE) {
         LOCK(cs_cache);
-        mnhfCache.insert(blockHash, signals);
+        mnhfCache.insert(blockHash, {});
         LogPrintf("CMNHFManager::GetFromCache: mnhf feature is disabled: return empty for block %s\n", pindex->GetBlockHash().ToString());
-        return signals;
+        return {};
     }
     if (!m_evoDb.Read(std::make_pair(DB_SIGNALS, blockHash), signals)) {
         LogPrintf("CMNHFManager::GetFromCache: failure: can't read MnEHF signals from db for %s\n", pindex->GetBlockHash().ToString());
@@ -258,7 +290,7 @@ void CMNHFManager::AddToCache(const Signals& signals, const CBlockIndex* const p
     const uint256& blockHash = pindex->GetBlockHash();
     {
         LOCK(cs_cache);
-        LogPrintf("%s: mnhf for block %s add to cache: %lld\n", __func__, pindex->GetBlockHash().ToString(), signals.size());
+        LogPrintf("CMNHFManager::AddToCache: mnhf for block %s add to cache: %lld\n", pindex->GetBlockHash().ToString(), signals.size());
         mnhfCache.insert(blockHash, signals);
     }
     m_evoDb.Write(std::make_pair(DB_SIGNALS, blockHash), signals);

src/evo/mnhftx.h

@@ -11,6 +11,7 @@
 #include <threadsafety.h>
 #include <univalue.h>
 
+#include <optional>
 #include <saltedhasher.h>
 #include <unordered_map>
 #include <unordered_lru_cache.h>
@@ -31,7 +32,7 @@ public:
     CBLSSignature sig;
 
     MNHFTx() = default;
-    bool Verify(const CBlockIndex* pQuorumIndex, const uint256& msgHash, TxValidationState& state) const;
+    bool Verify(const CBlockIndex* const pQuorumIndex, const uint256& msgHash, TxValidationState& state) const;
 
     SERIALIZE_METHODS(MNHFTx, obj)
     {
@@ -113,11 +114,25 @@ public:
      */
     void UpdateChainParams(const CBlockIndex* const pindex, const CBlockIndex* const pindexOld) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
+    /**
+     * This function prepares signals for new block.
+     * This data is filterd expired signals from previous blocks.
+     * This member function is not const because it calls non-const GetFromCache()
+     */
+    Signals GetSignalsStage(const CBlockIndex* const pindexPrev);
 private:
     void AddToCache(const Signals& signals, const CBlockIndex* const pindex);
+
+    /**
+     * This function returns list of signals available on previous block.
+     * NOTE: that some signals could expired between blocks.
+     * validate them by
+     */
     Signals GetFromCache(const CBlockIndex* const pindex);
+
 };
 
+std::optional<uint8_t> extractEHFSignal(const CTransaction& tx);
 bool CheckMNHFTx(const CTransaction& tx, const CBlockIndex* pindexPrev, TxValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
 #endif // BITCOIN_EVO_MNHFTX_H

src/miner.cpp

@@ -24,6 +24,7 @@
 #include <evo/specialtx.h>
 #include <evo/cbtx.h>
 #include <evo/creditpool.h>
+#include <evo/mnhftx.h>
 #include <evo/simplifiedmns.h>
 #include <governance/governance.h>
 #include <llmq/blockprocessor.h>
@@ -174,7 +175,8 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
         LogPrintf("%s: CCreditPool is %s\n", __func__, creditPool.ToString());
         creditPoolDiff.emplace(std::move(creditPool), pindexPrev, chainparams.GetConsensus());
     }
-    addPackageTxs(nPackagesSelected, nDescendantsUpdated, creditPoolDiff);
+    std::unordered_map<uint8_t, int> signals = m_chainstate.m_mnhfManager.GetSignalsStage(pindexPrev);
+    addPackageTxs(nPackagesSelected, nDescendantsUpdated, creditPoolDiff, signals);
 
     int64_t nTime1 = GetTimeMicros();
 
@@ -400,7 +402,7 @@ void BlockAssembler::SortForBlock(const CTxMemPool::setEntries& package, std::ve
 // Each time through the loop, we compare the best transaction in
 // mapModifiedTxs with the next transaction in the mempool to decide what
 // transaction package to work on next.
-void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpdated, std::optional<CCreditPoolDiff>& creditPoolDiff)
+void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpdated, std::optional<CCreditPoolDiff>& creditPoolDiff, std::unordered_map<uint8_t, int>& signals)
 {
     AssertLockHeld(m_mempool.cs);
 
@@ -460,7 +462,7 @@ void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpda
         if (creditPoolDiff != std::nullopt) {
             // If one transaction is skipped due to limits, it is not a reason to interrupt
             // whole process of adding transactions.
-            // `state` is local here because used to log info about this specific tx
+            // `state` is local here because used only to log info about this specific tx
             TxValidationState state;
 
             if (!creditPoolDiff->ProcessLockUnlockTransaction(iter->GetTx(), state)) {
@@ -473,6 +475,14 @@ void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpda
                 continue;
             }
         }
+        if (std::optional<uint8_t> signal = extractEHFSignal(iter->GetTx()); signal != std::nullopt) {
+            if (signals.find(*signal) != signals.end()) {
+                LogPrintf("%s: ehf signal tx %s skipped due to duplicate %d\n",
+                          __func__, iter->GetTx().GetHash().ToString(), *signal);
+                continue;
+            }
+            signals.insert({*signal, 0});
+        }
 
         // We skip mapTx entries that are inBlock, and mapModifiedTx shouldn't
         // contain anything that is inBlock.

src/miner.h

@@ -194,7 +194,8 @@ private:
     /** Add transactions based on feerate including unconfirmed ancestors
       * Increments nPackagesSelected / nDescendantsUpdated with corresponding
       * statistics from the package selection (for logging statistics). */
-    void addPackageTxs(int& nPackagesSelected, int& nDescendantsUpdated, std::optional<CCreditPoolDiff>& creditPoolDiff) EXCLUSIVE_LOCKS_REQUIRED(m_mempool.cs);
+    void addPackageTxs(int& nPackagesSelected, int& nDescendantsUpdated,
+                       std::optional<CCreditPoolDiff>& creditPoolDiff, std::unordered_map<uint8_t, int>& signals) EXCLUSIVE_LOCKS_REQUIRED(m_mempool.cs);
 
     // helper functions for addPackageTxs()
     /** Remove confirmed (inBlock) entries from given set */

src/validation.h

@@ -585,7 +585,9 @@ private:
     const std::unique_ptr<llmq::CChainLocksHandler>& m_clhandler;
     const std::unique_ptr<llmq::CInstantSendManager>& m_isman;
     const std::unique_ptr<llmq::CQuorumBlockProcessor>& m_quorum_block_processor;
+public:
     CMNHFManager& m_mnhfManager;
+private:
     CEvoDB& m_evoDb;
 
 public:

test/functional/feature_mnehf.py

@@ -43,9 +43,18 @@ class MnehfTest(DashTestFramework):
             index = mn.node.index
             self.stop_node(index)
             self.start_masternode(mn)
-        for i in range(len(self.nodes)):
+        for i in range(1, self.num_nodes):
                 self.connect_nodes(i, 0)
 
+    def slowly_generate_batch(self, amount):
+        self.log.info(f"Slowly generate {amount} blocks")
+        while amount > 0:
+            self.log.info(f"Generating batch of blocks {amount} left")
+            next = min(10, amount)
+            amount -= next
+            self.bump_mocktime(next)
+            self.nodes[1].generate(next)
+            self.sync_all()
 
     def create_mnehf(self, versionBit, pubkey=None):
         # request ID = sha256("mnhf", versionBit)
@@ -69,7 +78,7 @@ class MnehfTest(DashTestFramework):
         mnehf_tx.calc_sha256()
         msgHash = format(mnehf_tx.sha256, '064x')
 
-        self.log.info(f"Signing request_id: {request_id} msgHash: {msgHash}")
+        self.log.info(f"Signing request_id: {request_id} msgHash: {msgHash} quorum: {quorumHash}")
         recsig = self.get_recovered_sig(request_id, msgHash)
 
         mnehf_payload.quorumSig = bytearray.fromhex(recsig["sig"])
@@ -94,10 +103,11 @@ class MnehfTest(DashTestFramework):
 
     def ensure_tx_is_not_mined(self, tx_id):
         try:
-            self.nodes[0].gettransaction(tx_id)
+            assert_equal(self.nodes[0].getrawtransaction(tx_id, 1)['height'], -1);
             raise AssertionError("Transaction should not be mined")
-        except JSONRPCException as e:
+        except KeyError as e:
-            assert "Invalid or non-wallet transaction id" in e.error['message']
+            # KeyError is expected
+            pass
 
     def send_tx(self, tx, expected_error = None, reason = None):
         try:
@@ -155,6 +165,7 @@ class MnehfTest(DashTestFramework):
         ehf_unknown_tx_sent = self.send_tx(ehf_unknown_tx)
         self.send_tx(ehf_invalid_tx, expected_error='bad-mnhf-non-ehf')
         node.generate(1)
+        ehf_height = node.getblockcount()
         self.sync_all()
 
         ehf_block = node.getbestblockhash()
@@ -236,17 +247,23 @@ class MnehfTest(DashTestFramework):
 
         ehf_tx_second = self.create_mnehf(28, pubkey)
         assert_equal(get_bip9_details(node, 'testdummy')['status'], 'defined')
+
+        self.log.info("Ehf with same bit signal should fail after 575 blocks but be accepted after 576 on regnet.")
+        self.log.info(f"Current progress is from {ehf_height} to {node.getblockcount()}")
+        self.slowly_generate_batch(576 - (node.getblockcount() - ehf_height))
         ehf_tx_sent = self.send_tx(ehf_tx_second)
+        self.log.info(f"ehf tx sent: {ehf_tx_sent}")
+        self.log.info(f"block: {node.getblock(node.getbestblockhash())}")
+        self.ensure_tx_is_not_mined(ehf_tx_sent)
         node.generate(1)
         self.sync_all()
+        self.log.info(f"block: {node.getblock(node.getbestblockhash())}")
+        block = node.getblock(node.getbestblockhash())
+        assert ehf_tx_sent in block['tx']
 
         self.check_fork('defined')
-        for i in range(10):
+        self.slowly_generate_batch(12 * 4)
-            self.log.info(f"Generating {i} ...")
+        self.check_fork('active')
-            self.bump_mocktime(next)
-            node.generate(next)
-            self.sync_all()
-        self.check_fork('started')