From 9848611d10cfdd266be7ac4fcf1121147b8f6551 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 20 Feb 2024 11:49:51 -0500 Subject: [PATCH 01/12] walletdb: Load Txs last Need to load txs last so that IsMine works. --- src/wallet/walletdb.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index b09a0e40eb4..2a699b0811d 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -1197,14 +1197,14 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) // Load address book result = std::max(LoadAddressBookRecords(pwallet, *m_batch), result); - // Load tx records - result = std::max(LoadTxRecords(pwallet, *m_batch, upgraded_txs, any_unordered), result); - // Load SPKMs result = std::max(LoadActiveSPKMs(pwallet, *m_batch), result); // Load decryption keys result = std::max(LoadDecryptionKeys(pwallet, *m_batch), result); + + // Load tx records + result = std::max(LoadTxRecords(pwallet, *m_batch, upgraded_txs, any_unordered), result); } catch (...) { // Exceptions that can be ignored or treated as non-critical are handled by the individual loading functions. // Any uncaught exceptions will be caught here and treated as critical. From af8e6221c630eba038f4c245076a485f14191ffc Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 20 Feb 2024 11:49:53 -0500 Subject: [PATCH 02/12] wallet: Keep track of transaction outputs owned by the wallet When loading transactions to the wallet, check the outputs, and keep track of the ones that are IsMine. --- src/wallet/transaction.h | 22 ++++++++++++++++++++++ src/wallet/wallet.cpp | 26 ++++++++++++++++++++++++++ src/wallet/wallet.h | 6 ++++++ 3 files changed, 54 insertions(+) diff --git a/src/wallet/transaction.h b/src/wallet/transaction.h index 9079f6dd82a..ac9a8db8d9b 100644 --- a/src/wallet/transaction.h +++ b/src/wallet/transaction.h @@ -369,6 +369,28 @@ struct WalletTxOrderComparator { return a->nOrderPos < b->nOrderPos; } }; + +class WalletTXO +{ +private: + const CWalletTx& m_wtx; + const CTxOut& m_output; + isminetype m_ismine; + +public: + WalletTXO(const CWalletTx& wtx, const CTxOut& output, const isminetype ismine) + : m_wtx(wtx), + m_output(output), + m_ismine(ismine) + {} + + const CWalletTx& GetWalletTx() const { return m_wtx; } + + const CTxOut& GetTxOut() const { return m_output; } + + isminetype GetIsMine() const { return m_ismine; } + void SetIsMine(isminetype ismine) { m_ismine = ismine; } +}; } // namespace wallet #endif // BITCOIN_WALLET_TRANSACTION_H diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 652d6e9f6d8..86cd183d6d1 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1157,6 +1157,9 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const TxState& state, const // Break debit/credit balance caches: wtx.MarkDirty(); + // Cache the outputs that belong to the wallet + RefreshSingleTxTXOs(wtx); + // Notify UI of new or updated transaction NotifyTransactionChanged(hash, fInsertedNew ? CT_NEW : CT_UPDATED); @@ -1220,6 +1223,8 @@ bool CWallet::LoadToWallet(const uint256& hash, const UpdateWalletTxFn& fill_wtx // Update birth time when tx time is older than it. MaybeUpdateBirthTime(wtx.GetTxTime()); + // Make sure the tx outputs are known by the wallet + RefreshSingleTxTXOs(wtx); return true; } @@ -2427,6 +2432,9 @@ util::Result CWallet::RemoveTxs(WalletBatch& batch, std::vector& wtxOrdered.erase(it->second.m_it_wtxOrdered); for (const auto& txin : it->second.tx->vin) mapTxSpends.erase(txin.prevout); + for (unsigned int i = 0; i < it->second.tx->vout.size(); ++i) { + m_txos.erase(COutPoint(Txid::FromUint256(hash), i)); + } mapWallet.erase(it); NotifyTransactionChanged(hash, CT_DELETED); } @@ -4621,4 +4629,22 @@ std::optional CWallet::GetKey(const CKeyID& keyid) const } return std::nullopt; } + +void CWallet::RefreshSingleTxTXOs(const CWalletTx& wtx) +{ + AssertLockHeld(cs_wallet); + for (uint32_t i = 0; i < wtx.tx->vout.size(); ++i) { + const CTxOut& txout = wtx.tx->vout.at(i); + isminetype ismine = IsMine(txout); + if (ismine == ISMINE_NO) { + continue; + } + COutPoint outpoint(wtx.GetHash(), i); + if (m_txos.contains(outpoint)) { + m_txos.at(outpoint).SetIsMine(ismine); + } else { + m_txos.emplace(outpoint, WalletTXO{wtx, txout, ismine}); + } + } +} } // namespace wallet diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index d869f031bbf..ea7cc8b992a 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -428,6 +428,9 @@ private: //! Cache of descriptor ScriptPubKeys used for IsMine. Maps ScriptPubKey to set of spkms std::unordered_map, SaltedSipHasher> m_cached_spks; + //! Set of both spent and unspent transaction outputs owned by this wallet + std::unordered_map m_txos GUARDED_BY(cs_wallet); + /** * Catch wallet up to current chain, scanning new blocks, updating the best * block locator and m_last_block_processed, and registering for @@ -507,6 +510,9 @@ public: std::set GetTxConflicts(const CWalletTx& wtx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + /** Cache outputs that belong to the wallet from a single transaction */ + void RefreshSingleTxTXOs(const CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + /** * Return depth of transaction in blockchain: * <0 : conflicts with a transaction this deep in the blockchain From 326b7c59e767edb313292a0ba60065687f869d60 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 20 Feb 2024 11:49:56 -0500 Subject: [PATCH 03/12] wallet: Exit IsTrustedTx early if wtx is already in trusted_parents --- src/wallet/receive.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/wallet/receive.cpp b/src/wallet/receive.cpp index c164266f80b..4ff2451301f 100644 --- a/src/wallet/receive.cpp +++ b/src/wallet/receive.cpp @@ -257,6 +257,10 @@ bool CachedTxIsFromMe(const CWallet& wallet, const CWalletTx& wtx, const isminef bool CachedTxIsTrusted(const CWallet& wallet, const CWalletTx& wtx, std::set& trusted_parents) { AssertLockHeld(wallet.cs_wallet); + + // This wtx is already trusted + if (trusted_parents.contains(wtx.GetHash())) return true; + if (wtx.isConfirmed()) return true; if (wtx.isBlockConflicted()) return false; // using wtx's cached debit From c088e9b69914aad6f47d6bb7f6c137939a9979fe Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 20 Feb 2024 11:49:58 -0500 Subject: [PATCH 04/12] wallet: Change balance calculation to use m_txos Since we track the outputs owned by the wallet with m_txos, we can now calculate the balance of the wallet by iterating m_txos and summing up the amounts of the unspent txos. As ISMINE_USED is not an actual isminetype that we attach to outputs and was just passed into `CachedTxGetAvailableCredit` for convenience, we pull the same determining logic from that function into `GetBalances` in order to preserve existing behavior. --- src/wallet/receive.cpp | 83 +++++++++++++++++++++--------------------- src/wallet/wallet.h | 2 + 2 files changed, 44 insertions(+), 41 deletions(-) diff --git a/src/wallet/receive.cpp b/src/wallet/receive.cpp index 4ff2451301f..5ff3b9110a2 100644 --- a/src/wallet/receive.cpp +++ b/src/wallet/receive.cpp @@ -4,6 +4,7 @@ #include #include +#include #include #include #include @@ -297,27 +298,40 @@ bool CachedTxIsTrusted(const CWallet& wallet, const CWalletTx& wtx) Balance GetBalance(const CWallet& wallet, const int min_depth, bool avoid_reuse) { Balance ret; - isminefilter reuse_filter = avoid_reuse ? ISMINE_NO : ISMINE_USED; + bool allow_used_addresses = !avoid_reuse || !wallet.IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE); { LOCK(wallet.cs_wallet); - std::set trusted_parents; - for (const auto& entry : wallet.mapWallet) - { - const CWalletTx& wtx = entry.second; - const bool is_trusted{CachedTxIsTrusted(wallet, wtx, trusted_parents)}; - const int tx_depth{wallet.GetTxDepthInMainChain(wtx)}; - const CAmount tx_credit_mine{CachedTxGetAvailableCredit(wallet, wtx, ISMINE_SPENDABLE | reuse_filter)}; - const CAmount tx_credit_watchonly{CachedTxGetAvailableCredit(wallet, wtx, ISMINE_WATCH_ONLY | reuse_filter)}; - if (is_trusted && tx_depth >= min_depth) { - ret.m_mine_trusted += tx_credit_mine; - ret.m_watchonly_trusted += tx_credit_watchonly; + for (const auto& [outpoint, txo] : wallet.GetTXOs()) { + Assert(MoneyRange(txo.GetTxOut().nValue)); + + const bool is_trusted{CachedTxIsTrusted(wallet, txo.GetWalletTx())}; + const int tx_depth{wallet.GetTxDepthInMainChain(txo.GetWalletTx())}; + + if (!wallet.IsSpent(outpoint) && (allow_used_addresses || !wallet.IsSpentKey(txo.GetTxOut().scriptPubKey))) { + // Get the amounts for mine and watchonly + CAmount credit_mine = 0; + CAmount credit_watchonly = 0; + if (txo.GetIsMine() == ISMINE_SPENDABLE) { + credit_mine = txo.GetTxOut().nValue; + } else if (txo.GetIsMine() == ISMINE_WATCH_ONLY) { + credit_watchonly = txo.GetTxOut().nValue; + } else { + // We shouldn't see any other isminetypes + Assume(false); + } + + // Set the amounts in the return object + if (wallet.IsTxImmatureCoinBase(txo.GetWalletTx()) && txo.GetWalletTx().isConfirmed()) { + ret.m_mine_immature += credit_mine; + ret.m_watchonly_immature += credit_watchonly; + } else if (is_trusted && tx_depth >= min_depth) { + ret.m_mine_trusted += credit_mine; + ret.m_watchonly_trusted += credit_watchonly; + } else if (!is_trusted && txo.GetWalletTx().InMempool()) { + ret.m_mine_untrusted_pending += credit_mine; + ret.m_watchonly_untrusted_pending += credit_watchonly; + } } - if (!is_trusted && tx_depth == 0 && wtx.InMempool()) { - ret.m_mine_untrusted_pending += tx_credit_mine; - ret.m_watchonly_untrusted_pending += tx_credit_watchonly; - } - ret.m_mine_immature += CachedTxGetImmatureCredit(wallet, wtx, ISMINE_SPENDABLE); - ret.m_watchonly_immature += CachedTxGetImmatureCredit(wallet, wtx, ISMINE_WATCH_ONLY); } } return ret; @@ -329,32 +343,19 @@ std::map GetAddressBalances(const CWallet& wallet) { LOCK(wallet.cs_wallet); - std::set trusted_parents; - for (const auto& walletEntry : wallet.mapWallet) - { - const CWalletTx& wtx = walletEntry.second; + for (const auto& [outpoint, txo] : wallet.GetTXOs()) { + if (!CachedTxIsTrusted(wallet, txo.GetWalletTx())) continue; + if (wallet.IsTxImmatureCoinBase(txo.GetWalletTx())) continue; - if (!CachedTxIsTrusted(wallet, wtx, trusted_parents)) - continue; + int nDepth = wallet.GetTxDepthInMainChain(txo.GetWalletTx()); + if (nDepth < (CachedTxIsFromMe(wallet, txo.GetWalletTx(), ISMINE_ALL) ? 0 : 1)) continue; - if (wallet.IsTxImmatureCoinBase(wtx)) - continue; + CTxDestination addr; + Assume(wallet.IsMine(txo.GetTxOut())); + if(!ExtractDestination(txo.GetTxOut().scriptPubKey, addr)) continue; - int nDepth = wallet.GetTxDepthInMainChain(wtx); - if (nDepth < (CachedTxIsFromMe(wallet, wtx, ISMINE_ALL) ? 0 : 1)) - continue; - - for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) { - const auto& output = wtx.tx->vout[i]; - CTxDestination addr; - if (!wallet.IsMine(output)) - continue; - if(!ExtractDestination(output.scriptPubKey, addr)) - continue; - - CAmount n = wallet.IsSpent(COutPoint(Txid::FromUint256(walletEntry.first), i)) ? 0 : output.nValue; - balances[addr] += n; - } + CAmount n = wallet.IsSpent(outpoint) ? 0 : txo.GetTxOut().nValue; + balances[addr] += n; } } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index ea7cc8b992a..9a574b38218 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -510,6 +510,8 @@ public: 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; }; + /** Cache outputs that belong to the wallet from a single transaction */ void RefreshSingleTxTXOs(const CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); From 1b2dc9cec28555dbd6b83bd1ec74375baa7b20fd Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 20 Feb 2024 11:50:00 -0500 Subject: [PATCH 05/12] wallet: Recalculate the wallet's txos after any imports --- src/wallet/rpc/addresses.cpp | 3 +++ src/wallet/rpc/backup.cpp | 8 ++++++++ src/wallet/rpc/wallet.cpp | 1 + src/wallet/wallet.cpp | 8 ++++++++ src/wallet/wallet.h | 2 ++ 5 files changed, 22 insertions(+) diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp index 1c2951deeec..a06f1c20bf2 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/src/wallet/rpc/addresses.cpp @@ -312,6 +312,7 @@ RPCHelpMan addmultisigaddress() // Store destination in the addressbook pwallet->SetAddressBook(dest, label, AddressPurpose::SEND); + pwallet->RefreshAllTXOs(); // Make the descriptor std::unique_ptr descriptor = InferDescriptor(GetScriptForDestination(dest), spk_man); @@ -371,6 +372,7 @@ RPCHelpMan keypoolrefill() if (pwallet->GetKeyPoolSize() < kpSize) { throw JSONRPCError(RPC_WALLET_ERROR, "Error refreshing keypool."); } + pwallet->RefreshAllTXOs(); return UniValue::VNULL; }, @@ -402,6 +404,7 @@ RPCHelpMan newkeypool() LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*pwallet, true); spk_man.NewKeyPool(); + pwallet->RefreshAllTXOs(); return UniValue::VNULL; }, diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index 4ffc6f1e0dd..97497fbcc8e 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -206,6 +206,7 @@ RPCHelpMan importprivkey() pwallet->ImportScripts({GetScriptForDestination(WitnessV0KeyHash(vchAddress))}, /*timestamp=*/0); } } + pwallet->RefreshAllTXOs(); } if (fRescan) { RescanWallet(*pwallet, reserver); @@ -306,6 +307,7 @@ RPCHelpMan importaddress() } else { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid Bitcoin address or script"); } + pwallet->RefreshAllTXOs(); } if (fRescan) { @@ -472,6 +474,8 @@ RPCHelpMan importpubkey() pwallet->ImportScriptPubKeys(strLabel, script_pub_keys, /*have_solving_data=*/true, /*apply_label=*/true, /*timestamp=*/1); pwallet->ImportPubKeys({{pubKey.GetID(), false}}, {{pubKey.GetID(), pubKey}} , /*key_origins=*/{}, /*add_keypool=*/false, /*timestamp=*/1); + + pwallet->RefreshAllTXOs(); } if (fRescan) { @@ -619,6 +623,7 @@ RPCHelpMan importwallet() progress++; } + pwallet->RefreshAllTXOs(); pwallet->chain().showProgress("", 100, false); // hide progress dialog in GUI } pwallet->chain().showProgress("", 100, false); // hide progress dialog in GUI @@ -1405,6 +1410,8 @@ RPCHelpMan importmulti() nLowestTimestamp = timestamp; } } + + pwallet->RefreshAllTXOs(); } if (fRescan && fRunScan && requests.size()) { int64_t scannedTime = pwallet->RescanFromTime(nLowestTimestamp, reserver, /*update=*/true); @@ -1718,6 +1725,7 @@ RPCHelpMan importdescriptors() } } pwallet->ConnectScriptPubKeyManNotifiers(); + pwallet->RefreshAllTXOs(); } // Rescan the blockchain using the lowest timestamp diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index 5140ac8c059..e1e70875e36 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -570,6 +570,7 @@ static RPCHelpMan sethdseed() spk_man.SetHDSeed(master_pub_key); if (flush_key_pool) spk_man.NewKeyPool(); + pwallet->RefreshAllTXOs(); return UniValue::VNULL; }, diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 86cd183d6d1..007547c208a 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4647,4 +4647,12 @@ void CWallet::RefreshSingleTxTXOs(const CWalletTx& wtx) } } } + +void CWallet::RefreshAllTXOs() +{ + AssertLockHeld(cs_wallet); + for (const auto& [_, wtx] : mapWallet) { + RefreshSingleTxTXOs(wtx); + } +} } // namespace wallet diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 9a574b38218..f372106aeda 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -514,6 +514,8 @@ public: /** Cache outputs that belong to the wallet from a single transaction */ void RefreshSingleTxTXOs(const CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + /** Cache outputs that belong to the wallt for all tranasctions in the wallet */ + void RefreshAllTXOs() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /** * Return depth of transaction in blockchain: From 016f08cf674dc152b72232828b1874fe8c4490b8 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 20 Feb 2024 11:50:02 -0500 Subject: [PATCH 06/12] test: Test for balance update due to watchonly becoming spendable --- test/functional/wallet_balance.py | 60 +++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/test/functional/wallet_balance.py b/test/functional/wallet_balance.py index 9da53402a45..8069b0c6bed 100755 --- a/test/functional/wallet_balance.py +++ b/test/functional/wallet_balance.py @@ -4,6 +4,7 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test the wallet balance RPC methods.""" from decimal import Decimal +import time from test_framework.address import ADDRESS_BCRT1_UNSPENDABLE as ADDRESS_WATCHONLY from test_framework.blocktools import COINBASE_MATURITY @@ -13,6 +14,7 @@ from test_framework.util import ( assert_is_hash_string, assert_raises_rpc_error, ) +from test_framework.wallet_util import get_generate_key def create_transactions(node, address, amt, fees): @@ -311,7 +313,40 @@ class WalletTest(BitcoinTestFramework): self.nodes[0].createwallet('w2', False, True) self.nodes[0].importprivkey(privkey) assert_equal(self.nodes[0].getbalances()['mine']['untrusted_pending'], Decimal('0.1')) + self.nodes[0].unloadwallet("w2") + self.log.info("Test that an import that makes something spendable updates \"mine\" balance") + self.nodes[0].loadwallet(self.default_wallet_name) + default = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + self.nodes[0].createwallet(wallet_name="legacyspendableupdate", descriptors=False) + wallet = self.nodes[0].get_wallet_rpc("legacyspendableupdate") + + import_key1 = get_generate_key() + import_key2 = get_generate_key() + wallet.importaddress(import_key1.p2wpkh_addr) + wallet.importaddress(import_key2.p2wpkh_addr) + + amount = Decimal(15) + default.sendtoaddress(import_key1.p2wpkh_addr, amount) + default.sendtoaddress(import_key2.p2wpkh_addr, amount) + self.generate(self.nodes[0], 1) + + balances = wallet.getbalances() + assert_equal(balances["mine"]["trusted"], 0) + assert_equal(balances["watchonly"]["trusted"], amount * 2) + + # Rescanning should always update the txos by virtue of finding them again + wallet.importprivkey(privkey=import_key1.privkey, rescan=True) + balances = wallet.getbalances() + assert_equal(balances["mine"]["trusted"], amount) + assert_equal(balances["watchonly"]["trusted"], amount) + + # Don't rescan to make sure that the import updates the wallet txos + wallet.importprivkey(privkey=import_key2.privkey, rescan=False) + balances = wallet.getbalances() + assert_equal(balances["mine"]["trusted"], amount * 2) + assert_equal(balances["watchonly"]["trusted"], 0) + self.nodes[0].unloadwallet("legacyspendableupdate") # Tests the lastprocessedblock JSON object in getbalances, getwalletinfo # and gettransaction by checking for valid hex strings and by comparing @@ -338,5 +373,30 @@ class WalletTest(BitcoinTestFramework): assert_equal(tx_info['lastprocessedblock']['height'], prev_height) assert_equal(tx_info['lastprocessedblock']['hash'], prev_hash) + self.log.info("Test that the balance is updated by an import that makes an untracked output in an existing tx \"mine\"") + default = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + self.nodes[0].createwallet("importupdate") + wallet = self.nodes[0].get_wallet_rpc("importupdate") + + import_key1 = get_generate_key() + import_key2 = get_generate_key() + wallet.importprivkey(import_key1.privkey) + + amount = 15 + default.send([{import_key1.p2wpkh_addr: amount},{import_key2.p2wpkh_addr: amount}]) + self.generate(self.nodes[0], 1) + # Mock the time forward by 1 day so that "now" will exclude the block we just mined + self.nodes[0].setmocktime(int(time.time()) + 86400) + # 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) + + # Don't rescan to make sure that the import updates the wallet txos + wallet.importprivkey(privkey=import_key2.privkey, rescan=False) + balances = wallet.getbalances() + assert_equal(balances["mine"]["trusted"], amount * 2) + if __name__ == '__main__': WalletTest(__file__).main() From e2e0fcd5db9fd85491030f6d737c4dfb59f7b771 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 20 Feb 2024 11:50:07 -0500 Subject: [PATCH 07/12] wallet: Use wallet's TXO set in AvailableCoins Instead of iterating every transaction and every output stored in wallet when trying to figure out what outputs can be spent, iterate the TXO set which should be a lot smaller. --- src/wallet/spend.cpp | 219 ++++++++++++++++++++++--------------------- 1 file changed, 114 insertions(+), 105 deletions(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index aceed24a86c..3ddd67e2e53 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -324,137 +324,146 @@ CoinsResult AvailableCoins(const CWallet& wallet, std::vector outpoints; std::set trusted_parents; - for (const auto& entry : wallet.mapWallet) - { - const uint256& txid = entry.first; - const CWalletTx& wtx = entry.second; + // Cache for whether each tx passes the tx level checks (first bool), and whether the transaction is "safe" (second bool) + std::unordered_map, SaltedTxidHasher> tx_safe_cache; + for (const auto& [outpoint, txo] : wallet.GetTXOs()) { + const CWalletTx& wtx = txo.GetWalletTx(); + const CTxOut& output = txo.GetTxOut(); + + if (tx_safe_cache.contains(outpoint.hash) && !tx_safe_cache.at(outpoint.hash).first) { + continue; + } + + // Skip manually selected coins (the caller can fetch them directly) + if (coinControl && coinControl->HasSelected() && coinControl->IsSelected(outpoint)) + continue; + + if (wallet.IsLockedCoin(outpoint) && params.skip_locked) + continue; + + if (wallet.IsSpent(outpoint)) + continue; + + if (output.nValue < params.min_amount || output.nValue > params.max_amount) + continue; + + if (!allow_used_addresses && wallet.IsSpentKey(output.scriptPubKey)) { + continue; + } if (wallet.IsTxImmatureCoinBase(wtx) && !params.include_immature_coinbase) continue; + isminetype mine = wallet.IsMine(output); + + assert(mine != ISMINE_NO); + int nDepth = wallet.GetTxDepthInMainChain(wtx); if (nDepth < 0) continue; - // We should not consider coins which aren't at least in our mempool - // It's possible for these to be conflicted via ancestors which we may never be able to detect - if (nDepth == 0 && !wtx.InMempool()) - continue; + // Perform tx level checks if we haven't already come across outputs from this tx before. + if (!tx_safe_cache.contains(outpoint.hash)) { + tx_safe_cache[outpoint.hash] = {false, false}; - bool safeTx = CachedTxIsTrusted(wallet, wtx, trusted_parents); + // We should not consider coins which aren't at least in our mempool + // It's possible for these to be conflicted via ancestors which we may never be able to detect + if (nDepth == 0 && !wtx.InMempool()) + continue; - // We should not consider coins from transactions that are replacing - // other transactions. - // - // Example: There is a transaction A which is replaced by bumpfee - // transaction B. In this case, we want to prevent creation of - // a transaction B' which spends an output of B. - // - // Reason: If transaction A were initially confirmed, transactions B - // and B' would no longer be valid, so the user would have to create - // a new transaction C to replace B'. However, in the case of a - // one-block reorg, transactions B' and C might BOTH be accepted, - // when the user only wanted one of them. Specifically, there could - // be a 1-block reorg away from the chain where transactions A and C - // were accepted to another chain where B, B', and C were all - // accepted. - if (nDepth == 0 && wtx.mapValue.count("replaces_txid")) { - safeTx = false; + bool safeTx = CachedTxIsTrusted(wallet, wtx, trusted_parents); + + // We should not consider coins from transactions that are replacing + // other transactions. + // + // Example: There is a transaction A which is replaced by bumpfee + // transaction B. In this case, we want to prevent creation of + // a transaction B' which spends an output of B. + // + // Reason: If transaction A were initially confirmed, transactions B + // and B' would no longer be valid, so the user would have to create + // a new transaction C to replace B'. However, in the case of a + // one-block reorg, transactions B' and C might BOTH be accepted, + // when the user only wanted one of them. Specifically, there could + // be a 1-block reorg away from the chain where transactions A and C + // were accepted to another chain where B, B', and C were all + // accepted. + if (nDepth == 0 && wtx.mapValue.count("replaces_txid")) { + safeTx = false; + } + + // Similarly, we should not consider coins from transactions that + // have been replaced. In the example above, we would want to prevent + // creation of a transaction A' spending an output of A, because if + // transaction B were initially confirmed, conflicting with A and + // A', we wouldn't want to the user to create a transaction D + // intending to replace A', but potentially resulting in a scenario + // where A, A', and D could all be accepted (instead of just B and + // D, or just A and A' like the user would want). + if (nDepth == 0 && wtx.mapValue.count("replaced_by_txid")) { + safeTx = false; + } + + if (only_safe && !safeTx) { + continue; + } + + if (nDepth < min_depth || nDepth > max_depth) { + continue; + } + + tx_safe_cache[outpoint.hash] = {true, safeTx}; } - - // Similarly, we should not consider coins from transactions that - // have been replaced. In the example above, we would want to prevent - // creation of a transaction A' spending an output of A, because if - // transaction B were initially confirmed, conflicting with A and - // A', we wouldn't want to the user to create a transaction D - // intending to replace A', but potentially resulting in a scenario - // where A, A', and D could all be accepted (instead of just B and - // D, or just A and A' like the user would want). - if (nDepth == 0 && wtx.mapValue.count("replaced_by_txid")) { - safeTx = false; - } - - if (only_safe && !safeTx) { - continue; - } - - if (nDepth < min_depth || nDepth > max_depth) { + const auto& [tx_ok, tx_safe] = tx_safe_cache.at(outpoint.hash); + if (!Assume(tx_ok)) { continue; } bool tx_from_me = CachedTxIsFromMe(wallet, wtx, ISMINE_ALL); - for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) { - const CTxOut& output = wtx.tx->vout[i]; - const COutPoint outpoint(Txid::FromUint256(txid), i); + std::unique_ptr provider = wallet.GetSolvingProvider(output.scriptPubKey); - if (output.nValue < params.min_amount || output.nValue > params.max_amount) - continue; + int input_bytes = CalculateMaximumSignedInputSize(output, COutPoint(), provider.get(), can_grind_r, coinControl); + // Because CalculateMaximumSignedInputSize infers a solvable descriptor to get the satisfaction size, + // it is safe to assume that this input is solvable if input_bytes is greater than -1. + bool solvable = input_bytes > -1; + bool spendable = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (((mine & ISMINE_WATCH_ONLY) != ISMINE_NO) && (coinControl && coinControl->fAllowWatchOnly && solvable)); - // Skip manually selected coins (the caller can fetch them directly) - if (coinControl && coinControl->HasSelected() && coinControl->IsSelected(outpoint)) - continue; + // Filter by spendable outputs only + if (!spendable && params.only_spendable) continue; - if (wallet.IsLockedCoin(outpoint) && params.skip_locked) - continue; + // Obtain script type + std::vector> script_solutions; + TxoutType type = Solver(output.scriptPubKey, script_solutions); - if (wallet.IsSpent(outpoint)) - continue; + // If the output is P2SH and solvable, we want to know if it is + // a P2SH (legacy) or one of P2SH-P2WPKH, P2SH-P2WSH (P2SH-Segwit). We can determine + // this from the redeemScript. If the output is not solvable, it will be classified + // as a P2SH (legacy), since we have no way of knowing otherwise without the redeemScript + bool is_from_p2sh{false}; + if (type == TxoutType::SCRIPTHASH && solvable) { + CScript script; + if (!provider->GetCScript(CScriptID(uint160(script_solutions[0])), script)) continue; + type = Solver(script, script_solutions); + is_from_p2sh = true; + } - isminetype mine = wallet.IsMine(output); + result.Add(GetOutputType(type, is_from_p2sh), + COutput(outpoint, output, nDepth, input_bytes, spendable, solvable, tx_safe, wtx.GetTxTime(), tx_from_me, feerate)); - if (mine == ISMINE_NO) { - continue; - } + outpoints.push_back(outpoint); - if (!allow_used_addresses && wallet.IsSpentKey(output.scriptPubKey)) { - continue; - } - - std::unique_ptr provider = wallet.GetSolvingProvider(output.scriptPubKey); - - int input_bytes = CalculateMaximumSignedInputSize(output, COutPoint(), provider.get(), can_grind_r, coinControl); - // Because CalculateMaximumSignedInputSize infers a solvable descriptor to get the satisfaction size, - // it is safe to assume that this input is solvable if input_bytes is greater than -1. - bool solvable = input_bytes > -1; - bool spendable = ((mine & ISMINE_SPENDABLE) != ISMINE_NO) || (((mine & ISMINE_WATCH_ONLY) != ISMINE_NO) && (coinControl && coinControl->fAllowWatchOnly && solvable)); - - // Filter by spendable outputs only - if (!spendable && params.only_spendable) continue; - - // Obtain script type - std::vector> script_solutions; - TxoutType type = Solver(output.scriptPubKey, script_solutions); - - // If the output is P2SH and solvable, we want to know if it is - // a P2SH (legacy) or one of P2SH-P2WPKH, P2SH-P2WSH (P2SH-Segwit). We can determine - // this from the redeemScript. If the output is not solvable, it will be classified - // as a P2SH (legacy), since we have no way of knowing otherwise without the redeemScript - bool is_from_p2sh{false}; - if (type == TxoutType::SCRIPTHASH && solvable) { - CScript script; - if (!provider->GetCScript(CScriptID(uint160(script_solutions[0])), script)) continue; - type = Solver(script, script_solutions); - is_from_p2sh = true; - } - - result.Add(GetOutputType(type, is_from_p2sh), - COutput(outpoint, output, nDepth, input_bytes, spendable, solvable, safeTx, wtx.GetTxTime(), tx_from_me, feerate)); - - outpoints.push_back(outpoint); - - // Checks the sum amount of all UTXO's. - if (params.min_sum_amount != MAX_MONEY) { - if (result.GetTotalAmount() >= params.min_sum_amount) { - return result; - } - } - - // Checks the maximum number of UTXO's. - if (params.max_count > 0 && result.Size() >= params.max_count) { + // Checks the sum amount of all UTXO's. + if (params.min_sum_amount != MAX_MONEY) { + if (result.GetTotalAmount() >= params.min_sum_amount) { return result; } } + + // Checks the maximum number of UTXO's. + if (params.max_count > 0 && result.Size() >= params.max_count) { + return result; + } } if (feerate.has_value()) { From 961d0f4845114bea6ffa3ce2958273c3ee1f0869 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 20 Feb 2024 11:50:09 -0500 Subject: [PATCH 08/12] wallet: Retrieve TXO directly in FetchSelectedInputs Instead of searching mapWallet for the preselected inputs, search m_txos. wallet_fundrawtransaction.py spends external inputs and needs the change output to also belong to the test wallet for the oversized tx test. --- src/wallet/spend.cpp | 8 ++------ src/wallet/wallet.cpp | 10 ++++++++++ src/wallet/wallet.h | 1 + 3 files changed, 13 insertions(+), 6 deletions(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 3ddd67e2e53..3b99678b468 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -271,12 +271,8 @@ util::Result FetchSelectedInputs(const CWallet& wallet, const input_bytes = GetVirtualTransactionSize(input_bytes, 0, 0); } CTxOut txout; - if (auto ptr_wtx = wallet.GetWalletTx(outpoint.hash)) { - // Clearly invalid input, fail - if (ptr_wtx->tx->vout.size() <= outpoint.n) { - return util::Error{strprintf(_("Invalid pre-selected input %s"), outpoint.ToString())}; - } - txout = ptr_wtx->tx->vout.at(outpoint.n); + if (auto txo = wallet.GetTXO(outpoint)) { + txout = txo->GetTxOut(); if (input_bytes == -1) { input_bytes = CalculateMaximumSignedInputSize(txout, &wallet, &coin_control); } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 007547c208a..87dc02c105f 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4655,4 +4655,14 @@ void CWallet::RefreshAllTXOs() RefreshSingleTxTXOs(wtx); } } + +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; + } + return it->second; +} } // namespace wallet diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index f372106aeda..247253af1ae 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -511,6 +511,7 @@ public: 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; }; + std::optional GetTXO(const COutPoint& outpoint) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); /** Cache outputs that belong to the wallet from a single transaction */ void RefreshSingleTxTXOs(const CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); From 79a35663f0bbf0e46198ff7c41e15ff22974da02 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 20 Feb 2024 11:50:11 -0500 Subject: [PATCH 09/12] wallet: Recompute wallet TXOs after descriptor migration When a legacy wallet has been migrated to contain descriptors, but before the transactions have been updated to match, we need to recompute the wallet TXOs so that the transaction update will work correctly. --- src/wallet/wallet.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 87dc02c105f..3c07a4197fc 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4131,6 +4131,10 @@ util::Result CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch, return util::Error{_("Error: Unable to read wallet's best block locator record")}; } + // Update m_txos to match the descriptors remaining in this wallet + m_txos.clear(); + RefreshAllTXOs(); + // Check if the transactions in the wallet are still ours. Either they belong here, or they belong in the watchonly wallet. // We need to go through these in the tx insertion order so that lookups to spends works. std::vector txids_to_delete; From 933a550af5ae014c0e9f538f79841c08d6638074 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 20 Feb 2024 11:50:13 -0500 Subject: [PATCH 10/12] wallet: Have GetDebit use the wallet's TXO set Instead of looking up the previous tx in mapWallet, and then calling IsMine on the specified output, use the TXO set and its cached IsMine value. --- src/wallet/wallet.cpp | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 3c07a4197fc..06771c68442 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1593,16 +1593,10 @@ void CWallet::BlockUntilSyncedToCurrentChain() const { // and a not-"is mine" (according to the filter) input. CAmount CWallet::GetDebit(const CTxIn &txin, const isminefilter& filter) const { - { - LOCK(cs_wallet); - const auto mi = mapWallet.find(txin.prevout.hash); - if (mi != mapWallet.end()) - { - const CWalletTx& prev = (*mi).second; - if (txin.prevout.n < prev.tx->vout.size()) - if (IsMine(prev.tx->vout[txin.prevout.n]) & filter) - return prev.tx->vout[txin.prevout.n].nValue; - } + LOCK(cs_wallet); + auto txo = GetTXO(txin.prevout); + if (txo && (txo->GetIsMine() & filter)) { + return txo->GetTxOut().nValue; } return 0; } From 36743a5ee13f689156c77b35245f5690a2f741da Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 20 Feb 2024 11:50:15 -0500 Subject: [PATCH 11/12] wallet: Remove unused CachedTxGet{Available,Immature}Credit These two functions are no longer used as GetBalances now uses the TXO set rather than per-tx cached balances --- src/wallet/receive.cpp | 46 ----------------- src/wallet/receive.h | 4 -- src/wallet/test/wallet_tests.cpp | 89 -------------------------------- src/wallet/transaction.h | 4 +- 4 files changed, 1 insertion(+), 142 deletions(-) diff --git a/src/wallet/receive.cpp b/src/wallet/receive.cpp index 5ff3b9110a2..6de5e7d0daa 100644 --- a/src/wallet/receive.cpp +++ b/src/wallet/receive.cpp @@ -146,52 +146,6 @@ CAmount CachedTxGetChange(const CWallet& wallet, const CWalletTx& wtx) return wtx.nChangeCached; } -CAmount CachedTxGetImmatureCredit(const CWallet& wallet, const CWalletTx& wtx, const isminefilter& filter) -{ - AssertLockHeld(wallet.cs_wallet); - - if (wallet.IsTxImmatureCoinBase(wtx) && wtx.isConfirmed()) { - return GetCachableAmount(wallet, wtx, CWalletTx::IMMATURE_CREDIT, filter); - } - - return 0; -} - -CAmount CachedTxGetAvailableCredit(const CWallet& wallet, const CWalletTx& wtx, const isminefilter& filter) -{ - AssertLockHeld(wallet.cs_wallet); - - // Avoid caching ismine for NO or ALL cases (could remove this check and simplify in the future). - bool allow_cache = (filter & ISMINE_ALL) && (filter & ISMINE_ALL) != ISMINE_ALL; - - // Must wait until coinbase is safely deep enough in the chain before valuing it - if (wallet.IsTxImmatureCoinBase(wtx)) - return 0; - - if (allow_cache && wtx.m_amounts[CWalletTx::AVAILABLE_CREDIT].m_cached[filter]) { - return wtx.m_amounts[CWalletTx::AVAILABLE_CREDIT].m_value[filter]; - } - - bool allow_used_addresses = (filter & ISMINE_USED) || !wallet.IsWalletFlagSet(WALLET_FLAG_AVOID_REUSE); - CAmount nCredit = 0; - Txid hashTx = wtx.GetHash(); - for (unsigned int i = 0; i < wtx.tx->vout.size(); i++) { - const CTxOut& txout = wtx.tx->vout[i]; - if (!wallet.IsSpent(COutPoint(hashTx, i)) && (allow_used_addresses || !wallet.IsSpentKey(txout.scriptPubKey))) { - nCredit += OutputGetCredit(wallet, txout, filter); - if (!MoneyRange(nCredit)) - throw std::runtime_error(std::string(__func__) + " : value out of range"); - } - } - - if (allow_cache) { - wtx.m_amounts[CWalletTx::AVAILABLE_CREDIT].Set(filter, nCredit); - wtx.m_is_cache_empty = false; - } - - return nCredit; -} - void CachedTxGetAmounts(const CWallet& wallet, const CWalletTx& wtx, std::list& listReceived, std::list& listSent, CAmount& nFee, const isminefilter& filter, diff --git a/src/wallet/receive.h b/src/wallet/receive.h index d50644b4cf9..6cccd323e0e 100644 --- a/src/wallet/receive.h +++ b/src/wallet/receive.h @@ -29,10 +29,6 @@ CAmount CachedTxGetCredit(const CWallet& wallet, const CWalletTx& wtx, const ism //! filter decides which addresses will count towards the debit CAmount CachedTxGetDebit(const CWallet& wallet, const CWalletTx& wtx, const isminefilter& filter); CAmount CachedTxGetChange(const CWallet& wallet, const CWalletTx& wtx); -CAmount CachedTxGetImmatureCredit(const CWallet& wallet, const CWalletTx& wtx, const isminefilter& filter) - EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); -CAmount CachedTxGetAvailableCredit(const CWallet& wallet, const CWalletTx& wtx, const isminefilter& filter = ISMINE_SPENDABLE) - EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); struct COutputEntry { CTxDestination destination; diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index b5de4b4b3d3..165fc2c03e7 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -363,35 +363,6 @@ BOOST_FIXTURE_TEST_CASE(write_wallet_settings_concurrently, TestingSetup) /*num_expected_wallets=*/0); } -// Check that GetImmatureCredit() returns a newly calculated value instead of -// the cached value after a MarkDirty() call. -// -// This is a regression test written to verify a bugfix for the immature credit -// function. Similar tests probably should be written for the other credit and -// debit functions. -BOOST_FIXTURE_TEST_CASE(coin_mark_dirty_immature_credit, TestChain100Setup) -{ - CWallet wallet(m_node.chain.get(), "", CreateMockableWalletDatabase()); - - LOCK(wallet.cs_wallet); - LOCK(Assert(m_node.chainman)->GetMutex()); - CWalletTx wtx{m_coinbase_txns.back(), TxStateConfirmed{m_node.chainman->ActiveChain().Tip()->GetBlockHash(), m_node.chainman->ActiveChain().Height(), /*index=*/0}}; - wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS); - wallet.SetupDescriptorScriptPubKeyMans(); - - wallet.SetLastBlockProcessed(m_node.chainman->ActiveChain().Height(), m_node.chainman->ActiveChain().Tip()->GetBlockHash()); - - // Call GetImmatureCredit() once before adding the key to the wallet to - // cache the current immature credit amount, which is 0. - BOOST_CHECK_EQUAL(CachedTxGetImmatureCredit(wallet, wtx, ISMINE_SPENDABLE), 0); - - // Invalidate the cached value, add the key, and make sure a new immature - // credit amount is calculated. - wtx.MarkDirty(); - AddKey(wallet, coinbaseKey); - BOOST_CHECK_EQUAL(CachedTxGetImmatureCredit(wallet, wtx, ISMINE_SPENDABLE), 50*COIN); -} - static int64_t AddTx(ChainstateManager& chainman, CWallet& wallet, uint32_t lockTime, int64_t mockTime, int64_t blockTime) { CMutableTransaction tx; @@ -961,65 +932,5 @@ BOOST_FIXTURE_TEST_CASE(RemoveTxs, TestChain100Setup) TestUnloadWallet(std::move(wallet)); } -/** - * Checks a wallet invalid state where the inputs (prev-txs) of a new arriving transaction are not marked dirty, - * while the transaction that spends them exist inside the in-memory wallet tx map (not stored on db due a db write failure). - */ -BOOST_FIXTURE_TEST_CASE(wallet_sync_tx_invalid_state_test, TestingSetup) -{ - CWallet wallet(m_node.chain.get(), "", CreateMockableWalletDatabase()); - { - LOCK(wallet.cs_wallet); - wallet.SetWalletFlag(WALLET_FLAG_DESCRIPTORS); - wallet.SetupDescriptorScriptPubKeyMans(); - } - - // Add tx to wallet - const auto op_dest{*Assert(wallet.GetNewDestination(OutputType::BECH32M, ""))}; - - CMutableTransaction mtx; - mtx.vout.emplace_back(COIN, GetScriptForDestination(op_dest)); - mtx.vin.emplace_back(Txid::FromUint256(m_rng.rand256()), 0); - const auto& tx_id_to_spend = wallet.AddToWallet(MakeTransactionRef(mtx), TxStateInMempool{})->GetHash(); - - { - // Cache and verify available balance for the wtx - LOCK(wallet.cs_wallet); - const CWalletTx* wtx_to_spend = wallet.GetWalletTx(tx_id_to_spend); - BOOST_CHECK_EQUAL(CachedTxGetAvailableCredit(wallet, *wtx_to_spend), 1 * COIN); - } - - // Now the good case: - // 1) Add a transaction that spends the previously created transaction - // 2) Verify that the available balance of this new tx and the old one is updated (prev tx is marked dirty) - - mtx.vin.clear(); - mtx.vin.emplace_back(tx_id_to_spend, 0); - wallet.transactionAddedToMempool(MakeTransactionRef(mtx)); - const auto good_tx_id{mtx.GetHash()}; - - { - // Verify balance update for the new tx and the old one - LOCK(wallet.cs_wallet); - const CWalletTx* new_wtx = wallet.GetWalletTx(good_tx_id.ToUint256()); - BOOST_CHECK_EQUAL(CachedTxGetAvailableCredit(wallet, *new_wtx), 1 * COIN); - - // Now the old wtx - const CWalletTx* wtx_to_spend = wallet.GetWalletTx(tx_id_to_spend); - BOOST_CHECK_EQUAL(CachedTxGetAvailableCredit(wallet, *wtx_to_spend), 0 * COIN); - } - - // Now the bad case: - // 1) Make db always fail - // 2) Try to add a transaction that spends the previously created transaction and - // verify that we are not moving forward if the wallet cannot store it - GetMockableDatabase(wallet).m_pass = false; - mtx.vin.clear(); - mtx.vin.emplace_back(good_tx_id, 0); - BOOST_CHECK_EXCEPTION(wallet.transactionAddedToMempool(MakeTransactionRef(mtx)), - std::runtime_error, - HasReason("DB error adding transaction to wallet, write failed")); -} - BOOST_AUTO_TEST_SUITE_END() } // namespace wallet diff --git a/src/wallet/transaction.h b/src/wallet/transaction.h index ac9a8db8d9b..fef7d833997 100644 --- a/src/wallet/transaction.h +++ b/src/wallet/transaction.h @@ -225,7 +225,7 @@ public: std::multimap::const_iterator m_it_wtxOrdered; // memory only - enum AmountType { DEBIT, CREDIT, IMMATURE_CREDIT, AVAILABLE_CREDIT, AMOUNTTYPE_ENUM_ELEMENTS }; + enum AmountType { DEBIT, CREDIT, AMOUNTTYPE_ENUM_ELEMENTS }; mutable CachableAmount m_amounts[AMOUNTTYPE_ENUM_ELEMENTS]; /** * This flag is true if all m_amounts caches are empty. This is particularly @@ -322,8 +322,6 @@ public: { m_amounts[DEBIT].Reset(); m_amounts[CREDIT].Reset(); - m_amounts[IMMATURE_CREDIT].Reset(); - m_amounts[AVAILABLE_CREDIT].Reset(); fChangeCached = false; m_is_cache_empty = true; } From 48545bdb3b9f59a8b78640dbf20afff6e2663ad7 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 20 Feb 2024 11:50:17 -0500 Subject: [PATCH 12/12] bench: Have AvailableCoins benchmark include a lot of unrelated utxos One of the main issues with AvailableCoins is its performance when txs have unrelated outputs, so update the benchmark to check the performance of that. --- src/bench/wallet_create_tx.cpp | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/bench/wallet_create_tx.cpp b/src/bench/wallet_create_tx.cpp index 3c4b2f4f83a..d6173191a16 100644 --- a/src/bench/wallet_create_tx.cpp +++ b/src/bench/wallet_create_tx.cpp @@ -71,10 +71,17 @@ void generateFakeBlock(const CChainParams& params, coinbase_tx.vin[0].prevout.SetNull(); coinbase_tx.vout.resize(2); coinbase_tx.vout[0].scriptPubKey = coinbase_out_script; - coinbase_tx.vout[0].nValue = 49 * COIN; + coinbase_tx.vout[0].nValue = 48 * COIN; coinbase_tx.vin[0].scriptSig = CScript() << ++tip.tip_height << OP_0; coinbase_tx.vout[1].scriptPubKey = coinbase_out_script; // extra output coinbase_tx.vout[1].nValue = 1 * COIN; + + // Fill the coinbase with outputs that don't belong to the wallet in order to benchmark + // AvailableCoins' behavior with unnecessary TXOs + for (int i = 0; i < 50; ++i) { + coinbase_tx.vout.emplace_back(1 * COIN / 50, CScript(OP_TRUE)); + } + block.vtx = {MakeTransactionRef(std::move(coinbase_tx))}; block.nVersion = VERSIONBITS_LAST_OLD_BLOCK_VERSION; @@ -129,14 +136,14 @@ static void WalletCreateTx(benchmark::Bench& bench, const OutputType output_type // Check available balance auto bal = WITH_LOCK(wallet.cs_wallet, return wallet::AvailableCoins(wallet).GetTotalAmount()); // Cache - assert(bal == 50 * COIN * (chain_size - COINBASE_MATURITY)); + assert(bal == 49 * COIN * (chain_size - COINBASE_MATURITY)); wallet::CCoinControl coin_control; coin_control.m_allow_other_inputs = allow_other_inputs; CAmount target = 0; if (preset_inputs) { - // Select inputs, each has 49 BTC + // Select inputs, each has 48 BTC wallet::CoinFilterParams filter_coins; filter_coins.max_count = preset_inputs->num_of_internal_inputs; const auto& res = WITH_LOCK(wallet.cs_wallet, @@ -189,7 +196,7 @@ static void AvailableCoins(benchmark::Bench& bench, const std::vector