Merge bitcoin/bitcoin#24845: wallet: return error msg for "too-long-mempool-chain"

f3221d3 test: add wallet too-long-mempool-chain error coverage (furszy)
acf0119 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 f3221d3
  S3RK:
    Code review ACK f3221d3
  Xekyo:
    ACK f3221d3

Tree-SHA512: 13e5824b75ac302280ff894560a4ebf32a74f32fe49ef8281f2bc99c0104b92cef33d3b143c6e131f3a07eafe64533af7fc60abff585142c134b9d6e531a6a66

Author: glozow

src/wallet/spend.cpp

@@ -407,7 +407,8 @@ std::map<CTxDestination, std::vector<COutput>> ListCoins(const CWallet& wallet)
 FilteredOutputGroups GroupOutputs(const CWallet& wallet,
                           const CoinsResult& coins,
                           const CoinSelectionParams& coin_sel_params,
-                          const std::vector<SelectionFilter>& filters)
+                          const std::vector<SelectionFilter>& filters,
+                          std::vector<OutputGroup>& ret_discarded_groups)
 {
     FilteredOutputGroups filtered_groups;
 
@@ -424,11 +425,14 @@ FilteredOutputGroups GroupOutputs(const CWallet& wallet,
                 group.Insert(std::make_shared<COutput>(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;
@@ -492,6 +496,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;
@@ -504,7 +509,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);
             }
         }
     };
@@ -515,6 +522,15 @@ FilteredOutputGroups GroupOutputs(const CWallet& wallet,
     return filtered_groups;
 }
 
+FilteredOutputGroups GroupOutputs(const CWallet& wallet,
+                                  const CoinsResult& coins,
+                                  const CoinSelectionParams& params,
+                                  const std::vector<SelectionFilter>& filters)
+{
+    std::vector<OutputGroup> 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<SelectionResult>& res) { return !util::ErrorString(res).empty(); }
 
@@ -685,7 +701,24 @@ util::Result<SelectionResult> 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<OutputGroup> 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<SelectionResult>({_("Unconfirmed UTXOs are available, but spending them creates a chain of transactions that will be rejected by the mempool")});
+            }
+            return util::Result<SelectionResult>(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).
@@ -704,8 +737,13 @@ util::Result<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, Coin
                 if (HasErrorMsg(res)) res_detailed_errors.emplace_back(res);
             }
         }
-        // Coin Selection failed.
-        return res_detailed_errors.empty() ? util::Result<SelectionResult>(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<SelectionResult>(util::Error());
     }();
 
     return res;

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()