fix(net): bound signing message vector intake#7418
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughLLMQ signing-message deserialization now applies message-specific vector limits and bans peers when oversized payloads trigger failures. Batched signature shares are decoded incrementally with outer-batch and cumulative inner-share limits. Serialization and tests cover these boundaries. Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Peer
participant NetSigning
participant UnserializeBatchedSigShares
participant BanNode
Peer->>NetSigning: send LLMQ signing message
NetSigning->>UnserializeBatchedSigShares: decode payload with size limits
alt within limits
UnserializeBatchedSigShares-->>NetSigning: decoded batches
else oversized payload
UnserializeBatchedSigShares-->>NetSigning: throw std::ios_base::failure
NetSigning->>BanNode: BanNode(pfrom.GetId())
NetSigning-->>Peer: rethrow failure
end
Possibly related PRs
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc151dee36
ℹ️ 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".
| std::vector<CBatchedSigShares> msgs; | ||
| vRecv >> msgs; | ||
| size_t msg_size{0}; | ||
| if (!ReadLimitedVector(vRecv, msgs, MAX_MSGS_TOTAL_BATCHED_SIGS, msg_size)) { |
There was a problem hiding this comment.
When a peer sends a QBSIGSHARES message whose outer vector is within the new limit but one CBatchedSigShares::sigShares count is greater than 400, this call throws from CBatchedSigShares::Unserialize instead of returning false. Unlike the QSIGSHARESINV branch, this path has no local catch that calls BanNode; PeerManagerImpl::ProcessMessages catches std::exception and only logs it, so the peer remains connected and can repeatedly trigger the oversized inner-vector path even though oversized outer/total counts are banned.
Useful? React with 👍 / 👎.
|
🔍 Review in progress — actively reviewing now (commit 8ff75e0) |
|
bc151de to
ce347a1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce347a1381
ℹ️ 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".
| { | ||
| READWRITE(VARINT(obj.sessionId), obj.sigShares); | ||
| READWRITE(VARINT(obj.sessionId)); | ||
| if (ser_action.ForRead()) { |
There was a problem hiding this comment.
In SERIALIZE_METHODS, Ser passes a const CBatchedSigShares&, and this is a regular if, so the read branch is still compiled for the serialize instantiation. That branch mutates obj.sigShares with clear()/reserve()/emplace_back(), which is const in that instantiation, so builds that serialize CBatchedSigShares (for example CNetMsgMaker::Make(NetMsgType::QBSIGSHARES, msgs) in CSigSharesManager::SendMessages) fail to compile; split the read/write paths with the serialization helpers instead of a runtime branch.
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers: opus general failed (extra-usage quota), gpt-5.5 general completed; opus dash-core-commit-history failed (extra-usage quota), gpt-5.5 dash-core-commit-history completed; coordinator portability candidate validated by gpt-5.5 verifier. Verifier: gpt-5.5 fallback after opus quota failure.
Prior reconciliation: prior-1 is STILL VALID; both commits still use generic fix: subjects instead of an LLMQ area prefix, so the prior nitpick is carried forward.
Carried-forward prior findings: 1 nitpick (Use an LLMQ area prefix for commit subjects).
New findings in latest delta: 1 blocking build portability issue. The newly added bounded deserialization helpers list-initialize size_t from ReadCompactSize, which is a build-breaking narrowing conversion on Dash's documented 32-bit targets.
🔴 1 blocking | 💬 1 nitpick(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/net_signing.cpp`:
- [BLOCKING] src/llmq/net_signing.cpp:30-44: Avoid narrowing ReadCompactSize into size_t
`ReadCompactSize` returns `uint64_t`, but the new readers store it with braced initialization in `const size_t size{...}`. Dash still documents 32-bit depends/Guix targets such as `i686-pc-linux-gnu` and `arm-linux-gnueabihf`, where `size_t` is 32 bits; on those targets, list-initializing `size_t` from a non-constant `uint64_t` is ill-formed and the file will not compile. Read the compact size into a `uint64_t`, check it against the small max bound, then cast to `size_t` after the check. The same braced narrowing pattern also appears in `CBatchedSigShares` at `src/llmq/signing_shares.h:159`.
Note: GitHub does not allow me to submit REQUEST_CHANGES on my own PR, so this is posted as a COMMENT while preserving the verified blocking finding.
b040df8 to
44cc759
Compare
|
Addressed the review feedback for the ReadCompactSize narrowing issue and force-pushed the squashed branch update to
Validation: Verifier re-run: @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 7418 --repo dashpay/dash --json headRefName,headRefOid,commits --jq '.headRefName, .headRefOid'
echo "---"
gh pr diff 7418 --repo dashpay/dash | head -300Length of output: 8399 Verified the updated diff on
This all looks correct and addresses the narrowing issue. Nice work. ✅ Action performedReview finished.
|
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The current source fixes the prior ReadCompactSize narrowing problem by reading into uint64_t, checking the bound, and casting only after the check. One in-scope blocker remains: the top commit is a one-line compile fix for serialization code introduced by an earlier commit, leaving the permanent stack with a non-building intermediate commit unless it is squashed or reordered. The prior commit-subject nitpick also still applies because the LLMQ changes are still recorded with generic fix: subjects.
Source: reviewers: opus general failed; gpt-5.5 general completed; opus dash-core-commit-history failed; gpt-5.5 dash-core-commit-history completed. Verifier: gpt-5.5 completed. Failed lanes are retained in local artifacts.
Prior reconciliation:
- prior-1 FIXED:
ReadCompactSizeis now read intouint64_t, checked against the message bound, then cast tosize_tafter the check insrc/llmq/net_signing.cppandsrc/llmq/signing_shares.h. - prior-2 STILL VALID: the current stack still uses generic
fix:subjects for LLMQ signing commits.
Carried-forward prior findings:
- [NITPICK]
<commit:19392f213e1>:1— Use an LLMQ area prefix for commit subjects.
New findings in latest delta:
- [BLOCKING]
<commit:44cc7599966>:1— Squash the serialization compile fix into the commit it repairs.
🔴 1 blocking | 💬 1 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.
In `<commit:44cc7599966>`:
- [BLOCKING] <commit:44cc7599966>:1: Squash the serialization compile fix into the commit it repairs
Commit `44cc7599966` only changes `if (ser_action.ForRead())` to `if constexpr (ser_action.ForRead())` in `CBatchedSigShares`. That repairs code introduced in `19392f213e1`: `SERIALIZE_METHODS` instantiates serialization with a `const CBatchedSigShares&`, and without `if constexpr` the read-only branch that calls `clear()`, `reserve()`, and `emplace_back()` on `obj.sigShares` is still compiled for the const serialization path. Dash's PR hygiene requires each commit to build on its own, so leaving this as a separate top commit preserves a non-compiling intermediate state. Squash `44cc7599966` into `19392f213e1`, or otherwise rewrite the stack so the serialization commit is buildable.
In `<commit:19392f213e1>`:
- [NITPICK] <commit:19392f213e1>:1: Use an LLMQ area prefix for commit subjects
The current PR stack still uses generic subjects: `fix: bound batched sig-share intake`, `fix: bound LLMQ signing vector intake`, and `fix: compile batched sig share serialization`. These commits are all scoped to LLMQ signing, while Dash's commit convention uses an area prefix such as `llmq:`. Reword the surviving commits after the squash so the permanent history is scoped consistently.
Note: GitHub does not allow me to submit REQUEST_CHANGES on my own PR, so this is posted as a COMMENT while preserving the verified blocking finding.
44cc759 to
d32884e
Compare
|
Addressed the remaining self-review history feedback with a tree-equivalent branch rewrite. What changed:
New head: @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 7418 --repo dashpay/dash --json headRefName,headRefOid --jq '.headRefName, .headRefOid'
echo "---diff---"
gh pr diff 7418 --repo dashpay/dash > /tmp/pr7418.diff
wc -l /tmp/pr7418.diff
echo "---compare trees---"
git -C . rev-parse HEAD 2>/dev/null || trueLength of output: 599 🏁 Script executed: #!/bin/bash
set -euo pipefail
cat /tmp/pr7418.diffLength of output: 8178 Confirmed: the diff for the new head ( This is a pure history rewrite (squash + commit-message rewording), not a code change — the tree matches the previously reviewed head as you stated. No new findings from my side. ✅ Action performedReview finished.
|
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The latest push rewrote the stack without changing the final tree relative to 44cc7599966a4185aa8568e067c54eeb9315c20e, so I verified this as a cumulative current-head review. The current stack contains two scoped llmq: commits, and the serialization if constexpr repair is folded into the commit that introduces the bounded CBatchedSigShares deserialization. No in-scope correctness, security, or PR-hygiene findings remain.
Source: reviewers: opus general failed; gpt-5.5 general completed; opus dash-core-commit-history failed; gpt-5.5 dash-core-commit-history completed. Verifier: gpt-5.5 completed. Failed lanes are retained in local artifacts.
Prior reconciliation:
- prior-1 FIXED: the standalone
44cc7599966serialization compile-fix commit is gone, and2a2bc9d2251directly containsif constexpr (ser_action.ForRead())insrc/llmq/signing_shares.h. - prior-2 FIXED: both current commits use the
llmq:area prefix:llmq: bound batched sig-share intakeandllmq: bound LLMQ signing vector intake.
Carried-forward prior findings: None.
New findings in latest delta: None.
No findings.
Note: GitHub does not allow me to submit APPROVE on my own PR, so this exact-SHA clean review is posted as a COMMENT.
|
pls see 80779e7 for some suggestions |
d32884e to
d862d6c
Compare
|
Applied UdjinM6’s Updated head: What changed:
Validation passed:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
CI note for The failing test is the known intermittent Scope check: the actual merge-base diff for this PR only changes: The failing |
|
CI note: |
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The delta since the prior clean review at d862d6c is a single empty CI-retrigger commit (28c3575) with no file changes. Cumulative revalidation of the substantive commits (b907d3f LLMQ batched sig-share intake bound, d862d6c LLMQ signing vector intake bound) confirms the prior APPROVE still holds on code content. One hygiene suggestion: drop the empty CI-retrigger commit before merge, since Dash Core merges PR stacks without squashing and the project guide explicitly discourages empty CI-retrigger commits.
Source: reviewers: opus general completed; gpt-5.5 general failed; opus dash-core-commit-history completed; gpt-5.5 dash-core-commit-history failed. Verifier: opus completed. Failed lanes are retained in local artifacts.
🟡 1 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 `<commit:28c3575130a>`:
- [SUGGESTION] <commit:28c3575130a>:1: Drop empty CI-retrigger commit before merge
Commit 28c3575130a ("ci: retrigger linux64_multiprocess-test after feature_asset_locks flake") has zero file changes and was pushed only to re-run CI after an unrelated feature_asset_locks flake. Dash Core merges PR stacks into develop without squashing, so this no-op commit would become permanent history in a consensus-adjacent LLMQ hardening PR — noise in `git log` and a useless step in `git bisect`. The project agent guide (CLAUDE.md / AGENTS.md, PR Hygiene section) explicitly says: "If CI fails for reasons unrelated to the PR, document the evidence instead of pushing empty commits or unrelated changes." Reset to d862d6cf2a1 and force-push, or drop this commit via interactive rebase; capture the flake evidence in a PR comment instead. The two substantive commits (b907d3f, d862d6c) are cleanly scoped and should remain.
28c3575 to
f3c35ba
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-Forward Prior Findings
None. The sole prior finding (net_signing.h:34-36, doc comment overstating when the QBSIGSHARES aggregate limit is enforced) is FIXED at 430a702 — verified directly against git show 430a7024021 -- src/llmq/net_signing.h and the current source: the comment now reads 'throws std::ios_base::failure once detected' instead of the inaccurate 'before the offending element is decoded,' which correctly covers both the outer-count guard (net_signing.cpp:29-32, pre-decode) and the running-aggregate guard (net_signing.cpp:35-41, which fully decodes the crossing batch via vRecv >> msgs.back() at line 37 before comparing and throwing at lines 39-40).
New Findings In Latest Delta
All three commit-history lanes (codex, sonnet5, opus) independently converge on one commit-hygiene suggestion: the new commit 430a702 is a 2-line doc correction to prose introduced one commit earlier in this same unmerged PR (c9a1944). Since the PR has never shipped, this is a same-PR fixup that should be squashed into c9a1944 rather than left as a standalone entry in permanent history. I independently confirmed via git log --oneline that 430a702 sits directly after c9a1944 with no intervening commits, and via git show that it touches only the exact lines c9a1944 introduced. This is a valid, low-risk suggestion aligned with the project's atomic-commit guidance.
All other cumulative code (bounded intake across QSIGSHARE/QSIGSESANN/QSIGSHARESINV/QGETSIGSHARES/QBSIGSHARES, LIMITED_VECTOR capping on CBatchedSigShares::sigShares, uniform peer banning, and the dedicated aggregate-boundary unit tests) was re-verified and found correct — no new defects. Two additional open public threads referenced as evidence (QBSIGSHARES inner-count banning, and a claimed SERIALIZE_METHODS read-branch compile issue) were independently checked against source and do not hold at this SHA: UnserializeBatchedSigShares is called inside the try/catch at net_signing.cpp:121-130, so a LIMITED_VECTOR throw during batch decode is caught and bans the peer; and SERIALIZE_METHODS/READWRITE dispatch Ser/Unser via compile-time tag overload resolution, so the read path is never instantiated for the const-serializing case (an already-accepted pattern used identically elsewhere in the codebase).
Source (experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_opus_sample, bucket 0): reviewers codex/general=gpt-5.6-sol(completed); sonnet5/general=claude-sonnet-5(completed); opus/general=claude-opus-4-8(completed); codex/dash-core-commit-history=gpt-5.6-sol(completed); sonnet5/dash-core-commit-history=claude-sonnet-5(completed); opus/dash-core-commit-history=claude-opus-4-8(completed); verifier=verifier-sonnet5-7418-1783751800=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol reasoning=high (orchestration-only, not a reviewer/verifier).
🟡 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/net_signing.h`:
- [SUGGESTION] src/llmq/net_signing.h:34-36: Squash the doc-fix commit into the commit that introduced the inaccuracy
Commit 430a7024021 ("llmq: correct batched sig-share decoder docs") only rewrites the two-line UnserializeBatchedSigShares doc comment that was introduced one commit earlier in this same unmerged PR, by c9a194401f6 ("llmq: cover QBSIGSHARES aggregate sig-share bound with a testable decoder"). That comment has never shipped to develop, so this is the classic same-PR fixup pattern — a review thread flagged an inaccuracy in prose that was never correct in permanent history, and the very next commit corrects it. Left as a standalone commit, it leaves a bodyless 'correct docs' entry in git log/blame immediately after the commit it patches, which is confusing once the review conversation that prompted it is gone. Fold 430a7024021 into c9a194401f6 with `git rebase -i --autosquash` (or an explicit `fixup!`) so the decoder ships with accurate documentation from the moment it's introduced. The other three commits in the stack (c0af1562af5, 89c0933aa1c, c9a194401f6) are substantive, correctly ordered, and need no change.
|
@coderabbitai review |
✅ Action performedReview finished.
|
430a702 to
4f129b6
Compare
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Carried-Forward Prior Findings
None. The sole prior finding (net_signing.h:34-36, "squash the doc-fix commit into the commit that introduced the inaccuracy") is FIXED: the commit stack at the current head is exactly 3 commits (c0af156, 89c0933, 4f129b6) with no standalone doc-fix entry, and net_signing.h:34-36 already reads the corrected 'throws std::ios_base::failure once detected' wording inside the commit that introduces UnserializeBatchedSigShares — verified directly via git log --oneline and a Read of the current source, not inferred from the empty two-dot delta alone.
New Findings In Latest Delta
None. The literal 430a702..4f129b6 delta is tree-empty (a history rewrite, not rebase noise), so there is nothing new to surface from it. I independently re-verified the cumulative diff (net_signing.cpp/.h, signing_shares.h, llmq_utils_tests.cpp) against the checked-out source at 4f129b6: all four LLMQ signing intake paths (QSIGSHARE, QSIGSESANN, QSIGSHARESINV/QGETSIGSHARES, QBSIGSHARES) bound peer-controlled counts before/while decoding; ReadCompactSize results are staged in uint64_t (no size_t narrowing); CBatchedSigShares::sigShares is capped via the standard LIMITED_VECTOR/Using<LimitedVectorFormatter<n>> template-dispatch mechanism (confirmed via serialize.h:594,996-1012 — this is compile-time Ser/Unser template dispatch, not a runtime if branch, so the previously-flagged 'compiling the read branch into serialization' concern does not apply to this code); an oversized inner vector throws std::ios_base::failure from inside UnserializeBatchedSigShares, which propagates to the QBSIGSHARES try/catch at net_signing.cpp:121-130 and results in BanNode being called (so the previously-flagged 'no local catch/ban for oversized inner counts' concern also does not apply); and the aggregate-boundary tests in llmq_utils_tests.cpp call the real production decoder UnserializeBatchedSigShares directly. No new defects found.
Source (experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_primary, bucket 1): reviewers codex/general=gpt-5.6-sol(completed); sonnet5/general=claude-sonnet-5(completed); codex/dash-core-commit-history=gpt-5.6-sol(completed); sonnet5/dash-core-commit-history=claude-sonnet-5(completed); verifier=verifier-sonnet5-7418-1783755776=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol reasoning=high (orchestration-only, not a reviewer/verifier).
Note: posted as a COMMENT review because GitHub does not allow approving my own PR.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
CI note for exact head All three retries failed at This PR changes only the LLMQ signing intake/decoder files and tests ( Occurrence recorded on #7288: #7288 (comment) |
a54fcbe fix: reject custom LLMQ sizes above the maximum supported quorum size (PastaClaw) Pull request description: ## Issue being fixed Devnet and regtest LLMQ size overrides accepted quorum sizes above 400 even though the signing protocol cannot carry them. Signing-session inventories and batched signature-share messages are capped at 400, so an oversized custom quorum could start successfully but fail later when peers exchange signing shares. ## What was done - Define `Consensus::MAX_LLMQ_SIZE` as the authoritative 400-member protocol maximum. - Tie `MAX_MSGS_TOTAL_BATCHED_SIGS` to that shared maximum so the limits cannot drift. - Reject oversized `-llmqdevnetparams`, `-llmqtestparams`, `-llmqtestinstantsendparams`, and `-llmqtestplatformparams` values during chain-parameter construction. - Cover normal custom values, the inclusive 400-member boundary, and 401-member rejection for all four override paths. This is a fail-fast configuration fix discovered while reviewing #7418. It does not change mainnet or testnet parameters, and accepted custom sizes at or below 400 retain their existing behavior. ## Validation - Full unit suite: 744 test cases passed. - `feature_llmq_signing.py`: both variants passed. - Direct startup checks: 401-member devnet/regtest overrides fail with the intended error; 400-member overrides start normally. - `git diff --check` - whitespace lint - circular-dependency lint - Independent pre-PR review: `ship`, no findings. ## Scope This intentionally adds only the missing upper bound. Existing behavior for zero, negative, or otherwise inconsistent custom size/threshold pairs is unchanged. ACKs for top commit: PastaPastaPasta: utACK a54fcbe Tree-SHA512: 95bd40c487be20866263db83104f0840e2999bac88d828c55d0584bd386cc8f2ddcff0a99dfb71b110ad5550e942f82efda909ad7624c61c72d7fcd188903eff
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
|
89c0933 is not signed, lgtm otherwise |
The QSIGSHARE, QSIGSESANN and QSIGSHARESINV/QGETSIGSHARES handlers each decoded a std::vector unbounded before comparing the resulting count against a callsite-specific cap. That lets a malicious peer force us to allocate and decode arbitrarily large batches before we reject them. Replace the post-decode size checks with the shared UnserializeVectorWithMaxSize primitive, which reads the CompactSize prefix with the default range-checked ReadCompactSize (rejecting any count above the generic MAX_SIZE cap before this tighter callsite max_size is even applied), compares the resulting size_t directly against max_size with no separate uint64_t staging, and returns false without consuming any element on rejection. Convert the false return into an ios_base::failure so the surrounding catch uniformly logs, bans the peer, and rethrows for any decode failure — matching the existing QSIGSHARESINV/QGETSIGSHARES behavior on a malformed stream. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…oder The self-review of the batched sig-share intake bounds flagged one gap: the running-total abort in the QBSIGSHARES handler — where many individually-valid batches together exceed MAX_MSGS_TOTAL_BATCHED_SIGS — had no direct unit coverage. The check lived inline in NetSigning::ProcessMessage, so the only way to reach it was to drive a full P2P message through the handler. Extract the QBSIGSHARES decode loop into a free function, llmq::UnserializeBatchedSigShares(CDataStream&), that production calls inside the existing try/catch. Wire format, peer banning, and logging semantics are unchanged — the callsite still logs, bans, and rethrows on any ios_base::failure. The extracted decoder is the exact code that runs in production, so the new tests exercise the real path rather than a mirror. Add three targeted cases in llmq_utils_tests: - oversized outer batch count (built from empty batches so only the outer-count guard, not an end-of-stream artifact, can reject it), - oversized running aggregate across batches each within the per-batch cap, - an aggregate exactly at the cap, which must decode intact. Both count guards were mutation-verified: neutralizing either guard makes its corresponding test fail and no other. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4f129b6 to
8ff75e0
Compare
✅ Action performedReview finished.
|
Depends on #7439. Please review only the two command-specific commits here.
Issue being fixed or feature implemented
LLMQ signing P2P messages previously deserialized peer-controlled vectors before enforcing the existing per-message count limits. This hardens the signing message intake path so oversized counts are rejected before vector materialization.
This intentionally does not include the QFCOMMITMENT dynamic-bitset fix.
What was done?
NetSigning::ProcessMessage.CBatchedSigShares::sigSharesdeclaratively withLIMITED_VECTOR.The reusable bounded-vector serialization primitives are introduced separately in #7439.
How Has This Been Tested?
src/test/test_dash --run_test=llmq_utils_testssrc/test/test_dash --run_test=serialize_teststest/lint/lint-whitespace.pytest/lint/lint-circular-dependencies.pyBreaking Changes
None.
Checklist: