Merge dashpay#6030: refactor: remove llmq::CQuorum{BlockProcessor, Manager} globals, move to LLMQContext

d7c35d0 refactor: remove `llmq::CQuorumManager` global, move to `LLMQContext` (Kittywhiskers Van Gogh)
5b86df6 refactor: reduce `llmq::CQuorumManager` globals use, use args (Kittywhiskers Van Gogh)
1efd219 refactor: remove `llmq::CQuorumBlockProcessor` global, move to `LLMQContext` (Kittywhiskers Van Gogh)
1d3afe7 evo: use `gsl::not_null` in `CTxMemPool::ConnectManagers` (Kittywhiskers Van Gogh)
805537e evo: add `CMNHFManager::`(`Dis`)`connectManagers` to remove global use (Kittywhiskers Van Gogh)
cbb6828 refactor: access `llmq::CQuorumManager` through arg in `MNHFTx`, functions (Kittywhiskers Van Gogh)

Pull request description:

  ## Breaking Changes

  None expected.

  ## Checklist
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)**
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  knst:
    utACK d7c35d0
  knst:
    re-utACK dashpay@d7c35d0
  PastaPastaPasta:
    utACK d7c35d0

Tree-SHA512: 07e7494bc061f85259a3a6cfb13d052403419cdc94fc51904ac03468bbdbd1ed74429546309d2bfc6a31ba184b93dc32842e49d2960819d19313bcb7e403008e

Author: PastaPastaPasta

src/evo/assetlocktx.cpp

@@ -22,13 +22,13 @@
 /**
  *  Common code for Asset Lock and Asset Unlock
  */
-bool CheckAssetLockUnlockTx(const CTransaction& tx, gsl::not_null<const CBlockIndex*> pindexPrev, const std::optional<CRangesSet>& indexes, TxValidationState& state)
+bool CheckAssetLockUnlockTx(const llmq::CQuorumManager& qman, const CTransaction& tx, gsl::not_null<const CBlockIndex*> pindexPrev, const std::optional<CRangesSet>& indexes, TxValidationState& state)
 {
     switch (tx.nType) {
     case TRANSACTION_ASSET_LOCK:
         return CheckAssetLockTx(tx, state);
     case TRANSACTION_ASSET_UNLOCK:
-        return CheckAssetUnlockTx(tx, pindexPrev, indexes, state);
+        return CheckAssetUnlockTx(qman, tx, pindexPrev, indexes, state);
     default:
         return state.Invalid(TxValidationResult::TX_BAD_SPECIAL, "bad-not-asset-locks-at-all");
     }
@@ -107,7 +107,7 @@ std::string CAssetLockPayload::ToString() const
 
 const std::string ASSETUNLOCK_REQUESTID_PREFIX = "plwdtx";
 
-bool CAssetUnlockPayload::VerifySig(const uint256& msgHash, gsl::not_null<const CBlockIndex*> pindexTip, TxValidationState& state) const
+bool CAssetUnlockPayload::VerifySig(const llmq::CQuorumManager& qman, const uint256& msgHash, gsl::not_null<const CBlockIndex*> pindexTip, TxValidationState& state) const
 {
     // That quourm hash must be active at `requestHeight`,
     // and at the quorumHash must be active in either the current or previous quorum cycle
@@ -116,7 +116,7 @@ bool CAssetUnlockPayload::VerifySig(const uint256& msgHash, gsl::not_null<const
     Consensus::LLMQType llmqType = Params().GetConsensus().llmqTypePlatform;
 
     // We check at most 2 quorums
-    const auto quorums = llmq::quorumManager->ScanQuorums(llmqType, pindexTip, 2);
+    const auto quorums = qman.ScanQuorums(llmqType, pindexTip, 2);
 
     if (bool isActive = std::any_of(quorums.begin(), quorums.end(), [&](const auto &q) { return q->qc->quorumHash == quorumHash; }); !isActive) {
         return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-assetunlock-not-active-quorum");
@@ -128,7 +128,7 @@ bool CAssetUnlockPayload::VerifySig(const uint256& msgHash, gsl::not_null<const
         return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-assetunlock-too-late");
     }
 
-    const auto quorum = llmq::quorumManager->GetQuorum(llmqType, quorumHash);
+    const auto quorum = qman.GetQuorum(llmqType, quorumHash);
     assert(quorum);
 
     const uint256 requestId = ::SerializeHash(std::make_pair(ASSETUNLOCK_REQUESTID_PREFIX, index));
@@ -141,7 +141,7 @@ bool CAssetUnlockPayload::VerifySig(const uint256& msgHash, gsl::not_null<const
     return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-assetunlock-not-verified");
 }
 
-bool CheckAssetUnlockTx(const CTransaction& tx, gsl::not_null<const CBlockIndex*> pindexPrev, const std::optional<CRangesSet>& indexes, TxValidationState& state)
+bool CheckAssetUnlockTx(const llmq::CQuorumManager& qman, const CTransaction& tx, gsl::not_null<const CBlockIndex*> pindexPrev, const std::optional<CRangesSet>& indexes, TxValidationState& state)
 {
     // Some checks depends from blockchain status also, such as `known indexes` and `withdrawal limits`
     // They are omitted here and done by CCreditPool
@@ -182,7 +182,7 @@ bool CheckAssetUnlockTx(const CTransaction& tx, gsl::not_null<const CBlockIndex*
 
     uint256 msgHash = tx_copy.GetHash();
 
-    return assetUnlockTx.VerifySig(msgHash, pindexPrev, state);
+    return assetUnlockTx.VerifySig(qman, msgHash, pindexPrev, state);
 }
 
 bool GetAssetUnlockFee(const CTransaction& tx, CAmount& txfee, TxValidationState& state)

src/evo/assetlocktx.h

@@ -17,6 +17,9 @@
 class CBlockIndex;
 class CRangesSet;
 class TxValidationState;
+namespace llmq {
+class CQuorumManager;
+} // namespace llmq
 
 class CAssetLockPayload
 {
@@ -127,7 +130,7 @@ class CAssetUnlockPayload
         return obj;
     }
 
-    bool VerifySig(const uint256& msgHash, gsl::not_null<const CBlockIndex*> pindexTip, TxValidationState& state) const;
+    bool VerifySig(const llmq::CQuorumManager& qman, const uint256& msgHash, gsl::not_null<const CBlockIndex*> pindexTip, TxValidationState& state) const;
 
     // getters
     uint8_t getVersion() const
@@ -169,8 +172,8 @@ class CAssetUnlockPayload
 };
 
 bool CheckAssetLockTx(const CTransaction& tx, TxValidationState& state);
-bool CheckAssetUnlockTx(const CTransaction& tx, gsl::not_null<const CBlockIndex*> pindexPrev, const std::optional<CRangesSet>& indexes, TxValidationState& state);
-bool CheckAssetLockUnlockTx(const CTransaction& tx, gsl::not_null<const CBlockIndex*> pindexPrev, const std::optional<CRangesSet>& indexes, TxValidationState& state);
+bool CheckAssetUnlockTx(const llmq::CQuorumManager& qman, const CTransaction& tx, gsl::not_null<const CBlockIndex*> pindexPrev, const std::optional<CRangesSet>& indexes, TxValidationState& state);
+bool CheckAssetLockUnlockTx(const llmq::CQuorumManager& qman, const CTransaction& tx, gsl::not_null<const CBlockIndex*> pindexPrev, const std::optional<CRangesSet>& indexes, TxValidationState& state);
 bool GetAssetUnlockFee(const CTransaction& tx, CAmount& txfee, TxValidationState& state);
 
 #endif // BITCOIN_EVO_ASSETLOCKTX_H

src/evo/chainhelper.cpp

@@ -10,9 +10,10 @@
 
 CChainstateHelper::CChainstateHelper(CCreditPoolManager& cpoolman, CDeterministicMNManager& dmnman, CMNHFManager& mnhfman, CGovernanceManager& govman,
                                      llmq::CQuorumBlockProcessor& qblockman, const Consensus::Params& consensus_params,
-                                     const CMasternodeSync& mn_sync, const CSporkManager& sporkman, const llmq::CChainLocksHandler& clhandler)
+                                     const CMasternodeSync& mn_sync, const CSporkManager& sporkman, const llmq::CChainLocksHandler& clhandler,
+                                     const llmq::CQuorumManager& qman)
     : mn_payments{std::make_unique<CMNPaymentsProcessor>(dmnman, govman, consensus_params, mn_sync, sporkman)},
-      special_tx{std::make_unique<CSpecialTxProcessor>(cpoolman, dmnman, mnhfman, qblockman, consensus_params, clhandler)}
+      special_tx{std::make_unique<CSpecialTxProcessor>(cpoolman, dmnman, mnhfman, qblockman, consensus_params, clhandler, qman)}
 {}
 
 CChainstateHelper::~CChainstateHelper() = default;

src/evo/chainhelper.h

@@ -20,14 +20,16 @@ namespace Consensus { struct Params; }
 namespace llmq {
 class CChainLocksHandler;
 class CQuorumBlockProcessor;
+class CQuorumManager;
 }
 
 class CChainstateHelper
 {
 public:
     explicit CChainstateHelper(CCreditPoolManager& cpoolman, CDeterministicMNManager& dmnman, CMNHFManager& mnhfman, CGovernanceManager& govman,
                                llmq::CQuorumBlockProcessor& qblockman, const Consensus::Params& consensus_params,
-                               const CMasternodeSync& mn_sync, const CSporkManager& sporkman, const llmq::CChainLocksHandler& clhandler);
+                               const CMasternodeSync& mn_sync, const CSporkManager& sporkman, const llmq::CChainLocksHandler& clhandler,
+                               const llmq::CQuorumManager& qman);
     ~CChainstateHelper();
 
     CChainstateHelper() = delete;

src/evo/creditpool.cpp

@@ -267,12 +267,12 @@ bool CCreditPoolDiff::Unlock(const CTransaction& tx, TxValidationState& state)
     return true;
 }
 
-bool CCreditPoolDiff::ProcessLockUnlockTransaction(const CTransaction& tx, TxValidationState& state)
+bool CCreditPoolDiff::ProcessLockUnlockTransaction(const llmq::CQuorumManager& qman, const CTransaction& tx, TxValidationState& state)
 {
     if (!tx.IsSpecialTxVersion()) return true;
     if (tx.nType != TRANSACTION_ASSET_LOCK && tx.nType != TRANSACTION_ASSET_UNLOCK) return true;
 
-    if (!CheckAssetLockUnlockTx(tx, pindexPrev, pool.indexes, state)) {
+    if (!CheckAssetLockUnlockTx(qman, tx, pindexPrev, pool.indexes, state)) {
         // pass the state returned by the function above
         return false;
     }
@@ -292,7 +292,7 @@ bool CCreditPoolDiff::ProcessLockUnlockTransaction(const CTransaction& tx, TxVal
     }
 }
 
-std::optional<CCreditPoolDiff> GetCreditPoolDiffForBlock(CCreditPoolManager& cpoolman, const CBlock& block, const CBlockIndex* pindexPrev,
+std::optional<CCreditPoolDiff> GetCreditPoolDiffForBlock(CCreditPoolManager& cpoolman, const llmq::CQuorumManager& qman, const CBlock& block, const CBlockIndex* pindexPrev,
                                                          const Consensus::Params& consensusParams, const CAmount blockSubsidy, BlockValidationState& state)
 {
     try {
@@ -302,7 +302,7 @@ std::optional<CCreditPoolDiff> GetCreditPoolDiffForBlock(CCreditPoolManager& cpo
         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)) {
+            if (!creditPoolDiff.ProcessLockUnlockTransaction(qman, 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()));

src/evo/creditpool.h

@@ -23,11 +23,12 @@
 class CBlockIndex;
 class BlockValidationState;
 class TxValidationState;
-
-namespace Consensus
-{
-    struct Params;
-}
+namespace Consensus {
+struct Params;
+} // namespace Consensus
+namespace llmq {
+class CQuorumManager;
+} // namespace llmq
 
 struct CCreditPool {
     CAmount locked{0};
@@ -82,7 +83,7 @@ class CCreditPoolDiff {
      * to change amount of credit pool
      * @return true if transaction can be included in this block
      */
-    bool ProcessLockUnlockTransaction(const CTransaction& tx, TxValidationState& state);
+    bool ProcessLockUnlockTransaction(const llmq::CQuorumManager& qman, const CTransaction& tx, TxValidationState& state);
 
     /**
      * this function returns total amount of credits for the next block
@@ -134,7 +135,7 @@ class CCreditPoolManager
     CCreditPool ConstructCreditPool(const CBlockIndex* block_index, CCreditPool prev, const Consensus::Params& consensusParams);
 };
 
-std::optional<CCreditPoolDiff> GetCreditPoolDiffForBlock(CCreditPoolManager& cpoolman, const CBlock& block, const CBlockIndex* pindexPrev,
+std::optional<CCreditPoolDiff> GetCreditPoolDiffForBlock(CCreditPoolManager& cpoolman, const llmq::CQuorumManager& qman, const CBlock& block, const CBlockIndex* pindexPrev,
                                                          const Consensus::Params& consensusParams, const CAmount blockSubsidy, BlockValidationState& state);
 
 #endif

src/evo/mnhftx.cpp

@@ -84,14 +84,14 @@ CMNHFManager::Signals CMNHFManager::GetSignalsStage(const CBlockIndex* const pin
     return signals;
 }
 
-bool MNHFTx::Verify(const uint256& quorumHash, const uint256& requestId, const uint256& msgHash, TxValidationState& state) const
+bool MNHFTx::Verify(const llmq::CQuorumManager& qman, const uint256& quorumHash, const uint256& requestId, const uint256& msgHash, TxValidationState& state) const
 {
     if (versionBit >= VERSIONBITS_NUM_BITS) {
         return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-mnhf-nbit-out-of-bounds");
     }
 
     const Consensus::LLMQType& llmqType = Params().GetConsensus().llmqTypeMnhf;
-    const auto quorum = llmq::quorumManager->GetQuorum(llmqType, quorumHash);
+    const auto quorum = qman.GetQuorum(llmqType, quorumHash);
 
     const uint256 signHash = llmq::BuildSignHash(llmqType, quorum->qc->quorumHash, requestId, msgHash);
     if (!sig.VerifyInsecure(quorum->qc->quorumPublicKey, signHash)) {
@@ -101,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)
+bool CheckMNHFTx(const llmq::CQuorumManager& qman, const CTransaction& tx, const CBlockIndex* pindexPrev, TxValidationState& state)
 {
     if (!tx.IsSpecialTxVersion() || tx.nType != TRANSACTION_MNHF_SIGNAL) {
         return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-mnhf-type");
@@ -134,7 +134,7 @@ bool CheckMNHFTx(const CTransaction& tx, const CBlockIndex* pindexPrev, TxValida
     uint256 msgHash = tx_copy.GetHash();
 
 
-    if (!mnhfTx.signal.Verify(mnhfTx.signal.quorumHash, mnhfTx.GetRequestId(), msgHash, state)) {
+    if (!mnhfTx.signal.Verify(qman, mnhfTx.signal.quorumHash, mnhfTx.GetRequestId(), msgHash, state)) {
         // set up inside Verify
         return false;
     }
@@ -160,7 +160,7 @@ std::optional<uint8_t> extractEHFSignal(const CTransaction& tx)
     return opt_mnhfTx->signal.versionBit;
 }
 
-static bool extractSignals(const CBlock& block, const CBlockIndex* const pindex, std::vector<uint8_t>& new_signals, BlockValidationState& state)
+static bool extractSignals(const llmq::CQuorumManager& qman, const CBlock& block, const CBlockIndex* const pindex, std::vector<uint8_t>& new_signals, BlockValidationState& state)
 {
     // we skip the coinbase
     for (size_t i = 1; i < block.vtx.size(); ++i) {
@@ -172,7 +172,7 @@ static bool extractSignals(const CBlock& block, const CBlockIndex* const pindex,
         }
 
         TxValidationState tx_state;
-        if (!CheckMNHFTx(tx, pindex, tx_state)) {
+        if (!CheckMNHFTx(qman, tx, pindex, tx_state)) {
             return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, tx_state.GetRejectReason(), tx_state.GetDebugMessage());
         }
 
@@ -192,9 +192,11 @@ static bool extractSignals(const CBlock& block, const CBlockIndex* const pindex,
 
 std::optional<CMNHFManager::Signals> CMNHFManager::ProcessBlock(const CBlock& block, const CBlockIndex* const pindex, bool fJustCheck, BlockValidationState& state)
 {
+    assert(m_qman);
+
     try {
         std::vector<uint8_t> new_signals;
-        if (!extractSignals(block, pindex, new_signals, state)) {
+        if (!extractSignals(*m_qman, block, pindex, new_signals, state)) {
             // state is set inside extractSignals
             return std::nullopt;
         }
@@ -244,9 +246,11 @@ std::optional<CMNHFManager::Signals> CMNHFManager::ProcessBlock(const CBlock& bl
 
 bool CMNHFManager::UndoBlock(const CBlock& block, const CBlockIndex* const pindex)
 {
+    assert(m_qman);
+
     std::vector<uint8_t> excluded_signals;
     BlockValidationState state;
-    if (!extractSignals(block, pindex, excluded_signals, state)) {
+    if (!extractSignals(*m_qman, block, pindex, excluded_signals, state)) {
         LogPrintf("CMNHFManager::%s: failed to extract signals\n", __func__);
         return false;
     }
@@ -350,6 +354,13 @@ void CMNHFManager::AddSignal(const CBlockIndex* const pindex, int bit)
     AddToCache(signals, pindex);
 }
 
+void CMNHFManager::ConnectManagers(gsl::not_null<llmq::CQuorumManager*> qman)
+{
+    // Do not allow double-initialization
+    assert(m_qman == nullptr);
+    m_qman = qman;
+}
+
 std::string MNHFTx::ToString() const
 {
     return strprintf("MNHFTx(versionBit=%d, quorumHash=%s, sig=%s)",

src/evo/mnhftx.h

@@ -6,6 +6,7 @@
 #define BITCOIN_EVO_MNHFTX_H
 
 #include <bls/bls.h>
+#include <gsl/pointers.h>
 #include <primitives/transaction.h>
 #include <sync.h>
 #include <threadsafety.h>
@@ -22,6 +23,9 @@ class CBlock;
 class CBlockIndex;
 class CEvoDB;
 class TxValidationState;
+namespace llmq {
+class CQuorumManager;
+}
 
 // mnhf signal special transaction
 class MNHFTx
@@ -32,7 +36,8 @@ class MNHFTx
     CBLSSignature sig{};
 
     MNHFTx() = default;
-    bool Verify(const uint256& quorumHash, const uint256& requestId, const uint256& msgHash, TxValidationState& state) const;
+    bool Verify(const llmq::CQuorumManager& qman, const uint256& quorumHash, const uint256& requestId, const uint256& msgHash,
+                TxValidationState& state) const;
 
     SERIALIZE_METHODS(MNHFTx, obj)
     {
@@ -95,6 +100,7 @@ class CMNHFManager : public AbstractEHFManager
 {
 private:
     CEvoDB& m_evoDb;
+    llmq::CQuorumManager* m_qman{nullptr};
 
     static constexpr size_t MNHFCacheSize = 1000;
     Mutex cs_cache;
@@ -111,24 +117,45 @@ class CMNHFManager : public AbstractEHFManager
     /**
      * 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
+     *
+     * @pre Caller must ensure that LLMQContext has been initialized and the llmq::CQuorumManager pointer has been
+     *      set by calling ConnectManagers() for this CMNHFManager instance
      */
     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 ancestor block is already know.
+     * This function actually does nothing at the moment, because status of ancestor block is already known.
      * Although it should be still called to do some sanity checks
+     *
+     * @pre Caller must ensure that LLMQContext has been initialized and the llmq::CQuorumManager pointer has been
+     *      set by calling ConnectManagers() for this CMNHFManager instance
      */
     bool UndoBlock(const CBlock& block, const CBlockIndex* const pindex);
 
-
     // Implements interface
     Signals GetSignalsStage(const CBlockIndex* const pindexPrev) override;
 
     /**
      * Helper that used in Unit Test to forcely setup EHF signal for specific block
      */
     void AddSignal(const CBlockIndex* const pindex, int bit) EXCLUSIVE_LOCKS_REQUIRED(!cs_cache);
+
+    /**
+     * Set llmq::CQuorumManager pointer.
+     *
+     * Separated from constructor to allow LLMQContext to use CMNHFManager in read-only capacity.
+     * Required to mutate state.
+     */
+    void ConnectManagers(gsl::not_null<llmq::CQuorumManager*> qman);
+
+    /**
+     * Reset llmq::CQuorumManager pointer.
+     *
+     * @pre Must be called before LLMQContext (containing llmq::CQuorumManager) is destroyed.
+     */
+    void DisconnectManagers() { m_qman = nullptr; };
+
 private:
     void AddToCache(const Signals& signals, const CBlockIndex* const pindex);
 
@@ -148,6 +175,6 @@ class CMNHFManager : public AbstractEHFManager
 };
 
 std::optional<uint8_t> extractEHFSignal(const CTransaction& tx);
-bool CheckMNHFTx(const CTransaction& tx, const CBlockIndex* pindexPrev, TxValidationState& state);
+bool CheckMNHFTx(const llmq::CQuorumManager& qman, const CTransaction& tx, const CBlockIndex* pindexPrev, TxValidationState& state);
 
 #endif // BITCOIN_EVO_MNHFTX_H