fix: bound SPORK signature deserialization - #7438
Conversation
a31a24c to
d73610b
Compare
|
CI diagnosis: no branch change is needed. The linux64_sqlite failure is the known pre-existing feature_dip3_v19.py SPORK-sync flake tracked in #6702, not a regression in this PR. The test exhausted all three retries waiting for wait_for_sporks_same after v19 activation setup. Detailed logs show valid 86-byte SPORK messages were deserialized and accepted normally; the one lagging node received the revert-SPORK inventory and requested the object from another peer, but never received/processed it before timeout. There was no malformed-SPORK exception, deserialization rejection, or peer penalty. The exact upstream/develop worktree at 2bbf4a4 passed feature_dip3_v19.py locally, and the identical timeout has prior occurrences on unrelated wallet, release-note-only, superblock, and LLMQ PRs. The PR is proper as-is and the branch remains untouched. Maintainers can rerun CI if concerned. |
kwvg
left a comment
There was a problem hiding this comment.
spork_tests can be dropped, feature_sporks addition is sufficient.
| // A valid 65-byte signature vector round-trips through serialize/unserialize | ||
| // with the wire bytes unchanged — the fix must not have moved the wire format | ||
| // for legit messages. | ||
| BOOST_AUTO_TEST_CASE(spork_signature_exact_size_roundtrip) |
There was a problem hiding this comment.
The fix doesn't have the means to modify the payload contents, it's unclear what the test is supposed to guard against
There was a problem hiding this comment.
Agreed and addressed in aa6b6041714. I removed the redundant unit suite; the functional test remains the end-to-end guard for rejecting an oversized serialized signature vector.
| // (allocating before it discovers there is no data) and then throws a generic | ||
| // stream underflow. On the fix, LimitedVectorFormatter rejects the count | ||
| // before any allocation via ios_base::failure. | ||
| BOOST_AUTO_TEST_CASE(spork_signature_max_size_rejected_before_allocation) |
There was a problem hiding this comment.
Check is identical to feature_sporks, can be dropped
There was a problem hiding this comment.
Addressed in aa6b6041714: dropped src/test/spork_tests.cpp and its build entry. The retained feature_sporks.py coverage exercises the concrete oversized-SPORK disconnect path, while serialize_tests retains the bounded-vector boundary coverage.
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: - #7416 — quorum-data response vectors - #7418 — LLMQ signing message vectors - #7419 — CoinJoin message vectors - #7438 — SPORK signature vector Reviewing #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
|
This pull request has conflicts, please rebase. |
Add an idiomatic serialization wrapper LIMITED_VECTOR / LimitedVectorFormatter analogous to LIMITED_STRING and backed by the existing UnserializeVectorWithMaxSize primitive. Serialization preserves the ordinary std::vector wire format so it is a drop-in wrapper for fields declared as an unbounded vector but bounded in practice; deserialization rejects a CompactSize above the compile-time limit before any element allocation via std::ios_base::failure. Apply it to CSporkMessage::vchSig with CPubKey::COMPACT_SIGNATURE_SIZE (65) as the cap. Prior to this change a peer could hand us a bogus wire count on a SPORK message, causing the vector to be resized in chunks well before we had any chance to reject the message on signature verification. Shorter well-formed signatures continue to deserialize and are rejected by the existing cryptographic invalid-spork path. Catch malformed / truncated / oversized SPORK deserialization locally in the SPORK handler and Misbehaving(100) the peer so replay is punished rather than silently dropped by the generic ProcessMessages catch. The catch is narrowed to std::ios_base::failure so unrelated internal failures during message dispatch are not attributed to the peer. Wire format for valid 65-byte messages is unchanged on every network, including testnet's different signing preimage.
Add a Boost unit suite that pins the LimitedVectorFormatter gate applied to CSporkMessage::vchSig: - the vulnerability report's exact trigger — a canonical CompactSize encoding of MAX_SIZE with no signature bytes — must be rejected before allocation; the predicate matches on the formatter's "Vector length limit exceeded" so the assertion fails on the vulnerable implementation (which throws a downstream stream-underflow) and passes only on the fix; - a +1 boundary with a full payload short-circuits before element decoding even when every promised byte is on the wire; - a valid 65-byte signature round-trips through serialize/unserialize with the wire bytes unchanged, guaranteeing wire compatibility. Shorter well-formed signatures are intentionally left to the existing cryptographic invalid-spork punishment path, so no deserialize-time too-short assertion is added. Extend feature_sporks.py with the smallest practical p2p assertion: send a malformed SPORK carrying the oversized count through a dedicated peer and require the node to disconnect it. Existing propagation, GETSPORKS, persistence, and multi-node relay coverage is preserved.
Reviewer feedback (kwvg) on dashpay#7438: the CSporkMessage unit checks in spork_tests.cpp are redundant and unclear. The generic bounded-vector primitive is already exercised thoroughly by serialize_tests.cpp (within-limit round trip, wire compatibility, exact/one-over boundary, no-element-decode-on-reject, MAX_SIZE / 0xfe / 0xff CompactSize gate, unbounded serialization), and the concrete CSporkMessage oversized-signature path is covered end-to-end by feature_sporks.py, which sends a MAX_SIZE signature-length prefix and asserts the peer is disconnected rather than silently dropped. Remove the file and its src/Makefile.test.include entry. The deserialization bound itself (LIMITED_VECTOR on CSporkMessage::vchSig), the serialize.h primitives, the net_processing peer-attribution, and the serialize_tests / feature_sporks coverage are all retained. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
aa6b604 to
3cb73fd
Compare
|
✅ Review complete (commit 3cb73fd) |
|
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)
WalkthroughSPORK message signatures are now serialized and deserialized with a bounded size. SPORK processing catches deserialization failures, marks the peer misbehaving with a score of 100, and stops processing the message. The functional test constructs an oversized signature payload and verifies that the peer disconnects. Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
The production change correctly bounds SPORK signatures and penalizes malformed messages without changing valid serialization. Two in-scope suggestions remain: directly test the bound on CSporkMessage, and correct a commit message that claims to introduce primitives already present at the base.
Source: experiment sonnet-primary-opus-quarter-sample-20260710; reviewers codex/gpt-5.6-sol (general and dash-core-commit-history, high reasoning, ok) and sonnet5/claude-sonnet-5 (general and dash-core-commit-history, high effort, failed quota); final verifier codex/gpt-5.6-sol (Sol fallback after sonnet5/claude-sonnet-5 failed quota); orchestrator openai/gpt-5.6-sol (high reasoning, orchestration-only).
🟡 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 `test/functional/feature_sporks.py`:
- [SUGGESTION] test/functional/feature_sporks.py:123-129: Directly test the CSporkMessage allocation bound
This test verifies disconnection but does not prove that CSporkMessage rejects the declared signature size before allocation. If its field reverted to ordinary vector deserialization while the new local catch remained, the MAX_SIZE input would allocate the first 5 MiB batch, fail on missing payload bytes, and still disconnect the peer. The generic serialization tests validate LimitedVectorFormatter itself, not its application to CSporkMessage. Retain a focused unit test that deserializes a complete 66-byte signature and expects std::ios_base::failure; that fails with the unbounded field and directly protects the security invariant without depending on an exception-message sentinel.
In `<commit:d2e600f>`:
- [SUGGESTION] <commit:d2e600f>:1: Correct the commit message's prerequisite attribution
The opening paragraph says this commit adds LIMITED_VECTOR and LimitedVectorFormatter, but both were introduced by af6a0fda99a and are already present at base 9474fc5e2a1. This commit only applies the existing formatter to CSporkMessage and adds peer punishment. Reword the paragraph so the permanent history accurately describes the diff.
| # Oversized signature length prefix must trigger disconnect, not silent drop. | ||
| MAX_SIZE = 0x02000000 | ||
| bad_spork = msg_spork_raw() | ||
| bad_spork.raw = struct.pack("<iqq", 10001, 0, 0) + ser_compact_size(MAX_SIZE) | ||
| bad_peer = self.nodes[0].add_p2p_connection(SporkP2PInterface()) | ||
| bad_peer.send_message(bad_spork) | ||
| bad_peer.wait_for_disconnect() |
There was a problem hiding this comment.
🟡 Suggestion: Directly test the CSporkMessage allocation bound
This test verifies disconnection but does not prove that CSporkMessage rejects the declared signature size before allocation. If its field reverted to ordinary vector deserialization while the new local catch remained, the MAX_SIZE input would allocate the first 5 MiB batch, fail on missing payload bytes, and still disconnect the peer. The generic serialization tests validate LimitedVectorFormatter itself, not its application to CSporkMessage. Retain a focused unit test that deserializes a complete 66-byte signature and expects std::ios_base::failure; that fails with the unbounded field and directly protects the security invariant without depending on an exception-message sentinel.
source: ['codex-general']
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
3cb73fd test: drop redundant spork_tests unit suite (PastaClaw) 44a8fe1 test: regression coverage for CSporkMessage signature-size bound (PastaClaw) d2e600f fix: bound CSporkMessage signature vector allocation (PastaClaw) Pull request description: Depends on dashpay#7439. Please review only the two SPORK-specific commits here. ## What changed - bound `CSporkMessage::vchSig` to `CPubKey::COMPACT_SIGNATURE_SIZE` before vector allocation using #7439s `LIMITED_VECTOR` formatter - punish peers that send malformed, truncated, or oversized SPORK messages - add unit coverage for the exact `CompactSize(MAX_SIZE)` trigger and a P2P disconnect regression test ## Why A malformed SPORK message could declare a large signature length in a small payload. Generic byte-vector deserialization would allocate its first 5,000,000-byte chunk before discovering the payload was truncated. The resulting exception was caught by the generic message-processing handler without penalizing the peer, allowing repeated attempts on the same connection. Valid SPORK signatures are 65-byte compact signatures on every network. The testnet distinction changes the signing preimage, not the signature encoding. Shorter well-formed signatures continue through the existing invalid-signature path and are punished there; the formatter prevents oversized declarations from allocating first. The reusable bounded-vector serialization primitives are introduced separately in dashpay#7439. ## Validation - `src/test/test_dash --run_test=spork_tests,serialize_tests` - `test/functional/feature_sporks.py` The exact unit and functional regressions were also run against the vulnerable implementation: the unit test observed the old EOF-after-allocation path, and the malformed P2P peer was not disconnected. ACKs for top commit: PastaPastaPasta: utACK 3cb73fd kwvg: utACK 3cb73fd Tree-SHA512: d547f589c3920baaacb121c618b812314901639c449bd562ea150de3a8743e3fbff10c1c92f2e192de776452d74ad7dc9be02622ddd29d9e5f7e43eacd966fd2
Match the v23.1.8 release tree file mode for the SPORK functional test script after the dashpay#7438 backport resolution.
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
3cb73fd test: drop redundant spork_tests unit suite (PastaClaw) 44a8fe1 test: regression coverage for CSporkMessage signature-size bound (PastaClaw) d2e600f fix: bound CSporkMessage signature vector allocation (PastaClaw) Pull request description: Depends on dashpay#7439. Please review only the two SPORK-specific commits here. ## What changed - bound `CSporkMessage::vchSig` to `CPubKey::COMPACT_SIGNATURE_SIZE` before vector allocation using #7439s `LIMITED_VECTOR` formatter - punish peers that send malformed, truncated, or oversized SPORK messages - add unit coverage for the exact `CompactSize(MAX_SIZE)` trigger and a P2P disconnect regression test ## Why A malformed SPORK message could declare a large signature length in a small payload. Generic byte-vector deserialization would allocate its first 5,000,000-byte chunk before discovering the payload was truncated. The resulting exception was caught by the generic message-processing handler without penalizing the peer, allowing repeated attempts on the same connection. Valid SPORK signatures are 65-byte compact signatures on every network. The testnet distinction changes the signing preimage, not the signature encoding. Shorter well-formed signatures continue through the existing invalid-signature path and are punished there; the formatter prevents oversized declarations from allocating first. The reusable bounded-vector serialization primitives are introduced separately in dashpay#7439. ## Validation - `src/test/test_dash --run_test=spork_tests,serialize_tests` - `test/functional/feature_sporks.py` The exact unit and functional regressions were also run against the vulnerable implementation: the unit test observed the old EOF-after-allocation path, and the malformed P2P peer was not disconnected. ACKs for top commit: PastaPastaPasta: utACK 3cb73fd kwvg: utACK 3cb73fd Tree-SHA512: d547f589c3920baaacb121c618b812314901639c449bd562ea150de3a8743e3fbff10c1c92f2e192de776452d74ad7dc9be02622ddd29d9e5f7e43eacd966fd2
…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#7438 (upstream merge 4537f37, cherry-picked with -m1). v23.1.x adaptations: (1) on this branch spork messages are deserialized in CSporkManager::ProcessSpork (src/spork.cpp), not in net_processing's SPORK handler as on develop - the same try/catch is applied there, reporting the failure as MisbehavingError{100, ...} through the existing MessageProcessingResult path (equivalent to develop's Misbehaving(*peer, 100, ...)). (2) The functional test's msg_spork_raw/SporkP2PInterface helpers come from dashpay#7343 (commit 60efd84), which is not on this branch; they are included verbatim, and the new test registers MESSAGEMAP[b"spork"] itself since dashpay#7343's test method (which did the registration on develop) is absent here. The spork.h LIMITED_VECTOR bound is unchanged from upstream. (cherry picked from commit 4537f37d51d5b62be3aca4bfc9e40404db76ffc9)
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#7438 (upstream merge 4537f37, cherry-picked with -m1). v23.1.x adaptations: (1) on this branch spork messages are deserialized in CSporkManager::ProcessSpork (src/spork.cpp), not in net_processing's SPORK handler as on develop - the same try/catch is applied there, reporting the failure as MisbehavingError{100, ...} through the existing MessageProcessingResult path (equivalent to develop's Misbehaving(*peer, 100, ...)). (2) The functional test's msg_spork_raw/SporkP2PInterface helpers come from dashpay#7343 (commit 60efd84), which is not on this branch; they are included verbatim, and the new test registers MESSAGEMAP[b"spork"] itself since dashpay#7343's test method (which did the registration on develop) is absent here. The spork.h LIMITED_VECTOR bound is unchanged from upstream. (cherry picked from commit 4537f37d51d5b62be3aca4bfc9e40404db76ffc9)
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#7438 (upstream merge 4537f37, cherry-picked with -m1). v23.1.x adaptations: (1) on this branch spork messages are deserialized in CSporkManager::ProcessSpork (src/spork.cpp), not in net_processing's SPORK handler as on develop - the same try/catch is applied there, reporting the failure as MisbehavingError{100, ...} through the existing MessageProcessingResult path (equivalent to develop's Misbehaving(*peer, 100, ...)). (2) The functional test's msg_spork_raw/SporkP2PInterface helpers come from dashpay#7343 (commit 60efd84), which is not on this branch; they are included verbatim, and the new test registers MESSAGEMAP[b"spork"] itself since dashpay#7343's test method (which did the registration on develop) is absent here. The spork.h LIMITED_VECTOR bound is unchanged from upstream. (cherry picked from commit 4537f37d51d5b62be3aca4bfc9e40404db76ffc9)
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
728f505 doc: record the v23.1.8 critical crash fixes in the release notes (pasta) ca56af8 fix(llmq): reject parentless quorum base blocks instead of terminating (pasta) a801d3f fix(llmq): reject unregistered LLMQ types from qsigshare before quorum lookup (pasta) 6c1f611 fix: skip already-removed conflicts when a ProTx key change clears the mempool (pasta) 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: ## Issue being fixed or feature implemented Merges master (v23.1.8) back into develop so develop carries the v23.1.8 release and, most importantly, the three remotely reachable crash fixes shipped on top of it. Without this, develop-based nodes remain vulnerable to all three crashes. ## What was done? Merged `upstream/master` into `develop`. The genuinely new payload is: - **Three remote-crash fixes** (with their regression tests): - skip already-removed conflicts when a ProTx key change clears the mempool (`txmempool.cpp`) - reject unregistered LLMQ types from `qsigshare` before quorum lookup (`llmq/blockprocessor.cpp`, `llmq/quorumsman.cpp`) - reject parentless quorum base blocks instead of terminating (`llmq/commitment.cpp`, `llmq/utils.cpp`, `validation.{cpp,h}`) - **Release bookkeeping**: v23.1.8 release notes, archived 23.1.7 notes, flatpak release entry, regenerated v23.1.8 man pages, and the `configure.ac` version bump (develop keeps `IS_RELEASE=false` and its own configure flags). Everything else on master since the last merge (the v23.1.x security-hardening train, #7259–#7465 and #7398) was dual-merged and already exists on develop as its own merge commits, so all conflicts from those files resolve to develop's side. Deliberate adaptations, itemized in the merge commit message: - The `qsigshare` LLMQ-type gate is ported into develop's `net_signing.cpp` and the parentless-base DKG check into develop's `net_dkg.cpp` (develop moved message processing out of `signing_shares.cpp`/`dkgsessionmgr.cpp`). - The defence-in-depth `find()` lookup is additionally applied to `CQuorumManager::GetCachedMutableQuorum()`, a develop-only method reached with a wire-supplied LLMQ type from the `QDATA` handler. - The new mempool regression test is adapted to develop's `CreateProRegTx`/`CreateProUpServTx`/`CreateProUpRevTx` helper signatures and `MemPoolOptionsForTest`. - `src/active/quorums.cpp` (deleted by develop's ActiveContext refactor) stays deleted; develop's #7416 equivalent already covers the new layout. ## How Has This Been Tested? Built with `--enable-debug` on macOS (arm64). Ran the new/extended suites — `evo_deterministicmns_tests`, `llmq_invalid_type_tests`, `evo_utils_tests` — plus adjacent ones (`llmq_signing_tests`, `llmq_dkg_tests`, `llmq_blockprocessor_tests`, `llmq_commitment_tests`, `llmq_utils_tests`, `mempool_tests`, `validation_tests`): all pass. The three regression tests abort the process on unpatched code. ## Breaking Changes None. `ChainstateManager::IsQuorumTypeEnabled()` loosens its `pindexPrev` parameter from `gsl::not_null` to a plain pointer (null now returns false instead of aborting); all existing callers are unaffected. ## 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 - [x] 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 e15bb64 Tree-SHA512: 0897f0e1267c4083ef505127ceb18309e310caf36a066674ea298d66836afb86c762532e7cc68d6c0761142b2e5b291b497496b7e82d0f7ce60549761c0ebdd8
Depends on #7439. Please review only the two SPORK-specific commits here.
What changed
CSporkMessage::vchSigtoCPubKey::COMPACT_SIGNATURE_SIZEbefore vector allocation using #7439sLIMITED_VECTORformatterCompactSize(MAX_SIZE)trigger and a P2P disconnect regression testWhy
A malformed SPORK message could declare a large signature length in a small payload. Generic byte-vector deserialization would allocate its first 5,000,000-byte chunk before discovering the payload was truncated. The resulting exception was caught by the generic message-processing handler without penalizing the peer, allowing repeated attempts on the same connection.
Valid SPORK signatures are 65-byte compact signatures on every network. The testnet distinction changes the signing preimage, not the signature encoding. Shorter well-formed signatures continue through the existing invalid-signature path and are punished there; the formatter prevents oversized declarations from allocating first.
The reusable bounded-vector serialization primitives are introduced separately in #7439.
Validation
src/test/test_dash --run_test=spork_tests,serialize_teststest/functional/feature_sporks.pyThe exact unit and functional regressions were also run against the vulnerable implementation: the unit test observed the old EOF-after-allocation path, and the malformed P2P peer was not disconnected.