Many z_mergetoaddress updates

- add ZIP 317 support
- address review comments
- restructure `AsyncRPCOperation_mergetoaddress` (removing the just-added
  `prepare`)

doc/release-notes.md

@@ -21,24 +21,20 @@ RPC Changes
     also available for testnet and regtest nodes, in which case it does not
     return end-of-service halt information (as testnet and regtest nodes do not
     have an end-of-service halt feature.)
-- Several `z_sendmany` failures have moved from synchronous to asynchronous, so
-  while you should already be checking the async operation status, there are now
-  more cases that may trigger failure at that stage.
+- Several `z_sendmany`, `z_shieldcoinbase` and `z_mergetoaddress` failures have
+  moved from synchronous to asynchronous, so while you should already be
+  checking the async operation status, there are now more cases that may trigger
+  failure at that stage.
 - The `AllowRevealedRecipients` privacy policy is now required in order to choose a
   transparent change address for a transaction. This will only occur when the wallet
   is unable to construct the transaction without selecting funds from the transparent
   pool, so the impact of this change is that for such transactions, the user must specify
   `AllowFullyTransparent`.
+- The `z_shieldcoinbase` and `z_mergetoaddress` RPC methods now support an
+  optional privacy policy.
 - The `estimatepriority` RPC call has been removed.
 - The `priority_delta` argument to the `prioritisetransaction` RPC call now has
   no effect and must be set to a dummy value (0 or null).
-- The `z_shieldcoinbase` and `z_mergetoaddress` RPC methods no longer support
-  transfers of funds to Sprout recipients. While Sprout change may still be 
-  produced in the process of spending Sprout funds, it is no longer possible
-  to transfer funds between Sprout addresses. Also, some failures have moved 
-  from synchronous to asynchronous, so while you should already be checking 
-  the async operation status, there are now more cases that may trigger 
-  failure at that stage.
 
 Changes to Transaction Fee Selection
 ------------------------------------

qa/rpc-tests/mergetoaddress_helper.py

@@ -24,7 +24,7 @@ def assert_mergetoaddress_exception(expected_error_msg, merge_to_address_lambda)
     except Exception as e:
         fail("Expected JSONRPCException. Found %s" % repr(e))
     else:
-        fail("Expected exception: %s" % expected_error_msg)
+        fail("Expected exception: “%s”, but didn’t fail" % expected_error_msg)
 
 
 class MergeToAddressHelper:
@@ -129,9 +129,9 @@ class MergeToAddressHelper:
             "Amount out of range",
             lambda: test.nodes[0].z_mergetoaddress(self.any_zaddr_or_utxo, myzaddr, Decimal('21000000.00000001')))
 
-        # Merging will fail because fee is larger than sum of UTXOs
+        # Merging will fail because fee is larger than `-maxtxfee`
         assert_mergetoaddress_exception(
-            "Insufficient funds, have 50.00, which is less than miners fee 999.00",
+            "Fee (999.00 ZEC) is greater than the maximum fee allowed by this instance (0.10 ZEC). Run zcashd with `-maxtxfee` to adjust this limit.",
             lambda: test.nodes[0].z_mergetoaddress(self.any_zaddr_or_utxo, myzaddr, 999))
 
         # Merging will fail because transparent limit parameter must be at least 0
@@ -174,7 +174,7 @@ class MergeToAddressHelper:
         # Confirm balances and that do_not_shield_taddr containing funds of 10 was left alone
         assert_equal(test.nodes[0].getbalance(), Decimal('10'))
         assert_equal(test.nodes[0].z_getbalance(do_not_shield_taddr), Decimal('10'))
-        assert_equal(test.nodes[0].z_getbalance(myzaddr), Decimal('40') - DEFAULT_FEE)
+        assert_equal(test.nodes[0].z_getbalance(myzaddr), Decimal('40') - conventional_fee(4))
         assert_equal(test.nodes[1].getbalance(), Decimal('40'))
         assert_equal(test.nodes[2].getbalance(), Decimal('30'))
 
@@ -191,7 +191,7 @@ class MergeToAddressHelper:
         test.sync_all()
 
         assert_equal(test.nodes[0].z_getbalance(myzaddr), Decimal('0'))
-        assert_equal(test.nodes[0].z_getbalance(myzaddr2), Decimal('40') - DEFAULT_FEE)
+        assert_equal(test.nodes[0].z_getbalance(myzaddr2), Decimal('40') - conventional_fee(4))
 
         # Shield coinbase UTXOs from any node 2 taddr, and set fee to 0
         result = test.nodes[2].z_shieldcoinbase("*", myzaddr, 0)
@@ -202,7 +202,7 @@ class MergeToAddressHelper:
 
         assert_equal(test.nodes[0].getbalance(), Decimal('10'))
         assert_equal(test.nodes[0].z_getbalance(myzaddr), Decimal('30'))
-        assert_equal(test.nodes[0].z_getbalance(myzaddr2), Decimal('40') - DEFAULT_FEE)
+        assert_equal(test.nodes[0].z_getbalance(myzaddr2), Decimal('40') - conventional_fee(4))
         assert_equal(test.nodes[1].getbalance(), Decimal('60'))
         assert_equal(test.nodes[2].getbalance(), Decimal('0'))
 
@@ -213,9 +213,9 @@ class MergeToAddressHelper:
         generate_and_check(test.nodes[1], 2)
         test.sync_all()
 
-        assert_equal(test.nodes[0].getbalance(), Decimal('80') - DEFAULT_FEE)
+        assert_equal(test.nodes[0].getbalance(), Decimal('80') - conventional_fee(4))
         assert_equal(test.nodes[0].z_getbalance(do_not_shield_taddr), Decimal('10'))
-        assert_equal(test.nodes[0].z_getbalance(mytaddr), Decimal('70') - DEFAULT_FEE)
+        assert_equal(test.nodes[0].z_getbalance(mytaddr), Decimal('70') - conventional_fee(4))
         assert_equal(test.nodes[0].z_getbalance(myzaddr), Decimal('0'))
         assert_equal(test.nodes[0].z_getbalance(myzaddr2), Decimal('0'))
         assert_equal(test.nodes[1].getbalance(), Decimal('70'))
@@ -235,8 +235,8 @@ class MergeToAddressHelper:
         assert_equal(test.nodes[0].z_getbalance(do_not_shield_taddr), Decimal('0'))
         assert_equal(test.nodes[0].z_getbalance(mytaddr), Decimal('0'))
         assert_equal(test.nodes[0].z_getbalance(myzaddr), Decimal('0'))
-        assert_equal(test.nodes[1].getbalance(), Decimal('160') - DEFAULT_FEE)
-        assert_equal(test.nodes[1].z_getbalance(n1taddr), Decimal('80') - DEFAULT_FEE)
+        assert_equal(test.nodes[1].getbalance(), Decimal('160') - conventional_fee(4))
+        assert_equal(test.nodes[1].z_getbalance(n1taddr), Decimal('80') - conventional_fee(4))
         assert_equal(test.nodes[2].getbalance(), Decimal('0'))
 
         # Generate 5 regular UTXOs on node 0, and 20 regular UTXOs on node 2.

src/gtest/test_transaction_builder.cpp

@@ -317,18 +317,18 @@ TEST(TransactionBuilder, FailsWithNegativeChange)
     // 0.00005 z-ZEC out, default fee
     auto builder = TransactionBuilder(consensusParams, 1, std::nullopt);
     builder.AddSaplingOutput(fvk.ovk, pa, 5000, {});
-    EXPECT_EQ("Change cannot be negative", builder.Build().GetError());
+    EXPECT_EQ("Change cannot be negative: -0.00006 ZEC", builder.Build().GetError());
 
     // Fail if there is only a transparent output
     // 0.00005 t-ZEC out, default fee
     builder = TransactionBuilder(consensusParams, 1, std::nullopt, &keystore);
     builder.AddTransparentOutput(taddr, 5000);
-    EXPECT_EQ("Change cannot be negative", builder.Build().GetError());
+    EXPECT_EQ("Change cannot be negative: -0.00006 ZEC", builder.Build().GetError());
 
     // Fails if there is insufficient input
     // 0.00005 t-ZEC out, default fee, 0.00005999 z-ZEC in
     builder.AddSaplingSpend(expsk, testNote.note, testNote.tree.root(), testNote.tree.witness());
-    EXPECT_EQ("Change cannot be negative", builder.Build().GetError());
+    EXPECT_EQ("Change cannot be negative: -0.00000001 ZEC", builder.Build().GetError());
 
     // Succeeds if there is sufficient input
     builder.AddTransparentInput(COutPoint(), scriptPubKey, 1);

src/transaction_builder.cpp

@@ -501,7 +501,8 @@ TransactionBuilderResult TransactionBuilder::Build()
         change -= tOut.nValue;
     }
     if (change < 0) {
-        return TransactionBuilderResult("Change cannot be negative");
+        return TransactionBuilderResult(
+                strprintf("Change cannot be negative: %s", DisplayMoney(change)));
     }
 
     //

src/wallet/asyncrpcoperation_mergetoaddress.cpp

@@ -27,7 +27,6 @@
 #include "util/time.h"
 #include "wallet.h"
 #include "walletdb.h"
-#include "wallet/paymentdisclosuredb.h"
 #include "zcash/IncrementalMerkleTree.hpp"
 
 #include <chrono>
@@ -40,53 +39,16 @@
 
 using namespace libzcash;
 
-int mta_find_output(UniValue obj, int n)
-{
-    UniValue outputMapValue = find_value(obj, "outputmap");
-    if (!outputMapValue.isArray()) {
-        throw JSONRPCError(RPC_WALLET_ERROR, "Missing outputmap for JoinSplit operation");
-    }
-
-    UniValue outputMap = outputMapValue.get_array();
-    assert(outputMap.size() == ZC_NUM_JS_OUTPUTS);
-    for (size_t i = 0; i < outputMap.size(); i++) {
-        if (outputMap[i].get_int() == n) {
-            return i;
-        }
-    }
-
-    throw std::logic_error("n is not present in outputmap");
-}
-
 AsyncRPCOperation_mergetoaddress::AsyncRPCOperation_mergetoaddress(
-    WalletTxBuilder builder,
-    ZTXOSelector ztxoSelector,
-    SpendableInputs allInputs,
-    MergeToAddressRecipient recipient,
-    TransactionStrategy strategy,
-    CAmount fee,
-    UniValue contextInfo) :
-    builder_(builder), ztxoSelector_(ztxoSelector), allInputs_(allInputs), toPaymentAddress_(recipient.first), memo_(recipient.second), fee_(fee), strategy_(strategy), contextinfo_(contextInfo)
+        CWallet& wallet,
+        TransactionStrategy strategy,
+        TransactionEffects effects,
+        UniValue contextInfo)
+    : strategy_(strategy), effects_(effects), contextinfo_(contextInfo)
 {
-    if (!MoneyRange(fee)) {
-        throw JSONRPCError(RPC_INVALID_PARAMETER, "Fee is out of range");
-    }
+    effects.LockSpendable(*pwalletMain);
 
     KeyIO keyIO(Params());
-    isToTaddr_ = false;
-
-    examine(toPaymentAddress_, match {
-        [&](const CKeyID&) { isToTaddr_ = true; },
-        [&](const CScriptID&) { isToTaddr_ = true; },
-        [](const SproutPaymentAddress&) { },
-        [](const libzcash::SaplingPaymentAddress&) { },
-        [](const libzcash::UnifiedAddress&) {
-            // TODO: This should be removable with WalletTxBuilder, but need to test it.
-            throw JSONRPCError(
-                    RPC_INVALID_ADDRESS_OR_KEY,
-                    "z_mergetoaddress does not yet support sending to unified addresses");
-        },
-    });
 
     // Log the context info i.e. the call parameters to z_mergetoaddress
     if (LogAcceptCategory("zrpcunsafe")) {
@@ -94,15 +56,21 @@ AsyncRPCOperation_mergetoaddress::AsyncRPCOperation_mergetoaddress(
     } else {
         LogPrint("zrpc", "%s: z_mergetoaddress initialized\n", getId());
     }
-
-    // Enable payment disclosure if requested
-    paymentDisclosureMode = fExperimentalPaymentDisclosure;
 }
 
 AsyncRPCOperation_mergetoaddress::~AsyncRPCOperation_mergetoaddress()
 {
 }
 
+std::pair<uint256, UniValue>
+main_impl(
+        const CChainParams& chainparams,
+        CWallet& wallet,
+        const TransactionStrategy& strategy,
+        const TransactionEffects& effects,
+        const std::string& id,
+        bool testmode);
+
 void AsyncRPCOperation_mergetoaddress::main()
 {
     set_state(OperationStatus::EXECUTING);
@@ -116,7 +84,10 @@ void AsyncRPCOperation_mergetoaddress::main()
 
     std::optional<uint256> txid;
     try {
-        txid = main_impl(*pwalletMain, Params());
+        UniValue sendResult;
+        std::tie(txid, sendResult) =
+            main_impl(Params(), *pwalletMain, strategy_, effects_, getId(), testmode);
+        set_result(sendResult);
     } catch (const UniValue& objError) {
         int code = find_value(objError, "code").get_int();
         std::string message = find_value(objError, "message").get_str();
@@ -157,111 +128,68 @@ void AsyncRPCOperation_mergetoaddress::main()
     LogPrintf("%s", s);
 }
 
-tl::expected<void, InputSelectionError> AsyncRPCOperation_mergetoaddress::prepare(const CChainParams& chainparams, CWallet& wallet) {
-    CAmount minersFee = fee_;
-
-    size_t numInputs = allInputs_.utxos.size();
-
-    CAmount t_inputs_total = 0;
-    for (COutput& t : allInputs_.utxos) {
-        t_inputs_total += t.Value();
-    }
-
-    CAmount z_inputs_total = 0;
-    for (const SproutNoteEntry& t : allInputs_.sproutNoteEntries) {
-        z_inputs_total += t.note.value();
-    }
-
-    for (const SaplingNoteEntry& t : allInputs_.saplingNoteEntries) {
-        z_inputs_total += t.note.value();
-    }
-
-    for (const OrchardNoteMetadata& t : allInputs_.orchardNoteMetadata) {
-        z_inputs_total += t.GetNoteValue();
-    }
-
-    CAmount targetAmount = z_inputs_total + t_inputs_total;
-
-    CAmount sendAmount = targetAmount - minersFee;
-    if (sendAmount <= 0) {
-        return tl::make_unexpected(InvalidFundsError(sendAmount, InsufficientFundsError(minersFee)));
-    }
-
-    // Grab the current consensus branch ID
-    {
-        LOCK(cs_main);
-        consensusBranchId_ = CurrentEpochBranchId(chainActive.Height() + 1, chainparams.GetConsensus());
-    }
-
-    auto payment = Payment(toPaymentAddress_, sendAmount, memo_);
-
-    return builder_.PrepareTransaction(
-                wallet,
-                ztxoSelector_,
-                allInputs_,
-                {payment},
-                chainActive,
-                strategy_,
-                minersFee,
-                nAnchorConfirmations)
-            .map([&](const TransactionEffects& effects) -> void {
-                effects.LockSpendable(wallet);
-                effects_ = effects;
-            });
-}
-
-// Notes:
-// 1. #1359 Currently there is no limit set on the number of joinsplits, so size of tx could be invalid.
-// 2. #1277 Spendable notes are not locked, so an operation running in parallel could also try to use them.
-uint256 AsyncRPCOperation_mergetoaddress::main_impl(CWallet& wallet, const CChainParams& chainparams)
+std::pair<uint256, UniValue>
+main_impl(
+        const CChainParams& chainparams,
+        CWallet& wallet,
+        const TransactionStrategy& strategy,
+        const TransactionEffects& effects,
+        const std::string& id,
+        bool testmode)
 {
-    // The caller must call `prepare` before `main_impl` is invoked.
-    const auto effects = effects_.value();
     try {
         const auto& spendable = effects.GetSpendable();
         const auto& payments = effects.GetPayments();
-        spendable.LogInputs(getId());
+        spendable.LogInputs(id);
 
-        if (allInputs_.sproutNoteEntries.empty()
-            && allInputs_.saplingNoteEntries.empty()
-            && allInputs_.orchardNoteMetadata.empty()
-            && isToTaddr_) {
+        bool sendsToShielded = false;
+        for (const auto& resolvedPayment : payments.GetResolvedPayments()) {
+            sendsToShielded = sendsToShielded || examine(resolvedPayment.address, match {
+                [](const CKeyID&) { return true; },
+                [](const CScriptID&) { return true; },
+                [](const libzcash::SaplingPaymentAddress&) { return false; },
+                [](const libzcash::OrchardRawAddress&) { return false; },
+            });
+        }
+        if (spendable.sproutNoteEntries.empty()
+            && spendable.saplingNoteEntries.empty()
+            && spendable.orchardNoteMetadata.empty()
+            && !sendsToShielded) {
             LogPrint("zrpc", "%s: spending %s to send %s with fee %s\n",
-                getId(),
+                id,
                 FormatMoney(payments.Total()),
                 FormatMoney(spendable.Total()),
                 FormatMoney(effects.GetFee()));
         } else {
             LogPrint("zrpcunsafe", "%s: spending %s to send %s with fee %s\n",
-                getId(),
+                id,
                 FormatMoney(payments.Total()),
                 FormatMoney(spendable.Total()),
                 FormatMoney(effects.GetFee()));
         }
-        LogPrint("zrpc", "%s: total transparent input: %s\n", getId(),
+        LogPrint("zrpc", "%s: total transparent input: %s\n", id,
             FormatMoney(spendable.GetTransparentTotal()));
-        LogPrint("zrpcunsafe", "%s: total shielded input: %s\n", getId(),
+        LogPrint("zrpcunsafe", "%s: total shielded input: %s\n", id,
             FormatMoney(spendable.GetSaplingTotal() + spendable.GetOrchardTotal()));
-        LogPrint("zrpc", "%s: total transparent output: %s\n", getId(),
+        LogPrint("zrpc", "%s: total transparent output: %s\n", id,
             FormatMoney(payments.GetTransparentTotal()));
-        LogPrint("zrpcunsafe", "%s: total shielded Sapling output: %s\n", getId(),
+        LogPrint("zrpcunsafe", "%s: total shielded Sapling output: %s\n", id,
             FormatMoney(payments.GetSaplingTotal()));
-        LogPrint("zrpcunsafe", "%s: total shielded Orchard output: %s\n", getId(),
+        LogPrint("zrpcunsafe", "%s: total shielded Orchard output: %s\n", id,
             FormatMoney(payments.GetOrchardTotal()));
-        LogPrint("zrpc", "%s: fee: %s\n", getId(), FormatMoney(effects.GetFee()));
+        LogPrint("zrpc", "%s: fee: %s\n", id, FormatMoney(effects.GetFee()));
 
         auto buildResult = effects.ApproveAndBuild(
                 chainparams.GetConsensus(),
                 wallet,
                 chainActive,
-                strategy_);
+                strategy);
         auto tx = buildResult.GetTxOrThrow();
 
         UniValue sendResult = SendTransaction(tx, payments.GetResolvedPayments(), std::nullopt, testmode);
-        set_result(sendResult);
 
         effects.UnlockSpendable(wallet);
-        return tx.GetHash();
+        return {tx.GetHash(), sendResult};
     } catch (...) {
         effects.UnlockSpendable(wallet);
         throw;

src/wallet/asyncrpcoperation_mergetoaddress.h

@@ -27,9 +27,6 @@
 
 using namespace libzcash;
 
-// A recipient is a tuple of address, memo (optional if zaddr)
-typedef std::pair<libzcash::PaymentAddress, std::optional<Memo>> MergeToAddressRecipient;
-
 // Package of info which is passed to perform_joinsplit methods.
 struct MergeToAddressJSInfo {
     std::vector<JSInput> vjsin;
@@ -50,13 +47,10 @@ class AsyncRPCOperation_mergetoaddress : public AsyncRPCOperation
 {
 public:
     AsyncRPCOperation_mergetoaddress(
-        WalletTxBuilder builder,
-        ZTXOSelector ztxoSelector,
-        SpendableInputs allInputs,
-        MergeToAddressRecipient recipient,
-        TransactionStrategy strategy,
-        CAmount fee = DEFAULT_FEE,
-        UniValue contextInfo = NullUniValue);
+            CWallet& wallet,
+            TransactionStrategy strategy,
+            TransactionEffects effects,
+            UniValue contextInfo = NullUniValue);
     virtual ~AsyncRPCOperation_mergetoaddress();
 
     // We don't want to be copied or moved around
@@ -65,69 +59,20 @@ public:
     AsyncRPCOperation_mergetoaddress& operator=(AsyncRPCOperation_mergetoaddress const&) = delete; // Copy assign
     AsyncRPCOperation_mergetoaddress& operator=(AsyncRPCOperation_mergetoaddress&&) = delete;      // Move assign
 
-    tl::expected<void, InputSelectionError> prepare(const CChainParams& chainparams, CWallet& wallet);
-
     virtual void main();
 
     virtual UniValue getStatus() const;
 
-    bool testmode = false; // Set to true to disable sending txs and generating proofs
-
-    bool paymentDisclosureMode = false; // Set to true to save esk for encrypted notes in payment disclosure database.
+    /// Set to true to disable sending txs and generating proofs
+    bool testmode = false;
 
 private:
-    friend class TEST_FRIEND_AsyncRPCOperation_mergetoaddress; // class for unit testing
+    const TransactionStrategy strategy_;
 
-    UniValue contextinfo_; // optional data to include in return value from getStatus()
+    const TransactionEffects effects_;
 
-    uint32_t consensusBranchId_;
-    CAmount fee_;
-    int mindepth_;
-    bool isToTaddr_;
-    bool isToZaddr_;
-    PaymentAddress toPaymentAddress_;
-    std::optional<Memo> memo_;
-
-    ed25519::VerificationKey joinSplitPubKey_;
-    ed25519::SigningKey joinSplitPrivKey_;
-
-    // The key is the result string from calling JSOutPoint::ToString()
-    std::unordered_map<std::string, MergeToAddressWitnessAnchorData> jsopWitnessAnchorMap;
-
-    WalletTxBuilder builder_;
-    ZTXOSelector ztxoSelector_;
-    SpendableInputs allInputs_;
-    TransactionStrategy strategy_;
-
-    std::optional<TransactionEffects> effects_;
-
-    uint256 main_impl(CWallet& wallet, const CChainParams& chainparams);
-
-    // payment disclosure!
-    std::vector<PaymentDisclosureKeyInfo> paymentDisclosureData_;
+    /// optional data to include in return value from getStatus()
+    UniValue contextinfo_;
 };
 
-
-// To test private methods, a friend class can act as a proxy
-class TEST_FRIEND_AsyncRPCOperation_mergetoaddress
-{
-public:
-    std::shared_ptr<AsyncRPCOperation_mergetoaddress> delegate;
-
-    TEST_FRIEND_AsyncRPCOperation_mergetoaddress(std::shared_ptr<AsyncRPCOperation_mergetoaddress> ptr) : delegate(ptr) {}
-
-    // Delegated methods
-
-    uint256 main_impl(CWallet& wallet, const CChainParams& chainparams)
-    {
-        return delegate->main_impl(wallet, chainparams);
-    }
-
-    void set_state(OperationStatus state)
-    {
-        delegate->state_.store(state);
-    }
-};
-
-
 #endif // ZCASH_WALLET_ASYNCRPCOPERATION_MERGETOADDRESS_H

src/wallet/gtest/test_rpc_wallet.cpp

@@ -8,7 +8,6 @@
 #include "transaction_builder.h"
 #include "util/test.h"
 #include "gtest/utils.h"
-#include "wallet/asyncrpcoperation_mergetoaddress.h"
 #include "wallet/asyncrpcoperation_shieldcoinbase.h"
 #include "wallet/asyncrpcoperation_sendmany.h"
 #include "wallet/memo.h"
@@ -57,9 +56,9 @@ TEST(WalletRPCTests, RPCZMergeToAddressInternals)
     auto taddr = pwalletMain->GenerateNewKey(true).GetID();
     std::string taddr_string = keyIO.EncodeDestination(taddr);
 
-    MergeToAddressRecipient taddr1(keyIO.DecodePaymentAddress(taddr_string).value(), Memo());
+    NetAmountRecipient taddr1(keyIO.DecodePaymentAddress(taddr_string).value(), Memo());
     auto pa = pwalletMain->GenerateNewSproutZKey();
-    MergeToAddressRecipient zaddr1(pa, Memo());
+    NetAmountRecipient zaddr1(pa, Memo());
 
     WalletTxBuilder builder(Params(), minRelayTxFee);
     auto selector = CWallet::LegacyTransparentZTXOSelector(
@@ -72,11 +71,24 @@ TEST(WalletRPCTests, RPCZMergeToAddressInternals)
         SpendableInputs inputs;
         auto wtx = FakeWalletTx();
         inputs.utxos.emplace_back(COutput(&wtx, 0, 100, true));
-        auto operation = AsyncRPCOperation_mergetoaddress(std::move(builder), selector, inputs, zaddr1, strategy, 0);
-        operation.main();
-        EXPECT_TRUE(operation.isFailed());
-        std::string msg = operation.getErrorMessage();
-        EXPECT_EQ(msg, "Sending funds into the Sprout pool is no longer supported.");
+        builder.PrepareTransaction(
+                *pwalletMain,
+                selector,
+                inputs,
+                zaddr1,
+                chainActive,
+                strategy,
+                0,
+                1)
+            .map_error([](const auto& err) {
+                EXPECT_TRUE(examine(err, match {
+                    [](const AddressResolutionError& are) {
+                        return are == AddressResolutionError::SproutRecipientsNotSupported;
+                    },
+                    [](const auto&) { return false; },
+                }));
+            })
+            .map([](const auto&) { EXPECT_TRUE(false); });
     }
     }
     UnloadGlobalWallet();

src/wallet/rpcwallet.cpp

@@ -5370,8 +5370,8 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp)
             "      ,...\n"
             "    ]\n"
             "2. \"toaddress\"           (string, required) The taddr or zaddr to send the funds to.\n"
-            "3. fee                   (numeric, optional, default="
-            + strprintf("%s", FormatMoney(DEFAULT_FEE)) + ") The fee amount to attach to this transaction.\n"
+            "3. fee                   (numeric, optional, default=null) The fee amount in " + CURRENCY_UNIT + " to attach to this transaction. The default behavior\n"
+            "                         is to use a fee calculated according to ZIP 317.\n"
             "4. transparent_limit     (numeric, optional, default="
             + strprintf("%d", MERGE_TO_ADDRESS_DEFAULT_TRANSPARENT_LIMIT) + ") Limit on the maximum number of UTXOs to merge.  Set to 0 to use as many as will fit in the transaction.\n"
             "5. shielded_limit        (numeric, optional, default="
@@ -5529,14 +5529,9 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp)
                 string("Invalid parameter, unknown address format: ") + destStr);
     }
 
-    // Convert fee from currency format to zatoshis
-    CAmount nFee = DEFAULT_FEE;
-    if (params.size() > 2) {
-        if (params[2].get_real() == 0.0) {
-            nFee = 0;
-        } else {
-            nFee = AmountFromValue( params[2] );
-        }
+    std::optional<CAmount> nFee;
+    if (params.size() > 2 && !params[2].isNull()) {
+        nFee = AmountFromValue( params[2] );
     }
 
     int nUTXOLimit = MERGE_TO_ADDRESS_DEFAULT_TRANSPARENT_LIMIT;
@@ -5563,7 +5558,7 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp)
         memo = Memo::FromHexOrThrow(params[5].get_str());
     }
 
-    MergeToAddressRecipient recipient(destaddress.value(), memo);
+    NetAmountRecipient recipient(destaddress.value(), memo);
 
     // Prepare to get UTXOs and notes
     SpendableInputs allInputs;
@@ -5737,28 +5732,18 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp)
     // - We only have one from address
     // - It's equal to toaddress
     // - The address only contains a single UTXO or note
+    // TODO: Move this to WalletTxBuilder
     if (setAddress.size() == 1 && setAddress.count(destStr) && (numUtxos + numNotes) == 1) {
         throw JSONRPCError(RPC_INVALID_PARAMETER, "Destination address is also the only source address, and all its funds are already merged.");
     }
 
-    CAmount mergedValue = mergedUTXOValue + mergedNoteValue;
-    if (mergedValue < nFee) {
-        throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS,
-            strprintf("Insufficient funds, have %s, which is less than miners fee %s",
-            FormatMoney(mergedValue), FormatMoney(nFee)));
-    }
-
-    // Check that the user specified fee is sane (if too high, it can result in error -25 absurd fee)
-    CAmount netAmount = mergedValue - nFee;
-    if (nFee > netAmount) {
-        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Fee %s is greater than the net amount to be shielded %s", FormatMoney(nFee), FormatMoney(netAmount)));
-    }
-
     // Keep record of parameters in context object
     UniValue contextInfo(UniValue::VOBJ);
     contextInfo.pushKV("fromaddresses", params[0]);
     contextInfo.pushKV("toaddress", params[1]);
-    contextInfo.pushKV("fee", ValueFromAmount(nFee));
+    if (nFee.has_value()) {
+        contextInfo.pushKV("fee", ValueFromAmount(nFee.value()));
+    }
 
     // The privacy policy is determined early so as to be able to use it
     // for selector construction.
@@ -5769,37 +5754,14 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp)
                         params.size() > 6 ? std::optional(params[6].get_str()) : std::nullopt),
                 InterpretLegacyCompat(std::nullopt, {recipient.first}));
 
-    bool isSproutShielded = allInputs.sproutNoteEntries.size() > 0;
-    // Contextual transaction we will build on
-    CMutableTransaction contextualTx = CreateNewContextualCMutableTransaction(
-        chainparams.GetConsensus(),
-        nextBlockHeight,
-        isSproutShielded || nPreferredTxVersion < ZIP225_MIN_TX_VERSION);
-    if (contextualTx.nVersion == 1 && isSproutShielded) {
-        contextualTx.nVersion = 2; // Tx format should support vJoinSplit
-    }
-
-    if (isToSaplingZaddr || allInputs.saplingNoteEntries.size() > 0) {
-        std::optional<uint256> orchardAnchor;
-        if (!isSproutShielded && nPreferredTxVersion >= ZIP225_MIN_TX_VERSION && nAnchorConfirmations > 0) {
-            // Allow Orchard recipients by setting an Orchard anchor.
-            auto orchardAnchorHeight = nextBlockHeight - nAnchorConfirmations;
-            if (chainparams.GetConsensus().NetworkUpgradeActive(orchardAnchorHeight, Consensus::UPGRADE_NU5)) {
-                auto anchorBlockIndex = chainActive[orchardAnchorHeight];
-                assert(anchorBlockIndex != nullptr);
-                orchardAnchor = anchorBlockIndex->hashFinalOrchardRoot;
-            }
-        }
-    }
-
     WalletTxBuilder builder(Params(), minRelayTxFee);
 
-    // This currently isn’t too critical since we don’t yet use it for note selection and we never
-    // need a change address. The one thing this does is indicate whether or not Sprout can be
-    // selected. However, we eventually want a `ZTXOSelector` that can support this operation fully.
-    // I.e., be able to select from multiple pools in the legacy account.
+    // The ZTXOSelector here is only used to determine whether or not Sprout can be selected. It
+    // should not be used for other purposes (e.g., note selection or finding a change address).
+    // TODO: Add a ZTXOSelector that can support the full range of `z_mergetoaddress` behavior and
+    //       use it instead of `GetFilteredNotes`.
     std::optional<ZTXOSelector> ztxoSelector;
-    if (isSproutShielded) {
+    if (allInputs.sproutNoteEntries.size() > 0) {
         ztxoSelector = pwalletMain->ZTXOSelectorForAddress(
                 allInputs.sproutNoteEntries[0].address,
                 true,
@@ -5820,20 +5782,23 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp)
     }
 
     // Create operation and add to global queue
-    std::shared_ptr<AsyncRPCQueue> q = getAsyncRPCQueue();
-    auto mergeOp = new AsyncRPCOperation_mergetoaddress(
-                    std::move(builder),
-                    ztxoSelector.value(),
-                    allInputs,
-                    recipient,
-                    strategy,
-                    nFee,
-                    contextInfo);
-    mergeOp->prepare(chainparams, *pwalletMain).map_error([&](const InputSelectionError& err) {
-        ThrowInputSelectionError(err, ztxoSelector.value(), strategy);
-    });
+    auto effects = builder.PrepareTransaction(
+            *pwalletMain,
+            ztxoSelector.value(),
+            allInputs,
+            recipient,
+            chainActive,
+            strategy,
+            nFee,
+            nAnchorConfirmations)
+        .map_error([&](const auto& err) {
+            ThrowInputSelectionError(err, ztxoSelector.value(), strategy);
+        })
+        .value();
 
-    std::shared_ptr<AsyncRPCOperation> operation(mergeOp);
+    std::shared_ptr<AsyncRPCQueue> q = getAsyncRPCQueue();
+    std::shared_ptr<AsyncRPCOperation> operation(
+            new AsyncRPCOperation_mergetoaddress(*pwalletMain, strategy, effects, contextInfo));
     q->addOperation(operation);
     AsyncRPCOperationId operationId = operation->getId();
 

src/wallet/test/rpc_wallet_tests.cpp

@@ -17,7 +17,6 @@
 #include "asyncrpcqueue.h"
 #include "asyncrpcoperation.h"
 #include "wallet/asyncrpcoperation_common.h"
-#include "wallet/asyncrpcoperation_mergetoaddress.h"
 #include "wallet/asyncrpcoperation_sendmany.h"
 #include "wallet/asyncrpcoperation_shieldcoinbase.h"
 #include "wallet/memo.h"
@@ -1226,6 +1225,7 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_internals)
     SelectParams(CBaseChainParams::TESTNET);
     const Consensus::Params& consensusParams = Params().GetConsensus();
     KeyIO keyIO(Params());
+    WalletTxBuilder builder(Params(), minRelayTxFee);
 
     LOCK2(cs_main, pwalletMain->cs_wallet);
 
@@ -1255,7 +1255,6 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_internals)
                 // confirm this.
                 TransparentCoinbasePolicy::Allow,
                 false).value();
-        WalletTxBuilder builder(Params(), minRelayTxFee);
         std::vector<Payment> recipients = { Payment(zaddr1, 100*COIN, Memo::FromHexOrThrow("DEADBEEF")) };
         TransactionStrategy strategy(PrivacyPolicy::AllowRevealedSenders);
         std::shared_ptr<AsyncRPCOperation> operation(new AsyncRPCOperation_sendmany(std::move(builder), selector, recipients, 1, 1, strategy, std::nullopt));
@@ -1272,7 +1271,6 @@ BOOST_AUTO_TEST_CASE(rpc_z_sendmany_internals)
                 true,
                 TransparentCoinbasePolicy::Disallow,
                 false).value();
-        WalletTxBuilder builder(Params(), minRelayTxFee);
         std::vector<Payment> recipients = { Payment(taddr1, 100*COIN, Memo::FromHexOrThrow("DEADBEEF")) };
         TransactionStrategy strategy(PrivacyPolicy::AllowRevealedRecipients);
         std::shared_ptr<AsyncRPCOperation> operation(new AsyncRPCOperation_sendmany(std::move(builder), selector, recipients, 1, 1, strategy, std::nullopt));
@@ -1530,6 +1528,7 @@ BOOST_AUTO_TEST_CASE(rpc_z_shieldcoinbase_parameters)
 
     // Test constructor of AsyncRPCOperation_shieldcoinbase
     KeyIO keyIO(Params());
+    WalletTxBuilder builder(Params(), minRelayTxFee);
     UniValue retValue;
     BOOST_CHECK_NO_THROW(retValue = CallRPC("getnewaddress"));
     auto taddr2 = std::get<CKeyID>(keyIO.DecodePaymentAddress(retValue.get_str()).value());
@@ -1542,7 +1541,6 @@ BOOST_AUTO_TEST_CASE(rpc_z_shieldcoinbase_parameters)
             // confirm this.
             TransparentCoinbasePolicy::Allow,
             false).value();
-    WalletTxBuilder builder(Params(), minRelayTxFee);
 
     try {
         std::shared_ptr<AsyncRPCOperation> operation(new AsyncRPCOperation_shieldcoinbase(std::move(builder), selector, testnetzaddr, PrivacyPolicy::AllowRevealedSenders, SHIELD_COINBASE_DEFAULT_LIMIT, 1));
@@ -1634,29 +1632,44 @@ BOOST_AUTO_TEST_CASE(rpc_z_mergetoaddress_parameters)
     int nHeight = retValue.get_int();
     CMutableTransaction mtx = CreateNewContextualCMutableTransaction(Params().GetConsensus(), nHeight + 1, false);
 
-    // Test constructor of AsyncRPCOperation_mergetoaddress
     KeyIO keyIO(Params());
-    MergeToAddressRecipient testnetzaddr(
+    WalletTxBuilder builder(Params(), minRelayTxFee);
+    NetAmountRecipient testnetzaddr(
         keyIO.DecodePaymentAddress("ztjiDe569DPNbyTE6TSdJTaSDhoXEHLGvYoUnBU1wfVNU52TEyT6berYtySkd21njAeEoh8fFJUT42kua9r8EnhBaEKqCpP").value(),
         Memo());
-    WalletTxBuilder builder(Params(), minRelayTxFee);
     auto selector = CWallet::LegacyTransparentZTXOSelector(
             true,
             TransparentCoinbasePolicy::Disallow);
     TransactionStrategy strategy(PrivacyPolicy::AllowRevealedRecipients);
 
-    try {
-        auto operation = AsyncRPCOperation_mergetoaddress(builder, selector, {}, testnetzaddr, strategy, -1);
-        BOOST_FAIL("Fee value of -1 expected to be out of the valid range of values.");
-    } catch (const UniValue& objError) {
-        BOOST_CHECK( find_error(objError, "Fee is out of range"));
-    }
+    builder.PrepareTransaction(*pwalletMain, selector, {}, testnetzaddr, chainActive, strategy, -1, 1)
+        .map_error([&](const auto& err) {
+            // TODO: Provide `operator==` on `InputSelectionError` and use that here.
+            BOOST_CHECK(examine(err, match {
+                [](AddressResolutionError are) {
+                    // FIXME: Should be failing because of negative fee – change recipient address to not be Sprout.
+                    return are == AddressResolutionError::SproutRecipientsNotSupported;
+                },
+                [](const auto&) { return false; },
+            }));
+        })
+        .map([](const auto&) {
+            BOOST_FAIL("Fee value of -1 expected to be out of the valid range of values.");
+        });
 
-    {
-        auto operation = AsyncRPCOperation_mergetoaddress(builder, selector, {}, testnetzaddr, strategy, 1);
-        operation.main();
-        BOOST_CHECK_EQUAL(operation.getErrorMessage(), "Insufficient funds: have -0.00000001, need 0.00000001; note that coinbase outputs will not be selected if you specify ANY_TADDR, any transparent recipients are included, or if the `privacyPolicy` parameter is not set to `AllowRevealedSenders` or weaker.");
-    }
+    builder.PrepareTransaction(*pwalletMain, selector, {}, testnetzaddr, chainActive, strategy, 1, 1)
+        .map_error([&](const auto& err) {
+            // TODO: Provide `operator==` on `InputSelectionError` and use that here.
+            BOOST_CHECK(examine(err, match {
+                [](const InvalidFundsError& ife) {
+                    return std::holds_alternative<InsufficientFundsError>(ife.reason);
+                },
+                [](const auto&) { return false; },
+            }));
+        })
+        .map([](const auto&) {
+            BOOST_FAIL("Expected an error.");
+        });
 
     auto wtx = FakeWalletTx();
     SpendableInputs inputs;
@@ -1664,11 +1677,20 @@ BOOST_AUTO_TEST_CASE(rpc_z_mergetoaddress_parameters)
     inputs.sproutNoteEntries.emplace_back(SproutNoteEntry {JSOutPoint(), SproutPaymentAddress(), SproutNote(), "", 0});
     inputs.saplingNoteEntries.emplace_back(SaplingNoteEntry {SaplingOutPoint(), SaplingPaymentAddress(), SaplingNote({}, uint256(), 0, uint256(), Zip212Enabled::BeforeZip212), "", 0});
 
-    {
-        auto operation = AsyncRPCOperation_mergetoaddress(builder, selector, inputs, testnetzaddr, strategy, 1);
-        operation.main();
-        BOOST_CHECK_EQUAL(operation.getErrorMessage(), "Insufficient funds: have 0.00, need 0.00000001; note that coinbase outputs will not be selected if you specify ANY_TADDR, any transparent recipients are included, or if the `privacyPolicy` parameter is not set to `AllowRevealedSenders` or weaker.");
-    }
+    builder.PrepareTransaction(*pwalletMain, selector, inputs, testnetzaddr, chainActive, strategy, 1, 1)
+        .map_error([&](const auto& err) {
+            // TODO: Provide `operator==` on `InputSelectionError` and use that here.
+            BOOST_CHECK(examine(err, match {
+                [](AddressResolutionError are) {
+                    // FIXME: Should be failing because of insufficient funds – change recipient address to not be Sprout.
+                    return are == AddressResolutionError::SproutRecipientsNotSupported;
+                },
+                [](const auto&) { return false; },
+            }));
+        })
+        .map([](const auto&) {
+            BOOST_FAIL("Expected an error.");
+        });
 }
 
 void TestWTxStatus(const Consensus::Params consensusParams, const int delta) {

src/wallet/wallet_tx_builder.cpp

@@ -260,7 +260,7 @@ tl::expected<ChangeAddress, AddressResolutionError>
 WalletTxBuilder::GetChangeAddress(
         CWallet& wallet,
         const ZTXOSelector& selector,
-        SpendableInputs& spendable,
+        const SpendableInputs& spendable,
         const Payments& resolvedPayments,
         const TransactionStrategy& strategy,
         bool afterNU5) const
@@ -385,11 +385,11 @@ tl::expected<TransactionEffects, InputSelectionError>
 WalletTxBuilder::PrepareTransaction(
         CWallet& wallet,
         const ZTXOSelector& selector,
-        SpendableInputs& spendable,
+        const SpendableInputs& spendable,
         const Recipients& payments,
         const CChain& chain,
-        TransactionStrategy strategy,
-        std::optional<CAmount> fee,
+        const TransactionStrategy& strategy,
+        const std::optional<CAmount>& fee,
         uint32_t anchorConfirmations) const
 {
     if (fee.has_value() && maxTxFee < fee.value()) {
@@ -521,7 +521,7 @@ tl::expected<
 WalletTxBuilder::IterateLimit(
         CWallet& wallet,
         const ZTXOSelector& selector,
-        const TransactionStrategy strategy,
+        const TransactionStrategy& strategy,
         CAmount sendAmount,
         CAmount dustThreshold,
         const SpendableInputs& spendable,
@@ -615,11 +615,11 @@ tl::expected<InputSelection, InputSelectionError>
 WalletTxBuilder::ResolveInputsAndPayments(
         CWallet& wallet,
         const ZTXOSelector& selector,
-        SpendableInputs& spendableMut,
+        SpendableInputs spendableMut,
         const std::vector<Payment>& payments,
         const CChain& chain,
         const TransactionStrategy& strategy,
-        std::optional<CAmount> fee,
+        const std::optional<CAmount>& fee,
         bool afterNU5) const
 {
     LOCK2(cs_main, wallet.cs_wallet);

src/wallet/wallet_tx_builder.h

@@ -376,7 +376,7 @@ private:
     GetChangeAddress(
             CWallet& wallet,
             const ZTXOSelector& selector,
-            SpendableInputs& spendable,
+            const SpendableInputs& spendable,
             const Payments& resolvedPayments,
             const TransactionStrategy& strategy,
             bool afterNU5) const;
@@ -387,7 +387,7 @@ private:
     IterateLimit(
             CWallet& wallet,
             const ZTXOSelector& selector,
-            const TransactionStrategy strategy,
+            const TransactionStrategy& strategy,
             CAmount sendAmount,
             CAmount dustThreshold,
             const SpendableInputs& spendable,
@@ -403,11 +403,11 @@ private:
     ResolveInputsAndPayments(
             CWallet& wallet,
             const ZTXOSelector& selector,
-            SpendableInputs& spendable,
+            SpendableInputs spendable,
             const std::vector<Payment>& payments,
             const CChain& chain,
             const TransactionStrategy& strategy,
-            std::optional<CAmount> fee,
+            const std::optional<CAmount>& fee,
             bool afterNU5) const;
     /**
      * Compute the internal and external OVKs to use in transaction construction, given
@@ -431,12 +431,12 @@ public:
     PrepareTransaction(
             CWallet& wallet,
             const ZTXOSelector& selector,
-            SpendableInputs& spendable,
+            const SpendableInputs& spendable,
             const Recipients& payments,
             const CChain& chain,
-            TransactionStrategy strategy,
+            const TransactionStrategy& strategy,
             /// A fixed fee is used if provided, otherwise it is calculated based on ZIP 317.
-            std::optional<CAmount> fee,
+            const std::optional<CAmount>& fee,
             uint32_t anchorConfirmations) const;
 };