From 4bc0ae1ee2f45dec99d91c1c9d0cbef96141cc45 Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kittywhiskers@users.noreply.github.com> Date: Sun, 12 Dec 2021 19:08:12 +0530 Subject: [PATCH] merge bitcoin#16033: Hold cs_main when reading chainActive via getTipLocator(). Remove assumeLocked(). --- src/interfaces/chain.cpp | 10 +++------- src/interfaces/chain.h | 5 ----- src/wallet/test/wallet_tests.cpp | 31 +++++++++++++++++++------------ src/wallet/wallet.cpp | 2 +- 4 files changed, 23 insertions(+), 25 deletions(-) diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp index 760e39ee29..df8f6c7fb7 100644 --- a/src/interfaces/chain.cpp +++ b/src/interfaces/chain.cpp @@ -38,7 +38,7 @@ namespace interfaces { namespace { -class LockImpl : public Chain::Lock +class LockImpl : public Chain::Lock, public UniqueLock { Optional getHeight() override { @@ -171,10 +171,7 @@ class LockImpl : public Chain::Lock LockAnnotation lock(::cs_main); return CheckFinalTx(tx); } -}; -class LockingStateImpl : public LockImpl, public UniqueLock -{ using UniqueLock::UniqueLock; }; @@ -273,13 +270,12 @@ class ChainImpl : public Chain public: std::unique_ptr lock(bool try_lock) override { - auto result = MakeUnique(::cs_main, "cs_main", __FILE__, __LINE__, try_lock); + auto result = MakeUnique(::cs_main, "cs_main", __FILE__, __LINE__, try_lock); if (try_lock && result && !*result) return {}; // std::move necessary on some compilers due to conversion from - // LockingStateImpl to Lock pointer + // LockImpl to Lock pointer return std::move(result); } - std::unique_ptr assumeLocked() override { return MakeUnique(); } bool findBlock(const uint256& hash, CBlock* block, int64_t* time, int64_t* time_max) override { CBlockIndex* index; diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index a863469a38..10f1b3064d 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -147,11 +147,6 @@ public: //! unlocked when the returned interface is freed. virtual std::unique_ptr lock(bool try_lock = false) = 0; - //! Return Lock interface assuming chain is already locked. This - //! method is temporary and is only used in a few places to avoid changing - //! behavior while code is transitioned to use the Chain::Lock interface. - virtual std::unique_ptr assumeLocked() = 0; - //! Return whether node has the block and optionally return block metadata //! or contents. //! diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index d19fb19830..d5f43dd761 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -369,7 +369,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; @@ -388,7 +391,6 @@ public: } std::unique_ptr m_chain = interfaces::MakeChain(); - std::unique_ptr m_locked_chain = m_chain->assumeLocked(); // Temporary. Removed in upcoming lock cleanup std::unique_ptr wallet; }; @@ -400,8 +402,9 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup) // address. std::map> 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(list.begin()->first).ToString(), coinbaseAddress); @@ -416,8 +419,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(list.begin()->first).ToString(), coinbaseAddress); @@ -425,9 +429,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 available; - wallet->AvailableCoins(*m_locked_chain, available); + wallet->AvailableCoins(*locked_chain, available); BOOST_CHECK_EQUAL(available.size(), 2U); } for (const auto& group : list) { @@ -437,16 +442,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 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(list.begin()->first).ToString(), coinbaseAddress); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e6d5e21d89..03184d95ac 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -5019,7 +5019,7 @@ std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, return error(_("Unable to generate initial keys")); } - auto locked_chain = chain.assumeLocked(); // Temporary. Removed in upcoming lock cleanup + auto locked_chain = chain.lock(); walletInstance->ChainStateFlushed(locked_chain->getTipLocator()); // Try to create wallet backup right after new wallet was created