fix: bound mnListsCache admission to stop getmnlistd memory exhaustion - #7485
fix: bound mnListsCache admission to stop getmnlistd memory exhaustion#7485PastaPastaPasta wants to merge 4 commits into
Conversation
…lock Add mn_lists_cache_bounded regression for V044/V049: after N distinct historical list loads without CleanupCache, mnListsCache size must stay <= MAX_CACHE_LISTS. Expose size getters and hard-cap constants for the assertion. Pre-fix fails with size 320 > 256.
Unauthenticated GETMNLISTDIFF / GetListForBlock could force arbitrary historical CDeterministicMNList snapshots into an uncapped mnListsCache until the next block advanced CleanupCache. Gate inserts with the existing LIST_DIFFS_CACHE_SIZE recency window and hard-cap list/diff caches with oldest-height eviction. Fixes V044/V049.
The rebuild walk in GetListForBlockInternal() enforced the diffs cap on every insertion, so a long walk could evict a diff that the very same walk still needed. The previous commit papered over this with a disk re-read plus a bare assert(false) in the apply loop, adding a failure mode that did not exist before the fix. Enforce the diffs cap once, after the apply loop, instead. The walk then always resolves from cache, so the .at() lookup is restored unchanged from upstream and the re-read/assert path is deleted. The cap still bounds the map, just at a walk boundary rather than mid-walk. Keep DISK_SNAPSHOT_PERIOD, DISK_SNAPSHOTS and LIST_DIFFS_CACHE_SIZE private; only the two new caps needed to become public for the test. Document why post-hoc CleanupCache() is insufficient. Extend the regression test to pin the property that actually matters: the caches are pure memoisation, so bounding them must not change results. It now records lists for a spread of historical heights, forces eviction by sweeping every height, and re-reads them to confirm the recomputed values are identical. Observed cache size after the sweep is exactly MAX_CACHE_LISTS (256) against 320 swept heights, so the cap is the binding constraint. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
✅ Final review complete — no blockers (commit bf91603) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 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 |
![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
| // 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)); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Force eviction before asserting recomputation stability.
Line 1705 records results after the cache is already capped; the descending sweep preserves newer entries and immediately evicts newly requested older entries. Thus Line 1738 does not guarantee any previously cached result was evicted and rebuilt. Mine more than MAX_CACHE_LISTS additional blocks after recording expected, update the tip, then re-query those original indexes.
As per coding guidelines, src/test/**/*.cpp: “prefer small tests that prove the invariant being changed.”
🤖 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 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.
Source: Coding guidelines
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']
| // 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']
| // 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']
| // 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']
The V0NN/U0NN labels were private working identifiers from a local review pass. They carry no meaning outside that pass, so they are removed while the surrounding technical rationale is kept.
![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']
2 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 at all.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 two ways, in
src/evo/deterministicmns.{cpp,h}:ShouldRetainCacheHeight()declines to retain any height older than the windowCleanupCachewould have dropped anyway (height + LIST_DIFFS_CACHE_SIZE >= tipIndex->nHeight). Before the tip is known (early startup / first connect) it retains freely.EnforceListsCacheLimit()/EnforceDiffsCacheLimit()evict the oldest-height entry when exceeded, never the tip snapshot.MAX_CACHE_LISTS = 256,MAX_CACHE_DIFFS = LIST_DIFFS_CACHE_SIZE + 64.Admission is funneled through new
CacheMNList()/CacheMNListDiff()helpers so no call site can bypass the bound.MAX_CACHE_LISTSis sized well above honest steady-state usage: tip, plus live quorum base blocks, plus mini-snapshots within the recency window.Two implementation details worth noting for review:
CacheMNList()usesemplacerather than assignment becauseCDeterministicMNList::operator=locksm_cached_sml_mutex, which must not run whilecsis held (lock-order checker).RecalculateAndRepairDiffswalk could drop a diff the apply loop still needed, landing on a bareassert(false). The walk now admits every diff it reads unconditionally and the cap is enforced once the walk completes; theassert(false)path is deleted.How Has This Been Tested?
New unit test
mnlistscache_bounded_under_historical_requestsinsrc/test/evo_deterministicmns_tests.cppdrivesGetListForBlockover historical blocks and asserts both caches stay within their caps. It is committed before the fix and fails without it.GetListCacheSize()/GetListDiffsCacheSize()accessors read real production state rather than introducing test-only mutation hooks.Built and ran the unit suite locally on macOS/arm64 (
--enable-debug --enable-werror), full suite green. The functional suite has not been run against this branch; CI will cover it.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:
🤖 Generated with Claude Code