perf: cache mined commitment for quorum merkle root calculation #6756

knst · 2025-07-15T08:18:34Z

Issue being fixed or feature implemented

Every time when list of quorum is changed, the quorum final commitments for multiple active quorums are re-read from database.
It's a heavy operation in a current implementation due to heavy database reading, heavy BLS deserialization and multiple in-memory copies.

What was done?

add cache for active quorums, see llmq::utils::InitQuorumsCache(qc_hashes_cached)
use RVO for GetMinedCommitment, avoid extra allocation by unique_ptr
adjust return value of GetLastMinedCommitmentsPerQuorumIndexUntilBlock to avoid extra data copies and transformations

How Has This Been Tested?

Time of calculation CheckCbTxMerkleRoots is shortened for 40%, and this step is a significant part of block validation.
Expected impact to total time for block validation and re-index is 3-4%.

develop:

[bench]             - CachedGetQcHashesQcIndexedHashes: 1.16ms [5.22s]
[bench]       - CheckCbTxMerkleRoots: 1.30ms [6.04s]

vs PR:

[bench]             - CachedGetQcHashesQcIndexedHashes: 1.20ms [2.65s]
[bench]       - CheckCbTxMerkleRoots: 1.34ms [3.44s]

Breaking Changes

N/A

Checklist:

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
I have assigned this pull request to a milestone

coderabbitai · 2025-07-15T08:21:52Z

Walkthrough

This set of changes refactors and modernizes the handling of quorum commitments and their associated caches throughout the codebase. The main updates include changing the return type of GetMinedCommitment from a pointer with an output parameter to a structured std::pair containing the commitment and its block hash. Related methods that previously returned vectors of pairs now return vectors of block indices only, simplifying data flow. A new static LRU cache is introduced in CachedGetQcHashesQcIndexedHashes to optimize repeated serialization and quorum index extraction. Several functions are updated to use these new return types and caching mechanisms, and explicit template instantiations are added for the new cache type.

Estimated code review effort

3 (~45 minutes)

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a2cc53cc2ee09c808e7b48b2acc46d2fa7ad9f56 and eeb577b.

📒 Files selected for processing (7)

src/evo/cbtx.cpp (4 hunks)
src/evo/smldiff.cpp (2 hunks)
src/llmq/blockprocessor.cpp (4 hunks)
src/llmq/blockprocessor.h (1 hunks)
src/llmq/quorums.cpp (1 hunks)
src/llmq/snapshot.cpp (1 hunks)
src/llmq/utils.cpp (1 hunks)

📓 Path-based instructions (2)

src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit Inference Engine (CLAUDE.md)

src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

src/evo/smldiff.cpp

src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit Inference Engine (CLAUDE.md)

Masternode lists must use immutable data structures (Immer library) for thread safety

Files:

src/evo/smldiff.cpp

🧠 Learnings (2)

📓 Common learnings

Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.

Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.

src/evo/smldiff.cpp (6)

Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety

Learnt from: kwvg
PR: #6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.

Learnt from: knst
PR: #6511
File: src/evo/deterministicmns.cpp:1369-1373
Timestamp: 2025-01-07T18:50:44.838Z
Learning: The functions MigrateDBIfNeeded and MigrateDBIfNeeded2 in src/evo/deterministicmns.cpp are temporary and will be removed in a future version. Refactoring suggestions for these functions should be avoided.

Learnt from: kwvg
PR: #6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.

Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/evo/specialtx.h : Special transactions use payload extensions defined in src/evo/specialtx.h

Learnt from: kwvg
PR: #6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/is_basic_scheme_active=/true, /is_extended_addr=/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.

🚧 Files skipped from review as they are similar to previous changes (6)

src/llmq/quorums.cpp
src/llmq/blockprocessor.h
src/llmq/blockprocessor.cpp
src/llmq/utils.cpp
src/llmq/snapshot.cpp
src/evo/cbtx.cpp

🧰 Additional context used

📓 Path-based instructions (2)

src/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit Inference Engine (CLAUDE.md)

src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction

Files:

src/evo/smldiff.cpp

src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp}

📄 CodeRabbit Inference Engine (CLAUDE.md)

Masternode lists must use immutable data structures (Immer library) for thread safety

Files:

src/evo/smldiff.cpp

🧠 Learnings (2)

📓 Common learnings

Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.

Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.

src/evo/smldiff.cpp (6)

Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/{masternode,evo}/**/*.{cpp,h,cc,cxx,hpp} : Masternode lists must use immutable data structures (Immer library) for thread safety

Learnt from: kwvg
PR: #6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.

Learnt from: knst
PR: #6511
File: src/evo/deterministicmns.cpp:1369-1373
Timestamp: 2025-01-07T18:50:44.838Z
Learning: The functions MigrateDBIfNeeded and MigrateDBIfNeeded2 in src/evo/deterministicmns.cpp are temporary and will be removed in a future version. Refactoring suggestions for these functions should be avoided.

Learnt from: kwvg
PR: #6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.

Learnt from: CR
PR: dashpay/dash#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T18:42:49.794Z
Learning: Applies to src/evo/specialtx.h : Special transactions use payload extensions defined in src/evo/specialtx.h

Learnt from: kwvg
PR: #6665
File: src/evo/providertx.h:82-82
Timestamp: 2025-06-06T11:53:09.094Z
Learning: In ProTx serialization code (SERIALIZE_METHODS), version checks should use hardcoded maximum flags (/is_basic_scheme_active=/true, /is_extended_addr=/true) rather than deployment-based flags. This is because serialization code should be able to deserialize any structurally valid ProTx up to the maximum version the code knows how to handle, regardless of current consensus validity. Validation code, not serialization code, is responsible for checking whether a ProTx version is consensus-valid based on deployment status.

🔇 Additional comments (3)

src/evo/smldiff.cpp (3)

59-59: Excellent modernization using structured binding.

The change from pointer/output parameter to structured binding with std::pair return is a significant improvement:

Eliminates pointer null-checking complexities

Makes the return values explicit and type-safe

Uses modern C++17 structured binding syntax appropriately

60-63: Correct null check adaptation and efficient use of move semantics.

The changes properly adapt to the new return type:

Checking minedBlockHash == uint256::ZERO is the appropriate equivalent to the previous null pointer check

Using std::move(qc) avoids unnecessary copying of commitment objects, improving performance

Error handling logic is preserved correctly

25-26: Forward declaration signature confirmed correct

The forward declaration in src/evo/smldiff.cpp exactly matches the signature in:

src/llmq/utils.cpp

src/evo/cbtx.h

src/evo/cbtx.cpp

Removing the full include and using this forward declaration is safe. No further changes needed.

✨ Finishing Touches

📝 Generate Docstrings

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.

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Explain this complex logic.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai explain this code block.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and explain its main purpose.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Support

Need 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)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR.
@coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

github-actions · 2025-07-22T14:49:05Z

This pull request has conflicts, please rebase.

…terface

…lock and related code

… RVO CFinalCommitment is a heavy object to create and copy

…ommitment

github-actions · 2025-07-22T15:11:44Z

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

UdjinM6

utACK eeb577b

PastaPastaPasta

utACK

knst added this to the 23 milestone Jul 15, 2025

github-actions bot mentioned this pull request Jul 19, 2025

perf: use cache for SML #6738

Merged

5 tasks

github-actions bot added the needs rebase label Jul 22, 2025

knst added 7 commits July 22, 2025 22:09

refactor: simplify GetLastMinedCommitmentsPerQuorumIndexUntilBlock in…

40f3d98

…terface

refactor: futher simplification of GetMinedAndActiveCommitmentsUntilB…

6d90189

…lock and related code

perf: replace re-creation of CFinalCommitment and extra unique_ptr to…

d78fb02

… RVO CFinalCommitment is a heavy object to create and copy

refactor: avoid code duplication due to 2 implementation of GetMinedC…

f2b2479

…ommitment

perf: cache mined commitment for quorum merkle root calculation

8958152

refactor: resolve freshly introduced circular dependency

8254e06

perf: swap arrays instead assigments to save allocation

eeb577b

knst force-pushed the perf-qc-mined branch from a2cc53c to eeb577b Compare July 22, 2025 15:11

github-actions bot removed the needs rebase label Jul 22, 2025

knst requested review from PastaPastaPasta and UdjinM6 July 24, 2025 12:46

This was referenced Jul 24, 2025

perf: using asynchronous worker to validate BLS signatures in quorum commitments #6692

Merged

backport: trivial batch 380 #6779

Merged

UdjinM6 approved these changes Aug 15, 2025

View reviewed changes

PastaPastaPasta approved these changes Aug 19, 2025

View reviewed changes

PastaPastaPasta merged commit cc9da5d into dashpay:develop Aug 19, 2025
35 of 37 checks passed

knst deleted the perf-qc-mined branch August 19, 2025 10:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

perf: cache mined commitment for quorum merkle root calculation #6756

perf: cache mined commitment for quorum merkle root calculation #6756

Uh oh!

knst commented Jul 15, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Jul 15, 2025 •

edited

Loading

Chat

Support

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

Uh oh!

github-actions bot commented Jul 22, 2025

Uh oh!

github-actions bot commented Jul 22, 2025

Uh oh!

UdjinM6 left a comment

Uh oh!

PastaPastaPasta left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

perf: cache mined commitment for quorum merkle root calculation #6756

perf: cache mined commitment for quorum merkle root calculation #6756

Uh oh!

Conversation

knst commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

coderabbitai bot commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Estimated code review effort

Chat

Support

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

Uh oh!

github-actions bot commented Jul 22, 2025

Uh oh!

github-actions bot commented Jul 22, 2025

✅ No Merge Conflicts Detected

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

knst commented Jul 15, 2025 •

edited

Loading

coderabbitai bot commented Jul 15, 2025 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)