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