fix: update masternode rate limit on failed AddTrigger path

[PastaPastaPasta]

Audit finding U009 (high, CONFIRMED — downgraded from critical), plus U003, the same defect reported from a second audit region.

Issue

CGovernanceManager::AddGovernanceObjectInternal called MasternodeRateUpdate only after the AddTrigger check. The failed-trigger path returned early, so the rate buffer was never advanced.

MasternodeRateCheck short-circuits return true when mapLastMasternodeObject has no entry for the outpoint, and MasternodeRateUpdate is the sole writer of that map. A masternode operator key submitting only unparseable triggers therefore stayed absent from the map forever and was never throttled — one key could flood mapObjects without limit.

Fix

• src/governance/governance.cpp: call MasternodeRateUpdate before the AddTrigger check, so every attempt counts against the per-masternode buffer.

The review follow-up fixes a problem introduced by that hoist. MasternodeRateUpdate did two unrelated things — advance the rate buffer, and insert into setAdditionalRelayObjects for triggers created close to MAX_TIME_FUTURE_DEVIATION. Moving it above the check therefore armed deferred re-announcement for objects the node had just undone and PrepareDeletion-ed. Since CheckPostponedObjects only inspects fValid/fReady and never the delete flag, those objects would be announced to every peer and served on GETDATA — restoring the fan-out amplification the fix exists to remove.

The relay scheduling is now a separate ScheduleAdditionalRelay() called only after the check passes. Rate accounting counts every attempt; only kept objects are announced.

Tests

test: failed-trigger path must advance masternode rate limit precedes the fix. Verified by reverting: without the fix 12/12 flooded objects are accepted, with it 6.

Review notes

Wrongful-lockout was checked explicitly, since rate-limit bugs cut both ways. An honest MN creates one trigger per superblock cycle via CreateGovernanceTrigger, and only if it is the projected payee — and it pre-checks MasternodeRateCheck and declines rather than burning buffer slots. With RATE_BUFFER_SIZE = 5 the buffer must span a very short window to trip, so honest cadence is nowhere near it. mapLastMasternodeObject is persisted and keyed on the collateral outpoint, so a reconnect does not reset it.

Known-remaining, deliberately out of scope: GetRate() returns 0.0 below 5 samples, so a bounded 5-object burst per MN is still free; and TRIGGER vchData has no size cap (MAX_DATA_SIZE is enforced only for PROPOSALs). The latter likely matters more than the count limit.

Based on dashpay/dash develop @ 6d04c60ef36. Not rebase-tested against a newer tip; full functional suite not run.

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 0a0096ec-00b6-4c17-8bce-20f9ce17d372

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch sec/u009

---

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.}