perf: improve signing latency by collecting lazy signatures under lock by PastaPastaPasta · Pull Request #6911 · dashpay/dash

PastaPastaPasta · 2025-10-21T20:30:13Z

Issue being fixed or feature implemented

see benchmark code here: https://gist.github.com/PastaPastaPasta/01e2e3a22c7dbacf957a2b3222b1457c

ns/sigs	sigs/s	err%	total	benchmark
59,266.86	16,872.84	0.0%	0.76	`LLMQSigShares_LockOccupancy_Inside`
2.43	411,787,414.75	0.0%	0.00	`LLMQSigShares_LockOccupancy_Outside`
60,162.66	16,621.60	0.0%	0.77	`LLMQSigShares_MaterializeInsideLock`
58,093.54	17,213.62	0.0%	0.74	`LLMQSigShares_MaterializeOutsideLock`

this appears to show that calling the .Get inside the lock is very expensive, and that the impact to overall performance may actually improve a few percentage points as well. The main gain here is reducing the time spent inside of signing_shares.cpp cs lock to minimize overall contention.

AI interpretation

Benchmark Interpretation

The benchmarks demonstrate that performing CBLSLazySignature::Get() calls inside the cs lock is extremely expensive, and moving that work outside of the critical section nearly eliminates lock contention:

Metric	Inside Lock	Outside Lock	Improvement
Lock occupancy (ns/sig)	59,266.86	2.43	≈ 24,000× reduction
Total time per sig (ns/sig)	60,162.66	58,093.54	~3–4% faster overall

Interpretation:
Total signing work remains roughly constant, but the time spent holding the lock drops from ~3.8 ms per 64-share recovery to ~0.00015 ms. This drastically reduces contention in signing_shares.cpp, improving throughput and tail latency under concurrent load.

In multi-threaded real-world conditions, this means:
• Fewer stalls on cs
• Faster signature recovery for all participants
• Lower p95/p99 signing latency in high-load LLMQ signing scenarios

What was done?

Collect the pointers inside the lock, perform expensive BLS serialization outside the lock.

How Has This Been Tested?

builds; benched; this isn't super easy to detect in proper integration level benchmarks because only 1 of 15 nodes is the "recovery" member, so contention in this spot kinda gets overshadowed by all the other typical contention over this cs. The data I do have, shows the number of contentions over this lock isn't too bad, but the time spent in contention when there is contention is pretty bad, averaging 500μs (0.5ms).

Breaking Changes

None

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

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 (for repository code-owners and collaborators only)

github-actions · 2025-10-21T20:30:51Z

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

coderabbitai · 2025-10-21T20:34:19Z

Walkthrough

TryRecoverSig in src/llmq/signing_shares.cpp was refactored to reduce the critical-section duration. Under the lock the code now collects CBLSLazySignature wrappers and signer IDs (and an isSingleNode flag) instead of materializing full signatures. After releasing the lock, the lazy signatures are materialized (Get) and used for either single-node or multi-node recovery; the function also returns early if there are insufficient lazy signatures for recovery. Comments were updated to reflect lazy collection and post-lock materialization.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

The changes are localized and follow a consistent pattern (defer expensive BLS/Get work outside the lock) but require careful verification of concurrency semantics, correct handling of single-node vs multi-node branches, and that no race conditions or behavioral regressions were introduced.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The PR title "perf: improve signing latency by collecting lazy signatures under lock" directly aligns with the main change described in the raw summary. The title clearly identifies the type of change (performance improvement), the benefit (improved signing latency), and the key mechanism (collecting lazy signatures under lock). It is specific and descriptive without being verbose, accurately reflecting the core optimization: moving expensive BLS signature materialization outside the critical section to reduce lock contention in signing_shares.cpp.
Description Check	✅ Passed	The PR description is directly related to the changeset and provides substantial context for the optimization. It explains the performance problem with benchmark data, describes the solution approach (collect pointers under lock, materialize outside lock), details the testing methodology, and confirms no breaking changes. The description connects the technical change to the measured performance impact and addresses both the why and how of the modification, making it clear and relevant to the actual code changes in signing_shares.cpp.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 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 1662e976d0734645cddc5b9f789b18cf5c98c919 and 282ca7d.

📒 Files selected for processing (1)

src/llmq/signing_shares.cpp (2 hunks)

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

src/llmq/signing_shares.cpp

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)

GitHub Check: mac-build / Build source
GitHub Check: win64-build / Build source
GitHub Check: linux64_nowallet-build / Build source
GitHub Check: linux64_fuzz-build / Build source
GitHub Check: linux64_ubsan-build / Build source
GitHub Check: linux64-build / Build source
GitHub Check: linux64_sqlite-build / Build source
GitHub Check: linux64_tsan-build / Build source
GitHub Check: arm-linux-build / Build source
GitHub Check: Lint / Run linters

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

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

knst · 2025-10-22T06:57:54Z

src/llmq/signing_shares.cpp

+    std::vector<CBLSSignature> sigSharesForRecovery;
+    sigSharesForRecovery.reserve(lazySignatures.size());
+    for (const auto& lazySig : lazySignatures) {
+        sigSharesForRecovery.emplace_back(lazySig.Get()); // Expensive, but outside lock


Is copy of bls signature indeed that expensive?

Maybe there's a bug in our bls's wrapper which calls "is-valid" or "serialize&deserialize" for each constructor copy?

Just copy of 500bytes are not that expensive; I think we should improve performance of our bls-wrapper rather than do this refactoring... Do I miss anything?

It's not a copy; look into the implementation of .Get()

See:

dash/src/bls/bls.h

Lines 488 to 508 in a488c8d

const BLSObject& Get() const

{

std::unique_lock<std::mutex> l(mutex);

static BLSObject invalidObj;

if (!bufValid && !objInitialized) {

return invalidObj;

}

if (!objInitialized) {

obj.SetBytes(vecBytes, bufLegacyScheme);

if (!obj.IsValid()) {

bufValid = false;

return invalidObj;

}

if (!obj.CheckMalleable(vecBytes, bufLegacyScheme)) {

bufValid = false;

return invalidObj;

}

objInitialized = true;

}

return obj;

}

I'd love ways to increase performance here, but it is quite expensive as is.

github-actions · 2025-10-24T15:31:27Z

This pull request has conflicts, please rebase.

thepastaclaw

Code Review

Sound performance optimization. Both agents correctly confirmed no correctness, race, or consensus-safety issues. The two substantive findings are minor naming/comment clarity nitpicks on newly introduced code.

Reviewed commit: 8922fb1

💬 2 nitpick(s)

thepastaclaw · 2026-03-17T17:18:55Z

src/llmq/signing_shares.cpp

+    // Collect lazy signatures (cheap copy) under lock, then materialize outside lock
+    std::vector<CBLSLazySignature> lazySignatures;
    std::vector<CBLSId> idsForRecovery;
+    bool isSingleNode = false;


💬 Nitpick: isSingleNode should be isSingleMember for consistency

The codebase consistently uses 'member' terminology: is_single_member() (params.h:124), getQuorumMember(), quorum.members. The new variable isSingleNode breaks this convention. The pre-existing log strings ("single-node signature") use 'node' but those predate this PR; the new variable should align with the API it guards (is_single_member()).

💡 Suggested change

Suggested change

bool isSingleNode = false;

bool isSingleMember = false;

// ... (also update line 622: isSingleMember = true; and line 644: if (isSingleMember))

_{source: ['claude']}

thepastaclaw · 2026-03-17T17:18:55Z

src/llmq/signing_shares.cpp

    }

-    std::vector<CBLSSignature> sigSharesForRecovery;
+    // Collect lazy signatures (cheap copy) under lock, then materialize outside lock


💬 Nitpick: "cheap copy" is imprecise — state what it avoids

CBLSLazyWrapper's copy constructor acquires a mutex on the source and deep-copies a 96-byte array plus potentially a full bls::G2Element (if already materialized). It IS much cheaper than Get() on the hot path (unmaterialized objects skip BLS point decompression), but "cheap" without qualification could mislead a reader into thinking it's trivial. The same comment pattern appears at lines 620 and 633.

💡 Suggested change

Suggested change

// Collect lazy signatures (cheap copy) under lock, then materialize outside lock

// Collect lazy signatures (avoids BLS deserialization) under lock, then materialize outside lock

_{source: ['claude']}

UdjinM6 · 2026-03-17T19:10:08Z

src/llmq/signing_shares.cpp

-            idsForRecovery.emplace_back(quorum.members[sigShare.getQuorumMember()]->proTxHash);
-        }
+    // Materialize signatures outside the critical section (expensive BLS operations)
+    if (isSingleNode) {


Suggested change

if (isSingleNode) {

if (quorum.params.is_single_member()) {

and drop isSingleNode

PastaPastaPasta added this to the 23.1 milestone Oct 21, 2025

PastaPastaPasta force-pushed the perf/lazy-sigs-in-lock branch from 1662e97 to 282ca7d Compare October 21, 2025 20:37

knst reviewed Oct 22, 2025

View reviewed changes

github-actions bot mentioned this pull request Oct 22, 2025

refactor: move CSigSharesManager to ActiveContext, drop CActiveMasternodeManager from CSigSharesManager #6841

Merged

5 tasks

github-actions bot added the needs rebase label Oct 24, 2025

This was referenced Nov 25, 2025

feat: proactively push recovered signatures; fully connect instantsend quorums for recovered sigs #6967

Merged

refactor: enhance CSigSharesManager with multi-threaded worker pool and dispatcher #7004

Merged

UdjinM6 removed this from the 23.1 milestone Feb 15, 2026

perf: improve signing latency by collecting lazy signatures under lock

8922fb1

PastaPastaPasta force-pushed the perf/lazy-sigs-in-lock branch from 282ca7d to 8922fb1 Compare March 17, 2026 14:59

github-actions bot removed the needs rebase label Mar 17, 2026

thepastaclaw reviewed Mar 17, 2026

View reviewed changes

UdjinM6 reviewed Mar 17, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: improve signing latency by collecting lazy signatures under lock#6911

perf: improve signing latency by collecting lazy signatures under lock#6911
PastaPastaPasta wants to merge 1 commit intodashpay:developfrom
PastaPastaPasta:perf/lazy-sigs-in-lock

PastaPastaPasta commented Oct 21, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Oct 21, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Oct 21, 2025 •

edited

Loading

Uh oh!

knst Oct 22, 2025

Uh oh!

PastaPastaPasta Oct 22, 2025

Uh oh!

PastaPastaPasta Oct 22, 2025

Uh oh!

github-actions bot commented Oct 24, 2025

Uh oh!

thepastaclaw left a comment

Uh oh!

thepastaclaw Mar 17, 2026

Uh oh!

thepastaclaw Mar 17, 2026

Uh oh!

UdjinM6 Mar 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	const BLSObject& Get() const
	{
	std::unique_lock<std::mutex> l(mutex);
	static BLSObject invalidObj;
	if (!bufValid && !objInitialized) {
	return invalidObj;
	}
	if (!objInitialized) {
	obj.SetBytes(vecBytes, bufLegacyScheme);
	if (!obj.IsValid()) {
	bufValid = false;
	return invalidObj;
	}
	if (!obj.CheckMalleable(vecBytes, bufLegacyScheme)) {
	bufValid = false;
	return invalidObj;
	}
	objInitialized = true;
	}
	return obj;
	}

	bool isSingleNode = false;
	bool isSingleMember = false;
	// ... (also update line 622: isSingleMember = true; and line 644: if (isSingleMember))

	// Collect lazy signatures (cheap copy) under lock, then materialize outside lock
	// Collect lazy signatures (avoids BLS deserialization) under lock, then materialize outside lock

Conversation

PastaPastaPasta commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue being fixed or feature implemented

AI interpretation

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

github-actions bot commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ No Merge Conflicts Detected

Uh oh!

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

Uh oh!

Walkthrough

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

knst Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

PastaPastaPasta Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 24, 2025

Uh oh!

thepastaclaw left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

thepastaclaw Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

thepastaclaw Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

UdjinM6 Mar 17, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

PastaPastaPasta commented Oct 21, 2025 •

edited

Loading

github-actions bot commented Oct 21, 2025 •

edited

Loading

coderabbitai bot commented Oct 21, 2025 •

edited

Loading