Potential lock order inversion in mempool rescan (cs_wallet → cs_main vs cs_main → cs_wallet)

[thepastaclaw]

Summary

The mempool rescan code introduced by bitcoin#25351 (commit f89f8a1) calls requestMempoolTransactions while holding cs_wallet, creating a cs_wallet → cs_main lock ordering that conflicts with the existing cs_main → cs_wallet ordering in the mempool acceptance path.

Lock Order Analysis

Path 1 — Wallet rescan (src/wallet/wallet.cpp:2014):

WITH_LOCK(cs_wallet, chain().requestMempoolTransactions(*this));

requestMempoolTransactions takes LOCK2(cs_main, mempool->cs) and then calls CWallet::transactionAddedToMempool which does LOCK(cs_wallet) (reentrant on same thread).

Lock order: cs_wallet → cs_main

Path 2 — Mempool acceptance (src/validation.cpp:1185):

GetMainSignals().TransactionAddedToMempool(...)

Called while holding cs_main + mempool->cs. The signal is dispatched asynchronously via ENQUEUE_AND_LOG_EVENT to the scheduler thread. On the scheduler thread, NotificationsProxy::TransactionAddedToMempool calls CWallet::transactionAddedToMempool → LOCK(cs_wallet).

Lock order: cs_main → (async) → cs_wallet

Practical Impact

The actual deadlock risk is very low because the cs_main → cs_wallet path goes through the async validation interface scheduler — cs_main is released before the scheduler thread acquires cs_wallet. A real deadlock would require a synchronous path holding cs_main while trying to acquire cs_wallet, which doesn't currently exist.

However, the lock order debug assertions (-DDEBUG_LOCKORDER) would flag this as an inconsistency.

Note

Bitcoin Core still has the identical pattern on current master (wallet.cpp L2007). The fix would be to remove the outer WITH_LOCK(cs_wallet, ...) wrapper since transactionAddedToMempool acquires cs_wallet internally per-transaction:

-        WITH_LOCK(cs_wallet, chain().requestMempoolTransactions(*this));
+        chain().requestMempoolTransactions(*this);

Additionally, Dash-specific code in coinjoin/util.cpp has two instances of LOCK2(m_wallet.cs_wallet, ::cs_main) (lines 290, 334), which establishes the same cs_wallet → cs_main ordering and could interact with any future synchronous cs_main → cs_wallet path.

Introduced By

• Upstream: rpc, wallet: Scan mempool after import* - Second attempt bitcoin/bitcoin#25351
• Dash merge: f89f8a1 (backport batch backport: bitcoin#24419, #25351, #25466, #25701, #25707, #25733, #25872, #31364, #31305, #31051, partial bitcoin#31306 #6980)

/cc @UdjinM6

[knst]

Lock order: cs_main → (async) → cs_wallet

(async) in the gap mean that cs_main is not more locked by the time when cs_wallet will be locked.

The order cs_wallet -> cs_main is intentional and designed by.
You can see by src/wallet/test/util.cpp: LOCK2(wallet->cs_wallet, ::cs_main);

@thepastaclaw there's no issue, seems so.

[thepastaclaw]

Closing as invalid: after re-reading the callback path and the DEBUG_LOCKORDER implementation, there is no actual cs_main -> cs_wallet vs cs_wallet -> cs_main inversion here. The async validation path drops cs_main before the wallet lock, and the synchronous callback only re-locks cs_wallet recursively on the same thread.