Note to reviewers: preset_inputs.total_amount will be the sum of the outputs’ effective values if "subtract fee from outputs" is disabled, but will be the nominal amount if SFFO is active. See src/wallet/spend.h:164–172

I’m wondering whether there is a bug here. It seems to me that this is assuming that the effective value is only available when SFFO is not being used, but COutput.effective_value is set and aggregated whenever the COutputs have a feerate. Does this not explicitly need to handle SFFO? I.e., maybe this needs to be something along the lines of:

good catch!, fixed. This also revealed that the optional field CoinsResult::total_effective_amount has never being optional (it was being set to 0 during construction).

Since fees are not included when SFFO, I don't think this is reachable as should available_balance is always greater than recipients_sum in that case, so I don't think this needs any special SFFO treatment. Furthermore, if it were possible to reach this with SFFO, I think the error message would be confusing and it would actually be better to return "Insufficent Funds", so this whole check could probably be guarded by an if (m_subtract_fee_outputs) to skip it when SFFO is on.

ha.. true. It seems I'm not putting much brain power here.. updated per feedback. Thanks.

re: #25269 (comment)

Furthermore, if it were possible to reach this with SFFO, I think the error message would be confusing and it would actually be better to return "Insufficent Funds", so this whole check could probably be guarded by an if (m_subtract_fee_outputs) to skip it when SFFO is on.

I don't think I follow the logic behind this but it's possible I'm misunderstanding. If user is trying to send less money than their balance, and it fails because fees are too high it would seem like an "exceeds your balance when fees are included" message (especially one that specifies what fee the wallet was targeting) would be clearer than an "insufficient funds" message. It seems like should be true independently of whether the sender or recipient is paying the fees.

So I guess I don't understand the reason for a m_subtract_fee_outputs special case here, and it would seem simpler and better to drop. I do that see in practice that effective values are ignored when SFFO is used, so it should be safe to Assume(!coin_selection_params.m_subtract_fee_outputs) here if no other code changes, but this would seem like unnecessary assumption and not something this code needs to concern itself with.

re: #25269 (comment)

Thinking about this more I can see why SFFO should make the "amount exceeds your balance when fees are included" condition harder to trigger than when SFFO is not used, because when SFFO is enabled the wallet is able to reduce the amounts recipients receive to cover any fees, whereas without SFFO the amount received needs to be fixed. But I'd still expect the (available_balance >= recipients_sum && available_effective_balance < selection_target) condition to be able to trigger when SFFO is used if there are coins are which are not economical to send. Also even if there is other code making this condition difficult or impossible to trigger when SFFO is used, I still don't see a reason this code should be checking for SFFO specially. Even with SFFO, an error message that mentions fees and includes the attempted fee would still be more descriptive and helpful than the generic "insufficient funds" message.

re: #25269 (comment)

Update: think I finally figured this out. In the SFFO case the fee exceeds balance error can still happen but the checks here will not detect it. So I think the change I would suggest would just be dropping the m_subtract_fee_outputs code and writing a more descriptive comment:

--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -1164,12 +1164,19 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
 
         // Check if we have enough balance but cannot cover the fees
         CAmount available_balance = preset_inputs.total_amount + available_coins.GetTotalAmount();
-        // Note: Since SFFO reduces existing outputs to cover fees, recipients_sum should already be
-        // strictly less than available_balance when enabled. Still, as a sanity-check, skip when SFFO is enabled.
-        if (available_balance >= recipients_sum && !coin_selection_params.m_subtract_fee_outputs) {
+        if (available_balance >= recipients_sum) {
             CAmount available_effective_balance = preset_inputs.total_amount + *available_coins.GetEffectiveTotalAmount();
             if (available_effective_balance < selection_target) {
-                return util::Error{_("The total transaction amount exceeds your balance when fees are included")};
+                // Note: this fees exceeds balance error will not currently
+                // trigger when SFFO is enabled because the effective balance
+                // will equal the original balance in this case. This is not
+                // ideal because it doesn't provide the most descriptive error
+                // message, but it's not worth making this code more complex by
+                // checking the sizes of outputs fees can be subtracted from
+                // here. Also, fees exceeding the balance should happen more
+                // rarely with SFFO given the ability to take fees from outputs.
+                Assume(!coin_selection_params.m_subtract_fee_outputs);
+                return util::Error{strprintf(_("The total exceeds your balance when the %s transaction fee is included."), FormatMoney(selection_target - recipients_sum))};
             }
         }
 

In 98b7f10 "wallet: introduce "tx amount exceeds balance when fees are included" error"

I'm pretty sure that !coin_selection_params.m_subtract_fee_outputs is not necessary as it should not be possible for SelectCoins to fail but available_balance >= recipients_sum with SFFO.

Perhaps this could be made into an Assume inside the if?

In commit "wallet: fix, make 'total_effective_amount' optional actually optional" (5687530)

I wonder if this change requires updates to any code that might be assuming total_effective_amount is not null like:

Also it's not really clear what the connection is between this change in the rest of the PR. It would help to mention some motivation in the commit description.

In commit "wallet: introduce "tx amount exceeds balance when fees are included" error" (98b7f10)

Can this error message include amount of fees that was in the original error message. It seems like knowing how much the wallet is trying to spend could help the user address the problem. It looks like equivalent amount would be selection_target - recipient_sum here.

In commit "wallet: introduce "tx amount exceeds balance when fees are included" error" (98b7f10)

Is it safe to assume GetEffectiveTotalAmount() returns not null here after previous commit? Might be better to use value_or

re: #25269 (comment)

Furthermore, if it were possible to reach this with SFFO, I think the error message would be confusing and it would actually be better to return "Insufficent Funds", so this whole check could probably be guarded by an if (m_subtract_fee_outputs) to skip it when SFFO is on.

I don't think I follow the logic behind this but it's possible I'm misunderstanding. If user is trying to send less money than their balance, and it fails because fees are too high it would seem like an "exceeds your balance when fees are included" message (especially one that specifies what fee the wallet was targeting) would be clearer than an "insufficient funds" message. It seems like should be true independently of whether the sender or recipient is paying the fees.

So I guess I don't understand the reason for a m_subtract_fee_outputs special case here, and it would seem simpler and better to drop. I do that see in practice that effective values are ignored when SFFO is used, so it should be safe to Assume(!coin_selection_params.m_subtract_fee_outputs) here if no other code changes, but this would seem like unnecessary assumption and not something this code needs to concern itself with.