refactor: remove need for mutual access to private members

kwvg · kwvg · commit e742b7b6d59a · 2025-08-02T16:56:35.000Z
diff --git a/src/chainlock/chainlock.cpp b/src/chainlock/chainlock.cpp
@@ -32,12 +32,23 @@ CTransactionRef GetTransaction(const CBlockIndex* const block_index, const CTxMe
 using node::GetTransaction;
 
 namespace llmq {
+namespace {
+static constexpr int64_t CLEANUP_INTERVAL{1000 * 30};
+static constexpr int64_t CLEANUP_SEEN_TIMEOUT{24 * 60 * 60 * 1000};
+//! How long to wait for islocks until we consider a block with non-islocked TXs to be safe to sign
+static constexpr int64_t WAIT_FOR_ISLOCK_TIMEOUT{10 * 60};
+} // anonymous namespace
+
+bool AreChainLocksEnabled(const CSporkManager& sporkman)
+{
+    return sporkman.IsSporkActive(SPORK_19_CHAINLOCKS_ENABLED);
+}
+
 CChainLocksHandler::CChainLocksHandler(CChainState& chainstate, CQuorumManager& _qman, CSigningManager& _sigman,
                                        CSigSharesManager& _shareman, CSporkManager& sporkman, CTxMemPool& _mempool,
                                        const CMasternodeSync& mn_sync, bool is_masternode) :
     m_chainstate{chainstate},
     qman{_qman},
-    sigman{_sigman},
     spork_manager{sporkman},
     mempool{_mempool},
     m_mn_sync{mn_sync},
@@ -59,7 +70,7 @@ CChainLocksHandler::~CChainLocksHandler()
 void CChainLocksHandler::Start(const llmq::CInstantSendManager& isman)
 {
     if (m_signer) {
-        sigman.RegisterRecoveredSigsListener(m_signer.get());
+        m_signer->Start();
     }
     scheduler->scheduleEvery([&]() {
         CheckActiveState();
@@ -76,7 +87,7 @@ void CChainLocksHandler::Stop()
 {
     scheduler->stop();
     if (m_signer) {
-        sigman.UnregisterRecoveredSigsListener(m_signer.get());
+        m_signer->Stop();
     }
 }
 
@@ -105,6 +116,15 @@ chainlock::ChainLockSig CChainLocksHandler::GetBestChainLock() const
     return bestChainLock;
 }
 
+void CChainLocksHandler::UpdateTxFirstSeenMap(const std::unordered_set<uint256, StaticSaltedHasher>& tx, const int64_t& time)
+{
+    AssertLockNotHeld(cs);
+    LOCK(cs);
+    for (const auto& txid : tx) {
+        txFirstSeenTime.emplace(txid, time);
+    }
+}
+
 MessageProcessingResult CChainLocksHandler::ProcessNewChainLock(const NodeId from, const chainlock::ChainLockSig& clsig,
                                                                 const uint256& hash)
 {
@@ -258,30 +278,24 @@ void CChainLocksHandler::BlockConnected(const std::shared_ptr<const CBlock>& pbl
 
     // We need this information later when we try to sign a new tip, so that we can determine if all included TXs are safe.
     if (m_signer) {
-        LOCK(m_signer->cs_signer);
-        auto it = m_signer->blockTxs.find(pindex->GetBlockHash());
-        if (it == m_signer->blockTxs.end()) {
-            // We must create this entry even if there are no lockable transactions in the block, so that TrySignChainTip
-            // later knows about this block
-            it = m_signer->blockTxs.emplace(pindex->GetBlockHash(), std::make_shared<std::unordered_set<uint256, StaticSaltedHasher>>()).first;
-        }
-        auto& txids = *it->second;
-        for (const auto& tx : pblock->vtx) {
-            if (!tx->IsCoinBase() && !tx->vin.empty()) {
-                txids.emplace(tx->GetHash());
-            }
-        }
+        m_signer->UpdateBlockHashTxidMap(pindex->GetBlockHash(), pblock->vtx);
     }
 }
 
 void CChainLocksHandler::BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, gsl::not_null<const CBlockIndex*> pindexDisconnected)
 {
     if (m_signer) {
-        LOCK(m_signer->cs_signer);
-        m_signer->blockTxs.erase(pindexDisconnected->GetBlockHash());
+        m_signer->EraseFromBlockHashTxidMap(pindexDisconnected->GetBlockHash());
     }
 }
 
+int32_t CChainLocksHandler::GetBestChainLockHeight() const
+{
+    AssertLockNotHeld(cs);
+    LOCK(cs);
+    return bestChainLock.getHeight();
+}
+
 bool CChainLocksHandler::IsTxSafeForMining(const uint256& txid) const
 {
     int64_t txAge = 0;
@@ -309,7 +323,7 @@ void CChainLocksHandler::EnforceBestChainLock()
     {
         LOCK(cs);
 
-        if (!isEnabled) {
+        if (!IsEnabled()) {
             return;
         }
 
@@ -371,7 +385,7 @@ bool CChainLocksHandler::InternalHasChainLock(int nHeight, const uint256& blockH
 {
     AssertLockHeld(cs);
 
-    if (!isEnabled) {
+    if (!IsEnabled()) {
         return false;
     }
 
@@ -401,7 +415,7 @@ bool CChainLocksHandler::InternalHasConflictingChainLock(int nHeight, const uint
 {
     AssertLockHeld(cs);
 
-    if (!isEnabled) {
+    if (!IsEnabled()) {
         return false;
     }
 
@@ -474,9 +488,4 @@ void CChainLocksHandler::Cleanup()
         }
     }
 }
-
-bool AreChainLocksEnabled(const CSporkManager& sporkman)
-{
-    return sporkman.IsSporkActive(SPORK_19_CHAINLOCKS_ENABLED);
-}
 } // namespace llmq
diff --git a/src/chainlock/chainlock.h b/src/chainlock/chainlock.h
@@ -40,16 +40,9 @@ enum class VerifyRecSigStatus;
 
 class CChainLocksHandler
 {
-    static constexpr int64_t CLEANUP_INTERVAL = 1000 * 30;
-    static constexpr int64_t CLEANUP_SEEN_TIMEOUT = 24 * 60 * 60 * 1000;
-
-    // how long to wait for islocks until we consider a block with non-islocked TXs to be safe to sign
-    static constexpr int64_t WAIT_FOR_ISLOCK_TIMEOUT = 10 * 60;
-
 private:
     CChainState& m_chainstate;
     CQuorumManager& qman;
-    CSigningManager& sigman;
     CSporkManager& spork_manager;
     CTxMemPool& mempool;
     const CMasternodeSync& m_mn_sync;
@@ -87,6 +80,8 @@ class CChainLocksHandler
     bool AlreadyHave(const CInv& inv) const EXCLUSIVE_LOCKS_REQUIRED(!cs);
     bool GetChainLockByHash(const uint256& hash, chainlock::ChainLockSig& ret) const EXCLUSIVE_LOCKS_REQUIRED(!cs);
     chainlock::ChainLockSig GetBestChainLock() const EXCLUSIVE_LOCKS_REQUIRED(!cs);
+    void UpdateTxFirstSeenMap(const std::unordered_set<uint256, StaticSaltedHasher>& tx, const int64_t& time)
+        EXCLUSIVE_LOCKS_REQUIRED(!cs);
 
     [[nodiscard]] MessageProcessingResult ProcessNewChainLock(NodeId from, const chainlock::ChainLockSig& clsig,
                                                               const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(!cs);
@@ -103,16 +98,16 @@ class CChainLocksHandler
     bool HasConflictingChainLock(int nHeight, const uint256& blockHash) const EXCLUSIVE_LOCKS_REQUIRED(!cs);
     VerifyRecSigStatus VerifyChainLock(const chainlock::ChainLockSig& clsig) const;
 
+    [[nodiscard]] int32_t GetBestChainLockHeight() const EXCLUSIVE_LOCKS_REQUIRED(!cs);
     bool IsTxSafeForMining(const uint256& txid) const EXCLUSIVE_LOCKS_REQUIRED(!cs);
+    [[nodiscard]] bool IsEnabled() const { return isEnabled; }
 
 private:
     // these require locks to be held already
     bool InternalHasChainLock(int nHeight, const uint256& blockHash) const EXCLUSIVE_LOCKS_REQUIRED(cs);
     bool InternalHasConflictingChainLock(int nHeight, const uint256& blockHash) const EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     void Cleanup() EXCLUSIVE_LOCKS_REQUIRED(!cs);
-
-    friend class ::chainlock::ChainLockSigner;
 };
 
 bool AreChainLocksEnabled(const CSporkManager& sporkman);
diff --git a/src/chainlock/signing.cpp b/src/chainlock/signing.cpp
@@ -30,13 +30,23 @@ ChainLockSigner::ChainLockSigner(CChainState& chainstate, llmq::CChainLocksHandl
 
 ChainLockSigner::~ChainLockSigner() = default;
 
+void ChainLockSigner::Start()
+{
+    m_sigman.RegisterRecoveredSigsListener(this);
+}
+
+void ChainLockSigner::Stop()
+{
+    m_sigman.UnregisterRecoveredSigsListener(this);
+}
+
 void ChainLockSigner::TrySignChainTip(const llmq::CInstantSendManager& isman)
 {
     if (!m_mn_sync.IsBlockchainSynced()) {
         return;
     }
 
-    if (!m_clhandler.isEnabled) {
+    if (!m_clhandler.IsEnabled()) {
         return;
     }
 
@@ -57,23 +67,23 @@ void ChainLockSigner::TrySignChainTip(const llmq::CInstantSendManager& isman)
     // Later, we'll add the multiple attempts process.
 
     {
-        LOCK2(m_clhandler.cs, cs_signer);
+        LOCK(cs_signer);
 
         if (pindex->nHeight == lastSignedHeight) {
             // already signed this one
             return;
         }
+    }
 
-        if (m_clhandler.bestChainLock.getHeight() >= pindex->nHeight) {
-            // already got the same CLSIG or a better one
-            return;
-        }
+    if (m_clhandler.GetBestChainLockHeight() >= pindex->nHeight) {
+        // already got the same CLSIG or a better one
+        return;
+    }
 
-        if (m_clhandler.InternalHasConflictingChainLock(pindex->nHeight, pindex->GetBlockHash())) {
-            // don't sign if another conflicting CLSIG is already present. EnforceBestChainLock will later enforce
-            // the correct chain.
-            return;
-        }
+    if (m_clhandler.HasConflictingChainLock(pindex->nHeight, pindex->GetBlockHash())) {
+        // don't sign if another conflicting CLSIG is already present. EnforceBestChainLock will later enforce
+        // the correct chain.
+        return;
     }
 
     LogPrint(BCLog::CHAINLOCKS, "%s -- trying to sign %s, height=%d\n", __func__, pindex->GetBlockHash().ToString(), pindex->nHeight);
@@ -104,18 +114,9 @@ void ChainLockSigner::TrySignChainTip(const llmq::CInstantSendManager& isman)
             }
 
             for (const auto& txid : *txids) {
-                int64_t txAge = 0;
-                {
-                    LOCK(m_clhandler.cs);
-                    auto it = m_clhandler.txFirstSeenTime.find(txid);
-                    if (it != m_clhandler.txFirstSeenTime.end()) {
-                        txAge = GetTime<std::chrono::seconds>().count() - it->second;
-                    }
-                }
-
-                if (txAge < m_clhandler.WAIT_FOR_ISLOCK_TIMEOUT && !isman.IsLocked(txid)) {
-                    LogPrint(BCLog::CHAINLOCKS, "%s -- not signing block %s due to TX %s not being islocked and not old enough. age=%d\n", __func__,
-                              pindexWalk->GetBlockHash().ToString(), txid.ToString(), txAge);
+                if (!m_clhandler.IsTxSafeForMining(txid) && !isman.IsLocked(txid)) {
+                    LogPrint(BCLog::CHAINLOCKS, "%s -- not signing block %s due to TX %s not being islocked and not old enough.\n", __func__,
+                              pindexWalk->GetBlockHash().ToString(), txid.ToString());
                     return;
                 }
             }
@@ -127,12 +128,9 @@ void ChainLockSigner::TrySignChainTip(const llmq::CInstantSendManager& isman)
     uint256 requestId = ::SerializeHash(std::make_pair(CLSIG_REQUESTID_PREFIX, pindex->nHeight));
     uint256 msgHash = pindex->GetBlockHash();
 
-    {
-        LOCK(m_clhandler.cs);
-        if (m_clhandler.bestChainLock.getHeight() >= pindex->nHeight) {
-            // might have happened while we didn't hold cs
-            return;
-        }
+    if (m_clhandler.GetBestChainLockHeight() >= pindex->nHeight) {
+        // might have happened while we didn't hold cs
+        return;
     }
     {
         LOCK(cs_signer);
@@ -144,10 +142,34 @@ void ChainLockSigner::TrySignChainTip(const llmq::CInstantSendManager& isman)
     m_sigman.AsyncSignIfMember(Params().GetConsensus().llmqTypeChainLocks, m_shareman, requestId, msgHash);
 }
 
+void ChainLockSigner::EraseFromBlockHashTxidMap(const uint256& hash)
+{
+    AssertLockNotHeld(cs_signer);
+    LOCK(cs_signer);
+    blockTxs.erase(hash);
+}
+
+void ChainLockSigner::UpdateBlockHashTxidMap(const uint256& hash, const std::vector<CTransactionRef>& vtx)
+{
+    AssertLockNotHeld(cs_signer);
+    LOCK(cs_signer);
+    auto it = blockTxs.find(hash);
+    if (it == blockTxs.end()) {
+        // We must create this entry even if there are no lockable transactions in the block, so that TrySignChainTip
+        // later knows about this block
+        it = blockTxs.emplace(hash, std::make_shared<std::unordered_set<uint256, StaticSaltedHasher>>()).first;
+    }
+    auto& txids = *it->second;
+    for (const auto& tx : vtx) {
+        if (!tx->IsCoinBase() && !tx->vin.empty()) {
+            txids.emplace(tx->GetHash());
+        }
+    }
+}
+
 ChainLockSigner::BlockTxs::mapped_type ChainLockSigner::GetBlockTxs(const uint256& blockHash)
 {
     AssertLockNotHeld(cs_signer);
-    AssertLockNotHeld(m_clhandler.cs);
     AssertLockNotHeld(::cs_main);
 
     ChainLockSigner::BlockTxs::mapped_type ret;
@@ -188,31 +210,26 @@ ChainLockSigner::BlockTxs::mapped_type ChainLockSigner::GetBlockTxs(const uint25
             LOCK(cs_signer);
             blockTxs.emplace(blockHash, ret);
         }
-        {
-            LOCK(m_clhandler.cs);
-            for (const auto& txid : *ret) {
-                m_clhandler.txFirstSeenTime.emplace(txid, blockTime);
-            }
-        }
+        m_clhandler.UpdateTxFirstSeenMap(*ret, blockTime);
     }
     return ret;
 }
 
 MessageProcessingResult ChainLockSigner::HandleNewRecoveredSig(const llmq::CRecoveredSig& recoveredSig)
 {
-    if (!m_clhandler.isEnabled) {
+    if (!m_clhandler.IsEnabled()) {
         return {};
     }
 
     ChainLockSig clsig;
     {
-        LOCK2(m_clhandler.cs, cs_signer);
+        LOCK(cs_signer);
 
         if (recoveredSig.getId() != lastSignedRequestId || recoveredSig.getMsgHash() != lastSignedMsgHash) {
             // this is not what we signed, so lets not create a CLSIG for it
             return {};
         }
-        if (m_clhandler.bestChainLock.getHeight() >= lastSignedHeight) {
+        if (m_clhandler.GetBestChainLockHeight() >= lastSignedHeight) {
             // already got the same or a better CLSIG through the CLSIG message
             return {};
         }
diff --git a/src/chainlock/signing.h b/src/chainlock/signing.h
@@ -49,6 +49,14 @@ class ChainLockSigner : public llmq::CRecoveredSigsListener
                              llmq::CSigSharesManager& shareman, CSporkManager& sporkman, const CMasternodeSync& mn_sync);
     ~ChainLockSigner();
 
+    void Start();
+    void Stop();
+
+    void EraseFromBlockHashTxidMap(const uint256& hash)
+        EXCLUSIVE_LOCKS_REQUIRED(!cs_signer);
+    void UpdateBlockHashTxidMap(const uint256& hash, const std::vector<CTransactionRef>& vtx)
+        EXCLUSIVE_LOCKS_REQUIRED(!cs_signer);
+
     void TrySignChainTip(const llmq::CInstantSendManager& isman)
         EXCLUSIVE_LOCKS_REQUIRED(!cs_signer);
     [[nodiscard]] MessageProcessingResult HandleNewRecoveredSig(const llmq::CRecoveredSig& recoveredSig) override
@@ -60,8 +68,6 @@ class ChainLockSigner : public llmq::CRecoveredSigsListener
 private:
     [[nodiscard]] BlockTxs::mapped_type GetBlockTxs(const uint256& blockHash)
         EXCLUSIVE_LOCKS_REQUIRED(!cs_signer);
-
-    friend class ::llmq::CChainLocksHandler;
 };
 } // namespace chainlock
 
