Adjust wallet absurd fee check for ZIP 317

It now happens async (in `PrepareTransaction`) and ensures that the fee doesn’t
exceed the maximum useful value for a transaction.

qa/rpc-tests/test_framework/zip317.py

@@ -19,6 +19,11 @@ MARGINAL_FEE = 5000
 # https://zips.z.cash/zip-0317#fee-calculation.
 GRACE_ACTIONS = 2
 
+# Limits the relative probability of picking a given transaction to be at most `WEIGHT_RATIO_CAP`
+# times greater than a transaction that pays exactly the conventional fee. See
+# 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
 # required.
 ZIP_317_FEE = -1

qa/rpc-tests/wallet_parsing_amounts.py

@@ -70,19 +70,6 @@ class WalletAmountParsingTest(BitcoinTestFramework):
             print(errorString)
             assert(False)
 
-        # This fee is larger than the default fee and since amount=0
-        # it should trigger error
-        fee         = Decimal('0.1')
-        recipients  = [ {"address": myzaddr, "amount": Decimal('0.0') } ]
-        minconf     = 1
-        errorString = ''
-
-        try:
-            myopid = self.nodes[0].z_sendmany(myzaddr, recipients, minconf, fee)
-        except JSONRPCException as e:
-            errorString = e.error['message']
-        assert('Small transaction amount' in errorString)
-
         # This fee is less than default and greater than amount, but still valid
         fee         = Decimal('0.0000001')
         recipients  = [ {"address": myzaddr, "amount": Decimal('0.00000001') } ]

qa/rpc-tests/wallet_shieldingcoinbase.py

@@ -10,7 +10,7 @@ from test_framework.util import assert_equal, initialize_chain_clean, \
     start_nodes, connect_nodes_bi, wait_and_assert_operationid_status, \
     wait_and_assert_operationid_status_result, get_coinbase_address, \
     check_node_log, DEFAULT_FEE
-from test_framework.zip317 import conventional_fee, ZIP_317_FEE
+from test_framework.zip317 import conventional_fee, WEIGHT_RATIO_CAP, ZIP_317_FEE
 
 import sys
 import timeit
@@ -299,9 +299,11 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework):
         self.nodes[1].generate(1)
         self.sync_all()
 
+        many_recipient_fee = conventional_fee(2+num_t_recipients) # 2+ for padded Sapling input/change
+
         # check balance
         node2balance = amount_per_recipient * num_t_recipients
-        saplingvalue -= node2balance + conventional_fee(2+num_t_recipients) # 2+ for padded Sapling input/change
+        saplingvalue -= node2balance + many_recipient_fee
         assert_equal(self.nodes[2].getbalance(), node2balance)
         check_value_pool(self.nodes[0], 'sapling', saplingvalue)
 
@@ -320,12 +322,9 @@ class WalletShieldingCoinbaseTest (BitcoinTestFramework):
             errorString = e.error['message']
         assert_equal("Amount out of range" in errorString, True)
 
-        # Send will fail because fee is larger than sum of outputs
-        try:
-            self.nodes[0].z_sendmany(myzaddr, recipients, 1, (amount_per_recipient * num_t_recipients) + Decimal('0.00000001'))
-        except JSONRPCException as e:
-            errorString = e.error['message']
-        assert_equal("is greater than the sum of outputs" 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 prioritization benefit to a fee this large (see https://zips.z.cash/zip-0317#recommended-algorithm-for-block-template-construction) and likely indicates a mistake in setting the fee.')
 
         # Send will succeed because the balance of non-coinbase utxos is 10.0
         try:

src/wallet/asyncrpcoperation_common.cpp

@@ -4,6 +4,7 @@
 #include "init.h"
 #include "rpc/protocol.h"
 #include "util/moneystr.h"
+#include "zip317.h"
 
 extern UniValue signrawtransaction(const UniValue& params, bool fHelp);
 
@@ -166,6 +167,18 @@ void ThrowInputSelectionError(
                     "The proposed transaction would result in %s in change.",
                     FormatMoney(err.available - err.required)));
         },
+        [](const AbsurdFeeError& err) {
+            throw JSONRPCError(
+                RPC_INVALID_PARAMETER,
+                strprintf(
+                    "Fee %s is greater than %d times the conventional fee for this tx (which is "
+                    "%s). There is no prioritization benefit to a fee this large (see "
+                    "https://zips.z.cash/zip-0317#recommended-algorithm-for-block-template-construction) "
+                    "and likely indicates a mistake in setting the fee.",
+                    FormatMoney(err.fixedFee),
+                    WEIGHT_RATIO_CAP,
+                    FormatMoney(err.conventionalFee)));
+        },
         [](const ExcessOrchardActionsError& err) {
             std::string side;
             switch (err.side) {

src/wallet/rpcwallet.cpp

@@ -5031,26 +5031,7 @@ UniValue z_sendmany(const UniValue& params, bool fHelp)
     // Fee in Zatoshis, not currency format)
     std::optional<CAmount> nFee;
     if (params.size() > 3 && params[3].get_real() != -1.0) {
-        CAmount fixedFee = AmountFromValue( params[3] );
-
-        // Check that the user specified fee is not absurd.
-        // This allows amount=0 (and all amount < DEFAULT_FEE) transactions to use the default network fee
-        // or anything less than DEFAULT_FEE instead of being forced to use a custom fee and leak metadata
-        if (nTotalOut < DEFAULT_FEE) {
-            if (fixedFee > DEFAULT_FEE) {
-                throw JSONRPCError(
-                        RPC_INVALID_PARAMETER,
-                        strprintf("Small transaction amount %s has fee %s that is greater than the default fee %s", FormatMoney(nTotalOut), FormatMoney(fixedFee), FormatMoney(DEFAULT_FEE)));
-            }
-        } else {
-            // Check that the user specified fee is not absurd.
-            if (fixedFee > nTotalOut) {
-                throw JSONRPCError(
-                        RPC_INVALID_PARAMETER,
-                        strprintf("Fee %s is greater than the sum of outputs %s and also greater than the default fee", FormatMoney(fixedFee), FormatMoney(nTotalOut)));
-            }
-        }
-        nFee = fixedFee;
+        nFee = AmountFromValue( params[3] );
     }
 
     // Use input parameters as the optional context info to be returned by z_getoperationstatus and z_getoperationresult.

src/wallet/wallet_tx_builder.cpp

@@ -602,11 +602,27 @@ WalletTxBuilder::ResolveInputsAndPayments(
             } else {
                 return tl::make_unexpected(possibleChangeAddr.error());
             }
+
+            // TODO: This duplicates the check in the `else` branch of the containing `if`. Until we
+            //       can add Sprout change to `Payments` (#5660), we need to check this before
+            //       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);
+            if (finalFee > WEIGHT_RATIO_CAP * conventionalFee) {
+                return tl::make_unexpected(AbsurdFeeError(conventionalFee, finalFee));
+            }
             auto changeRes =
                 AddChangePayment(spendableMut, resolved, changeAddr.value(), changeAmount, targetAmount);
             if (!changeRes.has_value()) {
                 return tl::make_unexpected(changeRes.error());
             }
+        } else {
+            auto conventionalFee =
+                CalcZIP317Fee(spendableMut, resolved.GetResolvedPayments(), std::nullopt);
+            if (finalFee > WEIGHT_RATIO_CAP * conventionalFee) {
+                return tl::make_unexpected(AbsurdFeeError(resolved.Total(), finalFee));
+            }
         }
     } else {
         auto limitResult = IterateLimit(wallet, selector, strategy, sendAmount, dustThreshold, spendableMut, resolved, afterNU5);
@@ -915,7 +931,7 @@ TransactionBuilderResult TransactionEffects::ApproveAndBuild(
     // TODO: We currently can’t store Sprout change in `Payments`, so we only validate the
     //       spend/output balance in the case that `TransactionBuilder` doesn’t need to
     //       (re)calculate the change. In future, we shouldn’t rely on `TransactionBuilder` ever
-    //       calculating change.
+    //       calculating change. (#5660)
     if (changeAddr.has_value()) {
         examine(changeAddr.value(), match {
             [&](const SproutPaymentAddress& addr) {

src/wallet/wallet_tx_builder.h

@@ -290,6 +290,17 @@ public:
         available(available), required(required) { }
 };
 
+/// Error when a fee is higher than can be useful. This reduces the chance of accidentally
+/// overpaying with explicit fees.
+class AbsurdFeeError {
+public:
+    CAmount conventionalFee;
+    CAmount fixedFee;
+
+    AbsurdFeeError(CAmount conventionalFee, CAmount fixedFee):
+        conventionalFee(conventionalFee), fixedFee(fixedFee) { }
+};
+
 enum ActionSide {
     Input,
     Output,
@@ -310,6 +321,7 @@ typedef std::variant<
     AddressResolutionError,
     InvalidFundsError,
     ChangeNotAllowedError,
+    AbsurdFeeError,
     ExcessOrchardActionsError> InputSelectionError;
 
 class InputSelection {