refactor: replace RecursiveMutex with Mutex in meta store/info and add thread-safety annotations; avoid re-entrant locking #6868

PastaPastaPasta · 2025-09-30T23:26:00Z

Issue being fixed or feature implemented

Convert mutex from Recursive to non-recursive

What was done?

Describe your changes in detail

How Has This Been Tested?

Please describe in detail how you tested your changes.

Include details of your testing environment, and the tests you ran
to see how your change affects other areas of the code, etc.

Breaking Changes

Please describe any breaking changes your code introduces

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)

…add thread-safety annotations (LOCKS_EXCLUDED); avoid re-entrant locking

github-actions · 2025-09-30T23:26:27Z

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

coderabbitai · 2025-09-30T23:33:53Z

Walkthrough

Adds EXCLUSIVE_LOCKS_REQUIRED(!cs) annotations to multiple public methods in CMasternodeMetaMan and MasternodeMetaStore across src/masternode/meta.h and src/masternode/meta.cpp. Affected methods include GetMetaInfo, RemoveGovernanceObject, GetAndClearDirtyGovernanceObjectHashes, AlreadyHavePlatformBan, GetPlatformBan, RememberPlatformBan, ToString, and MasternodeMetaStore’s Serialize, Unserialize, and Clear. Also replaces RecursiveMutex with Mutex for CMasternodeMetaInfo::cs and MasternodeMetaStore::cs. Implementations still perform internal locking where present; the public signatures now require EXCLUSIVE_LOCKS_REQUIRED(!cs).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 25.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
Description Check	✅ Passed	The pull request description identifies the conversion from RecursiveMutex to a non-recursive Mutex, which directly relates to the changeset even though it omits details on annotations and testing.
Title Check	✅ Passed	The title concisely describes the primary refactor of replacing RecursiveMutex with Mutex in the masternode meta store/info and the addition of thread-safety annotations to prevent re-entrant locking, matching the key changes in the pull request.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

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

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.}

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 54e2588 and 41e4324.

📒 Files selected for processing (2)

src/masternode/meta.cpp (3 hunks)
src/masternode/meta.h (6 hunks)

🧰 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/masternode/meta.cpp
src/masternode/meta.h

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/masternode/meta.cpp
src/masternode/meta.h

🧬 Code graph analysis (2)

src/masternode/meta.cpp (1)

src/masternode/meta.h (3)

cs (54-79)

cs (56-56)

cs (137-252)

src/masternode/meta.h (1)

src/masternode/meta.cpp (11)

LOCKS_EXCLUDED (69-81)

LOCKS_EXCLUDED (118-124)

LOCKS_EXCLUDED (126-131)

LOCKS_EXCLUDED (133-137)

LOCKS_EXCLUDED (139-148)

LOCKS_EXCLUDED (150-154)

LOCKS_EXCLUDED (156-160)

GetMetaInfo (69-69)

RemoveGovernanceObject (62-67)

RemoveGovernanceObject (62-62)

RemoveGovernanceObject (118-118)

🪛 GitHub Actions: Clang Diff Format Check

src/masternode/meta.h

[error] 133-144: Clang-format differences detected between before/after formatting. Run clang-format-diff.py -p1 or clang-format to fix formatting.

⏰ 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). (9)

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

🔇 Additional comments (1)

src/masternode/meta.cpp (1)

69-154: No external LOCK(cs) detected around these APIs—safe from deadlock.

coderabbitai · 2025-09-30T23:33:56Z

src/masternode/meta.cpp

    for(const auto& p : metaInfos) {
        p.second->RemoveGovernanceObject(nGovernanceObjectHash);
    }


⚠️ Potential issue | 🟡 Minor

Run clang-format on this block.

CI flagged the formatting here (for(const …)); please run clang-format so the spacing matches the project style.

🤖 Prompt for AI Agents

In src/masternode/meta.cpp around lines 121 to 123, the for-loop formatting doesn't match project style (CI flagged spacing in "for(const auto& p : metaInfos)"); run clang-format or reformat this block so spacing/indentation follow the repository's style guide (e.g., add a space after 'for' and align braces) and commit the formatted changes.

UdjinM6

utACK 41e4324

kwvg · 2025-10-03T14:43:08Z

src/masternode/meta.h

    }

-    std::string ToString() const;
+    std::string ToString() const LOCKS_EXCLUDED(cs);


We switched to using the stronger EXCLUSIVE_LOCKS_REQUIRED(!cs) (negative capabilities) since dash#6001

…UIRED for various methods in meta store and info

kwvg

LGTM, minor nit

kwvg · 2025-10-03T16:15:40Z

src/masternode/meta.cpp

 }

-std::optional<PlatformBanMessage> CMasternodeMetaMan::GetPlatformBan(const uint256& inv_hash) const LOCKS_EXCLUDED(cs)
+std::optional<PlatformBanMessage> CMasternodeMetaMan::GetPlatformBan(const uint256& inv_hash) const EXCLUSIVE_LOCKS_REQUIRED(!cs)


nit: duplicate annotation, annotation in header is sufficient

UdjinM6

utACK 191c79c

kwvg

utACK 191c79c

masternode: replace RecursiveMutex with Mutex in meta store/info and …

41e4324

…add thread-safety annotations (LOCKS_EXCLUDED); avoid re-entrant locking

PastaPastaPasta added this to the 23 milestone Sep 30, 2025

coderabbitai bot reviewed Sep 30, 2025

View reviewed changes

UdjinM6 previously approved these changes Oct 1, 2025

View reviewed changes

PastaPastaPasta requested review from knst and kwvg October 3, 2025 14:36

kwvg requested changes Oct 3, 2025

View reviewed changes

refactor: update thread-safety annotations to use EXCLUSIVE_LOCKS_REQ…

191c79c

…UIRED for various methods in meta store and info

PastaPastaPasta dismissed UdjinM6’s stale review via 191c79c October 3, 2025 14:49

kwvg reviewed Oct 3, 2025

View reviewed changes

PastaPastaPasta requested a review from kwvg October 5, 2025 14:01

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

feat: share vecMasternodesUsed across all wallets, improve its handling #6875

Open

5 tasks

PastaPastaPasta requested a review from UdjinM6 October 7, 2025 15:02

UdjinM6 approved these changes Oct 7, 2025

View reviewed changes

kwvg approved these changes Oct 7, 2025

View reviewed changes

PastaPastaPasta merged commit fb01c5a into dashpay:develop Oct 8, 2025
34 of 38 checks passed

knst mentioned this pull request Oct 15, 2025

refactor: drop mutex and atomic from CMasternodeMetaInfo, access to object directly without shared_ptr #6894

Open

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: replace RecursiveMutex with Mutex in meta store/info and add thread-safety annotations; avoid re-entrant locking #6868

refactor: replace RecursiveMutex with Mutex in meta store/info and add thread-safety annotations; avoid re-entrant locking #6868

Uh oh!

PastaPastaPasta commented Sep 30, 2025

Uh oh!

github-actions bot commented Sep 30, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Sep 30, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot Sep 30, 2025

Uh oh!

UdjinM6 left a comment

Uh oh!

kwvg Oct 3, 2025

Uh oh!

kwvg left a comment

Uh oh!

kwvg Oct 3, 2025

Uh oh!

UdjinM6 left a comment

Uh oh!

kwvg left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

refactor: replace RecursiveMutex with Mutex in meta store/info and add thread-safety annotations; avoid re-entrant locking #6868

refactor: replace RecursiveMutex with Mutex in meta store/info and add thread-safety annotations; avoid re-entrant locking #6868

Uh oh!

Conversation

PastaPastaPasta commented Sep 30, 2025

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

github-actions bot commented Sep 30, 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 Sep 30, 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!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Sep 30, 2025

Choose a reason for hiding this comment

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

kwvg Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

kwvg left a comment

Choose a reason for hiding this comment

Uh oh!

kwvg Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

kwvg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Sep 30, 2025 •

edited

Loading

coderabbitai bot commented Sep 30, 2025 •

edited

Loading