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 8 commits into
Conversation
|
✅ Final review complete — no blockers (commit 082f399) |
![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 👍 / 👎.
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe 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 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 | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
|
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 👍 / 👎.
There was a problem hiding this comment.
Addressed some time ago and further hardened in 082f399. ProcessVote() assigns hashToRequest for an orphan whether or not the cache insert was new, so every relay of a still-parentless vote registers its sender as a fallback source — covered by orphan_vote_relayed_by_a_second_peer_adds_it_as_a_fallback. As of 082f399 that also holds when the voting masternode is at its per-masternode share, since a cached pair is now reported as a duplicate rather than being turned away by the share check.
🤖 Posted autonomously by Claude on behalf of pasta.
|
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 👍 / 👎.
… an orphan governance vote 1e37fc4 fix: only accept the voting key for funding votes in the orphan-vote gate (pasta) 1a51025 fix: require a valid masternode signature before caching an orphan vote (pasta) Pull request description: ## Issue being fixed or feature implemented `CGovernanceManager::ProcessVote()` caches votes whose parent governance object is not yet in `mapObjects` ("orphan votes") into `cmmapOrphanVotes`, keyed by the vote's `nParentHash`. Today the insert happens **before any masternode-membership or signature check**, and the exception raised carries a zero misbehaviour penalty. The only thing standing between a peer and that cache is the announce-then-request tracker in `src/governance/net_governance.cpp` (the peer has to INV the vote hash first). Nothing about the vote's contents is verified. So a peer can put arbitrary unvalidated attacker-chosen data into a node's governance cache and pay nothing for it, and the node will additionally emit an `MNGOVERNANCESYNC` request for the invented parent hash. Caching unverified peer data is the wrong default regardless of how much of it fits. Note also that the same garbage vote is scored differently depending on whether its parent object happens to have arrived: with a parent present, `CGovernanceObject::ProcessVote` rejects an unknown masternode with `GOVERNANCE_EXCEPTION_PERMANENT_ERROR` / penalty 20; without a parent, the identical vote is silently cached with penalty 0. ## What was done? In the orphan branch of `CGovernanceManager::ProcessVote`, require the vote to carry a valid signature from a masternode present in the tip list before it may enter `cmmapOrphanVotes`: ```cpp if (!vote.IsValidForUnknownParent(tip_mn_list)) { // GOVERNANCE_EXCEPTION_PERMANENT_ERROR, penalty 20 } ``` Notes on the specifics: * **The existing validator is called rather than re-implementing its checks inline.** `CGovernanceVote::IsValid` already performs the future-time check, the signal/outcome bounds checks, the `GetMNByCollateral` lookup and the signature verification. Duplicating those inline would guarantee they drift apart from the known-object path over time. * **Key selection is signal-aware** (`CGovernanceVote::IsValidForUnknownParent`). Which key is correct depends on the parent object's type and the vote signal (`onlyVotingKeyAllowed` in `CGovernanceObject::ProcessVote`): only `PROPOSAL` + `VOTE_SIGNAL_FUNDING` may ever use the voting key; every other signal requires the operator BLS key for every object type. So for a funding vote — whose parent type is by definition unknown on this path — either key is accepted, while all other signals are checked against the operator key only. This matters because the voting key is the lower-trust credential (routinely delegated to third-party voting services): without the signal check, a voting-key holder could cache non-funding votes that can never validate once their parent arrives. A funding vote on a non-proposal object still gets re-checked against the operator-key requirement at replay time. * **Penalty 20 / `GOVERNANCE_EXCEPTION_PERMANENT_ERROR`** matches exactly what `CGovernanceObject::ProcessVote` already applies for an unknown masternode or a failed `IsValid` on the known-object path, so the same bad vote now costs the sender the same either way. * **The orphan branch itself stays at penalty 0.** Once the gate passes, reaching that branch means the vote is signed by a masternode and the only reason it cannot be applied is that its parent has not arrived — a benign relay race that happens routinely during governance sync. Misbehaviour scores never decay, so scoring there would eventually disconnect honest relays. * **Gate rejections are deliberately not inserted into `cmapInvalidVotes`.** That would make replays cheaper to reject, but `cmapInvalidVotes` is sized `MAX_CACHE_SIZE = 1'000'000` and caching gate rejections would create a *new* unauthenticated path for filling it with attacker-chosen entries — i.e. exactly the class of problem this change is meant to reduce. * `m_dmnman.GetListAtChainTip()` is hoisted to the top of `ProcessVote` so both the orphan gate and the known-object path share a single call; previously it was fetched inline at the `govobj.ProcessVote` call site. **On verifying signatures under `cs_store`:** this is not a new class of work under that lock. The known-object path already does exactly this — `CGovernanceManager::ProcessVote` holds `cs_store` across `govobj.ProcessVote(...)`, which calls `vote.IsValid(...)` at `src/governance/object.cpp:458`. This change applies the established pattern to the orphan branch. It does add up to two verifications for a vote that fails both, but only on the orphan path and only for peers that already passed the announce-then-request gate. ### What this does and does not fix This is a validation change. It does **not** close the underlying resource-exhaustion issue on `cmmapOrphanVotes`, for four reasons worth stating plainly: 1. **A valid masternode signature is not scarce.** `nParentHash` *is* covered by the signature (see `GetSignatureString()` and the `SER_GETHASH` serialization in `src/governance/vote.h`), but nothing ties the signed parent hash to an object that actually exists. Any one of the ~4000 masternode keys can sign an unbounded number of votes naming invented parent hashes, and each one lands in a distinct cache slot. 2. **That path is penalty-0 by design** (see above), so a flood of well-signed orphan votes is unscored on purpose. 3. **Misbehaviour scoring is suppressed while `!IsSynced()`** — see the `m_node_sync.IsSynced()` condition guarding `PeerMisbehaving` in `net_governance.cpp` — which is precisely the window in which orphan votes are most common. 4. **Per-masternode vote rate limiting is unreachable here.** `GOVERNANCE_UPDATE_MIN` is enforced inside `CGovernanceObject::ProcessVote`, i.e. after the parent lookup, and it is explicitly disabled on replay (`ScopedLockBool guard(cs_store, fRateChecksEnabled, false)` in `CheckOrphanVotes`). What it does buy: the cost of entry into the orphan cache goes from *free for any unauthenticated peer* to *requires a masternode key*, and garbage votes that previously vanished into the cache unscored are now scoreable — consistently with the known-object path. That is correct hygiene, but the bound on the data structure is what actually caps the damage. Bounding/expiring the cache is complementary work and is being handled separately in #7517 and #7526; this PR is intentionally independent of both and will conflict with them textually. One known side effect is deliberately left out of scope here. `CGovernanceVote::CheckSignature(const CBLSPublicKey&)` logs its failure with an unconditional `LogPrintf`, unlike its `CKeyID` sibling and unlike the rest of `IsValid`, which use `LogPrint(BCLog::GOBJECT, ...)`. Reaching it previously required a vote naming a governance object we actually have; after the gate, a vote naming an invented parent hash reaches it too, so a peer holding a real masternode outpoint (public data) plus a garbage signature can write a line to debug.log per message without `-debug` being set. Putting that log behind the `gobject` category is a one-word fix but touches an unrelated file, so it is not bundled here. ## How Has This Been Tested? Built with `--enable-debug --enable-suppress-external-warnings --without-gui` on aarch64-apple-darwin (clang). New unit tests in `src/test/governance_inv_tests.cpp`: * `orphan_votes_require_a_valid_masternode_signature` — a vote naming an outpoint that is not in the tip masternode list, delivered by a peer that legitimately announced it, does not enter the orphan cache (`GetOrphanVoteObjectHashes()` stays empty), triggers no `MNGOVERNANCESYNC` request for the invented parent, and scores the sender 20. * `invalid_vote_is_scored_alike_with_and_without_a_parent_object` — the same unauthenticated vote costs 20 whether or not its parent object is present, i.e. the orphan gate and `CGovernanceObject::ProcessVote` agree. Two existing tests were updated. `governance_votes_require_peer_announcement_or_request` and `governance_vote_authorization_survives_unsynced_drop` previously used "an `MNGOVERNANCESYNC` was emitted" as the observable proving that a vote reached `ProcessVote`; the votes they build carry a placeholder signature, so under this change they no longer reach the orphan branch and no such message is sent. They now use the misbehaviour score as the observable instead: a peer that passes the announce-then-request gate reaches `ProcessVote` and is scored 20, while a peer that fails the gate returns before `ProcessVote` and stays at 0. That is a stricter test of the authorization gate than the old one — it distinguishes "reached `ProcessVote`" from "did not" rather than relying on an incidental side effect. Both now advance `mn_sync` to `MASTERNODE_SYNC_FINISHED`, since penalties are only applied once `IsSynced()`. Coverage limit, stated plainly: `GovernanceInvSetup` is a `TestingSetup{MAIN}` fixture with no chain and therefore an empty deterministic masternode list, so `CGovernanceVote::IsValid` short-circuits on the `GetMNByCollateral` lookup before reaching `CheckSignature`. These tests therefore prove that the gate exists, runs on the orphan path, rejects a vote no masternode could have authored, and scores it identically to the known-object path — but they do not exercise `CheckSignature` itself, in either direction. Covering that (a registered masternode with a forged signature rejected, and one with a valid signature still accepted into the orphan cache) needs a chain-backed fixture with a real ProRegTx, which would mean rebuilding this fixture on `TestChainSetup` and is deliberately not attempted here. The positive path is covered end-to-end by `feature_governance.py`, which votes with real masternodes. The new assertions were verified to fail against unmodified code: with the change to `governance.cpp` reverted and the tests kept, the suite reports 7 failures, including `check m_node.govman->GetOrphanVoteObjectHashes().empty() has failed` and `check CountQueuedMessages(*peer, NetMsgType::MNGOVERNANCESYNC) == 0U has failed [1 != 0]`. Ran: * `./src/test/test_dash --run_test=governance_inv_tests` — passes (6 cases) * `./src/test/test_dash` — passes (794 cases) * `test/functional/test_runner.py feature_governance.py feature_governance_cl.py` — passes * `test/lint/lint-whitespace.py`, `test/lint/lint-circular-dependencies.py` — clean ## Breaking Changes None to consensus, RPC or the P2P wire format. Behavioural change on the P2P vote path: a governance vote whose parent object is unknown is now dropped instead of cached unless it carries a valid masternode signature, and a peer that sends such a vote is assigned a misbehaviour score of 20 (only while fully synced). A node that legitimately relays orphan votes ahead of their parent objects is unaffected, since those votes are validly signed. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] 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 _(for repository code-owners and collaborators only)_ Top commit has no ACKs. Tree-SHA512: 6f6a2ad05c896e3774c8ca15985cae3d43481565053f3aacf43336a0a4402e171bd8ca16e73b010c5b2d14f81dd6c00e1da8cbd9223d45a6ca6a2b8f53ac5e08
|
This pull request has conflicts, please rebase. |
44b9656 to
efa5c3e
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:
|
There was a problem hiding this comment.
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 winAdd scheduled orphan-expiry coverage.
ExpireOrphanVotes()moves expiry intoCheckAndRemove(). 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 winIndex 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 intocmapVoteToObject. 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 requireHaveVoteForHash(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
📒 Files selected for processing (7)
src/governance/governance.cppsrc/governance/governance.hsrc/governance/net_governance.cppsrc/net_processing.cppsrc/net_processing.hsrc/test/governance_inv_tests.cppsrc/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
There was a problem hiding this comment.
💡 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".
| // peer sent us. A synthetic announcement is still an entry the peer's behaviour can | ||
| // cause us to create -- a peer that keeps naming objects we do not have would otherwise | ||
| // grow its tracker footprint without limit. | ||
| if (m_object_request.Count(peer->m_id) >= MAX_PEER_OBJECT_ANNOUNCEMENTS) continue; |
There was a problem hiding this comment.
When any of the first four matching peers has reached MAX_PEER_OBJECT_ANNOUNCEMENTS, peersToAsk has already stopped filling before this later continue, so capped peers consume selection slots and eligible peers later in m_peer_map are never considered. In ProcessInstantSendLock, where prefer_first is unset, four capped early peers can therefore prevent a missing locked transaction from being requested even though another peer's inventory filter contains it; the governance fallback set is similarly narrowed. Filter capped peers while selecting, or continue scanning until four announcements are actually registered.
AGENTS.md reference: AGENTS.md:L170-L171
Useful? React with 👍 / 👎.
5c47901 to
097ec83
Compare
There was a problem hiding this comment.
💡 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".
| return true; | ||
| }; | ||
|
|
||
| READ_LOCK(m_peer_mutex); |
There was a problem hiding this comment.
When a peer is finalized concurrently with an orphan-parent or InstantSend transaction fetch, this read lock remains held while try_ask_peer() acquires cs_main, but FinalizeNode() acquires cs_main before calling RemovePeer(), which needs the exclusive m_peer_mutex. The resulting lock-order inversion can permanently deadlock networking during an ordinary peer disconnect; snapshot the candidate PeerRefs and release m_peer_mutex before updating the request tracker.
AGENTS.md reference: AGENTS.md:L170-L173
Useful? React with 👍 / 👎.
097ec83 to
774911b
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
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.
| // 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, | ||
| current_time + (overloaded ? OVERLOADED_PEER_OBJECT_DELAY : 0us)); |
There was a problem hiding this comment.
🔴 Blocking: 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.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — prefer_first is not actually selected first no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| BOOST_AUTO_TEST_CASE(orphan_vote_relayed_by_a_second_peer_adds_it_as_a_fallback) | ||
| { | ||
| auto& govman = *m_node.govman; | ||
|
|
||
| const uint256 parent_hash{MakeProposal(uint256{}).GetHash()}; | ||
| CGovernanceVote vote{MakeVote(parent_hash, VOTE_SIGNAL_FUNDING, VOTE_OUTCOME_YES)}; | ||
| SignWithVotingKey(vote, mn_voting_key); | ||
|
|
||
| CGovernanceException exception1; | ||
| uint256 hash_to_request1; | ||
| BOOST_CHECK(!govman.ProcessVote(vote, exception1, hash_to_request1)); | ||
| BOOST_CHECK_EQUAL(hash_to_request1, parent_hash); | ||
|
|
||
| // Same vote, second time. The orphan cache rejects the duplicate, but the parent hash to | ||
| // request must not be suppressed along with it. | ||
| CGovernanceException exception2; | ||
| uint256 hash_to_request2; | ||
| BOOST_CHECK(!govman.ProcessVote(vote, exception2, hash_to_request2)); | ||
| BOOST_CHECK_EQUAL(hash_to_request2, parent_hash); |
There was a problem hiding this comment.
🔴 Blocking: 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.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Keep the duplicate-relay fix and its regression test in one commit no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| void PeerManagerImpl::AskPeersForObject(const CInv& inv, NodeId prefer_first) | ||
| { | ||
| std::vector<PeerRef> peersToAsk; | ||
| peersToAsk.reserve(4); | ||
| LOCK(cs_main); | ||
| READ_LOCK(m_peer_mutex); | ||
|
|
||
| { | ||
| READ_LOCK(m_peer_mutex); | ||
| // TODO consider prioritizing MNs again, once that flag is moved into Peer | ||
| for (const auto& [_, peer] : m_peer_map) { | ||
| if (peersToAsk.size() >= 4) { | ||
| break; | ||
| } | ||
| if (IsInvInFilter(*peer, txid)) { | ||
| peersToAsk.emplace_back(peer); | ||
| const auto current_time{GetTime<std::chrono::microseconds>()}; | ||
| size_t asked_count{0}; | ||
|
|
||
| // Register a fresh, preferred 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. If a peer here already has an | ||
| // announcement, ReceivedInv is a no-op and the existing one keeps its place. | ||
| auto try_ask_peer = [&](const PeerRef& peer) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { | ||
| if (State(peer->m_id) == nullptr) return false; | ||
| // Obey the same per-peer accounting AddObjectAnnouncement applies to announcements the peer | ||
| // sent us. A synthetic announcement is still an entry the peer's behaviour can cause us to | ||
| // create -- a peer that keeps naming objects we do not have would otherwise grow its tracker | ||
| // footprint without limit. | ||
| if (m_object_request.Count(peer->m_id) >= MAX_PEER_OBJECT_ANNOUNCEMENTS) return false; | ||
| const bool overloaded = m_object_request.CountInFlight(peer->m_id) >= MAX_PEER_OBJECT_REQUEST_IN_FLIGHT; | ||
| LogPrint(BCLog::NET, "PeerManagerImpl::%s -- %s: asking peer %d\n", __func__, inv.ToString(), | ||
| peer->m_id); | ||
|
|
||
| // 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, | ||
| current_time + (overloaded ? OVERLOADED_PEER_OBJECT_DELAY : 0us)); | ||
| return true; | ||
| }; | ||
|
|
||
| // 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()) { | ||
| if (try_ask_peer(it->second)) { | ||
| ++asked_count; | ||
| } | ||
| } | ||
| } | ||
| { | ||
| 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. | ||
| 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); | ||
|
|
||
| m_object_request.ReceivedInv(peer->m_id, CInv(MSG_TX, txid), /*preferred=*/true, current_time); | ||
|
|
||
| // TODO consider prioritizing MNs again, once that flag is moved into Peer | ||
| for (const auto& [_, peer] : m_peer_map) { | ||
| if (asked_count >= MAX_PEERS_TO_ASK_FOR_OBJECT) { | ||
| break; | ||
| } | ||
| if (peer->m_id == prefer_first) { | ||
| continue; | ||
| } | ||
| if (IsInvInFilter(*peer, inv.hash)) { | ||
| if (try_ask_peer(peer)) { | ||
| ++asked_count; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: 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.
source: ['codex']
There was a problem hiding this comment.
Resolved in this update — Add focused coverage for the new object-fetch routing no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
774911b to
977efd9
Compare
There was a problem hiding this comment.
💡 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".
| // are a ten-minute recovery window that the restart has already invalidated, so drop what | ||
| // was read and reassert the bound; the field stays in the stream to keep the on-disk format | ||
| // unchanged. Clear() does not touch the capacity. | ||
| cmmapOrphanVotes.Clear(); |
There was a problem hiding this comment.
On a normal restart within the ten-minute orphan lifetime, governance.dat contains still-valid signed votes whose absolute NodeSeconds expirations remain meaningful, but this unconditional clear discards every one before its parent can trigger CheckOrphanVotes(). Since the periodic orphan-parent sweep was also removed, those votes are recovered only if another peer happens to relay or resync them; instead, retain unexpired entries and prune only expired or over-limit items before setting the new capacity.
AGENTS.md reference: AGENTS.md:L171-L171
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
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.
e1b4b73 to
380b014
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The exact-head Codex precheck found no in-scope correctness, security, consensus, or test-coverage findings. All three prior findings are fixed by the rewritten two-commit stack, including balanced test syntax at each commit, focused multi-peer request-routing coverage, and folding the final implementation into its logical source commits.
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 zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
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/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
|
This pull request has conflicts, please rebase. |
The orphan-parent fetch helper was transaction-specific only in its CInv construction. Accept a CInv so other subsystems can use the object request tracker for objects they want but were never offered. Allow callers to name an explicit peer that demonstrably holds the object without requiring an inventory announcement. Register that peer together with inventory-filter candidates, while leaving request order and fallback scheduling to the tracker. Apply normal per-peer announcement and in-flight accounting to synthetic candidates. Snapshot candidates before taking cs_main so peer-map iteration and inventory-filter locking stay outside the global critical section, then recheck node state under cs_main before registration. Extract the candidate limit into MAX_PEERS_TO_ASK_FOR_OBJECT and demote the per-peer log line to BCLog::NET because callers may invoke this on a peer-driven path.
…han cache NetGovernance::Schedule sent one MNGOVERNANCESYNC per orphan parent hash per connected peer every five minutes for as long as the orphan remained. Orphan keys are peer-controlled, so this created repeated O(orphan parents x peers) outbound work outside the request tracker's limits. Seed the object request tracker directly from each peer that relays a valid orphan vote. Return the missing parent for duplicate relays as well, allowing later relayers to become fallback candidates. The tracker owns GETDATA scheduling, in-flight limits, expiry and fallback; candidate registration remains bounded per helper call without claiming that later relays cannot add candidates. Move orphan expiry into CheckAndRemove and cap the cache at MAX_ORPHAN_VOTES. Preserve the governance.dat format by consuming but discarding legacy orphan state and its serialized capacity, then reassert runtime cache bounds even after deserialization failures. Add focused coverage for cache bounds, legacy and failed loads, duplicate relays, and end-to-end multi-peer parent-request routing.
Exercise the full network path the unit tests stop short of: a P2P peer delivers a governance vote whose parent object the node does not have, and the node must respond with a tracker-driven getdata for the parent to that same peer -- the peer never announced the object, so no other mechanism can request it. Serving the object then replays the orphan vote. The old MNGOVERNANCESYNC recovery broadcast must not be sent; against a pre-change dashd this test fails waiting for the parent getdata. The test framework previously could not speak governance messages: govobj/govobjvote/govsync were known-but-ignored message types, and CGovernanceObject.serialize() was broken (wrong attribute name, deser_* used for writing, list defaults for byte fields). Fix the serializer and register proper message classes.
380b014 to
76a63d3
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The exact-head review found no in-scope correctness, security, consensus, performance, or test-coverage defects. The request-tracker integration bounds candidate registration and preserves fallback behavior, while orphan-cache policy survives initialization, successful legacy deserialization, and failed loads; focused unit and functional tests cover the new behavior.
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 zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
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/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
…ject The collateral is validated through node1's txindex when the object arrives, and a txindex miss is a hard reject with no postpone and no re-request (the sole announcer's tracker candidacy is already consumed). Drain the validation queue after feeding node1 the blocks, mirroring the existing guard on node0, so the object cannot race the async index.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: daa0c93a13
ℹ️ 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".
| } | ||
| if (IsInvInFilter(*peer, txid)) { | ||
| peersToAsk.emplace_back(peer); | ||
| if (candidates.size() >= MAX_CANDIDATES) break; |
There was a problem hiding this comment.
When the first eight matching peers are at MAX_PEER_OBJECT_ANNOUNCEMENTS or disconnect before cs_main is acquired, this early break prevents later eligible peers from ever reaching AddObjectAnnouncement, even though asked_count remains zero. The final code now scans eight candidates instead of the previously flagged four, but the fresh admission loop still proves that every one of those eight can be skipped; with more than eight matching peers, this can strand an InstantSend transaction or orphan-vote parent that an eligible later peer advertised. Continue scanning until four announcements are actually admitted rather than imposing a fixed pre-admission candidate window.
AGENTS.md reference: AGENTS.md:L170-L173
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The orphan-cache bound and request-tracker integration address the primary resource-exhaustion paths, but candidate discovery still stops before admission, so saturated or disconnected peers can consume all snapshot slots and prevent an eligible peer from serving a wanted object. The functional-test stabilization should also be folded into the commit introducing that test so the stack remains reliable at every revision.
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: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
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)
🔴 1 blocking | 🟡 1 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:2407-2430: Scan candidates until four actually pass admission
Candidate collection stops after eight matching peers, before `AddObjectAnnouncement()` checks whether those peers are still connected and below `MAX_PEER_OBJECT_ANNOUNCEMENTS`. If all eight fail admission, eligible matching peers later in `m_peer_map` are never considered and `asked_count` remains zero. This can leave the InstantSend caller, which has no explicit peer, without any candidate for a missing locked transaction even though another connected peer advertised it; the governance fallback set can fail similarly. Snapshot every matching peer while holding `m_peer_mutex`, then retain the existing `asked_count` limit so processing stops after four announcements are actually admitted.
In `<commit:b9038db>`:
- [SUGGESTION] <commit:b9038db>:1: Fold the validation-queue test repair into the test commit
Commit `b9038dbaf70` adds only the validation-interface queue drain needed to prevent the functional test introduced by `76a63d3f3f5` from racing node1's asynchronous txindex. Without those lines, the governance object can be rejected on a transient collateral lookup failure with no retry, leaving the introducing revision flaky. Squash this repair into `76a63d3f3f5` so the test is reliable from the commit that adds it.
| // Collect a few more candidates than we intend to ask, as spares for candidates that fail the | ||
| // per-peer accounting below; anything beyond that could never be used. | ||
| constexpr size_t MAX_CANDIDATES{2 * MAX_PEERS_TO_ASK_FOR_OBJECT}; | ||
| std::vector<PeerRef> candidates; | ||
| { | ||
| READ_LOCK(m_peer_mutex); | ||
| candidates.reserve(MAX_CANDIDATES); | ||
|
|
||
| // 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 explicitly. Candidate selection order remains | ||
| // the request tracker's responsibility. | ||
| if (explicit_peer != -1) { | ||
| if (auto it = m_peer_map.find(explicit_peer); it != m_peer_map.end()) { | ||
| candidates.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) { | ||
| break; | ||
| } | ||
| if (IsInvInFilter(*peer, txid)) { | ||
| peersToAsk.emplace_back(peer); | ||
| if (candidates.size() >= MAX_CANDIDATES) break; | ||
| if (peer->m_id != explicit_peer && IsInvInFilter(*peer, inv.hash)) { | ||
| candidates.emplace_back(peer); | ||
| } | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Scan candidates until four actually pass admission
Candidate collection stops after eight matching peers, before AddObjectAnnouncement() checks whether those peers are still connected and below MAX_PEER_OBJECT_ANNOUNCEMENTS. If all eight fail admission, eligible matching peers later in m_peer_map are never considered and asked_count remains zero. This can leave the InstantSend caller, which has no explicit peer, without any candidate for a missing locked transaction even though another connected peer advertised it; the governance fallback set can fail similarly. Snapshot every matching peer while holding m_peer_mutex, then retain the existing asked_count limit so processing stops after four announcements are actually admitted.
| // Collect a few more candidates than we intend to ask, as spares for candidates that fail the | |
| // per-peer accounting below; anything beyond that could never be used. | |
| constexpr size_t MAX_CANDIDATES{2 * MAX_PEERS_TO_ASK_FOR_OBJECT}; | |
| std::vector<PeerRef> candidates; | |
| { | |
| READ_LOCK(m_peer_mutex); | |
| candidates.reserve(MAX_CANDIDATES); | |
| // 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 explicitly. Candidate selection order remains | |
| // the request tracker's responsibility. | |
| if (explicit_peer != -1) { | |
| if (auto it = m_peer_map.find(explicit_peer); it != m_peer_map.end()) { | |
| candidates.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) { | |
| break; | |
| } | |
| if (IsInvInFilter(*peer, txid)) { | |
| peersToAsk.emplace_back(peer); | |
| if (candidates.size() >= MAX_CANDIDATES) break; | |
| if (peer->m_id != explicit_peer && IsInvInFilter(*peer, inv.hash)) { | |
| candidates.emplace_back(peer); | |
| } | |
| } | |
| std::vector<PeerRef> candidates; | |
| { | |
| READ_LOCK(m_peer_mutex); | |
| candidates.reserve(m_peer_map.size()); | |
| // 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 explicitly. Candidate selection order remains | |
| // the request tracker's responsibility. | |
| if (explicit_peer != -1) { | |
| if (auto it = m_peer_map.find(explicit_peer); it != m_peer_map.end()) { | |
| candidates.emplace_back(it->second); | |
| } | |
| } | |
| // TODO consider prioritizing MNs again, once that flag is moved into Peer | |
| for (const auto& [_, peer] : m_peer_map) { | |
| if (peer->m_id != explicit_peer && IsInvInFilter(*peer, inv.hash)) { | |
| candidates.emplace_back(peer); | |
| } | |
| } | |
| } |
source: ['codex']
There was a problem hiding this comment.
Valid — fixed in 92ddb9b (fixup into the commit that introduced it). The candidate cap was a micro-optimization from an earlier review pass ("reserve the cap instead of m_peer_map.size()"), and you are right that it traded a real behavioural narrowing for microseconds: since admission needs cs_main and cannot run under m_peer_mutex, a capped snapshot cannot know which candidates will be rejected. Reverted to snapshotting every matching peer, keeping the asked_count limit in the ask loop so we still stop after four announcements are actually admitted.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved in this update — Scan candidates until four actually pass admission no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8bcc860dd7
ℹ️ 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".
| if (const auto it{m_counts.find(outpoint)}; it != m_counts.end() && it->second >= m_max_per_mn) { | ||
| return InsertResult::MN_LIMIT; |
There was a problem hiding this comment.
When a masternode has exactly 500 cached orphans, relaying any one of those votes from a second peer returns MN_LIMIT before checking whether the entry is a duplicate. ProcessVote() consequently exits without setting hashToRequest, so the second relayer is not registered as a fallback even though the duplicate consumes no additional cache capacity; if the first peer fails and no periodic sweep remains, that parent can be stranded. Fresh evidence in the final code is this limit check's position ahead of m_cache.Insert, which contradicts the newly added duplicate-relay recovery path; detect duplicates first and add a boundary test with a full per-masternode share.
AGENTS.md reference: AGENTS.md:L171-L171
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 082f399. OrphanVoteCache::Insert() now checks for an existing (parent, vote) pair before applying the per-masternode share, via a new CacheMultiMap::HasEntry(). Covered by duplicates_are_recognized_at_the_per_masternode_share in the new src/test/orphan_vote_cache_tests.cpp.
🤖 Posted autonomously by Claude on behalf of pasta.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The PR addresses the primary orphan-vote resource-exhaustion paths, but two request-fallback defects remain: candidate discovery can stop before finding four admissible peers, and duplicate orphan relays stop registering fallback sources once their masternode reaches its cache share. The functional-test queue-drain repair also remains separated from the commit introducing the test, leaving that earlier revision susceptible to the txindex race.
Source: reviewer backend gpt-5.6-sol (Codex general); final verifier backend gpt-5.6-sol. Orchestration-only: openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
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) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking | 🟡 1 suggestion(s)
1 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 `<commit:b9038db>`:
- [SUGGESTION] <commit:b9038db>:1: Fold the validation-queue test repair into the test commit
Commit `b9038dbaf70` contains only the `syncwithvalidationinterfacequeue()` call and explanation needed to prevent the functional test introduced by `76a63d3f3f5` from racing node1's asynchronous txindex. At the introducing revision, the governance object can be rejected after a transient collateral lookup failure, with no retry because the sole peer's request candidacy has already been consumed. Fold this repair into `76a63d3f3f5` so the test is reliable from the revision that introduces it.
In `src/governance/governance.h`:
- [BLOCKING] src/governance/governance.h:85-93: Recognize duplicate orphans before enforcing the masternode limit
When a masternode has exactly `MAX_ORPHAN_VOTES_PER_MN` cached entries, `OrphanVoteCache::Insert()` returns `MN_LIMIT` before checking whether the incoming `(parent, vote)` pair is already cached. A second peer relaying one of those existing votes therefore causes `CGovernanceManager::ProcessVote()` to return before assigning `hashToRequest`, so `NetGovernance` never registers that peer as a fallback source for the missing parent. The duplicate consumes no additional cache capacity, and suppressing it violates the PR's stated invariant that every relay of a cached orphan can add its sender as a candidate now that the periodic sweep is gone. Detect an existing pair before applying the per-masternode capacity check, and add a boundary test that repeats a cached vote after filling that masternode's share.
In `src/net_processing.cpp`:
- [BLOCKING] src/net_processing.cpp:2407-2430: Scan candidates until four actually pass admission
(existing thread: https://github.com/dashpay/dash/pull/7526#discussion_r3763673689)
Candidate collection stops after eight matching peers, before `AddObjectAnnouncement()` verifies that each peer is still connected and below `MAX_PEER_OBJECT_ANNOUNCEMENTS`. If all eight fail admission, later eligible peers in `m_peer_map` are never considered and `asked_count` remains zero. This can leave the InstantSend caller, which has no explicit peer, unable to fetch a missing locked transaction even though another connected peer advertised it; governance fallback discovery can fail the same way. Snapshot all matching `PeerRef`s under `m_peer_mutex`, then keep the existing `asked_count` limit so processing stops after four announcements actually pass admission.
| InsertResult Insert(const uint256& parent_hash, const OrphanVote& orphan_vote) | ||
| { | ||
| const COutPoint outpoint{orphan_vote.vote.GetMasternodeOutpoint()}; | ||
| if (const auto it{m_counts.find(outpoint)}; it != m_counts.end() && it->second >= m_max_per_mn) { | ||
| return InsertResult::MN_LIMIT; | ||
| } | ||
| if (!m_cache.Insert(parent_hash, orphan_vote)) { | ||
| return InsertResult::DUPLICATE; | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Recognize duplicate orphans before enforcing the masternode limit
When a masternode has exactly MAX_ORPHAN_VOTES_PER_MN cached entries, OrphanVoteCache::Insert() returns MN_LIMIT before checking whether the incoming (parent, vote) pair is already cached. A second peer relaying one of those existing votes therefore causes CGovernanceManager::ProcessVote() to return before assigning hashToRequest, so NetGovernance never registers that peer as a fallback source for the missing parent. The duplicate consumes no additional cache capacity, and suppressing it violates the PR's stated invariant that every relay of a cached orphan can add its sender as a candidate now that the periodic sweep is gone. Detect an existing pair before applying the per-masternode capacity check, and add a boundary test that repeats a cached vote after filling that masternode's share.
source: ['codex']
There was a problem hiding this comment.
Valid — real bug, fixed in 082f399 (fixup into the commit that introduced it). Confirmed the consequence you describe: MN_LIMIT returned before the duplicate check meant ProcessVote() returned without setting hashToRequest, so a second peer relaying an already-cached vote from an at-share masternode was never registered as a fallback source — exactly the invariant the PR relies on now that the sweep is gone.
OrphanVoteCache::Insert() now tests for an existing (parent, vote) pair first (new CacheMultiMap::HasEntry(), mirroring the dedupe check Insert() already does internally) and only then applies the per-masternode share. Added duplicates_are_recognized_at_the_per_masternode_share, which fills a masternode share, asserts a new parent is refused with MN_LIMIT, and asserts both cached votes still repeat as DUPLICATE without changing cache size.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Resolved in this update — Recognize duplicate orphans before enforcing the masternode limit no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
…ouncement try_ask_peer re-implemented AddObjectAnnouncement's per-peer accounting line for line, so a future change to the caps or backoff would have to be made in two places with no compile error when one is missed. Retype AddObjectAnnouncement to NodeId with a bool result and a force_preferred flag and call it from AskPeersForObject. Also stop collecting a PeerRef for every matching peer: at most MAX_PEERS_TO_ASK_FOR_OBJECT are ever asked, so cap the candidate scan at twice that as spares for candidates that fail admission.
…acy-store tests The live orphan cache is never streamed from disk (Unserialize discards the legacy field into a local), so its capacity is fixed at construction and the SetMaxSize in Clear() could never matter; remove it and correct the comment that attributed the bound to Clear(). The two bound-survives-load tests existed to guard that dead line and could not fail; replace them with one cheap test pinning what actually matters -- a legacy file's orphan entries and capacity are dropped whether the load completes or throws -- and share one MakeLegacyStore helper with the invalid-vote-cache test instead of hand-transcribing the disk layout with empty stand-in maps. Removes ~4000 ECDSA operations from the suite.
The test kept its own byte-identical send-only copy of the wire message from before the framework class existed, leaving two definitions of MNGOVERNANCESYNC to drift apart.
The orphan cache was one global 1000-entry pool with oldest-first eviction and no per-source accounting, so a single registered voting key could mint valid votes naming invented parents and flush every honest orphan on demand -- and deliberately without penalty, since valid-MN-signed votes for unknown parents are routine during sync. Wrap the cache in governance::OrphanVoteCache, which enforces two bounds together: a per-masternode share (500) so one key can no longer displace everyone else, and a global cap raised to 100'000 (~40 MB worst case) now that filling it takes 200 distinct masternode keys. A vote over its key's share is dropped without seeding a parent request; duplicates keep seeding them, as before. The wrapper owns the eviction accounting: the inner map's capacity sits one above the global cap so its own pruning can never bypass the per-masternode counts. Also replace the mis-keyed cmmapOrphanVotes.Erase(voteHash) in RemoveInvalidVotes -- the map is keyed by parent hash, so that erase never matched anything -- with dropping the changed-key masternode's cached orphans, which can no longer validate on replay.
8bcc860 to
082f399
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 082f399458
ℹ️ 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(nodeid, inv, preferred, current_time + delay); | ||
| return true; |
There was a problem hiding this comment.
When this peer's parent GETDATA has timed out while another candidate keeps the hash alive, its tracker entry remains COMPLETED; if the same peer later relays a different orphan vote for that parent, ReceivedInv is documented to ignore the existing (peer, inv) entry, yet this helper returns success. Consequently the fresh evidence neither revives the peer nor reaches a later candidate once it consumes the four-peer budget, and after the remaining candidates fail the parent can be stranded now that the periodic sweep is gone. Fresh evidence in the final code is this unconditional success after the no-op-capable ReceivedInv; revive completed entries or report whether a live candidate was actually registered.
AGENTS.md reference: AGENTS.md:L170-L173
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The current head fixes both prior blocking findings: candidate discovery now snapshots every matching peer before applying the four-admission limit, and duplicate orphan votes are recognized before the per-masternode cap. The only remaining in-scope issue is commit-stack hygiene: the validation-queue synchronization required by the new functional test remains in a separate follow-up commit; the proposed completed-candidate blocker is refuted because the tracker intentionally retains COMPLETED entries to prevent retrying a peer that already failed. Source: reviewer backend gpt-5.6-sol (Codex general); final verifier backend gpt-5.6-sol; orchestration-only and not reviewer evidence: openclaw-agent/cliproxy/gpt-5.6-sol.
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
🟡 1 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 `<commit:b9038db>`:
- [SUGGESTION] <commit:b9038db>:1: Fold the validation-queue test repair into the test commit
Commit `b9038dbaf70` contains only the `syncwithvalidationinterfacequeue()` call required to stabilize the functional test introduced by `76a63d3f3f5`. At the introducing revision, node1 can serve the governance object before its asynchronous txindex has processed the submitted collateral blocks, causing a hard collateral lookup rejection with no retry after the sole request candidacy has been consumed. Fold this repair into `76a63d3f3f5` so the test is reliable at the revision where it is introduced.
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 explicit_peer)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. The synthetic announcements go through the same per-peer admission accounting (AddObjectAnnouncement) as peer-sent ones.explicit_peeris 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::ProcessMessagenow calls this instead of pushingMNGOVERNANCESYNC, and the 5-minute sweep plusGetOrphanVoteObjectHashes()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 toExpireOrphanVotes(), called fromCheckAndRemove()— the same 5-minute tick, one gate looser (IsBlockchainSyncedrather thanIsSynced).Bound the cache, globally and per masternode.
cmmapOrphanVotesbecomesgovernance::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, ~40 MB worst case, replacing the ~750 MBMAX_CACHE_SIZEceiling). 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.RemoveInvalidVotesnow also drops a changed-key or removed masternode's cached orphans, which could no longer validate on replay — replacing an erase that was keyed by vote hash into a parent-hash-keyed map and so never matched anything.We still serve
object_fetchrequests 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:nParentHashis 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?
Built and tested locally on aarch64-apple-darwin (
--enable-debug), fullmakeclean.New unit tests in
src/test/governance_vote_processing_tests.cpp:orphan_votes_per_masternode_are_boundedverifies one voting key's orphans are capped atMAX_ORPHAN_VOTES_PER_MNand that votes past the share stop seeding parent requests.orphan_vote_relayed_by_a_second_peer_adds_it_as_a_fallbackverifies duplicate relays still expose the missing parent for tracker registration.orphan_vote_relayers_seed_parent_request_candidatesverifies the full network path registers both relayers, but not an unrelated peer, as parent-fetch candidates.legacy_orphan_votes_are_discarded_on_loadverifies a legacy cache file's orphan entries and serialized capacity are dropped, both on a clean load and when the load fails mid-stream.orphan_vote_cache_testssuite exercisesOrphanVoteCacheaccounting directly with small bounds: per-key share enforcement and release on erase, global eviction releasing the evicted key's slot,EraseAllForMasternode, andClear.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.New functional test
test/functional/p2p_governance_orphan_vote.pycovers the path end to end at the wire level, which the unit tests stop short of: a P2P peer delivers a vote whose parent object the node lacks, the node responds with a tracker-drivengetdatafor the parent to that same peer (which never announced the object, so nothing else could request it), noMNGOVERNANCESYNCis sent, and serving the object replays the orphan vote. Against a pre-changedashdthe test fails waiting for the parentgetdata, confirming it pins the new behavior. Supporting framework work:govobj/govobjvote/govsyncmessage classes (previously known-but-ignored types), and a fix forCGovernanceObject.serialize()inmessages.py, which was broken (wrong attribute name,deser_*used for writing) and had never been usable. Passes 14 consecutive parallel-load runs locally.Functional:
feature_governance.py,feature_governance_cl.py,feature_governance_objects.py,p2p_govsync_bloom.pypass with the framework changes;p2p_instantsend.pyandrpc_verifyislock.pypass for the InstantSend caller that was updated.Rebased on develop after #7569 (invalid-vote cache removal) landed;
Unserializenow discards both legacy fields while preserving the on-disk field order.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. 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: