perf: use tiny cache for best chainlock in CbTx instead running heavy ReadBlockFromDisk #6656
Conversation
WalkthroughThe changes modify the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
🔇 Additional comments (2)
✨ Finishing Touches
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/evo/cbtx.cpp(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: Clang Diff Format Check
src/evo/cbtx.cpp
[error] 338-345: Clang format differences found. Code formatting does not match the expected style. Please run clang-format to fix formatting issues.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: win64-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: arm-linux-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
🔇 Additional comments (3)
src/evo/cbtx.cpp (3)
340-356: Good performance optimization with the caching mechanismThe implemented caching strategy for chainlock data is an effective way to avoid redundant disk reads through
GetNonNullCoinbaseChainlockwhen processing consecutive blocks. The mutex ensures thread safety when accessing the cached data.The code properly checks if the previous block index matches the cached one before reusing the cached chainlock, which prevents stale data issues.
384-389: LGTM: Proper cache update after validationThe code correctly updates the cache with the current block's chainlock data after validation, which ensures the cache remains valid for subsequent blocks.
461-464: Effective optimization for pre-V20 blocksThis is a good optimization that avoids unnecessary disk reads for blocks before V20 activation, as those blocks don't have chainlock data in the coinbase transaction. The check is placed before the existing DIP0003 check, which is correct since V20 comes after DIP0003 in the deployment timeline.
knst
force-pushed
the
perf-cl-cbtx-cache
branch
2 times, most recently
from
7ebc7af to
78e73f8
Compare
src/evo/cbtx.cpp
Outdated
| { | ||
| LOCK(cached_mutex); | ||
| cached_chainlock = std::make_pair(cbTx.bestCLSignature, cbTx.bestCLHeightDiff); | ||
| cached_pindex = pindex; | ||
| } |
There was a problem hiding this comment.
this extra scope is redundant
src/evo/cbtx.cpp
Outdated
| { | ||
| LOCK(cached_mutex); | ||
| cached_chainlock = std::make_pair(cbTx.bestCLSignature, cbTx.bestCLHeightDiff); | ||
| cached_pindex = pindex; | ||
| } |
src/evo/cbtx.cpp
Outdated
| { | ||
| LOCK(cached_mutex); | ||
| if (cached_pindex == pindex->pprev) { | ||
| use_cached = true; | ||
| prevBlockCoinbaseChainlock = cached_chainlock; | ||
| } | ||
| } |
There was a problem hiding this comment.
| { | |
| LOCK(cached_mutex); | |
| if (cached_pindex == pindex->pprev) { | |
| use_cached = true; | |
| prevBlockCoinbaseChainlock = cached_chainlock; | |
| } | |
| } | |
| if (LOCK(cached_mutex); cached_pindex == pindex->pprev) { | |
| use_cached = true; | |
| prevBlockCoinbaseChainlock = cached_chainlock; | |
| } |
| std::optional<std::pair<CBLSSignature, uint32_t>> prevBlockCoinbaseChainlock{std::nullopt}; | ||
| bool use_cached = false; | ||
| { | ||
| LOCK(cached_mutex); | ||
| if (cached_pindex == pindex->pprev) { | ||
| use_cached = true; | ||
| prevBlockCoinbaseChainlock = cached_chainlock; | ||
| } | ||
| } |
There was a problem hiding this comment.
or something like that
| std::optional<std::pair<CBLSSignature, uint32_t>> prevBlockCoinbaseChainlock{std::nullopt}; | |
| bool use_cached = false; | |
| { | |
| LOCK(cached_mutex); | |
| if (cached_pindex == pindex->pprev) { | |
| use_cached = true; | |
| prevBlockCoinbaseChainlock = cached_chainlock; | |
| } | |
| } | |
| auto [prevBlockCoinbaseChainlock, use_cached] = [&]() { | |
| LOCK(cached_mutex); | |
| if (cached_pindex == pindex->pprev) { | |
| return std::make_pair(cached_chainlock, true); | |
| } | |
| return std::make_pair(std::nullopt, false); | |
| }();``` |
src/evo/cbtx.cpp
Outdated
| { | ||
| LOCK(cached_mutex); | ||
| cached_chainlock = std::make_pair(cbTx.bestCLSignature, cbTx.bestCLHeightDiff); | ||
| cached_pindex = pindex; | ||
| } |
src/evo/cbtx.cpp
Outdated
| use_cached = true; | ||
| prevBlockCoinbaseChainlock = cached_chainlock; |
There was a problem hiding this comment.
isn't this bool the same as prevBlockCoinbaseChainlock.has_value()?
| UpdateLLMQTestParametersFromArgs(args, Consensus::LLMQType::LLMQ_TEST_INSTANTSEND); | ||
| UpdateLLMQTestParametersFromArgs(args, Consensus::LLMQType::LLMQ_TEST_PLATFORM); | ||
| UpdateLLMQInstantSendDIP0024FromArgs(args); | ||
| assert(consensus.V20Height >= consensus.DIP0003Height); |
There was a problem hiding this comment.
because of conversation here: https://github.com/dashpay/dash/pull/6656/files#r2075111410
…nstead running heavy ReadBlockFromDisk 18044c9 perf: use tiny cache for best chainlock in CbTx instead running heavy ReadBlockFromDisk (Konstantin Akimov) 66c9592 refactor: use more strict scope v20 instead dip0003 (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Validation of chainlocks requires to read previous block from disk for every new block. ## What was done? Added CL from CbTx of tip of the chain. ## How Has This Been Tested? develop: <img width="754" alt="image" src="https://github.com/user-attachments/assets/7e1a5ab4-9634-4fc7-910e-9124d2d742d0" /> ``` 2025-05-04T18:36:29Z [bench] - CheckCbTxBestChainlock: 3.34ms [18.68s] 2025-05-04T18:36:29Z [bench] - Connect total: 54.40ms [94.48s (16.14ms/blk)] ``` PR: <img width="754" alt="image" src="https://github.com/user-attachments/assets/462df0f1-358d-46dd-8a08-f3a0324bb282" /> ``` 2025-05-04T18:53:18Z [bench] - CheckCbTxBestChainlock: 3.15ms [16.41s] 2025-05-04T18:53:18Z [bench] - Connect total: 52.89ms [90.75s (15.51ms/blk)] ``` `perf` looks a bit confusing (like cpu spent for CheckCbTxBestChainlock is close to zero which contradicts to logs time (16seconds vs 18seconds). It happened due to missing data for bls / gmp - somehow they are not attached to the caller, but shown as a root nodes without proper stacktrace). Detailed break-down is: ``` 2025-05-04T19:50:37Z [bench] - CheckCbTxBestChainlock::payload: 0.22ms [1.34s] 2025-05-04T19:50:37Z [bench] - CheckCbTxBestChainlock::get-cb-cl: 0.52ms [2.50s] 2025-05-04T19:50:37Z [bench] - CheckCbTxBestChainlock::validate: 2.63ms [14.94s] <--- missing in perf 2025-05-04T19:50:37Z [bench] - CheckCbTxBestChainlock: 3.38ms [18.87s] ``` ## 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: utACK 18044c9 PastaPastaPasta: utACK 18044c9 Tree-SHA512: 21508505e4b3cec9cba7f3835e2b73a81461d7b626f97efb15d442767fa0862f6ba92ed1101e00b04dbbddab1b7c56da796b15c89d80c95ce9b0246bbbbe02f2
3 participants
Issue being fixed or feature implemented
Validation of chainlocks requires to read previous block from disk for every new block.
What was done?
Added CL from CbTx of tip of the chain.
How Has This Been Tested?
develop:
PR:
perflooks a bit confusing (like cpu spent for CheckCbTxBestChainlock is close to zero which contradicts to logs time (16seconds vs 18seconds). It happened due to missing data for bls / gmp - somehow they are not attached to the caller, but shown as a root nodes without proper stacktrace). Detailed break-down is:Breaking Changes
N/A
Checklist: