fix: require a valid masternode signature before caching an orphan governance vote

[PastaPastaPasta]

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:

if (!vote.IsValid(tip_mn_list, /*useVotingKey=*/true) && !vote.IsValid(tip_mn_list, /*useVotingKey=*/false)) {
    // 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.
• Both key variants are tried. Which key is correct depends on the parent object's type and
  the vote signal (onlyVotingKeyAllowed in CGovernanceObject::ProcessVote: PROPOSAL +
  VOTE_SIGNAL_FUNDING uses the voting key, everything else the operator BLS key). On the
  orphan path the parent type is by definition unknown, so either key must be accepted. A vote
  that is invalid under both is invalid under whichever one turns out to apply.
• 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.

The second commit fixes a logging wart that this change would otherwise turn into a spam vector.
CGovernanceVote::CheckSignature(const CBLSPublicKey&) logged 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 -- and since
the gate tries the voting key first and falls through, every rejected vote lands on that branch.
A peer needs only a real masternode outpoint, which is public, plus a garbage signature, to write a
line to debug.log per message. It is one word, and it is caused by this change, so it rides along
rather than being deferred.

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:

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

[coderabbitai]

Warning

Review limit reached

@PastaPastaPasta, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 26 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 923fbed6-7337-4c92-aa55-9ad22722fdc3

📥 Commits

Reviewing files that changed from the base of the PR and between c751ae4 and 09c5c66.

📒 Files selected for processing (3)

• src/governance/governance.cpp
• src/governance/vote.cpp
• src/test/governance_inv_tests.cpp

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[thepastaclaw]

✅ Review complete (commit 09c5c66)
Last checked: 2026-08-03 01:20 UTC

[github-actions]

Potential PR merge conflicts

This 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 first

These open PRs will likely need a rebase:

• #7517: fix: stop unvalidated governance orphan-vote amplification Changed files: src/governance/governance.cpp, src/test/governance_inv_tests.cpp.
• #7518: fix: memoize governance vote signature checks to bound govsync cost Changed files: src/governance/governance.cpp, src/governance/vote.cpp.
• #7526: fix: fetch orphan-vote parents via the request tracker instead of broadcasting Changed files: src/governance/governance.cpp, src/test/governance_inv_tests.cpp.

If these PRs merge first

This PR will likely need a rebase:

• #7526: fix: fetch orphan-vote parents via the request tracker instead of broadcasting Changed files: src/governance/governance.cpp, src/test/governance_inv_tests.cpp.

[thepastaclaw]

Final validation — Codex + Sonnet

This is a solid, well-tested fix that closes a real gap allowing unauthenticated orphan-vote cache pollution and gratuitous MNGOVERNANCESYNC requests, and the immediate follow-up correctly gates the newly-reachable BLS-failure log behind the gobject category. Both agents' correctness/security reviews came back clean; Codex additionally found two legitimate, narrowly-scoped refinements to the new orphan-gate key-selection logic and its test coverage that are worth addressing but don't block the fix. Commit hygiene is good overall, with one minor stylistic observation about the two-commit split.

Review provenance

• Codex reviewers: gpt-5.6-sol — general (completed), gpt-5.6-sol — dash-core-commit-history (completed)
• Verifier: claude-sonnet-5 — final-verifier
• Sonnet reviewers: claude-sonnet-5 — general (failed), claude-sonnet-5 — dash-core-commit-history (completed), claude-sonnet-5 — general (completed)

🟡 2 suggestion(s) | 💬 1 nitpick(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/governance/governance.cpp`:
- [SUGGESTION] src/governance/governance.cpp:826-828: Orphan-vote gate accepts the voting key for signals that always require the operator key
  CGovernanceObject::ProcessVote (object.cpp:455) only allows the voting-key signature when `type == PROPOSAL && signal == VOTE_SIGNAL_FUNDING`; for VALID, DELETE, and ENDORSED it requires the operator key regardless of the object's type. The new orphan gate here tries both keys unconditionally for every signal, so a vote signed only with the voting key (a credential that's meant to carry much lower trust — it's routinely delegated to third parties) can pass the gate, get cached, and trigger a real MNGOVERNANCESYNC request for a non-funding signal, even though that exact vote is guaranteed to fail once the parent object actually arrives and CGovernanceObject::ProcessVote runs the correct key check. The commit message frames this as unavoidable because "which [key] applies depends on the parent object's type and the vote signal," but that's only true for FUNDING — for every other signal the key is always the operator key irrespective of type, so the ambiguity (and the two-key fallback) should be restricted to the FUNDING signal.

In `src/test/governance_inv_tests.cpp`:
- [SUGGESTION] src/test/governance_inv_tests.cpp:496-506: New orphan-gate signature check has no test exercising an actual signature
  `orphan_votes_require_a_valid_masternode_signature` runs against an empty deterministic masternode list, so `CGovernanceVote::IsValid` returns false at the `GetMNByCollateral` lookup (vote.cpp:180) and never reaches `CheckSignature` for either key. This is the only new test added for the security-relevant change in this PR (gating orphan-vote caching on a real signature), and as written it can't distinguish 'rejected because unknown masternode' from 'rejected because bad signature' — nor would it catch a correctly-signed early vote being wrongly rejected, or a forged signature for a known masternode being wrongly accepted. Given this is consensus-adjacent governance code, a test that registers a real masternode and checks both a forged-signature rejection and a correctly-signed acceptance (ideally for both a funding and non-funding signal) would meaningfully strengthen coverage of the new logic.

[thepastaclaw]

🟡 Suggestion: Orphan-vote gate accepts the voting key for signals that always require the operator key

CGovernanceObject::ProcessVote (object.cpp:455) only allows the voting-key signature when type == PROPOSAL && signal == VOTE_SIGNAL_FUNDING; for VALID, DELETE, and ENDORSED it requires the operator key regardless of the object's type. The new orphan gate here tries both keys unconditionally for every signal, so a vote signed only with the voting key (a credential that's meant to carry much lower trust — it's routinely delegated to third parties) can pass the gate, get cached, and trigger a real MNGOVERNANCESYNC request for a non-funding signal, even though that exact vote is guaranteed to fail once the parent object actually arrives and CGovernanceObject::ProcessVote runs the correct key check. The commit message frames this as unavoidable because "which [key] applies depends on the parent object's type and the vote signal," but that's only true for FUNDING — for every other signal the key is always the operator key irrespective of type, so the ambiguity (and the two-key fallback) should be restricted to the FUNDING signal.

Suggested change

	// The parent object is unknown, so the vote signal cannot be mapped to a key type the way
	// CGovernanceObject::ProcessVote does it (see onlyVotingKeyAllowed there). Accept either key.
	if (!vote.IsValid(tip_mn_list, /*useVotingKey=*/true) && !vote.IsValid(tip_mn_list, /*useVotingKey=*/false)) {
	// Only a FUNDING signal is ambiguous while the parent type is unknown (a proposal uses
	// the voting key for FUNDING, everything else uses the operator key). VOTE_SIGNAL_NONE is
	// never processable.
	const bool valid_operator_signature{
	vote.GetSignal() != VOTE_SIGNAL_NONE && vote.IsValid(tip_mn_list, /*useVotingKey=*/false)};
	const bool valid_voting_signature{
	vote.GetSignal() == VOTE_SIGNAL_FUNDING && vote.IsValid(tip_mn_list, /*useVotingKey=*/true)};
	if (!valid_operator_signature && !valid_voting_signature) {

_{source: ['codex']}

[thepastaclaw]

🟡 Suggestion: New orphan-gate signature check has no test exercising an actual signature

orphan_votes_require_a_valid_masternode_signature runs against an empty deterministic masternode list, so CGovernanceVote::IsValid returns false at the GetMNByCollateral lookup (vote.cpp:180) and never reaches CheckSignature for either key. This is the only new test added for the security-relevant change in this PR (gating orphan-vote caching on a real signature), and as written it can't distinguish 'rejected because unknown masternode' from 'rejected because bad signature' — nor would it catch a correctly-signed early vote being wrongly rejected, or a forged signature for a known masternode being wrongly accepted. Given this is consensus-adjacent governance code, a test that registers a real masternode and checks both a forged-signature rejection and a correctly-signed acceptance (ideally for both a funding and non-funding signal) would meaningfully strengthen coverage of the new logic.

_{source: ['codex']}