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: 📥 CommitsReviewing files that changed from the base of the PR and between 4f129b609e3aed8c55eb9006b06982fa1c88d878 and 8ff75e0. 📒 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 |
![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: 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".
| @@ -99,13 +117,19 @@ void NetSigning::ProcessMessage(CNode& pfrom, const std::string& msg_type, CData | |||
| } | |||
| } else if (msg_type == NetMsgType::QBSIGSHARES) { | |||
| 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".
| @@ -154,7 +154,21 @@ class CBatchedSigShares | |||
| public: | |||
| SERIALIZE_METHODS(CBatchedSigShares, obj) | |||
| { | |||
| 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
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.
thepastaclaw
changed the title
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
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
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
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The delta since the prior clean review at d862d6cf 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
…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.
|
af6a0fd feat(serialize): add bounded-vector deserialization primitives (PastaClaw) Pull request description: ## Issue being fixed or feature implemented Network messages often have protocol-specific vector limits below the generic serialization limit. Callers need to enforce those limits before vector allocation and element decoding, without changing the wire format. ## What was done? - Factor the existing batched vector element decoder into a shared internal helper. - Add `UnserializeVectorWithMaxSize` for runtime bounds. - Add `LIMITED_VECTOR` / `LimitedVectorFormatter` for compile-time bounds in `READWRITE` declarations. - Keep serialization byte-for-byte compatible with ordinary vectors; only deserialization is bounded. - Compare CompactSize counts before narrowing or allocating, including counts at and above `MAX_SIZE`. ## Stacked adopters Each consumer remains a separate command-specific PR: - dashpay#7416 — quorum-data response vectors - dashpay#7418 — LLMQ signing message vectors - dashpay#7419 — CoinJoin message vectors - dashpay#7438 — SPORK signature vector Reviewing dashpay#7439 first leaves each child PR with only its protocol-specific policy, punishment, and regression tests. ## How Has This Been Tested? - `src/test/test_dash --run_test=serialize_tests` - Exact and over-limit boundaries, zero limits, custom element formatters, `MAX_SIZE` and `MAX_SIZE + 1` declarations, 64-bit CompactSize counts, wire compatibility, and rejection before element decode are covered. ## Breaking Changes None. Existing vector serialization and deserialization behavior is unchanged. ## 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 Top commit has no ACKs. Tree-SHA512: 61e4183a5a2a173254f3105d30969704c2c53dac3ce421f0b3e8e989bc87ec1a94d2757944694db219802403a668d7460943d507ab7948db30b97145d57f26d6
8ff75e0 llmq: cover QBSIGSHARES aggregate sig-share bound with a testable decoder (PastaClaw) e51e304 llmq: bound LLMQ signing vector intake (PastaClaw) c0af156 llmq: bound batched sig-share intake (PastaClaw) Pull request description: Depends on dashpay#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? - Use the shared runtime-bounded vector reader for signing message batches in `NetSigning::ProcessMessage`. - Bound `CBatchedSigShares::sigShares` declaratively with `LIMITED_VECTOR`. - Retain explicit running-total accounting for QBSIGSHARES, where many individually valid batches could exceed the aggregate cap. - Ban peers on malformed or oversized signing vectors. - Add unit coverage for the exact boundary and oversized batched sig-share vectors. The reusable bounded-vector serialization primitives are introduced separately in dashpay#7439. ## How Has This Been Tested? - `src/test/test_dash --run_test=llmq_utils_tests` - `src/test/test_dash --run_test=serialize_tests` - `test/lint/lint-whitespace.py` - `test/lint/lint-circular-dependencies.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: UdjinM6: utACK 8ff75e0 Tree-SHA512: 873fb78ef55749449c0548828dc8709bf0709637732a000bd009d8bba9ed0270e1582eb0020f852c3a8e8bb6d448d1f7b8c0402e271ac42155081a9d13d25bc8
af6a0fd feat(serialize): add bounded-vector deserialization primitives (PastaClaw) Pull request description: ## Issue being fixed or feature implemented Network messages often have protocol-specific vector limits below the generic serialization limit. Callers need to enforce those limits before vector allocation and element decoding, without changing the wire format. ## What was done? - Factor the existing batched vector element decoder into a shared internal helper. - Add `UnserializeVectorWithMaxSize` for runtime bounds. - Add `LIMITED_VECTOR` / `LimitedVectorFormatter` for compile-time bounds in `READWRITE` declarations. - Keep serialization byte-for-byte compatible with ordinary vectors; only deserialization is bounded. - Compare CompactSize counts before narrowing or allocating, including counts at and above `MAX_SIZE`. ## Stacked adopters Each consumer remains a separate command-specific PR: - dashpay#7416 — quorum-data response vectors - dashpay#7418 — LLMQ signing message vectors - dashpay#7419 — CoinJoin message vectors - dashpay#7438 — SPORK signature vector Reviewing dashpay#7439 first leaves each child PR with only its protocol-specific policy, punishment, and regression tests. ## How Has This Been Tested? - `src/test/test_dash --run_test=serialize_tests` - Exact and over-limit boundaries, zero limits, custom element formatters, `MAX_SIZE` and `MAX_SIZE + 1` declarations, 64-bit CompactSize counts, wire compatibility, and rejection before element decode are covered. ## Breaking Changes None. Existing vector serialization and deserialization behavior is unchanged. ## 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 Top commit has no ACKs. Tree-SHA512: 61e4183a5a2a173254f3105d30969704c2c53dac3ce421f0b3e8e989bc87ec1a94d2757944694db219802403a668d7460943d507ab7948db30b97145d57f26d6
8ff75e0 llmq: cover QBSIGSHARES aggregate sig-share bound with a testable decoder (PastaClaw) e51e304 llmq: bound LLMQ signing vector intake (PastaClaw) c0af156 llmq: bound batched sig-share intake (PastaClaw) Pull request description: Depends on dashpay#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? - Use the shared runtime-bounded vector reader for signing message batches in `NetSigning::ProcessMessage`. - Bound `CBatchedSigShares::sigShares` declaratively with `LIMITED_VECTOR`. - Retain explicit running-total accounting for QBSIGSHARES, where many individually valid batches could exceed the aggregate cap. - Ban peers on malformed or oversized signing vectors. - Add unit coverage for the exact boundary and oversized batched sig-share vectors. The reusable bounded-vector serialization primitives are introduced separately in dashpay#7439. ## How Has This Been Tested? - `src/test/test_dash --run_test=llmq_utils_tests` - `src/test/test_dash --run_test=serialize_tests` - `test/lint/lint-whitespace.py` - `test/lint/lint-circular-dependencies.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: UdjinM6: utACK 8ff75e0 Tree-SHA512: 873fb78ef55749449c0548828dc8709bf0709637732a000bd009d8bba9ed0270e1582eb0020f852c3a8e8bb6d448d1f7b8c0402e271ac42155081a9d13d25bc8
…lization 8f0b813 fix(governance): bound vote signature deserialization (PastaClaw) Pull request description: Uses the shared bounded-vector deserialization primitive merged in dashpay#7439. ## Motivation Governance vote signatures were deserialized through the generic byte-vector path. A peer could declare a very large signature length, causing allocation before the stream reported truncation. The outer message-processing catch did not score or disconnect the peer, allowing repeated malformed messages. ## Changes - bound network governance-vote signature reads to 96 bytes before allocation - require one of the two structurally valid encodings: 65-byte compact ECDSA or 96-byte BLS - score malformed or truncated governance vote messages with 100 misbehavior points - preserve disk, hash, and outbound serialization behavior - add focused unit coverage ## Testing - `./src/test/test_dash --run_test=governance_vote_wire_tests` (4/4 tests) - `./src/test/test_dash --run_test=serialize_tests` (10/10 tests) - `test/lint/lint-python.py` Tree-SHA512: backported to v23.1.x by cherry-picking 8f0b813 (applies cleanly). Backport note for v23.1.8 ------------------------- This was missing from the original v23.1.8 branch while its test follow-up dashpay#7450 ("test: make governance vote fixtures wire-valid", 915566d) was already included. That ordering was inverted: dashpay#7450 exists solely to adapt the dashpay#7442 governance-inv fixtures to the bound that dashpay#7440 introduces. Verified by removing dashpay#7450's SetSignature() line and rebuilding: without dashpay#7440 present the fixtures pass regardless, and re-adding dashpay#7440 reproduces exactly the six governance_inv_tests failures dashpay#7450's description cites. So the branch was shipping the compensating test change for a hardening fix it did not have, leaving CGovernanceVote::vchSig unbounded on the network path. The prerequisite dashpay#7439 (LIMITED_VECTOR) is already present via 099b99d, as are the sibling bounding backports dashpay#7416/dashpay#7418/dashpay#7419/dashpay#7438/dashpay#7444, so this restores the intended set rather than widening release scope. Reported-by: UdjinM6 Co-Authored-By: Claude <noreply@anthropic.com>
af6a0fd feat(serialize): add bounded-vector deserialization primitives (PastaClaw) Pull request description: ## Issue being fixed or feature implemented Network messages often have protocol-specific vector limits below the generic serialization limit. Callers need to enforce those limits before vector allocation and element decoding, without changing the wire format. ## What was done? - Factor the existing batched vector element decoder into a shared internal helper. - Add `UnserializeVectorWithMaxSize` for runtime bounds. - Add `LIMITED_VECTOR` / `LimitedVectorFormatter` for compile-time bounds in `READWRITE` declarations. - Keep serialization byte-for-byte compatible with ordinary vectors; only deserialization is bounded. - Compare CompactSize counts before narrowing or allocating, including counts at and above `MAX_SIZE`. ## Stacked adopters Each consumer remains a separate command-specific PR: - dashpay#7416 — quorum-data response vectors - dashpay#7418 — LLMQ signing message vectors - dashpay#7419 — CoinJoin message vectors - dashpay#7438 — SPORK signature vector Reviewing dashpay#7439 first leaves each child PR with only its protocol-specific policy, punishment, and regression tests. ## How Has This Been Tested? - `src/test/test_dash --run_test=serialize_tests` - Exact and over-limit boundaries, zero limits, custom element formatters, `MAX_SIZE` and `MAX_SIZE + 1` declarations, 64-bit CompactSize counts, wire compatibility, and rejection before element decode are covered. ## Breaking Changes None. Existing vector serialization and deserialization behavior is unchanged. ## 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 Top commit has no ACKs. Tree-SHA512: 61e4183a5a2a173254f3105d30969704c2c53dac3ce421f0b3e8e989bc87ec1a94d2757944694db219802403a668d7460943d507ab7948db30b97145d57f26d6 (cherry picked from commit 9474fc5)
Backport of dashpay#7418 (upstream merge 2406ebc, cherry-picked with -m1). v23.1.x adaptations: develop's QSIGSHARE/QSIGSESANN/QSIGSHARESINV/QGETSIGSHARES/QBSIGSHARES intake lives in NetSigning::ProcessMessage (moved there by the out-of-scope dashpay#7115 refactor); on this branch the same handlers live in CSigSharesManager::ProcessMessage and receive the identical transformation (UnserializeVectorWithMaxSize pre-decode bounds with log+ban+rethrow, and the UnserializeBatchedSigShares running-total decode for QBSIGSHARES). This branch keeps separate MAX_MSGS_CNT_QSIGSHARESINV/MAX_MSGS_CNT_QGETSIGSHARES constants where develop has a single merged MAX_MSGS_CNT_QSIGSHARES; both are 200, so the effective bounds are unchanged. The UnserializeBatchedSigShares helper and its doc comment live in signing_shares.{h,cpp} beside the handlers rather than in net_signing (develop's location), which here would create a signing_shares<->net_signing circular include. The CBatchedSigShares LIMITED_VECTOR change and the new unit tests are unchanged from upstream. (cherry picked from commit 2406ebcb9593e01ed4d55ac0dcf2a06423b8f2ad)
af6a0fd feat(serialize): add bounded-vector deserialization primitives (PastaClaw) Pull request description: ## Issue being fixed or feature implemented Network messages often have protocol-specific vector limits below the generic serialization limit. Callers need to enforce those limits before vector allocation and element decoding, without changing the wire format. ## What was done? - Factor the existing batched vector element decoder into a shared internal helper. - Add `UnserializeVectorWithMaxSize` for runtime bounds. - Add `LIMITED_VECTOR` / `LimitedVectorFormatter` for compile-time bounds in `READWRITE` declarations. - Keep serialization byte-for-byte compatible with ordinary vectors; only deserialization is bounded. - Compare CompactSize counts before narrowing or allocating, including counts at and above `MAX_SIZE`. ## Stacked adopters Each consumer remains a separate command-specific PR: - dashpay#7416 — quorum-data response vectors - dashpay#7418 — LLMQ signing message vectors - dashpay#7419 — CoinJoin message vectors - dashpay#7438 — SPORK signature vector Reviewing dashpay#7439 first leaves each child PR with only its protocol-specific policy, punishment, and regression tests. ## How Has This Been Tested? - `src/test/test_dash --run_test=serialize_tests` - Exact and over-limit boundaries, zero limits, custom element formatters, `MAX_SIZE` and `MAX_SIZE + 1` declarations, 64-bit CompactSize counts, wire compatibility, and rejection before element decode are covered. ## Breaking Changes None. Existing vector serialization and deserialization behavior is unchanged. ## 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 Top commit has no ACKs. Tree-SHA512: 61e4183a5a2a173254f3105d30969704c2c53dac3ce421f0b3e8e989bc87ec1a94d2757944694db219802403a668d7460943d507ab7948db30b97145d57f26d6 (cherry picked from commit 9474fc5)
Backport of dashpay#7418 (upstream merge 2406ebc, cherry-picked with -m1). v23.1.x adaptations: develop's QSIGSHARE/QSIGSESANN/QSIGSHARESINV/QGETSIGSHARES/QBSIGSHARES intake lives in NetSigning::ProcessMessage (moved there by the out-of-scope dashpay#7115 refactor); on this branch the same handlers live in CSigSharesManager::ProcessMessage and receive the identical transformation (UnserializeVectorWithMaxSize pre-decode bounds with log+ban+rethrow, and the UnserializeBatchedSigShares running-total decode for QBSIGSHARES). This branch keeps separate MAX_MSGS_CNT_QSIGSHARESINV/MAX_MSGS_CNT_QGETSIGSHARES constants where develop has a single merged MAX_MSGS_CNT_QSIGSHARES; both are 200, so the effective bounds are unchanged. The UnserializeBatchedSigShares helper and its doc comment live in signing_shares.{h,cpp} beside the handlers rather than in net_signing (develop's location), which here would create a signing_shares<->net_signing circular include. The CBatchedSigShares LIMITED_VECTOR change and the new unit tests are unchanged from upstream. (cherry picked from commit 2406ebcb9593e01ed4d55ac0dcf2a06423b8f2ad)
Backport of dashpay#7418 (upstream merge 2406ebc, cherry-picked with -m1). v23.1.x adaptations: develop's QSIGSHARE/QSIGSESANN/QSIGSHARESINV/QGETSIGSHARES/QBSIGSHARES intake lives in NetSigning::ProcessMessage (moved there by the out-of-scope dashpay#7115 refactor); on this branch the same handlers live in CSigSharesManager::ProcessMessage and receive the identical transformation (UnserializeVectorWithMaxSize pre-decode bounds with log+ban+rethrow, and the UnserializeBatchedSigShares running-total decode for QBSIGSHARES). This branch keeps separate MAX_MSGS_CNT_QSIGSHARESINV/MAX_MSGS_CNT_QGETSIGSHARES constants where develop has a single merged MAX_MSGS_CNT_QSIGSHARES; both are 200, so the effective bounds are unchanged. The UnserializeBatchedSigShares helper and its doc comment live in signing_shares.{h,cpp} beside the handlers rather than in net_signing (develop's location), which here would create a signing_shares<->net_signing circular include. The CBatchedSigShares LIMITED_VECTOR change and the new unit tests are unchanged from upstream. (cherry picked from commit 2406ebcb9593e01ed4d55ac0dcf2a06423b8f2ad)
af6a0fd feat(serialize): add bounded-vector deserialization primitives (PastaClaw) Pull request description: ## Issue being fixed or feature implemented Network messages often have protocol-specific vector limits below the generic serialization limit. Callers need to enforce those limits before vector allocation and element decoding, without changing the wire format. ## What was done? - Factor the existing batched vector element decoder into a shared internal helper. - Add `UnserializeVectorWithMaxSize` for runtime bounds. - Add `LIMITED_VECTOR` / `LimitedVectorFormatter` for compile-time bounds in `READWRITE` declarations. - Keep serialization byte-for-byte compatible with ordinary vectors; only deserialization is bounded. - Compare CompactSize counts before narrowing or allocating, including counts at and above `MAX_SIZE`. ## Stacked adopters Each consumer remains a separate command-specific PR: - dashpay#7416 — quorum-data response vectors - dashpay#7418 — LLMQ signing message vectors - dashpay#7419 — CoinJoin message vectors - dashpay#7438 — SPORK signature vector Reviewing dashpay#7439 first leaves each child PR with only its protocol-specific policy, punishment, and regression tests. ## How Has This Been Tested? - `src/test/test_dash --run_test=serialize_tests` - Exact and over-limit boundaries, zero limits, custom element formatters, `MAX_SIZE` and `MAX_SIZE + 1` declarations, 64-bit CompactSize counts, wire compatibility, and rejection before element decode are covered. ## Breaking Changes None. Existing vector serialization and deserialization behavior is unchanged. ## 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 Top commit has no ACKs. Tree-SHA512: 61e4183a5a2a173254f3105d30969704c2c53dac3ce421f0b3e8e989bc87ec1a94d2757944694db219802403a668d7460943d507ab7948db30b97145d57f26d6 (cherry picked from commit 9474fc5)
Backport of dashpay#7418 (upstream merge 2406ebc, cherry-picked with -m1). v23.1.x adaptations: develop's QSIGSHARE/QSIGSESANN/QSIGSHARESINV/QGETSIGSHARES/QBSIGSHARES intake lives in NetSigning::ProcessMessage (moved there by the out-of-scope dashpay#7115 refactor); on this branch the same handlers live in CSigSharesManager::ProcessMessage and receive the identical transformation (UnserializeVectorWithMaxSize pre-decode bounds with log+ban+rethrow, and the UnserializeBatchedSigShares running-total decode for QBSIGSHARES). This branch keeps separate MAX_MSGS_CNT_QSIGSHARESINV/MAX_MSGS_CNT_QGETSIGSHARES constants where develop has a single merged MAX_MSGS_CNT_QSIGSHARES; both are 200, so the effective bounds are unchanged. The UnserializeBatchedSigShares helper and its doc comment live in signing_shares.{h,cpp} beside the handlers rather than in net_signing (develop's location), which here would create a signing_shares<->net_signing circular include. The CBatchedSigShares LIMITED_VECTOR change and the new unit tests are unchanged from upstream. (cherry picked from commit 2406ebcb9593e01ed4d55ac0dcf2a06423b8f2ad)
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
4 participants
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: