From 8d6124606b3419d5e87cc0c07f7fe0817f78b6f7 Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 14 Oct 2026 22:22:41 -0500 Subject: [PATCH 1/3] fix: fetch orphan-vote parents through the object request tracker A governance vote whose parent object we do not have is kept as an orphan, and we try to fetch the parent. That fetch had two parts: - the peer that sent the vote was sent MNGOVERNANCESYNC(parent, empty filter), and - every 5 minutes NetGovernance::Schedule() sent the same message for every orphan parent hash to every connected peer. The sweep is O(orphan parents x peers) messages per tick, without a cap. PushMessage queues them whether or not fPauseSend is set. Every orphan parent comes from a peer-supplied vote: the parent hash is covered by the masternode signature, but nothing ties it to an object that exists. So one masternode key can invent parents without limit. At 30k orphan parents and 50 peers, one tick is about 1.5M messages (~180 MB of vSendMsg and ~100 MB of egress), repeated every 5 minutes for as long as the orphans live. The message only induces an announcement. A peer serves a non-zero MNGOVERNANCESYNC with an empty filter by replying INV(MSG_GOVERNANCE_OBJECT, parent), which goes through the normal object request tracker. So skip the round trip and register the vote's sender with the tracker directly, as if it had announced the parent. This is how orphan transactions already fetch their parents. It uses the same AlreadyHave() check, MAX_PEER_OBJECT_ANNOUNCEMENTS cap, overload delay and preferred/non-preferred scheduling as a received INV. The new PeerAddObjectAnnouncement() exposes it to NetGovernance. Because of this, an orphan vote gives a peer no more request capacity than it already has by announcing hashes itself. The sweep and GetOrphanVoteObjectHashes() are removed. Orphan expiry used to live in that function. It moves to CheckAndRemove(), which still runs on the same 5-minute tick, so orphans whose parent never arrives are still dropped. The sweep was also the fallback when the first peer asked did not deliver the parent. Instead, ProcessVote() now returns the parent hash for every relay of an orphan vote, including a vote we already hold. Each peer that relays it becomes a fetch candidate, and the tracker moves on to the next one when a request times out or a peer disconnects. Nodes on this version still serve the object-fetch form of MNGOVERNANCESYNC to older peers. Only the sending side changes. This also drops MAX_ORPHAN_OBJECT_REQUESTS, the per-tick cap on the sweep that came in with the v23.1.9 back-merge, together with its unit test and test-only AddOrphanVoteForTesting helper. With no sweep left there is nothing for it to cap. Co-Authored-By: Claude Opus 5.5 --- src/governance/governance.cpp | 61 +++++------------- src/governance/governance.h | 9 --- src/governance/net_governance.cpp | 30 ++------- src/net_processing.cpp | 7 ++ src/net_processing.h | 5 ++ src/test/governance_inv_tests.cpp | 38 +---------- src/test/governance_vote_processing_tests.cpp | 64 ++++++++++++++++++- 7 files changed, 98 insertions(+), 116 deletions(-) diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index fb4f26ddf1e9..6ca47f099d4c 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -389,14 +389,6 @@ void CGovernanceManager::AddGovernanceObjectForTesting(const CGovernanceObject& mapObjects.emplace(govobj.GetHash(), std::make_shared(govobj)); } -void CGovernanceManager::AddOrphanVoteForTesting(const CGovernanceVote& vote) -{ - AssertLockNotHeld(cs_store); - LOCK(cs_store); - cmmapOrphanVotes.Insert(vote.GetParentHash(), - governance::OrphanVote{vote, Now() + GOVERNANCE_ORPHAN_EXPIRATION_TIME}); -} - void CGovernanceManager::CheckAndRemove() { AssertLockNotHeld(cs_store); @@ -414,6 +406,17 @@ void CGovernanceManager::CheckAndRemove() ScopedLockBool guard(cs_store, fRateChecksEnabled, false); + // Clean up expired orphan votes + const auto now{Now()}; + const vote_cmm_t::list_t& items = cmmapOrphanVotes.GetItemList(); + for (auto it = items.begin(); it != items.end();) { + auto prevIt = it; + ++it; + if (prevIt->value.expiration < now) { + cmmapOrphanVotes.Erase(prevIt->key, prevIt->value); + } + } + // Clean up any expired or invalid triggers m_superblocks.Clean(nCachedBlockHeight); @@ -845,9 +848,10 @@ bool CGovernanceManager::ProcessVote(const CGovernanceVote& vote, CGovernanceExc // 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 - } + cmmapOrphanVotes.Insert(nHashGovobj, + governance::OrphanVote{vote, Now() + GOVERNANCE_ORPHAN_EXPIRATION_TIME}); + // Even if we already held this vote: every peer that relays it can serve the parent. + hashToRequest = nHashGovobj; LogPrint(BCLog::GOBJECT, "%s\n", msg); return false; } @@ -1132,41 +1136,6 @@ void CGovernanceManager::UpdatedBlockTip(const CBlockIndex* pindex) m_superblocks.ExecuteBestSuperblock(m_dmnman.GetListAtChainTip(), pindex->nHeight); } -std::vector CGovernanceManager::GetOrphanVoteObjectHashes() -{ - LOCK(cs_store); - - const auto now{Now()}; - - // Clean up expired orphan votes - const vote_cmm_t::list_t& items = cmmapOrphanVotes.GetItemList(); - for (auto it = items.begin(); it != items.end();) { - auto prevIt = it; - ++it; - if (prevIt->value.expiration < now) { - cmmapOrphanVotes.Erase(prevIt->key, prevIt->value); - } - } - - // Get hashes of objects we don't have yet - std::vector vecHashesFiltered; - std::vector vecHashes; - cmmapOrphanVotes.GetKeys(vecHashes); - for (const uint256& nHash : vecHashes) { - if (mapObjects.find(nHash) == mapObjects.end()) { - vecHashesFiltered.push_back(nHash); - } - } - - // A random subset, so that parent hashes ground to sort first cannot pin which ones are requested - if (vecHashesFiltered.size() > MAX_ORPHAN_OBJECT_REQUESTS) { - Shuffle(vecHashesFiltered.begin(), vecHashesFiltered.end(), FastRandomContext()); - vecHashesFiltered.resize(MAX_ORPHAN_OBJECT_REQUESTS); - } - - return vecHashesFiltered; -} - void CGovernanceManager::RemoveInvalidVotes() { AssertLockHeld(cs_store); diff --git a/src/governance/governance.h b/src/governance/governance.h index 631b2d939086..17bdf53fb87b 100644 --- a/src/governance/governance.h +++ b/src/governance/governance.h @@ -329,8 +329,6 @@ class CGovernanceManager : public GovernanceStore * running collateral or chain validation. */ void AddGovernanceObjectForTesting(const CGovernanceObject& govobj) EXCLUSIVE_LOCKS_REQUIRED(!cs_store); - /** Test-only helper: caches a vote as an orphan of its parent without validating it. */ - void AddOrphanVoteForTesting(const CGovernanceVote& vote) EXCLUSIVE_LOCKS_REQUIRED(!cs_store); // Thread-safe accessors bool HaveObjectForHash(const uint256& nHash) const @@ -357,13 +355,6 @@ class CGovernanceManager : public GovernanceStore // Used by NetGovernance std::vector FetchRelayInventory() EXCLUSIVE_LOCKS_REQUIRED(!cs_relay); void CheckAndRemove() EXCLUSIVE_LOCKS_REQUIRED(!cs_store); - /** Upper bound on the orphan parent hashes GetOrphanVoteObjectHashes() returns per call. Each one is - * re-requested from every peer, and one masternode key can sign votes for any number of invented - * parent hashes. */ - static constexpr size_t MAX_ORPHAN_OBJECT_REQUESTS{100}; - /** Get hashes of governance objects for which we have orphan votes, at most - * MAX_ORPHAN_OBJECT_REQUESTS of them chosen at random. Also cleans up expired orphans. */ - [[nodiscard]] std::vector GetOrphanVoteObjectHashes() EXCLUSIVE_LOCKS_REQUIRED(!cs_store); std::pair, std::vector> FetchGovernanceObjectVotes( size_t peers_per_hash_max, int64_t now, std::map>& map_asked_recently) const EXCLUSIVE_LOCKS_REQUIRED(!cs_store); diff --git a/src/governance/net_governance.cpp b/src/governance/net_governance.cpp index 56d193d9ad62..2929c514813d 100644 --- a/src/governance/net_governance.cpp +++ b/src/governance/net_governance.cpp @@ -45,22 +45,6 @@ void NetGovernance::Schedule(CScheduler& scheduler) [this]() -> void { if (!m_node_sync.IsSynced()) return; - // Request governance objects for orphan votes - auto vecOrphanHashes = m_gov_manager.GetOrphanVoteObjectHashes(); - if (!vecOrphanHashes.empty()) { - LogPrint(BCLog::GOBJECT, "NetGovernance::Schedule -- requesting %d orphan objects\n", - vecOrphanHashes.size()); - const CConnman::NodesSnapshot snap{m_connman, CConnman::FullyConnectedOnly}; - for (const uint256& nHash : vecOrphanHashes) { - for (CNode* pnode : snap.Nodes()) { - if (!pnode->CanRelay()) continue; - CNetMsgMaker msgMaker(pnode->GetCommonVersion()); - CBloomFilter filter; // Empty filter - we want the object, not votes - m_connman.PushMessage(pnode, msgMaker.Make(NetMsgType::MNGOVERNANCESYNC, nHash, filter)); - } - } - } - // CHECK AND REMOVE - REPROCESS GOVERNANCE OBJECTS m_gov_manager.CheckAndRemove(); }, @@ -113,9 +97,9 @@ void NetGovernance::ProcessMessage(CNode& peer, const std::string& msg_type, CDa LogPrint(BCLog::GOBJECT, "MNGOVERNANCESYNC -- syncing governance objects to our peer %s\n", peer.GetLogString()); const bool full_sync{nProp == uint256()}; const bool object_fetch{!full_sync && IsEmptyBloomFilter(filter)}; - // Nonzero govsync with an empty filter is used to retry missing-object - // fetches for orphan votes. Only full sync and actual known-object vote - // sync are fulfilled-request limited. + // Nonzero govsync with an empty filter is how older nodes fetch the + // parent object of an orphan vote. Only full sync and actual + // known-object vote sync are fulfilled-request limited. const bool track_request{full_sync || (!object_fetch && m_gov_manager.HaveSyncableObjectForHash(nProp))}; const std::string fulfilled_request{full_sync ? NetMsgType::MNGOVERNANCESYNC : @@ -258,10 +242,10 @@ void NetGovernance::ProcessMessage(CNode& peer, const std::string& msg_type, CDa } else { LogPrint(BCLog::GOBJECT, "MNGOVERNANCEOBJECTVOTE -- Rejected vote, error = %s\n", exception.what()); if (hashToRequest != uint256()) { - // Orphan vote - request the missing governance object - CNetMsgMaker msgMaker(peer.GetCommonVersion()); - CBloomFilter filter; // Empty filter - we just want the object, not votes - m_connman.PushMessage(&peer, msgMaker.Make(NetMsgType::MNGOVERNANCESYNC, hashToRequest, filter)); + // A node only relays a vote once it has the vote's parent object, so fetch the + // parent from this peer as if it had announced it. + WITH_LOCK(::cs_main, + m_peer_manager->PeerAddObjectAnnouncement(peer, CInv{MSG_GOVERNANCE_OBJECT, hashToRequest})); } if ((exception.GetNodePenalty() != 0) && m_node_sync.IsSynced()) { m_peer_manager->PeerMisbehaving(peer.GetId(), exception.GetNodePenalty()); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 2a956dd97c39..57f7ed8428ce 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -661,6 +661,8 @@ class PeerManagerImpl final : public PeerManager GetDataResponse PeerConsumeGetDataResponse(NodeId nodeid, const CInv& inv) override EXCLUSIVE_LOCKS_REQUIRED(!m_object_request_mutex, ::cs_main); void PeerForgetObjectRequest(const CInv& inv) override EXCLUSIVE_LOCKS_REQUIRED(!m_object_request_mutex, ::cs_main); + void PeerAddObjectAnnouncement(const CNode& node, const CInv& inv) override + EXCLUSIVE_LOCKS_REQUIRED(!m_object_request_mutex, !m_recent_confirmed_transactions_mutex, ::cs_main); void PeerPushInventory(NodeId nodeid, const CInv& inv) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); void PeerRelayInv(const CInv& inv) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); void PeerRelayInvFiltered(const CInv& inv, const CTransaction& relatedTx) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); @@ -7029,6 +7031,11 @@ void PeerManagerImpl::PeerForgetObjectRequest(const CInv& inv) WITH_LOCK(m_object_request_mutex, m_object_request.ForgetTxHash(inv)); } +void PeerManagerImpl::PeerAddObjectAnnouncement(const CNode& node, const CInv& inv) +{ + if (!AlreadyHave(inv)) AddObjectAnnouncement(node, inv, GetTime()); +} + void PeerManagerImpl::PeerPushInventory(NodeId nodeid, const CInv& inv) { PushInventory(nodeid, inv); diff --git a/src/net_processing.h b/src/net_processing.h index a4f4e6a311c1..745bac637177 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -112,6 +112,11 @@ class PeerManagerInternal * turns true), so it is not requested from anyone anymore. * Requires ::cs_main (see the PeerManagerImpl override). */ virtual void PeerForgetObjectRequest(const CInv& inv) = 0; + /** Treat the inv as announced by this peer: unless we already have the object, register it with + * the object request tracker under the same per-peer limits as a received INV, so it is fetched + * from this peer or, failing that, from another peer that announced it. + * Requires ::cs_main (see the PeerManagerImpl override). */ + virtual void PeerAddObjectAnnouncement(const CNode& node, const CInv& inv) = 0; virtual void PeerPushInventory(NodeId nodeid, const CInv& inv) = 0; virtual void PeerRelayInv(const CInv& inv) = 0; virtual void PeerRelayInvFiltered(const CInv& inv, const CTransaction& relatedTx) = 0; diff --git a/src/test/governance_inv_tests.cpp b/src/test/governance_inv_tests.cpp index af8c06e46a4a..74dac5e7714f 100644 --- a/src/test/governance_inv_tests.cpp +++ b/src/test/governance_inv_tests.cpp @@ -25,11 +25,9 @@ #include -#include #include #include #include -#include #include using namespace std::chrono_literals; @@ -493,8 +491,8 @@ BOOST_AUTO_TEST_CASE(orphan_votes_require_a_valid_masternode_signature) connman.FlushSendBuffer(*peer); ProcessGovernanceVote(net_gov, *peer, vote); - BOOST_CHECK(m_node.govman->GetOrphanVoteObjectHashes().empty()); - BOOST_CHECK_EQUAL(CountQueuedMessages(*peer, NetMsgType::MNGOVERNANCESYNC), 0U); + m_node.peerman->SendMessages(peer.get()); + BOOST_CHECK_EQUAL(CountQueuedMessages(*peer, NetMsgType::GETDATA), 0U); BOOST_CHECK_EQUAL(MisbehaviorScore(*m_node.peerman, *peer), 20); m_node.peerman->FinalizeNode(*peer); @@ -542,36 +540,4 @@ BOOST_AUTO_TEST_CASE(invalid_vote_is_scored_alike_with_and_without_a_parent_obje chainstate.ResetIbd(); } -// Every orphan parent hash is re-requested from every peer on each governance tick, and one -// masternode key can sign votes for any number of invented parents, so the hashes requested per tick -// must stay bounded. -BOOST_AUTO_TEST_CASE(orphan_object_requests_are_capped_per_tick) -{ - constexpr size_t cap{CGovernanceManager::MAX_ORPHAN_OBJECT_REQUESTS}; - std::set parents; - const auto add_orphans = [&](size_t count) { - for (size_t i = 0; i < count; ++i) { - const uint256 parent{GetRandHash()}; - parents.insert(parent); - m_node.govman->AddOrphanVoteForTesting(MakeGovernanceVote(parent)); - } - }; - - // Up to the cap, every orphan parent is requested. - add_orphans(cap); - auto hashes{m_node.govman->GetOrphanVoteObjectHashes()}; - BOOST_CHECK(std::set(hashes.begin(), hashes.end()) == parents); - - // Beyond it, each tick requests exactly cap distinct ones among them. - add_orphans(cap); - for (int tick = 0; tick < 2; ++tick) { - hashes = m_node.govman->GetOrphanVoteObjectHashes(); - BOOST_CHECK_EQUAL(hashes.size(), cap); - const std::set unique(hashes.begin(), hashes.end()); - BOOST_CHECK_EQUAL(unique.size(), cap); - BOOST_CHECK(std::includes(parents.begin(), parents.end(), unique.begin(), unique.end())); - } -} - - BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/governance_vote_processing_tests.cpp b/src/test/governance_vote_processing_tests.cpp index 57c05c103b8f..b496fdcffd28 100644 --- a/src/test/governance_vote_processing_tests.cpp +++ b/src/test/governance_vote_processing_tests.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -15,8 +16,11 @@ #include #include #include +#include +#include #include #include +#include #include