From acf0119d24c9f8fae825093249a46cd38e4a3a91 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 7 Mar 2023 18:47:28 -0300 Subject: [PATCH 1/2] wallet: return error msg for too-long-mempool-chain failure We currently return "Insufficient funds" which doesn't really describe what went wrong; the tx creation failed because of a long-mempool-chain, not because of a lack of funds. Also, return early from Coin Selection if the sum of the discarded coins decreases the available balance below the target amount. --- src/wallet/spend.cpp | 46 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 42 insertions(+), 4 deletions(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index a8ecce119aff5..60af9f9c66943 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -407,7 +407,8 @@ std::map> ListCoins(const CWallet& wallet) FilteredOutputGroups GroupOutputs(const CWallet& wallet, const CoinsResult& coins, const CoinSelectionParams& coin_sel_params, - const std::vector& filters) + const std::vector& filters, + std::vector& ret_discarded_groups) { FilteredOutputGroups filtered_groups; @@ -427,11 +428,14 @@ FilteredOutputGroups GroupOutputs(const CWallet& wallet, group.Insert(std::make_shared(output), ancestors, descendants); // Each filter maps to a different set of groups + bool accepted = false; for (const auto& sel_filter : filters) { const auto& filter = sel_filter.filter; if (!group.EligibleForSpending(filter)) continue; filtered_groups[filter].Push(group, type, /*insert_positive=*/true, /*insert_mixed=*/true); + accepted = true; } + if (!accepted) ret_discarded_groups.emplace_back(group); } } return filtered_groups; @@ -497,6 +501,7 @@ FilteredOutputGroups GroupOutputs(const CWallet& wallet, const OutputGroup& group = *group_it; // Each filter maps to a different set of groups + bool accepted = false; for (const auto& sel_filter : filters) { const auto& filter = sel_filter.filter; if (!group.EligibleForSpending(filter)) continue; @@ -509,7 +514,9 @@ FilteredOutputGroups GroupOutputs(const CWallet& wallet, OutputType type = script.second; // Either insert the group into the positive-only groups or the mixed ones. filtered_groups[filter].Push(group, type, positive_only, /*insert_mixed=*/!positive_only); + accepted = true; } + if (!accepted) ret_discarded_groups.emplace_back(group); } } }; @@ -520,6 +527,15 @@ FilteredOutputGroups GroupOutputs(const CWallet& wallet, return filtered_groups; } +FilteredOutputGroups GroupOutputs(const CWallet& wallet, + const CoinsResult& coins, + const CoinSelectionParams& params, + const std::vector& filters) +{ + std::vector unused; + return GroupOutputs(wallet, coins, params, filters, unused); +} + // Returns true if the result contains an error and the message is not empty static bool HasErrorMsg(const util::Result& res) { return !util::ErrorString(res).empty(); } @@ -692,7 +708,24 @@ util::Result AutomaticCoinSelection(const CWallet& wallet, Coin } // Group outputs and map them by coin eligibility filter - FilteredOutputGroups filtered_groups = GroupOutputs(wallet, available_coins, coin_selection_params, ordered_filters); + std::vector discarded_groups; + FilteredOutputGroups filtered_groups = GroupOutputs(wallet, available_coins, coin_selection_params, ordered_filters, discarded_groups); + + // Check if we still have enough balance after applying filters (some coins might be discarded) + CAmount total_discarded = 0; + CAmount total_unconf_long_chain = 0; + for (const auto& group : discarded_groups) { + total_discarded += group.GetSelectionAmount(); + if (group.m_ancestors >= max_ancestors || group.m_descendants >= max_descendants) total_unconf_long_chain += group.GetSelectionAmount(); + } + + if (CAmount total_amount = available_coins.GetTotalAmount() - total_discarded < value_to_select) { + // Special case, too-long-mempool cluster. + if (total_amount + total_unconf_long_chain > value_to_select) { + return util::Result({_("Unconfirmed UTXOs are available, but spending them creates a chain of transactions that will be rejected by the mempool")}); + } + return util::Result(util::Error()); // General "Insufficient Funds" + } // Walk-through the filters until the solution gets found. // If no solution is found, return the first detailed error (if any). @@ -711,8 +744,13 @@ util::Result AutomaticCoinSelection(const CWallet& wallet, Coin if (HasErrorMsg(res)) res_detailed_errors.emplace_back(res); } } - // Coin Selection failed. - return res_detailed_errors.empty() ? util::Result(util::Error()) : res_detailed_errors.front(); + + // Return right away if we have a detailed error + if (!res_detailed_errors.empty()) return res_detailed_errors.front(); + + + // General "Insufficient Funds" + return util::Result(util::Error()); }(); return res; From f3221d373a8623fe4808e00c7a92fbb6e0d6419b Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 7 Mar 2023 18:48:21 -0300 Subject: [PATCH 2/2] test: add wallet too-long-mempool-chain error coverage --- test/functional/wallet_create_tx.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/test/functional/wallet_create_tx.py b/test/functional/wallet_create_tx.py index 2d9bb38fcc2e5..11c82e15b7e4d 100755 --- a/test/functional/wallet_create_tx.py +++ b/test/functional/wallet_create_tx.py @@ -32,6 +32,7 @@ def run_test(self): self.test_anti_fee_sniping() self.test_tx_size_too_large() + self.test_create_too_long_mempool_chain() def test_anti_fee_sniping(self): self.log.info('Check that we have some (old) blocks and that anti-fee-sniping is disabled') @@ -80,6 +81,30 @@ def test_tx_size_too_large(self): ) self.nodes[0].settxfee(0) + def test_create_too_long_mempool_chain(self): + self.log.info('Check too-long mempool chain error') + df_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + + self.nodes[0].createwallet("too_long") + test_wallet = self.nodes[0].get_wallet_rpc("too_long") + + tx_data = df_wallet.send(outputs=[{test_wallet.getnewaddress(): 25}], options={"change_position": 0}) + txid = tx_data['txid'] + vout = 1 + + options = {"change_position": 0, "add_inputs": False} + for i in range(1, 25): + options['inputs'] = [{'txid': txid, 'vout': vout}] + tx_data = test_wallet.send(outputs=[{test_wallet.getnewaddress(): 25 - i}], options=options) + txid = tx_data['txid'] + + # Sending one more chained transaction will fail + options = {"minconf": 0, "include_unsafe": True, 'add_inputs': True} + assert_raises_rpc_error(-4, "Unconfirmed UTXOs are available, but spending them creates a chain of transactions that will be rejected by the mempool", + test_wallet.send, outputs=[{test_wallet.getnewaddress(): 0.3}], options=options) + + test_wallet.unloadwallet() + if __name__ == '__main__': CreateTxWalletTest().main()