From c54c4ee987bb5b9f46b9c48467d0e93201c69538 Mon Sep 17 00:00:00 2001 From: Greg Pfeil Date: Thu, 13 Apr 2023 18:38:21 -0600 Subject: [PATCH] Adjust wallet absurd fee check for ZIP 317 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 5 +++++ qa/rpc-tests/wallet_parsing_amounts.py | 13 ------------- qa/rpc-tests/wallet_shieldingcoinbase.py | 15 +++++++-------- src/wallet/asyncrpcoperation_common.cpp | 13 +++++++++++++ src/wallet/rpcwallet.cpp | 21 +-------------------- src/wallet/wallet_tx_builder.cpp | 18 +++++++++++++++++- src/wallet/wallet_tx_builder.h | 12 ++++++++++++ 7 files changed, 55 insertions(+), 42 deletions(-) diff --git a/qa/rpc-tests/test_framework/zip317.py b/qa/rpc-tests/test_framework/zip317.py index b5192f7eb..93fd12dc9 100644 --- a/qa/rpc-tests/test_framework/zip317.py +++ b/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 diff --git a/qa/rpc-tests/wallet_parsing_amounts.py b/qa/rpc-tests/wallet_parsing_amounts.py index bdfdf42c3..ff792b934 100755 --- a/qa/rpc-tests/wallet_parsing_amounts.py +++ b/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') } ] diff --git a/qa/rpc-tests/wallet_shieldingcoinbase.py b/qa/rpc-tests/wallet_shieldingcoinbase.py index 2d0f62971..1ff7050a0 100755 --- a/qa/rpc-tests/wallet_shieldingcoinbase.py +++ b/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: diff --git a/src/wallet/asyncrpcoperation_common.cpp b/src/wallet/asyncrpcoperation_common.cpp index f0514ab18..87f3b5243 100644 --- a/src/wallet/asyncrpcoperation_common.cpp +++ b/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) { diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 7acf6fbcc..2958b11cb 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -5031,26 +5031,7 @@ UniValue z_sendmany(const UniValue& params, bool fHelp) // Fee in Zatoshis, not currency format) std::optional 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. diff --git a/src/wallet/wallet_tx_builder.cpp b/src/wallet/wallet_tx_builder.cpp index ae8f11f2f..63bacc4e3 100644 --- a/src/wallet/wallet_tx_builder.cpp +++ b/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) { diff --git a/src/wallet/wallet_tx_builder.h b/src/wallet/wallet_tx_builder.h index 387313136..3c1e53b5d 100644 --- a/src/wallet/wallet_tx_builder.h +++ b/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 {