refactor: simplify CreateRecipients

Move validation logic out of `CreateRecipients` and instead take the
already validated outputs from `ParseOutputs` as an input.

Move SFFO parsing out of `CreateRecipients` into a new function,
`InterpretSubtractFeeFromOutputsInstructions`. This takes the SFFO instructions
from `sendmany` and `sendtoaddress` and turns them into a set of integers.
In a later commit, we will also move the SFFO parsing logic from
`FundTransaction` into this function.

Worth noting: a user can pass duplicate addresses and addresses that dont exist
in the transaction outputs as SFFO args to `sendmany` and `sendtoaddress`
without triggering a warning. This behavior is preserved in to keep this commit
strictly a refactor.

src/rpc/rawtransaction_util.h

@@ -10,6 +10,8 @@
 #include <map>
 #include <string>
 #include <optional>
+#include <addresstype.h>
+#include <consensus/amount.h>
 
 struct bilingual_str;
 class FillableSigningProvider;

src/wallet/rpc/spend.cpp

@@ -24,34 +24,14 @@
 
 
 namespace wallet {
-std::vector<CRecipient> CreateRecipients(const UniValue& address_amounts, const UniValue& subtract_fee_outputs)
+std::vector<CRecipient> CreateRecipients(const std::vector<std::pair<CTxDestination, CAmount>>& outputs, const std::set<int>& subtract_fee_outputs)
 {
     std::vector<CRecipient> recipients;
-    std::set<CTxDestination> destinations;
-    int i = 0;
-    for (const std::string& address: address_amounts.getKeys()) {
-        CTxDestination dest = DecodeDestination(address);
-        if (!IsValidDestination(dest)) {
-            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, std::string("Invalid Bitcoin address: ") + address);
-        }
-
-        if (destinations.count(dest)) {
-            throw JSONRPCError(RPC_INVALID_PARAMETER, std::string("Invalid parameter, duplicated address: ") + address);
-        }
-        destinations.insert(dest);
-
-        CAmount amount = AmountFromValue(address_amounts[i++]);
-
-        bool subtract_fee = false;
-        for (unsigned int idx = 0; idx < subtract_fee_outputs.size(); idx++) {
-            const UniValue& addr = subtract_fee_outputs[idx];
-            if (addr.get_str() == address) {
-                subtract_fee = true;
-            }
-        }
-
-        CRecipient recipient = {dest, amount, subtract_fee};
+    int idx{0};
+    for (const auto& [destination, amount] : outputs) {
+        CRecipient recipient = {destination, amount, subtract_fee_outputs.count(idx) == 1};
         recipients.push_back(recipient);
+        idx++;
     }
     return recipients;
 }
@@ -78,6 +58,36 @@ static void InterpretFeeEstimationInstructions(const UniValue& conf_target, cons
     }
 }
 
+std::set<int> InterpretSubtractFeeFromOutputInstructions(const UniValue& options, const std::vector<std::string>& destinations)
+{
+    int pos{0};
+    std::set<int> sffo_set;
+    UniValue subtract_fee_outputs(UniValue::VARR);
+    if (options.exists("subtractfeefromamount") || options.exists("subtractfeefrom")) {
+        if (options.exists("subtractfeefromamount")) {
+            if (options["subtractfeefromamount"].isBool() && options["subtractfeefromamount"].get_bool()) {
+                subtract_fee_outputs.push_back(destinations.at(0));
+            }
+        } else if (options.exists("subtractfeefrom")) {
+            if (!options["subtractfeefrom"].isNull()) {
+                subtract_fee_outputs = options["subtractfeefrom"].get_array();
+            }
+        }
+        for (const std::string& address : destinations) {
+            for (unsigned int idx = 0; idx < subtract_fee_outputs.size(); idx++) {
+                const UniValue& addr = subtract_fee_outputs[idx];
+                if (addr.get_str() == address) {
+                    sffo_set.insert(pos);
+                }
+            }
+            pos++;
+        }
+        return sffo_set;
+    }
+    // If no SFFO instructions, return an empty set
+    return sffo_set;
+}
+
 static UniValue FinishTransaction(const std::shared_ptr<CWallet> pwallet, const UniValue& options, const CMutableTransaction& rawTx)
 {
     // Make a blank psbt
@@ -277,11 +287,6 @@ RPCHelpMan sendtoaddress()
     if (!request.params[3].isNull() && !request.params[3].get_str().empty())
         mapValue["to"] = request.params[3].get_str();
 
-    bool fSubtractFeeFromAmount = false;
-    if (!request.params[4].isNull()) {
-        fSubtractFeeFromAmount = request.params[4].get_bool();
-    }
-
     CCoinControl coin_control;
     if (!request.params[5].isNull()) {
         coin_control.m_signal_bip125_rbf = request.params[5].get_bool();
@@ -298,12 +303,12 @@ RPCHelpMan sendtoaddress()
     UniValue address_amounts(UniValue::VOBJ);
     const std::string address = request.params[0].get_str();
     address_amounts.pushKV(address, request.params[1]);
-    UniValue subtractFeeFromAmount(UniValue::VARR);
-    if (fSubtractFeeFromAmount) {
-        subtractFeeFromAmount.push_back(address);
-    }
-
-    std::vector<CRecipient> recipients = CreateRecipients(address_amounts, subtractFeeFromAmount);
+    UniValue subtractFeeFromAmount(UniValue::VOBJ);
+    subtractFeeFromAmount.pushKV("subtractfeefromamount", request.params[4]);
+    std::vector<CRecipient> recipients = CreateRecipients(
+            ParseOutputs(address_amounts),
+            InterpretSubtractFeeFromOutputInstructions(subtractFeeFromAmount, address_amounts.getKeys())
+    );
     const bool verbose{request.params[10].isNull() ? false : request.params[10].get_bool()};
 
     return SendMoney(*pwallet, coin_control, recipients, mapValue, verbose);
@@ -387,9 +392,8 @@ RPCHelpMan sendmany()
     if (!request.params[3].isNull() && !request.params[3].get_str().empty())
         mapValue["comment"] = request.params[3].get_str();
 
-    UniValue subtractFeeFromAmount(UniValue::VARR);
-    if (!request.params[4].isNull())
-        subtractFeeFromAmount = request.params[4].get_array();
+    UniValue subtractFeeFromAmount(UniValue::VOBJ);
+    subtractFeeFromAmount.pushKV("subtractfeefrom", request.params[4]);
 
     CCoinControl coin_control;
     if (!request.params[5].isNull()) {
@@ -398,7 +402,10 @@ RPCHelpMan sendmany()
 
     SetFeeEstimateMode(*pwallet, coin_control, /*conf_target=*/request.params[6], /*estimate_mode=*/request.params[7], /*fee_rate=*/request.params[8], /*override_min_fee=*/false);
 
-    std::vector<CRecipient> recipients = CreateRecipients(sendTo, subtractFeeFromAmount);
+    std::vector<CRecipient> recipients = CreateRecipients(
+            ParseOutputs(sendTo),
+            InterpretSubtractFeeFromOutputInstructions(subtractFeeFromAmount, sendTo.getKeys())
+    );
     const bool verbose{request.params[9].isNull() ? false : request.params[9].get_bool()};
 
     return SendMoney(*pwallet, coin_control, recipients, std::move(mapValue), verbose);