Merge pull request #6568 from sellout/wallet_tx_builder-z_shieldcoinbase

Have z_shieldcoinbase use WalletTxBuilder

qa/rpc-tests/remove_sprout_shielding.py

@@ -34,64 +34,88 @@ HAS_CANOPY = [
 ]
 
 class RemoveSproutShieldingTest (BitcoinTestFramework):
+    def __init__(self):
+        super().__init__()
+        self.num_nodes = 4
+        self.cache_behavior = 'sprout'
 
     def setup_nodes(self):
         return start_nodes(self.num_nodes, self.options.tmpdir, extra_args=[HAS_CANOPY] * self.num_nodes)
 
-    def run_test (self):
+    def run_test(self):
-
         # Generate blocks up to Heartwood activation
         logging.info("Generating initial blocks. Current height is 200, advance to 210 (activate Heartwood but not Canopy)")
         self.nodes[0].generate(10)
         self.sync_all()
 
-        # Shield coinbase to Sprout on node 0. Should pass
+        n0_sprout_addr0 = self.nodes[0].listaddresses()[0]['sprout']['addresses'][0]
-        sprout_addr = self.nodes[0].z_getnewaddress('sprout')
+        n2_sprout_addr = self.nodes[2].listaddresses()[0]['sprout']['addresses'][0]
-        sprout_addr_node2 = self.nodes[2].z_getnewaddress('sprout')
+
-        myopid = self.nodes[0].z_shieldcoinbase(get_coinbase_address(self.nodes[0]), sprout_addr, 0)['opid']
+        # Attempt to shield coinbase to Sprout on node 0. Should fail;
-        wait_and_assert_operationid_status(self.nodes[0], myopid)
+        # transfers to Sprout are no longer supported
-        print("taddr -> Sprout z_shieldcoinbase tx accepted before Canopy on node 0")
+        try:
+            self.nodes[0].z_shieldcoinbase(get_coinbase_address(self.nodes[0]), n0_sprout_addr0, 0)['opid']
+        except JSONRPCException as e:
+            errorString = e.error['message'];
+        unsupported_sprout_msg = "Sending funds into the Sprout pool is no longer supported."
+        assert_equal(unsupported_sprout_msg, errorString)
 
         self.nodes[0].generate(1)
         self.sync_all()
-        assert_equal(self.nodes[0].z_getbalance(sprout_addr), Decimal('10'))
 
-        # Fund taddr_0 from shielded coinbase on node 0
+        # Check that we have the expected balance from the cached setup
-        taddr_0 = self.nodes[0].getnewaddress()
+        assert_equal(self.nodes[0].z_getbalance(n0_sprout_addr0), Decimal('50'))
+
+        # Fund n0_taddr0 from previously existing Sprout funds on node 0
+        n0_taddr0 = self.nodes[0].getnewaddress()
         for _ in range(3):
-            recipients = [{"address": taddr_0, "amount": Decimal('1')}]
+            recipients = [{"address": n0_taddr0, "amount": Decimal('1')}]
-            myopid = self.nodes[0].z_sendmany(sprout_addr, recipients, 1, 0, 'AllowRevealedRecipients')
+            myopid = self.nodes[0].z_sendmany(n0_sprout_addr0, recipients, 1, 0, 'AllowRevealedRecipients')
             wait_and_assert_operationid_status(self.nodes[0], myopid)
             self.sync_all()
             self.nodes[0].generate(1)
             self.sync_all()
+        assert_equal(self.nodes[0].z_getbalance(n0_taddr0), Decimal('3'))
 
-        # Create mergetoaddress taddr -> Sprout transaction and mine on node 0 before it is Canopy-aware. Should pass
+        # Create mergetoaddress taddr -> Sprout transaction and mine on node 0 before it is Canopy-aware. Should pass. This will spend the available funds in taddr0
-        merge_tx_0 = self.nodes[0].z_mergetoaddress(["ANY_TADDR"], self.nodes[1].z_getnewaddress('sprout'))
+        n1_sprout_addr0 = self.nodes[1].z_getnewaddress('sprout')
+        merge_tx_0 = self.nodes[0].z_mergetoaddress(["ANY_TADDR"], n1_sprout_addr0, 0)
         wait_and_assert_operationid_status(self.nodes[0], merge_tx_0['opid'])
         print("taddr -> Sprout z_mergetoaddress tx accepted before Canopy on node 0")
 
+        self.nodes[0].generate(1)
+        self.sync_all()
+        assert_equal(self.nodes[1].z_getbalance(n1_sprout_addr0), Decimal('3'))
+
+        # Send some funds back to n0_taddr0
+        recipients = [{"address": n0_taddr0, "amount": Decimal('1')}]
+        myopid = self.nodes[1].z_sendmany(n1_sprout_addr0, recipients, 1, 0, 'AllowRevealedRecipients')
+        wait_and_assert_operationid_status(self.nodes[1], myopid)
+
         # Mine to one block before Canopy activation on node 0; adding value
         # to the Sprout pool will fail now since the transaction must be
         # included in the next (or later) block, after Canopy has activated.
-        self.nodes[0].generate(5)
+        self.sync_all()
+        self.nodes[0].generate(4)
         self.sync_all()
         assert_equal(self.nodes[0].getblockchaininfo()['upgrades']['e9ff75a6']['status'], 'pending')
+        assert_equal(self.nodes[0].z_getbalance(n0_taddr0), Decimal('1'))
 
         # Shield coinbase to Sprout on node 0. Should fail
-        sprout_addr = self.nodes[0].z_getnewaddress('sprout')
+        n0_coinbase_taddr = get_coinbase_address(self.nodes[0])
+        n0_sprout_addr1 = self.nodes[0].z_getnewaddress('sprout')
         assert_raises_message(
             JSONRPCException,
-            "Sprout shielding is not supported after Canopy",
+            "Sending funds into the Sprout pool is no longer supported.",
             self.nodes[0].z_shieldcoinbase,
-            get_coinbase_address(self.nodes[0]), sprout_addr, 0)
+            n0_coinbase_taddr, n0_sprout_addr1, 0)
         print("taddr -> Sprout z_shieldcoinbase tx rejected at Canopy activation on node 0")
 
         # Create taddr -> Sprout z_sendmany transaction on node 0. Should fail
-        sprout_addr = self.nodes[1].z_getnewaddress('sprout')
+        n1_sprout_addr1 = self.nodes[1].z_getnewaddress('sprout')
-        recipients = [{"address": sprout_addr, "amount": Decimal('1')}]
+        recipients = [{"address": n1_sprout_addr1, "amount": Decimal('1')}]
-        myopid = self.nodes[0].z_sendmany(taddr_0, recipients, 1, 0)
+        myopid = self.nodes[0].z_sendmany(n0_taddr0, recipients, 1, 0)
-        wait_and_assert_operationid_status(self.nodes[0], myopid, "failed", "Sending funds into the Sprout pool is no longer supported.")
+        wait_and_assert_operationid_status(self.nodes[0], myopid, "failed", unsupported_sprout_msg)
         print("taddr -> Sprout z_sendmany tx rejected at Canopy activation on node 0")
 
         # Create z_mergetoaddress [taddr, Sprout] -> Sprout transaction on node 0. Should fail
@@ -130,7 +154,7 @@ class RemoveSproutShieldingTest (BitcoinTestFramework):
         self.sync_all()
 
         # Create z_mergetoaddress Sprout -> Sprout transaction on node 1. Should pass
-        merge_tx_2 = self.nodes[1].z_mergetoaddress(["ANY_SPROUT"], sprout_addr_node2)
+        merge_tx_2 = self.nodes[1].z_mergetoaddress(["ANY_SPROUT"], n2_sprout_addr)
         wait_and_assert_operationid_status(self.nodes[1], merge_tx_2['opid'])
         print("Sprout -> Sprout z_mergetoaddress tx accepted at NU5 activation on node 1")
 

qa/rpc-tests/wallet_shieldcoinbase.py

@@ -70,7 +70,7 @@ class WalletShieldCoinbaseTest (BitcoinTestFramework):
             self.nodes[2].z_shieldcoinbase(mytaddr, myzaddr)
         except JSONRPCException as e:
             errorString = e.error['message']
-        assert_equal("Could not find any coinbase funds to shield" in errorString, True)
+        assert_equal(errorString, "Invalid from address, no payment source found for address.")
 
         # Shielding will fail because fee is negative
         try:
@@ -84,14 +84,14 @@ class WalletShieldCoinbaseTest (BitcoinTestFramework):
             self.nodes[0].z_shieldcoinbase("*", myzaddr, Decimal('21000000.00000001'))
         except JSONRPCException as e:
             errorString = e.error['message']
-        assert_equal("Amount out of range" in errorString, True)
+        assert_equal(errorString, "Amount out of range")
 
         # Shielding will fail because fee is larger than sum of utxos
         try:
             self.nodes[0].z_shieldcoinbase("*", myzaddr, 999)
         except JSONRPCException as e:
             errorString = e.error['message']
-        assert_equal("Insufficient coinbase funds" in errorString, True)
+        assert_equal(errorString, "Insufficient funds: have 50.00, need 999.00.")
 
         # Shielding will fail because limit parameter must be at least 0
         try:
@@ -193,4 +193,4 @@ class WalletShieldCoinbaseTest (BitcoinTestFramework):
         self.sync_all()
 
 # Note, no "if __name__ == '__main__" and call the test here; it's called from
-# pool-specific derived classes in wallet_shieldcoinbase_*.py
+# pool-specific derived classes in wallet_shieldcoinbase_*.py

src/wallet/asyncrpcoperation_sendmany.cpp

@@ -155,6 +155,7 @@ AsyncRPCOperation_sendmany::main_impl(CWallet& wallet) {
 
     return preparedTx
         .map([&](const TransactionEffects& effects) {
+            effects.LockSpendable(wallet);
             try {
                 const auto& spendable = effects.GetSpendable();
                 const auto& payments = effects.GetPayments();

src/wallet/asyncrpcoperation_shieldcoinbase.cpp

@@ -18,7 +18,6 @@
 #include "proof_verifier.h"
 #include "rpc/protocol.h"
 #include "rpc/server.h"
-#include "transaction_builder.h"
 #include "timedata.h"
 #include "util/system.h"
 #include "util/moneystr.h"
@@ -29,7 +28,6 @@
 #include "util/match.h"
 #include "zcash/IncrementalMerkleTree.hpp"
 #include "miner.h"
-#include "wallet/paymentdisclosuredb.h"
 
 #include <array>
 #include <iostream>
@@ -43,60 +41,34 @@
 
 using namespace libzcash;
 
-static int 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_shieldcoinbase::AsyncRPCOperation_shieldcoinbase(
-        TransactionBuilder builder,
+        WalletTxBuilder builder,
-        CMutableTransaction contextualTx,
+        ZTXOSelector ztxoSelector,
-        std::vector<ShieldCoinbaseUTXO> inputs,
         PaymentAddress toAddress,
+        TransactionStrategy strategy,
+        int nUTXOLimit,
         CAmount fee,
         UniValue contextInfo) :
-        builder_(std::move(builder)), tx_(contextualTx), inputs_(inputs), fee_(fee), contextinfo_(contextInfo)
+    builder_(std::move(builder)),
+    ztxoSelector_(ztxoSelector),
+    toAddress_(toAddress),
+    strategy_(strategy),
+    nUTXOLimit_(nUTXOLimit),
+    fee_(fee),
+    contextinfo_(contextInfo)
 {
-    assert(contextualTx.nVersion >= 2);  // transaction format version must support vJoinSplit
+    assert(MoneyRange(fee));
+    assert(ztxoSelector.RequireSpendingKeys());
 
-    if (fee < 0 || fee > MAX_MONEY) {
+    examine(toAddress_, match {
-        throw JSONRPCError(RPC_INVALID_PARAMETER, "Fee is out of range");
+            [](const CKeyID&) {
-    }
+                throw JSONRPCError(RPC_VERIFY_REJECTED, "Cannot shield coinbase output to a p2pkh address.");
-
+            },
-    if (inputs.size() == 0) {
+            [](const CScriptID&) {
-        throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Empty inputs");
+                throw JSONRPCError(RPC_VERIFY_REJECTED, "Cannot shield coinbase output to a p2sh address.");
-    }
+            },
-
+            [](const auto&) { },
-    //  Check the destination address is valid for this network i.e. not testnet being used on mainnet
+        });
-    examine(toAddress, match {
-        [&](CKeyID addr) {
-            throw JSONRPCError(RPC_VERIFY_REJECTED, "Cannot shield coinbase output to a p2pkh address.");
-        },
-        [&](CScriptID addr) {
-            throw JSONRPCError(RPC_VERIFY_REJECTED, "Cannot shield coinbase output to a p2sh address.");
-        },
-        [&](libzcash::SaplingPaymentAddress addr) {
-            tozaddr_ = addr;
-        },
-        [&](libzcash::SproutPaymentAddress addr) {
-            tozaddr_ = addr;
-        },
-        [&](libzcash::UnifiedAddress addr) {
-            tozaddr_ = addr;
-        }
-    });
 
     // Log the context info
     if (LogAcceptCategory("zrpcunsafe")) {
@@ -104,34 +76,22 @@ AsyncRPCOperation_shieldcoinbase::AsyncRPCOperation_shieldcoinbase(
     } else {
         LogPrint("zrpc", "%s: z_shieldcoinbase initialized\n", getId());
     }
-
-    // Lock UTXOs
-    lock_utxos();
-
-    // Enable payment disclosure if requested
-    paymentDisclosureMode = fExperimentalPaymentDisclosure;
 }
 
 AsyncRPCOperation_shieldcoinbase::~AsyncRPCOperation_shieldcoinbase() {
 }
 
 void AsyncRPCOperation_shieldcoinbase::main() {
-    if (isCancelled()) {
-        unlock_utxos(); // clean up
-        return;
-    }
-
     set_state(OperationStatus::EXECUTING);
     start_execution_clock();
 
-    bool success = false;
-
 #ifdef ENABLE_MINING
     GenerateBitcoins(false, 0, Params());
 #endif
 
+    std::optional<uint256> txid;
     try {
-        success = main_impl();
+        txid = main_impl(*pwalletMain);
     } catch (const UniValue& objError) {
         int code = find_value(objError, "code").get_int();
         std::string message = find_value(objError, "message").get_str();
@@ -157,303 +117,141 @@ void AsyncRPCOperation_shieldcoinbase::main() {
 
     stop_execution_clock();
 
-    if (success) {
+    if (txid.has_value()) {
         set_state(OperationStatus::SUCCESS);
     } else {
         set_state(OperationStatus::FAILED);
     }
 
     std::string s = strprintf("%s: z_shieldcoinbase finished (status=%s", getId(), getStateAsString());
-    if (success) {
+    if (txid.has_value()) {
-        s += strprintf(", txid=%s)\n", tx_.GetHash().ToString());
+        s += strprintf(", txid=%s)\n", txid.value().GetHex());
     } else {
         s += strprintf(", error=%s)\n", getErrorMessage());
     }
     LogPrintf("%s",s);
+}
 
-    unlock_utxos(); // clean up
 
-    // !!! Payment disclosure START
+Remaining AsyncRPCOperation_shieldcoinbase::prepare(CWallet& wallet) {
-    if (success && paymentDisclosureMode && paymentDisclosureData_.size()>0) {
+    auto spendable = builder_.FindAllSpendableInputs(wallet, ztxoSelector_, COINBASE_MATURITY);
-        uint256 txidhash = tx_.GetHash();
+
-        std::shared_ptr<PaymentDisclosureDB> db = PaymentDisclosureDB::sharedInstance();
+    // Find unspent coinbase utxos and update estimated size
-        for (PaymentDisclosureKeyInfo p : paymentDisclosureData_) {
+    unsigned int max_tx_size = MAX_TX_SIZE_AFTER_SAPLING;
-            p.first.hash = txidhash;
+    CAmount shieldingValue = 0;
-            if (!db->Put(p.first, p.second)) {
+    CAmount remainingValue = 0;
-                LogPrint("paymentdisclosure", "%s: Payment Disclosure: Error writing entry to database for key %s\n", getId(), p.first.ToString());
+    size_t estimatedTxSize = 10000; // per ZIP 401 (https://zips.z.cash/zip-0401#specification)
+    size_t utxoCounter = 0;
+    size_t numUtxos = 0;
+    bool maxedOutFlag = false;
+
+    for (const COutput& out : spendable.utxos) {
+        auto scriptPubKey = out.tx->vout[out.i].scriptPubKey;
+        CAmount nValue = out.tx->vout[out.i].nValue;
+
+        CTxDestination address;
+        if (!ExtractDestination(scriptPubKey, address)) {
+            continue;
+        }
+
+        utxoCounter++;
+        if (!maxedOutFlag) {
+            size_t increase =
+                (std::get_if<CScriptID>(&address) != nullptr) ? CTXIN_SPEND_P2SH_SIZE : CTXIN_SPEND_P2PKH_SIZE;
+
+            if (estimatedTxSize + increase >= max_tx_size || (0 < nUTXOLimit_ && nUTXOLimit_ < utxoCounter)) {
+                maxedOutFlag = true;
             } else {
-                LogPrint("paymentdisclosure", "%s: Payment Disclosure: Successfully added entry to database for key %s\n", getId(), p.first.ToString());
+                estimatedTxSize += increase;
+                shieldingValue += nValue;
+                numUtxos++;
             }
         }
-    }
-    // !!! Payment disclosure END
-}
 
-bool AsyncRPCOperation_shieldcoinbase::main_impl() {
+        if (maxedOutFlag) {
-
+            remainingValue += nValue;
-    CAmount minersFee = fee_;
-
-    size_t numInputs = inputs_.size();
-
-    CAmount targetAmount = 0;
-    for (ShieldCoinbaseUTXO & utxo : inputs_) {
-        targetAmount += utxo.amount;
-    }
-
-    if (targetAmount <= minersFee) {
-        throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS,
-            strprintf("Insufficient coinbase funds, have %s and miners fee is %s",
-            FormatMoney(targetAmount), FormatMoney(minersFee)));
-    }
-
-    CAmount sendAmount = targetAmount - minersFee;
-    LogPrint("zrpc", "%s: spending %s to shield %s with fee %s\n",
-            getId(), FormatMoney(targetAmount), FormatMoney(sendAmount), FormatMoney(minersFee));
-
-    return std::visit(ShieldToAddress(this, sendAmount), tozaddr_);
-}
-
-void ShieldToAddress::shieldToAddress(const libzcash::RecipientAddress& recipient, AsyncRPCOperation_shieldcoinbase *m_op) {
-    m_op->builder_.SetFee(m_op->fee_);
-
-    // Sending from a t-address, which we don't have an ovk for. Instead,
-    // generate a common one from the HD seed. This ensures the data is
-    // recoverable, while keeping it logically separate from the ZIP 32
-    // Sapling key hierarchy, which the user might not be using.
-    // FIXME: update to use the ZIP-316 OVK (#5511)
-    HDSeed seed = pwalletMain->GetHDSeedForRPC();
-    uint256 ovk = ovkForShieldingFromTaddr(seed);
-
-    // Add transparent inputs
-    for (auto t : m_op->inputs_) {
-        m_op->builder_.AddTransparentInput(COutPoint(t.txid, t.vout), t.scriptPubKey, t.amount);
-    }
-
-    // Send all value to the target recipient
-    m_op->builder_.SendChangeTo(recipient, ovk);
-
-    // Build the transaction
-    m_op->tx_ = m_op->builder_.Build().GetTxOrThrow();
-}
-
-bool ShieldToAddress::operator()(const CKeyID &addr) const {
-    return false;
-}
-
-bool ShieldToAddress::operator()(const CScriptID &addr) const {
-    return false;
-}
-
-bool ShieldToAddress::operator()(const libzcash::SproutPaymentAddress &zaddr) const {
-    // update the transaction with these inputs
-    CMutableTransaction rawTx(m_op->tx_);
-    for (ShieldCoinbaseUTXO & t : m_op->inputs_) {
-        CTxIn in(COutPoint(t.txid, t.vout));
-        rawTx.vin.push_back(in);
-    }
-    m_op->tx_ = CTransaction(rawTx);
-
-    // Prepare raw transaction to handle JoinSplits
-    CMutableTransaction mtx(m_op->tx_);
-    ed25519::generate_keypair(m_op->joinSplitPrivKey_, m_op->joinSplitPubKey_);
-    mtx.joinSplitPubKey = m_op->joinSplitPubKey_;
-    m_op->tx_ = CTransaction(mtx);
-
-    // Create joinsplit
-    ShieldCoinbaseJSInfo info;
-    info.vpub_old = sendAmount;
-    info.vpub_new = 0;
-    JSOutput jso = JSOutput(zaddr, sendAmount);
-    info.vjsout.push_back(jso);
-    UniValue obj = m_op->perform_joinsplit(info);
-
-    auto txAndResult = SignSendRawTransaction(obj, std::nullopt, m_op->testmode);
-    m_op->tx_ = txAndResult.first;
-    m_op->set_result(txAndResult.second);
-    return true;
-}
-
-bool ShieldToAddress::operator()(const libzcash::SaplingPaymentAddress &zaddr) const {
-    ShieldToAddress::shieldToAddress(zaddr, m_op);
-
-    std::vector<RecipientMapping> recipientMappings;
-    UniValue sendResult = SendTransaction(m_op->tx_, recipientMappings, std::nullopt, m_op->testmode);
-    m_op->set_result(sendResult);
-
-    return true;
-}
-
-bool ShieldToAddress::operator()(const libzcash::UnifiedAddress &uaddr) const {
-    // TODO check if an Orchard address is present, send to it if so.
-    const auto receiver{uaddr.GetSaplingReceiver()};
-    if (receiver.has_value()) {
-        ShieldToAddress::shieldToAddress(receiver.value(), m_op);
-
-        std::vector<RecipientMapping> recipientMappings = {RecipientMapping(uaddr, receiver.value())};
-        UniValue sendResult = SendTransaction(m_op->tx_, recipientMappings, std::nullopt, m_op->testmode);
-        m_op->set_result(sendResult);
-
-        return true;
-    }
-    // This UA must contain a transparent address, which can't be the destination of coinbase shielding.
-    return false;
-}
-
-UniValue AsyncRPCOperation_shieldcoinbase::perform_joinsplit(ShieldCoinbaseJSInfo & info) {
-    uint32_t consensusBranchId;
-    uint256 anchor;
-    {
-        LOCK(cs_main);
-        consensusBranchId = CurrentEpochBranchId(chainActive.Height() + 1, Params().GetConsensus());
-        anchor = pcoinsTip->GetBestAnchor(SPROUT);
-    }
-
-
-    if (anchor.IsNull()) {
-        throw std::runtime_error("anchor is null");
-    }
-
-    // Make sure there are two inputs and two outputs
-    while (info.vjsin.size() < ZC_NUM_JS_INPUTS) {
-        info.vjsin.push_back(JSInput());
-    }
-
-    while (info.vjsout.size() < ZC_NUM_JS_OUTPUTS) {
-        info.vjsout.push_back(JSOutput());
-    }
-
-    if (info.vjsout.size() != ZC_NUM_JS_INPUTS || info.vjsin.size() != ZC_NUM_JS_OUTPUTS) {
-        throw runtime_error("unsupported joinsplit input/output counts");
-    }
-
-    CMutableTransaction mtx(tx_);
-
-    LogPrint("zrpcunsafe", "%s: creating joinsplit at index %d (vpub_old=%s, vpub_new=%s, in[0]=%s, in[1]=%s, out[0]=%s, out[1]=%s)\n",
-            getId(),
-            tx_.vJoinSplit.size(),
-            FormatMoney(info.vpub_old), FormatMoney(info.vpub_new),
-            FormatMoney(info.vjsin[0].note.value()), FormatMoney(info.vjsin[1].note.value()),
-            FormatMoney(info.vjsout[0].value), FormatMoney(info.vjsout[1].value)
-            );
-
-    // Generate the proof, this can take over a minute.
-    std::array<libzcash::JSInput, ZC_NUM_JS_INPUTS> inputs
-            {info.vjsin[0], info.vjsin[1]};
-    std::array<libzcash::JSOutput, ZC_NUM_JS_OUTPUTS> outputs
-            {info.vjsout[0], info.vjsout[1]};
-    std::array<size_t, ZC_NUM_JS_INPUTS> inputMap;
-    std::array<size_t, ZC_NUM_JS_OUTPUTS> outputMap;
-
-    uint256 esk; // payment disclosure - secret
-
-    assert(mtx.fOverwintered && (mtx.nVersion >= SAPLING_TX_VERSION));
-    JSDescription jsdesc = JSDescriptionInfo(
-            joinSplitPubKey_,
-            anchor,
-            inputs,
-            outputs,
-            info.vpub_old,
-            info.vpub_new
-    ).BuildRandomized(
-            inputMap,
-            outputMap,
-            !this->testmode,
-            &esk); // parameter expects pointer to esk, so pass in address
-    {
-        auto verifier = ProofVerifier::Strict();
-        if (!(verifier.VerifySprout(jsdesc, joinSplitPubKey_))) {
-            throw std::runtime_error("error verifying joinsplit");
         }
     }
 
-    mtx.vJoinSplit.push_back(jsdesc);
+    spendable.LimitTransparentUtxos(numUtxos);
 
-    // Empty output script.
+    // TODO: Don’t check this outside of `PrepareTransaction`
-    CScript scriptCode;
+    if (shieldingValue < fee_) {
-    CTransaction signTx(mtx);
+        ThrowInputSelectionError(
-    std::vector<CTxOut> allPrevOutputs;
+                InvalidFundsError(shieldingValue, InsufficientFundsError(fee_)),
-    for (ShieldCoinbaseUTXO & t : inputs_) {
+                ztxoSelector_,
-        allPrevOutputs.emplace_back(t.amount, t.scriptPubKey);
+                strategy_);
-    }
-    PrecomputedTransactionData txdata(signTx, allPrevOutputs);
-    uint256 dataToBeSigned = SignatureHash(scriptCode, signTx, NOT_AN_INPUT, SIGHASH_ALL, 0, consensusBranchId, txdata);
-
-    // Add the signature
-    ed25519::sign(
-        joinSplitPrivKey_,
-        {dataToBeSigned.begin(), 32},
-        mtx.joinSplitSig);
-
-    // Sanity check
-    if (!ed25519::verify(
-        mtx.joinSplitPubKey,
-        mtx.joinSplitSig,
-        {dataToBeSigned.begin(), 32}))
-    {
-        throw std::runtime_error("ed25519_verify failed");
     }
 
-    CTransaction rawTx(mtx);
+    // FIXME: This should be internal, and shouldn’t be a payment, but more like a change address,
-    tx_ = rawTx;
+    //        as we don’t know the amount at this point.
+    std::vector<Payment> payments = { Payment(toAddress_, shieldingValue - fee_, std::nullopt) };
 
-    CDataStream ss(SER_NETWORK, PROTOCOL_VERSION);
+    auto preparationResult = builder_.PrepareTransaction(
-    ss << rawTx;
+            wallet,
+            ztxoSelector_,
+            spendable,
+            payments,
+            chainActive,
+            strategy_,
+            fee_,
+            nAnchorConfirmations);
 
-    std::string encryptedNote1;
+    preparationResult
-    std::string encryptedNote2;
+        .map_error([&](const InputSelectionError& err) {
-    {
+            ThrowInputSelectionError(err, ztxoSelector_, strategy_);
-        CDataStream ss2(SER_NETWORK, PROTOCOL_VERSION);
+        })
-        ss2 << ((unsigned char) 0x00);
+        .map([&](const TransactionEffects& effects) {
-        ss2 << jsdesc.ephemeralKey;
+            effects.LockSpendable(wallet);
-        ss2 << jsdesc.ciphertexts[0];
+            effects_ = effects;
-        ss2 << ZCJoinSplit::h_sig(jsdesc.randomSeed, jsdesc.nullifiers, joinSplitPubKey_);
+        });
 
-        encryptedNote1 = HexStr(ss2.begin(), ss2.end());
+    return Remaining(utxoCounter, numUtxos, remainingValue, shieldingValue);
+}
+
+uint256 AsyncRPCOperation_shieldcoinbase::main_impl(CWallet& wallet) {
+    uint256 txid;
+
+    try {
+        const auto& spendable = effects_->GetSpendable();
+        const auto& payments = effects_->GetPayments();
+        spendable.LogInputs(getId());
+
+        LogPrint("zrpcunsafe", "%s: spending %s to send %s with fee %s\n", getId(),
+                 FormatMoney(payments.Total()),
+                 FormatMoney(spendable.Total()),
+                 FormatMoney(effects_->GetFee()));
+        LogPrint("zrpc", "%s: total transparent input: %s (to choose from)\n", getId(),
+                 FormatMoney(spendable.GetTransparentTotal()));
+        LogPrint("zrpcunsafe", "%s: total shielded input: %s (to choose from)\n", getId(),
+                 FormatMoney(spendable.GetSaplingTotal() + spendable.GetOrchardTotal()));
+        LogPrint("zrpc", "%s: total transparent output: %s\n", getId(),
+                 FormatMoney(payments.GetTransparentTotal()));
+        LogPrint("zrpcunsafe", "%s: total shielded Sapling output: %s\n", getId(),
+                 FormatMoney(payments.GetSaplingTotal()));
+        LogPrint("zrpcunsafe", "%s: total shielded Orchard output: %s\n", getId(),
+                 FormatMoney(payments.GetOrchardTotal()));
+        LogPrint("zrpc", "%s: fee: %s\n", getId(), FormatMoney(effects_->GetFee()));
+
+        auto buildResult = effects_->ApproveAndBuild(
+                Params().GetConsensus(),
+                wallet,
+                chainActive,
+                strategy_);
+
+        auto tx = buildResult.GetTxOrThrow();
+
+        UniValue sendResult = SendTransaction(tx, payments.GetResolvedPayments(), std::nullopt, testmode);
+        set_result(sendResult);
+
+        txid = tx.GetHash();
+
+        effects_->UnlockSpendable(wallet);
+        return txid;
+    } catch (...) {
+        effects_->UnlockSpendable(wallet);
+        throw;
     }
-    {
-        CDataStream ss2(SER_NETWORK, PROTOCOL_VERSION);
-        ss2 << ((unsigned char) 0x01);
-        ss2 << jsdesc.ephemeralKey;
-        ss2 << jsdesc.ciphertexts[1];
-        ss2 << ZCJoinSplit::h_sig(jsdesc.randomSeed, jsdesc.nullifiers, joinSplitPubKey_);
-
-        encryptedNote2 = HexStr(ss2.begin(), ss2.end());
-    }
-
-    UniValue arrInputMap(UniValue::VARR);
-    UniValue arrOutputMap(UniValue::VARR);
-    for (size_t i = 0; i < ZC_NUM_JS_INPUTS; i++) {
-        arrInputMap.push_back(static_cast<uint64_t>(inputMap[i]));
-    }
-    for (size_t i = 0; i < ZC_NUM_JS_OUTPUTS; i++) {
-        arrOutputMap.push_back(static_cast<uint64_t>(outputMap[i]));
-    }
-
-    KeyIO keyIO(Params());
-
-    // !!! Payment disclosure START
-    size_t js_index = tx_.vJoinSplit.size() - 1;
-    uint256 placeholder;
-    for (int i = 0; i < ZC_NUM_JS_OUTPUTS; i++) {
-        uint8_t mapped_index = outputMap[i];
-        // placeholder for txid will be filled in later when tx has been finalized and signed.
-        PaymentDisclosureKey pdKey = {placeholder, js_index, mapped_index};
-        JSOutput output = outputs[mapped_index];
-        libzcash::SproutPaymentAddress zaddr = output.addr;  // randomized output
-        PaymentDisclosureInfo pdInfo = {PAYMENT_DISCLOSURE_VERSION_EXPERIMENTAL, esk, joinSplitPrivKey_, zaddr};
-        paymentDisclosureData_.push_back(PaymentDisclosureKeyInfo(pdKey, pdInfo));
-
-        LogPrint("paymentdisclosure", "%s: Payment Disclosure: js=%d, n=%d, zaddr=%s\n", getId(), js_index, int(mapped_index), keyIO.EncodePaymentAddress(zaddr));
-    }
-    // !!! Payment disclosure END
-
-    UniValue obj(UniValue::VOBJ);
-    obj.pushKV("encryptednote1", encryptedNote1);
-    obj.pushKV("encryptednote2", encryptedNote2);
-    obj.pushKV("rawtxn", HexStr(ss.begin(), ss.end()));
-    obj.pushKV("inputmap", arrInputMap);
-    obj.pushKV("outputmap", arrOutputMap);
-    return obj;
 }
 
 /**
@@ -470,25 +268,3 @@ UniValue AsyncRPCOperation_shieldcoinbase::getStatus() const {
     obj.pushKV("params", contextinfo_ );
     return obj;
 }
-
-/**
- * Lock input utxos
- */
- void AsyncRPCOperation_shieldcoinbase::lock_utxos() {
-    LOCK2(cs_main, pwalletMain->cs_wallet);
-    for (auto utxo : inputs_) {
-        COutPoint outpt(utxo.txid, utxo.vout);
-        pwalletMain->LockCoin(outpt);
-    }
-}
-
-/**
- * Unlock input utxos
- */
-void AsyncRPCOperation_shieldcoinbase::unlock_utxos() {
-    LOCK2(cs_main, pwalletMain->cs_wallet);
-    for (auto utxo : inputs_) {
-        COutPoint outpt(utxo.txid, utxo.vout);
-        pwalletMain->UnlockCoin(outpt);
-    }
-}

src/wallet/asyncrpcoperation_shieldcoinbase.h

@@ -12,7 +12,7 @@
 #include "zcash/JoinSplit.hpp"
 #include "zcash/Address.hpp"
 #include "wallet.h"
-#include "wallet/paymentdisclosure.h"
+#include "wallet/wallet_tx_builder.h"
 
 #include <unordered_map>
 #include <tuple>
@@ -23,29 +23,45 @@
 
 using namespace libzcash;
 
-struct ShieldCoinbaseUTXO {
+/**
-    uint256 txid;
+When estimating the number of coinbase utxos we can shield in a single transaction:
-    int vout;
+1. An Orchard receiver is 9165 bytes.
-    CScript scriptPubKey;
+2. Transaction overhead ~ 100 bytes
-    CAmount amount;
+3. Spending a typical P2PKH is >=148 bytes, as defined in CTXIN_SPEND_P2PKH_SIZE.
-};
+4. Spending a multi-sig P2SH address can vary greatly:
+   https://github.com/bitcoin/bitcoin/blob/c3ad56f4e0b587d8d763af03d743fdfc2d180c9b/src/main.cpp#L517
+   In real-world coinbase utxos, we consider a 3-of-3 multisig, where the size is roughly:
+    (3*(33+1))+3 = 105 byte redeem script
+    105 + 1 + 3*(73+1) = 328 bytes of scriptSig, rounded up to 400 based on testnet experiments.
+*/
+#define CTXIN_SPEND_P2SH_SIZE 400
 
-// Package of info which is passed to perform_joinsplit methods.
+#define SHIELD_COINBASE_DEFAULT_LIMIT 50
-struct ShieldCoinbaseJSInfo
+
-{
+// transaction.h comment: spending taddr output requires CTxIn >= 148 bytes and
-    std::vector<JSInput> vjsin;
+// typical taddr txout is 34 bytes
-    std::vector<JSOutput> vjsout;
+#define CTXIN_SPEND_P2PKH_SIZE   148
-    CAmount vpub_old = 0;
+#define CTXOUT_REGULAR_SIZE     34
-    CAmount vpub_new = 0;
+
+class Remaining {
+public:
+    Remaining(size_t utxoCounter, size_t numUtxos, CAmount remainingValue, CAmount shieldingValue):
+        utxoCounter(utxoCounter), numUtxos(numUtxos), remainingValue(remainingValue), shieldingValue(shieldingValue) { }
+
+    size_t utxoCounter;
+    size_t numUtxos;
+    CAmount remainingValue;
+    CAmount shieldingValue;
 };
 
 class AsyncRPCOperation_shieldcoinbase : public AsyncRPCOperation {
 public:
     AsyncRPCOperation_shieldcoinbase(
-        TransactionBuilder builder,
+        WalletTxBuilder builder,
-        CMutableTransaction contextualTx,
+        ZTXOSelector ztxoSelector,
-        std::vector<ShieldCoinbaseUTXO> inputs,
         PaymentAddress toAddress,
+        TransactionStrategy strategy,
+        int nUTXOLimit,
         CAmount fee = DEFAULT_FEE,
         UniValue contextInfo = NullUniValue);
     virtual ~AsyncRPCOperation_shieldcoinbase();
@@ -56,63 +72,30 @@ public:
     AsyncRPCOperation_shieldcoinbase& operator=(AsyncRPCOperation_shieldcoinbase const&) = delete;  // Copy assign
     AsyncRPCOperation_shieldcoinbase& operator=(AsyncRPCOperation_shieldcoinbase &&) = delete;      // Move assign
 
+    Remaining prepare(CWallet& wallet);
+
     virtual void main();
 
     virtual UniValue getStatus() const;
 
-    bool testmode = false;  // Set to true to disable sending txs and generating proofs
+    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.
 
 private:
-    friend class ShieldToAddress;
     friend class TEST_FRIEND_AsyncRPCOperation_shieldcoinbase;    // class for unit testing
 
+    WalletTxBuilder builder_;
+    ZTXOSelector ztxoSelector_;
+    PaymentAddress toAddress_;
+    TransactionStrategy strategy_;
+    int nUTXOLimit_;
+    CAmount fee_;
+    std::optional<TransactionEffects> effects_;
+
     UniValue contextinfo_;     // optional data to include in return value from getStatus()
 
-    CAmount fee_;
+    uint256 main_impl(CWallet& wallet);
-    PaymentAddress tozaddr_;
-
-    ed25519::VerificationKey joinSplitPubKey_;
-    ed25519::SigningKey joinSplitPrivKey_;
-
-    std::vector<ShieldCoinbaseUTXO> inputs_;
-
-    TransactionBuilder builder_;
-    CTransaction tx_;
-
-    bool main_impl();
-
-    // JoinSplit without any input notes to spend
-    UniValue perform_joinsplit(ShieldCoinbaseJSInfo &);
-
-    void lock_utxos();
-
-    void unlock_utxos();
-
-    // payment disclosure!
-    std::vector<PaymentDisclosureKeyInfo> paymentDisclosureData_;
 };
 
-class ShieldToAddress
-{
-private:
-    AsyncRPCOperation_shieldcoinbase *m_op;
-    CAmount sendAmount;
-
-    static void shieldToAddress(const libzcash::RecipientAddress& recipient, AsyncRPCOperation_shieldcoinbase *m_op);
-public:
-    ShieldToAddress(AsyncRPCOperation_shieldcoinbase *op, CAmount sendAmount) :
-        m_op(op), sendAmount(sendAmount) {}
-
-    bool operator()(const CKeyID &zaddr) const;
-    bool operator()(const CScriptID &zaddr) const;
-    bool operator()(const libzcash::SproutPaymentAddress &zaddr) const;
-    bool operator()(const libzcash::SaplingPaymentAddress &zaddr) const;
-    bool operator()(const libzcash::UnifiedAddress &uaddr) const;
-};
-
-
 // To test private methods, a friend class can act as a proxy
 class TEST_FRIEND_AsyncRPCOperation_shieldcoinbase {
 public:
@@ -120,22 +103,10 @@ public:
 
     TEST_FRIEND_AsyncRPCOperation_shieldcoinbase(std::shared_ptr<AsyncRPCOperation_shieldcoinbase> ptr) : delegate(ptr) {}
 
-    CTransaction getTx() {
-        return delegate->tx_;
-    }
-
-    void setTx(CTransaction tx) {
-        delegate->tx_ = tx;
-    }
-
     // Delegated methods
 
-    bool main_impl() {
+    uint256 main_impl(CWallet& wallet) {
-        return delegate->main_impl();
+        return delegate->main_impl(wallet);
-    }
-
-    UniValue perform_joinsplit(ShieldCoinbaseJSInfo &info) {
-        return delegate->perform_joinsplit(info);
     }
 
     void set_state(OperationStatus state) {
@@ -145,4 +116,3 @@ public:
 
 
 #endif // ZCASH_WALLET_ASYNCRPCOPERATION_SHIELDCOINBASE_H
-

src/wallet/gtest/test_rpc_wallet.cpp

@@ -21,69 +21,6 @@ bool find_error(const UniValue& objError, const std::string& expected) {
     return find_value(objError, "message").get_str().find(expected) != string::npos;
 }
 
-TEST(WalletRPCTests,ZShieldCoinbaseInternals)
-{
-    LoadProofParameters();
-
-    SelectParams(CBaseChainParams::TESTNET);
-    const Consensus::Params& consensusParams = Params().GetConsensus();
-
-    // we need to use pwalletMain because of AsyncRPCOperation_shieldcoinbase
-    LoadGlobalWallet();
-    {
-    LOCK2(cs_main, pwalletMain->cs_wallet);
-
-    // Mutable tx containing contextual information we need to build tx
-    // We removed the ability to create pre-Sapling Sprout proofs, so we can
-    // only create Sapling-onwards transactions.
-    int nHeight = consensusParams.vUpgrades[Consensus::UPGRADE_SAPLING].nActivationHeight;
-    CMutableTransaction mtx = CreateNewContextualCMutableTransaction(consensusParams, nHeight + 1, false);
-
-    // Add keys manually
-    auto pa = pwalletMain->GenerateNewSproutZKey();
-
-    // Insufficient funds
-    {
-        std::vector<ShieldCoinbaseUTXO> inputs = { ShieldCoinbaseUTXO{uint256(),0,0} };
-        std::shared_ptr<AsyncRPCOperation> operation(new AsyncRPCOperation_shieldcoinbase(TransactionBuilder(), mtx, inputs, pa) );
-        operation->main();
-        EXPECT_TRUE(operation->isFailed());
-        std::string msg = operation->getErrorMessage();
-        EXPECT_TRUE(msg.find("Insufficient coinbase funds") != string::npos);
-    }
-
-    // Test the perform_joinsplit methods.
-    {
-        // Dummy input so the operation object can be instantiated.
-        std::vector<ShieldCoinbaseUTXO> inputs = { ShieldCoinbaseUTXO{uint256(),0,100000} };
-        std::shared_ptr<AsyncRPCOperation> operation(new AsyncRPCOperation_shieldcoinbase(TransactionBuilder(), mtx, inputs, pa) );
-        std::shared_ptr<AsyncRPCOperation_shieldcoinbase> ptr = std::dynamic_pointer_cast<AsyncRPCOperation_shieldcoinbase> (operation);
-        TEST_FRIEND_AsyncRPCOperation_shieldcoinbase proxy(ptr);
-        static_cast<AsyncRPCOperation_shieldcoinbase *>(operation.get())->testmode = true;
-
-        ShieldCoinbaseJSInfo info;
-        info.vjsin.push_back(JSInput());
-        info.vjsin.push_back(JSInput());
-        info.vjsin.push_back(JSInput());
-        try {
-            proxy.perform_joinsplit(info);
-        } catch (const std::runtime_error & e) {
-            EXPECT_TRUE(string(e.what()).find("unsupported joinsplit input") != string::npos);
-        }
-
-        info.vjsin.clear();
-        try {
-            proxy.perform_joinsplit(info);
-        } catch (const std::runtime_error & e) {
-            EXPECT_TRUE(string(e.what()).find("error verifying joinsplit") != string::npos);
-        }
-    }
-
-    }
-    UnloadGlobalWallet();
-}
-
-
 // TODO: test private methods
 TEST(WalletRPCTests, RPCZMergeToAddressInternals)
 {

src/wallet/rpcwallet.cpp

@@ -4700,10 +4700,6 @@ UniValue z_getoperationstatus_IMPL(const UniValue& params, bool fRemoveFinishedO
     return ret;
 }
 
-// transaction.h comment: spending taddr output requires CTxIn >= 148 bytes and typical taddr txout is 34 bytes
-#define CTXIN_SPEND_DUST_SIZE   148
-#define CTXOUT_REGULAR_SIZE     34
-
 size_t EstimateTxSize(
         const ZTXOSelector& ztxoSelector,
         const std::vector<Payment>& recipients,
@@ -4762,7 +4758,7 @@ size_t EstimateTxSize(
     CTransaction tx(mtx);
     txsize += GetSerializeSize(tx, SER_NETWORK, tx.nVersion);
     if (fromTaddr) {
-        txsize += CTXIN_SPEND_DUST_SIZE;
+        txsize += CTXIN_SPEND_P2PKH_SIZE;
         txsize += CTXOUT_REGULAR_SIZE; // There will probably be taddr change
     }
     txsize += CTXOUT_REGULAR_SIZE * taddrRecipientCount;
@@ -5030,7 +5026,7 @@ UniValue z_sendmany(const UniValue& params, bool fHelp)
 
     // Create operation and add to global queue
     auto nAnchorDepth = std::min((unsigned int) nMinDepth, nAnchorConfirmations);
-    WalletTxBuilder builder(Params(), minRelayTxFee);
+    WalletTxBuilder builder(chainparams, minRelayTxFee);
 
     std::shared_ptr<AsyncRPCQueue> q = getAsyncRPCQueue();
     std::shared_ptr<AsyncRPCOperation> operation(
@@ -5174,29 +5170,14 @@ UniValue z_getmigrationstatus(const UniValue& params, bool fHelp) {
     return migrationStatus;
 }
 
-/**
-When estimating the number of coinbase utxos we can shield in a single transaction:
-1. Joinsplit description is 1802 bytes.
-2. Transaction overhead ~ 100 bytes
-3. Spending a typical P2PKH is >=148 bytes, as defined in CTXIN_SPEND_DUST_SIZE.
-4. Spending a multi-sig P2SH address can vary greatly:
-   https://github.com/bitcoin/bitcoin/blob/c3ad56f4e0b587d8d763af03d743fdfc2d180c9b/src/main.cpp#L517
-   In real-world coinbase utxos, we consider a 3-of-3 multisig, where the size is roughly:
-    (3*(33+1))+3 = 105 byte redeem script
-    105 + 1 + 3*(73+1) = 328 bytes of scriptSig, rounded up to 400 based on testnet experiments.
-*/
-#define CTXIN_SPEND_P2SH_SIZE 400
-
-#define SHIELD_COINBASE_DEFAULT_LIMIT 50
-
 UniValue z_shieldcoinbase(const UniValue& params, bool fHelp)
 {
     if (!EnsureWalletIsAvailable(fHelp))
         return NullUniValue;
 
-    if (fHelp || params.size() < 2 || params.size() > 4)
+    if (fHelp || params.size() < 2 || params.size() > 5)
         throw runtime_error(
-            "z_shieldcoinbase \"fromaddress\" \"tozaddress\" ( fee ) ( limit )\n"
+            "z_shieldcoinbase \"fromaddress\" \"tozaddress\" ( fee ) ( limit ) ( privacyPolicy )\n"
             "\nShield transparent coinbase funds by sending to a shielded zaddr.  This is an asynchronous operation and utxos"
             "\nselected for shielding will be locked.  If there is an error, they are unlocked.  The RPC call `listlockunspent`"
             "\ncan be used to return a list of locked utxos.  The number of coinbase utxos selected for shielding can be limited"
@@ -5211,12 +5192,15 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp)
             + strprintf("%s", FormatMoney(DEFAULT_FEE)) + ") The fee amount to attach to this transaction.\n"
             "4. limit                 (numeric, optional, default="
             + strprintf("%d", SHIELD_COINBASE_DEFAULT_LIMIT) + ") Limit on the maximum number of utxos to shield.  Set to 0 to use as many as will fit in the transaction.\n"
+            "5. privacyPolicy         (string, optional, default=\"AllowRevealedSenders\") Policy for what information leakage is acceptable.\n"
+            "                         This allows the same values as z_sendmany, but only \"AllowRevealedSenders\" and \"AllowLinkingAccountAddresses\"\n"
+            "                         are relevant.\n"
             "\nResult:\n"
             "{\n"
-            "  \"remainingUTXOs\": xxx       (numeric) Number of coinbase utxos still available for shielding.\n"
+            "  \"remainingUTXOs\": xxx    (numeric) Number of coinbase utxos still available for shielding.\n"
-            "  \"remainingValue\": xxx       (numeric) Value of coinbase utxos still available for shielding.\n"
+            "  \"remainingValue\": xxx    (numeric) Value of coinbase utxos still available for shielding.\n"
-            "  \"shieldingUTXOs\": xxx        (numeric) Number of coinbase utxos being shielded.\n"
+            "  \"shieldingUTXOs\": xxx    (numeric) Number of coinbase utxos being shielded.\n"
-            "  \"shieldingValue\": xxx        (numeric) Value of coinbase utxos being shielded.\n"
+            "  \"shieldingValue\": xxx    (numeric) Value of coinbase utxos being shielded.\n"
             "  \"opid\": xxx          (string) An operationid to pass to z_getoperationstatus to get the result of the operation.\n"
             "}\n"
             "\nExamples:\n"
@@ -5227,6 +5211,25 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp)
     LOCK2(cs_main, pwalletMain->cs_wallet);
 
     ThrowIfInitialBlockDownload();
+    const auto chainparams = Params();
+    const auto consensus = chainparams.GetConsensus();
+    int nextBlockHeight = chainActive.Height() + 1;
+
+    // This API cannot be used to create coinbase shielding transactions before Sapling
+    // activation.
+    if (!consensus.NetworkUpgradeActive(nextBlockHeight, Consensus::UPGRADE_SAPLING)) {
+        throw JSONRPCError(
+            RPC_INVALID_PARAMETER, "Cannot create shielded transactions before Sapling has activated");
+    }
+
+    auto strategy =
+        ResolveTransactionStrategy(
+                ReifyPrivacyPolicy(
+                        PrivacyPolicy::AllowRevealedSenders,
+                        params.size() > 4 ? std::optional(params[4].get_str()) : std::nullopt),
+                // This has identical behavior to `AllowRevealedSenders` for this operation, but
+                // technically, this is what “LegacyCompat” means, so just for consistency.
+                PrivacyPolicy::AllowFullyTransparent);
 
     // Validate the from address
     auto fromaddress = params[0].get_str();
@@ -5235,53 +5238,58 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp)
     KeyIO keyIO(Params());
 
     // Set of source addresses to filter utxos by
-    std::set<CTxDestination> sources = {};
+    ZTXOSelector ztxoSelector = [&]() {
-    if (!isFromWildcard) {
+        if (isFromWildcard) {
-        CTxDestination taddr = keyIO.DecodeDestination(fromaddress);
+            return CWallet::LegacyTransparentZTXOSelector(true, TransparentCoinbasePolicy::Require);
-        if (IsValidDestination(taddr)) {
-            sources.insert(taddr);
         } else {
-            throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid from address, should be a taddr or \"*\".");
+            auto decoded = keyIO.DecodePaymentAddress(fromaddress);
-        }
+            if (!decoded.has_value()) {
-    }
+                throw JSONRPCError(
+                        RPC_INVALID_ADDRESS_OR_KEY,
+                        "Invalid from address: should be a taddr, zaddr, UA, or the string 'ANY_TADDR'.");
+            }
 
-    int nextBlockHeight = chainActive.Height() + 1;
+            auto ztxoSelectorOpt = pwalletMain->ZTXOSelectorForAddress(
-    const bool canopyActive = Params().GetConsensus().NetworkUpgradeActive(nextBlockHeight, Consensus::UPGRADE_CANOPY);
+                decoded.value(),
+                true,
+                TransparentCoinbasePolicy::Require,
+                strategy.AllowLinkingAccountAddresses());
+
+            if (!ztxoSelectorOpt.has_value()) {
+                throw JSONRPCError(
+                        RPC_INVALID_ADDRESS_OR_KEY,
+                        "Invalid from address, no payment source found for address.");
+            }
+
+            auto selectorAccount = pwalletMain->FindAccountForSelector(ztxoSelectorOpt.value());
+            examine(decoded.value(), match {
+                [&](const libzcash::UnifiedAddress&) {
+                    if (!selectorAccount.has_value() || selectorAccount.value() == ZCASH_LEGACY_ACCOUNT) {
+                        throw JSONRPCError(
+                                RPC_INVALID_ADDRESS_OR_KEY,
+                                "Invalid from address, UA does not correspond to a known account.");
+                    }
+                },
+                [&](const auto&) {
+                    if (selectorAccount.has_value() && selectorAccount.value() != ZCASH_LEGACY_ACCOUNT) {
+                        throw JSONRPCError(
+                                RPC_INVALID_ADDRESS_OR_KEY,
+                                "Invalid from address: is a bare receiver from a Unified Address in this wallet. Provide the UA as returned by z_getaddressforaccount instead.");
+                    }
+                }
+            });
+
+            return ztxoSelectorOpt.value();
+        }
+    }();
 
     // Validate the destination address
     auto destStr = params[1].get_str();
     auto destaddress = keyIO.DecodePaymentAddress(destStr);
-    if (destaddress.has_value()) {
+    if (!destaddress.has_value()) {
-        examine(destaddress.value(), match {
-            [&](const CKeyID& addr) {
-                throw JSONRPCError(RPC_VERIFY_REJECTED, "Cannot shield coinbase output to a p2pkh address.");
-            },
-            [&](const CScriptID&) {
-                throw JSONRPCError(RPC_VERIFY_REJECTED, "Cannot shield coinbase output to a p2sh address.");
-            },
-            [&](const libzcash::SaplingPaymentAddress& addr) {
-                // OK
-            },
-            [&](const libzcash::SproutPaymentAddress& addr) {
-                if (canopyActive) {
-                    throw JSONRPCError(RPC_VERIFY_REJECTED, "Sprout shielding is not supported after Canopy activation");
-                }
-            },
-            [&](const libzcash::UnifiedAddress& ua) {
-                if (!ua.GetSaplingReceiver().has_value()) {
-                    throw JSONRPCError(
-                            RPC_VERIFY_REJECTED,
-                            "Only Sapling shielding is currently supported by z_shieldcoinbase. "
-                            "Use z_sendmany with a transaction amount that results in no change for Orchard shielding.");
-                }
-                involvesOrchard = ua.GetOrchardReceiver().has_value();
-            }
-        });
-    } else {
         throw JSONRPCError(RPC_INVALID_PARAMETER, 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) {
@@ -5291,98 +5299,9 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp)
         }
     }
 
-    int nLimit = SHIELD_COINBASE_DEFAULT_LIMIT;
+    int nUTXOLimit = params.size() > 3 ? params[3].get_int() : SHIELD_COINBASE_DEFAULT_LIMIT;
-    if (params.size() > 3) {
+    if (nUTXOLimit < 0) {
-        nLimit = params[3].get_int();
+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Limit on maximum number of utxos cannot be negative");
-        if (nLimit < 0) {
-            throw JSONRPCError(RPC_INVALID_PARAMETER, "Limit on maximum number of utxos cannot be negative");
-        }
-    }
-
-    const bool saplingActive =  Params().GetConsensus().NetworkUpgradeActive(nextBlockHeight, Consensus::UPGRADE_SAPLING);
-
-    // We cannot create shielded transactions before Sapling activates.
-    if (!saplingActive) {
-        throw JSONRPCError(
-            RPC_INVALID_PARAMETER, "Cannot create shielded transactions before Sapling has activated");
-    }
-
-    bool overwinterActive = Params().GetConsensus().NetworkUpgradeActive(nextBlockHeight, Consensus::UPGRADE_OVERWINTER);
-    assert(overwinterActive);
-    unsigned int max_tx_size = MAX_TX_SIZE_AFTER_SAPLING;
-
-    // Prepare to get coinbase utxos
-    std::vector<ShieldCoinbaseUTXO> inputs;
-    CAmount shieldedValue = 0;
-    CAmount remainingValue = 0;
-    size_t estimatedTxSize = 2000;  // 1802 joinsplit description + tx overhead + wiggle room
-    size_t utxoCounter = 0;
-    bool maxedOutFlag = false;
-    const size_t mempoolLimit = nLimit;
-
-    // Get available utxos
-    vector<COutput> vecOutputs;
-    pwalletMain->AvailableCoins(vecOutputs, std::nullopt, true, NULL, false, true);
-
-    // Find unspent coinbase utxos and update estimated size
-    for (const COutput& out : vecOutputs) {
-        if (!out.fSpendable) {
-            continue;
-        }
-
-        CTxDestination address;
-        if (!ExtractDestination(out.tx->vout[out.i].scriptPubKey, address)) {
-            continue;
-        }
-
-        // If from address was not the wildcard "*", filter utxos
-        if (sources.size() > 0 && !sources.count(address)) {
-            continue;
-        }
-
-        if (!out.tx->IsCoinBase()) {
-            continue;
-        }
-
-        utxoCounter++;
-        auto scriptPubKey = out.tx->vout[out.i].scriptPubKey;
-        CAmount nValue = out.tx->vout[out.i].nValue;
-
-        if (!maxedOutFlag) {
-            size_t increase = (std::get_if<CScriptID>(&address) != nullptr) ? CTXIN_SPEND_P2SH_SIZE : CTXIN_SPEND_DUST_SIZE;
-            if (estimatedTxSize + increase >= max_tx_size ||
-                (mempoolLimit > 0 && utxoCounter > mempoolLimit))
-            {
-                maxedOutFlag = true;
-            } else {
-                estimatedTxSize += increase;
-                ShieldCoinbaseUTXO utxo = {out.tx->GetHash(), out.i, scriptPubKey, nValue};
-                inputs.push_back(utxo);
-                shieldedValue += nValue;
-            }
-        }
-
-        if (maxedOutFlag) {
-            remainingValue += nValue;
-        }
-    }
-
-    size_t numUtxos = inputs.size();
-
-    if (numUtxos == 0) {
-        throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, "Could not find any coinbase funds to shield.");
-    }
-
-    if (shieldedValue < nFee) {
-        throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS,
-            strprintf("Insufficient coinbase funds, have %s, which is less than miners fee %s",
-            FormatMoney(shieldedValue), FormatMoney(nFee)));
-    }
-
-    // Check that the user specified fee is sane (if too high, it can result in error -25 absurd fee)
-    CAmount netAmount = shieldedValue - 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
@@ -5391,42 +5310,26 @@ UniValue z_shieldcoinbase(const UniValue& params, bool fHelp)
     contextInfo.pushKV("toaddress", params[1]);
     contextInfo.pushKV("fee", ValueFromAmount(nFee));
 
-    // Builder (used if Sapling addresses are involved)
+    // Create the wallet builder
-    std::optional<uint256> orchardAnchor;
+    WalletTxBuilder builder(chainparams, minRelayTxFee);
-    if (nAnchorConfirmations > 0 && (involvesOrchard || nPreferredTxVersion >= ZIP225_MIN_TX_VERSION)) {
-        // Allow Orchard recipients by setting an Orchard anchor.
-        auto orchardAnchorHeight = nextBlockHeight - nAnchorConfirmations;
-        if (Params().GetConsensus().NetworkUpgradeActive(orchardAnchorHeight, Consensus::UPGRADE_NU5)) {
-            auto anchorBlockIndex = chainActive[orchardAnchorHeight];
-            assert(anchorBlockIndex != nullptr);
-            orchardAnchor = anchorBlockIndex->hashFinalOrchardRoot;
-        }
-    }
-    TransactionBuilder builder = TransactionBuilder(
-        Params().GetConsensus(), nextBlockHeight, orchardAnchor, pwalletMain);
 
-    // Contextual transaction we will build on
+    auto async_shieldcoinbase =
-    // (used if no Sapling addresses are involved)
+        new AsyncRPCOperation_shieldcoinbase(
-    CMutableTransaction contextualTx = CreateNewContextualCMutableTransaction(
+                std::move(builder), ztxoSelector, destaddress.value(), strategy, nUTXOLimit, nFee, contextInfo);
-        Params().GetConsensus(), nextBlockHeight,
+    auto results = async_shieldcoinbase->prepare(*pwalletMain);
-        nPreferredTxVersion < ZIP225_MIN_TX_VERSION);
-    if (contextualTx.nVersion == 1) {
-        contextualTx.nVersion = 2; // Tx format should support vJoinSplit
-    }
 
     // Create operation and add to global queue
     std::shared_ptr<AsyncRPCQueue> q = getAsyncRPCQueue();
-    std::shared_ptr<AsyncRPCOperation> operation( new AsyncRPCOperation_shieldcoinbase(
+    std::shared_ptr<AsyncRPCOperation> operation(async_shieldcoinbase);
-        std::move(builder), contextualTx, inputs, destaddress.value(), nFee, contextInfo) );
     q->addOperation(operation);
     AsyncRPCOperationId operationId = operation->getId();
 
     // Return continuation information
     UniValue o(UniValue::VOBJ);
-    o.pushKV("remainingUTXOs", static_cast<uint64_t>(utxoCounter - numUtxos));
+    o.pushKV("remainingUTXOs", static_cast<uint64_t>(results.utxoCounter - results.numUtxos));
-    o.pushKV("remainingValue", ValueFromAmount(remainingValue));
+    o.pushKV("remainingValue", ValueFromAmount(results.remainingValue));
-    o.pushKV("shieldingUTXOs", static_cast<uint64_t>(numUtxos));
+    o.pushKV("shieldingUTXOs", static_cast<uint64_t>(results.numUtxos));
-    o.pushKV("shieldingValue", ValueFromAmount(shieldedValue));
+    o.pushKV("shieldingValue", ValueFromAmount(results.shieldingValue));
     o.pushKV("opid", operationId);
     return o;
 }
@@ -5707,7 +5610,7 @@ UniValue z_mergetoaddress(const UniValue& params, bool fHelp)
             CAmount nValue = out.tx->vout[out.i].nValue;
 
             if (!maxedOutUTXOsFlag) {
-                size_t increase = (std::get_if<CScriptID>(&address) != nullptr) ? CTXIN_SPEND_P2SH_SIZE : CTXIN_SPEND_DUST_SIZE;
+                size_t increase = (std::get_if<CScriptID>(&address) != nullptr) ? CTXIN_SPEND_P2SH_SIZE : CTXIN_SPEND_P2PKH_SIZE;
                 if (estimatedTxSize + increase >= max_tx_size ||
                     (mempoolLimit > 0 && utxoCounter > mempoolLimit))
                 {

src/wallet/test/rpc_wallet_tests.cpp

@@ -1521,26 +1521,24 @@ BOOST_AUTO_TEST_CASE(rpc_z_shieldcoinbase_parameters)
     "100 -1"
     ), runtime_error);
 
-    // Mutable tx containing contextual information we need to build tx
-    UniValue retValue = CallRPC("getblockcount");
-    int nHeight = retValue.get_int();
-    CMutableTransaction mtx = CreateNewContextualCMutableTransaction(Params().GetConsensus(), nHeight + 1, false);
-    if (mtx.nVersion == 1) {
-        mtx.nVersion = 2;
-    }
-
     // Test constructor of AsyncRPCOperation_shieldcoinbase
     KeyIO keyIO(Params());
+    UniValue retValue;
+    BOOST_CHECK_NO_THROW(retValue = CallRPC("getnewaddress"));
+    auto taddr2 = std::get<CKeyID>(keyIO.DecodePaymentAddress(retValue.get_str()).value());
     auto testnetzaddr = std::get<libzcash::SproutPaymentAddress>(keyIO.DecodePaymentAddress("ztjiDe569DPNbyTE6TSdJTaSDhoXEHLGvYoUnBU1wfVNU52TEyT6berYtySkd21njAeEoh8fFJUT42kua9r8EnhBaEKqCpP").value());
+    auto selector = pwalletMain->ZTXOSelectorForAddress(
+            taddr2,
+            true,
+            // In the real transaction builder we use either Require or Disallow, but here we
+            // are checking that there are no UTXOs at all, so we allow either to be selected to
+            // confirm this.
+            TransparentCoinbasePolicy::Allow,
+            false).value();
+    WalletTxBuilder builder(Params(), minRelayTxFee);
 
     try {
-        std::shared_ptr<AsyncRPCOperation> operation(new AsyncRPCOperation_shieldcoinbase(TransactionBuilder(), mtx, {}, testnetzaddr, -1 ));
+        std::shared_ptr<AsyncRPCOperation> operation(new AsyncRPCOperation_shieldcoinbase(std::move(builder), selector, testnetzaddr, PrivacyPolicy::AllowRevealedSenders, SHIELD_COINBASE_DEFAULT_LIMIT, 1));
-    } catch (const UniValue& objError) {
-        BOOST_CHECK( find_error(objError, "Fee is out of range"));
-    }
-
-    try {
-        std::shared_ptr<AsyncRPCOperation> operation(new AsyncRPCOperation_shieldcoinbase(TransactionBuilder(), mtx, {}, testnetzaddr, 1));
     } catch (const UniValue& objError) {
         BOOST_CHECK( find_error(objError, "Empty inputs"));
     }

src/wallet/wallet.cpp

@@ -7757,6 +7757,13 @@ bool ZTXOSelector::SelectsOrchard() const {
     });
 }
 
+void SpendableInputs::LimitTransparentUtxos(size_t maxUtxoCount)
+{
+    while (utxos.size() > maxUtxoCount) {
+        utxos.pop_back();
+    }
+}
+
 bool SpendableInputs::LimitToAmount(
     const CAmount amountRequired,
     const CAmount dustThreshold,

src/wallet/wallet.h

@@ -958,6 +958,11 @@ public:
     std::vector<SaplingNoteEntry> saplingNoteEntries;
     std::vector<OrchardNoteMetadata> orchardNoteMetadata;
 
+    /**
+     * Retain the first `maxUtxoCount` utxos, and discard the rest.
+     */
+    void LimitTransparentUtxos(size_t maxUtxoCount);
+
     /**
      * Selectively discard notes that are not required to obtain the desired
      * amount. Returns `false` if the available inputs do not add up to the

src/wallet/wallet_tx_builder.cpp

@@ -157,7 +157,7 @@ WalletTxBuilder::PrepareTransaction(
     return selected.map([&](const InputSelection& resolvedSelection) {
         auto ovks = SelectOVKs(wallet, selector, spendable);
 
-        auto effects = TransactionEffects(
+        return TransactionEffects(
                 anchorConfirmations,
                 resolvedSelection.GetInputs(),
                 resolvedSelection.GetPayments(),
@@ -166,8 +166,6 @@ WalletTxBuilder::PrepareTransaction(
                 ovks.first,
                 ovks.second,
                 anchorHeight);
-        effects.LockSpendable(wallet);
-        return effects;
     });
 }