Merge #7178: refactor: remove dependencies of CInstantSendManager on mempool, signer, chainstate

a98d36c fix: set cached tip for isman without validationinterface (Konstantin Akimov)
770819e fix: code review comment by pastaclaw (Konstantin Akimov)
0c6db64 refactor: review fixes for CInstantSendManager decoupling (UdjinM6)
2f47b9e refactor: removed unused includes (Konstantin Akimov)
17db40f refactor: drop dependency of CInstantSendManager on chainlocks (Konstantin Akimov)
f506a00 refactor: drop dependency of CInstantSendManager on llmq::CSigningManager (Konstantin Akimov)
843716a refactor: move usages sigman directly to NetInstantSend (Konstantin Akimov)
1e88ac3 refactor: drop helper interface InstantSendSignerParent (Konstantin Akimov)
9c84ed3 refactor: re-order clean-up calls for confirmed block for instant-send handling (Konstantin Akimov)
8d0a772 refactor: remove dependency of CInstantSendManager on signer (Konstantin Akimov)
9c0e03f refactor: remove dependency of CInstantSendManager on chainstate (Konstantin Akimov)
2a06aec refactor: use only cached height of tip, don't retrieve it directly ever (Konstantin Akimov)
32916a4 refactor: move ResolveBlockConflicts to NetInstantSend (Konstantin Akimov)
5937d78 refactor: move all notification handlers from CInstantSendManager to NetInstantSend (Konstantin Akimov)
7785d7c refactor: drop dependency of mempool from instantsend/instantsend (Konstantin Akimov)
654ba8a refactor: move usages of mempool from CInstantSendManager to NetInstantSend (Konstantin Akimov)
a469587 refactor: simplify ProcessInstantSendLock by using PeerManager on place instead of returning std::variant (Konstantin Akimov)
2c23869 refactor: move ProcessInstantSendLock to NetInstantSend (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented

  Basically, CInstantSendManager's responsibility is remembering all known instant-send locks (by using `CInstantSendDb`) and it is used all over codebase to answer a question a basic but important question "Is transaction X locked?".

  In details, there are 4 public interfaces that are used to answer that question:
   - is transaction instant-send locked? `IsLocked`
   - is transaction / IS lock known? `IsKnown`, `AlreadyHave`
   - is instant-send enabled? (spork's related features, relevant for testnet & regtest only)
   - get me IS lock! (`GetInstantSendLockByTxid`, `GetInstantSendLockByHash`).

  In the reality, current implementation of CInstantSendManager has knowledges about multiple systems and components that are irrelevant, but not really needed to provide this base functionality.

  ## What was done?
  Removed dependency of CInstantSendManager on `instantsend::InstantSendSigner`, `llmq::CSigningManager`, `CChainState`, `chainlock::Chainlocks`, `CTxMemPool`.

  Removing dependency on CMasternodeSync is excluded from this PR because it requires bitcoin/bitcoin#25073 to be done first.

  ## How Has This Been Tested?
  Run unit & functional tests, linters.

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

ACKs for top commit:
  UdjinM6:
    utACK a98d36c

Tree-SHA512: a03b843f20fa08d9771a66eb4a5273d0340f0072f6568914d703ef8a31643e14e157bdb03171499a38b84603a8ae6906e99610a9d0d18de93347c6eec5c03f46

Author: PastaPastaPasta

src/active/context.cpp

@@ -32,7 +32,6 @@ ActiveContext::ActiveContext(CBLSWorker& bls_worker, ChainstateManager& chainman
                              const CMasternodeSync& mn_sync, const CBLSSecretKey& operator_sk,
                              const llmq::QvvecSyncModeMap& sync_map, const util::DbWrapperParams& db_params,
                              bool quorums_recovery, bool quorums_watch) :
-    m_isman{isman},
     m_qman{qman},
     nodeman{std::make_unique<CActiveMasternodeManager>(connman, dmnman, operator_sk)},
     dkgdbgman{std::make_unique<llmq::CDKGDebugManager>(dmnman, qsnapman, chainman)},
@@ -53,14 +52,12 @@ ActiveContext::ActiveContext(CBLSWorker& bls_worker, ChainstateManager& chainman
                                                                qblockman, qsnapman, *nodeman, chainman, sporkman,
                                                                llmq_params, quorums_watch, quorum_idx);
     });
-    m_isman.ConnectSigner(is_signer.get());
     m_qman.ConnectManagers(qman_handler.get(), qdkgsman.get());
 }
 
 ActiveContext::~ActiveContext()
 {
     m_qman.DisconnectManagers();
-    m_isman.DisconnectSigner();
 }
 
 void ActiveContext::Start(CConnman& connman, PeerManager& peerman, int16_t worker_count)

src/active/context.h

@@ -54,7 +54,6 @@ struct DbWrapperParams;
 
 struct ActiveContext final : public CValidationInterface {
 private:
-    llmq::CInstantSendManager& m_isman;
     llmq::CQuorumManager& m_qman;
 
 public:
@@ -98,6 +97,8 @@ struct ActiveContext final : public CValidationInterface {
     const std::unique_ptr<llmq::CEHFSignalsHandler> ehf_sighandler;
     const std::unique_ptr<llmq::QuorumParticipant> qman_handler;
     const std::unique_ptr<chainlock::ChainLockSigner> cl_signer;
+
+public:
     const std::unique_ptr<instantsend::InstantSendSigner> is_signer;
 
     /** Owned by PeerManager, use GetCJServer() */

src/dsnotificationinterface.cpp

@@ -5,34 +5,25 @@
 
 #include <dsnotificationinterface.h>
 
-#include <util/check.h>
-#include <validation.h>
-
 #include <coinjoin/coinjoin.h>
 #include <evo/deterministicmns.h>
 #include <evo/mnauth.h>
 #include <governance/governance.h>
 #include <instantsend/instantsend.h>
-#include <llmq/context.h>
-#include <llmq/dkgsessionmgr.h>
-#include <llmq/ehf_signals.h>
-#include <llmq/quorumsman.h>
 #include <masternode/sync.h>
+#include <util/check.h>
+#include <validation.h>
 
-CDSNotificationInterface::CDSNotificationInterface(CConnman& connman,
-                                                   CDSTXManager& dstxman,
-                                                   CMasternodeSync& mn_sync,
-                                                   CGovernanceManager& govman,
-                                                   const ChainstateManager& chainman,
-                                                   const std::unique_ptr<CDeterministicMNManager>& dmnman,
-                                                   const std::unique_ptr<LLMQContext>& llmq_ctx) :
+
+CDSNotificationInterface::CDSNotificationInterface(CConnman& connman, CDSTXManager& dstxman, CMasternodeSync& mn_sync,
+                                                   CGovernanceManager& govman, const ChainstateManager& chainman,
+                                                   const std::unique_ptr<CDeterministicMNManager>& dmnman) :
     m_connman{connman},
     m_dstxman{dstxman},
     m_mn_sync{mn_sync},
     m_govman{govman},
     m_chainman{chainman},
-    m_dmnman{dmnman},
-    m_llmq_ctx{llmq_ctx}
+    m_dmnman{dmnman}
 {
 }
 
@@ -76,7 +67,6 @@ void CDSNotificationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, con
         m_dstxman.UpdatedBlockTip(pindexNew);
     }
 
-    m_llmq_ctx->isman->UpdatedBlockTip(pindexNew);
     if (m_govman.IsValid()) {
         m_govman.UpdatedBlockTip(pindexNew);
     }
@@ -85,25 +75,16 @@ void CDSNotificationInterface::UpdatedBlockTip(const CBlockIndex *pindexNew, con
 void CDSNotificationInterface::TransactionAddedToMempool(const CTransactionRef& ptx, int64_t nAcceptTime,
                                                          uint64_t mempool_sequence)
 {
-    Assert(m_llmq_ctx)->isman->TransactionAddedToMempool(ptx);
     m_dstxman.TransactionAddedToMempool(ptx);
 }
 
-void CDSNotificationInterface::TransactionRemovedFromMempool(const CTransactionRef& ptx, MemPoolRemovalReason reason,
-                                                             uint64_t mempool_sequence)
-{
-    Assert(m_llmq_ctx)->isman->TransactionRemovedFromMempool(ptx);
-}
-
 void CDSNotificationInterface::BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindex)
 {
-    Assert(m_llmq_ctx)->isman->BlockConnected(pblock, pindex);
     m_dstxman.BlockConnected(pblock, pindex);
 }
 
 void CDSNotificationInterface::BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexDisconnected)
 {
-    Assert(m_llmq_ctx)->isman->BlockDisconnected(pblock, pindexDisconnected);
     m_dstxman.BlockDisconnected(pblock, pindexDisconnected);
 }
 
@@ -118,7 +99,6 @@ void CDSNotificationInterface::NotifyMasternodeListChanged(bool undo, const CDet
 void CDSNotificationInterface::NotifyChainLock(const CBlockIndex* pindex,
                                                const std::shared_ptr<const chainlock::ChainLockSig>& clsig)
 {
-    Assert(m_llmq_ctx)->isman->NotifyChainLock(pindex);
     if (m_mn_sync.IsBlockchainSynced()) {
         m_dstxman.NotifyChainLock(pindex);
     }

src/dsnotificationinterface.h

@@ -13,21 +13,16 @@ class CDeterministicMNManager;
 class CGovernanceManager;
 class ChainstateManager;
 class CMasternodeSync;
-struct LLMQContext;
 
 class CDSNotificationInterface : public CValidationInterface
 {
 public:
     CDSNotificationInterface() = delete;
     CDSNotificationInterface(const CDSNotificationInterface&) = delete;
     CDSNotificationInterface& operator=(const CDSNotificationInterface&) = delete;
-    explicit CDSNotificationInterface(CConnman& connman,
-                                      CDSTXManager& dstxman,
-                                      CMasternodeSync& mn_sync,
-                                      CGovernanceManager& govman,
-                                      const ChainstateManager& chainman,
-                                      const std::unique_ptr<CDeterministicMNManager>& dmnman,
-                                      const std::unique_ptr<LLMQContext>& llmq_ctx);
+    explicit CDSNotificationInterface(CConnman& connman, CDSTXManager& dstxman, CMasternodeSync& mn_sync,
+                                      CGovernanceManager& govman, const ChainstateManager& chainman,
+                                      const std::unique_ptr<CDeterministicMNManager>& dmnman);
     virtual ~CDSNotificationInterface();
 
     // a small helper to initialize current block height in sub-modules on startup
@@ -40,8 +35,6 @@ class CDSNotificationInterface : public CValidationInterface
     void SynchronousUpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) override;
     void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) override;
     void TransactionAddedToMempool(const CTransactionRef& tx, int64_t nAcceptTime, uint64_t mempool_sequence) override;
-    void TransactionRemovedFromMempool(const CTransactionRef& ptx, MemPoolRemovalReason reason,
-                                       uint64_t mempool_sequence) override;
     void BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindex) override;
     void BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexDisconnected) override;
     void NotifyMasternodeListChanged(bool undo, const CDeterministicMNList& oldMNList, const CDeterministicMNListDiff& diff) override;
@@ -54,7 +47,6 @@ class CDSNotificationInterface : public CValidationInterface
     CGovernanceManager& m_govman;
     const ChainstateManager& m_chainman;
     const std::unique_ptr<CDeterministicMNManager>& m_dmnman;
-    const std::unique_ptr<LLMQContext>& m_llmq_ctx;
 };
 
 extern std::unique_ptr<CDSNotificationInterface> g_ds_notification_interface;

src/init.cpp

@@ -97,6 +97,7 @@
 #include <instantsend/net_instantsend.h>
 #include <llmq/context.h>
 #include <llmq/dkgsessionmgr.h>
+#include <llmq/signing.h>
 #include <llmq/net_signing.h>
 #include <llmq/options.h>
 #include <llmq/observer/context.h>
@@ -2180,7 +2181,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     RegisterValidationInterface(node.peerman.get());
 
     g_ds_notification_interface = std::make_unique<CDSNotificationInterface>(
-        *node.connman, *node.dstxman, *node.mn_sync, *node.govman, chainman, node.dmnman, node.llmq_ctx
+        *node.connman, *node.dstxman, *node.mn_sync, *node.govman, chainman, node.dmnman // todo: replace unique_ptr for dmnman to reference
     );
     RegisterValidationInterface(g_ds_notification_interface.get());
 
@@ -2212,7 +2213,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 
     // ********************************************************* Step 7d: Setup other Dash services
 
-    node.peerman->AddExtraHandler(std::make_unique<NetInstantSend>(node.peerman.get(), *node.llmq_ctx->isman, *node.llmq_ctx->qman, chainman.ActiveChainstate()));
+    node.peerman->AddExtraHandler(std::make_unique<NetInstantSend>(node.peerman.get(), *node.llmq_ctx->isman, node.active_ctx ? node.active_ctx->is_signer.get() : nullptr, *node.llmq_ctx->sigman, *node.llmq_ctx->qman, *node.chainlocks, chainman.ActiveChainstate(), *node.mempool, *node.mn_sync));
     node.peerman->AddExtraHandler(std::make_unique<llmq::NetSigning>(node.peerman.get(), *node.llmq_ctx->sigman, node.active_ctx ? node.active_ctx->shareman.get() : nullptr, *node.sporkman));
 
     if (node.active_ctx) {
@@ -2431,6 +2432,11 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
             // so that GetProTxHash() is available for quorum connection setup.
         }
 
+        // Seed InstantSend tip-height cache; NetInstantSend receives future
+        // updates via CValidationInterface but misses InitializeCurrentBlockTip.
+        // TODO: move cache updates from NetInstantSend to g_ds_notification due to specific of Tip's processing
+        node.llmq_ctx->isman->CacheTipHeight(chainman.ActiveChain().Tip());
+
         {
             // Get all UTXOs for each MN collateral in one go so that we can fill coin cache early
             // and reduce further locking overhead for cs_main in other parts of code including GUI