fix: bound mnListsCache admission to stop getmnlistd memory exhaustion - #7485
fix: bound mnListsCache admission to stop getmnlistd memory exhaustion#7485PastaPastaPasta wants to merge 2 commits into
Conversation
|
⛔ Blockers found — Opus deferred (commit 0340fcd) |
Walkthrough
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/evo_deterministicmns_tests.cpp`:
- Around line 1705-1741: Update the cache-eviction test around expected and the
subsequent sweep to guarantee eviction: after recording expected, mine more than
CDeterministicMNManager::MAX_CACHE_LISTS additional blocks, refresh tip_index(),
and then re-query the original pindex entries. Keep the test focused on
verifying GetListForBlock results remain identical after those entries are
forced out and recomputed.
🪄 Autofix (Beta)
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: 23129f54-6606-4ad4-a2bb-91e7627c6863
📒 Files selected for processing (3)
src/evo/deterministicmns.cppsrc/evo/deterministicmns.hsrc/test/evo_deterministicmns_tests.cpp
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
Verified against source: the cache-bounding fix is logically sound — admission goes through CacheMNList()/CacheMNListDiff(), the rebuild walk correctly admits diffs unconditionally and defers EnforceDiffsCacheLimit() to after the walk (confirmed no mid-walk eviction remains), and the entire GetListForBlockInternal call including cache enforcement runs under a single cs lock so no cross-call overshoot is observable. No blocking issues found. The regression test is a real but narrow improvement over the pre-fix state; both Codex and CodeRabbit independently and correctly show it never crosses the diff cap or recency boundary and doesn't provably force eviction-then-recompute of the recorded 'expected' values before checking them. Commit-history findings about squashing the intermediate mid-walk-eviction bugfix and the intentionally-failing test commit are valid hygiene notes but don't rise to blocking since nothing in the stack fails to build or creates a severe bisect trap.
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 (completed)
🟡 4 suggestion(s) | 💬 1 nitpick(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/test/evo_deterministicmns_tests.cpp`:
- [SUGGESTION] src/test/evo_deterministicmns_tests.cpp:1699-1747: Regression test doesn't cross the diff cap/recency boundary and may never force eviction of the values it checks
n_blocks is MAX_CACHE_LISTS+64 = 320, but LIST_DIFFS_CACHE_SIZE is 2880 and MAX_CACHE_DIFFS is 2944, and ShouldRetainCacheHeight()'s recency window is also LIST_DIFFS_CACHE_SIZE blocks. Every height touched by this test sits well inside that window and far below the diff cap, so an implementation that dropped ShouldRetainCacheHeight() or EnforceDiffsCacheLimit() entirely would still pass. Separately, the 'expected' values are recorded via GetListForBlock() calls that themselves re-populate the cache, and the final descending sweep over every height in [tip-319, tip] revisits those exact heights again before the closing comparison loop runs — GetListForBlockInternal returns straight from cache on a hit with no eviction risk, so there's no guarantee any specific 'expected' entry was ever actually evicted-and-rebuilt by the time it's re-checked; the assertion can pass purely on cache hits. Strengthen this by (1) mining more than MAX_CACHE_DIFFS distinct blocks after recording 'expected' to force both the diff cap and a height outside the recency window, and (2) explicitly forcing eviction of a specific known-resident entry (e.g. mine MAX_CACHE_LISTS additional blocks and advance the tip) before re-querying it, ideally with a non-trivial (non-empty) MN state diff so the equality check is substantive rather than comparing empty lists.
- [SUGGESTION] src/test/evo_deterministicmns_tests.cpp:1683-1747: Test exercises the manager API directly, not the actual getmnlistd P2P request path
mn_lists_cache_bounded correctly drives CDeterministicMNManager::GetListForBlock directly, but the PR's stated threat model is an unauthenticated peer sending GETMNLISTDIFF for many historical blocks through net_processing/BuildSimplifiedMNListDiff. A lightweight functional-test follow-up that drives the fix through the real P2P request path would confirm the bound holds end-to-end, not just at the manager API surface. Not necessary for this PR to merge.
In `src/evo/deterministicmns.h`:
- [SUGGESTION] src/evo/deterministicmns.h:688-696: MAX_CACHE_LISTS=256 has thinner headroom over legitimate steady-state demand than the comment claims
Mainnet registers exactly five LLMQ types (llmq_50_60, llmq_60_75, llmq_400_60, llmq_400_85, llmq_100_67 — confirmed in chainparams.cpp CMainParams). Summing keepOldConnections+1 retained quorum-base heights per type (26 + 65 + 6 + 6 + 26) gives roughly 129 legitimately-retained quorum-base snapshots, plus the tip snapshot, plus any mini-snapshots (every 32 blocks within the 2880-block recency window, up to ~90 more) generated by ordinary multi-peer historical getmnlistd traffic. Since EnforceListsCacheLimit() evicts purely by oldest-height with no notion of 'this backs a live quorum', legitimate multi-peer load can push the working set toward 220+ entries against a 256 cap, causing avoidable eviction of quorum-base snapshots and repeated disk rebuilds well before any attack threshold is reached. This doesn't reopen the memory-exhaustion bug (the hard cap holds), but the comment's 'sized well above' framing overstates the margin.
In `<commit-stack:76a57be6149,4dc3786461e,9eff80c680c>`:
- [SUGGESTION] <commit-stack:76a57be6149,4dc3786461e,9eff80c680c>:1: Squash the intentionally-failing test commit and the same-day in-stack bugfix before merging to develop
The three-commit stack has two history-hygiene issues, both confirmed against the actual diffs: (1) commit 76a57be6149 adds a regression test that is documented to fail before commit 4dc3786461e lands — checking out this permanent-history midpoint leaves `make check` red by design, which can misdirect a `git bisect`. (2) commit 4dc3786461e introduced a mid-walk `EnforceDiffsCacheLimit()` call plus a disk-read/`assert(false)` fallback inside the rebuild loop; that logic could evict a diff the same walk still needed, and commit 9eff80c680c removes it the same day within the same PR before it ever shipped to develop, per its own commit message ('papered over a failure mode that did not exist before'). Neither issue breaks compilation or leaves develop broken (this never shipped), so it isn't a blocker, but it's a textbook case for `git rebase -i` squashing: fold the test into the fix it validates, and fold the corrective commit into the one it corrects, so `git bisect`/`git blame` never lands on an intermediate commit with a live crash path or a red test suite in the DMN-list rebuild code.
| constexpr size_t n_blocks = CDeterministicMNManager::MAX_CACHE_LISTS + 64; | ||
| for (size_t i = 0; i < n_blocks; ++i) { | ||
| setup.CreateAndProcessBlock({}, coinbase_pk); | ||
| dmnman.UpdatedBlockTip(tip_index()); | ||
| } | ||
|
|
||
| // Record the expected list for a spread of historical heights while they are | ||
| // still cached, so we can prove eviction does not change what is returned. | ||
| const CBlockIndex* tip = tip_index(); | ||
| BOOST_REQUIRE(tip != nullptr); | ||
| std::vector<std::pair<const CBlockIndex*, CDeterministicMNList>> expected; | ||
| for (int h = tip->nHeight; h >= 0 && h > tip->nHeight - static_cast<int>(n_blocks); h -= 37) { | ||
| const CBlockIndex* pindex = tip->GetAncestor(h); | ||
| BOOST_REQUIRE(pindex != nullptr); | ||
| expected.emplace_back(pindex, dmnman.GetListForBlock(pindex)); | ||
| } | ||
| BOOST_REQUIRE(expected.size() > 1); | ||
|
|
||
| // Now exercise GetListForBlock over every distinct historical height — the | ||
| // getmnlistd / BuildSimplifiedMNListDiff path an unauthenticated peer drives. | ||
| for (int h = tip->nHeight; h >= 0 && h > tip->nHeight - static_cast<int>(n_blocks); --h) { | ||
| const CBlockIndex* pindex = tip->GetAncestor(h); | ||
| BOOST_REQUIRE(pindex != nullptr); | ||
| (void)dmnman.GetListForBlock(pindex); | ||
| } | ||
|
|
||
| // Pre-fix: each ProcessBlock / historical load appends freely → size > MAX. | ||
| // Post-fix: insert-time retention + hard cap keep the cache bounded. | ||
| const size_t list_cache_size = dmnman.GetListCacheSize(); | ||
| BOOST_TEST_MESSAGE("mnListsCache size after sweep: " << list_cache_size); | ||
| BOOST_CHECK_MESSAGE(list_cache_size <= CDeterministicMNManager::MAX_CACHE_LISTS, | ||
| strprintf("mnListsCache size %zu exceeds hard cap %zu", list_cache_size, | ||
| CDeterministicMNManager::MAX_CACHE_LISTS)); | ||
| BOOST_CHECK_LE(dmnman.GetListDiffsCacheSize(), CDeterministicMNManager::MAX_CACHE_DIFFS); | ||
|
|
||
| // The cache is pure memoisation: bounding it must not change any result. Some | ||
| // of these entries have certainly been evicted by now, so they are recomputed | ||
| // from disk here — the values must still match what the cache returned above. | ||
| for (const auto& [pindex, want] : expected) { | ||
| const auto got = dmnman.GetListForBlock(pindex); | ||
| BOOST_CHECK_MESSAGE(got == want, | ||
| strprintf("GetListForBlock(%d) differs after cache eviction", pindex->nHeight)); | ||
| } | ||
|
|
||
| // Cleanup must still drop everything outside the recency window, and must not | ||
| // resurrect unbounded growth. | ||
| dmnman.DoMaintenance(); | ||
| BOOST_CHECK_LE(dmnman.GetListCacheSize(), CDeterministicMNManager::MAX_CACHE_LISTS); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Regression test doesn't cross the diff cap/recency boundary and may never force eviction of the values it checks
n_blocks is MAX_CACHE_LISTS+64 = 320, but LIST_DIFFS_CACHE_SIZE is 2880 and MAX_CACHE_DIFFS is 2944, and ShouldRetainCacheHeight()'s recency window is also LIST_DIFFS_CACHE_SIZE blocks. Every height touched by this test sits well inside that window and far below the diff cap, so an implementation that dropped ShouldRetainCacheHeight() or EnforceDiffsCacheLimit() entirely would still pass. Separately, the 'expected' values are recorded via GetListForBlock() calls that themselves re-populate the cache, and the final descending sweep over every height in [tip-319, tip] revisits those exact heights again before the closing comparison loop runs — GetListForBlockInternal returns straight from cache on a hit with no eviction risk, so there's no guarantee any specific 'expected' entry was ever actually evicted-and-rebuilt by the time it's re-checked; the assertion can pass purely on cache hits. Strengthen this by (1) mining more than MAX_CACHE_DIFFS distinct blocks after recording 'expected' to force both the diff cap and a height outside the recency window, and (2) explicitly forcing eviction of a specific known-resident entry (e.g. mine MAX_CACHE_LISTS additional blocks and advance the tip) before re-querying it, ideally with a non-trivial (non-empty) MN state diff so the equality check is substantive rather than comparing empty lists.
source: ['codex', 'coderabbit']
There was a problem hiding this comment.
Resolved in this update — Regression test doesn't cross the diff cap/recency boundary and may never force eviction of the values it checks no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| // runs once a new block has arrived, so between blocks an unauthenticated peer | ||
| // spamming getmnlistd for historical blocks could append entries without bound | ||
| // (a full mainnet list is several MB). Admission is bounded two ways: entries | ||
| // older than the window CleanupCache would drop anyway are not retained at all, | ||
| // and these caps evict the oldest-height entry when exceeded. | ||
| // MAX_CACHE_LISTS is sized well above honest steady-state usage (tip + live | ||
| // quorum bases + mini-snapshots within LIST_DIFFS_CACHE_SIZE of the tip). | ||
| static constexpr size_t MAX_CACHE_LISTS = 256; | ||
| // Diffs are small; allow a full recency window plus a margin for one rebuild walk. |
There was a problem hiding this comment.
🟡 Suggestion: MAX_CACHE_LISTS=256 has thinner headroom over legitimate steady-state demand than the comment claims
Mainnet registers exactly five LLMQ types (llmq_50_60, llmq_60_75, llmq_400_60, llmq_400_85, llmq_100_67 — confirmed in chainparams.cpp CMainParams). Summing keepOldConnections+1 retained quorum-base heights per type (26 + 65 + 6 + 6 + 26) gives roughly 129 legitimately-retained quorum-base snapshots, plus the tip snapshot, plus any mini-snapshots (every 32 blocks within the 2880-block recency window, up to ~90 more) generated by ordinary multi-peer historical getmnlistd traffic. Since EnforceListsCacheLimit() evicts purely by oldest-height with no notion of 'this backs a live quorum', legitimate multi-peer load can push the working set toward 220+ entries against a 256 cap, causing avoidable eviction of quorum-base snapshots and repeated disk rebuilds well before any attack threshold is reached. This doesn't reopen the memory-exhaustion bug (the hard cap holds), but the comment's 'sized well above' framing overstates the margin.
source: ['claude']
There was a problem hiding this comment.
Resolved in this update — MAX_CACHE_LISTS=256 has thinner headroom over legitimate steady-state demand than the comment claims no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| // the apply loop below can resolve every walked hash via mnListDiffsCache. The | ||
| // hard bound is enforced once after the apply loop, so eviction can never drop | ||
| // a diff this walk still needs. | ||
| mnListDiffsCache.emplace(pindex->GetBlockHash(), std::move(diff)); | ||
| listDiffIndexes.emplace_front(pindex); |
There was a problem hiding this comment.
💬 Nitpick: PR description overstates that all admission is funneled through the new helpers
The PR description states admission is funneled through CacheMNList()/CacheMNListDiff() 'so no call site can bypass the bound.' That's true for steady-state code, but during the RecalculateAndRepairDiffs rebuild walk, mnListDiffsCache.emplace(pindex->GetBlockHash(), std::move(diff)) at line 877 inserts directly, bypassing ShouldRetainCacheHeight() entirely — intentionally, so the walk can resolve every hash it reads, with EnforceDiffsCacheLimit() called once after the walk completes to restore the bound. This is correct (verified under the single cs lock scope), but the invariant is 'bounded once the lock is released,' not 'every individual insert goes through the gated helper' as the description implies. Worth a one-line correction so future readers don't assume CacheMNListDiff() is the only insertion path.
source: ['claude']
There was a problem hiding this comment.
Resolved in this update — PR description overstates that all admission is funneled through the new helpers no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| // V044/V049: unauthenticated getmnlistd can force arbitrary historical MN lists | ||
| // into mnListsCache. Between CleanupCache runs the map was append-only, so N | ||
| // distinct heights produced N retained full lists. Bound retention at insert. | ||
| BOOST_AUTO_TEST_CASE(mn_lists_cache_bounded) | ||
| { | ||
| TestChainDIP3Setup setup; | ||
| auto& dmnman = *Assert(setup.m_node.dmnman); | ||
| auto& chainman = *Assert(setup.m_node.chainman.get()); | ||
| const CScript coinbase_pk = GetScriptForRawPubKey(setup.coinbaseKey.GetPubKey()); | ||
| auto tip_index = [&] { return WITH_LOCK(::cs_main, return chainman.ActiveChain().Tip()); }; | ||
|
|
||
| dmnman.UpdatedBlockTip(tip_index()); | ||
| dmnman.DoMaintenance(); | ||
|
|
||
| // Mine more than the hard cap without running cleanup — mirrors the | ||
| // attacker window between blocks when getmnlistd populates the cache. | ||
| constexpr size_t n_blocks = CDeterministicMNManager::MAX_CACHE_LISTS + 64; | ||
| for (size_t i = 0; i < n_blocks; ++i) { | ||
| setup.CreateAndProcessBlock({}, coinbase_pk); | ||
| dmnman.UpdatedBlockTip(tip_index()); | ||
| } | ||
|
|
||
| // Record the expected list for a spread of historical heights while they are | ||
| // still cached, so we can prove eviction does not change what is returned. | ||
| const CBlockIndex* tip = tip_index(); | ||
| BOOST_REQUIRE(tip != nullptr); | ||
| std::vector<std::pair<const CBlockIndex*, CDeterministicMNList>> expected; | ||
| for (int h = tip->nHeight; h >= 0 && h > tip->nHeight - static_cast<int>(n_blocks); h -= 37) { | ||
| const CBlockIndex* pindex = tip->GetAncestor(h); | ||
| BOOST_REQUIRE(pindex != nullptr); | ||
| expected.emplace_back(pindex, dmnman.GetListForBlock(pindex)); | ||
| } | ||
| BOOST_REQUIRE(expected.size() > 1); | ||
|
|
||
| // Now exercise GetListForBlock over every distinct historical height — the | ||
| // getmnlistd / BuildSimplifiedMNListDiff path an unauthenticated peer drives. | ||
| for (int h = tip->nHeight; h >= 0 && h > tip->nHeight - static_cast<int>(n_blocks); --h) { | ||
| const CBlockIndex* pindex = tip->GetAncestor(h); | ||
| BOOST_REQUIRE(pindex != nullptr); | ||
| (void)dmnman.GetListForBlock(pindex); | ||
| } | ||
|
|
||
| // Pre-fix: each ProcessBlock / historical load appends freely → size > MAX. | ||
| // Post-fix: insert-time retention + hard cap keep the cache bounded. | ||
| const size_t list_cache_size = dmnman.GetListCacheSize(); | ||
| BOOST_TEST_MESSAGE("mnListsCache size after sweep: " << list_cache_size); | ||
| BOOST_CHECK_MESSAGE(list_cache_size <= CDeterministicMNManager::MAX_CACHE_LISTS, | ||
| strprintf("mnListsCache size %zu exceeds hard cap %zu", list_cache_size, | ||
| CDeterministicMNManager::MAX_CACHE_LISTS)); | ||
| BOOST_CHECK_LE(dmnman.GetListDiffsCacheSize(), CDeterministicMNManager::MAX_CACHE_DIFFS); | ||
|
|
||
| // The cache is pure memoisation: bounding it must not change any result. Some | ||
| // of these entries have certainly been evicted by now, so they are recomputed | ||
| // from disk here — the values must still match what the cache returned above. | ||
| for (const auto& [pindex, want] : expected) { | ||
| const auto got = dmnman.GetListForBlock(pindex); | ||
| BOOST_CHECK_MESSAGE(got == want, | ||
| strprintf("GetListForBlock(%d) differs after cache eviction", pindex->nHeight)); | ||
| } | ||
|
|
||
| // Cleanup must still drop everything outside the recency window, and must not | ||
| // resurrect unbounded growth. | ||
| dmnman.DoMaintenance(); | ||
| BOOST_CHECK_LE(dmnman.GetListCacheSize(), CDeterministicMNManager::MAX_CACHE_LISTS); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Test exercises the manager API directly, not the actual getmnlistd P2P request path
mn_lists_cache_bounded correctly drives CDeterministicMNManager::GetListForBlock directly, but the PR's stated threat model is an unauthenticated peer sending GETMNLISTDIFF for many historical blocks through net_processing/BuildSimplifiedMNListDiff. A lightweight functional-test follow-up that drives the fix through the real P2P request path would confirm the bound holds end-to-end, not just at the manager API surface. Not necessary for this PR to merge.
source: ['claude']
There was a problem hiding this comment.
Resolved in this update — Test exercises the manager API directly, not the actual getmnlistd P2P request path no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
![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: bf91603fec
ℹ️ 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".
| while (mnListDiffsCache.size() > MAX_CACHE_DIFFS) { | ||
| auto victim = mnListDiffsCache.end(); | ||
| for (auto it = mnListDiffsCache.begin(); it != mnListDiffsCache.end(); ++it) { | ||
| if (victim == mnListDiffsCache.end() || it->second.nHeight < victim->second.nHeight) { |
There was a problem hiding this comment.
When an unauthenticated GETMNLISTDIFF requests a historical block from an interval absent from the cache, the rebuild can insert up to 575 diffs before reaching its disk snapshot. This loop then removes every excess entry by rescanning the roughly MAX_CACHE_DIFFS-sized map, making each request Θ(excess × cache-size); because the inspected handler in net_processing.cpp performs the entire operation while holding cs_main, repeated requests can stall block processing with millions of comparisons in addition to the existing rebuild work. Select the victims in one traversal or avoid admitting non-retained walk entries rather than rescanning for each eviction.
AGENTS.md reference: AGENTS.md:L159-L164
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
The only change since the last Sonnet checkpoint is a one-line comment edit (bf91603) that strips internal audit-finding labels; the cache-bounding fix itself (funneling admission through CacheMNList/CacheMNListDiff with recency filtering and hard caps, plus the earlier correction removing the dangerous mid-walk eviction) is unchanged and verified sound against the code at head. Two real, non-blocking issues persist: the regression test never crosses the 2,880-block recency window or approaches the 2,944-entry diff cap, so two of the three admission-bounding mechanisms it claims to prove are untested, and the post-walk EnforceDiffsCacheLimit()/EnforceListsCacheLimit() eviction loops rescan the whole map per victim (up to ~575 evictions after a max-length rebuild walk) while cs_main is held on the unauthenticated GETMNLISTDIFF path — bounded, but an avoidable CPU multiplier worth fixing with a single-pass selection. The previously flagged cap-headroom concern is resolved as OUTDATED after a first-principles recount of overlapping LLMQ schedules and the recency window (union of live quorum-base heights is roughly 35, not 129+), and the four-commit stack still contains an intentionally-red test commit, a same-PR fix-of-a-fix, and a trailing one-line docs-only fixup that should be squashed before merge.
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 (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (completed)
🟡 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 `src/evo/deterministicmns.cpp`:
- [SUGGESTION] src/evo/deterministicmns.cpp:784-798: Post-walk diff-cache eviction rescans the whole map per victim while cs_main is held
`EnforceDiffsCacheLimit()` (and `EnforceListsCacheLimit()`, same pattern) evicts one entry per full linear scan of the map, looping until under cap: `while (size() > CAP) { scan all; erase lowest; }`. Verified call chain: `net_processing.cpp`'s `GETMNLISTDIFF` handler takes `LOCK(cs_main)` for the whole handler and calls `BuildSimplifiedMNListDiff()` -> `GetListForBlockInternal()`, which admits every diff unconditionally during its rebuild walk (bypassing the recency filter by design) and then calls `EnforceDiffsCacheLimit()` exactly once after the walk completes (line 924). A single request that reconstructs from the oldest allowed diff can walk up to `DISK_SNAPSHOT_PERIOD - 1` = 575 diffs; if the cache is already near `MAX_CACHE_DIFFS` (2,944) before the walk, the post-walk enforcement needs up to ~575 evictions, each rescanning a map of several thousand entries — on the order of 1.6-1.9M map-entry visits, all under the node's most contended lock. This does not reopen the unbounded-growth issue the PR fixes (size is still hard-capped per request), but it's an avoidable quadratic-ish cost on an unauthenticated, unrate-limited P2P message. Collect all excess victims in one traversal (as `CleanupCache()` already does) or use `std::nth_element`/a small min-heap keyed by height, then erase as a batch.
In `src/test/evo_deterministicmns_tests.cpp`:
- [SUGGESTION] src/test/evo_deterministicmns_tests.cpp:1686-1746: Regression test never crosses the recency window and likely never forces the diffs hard cap
`n_blocks = MAX_CACHE_LISTS + 64 = 320`, but `LIST_DIFFS_CACHE_SIZE` (the recency window used by `ShouldRetainCacheHeight()`) is `DISK_SNAPSHOT_PERIOD * DISK_SNAPSHOTS = 576 * 5 = 2880` (computed from `llmq_max_blocks()` over the full `available_llmqs` table, max is `llmq_400_85` at `4 * 576 = 2304` blocks -> `DISK_SNAPSHOTS = 2304/576+1 = 5`). Since 320 << 2880, every height the test touches passes `ShouldRetainCacheHeight()`, so the recency-rejection branch of `CacheMNList`/`CacheMNListDiff` is never exercised. Likewise `MAX_CACHE_DIFFS = 2880 + 64 = 2944` is far above the handful of diffs this test can ever produce, so `EnforceDiffsCacheLimit()`'s trimming loop is essentially a no-op here and `BOOST_CHECK_LE(dmnman.GetListDiffsCacheSize(), MAX_CACHE_DIFFS)` passes trivially. Only the `mnListsCache` hard cap (256) is actually forced and verified — two of the three admission-bounding mechanisms the test's docstring implies it proves (recency filter, diffs hard cap) aren't put under real pressure. Add a case that pushes `n_blocks` past 2880+64 (or directly drives many distinct diff heights) to exercise both boundaries.
In `<commit-stack:76a57be6149..bf91603fecb>`:
- [SUGGESTION] <commit-stack:76a57be6149..bf91603fecb>:1: Squash the intentionally-red test, its same-PR fix-of-a-fix, and the trailing docs-only fixup
The four-commit stack still has three seams that shouldn't reach develop's permanent history: (1) `76a57be6149` adds a test that is deliberately red until the very next commit — a bisect landing here sees a known failure that was never independently shipped; (2) `9eff80c680c` exists purely to remove a bug (mid-walk diff eviction + bare `assert(false)` fallback) that `4dc3786461e` introduced one commit earlier and that never shipped on its own — `git blame`/bisect on the final admission logic will point at a commit whose approach was already superseded before merge; (3) `bf91603fecb` is a one-line comment fixup removing 'V044/V049' labels that were only ever introduced by `76a57be6149` earlier in this same unmerged stack (and the same labels still linger in the `76a57be6149`/`4dc3786461e` commit *messages*, which this cleanup doesn't touch). None of this harms correctness or blocks merge, but squashing into two clean commits (test, then fix) — and rewording the commit messages to drop the remaining internal labels — would give develop a bisectable, self-consistent history instead of a patch-of-a-patch-of-a-typo chain.
| void CDeterministicMNManager::EnforceDiffsCacheLimit() | ||
| { | ||
| AssertLockHeld(cs); | ||
| while (mnListDiffsCache.size() > MAX_CACHE_DIFFS) { | ||
| auto victim = mnListDiffsCache.end(); | ||
| for (auto it = mnListDiffsCache.begin(); it != mnListDiffsCache.end(); ++it) { | ||
| if (victim == mnListDiffsCache.end() || it->second.nHeight < victim->second.nHeight) { | ||
| victim = it; | ||
| } | ||
| } | ||
| if (victim == mnListDiffsCache.end()) { | ||
| break; | ||
| } | ||
| mnListDiffsCache.erase(victim); | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Post-walk diff-cache eviction rescans the whole map per victim while cs_main is held
EnforceDiffsCacheLimit() (and EnforceListsCacheLimit(), same pattern) evicts one entry per full linear scan of the map, looping until under cap: while (size() > CAP) { scan all; erase lowest; }. Verified call chain: net_processing.cpp's GETMNLISTDIFF handler takes LOCK(cs_main) for the whole handler and calls BuildSimplifiedMNListDiff() -> GetListForBlockInternal(), which admits every diff unconditionally during its rebuild walk (bypassing the recency filter by design) and then calls EnforceDiffsCacheLimit() exactly once after the walk completes (line 924). A single request that reconstructs from the oldest allowed diff can walk up to DISK_SNAPSHOT_PERIOD - 1 = 575 diffs; if the cache is already near MAX_CACHE_DIFFS (2,944) before the walk, the post-walk enforcement needs up to ~575 evictions, each rescanning a map of several thousand entries — on the order of 1.6-1.9M map-entry visits, all under the node's most contended lock. This does not reopen the unbounded-growth issue the PR fixes (size is still hard-capped per request), but it's an avoidable quadratic-ish cost on an unauthenticated, unrate-limited P2P message. Collect all excess victims in one traversal (as CleanupCache() already does) or use std::nth_element/a small min-heap keyed by height, then erase as a batch.
source: ['claude', 'codex']
| BOOST_AUTO_TEST_CASE(mn_lists_cache_bounded) | ||
| { | ||
| TestChainDIP3Setup setup; | ||
| auto& dmnman = *Assert(setup.m_node.dmnman); | ||
| auto& chainman = *Assert(setup.m_node.chainman.get()); | ||
| const CScript coinbase_pk = GetScriptForRawPubKey(setup.coinbaseKey.GetPubKey()); | ||
| auto tip_index = [&] { return WITH_LOCK(::cs_main, return chainman.ActiveChain().Tip()); }; | ||
|
|
||
| dmnman.UpdatedBlockTip(tip_index()); | ||
| dmnman.DoMaintenance(); | ||
|
|
||
| // Mine more than the hard cap without running cleanup — mirrors the | ||
| // attacker window between blocks when getmnlistd populates the cache. | ||
| constexpr size_t n_blocks = CDeterministicMNManager::MAX_CACHE_LISTS + 64; | ||
| for (size_t i = 0; i < n_blocks; ++i) { | ||
| setup.CreateAndProcessBlock({}, coinbase_pk); | ||
| dmnman.UpdatedBlockTip(tip_index()); | ||
| } | ||
|
|
||
| // Record the expected list for a spread of historical heights while they are | ||
| // still cached, so we can prove eviction does not change what is returned. | ||
| const CBlockIndex* tip = tip_index(); | ||
| BOOST_REQUIRE(tip != nullptr); | ||
| std::vector<std::pair<const CBlockIndex*, CDeterministicMNList>> expected; | ||
| for (int h = tip->nHeight; h >= 0 && h > tip->nHeight - static_cast<int>(n_blocks); h -= 37) { | ||
| const CBlockIndex* pindex = tip->GetAncestor(h); | ||
| BOOST_REQUIRE(pindex != nullptr); | ||
| expected.emplace_back(pindex, dmnman.GetListForBlock(pindex)); | ||
| } | ||
| BOOST_REQUIRE(expected.size() > 1); | ||
|
|
||
| // Now exercise GetListForBlock over every distinct historical height — the | ||
| // getmnlistd / BuildSimplifiedMNListDiff path an unauthenticated peer drives. | ||
| for (int h = tip->nHeight; h >= 0 && h > tip->nHeight - static_cast<int>(n_blocks); --h) { | ||
| const CBlockIndex* pindex = tip->GetAncestor(h); | ||
| BOOST_REQUIRE(pindex != nullptr); | ||
| (void)dmnman.GetListForBlock(pindex); | ||
| } | ||
|
|
||
| // Pre-fix: each ProcessBlock / historical load appends freely → size > MAX. | ||
| // Post-fix: insert-time retention + hard cap keep the cache bounded. | ||
| const size_t list_cache_size = dmnman.GetListCacheSize(); | ||
| BOOST_TEST_MESSAGE("mnListsCache size after sweep: " << list_cache_size); | ||
| BOOST_CHECK_MESSAGE(list_cache_size <= CDeterministicMNManager::MAX_CACHE_LISTS, | ||
| strprintf("mnListsCache size %zu exceeds hard cap %zu", list_cache_size, | ||
| CDeterministicMNManager::MAX_CACHE_LISTS)); | ||
| BOOST_CHECK_LE(dmnman.GetListDiffsCacheSize(), CDeterministicMNManager::MAX_CACHE_DIFFS); | ||
|
|
||
| // The cache is pure memoisation: bounding it must not change any result. Some | ||
| // of these entries have certainly been evicted by now, so they are recomputed | ||
| // from disk here — the values must still match what the cache returned above. | ||
| for (const auto& [pindex, want] : expected) { | ||
| const auto got = dmnman.GetListForBlock(pindex); | ||
| BOOST_CHECK_MESSAGE(got == want, | ||
| strprintf("GetListForBlock(%d) differs after cache eviction", pindex->nHeight)); | ||
| } | ||
|
|
||
| // Cleanup must still drop everything outside the recency window, and must not | ||
| // resurrect unbounded growth. | ||
| dmnman.DoMaintenance(); | ||
| BOOST_CHECK_LE(dmnman.GetListCacheSize(), CDeterministicMNManager::MAX_CACHE_LISTS); |
There was a problem hiding this comment.
🟡 Suggestion: Regression test never crosses the recency window and likely never forces the diffs hard cap
n_blocks = MAX_CACHE_LISTS + 64 = 320, but LIST_DIFFS_CACHE_SIZE (the recency window used by ShouldRetainCacheHeight()) is DISK_SNAPSHOT_PERIOD * DISK_SNAPSHOTS = 576 * 5 = 2880 (computed from llmq_max_blocks() over the full available_llmqs table, max is llmq_400_85 at 4 * 576 = 2304 blocks -> DISK_SNAPSHOTS = 2304/576+1 = 5). Since 320 << 2880, every height the test touches passes ShouldRetainCacheHeight(), so the recency-rejection branch of CacheMNList/CacheMNListDiff is never exercised. Likewise MAX_CACHE_DIFFS = 2880 + 64 = 2944 is far above the handful of diffs this test can ever produce, so EnforceDiffsCacheLimit()'s trimming loop is essentially a no-op here and BOOST_CHECK_LE(dmnman.GetListDiffsCacheSize(), MAX_CACHE_DIFFS) passes trivially. Only the mnListsCache hard cap (256) is actually forced and verified — two of the three admission-bounding mechanisms the test's docstring implies it proves (recency filter, diffs hard cap) aren't put under real pressure. Add a case that pushes n_blocks past 2880+64 (or directly drives many distinct diff heights) to exercise both boundaries.
source: ['claude', 'codex']
bf91603 to
f2f1e9f
Compare
knst
left a comment
knst
left a comment
There was a problem hiding this comment.
most likely performance degradation. See comment
| // and these caps evict the oldest-height entry when exceeded. | ||
| // MAX_CACHE_LISTS is sized well above honest steady-state usage (tip + live | ||
| // quorum bases + mini-snapshots within LIST_DIFFS_CACHE_SIZE of the tip). | ||
| static constexpr size_t MAX_CACHE_LISTS = 256; |
There was a problem hiding this comment.
probably wrong and will affect performance significantly.
It should be at least DISK_SNAPSHOT_PERIOD * 2, because diffs are calculated from snapshot.
Testing for this PR should involve performance test on release build for validating / invalidating blocks from real chain close to tip (at least after DIP3 activation)
This cache is used not only for RPC calls but for calculating diff between blocks for block invalidation.
There was a problem hiding this comment.
Re: cap sizing and perf test — updated in latest push (pending commit):
Cap: MAX_CACHE_LISTS = DISK_SNAPSHOT_PERIOD * 2 (1152), addressing the snapshot-boundary concern.
Stale CPU regression: the recency filter that categorically dropped stale mini-snapshots is replaced with a bounded LRU stale tier (MAX_STALE_CACHE_LISTS = 32). Repeat stale getmnlistd requests reuse cached mini-snapshots instead of re-reading multi-MB disk snapshots on every call under cs_main.
Release-build mainnet benchmarks (cloned datadir via ditto --clone, scripts in contrib/devtools/):
| Build | protx diff 20× (stale) | invalidate tip−600 | reconsider | invalidate tip−1200 | reconsider |
|---|---|---|---|---|---|
| develop (baseline) | 1.18s | 1.68s | 4.74s | 2.33s | 8.13s |
| sec/v044 + two-tier | (pending push — startup crash on clone during local run; will re-run after push) |
Develop numbers establish the pre-fix baseline. PR-branch binaries hit an init abort on the cloned datadir during local benchmarking (unrelated to cache changes — occurs before RPC is up); I will re-run the full three-way table once the amended commit lands.
Follow-up tracked for per-peer rate limiting: #7564.
|
This pull request has conflicts, please rebase. |
CDeterministicMNManager's in-memory caches (mnListsCache, mnListDiffsCache) are only trimmed by CleanupCache(), which runs when a new block arrives. Between blocks there is no bound: GETMNLISTDIFF accepts an arbitrary historical baseBlockHash and GetListForBlock appends a cache entry per requested block, so an unauthenticated peer requesting many distinct historical blocks drives cache growth without ceiling (a full mainnet MN list is several MB). Bound admission through new CacheMNList()/CacheMNListDiff() helpers: entries older than the recency window CleanupCache would drop anyway (height + LIST_DIFFS_CACHE_SIZE < tip) are not retained at all, and hard caps evict the lowest-height entries in a single pass (std::nth_element), never the tip snapshot. MAX_CACHE_LISTS = DISK_SNAPSHOT_PERIOD * 2: lists are rebuilt by applying up to DISK_SNAPSHOT_PERIOD - 1 diffs from the previous on-disk snapshot, so validation/invalidation spanning a snapshot boundary can keep two snapshot periods of lists resident without eviction thrash. MAX_CACHE_DIFFS = LIST_DIFFS_CACHE_SIZE + 64. The rebuild walk in GetListForBlockInternal() admits every diff it reads unconditionally so the apply loop can resolve every walked hash; the diff cap is enforced once after the walk completes, so eviction can never drop a diff the walk still needs. Add mn_lists_cache_bounded regression test: drive GetListForBlock over more distinct historical heights than the cap without running cleanup, assert both caches stay bounded, and prove eviction never changes a returned list by re-querying entries guaranteed to have been evicted.
f2f1e9f to
416294a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Rebased on latest 🤖 Posted autonomously by Claude on behalf of pasta. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 416294a78e
ℹ️ 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".
| if (!ShouldRetainCacheHeight(list.GetHeight())) { | ||
| return; |
There was a problem hiding this comment.
When an unauthenticated GETMNLISTDIFF repeatedly names a base older than LIST_DIFFS_CACHE_SIZE, this return rejects every mini-snapshot produced during reconstruction. Consequently, each identical request reapplies up to 575 diffs from the preceding disk snapshot—and rereads them once the diff cache is saturated—while the inspected handler in src/net_processing.cpp:5458-5482 holds cs_main; previously, the first request populated 32-block mini-snapshots and reduced repeats to at most about 31 applications. Keep a bounded stale working set (for example with recency/LRU eviction) rather than making old intervals permanent cache misses.
AGENTS.md reference: AGENTS.md:L162-L169
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The hard caps stop unbounded cache growth, but rejecting all stale list snapshots makes repeated historical requests rebuild up to 575 diffs while the P2P handler holds cs_main, leaving a blocking CPU-denial-of-service regression. The regression test confirms the list-cache cap but does not exercise the stale-height admission path or the diff-cache hard cap. Source: reviewer backends gpt-5.6-sol (general) and gpt-5.6-sol (dash-core-commit-history); final verifier backend 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 | 🟡 1 suggestion(s)
🤖 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/evo/deterministicmns.cpp`:
- [BLOCKING] src/evo/deterministicmns.cpp:769-775: Stale-height rejection makes repeated historical requests permanent cache misses
For a requested height older than `tipIndex->nHeight - LIST_DIFFS_CACHE_SIZE`, this predicate rejects both the persistent snapshot loaded at lines 871-874 and every 32-block mini-snapshot produced at lines 927-942. An unauthenticated `GETMNLISTDIFF` can repeatedly choose a block immediately before a 576-block snapshot boundary and force `GetListForBlockInternal()` to read and apply up to 575 diffs on every request. On an established node, the diff cache already contains roughly the recent 2,880-block window; after the stale walk, `EnforceDiffsCacheLimit()` evicts the newly loaded lower-height stale diffs first, retaining only the small 64-entry margin and making the next identical request repeat almost the entire walk. Before this change, the first request populated mini-snapshots and subsequent requests in the same interval applied at most about 31 diffs until cleanup. The P2P handler performs this work while holding `cs_main`, so repeated requests can stall validation and network processing. The same policy degrades deep block disconnection because `tipIndex` remains at the old tip until disconnection finishes, preventing stale mini-snapshots from helping the walk. Keep a separately bounded/LRU stale working set instead of categorically rejecting stale snapshots or always evicting them in favor of newer heights.
In `src/test/evo_deterministicmns_tests.cpp`:
- [SUGGESTION] src/test/evo_deterministicmns_tests.cpp:3144-3154: Regression test does not exercise stale admission or the diff-cache cap
The test mines `MAX_CACHE_LISTS + 64`, which is 1,216 blocks, while `LIST_DIFFS_CACHE_SIZE` is 2,880 and `MAX_CACHE_DIFFS` is 2,944. Every requested height therefore remains inside `ShouldRetainCacheHeight()`'s recency window, and the test creates fewer diffs than the hard cap. As the comment acknowledges, the diff-size assertion is trivial: implementations with a broken stale-height predicate or no diff eviction would still pass. Add coverage beyond the 2,880-block recency boundary and the 2,944-entry diff cap, including repeated access to one stale interval, so the test verifies both new mechanisms and catches permanent stale-cache misses.
| bool CDeterministicMNManager::ShouldRetainCacheHeight(int height) | ||
| { | ||
| AssertLockHeld(cs); | ||
| // Before tip is known, retain freely (early startup / first connect). | ||
| if (!tipIndex) return true; | ||
| // Same recency window CleanupCache uses for the "too old" drop predicate. | ||
| return height + LIST_DIFFS_CACHE_SIZE >= tipIndex->nHeight; |
There was a problem hiding this comment.
🔴 Blocking: Stale-height rejection makes repeated historical requests permanent cache misses
For a requested height older than tipIndex->nHeight - LIST_DIFFS_CACHE_SIZE, this predicate rejects both the persistent snapshot loaded at lines 871-874 and every 32-block mini-snapshot produced at lines 927-942. An unauthenticated GETMNLISTDIFF can repeatedly choose a block immediately before a 576-block snapshot boundary and force GetListForBlockInternal() to read and apply up to 575 diffs on every request. On an established node, the diff cache already contains roughly the recent 2,880-block window; after the stale walk, EnforceDiffsCacheLimit() evicts the newly loaded lower-height stale diffs first, retaining only the small 64-entry margin and making the next identical request repeat almost the entire walk. Before this change, the first request populated mini-snapshots and subsequent requests in the same interval applied at most about 31 diffs until cleanup. The P2P handler performs this work while holding cs_main, so repeated requests can stall validation and network processing. The same policy degrades deep block disconnection because tipIndex remains at the old tip until disconnection finishes, preventing stale mini-snapshots from helping the walk. Keep a separately bounded/LRU stale working set instead of categorically rejecting stale snapshots or always evicting them in favor of newer heights.
source: ['codex']
There was a problem hiding this comment.
Resolved in 0340fcd — Stale-height rejection makes repeated historical requests permanent cache misses no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
| // Pre-fix: each ProcessBlock / historical load appends freely → size > MAX. | ||
| // Post-fix: insert-time retention + hard cap keep the cache bounded. | ||
| // (The diffs cache stays far below its cap here — its bound only bites on | ||
| // walks that read stale diffs back from disk — so only the lists cap is | ||
| // driven past its limit by this test.) | ||
| const size_t list_cache_size = dmnman.GetListCacheSize(); | ||
| BOOST_TEST_MESSAGE("mnListsCache size after sweep: " << list_cache_size); | ||
| BOOST_CHECK_MESSAGE(list_cache_size <= CDeterministicMNManager::MAX_CACHE_LISTS, | ||
| strprintf("mnListsCache size %zu exceeds hard cap %zu", list_cache_size, | ||
| CDeterministicMNManager::MAX_CACHE_LISTS)); | ||
| BOOST_CHECK_LE(dmnman.GetListDiffsCacheSize(), CDeterministicMNManager::MAX_CACHE_DIFFS); |
There was a problem hiding this comment.
🟡 Suggestion: Regression test does not exercise stale admission or the diff-cache cap
The test mines MAX_CACHE_LISTS + 64, which is 1,216 blocks, while LIST_DIFFS_CACHE_SIZE is 2,880 and MAX_CACHE_DIFFS is 2,944. Every requested height therefore remains inside ShouldRetainCacheHeight()'s recency window, and the test creates fewer diffs than the hard cap. As the comment acknowledges, the diff-size assertion is trivial: implementations with a broken stale-height predicate or no diff eviction would still pass. Add coverage beyond the 2,880-block recency boundary and the 2,944-entry diff cap, including repeated access to one stale interval, so the test verifies both new mechanisms and catches permanent stale-cache misses.
source: ['codex']
There was a problem hiding this comment.
Resolved in 0340fcd — Regression test does not exercise stale admission or the diff-cache cap no longer present.
Auto-resolved by the review system based on the latest commit diff. If you believe this was closed in error, reopen the thread.
|
Update: two-tier stale LRU cache addresses the blocking review. Recent tier cap 1152; stale tier LRU cap 32. Unit test extended. Develop baseline benchmarks: protx diff 20x 1.18s, invalidate tip-600 1.68s/4.74s, tip-1200 2.33s/8.13s. Follow-up rate limit #7564. Amend commit pending local push. |
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3ffe31548
ℹ️ 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".
| ROOT="/Users/pasta/.cursor/worktrees/dash/vuub" | ||
| SRC_DATADIR="/Users/pasta/Library/Application Support/DashCore" |
There was a problem hiding this comment.
On any checkout other than the author's /Users/pasta/... layout, this runner fails before benchmarking because ROOT, SRC_DATADIR, and the binary paths point to private worktrees; run-benchmark-mnlist-cache-7485-resume.sh repeats the same assumptions. Since the change directs developers to this committed runner for reproducing its benchmark, derive the repository root and accept the datadir and binary locations as arguments instead.
Useful? React with 👍 / 👎.
Mainnet performance (release build, knst's gate)Setup: release binaries (
Repeated historical
|
| base (develop) | PR head | two-tier | |
|---|---|---|---|
| warm recent (5 period ends) | 0.08→0.04 s | 0.08→0.04 s | 0.08→0.04 s |
| stale #1 | 0.04 s | 0.06 s | 0.04 s |
| stale #2…#5 | 0.00 s | 0.05 s each | 0.00 s |
| RSS start → after stale | 1.51 → 1.58 GB | 1.50 → 1.57 GB | 1.50 → 1.58 GB |
PR head pays ~50 ms on every repeat (rebuild from disk). Two-tier matches develop after first warm-up.
Historical period-end sweep (200 requests, stepping 576 blocks back)
| base (develop) | PR head | two-tier | |
|---|---|---|---|
| elapsed | 15 s | 23 s | 13 s |
| RSS start → final | 1.51 → 4.18 GB | 1.50 → 1.63 GB | 1.51 → 1.81 GB |
Memory stays bounded; develop grows without limit under the same traffic.
Invalidate / reconsider near tip
| depth | metric | base | PR head | two-tier |
|---|---|---|---|---|
| 1152 | cold invalidate | 2.72 s | 2.65 s | 2.43 s |
| 1152 | reconnect | 7.86 s | 7.94 s | 7.72 s |
| 1152 | warm invalidate | 1.39 s | 1.42 s | 1.45 s |
| 1152 | final reconnect | 6.80 s | 6.79 s | 6.78 s |
| 2900 | cold invalidate | 5.33 s | 5.55 s | 4.38 s |
| 2900 | reconnect | 18.26 s | 19.01 s | 18.89 s |
| 2900 | warm invalidate | 2.97 s | 3.50 s | 3.24 s |
| 2900 | final reconnect | 18.73 s | 19.36 s† | 18.52 s |
† PR-head deep final reconnect from debug.log (elapsed_time_ms=19362); parent script stalled on txindex resync.
Takeaway
- Memory DoS fixed: historical sweep RSS stays ~1.6–1.8 GB vs develop climbing past 4 GB.
- CPU regression from pure stale rejection fixed: two-tier restores develop-like repeat stale latency (0.00 s after first hit).
- Validate/invalidate near tip: within noise of develop on both 1152 and 2900 depths — no regression for knst's release-build path.
Unit test evo_dip3_activation_tests/mn_lists_cache_bounded also covers caps + stale LRU routing on a synthetic chain.
Follow-up still open for per-peer GETMNLISTDIFF rate limiting: #7564.
🤖 Posted autonomously by Claude on behalf of pasta.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/test/evo_deterministicmns_tests.cpp (1)
3162-3175: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winProve the stale-cache hit.
Lines 3173-3175 only verify result equality and a size bound. A second lookup that rebuilds the list and refreshes its stale entry can pass all these checks.
Add narrow test diagnostics, such as a stale-cache hit counter, and assert that the second lookup is a hit with no rebuild. As per coding guidelines,
src/test/**/*.cpp: “Add small tests proving invariants when changing ... timing/shutdown behavior.”🤖 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/evo_deterministicmns_tests.cpp` around lines 3162 - 3175, Strengthen the stale-list cache test around dmnman.GetListForBlock by exposing or using narrow diagnostics for stale-cache hits and rebuilds, then capture counters before the repeated stale_pindex lookup. Assert the second lookup increments the hit counter without incrementing the rebuild counter, while preserving the existing result and cache-size checks.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.
Inline comments:
In `@contrib/devtools/benchmark-mnlist-cache-7485.sh`:
- Around line 11-26: Update the benchmark script’s RPCPASS initialization to
generate a unique per-run password only when RPCPASS is unset, preserving an
explicitly supplied password. Set umask 077 before the cat block that writes
CONF so the generated configuration file is restricted to its owner.
In `@contrib/devtools/run-benchmark-mnlist-cache-7485.sh`:
- Around line 5-20: Remove hard-coded workstation paths from
contrib/devtools/run-benchmark-mnlist-cache-7485.sh lines 5-20 by deriving ROOT
or accepting it as input, requiring SRC_DATADIR and binary locations via
arguments or environment variables, and making the clone command configurable;
apply the same portable configuration interface to
contrib/devtools/run-benchmark-mnlist-cache-7485-resume.sh lines 5-12, while
preserving the existing benchmark runner behavior.
---
Nitpick comments:
In `@src/test/evo_deterministicmns_tests.cpp`:
- Around line 3162-3175: Strengthen the stale-list cache test around
dmnman.GetListForBlock by exposing or using narrow diagnostics for stale-cache
hits and rebuilds, then capture counters before the repeated stale_pindex
lookup. Assert the second lookup increments the hit counter without incrementing
the rebuild counter, while preserving the existing result and cache-size checks.
🪄 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: 3d00ec84-5634-4c28-9a98-1dc18f00a070
📒 Files selected for processing (7)
contrib/devtools/benchmark-mnlist-cache-7485.shcontrib/devtools/run-benchmark-mnlist-cache-7485-resume.shcontrib/devtools/run-benchmark-mnlist-cache-7485.shdoc/release-notes-7485.mdsrc/evo/deterministicmns.cppsrc/evo/deterministicmns.hsrc/test/evo_deterministicmns_tests.cpp
| RPCUSER="${RPCUSER:-bench7485}" | ||
| RPCPASS="${RPCPASS:-bench7485pass}" | ||
| PIDFILE="${DATADIR}/benchmark-${LABEL}.pid" | ||
| LOGFILE="${DATADIR}/benchmark-${LABEL}.log" | ||
| CONF="${DATADIR}/benchmark-${LABEL}.conf" | ||
|
|
||
| cat >"${CONF}" <<EOF | ||
| server=1 | ||
| rpcuser=${RPCUSER} | ||
| rpcpassword=${RPCPASS} | ||
| rpcport=${RPCPORT} | ||
| listen=0 | ||
| connect=0 | ||
| dnsseed=0 | ||
| discover=0 | ||
| EOF |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Use a unique RPC password and restrict the configuration file.
The committed default password lets another local process authenticate to the benchmark node while it runs. listen=0 does not disable RPC access. The generated configuration file also inherits the caller umask.
Generate a per-run password when RPCPASS is unset. Set umask 077 before writing CONF.
🤖 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 `@contrib/devtools/benchmark-mnlist-cache-7485.sh` around lines 11 - 26, Update
the benchmark script’s RPCPASS initialization to generate a unique per-run
password only when RPCPASS is unset, preserving an explicitly supplied password.
Set umask 077 before the cat block that writes CONF so the generated
configuration file is restricted to its owner.
| ROOT="/Users/pasta/.cursor/worktrees/dash/vuub" | ||
| SRC_DATADIR="/Users/pasta/Library/Application Support/DashCore" | ||
| BENCH_SCRIPT="${ROOT}/contrib/devtools/benchmark-mnlist-cache-7485.sh" | ||
| RESULTS="${ROOT}/contrib/devtools/benchmark-mnlist-cache-7485-results.txt" | ||
|
|
||
| DEVELOP_D="${ROOT}/../benchmark-bins/develop" | ||
| PRHEAD_D="${ROOT}/../benchmark-bins/prhead" | ||
| TWOTIER_D="${ROOT}/../benchmark-bins/twotier" | ||
|
|
||
| mkdir -p "${DEVELOP_D}" "${PRHEAD_D}" "${TWOTIER_D}" | ||
| ln -sf /Users/pasta/workspace/dash-upstream-develop/src/dashd "${DEVELOP_D}/dashd" | ||
| ln -sf /Users/pasta/workspace/dash-upstream-develop/src/dash-cli "${DEVELOP_D}/dash-cli" | ||
| ln -sf /Users/pasta/.t3/worktrees/dash/t3code-fa7e3825/src/dashd "${PRHEAD_D}/dashd" | ||
| ln -sf /Users/pasta/.t3/worktrees/dash/t3code-fa7e3825/src/dash-cli "${PRHEAD_D}/dash-cli" | ||
| ln -sf "${ROOT}/src/dashd" "${TWOTIER_D}/dashd" | ||
| ln -sf "${ROOT}/src/dash-cli" "${TWOTIER_D}/dash-cli" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove workstation-specific benchmark configuration.
These committed scripts only run with the author’s absolute paths. The full runner also requires the macOS-only ditto command.
contrib/devtools/run-benchmark-mnlist-cache-7485.sh#L5-L20: derive the repository root or accept it as an input. Require the source datadir and binary locations through arguments or environment variables. Make the clone command configurable.contrib/devtools/run-benchmark-mnlist-cache-7485-resume.sh#L5-L12: use the same portable configuration interface as the full runner.
📍 Affects 2 files
contrib/devtools/run-benchmark-mnlist-cache-7485.sh#L5-L20(this comment)contrib/devtools/run-benchmark-mnlist-cache-7485-resume.sh#L5-L12
🤖 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 `@contrib/devtools/run-benchmark-mnlist-cache-7485.sh` around lines 5 - 20,
Remove hard-coded workstation paths from
contrib/devtools/run-benchmark-mnlist-cache-7485.sh lines 5-20 by deriving ROOT
or accepting it as input, requiring SRC_DATADIR and binary locations via
arguments or environment variables, and making the clone command configurable;
apply the same portable configuration interface to
contrib/devtools/run-benchmark-mnlist-cache-7485-resume.sh lines 5-12, while
preserving the existing benchmark runner behavior.
b3ffe31 to
aa93a4c
Compare
Keep a hard cap on recent masternode lists and diffs so unauthenticated historical GETMNLISTDIFF traffic cannot grow memory without bound between blocks. Add a small LRU tier for stale mini-snapshots so repeated requests for heights older than the recency window stay cheap after first warm-up, instead of re-walking up to ~575 diffs under cs_main on every call. Extend the unit test past the recency and diff-cap windows, and document the change in release notes.
aa93a4c to
0340fcd
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0340fcd12a
ℹ️ 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".
| if (auto stale = GetStaleList(pindex->GetBlockHash())) { | ||
| snapshot = std::move(*stale); | ||
| break; |
There was a problem hiding this comment.
When evodb_repair repairs an older interval after one of its lists has entered the new stale cache, this lookup continues returning the pre-repair list: WriteRepairedDiffs() erases repaired hashes only from mnListDiffsCache and mnListsCache, not mnStaleListsCache. Consequently, subsequent GetListForBlock()/getmnlistdiff calls can still use corrupted state until that stale entry happens to be evicted or the node restarts; erase the corresponding stale entries as part of repair invalidation.
AGENTS.md reference: AGENTS.md:L164-L169
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The two-tier cache design now bounds memory while preserving efficient repeated access to stale historical lists, and the expanded regression test crosses both the recency window and diff-cache cap. One blocking correctness issue remains: EvoDB repair invalidates the existing list and diff caches but leaves repaired historical lists in the new stale tier, allowing pre-repair state to shadow corrected disk data.
Source: reviewers gpt-5.6-sol (general) and gpt-5.6-sol (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 | 🟡 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 `src/evo/deterministicmns.cpp`:
- [BLOCKING] src/evo/deterministicmns.cpp:1602-1608: Invalidate stale cached lists when repairing EvoDB diffs
`WriteRepairedDiffs()` invalidates repaired hashes in `mnListDiffsCache` and `mnListsCache`, but not in the newly introduced `mnStaleListsCache`. `RepairSnapshotPair()` recalculates every block between the affected snapshots, so a historical list built from one of the old diffs can already be resident under the same repaired block hash. After repair, `GetListForBlockInternal()` consults `GetStaleList()` before reading corrected data from EvoDB and can therefore continue returning the pre-repair list until LRU eviction or restart. Erase each repaired hash from the stale tier alongside the other two caches.
In `src/test/evo_deterministicmns_tests.cpp`:
- [SUGGESTION] src/test/evo_deterministicmns_tests.cpp:3162-3175: Repeated-access check does not prove the stale cache was hit
The result-equality and cache-size assertions cannot distinguish a stale-cache hit from another complete reconstruction. An implementation that still populates `mnStaleListsCache` but stops consulting it would produce the same lists and preserve the same size bound, so this test would pass while reintroducing the repeated historical rebuild that the stale tier is meant to prevent under `cs_main`. Add a narrow test diagnostic for stale hits or rebuilds and assert that the repeated lookup reaches a resident stale mini-snapshot without performing another full walk.
In `<commit:0340fcd12a6>`:
- [SUGGESTION] <commit:0340fcd12a6>:1: Squash the stale-cache correction into the original cache-bound fix
Commit `416294a78e6` introduces the cache bounds by rejecting stale snapshots, while the immediately following commit `0340fcd12a6` corrects that new policy with the bounded stale LRU and replaces the original test parameters to cover the final design. Both commits have the identical subject and implement one logical memory-exhaustion fix. Squash them so permanent history does not contain an intermediate implementation already known within this PR to cause repeated stale rebuilds, and so `git log` does not present two indistinguishable commits for the same change.
| // Repeat access to one stale interval must be cache-served, not a full re-walk. | ||
| const int stale_target_height = tip->nHeight - static_cast<int>(recency_window) - 100; | ||
| BOOST_REQUIRE(stale_target_height >= 0); | ||
| const CBlockIndex* stale_pindex = tip->GetAncestor(stale_target_height); | ||
| BOOST_REQUIRE(stale_pindex != nullptr); | ||
| const auto stale_first = dmnman.GetListForBlock(stale_pindex); | ||
| const size_t stale_size_after_first = dmnman.GetStaleListCacheSize(); | ||
| const CBlockIndex* stale_neighbor = tip->GetAncestor(stale_target_height + 16); | ||
| BOOST_REQUIRE(stale_neighbor != nullptr); | ||
| const auto stale_neighbor_result = dmnman.GetListForBlock(stale_neighbor); | ||
| const auto stale_second = dmnman.GetListForBlock(stale_pindex); | ||
| BOOST_CHECK(stale_first == stale_second); | ||
| BOOST_CHECK(stale_neighbor_result == dmnman.GetListForBlock(stale_neighbor)); | ||
| BOOST_CHECK_LE(dmnman.GetStaleListCacheSize(), stale_size_after_first + 1); |
There was a problem hiding this comment.
🟡 Suggestion: Repeated-access check does not prove the stale cache was hit
The result-equality and cache-size assertions cannot distinguish a stale-cache hit from another complete reconstruction. An implementation that still populates mnStaleListsCache but stops consulting it would produce the same lists and preserve the same size bound, so this test would pass while reintroducing the repeated historical rebuild that the stale tier is meant to prevent under cs_main. Add a narrow test diagnostic for stale hits or rebuilds and assert that the repeated lookup reaches a resident stale mini-snapshot without performing another full walk.
source: ['codex']
3 participants
Issue being fixed or feature implemented
CDeterministicMNManager's in-memory caches (mnListsCache,mnListDiffsCache) are only trimmed byCleanupCache(), which runs when a new block arrives. Between blocks there is no bound.GETMNLISTDIFFaccepts an arbitrary historicalbaseBlockHash, andGetListForBlockappends a cache entry per requested block. A peer requesting many distinct historical blocks therefore drives cache growth with no ceiling until the next block arrives. A full mainnet MN list is several MB, and the request requires no authentication, no proof of work, and is not rate limited.What was done?
Admission into both caches is bounded in
src/evo/deterministicmns.{cpp,h}:CacheMNList()retains tip-recent heights inmnListsCachewith hard capMAX_CACHE_LISTS = DISK_SNAPSHOT_PERIOD * 2(1152). Lowest-height-first eviction; tip snapshot protected.MAX_STALE_CACHE_LISTS = 32) instead of being dropped. Repeat stalegetmnlistdrequests reuse cached mini-snapshots instead of re-reading multi-MB disk snapshots on every call undercs_main.CacheMNListDiff()is recency-gated; rebuild walks admit diffs unconditionally thenEnforceDiffsCacheLimit()runs once after the walk (MAX_CACHE_DIFFS = LIST_DIFFS_CACHE_SIZE + 64).Rebuild walks use direct
emplacefor diffs (bypassing the recency filter by design) so the apply loop can resolve every walked hash; the hard bound is enforced once after the walk completes.How Has This Been Tested?
evo_dip3_activation_tests/mn_lists_cache_boundedmines past the recency window and diff cap, verifies both tiers stay bounded, stale LRU routing, and result stability after eviction.contrib/devtools/run-benchmark-mnlist-cache-7485.sh(cloned datadir): staleprotx diffrepeats andinvalidateblock/reconsiderblocknear tip.--enable-debug --enable-werror).Breaking Changes
None. The caches are memory-only and derived — no consensus behaviour, on-disk format, or P2P protocol change. A cache miss falls through to the existing rebuild path.
Checklist: