Add AssertLockHeld assertions in CWallet::ListCoins

Summary:
 * Add AssertLockHeld assertions in CWallet::ListCoins
 * Add EXCLUSIVE_LOCKS_REQUIRED to CWallet::ListCoins

Suggested by MarcoFalke <falke.marco@gmail.com> in
bitcoin/bitcoin#10605 (comment)

This is a backport of Core PR10605

Test Plan:
 * Build with clang and make sure there are no warning related to locks.
 * Build in debug mode and run the extended test suite.

Reviewers: #bitcoin_abc, jasonbcox

Reviewed By: #bitcoin_abc, jasonbcox

Differential Revision: https://reviews.bitcoinabc.org/D4170

src/wallet/test/wallet_tests.cpp

@@ -338,7 +338,11 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup) {
 
     // Confirm ListCoins initially returns 1 coin grouped under coinbaseKey
     // address.
-    auto list = wallet->ListCoins();
+    std::map<CTxDestination, std::vector<COutput>> list;
+    {
+        LOCK2(cs_main, wallet->cs_wallet);
+        list = wallet->ListCoins();
+    }
     BOOST_CHECK_EQUAL(list.size(), 1U);
     BOOST_CHECK_EQUAL(boost::get<CKeyID>(list.begin()->first).ToString(),
                       coinbaseAddress);
@@ -353,7 +357,10 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup) {
     // pubkey.
     AddTx(CRecipient{GetScriptForRawPubKey({}), 1 * COIN,
                      false /* subtract fee */});
-    list = wallet->ListCoins();
+    {
+        LOCK2(cs_main, wallet->cs_wallet);
+        list = wallet->ListCoins();
+    }
     BOOST_CHECK_EQUAL(list.size(), 1U);
     BOOST_CHECK_EQUAL(boost::get<CKeyID>(list.begin()->first).ToString(),
                       coinbaseAddress);
@@ -380,7 +387,10 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup) {
     }
     // Confirm ListCoins still returns same result as before, despite coins
     // being locked.
-    list = wallet->ListCoins();
+    {
+        LOCK2(cs_main, wallet->cs_wallet);
+        list = wallet->ListCoins();
+    }
     BOOST_CHECK_EQUAL(list.size(), 1U);
     BOOST_CHECK_EQUAL(boost::get<CKeyID>(list.begin()->first).ToString(),
                       coinbaseAddress);

src/wallet/wallet.cpp

@@ -2520,20 +2520,12 @@ void CWallet::AvailableCoins(std::vector<COutput> &vCoins, bool fOnlySafe,
 }
 
 std::map<CTxDestination, std::vector<COutput>> CWallet::ListCoins() const {
-    // TODO: Add AssertLockHeld(cs_wallet) here.
-    //
-    // Because the return value from this function contains pointers to
-    // CWalletTx objects, callers to this function really should acquire the
-    // cs_wallet lock before calling it. However, the current caller doesn't
-    // acquire this lock yet. There was an attempt to add the missing lock in
-    // https://github.com/bitcoin/bitcoin/pull/10340, but that change has been
-    // postponed until after https://github.com/bitcoin/bitcoin/pull/10244 to
-    // avoid adding some extra complexity to the Qt code.
+    AssertLockHeld(cs_main);
+    AssertLockHeld(cs_wallet);
 
     std::map<CTxDestination, std::vector<COutput>> result;
     std::vector<COutput> availableCoins;
 
-    LOCK2(cs_main, cs_wallet);
     AvailableCoins(availableCoins);
 
     for (auto &coin : availableCoins) {

src/wallet/wallet.h

@@ -857,7 +857,8 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface {
      * Return list of available coins and locked coins grouped by non-change
      * output address.
      */
-    std::map<CTxDestination, std::vector<COutput>> ListCoins() const;
+    std::map<CTxDestination, std::vector<COutput>> ListCoins() const
+        EXCLUSIVE_LOCKS_REQUIRED(cs_main, cs_wallet);
 
     /**
      * Find non-change parent output.