From f0a46cfeb7b1a089ae6616c17878faf78ca5c738 Mon Sep 17 00:00:00 2001 From: pasta Date: Sun, 2 Aug 2026 16:54:11 -0500 Subject: [PATCH 1/4] refactor: generalize AskPeersForTransaction to AskPeersForObject The orphan-parent fetch helper was tx-specific only in its CInv construction. Take a CInv instead of a txid so other subsystems can use the object request tracker to fetch something they know they want but were never offered. Add prefer_first for a peer that demonstrably holds the object without having announced it: such a peer is in no inventory filter, so it is unreachable by the existing filter-based candidate search and can only be named. Extract the hardcoded 4 into MAX_PEERS_TO_ASK_FOR_OBJECT. Demote the per-peer log line from LogPrintf to LogPrint(BCLog::NET). The next commit calls this on a path a peer can drive, where unconditional logging would be a log-spam vector. --- src/instantsend/net_instantsend.cpp | 2 +- src/net_processing.cpp | 55 +++++++++++++++++++---------- src/net_processing.h | 8 ++++- 3 files changed, 44 insertions(+), 21 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 9e5df50aeece..65d52437cafa 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,17 @@ 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 prefer_first) 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 prefer_first. */ + void AskPeersForObject(const CInv& inv, NodeId prefer_first) + 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); @@ -2387,19 +2393,30 @@ 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 prefer_first) { std::vector peersToAsk; - peersToAsk.reserve(4); + peersToAsk.reserve(MAX_PEERS_TO_ASK_FOR_OBJECT); { READ_LOCK(m_peer_mutex); + // 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. Ask it first: it is the one candidate we have + // positive evidence for. + if (prefer_first != -1) { + if (auto it = m_peer_map.find(prefer_first); it != m_peer_map.end()) { + peersToAsk.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) { + if (peersToAsk.size() >= MAX_PEERS_TO_ASK_FOR_OBJECT) { break; } - if (IsInvInFilter(*peer, txid)) { + if (peer->m_id == prefer_first) { + continue; + } + if (IsInvInFilter(*peer, inv.hash)) { peersToAsk.emplace_back(peer); } } @@ -2407,23 +2424,23 @@ void PeerManagerImpl::AskPeersForTransaction(const uint256& txid) { 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. + // Register a fresh, preferred (undelayed) 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, and + // there is nothing to "unstick" -- the tracker deletes a hash'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); + LogPrint(BCLog::NET, "PeerManagerImpl::%s -- %s: asking peer %d\n", __func__, inv.ToString(), + peer->m_id); - m_object_request.ReceivedInv(peer->m_id, CInv(MSG_TX, txid), /*preferred=*/true, current_time); + m_object_request.ReceivedInv(peer->m_id, inv, /*preferred=*/true, current_time); } } } @@ -6782,9 +6799,9 @@ void PeerManagerImpl::PeerRelayTransaction(const uint256& txid) RelayTransaction(txid); } -void PeerManagerImpl::PeerAskPeersForTransaction(const uint256& txid) +void PeerManagerImpl::PeerAskPeersForObject(const CInv& inv, NodeId prefer_first) { - AskPeersForTransaction(txid); + AskPeersForObject(inv, prefer_first); } size_t PeerManagerImpl::PeerGetRequestedObjectCount(NodeId nodeid) const diff --git a/src/net_processing.h b/src/net_processing.h index 70e136f43f82..feb7a7caffdb 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -111,7 +111,13 @@ 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 peers + * known to have the hash, plus prefer_first if set -- use that for a peer that demonstrably has + * the object without having announced it (e.g. it sent a vote naming this parent object). + * Requires ::cs_main is NOT held. */ + virtual void PeerAskPeersForObject(const CInv& inv, NodeId prefer_first = -1) = 0; virtual size_t PeerGetRequestedObjectCount(NodeId nodeid) const = 0; virtual void PeerPostProcessMessage(MessageProcessingResult&& ret) = 0; }; From 45467b7ddc029d3adc413e5a92223ab8f319c533 Mon Sep 17 00:00:00 2001 From: pasta Date: Sun, 2 Aug 2026 17:58:15 -0500 Subject: [PATCH 2/4] 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 5 minutes, uncapped, for as long as the orphans lived. Orphan keys come from any unauthenticated peer, so that is O(peer-controlled x peers) outbound messages on a timer. PushMessage appends to vSendMsg regardless of fPauseSend, so the per-peer send buffer ceiling does not bound it. The sweep was also redundant. A non-zero MNGOVERNANCESYNC with an empty filter is special-cased on the serving side to reply with an INV{MSG_GOVERNANCE_OBJECT}, which flows into the object request tracker anyway; the broadcast existed only to induce that announcement, and had to be exempted from the HasFulfilledRequest anti-spam accounting to work. Seed the tracker directly instead, via PeerAskPeersForObject, naming the peer that supplied the vote: holding a vote for an object is evidence it has the object, and it may never have announced the object to us. The tracker then owns GETDATA scheduling, in-flight limits, expiry-driven fallback and AlreadyHave dedup. Fan-out per orphan parent drops from O(peers) every 5 minutes to at most 4 requests, once, and one round trip is saved. Move orphan expiry out of the deleted GetOrphanVoteObjectHashes() into ExpireOrphanVotes(), called from CheckAndRemove() on the same 5-minute tick. Insertion is gated on IsBlockchainSynced() just as CheckAndRemove() is, so orphans can only be created in states where expiry also runs. Bound cmmapOrphanVotes with MAX_ORPHAN_VOTES = 1000 rather than MAX_CACHE_SIZE = 1000000. Each retained entry costs ~750 bytes: CacheMultiMap stores the value twice, and each CGovernanceVote copy holds a heap-allocated signature. No masternode/signature validation is added before orphan insertion. A valid MN signature is not scarce (nParentHash is signed, but nothing ties it to an object that exists), the orphan branch must stay at penalty 0 because reaching it is a routine relay race for honest peers, and scoring is suppressed while !IsSynced() anyway. It would add ECDSA and BLS verification under cs_store on a peer-driven path. --- src/governance/governance.cpp | 28 +++++------ src/governance/governance.h | 15 +++++- src/governance/net_governance.cpp | 30 ++++-------- src/test/governance_inv_tests.cpp | 79 +++++++++++++++++++++++++++++-- 4 files changed, 109 insertions(+), 43 deletions(-) diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index d87fef40fac5..bf75c341afec 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -64,7 +64,7 @@ GovernanceStore::GovernanceStore() : mapObjects(), mapErasedGovernanceObjects(), cmapInvalidVotes(MAX_CACHE_SIZE), - cmmapOrphanVotes(MAX_CACHE_SIZE), + cmmapOrphanVotes(MAX_ORPHAN_VOTES), mapLastMasternodeObject(), lastMNListForVotingKeys(std::make_shared()) { @@ -407,6 +407,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); @@ -1087,13 +1092,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; @@ -1102,18 +1105,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 cca00d8cdb60..5bcc1173a416 100644 --- a/src/governance/governance.h +++ b/src/governance/governance.h @@ -177,6 +177,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; @@ -363,8 +370,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); @@ -415,6 +422,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 44465e4d5bf6..4e43cebf12a7 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}); @@ -262,11 +248,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. Ask this peer first -- 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 93e63e9ac33f..3bbb97d5ecc8 100644 --- a/src/test/governance_inv_tests.cpp +++ b/src/test/governance_inv_tests.cpp @@ -408,7 +408,12 @@ BOOST_AUTO_TEST_CASE(governance_votes_require_peer_announcement_or_request) connman.FlushSendBuffer(*announcing_peer); ProcessGovernanceVote(net_gov, *announcing_peer, vote); - BOOST_CHECK_EQUAL(CountQueuedMessages(*announcing_peer, NetMsgType::MNGOVERNANCESYNC), 1U); + // The parent object is unknown, so the vote is held as an orphan and its parent is fetched via + // the request tracker (asking this peer, which demonstrably has it) rather than by messaging + // every peer directly. + BOOST_CHECK_EQUAL(CountQueuedMessages(*announcing_peer, NetMsgType::MNGOVERNANCESYNC), 0U); + BOOST_CHECK(WITH_LOCK(::cs_main, return m_node.peerman->PeerConsumeObjectRequest( + announcing_peer->GetId(), CInv{MSG_GOVERNANCE_OBJECT, uint256S("31")}))); AssertMisbehaviorScore(*m_node.peerman, *announcing_peer, 0); connman.FlushSendBuffer(*second_announcing_peer); @@ -459,16 +464,82 @@ BOOST_AUTO_TEST_CASE(governance_vote_authorization_survives_unsynced_drop) BOOST_CHECK_EQUAL(CountQueuedMessages(*peer, NetMsgType::MNGOVERNANCESYNC), 0U); // 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. + // registers a tracker request for the missing object. Had the unsynced drop consumed the + // request, the gate would now reject the vote as unrequested and register nothing. m_node.mn_sync->SwitchToNextAsset(); BOOST_REQUIRE(m_node.mn_sync->IsBlockchainSynced()); connman.FlushSendBuffer(*peer); ProcessGovernanceVote(net_gov, *peer, vote); - BOOST_CHECK_EQUAL(CountQueuedMessages(*peer, NetMsgType::MNGOVERNANCESYNC), 1U); + BOOST_CHECK(WITH_LOCK(::cs_main, return m_node.peerman->PeerConsumeObjectRequest( + peer->GetId(), CInv{MSG_GOVERNANCE_OBJECT, uint256S("41")}))); m_node.peerman->FinalizeNode(*peer); chainstate.ResetIbd(); } +// An orphan vote must not turn into traffic aimed at peers that had nothing to do with it. The +// parent fetch goes to the peer that supplied the vote, via the request tracker; a bystander peer +// sees neither a message nor a tracker entry, so N orphans cost O(1) per orphan rather than +// O(peers) per orphan on a timer. +BOOST_AUTO_TEST_CASE(orphan_vote_parent_fetch_does_not_fan_out_to_other_peers) +{ + LOCK(NetEventsInterface::g_msgproc_mutex); + + TestChainState& chainstate = *static_cast(&m_node.chainman->ActiveChainstate()); + chainstate.JumpOutOfIbd(); + + NetGovernance net_gov(m_node.peerman.get(), *m_node.govman, *m_node.mn_sync, *m_node.netfulfilledman, + *m_node.connman); + + auto voting_peer{MakeGovernanceInvPeer(/*id=*/41)}; + auto bystander_peer{MakeGovernanceInvPeer(/*id=*/42)}; + m_node.peerman->InitializeNode(*voting_peer, NODE_NETWORK); + m_node.peerman->InitializeNode(*bystander_peer, NODE_NETWORK); + auto& connman = static_cast(*m_node.connman); + + const uint256 parent_hash{uint256S("51")}; + const CGovernanceVote vote{MakeGovernanceVote(parent_hash)}; + const CInv vote_inv{MSG_GOVERNANCE_OBJECT_VOTE, vote.GetHash()}; + const CInv parent_inv{MSG_GOVERNANCE_OBJECT, parent_hash}; + + ProcessInv(*m_node.peerman, *voting_peer, vote_inv); + connman.FlushSendBuffer(*voting_peer); + connman.FlushSendBuffer(*bystander_peer); + ProcessGovernanceVote(net_gov, *voting_peer, vote); + + // The supplying peer is asked, through the tracker. + BOOST_CHECK(WITH_LOCK(::cs_main, + return m_node.peerman->PeerConsumeObjectRequest(voting_peer->GetId(), parent_inv))); + + // The bystander never announced the vote or the object, so it is neither messaged nor asked. + BOOST_CHECK_EQUAL(CountQueuedMessages(*bystander_peer, NetMsgType::MNGOVERNANCESYNC), 0U); + BOOST_CHECK_EQUAL(CountQueuedInventory(*bystander_peer, parent_inv), 0U); + BOOST_CHECK(!WITH_LOCK(::cs_main, + return m_node.peerman->PeerConsumeObjectRequest(bystander_peer->GetId(), parent_inv))); + + m_node.peerman->FinalizeNode(*voting_peer); + m_node.peerman->FinalizeNode(*bystander_peer); + chainstate.ResetIbd(); +} + +// The orphan cache is filled from the network by any peer, keyed by a parent hash we cannot verify +// until the parent arrives, so its size must be bounded by us and not by the sender. Each retained +// entry costs ~750 bytes (CacheMultiMap stores the value twice, and the vote holds a heap-allocated +// signature), which is why MAX_CACHE_SIZE is the wrong bound for this particular cache. +BOOST_AUTO_TEST_CASE(orphan_vote_cache_is_bounded) +{ + constexpr size_t OVERSHOOT = 50; + + for (size_t i = 0; i < CGovernanceManager::MAX_ORPHAN_VOTES + OVERSHOOT; ++i) { + // Distinct parent hash per vote, so each would occupy its own cache key. + const CGovernanceVote vote{MakeGovernanceVote(uint256S(strprintf("%x", i + 1)))}; + CGovernanceException exception; + uint256 hash_to_request; + BOOST_CHECK(!m_node.govman->ProcessVote(vote, exception, hash_to_request)); + } + + BOOST_CHECK_EQUAL(m_node.govman->GetOrphanVoteCount(), + static_cast(CGovernanceManager::MAX_ORPHAN_VOTES)); +} + BOOST_AUTO_TEST_SUITE_END() From 7bf1403ae363c4e15fc44467c1cee3758ee801da Mon Sep 17 00:00:00 2001 From: pasta Date: Sun, 2 Aug 2026 18:17:55 -0500 Subject: [PATCH 3/4] fix: reassert the orphan-vote bound on load, bound synthetic announcements CacheMultiMap serializes its own capacity. Setting it in the GovernanceStore constructor is therefore undone by Unserialize on any node that has an existing governance.dat, and since the on-disk format is deliberately unchanged those files still load. MAX_ORPHAN_VOTES would have applied to fresh nodes only -- the case that needs it least -- with no visible symptom. Reassert it after reading, and drop the orphans the file carried: they are a ten-minute recovery window the restart already invalidated. Clear() does not touch the capacity, so both calls are needed. AskPeersForObject registered synthetic announcements straight into the tracker, skipping the MAX_PEER_OBJECT_ANNOUNCEMENTS ceiling and overload delay that AddObjectAnnouncement applies to peer-sent ones. That was harmless while only InstantSend called it, but the governance orphan path lets a peer drive it, so apply the same per-peer accounting. Also correct the AskPeersForObject contract: the known-inventory filter is only read for peers that enabled transaction relay, so for other object types prefer_first may be the only candidate. That is intended -- the sweep this replaced also skipped non-relaying peers -- but the previous wording claimed more than the code does. --- src/governance/governance.h | 8 +++++++ src/net_processing.cpp | 11 ++++++++- src/net_processing.h | 11 ++++++--- src/test/governance_inv_tests.cpp | 38 +++++++++++++++++++++++++++++++ 4 files changed, 64 insertions(+), 4 deletions(-) diff --git a/src/governance/governance.h b/src/governance/governance.h index 5bcc1173a416..0edc9b42445b 100644 --- a/src/governance/governance.h +++ b/src/governance/governance.h @@ -239,6 +239,14 @@ class GovernanceStore >> mapObjects >> mapLastMasternodeObject >> *lastMNListForVotingKeys; + + // CacheMultiMap serializes its own capacity, so a file written before MAX_ORPHAN_VOTES + // existed restores the old one and the bound would apply to fresh nodes only. Orphan votes + // are a ten-minute recovery window that the restart has already invalidated, so drop what + // was read and reassert the bound; the field stays in the stream to keep the on-disk format + // unchanged. Clear() does not touch the capacity. + cmmapOrphanVotes.Clear(); + cmmapOrphanVotes.SetMaxSize(MAX_ORPHAN_VOTES); } void Clear() diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 65d52437cafa..a4a11b342d64 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2437,10 +2437,19 @@ void PeerManagerImpl::AskPeersForObject(const CInv& inv, NodeId prefer_first) // 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; + // 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) continue; + 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); - m_object_request.ReceivedInv(peer->m_id, inv, /*preferred=*/true, current_time); + // 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)); } } } diff --git a/src/net_processing.h b/src/net_processing.h index feb7a7caffdb..364e213440c8 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -113,9 +113,14 @@ class PeerManagerInternal virtual void PeerRelayRecoveredSig(const llmq::CRecoveredSig& sig, bool proactive_relay) = 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 peers - * known to have the hash, plus prefer_first if set -- use that for a peer that demonstrably has - * the object without having announced it (e.g. it sent a vote naming this parent object). + * per-peer in-flight limits, expiry, and fallback to the next candidate. + * + * Candidates are prefer_first, 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 -- prefer_first 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. + * * Requires ::cs_main is NOT held. */ virtual void PeerAskPeersForObject(const CInv& inv, NodeId prefer_first = -1) = 0; virtual size_t PeerGetRequestedObjectCount(NodeId nodeid) const = 0; diff --git a/src/test/governance_inv_tests.cpp b/src/test/governance_inv_tests.cpp index 3bbb97d5ecc8..38b79e5e51f1 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 @@ -542,4 +544,40 @@ BOOST_AUTO_TEST_CASE(orphan_vote_cache_is_bounded) static_cast(CGovernanceManager::MAX_ORPHAN_VOTES)); } +// CacheMultiMap serializes its own capacity, so loading a governance.dat written before +// MAX_ORPHAN_VOTES existed would restore the old 1'000'000 and silently un-bound the cache for the +// rest of the run -- leaving the bound in force on fresh nodes only, which is where it is least +// needed. The on-disk format is unchanged, so this has to be reasserted on load rather than avoided +// by a version bump. +BOOST_AUTO_TEST_CASE(orphan_vote_bound_survives_loading_an_old_cache_file) +{ + // Stand in for a pre-existing governance.dat: the same field order GovernanceStore writes, with + // the orphan map carrying the historical capacity and an entry stored under it. The two maps + // whose value types are internal to GovernanceStore are written empty, which serializes as a + // count of zero without naming those types. + CDataStream ss{SER_DISK, CLIENT_VERSION}; + CacheMultiMap legacy_orphans{1'000'000}; + legacy_orphans.Insert(uint256S("61"), + governance::OrphanVote{MakeGovernanceVote(uint256S("61")), NodeSeconds{9999s}}); + ss << std::string{"CGovernanceManager-Version-16"} << std::map{} + << CacheMap{1'000'000} << legacy_orphans + << std::map{} << std::map{} << CDeterministicMNList{}; + + BOOST_REQUIRE_NO_THROW(ss >> *m_node.govman); + + // The file's orphan state is not retained. + BOOST_CHECK_EQUAL(m_node.govman->GetOrphanVoteCount(), 0U); + + // And the bound is ours, not the file's. Without the reassert this holds 1'000'000 and keeps + // every one of the votes below. + for (size_t i = 0; i < CGovernanceManager::MAX_ORPHAN_VOTES + 25; ++i) { + const CGovernanceVote vote{MakeGovernanceVote(uint256S(strprintf("%x", i + 1)))}; + CGovernanceException exception; + uint256 hash_to_request; + BOOST_CHECK(!m_node.govman->ProcessVote(vote, exception, hash_to_request)); + } + BOOST_CHECK_EQUAL(m_node.govman->GetOrphanVoteCount(), + static_cast(CGovernanceManager::MAX_ORPHAN_VOTES)); +} + BOOST_AUTO_TEST_SUITE_END() From 44b965602a2656d32124aa107d646111aa6a1ce9 Mon Sep 17 00:00:00 2001 From: pasta Date: Sun, 2 Aug 2026 18:56:43 -0500 Subject: [PATCH 4/4] fix: request an orphan's parent on every relay, not only the first The parent request was conditional on cmmapOrphanVotes.Insert() returning true. OrphanVote compares by vote, so a second peer relaying a vote we already hold is a duplicate, the insert fails, and hashToRequest stayed null -- that peer never became a candidate for the parent. That condition made sense when the request was a direct PushMessage, where it avoided sending the same peer a redundant message, and it was harmless anyway while the five-minute sweep asked every peer regardless. With the sweep gone and requests routed through the object request tracker, it strands the parent: a peer relays a given vote once, so a duplicate relay is the only evidence we will ever get that this peer has the parent, and if the peer we asked first never delivers there is nothing left to fall back on. Request unconditionally instead. The tracker already dedups per peer, so repeating this for a peer that is already a candidate is a no-op, and the announcement accounting added earlier bounds what a peer can accumulate. --- src/governance/governance.cpp | 9 ++++--- src/test/governance_inv_tests.cpp | 43 +++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 3 deletions(-) diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index bf75c341afec..69e5fb28a4a4 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -829,9 +829,12 @@ bool CGovernanceManager::ProcessVote(const CGovernanceVote& vote, CGovernanceExc std::string msg{strprintf("CGovernanceManager::%s -- Unknown parent object %s, MN outpoint = %s", __func__, nHashGovobj.ToString(), vote.GetMasternodeOutpoint().ToStringShort())}; 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; } diff --git a/src/test/governance_inv_tests.cpp b/src/test/governance_inv_tests.cpp index 38b79e5e51f1..f4426acd4de3 100644 --- a/src/test/governance_inv_tests.cpp +++ b/src/test/governance_inv_tests.cpp @@ -524,6 +524,49 @@ BOOST_AUTO_TEST_CASE(orphan_vote_parent_fetch_does_not_fan_out_to_other_peers) chainstate.ResetIbd(); } +// A peer relays a given vote once, so a second peer sending a vote we already hold is the only +// evidence we will ever get that it has the parent. It has to become a fallback candidate: the peer +// asked first may go away or never answer, and there is no periodic sweep to fall back on. +BOOST_AUTO_TEST_CASE(orphan_vote_relayed_by_a_second_peer_adds_it_as_a_fallback) +{ + LOCK(NetEventsInterface::g_msgproc_mutex); + + TestChainState& chainstate = *static_cast(&m_node.chainman->ActiveChainstate()); + chainstate.JumpOutOfIbd(); + + NetGovernance net_gov(m_node.peerman.get(), *m_node.govman, *m_node.mn_sync, *m_node.netfulfilledman, + *m_node.connman); + + auto first_peer{MakeGovernanceInvPeer(/*id=*/51)}; + auto second_peer{MakeGovernanceInvPeer(/*id=*/52)}; + m_node.peerman->InitializeNode(*first_peer, NODE_NETWORK); + m_node.peerman->InitializeNode(*second_peer, NODE_NETWORK); + + const uint256 parent_hash{uint256S("71")}; + const CGovernanceVote vote{MakeGovernanceVote(parent_hash)}; + const CInv vote_inv{MSG_GOVERNANCE_OBJECT_VOTE, vote.GetHash()}; + const CInv parent_inv{MSG_GOVERNANCE_OBJECT, parent_hash}; + + ProcessInv(*m_node.peerman, *first_peer, vote_inv); + ProcessGovernanceVote(net_gov, *first_peer, vote); + BOOST_CHECK(WITH_LOCK(::cs_main, + return m_node.peerman->PeerConsumeObjectRequest(first_peer->GetId(), parent_inv))); + + // Same vote, different peer. The orphan cache rejects the duplicate, but the request must not be + // suppressed along with it -- the request is for the parent object, not for the vote. + ProcessInv(*m_node.peerman, *second_peer, vote_inv); + ProcessGovernanceVote(net_gov, *second_peer, vote); + BOOST_CHECK(WITH_LOCK(::cs_main, + return m_node.peerman->PeerConsumeObjectRequest(second_peer->GetId(), parent_inv))); + + // The duplicate must still not be double-counted as orphan state. + BOOST_CHECK_EQUAL(m_node.govman->GetOrphanVoteCount(), 1U); + + m_node.peerman->FinalizeNode(*first_peer); + m_node.peerman->FinalizeNode(*second_peer); + chainstate.ResetIbd(); +} + // The orphan cache is filled from the network by any peer, keyed by a parent hash we cannot verify // until the parent arrives, so its size must be bounded by us and not by the sender. Each retained // entry costs ~750 bytes (CacheMultiMap stores the value twice, and the vote holds a heap-allocated