Skip to content

LLMQ: Bound pending signature shares #1930

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

Open
navidR wants to merge 1 commit into
base: master
Choose a base branch
from
+353 −47
Open

LLMQ: Bound pending signature shares #1930

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 27 additions & 1 deletion src/llmq/quorums_signing_shares.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -538,11 +538,37 @@ bool CSigSharesManager::ProcessMessageBatchedSigShares(CNode* pfrom, const CBatc
}
auto& nodeState = it->second;
for (auto& s : sigShares) {
nodeState.pendingIncomingSigShares.Add(s.GetKey(), s);
TryAddPendingIncomingSigShare(pfrom->id, nodeState, s);

@codeant-ai codeant-ai Bot Aug 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion: The return value is ignored, so a cap-rejected share is permanently dropped after its request was cleared and its announcement bit was consumed. [incomplete implementation]

Assessment: 🟠 Major · 🔁 Occurrence: Sometimes

Use CodeAnt Skill Fix in Cursor Fix in VSCode Claude

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** src/llmq/quorums_signing_shares.cpp
**Line:** 541:541
**Comment:**
	*Incomplete Implementation: The return value is ignored, so a cap-rejected share is permanently dropped after its request was cleared and its announcement bit was consumed.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

}
return true;
}

bool CSigSharesManager::TryAddPendingIncomingSigShare(NodeId nodeId, CSigSharesNodeState& nodeState, const CSigShare& sigShare)
{
AssertLockHeld(cs);

if (nodeState.banned) {
return false;
}
if (nodeState.pendingIncomingSigShares.Size() >= MAX_PENDING_SIG_SHARES_PER_NODE) {
LogPrint("llmq-sigs", "CSigSharesManager::%s -- per-node pending sig shares cap reached (%d), dropping sigShare. node=%d\n",
__func__, MAX_PENDING_SIG_SHARES_PER_NODE, nodeId);
return false;
}

size_t total{0};
for (const auto& p : nodeStates) {
total += p.second.pendingIncomingSigShares.Size();
}
Comment on lines +559 to +562

@codeant-ai codeant-ai Bot Aug 30, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion: Each share scans every node while holding cs; a peer can send repeated batches and cause expensive lock-held work that delays other signing messages. [performance]

Assessment: 🟠 Major · 🔁 Occurrence: Sometimes

Use CodeAnt Skill Fix in Cursor Fix in VSCode Claude

Prompt for AI Agent 🤖 
This is a comment left during a code review.

**Path:** src/llmq/quorums_signing_shares.cpp
**Line:** 559:562
**Comment:**
	*Performance: Each share scans every node while holding `cs`; a peer can send repeated batches and cause expensive lock-held work that delays other signing messages.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

if (total >= MAX_PENDING_SIG_SHARES_TOTAL) {
LogPrint("llmq-sigs", "CSigSharesManager::%s -- global pending sig shares cap reached (%d), dropping sigShare. node=%d\n",
__func__, MAX_PENDING_SIG_SHARES_TOTAL, nodeId);
return false;
}

return nodeState.pendingIncomingSigShares.Add(sigShare.GetKey(), sigShare);
}

bool CSigSharesManager::PreVerifyBatchedSigShares(NodeId nodeId, const CSigSharesNodeState::SessionInfo& session, const CBatchedSigShares& batchedSigShares, bool& retBan)
{
retBan = false;
Expand Down
168 changes: 122 additions & 46 deletions src/llmq/quorums_signing_shares.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ class CScheduler;

namespace llmq
{
struct CSigSharesManagerTestAccess;

// <signHash, quorumMember>
typedef std::pair<uint256, uint16_t> SigShareKey;

Expand Down Expand Up @@ -138,49 +140,135 @@ class CBatchedSigShares
std::string ToInvString() const;
};

template<typename T>
class SigShareMap
/**
* Two-level (signHash -> quorumMember) map with a running entry count, so Size() is O(1)
* instead of a fold over all sign hash buckets. All structural mutations go through the
* counted methods; Buckets() is for lookups and in-place value updates only.
*/
template <typename T>
class CountedBucketMap
{
public:
using BucketMap = std::unordered_map<uint256, std::unordered_map<uint16_t, T>, StaticSaltedHasher>;

private:
std::unordered_map<uint256, std::unordered_map<uint16_t, T>, StaticSaltedHasher> internalMap;
BucketMap m_data;
size_t m_num_entries{0};

public:
bool Add(const SigShareKey& k, const T& v)
BucketMap& Buckets()
{
auto& m = internalMap[k.first];
return m.emplace(k.second, v).second;
return m_data;
}

const BucketMap& Buckets() const
{
return m_data;
}

size_t Size() const
{
return m_num_entries;
}

bool Emplace(const SigShareKey& k, const T& v)
{
if (!m_data[k.first].emplace(k.second, v).second) {
return false;
}
++m_num_entries;
return true;
}

void Erase(const SigShareKey& k)
{
auto it = internalMap.find(k.first);
if (it == internalMap.end()) {
auto it = m_data.find(k.first);
if (it == m_data.end()) {
return;
}
it->second.erase(k.second);
m_num_entries -= it->second.erase(k.second);
if (it->second.empty()) {
internalMap.erase(it);
m_data.erase(it);
}
}

void EraseBucket(const uint256& signHash)
{
auto it = m_data.find(signHash);
if (it == m_data.end()) {
return;
}
m_num_entries -= it->second.size();
m_data.erase(it);
}

template <typename F>
void EraseIf(F&& f)
{
for (auto it = m_data.begin(); it != m_data.end();) {
SigShareKey k;
k.first = it->first;
for (auto jt = it->second.begin(); jt != it->second.end();) {
k.second = jt->first;
if (f(k, jt->second)) {
jt = it->second.erase(jt);
--m_num_entries;
} else {
++jt;
}
}
if (it->second.empty()) {
it = m_data.erase(it);
} else {
++it;
}
}
}

void Clear()
{
internalMap.clear();
m_data.clear();
m_num_entries = 0;
}
};

template <typename T>
class SigShareMap
{
private:
CountedBucketMap<T> internalMap;

public:
bool Add(const SigShareKey& k, const T& v)
{
return internalMap.Emplace(k, v);
}

void Erase(const SigShareKey& k)
{
internalMap.Erase(k);
}

void Clear()
{
internalMap.Clear();
}

bool Has(const SigShareKey& k) const
{
auto it = internalMap.find(k.first);
if (it == internalMap.end()) {
const auto& buckets = internalMap.Buckets();
auto it = buckets.find(k.first);
if (it == buckets.end()) {
return false;
}
return it->second.count(k.second) != 0;
}

T* Get(const SigShareKey& k)
{
auto it = internalMap.find(k.first);
if (it == internalMap.end()) {
auto& buckets = internalMap.Buckets();
auto it = buckets.find(k.first);
if (it == buckets.end()) {
return nullptr;
}

Expand All @@ -204,75 +292,58 @@ class SigShareMap

const T* GetFirst() const
{
if (internalMap.empty()) {
const auto& buckets = internalMap.Buckets();
if (buckets.empty()) {
return nullptr;
}
return &internalMap.begin()->second.begin()->second;
return &buckets.begin()->second.begin()->second;
}

size_t Size() const
{
size_t s = 0;
for (auto& p : internalMap) {
s += p.second.size();
}
return s;
return internalMap.Size();
}

size_t CountForSignHash(const uint256& signHash) const
{
auto it = internalMap.find(signHash);
if (it == internalMap.end()) {
const auto& buckets = internalMap.Buckets();
auto it = buckets.find(signHash);
if (it == buckets.end()) {
return 0;
}
return it->second.size();
}

bool Empty() const
{
return internalMap.empty();
return internalMap.Buckets().empty();
}

const std::unordered_map<uint16_t, T>* GetAllForSignHash(const uint256& signHash)
{
auto it = internalMap.find(signHash);
if (it == internalMap.end()) {
const auto& buckets = internalMap.Buckets();
auto it = buckets.find(signHash);
if (it == buckets.end()) {
return nullptr;
}
return &it->second;
}

void EraseAllForSignHash(const uint256& signHash)
{
internalMap.erase(signHash);
internalMap.EraseBucket(signHash);
}

template<typename F>
void EraseIf(F&& f)
{
for (auto it = internalMap.begin(); it != internalMap.end(); ) {
SigShareKey k;
k.first = it->first;
for (auto jt = it->second.begin(); jt != it->second.end(); ) {
k.second = jt->first;
if (f(k, jt->second)) {
jt = it->second.erase(jt);
} else {
++jt;
}
}
if (it->second.empty()) {
it = internalMap.erase(it);
} else {
++it;
}
}
internalMap.EraseIf(f);
}

template<typename F>
void ForEach(F&& f)
{
for (auto& p : internalMap) {
for (auto& p : internalMap.Buckets()) {
SigShareKey k;
k.first = p.first;
for (auto& p2 : p.second) {
Expand Down Expand Up @@ -342,8 +413,12 @@ class CSigSharesNodeState

class CSigSharesManager : public CRecoveredSigsListener
{
friend struct CSigSharesManagerTestAccess;

static const int64_t SESSION_NEW_SHARES_TIMEOUT = 60;
static const int64_t SIG_SHARE_REQUEST_TIMEOUT = 5;
static constexpr size_t MAX_PENDING_SIG_SHARES_PER_NODE{1000};
static constexpr size_t MAX_PENDING_SIG_SHARES_TOTAL{10000};

// we try to keep total message size below 10k
const size_t MAX_MSGS_CNT_QSIGSESANN = 100;
Expand Down Expand Up @@ -421,6 +496,7 @@ class CSigSharesManager : public CRecoveredSigsListener
private:
bool GetSessionInfoByRecvId(NodeId nodeId, uint32_t sessionId, CSigSharesNodeState::SessionInfo& retInfo);
CSigShare RebuildSigShare(const CSigSharesNodeState::SessionInfo& session, const CBatchedSigShares& batchedSigShares, size_t idx);
bool TryAddPendingIncomingSigShare(NodeId nodeId, CSigSharesNodeState& nodeState, const CSigShare& sigShare);

void Cleanup();
void RemoveSigSharesForSession(const uint256& signHash);
Expand Down
1 change: 1 addition & 0 deletions src/test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ add_executable(test_firo
${CMAKE_CURRENT_SOURCE_DIR}/evospork_tests.cpp
${CMAKE_CURRENT_SOURCE_DIR}/evo_deterministicmns_tests.cpp
${CMAKE_CURRENT_SOURCE_DIR}/evo_simplifiedmns_tests.cpp
${CMAKE_CURRENT_SOURCE_DIR}/quorums_signing_pending_tests.cpp
${CMAKE_CURRENT_SOURCE_DIR}/progpow_tests.cpp
${CMAKE_CURRENT_SOURCE_DIR}/bls_tests.cpp
${CMAKE_CURRENT_SOURCE_DIR}/sparkmessage_tests.cpp
Expand Down
Loading
Loading