instantsend: Remove islocks for rejected/removed txes (#4155)

* instantsend: Resolve block conflicts first and take care of mempool ones later

* refactor: Rename RemoveChainLockConflictingLock -> RemoveConflictingLock

* instantsend: Handle transaction removal from mempool (for all reasons besides inclusion in blocks)

* instantsend: Remove old islocks with no known txes from db (once)

* refactor: Replace magic number with CURRENT_VERSION

* fix: Do not remove islocks for (yet) valid orphans

* Apply suggestions from code review

Co-authored-by: dustinface <35775977+xdustinface@users.noreply.github.com>

Co-authored-by: dustinface <35775977+xdustinface@users.noreply.github.com>

src/dsnotificationinterface.cpp

@@ -83,6 +83,11 @@ void CDSNotificationInterface::TransactionAddedToMempool(const CTransactionRef&
     CCoinJoin::TransactionAddedToMempool(ptx);
 }
 
+void CDSNotificationInterface::TransactionRemovedFromMempool(const CTransactionRef& ptx)
+{
+    llmq::quorumInstantSendManager->TransactionRemovedFromMempool(ptx);
+}
+
 void CDSNotificationInterface::BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindex, const std::vector<CTransactionRef>& vtxConflicted)
 {
     // TODO: Tempoarily ensure that mempool removals are notified before

src/dsnotificationinterface.h

@@ -23,6 +23,7 @@ protected:
     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) override;
+    void TransactionRemovedFromMempool(const CTransactionRef& ptx) override;
     void BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindex, const std::vector<CTransactionRef>& vtxConflicted) 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;

src/llmq/quorums_instantsend.cpp

@@ -31,6 +31,10 @@ static const std::string DB_MINED_BY_HEIGHT_AND_HASH = "is_m";
 static const std::string DB_ARCHIVED_BY_HEIGHT_AND_HASH = "is_a1";
 static const std::string DB_ARCHIVED_BY_HASH = "is_a2";
 
+static const std::string DB_VERSION = "is_v";
+
+const int CInstantSendDb::CURRENT_VERSION;
+
 CInstantSendManager* quorumInstantSendManager;
 
 uint256 CInstantSendLock::GetRequestId() const
@@ -43,6 +47,46 @@ uint256 CInstantSendLock::GetRequestId() const
 
 ////////////////
 
+CInstantSendDb::CInstantSendDb(CDBWrapper& _db) : db(_db)
+{
+    Upgrade();
+}
+
+void CInstantSendDb::Upgrade()
+{
+    int v{0};
+    if (!db.Read(DB_VERSION, v) || v < CInstantSendDb::CURRENT_VERSION) {
+        CDBBatch batch(db);
+        CInstantSendLock islock;
+        CTransactionRef tx;
+        uint256 hashBlock;
+
+        auto it = std::unique_ptr<CDBIterator>(db.NewIterator());
+        auto firstKey = std::make_tuple(DB_ISLOCK_BY_HASH, uint256());
+        it->Seek(firstKey);
+        decltype(firstKey) curKey;
+
+        while (it->Valid()) {
+            if (!it->GetKey(curKey) || std::get<0>(curKey) != DB_ISLOCK_BY_HASH) {
+                break;
+            }
+            if (it->GetValue(islock)) {
+                if (!GetTransaction(islock.txid, tx, Params().GetConsensus(), hashBlock)) {
+                    // Drop locks for unknown txes
+                    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));
+                    }
+                    batch.Erase(curKey);
+                }
+            }
+            it->Next();
+        }
+        batch.Write(DB_VERSION, CInstantSendDb::CURRENT_VERSION);
+        db.WriteBatch(batch);
+    }
+}
+
 void CInstantSendDb::WriteNewInstantSendLock(const uint256& hash, const CInstantSendLock& islock)
 {
     CDBBatch batch(db);
@@ -982,8 +1026,8 @@ void CInstantSendManager::ProcessInstantSendLock(NodeId from, const uint256& has
         g_connman->RelayInvFiltered(inv, islock->txid, LLMQS_PROTO_VERSION);
     }
 
-    RemoveMempoolConflictsForLock(hash, *islock);
     ResolveBlockConflicts(hash, *islock);
+    RemoveMempoolConflictsForLock(hash, *islock);
 
     if (tx != nullptr) {
         LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- notify about an in-time lock for tx %s\n", __func__, tx->GetHash().ToString());
@@ -1023,6 +1067,23 @@ void CInstantSendManager::TransactionAddedToMempool(const CTransactionRef& tx)
     }
 }
 
+void CInstantSendManager::TransactionRemovedFromMempool(const CTransactionRef& tx)
+{
+    if (tx->vin.empty()) {
+        return;
+    }
+
+    LOCK(cs);
+    CInstantSendLockPtr islock = db.GetInstantSendLockByTxid(tx->GetHash());
+
+    if (islock == nullptr) {
+        return;
+    }
+
+    LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- transaction %s was removed from mempool\n", __func__, tx->GetHash().ToString());
+    RemoveConflictingLock(::SerializeHash(*islock), *islock);
+}
+
 void CInstantSendManager::BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindex, const std::vector<CTransactionRef>& vtxConflicted)
 {
     if (!IsInstantSendEnabled()) {
@@ -1291,7 +1352,9 @@ void CInstantSendManager::ResolveBlockConflicts(const uint256& islockHash, const
     // when large parts of the masternode network are controlled by an attacker. In this case we must still find consensus
     // and its better to sacrifice individual ISLOCKs then to sacrifice whole ChainLocks.
     if (hasChainLockedConflict) {
-        RemoveChainLockConflictingLock(islockHash, islock);
+        LogPrintf("CInstantSendManager::%s -- txid=%s, islock=%s: at least one conflicted TX already got a ChainLock\n", __func__,
+                  islock.txid.ToString(), islockHash.ToString());
+        RemoveConflictingLock(islockHash, islock);
         return;
     }
 
@@ -1330,9 +1393,9 @@ void CInstantSendManager::ResolveBlockConflicts(const uint256& islockHash, const
     }
 }
 
-void CInstantSendManager::RemoveChainLockConflictingLock(const uint256& islockHash, const llmq::CInstantSendLock& islock)
+void CInstantSendManager::RemoveConflictingLock(const uint256& islockHash, const llmq::CInstantSendLock& islock)
 {
-    LogPrintf("CInstantSendManager::%s -- txid=%s, islock=%s: at least one conflicted TX already got a ChainLock. Removing ISLOCK and its chained children.\n", __func__,
+    LogPrintf("CInstantSendManager::%s -- txid=%s, islock=%s: Removing ISLOCK and its chained children\n", __func__,
               islock.txid.ToString(), islockHash.ToString());
     int tipHeight;
     {

src/llmq/quorums_instantsend.h

@@ -43,6 +43,8 @@ typedef std::shared_ptr<CInstantSendLock> CInstantSendLockPtr;
 class CInstantSendDb
 {
 private:
+    static const int CURRENT_VERSION = 1;
+
     CDBWrapper& db;
 
     mutable unordered_lru_cache<uint256, CInstantSendLockPtr, StaticSaltedHasher, 10000> islockCache;
@@ -52,8 +54,10 @@ private:
     void WriteInstantSendLockMined(CDBBatch& batch, const uint256& hash, int nHeight);
     void RemoveInstantSendLockMined(CDBBatch& batch, const uint256& hash, int nHeight);
 
+    void Upgrade();
+
 public:
-    explicit CInstantSendDb(CDBWrapper& _db) : db(_db) {}
+    explicit CInstantSendDb(CDBWrapper& _db);
 
     void WriteNewInstantSendLock(const uint256& hash, const CInstantSendLock& islock);
     void RemoveInstantSendLock(CDBBatch& batch, const uint256& hash, CInstantSendLockPtr islock, bool keep_cache = true);
@@ -146,6 +150,7 @@ public:
     void ProcessInstantSendLock(NodeId from, const uint256& hash, const CInstantSendLockPtr& islock);
 
     void TransactionAddedToMempool(const CTransactionRef& tx);
+    void TransactionRemovedFromMempool(const CTransactionRef& tx);
     void BlockConnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindex, const std::vector<CTransactionRef>& vtxConflicted);
     void BlockDisconnected(const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexDisconnected);
 
@@ -161,7 +166,7 @@ public:
 
     void RemoveMempoolConflictsForLock(const uint256& hash, const CInstantSendLock& islock);
     void ResolveBlockConflicts(const uint256& islockHash, const CInstantSendLock& islock);
-    void RemoveChainLockConflictingLock(const uint256& islockHash, const CInstantSendLock& islock);
+    void RemoveConflictingLock(const uint256& islockHash, const CInstantSendLock& islock);
     static void AskNodesForLockedTx(const uint256& txid);
     void ProcessPendingRetryLockTxs();
 

src/net_processing.cpp

@@ -2938,6 +2938,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
                 // We will continue to reject this tx since it has rejected
                 // parents so avoid re-requesting it from other peers.
                 recentRejects->insert(tx.GetHash());
+                llmq::quorumInstantSendManager->TransactionRemovedFromMempool(ptx);
             }
         } else {
             if (!state.CorruptionPossible()) {
@@ -2965,6 +2966,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
                     LogPrintf("Not relaying invalid transaction %s from whitelisted peer=%d (%s)\n", tx.GetHash().ToString(), pfrom->GetId(), FormatStateMessage(state));
                 }
             }
+            llmq::quorumInstantSendManager->TransactionRemovedFromMempool(ptx);
         }
 
         int nDoS = 0;

src/txmempool.cpp

@@ -622,6 +622,9 @@ bool CTxMemPool::removeSpentIndex(const uint256 txhash)
 void CTxMemPool::removeUnchecked(txiter it, MemPoolRemovalReason reason)
 {
     NotifyEntryRemoved(it->GetSharedTx(), reason);
+    if (reason != MemPoolRemovalReason::BLOCK) {
+        llmq::quorumInstantSendManager->TransactionRemovedFromMempool(it->GetSharedTx());
+    }
     const uint256 hash = it->GetTx().GetHash();
     for (const CTxIn& txin : it->GetTx().vin)
         mapNextTx.erase(txin.prevout);

src/validation.cpp

@@ -2379,7 +2379,7 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
                 continue;
             }
             if (llmq::chainLocksHandler->HasChainLock(pindex->nHeight, pindex->GetBlockHash())) {
-                llmq::quorumInstantSendManager->RemoveChainLockConflictingLock(::SerializeHash(*conflictLock), *conflictLock);
+                llmq::quorumInstantSendManager->RemoveConflictingLock(::SerializeHash(*conflictLock), *conflictLock);
                 assert(llmq::quorumInstantSendManager->GetConflictingLock(*tx) == nullptr);
             } else {
                 // The node which relayed this should switch to correct chain.