Repository: bitcoin/bitcoin (language: cpp)

Issue:
Plumb "too-long-mempool-chain" to RPC error for send/sendtoaddress
**Is your feature request related to a problem? Please describe.**

If coin selection fails as part of `send` or `sendtoaddress`, the error message returned to the RPC client is `Insufficient funds`, even if the actual cause is due to `too-long-mempool-chain`.  This is likely to alarm users unnecessarily, as they will believe this means their funds have permanently vanished rather than being temporarily inaccessible.

**Describe the solution you'd like**

If coin selection fails due to `too-long-mempool-chain`, the `send` and `sendtoaddress` methods should pass that error back to the RPC client, instead of `Insufficient funds`.

**Describe alternatives you've considered**

It would be even nicer from a UX standpoint if the limit on mempool chain length were removed, but AIUI [that's a DoS vector](https://github.com/bitcoin/bitcoin/issues/9752#issuecomment-281464014), so is not going to happen.

**Additional context**

Apparently this is [already done for `sendrawtransaction`](https://github.com/bitcoin/bitcoin/pull/10015#issuecomment-288597075).  The discussion in https://github.com/bitcoin/bitcoin/pull/10015 is relevant (it appears that the PR was withdrawn for reasons unrelated to my request).



Discussion / hints:
The relevant code seems to be here: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpc/spend.cpp#L73-L76
Seems to me that "insufficient funds" is the correct error message here if you cannot construct a transaction that the mempool would accept. Similarly, you may get "insufficient funds" if your coins are premature or nonfinal. If a user is interested in a more detailed breakdown of their coins, including whether things are spendable / safe to spend, they might want to use something like `listunspent`.
> Seems to me that "insufficient funds" is the correct error message here if you cannot construct a transaction that the mempool would accept. Similarly, you may get "insufficient funds" if your coins are premature or nonfinal. If a user is interested in a more detailed breakdown of their coins, including whether things are spendable / safe to spend, they might want to use something like `listunspent`.

@glozow Why do you think the `too-long-mempool-chain` error code exists if it shouldn't be used here?
`too-long-mempool-chain` corresponds to the reason a transaction is rejected from mempool. It's helpful, for example, to debug a transaction you submitted through `sendrawtransaction` or `testmempoolaccept`. It's also logged for rejected p2p transactions.
@glozow Okay, so given your description, I don't understand why that code wouldn't be intuitively applicable to `send` when the method fails because the transaction would be rejected from the mempool.  The status quo is confusing to nontechnical users (source: personal experience providing tech support to users who think their funds have been stolen after seeing the "Insufficient funds" error code).

FWIW I agree with @achow101 in https://github.com/bitcoin/bitcoin/pull/24502#issuecomment-1061742337 : 

> I think "insufficient funds" is too generic of an error message for coin selection failures as there are a ton of different reasons why coin selection might fail. I would like to have more granular error messages

We can have errors that contain more explanation than just a few words. For example, this error could be "Insufficient funds: Too long mempool chain. Creating this transaction would require creating a chain of unconfirmed transaction that will currently be rejected by the mempool."
> We can have errors that contain more explanation than just a few words. For example, this error could be "Insufficient funds: Too long mempool chain. Creating this transaction would require creating a chain of unconfirmed transaction that will currently be rejected by the mempool."

@achow101 I have no objection to this suggestion.
> "Insufficient funds: Too long mempool chain. Creating this transaction would require creating a chain of unconfirmed transaction that will currently be rejected by the mempool."

I might prefer something like "insufficient funds: unconfirmed UTXOs are available, but spending them creates a chain of transactions that will be rejected by the mempool. These funds will become available when the transaction confirms" or something.

In general, I'm not saying I'm against more specific error messages. I'm saying that, since the user was not responsible for creating the transaction in the first place, it seems unhelpful to tell them the transaction was too-long-mempool-chain, as they can't do anything differently to change this. The information that would be helpful to them is "you have money, but it's in unconfirmed UTXOs and I can't create a valid transaction right now."

Fix patch:
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<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;
 
@@ -427,11 +428,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;
@@ -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<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(); }
 
@@ -692,7 +708,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).
@@ -711,8 +744,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 patch:
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()