Repository navigation
fix: fetch governance orphan-vote parents through the request tracker and bound the orphan cache per masternode - #7886
Conversation
A governance vote whose parent object we do not have is kept as an orphan, and we try to fetch the parent. That fetch had two parts: - the peer that sent the vote was sent MNGOVERNANCESYNC(parent, empty filter), and - every 5 minutes NetGovernance::Schedule() sent the same message for every orphan parent hash to every connected peer. The sweep is O(orphan parents x peers) messages per tick, without a cap. PushMessage queues them whether or not fPauseSend is set. Every orphan parent comes from a peer-supplied vote: the parent hash is covered by the masternode signature, but nothing ties it to an object that exists. So one masternode key can invent parents without limit. At 30k orphan parents and 50 peers, one tick is about 1.5M messages (~180 MB of vSendMsg and ~100 MB of egress), repeated every 5 minutes for as long as the orphans live. The message only induces an announcement. A peer serves a non-zero MNGOVERNANCESYNC with an empty filter by replying INV(MSG_GOVERNANCE_OBJECT, parent), which goes through the normal object request tracker. So skip the round trip and register the vote's sender with the tracker directly, as if it had announced the parent. This is how orphan transactions already fetch their parents. It uses the same AlreadyHave() check, MAX_PEER_OBJECT_ANNOUNCEMENTS cap, overload delay and preferred/non-preferred scheduling as a received INV. The new PeerAddObjectAnnouncement() exposes it to NetGovernance. Because of this, an orphan vote gives a peer no more request capacity than it already has by announcing hashes itself. The sweep and GetOrphanVoteObjectHashes() are removed. Orphan expiry used to live in that function. It moves to CheckAndRemove(), which still runs on the same 5-minute tick, so orphans whose parent never arrives are still dropped. The sweep was also the fallback when the first peer asked did not deliver the parent. Instead, ProcessVote() now returns the parent hash for every relay of an orphan vote, including a vote we already hold. Each peer that relays it becomes a fetch candidate, and the tracker moves on to the next one when a request times out or a peer disconnects. Nodes on this version still serve the object-fetch form of MNGOVERNANCESYNC to older peers. Only the sending side changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 43 seconds. View limit details
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 |
|
✅ Final review complete — no blockers (commit 1045665) · triage: normal |
e550e5d to
6634275
Compare
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:
|
![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: e550e5d849
ℹ️ 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".
The orphan vote cache allowed 1'000'000 entries, at roughly 800 bytes each. Nothing makes its entries scarce. A vote needs a valid masternode signature, but nothing ties the signed parent hash to an object that exists, so one masternode key can sign votes for as many invented parents as it likes. Eviction is oldest-first across the whole cache, so a global bound alone would let that one key push every other masternode's orphans out. governance::OrphanVoteCache wraps the CacheMultiMap and applies two bounds: - MAX_ORPHAN_VOTES_PER_MN = 100. Once a masternode holds that many orphan votes, its further new votes are dropped and ProcessVote() no longer returns their parent for fetching. A masternode has one current vote per signal on an object, and an orphan expires after ten minutes, so an honest masternode needs far fewer than 100 pending votes for objects we lack. - MAX_ORPHAN_VOTES = 50'000, about 40 MB in the worst case (every entry with its own parent hash). Filling it takes 500 masternodes. The wrapper evicts the oldest entry itself, before the inner map would prune on its own, so the evicted vote's masternode gets that part of its share back. A vote that is already held is still reported as held when its masternode is at its share, so every peer that relays it still becomes a candidate for fetching the parent. Erase() releases the share of the stored entry, not of the vote passed in. The two only have to compare equal, and CGovernanceVote::operator< currently treats every vote for the same parent as equal (fixed separately in dashpay#7819). CacheMultiMap stores its capacity in governance.dat. Reading it into the live cache would set the bound to whatever the writing version used (1'000'000 for all earlier versions) and would leave the per-masternode counts empty. The stored orphans are now re-inserted through the bounded Insert() instead. The on-disk format does not change. The orphan erase in RemoveInvalidVotes() is removed. It looked up a vote hash in a map keyed by parent hash, so it never matched anything. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
6634275 to
1045665
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Static verification of the complete PR diff at 1045665 supports the reviewers' clean assessments: orphan-parent fetching uses the existing request tracker, and cache bounds account for eviction, expiry, and deserialization without changing the stored format. The previously discussed zero-capacity precondition is enforced with assert(max_total > 0), and no actionable in-scope defects were identified. No builds or tests were run in this lane; the supplied CI snapshot shows lint and ClangFormat passing, while source builds and platform checks remain queued.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The change spans governance orphan-cache accounting, expiry, and peer request scheduling, but does not itself change consensus, funds handling, cryptography, network deserialization, or storage migrations. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 14% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer
thepastaclaw
added
the
pastaclaw:approved
2 participants
Issue being fixed or feature implemented
A governance vote whose parent object we do not have is kept as an orphan, and the node tries to fetch the parent. Two things in that path grow with input a peer controls:
Fan-out. Every 5 minutes,
NetGovernance::Schedule()sent oneMNGOVERNANCESYNC(parent, empty filter)per orphan parent hash to every connected peer, with no cap. That is O(orphan parents × peers) messages per tick.PushMessagequeues them whether or notfPauseSendis set. With 30k orphan parents and 50 peers, one tick is about 1.5M messages: roughly 180 MB ofvSendMsgand 100 MB of egress, repeated every 5 minutes for as long as the orphans live.Cache.
cmmapOrphanVotesallowed 1,000,000 entries at roughly 800 bytes each.Nothing makes orphan votes scarce. A vote needs a valid masternode signature, but nothing ties the signed
nParentHashto an object that exists. One masternode key can sign votes for as many invented parents as it likes.The broadcast was also unnecessary. When a peer receives a non-zero
MNGOVERNANCESYNCwith an empty filter (object_fetchinnet_governance.cpp), it replies with a plainINV(MSG_GOVERNANCE_OBJECT, parent), and that INV goes through the normal object request tracker. The message only made the peer announce the object.This replaces #7526, which was closed so it could be rebuilt. The design is the same: fetch through the tracker, delete the sweep, and bound the cache globally and per masternode. Changes relative to #7526:
AskPeersForTransactionis not generalized. fix: fetch orphan-vote parents via the request tracker instead of broadcasting #7526 did that in a commit labelledrefactor:that changed InstantSend behavior. Here the InstantSend path is untouched. The vote's sender is simply treated as having announced the parent.force_preferred. A vote relayer gets the same preferred/non-preferred treatment as any announcer, so an inbound relayer cannot jump ahead of outbound peers.develop.governance.datfield change, no new test files, and no framework message classes. +291/−70 in 8 files, compared with +699/−153 in 17.What was done?
fix: fetch orphan-vote parents through the object request trackerPeerManagerInternal::PeerAddObjectAnnouncement(node, inv), which isif (!AlreadyHave(inv)) AddObjectAnnouncement(...). This is what a received INV does, and how orphan transactions already fetch their parents. So it gets the sameMAX_PEER_OBJECT_ANNOUNCEMENTScap, overload delay and preferred/non-preferred scheduling. An orphan vote gives a peer no request capacity beyond what it already has by announcing hashes itself.NetGovernanceregisters the vote's sender this way instead of sending itMNGOVERNANCESYNC. Nodes only relay votes whose parent they have, so the sender is the peer to ask.GetOrphanVoteObjectHashes()are deleted. Orphan expiry used to happen insideGetOrphanVoteObjectHashes(). It moves toCheckAndRemove(), which still runs on the same tick, so orphans whose parent never arrives are still dropped.ProcessVote()now returns the parent hash for every relay of an orphan vote, including a vote that is already held. Every relayer becomes a fetch candidate, and the tracker moves to the next one when a request times out or the peer disconnects. Before this change, the sweep was the fallback.object_fetchform ofMNGOVERNANCESYNCfrom older peers. Only the sending side changes.fix: bound the governance orphan vote cache per masternodegovernance::OrphanVoteCachewraps theCacheMultiMap. It applies a per-masternode share (MAX_ORPHAN_VOTES_PER_MN = 100) and a global bound (MAX_ORPHAN_VOTES = 50,000). Eviction is oldest-first. Without the share, one key could evict every other masternode's orphans.governance.datstoresCacheMultiMap's capacity, and earlier versions wrote 1,000,000. Adopting the stored map would make that the live bound and leave the per-masternode counts empty. Stored orphans are therefore re-inserted through the boundedInsert(). The on-disk format is unchanged.RemoveInvalidVotes()is removed. It looked up a vote hash in a map keyed by parent hash, so it never matched anything.Sizing. I measured about 800 bytes per entry (allocator-reported, macOS, BLS-size signature, one parent hash per entry). That puts the global bound at about 40 MB. A masternode has one current vote per signal on an object, and an orphan expires after ten minutes, so an honest masternode needs far fewer than 100 pending votes for objects we lack. Filling the cache takes 500 masternodes. These figures still hold with the corrected vote ordering from #7819, which allows many votes per parent: entries cost the same (slightly less when they share a parent), and #7819 keeps at most one current vote per masternode, signal, outcome and signing key for each parent.
Deliberate behavior narrowing. The sweep asked every connected peer every 5 minutes for the whole 10-minute life of an orphan. Now only peers with evidence are asked: those that relayed the vote. If all of them fail to deliver, a synced node recovers through ordinary flood relay of the object. A restarting node is covered by governance sync. Orphans loaded from
governance.datno longer cause any fetch either: they are replayed if their parent arrives by other means, and otherwise expire within ten minutes. The fan-out cannot be removed while keeping that persistence, because the persistence was the fan-out.Deliberately not done.
!IsSynced(), and the relaying peer may be innocent. The per-masternode share is the response to a misbehaving key.Relationship to #7819. #7819 fixes
CGovernanceVote::operator<, which has returned false for every pair since 2016, so today the orphan cache keeps only the first vote per parent. It also fixes a dangling reference inCacheMultiMap::Insert, and keeps only the newest orphan per signer. The two PRs are independent and either can land first.OrphanVoteCacheis correct under either ordering: it evicts before the inner map would prune, so it never reaches theInsertpath #7819 fixes, andErase()credits the share of the stored entry it finds.Merging them conflicts in two places:
ProcessVote()ingovernance.cpp: fix: preserve and bound orphan governance votes #7819'sInsertOrphanVote()takes anOrphanVoteCache&instead of the raw map, with a rangedGetAll()added to the wrapper. It should also returntrue(held) rather thanfalsewhen a newer vote from the same signer is already cached. Otherwise a second peer relaying an already-held vote is not registered for the parent, andorphan_vote_parent_is_fetched_from_the_peers_relaying_the_votefails, which guards this.governance_vote_processing_tests.cpp: both PRs add cases at the same spot. fix: preserve and bound orphan governance votes #7819's new test still callsGetOrphanVoteObjectHashes(), which this PR removes.I merged #7819's head into this branch with that resolution. On the result, the
governance_*,cachemultimap_tests,net_tests,denialofservice_tests,txrequest_testsandpeerman_testsunit suites pass, including both PRs' new tests.How Has This Been Tested?
Built on aarch64-apple-darwin with
--enable-debug --enable-werror --enable-suppress-external-warnings. Each commit was built and passed the unit tests below.New or changed unit tests in
src/test/governance_vote_processing_tests.cpp(existing fixture with a registered masternode):orphan_vote_parent_is_fetched_from_the_peers_relaying_the_vote: two peers relay the same orphan vote throughNetGovernance.SendMessagesqueues aGETDATAfor the parent to the first peer. After the first peer disconnects, theGETDATAgoes to the second. The second serves the object, and the orphan vote is applied. Fails ondevelop. Also fails if the parent hash is returned only for a newly stored vote.orphan_votes_are_limited_per_masternode: fills one masternode's share throughProcessVote(). Its next vote, for a real proposal, is dropped with no parent to fetch, and it is not applied when the proposal arrives (yes count 0). A repeat of a held vote still returns its parent. The share is still in effect after agovernance.datround trip, and it frees up once the orphans expire andCheckAndRemove()runs. Fails ondevelop.orphan_vote_eviction_frees_the_masternode_share:OrphanVoteCachewith bounds 2/2. A global eviction returns the evicted masternode's share.governance_inv_tests/orphan_votes_require_a_valid_masternode_signaturenow checks that noGETDATAis sent instead of checking for noMNGOVERNANCESYNC. TwoGetOrphanVoteObjectHashes()checks inorphan_vote_is_cached_and_applied_when_parent_arrivesare removed: the test's yes-count check already shows the vote was held and applied.Unit tests run: the full
test_dashsuite at the tip, andgovernance_*,net_tests,denialofservice_tests,txrequest_tests,peerman_testsat each commit.Functional tests run:
feature_governance.py --descriptors,feature_governance_cl.py --descriptors,feature_governance_objects.py,feature_governance_txindex_devnet.py,p2p_govsync_bloom.py,p2p_instantsend.py.Lint:
lint-whitespace.py,lint-includes.py,lint-circular-dependencies.py, andclang-format-diffon the changed lines.Breaking Changes
None. No consensus, P2P message or
governance.datformat change. Nodes no longer send theobject_fetchform ofMNGOVERNANCESYNC, but they still answer it.Checklist:
🤖 Generated with Claude Code