fix: bound pending sig share queue - #7415
Conversation
|
✅ Review complete (commit bdca48c) |
|
![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: 18d245370f
ℹ️ 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 !ns.pendingIncomingSigShares.Empty(); | ||
| }, | ||
| rnd); | ||
|
|
||
| pendingIncomingSigSharesCount -= erasedCount; |
There was a problem hiding this comment.
When BLS verification is slower than incoming QSIGSHARE/QBSIGSHARES traffic, this decrement frees global capacity as soon as the dispatcher collects a batch, before those shares have been verified; NetSigning::WorkThreadDispatcher then moves the batch into worker_pool.push, whose underlying ctpl queue is unbounded, so peers can refill another 10k shares while the previous batches are still queued/in flight. That leaves the intended global backpressure bypassable under sustained traffic; keep these shares counted until the worker finishes or bound the worker queue as well.
Useful? React with 👍 / 👎.
|
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)
WalkthroughThis PR replaces fold-based sig-share sizing with counted bucket storage, adds per-node and global caps for pending incoming shares, and bounds verification batches and collection by share count. Pending shares are rejected for banned or over-cap nodes and cleared when nodes are banned. Unit tests cover map mutations, bucket erasure, and pending-session removal. Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SigShareHandler
participant CSigSharesManager
participant WorkerPool
SigShareHandler->>CSigSharesManager: Admit pending signature shares
CSigSharesManager->>CSigSharesManager: Enforce per-node and global caps
CSigSharesManager->>WorkerPool: Queue bounded verification batches
WorkerPool->>CSigSharesManager: Process pending signature shares
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
There was a problem hiding this comment.
Code Review
Source: reviewers claude/general=opus(failed), codex/general=gpt-5.5(completed), claude/dash-core-commit-history=opus(failed), codex/dash-core-commit-history=gpt-5.5(completed); verifier=gpt-5.5
The PR adds per-node and global admission caps for pending LLMQ sig shares, and the local map/count bookkeeping checks out for the paths reviewed. However, the global cap is released when shares are dispatched to the worker pool, before BLS verification completes, so the unbounded worker queue can still retain arbitrarily many pending verification batches. That leaves the PR's stated backpressure goal incomplete.
🔴 1 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [BLOCKING] src/llmq/signing_shares.cpp:565: Keep dispatched verification work under the global cap
`CollectPendingSigSharesToVerify` subtracts shares from `pendingIncomingSigSharesCount` as soon as they are copied into `retSigShares`, but `NetSigning::WorkThreadDispatcher` then moves those vectors into `worker_pool.push()` for later verification. That pool uses an unbounded `std::queue`, and only 1-4 worker threads drain it, so a peer can repeatedly fill the 10k pending map, let the dispatcher move batches into queued lambdas, and immediately refill the map while earlier batches are still retained and unverified. The new cap therefore limits only the pre-dispatch maps, not the actual retained verification backlog this PR is meant to bound. Keep queued-for-verification shares counted until the worker finishes, or add an explicit bounded in-flight/queued verification counter before accepting more incoming shares.
Note: posted as a COMMENT review because GitHub does not allow approving/requesting changes on my own PR via the normal review event.
18d2453 to
64c31bb
Compare
|
Handled the review blocker in What changed:
Validation:
|
|
@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: 64c31bb2e8
ℹ️ 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".
| __func__, MAX_PENDING_SIG_SHARES_TOTAL, nodeId); | ||
| return false; | ||
| } | ||
| if (nodeState.GetPendingIncomingSigSharesCount() >= MAX_PENDING_SIG_SHARES_PER_NODE) { |
There was a problem hiding this comment.
When BLS verification is slower than incoming traffic from one peer, this check only sees shares still in that peer's pendingIncomingSigShares map; CollectPendingSigSharesToVerify now erases dispatched shares from the node state while keeping only the global reservation until the worker releases them. Fresh evidence in this revision is that the global counter is reserved until ReleasePendingIncomingSigShares, but the per-node counter is decremented at dispatch, so the same peer can refill another 1000 shares repeatedly and occupy the full 10000-share global cap, starving other peers despite the new per-node limit.
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers claude/general=opus(failed), codex/general=gpt-5.5(completed), claude/dash-core-commit-history=opus(failed), codex/dash-core-commit-history=gpt-5.5(completed); verifier=gpt-5.5.
Carried-forward prior findings: prior-18d24537-1 is FIXED by 64c31bb2e87; queued worker-pool sig shares now remain reserved against the global cap until ReleasePendingIncomingSigShares runs after processing. New findings in latest delta: the same reservation model was not applied to the per-node cap, so one peer can cycle shares into worker tasks and refill its node-state queue until it occupies the full 10000-share global budget. The latest commit also repairs the original cap commit and should be squashed for one logical change.
🔴 1 blocking | 🟡 1 suggestion(s)
Note: GitHub does not allow me to request changes on my own PR, so this is posted as a COMMENT while preserving the verified blocking findings.
Posting note: review_poster could not post an inline/top-level review cleanly; posting exact-SHA body-only review from the same verifier output.
Prior Finding Reconciliation
- Keep dispatched verification work under the global cap — FIXED in 64c31bb2e87:
CollectPendingSigSharesToVerifynow keeps queued shares reserved against the global counter, andNetSigning::ProcessPendingSigSharesreleases that reservation after worker processing viaPendingSigSharesReservation.
Carried-Forward Prior Findings
- None.
New Findings In Latest Delta
- blocking: Keep queued shares under the per-node cap too —
src/llmq/signing_shares.cpp:1308 - suggestion: Squash the queued-share cap repair into the original cap commit —
<commit:64c31bb2e87>:1
Verified Findings
blocking: Keep queued shares under the per-node cap too
src/llmq/signing_shares.cpp:1308-1311
TryAddPendingIncomingSigShare enforces the per-node limit only with nodeState.GetPendingIncomingSigSharesCount(), but CollectPendingSigSharesToVerify erases shares from that node state when they are moved into worker-pool tasks. The new global reservation keeps those queued shares charged to pendingIncomingSigSharesCount until processing releases them, but the sender's node-state count drops immediately. A single peer can therefore fill its 1000-share per-node queue, let the dispatcher move those shares into queued verification work, and refill repeatedly until it monopolizes the full 10000-share global budget. Keep queued-but-unverified shares charged to the originating node, or add an in-flight per-node counter that is released together with the global reservation.
suggestion: Squash the queued-share cap repair into the original cap commit
<commit:64c31bb2e87>:1-1
64c31bb2e87 only repairs the cap accounting introduced by 1b040ba5e04: it keeps sig shares dispatched to the worker pool counted against the global pending-share cap until verification releases them. Since 1b040ba5e04 is already titled fix: bound pending sig share queue, leaving the repair as a separate permanent commit makes the first commit incomplete for the same logical change. Squash 64c31bb2e87 into 1b040ba5e04 before merge.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
- [BLOCKING] src/llmq/signing_shares.cpp:1308-1311: Keep queued shares under the per-node cap too
`TryAddPendingIncomingSigShare` enforces the per-node limit only with `nodeState.GetPendingIncomingSigSharesCount()`, but `CollectPendingSigSharesToVerify` erases shares from that node state when they are moved into worker-pool tasks. The new global reservation keeps those queued shares charged to `pendingIncomingSigSharesCount` until processing releases them, but the sender's node-state count drops immediately. A single peer can therefore fill its 1000-share per-node queue, let the dispatcher move those shares into queued verification work, and refill repeatedly until it monopolizes the full 10000-share global budget. Keep queued-but-unverified shares charged to the originating node, or add an in-flight per-node counter that is released together with the global reservation.
- [SUGGESTION] <commit:64c31bb2e87>:1-1: Squash the queued-share cap repair into the original cap commit
`64c31bb2e87` only repairs the cap accounting introduced by `1b040ba5e04`: it keeps sig shares dispatched to the worker pool counted against the global pending-share cap until verification releases them. Since `1b040ba5e04` is already titled `fix: bound pending sig share queue`, leaving the repair as a separate permanent commit makes the first commit incomplete for the same logical change. Squash `64c31bb2e87` into `1b040ba5e04` before merge.
64c31bb to
7827750
Compare
|
Pushed
Local validation:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers claude/general=opus(failed: quota), codex/general=gpt-5.5(completed), claude/dash-core-commit-history=opus(failed: quota), codex/dash-core-commit-history=gpt-5.5(completed); verifier=gpt-5.5.
Carried-forward prior findings: both prior-64c31bb2-1 and prior-64c31bb2-2 are fixed at 782775085e2388755a6a4dfb090c303fc635fc5a. New findings in the latest delta: none; the per-node in-flight reservation now keeps queued shares charged against the per-node cap, and the stack has been rewritten to one logical commit.
Note: GitHub does not allow me to approve my own PR, so this clean automated review is posted as a COMMENT.
Prior Finding Reconciliation
- Keep queued shares under the per-node cap too — FIXED. The current code adds per-node in-flight reservation accounting and checks pending plus in-flight shares before accepting more from a node.
- Squash the queued-share cap repair into the original cap commit — FIXED. The current stack is a single
fix: bound pending sig share queuecommit.
Carried-Forward Prior Findings
- None.
New Findings In Latest Delta
- None.
UdjinM6
left a comment
UdjinM6
left a comment
There was a problem hiding this comment.
utACK 782775085e2388755a6a4dfb090c303fc635fc5a
|
concept ack, but implementation is a bit over-engineered: Instead I think better to create a dedicated datastructure which is a tuple of std::map<uint256, std::map<A, B>> and counts in O(1) total amount of elements. It has been implemented as CountedBucketMap in this branch: knst/fix/bound-pending-sig-share-queue-v2 That approach is better than current approach of PR due to several reasons:
@thepastaclaw please check if any disadvantages with my https://github.com/knst/dash/tree/fix/bound-pending-sig-share-queue-v2-tmp and pull changes from there |
There was a problem hiding this comment.
see branch and comment above
https://github.com/knst/dash/tree/fix/bound-pending-sig-share-queue-v2-tmp #7415 (comment)
| { | ||
| CSigSharesNodeState node_state; | ||
|
|
||
| for (size_t i{0}; i < MAX_PENDING_SIG_SHARES_PER_NODE - 1; ++i) { |
There was a problem hiding this comment.
nit:
for (size_t i{1}; i < MAX_PENDING_SIG_SHARES_PER_NODE; ++i) {
BOOST_CHECK(node_state.AddPendingIncomingSigShare(MakeSigShare(i)));
There was a problem hiding this comment.
Superseded by the CountedBucketMap integration at 906f8118251: the old per-node cap-loop test is removed, and the replacement tests exercise the counted map and session removal directly.
7827750 to
906f811
Compare
|
Checked the
Force-pushed the reviewed four-commit stack as head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 906f811825
ℹ️ 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".
| ProcessPendingSigShares(std::move(sigSharesByNodes), std::move(quorums)); | ||
| --unverified_batches; |
There was a problem hiding this comment.
If ProcessPendingSigShares exits via an exception, this task never reaches the decrement, and the returned future from worker_pool.push is ignored so the failure is silent. One concrete path is a stale quorum: CollectPendingSigSharesToVerify can return false after filling sigSharesByNodes but without all retQuorums, then ProcessPendingSigShares hits quorums.at(...); after four such tasks unverified_batches < MAX_UNVERIFIED_BATCHES stays false and the dispatcher stops draining pending shares until restart. Use a scope guard/RAII cleanup so the slot is released on every exit path.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/llmq/net_signing.cpp`:
- Around line 338-342: The unverified_batches counter is not decremented if
ProcessPendingSigShares throws, which can permanently block new batches. In the
worker_pool lambda in net_signing.cpp, wrap the decrement with an RAII guard so
it always runs on every exit path, even when ProcessPendingSigShares or
quorums.at(...) throws. Keep the fix localized around the lambda that calls
ProcessPendingSigShares and updates unverified_batches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5a211296-01e8-4294-852e-24be50d824bd
📥 Commits
Reviewing files that changed from the base of the PR and between 782775085e2388755a6a4dfb090c303fc635fc5a and 906f811825176562cca4592d455e8a5c9549e8af.
📒 Files selected for processing (5)
src/llmq/net_signing.cppsrc/llmq/net_signing.hsrc/llmq/signing_shares.cppsrc/llmq/signing_shares.hsrc/test/llmq_utils_tests.cpp
|
The This PR is proper as-is: it only bounds pending sig-share admission/verification work, while the failure happens after recovery and quorum rotation when |
CollectPendingSigSharesToVerify used to bound each dispatched batch by unique (nodeId, signHash) sessions. A single session can carry up to the full MAX_PENDING_SIG_SHARES_TOTAL (10000) shares, so with the four in-flight batches allowed on the worker pool, tens of thousands of BLS shares could sit outside the pending accounting. Bound each batch by the actual number of shares added (32) instead. The rotation-based fairness across peers is preserved: one share at a time in randomized round-robin order. Combined with MAX_UNVERIFIED_BATCHES=4, at most 128 shares can be in-flight in / on the worker pool at once. Also collapse the O(peers) global-cap sum in TryAddPendingIncomingSigShare into a running m_pending_sig_shares_total maintained on every mutation site. The primitives now return the count they erased so the running total stays in lockstep with the aggregate node-state size. Add regression coverage for the count-returning primitives and for CSigSharesNodeState::RemoveSession's returned drop count.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 355da45d4a
ℹ️ 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".
| auto& nodeState = nodeStates[pfrom.GetId()]; | ||
| for (const auto& s : sigSharesToProcess) { | ||
| nodeState.pendingIncomingSigShares.Add(s.GetKey(), s); | ||
| TryAddPendingIncomingSigShare(pfrom.GetId(), nodeState, s); |
There was a problem hiding this comment.
When TryAddPendingIncomingSigShare returns false because the per-node or global pending-share cap is full, this loop silently drops a requested QBSIGSHARES response even though line 401 already removed the peer's requestedSigShares entry and the request path clears session.announced.inv once the request is sent. In that over-cap scenario the share is not verified and is not requested from the same peer again, so recovery can stall if no other peer advertises that quorum member; keep the request/announcement state until the share is admitted, or requeue it on admission failure.
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers claude/general=opus(completed), codex/general=gpt-5.5(failed), claude/dash-core-commit-history=opus(completed), codex/dash-core-commit-history=gpt-5.5(failed); verifier=opus.
Carried-forward prior findings: none. New findings in latest delta: none. Prior finding prior-18d24537-1 (Global pending-sig-share cap check is O(peers) per admission) is FIXED at 355da45 — a running counter m_pending_sig_shares_total (src/llmq/signing_shares.h:477) is maintained at every mutation site (Add:504, Erase:547, RemoveSession:1412, Clear:1463, EraseAllForSignHash-related:1402/1434) so the global cap check at line 496 is now O(1). Cumulative review of upstream/develop...HEAD finds no additional issues.
Posting note: review_poster could not post an inline/top-level review cleanly; posting exact-SHA body-only review from the same verifier output.
Prior Finding Reconciliation
- Global pending-sig-share cap check is O(peers) per admission — FIXED at 355da45. Introduced size_t m_pending_sig_shares_total GUARDED_BY(cs) (src/llmq/signing_shares.h:477) as a running total. All pendingIncomingSigShares mutation sites update it: ++ on successful Add (signing_shares.cpp:504), -= Erase return value on batch collection (547), RemoveSession (1412), Clear (1463), and EraseAllForSignHash-adjacent removals (1402, 1434). The global admission check at line 496 now reads the counter instead of iterating nodeStates, restoring O(1) behavior on the hot admission path.
Carried-Forward Prior Findings
- None.
New Findings In Latest Delta
- None.
Verified Findings
No active findings.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@knst applied on top of as requested and pushed the branch at , preserving your authorship. Focused validation passed locally: GEN obj/build.h |
|
@knst applied |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Code Review
Source: reviewers claude/general=opus(completed), codex/general=gpt-5.6-sol(failed), claude/dash-core-commit-history=opus(completed), codex/dash-core-commit-history=gpt-5.6-sol(failed); verifier=opus.
Carried-forward prior findings: none. The prior review at 355da45 had an empty active-finding set, so classification is not applicable and nothing is carried forward; I re-validated that the previously-clean areas remain clean at head bdca48c. New findings in latest delta: two non-blocking suggestions, both verified against the current checkout. (1) The head commit bdca48 fully reverts the running-total accounting mechanism introduced one commit earlier in 355da45 (member m_pending_sig_shares_total, count-returning Erase/EraseBucket/EraseIf/Clear/EraseAllForSignHash/RemoveSession primitives, and their asserting tests) — pure intra-PR churn that should be squashed, especially since 355da45's commit message documents an approach the shipped code does not use. (2) The cap/drop/banned enforcement in TryAddPendingIncomingSigShare has no direct unit coverage. The PR is otherwise a well-bounded DoS-hardening change; no blocking issues.
🟡 2 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/llmq/signing_shares.h`:
- [SUGGESTION] src/llmq/signing_shares.h:459: Squash bdca48 into 355da45 — running-total mechanism is introduced and reverted within the same PR
Commit 355da45 ("fix: cap sig-share verification batch by share count") introduces a global-cap accounting mechanism: the member `m_pending_sig_shares_total GUARDED_BY(cs)`, count-returning versions of the `CountedBucketMap`/`SigShareMap` primitives (`Erase`, `EraseBucket`, `EraseIf`, `Clear`, `EraseAllForSignHash`) and `CSigSharesNodeState::RemoveSession`, plus regression tests asserting those returned counts. Its commit message body explicitly documents this: "collapse the O(peers) global-cap sum ... into a running m_pending_sig_shares_total maintained on every mutation site. The primitives now return the count they erased ...".
The immediately-following head commit bdca48 ("refactor: use CountedBucketMap for counting total amount of pending sigs") undoes all of it: it deletes `m_pending_sig_shares_total`, reverts every primitive and `RemoveSession` back to `void`, restores the O(peers) sum over `nodeStates[*].pendingIncomingSigShares.Size()` in `TryAddPendingIncomingSigShare` (signing_shares.cpp:496-501), and drops the count assertions from the tests.
Net effect on permanent develop history: the running-total member, the count-returning API, and their asserting tests exist only in 355da45 and are erased by the very next commit — they never ship. A `git bisect` landing on 355da45 would show a design the shipped code never had, `git blame` records a return-type flip-flop across these primitives, and 355da45's message body describes an approach directly contradicted by the final code. Both commits compile independently and this is DoS/resource hardening rather than consensus logic, so it is not blocking, but it should be cleaned up before merge given the misleading permanent history. Squash bdca48 into 355da45 (e.g. mark bdca48 as `fixup!`/`squash!` of 355da45 and rebase --autosquash) and reword the merged message to drop the now-obsolete running-total rationale while keeping 355da45's genuinely shipped change (share-count batch bounding + MAX_UNVERIFIED_BATCHES) alongside the final O(peers)-sum cap check.
In `src/llmq/signing_shares.cpp`:
- [SUGGESTION] src/llmq/signing_shares.cpp:483-508: Cap/drop/banned enforcement in TryAddPendingIncomingSigShare has no direct unit coverage
The PR's stated purpose is bounding pending incoming sig shares, but the enforcement logic in `TryAddPendingIncomingSigShare` — the per-node cap at `MAX_PENDING_SIG_SHARES_PER_NODE` (line 491), the global cap at `MAX_PENDING_SIG_SHARES_TOTAL` (line 502), and the banned-state refusal (line 488) — is not exercised by any added test. A grep of llmq_utils_tests.cpp finds no reference to `TryAddPendingIncomingSigShare`, `MAX_PENDING_SIG_SHARES`, or the banned path; the new tests only cover the supporting `CountedBucketMap` size tracking and `RemoveSession` behavior. So the machinery the caps rely on (`Size()` accuracy) is tested, but the threshold/drop-without-misbehavior/banned-refusal behavior that is the point of the change is not. `TryAddPendingIncomingSigShare` is private and needs a `CSigSharesManager` plus quorum setup to drive, which makes this materially harder than the node-state-level tests already added, so this is a suggestion rather than a blocker.
| 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(BCLog::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& [_, ns] : nodeStates) { | ||
| // the size of nodeStates is limited by DEFAULT_MAX_PEER_CONNECTIONS(125) so it should not be performance issue | ||
| // The name of variable is intentionally mentioned in comment to make this code snippet relevant for possible changes in future | ||
| total += ns.pendingIncomingSigShares.Size(); | ||
| } | ||
| if (total >= MAX_PENDING_SIG_SHARES_TOTAL) { | ||
| LogPrint(BCLog::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); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Cap/drop/banned enforcement in TryAddPendingIncomingSigShare has no direct unit coverage
The PR's stated purpose is bounding pending incoming sig shares, but the enforcement logic in TryAddPendingIncomingSigShare — the per-node cap at MAX_PENDING_SIG_SHARES_PER_NODE (line 491), the global cap at MAX_PENDING_SIG_SHARES_TOTAL (line 502), and the banned-state refusal (line 488) — is not exercised by any added test. A grep of llmq_utils_tests.cpp finds no reference to TryAddPendingIncomingSigShare, MAX_PENDING_SIG_SHARES, or the banned path; the new tests only cover the supporting CountedBucketMap size tracking and RemoveSession behavior. So the machinery the caps rely on (Size() accuracy) is tested, but the threshold/drop-without-misbehavior/banned-refusal behavior that is the point of the change is not. TryAddPendingIncomingSigShare is private and needs a CSigSharesManager plus quorum setup to drive, which makes this materially harder than the node-state-level tests already added, so this is a suggestion rather than a blocker.
source: ['claude']
bdca48c refactor: use CountedBucketMap for counting total amount of pending sigs (Konstantin Akimov) 355da45 fix: cap sig-share verification batch by share count (PastaClaw) 08d9533 fix: cap amount of unverified batches by 4 (Konstantin Akimov) e2862fe fix: the actual cap enforcement for pending signatures (Konstantin Akimov) 4666ef3 test: add regression tests for CountedBucketMap (Konstantin Akimov) ec15f90 refactor: use dedicated util structure to count internal objects in signing-shares (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Pending incoming LLMQ sig shares are admitted cheaply and verified later by worker threads. Direct and batched share traffic could grow retained pending verification work without per-node/global backpressure. This replaces closed PR dashpay#7413 and keeps the same scope: resource/backpressure hardening for LLMQ signing, with no consensus or user-facing behavior change. ## What was done? - Add per-node and global caps for pending incoming sig shares. - Track pending counts explicitly so cap checks do not require repeated scans. - Gate `QSIGSHARE` and `QBSIGSHARES` pending admission; over-cap shares are dropped without misbehavior scoring. - Clear pending incoming shares when a peer is banned and skip banned node states during collection. - Refuse new pending work for already-banned node states. - Add unit coverage for cap/drop/count cleanup behavior. ## How Has This Been Tested? - `./autogen.sh` - `./configure --without-gui --disable-bench --disable-fuzz-binary` - `make -C src test/test_dash -j$(sysctl -n hw.ncpu)` - `src/test/test_dash --run_test=llmq_utils_tests` - `git diff --check upstream/develop..HEAD` - `COMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.py` ## Breaking Changes None. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: kwvg: utACK bdca48c Tree-SHA512: e2403635a608aab5488dbf10b481a778e735a6e5b065eec5a4cc444818f61e89b3a688f82c14aaceae07fd714432d39c013769a54aa66e31bb8abddb3531aaf3
bdca48c refactor: use CountedBucketMap for counting total amount of pending sigs (Konstantin Akimov) 355da45 fix: cap sig-share verification batch by share count (PastaClaw) 08d9533 fix: cap amount of unverified batches by 4 (Konstantin Akimov) e2862fe fix: the actual cap enforcement for pending signatures (Konstantin Akimov) 4666ef3 test: add regression tests for CountedBucketMap (Konstantin Akimov) ec15f90 refactor: use dedicated util structure to count internal objects in signing-shares (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Pending incoming LLMQ sig shares are admitted cheaply and verified later by worker threads. Direct and batched share traffic could grow retained pending verification work without per-node/global backpressure. This replaces closed PR dashpay#7413 and keeps the same scope: resource/backpressure hardening for LLMQ signing, with no consensus or user-facing behavior change. ## What was done? - Add per-node and global caps for pending incoming sig shares. - Track pending counts explicitly so cap checks do not require repeated scans. - Gate `QSIGSHARE` and `QBSIGSHARES` pending admission; over-cap shares are dropped without misbehavior scoring. - Clear pending incoming shares when a peer is banned and skip banned node states during collection. - Refuse new pending work for already-banned node states. - Add unit coverage for cap/drop/count cleanup behavior. ## How Has This Been Tested? - `./autogen.sh` - `./configure --without-gui --disable-bench --disable-fuzz-binary` - `make -C src test/test_dash -j$(sysctl -n hw.ncpu)` - `src/test/test_dash --run_test=llmq_utils_tests` - `git diff --check upstream/develop..HEAD` - `COMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.py` ## Breaking Changes None. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: kwvg: utACK bdca48c Tree-SHA512: e2403635a608aab5488dbf10b481a778e735a6e5b065eec5a4cc444818f61e89b3a688f82c14aaceae07fd714432d39c013769a54aa66e31bb8abddb3531aaf3
Backport of dashpay#7415 (upstream merge ecf2382, cherry-picked with -m1). v23.1.x adaptations: (1) the per-node/global pending caps, TryAddPendingIncomingSigShare, CountedBucketMap and the share-count batch bound apply unchanged; only develop-only context around them (the dashpay#7115 NetSigning dispatcher, GetMaxSessionsForPeer session caps) was not brought along. (2) Develop's MAX_UNVERIFIED_BATCHES cap on dispatched-but-unverified worker-pool batches has no counterpart here: on this branch shares leave the capped pending maps only inside the worker that immediately verifies them (ProcessPendingSigSharesLoop), so shares never accumulate in the pool's task queue and there is nothing to bound. (3) llmq_utils_tests.cpp gains the PR's three new test cases plus the MakeSigShare helper they need (taken verbatim from develop, where an out-of-scope earlier PR introduced it); develop-only session-limit tests are not included. (cherry picked from commit ecf2382)
Backport of dashpay#7415 (upstream merge ecf2382, cherry-picked with -m1). v23.1.x adaptations: (1) the per-node/global pending caps, TryAddPendingIncomingSigShare, CountedBucketMap and the share-count batch bound apply unchanged; only develop-only context around them (the dashpay#7115 NetSigning dispatcher, GetMaxSessionsForPeer session caps) was not brought along. (2) Develop's MAX_UNVERIFIED_BATCHES cap on dispatched-but-unverified worker-pool batches has no counterpart here: on this branch shares leave the capped pending maps only inside the worker that immediately verifies them (ProcessPendingSigSharesLoop), so shares never accumulate in the pool's task queue and there is nothing to bound. (3) llmq_utils_tests.cpp gains the PR's three new test cases plus the MakeSigShare helper they need (taken verbatim from develop, where an out-of-scope earlier PR introduced it); develop-only session-limit tests are not included. (cherry picked from commit ecf2382)
Backport of dashpay#7415 (upstream merge ecf2382, cherry-picked with -m1). v23.1.x adaptations: (1) the per-node/global pending caps, TryAddPendingIncomingSigShare, CountedBucketMap and the share-count batch bound apply unchanged; only develop-only context around them (the dashpay#7115 NetSigning dispatcher, GetMaxSessionsForPeer session caps) was not brought along. (2) Develop's MAX_UNVERIFIED_BATCHES cap on dispatched-but-unverified worker-pool batches has no counterpart here: on this branch shares leave the capped pending maps only inside the worker that immediately verifies them (ProcessPendingSigSharesLoop), so shares never accumulate in the pool's task queue and there is nothing to bound. (3) llmq_utils_tests.cpp gains the PR's three new test cases plus the MakeSigShare helper they need (taken verbatim from develop, where an out-of-scope earlier PR introduced it); develop-only session-limit tests are not included. (cherry picked from commit ecf2382)
Backport of dashpay#7415 (upstream merge ecf2382, cherry-picked with -m1). v23.1.x adaptations: (1) the per-node/global pending caps, TryAddPendingIncomingSigShare, CountedBucketMap and the share-count batch bound apply unchanged; only develop-only context around them (the dashpay#7115 NetSigning dispatcher, GetMaxSessionsForPeer session caps) was not brought along. (2) Develop's MAX_UNVERIFIED_BATCHES cap on dispatched-but-unverified worker-pool batches has no counterpart here: on this branch shares leave the capped pending maps only inside the worker that immediately verifies them (ProcessPendingSigSharesLoop), so shares never accumulate in the pool's task queue and there is nothing to bound. (3) llmq_utils_tests.cpp gains the PR's three new test cases plus the MakeSigShare helper they need (taken verbatim from develop, where an out-of-scope earlier PR introduced it); develop-only session-limit tests are not included. (cherry picked from commit ecf2382)
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
5 participants
Issue being fixed or feature implemented
Pending incoming LLMQ sig shares are admitted cheaply and verified later by worker threads. Direct and batched share traffic could grow retained pending verification work without per-node/global backpressure.
This replaces closed PR #7413 and keeps the same scope: resource/backpressure hardening for LLMQ signing, with no consensus or user-facing behavior change.
What was done?
QSIGSHAREandQBSIGSHARESpending admission; over-cap shares are dropped without misbehavior scoring.How Has This Been Tested?
./autogen.sh./configure --without-gui --disable-bench --disable-fuzz-binarymake -C src test/test_dash -j$(sysctl -n hw.ncpu)src/test/test_dash --run_test=llmq_utils_testsgit diff --check upstream/develop..HEADCOMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.pyBreaking Changes
None.
Checklist: