diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -82,12 +82,8 @@ //! internal workings of the bitcoin node, and not being very convenient to use. //! Chain methods should be cleaned up and simplified over time. Examples: //! -//! * The Chain::lock() method, which lets clients delay chain tip updates -//! should be removed when clients are able to respond to updates -//! asynchronously -//! (https://github.com/bitcoin/bitcoin/pull/10973#issuecomment-380101269). -//! -//! * The initMessage() and showProgress() methods which the wallet uses to send +//! * The initMessages() and showProgress() methods which the wallet uses to +//! send //! notifications to the GUI should go away when GUI and wallet can directly //! communicate with each other without going through the node //! (https://github.com/bitcoin/bitcoin/pull/15288#discussion_r253321096). @@ -95,24 +91,18 @@ //! * The handleRpc, registerRpcs, rpcEnableDeprecated methods and other RPC //! methods can go away if wallets listen for HTTP requests on their own //! ports instead of registering to handle requests on the node HTTP port. +//! +//! * Move fee estimation queries to an asynchronous interface and let the +//! wallet cache it, fee estimation being driven by node mempool, wallet +//! should be the consumer. +//! +//! * The `guessVerificationProgress`, `getBlockHeight`, `getBlockHash`, etc +//! methods can go away if rescan logic is moved on the node side, and wallet +//! only register rescan request. class Chain { public: virtual ~Chain() {} - //! Interface for querying locked chain state, used by legacy code that - //! assumes state won't change between calls. New code should avoid using - //! the Lock interface and instead call higher-level Chain methods - //! that return more information so the chain doesn't need to stay locked - //! between calls. - class Lock { - public: - virtual ~Lock() {} - }; - - //! Return Lock interface. Chain is locked when this is called, and - //! unlocked when the returned interface is freed. - virtual std::unique_ptr lock(bool try_lock = false) = 0; - //! Get current chain height, not including genesis block (returns 0 if //! chain only contains genesis block, nullopt if chain does not contain //! any blocks) diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp --- a/src/interfaces/chain.cpp +++ b/src/interfaces/chain.cpp @@ -65,10 +65,6 @@ return true; } - class LockImpl : public Chain::Lock, public UniqueLock { - using UniqueLock::UniqueLock; - }; // namespace interfaces - class NotificationsProxy : public CValidationInterface { public: explicit NotificationsProxy( @@ -169,16 +165,6 @@ public: explicit ChainImpl(NodeContext &node, const CChainParams ¶ms) : m_node(node), m_params(params) {} - std::unique_ptr lock(bool try_lock) override { - auto lock = std::make_unique( - ::cs_main, "cs_main", __FILE__, __LINE__, try_lock); - if (try_lock && lock && !*lock) { - return {}; - } - // Temporary to avoid CWG 1579 - std::unique_ptr result = std::move(lock); - return result; - } Optional getHeight() override { LOCK(::cs_main); int height = ::ChainActive().Height(); diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -291,6 +291,9 @@ using CanGetAddressesChangedFn = std::function; virtual std::unique_ptr handleCanGetAddressesChanged(CanGetAddressesChangedFn fn) = 0; + + //! Return pointer to internal wallet class, useful for testing. + virtual CWallet *wallet() { return nullptr; } }; //! Information about one wallet address. diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -211,22 +211,18 @@ return m_wallet->GetDestValues(prefix); } void lockCoin(const COutPoint &output) override { - auto locked_chain = m_wallet->chain().lock(); LOCK(m_wallet->cs_wallet); return m_wallet->LockCoin(output); } void unlockCoin(const COutPoint &output) override { - auto locked_chain = m_wallet->chain().lock(); LOCK(m_wallet->cs_wallet); return m_wallet->UnlockCoin(output); } bool isLockedCoin(const COutPoint &output) override { - auto locked_chain = m_wallet->chain().lock(); LOCK(m_wallet->cs_wallet); return m_wallet->IsLockedCoin(output); } void listLockedCoins(std::vector &outputs) override { - auto locked_chain = m_wallet->chain().lock(); LOCK(m_wallet->cs_wallet); return m_wallet->ListLockedCoins(outputs); } @@ -235,7 +231,6 @@ const CCoinControl &coin_control, bool sign, int &change_pos, Amount &fee, bilingual_str &fail_reason) override { - auto locked_chain = m_wallet->chain().lock(); LOCK(m_wallet->cs_wallet); CTransactionRef tx; if (!m_wallet->CreateTransaction(recipients, tx, fee, change_pos, @@ -246,7 +241,6 @@ } void commitTransaction(CTransactionRef tx, WalletValueMap value_map, WalletOrderForm order_form) override { - auto locked_chain = m_wallet->chain().lock(); LOCK(m_wallet->cs_wallet); m_wallet->CommitTransaction(std::move(tx), std::move(value_map), std::move(order_form)); @@ -255,12 +249,10 @@ return m_wallet->TransactionCanBeAbandoned(txid); } bool abandonTransaction(const TxId &txid) override { - auto locked_chain = m_wallet->chain().lock(); LOCK(m_wallet->cs_wallet); return m_wallet->AbandonTransaction(txid); } CTransactionRef getTx(const TxId &txid) override { - auto locked_chain = m_wallet->chain().lock(); LOCK(m_wallet->cs_wallet); auto mi = m_wallet->mapWallet.find(txid); if (mi != m_wallet->mapWallet.end()) { @@ -269,7 +261,6 @@ return {}; } WalletTx getWalletTx(const TxId &txid) override { - auto locked_chain = m_wallet->chain().lock(); LOCK(m_wallet->cs_wallet); auto mi = m_wallet->mapWallet.find(txid); if (mi != m_wallet->mapWallet.end()) { @@ -278,7 +269,6 @@ return {}; } std::vector getWalletTxs() override { - auto locked_chain = m_wallet->chain().lock(); LOCK(m_wallet->cs_wallet); std::vector result; result.reserve(m_wallet->mapWallet.size()); @@ -290,10 +280,6 @@ bool tryGetTxStatus(const TxId &txid, interfaces::WalletTxStatus &tx_status, int &num_blocks, int64_t &block_time) override { - auto locked_chain = m_wallet->chain().lock(true /* try_lock */); - if (!locked_chain) { - return false; - } TRY_LOCK(m_wallet->cs_wallet, locked_wallet); if (!locked_wallet) { return false; @@ -313,7 +299,6 @@ WalletOrderForm &order_form, bool &in_mempool, int &num_blocks) override { - auto locked_chain = m_wallet->chain().lock(); LOCK(m_wallet->cs_wallet); auto mi = m_wallet->mapWallet.find(txid); if (mi != m_wallet->mapWallet.end()) { @@ -350,10 +335,6 @@ } bool tryGetBalances(WalletBalances &balances, int &num_blocks) override { - auto locked_chain = m_wallet->chain().lock(true /* try_lock */); - if (!locked_chain) { - return false; - } TRY_LOCK(m_wallet->cs_wallet, locked_wallet); if (!locked_wallet) { return false; @@ -369,27 +350,22 @@ return m_wallet->GetAvailableBalance(&coin_control); } isminetype txinIsMine(const CTxIn &txin) override { - auto locked_chain = m_wallet->chain().lock(); LOCK(m_wallet->cs_wallet); return m_wallet->IsMine(txin); } isminetype txoutIsMine(const CTxOut &txout) override { - auto locked_chain = m_wallet->chain().lock(); LOCK(m_wallet->cs_wallet); return m_wallet->IsMine(txout); } Amount getDebit(const CTxIn &txin, isminefilter filter) override { - auto locked_chain = m_wallet->chain().lock(); LOCK(m_wallet->cs_wallet); return m_wallet->GetDebit(txin, filter); } Amount getCredit(const CTxOut &txout, isminefilter filter) override { - auto locked_chain = m_wallet->chain().lock(); LOCK(m_wallet->cs_wallet); return m_wallet->GetCredit(txout, filter); } CoinsList listCoins() override { - auto locked_chain = m_wallet->chain().lock(); LOCK(m_wallet->cs_wallet); CoinsList result; for (const auto &entry : m_wallet->ListCoins()) { @@ -404,7 +380,6 @@ } std::vector getCoins(const std::vector &outputs) override { - auto locked_chain = m_wallet->chain().lock(); LOCK(m_wallet->cs_wallet); std::vector result; result.reserve(outputs.size()); @@ -480,6 +455,7 @@ const CCoinControl &coin_control) override { return GetMinimumFee(*m_wallet, tx_bytes, coin_control); } + CWallet *wallet() override { return m_wallet.get(); } std::shared_ptr m_wallet; }; diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -121,7 +121,6 @@ wallet->LoadWallet(firstRun); { auto spk_man = wallet->GetOrCreateLegacyScriptPubKeyMan(); - auto locked_chain = wallet->chain().lock(); LOCK2(wallet->cs_wallet, spk_man->cs_KeyStore); wallet->SetAddressBook( GetDestinationForKey(test.coinbaseKey.GetPubKey(), diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -150,7 +150,6 @@ WalletRescanReserver reserver(*pwallet); bool fRescan = true; { - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); EnsureWalletIsUnlocked(pwallet); @@ -327,7 +326,6 @@ } { - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); CTxDestination dest = @@ -368,7 +366,6 @@ if (fRescan) { RescanWallet(*pwallet, reserver); { - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); pwallet->ReacceptWalletTransactions(); } @@ -426,7 +423,6 @@ "Something wrong with merkleblock"); } - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); int height; if (!pwallet->chain().findAncestorByHash(pwallet->GetLastBlockHash(), @@ -486,7 +482,6 @@ } .Check(request); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); TxId txid(ParseHashV(request.params[0], "txid")); @@ -585,7 +580,6 @@ } { - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); std::set script_pub_keys; @@ -606,7 +600,6 @@ if (fRescan) { RescanWallet(*pwallet, reserver); { - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); pwallet->ReacceptWalletTransactions(); } @@ -662,7 +655,6 @@ int64_t nTimeBegin = 0; bool fGood = true; { - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); EnsureWalletIsUnlocked(pwallet); @@ -843,7 +835,6 @@ LegacyScriptPubKeyMan &spk_man = EnsureLegacyScriptPubKeyMan(*wallet); - auto locked_chain = pwallet->chain().lock(); LOCK2(pwallet->cs_wallet, spk_man.cs_KeyStore); EnsureWalletIsUnlocked(pwallet); @@ -904,7 +895,6 @@ // the user could have gotten from another RPC command prior to now wallet.BlockUntilSyncedToCurrentChain(); - auto locked_chain = pwallet->chain().lock(); LOCK2(wallet.cs_wallet, spk_man.cs_KeyStore); EnsureWalletIsUnlocked(&wallet); @@ -934,7 +924,7 @@ std::map mapKeyBirth; const std::map &mapKeyPool = spk_man.GetAllReserveKeys(); - wallet.GetKeyBirthTimes(*locked_chain, mapKeyBirth); + wallet.GetKeyBirthTimes(mapKeyBirth); std::set scripts = spk_man.GetCScripts(); @@ -1720,7 +1710,6 @@ int64_t nLowestTimestamp = 0; UniValue response(UniValue::VARR); { - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); EnsureWalletIsUnlocked(pwallet); @@ -1759,7 +1748,6 @@ int64_t scannedTime = pwallet->RescanFromTime( nLowestTimestamp, reserver, true /* update */); { - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); pwallet->ReacceptWalletTransactions(); } diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -161,9 +161,8 @@ return *spk_man; } -static void WalletTxToJSON(interfaces::Chain &chain, - interfaces::Chain::Lock &locked_chain, - const CWalletTx &wtx, UniValue &entry) { +static void WalletTxToJSON(interfaces::Chain &chain, const CWalletTx &wtx, + UniValue &entry) { int confirms = wtx.GetDepthInMainChain(); entry.pushKV("confirmations", confirms); if (wtx.IsCoinBase()) { @@ -463,7 +462,6 @@ // the user could have gotten from another RPC command prior to now pwallet->BlockUntilSyncedToCurrentChain(); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); CTxDestination dest = @@ -543,12 +541,10 @@ // the user could have gotten from another RPC command prior to now pwallet->BlockUntilSyncedToCurrentChain(); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); UniValue jsonGroupings(UniValue::VARR); - std::map balances = - pwallet->GetAddressBalances(*locked_chain); + std::map balances = pwallet->GetAddressBalances(); for (const std::set &grouping : pwallet->GetAddressGroupings()) { UniValue jsonGrouping(UniValue::VARR); @@ -609,7 +605,6 @@ } .Check(request); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); EnsureWalletIsUnlocked(pwallet); @@ -748,7 +743,6 @@ // the user could have gotten from another RPC command prior to now pwallet->BlockUntilSyncedToCurrentChain(); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); return ValueFromAmount(GetReceived(*pwallet, request.params, @@ -793,7 +787,6 @@ // the user could have gotten from another RPC command prior to now pwallet->BlockUntilSyncedToCurrentChain(); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); return ValueFromAmount(GetReceived(*pwallet, request.params, @@ -847,7 +840,6 @@ // the user could have gotten from another RPC command prior to now pwallet->BlockUntilSyncedToCurrentChain(); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); const UniValue &dummy_value = request.params[0]; @@ -895,7 +887,6 @@ // the user could have gotten from another RPC command prior to now pwallet->BlockUntilSyncedToCurrentChain(); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); return ValueFromAmount(pwallet->GetBalance().m_mine_untrusted_pending); @@ -990,7 +981,6 @@ // the user could have gotten from another RPC command prior to now pwallet->BlockUntilSyncedToCurrentChain(); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); if (!request.params[0].isNull() && !request.params[0].get_str().empty()) { @@ -1120,7 +1110,6 @@ LegacyScriptPubKeyMan &spk_man = EnsureLegacyScriptPubKeyMan(*pwallet); - auto locked_chain = pwallet->chain().lock(); LOCK2(pwallet->cs_wallet, spk_man.cs_KeyStore); std::string label; @@ -1166,9 +1155,8 @@ tallyitem() {} }; -static UniValue -ListReceived(const Config &config, interfaces::Chain::Lock &locked_chain, - CWallet *const pwallet, const UniValue ¶ms, bool by_label) +static UniValue ListReceived(const Config &config, CWallet *const pwallet, + const UniValue ¶ms, bool by_label) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet) { // Minimum confirmations int nMinDepth = 1; @@ -1387,10 +1375,9 @@ // the user could have gotten from another RPC command prior to now pwallet->BlockUntilSyncedToCurrentChain(); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); - return ListReceived(config, *locked_chain, pwallet, request.params, false); + return ListReceived(config, pwallet, request.params, false); } static UniValue listreceivedbylabel(const Config &config, @@ -1438,10 +1425,9 @@ // the user could have gotten from another RPC command prior to now pwallet->BlockUntilSyncedToCurrentChain(); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); - return ListReceived(config, *locked_chain, pwallet, request.params, true); + return ListReceived(config, pwallet, request.params, true); } static void MaybePushAddress(UniValue &entry, const CTxDestination &dest) { @@ -1462,8 +1448,7 @@ * @param filter_ismine The "is mine" filter flags. * @param filter_label Optional label string to filter incoming transactions. */ -static void ListTransactions(interfaces::Chain::Lock &locked_chain, - CWallet *const pwallet, const CWalletTx &wtx, +static void ListTransactions(CWallet *const pwallet, const CWalletTx &wtx, int nMinDepth, bool fLong, UniValue &ret, const isminefilter &filter_ismine, const std::string *filter_label) @@ -1495,7 +1480,7 @@ entry.pushKV("vout", s.vout); entry.pushKV("fee", ValueFromAmount(-1 * nFee)); if (fLong) { - WalletTxToJSON(pwallet->chain(), locked_chain, wtx, entry); + WalletTxToJSON(pwallet->chain(), wtx, entry); } entry.pushKV("abandoned", wtx.isAbandoned()); ret.push_back(entry); @@ -1537,7 +1522,7 @@ } entry.pushKV("vout", r.vout); if (fLong) { - WalletTxToJSON(pwallet->chain(), locked_chain, wtx, entry); + WalletTxToJSON(pwallet->chain(), wtx, entry); } ret.push_back(entry); } @@ -1699,7 +1684,6 @@ UniValue ret(UniValue::VARR); { - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); const CWallet::TxItems &txOrdered = pwallet->wtxOrdered; @@ -1708,8 +1692,8 @@ for (CWallet::TxItems::const_reverse_iterator it = txOrdered.rbegin(); it != txOrdered.rend(); ++it) { CWalletTx *const pwtx = (*it).second; - ListTransactions(*locked_chain, pwallet, *pwtx, 0, true, ret, - filter, filter_label); + ListTransactions(pwallet, *pwtx, 0, true, ret, filter, + filter_label); if (int(ret.size()) >= (nCount + nFrom)) { break; } @@ -1837,7 +1821,6 @@ // the user could have gotten from another RPC command prior to now pwallet->BlockUntilSyncedToCurrentChain(); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); // The way the 'height' is initialized is just a workaround for the gcc bug @@ -1886,8 +1869,8 @@ CWalletTx tx = pairWtx.second; if (depth == -1 || tx.GetDepthInMainChain() < depth) { - ListTransactions(*locked_chain, pwallet, tx, 0, true, transactions, - filter, nullptr /* filter_label */); + ListTransactions(pwallet, tx, 0, true, transactions, filter, + nullptr /* filter_label */); } } @@ -1907,9 +1890,8 @@ // We want all transactions regardless of confirmation count to // appear here, even negative confirmation ones, hence the big // negative. - ListTransactions(*locked_chain, pwallet, it->second, -100000000, - true, removed, filter, - nullptr /* filter_label */); + ListTransactions(pwallet, it->second, -100000000, true, removed, + filter, nullptr /* filter_label */); } } blockId = block.hashPrevBlock; @@ -2034,7 +2016,6 @@ // the user could have gotten from another RPC command prior to now pwallet->BlockUntilSyncedToCurrentChain(); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); TxId txid(ParseHashV(request.params[0], "txid")); @@ -2066,10 +2047,10 @@ entry.pushKV("fee", ValueFromAmount(nFee)); } - WalletTxToJSON(pwallet->chain(), *locked_chain, wtx, entry); + WalletTxToJSON(pwallet->chain(), wtx, entry); UniValue details(UniValue::VARR); - ListTransactions(*locked_chain, pwallet, wtx, 0, false, details, filter, + ListTransactions(pwallet, wtx, 0, false, details, filter, nullptr /* filter_label */); entry.pushKV("details", details); @@ -2123,7 +2104,6 @@ // the user could have gotten from another RPC command prior to now pwallet->BlockUntilSyncedToCurrentChain(); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); TxId txid(ParseHashV(request.params[0], "txid")); @@ -2168,7 +2148,6 @@ // the user could have gotten from another RPC command prior to now pwallet->BlockUntilSyncedToCurrentChain(); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); std::string strDest = request.params[0].get_str(); @@ -2206,7 +2185,6 @@ "Error: Private keys are disabled for this wallet"); } - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); // 0 is interpreted by TopUpKeyPool() as the default keypool size given by @@ -2267,7 +2245,6 @@ int64_t nSleepTime; { - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); if (!pwallet->IsCrypted()) { @@ -2366,7 +2343,6 @@ } .Check(request); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); if (!pwallet->IsCrypted()) { @@ -2432,7 +2408,6 @@ } .Check(request); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); if (!pwallet->IsCrypted()) { @@ -2486,7 +2461,6 @@ } .Check(request); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { @@ -2605,7 +2579,6 @@ // the user could have gotten from another RPC command prior to now pwallet->BlockUntilSyncedToCurrentChain(); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); RPCTypeCheckArgument(request.params[0], UniValue::VBOOL); @@ -2732,7 +2705,6 @@ } .Check(request); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); std::vector vOutpts; @@ -2773,7 +2745,6 @@ } .Check(request); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); Amount nAmount = AmountFromValue(request.params[0]); @@ -2844,7 +2815,6 @@ // the user could have gotten from another RPC command prior to now wallet.BlockUntilSyncedToCurrentChain(); - auto locked_chain = wallet.chain().lock(); LOCK(wallet.cs_wallet); UniValue obj(UniValue::VOBJ); @@ -2953,7 +2923,6 @@ // the user could have gotten from another RPC command prior to now pwallet->BlockUntilSyncedToCurrentChain(); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); UniValue obj(UniValue::VOBJ); @@ -3505,7 +3474,6 @@ cctl.m_avoid_address_reuse = false; cctl.m_min_depth = nMinDepth; cctl.m_max_depth = nMaxDepth; - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); pwallet->AvailableCoins(vecOutputs, !include_unsafe, &cctl, nMinimumAmount, nMaximumAmount, @@ -3908,7 +3876,6 @@ } // Sign the transaction - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); EnsureWalletIsUnlocked(pwallet); @@ -3987,7 +3954,6 @@ Optional stop_height; BlockHash start_block; { - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); int tip_height = pwallet->GetLastBlockHeight(); @@ -4509,7 +4475,6 @@ "Cannot set a HD seed to a wallet with private keys disabled"); } - auto locked_chain = pwallet->chain().lock(); LOCK2(pwallet->cs_wallet, spk_man.cs_KeyStore); // Do not do anything to non-HD wallets diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -43,8 +43,6 @@ NodeContext node; auto chain = interfaces::MakeChain(node, Params()); - auto locked_chain = chain->lock(); - LockAssertion lock(::cs_main); // Verify ScanForWalletTransactions fails to read an unknown start block. { @@ -92,7 +90,10 @@ } // Prune the older block file. - PruneOneBlockFile(oldTip->GetBlockPos().nFile); + { + LOCK(cs_main); + PruneOneBlockFile(oldTip->GetBlockPos().nFile); + } UnlinkPrunedFiles({oldTip->GetBlockPos().nFile}); // Verify ScanForWalletTransactions only picks transactions in the new block @@ -119,7 +120,10 @@ } // Prune the remaining block file. - PruneOneBlockFile(newTip->GetBlockPos().nFile); + { + LOCK(cs_main); + PruneOneBlockFile(newTip->GetBlockPos().nFile); + } UnlinkPrunedFiles({newTip->GetBlockPos().nFile}); // Verify ScanForWalletTransactions scans no blocks. @@ -154,11 +158,12 @@ NodeContext node; auto chain = interfaces::MakeChain(node, Params()); - auto locked_chain = chain->lock(); - LockAssertion lock(::cs_main); // Prune the older block file. - PruneOneBlockFile(oldTip->GetBlockPos().nFile); + { + LOCK(cs_main); + PruneOneBlockFile(oldTip->GetBlockPos().nFile); + } UnlinkPrunedFiles({oldTip->GetBlockPos().nFile}); // Verify importmulti RPC returns failure for a key whose creation time is @@ -244,8 +249,6 @@ NodeContext node; auto chain = interfaces::MakeChain(node, Params()); - auto locked_chain = chain->lock(); - LockAssertion lock(::cs_main); std::string backup_file = (GetDataDir() / "wallet.backup").string(); @@ -315,8 +318,6 @@ auto spk_man = wallet.GetOrCreateLegacyScriptPubKeyMan(); CWalletTx wtx(&wallet, m_coinbase_txns.back()); - auto locked_chain = chain->lock(); - LockAssertion lock(::cs_main); LOCK2(wallet.cs_wallet, spk_man->cs_KeyStore); wallet.SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash()); @@ -344,8 +345,6 @@ SetMockTime(mockTime); CBlockIndex *block = nullptr; if (blockTime > 0) { - auto locked_chain = wallet.chain().lock(); - LockAssertion lock(::cs_main); auto inserted = ::BlockIndex().emplace(BlockHash(GetRandHash()), new CBlockIndex); assert(inserted.second); @@ -356,7 +355,6 @@ } CWalletTx wtx(&wallet, MakeTransactionRef(tx)); - LOCK(cs_main); LOCK(wallet.cs_wallet); // If transaction is already in map, to avoid inconsistencies, // unconfirmation is needed before confirm again with different block. @@ -508,7 +506,7 @@ std::make_unique(Params(), m_chain.get(), WalletLocation(), WalletDatabase::CreateMock()); { - LOCK(wallet->cs_wallet); + LOCK2(wallet->cs_wallet, ::cs_main); wallet->SetLastBlockProcessed( ::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash()); @@ -538,7 +536,6 @@ bilingual_str error; CCoinControl dummy; { - auto locked_chain = m_chain->lock(); BOOST_CHECK(wallet->CreateTransaction({recipient}, tx, fee, changePos, error, dummy)); } @@ -552,7 +549,6 @@ CreateAndProcessBlock({CMutableTransaction(blocktx)}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); - LOCK(cs_main); LOCK(wallet->cs_wallet); wallet->SetLastBlockProcessed(wallet->GetLastBlockHeight() + 1, ::ChainActive().Tip()->GetBlockHash()); @@ -577,7 +573,6 @@ // address. std::map> list; { - auto locked_chain = m_chain->lock(); LOCK(wallet->cs_wallet); list = wallet->ListCoins(); } @@ -596,7 +591,6 @@ AddTx(CRecipient{GetScriptForRawPubKey({}), 1 * COIN, false /* subtract fee */}); { - auto locked_chain = m_chain->lock(); LOCK(wallet->cs_wallet); list = wallet->ListCoins(); } @@ -607,7 +601,6 @@ // Lock both coins. Confirm number of available coins drops to 0. { - auto locked_chain = m_chain->lock(); LOCK(wallet->cs_wallet); std::vector available; wallet->AvailableCoins(available); @@ -620,7 +613,6 @@ } } { - auto locked_chain = m_chain->lock(); LOCK(wallet->cs_wallet); std::vector available; wallet->AvailableCoins(available); @@ -629,7 +621,6 @@ // Confirm ListCoins still returns same result as before, despite coins // being locked. { - auto locked_chain = m_chain->lock(); LOCK(wallet->cs_wallet); list = wallet->ListCoins(); } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -860,11 +860,6 @@ bool IsLocked() const override; bool Lock(); - /** Interface to assert chain access and if successful lock it */ - std::unique_ptr LockChain() { - return m_chain ? m_chain->lock() : nullptr; - } - /** Interface to assert chain access */ bool HaveChain() const { return m_chain ? true : false; } @@ -1027,8 +1022,7 @@ const SecureString &strNewWalletPassphrase); bool EncryptWallet(const SecureString &strWalletPassphrase); - void GetKeyBirthTimes(interfaces::Chain::Lock &locked_chain, - std::map &mapKeyBirth) const + void GetKeyBirthTimes(std::map &mapKeyBirth) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); unsigned int ComputeTimeSmart(const CWalletTx &wtx) const; @@ -1187,8 +1181,7 @@ std::set> GetAddressGroupings() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - std::map - GetAddressBalances(interfaces::Chain::Lock &locked_chain); + std::map GetAddressBalances(); std::set GetLabelAddresses(const std::string &label) const; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1020,7 +1020,6 @@ } bool CWallet::TransactionCanBeAbandoned(const TxId &txid) const { - auto locked_chain = chain().lock(); LOCK(cs_wallet); const CWalletTx *wtx = GetWalletTx(txid); return wtx && !wtx->isAbandoned() && wtx->GetDepthInMainChain() == 0 && @@ -1037,8 +1036,6 @@ } bool CWallet::AbandonTransaction(const TxId &txid) { - // Temporary. Removed in upcoming lock cleanup - auto locked_chain = chain().lock(); LOCK(cs_wallet); WalletBatch batch(*database, "r+"); @@ -1099,7 +1096,6 @@ void CWallet::MarkConflicted(const BlockHash &hashBlock, int conflicting_height, const TxId &txid) { - auto locked_chain = chain().lock(); LOCK(cs_wallet); int conflictconfirms = @@ -1169,7 +1165,6 @@ } void CWallet::TransactionAddedToMempool(const CTransactionRef &ptx) { - auto locked_chain = chain().lock(); LOCK(cs_wallet); CWalletTx::Confirmation confirm(CWalletTx::Status::UNCONFIRMED, /* block_height */ 0, BlockHash(), @@ -1194,7 +1189,6 @@ const std::vector &vtxConflicted, int height) { const BlockHash &block_hash = block.GetHash(); - auto locked_chain = chain().lock(); LOCK(cs_wallet); m_last_block_processed_height = height; @@ -1211,7 +1205,6 @@ } void CWallet::BlockDisconnected(const CBlock &block, int height) { - auto locked_chain = chain().lock(); LOCK(cs_wallet); // At block disconnection, this will change an abandoned transaction to @@ -1803,7 +1796,6 @@ bool reorg = false; if (chain().findBlock(block_hash, FoundBlock().data(block)) && !block.IsNull()) { - auto locked_chain = chain().lock(); LOCK(cs_wallet); next_block = chain().findNextBlock( block_hash, block_height, FoundBlock().hash(next_block_hash), @@ -1841,7 +1833,6 @@ break; } { - auto locked_chain = chain().lock(); if (!next_block || reorg) { // break successfully when rescan has reached the tip, or // previous block is no longer on the chain due to a reorg @@ -2198,8 +2189,7 @@ int submitted_tx_count = 0; - { // locked_chain and cs_wallet scope - auto locked_chain = chain().lock(); + { // cs_wallet scope LOCK(cs_wallet); // Relay transactions @@ -2216,7 +2206,7 @@ ++submitted_tx_count; } } - } // locked_chain and cs_wallet + } // cs_wallet if (submitted_tx_count > 0) { WalletLogPrintf("%s: resubmit %u unconfirmed transactions\n", __func__, @@ -2241,7 +2231,6 @@ bool avoid_reuse) const { Balance ret; isminefilter reuse_filter = avoid_reuse ? ISMINE_NO : ISMINE_USED; - auto locked_chain = chain().lock(); LOCK(cs_wallet); std::set trusted_parents; for (const auto &entry : mapWallet) { @@ -2267,7 +2256,6 @@ } Amount CWallet::GetAvailableBalance(const CCoinControl *coinControl) const { - auto locked_chain = chain().lock(); LOCK(cs_wallet); Amount balance = Amount::zero(); @@ -2765,7 +2753,6 @@ // Acquire the locks to prevent races to the new locked unspents between the // CreateTransaction call and LockCoin calls (when lockUnspents is true). - auto locked_chain = chain().lock(); LOCK(cs_wallet); CTransactionRef tx_new; @@ -2917,7 +2904,6 @@ { std::set setCoins; - auto locked_chain = chain().lock(); LOCK(cs_wallet); txNew.nLockTime = GetLocktimeForNewTransaction( chain(), GetLastBlockHash(), GetLastBlockHeight()); @@ -3295,7 +3281,6 @@ void CWallet::CommitTransaction( CTransactionRef tx, mapValue_t mapValue, std::vector> orderForm) { - auto locked_chain = chain().lock(); LOCK(cs_wallet); CWalletTx wtxNew(this, std::move(tx)); @@ -3338,11 +3323,6 @@ } DBErrors CWallet::LoadWallet(bool &fFirstRunRet) { - // Even if we don't use this lock in this function, we want to preserve - // lock order in LoadToWallet if query of chain state is needed to know - // tx status. If lock can't be taken (e.g bitcoin-wallet), tx confirmation - // status may be not reliable. - auto locked_chain = LockChain(); LOCK(cs_wallet); fFirstRunRet = false; @@ -3577,8 +3557,7 @@ } } -std::map -CWallet::GetAddressBalances(interfaces::Chain::Lock &locked_chain) { +std::map CWallet::GetAddressBalances() { std::map balances; LOCK(cs_wallet); @@ -3814,8 +3793,7 @@ /** @} */ // end of Actions -void CWallet::GetKeyBirthTimes(interfaces::Chain::Lock &locked_chain, - std::map &mapKeyBirth) const { +void CWallet::GetKeyBirthTimes(std::map &mapKeyBirth) const { AssertLockHeld(cs_wallet); mapKeyBirth.clear(); @@ -4057,11 +4035,6 @@ CWallet dummyWallet(chainParams, &chain, WalletLocation(), WalletDatabase::CreateDummy()); std::string backup_filename; - // Even if we don't use this lock in this function, we want to preserve - // lock order in LoadToWallet if query of chain state is needed to know - // tx status. If lock can't be taken, tx confirmation status may be not - // reliable. - auto locked_chain = dummyWallet.LockChain(); if (!WalletBatch::Recover( wallet_path, static_cast(&dummyWallet), WalletBatch::RecoverKeysOnlyFilter, backup_filename)) { @@ -4158,7 +4131,6 @@ } } - auto locked_chain = chain.lock(); walletInstance->ChainStateFlushed(chain.getTipLocator()); } else if (wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS) { // Make it impossible to disable private keys after creation @@ -4290,9 +4262,19 @@ // Try to top up keypool. No-op if the wallet is locked. walletInstance->TopUpKeyPool(); - auto locked_chain = chain.lock(); LOCK(walletInstance->cs_wallet); + // Register wallet with validationinterface. It's done before rescan to + // avoid missing block connections between end of rescan and validation + // subscribing. Because of wallet lock being hold, block connection + // notifications are going to be pending on the validation-side until lock + // release. It's likely to have block processing duplicata (if rescan block + // range overlaps with notification one) but we guarantee at least than + // wallet state is correct after notifications delivery. This is temporary + // until rescan and notifications delivery are unified under same interface. + walletInstance->m_chain_notifications_handler = + walletInstance->chain().handleNotifications(walletInstance); + int rescan_height = 0; if (!gArgs.GetBoolArg("-rescan", false)) { WalletBatch batch(*walletInstance->database); @@ -4411,11 +4393,6 @@ } } - // Register with the validation interface. It's ok to do this after rescan - // since we're still holding locked_chain. - walletInstance->m_chain_notifications_handler = - walletInstance->chain().handleNotifications(walletInstance); - walletInstance->SetBroadcastTransactions( gArgs.GetBoolArg("-walletbroadcast", DEFAULT_WALLETBROADCAST)); @@ -4487,7 +4464,6 @@ } void CWallet::postInitProcess() { - auto locked_chain = chain().lock(); LOCK(cs_wallet); // Add wallet transactions that aren't already in a block to mempool.