Merge dashpay#6770: refactor: follow-up dash#6742, implement review suggestions, avoid redundant transaction queries

f488c43 fix: remove unnecessary tx fetching in `RemoveMempoolConflictsForLock` (Kittywhiskers Van Gogh)
dd54011 refactor: move inexpensive checks earlier in `ProcessInstantSendLock()` (Kittywhiskers Van Gogh)
c86d886 refactor: abstract away InstantSend parent implementation from signer (Kittywhiskers Van Gogh)
cbfd325 refactor: avoid shared_ptr copies in `{WriteNew,Remove}InstantSendLock()` (Kittywhiskers Van Gogh)
234a16a refactor: use unordered set for faster duplicates checking (Kittywhiskers Van Gogh)
0459848 refactor: `constexpr` static string definitions (Kittywhiskers Van Gogh)
710c504 refactor: s/cs_inputReqests/cs_input_requests/g (Kittywhiskers Van Gogh)
31065d1 chore: mark `hashBlock` as unused in `HandleNewInputLockRecoveredSig()` (Kittywhiskers Van Gogh)
cf47f14 chore: apply most `clang-format` suggestions (Kittywhiskers Van Gogh)
63371e0 trivial: move forward declaration above `using` declarations (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Dependent on dashpay#6742

  * This pull request aims to address reviewer concerns raised during [dash#6742](dashpay#6742), the following changes were made with that in mind
    * Marking unused variable `hashBlock` by prefixing with underscore, [comment](dashpay#6742 (comment))
    * Correct typo in variable name `cs_inputReqests` (now `cs_input_requests`, [comment](dashpay#6770 (comment))), [comment](dashpay#6742 (comment))
    * Using `constexpr` for `std::string_view`s, [comment](dashpay#6742 (comment))
    * Utilizing a faster way to validate uniqueness of entries (`std::unordered_set`), [comment](dashpay#6742 (comment)). The construction of the unordered set is inspired by similar usage in `policy/package.cpp` ([source](https://github.com/dashpay/dash/blob/f648039258d2192ec5e505721e34edaa0bda568c/src/policy/packages.cpp#L51)).
    * Avoiding unnecessary `std::shared_ptr` copying (`InstantSendLockPtr` is a `std::shared_ptr<InstantSendLock>`), [comment](dashpay#6742 (comment))
    * Resolving circular dependency by cherry-picking 1e2e854, [comment](dashpay#6742 (comment)) but has been renamed to `InstantSendSignerParent`. For rationale, see [comment](dashpay#6761 (comment)).

  * Additionally, optimizations have been made to `ProcessInstantSendLock()`
    * The _cheaper_ database checks have been moved _earlier_ in the function to allow for faster bail-out. This is alongside consolidating network requests to the tail of the function and general cleanup for better clarity.

    * Dropping the peer request for a transaction we know about in `RemoveMempoolConflictsForLock()` (introduced in [dash#2898](dashpay#2898)) as `ProcessInstantSendLock()` calls it when we _know_ about the transaction and seek to remove conflicts ([source](https://github.com/dashpay/dash/blob/f648039258d2192ec5e505721e34edaa0bda568c/src/instantsend/instantsend.cpp#L419-L420))

      The only other caller, `TransactionAddedToMempool()` calls it when we have knowledge of the transaction and want to purge conflicts from the mempool ([source](https://github.com/dashpay/dash/blob/f648039258d2192ec5e505721e34edaa0bda568c/src/instantsend/instantsend.cpp#L455-L463)).

      In both cases, we already know the _good_ transaction. Additionally, tests introduced in [dash#2898](dashpay#2898) still pass ([source](https://github.com/dashpay/dash/blob/f648039258d2192ec5e505721e34edaa0bda568c/test/functional/p2p_instantsend.py#L95-L142)), indicating a lack of regression but this change may require additional scrutiny.

  ## Breaking Changes

  None expected.

  ## Checklist

  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)**
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  PastaPastaPasta:
    utACK f488c43
  knst:
    utACK f488c43
  UdjinM6:
    utACK f488c43

Tree-SHA512: dcd153e14747639f8dbbbd5c3bea1123e4f84f82884787992e20f494227913763bf19a73b485e345dddb63ad551c856bc56dcad72e67304f84df60cb76deaa74

Author: PastaPastaPasta

src/dsnotificationinterface.cpp

@@ -104,7 +104,7 @@ void CDSNotificationInterface::TransactionAddedToMempool(const CTransactionRef&
 {
     assert(m_cj_ctx && m_llmq_ctx);
 
-    m_llmq_ctx->isman->TransactionAddedToMempool(m_peerman, ptx);
+    m_llmq_ctx->isman->TransactionAddedToMempool(ptx);
     m_llmq_ctx->clhandler->TransactionAddedToMempool(ptx, nAcceptTime);
     m_cj_ctx->dstxman->TransactionAddedToMempool(ptx);
 }

src/instantsend/db.cpp

@@ -9,17 +9,18 @@
 #include <primitives/block.h>
 #include <util/system.h>
 
-static const std::string_view DB_ARCHIVED_BY_HASH = "is_a2";
-static const std::string_view DB_ARCHIVED_BY_HEIGHT_AND_HASH = "is_a1";
-static const std::string_view DB_HASH_BY_OUTPOINT = "is_in";
-static const std::string_view DB_HASH_BY_TXID = "is_tx";
-static const std::string_view DB_ISLOCK_BY_HASH = "is_i";
-static const std::string_view DB_MINED_BY_HEIGHT_AND_HASH = "is_m";
-static const std::string_view DB_VERSION = "is_v";
+static constexpr std::string_view DB_ARCHIVED_BY_HASH{"is_a2"};
+static constexpr std::string_view DB_ARCHIVED_BY_HEIGHT_AND_HASH{"is_a1"};
+static constexpr std::string_view DB_HASH_BY_OUTPOINT{"is_in"};
+static constexpr std::string_view DB_HASH_BY_TXID{"is_tx"};
+static constexpr std::string_view DB_ISLOCK_BY_HASH{"is_i"};
+static constexpr std::string_view DB_MINED_BY_HEIGHT_AND_HASH{"is_m"};
+static constexpr std::string_view DB_VERSION{"is_v"};
 
 namespace instantsend {
 namespace {
-static std::tuple<std::string, uint32_t, uint256> BuildInversedISLockKey(std::string_view k, int nHeight, const uint256& islockHash)
+static std::tuple<std::string, uint32_t, uint256> BuildInversedISLockKey(std::string_view k, int nHeight,
+                                                                         const uint256& islockHash)
 {
     return std::make_tuple(std::string{k}, htobe32_internal(std::numeric_limits<uint32_t>::max() - nHeight), islockHash);
 }
@@ -50,44 +51,39 @@ void CInstantSendDb::Upgrade(bool unitTests)
     }
 }
 
-void CInstantSendDb::WriteNewInstantSendLock(const uint256& hash, const InstantSendLock& islock)
+void CInstantSendDb::WriteNewInstantSendLock(const uint256& hash, const InstantSendLockPtr& islock)
 {
     LOCK(cs_db);
     CDBBatch batch(*db);
-    batch.Write(std::make_tuple(DB_ISLOCK_BY_HASH, hash), islock);
-    batch.Write(std::make_tuple(DB_HASH_BY_TXID, islock.txid), hash);
-    for (const auto& in : islock.inputs) {
+    batch.Write(std::make_tuple(DB_ISLOCK_BY_HASH, hash), *islock);
+    batch.Write(std::make_tuple(DB_HASH_BY_TXID, islock->txid), hash);
+    for (const auto& in : islock->inputs) {
         batch.Write(std::make_tuple(DB_HASH_BY_OUTPOINT, in), hash);
     }
     db->WriteBatch(batch);
 
-    islockCache.insert(hash, std::make_shared<InstantSendLock>(islock));
-    txidCache.insert(islock.txid, hash);
-    for (const auto& in : islock.inputs) {
+    islockCache.insert(hash, islock);
+    txidCache.insert(islock->txid, hash);
+    for (const auto& in : islock->inputs) {
         outpointCache.insert(in, hash);
     }
 }
 
-void CInstantSendDb::RemoveInstantSendLock(CDBBatch& batch, const uint256& hash, InstantSendLockPtr islock, bool keep_cache)
+void CInstantSendDb::RemoveInstantSendLock(CDBBatch& batch, const uint256& hash, const InstantSendLock& islock,
+                                           bool keep_cache)
 {
     AssertLockHeld(cs_db);
-    if (!islock) {
-        islock = GetInstantSendLockByHashInternal(hash, false);
-        if (!islock) {
-            return;
-        }
-    }
 
     batch.Erase(std::make_tuple(DB_ISLOCK_BY_HASH, hash));
-    batch.Erase(std::make_tuple(DB_HASH_BY_TXID, islock->txid));
-    for (auto& in : islock->inputs) {
+    batch.Erase(std::make_tuple(DB_HASH_BY_TXID, islock.txid));
+    for (auto& in : islock.inputs) {
         batch.Erase(std::make_tuple(DB_HASH_BY_OUTPOINT, in));
     }
 
     if (!keep_cache) {
         islockCache.erase(hash);
-        txidCache.erase(islock->txid);
-        for (const auto& in : islock->inputs) {
+        txidCache.erase(islock.txid);
+        for (const auto& in : islock.inputs) {
             outpointCache.erase(in);
         }
     }
@@ -151,7 +147,7 @@ std::unordered_map<uint256, InstantSendLockPtr, StaticSaltedHasher> CInstantSend
         auto& islockHash = std::get<2>(curKey);
 
         if (auto islock = GetInstantSendLockByHashInternal(islockHash, false)) {
-            RemoveInstantSendLock(batch, islockHash, islock);
+            RemoveInstantSendLock(batch, islockHash, *islock);
             ret.try_emplace(islockHash, std::move(islock));
         }
 
@@ -221,7 +217,8 @@ void CInstantSendDb::WriteBlockInstantSendLocks(const gsl::not_null<std::shared_
     db->WriteBatch(batch);
 }
 
-void CInstantSendDb::RemoveBlockInstantSendLocks(const gsl::not_null<std::shared_ptr<const CBlock>>& pblock, gsl::not_null<const CBlockIndex*> pindexDisconnected)
+void CInstantSendDb::RemoveBlockInstantSendLocks(const gsl::not_null<std::shared_ptr<const CBlock>>& pblock,
+                                                 gsl::not_null<const CBlockIndex*> pindexDisconnected)
 {
     LOCK(cs_db);
     CDBBatch batch(*db);
@@ -241,7 +238,8 @@ void CInstantSendDb::RemoveBlockInstantSendLocks(const gsl::not_null<std::shared
 bool CInstantSendDb::KnownInstantSendLock(const uint256& islockHash) const
 {
     LOCK(cs_db);
-    return GetInstantSendLockByHashInternal(islockHash) != nullptr || db->Exists(std::make_tuple(DB_ARCHIVED_BY_HASH, islockHash));
+    return GetInstantSendLockByHashInternal(islockHash) != nullptr ||
+           db->Exists(std::make_tuple(DB_ARCHIVED_BY_HASH, islockHash));
 }
 
 size_t CInstantSendDb::GetInstantSendLockCount() const
@@ -354,7 +352,8 @@ std::vector<uint256> CInstantSendDb::GetInstantSendLocksByParent(const uint256&
     return result;
 }
 
-std::vector<uint256> CInstantSendDb::RemoveChainedInstantSendLocks(const uint256& islockHash, const uint256& txid, int nHeight)
+std::vector<uint256> CInstantSendDb::RemoveChainedInstantSendLocks(const uint256& islockHash, const uint256& txid,
+                                                                   int nHeight)
 {
     LOCK(cs_db);
     std::vector<uint256> result;
@@ -374,7 +373,7 @@ std::vector<uint256> CInstantSendDb::RemoveChainedInstantSendLocks(const uint256
                 continue;
             }
 
-            RemoveInstantSendLock(batch, childIslockHash, childIsLock, false);
+            RemoveInstantSendLock(batch, childIslockHash, *childIsLock, false);
             WriteInstantSendLockArchived(batch, childIslockHash, nHeight);
             result.emplace_back(childIslockHash);
 
@@ -384,7 +383,9 @@ std::vector<uint256> CInstantSendDb::RemoveChainedInstantSendLocks(const uint256
         }
     }
 
-    RemoveInstantSendLock(batch, islockHash, nullptr, false);
+    if (auto islock = GetInstantSendLockByHashInternal(islockHash, /*use_cache=*/false)) {
+        RemoveInstantSendLock(batch, islockHash, *islock, false);
+    }
     WriteInstantSendLockArchived(batch, islockHash, nHeight);
     result.emplace_back(islockHash);
 

src/instantsend/db.h

@@ -33,9 +33,9 @@ class CInstantSendDb
 
     static constexpr int CURRENT_VERSION{1};
 
-    int best_confirmed_height GUARDED_BY(cs_db) {0};
+    int best_confirmed_height GUARDED_BY(cs_db){0};
 
-    std::unique_ptr<CDBWrapper> db GUARDED_BY(cs_db) {nullptr};
+    std::unique_ptr<CDBWrapper> db GUARDED_BY(cs_db){nullptr};
     mutable unordered_lru_cache<uint256, InstantSendLockPtr, StaticSaltedHasher, 10000> islockCache GUARDED_BY(cs_db);
     mutable unordered_lru_cache<uint256, uint256, StaticSaltedHasher, 10000> txidCache GUARDED_BY(cs_db);
 
@@ -51,7 +51,8 @@ class CInstantSendDb
      * @param islock The InstantSend Lock object itself
      * @param keep_cache Should we still keep corresponding entries in the cache or not
      */
-    void RemoveInstantSendLock(CDBBatch& batch, const uint256& hash, InstantSendLockPtr islock, bool keep_cache = true) EXCLUSIVE_LOCKS_REQUIRED(cs_db);
+    void RemoveInstantSendLock(CDBBatch& batch, const uint256& hash, const InstantSendLock& islock,
+                               bool keep_cache = true) EXCLUSIVE_LOCKS_REQUIRED(cs_db);
     /**
      * Marks an InstantSend Lock as archived.
      * @param batch Object used to batch many calls together
@@ -88,7 +89,7 @@ class CInstantSendDb
      * @param hash The hash of the InstantSend Lock
      * @param islock The InstantSend Lock object itself
      */
-    void WriteNewInstantSendLock(const uint256& hash, const InstantSendLock& islock) EXCLUSIVE_LOCKS_REQUIRED(!cs_db);
+    void WriteNewInstantSendLock(const uint256& hash, const InstantSendLockPtr& islock) EXCLUSIVE_LOCKS_REQUIRED(!cs_db);
     /**
      * This method updates a DB entry for an InstantSend Lock from being not included in a block to being included in a block
      * @param hash The hash of the InstantSend Lock