fix: fetch orphan-vote parents via the request tracker instead of broadcasting #7526
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -57,6 +57,109 @@ inline bool operator<(const OrphanVote& lhs, const OrphanVote& rhs) | |
| { | ||
| return lhs.vote < rhs.vote; | ||
| } | ||
|
|
||
| /** Bounded holding area for votes whose parent object has not arrived yet. Entries are | ||
| * peer-supplied and evicted oldest-first, so two bounds are enforced together: a global one that | ||
| * caps memory, and a per-masternode one so that a single voting key cannot mint votes naming | ||
| * invented parents and flush everyone else's orphans on demand. */ | ||
| class OrphanVoteCache | ||
| { | ||
| public: | ||
| using cache_t = CacheMultiMap<uint256, OrphanVote>; | ||
|
|
||
| enum class InsertResult { | ||
| OK, | ||
| DUPLICATE, //!< this (parent, vote) pair is already cached | ||
| MN_LIMIT, //!< the vote's masternode already holds its full share of the cache | ||
| }; | ||
|
|
||
| OrphanVoteCache(size_t max_total, size_t max_per_mn) : | ||
| m_max_total{max_total}, | ||
| m_max_per_mn{max_per_mn}, | ||
| // Eviction happens here, before the inner map is ever full, so its own self-pruning | ||
| // (which would bypass the per-masternode accounting) can never trigger. | ||
| m_cache{static_cast<cache_t::size_type>(max_total + 1)} | ||
| { | ||
| } | ||
|
|
||
| InsertResult Insert(const uint256& parent_hash, const OrphanVote& orphan_vote) | ||
| { | ||
| // A pair we already hold is reported as a duplicate even when its masternode is over its | ||
| // share: it costs no capacity, and the caller treats a repeat relay as fresh evidence that | ||
| // the sending peer has the missing parent. | ||
| if (m_cache.HasEntry(parent_hash, orphan_vote)) { | ||
| return InsertResult::DUPLICATE; | ||
| } | ||
| const COutPoint outpoint{orphan_vote.vote.GetMasternodeOutpoint()}; | ||
| if (const auto it{m_counts.find(outpoint)}; it != m_counts.end() && it->second >= m_max_per_mn) { | ||
| return InsertResult::MN_LIMIT; | ||
| } | ||
| if (!m_cache.Insert(parent_hash, orphan_vote)) { | ||
| return InsertResult::DUPLICATE; | ||
| } | ||
|
Comment on lines
+85
to
+99
There was a problem hiding this comment. 🔴 Blocking: Recognize duplicate orphans before enforcing the masternode limit When a masternode has exactly source: ['codex']
Member
Author
There was a problem hiding this comment. Valid — real bug, fixed in 082f399 (fixup into the commit that introduced it). Confirmed the consequence you describe:
🤖 Posted autonomously by Claude on behalf of pasta. There was a problem hiding this comment. Resolved in this update — Recognize duplicate orphans before enforcing the masternode limit no longer present. Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread. |
||
| ++m_counts[outpoint]; | ||
| if (m_cache.GetSize() > m_max_total) { | ||
| // Insert prepends, so the back is the oldest entry cache-wide and never the one just | ||
| // added. Copied, not referenced: Erase destroys the node. | ||
| const auto oldest{m_cache.GetItemList().back()}; | ||
| Erase(oldest.key, oldest.value); | ||
| } | ||
| return InsertResult::OK; | ||
| } | ||
|
|
||
| void Erase(const uint256& parent_hash, const OrphanVote& orphan_vote) | ||
| { | ||
| // Callers may pass a reference into the cache's own item list (e.g. the expiry sweep), | ||
| // which the erase below invalidates; take what we need first. | ||
| const COutPoint outpoint{orphan_vote.vote.GetMasternodeOutpoint()}; | ||
| const auto size_before{m_cache.GetSize()}; | ||
| m_cache.Erase(parent_hash, orphan_vote); | ||
| if (m_cache.GetSize() == size_before) return; | ||
| if (const auto it{m_counts.find(outpoint)}; it != m_counts.end() && --it->second == 0) { | ||
| m_counts.erase(it); | ||
| } | ||
| } | ||
|
|
||
| //! Drop every cached vote from one masternode, e.g. because its keys changed and the votes | ||
| //! can no longer validate on replay. Returns how many were dropped. | ||
| size_t EraseAllForMasternode(const COutPoint& outpoint) | ||
| { | ||
| if (m_counts.find(outpoint) == m_counts.end()) return 0; | ||
| // Collect first: Erase destroys the nodes being iterated. | ||
| std::vector<std::pair<uint256, OrphanVote>> expelled; | ||
| for (const auto& item : m_cache.GetItemList()) { | ||
| if (item.value.vote.GetMasternodeOutpoint() == outpoint) { | ||
| expelled.emplace_back(item.key, item.value); | ||
| } | ||
| } | ||
| for (const auto& [parent_hash, orphan_vote] : expelled) { | ||
| Erase(parent_hash, orphan_vote); | ||
| } | ||
| return expelled.size(); | ||
| } | ||
|
|
||
| bool GetAll(const uint256& parent_hash, std::vector<OrphanVote>& votes) | ||
| { | ||
| return m_cache.GetAll(parent_hash, votes); | ||
| } | ||
|
|
||
| void Clear() | ||
| { | ||
| m_cache.Clear(); | ||
| m_counts.clear(); | ||
| } | ||
|
|
||
| size_t GetSize() const { return m_cache.GetSize(); } | ||
| const cache_t::list_t& GetItemList() const { return m_cache.GetItemList(); } | ||
| //! The inner map, for writing the legacy on-disk field. | ||
| const cache_t& Store() const { return m_cache; } | ||
|
|
||
| private: | ||
| const size_t m_max_total; | ||
| const size_t m_max_per_mn; | ||
| cache_t m_cache; | ||
| std::map<COutPoint, size_t> m_counts; | ||
| }; | ||
| } // namespace governance | ||
|
|
||
| static constexpr int RATE_BUFFER_SIZE = 5; | ||
|
|
@@ -176,6 +279,17 @@ class GovernanceStore | |
| using txout_m_t = std::map<COutPoint, last_object_rec>; | ||
| using vote_cmm_t = CacheMultiMap<uint256, governance::OrphanVote>; | ||
|
|
||
| public: | ||
| /** Bounds for the orphan-vote cache, which is filled from the network by any peer with a | ||
| * parent object we do not have. The per-masternode bound is the security control: eviction | ||
| * is oldest-first, and every entry costs its sender a registered masternode voting key, so | ||
| * one key can occupy at most its share instead of flushing the whole cache; the value is | ||
| * far above the number of in-flight objects one masternode can plausibly have voted on. | ||
| * The global bound caps memory: at roughly 400 bytes per entry this allows ~40 MB, and | ||
| * filling it takes 200 distinct masternode keys. */ | ||
| static constexpr size_t MAX_ORPHAN_VOTES = 100'000; | ||
| static constexpr size_t MAX_ORPHAN_VOTES_PER_MN = 500; | ||
|
|
||
| protected: | ||
| static constexpr int MAX_CACHE_SIZE = 1000000; | ||
| static const std::string SERIALIZATION_VERSION_STRING; | ||
|
|
@@ -190,7 +304,7 @@ class GovernanceStore | |
| // key - governance object's hash | ||
| // value - expiration time for deleted objects | ||
| std::map<uint256, int64_t> mapErasedGovernanceObjects 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<CDeterministicMNList> lastMNListForVotingKeys GUARDED_BY(cs_store); | ||
|
|
@@ -208,7 +322,7 @@ class GovernanceStore | |
| s << SERIALIZATION_VERSION_STRING | ||
| << mapErasedGovernanceObjects | ||
| << empty_invalid_votes | ||
| << m_orphan_votes.Store() | ||
| << mapObjects | ||
| << mapLastMasternodeObject | ||
| << *lastMNListForVotingKeys; | ||
|
|
@@ -228,9 +342,15 @@ class GovernanceStore | |
|
|
||
| // TODO: Stop consuming the historical invalid-vote-cache field on the next disk-format version bump. | ||
| CacheMap<uint256, CGovernanceVote> discarded_invalid_votes; | ||
| // The historical format stores CacheMultiMap's capacity with its entries. Consume that | ||
| // field to preserve the format, but keep both the stale orphan votes and their disk-supplied | ||
| // capacity out of the live cache. Orphans are a ten-minute recovery window invalidated by | ||
| // the restart; the live capacity is fixed at construction. | ||
| vote_cmm_t discarded_orphan_votes; | ||
|
|
||
| s >> mapErasedGovernanceObjects | ||
| >> discarded_invalid_votes | ||
| >> discarded_orphan_votes | ||
| >> mapObjects | ||
| >> mapLastMasternodeObject | ||
| >> *lastMNListForVotingKeys; | ||
|
|
@@ -365,8 +485,8 @@ class CGovernanceManager : public GovernanceStore | |
| // Used by NetGovernance | ||
| std::vector<CInv> FetchRelayInventory() EXCLUSIVE_LOCKS_REQUIRED(!cs_relay); | ||
| void CheckAndRemove() EXCLUSIVE_LOCKS_REQUIRED(!cs_store); | ||
| /** Number of orphan votes currently held, so the MAX_ORPHAN_VOTES bound can be asserted. */ | ||
| [[nodiscard]] size_t GetOrphanVoteCount() const EXCLUSIVE_LOCKS_REQUIRED(!cs_store); | ||
| std::pair<std::vector<uint256>, std::vector<uint256>> FetchGovernanceObjectVotes( | ||
| size_t peers_per_hash_max, int64_t now, std::map<uint256, std::map<CService, int64_t>>& map_asked_recently) const | ||
| EXCLUSIVE_LOCKS_REQUIRED(!cs_store); | ||
|
|
@@ -417,6 +537,10 @@ class CGovernanceManager : public GovernanceStore | |
| void CheckOrphanVotes(CGovernanceObject& govobj) | ||
| EXCLUSIVE_LOCKS_REQUIRED(cs_store, !cs_relay); | ||
|
|
||
| /** Drop orphan votes whose parent object never arrived within GOVERNANCE_ORPHAN_EXPIRATION_TIME. */ | ||
| void ExpireOrphanVotes() | ||
| EXCLUSIVE_LOCKS_REQUIRED(cs_store); | ||
|
|
||
| void RebuildIndexes() | ||
| EXCLUSIVE_LOCKS_REQUIRED(cs_store); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,23 +45,9 @@ void NetGovernance::Schedule(CScheduler& scheduler) | |
| [this]() -> void { | ||
| if (!m_node_sync.IsSynced()) return; | ||
|
|
||
| // CHECK AND REMOVE - REPROCESS GOVERNANCE OBJECTS | ||
| // Also expires orphan votes whose parent object never arrived. Fetching those parents | ||
| // is driven by the object request tracker from ProcessMessage(), not from here. | ||
| m_gov_manager.CheckAndRemove(); | ||
| }, | ||
| std::chrono::minutes{5}); | ||
|
|
@@ -257,11 +243,13 @@ void NetGovernance::ProcessMessage(CNode& peer, const std::string& msg_type, CDa | |
| // m_peer_manager->PeerRelayInv(CInv{MSG_GOVERNANCE_OBJECT_VOTE, nHash}); | ||
| } else { | ||
| LogPrint(BCLog::GOBJECT, "MNGOVERNANCEOBJECTVOTE -- Rejected vote, error = %s\n", exception.what()); | ||
| if (!hashToRequest.IsNull()) { | ||
|
There was a problem hiding this comment.
When the same orphan vote is later received from a second peer—particularly after the first peer's parent request timed out— AGENTS.md reference: AGENTS.md:L165-L175 Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Addressed some time ago and further hardened in 082f399. 🤖 Posted autonomously by Claude on behalf of pasta. |
||
| // Orphan vote: fetch the parent object through the request tracker, which owns | ||
| // GETDATA scheduling, per-peer in-flight limits, expiry and fallback to another | ||
| // peer. Register this peer explicitly -- holding a vote for the object is evidence | ||
| // it has the object, and it may never have announced the object to us. | ||
| m_peer_manager->PeerAskPeersForObject(CInv{MSG_GOVERNANCE_OBJECT, hashToRequest}, | ||
| peer.GetId()); | ||
| } | ||
| if ((exception.GetNodePenalty() != 0) && m_node_sync.IsSynced()) { | ||
| m_peer_manager->PeerMisbehaving(peer.GetId(), exception.GetNodePenalty()); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a masternode has exactly 500 cached orphans, relaying any one of those votes from a second peer returns
MN_LIMITbefore checking whether the entry is a duplicate.ProcessVote()consequently exits without settinghashToRequest, so the second relayer is not registered as a fallback even though the duplicate consumes no additional cache capacity; if the first peer fails and no periodic sweep remains, that parent can be stranded. Fresh evidence in the final code is this limit check's position ahead ofm_cache.Insert, which contradicts the newly added duplicate-relay recovery path; detect duplicates first and add a boundary test with a full per-masternode share.AGENTS.md reference: AGENTS.md:L171-L171
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed and fixed in 082f399.
OrphanVoteCache::Insert()now checks for an existing(parent, vote)pair before applying the per-masternode share, via a newCacheMultiMap::HasEntry(). Covered byduplicates_are_recognized_at_the_per_masternode_sharein the newsrc/test/orphan_vote_cache_tests.cpp.🤖 Posted autonomously by Claude on behalf of pasta.