Walkthrough

Added #include <node/blockstorage.h> and an in-memory mini-snapshot caching step inside CDeterministicMNManager::GetListForBlockInternal (src/evo/deterministicmns.cpp). After applying each cached diff to the snapshot, when not reindexing (fReindex == false) and the snapshot height is a multiple of 32 (MINI_SNAPSHOT_INTERVAL = 32), the code inserts the current snapshot into mnListsCache keyed by snapshot.GetBlockHash(). No changes to exported/public declarations; existing snapshot/diff loading and other caching logic remain unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

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.

@PastaPastaPasta

at first, I haven't planned to get this one merged, because I assumed that it improves only UndoBlock for several blocks; which maybe useful for reconsiderblock; and have very little benefits on mainnet; but actually it improves RPC performance for protx diff, especially for multiple calls for blocks that are close to each other. Also protx list is improved, if it's called multiple times for blocks that are close to each other in chain.

Consider:

1. Add Test Coverage
   • The PR lacks unit tests verifying the caching behavior
   • Consider adding tests to ensure:
     • Snapshots are created at correct intervals
     • Cache hits improve performance
     • Cache cleanup doesn't remove actively used snapshots
2. Documentation
   - The inline comment is good but could explain the "575 diffs" calculation more clearly
   - Consider adding a brief explanation of why 32 was chosen as the interval

1. We should probably disable this extra cache while node is still reindexing e.g

diff --git a/src/evo/deterministicmns.cpp b/src/evo/deterministicmns.cpp
index 921a119642..3f2dab1d3e 100644
--- a/src/evo/deterministicmns.cpp
+++ b/src/evo/deterministicmns.cpp
@@ -16,6 +16,7 @@
 #include <consensus/validation.h>
 #include <deploymentstatus.h>
 #include <messagesigner.h>
+#include <node/blockstorage.h>
 #include <script/standard.h>
 #include <stats/client.h>
 #include <uint256.h>
@@ -26,6 +27,8 @@
 #include <optional>
 #include <memory>
 
+using node::fReindex;
+
 static const std::string DB_LIST_SNAPSHOT = "dmn_S3";
 static const std::string DB_LIST_DIFF = "dmn_D3";
 
@@ -780,7 +783,7 @@ CDeterministicMNList CDeterministicMNManager::GetListForBlockInternal(gsl::not_n
     for (const auto& diffIndex : listDiffIndexes) {
         const auto& diff = mnListDiffsCache.at(diffIndex->GetBlockHash());
         snapshot.ApplyDiff(diffIndex, diff);
-        if (snapshot.GetHeight() % 32 == 0) {
+        if (!fReindex && (snapshot.GetHeight() % 32 == 0)) {
             // Add this temporary mini-snapshot to cache.
             // This extra cached mn-list helps to improve performance of GetListForBlock
             // for close blocks, because in the worst cases each of them requires to retrieve

1. Consider Making Cache Interval Configurable
   • The hardcoded value 32 could be a named constant or configurable parameter
     static constexpr int MINI_SNAPSHOT_INTERVAL = 32;
     if (snapshot.GetHeight() % MINI_SNAPSHOT_INTERVAL == 0) {

Agree

1. Add Test Coverage

   • The PR lacks unit tests verifying the caching behavior

   • Consider adding tests to ensure:

     • Snapshots are created at correct intervals
     • Cache hits improve performance
     • Cache cleanup doesn't remove actively used snapshots

Adding unit tests here feels like an overkill tbh

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

Consider Making Cache Interval Configurable

I declared named const MINI_SNAPSHOT_INTERVAL for it

The PR lacks unit tests verifying the caching behavior

It's a bit overkill IMO. considerable, possible to implement bench module for it; but it's not unit test. Also notice, I haven't touch cleanup function -> almost impossible to break any current logic (such as active quorum, tip caching)

Cache cleanup doesn't remove actively used snapshots

Implementation of cache cleanup is intact; every cleanup() call called by scheduler will remove this mini-snapshots.
They are meant to live only seconds; don't abuse RAM and helps only for consequent and often calls of RPC.

They help to improve performance for 50% of calculation protx diff N N+1 (it's an ideal case when cache will exist for sure) or reconsideration of any amount of blocks bigger than 1.

• Consider adding a brief explanation of why 32 was chosen as the interval

24 is a theoretically calculated number for MINI_SNAPSHOT_INTERVAL to reduce complexity from O(N^2) to O(N * sqrt(N)) where N is the distance between snapshots; for case if you want to pass all blocks between snapshots one time.
Though, I bench it, 32 and 24 had very close runtime, but 32 uses less RAM.
So, 32 has been chosen as it is close to theoretically calculated value; give a good performance boost and more conservative about RAM usage.

The best theoretical performance will have MINI_SNAPSHOT_INTERVAL=1 but it's a bit abusive for RAM, if scheduler won't be called on time.
Theoretical difference between MINI_SNAPSHOT_INTERVAL=1 and MINI_SNAPSHOT_INTERVAL=32 is less than 40% (assumed by results of benchmarking with perf) and it will potentially improve benefits of this PR from 10x for undo and 8x for protx diff to potentially up 15x for undo and up to 12x for protx diff in the best optimistic scenario, but I would prefer to be more conservative for RAM.

A new circular dependency in the form of "evo/deterministicmns -> node/blockstorage -> evo/deterministicmns" appears to have been introduced.
Good job! The circular dependency "dsnotificationinterface -> llmq/chainlocks -> node/blockstorage -> dsnotificationinterface" is no longer present.
Please remove it from EXPECTED_CIRCULAR_DEPENDENCIES in /__w/dash/dash/test/lint/lint-circular-dependencies.py

any ideas how to avoid it? Due to using fReindex

Nevermind, I already solved it by #6728, which is merged.
Fixed by rebasing on top of develop