fix: fetch orphan-vote parents via the request tracker instead of broadcasting #7526
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,23 +45,9 @@ void NetGovernance::Schedule(CScheduler& scheduler) | |
| [this]() -> void { | ||
| if (!m_node_sync.IsSynced()) return; | ||
|
|
||
| // 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.IsNull()) { | ||
|
There was a problem hiding this comment.
When the same orphan vote is later received from a second peer—particularly after the first peer's parent request timed out— AGENTS.md reference: AGENTS.md:L165-L175 Useful? React with 👍 / 👎. |
||
| // 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()); | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 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 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,43 +2393,54 @@ void PeerManagerImpl::SendPings() | |
| for(auto& it : m_peer_map) it.second->m_ping_queued = true; | ||
| } | ||
|
|
||
| void PeerManagerImpl::AskPeersForObject(const CInv& inv, NodeId prefer_first) | ||
| { | ||
| std::vector<PeerRef> peersToAsk; | ||
| 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() >= MAX_PEERS_TO_ASK_FOR_OBJECT) { | ||
| break; | ||
| } | ||
| if (peer->m_id == prefer_first) { | ||
| continue; | ||
| } | ||
| if (IsInvInFilter(*peer, inv.hash)) { | ||
| peersToAsk.emplace_back(peer); | ||
| } | ||
| } | ||
| } | ||
| { | ||
| LOCK(cs_main); | ||
| const auto current_time{GetTime<std::chrono::microseconds>()}; | ||
| // 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; | ||
| 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); | ||
|
There was a problem hiding this comment.
When a peer alternates an authorized vote AGENTS.md reference: AGENTS.md:L165-L175 Useful? React with 👍 / 👎. |
||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+2396
to
2455
There was a problem hiding this comment. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Description: Inspect IsInvInFilter's implementation to determine whether it is
# scoped to the tx-specific known-inventory filter or generalizes across CInv types.
ast-grep run --pattern 'bool PeerManagerImpl::IsInvInFilter($$$) {
$$$
}' --lang cpp src/net_processing.cpp
rg -n -B2 -A15 'bool PeerManagerImpl::IsInvInFilter' src/net_processing.cppRepository: dashpay/dash Length of output: 761 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== declarations =="
rg -n -B3 -A5 'IsInvInFilter|AskPeersForObject|PeerAskPeersForObject|m_object_request.*RequestedInv|m_object_request.*ReceivedInv|m_tx_inventory_known_filter|TxRelay' src/net_processing.cpp src/net_processing.h src/net.h src/net.h-common.h 2>/dev/null | head -n 240
echo
echo "== call sites with surrounding context =="
rg -n -B6 -A8 'AskPeersForObject\(|PeerAskPeersForObject\(' src test -g '*.cpp' -g '*.h' -g '*.c' -g '*.cc' | head -n 240
echo
echo "== CInv enum usage relevant to object inventory =="
rg -n 'MSG_[A-Z_]+|SendTransaction|PushInventory|Inventory|TxFrom|TxTo|m_tx_inventory_known_filter\.insert|\.add\(' src/net_processing.cpp src/validation.cpp src/policy | sed -n '1,220p'Repository: dashpay/dash Length of output: 15683 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== declaration and relevant declarations =="
rg -n -B4 -A8 'IsInvInFilter|AskPeersForObject|PeerAskPeersForObject|m_object_request|TxRelay|m_tx_inventory_known_filter' src/net_processing.cpp src/net_processing.h src/net.h src/net.h-common.h 2>/dev/null | sed -n '1,260p'Repository: dashpay/dash Length of output: 19362 Scope
🤖 Prompt for AI Agents |
||
|
|
@@ -6782,9 +6799,9 @@ void PeerManagerImpl::PeerRelayTransaction(const uint256& txid) | |
| RelayTransaction(txid); | ||
| } | ||
|
|
||
| void PeerManagerImpl::PeerAskPeersForObject(const CInv& inv, NodeId prefer_first) | ||
| { | ||
| AskPeersForObject(inv, prefer_first); | ||
| } | ||
|
|
||
| size_t PeerManagerImpl::PeerGetRequestedObjectCount(NodeId nodeid) const | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On upgrades that load an existing
governance.dat, this constructor limit is overwritten whenCacheMultiMap::Unserializerestores its serializednMaxSize. Because the serialization version remainsCGovernanceManager-Version-16, existing files contain the old 1,000,000-entry limit, so nearly every upgraded node continues accepting that many orphan votes despite this change. EnforceMAX_ORPHAN_VOTESafter loading, including pruning any excess retained entries, rather than relying only on the constructor.AGENTS.md reference: AGENTS.md:L166-L175
Useful? React with 👍 / 👎.