From 6a7aa015747e2634fe5a4b2f7fa0d104eb75c796 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Thu, 15 May 2025 17:57:38 -0700 Subject: [PATCH] wallet: Remove COutput::spendable and AvailableCoinsListUnspent In descriptor wallets, we consider all outputs to be spendable as we no longer have mixed mine and watchonly in a wallet. As such, COutput::spendable is meaningless and can be removed. Furthermore, CoinFilterParams::only_spendable can be removed as that was essentially checking for COutput::spendable. Lastly, AvailableCoinsListUnspent can also be removed as the wrapper is now only setting the feerate to std::nullopt which is trivial enough that a dedicated wrapper is not needed. --- src/bench/coin_selection.cpp | 4 +-- src/wallet/coinselection.h | 10 +++----- src/wallet/rpc/coins.cpp | 7 ++--- src/wallet/spend.cpp | 34 ++++++++----------------- src/wallet/spend.h | 8 ------ src/wallet/test/coinselection_tests.cpp | 2 +- src/wallet/test/coinselector_tests.cpp | 16 ++++++------ src/wallet/test/fuzz/coinselection.cpp | 2 +- src/wallet/test/group_outputs_tests.cpp | 1 - src/wallet/test/wallet_tests.cpp | 4 +-- 10 files changed, 31 insertions(+), 57 deletions(-) diff --git a/src/bench/coin_selection.cpp b/src/bench/coin_selection.cpp index a752f73610..dfefa1b530 100644 --- a/src/bench/coin_selection.cpp +++ b/src/bench/coin_selection.cpp @@ -74,7 +74,7 @@ static void CoinSelection(benchmark::Bench& bench) wallet::CoinsResult available_coins; for (const auto& wtx : wtxs) { const auto txout = wtx->tx->vout.at(0); - available_coins.coins[OutputType::BECH32].emplace_back(COutPoint(wtx->GetHash(), 0), txout, /*depth=*/6 * 24, CalculateMaximumSignedInputSize(txout, &wallet, /*coin_control=*/nullptr), /*spendable=*/true, /*solvable=*/true, /*safe=*/true, wtx->GetTxTime(), /*from_me=*/true, /*fees=*/ 0); + available_coins.coins[OutputType::BECH32].emplace_back(COutPoint(wtx->GetHash(), 0), txout, /*depth=*/6 * 24, CalculateMaximumSignedInputSize(txout, &wallet, /*coin_control=*/nullptr), /*solvable=*/true, /*safe=*/true, wtx->GetTxTime(), /*from_me=*/true, /*fees=*/ 0); } const CoinEligibilityFilter filter_standard(1, 6, 0); @@ -105,7 +105,7 @@ static void add_coin(const CAmount& nValue, int nInput, std::vector CMutableTransaction tx; tx.vout.resize(nInput + 1); tx.vout[nInput].nValue = nValue; - COutput output(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/ 0, /*input_bytes=*/ -1, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ true, /*fees=*/ 0); + COutput output(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/0, /*input_bytes=*/-1, /*solvable=*/true, /*safe=*/true, /*time=*/0, /*from_me=*/true, /*fees=*/0); set.emplace_back(); set.back().Insert(std::make_shared(output), /*ancestors=*/ 0, /*descendants=*/ 0); } diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index c86765c4a5..ca9f269818 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -50,9 +50,6 @@ public: /** Pre-computed estimated size of this output as a fully-signed input in a transaction. Can be -1 if it could not be calculated */ int input_bytes; - /** Whether we have the private keys to spend this output */ - bool spendable; - /** Whether we know how to spend this output, ignoring the lack of keys */ bool solvable; @@ -75,12 +72,11 @@ public: /** The fee necessary to bump this UTXO's ancestor transactions to the target feerate */ CAmount ancestor_bump_fees{0}; - COutput(const COutPoint& outpoint, const CTxOut& txout, int depth, int input_bytes, bool spendable, bool solvable, bool safe, int64_t time, bool from_me, const std::optional feerate = std::nullopt) + COutput(const COutPoint& outpoint, const CTxOut& txout, int depth, int input_bytes, bool solvable, bool safe, int64_t time, bool from_me, const std::optional feerate = std::nullopt) : outpoint{outpoint}, txout{txout}, depth{depth}, input_bytes{input_bytes}, - spendable{spendable}, solvable{solvable}, safe{safe}, time{time}, @@ -93,8 +89,8 @@ public: } } - COutput(const COutPoint& outpoint, const CTxOut& txout, int depth, int input_bytes, bool spendable, bool solvable, bool safe, int64_t time, bool from_me, const CAmount fees) - : COutput(outpoint, txout, depth, input_bytes, spendable, solvable, safe, time, from_me) + COutput(const COutPoint& outpoint, const CTxOut& txout, int depth, int input_bytes, bool solvable, bool safe, int64_t time, bool from_me, const CAmount fees) + : COutput(outpoint, txout, depth, input_bytes, solvable, safe, time, from_me) { // if input_bytes is unknown, then fees should be 0, if input_bytes is known, then the fees should be a positive integer or 0 (input_bytes known and fees = 0 only happens in the tests) assert((input_bytes < 0 && fees == 0) || (input_bytes > 0 && fees >= 0)); diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp index 1902af1ce2..4aef0dda88 100644 --- a/src/wallet/rpc/coins.cpp +++ b/src/wallet/rpc/coins.cpp @@ -498,7 +498,7 @@ RPCHelpMan listunspent() {RPCResult::Type::STR_AMOUNT, "ancestorfees", /*optional=*/true, "The total fees of in-mempool ancestors (including this one) with fee deltas used for mining priority in " + CURRENCY_ATOM + " (if transaction is in the mempool)"}, {RPCResult::Type::STR_HEX, "redeemScript", /*optional=*/true, "The redeem script if the output script is P2SH"}, {RPCResult::Type::STR, "witnessScript", /*optional=*/true, "witness script if the output script is P2WSH or P2SH-P2WSH"}, - {RPCResult::Type::BOOL, "spendable", "Whether we have the private keys to spend this output"}, + {RPCResult::Type::BOOL, "spendable", "(DEPRECATED) Always true"}, {RPCResult::Type::BOOL, "solvable", "Whether we know how to spend this output, ignoring the lack of keys"}, {RPCResult::Type::BOOL, "reused", /*optional=*/true, "(only present if avoid_reuse is set) Whether this output is reused/dirty (sent to an address that was previously spent from)"}, {RPCResult::Type::STR, "desc", /*optional=*/true, "(only when solvable) A descriptor for spending this output"}, @@ -598,8 +598,9 @@ RPCHelpMan listunspent() cctl.m_min_depth = nMinDepth; cctl.m_max_depth = nMaxDepth; cctl.m_include_unsafe_inputs = include_unsafe; + filter_coins.check_version_trucness = false; LOCK(pwallet->cs_wallet); - vecOutputs = AvailableCoinsListUnspent(*pwallet, &cctl, filter_coins).All(); + vecOutputs = AvailableCoins(*pwallet, &cctl, /*feerate=*/std::nullopt, filter_coins).All(); } LOCK(pwallet->cs_wallet); @@ -672,7 +673,7 @@ RPCHelpMan listunspent() entry.pushKV("ancestorfees", uint64_t(ancestor_fees)); } } - entry.pushKV("spendable", out.spendable); + entry.pushKV("spendable", true); // Any coins we list are always spendable entry.pushKV("solvable", out.solvable); if (out.solvable) { std::unique_ptr provider = pwallet->GetSolvingProvider(scriptPubKey); diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index e29ca04526..a5bd816b56 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -309,8 +309,8 @@ util::Result FetchSelectedInputs(const CWallet& wallet, const return util::Error{strprintf(_("Not solvable pre-selected input %s"), outpoint.ToString())}; // Not solvable, can't estimate size for fee } - /* Set some defaults for depth, spendable, solvable, safe, time, and from_me as these don't matter for preset inputs since no selection is being done. */ - COutput output(outpoint, txout, /*depth=*/ 0, input_bytes, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ false, coin_selection_params.m_effective_feerate); + /* Set some defaults for depth, solvable, safe, time, and from_me as these don't matter for preset inputs since no selection is being done. */ + COutput output(outpoint, txout, /*depth=*/0, input_bytes, /*solvable=*/true, /*safe=*/true, /*time=*/0, /*from_me=*/false, coin_selection_params.m_effective_feerate); output.ApplyBumpFee(map_of_bump_fees.at(output.outpoint)); result.Insert(output, coin_selection_params.m_subtract_fee_outputs); } @@ -452,10 +452,6 @@ CoinsResult AvailableCoins(const CWallet& wallet, // 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; - - // Filter by spendable outputs only - if (!spendable && params.only_spendable) continue; // Obtain script type std::vector> script_solutions; @@ -474,7 +470,7 @@ CoinsResult AvailableCoins(const CWallet& wallet, } auto available_output_type = GetOutputType(type, is_from_p2sh); - auto available_output = COutput(outpoint, output, nDepth, input_bytes, spendable, solvable, tx_safe, wtx.GetTxTime(), tx_from_me, feerate); + auto available_output = COutput(outpoint, output, nDepth, input_bytes, solvable, tx_safe, wtx.GetTxTime(), tx_from_me, feerate); if (wtx.tx->version == TRUC_VERSION && nDepth == 0 && params.check_version_trucness) { unconfirmed_truc_coins.emplace_back(available_output_type, available_output); auto [it, _] = truc_txid_by_value.try_emplace(wtx.tx->GetHash(), 0); @@ -528,13 +524,6 @@ CoinsResult AvailableCoins(const CWallet& wallet, return result; } -CoinsResult AvailableCoinsListUnspent(const CWallet& wallet, const CCoinControl* coinControl, CoinFilterParams params) -{ - params.only_spendable = false; - params.check_version_trucness = false; - return AvailableCoins(wallet, coinControl, /*feerate=*/ std::nullopt, params); -} - const CTxOut& FindNonChangeParentOutput(const CWallet& wallet, const COutPoint& outpoint) { AssertLockHeld(wallet.cs_wallet); @@ -563,21 +552,18 @@ std::map> ListCoins(const CWallet& wallet) CCoinControl coin_control; CoinFilterParams coins_params; - coins_params.only_spendable = false; coins_params.skip_locked = false; for (const COutput& coin : AvailableCoins(wallet, &coin_control, /*feerate=*/std::nullopt, coins_params).All()) { CTxDestination address; - if ((coin.spendable || (wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && coin.solvable))) { - if (!ExtractDestination(FindNonChangeParentOutput(wallet, coin.outpoint).scriptPubKey, address)) { - // For backwards compatibility, we convert P2PK output scripts into PKHash destinations - if (auto pk_dest = std::get_if(&address)) { - address = PKHash(pk_dest->GetPubKey()); - } else { - continue; - } + if (!ExtractDestination(FindNonChangeParentOutput(wallet, coin.outpoint).scriptPubKey, address)) { + // For backwards compatibility, we convert P2PK output scripts into PKHash destinations + if (auto pk_dest = std::get_if(&address)) { + address = PKHash(pk_dest->GetPubKey()); + } else { + continue; } - result[address].emplace_back(coin); } + result[address].emplace_back(coin); } return result; } diff --git a/src/wallet/spend.h b/src/wallet/spend.h index 19625c113e..e8761bf73d 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -77,8 +77,6 @@ struct CoinFilterParams { CAmount min_sum_amount{MAX_MONEY}; // Maximum number of outputs that can be returned uint64_t max_count{0}; - // By default, return only spendable outputs - bool only_spendable{true}; // By default, do not include immature coinbase outputs bool include_immature_coinbase{false}; // By default, skip locked UTXOs @@ -96,12 +94,6 @@ CoinsResult AvailableCoins(const CWallet& wallet, std::optional feerate = std::nullopt, const CoinFilterParams& params = {}) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); -/** - * Wrapper function for AvailableCoins which skips the `feerate` and `CoinFilterParams::only_spendable` parameters. Use this function - * to list all available coins (e.g. listunspent RPC) while not intending to fund a transaction. - */ -CoinsResult AvailableCoinsListUnspent(const CWallet& wallet, const CCoinControl* coinControl = nullptr, CoinFilterParams params = {}) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); - /** * Find non-change parent output. */ diff --git a/src/wallet/test/coinselection_tests.cpp b/src/wallet/test/coinselection_tests.cpp index 9c39068222..b4608383ce 100644 --- a/src/wallet/test/coinselection_tests.cpp +++ b/src/wallet/test/coinselection_tests.cpp @@ -53,7 +53,7 @@ static OutputGroup MakeCoin(const CAmount& amount, bool is_eff_value = true, Coi tx.vout[0].nValue = amount + int(is_eff_value) * fees; tx.nLockTime = next_lock_time++; // so all transactions get different hashes OutputGroup group(cs_params); - group.Insert(std::make_shared(COutPoint(tx.GetHash(), 0), tx.vout.at(0), /*depth=*/1, /*input_bytes=*/custom_spending_vsize, /*spendable=*/true, /*solvable=*/true, /*safe=*/true, /*time=*/0, /*from_me=*/false, /*fees=*/fees), /*ancestors=*/0, /*descendants=*/0); + group.Insert(std::make_shared(COutPoint(tx.GetHash(), 0), tx.vout.at(0), /*depth=*/1, /*input_bytes=*/custom_spending_vsize, /*solvable=*/true, /*safe=*/true, /*time=*/0, /*from_me=*/false, /*fees=*/fees), /*ancestors=*/0, /*descendants=*/0); return group; } diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index fbf58eb13a..9a61a4d567 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -43,7 +43,7 @@ static void add_coin(const CAmount& nValue, int nInput, SelectionResult& result) tx.vout.resize(nInput + 1); tx.vout[nInput].nValue = nValue; tx.nLockTime = nextLockTime++; // so all transactions get different hashes - COutput output(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/ 1, /*input_bytes=*/ -1, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ false, /*fees=*/ 0); + COutput output(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/1, /*input_bytes=*/-1, /*solvable=*/true, /*safe=*/true, /*time=*/0, /*from_me=*/false, /*fees=*/ 0); OutputGroup group; group.Insert(std::make_shared(output), /*ancestors=*/ 0, /*descendants=*/ 0); result.AddInput(group); @@ -55,7 +55,7 @@ static void add_coin(const CAmount& nValue, int nInput, SelectionResult& result, tx.vout.resize(nInput + 1); tx.vout[nInput].nValue = nValue; tx.nLockTime = nextLockTime++; // so all transactions get different hashes - std::shared_ptr coin = std::make_shared(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/ 1, /*input_bytes=*/ 148, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ false, fee); + std::shared_ptr coin = std::make_shared(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/1, /*input_bytes=*/148, /*solvable=*/true, /*safe=*/true, /*time=*/0, /*from_me=*/false, fee); OutputGroup group; group.Insert(coin, /*ancestors=*/ 0, /*descendants=*/ 0); coin->long_term_fee = long_term_fee; // group.Insert() will modify long_term_fee, so we need to set it afterwards @@ -78,7 +78,7 @@ static void add_coin(CoinsResult& available_coins, CWallet& wallet, const CAmoun assert(ret.second); CWalletTx& wtx = (*ret.first).second; const auto& txout = wtx.tx->vout.at(nInput); - available_coins.Add(OutputType::BECH32, {COutPoint(wtx.GetHash(), nInput), txout, nAge, custom_size == 0 ? CalculateMaximumSignedInputSize(txout, &wallet, /*coin_control=*/nullptr) : custom_size, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx.GetTxTime(), fIsFromMe, feerate}); + available_coins.Add(OutputType::BECH32, {COutPoint(wtx.GetHash(), nInput), txout, nAge, custom_size == 0 ? CalculateMaximumSignedInputSize(txout, &wallet, /*coin_control=*/nullptr) : custom_size, /*solvable=*/true, /*safe=*/true, wtx.GetTxTime(), fIsFromMe, feerate}); } // Helpers @@ -911,26 +911,26 @@ BOOST_AUTO_TEST_CASE(effective_value_test) tx.vout[nInput].nValue = nValue; // standard case, pass feerate in constructor - COutput output1(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/ 1, input_bytes, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ false, feerate); + COutput output1(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/1, input_bytes, /*solvable=*/true, /*safe=*/true, /*time=*/0, /*from_me=*/false, feerate); const CAmount expected_ev1 = 9852; // 10000 - 148 BOOST_CHECK_EQUAL(output1.GetEffectiveValue(), expected_ev1); // input bytes unknown (input_bytes = -1), pass feerate in constructor - COutput output2(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/ 1, /*input_bytes=*/ -1, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ false, feerate); + COutput output2(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/1, /*input_bytes=*/-1, /*solvable=*/true, /*safe=*/true, /*time=*/0, /*from_me=*/ false, feerate); BOOST_CHECK_EQUAL(output2.GetEffectiveValue(), nValue); // The effective value should be equal to the absolute value if input_bytes is -1 // negative effective value, pass feerate in constructor - COutput output3(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/ 1, input_bytes, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ false, CFeeRate(100000)); + COutput output3(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/1, input_bytes, /*solvable=*/true, /*safe=*/true, /*time=*/0, /*from_me=*/false, CFeeRate(100000)); const CAmount expected_ev3 = -4800; // 10000 - 14800 BOOST_CHECK_EQUAL(output3.GetEffectiveValue(), expected_ev3); // standard case, pass fees in constructor const CAmount fees = 148; - COutput output4(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/ 1, input_bytes, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ false, fees); + COutput output4(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/1, input_bytes, /*solvable=*/true, /*safe=*/true, /*time=*/0, /*from_me=*/false, fees); BOOST_CHECK_EQUAL(output4.GetEffectiveValue(), expected_ev1); // input bytes unknown (input_bytes = -1), pass fees in constructor - COutput output5(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/ 1, /*input_bytes=*/ -1, /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, /*time=*/ 0, /*from_me=*/ false, /*fees=*/ 0); + COutput output5(COutPoint(tx.GetHash(), nInput), tx.vout.at(nInput), /*depth=*/1, /*input_bytes=*/-1, /*solvable=*/true, /*safe=*/true, /*time=*/0, /*from_me=*/false, /*fees=*/0); BOOST_CHECK_EQUAL(output5.GetEffectiveValue(), nValue); // The effective value should be equal to the absolute value if input_bytes is -1 } diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index a0b2142e60..8bb183405c 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -23,7 +23,7 @@ static void AddCoin(const CAmount& value, int n_input, int n_input_bytes, int lo tx.vout.resize(n_input + 1); tx.vout[n_input].nValue = value; tx.nLockTime = locktime; // all transactions get different hashes - coins.emplace_back(COutPoint(tx.GetHash(), n_input), tx.vout.at(n_input), /*depth=*/0, n_input_bytes, /*spendable=*/true, /*solvable=*/true, /*safe=*/true, /*time=*/0, /*from_me=*/true, fee_rate); + coins.emplace_back(COutPoint(tx.GetHash(), n_input), tx.vout.at(n_input), /*depth=*/0, n_input_bytes, /*solvable=*/true, /*safe=*/true, /*time=*/0, /*from_me=*/true, fee_rate); } // Randomly distribute coins to instances of OutputGroup diff --git a/src/wallet/test/group_outputs_tests.cpp b/src/wallet/test/group_outputs_tests.cpp index 4609c37ac7..f085f4de59 100644 --- a/src/wallet/test/group_outputs_tests.cpp +++ b/src/wallet/test/group_outputs_tests.cpp @@ -51,7 +51,6 @@ static void addCoin(CoinsResult& coins, txout, depth, CalculateMaximumSignedInputSize(txout, &wallet, /*coin_control=*/nullptr), - /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx.GetTxTime(), diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index b23b6e9460..bcff25931a 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -424,7 +424,7 @@ BOOST_FIXTURE_TEST_CASE(ListCoinsTest, ListCoinsTestingSetup) // Lock both coins. Confirm number of available coins drops to 0. { LOCK(wallet->cs_wallet); - BOOST_CHECK_EQUAL(AvailableCoinsListUnspent(*wallet).Size(), 2U); + BOOST_CHECK_EQUAL(AvailableCoins(*wallet).Size(), 2U); } for (const auto& group : list) { for (const auto& coin : group.second) { @@ -434,7 +434,7 @@ BOOST_FIXTURE_TEST_CASE(ListCoinsTest, ListCoinsTestingSetup) } { LOCK(wallet->cs_wallet); - BOOST_CHECK_EQUAL(AvailableCoinsListUnspent(*wallet).Size(), 0U); + BOOST_CHECK_EQUAL(AvailableCoins(*wallet).Size(), 0U); } // Confirm ListCoins still returns same result as before, despite coins // being locked.