Skip to content

fix: fetch orphan-vote parents via the request tracker instead of broadcasting #7526

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
PastaPastaPasta wants to merge 5 commits into from
+699 −153
Closed

fix: fetch orphan-vote parents via the request tracker instead of broadcasting #7526

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3042532
refactor: generalize AskPeersForTransaction to AskPeersForObject
PastaPastaPasta Aug 10, 2026
1be2dba
fix: fetch orphan-vote parents via the request tracker, bound the orp…
PastaPastaPasta Aug 10, 2026
f694169
test: cover orphan-vote parent fetch via the request tracker end to end
PastaPastaPasta Aug 11, 2026
6e6e0de
feat: bound orphan votes per masternode and raise the global cache cap
PastaPastaPasta Aug 12, 2026
10abb8d
perf: write the orphan-vote field of governance.dat empty
PastaPastaPasta Oct 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Makefile.test.include
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ BITCOIN_TESTS =\
test/net_peer_eviction_tests.cpp \
test/net_tests.cpp \
test/netbase_tests.cpp \
test/orphan_vote_cache_tests.cpp \
test/orphanage_tests.cpp \
test/pmt_tests.cpp \
test/policyestimator_tests.cpp \
Expand Down
9 changes: 9 additions & 0 deletions src/cachemultimap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,15 @@ class CacheMultiMap
return (mapIndex.find(key) != mapIndex.end());
}

bool HasEntry(const K& key, const V& value) const
{
map_cit mit = mapIndex.find(key);
if (mit == mapIndex.end()) {
return false;
}
return mit->second.count(value) > 0;
}

bool Get(const K& key, V& value) const
{
map_cit it = mapIndex.find(key);
Expand Down
60 changes: 35 additions & 25 deletions src/governance/governance.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ GovernanceStore::GovernanceStore() :
cs_store(),
mapObjects(),
mapErasedGovernanceObjects(),
cmmapOrphanVotes(MAX_CACHE_SIZE),
m_orphan_votes(MAX_ORPHAN_VOTES, MAX_ORPHAN_VOTES_PER_MN),
mapLastMasternodeObject(),
lastMNListForVotingKeys(std::make_shared<CDeterministicMNList>())
{
Expand Down Expand Up @@ -281,7 +281,7 @@ void CGovernanceManager::CheckOrphanVotes(CGovernanceObject& govobj)

uint256 nHash = govobj.GetHash();
std::vector<governance::OrphanVote> orphan_votes;
cmmapOrphanVotes.GetAll(nHash, orphan_votes);
m_orphan_votes.GetAll(nHash, orphan_votes);

ScopedLockBool guard(cs_store, fRateChecksEnabled, false);

Expand All @@ -298,7 +298,7 @@ void CGovernanceManager::CheckOrphanVotes(CGovernanceObject& govobj)
fRemove = true;
}
if (fRemove) {
cmmapOrphanVotes.Erase(nHash, orphan_vote);
m_orphan_votes.Erase(nHash, orphan_vote);
}
}
}
Expand Down Expand Up @@ -415,6 +415,11 @@ void CGovernanceManager::CheckAndRemove()

ScopedLockBool guard(cs_store, fRateChecksEnabled, false);

// Drop orphan votes whose parent never arrived. Votes for an object that did arrive are
// consumed by CheckOrphanVotes() at that point, so anything still here is either waiting or
// dead; this is the only thing that removes the latter.
ExpireOrphanVotes();

// Clean up any expired or invalid triggers
m_superblocks.Clean(nCachedBlockHeight);

Expand Down Expand Up @@ -841,14 +846,29 @@ bool CGovernanceManager::ProcessVote(const CGovernanceVote& vote, CGovernanceExc
exception = CGovernanceException(msg, GOVERNANCE_EXCEPTION_PERMANENT_ERROR, 20);
return false;
}
const auto insert_result = m_orphan_votes.Insert(
nHashGovobj, governance::OrphanVote{vote, Now<NodeSeconds>() + GOVERNANCE_ORPHAN_EXPIRATION_TIME});
if (insert_result == governance::OrphanVoteCache::InsertResult::MN_LIMIT) {
// This masternode already fills its share of the cache with votes for parents that
// never arrived; drop the vote and stop letting the key seed parent requests. Still no
// penalty: the sending peer may merely be relaying, and misbehaviour scores never decay.
std::string msg{strprintf("CGovernanceManager::%s -- Masternode %s is over its orphan-vote share, "
"dropping vote for unknown parent object %s",
__func__, vote.GetMasternodeOutpoint().ToStringShort(), nHashGovobj.ToString())};
exception = CGovernanceException(msg, GOVERNANCE_EXCEPTION_WARNING);
LogPrint(BCLog::GOBJECT, "%s\n", msg);
return false;
}
std::string msg{strprintf("CGovernanceManager::%s -- Unknown parent object %s, MN outpoint = %s", __func__,
nHashGovobj.ToString(), vote.GetMasternodeOutpoint().ToStringShort())};
// 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<NodeSeconds>() + GOVERNANCE_ORPHAN_EXPIRATION_TIME})) {
hashToRequest = nHashGovobj; // Caller should request this object
}
// Ask for the parent whether or not the vote itself was new to us. A vote we already hold,
// relayed by a second peer, is fresh evidence that this peer has the parent -- and it is the
// only evidence we will get, since a peer relays a given vote once. Suppressing the request
// on a duplicate would strand the parent whenever the first peer we asked fails to deliver.
hashToRequest = nHashGovobj;
LogPrint(BCLog::GOBJECT, "%s\n", msg);
return false;
}
Expand Down Expand Up @@ -1023,7 +1043,7 @@ void GovernanceStore::Clear()
LOCK(cs_store);
mapObjects.clear();
mapErasedGovernanceObjects.clear();
cmmapOrphanVotes.Clear();
m_orphan_votes.Clear();
mapLastMasternodeObject.clear();
lastMNListForVotingKeys = std::make_shared<CDeterministicMNList>();
}
Expand Down Expand Up @@ -1133,33 +1153,24 @@ void CGovernanceManager::UpdatedBlockTip(const CBlockIndex* pindex)
m_superblocks.ExecuteBestSuperblock(m_dmnman.GetListAtChainTip(), pindex->nHeight);
}

std::vector<uint256> CGovernanceManager::GetOrphanVoteObjectHashes()
void CGovernanceManager::ExpireOrphanVotes()
{
LOCK(cs_store);
AssertLockHeld(cs_store);

const auto now{Now<NodeSeconds>()};

// Clean up expired orphan votes
const vote_cmm_t::list_t& items = cmmapOrphanVotes.GetItemList();
const auto& items = m_orphan_votes.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<uint256> vecHashesFiltered;
std::vector<uint256> vecHashes;
cmmapOrphanVotes.GetKeys(vecHashes);
for (const uint256& nHash : vecHashes) {
if (mapObjects.find(nHash) == mapObjects.end()) {
vecHashesFiltered.push_back(nHash);
m_orphan_votes.Erase(prevIt->key, prevIt->value);
}
}
}

return vecHashesFiltered;
size_t CGovernanceManager::GetOrphanVoteCount() const
{
return WITH_LOCK(cs_store, return m_orphan_votes.GetSize());
}

void CGovernanceManager::RemoveInvalidVotes()
Expand Down Expand Up @@ -1201,7 +1212,6 @@ void CGovernanceManager::RemoveInvalidVotes()
}
for (const auto& voteHash : removed) {
cmapVoteToObject.Erase(voteHash);
cmmapOrphanVotes.Erase(voteHash);
}
}
}
Expand Down
119 changes: 114 additions & 5 deletions src/governance/governance.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,89 @@ 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;
Comment thread
PastaPastaPasta marked this conversation as resolved.
}
if (!m_cache.Insert(parent_hash, orphan_vote)) {
return InsertResult::DUPLICATE;
}
Comment thread
PastaPastaPasta marked this conversation as resolved.
++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);
}
}

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(); }

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;
Expand Down Expand Up @@ -175,6 +258,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: CacheMultiMap stores each vote twice, so an entry costs
* roughly 750 bytes and this allows ~75 MB; 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;
Expand All @@ -189,7 +283,7 @@ class GovernanceStore
// key - governance object's hash
// value - expiration time for deleted objects
std::map<uint256, int64_t> 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<CDeterministicMNList> lastMNListForVotingKeys GUARDED_BY(cs_store);
Expand All @@ -202,9 +296,14 @@ class GovernanceStore
void Serialize(Stream &s) const EXCLUSIVE_LOCKS_REQUIRED(!cs_store)
{
LOCK(cs_store);
// TODO: Remove the historical orphan-vote field on the next disk-format version bump.
// Unserialize discards the orphans, so write none. Builds that predate this still load the
// field, capacity included, as their live cache: keep the capacity they always wrote, since
// a CacheMultiMap with capacity 0 never prunes.
const vote_cmm_t empty_orphan_votes{MAX_CACHE_SIZE};
s << SERIALIZATION_VERSION_STRING
<< mapErasedGovernanceObjects
<< cmmapOrphanVotes
<< empty_orphan_votes
<< mapObjects
<< mapLastMasternodeObject
<< *lastMNListForVotingKeys;
Expand All @@ -222,8 +321,14 @@ class GovernanceStore
return;
}

// 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
>> cmmapOrphanVotes
>> discarded_orphan_votes
>> mapObjects
>> mapLastMasternodeObject
>> *lastMNListForVotingKeys;
Expand Down Expand Up @@ -358,8 +463,8 @@ class CGovernanceManager : public GovernanceStore
// Used by NetGovernance
std::vector<CInv> FetchRelayInventory() EXCLUSIVE_LOCKS_REQUIRED(!cs_relay);
void CheckAndRemove() EXCLUSIVE_LOCKS_REQUIRED(!cs_store);
/** Get hashes of governance objects for which we have orphan votes. Also cleans up expired orphans. */
[[nodiscard]] std::vector<uint256> GetOrphanVoteObjectHashes() 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);
Expand Down Expand Up @@ -415,6 +520,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);

Expand Down
36 changes: 12 additions & 24 deletions src/governance/net_governance.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,23 +45,9 @@ void NetGovernance::Schedule(CScheduler& scheduler)
[this]() -> void {
if (!m_node_sync.IsSynced()) return;

Comment on lines 46 to 47

@thepastaclaw thepastaclaw Oct 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⏭️ Deferred (maintainer decision; not blocking) at 10abb8d3; see the replies below.

🟡 Suggestion: Run orphan expiry while blockchain sync is complete

The scheduler returns unless IsSynced() is true, but CheckAndRemove() and ProcessMessage() deliberately use the broader IsBlockchainSynced() gate. During the interval where blockchain synchronization has completed but masternode/governance synchronization is still in progress, orphan votes can be accepted while ExpireOrphanVotes() is not called every five minutes. They may therefore remain beyond their ten-minute lifetime and continue consuming the per-masternode and global cache bounds. Use the same blockchain-sync gate as CheckAndRemove() so the scheduled expiry path matches the behavior described by the PR.

Suggested change
if (!m_node_sync.IsSynced()) return;
if (!m_node_sync.IsBlockchainSynced()) return;

source: gpt-6.1-sol (phase2-reviewer: general, dash-core-commit-history)

@PastaPastaPasta PastaPastaPasta Oct 13, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving the gate as is. On develop the 5-minute task already returns unless IsSynced(), and orphan expiry already ran under that gate, inside GetOrphanVoteObjectHashes() in the same task. So during the "blockchain synced, governance still syncing" window, expiry is no more delayed than it was before this PR.

Loosening it to IsBlockchainSynced() would also start running all of CheckAndRemove() (object reprocessing, trigger cleanup, vote cleanup) while governance sync is still in progress. That changes existing behaviour beyond this fix. In that window the orphan cache is still bounded (500 per masternode, 100'000 total), and expiry resumes on the first pass after full sync.

🤖 Posted autonomously by Claude on behalf of pasta.

@thepastaclaw thepastaclaw Oct 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that this is pre-existing behavior: the IsSynced() gate and the resulting expiry timing were already present before this change. I accept keeping that gate here to avoid broadening CheckAndRemove() into the governance-sync window; this can be addressed separately if the wider sync-state policy changes.

Noted as intentionally deferred; I will not press it further here.

// 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
// 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});
Expand Down Expand Up @@ -113,9 +99,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 peers retry
// missing-object fetches for orphan votes. 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 :
Expand Down Expand Up @@ -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 != 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));
if (!hashToRequest.IsNull()) {
Comment thread
PastaPastaPasta marked this conversation as resolved.
// 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());
Expand Down
2 changes: 1 addition & 1 deletion src/instantsend/net_instantsend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,7 +419,7 @@ void NetInstantSend::ProcessInstantSendLock(NodeId from, const uint256& hash, co
m_peer_manager->PeerRelayInvFiltered(inv, *tx);
} else {
m_peer_manager->PeerRelayInvFiltered(inv, islock->txid);
m_peer_manager->PeerAskPeersForTransaction(islock->txid);
m_peer_manager->PeerAskPeersForObject(CInv{MSG_TX, islock->txid});
}
}

Expand Down
Loading
Loading