refactor: extract buildSignHash outside acquiring cs #6915

PastaPastaPasta · 2025-10-24T16:49:08Z

Issue being fixed or feature implemented

extract recoveredSig.buildSignHash from mutex

Lines 14 to 22 in fd6ad86

    
           SignHash::SignHash(Consensus::LLMQType llmqType, const uint256& quorumHash, const uint256& id, const uint256& msgHash) 
        
           { 
        
               CHashWriter h(SER_GETHASH, 0); 
        
               h << llmqType; 
        
               h << quorumHash; 
        
               h << id; 
        
               h << msgHash; 
        
               m_hash = h.GetHash(); 
        
           }

buildSignHash is non-trivial; let's extract it to avoid locking cs longer than needed.

How Has This Been Tested?

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-24T16:49:44Z

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

coderabbitai · 2025-10-24T16:52:32Z

Walkthrough

In src/llmq/signing_shares.cpp, the CSigSharesManager::HandleNewRecoveredSig method introduces a local variable to cache the result of recoveredSig.buildSignHash().Get(). This cached value is then reused when calling RemoveSigSharesForSession(signHash), eliminating a redundant computation of the same sign hash. No control flow or functional behavior is altered.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Rationale: Single file, one method, straightforward caching optimization with no logic changes. Verification requires only confirming the cached value is identical to what was previously computed twice and that all call sites receive the correct value.

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 "refactor: extract buildSignHash outside acquiring cs" directly describes the main change in the changeset. According to the raw summary, a new local variable is introduced to cache buildSignHash().Get() to avoid recomputing it, which aligns with the title's intent to extract buildSignHash outside the mutex-protected section. The title is concise, clear, and specific enough that a teammate scanning the history would understand the primary refactoring goal—optimizing performance by moving a non-trivial computation outside a critical section lock.
Description Check	✅ Passed	The PR description is clearly related to the changeset. It identifies the specific issue ("extract recoveredSig.buildSignHash from mutex"), provides a code reference link, and explains the rationale ("buildSignHash is non-trivial; let's extract it to avoid locking cs longer than needed"). While the description could include more testing details, the criteria specifies that level of detail is not important for passing this check. The description conveys meaningful information about what is being changed and why, making it related to the changeset rather than vague or off-topic.

✨ 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 2e71a9a and d8bd26a.

📒 Files selected for processing (1)

src/llmq/signing_shares.cpp (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

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/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). (11)

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

🔇 Additional comments (1)

src/llmq/signing_shares.cpp (1)

1729-1735: LGTM! Effective lock contention reduction.

The refactoring correctly extracts the non-trivial buildSignHash() computation outside the critical section, reducing the time cs is held. The cached signHash value is then safely reused within the lock. This preserves functionality while improving concurrency.

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-25T08:17:26Z

src/llmq/signing_shares.cpp


 MessageProcessingResult CSigSharesManager::HandleNewRecoveredSig(const llmq::CRecoveredSig& recoveredSig)
 {
+    auto signHash = recoveredSig.buildSignHash().Get();


nit: there's no reason to put auto everywhere.

Suggested change

auto signHash = recoveredSig.buildSignHash().Get();

uint256 signHash{recoveredSig.buildSignHash().Get()};

knst · 2025-10-25T08:18:45Z

src/llmq/signing_shares.cpp

+    auto signHash = recoveredSig.buildSignHash().Get();
    LOCK(cs);
-    RemoveSigSharesForSession(recoveredSig.buildSignHash().Get());
+    RemoveSigSharesForSession(signHash);


or even better

SignHash hash{recoveredSig.buildSignHash()}; LOCK(cs); RemoveSigSharesForSession(hash.Get());

UdjinM6

Using SignHash here, in RemoveSigSharesForSession etc. might be a good idea for future refactoring but it's out-of-scope for this PR imo.

utACK d8bd26a

refactor: extract buildSignHash outside acquiring cs

d8bd26a

PastaPastaPasta added this to the 23.1 milestone Oct 24, 2025

PastaPastaPasta requested review from UdjinM6 and knst October 24, 2025 22:58

knst reviewed Oct 25, 2025

View reviewed changes

UdjinM6 approved these changes Nov 4, 2025

View reviewed changes

PastaPastaPasta merged commit b33dec1 into dashpay:develop Nov 4, 2025
31 of 34 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: extract buildSignHash outside acquiring cs #6915

refactor: extract buildSignHash outside acquiring cs #6915

Uh oh!

PastaPastaPasta commented Oct 24, 2025

Uh oh!

github-actions bot commented Oct 24, 2025

Uh oh!

coderabbitai bot commented Oct 24, 2025

Uh oh!

knst Oct 25, 2025

Uh oh!

knst Oct 25, 2025

Uh oh!

UdjinM6 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	SignHash::SignHash(Consensus::LLMQType llmqType, const uint256& quorumHash, const uint256& id, const uint256& msgHash)
	{
	CHashWriter h(SER_GETHASH, 0);
	h << llmqType;
	h << quorumHash;
	h << id;
	h << msgHash;
	m_hash = h.GetHash();
	}

	auto signHash = recoveredSig.buildSignHash().Get();
	uint256 signHash{recoveredSig.buildSignHash().Get()};

refactor: extract buildSignHash outside acquiring cs #6915

refactor: extract buildSignHash outside acquiring cs #6915

Uh oh!

Conversation

PastaPastaPasta commented Oct 24, 2025

Issue being fixed or feature implemented

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

github-actions bot commented Oct 24, 2025

✅ No Merge Conflicts Detected

Uh oh!

coderabbitai bot commented Oct 24, 2025

Walkthrough

Estimated code review effort

Pre-merge checks and finishing touches

Uh oh!

knst Oct 25, 2025

Choose a reason for hiding this comment

Uh oh!

knst Oct 25, 2025

Choose a reason for hiding this comment

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants