mempool: Add compile time checking for ::mempool.cs runtime locking assertions #13080

1. 
   practicalswift commented at 8:59 pm on April 25, 2018: contributor

   Add compile time checking for ::mempool.cs 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. in src/rpc/blockchain.cpp:1455 in 65379d3c2d outdated

   1451@@ -1452,7 +1452,7 @@ UniValue preciousblock(const JSONRPCRequest& request)
   1452     return NullUniValue;
   1453 }
   1454 
   1455-UniValue invalidateblock(const JSONRPCRequest& request)
   1456+static UniValue invalidateblock(const JSONRPCRequest& request) EXCLUSIVE_LOCKS_REQUIRED(::mempool.cs)
   

   ---

   ---

   

   MarcoFalke commented at 2:14 am on April 26, 2018:

   All the locks are taken. I don’t see why this is necessary.

   ---

   

   practicalswift commented at 3:45 am on April 26, 2018:

   @MarcoFalke invalidateblock(…) calls InvalidateBlock(…) without first locking ::mempool.cs. Calling InvalidateBlock(…) requires holding mutex mempool.cs exclusively. Then it follows that calling invalidateblock(…) should require ::mempool.cs too which is what the annotation says? What am I missing? :-)

   ---

   

   MarcoFalke commented at 11:14 am on April 26, 2018:

   Calling InvalidateBlock(…) requires holding mutex mempool.cs

   Yeah, I looked and it seems all the locks are taken here, which one did I miss?

   ---

   

   practicalswift commented at 12:50 pm on April 26, 2018:

   Ah, sorry. The InvalidateBlock(…) annotation seems incorrect. Thanks!
4. practicalswift force-pushed on Apr 26, 2018
5. 
   practicalswift commented at 12:50 pm on April 26, 2018: contributor

   @MarcoFalke Updated! Please re-review :-)
6. practicalswift force-pushed on Apr 26, 2018
7. 
   MarcoFalke commented at 1:06 pm on April 26, 2018: member

   utACK 03774eff0e6f5242f02125cecb4d61fa74c00300
8. in src/rpc/blockchain.cpp:1466 in 03774eff0e outdated

   1451@@ -1452,7 +1452,7 @@ UniValue preciousblock(const JSONRPCRequest& request)
   1452     return NullUniValue;
   1453 }
   1454 
   1455-UniValue invalidateblock(const JSONRPCRequest& request)
   1456+static UniValue invalidateblock(const JSONRPCRequest& request)
   

   ---

   ---

   

   TheBlueMatt commented at 0:06 am on April 28, 2018:

   How’d this slip in here? I’m kinda surprised it even linked with this.
9. practicalswift force-pushed on Apr 28, 2018
10. 
    practicalswift commented at 9:10 am on April 28, 2018: contributor

    @TheBlueMatt Thanks! Updated. Please re-review :-)
11. 
    MarcoFalke commented at 1:13 pm on April 28, 2018: member

    re-utACK aa8341e3ae934587149dd609b542b21f9cf6017f (Only change was to get rid of unrelated change)
12. practicalswift renamed this:
    Add compile time checking for all ::mempool.cs runtime locking assertions
    mempool: Add compile time checking for all ::mempool.cs runtime locking assertions
    on Apr 30, 2018
13. 
    TheBlueMatt commented at 6:29 pm on May 2, 2018: member

    Missing CheckSequenceLocks.
14. 
    practicalswift commented at 6:41 pm on May 2, 2018: contributor

    @TheBlueMatt Given that @MarcoFalke has utACK:ed aa8341e and in light of #13081 (review), what about adding CheckSequenceLocks(…) (and GetConflicts(…) from #13081) in a follow-up PR post merge where I re-check all the AssertLockHeld(…):s manually? That’s OK? :-)

    Both CheckSequenceLocks and GetConflicts will require moving stuff around in the header files in order to avoid member access into incomplete types.

15. practicalswift renamed this:
    mempool: Add compile time checking for all ::mempool.cs runtime locking assertions
    mempool: Add compile time checking for ::mempool.cs runtime locking assertions
    on May 3, 2018
16. Add compile time checking for all ::mempool.cs runtime locking assertions cbba1d2da4
17. practicalswift force-pushed on May 5, 2018
18. 
    MarcoFalke commented at 2:32 pm on May 5, 2018: member

    re-utACK cbba1d2da42a9c813c50ea3b3c13ec9a518bc147
19. MarcoFalke merged this on May 5, 2018
20. MarcoFalke closed this on May 5, 2018

    ---

21. MarcoFalke referenced this in commit 66cc47be98 on May 5, 2018
22. PastaPastaPasta referenced this in commit 8784e4b82a on Jun 17, 2020
23. PastaPastaPasta referenced this in commit 1ca8c77592 on Jun 27, 2020
24. PastaPastaPasta referenced this in commit 2ba0fb5ad8 on Jun 28, 2020
25. practicalswift deleted the branch on Apr 10, 2021
26. gades referenced this in commit e0e5cea096 on Mar 16, 2022
27. DrahtBot locked this on Aug 18, 2022

Contributors
practicalswift MarcoFalke TheBlueMatt

Labels
Refactoring