Merge #16033: Hold cs_main when reading chainActive via getTipLocator(). Remove assumeLocked().

9402ef0739 Remove temporary method assumeLocked(). Remove LockingStateImpl. Remove redundant cs_main locks. (practicalswift)
593a8e8a2c wallet: Use chain.lock() instead of temporary chain.assumeLocked() (practicalswift)

Pull request description:

  Fixes #16028.

  Problem description:

  `LockAnnotation lock(::cs_main)` is a guarantee to the compiler thread analysis that `::cs_main` is locked (when it couldn't be determined otherwise).

  Despite being annotated with the locking guarantee ...

  https://github.com/bitcoin/bitcoin/blob/65526fc8666fef35ef908dbc225f706bef642c7e/src/interfaces/chain.cpp#L134-L138

  ... `getTipLocator()` reads `chainActive` (via `::ChainActive()`) without holding `cs_main`.

  This can be verified by adding the following `AssertLockHeld(cs_main)`:

  ```
  $ git diff
  diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp
  index 59623284d..9fc693a0f 100644
  --- a/src/interfaces/chain.cpp
  +++ b/src/interfaces/chain.cpp
  @@ -134,6 +134,7 @@ class LockImpl : public Chain::Lock
       CBlockLocator getTipLocator() override
       {
           LockAnnotation lock(::cs_main);
  +        AssertLockHeld(::cs_main);
           return ::ChainActive().GetLocator();
       }
       Optional<int> findLocatorFork(const CBlockLocator& locator) override
  $ make check
  ../build-aux/test-driver: line 107: 12881 Aborted                 "$@" > $log_file 2>&1
  FAIL: qt/test/test_bitcoin-qt
  ```

ACKs for commit 9402ef:
  MarcoFalke:
    utACK 9402ef0739fdcd8e989c07c0595095e9608b243c
  ryanofsky:
    utACK 9402ef0739fdcd8e989c07c0595095e9608b243c. Changes are consolidating commits and removing redundant lock2 cs_main calls

Tree-SHA512: 0a030bf0c07eb53194ecc246f973ef389dd42a0979f51932bf94bdf7e90c52473ae03be49718ee1629582b05dd8e0dc020b5a210318c93378ea4ace90c0f9f72

2 files changed, 20 insertions, 13 deletions

diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index 69a78f1fc0..62630c011a 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -368,7 +368,10 @@ public:
         int changePos = -1;
         std::string error;
         CCoinControl dummy;
-        BOOST_CHECK(wallet->CreateTransaction(*m_locked_chain, {recipient}, tx, reservekey, fee, changePos, error, dummy));
+        {
+            auto locked_chain = m_chain->lock();
+            BOOST_CHECK(wallet->CreateTransaction(*locked_chain, {recipient}, tx, reservekey, fee, changePos, error, dummy));
+        }
         CValidationState state;
         BOOST_CHECK(wallet->CommitTransaction(tx, {}, {}, reservekey, state));
         CMutableTransaction blocktx;
@@ -387,7 +390,6 @@ public:
     }
 
     std::unique_ptr<interfaces::Chain> m_chain = interfaces::MakeChain();
-    std::unique_ptr<interfaces::Chain::Lock> m_locked_chain = m_chain->assumeLocked();  // Temporary. Removed in upcoming lock cleanup
     std::unique_ptr<CWallet> wallet;
 };
 
@@ -399,8 +401,9 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup)
     // address.
     std::map<CTxDestination, std::vector<COutput>> list;
     {
-        LOCK2(cs_main, wallet->cs_wallet);
-        list = wallet->ListCoins(*m_locked_chain);
+        auto locked_chain = m_chain->lock();
+        LOCK(wallet->cs_wallet);
+        list = wallet->ListCoins(*locked_chain);
     }
     BOOST_CHECK_EQUAL(list.size(), 1U);
     BOOST_CHECK_EQUAL(boost::get<PKHash>(list.begin()->first).ToString(), coinbaseAddress);
@@ -415,8 +418,9 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup)
     // pubkey.
     AddTx(CRecipient{GetScriptForRawPubKey({}), 1 * COIN, false /* subtract fee */});
     {
-        LOCK2(cs_main, wallet->cs_wallet);
-        list = wallet->ListCoins(*m_locked_chain);
+        auto locked_chain = m_chain->lock();
+        LOCK(wallet->cs_wallet);
+        list = wallet->ListCoins(*locked_chain);
     }
     BOOST_CHECK_EQUAL(list.size(), 1U);
     BOOST_CHECK_EQUAL(boost::get<PKHash>(list.begin()->first).ToString(), coinbaseAddress);
@@ -424,9 +428,10 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup)
 
     // Lock both coins. Confirm number of available coins drops to 0.
     {
-        LOCK2(cs_main, wallet->cs_wallet);
+        auto locked_chain = m_chain->lock();
+        LOCK(wallet->cs_wallet);
         std::vector<COutput> available;
-        wallet->AvailableCoins(*m_locked_chain, available);
+        wallet->AvailableCoins(*locked_chain, available);
         BOOST_CHECK_EQUAL(available.size(), 2U);
     }
     for (const auto& group : list) {
@@ -436,16 +441,18 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup)
         }
     }
     {
-        LOCK2(cs_main, wallet->cs_wallet);
+        auto locked_chain = m_chain->lock();
+        LOCK(wallet->cs_wallet);
         std::vector<COutput> available;
-        wallet->AvailableCoins(*m_locked_chain, available);
+        wallet->AvailableCoins(*locked_chain, available);
         BOOST_CHECK_EQUAL(available.size(), 0U);
     }
     // Confirm ListCoins still returns same result as before, despite coins
     // being locked.
     {
-        LOCK2(cs_main, wallet->cs_wallet);
-        list = wallet->ListCoins(*m_locked_chain);
+        auto locked_chain = m_chain->lock();
+        LOCK(wallet->cs_wallet);
+        list = wallet->ListCoins(*locked_chain);
     }
     BOOST_CHECK_EQUAL(list.size(), 1U);
     BOOST_CHECK_EQUAL(boost::get<PKHash>(list.begin()->first).ToString(), coinbaseAddress);
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 7038eff4bb..260617c666 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -4074,7 +4074,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
             return nullptr;
         }
 
-        auto locked_chain = chain.assumeLocked();  // Temporary. Removed in upcoming lock cleanup
+        auto locked_chain = chain.lock();
         walletInstance->ChainStateFlushed(locked_chain->getTipLocator());
     } else if (wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS) {
         // Make it impossible to disable private keys after creation