Skip to content

Conversation

@PastaPastaPasta
Copy link
Member

Issue being fixed or feature implemented

Avoiding using raw size as indication for special logic; use a helper function in-case the conditions change in the future (however unlikely that may be)

What was done?

added helper for is_single_member

How Has This Been Tested?

builds

Breaking Changes

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)

@PastaPastaPasta PastaPastaPasta added this to the 23.1 milestone Oct 21, 2025
@github-actions
Copy link

⚠️ Potential Merge Conflicts Detected

This PR has potential conflicts with the following open PRs:

Please coordinate with the authors of these PRs to avoid merge conflicts.

@coderabbitai
Copy link

coderabbitai bot commented Oct 21, 2025

Walkthrough

This pull request introduces a new public constexpr method is_single_member() to the LLMQParams class that returns true when the quorum size equals 1. The method is then substituted throughout the codebase in five files (commitment.cpp, dkgsessionhandler.cpp, signing_shares.cpp, and quorums.cpp) to replace explicit size == 1 comparisons with calls to this new accessor. The control flow and behavior remain functionally identical; the changes are purely structural, replacing direct size checks with a dedicated predicate method.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

The changes follow a consistent, repetitive refactoring pattern applied homogeneously across multiple files. Each modification is mechanical—replacing direct size == 1 checks with the new is_single_member() method call—with no logic modifications, error handling changes, or behavioral alterations. The method addition itself is trivial and straightforward. The homogeneous nature of the substitutions reduces cognitive load per file, making this a straightforward refactoring review.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% 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 pull request title "refactor: replace size check with is_single_member() method for clarity" directly and accurately describes the main change in the changeset. The title correctly identifies that this is a refactoring effort that replaces direct size comparisons with a new helper method, which aligns with all the file changes shown in the summary. The title is concise, specific, and clearly communicates the primary change a teammate would encounter when reviewing this commit in history.
Description Check ✅ Passed The pull request description is directly related to the changeset. It explains the rationale for the change (using a helper method to avoid raw size checks and allow for future flexibility), documents what was done (adding the is_single_member helper), and includes testing confirmation. The description provides meaningful context about the purpose of the refactoring and is not vague or off-topic, making it adequate for understanding the changeset's intent.
✨ 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 7074278 and 2f540be.

📒 Files selected for processing (5)
  • src/llmq/commitment.cpp (1 hunks)
  • src/llmq/dkgsessionhandler.cpp (1 hunks)
  • src/llmq/params.h (1 hunks)
  • src/llmq/signing_shares.cpp (2 hunks)
  • src/rpc/quorums.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/commitment.cpp
  • src/llmq/params.h
  • src/llmq/dkgsessionhandler.cpp
  • src/rpc/quorums.cpp
  • src/llmq/signing_shares.cpp
🧠 Learnings (2)
📓 Common learnings
Learnt from: knst
PR: dashpay/dash#6692
File: src/llmq/commitment.cpp:54-60
Timestamp: 2025-08-19T15:08:00.835Z
Learning: In Dash Core, llmq_params.size == 1 is used only for regtest environments, not on public networks, which significantly reduces the risk profile of code paths specific to single-member quorums.
📚 Learning: 2025-08-19T15:08:00.835Z
Learnt from: knst
PR: dashpay/dash#6692
File: src/llmq/commitment.cpp:54-60
Timestamp: 2025-08-19T15:08:00.835Z
Learning: In Dash Core, llmq_params.size == 1 is used only for regtest environments, not on public networks, which significantly reduces the risk profile of code paths specific to single-member quorums.

Applied to files:

  • src/llmq/params.h
🔇 Additional comments (6)
src/llmq/signing_shares.cpp (2)

784-784: LGTM! Clear semantic improvement.

The replacement of the raw size check with is_single_member() makes the intent explicit and improves code readability.


1553-1553: LGTM! Consistent refactoring.

The use of is_single_member() here aligns with the usage in TryRecoverSig and improves maintainability by centralizing the single-member condition logic.

src/llmq/params.h (1)

124-124: LGTM! Well-designed helper method.

The new is_single_member() constexpr method is correctly implemented with appropriate attributes ([[nodiscard]] and const). This provides a clear, semantic alternative to raw size == 1 checks and improves code maintainability across the codebase.

Based on learnings

src/rpc/quorums.cpp (1)

214-214: LGTM! Improved clarity in RPC output logic.

The use of is_single_member() makes the branching logic for public key share emission more self-documenting.

src/llmq/dkgsessionhandler.cpp (1)

554-554: LGTM! Critical path now more explicit.

The use of is_single_member() to control the DKG round handling makes this important branching logic clearer. This path determines whether to use the simplified single-commitment flow or the full multi-phase DKG process.

src/llmq/commitment.cpp (1)

54-54: LGTM! Signature verification logic now clearer.

Replacing the size check with is_single_member() improves the readability of this critical signature verification path. The distinction between single-operator and aggregated-member signature verification is now more explicit.

Based on learnings


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.

Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 2f540be

ss3.str(), quorumPublicKey.ToString(), commitmentHash.ToString());
}
if (llmq_params.size == 1) {
if (llmq_params.is_single_member()) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's change this one too

if (llmq_params.size != 1 && quorumVvecHash.IsNull()) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants