fix: chain halt if some invalid asset lock transactions are in mempool (#5648)

## Issue being fixed or feature implemented
As discovered during platform testing by @shumkov , it seems as the
chain can halt in miner if somehow mempool would have several
transactions that are somehow invalid (maybe too low fee or something
else). They can't be mined, but miner can't prepare a valid block with
correct Credit Pool amount.

It is indeed can happen although I haven't reproduced it with functional
tests at the moment 🤷‍♂️

## What was done?
Refactored and simplified a logic of Credit Pool amount of validation
and added one more layer of validation: after all transaction are
actually added to block by miner, it is recalculated one more time.
Also used correct `pindexPrev` instead Tip() for EHF signals.

## How Has This Been Tested?
Before this changes platform failed with this error and chain halt:
```
2023-10-20T06:20:16Z (mocktime: 2023-10-20T06:28:29Z) ERROR: ConnectBlock(DASH): CheckCreditPoolDiffForBlock for block 9d635e1fd0d7a8a5bf16ce158d3a39cbf903864bb6d671769836ea7db6055230 failed with bad-cbtx-asse locked-amount
```
With changes from this PR platform is generate the asset-lock
transactions that are included to block and chain is not halt:
```
2023-10-27T10:45:37Z (mocktime: 2023-10-27T14:37:22Z) GetCreditPoolDiffForBlock: CCreditPool is CCreditPool(locked=32100015, currentLimit=32100015)
```

unit/functional tests are succeed.


## Breaking Changes
N/A; no 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
- [ ] 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>

src/evo/creditpool.cpp

@@ -215,37 +215,17 @@ CCreditPoolManager::CCreditPoolManager(CEvoDB& _evoDb)
 {
 }
 
-CCreditPoolDiff::CCreditPoolDiff(CCreditPool starter, const CBlockIndex *pindex, const Consensus::Params& consensusParams) :
+CCreditPoolDiff::CCreditPoolDiff(CCreditPool starter, const CBlockIndex *pindex, const Consensus::Params& consensusParams, const CAmount blockSubsidy) :
     pool(std::move(starter)),
     pindex(pindex),
     params(consensusParams)
 {
     assert(pindex);
-}
 
-void CCreditPoolDiff::AddRewardRealloced(const CAmount reward) {
+    if (llmq::utils::IsMNRewardReallocationActive(pindex)) {
-    assert(MoneyRange(reward));
-    platformReward += reward;
-}
-
-bool CCreditPoolDiff::SetTarget(const CTransaction& tx, const CAmount blockSubsidy, TxValidationState& state)
-{
-    CCbTx cbTx;
-    if (!GetTxPayload(tx, cbTx)) {
-        return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-cbtx-payload");
-    }
-
-    if (cbTx.nVersion != 3) return true;
-
-    targetBalance = cbTx.creditPoolBalance;
-
-    if (!llmq::utils::IsMNRewardReallocationActive(pindex)) return true;
         // We consider V20 active if mn_rr is active
-
+        platformReward = MasternodePayments::PlatformShare(GetMasternodePayment(pindex->nHeight, blockSubsidy, /*fV20Active=*/ true));
-    platformReward = MasternodePayments::PlatformShare(GetMasternodePayment(cbTx.nHeight, blockSubsidy, /* v20_active= */ true));
+    }
-    LogPrintf("CreditPool: set target to %lld with MN reward %lld\n", *targetBalance, platformReward);
-
-    return true;
 }
 
 bool CCreditPoolDiff::Lock(const CTransaction& tx, TxValidationState& state)
@@ -288,15 +268,6 @@ bool CCreditPoolDiff::Unlock(const CTransaction& tx, TxValidationState& state)
     return true;
 }
 
-bool CCreditPoolDiff::ProcessCoinbaseTransaction(const CTransaction& tx, const CAmount blockSubsidy, TxValidationState& state)
-{
-    if (tx.nVersion != 3) return true;
-
-    assert(tx.nType == TRANSACTION_COINBASE);
-
-    return SetTarget(tx, blockSubsidy, state);
-}
-
 bool CCreditPoolDiff::ProcessLockUnlockTransaction(const CTransaction& tx, TxValidationState& state)
 {
     if (tx.nVersion != 3) return true;
@@ -321,3 +292,28 @@ bool CCreditPoolDiff::ProcessLockUnlockTransaction(const CTransaction& tx, TxVal
         return state.Invalid(TxValidationResult::TX_CONSENSUS, "failed-procassetlocksinblock");
     }
 }
+
+std::optional<CCreditPoolDiff> GetCreditPoolDiffForBlock(const CBlock& block, const CBlockIndex* pindexPrev, const Consensus::Params& consensusParams,
+                                                         const CAmount blockSubsidy, BlockValidationState& state)
+{
+    try {
+        const CCreditPool creditPool = creditPoolManager->GetCreditPool(pindexPrev, consensusParams);
+        LogPrintf("%s: CCreditPool is %s\n", __func__, creditPool.ToString());
+        CCreditPoolDiff creditPoolDiff(creditPool, pindexPrev, consensusParams, blockSubsidy);
+        for (size_t i = 1; i < block.vtx.size(); ++i) {
+            const auto& tx = *block.vtx[i];
+            TxValidationState tx_state;
+            if (!creditPoolDiff.ProcessLockUnlockTransaction(tx, tx_state)) {
+                assert(tx_state.GetResult() == TxValidationResult::TX_CONSENSUS);
+                state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, tx_state.GetRejectReason(),
+                                 strprintf("Process Lock/Unlock Transaction failed at Credit Pool (tx hash %s) %s", tx.GetHash().ToString(), tx_state.GetDebugMessage()));
+                return std::nullopt;
+            }
+        }
+        return creditPoolDiff;
+    } catch (const std::exception& e) {
+        LogPrintf("%s -- failed: %s\n", __func__, e.what());
+        state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "failed-getcreditpooldiff");
+        return std::nullopt;
+    }
+}

src/evo/creditpool.h

@@ -69,20 +69,12 @@ private:
     CAmount sessionUnlocked{0};
     CAmount platformReward{0};
 
-    // target value is used to validate CbTx. If values mismatched, block is invalid
-    std::optional<CAmount> targetBalance;
-
     const CBlockIndex *pindex{nullptr};
     const Consensus::Params& params;
 public:
-    explicit CCreditPoolDiff(CCreditPool starter, const CBlockIndex *pindex, const Consensus::Params& consensusParams);
+    explicit CCreditPoolDiff(CCreditPool starter, const CBlockIndex *pindex,
-
+                             const Consensus::Params& consensusParams,
-    /**
+                             const CAmount blockSubsidy);
-     * This function should be called for each block's coinbase transaction
-     * to change amount of credit pool
-     * coinbase transaction's Payload must be valid if nVersion of coinbase transaction equals 3
-     */
-    bool ProcessCoinbaseTransaction(const CTransaction& tx, const CAmount blockSubsidy, TxValidationState& state);
 
     /**
      * This function should be called for each Asset Lock/Unlock tx
@@ -92,24 +84,17 @@ public:
     bool ProcessLockUnlockTransaction(const CTransaction& tx, TxValidationState& state);
 
     /**
-     * This function should be called by miner for initialization of MasterNode reward
+     * this function returns total amount of credits for the next block
      */
-    void AddRewardRealloced(const CAmount reward);
-
     CAmount GetTotalLocked() const {
         return pool.locked + sessionLocked - sessionUnlocked + platformReward;
     }
 
-    const std::optional<CAmount>& GetTargetBalance() const {
-        return targetBalance;
-    }
-
     std::string ToString() const {
-        return strprintf("CCreditPoolDiff(target=%lld, sessionLocked=%lld, sessionUnlocked=%lld, newIndexes=%lld, pool=%s)", GetTargetBalance() ? *GetTargetBalance() : -1, sessionLocked, sessionUnlocked, newIndexes.size(), pool.ToString());
+        return strprintf("CCreditPoolDiff(sessionLocked=%lld, sessionUnlocked=%lld, platforomReward=%lld, newIndexes=%lld, pool=%s)", sessionLocked, sessionUnlocked, platformReward, newIndexes.size(), pool.ToString());
     }
 
 private:
-    bool SetTarget(const CTransaction& tx, const CAmount blockSubsidy, TxValidationState& state);
     bool Lock(const CTransaction& tx, TxValidationState& state);
     bool Unlock(const CTransaction& tx, TxValidationState& state);
 };
@@ -148,6 +133,9 @@ private:
     CCreditPool ConstructCreditPool(const CBlockIndex* block_index, CCreditPool prev, const Consensus::Params& consensusParams);
 };
 
+std::optional<CCreditPoolDiff> GetCreditPoolDiffForBlock(const CBlock& block, const CBlockIndex* pindexPrev, const Consensus::Params& consensusParams,
+                                                         const CAmount blockSubsidy, BlockValidationState& state);
+
 extern std::unique_ptr<CCreditPoolManager> creditPoolManager;
 
 #endif

src/evo/specialtxman.cpp

@@ -275,38 +275,27 @@ bool UndoSpecialTxsInBlock(const CBlock& block, const CBlockIndex* pindex, CMNHF
 bool CheckCreditPoolDiffForBlock(const CBlock& block, const CBlockIndex* pindex, const Consensus::Params& consensusParams,
                                 const CAmount blockSubsidy, BlockValidationState& state)
 {
-    AssertLockHeld(cs_main);
-
     try {
+        if (!llmq::utils::IsV20Active(pindex)) return true;
 
-        if (!llmq::utils::IsV20Active(pindex->pprev)) return true;
+        auto creditPoolDiff = GetCreditPoolDiffForBlock(block, pindex->pprev, consensusParams, blockSubsidy, state);
+        if (!creditPoolDiff.has_value()) return false;
 
-        const CCreditPool creditPool = creditPoolManager->GetCreditPool(pindex->pprev, consensusParams);
+        // If we get there we have v20 activated and credit pool amount must be included in block CbTx
-        LogPrintf("%s: CCreditPool is %s\n", __func__, creditPool.ToString());
+        const auto& tx = *block.vtx[0];
-        CCreditPoolDiff creditPoolDiff(creditPool, pindex->pprev, consensusParams);
+        assert(tx.IsCoinBase());
+        assert(tx.nVersion == 3);
+        assert(tx.nType == TRANSACTION_COINBASE);
 
-        for (const auto& ptr_tx : block.vtx) {
+        CCbTx cbTx;
-            TxValidationState tx_state;
+        if (!GetTxPayload(tx, cbTx)) {
-            if (ptr_tx->IsCoinBase()) {
+            return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-payload");
-                if (!creditPoolDiff.ProcessCoinbaseTransaction(*ptr_tx, blockSubsidy, tx_state)) {
-                    assert(tx_state.GetResult() == TxValidationResult::TX_CONSENSUS);
-                    return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, tx_state.GetRejectReason(),
-                                     strprintf("Process Coinbase Transaction failed at Credit Pool (tx hash %s) %s", ptr_tx->GetHash().ToString(), tx_state.GetDebugMessage()));
         }
-            } else {
+        CAmount target_balance{cbTx.creditPoolBalance};
-                if (!creditPoolDiff.ProcessLockUnlockTransaction(*ptr_tx, tx_state)) {
+        // But it maybe not included yet in previous block yet; in this case value must be 0
-                    assert(tx_state.GetResult() == TxValidationResult::TX_CONSENSUS);
+        CAmount locked_calculated{creditPoolDiff->GetTotalLocked()};
-                    return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, tx_state.GetRejectReason(),
+        if (target_balance != locked_calculated) {
-                                     strprintf("Process Lock/Unlock Transaction failed at Credit Pool (tx hash %s) %s", ptr_tx->GetHash().ToString(), tx_state.GetDebugMessage()));
+            LogPrintf("%s: mismatched locked amount in CbTx: %lld against re-calculated: %lld\n", __func__, target_balance, locked_calculated);
-                }
-            }
-        }
-        CAmount locked_proposed{0};
-        if (creditPoolDiff.GetTargetBalance()) locked_proposed = *creditPoolDiff.GetTargetBalance();
-
-        CAmount locked_calculated = creditPoolDiff.GetTotalLocked();
-        if (locked_proposed != locked_calculated) {
-            LogPrintf("%s: mismatched locked amount in CbTx: %lld against re-calculated: %lld\n", __func__, locked_proposed, locked_calculated);
             return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-assetlocked-amount");
         }
 

src/evo/specialtxman.h

@@ -34,6 +34,6 @@ bool ProcessSpecialTxsInBlock(const CBlock& block, const CBlockIndex* pindex, CM
 bool UndoSpecialTxsInBlock(const CBlock& block, const CBlockIndex* pindex, CMNHFManager& mnhfManager,
                            llmq::CQuorumBlockProcessor& quorum_block_processor) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 bool CheckCreditPoolDiffForBlock(const CBlock& block, const CBlockIndex* pindex, const Consensus::Params& consensusParams,
-                                const CAmount blockSubsidy, BlockValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+                                const CAmount blockSubsidy, BlockValidationState& state);
 
 #endif // BITCOIN_EVO_SPECIALTXMAN_H

src/miner.cpp

@@ -169,14 +169,7 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
     int nPackagesSelected = 0;
     int nDescendantsUpdated = 0;
 
-    std::optional<CCreditPoolDiff> creditPoolDiff;
+    addPackageTxs(nPackagesSelected, nDescendantsUpdated, pindexPrev);
-    if (fV20Active_context) {
-        CCreditPool creditPool = creditPoolManager->GetCreditPool(pindexPrev, chainparams.GetConsensus());
-        LogPrintf("%s: CCreditPool is %s\n", __func__, creditPool.ToString());
-        creditPoolDiff.emplace(std::move(creditPool), pindexPrev, chainparams.GetConsensus());
-    }
-    std::unordered_map<uint8_t, int> signals = m_chainstate.GetMNHFSignalsStage();
-    addPackageTxs(nPackagesSelected, nDescendantsUpdated, creditPoolDiff, signals);
 
     int64_t nTime1 = GetTimeMicros();
 
@@ -192,7 +185,6 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
     coinbaseTx.vout[0].scriptPubKey = scriptPubKeyIn;
 
     // NOTE: unlike in bitcoin, we need to pass PREVIOUS block height here
-    bool fMNRewardReallocated = llmq::utils::IsMNRewardReallocationActive(pindexPrev);
     CAmount blockSubsidy = GetBlockSubsidyInner(pindexPrev->nBits, pindexPrev->nHeight, Params().GetConsensus(), fV20Active_context);
     CAmount blockReward = blockSubsidy + nFees;
 
@@ -234,14 +226,12 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
                     // not an error
                     LogPrintf("CreateNewBlock() h[%d] CbTx failed to find best CL. Inserting null CL\n", nHeight);
                 }
-                assert(creditPoolDiff != std::nullopt);
+                BlockValidationState state;
-
+                const auto creditPoolDiff = GetCreditPoolDiffForBlock(*pblock, pindexPrev, chainparams.GetConsensus(), blockSubsidy, state);
-                if (fMNRewardReallocated) {
+                if (creditPoolDiff == std::nullopt) {
-                    const CAmount masternodeReward = GetMasternodePayment(nHeight, blockSubsidy, fV20Active_context);
+                    throw std::runtime_error(strprintf("%s: GetCreditPoolDiffForBlock failed: %s", __func__, state.ToString()));
-                    const CAmount reallocedReward = MasternodePayments::PlatformShare(masternodeReward);
-                    LogPrint(BCLog::MNPAYMENTS, "%s: add MN reward %lld (%lld) to credit pool\n", __func__, masternodeReward, reallocedReward);
-                    creditPoolDiff->AddRewardRealloced(reallocedReward);
                 }
+
                 cbTx.creditPoolBalance = creditPoolDiff->GetTotalLocked();
             }
         }
@@ -403,10 +393,23 @@ 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, std::unordered_map<uint8_t, int>& signals)
+void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpdated, const CBlockIndex* const pindexPrev)
 {
+    AssertLockHeld(cs_main); // for GetMNHFSignalsStage()
     AssertLockHeld(m_mempool.cs);
 
+    // This credit pool is used only to check withdrawal limits and to find
+    // duplicates of indexes. There's used `BlockSubsidy` equaled to 0
+    std::optional<CCreditPoolDiff> creditPoolDiff;
+    if (llmq::utils::IsV20Active(pindexPrev)) {
+        CCreditPool creditPool = creditPoolManager->GetCreditPool(pindexPrev, chainparams.GetConsensus());
+        LogPrintf("%s: CCreditPool is %s\n", __func__, creditPool.ToString());
+        creditPoolDiff.emplace(std::move(creditPool), pindexPrev, chainparams.GetConsensus(), 0);
+    }
+
+    // This map with signals is used only to find duplicates
+    std::unordered_map<uint8_t, int> signals = m_chainstate.GetMNHFSignalsStage(pindexPrev);
+
     // mapModifiedTx will store sorted packages after they are modified
     // because some of their txs are already in the block
     indexed_modified_transaction_set mapModifiedTx;

src/miner.h

@@ -20,7 +20,6 @@
 class CBlockIndex;
 class CChainParams;
 class CConnman;
-class CCreditPoolDiff;
 class CGovernanceManager;
 class CScript;
 class CSporkManager;
@@ -195,7 +194,7 @@ private:
       * Increments nPackagesSelected / nDescendantsUpdated with corresponding
       * statistics from the package selection (for logging statistics). */
     void addPackageTxs(int& nPackagesSelected, int& nDescendantsUpdated,
-                       std::optional<CCreditPoolDiff>& creditPoolDiff, std::unordered_map<uint8_t, int>& signals) EXCLUSIVE_LOCKS_REQUIRED(m_mempool.cs);
+                       const CBlockIndex* pindexPrev) EXCLUSIVE_LOCKS_REQUIRED(m_mempool.cs);
 
     // helper functions for addPackageTxs()
     /** Remove confirmed (inBlock) entries from given set */

src/validation.cpp

@@ -2509,11 +2509,9 @@ CoinsCacheSizeState CChainState::GetCoinsCacheSizeState(
     return CoinsCacheSizeState::OK;
 }
 
-std::unordered_map<uint8_t, int> CChainState::GetMNHFSignalsStage()
+std::unordered_map<uint8_t, int> CChainState::GetMNHFSignalsStage(const CBlockIndex* const pindexPrev)
 {
-    const CBlockIndex* const tip = m_chain.Tip();
+    return this->m_mnhfManager.GetSignalsStage(pindexPrev);
-    if (tip == nullptr) return {};
-    return this->m_mnhfManager.GetSignalsStage(tip);
 }
 
 bool CChainState::FlushStateToDisk(

src/validation.h

@@ -778,7 +778,7 @@ public:
         size_t max_mempool_size_bytes) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 
     /** Return list of MN EHF signals for current Tip() */
-    std::unordered_map<uint8_t, int> GetMNHFSignalsStage() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
+    std::unordered_map<uint8_t, int> GetMNHFSignalsStage(const CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 
     std::string ToString() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
 private: