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

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,17 @@ bool CGovernanceManager::ProcessVote(const CGovernanceVote& vote, CGovernanceExc
 
     auto it = mapObjects.find(nHashGovobj);
     if (it == mapObjects.end()) {
+        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);
+            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
@@ -842,7 +853,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) {

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)
 {

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. */

src/test/governance_inv_tests.cpp

@@ -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>
@@ -43,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<CGovernanceManager>(*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.
@@ -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()};
@@ -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<ConnmanTestMsg&>(*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<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);
 
     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()