[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

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

✨ 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.}