Address review comments re: legacy wallet ZIP 317

qa/rpc-tests/wallet_shieldingcoinbase.py

@@ -319,8 +319,10 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework):
         assert_equal("Amount out of range" in errorString, True)
 
         # Send will fail because fee is more than `WEIGHT_RATIO_CAP * conventional_fee`
-        myopid = self.nodes[0].z_sendmany(myzaddr, recipients, 1, (WEIGHT_RATIO_CAP * many_recipient_fee) + Decimal('0.00000001'))
-        wait_and_assert_operationid_status(self.nodes[0], myopid, 'failed', 'Fee 0.40000001 is greater than 4 times the conventional fee for this tx (which is 0.10). There is no prioritisation benefit to a fee this large (see https://zips.z.cash/zip-0317#recommended-algorithm-for-block-template-construction), and it likely indicates a mistake in setting the fee.')
+        num_fewer_recipients = 400
+        fewer_recipients = recipients[0:num_fewer_recipients]
+        myopid = self.nodes[0].z_sendmany(myzaddr, fewer_recipients, 1, (WEIGHT_RATIO_CAP * conventional_fee(2+num_fewer_recipients)) + Decimal('0.00000001'))
+        wait_and_assert_operationid_status(self.nodes[0], myopid, 'failed', 'Fee 0.08040001 is greater than 4 times the conventional fee for this tx (which is 0.0201). There is no prioritisation benefit to a fee this large (see https://zips.z.cash/zip-0317#recommended-algorithm-for-block-template-construction), and it likely indicates a mistake in setting the fee.')
 
         # Send will succeed because the balance of non-coinbase utxos is 10.0
         try:

src/amount.cpp

@@ -5,6 +5,7 @@
 // file COPYING or https://www.opensource.org/licenses/mit-license.php .
 
 #include "amount.h"
+#include "consensus/consensus.h"
 #include "policy/fees.h"
 
 #include "tinyformat.h"
@@ -14,10 +15,11 @@ const std::string MINOR_CURRENCY_UNIT = "zatoshis";
 
 CFeeRate::CFeeRate(const CAmount& nFeePaid, size_t nSize)
 {
-    if (nSize > 0)
-        nSatoshisPerK = nFeePaid*1000/nSize;
-    else
+    if (nSize > 0) {
+        nSatoshisPerK = std::min(nFeePaid*1000/nSize, (uint64_t)INT64_MAX / MAX_BLOCK_SIZE);
+    } else {
         nSatoshisPerK = 0;
+    }
 }
 
 CAmount CFeeRate::GetFeeForRelay(size_t nSize) const

src/main.h

@@ -76,6 +76,8 @@ static const CAmount DEFAULT_TRANSACTION_MAXFEE = 0.1 * COIN;
 static const CAmount HIGH_TX_FEE_PER_KB = 0.01 * COIN;
 /** Warn if -maxtxfee is set to a fee higher than this in zatoshis. */
 static const CAmount HIGH_MAX_TX_FEE = 100 * HIGH_TX_FEE_PER_KB;
+//! -maxtxfee will error if called with a fee that won’t allow tx to have this many actions
+static const unsigned int LOW_LOGICAL_ACTIONS = 10;
 /** Default for -maxorphantx, maximum number of orphan transactions kept in memory */
 static const unsigned int DEFAULT_MAX_ORPHAN_TRANSACTIONS = 100;
 /** Default for -limitancestorcount, max number of in-mempool ancestors */

src/util/moneystr.cpp

@@ -33,6 +33,10 @@ std::string FormatMoney(const CAmount& n)
     return str;
 }
 
+std::string DisplayMoney(const CAmount& zat)
+{
+    return CURRENCY_UNIT + " " + FormatMoney(zat);
+}
 
 bool ParseMoney(const string& str, CAmount& nRet)
 {

src/util/moneystr.h

@@ -16,6 +16,9 @@
 #include "amount.h"
 
 std::string FormatMoney(const CAmount& n);
+/// Like `FormatMoney`, but meant to be human-readable, not parseable. E.g., it includes the
+/// `CURRENCY_UNIT` in the result.
+std::string DisplayMoney(const CAmount& n);
 bool ParseMoney(const std::string& str, CAmount& nRet);
 bool ParseMoney(const char* pszIn, CAmount& nRet);
 

src/wallet/asyncrpcoperation_common.cpp

@@ -183,10 +183,10 @@ void ThrowInputSelectionError(
             throw JSONRPCError(
                 RPC_INVALID_PARAMETER,
                 strprintf(
-                    "Fee %s is greater than the maximum fee allowed by this instance (%s). Run "
+                    "Fee (%s) is greater than the maximum fee allowed by this instance (%s). Run "
                     "zcashd with `-maxtxfee` to adjust this limit.",
-                    FormatMoney(err.fixedFee),
-                    FormatMoney(maxTxFee)));
+                    DisplayMoney(err.fixedFee),
+                    DisplayMoney(maxTxFee)));
         },
         [](const ExcessOrchardActionsError& err) {
             std::string side;

src/wallet/wallet.cpp

@@ -30,6 +30,7 @@
 #include "zcash/Address.hpp"
 #include "zcash/JoinSplit.hpp"
 #include "zcash/Note.hpp"
+#include "zip317.h"
 #include "crypter.h"
 #include "wallet/asyncrpcoperation_saplingmigration.h"
 
@@ -5801,12 +5802,7 @@ CAmount CWallet::ConstrainFee(CAmount requestedFee, unsigned int nTxBytes)
 CAmount CWallet::GetMinimumFee(const CTransaction& tx, unsigned int nTxBytes)
 {
     // payTxFee is user-set "I want to pay this much"
-    CAmount nFeeNeeded = payTxFee.GetFee(nTxBytes);
-    // TODO: fPayAtLeastCustomFee is always true so we could simplify this by saying
-    // `payTxFee.GetFee(std::max(1000, nTxBytes))` above, if we do not remove this code
-    // completely.
-    if (fPayAtLeastCustomFee && nFeeNeeded > 0 && nFeeNeeded < payTxFee.GetFeePerK())
-        nFeeNeeded = payTxFee.GetFeePerK();
+    CAmount nFeeNeeded = payTxFee.GetFee(std::max((unsigned int)1000, nTxBytes));
     // User didn't set: use conventional fee
     if (nFeeNeeded == 0)
         nFeeNeeded = tx.GetConventionalFee();
@@ -5814,8 +5810,6 @@ CAmount CWallet::GetMinimumFee(const CTransaction& tx, unsigned int nTxBytes)
 }
 
 
-
-
 DBErrors CWallet::LoadWallet(bool& fFirstRunRet)
 {
     if (!fFileBacked)
@@ -6728,10 +6722,10 @@ bool CWallet::ParameterInteraction(const CChainParams& params)
         if (nMaxFee > HIGH_MAX_TX_FEE)
             UIWarning(_("-maxtxfee is set to a very high fee rate! Fee rates this large could be paid on a single transaction."));
         maxTxFee = nMaxFee;
-        if (CFeeRate(maxTxFee, 1000) < ::minRelayTxFee)
+        if (maxTxFee < CalculateConventionalFee(LOW_LOGICAL_ACTIONS))
         {
-            return UIError(strprintf(_("Invalid amount for -maxtxfee=<amount>: '%s' (must be at least the minimum relay fee rate of %s to prevent stuck transactions)"),
-                                       mapArgs["-maxtxfee"], ::minRelayTxFee.ToString()));
+            return UIError(strprintf(_("Invalid amount for -maxtxfee=<amount>: '%s' (must allow for at least %d logical actions at the conventional fee)"),
+                                       mapArgs["-maxtxfee"], LOW_LOGICAL_ACTIONS));
         }
     }
     nTxConfirmTarget = GetArg("-txconfirmtarget", DEFAULT_TX_CONFIRM_TARGET);

src/wallet/wallet_tx_builder.cpp

@@ -2,6 +2,7 @@
 // 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 "zip317.h"
 
@@ -962,11 +963,9 @@ TransactionBuilderResult TransactionEffects::ApproveAndBuild(
         if (fee < minRelayFee) {
             return TransactionBuilderResult(
                     strprintf(
-                            "Fee (%s %d) is below the minimum relay fee for this transaction (%s %d)",
-                            CURRENCY_UNIT,
-                            fee,
-                            CURRENCY_UNIT,
-                            minRelayFee));
+                            "Fee (%s) is below the minimum relay fee for this transaction (%s)",
+                            DisplayMoney(fee),
+                            DisplayMoney(minRelayFee)));
         }
     }