wallet: GetWalletTx and IsMine require cs_wallet lock #19289

1. 
   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.
2. 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.
3. 
   MarcoFalke commented at 11:23 pm on June 15, 2020: member

   Is this a refactor or a bugfix or a potential bugfix?
4. 
   promag commented at 0: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.

5. DrahtBot added the label Wallet on Jun 16, 2020
6. promag force-pushed on Jun 16, 2020
7. 
   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.

8. 
   promag commented at 9:18 am on July 14, 2020: member

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

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

    Code review ACK d8441f30ff57e4ae98cff6694c995eaffc19c51a
14. 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 0:14 am on August 21, 2020:

    Pushed b8405b833ad28351c80fb10f6f896f974013fd9e which touches IsChange and GetChange only.
15. 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 0:12 am on August 21, 2020:

    Done in #19773.
16. ryanofsky approved
17. 
    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
18. wallet: IsChange requires cs_wallet lock b8405b833a
19. ryanofsky approved
20. 
    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
21. fanquake requested review from meshcollider on Aug 25, 2020
22. 
    laanwj commented at 2:20 pm on August 27, 2020: member

    Code review ACK b8405b833ad28351c80fb10f6f896f974013fd9e
23. laanwj merged this on Aug 27, 2020
24. laanwj closed this on Aug 27, 2020

    ---

25. promag deleted the branch on Aug 27, 2020
26. 
    promag commented at 2:26 pm on August 27, 2020: member

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

Contributors
promag MarcoFalke DrahtBot meshcollider ryanofsky laanwj

Review Requested
meshcollider

Labels
Wallet