fix(net): bound governance vote signature deserialization by thepastaclaw · Pull Request #7440 · dashpay/dash

thepastaclaw · 2026-07-10T02:38:10Z

Uses the shared bounded-vector deserialization primitive merged in #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
git diff --check upstream/develop...HEAD

thepastaclaw · 2026-07-10T05:34:39Z

@coderabbitai review

coderabbitai · 2026-07-10T05:34:47Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai · 2026-07-10T05:40:16Z

Walkthrough

Governance vote network deserialization now caps signature payloads at the BLS signature size and accepts only compact or BLS signature lengths. Malformed or truncated network votes raise std::ios_base::failure; the message handler catches this and marks the peer misbehaving. Compile-time checks tie vote constants to cryptographic signature sizes. New wire tests cover invalid, truncated, valid, and disk deserialization paths, and are included in the test build.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

dashpay/dash#7397: Also hardens malformed governance network message handling in NetGovernance::ProcessMessage.

Suggested reviewers: knst, UdjinM6

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 35.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the main change: bounding governance vote signature deserialization.
Description check	✅ Passed	The description matches the changeset and accurately describes the motivation, implementation, testing, and behavior preserved.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

github-actions · 2026-07-10T15:51:07Z

This pull request has conflicts, please rebase.

Limit network governance-vote signature reads to 96 bytes before allocation and require the structurally valid 65-byte ECDSA or 96-byte BLS encodings. Score malformed or truncated governance vote messages with 100 misbehavior points while preserving disk, hash, and outbound serialization behavior. Add focused unit coverage for malformed, oversized, and valid signature encodings.

thepastaclaw · 2026-07-10T18:30:06Z

✅ Review complete (commit 8f0b813)

knst

utACK 8f0b813

thepastaclaw

Code Review

PR #7440 correctly bounds governance-vote signature deserialization to the two legitimate wire encodings (65-byte compact ECDSA, 96-byte BLS) via LIMITED_VECTOR, throwing before allocation on oversized counts, and routes malformed/truncated votes to 100-point misbehavior scoring instead of the outer log-only handler. Disk, hash, and write serialization paths remain unbounded as intended, and static_asserts keep the size constants tied to CPubKey/CBLSSignature. The one gap is test coverage: new tests exercise only the raw stream deserialization boundary, not the handler-level peer-scoring behavior this PR introduces.

Source (experiment sonnet-primary-opus-quarter-sample-20260710, cohort sonnet_primary, bucket 2): reviewers codex/general=gpt-5.6-sol(completed); sonnet5/general=claude-sonnet-5(completed); codex/dash-core-commit-history=gpt-5.6-sol(completed); sonnet5/dash-core-commit-history=claude-sonnet-5(completed); verifier=verifier-sonnet5-7440-1783711121=claude-sonnet-5; orchestrator=openai/gpt-5.6-sol reasoning=high (orchestration-only, not a reviewer/verifier).

🟡 1 suggestion(s)

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/test/governance_vote_wire_tests.cpp`:
- [SUGGESTION] src/test/governance_vote_wire_tests.cpp:1-96: No test exercises the peer-misbehavior-scoring path this PR adds
  All four new tests call `ss >> vote` directly and assert on `std::ios_base::failure`. None of them go through `NetGovernance::ProcessMessage`, which is where this PR's actual security behavior lives: catching that exception and calling `PeerMisbehaving(peer.GetId(), 100, "malformed governance vote")` (net_governance.cpp:199-204). As written, a regression that removes the catch, changes the penalty, or forgets the `return` after scoring would leave every test in this file green while silently breaking peer-scoring. Add a handler-level test that feeds an oversized or truncated `govobjvote` payload through `NetGovernance::ProcessMessage` and asserts the peer accumulates 100 misbehavior points.

thepastaclaw · 2026-07-10T19:30:36Z

+// Copyright (c) 2026 The Dash Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <governance/vote.h>
+#include <primitives/transaction.h>
+#include <serialize.h>
+#include <streams.h>
+#include <uint256.h>
+#include <version.h>
+
+#include <test/util/setup_common.h>
+
+#include <boost/test/unit_test.hpp>
+
+#include <ios>
+#include <limits>
+#include <vector>
+
+BOOST_FIXTURE_TEST_SUITE(governance_vote_wire_tests, BasicTestingSetup)
+
+namespace {
+void WriteVoteHeader(CDataStream& ss)
+{
+    ss << COutPoint{uint256::ONE, 0} << uint256::ONE
+       << int{1} /*outcome*/ << int{1} /*signal*/ << int64_t{1'700'000'000};
+}
+
+CDataStream MakeVoteWire(size_t sig_len)
+{
+    CDataStream ss(SER_NETWORK, PROTOCOL_VERSION);
+    WriteVoteHeader(ss);
+    ss << std::vector<unsigned char>(sig_len, 0xAA);
+    return ss;
+}
+} // namespace
+
+// Reject invalid signature lengths, including a maximal CompactSize prefix.
+BOOST_AUTO_TEST_CASE(rejects_invalid_sizes)
+{
+    for (size_t bad : {size_t{0}, size_t{64}, size_t{66}, size_t{95}, size_t{97}, size_t{128}}) {
+        CDataStream ss = MakeVoteWire(bad);
+        CGovernanceVote vote;
+        BOOST_CHECK_THROW(ss >> vote, std::ios_base::failure);
+    }
+
+    CDataStream ss(SER_NETWORK, PROTOCOL_VERSION);
+    WriteVoteHeader(ss);
+    WriteCompactSize(ss, std::numeric_limits<uint64_t>::max());
+    CGovernanceVote vote;
+    BOOST_CHECK_THROW(ss >> vote, std::ios_base::failure);
+}
+
+// Truncated element bytes must surface as ios_base::failure so the govobjvote
+// handler scores the peer.
+BOOST_AUTO_TEST_CASE(truncated_signature_throws_ios_failure)
+{
+    CDataStream ss(SER_NETWORK, PROTOCOL_VERSION);
+    WriteVoteHeader(ss);
+    ss << uint8_t{CGovernanceVote::BLS_SIG_SIZE};
+    ss.write(MakeByteSpan(std::vector<unsigned char>(10, 0xBB)));
+
+    CGovernanceVote vote;
+    BOOST_CHECK_THROW(ss >> vote, std::ios_base::failure);
+}
+
+// 65-byte ECDSA and 96-byte BLS round-trip cleanly over the network.
+BOOST_AUTO_TEST_CASE(accepts_legitimate_boundary_sizes)
+{
+    for (size_t sig_len : {CGovernanceVote::COMPACT_SIG_SIZE, CGovernanceVote::BLS_SIG_SIZE}) {
+        CDataStream ss = MakeVoteWire(sig_len);
+        const size_t wire_bytes = ss.size();
+
+        CGovernanceVote vote;
+        BOOST_REQUIRE_NO_THROW(ss >> vote);
+        BOOST_CHECK_EQUAL(ss.size(), 0U);
+
+        CDataStream out(SER_NETWORK, PROTOCOL_VERSION);
+        out << vote;
+        BOOST_CHECK_EQUAL(out.size(), wire_bytes);
+    }
+}
+
+// SER_DISK reads stay unbounded — existing on-disk data must load unchanged.
+BOOST_AUTO_TEST_CASE(ser_disk_deserialization_unaffected)
+{
+    CDataStream ss(SER_DISK, PROTOCOL_VERSION);
+    WriteVoteHeader(ss);
+    ss << std::vector<unsigned char>(128, 0xCD);
+
+    CGovernanceVote vote;
+    BOOST_REQUIRE_NO_THROW(ss >> vote);
+    BOOST_CHECK_EQUAL(ss.size(), 0U);
+}
+
+BOOST_AUTO_TEST_SUITE_END()


🟡 Suggestion: No test exercises the peer-misbehavior-scoring path this PR adds

All four new tests call ss >> vote directly and assert on std::ios_base::failure. None of them go through NetGovernance::ProcessMessage, which is where this PR's actual security behavior lives: catching that exception and calling PeerMisbehaving(peer.GetId(), 100, "malformed governance vote") (net_governance.cpp:199-204). As written, a regression that removes the catch, changes the penalty, or forgets the return after scoring would leave every test in this file green while silently breaking peer-scoring. Add a handler-level test that feeds an oversized or truncated govobjvote payload through NetGovernance::ProcessMessage and asserts the peer accumulates 100 misbehavior points.

_{source: ['codex']}

thepastaclaw force-pushed the fix/governance-vote-signature-bounds branch 5 times, most recently from 8424a28 to e444dda Compare July 10, 2026 03:30

thepastaclaw changed the title ~~fix(governance): bound vote signature deserialization~~ fix(net): bound governance vote signature deserialization Jul 10, 2026

github-actions Bot added the needs rebase label Jul 10, 2026

thepastaclaw force-pushed the fix/governance-vote-signature-bounds branch from e444dda to 8f0b813 Compare July 10, 2026 15:53

github-actions Bot removed the needs rebase label Jul 10, 2026

PastaPastaPasta marked this pull request as ready for review July 10, 2026 18:28

knst approved these changes Jul 10, 2026

View reviewed changes

thepastaclaw commented Jul 10, 2026

View reviewed changes

PastaPastaPasta merged commit 24c0ead into dashpay:develop Jul 10, 2026
48 checks passed

UdjinM6 added this to the 24 milestone Jul 11, 2026

This was referenced Jul 12, 2026

refactor: clean up governance inv tests after request cache removal #7449

Open

test: make governance vote fixtures wire-valid #7450

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(net): bound governance vote signature deserialization#7440

fix(net): bound governance vote signature deserialization#7440
PastaPastaPasta merged 1 commit into
dashpay:developfrom
thepastaclaw:fix/governance-vote-signature-bounds

thepastaclaw commented Jul 10, 2026 •

edited

Loading

Uh oh!

thepastaclaw commented Jul 10, 2026

Uh oh!

coderabbitai Bot commented Jul 10, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Jul 10, 2026 •

edited

Loading

❌ Failed checks (1 warning)

Uh oh!

github-actions Bot commented Jul 10, 2026

Uh oh!

thepastaclaw commented Jul 10, 2026 •

edited

Loading

Uh oh!

knst left a comment

Uh oh!

thepastaclaw left a comment

Uh oh!

thepastaclaw Jul 10, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

thepastaclaw commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Changes

Testing

Uh oh!

thepastaclaw commented Jul 10, 2026

Uh oh!

coderabbitai Bot commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

❌ Failed checks (1 warning)

Uh oh!

github-actions Bot commented Jul 10, 2026

Uh oh!

thepastaclaw commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knst left a comment

Choose a reason for hiding this comment

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

thepastaclaw Jul 10, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thepastaclaw commented Jul 10, 2026 •

edited

Loading

coderabbitai Bot commented Jul 10, 2026 •

edited

Loading

coderabbitai Bot commented Jul 10, 2026 •

edited

Loading

thepastaclaw commented Jul 10, 2026 •

edited

Loading