wallet: Add compile time checking for cs_wallet runtime locking assertions #13081

1. 
   practicalswift commented at 9:17 pm on April 25, 2018: contributor

   Add compile time checking for cs_wallet runtime locking assertions.

   This PR is a subset of #12665. The PR was broken up to make reviewing easier.

   The intention is that literally all EXCLUSIVE_LOCKS_REQUIRED/LOCKS_EXCLUDED:s added in this PR should follow either directly or indirectly from AssertLockHeld(…)/AssertLockNotHeld(…):s already existing in the repo.

   Consider the case where function A(…) contains AssertLockHeld(cs_foo) (without first locking cs_foo in A), and that B(…) calls A(…) (without first locking cs_main):

   • It directly follows that: A(…) should have an EXCLUSIVE_LOCKS_REQUIRED(cs_foo) annotation.
   • It indirectly follows that: B(…) should have an EXCLUSIVE_LOCKS_REQUIRED(cs_foo) annotation.
2. fanquake added the label Refactoring on Apr 25, 2018
3. 
   MarcoFalke commented at 10:16 pm on April 26, 2018: member

   utACK c51b9e36e98ed1fd2151d923ae233970c4c39713
4. 
   TheBlueMatt commented at 0:16 am on April 28, 2018: member

   Looks like GetConflicts was missed,
5. practicalswift force-pushed on Apr 29, 2018
6. 
   practicalswift commented at 8:05 pm on April 29, 2018: contributor

   @TheBlueMatt Correct! Added! @MarcoFalke @TheBlueMatt Please re-review! :-)
7. in src/wallet/wallet.cpp:1849 in 775b74e1e3 outdated

   1845@@ -1846,7 +1846,7 @@ bool CWalletTx::RelayWalletTransaction(CConnman* connman)
   1846     return false;
   1847 }
   1848 
   1849-std::set<uint256> CWalletTx::GetConflicts() const
   1850+std::set<uint256> CWalletTx::GetConflicts() const EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
   

   ---

   ---

   

   MarcoFalke commented at 9:00 pm on April 29, 2018:

   Add this to the header file instead?

   ---

   

   practicalswift commented at 6:16 am on April 30, 2018:

   Moving it to the header file would generate a member access into an at that stage incomplete type CWallet I’m afraid. Any suggestion?

   ---

   

   MarcoFalke commented at 5:01 pm on April 30, 2018:

   You can solve it by moving stuff around in the header file (i.e. make a forward declaration of CWalletTx, move the CWalletTx to the line after CWallet, move methods that still depend on the type to the cpp file, then add all the locking annotations. Note that just adding the annotation here is not sufficient, since I get a warning in wallet/rpcwallet.cpp…)

   Personally, I had the feeling that c51b9e3 was a clear and obvious step in the right direction. I don’t like to hold back the pull request based on that a single method was “missed”. That can be fixed up later in a separate pull … I’d suggest resetting hard to c51b9e3 and merge it

   ---

   

   MarcoFalke commented at 6:30 pm on May 2, 2018:

   @practicalswift @TheBlueMatt Any further thoughts here?

   ---

   

   practicalswift commented at 6:40 pm on May 2, 2018:

   @MarcoFalke Good point. I’ll revert to c51b9e3.
8. practicalswift renamed this:
   Add compile time checking for all cs_wallet runtime locking assertions
   wallet: Add compile time checking for all cs_wallet runtime locking assertions
   on Apr 30, 2018
9. practicalswift force-pushed on May 2, 2018
10. practicalswift force-pushed on May 2, 2018
11. 
    practicalswift commented at 6:52 pm on May 2, 2018: contributor

    Now reverted to c51b9e36e98ed1fd2151d923ae233970c4c39713 as suggested by @MarcoFalke. Will add GetConflicts(…) in a later PR.

    Ready for merge? :-)

12. 
    MarcoFalke commented at 6:57 pm on May 2, 2018: member

    @ryanofsky or @jnewbery You are doing a lot with the wallet lately. Mind taking a look here?
13. practicalswift renamed this:
    wallet: Add compile time checking for all cs_wallet runtime locking assertions
    wallet: Add compile time checking for cs_wallet runtime locking assertions
    on May 3, 2018
14. practicalswift force-pushed on May 5, 2018
15. 
    MarcoFalke commented at 1:57 pm on May 5, 2018: member

    re-utACK 1b34bda4c9beb993e2378a5c8cb22f92c421eba0
16. 
    ryanofsky commented at 7:33 pm on May 11, 2018: member

    @ryanofsky or @jnewbery You are doing a lot with the wallet lately. Mind taking a look here?

    utACK 1b34bda4c9beb993e2378a5c8cb22f92c421eba0.

    I’m not very familiar with these annotations, but spot checking a few of them, they looked correct, and I assume there is basically no harm if an annotation is added incorrectly (other than maybe having to remove the annotation in a future change?). I also didn’t check for missing annotations, because that seemed equally harmless.

17. 
    MarcoFalke commented at 12:53 pm on May 14, 2018: member

    Needs rebase
18. Add compile time checking for all cs_wallet runtime locking assertions 66b0b1b2a6
19. practicalswift force-pushed on May 14, 2018
20. 
    practicalswift commented at 12:57 pm on May 14, 2018: contributor

    Rebased! Please re-review :-)
21. 
    MarcoFalke commented at 1:15 pm on May 14, 2018: member

    re-utACK 66b0b1b2a6caf9baa2877e414414ec3b37121b8d
22. MarcoFalke merged this on May 14, 2018
23. MarcoFalke closed this on May 14, 2018

    ---

24. MarcoFalke referenced this in commit 0dec5b5af4 on May 14, 2018
25. jasonbcox referenced this in commit ec813bc914 on Sep 13, 2019
26. jonspock referenced this in commit de726375e2 on Dec 22, 2019
27. proteanx referenced this in commit b399b79eaa on Dec 23, 2019
28. PastaPastaPasta referenced this in commit f671d8e2c0 on Jun 17, 2020
29. PastaPastaPasta referenced this in commit 5aa65d0e68 on Jun 27, 2020
30. PastaPastaPasta referenced this in commit 3cfa14c5af on Jun 28, 2020
31. PastaPastaPasta referenced this in commit 4d49ad803c on Jun 29, 2020
32. PastaPastaPasta referenced this in commit 795076168b on Jul 1, 2020
33. practicalswift deleted the branch on Apr 10, 2021
34. gades referenced this in commit cb0f86cd7b on Mar 10, 2022
35. DrahtBot locked this on Aug 18, 2022

Contributors
practicalswift MarcoFalke TheBlueMatt ryanofsky

Labels
Refactoring