Skip to content

fix: stop caching invalid governance votes #7569

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 1 commit into from
Aug 11, 2026
Merged

fix: stop caching invalid governance votes #7569

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 0 additions & 13 deletions src/governance/governance.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ GovernanceStore::GovernanceStore() :
cs_store(),
mapObjects(),
mapErasedGovernanceObjects(),
cmapInvalidVotes(MAX_CACHE_SIZE),
cmmapOrphanVotes(MAX_CACHE_SIZE),
mapLastMasternodeObject(),
lastMNListForVotingKeys(std::make_shared<CDeterministicMNList>())
Expand Down Expand Up @@ -816,14 +815,6 @@ bool CGovernanceManager::ProcessVote(const CGovernanceVote& vote, CGovernanceExc
return false;
}

if (cmapInvalidVotes.HasKey(nHashVote)) {
std::string msg{strprintf("CGovernanceManager::%s -- Old invalid vote, MN outpoint = %s, governance object hash = %s",
__func__, vote.GetMasternodeOutpoint().ToStringShort(), nHashGovobj.ToString())};
LogPrint(BCLog::GOBJECT, "%s\n", msg);
exception = CGovernanceException(msg, GOVERNANCE_EXCEPTION_PERMANENT_ERROR, 20);
return false;
}

auto it = mapObjects.find(nHashGovobj);
if (it == mapObjects.end()) {
if (!vote.IsValidForUnknownParent(tip_mn_list)) {
Expand Down Expand Up @@ -856,8 +847,6 @@ bool CGovernanceManager::ProcessVote(const CGovernanceVote& vote, CGovernanceExc
bool fOk = govobj.ProcessVote(m_mn_metaman, fRateChecksEnabled, tip_mn_list, vote, exception);
if (fOk) {
fOk = cmapVoteToObject.Insert(nHashVote, it->second);
} else if (exception.GetType() == GOVERNANCE_EXCEPTION_PERMANENT_ERROR && exception.GetNodePenalty() == 20) {
cmapInvalidVotes.Insert(nHashVote, vote);
}
Comment on lines 847 to 850

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Retain exact invalid-vote deduplication

When peers repeatedly advertise and serve the same invalid BLS or ECDSA vote for a known governance object, each wire message is deserialized into a fresh CGovernanceVote, so its per-instance signature memo starts empty. Because this branch now stores only successful votes, ProcessVote repeats the expensive signature verification while holding cs_store, and ConfirmInventoryRequest continues requesting the hash because cmapVoteToObject never contains it. The removed global cache bounded exact repeats to one verification; retain an invalid cache keyed by the semantic vote hash plus signature instead of eliminating it.

AGENTS.md reference: AGENTS.md:L162-L171

Useful? React with 👍 / 👎.

@PastaPastaPasta PastaPastaPasta Aug 10, 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.

as discussed in the PR description; this was the first thought, but was decided against. An attacker can just twiddle with bits inside the vote to make more and more invalid votes. they are limited to 5 before their connection is dropped.

Avoiding the verify operation is fine, but isn't much (5 BLS verifies or whatever). And introduces bugs such as what this is fixing.

return fOk;
}
Expand Down Expand Up @@ -1022,7 +1011,6 @@ void GovernanceStore::Clear()
LOCK(cs_store);
mapObjects.clear();
mapErasedGovernanceObjects.clear();
cmapInvalidVotes.Clear();
cmmapOrphanVotes.Clear();
mapLastMasternodeObject.clear();
lastMNListForVotingKeys = std::make_shared<CDeterministicMNList>();
Expand Down Expand Up @@ -1169,7 +1157,6 @@ void CGovernanceManager::RemoveInvalidVotes()
}
for (const auto& voteHash : removed) {
cmapVoteToObject.Erase(voteHash);
cmapInvalidVotes.Erase(voteHash);
cmmapOrphanVotes.Erase(voteHash);
}
}
Expand Down
9 changes: 6 additions & 3 deletions src/governance/governance.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,6 @@ class GovernanceStore
// key - governance object's hash
// value - expiration time for deleted objects
std::map<uint256, int64_t> mapErasedGovernanceObjects GUARDED_BY(cs_store);
CacheMap<uint256, CGovernanceVote> cmapInvalidVotes GUARDED_BY(cs_store);
vote_cmm_t cmmapOrphanVotes GUARDED_BY(cs_store);
txout_m_t mapLastMasternodeObject GUARDED_BY(cs_store);
// used to check for changed voting keys
Expand All @@ -204,9 +203,11 @@ 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
<< cmapInvalidVotes
<< empty_invalid_votes
<< cmmapOrphanVotes
<< mapObjects
<< mapLastMasternodeObject
Expand All @@ -225,8 +226,10 @@ 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
>> cmapInvalidVotes
>> discarded_invalid_votes
>> cmmapOrphanVotes
>> mapObjects
>> mapLastMasternodeObject
Expand Down
90 changes: 89 additions & 1 deletion src/test/governance_vote_processing_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include <netfulfilledman.h>
#include <primitives/transaction.h>
#include <script/standard.h>
#include <streams.h>
#include <timedata.h>
#include <uint256.h>
#include <util/strencodings.h>
Expand Down Expand Up @@ -202,6 +203,16 @@ struct GovernanceVoteSetup : public TestChainSetup {
return vote;
}
};

struct TestGovernanceStore : GovernanceStore {
using GovernanceStore::last_object_rec;

size_t ObjectCount() const
{
LOCK(cs_store);
return mapObjects.size();
}
};
} // namespace

BOOST_FIXTURE_TEST_SUITE(governance_vote_processing_tests, GovernanceVoteSetup)
Expand Down Expand Up @@ -280,7 +291,7 @@ BOOST_AUTO_TEST_CASE(unsigned_and_unknown_masternode_votes_are_rejected)
BOOST_CHECK_EQUAL(exception.GetNodePenalty(), 20);
BOOST_CHECK(!govman.HaveVoteForHash(forged.GetHash()));

// Re-sending a vote already known to be invalid is still punished, without re-verifying it.
// Re-sending the same invalid vote is still rejected and punished.
CGovernanceException repeat_exception;
BOOST_CHECK(!govman.ProcessVote(forged, repeat_exception, hash_to_request));
BOOST_CHECK_EQUAL(repeat_exception.GetType(), GOVERNANCE_EXCEPTION_PERMANENT_ERROR);
Expand Down Expand Up @@ -326,6 +337,47 @@ BOOST_AUTO_TEST_CASE(unsigned_and_unknown_masternode_votes_are_rejected)
BOOST_CHECK_EQUAL(stored->GetAbsoluteYesCount(tip_mn_list(), VOTE_SIGNAL_VALID), 0);
}

BOOST_AUTO_TEST_CASE(invalid_signature_does_not_suppress_valid_vote)
{
auto& govman = *m_node.govman;
const CGovernanceObject proposal{MakeProposal(uint256::ONE)};
const uint256 parent_hash{proposal.GetHash()};
govman.AddGovernanceObjectForTesting(proposal);
auto stored = govman.FindConstGovernanceObject(parent_hash);
BOOST_REQUIRE(stored != nullptr);

CKey attacker_key;
attacker_key.MakeNewKey(true);
CGovernanceVote forged{MakeVote(parent_hash, VOTE_SIGNAL_FUNDING, VOTE_OUTCOME_YES)};
SignWithVotingKey(forged, attacker_key);

CGovernanceException exception;
uint256 hash_to_request;
BOOST_CHECK(!govman.ProcessVote(forged, exception, hash_to_request));
BOOST_CHECK_EQUAL(exception.GetNodePenalty(), 20);

CGovernanceVote legitimate{forged};
SignWithVotingKey(legitimate, mn_voting_key);
BOOST_CHECK_EQUAL(forged.GetHash(), legitimate.GetHash());
BOOST_CHECK(govman.ProcessVote(legitimate, exception, hash_to_request));
BOOST_CHECK(govman.HaveVoteForHash(legitimate.GetHash()));
BOOST_CHECK_EQUAL(stored->GetAbsoluteYesCount(tip_mn_list(), VOTE_SIGNAL_FUNDING), 1);

CBLSSecretKey attacker_operator_key;
attacker_operator_key.MakeNewKey();
CGovernanceVote forged_bls{MakeVote(parent_hash, VOTE_SIGNAL_VALID, VOTE_OUTCOME_YES)};
SignWithOperatorKey(forged_bls, attacker_operator_key);
BOOST_CHECK(!govman.ProcessVote(forged_bls, exception, hash_to_request));
BOOST_CHECK_EQUAL(exception.GetNodePenalty(), 20);

CGovernanceVote legitimate_bls{forged_bls};
SignWithOperatorKey(legitimate_bls, mn_operator_key);
BOOST_CHECK_EQUAL(forged_bls.GetHash(), legitimate_bls.GetHash());
BOOST_CHECK(govman.ProcessVote(legitimate_bls, exception, hash_to_request));
BOOST_CHECK(govman.HaveVoteForHash(legitimate_bls.GetHash()));
BOOST_CHECK_EQUAL(stored->GetAbsoluteYesCount(tip_mn_list(), VOTE_SIGNAL_VALID), 1);
}

// Funding a proposal is the one decision reserved for the voting key; every other signal may be
// signed by the operator (BLS) key. Mixing the two up must not be accepted either way around.
//
Expand Down Expand Up @@ -381,4 +433,40 @@ 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)
{
const uint256 parent_hash{MakeProposal(uint256::ONE).GetHash()};
CKey attacker_key;
attacker_key.MakeNewKey(true);

CGovernanceVote forged{MakeVote(parent_hash, VOTE_SIGNAL_FUNDING, VOTE_OUTCOME_YES)};
SignWithVotingKey(forged, attacker_key);

CacheMap<uint256, CGovernanceVote> legacy_invalid_votes{3};
legacy_invalid_votes.Insert(forged.GetHash(), forged);
const auto proposal = std::make_shared<CGovernanceObject>(MakeProposal(uint256::ONE));

CDataStream stream{SER_DISK, CLIENT_VERSION};
stream << 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}}
<< std::map<COutPoint, TestGovernanceStore::last_object_rec>{}
<< CDeterministicMNList{};

TestGovernanceStore store;
store.Unserialize(stream);
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);
}

BOOST_AUTO_TEST_SUITE_END()
Loading