fix: bound governance inv request cache

src/governance/governance.cpp

@@ -24,6 +24,7 @@
 #include <util/time.h>
 #include <validationinterface.h>
 
+#include <algorithm>
 #include <ranges>
 
 const std::string GovernanceStore::SERIALIZATION_VERSION_STRING = "CGovernanceManager-Version-16";
@@ -441,17 +442,11 @@ 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;
-        }
-    }
+    PruneExpiredRequestedHashes(nNow);
     }
 
-    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, request cache size=%d\n", ToString(),
+             m_requested_hashes.GetSize());
 }
 
 std::vector<CInv> CGovernanceManager::FetchRelayInventory()
@@ -595,13 +590,33 @@ 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);
+    const auto nNow = GetTime<std::chrono::seconds>();
+    const auto valid_until = nNow + RELIABLE_PROPAGATION_TIME;
+
+    if (!m_requested_hashes.HasKey(inv.hash)) {
+        // Opportunistically reclaim expired slots before we lean on eviction.
+        if (m_requested_hashes.GetSize() >= governance::MAX_REQUESTED_HASHES && m_requested_hash_time_next_cleanup < nNow) {
+            PruneExpiredRequestedHashes(nNow);
+        }
+
+        // Preserve intake liveness under saturation. Returning false here would
+        // make AlreadyHave suppress all new governance INVs, letting one peer
+        // that keeps the cache full eclipse honest announcements. CacheMap::Insert
+        // instead evicts its oldest (back) entry when full, so a new hash is
+        // always admitted; memory stays bounded by MAX_REQUESTED_HASHES.
+        if (m_requested_hashes.GetSize() >= governance::MAX_REQUESTED_HASHES) {
+            LogPrint(BCLog::GOBJECT, /* Continued */
+                     "CGovernanceManager::ConfirmInventoryRequest request cache full, evicting oldest to admit %s inv hash %s\n",
+                     inv.type == MSG_GOVERNANCE_OBJECT ? "object" : "vote", inv.hash.ToString());
+        }
 
-    if (inserted) {
+        m_requested_hashes.Insert(inv.hash, valid_until);
+        if (valid_until < m_requested_hash_time_next_cleanup) {
+            m_requested_hash_time_next_cleanup = valid_until;
+        }
         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());
+                 "CGovernanceManager::ConfirmInventoryRequest added %s inv hash to request cache, size=%d\n",
+                 inv.type == MSG_GOVERNANCE_OBJECT ? "object" : "vote", m_requested_hashes.GetSize());
     }
 
     LogPrint(BCLog::GOBJECT, "CGovernanceManager::ConfirmInventoryRequest reached end, returning true\n");
@@ -612,7 +627,35 @@ size_t CGovernanceManager::RequestedHashCacheSizeForTesting() const
 {
     AssertLockNotHeld(cs_store);
     LOCK(cs_store);
-    return m_requested_hash_time.size();
+    return m_requested_hashes.GetSize();
+}
+
+size_t CGovernanceManager::RequestedHashCacheMaxSizeForTesting() const
+{
+    return governance::MAX_REQUESTED_HASHES;
+}
+
+void CGovernanceManager::PruneExpiredRequestedHashes(std::chrono::seconds now)
+{
+    AssertLockHeld(cs_store);
+
+    // CacheMap::Insert adds entries at the front and its capacity eviction
+    // removes from the back, so the back remains the oldest inserted request.
+    // Expiration order can differ if the wall clock moves backwards, however,
+    // so inspect every entry rather than stopping at the first unexpired one.
+    const auto& items = m_requested_hashes.GetItemList();
+    auto next_cleanup = std::chrono::seconds::max();
+    for (auto it = items.begin(); it != items.end();) {
+        const auto& item = *it++;
+        if (item.value >= now) {
+            next_cleanup = std::min(next_cleanup, item.value);
+            continue;
+        }
+        // Copy the key before Erase; it aliases the list node being destroyed.
+        const uint256 hash = item.key;
+        m_requested_hashes.Erase(hash);
+    }
+    m_requested_hash_time_next_cleanup = next_cleanup;
 }
 
 std::vector<CInv> CGovernanceManager::GetSyncableVoteInvs(const uint256& nProp, const CBloomFilter& filter) const
@@ -951,13 +994,10 @@ 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);
+    // Only accept one response. Returns false when we never requested this
+    // hash, i.e. the peer sent an unsolicited or already-consumed message.
+    if (!m_requested_hashes.HasKey(nHash)) return false;
+    m_requested_hashes.Erase(nHash);
     return true;
 }
 
@@ -1017,7 +1057,8 @@ void CGovernanceManager::Clear()
     cmapVoteToObject.Clear();
     mapPostponedObjects.clear();
     setAdditionalRelayObjects.clear();
-    m_requested_hash_time.clear();
+    m_requested_hashes.Clear();
+    m_requested_hash_time_next_cleanup = std::chrono::seconds::max();
     fRateChecksEnabled = true;
     m_superblocks.Clear();
 }
@@ -1152,7 +1193,7 @@ void CGovernanceManager::RemoveInvalidVotes()
                 cmapVoteToObject.Erase(voteHash);
                 cmapInvalidVotes.Erase(voteHash);
                 cmmapOrphanVotes.Erase(voteHash);
-                m_requested_hash_time.erase(voteHash);
+                m_requested_hashes.Erase(voteHash);
             }
         }
     }

src/governance/governance.h

@@ -41,6 +41,9 @@ namespace governance {
 class SuperblockManager;
 // How long a requested governance inv hash remains in the request cache.
 inline constexpr std::chrono::seconds RELIABLE_PROPAGATION_TIME{60};
+// Bound pending governance inv request hashes retained before responses arrive
+// or periodic cleanup expires them.
+inline constexpr size_t MAX_REQUESTED_HASHES{50000};
 } // namespace governance
 
 using vote_time_pair_t = std::pair<CGovernanceVote, int64_t>;
@@ -251,7 +254,15 @@ 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;
+    // Governance inv hashes we have requested and are awaiting a response for,
+    // mapped to each hash's expiration time. CacheMap bounds the set at
+    // MAX_REQUESTED_HASHES; when full it evicts its oldest (back) entry on
+    // Insert so a new honest hash is always admitted rather than suppressed
+    // (see ConfirmInventoryRequest). Newest entries sit at the front and oldest
+    // at the back for capacity eviction. Expiry pruning scans all entries because
+    // wall-clock rollback can make expiration order differ from insertion order.
+    CacheMap<uint256, std::chrono::seconds> m_requested_hashes{governance::MAX_REQUESTED_HASHES};
+    std::chrono::seconds m_requested_hash_time_next_cleanup{std::chrono::seconds::max()};
     bool fRateChecksEnabled{true};
 
     mutable Mutex cs_relay;
@@ -303,6 +314,9 @@ class CGovernanceManager : public GovernanceStore
      *  ConfirmInventoryRequest pending expiration in CheckAndRemove. */
     size_t RequestedHashCacheSizeForTesting() const
         EXCLUSIVE_LOCKS_REQUIRED(!cs_store);
+    /** Test-only accessor: maximum inv hashes tracked before dropping new requests. */
+    size_t RequestedHashCacheMaxSizeForTesting() 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)
@@ -404,6 +418,9 @@ class CGovernanceManager : public GovernanceStore
 
     void RemoveInvalidVotes()
         EXCLUSIVE_LOCKS_REQUIRED(cs_store);
+
+    void PruneExpiredRequestedHashes(std::chrono::seconds now)
+        EXCLUSIVE_LOCKS_REQUIRED(cs_store);
 };
 
 #endif // BITCOIN_GOVERNANCE_GOVERNANCE_H

src/governance/net_governance.cpp

@@ -216,7 +216,7 @@ bool NetGovernance::AlreadyHave(const CInv& inv)
     }
     // 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.
+    // fill and continually churn the request cache since CheckAndRemove never runs in that mode.
     if (!m_gov_manager.IsValid()) return true;
     return !m_gov_manager.ConfirmInventoryRequest(inv);
 }

src/init.cpp

@@ -2328,7 +2328,7 @@ 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.
+    // and churn the governance inv request cache without a cleanup task.
     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));
 

src/test/governance_inv_tests.cpp

@@ -19,6 +19,8 @@
 
 #include <test/util/setup_common.h>
 
+#include <arith_uint256.h>
+
 #include <boost/test/unit_test.hpp>
 
 #include <atomic>
@@ -117,6 +119,138 @@ BOOST_AUTO_TEST_CASE(vote_inv_request_expiration)
     CheckInvExpirationCycle(*m_node.govman, CInv{MSG_GOVERNANCE_OBJECT_VOTE, uint256S("02")});
 }
 
+BOOST_AUTO_TEST_CASE(inv_request_cache_prunes_after_clock_rollback)
+{
+    const auto initial_time = GetTime<std::chrono::seconds>();
+    const uint256 older_hash = uint256S("03");
+    const uint256 newer_hash = uint256S("04");
+
+    BOOST_REQUIRE(m_node.govman->ConfirmInventoryRequest(CInv{MSG_GOVERNANCE_OBJECT, older_hash}));
+
+    // A request inserted after a wall-clock rollback expires before the older
+    // insertion, so expiration order no longer matches CacheMap order.
+    SetMockTime(initial_time - 30s);
+    BOOST_REQUIRE(m_node.govman->ConfirmInventoryRequest(CInv{MSG_GOVERNANCE_OBJECT, newer_hash}));
+
+    SetMockTime(initial_time + 31s);
+    m_node.govman->CheckAndRemove();
+
+    BOOST_CHECK_EQUAL(m_node.govman->RequestedHashCacheSizeForTesting(), 1U);
+    BOOST_CHECK(m_node.govman->AcceptMessage(older_hash));
+    BOOST_CHECK(!m_node.govman->AcceptMessage(newer_hash));
+}
+
+BOOST_AUTO_TEST_CASE(inv_request_cache_is_bounded)
+{
+    const size_t max_size = m_node.govman->RequestedHashCacheMaxSizeForTesting();
+    BOOST_REQUIRE_GT(max_size, 0U);
+
+    for (size_t i = 0; i < max_size; ++i) {
+        const auto hash = ArithToUint256(arith_uint256{i + 100});
+        BOOST_REQUIRE(m_node.govman->ConfirmInventoryRequest(CInv{MSG_GOVERNANCE_OBJECT, hash}));
+    }
+    BOOST_CHECK_EQUAL(m_node.govman->RequestedHashCacheSizeForTesting(), max_size);
+
+    // Duplicate hash does not grow the cache.
+    const CInv duplicate_inv{MSG_GOVERNANCE_OBJECT, ArithToUint256(arith_uint256{100})};
+    BOOST_CHECK(m_node.govman->ConfirmInventoryRequest(duplicate_inv));
+    BOOST_CHECK_EQUAL(m_node.govman->RequestedHashCacheSizeForTesting(), max_size);
+
+    // Saturation must not cause AlreadyHave to suppress a new honest hash. The
+    // over-limit hash is admitted (return true) and the oldest tracked hash is
+    // evicted to keep the cache bounded at max_size.
+    const CInv over_limit_inv{MSG_GOVERNANCE_OBJECT, ArithToUint256(arith_uint256{max_size + 100})};
+    BOOST_CHECK(m_node.govman->ConfirmInventoryRequest(over_limit_inv));
+    BOOST_CHECK_EQUAL(m_node.govman->RequestedHashCacheSizeForTesting(), max_size);
+
+    SetMockTime(GetTime<std::chrono::seconds>() + governance::RELIABLE_PROPAGATION_TIME + 1s);
+
+    const CInv after_expiry_inv{MSG_GOVERNANCE_OBJECT, ArithToUint256(arith_uint256{max_size + 101})};
+    BOOST_CHECK(m_node.govman->ConfirmInventoryRequest(after_expiry_inv));
+    BOOST_CHECK_EQUAL(m_node.govman->RequestedHashCacheSizeForTesting(), 1U);
+}
+
+// Guards against an eclipse where one peer saturates the request cache with
+// 50k announcements and AlreadyHave would then suppress every subsequent
+// honest INV. After the cache is full, a new hash must still be admitted
+// (ConfirmInventoryRequest returns true -> AlreadyHave returns false), the
+// oldest tracked hash is evicted, and total memory stays bounded at max_size.
+BOOST_AUTO_TEST_CASE(inv_request_cache_preserves_liveness_under_saturation)
+{
+    const size_t max_size = m_node.govman->RequestedHashCacheMaxSizeForTesting();
+    BOOST_REQUIRE_GT(max_size, 0U);
+
+    // Simulate an attacker saturating the cache with fresh, distinct hashes.
+    const uint256 oldest_hash = ArithToUint256(arith_uint256{1});
+    BOOST_REQUIRE(m_node.govman->ConfirmInventoryRequest(CInv{MSG_GOVERNANCE_OBJECT, oldest_hash}));
+    for (size_t i = 1; i < max_size; ++i) {
+        const auto hash = ArithToUint256(arith_uint256{i + 1});
+        BOOST_REQUIRE(m_node.govman->ConfirmInventoryRequest(CInv{MSG_GOVERNANCE_OBJECT, hash}));
+    }
+    BOOST_REQUIRE_EQUAL(m_node.govman->RequestedHashCacheSizeForTesting(), max_size);
+
+    // Honest peer announces a brand new hash while the cache is saturated: it
+    // must be admitted so we can request it from that peer, and the cache
+    // must not exceed its bound.
+    const CInv honest_inv{MSG_GOVERNANCE_OBJECT, ArithToUint256(arith_uint256{max_size + 1000})};
+    BOOST_CHECK(m_node.govman->ConfirmInventoryRequest(honest_inv));
+    BOOST_CHECK_EQUAL(m_node.govman->RequestedHashCacheSizeForTesting(), max_size);
+
+    // AcceptMessage returns true iff the hash is currently tracked in the
+    // request cache. Use it as an oracle to prove FIFO eviction actually
+    // ran on the oldest entry and left the fresh honest entry intact.
+    BOOST_CHECK(!m_node.govman->AcceptMessage(oldest_hash));
+    BOOST_CHECK_EQUAL(m_node.govman->RequestedHashCacheSizeForTesting(), max_size);
+    BOOST_CHECK(m_node.govman->AcceptMessage(honest_inv.hash));
+    BOOST_CHECK_EQUAL(m_node.govman->RequestedHashCacheSizeForTesting(), max_size - 1);
+
+    // Vote INVs must also remain admissible under object-INV saturation.
+    const CInv honest_vote{MSG_GOVERNANCE_OBJECT_VOTE, ArithToUint256(arith_uint256{max_size + 2000})};
+    BOOST_CHECK(m_node.govman->ConfirmInventoryRequest(honest_vote));
+    BOOST_CHECK(m_node.govman->AcceptMessage(honest_vote.hash));
+}
+
+// Guards eviction ordering across an accept-then-reannounce sequence. A hash is
+// tracked, consumed by AcceptMessage, then re-announced so it becomes one of the
+// newest entries. Because CacheMap keeps its index and ordering list in lockstep
+// on every erase, the re-announced entry sits at the front and a later eviction
+// must drop a truly-oldest entry instead of the freshly re-announced one.
+BOOST_AUTO_TEST_CASE(inv_request_cache_eviction_survives_accept_then_reannounce)
+{
+    const size_t max_size = m_node.govman->RequestedHashCacheMaxSizeForTesting();
+    BOOST_REQUIRE_GT(max_size, 2U);
+
+    // Track a hash; filling the rest below leaves it as the oldest entry.
+    const uint256 reannounced = ArithToUint256(arith_uint256{7});
+    BOOST_REQUIRE(m_node.govman->ConfirmInventoryRequest(CInv{MSG_GOVERNANCE_OBJECT, reannounced}));
+
+    // Fill the rest of the cache so the next new hash triggers eviction.
+    for (size_t i = 1; i < max_size; ++i) {
+        BOOST_REQUIRE(m_node.govman->ConfirmInventoryRequest(CInv{MSG_GOVERNANCE_OBJECT,
+            ArithToUint256(arith_uint256{i + 100})}));
+    }
+    BOOST_REQUIRE_EQUAL(m_node.govman->RequestedHashCacheSizeForTesting(), max_size);
+
+    // AcceptMessage the hash (this is what NetGovernance does after successfully
+    // receiving the object/vote). It removes the entry from index and order
+    // together, so no stale ordering slot can linger.
+    BOOST_REQUIRE(m_node.govman->AcceptMessage(reannounced));
+    BOOST_REQUIRE_EQUAL(m_node.govman->RequestedHashCacheSizeForTesting(), max_size - 1);
+
+    // Re-announcing `reannounced` re-inserts it as one of the newest entries
+    // with a fresh valid_until.
+    BOOST_CHECK(m_node.govman->ConfirmInventoryRequest(CInv{MSG_GOVERNANCE_OBJECT, reannounced}));
+    BOOST_CHECK_EQUAL(m_node.govman->RequestedHashCacheSizeForTesting(), max_size);
+
+    // A brand new hash now forces eviction. The truly-oldest tracked entry must
+    // be evicted, leaving the freshly re-announced entry intact.
+    const CInv new_inv{MSG_GOVERNANCE_OBJECT, ArithToUint256(arith_uint256{max_size + 500})};
+    BOOST_CHECK(m_node.govman->ConfirmInventoryRequest(new_inv));
+    BOOST_CHECK_EQUAL(m_node.govman->RequestedHashCacheSizeForTesting(), max_size);
+    BOOST_CHECK(m_node.govman->AcceptMessage(reannounced));
+    BOOST_CHECK(m_node.govman->AcceptMessage(new_inv.hash));
+}
+
 // 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