Merge dashpay#6680: perf: avoid heavy object copy during ApplyDiff for CDeterministicMNList

73b68fe perf: avoid heavy object copy during ApplyDiff for CDeterministicMNList (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  The function `ApplyDiff` for CDeterministicMNList creates copy of current object and return it.
  It is not efficient in case of applying several diffs one after one.
  In case if `UndoBlock` and in case of calculating Quorum Members for old blocks (without caches) up to 576 temporary copies of CDeterministicMNList may be created and destroyed.

  ## What was done?
  `ApplyDiff` now changes current object, not returning a copy.

  It speed ups calculation of GetDeterministicMNListInternal for ~10%; this helper is most hot function during:
   - block undo
   - RPC `quorum info` for distant quorums (when it's not in cache).
   - RPC `quorum verify` for distant quorums (when it's not in cache)

  ## How Has This Been Tested?
  Run unit / functional tests

  Collected `perf` stats for block-undo.

  develop:
  <img width="772" alt="image" src="https://github.com/user-attachments/assets/a74e5dbb-314e-4067-b0dc-372235d37fb5" />

  PR:
  <img width="772" alt="image" src="https://github.com/user-attachments/assets/a587cb6d-96f1-4932-9143-90b7a08b2751" />

  ## Breaking Changes
  n/a

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  UdjinM6:
    ACK 73b68fe
  PastaPastaPasta:
    utACK 73b68fe

Tree-SHA512: cd82e35bdef6793d5bb0ea2189a66b01e50f318dbefd0283dab2973f6df8d99a731cef7b49aed5e8f9ca54c2282ab5570a35224e34c6d10447caeb9e097a31e5

Author: PastaPastaPasta

src/evo/deterministicmns.cpp

@@ -372,31 +372,28 @@ CDeterministicMNListDiff CDeterministicMNList::BuildDiff(const CDeterministicMNL
     return diffRet;
 }
 
-CDeterministicMNList CDeterministicMNList::ApplyDiff(gsl::not_null<const CBlockIndex*> pindex, const CDeterministicMNListDiff& diff) const
+void CDeterministicMNList::ApplyDiff(gsl::not_null<const CBlockIndex*> pindex, const CDeterministicMNListDiff& diff)
 {
-    CDeterministicMNList result = *this;
-    result.blockHash = pindex->GetBlockHash();
-    result.nHeight = pindex->nHeight;
+    blockHash = pindex->GetBlockHash();
+    nHeight = pindex->nHeight;
 
     for (const auto& id : diff.removedMns) {
-        auto dmn = result.GetMNByInternalId(id);
+        auto dmn = GetMNByInternalId(id);
         if (!dmn) {
             throw std::runtime_error(strprintf("%s: can't find a removed masternode, id=%d", __func__, id));
         }
-        result.RemoveMN(dmn->proTxHash);
+        RemoveMN(dmn->proTxHash);
     }
     for (const auto& dmn : diff.addedMNs) {
-        result.AddMN(dmn);
+        AddMN(dmn);
     }
     for (const auto& p : diff.updatedMNs) {
-        auto dmn = result.GetMNByInternalId(p.first);
+        auto dmn = GetMNByInternalId(p.first);
         if (!dmn) {
             throw std::runtime_error(strprintf("%s: can't find an updated masternode, id=%d", __func__, p.first));
         }
-        result.UpdateMN(*dmn, p.second);
+        UpdateMN(*dmn, p.second);
     }
-
-    return result;
 }
 
 void CDeterministicMNList::AddMN(const CDeterministicMNCPtr& dmn, bool fBumpTotalCount)
@@ -624,7 +621,8 @@ bool CDeterministicMNManager::UndoBlock(gsl::not_null<const CBlockIndex*> pindex
         mnListDiffsCache.erase(blockHash);
     }
     if (diff.HasChanges()) {
-        CDeterministicMNList curList{prevList.ApplyDiff(pindex, diff)};
+        CDeterministicMNList curList{prevList};
+        curList.ApplyDiff(pindex, diff);
 
         auto inversedDiff{curList.BuildDiff(prevList)};
         updatesRet = {curList, prevList, inversedDiff};
@@ -1007,12 +1005,7 @@ CDeterministicMNList CDeterministicMNManager::GetListForBlockInternal(gsl::not_n
 
     for (const auto& diffIndex : listDiffIndexes) {
         const auto& diff = mnListDiffsCache.at(diffIndex->GetBlockHash());
-        if (diff.HasChanges()) {
-            snapshot = snapshot.ApplyDiff(diffIndex, diff);
-        } else {
-            snapshot.SetBlockHash(diffIndex->GetBlockHash());
-            snapshot.SetHeight(diffIndex->nHeight);
-        }
+        snapshot.ApplyDiff(diffIndex, diff);
     }
 
     if (tipIndex) {

src/evo/deterministicmns.h

@@ -380,7 +380,12 @@ class CDeterministicMNList
     void PoSeDecrease(const CDeterministicMN& dmn);
 
     [[nodiscard]] CDeterministicMNListDiff BuildDiff(const CDeterministicMNList& to) const;
-    [[nodiscard]] CDeterministicMNList ApplyDiff(gsl::not_null<const CBlockIndex*> pindex, const CDeterministicMNListDiff& diff) const;
+    /**
+     * Apply Diff modifies current object.
+     * It is more efficient than creating a copy due to heavy copy constructor.
+     * Calculating for old block may require up to {DISK_SNAPSHOT_PERIOD} object copy & destroy.
+     */
+    void ApplyDiff(gsl::not_null<const CBlockIndex*> pindex, const CDeterministicMNListDiff& diff);
 
     void AddMN(const CDeterministicMNCPtr& dmn, bool fBumpTotalCount = true);
     void UpdateMN(const CDeterministicMN& oldDmn, const std::shared_ptr<const CDeterministicMNState>& pdmnState);

src/test/evo_deterministicmns_tests.cpp

@@ -366,9 +366,10 @@ void FuncV19Activation(TestChainSetup& setup)
 
     // check mn list/diff
     CDeterministicMNListDiff dummy_diff = base_list.BuildDiff(tip_list);
-    CDeterministicMNList dummmy_list = base_list.ApplyDiff(chainman.ActiveChain().Tip(), dummy_diff);
+    CDeterministicMNList dummy_list{base_list};
+    dummy_list.ApplyDiff(chainman.ActiveChain().Tip(), dummy_diff);
     // Lists should match
-    BOOST_REQUIRE(dummmy_list == tip_list);
+    BOOST_REQUIRE(dummy_list == tip_list);
 
     // mine 10 more blocks
     for (int i = 0; i < 10; ++i)
@@ -389,20 +390,22 @@ void FuncV19Activation(TestChainSetup& setup)
     const CBlockIndex* v19_index = chainman.ActiveChain().Tip()->GetAncestor(Params().GetConsensus().V19Height);
     auto v19_list = dmnman.GetListForBlock(v19_index);
     dummy_diff = v19_list.BuildDiff(tip_list);
-    dummmy_list = v19_list.ApplyDiff(chainman.ActiveChain().Tip(), dummy_diff);
-    BOOST_REQUIRE(dummmy_list == tip_list);
+    dummy_list = v19_list;
+    dummy_list.ApplyDiff(chainman.ActiveChain().Tip(), dummy_diff);
+    BOOST_REQUIRE(dummy_list == tip_list);
 
     // NOTE: this fails on v19/v19.1 with errors like:
     // "RemoveMN: Can't delete a masternode ... with a pubKeyOperator=..."
     dummy_diff = base_list.BuildDiff(tip_list);
-    dummmy_list = base_list.ApplyDiff(chainman.ActiveChain().Tip(), dummy_diff);
-    BOOST_REQUIRE(dummmy_list == tip_list);
+    dummy_list = base_list;
+    dummy_list.ApplyDiff(chainman.ActiveChain().Tip(), dummy_diff);
+    BOOST_REQUIRE(dummy_list == tip_list);
 
-    dummmy_list = base_list;
+    dummy_list = base_list;
     for (const auto& diff : diffs) {
-        dummmy_list = dummmy_list.ApplyDiff(chainman.ActiveChain().Tip(), diff);
+        dummy_list.ApplyDiff(chainman.ActiveChain().Tip(), diff);
     }
-    BOOST_REQUIRE(dummmy_list == tip_list);
+    BOOST_REQUIRE(dummy_list == tip_list);
 };
 
 void FuncDIP3Protx(TestChainSetup& setup)