fix: assertion in CMNHFManager due to missing data in evoDB (dashpay#5736)

Prior required changes: bitcoin#19438 from
dashpay#5740

## Issue being fixed or feature implemented
Assertion failure:
```
  assertion: ok
  file: evo/mnhftx.cpp, line: 287
  function: AbstractEHFManager::Signals CMNHFManager::GetFromCache(const CBlockIndex*)
No debug information available for stacktrace. You should add debug information and then run: dash-qt -printcrashinfo=bvcgc43iinzgc43ijfxgm3ybaadwiyltnawxc5e3ifzxgzlsoruw63ramzqws3dvojstucraebqxg43foj2gs33ohiqg62ykeaqgm2lmmu5cazlwn4xw23timz2hqltdobycyidmnfxgkoragi4docraebthk3tdoruw63r2ebawe43uojqwg5cfjbde2ylomftwk4r2hjjwsz3omfwhgicdjvheqrsnmfxgcz3foi5dur3fordhe33ninqwg2dffbrw63ttoqqegqtmn5rwwslomrsxqkrjbyrelhyaaaaaaaedgkiaaaaaaaadsm4qaaaaaaaa7gpyqaaaaaaaa2njraaaaaaaadkl3caaaaaaaabhxznqaaaaaaadqa2eaaaaaaaax33twaaaaaaabwaihqaaaaaaac7yooqbaaaaaahba45qcaaaaaacwkz2aeaaaaaaeitgeaiaaaaaaaa=
dash-qt: evo/mnhftx.cpp:287: AbstractEHFManager::Signals CMNHFManager::GetFromCache(const CBlockIndex*): Assertion `ok' failed.
```

This can happen in case if Dash Core has been update from v19 (or
earlier v20.alphaX) to v20.0.0 after v20 activation without re-indexing

## What was done?
`CMNHFManager` is visiting missing blocks recursively until reach first
v20 block or first block actually saved in evoDb.


Without changes from bitcoin#19438 there's an other issue:
```
2023-11-27T11:12:10Z POTENTIAL DEADLOCK DETECTED
Previous lock order was:
 (2) 'cs_main' in llmq/instantsend.cpp:459 (in thread 'isman')
 (1) 'cs_llmq_vbc' in llmq/utils.cpp:711 (in thread 'isman')
Current lock order is:
 'cs_dip3list' in qt/masternodelist.cpp:135 (TRY) (in thread 'main')
 (1) 'cs_llmq_vbc' in llmq/utils.cpp:719 (in thread 'main')
 (2) 'cs_main' in node/blockstorage.cpp:77 (in thread 'main')
Assertion failed: detected inconsistent lock order for 'cs_main' in node/blockstorage.cpp:77 (in thread 'main'), details in debug log.
2023-11-27T11:12:10Z Posix Signal: Aborted
```

## How Has This Been Tested?
Run unit/functional test; run dash-qt on my local backup of problematic
storage (succeed without error); reindex testnet.

## Breaking Changes
N/A


## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] 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 _(for repository
code-owners and collaborators only)_

Author: knst

src/evo/mnhftx.cpp

@@ -10,13 +10,15 @@
 #include <llmq/signing.h>
 #include <llmq/utils.h>
 #include <llmq/quorums.h>
+#include <node/blockstorage.h>
 
 #include <chain.h>
 #include <chainparams.h>
 #include <validation.h>
 #include <versionbits.h>
 
 #include <algorithm>
+#include <stack>
 #include <string>
 #include <vector>
 
@@ -53,7 +55,7 @@ CMNHFManager::~CMNHFManager()
 
 CMNHFManager::Signals CMNHFManager::GetSignalsStage(const CBlockIndex* const pindexPrev)
 {
-    Signals signals = GetFromCache(pindexPrev);
+    Signals signals = GetForBlock(pindexPrev);
     const int height = pindexPrev->nHeight + 1;
     for (auto it = signals.begin(); it != signals.end(); ) {
         bool found{false};
@@ -99,7 +101,7 @@ bool MNHFTx::Verify(const uint256& quorumHash, const uint256& requestId, const u
     return true;
 }
 
-bool CheckMNHFTx(const CTransaction& tx, const CBlockIndex* pindexPrev, TxValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
+bool CheckMNHFTx(const CTransaction& tx, const CBlockIndex* pindexPrev, TxValidationState& state)
 {
     if (tx.nVersion != 3 || tx.nType != TRANSACTION_MNHF_SIGNAL) {
         return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-mnhf-type");
@@ -114,7 +116,7 @@ bool CheckMNHFTx(const CTransaction& tx, const CBlockIndex* pindexPrev, TxValida
         return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-mnhf-version");
     }
 
-    const CBlockIndex* pindexQuorum = g_chainman.m_blockman.LookupBlockIndex(mnhfTx.signal.quorumHash);
+    const CBlockIndex* pindexQuorum = WITH_LOCK(::cs_main, return g_chainman.m_blockman.LookupBlockIndex(mnhfTx.signal.quorumHash));
     if (!pindexQuorum) {
         return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-mnhf-quorum-hash");
     }
@@ -160,8 +162,6 @@ std::optional<uint8_t> extractEHFSignal(const CTransaction& tx)
 
 static bool extractSignals(const CBlock& block, const CBlockIndex* const pindex, std::vector<uint8_t>& new_signals, BlockValidationState& state)
 {
-    AssertLockHeld(cs_main);
-
     // we skip the coinbase
     for (size_t i = 1; i < block.vtx.size(); ++i) {
         const CTransaction& tx = *block.vtx[i];
@@ -190,39 +190,41 @@ static bool extractSignals(const CBlock& block, const CBlockIndex* const pindex,
     return true;
 }
 
-bool CMNHFManager::ProcessBlock(const CBlock& block, const CBlockIndex* const pindex, bool fJustCheck, BlockValidationState& state)
+std::optional<CMNHFManager::Signals> CMNHFManager::ProcessBlock(const CBlock& block, const CBlockIndex* const pindex, bool fJustCheck, BlockValidationState& state)
 {
     try {
         std::vector<uint8_t> new_signals;
         if (!extractSignals(block, pindex, new_signals, state)) {
             // state is set inside extractSignals
-            return false;
+            return std::nullopt;
         }
         Signals signals = GetSignalsStage(pindex->pprev);
         if (new_signals.empty()) {
             if (!fJustCheck) {
                 AddToCache(signals, pindex);
             }
             LogPrint(BCLog::EHF, "CMNHFManager::ProcessBlock: no new signals; number of known signals: %d\n", signals.size());
-            return true;
+            return signals;
         }
 
-        int mined_height = pindex->nHeight;
+        const int mined_height = pindex->nHeight;
 
         // Extra validation of signals to be sure that it can succeed
         for (const auto& versionBit : new_signals) {
             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");
+                state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-mnhf-duplicate");
+                return std::nullopt;
             }
 
             if (!Params().IsValidMNActivation(versionBit, pindex->GetMedianTimePast())) {
-                return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-mnhf-non-mn-fork");
+                state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-mnhf-non-mn-fork");
+                return std::nullopt;
             }
         }
         if (fJustCheck) {
             // We are done, no need actually update any params
-            return true;
+            return signals;
         }
         for (const auto& versionBit : new_signals) {
             if (Params().IsValidMNActivation(versionBit, pindex->GetMedianTimePast())) {
@@ -232,10 +234,11 @@ bool CMNHFManager::ProcessBlock(const CBlock& block, const CBlockIndex* const pi
         }
 
         AddToCache(signals, pindex);
-        return true;
+        return signals;
     } catch (const std::exception& e) {
         LogPrintf("CMNHFManager::ProcessBlock -- failed: %s\n", e.what());
-        return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "failed-proc-mnhf-inblock");
+        state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "failed-proc-mnhf-inblock");
+        return std::nullopt;
     }
 }
 
@@ -251,7 +254,7 @@ bool CMNHFManager::UndoBlock(const CBlock& block, const CBlockIndex* const pinde
         return true;
     }
 
-    const Signals signals = GetFromCache(pindex);
+    const Signals signals = GetForBlock(pindex);
     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());
@@ -261,18 +264,48 @@ bool CMNHFManager::UndoBlock(const CBlock& block, const CBlockIndex* const pinde
     return true;
 }
 
-CMNHFManager::Signals CMNHFManager::GetFromCache(const CBlockIndex* const pindex)
+CMNHFManager::Signals CMNHFManager::GetForBlock(const CBlockIndex* pindex)
 {
     if (pindex == nullptr) return {};
 
+    std::stack<const CBlockIndex *> to_calculate;
+
+    std::optional<CMNHFManager::Signals> signalsTmp;
+    while (!(signalsTmp = GetFromCache(pindex)).has_value()) {
+        to_calculate.push(pindex);
+        pindex = pindex->pprev;
+    }
+
+    const Consensus::Params& consensusParams{Params().GetConsensus()};
+    while (!to_calculate.empty()) {
+        CBlock block;
+        if (!ReadBlockFromDisk(block, pindex, consensusParams)) {
+            throw std::runtime_error("failed-getehfforblock-read");
+        }
+        BlockValidationState state;
+        signalsTmp = ProcessBlock(block, pindex, false, state);
+        if (!signalsTmp.has_value()) {
+            LogPrintf("%s: process block failed due to %s\n", __func__, state.ToString());
+            throw std::runtime_error("failed-getehfforblock-construct");
+        }
+
+        to_calculate.pop();
+    }
+    return *signalsTmp;
+}
+
+std::optional<CMNHFManager::Signals> CMNHFManager::GetFromCache(const CBlockIndex* const pindex)
+{
+    Signals signals{};
+    if (pindex == nullptr) return signals;
+
     // 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 {};
+    if (pindex->phashBlock == nullptr) return signals;
 
 
     const uint256& blockHash = pindex->GetBlockHash();
-    Signals signals{};
     {
         LOCK(cs_cache);
         if (mnhfCache.get(blockHash, signals)) {
@@ -282,15 +315,16 @@ CMNHFManager::Signals CMNHFManager::GetFromCache(const CBlockIndex* const pindex
     {
         LOCK(cs_cache);
         if (ThresholdState::ACTIVE != v20_activation.State(pindex->pprev, Params().GetConsensus(), Consensus::DEPLOYMENT_V20)) {
-            mnhfCache.insert(blockHash, {});
-            return {};
+            mnhfCache.insert(blockHash, signals);
+            return signals;
         }
     }
-    bool ok = m_evoDb.Read(std::make_pair(DB_SIGNALS, blockHash), signals);
-    assert(ok);
-    LOCK(cs_cache);
-    mnhfCache.insert(blockHash, signals);
-    return signals;
+    if (m_evoDb.Read(std::make_pair(DB_SIGNALS, blockHash), signals)) {
+        LOCK(cs_cache);
+        mnhfCache.insert(blockHash, signals);
+        return signals;
+    }
+    return std::nullopt;
 }
 
 void CMNHFManager::AddToCache(const Signals& signals, const CBlockIndex* const pindex)
@@ -310,7 +344,7 @@ void CMNHFManager::AddToCache(const Signals& signals, const CBlockIndex* const p
 
 void CMNHFManager::AddSignal(const CBlockIndex* const pindex, int bit)
 {
-    auto signals = GetFromCache(pindex->pprev);
+    auto signals = GetForBlock(pindex->pprev);
     signals.emplace(bit, pindex->nHeight);
     AddToCache(signals, pindex);
 }

src/evo/mnhftx.h

@@ -22,7 +22,6 @@ class CBlock;
 class CBlockIndex;
 class CEvoDB;
 class TxValidationState;
-extern RecursiveMutex cs_main;
 
 // mnhf signal special transaction
 class MNHFTx
@@ -110,14 +109,17 @@ class CMNHFManager : public AbstractEHFManager
     explicit CMNHFManager(const CMNHFManager&) = delete;
 
     /**
-     * Every new block should be processed when Tip() is updated by calling of CMNHFManager::ProcessBlock
+     * Every new block should be processed when Tip() is updated by calling of CMNHFManager::ProcessBlock.
+     * This function actually does only validate EHF transaction for this block and update internal caches/evodb state
      */
-    bool ProcessBlock(const CBlock& block, const CBlockIndex* const pindex, bool fJustCheck, BlockValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    std::optional<Signals> ProcessBlock(const CBlock& block, const CBlockIndex* const pindex, bool fJustCheck, BlockValidationState& state);
 
     /**
      * Every undo block should be processed when Tip() is updated by calling of CMNHFManager::UndoBlock
+     * This function actually does nothing at the moment, because status of ancester block is already know.
+     * Altough it should be still called to do some sanity checks
      */
-    bool UndoBlock(const CBlock& block, const CBlockIndex* const pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+    bool UndoBlock(const CBlock& block, const CBlockIndex* const pindex);
 
 
     // Implements interface
@@ -132,13 +134,20 @@ class CMNHFManager : public AbstractEHFManager
 
     /**
      * This function returns list of signals available on previous block.
+     * if the signals for previous block is not available in cache it would read blocks from disk
+     * until state won't be recovered.
      * NOTE: that some signals could expired between blocks.
-     * validate them by
      */
-    Signals GetFromCache(const CBlockIndex* const pindex);
+    Signals GetForBlock(const CBlockIndex* const pindex);
+
+    /**
+     * This function access to in-memory cache or to evo db but does not calculate anything
+     * NOTE: that some signals could expired between blocks.
+     */
+    std::optional<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);
+bool CheckMNHFTx(const CTransaction& tx, const CBlockIndex* pindexPrev, TxValidationState& state);
 
 #endif // BITCOIN_EVO_MNHFTX_H