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

…, access to object directly without shared_ptr 572bafd refactor: rename IsDsqOver to IsMixingThresholdExceeded (Konstantin Akimov) 16915ff fixup: lock annotation for CMasternodeMetaMan::nDsqCount (Konstantin Akimov) 982b68e fix: usage of ResetPlatformBan in CDeterministicMNList (Konstantin Akimov) f860031 refactoring: apply code review suggestions (Konstantin Akimov) fe3966d fix: removed leftover annotation, as follow-up conflict resolving with #6880 (Konstantin Akimov) 969b841 refactor: drop return bool from AddGovernanceVote which is always true (Konstantin Akimov) 04ab976 refactor: remove unused CConnMan from meta.h (Konstantin Akimov) ed27d90 refactor: use GetMetaInfoOrDefault widely, final (Konstantin Akimov) b79dd90 refactor: use a helper GetMetaInfo and GetMetaInfoOrDefault internally (Konstantin Akimov) 69ed5f5 refactor: remove useless helpers of CMasternodeMetaInfo (Konstantin Akimov) 1bfd6ff refactor: drop mutex and atomics from CMasternodeMetaInfo (Konstantin Akimov) d5e693f refactor: drop shared_ptr from CMasternodeMetaMan and make GetMetaInfo a private member (Konstantin Akimov) c0e146f refactor: hide direct usages of GetMetaInfo for net_processing (Konstantin Akimov) b1a03e6 refactor: hide direct usages of GetMetaInfo from net module (Konstantin Akimov) a40c418 refactor: hide direct calls of GetMetaInfo from llmq/utils (Konstantin Akimov) 7ba51ec refactor: remove direct usages of GetMetaInfo for dkgsession (Konstantin Akimov) 7ec239b refactor: replace direct usages of GetMetaInfo when platform unban node in mn-list (Konstantin Akimov) 16abbed refactor: replace direct usages of GetMetaInfo while calculating dsq threshold (Konstantin Akimov) 6cb2344 refactor: replace direct call of MetaInfo from rpc/evo code to helper (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Current implementation: 1. every access to `CMasternodeMetaInfo` is done by accessing `GetMetaInfo` under mutex `CMasternodeMetaMan::cs` 2. `GetMetaInfo` returns shared_ptr to object 3. every read or write of `GetMetaInfo` is protected by atomic, second mutex `CMasternodeMetaInfo::cs`; access to object is done indirectly by one more memory hop (because shared_ptr) It is not efficient, because `GetMetaInfo` is spread all over code base and not a single case need shared_ptr here; all accesses are brief and short (single read or single write; except RPC where all structure is serialized). ## What was done? This PR is follow-up for PR #6868 with further improvements and simplification of code. Instead returning shared_ptr with `CMasternodeMetaInfo` directly access to members of `CMasternodeMetaMan` under its mutex; all mutexes, atomic and shared_ptr itself are removed from `CMasternodeMetaInfo` accesses. It simplified implementation significantly, removed big amount of thread-safety annotations. Performance is probably improved, but improvement is unmeasurable, because `CMasternodeMetaInfo` and `CMasternodeMetaMan` has not been spotted as hot spot in profiler neither hot spot in lock contentions logs. Also this PR removes duplicated code between coinjoin/client and coinjoin/server by creating a helper in `CMasternodeMetaMan` ## How Has This Been Tested? Run unit & functional tests ## Breaking Changes N/A ## Checklist: - [x] 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 - [x] I have assigned this pull request to a milestone ACKs for top commit: UdjinM6: utACK 572bafd Tree-SHA512: a9a955a6d2d5d9c18f35bd900a4d2064217974c44277572cdaf10d729cafa4e937b4fc361d64a5132439227fcc7fd3fff98358e20aa7adcbfbbe2a399f9e5052

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

Merged

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

Merged

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