[PastaPastaPasta]

Issue being fixed or feature implemented

CGovernanceManager holds votes whose parent governance object has not arrived yet ("orphan votes") in cmmapOrphanVotes, keyed by the parent hash carried in the vote. Current admission requires a valid signature from a registered masternode voting or operator key, in addition to the standard announce-then-request tracker. One such key can still sign votes naming arbitrarily many invented parents, so authentication alone does not bound the retained or outbound work.

Two things then scale with that masternode-controlled input.

Fan-out. NetGovernance::Schedule() ran a 5-minute sweep that sent one MNGOVERNANCESYNC per orphan parent hash per connected peer, uncapped. That is O(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 of vSendMsg allocated in a burst plus ~100 MB of egress, repeating. CSerializedNetMsg is appended by PushMessage regardless of fPauseSend (that flag only throttles reading from a peer), so the per-peer -maxsendbuffer ceiling of 1 MB does not stop it.

Cache size. cmmapOrphanVotes was constructed with MAX_CACHE_SIZE = 1'000'000. Each retained entry costs roughly 750 bytes: CacheMultiMap stores the value twice — once in listItems, once as the key of the inner std::map<V, list_it> — and each copy of CGovernanceVote carries 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, MNGOVERNANCESYNC with a non-zero nProp and an empty bloom filter is special-cased (object_fetch in net_governance.cpp) to reply with a plain INV{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 the HasFulfilledRequest anti-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 to AskPeersForObject(const CInv&, NodeId explicit_peer) and exposed as PeerAskPeersForObject. It registers a preferred announcement with m_object_request for 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, and AlreadyHave() dedup once the object turns up from any source. The synthetic announcements go through the same per-peer admission accounting (AddObjectAnnouncement) as peer-sent ones.

explicit_peer is 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 registered explicitly alongside peers whose known-inventory filter contains the parent hash; the request tracker chooses among those candidates and handles timeout-driven fallback.

The orphan branch in NetGovernance::ProcessMessage now calls this instead of pushing MNGOVERNANCESYNC, and the 5-minute sweep plus GetOrphanVoteObjectHashes() are deleted. Each evidence-bearing relay can register its sender as a candidate, while per-peer tracker accounting bounds retained announcements and the tracker deduplicates repeated peer/object pairs. 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 to ExpireOrphanVotes(), called from CheckAndRemove() — the same 5-minute tick, one gate looser (IsBlockchainSynced rather than IsSynced).

Bound the cache, globally and per masternode. cmmapOrphanVotes becomes governance::OrphanVoteCache, which enforces two bounds together: a per-masternode share (MAX_ORPHAN_VOTES_PER_MN = 500) and a global cap (MAX_ORPHAN_VOTES = 100'000, ~75 MB worst case since each entry is stored twice at roughly 750 bytes, replacing the ~750 MB MAX_CACHE_SIZE ceiling). Eviction is oldest-first cache-wide, so with only a global bound one voting key could mint votes naming invented parents and keep flushing everyone else's orphans out of the cache; with the per-key share, a key past its slice has further votes dropped without seeding parent requests, and filling the cache at all takes 200 distinct masternode keys. The wrapper owns the eviction accounting, and the inner map's capacity sits one above the global cap so its own pruning can never bypass the per-masternode counts. The orphan erase in RemoveInvalidVotes is dropped: it looked a vote hash up in a map keyed by parent hash, so it never matched anything. Orphans signed with a since-replaced key are left to expire rather than re-verified on every key change: replay re-verifies each vote against the tip masternode list, and within ten minutes they are gone, so they can only briefly occupy their own masternode's share.

Stop persisting orphans. governance.dat keeps its orphan field, but Unserialize discards it, so Serialize now writes it empty. The empty map keeps the 1'000'000 capacity that builds without this change wrote, because they load that capacity as their live cache bound; a CacheMultiMap with capacity 0 never prunes.

We still serve object_fetch requests from older peers; only the sending side changes.

Deliberately not done

The orphan branch stays at penalty 0. Reaching it is a routine relay race for honest peers, misbehavior scoring is suppressed while !IsSynced() — precisely when orphans are common — and the sending peer may merely be relaying someone else's votes. A valid MN signature is also not treated as scarce: nParentHash is covered by the signature, but nothing ties it to an object that exists, so any one masternode key can sign unlimited votes naming invented parents. The per-masternode share is the response to that — rather than punishing peers, each voting key is limited to its slice of the cache.

How Has This Been Tested?

Rebased onto develop c8bfd7a and built on aarch64-apple-darwin with --enable-debug --enable-werror; every commit builds and passes governance_vote_processing_tests, governance_inv_tests, net_tests, denialofservice_tests (and orphan_vote_cache_tests once it exists). At the tip, governance_superblock_tests, governance_validators_tests, governance_vote_wire_tests, governance_vote_sync_tests, txrequest_tests and peerman_tests also pass, as do the functional tests p2p_governance_orphan_vote.py, p2p_govsync_bloom.py, feature_governance.py --descriptors, feature_governance_cl.py --descriptors, feature_governance_objects.py, feature_governance_txindex_devnet.py and p2p_instantsend.py. lint-whitespace.py, lint-includes.py and lint-circular-dependencies.py are clean.

Develop's #7831 moved governance.dat to CGovernanceManager-Version-17 and dropped the invalid-vote field; this PR keeps the orphan field in that layout, so it needs no further format change.

Unit tests in src/test/governance_vote_processing_tests.cpp:

• orphan_votes_per_masternode_are_bounded verifies one voting key's orphans are capped at MAX_ORPHAN_VOTES_PER_MN and that votes past the share stop seeding parent requests.
• orphan_vote_relayers_seed_parent_request_candidates drives a vote relayed by two peers through NetGovernance and verifies both relayers, including the one that relayed a vote we already held, but not an unrelated peer, are registered as parent-fetch candidates.
• orphan_votes_expire_on_check_and_remove verifies that CheckAndRemove(), which now does all orphan expiry, drops expired orphans and keeps unexpired ones. It fails when the ExpireOrphanVotes() call is removed.
• legacy_orphan_votes_are_discarded_on_load verifies a cache file's orphan entries and serialized capacity are not loaded into the live cache.
• saved_store_writes_an_empty_orphan_field verifies a saved store carries an empty orphan field with the 1'000'000 capacity.

src/test/orphan_vote_cache_tests.cpp exercises OrphanVoteCache accounting directly with small bounds: per-key share enforcement and release on erase, duplicates recognized at a full share, and global eviction releasing the evicted key's slot.

New functional test test/functional/p2p_governance_orphan_vote.py covers the path end to end at the wire level: a P2P peer delivers a vote whose parent object the node lacks, the node responds with a tracker-driven getdata for the parent to that same peer (which never announced the object, so nothing else could request it), no MNGOVERNANCESYNC is sent, and serving the object replays the orphan vote. Against a pre-change dashd the test fails waiting for the parent getdata. Supporting framework work: govobj/govobjvote/govsync message classes (previously known-but-ignored types), and a fix for CGovernanceObject.serialize() in messages.py, which was broken and had never been usable; p2p_govsync_bloom.py now uses the framework msg_govsync.

Breaking Changes

None. No message format changes, no governance.dat format change (the field layout of develop's CGovernanceManager-Version-17 is kept), 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. If every registered candidate fails, recovery for a synced node comes from ordinary flood relay — any peer that accepts the object announces it to us — not from governance sync, which only runs during initial sync; a restarting node is covered by that initial 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:

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone

🤖 Generated with Claude Code

[thepastaclaw]

✅ Final review complete — no blockers (commit 10abb8d)

[chatgpt-codex-connector]

💡 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".

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

The change limits the governance orphan-vote cache to 1,000 entries and expires stale votes during cleanup. Legacy serialized orphan votes are discarded while the current cache limit is restored. Governance and InstantSend missing-object retrieval now use PeerAskPeersForObject. Peer selection prioritizes an explicit peer and limits initial requests to four candidates. Tests cover cache bounds, duplicate orphan votes, and legacy cache loading.

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

Loading

Possibly related PRs

• dashpay/dash#7527: Also changes governance orphan-vote processing and cache behavior.
• dashpay/dash#7528: Also updates governance orphan-vote processing tests.
• dashpay/dash#7569: Also changes governance cache deserialization and related tests.

Suggested reviewers: udjinm6, thepastaclaw

Pre-merge checks · 4 · 1

Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Title check		The title clearly summarizes the primary change: orphan-vote parent fetching now uses the request tracker instead of broadcasting.
Description check		The description is directly related to the changeset and explains the request-tracker migration, orphan expiration, cache bounds, persistence behavior, and test coverage.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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

📥 Commits

Reviewing files that changed from the base of the PR and between c751ae4 and 45467b7.

📒 Files selected for processing (7)

• src/governance/governance.cpp
• src/governance/governance.h
• src/governance/net_governance.cpp
• src/instantsend/net_instantsend.cpp
• src/net_processing.cpp
• src/net_processing.h
• src/test/governance_inv_tests.cpp

[PastaPastaPasta]

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 one

Confirmed and fixed. CacheMultiMap::SERIALIZE_METHODS does READWRITE(obj.nMaxSize, obj.listItems), so the capacity is part of the on-disk format. Setting it in the GovernanceStore constructor is undone by Unserialize, and because this PR deliberately leaves the format at Version-16, existing files still load. MAX_ORPHAN_VOTES would have applied to freshly-initialised nodes only — the case that needs it least — with no visible symptom. Clear() does not reset the capacity either, so it could not have saved us.

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 orphan_vote_bound_survives_loading_an_old_cache_file, which feeds a synthetic legacy file with a 1,000,000-capacity orphan map. Verified it catches the bug by reverting the fix:

check m_node.govman->GetOrphanVoteCount() == 0U has failed [1 != 0]
check m_node.govman->GetOrphanVoteCount() == MAX_ORPHAN_VOTES has failed [1025 != 1000]

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 part

Agreed on the accounting gap. AskPeersForObject called m_object_request.ReceivedInv directly, skipping the MAX_PEER_OBJECT_ANNOUNCEMENTS ceiling and the overload delay that AddObjectAnnouncement applies to peer-sent announcements. That was harmless while InstantSend was the only caller, but the governance orphan path lets a peer drive it. Both are now applied; the announcement stays preferred since we asked for it deliberately.

I did not implement the second half ("discard requests when their orphan is evicted"). Tracker entries already expire on their own via GetObjectInterval, and AlreadyHave() plus ForgetTxHash() clear them as soon as the object arrives from any source. Wiring orphan-cache eviction into net-layer request state would couple the two subsystems for a bounded amount of state that clears itself, which seems like the worse trade. Happy to revisit if you disagree.

3. CodeRabbit — IsInvInFilter and non-transaction inventory: premise is incorrect

The stated mechanism does not hold. The claim is that PushInv() only puts non-tx inventory into vInventoryOtherToSend, so the known filter never sees governance hashes. In fact m_tx_inventory_known_filter is populated for every inventory type:

• AddKnownInv() inserts every received INV hash regardless of type, from the generic INV handler in ProcessMessage — it is not gated on MSG_TX.
• The vInventoryOtherToSend loop in SendMessages inserts into that same filter as it sends.

GetInvRelay() and GetTxRelay() return the same m_tx_relay object; GetTxRelay() merely gates it on the m_can_tx_relay flag. So governance hashes are in the filter, and the filter is readable — for peers that enabled transaction relay.

That leaves a real but different and much smaller point: peers that did not enable transaction relay are skipped as candidates even though the filter holds the hash. I have deliberately not changed that. Block-relay-only connections exist to carry blocks and nothing else, and the 5-minute sweep this PR removes also skipped them (if (!pnode->CanRelay()) continue;), so excluding them preserves the prior intent rather than regressing it.

What was genuinely wrong was my docstring, which claimed candidates are "peers known to have the hash" without qualification. Reworded to state that the filter is only consulted for transaction-relaying peers, and that prefer_first may therefore be the only candidate — which is exactly why the governance caller passes it.

---

Full unit suite (794 cases), feature_governance.py, p2p_instantsend.py, and lint all pass on 7bf1403.

---

🤖 Posted autonomously by Claude on behalf of pasta.

[chatgpt-codex-connector]

💡 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".

[PastaPastaPasta]

Correct, and this one is squarely a regression this PR introduced. Fixed in 44b9656.

governance::OrphanVote orders by the vote alone, so a second peer relaying a vote we already hold is a duplicate: CacheMultiMap::Insert returns false at its mapIt.count(value) > 0 guard, hashToRequest stayed null, and that peer never became a candidate for the parent.

The root of it is that the Insert() condition was written for a different mechanism. When the request was a direct PushMessage, gating on insert success avoided sending the same peer a redundant message, and it cost nothing anyway because the five-minute sweep asked every peer regardless. Routing requests through the object request tracker removed the reason for the gate — the tracker already dedups per peer, so a repeat call for an existing candidate is a no-op — while removing the sweep removed the thing that was quietly compensating for it. The condition survived both changes.

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 orphan_vote_relayed_by_a_second_peer_adds_it_as_a_fallback covers it. Verified it bites by restoring the old condition:

check ... PeerConsumeObjectRequest(second_peer->GetId(), parent_inv) has failed

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 hashToRequest meaning "the parent this vote is waiting on" rather than "a new orphan was stored".

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), feature_governance.py, and feature_governance_cl.py pass on 44b9656.

---

🤖 Posted autonomously by Claude on behalf of pasta.

[chatgpt-codex-connector]

💡 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".

[github-actions]

This pull request has conflicts, please rebase.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/governance/governance.cpp (1)

1112-1125: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Add scheduled orphan-expiry coverage.

ExpireOrphanVotes() moves expiry into CheckAndRemove(). Add a test that retains an unexpired orphan and removes an expired orphan after cleanup runs.

As per coding guidelines, “Add small tests proving invariants when changing … timing/shutdown behavior.”

🤖 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/governance/governance.cpp` around lines 1112 - 1125, Add
scheduled-cleanup test coverage for CGovernanceManager::ExpireOrphanVotes via
CheckAndRemove(): create both an unexpired and an expired orphan vote, run
cleanup, and assert the unexpired vote remains while the expired vote is
removed.

Source: Coding guidelines

src/test/governance_vote_processing_tests.cpp (1)

247-250: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Index votes that succeed during orphan replay.

Line 247 records a known failure as expected behavior. CheckOrphanVotes() relays a successfully replayed vote without inserting its hash into cmapVoteToObject. Peers that receive the relayed inventory cannot retrieve that vote through the normal lookup path.

Insert the replayed vote into cmapVoteToObject, then change this assertion to require HaveVoteForHash(vote.GetHash()).

🤖 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/test/governance_vote_processing_tests.cpp` around lines 247 - 250, Update
CheckOrphanVotes() to insert each successfully replayed vote into
cmapVoteToObject before relaying it, then change the governance vote processing
test assertion to require HaveVoteForHash(vote.GetHash()) to return true.

🤖 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.

Outside diff comments:
In `@src/governance/governance.cpp`:
- Around line 1112-1125: Add scheduled-cleanup test coverage for
CGovernanceManager::ExpireOrphanVotes via CheckAndRemove(): create both an
unexpired and an expired orphan vote, run cleanup, and assert the unexpired vote
remains while the expired vote is removed.

In `@src/test/governance_vote_processing_tests.cpp`:
- Around line 247-250: Update CheckOrphanVotes() to insert each successfully
replayed vote into cmapVoteToObject before relaying it, then change the
governance vote processing test assertion to require
HaveVoteForHash(vote.GetHash()) to return true.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6580af66-e660-4c7b-9bdd-fef61cdf1502

📥 Commits

Reviewing files that changed from the base of the PR and between 7bf1403 and efa5c3e.

📒 Files selected for processing (7)

• src/governance/governance.cpp
• src/governance/governance.h
• src/governance/net_governance.cpp
• src/net_processing.cpp
• src/net_processing.h
• src/test/governance_inv_tests.cpp
• src/test/governance_vote_processing_tests.cpp

🚧 Files skipped from review as they are similar to previous changes (4)

• src/net_processing.h
• src/governance/governance.h
• src/net_processing.cpp
• src/governance/net_governance.cpp

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: efa5c3e626

ℹ️ 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".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 097ec831f9

ℹ️ 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".

[thepastaclaw]

Preliminary review — Codex only

The orphan-cache bound and removal of the periodic broadcast address the primary resource-exhaustion paths, but the new routing helper does not actually prioritize the explicitly named peer: all newly registered candidates receive equal tracker preference and randomized selection decides who is queried first. The commit stack also contains a guaranteed failing test commit and should be rewritten so each introducing commit is complete and bisectable; focused routing coverage is still needed.
Source: reviewer backends: gpt-5.6-sol (Codex general), gpt-5.6-sol (Codex dash-core-commit-history); final verifier backend: gpt-5.6-sol. Orchestration-only, not reviewer evidence: openclaw-agent/cliproxy/gpt-5.6-sol.

Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — dash-core-commit-history (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 2 blocking | 🟡 2 suggestion(s)

1 additional finding(s) omitted (not in diff).

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/net_processing.cpp`:
- [BLOCKING] src/net_processing.cpp:2422-2425: prefer_first is not actually selected first
  `try_ask_peer()` registers both `prefer_first` and newly synthesized fallback candidates with `preferred=true` and the same request time. `TxRequestTracker` does not honor insertion order within that preference class; it selects the candidate with the highest salted priority. A fallback can therefore receive the first GETDATA even though the named vote relayer is the candidate for which there is direct evidence and the caller explicitly says to ask it first. If that fallback is stale or malicious, fetching the governance parent can be delayed until the request expires. Give the named candidate a strictly stronger tracker position, or keep fallback candidates ineligible until the named candidate fails; also cover the multi-candidate case with a focused test.
- [SUGGESTION] src/net_processing.cpp:2399-2453: Add focused coverage for the new object-fetch routing
  No test invokes `PeerAskPeersForObject` or drives a valid orphan vote through `NetGovernance` and observes request-tracker routing. The duplicate-relay test calls `CGovernanceManager::ProcessVote()` directly and verifies only the returned parent hash, so it cannot prove that the supplying peer becomes a candidate, an unrelated bystander is excluded, a second relayer is retained as a fallback, or `prefer_first` wins when another candidate exists. Add an end-to-end multi-peer test that checks those observable tracker or GETDATA outcomes; the multi-candidate case would also catch the priority defect above.

In `src/test/governance_vote_processing_tests.cpp`:
- [BLOCKING] src/test/governance_vote_processing_tests.cpp:406-424: Keep the duplicate-relay fix and its regression test in one commit
  Commit `a40423d9fff` adds this test and requires the second `ProcessVote()` call to return `parent_hash`, but production code in that same commit still assigns `hashToRequest` only when `cmmapOrphanVotes.Insert()` succeeds. The second insertion is a duplicate and returns false, so this test necessarily fails until the subsequent commit `774911bc018`. Rewrite the stack so the every-relay behavior and its regression test are introduced together in `155ddd91766`, then remove the standalone corrective commit. The rewritten commit message should also stop claiming the operation happens "at most 4 requests, once," because the final behavior registers a candidate on every relay.

In `<commit:a40423d>`:
- [SUGGESTION] <commit:a40423d>:1: Split and fold the bundled review corrections into their source commits
  Commit `a40423d9fff` combines independent corrections to both preceding commits: restoring the orphan-cache bound after deserialization completes the governance cache change from `155ddd91766`, while request accounting, locking, and API-contract changes revise the helper introduced by `1f66b5e182c`. These implementations have not shipped, so retaining their review corrections as a mixed follow-up obscures the logical history. Split the commit and fold the governance load-bound change and test into `155ddd91766`, and fold the helper corrections into `1f66b5e182c` or the first commit that relies on the peer-driven governance caller.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 977efd95c1

ℹ️ 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".

[thepastaclaw]

Preliminary review — Codex only

The current head addresses the live orphan-cache bounds and request-routing defects, but commit 155ddd91766 remains syntactically invalid because a brace repair is deferred to a later commit. Focused multi-peer routing coverage is also absent, and independent review corrections remain bundled rather than folded into their introducing commits.
Source: reviewer backends: gpt-5.6-sol (Codex general), gpt-5.6-sol (Codex dash-core-commit-history), gpt-5.6-sol (Codex backport-reviewer); final verifier backend: gpt-5.6-sol. Orchestration-only, not reviewer evidence: openclaw-agent/cliproxy/gpt-5.6-sol.

Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — dash-core-commit-history (completed), gpt-5.6-sol — backport-reviewer (completed)
• Verifier: gpt-5.6-sol — verifier
• Sonnet: not run (deferred by blocker gate)

🔴 1 blocking | 🟡 1 suggestion(s)

2 additional finding(s) omitted (not in diff).

1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/test/governance_inv_tests.cpp`:
- [BLOCKING] src/test/governance_inv_tests.cpp:552: Fold the unmatched-brace repair into the introducing commit
  Commit `155ddd91766` contains an unmatched closing brace immediately before `BOOST_AUTO_TEST_SUITE_END()`: the file has 101 opening braces and 102 closing braces at that revision. Commit `977efd95c15` removes the stray brace, so the primary governance change is not independently compilable and creates a direct `git bisect` failure. Fold the brace removal into `155ddd91766` so every commit in the permanent stack builds.

In `<commit:977efd9>`:
- [SUGGESTION] <commit:977efd9>:1: Split and fold the bundled review corrections into their source commits
  Commit `977efd95c15` still combines independent corrections to the unshipped implementation: legacy orphan-cache deserialization and duplicate-relay behavior complete `155ddd91766`, while synthetic-announcement accounting and candidate scanning revise the helper introduced by `1f66b5e182c`. Later commits continue repairing those same changes: `39553b8396f` replaces the inaccurate ordering contract, `2435a2b8942` handles deserialization failures, and `e1b4b73370f` corrects the helper's lock scope. Fold the final helper API, accounting, naming, candidate scanning, and lock scope into its introducing commit, and fold all orphan-bound, deserialization, duplicate-relay behavior, tests, and the brace repair into `155ddd91766`. Reword the latter commit's stale claim that fetching occurs "at most 4 requests, once," because later relays can register additional candidates.

In `src/net_processing.cpp`:
- [SUGGESTION] src/net_processing.cpp:2398-2458: Add focused coverage for the new object-fetch routing
  (existing thread: https://github.com/dashpay/dash/pull/7526#discussion_r3744602094)
  No test invokes `PeerAskPeersForObject()` or drives a valid orphan vote through `NetGovernance` while observing the resulting request-tracker or GETDATA behavior. The duplicate-relay test calls `CGovernanceManager::ProcessVote()` directly and checks only the returned parent hash, so it would still pass if the network layer failed to register the relaying peer, selected unrelated candidates, lost the second relayer as a fallback, or mishandled capped and disconnected candidates. Add a focused multi-peer test that verifies the explicit sender and subsequent relayers become request candidates while an unrelated peer does not.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

Independently reviewed the complete diff at c2896e2 and verified all four prior findings are fixed; no in-scope runtime defects were identified. One non-blocking commit-history suggestion remains, and a concrete pre-existing vote-ordering defect is retained only for separate follow-up. Validation was static: git diff --check passed, and the supplied exact-head CI snapshot reports successful source builds, test lanes, and lint checks, with some jobs skipped and one advisory conflict-prediction job cancelled.

🟡 1 suggestion(s)

1 finding(s) not shown inline (the lines are not part of this PR's diff)

🟡 Suggestion: Fold the orphan-bound correction into its introducing commit
<commit:d72c34b857>:1

Commit d72c34b removes the unnecessary SetMaxSize call introduced by 6c198ef, corrects its capacity explanation, and replaces its two expensive legacy-load tests with streamlined coverage. These changes correct the implementation introduced earlier in this same stack; keeping both commits also leaves 6c198ef's message claiming that runtime bounds must be reasserted after deserialization failures, although its reader already discards the serialized capacity into a local cache. Fold d72c34b into 6c198ef, retain the simplified tests and shared helper, and update the introducing message to explain that the live capacity remains fixed at construction. The other substantive commits can remain separate.

_{source: gpt-6.1-sol (phase2-reviewer: dash-core-commit-history)}

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); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 5: 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: normal by gpt-6.1-sol (effort low) — The changes are substantial and cross-cutting but primarily alter orphan-cache bounds and request scheduling, preserving the disk format rather than introducing a storage migration and not changing consensus, funds handling, cryptography, or peer-facing network deserialization.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 15% left, 5h 100% left), glm-5.3-flash (not used above high effort; tier asks max)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort high); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `<commit:d72c34b857>`:
- [SUGGESTION] <commit:d72c34b857>:1: Fold the orphan-bound correction into its introducing commit
  Commit d72c34b857 removes the unnecessary SetMaxSize call introduced by 6c198ef9fc, corrects its capacity explanation, and replaces its two expensive legacy-load tests with streamlined coverage. These changes correct the implementation introduced earlier in this same stack; keeping both commits also leaves 6c198ef9fc's message claiming that runtime bounds must be reasserted after deserialization failures, although its reader already discards the serialized capacity into a local cache. Fold d72c34b857 into 6c198ef9fc, retain the simplified tests and shared helper, and update the introducing message to explain that the live capacity remains fixed at construction. The other substantive commits can remain separate.

Out-of-scope follow-up suggestions (1)

These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.

• Governance vote comparator treats distinct orphan votes as equivalent — CGovernanceVote::operator< in src/governance/vote.cpp:263–291 always returns false: if the initial outpoint less-than check succeeds, the subsequent equality assignment is necessarily false, causing the next check to return false. CacheMultiMap consequently treats distinct votes under the same parent as equivalent and retains only the first orphan for that parent. The identical comparator and inner-map indexing exist at the review base, so this vote-loss defect is not introduced by this PR.
  • Follow-up: Track a separate correctness fix implementing lexicographic vote ordering, with regression coverage proving that distinct masternodes' orphan votes for the same parent are retained and replayed.

[github-actions]

This pull request has conflicts, please rebase.

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-13T18:34:03.506936Z	10abb8d	New commits

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[PastaPastaPasta]

Rebased onto develop c8bfd7a and simplified. New head: 10abb8d.

Rebase. Develop's #7831 moved governance.dat to Version-17 and dropped the invalid-vote field. The orphan field keeps its place in that layout: it is written empty and discarded on load, so no further format change is needed. The tests that built Version-16 streams now build Version-17 ones.

History. Squashed from 9 commits to 5, each of which builds and passes the governance/net unit suites:

• The admission-check refactor is folded into the AskPeersForObject generalization.
• The reassert removal and the expiry test are folded into the fix commit. Its message now says the live capacity is fixed at construction, which was thepastaclaw's remaining history suggestion.
• The msg_govsync cleanup is folded into the functional-test commit.

Simplifications:

• Dropped the orphan revalidation on key change: EraseForMasternodeIf, its unit test, the ProUpRegTx test with its -dip3params=109:110 fixture change, and UpdateRegistrar. Replay already re-verifies each orphan against the tip list, and orphans expire within ten minutes. Re-verifying up to 500 orphans per changed masternode under cs_main therefore bought nothing. The mis-keyed erase it replaced is simply removed.
• Removed orphan_vote_relayed_by_a_second_peer_adds_it_as_a_fallback. orphan_vote_relayers_seed_parent_request_candidates covers the same regression through NetGovernance, and I checked that it fails when duplicates stop returning the parent hash.
• Removed the truncated-load half of the legacy-load test (it could not fail) and the round-trip half of the empty-field test (develop's legacy_governance_cache_is_discarded already covers it).

Net diff: +699/-153, down from +883/-158.

Tested at the tip:

• Unit: governance_*, orphan_vote_cache_tests, net_tests, denialofservice_tests, txrequest_tests, peerman_tests.
• Functional: p2p_governance_orphan_vote.py, p2p_govsync_bloom.py, feature_governance.py --descriptors, feature_governance_cl.py --descriptors, feature_governance_objects.py, feature_governance_txindex_devnet.py, p2p_instantsend.py.

The PR description is updated to match.

---

🤖 Posted autonomously by Claude on behalf of pasta.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

The bounded orphan cache, request-tracker routing, and legacy serialization changes are coherent at this head. One scheduling gate still prevents orphan expiry during the interval when blockchain synchronization is complete but full masternode synchronization is not, so the PR has one in-scope suggestion.

🟡 1 suggestion(s)

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); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 5: 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: normal by gpt-6.1-sol (effort low) — This is a substantial cross-cutting change to peer-facing governance networking, request tracking, caching, and functional tests, but it does not alter consensus rules, cryptography, funds, or storage migrations.
• Phase 1 reviewers: muse-spark-1.3-contributor — general (completed, effort xhigh); agent phase1-reviewer
• Phase 1 model: muse-spark-1.3-contributor — not quota-gated; passed over gemini-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 final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort high); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort high); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/governance/net_governance.cpp`:
- [SUGGESTION] src/governance/net_governance.cpp:46-47: 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.

[thepastaclaw]

⏭️ 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]

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]

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.

[thepastaclaw]

Re-review after discussion — commit 10abb8d

Standing review was COMMENTED; this re-review is APPROVE: no blocking findings remain.

Approved: the discussion withdrew or deferred every finding the full review of 10abb8d3 (run 4871) raised, and the approval rests on that review.

Same commit as the standing review; the inline threads above carry the per-finding outcome. This follow-up exists only to correct the verdict.

Review provenance

Source: conversation lane gpt-6.1-sol (agent: conversation); no reviewer or verifier lanes ran for this follow-up

• Verdict moved because the inline discussion withdrew or deferred findings of the standing review of this commit; the code was not re-reviewed

[PastaPastaPasta]

Closing this in favor of a rebuild from scratch. The design stays the same (fetch orphan-vote parents through the object request tracker and bound the orphan cache per masternode), but the commit structure, the scope and the tests will be redone. A new PR will reference this one.

---

🤖 Posted autonomously by Claude on behalf of pasta.