Skip to content

fix: require a valid masternode signature before caching an orphan governance vote #7527

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

Open
PastaPastaPasta wants to merge 1 commit into
base: develop
Choose a base branch
from
+111 −19
Open

fix: require a valid masternode signature before caching an orphan governance vote #7527

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
15 changes: 14 additions & 1 deletion src/governance/governance.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -801,6 +801,8 @@ bool CGovernanceManager::ProcessVote(const CGovernanceVote& vote, CGovernanceExc
AssertLockNotHeld(cs_store);
hashToRequest = uint256{};

const auto tip_mn_list{m_dmnman.GetListAtChainTip()};

LOCK(cs_store);
uint256 nHashVote = vote.GetHash();
uint256 nHashGovobj = vote.GetParentHash();
Expand All @@ -821,8 +823,19 @@ bool CGovernanceManager::ProcessVote(const CGovernanceVote& vote, CGovernanceExc

auto it = mapObjects.find(nHashGovobj);
if (it == mapObjects.end()) {
// The parent object is unknown, so the vote signal cannot be mapped to a key type the way
// CGovernanceObject::ProcessVote does it (see onlyVotingKeyAllowed there). Accept either key.
if (!vote.IsValid(tip_mn_list, /*useVotingKey=*/true) && !vote.IsValid(tip_mn_list, /*useVotingKey=*/false)) {
Comment on lines +826 to +828

@thepastaclaw thepastaclaw Aug 3, 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.

🟡 Suggestion: Orphan-vote gate accepts the voting key for signals that always require the operator key

CGovernanceObject::ProcessVote (object.cpp:455) only allows the voting-key signature when type == PROPOSAL && signal == VOTE_SIGNAL_FUNDING; for VALID, DELETE, and ENDORSED it requires the operator key regardless of the object's type. The new orphan gate here tries both keys unconditionally for every signal, so a vote signed only with the voting key (a credential that's meant to carry much lower trust — it's routinely delegated to third parties) can pass the gate, get cached, and trigger a real MNGOVERNANCESYNC request for a non-funding signal, even though that exact vote is guaranteed to fail once the parent object actually arrives and CGovernanceObject::ProcessVote runs the correct key check. The commit message frames this as unavoidable because "which [key] applies depends on the parent object's type and the vote signal," but that's only true for FUNDING — for every other signal the key is always the operator key irrespective of type, so the ambiguity (and the two-key fallback) should be restricted to the FUNDING signal.

Suggested change
// The parent object is unknown, so the vote signal cannot be mapped to a key type the way
// CGovernanceObject::ProcessVote does it (see onlyVotingKeyAllowed there). Accept either key.
if (!vote.IsValid(tip_mn_list, /*useVotingKey=*/true) && !vote.IsValid(tip_mn_list, /*useVotingKey=*/false)) {
// Only a FUNDING signal is ambiguous while the parent type is unknown (a proposal uses
// the voting key for FUNDING, everything else uses the operator key). VOTE_SIGNAL_NONE is
// never processable.
const bool valid_operator_signature{
vote.GetSignal() != VOTE_SIGNAL_NONE && vote.IsValid(tip_mn_list, /*useVotingKey=*/false)};
const bool valid_voting_signature{
vote.GetSignal() == VOTE_SIGNAL_FUNDING && vote.IsValid(tip_mn_list, /*useVotingKey=*/true)};
if (!valid_operator_signature && !valid_voting_signature) {

source: ['codex']

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 3, 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 Require the operator key for non-funding orphan votes

For orphan votes using VALID, DELETE, or ENDORSED, the parent type is irrelevant: CGovernanceObject::ProcessVote selects the voting key only for PROPOSAL plus FUNDING, so these signals must always use the operator BLS key. Trying the voting key first here allows an ECDSA-signed non-funding vote to enter cmmapOrphanVotes and trigger a parent request even though it can never pass once its parent arrives, at which point it is rejected without any peer penalty. Only FUNDING needs the either-key fallback; require the operator key for every other signal.

AGENTS.md reference: AGENTS.md:L159-L166

Useful? React with 👍 / 👎.

@chatgpt-codex-connector chatgpt-codex-connector Bot Aug 3, 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 Gate BLS signature failures behind governance logging

When a peer announces an orphan vote containing a real public masternode outpoint but a garbage signature, the voting-key check fails and the second call reaches CGovernanceVote::CheckSignature(const CBLSPublicKey&), whose failure is still logged with unconditional LogPrintf at vote.cpp:153. Arbitrary parent hashes bypass the known-object rate checks, and peer penalties are suppressed until full sync, so an unauthenticated peer can emit one debug.log line per message throughout that sync window; change that failure to category-gated logging or avoid invoking the incompatible verifier based on the signature encoding.

AGENTS.md reference: AGENTS.md:L159-L166

Useful? React with 👍 / 👎.

std::string msg{strprintf("CGovernanceManager::%s -- Invalid vote for unknown parent object %s, MN outpoint = %s, vote hash = %s",
__func__, nHashGovobj.ToString(), vote.GetMasternodeOutpoint().ToStringShort(), nHashVote.ToString())};
LogPrint(BCLog::GOBJECT, "%s\n", msg);
exception = CGovernanceException(msg, GOVERNANCE_EXCEPTION_PERMANENT_ERROR, 20);
return false;
}
std::string msg{strprintf("CGovernanceManager::%s -- Unknown parent object %s, MN outpoint = %s", __func__,
nHashGovobj.ToString(), vote.GetMasternodeOutpoint().ToStringShort())};
// No penalty: the vote is signed by a masternode, it just arrived before its parent object,
// which routinely happens during governance sync. Misbehaviour scores never decay.
exception = CGovernanceException(msg, GOVERNANCE_EXCEPTION_WARNING);
if (cmmapOrphanVotes.Insert(nHashGovobj, governance::OrphanVote{vote, Now<NodeSeconds>() + GOVERNANCE_ORPHAN_EXPIRATION_TIME})) {
hashToRequest = nHashGovobj; // Caller should request this object
Expand All @@ -839,7 +852,7 @@ bool CGovernanceManager::ProcessVote(const CGovernanceVote& vote, CGovernanceExc
return false;
}

bool fOk = govobj.ProcessVote(m_mn_metaman, fRateChecksEnabled, m_dmnman.GetListAtChainTip(), vote, exception);
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) {
Expand Down
115 changes: 97 additions & 18 deletions src/test/governance_inv_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include <governance/governance.h>
#include <governance/net_governance.h>
#include <governance/object.h>
#include <masternode/meta.h>
#include <masternode/sync.h>
#include <net.h>
#include <net_processing.h>
Expand Down Expand Up @@ -394,7 +395,12 @@ BOOST_AUTO_TEST_CASE(governance_votes_require_peer_announcement_or_request)
m_node.peerman->InitializeNode(*second_announcing_peer, NODE_NETWORK);
m_node.peerman->InitializeNode(*unsolicited_peer, NODE_NETWORK);

auto& connman = static_cast<ConnmanTestMsg&>(*m_node.connman);
// The vote below carries an unknown masternode outpoint, so CGovernanceManager::ProcessVote
// rejects it with a penalty of 20. Only a peer that passes the announce-then-request gate
// reaches ProcessVote at all, which makes the score the observable for the gate itself.
// Penalties are applied only once fully synced.
m_node.mn_sync->SwitchToNextAsset();
BOOST_REQUIRE(m_node.mn_sync->IsSynced());

const CGovernanceVote vote{MakeGovernanceVote(uint256S("31"))};
const CInv vote_inv{MSG_GOVERNANCE_OBJECT_VOTE, vote.GetHash()};
Expand All @@ -406,20 +412,17 @@ BOOST_AUTO_TEST_CASE(governance_votes_require_peer_announcement_or_request)
ProcessInv(*m_node.peerman, *announcing_peer, vote_inv);
ProcessInv(*m_node.peerman, *second_announcing_peer, vote_inv);

connman.FlushSendBuffer(*announcing_peer);
ProcessGovernanceVote(net_gov, *announcing_peer, vote);
BOOST_CHECK_EQUAL(CountQueuedMessages(*announcing_peer, NetMsgType::MNGOVERNANCESYNC), 1U);
AssertMisbehaviorScore(*m_node.peerman, *announcing_peer, 0);
BOOST_CHECK(
!WITH_LOCK(::cs_main, return m_node.peerman->PeerConsumeObjectRequest(announcing_peer->GetId(), vote_inv)));
AssertMisbehaviorScore(*m_node.peerman, *announcing_peer, 20);

connman.FlushSendBuffer(*second_announcing_peer);
ProcessGovernanceVote(net_gov, *second_announcing_peer, vote);
// Second announcer: the gate accepts (it independently announced the vote) and consumes
// its per-peer request entry. A rejected vote would return before the gate consumes and
// leave the entry intact, so this proves the accept path independently of the (deduped)
// orphan-request side effect.
// Consumption is per-peer: the second announcer's own entry is accepted and consumed,
// independent of the first peer's already-consumed entry.
BOOST_CHECK(!WITH_LOCK(::cs_main,
return m_node.peerman->PeerConsumeObjectRequest(second_announcing_peer->GetId(), vote_inv)));
AssertMisbehaviorScore(*m_node.peerman, *second_announcing_peer, 0);
AssertMisbehaviorScore(*m_node.peerman, *second_announcing_peer, 20);

m_node.peerman->FinalizeNode(*announcing_peer);
m_node.peerman->FinalizeNode(*second_announcing_peer);
Expand All @@ -442,7 +445,6 @@ BOOST_AUTO_TEST_CASE(governance_vote_authorization_survives_unsynced_drop)

auto peer{MakeGovernanceInvPeer(/*id=*/31)};
m_node.peerman->InitializeNode(*peer, NODE_NETWORK);
auto& connman = static_cast<ConnmanTestMsg&>(*m_node.connman);

const CGovernanceVote vote{MakeGovernanceVote(uint256S("41"))};
const CInv vote_inv{MSG_GOVERNANCE_OBJECT_VOTE, vote.GetHash()};
Expand All @@ -454,21 +456,98 @@ BOOST_AUTO_TEST_CASE(governance_vote_authorization_survives_unsynced_drop)
// Not synced: delivering the vote is dropped at the sync gate and must NOT consume the request.
m_node.mn_sync->Reset(/*fForce=*/true, /*fNotifyReset=*/false);
BOOST_REQUIRE(!m_node.mn_sync->IsBlockchainSynced());
connman.FlushSendBuffer(*peer);
ProcessGovernanceVote(net_gov, *peer, vote);
BOOST_CHECK_EQUAL(CountQueuedMessages(*peer, NetMsgType::MNGOVERNANCESYNC), 0U);
AssertMisbehaviorScore(*m_node.peerman, *peer, 0);

// Back in sync, the retransmit is still authorized and reaches ProcessVote, which rejects the
// unknown masternode outpoint with a penalty of 20. Had the unsynced drop consumed the request,
// the gate would now reject the vote as unrequested and return before ProcessVote, leaving the
// score at 0.
while (!m_node.mn_sync->IsSynced()) {
m_node.mn_sync->SwitchToNextAsset();
}
ProcessGovernanceVote(net_gov, *peer, vote);
AssertMisbehaviorScore(*m_node.peerman, *peer, 20);

m_node.peerman->FinalizeNode(*peer);
chainstate.ResetIbd();
}

// A vote whose parent object is unknown must prove masternode authorship before it is cached.
BOOST_AUTO_TEST_CASE(orphan_votes_require_a_valid_masternode_signature)
{
LOCK(NetEventsInterface::g_msgproc_mutex);

// Back in sync, the retransmit is still authorized: ProcessVote runs and (orphan parent)
// requests the missing object. Had the unsynced drop consumed the request, the gate would now
// reject the vote as unrequested and send no MNGOVERNANCESYNC.
TestChainState& chainstate =
*static_cast<TestChainState*>(&m_node.chainman->ActiveChainstate());
chainstate.JumpOutOfIbd();

// Penalties are applied only once fully synced.
m_node.mn_sync->SwitchToNextAsset();
BOOST_REQUIRE(m_node.mn_sync->IsBlockchainSynced());
BOOST_REQUIRE(m_node.mn_sync->IsSynced());

NetGovernance net_gov(m_node.peerman.get(), *m_node.govman, *m_node.mn_sync,
*m_node.netfulfilledman, *m_node.connman);

auto peer{MakeGovernanceInvPeer(/*id=*/41)};
m_node.peerman->InitializeNode(*peer, NODE_NETWORK);
auto& connman = static_cast<ConnmanTestMsg&>(*m_node.connman);

// The tip masternode list is empty in this setup, so no vote can name a known collateral.
const CGovernanceVote vote{MakeGovernanceVote(uint256S("51"))};
const CInv vote_inv{MSG_GOVERNANCE_OBJECT_VOTE, vote.GetHash()};

ProcessInv(*m_node.peerman, *peer, vote_inv);
connman.FlushSendBuffer(*peer);
ProcessGovernanceVote(net_gov, *peer, vote);
BOOST_CHECK_EQUAL(CountQueuedMessages(*peer, NetMsgType::MNGOVERNANCESYNC), 1U);

BOOST_CHECK(m_node.govman->GetOrphanVoteObjectHashes().empty());
BOOST_CHECK_EQUAL(CountQueuedMessages(*peer, NetMsgType::MNGOVERNANCESYNC), 0U);
AssertMisbehaviorScore(*m_node.peerman, *peer, 20);
Comment on lines +496 to +506

@thepastaclaw thepastaclaw Aug 3, 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.

🟡 Suggestion: New orphan-gate signature check has no test exercising an actual signature

orphan_votes_require_a_valid_masternode_signature runs against an empty deterministic masternode list, so CGovernanceVote::IsValid returns false at the GetMNByCollateral lookup (vote.cpp:180) and never reaches CheckSignature for either key. This is the only new test added for the security-relevant change in this PR (gating orphan-vote caching on a real signature), and as written it can't distinguish 'rejected because unknown masternode' from 'rejected because bad signature' — nor would it catch a correctly-signed early vote being wrongly rejected, or a forged signature for a known masternode being wrongly accepted. Given this is consensus-adjacent governance code, a test that registers a real masternode and checks both a forged-signature rejection and a correctly-signed acceptance (ideally for both a funding and non-funding signal) would meaningfully strengthen coverage of the new logic.

source: ['codex']


m_node.peerman->FinalizeNode(*peer);
chainstate.ResetIbd();
}

// The same unauthenticated vote must cost the sender the same whether or not its parent object
// happens to have arrived first.
BOOST_AUTO_TEST_CASE(invalid_vote_is_scored_alike_with_and_without_a_parent_object)
{
LOCK(NetEventsInterface::g_msgproc_mutex);

TestChainState& chainstate =
*static_cast<TestChainState*>(&m_node.chainman->ActiveChainstate());
chainstate.JumpOutOfIbd();

m_node.mn_sync->SwitchToNextAsset();
BOOST_REQUIRE(m_node.mn_sync->IsSynced());
// The known-object path runs CGovernanceObject::ProcessVote, which asserts metaman.IsValid().
BOOST_REQUIRE(m_node.mn_metaman->LoadCache(/*load_cache=*/false));

NetGovernance net_gov(m_node.peerman.get(), *m_node.govman, *m_node.mn_sync,
*m_node.netfulfilledman, *m_node.connman);

auto orphan_peer{MakeGovernanceInvPeer(/*id=*/51)};
auto known_parent_peer{MakeGovernanceInvPeer(/*id=*/52)};
m_node.peerman->InitializeNode(*orphan_peer, NODE_NETWORK);
m_node.peerman->InitializeNode(*known_parent_peer, NODE_NETWORK);

const CGovernanceObject govobj{MakeGovernanceObject(GetTime<std::chrono::seconds>().count(), uint256S("61"))};
const CGovernanceVote orphan_vote{MakeGovernanceVote(uint256S("62"))};
const CGovernanceVote known_parent_vote{MakeGovernanceVote(govobj.GetHash())};

ProcessInv(*m_node.peerman, *orphan_peer, CInv{MSG_GOVERNANCE_OBJECT_VOTE, orphan_vote.GetHash()});
ProcessGovernanceVote(net_gov, *orphan_peer, orphan_vote);
AssertMisbehaviorScore(*m_node.peerman, *orphan_peer, 20);

m_node.govman->AddGovernanceObjectForTesting(govobj);
ProcessInv(*m_node.peerman, *known_parent_peer, CInv{MSG_GOVERNANCE_OBJECT_VOTE, known_parent_vote.GetHash()});
ProcessGovernanceVote(net_gov, *known_parent_peer, known_parent_vote);
AssertMisbehaviorScore(*m_node.peerman, *known_parent_peer, 20);

m_node.peerman->FinalizeNode(*orphan_peer);
m_node.peerman->FinalizeNode(*known_parent_peer);
chainstate.ResetIbd();
}

BOOST_AUTO_TEST_SUITE_END()
Loading