wallet: Avoid locking cs_wallet recursively

promag commented at 2:37 pm on August 29, 2020: member

This is another small change towards non recursive wallet lock.

DrahtBot added the label Wallet on Aug 29, 2020

DrahtBot commented at 6:30 pm on August 29, 2020: member

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.

promag force-pushed on Aug 29, 2020

promag force-pushed on Aug 30, 2020

hebasto commented at 7:46 am on August 30, 2020: member

Concept ACK.

in src/interfaces/wallet.cpp:379 in d306298e4f outdated

370@@ -370,7 +371,11 @@ class WalletImpl : public Wallet
371         balances = getBalances();
372         return true;
373     }
374-    CAmount getBalance() override { return m_wallet->GetBalance().m_mine_trusted; }
375+    CAmount getBalance() override
376+    {
377+        LOCK(m_wallet->cs_wallet);
378+        return m_wallet->GetBalance().m_mine_trusted;
379+    }

hebasto commented at 8:45 am on August 30, 2020:

Probably, it would be a cleaner code with two methods:

getBalance() EXCLUSIVE_LOCKS_REQUIRED(!m_wallet->cs_wallet)
getBalance_cs() EXCLUSIVE_LOCKS_REQUIRED(m_wallet->cs_wallet)

See:

#19238 (comment)
a7b1a649ec473bd9811e34cf489bd88b3c3d6406 (#19238)

hebasto commented at 10:48 am on August 30, 2020:

It could replace 6 instances of added WITH_LOCK() :)

promag commented at 12:46 pm on August 31, 2020:

Do you mean getBalance or GetBalance? I don’t see why we want that on interfaces::Wallet. If you mean GetBalance then ATM I tend to prefer explicit WITH_LOCK/LOCK for so few cases. If we go that way then I’d prefer to overload GetBalance(Lock).

hebasto commented at 1:49 pm on August 31, 2020:

Do you mean getBalance or GetBalance?

My suggestion is wrong and confusing. Please ignore it.

If we go that way then I’d prefer to overload GetBalance(Lock).

I’m afraid Clang Thread Safety Analysis won’t understand it due to https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#no-alias-analysis

promag force-pushed on Aug 30, 2020

hebasto commented at 2:25 pm on August 31, 2020: member

ACK 9c5a28c343aaf38e596bb8fc43743527690bf3bd “wallet: GetLabelAddresses requires cs_wallet lock “ ACK 7a138a8054ff4f76a0c9d89fd89f5ee04eae793e “wallet: AddWalletDescriptor requires cs_wallet lock” Approach NACK a98d192181e657211754a6e4f5afd78059cee238 “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 commented at 2:33 pm on August 31, 2020: member

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?

MarcoFalke commented at 2:34 pm on August 31, 2020: member

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 commented at 2:41 pm on August 31, 2020: member

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

hebasto commented at 2:43 pm on August 31, 2020: member

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

promag commented at 2:45 pm on August 31, 2020: member

Actually moving implementation class declaration into a header will help.

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

ryanofsky commented at 3:05 pm on August 31, 2020: member

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 commented at 3:11 pm on August 31, 2020: member

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 commented at 5:33 pm on August 31, 2020: member

@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 force-pushed on Nov 7, 2020

promag commented at 11:51 am on November 7, 2020: member

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

DrahtBot added the label Needs rebase on Nov 9, 2020

in src/interfaces/wallet.cpp:351 in fa2cf369e0 outdated

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

hebasto commented at 9:56 am on November 12, 2020:

Could the lock be bounded to GetBalance only?

promag commented at 10:20 am on November 12, 2020:

WITH_LOCK?

hebasto commented at 10:22 am on November 12, 2020:

WITH_LOCK?

That is my thought.

promag commented at 3:18 pm on November 12, 2020:

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

hebasto commented at 10:00 am on November 12, 2020: member

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.

promag force-pushed on Nov 12, 2020

DrahtBot removed the label Needs rebase on Nov 12, 2020

hebasto approved

hebasto commented at 10:29 am on November 12, 2020: member

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

DrahtBot added the label Needs rebase on Dec 1, 2020

ryanofsky approved

ryanofsky commented at 0:47 am on January 18, 2021: member

Code review ACK 97dbdcd7eb1cf8c10f9cfd92c23ac9ef58d1d5cb. 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 commented at 3:31 pm on September 3, 2021: member

@promag

A friendly reminder to rebase :)

Happy to re-ACK then.

wallet: GetLabelAddresses requires cs_wallet lock

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

32d036e8da

wallet: AddWalletDescriptor requires cs_wallet lock

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

5fabde6fad

promag force-pushed on Sep 3, 2021

DrahtBot removed the label Needs rebase on Sep 3, 2021

ryanofsky approved

ryanofsky commented at 5:56 pm on September 3, 2021: member

Code review ACK 97dbdcd7eb1cf8c10f9cfd92c23ac9ef58d1d5cb. 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 approved

hebasto commented at 6:59 pm on September 3, 2021: member

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

promag commented at 10:39 am on September 4, 2021: member

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

MarcoFalke merged this on Sep 7, 2021

MarcoFalke closed this on Sep 7, 2021

sidhujag referenced this in commit f9777137e4 on Sep 7, 2021

DrahtBot locked this on Sep 7, 2022

wallet: Avoid locking cs_wallet recursively #19833

Conflicts