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, 4 insertions, 11 deletions

diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index e9dd96d22c..e229962ca5 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -1377,18 +1377,11 @@ util::Result<CreatedTransactionResult> CreateTransaction(
     return res;
 }
 
-util::Result<CreatedTransactionResult> FundTransaction(CWallet& wallet, const CMutableTransaction& tx, std::optional<unsigned int> change_pos, bool lockUnspents, const std::set<int>& setSubtractFeeFromOutputs, CCoinControl coinControl)
+util::Result<CreatedTransactionResult> FundTransaction(CWallet& wallet, const CMutableTransaction& tx, const std::vector<CRecipient>& vecSend, std::optional<unsigned int> change_pos, bool lockUnspents, CCoinControl coinControl)
 {
-    std::vector<CRecipient> vecSend;
-
-    // Turn the txout set into a CRecipient vector.
-    for (size_t idx = 0; idx < tx.vout.size(); idx++) {
-        const CTxOut& txOut = tx.vout[idx];
-        CTxDestination dest;
-        ExtractDestination(txOut.scriptPubKey, dest);
-        CRecipient recipient = {dest, txOut.nValue, setSubtractFeeFromOutputs.count(idx) == 1};
-        vecSend.push_back(recipient);
-    }
+    // We want to make sure tx.vout is not used now that we are passing outputs as a vector of recipients.
+    // This sets us up to remove tx completely in a future PR in favor of passing the inputs directly.
+    assert(tx.vout.empty());
 
     // Set the user desired locktime
     coinControl.m_locktime = tx.nLockTime;