wallet: GetWalletTx and IsMine require cs_wallet lock #19289

promag · 2020-06-15T22:52:06Z

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

promag · 2020-06-15T22:54:10Z

src/interfaces/wallet.cpp

Below IsMine is called for each input and output.

maflcko · 2020-06-15T23:23:20Z

Is this a refactor or a bugfix or a potential bugfix?

promag · 2020-06-16T00:00:46Z

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 · 2020-06-16T09:50:38Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

refactor: Remove CAddressBookData::destdata #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 · 2020-07-14T09:18:57Z

@meshcollider ping.

promag · 2020-08-16T23:07:22Z

Removed 20fa4f1512c85bb967a3cb9edda9f80f46d5b11f to make this a light bug fix.

meshcollider

Code review ACK d8441f3

ryanofsky

Code review ACK d8441f3. This is good. Simple changes that make cs_wallet locks simpler and safer. Left suggestions below, maybe for followups, fine to ignore them here

ryanofsky · 2020-08-19T20:35:00Z

src/wallet/wallet.cpp

        nFee = nDebit - nValueOut;
    }

+    LOCK(pwallet->cs_wallet);


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

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

Will look into it.

Pushed b8405b8 which touches IsChange and GetChange only.

ryanofsky · 2020-08-19T20:45:58Z

src/wallet/wallet.cpp

    if (!InMempool()) return false;

    // Trusted if all inputs are from us and are in the mempool:
+    LOCK(pwallet->cs_wallet);


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

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.

Good point, will address, thanks!

Done in #19773.

ryanofsky

Code review ACK b8405b8. Just new commit since last review changing IsChange GetChange locks and adding annotations

laanwj · 2020-08-27T14:20:54Z

Code review ACK b8405b8

promag · 2020-08-27T14:26:53Z

Thanks

… lock b8405b8 wallet: IsChange requires cs_wallet lock (João Barbosa) d8441f3 wallet: IsMine overloads require cs_wallet lock (João Barbosa) a13cafc wallet: GetWalletTx requires cs_wallet lock (João Barbosa) Pull request description: This change removes some unlock/lock and lock/lock cases regarding `GetWalletTx` and `IsMine` overloads. ACKs for top commit: laanwj: Code review ACK b8405b8 ryanofsky: Code review ACK b8405b8. Just new commit since last review changing IsChange GetChange locks and adding annotations Tree-SHA512: 40d37c4fe5d10a1407f57d899d5822bb285633d8dbfad8afcf15a9b41b428ed9971a9a7b1aae84318371155132df3002699a15dab56e004527d50c889829187d

Summary: PR description: > This change removes some unlock/lock and lock/lock cases regarding GetWalletTx and IsMine overloads. This is a backport of [[bitcoin/bitcoin#19289 | core#19289]] [1/3] bitcoin/bitcoin@a13cafc Test Plan: With TSAN: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10095

Summary: This is a backport of [[bitcoin/bitcoin#19289 | core#19289]] [2/3] bitcoin/bitcoin@d8441f3 Depends on D10095 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10096

promag commented Jun 15, 2020

View reviewed changes

src/interfaces/wallet.cpp Outdated

Copy link

Contributor Author

promag Jun 15, 2020 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Below IsMine is called for each input and output.

DrahtBot added the Wallet label Jun 16, 2020

promag force-pushed the 2020-06-wallet-less-locks branch from b8282e2 to 20fa4f1 Compare June 16, 2020 00:19

promag added 2 commits August 17, 2020 00:06

wallet: GetWalletTx requires cs_wallet lock

a13cafc

wallet: IsMine overloads require cs_wallet lock

d8441f3

promag force-pushed the 2020-06-wallet-less-locks branch from 20fa4f1 to d8441f3 Compare August 16, 2020 23:06

promag mentioned this pull request Aug 16, 2020

wallet: Avoid multiple BerkeleyBatch in DelAddressBook #19738

Merged

meshcollider reviewed Aug 17, 2020

View reviewed changes

ryanofsky approved these changes Aug 19, 2020

View reviewed changes

wallet: IsChange requires cs_wallet lock

b8405b8

promag mentioned this pull request Aug 21, 2020

wallet: Avoid recursive lock in IsTrusted #19773

Merged

ryanofsky approved these changes Aug 24, 2020

View reviewed changes

fanquake requested a review from meshcollider August 25, 2020 07:06

laanwj merged commit 91af7ef into bitcoin:master Aug 27, 2020

promag deleted the 2020-06-wallet-less-locks branch August 27, 2020 14:26

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

wallet: GetWalletTx and IsMine require cs_wallet lock #19289

wallet: GetWalletTx and IsMine require cs_wallet lock #19289

Uh oh!

Conversation

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

Uh oh!

Uh oh!

promag Jun 15, 2020 • 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!

maflcko commented Jun 15, 2020

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

promag commented Jul 14, 2020

Uh oh!

promag commented Aug 16, 2020

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

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!

ryanofsky Aug 19, 2020

Choose a reason for hiding this comment

Uh oh!

promag Aug 19, 2020

Choose a reason for hiding this comment

Uh oh!

promag Aug 21, 2020

Choose a reason for hiding this comment

Uh oh!

ryanofsky Aug 19, 2020

Choose a reason for hiding this comment

Uh oh!

promag Aug 19, 2020

Choose a reason for hiding this comment

Uh oh!

promag Aug 21, 2020

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Aug 27, 2020

Uh oh!

promag commented Aug 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

promag commented Jun 15, 2020 •

edited

Loading

promag Jun 15, 2020 •

edited

Loading

promag commented Jun 16, 2020 •

edited

Loading

DrahtBot commented Jun 16, 2020 •

edited

Loading

ryanofsky left a comment •

edited

Loading