Merge dashpay#6730: refactor: break multiple circular dependencies over evo/deterministicmns and evo/cbtx

86c3dad fmt: re-order included for specialtxman.h (Konstantin Akimov)
8f40f40 refactor: remove un-needed header validationinterface.h from evo/{dmnstate,deterministicmns} (Konstantin Akimov)
51a83e7 fix: clang formatting and adjusting logs for BuildNewListFromBlock (Konstantin Akimov)
8e8d6eb refactor: remove dependency of evo/deterministicmns on llmq/utils and llmq/commitment (Konstantin Akimov)
a20e1a0 refactor: drop unused arguments from CDeterministicMNManager::ProcessBlock (Konstantin Akimov)
c873e55 refactor: move CL code validation to break circular dependencies over CbTx (Konstantin Akimov)
1011f6b refactor: drop dependency of CbTx on CSimplifiedMNList (Konstantin Akimov)
48a0357 refactor: only code move for CalcCbTxMerkleRootMNList from cbtx.h to simplifiedmns.h (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  Addressed some exception for circular dependencies.

  ## What was done?
  See each commit. Most changes here are just moving functions between files, `git diff --color-moved=zebra` may help to review it.

  Though, `GetAllQuorumMembers()` is now called by `BuildNewListFromBlock()` compared to current behavior in develop branch.

  ## How Has This Been Tested?
  Removed these exception from linter:
  ```
  -    "core_io -> evo/cbtx -> evo/simplifiedmns -> core_io",
  -    "evo/cbtx -> evo/simplifiedmns -> evo/cbtx",
  -    "evo/deterministicmns -> llmq/commitment -> evo/deterministicmns",
  -    "evo/deterministicmns -> llmq/commitment -> validation -> evo/deterministicmns",
  -    "evo/deterministicmns -> llmq/commitment -> validation -> txmempool -> evo/deterministicmns",
  -    "evo/deterministicmns -> llmq/utils -> evo/deterministicmns",
  -    "evo/deterministicmns -> llmq/utils -> llmq/snapshot -> evo/simplifiedmns -> evo/deterministicmns",
  -    "evo/deterministicmns -> llmq/utils -> net -> evo/deterministicmns",
  -    "evo/deterministicmns -> validationinterface -> evo/deterministicmns",
  -    "evo/deterministicmns -> validationinterface -> governance/vote -> evo/deterministicmns",
  +    "core_io -> evo/assetlocktx -> llmq/signing -> net_processing -> evo/simplifiedmns -> core_io",
  ```

  New one appeared in the list because it is a longer circular dependency that always has been here but has been hidden by shorter loop: `core_io -> evo/cbtx -> evo/simplifiedmns -> core_io`.

  ## 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
  - [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

ACKs for top commit:
  kwvg:
    utACK 86c3dad
  UdjinM6:
    re-utACK 86c3dad

Tree-SHA512: 990ad21ca7e0fdf7207990bc4d8bad9453a3cdd6e5988941533f5c7e5cc4e6a70a412b92c3c9eba80b76732d25eaefe62c8491a0df6e621cf592de7098f9f50e

Author: PastaPastaPasta

src/evo/cbtx.cpp

@@ -4,10 +4,8 @@
 
 #include <consensus/validation.h>
 #include <evo/cbtx.h>
-#include <evo/simplifiedmns.h>
 #include <evo/specialtx.h>
 #include <llmq/blockprocessor.h>
-#include <llmq/chainlocks.h>
 #include <llmq/commitment.h>
 #include <llmq/options.h>
 #include <llmq/quorums.h>
@@ -45,93 +43,23 @@ bool CheckCbTx(const CCbTx& cbTx, const CBlockIndex* pindexPrev, TxValidationSta
     return true;
 }
 
-// This can only be done after the block has been fully processed, as otherwise we won't have the finished MN list
 bool CheckCbTxMerkleRoots(const CBlock& block, const CCbTx& cbTx, const CBlockIndex* pindex,
-                          const llmq::CQuorumBlockProcessor& quorum_block_processor, CSimplifiedMNList&& sml,
-                          BlockValidationState& state)
+                          const llmq::CQuorumBlockProcessor& quorum_block_processor, BlockValidationState& state)
 {
-    if (pindex) {
-        static int64_t nTimeMerkleMNL = 0;
-        static int64_t nTimeMerkleQuorum = 0;
-
-        int64_t nTime1 = GetTimeMicros();
+    if (pindex && cbTx.nVersion >= CCbTx::Version::MERKLE_ROOT_QUORUMS) {
         uint256 calculatedMerkleRoot;
-        if (!CalcCbTxMerkleRootMNList(calculatedMerkleRoot, std::move(sml), state)) {
+        if (!CalcCbTxMerkleRootQuorums(block, pindex->pprev, quorum_block_processor, calculatedMerkleRoot, state)) {
             // pass the state returned by the function above
             return false;
         }
-        if (calculatedMerkleRoot != cbTx.merkleRootMNList) {
-            return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-mnmerkleroot");
-        }
-
-        int64_t nTime2 = GetTimeMicros();
-        nTimeMerkleMNL += nTime2 - nTime1;
-        LogPrint(BCLog::BENCHMARK, "          - CalcCbTxMerkleRootMNList: %.2fms [%.2fs]\n", 0.001 * (nTime2 - nTime1),
-                 nTimeMerkleMNL * 0.000001);
-
-        if (cbTx.nVersion >= CCbTx::Version::MERKLE_ROOT_QUORUMS) {
-            if (!CalcCbTxMerkleRootQuorums(block, pindex->pprev, quorum_block_processor, calculatedMerkleRoot, state)) {
-                // pass the state returned by the function above
-                return false;
-            }
-            if (calculatedMerkleRoot != cbTx.merkleRootQuorums) {
-                return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-quorummerkleroot");
-            }
+        if (calculatedMerkleRoot != cbTx.merkleRootQuorums) {
+            return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-quorummerkleroot");
         }
-
-        int64_t nTime3 = GetTimeMicros();
-        nTimeMerkleQuorum += nTime3 - nTime2;
-        LogPrint(BCLog::BENCHMARK, "          - CalcCbTxMerkleRootQuorums: %.2fms [%.2fs]\n", 0.001 * (nTime3 - nTime2),
-                 nTimeMerkleQuorum * 0.000001);
     }
 
     return true;
 }
 
-bool CalcCbTxMerkleRootMNList(uint256& merkleRootRet, CSimplifiedMNList&& sml, BlockValidationState& state)
-{
-    try {
-        static std::atomic<int64_t> nTimeMerkle = 0;
-
-        int64_t nTime1 = GetTimeMicros();
-
-        static Mutex cached_mutex;
-        static CSimplifiedMNList smlCached GUARDED_BY(cached_mutex);
-        static uint256 merkleRootCached GUARDED_BY(cached_mutex);
-        static bool mutatedCached GUARDED_BY(cached_mutex) {false};
-
-        LOCK(cached_mutex);
-        if (sml == smlCached) {
-            merkleRootRet = merkleRootCached;
-            if (mutatedCached) {
-                return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "mutated-cached-calc-cb-mnmerkleroot");
-            }
-            return true;
-        }
-
-        bool mutated = false;
-        merkleRootRet = sml.CalcMerkleRoot(&mutated);
-
-        int64_t nTime2 = GetTimeMicros();
-        nTimeMerkle += nTime2 - nTime1;
-        LogPrint(BCLog::BENCHMARK, "            - CalcMerkleRoot: %.2fms [%.2fs]\n", 0.001 * (nTime2 - nTime1),
-                 nTimeMerkle * 0.000001);
-
-        smlCached = std::move(sml);
-        merkleRootCached = merkleRootRet;
-        mutatedCached = mutated;
-
-        if (mutated) {
-            return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "mutated-calc-cb-mnmerkleroot");
-        }
-
-        return true;
-    } catch (const std::exception& e) {
-        LogPrintf("%s -- failed: %s\n", __func__, e.what());
-        return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "failed-calc-cb-mnmerkleroot");
-    }
-}
-
 using QcHashMap = std::map<Consensus::LLMQType, std::vector<uint256>>;
 using QcIndexedHashMap = std::map<Consensus::LLMQType, std::map<int16_t, uint256>>;
 
@@ -288,129 +216,6 @@ bool CalcCbTxMerkleRootQuorums(const CBlock& block, const CBlockIndex* pindexPre
     return true;
 }
 
-bool CheckCbTxBestChainlock(const CCbTx& cbTx, const CBlockIndex* pindex,
-                            const llmq::CChainLocksHandler& chainlock_handler, BlockValidationState& state)
-{
-    if (cbTx.nVersion < CCbTx::Version::CLSIG_AND_BALANCE) {
-        return true;
-    }
-
-    static Mutex cached_mutex;
-    static const CBlockIndex* cached_pindex GUARDED_BY(cached_mutex){nullptr};
-    static std::optional<std::pair<CBLSSignature, uint32_t>> cached_chainlock GUARDED_BY(cached_mutex){std::nullopt};
-
-    auto best_clsig = chainlock_handler.GetBestChainLock();
-    if (best_clsig.getHeight() == pindex->nHeight - 1 && cbTx.bestCLHeightDiff == 0 && cbTx.bestCLSignature == best_clsig.getSig()) {
-        // matches our best clsig which still hold values for the previous block
-        LOCK(cached_mutex);
-        cached_chainlock = std::make_pair(cbTx.bestCLSignature, cbTx.bestCLHeightDiff);
-        cached_pindex = pindex;
-        return true;
-    }
-
-    std::optional<std::pair<CBLSSignature, uint32_t>> prevBlockCoinbaseChainlock{std::nullopt};
-    if (LOCK(cached_mutex); cached_pindex == pindex->pprev) {
-        prevBlockCoinbaseChainlock = cached_chainlock;
-    }
-    if (!prevBlockCoinbaseChainlock.has_value()) {
-        prevBlockCoinbaseChainlock = GetNonNullCoinbaseChainlock(pindex->pprev);
-    }
-    // If std::optional prevBlockCoinbaseChainlock is empty, then up to the previous block, coinbase Chainlock is null.
-    if (prevBlockCoinbaseChainlock.has_value()) {
-        // Previous block Coinbase has a non-null Chainlock: current block's Chainlock must be non-null and at least as new as the previous one
-        if (!cbTx.bestCLSignature.IsValid()) {
-            // IsNull() doesn't exist for CBLSSignature: we assume that a non valid BLS sig is null
-            return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-null-clsig");
-        }
-        if (cbTx.bestCLHeightDiff > prevBlockCoinbaseChainlock.value().second + 1) {
-            return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-older-clsig");
-        }
-    }
-
-    // IsNull() doesn't exist for CBLSSignature: we assume that a valid BLS sig is non-null
-    if (cbTx.bestCLSignature.IsValid()) {
-        int curBlockCoinbaseCLHeight = pindex->nHeight - static_cast<int>(cbTx.bestCLHeightDiff) - 1;
-        if (best_clsig.getHeight() == curBlockCoinbaseCLHeight && best_clsig.getSig() == cbTx.bestCLSignature) {
-            // matches our best (but outdated) clsig, no need to verify it again
-            LOCK(cached_mutex);
-            cached_chainlock = std::make_pair(cbTx.bestCLSignature, cbTx.bestCLHeightDiff);
-            cached_pindex = pindex;
-            return true;
-        }
-        uint256 curBlockCoinbaseCLBlockHash = pindex->GetAncestor(curBlockCoinbaseCLHeight)->GetBlockHash();
-        if (chainlock_handler.VerifyChainLock(llmq::CChainLockSig(curBlockCoinbaseCLHeight, curBlockCoinbaseCLBlockHash, cbTx.bestCLSignature)) != llmq::VerifyRecSigStatus::Valid) {
-            return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-invalid-clsig");
-        }
-        LOCK(cached_mutex);
-        cached_chainlock = std::make_pair(cbTx.bestCLSignature, cbTx.bestCLHeightDiff);
-        cached_pindex = pindex;
-    } else if (cbTx.bestCLHeightDiff != 0) {
-        // Null bestCLSignature is allowed only with bestCLHeightDiff = 0
-        return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cbtx-cldiff");
-    }
-
-    return true;
-}
-
-bool CalcCbTxBestChainlock(const llmq::CChainLocksHandler& chainlock_handler, const CBlockIndex* pindexPrev,
-                           uint32_t& bestCLHeightDiff, CBLSSignature& bestCLSignature)
-{
-    auto best_clsig = chainlock_handler.GetBestChainLock();
-    if (best_clsig.getHeight() < Params().GetConsensus().DeploymentHeight(Consensus::DEPLOYMENT_V19)) {
-        // We don't want legacy BLS ChainLocks in CbTx (can happen on regtest/devenets)
-        best_clsig = llmq::CChainLockSig{};
-    }
-    if (best_clsig.getHeight() == pindexPrev->nHeight) {
-        // Our best CL is the newest one possible
-        bestCLHeightDiff = 0;
-        bestCLSignature = best_clsig.getSig();
-        return true;
-    }
-
-    auto prevBlockCoinbaseChainlock = GetNonNullCoinbaseChainlock(pindexPrev);
-    if (prevBlockCoinbaseChainlock.has_value()) {
-        // Previous block Coinbase contains a non-null CL: We must insert the same sig or a better (newest) one
-        if (best_clsig.IsNull()) {
-            // We don't know any CL, therefore inserting the CL of the previous block
-            bestCLHeightDiff = prevBlockCoinbaseChainlock->second + 1;
-            bestCLSignature = prevBlockCoinbaseChainlock->first;
-            return true;
-        }
-
-        // We check if our best CL is newer than the one from previous block Coinbase
-        int curCLHeight = best_clsig.getHeight();
-        int prevCLHeight = pindexPrev->nHeight - static_cast<int>(prevBlockCoinbaseChainlock->second) - 1;
-        if (curCLHeight < prevCLHeight) {
-            // Our best CL isn't newer: inserting CL from previous block
-            bestCLHeightDiff = prevBlockCoinbaseChainlock->second + 1;
-            bestCLSignature = prevBlockCoinbaseChainlock->first;
-        }
-        else {
-            // Our best CL is newer
-            bestCLHeightDiff = pindexPrev->nHeight - best_clsig.getHeight();
-            bestCLSignature = best_clsig.getSig();
-        }
-
-        return true;
-    }
-    else {
-        // Previous block Coinbase has no CL. We can either insert null or any valid CL
-        if (best_clsig.IsNull()) {
-            // We don't know any CL, therefore inserting a null CL
-            bestCLHeightDiff = 0;
-            bestCLSignature.Reset();
-            return false;
-        }
-
-        // Inserting our best CL
-        bestCLHeightDiff = pindexPrev->nHeight - best_clsig.getHeight();
-        bestCLSignature = chainlock_handler.GetBestChainLock().getSig();
-
-        return true;
-    }
-}
-
-
 std::string CCbTx::ToString() const
 {
     return strprintf("CCbTx(nVersion=%d, nHeight=%d, merkleRootMNList=%s, merkleRootQuorums=%s, bestCLHeightDiff=%d, bestCLSig=%s, creditPoolBalance=%d.%08d)",

src/evo/cbtx.h

@@ -15,10 +15,7 @@ class BlockValidationState;
 class CBlock;
 class CBlockIndex;
 class TxValidationState;
-class CSimplifiedMNList;
-
 namespace llmq {
-class CChainLocksHandler;
 class CQuorumBlockProcessor;
 }// namespace llmq
 
@@ -66,19 +63,13 @@ template<> struct is_serializable_enum<CCbTx::Version> : std::true_type {};
 
 bool CheckCbTx(const CCbTx& cbTx, const CBlockIndex* pindexPrev, TxValidationState& state);
 
+// This can only be done after the block has been fully processed, as otherwise we won't have the finished MN list
 bool CheckCbTxMerkleRoots(const CBlock& block, const CCbTx& cbTx, const CBlockIndex* pindex,
-                          const llmq::CQuorumBlockProcessor& quorum_block_processor, CSimplifiedMNList&& sml,
-                          BlockValidationState& state);
-bool CalcCbTxMerkleRootMNList(uint256& merkleRootRet, CSimplifiedMNList&& sml, BlockValidationState& state);
+                          const llmq::CQuorumBlockProcessor& quorum_block_processor, BlockValidationState& state);
 bool CalcCbTxMerkleRootQuorums(const CBlock& block, const CBlockIndex* pindexPrev,
                                const llmq::CQuorumBlockProcessor& quorum_block_processor, uint256& merkleRootRet,
                                BlockValidationState& state);
 
-bool CheckCbTxBestChainlock(const CCbTx& cbTx, const CBlockIndex* pindexPrev,
-                            const llmq::CChainLocksHandler& chainlock_handler, BlockValidationState& state);
-bool CalcCbTxBestChainlock(const llmq::CChainLocksHandler& chainlock_handler, const CBlockIndex* pindexPrev,
-                           uint32_t& bestCLHeightDiff, CBLSSignature& bestCLSignature);
-
 std::optional<std::pair<CBLSSignature, uint32_t>> GetNonNullCoinbaseChainlock(const CBlockIndex* pindex);
 
 #endif // BITCOIN_EVO_CBTX_H