diff options
Diffstat (limited to 'src')
-rw-r--r-- | src/interfaces/chain.cpp | 144 | ||||
-rw-r--r-- | src/interfaces/chain.h | 104 | ||||
-rw-r--r-- | src/interfaces/wallet.cpp | 40 | ||||
-rw-r--r-- | src/interfaces/wallet.h | 3 | ||||
-rw-r--r-- | src/qt/test/wallettests.cpp | 1 | ||||
-rw-r--r-- | src/wallet/feebumper.cpp | 6 | ||||
-rw-r--r-- | src/wallet/rpcdump.cpp | 16 | ||||
-rw-r--r-- | src/wallet/rpcwallet.cpp | 82 | ||||
-rw-r--r-- | src/wallet/test/wallet_tests.cpp | 117 | ||||
-rw-r--r-- | src/wallet/wallet.cpp | 104 | ||||
-rw-r--r-- | src/wallet/wallet.h | 19 |
11 files changed, 257 insertions, 379 deletions
diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp index 0e7641ae32..c5262e4bc0 100644 --- a/src/interfaces/chain.cpp +++ b/src/interfaces/chain.cpp @@ -53,88 +53,6 @@ bool FillBlock(const CBlockIndex* index, const FoundBlock& block, UniqueLock<Rec return true; } -class LockImpl : public Chain::Lock, public UniqueLock<RecursiveMutex> -{ - Optional<int> getHeight() override - { - LockAssertion lock(::cs_main); - int height = ::ChainActive().Height(); - if (height >= 0) { - return height; - } - return nullopt; - } - Optional<int> 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); - CBlockIndex* block = ::ChainActive()[height]; - assert(block != nullptr); - return block->GetBlockHash(); - } - 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<int> 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<int> findFork(const uint256& hash, Optional<int>* 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<int> 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; -}; - class NotificationsProxy : public CValidationInterface { public: @@ -227,12 +145,64 @@ class ChainImpl : public Chain { public: explicit ChainImpl(NodeContext& node) : m_node(node) {} - std::unique_ptr<Chain::Lock> lock(bool try_lock) override + Optional<int> getHeight() override + { + LOCK(::cs_main); + int height = ::ChainActive().Height(); + if (height >= 0) { + return height; + } + return nullopt; + } + Optional<int> getBlockHeight(const uint256& hash) override + { + LOCK(::cs_main); + CBlockIndex* block = LookupBlockIndex(hash); + if (block && ::ChainActive().Contains(block)) { + return block->nHeight; + } + return nullopt; + } + uint256 getBlockHash(int height) override + { + LOCK(::cs_main); + CBlockIndex* block = ::ChainActive()[height]; + 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<int> findFirstBlockWithTimeAndHeight(int64_t time, int height, uint256* hash) 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; + 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<int> 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 { diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index fb77c81cdc..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,68 +67,53 @@ 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() {} - - //! 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<int> 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<int> getBlockHeight(const uint256& hash) = 0; - - //! 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<int> 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<int> findFork(const uint256& hash, Optional<int>* 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<int> findLocatorFork(const CBlockLocator& locator) = 0; - - //! Check if transaction will be final given chain height current time. - virtual bool checkFinalTx(const CTransaction& tx) = 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<int> 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<int> getBlockHeight(const uint256& hash) = 0; + + //! 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<int> 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<int> findLocatorFork(const CBlockLocator& locator) = 0; - //! 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; + //! 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. diff --git a/src/interfaces/wallet.cpp b/src/interfaces/wallet.cpp index 152917ac60..752448aac7 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<int>::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(); @@ -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,10 +221,9 @@ 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(*locked_chain, recipients, tx, fee, change_pos, + if (!m_wallet->CreateTransaction(recipients, tx, fee, change_pos, fail_reason, coin_control, sign)) { return {}; } @@ -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; @@ -322,7 +308,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, @@ -331,14 +317,13 @@ 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()) { - 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); + tx_status = MakeWalletTxStatus(*m_wallet, mi->second); return MakeWalletTx(*m_wallet, mi->second); } return {}; @@ -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,34 +369,29 @@ 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(*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), @@ -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 dafa022ded..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); @@ -219,7 +217,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; } @@ -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 49c583c422..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); @@ -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()); @@ -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); @@ -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()); @@ -445,7 +444,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(); } @@ -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); @@ -572,7 +569,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<CTxDestination> address_set; @@ -602,7 +599,7 @@ static CAmount GetReceived(interfaces::Chain::Lock& locked_chain, const CWallet& CAmount amount = 0; for (const std::pair<const uint256, CWalletTx>& 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; } @@ -652,10 +649,9 @@ 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(*locked_chain, *pwallet, request.params, /* by_label */ false)); + return ValueFromAmount(GetReceived(*pwallet, request.params, /* by_label */ false)); } @@ -693,10 +689,9 @@ 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(*locked_chain, *pwallet, request.params, /* by_label */ true)); + 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()) { @@ -913,7 +905,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 */); @@ -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; @@ -1049,7 +1040,7 @@ static UniValue ListReceived(interfaces::Chain::Lock& locked_chain, const CWalle for (const std::pair<const uint256, CWalletTx>& 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; } @@ -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,9 +2914,8 @@ 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(*locked_chain, vecOutputs, !include_unsafe, &cctl, nMinimumAmount, nMaximumAmount, nMinimumSumAmount, nMaximumCount); + pwallet->AvailableCoins(vecOutputs, !include_unsafe, &cctl, nMinimumAmount, nMaximumAmount, nMinimumSumAmount, nMaximumCount); } LOCK(pwallet->cs_wallet); @@ -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(); @@ -3563,8 +3533,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"); } } @@ -4011,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 1215956bb4..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,8 +518,7 @@ public: std::string error; 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; @@ -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,9 +550,8 @@ 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(*locked_chain); + list = wallet->ListCoins(); } BOOST_CHECK_EQUAL(list.size(), 1U); BOOST_CHECK_EQUAL(boost::get<PKHash>(list.begin()->first).ToString(), coinbaseAddress); @@ -571,9 +566,8 @@ 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(*locked_chain); + list = wallet->ListCoins(); } BOOST_CHECK_EQUAL(list.size(), 1U); BOOST_CHECK_EQUAL(boost::get<PKHash>(list.begin()->first).ToString(), coinbaseAddress); @@ -581,10 +575,9 @@ 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(*locked_chain, available); + wallet->AvailableCoins(available); BOOST_CHECK_EQUAL(available.size(), 2U); } for (const auto& group : list) { @@ -594,18 +587,16 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup) } } { - auto locked_chain = m_chain->lock(); LOCK(wallet->cs_wallet); std::vector<COutput> 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 // being locked. { - 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<PKHash>(list.begin()->first).ToString(), coinbaseAddress); @@ -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 6d46841cee..a20ede59fd 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<int> 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<int> block_height = chain().getBlockHeight(wtxIn.m_confirm.hashBlock); if (block_height) { // Update cached block height variable since it not stored in the // serialized transaction. @@ -975,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(); @@ -993,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+"); @@ -1048,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; @@ -1111,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); @@ -1133,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; @@ -1147,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 @@ -1686,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) { @@ -1715,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 @@ -1928,16 +1919,16 @@ bool CWalletTx::InMempool() const return fInMempool; } -bool CWalletTx::IsTrusted(interfaces::Chain::Lock& locked_chain) const +bool CWalletTx::IsTrusted() const { std::set<uint256> s; - return IsTrusted(locked_chain, s); + return IsTrusted(s); } -bool CWalletTx::IsTrusted(interfaces::Chain::Lock& locked_chain, std::set<uint256>& trusted_parents) const +bool CWalletTx::IsTrusted(std::set<uint256>& 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; @@ -1959,7 +1950,7 @@ bool CWalletTx::IsTrusted(interfaces::Chain::Lock& locked_chain, std::set<uint25 // If we've already trusted this parent, continue if (trusted_parents.count(parent->GetHash())) 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; @@ -2003,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 @@ -2017,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); @@ -2045,13 +2035,12 @@ 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) { 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)}; @@ -2072,12 +2061,11 @@ 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; std::vector<COutput> 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; @@ -2086,7 +2074,7 @@ CAmount CWallet::GetAvailableBalance(const CCoinControl* coinControl) const return balance; } -void CWallet::AvailableCoins(interfaces::Chain::Lock& locked_chain, std::vector<COutput>& 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<COutput>& vCoins, bool fOnlySafe, const CCoinControl* coinControl, const CAmount& nMinimumAmount, const CAmount& nMaximumAmount, const CAmount& nMinimumSumAmount, const uint64_t nMaximumCount) const { AssertLockHeld(cs_wallet); @@ -2104,7 +2092,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; } @@ -2120,7 +2108,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. @@ -2208,14 +2196,14 @@ void CWallet::AvailableCoins(interfaces::Chain::Lock& locked_chain, std::vector< } } -std::map<CTxDestination, std::vector<COutput>> CWallet::ListCoins(interfaces::Chain::Lock& locked_chain) const +std::map<CTxDestination, std::vector<COutput>> CWallet::ListCoins() const { AssertLockHeld(cs_wallet); std::map<CTxDestination, std::vector<COutput>> result; std::vector<COutput> availableCoins; - AvailableCoins(locked_chain, availableCoins); + AvailableCoins(availableCoins); for (const COutput& coin : availableCoins) { CTxDestination address; @@ -2551,11 +2539,10 @@ 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; - 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 +2658,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<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, - int& nChangePosInOut, std::string& strFailReason, const CCoinControl& coin_control, bool sign) +bool CWallet::CreateTransaction(const std::vector<CRecipient>& 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); @@ -2703,12 +2689,11 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std int nBytes; { std::set<CInputCoin> setCoins; - auto locked_chain = chain().lock(); LOCK(cs_wallet); txNew.nLockTime = GetLocktimeForNewTransaction(chain(), GetLastBlockHash(), GetLastBlockHeight()); { std::vector<COutput> 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 @@ -3021,7 +3006,6 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std 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)); @@ -3061,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; @@ -3284,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; @@ -3295,7 +3274,7 @@ std::map<CTxDestination, CAmount> 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()) @@ -3511,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(); @@ -3721,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; } @@ -3814,8 +3788,7 @@ std::shared_ptr<CWallet> 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); @@ -3928,24 +3901,33 @@ 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)) { WalletBatch batch(*walletInstance->database); CBlockLocator locator; if (batch.ReadBestBlock(locator)) { - if (const Optional<int> fork_height = locked_chain->findLocatorFork(locator)) { + if (const Optional<int> fork_height = chain.findLocatorFork(locator)) { rescan_height = *fork_height; } } } - const Optional<int> tip_height = locked_chain->getHeight(); + const Optional<int> 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(); @@ -3962,7 +3944,7 @@ std::shared_ptr<CWallet> 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; } @@ -3984,19 +3966,19 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain, if (!time_first_key || time < *time_first_key) time_first_key = time; } if (time_first_key) { - if (Optional<int> first_block = locked_chain->findFirstBlockWithTimeAndHeight(*time_first_key - TIMESTAMP_WINDOW, rescan_height, nullptr)) { + if (Optional<int> first_block = chain.findFirstBlockWithTimeAndHeight(*time_first_key - TIMESTAMP_WINDOW, rescan_height, nullptr)) { rescan_height = *first_block; } } { 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; } } - walletInstance->chainStateFlushed(locked_chain->getTipLocator()); + walletInstance->chainStateFlushed(chain.getTipLocator()); walletInstance->database->IncrementUpdateCounter(); // Restore wallet transaction metadata after -zapwallettxes=1 @@ -4031,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)); { @@ -4093,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 24c78fceb3..72b3cf1fb8 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<uint256>& trusted_parents) const; + bool IsTrusted() const; + bool IsTrusted(std::set<uint256>& trusted_parents) const; int64_t GetTxTime() const; @@ -775,8 +775,8 @@ 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; } std::map<uint256, CWalletTx> mapWallet GUARDED_BY(cs_wallet); @@ -805,12 +805,12 @@ public: /** * populate vCoins with vector of available COutputs. */ - void AvailableCoins(interfaces::Chain::Lock& locked_chain, std::vector<COutput>& 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<COutput>& 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<CTxDestination, std::vector<COutput>> ListCoins(interfaces::Chain::Lock& locked_chain) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + std::map<CTxDestination, std::vector<COutput>> ListCoins() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /** * Find non-change parent output. @@ -875,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; /** @@ -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<CRecipient>& vecSend, CTransactionRef& tx, CAmount& nFeeRet, int& nChangePosInOut, - std::string& strFailReason, const CCoinControl& coin_control, bool sign = true); + bool CreateTransaction(const std::vector<CRecipient>& 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 @@ -1012,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; |