From ff829d4f5c5cb59664e301ecf6360e72497ed0f3 Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 7 Oct 2026 02:22:15 -0500 Subject: [PATCH 1/2] fix: expire truncated recovered-signature markers Truncating a recovered signature deletes the object but keeps its hash (rs_h) and session (rs_s) markers for late-share filtering. Age cleanup found those markers through the object, so after a truncation it dropped the time index entry and left both markers on disk for good. Markers now carry their write time: rs_h values become (type, id, time), rs_s values become the time, and a new rs_e index orders them by (time, prefix, hash). Expiry walks rs_e and erases a marker only while its stored time still matches, so a refreshed marker survives a stale entry. rs_e is the only path that deletes markers, including for Platform signatures. Older releases can still read the extended values: rs_h is read as a pair and rs_s is only checked for existence. Markers in the old format are adopted by a bounded scan that resumes from a saved cursor (rs_c) and keeps cycling, so values written by an older binary after a downgrade are adopted too. Old markers have no age, so each is given one full expiry window from adoption. The object cleanup now removes an object only if its write time matches the rs_t entry, so a stale entry cannot delete a newer object written under the same id. Index writes, truncations, cleanup and the cached marker lookups are serialized by cs_cache. Lookups under cs_main wait on that lock, so every scan made under it examines at most 1000 entries per pass, and an expired backlog drains over several passes. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/llmq/signing.cpp | 263 +++++++++++++++++++++------------- src/llmq/signing.h | 10 +- src/test/llmq_utils_tests.cpp | 150 +++++++++++++++++++ 3 files changed, 317 insertions(+), 106 deletions(-) diff --git a/src/llmq/signing.cpp b/src/llmq/signing.cpp index b8e380e26e2b..126b5c91c41d 100644 --- a/src/llmq/signing.cpp +++ b/src/llmq/signing.cpp @@ -15,12 +15,19 @@ #include #include +#include #include #include #include namespace llmq { +namespace { +// Cleanup holds cs_cache, which lookups under cs_main wait on, so each pass examines a bounded +// number of index entries. Expiry indexes erase every examined entry; the next pass resumes there. +constexpr size_t MAX_CLEANUP_ENTRIES{1000}; +} // namespace + CRecoveredSigsDb::CRecoveredSigsDb(const util::DbWrapperParams& db_params) : db{util::MakeDbWrapper({db_params.path / "llmq" / "recsigdb", db_params.memory, db_params.wipe, /*cache_size=*/8 << 20})} { @@ -41,55 +48,46 @@ bool CRecoveredSigsDb::HasRecoveredSig(Consensus::LLMQType llmqType, const uint2 bool CRecoveredSigsDb::HasRecoveredSigForId(Consensus::LLMQType llmqType, const uint256& id) const { auto cacheKey = std::make_pair(llmqType, id); + LOCK(cs_cache); bool ret; - { - LOCK(cs_cache); - if (hasSigForIdCache.get(cacheKey, ret)) { - return ret; - } + if (hasSigForIdCache.get(cacheKey, ret)) { + return ret; } auto k = std::make_tuple(std::string("rs_r"), llmqType, id); ret = db->Exists(k); - LOCK(cs_cache); hasSigForIdCache.insert(cacheKey, ret); return ret; } bool CRecoveredSigsDb::HasRecoveredSigForSession(const uint256& signHash) const { + LOCK(cs_cache); bool ret; - { - LOCK(cs_cache); - if (hasSigForSessionCache.get(signHash, ret)) { - return ret; - } + if (hasSigForSessionCache.get(signHash, ret)) { + return ret; } auto k = std::make_tuple(std::string("rs_s"), signHash); ret = db->Exists(k); - LOCK(cs_cache); hasSigForSessionCache.insert(signHash, ret); return ret; } bool CRecoveredSigsDb::HasRecoveredSigForHash(const uint256& hash) const { + LOCK(cs_cache); bool ret; - { - LOCK(cs_cache); - if (hasSigForHashCache.get(hash, ret)) { - return ret; - } + if (hasSigForHashCache.get(hash, ret)) { + return ret; } auto k = std::make_tuple(std::string("rs_h"), hash); ret = db->Exists(k); - LOCK(cs_cache); hasSigForHashCache.insert(hash, ret); return ret; } @@ -145,6 +143,7 @@ bool CRecoveredSigsDb::GetRecoveredSig(Consensus::LLMQType llmqType, const uint2 void CRecoveredSigsDb::WriteRecoveredSig(const llmq::CRecoveredSig& recSig) { + LOCK(cs_cache); CDBBatch batch(*db); uint32_t curTime = GetTime().count(); @@ -166,61 +165,34 @@ void CRecoveredSigsDb::WriteRecoveredSig(const llmq::CRecoveredSig& recSig) // store by object hash auto k3 = std::make_tuple(std::string("rs_h"), recSig.GetHash()); - batch.Write(k3, std::make_pair(recSig.getLlmqType(), recSig.getId())); + batch.Write(k3, std::make_tuple(recSig.getLlmqType(), recSig.getId(), curTime)); // store by signHash auto signHash = recSig.buildSignHash(); auto k4 = std::make_tuple(std::string("rs_s"), signHash.Get()); - batch.Write(k4, static_cast(1)); + batch.Write(k4, curTime); + for (const auto& [prefix, hash] : + {std::make_pair(std::string("rs_h"), recSig.GetHash()), std::make_pair(std::string("rs_s"), signHash.Get())}) { + batch.Write(std::make_tuple(std::string("rs_e"), htobe32_internal(curTime), prefix, hash), uint8_t{1}); + } db->WriteBatch(batch); - { - LOCK(cs_cache); - if (recSig.getLlmqType() != Params().GetConsensus().llmqTypePlatform) { - hasSigForIdCache.insert(std::make_pair(recSig.getLlmqType(), recSig.getId()), true); - } - hasSigForSessionCache.insert(signHash.Get(), true); - hasSigForHashCache.insert(recSig.GetHash(), true); + if (recSig.getLlmqType() != Params().GetConsensus().llmqTypePlatform) { + hasSigForIdCache.insert(std::make_pair(recSig.getLlmqType(), recSig.getId()), true); } + hasSigForSessionCache.insert(signHash.Get(), true); + hasSigForHashCache.insert(recSig.GetHash(), true); } -void CRecoveredSigsDb::RemoveRecoveredSig(CDBBatch& batch, Consensus::LLMQType llmqType, const uint256& id, bool deleteHashKey, bool deleteTimeKey) +void CRecoveredSigsDb::RemoveRecoveredSig(CDBBatch& batch, Consensus::LLMQType llmqType, const uint256& id) { + AssertLockHeld(cs_cache); CRecoveredSig recSig; - if (!ReadRecoveredSig(llmqType, id, recSig)) { - return; - } - - auto signHash = recSig.buildSignHash(); - - auto k1 = std::make_tuple(std::string("rs_r"), recSig.getLlmqType(), recSig.getId()); - auto k2 = std::make_tuple(std::string("rs_r"), recSig.getLlmqType(), recSig.getId(), recSig.getMsgHash()); - auto k3 = std::make_tuple(std::string("rs_h"), recSig.GetHash()); - auto k4 = std::make_tuple(std::string("rs_s"), signHash.Get()); - batch.Erase(k1); - batch.Erase(k2); - if (deleteHashKey) { - batch.Erase(k3); - batch.Erase(k4); - } - - if (deleteTimeKey) { - CDataStream writeTimeDs(SER_DISK, CLIENT_VERSION); - if (db->ReadDataStream(k2, writeTimeDs)) { - uint32_t writeTime; - writeTimeDs >> writeTime; - auto k5 = std::make_tuple(std::string("rs_t"), htobe32_internal(writeTime), recSig.getLlmqType(), recSig.getId()); - batch.Erase(k5); - } - } - - LOCK(cs_cache); - hasSigForIdCache.erase(std::make_pair(recSig.getLlmqType(), recSig.getId())); - if (deleteHashKey) { - hasSigForSessionCache.erase(signHash.Get()); - hasSigForHashCache.erase(recSig.GetHash()); - } + if (!ReadRecoveredSig(llmqType, id, recSig)) return; + batch.Erase(std::make_tuple(std::string("rs_r"), llmqType, id)); + batch.Erase(std::make_tuple(std::string("rs_r"), llmqType, id, recSig.getMsgHash())); + hasSigForIdCache.erase(std::make_pair(llmqType, id)); } // Remove the recovered sig itself and all keys required to get from id -> recSig @@ -228,8 +200,9 @@ void CRecoveredSigsDb::RemoveRecoveredSig(CDBBatch& batch, Consensus::LLMQType l // late-share filtering still returns true void CRecoveredSigsDb::TruncateRecoveredSig(Consensus::LLMQType llmqType, const uint256& id) { + LOCK(cs_cache); CDBBatch batch(*db); - RemoveRecoveredSig(batch, llmqType, id, false, false); + RemoveRecoveredSig(batch, llmqType, id); db->WriteBatch(batch); } @@ -239,20 +212,15 @@ void CRecoveredSigsDb::CleanupOldPlatformSigs(int64_t maxAge) auto start = std::make_tuple(std::string("rs_u"), uint32_t{0}, uint256{}); const auto end_time = static_cast(GetTime().count() - maxAge); CDBBatch batch(*db); - for (cursor->Seek(start); cursor->Valid(); cursor->Next()) { + cursor->Seek(start); + for (size_t examined{0}; examined < MAX_CLEANUP_ENTRIES && cursor->Valid(); ++examined, cursor->Next()) { decltype(start) key; if (!cursor->GetKey(key) || std::get<0>(key) != "rs_u" || be32toh_internal(std::get<1>(key)) >= end_time) break; const auto& hash = std::get<2>(key); CRecoveredSig rec_sig; if (db->Read(std::make_pair(std::string("rs_p"), hash), rec_sig)) { - const auto sign_hash = rec_sig.buildSignHash().Get(); batch.Erase(std::make_pair(std::string("rs_p"), hash)); batch.Erase(std::make_tuple(std::string("rs_m"), rec_sig.getLlmqType(), rec_sig.getId(), rec_sig.getMsgHash())); - batch.Erase(std::make_pair(std::string("rs_h"), hash)); - batch.Erase(std::make_pair(std::string("rs_s"), sign_hash)); - LOCK(cs_cache); - hasSigForHashCache.erase(hash); - hasSigForSessionCache.erase(sign_hash); } batch.Erase(key); if (batch.SizeEstimate() >= (1 << 24)) { @@ -263,56 +231,145 @@ void CRecoveredSigsDb::CleanupOldPlatformSigs(int64_t maxAge) db->WriteBatch(batch); } -void CRecoveredSigsDb::CleanupOldRecoveredSigs(int64_t maxAge) -{ - CleanupOldPlatformSigs(maxAge); - std::unique_ptr pcursor(db->NewIterator()); - - auto start = std::make_tuple(std::string("rs_t"), static_cast(0), static_cast(0), uint256()); - uint32_t endTime = static_cast(GetTime().count() - maxAge); - pcursor->Seek(start); +namespace { +struct RecoveredSigMarker { + std::pair owner{}; + std::optional time; +}; - std::vector> toDelete; - std::vector toDelete2; - - while (pcursor->Valid()) { - decltype(start) k; - - if (!pcursor->GetKey(k) || std::get<0>(k) != "rs_t") { - break; +std::optional ReadRecoveredSigMarker(const CDBWrapper& db, const std::string& prefix, const uint256& hash) +{ + CDataStream value{SER_DISK, CLIENT_VERSION}; + if (!db.ReadDataStream(std::make_pair(prefix, hash), value)) return std::nullopt; + RecoveredSigMarker marker; + try { + if (prefix == "rs_h") { + value >> marker.owner; + if (value.empty()) return marker; + } else if (prefix == "rs_s") { + if (value.size() == 1) { + uint8_t legacy; + value >> legacy; + return legacy == 1 ? std::optional{marker} : std::nullopt; + } + } else { + return std::nullopt; } - if (be32toh_internal(std::get<1>(k)) >= endTime) { + if (value.size() != sizeof(uint32_t)) return std::nullopt; + uint32_t time; + value >> time; + marker.time = time; + return marker; + } catch (const std::exception&) { + return std::nullopt; + } +} +} // namespace + +void CRecoveredSigsDb::MigrateLegacyMarkers(const std::string& prefix) +{ + AssertLockHeld(cs_cache); + const auto cursor_key = std::make_pair(std::string("rs_c"), prefix); + uint256 last; + const bool have_cursor = db->Read(cursor_key, last); + std::unique_ptr cursor{db->NewIterator()}; + auto start = std::make_pair(prefix, last); + cursor->Seek(start); + if (have_cursor && cursor->Valid()) { + decltype(start) key; + if (cursor->GetKey(key) && key == start) cursor->Next(); + } + CDBBatch batch(*db); + bool finished{false}; + const uint32_t now = GetTime().count(); + for (size_t examined{0}; examined < MAX_CLEANUP_ENTRIES && cursor->Valid(); ++examined, cursor->Next()) { + decltype(start) key; + if (!cursor->GetKey(key) || key.first != prefix) { + finished = true; break; } - - toDelete.emplace_back(std::get<2>(k), std::get<3>(k)); - toDelete2.emplace_back(k); - - pcursor->Next(); + last = key.second; + const auto marker = ReadRecoveredSigMarker(*db, prefix, key.second); + if (!marker || marker->time) continue; + // Uncorrelated legacy session markers have no recoverable age; retain one grace interval. + if (prefix == "rs_h") { + batch.Write(key, std::make_tuple(marker->owner.first, marker->owner.second, now)); + } else { + batch.Write(key, now); + } + batch.Write(std::make_tuple(std::string("rs_e"), htobe32_internal(now), prefix, key.second), uint8_t{1}); } - pcursor.reset(); - - if (toDelete.empty()) { - return; + if (finished || !cursor->Valid()) { + batch.Erase(cursor_key); + } else { + batch.Write(cursor_key, last); } + db->WriteBatch(batch); +} +void CRecoveredSigsDb::CleanupOldMarkers(int64_t maxAge) +{ + AssertLockHeld(cs_cache); + std::unique_ptr cursor{db->NewIterator()}; + auto start = std::make_tuple(std::string("rs_e"), uint32_t{0}, std::string{}, uint256{}); + const auto end_time = static_cast(GetTime().count() - maxAge); CDBBatch batch(*db); - for (const auto& e : toDelete) { - RemoveRecoveredSig(batch, e.first, e.second, true, false); - + cursor->Seek(start); + for (size_t examined{0}; examined < MAX_CLEANUP_ENTRIES && cursor->Valid(); ++examined, cursor->Next()) { + decltype(start) key; + if (!cursor->GetKey(key) || std::get<0>(key) != "rs_e" || be32toh_internal(std::get<1>(key)) >= end_time) break; + const auto& prefix = std::get<2>(key); + const auto& hash = std::get<3>(key); + const auto marker = ReadRecoveredSigMarker(*db, prefix, hash); + if (marker && marker->time == be32toh_internal(std::get<1>(key))) { + batch.Erase(std::make_pair(prefix, hash)); + if (prefix == "rs_h") hasSigForHashCache.erase(hash); + if (prefix == "rs_s") hasSigForSessionCache.erase(hash); + } + batch.Erase(key); if (batch.SizeEstimate() >= (1 << 24)) { db->WriteBatch(batch); batch.Clear(); } } + db->WriteBatch(batch); +} - for (const auto& e : toDelete2) { - batch.Erase(e); +void CRecoveredSigsDb::CleanupOldRecoveredSigs(int64_t maxAge) +{ + LOCK(cs_cache); + // Cycle the bounded scans so downgrade-written legacy values are adopted on re-upgrade too. + MigrateLegacyMarkers("rs_h"); + MigrateLegacyMarkers("rs_s"); + CleanupOldMarkers(maxAge); + CleanupOldPlatformSigs(maxAge); + std::unique_ptr cursor{db->NewIterator()}; + auto start = std::make_tuple(std::string("rs_t"), uint32_t{0}, static_cast(0), uint256{}); + const auto end_time = static_cast(GetTime().count() - maxAge); + CDBBatch batch(*db); + size_t removed{0}; + cursor->Seek(start); + for (size_t examined{0}; examined < MAX_CLEANUP_ENTRIES && cursor->Valid(); ++examined, cursor->Next()) { + decltype(start) key; + if (!cursor->GetKey(key) || std::get<0>(key) != "rs_t" || be32toh_internal(std::get<1>(key)) >= end_time) break; + const auto type = std::get<2>(key); + const auto& id = std::get<3>(key); + CRecoveredSig rec_sig; + uint32_t write_time; + if (ReadRecoveredSig(type, id, rec_sig) && + db->Read(std::make_tuple(std::string("rs_r"), type, id, rec_sig.getMsgHash()), write_time) && + write_time == be32toh_internal(std::get<1>(key))) { + RemoveRecoveredSig(batch, type, id); + ++removed; + } + batch.Erase(key); + if (batch.SizeEstimate() >= (1 << 24)) { + db->WriteBatch(batch); + batch.Clear(); + } } - db->WriteBatch(batch); - - LogPrint(BCLog::LLMQ, "CRecoveredSigsDb::%d -- deleted %d entries\n", __func__, toDelete.size()); + LogPrint(BCLog::LLMQ, "CRecoveredSigsDb::%s -- deleted %d entries\n", __func__, removed); } bool CRecoveredSigsDb::HasVotedOnId(Consensus::LLMQType llmqType, const uint256& id) const diff --git a/src/llmq/signing.h b/src/llmq/signing.h index 45e51a696a21..0b11ac210553 100644 --- a/src/llmq/signing.h +++ b/src/llmq/signing.h @@ -15,6 +15,7 @@ #include #include +#include #include #include #include @@ -115,6 +116,7 @@ class CRecoveredSigsDb private: std::unique_ptr db{nullptr}; + // Serialize index mutations with cache lookups and invalidation. mutable Mutex cs_cache; mutable unordered_lru_cache, bool, StaticSaltedHasher, 30000> hasSigForIdCache GUARDED_BY(cs_cache); mutable Uint256LruHashMap hasSigForSessionCache GUARDED_BY(cs_cache); @@ -144,10 +146,12 @@ class CRecoveredSigsDb void CleanupOldVotes(int64_t maxAge); private: - void CleanupOldPlatformSigs(int64_t maxAge) EXCLUSIVE_LOCKS_REQUIRED(!cs_cache); + void MigrateLegacyMarkers(const std::string& prefix) EXCLUSIVE_LOCKS_REQUIRED(cs_cache); + void CleanupOldMarkers(int64_t maxAge) EXCLUSIVE_LOCKS_REQUIRED(cs_cache); + void CleanupOldPlatformSigs(int64_t maxAge) EXCLUSIVE_LOCKS_REQUIRED(cs_cache); bool ReadRecoveredSig(Consensus::LLMQType llmqType, const uint256& id, CRecoveredSig& ret) const; - void RemoveRecoveredSig(CDBBatch& batch, Consensus::LLMQType llmqType, const uint256& id, bool deleteHashKey, - bool deleteTimeKey) EXCLUSIVE_LOCKS_REQUIRED(!cs_cache); + void RemoveRecoveredSig(CDBBatch& batch, Consensus::LLMQType llmqType, const uint256& id) + EXCLUSIVE_LOCKS_REQUIRED(cs_cache); }; class CRecoveredSigsListener diff --git a/src/test/llmq_utils_tests.cpp b/src/test/llmq_utils_tests.cpp index bfee20443afd..cff366292f55 100644 --- a/src/test/llmq_utils_tests.cpp +++ b/src/test/llmq_utils_tests.cpp @@ -110,6 +110,156 @@ BOOST_AUTO_TEST_CASE(platform_recovered_sigs_persistence_and_expiry) SetMockTime(0); } +BOOST_AUTO_TEST_CASE(truncated_recovered_sig_markers_expire_after_reopen) +{ + const auto type = Params().GetConsensus().llmqTypeChainLocks; + const auto id = GetTestQuorumHash(1); + const auto quorum = GetTestQuorumHash(2); + const auto msg = GetTestQuorumHash(3); + CBLSSecretKey key; + key.MakeNewKey(); + const CRecoveredSig sig{type, quorum, id, msg, + key.Sign(SignHash{type, quorum, id, msg}.Get(), bls::bls_legacy_scheme.load())}; + const auto path = m_args.GetDataDirBase() / "truncated_sigs"; + const auto now = GetTime(); + SetMockTime(now); + { + CRecoveredSigsDb db({.path = path, .wipe = true}); + db.WriteRecoveredSig(sig); + db.TruncateRecoveredSig(type, id); + BOOST_CHECK(!db.HasRecoveredSigForId(type, id)); + BOOST_CHECK(db.HasRecoveredSigForHash(sig.GetHash())); + BOOST_CHECK(db.HasRecoveredSigForSession(sig.buildSignHash().Get())); + } + { + auto legacy_reader = util::MakeDbWrapper({.path = path / "llmq" / "recsigdb"}); + std::pair owner; + BOOST_REQUIRE(legacy_reader->Read(std::make_pair(std::string("rs_h"), sig.GetHash()), owner)); + BOOST_CHECK(owner == std::make_pair(type, id)); + BOOST_CHECK(legacy_reader->Exists(std::make_pair(std::string("rs_s"), sig.buildSignHash().Get()))); + } + { + CRecoveredSigsDb db({.path = path}); + BOOST_CHECK(db.HasRecoveredSigForHash(sig.GetHash())); + BOOST_CHECK(db.HasRecoveredSigForSession(sig.buildSignHash().Get())); + SetMockTime(now + 99); + db.CleanupOldRecoveredSigs(100); + BOOST_CHECK(db.HasRecoveredSigForHash(sig.GetHash())); + BOOST_CHECK(db.HasRecoveredSigForSession(sig.buildSignHash().Get())); + SetMockTime(now + 101); + db.CleanupOldRecoveredSigs(100); + BOOST_CHECK(!db.HasRecoveredSigForHash(sig.GetHash())); + BOOST_CHECK(!db.HasRecoveredSigForSession(sig.buildSignHash().Get())); + } + { + CRecoveredSigsDb db({.path = path}); + BOOST_CHECK(!db.HasRecoveredSigForHash(sig.GetHash())); + BOOST_CHECK(!db.HasRecoveredSigForSession(sig.buildSignHash().Get())); + } + SetMockTime(0); +} + +BOOST_AUTO_TEST_CASE(recovered_sig_reinsertion_survives_stale_expiry) +{ + const auto type = Params().GetConsensus().llmqTypeChainLocks; + const auto id = GetTestQuorumHash(1); + const auto quorum = GetTestQuorumHash(2); + CBLSSecretKey key; + key.MakeNewKey(); + const auto make_sig = [&](uint32_t message) { + const auto msg = GetTestQuorumHash(message); + return CRecoveredSig{type, quorum, id, msg, + key.Sign(SignHash{type, quorum, id, msg}.Get(), bls::bls_legacy_scheme.load())}; + }; + const auto now = GetTime(); + for (const bool same_message : {false, true}) { + const auto first = make_sig(3); + const auto second = make_sig(same_message ? 3 : 4); + const auto path = m_args.GetDataDirBase() / (same_message ? "refreshed_sigs" : "reinserted_sigs"); + { + CRecoveredSigsDb db({.path = path, .wipe = true}); + SetMockTime(now); + db.WriteRecoveredSig(first); + db.TruncateRecoveredSig(type, id); + SetMockTime(now + 60); + db.WriteRecoveredSig(second); + } + { + CRecoveredSigsDb db({.path = path}); + SetMockTime(now + 101); + db.CleanupOldRecoveredSigs(100); + CRecoveredSig stored; + BOOST_REQUIRE(db.GetRecoveredSigById(type, id, stored)); + BOOST_CHECK(stored.GetHash() == second.GetHash()); + BOOST_CHECK(db.HasRecoveredSigForHash(second.GetHash())); + BOOST_CHECK(db.HasRecoveredSigForSession(second.buildSignHash().Get())); + if (!same_message) { + BOOST_CHECK(!db.HasRecoveredSigForHash(first.GetHash())); + BOOST_CHECK(!db.HasRecoveredSigForSession(first.buildSignHash().Get())); + } + SetMockTime(now + 161); + db.CleanupOldRecoveredSigs(100); + BOOST_CHECK(!db.HasRecoveredSigForId(type, id)); + BOOST_CHECK(!db.HasRecoveredSigForHash(second.GetHash())); + BOOST_CHECK(!db.HasRecoveredSigForSession(second.buildSignHash().Get())); + } + } + SetMockTime(0); +} + +BOOST_AUTO_TEST_CASE(legacy_recovered_sig_markers_migrate_with_bounded_grace) +{ + const auto type = Params().GetConsensus().llmqTypeChainLocks; + const auto path = m_args.GetDataDirBase() / "legacy_markers"; + const auto db_path = path / "llmq" / "recsigdb"; + const auto now = GetTime(); + // Serialized legacy index records can outlive both the signature and its old rs_t entry. + { + auto legacy = util::MakeDbWrapper({.path = db_path, .wipe = true}); + CDBBatch batch{*legacy}; + for (uint32_t i = 1; i <= 3; ++i) { + const auto hash = GetTestQuorumHash(i); + batch.Write(std::make_pair(std::string("rs_h"), hash), std::make_pair(type, hash)); + batch.Write(std::make_pair(std::string("rs_s"), hash), uint8_t{1}); + } + BOOST_REQUIRE(legacy->WriteBatch(batch)); + } + { + CRecoveredSigsDb db({.path = path}); + SetMockTime(now); + db.CleanupOldRecoveredSigs(100); + for (uint32_t i = 1; i <= 3; ++i) { + BOOST_CHECK(db.HasRecoveredSigForHash(GetTestQuorumHash(i))); + BOOST_CHECK(db.HasRecoveredSigForSession(GetTestQuorumHash(i))); + } + SetMockTime(now + 101); + db.CleanupOldRecoveredSigs(100); + for (uint32_t i = 1; i <= 3; ++i) { + BOOST_CHECK(!db.HasRecoveredSigForHash(GetTestQuorumHash(i))); + BOOST_CHECK(!db.HasRecoveredSigForSession(GetTestQuorumHash(i))); + } + } + // A later legacy writer must be adopted again, even after an earlier sweep completed. + const auto later = GetTestQuorumHash(2000); + { + auto legacy = util::MakeDbWrapper({.path = db_path}); + legacy->Write(std::make_pair(std::string("rs_h"), later), std::make_pair(type, later)); + legacy->Write(std::make_pair(std::string("rs_s"), later), uint8_t{1}); + } + { + CRecoveredSigsDb db({.path = path}); + SetMockTime(now + 300); + db.CleanupOldRecoveredSigs(100); + BOOST_CHECK(db.HasRecoveredSigForHash(later)); + BOOST_CHECK(db.HasRecoveredSigForSession(later)); + SetMockTime(now + 401); + db.CleanupOldRecoveredSigs(100); + BOOST_CHECK(!db.HasRecoveredSigForHash(later)); + BOOST_CHECK(!db.HasRecoveredSigForSession(later)); + } + SetMockTime(0); +} + BOOST_AUTO_TEST_CASE(trivially_passes) { BOOST_CHECK(true); } static CSigSesAnn MakeSigSesAnn(uint32_t session_id, uint32_t nonce, Consensus::LLMQType llmq_type = Consensus::LLMQType::LLMQ_50_60) From c1868275eafe9c12e7e5dd95e34c77be64c04851 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Sun, 11 Oct 2026 01:20:28 +0700 Subject: [PATCH 2/2] refactor: drop the persisted cursor from the legacy marker scan MigrateLegacyMarkers kept its resume point in the database under rs_c and restarted from the beginning once a prefix was scanned through, so every cleanup pass kept re-reading 2000 marker values forever. The cycling existed only to adopt legacy values written by an older binary after a downgrade, and switching binaries always restarts the process. Keep the resume point per prefix in memory instead and drop the prefix once the cursor leaves it, so each process scans the markers once. The resume point is the next key to examine rather than the last one examined, so a deleted entry needs no equal-key skip: the seek lands on its successor. --- src/llmq/signing.cpp | 26 ++++++++------------------ src/llmq/signing.h | 3 +++ 2 files changed, 11 insertions(+), 18 deletions(-) diff --git a/src/llmq/signing.cpp b/src/llmq/signing.cpp index 126b5c91c41d..c11f1b531238 100644 --- a/src/llmq/signing.cpp +++ b/src/llmq/signing.cpp @@ -269,26 +269,16 @@ std::optional ReadRecoveredSigMarker(const CDBWrapper& db, c void CRecoveredSigsDb::MigrateLegacyMarkers(const std::string& prefix) { AssertLockHeld(cs_cache); - const auto cursor_key = std::make_pair(std::string("rs_c"), prefix); - uint256 last; - const bool have_cursor = db->Read(cursor_key, last); + const auto resume = m_legacy_scan_resume.find(prefix); + if (resume == m_legacy_scan_resume.end()) return; std::unique_ptr cursor{db->NewIterator()}; - auto start = std::make_pair(prefix, last); + auto start = std::make_pair(prefix, resume->second); cursor->Seek(start); - if (have_cursor && cursor->Valid()) { - decltype(start) key; - if (cursor->GetKey(key) && key == start) cursor->Next(); - } CDBBatch batch(*db); - bool finished{false}; const uint32_t now = GetTime().count(); for (size_t examined{0}; examined < MAX_CLEANUP_ENTRIES && cursor->Valid(); ++examined, cursor->Next()) { decltype(start) key; - if (!cursor->GetKey(key) || key.first != prefix) { - finished = true; - break; - } - last = key.second; + if (!cursor->GetKey(key) || key.first != prefix) break; const auto marker = ReadRecoveredSigMarker(*db, prefix, key.second); if (!marker || marker->time) continue; // Uncorrelated legacy session markers have no recoverable age; retain one grace interval. @@ -299,10 +289,11 @@ void CRecoveredSigsDb::MigrateLegacyMarkers(const std::string& prefix) } batch.Write(std::make_tuple(std::string("rs_e"), htobe32_internal(now), prefix, key.second), uint8_t{1}); } - if (finished || !cursor->Valid()) { - batch.Erase(cursor_key); + decltype(start) next; + if (cursor->Valid() && cursor->GetKey(next) && next.first == prefix) { + resume->second = next.second; } else { - batch.Write(cursor_key, last); + m_legacy_scan_resume.erase(resume); } db->WriteBatch(batch); } @@ -338,7 +329,6 @@ void CRecoveredSigsDb::CleanupOldMarkers(int64_t maxAge) void CRecoveredSigsDb::CleanupOldRecoveredSigs(int64_t maxAge) { LOCK(cs_cache); - // Cycle the bounded scans so downgrade-written legacy values are adopted on re-upgrade too. MigrateLegacyMarkers("rs_h"); MigrateLegacyMarkers("rs_s"); CleanupOldMarkers(maxAge); diff --git a/src/llmq/signing.h b/src/llmq/signing.h index 0b11ac210553..b2ee2d22ba00 100644 --- a/src/llmq/signing.h +++ b/src/llmq/signing.h @@ -121,6 +121,9 @@ class CRecoveredSigsDb mutable unordered_lru_cache, bool, StaticSaltedHasher, 30000> hasSigForIdCache GUARDED_BY(cs_cache); mutable Uint256LruHashMap hasSigForSessionCache GUARDED_BY(cs_cache); mutable Uint256LruHashMap hasSigForHashCache GUARDED_BY(cs_cache); + // Legacy marker values only appear after a downgrade, which needs a restart, so one bounded scan per + // process adopts them all. A prefix is dropped once scanned through. + std::unordered_map m_legacy_scan_resume GUARDED_BY(cs_cache){{"rs_h", {}}, {"rs_s", {}}}; public: explicit CRecoveredSigsDb(const util::DbWrapperParams& db_params);