fix: bound mnListsCache admission to stop getmnlistd memory DoS

[PastaPastaPasta]

Audit finding V044. An unauthenticated peer can grow CDeterministicMNManager's in-memory caches without bound via getmnlistd.

Issue

mnListsCache and mnListDiffsCache were only trimmed by CleanupCache(), which runs when a new block arrives. Between blocks there is no bound at all: a peer spamming getmnlistd for historical blocks drives GetListForBlock to append an entry per requested block, and a full mainnet MN list is several MB. No proof of work, no authentication, no rate limit on the request side.

Fix

Bound admission two ways, in src/evo/deterministicmns.{cpp,h}:

• ShouldRetainCacheHeight() — do not retain at all any height older than the recency window CleanupCache would have dropped anyway (height + LIST_DIFFS_CACHE_SIZE >= tipIndex->nHeight). Retains freely before the tip is known, for early startup.
• EnforceListsCacheLimit() / EnforceDiffsCacheLimit() — hard caps (MAX_CACHE_LISTS = 256, MAX_CACHE_DIFFS = LIST_DIFFS_CACHE_SIZE + 64) that evict the oldest-height entry, never the tip snapshot.

Admission is funneled through new CacheMNList() / CacheMNListDiff() helpers so no call site can bypass the bound.

MAX_CACHE_LISTS is sized well above honest steady-state usage (tip + live quorum bases + mini-snapshots within the recency window).

Tests

test: prove mnListsCache grows unboundedly via historical GetListForBlock precedes the fix and fails without it. GetListCacheSize() / GetListDiffsCacheSize() accessors expose real production state rather than adding test-only mutation hooks.

Review notes

The third commit is a review follow-up worth reading on its own: the initial fix evicted diffs during the rebuild walk, so a diff the apply loop still needed could be evicted mid-walk, landing on a bare assert(false) — a remotely-triggerable crash, i.e. worse than the DoS being fixed. The follow-up removes the mid-walk eviction, admits every diff the walk reads unconditionally, and enforces the cap once the walk is done. It also deletes the assert(false) path.

Two locking details, both deliberate:

• CacheMNList prefers emplace over assignment because CDeterministicMNList::operator= locks m_cached_sml_mutex, which must not run while cs is held (lock-order checker).
• All new helpers are EXCLUSIVE_LOCKS_REQUIRED(cs) with AssertLockHeld.

Based on dashpay/dash develop @ 6d04c60ef36. Not rebase-tested against any newer tip; full functional suite not run.

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: e6cf6c95-c50b-4a5a-ab18-e55c6b508674

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch sec/v044

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}