fix: stop unvalidated governance orphan-vote amplification - #7517
fix: stop unvalidated governance orphan-vote amplification#7517PastaPastaPasta wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 34 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
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 |
|
⛔ Blockers found — Sonnet deferred (commit 1f9db83) |
![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: 3dfcfc8403
ℹ️ 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".
| Shuffle(vecHashesFiltered.begin(), vecHashesFiltered.end(), FastRandomContext()); | ||
| vecHashesFiltered.resize(MAX_ORPHAN_OBJECT_REQUESTS_PER_TICK); |
There was a problem hiding this comment.
When more than 100 signed orphan parents are cached and the immediate request to the announcing peer fails, random sampling does not prevent starvation: Schedule() runs every 5 minutes while each orphan expires after 10 minutes, so a cache of 1,000 gives each parent only one or two 10% chances of being retried through other peers before removal. Thus roughly 81–90% of those parents may never be requested again, whereas the previous scheduler retried every retained orphan; use rotating batches or otherwise ensure coverage within the expiration window.
AGENTS.md reference: AGENTS.md:L157-L175
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The orphan-cache bounds and pre-insertion validation address the original unauthenticated cache-filling vector, but cached orphan votes are not recognized before the newly added cryptographic checks. A peer can therefore replay a valid orphan vote and repeatedly force ECDSA/BLS verification while holding the governance-store lock; the corrective follow-up commit should also be folded into the implementation commit for a hygienic history.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s)
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/governance/governance.cpp`:
- [BLOCKING] src/governance/governance.cpp:860-861: Cached orphan replays repeatedly perform signature verification
The known-vote checks at the start of `ProcessVote()` cover only `cmapVoteToObject` and `cmapInvalidVotes`; duplicate orphan detection does not occur until `cmmapOrphanVotes.Insert()` at line 882, after the ECDSA/BLS checks shown here. The inventory path has the same gap because `ConfirmInventoryRequest()` and `HaveVoteForHash()` do not consult cached orphan votes. After each payload, `PeerConsumeObjectRequest()` consumes the request-tracker entry, so the same peer can announce the hash again, receive another GETDATA, and resend the vote. Each replay then performs cryptographic verification while `cs_store` is held, receives no penalty, and is rejected only by the late duplicate insertion. The 1,000-entry cache bound does not limit this revalidation rate. Maintain a hash-index of cached orphan votes and treat those hashes as known before signature verification and inventory retrieval; add a regression test that replays an already cached valid orphan and confirms the signature-validation path is not entered again.
In `<commit:3dfcfc84037>`:
- [SUGGESTION] <commit:3dfcfc84037>:1: Squash the corrective review follow-up into the main fix
Commit `3dfcfc84037` changes the same implementation introduced by `be6a351ed0e`: it removes the transient penalty for valid orphan relays, replaces biased fixed-prefix truncation with random sampling, and repairs the accompanying tests. `CONTRIBUTING.md` identifies commits that repeatedly change the same lines as fixup commits that may need squashing. Fold this follow-up into `be6a351ed0e` so the permanent implementation commit does not temporarily penalize honest relays or starve higher-sorting orphan parents.
| const bool sig_ok = vote.CheckSignature(dmn->pdmnState->keyIDVoting) || | ||
| vote.CheckSignature(dmn->pdmnState->pubKeyOperator.Get()); |
There was a problem hiding this comment.
🔴 Blocking: Cached orphan replays repeatedly perform signature verification
The known-vote checks at the start of ProcessVote() cover only cmapVoteToObject and cmapInvalidVotes; duplicate orphan detection does not occur until cmmapOrphanVotes.Insert() at line 882, after the ECDSA/BLS checks shown here. The inventory path has the same gap because ConfirmInventoryRequest() and HaveVoteForHash() do not consult cached orphan votes. After each payload, PeerConsumeObjectRequest() consumes the request-tracker entry, so the same peer can announce the hash again, receive another GETDATA, and resend the vote. Each replay then performs cryptographic verification while cs_store is held, receives no penalty, and is rejected only by the late duplicate insertion. The 1,000-entry cache bound does not limit this revalidation rate. Maintain a hash-index of cached orphan votes and treat those hashes as known before signature verification and inventory retrieval; add a regression test that replays an already cached valid orphan and confirms the signature-validation path is not entered again.
source: ['codex']
3dfcfc8 to
c31d324
Compare
ProcessVote inserted a vote into cmmapOrphanVotes as soon as the parent object lookup missed, before any masternode-membership or signature check and with a zero misbehaviour penalty. The cache holds a million entries keyed by an attacker-chosen parent hash, so a peer could park hundreds of megabytes of unvalidated data. Worse, every orphan key produced one MNGOVERNANCESYNC per connected peer every 5 minutes, ignoring fPauseSend, and the map was persisted to governance.dat so a flood survived restart and re-drove the fan-out on boot. Any unauthenticated peer can do this. Require a tip-list masternode and a valid voting-or-operator signature before a vote may enter the orphan cache; UpdateHash()/GetSignatureHash() cover nParentHash, so votes cannot be repointed at fresh parent hashes without masternode keys. Bound the cache, sample orphan-object requests instead of requesting every key, skip peers with a paused send buffer, and stop persisting orphan votes.
c31d324 to
1f9db83
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Carried-forward prior findings: the cached-orphan replay blocker remains valid at the current head, while the commit-history squash suggestion is fixed by the rewritten single-commit stack. The existing retry-coverage suggestion also remains valid; the latest delta introduces no genuinely new findings.
Source: Codex general reviewer gpt-5.6-sol; Codex dash-core-commit-history reviewer gpt-5.6-sol; Codex verifier gpt-5.6-sol. The openclaw-agent coordinator is orchestration-only.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 1 suggestion(s)
1 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/governance/governance.cpp`:
- [SUGGESTION] src/governance/governance.cpp:1178-1180: Retry retained orphans before they expire
`NetGovernance::Schedule()` first runs after five minutes and repeats every five minutes, while orphan votes expire after ten minutes. With 1,000 retained parent hashes and a random sample of 100, each parent typically gets only one or two 10% chances to enter the scheduled batch, leaving approximately 81–90% without any scheduled retry before expiration. The immediate request targets only the announcing peer, so if that peer does not provide the parent, most retained orphans can expire without querying another peer. Use coverage-tracked rotating batches, a shorter retry interval, a longer expiration period, or another bounded mechanism that covers retained parents before expiration.
| if (vecHashesFiltered.size() > MAX_ORPHAN_OBJECT_REQUESTS_PER_TICK) { | ||
| Shuffle(vecHashesFiltered.begin(), vecHashesFiltered.end(), FastRandomContext()); | ||
| vecHashesFiltered.resize(MAX_ORPHAN_OBJECT_REQUESTS_PER_TICK); |
There was a problem hiding this comment.
🟡 Suggestion: Retry retained orphans before they expire
NetGovernance::Schedule() first runs after five minutes and repeats every five minutes, while orphan votes expire after ten minutes. With 1,000 retained parent hashes and a random sample of 100, each parent typically gets only one or two 10% chances to enter the scheduled batch, leaving approximately 81–90% without any scheduled retry before expiration. The immediate request targets only the announcing peer, so if that peer does not provide the parent, most retained orphans can expire without querying another peer. Use coverage-tracked rotating batches, a shorter retry interval, a longer expiration period, or another bounded mechanism that covers retained parents before expiration.
source: ['codex']
… 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
2 participants
Issue being fixed or feature implemented
CGovernanceManager::ProcessVote()inserted a vote intocmmapOrphanVotesas soon asmapObjects.find(vote.GetParentHash())missed - before any masternode-membership or signature check, and with a zero misbehaviour penalty. The cache is sizedMAX_CACHE_SIZE = 1'000'000and keyed by the attacker-chosen parent hash, andCacheMultiMapstores each vote twice, so a peer can park roughly 600 MB of unvalidated data there.The larger problem is fan-out.
GetOrphanVoteObjectHashes()returned every orphan key uncapped, andNetGovernance::Schedule()sends oneMNGOVERNANCESYNCper key per connected peer every 5 minutes, ignoringfPauseSend. The orphan map was also serialised intogovernance.dat, so a flood survived restart and re-drove the fan-out on boot.This is reachable by any unauthenticated P2P peer; the only gate is the standard announce-then-request tracker. No masternode, quorum membership or RPC access is required.
What was done?
UpdateHash()/GetSignatureHash()covernParentHash, so votes cannot be repointed at fresh parent hashes without masternode keys.MAX_ORPHAN_VOTES = 1000.governance.dat.Note that the added signature verification now runs under
cs_store; this is bounded by the new discouragement score on invalid votes.How Has This Been Tested?
The first commit adds a regression test demonstrating that unvalidated votes fill the orphan cache, ordered before the fix.
Full build and test validation is delegated to CI on this PR; the changes were not built locally.
Breaking Changes
governance.datis bumped to v17. Upgrading nodes will discard their existing governance cache and re-sync governance data on first start. No consensus or P2P protocol change.Checklist: