diff --git a/qa/rpc-tests/test_framework/zip317.py b/qa/rpc-tests/test_framework/zip317.py index 93fd12dc9..dbe8f7e51 100644 --- a/qa/rpc-tests/test_framework/zip317.py +++ b/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 diff --git a/src/wallet/asyncrpcoperation_common.cpp b/src/wallet/asyncrpcoperation_common.cpp index 87f3b5243..796a58d81 100644 --- a/src/wallet/asyncrpcoperation_common.cpp +++ b/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)", diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 2958b11cb..27d0732d2 100644 --- a/src/wallet/rpcwallet.cpp +++ b/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" diff --git a/src/wallet/wallet_tx_builder.cpp b/src/wallet/wallet_tx_builder.cpp index 63bacc4e3..057274614 100644 --- a/src/wallet/wallet_tx_builder.cpp +++ b/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& inputs, const std::vector& payments, const std::optional& changeAddr) { - std::vector vouts{}; + std::vector 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 vin{}; + size_t sproutInputCount = 0; + size_t saplingInputCount = 0; + size_t orchardInputCount = 0; if (inputs.has_value()) { - std::vector vins{}; - for (const auto& utxo : inputs.value().utxos) { - vins.emplace_back( + for (const auto& utxo : inputs->utxos) { + vin.emplace_back( COutPoint(utxo.tx->GetHash(), utxo.i), utxo.tx->vout[utxo.i].scriptPubKey); } - logicalActionCount = CalculateLogicalActionCount( - vins, - vouts, - 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)); + sproutInputCount = inputs->sproutNoteEntries.size(); + saplingInputCount = inputs->saplingNoteEntries.size(); + orchardInputCount = inputs->orchardNoteMetadata.size(); } + 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 - // funds to cover the total payments + fee. + // we can only select Orchard addresses if we’re not sending from Sprout, since there is no tx + // version where both Sprout and Orchard are valid. bool canResolveOrchard = afterNU5 && !selector.SelectsSprout(); std::vector resolvedPayments; std::optional resolutionError; diff --git a/src/wallet/wallet_tx_builder.h b/src/wallet/wallet_tx_builder.h index 3c1e53b5d..d44b665bc 100644 --- a/src/wallet/wallet_tx_builder.h +++ b/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; diff --git a/src/zip317.cpp b/src/zip317.cpp index b78ab1a61..b9b65e7c8 100644 --- a/src/zip317.cpp +++ b/src/zip317.cpp @@ -15,9 +15,9 @@ CAmount CalculateConventionalFee(size_t logicalActionCount) { } template -size_t GetUTXOFieldSize(const std::vector& utxos) { - auto size = GetSerializeSize(utxos, SER_NETWORK, PROTOCOL_VERSION); - auto countSize = GetSizeOfCompactSize(utxos.size()); +static size_t GetTxIOFieldSize(const std::vector& txIOs) { + auto size = GetSerializeSize(txIOs, SER_NETWORK, PROTOCOL_VERSION); + 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_out_total_size = GetUTXOFieldSize(vout); + const size_t tx_in_total_size = GetTxIOFieldSize(vin); + 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)) +