-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: bound mnListsCache admission to stop getmnlistd memory exhaustion #7485
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
76b5d96
6fee2db
f2f1e9f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -664,14 +664,14 @@ bool CDeterministicMNManager::ProcessBlock(const CBlock& block, gsl::not_null<co | |
| m_evoDb.Write(std::make_pair(DB_LIST_DIFF, newList.GetBlockHash()), diff); | ||
| if ((nHeight % DISK_SNAPSHOT_PERIOD) == 0 || pindex->pprev == m_initial_snapshot_index) { | ||
| m_evoDb.Write(std::make_pair(DB_LIST_SNAPSHOT, newList.GetBlockHash()), newList); | ||
| mnListsCache.emplace(newList.GetBlockHash(), newList); | ||
| CacheMNList(newList.GetBlockHash(), newList); | ||
| LogPrintf("CDeterministicMNManager::%s -- Wrote snapshot. nHeight=%d, mapCurMNs.allMNsCount=%d\n", | ||
| __func__, nHeight, newList.GetCounts().total()); | ||
| } | ||
|
|
||
| diff.nHeight = pindex->nHeight; | ||
| mnListDiffsCache.emplace(pindex->GetBlockHash(), diff); | ||
| mnListsCache.emplace(newList.GetBlockHash(), newList); | ||
| CacheMNListDiff(pindex->GetBlockHash(), diff); | ||
| CacheMNList(newList.GetBlockHash(), newList); | ||
| } catch (const std::exception& e) { | ||
| LogPrintf("CDeterministicMNManager::%s -- internal error: %s\n", __func__, e.what()); | ||
| return state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "failed-dmn-block"); | ||
|
|
@@ -750,6 +750,80 @@ void CDeterministicMNManager::UpdatedBlockTip(gsl::not_null<const CBlockIndex*> | |
| tipIndex = pindex; | ||
| } | ||
|
|
||
| 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; | ||
| } | ||
|
|
||
| void CDeterministicMNManager::EnforceListsCacheLimit() | ||
| { | ||
| AssertLockHeld(cs); | ||
| while (mnListsCache.size() > MAX_CACHE_LISTS) { | ||
| // Evict the oldest-height entry that is not the tip snapshot. | ||
| auto victim = mnListsCache.end(); | ||
| for (auto it = mnListsCache.begin(); it != mnListsCache.end(); ++it) { | ||
| if (tipIndex != nullptr && it->first == tipIndex->GetBlockHash()) { | ||
| continue; | ||
| } | ||
| if (victim == mnListsCache.end() || it->second.GetHeight() < victim->second.GetHeight()) { | ||
| victim = it; | ||
| } | ||
| } | ||
| if (victim == mnListsCache.end()) { | ||
| // Only the tip remains; nothing further to drop. | ||
| break; | ||
| } | ||
| mnListsCache.erase(victim); | ||
| } | ||
| } | ||
|
|
||
| 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); | ||
| } | ||
|
Comment on lines
+784
to
+798
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: Post-walk diff-cache eviction rescans the whole map per victim while cs_main is held
source: ['claude', 'codex'] |
||
| } | ||
|
|
||
| void CDeterministicMNManager::CacheMNList(const uint256& block_hash, const CDeterministicMNList& list) | ||
| { | ||
| AssertLockHeld(cs); | ||
| if (!ShouldRetainCacheHeight(list.GetHeight())) { | ||
| return; | ||
| } | ||
| // Prefer emplace over assign: CDeterministicMNList::operator= locks m_cached_sml_mutex | ||
| // and must not run while cs is held (lock-order checker). | ||
| const auto [_, inserted] = mnListsCache.emplace(block_hash, list); | ||
| if (inserted) { | ||
| EnforceListsCacheLimit(); | ||
| } | ||
| } | ||
|
|
||
| void CDeterministicMNManager::CacheMNListDiff(const uint256& block_hash, CDeterministicMNListDiff diff) | ||
| { | ||
| AssertLockHeld(cs); | ||
| if (!ShouldRetainCacheHeight(diff.nHeight)) { | ||
| return; | ||
| } | ||
| const auto [_, inserted] = mnListDiffsCache.emplace(block_hash, std::move(diff)); | ||
| if (inserted) { | ||
| EnforceDiffsCacheLimit(); | ||
| } | ||
| } | ||
|
|
||
| CDeterministicMNList CDeterministicMNManager::GetListForBlockInternal(gsl::not_null<const CBlockIndex*> pindex) | ||
| { | ||
| CDeterministicMNList snapshot; | ||
|
|
@@ -771,7 +845,8 @@ CDeterministicMNList CDeterministicMNManager::GetListForBlockInternal(gsl::not_n | |
| } | ||
|
|
||
| if (m_evoDb.Read(std::make_pair(DB_LIST_SNAPSHOT, pindex->GetBlockHash()), snapshot)) { | ||
| mnListsCache.emplace(pindex->GetBlockHash(), snapshot); | ||
| // Use the list; only retain it in the cache if it is tip-recent. | ||
| CacheMNList(pindex->GetBlockHash(), snapshot); | ||
| break; | ||
| } | ||
|
|
||
|
|
@@ -788,13 +863,17 @@ CDeterministicMNList CDeterministicMNManager::GetListForBlockInternal(gsl::not_n | |
| // no snapshot and no diff on disk means that it's the initial snapshot | ||
| m_initial_snapshot_index = pindex; | ||
| snapshot = CDeterministicMNList(pindex->GetBlockHash(), pindex->nHeight, 0); | ||
| mnListsCache.emplace(pindex->GetBlockHash(), snapshot); | ||
| CacheMNList(pindex->GetBlockHash(), snapshot); | ||
| LogPrintf("CDeterministicMNManager::%s -- initial snapshot. blockHash=%s nHeight=%d\n", | ||
| __func__, snapshot.GetBlockHash().ToString(), snapshot.GetHeight()); | ||
| break; | ||
| } | ||
|
|
||
| diff.nHeight = pindex->nHeight; | ||
| // Cache for this rebuild pass even if older than the retention window, so that | ||
| // 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); | ||
|
Comment on lines
+874
to
878
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💬 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, source: ['claude'] |
||
| pindex = pindex->pprev; | ||
|
|
@@ -818,14 +897,14 @@ CDeterministicMNList CDeterministicMNManager::GetListForBlockInternal(gsl::not_n | |
| // There is also separate in-memory caching for the current tip and active quorums, | ||
| // but this mini-snapshot cache specifically speeds up repeated requests | ||
| // for nearby historical blocks. | ||
| mnListsCache.emplace(snapshot.GetBlockHash(), snapshot); | ||
| CacheMNList(snapshot.GetBlockHash(), snapshot); | ||
| } | ||
| } | ||
|
|
||
| if (tipIndex) { | ||
| // always keep a snapshot for the tip | ||
| if (const auto snapshot_hash = snapshot.GetBlockHash(); snapshot_hash == tipIndex->GetBlockHash()) { | ||
| mnListsCache.emplace(snapshot_hash, snapshot); | ||
| CacheMNList(snapshot_hash, snapshot); | ||
| } else { | ||
| // keep snapshots for yet alive quorums | ||
| if (std::ranges::any_of(Params().GetConsensus().llmqs, [&snapshot, this]( | ||
|
|
@@ -835,11 +914,15 @@ CDeterministicMNList CDeterministicMNManager::GetListForBlockInternal(gsl::not_n | |
| (snapshot.GetHeight() + params.dkgInterval * (params.keepOldConnections + 1) >= | ||
| tipIndex->nHeight); | ||
| })) { | ||
| mnListsCache.emplace(snapshot_hash, snapshot); | ||
| CacheMNList(snapshot_hash, snapshot); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // The rebuild walk above admits every diff it reads unconditionally, so that the | ||
| // apply loop can resolve them. Enforce the hard bound now that the walk is done. | ||
| EnforceDiffsCacheLimit(); | ||
|
|
||
| assert(snapshot.GetHeight() != -1); | ||
| return snapshot; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -683,6 +683,19 @@ class CDeterministicMNManager | |
| static constexpr int DISK_SNAPSHOTS = llmq_max_blocks() / DISK_SNAPSHOT_PERIOD + 1; | ||
| static constexpr int LIST_DIFFS_CACHE_SIZE = DISK_SNAPSHOT_PERIOD * DISK_SNAPSHOTS; | ||
|
|
||
| public: | ||
| // Hard caps on the in-memory caches. CleanupCache() alone is not enough: it only | ||
| // 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; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. probably wrong and will affect performance significantly. It should be at least 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. |
||
| // Diffs are small; allow a full recency window plus a margin for one rebuild walk. | ||
|
Comment on lines
+688
to
+696
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 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'] |
||
| static constexpr size_t MAX_CACHE_DIFFS = static_cast<size_t>(LIST_DIFFS_CACHE_SIZE) + 64; | ||
|
|
||
| private: | ||
| Mutex cs; | ||
| Mutex cs_cleanup; | ||
|
|
@@ -720,6 +733,18 @@ class CDeterministicMNManager | |
| }; | ||
| CDeterministicMNList GetListAtChainTip() EXCLUSIVE_LOCKS_REQUIRED(!cs); | ||
|
|
||
| // In-memory list/diff cache sizes (for tests and diagnostics). | ||
| size_t GetListCacheSize() EXCLUSIVE_LOCKS_REQUIRED(!cs) | ||
| { | ||
| LOCK(cs); | ||
| return mnListsCache.size(); | ||
| } | ||
| size_t GetListDiffsCacheSize() EXCLUSIVE_LOCKS_REQUIRED(!cs) | ||
| { | ||
| LOCK(cs); | ||
| return mnListDiffsCache.size(); | ||
| } | ||
|
|
||
| // Test if given TX is a ProRegTx which also contains the collateral at index n | ||
| static bool IsProTxWithCollateral(const CTransactionRef& tx, uint32_t n); | ||
|
|
||
|
|
@@ -759,6 +784,12 @@ class CDeterministicMNManager | |
| private: | ||
| void CleanupCache(int nHeight) EXCLUSIVE_LOCKS_REQUIRED(cs); | ||
| CDeterministicMNList GetListForBlockInternal(gsl::not_null<const CBlockIndex*> pindex) EXCLUSIVE_LOCKS_REQUIRED(cs); | ||
| // Retain only tip-recent heights (same window CleanupCache uses for "too old"). | ||
| [[nodiscard]] bool ShouldRetainCacheHeight(int height) EXCLUSIVE_LOCKS_REQUIRED(cs); | ||
| void CacheMNList(const uint256& block_hash, const CDeterministicMNList& list) EXCLUSIVE_LOCKS_REQUIRED(cs); | ||
| void CacheMNListDiff(const uint256& block_hash, CDeterministicMNListDiff diff) EXCLUSIVE_LOCKS_REQUIRED(cs); | ||
| void EnforceListsCacheLimit() EXCLUSIVE_LOCKS_REQUIRED(cs); | ||
| void EnforceDiffsCacheLimit() EXCLUSIVE_LOCKS_REQUIRED(cs); | ||
|
|
||
| // Helper methods for RecalculateAndRepairDiffs | ||
| std::vector<const CBlockIndex*> CollectSnapshotBlocks(const CBlockIndex* start_index, const CBlockIndex* stop_index, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1680,6 +1680,72 @@ BOOST_AUTO_TEST_CASE(field_bit_migration_validation) | |
| BOOST_CHECK_EQUAL(usedBits.size(), 19); | ||
| } | ||
|
|
||
| // 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)); | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| // 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); | ||
|
Comment on lines
+1686
to
+1746
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Suggestion: Regression test never crosses the recency window and likely never forces the diffs hard cap
source: ['claude', 'codex'] |
||
| } | ||
|
Comment on lines
+1699
to
+1747
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 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'] |
||
|
|
||
| BOOST_AUTO_TEST_CASE(migration_logic_validation) | ||
| { | ||
| // Test the database migration logic for nVersion-first format conversion. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When an unauthenticated
GETMNLISTDIFFrequests 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 roughlyMAX_CACHE_DIFFS-sized map, making each request Θ(excess × cache-size); because the inspected handler innet_processing.cppperforms the entire operation while holdingcs_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 👍 / 👎.