From 93667f0ca4c6c5d095af97e823ea1362fd3a4c11 Mon Sep 17 00:00:00 2001 From: PastaClaw Date: Fri, 10 Jul 2026 11:15:46 -0500 Subject: [PATCH] fix: bound governance inv request cache Bound pending governance inventory requests at the protocol INV limit while preserving intake liveness by evicting the oldest request when saturated. Keep expiry cleanup correct across wall-clock rollback by scanning all cached entries, and cover capacity, saturation, accept/reannounce ordering, rollback expiry, and real INV routing with unit tests. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/governance/governance.cpp | 89 ++++++++++++++------ src/governance/governance.h | 19 ++++- src/governance/net_governance.cpp | 2 +- src/init.cpp | 2 +- src/test/governance_inv_tests.cpp | 134 ++++++++++++++++++++++++++++++ 5 files changed, 219 insertions(+), 27 deletions(-) diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index ec4419bce7b5..4cea2d506eff 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -24,6 +24,7 @@ #include #include +#include #include 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 CGovernanceManager::FetchRelayInventory() @@ -595,13 +590,33 @@ 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); + const auto nNow = GetTime(); + 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 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); } } } diff --git a/src/governance/governance.h b/src/governance/governance.h index dff378c20d47..2a3b98a655c8 100644 --- a/src/governance/governance.h +++ b/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; @@ -251,7 +254,15 @@ class CGovernanceManager : public GovernanceStore object_ref_cm_t cmapVoteToObject; std::map> mapPostponedObjects; std::set setAdditionalRelayObjects; - std::map 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 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 diff --git a/src/governance/net_governance.cpp b/src/governance/net_governance.cpp index 97e21b9025a8..cd8dec72f78f 100644 --- a/src/governance/net_governance.cpp +++ b/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); } diff --git a/src/init.cpp b/src/init.cpp index 3884ed3fd7f4..fc2969cce6fb 100644 --- a/src/init.cpp +++ b/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(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/test/governance_inv_tests.cpp b/src/test/governance_inv_tests.cpp index 9a5c06561199..2657eb0e3f49 100644 --- a/src/test/governance_inv_tests.cpp +++ b/src/test/governance_inv_tests.cpp @@ -19,6 +19,8 @@ #include +#include + #include #include @@ -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(); + 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() + 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