wallet: Avoid locking cs_wallet recursively #19833

promag · 2020-08-29T14:37:14Z

This is another small change towards non recursive wallet lock.

DrahtBot · 2020-08-29T18:30:25Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22341 (rpc: add getxpub by Sjors)
#20205 (wallet: Properly support a wallet id by achow101)
#19602 (wallet: Migrate legacy wallets to descriptor wallets by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

hebasto · 2020-08-30T07:46:56Z

Concept ACK.

src/interfaces/wallet.cpp

hebasto

ACK 9c5a28c "wallet: GetLabelAddresses requires cs_wallet lock "
ACK 7a138a8 "wallet: AddWalletDescriptor requires cs_wallet lock"
Approach NACK a98d192 "wallet: GetBalance requires cs_wallet lock"

Placing LOCK() macros in member functions of a class that implements an interface is not a good idea. When CWallet::cs_wallet actually transits from RecurviseMutex to Mutex we will be forced to guard all such functions with negative assertions EXCLUSIVE_LOCKS_REQUIRED(!m_wallet->cs_wallet). But they won't work as they are placed not in a header file.

promag · 2020-08-31T14:33:51Z

we will be forced to guard all such functions with negative assertions EXCLUSIVE_LOCKS_REQUIRED(!m_wallet->cs_wallet)

Seems unnecessary no? Locking an already locked Mutex fails. If you really want static analysis then we could add a new method in the private implementation just to add the annotation?

maflcko · 2020-08-31T14:34:32Z

Placing LOCK() macros in member functions of a class that implements an interface is not a good idea. When CWallet::cs_wallet actually transits from RecurviseMutex to Mutex we will be forced to guard all such functions with negative assertions EXCLUSIVE_LOCKS_REQUIRED(!m_wallet->cs_wallet). But they won't work as they are placed not in a header file.

There are already LOCK(cs_wallet) in the interface cpp file. How is the new one any different and how would it make it any harder. Also, I am not sure if we really want to annotate extensively with negative lock annotations. Has there been a conceptual discussion about this?

hebasto · 2020-08-31T14:41:11Z

@promag

we will be forced to guard all such functions with negative assertions EXCLUSIVE_LOCKS_REQUIRED(!m_wallet->cs_wallet)

Seems unnecessary no? Locking an already locked Mutex fails.

Yes, it does. In run time.

If you really want static analysis then we could add a new method in the private implementation just to add the annotation?

Actually moving implementation class declaration into a header will help.

@MarcoFalke

There are already LOCK(cs_wallet) in the interface cpp file.

Yes, there are. Unfortunately.

Also, I am not sure if we really want to annotate extensively with negative lock annotations. Has there been a conceptual discussion about this?

You are correct. But it seems premature until transit from RecurviseMutex to Mutex happens. Btw, do you see downsides of it?

hebasto

ACK a98d192, convinced after discussion, tested on Linux Mint 20 (x86_64).

promag · 2020-08-31T14:45:26Z

Actually moving implementation class declaration into a header will help.

I don't understand what you mean, please elaborate.

ryanofsky · 2020-08-31T15:05:51Z

I don't think you could use EXCLUSIVE_LOCKS_REQUIRED(!m_wallet->cs_wallet) in the interfaces::WalletImpl class, for the same reason you couldn't use EXCLUSIVE_LOCKS_REQUIRED(!pwallet->cs_wallet) on RPC methods, because method callers can't know which wallet object the wallet pointer points to, so couldn't provably meet the requirements.

To make cs_wallet non-recursive, you I think you just need to make a blanket assumption that cs_wallet will not held when calling RPC methods or interfaces::Wallet* methods, and it won't be possible to use locking annotations in a meaningful way.

ryanofsky · 2020-08-31T15:11:02Z

Actually moving implementation class declaration into a header will help.

The reason the implementation class is not in a header is that implementation class methods are never called directly. They are only called through the interface, and the interface is only called by the GUI. I don't think there's a way to make GUI aware of cs_wallet and be able to satisfy assertions about whether it is locked or unlocked.

hebasto · 2020-08-31T17:33:30Z

@ryanofsky

Actually moving implementation class declaration into a header will help.

The reason the implementation class is not in a header is that implementation class methods are never called directly. They are only called through the interface, and the interface is only called by the GUI. I don't think there's a way to make GUI aware of cs_wallet and be able to satisfy assertions about whether it is locked or unlocked.

We could use LOCK_RETURNED aka RETURN_CAPABILITY.

@promag

Actually moving implementation class declaration into a header will help.

I don't understand what you mean, please elaborate.

Mind looking into this branch: https://github.com/hebasto/bitcoin/commits/pr19833-0831-demo ?

promag · 2020-11-07T11:51:38Z

@hebasto rebased to have a fresh build, mind taking a 2nd look?

@meshcollider ping.

hebasto

LGTM, but another rebasing seems required :)

I think this pull is an improvement of the current CWallet interface design. In the future it could be re-designed with the concurrency in mind, i.e., without public access to mutexes etc.

hebasto · 2020-11-12T09:56:36Z

src/interfaces/wallet.cpp

@@ -348,6 +348,7 @@ class WalletImpl : public Wallet
    }
    WalletBalances getBalances() override
    {
+        LOCK(m_wallet->cs_wallet);


Could the lock be bounded to GetBalance only?

WITH_LOCK?

That is my thought.

I think it's no big deal for now. I'll do it it I have to update.

hebasto

ACK 97dbdcd, I have reviewed the code and it looks OK, I agree it can be merged.

ryanofsky

Code review ACK 97dbdcd. This makes locking code less confusing without changing behavior and should make it easier to switch to a non-recursive lock.

hebasto's approach hebasto/bitcoin@pr19833-0831-demo (commits) to use lock annotations to prevent the gui from calling wallet methods while holding cs_wallet could be an interesting followup. Though if there are cases where wallet is locking cs_wallet, and then making a gui callback which calls interfaces::Wallet methods, it might be an indication that the notification API is too low level, and more code should be moved from the GUI to the wallet, so code can run entirely in the wallet, instead of calling out of the wallet to the gui then back into the wallet.

hebasto · 2021-09-03T15:31:42Z

@promag

A friendly reminder to rebase :)

Happy to re-ACK then.

No change in behavior, the lock is already held at call sites.

ryanofsky

Code review ACK 97dbdcd. I didn't look to see what previously discussed followups may be still applicable, but the current version of this does nicely cleans up a few locks and looks good

hebasto

ACK 5fabde6, I have reviewed the code and it looks OK, I agree it can be merged.

promag · 2021-09-04T10:39:12Z

To be fair #22100 simplified this change, 97dbdcd is no longer necessary.

5fabde6 wallet: AddWalletDescriptor requires cs_wallet lock (João Barbosa) 32d036e wallet: GetLabelAddresses requires cs_wallet lock (João Barbosa) Pull request description: This is another small change towards non recursive wallet lock. ACKs for top commit: hebasto: ACK 5fabde6, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: 00506f0159c56854a171e58a451db8dd9b9f735039697b1cf2ca7f54de61fb51cc1e5eff42265233e041b4b1bfd29c2247496dc4456578e1a23c323bdec2901b

DrahtBot added the Wallet label Aug 29, 2020

This was referenced Aug 29, 2020

wallet: importdescriptors update existing #19651

Merged

wallet: Migrate legacy wallets to descriptor wallets #19602

Merged

promag force-pushed the 2020-08-cswallet branch 2 times, most recently from ec3f4bf to d306298 Compare August 30, 2020 00:51

hebasto mentioned this pull request Aug 30, 2020

Replace all of the RecursiveMutex instances with the Mutex ones #19303

Open

36 tasks

hebasto reviewed Aug 30, 2020

View reviewed changes

src/interfaces/wallet.cpp Outdated Show resolved Hide resolved

promag force-pushed the 2020-08-cswallet branch from d306298 to a98d192 Compare August 30, 2020 10:43

hebasto reviewed Aug 31, 2020

View reviewed changes

hebasto approved these changes Aug 31, 2020

View reviewed changes

This was referenced Sep 4, 2020

scripted-diff: Restore AssertLockHeld after #19668, remove LockAssertion #19865

Closed

Avoid locking CTxMemPool::cs recursively in some cases #19872

Closed

DrahtBot mentioned this pull request Oct 3, 2020

Use shared pointers only in validation interface #18354

Closed

This was referenced Oct 21, 2020

wallet: Properly support a wallet id #20205

Closed

rpc, wallet: Expose wallet id in getwalletinfo RPC output #20243

Closed

This was referenced Oct 30, 2020

Disable and fix tests for when BDB is not compiled #20267

Merged

wallet: fix change detection of imported internal descriptors #20266

Merged

promag force-pushed the 2020-08-cswallet branch from a98d192 to fa2cf36 Compare November 7, 2020 11:51

DrahtBot added the Needs rebase label Nov 9, 2020

hebasto reviewed Nov 12, 2020

View reviewed changes

promag force-pushed the 2020-08-cswallet branch from fa2cf36 to 97dbdcd Compare November 12, 2020 10:08

DrahtBot removed the Needs rebase label Nov 12, 2020

hebasto approved these changes Nov 12, 2020

View reviewed changes

DrahtBot added the Needs rebase label Dec 1, 2020

ryanofsky approved these changes Jan 18, 2021

View reviewed changes

promag added 2 commits September 3, 2021 18:28

wallet: GetLabelAddresses requires cs_wallet lock

32d036e

No change in behavior, the lock is already held at call sites.

wallet: AddWalletDescriptor requires cs_wallet lock

5fabde6

No change in behavior, the lock is already held at call sites.

promag force-pushed the 2020-08-cswallet branch from 97dbdcd to 5fabde6 Compare September 3, 2021 17:36

DrahtBot removed the Needs rebase label Sep 3, 2021

ryanofsky approved these changes Sep 3, 2021

View reviewed changes

hebasto approved these changes Sep 3, 2021

View reviewed changes

DrahtBot mentioned this pull request Sep 4, 2021

rpc: add path to gethdkey #22341

Closed

maflcko merged commit 9393666 into bitcoin:master Sep 7, 2021

bitcoin locked as resolved and limited conversation to collaborators Sep 7, 2022

wallet: Avoid locking cs_wallet recursively #19833

wallet: Avoid locking cs_wallet recursively #19833

Uh oh!

Conversation

promag commented Aug 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

hebasto commented Aug 30, 2020

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

promag commented Aug 31, 2020

Uh oh!

maflcko commented Aug 31, 2020

Uh oh!

hebasto commented Aug 31, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

promag commented Aug 31, 2020

Uh oh!

ryanofsky commented Aug 31, 2020

Uh oh!

ryanofsky commented Aug 31, 2020

Uh oh!

hebasto commented Aug 31, 2020

Uh oh!

promag commented Nov 7, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto Nov 12, 2020

Choose a reason for hiding this comment

Uh oh!

promag Nov 12, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto Nov 12, 2020

Choose a reason for hiding this comment

Uh oh!

promag Nov 12, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Sep 3, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

promag commented Sep 4, 2021

Uh oh!

Uh oh!

promag commented Aug 29, 2020 •

edited

Loading

DrahtBot commented Aug 29, 2020 •

edited

Loading