From 545e85eccc2441c6d7745bb90d88dc14718455a2 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Thu, 15 Jun 2017 14:56:35 -0400 Subject: [PATCH 1/2] Add AssertLockHeld assertions in CWallet::ListCoins --- src/wallet/test/wallet_tests.cpp | 16 +++++++++++++--- src/wallet/wallet.cpp | 12 ++---------- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 14c5ad721..10e6da8b1 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -317,7 +317,11 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup) // Confirm ListCoins initially returns 1 coin grouped under coinbaseKey // address. - auto list = wallet->ListCoins(); + std::map> list; + { + LOCK2(cs_main, wallet->cs_wallet); + list = wallet->ListCoins(); + } BOOST_CHECK_EQUAL(list.size(), 1U); BOOST_CHECK_EQUAL(boost::get(list.begin()->first).ToString(), coinbaseAddress); BOOST_CHECK_EQUAL(list.begin()->second.size(), 1U); @@ -330,7 +334,10 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup) // coinbaseKey pubkey, even though the change address has a different // 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(list.begin()->first).ToString(), coinbaseAddress); BOOST_CHECK_EQUAL(list.begin()->second.size(), 2U); @@ -356,7 +363,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(list.begin()->first).ToString(), coinbaseAddress); BOOST_CHECK_EQUAL(list.begin()->second.size(), 2U); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 7d46f0274..40ff06310 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2364,20 +2364,12 @@ void CWallet::AvailableCoins(std::vector &vCoins, bool fOnlySafe, const std::map> 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> result; std::vector availableCoins; - LOCK2(cs_main, cs_wallet); AvailableCoins(availableCoins); for (auto& coin : availableCoins) { From 62b6f0f21e24ff367d096c80ebdf398de4a98163 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Fri, 31 Aug 2018 08:11:01 -0400 Subject: [PATCH 2/2] Add EXCLUSIVE_LOCKS_REQUIRED to CWallet::ListCoins Suggested by MarcoFalke in https://github.com/bitcoin/bitcoin/pull/10605#issuecomment-417643535 --- src/wallet/wallet.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index b85f374a0..c8d5e6a78 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -821,7 +821,7 @@ public: /** * Return list of available coins and locked coins grouped by non-change output address. */ - std::map> ListCoins() const; + std::map> ListCoins() const EXCLUSIVE_LOCKS_REQUIRED(cs_main, cs_wallet); /** * Find non-change parent output.