Merge #19982: test: Fix inconsistent lock order in wallet_tests/CreateWallet

e1e68b6305beb47ebf7ee48f14e12fdebdfea1ef test: Fix inconsistent lock order in wallet_tests/CreateWallet (Hennadii Stepanov)
cb23fe01c125e1820f3c37348e06d98c93e6aec2 [skip ci] sync: Check precondition in LEAVE_CRITICAL_SECTION() macro (Hennadii Stepanov)
c5e3e74f70c29ac8852903ef425f5f327d5da969 sync: Improve CheckLastCritical() (Hennadii Stepanov)

Pull request description:

  This PR:
  - fixes #19049 that was caused by #16426
  - removes `wallet_tests::CreateWallet` suppression from the `test/sanitizer_suppressions/tsan`

  The example of the improved `CheckLastCritical()`/`LEAVE_CRITICAL_SECTION()` log (could be got when compiled without the last commit):
  ```
  2020-09-20T08:34:28.429485Z [test] INCONSISTENT LOCK ORDER DETECTED
  2020-09-20T08:34:28.429493Z [test] Current lock order (least recent first) is:
  2020-09-20T08:34:28.429501Z [test]  'walletInstance->cs_wallet' in wallet/wallet.cpp:4007 (in thread 'test')
  2020-09-20T08:34:28.429508Z [test]  'cs_wallets' in wallet/wallet.cpp:4089 (in thread 'test')
  ```

  Currently, there are other "naked" `LEAVE_CRITICAL_SECTION()` in the code base:
  https://github.com/bitcoin/bitcoin/blob/b99a1633b270e0e89479b2bb2ae19a8a8dc0fa05/src/rpc/mining.cpp#L698

  https://github.com/bitcoin/bitcoin/blob/b99a1633b270e0e89479b2bb2ae19a8a8dc0fa05/src/checkqueue.h#L208

ACKs for top commit:
  MarcoFalke:
    review ACK e1e68b6305beb47ebf7ee48f14e12fdebdfea1ef 💂
  ryanofsky:
    Code review ACK e1e68b6305beb47ebf7ee48f14e12fdebdfea1ef. Just trivial rebase and suggested switch to BOOST_CHECK_EXCEPTION since last review
  vasild:
    ACK e1e68b630

Tree-SHA512: a627680eac3af4b4c02772473d68322ce8d3811bf6b035d3485ccc97d35755bef933cffabd3f20b126f89e3301eccecec3f769df34415fb7c426c967b6ce36e6

1 files changed, 5 insertions, 1 deletions

diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index eb0bbb6ccc..a6db261914 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -28,6 +28,8 @@ RPCHelpMan importmulti();
 RPCHelpMan dumpwallet();
 RPCHelpMan importwallet();
 
+extern RecursiveMutex cs_wallets;
+
 // Ensure that fee levels defined in the wallet are at least as high
 // as the default levels for node policy.
 static_assert(DEFAULT_TRANSACTION_MINFEE >= DEFAULT_MIN_RELAY_TX_FEE, "wallet minimum fee is smaller than default relay fee");
@@ -761,16 +763,18 @@ BOOST_FIXTURE_TEST_CASE(CreateWallet, TestChain100Setup)
     // deadlock during the sync and simulates a new block notification happening
     // as soon as possible.
     addtx_count = 0;
-    auto handler = HandleLoadWallet([&](std::unique_ptr<interfaces::Wallet> wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet->wallet()->cs_wallet) {
+    auto handler = HandleLoadWallet([&](std::unique_ptr<interfaces::Wallet> wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet->wallet()->cs_wallet, cs_wallets) {
             BOOST_CHECK(rescan_completed);
             m_coinbase_txns.push_back(CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
             block_tx = TestSimpleSpend(*m_coinbase_txns[2], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey()));
             m_coinbase_txns.push_back(CreateAndProcessBlock({block_tx}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]);
             mempool_tx = TestSimpleSpend(*m_coinbase_txns[3], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey()));
             BOOST_CHECK(m_node.chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, false, error));
+            LEAVE_CRITICAL_SECTION(cs_wallets);
             LEAVE_CRITICAL_SECTION(wallet->wallet()->cs_wallet);
             SyncWithValidationInterfaceQueue();
             ENTER_CRITICAL_SECTION(wallet->wallet()->cs_wallet);
+            ENTER_CRITICAL_SECTION(cs_wallets);
         });
     wallet = TestLoadWallet(*m_node.chain);
     BOOST_CHECK_EQUAL(addtx_count, 4);