Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Makefile.test.include
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ BITCOIN_TESTS =\
test/governance_inv_tests.cpp \
test/governance_superblock_tests.cpp \
test/governance_validators_tests.cpp \
test/governance_vote_wire_tests.cpp \
test/coinjoin_inouts_tests.cpp \
test/coinjoin_dstxmanager_tests.cpp \
test/coinjoin_queue_tests.cpp \
Expand Down
10 changes: 9 additions & 1 deletion src/governance/net_governance.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,15 @@ void NetGovernance::ProcessMessage(CNode& peer, const std::string& msg_type, CDa
// A NEW GOVERNANCE OBJECT VOTE HAS ARRIVED
else if (msg_type == NetMsgType::MNGOVERNANCEOBJECTVOTE) {
CGovernanceVote vote;
vRecv >> vote;
// Catch malformed/truncated votes locally so the wire-cap rejection
// in CGovernanceVote scores the peer instead of falling through to
// the outer log-only handler.
try {
vRecv >> vote;
} catch (const std::ios_base::failure&) {
m_peer_manager->PeerMisbehaving(peer.GetId(), 100, "malformed governance vote");
return;
}

uint256 nHash = vote.GetHash();

Expand Down
4 changes: 4 additions & 0 deletions src/governance/vote.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,16 @@
#include <evo/dmn_types.h>
#include <masternode/sync.h>
#include <messagesigner.h>
#include <pubkey.h>

#include <chainparams.h>
#include <logging.h>
#include <timedata.h>
#include <util/string.h>

static_assert(CGovernanceVote::COMPACT_SIG_SIZE == CPubKey::COMPACT_SIGNATURE_SIZE);
static_assert(CGovernanceVote::BLS_SIG_SIZE == CBLSSignature::SerSize);

std::string CGovernanceVoting::ConvertOutcomeToString(vote_outcome_enum_t nOutcome)
{
static const std::map<vote_outcome_enum_t, std::string> mapOutcomeString = {
Expand Down
20 changes: 19 additions & 1 deletion src/governance/vote.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include <hash.h>
#include <primitives/transaction.h>
#include <serialize.h>
#include <uint256.h>
#include <util/string.h>

Expand Down Expand Up @@ -60,6 +61,13 @@ class CGovernanceVote

friend bool operator<(const CGovernanceVote& vote1, const CGovernanceVote& vote2);

public:
// Wire-valid signature encodings: compact ECDSA voting key (FUNDING)
// or BLS operator key (other signals). Kept in sync via static_assert
// against CPubKey::COMPACT_SIGNATURE_SIZE / CBLSSignature::SerSize in vote.cpp.
static constexpr size_t COMPACT_SIG_SIZE = 65;
static constexpr size_t BLS_SIG_SIZE = 96;

private:
COutPoint masternodeOutpoint;
uint256 nParentHash;
Expand Down Expand Up @@ -123,7 +131,17 @@ class CGovernanceVote
{
READWRITE(obj.masternodeOutpoint, obj.nParentHash, obj.nVoteOutcome, obj.nVoteSignal, obj.nTime);
if (!(s.GetType() & SER_GETHASH)) {
READWRITE(obj.vchSig);
// Network reads: cap the signature vector before allocation and require
// one of the two legitimate encodings. Other paths (disk, hash, write)
// keep the unbounded default.
if (ser_action.ForRead() && (s.GetType() & SER_NETWORK)) {
READWRITE(LIMITED_VECTOR(obj.vchSig, BLS_SIG_SIZE));
if (obj.vchSig.size() != COMPACT_SIG_SIZE && obj.vchSig.size() != BLS_SIG_SIZE) {
throw std::ios_base::failure("bad governance vote signature size");
}
} else {
READWRITE(obj.vchSig);
}
}
SER_READ(obj, obj.UpdateHash());
}
Expand Down
96 changes: 96 additions & 0 deletions src/test/governance_vote_wire_tests.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
// 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()
Comment on lines +1 to +96

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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']