wallet: Call load handlers without cs_wallet locked #22868

promag · 2021-09-02T22:18:13Z

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

DrahtBot · 2021-09-04T15:21:00Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

hebasto · 2021-09-07T18:55:28Z

Why this change is safe?

promag · 2021-09-07T20:31:02Z

@hebasto what is the concern? The lock can be acquired inside the load handler. This change also avoids recursive locks on cs_wallet.

ryanofsky · 2021-09-08T20:39:05Z

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.

promag · 2021-09-11T22:12:56Z

@ryanofsky

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_fns callbacks 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.

promag · 2021-09-12T20:49:07Z

@ryanofsky see https://github.com/bitcoin/bitcoin/pull/11634/files#r227725517 for when the lock was added and @practicalswift submitted a similar change #14560 but @MarcoFalke rejected it.

ryanofsky

Code review ACK 2c87f99. 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_fns callbacks 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?

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_wallet is 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.

promag · 2021-09-24T15:08:48Z

@ryanofsky thanks! You might want to see bitcoin-core/gui@c09fd3a, it needs this change otherwise it deadlocks.

ryanofsky · 2021-11-08T17:05:29Z

@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?

promag · 2021-11-08T17:07:19Z

@ryanofsky yes, bitcoin-core/gui#417

Edit: I guess I have to rebase.

promag · 2021-11-08T22:00:30Z

Rebased due to trivial conflict with #23123.

ryanofsky

Code review ACK 7850d33 with caveats from #22868 (review) that I'm not 100% sure this won't cause deadlocks now or in the future, but do think this seems safe and is probably a good simplification. Also I think the PR description is lacking essential information like why it is safe to remove the lock and how long it's been unnecessary.

meshcollider

Looks correct to me, utACK 7850d33

Agree with Russ though, definitely think the PR description needs updating.

promag · 2021-11-22T09:38:53Z

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

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>

meshcollider

re-utACK f13a22a

ryanofsky · 2021-11-24T05:15:01Z

PR description doesn't seem totally accurate, from #22868 (comment)

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 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 bitcoin-core/gui#417 needs this

ryanofsky

Code review ACK f13a22a. Only change since last review is adding a lock order comment

jonatack

ACK f13a22a

jonatack · 2021-11-24T12:34:13Z

src/wallet/context.h

@@ -34,6 +34,8 @@ using LoadWalletFn = std::function<void(std::unique_ptr<interfaces::Wallet> wall
 struct WalletContext {
    interfaces::Chain* chain{nullptr};
    ArgsManager* args{nullptr}; // Currently a raw pointer because the memory is not managed by this struct
+    // It is unsafe to lock this after locking a CWallet::cs_wallet mutex because
+    // this could introduce inconsistent lock ordering and cause deadlocks.


Haven't tried, but would there be any point in using EXCLUDES / LOCKS_EXCLUDED (or REQUIRES(!mutex) with -Wthread-safety-negative) annotations to enforce this?

f13a22a wallet: Call load handlers without cs_wallet locked (João Barbosa) Pull request description: 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 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. ACKs for top commit: meshcollider: re-utACK f13a22a ryanofsky: Code review ACK f13a22a. Only change since last review is adding a lock order comment jonatack: ACK f13a22a Tree-SHA512: d51976c3aae4bebc2d1997c88edff712d21fc5523801f5614062a10f826e164579973aeb1981bb1cbc243ecff6af3250362f544c02a79e5d135cbbca1704be62

Gemk83

Ok

promag mentioned this pull request Sep 2, 2021

Ditch wallet model juggling bitcoin-core/gui#417

Closed

DrahtBot added the Wallet label Sep 2, 2021

promag force-pushed the 2021-09-wallet-load-lock branch 4 times, most recently from 1ba170e to 2c87f99 Compare September 3, 2021 07:47

DrahtBot mentioned this pull request Sep 4, 2021

Wallet passive startup #15719

Closed

DrahtBot mentioned this pull request Sep 9, 2021

refactor: Remove gArgs from wallet.h and wallet.cpp (2) #22928

Merged

ryanofsky approved these changes Sep 24, 2021

View reviewed changes

DrahtBot mentioned this pull request Sep 29, 2021

Remove -rescan startup parameter #23123

Closed

DrahtBot added the Needs rebase label Sep 30, 2021

promag force-pushed the 2021-09-wallet-load-lock branch from 2c87f99 to 7850d33 Compare November 8, 2021 21:54

DrahtBot removed the Needs rebase label Nov 8, 2021

ryanofsky approved these changes Nov 10, 2021

View reviewed changes

meshcollider reviewed Nov 22, 2021

View reviewed changes

promag force-pushed the 2021-09-wallet-load-lock branch from 7850d33 to c34d934 Compare November 22, 2021 09:32

wallet: Call load handlers without cs_wallet locked

f13a22a

Co-authored-by: Russell Yanofsky <russ@yanofsky.org>

promag force-pushed the 2021-09-wallet-load-lock branch from c34d934 to f13a22a Compare November 22, 2021 09:43

meshcollider reviewed Nov 23, 2021

View reviewed changes

ryanofsky approved these changes Nov 24, 2021

View reviewed changes

jonatack reviewed Nov 24, 2021

View reviewed changes

meshcollider merged commit 200d97f into bitcoin:master Nov 27, 2021

promag deleted the 2021-09-wallet-load-lock branch February 23, 2022 00:49

Gemk83 reviewed Feb 26, 2022

View reviewed changes

bitcoin locked and limited conversation to collaborators Feb 26, 2023

wallet: Call load handlers without cs_wallet locked #22868

wallet: Call load handlers without cs_wallet locked #22868

Uh oh!

Conversation

promag commented Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Sep 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

hebasto commented Sep 7, 2021

Uh oh!

promag commented Sep 7, 2021

Uh oh!

ryanofsky commented Sep 8, 2021

Uh oh!

promag commented Sep 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Sep 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Sep 24, 2021

Uh oh!

ryanofsky commented Nov 8, 2021

Uh oh!

promag commented Nov 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Nov 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

meshcollider left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Nov 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Nov 24, 2021

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Nov 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gemk83 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

promag commented Sep 2, 2021 •

edited

Loading

DrahtBot commented Sep 4, 2021 •

edited

Loading

promag commented Sep 11, 2021 •

edited

Loading

promag commented Sep 12, 2021 •

edited

Loading

ryanofsky left a comment •

edited

Loading

promag commented Nov 8, 2021 •

edited

Loading

promag commented Nov 8, 2021 •

edited

Loading

meshcollider left a comment •

edited

Loading

promag commented Nov 22, 2021 •

edited

Loading

ryanofsky left a comment •

edited

Loading

jonatack Nov 24, 2021 •

edited

Loading