diff --git a/src/wallet/receive.cpp b/src/wallet/receive.cpp index e3b1f8a76dcb37..329c20f332ef55 100644 --- a/src/wallet/receive.cpp +++ b/src/wallet/receive.cpp @@ -280,6 +280,8 @@ Balance GetBalance(const CWallet& wallet, const int min_depth, bool avoid_reuse) const bool is_trusted{CachedTxIsTrusted(wallet, txo.GetState(), outpoint.hash)}; const int tx_depth{wallet.GetTxStateDepthInMainChain(txo.GetState())}; + Assert(tx_depth >= 0); + Assert(!wallet.IsSpent(outpoint, /*min_depth=*/1)); if (!wallet.IsSpent(outpoint) && (allow_used_addresses || !wallet.IsSpentKey(txo.GetTxOut().scriptPubKey))) { // Get the amounts for mine and watchonly diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index d1ff852ad387b9..373115579f21ac 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -331,6 +331,10 @@ CoinsResult AvailableCoins(const CWallet& wallet, if (wallet.IsLockedCoin(outpoint) && params.skip_locked) continue; + int nDepth = wallet.GetTxStateDepthInMainChain(txo.GetState()); + Assert(nDepth >= 0); + Assert(!wallet.IsSpent(outpoint, /*min_depth=*/1)); + if (wallet.IsSpent(outpoint)) continue; @@ -348,10 +352,6 @@ CoinsResult AvailableCoins(const CWallet& wallet, assert(mine != ISMINE_NO); - int nDepth = wallet.GetTxStateDepthInMainChain(txo.GetState()); - if (nDepth < 0) - continue; - if (tx_safe_cache.count(outpoint.hash) == 0 ) { tx_safe_cache[outpoint.hash] = {false, false}; const CWalletTx& wtx = *wallet.GetWalletTx(outpoint.hash); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index b3209ed6362979..8e3c9066540a26 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1152,6 +1152,20 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const TxState& state, const // Break debit/credit balance caches: wtx.MarkDirty(); + // Remove or add back the inputs from m_txos to match the state of this tx. + if (wtx.isConfirmed()) + { + // When a transaction becomes confirmed, we can remove all of the txos that were spent + // in its inputs as they are no longer relevant. + for (const CTxIn& txin : wtx.tx->vin) { + MarkTXOUnusable(txin.prevout); + } + } else if (wtx.isInactive()) { + // When a transaction becomes inactive, we need to mark its inputs as usable again + for (const CTxIn& txin : wtx.tx->vin) { + MarkTXOUsable(txin.prevout); + } + } // Get the outputs that belong to the wallet RefreshWalletTxTXOs(wtx); @@ -1411,6 +1425,13 @@ void CWallet::RecursiveUpdateTxState(const uint256& tx_hash, const TryUpdatingSt todo.insert(iter->second); } } + if (wtx.state() || wtx.state()) { + // If the state applied is conflicted or confirmed, the outputs are unusable + MarkTXOUnusable(COutPoint(now, i)); + } else { + // Otherwise make the outputs usable + MarkTXOUsable(COutPoint(now, i)); + } } if (update_state == TxUpdate::NOTIFY_CHANGED) { @@ -1420,6 +1441,21 @@ void CWallet::RecursiveUpdateTxState(const uint256& tx_hash, const TryUpdatingSt // If a transaction changes its tx state, that usually changes the balance // available of the outputs it spends. So force those to be recomputed MarkInputsDirty(wtx.tx); + // Make the non-conflicted inputs usable again + for (unsigned int i = 0; i < wtx.tx->vin.size(); ++i) { + const CTxIn& txin = wtx.tx->vin.at(i); + auto unusable_txo_it = m_unusable_txos.find(txin.prevout); + if (unusable_txo_it == m_unusable_txos.end()) { + continue; + } + + if (std::get_if(&unusable_txo_it->second.GetState()) || + std::get_if(&unusable_txo_it->second.GetState())) { + continue; + } + + MarkTXOUsable(txin.prevout); + } } } } @@ -3253,6 +3289,10 @@ bool CWallet::AttachChain(const std::shared_ptr& walletInstance, interf } walletInstance->m_attaching_chain = false; + // Remove TXOs that have already been spent + // We do this here as we need to have an attached chain to figure out what has actually been spent. + walletInstance->PruneSpentTXOs(); + return true; } @@ -4325,6 +4365,7 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle return res; } +using TXOMap = std::unordered_map; void CWallet::RefreshWalletTxTXOs(const CWalletTx& wtx) { AssertLockHeld(cs_wallet); @@ -4332,19 +4373,26 @@ void CWallet::RefreshWalletTxTXOs(const CWalletTx& wtx) const CTxOut& txout = wtx.tx->vout.at(i); COutPoint outpoint(wtx.GetHash(), i); - auto it = m_txos.find(outpoint); - isminetype ismine = IsMine(txout); if (ismine == ISMINE_NO) { continue; } - if (it != m_txos.end()) { + auto it = wtx.m_txos.find(i); + if (it != wtx.m_txos.end()) { it->second.SetIsMine(ismine); it->second.SetState(wtx.GetState()); } else { - auto [txo_it, _] = m_txos.emplace(outpoint, WalletTXO{txout, ismine, wtx.GetState(), wtx.IsCoinBase(), wtx.m_from_me, wtx.GetTxTime()}); - wtx.m_txos.emplace(i, txo_it->second); + TXOMap::iterator txo_it; + bool txos_inserted = false; + if (m_last_block_processed_height >= 0 && IsSpent(outpoint, /*min_depth=*/1)) { + std::tie(txo_it, txos_inserted) = m_unusable_txos.emplace(outpoint, WalletTXO{txout, ismine, wtx.GetState(), wtx.IsCoinBase(), wtx.m_from_me, wtx.GetTxTime()}); + assert(txos_inserted); + } else { + std::tie(txo_it, txos_inserted) = m_txos.emplace(outpoint, WalletTXO{txout, ismine, wtx.GetState(), wtx.IsCoinBase(), wtx.m_from_me, wtx.GetTxTime()}); + } + auto [_, wtx_inserted] = wtx.m_txos.emplace(i, txo_it->second); + assert(wtx_inserted); } } } @@ -4361,9 +4409,58 @@ std::optional CWallet::GetTXO(const COutPoint& outpoint) const { AssertLockHeld(cs_wallet); const auto& it = m_txos.find(outpoint); - if (it == m_txos.end()) { - return std::nullopt; + if (it != m_txos.end()) { + return it->second; } - return it->second; + const auto& u_it = m_unusable_txos.find(outpoint); + if (u_it != m_unusable_txos.end()) { + return u_it->second; + } + return std::nullopt; +} + +void CWallet::PruneSpentTXOs() +{ + AssertLockHeld(cs_wallet); + auto it = m_txos.begin(); + while (it != m_txos.end()) { + if (std::get_if(&it->second.GetState()) || IsSpent(it->first, /*min_depth=*/1)) { + it = MarkTXOUnusable(it->first).first; + } else { + it++; + } + } +} + +std::pair CWallet::MarkTXOUnusable(const COutPoint& outpoint) +{ + AssertLockHeld(cs_wallet); + auto txos_it = m_txos.find(outpoint); + auto unusable_txos_it = m_unusable_txos.end(); + if (txos_it != m_txos.end()) { + auto next_txo_it = std::next(txos_it); + auto nh = m_txos.extract(txos_it); + txos_it = next_txo_it; + auto [position, inserted, _] = m_unusable_txos.insert(std::move(nh)); + unusable_txos_it = position; + assert(inserted); + } + return {txos_it, unusable_txos_it}; +} + +std::pair CWallet::MarkTXOUsable(const COutPoint& outpoint) +{ + AssertLockHeld(cs_wallet); + auto txos_it = m_txos.end(); + auto unusable_txos_it = m_unusable_txos.find(outpoint); + if (unusable_txos_it != m_unusable_txos.end()) { + auto next_unusable_txo_it = std::next(unusable_txos_it); + auto nh = m_unusable_txos.extract(unusable_txos_it); + unusable_txos_it = next_unusable_txo_it; + auto [position, inserted, _] = m_txos.insert(std::move(nh)); + assert(inserted); + txos_it = position; + } + return {unusable_txos_it, txos_it}; } } // namespace wallet diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 912229359357cb..b4f3e3a0a30852 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -420,7 +420,11 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati void AddScriptPubKeyMan(const uint256& id, std::unique_ptr spkm_man); //! Set of transaction outputs owned by this wallet - std::unordered_map m_txos GUARDED_BY(cs_wallet); + using TXOMap = std::unordered_map; + TXOMap m_txos GUARDED_BY(cs_wallet); + //! Set of transaction outputs that are definitely no longer usuable + //! These outputs may already be spent in a confirmed tx, or are the outputs of a conflicted tx + TXOMap m_unusable_txos GUARDED_BY(cs_wallet); /** * Catch wallet up to current chain, scanning new blocks, updating the best @@ -501,11 +505,14 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati std::set GetTxConflicts(const CWalletTx& wtx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); - const std::unordered_map& GetTXOs() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { AssertLockHeld(cs_wallet); return m_txos; }; + const TXOMap& GetTXOs() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) { AssertLockHeld(cs_wallet); return m_txos; }; std::optional GetTXO(const COutPoint& outpoint) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void RefreshWalletTxTXOs(const CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); void RefreshWalletTXOs() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + void PruneSpentTXOs() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + std::pair MarkTXOUnusable(const COutPoint& outpoint) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + std::pair MarkTXOUsable(const COutPoint& outpoint) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /** * Return depth of transaction in blockchain: diff --git a/test/functional/wallet_balance.py b/test/functional/wallet_balance.py index e0e183211a690b..742ed29c5fae96 100755 --- a/test/functional/wallet_balance.py +++ b/test/functional/wallet_balance.py @@ -14,6 +14,7 @@ assert_equal, assert_is_hash_string, assert_raises_rpc_error, + find_vout_for_address, ) from test_framework.wallet_util import get_generate_key @@ -400,5 +401,35 @@ def test_balances(*, fee_node_1=0): balances = wallet.getbalances() assert_equal(balances["mine"]["trusted"], amount * 2) + wallet.unloadwallet() + + self.log.info("Test that the balance is unchanged by an import that makes an already spent output in an existing tx \"mine\"") + self.nodes[0].createwallet("importalreadyspent") + wallet = self.nodes[0].get_wallet_rpc("importalreadyspent") + + import_change_key = get_generate_key() + addr1 = wallet.getnewaddress() + addr2 = wallet.getnewaddress() + + default.importprivkey(privkey=import_change_key.privkey, rescan=False) + + res = default.send(outputs=[{addr1: amount}], options={"change_address": import_change_key.p2wpkh_addr}) + inputs = [{"txid":res["txid"], "vout": find_vout_for_address(default, res["txid"], import_change_key.p2wpkh_addr)}] + default.send(outputs=[{addr2: amount}], options={"inputs": inputs, "add_inputs": True}) + + # Mock the time forward by another day so that "now" will exclude the block we just mined + self.nodes[0].setmocktime(int(time.time()) + 86400 * 2) + # Mine 11 blocks to move the MTP past the block we just mined + self.generate(self.nodes[0], 11, sync_fun=self.no_op) + + balances = wallet.getbalances() + assert_equal(balances["mine"]["trusted"], amount * 2) + + # Don't rescan to make sure that the import updates the wallet txos + # The balance should not change because the output for this key is already spent + wallet.importprivkey(privkey=import_change_key.privkey, rescan=False) + balances = wallet.getbalances() + assert_equal(balances["mine"]["trusted"], amount * 2) + if __name__ == '__main__': WalletTest().main()