Merge bitcoin/bitcoin#28560: wallet, rpc: `FundTransaction` refactor

18ad1b9142e91cef2f5c6a693eeb2d0fbb8c517d refactor: pass CRecipient to FundTransaction (josibake)
5ad19668dbcc47486d1c18f711cea3d8a9d2e7e2 refactor: simplify `CreateRecipients` (josibake)
47353a608dc6e20e5fd2ca53850d6f9aa3240d4a refactor: remove out param from `ParseRecipients` (josibake)
f7384b921c3460c7a3cc7827a68b2c613bd98f8e refactor: move parsing to new function (josibake)
6f569ac903e5ddaac275996a5d0c31b2220b7b81 refactor: move normalization to new function (josibake)
435fe5cd96599c518e26efe444c9d94d1277996b test: add tests for fundrawtx and sendmany rpcs (josibake)

Pull request description:

  ## Motivation

  The primary motivation for this PR is to enable `FundTransaction` to take a vector of `CRecipient` objects to allow passing BIP352 silent payment addresses to RPCs that use `FundTransaction` (e.g. `send`, `walletcreatefundedpsbt`). To do that, SFFO logic needs to be moved out of `FundTransaction` so the `CRecipient` objects with the correct SFFO information can be created and then passed to `FundTransaction`.

  As a secondary motivation, this PR moves the SFFO stuff closer to the caller, making the code cleaner and easier to understand. This is done by having a single function which parses RPC inputs for SFFO and consistently using the `set<int>` method for communicating SFFO.

  I'm also not convinced we need to pass a full `CMutableTx` object to `FundTransaction`, but I'm leaving that for a follow-up PR/discussion, as its not a blocker for silent payments.

ACKs for top commit:
  S3RK:
    reACK 18ad1b9142e91cef2f5c6a693eeb2d0fbb8c517d
  josibake:
    > According to my `range-diff` nothing changed. reACK [18ad1b9](https://github.com/bitcoin/bitcoin/commit/18ad1b9142e91cef2f5c6a693eeb2d0fbb8c517d)
  achow101:
    ACK 18ad1b9142e91cef2f5c6a693eeb2d0fbb8c517d

Tree-SHA512: d61f017cf7d98489ef216475b68693fd77e7b53a26a6477dcd73e7e5ceff5036b2d21476e377839e710bb73644759d42c4f9f4b14ed96b3e56ed87b07aa6d1a7

1 files changed, 26 insertions, 11 deletions

diff --git a/src/rpc/rawtransaction_util.cpp b/src/rpc/rawtransaction_util.cpp
index c471986a44..a9e11622a7 100644
--- a/src/rpc/rawtransaction_util.cpp
+++ b/src/rpc/rawtransaction_util.cpp
@@ -70,7 +70,7 @@ void AddInputs(CMutableTransaction& rawTx, const UniValue& inputs_in, std::optio
     }
 }
 
-void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in)
+UniValue NormalizeOutputs(const UniValue& outputs_in)
 {
     if (outputs_in.isNull()) {
         throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, output argument must be non-null");
@@ -94,11 +94,15 @@ void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in)
         }
         outputs = std::move(outputs_dict);
     }
+    return outputs;
+}
 
+std::vector<std::pair<CTxDestination, CAmount>> ParseOutputs(const UniValue& outputs)
+{
     // Duplicate checking
     std::set<CTxDestination> destinations;
+    std::vector<std::pair<CTxDestination, CAmount>> parsed_outputs;
     bool has_data{false};
-
     for (const std::string& name_ : outputs.getKeys()) {
         if (name_ == "data") {
             if (has_data) {
@@ -106,11 +110,12 @@ void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in)
             }
             has_data = true;
             std::vector<unsigned char> data = ParseHexV(outputs[name_].getValStr(), "Data");
-
-            CTxOut out(0, CScript() << OP_RETURN << data);
-            rawTx.vout.push_back(out);
+            CTxDestination destination{CNoDestination{CScript() << OP_RETURN << data}};
+            CAmount amount{0};
+            parsed_outputs.emplace_back(destination, amount);
         } else {
-            CTxDestination destination = DecodeDestination(name_);
+            CTxDestination destination{DecodeDestination(name_)};
+            CAmount amount{AmountFromValue(outputs[name_])};
             if (!IsValidDestination(destination)) {
                 throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, std::string("Invalid Bitcoin address: ") + name_);
             }
@@ -118,13 +123,23 @@ void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in)
             if (!destinations.insert(destination).second) {
                 throw JSONRPCError(RPC_INVALID_PARAMETER, std::string("Invalid parameter, duplicated address: ") + name_);
             }
+            parsed_outputs.emplace_back(destination, amount);
+        }
+    }
+    return parsed_outputs;
+}
+
+void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in)
+{
+    UniValue outputs(UniValue::VOBJ);
+    outputs = NormalizeOutputs(outputs_in);
 
-            CScript scriptPubKey = GetScriptForDestination(destination);
-            CAmount nAmount = AmountFromValue(outputs[name_]);
+    std::vector<std::pair<CTxDestination, CAmount>> parsed_outputs = ParseOutputs(outputs);
+    for (const auto& [destination, nAmount] : parsed_outputs) {
+        CScript scriptPubKey = GetScriptForDestination(destination);
 
-            CTxOut out(nAmount, scriptPubKey);
-            rawTx.vout.push_back(out);
-        }
+        CTxOut out(nAmount, scriptPubKey);
+        rawTx.vout.push_back(out);
     }
 }