perf: reuse result of BuildNewListFromBlock #6640
Conversation
WalkthroughThe changes across the codebase refactor how masternode list Merkle root calculations and validations are performed. Functions related to checking and calculating the Merkle root for masternode lists ( Consequently, call sites in components such as the special transaction processor, miner, and test setup are updated to explicitly construct the deterministic masternode list and convert it to a simplified list before invoking Merkle root calculations or checks. The Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
⏰ Context from checks skipped due to timeout of 90000ms (10)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/evo/deterministicmns.cpp (1)
600-605: Fix code formattingThere's a formatting issue in this section according to the Clang format check. Please run the formatter to fix the spacing.
-bool CDeterministicMNManager::ProcessBlock(const CBlock& block, gsl::not_null<const CBlockIndex*> pindex, - BlockValidationState& state, const CCoinsViewCache& view, - llmq::CQuorumSnapshotManager& qsnapman, - const CDeterministicMNList& newList, - std::optional<MNListUpdates>& updatesRet) +bool CDeterministicMNManager::ProcessBlock(const CBlock& block, gsl::not_null<const CBlockIndex*> pindex, + BlockValidationState& state, const CCoinsViewCache& view, + llmq::CQuorumSnapshotManager& qsnapman, + const CDeterministicMNList& newList, + std::optional<MNListUpdates>& updatesRet)🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 600-610: Clang format differences found. Code formatting does not match the expected style.
src/evo/cbtx.h (1)
86-92: Fix code formattingThere's a formatting issue in this section according to the Clang format check. Please run the formatter to fix the spacing.
-bool CheckCbTxMerkleRoots(const CBlock& block, const CBlockIndex* pindex, - const llmq::CQuorumBlockProcessor& quorum_block_processor, - CSimplifiedMNList&& sml, - BlockValidationState& state); -bool CalcCbTxMerkleRootMNList(uint256& merkleRootRet, CSimplifiedMNList&& sml, BlockValidationState& state); +bool CheckCbTxMerkleRoots(const CBlock& block, const CBlockIndex* pindex, + const llmq::CQuorumBlockProcessor& quorum_block_processor, + CSimplifiedMNList&& sml, + BlockValidationState& state); +bool CalcCbTxMerkleRootMNList(uint256& merkleRootRet, CSimplifiedMNList&& sml, BlockValidationState& state);🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 86-90: Clang format differences found. Code formatting does not match the expected style.
src/evo/cbtx.cpp (1)
61-67: Fix code formattingThere's a formatting issue in this section according to the Clang format check. Please run the formatter to fix the spacing.
-bool CheckCbTxMerkleRoots(const CBlock& block, const CBlockIndex* pindex, - const llmq::CQuorumBlockProcessor& quorum_block_processor, - CSimplifiedMNList&& sml, - BlockValidationState& state) +bool CheckCbTxMerkleRoots(const CBlock& block, const CBlockIndex* pindex, + const llmq::CQuorumBlockProcessor& quorum_block_processor, + CSimplifiedMNList&& sml, + BlockValidationState& state)🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 61-70: Clang format differences found. Code formatting does not match the expected style.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/evo/cbtx.cpp(4 hunks)src/evo/cbtx.h(2 hunks)src/evo/deterministicmns.cpp(2 hunks)src/evo/deterministicmns.h(1 hunks)src/evo/specialtxman.cpp(2 hunks)src/node/miner.cpp(2 hunks)src/test/util/setup_common.cpp(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/evo/specialtxman.cpp (3)
src/evo/deterministicmns.h (7)
pindex(371-371)pindex(585-585)pindex(587-587)pindex(596-596)pindex(609-609)block(582-584)block(590-592)src/evo/specialtxman.h (3)
block(72-73)block(75-75)block(77-77)src/evo/cbtx.cpp (2)
CheckCbTxMerkleRoots(63-117)CheckCbTxMerkleRoots(63-66)
🪛 GitHub Actions: Clang Diff Format Check
src/evo/specialtxman.cpp
[error] 7-20: Clang format differences found. Code formatting does not match the expected style.
src/evo/deterministicmns.h
[error] 580-590: Clang format differences found. Code formatting does not match the expected style.
src/evo/deterministicmns.cpp
[error] 600-610: Clang format differences found. Code formatting does not match the expected style.
src/evo/cbtx.cpp
[error] 61-70: Clang format differences found. Code formatting does not match the expected style.
src/evo/cbtx.h
[error] 86-90: Clang format differences found. Code formatting does not match the expected style.
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: x86_64-apple-darwin / Build depends
🔇 Additional comments (14)
src/test/util/setup_common.cpp (2)
54-54: Added include for simplified masternode list header.The include is necessary to support the optimization where a simplified masternode list is constructed from the deterministic masternode list.
490-494: Optimize by reusing the built masternode list.The code now builds the deterministic masternode list once and reuses it when calculating the masternode list merkle root, which is in line with the PR's performance optimization goal. The implementation includes appropriate error checking with the assertion.
src/node/miner.cpp (2)
31-31: Added include for deterministicmns header.The include is necessary to support the optimization changes that follow.
228-233: Performance improvement by reusing the built masternode list.Instead of recalculating the deterministic masternode list inside
CalcCbTxMerkleRootMNList, this implementation builds it once and passes it to the function. This aligns with the PR's goal of reducing redundant computations during block validation, particularly important given the large number of masternodes. The error handling is properly done with clear error messages.src/evo/specialtxman.cpp (2)
178-190: Optimized implementation to build and reuse the masternode list.This implementation explicitly builds the deterministic masternode list once and reuses it for both the
ProcessBlockcall and theCheckCbTxMerkleRootscall. This aligns with the PR's performance optimization goal and matches the updated interface of theProcessBlockmethod. The code properly handles errors and state passing.
196-196: Use the pre-built masternode list for checking merkle roots.The function now passes a
CSimplifiedMNListconstructed from the pre-built deterministic masternode list instead of havingCheckCbTxMerkleRootsbuild it internally. This completes the optimization by reusing the same list for both operations.src/evo/deterministicmns.cpp (2)
601-605: Process block function signature refactored to accept pre-calculated deterministic MN listThe function signature now accepts a pre-built
CDeterministicMNListas a parameter instead of recalculating it internally. This is the main performance optimization of this PR - eliminating the redundant call toBuildNewListFromBlock.
614-623: Refactored code flow to use externally provided MN listThe function no longer builds the new list internally, but instead uses the provided
newListto compute the diff against the old list. This change aligns with the PR objective of reusing the pre-calculated list during block validation.src/evo/cbtx.h (2)
88-91: Modified CheckCbTxMerkleRoots to accept pre-calculated simplified MN listThe function signature has been updated to take a pre-built
CSimplifiedMNListas an rvalue reference parameter instead of constructing it internally. This matches the PR objectives by eliminating duplicate construction of the masternode list.
92-92: Simplified CalcCbTxMerkleRootMNList function signatureThe function has been significantly simplified to accept only the output parameter for the Merkle root, a pre-built simplified MN list, and the validation state. Dependencies on block, previous index, deterministic MN manager, quorum snapshot manager, and coins view cache have been removed.
src/evo/cbtx.cpp (4)
63-66: CheckCbTxMerkleRoots function signature updated to match headerThe implementation now accepts a pre-built
CSimplifiedMNListas an rvalue reference parameter, consistent with the header file changes and PR optimization objective.
90-90: Using pre-calculated simplified MN list for Merkle root calculationThe code now passes the externally built simplified MN list to
CalcCbTxMerkleRootMNListusing std::move to avoid unnecessary copying, which is efficient and aligns with the optimization goal.
119-119: Simplified CalcCbTxMerkleRootMNList function signatureThe function implementation now matches the updated header declaration, accepting only the necessary parameters: the output Merkle root, the pre-built simplified MN list, and the validation state.
143-144: Updated benchmark loggingThe benchmark logging correctly shows the timing for the Merkle root calculation only, without including the time to build the simplified MN list (which is now done outside this function).
src/evo/specialtxman.cpp
Outdated
| @@ -10,6 +10,7 @@ | |||
| #include <evo/cbtx.h> | |||
| #include <evo/creditpool.h> | |||
| #include <evo/deterministicmns.h> | |||
| #include <evo/simplifiedmns.h> | |||
There was a problem hiding this comment.
Added include for simplified masternode list header.
The inclusion is necessary to support the use of CSimplifiedMNList in the updated code.
There's a Clang format issue in the file that should be fixed. The GitHub Actions pipeline shows a formatting error for lines 7-20.
knst
force-pushed
the
perf-BuildNewListFromBlock
branch
from
cdbe900 to
2f74b8d
Compare
Issue being fixed or feature implemented
Method
BuildNewListFromBlockis called twice for each block during validation. These calculation are pretty heavy assuming that there's thousands of masternodes. It is not much for 1 block but that's a lot of CPU calculations during full re-index or initial indexation for 2M of blocks.What was done?
Methods
CalcCbTxMerkleRootMNListandCDeterministicMNManager::ProcessBlockduring block calculation re-use same pre-calculated SML instead internal re-calculation.How Has This Been Tested?
Benchmarks are done on branch including this changes: #6632
I run it several times in different days and results are differ, but it is consistently faster.
There's logs for 3 different runs, see sum of
m_dmnmanandCheckCbTxMerkleRoots.Run time for ~5000blocks:
Run time for 6000 blocks:
Run time for 9000 blocks:
Breaking Changes
N/A
Checklist: