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

Merged
PastaPastaPasta merged 6 commits into from
Jun 25, 2026
Merged

fix: limit signing share sessions per peer #7351

Show file tree
Hide file tree
Changes from 4 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
42 changes: 41 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 @@ -133,6 +144,21 @@ CSigSharesNodeState::Session& CSigSharesNodeState::GetOrCreateSessionFromAnn(con
return s;
}

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

@chatgpt-codex-connector chatgpt-codex-connector Bot Jun 9, 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.

P2 Badge Count only announcement-created sessions in the cap

When this node already has maxSessions sessions for a peer that were created locally for outgoing traffic (for example CollectSigSharesToAnnounce calls GetOrCreateSessionFromShare for that peer before SendMessages assigns a send session), this count rejects the peer's first unrelated QSIGSESANN even though the new limit is meant to constrain sessions created from announcements. NetSigning::ProcessMessage treats this false return as a ban, so under a busy signing backlog an honest peer can be banned because our own send-side sessions filled the per-peer count; the cap should exclude send-only sessions or track announcement-created sessions separately.

Useful? React with 👍 / 👎.

@thepastaclaw thepastaclaw Jun 10, 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.

Addressed in 03dddee: the session cap now counts only sessions that were actually introduced by QSIGSESANN announcements. Send-only sessions created locally via sig shares no longer consume the announcement-session budget, while existing-session announcements are still accepted and then marked as announcement-backed.

Validation:

  • git diff --check
  • make -C src -j8 test/test_dash
  • src/test/test_dash --run_test=llmq_utils_tests --catch_system_errors=no

}

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; });
}

CSigSharesNodeState::Session* CSigSharesNodeState::GetSessionBySignHash(const uint256& signHash)
{
auto it = sessions.find(signHash);
Expand Down Expand Up @@ -206,7 +232,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 +252,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.GetSessionCount(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 +1282,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
4 changes: 3 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 @@ -327,7 +327,6 @@ class CSigSharesNodeState
CSigSharesInv requested;
CSigSharesInv knows;
};
// TODO limit number of sessions per node
Uint256HashMap<Session> sessions;

std::unordered_map<uint32_t, Session*> sessionByRecvId;
Expand All @@ -339,6 +338,9 @@ 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;
Session* GetSessionBySignHash(const uint256& signHash);
Session* GetSessionByRecvId(uint32_t sessionId);
bool GetSessionInfoByRecvId(uint32_t sessionId, SessionInfo& retInfo);
Expand Down
52 changes: 52 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,57 @@ 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)};
}

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(node_state.CanCreateSessionFromAnn(ann2, max_sessions));
node_state.GetOrCreateSessionFromAnn(ann2);
BOOST_CHECK_EQUAL(node_state.GetSessionCount(), 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_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