From 34cdcc2c6e6843d36fc69dc1fc1eb032085a6504 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Thu, 23 Mar 2023 15:44:42 +0000 Subject: [PATCH 1/4] Merge bitcoin/bitcoin#24845: wallet: return error msg for "too-long-mempool-chain" f3221d373a8623fe4808e00c7a92fbb6e0d6419b test: add wallet too-long-mempool-chain error coverage (furszy) acf0119d24c9f8fae825093249a46cd38e4a3a91 wallet: return error msg for too-long-mempool-chain failure (furszy) Pull request description: Fixes #23144. We currently return a general "Insufficient funds" from Coin Selection when we actually skipped unconfirmed UTXOs that surpassed the mempool ancestors limit. This PR make the error clearer by returning: "Unconfirmed UTXOs are available, but spending them creates a chain of transactions that will be rejected by the mempool" Also, added an early return from Coin Selection if the sum of the discarded coins decreases the available balance below the target amount. ACKs for top commit: achow101: ACK f3221d373a8623fe4808e00c7a92fbb6e0d6419b S3RK: Code review ACK f3221d373a8623fe4808e00c7a92fbb6e0d6419b Xekyo: ACK f3221d373a8623fe4808e00c7a92fbb6e0d6419b Tree-SHA512: 13e5824b75ac302280ff894560a4ebf32a74f32fe49ef8281f2bc99c0104b92cef33d3b143c6e131f3a07eafe64533af7fc60abff585142c134b9d6e531a6a66 Co-authored-by: glozow --- src/wallet/spend.cpp | 46 ++++++++++++++++++++++++++--- test/functional/wallet_create_tx.py | 25 ++++++++++++++++ 2 files changed, 67 insertions(+), 4 deletions(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 85a88e2a6a61..42e1a115d2af 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,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); } } }; @@ -522,6 +529,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 +715,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). @@ -718,8 +751,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..1bed2d76dc40 100755 --- a/test/functional/wallet_create_tx.py +++ b/test/functional/wallet_create_tx.py @@ -26,6 +26,7 @@ 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() 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 +110,30 @@ 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 + + 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() From 34e8991f2dd023a4a99e8b36daf9b6e7069ba990 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Tue, 28 Mar 2023 15:18:59 +0100 Subject: [PATCH 2/4] Merge bitcoin/bitcoin#27318: test: wallet_create_tx.py fix race 8aab5157c55249c9023ae4e9654f5d42aaa4f314 test: wallet_create_tx.py fix race (furszy) Pull request description: Fixes #27316 Because wallets are internally synchronized through the validation interface, and the interface dispatches events on a worker thread, it is possible for a transaction created by the first wallet to not arrive to the second wallet before the second wallet attempts to use one of its outputs. This is because we do not wait for the `BroadcastTransaction` callback during the wallet's "submit to mempool" process. To address this in the tests, we need to manually sync the validation queue. ACKs for top commit: josibake: ACK https://github.com/bitcoin/bitcoin/pull/27318/commits/8aab5157c55249c9023ae4e9654f5d42aaa4f314 theStack: ACK 8aab5157c55249c9023ae4e9654f5d42aaa4f314 Tree-SHA512: 76364370ab292a5c3ea1ed61cd353fc626a9e9cd6ce18464c24da1b3dcb34b65006e2bc42b84bbd25af03f9449231990bf789504728972db3217b569099eb309 Co-authored-by: fanquake --- test/functional/wallet_create_tx.py | 1 + 1 file changed, 1 insertion(+) diff --git a/test/functional/wallet_create_tx.py b/test/functional/wallet_create_tx.py index 1bed2d76dc40..8612a5048035 100755 --- a/test/functional/wallet_create_tx.py +++ b/test/functional/wallet_create_tx.py @@ -121,6 +121,7 @@ def test_create_too_long_mempool_chain(self): 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}] From 69c6af3aa5233b1da9f4d141dcb04711ca255bf2 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Mon, 23 Mar 2026 17:05:30 -0700 Subject: [PATCH 3/4] Merge bitcoin/bitcoin#34888: wallet: fix amount computed as boolean in coin selection 0026b330c4abbbbdb96e4f0c4d380d70d8e592ab wallet: fix amount computed as boolean in coin selection (furszy) Pull request description: Stumbled upon this tiny bug. This has been working by accident. The comparison is evaluated first, so `total_amount` ends up holding a boolean instead of the actual amount. It can be verified by adding the value to the returned error message and running the `wallet_create_tx.py` test. Note: I assume something like `-Wparentheses` in CI (or similar) should help us catching similar issues elsewhere. ACKs for top commit: fjahr: utACK 0026b330c4abbbbdb96e4f0c4d380d70d8e592ab achow101: ACK 0026b330c4abbbbdb96e4f0c4d380d70d8e592ab andrewtoth: utACK 0026b330c4abbbbdb96e4f0c4d380d70d8e592ab luke-jr: utACK 0026b330c4abbbbdb96e4f0c4d380d70d8e592ab Tree-SHA512: 289c1eb34e59caae0a9e6814a14e4a7ba72f26e3b26717bb3f9e60335c9c5efcebe7e5997f799752096cf91df416a82b8677a9900e5ec54b6d13921d4299be96 Co-authored-by: Ava Chow --- src/wallet/spend.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 42e1a115d2af..63dc8ba4b660 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -726,7 +726,7 @@ util::Result AutomaticCoinSelection(const CWallet& wallet, Coin 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) { + 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")}); From ad9bef6db9471fd9adc0f816ef0f59798c71c8f6 Mon Sep 17 00:00:00 2001 From: Konstantin Akimov Date: Thu, 24 Sep 2026 14:20:33 -0700 Subject: [PATCH 4/4] partial Merge bitcoin/bitcoin#36284: wallet: don't double discard output groups with avoidpartialspends BACKPORT NOTE: missing changes in src/wallet/test/group_outputs_tests.cpp and src/wallet/spend.h 719324977a41ab5132699d9c81032f8b2687f599 test: Check that ineligible output groups are discarded only once (Fabian Jahr) 20307f0ac0e17c6e478e2e11318dc7a9b18fc5d1 wallet: don't double count discarded output groups with avoidpartialspends (Fabian Jahr) Pull request description: With `-avoidpartialspends` or `avoid_reuse`, `GroupOutputs` puts every positive-value output into both the mixed and the positive-only map and runs `push_output_groups` on each, so a group rejected by the eligibility filters lands in `discarded_groups` twice. `AutomaticCoinSelection` then subtracts it twice and could fail with an insufficient funds error even when there would be enough confirmed coins to cover the payment. Not a problem in a default wallet but it can happen with `sendtoaddress` from an `avoid_reuse` wallet, or with `-avoidpartialspends=1`. The simplest possible fix is to record discards only from the mixed map pass, which contains every output anyway. Also adds a test that reproduces the issue. ACKs for top commit: achow101: ACK 719324977a41ab5132699d9c81032f8b2687f599 pablomartin4btc: ACK 719324977a41ab5132699d9c81032f8b2687f599 vicjuma: ACK 719324977a41ab5132699d9c81032f8b2687f599 polespinasa: code reviewed ACK 719324977a41ab5132699d9c81032f8b2687f599 Tree-SHA512: 3028f8feeacecc601aacfce757fc885b7bd25cf6ea2ad9d9b65c56b5da71096aa48d0d70ce04d80fc2b147d67109c47d190e5dc925d06c67dfcf33cf8cc715d8 Co-authored-by: Ava Chow --- src/wallet/spend.cpp | 3 ++- test/functional/wallet_create_tx.py | 27 +++++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 63dc8ba4b660..fa0dc01d15a7 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -518,7 +518,8 @@ FilteredOutputGroups GroupOutputs(const CWallet& wallet, filtered_groups[filter].Push(group, type, positive_only, /*insert_mixed=*/!positive_only); accepted = true; } - if (!accepted) ret_discarded_groups.emplace_back(group); + // 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); } } }; diff --git a/test/functional/wallet_create_tx.py b/test/functional/wallet_create_tx.py index 8612a5048035..30ede9003f4a 100755 --- a/test/functional/wallet_create_tx.py +++ b/test/functional/wallet_create_tx.py @@ -27,6 +27,7 @@ def run_test(self): 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') @@ -135,6 +136,32 @@ def test_create_too_long_mempool_chain(self): 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()