fix: update masternode rate limit on failed governance trigger path

[PastaPastaPasta]

Issue being fixed or feature implemented

In AddGovernanceObjectInternal(), a trigger object is emplaced into mapObjects, then AddTrigger() is attempted; on failure the function calls PrepareDeletion() and returns early - before MasternodeRateUpdate().

MasternodeRateCheck() short-circuits with "allow" when the outpoint has no entry in mapLastMasternodeObject. So a masternode that never lands a successful trigger never gets a rate-buffer entry at all, and every subsequent malformed-but-signed trigger passes the rate check unbounded. The bypass defeats precisely the limiter designed to stop it.

Each rejected trigger still costs a BLS verification, a mapObjects entry held for the deletion delay, a governance.dat write, and - after erasure - a mapErasedGovernanceObjects entry retained until roughly 60 days on mainnet. That last accumulator is the component that actually persists.

Triggering requires a valid operator key for a masternode in the tip DMN list, and the victim must have requested the hash via INV. Objects are not relayed on this path, so there is no fan-out and the attacker must connect to each victim directly.

What was done?

• Move MasternodeRateUpdate() above the AddTrigger check so the rate buffer is advanced on the failure path too. The five-slot rate buffer then sticks, since it only advances on accept.
• Extract relay scheduling so a trigger that was just marked deleted does not get added to the additional-relay set. This second commit is a necessary self-correction: it prevents a regression the first commit would otherwise introduce, rather than fixing a pre-existing bug.
• Drop internal audit finding identifiers from the added comments while keeping the technical rationale.

The rate buffer cannot be gamed by spreading creation timestamps: the accepted timestamp window is narrower than the spread that would be needed to keep the computed rate below the maximum.

How Has This Been Tested?

The first commit adds a regression test showing the failed-trigger path must advance the masternode rate limit, ordered before the fix.

Full build and test validation is delegated to CI on this PR; the changes were not built locally.

Breaking Changes

None.

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

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 17 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: 82a1f5ba-8775-4bc4-aa8b-011a88edec3e

📥 Commits

Reviewing files that changed from the base of the PR and between f1dde51 and 7c7bda9.

📒 Files selected for processing (4)

• src/Makefile.test.include
• src/governance/governance.cpp
• src/governance/governance.h
• src/test/governance_failed_trigger_rate_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]

✅ Final review complete — no blockers (commit 7c7bda9)

[thepastaclaw]

Final validation — Codex + Sonnet

The final code correctly advances the masternode rate buffer for failed triggers while deferring relay scheduling until after the trigger is retained, and the tests cover both behaviors. No correctness blockers remain, but two same-stack fixup commits should be folded into their originating commits to keep the history atomic and avoid preserving a known regression.

Review provenance

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

🟡 2 suggestion(s)

2 additional finding(s) omitted (not in diff).

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `<commit:217a4097042>`:
- [SUGGESTION] <commit:217a4097042>:1: Squash the relay self-correction into the original fix
  Commit 8862d235bb6 moves MasternodeRateUpdate() above the AddTrigger check while that function still inserts near-future triggers into setAdditionalRelayObjects. This creates a new intermediate regression where a malformed trigger marked for deletion can later be re-announced and served on GETDATA. Commit 217a4097042 correctly separates rate accounting from relay scheduling and adds targeted coverage, but it explicitly repairs behavior introduced by the preceding commit in this PR. Fold its governance and regression-test changes into 8862d235bb6 so the rate-limit fix is atomic and no retained commit contains the known relay-amplification regression.

In `<commit:39ba2d8ba97>`:
- [SUGGESTION] <commit:39ba2d8ba97>:1: Fold the audit-ID cleanup into the originating commits
  Commit 39ba2d8ba97 only removes private audit identifiers from two comments introduced earlier in this same stack, making it a fixup commit under CONTRIBUTING.md's definition of commits that repeatedly change the same lines. It also leaves U009/U003 in the permanent messages of 8862d235bb6 and 217a4097042, so the stated cleanup remains incomplete. Amend the originating comments and commit messages to omit the identifiers, then drop this standalone cleanup commit.

[thepastaclaw]

Final validation — Codex + Sonnet

The final tree correctly advances the masternode rate buffer for failed triggers while scheduling deferred relay only after a trigger is retained, and the tests cover both paths. Carried forward from the prior review, two commit-history suggestions remain unresolved: the same-PR relay self-correction should be folded into the original fix, and the audit-ID cleanup should be folded into the commits that introduced those references. The latest delta is a pure rebase with identical PR patches, so no new defects or findings were introduced.

Review provenance

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

🟡 2 suggestion(s)

2 additional finding(s) omitted (not in diff).

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `<commit:6a36c89a4a6>`:
- [SUGGESTION] <commit:6a36c89a4a6>:1: Squash the relay self-correction into the original fix
  Commit 643ba6d2068 moves MasternodeRateUpdate() above the AddTrigger check while deferred-relay scheduling is still a side effect of that function. At this intermediate commit, a malformed trigger rejected by AddTrigger and marked for deletion can still be queued in setAdditionalRelayObjects, later re-announced, and served on GETDATA. Commit 6a36c89a4a6 correctly extracts ScheduleAdditionalRelay() and calls it only after AddTrigger succeeds, but this repairs a regression introduced by the immediately preceding commit in the same PR. Fold the production correction and its relay regression coverage into the original fix so every retained commit contains the complete rate-limit fix without the known relay-amplification path.

In `<commit:7c7bda98220>`:
- [SUGGESTION] <commit:7c7bda98220>:1: Fold the audit-ID cleanup into the originating commits
  Commit 7c7bda98220 only removes private U009/U003 identifiers from two comments introduced earlier in this same stack, making it an intra-stack fixup rather than an independent documentation change. The identifiers also remain in the permanent messages of commits 643ba6d2068 and 6a36c89a4a6. Amend the originating comments and commit messages to omit the private identifiers, then drop the standalone cleanup commit so the cleanup is complete and the history remains atomic.