fix: fetch orphan-vote parents via the request tracker instead of broadcasting

src/governance/governance.cpp

@@ -63,7 +63,7 @@ GovernanceStore::GovernanceStore() :
     cs_store(),
     mapObjects(),
     mapErasedGovernanceObjects(),
-    cmmapOrphanVotes(MAX_CACHE_SIZE),
+    cmmapOrphanVotes(MAX_ORPHAN_VOTES),
     mapLastMasternodeObject(),
     lastMNListForVotingKeys(std::make_shared<CDeterministicMNList>())
 {
@@ -406,6 +406,11 @@ void CGovernanceManager::CheckAndRemove()
 
     ScopedLockBool guard(cs_store, fRateChecksEnabled, false);
 
+    // Drop orphan votes whose parent never arrived. Votes for an object that did arrive are
+    // consumed by CheckOrphanVotes() at that point, so anything still here is either waiting or
+    // dead; this is the only thing that removes the latter.
+    ExpireOrphanVotes();
+
     // Clean up any expired or invalid triggers
     m_superblocks.Clean(nCachedBlockHeight);
 
@@ -829,9 +834,12 @@ 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<NodeSeconds>() + GOVERNANCE_ORPHAN_EXPIRATION_TIME})) {
-            hashToRequest = nHashGovobj; // Caller should request this object
-        }
+        cmmapOrphanVotes.Insert(nHashGovobj, governance::OrphanVote{vote, Now<NodeSeconds>() + GOVERNANCE_ORPHAN_EXPIRATION_TIME});
+        // Ask for the parent whether or not the vote itself was new to us. A vote we already hold,
+        // relayed by a second peer, is fresh evidence that this peer has the parent -- and it is the
+        // only evidence we will get, since a peer relays a given vote once. Suppressing the request
+        // on a duplicate would strand the parent whenever the first peer we asked fails to deliver.
+        hashToRequest = nHashGovobj;
         LogPrint(BCLog::GOBJECT, "%s\n", msg);
         return false;
     }
@@ -1012,6 +1020,7 @@ void GovernanceStore::Clear()
     mapObjects.clear();
     mapErasedGovernanceObjects.clear();
     cmmapOrphanVotes.Clear();
+    cmmapOrphanVotes.SetMaxSize(MAX_ORPHAN_VOTES);
     mapLastMasternodeObject.clear();
     lastMNListForVotingKeys = std::make_shared<CDeterministicMNList>();
 }
@@ -1089,13 +1098,11 @@ void CGovernanceManager::UpdatedBlockTip(const CBlockIndex* pindex)
     m_superblocks.ExecuteBestSuperblock(m_dmnman.GetListAtChainTip(), pindex->nHeight);
 }
 
-std::vector<uint256> CGovernanceManager::GetOrphanVoteObjectHashes()
+void CGovernanceManager::ExpireOrphanVotes()
 {
-    LOCK(cs_store);
+    AssertLockHeld(cs_store);
 
     const auto now{Now<NodeSeconds>()};
-
-    // 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;
@@ -1104,18 +1111,11 @@ std::vector<uint256> CGovernanceManager::GetOrphanVoteObjectHashes()
             cmmapOrphanVotes.Erase(prevIt->key, prevIt->value);
         }
     }
+}
 
-    // Get hashes of objects we don't have yet
-    std::vector<uint256> vecHashesFiltered;
-    std::vector<uint256> vecHashes;
-    cmmapOrphanVotes.GetKeys(vecHashes);
-    for (const uint256& nHash : vecHashes) {
-        if (mapObjects.find(nHash) == mapObjects.end()) {
-            vecHashesFiltered.push_back(nHash);
-        }
-    }
-
-    return vecHashesFiltered;
+size_t CGovernanceManager::GetOrphanVoteCount() const
+{
+    return WITH_LOCK(cs_store, return cmmapOrphanVotes.GetSize());
 }
 
 void CGovernanceManager::RemoveInvalidVotes()

src/governance/governance.h

@@ -176,6 +176,13 @@ class GovernanceStore
     using txout_m_t = std::map<COutPoint, last_object_rec>;
     using vote_cmm_t = CacheMultiMap<uint256, governance::OrphanVote>;
 
+public:
+    /** Bound for the orphan-vote cache, which is filled from the network by any peer with a parent
+     *  object we do not have. Orphans are short-lived recovery state for votes that outran their
+     *  object during relay, so this only has to cover objects genuinely in flight, not the whole
+     *  governance set. MAX_CACHE_SIZE would allow ~750 MB of peer-supplied data here. */
+    static constexpr int MAX_ORPHAN_VOTES = 1000;
+
 protected:
     static constexpr int MAX_CACHE_SIZE = 1000000;
     static const std::string SERIALIZATION_VERSION_STRING;
@@ -228,9 +235,15 @@ class GovernanceStore
 
         // TODO: Stop consuming the historical invalid-vote-cache field on the next disk-format version bump.
         CacheMap<uint256, CGovernanceVote> discarded_invalid_votes;
+        // The historical format stores CacheMultiMap's capacity with its entries. Consume that
+        // field to preserve the format, but keep both the stale orphan votes and their disk-supplied
+        // capacity out of the live cache. Orphans are a ten-minute recovery window invalidated by
+        // the restart; the live capacity is node policy established by Clear().
+        vote_cmm_t discarded_orphan_votes;
+
         s   >> mapErasedGovernanceObjects
             >> discarded_invalid_votes
-            >> cmmapOrphanVotes
+            >> discarded_orphan_votes
             >> mapObjects
             >> mapLastMasternodeObject
             >> *lastMNListForVotingKeys;
@@ -365,8 +378,8 @@ class CGovernanceManager : public GovernanceStore
     // Used by NetGovernance
     std::vector<CInv> FetchRelayInventory() EXCLUSIVE_LOCKS_REQUIRED(!cs_relay);
     void CheckAndRemove() EXCLUSIVE_LOCKS_REQUIRED(!cs_store);
-    /** Get hashes of governance objects for which we have orphan votes. Also cleans up expired orphans. */
-    [[nodiscard]] std::vector<uint256> GetOrphanVoteObjectHashes() EXCLUSIVE_LOCKS_REQUIRED(!cs_store);
+    /** Number of orphan votes currently held, so the MAX_ORPHAN_VOTES bound can be asserted. */
+    [[nodiscard]] size_t GetOrphanVoteCount() const EXCLUSIVE_LOCKS_REQUIRED(!cs_store);
     std::pair<std::vector<uint256>, std::vector<uint256>> FetchGovernanceObjectVotes(
         size_t peers_per_hash_max, int64_t now, std::map<uint256, std::map<CService, int64_t>>& map_asked_recently) const
         EXCLUSIVE_LOCKS_REQUIRED(!cs_store);
@@ -417,6 +430,10 @@ class CGovernanceManager : public GovernanceStore
     void CheckOrphanVotes(CGovernanceObject& govobj)
         EXCLUSIVE_LOCKS_REQUIRED(cs_store, !cs_relay);
 
+    /** Drop orphan votes whose parent object never arrived within GOVERNANCE_ORPHAN_EXPIRATION_TIME. */
+    void ExpireOrphanVotes()
+        EXCLUSIVE_LOCKS_REQUIRED(cs_store);
+
     void RebuildIndexes()
         EXCLUSIVE_LOCKS_REQUIRED(cs_store);
 

src/governance/net_governance.cpp

@@ -45,23 +45,9 @@ 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
+            // Also expires orphan votes whose parent object never arrived. Fetching those parents
+            // is driven by the object request tracker from ProcessMessage(), not from here.
             m_gov_manager.CheckAndRemove();
         },
         std::chrono::minutes{5});
@@ -257,11 +243,13 @@ void NetGovernance::ProcessMessage(CNode& peer, const std::string& msg_type, CDa
             // m_peer_manager->PeerRelayInv(CInv{MSG_GOVERNANCE_OBJECT_VOTE, nHash});
         } 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));
+            if (!hashToRequest.IsNull()) {
+                // Orphan vote: fetch the parent object through the request tracker, which owns
+                // GETDATA scheduling, per-peer in-flight limits, expiry and fallback to another
+                // peer. Register this peer explicitly -- holding a vote for the object is evidence
+                // it has the object, and it may never have announced the object to us.
+                m_peer_manager->PeerAskPeersForObject(CInv{MSG_GOVERNANCE_OBJECT, hashToRequest},
+                                                      peer.GetId());
             }
             if ((exception.GetNodePenalty() != 0) && m_node_sync.IsSynced()) {
                 m_peer_manager->PeerMisbehaving(peer.GetId(), exception.GetNodePenalty());

src/instantsend/net_instantsend.cpp

@@ -414,7 +414,7 @@ void NetInstantSend::ProcessInstantSendLock(NodeId from, const uint256& hash, co
         m_peer_manager->PeerRelayInvFiltered(inv, *tx);
     } else {
         m_peer_manager->PeerRelayInvFiltered(inv, islock->txid);
-        m_peer_manager->PeerAskPeersForTransaction(islock->txid);
+        m_peer_manager->PeerAskPeersForObject(CInv{MSG_TX, islock->txid});
     }
 }
 

src/net_processing.cpp

@@ -85,6 +85,10 @@ using node::fReindex;
 /** Maximum number of in-flight object requests from a peer. It is not a hard limit, but the
  *  threshold at which point the OVERLOADED_PEER_OBJECT_DELAY kicks in. */
 static constexpr int32_t MAX_PEER_OBJECT_REQUEST_IN_FLIGHT = 100;
+/** How many peers to ask for an object we want but were never offered (see AskPeersForObject).
+ *  Small on purpose: the request tracker retries and falls back to the next candidate on expiry, so
+ *  this is the width of the initial attempt, not the number of chances to obtain the object. */
+static constexpr size_t MAX_PEERS_TO_ASK_FOR_OBJECT = 4;
 /** Maximum number of announced objects from a peer.
  *  Unlike Bitcoin, this is not reduced to 5000: governance vote sync legitimately announces up to
  *  MAX_INV_SZ objects from a single peer (see CGovernanceManager). */
@@ -644,15 +648,16 @@ class PeerManagerImpl final : public PeerManager
     void PeerRelayDSQ(const CCoinJoinQueue& queue) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
     void PeerRelayTransaction(const uint256& txid) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
     void PeerRelayRecoveredSig(const llmq::CRecoveredSig& sig, bool proactive_relay) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
-    void PeerAskPeersForTransaction(const uint256& txid) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
+    void PeerAskPeersForObject(const CInv& inv, NodeId explicit_peer) override
+        EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !cs_main);
     size_t PeerGetRequestedObjectCount(NodeId nodeid) const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, ::cs_main);
     void PeerPostProcessMessage(MessageProcessingResult&& ret) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
 
 private:
     void _RelayTransaction(const uint256& txid) EXCLUSIVE_LOCKS_REQUIRED(cs_main, !m_peer_mutex);
 
-    /** Ask peers that have a transaction in their inventory to relay it to us. */
-    void AskPeersForTransaction(const uint256& txid) EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
+    /** Ask peers that have the object in their inventory to relay it to us, plus explicit_peer. */
+    void AskPeersForObject(const CInv& inv, NodeId explicit_peer) EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !cs_main);
 
     /** Relay inventories to peers that find it relevant */
     void RelayInvFiltered(const CInv& inv, const CTransaction& relatedTx) EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
@@ -2390,43 +2395,65 @@ void PeerManagerImpl::SendPings()
     for(auto& it : m_peer_map) it.second->m_ping_queued = true;
 }
 
-void PeerManagerImpl::AskPeersForTransaction(const uint256& txid)
+void PeerManagerImpl::AskPeersForObject(const CInv& inv, NodeId explicit_peer)
 {
-    std::vector<PeerRef> peersToAsk;
-    peersToAsk.reserve(4);
-
+    std::vector<PeerRef> candidates;
     {
         READ_LOCK(m_peer_mutex);
+        candidates.reserve(m_peer_map.size());
+
+        // A peer that holds the object without having announced it is not in any inventory filter,
+        // so it can only be reached by being named explicitly. Candidate selection order remains
+        // the request tracker's responsibility.
+        if (explicit_peer != -1) {
+            if (auto it = m_peer_map.find(explicit_peer); it != m_peer_map.end()) {
+                candidates.emplace_back(it->second);
+            }
+        }
+
         // TODO consider prioritizing MNs again, once that flag is moved into Peer
         for (const auto& [_, peer] : m_peer_map) {
-            if (peersToAsk.size() >= 4) {
-                break;
-            }
-            if (IsInvInFilter(*peer, txid)) {
-                peersToAsk.emplace_back(peer);
+            if (peer->m_id != explicit_peer && IsInvInFilter(*peer, inv.hash)) {
+                candidates.emplace_back(peer);
             }
         }
     }
-    {
-        LOCK(cs_main);
-        const auto current_time{GetTime<std::chrono::microseconds>()};
-        // Register a fresh, preferred (undelayed) MSG_TX announcement from each peer we intend to
-        // ask, so the transaction is requested ASAP. We deliberately do not forget existing
-        // announcements for this txid: any live candidate/request from another peer must survive as
-        // a fallback, and there is nothing to "unstick" -- the tracker deletes a txid's COMPLETED
-        // announcements automatically once no live one remains, so a completed entry only lingers
-        // while some peer is still being tried. If a peer here already has an announcement,
-        // ReceivedInv is a no-op and the existing one (in flight or queued) keeps its place.
-        for (PeerRef& peer : peersToAsk) {
-            // The peer may have been disconnected (and its tracker state wiped by DisconnectedPeer)
-            // after we collected it above but before we took cs_main. Registering an announcement
-            // for a gone peer would leave a candidate that is never requested and could block the
-            // live fallback peers, so skip it.
-            if (State(peer->m_id) == nullptr) continue;
-            LogPrintf("PeerManagerImpl::%s -- txid=%s: asking other peer %d for correct TX\n", __func__,
-                      txid.ToString(), peer->m_id);
-
-            m_object_request.ReceivedInv(peer->m_id, CInv(MSG_TX, txid), /*preferred=*/true, current_time);
+
+    LOCK(cs_main);
+
+    const auto current_time{GetTime<std::chrono::microseconds>()};
+    size_t asked_count{0};
+
+    // Register a fresh, preferred announcement from each peer we intend to ask, so the object is
+    // requested ASAP. We deliberately do not forget existing announcements for this hash: any live
+    // candidate/request from another peer must survive as a fallback. If a peer here already has an
+    // announcement, ReceivedInv is a no-op and the existing one keeps its place.
+    auto try_ask_peer = [&](const PeerRef& peer) EXCLUSIVE_LOCKS_REQUIRED(cs_main) {
+        // The peer may have disconnected after the candidate snapshot. Recheck under cs_main so we
+        // cannot register an announcement after FinalizeNode has already cleaned up this peer.
+        if (State(peer->m_id) == nullptr) return false;
+        // Obey the same per-peer accounting AddObjectAnnouncement applies to announcements the peer
+        // sent us. A synthetic announcement is still an entry the peer's behaviour can cause us to
+        // create -- a peer that keeps naming objects we do not have would otherwise grow its tracker
+        // footprint without limit.
+        if (m_object_request.Count(peer->m_id) >= MAX_PEER_OBJECT_ANNOUNCEMENTS) return false;
+        const bool overloaded = m_object_request.CountInFlight(peer->m_id) >= MAX_PEER_OBJECT_REQUEST_IN_FLIGHT;
+        LogPrint(BCLog::NET, "PeerManagerImpl::%s -- %s: asking peer %d\n", __func__, inv.ToString(),
+                 peer->m_id);
+
+        // Preferred and otherwise undelayed: unlike a peer-initiated announcement, we asked for this
+        // one and want it as soon as the peer's in-flight budget allows.
+        m_object_request.ReceivedInv(peer->m_id, inv, /*preferred=*/true,
+                                     current_time + (overloaded ? OVERLOADED_PEER_OBJECT_DELAY : 0us));
+        return true;
+    };
+
+    for (const auto& peer : candidates) {
+        if (asked_count >= MAX_PEERS_TO_ASK_FOR_OBJECT) {
+            break;
+        }
+        if (try_ask_peer(peer)) {
+            ++asked_count;
         }
     }
 }
@@ -6797,9 +6824,9 @@ void PeerManagerImpl::PeerRelayTransaction(const uint256& txid)
     RelayTransaction(txid);
 }
 
-void PeerManagerImpl::PeerAskPeersForTransaction(const uint256& txid)
+void PeerManagerImpl::PeerAskPeersForObject(const CInv& inv, NodeId explicit_peer)
 {
-    AskPeersForTransaction(txid);
+    AskPeersForObject(inv, explicit_peer);
 }
 
 size_t PeerManagerImpl::PeerGetRequestedObjectCount(NodeId nodeid) const

src/net_processing.h

@@ -111,7 +111,19 @@ class PeerManagerInternal
     virtual void PeerRelayTransaction(const uint256& txid) = 0;
     virtual void PeerRelayDSQ(const CCoinJoinQueue& queue) = 0;
     virtual void PeerRelayRecoveredSig(const llmq::CRecoveredSig& sig, bool proactive_relay) = 0;
-    virtual void PeerAskPeersForTransaction(const uint256& txid) = 0;
+    /** Ask a few peers for an object we want but have not been offered, by registering a synthetic
+     *  announcement with the request tracker. The tracker then owns the fetch: GETDATA scheduling,
+     *  per-peer in-flight limits, expiry, and fallback to the next candidate.
+     *
+     *  Candidates are explicit_peer, if set, plus peers whose known-inventory filter already contains
+     *  the hash. That filter is only consulted for peers that enabled transaction relay, so for an
+     *  object type carried outside transaction relay -- and for any object nobody has announced to
+     *  us -- explicit_peer may be the only candidate. Pass it whenever a specific peer demonstrably
+     *  has the object without having announced it, such as one that sent a vote naming this parent.
+     *  The request tracker chooses among the candidates; explicit_peer does not imply request order.
+     *
+     *  Requires ::cs_main is NOT held. */
+    virtual void PeerAskPeersForObject(const CInv& inv, NodeId explicit_peer = -1) = 0;
     virtual size_t PeerGetRequestedObjectCount(NodeId nodeid) const = 0;
     virtual void PeerPostProcessMessage(MessageProcessingResult&& ret) = 0;
 };

src/test/governance_inv_tests.cpp

@@ -2,8 +2,10 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
+#include <clientversion.h>
 #include <common/bloom.h>
 #include <evo/chainhelper.h>
+#include <evo/deterministicmns.h>
 #include <governance/governance.h>
 #include <governance/net_governance.h>
 #include <governance/object.h>
@@ -501,7 +503,7 @@ 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(m_node.govman->GetOrphanVoteCount(), 0U);
     BOOST_CHECK_EQUAL(CountQueuedMessages(*peer, NetMsgType::MNGOVERNANCESYNC), 0U);
     AssertMisbehaviorScore(*m_node.peerman, *peer, 20);