diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 85a88e2a6a61..fa0dc01d15a7 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -412,7 +412,8 @@ static bool isGroupISLocked(const OutputGroup& group, interfaces::Chain& chain) 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; @@ -430,11 +431,14 @@ FilteredOutputGroups GroupOutputs(const CWallet& wallet, bool isISLocked = isGroupISLocked(group, wallet.chain()); // 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, isISLocked)) 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; @@ -499,6 +503,7 @@ FilteredOutputGroups GroupOutputs(const CWallet& wallet, bool isISLocked = isGroupISLocked(group, wallet.chain()); // 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, isISLocked)) continue; @@ -511,7 +516,10 @@ 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; } + // The positive-only groups are a subset of the mixed ones, don't record them twice + if (!accepted && !positive_only) ret_discarded_groups.emplace_back(group); } } }; @@ -522,6 +530,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(); } @@ -699,7 +716,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; total_amount < 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). @@ -718,8 +752,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; diff --git a/test/functional/wallet_create_tx.py b/test/functional/wallet_create_tx.py index 994ed3d011bf..30ede9003f4a 100755 --- a/test/functional/wallet_create_tx.py +++ b/test/functional/wallet_create_tx.py @@ -26,6 +26,8 @@ def run_test(self): self.test_anti_fee_sniping() self.test_tx_size_too_large() self.test_compact_size_counts() + self.test_create_too_long_mempool_chain() + self.test_too_long_mempool_chain_avoid_partial_spends() def test_anti_fee_sniping(self): self.log.info('Check that we have some (old) blocks and that anti-fee-sniping is disabled') @@ -109,6 +111,57 @@ def check_fee(txid, num_inputs, num_outputs): self.generate(node, 1) wallet.unloadwallet() + 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 + + self.nodes[0].syncwithvalidationinterfacequeue() + 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() + + def test_too_long_mempool_chain_avoid_partial_spends(self): + self.log.info('Check that a discarded too-long-chain coin is not counted twice with avoidpartialspends') + df_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name) + + # avoid_reuse implies avoidpartialspends + self.nodes[0].createwallet(wallet_name="aps", avoid_reuse=True) + aps_wallet = self.nodes[0].get_wallet_rpc("aps") + + df_wallet.sendtoaddress(aps_wallet.getnewaddress(), 0.3) + self.generate(self.nodes[0], 1) + + # Spend the 0.3 coin to ourselves until it hits the ancestor limit. The chain is from us, so + # it is trusted and available for selection, but too long to be eligible, so it gets discarded. + for _ in range(25): + txid = aps_wallet.sendall([aps_wallet.getnewaddress()])['txid'] + assert_equal(self.nodes[0].getmempoolentry(txid)['ancestorcount'], 25) + + # Add a confirmed 0.7 coin at another address, leaving the chain in the mempool + txid = df_wallet.sendtoaddress(aps_wallet.getnewaddress(), 0.7) + self.generateblock(self.nodes[0], output=df_wallet.getnewaddress(), transactions=[txid]) + + # The confirmed 0.7 coin alone covers this payment + aps_wallet.sendtoaddress(df_wallet.getnewaddress(), 0.65) + + aps_wallet.unloadwallet() + if __name__ == '__main__': CreateTxWalletTest().main()