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/README.md b/README.md
index 7e5743b779e..bc777728dc0 100644
--- a/README.md
+++ b/README.md
@@ -1,4 +1,4 @@
-Zcash 5.5.0
+Zcash 5.5.1
 <img align="right" width="120" height="80" src="doc/imgs/logo.png">
 ===========
 
diff --git a/configure.ac b/configure.ac
index 0f0faf6fe09..30096a3e228 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2,7 +2,7 @@ dnl require autoconf 2.60 (AS_ECHO/AS_ECHO_N)
 AC_PREREQ([2.60])
 define(_CLIENT_VERSION_MAJOR, 5)
 define(_CLIENT_VERSION_MINOR, 5)
-define(_CLIENT_VERSION_REVISION, 0)
+define(_CLIENT_VERSION_REVISION, 1)
 define(_CLIENT_VERSION_BUILD, 50)
 define(_ZC_BUILD_VAL, m4_if(m4_eval(_CLIENT_VERSION_BUILD < 25), 1, m4_incr(_CLIENT_VERSION_BUILD), m4_eval(_CLIENT_VERSION_BUILD < 50), 1, m4_eval(_CLIENT_VERSION_BUILD - 24), m4_eval(_CLIENT_VERSION_BUILD == 50), 1, , m4_eval(_CLIENT_VERSION_BUILD - 50)))
 define(_CLIENT_VERSION_SUFFIX, m4_if(m4_eval(_CLIENT_VERSION_BUILD < 25), 1, _CLIENT_VERSION_REVISION-beta$1, m4_eval(_CLIENT_VERSION_BUILD < 50), 1, _CLIENT_VERSION_REVISION-rc$1, m4_eval(_CLIENT_VERSION_BUILD == 50), 1, _CLIENT_VERSION_REVISION, _CLIENT_VERSION_REVISION-$1)))
diff --git a/contrib/debian/changelog b/contrib/debian/changelog
index 5fdfb1dff1c..072f9481fd2 100644
--- a/contrib/debian/changelog
+++ b/contrib/debian/changelog
@@ -1,3 +1,9 @@
+zcash (5.5.1) stable; urgency=medium
+
+  * 5.5.1 release.
+
+ -- Electric Coin Company <team@electriccoin.co>  Tue, 16 May 2023 11:20:05 -0600
+
 zcash (5.5.0) stable; urgency=medium
 
   * 5.5.0 release.
diff --git a/contrib/gitian-descriptors/gitian-linux-parallel.yml b/contrib/gitian-descriptors/gitian-linux-parallel.yml
index 9aa85723f39..18535c9adbd 100644
--- a/contrib/gitian-descriptors/gitian-linux-parallel.yml
+++ b/contrib/gitian-descriptors/gitian-linux-parallel.yml
@@ -1,5 +1,5 @@
 ---
-name: "zcash-5.5.0"
+name: "zcash-5.5.1"
 enable_cache: true
 distro: "debian"
 suites:
diff --git a/contrib/gitian-descriptors/gitian-linux.yml b/contrib/gitian-descriptors/gitian-linux.yml
index 368fcdf6152..86229bda2b5 100644
--- a/contrib/gitian-descriptors/gitian-linux.yml
+++ b/contrib/gitian-descriptors/gitian-linux.yml
@@ -1,5 +1,5 @@
 ---
-name: "zcash-5.5.0"
+name: "zcash-5.5.1"
 enable_cache: true
 distro: "debian"
 suites:
diff --git a/doc/authors.md b/doc/authors.md
index 78d5e6ecf6e..98d33ae6173 100644
--- a/doc/authors.md
+++ b/doc/authors.md
@@ -7,7 +7,7 @@ Simon Liu (460)
 Daira Hopwood (392)
 Sean Bowe (390)
 Eirik Ogilvie-Wigley (216)
-Greg Pfeil (174)
+Greg Pfeil (184)
 Wladimir J. van der Laan (160)
 Pieter Wuille (149)
 Alfredo Garcia (120)
diff --git a/doc/book/src/user/release-support.md b/doc/book/src/user/release-support.md
index 02b961bcc14..8d8ecf89c42 100644
--- a/doc/book/src/user/release-support.md
+++ b/doc/book/src/user/release-support.md
@@ -50,4 +50,5 @@ time, and may shift due to changes in network solution power.
 | 5.5.0-rc2 | 2023-04-25 | 2193300 | 2023-08-15 |
 | 5.5.0-rc3 | 2023-04-27 | 2195224 | 2023-08-17 |
 | 5.5.0 | 2023-04-27 | 2196024 | 2023-08-17 |
+| 5.5.1 | 2023-05-16 | 2217487 | 2023-09-05 |
 <!-- RELEASE_SCRIPT_END_MARKER -->
diff --git a/doc/man/zcash-cli.1 b/doc/man/zcash-cli.1
index 75815c8f2f8..6855e0f852f 100644
--- a/doc/man/zcash-cli.1
+++ b/doc/man/zcash-cli.1
@@ -1,9 +1,9 @@
 .\" DO NOT MODIFY THIS FILE!  It was generated by help2man 1.49.1.
-.TH ZCASH-CLI "1" "April 2023" "zcash-cli v5.5.0" "User Commands"
+.TH ZCASH-CLI "1" "May 2023" "zcash-cli v5.5.1" "User Commands"
 .SH NAME
-zcash-cli \- manual page for zcash-cli v5.5.0
+zcash-cli \- manual page for zcash-cli v5.5.1
 .SH DESCRIPTION
-Zcash RPC client version v5.5.0
+Zcash RPC client version v5.5.1
 .PP
 In order to ensure you are adequately protecting your privacy when using Zcash,
 please see <https://z.cash/support/security/>.
diff --git a/doc/man/zcash-tx.1 b/doc/man/zcash-tx.1
index 2d238e6cc9b..35b3cc39cf6 100644
--- a/doc/man/zcash-tx.1
+++ b/doc/man/zcash-tx.1
@@ -1,9 +1,9 @@
 .\" DO NOT MODIFY THIS FILE!  It was generated by help2man 1.49.1.
-.TH ZCASH-TX "1" "April 2023" "zcash-tx v5.5.0" "User Commands"
+.TH ZCASH-TX "1" "May 2023" "zcash-tx v5.5.1" "User Commands"
 .SH NAME
-zcash-tx \- manual page for zcash-tx v5.5.0
+zcash-tx \- manual page for zcash-tx v5.5.1
 .SH DESCRIPTION
-Zcash zcash\-tx utility version v5.5.0
+Zcash zcash\-tx utility version v5.5.1
 .SS "Usage:"
 .TP
 zcash\-tx [options] <hex\-tx> [commands]
diff --git a/doc/man/zcashd-wallet-tool.1 b/doc/man/zcashd-wallet-tool.1
index 674602617e3..5c96fc00f79 100644
--- a/doc/man/zcashd-wallet-tool.1
+++ b/doc/man/zcashd-wallet-tool.1
@@ -1,7 +1,7 @@
 .\" DO NOT MODIFY THIS FILE!  It was generated by help2man 1.49.1.
-.TH ZCASHD-WALLET-TOOL "1" "April 2023" "zcashd-wallet-tool v5.5.0" "User Commands"
+.TH ZCASHD-WALLET-TOOL "1" "May 2023" "zcashd-wallet-tool v5.5.1" "User Commands"
 .SH NAME
-zcashd-wallet-tool \- manual page for zcashd-wallet-tool v5.5.0
+zcashd-wallet-tool \- manual page for zcashd-wallet-tool v5.5.1
 .SH SYNOPSIS
 .B zcashd-wallet-tool
 [\fI\,OPTIONS\/\fR]
diff --git a/doc/man/zcashd.1 b/doc/man/zcashd.1
index 1f032c443ea..42bf4d73619 100644
--- a/doc/man/zcashd.1
+++ b/doc/man/zcashd.1
@@ -1,9 +1,9 @@
 .\" DO NOT MODIFY THIS FILE!  It was generated by help2man 1.49.1.
-.TH ZCASHD "1" "April 2023" "zcashd v5.5.0" "User Commands"
+.TH ZCASHD "1" "May 2023" "zcashd v5.5.1" "User Commands"
 .SH NAME
-zcashd \- manual page for zcashd v5.5.0
+zcashd \- manual page for zcashd v5.5.1
 .SH DESCRIPTION
-Zcash Daemon version v5.5.0
+Zcash Daemon version v5.5.1
 .PP
 In order to ensure you are adequately protecting your privacy when using Zcash,
 please see <https://z.cash/support/security/>.
@@ -98,7 +98,7 @@ Keep at most <n> unconnectable transactions in memory (default: 100)
 .HP
 \fB\-par=\fR<n>
 .IP
-Set the number of script verification threads (\fB\-16\fR to 16, 0 = auto, <0 =
+Set the number of script verification threads (\fB\-8\fR to 16, 0 = auto, <0 =
 leave that many cores free, default: 0)
 .HP
 \fB\-pid=\fR<file>
diff --git a/doc/release-notes/release-notes-5.5.1.md b/doc/release-notes/release-notes-5.5.1.md
new file mode 100644
index 00000000000..484fb78e775
--- /dev/null
+++ b/doc/release-notes/release-notes-5.5.1.md
@@ -0,0 +1,31 @@
+Notable changes
+===============
+
+Fixes
+-----
+
+Fixes an issue that could cause a node to crash if the privacy policy does not
+include `AllowRevealedRecipients` when attempting to create a transaction that
+results in transparent change. See
+[#6662](https://github.com/zcash/zcash/pull/6662) for details.
+
+Also corrects an underestimate of the [ZIP 317](https://zips.z.cash/zip-0317)
+conventional fee when spending UTXOs, which can result in mining of the
+transaction being delayed or blocked. See
+[#6660](https://github.com/zcash/zcash/pull/6660) for details.
+
+Changelog
+=========
+
+Greg Pfeil (10):
+      Add a test for WalletTxBuilder with legacy account
+      Handle errors when getting change addr for account
+      Test that WalletTxBuilder uses correct ZIP 317 fee
+      Correct fee calculation for vin in tx creation
+      Calculate consensusBranchId sooner
+      Extract fee check from test
+      Delay postponed dependencies
+      Add release notes for v5.5.1
+      make-release.py: Versioning changes for 5.5.1.
+      make-release.py: Updated manpages for 5.5.1.
+
diff --git a/qa/zcash/postponed-updates.txt b/qa/zcash/postponed-updates.txt
index ddd1614e9b6..4b96cf6cd9f 100644
--- a/qa/zcash/postponed-updates.txt
+++ b/qa/zcash/postponed-updates.txt
@@ -6,23 +6,26 @@
 
 # CCache 4.7 appears to drop support for Ubuntu 18.04.
 # We will drop Ubuntu 18.04 no later than May 2023.
-native_ccache 4.7 2023-05-01
-native_ccache 4.7.1 2023-05-01
-native_ccache 4.7.2 2023-05-01
-native_ccache 4.7.3 2023-05-01
-native_ccache 4.7.4 2023-05-01
-native_ccache 4.7.5 2023-05-01
-native_ccache 4.8 2023-05-01
+native_ccache 4.7 2023-06-01
+native_ccache 4.7.1 2023-06-01
+native_ccache 4.7.2 2023-06-01
+native_ccache 4.7.3 2023-06-01
+native_ccache 4.7.4 2023-06-01
+native_ccache 4.7.5 2023-06-01
+native_ccache 4.8 2023-06-01
 
 # Clang and Rust are currently pinned to LLVM 15
-libcxx 15.0.7 2023-04-30
-libcxx 16.0.0 2023-04-30
-libcxx 16.0.1 2023-04-30
-libcxx 16.0.2 2023-04-30
-native_clang 15.0.7 2023-04-30
-native_clang 16.0.0 2023-04-30
-native_clang 16.0.1 2023-04-30
-native_clang 16.0.2 2023-04-30
+libcxx 15.0.7 2023-06-01
+libcxx 16.0.0 2023-06-01
+libcxx 16.0.1 2023-06-01
+libcxx 16.0.2 2023-06-01
+libcxx 16.0.2 2023-06-01
+libcxx 16.0.3 2023-06-01
+native_clang 15.0.7 2023-06-01
+native_clang 16.0.0 2023-06-01
+native_clang 16.0.1 2023-06-01
+native_clang 16.0.2 2023-06-01
+native_clang 16.0.3 2023-06-01
 
 # We follow upstream Bitcoin Core's LevelDB updates
 leveldb 1.23 2023-06-01
diff --git a/src/clientversion.h b/src/clientversion.h
index cd4a976ee25..617e28e597c 100644
--- a/src/clientversion.h
+++ b/src/clientversion.h
@@ -17,7 +17,7 @@
 //! These need to be macros, as clientversion.cpp's and bitcoin*-res.rc's voodoo requires it
 #define CLIENT_VERSION_MAJOR 5
 #define CLIENT_VERSION_MINOR 5
-#define CLIENT_VERSION_REVISION 0
+#define CLIENT_VERSION_REVISION 1
 #define CLIENT_VERSION_BUILD 50
 
 //! Set to true for release, false for prerelease or test build
diff --git a/src/deprecation.h b/src/deprecation.h
index b03fe072aee..9da0337c472 100644
--- a/src/deprecation.h
+++ b/src/deprecation.h
@@ -13,7 +13,7 @@
 // Per https://zips.z.cash/zip-0200
 // Shut down nodes running this version of code, 16 weeks' worth of blocks after the estimated
 // release block height. A warning is shown during the 14 days' worth of blocks prior to shut down.
-static const int APPROX_RELEASE_HEIGHT = 2067000;
+static const int APPROX_RELEASE_HEIGHT = 2088463;
 static const int RELEASE_TO_DEPRECATION_WEEKS = 16;
 static const int EXPECTED_BLOCKS_PER_HOUR = 3600 / Consensus::POST_BLOSSOM_POW_TARGET_SPACING;
 static_assert(EXPECTED_BLOCKS_PER_HOUR == 48, "The value of Consensus::POST_BLOSSOM_POW_TARGET_SPACING was chosen such that this assertion holds.");
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index b6891cd2a90..b1d21ffd918 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -2158,9 +2158,11 @@ bool CWallet::SelectorMatchesAddress(
     });
 }
 
-std::optional<RecipientAddress> CWallet::GenerateChangeAddressForAccount(
+tl::expected<RecipientAddress, AccountChangeAddressFailure>
+CWallet::GenerateChangeAddressForAccount(
         libzcash::AccountId accountId,
-        std::set<OutputPool> changeOptions) {
+        std::set<OutputPool> changeOptions)
+{
     AssertLockHeld(cs_wallet);
 
     if (accountId == ZCASH_LEGACY_ACCOUNT) {
@@ -2170,6 +2172,7 @@ std::optional<RecipientAddress> CWallet::GenerateChangeAddressForAccount(
                 return GenerateNewKey(false).GetID();
             }
         }
+        return tl::make_unexpected(AccountChangeAddressFailure::TransparentChangeNotPermitted);
     } else {
         auto ufvk = this->GetUnifiedFullViewingKeyByAccount(accountId);
         if (ufvk.has_value()) {
@@ -2197,10 +2200,11 @@ std::optional<RecipientAddress> CWallet::GenerateChangeAddressForAccount(
                     return changeAddr.value();
                 }
             }
+            return tl::make_unexpected(AccountChangeAddressFailure::DisjointReceivers);
+        } else {
+            return tl::make_unexpected(AccountChangeAddressFailure::NoSuchAccount);
         }
     }
-
-    return std::nullopt;
 }
 
 SpendableInputs CWallet::FindSpendableInputs(
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index e06c3282438..90dc0694859 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -1188,6 +1188,12 @@ class WalletBatchScanner : public BatchScanner {
         const int nHeight);
 };
 
+enum class AccountChangeAddressFailure {
+    DisjointReceivers,
+    TransparentChangeNotPermitted,
+    NoSuchAccount,
+};
+
 /**
  * A CWallet is an extension of a keystore, which also maintains a set of transactions and balances,
  * and provides the ability to create new transactions.
@@ -1607,7 +1613,8 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
      * Returns `std::nullopt` if the account does not have an internal spending
      * key matching the requested `OutputPool`.
      */
-    std::optional<libzcash::RecipientAddress> GenerateChangeAddressForAccount(
+    tl::expected<libzcash::RecipientAddress, AccountChangeAddressFailure>
+    GenerateChangeAddressForAccount(
             libzcash::AccountId accountId,
             std::set<libzcash::OutputPool> changeOptions);
 
diff --git a/src/wallet/wallet_tx_builder.cpp b/src/wallet/wallet_tx_builder.cpp
index 138da855f63..f2b31a3bfcf 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(
@@ -376,11 +386,20 @@ WalletTxBuilder::GetChangeAddress(
                     fvk.GetKnownReceiverTypes());
         },
         [&](const AccountZTXOPattern& acct) -> tl::expected<ChangeAddress, AddressResolutionError> {
-                auto addr = wallet.GenerateChangeAddressForAccount(
+            return wallet.GenerateChangeAddressForAccount(
                     acct.GetAccountId(),
-                    getAllowedChangePools(acct.GetReceiverTypes()));
-            assert(addr.has_value());
-            return addr.value();
+                    getAllowedChangePools(acct.GetReceiverTypes()))
+                .map_error([](auto err) {
+                    switch (err) {
+                        case AccountChangeAddressFailure::DisjointReceivers:
+                            return AddressResolutionError::CouldNotResolveReceiver;
+                        case AccountChangeAddressFailure::TransparentChangeNotPermitted:
+                            return AddressResolutionError::TransparentChangeNotAllowed;
+                        case AccountChangeAddressFailure::NoSuchAccount:
+                            throw std::runtime_error("Selector account doesn’t exist.");
+                    }
+                });
+
         }
     });
 }
@@ -405,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(
@@ -414,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);
@@ -475,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>
@@ -535,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};
@@ -584,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(
@@ -626,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);
 
@@ -720,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));
             }
@@ -731,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 fdb65952560..62b2e61b027 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 09c9eca44c6..f98f93ba3a6 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 84f1d18660f..3201bdebde1 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 9cf10cad156..29409d03b98 100644
--- a/src/wallet/gtest/test_rpc_wallet.cpp
+++ b/src/wallet/gtest/test_rpc_wallet.cpp
@@ -8,16 +8,18 @@
 #include "transaction_builder.h"
 #include "util/test.h"
 #include "gtest/utils.h"
+#include "wallet/asyncrpcoperation_common.h"
 #include "wallet/asyncrpcoperation_shieldcoinbase.h"
 #include "wallet/asyncrpcoperation_sendmany.h"
 #include "zcash/JoinSplit.hpp"
 #include "zcash/memo.h"
+#include "zip317.h"
 
 #include <librustzcash.h>
 #include <rust/ed25519.h>
 
-namespace {
-
+namespace
+{
 bool find_error(const UniValue& objError, const std::string& expected) {
     return find_value(objError, "message").get_str().find(expected) != string::npos;
 }
@@ -29,6 +31,124 @@ CWalletTx FakeWalletTx() {
     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)
+{
+    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;
+        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);
+
+        WalletTxBuilder builder(Params(), minRelayTxFee);
+
+        auto selector = CWallet::LegacyTransparentZTXOSelector(
+                true,
+                TransparentCoinbasePolicy::Disallow);
+
+        { // send from legacy account with change, but insufficient policy
+            auto saplingKey = pwalletMain->GenerateNewLegacySaplingZKey();
+            Payment saplingPayment(saplingKey, 4 * COIN, std::nullopt);
+            std::vector<Payment> payments {saplingPayment};
+
+            TransactionStrategy strategy(PrivacyPolicy::AllowRevealedSenders);
+
+            SpendableInputs inputs;
+            inputs.utxos.emplace_back(COutput(&wtx, 0, 100, true));
+
+            builder.PrepareTransaction(
+                    *pwalletMain,
+                    selector,
+                    inputs,
+                    payments,
+                    chainActive,
+                    strategy,
+                    std::nullopt,
+                    1)
+                .map_error([&](const auto& err) {
+                    examine(err, match {
+                        [](AddressResolutionError are) {
+                            EXPECT_EQ(are, AddressResolutionError::TransparentChangeNotAllowed);
+                        },
+                        [&](const auto& e) {
+                            try {
+                                ThrowInputSelectionError(e, selector, strategy);
+                            } catch (const UniValue& value) {
+                                FAIL() << value.write();
+                            }
+                        },
+                    });
+                })
+                .map([](const auto&) {
+                    FAIL() << "Expected an error";
+                });
+        }
+
+        // Tear down
+        chainActive.SetTip(NULL);
+        mapBlockIndex.erase(blockHash);
+
+    }
+    // Revert to default
+    RegtestDeactivateSapling();
+    UnloadGlobalWallet();
 }
 
 // TODO: test private methods
@@ -44,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
@@ -71,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(
@@ -112,6 +240,11 @@ TEST(WalletRPCTests, RPCZMergeToAddressInternals)
             }));
         })
         .map([](const auto&) { EXPECT_TRUE(false); });
+
+    // Tear down
+    chainActive.SetTip(NULL);
+    mapBlockIndex.erase(blockHash);
+
     }
     UnloadGlobalWallet();
 }
@@ -131,8 +264,6 @@ TEST(WalletRPCTests, RPCZsendmanyTaddrToSapling)
         pwalletMain->GenerateNewSeed();
     }
 
-    UniValue retValue;
-
     KeyIO keyIO(Params());
     // add keys manually
     auto taddr = pwalletMain->GenerateNewKey(true).GetID();
@@ -239,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();
+}