fix: fetch governance orphan-vote parents through the request tracker and bound the orphan cache per masternode

src/cachemultimap.h

@@ -110,6 +110,16 @@ class CacheMultiMap
         return (mapIndex.find(key) != mapIndex.end());
     }
 
+    const V* Find(const K& key, const V& value) const
+    {
+        map_cit mit = mapIndex.find(key);
+        if (mit == mapIndex.end()) {
+            return nullptr;
+        }
+        it_map_cit it = mit->second.find(value);
+        return it == mit->second.end() ? nullptr : &it->second->value;
+    }
+
     bool Get(const K& key, V& value) const
     {
         map_cit it = mapIndex.find(key);

src/governance/governance.cpp

@@ -61,11 +61,72 @@ class ScopedLockBool
 };
 } // anonymous namespace
 
+namespace governance {
+OrphanVoteCache::OrphanVoteCache(size_t max_total, size_t max_per_mn) :
+    m_max_per_mn{max_per_mn},
+    m_cache{static_cast<CacheMultiMap<uint256, OrphanVote>::size_type>(max_total)}
+{
+    assert(max_total > 0);
+}
+
+bool OrphanVoteCache::Insert(const uint256& parent_hash, const OrphanVote& orphan_vote)
+{
+    if (m_cache.Find(parent_hash, orphan_vote) != nullptr) return true;
+
+    const COutPoint& outpoint{orphan_vote.vote.GetMasternodeOutpoint()};
+    if (const auto it{m_count_per_mn.find(outpoint)}; it != m_count_per_mn.end() && it->second >= m_max_per_mn) {
+        return false;
+    }
+    // Evict before m_cache would prune by itself, so the evicted vote's masternode gets its share back.
+    if (m_cache.GetSize() >= m_cache.GetMaxSize()) {
+        const auto& oldest{m_cache.GetItemList().back()};
+        Erase(oldest.key, oldest.value);
+    }
+    m_cache.Insert(parent_hash, orphan_vote);
+    ++m_count_per_mn[outpoint];
+    return true;
+}
+
+void OrphanVoteCache::Erase(const uint256& parent_hash, const OrphanVote& orphan_vote)
+{
+    const OrphanVote* stored{m_cache.Find(parent_hash, orphan_vote)};
+    if (stored == nullptr) return;
+    // Release the share of the stored entry, which the argument only has to compare equal to. Read it
+    // first: erasing destroys it.
+    const COutPoint outpoint{stored->vote.GetMasternodeOutpoint()};
+    m_cache.Erase(parent_hash, orphan_vote);
+
+    const auto it{m_count_per_mn.find(outpoint)};
+    if (!Assume(it != m_count_per_mn.end())) return;
+    if (--it->second == 0) m_count_per_mn.erase(it);
+}
+
+void OrphanVoteCache::EraseExpired(NodeSeconds now)
+{
+    const auto& items{m_cache.GetItemList()};
+    for (auto it = items.begin(); it != items.end();) {
+        const auto& item{*it++};
+        if (item.value.expiration < now) Erase(item.key, item.value);
+    }
+}
+
+void OrphanVoteCache::GetAll(const uint256& parent_hash, std::vector<OrphanVote>& orphan_votes)
+{
+    m_cache.GetAll(parent_hash, orphan_votes);
+}
+
+void OrphanVoteCache::Clear()
+{
+    m_cache.Clear();
+    m_count_per_mn.clear();
+}
+} // namespace governance
+
 GovernanceStore::GovernanceStore() :
     cs_store(),
     mapObjects(),
     mapErasedGovernanceObjects(),
-    cmmapOrphanVotes(MAX_CACHE_SIZE),
+    m_orphan_votes(governance::MAX_ORPHAN_VOTES, governance::MAX_ORPHAN_VOTES_PER_MN),
     mapLastMasternodeObject(),
     lastMNListForVotingKeys(std::make_shared<CDeterministicMNList>())
 {
@@ -281,7 +342,7 @@ void CGovernanceManager::CheckOrphanVotes(CGovernanceObject& govobj)
 
     uint256 nHash = govobj.GetHash();
     std::vector<governance::OrphanVote> orphan_votes;
-    cmmapOrphanVotes.GetAll(nHash, orphan_votes);
+    m_orphan_votes.GetAll(nHash, orphan_votes);
 
     ScopedLockBool guard(cs_store, fRateChecksEnabled, false);
 
@@ -298,7 +359,7 @@ void CGovernanceManager::CheckOrphanVotes(CGovernanceObject& govobj)
             fRemove = true;
         }
         if (fRemove) {
-            cmmapOrphanVotes.Erase(nHash, orphan_vote);
+            m_orphan_votes.Erase(nHash, orphan_vote);
         }
     }
 }
@@ -405,6 +466,8 @@ void CGovernanceManager::CheckAndRemove()
 
     ScopedLockBool guard(cs_store, fRateChecksEnabled, false);
 
+    m_orphan_votes.EraseExpired(Now<NodeSeconds>());
+
     // Clean up any expired or invalid triggers
     m_superblocks.Clean(nCachedBlockHeight);
 
@@ -836,8 +899,13 @@ 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
+        if (m_orphan_votes.Insert(nHashGovobj,
+                                  governance::OrphanVote{vote, Now<NodeSeconds>() + GOVERNANCE_ORPHAN_EXPIRATION_TIME})) {
+            // Even if we already held this vote: every peer that relays it can serve the parent.
+            hashToRequest = nHashGovobj;
+        } else {
+            LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s -- MN outpoint %s holds its share of orphan votes, dropping vote %s\n",
+                     __func__, vote.GetMasternodeOutpoint().ToStringShort(), nHashVote.ToString());
         }
         LogPrint(BCLog::GOBJECT, "%s\n", msg);
         return false;
@@ -1013,7 +1081,7 @@ void GovernanceStore::Clear()
     LOCK(cs_store);
     mapObjects.clear();
     mapErasedGovernanceObjects.clear();
-    cmmapOrphanVotes.Clear();
+    m_orphan_votes.Clear();
     mapLastMasternodeObject.clear();
     lastMNListForVotingKeys = std::make_shared<CDeterministicMNList>();
 }
@@ -1123,35 +1191,6 @@ void CGovernanceManager::UpdatedBlockTip(const CBlockIndex* pindex)
     m_superblocks.ExecuteBestSuperblock(m_dmnman.GetListAtChainTip(), pindex->nHeight);
 }
 
-std::vector<uint256> CGovernanceManager::GetOrphanVoteObjectHashes()
-{
-    LOCK(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;
-        ++it;
-        if (prevIt->value.expiration < now) {
-            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;
-}
-
 void CGovernanceManager::RemoveInvalidVotes()
 {
     AssertLockHeld(cs_store);
@@ -1191,7 +1230,6 @@ void CGovernanceManager::RemoveInvalidVotes()
             }
             for (const auto& voteHash : removed) {
                 cmapVoteToObject.Erase(voteHash);
-                cmmapOrphanVotes.Erase(voteHash);
             }
         }
     }

src/governance/governance.h

@@ -58,6 +58,53 @@ inline bool operator<(const OrphanVote& lhs, const OrphanVote& rhs)
 {
     return lhs.vote < rhs.vote;
 }
+
+// Orphan votes are not scarce: one masternode can sign any number of votes naming parents that do
+// not exist. Each masternode is therefore limited to its own share of the cache, and the cache as a
+// whole holds about 40 MB at most (roughly 800 bytes per entry). Filling it takes 500 masternodes.
+static constexpr size_t MAX_ORPHAN_VOTES{50'000};
+static constexpr size_t MAX_ORPHAN_VOTES_PER_MN{100};
+
+/** Votes waiting for their parent object, keyed by parent hash, with the oldest evicted once the
+ *  cache is full and each masternode limited to a share of it. */
+class OrphanVoteCache
+{
+public:
+    explicit OrphanVoteCache(size_t max_total, size_t max_per_mn);
+
+    /** Returns whether the vote is held, either newly or because it already was. A new vote is
+     *  dropped if its masternode already holds max_per_mn orphan votes. */
+    [[nodiscard]] bool Insert(const uint256& parent_hash, const OrphanVote& orphan_vote);
+    void Erase(const uint256& parent_hash, const OrphanVote& orphan_vote);
+    void EraseExpired(NodeSeconds now);
+    void GetAll(const uint256& parent_hash, std::vector<OrphanVote>& orphan_votes);
+    void Clear();
+
+    template <typename Stream>
+    void Serialize(Stream& s) const
+    {
+        s << m_cache;
+    }
+
+    template <typename Stream>
+    void Unserialize(Stream& s)
+    {
+        // Re-insert rather than adopt the stored cache: the file carries the capacity of whichever
+        // version wrote it, and not the per-masternode counts. Oldest first, to keep the order.
+        CacheMultiMap<uint256, OrphanVote> stored;
+        s >> stored;
+        Clear();
+        const auto& items = stored.GetItemList();
+        for (auto it = items.rbegin(); it != items.rend(); ++it) {
+            (void)Insert(it->key, it->value);
+        }
+    }
+
+private:
+    const size_t m_max_per_mn;
+    CacheMultiMap<uint256, OrphanVote> m_cache;
+    std::map<COutPoint, size_t> m_count_per_mn;
+};
 } // namespace governance
 
 static constexpr int RATE_BUFFER_SIZE = 5;
@@ -173,7 +220,6 @@ class GovernanceStore
     };
 
     using txout_m_t = std::map<COutPoint, last_object_rec>;
-    using vote_cmm_t = CacheMultiMap<uint256, governance::OrphanVote>;
 
 protected:
     static constexpr int MAX_CACHE_SIZE = 1000000;
@@ -189,7 +235,7 @@ class GovernanceStore
     //   key   - governance object's hash
     //   value - expiration time for deleted objects
     std::map<uint256, int64_t> mapErasedGovernanceObjects GUARDED_BY(cs_store);
-    vote_cmm_t cmmapOrphanVotes GUARDED_BY(cs_store);
+    governance::OrphanVoteCache m_orphan_votes GUARDED_BY(cs_store);
     txout_m_t mapLastMasternodeObject GUARDED_BY(cs_store);
     // used to check for changed voting keys
     std::shared_ptr<CDeterministicMNList> lastMNListForVotingKeys GUARDED_BY(cs_store);
@@ -204,7 +250,7 @@ class GovernanceStore
         LOCK(cs_store);
         s   << SERIALIZATION_VERSION_STRING
             << mapErasedGovernanceObjects
-            << cmmapOrphanVotes
+            << m_orphan_votes
             << mapObjects
             << mapLastMasternodeObject
             << *lastMNListForVotingKeys;
@@ -223,7 +269,7 @@ class GovernanceStore
         }
 
         s   >> mapErasedGovernanceObjects
-            >> cmmapOrphanVotes
+            >> m_orphan_votes
             >> mapObjects
             >> mapLastMasternodeObject
             >> *lastMNListForVotingKeys;
@@ -355,8 +401,6 @@ 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);
     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);
@@ -372,7 +416,8 @@ class CGovernanceManager : public GovernanceStore
 
     CDeterministicMNManager& GetMNManager();
     /** Process a governance vote. Returns true on success.
-     *  If the vote is for an unknown object (orphan), hashToRequest is set to the object hash. */
+     *  If the vote is for an unknown object (orphan) and is held until that object arrives,
+     *  hashToRequest is set to the object hash. */
     bool ProcessVote(const CGovernanceVote& vote, CGovernanceException& exception, uint256& hashToRequest)
         EXCLUSIVE_LOCKS_REQUIRED(!cs_store);
 

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());

src/net_processing.cpp

@@ -642,6 +642,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);
@@ -6958,6 +6960,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<std::chrono::microseconds>());
+}
+
 void PeerManagerImpl::PeerPushInventory(NodeId nodeid, const CInv& inv)
 {
     PushInventory(nodeid, inv);

src/net_processing.h

@@ -109,6 +109,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;

src/test/governance_inv_tests.cpp

@@ -491,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);