Add compile time checking for cs_main runtime locking assertions #13083

  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.

 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?

   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 :-)

 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?

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

  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 :-)