fix: bound governance inv request cache#7425
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThe governance manager now stores requested inventory hashes in a bounded 50,000-entry cache. It prunes expired entries, rejects duplicates, evicts entries when saturated, and updates acceptance, clearing, invalid-vote removal, and cleanup to use the new cache. Tests cover expiry, bounds, saturation, FIFO eviction, and re-announcement behavior. Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Peer
participant CGovernanceManager
participant RequestedHashCache
Peer->>CGovernanceManager: ConfirmInventoryRequest(inv)
CGovernanceManager->>RequestedHashCache: Check hash presence
alt cache full and cleanup due
CGovernanceManager->>CGovernanceManager: PruneExpiredRequestedHashes(now)
CGovernanceManager->>RequestedHashCache: Remove expired entries
end
CGovernanceManager->>RequestedHashCache: Insert hash and expiry
RequestedHashCache-->>CGovernanceManager: Evict oldest entry when full
Peer->>CGovernanceManager: AcceptMessage(nHash)
CGovernanceManager->>RequestedHashCache: HasKey and erase hash
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
✅ Review complete (commit 93667f0) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers: opus general failed_quota, gpt-5.5 general completed, opus dash-core-commit-history failed_quota, gpt-5.5 dash-core-commit-history completed; verifier: gpt-5.5 completed after opus verifier failed_quota.
Verified the worktree is exactly at 8b73122 and inspected the governance request-cache changes, related networking paths, cleanup behavior, and added unit coverage. No concrete agent or CodeRabbit findings were present, and I found no in-scope correctness issues to carry forward.
No in-scope findings.
Note: GitHub does not allow me to submit an APPROVE review on my own PR, so this is posted as a COMMENT while preserving the verified clean result.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/governance/governance.cpp (1)
593-616: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffVerify
unordered_lru_cachefit for this bounded/TTL cache.The repository's coding guidelines state that
src/{masternode,llmq,evo,coinjoin,governance}/**/*.{cpp,h}should useunordered_lru_cachefor LRU-evicted caching.m_requested_hash_timeis now hand-bounded with a custom cap + TTL-pruning scheme instead. Since this cache needs time-based expiry (not pure recency-based LRU eviction), migrating may not be a clean fit — worth confirming whetherunordered_lru_cachesupports TTL-style eviction before deciding to refactor.Logic itself (opportunistic prune-then-cap-check in Lines 594-612) is correct: pruning is only attempted when the cache is already full and the tracked next-cleanup time has passed, and insertion is gated on the post-prune size.
🤖 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 593 - 616, The current m_requested_hash_time cache in CGovernanceManager::ConfirmInventoryRequest is manually bounded with TTL pruning, so first confirm whether unordered_lru_cache can support this time-based expiry model before refactoring. If it cannot express TTL-style eviction cleanly, keep the existing prune-then-cap logic and document that this cache is intentionally not a pure LRU case; if it can, migrate the cache while preserving the current full-cache pruning and insertion gating behavior.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 593-616: The current m_requested_hash_time cache in
CGovernanceManager::ConfirmInventoryRequest is manually bounded with TTL
pruning, so first confirm whether unordered_lru_cache can support this
time-based expiry model before refactoring. If it cannot express TTL-style
eviction cleanly, keep the existing prune-then-cap logic and document that this
cache is intentionally not a pure LRU case; if it can, migrate the cache while
preserving the current full-cache pruning and insertion gating behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 62f71a87-31c6-42e9-b8b5-1281b0e482db
📒 Files selected for processing (3)
src/governance/governance.cppsrc/governance/governance.hsrc/test/governance_inv_tests.cpp
There was a problem hiding this comment.
Code Review
Source: reviewers: opus general failed_quota, gpt-5.5 general completed, opus dash-core-commit-history failed_quota, gpt-5.5 dash-core-commit-history completed; verifier: gpt-5.5 completed after opus verifier failed_quota.
Prior reconciliation: the previous exact-SHA review at 8b73122 had zero verified findings, so there are no carried-forward prior findings.
Latest delta: 43b1ab8 only adds the lint-required /* Continued */ marker to the multiline governance LogPrint. That does not introduce a runtime issue, but it is a commit-history fixup to the preceding cache-bound commit.
Cumulative current-head review found no additional code defects. I did not run tests in this review worktree because src/test/test_dash is not built there.
1 suggestion:
Review finding details
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `<commit:43b1ab8>`:
- [SUGGESTION] <commit:43b1ab8>:1: Squash the one-line log-continuation fixup
Commit 43b1ab805111df18bebfe2f2790a85fdb06e5d79 only changes the LogPrint call introduced by the immediately preceding governance cache-bound commit to add the continuation marker. As permanent history, this is a review fixup rather than a standalone logical change, so it adds avoidable noise to git log and blame for the actual cache fix. Squash it into 8b73122a5466035ec9cdf3e6281bc445e5e82d9a before merge.
|
Checked the |
thepastaclaw
force-pushed
the
fix/governance-inv-request-bound
branch
from
43b1ab8 to
45e84b5
Compare
|
Addressed the commit-history review finding by squashing the one-line log-continuation fixup into the original governance cache-bound commit.\n\n- Fixed: |
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers: opus general failed_quota, gpt-5.5 general completed, opus dash-core-commit-history failed_quota, gpt-5.5 dash-core-commit-history completed; verifier: gpt-5.5 completed after opus verifier failed_quota.
Verified current head 45e84b5 against the changed governance cache code, tests, and prior review artifact. The cache bound preserves duplicate handling and response gating, the expiry marker is updated on prune/clear, and the focused tests cover the new cap and expiration path. No in-scope findings remain, and the prior one-line fixup history finding is resolved by the single squashed commit.
Prior reconciliation: prior-1 is FIXED. The previous suggestion to squash the standalone 43b1ab80 log-continuation fixup no longer applies because the current stack is a single substantive commit at 45e84b51.
Carried-Forward Prior Findings: None.
New Findings In Latest Delta: None.
No in-scope findings.
Note: GitHub does not allow me to submit an APPROVE review on my own PR, so this is posted as a COMMENT while preserving the verified clean result.
|
Status recheck: #7414 has merged, and this PR is still conflict-clean at |
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
Independent current-head DoS review found a saturation policy that turns the memory cap into a global governance-intake eclipse.
![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: 18d4725d99
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: reviewers claude/general=opus(completed), codex/general=gpt-5.5(failed), claude/dash-core-commit-history=opus(completed), codex/dash-core-commit-history=gpt-5.5(failed); verifier=opus
The bounded FIFO cache correctly routes every erase path through EraseRequestedHash so the map and ordering list stay in lockstep, and Clear/Prune reset auxiliary state. Test coverage (bounded, liveness-under-saturation, accept-then-reannounce, expiration cycle) is solid; no correctness, security, or consensus concerns found. Two non-blocking observations: stale test doc comments describing a desync the PR itself eliminates, and a commit-history nit — the two-commit stack should be squashed because 18d4725 reworks the admission policy 45e84b5 just introduced before either has shipped, and Dash merges without squashing so bisect would land on the known-flawed intermediate.
Prior reconciliation: the prior exact-SHA review at 45e84b5 produced zero findings, so nothing is carried forward. Reviewers still confirmed the prior verified output was clean before concluding.
Carried-Forward Prior Findings: none. The prior review was clean; no prior findings exist to re-validate against current head 18d4725.
New Findings In Latest Delta: findings whose root cause is introduced by the 45e84b5..18d4725 delta are listed here, separately from any cumulative current-head findings.
💬 2 nitpick(s)
1 additional finding(s) omitted (not in diff).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/governance/governance.cpp (1)
637-657: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
CacheMap’s ordering contract is required here.PruneExpiredRequestedHashes()assumes the back ofGetItemList()is always the oldest entry, andCacheMapdoes use the back for eviction. Keep this dependency explicit, since any change toInsert/PruneLastordering would break the prune logic.🤖 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 637 - 657, Document the ordering dependency in CGovernanceManager::PruneExpiredRequestedHashes: explicitly state that CacheMap::GetItemList() keeps the oldest entry at the back because CacheMap eviction uses the back, and that changes to CacheMap::Insert or PruneLast ordering must preserve this contract.
🤖 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.
Outside diff comments:
In `@src/governance/governance.cpp`:
- Around line 637-657: Document the ordering dependency in
CGovernanceManager::PruneExpiredRequestedHashes: explicitly state that
CacheMap::GetItemList() keeps the oldest entry at the back because CacheMap
eviction uses the back, and that changes to CacheMap::Insert or PruneLast
ordering must preserve this contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 01f08ef2-25f4-472b-9570-5f4a727a3b12
📒 Files selected for processing (5)
src/governance/governance.cppsrc/governance/governance.hsrc/governance/net_governance.cppsrc/init.cppsrc/test/governance_inv_tests.cpp
✅ Files skipped from review due to trivial changes (2)
- src/governance/net_governance.cpp
- src/init.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/governance_inv_tests.cpp
There was a problem hiding this comment.
Code Review
Source: reviewers claude/general=opus(completed), codex/general=gpt-5.5(failed), claude/dash-core-commit-history=opus(completed), codex/dash-core-commit-history=gpt-5.5(failed); verifier=opus
Verified the CacheMap refactor at 759474f1 against cachemap.h, the literal 18d4725d..759474f1 delta, and the full PR diff. Insert deduplicates without reordering, evicts the oldest entry when full so honest INVs remain admissible, and every erase/clear path routes through CacheMap, keeping its index and ordering list synchronized. PruneExpiredRequestedHashes correctly walks the oldest entries under the monotonic-time ordering invariant. No correctness, security, or consensus concerns were found.
Prior Finding Reconciliation
- FIXED — stale test comments:
src/test/governance_inv_tests.cpp:192-215now describesCacheMapkeeping its index and ordering list in lockstep and explicitly says no stale ordering slot can linger. The stale-FIFO wording from the prior finding is gone. - STILL VALID — commit-stack squash: the prior suggestion to squash
18d4725dinto45e84b51remains valid and is broadened to include759474f1, which replaces the hand-rolled FIFO introduced by18d4725dwithCacheMapbefore any of the commits ship.
Carried-Forward Prior Findings
-
[SUGGESTION]
<commit:18d4725d>:1: Squash the three-commit stack into one — each commit reworks the previous before anything shipsThe three commits are a single never-shipped change iterated in place, not a refactor/feature/tests progression:
45e84b51addsMAX_REQUESTED_HASHESand makesConfirmInventoryRequestreturn false when the cache is full.18d4725didentifies that return-false behavior as an eclipse vector and replaces it with a hand-rolled FIFO: astd::list<uint256>mirror, iterator storage in the map value, andEraseRequestedHash/EvictOldestRequestedHashhelpers.759474f1then deletes that entire hand-rolled apparatus and re-implements the same behavior on the existingCacheMapabstraction.
Because Dash merges without squashing,
git bisectinto45e84b51lands on the deliberately known eclipse-vulnerable design, while18d4725dintroduces a structure discarded one commit later. Squash all three into onefix: bound governance inv request cachecommit that usesCacheMapfrom the start. The current-head code is correct; this is commit-history hygiene only.
New Findings In Latest Delta
None. The CacheMap replacement is behavior-preserving; it only broadens the evidence for the carried-forward squash suggestion above.
Cumulative Current-Head Findings
None beyond the carried-forward commit-history suggestion.
🟡 1 suggestion
🤖 Prompt for the review finding with AI agents
This finding is from an automated code review. Verify it against the current commit stack before acting.
- [SUGGESTION] <commit:18d4725d>:1: Squash the three-commit stack into one. Commits 45e84b51, 18d4725d, and 759474f1 are successive implementations of the same never-shipped bounded governance inventory request-cache change. Squash them into one commit that uses CacheMap from the start so develop history does not retain an eclipse-vulnerable intermediate and a hand-rolled FIFO discarded one commit later.
Bound pending governance inventory requests at the protocol INV limit while preserving intake liveness by evicting the oldest request when saturated. Keep expiry cleanup correct across wall-clock rollback by scanning all cached entries, and cover capacity, saturation, accept/reannounce ordering, rollback expiry, and real INV routing with unit tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
thepastaclaw
force-pushed
the
fix/governance-inv-request-bound
branch
from
759474f to
93667f0
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Source: orchestrator=openai/gpt-5.6-sol(high, orchestration-only); reviewers codex/general=gpt-5.6-sol(completed), sonnet5/general=claude-sonnet-5(failed), codex/dash-core-commit-history=gpt-5.6-sol(completed), sonnet5/dash-core-commit-history=claude-sonnet-5(failed); fallback verifier=gpt-5.6-sol (verifier-codex-7425-1783704169).
Prior Finding Reconciliation: prior-1 is FIXED. Range-diff and commit history confirm that the former three-commit stack was replaced by the single coherent commit 93667f0, eliminating the superseded intermediate designs and their bisect/blame concerns.
Carried-Forward Prior Findings: none.
New Findings In Latest Delta: none. The literal 759474f..93667f0 delta correctly scans every cached request when expiration order diverges after a wall-clock rollback, preserves valid entries, removes expired entries safely, and recalculates the earliest cleanup time. The added rollback test exercises this behavior.
Additional Cumulative Findings: one documentation nitpick. The test-only maximum-size accessor still describes the superseded drop-new policy even though the cumulative implementation admits new hashes by evicting the oldest cached request. No correctness, security, consensus, or commit-history defects were found. The cumulative PR diff is the single five-file commit above merge base f084efb; diff-check and the scoped whitespace lint pass.
💬 1 nitpick(s)
| * ConfirmInventoryRequest pending expiration in CheckAndRemove. */ | ||
| size_t RequestedHashCacheSizeForTesting() const | ||
| EXCLUSIVE_LOCKS_REQUIRED(!cs_store); | ||
| /** Test-only accessor: maximum inv hashes tracked before dropping new requests. */ |
There was a problem hiding this comment.
💬 Nitpick: Accessor comment describes the superseded drop-new policy
The comment says saturation drops new requests, but ConfirmInventoryRequest deliberately admits each new hash and CacheMap::Insert evicts the oldest cached request when the 50,000-entry limit is reached. This eviction behavior is the liveness property implemented at governance.cpp:602-613 and exercised by the saturation tests, so the test API documentation should describe FIFO eviction instead of the superseded refusal policy.
| /** Test-only accessor: maximum inv hashes tracked before dropping new requests. */ | |
| /** Test-only accessor: maximum inv hashes tracked; saturation evicts the oldest request. */ |
source: ['codex-general']
|
Closing as superseded by #7442, which removes the redundant governance-owned request cache and reuses the bounded per-peer net-layer request tracker instead. |
3 participants
Issue being fixed or feature implemented
Bounds the governance inventory request cache used to track pending governance object and vote requests.
Previously, unknown governance inventory hashes could be retained until the reliable-propagation timeout without a hard cache size limit. This change keeps that state finite while preserving duplicate handling and response acceptance semantics.
What was done?
MAX_REQUESTED_HASHEScap for pending governance inventory request hashes.How Has This Been Tested?
Tested locally on macOS in a fresh worktree rebased on current
upstream/develop:make -C src test/test_dash -j$(sysctl -n hw.ncpu)src/test/test_dash --run_test=governance_inv_testsgit diff --check upstream/develop..HEADCOMMIT_RANGE=upstream/develop..HEAD test/lint/lint-whitespace.pyupstream/develop..HEAD: no significant issues found; recommendation ship.Breaking Changes
None.
Checklist: