refactor: improved initialization of members of LLMQContext and related changes (dashpay#5150)

LLMQContext uses RAII to initialize all members. Ensured that all
members always initialized correctly in proper order if LLMQContext
exists.

BlockAssembler, CChainState use too many agruments and they are making
wrong assumption that members of LLMQContext can be constructed and used
independently, but that's not true. Instead, let's pass LLMQContext
whenever possible.

## Issue being fixed or feature implemented
dashpay/dash-issues#52

## How Has This Been Tested?
Run unit/functional test and introduce no breaking changes.


## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone

Author: knst

src/llmq/context.cpp

@@ -20,108 +20,85 @@
 #include <masternode/sync.h>
 
 LLMQContext::LLMQContext(CEvoDB& evo_db, CTxMemPool& mempool, CConnman& connman, CSporkManager& sporkman,
-                         const std::unique_ptr<PeerManager>& peerman, bool unit_tests, bool wipe) {
-    Create(evo_db, mempool, connman, sporkman, peerman, unit_tests, wipe);
-
-    /* Context aliases to globals used by the LLMQ system */
-    quorum_block_processor = llmq::quorumBlockProcessor.get();
-    qman = llmq::quorumManager.get();
-    clhandler = llmq::chainLocksHandler.get();
-    isman = llmq::quorumInstantSendManager.get();
-}
-
-LLMQContext::~LLMQContext() {
-    isman = nullptr;
-    clhandler = nullptr;
-    qman = nullptr;
-    quorum_block_processor = nullptr;
-
-    Destroy();
-}
-
-void LLMQContext::Create(CEvoDB& evo_db, CTxMemPool& mempool, CConnman& connman, CSporkManager& sporkman,
-                         const std::unique_ptr<PeerManager>& peerman, bool unit_tests, bool wipe) {
-    bls_worker = std::make_shared<CBLSWorker>();
-
-    dkg_debugman = std::make_unique<llmq::CDKGDebugManager>();
-    llmq::quorumBlockProcessor = std::make_unique<llmq::CQuorumBlockProcessor>(evo_db, connman, peerman);
-    qdkgsman = std::make_unique<llmq::CDKGSessionManager>(connman, *bls_worker, *dkg_debugman, *llmq::quorumBlockProcessor, sporkman, peerman, unit_tests, wipe);
-    llmq::quorumManager = std::make_unique<llmq::CQuorumManager>(evo_db, connman, *bls_worker, *llmq::quorumBlockProcessor, *qdkgsman, ::masternodeSync, peerman);
-    sigman = std::make_unique<llmq::CSigningManager>(connman, *llmq::quorumManager, peerman, unit_tests, wipe);
-    shareman = std::make_unique<llmq::CSigSharesManager>(connman, *llmq::quorumManager, *sigman, peerman);
-    llmq::chainLocksHandler = std::make_unique<llmq::CChainLocksHandler>(mempool, connman, sporkman, *sigman, *shareman, *llmq::quorumManager, *::masternodeSync, peerman);
-    llmq::quorumInstantSendManager = std::make_unique<llmq::CInstantSendManager>(mempool, connman, sporkman, *llmq::quorumManager, *sigman, *shareman, *llmq::chainLocksHandler, *::masternodeSync, peerman, unit_tests, wipe);
-
+                         const std::unique_ptr<PeerManager>& peerman, bool unit_tests, bool wipe) :
+    bls_worker{std::make_shared<CBLSWorker>()},
+    dkg_debugman{std::make_unique<llmq::CDKGDebugManager>()},
+    quorum_block_processor{[&]() -> llmq::CQuorumBlockProcessor* const {
+        assert(llmq::quorumBlockProcessor == nullptr);
+        llmq::quorumBlockProcessor = std::make_unique<llmq::CQuorumBlockProcessor>(evo_db, connman, peerman);
+        return llmq::quorumBlockProcessor.get();
+    }()},
+    qdkgsman{std::make_unique<llmq::CDKGSessionManager>(connman, *bls_worker, *dkg_debugman, *quorum_block_processor, sporkman, peerman, unit_tests, wipe)},
+    qman{[&]() -> llmq::CQuorumManager* const {
+        assert(llmq::quorumManager == nullptr);
+        llmq::quorumManager = std::make_unique<llmq::CQuorumManager>(evo_db, connman, *bls_worker, *quorum_block_processor, *qdkgsman, ::masternodeSync, peerman);
+        return llmq::quorumManager.get();
+    }()},
+    sigman{std::make_unique<llmq::CSigningManager>(connman, *llmq::quorumManager, peerman, unit_tests, wipe)},
+    shareman{std::make_unique<llmq::CSigSharesManager>(connman, *llmq::quorumManager, *sigman, peerman)},
+    clhandler{[&]() -> llmq::CChainLocksHandler* const {
+        assert(llmq::chainLocksHandler == nullptr);
+        llmq::chainLocksHandler = std::make_unique<llmq::CChainLocksHandler>(mempool, connman, sporkman, *sigman, *shareman, *llmq::quorumManager, *::masternodeSync, peerman);
+        return llmq::chainLocksHandler.get();
+    }()},
+    isman{[&]() -> llmq::CInstantSendManager* const {
+        assert(llmq::quorumInstantSendManager == nullptr);
+        llmq::quorumInstantSendManager = std::make_unique<llmq::CInstantSendManager>(mempool, connman, sporkman, *llmq::quorumManager, *sigman, *shareman, *llmq::chainLocksHandler, *::masternodeSync, peerman, unit_tests, wipe);
+        return llmq::quorumInstantSendManager.get();
+    }()}
+{
     // NOTE: we use this only to wipe the old db, do NOT use it for anything else
     // TODO: remove it in some future version
     auto llmqDbTmp = std::make_unique<CDBWrapper>(unit_tests ? "" : (GetDataDir() / "llmq"), 1 << 20, unit_tests, true);
 }
 
-void LLMQContext::Destroy() {
+LLMQContext::~LLMQContext() {
+    // LLMQContext doesn't own these objects, but still need to care of them for consistancy:
     llmq::quorumInstantSendManager.reset();
     llmq::chainLocksHandler.reset();
-    shareman.reset();
-    sigman.reset();
     llmq::quorumManager.reset();
-    qdkgsman.reset();
     llmq::quorumBlockProcessor.reset();
-    dkg_debugman.reset();
-    bls_worker.reset();
     {
         LOCK(llmq::cs_llmq_vbc);
         llmq::llmq_versionbitscache.Clear();
     }
 }
 
 void LLMQContext::Interrupt() {
-    if (shareman != nullptr) {
-        shareman->InterruptWorkerThread();
-    }
-    if (llmq::quorumInstantSendManager != nullptr) {
-        llmq::quorumInstantSendManager->InterruptWorkerThread();
-    }
+    shareman->InterruptWorkerThread();
+
+    assert(isman == llmq::quorumInstantSendManager.get());
+    llmq::quorumInstantSendManager->InterruptWorkerThread();
 }
 
 void LLMQContext::Start() {
-    if (bls_worker != nullptr) {
-        bls_worker->Start();
-    }
-    if (qdkgsman != nullptr) {
-        qdkgsman->StartThreads();
-    }
-    if (llmq::quorumManager != nullptr) {
-        llmq::quorumManager->Start();
-    }
-    if (shareman != nullptr) {
-        shareman->RegisterAsRecoveredSigsListener();
-        shareman->StartWorkerThread();
-    }
-    if (llmq::chainLocksHandler != nullptr) {
-        llmq::chainLocksHandler->Start();
-    }
-    if (llmq::quorumInstantSendManager != nullptr) {
-        llmq::quorumInstantSendManager->Start();
-    }
+    assert(quorum_block_processor == llmq::quorumBlockProcessor.get());
+    assert(qman == llmq::quorumManager.get());
+    assert(clhandler == llmq::chainLocksHandler.get());
+    assert(isman == llmq::quorumInstantSendManager.get());
+
+    bls_worker->Start();
+    qdkgsman->StartThreads();
+    qman->Start();
+    shareman->RegisterAsRecoveredSigsListener();
+    shareman->StartWorkerThread();
+
+    llmq::chainLocksHandler->Start();
+    llmq::quorumInstantSendManager->Start();
 }
 
 void LLMQContext::Stop() {
-    if (llmq::quorumInstantSendManager != nullptr) {
-        llmq::quorumInstantSendManager->Stop();
-    }
-    if (llmq::chainLocksHandler != nullptr) {
-        llmq::chainLocksHandler->Stop();
-    }
-    if (shareman != nullptr) {
-        shareman->StopWorkerThread();
-        shareman->UnregisterAsRecoveredSigsListener();
-    }
-    if (llmq::quorumManager != nullptr) {
-        llmq::quorumManager->Stop();
-    }
-    if (qdkgsman != nullptr) {
-        qdkgsman->StopThreads();
-    }
-    if (bls_worker != nullptr) {
-        bls_worker->Stop();
-    }
+    assert(quorum_block_processor == llmq::quorumBlockProcessor.get());
+    assert(qman == llmq::quorumManager.get());
+    assert(clhandler == llmq::chainLocksHandler.get());
+    assert(isman == llmq::quorumInstantSendManager.get());
+
+    llmq::quorumInstantSendManager->Stop();
+    llmq::chainLocksHandler->Stop();
+
+    shareman->StopWorkerThread();
+    shareman->UnregisterAsRecoveredSigsListener();
+    qman->Stop();
+    qdkgsman->StopThreads();
+    bls_worker->Stop();
 }

src/llmq/context.h

@@ -27,27 +27,35 @@ class CInstantSendManager;
 }
 
 struct LLMQContext {
+    LLMQContext() = delete;
+    LLMQContext(const LLMQContext&) = delete;
     LLMQContext(CEvoDB& evo_db, CTxMemPool& mempool, CConnman& connman, CSporkManager& sporkman,
                 const std::unique_ptr<PeerManager>& peerman, bool unit_tests, bool wipe);
     ~LLMQContext();
 
-    void Create(CEvoDB& evo_db, CTxMemPool& mempool, CConnman& connman, CSporkManager& sporkman,
-                const std::unique_ptr<PeerManager>& peerman, bool unit_tests, bool wipe);
-    void Destroy();
     void Interrupt();
     void Start();
     void Stop();
 
-    std::shared_ptr<CBLSWorker> bls_worker;
-    std::unique_ptr<llmq::CDKGDebugManager> dkg_debugman;
-    std::unique_ptr<llmq::CDKGSessionManager> qdkgsman;
-    std::unique_ptr<llmq::CSigSharesManager> shareman;
-    std::unique_ptr<llmq::CSigningManager> sigman;
-
-    llmq::CQuorumBlockProcessor* quorum_block_processor{nullptr};
-    llmq::CQuorumManager* qman{nullptr};
-    llmq::CChainLocksHandler* clhandler{nullptr};
-    llmq::CInstantSendManager* isman{nullptr};
+    /** Guaranteed if LLMQContext is initialized then all members are valid too
+     *
+     *  Please note, that members here should not be re-ordered, because initialization
+     *  some of them requires other member initialized.
+     *  For example, constructor `quorumManager` requires `bls_worker`.
+     *
+     *  Some objects are still global variables and their de-globalization is not trivial
+     *  at this point. LLMQContext keeps just a pointer to them and doesn't own these objects,
+     *  but it still guarantees that objects are created and valid
+     */
+    const std::shared_ptr<CBLSWorker> bls_worker;
+    const std::unique_ptr<llmq::CDKGDebugManager> dkg_debugman;
+    llmq::CQuorumBlockProcessor* const quorum_block_processor;
+    const std::unique_ptr<llmq::CDKGSessionManager> qdkgsman;
+    llmq::CQuorumManager* const qman;
+    const std::unique_ptr<llmq::CSigningManager> sigman;
+    const std::unique_ptr<llmq::CSigSharesManager> shareman;
+    llmq::CChainLocksHandler* const clhandler;
+    llmq::CInstantSendManager* const isman;
 };
 
 #endif // BITCOIN_LLMQ_CONTEXT_H

src/miner.cpp

@@ -28,6 +28,7 @@
 #include <governance/governance.h>
 #include <llmq/blockprocessor.h>
 #include <llmq/chainlocks.h>
+#include <llmq/context.h>
 #include <llmq/instantsend.h>
 #include <llmq/utils.h>
 #include <masternode/payments.h>
@@ -57,16 +58,15 @@ BlockAssembler::Options::Options() {
 }
 
 BlockAssembler::BlockAssembler(const CSporkManager& sporkManager, CGovernanceManager& governanceManager,
-                               const llmq::CQuorumBlockProcessor& quorumBlockProcessor, llmq::CChainLocksHandler& clhandler,
-                               llmq::CInstantSendManager& isman, CEvoDB& evoDb, CChainState& chainstate, const CTxMemPool& mempool, const CChainParams& params, const Options& options) :
+                               LLMQContext& llmq_ctx, CEvoDB& evoDb, CChainState& chainstate, const CTxMemPool& mempool, const CChainParams& params, const Options& options) :
       chainparams(params),
       m_mempool(mempool),
       m_chainstate(chainstate),
       spork_manager(sporkManager),
       governance_manager(governanceManager),
-      quorum_block_processor(quorumBlockProcessor),
-      m_clhandler(clhandler),
-      m_isman(isman),
+      quorum_block_processor(*llmq_ctx.quorum_block_processor),
+      m_clhandler(*llmq_ctx.clhandler),
+      m_isman(*llmq_ctx.isman),
       m_evoDb(evoDb)
 {
     blockMinFeeRate = options.blockMinFeeRate;
@@ -92,9 +92,8 @@ static BlockAssembler::Options DefaultOptions()
 }
 
 BlockAssembler::BlockAssembler(const CSporkManager& sporkManager, CGovernanceManager& governanceManager,
-                               const llmq::CQuorumBlockProcessor& quorumBlockProcessor, llmq::CChainLocksHandler& clhandler,
-                               llmq::CInstantSendManager& isman, CEvoDB& evoDb, CChainState& chainstate, const CTxMemPool& mempool, const CChainParams& params)
-    : BlockAssembler(sporkManager, governanceManager, quorumBlockProcessor, clhandler, isman, evoDb, chainstate, mempool, params, DefaultOptions()) {}
+                               LLMQContext& llmq_ctx, CEvoDB& evoDb, CChainState& chainstate, const CTxMemPool& mempool, const CChainParams& params)
+    : BlockAssembler(sporkManager, governanceManager, llmq_ctx, evoDb, chainstate, mempool, params, DefaultOptions()) {}
 
 void BlockAssembler::resetBlock()
 {

src/miner.h

@@ -24,6 +24,7 @@ class CCreditPoolDiff;
 class CGovernanceManager;
 class CScript;
 class CSporkManager;
+struct LLMQContext;
 
 namespace Consensus { struct Params; };
 namespace llmq {
@@ -172,11 +173,9 @@ class BlockAssembler
     };
 
     explicit BlockAssembler(const CSporkManager& sporkManager, CGovernanceManager& governanceManager,
-                            const llmq::CQuorumBlockProcessor& quorumBlockProcessor, llmq::CChainLocksHandler& clhandler,
-                            llmq::CInstantSendManager& isman, CEvoDB& evoDb, CChainState& chainstate, const CTxMemPool& mempool, const CChainParams& params);
+                            LLMQContext& llmq_ctx, CEvoDB& evoDb, CChainState& chainstate, const CTxMemPool& mempool, const CChainParams& params);
     explicit BlockAssembler(const CSporkManager& sporkManager, CGovernanceManager& governanceManager,
-                            const llmq::CQuorumBlockProcessor& quorumBlockProcessor, llmq::CChainLocksHandler& clhandler,
-                            llmq::CInstantSendManager& isman, CEvoDB& evoDb, CChainState& chainstate, const CTxMemPool& mempool, const CChainParams& params, const Options& options);
+                            LLMQContext& llmq_ctx, CEvoDB& evoDb, CChainState& chainstate, const CTxMemPool& mempool, const CChainParams& params, const Options& options);
 
     /** Construct a new block template with coinbase to scriptPubKeyIn */
     std::unique_ptr<CBlockTemplate> CreateNewBlock(const CScript& scriptPubKeyIn);

src/rpc/evo.cpp

@@ -1471,7 +1471,7 @@ static UniValue protx_diff(const JSONRPCRequest& request, const ChainstateManage
 
     CSimplifiedMNListDiff mnListDiff;
     std::string strError;
-    LLMQContext& llmq_ctx = EnsureAnyLLMQContext(request.context);
+    const LLMQContext& llmq_ctx = EnsureAnyLLMQContext(request.context);
 
     if (!BuildSimplifiedMNListDiff(baseBlockHash, blockHash, mnListDiff, *llmq_ctx.quorum_block_processor, strError, extended)) {
         throw std::runtime_error(strError);

src/rpc/mining.cpp

@@ -149,8 +149,7 @@ static bool GenerateBlock(ChainstateManager& chainman, CBlock& block, uint64_t&
 }
 
 static UniValue generateBlocks(ChainstateManager& chainman, const CTxMemPool& mempool, CEvoDB& evodb,
-                        llmq::CQuorumBlockProcessor& quorum_block_processor, llmq::CChainLocksHandler& clhandler,
-                        llmq::CInstantSendManager& isman, const CScript& coinbase_script, int nGenerate, uint64_t nMaxTries)
+                        LLMQContext& llmq_ctx, const CScript& coinbase_script, int nGenerate, uint64_t nMaxTries)
 {
     int nHeightEnd = 0;
     int nHeight = 0;
@@ -165,7 +164,7 @@ static UniValue generateBlocks(ChainstateManager& chainman, const CTxMemPool& me
     UniValue blockHashes(UniValue::VARR);
     while (nHeight < nHeightEnd && !ShutdownRequested())
     {
-        std::unique_ptr<CBlockTemplate> pblocktemplate(BlockAssembler(*sporkManager, *governance, quorum_block_processor, clhandler, isman, evodb, chainman.ActiveChainstate(), mempool, Params()).CreateNewBlock(coinbase_script));
+        std::unique_ptr<CBlockTemplate> pblocktemplate(BlockAssembler(*sporkManager, *governance, llmq_ctx, evodb, chainman.ActiveChainstate(), mempool, Params()).CreateNewBlock(coinbase_script));
         if (!pblocktemplate.get())
             throw JSONRPCError(RPC_INTERNAL_ERROR, "Couldn't create new block");
         CBlock *pblock = &pblocktemplate->block;
@@ -252,7 +251,7 @@ static UniValue generatetodescriptor(const JSONRPCRequest& request)
     ChainstateManager& chainman = EnsureChainman(node);
     LLMQContext& llmq_ctx = EnsureLLMQContext(node);
 
-    return generateBlocks(chainman, mempool, *node.evodb, *llmq_ctx.quorum_block_processor, *llmq_ctx.clhandler, *llmq_ctx.isman, coinbase_script, num_blocks, max_tries);
+    return generateBlocks(chainman, mempool, *node.evodb, llmq_ctx, coinbase_script, num_blocks, max_tries);
 }
 
 static UniValue generatetoaddress(const JSONRPCRequest& request)
@@ -291,7 +290,7 @@ static UniValue generatetoaddress(const JSONRPCRequest& request)
 
     CScript coinbase_script = GetScriptForDestination(destination);
 
-    return generateBlocks(chainman, mempool, *node.evodb, *llmq_ctx.quorum_block_processor, *llmq_ctx.clhandler, *llmq_ctx.isman, coinbase_script, num_blocks, max_tries);
+    return generateBlocks(chainman, mempool, *node.evodb, llmq_ctx, coinbase_script, num_blocks, max_tries);
 }
 
 static UniValue generateblock(const JSONRPCRequest& request)
@@ -371,7 +370,7 @@ static UniValue generateblock(const JSONRPCRequest& request)
         LOCK(cs_main);
 
         CTxMemPool empty_mempool;
-        std::unique_ptr<CBlockTemplate> blocktemplate(BlockAssembler(*sporkManager, *governance, *llmq_ctx.quorum_block_processor, *llmq_ctx.clhandler, *llmq_ctx.isman, *node.evodb, active_chainstate, empty_mempool, chainparams).CreateNewBlock(coinbase_script));
+        std::unique_ptr<CBlockTemplate> blocktemplate(BlockAssembler(*sporkManager, *governance, llmq_ctx, *node.evodb, active_chainstate, empty_mempool, chainparams).CreateNewBlock(coinbase_script));
         if (!blocktemplate) {
             throw JSONRPCError(RPC_INTERNAL_ERROR, "Couldn't create new block");
         }
@@ -787,7 +786,7 @@ static UniValue getblocktemplate(const JSONRPCRequest& request)
         // Create new block
         CScript scriptDummy = CScript() << OP_TRUE;
         LLMQContext& llmq_ctx = EnsureAnyLLMQContext(request.context);
-        pblocktemplate = BlockAssembler(*sporkManager, *governance, *llmq_ctx.quorum_block_processor, *llmq_ctx.clhandler, *llmq_ctx.isman, *node.evodb, active_chainstate, mempool, Params()).CreateNewBlock(scriptDummy);
+        pblocktemplate = BlockAssembler(*sporkManager, *governance, llmq_ctx, *node.evodb, active_chainstate, mempool, Params()).CreateNewBlock(scriptDummy);
         if (!pblocktemplate)
             throw JSONRPCError(RPC_OUT_OF_MEMORY, "Out of memory");