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