refactor: make MNActivationHeight in Params() indeed constant (#5658)

## Issue being fixed or feature implemented
Addressed issues and comments from [PR
comment](#5469 (comment))
and [PR
comment](#5469 (comment))

`Params()` should be const; global variable `CMNHFManager` is a better
out-come.


## What was done?
The helpers and direct calls of `UpdateMNParams` for each block to
update non-constant member in `Params()` is not needed anymore. Instead
`CMNHFManager` takes cares about status of Signals for each block,
update them dynamically and save in evo db.


## How Has This Been Tested?
Run unit/functional tests.

## Breaking Changes
Changed rpc `getblockchaininfo`. 
the field `ehf` changed meaning: it's now only a flag -1/0; but it is
introduced a new field `ehf_height` now that a height.


## 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
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone

---------

Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>
Co-authored-by: thephez <thephez@users.noreply.github.com>

src/chainparams.cpp

@@ -106,25 +106,22 @@ static CBlock FindDevNetGenesisBlock(const CBlock &prevBlock, const CAmount& rew
     assert(false);
 }
 
-bool CChainParams::UpdateMNActivationParam(int nBit, int height, int64_t timePast, bool fJustCheck) const
+bool CChainParams::IsValidMNActivation(int nBit, int64_t timePast) const
 {
     assert(nBit < VERSIONBITS_NUM_BITS);
 
     for (int index = 0; index < Consensus::MAX_VERSION_BITS_DEPLOYMENTS; ++index) {
         if (consensus.vDeployments[index].bit == nBit) {
             auto& deployment = consensus.vDeployments[index];
             if (timePast > deployment.nTimeout || timePast < deployment.nStartTime) {
-                LogPrintf("%s: activation by bit=%d height=%d deployment='%s' is out of time range start=%lld timeout=%lld\n", __func__, nBit, height, VersionBitsDeploymentInfo[Consensus::DeploymentPos(index)].name, deployment.nStartTime, deployment.nTimeout);
+                LogPrintf("%s: activation by bit=%d deployment='%s' is out of time range start=%lld timeout=%lld\n", __func__, nBit, VersionBitsDeploymentInfo[Consensus::DeploymentPos(index)].name, deployment.nStartTime, deployment.nTimeout);
                 continue;
             }
-            if (deployment.nMNActivationHeight < 0) {
-                LogPrintf("%s: trying to set MnEHF height=%d for non-masternode activation fork bit=%d\n", __func__, height, nBit);
+            if (!deployment.useEHF) {
+                LogPrintf("%s: trying to set MnEHF for non-masternode activation fork bit=%d\n", __func__, nBit);
                 return false;
             }
-            LogPrintf("%s: set MnEHF height=%d for bit=%d fJustCheck=%d is valid\n", __func__, height, nBit, fJustCheck);
-            if (!fJustCheck) {
-                deployment.nMNActivationHeight = height;
-            }
+            LogPrintf("%s: set MnEHF for bit=%d is valid\n", __func__, nBit);
             return true;
         }
     }
@@ -224,7 +221,7 @@ class CMainParams : public CChainParams {
         consensus.vDeployments[Consensus::DEPLOYMENT_MN_RR].nThresholdStart = 3226;     // 80% of 4032
         consensus.vDeployments[Consensus::DEPLOYMENT_MN_RR].nThresholdMin = 2420;       // 60% of 4032
         consensus.vDeployments[Consensus::DEPLOYMENT_MN_RR].nFalloffCoeff = 5;          // this corresponds to 10 periods
-        consensus.vDeployments[Consensus::DEPLOYMENT_MN_RR].nMNActivationHeight = 0;    // requires EHF activation
+        consensus.vDeployments[Consensus::DEPLOYMENT_MN_RR].useEHF = true;
 
         // The best chain should have at least this much work.
         consensus.nMinimumChainWork = uint256S("0x000000000000000000000000000000000000000000008677827656704520eb39"); // 1889000
@@ -422,7 +419,7 @@ class CTestNetParams : public CChainParams {
         consensus.vDeployments[Consensus::DEPLOYMENT_MN_RR].nThresholdStart = 80;       // 80% of 100
         consensus.vDeployments[Consensus::DEPLOYMENT_MN_RR].nThresholdMin = 60;         // 60% of 100
         consensus.vDeployments[Consensus::DEPLOYMENT_MN_RR].nFalloffCoeff = 5;          // this corresponds to 10 periods
-        consensus.vDeployments[Consensus::DEPLOYMENT_MN_RR].nMNActivationHeight = 0;    // requires EHF activation
+        consensus.vDeployments[Consensus::DEPLOYMENT_MN_RR].useEHF = true;
 
         // The best chain should have at least this much work.
         consensus.nMinimumChainWork = uint256S("0x00000000000000000000000000000000000000000000000002d68d24632e300f"); // 905100
@@ -595,7 +592,7 @@ class CDevNetParams : public CChainParams {
         consensus.vDeployments[Consensus::DEPLOYMENT_MN_RR].nThresholdStart = 80; // 80% of 100
         consensus.vDeployments[Consensus::DEPLOYMENT_MN_RR].nThresholdMin = 60;   // 60% of 100
         consensus.vDeployments[Consensus::DEPLOYMENT_MN_RR].nFalloffCoeff = 5;     // this corresponds to 10 periods
-        consensus.vDeployments[Consensus::DEPLOYMENT_MN_RR].nMNActivationHeight = 0;    // requires EHF activation
+        consensus.vDeployments[Consensus::DEPLOYMENT_MN_RR].useEHF = true;
 
         // The best chain should have at least this much work.
         consensus.nMinimumChainWork = uint256S("0x000000000000000000000000000000000000000000000000000000000000000");
@@ -845,7 +842,7 @@ class CRegTestParams : public CChainParams {
         consensus.vDeployments[Consensus::DEPLOYMENT_MN_RR].nThresholdStart = 9; // 80% of 12
         consensus.vDeployments[Consensus::DEPLOYMENT_MN_RR].nThresholdMin = 7;   // 60% of 7
         consensus.vDeployments[Consensus::DEPLOYMENT_MN_RR].nFalloffCoeff = 5;     // this corresponds to 10 periods
-        consensus.vDeployments[Consensus::DEPLOYMENT_MN_RR].nMNActivationHeight = 0;    // requires EHF activation
+        consensus.vDeployments[Consensus::DEPLOYMENT_MN_RR].useEHF = true;
 
         // The best chain should have at least this much work.
         consensus.nMinimumChainWork = uint256S("0x00");
@@ -953,7 +950,7 @@ class CRegTestParams : public CChainParams {
     /**
      * Allows modifying the Version Bits regtest parameters.
      */
-    void UpdateVersionBitsParameters(Consensus::DeploymentPos d, int64_t nStartTime, int64_t nTimeout, int64_t nWindowSize, int64_t nThresholdStart, int64_t nThresholdMin, int64_t nFalloffCoeff, int64_t nMNActivationHeight)
+    void UpdateVersionBitsParameters(Consensus::DeploymentPos d, int64_t nStartTime, int64_t nTimeout, int64_t nWindowSize, int64_t nThresholdStart, int64_t nThresholdMin, int64_t nFalloffCoeff, int64_t nUseEHF)
     {
         consensus.vDeployments[d].nStartTime = nStartTime;
         consensus.vDeployments[d].nTimeout = nTimeout;
@@ -969,8 +966,8 @@ class CRegTestParams : public CChainParams {
         if (nFalloffCoeff != -1) {
             consensus.vDeployments[d].nFalloffCoeff = nFalloffCoeff;
         }
-        if (nMNActivationHeight != -1) {
-            consensus.vDeployments[d].nMNActivationHeight = nMNActivationHeight;
+        if (nUseEHF != -1) {
+            consensus.vDeployments[d].useEHF = nUseEHF > 0;
         }
     }
     void UpdateActivationParametersFromArgs(const ArgsManager& args);
@@ -1049,9 +1046,9 @@ void CRegTestParams::UpdateActivationParametersFromArgs(const ArgsManager& args)
             throw std::runtime_error("Version bits parameters malformed, expecting "
                     "<deployment>:<start>:<end> or "
                     "<deployment>:<start>:<end>:<window>:<threshold> or "
-                    "<deployment>:<start>:<end>:<window>:<thresholdstart>:<thresholdmin>:<falloffcoeff>:<mnactivation>");
+                    "<deployment>:<start>:<end>:<window>:<thresholdstart>:<thresholdmin>:<falloffcoeff>:<useehf>");
         }
-        int64_t nStartTime, nTimeout, nWindowSize = -1, nThresholdStart = -1, nThresholdMin = -1, nFalloffCoeff = -1, nMNActivationHeight = -1;
+        int64_t nStartTime, nTimeout, nWindowSize = -1, nThresholdStart = -1, nThresholdMin = -1, nFalloffCoeff = -1, nUseEHF = -1;
         if (!ParseInt64(vDeploymentParams[1], &nStartTime)) {
             throw std::runtime_error(strprintf("Invalid nStartTime (%s)", vDeploymentParams[1]));
         }
@@ -1073,17 +1070,17 @@ void CRegTestParams::UpdateActivationParametersFromArgs(const ArgsManager& args)
             if (!ParseInt64(vDeploymentParams[6], &nFalloffCoeff)) {
                 throw std::runtime_error(strprintf("Invalid nFalloffCoeff (%s)", vDeploymentParams[6]));
             }
-            if (!ParseInt64(vDeploymentParams[7], &nMNActivationHeight)) {
-                throw std::runtime_error(strprintf("Invalid nMNActivationHeight (%s)", vDeploymentParams[7]));
+            if (!ParseInt64(vDeploymentParams[7], &nUseEHF)) {
+                throw std::runtime_error(strprintf("Invalid nUseEHF (%s)", vDeploymentParams[7]));
             }
         }
         bool found = false;
         for (int j=0; j < (int)Consensus::MAX_VERSION_BITS_DEPLOYMENTS; ++j) {
             if (vDeploymentParams[0] == VersionBitsDeploymentInfo[j].name) {
-                UpdateVersionBitsParameters(Consensus::DeploymentPos(j), nStartTime, nTimeout, nWindowSize, nThresholdStart, nThresholdMin, nFalloffCoeff, nMNActivationHeight);
+                UpdateVersionBitsParameters(Consensus::DeploymentPos(j), nStartTime, nTimeout, nWindowSize, nThresholdStart, nThresholdMin, nFalloffCoeff, nUseEHF);
                 found = true;
-                LogPrintf("Setting version bits activation parameters for %s to start=%ld, timeout=%ld, window=%ld, thresholdstart=%ld, thresholdmin=%ld, falloffcoeff=%ld, mnactivationHeight=%ld\n",
-                          vDeploymentParams[0], nStartTime, nTimeout, nWindowSize, nThresholdStart, nThresholdMin, nFalloffCoeff, nMNActivationHeight);
+                LogPrintf("Setting version bits activation parameters for %s to start=%ld, timeout=%ld, window=%ld, thresholdstart=%ld, thresholdmin=%ld, falloffcoeff=%ld, useehf=%ld\n",
+                          vDeploymentParams[0], nStartTime, nTimeout, nWindowSize, nThresholdStart, nThresholdMin, nFalloffCoeff, nUseEHF);
                 break;
             }
         }

src/chainparams.h

@@ -135,15 +135,13 @@ class CChainParams
     void UpdateBudgetParameters(int nMasternodePaymentsStartBlock, int nBudgetPaymentsStartBlock, int nSuperblockStartBlock);
     void UpdateLLMQInstantSend(Consensus::LLMQType llmqType);
     /**
-     * Update params for Masternodes EHF
+     * Validate params for Masternodes EHF
      *
      * @param[in] nBit The version bit to update
-     * @param[in] height The height of block where that signal is mined
      * @param[in] timePast The block time to validate if release is already time-outed
-     * @param[in] fJustCheck If true do not update any internal data, only validate params
      * @return Whether params are legit and params are updated (if release is known)
      */
-    bool UpdateMNActivationParam(int nBit, int height, int64_t timePast, bool fJustCheck) const;
+    bool IsValidMNActivation(int nBit, int64_t timePast) const;
     int PoolMinParticipants() const { return nPoolMinParticipants; }
     int PoolMaxParticipants() const { return nPoolMaxParticipants; }
     int FulfilledRequestExpireTime() const { return nFulfilledRequestExpireTime; }

src/consensus/params.h

@@ -40,6 +40,11 @@ struct BIP9Deployment {
     int64_t nThresholdMin{0};
     /** A coefficient which adjusts the speed a required number of signaling blocks is decreasing from nThresholdStart to nThresholdMin at with each period. */
     int64_t nFalloffCoeff{0};
+    /** This value is used for forks activated by masternodes.
+      * false means it is a regular fork, no masternodes confirmation is needed.
+      * true means that a signalling of masternodes is expected first to determine a height when miners signals are matter.
+      */
+    bool useEHF{false};
 
     /** Constant for nTimeout very far in the future. */
     static constexpr int64_t NO_TIMEOUT = std::numeric_limits<int64_t>::max();
@@ -49,13 +54,6 @@ struct BIP9Deployment {
      *  process (which takes at least 3 BIP9 intervals). Only tests that specifically test the
      *  behaviour during activation cannot use this. */
     static constexpr int64_t ALWAYS_ACTIVE = -1;
-
-    /** this value is used for forks activated by master nodes.
-      * negative values means it is regular fork, no masternodes confirmation is needed.
-      * 0 means that there's no approval from masternodes yet.
-      * Otherwise it shows minimum height when miner's signals for this block can be assumed
-      */
-    mutable int64_t nMNActivationHeight{-1};
 };
 
 /**

src/evo/mnhftx.cpp

@@ -37,6 +37,19 @@ CMutableTransaction MNHFTxPayload::PrepareTx() const
     return tx;
 }
 
+CMNHFManager::CMNHFManager(CEvoDB& evoDb) :
+    m_evoDb(evoDb)
+{
+    assert(globalInstance == nullptr);
+    globalInstance = this;
+}
+
+CMNHFManager::~CMNHFManager()
+{
+    assert(globalInstance != nullptr);
+    globalInstance = nullptr;
+}
+
 CMNHFManager::Signals CMNHFManager::GetSignalsStage(const CBlockIndex* const pindexPrev)
 {
     Signals signals = GetFromCache(pindexPrev);
@@ -123,7 +136,7 @@ bool CheckMNHFTx(const CTransaction& tx, const CBlockIndex* pindexPrev, TxValida
         return false;
     }
 
-    if (!Params().UpdateMNActivationParam(mnhfTx.signal.versionBit, pindexPrev->nHeight, pindexPrev->GetMedianTimePast(), /* fJustCheck= */ true )) {
+    if (!Params().IsValidMNActivation(mnhfTx.signal.versionBit, pindexPrev->GetMedianTimePast())) {
         return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-mnhf-non-ehf");
     }
 
@@ -202,7 +215,7 @@ bool CMNHFManager::ProcessBlock(const CBlock& block, const CBlockIndex* const pi
                 return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-mnhf-duplicate");
             }
 
-            if (!Params().UpdateMNActivationParam(versionBit, mined_height, pindex->GetMedianTimePast(), true /* fJustCheck */)) {
+            if (!Params().IsValidMNActivation(versionBit, pindex->GetMedianTimePast())) {
                 return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-mnhf-non-mn-fork");
             }
         }
@@ -211,11 +224,8 @@ bool CMNHFManager::ProcessBlock(const CBlock& block, const CBlockIndex* const pi
             return true;
         }
         for (const auto& versionBit : new_signals) {
-            signals.insert({versionBit, mined_height});
-
-            if (!Params().UpdateMNActivationParam(versionBit, mined_height, pindex->GetMedianTimePast(), false /* fJustCheck */)) {
-                // it should not ever fail - all checks are done above
-                assert(false);
+            if (Params().IsValidMNActivation(versionBit, pindex->GetMedianTimePast())) {
+                signals.insert({versionBit, mined_height});
             }
 
         }
@@ -244,42 +254,22 @@ bool CMNHFManager::UndoBlock(const CBlock& block, const CBlockIndex* const pinde
     for (const auto& versionBit : excluded_signals) {
         LogPrintf("%s: exclude mnhf bit=%d block:%s number of known signals:%lld\n", __func__, versionBit, pindex->GetBlockHash().ToString(), signals.size());
         assert(signals.find(versionBit) != signals.end());
-
-        bool update_ret = Params().UpdateMNActivationParam(versionBit, 0, pindex->GetMedianTimePast(), false /* fJustCheck */);
-        assert(update_ret);
+        assert(Params().IsValidMNActivation(versionBit, pindex->GetMedianTimePast()));
     }
 
     return true;
 }
 
-void CMNHFManager::UpdateChainParams(const CBlockIndex* const pindex, const CBlockIndex* const pindexOld)
+CMNHFManager::Signals CMNHFManager::GetFromCache(const CBlockIndex* const pindex)
 {
-    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) {
-        const uint8_t versionBit = signal.first;
-
-        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(), /* fJustCheck= */ false);
-        assert(update_ret);
-    }
-
-    Signals signals{GetFromCache(pindex)};
-    for (const auto& signal: signals) {
-        const uint8_t versionBit = signal.first;
-        const int value = signal.second;
+    if (pindex == nullptr) return {};
 
-        LogPrintf("%s: load mnhf bit=%d block:%s number of known signals:%lld\n", __func__, versionBit, pindex->GetBlockHash().ToString(), signals.size());
+    // TODO: remove this check of phashBlock to nullptr
+    // This check is needed only because unit test 'versionbits_tests.cpp'
+    // lets `phashBlock` to be nullptr
+    if (pindex->phashBlock == nullptr) return {};
 
-        bool update_ret = Params().UpdateMNActivationParam(versionBit, value, pindex->GetMedianTimePast(), /* fJustCheck= */ false);
-        assert(update_ret);
-    }
-}
 
-CMNHFManager::Signals CMNHFManager::GetFromCache(const CBlockIndex* const pindex)
-{
-    if (pindex == nullptr) return {};
     const uint256& blockHash = pindex->GetBlockHash();
     Signals signals{};
     {
@@ -293,9 +283,8 @@ CMNHFManager::Signals CMNHFManager::GetFromCache(const CBlockIndex* const pindex
         mnhfCache.insert(blockHash, {});
         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());
-    }
+    bool ok = m_evoDb.Read(std::make_pair(DB_SIGNALS, blockHash), signals);
+    assert(ok);
     LOCK(cs_cache);
     mnhfCache.insert(blockHash, signals);
     return signals;
@@ -308,9 +297,19 @@ void CMNHFManager::AddToCache(const Signals& signals, const CBlockIndex* const p
         LOCK(cs_cache);
         mnhfCache.insert(blockHash, signals);
     }
+    if (VersionBitsState(pindex->pprev, Params().GetConsensus(), Consensus::DEPLOYMENT_V20, versionbitscache) != ThresholdState::ACTIVE) {
+        return;
+    }
     m_evoDb.Write(std::make_pair(DB_SIGNALS, blockHash), signals);
 }
 
+void CMNHFManager::AddSignal(const CBlockIndex* const pindex, int bit)
+{
+    auto signals = GetFromCache(pindex->pprev);
+    signals.emplace(bit, pindex->nHeight);
+    AddToCache(signals, pindex);
+}
+
 std::string MNHFTx::ToString() const
 {
     return strprintf("MNHFTx(versionBit=%d, quorumHash=%s, sig=%s)",

src/evo/mnhftx.h

@@ -15,6 +15,7 @@
 #include <saltedhasher.h>
 #include <unordered_map>
 #include <unordered_lru_cache.h>
+#include <versionbits.h>
 
 class BlockValidationState;
 class CBlock;
@@ -91,11 +92,8 @@ class MNHFTxPayload
     }
 };
 
-class CMNHFManager
+class CMNHFManager : public AbstractEHFManager
 {
-public:
-    using Signals = std::unordered_map<uint8_t, int>;
-
 private:
     CEvoDB& m_evoDb;
 
@@ -105,9 +103,9 @@ class CMNHFManager
     unordered_lru_cache<uint256, Signals, StaticSaltedHasher> mnhfCache GUARDED_BY(cs_cache) {MNHFCacheSize};
 
 public:
-    explicit CMNHFManager(CEvoDB& evoDb) :
-        m_evoDb(evoDb) {}
-    ~CMNHFManager() = default;
+    explicit CMNHFManager(CEvoDB& evoDb);
+    ~CMNHFManager();
+    explicit CMNHFManager(const CMNHFManager&) = delete;
 
     /**
      * Every new block should be processed when Tip() is updated by calling of CMNHFManager::ProcessBlock
@@ -119,19 +117,14 @@ class CMNHFManager
      */
     bool UndoBlock(const CBlock& block, const CBlockIndex* const pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
-    /**
-     * Once app is started, need to initialize dictionary will all known signals at the current Tip()
-     * by calling UpdateChainParams()
-     */
-    void UpdateChainParams(const CBlockIndex* const pindex, const CBlockIndex* const pindexOld) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+
+    // Implements interface
+    Signals GetSignalsStage(const CBlockIndex* const pindexPrev) override;
 
     /**
-     * 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()
+     * Helper that used in Unit Test to forcely setup EHF signal for specific block
      */
-    Signals GetSignalsStage(const CBlockIndex* const pindexPrev);
-
+    void AddSignal(const CBlockIndex* const pindex, int bit) LOCKS_EXCLUDED(cs_cache);
 private:
     void AddToCache(const Signals& signals, const CBlockIndex* const pindex);
 

src/init.cpp

@@ -2103,9 +2103,6 @@ bool AppInitMain(const CoreContext& context, NodeContext& node, interfaces::Bloc
                             LogPrintf("%s: bls_legacy_scheme=%d\n", __func__, bls::bls_legacy_scheme.load());
                         }
 
-                        LogPrintf("init.cpp: update chain params right after bls\n");
-                        node.mnhf_manager->UpdateChainParams(::ChainActive().Tip(), nullptr);
-
                         if (!CVerifyDB().VerifyDB(
                                 *chainstate, chainparams, chainstate->CoinsDB(),
                                 *node.evodb,