From a729fbdd1b64187bbe2e459e4f54f88e9a7753ab Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Wed, 7 Aug 2019 15:00:46 +0200 Subject: [PATCH 1/5] Remove unused overload of RemoveInstantSendLock --- src/llmq/quorums_instantsend.cpp | 7 ------- src/llmq/quorums_instantsend.h | 1 - 2 files changed, 8 deletions(-) diff --git a/src/llmq/quorums_instantsend.cpp b/src/llmq/quorums_instantsend.cpp index 1f12ea257697..5c718795b815 100644 --- a/src/llmq/quorums_instantsend.cpp +++ b/src/llmq/quorums_instantsend.cpp @@ -58,13 +58,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) { diff --git a/src/llmq/quorums_instantsend.h b/src/llmq/quorums_instantsend.h index 1a7521c2dd57..5a1dd8b3f851 100644 --- a/src/llmq/quorums_instantsend.h +++ b/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); From e1a74cadfa188427fd6e04953a552a0f65e539fe Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Wed, 7 Aug 2019 15:07:12 +0200 Subject: [PATCH 2/5] Move deletion of recovered sigs into own method --- src/llmq/quorums_signing.cpp | 53 +++++++++++++++++++++++------------- src/llmq/quorums_signing.h | 2 ++ 2 files changed, 36 insertions(+), 19 deletions(-) diff --git a/src/llmq/quorums_signing.cpp b/src/llmq/quorums_signing.cpp index 708872f4a67c..8279ad24f57e 100644 --- a/src/llmq/quorums_signing.cpp +++ b/src/llmq/quorums_signing.cpp @@ -256,6 +256,39 @@ void CRecoveredSigsDb::WriteRecoveredSig(const llmq::CRecoveredSig& recSig) } } +void CRecoveredSigsDb::RemoveRecoveredSig(CDBBatch& batch, Consensus::LLMQType llmqType, const uint256& id) +{ + 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); + + 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); + db.WriteBatch(batch); +} + void CRecoveredSigsDb::CleanupOldRecoveredSigs(int64_t maxAge) { std::unique_ptr pcursor(db.NewIterator()); @@ -292,25 +325,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); if (batch.SizeEstimate() >= (1 << 24)) { db.WriteBatch(batch); diff --git a/src/llmq/quorums_signing.h b/src/llmq/quorums_signing.h index 0eec5c393490..46f187fed749 100644 --- a/src/llmq/quorums_signing.h +++ b/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); }; class CRecoveredSigsListener From c734ebe965c5c11b08971ad41479ab8f21a7ae1b Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Wed, 7 Aug 2019 15:07:44 +0200 Subject: [PATCH 3/5] Remove recovered sigs for fully confirmed IS locks --- src/llmq/quorums_instantsend.cpp | 9 +++++++++ src/llmq/quorums_signing.cpp | 5 +++++ src/llmq/quorums_signing.h | 4 ++++ 3 files changed, 18 insertions(+) diff --git a/src/llmq/quorums_instantsend.cpp b/src/llmq/quorums_instantsend.cpp index 5c718795b815..aa6921128a25 100644 --- a/src/llmq/quorums_instantsend.cpp +++ b/src/llmq/quorums_instantsend.cpp @@ -1084,6 +1084,8 @@ void CInstantSendManager::UpdatedBlockTip(const CBlockIndex* pindexNew) void CInstantSendManager::HandleFullyConfirmedBlock(const CBlockIndex* pindex) { + auto& consensusParams = Params().GetConsensus(); + std::unordered_map removeISLocks; { LOCK(cs); @@ -1101,7 +1103,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 diff --git a/src/llmq/quorums_signing.cpp b/src/llmq/quorums_signing.cpp index 8279ad24f57e..5ad9c90a60d2 100644 --- a/src/llmq/quorums_signing.cpp +++ b/src/llmq/quorums_signing.cpp @@ -695,6 +695,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(); diff --git a/src/llmq/quorums_signing.h b/src/llmq/quorums_signing.h index 46f187fed749..2df93c2226d4 100644 --- a/src/llmq/quorums_signing.h +++ b/src/llmq/quorums_signing.h @@ -146,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); From fb4bf04a63e34002672dddcb6f1d7ed28dc5088b Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Wed, 7 Aug 2019 17:29:40 +0200 Subject: [PATCH 4/5] 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). --- src/llmq/quorums_signing.cpp | 23 ++++++++++++++++++----- src/llmq/quorums_signing.h | 2 +- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/src/llmq/quorums_signing.cpp b/src/llmq/quorums_signing.cpp index 5ad9c90a60d2..0634a1dffe01 100644 --- a/src/llmq/quorums_signing.cpp +++ b/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,7 +259,7 @@ void CRecoveredSigsDb::WriteRecoveredSig(const llmq::CRecoveredSig& recSig) } } -void CRecoveredSigsDb::RemoveRecoveredSig(CDBBatch& batch, Consensus::LLMQType llmqType, const uint256& id) +void CRecoveredSigsDb::RemoveRecoveredSig(CDBBatch& batch, Consensus::LLMQType llmqType, const uint256& id, bool deleteTimeKey) { AssertLockHeld(cs); @@ -276,6 +279,16 @@ void CRecoveredSigsDb::RemoveRecoveredSig(CDBBatch& batch, Consensus::LLMQType l batch.Erase(k3); batch.Erase(k4); + if (deleteTimeKey) { + CDataStream writeTimeDs(SER_DISK, CLIENT_VERSION); + 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()); @@ -285,7 +298,7 @@ void CRecoveredSigsDb::RemoveRecoveredSig(Consensus::LLMQType llmqType, const ui { LOCK(cs); CDBBatch batch(db); - RemoveRecoveredSig(batch, llmqType, id); + RemoveRecoveredSig(batch, llmqType, id, true); db.WriteBatch(batch); } @@ -325,7 +338,7 @@ void CRecoveredSigsDb::CleanupOldRecoveredSigs(int64_t maxAge) { LOCK(cs); for (auto& e : toDelete) { - RemoveRecoveredSig(batch, e.first, e.second); + RemoveRecoveredSig(batch, e.first, e.second, false); if (batch.SizeEstimate() >= (1 << 24)) { db.WriteBatch(batch); diff --git a/src/llmq/quorums_signing.h b/src/llmq/quorums_signing.h index 2df93c2226d4..407a21ba7c35 100644 --- a/src/llmq/quorums_signing.h +++ b/src/llmq/quorums_signing.h @@ -97,7 +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); + void RemoveRecoveredSig(CDBBatch& batch, Consensus::LLMQType llmqType, const uint256& id, bool deleteTimeKey); }; class CRecoveredSigsListener From 62dea18ab77324f65a4dba9e691374097a511eb1 Mon Sep 17 00:00:00 2001 From: Alexander Block Date: Thu, 8 Aug 2019 14:00:37 +0200 Subject: [PATCH 5/5] Add TODO --- src/llmq/quorums_signing.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/llmq/quorums_signing.cpp b/src/llmq/quorums_signing.cpp index 0634a1dffe01..8c8346c4ae98 100644 --- a/src/llmq/quorums_signing.cpp +++ b/src/llmq/quorums_signing.cpp @@ -281,6 +281,7 @@ void CRecoveredSigsDb::RemoveRecoveredSig(CDBBatch& batch, Consensus::LLMQType l 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;