diff --git a/src/cachemultimap.h b/src/cachemultimap.h index 88f3f12c2bea..70f80e5e8f0b 100644 --- a/src/cachemultimap.h +++ b/src/cachemultimap.h @@ -110,6 +110,16 @@ class CacheMultiMap return (mapIndex.find(key) != mapIndex.end()); } + const V* Find(const K& key, const V& value) const + { + map_cit mit = mapIndex.find(key); + if (mit == mapIndex.end()) { + return nullptr; + } + it_map_cit it = mit->second.find(value); + return it == mit->second.end() ? nullptr : &it->second->value; + } + bool Get(const K& key, V& value) const { map_cit it = mapIndex.find(key); diff --git a/src/governance/governance.cpp b/src/governance/governance.cpp index fb4f26ddf1e9..9ab887cb827d 100644 --- a/src/governance/governance.cpp +++ b/src/governance/governance.cpp @@ -62,11 +62,72 @@ class ScopedLockBool }; } // anonymous namespace +namespace governance { +OrphanVoteCache::OrphanVoteCache(size_t max_total, size_t max_per_mn) : + m_max_per_mn{max_per_mn}, + m_cache{static_cast::size_type>(max_total)} +{ + assert(max_total > 0); +} + +bool OrphanVoteCache::Insert(const uint256& parent_hash, const OrphanVote& orphan_vote) +{ + if (m_cache.Find(parent_hash, orphan_vote) != nullptr) return true; + + const COutPoint& outpoint{orphan_vote.vote.GetMasternodeOutpoint()}; + if (const auto it{m_count_per_mn.find(outpoint)}; it != m_count_per_mn.end() && it->second >= m_max_per_mn) { + return false; + } + // Evict before m_cache would prune by itself, so the evicted vote's masternode gets its share back. + if (m_cache.GetSize() >= m_cache.GetMaxSize()) { + const auto& oldest{m_cache.GetItemList().back()}; + Erase(oldest.key, oldest.value); + } + m_cache.Insert(parent_hash, orphan_vote); + ++m_count_per_mn[outpoint]; + return true; +} + +void OrphanVoteCache::Erase(const uint256& parent_hash, const OrphanVote& orphan_vote) +{ + const OrphanVote* stored{m_cache.Find(parent_hash, orphan_vote)}; + if (stored == nullptr) return; + // Release the share of the stored entry, which the argument only has to compare equal to. Read it + // first: erasing destroys it. + const COutPoint outpoint{stored->vote.GetMasternodeOutpoint()}; + m_cache.Erase(parent_hash, orphan_vote); + + const auto it{m_count_per_mn.find(outpoint)}; + if (!Assume(it != m_count_per_mn.end())) return; + if (--it->second == 0) m_count_per_mn.erase(it); +} + +void OrphanVoteCache::EraseExpired(NodeSeconds now) +{ + const auto& items{m_cache.GetItemList()}; + for (auto it = items.begin(); it != items.end();) { + const auto& item{*it++}; + if (item.value.expiration < now) Erase(item.key, item.value); + } +} + +void OrphanVoteCache::GetAll(const uint256& parent_hash, std::vector& orphan_votes) +{ + m_cache.GetAll(parent_hash, orphan_votes); +} + +void OrphanVoteCache::Clear() +{ + m_cache.Clear(); + m_count_per_mn.clear(); +} +} // namespace governance + GovernanceStore::GovernanceStore() : cs_store(), mapObjects(), mapErasedGovernanceObjects(), - cmmapOrphanVotes(MAX_CACHE_SIZE), + m_orphan_votes(governance::MAX_ORPHAN_VOTES, governance::MAX_ORPHAN_VOTES_PER_MN), mapLastMasternodeObject(), lastMNListForVotingKeys(std::make_shared()) { @@ -282,7 +343,7 @@ void CGovernanceManager::CheckOrphanVotes(CGovernanceObject& govobj) uint256 nHash = govobj.GetHash(); std::vector orphan_votes; - cmmapOrphanVotes.GetAll(nHash, orphan_votes); + m_orphan_votes.GetAll(nHash, orphan_votes); ScopedLockBool guard(cs_store, fRateChecksEnabled, false); @@ -299,7 +360,7 @@ void CGovernanceManager::CheckOrphanVotes(CGovernanceObject& govobj) fRemove = true; } if (fRemove) { - cmmapOrphanVotes.Erase(nHash, orphan_vote); + m_orphan_votes.Erase(nHash, orphan_vote); } } } @@ -389,14 +450,6 @@ void CGovernanceManager::AddGovernanceObjectForTesting(const CGovernanceObject& mapObjects.emplace(govobj.GetHash(), std::make_shared(govobj)); } -void CGovernanceManager::AddOrphanVoteForTesting(const CGovernanceVote& vote) -{ - AssertLockNotHeld(cs_store); - LOCK(cs_store); - cmmapOrphanVotes.Insert(vote.GetParentHash(), - governance::OrphanVote{vote, Now() + GOVERNANCE_ORPHAN_EXPIRATION_TIME}); -} - void CGovernanceManager::CheckAndRemove() { AssertLockNotHeld(cs_store); @@ -414,6 +467,8 @@ void CGovernanceManager::CheckAndRemove() ScopedLockBool guard(cs_store, fRateChecksEnabled, false); + m_orphan_votes.EraseExpired(Now()); + // Clean up any expired or invalid triggers m_superblocks.Clean(nCachedBlockHeight); @@ -845,8 +900,13 @@ bool CGovernanceManager::ProcessVote(const CGovernanceVote& vote, CGovernanceExc // No penalty: the vote is signed by a masternode, it just arrived before its parent object, // which routinely happens during governance sync. Misbehaviour scores never decay. exception = CGovernanceException(msg, GOVERNANCE_EXCEPTION_WARNING); - if (cmmapOrphanVotes.Insert(nHashGovobj, governance::OrphanVote{vote, Now() + GOVERNANCE_ORPHAN_EXPIRATION_TIME})) { - hashToRequest = nHashGovobj; // Caller should request this object + if (m_orphan_votes.Insert(nHashGovobj, + governance::OrphanVote{vote, Now() + GOVERNANCE_ORPHAN_EXPIRATION_TIME})) { + // Even if we already held this vote: every peer that relays it can serve the parent. + hashToRequest = nHashGovobj; + } else { + LogPrint(BCLog::GOBJECT, "CGovernanceManager::%s -- MN outpoint %s holds its share of orphan votes, dropping vote %s\n", + __func__, vote.GetMasternodeOutpoint().ToStringShort(), nHashVote.ToString()); } LogPrint(BCLog::GOBJECT, "%s\n", msg); return false; @@ -1022,7 +1082,7 @@ void GovernanceStore::Clear() LOCK(cs_store); mapObjects.clear(); mapErasedGovernanceObjects.clear(); - cmmapOrphanVotes.Clear(); + m_orphan_votes.Clear(); mapLastMasternodeObject.clear(); lastMNListForVotingKeys = std::make_shared(); } @@ -1132,41 +1192,6 @@ void CGovernanceManager::UpdatedBlockTip(const CBlockIndex* pindex) m_superblocks.ExecuteBestSuperblock(m_dmnman.GetListAtChainTip(), pindex->nHeight); } -std::vector CGovernanceManager::GetOrphanVoteObjectHashes() -{ - LOCK(cs_store); - - const auto now{Now()}; - - // Clean up expired orphan votes - const vote_cmm_t::list_t& items = cmmapOrphanVotes.GetItemList(); - for (auto it = items.begin(); it != items.end();) { - auto prevIt = it; - ++it; - if (prevIt->value.expiration < now) { - cmmapOrphanVotes.Erase(prevIt->key, prevIt->value); - } - } - - // Get hashes of objects we don't have yet - std::vector vecHashesFiltered; - std::vector vecHashes; - cmmapOrphanVotes.GetKeys(vecHashes); - for (const uint256& nHash : vecHashes) { - if (mapObjects.find(nHash) == mapObjects.end()) { - vecHashesFiltered.push_back(nHash); - } - } - - // A random subset, so that parent hashes ground to sort first cannot pin which ones are requested - if (vecHashesFiltered.size() > MAX_ORPHAN_OBJECT_REQUESTS) { - Shuffle(vecHashesFiltered.begin(), vecHashesFiltered.end(), FastRandomContext()); - vecHashesFiltered.resize(MAX_ORPHAN_OBJECT_REQUESTS); - } - - return vecHashesFiltered; -} - void CGovernanceManager::RemoveInvalidVotes() { AssertLockHeld(cs_store); @@ -1206,7 +1231,6 @@ void CGovernanceManager::RemoveInvalidVotes() } for (const auto& voteHash : removed) { cmapVoteToObject.Erase(voteHash); - cmmapOrphanVotes.Erase(voteHash); } } } diff --git a/src/governance/governance.h b/src/governance/governance.h index 631b2d939086..b75114206691 100644 --- a/src/governance/governance.h +++ b/src/governance/governance.h @@ -58,6 +58,57 @@ inline bool operator<(const OrphanVote& lhs, const OrphanVote& rhs) { return lhs.vote < rhs.vote; } + +// Orphan votes are not scarce: one masternode can sign any number of votes naming parents that do +// not exist. Each masternode is therefore limited to its own share of the cache, and the cache as a +// whole holds about 40 MB at most (roughly 800 bytes per entry). Filling it takes 500 masternodes. +static constexpr size_t MAX_ORPHAN_VOTES{50'000}; +static constexpr size_t MAX_ORPHAN_VOTES_PER_MN{100}; + +/** Votes waiting for their parent object, keyed by parent hash, with the oldest evicted once the + * cache is full and each masternode limited to a share of it. */ +class OrphanVoteCache +{ +public: + explicit OrphanVoteCache(size_t max_total, size_t max_per_mn); + + /** Returns whether the vote is held, either newly or because it already was. A new vote is + * dropped if its masternode already holds max_per_mn orphan votes. */ + [[nodiscard]] bool Insert(const uint256& parent_hash, const OrphanVote& orphan_vote); + void Erase(const uint256& parent_hash, const OrphanVote& orphan_vote); + void EraseExpired(NodeSeconds now); + void GetAll(const uint256& parent_hash, std::vector& orphan_votes); + void Clear(); + + template + void Serialize(Stream& s) const + { + s << m_cache; + } + + template + void Unserialize(Stream& s) + { + // Re-insert rather than adopt the stored cache: the file carries the capacity of whichever + // version wrote it, and not the per-masternode counts. Oldest first, to keep the order. + // Skip expired entries, which earlier versions kept while unsynced, so they cannot take up a + // masternode's share ahead of its live votes. + CacheMultiMap stored; + s >> stored; + Clear(); + const auto now{Now()}; + const auto& items = stored.GetItemList(); + for (auto it = items.rbegin(); it != items.rend(); ++it) { + if (it->value.expiration < now) continue; + (void)Insert(it->key, it->value); + } + } + +private: + const size_t m_max_per_mn; + CacheMultiMap m_cache; + std::map m_count_per_mn; +}; } // namespace governance static constexpr int RATE_BUFFER_SIZE = 5; @@ -173,7 +224,6 @@ class GovernanceStore }; using txout_m_t = std::map; - using vote_cmm_t = CacheMultiMap; protected: static constexpr int MAX_CACHE_SIZE = 1000000; @@ -189,7 +239,7 @@ class GovernanceStore // key - governance object's hash // value - expiration time for deleted objects std::map mapErasedGovernanceObjects GUARDED_BY(cs_store); - vote_cmm_t cmmapOrphanVotes GUARDED_BY(cs_store); + governance::OrphanVoteCache m_orphan_votes GUARDED_BY(cs_store); txout_m_t mapLastMasternodeObject GUARDED_BY(cs_store); // used to check for changed voting keys std::shared_ptr lastMNListForVotingKeys GUARDED_BY(cs_store); @@ -204,7 +254,7 @@ class GovernanceStore LOCK(cs_store); s << SERIALIZATION_VERSION_STRING << mapErasedGovernanceObjects - << cmmapOrphanVotes + << m_orphan_votes << mapObjects << mapLastMasternodeObject << *lastMNListForVotingKeys; @@ -223,7 +273,7 @@ class GovernanceStore } s >> mapErasedGovernanceObjects - >> cmmapOrphanVotes + >> m_orphan_votes >> mapObjects >> mapLastMasternodeObject >> *lastMNListForVotingKeys; @@ -329,8 +379,6 @@ class CGovernanceManager : public GovernanceStore * running collateral or chain validation. */ void AddGovernanceObjectForTesting(const CGovernanceObject& govobj) EXCLUSIVE_LOCKS_REQUIRED(!cs_store); - /** Test-only helper: caches a vote as an orphan of its parent without validating it. */ - void AddOrphanVoteForTesting(const CGovernanceVote& vote) EXCLUSIVE_LOCKS_REQUIRED(!cs_store); // Thread-safe accessors bool HaveObjectForHash(const uint256& nHash) const @@ -357,13 +405,6 @@ class CGovernanceManager : public GovernanceStore // Used by NetGovernance std::vector FetchRelayInventory() EXCLUSIVE_LOCKS_REQUIRED(!cs_relay); void CheckAndRemove() EXCLUSIVE_LOCKS_REQUIRED(!cs_store); - /** Upper bound on the orphan parent hashes GetOrphanVoteObjectHashes() returns per call. Each one is - * re-requested from every peer, and one masternode key can sign votes for any number of invented - * parent hashes. */ - static constexpr size_t MAX_ORPHAN_OBJECT_REQUESTS{100}; - /** Get hashes of governance objects for which we have orphan votes, at most - * MAX_ORPHAN_OBJECT_REQUESTS of them chosen at random. Also cleans up expired orphans. */ - [[nodiscard]] std::vector GetOrphanVoteObjectHashes() EXCLUSIVE_LOCKS_REQUIRED(!cs_store); std::pair, std::vector> FetchGovernanceObjectVotes( size_t peers_per_hash_max, int64_t now, std::map>& map_asked_recently) const EXCLUSIVE_LOCKS_REQUIRED(!cs_store); @@ -379,7 +420,8 @@ class CGovernanceManager : public GovernanceStore CDeterministicMNManager& GetMNManager(); /** Process a governance vote. Returns true on success. - * If the vote is for an unknown object (orphan), hashToRequest is set to the object hash. */ + * If the vote is for an unknown object (orphan) and is held until that object arrives, + * hashToRequest is set to the object hash. */ bool ProcessVote(const CGovernanceVote& vote, CGovernanceException& exception, uint256& hashToRequest) EXCLUSIVE_LOCKS_REQUIRED(!cs_store); diff --git a/src/governance/net_governance.cpp b/src/governance/net_governance.cpp index 56d193d9ad62..2929c514813d 100644 --- a/src/governance/net_governance.cpp +++ b/src/governance/net_governance.cpp @@ -45,22 +45,6 @@ void NetGovernance::Schedule(CScheduler& scheduler) [this]() -> void { if (!m_node_sync.IsSynced()) return; - // Request governance objects for orphan votes - auto vecOrphanHashes = m_gov_manager.GetOrphanVoteObjectHashes(); - if (!vecOrphanHashes.empty()) { - LogPrint(BCLog::GOBJECT, "NetGovernance::Schedule -- requesting %d orphan objects\n", - vecOrphanHashes.size()); - const CConnman::NodesSnapshot snap{m_connman, CConnman::FullyConnectedOnly}; - for (const uint256& nHash : vecOrphanHashes) { - for (CNode* pnode : snap.Nodes()) { - if (!pnode->CanRelay()) continue; - CNetMsgMaker msgMaker(pnode->GetCommonVersion()); - CBloomFilter filter; // Empty filter - we want the object, not votes - m_connman.PushMessage(pnode, msgMaker.Make(NetMsgType::MNGOVERNANCESYNC, nHash, filter)); - } - } - } - // CHECK AND REMOVE - REPROCESS GOVERNANCE OBJECTS m_gov_manager.CheckAndRemove(); }, @@ -113,9 +97,9 @@ void NetGovernance::ProcessMessage(CNode& peer, const std::string& msg_type, CDa LogPrint(BCLog::GOBJECT, "MNGOVERNANCESYNC -- syncing governance objects to our peer %s\n", peer.GetLogString()); const bool full_sync{nProp == uint256()}; const bool object_fetch{!full_sync && IsEmptyBloomFilter(filter)}; - // Nonzero govsync with an empty filter is used to retry missing-object - // fetches for orphan votes. Only full sync and actual known-object vote - // sync are fulfilled-request limited. + // Nonzero govsync with an empty filter is how older nodes fetch the + // parent object of an orphan vote. Only full sync and actual + // known-object vote sync are fulfilled-request limited. const bool track_request{full_sync || (!object_fetch && m_gov_manager.HaveSyncableObjectForHash(nProp))}; const std::string fulfilled_request{full_sync ? NetMsgType::MNGOVERNANCESYNC : @@ -258,10 +242,10 @@ void NetGovernance::ProcessMessage(CNode& peer, const std::string& msg_type, CDa } else { LogPrint(BCLog::GOBJECT, "MNGOVERNANCEOBJECTVOTE -- Rejected vote, error = %s\n", exception.what()); if (hashToRequest != uint256()) { - // Orphan vote - request the missing governance object - CNetMsgMaker msgMaker(peer.GetCommonVersion()); - CBloomFilter filter; // Empty filter - we just want the object, not votes - m_connman.PushMessage(&peer, msgMaker.Make(NetMsgType::MNGOVERNANCESYNC, hashToRequest, filter)); + // A node only relays a vote once it has the vote's parent object, so fetch the + // parent from this peer as if it had announced it. + WITH_LOCK(::cs_main, + m_peer_manager->PeerAddObjectAnnouncement(peer, CInv{MSG_GOVERNANCE_OBJECT, hashToRequest})); } if ((exception.GetNodePenalty() != 0) && m_node_sync.IsSynced()) { m_peer_manager->PeerMisbehaving(peer.GetId(), exception.GetNodePenalty()); diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 2a956dd97c39..57f7ed8428ce 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -661,6 +661,8 @@ class PeerManagerImpl final : public PeerManager GetDataResponse PeerConsumeGetDataResponse(NodeId nodeid, const CInv& inv) override EXCLUSIVE_LOCKS_REQUIRED(!m_object_request_mutex, ::cs_main); void PeerForgetObjectRequest(const CInv& inv) override EXCLUSIVE_LOCKS_REQUIRED(!m_object_request_mutex, ::cs_main); + void PeerAddObjectAnnouncement(const CNode& node, const CInv& inv) override + EXCLUSIVE_LOCKS_REQUIRED(!m_object_request_mutex, !m_recent_confirmed_transactions_mutex, ::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); @@ -7029,6 +7031,11 @@ void PeerManagerImpl::PeerForgetObjectRequest(const CInv& inv) WITH_LOCK(m_object_request_mutex, m_object_request.ForgetTxHash(inv)); } +void PeerManagerImpl::PeerAddObjectAnnouncement(const CNode& node, const CInv& inv) +{ + if (!AlreadyHave(inv)) AddObjectAnnouncement(node, inv, GetTime()); +} + void PeerManagerImpl::PeerPushInventory(NodeId nodeid, const CInv& inv) { PushInventory(nodeid, inv); diff --git a/src/net_processing.h b/src/net_processing.h index a4f4e6a311c1..745bac637177 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -112,6 +112,11 @@ class PeerManagerInternal * turns true), so it is not requested from anyone anymore. * Requires ::cs_main (see the PeerManagerImpl override). */ virtual void PeerForgetObjectRequest(const CInv& inv) = 0; + /** Treat the inv as announced by this peer: unless we already have the object, register it with + * the object request tracker under the same per-peer limits as a received INV, so it is fetched + * from this peer or, failing that, from another peer that announced it. + * Requires ::cs_main (see the PeerManagerImpl override). */ + virtual void PeerAddObjectAnnouncement(const CNode& node, 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 af8c06e46a4a..74dac5e7714f 100644 --- a/src/test/governance_inv_tests.cpp +++ b/src/test/governance_inv_tests.cpp @@ -25,11 +25,9 @@ #include -#include #include #include #include -#include #include using namespace std::chrono_literals; @@ -493,8 +491,8 @@ BOOST_AUTO_TEST_CASE(orphan_votes_require_a_valid_masternode_signature) connman.FlushSendBuffer(*peer); ProcessGovernanceVote(net_gov, *peer, vote); - BOOST_CHECK(m_node.govman->GetOrphanVoteObjectHashes().empty()); - BOOST_CHECK_EQUAL(CountQueuedMessages(*peer, NetMsgType::MNGOVERNANCESYNC), 0U); + m_node.peerman->SendMessages(peer.get()); + BOOST_CHECK_EQUAL(CountQueuedMessages(*peer, NetMsgType::GETDATA), 0U); BOOST_CHECK_EQUAL(MisbehaviorScore(*m_node.peerman, *peer), 20); m_node.peerman->FinalizeNode(*peer); @@ -542,36 +540,4 @@ BOOST_AUTO_TEST_CASE(invalid_vote_is_scored_alike_with_and_without_a_parent_obje chainstate.ResetIbd(); } -// Every orphan parent hash is re-requested from every peer on each governance tick, and one -// masternode key can sign votes for any number of invented parents, so the hashes requested per tick -// must stay bounded. -BOOST_AUTO_TEST_CASE(orphan_object_requests_are_capped_per_tick) -{ - constexpr size_t cap{CGovernanceManager::MAX_ORPHAN_OBJECT_REQUESTS}; - std::set parents; - const auto add_orphans = [&](size_t count) { - for (size_t i = 0; i < count; ++i) { - const uint256 parent{GetRandHash()}; - parents.insert(parent); - m_node.govman->AddOrphanVoteForTesting(MakeGovernanceVote(parent)); - } - }; - - // Up to the cap, every orphan parent is requested. - add_orphans(cap); - auto hashes{m_node.govman->GetOrphanVoteObjectHashes()}; - BOOST_CHECK(std::set(hashes.begin(), hashes.end()) == parents); - - // Beyond it, each tick requests exactly cap distinct ones among them. - add_orphans(cap); - for (int tick = 0; tick < 2; ++tick) { - hashes = m_node.govman->GetOrphanVoteObjectHashes(); - BOOST_CHECK_EQUAL(hashes.size(), cap); - const std::set unique(hashes.begin(), hashes.end()); - BOOST_CHECK_EQUAL(unique.size(), cap); - BOOST_CHECK(std::includes(parents.begin(), parents.end(), unique.begin(), unique.end())); - } -} - - BOOST_AUTO_TEST_SUITE_END() diff --git a/src/test/governance_vote_processing_tests.cpp b/src/test/governance_vote_processing_tests.cpp index 57c05c103b8f..f1376b4d0951 100644 --- a/src/test/governance_vote_processing_tests.cpp +++ b/src/test/governance_vote_processing_tests.cpp @@ -2,10 +2,12 @@ // Distributed under the MIT software license, see the accompanying // file COPYING or http://www.opensource.org/licenses/mit-license.php. +#include #include #include #include #include +#include #include #include #include @@ -15,8 +17,11 @@ #include #include #include +#include +#include #include #include +#include #include