[knst]

Issue being fixed or feature implemented

Building a premature commitment for llmq_400_85 takes about 5 seconds of single-threaded work, almost all of it avoidable. SendCommitment and the verification of every received premature commitment call CDKGSessionManager::GetVerifiedContributions, which reads each member's verification vector back from LevelDB and decodes it: 400 members x 340 BLS public keys, about 34 µs per key (25 µs subgroup check, 8 µs point decompression). The contributions cache was filled only by reads and expired after 60 seconds, so it was always cold at commit time, although the DKG session still held the same vectors in memory.

#8130 reports that this delay made members miss short commit phases on mainnet, and fixes it. This PR is a smaller alternative to the cache part of #8130.

What was done?

• WriteVerifiedVvecContribution now also puts the vector into contributionsCache, sharing the pointer the session already holds (no copy).
• WriteVerifiedSkContribution updates the cached entry, so a share verified or justified later is visible to the next lookup.
• CleanupCache drops an entry after 60 seconds only when nothing else references its vector, so entries live as long as their session and entries loaded from disk expire as before.
• Benchmarks for the commit step at 400 members / threshold 340: BLSDKG_BuildCommitment_cold_400, BLSDKG_BuildCommitment_cached_400, BLSDKG_VerifyCommitment_400.

The database writes and the cold-read path are unchanged, so restarts, watchers and later quorum builds behave as before.

Compared to #8130

#8130 has the same idea but is +580 −9 across 7 files. This PR is +88 −6 across 3 files and reaches the same speed.

One behaviour difference from #8130: under the legacy BLS scheme (before v19) a received vector is cached as decoded, without the subgroup check that the database read applies. #8130 falls back to the database read in that case. There's no reason to keep different behaviour for 2 scenarios.

How Has This Been Tested?

Measurements

x86_64, blst, release build, 400 members / threshold 340:

Benchmark	Time
BLSDKG_BuildCommitment_cold_400 (decode from database encoding first)	5.28 s
BLSDKG_BuildCommitment_cached_400	0.09 s
BLSDKG_VerifyCommitment_400 (per received commitment)	0.018 s

Through the real CDKGSessionManager, using the end-to-end benchmark from #8130: 5.03 s on develop, 0.20 s with this PR, 0.22 s on #8130, same commitment hash in all three.

Breaking Changes

None. No protocol, message, consensus or on-disk change.

Checklist:

• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone (if you have permission to set one)

[thepastaclaw]

🔍 Review in progress — actively reviewing now (commit 898351d) · triage: low
██████████████████░░ 92% · taking longer than usual · running for 41 min
✅ triage → ⏳ Phase 1 (0/1 lanes) → ▫️ verify 1 → ▫️ gate → ▫️ Phase 2 → ▫️ verify 2 → ▫️ publish
_{Estimated from recent reviews of this tier · updated 08:58 UTC · live progress}

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

The DKG benchmark code now supports an explicit threshold and includes benchmarks for commitment building and verification. The session manager now caches verified vectors, updates cached secret-key contributions, and retains expired entries while they have references outside the cache.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• dashpay/dash#8130: It changes caching and expiry behavior for verified DKG contributions in the same session-manager code path.

Merge Risk: Minimal · up to 89835

The change keeps verified DKG contribution vectors cached through the commit phase to avoid repeated database decoding, and no concrete behavioral defect was established. The remaining items are the PR title scope and milestone checks, which should be fixed before merge. A legacy-scheme (pre-V19) edge case was not fully traced, but the evidence gathered does not indicate a failure.

Pre-merge checks: 4

Check name	Status	Explanation
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Title check		The title "perf: keep verified DKG contributions cached through the commit phase" matches the main change. The PR caches verified DKG contribution vectors in contributionsCache so that commitment buil…
Description check		The description is specific to the changeset. It explains that the DKG contributions cache is filled only by reads and expires after 60 seconds, so commitment building reads and decodes vectors from L…

Full details: Title check

Explanation

The title "perf: keep verified DKG contributions cached through the commit phase" matches the main change. The PR caches verified DKG contribution vectors in contributionsCache so that commitment building and verification do not reload and decode them from LevelDB. The title is short, specific, and states the primary performance goal. It does not cover the benchmarks or the cleanup rule, which is acceptable.

Full details: Description check

Explanation

The description is specific to the changeset. It explains that the DKG contributions cache is filled only by reads and expires after 60 seconds, so commitment building reads and decodes vectors from LevelDB. It lists the changes to WriteVerifiedVvecContribution, WriteVerifiedSkContribution and CleanupCache, the three new benchmarks at 400 members and threshold 340, and the measured timings. It also describes the legacy BLS scheme difference from #8130 and states that no protocol, consensus or on-disk changes are made. This matches the summary of changes in dkgsessionmgr.cpp, dkgsessionmgr.h and bls_dkg.cpp.

Finishing Touches

Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

• X
• Mastodon
• Reddit
• LinkedIn

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Repository UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 7b2ce74e-1c06-4ccf-849b-43cc17091671

Commits

Reviewing files that changed from the base of the PR and between 6f5e9cf and 898351d.

Files selected for processing: 3

• src/bench/bls_dkg.cpp
• src/llmq/dkgsessionmgr.cpp
• src/llmq/dkgsessionmgr.h

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.

[PastaPastaPasta]

Tested this for v24.0.1, where it replaces the cache part of #8130.

• Read against the session code: the vector is always written on receipt and the secret-key share only afterwards (VerifyPendingContributions, justifications), so the share update always finds the entry; and an entry can only be evicted once the session no longer holds the vector. On a synced mainnet node the result is the same as a cold read.
• The source commit (898351d) applies to v24.0.x with one conflict, in CleanupCache, where v24.0.x still has the hand-written erase loop; resolved by taking the new std::erase_if.
• On that v24.0.x build (relic, --enable-debug --enable-werror): unit suites llmq*, dkg*, bls*, evo* pass; feature_llmq_dkgerrors.py, feature_llmq_simplepose.py (both variants), feature_llmq_rotation.py, feature_llmq_signing.py (both variants), feature_llmq_data_recovery.py, feature_llmq_connections.py, feature_llmq_chainlocks.py, feature_llmq_evo.py, feature_llmq_singlenode.py and feature_mnehf.py (both variants) pass.

#8128 now carries this commit. The "Require a milestone" check is failing here only because no milestone is set yet.

---

🤖 Posted autonomously by Claude on behalf of pasta.