Evo: speed up early reindex merkle checks#1864
Open
navidR wants to merge 1 commit into
Open
Conversation
Contributor
Summary by CodeRabbitRelease Notes
WalkthroughThe PR extends deterministic MN outputs into CbTx merkle-root validation, adds change-aware caching for MN-list root calculation, expands invalidation regression coverage, and switches several validation logs to the ChangesCbTx Deterministic MN Root Pipeline
Validation Categorized Logging
Estimated code review effort: 4 (Complex) | ~50 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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
navidR
force-pushed
the
dev/navidr/reindex-early-speedup
branch
from
155b6d9 to
c0db1be
Compare
|
User rahimi.nv@gmail.com does not have a PR Review subscription. Go to Team management and add this email to the PR Review subscription. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/evo/cbtx.h (1)
49-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new parameter contract with Doxygen comments.
The
deterministicMNListpointer andcbTxMerkleRootMNListChangedflag carry non-obvious preconditions (per the implementation incbtx.cpp, the pointer is only used if its height/block hash matchpindex/block, otherwise callers silently fall back to a full rebuild; the flag controls whether cached merkle-root values are reused). None of this is documented at the declaration site, making misuse easy for future callers.📝 Suggested Doxygen documentation
+/** + * `@param` deterministicMNList Optional pre-computed MN list for `pindex`. Only used if its height and + * block hash match `pindex`/`block`; otherwise the MN list is rebuilt from scratch. + * `@param` cbTxMerkleRootMNListChanged Set to false only when the caller has verified the cbTx-relevant + * MN-list fields are unchanged since the previous block, allowing the merkle root to be reused + * from cache. Defaults to true (always recompute) for safety. + */ bool CheckCbTxMerkleRoots(const CBlock& block, const CBlockIndex* pindex, CValidationState& state, const CDeterministicMNList* deterministicMNList = nullptr, bool cbTxMerkleRootMNListChanged = true); bool CalcCbTxMerkleRootMNList(const CBlock& block, const CBlockIndex* pindexPrev, uint256& merkleRootRet, CValidationState& state); +/** + * `@param` cbTxMerkleRootMNListChanged See CheckCbTxMerkleRoots. Must be false only when the caller can + * guarantee no cbTx-relevant change occurred relative to `pindexPrev`. + */ bool CalcCbTxMerkleRootMNList(const CDeterministicMNList& deterministicMNList, uint256& merkleRootRet, const CBlockIndex* pindexPrev = nullptr, bool cbTxMerkleRootMNListChanged = true);As per coding guidelines, "Prefer Doxygen-compatible comments for non-obvious public interfaces" for
**/*.{h,hpp}files.🤖 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.h` around lines 49 - 51, Add Doxygen-compatible comments to the cbtx.h declarations for CheckCbTxMerkleRoots and CalcCbTxMerkleRootMNList to document the new parameter contract. Explain the intended use and preconditions for deterministicMNList and cbTxMerkleRootMNListChanged, including when the pointer is honored versus ignored and when cached merkle-root values may be reused. Keep the comments at the declaration site so callers can understand the behavior without reading cbtx.cpp.Source: Coding guidelines
🤖 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.
Inline comments:
In `@src/evo/cbtx.cpp`:
- Around line 177-184: The smlCached comparison in the cache-hit path needs to
be guarded by hasCached like the earlier cache checks, because the current
SimplifiedMNListsEqual(sml, smlCached) branch can succeed on the first call with
default-constructed cache state. Update the existing cache logic around
smlCached, deterministicMNListCached, merkleRootCached, and mutatedCached so the
equality check only runs when hasCached is true, keeping the invariant explicit
and avoiding accidental reliance on default values.
---
Nitpick comments:
In `@src/evo/cbtx.h`:
- Around line 49-51: Add Doxygen-compatible comments to the cbtx.h declarations
for CheckCbTxMerkleRoots and CalcCbTxMerkleRootMNList to document the new
parameter contract. Explain the intended use and preconditions for
deterministicMNList and cbTxMerkleRootMNListChanged, including when the pointer
is honored versus ignored and when cached merkle-root values may be reused. Keep
the comments at the declaration site so callers can understand the behavior
without reading cbtx.cpp.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 55711e21-8912-4391-9422-cc4a80a74f5a
📒 Files selected for processing (12)
qa/rpc-tests/dip3-deterministicmns.pysrc/dsnotificationinterface.cppsrc/evo/cbtx.cppsrc/evo/cbtx.hsrc/evo/deterministicmns.cppsrc/evo/deterministicmns.hsrc/evo/specialtx.cppsrc/llmq/quorums_dkgsession.cppsrc/llmq/quorums_dkgsessionhandler.cppsrc/test/evo_deterministicmns_tests.cppsrc/test/evo_simplifiedmns_tests.cppsrc/validation.cpp
✅ Files skipped from review due to trivial changes (1)
- src/validation.cpp
🚧 Files skipped from review as they are similar to previous changes (6)
- src/dsnotificationinterface.cpp
- src/evo/specialtx.cpp
- qa/rpc-tests/dip3-deterministicmns.py
- src/evo/deterministicmns.h
- src/evo/deterministicmns.cpp
- src/test/evo_deterministicmns_tests.cpp
Comment on lines
+177
to
184
| static CSimplifiedMNList smlCached; | ||
|
|
||
| if (sml.mnList == smlCached.mnList) { | ||
| if (SimplifiedMNListsEqual(sml, smlCached)) { | ||
| deterministicMNListCached = deterministicMNList; | ||
| hasCached = true; | ||
| merkleRootRet = merkleRootCached; | ||
| return !mutatedCached; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard the smlCached comparison with hasCached.
Unlike the two earlier cache checks, this branch does not verify hasCached. On the first invocation smlCached is default-constructed (empty); if sml is also empty the comparison succeeds and returns the default merkleRootCached/mutatedCached. It is correct only by coincidence (empty-list root is null). Add the guard to keep the invariant explicit and robust to future default changes.
🛡️ Proposed guard
- if (SimplifiedMNListsEqual(sml, smlCached)) {
+ if (hasCached && SimplifiedMNListsEqual(sml, smlCached)) {📝 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
| static CSimplifiedMNList smlCached; | |
| if (sml.mnList == smlCached.mnList) { | |
| if (SimplifiedMNListsEqual(sml, smlCached)) { | |
| deterministicMNListCached = deterministicMNList; | |
| hasCached = true; | |
| merkleRootRet = merkleRootCached; | |
| return !mutatedCached; | |
| } | |
| static CSimplifiedMNList smlCached; | |
| if (hasCached && SimplifiedMNListsEqual(sml, smlCached)) { | |
| deterministicMNListCached = deterministicMNList; | |
| hasCached = true; | |
| merkleRootRet = merkleRootCached; | |
| return !mutatedCached; | |
| } |
🤖 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 177 - 184, The smlCached comparison in the
cache-hit path needs to be guarded by hasCached like the earlier cache checks,
because the current SimplifiedMNListsEqual(sml, smlCached) branch can succeed on
the first call with default-constructed cache state. Update the existing cache
logic around smlCached, deterministicMNListCached, merkleRootCached, and
mutatedCached so the equality check only runs when hasCached is true, keeping
the invariant explicit and avoiding accidental reliance on default values.
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.