Skip to content

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

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.

Sign up for GitHub

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

Merged
PastaPastaPasta merged 1 commit into from
May 27, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 11 additions & 18 deletions src/evo/deterministicmns.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,31 +423,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;
}
Comment on lines 426 to 448
Copy link

@coderabbitai coderabbitai bot May 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add consistency guards inside ApplyDiff

The in-place variant looks solid, but a few inexpensive assertions would harden it even more:

  1. assert(diff.nHeight == pindex->nHeight);
    – Helps detect accidental mismatches when we pass the wrong diff/pindex pair.

  2. Early exit when the diff is empty to skip hash/height updates in the no-op case (micro-optimisation but costs nothing).

 void CDeterministicMNList::ApplyDiff(gsl::not_null<const CBlockIndex*> pindex, const CDeterministicMNListDiff& diff)
 {
+    // Sanity: the diff we’re applying must belong to the same block.
+    assert(diff.nHeight == -1 || diff.nHeight == pindex->nHeight);
+    if (!diff.HasChanges()) {
+        blockHash = pindex->GetBlockHash();
+        nHeight   = pindex->nHeight;
+        return;
+    }

Those checks catch logical errors early without any runtime penalty in production builds (asserts are stripped in -DNDEBUG).

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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::ApplyDiff(gsl::not_null<const CBlockIndex*> pindex, const CDeterministicMNListDiff& diff)
{
// Sanity: the diff we’re applying must belong to the same block.
assert(diff.nHeight == -1 || diff.nHeight == pindex->nHeight);
if (!diff.HasChanges()) {
blockHash = pindex->GetBlockHash();
nHeight = pindex->nHeight;
return;
}
blockHash = pindex->GetBlockHash();
nHeight = pindex->nHeight;
for (const auto& id : diff.removedMns) {
auto dmn = GetMNByInternalId(id);
if (!dmn) {
throw std::runtime_error(strprintf("%s: can't find a removed masternode, id=%d", __func__, id));
}
RemoveMN(dmn->proTxHash);
}
for (const auto& dmn : diff.addedMNs) {
AddMN(dmn);
}
for (const auto& p : diff.updatedMNs) {
auto dmn = GetMNByInternalId(p.first);
if (!dmn) {
throw std::runtime_error(strprintf("%s: can't find an updated masternode, id=%d", __func__, p.first));
}
UpdateMN(*dmn, p.second);
}
}
🤖 Prompt for AI Agents 
In src/evo/deterministicmns.cpp around lines 426 to 448, add an assertion at the
start of ApplyDiff to verify that diff.nHeight equals pindex->nHeight to ensure
consistency between the diff and block index. Also, add an early return if the
diff is empty (no removed, added, or updated masternodes) to avoid unnecessary
updates to blockHash and nHeight. These changes improve robustness and
efficiency without affecting production performance due to asserts being
disabled in release builds.


void CDeterministicMNList::AddMN(const CDeterministicMNCPtr& dmn, bool fBumpTotalCount)
Expand Down Expand Up @@ -708,7 +705,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};
Expand Down Expand Up @@ -1096,12 +1094,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);
Copy link
Member

@PastaPastaPasta PastaPastaPasta May 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I correct that this is the only place we have a performance improvement right? All other instances, just now have an explicit copy instead of implicit copy. This change does seem worth it, but only this specific one in GetListForBlockInternal has any perf improvement

Copy link
Collaborator Author

@knst knst May 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not counting unit tests, there are only 2 usages of ApplyDiff: GetListForBlockInternal and in UndoBlock. GetListForBlockInternal will be benefitial from this optimisation; undo block - doesn't become slower either faster.

}

if (tipIndex) {
Expand Down
7 changes: 6 additions & 1 deletion src/evo/deterministicmns.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,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);
Expand Down
21 changes: 12 additions & 9 deletions src/test/evo_deterministicmns_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -369,9 +369,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)
Expand All @@ -392,20 +393,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)
Expand Down
Loading