Skip to content

fix: bound governance object signatures and reset the governance cache #7831

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
PastaPastaPasta merged 2 commits into from
Oct 10, 2026
Merged

fix: bound governance object signatures and reset the governance cache #7831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f2f1dd4
fix: reset the governance cache when upgrading
PastaPastaPasta Oct 9, 2026
9e5ee16
fix: bound governance object signatures
PastaPastaPasta Oct 9, 2026
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
8 changes: 8 additions & 0 deletions doc/release-notes-7831.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
P2P and network changes
-----------------------

- Governance object signatures larger than 96 bytes are now rejected, both from
the network and from the on-disk governance cache (`governance.dat`). The
cache format version was bumped for this: on the first start after
upgrading, the existing cache is discarded and governance objects and votes
are synced again from the network. (#7831)
5 changes: 4 additions & 1 deletion src/governance/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

#include <serialize.h>

#include <cstddef>
#include <string>
#include <vector>

Expand All @@ -33,6 +34,8 @@ namespace Governance {
class Object
{
public:
static constexpr size_t MAX_SIGNATURE_SIZE{96};

Object() = default;

Object(const uint256& nHashParent, int nRevision, int64_t nTime, const uint256& nCollateralHash, const std::string& strDataHex);
Expand Down Expand Up @@ -79,7 +82,7 @@ class Object
obj.masternodeOutpoint
);
if (!(s.GetType() & SER_GETHASH)) {
READWRITE(obj.vchSig);
READWRITE(LIMITED_VECTOR(obj.vchSig, MAX_SIGNATURE_SIZE));
}
}
};
Expand Down
2 changes: 1 addition & 1 deletion src/governance/governance.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@

#include <ranges>

const std::string GovernanceStore::SERIALIZATION_VERSION_STRING = "CGovernanceManager-Version-16";
const std::string GovernanceStore::SERIALIZATION_VERSION_STRING = "CGovernanceManager-Version-17";

namespace {
constexpr std::chrono::seconds GOVERNANCE_DELETION_DELAY{10min};
Expand Down
6 changes: 0 additions & 6 deletions src/governance/governance.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,11 +202,8 @@ class GovernanceStore
void Serialize(Stream &s) const EXCLUSIVE_LOCKS_REQUIRED(!cs_store)
{
LOCK(cs_store);
// TODO: Remove the historical invalid-vote-cache field on the next disk-format version bump.
const CacheMap<uint256, CGovernanceVote> empty_invalid_votes{MAX_CACHE_SIZE};
s << SERIALIZATION_VERSION_STRING
<< mapErasedGovernanceObjects
<< empty_invalid_votes

@knst knst Oct 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is the only reason to bump version is Remove the historical invalid-vote-cache field or there's any other reason?

Seems as unrelated for PR's purpose.

@PastaPastaPasta PastaPastaPasta Oct 9, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The field removal is not the reason for the bump. The bump is the migration for the next commit, which bounds vchSig on disk reads as well as network reads.

Proposal signatures were never bounded or checked (only triggers verify a signature), so an existing governance.dat can hold a proposal with an oversized signature. If the disk-read bound landed without a version bump, such a cache would fail to deserialize and CFlatDB would drop it with an ERROR log line. With the bump, every node drops the old cache once, on purpose, and the release note documents it.

Removing the invalid-vote-cache field only rides along because the format changes anyway; the two TODOs from 0a8c4da asked for exactly that on the next disk-format bump. Leaving it in would mean another format bump later just to remove it. I reworded the first commit description in the PR body to make that ordering clearer.

🤖 Posted autonomously by Claude on behalf of pasta.

<< cmmapOrphanVotes
<< mapObjects
<< mapLastMasternodeObject
Expand All @@ -225,10 +222,7 @@ class GovernanceStore
return;
}

// TODO: Stop consuming the historical invalid-vote-cache field on the next disk-format version bump.
CacheMap<uint256, CGovernanceVote> discarded_invalid_votes;
s >> mapErasedGovernanceObjects
>> discarded_invalid_votes
>> cmmapOrphanVotes
>> mapObjects
>> mapLastMasternodeObject
Expand Down
35 changes: 24 additions & 11 deletions src/test/governance_vote_processing_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -437,7 +437,7 @@ BOOST_AUTO_TEST_CASE(proposal_funding_votes_require_the_voting_key)
BOOST_CHECK_EQUAL(stored->GetAbsoluteYesCount(tip_mn_list(), VOTE_SIGNAL_FUNDING), 1);
}

BOOST_AUTO_TEST_CASE(legacy_invalid_vote_cache_is_discarded)
BOOST_AUTO_TEST_CASE(legacy_governance_cache_is_discarded)
{
const uint256 parent_hash{MakeProposal(uint256::ONE).GetHash()};
CKey attacker_key;
Expand All @@ -449,28 +449,41 @@ BOOST_AUTO_TEST_CASE(legacy_invalid_vote_cache_is_discarded)
CacheMap<uint256, CGovernanceVote> legacy_invalid_votes{3};
legacy_invalid_votes.Insert(forged.GetHash(), forged);
const auto proposal = std::make_shared<CGovernanceObject>(MakeProposal(uint256::ONE));
const std::map<uint256, std::shared_ptr<CGovernanceObject>> objects{{proposal->GetHash(), proposal}};

CDataStream stream{SER_DISK, CLIENT_VERSION};
stream << std::string{"CGovernanceManager-Version-16"}
// A version 16 cache, including its invalid-vote field, is dropped as a whole.
CDataStream legacy{SER_DISK, CLIENT_VERSION};
legacy << std::string{"CGovernanceManager-Version-16"}
<< std::map<uint256, int64_t>{}
<< legacy_invalid_votes
<< CacheMultiMap<uint256, governance::OrphanVote>{3}
<< std::map<uint256, std::shared_ptr<CGovernanceObject>>{{proposal->GetHash(), proposal}}
<< objects
<< std::map<COutPoint, TestGovernanceStore::last_object_rec>{}
<< CDeterministicMNList{};

TestGovernanceStore legacy_store;
legacy_store.Unserialize(legacy);
BOOST_CHECK_EQUAL(legacy_store.ObjectCount(), 0U);

// The current format has no invalid-vote field and round-trips.
CDataStream current{SER_DISK, CLIENT_VERSION};
current << std::string{"CGovernanceManager-Version-17"}
<< std::map<uint256, int64_t>{}
<< CacheMultiMap<uint256, governance::OrphanVote>{3}
<< objects
<< std::map<COutPoint, TestGovernanceStore::last_object_rec>{}
<< CDeterministicMNList{};

TestGovernanceStore store;
store.Unserialize(stream);
store.Unserialize(current);
BOOST_CHECK_EQUAL(store.ObjectCount(), 1U);

CDataStream saved{SER_DISK, CLIENT_VERSION};
store.Serialize(saved);
std::string version;
std::map<uint256, int64_t> erased_objects;
CacheMap<uint256, CGovernanceVote> saved_invalid_votes;
saved >> version >> erased_objects >> saved_invalid_votes;
BOOST_CHECK_EQUAL(version, "CGovernanceManager-Version-16");
BOOST_CHECK_EQUAL(saved_invalid_votes.GetSize(), 0U);
TestGovernanceStore reloaded;
reloaded.Unserialize(saved);
BOOST_CHECK_EQUAL(reloaded.ObjectCount(), 1U);
BOOST_CHECK(saved.empty());
}

BOOST_AUTO_TEST_SUITE_END()
21 changes: 21 additions & 0 deletions src/test/governance_vote_wire_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,27 @@ BOOST_AUTO_TEST_CASE(ser_disk_deserialization_unaffected)
BOOST_CHECK_EQUAL(ss.size(), 0U);
}

// Object signatures are bounded to 96 bytes (BLS) on network and disk reads alike.
BOOST_AUTO_TEST_CASE(object_signature_size_limit)
{
Governance::Object object;
for (int ser_type : {SER_NETWORK, SER_DISK}) {
for (size_t sig_len : {size_t{0}, size_t{65}, size_t{96}, size_t{97}}) {
object.vchSig.assign(sig_len, 0xAA);
CDataStream ss(ser_type, PROTOCOL_VERSION);
ss << object;

Governance::Object decoded;
if (sig_len <= Governance::Object::MAX_SIGNATURE_SIZE) {
BOOST_CHECK_NO_THROW(ss >> decoded);
BOOST_CHECK(decoded.vchSig == object.vchSig);
} else {
BOOST_CHECK_THROW(ss >> decoded, std::ios_base::failure);
}
}
}
}

BOOST_AUTO_TEST_CASE(extreme_timestamp_is_rejected_without_overflow)
{
CGovernanceVote vote;
Expand Down
Loading