[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() allows any masternode that has no entry in mapLastMasternodeObject, and only MasternodeRateUpdate() creates that entry. A masternode that never lands a successful trigger therefore never gets a rate-buffer entry, and every malformed but signed trigger it sends passes the rate check. That defeats the limiter meant to stop exactly this.

Each rejected trigger still costs a BLS verification, a mapObjects entry held for the deletion delay, and, after erasure, a mapErasedGovernanceObjects entry retained for about two superblock cycles (roughly 60 days on mainnet).

Sending one needs a valid operator key for a masternode in the tip list, and the victim must have requested the hash after an INV. Rejected triggers are not relayed, so there is no fan-out; the attacker has to connect to each victim directly.

What was done?

• Call MasternodeRateUpdate() before the AddTrigger check, so failed attempts advance the masternode's rate buffer too.
• MasternodeRateUpdate() also queued near-future triggers for the deferred re-announcement in CheckPostponedObjects. That part is split out into ScheduleTriggerRelay() and still runs only after AddTrigger succeeds. Otherwise moving the rate update would make the node re-announce, and serve on GETDATA, a trigger it had just undone and marked for deletion.

Scope note: creation-time rate limiting on short-cycle networks

The rate buffer stores the object's signer-chosen creation time, and MasternodeRateCheck accepts creation times from now - 2 * cycle to now + 1h, with a maximum rate of 2.2 / cycle over five entries. On mainnet (cycle = 16616 blocks, about 28.8 days) the widest possible span is about 2 * cycle, so a full buffer is always rejected (5 / (2 * cycle) = 2.5 / cycle > 2.2 / cycle). On testnet, devnet (1 h cycle) and regtest the accepted window is wider than 5 / 2.2 cycles, so an operator who alternates creation times between the window ends can stay under the rate. That weakness is in the existing limiter and applies equally to well-formed triggers today. This PR only brings failed triggers under the same limiter. Moving the limiter to receipt time would change how valid triggers are throttled and is left for a separate change.

How Has This Been Tested?

Two new cases in src/test/governance_vote_processing_tests.cpp, using its existing chain-backed fixture, which already registers a masternode with a known operator key. No new file or fixture, and governance_inv_tests is unchanged.

• failed_triggers_advance_the_masternode_rate_limit: submits 12 signed triggers that fail AddTrigger and checks that exactly RATE_BUFFER_SIZE - 1 (4) are stored. It fails with 12 != 4 on develop's governance.cpp/governance.h.
• failed_trigger_is_not_scheduled_for_deferred_relay: a failed trigger created inside the deferred-relay window is never announced, even after it ages past RELIABLE_PROPAGATION_TIME. It fails when ScheduleTriggerRelay() is called before AddTrigger, which is the regression the split prevents.

Run locally on macOS arm64 with --enable-debug --enable-werror:

• src/test/test_dash --run_test=governance_vote_processing_tests,governance_inv_tests,governance_superblock_tests,governance_validators_tests passes.
• test/functional/test_runner.py feature_governance.py feature_governance_objects.py feature_governance_cl.py passes.
• test/lint/lint-whitespace.py passes.

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]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 1c3413e9-87fe-4f5e-be8f-0cb232a9cea6

📥 Commits

Reviewing files that changed from the base of the PR and between 7f3a826 and a551177.

📒 Files selected for processing (3)

• src/governance/governance.cpp
• src/governance/governance.h
• src/test/governance_vote_processing_tests.cpp

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.

---

Walkthrough

Governance trigger handling now records trigger creation times in the masternode rate buffer before registration. Rejected triggers are marked for deletion and are not relayed. Retained triggers can be scheduled for deferred relay when their creation time falls within the future-time reliability window. New tests check rate-buffer use and verify that failed triggers are not relayed immediately or after aging.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to a5511

No new merge-blocking risk is established for this change. The existing rate-limiter concern remains separately documented.

Security Architecture Review

Security architecture risk: 🔵 Low · up to a5511

The change strengthens failed-trigger throttling without adding a network entrypoint or announcing triggers rejected during registration. No introduced security regression was identified. Residual uncertainty concerns interruption and restart behavior, which the added tests do not exercise.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The affected network path is independently reachable on each victim node only after request authorization and validation of a trigger signed by an operator key for a masternode in that node's tip list. Registration failures are not announced by this path, so the changed rejection branch does not provide automatic relay fan-out.

Security Findings and Attack Paths

• observed — The base rejection branch skipped creation of the rate record, while the unchanged rate check allowed an outpoint with no record. The head closes that accounting gap for locally valid triggers rejected during registration. This condition predates the PR; no introduced or worsened attack path was established.

Trust Boundaries and Controls

• observed — Operator identity validation still precedes accounting. A cached rate-check bypass is followed by a forced check after successful local validation. Relay scheduling occurs only after registration succeeds; the deferred consumer subsequently checks object presence and timestamp readiness.

Resilience and Maintainability Implications

• observed — Persistence snapshots take the same store lock as admission. Object serialization preserves deletion time but not the cached deletion flag; initialization retries loaded trigger registration and reapplies deletion on failure. Deferred relay membership is volatile. These recovery mechanisms are unchanged by the PR, and complete interruption behavior was not demonstrated.

Hardening Proposals

• proposed — Add restart and serialization-round-trip coverage for rejected and retained near-future triggers, checking preservation of rate accounting, reapplication of deletion, and intended relay behavior. This would strengthen recovery assurance, not remedy an established PR regression.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly describes the main change: updating the masternode rate limit when a governance trigger fails.
Description check	✅ Passed	The description explains the failed-trigger rate-limit issue, the relay-scheduling change, and the regression tests. It is relevant to the changeset.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

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 a551177) · triage: low

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

[PastaPastaPasta]

LGTM; but this one is a bit interesting enough I want another's review

[PastaPastaPasta]

PR #7536 adds shared test/util/masternode.{h,cpp} helpers for this UTXO selection, funding, signing, and ProRegTx plumbing. If #7536 merges first, please rebase this PR and use <test/util/masternode.h> instead of retaining the local copy. The shared BuildSimpleUtxoMap also skips unspendable outputs.

[thepastaclaw]

Final validation — Codex + Sonnet

Independently verified: PR #7521 is now a single commit (1d6dda8) sitting directly on develop, containing the rate-buffer-update reordering, the ScheduleAdditionalRelay extraction gated to run only after AddTrigger succeeds, and both regression tests. The diff between the previously reviewed head (7c7bda9) and the current head is empty, confirming this is a history-only rewrite with no source changes. Both prior commit-hygiene findings are fixed: the self-correction is folded atomically into the fix, and no audit-ID identifiers remain in code, tests, or the commit message.

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 (failed), claude-sonnet-5 — general (completed), claude-sonnet-5 — dash-core-commit-history (failed), claude-sonnet-5 — dash-core-commit-history (failed), claude-sonnet-5 — dash-core-commit-history (failed), claude-sonnet-5 — dash-core-commit-history (failed), claude-sonnet-5 — dash-core-commit-history (completed)

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4e76c10a5d

ℹ️ 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".

[github-actions]

This pull request has conflicts, please rebase.

[PastaPastaPasta]

LGTM; but this one is a bit interesting enough I want another's review

[knst]

implementation is fine and regressions test idea is fine.
Though, impplementation of regressions tests requires re-working

[PastaPastaPasta]

Addressed @knst's review in a03a5e6:

• Moved failed-trigger rate regressions into src/test/governance_inv_tests.cpp (dropped the standalone file)
• Switched mock time to SetMockTime(0s)
• Renamed ScheduleAdditionalRelay → ScheduleTriggerRelay and clarified trigger-only comments

Kept a separate FailedTriggerRateSetup because GovernanceInvSetup can't exercise the signed-trigger / rate-buffer path (needs DIP3 tip MN + operator key).

Verified:

• ./src/test/test_dash --run_test=governance_failed_trigger_rate_tests,governance_inv_tests

[coderabbitai]

🧹 Nitpick comments (2)

src/test/governance_inv_tests.cpp (1)

628-631: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Derive the near-future offset from the named constants.

3550 encodes the current values of MAX_TIME_FUTURE_DEVIATION and RELIABLE_PROPAGATION_TIME. If either constant changes, near_future may fall out of the ScheduleTriggerRelay() arming window, and this test stops covering the deferred-relay regression. Compute the offset from the constants, and make the now + 120 readiness step depend on RELIABLE_PROPAGATION_TIME as well.

♻️ Proposed change to derive the offsets

-    const int64_t near_future = now + 3550;
+    // Halfway inside the arming window so the test stays valid if either constant changes.
+    const int64_t near_future = now + count_seconds(MAX_TIME_FUTURE_DEVIATION) -
+                                count_seconds(RELIABLE_PROPAGATION_TIME) / 2;

-    SetMockTime(std::chrono::seconds{now + 120});
+    SetMockTime(std::chrono::seconds{now} + RELIABLE_PROPAGATION_TIME + 1s);

🤖 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/test/governance_inv_tests.cpp` around lines 628 - 631, Update the
near_future setup in the governance inventory test to derive its offset from
MAX_TIME_FUTURE_DEVIATION and RELIABLE_PROPAGATION_TIME instead of the
hard-coded 3550, keeping it inside the ScheduleTriggerRelay() arming window.
Also replace the fixed now + 120 readiness step with an offset derived from
RELIABLE_PROPAGATION_TIME so both timing assumptions track the named constants.

src/governance/governance.cpp (1)

724-725: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Use GetAdjustedTime() for the addable-relay threshold.

ScheduleTriggerRelay arming uses GetTime(), but CheckPostponedObjects checks validity and readiness for setAdditionalRelayObjects with GetAdjustedTime(). When the network offset is non-zero, the extra-relay decision can disagree with the drained readiness decision; use GetAdjustedTime() here to keep the same clock path.

♻️ Proposed change

-    if (govobj.GetCreationTime() >
-        GetTime() + count_seconds(MAX_TIME_FUTURE_DEVIATION) - count_seconds(RELIABLE_PROPAGATION_TIME)) {
+    const auto now{std::chrono::time_point_cast<std::chrono::seconds>(GetAdjustedTime())};
+    if (govobj.CreationTime() > now + MAX_TIME_FUTURE_DEVIATION - RELIABLE_PROPAGATION_TIME) {

🤖 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/governance/governance.cpp` around lines 724 - 725, The future deviation
check in CheckPostponedObjects uses GetTime() for the addable-relay threshold,
but elsewhere in the same method GetAdjustedTime() is used for validity and
readiness checks. When network offset is non-zero, this clock inconsistency
causes the relay decision to disagree with the readiness decision. Replace
GetTime() with GetAdjustedTime() in the govobj.GetCreationTime() comparison to
ensure both relay-relay and readiness logic follow the same clock path.

Source: Coding guidelines

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

Nitpick comments:
In `@src/governance/governance.cpp`:
- Around line 724-725: The future deviation check in CheckPostponedObjects uses
GetTime() for the addable-relay threshold, but elsewhere in the same method
GetAdjustedTime() is used for validity and readiness checks. When network offset
is non-zero, this clock inconsistency causes the relay decision to disagree with
the readiness decision. Replace GetTime() with GetAdjustedTime() in the
govobj.GetCreationTime() comparison to ensure both relay-relay and readiness
logic follow the same clock path.

In `@src/test/governance_inv_tests.cpp`:
- Around line 628-631: Update the near_future setup in the governance inventory
test to derive its offset from MAX_TIME_FUTURE_DEVIATION and
RELIABLE_PROPAGATION_TIME instead of the hard-coded 3550, keeping it inside the
ScheduleTriggerRelay() arming window. Also replace the fixed now + 120 readiness
step with an offset derived from RELIABLE_PROPAGATION_TIME so both timing
assumptions track the named constants.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 130bcbdd-d48b-46cf-ad6b-892c23b4710f

📥 Commits

Reviewing files that changed from the base of the PR and between 9b103c0 and a03a5e6.

📒 Files selected for processing (3)

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

[PastaPastaPasta]

Follow-up for suite naming / single-setup feedback in 5b4cc0f:

• One suite only: BOOST_FIXTURE_TEST_SUITE(governance_inv_tests, GovernanceInvSetup) (matches filename)
• One fixture: GovernanceInvSetup is now the complex DIP3/TestChainSetup+ProRegTx setup and also wires the NetGovernance INV path, so the lighter setup is gone rather than duplicated
• Failed-trigger cases live in that same suite

Verified: ./src/test/test_dash --run_test=governance_inv_tests (6 cases)

[coderabbitai]

🧹 Nitpick comments (1)

src/test/governance_inv_tests.cpp (1)

116-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Clarify the fixture teardown after leaving mock time set.

SetMockTime(0s) clears the mock clock globally; this comment does not match that effect. The tests set per-case mock time instead. Reset to 0s from the destructor or remove the misleading fixture-level comment.

🤖 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/test/governance_inv_tests.cpp` around lines 116 - 117, Update the test
fixture around SetMockTime(0s) so teardown resets the global mock clock from its
destructor, or remove the misleading fixture-level comment if teardown already
performs the reset; keep per-case mock-time setup unchanged.

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

Nitpick comments:
In `@src/test/governance_inv_tests.cpp`:
- Around line 116-117: Update the test fixture around SetMockTime(0s) so
teardown resets the global mock clock from its destructor, or remove the
misleading fixture-level comment if teardown already performs the reset; keep
per-case mock-time setup unchanged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a1750e5a-7996-4083-a83c-7361dcf84048

📥 Commits

Reviewing files that changed from the base of the PR and between a03a5e6 and 5b4cc0f.

📒 Files selected for processing (1)

• src/test/governance_inv_tests.cpp

[thepastaclaw]

Preliminary review — Codex only

The production reordering correctly advances the failed-trigger rate buffer, relay scheduling remains gated until after AddTrigger succeeds, and both carried-forward findings remain fixed. However, failed attempts are still accounted using signer-controlled creation timestamps, and the exact rate math permits an operator to submit malformed triggers indefinitely on default testnet, devnet, and regtest by alternating timestamps across the accepted window. The two review-driven test follow-up commits should also be squashed into the originating fix, but that history issue is non-blocking.

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:711: Throttle failed triggers using non-attacker-controlled time
  The failed-trigger limiter records the operator-supplied creation timestamp, so the PR's flood protection remains bypassable on short-cycle networks. CRateCheckBuffer::GetRate() returns 5 / (max_timestamp - min_timestamp), while MasternodeRateCheck() accepts timestamps from now - 2 * cycle through now + 3600 and permits rates below 2.2 / cycle. On default testnet and devnet, cycle = 24 * 150 = 3600 seconds, making the accepted span 3 cycles and the endpoint-spaced rate 5 / (3 * cycle), which is below 2.2 / cycle. On regtest, cycle = 20 * 150 = 3000 seconds and the accepted span is 3.2 cycles, which also passes. By alternating unique, signature-valid malformed triggers between the accepted endpoints, every rolling five-entry buffer contains both endpoints, so every submission reaches BLS verification, AddTrigger, and mapObjects insertion indefinitely. Mainnet's roughly 2.001-cycle window is too narrow for this bypass, but this implementation and its stated protection are network-generic. Account failed attempts using receipt time or another non-signer-controlled value, and add an endpoint-spaced regression test rather than testing only consecutive creation timestamps.

In `<commit:a03a5e68893>`:
- [SUGGESTION] <commit:a03a5e68893>:1: Squash the review-driven test follow-ups into the fix
  Commits a03a5e68893 and 5b4cc0f9d1d are fixups to code introduced by 4e76c10a5da in the same unshipped stack. The first follow-up immediately deletes the standalone test file and Makefile entry created by the fix, relocates the same regressions, and renames the newly introduced helper; the second immediately rewrites that relocation to remove the extra fixture and suite. CONTRIBUTING.md specifically identifies commits that repeatedly change the same code as fixup commits that should be squashed. Fold both follow-ups into 4e76c10a5da so the final helper name, test location, and unified fixture land atomically without permanent blame churn.

[github-actions]

This pull request has conflicts, please rebase.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/test/governance_inv_tests.cpp (1)

534-536: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Exercise the invalid-signature path for a registered masternode.

Line 534 is incorrect. GovernanceInvSetup registers mn_outpoint at lines 67-77. MakeGovernanceVote() uses a different, unknown outpoint. This test passes on the unknown-masternode rejection path. It does not prove that an orphan vote from mn_outpoint with an invalid signature is rejected before caching.

Proposed test input

-    // The tip masternode list is empty in this setup, so no vote can name a known collateral.
-    const CGovernanceVote vote{MakeGovernanceVote(uint256S("51"))};
+    CGovernanceVote vote{mn_outpoint, uint256S("51"), VOTE_SIGNAL_FUNDING, VOTE_OUTCOME_YES};
+    vote.SetTime(GetTime<std::chrono::seconds>().count());
+    vote.SetSignature(std::vector<unsigned char>(CGovernanceVote::COMPACT_SIG_SIZE));

Based on PR objectives: orphan votes must prove masternode authorship before caching.

🤖 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/test/governance_inv_tests.cpp` around lines 534 - 536, Update the vote
construction in the governance inventory test to use the registered mn_outpoint
from GovernanceInvSetup while retaining an invalid signature. Ensure the
resulting orphan vote reaches invalid-signature validation for the known
masternode before caching, rather than the unknown-masternode rejection path.

🤖 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/test/governance_inv_tests.cpp`:
- Around line 584-633: Update failed_trigger_path_advances_masternode_rate_limit
to assert that exactly RATE_BUFFER_SIZE failed triggers are stored, replacing
the permissive upper-bound check while retaining the total-statistics
consistency checks.

---

Outside diff comments:
In `@src/test/governance_inv_tests.cpp`:
- Around line 534-536: Update the vote construction in the governance inventory
test to use the registered mn_outpoint from GovernanceInvSetup while retaining
an invalid signature. Ensure the resulting orphan vote reaches invalid-signature
validation for the known masternode before caching, rather than the
unknown-masternode rejection path.

🪄 Autofix

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: d28af221-d493-4ccd-96b3-6de90538a389

📥 Commits

Reviewing files that changed from the base of the PR and between 5b4cc0f and 7f3a826.

📒 Files selected for processing (3)

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

🚧 Files skipped from review as they are similar to previous changes (2)

• src/governance/governance.h
• src/governance/governance.cpp

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7f3a826a50

ℹ️ 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".

[thepastaclaw]

Preliminary review — Codex only

The production reordering records failed AddTrigger attempts and correctly avoids scheduling rejected triggers for deferred relay. However, the flood protection remains bypassable on testnet, devnet, and regtest because failed attempts are measured using signer-controlled creation timestamps; the test suite also introduces avoidable repeated chain setup and a wall-clock-sensitive relay regression. Source: reviewers gpt-5.6-sol (general and dash-core-commit-history); final verifier gpt-5.6-sol. openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.

Validated blockers were found in the Codex precheck. Opus 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 | 🟡 3 suggestion(s)

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

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 `<commit:6e468b85fba>`:
- [SUGGESTION] <commit:6e468b85fba>:1: Squash the review-driven test follow-ups into the fix
  Commits 6e468b85fba and 7f3a826a504 are fixups to material introduced by c2d8a9cf154 in this same unshipped stack. The first deletes the standalone test file and Makefile entry introduced by c2d8a9cf154, relocates the regressions, and renames the new relay helper; the second immediately rewrites that relocation to replace the separate fixture and suite with the final unified setup. CONTRIBUTING.md identifies commits that repeatedly change the same code as fixup commits that may need squashing. Fold both follow-ups into c2d8a9cf154 so the final helper name, test location, and fixture arrangement land atomically without permanent add/delete and fixture-rewrite churn.

In `src/test/governance_inv_tests.cpp`:
- [SUGGESTION] src/test/governance_inv_tests.cpp:56-57: Keep lightweight INV tests off the chain fixture
  BOOST_FIXTURE_TEST_SUITE constructs GovernanceInvSetup separately for every test case. Changing it to TestChainSetup means each of the six pre-existing lightweight INV/vote cases now mines 107 blocks, mines two additional blocks, registers a ProTx, and repeats chain/database initialization even though only the two new failed-trigger regressions use mn_outpoint or operator_key. That adds 654 unnecessary block constructions to the pre-existing cases. Keep their lightweight fixture and put the two signed-trigger cases under a separate chain fixture in this file.
- [SUGGESTION] src/test/governance_inv_tests.cpp:644-645: Freeze time in the deferred-relay regression
  SetMockTime(0s) disables mock time because NodeClock::now() falls back to the system clock when the stored value is zero. The trigger is only ten seconds inside the deferred-relay arming interval: creation_time is captured_now + 3550, while scheduling requires it to remain greater than current_time + 3540. If signing and processing take ten seconds or more on an instrumented runner, even the buggy scheduling behavior does not queue the trigger and the regression passes falsely. Capture the current wall time and immediately freeze it at that positive value before constructing the trigger.

In `src/governance/governance.cpp`:
- [BLOCKING] src/governance/governance.cpp:714: Throttle failed triggers using non-attacker-controlled time
  (existing thread: https://github.com/dashpay/dash/pull/7521#discussion_r3715379072)
  The failed-trigger limiter records the operator-supplied creation timestamp, so the PR's flood protection remains bypassable on short-cycle networks. CRateCheckBuffer::GetRate() returns 5 / (max_timestamp - min_timestamp), while MasternodeRateCheck() accepts timestamps from now - 2 * cycle through now + 3600 and accepts rates below 2.2 / cycle. Testnet and default devnet use a 3600-second cycle, giving an accepted span of three cycles and an endpoint-spaced rate of 5 / (3 * cycle), below the limit. Regtest uses a 3000-second cycle and has a 3.2-cycle accepted span, which also passes. A valid operator can alternate unique, signature-valid malformed triggers near both endpoints so every rolling five-entry buffer retains the full span; each submission continues through BLS verification, AddTrigger, and mapObjects insertion. Mainnet's roughly 2.001-cycle window is too narrow for this technique, but the implementation and stated protection are network-generic. Account failed attempts using receipt time or another non-signer-controlled value, and add a regression using endpoint-spaced creation timestamps.

[thepastaclaw]

Re-review — Final validation — Phase 1 + Phase 2

The complete diff at a551177 correctly counts locally validated failed triggers while keeping deferred relay scheduling after successful trigger registration. No in-scope defects were confirmed; the prior creation-time limiter concern is pre-existing and explicitly deferred. Verification was static only, and the supplied exact-head CI snapshot reports successful source builds, tests, and lint with no pending checks.

🔴 0 blocking | 🟡 0 suggestion(s) | 💬 0 nitpick(s)

Review provenance

Source: reviewer 1: glm-5.3-flash (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)

• Triage: low by gpt-6.1-sol (effort low) — The diff is a small, contained control-flow fix that counts rejected triggers in the existing rate buffer while preserving successful-trigger-only deferred relay scheduling, with focused regression tests.
• Phase 1 reviewers: glm-5.3-flash — general (completed, effort high); agent phase1-reviewer
• Phase 1 model: glm-5.3-flash — zai quota: 5h 91% left, weekly 94% left; passed over gemini-3.8-flash-high (antigravity below 15% reserve: weekly 15% left, 5h 100% left)
• Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
• Fresh verifier: gpt-6.1-sol — final-verifier; agent sol-verifier
• Phase 2 reviewers: gpt-6.1-sol — general (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — general (completed, effort medium); agent phase2-reviewer, gpt-6.1-sol — dash-core-commit-history (completed, effort medium); agent phase2-reviewer

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify the current code and confirm that no unresolved issues remain.

No unresolved findings remain from the prior review on this head.

[knst]

LGTM a551177