Address review feedback for ZIP 317 fees in wallet

qa/rpc-tests/test_framework/zip317.py

@@ -12,7 +12,7 @@
 from test_framework.mininode import COIN
 from decimal import Decimal
 
-# The fee per logical action, in ZAT. See https://zips.z.cash/zip-0317#fee-calculation.
+# The fee per logical action, in zatoshis. See https://zips.z.cash/zip-0317#fee-calculation.
 MARGINAL_FEE = 5000
 
 # The lower bound on the number of logical actions in a tx, for purposes of fee calculation. See
@@ -24,7 +24,7 @@ GRACE_ACTIONS = 2
 # https://zips.z.cash/zip-0317#recommended-algorithm-for-block-template-construction
 WEIGHT_RATIO_CAP = 4
 
-# The Zcashd RPC sentinel value to indicate the conventional_fee when a positional argument is
+# The zcashd RPC sentinel value to indicate the conventional_fee when a positional argument is
 # required.
 ZIP_317_FEE = -1
 

src/wallet/asyncrpcoperation_common.cpp

@@ -124,7 +124,7 @@ void ThrowInputSelectionError(
                     "Insufficient funds: have %s, %s",
                     FormatMoney(err.available),
                     examine(err.reason, match {
-                        [](const QuasiChangeError& qce) {
+                        [](const PhantomChangeError& qce) {
                             return strprintf(
                                     "need %s more to surpass the dust threshold and avoid being "
                                     "forced to over-pay the fee. Alternatively, you could specify "
@@ -137,7 +137,7 @@ void ThrowInputSelectionError(
                             return strprintf("need %s", FormatMoney(ife.required));
                         },
                         // TODO: Add the fee here, so we can suggest specifying an explicit fee (see
-                        //       `QuasiChangeError`).
+                        //       `PhantomChangeError`).
                         [](const DustThresholdError& dte) {
                             return strprintf(
                                     "need %s more to avoid creating invalid change output %s (dust threshold is %s)",

src/wallet/rpcwallet.cpp

@@ -4813,7 +4813,7 @@ UniValue z_sendmany(const UniValue& params, bool fHelp)
             "                           the output is being sent to a transparent address, it’s an error to include this field.\n"
             "    }, ... ]\n"
             "3. minconf               (numeric, optional, default=" + strprintf("%u", DEFAULT_NOTE_CONFIRMATIONS) + ") Only use funds confirmed at least this many times.\n"
-            "4. fee                   (numeric, optional, default=-1) The fee amount to attach to this transaction. The default behavior\n"
+            "4. fee                   (numeric, optional, default=-1) The fee amount in " + CURRENCY_UNIT + " to attach to this transaction. The default behavior\n"
             "                         is to use a fee calculated according to ZIP 317.\n"
             "5. privacyPolicy         (string, optional, default=\"LegacyCompat\") Policy for what information leakage is acceptable.\n"
             "                         One of the following strings:\n"

src/wallet/wallet_tx_builder.cpp

@@ -200,31 +200,26 @@ SpendableInputs WalletTxBuilder::FindAllSpendableInputs(
     return wallet.FindSpendableInputs(selector, minDepth, std::nullopt);
 }
 
-static size_t NotOne(size_t n)
+static size_t PadCount(size_t n)
 {
     return n == 1 ? 2 : n;
 }
 
-/// ISSUES:
-/// • z_shieldcoinbase currently checks the fee in advance of calling PrepareTransaction in order to
-///   determine the payment amount. But that fee possibly isn’t correct. We could perhaps have _no_
-///   payments in z_shieldcoinbase, but provide an explicit change address? (Don’t expose this via
-///   RPC, and give it a different name, e.g. `NetFundsRecipient` in the WalletTxBuilder interface)
 static CAmount
 CalcZIP317Fee(
         const std::optional<SpendableInputs>& inputs,
         const std::vector<ResolvedPayment>& payments,
         const std::optional<ChangeAddress>& changeAddr)
 {
-    std::vector<CTxOut> vouts{};
+    std::vector<CTxOut> vout{};
     size_t sproutOutputCount{}, saplingOutputCount{}, orchardOutputCount{};
     for (const auto& payment : payments) {
         std::visit(match {
             [&](const CKeyID& addr) {
-                vouts.emplace_back(payment.amount, GetScriptForDestination(addr));
+                vout.emplace_back(payment.amount, GetScriptForDestination(addr));
             },
             [&](const CScriptID& addr) {
-                vouts.emplace_back(payment.amount, GetScriptForDestination(addr));
+                vout.emplace_back(payment.amount, GetScriptForDestination(addr));
             },
             [&](const libzcash::SaplingPaymentAddress&) {
                 ++saplingOutputCount;
@@ -241,10 +236,10 @@ CalcZIP317Fee(
             [&](const RecipientAddress& addr) {
                 examine(addr, match {
                     [&](const CKeyID& taddr) {
-                        vouts.emplace_back(0, GetScriptForDestination(taddr));
+                        vout.emplace_back(0, GetScriptForDestination(taddr));
                     },
                     [&](const CScriptID taddr) {
-                        vouts.emplace_back(0, GetScriptForDestination(taddr));
+                        vout.emplace_back(0, GetScriptForDestination(taddr));
                     },
                     [&](const libzcash::SaplingPaymentAddress&) { ++saplingOutputCount; },
                     [&](const libzcash::OrchardRawAddress&) { ++orchardOutputCount; }
@@ -253,38 +248,34 @@ CalcZIP317Fee(
         });
     }
 
-    size_t logicalActionCount;
+    std::vector<CTxIn> vin{};
+    size_t sproutInputCount = 0;
+    size_t saplingInputCount = 0;
+    size_t orchardInputCount = 0;
     if (inputs.has_value()) {
-        std::vector<CTxIn> vins{};
+        for (const auto& utxo : inputs->utxos) {
-        for (const auto& utxo : inputs.value().utxos) {
+            vin.emplace_back(
-            vins.emplace_back(
                     COutPoint(utxo.tx->GetHash(), utxo.i),
                     utxo.tx->vout[utxo.i].scriptPubKey);
         }
-        logicalActionCount = CalculateLogicalActionCount(
+        sproutInputCount = inputs->sproutNoteEntries.size();
-                vins,
+        saplingInputCount = inputs->saplingNoteEntries.size();
-                vouts,
+        orchardInputCount = inputs->orchardNoteMetadata.size();
-                std::max(inputs.value().sproutNoteEntries.size(), sproutOutputCount),
-                inputs.value().saplingNoteEntries.size(),
-                NotOne(saplingOutputCount),
-                NotOne(std::max(inputs.value().orchardNoteMetadata.size(), orchardOutputCount)));
-
-    } else {
-        logicalActionCount = CalculateLogicalActionCount(
-                {},
-                vouts,
-                sproutOutputCount,
-                0,
-                NotOne(saplingOutputCount),
-                NotOne(orchardOutputCount));
     }
+    size_t logicalActionCount = CalculateLogicalActionCount(
+            vin,
+            vout,
+            std::max(sproutInputCount, sproutOutputCount),
+            saplingInputCount,
+            PadCount(saplingOutputCount),
+            PadCount(std::max(orchardInputCount, orchardOutputCount)));
 
     return CalculateConventionalFee(logicalActionCount);
 }
 
 InvalidFundsError ReportInvalidFunds(
         const SpendableInputs& spendable,
-        bool hasQuasiChange,
+        bool hasPhantomChange,
         CAmount fee,
         CAmount dustThreshold,
         CAmount targetAmount,
@@ -292,9 +283,9 @@ InvalidFundsError ReportInvalidFunds(
 {
     return InvalidFundsError(
             spendable.Total(),
-            hasQuasiChange
+            hasPhantomChange
             // TODO: NEED TESTS TO EXERCISE THIS
-            ? InvalidFundsReason(QuasiChangeError(fee, dustThreshold))
+            ? InvalidFundsReason(PhantomChangeError(fee, dustThreshold))
             : (changeAmount > 0 && changeAmount < dustThreshold
                // TODO: we should provide the option for the caller to explicitly forego change
                // (definitionally an amount below the dust amount) and send the extra to the
@@ -367,6 +358,7 @@ WalletTxBuilder::IterateLimit(
     CAmount targetAmount{0};
 
     do {
+        // NB: This makes a fresh copy so that we start from the full set of notes when we re-limit.
         spendableMut = spendable;
 
         targetAmount = sendAmount + updatedFee;
@@ -459,8 +451,8 @@ WalletTxBuilder::ResolveInputsAndPayments(
     CAmount maxOrchardAvailable = spendableMut.GetOrchardTotal();
     uint32_t orchardOutputs{0};
 
-    // we can only select Orchard addresses if there are sufficient non-Sprout
+    // we can only select Orchard addresses if we’re not sending from Sprout, since there is no tx
-    // funds to cover the total payments + fee.
+    // version where both Sprout and Orchard are valid.
     bool canResolveOrchard = afterNU5 && !selector.SelectsSprout();
     std::vector<ResolvedPayment> resolvedPayments;
     std::optional<AddressResolutionError> resolutionError;

src/wallet/wallet_tx_builder.h

@@ -241,12 +241,16 @@ enum class AddressResolutionError {
     RevealingReceiverAmountsNotAllowed,
 };
 
-class QuasiChangeError {
+/// Phantom change is change that appears to exist until we add the output for it, at which point it
+/// is consumed by the increase to the conventional fee. When we are at the limit of selectable
+/// notes, this makes it impossible to create the transaction without either creating a 0-valued
+/// output or overpaying the fee.
+class PhantomChangeError {
 public:
     CAmount finalFee;
     CAmount dustThreshold;
 
-    QuasiChangeError(CAmount finalFee, CAmount dustThreshold):
+    PhantomChangeError(CAmount finalFee, CAmount dustThreshold):
         finalFee(finalFee), dustThreshold(dustThreshold) { }
 };
 
@@ -268,7 +272,7 @@ public:
 };
 
 typedef std::variant<
-    QuasiChangeError,
+    PhantomChangeError,
     InsufficientFundsError,
     DustThresholdError> InvalidFundsReason;
 

src/zip317.cpp

@@ -15,9 +15,9 @@ CAmount CalculateConventionalFee(size_t logicalActionCount) {
 }
 
 template<typename T>
-size_t GetUTXOFieldSize(const std::vector<T>& utxos) {
+static size_t GetTxIOFieldSize(const std::vector<T>& txIOs) {
-    auto size = GetSerializeSize(utxos, SER_NETWORK, PROTOCOL_VERSION);
+    auto size = GetSerializeSize(txIOs, SER_NETWORK, PROTOCOL_VERSION);
-    auto countSize = GetSizeOfCompactSize(utxos.size());
+    auto countSize = GetSizeOfCompactSize(txIOs.size());
     return size - countSize;
 }
 
@@ -28,8 +28,8 @@ size_t CalculateLogicalActionCount(
         unsigned int saplingSpendCount,
         unsigned int saplingOutputCount,
         unsigned int orchardActionCount) {
-    const size_t tx_in_total_size = GetUTXOFieldSize(vin);
+    const size_t tx_in_total_size = GetTxIOFieldSize(vin);
-    const size_t tx_out_total_size = GetUTXOFieldSize(vout);
+    const size_t tx_out_total_size = GetTxIOFieldSize(vout);
 
     return std::max(ceil_div(tx_in_total_size, P2PKH_STANDARD_INPUT_SIZE),
                     ceil_div(tx_out_total_size, P2PKH_STANDARD_OUTPUT_SIZE)) +