diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index ea04abc07956..7bbcf10be24f 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,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() + 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) { 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 93e63e9ac33f..880670bf862d 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 @@ -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(*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(*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()