Merge #6953: feat: instantsend cache heights and reduce cs_main contention

e918729 refactor: add CacheTipHeight, improve cache API safety (UdjinM6)
493d077 refactor: simplify optional usage (pasta)
0e1f114 instantsend: cache heights and reduce cs_main contention (pasta)

Pull request description:

  ## Issue being fixed or feature implemented
  Today's testing on testnet showed under load, cs_main was a major contender:

  (however, note the major contenders from before #6912 / #6468 are now significantly reduced)

  ```
  ========================================================================================================================
  TOP 40 LOCATIONS BY TOTAL CONTENTION TIME (AGGREGATE)
  ========================================================================================================================
  Lock Name                                Location                               Count    Total(μs)    Avg(μs)    Max(μs)  Nodes
  ------------------------------------------------------------------------------------------------------------------------
  cs                                       llmq/signing_shares.cpp:507            11952     81357095     6807.0     299734      8
  ::cs_main                                instantsend/signing.cpp:210            75241     62985844      837.1      93240      8
  cs_main                                  index/base.cpp:340                     29584     27040349      914.0    1613365      8
  cs_main                                  net_processing.cpp:5462                69291     19784473      285.5    1329191      8
  cs                                       llmq/signing_shares.cpp:1732            2355     17466035     7416.6     127471      8
  ::cs_main                                llmq/quorums.cpp:1214                  38975     15478543      397.1     883884      8
  m_nodes_mutex                            net.cpp:2043                           33457     14736059      440.4      31622      8
  ::cs_main                                llmq/quorums.cpp:526                   10587     10577503      999.1    2083501      8
  pnode->cs_vSend                          net.cpp:2435                           68528      9671050      141.1      37265      8
  cs_main                                  net_processing.cpp:4196                 4945      8608157     1740.8    1326146      8
  ::cs_main                                instantsend/instantsend.cpp:272         5602      8416269     1502.4    1260997      8
  cs                                       txmempool.cpp:1319                      7942      8059684     1014.8     356199      8
  ::cs_main                                validation.cpp:3747                      309      6468144    20932.5    1225274      8
  ::cs_main                                validation.cpp:6009                    21855      3393152      155.3     228195      8
  pnode->cs_vSend                          net.cpp:4709                            7532      2299813      305.3      41543      8
  m_nodes_mutex                            ./net.h:1374                            6229      1998506      320.8      15593      8
  inv_relay->m_tx_inventory_mutex          net_processing.cpp:1169                 7687      1871859      243.5      16292      8
  cs_db                                    instantsend/db.cpp:239                  4323      1527297      353.3      20779      8
  cs_cache                                 spork.cpp:244                          18083      1331472       73.6      27722      8
  ::cs_main                                chainlock/signing.cpp:58                  98      1312496    13392.8    1273691      8
  cs_db                                    ./instantsend/db.h:139                  4381      1305738      298.0      29777      8
  mutexMsgProc                             net.cpp:2604                            4877      1243737      255.0      24641      8
  ```

  - Add unordered_lru_cache for block heights in CInstantSendManager
  - Provide GetTipHeight() (non-optional) and GetBlockHeight() helpers
  - Use cache in InstantSendSigner::CheckCanLock
  - Cache-first for cycleHash height in ISLOCK message path and batch verify
  - Cache-first mined-height for HasChainLock and WriteInstantSendLockMined

  ## What was done?

  ## How Has This Been Tested?

  ## Breaking Changes
    _Please describe any breaking changes your code introduces_

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [ ] I have performed a self-review of my own code
  - [ ] 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
  - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK e918729

Tree-SHA512: 1ea45e92c717dbefe5625bba3b27612b9ffbb21ea106d11005d78836fbe6ce00503e3f3fdb927fb405b09fd795f50137ca2beb07351bf94488d2af5694e543ba

Author: PastaPastaPasta

src/instantsend/instantsend.cpp

@@ -145,25 +145,32 @@ std::variant<uint256, CTransactionRef, std::monostate> CInstantSendManager::Proc
 
     uint256 hashBlock{};
     const auto tx = GetTransaction(nullptr, &mempool, islock->txid, Params().GetConsensus(), hashBlock);
-    const CBlockIndex* pindexMined{nullptr};
     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
-    if (found_transaction && !hashBlock.IsNull()) {
-        pindexMined = WITH_LOCK(::cs_main, return m_chainstate.m_blockman.LookupBlockIndex(hashBlock));
-
+    std::optional<int> minedHeight = GetBlockHeight(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) {
+                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 (pindexMined != nullptr && clhandler.HasChainLock(pindexMined->nHeight, pindexMined->GetBlockHash())) {
-            LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%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);
+        if (minedHeight.has_value() && clhandler.HasChainLock(*minedHeight, hashBlock)) {
+            LogPrint(BCLog::INSTANTSEND, /* Continued */
+                     "CInstantSendManager::%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 std::monostate{};
         }
     }
 
     if (found_transaction) {
         db.WriteNewInstantSendLock(hash, islock);
-        if (pindexMined) {
-            db.WriteInstantSendLockMined(hash, pindexMined->nHeight);
+        if (minedHeight.has_value()) {
+            db.WriteInstantSendLockMined(hash, *minedHeight);
         }
     } else {
         // put it in a separate pending map and try again later
@@ -251,6 +258,8 @@ void CInstantSendManager::BlockConnected(const std::shared_ptr<const CBlock>& pb
         return;
     }
 
+    CacheTipHeight(pindex);
+
     if (m_mn_sync.IsBlockchainSynced()) {
         const bool has_chainlock = clhandler.HasChainLock(pindex->nHeight, pindex->GetBlockHash());
         for (const auto& tx : pblock->vtx) {
@@ -278,6 +287,13 @@ void CInstantSendManager::BlockConnected(const std::shared_ptr<const CBlock>& pb
 void CInstantSendManager::BlockDisconnected(const std::shared_ptr<const CBlock>& pblock,
                                             const CBlockIndex* pindexDisconnected)
 {
+    {
+        LOCK(cs_height_cache);
+        m_cached_block_heights.erase(pindexDisconnected->GetBlockHash());
+    }
+
+    CacheTipHeight(pindexDisconnected->pprev);
+
     db.RemoveBlockInstantSendLocks(pblock, pindexDisconnected);
 }
 
@@ -417,6 +433,8 @@ void CInstantSendManager::NotifyChainLock(const CBlockIndex* pindexChainLock)
 
 void CInstantSendManager::UpdatedBlockTip(const CBlockIndex* pindexNew)
 {
+    CacheTipHeight(pindexNew);
+
     bool fDIP0008Active = pindexNew->pprev && pindexNew->pprev->nHeight >= Params().GetConsensus().DIP0008Height;
 
     if (AreChainLocksEnabled(spork_manager) && fDIP0008Active) {
@@ -597,7 +615,7 @@ void CInstantSendManager::RemoveConflictingLock(const uint256& islockHash, const
 {
     LogPrintf("CInstantSendManager::%s -- txid=%s, islock=%s: Removing ISLOCK and its chained children\n", __func__,
               islock.txid.ToString(), islockHash.ToString());
-    int tipHeight = WITH_LOCK(::cs_main, return m_chainstate.m_chain.Height());
+    const int tipHeight = GetTipHeight();
 
     auto removedIslocks = db.RemoveChainedInstantSendLocks(islockHash, islock.txid, tipHeight);
     for (const auto& h : removedIslocks) {
@@ -703,6 +721,64 @@ size_t CInstantSendManager::GetInstantSendLockCount() const
     return db.GetInstantSendLockCount();
 }
 
+void CInstantSendManager::CacheBlockHeightInternal(const CBlockIndex* const block_index) const
+{
+    AssertLockHeld(cs_height_cache);
+    m_cached_block_heights.insert(block_index->GetBlockHash(), block_index->nHeight);
+}
+
+void CInstantSendManager::CacheBlockHeight(const CBlockIndex* const block_index) const
+{
+    LOCK(cs_height_cache);
+    CacheBlockHeightInternal(block_index);
+}
+
+std::optional<int> CInstantSendManager::GetBlockHeight(const uint256& hash) const
+{
+    if (hash.IsNull()) {
+        return std::nullopt;
+    }
+    {
+        LOCK(cs_height_cache);
+        int cached_height{0};
+        if (m_cached_block_heights.get(hash, cached_height)) return cached_height;
+    }
+
+    const CBlockIndex* pindex = WITH_LOCK(::cs_main, return m_chainstate.m_blockman.LookupBlockIndex(hash));
+    if (pindex == nullptr) {
+        return std::nullopt;
+    }
+
+    CacheBlockHeight(pindex);
+    return pindex->nHeight;
+}
+
+void CInstantSendManager::CacheTipHeight(const CBlockIndex* const tip) const
+{
+    LOCK(cs_height_cache);
+    if (tip) {
+        CacheBlockHeightInternal(tip);
+        m_cached_tip_height = tip->nHeight;
+    } else {
+        m_cached_tip_height = -1;
+    }
+}
+
+int CInstantSendManager::GetTipHeight() const
+{
+    {
+        LOCK(cs_height_cache);
+        if (m_cached_tip_height >= 0) {
+            return m_cached_tip_height;
+        }
+    }
+
+    const CBlockIndex* tip = WITH_LOCK(::cs_main, return m_chainstate.m_chain.Tip());
+
+    CacheTipHeight(tip);
+    return tip ? tip->nHeight : -1;
+}
+
 bool CInstantSendManager::IsInstantSendEnabled() const
 {
     return !fReindex && !fImporting && spork_manager.IsSporkActive(SPORK_2_INSTANTSEND_ENABLED);

src/instantsend/instantsend.h

@@ -17,11 +17,13 @@
 #include <unordered_lru_cache.h>
 
 #include <atomic>
-#include <unordered_map>
+#include <optional>
 #include <unordered_set>
 #include <variant>
 #include <vector>
 
+#include <saltedhasher.h>
+
 class CBlockIndex;
 class CChainState;
 class CDataStream;
@@ -91,6 +93,14 @@ class CInstantSendManager final : public instantsend::InstantSendSignerParent
     mutable Mutex cs_timingsTxSeen;
     Uint256HashMap<int64_t> timingsTxSeen GUARDED_BY(cs_timingsTxSeen);
 
+    mutable Mutex cs_height_cache;
+    static constexpr size_t MAX_BLOCK_HEIGHT_CACHE{16384};
+    mutable unordered_lru_cache<uint256, int, StaticSaltedHasher, MAX_BLOCK_HEIGHT_CACHE> m_cached_block_heights
+        GUARDED_BY(cs_height_cache);
+    mutable int m_cached_tip_height GUARDED_BY(cs_height_cache){-1};
+
+    void CacheBlockHeightInternal(const CBlockIndex* const block_index) const EXCLUSIVE_LOCKS_REQUIRED(cs_height_cache);
+
 public:
     CInstantSendManager() = delete;
     CInstantSendManager(const CInstantSendManager&) = delete;
@@ -122,7 +132,7 @@ class CInstantSendManager final : public instantsend::InstantSendSignerParent
     void RemoveMempoolConflictsForLock(const uint256& hash, const instantsend::InstantSendLock& islock)
         EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingRetry);
     void ResolveBlockConflicts(const uint256& islockHash, const instantsend::InstantSendLock& islock)
-        EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks, !cs_pendingRetry);
+        EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks, !cs_pendingRetry, !cs_height_cache);
 
     void HandleFullyConfirmedBlock(const CBlockIndex* pindex)
         EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingRetry);
@@ -142,14 +152,15 @@ class CInstantSendManager final : public instantsend::InstantSendSignerParent
     CSigningManager& Sigman() { return sigman; }
     [[nodiscard]] std::variant<uint256, CTransactionRef, std::monostate> ProcessInstantSendLock(
         NodeId from, const uint256& hash, const instantsend::InstantSendLockPtr& islock)
-        EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks, !cs_pendingRetry);
+        EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks, !cs_pendingRetry, !cs_height_cache);
 
     void TransactionAddedToMempool(const CTransactionRef& tx)
         EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks, !cs_pendingRetry, !cs_timingsTxSeen);
-    void TransactionRemovedFromMempool(const CTransactionRef& tx);
+    void TransactionRemovedFromMempool(const CTransactionRef& tx) EXCLUSIVE_LOCKS_REQUIRED(!cs_height_cache);
     void BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindex)
-        EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks, !cs_pendingRetry, !cs_timingsTxSeen);
-    void BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexDisconnected);
+        EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks, !cs_pendingRetry, !cs_timingsTxSeen, !cs_height_cache);
+    void BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexDisconnected)
+        EXCLUSIVE_LOCKS_REQUIRED(!cs_height_cache);
 
     bool AlreadyHave(const CInv& inv) const EXCLUSIVE_LOCKS_REQUIRED(!cs_pendingLocks);
     bool GetInstantSendLockByHash(const uint256& hash, instantsend::InstantSendLock& ret) const
@@ -159,14 +170,20 @@ class CInstantSendManager final : public instantsend::InstantSendSignerParent
     void NotifyChainLock(const CBlockIndex* pindexChainLock)
         EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingRetry);
     void UpdatedBlockTip(const CBlockIndex* pindexNew)
-        EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingRetry);
+        EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingRetry, !cs_height_cache);
 
-    void RemoveConflictingLock(const uint256& islockHash, const instantsend::InstantSendLock& islock);
+    void RemoveConflictingLock(const uint256& islockHash, const instantsend::InstantSendLock& islock)
+        EXCLUSIVE_LOCKS_REQUIRED(!cs_height_cache);
     void TryEmplacePendingLock(const uint256& hash, const NodeId id, const instantsend::InstantSendLockPtr& islock) override
         EXCLUSIVE_LOCKS_REQUIRED(!cs_pendingLocks);
 
     size_t GetInstantSendLockCount() const;
 
+    void CacheBlockHeight(const CBlockIndex* const block_index) const EXCLUSIVE_LOCKS_REQUIRED(!cs_height_cache);
+    std::optional<int> GetBlockHeight(const uint256& hash) const override EXCLUSIVE_LOCKS_REQUIRED(!cs_height_cache);
+    void CacheTipHeight(const CBlockIndex* const tip) const EXCLUSIVE_LOCKS_REQUIRED(!cs_height_cache);
+    int GetTipHeight() const override EXCLUSIVE_LOCKS_REQUIRED(!cs_height_cache);
+
     bool IsInstantSendEnabled() const override;
     /**
      * If true, MN should sign all transactions, if false, MN should not sign

src/instantsend/net_instantsend.cpp

@@ -34,18 +34,18 @@ void NetInstantSend::ProcessMessage(CNode& pfrom, const std::string& msg_type, C
         return;
     }
 
-    int block_height = [this, islock] {
+    auto cycleHeightOpt = m_is_manager.GetBlockHeight(islock->cycleHash);
+    if (!cycleHeightOpt) {
         const auto blockIndex = WITH_LOCK(::cs_main, return m_chainstate.m_blockman.LookupBlockIndex(islock->cycleHash));
         if (blockIndex == nullptr) {
-            return -1;
+            // Maybe we don't have the block yet or maybe some peer spams invalid values for cycleHash
+            m_peer_manager->PeerMisbehaving(pfrom.GetId(), 1);
+            return;
         }
-        return blockIndex->nHeight;
-    }();
-    if (block_height < 0) {
-        // Maybe we don't have the block yet or maybe some peer spams invalid values for cycleHash
-        m_peer_manager->PeerMisbehaving(pfrom.GetId(), 1);
-        return;
+        m_is_manager.CacheBlockHeight(blockIndex);
+        cycleHeightOpt = blockIndex->nHeight;
     }
+    const int block_height = *cycleHeightOpt;
 
     // Deterministic islocks MUST use rotation based llmq
     auto llmqType = Params().GetConsensus().llmqTypeDIP0024InstantSend;
@@ -125,16 +125,18 @@ Uint256HashSet NetInstantSend::ProcessPendingInstantSendLocks(
             continue;
         }
 
-        const auto blockIndex = WITH_LOCK(::cs_main, return m_chainstate.m_blockman.LookupBlockIndex(islock->cycleHash));
-        if (blockIndex == nullptr) {
+        auto cycleHeightOpt = m_is_manager.GetBlockHeight(islock->cycleHash);
+        if (!cycleHeightOpt) {
             batchVerifier.badSources.emplace(nodeId);
             continue;
         }
 
         int nSignHeight{-1};
         const auto dkgInterval = llmq_params.dkgInterval;
-        if (blockIndex->nHeight + dkgInterval < m_chainstate.m_chain.Height()) {
-            nSignHeight = blockIndex->nHeight + dkgInterval - 1;
+        const int tipHeight = m_is_manager.GetTipHeight();
+        const int cycleHeight = *cycleHeightOpt;
+        if (cycleHeight + dkgInterval < tipHeight) {
+            nSignHeight = cycleHeight + dkgInterval - 1;
         }
         // For RegTest non-rotating quorum cycleHash has directly quorum hash
         auto quorum = llmq_params.useRotation ? llmq::SelectQuorumForSigning(llmq_params, m_chainstate.m_chain, m_qman,

src/instantsend/signing.cpp

@@ -204,16 +204,28 @@ bool InstantSendSigner::CheckCanLock(const COutPoint& outpoint, bool printDebug,
         return false;
     }
 
-    const CBlockIndex* pindexMined;
-    int nTxAge;
-    {
-        LOCK(::cs_main);
-        pindexMined = m_chainstate.m_blockman.LookupBlockIndex(hashBlock);
-        nTxAge = m_chainstate.m_chain.Height() - pindexMined->nHeight + 1;
+    const auto blockHeight = m_isman.GetBlockHeight(hashBlock);
+    if (!blockHeight) {
+        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;
+    }
+
+    const int tipHeight = m_isman.GetTipHeight();
+
+    if (tipHeight < *blockHeight) {
+        if (printDebug) {
+            LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: cached tip height %d is below block height %d for parent TX %s\n",
+                     __func__, txHash.ToString(), tipHeight, *blockHeight, outpoint.hash.ToString());
+        }
+        return false;
     }
 
-    if (nTxAge < nInstantSendConfirmationsRequired &&
-        !m_clhandler.HasChainLock(pindexMined->nHeight, pindexMined->GetBlockHash())) {
+    const int nTxAge = tipHeight - *blockHeight + 1;
+
+    if (nTxAge < nInstantSendConfirmationsRequired && !m_clhandler.HasChainLock(*blockHeight, hashBlock)) {
         if (printDebug) {
             LogPrint(BCLog::INSTANTSEND, "%s -- txid=%s: outpoint %s too new and not ChainLocked. nTxAge=%d, nInstantSendConfirmationsRequired=%d\n", __func__,
                      txHash.ToString(), outpoint.ToStringShort(), nTxAge, nInstantSendConfirmationsRequired);

src/instantsend/signing.h

@@ -8,6 +8,8 @@
 #include <instantsend/lock.h>
 #include <llmq/signing.h>
 
+#include <optional>
+
 class CMasternodeSync;
 class CSporkManager;
 class CTxMemPool;
@@ -34,6 +36,8 @@ class InstantSendSignerParent
     virtual bool IsLocked(const uint256& txHash) const = 0;
     virtual InstantSendLockPtr GetConflictingLock(const CTransaction& tx) const = 0;
     virtual void TryEmplacePendingLock(const uint256& hash, const NodeId id, const InstantSendLockPtr& islock) = 0;
+    virtual std::optional<int> GetBlockHeight(const uint256& hash) const = 0;
+    virtual int GetTipHeight() const = 0;
 };
 
 class InstantSendSigner final : public llmq::CRecoveredSigsListener