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 2 commits into
Conversation
|
✅ Final review complete — no blockers (commit 380b014) |
![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 👍 / 👎.
|
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:
If these PRs merge firstThis PR 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.
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.
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)
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.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.
cmmapOrphanVotesis constructed withMAX_ORPHAN_VOTES = 1000instead ofMAX_CACHE_SIZE. Orphans are short-lived recovery state for votes that outran their object in relay, so the bound only has to cover objects genuinely in flight.We still serve
object_fetchrequests from older peers; only the sending side changes.Deliberately not done
No masternode/signature validation was added before orphan insertion. A valid MN signature is not a scarce resource —
nParentHashis covered by the signature, but nothing ties it to an object that exists, so any one of the masternode keys can sign unlimited votes naming invented parents. The orphan branch also has to stay at penalty 0, because reaching it is a routine relay race for honest peers, and misbehavior scoring is suppressed while!IsSynced()— precisely when orphans are common. Validation would add ECDSA and BLS verification undercs_storeon a path a peer can drive. The bound and the tracker are what actually close this; validation would be costly hardening on top, and is better considered separately.How Has This Been Tested?
Built and tested locally on aarch64-apple-darwin (
--enable-debug), fullmakeclean.New unit tests in
src/test/governance_vote_processing_tests.cpp:orphan_vote_cache_is_boundedverifies the live cache never exceedsMAX_ORPHAN_VOTES.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.orphan_vote_bound_survives_loading_an_old_cache_fileverifies legacy serialized capacity cannot override node policy.orphan_vote_bound_survives_a_failed_old_cache_loadverifies the bound also survives a later deserialization failure.Unit:
governance_inv_tests,governance_superblock_tests,governance_validators_tests,governance_vote_wire_tests,denialofservice_tests,net_tests,net_peer_eviction_tests,peerman_tests— all pass.Functional:
feature_governance.py,feature_governance_cl.pypass;p2p_instantsend.pyandrpc_verifyislock.pypass for the InstantSend caller that was updated.Lint:
lint-whitespace.py,lint-circular-dependencies.pyclean.Breaking Changes
None. No message format changes, no
governance.datformat change, no consensus or P2P protocol change. Purely a change in what this node sends.One behavioral note for reviewers, called out explicitly because it is a deliberate narrowing rather than a strict improvement. The old sweep re-asked every connected peer every 5 minutes for an orphan's full 10-minute life. The new path registers only peers that give us evidence they have the parent: the peer that relayed the vote, plus any that already announced that specific object hash. So the set of peers asked is driven by who actually relays to us rather than by who happens to be connected.
Every relay of a vote for a still-missing parent adds its sender as a candidate, including a relay of a vote we already hold, so fallbacks accumulate as the vote propagates rather than being fixed at the first sender. The tracker retries and moves to the next candidate on expiry, and the object also arrives through ordinary governance sync. The accepted trade is that the old persistence was the amplification: it cannot be kept without keeping the
O(orphans x peers)term.Checklist: