Add compile time checking for cs_main runtime locking assertions #13083

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

   Add compile time checking for cs_main 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. practicalswift renamed this:
   Add compile time checking for all cs_main runtime locking assertions
   Add compile time checking for cs_main runtime locking assertions
   on May 3, 2018
4. practicalswift force-pushed on May 5, 2018
5. practicalswift force-pushed on May 14, 2018
6. 
   practicalswift commented at 1:34 pm on May 14, 2018: contributor

   Rebased!
7. practicalswift force-pushed on May 24, 2018
8. 
   practicalswift commented at 8:03 pm on May 24, 2018: contributor

   Rebased!
9. practicalswift force-pushed on May 29, 2018
10. 
    practicalswift commented at 10:04 pm on May 29, 2018: contributor

    Rebased!
11. in src/qt/transactiondesc.cpp:51 in 8a3505c25b outdated

    47@@ -48,7 +48,7 @@ QString TransactionDesc::FormatTxStatus(const interfaces::WalletTx& wtx, const i
    48     }
    49 }
    50 
    51-QString TransactionDesc::toHTML(interfaces::Node& node, interfaces::Wallet& wallet, TransactionRecord *rec, int unit)
    52+QString TransactionDesc::toHTML(interfaces::Node& node, interfaces::Wallet& wallet, TransactionRecord *rec, int unit) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
    

    ---

    ---

    

    MarcoFalke commented at 11:59 pm on May 29, 2018:

    I can’t see why any of the changes in the qt and rpc folders are needed. Please make sure you don’t add false positives.
12. practicalswift force-pushed on May 30, 2018
13. DrahtBot added the label Needs rebase on Jun 15, 2018
14. practicalswift force-pushed on Jun 15, 2018
15. 
    practicalswift commented at 8:15 pm on June 15, 2018: contributor

    Rebased! :-)
16. DrahtBot removed the label Needs rebase on Jun 19, 2018
17. DrahtBot added the label Needs rebase on Jul 9, 2018
18. practicalswift force-pushed on Jul 11, 2018
19. 
    practicalswift commented at 11:39 pm on July 11, 2018: contributor

    Rebased!
20. 
    MarcoFalke commented at 0:51 am on July 12, 2018: member

    Travis output:

    0bench/block_assemble.cpp:102:18: error: calling function 'AcceptToMemoryPool' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
    1        bool ret{::AcceptToMemoryPool(::mempool, state, txr, nullptr /* pfMissingInputs */, nullptr /* plTxnReplaced */, false /* bypass_limits */, /* nAbsurdFee */ 0)};
    2                 ^
    31 error generated.
    

21. DrahtBot removed the label Needs rebase on Jul 12, 2018
22. practicalswift force-pushed on Jul 12, 2018
23. 
    practicalswift commented at 8:56 am on July 12, 2018: contributor

    @MarcoFalke Oh thanks! Now fixed. Also incorporated the CChainState changes you suggested in the other PR. Please re-review :-)
24. DrahtBot added the label Needs rebase on Jul 14, 2018
25. in src/validation.h:416 in dbb3af8b50 outdated

    412@@ -413,7 +413,7 @@ bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const CBlockIndex* pindex
    413 /** Context-independent validity checks */
    414 bool CheckBlock(const CBlock& block, CValidationState& state, const Consensus::Params& consensusParams, bool fCheckPOW = true, bool fCheckMerkleRoot = true);
    415 
    416-/** Check a block is completely valid from start to finish (only works on top of our current best block) */
    417+/** Check a block is completely valid from start to finish (only works on top of our current best block, with cs_main held) */
    

    ---

    ---

    

    MarcoFalke commented at 6:15 pm on July 14, 2018:

    Unwanted change?
26. practicalswift force-pushed on Jul 15, 2018
27. practicalswift force-pushed on Jul 15, 2018
28. 
    practicalswift commented at 11:18 pm on July 15, 2018: contributor

    @MarcoFalke Thanks! Locking comment removed. Also rebased. Please re-review :-)
29. practicalswift force-pushed on Jul 15, 2018
30. DrahtBot removed the label Needs rebase on Jul 16, 2018
31. DrahtBot added the label Needs rebase on Jul 24, 2018
32. practicalswift force-pushed on Aug 1, 2018
33. DrahtBot removed the label Needs rebase on Aug 1, 2018
34. practicalswift force-pushed on Aug 13, 2018
35. 
    practicalswift commented at 11:44 am on August 13, 2018: contributor

    Rebase number seven performed! :-)
36. 
    MarcoFalke commented at 1:25 pm on August 13, 2018: member

    utACK 2cb4b9a4bdec7e70e188e4cbf5588e0957112647
37. in src/validationinterface.h:10 in 2cb4b9a4bd outdated

     6@@ -7,10 +7,12 @@
     7 #define BITCOIN_VALIDATIONINTERFACE_H
     8 
     9 #include <primitives/transaction.h> // CTransaction(Ref)
    10+#include <sync.h>
    

    ---

    ---

    

    MarcoFalke commented at 1:19 am on August 23, 2018:

    Could use a forward decl of class CCriticalSection; instead?

    ---

    

    practicalswift commented at 9:33 pm on August 25, 2018:

    @MarcoFalke sync.h is needed for LOCKS_EXCLUDED, etc too :-)
38. DrahtBot added the label Needs rebase on Aug 25, 2018
39. 
    DrahtBot commented at 4:40 pm on August 25, 2018: member
40. practicalswift force-pushed on Aug 25, 2018
41. 
    practicalswift commented at 9:38 pm on August 25, 2018: contributor

    @MarcoFalke Rebased. Please re-review. Any outstanding issues? :-)
42. 
    MarcoFalke commented at 9:44 pm on August 25, 2018: member

    re-utACK 5cc07926d82b4fecdc7ec6b68f113b709cfa1938
43. 
    practicalswift commented at 9:59 pm on August 25, 2018: contributor

    If this is merged then a lot of the other locking annotations PR:s would be drastically reduced since they repeat cs_main related annotations already contained in this PR. Review of this specific locking annotations PR would thus be extra appreciated :-)

    Reviewers of previous locking annotations PR:s – @Empact, @laanwj, @TheBlueMatt or @promag – would you mind take a look at this PR? :-)

44. 
    MarcoFalke commented at 10:04 pm on August 25, 2018: member

    @practicalswift Travis failed here
45. Add compile time checking for all cs_main runtime locking assertions 9e0a514112
46. practicalswift force-pushed on Aug 25, 2018
47. MarcoFalke merged this on Aug 25, 2018
48. MarcoFalke closed this on Aug 25, 2018

    ---

49. MarcoFalke referenced this in commit 91186e5984 on Aug 25, 2018
50. MarcoFalke removed the label Needs rebase on Aug 25, 2018
51. in src/txmempool.cpp:500 in 9e0a514112

    496@@ -497,7 +497,7 @@ void CTxMemPool::removeRecursive(const CTransaction &origTx, MemPoolRemovalReaso
    497     }
    498 }
    499 
    500-void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight, int flags)
    501+void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
    

    ---

    ---

    

    MarcoFalke commented at 10:39 pm on August 25, 2018:

    Looks like this was added in the cpp file. Would you mind moving it to the header file?
52. in src/wallet/wallet.cpp:4413 in 9e0a514112

    4409@@ -4410,7 +4410,7 @@ bool CMerkleTx::IsImmatureCoinBase() const
    4410     return GetBlocksToMaturity() > 0;
    4411 }
    4412 
    4413-bool CWalletTx::AcceptToMemoryPool(const CAmount& nAbsurdFee, CValidationState& state)
    4414+bool CWalletTx::AcceptToMemoryPool(const CAmount& nAbsurdFee, CValidationState& state) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
    

    ---

    ---

    

    MarcoFalke commented at 10:39 pm on August 25, 2018:

    Same here
53. in src/interfaces/wallet.cpp:60 in 9e0a514112

    56@@ -57,7 +57,7 @@ class PendingWalletTxImpl : public PendingWalletTx
    57 };
    58 
    59 //! Construct wallet tx struct.
    60-WalletTx MakeWalletTx(CWallet& wallet, const CWalletTx& wtx)
    61+static WalletTx MakeWalletTx(CWallet& wallet, const CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
    

    ---

    ---

    

    ryanofsky commented at 3:20 pm on August 27, 2018:

    These functions are in an anonymous namespace so adding static here has no effect.

    I think it would be better to avoid using static in cases like this. Anonymous namespaces provide a reliable way of hiding all types of c++ linker symbols, while static keyword works on a non-member symbols (and was actually deprecated in C++03, though it is undeprecated now: https://stackoverflow.com/questions/8460327/why-are-anonymous-namespaces-not-a-sufficient-replacement-for-namespace-static).

    ---

    

    practicalswift commented at 3:26 pm on August 27, 2018:

    @ryanofsky Yes, I know. I simply missed the fact that it was in an anonymous namespace :-)

    Thanks for noticing though :-)

54. practicalswift deleted the branch on Apr 10, 2021
55. Munkybooty referenced this in commit c69df3bc97 on Jun 30, 2021
56. Munkybooty referenced this in commit c09e919c67 on Jun 30, 2021
57. Munkybooty referenced this in commit 8a6e8d7a28 on Jul 1, 2021
58. Munkybooty referenced this in commit a898756320 on Jul 2, 2021
59. Munkybooty referenced this in commit 5c9d57b8b8 on Jul 4, 2021
60. UdjinM6 referenced this in commit 0f26a0df06 on Jul 7, 2021
61. Munkybooty referenced this in commit 23f332711e on Jul 7, 2021
62. Munkybooty referenced this in commit eae3b9abc4 on Jul 7, 2021
63. Munkybooty referenced this in commit 06e467d037 on Jul 8, 2021
64. gades referenced this in commit 956217b1c7 on May 1, 2022
65. DrahtBot locked this on Aug 18, 2022

Contributors
practicalswift MarcoFalke DrahtBot ryanofsky

Labels
Refactoring