Don’t have cs_wallet locked while calling each context.wallet_load_fns. A load handler can always lock cs_wallet if needed.
The lock was added in 1c7e25db0c to satisfy TSAN. With 44c430ffac most of the code requiring the lock is in CWallet::AttachChain. A comment is added to warn about wallets_mutex and cs_wallet lock ordering.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
what is the concern?
This does seem probably safe, but it would save reviewers some effort to know when the lock was added, when it stopped being necessary, and the specific reason why it was necessary whenever it was needed (for example whether it was avoiding deadlocks or asserts by acquiring recursive locks in a consistent order, or whether it was avoiding race condition.) This information could go in the PR description.
One concern might be that one of the wallet_load_fns callbacks could call a wallet method and acquire cs_wallet mutex after context.wallets_mutex, and maybe this would be in inconsistent order.
but it would save reviewers some effort to know when the lock was added, when it stopped being necessary
My bad for not writing this, I went thru history before making the change, will redo and update this.
One concern might be that one of the
wallet_load_fnscallbacks could call a wallet method and acquire cs_wallet mutex after context.wallets_mutex, and maybe this would be in inconsistent order.
Would be bad to lock one cs_wallet after wallets_mutex? In master that already happens but works because cs_wallet is recursive. Couldn’t find more cases of cs_wallet -> wallets_mutex lock order.
Code review ACK 2c87f99411e6529f69af9b299c9cc7f050f5caa7. It seems safe to remove this lock because it look like it was only added by accident in #11634 to try to satisfy lock annotations, even though it was never really needed to satisfy lock annotations even according to the thread there https://github.com/bitcoin/bitcoin/pull/11634/files#r227725517. (EDIT: lock was not added by accident, it was needed but was always broader than it needed to be and it became unnecessary after a recursive lock was added in #20773)
⬆️ I think above information would be helpful to add to PR description because otherwise this PR seems like a random change.
After this PR, I think there are no more cases where cs_wallet and wallets_mutex are locked at the same time so there is no required lock ordering between them. But in the future, GUI code could do arbitrary things in its wallet_load_fns handler, and maybe it will call a wallet function and lock cs_wallet in one. In that case the lock order would be wallets_mutex first then cs_wallet second. It may be good to state some intended lock ordering, maybe adding a code comment to WalletContext::wallets_mutex like “It is unsafe to lock this after locking a CWallet::cs_wallet mutex because this could introduce inconsistent lock ordering and cause deadlocks.”
One concern might be that one of the
wallet_load_fnscallbacks could call a wallet method and acquire cs_wallet mutex after context.wallets_mutex, and maybe this would be in inconsistent order.Would be bad to lock one
cs_walletafterwallets_mutex?
If there is any thread locking wallets_mutex after cs_wallet then it would be bad, but if not, then it’s fine.
In master that already happens but works because
cs_walletis recursive. Couldn’t find more cases of cs_wallet -> wallets_mutex lock order.
I don’t think this matters because recursive locks are no-ops. It a thread locks mutex A, then locks mutex B, then recursively locks mutex A again, then the recursive lock isn’t doing anything, and the lock order is still A before B.
@ryanofsky thanks! You might want to see bitcoin-core/gui@c09fd3a, it needs this change otherwise it deadlocks.
Thanks! That also looks like a good change. Is there a PR or issue this is all a part of?
@ryanofsky yes, https://github.com/bitcoin-core/gui/pull/417
Edit: I guess I have to rebase.
Looks correct to me, utACK 7850d33f7ff1c2ef7e34248efdbb60e792419d46
Agree with Russ though, definitely think the PR description needs updating.
But in the future, GUI code could do arbitrary things in its
wallet_load_fnshandler, and maybe it will call a wallet function and lockcs_walletin one. In that case the lock order would bewallets_mutexfirst thencs_walletsecond
This is already the case and it is where the GUI models are filled.
@meshcollider @ryanofsky updated OP, added wallets_mutex comment as suggested.
Also rebased.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
PR description doesn’t seem totally accurate, from #22868#issue-987214408
Don’t have
cs_walletlocked while calling eachcontext.wallet_load_fns. A load handler can always lockcs_walletif needed.The lock was added in 1c7e25d to satisfy TSAN. With 44c430f most of the code requiring the lock is in
CWallet::AttachChain. A comment is added to warn about wallets_mutex and cs_wallet lock ordering.
If my understanding of https://github.com/bitcoin/bitcoin/pull/11634/files#r227725517 is correct, then the lock was not added to satsify TSAN but was added to satisfy clang annotations. I think you could summarize this by saying that this LOCK(cs_wallet) was correctly added in #11634, but it became no longer unnecessary after #20773 when code requiring the lock was moved to the new AttachChain method.
Also it now seems like the motivation for this change is not just removing an unnecessary lock, but reversing the 1. cs_wallet 2. wallets_mutex lock order so it can now be 1. wallets_mutex 2. cs_wallet, because upcoming GUI PR https://github.com/bitcoin-core/gui/pull/417 needs this
33@@ -34,6 +34,8 @@ using LoadWalletFn = std::function<void(std::unique_ptr<interfaces::Wallet> wall
34 struct WalletContext {
35 interfaces::Chain* chain{nullptr};
36 ArgsManager* args{nullptr}; // Currently a raw pointer because the memory is not managed by this struct
37+ // It is unsafe to lock this after locking a CWallet::cs_wallet mutex because
38+ // this could introduce inconsistent lock ordering and cause deadlocks.
EXCLUDES / LOCKS_EXCLUDED (or REQUIRES(!mutex) with -Wthread-safety-negative) annotations to enforce this?
