Skip to content

fix: limit signing share sessions per peer #7351

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
thepastaclaw wants to merge 6 commits into
base: develop
Choose a base branch
from
+135 −2
Open

fix: limit signing share sessions per peer #7351

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e5e8d1e
fix: limit signing share sessions per peer
thepastaclaw Jun 8, 2026
135f8ce
fix: refine signing share session cap
thepastaclaw Jun 9, 2026
6595948
test: avoid signing share Makefile conflict
thepastaclaw Jun 9, 2026
f6aa12a
fix: preserve signing share session expiry on refresh
thepastaclaw Jun 9, 2026
03dddee
fix: count only announced signing share sessions
thepastaclaw Jun 10, 2026
76a6fd7
fix: ignore excess signing share sessions
thepastaclaw Jun 16, 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
50 changes: 49 additions & 1 deletion src/llmq/signing_shares.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,21 @@

#include <cxxtimer.hpp>

#include <algorithm>
#include <ranges>

namespace llmq
{
namespace {
constexpr size_t MAX_SESSIONS_PER_PEER_FACTOR{4};
constexpr size_t MIN_SESSIONS_PER_PEER{100};
Comment on lines +31 to +32

@thepastaclaw thepastaclaw Jun 16, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

💬 Nitpick: Document the cap factor and floor

MAX_SESSIONS_PER_PEER_FACTOR{4} and MIN_SESSIONS_PER_PEER{100} lack any rationale. With LLMQ_400_* (size=400) the per-peer cap is 1600 announcement-derived sessions; each Session carries three quorum-sized CSigSharesInv bitsets plus several uint256s and a CQuorumCPtr. A short comment tying these constants to the threat model in the PR description would help future maintainers reason about adjustments and explain why the 100 floor exists even though no current LLMQ size requires it.

source: ['claude']


size_t GetMaxSessionsForPeer(const Consensus::LLMQParams& params)
{
return std::max<size_t>(size_t(params.size) * MAX_SESSIONS_PER_PEER_FACTOR, MIN_SESSIONS_PER_PEER);
}
} // namespace

void CSigShare::UpdateKey()
{
key.first = this->buildSignHash().Get();
Expand Down Expand Up @@ -130,9 +141,32 @@ CSigSharesNodeState::Session& CSigSharesNodeState::GetOrCreateSessionFromAnn(con
if (s.announced.inv.empty()) {
InitSession(s, signHash, ann);
}
s.receivedAnnouncement = true;
return s;
}

bool CSigSharesNodeState::CanCreateSessionFromAnn(const llmq::CSigSesAnn& ann, size_t maxSessions) const
{
return sessions.count(ann.buildSignHash().Get()) != 0 || GetAnnouncementSessionCount(ann.getLlmqType()) < maxSessions;
}

size_t CSigSharesNodeState::GetSessionCount() const
{
return sessions.size();
}

size_t CSigSharesNodeState::GetSessionCount(Consensus::LLMQType llmqType) const
{
return std::ranges::count_if(sessions, [&](const auto& kv) { return kv.second.llmqType == llmqType; });
}

size_t CSigSharesNodeState::GetAnnouncementSessionCount(Consensus::LLMQType llmqType) const
{
return std::ranges::count_if(sessions, [&](const auto& kv) {
return kv.second.receivedAnnouncement && kv.second.llmqType == llmqType;
});
}
Comment on lines +148 to +168

@thepastaclaw thepastaclaw Jun 16, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

💬 Nitpick: Linear scan per announcement, recomputed on rejection

GetAnnouncementSessionCount(llmqType) is O(n) over sessions and runs from CanCreateSessionFromAnn on every QSIGSESANN. On the rejection path it is then recomputed for the LogPrint at line 266, doubling the cost exactly when a peer is being throttled. A per-llmq-type counter on CSigSharesNodeState (maintained when receivedAnnouncement transitions false→true and on session removal) would make admission O(1). At minimum, cache the count in a local in ProcessMessageSigSesAnn so the rejection log reuses it.

source: ['claude']


CSigSharesNodeState::Session* CSigSharesNodeState::GetSessionBySignHash(const uint256& signHash)
{
auto it = sessions.find(signHash);
Expand Down Expand Up @@ -206,7 +240,8 @@ void CSigSharesManager::UnregisterRecoveryInterface()
bool CSigSharesManager::ProcessMessageSigSesAnn(const CNode& pfrom, const CSigSesAnn& ann)
{
auto llmqType = ann.getLlmqType();
if (!Params().GetLLMQ(llmqType).has_value()) {
const auto& llmq_params_opt = Params().GetLLMQ(llmqType);
if (!llmq_params_opt.has_value()) {
return false;
}
if (ann.getSessionId() == UNINITIALIZED_SESSION_ID || ann.getQuorumHash().IsNull() || ann.getId().IsNull() || ann.getMsgHash().IsNull()) {
Expand All @@ -225,7 +260,15 @@ bool CSigSharesManager::ProcessMessageSigSesAnn(const CNode& pfrom, const CSigSe

LOCK(cs);
auto& nodeState = nodeStates[pfrom.GetId()];
const size_t maxSessions = GetMaxSessionsForPeer(*llmq_params_opt);
if (!nodeState.CanCreateSessionFromAnn(ann, maxSessions)) {
LogPrint(BCLog::LLMQ_SIGS, "CSigSharesManager::%s -- too many sessions. cnt=%d, max=%d, llmqType=%d, node=%d\n",
__func__, nodeState.GetAnnouncementSessionCount(llmqType), maxSessions, static_cast<int>(llmqType), pfrom.GetId());
return false;

@UdjinM6 UdjinM6 Jun 16, 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.

Not sure we should ban here. Wouldn't ignoring it be safer?

Suggested change
return false;
return true;

}
Comment on lines 260 to +268

@thepastaclaw thepastaclaw Jun 16, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

💬 Nitpick: Quorum lookup precedes the cap check

qman.GetQuorum(llmqType, ann.getQuorumHash()) runs at line 253, before the per-peer cap check at line 264. A peer flooding QSIGSESANN with valid quorum hashes still pays the quorum-lookup cost on every announcement after it has been throttled. Moving the cap gate ahead of the quorum resolution (the gate only needs the llmqType and the existing cs lock) would make the throttle cheaper to enforce. Minor sequencing improvement that compounds with addressing the linear scan above.

source: ['claude']

const uint256 signHash = ann.buildSignHash().Get();
auto& session = nodeState.GetOrCreateSessionFromAnn(ann);
timeSeenForSessions.try_emplace(signHash, GetTime<std::chrono::seconds>().count());
nodeState.sessionByRecvId.erase(session.recvSessionId);
nodeState.sessionByRecvId.erase(ann.getSessionId());
session.recvSessionId = ann.getSessionId();
Expand Down Expand Up @@ -1247,6 +1290,11 @@ void CSigSharesManager::Cleanup()
doneSessions.emplace(sigShare.GetSignHash());
}
});
for (const auto& [signHash, _] : timeSeenForSessions) {
if (doneSessions.count(signHash) == 0 && sigman.HasRecoveredSigForSession(signHash)) {
doneSessions.emplace(signHash);
}
}
for (const auto& signHash : doneSessions) {
RemoveSigSharesForSession(signHash);
}
Expand Down
7 changes: 6 additions & 1 deletion src/llmq/signing_shares.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,8 +326,9 @@ class CSigSharesNodeState
CSigSharesInv announced;
CSigSharesInv requested;
CSigSharesInv knows;

bool receivedAnnouncement{false};
};
// TODO limit number of sessions per node
Uint256HashMap<Session> sessions;

std::unordered_map<uint32_t, Session*> sessionByRecvId;
Expand All @@ -339,6 +340,10 @@ class CSigSharesNodeState

Session& GetOrCreateSessionFromShare(const CSigShare& sigShare);
Session& GetOrCreateSessionFromAnn(const CSigSesAnn& ann);
[[nodiscard]] bool CanCreateSessionFromAnn(const CSigSesAnn& ann, size_t maxSessions) const;
[[nodiscard]] size_t GetSessionCount() const;
[[nodiscard]] size_t GetSessionCount(Consensus::LLMQType llmqType) const;
[[nodiscard]] size_t GetAnnouncementSessionCount(Consensus::LLMQType llmqType) const;
Session* GetSessionBySignHash(const uint256& signHash);
Session* GetSessionByRecvId(uint32_t sessionId);
bool GetSessionInfoByRecvId(uint32_t sessionId, SessionInfo& retInfo);
Expand Down
80 changes: 80 additions & 0 deletions src/test/llmq_utils_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include <consensus/params.h>
#include <llmq/params.h>
#include <llmq/signing_shares.h>
#include <llmq/utils.h>
#include <netaddress.h>
#include <random.h>
Expand All @@ -23,6 +24,85 @@ BOOST_FIXTURE_TEST_SUITE(llmq_utils_tests, BasicTestingSetup)

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)
{
return CSigSesAnn{session_id, llmq_type, GetTestQuorumHash(1), GetTestQuorumHash(2), GetTestQuorumHash(nonce)};
}

static CSigShare MakeSigShare(uint32_t nonce, Consensus::LLMQType llmq_type = Consensus::LLMQType::LLMQ_50_60)
{
CSigShare sig_share{llmq_type, GetTestQuorumHash(1), GetTestQuorumHash(2), GetTestQuorumHash(nonce), 1, CBLSLazySignature{}};
sig_share.UpdateKey();
return sig_share;
}

BOOST_AUTO_TEST_CASE(sig_ses_ann_respects_session_limit_but_allows_refresh)
{
CSigSharesNodeState node_state;

const CSigSesAnn ann1{MakeSigSesAnn(1, 1)};
const CSigSesAnn ann2{MakeSigSesAnn(2, 2)};
const CSigSesAnn ann3{MakeSigSesAnn(3, 3)};
constexpr size_t max_sessions{2};

BOOST_CHECK(node_state.CanCreateSessionFromAnn(ann1, max_sessions));
node_state.GetOrCreateSessionFromAnn(ann1);
BOOST_CHECK_EQUAL(node_state.GetSessionCount(), 1U);
BOOST_CHECK_EQUAL(node_state.GetAnnouncementSessionCount(Consensus::LLMQType::LLMQ_50_60), 1U);

BOOST_CHECK(node_state.CanCreateSessionFromAnn(ann2, max_sessions));
node_state.GetOrCreateSessionFromAnn(ann2);
BOOST_CHECK_EQUAL(node_state.GetSessionCount(), max_sessions);
BOOST_CHECK_EQUAL(node_state.GetAnnouncementSessionCount(Consensus::LLMQType::LLMQ_50_60), max_sessions);

BOOST_CHECK(!node_state.CanCreateSessionFromAnn(ann3, max_sessions));

const CSigSesAnn ann1_refresh{4, Consensus::LLMQType::LLMQ_50_60, ann1.getQuorumHash(), ann1.getId(), ann1.getMsgHash()};
BOOST_CHECK(node_state.CanCreateSessionFromAnn(ann1_refresh, max_sessions));
node_state.GetOrCreateSessionFromAnn(ann1_refresh);
BOOST_CHECK_EQUAL(node_state.GetSessionCount(), max_sessions);
BOOST_CHECK_EQUAL(node_state.GetAnnouncementSessionCount(Consensus::LLMQType::LLMQ_50_60), max_sessions);
}

BOOST_AUTO_TEST_CASE(sig_ses_ann_limit_ignores_send_only_sessions)
{
CSigSharesNodeState node_state;

constexpr size_t max_sessions{1};
const CSigShare sig_share{MakeSigShare(1)};
const CSigSesAnn ann{MakeSigSesAnn(1, 2)};

node_state.GetOrCreateSessionFromShare(sig_share);
BOOST_CHECK_EQUAL(node_state.GetSessionCount(Consensus::LLMQType::LLMQ_50_60), 1U);
BOOST_CHECK_EQUAL(node_state.GetAnnouncementSessionCount(Consensus::LLMQType::LLMQ_50_60), 0U);

BOOST_CHECK(node_state.CanCreateSessionFromAnn(ann, max_sessions));
node_state.GetOrCreateSessionFromAnn(ann);
BOOST_CHECK_EQUAL(node_state.GetSessionCount(Consensus::LLMQType::LLMQ_50_60), 2U);
BOOST_CHECK_EQUAL(node_state.GetAnnouncementSessionCount(Consensus::LLMQType::LLMQ_50_60), 1U);
}

BOOST_AUTO_TEST_CASE(sig_ses_ann_limit_is_per_llmq_type)
{
CSigSharesNodeState node_state;

constexpr size_t max_sessions{1};
const CSigSesAnn ann1{MakeSigSesAnn(1, 1)};
const CSigSesAnn ann2{MakeSigSesAnn(2, 2)};
const CSigSesAnn other_type_ann{MakeSigSesAnn(3, 3, Consensus::LLMQType::LLMQ_400_60)};

BOOST_CHECK(node_state.CanCreateSessionFromAnn(ann1, max_sessions));
node_state.GetOrCreateSessionFromAnn(ann1);
BOOST_CHECK_EQUAL(node_state.GetSessionCount(), 1U);
BOOST_CHECK_EQUAL(node_state.GetSessionCount(Consensus::LLMQType::LLMQ_50_60), 1U);

BOOST_CHECK(!node_state.CanCreateSessionFromAnn(ann2, max_sessions));
BOOST_CHECK(node_state.CanCreateSessionFromAnn(other_type_ann, max_sessions));
node_state.GetOrCreateSessionFromAnn(other_type_ann);
BOOST_CHECK_EQUAL(node_state.GetSessionCount(), 2U);
BOOST_CHECK_EQUAL(node_state.GetSessionCount(Consensus::LLMQType::LLMQ_400_60), 1U);
}

BOOST_AUTO_TEST_CASE(deterministic_outbound_connection_test)
{
// Test deterministic behavior
Expand Down
Loading