Skip to content

fix: expire truncated recovered-signature markers #7844

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
PastaPastaPasta merged 2 commits into from
Oct 14, 2026
Merged

fix: expire truncated recovered-signature markers #7844

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ff829d4
fix: expire truncated recovered-signature markers
PastaPastaPasta Oct 7, 2026
c186827
refactor: drop the persisted cursor from the legacy marker scan
knst Oct 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
255 changes: 151 additions & 104 deletions src/llmq/signing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,19 @@
#include <util/system.h>

#include <algorithm>
#include <optional>
#include <ranges>
#include <unordered_map>
#include <unordered_set>

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})}
{
Expand All @@ -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;
}
Expand Down Expand Up @@ -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<std::chrono::seconds>().count();
Expand All @@ -166,70 +165,44 @@ 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<uint8_t>(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
// This will leave the byHash and signHash key in-place so that HasRecoveredSigForHash /
// 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);
}

Expand All @@ -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<uint32_t>(GetTime<std::chrono::seconds>().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)) {
Expand All @@ -263,56 +231,135 @@ void CRecoveredSigsDb::CleanupOldPlatformSigs(int64_t maxAge)
db->WriteBatch(batch);
}

void CRecoveredSigsDb::CleanupOldRecoveredSigs(int64_t maxAge)
{
CleanupOldPlatformSigs(maxAge);
std::unique_ptr<CDBIterator> pcursor(db->NewIterator());

auto start = std::make_tuple(std::string("rs_t"), static_cast<uint32_t>(0), static_cast<Consensus::LLMQType>(0), uint256());
uint32_t endTime = static_cast<uint32_t>(GetTime<std::chrono::seconds>().count() - maxAge);
pcursor->Seek(start);

std::vector<std::pair<Consensus::LLMQType, uint256>> toDelete;
std::vector<decltype(start)> toDelete2;
namespace {
struct RecoveredSigMarker {
std::pair<Consensus::LLMQType, uint256> owner{};
std::optional<uint32_t> time;
};

while (pcursor->Valid()) {
decltype(start) k;

if (!pcursor->GetKey(k) || std::get<0>(k) != "rs_t") {
break;
}
if (be32toh_internal(std::get<1>(k)) >= endTime) {
break;
std::optional<RecoveredSigMarker> 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;
}

toDelete.emplace_back(std::get<2>(k), std::get<3>(k));
toDelete2.emplace_back(k);

pcursor->Next();
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;
}
pcursor.reset();
}
} // namespace

if (toDelete.empty()) {
return;
void CRecoveredSigsDb::MigrateLegacyMarkers(const std::string& prefix)
{
AssertLockHeld(cs_cache);
const auto resume = m_legacy_scan_resume.find(prefix);
if (resume == m_legacy_scan_resume.end()) return;
std::unique_ptr<CDBIterator> cursor{db->NewIterator()};
auto start = std::make_pair(prefix, resume->second);
cursor->Seek(start);
CDBBatch batch(*db);
const uint32_t now = GetTime<std::chrono::seconds>().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) 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.
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});
}
decltype(start) next;
if (cursor->Valid() && cursor->GetKey(next) && next.first == prefix) {
resume->second = next.second;
} else {
m_legacy_scan_resume.erase(resume);
}
db->WriteBatch(batch);
}

void CRecoveredSigsDb::CleanupOldMarkers(int64_t maxAge)
{
AssertLockHeld(cs_cache);
std::unique_ptr<CDBIterator> cursor{db->NewIterator()};
auto start = std::make_tuple(std::string("rs_e"), uint32_t{0}, std::string{}, uint256{});
const auto end_time = static_cast<uint32_t>(GetTime<std::chrono::seconds>().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()) {

@knst knst Oct 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if examined every time only MAX_CLEANUP_ENTRIES entries and all of them valid, even if MAX_CLEANUP_ENTRIES * 100 entries exist they will never be cleaned up by this loop because cursor->Next() goes consequently and start is always 0second.

UPD: it seems irrelevant. Check this one instead: cdff273

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);
MigrateLegacyMarkers("rs_h");
MigrateLegacyMarkers("rs_s");
CleanupOldMarkers(maxAge);
CleanupOldPlatformSigs(maxAge);
std::unique_ptr<CDBIterator> cursor{db->NewIterator()};
auto start = std::make_tuple(std::string("rs_t"), uint32_t{0}, static_cast<Consensus::LLMQType>(0), uint256{});
const auto end_time = static_cast<uint32_t>(GetTime<std::chrono::seconds>().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;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
batch.Erase(key);
Comment on lines +348 to +355

@thepastaclaw thepastaclaw Oct 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Withdrawn at ff829d4f; see the replies below.

🔴 Blocking: Preserve expiry for historical one-byte rs_r values

Commit 8c49d9b changed the four-part rs_r value from uint8_t{1} to a uint32_t timestamp. For a persisted object with that historical one-byte value, the new db->Read(..., write_time) fails because CDBWrapper::Read catches the short-stream exception and returns false. Cleanup therefore skips RemoveRecoveredSig but still erases the expired rs_t entry. Unlike the base implementation, which removed expired objects without deserializing this value, the new implementation permanently strands the by-id object and message index. Marker migration only visits rs_h/rs_s, so subsequent cleanup and database reopen do not repair this; HasRecoveredSigForId and non-Platform HasRecoveredSigForSigning continue returning true. Recognize the exact one-byte legacy value 1 when processing an expired rs_t entry, while retaining timestamp equality for current four-byte values. Add a persisted legacy-object regression with repeated cleanup, reopen, invalid-format controls, and refreshed-object protection.

source: muse-spark-1.3-contributor (phase1-reviewer: general); gpt-6.1-sol (phase2-reviewer: general)

@PastaPastaPasta PastaPastaPasta Oct 7, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not changing this; the branch was removed on purpose. The one-byte rs_r value was only written by pre-v0.14.1 nodes (8c49d9b, 2019), and that same commit started writing an rs_t entry for every record. Every binary since then has run CleanupOldRecoveredSigs, which deleted those records unconditionally about 7 days after they were written, so no database that has run any post-2019 node still holds one. Develop also dropped the <v0.14.1 compatibility path deliberately in 8058867. The only way to reach this state is opening a 2019 LLMQ database with no intermediate version, which isn't a supported upgrade path, and a short rs_r value makes CDBWrapper::Read return false cleanly rather than misbehave.

🤖 Posted autonomously by Claude on behalf of pasta.

@thepastaclaw thepastaclaw Oct 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked 8c49d9b5 and 8058867b: the timestamp format dates to 2019, and the latter explicitly removed compatibility with upgrades from before v0.14.1. The short-read behavior I described is real, but I was wrong to treat that unsupported upgrade state as a compatibility requirement for this PR. The blocking classification was unjustified, and I’m withdrawing the finding.

Withdrawing this finding.

@thepastaclaw thepastaclaw Oct 7, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Withdrawn (re-reviewed at ff829d4f): Your supported-upgrade boundary is corroborated by 8058867, already in the base, which deliberately removed the pre-v0.14.1 compatibility guard. Although the historical one-byte value fails the new timestamp read, CDBWrapper::Read handles that cleanly, and I withdraw the request to restore support for that obsolete format.

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
Expand Down
Loading
Loading