From 1a510252b76102654239192edc89ee15f4e1cda9 Mon Sep 17 00:00:00 2001 From: pasta Date: Sun, 2 Aug 2026 18:19:17 -0500 Subject: [PATCH 1/2] fix: require a valid masternode signature before caching an orphan vote CGovernanceManager::ProcessVote inserted a vote whose parent governance object is unknown into cmmapOrphanVotes before performing any masternode-membership or signature check, and raised a zero-penalty exception for it. The only gate was the announce-then-request tracker, so a peer could place arbitrary unvalidated data in the cache for free and additionally provoke an MNGOVERNANCESYNC request for an invented parent hash. Reject such a vote unless CGovernanceVote::IsValid accepts it under either the voting key or the operator BLS key; which one applies depends on the parent object's type and the vote signal, and the parent is by definition unknown here. Rejections carry GOVERNANCE_EXCEPTION_PERMANENT_ERROR with a penalty of 20, matching what CGovernanceObject::ProcessVote already applies for an unknown masternode, so the same bad vote costs the sender the same whether or not its parent has arrived. The orphan branch itself stays at penalty 0: past the gate, the vote is masternode-signed and merely early, which is a routine relay race during governance sync. Gate rejections are deliberately not added to cmapInvalidVotes, which would open a new unauthenticated path into that cache. GetListAtChainTip() is hoisted so both paths share one call. --- src/governance/governance.cpp | 15 +++- src/test/governance_inv_tests.cpp | 115 +++++++++++++++++++++++++----- 2 files changed, 111 insertions(+), 19 deletions(-) diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index ea04abc07956..a8a0ca2d2547 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -804,6 +804,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(); @@ -824,8 +826,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)) { + 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() + GOVERNANCE_ORPHAN_EXPIRATION_TIME})) { hashToRequest = nHashGovobj; // Caller should request this object @@ -842,7 +855,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) { diff --git a/src/test/governance_inv_tests.cpp b/src/test/governance_inv_tests.cpp index 93e63e9ac33f..471b9cd3dcf3 100644 --- a/src/test/governance_inv_tests.cpp +++ b/src/test/governance_inv_tests.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -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(*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()}; @@ -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); @@ -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(*m_node.connman); const CGovernanceVote vote{MakeGovernanceVote(uint256S("41"))}; const CInv vote_inv{MSG_GOVERNANCE_OBJECT_VOTE, vote.GetHash()}; @@ -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(&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(*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); 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(&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().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() From 1e37fc4c369ff543c4a41af054965e1607d1b188 Mon Sep 17 00:00:00 2001 From: pasta Date: Fri, 7 Aug 2026 14:57:11 -0500 Subject: [PATCH 2/2] fix: only accept the voting key for funding votes in the orphan-vote gate The orphan gate accepted either key for every signal, but onlyVotingKeyAllowed in CGovernanceObject::ProcessVote only ever permits the voting key for PROPOSAL + VOTE_SIGNAL_FUNDING; VALID, DELETE and ENDORSED require the operator key regardless of the parent's type. Accepting the voting key for those signals let the lower-trust credential (routinely delegated to third-party voting services) cache votes that could never validate once their parent arrived, and scored them 0 where the known-object path scores 20. Name the rule CGovernanceVote::IsValidForUnknownParent next to IsValid so the key-selection knowledge stays in one place, and skip the pointless voting-key attempt for non-funding votes. Also update a fixture comment left stale by the parity test added earlier in this branch. --- src/governance/governance.cpp | 4 +--- src/governance/vote.cpp | 8 ++++++++ src/governance/vote.h | 9 +++++++++ src/test/governance_inv_tests.cpp | 6 +++--- 4 files changed, 21 insertions(+), 6 deletions(-) diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index a8a0ca2d2547..7bbcf10be24f 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -826,9 +826,7 @@ 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)) { + if (!vote.IsValidForUnknownParent(tip_mn_list)) { 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); diff --git a/src/governance/vote.cpp b/src/governance/vote.cpp index afef2543fbae..6b898b753f46 100644 --- a/src/governance/vote.cpp +++ b/src/governance/vote.cpp @@ -241,6 +241,14 @@ bool CGovernanceVote::IsValid(const CDeterministicMNList& tip_mn_list, bool useV } } +bool CGovernanceVote::IsValidForUnknownParent(const CDeterministicMNList& tip_mn_list) const +{ + if (nVoteSignal == VOTE_SIGNAL_FUNDING && IsValid(tip_mn_list, /*useVotingKey=*/true)) { + return true; + } + return IsValid(tip_mn_list, /*useVotingKey=*/false); +} + bool operator==(const CGovernanceVote& vote1, const CGovernanceVote& vote2) { diff --git a/src/governance/vote.h b/src/governance/vote.h index 4c20c917c038..36df8a199fed 100644 --- a/src/governance/vote.h +++ b/src/governance/vote.h @@ -149,6 +149,15 @@ class CGovernanceVote bool CheckSignature(const CKeyID& keyID) const; bool CheckSignature(const CBLSPublicKey& pubKey) const; bool IsValid(const CDeterministicMNList& tip_mn_list, bool useVotingKey) const; + /** + * Validation for a vote whose parent object is unknown, so the exact key + * requirement cannot be derived yet. Only a funding vote can ever be + * voting-key-signed (PROPOSAL + VOTE_SIGNAL_FUNDING -- see onlyVotingKeyAllowed + * in CGovernanceObject::ProcessVote); every other signal requires the operator + * key for every object type, and a funding vote on a non-proposal will be + * re-checked against the operator-key requirement when its parent arrives. + */ + bool IsValidForUnknownParent(const CDeterministicMNList& tip_mn_list) const; /** The memoised verdict for this key, or nullopt if the next CheckSignature * with it would have to run the cryptography. */ diff --git a/src/test/governance_inv_tests.cpp b/src/test/governance_inv_tests.cpp index 471b9cd3dcf3..880670bf862d 100644 --- a/src/test/governance_inv_tests.cpp +++ b/src/test/governance_inv_tests.cpp @@ -44,9 +44,9 @@ struct GovernanceInvSetup : public TestingSetup { BOOST_REQUIRE(m_node.mn_sync->IsBlockchainSynced()); BOOST_REQUIRE(m_node.mn_metaman); - // Note: mn_metaman is left unloaded. No test here reaches - // CGovernanceObject::ProcessVote, which asserts metaman.IsValid() -- a vote whose - // parent object exists would, and would need it loaded first. + // Note: mn_metaman is left unloaded. Reaching CGovernanceObject::ProcessVote asserts + // metaman.IsValid(), so a test that delivers a vote whose parent object exists must + // load it first (see invalid_vote_is_scored_alike_with_and_without_a_parent_object). m_node.govman = std::make_unique(*m_node.mn_metaman, *m_node.chainman, *m_node.chain_helper->superblocks, *m_node.dmnman, *m_node.mn_sync); // Match runtime preconditions: NetGovernance::AlreadyHave claims we // already have the inv when governance isn't loaded (e.g.