diff --git a/src/util/insert.h b/src/util/insert.h index 995b382a033..96dc51b5006 100644 --- a/src/util/insert.h +++ b/src/util/insert.h @@ -19,6 +19,11 @@ inline void insert(std::set& dst, const Tsrc& src) { dst.insert(src.begin(), src.end()); } +template +inline void insert(std::set& dst, const Tsrc& src) { + dst.insert(src.begin(), src.end()); +} + } // namespace util #endif // BITCOIN_UTIL_INSERT_H diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index 192e49361a1..8977d999382 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -908,7 +908,7 @@ void SelectionResult::AddInput(const OutputGroup& group) m_weight += group.m_weight; } -void SelectionResult::AddInputs(const std::set>& inputs, bool subtract_fee_outputs) +void SelectionResult::AddInputs(const OutputSet& inputs, bool subtract_fee_outputs) { // As it can fail, combine inputs first InsertInputs(inputs); @@ -933,7 +933,7 @@ void SelectionResult::Merge(const SelectionResult& other) m_weight += other.m_weight; } -const std::set>& SelectionResult::GetInputSet() const +const OutputSet& SelectionResult::GetInputSet() const { return m_selected_inputs; } diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index 9f2aefafe1f..22103de167c 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -319,11 +319,18 @@ enum class SelectionAlgorithm : uint8_t std::string GetAlgorithmName(SelectionAlgorithm algo); +struct OutputPtrComparator { + bool operator()(const std::shared_ptr& a, const std::shared_ptr& b) const { + return *a < *b; + } +}; +using OutputSet = std::set, OutputPtrComparator>; + struct SelectionResult { private: /** Set of inputs selected by the algorithm to use in the transaction */ - std::set> m_selected_inputs; + OutputSet m_selected_inputs; /** The target the algorithm selected for. Equal to the recipient amount plus non-input fees */ CAmount m_target; /** The algorithm used to produce this result */ @@ -368,7 +375,7 @@ public: void Clear(); void AddInput(const OutputGroup& group); - void AddInputs(const std::set>& inputs, bool subtract_fee_outputs); + void AddInputs(const OutputSet& inputs, bool subtract_fee_outputs); /** How much individual inputs overestimated the bump fees for shared ancestries */ void SetBumpFeeDiscount(CAmount discount); @@ -409,7 +416,7 @@ public: void Merge(const SelectionResult& other); /** Get m_selected_inputs */ - const std::set>& GetInputSet() const; + const OutputSet& GetInputSet() const; /** Get the vector of COutputs that will be used to fill in a CTransaction's vin */ std::vector> GetShuffledInputVector() const; diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 28c4d639323..8e0f240a6fe 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -411,7 +411,7 @@ public: CoinSelectionParams params(rng); // Note: for now, swallow any error. if (auto res = FetchSelectedInputs(*m_wallet, coin_control, params)) { - total_amount += res->total_amount; + total_amount += res->GetTotalAmount(); } } diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index e4ac98bb25c..edde9234522 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -203,7 +203,7 @@ size_t CoinsResult::Size() const std::vector CoinsResult::All() const { std::vector all; - all.reserve(coins.size()); + all.reserve(Size()); for (const auto& it : coins) { all.insert(all.end(), it.second.begin(), it.second.end()); } @@ -223,7 +223,7 @@ void CoinsResult::Erase(const std::unordered_set FetchSelectedInputs(const CWallet& wallet, const CCoinControl& coin_control, +util::Result FetchSelectedInputs(const CWallet& wallet, const CCoinControl& coin_control, const CoinSelectionParams& coin_selection_params) { - PreSelectedInputs result; + CoinsResult result; const bool can_grind_r = wallet.CanGrindR(); std::map map_of_bump_fees = wallet.chain().calculateIndividualBumpFees(coin_control.ListSelected(), coin_selection_params.m_effective_feerate); for (const COutPoint& outpoint : coin_control.ListSelected()) { @@ -312,7 +312,7 @@ util::Result FetchSelectedInputs(const CWallet& wallet, const /* 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); + result.Add(OutputType::UNKNOWN, output); } return result; } @@ -788,7 +788,7 @@ util::Result ChooseSelectionResult(interfaces::Chain& chain, co // If the chosen input set has unconfirmed inputs, check for synergies from overlapping ancestry for (auto& result : results) { std::vector outpoints; - std::set> coins = result.GetInputSet(); + OutputSet coins = result.GetInputSet(); CAmount summed_bump_fees = 0; for (auto& coin : coins) { if (coin->depth > 0) continue; // Bump fees only exist for unconfirmed inputs @@ -811,12 +811,12 @@ util::Result ChooseSelectionResult(interfaces::Chain& chain, co return *std::min_element(results.begin(), results.end()); } -util::Result SelectCoins(const CWallet& wallet, CoinsResult& available_coins, const PreSelectedInputs& pre_set_inputs, +util::Result SelectCoins(const CWallet& wallet, CoinsResult& available_coins, const CoinsResult& pre_set_inputs, const CAmount& nTargetValue, const CCoinControl& coin_control, const CoinSelectionParams& coin_selection_params) { // Deduct preset inputs amount from the search target - CAmount selection_target = nTargetValue - pre_set_inputs.total_amount; + CAmount selection_target = nTargetValue - pre_set_inputs.GetAppropriateTotal(coin_selection_params.m_subtract_fee_outputs).value_or(0); // Return if automatic coin selection is disabled, and we don't cover the selection target if (!coin_control.m_allow_other_inputs && selection_target > 0) { @@ -824,18 +824,22 @@ util::Result SelectCoins(const CWallet& wallet, CoinsResult& av "Please allow other inputs to be automatically selected or include more coins manually")}; } + OutputSet preset_coin_set; + for (const auto& output: pre_set_inputs.All()) { + preset_coin_set.insert(std::make_shared(output)); + } + // Return if we can cover the target only with the preset inputs if (selection_target <= 0) { SelectionResult result(nTargetValue, SelectionAlgorithm::MANUAL); - result.AddInputs(pre_set_inputs.coins, coin_selection_params.m_subtract_fee_outputs); + result.AddInputs(preset_coin_set, coin_selection_params.m_subtract_fee_outputs); result.RecalculateWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, coin_selection_params.m_change_fee); return result; } // Return early if we cannot cover the target with the wallet's UTXO. // We use the total effective value if we are not subtracting fee from outputs and 'available_coins' contains the data. - CAmount available_coins_total_amount = coin_selection_params.m_subtract_fee_outputs ? available_coins.GetTotalAmount() : - (available_coins.GetEffectiveTotalAmount().has_value() ? *available_coins.GetEffectiveTotalAmount() : 0); + CAmount available_coins_total_amount = available_coins.GetAppropriateTotal(coin_selection_params.m_subtract_fee_outputs).value_or(0); if (selection_target > available_coins_total_amount) { return util::Error(); // Insufficient funds } @@ -846,8 +850,10 @@ util::Result SelectCoins(const CWallet& wallet, CoinsResult& av // If needed, add preset inputs to the automatic coin selection result if (!pre_set_inputs.coins.empty()) { - SelectionResult preselected(pre_set_inputs.total_amount, SelectionAlgorithm::MANUAL); - preselected.AddInputs(pre_set_inputs.coins, coin_selection_params.m_subtract_fee_outputs); + auto preset_total = pre_set_inputs.GetAppropriateTotal(coin_selection_params.m_subtract_fee_outputs); + assert(preset_total.has_value()); + SelectionResult preselected(preset_total.value(), SelectionAlgorithm::MANUAL); + preselected.AddInputs(preset_coin_set, coin_selection_params.m_subtract_fee_outputs); op_selection_result->Merge(preselected); op_selection_result->RecalculateWaste(coin_selection_params.min_viable_change, coin_selection_params.m_cost_of_change, @@ -1182,7 +1188,7 @@ static util::Result CreateTransactionInternal( } // Fetch manually selected coins - PreSelectedInputs preset_inputs; + CoinsResult preset_inputs; if (coin_control.HasSelected()) { auto res_fetch_inputs = FetchSelectedInputs(wallet, coin_control, coin_selection_params); if (!res_fetch_inputs) return util::Error{util::ErrorString(res_fetch_inputs)}; @@ -1201,7 +1207,25 @@ static util::Result CreateTransactionInternal( if (!select_coins_res) { // 'SelectCoins' either returns a specific error message or, if empty, means a general "Insufficient funds". const bilingual_str& err = util::ErrorString(select_coins_res); - return util::Error{err.empty() ?_("Insufficient funds") : err}; + if (!err.empty()) return util::Error{err}; + + // Check if we have enough balance but cannot cover the fees + CAmount available_balance = preset_inputs.GetTotalAmount() + available_coins.GetTotalAmount(); + // Note: if SelectCoins() fails when SFFO is enabled (recipients_sum = selection_target with SFFO), + // then recipients_sum > available_balance and we wouldn't enter into the if condition below. + if (available_balance >= recipients_sum) { + // If we have coins with balance, they should have effective values since we constructed them with valid feerate. + assert(!preset_inputs.Size() || preset_inputs.GetEffectiveTotalAmount().has_value()); + assert(!available_coins.Size() || available_coins.GetEffectiveTotalAmount().has_value()); + CAmount available_effective_balance = preset_inputs.GetEffectiveTotalAmount().value_or(0) + available_coins.GetEffectiveTotalAmount().value_or(0); + if (available_effective_balance < selection_target) { + Assume(!coin_selection_params.m_subtract_fee_outputs); + return util::Error{strprintf(_("The total exceeds your balance when the %s transaction fee is included."), FormatMoney(selection_target - recipients_sum))}; + } + } + + // General failure description + return util::Error{_("Insufficient funds")}; } const SelectionResult& result = *select_coins_res; TRACEPOINT(coin_selection, selected_coins, diff --git a/src/wallet/spend.h b/src/wallet/spend.h index b10317f49a0..75090b308c3 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -58,14 +58,18 @@ struct CoinsResult { void Shuffle(FastRandomContext& rng_fast); void Add(OutputType type, const COutput& out); - CAmount GetTotalAmount() { return total_amount; } - std::optional GetEffectiveTotalAmount() {return total_effective_amount; } + CAmount GetTotalAmount() const { return total_amount; } + std::optional GetEffectiveTotalAmount() const { return total_effective_amount; } + // Returns the appropriate total based on whether fees are being subtracted from outputs + std::optional GetAppropriateTotal(bool subtract_fee_outputs) const { + return subtract_fee_outputs ? total_amount : total_effective_amount; + } private: /** Sum of all available coins raw value */ CAmount total_amount{0}; /** Sum of all available coins effective value (each output value minus fees required to spend it) */ - std::optional total_effective_amount{0}; + std::optional total_effective_amount; }; struct CoinFilterParams { @@ -152,31 +156,11 @@ util::Result AttemptSelection(interfaces::Chain& chain, const C */ util::Result ChooseSelectionResult(interfaces::Chain& chain, const CAmount& nTargetValue, Groups& groups, const CoinSelectionParams& coin_selection_params); -// User manually selected inputs that must be part of the transaction -struct PreSelectedInputs -{ - std::set> coins; - // If subtract fee from outputs is disabled, the 'total_amount' - // will be the sum of each output effective value - // instead of the sum of the outputs amount - CAmount total_amount{0}; - - void Insert(const COutput& output, bool subtract_fee_outputs) - { - if (subtract_fee_outputs) { - total_amount += output.txout.nValue; - } else { - total_amount += output.GetEffectiveValue(); - } - coins.insert(std::make_shared(output)); - } -}; - /** * Fetch and validate coin control selected inputs. * Coins could be internal (from the wallet) or external. */ -util::Result FetchSelectedInputs(const CWallet& wallet, const CCoinControl& coin_control, +util::Result FetchSelectedInputs(const CWallet& wallet, const CCoinControl& coin_control, const CoinSelectionParams& coin_selection_params) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); /** @@ -198,7 +182,7 @@ util::Result AutomaticCoinSelection(const CWallet& wallet, Coin * Select all coins from coin_control, and if coin_control 'm_allow_other_inputs=true', call 'AutomaticCoinSelection' to * select a set of coins such that nTargetValue - pre_set_inputs.total_amount is met. */ -util::Result SelectCoins(const CWallet& wallet, CoinsResult& available_coins, const PreSelectedInputs& pre_set_inputs, +util::Result SelectCoins(const CWallet& wallet, CoinsResult& available_coins, const CoinsResult& pre_set_inputs, const CAmount& nTargetValue, const CCoinControl& coin_control, const CoinSelectionParams& coin_selection_params) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet); diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index f7354e480e1..7f55d76585b 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -30,8 +30,6 @@ BOOST_FIXTURE_TEST_SUITE(coinselector_tests, WalletTestingSetup) // we repeat those tests this many times and only complain if all iterations of the test fail #define RANDOM_REPEATS 5 -typedef std::set> CoinSet; - static const CoinEligibilityFilter filter_standard(1, 6, 0); static const CoinEligibilityFilter filter_confirmed(1, 1, 0); static const CoinEligibilityFilter filter_standard_extra(6, 6, 0); @@ -117,7 +115,7 @@ static bool EquivalentResult(const SelectionResult& a, const SelectionResult& b) /** Check if this selection is equal to another one. Equal means same inputs (i.e same value and prevout) */ static bool EqualResult(const SelectionResult& a, const SelectionResult& b) { - std::pair ret = std::mismatch(a.GetInputSet().begin(), a.GetInputSet().end(), b.GetInputSet().begin(), + std::pair ret = std::mismatch(a.GetInputSet().begin(), a.GetInputSet().end(), b.GetInputSet().begin(), [](const std::shared_ptr& a, const std::shared_ptr& b) { return a->outpoint == b->outpoint; }); @@ -224,8 +222,8 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) coin_control.m_allow_other_inputs = true; COutput select_coin = available_coins.All().at(0); coin_control.Select(select_coin.outpoint); - PreSelectedInputs selected_input; - selected_input.Insert(select_coin, coin_selection_params_bnb.m_subtract_fee_outputs); + CoinsResult selected_input; + selected_input.Add(OutputType::BECH32, select_coin); available_coins.Erase({available_coins.coins[OutputType::BECH32].begin()->outpoint}); LOCK(wallet->cs_wallet); @@ -255,8 +253,8 @@ BOOST_AUTO_TEST_CASE(bnb_search_test) coin_control.m_allow_other_inputs = true; COutput select_coin = available_coins.All().at(1); // pre select 9 coin coin_control.Select(select_coin.outpoint); - PreSelectedInputs selected_input; - selected_input.Insert(select_coin, coin_selection_params_bnb.m_subtract_fee_outputs); + CoinsResult selected_input; + selected_input.Add(OutputType::BECH32, select_coin); available_coins.Erase({(++available_coins.coins[OutputType::BECH32].begin())->outpoint}); const auto result13 = SelectCoins(*wallet, available_coins, selected_input, 10 * CENT, coin_control, coin_selection_params_bnb); BOOST_CHECK(EquivalentResult(expected_result, *result13)); @@ -1292,7 +1290,7 @@ static util::Result select_coins(const CAmount& target, const C return result; } -static bool has_coin(const CoinSet& set, CAmount amount) +static bool has_coin(const OutputSet& set, CAmount amount) { return std::any_of(set.begin(), set.end(), [&](const auto& coin) { return coin->GetEffectiveValue() == amount; }); } diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index 86ebd233ee3..084777f8490 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -68,7 +68,7 @@ static CAmount CreateCoins(FuzzedDataProvider& fuzzed_data_provider, std::vector static SelectionResult ManualSelection(std::vector& utxos, const CAmount& total_amount, const bool& subtract_fee_outputs) { SelectionResult result(total_amount, SelectionAlgorithm::MANUAL); - std::set> utxo_pool; + OutputSet utxo_pool; for (const auto& utxo : utxos) { utxo_pool.insert(std::make_shared(utxo)); } @@ -445,7 +445,7 @@ void FuzzCoinSelectionAlgorithm(std::span buffer) { std::vector utxos; CAmount new_total_balance{CreateCoins(fuzzed_data_provider, utxos, coin_params, next_locktime)}; if (new_total_balance > 0) { - std::set> new_utxo_pool; + OutputSet new_utxo_pool; for (const auto& utxo : utxos) { new_utxo_pool.insert(std::make_shared(utxo)); } @@ -462,7 +462,7 @@ void FuzzCoinSelectionAlgorithm(std::span buffer) { auto manual_selection{ManualSelection(manual_inputs, manual_balance, coin_params.m_subtract_fee_outputs)}; if (result) { const CAmount old_target{result->GetTarget()}; - const std::set> input_set{result->GetInputSet()}; + const OutputSet input_set{result->GetInputSet()}; const int old_weight{result->GetWeight()}; result->Merge(manual_selection); assert(result->GetInputSet().size() == input_set.size() + manual_inputs.size()); diff --git a/test/functional/wallet_bumpfee.py b/test/functional/wallet_bumpfee.py index 044ddc968b8..1a30f0c3cde 100755 --- a/test/functional/wallet_bumpfee.py +++ b/test/functional/wallet_bumpfee.py @@ -801,7 +801,7 @@ def test_no_more_inputs_fails(self, rbf_node, dest_address): self.generatetoaddress(rbf_node, 1, dest_address) # spend all funds, no change output rbfid = rbf_node.sendall(recipients=[rbf_node.getnewaddress()])['txid'] - assert_raises_rpc_error(-4, "Unable to create transaction. Insufficient funds", rbf_node.bumpfee, rbfid) + assert_raises_rpc_error(-4, "Unable to create transaction. The total exceeds your balance when the 0.00001051 transaction fee is included.", rbf_node.bumpfee, rbfid) self.clear_mempool() diff --git a/test/functional/wallet_fundrawtransaction.py b/test/functional/wallet_fundrawtransaction.py index 3577a2f4177..de0aafc6047 100755 --- a/test/functional/wallet_fundrawtransaction.py +++ b/test/functional/wallet_fundrawtransaction.py @@ -155,6 +155,7 @@ class RawTransactionsTest(BitcoinTestFramework): self.test_input_confs_control() self.test_duplicate_outputs() self.test_watchonly_cannot_grind_r() + self.test_cannot_cover_fees() def test_duplicate_outputs(self): self.log.info("Test deserializing and funding a transaction with duplicate outputs") @@ -1456,7 +1457,8 @@ class RawTransactionsTest(BitcoinTestFramework): # To test this does not happen, we subtract 202 sats from the input value. If working correctly, this should # fail with insufficient funds rather than bitcoind asserting. rawtx = w.createrawtransaction(inputs=[], outputs=[{self.nodes[0].getnewaddress(address_type="bech32"): 1 - 0.00000202}]) - assert_raises_rpc_error(-4, "Insufficient funds", w.fundrawtransaction, rawtx, fee_rate=1.85) + expected_err_msg = "The total exceeds your balance when the 0.00000078 transaction fee is included." + assert_raises_rpc_error(-4, expected_err_msg, w.fundrawtransaction, rawtx, fee_rate=1.85) def test_input_confs_control(self): self.nodes[0].createwallet("minconf") @@ -1542,5 +1544,45 @@ class RawTransactionsTest(BitcoinTestFramework): watchonly_funded = watchonly.fundrawtransaction(hexstring=tx, fee_rate=10) assert_greater_than(watchonly_funded["fee"], funded["fee"]) + def test_cannot_cover_fees(self): + self.log.info("Test error message when transaction amount exceeds available balance when fees are included") + default_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + + self.nodes[1].createwallet("cannot_cover_fees") + wallet = self.nodes[1].get_wallet_rpc("cannot_cover_fees") + + # Set up wallet with 2 utxos: 0.3 BTC and 0.15 BTC + default_wallet.sendtoaddress(wallet.getnewaddress(), 0.3) + txid2 = default_wallet.sendtoaddress(wallet.getnewaddress(), 0.15) + self.generate(self.nodes[0], 1) + vout2 = next(utxo["vout"] for utxo in wallet.listunspent() if utxo["txid"] == txid2) + amount_with_fee_err_msg = "The total exceeds your balance when the {} transaction fee is included." + + self.log.info("Test without preselected inputs") + self.log.info("Attempt to send 0.45 BTC without SFFO") + rawtx = wallet.createrawtransaction(inputs=[], outputs=[{default_wallet.getnewaddress(): 0.45}]) + assert_raises_rpc_error(-4, amount_with_fee_err_msg.format("0.00000042"), wallet.fundrawtransaction, rawtx, options={"fee_rate":1}) + + self.log.info("Send 0.45 BTC with SFFO") + wallet.fundrawtransaction(rawtx, options={"subtractFeeFromOutputs":[0]}) + + self.log.info("Attempt to send 0.45 BTC by restricting coin selection with minconf=6") + assert_raises_rpc_error(-4, "Insufficient funds", wallet.fundrawtransaction, rawtx, options={"minconf":6}) + + self.log.info("Test with preselected inputs") + self.log.info("Attempt to send 0.45 BTC preselecting 0.15 BTC utxo") + rawtx = wallet.createrawtransaction(inputs=[{"txid": txid2, "vout": vout2}], outputs=[{default_wallet.getnewaddress(): 0.45}]) + assert_raises_rpc_error(-4, amount_with_fee_err_msg.format("0.00000042"), wallet.fundrawtransaction, rawtx, options={"fee_rate":1}) + + self.log.info("Send 0.45 BTC preselecting 0.15 BTC utxo with SFFO") + wallet.fundrawtransaction(hexstring=rawtx, options={"subtractFeeFromOutputs":[0]}) + + self.log.info("Attempt to send 0.15 BTC using only the 0.15 BTC preselected utxo") + rawtx = wallet.createrawtransaction(inputs=[{"txid": txid2, "vout": vout2}], outputs=[{default_wallet.getnewaddress(): 0.15}]) + assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, wallet.fundrawtransaction, rawtx, options={"fee_rate":1, "add_inputs":False}) + self.log.info("Send 0.15 BTC using only the 0.15 BTC preselected utxo with SFFO") + wallet.fundrawtransaction(hexstring=rawtx, options={"subtractFeeFromOutputs":[0], "add_inputs":False}) + + if __name__ == '__main__': RawTransactionsTest(__file__).main()