Repository: zcash/zcash (language: rust)

Issue:
[wallet] CalcZIP317Fee used by wallet_tx_builder underestimates the ZIP 317 fee
### Describe the issue
`CalcZIP317Fee` calculates the size of transparent inputs using the UTXOs' scriptPubKeys, rather than the scriptSigs needed to spend each coin: https://github.com/zcash/zcash/blob/10894e09d3ed488e056418be8d181d70d87a760f/src/wallet/wallet_tx_builder.cpp#L70-L74
This causes `wallet_tx_builder.cpp` to calculate an insufficient ZIP 317 fee in RPC methods that use it.

### Can you reliably reproduce the issue?
Yes, this is reproduced by the `wallet_shieldcoinbase*` RPC tests on the branch for #6646. However, I think that any use of `z_sendmany`, `z_shieldcoinbase`, or `z_mergetoaddress` with a sufficiently large number of transparent inputs, and with no explicit fee specified, should show the problem.

### Expected behaviour
These RPC methods should create transactions with the correct ZIP 317 conventional fee when spending transparent inputs.

### The version of Zcash you were using:
The #6646 branch, but the problem was introduced in #6524, and also occurs on master and in v5.5.0.

### Any extra information that might be useful in the debugging process.
https://github.com/zcash/zcash/pull/6646/commits/4e82a9904a1f4bca8d2892db90726992bcea5d46 includes useful debug logging for this issue.


Discussion / hints:

Fix patch:
diff --git a/src/wallet/wallet_tx_builder.cpp b/src/wallet/wallet_tx_builder.cpp
index fa9877051fe..c4752bd7950 100644
--- a/src/wallet/wallet_tx_builder.cpp
+++ b/src/wallet/wallet_tx_builder.cpp
@@ -2,8 +2,10 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or https://www.opensource.org/licenses/mit-license.php .
 
-#include "util/moneystr.h"
 #include "wallet/wallet_tx_builder.h"
+
+#include "script/sign.h"
+#include "util/moneystr.h"
 #include "zip317.h"
 
 using namespace libzcash;
@@ -21,9 +23,11 @@ static size_t PadCount(size_t n)
 
 static CAmount
 CalcZIP317Fee(
+        const CWallet& wallet,
         const std::optional<SpendableInputs>& inputs,
         const std::vector<ResolvedPayment>& payments,
-        const std::optional<ChangeAddress>& changeAddr)
+        const std::optional<ChangeAddress>& changeAddr,
+        uint32_t consensusBranchId)
 {
     std::vector<CTxOut> vout{};
     size_t sproutOutputCount{}, saplingOutputCount{}, orchardOutputCount{};
@@ -68,9 +72,13 @@ CalcZIP317Fee(
     size_t orchardInputCount = 0;
     if (inputs.has_value()) {
         for (const auto& utxo : inputs.value().utxos) {
-            vin.emplace_back(
-                    COutPoint(utxo.tx->GetHash(), utxo.i),
-                    utxo.tx->vout[utxo.i].scriptPubKey);
+            SignatureData sigdata;
+            ProduceSignature(
+                    DummySignatureCreator(&wallet),
+                    utxo.tx->vout[utxo.i].scriptPubKey,
+                    sigdata,
+                    consensusBranchId);
+            vin.emplace_back(COutPoint(utxo.tx->GetHash(), utxo.i), sigdata.scriptSig);
         }
         sproutInputCount = inputs.value().sproutNoteEntries.size();
         saplingInputCount = inputs.value().saplingNoteEntries.size();
@@ -211,12 +219,14 @@ ValidateAmount(const SpendableInputs& spendable, const CAmount& fee)
 
 static tl::expected<std::pair<ResolvedPayment, CAmount>, InputSelectionError>
 ResolveNetPayment(
+        const CWallet& wallet,
         const ZTXOSelector& selector,
         const SpendableInputs& spendable,
         const NetAmountRecipient& netpay,
         const std::optional<CAmount>& fee,
         const TransactionStrategy& strategy,
-        bool afterNU5)
+        bool afterNU5,
+        uint32_t consensusBranchId)
 {
     bool canResolveOrchard = afterNU5 && !selector.SelectsSprout();
     CAmount maxSaplingAvailable = spendable.GetSaplingTotal();
@@ -241,7 +251,7 @@ ResolveNetPayment(
                     orchardOutputs)
                 .map_error([](const auto& error) -> InputSelectionError { return error; })
                 .and_then([&](const auto& rpayment) {
-                    auto finalFee = fee.value_or(CalcZIP317Fee(spendable, {rpayment}, std::nullopt));
+                    auto finalFee = fee.value_or(CalcZIP317Fee(wallet, spendable, {rpayment}, std::nullopt, consensusBranchId));
                     return ValidateAmount(spendable, finalFee)
                         .and_then([&](void) {
                             return ResolvePayment(
@@ -414,8 +424,10 @@ WalletTxBuilder::PrepareTransaction(
         }
     }
 
+    auto consensus = params.GetConsensus();
     int anchorHeight = GetAnchorHeight(chain, anchorConfirmations);
-    bool afterNU5 = params.GetConsensus().NetworkUpgradeActive(anchorHeight, Consensus::UPGRADE_NU5);
+    bool afterNU5 = consensus.NetworkUpgradeActive(anchorHeight, Consensus::UPGRADE_NU5);
+    auto consensusBranchId = CurrentEpochBranchId(chain.Height(), consensus);
     auto selected = examine(payments, match {
             [&](const std::vector<Payment>& payments) {
                 return ResolveInputsAndPayments(
@@ -423,13 +435,13 @@ WalletTxBuilder::PrepareTransaction(
                         selector,
                         spendable,
                         payments,
-                        chain,
                         strategy,
                         fee,
-                        afterNU5);
+                        afterNU5,
+                        consensusBranchId);
             },
             [&](const NetAmountRecipient& netRecipient) {
-                return ResolveNetPayment(selector, spendable, netRecipient, fee, strategy, afterNU5)
+                return ResolveNetPayment(wallet, selector, spendable, netRecipient, fee, strategy, afterNU5, consensusBranchId)
                     .map([&](const auto& pair) {
                         const auto& [payment, finalFee] = pair;
                         return InputSelection(spendable, {{payment}}, finalFee, std::nullopt);
@@ -484,16 +496,20 @@ SpendableInputs WalletTxBuilder::FindAllSpendableInputs(
 }
 
 CAmount GetConstrainedFee(
+        const CWallet& wallet,
         const std::optional<SpendableInputs>& inputs,
         const std::vector<ResolvedPayment>& payments,
-        const std::optional<ChangeAddress>& changeAddr)
+        const std::optional<ChangeAddress>& changeAddr,
+        uint32_t consensusBranchId)
 {
     // We know that minRelayFee <= MINIMUM_FEE <= conventional_fee, so we can use an arbitrary
     // transaction size when constraining the fee, because we are guaranteed to already satisfy the
     // lower bound.
     constexpr unsigned int DUMMY_TX_SIZE = 1;
 
-    return CWallet::ConstrainFee(CalcZIP317Fee(inputs, payments, changeAddr), DUMMY_TX_SIZE);
+    return CWallet::ConstrainFee(
+            CalcZIP317Fee(wallet, inputs, payments, changeAddr, consensusBranchId),
+            DUMMY_TX_SIZE);
 }
 
 static tl::expected<void, InputSelectionError>
@@ -544,12 +560,13 @@ WalletTxBuilder::IterateLimit(
         CAmount dustThreshold,
         const SpendableInputs& spendable,
         Payments& resolved,
-        bool afterNU5) const
+        bool afterNU5,
+        uint32_t consensusBranchId) const
 {
     SpendableInputs spendableMut;
 
     auto previousFee = MINIMUM_FEE;
-    auto updatedFee = GetConstrainedFee(std::nullopt, resolved.GetResolvedPayments(), std::nullopt);
+    auto updatedFee = GetConstrainedFee(wallet, std::nullopt, resolved.GetResolvedPayments(), std::nullopt, consensusBranchId);
     // This is used to increase the target amount just enough (generally by 0 or 1) to force
     // selection of additional notes.
     CAmount bumpTargetAmount{0};
@@ -593,9 +610,11 @@ WalletTxBuilder::IterateLimit(
             }
             previousFee = updatedFee;
             updatedFee = GetConstrainedFee(
+                    wallet,
                     spendableMut,
                     resolved.GetResolvedPayments(),
-                    changeAmount > 0 ? changeAddr : std::nullopt);
+                    changeAmount > 0 ? changeAddr : std::nullopt,
+                    consensusBranchId);
         } else {
             return tl::make_unexpected(
                     ReportInvalidFunds(
@@ -635,10 +654,10 @@ WalletTxBuilder::ResolveInputsAndPayments(
         const ZTXOSelector& selector,
         const SpendableInputs& spendable,
         const std::vector<Payment>& payments,
-        const CChain& chain,
         const TransactionStrategy& strategy,
         const std::optional<CAmount>& fee,
-        bool afterNU5) const
+        bool afterNU5,
+        uint32_t consensusBranchId) const
 {
     LOCK2(cs_main, wallet.cs_wallet);
 
@@ -729,7 +748,7 @@ WalletTxBuilder::ResolveInputsAndPayments(
             //       adding the change payment. We can remove this check and make the later one
             //       unconditional once that’s fixed.
             auto conventionalFee =
-                CalcZIP317Fee(spendableMut, resolved.GetResolvedPayments(), changeAddr);
+                CalcZIP317Fee(wallet, spendableMut, resolved.GetResolvedPayments(), changeAddr, consensusBranchId);
             if (finalFee > WEIGHT_RATIO_CAP * conventionalFee) {
                 return tl::make_unexpected(AbsurdFeeError(conventionalFee, finalFee));
             }
@@ -740,13 +759,13 @@ WalletTxBuilder::ResolveInputsAndPayments(
             }
         } else {
             auto conventionalFee =
-                CalcZIP317Fee(spendableMut, resolved.GetResolvedPayments(), std::nullopt);
+                CalcZIP317Fee(wallet, spendableMut, resolved.GetResolvedPayments(), std::nullopt, consensusBranchId);
             if (finalFee > WEIGHT_RATIO_CAP * conventionalFee) {
                 return tl::make_unexpected(AbsurdFeeError(resolved.Total(), finalFee));
             }
         }
     } else {
-        auto limitResult = IterateLimit(wallet, selector, strategy, sendAmount, dustThreshold, spendable, resolved, afterNU5);
+        auto limitResult = IterateLimit(wallet, selector, strategy, sendAmount, dustThreshold, spendable, resolved, afterNU5, consensusBranchId);
         if (limitResult.has_value()) {
             std::tie(spendableMut, finalFee, changeAddr) = limitResult.value();
             targetAmount = sendAmount + finalFee;
diff --git a/src/wallet/wallet_tx_builder.h b/src/wallet/wallet_tx_builder.h
index 371b0fcc221..7033ba501ab 100644
--- a/src/wallet/wallet_tx_builder.h
+++ b/src/wallet/wallet_tx_builder.h
@@ -402,7 +402,8 @@ class WalletTxBuilder {
             CAmount dustThreshold,
             const SpendableInputs& spendable,
             Payments& resolved,
-            bool afterNU5) const;
+            bool afterNU5,
+            uint32_t consensusBranchId) const;
 
     /**
      * Select inputs sufficient to fulfill the specified requested payments,
@@ -415,10 +416,10 @@ class WalletTxBuilder {
             const ZTXOSelector& selector,
             const SpendableInputs& spendable,
             const std::vector<Payment>& payments,
-            const CChain& chain,
             const TransactionStrategy& strategy,
             const std::optional<CAmount>& fee,
-            bool afterNU5) const;
+            bool afterNU5,
+            uint32_t consensusBranchId) const;
     /**
      * Compute the internal and external OVKs to use in transaction construction, given
      * the spendable inputs.


Test patch:
diff --git a/qa/rpc-tests/mergetoaddress_helper.py b/qa/rpc-tests/mergetoaddress_helper.py
index 7d1bb61e7ae..3999697dca6 100755
--- a/qa/rpc-tests/mergetoaddress_helper.py
+++ b/qa/rpc-tests/mergetoaddress_helper.py
@@ -173,7 +173,7 @@ def generate_and_check(node, expected_transactions):
         # 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') - conventional_fee(4))
+        assert_equal(test.nodes[0].z_getbalance(myzaddr), Decimal('40') - conventional_fee(5))
         assert_equal(test.nodes[1].getbalance(), Decimal('40'))
         assert_equal(test.nodes[2].getbalance(), Decimal('30'))
 
@@ -190,7 +190,7 @@ def generate_and_check(node, expected_transactions):
         test.sync_all()
 
         assert_equal(test.nodes[0].z_getbalance(myzaddr), Decimal('0'))
-        assert_equal(test.nodes[0].z_getbalance(myzaddr2), Decimal('40') - conventional_fee(4))
+        assert_equal(test.nodes[0].z_getbalance(myzaddr2), Decimal('40') - conventional_fee(5))
 
         # Shield coinbase UTXOs from any node 2 taddr, and set fee to 0
         result = test.nodes[2].z_shieldcoinbase("*", myzaddr, 0)
@@ -201,7 +201,7 @@ def generate_and_check(node, expected_transactions):
 
         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') - conventional_fee(4))
+        assert_equal(test.nodes[0].z_getbalance(myzaddr2), Decimal('40') - conventional_fee(5))
         assert_equal(test.nodes[1].getbalance(), Decimal('60'))
         assert_equal(test.nodes[2].getbalance(), Decimal('0'))
 
@@ -212,9 +212,9 @@ def generate_and_check(node, expected_transactions):
         generate_and_check(test.nodes[1], 2)
         test.sync_all()
 
-        assert_equal(test.nodes[0].getbalance(), Decimal('80') - conventional_fee(4))
+        assert_equal(test.nodes[0].getbalance(), Decimal('80') - conventional_fee(5))
         assert_equal(test.nodes[0].z_getbalance(do_not_shield_taddr), Decimal('10'))
-        assert_equal(test.nodes[0].z_getbalance(mytaddr), Decimal('70') - conventional_fee(4))
+        assert_equal(test.nodes[0].z_getbalance(mytaddr), Decimal('70') - conventional_fee(5))
         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'))
@@ -234,8 +234,8 @@ def generate_and_check(node, expected_transactions):
         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') - conventional_fee(4))
-        assert_equal(test.nodes[1].z_getbalance(n1taddr), Decimal('80') - conventional_fee(4))
+        assert_equal(test.nodes[1].getbalance(), Decimal('160') - conventional_fee(5))
+        assert_equal(test.nodes[1].z_getbalance(n1taddr), Decimal('80') - conventional_fee(5))
         assert_equal(test.nodes[2].getbalance(), Decimal('0'))
 
         # Generate 5 regular UTXOs on node 0, and 20 regular UTXOs on node 2.
diff --git a/qa/rpc-tests/wallet_shieldcoinbase.py b/qa/rpc-tests/wallet_shieldcoinbase.py
index 478d4c62281..dd7f5ed630b 100755
--- a/qa/rpc-tests/wallet_shieldcoinbase.py
+++ b/qa/rpc-tests/wallet_shieldcoinbase.py
@@ -111,7 +111,7 @@ def run_test (self):
         # Confirm balances and that do_not_shield_taddr containing funds of 10 was left alone
         assert_equal(self.nodes[0].getbalance(), 10)
         assert_equal(self.nodes[0].z_getbalance(do_not_shield_taddr), Decimal('10.0'))
-        self.test_check_balance_zaddr(self.nodes[0], Decimal('40.0') - conventional_fee(4))
+        self.test_check_balance_zaddr(self.nodes[0], Decimal('40.0') - conventional_fee(6))
         assert_equal(self.nodes[1].getbalance(), 20)
         assert_equal(self.nodes[2].getbalance(), 30)
 
@@ -123,7 +123,7 @@ def run_test (self):
         self.sync_all()
 
         assert_equal(self.nodes[0].getbalance(), 10)
-        self.test_check_balance_zaddr(self.nodes[0], Decimal('70.0') - conventional_fee(4))
+        self.test_check_balance_zaddr(self.nodes[0], Decimal('70.0') - conventional_fee(6))
         assert_equal(self.nodes[1].getbalance(), 30)
         assert_equal(self.nodes[2].getbalance(), 0)
 
diff --git a/src/wallet/gtest/test_rpc_wallet.cpp b/src/wallet/gtest/test_rpc_wallet.cpp
index fbcd9647a9a..a95d3999f8a 100644
--- a/src/wallet/gtest/test_rpc_wallet.cpp
+++ b/src/wallet/gtest/test_rpc_wallet.cpp
@@ -13,6 +13,7 @@
 #include "wallet/asyncrpcoperation_sendmany.h"
 #include "wallet/memo.h"
 #include "zcash/JoinSplit.hpp"
+#include "zip317.h"
 
 #include <librustzcash.h>
 #include <rust/ed25519.h>
@@ -29,6 +30,25 @@ CWalletTx FakeWalletTx() {
     mtx.vout[0].nValue = 1;
     return CWalletTx(nullptr, mtx);
 }
+
+/// Expects that the fee calculated during transaction construction matches the fee used by block
+/// construction. It allows the fee included in the transaction to be `MARGINAL_FEE` higher than the
+/// fee expected by block construction.
+void ExpectConsistentFee(const TransactionStrategy& strategy, const TransactionEffects& effects)
+{
+    auto buildResult = effects.ApproveAndBuild(
+            Params().GetConsensus(),
+            *pwalletMain,
+            chainActive,
+            strategy);
+    auto tx = buildResult.GetTxOrThrow();
+
+    auto expectedFee = tx.GetConventionalFee();
+    // Allow one incremental fee tick of buffer above the conventional fee.
+    EXPECT_TRUE(effects.GetFee() == expectedFee || effects.GetFee() == expectedFee + MARGINAL_FEE)
+        << "effects.GetFee() = " << effects.GetFee() << std::endl
+        << "tx.GetConventionalFee() = " << expectedFee;
+}
 }
 
 TEST(WalletRPCTests, PrepareTransaction)
@@ -46,8 +66,6 @@ TEST(WalletRPCTests, PrepareTransaction)
             pwalletMain->GenerateNewSeed();
         }
 
-        UniValue retValue;
-
         KeyIO keyIO(Params());
         // add keys manually
         auto taddr = pwalletMain->GenerateNewKey(true).GetID();
@@ -146,7 +164,15 @@ TEST(WalletRPCTests, RPCZMergeToAddressInternals)
     {
     LOCK2(cs_main, pwalletMain->cs_wallet);
 
-    UniValue retValue;
+    EXPECT_EQ(-1, chainActive.Height());
+    CBlock block;
+    block.hashMerkleRoot = BlockMerkleRoot(block);
+    auto blockHash = block.GetHash();
+    CBlockIndex fakeIndex {block};
+    mapBlockIndex.insert(std::make_pair(blockHash, &fakeIndex));
+    chainActive.SetTip(&fakeIndex);
+    EXPECT_TRUE(chainActive.Contains(&fakeIndex));
+    EXPECT_EQ(0, chainActive.Height());
 
     // Mutable tx containing contextual information we need to build tx
     // We removed the ability to create pre-Sapling Sprout proofs, so we can
@@ -173,7 +199,7 @@ TEST(WalletRPCTests, RPCZMergeToAddressInternals)
 
     SpendableInputs inputs;
     auto wtx = FakeWalletTx();
-    inputs.utxos.emplace_back(COutput(&wtx, 0, 100, true));
+    inputs.utxos.emplace_back(&wtx, 0, 100, true);
 
     // Can’t send to Sprout
     builder.PrepareTransaction(
@@ -214,6 +240,11 @@ TEST(WalletRPCTests, RPCZMergeToAddressInternals)
             }));
         })
         .map([](const auto&) { EXPECT_TRUE(false); });
+
+    // Tear down
+    chainActive.SetTip(NULL);
+    mapBlockIndex.erase(blockHash);
+
     }
     UnloadGlobalWallet();
 }
@@ -233,8 +264,6 @@ TEST(WalletRPCTests, RPCZsendmanyTaddrToSapling)
         pwalletMain->GenerateNewSeed();
     }
 
-    UniValue retValue;
-
     KeyIO keyIO(Params());
     // add keys manually
     auto taddr = pwalletMain->GenerateNewKey(true).GetID();
@@ -341,3 +370,137 @@ TEST(WalletRPCTests, RPCZsendmanyTaddrToSapling)
     RegtestDeactivateSapling();
     UnloadGlobalWallet();
 }
+
+TEST(WalletRPCTests, ZIP317Fee)
+{
+    LoadProofParameters();
+    SelectParams(CBaseChainParams::TESTNET);
+
+    LoadGlobalWallet();
+
+    RegtestActivateSapling();
+    {
+        LOCK2(cs_main, pwalletMain->cs_wallet);
+
+        if (!pwalletMain->HaveMnemonicSeed()) {
+            pwalletMain->GenerateNewSeed();
+        }
+
+        KeyIO keyIO(Params());
+        // add keys manually
+        auto taddr = pwalletMain->GenerateNewKey(true).GetID();
+        auto pa = pwalletMain->GenerateNewLegacySaplingZKey();
+
+        const Consensus::Params& consensusParams = Params().GetConsensus();
+
+        int nextBlockHeight = chainActive.Height() + 1;
+
+        // Add a fake transaction to the wallet
+        CMutableTransaction mtx = CreateNewContextualCMutableTransaction(consensusParams, nextBlockHeight, false);
+        CScript scriptPubKey = CScript() << OP_DUP << OP_HASH160 << ToByteVector(taddr) << OP_EQUALVERIFY << OP_CHECKSIG;
+        size_t utxoCount = 100;
+        for (size_t i = 0; i < utxoCount; i++) {
+            mtx.vout.push_back(CTxOut(5 * COIN, scriptPubKey));
+        }
+        CWalletTx wtx(pwalletMain, mtx);
+        pwalletMain->LoadWalletTx(wtx);
+
+        // Fake-mine the transaction
+        EXPECT_EQ(-1, chainActive.Height());
+        CBlock block;
+        block.vtx.push_back(wtx);
+        block.hashMerkleRoot = BlockMerkleRoot(block);
+        auto blockHash = block.GetHash();
+        CBlockIndex fakeIndex {block};
+        mapBlockIndex.insert(std::make_pair(blockHash, &fakeIndex));
+        chainActive.SetTip(&fakeIndex);
+        EXPECT_TRUE(chainActive.Contains(&fakeIndex));
+        EXPECT_EQ(0, chainActive.Height());
+        wtx.SetMerkleBranch(block);
+        pwalletMain->LoadWalletTx(wtx);
+
+        // Add keys manually
+        std::string taddr_string = keyIO.EncodeDestination(taddr);
+
+        WalletTxBuilder builder(Params(), minRelayTxFee);
+
+        auto selector = CWallet::LegacyTransparentZTXOSelector(
+                true,
+                TransparentCoinbasePolicy::Disallow);
+
+        { // test transparent inputs to NetAmountRecipient
+            auto saplingKey = pwalletMain->GenerateNewLegacySaplingZKey();
+            NetAmountRecipient zaddr(saplingKey, std::nullopt);
+
+            TransactionStrategy strategy(PrivacyPolicy::AllowRevealedSenders);
+
+            SpendableInputs inputs;
+            for (size_t i = 0; i < utxoCount; i++) {
+                inputs.utxos.emplace_back(&wtx, i, 100, true);
+            }
+
+            auto effects = builder.PrepareTransaction(
+                    *pwalletMain,
+                    selector,
+                    inputs,
+                    zaddr,
+                    chainActive,
+                    strategy,
+                    std::nullopt,
+                    1)
+            .map_error([&](const auto& err) {
+                try {
+                    ThrowInputSelectionError(err, selector, strategy);
+                } catch (const UniValue& value) {
+                    FAIL() << value.write();
+                }
+            })
+            .value();
+
+            ExpectConsistentFee(strategy, effects);
+        }
+
+        { // test transparent inputs to Payment vector
+            auto saplingKey = pwalletMain->GenerateNewLegacySaplingZKey();
+            Payment saplingPayment(saplingKey, 200 * COIN, std::nullopt);
+            auto saplingKey2 = pwalletMain->GenerateNewLegacySaplingZKey();
+            Payment saplingPayment2(saplingKey2, 200 * COIN, std::nullopt);
+            std::vector<Payment> payments {saplingPayment, saplingPayment2};
+
+            TransactionStrategy strategy(PrivacyPolicy::AllowFullyTransparent);
+
+            SpendableInputs inputs;
+            for (size_t i = 0; i < utxoCount; i++) {
+                inputs.utxos.emplace_back(&wtx, i, 100, true);
+            }
+
+            auto effects = builder.PrepareTransaction(
+                    *pwalletMain,
+                    selector,
+                    inputs,
+                    payments,
+                    chainActive,
+                    strategy,
+                    std::nullopt,
+                    1)
+                .map_error([&](const auto& err) {
+                    try {
+                        ThrowInputSelectionError(err, selector, strategy);
+                    } catch (const UniValue& value) {
+                        FAIL() << value.write();
+                    }
+                })
+                .value();
+
+            ExpectConsistentFee(strategy, effects);
+        }
+
+        // Tear down
+        chainActive.SetTip(NULL);
+        mapBlockIndex.erase(blockHash);
+
+    }
+    // Revert to default
+    RegtestDeactivateSapling();
+    UnloadGlobalWallet();
+}