Skip to content

fix(net): authorize governance inv responses via the net-layer per-peer request tracker #7442

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
UdjinM6 wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

fix(net): authorize governance inv responses via the net-layer per-peer request tracker #7442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3f5536c
fix(net): authorize governance inv responses via the per-peer request…
UdjinM6 Jul 10, 2026
0c0ec8c
test: cover net-layer per-peer governance inv authorization
UdjinM6 Jul 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 4 additions & 43 deletions src/governance/governance.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,18 +485,9 @@ void CGovernanceManager::CheckAndRemove()
}
}

// forget about expired requests
for (auto r_it = m_requested_hash_time.begin(); r_it != m_requested_hash_time.end();) {
if (r_it->second < nNow) {
m_requested_hash_time.erase(r_it++);
} else {
++r_it;
}
}
}

LogPrint(BCLog::GOBJECT, "CGovernanceManager::UpdateCachesAndClean -- %s, m_requested_hash_time size=%d\n",
ToString(), m_requested_hash_time.size());
LogPrint(BCLog::GOBJECT, "CGovernanceManager::UpdateCachesAndClean -- %s\n", ToString());
}

std::vector<CInv> CGovernanceManager::FetchRelayInventory()
Expand Down Expand Up @@ -640,26 +631,12 @@ bool CGovernanceManager::ConfirmInventoryRequest(const CInv& inv)
return false;
}

const auto valid_until = GetTime<std::chrono::seconds>() + RELIABLE_PROPAGATION_TIME;
const auto& [_itr, inserted] = m_requested_hash_time.emplace(inv.hash, valid_until);

if (inserted) {
LogPrint(BCLog::GOBJECT, /* Continued */
"CGovernanceManager::ConfirmInventoryRequest added %s inv hash to m_requested_hash_time, size=%d\n",
inv.type == MSG_GOVERNANCE_OBJECT ? "object" : "vote", m_requested_hash_time.size());
}

LogPrint(BCLog::GOBJECT, "CGovernanceManager::ConfirmInventoryRequest reached end, returning true\n");
// We don't have it and it's a known type: signal that we want to fetch it.
// The net-layer per-peer request tracker records the pending request; acceptance
// of the eventual response is gated on that tracker (see NetGovernance::ProcessMessage).
return true;
}

size_t CGovernanceManager::RequestedHashCacheSizeForTesting() const
{
AssertLockNotHeld(cs_store);
LOCK(cs_store);
return m_requested_hash_time.size();
}

std::vector<CInv> CGovernanceManager::GetSyncableVoteInvs(const uint256& nProp, const CBloomFilter& filter) const
{
LOCK(cs_store);
Expand Down Expand Up @@ -992,20 +969,6 @@ std::pair<std::vector<uint256>, std::vector<uint256>> CGovernanceManager::FetchG
return {vTriggerObjHashes, vOtherObjHashes};
}

bool CGovernanceManager::AcceptMessage(const uint256& nHash)
{
AssertLockNotHeld(cs_store);
LOCK(cs_store);
auto it = m_requested_hash_time.find(nHash);
if (it == m_requested_hash_time.end()) {
// We never requested this
return false;
}
// Only accept one response
m_requested_hash_time.erase(it);
return true;
}

void CGovernanceManager::RebuildIndexes()
{
AssertLockHeld(cs_store);
Expand Down Expand Up @@ -1062,7 +1025,6 @@ void CGovernanceManager::Clear()
cmapVoteToObject.Clear();
mapPostponedObjects.clear();
setAdditionalRelayObjects.clear();
m_requested_hash_time.clear();
fRateChecksEnabled = true;
m_superblocks.Clear();
}
Expand Down Expand Up @@ -1197,7 +1159,6 @@ void CGovernanceManager::RemoveInvalidVotes()
cmapVoteToObject.Erase(voteHash);
cmapInvalidVotes.Erase(voteHash);
cmmapOrphanVotes.Erase(voteHash);
m_requested_hash_time.erase(voteHash);
}
}
}
Expand Down
7 changes: 0 additions & 7 deletions src/governance/governance.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,6 @@ class CGovernanceManager : public GovernanceStore
object_ref_cm_t cmapVoteToObject;
std::map<uint256, std::shared_ptr<CGovernanceObject>> mapPostponedObjects;
std::set<uint256> setAdditionalRelayObjects;
std::map<uint256, std::chrono::seconds> m_requested_hash_time;
bool fRateChecksEnabled{true};

mutable Mutex cs_relay;
Expand Down Expand Up @@ -299,10 +298,6 @@ class CGovernanceManager : public GovernanceStore
*/
bool ConfirmInventoryRequest(const CInv& inv)
EXCLUSIVE_LOCKS_REQUIRED(!cs_store);
/** Test-only accessor: number of inv hashes currently tracked by
* ConfirmInventoryRequest pending expiration in CheckAndRemove. */
size_t RequestedHashCacheSizeForTesting() const
EXCLUSIVE_LOCKS_REQUIRED(!cs_store);
bool ProcessVoteAndRelay(const CGovernanceVote& vote, CGovernanceException& exception, CConnman& connman)
EXCLUSIVE_LOCKS_REQUIRED(!cs_store, !cs_relay);
void RelayObject(const CGovernanceObject& obj)
Expand Down Expand Up @@ -365,8 +360,6 @@ class CGovernanceManager : public GovernanceStore
/** Returns inventory items for syncable votes on a specific object, filtered by bloom filter */
[[nodiscard]] std::vector<CInv> GetSyncableVoteInvs(const uint256& nProp, const CBloomFilter& filter) const
EXCLUSIVE_LOCKS_REQUIRED(!cs_store);
/// Called to indicate a requested object or vote has been received
bool AcceptMessage(const uint256& nHash) EXCLUSIVE_LOCKS_REQUIRED(!cs_store);
bool ProcessObject(const std::string& peer_str, const uint256& hash, CGovernanceObject& govobj)
EXCLUSIVE_LOCKS_REQUIRED(::cs_main, !cs_store, !cs_relay);

Expand Down
23 changes: 15 additions & 8 deletions src/governance/net_governance.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,8 +168,6 @@ void NetGovernance::ProcessMessage(CNode& peer, const std::string& msg_type, CDa

uint256 nHash = govobj.GetHash();

WITH_LOCK(::cs_main, m_peer_manager->PeerEraseObjectRequest(peer.GetId(), CInv{MSG_GOVERNANCE_OBJECT, nHash}));

if (!m_node_sync.IsBlockchainSynced()) {
LogPrint(BCLog::GOBJECT, "MNGOVERNANCEOBJECT -- masternode list not synced\n");
return;
Expand All @@ -179,7 +177,13 @@ void NetGovernance::ProcessMessage(CNode& peer, const std::string& msg_type, CDa

LogPrint(BCLog::GOBJECT, "MNGOVERNANCEOBJECT -- Received object: %s\n", strHash);

if (!m_gov_manager.AcceptMessage(nHash)) {
// Only accept an object if this peer announced it or we requested it from this peer. The
// net-layer per-peer request tracker is the authorization source (already bounded), so no
// separate governance-side request cache is needed. Consume only after the sync gate, so a
// message dropped while not synced does not burn the authorization for a later retransmit.
const bool announced_or_requested = WITH_LOCK(
::cs_main, return m_peer_manager->PeerConsumeObjectRequest(peer.GetId(), CInv{MSG_GOVERNANCE_OBJECT, nHash}));
if (!announced_or_requested) {
LogPrint(BCLog::GOBJECT, "MNGOVERNANCEOBJECT -- Received unrequested object: %s\n", strHash);
return;
}
Expand All @@ -197,8 +201,6 @@ void NetGovernance::ProcessMessage(CNode& peer, const std::string& msg_type, CDa

uint256 nHash = vote.GetHash();

WITH_LOCK(::cs_main, m_peer_manager->PeerEraseObjectRequest(peer.GetId(), CInv{MSG_GOVERNANCE_OBJECT_VOTE, nHash}));

// Ignore such messages until masternode list is synced
if (!m_node_sync.IsBlockchainSynced()) {
LogPrint(BCLog::GOBJECT, "MNGOVERNANCEOBJECTVOTE -- masternode list not synced\n");
Expand All @@ -210,7 +212,13 @@ void NetGovernance::ProcessMessage(CNode& peer, const std::string& msg_type, CDa

std::string strHash = nHash.ToString();

if (!m_gov_manager.AcceptMessage(nHash)) {
// Only accept a vote if this peer announced it or we requested it from this peer. Consume
// after the sync gate (see MNGOVERNANCEOBJECT above) so a vote dropped while not synced does
// not burn the authorization for a later retransmit.
const bool announced_or_requested = WITH_LOCK(
::cs_main,
return m_peer_manager->PeerConsumeObjectRequest(peer.GetId(), CInv{MSG_GOVERNANCE_OBJECT_VOTE, nHash}));
if (!announced_or_requested) {
LogPrint(BCLog::GOBJECT, /* Continued */
"MNGOVERNANCEOBJECTVOTE -- Received unrequested vote object: %s, hash: %s, peer = %d\n",
vote.ToString(tip_mn_list), strHash, peer.GetId());
Expand Down Expand Up @@ -255,8 +263,7 @@ bool NetGovernance::AlreadyHave(const CInv& inv)
return false;
}
// When governance isn't loaded (e.g. -disablegovernance), claim we already have
// the item so we don't fetch or track it. ConfirmInventoryRequest would otherwise
// grow m_requested_hash_time unbounded since CheckAndRemove never runs in that mode.
// the item so we don't fetch or track it in the net-layer request tracker.
if (!m_gov_manager.IsValid()) return true;
return !m_gov_manager.ConfirmInventoryRequest(inv);
}
Expand Down
4 changes: 2 additions & 2 deletions src/init.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2327,8 +2327,8 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
}
// Always register NetGovernance so it can suppress governance inv items in AlreadyHave()
// even when -disablegovernance is set. The handler's ProcessMessage/Schedule paths
// early-return on !IsValid(), and AlreadyHave() short-circuits to true so we don't grow
// m_requested_hash_time without a cleanup task.
// early-return on !IsValid(), and AlreadyHave() short-circuits to true so we don't
// track governance inventory that cannot be processed.
node.peerman->AddExtraHandler(std::make_unique<NetGovernance>(node.peerman.get(), *node.govman, *node.mn_sync, *node.netfulfilledman, *node.connman));
node.peerman->AddExtraHandler(std::make_unique<SyncManager>(node.peerman.get(), *node.govman, *node.mn_sync, *node.connman, *node.netfulfilledman));

Expand Down
35 changes: 35 additions & 0 deletions src/net_processing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,6 +654,7 @@ class PeerManagerImpl final : public PeerManager
void PeerMisbehaving(const NodeId pnode, const int howmuch, const std::string& message = "") override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
bool PeerIsBanned(const NodeId node_id) override EXCLUSIVE_LOCKS_REQUIRED(cs_main, !m_peer_mutex);
void PeerEraseObjectRequest(const NodeId nodeid, const CInv& inv) override EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
bool PeerConsumeObjectRequest(NodeId nodeid, const CInv& inv) override EXCLUSIVE_LOCKS_REQUIRED(::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);
Expand Down Expand Up @@ -681,6 +682,7 @@ class PeerManagerImpl final : public PeerManager
void RelayInvFiltered(const CInv& inv, const uint256& relatedTxHash) EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);

void EraseObjectRequest(NodeId nodeid, const CInv& inv) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
bool ConsumeObjectRequest(NodeId nodeid, const CInv& inv) EXCLUSIVE_LOCKS_REQUIRED(::cs_main);

void RequestObject(NodeId nodeid, const CInv& inv, std::chrono::microseconds current_time, bool fForce = false)
EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
Expand Down Expand Up @@ -1587,6 +1589,26 @@ void PeerManagerImpl::EraseObjectRequest(NodeId nodeid, const CInv& inv)
state->m_object_download.m_object_in_flight.erase(inv);
}

bool PeerManagerImpl::ConsumeObjectRequest(NodeId nodeid, const CInv& inv)
{
AssertLockHeld(cs_main);

CNodeState* state = State(nodeid);
if (state == nullptr)
return false;

LogPrint(BCLog::NET, "%s -- inv=(%s)\n", __func__, inv.ToString());
auto& object_download = state->m_object_download;
const bool announced = object_download.m_object_announced.erase(inv) != 0;
const bool in_flight = object_download.m_object_in_flight.erase(inv) != 0;
// Any leftover m_object_process_time entry for this inv is left in place (removing it here
// would be an O(n) scan). The SendMessages drain skips queued entries whose per-peer
// announced/in-flight state was already consumed, so no redundant GETDATA is issued -- and we
// deliberately do not set the global g_erased_object_requests marker (avoids poisoning it via
// an unsolicited push).
return announced || in_flight;
}
Comment on lines +1592 to +1610

@thepastaclaw thepastaclaw Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: Squash 'fix: apply review suggestions' into the commits it corrects

Commit c89ec52 ('fix: apply review suggestions') is a review-follow-up whose entire diff corrects code introduced earlier in this same PR, none of which has ever shipped to develop:

  • It renames PeerRequestedObject -> PeerConsumeObjectRequest and RequestedObject -> ConsumeObjectRequest, symbols introduced in commit 94d9769 (this PR).
  • It fixes a real bug in ConsumeObjectRequest (also erase the queued m_object_process_time entry so SendMessages does not issue a redundant GETDATA), in a function likewise introduced in 94d9769.
  • It updates net_tests.cpp / governance_inv_tests.cpp, both added in commit 3dd5d72 (this PR).

Dash merges without squashing, so a generic 'fix: apply review suggestions' commit lands permanently and becomes bisect/blame noise: git blame on the renamed function and the process_time fix would point here instead of at the commit that introduced the logic, and the subject only makes sense alongside the now-gone review conversation. Per the project commit-hygiene guidance (atomic commits, each building and making sense on its own), fold the net_processing.cpp/.h and net_governance.cpp changes into 94d9769 and the test changes into 3dd5d72 (e.g. git rebase -i --autosquash after marking them fixup!). Non-blocking, but the stack should be cleaned before merge.

source: ['claude']


std::chrono::microseconds GetObjectRequestTime(const CInv& inv) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
{
AssertLockHeld(cs_main);
Expand Down Expand Up @@ -6669,6 +6691,14 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
// Erase this entry from object_process_time (it may be added back for
// processing at a later time, see below)
object_process_time.erase(object_process_time.begin());
// Drop stale entries whose per-peer request was already consumed or received (no
// longer announced or in-flight), so a consumed announcement is not re-requested.
// This covers ConsumeObjectRequest, which erases that state without setting the
// global g_erased_object_requests marker checked below.
if (state.m_object_download.m_object_announced.count(inv) == 0 &&
state.m_object_download.m_object_in_flight.count(inv) == 0) {
continue;
}
if (g_erased_object_requests.count(inv.hash)) {
LogPrint(BCLog::NET, "%s -- GETDATA skipping inv=(%s), peer=%d\n", __func__, inv.ToString(), pto->GetId());
state.m_object_download.m_object_announced.erase(inv);
Expand Down Expand Up @@ -6730,6 +6760,11 @@ void PeerManagerImpl::PeerEraseObjectRequest(const NodeId nodeid, const CInv& in
EraseObjectRequest(nodeid, inv);
}

bool PeerManagerImpl::PeerConsumeObjectRequest(NodeId nodeid, const CInv& inv)
{
return ConsumeObjectRequest(nodeid, inv);
}

void PeerManagerImpl::PeerPushInventory(NodeId nodeid, const CInv& inv)
{
PushInventory(nodeid, inv);
Expand Down
7 changes: 7 additions & 0 deletions src/net_processing.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,14 @@ class PeerManagerInternal
public:
virtual void PeerMisbehaving(const NodeId pnode, const int howmuch, const std::string& message = "") = 0;
virtual bool PeerIsBanned(const NodeId node_id) = 0;
/** Erase a pending object request for a peer and update global request tracking. */
virtual void PeerEraseObjectRequest(const NodeId nodeid, const CInv& inv) = 0;
/** Consume this peer's pending request for the inv -- erase the matching per-peer announced /
* in-flight state -- and return whether such a request existed (the peer announced the inv, or
* we requested it from the peer). Any queued m_object_process_time entry is left in place; the
* SendMessages drain skips it once the announced/in-flight state is gone.
* Requires ::cs_main (see the PeerManagerImpl override). */
virtual bool PeerConsumeObjectRequest(NodeId nodeid, 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;
Expand Down
Loading
Loading