diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index d96946e7b89..9d6cf41a428 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -330,137 +330,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()) {