fix: update masternode rate limit on failed governance trigger path - #7521
fix: update masternode rate limit on failed governance trigger path#7521PastaPastaPasta wants to merge 3 commits into
Conversation
WalkthroughTrigger rate usage is recorded before governance trigger validation. Failed Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant TriggerSubmitter
participant CGovernanceManager
participant RateLimiter
participant RelayScheduler
TriggerSubmitter->>CGovernanceManager: Submit trigger
CGovernanceManager->>RateLimiter: Record trigger usage
CGovernanceManager->>CGovernanceManager: Validate trigger
alt Valid trigger
CGovernanceManager->>RelayScheduler: Schedule future relay if required
else Failed trigger
CGovernanceManager->>CGovernanceManager: Mark for deletion
end
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
⛔ Blockers found — Opus deferred (commit 7f3a826) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
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.
39ba2d8 to
7c7bda9
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
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.
7c7bda9 to
1d6dda8
Compare
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
LGTM; but this one is a bit interesting enough I want another's review
|
PR #7536 adds shared |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
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)
1d6dda8 to
4e76c10
Compare
![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: 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".
|
|
||
| int64_t nTimestamp = govobj.GetCreationTime(); | ||
| it->second.triggerBuffer.AddTimestamp(nTimestamp); | ||
| it->second.triggerBuffer.AddTimestamp(govobj.GetCreationTime()); |
There was a problem hiding this comment.
On testnet/devnet, where the superblock cycle is one hour, the accepted creation-time window spans three cycles (now - 2 * cycle through now + 1h), while the rate check permits a full buffer whenever its timestamp span exceeds 5 * cycle / 2.2, or about 2.27 cycles. A valid operator can therefore alternate malformed signed triggers between the window endpoints; every five-entry buffer retains both endpoints, GetRate() stays below dMaxRate, and every failed AddTrigger continues entering mapObjects. Regtest is similarly affected, so the consecutive-timestamp test passes while the flood remains unbounded on these networks. Record a non-attacker-controlled receipt time for failed attempts, or otherwise constrain this buffer, and test endpoint-spaced timestamps.
AGENTS.md reference: AGENTS.md:L166-L166
Useful? React with 👍 / 👎.
|
This pull request has conflicts, please rebase. |
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
LGTM; but this one is a bit interesting enough I want another's review
knst
left a comment
knst
left a comment
There was a problem hiding this comment.
implementation is fine and regressions test idea is fine.
Though, impplementation of regressions tests requires re-working
| // message would then fan out to every peer, which in turn re-announce it. | ||
| BOOST_AUTO_TEST_CASE(failed_trigger_is_not_scheduled_for_additional_relay) | ||
| { | ||
| SetMockTime(1'700'000'000s); |
There was a problem hiding this comment.
why? regresions tests usually use 0 or now
1700000000 looks strange because it won't be updated every year anyway
There was a problem hiding this comment.
idk? does it matter?
There was a problem hiding this comment.
Fair point — switched these to SetMockTime(0s). The absolute epoch value wasn't load-bearing; we only need a deterministic clock inside the accepted rate-check window.
| COutPoint mn_outpoint; | ||
| CBLSSecretKey operator_key; | ||
|
|
||
| FailedTriggerRateSetup() : |
There was a problem hiding this comment.
there's not need to create new file and new FailedTriggerRateSetup testing environment.
Re-use src/test/governance_inv_tests.cpp, this file is almost identical copy of that one.
There was a problem hiding this comment.
Done: the regressions now live in src/test/governance_inv_tests.cpp and the standalone file is gone.
I kept a dedicated FailedTriggerRateSetup (TestChainSetup + ProRegTx + sync FINISHED) rather than reusing GovernanceInvSetup, because the INV fixture is MAIN/TestingSetup without a tip MN list or operator key. ProcessObject requires IsValidLocally (signature + DMN membership) before the failed-AddTrigger / rate-buffer path is exercised, so the INV fixture can't cover this regression.
There was a problem hiding this comment.
Follow-up: folded further so there is only one suite/fixture in this file — governance_inv_tests / GovernanceInvSetup. The complex DIP3 setup now also covers the INV/vote cases.
|
|
||
| /** Queue a deferred re-announcement for a trigger that is too new to propagate | ||
| * reliably yet. Only call this for objects we are keeping. */ | ||
| void ScheduleAdditionalRelay(const CGovernanceObject& govobj) |
There was a problem hiding this comment.
nit: it works with triggers only ; consider renaming it to ScheduleTriggerRelay or something similar for clarity. Edit comment also, it is not 'for objects' it is 'for triggers' only.
There was a problem hiding this comment.
Agreed — renamed to ScheduleTriggerRelay and updated the comments to say triggers only.
|
Addressed @knst's review in a03a5e6:
Kept a separate Verified:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/test/governance_inv_tests.cpp (1)
628-631: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the near-future offset from the named constants.
3550encodes the current values ofMAX_TIME_FUTURE_DEVIATIONandRELIABLE_PROPAGATION_TIME. If either constant changes,near_futuremay fall out of theScheduleTriggerRelay()arming window, and this test stops covering the deferred-relay regression. Compute the offset from the constants, and make thenow + 120readiness step depend onRELIABLE_PROPAGATION_TIMEas 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 valueUse
GetAdjustedTime()for the addable-relay threshold.
ScheduleTriggerRelayarming usesGetTime(), butCheckPostponedObjectschecks validity and readiness forsetAdditionalRelayObjectswithGetAdjustedTime(). When the network offset is non-zero, the extra-relay decision can disagree with the drained readiness decision; useGetAdjustedTime()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
📒 Files selected for processing (3)
src/governance/governance.cppsrc/governance/governance.hsrc/test/governance_inv_tests.cpp
|
Follow-up for suite naming / single-setup feedback in 5b4cc0f:
Verified: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/test/governance_inv_tests.cpp (1)
116-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify 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 to0sfrom 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
📒 Files selected for processing (1)
src/test/governance_inv_tests.cpp
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
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.
|
|
||
| int64_t nTimestamp = govobj.GetCreationTime(); | ||
| it->second.triggerBuffer.AddTimestamp(nTimestamp); | ||
| it->second.triggerBuffer.AddTimestamp(govobj.GetCreationTime()); |
There was a problem hiding this comment.
🔴 Blocking: 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.
source: ['codex']
|
This pull request has conflicts, please rebase. |
AddGovernanceObjectInternal emplaced a trigger into mapObjects, attempted AddTrigger, and on failure returned early before MasternodeRateUpdate. MasternodeRateCheck treats a missing rate-buffer entry as allow, so a masternode that never lands a successful trigger was never rate-limited at all, defeating precisely the limiter meant to stop it. Each rejected trigger still cost a BLS verification and left a mapErasedGovernanceObjects entry retained for roughly 60 days on mainnet. Advance the rate buffer on the failure path too. Relay scheduling is extracted so a trigger just marked deleted is not added to the additional-relay set; that part is a self-correction for a regression this change would otherwise introduce, not a pre-existing bug.
Move the failed-trigger rate regressions into governance_inv_tests.cpp, use SetMockTime(0s), and rename ScheduleAdditionalRelay to ScheduleTriggerRelay since it only applies to triggers.
Fold the failed-trigger rate regressions into the single governance_inv_tests suite. GovernanceInvSetup now owns the DIP3 / ProRegTx path used by those cases and also covers the INV/vote tests.
5b4cc0f to
7f3a826
Compare
There was a problem hiding this comment.
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 winExercise the invalid-signature path for a registered masternode.
Line 534 is incorrect.
GovernanceInvSetupregistersmn_outpointat 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 frommn_outpointwith 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
📒 Files selected for processing (3)
src/governance/governance.cppsrc/governance/governance.hsrc/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
There was a problem hiding this comment.
💡 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".
| CBLSSecretKey operator_key; | ||
|
|
||
| GovernanceInvSetup() : | ||
| TestChainSetup(/*num_blocks=*/107, CBaseChainParams::REGTEST, {"-dip3params=109:500"}) |
There was a problem hiding this comment.
Because BOOST_FIXTURE_TEST_SUITE constructs GovernanceInvSetup separately for every test case, changing the shared fixture to TestChainSetup(/*num_blocks=*/107, ...) makes each of the six existing lightweight INV/vote tests mine 107 blocks, activate DIP3, mine two more blocks, and register a ProTx even though only the two new rate-limit tests use mn_outpoint or operator_key. This unnecessarily multiplies expensive chain and database setup across the suite; keep the lightweight fixture for the existing cases and place the two trigger tests under a separate chain fixture, which can remain in this file.
AGENTS.md reference: AGENTS.md:L100-L106
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
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.
| TestChainSetup(/*num_blocks=*/107, CBaseChainParams::REGTEST, {"-dip3params=109:500"}) | ||
| { |
There was a problem hiding this comment.
🟡 Suggestion: 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.
source: ['codex']
| SetMockTime(0s); | ||
| const int64_t now = GetTime<std::chrono::seconds>().count(); |
There was a problem hiding this comment.
🟡 Suggestion: 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.
| SetMockTime(0s); | |
| const int64_t now = GetTime<std::chrono::seconds>().count(); | |
| const int64_t now = GetTime<std::chrono::seconds>().count(); | |
| SetMockTime(std::chrono::seconds{now}); |
source: ['codex']
3 participants
Issue being fixed or feature implemented
In
AddGovernanceObjectInternal(), a trigger object is emplaced intomapObjects, thenAddTrigger()is attempted; on failure the function callsPrepareDeletion()and returns early - beforeMasternodeRateUpdate().MasternodeRateCheck()short-circuits with "allow" when the outpoint has no entry inmapLastMasternodeObject. 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
mapObjectsentry held for the deletion delay, agovernance.datwrite, and - after erasure - amapErasedGovernanceObjectsentry 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?
MasternodeRateUpdate()above theAddTriggercheck so the rate buffer is advanced on the failure path too. The five-slot rate buffer then sticks, since it only advances on accept.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?
Adds failed-trigger rate regressions in
src/test/governance_inv_tests.cppshowing the failed-trigger path must advance the masternode rate limit and must not schedule deferred trigger relay. Chain/ProRegTx plumbing uses the sharedsrc/test/util/masternode.hmodule from #7536.make -C src -j8 test/test_dash./src/test/test_dash --run_test=governance_failed_trigger_rate_tests,governance_inv_tests— passes.Remaining validation is delegated to CI on this PR.
Breaking Changes
None.
Checklist: