From fd910822b95e3259a2007bee35b3d1081b448d14 Mon Sep 17 00:00:00 2001 From: pasta Date: Mon, 10 Aug 2026 08:57:53 -0500 Subject: [PATCH 1/8] refactor: generalize AskPeersForTransaction to AskPeersForObject The orphan-parent fetch helper was transaction-specific only in its CInv construction. Accept a CInv so other subsystems can use the object request tracker for objects they want but were never offered. Allow callers to name an explicit peer that demonstrably holds the object without requiring an inventory announcement. Register that peer together with inventory-filter candidates, while leaving request order and fallback scheduling to the tracker. Apply normal per-peer announcement and in-flight accounting to synthetic candidates. Snapshot candidates before taking cs_main so peer-map iteration and inventory-filter locking stay outside the global critical section, then recheck node state under cs_main before registration. Extract the candidate limit into MAX_PEERS_TO_ASK_FOR_OBJECT and demote the per-peer log line to BCLog::NET because callers may invoke this on a peer-driven path. --- src/instantsend/net_instantsend.cpp | 2 +- src/net_processing.cpp | 95 ++++++++++++++++++----------- src/net_processing.h | 14 ++++- 3 files changed, 75 insertions(+), 36 deletions(-) diff --git a/src/instantsend/net_instantsend.cpp b/src/instantsend/net_instantsend.cpp index 07e86fd780f4..2c8cedaab96a 100644 --- a/src/instantsend/net_instantsend.cpp +++ b/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}); } } diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 0e265c36bb2c..2312d7ad7ae3 100644 --- a/src/net_processing.cpp +++ b/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 peersToAsk; - peersToAsk.reserve(4); - + std::vector 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()}; - // 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()}; + 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 diff --git a/src/net_processing.h b/src/net_processing.h index 761da8001c07..477c6d848681 100644 --- a/src/net_processing.h +++ b/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; }; From 65029bcf3a9c8a15b6948c08459ee69eaaee62e8 Mon Sep 17 00:00:00 2001 From: pasta Date: Mon, 10 Aug 2026 08:58:04 -0500 Subject: [PATCH 2/8] fix: fetch orphan-vote parents via the request tracker, bound the orphan cache NetGovernance::Schedule sent one MNGOVERNANCESYNC per orphan parent hash per connected peer every five minutes for as long as the orphan remained. Orphan keys are peer-controlled, so this created repeated O(orphan parents x peers) outbound work outside the request tracker's limits. Seed the object request tracker directly from each peer that relays a valid orphan vote. Return the missing parent for duplicate relays as well, allowing later relayers to become fallback candidates. The tracker owns GETDATA scheduling, in-flight limits, expiry and fallback; candidate registration remains bounded per helper call without claiming that later relays cannot add candidates. Move orphan expiry into CheckAndRemove and cap the cache at MAX_ORPHAN_VOTES. Preserve the governance.dat format by consuming but discarding legacy orphan state and its serialized capacity, then reassert runtime cache bounds even after deserialization failures. Add focused coverage for cache bounds, legacy and failed loads, duplicate relays, and end-to-end multi-peer parent-request routing. --- src/governance/governance.cpp | 38 ++--- src/governance/governance.h | 23 ++- src/governance/net_governance.cpp | 30 +--- src/test/governance_inv_tests.cpp | 4 +- src/test/governance_vote_processing_tests.cpp | 159 +++++++++++++++++- 5 files changed, 208 insertions(+), 46 deletions(-) diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index d1f9f6100535..2a876660bb25 100644 --- a/src/governance/governance.cpp +++ b/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()) { @@ -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() + GOVERNANCE_ORPHAN_EXPIRATION_TIME})) { - hashToRequest = nHashGovobj; // Caller should request this object - } + cmmapOrphanVotes.Insert(nHashGovobj, governance::OrphanVote{vote, Now() + 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(); } @@ -1089,13 +1098,11 @@ void CGovernanceManager::UpdatedBlockTip(const CBlockIndex* pindex) m_superblocks.ExecuteBestSuperblock(m_dmnman.GetListAtChainTip(), pindex->nHeight); } -std::vector CGovernanceManager::GetOrphanVoteObjectHashes() +void CGovernanceManager::ExpireOrphanVotes() { - LOCK(cs_store); + AssertLockHeld(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; @@ -1104,18 +1111,11 @@ std::vector CGovernanceManager::GetOrphanVoteObjectHashes() 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); - } - } - - return vecHashesFiltered; +size_t CGovernanceManager::GetOrphanVoteCount() const +{ + return WITH_LOCK(cs_store, return cmmapOrphanVotes.GetSize()); } void CGovernanceManager::RemoveInvalidVotes() diff --git a/src/governance/governance.h b/src/governance/governance.h index 6b2eebc62ffb..51521e0b708e 100644 --- a/src/governance/governance.h +++ b/src/governance/governance.h @@ -176,6 +176,13 @@ class GovernanceStore using txout_m_t = std::map; using vote_cmm_t = CacheMultiMap; +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 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 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 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> FetchGovernanceObjectVotes( size_t peers_per_hash_max, int64_t now, std::map>& 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); diff --git a/src/governance/net_governance.cpp b/src/governance/net_governance.cpp index 2e8b5da79140..278dea3cf71a 100644 --- a/src/governance/net_governance.cpp +++ b/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()); diff --git a/src/test/governance_inv_tests.cpp b/src/test/governance_inv_tests.cpp index 880670bf862d..caafefdae2de 100644 --- a/src/test/governance_inv_tests.cpp +++ b/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 #include #include +#include #include #include #include @@ -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); diff --git a/src/test/governance_vote_processing_tests.cpp b/src/test/governance_vote_processing_tests.cpp index a5aa2124e3a3..2069c39ed3e5 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,6 +16,7 @@ #include #include #include +#include #include #include #include