wallet: GetWalletTx and IsMine require cs_wallet lock #19289

pull promag wants to merge 3 commits into bitcoin:master from promag:2020-06-wallet-less-locks changing 3 files +45 −32

promag commented at 10:52 PM on June 15, 2020: member

This change removes some unlock/lock and lock/lock cases regarding GetWalletTx and IsMine overloads.

in src/interfaces/wallet.cpp:40 in b8282e20cc outdated

  36 | @@ -37,6 +37,7 @@ namespace {
  37 |  //! Construct wallet tx struct.
  38 |  WalletTx MakeWalletTx(CWallet& wallet, const CWalletTx& wtx)
  39 |  {
  40 | +    LOCK(wallet.cs_wallet);

promag commented at 10:54 PM on June 15, 2020:

Below IsMine is called for each input and output.

MarcoFalke commented at 11:23 PM on June 15, 2020: member

Is this a refactor or a bugfix or a potential bugfix?
promag commented at 12:00 AM on June 16, 2020: member

I'd say refactor. Only change in behavior is that now some races aren't possible, for instance, some import against MakeWalletTx. But I don't see these as bugfix.

Edit: well, without this change, if some transaction has multiple inputs/outputs for the same key, and this key is imported concurrently to MakeWalletTx, then the resulting WalletTx can be wrong. But this is very unlikely I think.
DrahtBot added the label Wallet on Jun 16, 2020
promag force-pushed on Jun 16, 2020
DrahtBot commented at 9:50 AM on June 16, 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:
- #18608 (refactor: Remove CAddressBookData::destdata by ryanofsky)
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 commented at 9:18 AM on July 14, 2020: member

@meshcollider ping.
wallet: GetWalletTx requires cs_wallet lock a13cafc6c6
wallet: IsMine overloads require cs_wallet lock d8441f30ff
promag force-pushed on Aug 16, 2020
promag commented at 11:07 PM on August 16, 2020: member

Removed 20fa4f1512c85bb967a3cb9edda9f80f46d5b11f to make this a light bug fix.
meshcollider commented at 3:23 AM on August 17, 2020: contributor

Code review ACK d8441f30ff57e4ae98cff6694c995eaffc19c51a
in src/wallet/wallet.cpp:1602 in d8441f30ff outdated
```
1598 | @@ -1597,6 +1599,7 @@ void CWalletTx::GetAmounts(std::list<COutputEntry>& listReceived,
1599 |          nFee = nDebit - nValueOut;
1600 |      }
1601 |  
1602 | +    LOCK(pwallet->cs_wallet);
```
ryanofsky commented at 8:35 PM on August 19, 2020:

In commit "wallet: IsMine overloads require cs_wallet lock" (d8441f30ff57e4ae98cff6694c995eaffc19c51a)

Maybe with this expanded lock scope. EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) could be added to IsChange(), and that method wouldn't need to lock cs_wallet recursively. Not sure how much extra work that is, however, so maybe it is a suggestion for a followup

promag commented at 9:15 PM on August 19, 2020:

Will look into it.

promag commented at 12:14 AM on August 21, 2020:

Pushed b8405b833ad28351c80fb10f6f896f974013fd9e which touches IsChange and GetChange only.
in src/wallet/wallet.cpp:1986 in a13cafc6c6 outdated
```
1982 | @@ -1983,6 +1983,7 @@ bool CWalletTx::IsTrusted(std::set<uint256>& trusted_parents) const
1983 |      if (!InMempool()) return false;
1984 |  
1985 |      // Trusted if all inputs are from us and are in the mempool:
1986 | +    LOCK(pwallet->cs_wallet);
```
ryanofsky commented at 8:45 PM on August 19, 2020:

In commit "wallet: GetWalletTx requires cs_wallet lock" (a13cafc6c6998baedf3c5766259c21fcd763b99e)

Adding a lock here doesn't seem like the best thing because this is a recursive function. So code above can run unlocked during the base call but locked during recursive calls, which is confusing and could make it harder to detect bugs.

Maybe instead of locking here, IsTrusted could get an EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) annotation. It seems like IsTrusted is only called a few places, and with cs_wallet already locked, so it wouldn't be a big change.

promag commented at 9:16 PM on August 19, 2020:

Good point, will address, thanks!

promag commented at 12:12 AM on August 21, 2020:

Done in #19773.
ryanofsky approved
ryanofsky commented at 8:49 PM on August 19, 2020: member

Code review ACK d8441f30ff57e4ae98cff6694c995eaffc19c51a. This is good. Simple changes that make cs_wallet locks simpler and safer. Left suggestions below, maybe for followups, fine to ignore them here
wallet: IsChange requires cs_wallet lock b8405b833a
ryanofsky approved
ryanofsky commented at 10:44 PM on August 24, 2020: member

Code review ACK b8405b833ad28351c80fb10f6f896f974013fd9e. Just new commit since last review changing IsChange GetChange locks and adding annotations
fanquake requested review from meshcollider on Aug 25, 2020
laanwj commented at 2:20 PM on August 27, 2020: member

Code review ACK b8405b833ad28351c80fb10f6f896f974013fd9e
laanwj merged this on Aug 27, 2020
laanwj closed this on Aug 27, 2020
promag deleted the branch on Aug 27, 2020
promag commented at 2:26 PM on August 27, 2020: member

Thanks
sidhujag referenced this in commit d0d5d4413a on Aug 28, 2020
Fabcien referenced this in commit 9712e14453 on Sep 14, 2021
Fabcien referenced this in commit 96bb65f202 on Sep 14, 2021
DrahtBot locked this on Feb 15, 2022

Contributors

Review Requested
meshcollider

Labels

Wallet

Linked (view graph)

#19773 wallet: Avoid recursive lock in IsTrusted

wallet: GetWalletTx and IsMine require cs_wallet lock #19289

Conflicts