From b855592d835bf4b3fb1263b88d4f96669a1722b1 Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Tue, 9 Jul 2019 20:07:39 -0400 Subject: [PATCH 1/5] [wallet] Move getHeight from Chain::Lock interface to simple Chain Instead of calling getHeight, we rely on CWallet::m_last_block processed_height where it's possible. --- src/interfaces/chain.cpp | 18 +++++++++--------- src/interfaces/chain.h | 10 +++++----- src/interfaces/wallet.cpp | 4 ++-- src/wallet/feebumper.cpp | 2 +- src/wallet/rpcwallet.cpp | 11 +++++------ src/wallet/test/wallet_tests.cpp | 2 +- src/wallet/wallet.cpp | 7 +++---- src/wallet/wallet.h | 3 +-- 8 files changed, 27 insertions(+), 30 deletions(-) diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp index 0e7641ae320..b891cf2ee59 100644 --- a/src/interfaces/chain.cpp +++ b/src/interfaces/chain.cpp @@ -55,15 +55,6 @@ bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLock { - Optional getHeight() override - { - LockAssertion lock(::cs_main); - int height = ::ChainActive().Height(); - if (height >= 0) { - return height; - } - return nullopt; - } Optional getBlockHeight(const uint256& hash) override { LockAssertion lock(::cs_main); @@ -234,6 +225,15 @@ public: std::unique_ptr result = std::move(lock); // Temporary to avoid CWG 1579 return result; } + Optional getHeight() override + { + LOCK(::cs_main); + int height = ::ChainActive().Height(); + if (height >= 0) { + return height; + } + return nullopt; + } bool findBlock(const uint256& hash, const FoundBlock& block) override { WAIT_LOCK(cs_main, lock); diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index fb77c81cdcb..03635fd74fa 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -87,11 +87,6 @@ public: public: virtual ~Lock() {} - //! Get current chain height, not including genesis block (returns 0 if - //! chain only contains genesis block, nullopt if chain does not contain - //! any blocks). - virtual Optional getHeight() = 0; - //! Get block height above genesis block. Returns 0 for genesis block, //! 1 for following block, and so on. Returns nullopt for a block not //! included in the current chain. @@ -135,6 +130,11 @@ public: //! 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) + virtual Optional getHeight() = 0; + //! Return whether node has the block and optionally return block metadata //! or contents. virtual bool findBlock(const uint256& hash, const FoundBlock& block={}) = 0; diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index 152917ac605..0c31ae0c818 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -228,7 +228,7 @@ public: auto locked_chain = m_wallet->chain().lock(); LOCK(m_wallet->cs_wallet); CTransactionRef tx; - if (!m_wallet->CreateTransaction(*locked_chain, recipients, tx, fee, change_pos, + if (!m_wallet->CreateTransaction(recipients, tx, fee, change_pos, fail_reason, coin_control, sign)) { return {}; } @@ -335,7 +335,7 @@ public: LOCK(m_wallet->cs_wallet); auto mi = m_wallet->mapWallet.find(txid); if (mi != m_wallet->mapWallet.end()) { - num_blocks = locked_chain->getHeight().get_value_or(-1); + num_blocks = m_wallet->GetLastBlockHeight(); in_mempool = mi->second.InMempool(); order_form = mi->second.vOrderForm; tx_status = MakeWalletTxStatus(*locked_chain, mi->second); diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index dafa022dedb..d596db431ac 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -219,7 +219,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo CAmount fee_ret; int change_pos_in_out = -1; // No requested location for change std::string fail_reason; - if (!wallet.CreateTransaction(*locked_chain, recipients, tx_new, fee_ret, change_pos_in_out, fail_reason, new_coin_control, false)) { + if (!wallet.CreateTransaction(recipients, tx_new, fee_ret, change_pos_in_out, fail_reason, new_coin_control, false)) { errors.push_back("Unable to create transaction: " + fail_reason); return Result::WALLET_ERROR; } diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 49c583c4226..4c78e6fdce1 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -322,7 +322,7 @@ static UniValue setlabel(const JSONRPCRequest& request) } -static CTransactionRef SendMoney(interfaces::Chain::Lock& locked_chain, CWallet * const pwallet, const CTxDestination &address, CAmount nValue, bool fSubtractFeeFromAmount, const CCoinControl& coin_control, mapValue_t mapValue) +static CTransactionRef SendMoney(CWallet* const pwallet, const CTxDestination& address, CAmount nValue, bool fSubtractFeeFromAmount, const CCoinControl& coin_control, mapValue_t mapValue) { CAmount curBalance = pwallet->GetBalance(0, coin_control.m_avoid_address_reuse).m_mine_trusted; @@ -344,7 +344,7 @@ static CTransactionRef SendMoney(interfaces::Chain::Lock& locked_chain, CWallet CRecipient recipient = {scriptPubKey, nValue, fSubtractFeeFromAmount}; vecSend.push_back(recipient); CTransactionRef tx; - if (!pwallet->CreateTransaction(locked_chain, vecSend, tx, nFeeRequired, nChangePosRet, strError, coin_control)) { + if (!pwallet->CreateTransaction(vecSend, tx, nFeeRequired, nChangePosRet, strError, coin_control)) { if (!fSubtractFeeFromAmount && nValue + nFeeRequired > curBalance) strError = strprintf("Error: This transaction requires a transaction fee of at least %s", FormatMoney(nFeeRequired)); throw JSONRPCError(RPC_WALLET_ERROR, strError); @@ -445,7 +445,7 @@ static UniValue sendtoaddress(const JSONRPCRequest& request) EnsureWalletIsUnlocked(pwallet); - CTransactionRef tx = SendMoney(*locked_chain, pwallet, dest, nAmount, fSubtractFeeFromAmount, coin_control, std::move(mapValue)); + CTransactionRef tx = SendMoney(pwallet, dest, nAmount, fSubtractFeeFromAmount, coin_control, std::move(mapValue)); return tx->GetHash().GetHex(); } @@ -913,7 +913,7 @@ static UniValue sendmany(const JSONRPCRequest& request) int nChangePosRet = -1; std::string strFailReason; CTransactionRef tx; - bool fCreated = pwallet->CreateTransaction(*locked_chain, vecSend, tx, nFeeRequired, nChangePosRet, strFailReason, coin_control); + bool fCreated = pwallet->CreateTransaction(vecSend, tx, nFeeRequired, nChangePosRet, strFailReason, coin_control); if (!fCreated) throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, strFailReason); pwallet->CommitTransaction(tx, std::move(mapValue), {} /* orderForm */); @@ -3563,8 +3563,7 @@ UniValue rescanblockchain(const JSONRPCRequest& request) stop_height = request.params[1].get_int(); if (*stop_height < 0 || *stop_height > tip_height) { throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid stop_height"); - } - else if (*stop_height < start_height) { + } else if (*stop_height < start_height) { throw JSONRPCError(RPC_INVALID_PARAMETER, "stop_height must be greater than start_height"); } } diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 1215956bb4a..1c569cbef1d 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -521,7 +521,7 @@ public: CCoinControl dummy; { auto locked_chain = m_chain->lock(); - BOOST_CHECK(wallet->CreateTransaction(*locked_chain, {recipient}, tx, fee, changePos, error, dummy)); + BOOST_CHECK(wallet->CreateTransaction({recipient}, tx, fee, changePos, error, dummy)); } wallet->CommitTransaction(tx, {}, {}); CMutableTransaction blocktx; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 6d46841cee2..3be0453978e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2555,7 +2555,7 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC LOCK(cs_wallet); CTransactionRef tx_new; - if (!CreateTransaction(*locked_chain, vecSend, tx_new, nFeeRet, nChangePosInOut, strFailReason, coinControl, false)) { + if (!CreateTransaction(vecSend, tx_new, nFeeRet, nChangePosInOut, strFailReason, coinControl, false)) { return false; } @@ -2671,8 +2671,7 @@ OutputType CWallet::TransactionChangeType(OutputType change_type, const std::vec return m_default_address_type; } -bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std::vector& vecSend, CTransactionRef& tx, CAmount& nFeeRet, - int& nChangePosInOut, std::string& strFailReason, const CCoinControl& coin_control, bool sign) +bool CWallet::CreateTransaction(const std::vector& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, std::string& strFailReason, const CCoinControl& coin_control, bool sign) { CAmount nValue = 0; const OutputType change_type = TransactionChangeType(coin_control.m_change_type ? *coin_control.m_change_type : m_default_change_type, vecSend); @@ -3943,7 +3942,7 @@ std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, } } - const Optional tip_height = locked_chain->getHeight(); + const Optional tip_height = chain.getHeight(); if (tip_height) { walletInstance->m_last_block_processed = locked_chain->getBlockHash(*tip_height); walletInstance->m_last_block_processed_height = *tip_height; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 24c78fceb3d..662f5d4bb32 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -961,8 +961,7 @@ public: * selected by SelectCoins(); Also create the change output, when needed * @note passing nChangePosInOut as -1 will result in setting a random position */ - bool CreateTransaction(interfaces::Chain::Lock& locked_chain, const std::vector& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, - std::string& strFailReason, const CCoinControl& coin_control, bool sign = true); + bool CreateTransaction(const std::vector& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, std::string& strFailReason, const CCoinControl& coin_control, bool sign = true); /** * Submit the transaction to the node's mempool and then relay to peers. * Should be called after CreateTransaction unless you want to abort From de13363a472ea30dff2f8f55c6ae572281115380 Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Mon, 15 Jul 2019 18:20:12 -0400 Subject: [PATCH 2/5] [wallet] Move getBlockHeight from Chain::Lock interface to simple Chain Add HaveChain to assert chain access for wallet-tool in LoadToWallet. --- src/interfaces/chain.cpp | 18 +++++++++--------- src/interfaces/chain.h | 10 +++++----- src/wallet/wallet.cpp | 7 +++---- src/wallet/wallet.h | 3 +++ 4 files changed, 20 insertions(+), 18 deletions(-) diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp index b891cf2ee59..ccdbf1f1d1d 100644 --- a/src/interfaces/chain.cpp +++ b/src/interfaces/chain.cpp @@ -55,15 +55,6 @@ bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLock { - Optional getBlockHeight(const uint256& hash) override - { - LockAssertion lock(::cs_main); - CBlockIndex* block = LookupBlockIndex(hash); - if (block && ::ChainActive().Contains(block)) { - return block->nHeight; - } - return nullopt; - } uint256 getBlockHash(int height) override { LockAssertion lock(::cs_main); @@ -234,6 +225,15 @@ public: } return nullopt; } + Optional getBlockHeight(const uint256& hash) override + { + LOCK(::cs_main); + CBlockIndex* block = LookupBlockIndex(hash); + if (block && ::ChainActive().Contains(block)) { + return block->nHeight; + } + return nullopt; + } bool findBlock(const uint256& hash, const FoundBlock& block) override { WAIT_LOCK(cs_main, lock); diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index 03635fd74fa..b85484c11bd 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -87,11 +87,6 @@ public: public: virtual ~Lock() {} - //! Get block height above genesis block. Returns 0 for genesis block, - //! 1 for following block, and so on. Returns nullopt for a block not - //! included in the current chain. - virtual Optional getBlockHeight(const uint256& hash) = 0; - //! Get block hash. Height must be valid or this function will abort. virtual uint256 getBlockHash(int height) = 0; @@ -135,6 +130,11 @@ public: //! any blocks) virtual Optional getHeight() = 0; + //! Get block height above genesis block. Returns 0 for genesis block, + //! 1 for following block, and so on. Returns nullopt for a block not + //! included in the current chain. + virtual Optional getBlockHeight(const uint256& hash) = 0; + //! Return whether node has the block and optionally return block metadata //! or contents. virtual bool findBlock(const uint256& hash, const FoundBlock& block={}) = 0; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 3be0453978e..b7117e2e590 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -885,10 +885,9 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFlushOnClose) void CWallet::LoadToWallet(CWalletTx& wtxIn) { - // If wallet doesn't have a chain (e.g bitcoin-wallet), lock can't be taken. - auto locked_chain = LockChain(); - if (locked_chain) { - Optional block_height = locked_chain->getBlockHeight(wtxIn.m_confirm.hashBlock); + // If wallet doesn't have a chain (e.g wallet-tool), don't bother to update txn. + if (HaveChain()) { + Optional block_height = chain().getBlockHeight(wtxIn.m_confirm.hashBlock); if (block_height) { // Update cached block height variable since it not stored in the // serialized transaction. diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 662f5d4bb32..fa2821dfeae 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -778,6 +778,9 @@ public: /** 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; } + std::map mapWallet GUARDED_BY(cs_wallet); typedef std::multimap TxItems; From 0a76287387950bc9c5b634e95c5cd5fb1029f42d Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Tue, 16 Jul 2019 15:20:01 -0400 Subject: [PATCH 3/5] [wallet] Move getBlockHash from Chain::Lock interface to simple Chain --- src/interfaces/chain.cpp | 14 +++++++------- src/interfaces/chain.h | 6 +++--- src/wallet/wallet.cpp | 4 ++-- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp index ccdbf1f1d1d..a4702558dc4 100644 --- a/src/interfaces/chain.cpp +++ b/src/interfaces/chain.cpp @@ -55,13 +55,6 @@ bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLock { - uint256 getBlockHash(int height) override - { - LockAssertion lock(::cs_main); - CBlockIndex* block = ::ChainActive()[height]; - assert(block != nullptr); - return block->GetBlockHash(); - } bool haveBlockOnDisk(int height) override { LockAssertion lock(::cs_main); @@ -234,6 +227,13 @@ public: } return nullopt; } + uint256 getBlockHash(int height) override + { + LOCK(::cs_main); + CBlockIndex* block = ::ChainActive()[height]; + assert(block); + return block->GetBlockHash(); + } bool findBlock(const uint256& hash, const FoundBlock& block) override { WAIT_LOCK(cs_main, lock); diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index b85484c11bd..c82ade88974 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -87,9 +87,6 @@ public: public: virtual ~Lock() {} - //! Get block hash. Height must be valid or this function will abort. - virtual uint256 getBlockHash(int height) = 0; - //! Check that the block is available on disk (i.e. has not been //! pruned), and contains transactions. virtual bool haveBlockOnDisk(int height) = 0; @@ -135,6 +132,9 @@ public: //! included in the current chain. virtual Optional getBlockHeight(const uint256& hash) = 0; + //! Get block hash. Height must be valid or this function will abort. + virtual uint256 getBlockHash(int height) = 0; + //! Return whether node has the block and optionally return block metadata //! or contents. virtual bool findBlock(const uint256& hash, const FoundBlock& block={}) = 0; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index b7117e2e590..859a52f3e73 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3943,7 +3943,7 @@ std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, const Optional tip_height = chain.getHeight(); if (tip_height) { - walletInstance->m_last_block_processed = locked_chain->getBlockHash(*tip_height); + walletInstance->m_last_block_processed = chain.getBlockHash(*tip_height); walletInstance->m_last_block_processed_height = *tip_height; } else { walletInstance->m_last_block_processed.SetNull(); @@ -3989,7 +3989,7 @@ std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, { WalletRescanReserver reserver(*walletInstance); - if (!reserver.reserve() || (ScanResult::SUCCESS != walletInstance->ScanForWalletTransactions(locked_chain->getBlockHash(rescan_height), rescan_height, {} /* max height */, reserver, true /* update */).status)) { + if (!reserver.reserve() || (ScanResult::SUCCESS != walletInstance->ScanForWalletTransactions(chain.getBlockHash(rescan_height), rescan_height, {} /* max height */, reserver, true /* update */).status)) { error = _("Failed to rescan the wallet during initialization").translated; return nullptr; } From 841178820d31e1c24a00cb2c8fc0b1fd2f126f56 Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Tue, 16 Jul 2019 17:01:46 -0400 Subject: [PATCH 4/5] [wallet] Move methods from Chain::Lock interface to simple Chain Remove findPruned and findFork, no more used after 17954. --- src/interfaces/chain.cpp | 86 +++++++++++++------------------- src/interfaces/chain.h | 52 ++++++++----------- src/interfaces/wallet.cpp | 12 ++--- src/wallet/rpcwallet.cpp | 14 +++--- src/wallet/test/wallet_tests.cpp | 10 ++-- src/wallet/wallet.cpp | 38 +++++++------- src/wallet/wallet.h | 8 +-- 7 files changed, 97 insertions(+), 123 deletions(-) diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp index a4702558dc4..11cd20d785c 100644 --- a/src/interfaces/chain.cpp +++ b/src/interfaces/chain.cpp @@ -55,58 +55,6 @@ bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLock { - bool haveBlockOnDisk(int height) override - { - LockAssertion lock(::cs_main); - CBlockIndex* block = ::ChainActive()[height]; - return block && ((block->nStatus & BLOCK_HAVE_DATA) != 0) && block->nTx > 0; - } - Optional findFirstBlockWithTimeAndHeight(int64_t time, int height, uint256* hash) override - { - LockAssertion lock(::cs_main); - CBlockIndex* block = ::ChainActive().FindEarliestAtLeast(time, height); - if (block) { - if (hash) *hash = block->GetBlockHash(); - return block->nHeight; - } - return nullopt; - } - Optional findFork(const uint256& hash, Optional* height) override - { - LockAssertion lock(::cs_main); - const CBlockIndex* block = LookupBlockIndex(hash); - const CBlockIndex* fork = block ? ::ChainActive().FindFork(block) : nullptr; - if (height) { - if (block) { - *height = block->nHeight; - } else { - height->reset(); - } - } - if (fork) { - return fork->nHeight; - } - return nullopt; - } - CBlockLocator getTipLocator() override - { - LockAssertion lock(::cs_main); - return ::ChainActive().GetLocator(); - } - Optional findLocatorFork(const CBlockLocator& locator) override - { - LockAssertion lock(::cs_main); - if (CBlockIndex* fork = FindForkInGlobalIndex(::ChainActive(), locator)) { - return fork->nHeight; - } - return nullopt; - } - bool checkFinalTx(const CTransaction& tx) override - { - LockAssertion lock(::cs_main); - return CheckFinalTx(tx); - } - using UniqueLock::UniqueLock; }; @@ -234,6 +182,40 @@ public: assert(block); return block->GetBlockHash(); } + bool haveBlockOnDisk(int height) override + { + LOCK(cs_main); + CBlockIndex* block = ::ChainActive()[height]; + return block && ((block->nStatus & BLOCK_HAVE_DATA) != 0) && block->nTx > 0; + } + Optional findFirstBlockWithTimeAndHeight(int64_t time, int height, uint256* hash) override + { + LOCK(cs_main); + CBlockIndex* block = ::ChainActive().FindEarliestAtLeast(time, height); + if (block) { + if (hash) *hash = block->GetBlockHash(); + return block->nHeight; + } + return nullopt; + } + CBlockLocator getTipLocator() override + { + LOCK(cs_main); + return ::ChainActive().GetLocator(); + } + bool checkFinalTx(const CTransaction& tx) override + { + LOCK(cs_main); + return CheckFinalTx(tx); + } + Optional findLocatorFork(const CBlockLocator& locator) override + { + LOCK(cs_main); + if (CBlockIndex* fork = FindForkInGlobalIndex(::ChainActive(), locator)) { + return fork->nHeight; + } + return nullopt; + } bool findBlock(const uint256& hash, const FoundBlock& block) override { WAIT_LOCK(cs_main, lock); diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index c82ade88974..d35d3869252 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -86,36 +86,6 @@ public: { public: virtual ~Lock() {} - - //! Check that the block is available on disk (i.e. has not been - //! pruned), and contains transactions. - virtual bool haveBlockOnDisk(int height) = 0; - - //! Return height of the first block in the chain with timestamp equal - //! or greater than the given time and height equal or greater than the - //! given height, or nullopt if there is no block with a high enough - //! timestamp and height. Also return the block hash as an optional output parameter - //! (to avoid the cost of a second lookup in case this information is needed.) - virtual Optional findFirstBlockWithTimeAndHeight(int64_t time, int height, uint256* hash) = 0; - - //! Return height of the specified block if it is on the chain, otherwise - //! return the height of the highest block on chain that's an ancestor - //! of the specified block, or nullopt if there is no common ancestor. - //! Also return the height of the specified block as an optional output - //! parameter (to avoid the cost of a second hash lookup in case this - //! information is desired). - virtual Optional findFork(const uint256& hash, Optional* height) = 0; - - //! Get locator for the current chain tip. - virtual CBlockLocator getTipLocator() = 0; - - //! Return height of the highest block on chain in common with the locator, - //! which will either be the original block used to create the locator, - //! or one of its ancestors. - virtual Optional findLocatorFork(const CBlockLocator& locator) = 0; - - //! Check if transaction will be final given chain height current time. - virtual bool checkFinalTx(const CTransaction& tx) = 0; }; //! Return Lock interface. Chain is locked when this is called, and @@ -135,6 +105,28 @@ public: //! Get block hash. Height must be valid or this function will abort. virtual uint256 getBlockHash(int height) = 0; + //! Check that the block is available on disk (i.e. has not been + //! pruned), and contains transactions. + virtual bool haveBlockOnDisk(int height) = 0; + + //! Return height of the first block in the chain with timestamp equal + //! or greater than the given time and height equal or greater than the + //! given height, or nullopt if there is no block with a high enough + //! timestamp and height. Also return the block hash as an optional output parameter + //! (to avoid the cost of a second lookup in case this information is needed.) + virtual Optional findFirstBlockWithTimeAndHeight(int64_t time, int height, uint256* hash) = 0; + + //! Get locator for the current chain tip. + virtual CBlockLocator getTipLocator() = 0; + + //! Return height of the highest block on chain in common with the locator, + //! which will either be the original block used to create the locator, + //! or one of its ancestors. + virtual Optional findLocatorFork(const CBlockLocator& locator) = 0; + + //! Check if transaction will be final given chain height current time. + virtual bool checkFinalTx(const CTransaction& tx) = 0; + //! Return whether node has the block and optionally return block metadata //! or contents. virtual bool findBlock(const uint256& hash, const FoundBlock& block={}) = 0; diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index 0c31ae0c818..de26f329d71 100644 --- a/src/interfaces/wallet.cpp +++ b/src/interfaces/wallet.cpp @@ -60,7 +60,7 @@ WalletTx MakeWalletTx(CWallet& wallet, const CWalletTx& wtx) } //! Construct wallet tx status struct. -WalletTxStatus MakeWalletTxStatus(interfaces::Chain::Lock& locked_chain, const CWalletTx& wtx) +WalletTxStatus MakeWalletTxStatus(CWallet& wallet, const CWalletTx& wtx) { WalletTxStatus result; result.block_height = wtx.m_confirm.block_height > 0 ? wtx.m_confirm.block_height : std::numeric_limits::max(); @@ -68,8 +68,8 @@ WalletTxStatus MakeWalletTxStatus(interfaces::Chain::Lock& locked_chain, const C result.depth_in_main_chain = wtx.GetDepthInMainChain(); result.time_received = wtx.nTimeReceived; result.lock_time = wtx.tx->nLockTime; - result.is_final = locked_chain.checkFinalTx(*wtx.tx); - result.is_trusted = wtx.IsTrusted(locked_chain); + result.is_final = wallet.chain().checkFinalTx(*wtx.tx); + result.is_trusted = wtx.IsTrusted(); result.is_abandoned = wtx.isAbandoned(); result.is_coinbase = wtx.IsCoinBase(); result.is_in_main_chain = wtx.IsInMainChain(); @@ -322,7 +322,7 @@ public: num_blocks = m_wallet->GetLastBlockHeight(); block_time = -1; CHECK_NONFATAL(m_wallet->chain().findBlock(m_wallet->GetLastBlockHash(), FoundBlock().time(block_time))); - tx_status = MakeWalletTxStatus(*locked_chain, mi->second); + tx_status = MakeWalletTxStatus(*m_wallet, mi->second); return true; } WalletTx getWalletTxDetails(const uint256& txid, @@ -338,7 +338,7 @@ public: num_blocks = m_wallet->GetLastBlockHeight(); in_mempool = mi->second.InMempool(); order_form = mi->second.vOrderForm; - tx_status = MakeWalletTxStatus(*locked_chain, mi->second); + tx_status = MakeWalletTxStatus(*m_wallet, mi->second); return MakeWalletTx(*m_wallet, mi->second); } return {}; @@ -413,7 +413,7 @@ public: auto locked_chain = m_wallet->chain().lock(); LOCK(m_wallet->cs_wallet); CoinsList result; - for (const auto& entry : m_wallet->ListCoins(*locked_chain)) { + for (const auto& entry : m_wallet->ListCoins()) { auto& group = result[entry.first]; for (const auto& coin : entry.second) { group.emplace_back(COutPoint(coin.tx->GetHash(), coin.i), diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 4c78e6fdce1..4a026f9ef4b 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -148,7 +148,7 @@ static void WalletTxToJSON(interfaces::Chain& chain, interfaces::Chain::Lock& lo CHECK_NONFATAL(chain.findBlock(wtx.m_confirm.hashBlock, FoundBlock().time(block_time))); entry.pushKV("blocktime", block_time); } else { - entry.pushKV("trusted", wtx.IsTrusted(locked_chain)); + entry.pushKV("trusted", wtx.IsTrusted()); } uint256 hash = wtx.GetHash(); entry.pushKV("txid", hash.GetHex()); @@ -572,7 +572,7 @@ static UniValue signmessage(const JSONRPCRequest& request) return signature; } -static CAmount GetReceived(interfaces::Chain::Lock& locked_chain, const CWallet& wallet, const UniValue& params, bool by_label) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) +static CAmount GetReceived(const CWallet& wallet, const UniValue& params, bool by_label) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet) { std::set address_set; @@ -602,7 +602,7 @@ static CAmount GetReceived(interfaces::Chain::Lock& locked_chain, const CWallet& CAmount amount = 0; for (const std::pair& wtx_pair : wallet.mapWallet) { const CWalletTx& wtx = wtx_pair.second; - if (wtx.IsCoinBase() || !locked_chain.checkFinalTx(*wtx.tx) || wtx.GetDepthInMainChain() < min_depth) { + if (wtx.IsCoinBase() || !wallet.chain().checkFinalTx(*wtx.tx) || wtx.GetDepthInMainChain() < min_depth) { continue; } @@ -655,7 +655,7 @@ static UniValue getreceivedbyaddress(const JSONRPCRequest& request) auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); - return ValueFromAmount(GetReceived(*locked_chain, *pwallet, request.params, /* by_label */ false)); + return ValueFromAmount(GetReceived(*pwallet, request.params, /* by_label */ false)); } @@ -696,7 +696,7 @@ static UniValue getreceivedbylabel(const JSONRPCRequest& request) auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); - return ValueFromAmount(GetReceived(*locked_chain, *pwallet, request.params, /* by_label */ true)); + return ValueFromAmount(GetReceived(*pwallet, request.params, /* by_label */ true)); } @@ -1049,7 +1049,7 @@ static UniValue ListReceived(interfaces::Chain::Lock& locked_chain, const CWalle for (const std::pair& pairWtx : pwallet->mapWallet) { const CWalletTx& wtx = pairWtx.second; - if (wtx.IsCoinBase() || !locked_chain.checkFinalTx(*wtx.tx)) { + if (wtx.IsCoinBase() || !pwallet->chain().checkFinalTx(*wtx.tx)) { continue; } @@ -2942,7 +2942,7 @@ static UniValue listunspent(const JSONRPCRequest& request) cctl.m_max_depth = nMaxDepth; auto locked_chain = pwallet->chain().lock(); LOCK(pwallet->cs_wallet); - pwallet->AvailableCoins(*locked_chain, vecOutputs, !include_unsafe, &cctl, nMinimumAmount, nMaximumAmount, nMinimumSumAmount, nMaximumCount); + pwallet->AvailableCoins(vecOutputs, !include_unsafe, &cctl, nMinimumAmount, nMaximumAmount, nMinimumSumAmount, nMaximumCount); } LOCK(pwallet->cs_wallet); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 1c569cbef1d..227cf275cc6 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -556,7 +556,7 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup) { auto locked_chain = m_chain->lock(); LOCK(wallet->cs_wallet); - list = wallet->ListCoins(*locked_chain); + list = wallet->ListCoins(); } BOOST_CHECK_EQUAL(list.size(), 1U); BOOST_CHECK_EQUAL(boost::get(list.begin()->first).ToString(), coinbaseAddress); @@ -573,7 +573,7 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup) { auto locked_chain = m_chain->lock(); LOCK(wallet->cs_wallet); - list = wallet->ListCoins(*locked_chain); + list = wallet->ListCoins(); } BOOST_CHECK_EQUAL(list.size(), 1U); BOOST_CHECK_EQUAL(boost::get(list.begin()->first).ToString(), coinbaseAddress); @@ -584,7 +584,7 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup) auto locked_chain = m_chain->lock(); LOCK(wallet->cs_wallet); std::vector available; - wallet->AvailableCoins(*locked_chain, available); + wallet->AvailableCoins(available); BOOST_CHECK_EQUAL(available.size(), 2U); } for (const auto& group : list) { @@ -597,7 +597,7 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup) auto locked_chain = m_chain->lock(); LOCK(wallet->cs_wallet); std::vector available; - wallet->AvailableCoins(*locked_chain, available); + wallet->AvailableCoins(available); BOOST_CHECK_EQUAL(available.size(), 0U); } // Confirm ListCoins still returns same result as before, despite coins @@ -605,7 +605,7 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup) { auto locked_chain = m_chain->lock(); LOCK(wallet->cs_wallet); - list = wallet->ListCoins(*locked_chain); + list = wallet->ListCoins(); } 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 859a52f3e73..6e8f7e0e8f7 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1927,16 +1927,16 @@ bool CWalletTx::InMempool() const return fInMempool; } -bool CWalletTx::IsTrusted(interfaces::Chain::Lock& locked_chain) const +bool CWalletTx::IsTrusted() const { std::set s; - return IsTrusted(locked_chain, s); + return IsTrusted(s); } -bool CWalletTx::IsTrusted(interfaces::Chain::Lock& locked_chain, std::set& trusted_parents) const +bool CWalletTx::IsTrusted(std::set& trusted_parents) const { // Quick answer in most cases - if (!locked_chain.checkFinalTx(*tx)) return false; + if (!pwallet->chain().checkFinalTx(*tx)) return false; int nDepth = GetDepthInMainChain(); if (nDepth >= 1) return true; if (nDepth < 0) return false; @@ -1958,7 +1958,7 @@ bool CWalletTx::IsTrusted(interfaces::Chain::Lock& locked_chain, std::setGetHash())) continue; // Recurse to check that the parent is also trusted - if (!parent->IsTrusted(locked_chain, trusted_parents)) return false; + if (!parent->IsTrusted(trusted_parents)) return false; trusted_parents.insert(parent->GetHash()); } return true; @@ -2050,7 +2050,7 @@ CWallet::Balance CWallet::GetBalance(const int min_depth, bool avoid_reuse) cons for (const auto& entry : mapWallet) { const CWalletTx& wtx = entry.second; - const bool is_trusted{wtx.IsTrusted(*locked_chain, trusted_parents)}; + const bool is_trusted{wtx.IsTrusted(trusted_parents)}; const int tx_depth{wtx.GetDepthInMainChain()}; const CAmount tx_credit_mine{wtx.GetAvailableCredit(/* fUseCache */ true, ISMINE_SPENDABLE | reuse_filter)}; const CAmount tx_credit_watchonly{wtx.GetAvailableCredit(/* fUseCache */ true, ISMINE_WATCH_ONLY | reuse_filter)}; @@ -2076,7 +2076,7 @@ CAmount CWallet::GetAvailableBalance(const CCoinControl* coinControl) const CAmount balance = 0; std::vector vCoins; - AvailableCoins(*locked_chain, vCoins, true, coinControl); + AvailableCoins(vCoins, true, coinControl); for (const COutput& out : vCoins) { if (out.fSpendable) { balance += out.tx->tx->vout[out.i].nValue; @@ -2085,7 +2085,7 @@ CAmount CWallet::GetAvailableBalance(const CCoinControl* coinControl) const return balance; } -void CWallet::AvailableCoins(interfaces::Chain::Lock& locked_chain, std::vector& vCoins, bool fOnlySafe, const CCoinControl* coinControl, const CAmount& nMinimumAmount, const CAmount& nMaximumAmount, const CAmount& nMinimumSumAmount, const uint64_t nMaximumCount) const +void CWallet::AvailableCoins(std::vector& vCoins, bool fOnlySafe, const CCoinControl* coinControl, const CAmount& nMinimumAmount, const CAmount& nMaximumAmount, const CAmount& nMinimumSumAmount, const uint64_t nMaximumCount) const { AssertLockHeld(cs_wallet); @@ -2103,7 +2103,7 @@ void CWallet::AvailableCoins(interfaces::Chain::Lock& locked_chain, std::vector< const uint256& wtxid = entry.first; const CWalletTx& wtx = entry.second; - if (!locked_chain.checkFinalTx(*wtx.tx)) { + if (!chain().checkFinalTx(*wtx.tx)) { continue; } @@ -2119,7 +2119,7 @@ void CWallet::AvailableCoins(interfaces::Chain::Lock& locked_chain, std::vector< if (nDepth == 0 && !wtx.InMempool()) continue; - bool safeTx = wtx.IsTrusted(locked_chain, trusted_parents); + bool safeTx = wtx.IsTrusted(trusted_parents); // We should not consider coins from transactions that are replacing // other transactions. @@ -2207,14 +2207,14 @@ void CWallet::AvailableCoins(interfaces::Chain::Lock& locked_chain, std::vector< } } -std::map> CWallet::ListCoins(interfaces::Chain::Lock& locked_chain) const +std::map> CWallet::ListCoins() const { AssertLockHeld(cs_wallet); std::map> result; std::vector availableCoins; - AvailableCoins(locked_chain, availableCoins); + AvailableCoins(availableCoins); for (const COutput& coin : availableCoins) { CTxDestination address; @@ -2706,7 +2706,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CTransac txNew.nLockTime = GetLocktimeForNewTransaction(chain(), GetLastBlockHash(), GetLastBlockHeight()); { std::vector vAvailableCoins; - AvailableCoins(*locked_chain, vAvailableCoins, true, &coin_control, 1, MAX_MONEY, MAX_MONEY, 0); + AvailableCoins(vAvailableCoins, true, &coin_control, 1, MAX_MONEY, MAX_MONEY, 0); CoinSelectionParams coin_selection_params; // Parameters for coin selection, init with dummy // Create change script that will be used if we need change @@ -3293,7 +3293,7 @@ std::map CWallet::GetAddressBalances(interfaces::Chain: { const CWalletTx& wtx = walletEntry.second; - if (!wtx.IsTrusted(locked_chain, trusted_parents)) + if (!wtx.IsTrusted(trusted_parents)) continue; if (wtx.IsImmatureCoinBase()) @@ -3813,7 +3813,7 @@ std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, } auto locked_chain = chain.lock(); - walletInstance->chainStateFlushed(locked_chain->getTipLocator()); + walletInstance->chainStateFlushed(chain.getTipLocator()); } else if (wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS) { // Make it impossible to disable private keys after creation error = strprintf(_("Error loading %s: Private keys can only be disabled during creation").translated, walletFile); @@ -3935,7 +3935,7 @@ std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, WalletBatch batch(*walletInstance->database); CBlockLocator locator; if (batch.ReadBestBlock(locator)) { - if (const Optional fork_height = locked_chain->findLocatorFork(locator)) { + if (const Optional fork_height = chain.findLocatorFork(locator)) { rescan_height = *fork_height; } } @@ -3960,7 +3960,7 @@ std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, // If a block is pruned after this check, we will load the wallet, // but fail the rescan with a generic error. int block_height = *tip_height; - while (block_height > 0 && locked_chain->haveBlockOnDisk(block_height - 1) && rescan_height != block_height) { + while (block_height > 0 && chain.haveBlockOnDisk(block_height - 1) && rescan_height != block_height) { --block_height; } @@ -3982,7 +3982,7 @@ std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, if (!time_first_key || time < *time_first_key) time_first_key = time; } if (time_first_key) { - if (Optional first_block = locked_chain->findFirstBlockWithTimeAndHeight(*time_first_key - TIMESTAMP_WINDOW, rescan_height, nullptr)) { + if (Optional first_block = chain.findFirstBlockWithTimeAndHeight(*time_first_key - TIMESTAMP_WINDOW, rescan_height, nullptr)) { rescan_height = *first_block; } } @@ -3994,7 +3994,7 @@ std::shared_ptr CWallet::CreateWalletFromFile(interfaces::Chain& chain, return nullptr; } } - walletInstance->chainStateFlushed(locked_chain->getTipLocator()); + walletInstance->chainStateFlushed(chain.getTipLocator()); walletInstance->database->IncrementUpdateCounter(); // Restore wallet transaction metadata after -zapwallettxes=1 diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index fa2821dfeae..fb23ea82a73 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -499,8 +499,8 @@ public: bool IsEquivalentTo(const CWalletTx& tx) const; bool InMempool() const; - bool IsTrusted(interfaces::Chain::Lock& locked_chain) const; - bool IsTrusted(interfaces::Chain::Lock& locked_chain, std::set& trusted_parents) const; + bool IsTrusted() const; + bool IsTrusted(std::set& trusted_parents) const; int64_t GetTxTime() const; @@ -808,12 +808,12 @@ public: /** * populate vCoins with vector of available COutputs. */ - void AvailableCoins(interfaces::Chain::Lock& locked_chain, std::vector& vCoins, bool fOnlySafe = true, const CCoinControl* coinControl = nullptr, const CAmount& nMinimumAmount = 1, const CAmount& nMaximumAmount = MAX_MONEY, const CAmount& nMinimumSumAmount = MAX_MONEY, const uint64_t nMaximumCount = 0) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + void AvailableCoins(std::vector& vCoins, bool fOnlySafe = true, const CCoinControl* coinControl = nullptr, const CAmount& nMinimumAmount = 1, const CAmount& nMaximumAmount = MAX_MONEY, const CAmount& nMinimumSumAmount = MAX_MONEY, const uint64_t nMaximumCount = 0) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /** * Return list of available coins and locked coins grouped by non-change output address. */ - std::map> ListCoins(interfaces::Chain::Lock& locked_chain) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + std::map> ListCoins() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /** * Find non-change parent output. From 6a72f26968cf931c985d8d4797b6264274cabd06 Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Wed, 18 Dec 2019 17:46:53 -0500 Subject: [PATCH 5/5] [wallet] Remove locked_chain from CWallet, its RPCs and tests This change is intended to make the bitcoin node and its rpc, network and gui interfaces more responsive while the wallet is in use. Currently because the node's cs_main mutex is always locked before the wallet's cs_wallet mutex (to prevent deadlocks), cs_main currently stays locked while the wallet does relatively slow things like creating and listing transactions. This commit only remmove chain lock tacking in wallet code, and invert lock order from cs_main, cs_wallet to cs_wallet, cs_main. must happen at once to avoid any deadlock. Previous commit were only removing Chain::Lock methods to Chain interface and enforcing they take cs_main. Remove LockChain method from CWallet and Chain::Lock interface. --- src/interfaces/chain.cpp | 12 ---- src/interfaces/chain.h | 30 +++------ src/interfaces/wallet.cpp | 24 +------ src/interfaces/wallet.h | 3 + src/qt/test/wallettests.cpp | 1 - src/wallet/feebumper.cpp | 4 -- src/wallet/rpcdump.cpp | 16 +---- src/wallet/rpcwallet.cpp | 57 ++++------------- src/wallet/test/wallet_tests.cpp | 105 ++++++++++++++++++------------- src/wallet/wallet.cpp | 48 +++++--------- 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 11cd20d785c..c5262e4bc0a 100644 --- a/src/interfaces/chain.cpp +++ b/src/interfaces/chain.cpp @@ -53,11 +53,6 @@ bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLock -{ - 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 lock(bool try_lock) override - { - auto lock = MakeUnique(::cs_main, "cs_main", __FILE__, __LINE__, try_lock); - if (try_lock && lock && !*lock) return {}; - std::unique_ptr result = std::move(lock); // Temporary to avoid CWG 1579 - return result; - } Optional getHeight() override { LOCK(::cs_main); diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index d35d3869252..e33fe54ac8b 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(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 de26f329d71..752448aac79 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& 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 getWalletTxs() override { - auto locked_chain = m_wallet->chain().lock(); LOCK(m_wallet->cs_wallet); std::vector 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 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()); @@ -496,6 +473,7 @@ public: { return MakeHandler(m_wallet->NotifyCanGetAddressesChanged.connect(fn)); } + CWallet* wallet() override { return m_wallet.get(); } std::shared_ptr m_wallet; }; diff --git a/src/interfaces/wallet.h b/src/interfaces/wallet.h index 5d870c5e3d8..e5e3b806638 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; 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/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index 94a1e0a63db..2ee9ae0d866 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 d596db431ac..01fcb1680e7 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& 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 9417e2bd586..c863d225307 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 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 mapKeyBirth; const std::map& mapKeyPool = spk_man.GetAllReserveKeys(); - pwallet->GetKeyBirthTimes(*locked_chain, mapKeyBirth); + pwallet->GetKeyBirthTimes(mapKeyBirth); std::set 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 4a026f9ef4b..91162d575dc 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 balances = pwallet->GetAddressBalances(*locked_chain); + std::map balances = pwallet->GetAddressBalances(); for (const std::set& 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 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 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 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 227cf275cc6..c049f2f15f2 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(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> 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 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 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 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 6e8f7e0e8f7..a20ede59fd4 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 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& vecSend, CTransac int nBytes; { std::set 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& vecSend, CTransac 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)); @@ -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& destinations } } -std::map CWallet::GetAddressBalances(interfaces::Chain::Lock& locked_chain) const +std::map CWallet::GetAddressBalances() const { std::map balances; @@ -3509,7 +3490,7 @@ void CWallet::ListLockedCoins(std::vector& vOutpts) const /** @} */ // 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(); @@ -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::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::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::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 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 &mapKeyBirth) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + void GetKeyBirthTimes(std::map &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> GetAddressGroupings() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - std::map GetAddressBalances(interfaces::Chain::Lock& locked_chain) const; + std::map GetAddressBalances() const; std::set GetLabelAddresses(const std::string& label) const;