refactor: review fixes for CInstantSendManager decoupling

- Rename IsKnownTx to HasTxForLock for clarity (returns true when
  the lock's transaction is known, not pending without a TX)
- Fix unconditional LookupBlockIndex in CheckCanLock: only take
  cs_main on cache miss instead of every call
- Fix log prefix typos: NetSigning -> NetInstantSend in
  ProcessInstantSendLock
- Remove redundant LookupBlockIndex fallback in ProcessInstantSendLock
  that duplicated the static GetBlockHeight helper
- Simplify AttachISLockToTx: use for-loop with early-continue,
  remove unnecessary variable
- Remove double blank line

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: 2 people

src/active/context.h

@@ -100,6 +100,7 @@ struct ActiveContext final : public CValidationInterface {
 public:
     const std::unique_ptr<instantsend::InstantSendSigner> is_signer;
 
+public:
     /** Owned by PeerManager, use GetCJServer() */
     CCoinJoinServer* m_cj_server{nullptr};
 };

src/instantsend/instantsend.cpp

@@ -145,22 +145,19 @@ void CInstantSendManager::RemoveBlockISLocks(const std::shared_ptr<const CBlock>
 
 instantsend::InstantSendLockPtr CInstantSendManager::AttachISLockToTx(const CTransactionRef& tx)
 {
-    instantsend::InstantSendLockPtr ret_islock{nullptr};
     LOCK(cs_pendingLocks);
-    auto it = pendingNoTxInstantSendLocks.begin();
-    while (it != pendingNoTxInstantSendLocks.end()) {
-        if (it->second.islock->txid == tx->GetHash()) {
-            // we received an islock earlier, let's put it back into pending and verify/lock
-            LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, islock=%s\n", __func__,
-                     tx->GetHash().ToString(), it->first.ToString());
-            ret_islock = it->second.islock;
-            pendingInstantSendLocks.try_emplace(it->first, it->second);
-            pendingNoTxInstantSendLocks.erase(it);
-            return ret_islock;
-        }
-        ++it;
+    for (auto it = pendingNoTxInstantSendLocks.begin(); it != pendingNoTxInstantSendLocks.end(); ++it) {
+        if (it->second.islock->txid != tx->GetHash()) continue;
+
+        // we received an islock earlier, let's put it back into pending and verify/lock
+        LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, islock=%s\n", __func__,
+                 tx->GetHash().ToString(), it->first.ToString());
+        auto islock = it->second.islock;
+        pendingInstantSendLocks.try_emplace(it->first, it->second);
+        pendingNoTxInstantSendLocks.erase(it);
+        return islock;
     }
-    return ret_islock; // not found, nullptr
+    return nullptr;
 }
 
 void CInstantSendManager::AddNonLockedTx(const CTransactionRef& tx, const CBlockIndex* pindexMined)
@@ -290,7 +287,7 @@ std::unordered_map<const CBlockIndex*, Uint256HashMap<CTransactionRef>> CInstant
     return conflicts;
 }
 
-bool CInstantSendManager::IsKnownTx(const uint256& islockHash) const
+bool CInstantSendManager::HasTxForLock(const uint256& islockHash) const
 {
     LOCK(cs_pendingLocks);
     return pendingNoTxInstantSendLocks.find(islockHash) == pendingNoTxInstantSendLocks.end();

src/instantsend/instantsend.h

@@ -104,7 +104,7 @@ class CInstantSendManager
     std::unordered_map<const CBlockIndex*, Uint256HashMap<CTransactionRef>> RetrieveISConflicts(
         const uint256& islockHash, const instantsend::InstantSendLock& islock)
         EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks);
-    bool IsKnownTx(const uint256& islockHash) const EXCLUSIVE_LOCKS_REQUIRED(!cs_pendingLocks);
+    bool HasTxForLock(const uint256& islockHash) const EXCLUSIVE_LOCKS_REQUIRED(!cs_pendingLocks);
 
     bool IsLocked(const uint256& txHash) const;
     bool IsWaitingForTx(const uint256& txHash) const EXCLUSIVE_LOCKS_REQUIRED(!cs_pendingLocks);

src/instantsend/net_instantsend.cpp

@@ -352,7 +352,7 @@ void NetInstantSend::ProcessPendingISLocks(std::vector<instantsend::PendingISLoc
 
 void NetInstantSend::ProcessInstantSendLock(NodeId from, const uint256& hash, const instantsend::InstantSendLockPtr& islock)
 {
-    LogPrint(BCLog::INSTANTSEND, "NetSigning::%s -- txid=%s, islock=%s: processing islock, peer=%d\n", __func__,
+    LogPrint(BCLog::INSTANTSEND, "NetInstantSend::%s -- txid=%s, islock=%s: processing islock, peer=%d\n", __func__,
              islock->txid.ToString(), hash.ToString(), from);
 
     if (m_signer) {
@@ -364,21 +364,13 @@ void NetInstantSend::ProcessInstantSendLock(NodeId from, const uint256& hash, co
     auto tx = GetTransaction(nullptr, &m_mempool, islock->txid, Params().GetConsensus(), hashBlock);
     const bool found_transaction{tx != nullptr};
     // we ignore failure here as we must be able to propagate the lock even if we don't have the TX locally
-    std::optional<int> minedHeight = GetBlockHeight(m_is_manager, m_chainstate, hashBlock);
+    const auto minedHeight = GetBlockHeight(m_is_manager, m_chainstate, hashBlock);
     if (found_transaction) {
-        if (!minedHeight.has_value()) {
-            const CBlockIndex* pindexMined = WITH_LOCK(::cs_main,
-                                                       return m_chainstate.m_blockman.LookupBlockIndex(hashBlock));
-            if (pindexMined != nullptr) {
-                m_is_manager.CacheBlockHeight(pindexMined);
-                minedHeight = pindexMined->nHeight;
-            }
-        }
         // Let's see if the TX that was locked by this islock is already mined in a ChainLocked block. If yes,
         // we can simply ignore the islock, as the ChainLock implies locking of all TXs in that chain
         if (minedHeight.has_value() && m_chainlocks.HasChainLock(*minedHeight, hashBlock)) {
             LogPrint(BCLog::INSTANTSEND, /* Continued */
-                     "NetSigning::%s -- txlock=%s, islock=%s: dropping islock as it already got a "
+                     "NetInstantSend::%s -- txlock=%s, islock=%s: dropping islock as it already got a "
                      "ChainLock in block %s, peer=%d\n",
                      __func__, islock->txid.ToString(), hash.ToString(), hashBlock.ToString(), from);
             return;
@@ -388,7 +380,6 @@ void NetInstantSend::ProcessInstantSendLock(NodeId from, const uint256& hash, co
         m_is_manager.AddPendingISLock(hash, islock, from);
     }
 
-
     // This will also add children TXs to pendingRetryTxs
     m_is_manager.RemoveNonLockedTx(islock->txid, true);
     // We don't need the recovered sigs for the inputs anymore. This prevents unnecessary propagation of these sigs.
@@ -398,8 +389,8 @@ void NetInstantSend::ProcessInstantSendLock(NodeId from, const uint256& hash, co
 
     if (found_transaction) {
         RemoveMempoolConflictsForLock(hash, *islock);
-        LogPrint(BCLog::INSTANTSEND, "NetSigning::%s -- notify about lock %s for tx %s\n", __func__, hash.ToString(),
-                 tx->GetHash().ToString());
+        LogPrint(BCLog::INSTANTSEND, "NetInstantSend::%s -- notify about lock %s for tx %s\n", __func__,
+                 hash.ToString(), tx->GetHash().ToString());
         GetMainSignals().NotifyTransactionLock(tx, islock);
         // bump m_mempool counter to make sure newly locked txes are picked up by getblocktemplate
         m_mempool.AddTransactionsUpdated(1);
@@ -588,7 +579,7 @@ void NetInstantSend::ResolveBlockConflicts(const uint256& islockHash, const inst
         return;
     }
 
-    bool isLockedTxKnown = m_is_manager.IsKnownTx(islockHash);
+    bool hasTxForLock = m_is_manager.HasTxForLock(islockHash);
 
     bool activateBestChain = false;
     for (const auto& p : conflicts) {
@@ -605,7 +596,7 @@ void NetInstantSend::ResolveBlockConflicts(const uint256& islockHash, const inst
             // This should not have happened and we are in a state were it's not safe to continue anymore
             assert(false);
         }
-        if (isLockedTxKnown) {
+        if (hasTxForLock) {
             activateBestChain = true;
         } else {
             LogPrintf("NetInstantSend::%s -- resetting block %s\n", __func__, pindex2->GetBlockHash().ToString());

src/instantsend/signing.cpp

@@ -206,19 +206,20 @@ bool InstantSendSigner::CheckCanLock(const COutPoint& outpoint, bool printDebug,
 
     int blockHeight{0};
     if (!hashBlock.IsNull()) {
-        auto ret = m_isman.GetCachedHeight(hashBlock);
-        if (ret) blockHeight = *ret;
-
-        const CBlockIndex* pindex = WITH_LOCK(::cs_main, return m_chainstate.m_blockman.LookupBlockIndex(hashBlock));
-        if (pindex == nullptr) {
-            if (printDebug) {
-                LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: failed to determine mined height for parent TX %s\n",
-                         __func__, txHash.ToString(), outpoint.hash.ToString());
+        if (auto ret = m_isman.GetCachedHeight(hashBlock)) {
+            blockHeight = *ret;
+        } else {
+            const CBlockIndex* pindex = WITH_LOCK(::cs_main, return m_chainstate.m_blockman.LookupBlockIndex(hashBlock));
+            if (pindex == nullptr) {
+                if (printDebug) {
+                    LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: failed to determine mined height for parent TX %s\n",
+                             __func__, txHash.ToString(), outpoint.hash.ToString());
+                }
+                return false;
             }
-            return false;
+            m_isman.CacheBlockHeight(pindex);
+            blockHeight = pindex->nHeight;
         }
-        m_isman.CacheBlockHeight(pindex);
-        blockHeight = pindex->nHeight;
     }
 
     const int tipHeight = m_isman.GetTipHeight();