Remove recovered sigs from the LLMQ db when corresponding IS locks get confirmed (dashpay#3048)

* Remove unused overload of RemoveInstantSendLock

* Move deletion of recovered sigs into own method

* Remove recovered sigs for fully confirmed IS locks

* Also remove rs_t entries when removing recovered sigs from the outside

CleanupOldRecoveredSigs already does this as the last step, but when
RemoveRecoveredSig is called from the outside (e.g. from InstantSend),
these keys are not removed. This PR fixes this by storing the write time
into rs_r and later uses it to remove the rs_t entry.

Old entries will be incompatible with this (1 byte written in the past,
4 bytes written now). This checked by comparing the data size with
sizeof(uint32_t).

* Add TODO

Author: codablock

src/llmq/quorums_instantsend.cpp

@@ -61,13 +61,6 @@ void CInstantSendDb::WriteNewInstantSendLock(const uint256& hash, const CInstant
     }
 }
 
-void CInstantSendDb::RemoveInstantSendLock(const uint256& hash, CInstantSendLockPtr islock)
-{
-    CDBBatch batch(db);
-    RemoveInstantSendLock(batch, hash, islock);
-    db.WriteBatch(batch);
-}
-
 void CInstantSendDb::RemoveInstantSendLock(CDBBatch& batch, const uint256& hash, CInstantSendLockPtr islock)
 {
     if (!islock) {
@@ -1106,6 +1099,8 @@ void CInstantSendManager::UpdatedBlockTip(const CBlockIndex* pindexNew)
 
 void CInstantSendManager::HandleFullyConfirmedBlock(const CBlockIndex* pindex)
 {
+    auto& consensusParams = Params().GetConsensus();
+
     std::unordered_map<uint256, CInstantSendLockPtr> removeISLocks;
     {
         LOCK(cs);
@@ -1123,7 +1118,14 @@ void CInstantSendManager::HandleFullyConfirmedBlock(const CBlockIndex* pindex)
             for (auto& in : islock->inputs) {
                 auto inputRequestId = ::SerializeHash(std::make_pair(INPUTLOCK_REQUESTID_PREFIX, in));
                 inputRequestIds.erase(inputRequestId);
+
+                // no need to keep recovered sigs for fully confirmed IS locks, as there is no chance for conflicts
+                // from now on. All inputs are spent now and can't be spend in any other TX.
+                quorumSigningManager->RemoveRecoveredSig(consensusParams.llmqForInstantSend, inputRequestId);
             }
+
+            // same as in the loop
+            quorumSigningManager->RemoveRecoveredSig(consensusParams.llmqForInstantSend, islock->GetRequestId());
         }
 
         // Find all previously unlocked TXs that got locked by this fully confirmed (ChainLock) block and remove them

src/llmq/quorums_instantsend.h

@@ -53,7 +53,6 @@ class CInstantSendDb
     CInstantSendDb(CDBWrapper& _db) : db(_db) {}
 
     void WriteNewInstantSendLock(const uint256& hash, const CInstantSendLock& islock);
-    void RemoveInstantSendLock(const uint256& hash, CInstantSendLockPtr islock);
     void RemoveInstantSendLock(CDBBatch& batch, const uint256& hash, CInstantSendLockPtr islock);
 
     void WriteInstantSendLockMined(const uint256& hash, int nHeight);

src/llmq/quorums_signing.cpp

@@ -224,12 +224,15 @@ void CRecoveredSigsDb::WriteRecoveredSig(const llmq::CRecoveredSig& recSig)
 {
     CDBBatch batch(db);
 
+    uint32_t curTime = GetAdjustedTime();
+
     // we put these close to each other to leverage leveldb's key compaction
     // this way, the second key can be used for fast HasRecoveredSig checks while the first key stores the recSig
     auto k1 = std::make_tuple(std::string("rs_r"), recSig.llmqType, recSig.id);
     auto k2 = std::make_tuple(std::string("rs_r"), recSig.llmqType, recSig.id, recSig.msgHash);
     batch.Write(k1, recSig);
-    batch.Write(k2, (uint8_t)1);
+    // this key is also used to store the current time, so that we can easily get to the "rs_t" key when we have the id
+    batch.Write(k2, curTime);
 
     // store by object hash
     auto k3 = std::make_tuple(std::string("rs_h"), recSig.GetHash());
@@ -241,7 +244,7 @@ void CRecoveredSigsDb::WriteRecoveredSig(const llmq::CRecoveredSig& recSig)
     batch.Write(k4, (uint8_t)1);
 
     // store by current time. Allows fast cleanup of old recSigs
-    auto k5 = std::make_tuple(std::string("rs_t"), (uint32_t)htobe32(GetAdjustedTime()), recSig.llmqType, recSig.id);
+    auto k5 = std::make_tuple(std::string("rs_t"), (uint32_t)htobe32(curTime), recSig.llmqType, recSig.id);
     batch.Write(k5, (uint8_t)1);
 
     db.WriteBatch(batch);
@@ -256,6 +259,50 @@ void CRecoveredSigsDb::WriteRecoveredSig(const llmq::CRecoveredSig& recSig)
     }
 }
 
+void CRecoveredSigsDb::RemoveRecoveredSig(CDBBatch& batch, Consensus::LLMQType llmqType, const uint256& id, bool deleteTimeKey)
+{
+    AssertLockHeld(cs);
+
+    CRecoveredSig recSig;
+    if (!ReadRecoveredSig(llmqType, id, recSig)) {
+        return;
+    }
+
+    auto signHash = CLLMQUtils::BuildSignHash(recSig);
+
+    auto k1 = std::make_tuple(std::string("rs_r"), recSig.llmqType, recSig.id);
+    auto k2 = std::make_tuple(std::string("rs_r"), recSig.llmqType, recSig.id, recSig.msgHash);
+    auto k3 = std::make_tuple(std::string("rs_h"), recSig.GetHash());
+    auto k4 = std::make_tuple(std::string("rs_s"), signHash);
+    batch.Erase(k1);
+    batch.Erase(k2);
+    batch.Erase(k3);
+    batch.Erase(k4);
+
+    if (deleteTimeKey) {
+        CDataStream writeTimeDs(SER_DISK, CLIENT_VERSION);
+        // TODO remove the size() == sizeof(uint32_t) in a future version (when we stop supporting upgrades from < 0.14.1)
+        if (db.ReadDataStream(k2, writeTimeDs) && writeTimeDs.size() == sizeof(uint32_t)) {
+            uint32_t writeTime;
+            writeTimeDs >> writeTime;
+            auto k5 = std::make_tuple(std::string("rs_t"), (uint32_t) htobe32(writeTime), recSig.llmqType, recSig.id);
+            batch.Erase(k5);
+        }
+    }
+
+    hasSigForIdCache.erase(std::make_pair((Consensus::LLMQType)recSig.llmqType, recSig.id));
+    hasSigForSessionCache.erase(signHash);
+    hasSigForHashCache.erase(recSig.GetHash());
+}
+
+void CRecoveredSigsDb::RemoveRecoveredSig(Consensus::LLMQType llmqType, const uint256& id)
+{
+    LOCK(cs);
+    CDBBatch batch(db);
+    RemoveRecoveredSig(batch, llmqType, id, true);
+    db.WriteBatch(batch);
+}
+
 void CRecoveredSigsDb::CleanupOldRecoveredSigs(int64_t maxAge)
 {
     std::unique_ptr<CDBIterator> pcursor(db.NewIterator());
@@ -292,25 +339,7 @@ void CRecoveredSigsDb::CleanupOldRecoveredSigs(int64_t maxAge)
     {
         LOCK(cs);
         for (auto& e : toDelete) {
-            CRecoveredSig recSig;
-            if (!ReadRecoveredSig(e.first, e.second, recSig)) {
-                continue;
-            }
-
-            auto signHash = CLLMQUtils::BuildSignHash(recSig);
-
-            auto k1 = std::make_tuple(std::string("rs_r"), recSig.llmqType, recSig.id);
-            auto k2 = std::make_tuple(std::string("rs_r"), recSig.llmqType, recSig.id, recSig.msgHash);
-            auto k3 = std::make_tuple(std::string("rs_h"), recSig.GetHash());
-            auto k4 = std::make_tuple(std::string("rs_s"), signHash);
-            batch.Erase(k1);
-            batch.Erase(k2);
-            batch.Erase(k3);
-            batch.Erase(k4);
-
-            hasSigForIdCache.erase(std::make_pair((Consensus::LLMQType)recSig.llmqType, recSig.id));
-            hasSigForSessionCache.erase(signHash);
-            hasSigForHashCache.erase(recSig.GetHash());
+            RemoveRecoveredSig(batch, e.first, e.second, false);
 
             if (batch.SizeEstimate() >= (1 << 24)) {
                 db.WriteBatch(batch);
@@ -680,6 +709,11 @@ void CSigningManager::PushReconstructedRecoveredSig(const llmq::CRecoveredSig& r
     pendingReconstructedRecoveredSigs.emplace_back(recoveredSig, quorum);
 }
 
+void CSigningManager::RemoveRecoveredSig(Consensus::LLMQType llmqType, const uint256& id)
+{
+    db.RemoveRecoveredSig(llmqType, id);
+}
+
 void CSigningManager::Cleanup()
 {
     int64_t now = GetTimeMillis();

src/llmq/quorums_signing.h

@@ -84,6 +84,7 @@ class CRecoveredSigsDb
     bool GetRecoveredSigByHash(const uint256& hash, CRecoveredSig& ret);
     bool GetRecoveredSigById(Consensus::LLMQType llmqType, const uint256& id, CRecoveredSig& ret);
     void WriteRecoveredSig(const CRecoveredSig& recSig);
+    void RemoveRecoveredSig(Consensus::LLMQType llmqType, const uint256& id);
 
     void CleanupOldRecoveredSigs(int64_t maxAge);
 
@@ -96,6 +97,7 @@ class CRecoveredSigsDb
 
 private:
     bool ReadRecoveredSig(Consensus::LLMQType llmqType, const uint256& id, CRecoveredSig& ret);
+    void RemoveRecoveredSig(CDBBatch& batch, Consensus::LLMQType llmqType, const uint256& id, bool deleteTimeKey);
 };
 
 class CRecoveredSigsListener
@@ -144,6 +146,10 @@ class CSigningManager
     // This is the case for example when a signature appears as part of InstantSend or ChainLocks
     void PushReconstructedRecoveredSig(const CRecoveredSig& recoveredSig, const CQuorumCPtr& quorum);
 
+    // This is called when a recovered signature can be safely removed from the DB. This is only safe when some other
+    // mechanism prevents possible conflicts. As an example, ChainLocks prevent conflicts in confirmed TXs InstantSend votes
+    void RemoveRecoveredSig(Consensus::LLMQType llmqType, const uint256& id);
+
 private:
     void ProcessMessageRecoveredSig(CNode* pfrom, const CRecoveredSig& recoveredSig, CConnman& connman);
     bool PreVerifyRecoveredSig(NodeId nodeId, const CRecoveredSig& recoveredSig, bool& retBan);