diff options
-rw-r--r-- | src/interfaces/chain.cpp | 12 | ||||
-rw-r--r-- | src/interfaces/chain.h | 30 | ||||
-rw-r--r-- | src/interfaces/wallet.cpp | 24 | ||||
-rw-r--r-- | src/interfaces/wallet.h | 3 | ||||
-rw-r--r-- | src/qt/test/wallettests.cpp | 1 | ||||
-rw-r--r-- | src/wallet/feebumper.cpp | 4 | ||||
-rw-r--r-- | src/wallet/rpcdump.cpp | 16 | ||||
-rw-r--r-- | src/wallet/rpcwallet.cpp | 57 | ||||
-rw-r--r-- | src/wallet/test/wallet_tests.cpp | 105 | ||||
-rw-r--r-- | src/wallet/wallet.cpp | 48 | ||||
-rw-r--r-- | src/wallet/wallet.h | 7 |
11 files changed, 106 insertions, 201 deletions
diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp index 11cd20d785..c5262e4bc0 100644 --- a/src/interfaces/chain.cpp +++ b/src/interfaces/chain.cpp @@ -53,11 +53,6 @@ bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLock<Rec return true; } -class LockImpl : public Chain::Lock, public UniqueLock<RecursiveMutex> -{ - using UniqueLock::UniqueLock; -}; - class NotificationsProxy : public CValidationInterface { public: @@ -150,13 +145,6 @@ class ChainImpl : public Chain { public: explicit ChainImpl(NodeContext& node) : m_node(node) {} - std::unique_ptr<Chain::Lock> lock(bool try_lock) override - { - auto lock = MakeUnique<LockImpl>(::cs_main, "cs_main", __FILE__, __LINE__, try_lock); - if (try_lock && lock && !*lock) return {}; - std::unique_ptr<Chain::Lock> result = std::move(lock); // Temporary to avoid CWG 1579 - return result; - } Optional<int> getHeight() override { LOCK(::cs_main); diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index d35d386925..e33fe54ac8 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -59,12 +59,7 @@ public: //! 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). @@ -72,26 +67,19 @@ public: //! * 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> 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/wallet.cpp b/src/interfaces/wallet.cpp index de26f329d7..752448aac7 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -196,25 +196,21 @@ public: } 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.hash, output.n); } void listLockedCoins(std::vector<COutPoint>& outputs) override { - auto locked_chain = m_wallet->chain().lock(); LOCK(m_wallet->cs_wallet); return m_wallet->ListLockedCoins(outputs); } @@ -225,7 +221,6 @@ public: CAmount& fee, std::string& 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, @@ -238,14 +233,12 @@ public: 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)); } bool transactionCanBeAbandoned(const uint256& txid) override { return m_wallet->TransactionCanBeAbandoned(txid); } bool abandonTransaction(const uint256& txid) override { - auto locked_chain = m_wallet->chain().lock(); LOCK(m_wallet->cs_wallet); return m_wallet->AbandonTransaction(txid); } @@ -273,7 +266,6 @@ public: } CTransactionRef getTx(const uint256& 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()) { @@ -283,7 +275,6 @@ public: } WalletTx getWalletTx(const uint256& 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()) { @@ -293,7 +284,6 @@ public: } std::vector<WalletTx> getWalletTxs() override { - auto locked_chain = m_wallet->chain().lock(); LOCK(m_wallet->cs_wallet); std::vector<WalletTx> result; result.reserve(m_wallet->mapWallet.size()); @@ -307,10 +297,6 @@ public: 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; @@ -331,7 +317,6 @@ public: 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()) { @@ -368,8 +353,6 @@ public: } bool tryGetBalances(WalletBalances& balances, int& num_blocks, bool force, int cached_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; @@ -386,31 +369,26 @@ public: } 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); } CAmount 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); } CAmount 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()) { @@ -424,7 +402,6 @@ public: } std::vector<WalletTxOut> getCoins(const std::vector<COutPoint>& outputs) override { - auto locked_chain = m_wallet->chain().lock(); LOCK(m_wallet->cs_wallet); std::vector<WalletTxOut> result; result.reserve(outputs.size()); @@ -496,6 +473,7 @@ public: { return MakeHandler(m_wallet->NotifyCanGetAddressesChanged.connect(fn)); } + CWallet* wallet() override { return m_wallet.get(); } std::shared_ptr<CWallet> m_wallet; }; diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 5d870c5e3d..e5e3b80663 100644 --- a/src/interfaces/wallet.h +++ b/src/interfaces/wallet.h @@ -300,6 +300,9 @@ public: //! Register handler for keypool changed messages. using CanGetAddressesChangedFn = std::function<void()>; virtual std::unique_ptr<Handler> 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/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index 94a1e0a63d..2ee9ae0d86 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -145,7 +145,6 @@ void TestGUI(interfaces::Node& node) 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(), wallet->m_default_address_type), "", "receive"); spk_man->AddKeyPubKey(test.coinbaseKey, test.coinbaseKey.GetPubKey()); diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index d596db431a..01fcb1680e 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -140,7 +140,6 @@ namespace feebumper { bool TransactionCanBeBumped(const CWallet& wallet, const uint256& txid) { - auto locked_chain = wallet.chain().lock(); LOCK(wallet.cs_wallet); const CWalletTx* wtx = wallet.GetWalletTx(txid); if (wtx == nullptr) return false; @@ -156,7 +155,6 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo // We are going to modify coin control later, copy to re-use CCoinControl new_coin_control(coin_control); - auto locked_chain = wallet.chain().lock(); LOCK(wallet.cs_wallet); errors.clear(); auto it = wallet.mapWallet.find(txid); @@ -240,14 +238,12 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo } bool SignTransaction(CWallet& wallet, CMutableTransaction& mtx) { - auto locked_chain = wallet.chain().lock(); LOCK(wallet.cs_wallet); return wallet.SignTransaction(mtx); } Result CommitTransaction(CWallet& wallet, const uint256& txid, CMutableTransaction&& mtx, std::vector<std::string>& errors, uint256& bumped_txid) { - auto locked_chain = wallet.chain().lock(); LOCK(wallet.cs_wallet); if (!errors.empty()) { return Result::MISC_ERROR; diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 9417e2bd58..c863d22530 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -133,7 +133,6 @@ UniValue importprivkey(const JSONRPCRequest& request) WalletRescanReserver reserver(*pwallet); bool fRescan = true; { - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); EnsureWalletIsUnlocked(pwallet); @@ -285,7 +284,6 @@ UniValue importaddress(const JSONRPCRequest& request) fP2SH = request.params[3].get_bool(); { - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); CTxDestination dest = DecodeDestination(request.params[0].get_str()); @@ -317,7 +315,6 @@ UniValue importaddress(const JSONRPCRequest& request) { RescanWallet(*pwallet, reserver); { - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); pwallet->ReacceptWalletTransactions(); } @@ -361,7 +358,6 @@ UniValue importprunedfunds(const JSONRPCRequest& request) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Something wrong with merkleblock"); } - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); int height; if (!pwallet->chain().findAncestorByHash(pwallet->GetLastBlockHash(), merkleBlock.header.GetHash(), FoundBlock().height(height))) { @@ -407,7 +403,6 @@ UniValue removeprunedfunds(const JSONRPCRequest& request) }, }.Check(request); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); uint256 hash(ParseHashV(request.params[0], "txid")); @@ -487,7 +482,6 @@ UniValue importpubkey(const JSONRPCRequest& request) throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey is not a valid public key"); { - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); std::set<CScript> script_pub_keys; @@ -505,7 +499,6 @@ UniValue importpubkey(const JSONRPCRequest& request) { RescanWallet(*pwallet, reserver); { - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); pwallet->ReacceptWalletTransactions(); } @@ -557,7 +550,6 @@ UniValue importwallet(const JSONRPCRequest& request) int64_t nTimeBegin = 0; bool fGood = true; { - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); EnsureWalletIsUnlocked(pwallet); @@ -700,7 +692,6 @@ UniValue dumpprivkey(const JSONRPCRequest& request) LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*wallet); - auto locked_chain = pwallet->chain().lock(); LOCK2(pwallet->cs_wallet, spk_man.cs_KeyStore); EnsureWalletIsUnlocked(pwallet); @@ -756,7 +747,6 @@ UniValue dumpwallet(const JSONRPCRequest& request) // the user could have gotten from another RPC command prior to now wallet.BlockUntilSyncedToCurrentChain(); - auto locked_chain = pwallet->chain().lock(); LOCK2(pwallet->cs_wallet, spk_man.cs_KeyStore); EnsureWalletIsUnlocked(&wallet); @@ -780,7 +770,7 @@ UniValue dumpwallet(const JSONRPCRequest& request) std::map<CKeyID, int64_t> mapKeyBirth; const std::map<CKeyID, int64_t>& mapKeyPool = spk_man.GetAllReserveKeys(); - pwallet->GetKeyBirthTimes(*locked_chain, mapKeyBirth); + pwallet->GetKeyBirthTimes(mapKeyBirth); std::set<CScriptID> scripts = spk_man.GetCScripts(); @@ -1379,7 +1369,6 @@ UniValue importmulti(const JSONRPCRequest& mainRequest) int64_t nLowestTimestamp = 0; UniValue response(UniValue::VARR); { - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); EnsureWalletIsUnlocked(pwallet); @@ -1414,7 +1403,6 @@ UniValue importmulti(const JSONRPCRequest& mainRequest) if (fRescan && fRunScan && requests.size()) { int64_t scannedTime = pwallet->RescanFromTime(nLowestTimestamp, reserver, true /* update */); { - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); pwallet->ReacceptWalletTransactions(); } @@ -1676,7 +1664,6 @@ UniValue importdescriptors(const JSONRPCRequest& main_request) { bool rescan = false; UniValue response(UniValue::VARR); { - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); EnsureWalletIsUnlocked(pwallet); @@ -1705,7 +1692,6 @@ UniValue importdescriptors(const JSONRPCRequest& main_request) { if (rescan) { int64_t scanned_time = pwallet->RescanFromTime(lowest_timestamp, 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 index 4a026f9ef4..91162d575d 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -133,7 +133,7 @@ LegacyScriptPubKeyMan& EnsureLegacyScriptPubKeyMan(CWallet& wallet, bool also_cr 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); @@ -399,7 +399,6 @@ static UniValue sendtoaddress(const JSONRPCRequest& request) // 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 = DecodeDestination(request.params[0].get_str()); @@ -487,11 +486,10 @@ static UniValue listaddressgroupings(const JSONRPCRequest& request) // 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<CTxDestination, CAmount> balances = pwallet->GetAddressBalances(*locked_chain); + std::map<CTxDestination, CAmount> balances = pwallet->GetAddressBalances(); for (const std::set<CTxDestination>& grouping : pwallet->GetAddressGroupings()) { UniValue jsonGrouping(UniValue::VARR); for (const CTxDestination& address : grouping) @@ -543,7 +541,6 @@ static UniValue signmessage(const JSONRPCRequest& request) }, }.Check(request); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); EnsureWalletIsUnlocked(pwallet); @@ -652,7 +649,6 @@ static UniValue getreceivedbyaddress(const JSONRPCRequest& request) // 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, /* by_label */ false)); @@ -693,7 +689,6 @@ static UniValue getreceivedbylabel(const JSONRPCRequest& request) // 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, /* by_label */ true)); @@ -736,7 +731,6 @@ static UniValue getbalance(const JSONRPCRequest& request) // 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]; @@ -778,7 +772,6 @@ static UniValue getunconfirmedbalance(const JSONRPCRequest &request) // 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); @@ -841,7 +834,6 @@ static UniValue sendmany(const JSONRPCRequest& request) // 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()) { @@ -963,7 +955,6 @@ static UniValue addmultisigaddress(const JSONRPCRequest& request) LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*pwallet); - auto locked_chain = pwallet->chain().lock(); LOCK2(pwallet->cs_wallet, spk_man.cs_KeyStore); std::string label; @@ -1016,7 +1007,7 @@ struct tallyitem } }; -static UniValue ListReceived(interfaces::Chain::Lock& locked_chain, const CWallet* const pwallet, const UniValue& params, bool by_label) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet) +static UniValue ListReceived(const CWallet* const pwallet, const UniValue& params, bool by_label) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet) { // Minimum confirmations int nMinDepth = 1; @@ -1209,10 +1200,9 @@ static UniValue listreceivedbyaddress(const JSONRPCRequest& request) // 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(*locked_chain, pwallet, request.params, false); + return ListReceived(pwallet, request.params, false); } static UniValue listreceivedbylabel(const JSONRPCRequest& request) @@ -1254,10 +1244,9 @@ static UniValue listreceivedbylabel(const JSONRPCRequest& request) // 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(*locked_chain, pwallet, request.params, true); + return ListReceived(pwallet, request.params, true); } static void MaybePushAddress(UniValue & entry, const CTxDestination &dest) @@ -1278,7 +1267,7 @@ static void MaybePushAddress(UniValue & entry, const CTxDestination &dest) * @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, const CWallet* const pwallet, const CWalletTx& wtx, int nMinDepth, bool fLong, UniValue& ret, const isminefilter& filter_ismine, const std::string* filter_label) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet) +static void ListTransactions(const CWallet* const pwallet, const CWalletTx& wtx, int nMinDepth, bool fLong, UniValue& ret, const isminefilter& filter_ismine, const std::string* filter_label) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet) { CAmount nFee; std::list<COutputEntry> listReceived; @@ -1307,7 +1296,7 @@ static void ListTransactions(interfaces::Chain::Lock& locked_chain, const CWalle entry.pushKV("vout", s.vout); entry.pushKV("fee", ValueFromAmount(-nFee)); if (fLong) - WalletTxToJSON(pwallet->chain(), locked_chain, wtx, entry); + WalletTxToJSON(pwallet->chain(), wtx, entry); entry.pushKV("abandoned", wtx.isAbandoned()); ret.push_back(entry); } @@ -1349,7 +1338,7 @@ static void ListTransactions(interfaces::Chain::Lock& locked_chain, const CWalle } entry.pushKV("vout", r.vout); if (fLong) - WalletTxToJSON(pwallet->chain(), locked_chain, wtx, entry); + WalletTxToJSON(pwallet->chain(), wtx, entry); ret.push_back(entry); } } @@ -1464,7 +1453,6 @@ UniValue listtransactions(const JSONRPCRequest& request) UniValue ret(UniValue::VARR); { - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); const CWallet::TxItems & txOrdered = pwallet->wtxOrdered; @@ -1473,7 +1461,7 @@ UniValue listtransactions(const JSONRPCRequest& request) 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; } } @@ -1557,7 +1545,6 @@ static UniValue listsinceblock(const JSONRPCRequest& request) // 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 #47679 since version 4.6.0. @@ -1598,7 +1585,7 @@ static UniValue listsinceblock(const JSONRPCRequest& request) CWalletTx tx = pairWtx.second; if (depth == -1 || abs(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 */); } } @@ -1615,7 +1602,7 @@ static UniValue listsinceblock(const JSONRPCRequest& request) if (it != pwallet->mapWallet.end()) { // 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; @@ -1700,7 +1687,6 @@ static UniValue gettransaction(const JSONRPCRequest& request) // the user could have gotten from another RPC command prior to now pwallet->BlockUntilSyncedToCurrentChain(); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); uint256 hash(ParseHashV(request.params[0], "txid")); @@ -1729,10 +1715,10 @@ static UniValue gettransaction(const JSONRPCRequest& request) if (wtx.IsFromMe(filter)) 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, nullptr /* filter_label */); + ListTransactions(pwallet, wtx, 0, false, details, filter, nullptr /* filter_label */); entry.pushKV("details", details); std::string strHex = EncodeHexTx(*wtx.tx, pwallet->chain().rpcSerializationFlags()); @@ -1776,7 +1762,6 @@ static UniValue abandontransaction(const JSONRPCRequest& request) // the user could have gotten from another RPC command prior to now pwallet->BlockUntilSyncedToCurrentChain(); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); uint256 hash(ParseHashV(request.params[0], "txid")); @@ -1817,7 +1802,6 @@ static UniValue backupwallet(const JSONRPCRequest& request) // 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(); @@ -1855,7 +1839,6 @@ static UniValue keypoolrefill(const JSONRPCRequest& request) throw JSONRPCError(RPC_WALLET_ERROR, "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 -keypool @@ -1909,7 +1892,6 @@ static UniValue walletpassphrase(const JSONRPCRequest& request) int64_t nSleepTime; { - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); if (!pwallet->IsCrypted()) { @@ -1991,7 +1973,6 @@ static UniValue walletpassphrasechange(const JSONRPCRequest& request) }, }.Check(request); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); if (!pwallet->IsCrypted()) { @@ -2047,7 +2028,6 @@ static UniValue walletlock(const JSONRPCRequest& request) }, }.Check(request); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); if (!pwallet->IsCrypted()) { @@ -2094,7 +2074,6 @@ static UniValue encryptwallet(const JSONRPCRequest& request) }, }.Check(request); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { @@ -2173,7 +2152,6 @@ static UniValue lockunspent(const JSONRPCRequest& request) // 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); @@ -2286,7 +2264,6 @@ static UniValue listlockunspent(const JSONRPCRequest& request) }, }.Check(request); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); std::vector<COutPoint> vOutpts; @@ -2329,7 +2306,6 @@ static UniValue settxfee(const JSONRPCRequest& request) }, }.Check(request); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); CAmount nAmount = AmountFromValue(request.params[0]); @@ -2388,7 +2364,6 @@ static UniValue getbalances(const JSONRPCRequest& request) // the user could have gotten from another RPC command prior to now wallet.BlockUntilSyncedToCurrentChain(); - auto locked_chain = wallet.chain().lock(); LOCK(wallet.cs_wallet); const auto bal = wallet.GetBalance(); @@ -2465,7 +2440,6 @@ static UniValue getwalletinfo(const JSONRPCRequest& request) // 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); @@ -2940,7 +2914,6 @@ static UniValue listunspent(const JSONRPCRequest& request) 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, nMinimumSumAmount, nMaximumCount); } @@ -3312,7 +3285,6 @@ UniValue signrawtransactionwithwallet(const JSONRPCRequest& request) } // Sign the transaction - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); EnsureWalletIsUnlocked(pwallet); @@ -3441,7 +3413,6 @@ static UniValue bumpfee(const JSONRPCRequest& request) // the user could have gotten from another RPC command prior to now pwallet->BlockUntilSyncedToCurrentChain(); - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); EnsureWalletIsUnlocked(pwallet); @@ -3548,7 +3519,6 @@ UniValue rescanblockchain(const JSONRPCRequest& request) Optional<int> stop_height; uint256 start_block; { - auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); int tip_height = pwallet->GetLastBlockHeight(); @@ -4010,7 +3980,6 @@ UniValue sethdseed(const JSONRPCRequest& request) throw JSONRPCError(RPC_WALLET_ERROR, "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 index 227cf275cc..c049f2f15f 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -75,8 +75,6 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) NodeContext node; auto chain = interfaces::MakeChain(node); - auto locked_chain = chain->lock(); - LockAssertion lock(::cs_main); // Verify ScanForWalletTransactions fails to read an unknown start block. { @@ -116,7 +114,10 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) } // 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 @@ -139,7 +140,10 @@ BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) } // 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. @@ -171,11 +175,12 @@ BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup) NodeContext node; auto chain = interfaces::MakeChain(node); - 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 @@ -241,8 +246,6 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup) NodeContext node; auto chain = interfaces::MakeChain(node); - auto locked_chain = chain->lock(); - LockAssertion lock(::cs_main); std::string backup_file = (GetDataDir() / "wallet.backup").string(); @@ -308,8 +311,6 @@ BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup) 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()); @@ -334,8 +335,6 @@ static int64_t AddTx(CWallet& wallet, uint32_t lockTime, int64_t mockTime, int64 SetMockTime(mockTime); CBlockIndex* block = nullptr; if (blockTime > 0) { - auto locked_chain = wallet.chain().lock(); - LockAssertion lock(::cs_main); auto inserted = ::BlockIndex().emplace(GetRandHash(), new CBlockIndex); assert(inserted.second); const uint256& hash = inserted.first->first; @@ -345,7 +344,6 @@ static int64_t AddTx(CWallet& wallet, uint32_t lockTime, int64_t mockTime, int64 } 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. @@ -492,7 +490,7 @@ public: CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); wallet = MakeUnique<CWallet>(m_chain.get(), WalletLocation(), WalletDatabase::CreateMock()); { - LOCK2(::cs_main, wallet->cs_wallet); + LOCK2(wallet->cs_wallet, ::cs_main); wallet->SetLastBlockProcessed(::ChainActive().Height(), ::ChainActive().Tip()->GetBlockHash()); } bool firstRun; @@ -520,7 +518,6 @@ public: std::string error; CCoinControl dummy; { - auto locked_chain = m_chain->lock(); BOOST_CHECK(wallet->CreateTransaction({recipient}, tx, fee, changePos, error, dummy)); } wallet->CommitTransaction(tx, {}, {}); @@ -531,7 +528,6 @@ public: } CreateAndProcessBlock({CMutableTransaction(blocktx)}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())); - LOCK(cs_main); LOCK(wallet->cs_wallet); wallet->SetLastBlockProcessed(wallet->GetLastBlockHeight() + 1, ::ChainActive().Tip()->GetBlockHash()); auto it = wallet->mapWallet.find(tx->GetHash()); @@ -554,7 +550,6 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup) // address. std::map<CTxDestination, std::vector<COutput>> list; { - auto locked_chain = m_chain->lock(); LOCK(wallet->cs_wallet); list = wallet->ListCoins(); } @@ -571,7 +566,6 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup) // pubkey. AddTx(CRecipient{GetScriptForRawPubKey({}), 1 * COIN, false /* subtract fee */}); { - auto locked_chain = m_chain->lock(); LOCK(wallet->cs_wallet); list = wallet->ListCoins(); } @@ -581,7 +575,6 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup) // Lock both coins. Confirm number of available coins drops to 0. { - auto locked_chain = m_chain->lock(); LOCK(wallet->cs_wallet); std::vector<COutput> available; wallet->AvailableCoins(available); @@ -594,7 +587,6 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup) } } { - auto locked_chain = m_chain->lock(); LOCK(wallet->cs_wallet); std::vector<COutput> available; wallet->AvailableCoins(available); @@ -603,7 +595,6 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup) // 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(); } @@ -694,11 +685,20 @@ BOOST_FIXTURE_TEST_CASE(wallet_descriptor_test, BasicTestingSetup) //! conditions if it's called the same time an incoming transaction shows up in //! the mempool or a new block. //! -//! It isn't possible for a unit test to totally verify there aren't race -//! conditions without hooking into the implementation more, so this test just -//! verifies that new transactions are detected during loading without any -//! notifications at all, to infer that timing of notifications shouldn't -//! matter. The test could be extended to cover other scenarios in the future. +//! It isn't possible to verify there aren't race condition in every case, so +//! this test just checks two specific cases and ensures that timing of +//! notifications in these cases doesn't prevent the wallet from detecting +//! transactions. +//! +//! In the first case, block and mempool transactions are created before the +//! wallet is loaded, but notifications about these transactions are delayed +//! until after it is loaded. The notifications are superfluous in this case, so +//! the test verifies the transactions are detected before they arrive. +//! +//! In the second case, block and mempool transactions are created after the +//! wallet rescan and notifications are immediately synced, to verify the wallet +//! must already have a handler in place for them, and there's no gap after +//! rescanning where new transactions in new blocks could be lost. BOOST_FIXTURE_TEST_CASE(CreateWalletFromFile, TestChain100Setup) { // Create new wallet with known key and unload it. @@ -709,6 +709,7 @@ BOOST_FIXTURE_TEST_CASE(CreateWalletFromFile, TestChain100Setup) AddKey(*wallet, key); TestUnloadWallet(std::move(wallet)); + // Add log hook to detect AddToWallet events from rescans, blockConnected, // and transactionAddedToMempool notifications int addtx_count = 0; @@ -717,21 +718,14 @@ BOOST_FIXTURE_TEST_CASE(CreateWalletFromFile, TestChain100Setup) return false; }); + bool rescan_completed = false; DebugLogHelper rescan_check("[default wallet] Rescan completed", [&](const std::string* s) { - if (s) { - // For now, just assert that cs_main is being held during the - // rescan, ensuring that a new block couldn't be connected - // that the wallet would miss. After - // https://github.com/bitcoin/bitcoin/pull/16426 when cs_main is no - // longer held, the test can be extended to append a new block here - // and check it's handled correctly. - AssertLockHeld(::cs_main); - rescan_completed = true; - } + if (s) rescan_completed = true; return false; }); + // Block the queue to prevent the wallet receiving blockConnected and // transactionAddedToMempool notifications, and create block and mempool // transactions paying to the wallet @@ -746,29 +740,56 @@ BOOST_FIXTURE_TEST_CASE(CreateWalletFromFile, TestChain100Setup) auto mempool_tx = TestSimpleSpend(*m_coinbase_txns[1], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey())); BOOST_CHECK(chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, false, error)); + // Reload wallet and make sure new transactions are detected despite events // being blocked wallet = TestLoadWallet(*chain); BOOST_CHECK(rescan_completed); BOOST_CHECK_EQUAL(addtx_count, 2); - unsigned int block_tx_time, mempool_tx_time; { LOCK(wallet->cs_wallet); - block_tx_time = wallet->mapWallet.at(block_tx.GetHash()).nTimeReceived; - mempool_tx_time = wallet->mapWallet.at(mempool_tx.GetHash()).nTimeReceived; + BOOST_CHECK_EQUAL(wallet->mapWallet.count(block_tx.GetHash()), 1); + BOOST_CHECK_EQUAL(wallet->mapWallet.count(mempool_tx.GetHash()), 1); } + // Unblock notification queue and make sure stale blockConnected and // transactionAddedToMempool events are processed promise.set_value(); SyncWithValidationInterfaceQueue(); BOOST_CHECK_EQUAL(addtx_count, 4); + + + TestUnloadWallet(std::move(wallet)); + + + // Load wallet again, this time creating new block and mempool transactions + // paying to the wallet as the wallet finishes loading and syncing the + // queue so the events have to be handled immediately. Releasing the wallet + // lock during the sync is a little artificial but is needed to avoid a + // deadlock during the sync and simulates a new block notification happening + // as soon as possible. + addtx_count = 0; + auto handler = HandleLoadWallet([&](std::unique_ptr<interfaces::Wallet> wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet->wallet()->cs_wallet) { + BOOST_CHECK(rescan_completed); + m_coinbase_txns.push_back(CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]); + block_tx = TestSimpleSpend(*m_coinbase_txns[2], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey())); + m_coinbase_txns.push_back(CreateAndProcessBlock({block_tx}, GetScriptForRawPubKey(coinbaseKey.GetPubKey())).vtx[0]); + mempool_tx = TestSimpleSpend(*m_coinbase_txns[3], 0, coinbaseKey, GetScriptForRawPubKey(key.GetPubKey())); + BOOST_CHECK(chain->broadcastTransaction(MakeTransactionRef(mempool_tx), DEFAULT_TRANSACTION_MAXFEE, false, error)); + LEAVE_CRITICAL_SECTION(wallet->wallet()->cs_wallet); + SyncWithValidationInterfaceQueue(); + ENTER_CRITICAL_SECTION(wallet->wallet()->cs_wallet); + }); + wallet = TestLoadWallet(*chain); + BOOST_CHECK_EQUAL(addtx_count, 4); { LOCK(wallet->cs_wallet); - BOOST_CHECK_EQUAL(block_tx_time, wallet->mapWallet.at(block_tx.GetHash()).nTimeReceived); - BOOST_CHECK_EQUAL(mempool_tx_time, wallet->mapWallet.at(mempool_tx.GetHash()).nTimeReceived); + BOOST_CHECK_EQUAL(wallet->mapWallet.count(block_tx.GetHash()), 1); + BOOST_CHECK_EQUAL(wallet->mapWallet.count(mempool_tx.GetHash()), 1); } + TestUnloadWallet(std::move(wallet)); } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 6e8f7e0e8f..a20ede59fd 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -974,7 +974,6 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, CWalletTx::Co bool CWallet::TransactionCanBeAbandoned(const uint256& hashTx) const { - auto locked_chain = chain().lock(); LOCK(cs_wallet); const CWalletTx* wtx = GetWalletTx(hashTx); return wtx && !wtx->isAbandoned() && wtx->GetDepthInMainChain() == 0 && !wtx->InMempool(); @@ -992,7 +991,6 @@ void CWallet::MarkInputsDirty(const CTransactionRef& tx) bool CWallet::AbandonTransaction(const uint256& hashTx) { - auto locked_chain = chain().lock(); // Temporary. Removed in upcoming lock cleanup LOCK(cs_wallet); WalletBatch batch(*database, "r+"); @@ -1047,7 +1045,6 @@ bool CWallet::AbandonTransaction(const uint256& hashTx) void CWallet::MarkConflicted(const uint256& hashBlock, int conflicting_height, const uint256& hashTx) { - auto locked_chain = chain().lock(); LOCK(cs_wallet); int conflictconfirms = (m_last_block_processed_height - conflicting_height + 1) * -1; @@ -1110,7 +1107,6 @@ void CWallet::SyncTransaction(const CTransactionRef& ptx, CWalletTx::Confirmatio } void CWallet::transactionAddedToMempool(const CTransactionRef& ptx) { - auto locked_chain = chain().lock(); LOCK(cs_wallet); CWalletTx::Confirmation confirm(CWalletTx::Status::UNCONFIRMED, /* block_height */ 0, {}, /* nIndex */ 0); SyncTransaction(ptx, confirm); @@ -1132,7 +1128,6 @@ void CWallet::transactionRemovedFromMempool(const CTransactionRef &ptx) { void CWallet::blockConnected(const CBlock& block, int height) { const uint256& block_hash = block.GetHash(); - auto locked_chain = chain().lock(); LOCK(cs_wallet); m_last_block_processed_height = height; @@ -1146,7 +1141,6 @@ void CWallet::blockConnected(const CBlock& block, int height) 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 @@ -1685,7 +1679,6 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc uint256 next_block_hash; 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), &reorg); if (reorg) { @@ -1714,7 +1707,6 @@ CWallet::ScanResult CWallet::ScanForWalletTransactions(const uint256& start_bloc 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 @@ -2002,8 +1994,7 @@ void CWallet::ResendWalletTransactions() int submitted_tx_count = 0; - { // locked_chain and cs_wallet scope - auto locked_chain = chain().lock(); + { // cs_wallet scope LOCK(cs_wallet); // Relay transactions @@ -2016,7 +2007,7 @@ void CWallet::ResendWalletTransactions() std::string unused_err_string; if (wtx.SubmitMemoryPoolAndRelay(unused_err_string, true)) ++submitted_tx_count; } - } // locked_chain and cs_wallet + } // cs_wallet if (submitted_tx_count > 0) { WalletLogPrintf("%s: resubmit %u unconfirmed transactions\n", __func__, submitted_tx_count); @@ -2044,7 +2035,6 @@ CWallet::Balance CWallet::GetBalance(const int min_depth, bool avoid_reuse) cons Balance ret; isminefilter reuse_filter = avoid_reuse ? ISMINE_NO : ISMINE_USED; { - auto locked_chain = chain().lock(); LOCK(cs_wallet); std::set<uint256> trusted_parents; for (const auto& entry : mapWallet) @@ -2071,7 +2061,6 @@ CWallet::Balance CWallet::GetBalance(const int min_depth, bool avoid_reuse) cons CAmount CWallet::GetAvailableBalance(const CCoinControl* coinControl) const { - auto locked_chain = chain().lock(); LOCK(cs_wallet); CAmount balance = 0; @@ -2550,7 +2539,6 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC // 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; @@ -2701,7 +2689,6 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CTransac int nBytes; { std::set<CInputCoin> setCoins; - auto locked_chain = chain().lock(); LOCK(cs_wallet); txNew.nLockTime = GetLocktimeForNewTransaction(chain(), GetLastBlockHash(), GetLastBlockHeight()); { @@ -3019,7 +3006,6 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CTransac void CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::vector<std::pair<std::string, std::string>> orderForm) { - auto locked_chain = chain().lock(); LOCK(cs_wallet); CWalletTx wtxNew(this, std::move(tx)); @@ -3059,11 +3045,6 @@ void CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::ve 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; @@ -3282,7 +3263,7 @@ void CWallet::MarkDestinationsDirty(const std::set<CTxDestination>& destinations } } -std::map<CTxDestination, CAmount> CWallet::GetAddressBalances(interfaces::Chain::Lock& locked_chain) const +std::map<CTxDestination, CAmount> CWallet::GetAddressBalances() const { std::map<CTxDestination, CAmount> balances; @@ -3509,7 +3490,7 @@ void CWallet::ListLockedCoins(std::vector<COutPoint>& vOutpts) const /** @} */ // end of Actions -void CWallet::GetKeyBirthTimes(interfaces::Chain::Lock& locked_chain, std::map<CKeyID, int64_t>& mapKeyBirth) const { +void CWallet::GetKeyBirthTimes(std::map<CKeyID, int64_t>& mapKeyBirth) const { AssertLockHeld(cs_wallet); mapKeyBirth.clear(); @@ -3719,11 +3700,6 @@ bool CWallet::Verify(interfaces::Chain& chain, const WalletLocation& location, b // Recover readable keypairs: CWallet dummyWallet(&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, (void *)&dummyWallet, WalletBatch::RecoverKeysOnlyFilter, backup_filename)) { return false; } @@ -3812,7 +3788,6 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain, } } - 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 @@ -3926,9 +3901,18 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain, // 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)) { @@ -4029,9 +4013,6 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain, } } - // 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)); { @@ -4091,7 +4072,6 @@ bool CWallet::UpgradeWallet(int version, std::string& error, std::vector<std::st void CWallet::postInitProcess() { - auto locked_chain = chain().lock(); LOCK(cs_wallet); // Add wallet transactions that aren't already in a block to mempool diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index fb23ea82a7..72b3cf1fb8 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -775,9 +775,6 @@ public: bool IsLocked() const override; bool Lock(); - /** Interface to assert chain access and if successful lock it */ - std::unique_ptr<interfaces::Chain::Lock> LockChain() { return m_chain ? m_chain->lock() : nullptr; } - /** Interface to assert chain access */ bool HaveChain() const { return m_chain ? true : false; } @@ -878,7 +875,7 @@ public: bool ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase, const SecureString& strNewWalletPassphrase); bool EncryptWallet(const SecureString& strWalletPassphrase); - void GetKeyBirthTimes(interfaces::Chain::Lock& locked_chain, std::map<CKeyID, int64_t> &mapKeyBirth) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + void GetKeyBirthTimes(std::map<CKeyID, int64_t> &mapKeyBirth) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); unsigned int ComputeTimeSmart(const CWalletTx& wtx) const; /** @@ -1014,7 +1011,7 @@ public: int64_t GetOldestKeyPoolTime() const; std::set<std::set<CTxDestination>> GetAddressGroupings() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - std::map<CTxDestination, CAmount> GetAddressBalances(interfaces::Chain::Lock& locked_chain) const; + std::map<CTxDestination, CAmount> GetAddressBalances() const; std::set<CTxDestination> GetLabelAddresses(const std::string& label) const; |