Evo: speed up early reindex merkle checks#1864
Closed
navidR wants to merge 1 commit into
Closed
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (5)
Summary by CodeRabbitRelease Notes
WalkthroughThe PR extends EVO coinbase transaction (CbTx) Merkle root validation with multi-level caching and deterministic MN list awareness. New change-detection methods ( ChangesCbTx Merkle Root Caching with Deterministic MN List
DIP3 Invalidation Regression Test Suite
Supporting Infrastructure Changes
Sequence DiagramsequenceDiagram
participant ProcessSpecialTxsInBlock
participant CDeterministicMNManager
participant CheckCbTxMerkleRoots
participant CalcCbTxMerkleRootMNList
ProcessSpecialTxsInBlock->>CDeterministicMNManager: ProcessBlock(block, pindex, state, fJustCheck, &newList, &changed)
CDeterministicMNManager-->>ProcessSpecialTxsInBlock: newList (CDeterministicMNList), changed (bool)
ProcessSpecialTxsInBlock->>CheckCbTxMerkleRoots: (block, pindex, state, &newList, changed)
CheckCbTxMerkleRoots->>CalcCbTxMerkleRootMNList: (newList, merkleRootRet, pindexPrev, changed)
Note over CalcCbTxMerkleRootMNList: Check cache: changed flag + block hash<br/>→ HasSameMNMap<br/>→ SimplifiedMNListsEqual<br/>→ compute & cache
CalcCbTxMerkleRootMNList-->>CheckCbTxMerkleRoots: merkle root
CheckCbTxMerkleRoots-->>ProcessSpecialTxsInBlock: validation result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/validation.cpp (1)
578-578: ⚡ Quick winReuse
hashTxin the trace log.
CheckTransactionalready receives the txid ashashTx; recomputingtx.GetHash()here adds avoidable work on a hot path when validation tracing is enabled.♻️ Proposed fix
- LogPrint("validation", "CheckTransaction nHeight=%d, isVerifyDB=%d, isCheckWallet=%d, txHash=%s\n", nHeight, (int)isVerifyDB, (int)isCheckWallet, tx.GetHash().ToString()); + LogPrint("validation", "CheckTransaction nHeight=%d, isVerifyDB=%d, isCheckWallet=%d, txHash=%s\n", nHeight, (int)isVerifyDB, (int)isCheckWallet, hashTx.ToString());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/validation.cpp` at line 578, The LogPrint call in the CheckTransaction function is unnecessarily recomputing the transaction hash by calling tx.GetHash().ToString() when the function already receives this value as the hashTx parameter. Replace the tx.GetHash().ToString() call with the hashTx parameter in the LogPrint statement to eliminate redundant work on this hot validation path.src/evo/cbtx.cpp (1)
148-199: 💤 Low valueWell-designed three-level caching strategy.
The multi-level caching (change flag + block hash, mnMap identity, simplified list equality) is sound and thread-safe under the existing lock. Consider adding a brief comment block before line 159 summarizing the three cache levels for future maintainers, as this logic is subtle and critical to the optimization.
📝 Suggested documentation
static bool hasCached{false}; + // Three-level cache to avoid recomputing merkle root: + // 1. If cbTxMerkleRootMNListChanged==false and building on cached block, reuse merkle root + // 2. If mnMap identity matches (structural sharing), reuse merkle root + // 3. If simplified MN lists are equal, reuse merkle root if (!cbTxMerkleRootMNListChanged && pindexPrev && hasCached && deterministicMNListCached.GetBlockHash() == pindexPrev->GetBlockHash()) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/evo/cbtx.cpp` around lines 148 - 199, Add a comment block before the first cache check in the CalcCbTxMerkleRootMNList function (before the if statement checking cbTxMerkleRootMNListChanged) that explains the three-level caching strategy. The comment should briefly describe: the first cache level that uses the change flag and block hash comparison, the second cache level that uses MN map identity checking via HasSameMNMap, and the third cache level that uses simplified list equality comparison via SimplifiedMNListsEqual. This will help future maintainers understand the subtle optimization logic without needing to reverse-engineer the intent from the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/evo/cbtx.cpp`:
- Around line 148-199: Add a comment block before the first cache check in the
CalcCbTxMerkleRootMNList function (before the if statement checking
cbTxMerkleRootMNListChanged) that explains the three-level caching strategy. The
comment should briefly describe: the first cache level that uses the change flag
and block hash comparison, the second cache level that uses MN map identity
checking via HasSameMNMap, and the third cache level that uses simplified list
equality comparison via SimplifiedMNListsEqual. This will help future
maintainers understand the subtle optimization logic without needing to
reverse-engineer the intent from the code.
In `@src/validation.cpp`:
- Line 578: The LogPrint call in the CheckTransaction function is unnecessarily
recomputing the transaction hash by calling tx.GetHash().ToString() when the
function already receives this value as the hashTx parameter. Replace the
tx.GetHash().ToString() call with the hashTx parameter in the LogPrint statement
to eliminate redundant work on this hot validation path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 108ff02a-c859-4f6b-a785-8ef0f0fff979
📒 Files selected for processing (7)
src/evo/cbtx.cppsrc/evo/cbtx.hsrc/evo/deterministicmns.cppsrc/evo/deterministicmns.hsrc/evo/specialtx.cppsrc/test/evo_simplifiedmns_tests.cppsrc/validation.cpp
Contributor
Author
|
navidR
force-pushed
the
dev/navidr/reindex-early-speedup
branch
from
58e77a6 to
155b6d9
Compare
coderabbitai
Bot
added
the
size:S
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This patch speeds up early -reindex around the DIP3 deterministic masternode activation window by avoiding redundant cbTx masternode-list merkle-root rebuilds. During ConnectBlock, Firo already builds the deterministic masternode list and diff for the block; the patch reuses that freshly computed list for cbTx merkle validation, and if the diff only changes fields that are not serialized into CSimplifiedMNListEntry, it safely reuses the previous merkle root instead of rebuilding and rehashing the same effective simplified MN list. Consensus behavior is preserved because additions, removals, and every state field that affects the cbTx MN merkle root still force recomputation, while unrelated fields such as payout/last-paid metadata do not. It also gates several high-volume validation trace logs behind -debug=validation, reducing reindex log I/O without removing opt-in diagnostics.