fix: limit signing share sessions per peer - #7351
Conversation
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR enforces per-peer session limits for LLMQ signing shares. It adds GetMaxSessionsForPeer and related constants, four 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 or over cap
CSigSharesNodeState-->>CSigSharesManager: false
CSigSharesManager-->>Peer: reject announcement (logged)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 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)
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. |
99be933 to
f54c9b7
Compare
|
@coderabbitai review |
|
Follow-up pushed in f54c9b7446: 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.
|
f54c9b7 to
e5e8d1e
Compare
|
CI follow-up pushed in What changed:
Validation:
Note: the prior |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
✅ Review complete (commit 76a6fd7) |
|
@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.
|
| 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; |
There was a problem hiding this comment.
Not sure we should ban here. Wouldn't ignoring it be safer?
| return false; | |
| return true; |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Targeted DoS-hardening change with appropriate scope and unit coverage. No correctness or security issues. Three nitpick-level observations from Claude on documentation, an O(n) scan on a hot path, and operation sequencing — none block the PR.
💬 3 nitpick(s)
| constexpr size_t MAX_SESSIONS_PER_PEER_FACTOR{4}; | ||
| constexpr size_t MIN_SESSIONS_PER_PEER{100}; |
There was a problem hiding this comment.
💬 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']
| 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; | ||
| }); | ||
| } |
There was a problem hiding this comment.
💬 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']
|
|
||
| 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 true; | ||
| } |
There was a problem hiding this comment.
💬 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']
|
This pull request has conflicts, please rebase. |
Cherry-picked from upstream 11f524b. One adaptation: the include block additionally carries <ranges>. Upstream's diff adds only <algorithm> because develop already included <ranges>; v23.1.x did not, and the backported GetSessionCount()/GetAnnouncementSessionCount() use std::ranges::count_if.
24920a0 chore: prepare v23.1.8 release (pasta) 2194248 Merge #7348: fix: penalize oversized notfound messages (pasta) f5c72c3 Merge #7347: fix: punish invalid dstx messages (pasta) 550caf7 Merge #7465: fix(qt): handle pixel-sized fonts when scaling widgets (pasta) e203710 Merge #7419: fix(net): bound CoinJoin message vector intake (pasta) 5f5b960 Merge #7418: fix(net): bound signing message vector intake (Pasta) 7cc2cca Merge #7450: test: make governance vote fixtures wire-valid (Pasta) f011c80 Merge #7440: fix(net): bound governance vote signature deserialization (Pasta) 4b4d96a Merge #7442: fix(net): authorize governance inv responses via the net-layer per-peer request tracker (Pasta) f855b13 Merge #7444: fix(net): bound bloom message vectors before allocation (Pasta) 5b5c6fb Merge #7415: fix: bound pending sig share queue (Pasta) 9bbe808 Merge #7416: fix(net): bound quorum data response vectors (Pasta) da42f50 Merge #7424: fix: bound ChainLock seen cache (Pasta) 5b310df Merge #7438: fix: bound SPORK signature deserialization (Pasta) e118d0c Merge #7259: fix: dangling point to cj client (Pasta) 9921621 Merge #7439: refactor: add bounded vector deserialization (Pasta) 89bdf7c Merge #7414: fix(net): throttle per-object governance vote sync requests (Pasta) 44c396d Merge #7402: fix: bound pending recovered sig queue to prevent remote OOM (Pasta) 05cfe27 Merge #7351: fix: limit signing share sessions per peer (pasta) 3ef3a5b Merge #7408: fix: bound DKG contribution blob intake (pasta) 0ea6532 Merge #7387: test: migrate governance inv cache coverage to unit tests (Pasta) 8ffdf7f Merge #7398: backport: compact block relay hardening (bitcoin#26898, bitcoin#27626, bitcoin#27743, bitcoin#26969, bitcoin#29412, bitcoin#32646, bitcoin#33296) (Pasta) 2915142 backport: bitcoin#27608 - p2p: Avoid prematurely clearing download state for other peers (PastaClaw) 90b5473 Merge #7396: fix: run of circular-dependencies with python3.15 (Pasta) b003cdc Merge #7395: ci: update GitHub Actions pins for Node 24 (pasta) 97c3dd1 Merge #7394: fix: stabilize par help text in manpages (pasta) 8f8616b Merge #7372: backport: bitcoin#32693: depends: fix cmake compatibility error for freetype (pasta) 48f72be Merge #7360: fix: empty platformP2PPort deprecated field in protx listdiff results (pasta) a8cccff Merge #7298: fix(qt): keep PoSe score visible when hiding banned masternodes (pasta) Pull request description: Release PR for Dash Core v23.1.8, a patch release on top of v23.1.7. Fast-forwards from `v23.1.x` (currently at `chore: prepare v23.1.7 release`), 29 commits, no merge commits, no conflicts. ## Contents Backports of PRs already reviewed and merged on `develop`: `#7259` `#7347` `#7348` `#7351` `#7298` `#7360` `#7372` `#7387` `#7394` `#7395` `#7396` `#7398` `#7402` `#7408` `#7414` `#7415` `#7416` `#7418` `#7419` `#7424` `#7438` `#7439` `#7440` `#7442` `#7444` `#7450` `#7465` Plus `backport: bitcoin#27608`, a single commit taken from Dash #7237 because #7398's compact-block hardening depends on it. The rest of that v0.26 batch is intentionally not included on v23.1.x. The commit is byte-identical to its reviewed counterpart inside #7237. And release preparation: version bump, regenerated man pages, release notes, archived 23.1.7 notes. ## Note for reviewers: this branch was rebuilt An earlier revision of this PR was discarded and the branch rebuilt from scratch. Review comments on the previous revision point at commits that no longer exist, though the feedback itself was carried over (see below). The reason: several commits titled `Merge #NNNN` in the earlier revision contained substantial code that exists nowhere upstream — apparently written from a description of each PR rather than ported from its diff. For example, `feature_llmq_simplepose.py` is byte-identical between v23.1.7 and `develop`, yet the earlier `Merge #7408` rewrote 66 lines of it; `test/functional/p2p_governance_invs.py` does not exist on `develop` at all, yet had grown from 62 to 148 lines. That mislabeling matters because a commit titled `Merge #NNNN` invites less scrutiny, not more. It also had consequences: the earlier revision was **missing #7440 entirely**, and contained eleven consecutive commits that did not compile (code written against newer upstream APIs this branch does not have — `Misbehaving(Peer&)`, and `PeerIsBanned` used five commits before it was declared). Every commit on this branch has now been diffed against its upstream merge commit. Where a backport differs, it is because v23.1.x predates an upstream refactor and the change had to be applied to the pre-refactor file — for example #7418 and #7438 patch `signing_shares.cpp` / `spork.cpp` where upstream patches `net_signing.cpp` / `net_processing.cpp`. ## Dropped from this branch - **#7350** (`net: don't lock cs_main while reading blocks`) — dropped on review feedback. It is a 110-line lock-structure refactor of `ProcessGetBlockData` with no measured benefit, and it would add avoidable churn to the eventual master→develop merge-back. Nothing on this branch depends on it: #7398's compact-block work precedes it, and the remaining 14 commits replay with zero conflicts once it is removed. Thanks @knst. ## Added after the initial review pass - **#7351** (`fix: limit signing share sessions per peer`) — cherry-picked as a single commit and placed before #7402, matching upstream's merge order. The include block additionally carries `<ranges>`: upstream's diff adds only `<algorithm>` because develop already had it, whereas v23.1.x did not and the backported `GetSessionCount()` / `GetAnnouncementSessionCount()` use `std::ranges::count_if`. - **#7465** (`fix(qt): handle pixel-sized fonts when scaling widgets`) — cherry-picked from the five upstream commits. `optiontests.cpp` additionally includes `qt/guiutil_font.h`, because `fontsLoaded()` and `updateFonts()` are declared there on v23.1.x while develop declares them in `qt/guiutil.h`, which is all the upstream test includes. Two further backports were added later and applied without any adaptation -- their diffs are byte-for-byte identical to upstream: - **#7347** (`fix: punish invalid dstx messages`) - **#7348** (`fix: penalize oversized notfound messages`) ## Adaptations worth flagging - **#7360** — upstream gates `platformP2PPort` / `platformHTTPPort` in `protx listdiff` behind `IsServiceDeprecatedRPCEnabled()`. On 23.x those deprecated fields are deliberately not enforced through gating (see `bbcd9d543e6`), so shipping the gate as-is would silently drop two fields that v23.1.7 always returned. Changed to `if (true)` with a comment, per review feedback, keeping the block aligned with `develop`. The substantive fix from #7360 — reading the live port from `netInfo` instead of the always-zero scalar — is retained. - **#7415** — the pending-map caps (`MAX_PENDING_SIG_SHARES_PER_NODE`, `MAX_PENDING_SIG_SHARES_TOTAL`) are backported. The additional bound upstream places on batches awaiting verification is not, because it guards a condition that does not exist here: upstream's dispatcher pushes one task per batch inside an inner loop, whereas v23.1.x pushes a single looping worker per 10 ms tick. There is no unbounded task queue to bound. - **Man pages** — regenerated without the `lock` debug category, which only exists under `DEBUG_LOCKCONTENTION` and so is absent from release binaries. Thanks @UdjinM6 for catching this. ## Known CI failure macOS jobs are expected to fail. `actions/upload-artifact@v6` rejects filenames containing `:`, and the Xcode SDK ships Perl man pages with `::` in the name. A release-branch-only workaround existed on the earlier revision but was dropped as it corresponds to no upstream PR. This is accepted for this release. ## Testing - Every commit through #7465 compiles individually (verified for 27 of the 29; the three additions below were verified at the tip) — verified individually, not just at the tip. - Full build clean; no new warnings. - Unit tests pass. - Functional tests pass: `feature_llmq_signing` (both variants), `feature_llmq_chainlocks`, `feature_llmq_dkgerrors`, `feature_llmq_is_cl_conflicts`, `p2p_instantsend`, `feature_dip3_deterministicmns` (both wallet types), `rpc_coinjoin`. - Qt unit tests pass (32 cases, run under the `cocoa` platform plugin so the pixel-sized font regression from #7465 actually executes rather than self-skipping). - Lint: one pre-existing `lint-cppcheck-dash` failure, identical on v23.1.7, in files this branch does not touch. Top commit has no ACKs. Tree-SHA512: 0fa469c9a33820aa85fbb8b90c5877409d09490f746f1300b05ceda470a600765f900bec42e5aad5d90a2d46b44e28c20e44dc4a8fe719553a072298069eaec4
Upstream landed its own "release: prepare v23.1.8" (dashpay#7493) plus parallel merges of several PRs this branch had already backported, so most conflicts are two adaptations of the same change rather than divergent intent. Resolutions: - Release artifacts (manpages, flatpak metainfo, release notes, configure.ac): took upstream's published v23.1.8 text as the base, then re-added the CoinJoin/wallet, GUI, and credits content that upstream's copy lacked. - LLMQ/net/test files where upstream carried a refined superset (dashpay#7351, dashpay#7347, dashpay#7348, dashpay#7465): took upstream. - CoinJoin client lifetime, overviewpage mixing-state, dmnstate platform ports, and coinjoin_tests: kept this branch's versions. - test/functional/p2p_governance_invs.py: accepted upstream's removal in favor of the src/test/governance_inv_tests.cpp migration (dashpay#7387). Auto-merge produced two duplicate definitions that would not compile (IsSyncableObject in governance.cpp, UnserializeBatchedSigShares in signing_shares.cpp); both were reduced to a single definition. Co-Authored-By: Claude <noreply@anthropic.com>
3 participants
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