From 3f5536cae6fab72315ff13c87db61e7b064a6c9f Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Fri, 10 Jul 2026 16:45:18 +0300 Subject: [PATCH 1/2] fix(net): authorize governance inv responses via the per-peer request tracker Governance tracked pending object/vote requests in a global, governance-owned cache (CGovernanceManager::m_requested_hash_time): ConfirmInventoryRequest recorded a hash and AcceptMessage authorized the eventual response, expiring entries after RELIABLE_PROPAGATION_TIME. The cache was unbounded, and a global bound would let a single INV saturate it. The net layer already tracks pending object requests per peer in CNodeState::ObjectDownloadState (m_object_announced / m_object_in_flight), which is bounded per peer by construction, and governance already touches it on receipt. Reuse it as the authorization source and drop the governance-side cache: - Add PeerManager::PeerConsumeObjectRequest(nodeid, inv): consume (erase) the peer's 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). It deliberately does NOT set the global g_already_asked_for / g_erased_object_requests markers, so an unsolicited push from an untracked peer cannot poison request scheduling and suppress a later legitimate fetch. PeerEraseObjectRequest / EraseObjectRequest keep their existing behavior for tx relay and the other subsystems. - SendMessages skips draining a queued request whose per-peer announced/in-flight state was already consumed (or received), so consuming an announcement never triggers a redundant GETDATA and stays O(1) (no scan of m_object_process_time). - Gate MNGOVERNANCEOBJECT / MNGOVERNANCEOBJECTVOTE acceptance on PeerConsumeObjectRequest instead of AcceptMessage. - Remove m_requested_hash_time, AcceptMessage, the ConfirmInventoryRequest cache write, the CheckAndRemove prune, and the obsolete request-cache unit tests. ConfirmInventoryRequest keeps only its "do we already have it?" role for AlreadyHave. Acceptance becomes per-peer: an object/vote is accepted only if the sending peer announced it or we requested it from that peer. In the inv->getdata gossip model senders are always prior announcers, so legitimate flows are unaffected; genuinely unsolicited pushes are now rejected. Co-Authored-By: Claude Opus 4.8 --- src/governance/governance.cpp | 47 +--------- src/governance/governance.h | 7 -- src/governance/net_governance.cpp | 23 +++-- src/init.cpp | 4 +- src/net_processing.cpp | 35 ++++++++ src/net_processing.h | 7 ++ src/test/governance_inv_tests.cpp | 142 ------------------------------ 7 files changed, 63 insertions(+), 202 deletions(-) diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index bc0ccfa4a4ab..ede7ac1dfaef 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -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 CGovernanceManager::FetchRelayInventory() @@ -640,26 +631,12 @@ bool CGovernanceManager::ConfirmInventoryRequest(const CInv& inv) return false; } - const auto valid_until = GetTime() + 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 CGovernanceManager::GetSyncableVoteInvs(const uint256& nProp, const CBloomFilter& filter) const { LOCK(cs_store); @@ -992,20 +969,6 @@ std::pair, std::vector> 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); @@ -1062,7 +1025,6 @@ void CGovernanceManager::Clear() cmapVoteToObject.Clear(); mapPostponedObjects.clear(); setAdditionalRelayObjects.clear(); - m_requested_hash_time.clear(); fRateChecksEnabled = true; m_superblocks.Clear(); } @@ -1197,7 +1159,6 @@ void CGovernanceManager::RemoveInvalidVotes() cmapVoteToObject.Erase(voteHash); cmapInvalidVotes.Erase(voteHash); cmmapOrphanVotes.Erase(voteHash); - m_requested_hash_time.erase(voteHash); } } } diff --git a/src/governance/governance.h b/src/governance/governance.h index fa35b6fe2119..87833b8fee80 100644 --- a/src/governance/governance.h +++ b/src/governance/governance.h @@ -251,7 +251,6 @@ class CGovernanceManager : public GovernanceStore object_ref_cm_t cmapVoteToObject; std::map> mapPostponedObjects; std::set setAdditionalRelayObjects; - std::map m_requested_hash_time; bool fRateChecksEnabled{true}; mutable Mutex cs_relay; @@ -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) @@ -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 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); diff --git a/src/governance/net_governance.cpp b/src/governance/net_governance.cpp index da27cf57b453..6a753a81722b 100644 --- a/src/governance/net_governance.cpp +++ b/src/governance/net_governance.cpp @@ -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; @@ -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; } @@ -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"); @@ -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()); @@ -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); } diff --git a/src/init.cpp b/src/init.cpp index 3884ed3fd7f4..b4db92b13834 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -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(node.peerman.get(), *node.govman, *node.mn_sync, *node.netfulfilledman, *node.connman)); node.peerman->AddExtraHandler(std::make_unique(node.peerman.get(), *node.govman, *node.mn_sync, *node.connman, *node.netfulfilledman)); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 2aabdc199172..ad4d112062a4 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -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); @@ -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); @@ -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; +} + std::chrono::microseconds GetObjectRequestTime(const CInv& inv) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { AssertLockHeld(cs_main); @@ -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); @@ -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); diff --git a/src/net_processing.h b/src/net_processing.h index ff7631b1f2ed..0abe1fcfd6f8 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -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; diff --git a/src/test/governance_inv_tests.cpp b/src/test/governance_inv_tests.cpp index 5da39063e58a..aa71d62350d3 100644 --- a/src/test/governance_inv_tests.cpp +++ b/src/test/governance_inv_tests.cpp @@ -74,39 +74,6 @@ struct GovernanceInvSetup : public TestingSetup { } }; -// Replaces the per-type loop in test/functional/p2p_governance_invs.py: an -// inv hash is recorded by ConfirmInventoryRequest, deduplicated while valid, -// and purged by CheckAndRemove only after the reliable propagation timeout. -void CheckInvExpirationCycle(CGovernanceManager& govman, const CInv& inv) -{ - BOOST_CHECK_EQUAL(govman.RequestedHashCacheSizeForTesting(), 0U); - - // First inv is recorded. - BOOST_CHECK(govman.ConfirmInventoryRequest(inv)); - BOOST_CHECK_EQUAL(govman.RequestedHashCacheSizeForTesting(), 1U); - - // Duplicate inv before expiry does not re-insert. - BOOST_CHECK(govman.ConfirmInventoryRequest(inv)); - BOOST_CHECK_EQUAL(govman.RequestedHashCacheSizeForTesting(), 1U); - - // Cleanup before the reliable propagation timeout must not expire the entry. - govman.CheckAndRemove(); - BOOST_CHECK_EQUAL(govman.RequestedHashCacheSizeForTesting(), 1U); - - // Still recorded -> another inv for the same hash is treated as a duplicate. - BOOST_CHECK(govman.ConfirmInventoryRequest(inv)); - BOOST_CHECK_EQUAL(govman.RequestedHashCacheSizeForTesting(), 1U); - - // Advance past the reliable propagation timeout and clean: the entry is evicted. - SetMockTime(GetTime() + governance::RELIABLE_PROPAGATION_TIME + 1s); - govman.CheckAndRemove(); - BOOST_CHECK_EQUAL(govman.RequestedHashCacheSizeForTesting(), 0U); - - // After eviction the same inv must be accepted and recorded again. - BOOST_CHECK(govman.ConfirmInventoryRequest(inv)); - BOOST_CHECK_EQUAL(govman.RequestedHashCacheSizeForTesting(), 1U); -} - size_t CountQueuedMessages(const CNode& peer, const std::string& msg_type) { LOCK(peer.cs_vSend); @@ -142,115 +109,6 @@ size_t CountQueuedInventory(const CNode& peer, const CInv& expected_inv) BOOST_FIXTURE_TEST_SUITE(governance_inv_tests, GovernanceInvSetup) -BOOST_AUTO_TEST_CASE(object_inv_request_expiration) -{ - CheckInvExpirationCycle(*m_node.govman, CInv{MSG_GOVERNANCE_OBJECT, uint256S("01")}); -} - -BOOST_AUTO_TEST_CASE(vote_inv_request_expiration) -{ - CheckInvExpirationCycle(*m_node.govman, CInv{MSG_GOVERNANCE_OBJECT_VOTE, uint256S("02")}); -} - -// Replaces the end-to-end check the old functional test performed via real P2P: -// a governance INV delivered to PeerManager::ProcessMessage must reach -// CGovernanceManager::ConfirmInventoryRequest through PeerManagerImpl::AlreadyHave -// and the registered NetGovernance handler. Exercising the full inbound INV path -// keeps the wiring from regressing if PeerManager's dispatch ever changes. -BOOST_AUTO_TEST_CASE(peerman_inv_routes_to_governance_request_cache) -{ - LOCK(NetEventsInterface::g_msgproc_mutex); - - in_addr peer_in_addr{}; - peer_in_addr.s_addr = htonl(0x01020304); - CNode peer{/*id=*/0, - /*sock=*/nullptr, - /*addrIn=*/CAddress{CService{peer_in_addr, 8333}, NODE_NETWORK}, - /*nKeyedNetGroupIn=*/0, - /*nLocalHostNonceIn=*/0, - /*addrBindIn=*/CAddress{}, - /*addrNameIn=*/std::string{}, - /*conn_type_in=*/ConnectionType::INBOUND, - /*inbound_onion=*/false}; - peer.nVersion = PROTOCOL_VERSION; - peer.SetCommonVersion(PROTOCOL_VERSION); - m_node.peerman->InitializeNode(peer, NODE_NETWORK); - peer.fSuccessfullyConnected = true; - - auto make_inv_stream = [](const CInv& inv) { - CDataStream s{SER_NETWORK, PROTOCOL_VERSION}; - s << std::vector{inv}; - return s; - }; - - BOOST_CHECK_EQUAL(m_node.govman->RequestedHashCacheSizeForTesting(), 0U); - - const std::atomic interrupt_dummy{false}; - - // Object INV: PeerManager -> AlreadyHave -> NetGovernance -> ConfirmInventoryRequest. - { - const CInv inv{MSG_GOVERNANCE_OBJECT, uint256S("06")}; - auto stream = make_inv_stream(inv); - m_node.peerman->ProcessMessage(peer, NetMsgType::INV, stream, - /*time_received=*/std::chrono::microseconds{0}, - interrupt_dummy); - BOOST_CHECK_EQUAL(m_node.govman->RequestedHashCacheSizeForTesting(), 1U); - - // Duplicate INV with the same hash must not grow the cache. - auto dup_stream = make_inv_stream(inv); - m_node.peerman->ProcessMessage(peer, NetMsgType::INV, dup_stream, - std::chrono::microseconds{0}, interrupt_dummy); - BOOST_CHECK_EQUAL(m_node.govman->RequestedHashCacheSizeForTesting(), 1U); - } - - // Vote INV travels the same path and adds a separate entry. - { - const CInv vote_inv{MSG_GOVERNANCE_OBJECT_VOTE, uint256S("07")}; - auto stream = make_inv_stream(vote_inv); - m_node.peerman->ProcessMessage(peer, NetMsgType::INV, stream, - std::chrono::microseconds{0}, interrupt_dummy); - BOOST_CHECK_EQUAL(m_node.govman->RequestedHashCacheSizeForTesting(), 2U); - } - - m_node.peerman->FinalizeNode(peer); -} - -// Pins the periodic-cleanup wiring the deleted functional test exercised via -// node.mockscheduler: NetGovernance::Schedule queues a task that calls -// CGovernanceManager::CheckAndRemove, so an expired inv request is purged -// without any manual CheckAndRemove call. -BOOST_AUTO_TEST_CASE(net_governance_schedule_drives_check_and_remove) -{ - // NetGovernance::Schedule's periodic callback short-circuits on - // !m_node_sync.IsSynced(); advance from GOVERNANCE to FINISHED. - m_node.mn_sync->SwitchToNextAsset(); - BOOST_REQUIRE(m_node.mn_sync->IsSynced()); - - // Pre-load an entry that has already passed the reliable propagation timeout so - // the very next CheckAndRemove evicts it. - const CInv inv{MSG_GOVERNANCE_OBJECT, uint256S("05")}; - BOOST_REQUIRE(m_node.govman->ConfirmInventoryRequest(inv)); - BOOST_CHECK_EQUAL(m_node.govman->RequestedHashCacheSizeForTesting(), 1U); - SetMockTime(GetTime() + governance::RELIABLE_PROPAGATION_TIME + 1s); - - // Drive a dedicated scheduler so the assertion is independent of - // m_node.scheduler's existing workload. - CScheduler scheduler; - NetGovernance net_gov(m_node.peerman.get(), *m_node.govman, *m_node.mn_sync, - *m_node.netfulfilledman, *m_node.connman); - net_gov.Schedule(scheduler); - std::thread worker([&] { scheduler.serviceQueue(); }); - - // First periodic fire is at +5min; bump the clock so the queue is ready. - scheduler.MockForward(std::chrono::minutes{5}); - - // Queue a stop marker after the mocked-forward tasks; due cleanup runs first. - scheduler.scheduleFromNow([&scheduler] { scheduler.stop(); }, 1ms); - worker.join(); - - BOOST_CHECK_EQUAL(m_node.govman->RequestedHashCacheSizeForTesting(), 0U); -} - BOOST_AUTO_TEST_CASE(per_object_vote_sync_is_fulfilled_request_limited) { LOCK(NetEventsInterface::g_msgproc_mutex); From 0c0ec8c4b74184dd053ba57428acf3755094bea1 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Fri, 10 Jul 2026 16:46:05 +0300 Subject: [PATCH 2/2] test: cover net-layer per-peer governance inv authorization - net_tests: peer_consume_object_request_authorizes_and_erases_per_peer_state exercises PeerConsumeObjectRequest for announced-only and requested invs, that it erases the per-peer entry, that a consumed announcement is not re-requested by SendMessages (drain-side skip, no redundant GETDATA), and the anti-poisoning property (a rejected unsolicited push must not suppress a later legitimate fetch). - governance_inv_tests: object and vote acceptance require the sending peer to have announced the inv (or us to have requested it); an unsolicited peer is rejected without misbehaving, and a second independent announcer's copy is accepted with its own per-peer entry consumed. Co-Authored-By: Claude Opus 4.8 --- src/test/governance_inv_tests.cpp | 221 ++++++++++++++++++++++++++++++ src/test/net_tests.cpp | 85 ++++++++++++ 2 files changed, 306 insertions(+) diff --git a/src/test/governance_inv_tests.cpp b/src/test/governance_inv_tests.cpp index aa71d62350d3..a33fc7a516fb 100644 --- a/src/test/governance_inv_tests.cpp +++ b/src/test/governance_inv_tests.cpp @@ -16,16 +16,19 @@ #include #include #include +#include #include #include #include #include +#include #include #include #include +#include #include #include @@ -105,6 +108,62 @@ size_t CountQueuedInventory(const CNode& peer, const CInv& expected_inv) } return count; } + +std::unique_ptr MakeGovernanceInvPeer(NodeId id) +{ + in_addr peer_in_addr{}; + peer_in_addr.s_addr = htonl(0x01020305 + id); + auto peer{std::make_unique(id, + /*sock=*/nullptr, + /*addrIn=*/CAddress{CService{peer_in_addr, 8333}, NODE_NETWORK}, + /*nKeyedNetGroupIn=*/0, + /*nLocalHostNonceIn=*/0, + /*addrBindIn=*/CAddress{}, + /*addrNameIn=*/std::string{}, + /*conn_type_in=*/ConnectionType::INBOUND, + /*inbound_onion=*/false)}; + peer->nVersion = PROTOCOL_VERSION; + peer->SetCommonVersion(PROTOCOL_VERSION); + peer->fSuccessfullyConnected = true; + return peer; +} + +void ProcessInv(PeerManager& peerman, CNode& peer, const CInv& inv) + EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex) +{ + CDataStream inv_stream{SER_NETWORK, PROTOCOL_VERSION}; + inv_stream << std::vector{inv}; + std::atomic interrupt_dummy{false}; + peerman.ProcessMessage(peer, NetMsgType::INV, inv_stream, GetTime(), interrupt_dummy); +} + +CGovernanceObject MakeGovernanceObject(int64_t creation_time, const uint256& collateral_hash) +{ + const std::string data{ + R"({"type":1,"name":"proposal","start_epoch":1700000000,"end_epoch":1700100000,"payment_amount":1.0,"payment_address":"XwnLY9Tf7Zsef8gMGL2fhWA9ZmMjt4KPwV","url":"https://dash.org"})"}; + return CGovernanceObject{uint256{}, /*revision=*/1, creation_time, collateral_hash, HexStr(data)}; +} + +void ProcessGovernanceObject(NetGovernance& net_gov, CNode& peer, const CGovernanceObject& govobj) +{ + CDataStream object_stream{SER_NETWORK, PROTOCOL_VERSION}; + object_stream << govobj; + net_gov.ProcessMessage(peer, NetMsgType::MNGOVERNANCEOBJECT, object_stream); +} + +CGovernanceVote MakeGovernanceVote(const uint256& parent_hash) +{ + CGovernanceVote vote{COutPoint{uint256S("11"), 1}, parent_hash, VOTE_SIGNAL_FUNDING, VOTE_OUTCOME_YES}; + vote.SetTime(GetTime().count()); + return vote; +} + +void ProcessGovernanceVote(NetGovernance& net_gov, CNode& peer, const CGovernanceVote& vote) +{ + CDataStream vote_stream{SER_NETWORK, PROTOCOL_VERSION}; + vote_stream << vote; + net_gov.ProcessMessage(peer, NetMsgType::MNGOVERNANCEOBJECTVOTE, vote_stream); +} } // namespace BOOST_FIXTURE_TEST_SUITE(governance_inv_tests, GovernanceInvSetup) @@ -272,4 +331,166 @@ BOOST_AUTO_TEST_CASE(per_object_vote_sync_is_fulfilled_request_limited) m_node.peerman->FinalizeNode(peer); } +BOOST_AUTO_TEST_CASE(governance_objects_require_peer_announcement_or_request) +{ + 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 announcing_peer{MakeGovernanceInvPeer(/*id=*/11)}; + auto second_announcing_peer{MakeGovernanceInvPeer(/*id=*/12)}; + auto unsolicited_peer{MakeGovernanceInvPeer(/*id=*/13)}; + m_node.peerman->InitializeNode(*announcing_peer, NODE_NETWORK); + m_node.peerman->InitializeNode(*second_announcing_peer, NODE_NETWORK); + m_node.peerman->InitializeNode(*unsolicited_peer, NODE_NETWORK); + + CNodeStateStats stats; + BOOST_REQUIRE(m_node.peerman->GetNodeStateStats(announcing_peer->GetId(), stats)); + BOOST_CHECK_EQUAL(stats.m_misbehavior_score, 0); + + const CGovernanceObject govobj{MakeGovernanceObject(GetTime().count(), uint256S("21"))}; + const CInv object_inv{MSG_GOVERNANCE_OBJECT, govobj.GetHash()}; + + ProcessInv(*m_node.peerman, *announcing_peer, object_inv); + ProcessInv(*m_node.peerman, *second_announcing_peer, object_inv); + // Pre-seed the object so ProcessObject short-circuits on "already seen" and the + // acceptance gate is exercised in isolation from collateral/chain validation. Both + // ProcessInv calls ran first, so the object was still absent at INV time and both + // peers registered a real pending request in the net-layer tracker. + m_node.govman->AddPostponedObject(govobj); + + // Announcing peer: the gate accepts (it announced the inv) and consumes its per-peer + // request entry. Checking the entry is gone proves the gate actually consulted the + // net-layer tracker, independent of the pre-seed above (a rejected object would leave + // the entry untouched). + ProcessGovernanceObject(net_gov, *announcing_peer, govobj); + BOOST_CHECK( + !WITH_LOCK(::cs_main, return m_node.peerman->PeerConsumeObjectRequest(announcing_peer->GetId(), object_inv))); + BOOST_REQUIRE(m_node.peerman->GetNodeStateStats(announcing_peer->GetId(), stats)); + BOOST_CHECK_EQUAL(stats.m_misbehavior_score, 0); + + ProcessGovernanceObject(net_gov, *second_announcing_peer, govobj); + // Consumption is per-peer: the second announcer's own entry is accepted and consumed, + // independent of the first peer's already-consumed entry. + BOOST_CHECK(!WITH_LOCK(::cs_main, + return m_node.peerman->PeerConsumeObjectRequest(second_announcing_peer->GetId(), object_inv))); + BOOST_REQUIRE(m_node.peerman->GetNodeStateStats(second_announcing_peer->GetId(), stats)); + BOOST_CHECK_EQUAL(stats.m_misbehavior_score, 0); + + const CGovernanceObject unsolicited_govobj{MakeGovernanceObject(GetTime().count() + 1, uint256S("22"))}; + ProcessGovernanceObject(net_gov, *unsolicited_peer, unsolicited_govobj); + BOOST_CHECK(!m_node.govman->HaveObjectForHash(unsolicited_govobj.GetHash())); + BOOST_REQUIRE(m_node.peerman->GetNodeStateStats(unsolicited_peer->GetId(), stats)); + BOOST_CHECK_EQUAL(stats.m_misbehavior_score, 0); + + m_node.peerman->FinalizeNode(*announcing_peer); + m_node.peerman->FinalizeNode(*second_announcing_peer); + m_node.peerman->FinalizeNode(*unsolicited_peer); + chainstate.ResetIbd(); +} + +BOOST_AUTO_TEST_CASE(governance_votes_require_peer_announcement_or_request) +{ + 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 announcing_peer{MakeGovernanceInvPeer(/*id=*/21)}; + auto second_announcing_peer{MakeGovernanceInvPeer(/*id=*/22)}; + auto unsolicited_peer{MakeGovernanceInvPeer(/*id=*/23)}; + m_node.peerman->InitializeNode(*announcing_peer, NODE_NETWORK); + m_node.peerman->InitializeNode(*second_announcing_peer, NODE_NETWORK); + m_node.peerman->InitializeNode(*unsolicited_peer, NODE_NETWORK); + + auto& connman = static_cast(*m_node.connman); + CNodeStateStats stats; + + const CGovernanceVote vote{MakeGovernanceVote(uint256S("31"))}; + const CInv vote_inv{MSG_GOVERNANCE_OBJECT_VOTE, vote.GetHash()}; + + ProcessGovernanceVote(net_gov, *unsolicited_peer, vote); + BOOST_CHECK_EQUAL(CountQueuedMessages(*unsolicited_peer, NetMsgType::MNGOVERNANCESYNC), 0U); + BOOST_REQUIRE(m_node.peerman->GetNodeStateStats(unsolicited_peer->GetId(), stats)); + BOOST_CHECK_EQUAL(stats.m_misbehavior_score, 0); + + ProcessInv(*m_node.peerman, *announcing_peer, vote_inv); + ProcessInv(*m_node.peerman, *second_announcing_peer, vote_inv); + + connman.FlushSendBuffer(*announcing_peer); + ProcessGovernanceVote(net_gov, *announcing_peer, vote); + BOOST_CHECK_EQUAL(CountQueuedMessages(*announcing_peer, NetMsgType::MNGOVERNANCESYNC), 1U); + BOOST_REQUIRE(m_node.peerman->GetNodeStateStats(announcing_peer->GetId(), stats)); + BOOST_CHECK_EQUAL(stats.m_misbehavior_score, 0); + + connman.FlushSendBuffer(*second_announcing_peer); + ProcessGovernanceVote(net_gov, *second_announcing_peer, vote); + // Second announcer: the gate accepts (it independently announced the vote) and consumes + // its per-peer request entry. A rejected vote would return before the gate consumes and + // leave the entry intact, so this proves the accept path independently of the (deduped) + // orphan-request side effect. + BOOST_CHECK(!WITH_LOCK(::cs_main, + return m_node.peerman->PeerConsumeObjectRequest(second_announcing_peer->GetId(), vote_inv))); + BOOST_REQUIRE(m_node.peerman->GetNodeStateStats(second_announcing_peer->GetId(), stats)); + BOOST_CHECK_EQUAL(stats.m_misbehavior_score, 0); + + m_node.peerman->FinalizeNode(*announcing_peer); + m_node.peerman->FinalizeNode(*second_announcing_peer); + m_node.peerman->FinalizeNode(*unsolicited_peer); + chainstate.ResetIbd(); +} + +// A message received while not blockchain-synced is dropped, but the per-peer authorization must +// survive so a retransmit after sync is still accepted (the consume happens after the sync gate). +BOOST_AUTO_TEST_CASE(governance_vote_authorization_survives_unsynced_drop) +{ + 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 peer{MakeGovernanceInvPeer(/*id=*/31)}; + m_node.peerman->InitializeNode(*peer, NODE_NETWORK); + auto& connman = static_cast(*m_node.connman); + + const CGovernanceVote vote{MakeGovernanceVote(uint256S("41"))}; + const CInv vote_inv{MSG_GOVERNANCE_OBJECT_VOTE, vote.GetHash()}; + + // Synced: announce the vote so we hold a per-peer request for it. + BOOST_REQUIRE(m_node.mn_sync->IsBlockchainSynced()); + ProcessInv(*m_node.peerman, *peer, vote_inv); + + // Not synced: delivering the vote is dropped at the sync gate and must NOT consume the request. + m_node.mn_sync->Reset(/*fForce=*/true, /*fNotifyReset=*/false); + BOOST_REQUIRE(!m_node.mn_sync->IsBlockchainSynced()); + connman.FlushSendBuffer(*peer); + ProcessGovernanceVote(net_gov, *peer, vote); + 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. + 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); + + m_node.peerman->FinalizeNode(*peer); + chainstate.ResetIbd(); +} + BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index d0ecb8639d51..47519d8539ce 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -37,6 +37,36 @@ using namespace std::literals; BOOST_FIXTURE_TEST_SUITE(net_tests, RegTestingSetup) +namespace { +std::unique_ptr MakeTestPeer(NodeId id) +{ + in_addr peer_in_addr{}; + peer_in_addr.s_addr = htonl(0x01020304 + id); + auto peer{std::make_unique(id, + /*sock=*/nullptr, + /*addrIn=*/CAddress{CService{peer_in_addr, 8333}, NODE_NETWORK}, + /*nKeyedNetGroupIn=*/0, + /*nLocalHostNonceIn=*/0, + /*addrBindIn=*/CAddress{}, + /*addrNameIn=*/std::string{}, + /*conn_type_in=*/ConnectionType::OUTBOUND_FULL_RELAY, + /*inbound_onion=*/false)}; + peer->nVersion = PROTOCOL_VERSION; + peer->SetCommonVersion(PROTOCOL_VERSION); + peer->fSuccessfullyConnected = true; + return peer; +} + +void ProcessInv(PeerManager& peerman, CNode& peer, const CInv& inv) + EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex) +{ + CDataStream inv_stream{SER_NETWORK, PROTOCOL_VERSION}; + inv_stream << std::vector{inv}; + std::atomic interrupt_dummy{false}; + peerman.ProcessMessage(peer, NetMsgType::INV, inv_stream, GetTime(), interrupt_dummy); +} +} // namespace + BOOST_AUTO_TEST_CASE(cnode_listen_port) { // test default @@ -49,6 +79,61 @@ BOOST_AUTO_TEST_CASE(cnode_listen_port) BOOST_CHECK(port == altPort); } +BOOST_AUTO_TEST_CASE(peer_requested_object_authorizes_and_erases_per_peer_state) +{ + LOCK(NetEventsInterface::g_msgproc_mutex); + + TestChainState& chainstate = + *static_cast(&m_node.chainman->ActiveChainstate()); + chainstate.JumpOutOfIbd(); + + auto peer{MakeTestPeer(/*id=*/0)}; + m_node.peerman->InitializeNode(*peer, NODE_NETWORK); + + const CInv announced_inv{MSG_SPORK, uint256S("01")}; + ProcessInv(*m_node.peerman, *peer, announced_inv); + // The announcement is queued for a GETDATA that hasn't been sent yet. + BOOST_CHECK_EQUAL(WITH_LOCK(::cs_main, return m_node.peerman->GetRequestedObjectCount(peer->GetId())), 1U); + // Consuming clears the peer's announced/in-flight state and returns true exactly once. + BOOST_CHECK(WITH_LOCK(::cs_main, return m_node.peerman->PeerConsumeObjectRequest(peer->GetId(), announced_inv))); + BOOST_CHECK(!WITH_LOCK(::cs_main, return m_node.peerman->PeerConsumeObjectRequest(peer->GetId(), announced_inv))); + // The queued GETDATA is left in place by the consume, but SendMessages must skip it (the + // per-peer request was consumed) rather than re-requesting, and drains the stale entry. + // Since this path never sets g_erased_object_requests, that skip relies on the drain-side + // announced/in-flight check. + SetMockTime(GetTime() + 61s); + m_node.peerman->SendMessages(peer.get()); + BOOST_CHECK_EQUAL(WITH_LOCK(::cs_main, return m_node.peerman->GetRequestedObjectCount(peer->GetId())), 0U); + // Not re-requested: no in-flight entry was created for the consumed announcement. + BOOST_CHECK(!WITH_LOCK(::cs_main, return m_node.peerman->PeerConsumeObjectRequest(peer->GetId(), announced_inv))); + + // Authorization must also survive getdata scheduling. After SendMessages issues the + // GETDATA the inv is in-flight (and still announced); PeerConsumeObjectRequest returns true + // for either state, so this checks that a requested inv authorizes -- not the in-flight + // branch specifically. + const CInv requested_inv{MSG_SPORK, uint256S("02")}; + ProcessInv(*m_node.peerman, *peer, requested_inv); + SetMockTime(GetTime() + 61s); + m_node.peerman->SendMessages(peer.get()); + BOOST_CHECK(WITH_LOCK(::cs_main, return m_node.peerman->PeerConsumeObjectRequest(peer->GetId(), requested_inv))); + BOOST_CHECK(!WITH_LOCK(::cs_main, return m_node.peerman->PeerConsumeObjectRequest(peer->GetId(), requested_inv))); + + const CInv unsolicited_inv{MSG_SPORK, uint256S("03")}; + BOOST_CHECK(!WITH_LOCK(::cs_main, return m_node.peerman->PeerConsumeObjectRequest(peer->GetId(), unsolicited_inv))); + + // A failed per-peer authorization check must not poison the global erased-object marker: + // a later legitimate announcement for the same hash must still be requested (GETDATA + // scheduled) and therefore authorize. + ProcessInv(*m_node.peerman, *peer, unsolicited_inv); + SetMockTime(GetTime() + 61s); + m_node.peerman->SendMessages(peer.get()); + BOOST_CHECK(WITH_LOCK(::cs_main, return m_node.peerman->PeerConsumeObjectRequest(peer->GetId(), unsolicited_inv))); + + m_node.peerman->FinalizeNode(*peer); + chainstate.ResetIbd(); + SetMockTime(0s); +} + BOOST_AUTO_TEST_CASE(cnode_simple_test) { NodeId id = 0;