perf: parse CbTx only once perf block #6719
Conversation
It combines calls CheckCbTxMerkleRoots and CheckCbTxBestChainlock
It combines CheckCbTx with other checks (CheckCbTxMerkleRoots and CheckCbTxBestChainlock)
Re-use CCbTx for CheckCreditPoolDiffForBlock
WalkthroughThe changes refactor several special transaction validation functions to accept a ✨ 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: 0
🧹 Nitpick comments (3)
src/evo/specialtxman.cpp (3)
133-136: Clarify the comment about transaction processing.The comment "starts from the 2nd transaction" is misleading. The code only skips the first transaction if it's a coinbase transaction.
for (size_t i = 0; i < block.vtx.size(); ++i) { - // we validated CCbTx above, starts from the 2nd transaction + // Skip coinbase transaction as we already validated its CCbTx above if (i == 0 && block.vtx[i]->nType == TRANSACTION_COINBASE) continue;
140-140: Fix typo in comment.Minor grammatical correction needed.
-// At this moment CheckSpecialTx() may fail by 2 possible ways: +// At this point CheckSpecialTx() may fail by 2 possible ways:
153-159: Simplify redundant condition check.The condition
fCheckCbTxMerkleRoots && block.vtx[0]->nType == TRANSACTION_COINBASEis redundant sinceopt_cbTxonly has a value when both conditions are already true.-if (fCheckCbTxMerkleRoots && block.vtx[0]->nType == TRANSACTION_COINBASE) { +if (opt_cbTx.has_value()) { if (!CheckCreditPoolDiffForBlock(block, pindex, *opt_cbTx, state)) { return error("CSpecialTxProcessor: CheckCreditPoolDiffForBlock for block %s failed with %s", pindex->GetBlockHash().ToString(), state.ToString()); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/evo/cbtx.cpp(5 hunks)src/evo/cbtx.h(1 hunks)src/evo/specialtxman.cpp(5 hunks)src/evo/specialtxman.h(2 hunks)src/validation.cpp(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/validation.cpp (2)
src/evo/specialtxman.h (3)
block(68-69)block(71-71)block(75-76)src/validation.h (20)
block(682-682)block(683-683)block(714-714)block(755-755)block(881-885)block(1016-1016)pindex(708-708)pindex(753-753)pindex(757-757)state(185-187)state(185-185)state(646-649)state(673-675)state(686-686)state(693-693)state(698-698)state(703-703)state(750-750)state(751-751)state(760-760)
src/evo/specialtxman.cpp (4)
src/evo/specialtxman.h (4)
tx(66-66)block(68-69)block(71-71)block(75-76)src/evo/cbtx.h (3)
CheckCbTx(67-67)CheckCbTxMerkleRoots(69-71)CheckCbTxBestChainlock(77-78)src/evo/cbtx.cpp (6)
CheckCbTx(22-45)CheckCbTx(22-22)CheckCbTxMerkleRoots(48-88)CheckCbTxMerkleRoots(48-50)CheckCbTxBestChainlock(290-352)CheckCbTxBestChainlock(290-291)src/validation.cpp (8)
pindex(2063-2063)pindex(2063-2063)pindex(2067-2073)pindex(2067-2067)pindex(2991-2998)pindex(2991-2991)GetBlockSubsidy(1429-1434)GetBlockSubsidy(1429-1429)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build container / Build container
- GitHub Check: Build slim container / Build container
🔇 Additional comments (11)
src/validation.cpp (2)
2506-2508: Timing update looks correctThe new
nTime5_3checkpoint is taken immediately afterIsBlockValueValid()and the cumulativenTimeValueValidis updated and logged. Variable names and math are consistent with surrounding timing code.
2515-2517: Consistent follow-up timerSame pattern applies here for
nTimePayeeValid; implementation matches adjacent benchmark blocks.src/evo/specialtxman.h (2)
16-16: LGTM!Good addition of the forward declaration for
CCbTxto support the updated method signature.
74-76: LGTM! Signature change aligns with the optimization goal.The change from
CAmount blockSubsidytoconst CCbTx& cbTxparameter is appropriate as it allows the function to access thecreditPoolBalancedirectly from the already-parsed coinbase transaction payload, avoiding redundant parsing.src/evo/cbtx.h (1)
67-78: LGTM! API changes support single-parse optimization.The signature changes for
CheckCbTx,CheckCbTxMerkleRoots, andCheckCbTxBestChainlockto acceptconst CCbTx&directly are well-designed. This eliminates redundant payload extraction in each function and aligns perfectly with the PR's performance optimization goal.src/evo/cbtx.cpp (1)
22-45: LGTM! Clean implementation of the single-parse optimization.The removal of CCbTx extraction logic from
CheckCbTx,CheckCbTxMerkleRoots, andCheckCbTxBestChainlockfunctions successfully eliminates redundant parsing operations. The functions now focus solely on validation logic, improving both performance and code clarity.Also applies to: 48-88, 290-352
src/evo/specialtxman.cpp (5)
50-59: LGTM! Proper validation of coinbase transactions.The addition of the
TRANSACTION_COINBASEcase correctly validates that the transaction is indeed a coinbase and properly extracts the CCbTx payload before validation.
101-125: LGTM! Core optimization implemented correctly.This is the key performance improvement - extracting the CCbTx payload once at the beginning of block processing when merkle root checks are enabled. The implementation includes proper validation and error handling.
195-213: LGTM! Efficient reuse of parsed CCbTx.The code correctly reuses the pre-parsed CCbTx for both merkle root and chainlock validation, achieving the optimization goal. Proper optional value checking ensures safety.
282-283: Verify the deployment check change from DIP0003 to DIP0008.The deployment check has been changed from DIP0003 to DIP0008. While this appears intentional, please confirm this aligns with the expected activation requirements for credit pool validation.
277-291: LGTM! Clean implementation of signature change.The function now correctly:
- Accepts the pre-parsed CCbTx object
- Retrieves the block subsidy internally
- Uses
cbTx.creditPoolBalancefor validationThis change successfully eliminates redundant CCbTx parsing while maintaining the same validation logic.
0c7cfd9 refactor: make CheckCreditPoolDiffForBlock private and tidy up it (Konstantin Akimov) a7fe377 refactor: bench time is properly aligned, variables properly renamed (Konstantin Akimov) ebb9dc0 perf: parse CCbTx once during block validation for Credit Poool (Konstantin Akimov) 59c2407 refactor: remove ProcessSpecialTx and UndoSpecialTx which do nothing (Konstantin Akimov) 60bf96d perf: exclude parsing CCbTx if flag fCheckCbTxMerkleRoots is not set (Konstantin Akimov) b911929 perf: parse CCbTx once during block validation (check CbTx) (Konstantin Akimov) d4f4173 perf: parse CCbTx once during block validation (merkle roots and CL) (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Coinbase Transaction (CCbTx) is parsed several times: - itself validation (during CheckCbTx) - for merkle roots (during CheckCbTxMerkleRoots) - for CL validation (during CheckCbTxBestChainlock) - for Credit Pool (during CheckCreditPoolDiffForBlock) For pre-v20 blocks this parsing has been relatively cheap, but once V20 has been activated, each CbTx has CL signature and its deserialization become expensive due to heavy call of `CBLSWrapper<bls::G2Element, 96ul, CBLSSignature>::Unserialize<CDataStream>(CDataStream&, bool)`. ## What was done? CCbTx is parsed only once and reused for all related checks. This PR makes validation of blocks after v20 activation for ~6% faster. Validation of pre-v20 block is expected to be improved insignificantly (<1% of total time). ## How Has This Been Tested? Invalidated + reconsidered ~8.5k blocks (~2 weeks) after v20 activation. Develop: <img width="704" alt="image" src="https://github.com/user-attachments/assets/08b63507-4cbf-4e2c-917e-1287ff17210d" /> ``` [bench] - Loop: 0.22ms [10.01s] [bench] - GetTxPayload: 0.21ms [2.16s] [bench] - CheckCbTxMerkleRoots: 3.23ms [28.46s] [bench] - CheckCbTxBestChainlock: 3.16ms [24.65s] [bench] - ProcessSpecialTxsInBlock: 8.15ms [101.19s (11.66ms/blk)] [bench] - CheckCreditPoolDiffForBlock: 0.22ms [2.98s (0.34ms/blk)] [bench] - Dash specific: 0.42ms [4.98s (0.57ms/blk)] [bench] - Connect total: 8.87ms [113.20s (13.04ms/blk)] [bench] - Connect block: 9.26ms [116.51s (13.42ms/blk)] ``` PR: <img width="704" alt="image" src="https://github.com/user-attachments/assets/697d9b73-83f4-419e-9ecc-fa9c24e1eafc" /> ``` [bench] - GetTxPayload: 0.24ms [2.34s] [bench] - Loop: 0.02ms [7.89s] [bench] - CheckCreditPoolDiffForBlock: 0.01ms [0.76s] [bench] - CheckCbTxMerkleRoots: 3.17ms [26.50s] [bench] - CheckCbTxBestChainlock: 3.08ms [22.77s] [bench] - ProcessSpecialTxsInBlock: 8.02ms [98.70s (11.37ms/blk)] [bench] - Dash specific: 0.27ms [1.97s (0.23ms/blk)] [bench] - Connect total: 8.58ms [107.65s (12.40ms/blk)] [bench] - Connect block: 8.92ms [110.97s (12.78ms/blk)] ``` Some insignificant lines (not relevant to PR) are removed from both screenshots and bench logs, only affected functions are shown. ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [x] 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 0c7cfd9 PastaPastaPasta: utACK 0c7cfd9 Tree-SHA512: b684f8b3073870ac40cb4de19066e9424307e2c39c753e0bc3c584d491c8dbc1634a35b63fe906acfe03bf9e1b78f0642cee4c0c3715533570ea6693fa4767ea
0c7cfd9 refactor: make CheckCreditPoolDiffForBlock private and tidy up it (Konstantin Akimov) a7fe377 refactor: bench time is properly aligned, variables properly renamed (Konstantin Akimov) ebb9dc0 perf: parse CCbTx once during block validation for Credit Poool (Konstantin Akimov) 59c2407 refactor: remove ProcessSpecialTx and UndoSpecialTx which do nothing (Konstantin Akimov) 60bf96d perf: exclude parsing CCbTx if flag fCheckCbTxMerkleRoots is not set (Konstantin Akimov) b911929 perf: parse CCbTx once during block validation (check CbTx) (Konstantin Akimov) d4f4173 perf: parse CCbTx once during block validation (merkle roots and CL) (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Coinbase Transaction (CCbTx) is parsed several times: - itself validation (during CheckCbTx) - for merkle roots (during CheckCbTxMerkleRoots) - for CL validation (during CheckCbTxBestChainlock) - for Credit Pool (during CheckCreditPoolDiffForBlock) For pre-v20 blocks this parsing has been relatively cheap, but once V20 has been activated, each CbTx has CL signature and its deserialization become expensive due to heavy call of `CBLSWrapper<bls::G2Element, 96ul, CBLSSignature>::Unserialize<CDataStream>(CDataStream&, bool)`. ## What was done? CCbTx is parsed only once and reused for all related checks. This PR makes validation of blocks after v20 activation for ~6% faster. Validation of pre-v20 block is expected to be improved insignificantly (<1% of total time). ## How Has This Been Tested? Invalidated + reconsidered ~8.5k blocks (~2 weeks) after v20 activation. Develop: <img width="704" alt="image" src="https://github.com/user-attachments/assets/08b63507-4cbf-4e2c-917e-1287ff17210d" /> ``` [bench] - Loop: 0.22ms [10.01s] [bench] - GetTxPayload: 0.21ms [2.16s] [bench] - CheckCbTxMerkleRoots: 3.23ms [28.46s] [bench] - CheckCbTxBestChainlock: 3.16ms [24.65s] [bench] - ProcessSpecialTxsInBlock: 8.15ms [101.19s (11.66ms/blk)] [bench] - CheckCreditPoolDiffForBlock: 0.22ms [2.98s (0.34ms/blk)] [bench] - Dash specific: 0.42ms [4.98s (0.57ms/blk)] [bench] - Connect total: 8.87ms [113.20s (13.04ms/blk)] [bench] - Connect block: 9.26ms [116.51s (13.42ms/blk)] ``` PR: <img width="704" alt="image" src="https://github.com/user-attachments/assets/697d9b73-83f4-419e-9ecc-fa9c24e1eafc" /> ``` [bench] - GetTxPayload: 0.24ms [2.34s] [bench] - Loop: 0.02ms [7.89s] [bench] - CheckCreditPoolDiffForBlock: 0.01ms [0.76s] [bench] - CheckCbTxMerkleRoots: 3.17ms [26.50s] [bench] - CheckCbTxBestChainlock: 3.08ms [22.77s] [bench] - ProcessSpecialTxsInBlock: 8.02ms [98.70s (11.37ms/blk)] [bench] - Dash specific: 0.27ms [1.97s (0.23ms/blk)] [bench] - Connect total: 8.58ms [107.65s (12.40ms/blk)] [bench] - Connect block: 8.92ms [110.97s (12.78ms/blk)] ``` Some insignificant lines (not relevant to PR) are removed from both screenshots and bench logs, only affected functions are shown. ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [x] 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 0c7cfd9 PastaPastaPasta: utACK 0c7cfd9 Tree-SHA512: b684f8b3073870ac40cb4de19066e9424307e2c39c753e0bc3c584d491c8dbc1634a35b63fe906acfe03bf9e1b78f0642cee4c0c3715533570ea6693fa4767ea
3 participants
Issue being fixed or feature implemented
Coinbase Transaction (CCbTx) is parsed several times:
For pre-v20 blocks this parsing has been relatively cheap, but once V20 has been activated, each CbTx has CL signature and its deserialization become expensive due to heavy call of
CBLSWrapper<bls::G2Element, 96ul, CBLSSignature>::Unserialize<CDataStream>(CDataStream&, bool).What was done?
CCbTx is parsed only once and reused for all related checks.
This PR makes validation of blocks after v20 activation for ~6% faster.
Validation of pre-v20 block is expected to be improved insignificantly (<1% of total time).
How Has This Been Tested?
Invalidated + reconsidered ~8.5k blocks (~2 weeks) after v20 activation.
Develop:
PR:
Some insignificant lines (not relevant to PR) are removed from both screenshots and bench logs, only affected functions are shown.
Breaking Changes
N/A
Checklist: