fix: limit signing share sessions per peer#7351
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThis PR enforces per-peer session limits for LLMQ signing shares. It adds GetMaxSessionsForPeer and related constants, four [[nodiscard]] CSigSharesNodeState query methods, and marks sessions that received announcements. ProcessMessageSigSesAnn now fetches LLMQ params, computes a per-peer maxSessions, rejects announcements exceeding the cap with logging, and records time-seen entries for accepted sign hashes. Cleanup now also considers sign hashes in timeSeenForSessions when removing recovered sessions. Unit tests verify limit enforcement, refresh semantics, send-only session exclusions, and per-LLMQType scoping. Sequence Diagram(s)sequenceDiagram
participant Peer
participant CSigSharesManager
participant CSigSharesNodeState
participant TimeSeenStore
Peer->>CSigSharesManager: CSigSesAnn(llmqType, signHash)
CSigSharesManager->>CSigSharesManager: Params().GetLLMQ(llmqType)
CSigSharesManager->>CSigSharesNodeState: CanCreateSessionFromAnn(ann, maxSessions)
alt below cap
CSigSharesNodeState-->>CSigSharesManager: true
CSigSharesManager->>TimeSeenStore: timeSeenForSessions.try_emplace(signHash)
CSigSharesManager-->>Peer: accept announcement
else at/over cap
CSigSharesNodeState-->>CSigSharesManager: false
CSigSharesManager-->>Peer: reject announcement (logged)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)src/llmq/signing_shares.cppsrc/llmq/signing_shares.cpp:5:10: fatal error: 'llmq/signing_shares.h' file not found ... [truncated 1097 characters] ... install/lib/clang/18/include" src/test/llmq_utils_tests.cppsrc/test/llmq_utils_tests.cpp:5:10: fatal error: 'test/util/llmq_tests.h' file not found ... [truncated 1106 characters] ... stall/lib/clang/18/include" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
thepastaclaw
force-pushed
the
fix/qsigsesann-session-cap
branch
from
99be933 to
f54c9b7
Compare
|
@coderabbitai review |
|
Follow-up pushed in f54c9b7: kept the per-peer signing-session cap bounded, but added a minimum cap of 100 sessions so small test/devnet quorums do not exhaust the limit during normal ChainLock/InstantSend bursts before session cleanup runs. Validation:
@coderabbitai review |
✅ Action performedReview finished.
|
|
✅ Action performedReview finished.
|
thepastaclaw
force-pushed
the
fix/qsigsesann-session-cap
branch
from
f54c9b7 to
e5e8d1e
Compare
|
CI follow-up pushed in What changed:
Validation:
Note: the prior |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
✅ Review complete (commit 03dddee) |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Follow-up pushed in 6595948 to avoid the |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6595948482
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return false; | ||
| } | ||
| auto& session = nodeState.GetOrCreateSessionFromAnn(ann); | ||
| timeSeenForSessions.insert_or_assign(ann.buildSignHash().Get(), GetTime<std::chrono::seconds>().count()); |
There was a problem hiding this comment.
When a peer periodically re-announces an already-known QSIGSESANN before SESSION_NEW_SHARES_TIMEOUT, insert_or_assign refreshes timeSeenForSessions, and cleanup only expires sessions by comparing the current time to that stored value. That lets announcement-only sessions stay alive indefinitely, so an attacker can keep its capped session slots and the corresponding per-peer state pinned forever; seed this timestamp only when the signHash is first inserted rather than on refresh.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in f6aa12a: QSIGSESANN announcements now seed timeSeenForSessions only on first insert (try_emplace) instead of refreshing it on repeat announcements. Actual sig-share receipt still refreshes the timestamp via ProcessSigShare, so announcement-only sessions expire normally.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6aa12a20f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| bool CSigSharesNodeState::CanCreateSessionFromAnn(const llmq::CSigSesAnn& ann, size_t maxSessions) const | ||
| { | ||
| return sessions.count(ann.buildSignHash().Get()) != 0 || GetSessionCount(ann.getLlmqType()) < maxSessions; |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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 --checkmake -C src -j8 test/test_dashsrc/test/test_dash --run_test=llmq_utils_tests --catch_system_errors=no
|
Follow-up pushed in Validation:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
fix: limit signing share sessions per peer
Issue being fixed or feature implemented
Peers can announce many distinct QSIGSESANN signing sessions. Without a per-peer
cap, a peer can make its node state grow with announcement-only sessions.
What was done?
accepted.
refreshing that timeout on repeated announcements.
How Has This Been Tested?
Tested on macOS arm64.
git diff --check upstream/develop...HEADmake -j8 src/test/test_dashsrc/test/test_dash --run_test=llmq_signing_shares_testsshipBreaking Changes
None.
Checklist