fix: remove redundant cs_wallet lock around requestMempoolTransactions

[thepastaclaw]

fix: remove redundant cs_wallet lock around requestMempoolTransactions

Summary

The WITH_LOCK(cs_wallet, ...) wrapper around
chain().requestMempoolTransactions(*this) in ScanForWalletTransactions
creates an inconsistent lock ordering:

WITH_LOCK(cs_wallet, chain().requestMempoolTransactions(*this));
// requestMempoolTransactions takes LOCK2(cs_main, mempool->cs),
// then calls transactionAddedToMempool → LOCK(cs_wallet)

Lock order: cs_wallet → cs_main, which conflicts with the standard
cs_main → cs_wallet ordering and would trigger DEBUG_LOCKORDER assertions.

Fix

Remove the outer WITH_LOCK(cs_wallet, ...) wrapper.
transactionAddedToMempool acquires cs_wallet internally per-transaction,
so the outer lock is redundant.

ScanForWalletTransactions does not hold cs_wallet at this point in the
function — all prior LOCK(cs_wallet) scopes have been released.

Notes

• Bitcoin Core has the identical pattern on current master; this is a local fix
• The requestMempoolTransactions call in LoadWallet (line 3472) is already
  called while cs_wallet is held via RecursiveMutex — no change needed there
• Reported in Potential lock order inversion in mempool rescan (cs_wallet → cs_main vs cs_main → cs_wallet) #7226

Validation

1-line removal of a redundant lock wrapper. No behavioral change; cs_wallet
is re-acquired per-transaction inside transactionAddedToMempool.
Verified by code inspection.

[thepastaclaw]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[github-actions]

This pull request has conflicts, please rebase.

[github-actions]

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: d2899c35-8329-4893-937d-b7d02bbec363

📥 Commits

Reviewing files that changed from the base of the PR and between 32fa1a8 and ea1e8fe.

📒 Files selected for processing (1)

• src/wallet/wallet.cpp

---

Walkthrough

A single line in src/wallet/wallet.cpp was modified within the ScanForWalletTransactions function. The wallet mutex lock wrapper (WITH_LOCK(cs_wallet, ...)) was removed from around a chain().requestMempoolTransactions(*this) call, replacing it with a direct call. This changes the synchronization boundary during mempool refresh, removing the per-call wallet mutex scope for this specific operation. No modifications to logic, error handling, or other functionality accompany this change.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the main change: removing a redundant lock wrapper around requestMempoolTransactions.
Description check	✅ Passed	The description is comprehensive and directly related to the changeset, explaining the lock ordering issue, the fix rationale, and validation approach.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

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

📝 Coding Plan

• Generate coding plan for human review comments

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

Tip

CodeRabbit can suggest fixes for GitHub Check annotations.

Configure the reviews.tools.github-checks setting to adjust the time to wait for GitHub Checks to complete.

[thepastaclaw]

Code Review

The PR correctly fixes the cs_wallet → cs_main lock ordering violation in ScanForWalletTransactions and soundly refactors requestMempoolTransactions to snapshot under lock then notify outside lock. However, the identical lock ordering violation remains in postInitProcess, which is reachable from multi-threaded RPC handlers (CreateWallet, LoadWallet). The PR description's claim that RecursiveMutex makes this safe is incorrect — RecursiveMutex only prevents self-deadlock on re-entrant acquisition by the same thread, not cross-thread deadlock from lock ordering inversion.

Reviewed commit: e7a422b

🟡 1 suggestion(s)

1 additional finding(s) omitted (not in diff).

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `src/wallet/wallet.cpp`:
- [SUGGESTION] lines 3463-3473: postInitProcess() still holds cs_wallet across requestMempoolTransactions — same lock ordering violation this PR fixes elsewhere
  postInitProcess() acquires `LOCK(cs_wallet)` at line 3465, then calls `chain().requestMempoolTransactions(*this)` at line 3472, which acquires `LOCK2(::cs_main, m_node.mempool->cs)` in interfaces.cpp. This is the same cs_wallet → cs_main ordering violation that the PR correctly fixes in ScanForWalletTransactions.

The PR description states no change is needed here because cs_wallet is a RecursiveMutex. This reasoning is incorrect: RecursiveMutex only prevents self-deadlock when the *same thread* re-acquires cs_wallet (which happens when transactionAddedToMempool re-enters cs_wallet). It does NOT prevent cross-thread deadlock from the cs_wallet → cs_main ordering inversion.

Deadlock scenario: Thread A (postInitProcess via CreateWallet/LoadWallet RPC) holds cs_wallet, waits for cs_main. Thread B (block validation) holds cs_main, waits for cs_wallet via a ValidationInterface callback.

This is pre-existing and not introduced by this PR, but since the same pattern is being addressed, it would be natural to fix it here. The fix is identical: release cs_wallet before calling requestMempoolTransactions.

[thepastaclaw]

The p2p_node_network_limited TSAN failure is a known flaky test — it's the same issue tracked in #7228 (the test tries to use node 0 after it's been stopped for a Dash-specific governance stderr check). That's been fixed on the backport branch and will land separately.

Removed the empty retrigger commit and force-pushed to keep history clean. CI will re-run on the force push.