fix: fetch orphan-vote parents via the request tracker instead of broadcasting - #7526
fix: fetch orphan-vote parents via the request tracker instead of broadcasting#7526PastaPastaPasta wants to merge 4 commits into
Conversation
The orphan-parent fetch helper was tx-specific only in its CInv construction. Take a CInv instead of a txid so other subsystems can use the object request tracker to fetch something they know they want but were never offered. Add prefer_first for a peer that demonstrably holds the object without having announced it: such a peer is in no inventory filter, so it is unreachable by the existing filter-based candidate search and can only be named. Extract the hardcoded 4 into MAX_PEERS_TO_ASK_FOR_OBJECT. Demote the per-peer log line from LogPrintf to LogPrint(BCLog::NET). The next commit calls this on a path a peer can drive, where unconditional logging would be a log-spam vector.
…han cache
NetGovernance::Schedule() sent one MNGOVERNANCESYNC per orphan parent hash per connected peer every 5 minutes, uncapped, for as long as the orphans lived. Orphan keys come from any unauthenticated peer, so that is O(peer-controlled x peers) outbound messages on a timer. PushMessage appends to vSendMsg regardless of fPauseSend, so the per-peer send buffer ceiling does not bound it.
The sweep was also redundant. A non-zero MNGOVERNANCESYNC with an empty filter is special-cased on the serving side to reply with an INV{MSG_GOVERNANCE_OBJECT}, which flows into the object request tracker anyway; the broadcast existed only to induce that announcement, and had to be exempted from the HasFulfilledRequest anti-spam accounting to work.
Seed the tracker directly instead, via PeerAskPeersForObject, naming the peer that supplied the vote: holding a vote for an object is evidence it has the object, and it may never have announced the object to us. The tracker then owns GETDATA scheduling, in-flight limits, expiry-driven fallback and AlreadyHave dedup. Fan-out per orphan parent drops from O(peers) every 5 minutes to at most 4 requests, once, and one round trip is saved.
Move orphan expiry out of the deleted GetOrphanVoteObjectHashes() into ExpireOrphanVotes(), called from CheckAndRemove() on the same 5-minute tick. Insertion is gated on IsBlockchainSynced() just as CheckAndRemove() is, so orphans can only be created in states where expiry also runs.
Bound cmmapOrphanVotes with MAX_ORPHAN_VOTES = 1000 rather than MAX_CACHE_SIZE = 1000000. Each retained entry costs ~750 bytes: CacheMultiMap stores the value twice, and each CGovernanceVote copy holds a heap-allocated signature.
No masternode/signature validation is added before orphan insertion. A valid MN signature is not scarce (nParentHash is signed, but nothing ties it to an object that exists), the orphan branch must stay at penalty 0 because reaching it is a routine relay race for honest peers, and scoring is suppressed while !IsSynced() anyway. It would add ECDSA and BLS verification under cs_store on a peer-driven path.
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
If these PRs merge firstThis PR will likely need a rebase:
|
|
🔍 Review in progress — actively reviewing now (commit 44b9656) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45467b7ddc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| mapErasedGovernanceObjects(), | ||
| cmapInvalidVotes(MAX_CACHE_SIZE), | ||
| cmmapOrphanVotes(MAX_CACHE_SIZE), | ||
| cmmapOrphanVotes(MAX_ORPHAN_VOTES), |
There was a problem hiding this comment.
On upgrades that load an existing governance.dat, this constructor limit is overwritten when CacheMultiMap::Unserialize restores its serialized nMaxSize. Because the serialization version remains CGovernanceManager-Version-16, existing files contain the old 1,000,000-entry limit, so nearly every upgraded node continues accepting that many orphan votes despite this change. Enforce MAX_ORPHAN_VOTES after loading, including pruning any excess retained entries, rather than relying only on the constructor.
AGENTS.md reference: AGENTS.md:L166-L175
Useful? React with 👍 / 👎.
| peer->m_id); | ||
|
|
||
| m_object_request.ReceivedInv(peer->m_id, CInv(MSG_TX, txid), /*preferred=*/true, current_time); | ||
| m_object_request.ReceivedInv(peer->m_id, inv, /*preferred=*/true, current_time); |
There was a problem hiding this comment.
When a peer alternates an authorized vote INV with an orphan vote naming a fresh parent, consuming the vote announcement frees its tracker slot and this direct ReceivedInv adds a new parent entry, so the peer can repeat the sequence independently of the 1,000-entry orphan cache. Unlike AddObjectAnnouncement, this path checks neither MAX_PEER_OBJECT_ANNOUNCEMENTS nor the in-flight overload threshold and always makes the request immediately eligible; consequently SendMessages can queue a large attacker-controlled burst of GETDATA requests while retaining all parent entries until completion or expiry. Route these synthetic announcements through equivalent count/delay accounting and discard requests when their orphan is evicted.
AGENTS.md reference: AGENTS.md:L165-L175
Useful? React with 👍 / 👎.
WalkthroughThe change bounds the governance orphan-vote cache at 1,000 entries and expires stale votes during scheduled cleanup. It replaces hash collection with count reporting. Governance and InstantSend missing-object retrieval now use the generic Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant GovernanceManager
participant PeerManagerImpl
participant SupplyingPeer
GovernanceManager->>PeerManagerImpl: Request missing parent CInv
PeerManagerImpl->>SupplyingPeer: Register preferred GETDATA request
SupplyingPeer-->>PeerManagerImpl: Provide parent object
PeerManagerImpl-->>GovernanceManager: Process parent object and orphan vote
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/net_processing.cpp`:
- Around line 2396-2446: Update AskPeersForObject’s candidate discovery to cover
non-transaction CInv types as well, since IsInvInFilter only reflects
transaction inventory knowledge. Track or otherwise consult peers’ known
non-transaction inventory (including entries populated by PushInv) when building
peersToAsk, while preserving prefer_first prioritization and the existing
request limits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 637b22bb-7ce5-4c0d-adb2-77221a62b2f5
📒 Files selected for processing (7)
src/governance/governance.cppsrc/governance/governance.hsrc/governance/net_governance.cppsrc/instantsend/net_instantsend.cppsrc/net_processing.cppsrc/net_processing.hsrc/test/governance_inv_tests.cpp
| void PeerManagerImpl::AskPeersForObject(const CInv& inv, NodeId prefer_first) | ||
| { | ||
| std::vector<PeerRef> peersToAsk; | ||
| peersToAsk.reserve(4); | ||
| peersToAsk.reserve(MAX_PEERS_TO_ASK_FOR_OBJECT); | ||
|
|
||
| { | ||
| READ_LOCK(m_peer_mutex); | ||
| // A peer that holds the object without having announced it is not in any inventory filter, | ||
| // so it can only be reached by being named. Ask it first: it is the one candidate we have | ||
| // positive evidence for. | ||
| if (prefer_first != -1) { | ||
| if (auto it = m_peer_map.find(prefer_first); it != m_peer_map.end()) { | ||
| peersToAsk.emplace_back(it->second); | ||
| } | ||
| } | ||
| // TODO consider prioritizing MNs again, once that flag is moved into Peer | ||
| for (const auto& [_, peer] : m_peer_map) { | ||
| if (peersToAsk.size() >= 4) { | ||
| if (peersToAsk.size() >= MAX_PEERS_TO_ASK_FOR_OBJECT) { | ||
| break; | ||
| } | ||
| if (IsInvInFilter(*peer, txid)) { | ||
| if (peer->m_id == prefer_first) { | ||
| continue; | ||
| } | ||
| if (IsInvInFilter(*peer, inv.hash)) { | ||
| peersToAsk.emplace_back(peer); | ||
| } | ||
| } | ||
| } | ||
| { | ||
| LOCK(cs_main); | ||
| const auto current_time{GetTime<std::chrono::microseconds>()}; | ||
| // Register a fresh, preferred (undelayed) MSG_TX announcement from each peer we intend to | ||
| // ask, so the transaction is requested ASAP. We deliberately do not forget existing | ||
| // announcements for this txid: any live candidate/request from another peer must survive as | ||
| // a fallback, and there is nothing to "unstick" -- the tracker deletes a txid's COMPLETED | ||
| // announcements automatically once no live one remains, so a completed entry only lingers | ||
| // while some peer is still being tried. If a peer here already has an announcement, | ||
| // ReceivedInv is a no-op and the existing one (in flight or queued) keeps its place. | ||
| // Register a fresh, preferred (undelayed) announcement from each peer we intend to ask, so | ||
| // the object is requested ASAP. We deliberately do not forget existing announcements for | ||
| // this hash: any live candidate/request from another peer must survive as a fallback, and | ||
| // there is nothing to "unstick" -- the tracker deletes a hash's COMPLETED announcements | ||
| // automatically once no live one remains, so a completed entry only lingers while some peer | ||
| // is still being tried. If a peer here already has an announcement, ReceivedInv is a no-op | ||
| // and the existing one (in flight or queued) keeps its place. | ||
| for (PeerRef& peer : peersToAsk) { | ||
| // The peer may have been disconnected (and its tracker state wiped by DisconnectedPeer) | ||
| // after we collected it above but before we took cs_main. Registering an announcement | ||
| // for a gone peer would leave a candidate that is never requested and could block the | ||
| // live fallback peers, so skip it. | ||
| if (State(peer->m_id) == nullptr) continue; | ||
| LogPrintf("PeerManagerImpl::%s -- txid=%s: asking other peer %d for correct TX\n", __func__, | ||
| txid.ToString(), peer->m_id); | ||
| LogPrint(BCLog::NET, "PeerManagerImpl::%s -- %s: asking peer %d\n", __func__, inv.ToString(), | ||
| peer->m_id); | ||
|
|
||
| m_object_request.ReceivedInv(peer->m_id, CInv(MSG_TX, txid), /*preferred=*/true, current_time); | ||
| m_object_request.ReceivedInv(peer->m_id, inv, /*preferred=*/true, current_time); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect IsInvInFilter's implementation to determine whether it is
# scoped to the tx-specific known-inventory filter or generalizes across CInv types.
ast-grep run --pattern 'bool PeerManagerImpl::IsInvInFilter($$$) {
$$$
}' --lang cpp src/net_processing.cpp
rg -n -B2 -A15 'bool PeerManagerImpl::IsInvInFilter' src/net_processing.cppRepository: dashpay/dash
Length of output: 761
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== declarations =="
rg -n -B3 -A5 'IsInvInFilter|AskPeersForObject|PeerAskPeersForObject|m_object_request.*RequestedInv|m_object_request.*ReceivedInv|m_tx_inventory_known_filter|TxRelay' src/net_processing.cpp src/net_processing.h src/net.h src/net.h-common.h 2>/dev/null | head -n 240
echo
echo "== call sites with surrounding context =="
rg -n -B6 -A8 'AskPeersForObject\(|PeerAskPeersForObject\(' src test -g '*.cpp' -g '*.h' -g '*.c' -g '*.cc' | head -n 240
echo
echo "== CInv enum usage relevant to object inventory =="
rg -n 'MSG_[A-Z_]+|SendTransaction|PushInventory|Inventory|TxFrom|TxTo|m_tx_inventory_known_filter\.insert|\.add\(' src/net_processing.cpp src/validation.cpp src/policy | sed -n '1,220p'Repository: dashpay/dash
Length of output: 15683
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== declaration and relevant declarations =="
rg -n -B4 -A8 'IsInvInFilter|AskPeersForObject|PeerAskPeersForObject|m_object_request|TxRelay|m_tx_inventory_known_filter' src/net_processing.cpp src/net_processing.h src/net.h src/net.h-common.h 2>/dev/null | sed -n '1,260p'Repository: dashpay/dash
Length of output: 19362
Scope AskPeersForObject candidate discovery or extend it beyond transactions.
IsInvInFilter() checks Peer::TxRelay::m_tx_inventory_known_filter, while PushInv() only inserts non-tx inventories (governance/IS/etc.) into vInventoryOtherToSend. For non-tx CInv types, the fallback candidate loop can only discover prefer_first, so the public contract ("Candidates are peers known to have the hash") does not hold. Restrict the docstring for non-tx invs without prefer_first, or track non-tx inventory similarly and include it here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/net_processing.cpp` around lines 2396 - 2446, Update AskPeersForObject’s
candidate discovery to cover non-transaction CInv types as well, since
IsInvInFilter only reflects transaction inventory knowledge. Track or otherwise
consult peers’ known non-transaction inventory (including entries populated by
PushInv) when building peersToAsk, while preserving prefer_first prioritization
and the existing request limits.
…ments CacheMultiMap serializes its own capacity. Setting it in the GovernanceStore constructor is therefore undone by Unserialize on any node that has an existing governance.dat, and since the on-disk format is deliberately unchanged those files still load. MAX_ORPHAN_VOTES would have applied to fresh nodes only -- the case that needs it least -- with no visible symptom. Reassert it after reading, and drop the orphans the file carried: they are a ten-minute recovery window the restart already invalidated. Clear() does not touch the capacity, so both calls are needed. AskPeersForObject registered synthetic announcements straight into the tracker, skipping the MAX_PEER_OBJECT_ANNOUNCEMENTS ceiling and overload delay that AddObjectAnnouncement applies to peer-sent ones. That was harmless while only InstantSend called it, but the governance orphan path lets a peer drive it, so apply the same per-peer accounting. Also correct the AskPeersForObject contract: the known-inventory filter is only read for peers that enabled transaction relay, so for other object types prefer_first may be the only candidate. That is intended -- the sweep this replaced also skipped non-relaying peers -- but the previous wording claimed more than the code does.
|
Went through all three automated findings. Two were real and are fixed in 7bf1403; the third was based on an incorrect premise, but pointed at a docstring that did overclaim. 1. Codex — orphan-cache limit lost after deserialization: correct, and the most important oneConfirmed and fixed. Fixed by reasserting the bound after reading, and dropping the orphans the file carried (they are a ten-minute recovery window that the restart has already invalidated). The field stays in the stream so the on-disk format is unchanged. Added Good catch — this would have shipped as a silent no-op on every upgraded node. 2. Codex — request-tracker limits on synthetic announcements: partly correct, fixed the valid partAgreed on the accounting gap. I did not implement the second half ("discard requests when their orphan is evicted"). Tracker entries already expire on their own via 3. CodeRabbit —
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bf1403ae3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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()) { |
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—ProcessVote rejects the duplicate cache insertion and leaves hashToRequest null, so this condition skips PeerAskPeersForObject and never registers the second peer as a fallback. Because this change also removes the periodic all-peer orphan sweep, the parent can remain unavailable until an unrelated object announcement or full governance resync, despite the second peer providing the same evidence that motivated preferring the first peer. Return the cached orphan's parent for later relays, or otherwise register each relaying peer while the orphan remains pending.
AGENTS.md reference: AGENTS.md:L165-L175
Useful? React with 👍 / 👎.
The parent request was conditional on cmmapOrphanVotes.Insert() returning true. OrphanVote compares by vote, so a second peer relaying a vote we already hold is a duplicate, the insert fails, and hashToRequest stayed null -- that peer never became a candidate for the parent. That condition made sense when the request was a direct PushMessage, where it avoided sending the same peer a redundant message, and it was harmless anyway while the five-minute sweep asked every peer regardless. With the sweep gone and requests routed through the object request tracker, it strands the parent: a peer relays a given vote once, so a duplicate relay is the only evidence we will ever get that this peer has the parent, and if the peer we asked first never delivers there is nothing left to fall back on. Request unconditionally instead. The tracker already dedups per peer, so repeating this for a peer that is already a candidate is a no-op, and the announcement accounting added earlier bounds what a peer can accumulate.
|
Correct, and this one is squarely a regression this PR introduced. Fixed in 44b9656.
The root of it is that the The request is for the parent object, not for the vote, so it is now issued on every relay while the parent is unknown. A peer relays a given vote once, which makes a duplicate relay the only evidence we will ever get that this particular peer has the parent; discarding it left nothing to fall back on once the first peer we asked went quiet. Regression test It also asserts the orphan cache still holds one entry, so the duplicate is not double-counted as orphan state. I did not add a path that re-derives a parent hash from the cache for an already-known vote, which was your other suggested shape. Requesting unconditionally on the orphan path gets the same coverage without a cache lookup, and it keeps I have also reworded the behavioural caveat in the PR description, which previously said fallbacks accumulate only as further votes arrive — that was written against the buggy behaviour and understated things in a way this fix corrects. Full unit suite (794 cases), 🤖 Posted autonomously by Claude on behalf of pasta. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 44b965602a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| m_object_request.ReceivedInv(peer->m_id, CInv(MSG_TX, txid), /*preferred=*/true, current_time); | ||
| // Preferred and otherwise undelayed: unlike a peer-initiated announcement, we asked for | ||
| // this one and want it as soon as the peer's in-flight budget allows. | ||
| m_object_request.ReceivedInv(peer->m_id, inv, /*preferred=*/true, |
There was a problem hiding this comment.
When another peer's inventory filter also contains this hash, every candidate is registered with preferred=true; TxRequestTracker then selects the candidate with the highest randomized priority, not the first inserted candidate. Thus prefer_first does not actually ask the orphan-vote relayer first, and a stale or malicious alternate announcement can delay the parent fetch by the 60-second governance-object request interval. Give the named peer higher tracker priority than the fallback candidates and cover the multi-candidate case in a focused test.
AGENTS.md reference: AGENTS.md:L165-L175
Useful? React with 👍 / 👎.
2 participants
Issue being fixed or feature implemented
CGovernanceManagerholds votes whose parent governance object has not arrived yet ("orphan votes") incmmapOrphanVotes, keyed by the parent hash carried in the vote. Any unauthenticated peer can put entries there: the only gate is the standard announce-then-request tracker.Two things then scale with that peer-controlled input.
Fan-out.
NetGovernance::Schedule()ran a 5-minute sweep that sent oneMNGOVERNANCESYNCper orphan parent hash per connected peer, uncapped. That isO(orphans x peers)outbound messages every 5 minutes, for as long as the orphans live. With ~30k orphans and 50 peers that is ~1.5M messages per tick — roughly 180 MB ofvSendMsgallocated in a burst plus ~100 MB of egress, repeating.CSerializedNetMsgis appended byPushMessageregardless offPauseSend(that flag only throttles reading from a peer), so the per-peer-maxsendbufferceiling of 1 MB does not stop it.Cache size.
cmmapOrphanVoteswas constructed withMAX_CACHE_SIZE = 1'000'000. Each retained entry costs roughly 750 bytes:CacheMultiMapstores the value twice — once inlistItems, once as the key of the innerstd::map<V, list_it>— and each copy ofCGovernanceVotecarries a heap-allocated signature. Reaching the full ceiling is throttled by the fetch path, so the realistic figure is tens of MB rather than the ~750 MB the bound permits; it is still not a bound this node chose.The fan-out is the larger of the two, in bandwidth and in memory.
Worth noting what the sweep was actually doing. On the serving side,
MNGOVERNANCESYNCwith a non-zeronPropand an empty bloom filter is special-cased (object_fetchinnet_governance.cpp) to reply with a plainINV{MSG_GOVERNANCE_OBJECT, nProp}— which then flows into the ordinary object request tracker. So the sweep was an unbounded broadcast whose only purpose was to induce an announcement that the tracker would act on. It also had to be exempted from theHasFulfilledRequestanti-spam accounting to work at all.This is resource exhaustion only. Orphan votes never reach consensus, and the worst functional outcome is dropped governance votes that re-sync.
What was done?
Fetch orphan parents through the object request tracker instead of broadcasting.
PeerManagerImpl::AskPeersForTransaction(txid)already implemented the right pattern for exactly this problem — fetching a parent you know you want but were never offered — for orphan transactions. It is generalized toAskPeersForObject(const CInv&, NodeId prefer_first)and exposed asPeerAskPeersForObject. It registers a preferred announcement withm_object_requestfor a small number of peers and lets the tracker own the fetch: GETDATA scheduling,MAX_PEER_OBJECT_REQUEST_IN_FLIGHT,OVERLOADED_PEER_OBJECT_DELAY, expiry-driven fallback to the next candidate, andAlreadyHave()dedup once the object turns up from any source.prefer_firstis new. A peer that holds an object without having announced it appears in no inventory filter, so the existing filter-based candidate search cannot reach it. The peer that sent us an orphan vote is exactly that case — holding a vote for an object is evidence it has the object — so it is named directly.The orphan branch in
NetGovernance::ProcessMessagenow calls this instead of pushingMNGOVERNANCESYNC, and the 5-minute sweep plusGetOrphanVoteObjectHashes()are deleted. Fan-out per orphan parent goes fromO(peers)every 5 minutes for the orphan's lifetime to at most 4 tracker-managed requests, once. One round trip is also saved, since the tracker is seeded directly rather than via an induced INV.Keep expiring orphans. Expiry lived inside
GetOrphanVoteObjectHashes(). It moves toExpireOrphanVotes(), called fromCheckAndRemove()— the same 5-minute tick, one gate looser (IsBlockchainSyncedrather thanIsSynced).Bound the cache.
cmmapOrphanVotesis constructed withMAX_ORPHAN_VOTES = 1000instead ofMAX_CACHE_SIZE. Orphans are short-lived recovery state for votes that outran their object in relay, so the bound only has to cover objects genuinely in flight.We still serve
object_fetchrequests from older peers; only the sending side changes.Deliberately not done
No masternode/signature validation was added before orphan insertion. A valid MN signature is not a scarce resource —
nParentHashis covered by the signature, but nothing ties it to an object that exists, so any one of the masternode keys can sign unlimited votes naming invented parents. The orphan branch also has to stay at penalty 0, because reaching it is a routine relay race for honest peers, and misbehavior scoring is suppressed while!IsSynced()— precisely when orphans are common. Validation would add ECDSA and BLS verification undercs_storeon a path a peer can drive. The bound and the tracker are what actually close this; validation would be costly hardening on top, and is better considered separately.How Has This Been Tested?
Built and tested locally on aarch64-apple-darwin (
--enable-debug), fullmakeclean.New unit tests in
src/test/governance_inv_tests.cpp:orphan_vote_parent_fetch_does_not_fan_out_to_other_peers— an orphan vote results in a tracker request against the supplying peer, while a connected bystander that announced nothing receives no message, no INV, and no tracker entry.orphan_vote_cache_is_bounded— pushingMAX_ORPHAN_VOTES + 50distinct orphans leaves exactlyMAX_ORPHAN_VOTESheld.Two existing tests (
governance_votes_require_peer_announcement_or_request,governance_vote_authorization_survives_unsynced_drop) asserted on the oldMNGOVERNANCESYNCbroadcast as the signal that the orphan path ran; they now assert the tracker holds aMSG_GOVERNANCE_OBJECTrequest for the parent from that peer.All four assertions were confirmed to fail when the fix is reverted (1050 != 1000 for the bound; no tracker request for the routing).
Unit:
governance_inv_tests,governance_superblock_tests,governance_validators_tests,governance_vote_wire_tests,denialofservice_tests,net_tests,net_peer_eviction_tests,peerman_tests— all pass.Functional:
feature_governance.py,feature_governance_cl.pypass;p2p_instantsend.pyandrpc_verifyislock.pypass for the InstantSend caller that was updated.Lint:
lint-whitespace.py,lint-circular-dependencies.pyclean.Breaking Changes
None. No message format changes, no
governance.datformat change, no consensus or P2P protocol change. Purely a change in what this node sends.One behavioral note for reviewers, called out explicitly because it is a deliberate narrowing rather than a strict improvement. The old sweep re-asked every connected peer every 5 minutes for an orphan's full 10-minute life. The new path registers only peers that give us evidence they have the parent: the peer that relayed the vote, plus any that already announced that specific object hash. So the set of peers asked is driven by who actually relays to us rather than by who happens to be connected.
Every relay of a vote for a still-missing parent adds its sender as a candidate, including a relay of a vote we already hold, so fallbacks accumulate as the vote propagates rather than being fixed at the first sender. The tracker retries and moves to the next candidate on expiry, and the object also arrives through ordinary governance sync. The accepted trade is that the old persistence was the amplification: it cannot be kept without keeping the
O(orphans x peers)term.Checklist: