…h cleanup

It fixes return reference to const variable under mutex which is not a good idea

…mplate lambdas

… TODO

…y after releasing the mutex that guards it

32a9f13 wallet: avoid returning a reference to vMasterKey after releasing the mutex that guards it (Vasil Dimov)

Pull request description:

  `CWallet::GetEncryptionKey()` would return a reference to the internal
  `CWallet::vMasterKey`, guarded by `CWallet::cs_wallet`, which is unsafe.

  Returning a copy would be a shorter solution, but could have security
  implications of the master key remaining somewhere in the memory even
  after `CWallet::Lock()` (the current calls to
  `CWallet::GetEncryptionKey()` are safe, but that is not future proof).

  So, instead of `EncryptSecret(m_storage.GetEncryptionKey(), ...)`
  change the `GetEncryptionKey()` method to provide the encryption
  key to a given callback:
  `m_storage.WithEncryptionKey([](const CKeyingMaterial& k) { EncryptSecret(k, ...); })`

  This silences the following (clang 18):

  ```
  wallet/wallet.cpp:3520:12: error: returning variable 'vMasterKey' by reference requires holding mutex 'cs_wallet' [-Werror,-Wthread-safety-reference-return]
   3520 |     return vMasterKey;
        |            ^
  ```

  ---
  _Previously this PR modified both ArgsManager and wallet code. But the ArgsManager commit bitcoin@856c887 was merged in bitcoin#29040 so now this only affects wallet code. The previous PR description was:_

  Avoid this unsafe pattern from `ArgsManager` and `CWallet`:

  ```cpp
  class A
  {
      Mutex mutex;
      Foo member GUARDED_BY(mutex);
      const Foo& Get()
      {
          LOCK(mutex);
          return member;
      } // callers of `Get()` will have access to `member` without owning the mutex.
  ```

ACKs for top commit:
  achow101:
    ACK 32a9f13
  ryanofsky:
    Code review ACK 32a9f13. This seems like a potentially real race condition, and the fix here is pretty simple.
  furszy:
    ACK 32a9f13

Tree-SHA512: 133da84691642afc1a73cf14ad004a7266cb4be1a6a3ec634d131dca5dbcdef52522c1d5eb04f5b6c4e06e1fc3e6ac57315f8fe1e207b464ca025c2b4edefdc1

ea74e10 doc: Add best practice for annotating/asserting locks (Hennadii Stepanov)
2ee7743 sync.h: Make runtime lock checks require compile-time lock checks (Anthony Towns)
23d71d1 Do not hide compile-time thread safety warnings (Hennadii Stepanov)
3ddc150 Add missed thread safety annotations (Hennadii Stepanov)
af9ea55 Use LockAssertion utility class instead of AssertLockHeld() (Hennadii Stepanov)

Pull request description:

  On the way of transit from `RecursiveMutex` to `Mutex` (see bitcoin#19303) it is crucial to have run-time `AssertLockHeld()` assertion that does _not_ hide compile-time Clang Thread Safety Analysis warnings.

  On master (65e4eca) using `AssertLockHeld()` could hide Clang Thread Safety Analysis warnings, e.g., with the following patch applied:
  ```diff
  --- a/src/txmempool.h
  +++ b/src/txmempool.h
  @@ -607,7 +607,7 @@ public:
       void addUnchecked(const CTxMemPoolEntry& entry, setEntries& setAncestors, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main);

       void removeRecursive(const CTransaction& tx, MemPoolRemovalReason reason) EXCLUSIVE_LOCKS_REQUIRED(cs);
  -    void removeForReorg(const CCoinsViewCache* pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main);
  +    void removeForReorg(const CCoinsViewCache* pcoins, unsigned int nMemPoolHeight, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
       void removeConflicts(const CTransaction& tx) EXCLUSIVE_LOCKS_REQUIRED(cs);
       void removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int nBlockHeight) EXCLUSIVE_LOCKS_REQUIRED(cs);

  ```
  Clang compiles the code without any thread safety warnings.

  See "Add missed thread safety annotations" commit for the actual thread safety warnings that are fixed in this PR.

ACKs for top commit:
  MarcoFalke:
    ACK ea74e10 🎙
  jnewbery:
    ACK ea74e10
  ajtowns:
    ACK ea74e10

Tree-SHA512: 8cba996e526751a1cb0e613c0cc1b10f027a3e9945fbfb4bd30f6355fd36b9f9c2e1e95ed3183fc254b42df7c30223278e18e5bdb5e1ef85db7fef067595d447

…globals

fa5eabe refactor: Remove negative lock annotations from globals (MarcoFalke)

Pull request description:

  They only make sense for mutexes that are private members. Until cs_main is a private member the negative annotations should be replaced by excluded annotations, which are optional.

ACKs for top commit:
  sipa:
    utACK fa5eabe
  ajtowns:
    ACK fa5eabe
  hebasto:
    ACK fa5eabe
  vasild:
    ACK fa5eabe

Tree-SHA512: 06f8a200304f81533010efcc42d9f59b8c4d0ae355920c0a28efb6fa161a3e3e68f2dfffb0c009afd9c2501e6a293c6e5a419a64d718f1f4e79668ab2ab1fcdc

…e LockAssertion

0bd1184 Remove unused LockAssertion struct (Hennadii Stepanov)
ab2a442 Replace LockAssertion with a proper thread safety annotations (Hennadii Stepanov)
73f71e1 refactor: Use explicit function type instead of template (Hennadii Stepanov)

Pull request description:

  This PR replaces `LockAssertion` with `AssertLockHeld`, and removes `LockAssertion`.

  This PR is compared with alternatives in https://github.com/bitcoin-core/bitcoin-devwiki/wiki/AssertLockHeld-PRs

ACKs for top commit:
  MarcoFalke:
    ACK 0bd1184
  ajtowns:
    ACK 0bd1184
  vasild:
    ACK 0bd1184

Tree-SHA512: ef7780dd689faf0bb479fdb97c49bc652e2dd10c148234bb95502dfbb676442d8565ee37864d923ca21a25f9dc2a335bf46ee82c095e387b59a664ab05c0ae41