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".
| }, | ||
| 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.
thepastaclaw
force-pushed
the
fix/bound-pending-sig-share-queue
branch
from
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 64c31bb; 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 64c31bb:
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 7827750. 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.
|
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.
thepastaclaw
force-pushed
the
fix/bound-pending-sig-share-queue
branch
from
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
📒 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
thepastaclaw
force-pushed
the
fix/bound-pending-sig-share-queue
branch
from
906f811 to
08d9533
Compare
|
Pushed
Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08d9533092
ℹ️ 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".
| // Batches awaiting verification are bounded so that under flood shares back up in the capped | ||
| // pending maps instead of migrating into the unbounded worker pool task queue. | ||
| static constexpr int MAX_UNVERIFIED_BATCHES{4}; | ||
| while (!workInterrupt && unverified_batches < MAX_UNVERIFIED_BATCHES) { |
There was a problem hiding this comment.
Fresh evidence beyond the earlier queue-cap concern is that CollectPendingSigSharesToVerify(32, ...) limits unique (nodeId, signHash) pairs, not actual shares; if peers fill the pending maps with many quorum members for the same few sessions, one collected “batch” can contain up to the 10k pending-share cap. This line then allows four such batches to be queued/in flight after they have been erased from the capped pending maps, so flood traffic can still move tens of thousands of BLS shares into verification work despite the intended global cap. Cap the number of shares collected per dispatch or keep queued/in-flight shares charged against the cap.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 355da45d4a4. CollectPendingSigSharesToVerify now caps each dispatch by actual shares added (MAX_SHARES_PER_BATCH = 32) instead of unique sessions. With four unverified batches, at most 128 shares can sit in/on the worker pool outside the 10,000-share pending maps; randomized peer round-robin fairness is preserved. Focused LLMQ tests and neighboring suites pass.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Prior finding reconciliation: FIXED — UnverifiedBatchGuard now decrements unverified_batches on every worker exit path, including exception unwinding. Carried-forward prior findings: none. New findings in the latest delta (906f811..08d9533): none; that delta is the RAII fix. New cumulative current-head finding outside the latest delta: one non-blocking performance suggestion because the newly introduced global pending-share cap walks all node states under cs for every admission.
Source: reviewers opus (general, dash-core-commit-history); gpt-5.5 (general, dash-core-commit-history; failed: ACP refresh token revoked). Verifier: opus.
🟡 1 suggestion(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.
In `src/llmq/signing_shares.cpp`:
- [SUGGESTION] src/llmq/signing_shares.cpp:496-504: Global pending-sig-share cap check is O(peers) per admission
TryAddPendingIncomingSigShare iterates every entry in nodeStates while holding cs to compute the global total on every QSIGSHARE/QBSIGSHARES admission. The per-node check is O(1) thanks to CountedBucketMap::Size(), but this loop reintroduces O(peers) work on the hot admission path. Under a flood with many connected peers, cs is held longer per accepted share, partially defeating the O(1) counting refactor and making the backpressure path itself scale with peer count. Maintain a manager-level running total that is updated alongside per-node Add/Erase operations so the global cap remains O(1). Correctness is unaffected, so this is non-blocking.
| size_t total{0}; | ||
| for (const auto& [_, ns] : nodeStates) { | ||
| 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; | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Global pending-sig-share cap check is O(peers) per admission
TryAddPendingIncomingSigShare iterates every entry in nodeStates while holding cs to compute the global total on every QSIGSHARE/QBSIGSHARES admission. The per-node check is O(1) thanks to CountedBucketMap::Size(), but this loop reintroduces O(peers) work on the hot admission path. Under a flood with many connected peers, cs is held longer per accepted share, partially defeating the O(1) counting refactor and making the backpressure path itself scale with peer count. Maintain a manager-level running total that is updated alongside per-node Add/Erase operations so the global cap remains O(1). Correctness is unaffected, so this is non-blocking.
source: ['claude']
There was a problem hiding this comment.
Fixed in 355da45d4a4 as part of the same accounting correction. A manager-level m_pending_sig_shares_total is now updated on every add/erase/session removal/node removal/ban path, so the global admission cap is O(1) instead of walking all peers under cs. The erase primitives return exact removed counts, with regression coverage.
|
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']
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: