diff --git a/src/bench/wallet_create_tx.cpp b/src/bench/wallet_create_tx.cpp index 3c4b2f4f83a..7fb535f4c72 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, @@ -152,7 +159,7 @@ static void WalletCreateTx(benchmark::Bench& bench, const OutputType output_type if (coin_control.m_allow_other_inputs) target += 50 * COIN; std::vector recipients = {{dest, target, true}}; - bench.epochIterations(5).run([&] { + bench.run([&] { LOCK(wallet.cs_wallet); const auto& tx_res = CreateTransaction(wallet, recipients, /*change_pos=*/std::nullopt, coin_control); assert(tx_res); @@ -189,9 +196,9 @@ static void AvailableCoins(benchmark::Bench& bench, const std::vector #include +#include #include #include #include @@ -145,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, @@ -257,6 +212,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 @@ -293,27 +252,41 @@ 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; + for (const auto& [outpoint, txo] : wallet.GetTXOs()) { + const CWalletTx& wtx = txo.GetWalletTx(); + 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; + + 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(wtx) && wtx.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 && wtx.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; @@ -326,31 +299,21 @@ 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()) { + const CWalletTx& wtx = txo.GetWalletTx(); - if (!CachedTxIsTrusted(wallet, wtx, trusted_parents)) - continue; - - if (wallet.IsTxImmatureCoinBase(wtx)) - continue; + if (!CachedTxIsTrusted(wallet, wtx, trusted_parents)) continue; + if (wallet.IsTxImmatureCoinBase(wtx)) continue; int nDepth = wallet.GetTxDepthInMainChain(wtx); - if (nDepth < (CachedTxIsFromMe(wallet, wtx, ISMINE_ALL) ? 0 : 1)) - continue; + 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; + CTxDestination addr; + Assume(wallet.IsMine(txo.GetTxOut())); + if(!ExtractDestination(txo.GetTxOut().scriptPubKey, addr)) continue; - CAmount n = wallet.IsSpent(COutPoint(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/receive.h b/src/wallet/receive.h index b795689de51..330fbf21a65 100644 --- a/src/wallet/receive.h +++ b/src/wallet/receive.h @@ -30,10 +30,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/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp index 39df2166bef..eece0815111 100644 --- a/src/wallet/rpc/addresses.cpp +++ b/src/wallet/rpc/addresses.cpp @@ -250,6 +250,7 @@ RPCHelpMan keypoolrefill() if (pwallet->GetKeyPoolSize() < kpSize) { throw JSONRPCError(RPC_WALLET_ERROR, "Error refreshing keypool."); } + pwallet->RefreshAllTXOs(); return UniValue::VNULL; }, diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index f7d37239c6c..edd1fb86c99 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -394,6 +394,7 @@ RPCHelpMan importdescriptors() } } pwallet->ConnectScriptPubKeyManNotifiers(); + pwallet->RefreshAllTXOs(); } // Rescan the blockchain using the lowest timestamp diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index d96946e7b89..fe3f4b57b95 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -277,12 +277,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); } @@ -330,137 +326,146 @@ CoinsResult AvailableCoins(const CWallet& wallet, std::vector outpoints; std::set trusted_parents; - for (const auto& entry : wallet.mapWallet) - { - const Txid& 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 (wallet.IsTxImmatureCoinBase(wtx) && !params.include_immature_coinbase) + if (tx_safe_cache.contains(outpoint.hash) && !tx_safe_cache.at(outpoint.hash).first) { continue; + } 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); + if (wallet.IsTxImmatureCoinBase(wtx) && !params.include_immature_coinbase) + 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; + 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; + + 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) { + const auto& [tx_ok, tx_safe] = tx_safe_cache.at(outpoint.hash); + if (!Assume(tx_ok)) { continue; } - if (nDepth < min_depth || nDepth > max_depth) { + if (output.nValue < params.min_amount || output.nValue > params.max_amount) + 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; + + isminetype mine = wallet.IsMine(output); + assert(mine != ISMINE_NO); + + if (!allow_used_addresses && wallet.IsSpentKey(output.scriptPubKey)) { 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, 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()) { diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 1b0cfe2c4f5..8f8da3c3025 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -244,35 +244,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; @@ -751,65 +722,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); - 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 1ec62b992b6..b44f0c7d059 100644 --- a/src/wallet/transaction.h +++ b/src/wallet/transaction.h @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -218,7 +219,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 @@ -315,8 +316,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; } @@ -362,6 +361,30 @@ 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) + { + Assume(std::ranges::find(wtx.tx->vout, output) != wtx.tx->vout.end()); + } + + 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 991b138b85f..36da4396106 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1094,6 +1094,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 + RefreshTXOsFromTx(wtx); + // Notify UI of new or updated transaction NotifyTransactionChanged(hash, fInsertedNew ? CT_NEW : CT_UPDATED); @@ -1157,6 +1160,8 @@ bool CWallet::LoadToWallet(const Txid& 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 + RefreshTXOsFromTx(wtx); return true; } @@ -1538,16 +1543,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; } @@ -2324,6 +2323,9 @@ util::Result CWallet::RemoveTxs(WalletBatch& batch, std::vector& txs 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); } @@ -3735,6 +3737,9 @@ util::Result> CWallet::AddWall // Save the descriptor to DB spk_man->WriteDescriptor(); + // Break balance caches so that outputs that are now IsMine in already known txs will be included in the balance + MarkDirty(); + return std::reference_wrapper(*spk_man); } @@ -3890,6 +3895,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; @@ -4422,4 +4431,40 @@ void CWallet::WriteBestBlock() const batch.WriteBestBlock(loc); } } + +void CWallet::RefreshTXOsFromTx(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}); + } + } +} + +void CWallet::RefreshAllTXOs() +{ + AssertLockHeld(cs_wallet); + for (const auto& [_, wtx] : mapWallet) { + RefreshTXOsFromTx(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 03c2ca2ffbf..2db522c4f4d 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -438,6 +438,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 @@ -520,6 +523,14 @@ 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 RefreshTXOsFromTx(const CWalletTx& wtx) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + /** Cache outputs that belong to the wallet for all transactions in the wallet */ + void RefreshAllTXOs() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + /** * Return depth of transaction in blockchain: * <0 : conflicts with a transaction this deep in the blockchain diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 6f552076633..c23686818bd 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -1163,14 +1163,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, 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, any_unordered), result); } catch (std::runtime_error& e) { // 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. diff --git a/test/functional/wallet_balance.py b/test/functional/wallet_balance.py index c3d9596b8a0..91f58efc622 100755 --- a/test/functional/wallet_balance.py +++ b/test/functional/wallet_balance.py @@ -4,15 +4,18 @@ # 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 +from test_framework.descriptors import descsum_create from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_equal, assert_is_hash_string, assert_raises_rpc_error, ) +from test_framework.wallet_util import get_generate_key def create_transactions(node, address, amt, fees): @@ -279,5 +282,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.importdescriptors([{"desc": descsum_create(f"wpkh({import_key1.privkey})"), "timestamp": "now"}]) + + 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.importdescriptors([{"desc": descsum_create(f"wpkh({import_key2.privkey})"), "timestamp": "now"}]) + balances = wallet.getbalances() + assert_equal(balances["mine"]["trusted"], amount * 2) + if __name__ == '__main__': WalletTest(__file__).main()