diff --git a/contrib/debian/examples/zcash.conf b/contrib/debian/examples/zcash.conf index c22196210..c01332d85 100644 --- a/contrib/debian/examples/zcash.conf +++ b/contrib/debian/examples/zcash.conf @@ -74,7 +74,7 @@ #rpcpassword=YourSuperGreatPasswordNumber_DO_NOT_USE_THIS_OR_YOU_WILL_GET_ROBBED_385593 # How many seconds zcash will wait for a complete RPC HTTP request. -# after the HTTP connection is established. +# after the HTTP connection is established. #rpcclienttimeout=30 # By default, only RPC connections from localhost are allowed. @@ -82,7 +82,7 @@ # either as a single IPv4/IPv6 or with a subnet specification. # NOTE: opening up the RPC port to hosts outside your local trusted network is NOT RECOMMENDED, -# because the rpcpassword is transmitted over the network unencrypted and also because anyone +# because the rpcpassword is transmitted over the network unencrypted and also because anyone # that can authenticate on the RPC port can steal your keys + take over the account running zcashd # For more information see https://github.com/zcash/zcash/issues/1497 @@ -114,17 +114,6 @@ # the fee rate is applied as though it were 1000 bytes. #paytxfee= -# If paytxfee is not set, include enough fee that transactions created by -# legacy APIs begin confirmation on average within n blocks. This is only -# used if there is sufficient mempool data to estimate the fee; if not, -# the fallback fee set by mintxfee is used. -#txconfirmtarget=2 - -# The fallback fee rate (in ZEC per 1000 bytes) used by legacy APIs when -# paytxfee has not been set and there is insufficient mempool data to -# estimate a fee according to the txconfirmtarget option. -#mintxfee=0.00001 - ## ## Miscellaneous options ## diff --git a/doc/man/zcashd.1 b/doc/man/zcashd.1 index 7e8354c4b..4df17a146 100644 --- a/doc/man/zcashd.1 +++ b/doc/man/zcashd.1 @@ -309,11 +309,6 @@ Enable the Sprout to Sapling migration .IP Set the Sapling migration address .HP -\fB\-mintxfee=\fR -.IP -Fees (in ZEC/kB) smaller than this are considered zero fee for -transaction creation (default: 0.00001) -.HP \fB\-orchardactionlimit=\fR .IP Set the maximum number of Orchard actions permitted in a transaction @@ -340,11 +335,6 @@ Send transactions as zero\-fee transactions if possible (default: 0) .IP Spend unconfirmed change when sending transactions (default: 1) .HP -\fB\-txconfirmtarget=\fR -.IP -If paytxfee is not set, include enough fee so transactions begin -confirmation on average within n blocks (default: 2) -.HP \fB\-txexpirydelta\fR .IP Set the number of blocks after which a transaction that has not been diff --git a/doc/release-notes.md b/doc/release-notes.md index c489eb355..6469503a4 100644 --- a/doc/release-notes.md +++ b/doc/release-notes.md @@ -25,8 +25,8 @@ RPC Changes while you should already be checking the async operation status, there are now more cases that may trigger failure at that stage. - The `AllowRevealedRecipients` privacy policy is now required in order to choose a - transparent change address for a transaction. This will only occur when the wallet - is unable to construct the transaction without selecting funds from the transparent + transparent change address for a transaction. This will only occur when the wallet + is unable to construct the transaction without selecting funds from the transparent pool, so the impact of this change is that for such transactions, the user must specify `AllowFullyTransparent`. - The `estimatepriority` RPC call has been removed. @@ -59,6 +59,12 @@ Removal of Priority Estimation these priority estimates. It will automatically be converted to the new format which is not readable by prior versions of the software. +Removal of obsolete config options +---------------------------------- + +- The fee changes for ZIP 317 have made the `-mintxfee` and `-txconfirmtarget` config options + obsolete. They have been removed and will now cause a warning if used. + [Deprecations](https://zcash.github.io/zcash/user/deprecation.html) -------------- diff --git a/qa/rpc-tests/prioritisetransaction.py b/qa/rpc-tests/prioritisetransaction.py index 42dc8f406..d297a5d47 100755 --- a/qa/rpc-tests/prioritisetransaction.py +++ b/qa/rpc-tests/prioritisetransaction.py @@ -12,7 +12,7 @@ from test_framework.util import ( sync_blocks, sync_mempools, ) -from test_framework.mininode import COIN +from test_framework.zip317 import conventional_fee_zats import time @@ -43,8 +43,6 @@ class PrioritiseTransactionTest (BitcoinTestFramework): print("Mining 11kb blocks...") self.nodes[0].generate(501) - base_fee = self.nodes[0].getnetworkinfo()['relayfee'] - # 11 kb blocks will only hold about 50 txs, so this will fill mempool with older txs taddr = self.nodes[1].getnewaddress() for _ in range(900): @@ -74,7 +72,7 @@ class PrioritiseTransactionTest (BitcoinTestFramework): print("in_block_template =", in_block_template) #assert_equal(in_block_template, False) - priority_success = self.nodes[0].prioritisetransaction(priority_tx_0, 0, int(3 * base_fee * COIN)) + priority_success = self.nodes[0].prioritisetransaction(priority_tx_0, 0, conventional_fee_zats(2)) assert(priority_success) # Check that prioritised transaction is not in getblocktemplate() @@ -113,7 +111,7 @@ class PrioritiseTransactionTest (BitcoinTestFramework): # Node 1 doesn't get the next block, so this *shouldn't* be mined despite being prioritised on node 1 priority_tx_1 = self.nodes[1].sendtoaddress(self.nodes[0].getnewaddress(), 0.1) - self.nodes[1].prioritisetransaction(priority_tx_1, 0, int(3 * base_fee * COIN)) + self.nodes[1].prioritisetransaction(priority_tx_1, 0, conventional_fee_zats(2)) # Mine block on node 0 blk_hash = self.nodes[0].generate(1) diff --git a/qa/rpc-tests/show_help.py b/qa/rpc-tests/show_help.py index f58ab0969..9d20af6f7 100755 --- a/qa/rpc-tests/show_help.py +++ b/qa/rpc-tests/show_help.py @@ -257,12 +257,6 @@ Wallet options: -migrationdestaddress= Set the Sapling migration address - -mintxfee= - The fallback fee rate (in ZEC per 1000 bytes) used by legacy APIs - (sendtoaddress, sendmany, and fundrawtransaction) when -paytxfee has not - been set and there is insufficient mempool data to estimate a fee - according to the -txconfirmtarget option (default: 0.00001) - -orchardactionlimit= Set the maximum number of Orchard actions permitted in a transaction (default 50) @@ -271,9 +265,8 @@ Wallet options: The preferred fee rate (in ZEC per 1000 bytes) used for transactions created by legacy APIs (sendtoaddress, sendmany, and fundrawtransaction). If the transaction is less than 1000 bytes then the - fee rate is applied as though it were 1000 bytes. See the descriptions - of -txconfirmtarget and -mintxfee options for how the fee is calculated - when this option is not set. + fee rate is applied as though it were 1000 bytes. When this option is + not set, the ZIP 317 fee calculation is used. -rescan Rescan the block chain for missing wallet transactions on startup @@ -285,13 +278,6 @@ Wallet options: -spendzeroconfchange Spend unconfirmed change when sending transactions (default: 1) - -txconfirmtarget= - If -paytxfee is not set, include enough fee that transactions created by - legacy APIs (sendtoaddress, sendmany, and fundrawtransaction) begin - confirmation on average within n blocks. This is only used if there is - sufficient mempool data to estimate the fee; if not, the fallback fee - set by -mintxfee is used. (default: 2) - -txexpirydelta Set the number of blocks after which a transaction that has not been mined will become invalid (min: 4, default: 20 (pre-Blossom) or 40 diff --git a/qa/rpc-tests/wallet_shieldingcoinbase.py b/qa/rpc-tests/wallet_shieldingcoinbase.py index 390238b46..3b8972533 100755 --- a/qa/rpc-tests/wallet_shieldingcoinbase.py +++ b/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: diff --git a/src/amount.cpp b/src/amount.cpp index 1fd13c777..948cd3143 100644 --- a/src/amount.cpp +++ b/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 diff --git a/src/main.h b/src/main.h index 3e985bf3b..39b9cf9fb 100644 --- a/src/main.h +++ b/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 */ diff --git a/src/util/moneystr.cpp b/src/util/moneystr.cpp index 87e94fd6a..dfc9ed0dd 100644 --- a/src/util/moneystr.cpp +++ b/src/util/moneystr.cpp @@ -33,6 +33,10 @@ std::string FormatMoney(const CAmount& n) return str; } +std::string DisplayMoney(const CAmount& zat) +{ + return FormatMoney(zat) + " " + CURRENCY_UNIT; +} bool ParseMoney(const string& str, CAmount& nRet) { diff --git a/src/util/moneystr.h b/src/util/moneystr.h index 2287528e1..8ebabf6e7 100644 --- a/src/util/moneystr.h +++ b/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); diff --git a/src/wallet/asyncrpcoperation_common.cpp b/src/wallet/asyncrpcoperation_common.cpp index 0bd6e6037..85fd4bc76 100644 --- a/src/wallet/asyncrpcoperation_common.cpp +++ b/src/wallet/asyncrpcoperation_common.cpp @@ -179,6 +179,15 @@ void ThrowInputSelectionError( WEIGHT_RATIO_CAP, FormatMoney(err.conventionalFee))); }, + [](const MaxFeeError& err) { + throw JSONRPCError( + RPC_INVALID_PARAMETER, + strprintf( + "Fee (%s) is greater than the maximum fee allowed by this instance (%s). Run " + "zcashd with `-maxtxfee` to adjust this limit.", + DisplayMoney(err.fixedFee), + DisplayMoney(maxTxFee))); + }, [](const ExcessOrchardActionsError& err) { std::string side; switch (err.side) { diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 0c81b48ee..89d5f3f74 100644 --- a/src/wallet/wallet.cpp +++ b/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" @@ -47,7 +48,6 @@ using namespace libzcash; CWallet* pwalletMain = NULL; /** Transaction fee set by the user */ CFeeRate payTxFee(DEFAULT_TRANSACTION_FEE); -unsigned int nTxConfirmTarget = DEFAULT_TX_CONFIRM_TARGET; bool bSpendZeroConfChange = DEFAULT_SPEND_ZEROCONF_CHANGE; bool fPayAtLeastCustomFee = true; unsigned int nAnchorConfirmations = DEFAULT_ANCHOR_CONFIRMATIONS; @@ -55,13 +55,6 @@ unsigned int nOrchardActionLimit = DEFAULT_ORCHARD_ACTION_LIMIT; const char * DEFAULT_WALLET_DAT = "wallet.dat"; -/** - * -mintxfee: the fallback fee rate (in ZEC per 1000 bytes) used by legacy APIs - * when -paytxfee has not been set and there is insufficient mempool data to - * estimate a fee according to the -txconfirmtarget option. - */ -CFeeRate CWallet::minTxFee = CFeeRate(DEFAULT_TRANSACTION_MINFEE); - std::set CWallet::DefaultReceiverTypes(int nHeight) { // For now, just ignore the height information because the default // is always the same. @@ -5710,6 +5703,15 @@ bool CWallet::CreateTransaction(const vector& vecSend, CWalletTx& wt unsigned int nBytes = ::GetSerializeSize(txNew, SER_NETWORK, PROTOCOL_VERSION); + // Limit size + if (nBytes >= max_tx_size) + { + strFailReason = _("Transaction too large"); + return false; + } + + CAmount nFeeNeeded = GetMinimumFee(txNew, nBytes); + // Remove scriptSigs if we used dummy signatures for fee calculation if (!sign) { for (CTxIn& vin : txNew.vin) @@ -5719,15 +5721,6 @@ bool CWallet::CreateTransaction(const vector& vecSend, CWalletTx& wt // Embed the constructed transaction data in wtxNew. *static_cast(&wtxNew) = CTransaction(txNew); - // Limit size - if (nBytes >= max_tx_size) - { - strFailReason = _("Transaction too large"); - return false; - } - - CAmount nFeeNeeded = GetMinimumFee(nBytes, nTxConfirmTarget, mempool); - // If we made it here and we aren't even able to meet the relay fee on the next pass, give up // because we must be at the maximum allowed fee. if (nFeeNeeded < ::minRelayTxFee.GetFeeForRelay(nBytes)) @@ -5800,39 +5793,22 @@ bool CWallet::CommitTransaction(CWalletTx& wtxNew, std::optional 0 && nFeeNeeded < payTxFee.GetFeePerK()) - nFeeNeeded = payTxFee.GetFeePerK(); - // User didn't set: use -txconfirmtarget to estimate... + CAmount nFeeNeeded = payTxFee.GetFee(std::max((unsigned int)1000, nTxBytes)); + // User didn't set: use conventional fee if (nFeeNeeded == 0) - nFeeNeeded = pool.estimateFee(nConfirmTarget).GetFee(nTxBytes); - // ... unless we don't have enough mempool data, in which case fall - // back to the required fee - if (nFeeNeeded == 0) - nFeeNeeded = GetRequiredFee(nTxBytes); - // prevent user from paying a non-sense fee (like 1 satoshi): 0 < fee < minRelayFee - if (nFeeNeeded < ::minRelayTxFee.GetFeeForRelay(nTxBytes)) - nFeeNeeded = ::minRelayTxFee.GetFeeForRelay(nTxBytes); - // But always obey the maximum - if (nFeeNeeded > maxTxFee) - nFeeNeeded = maxTxFee; - return nFeeNeeded; + nFeeNeeded = tx.GetConventionalFee(); + return ConstrainFee(nFeeNeeded, nTxBytes); } - - DBErrors CWallet::LoadWallet(bool& fFirstRunRet) { if (!fFileBacked) @@ -6506,21 +6482,14 @@ std::string CWallet::GetWalletHelpString(bool showDebug) strUsage += HelpMessageOpt("-keypool=", strprintf(_("Set key pool size to (default: %u)"), DEFAULT_KEYPOOL_SIZE)); strUsage += HelpMessageOpt("-migration", _("Enable the Sprout to Sapling migration")); strUsage += HelpMessageOpt("-migrationdestaddress=", _("Set the Sapling migration address")); - strUsage += HelpMessageOpt("-mintxfee=", strprintf(_("The fallback fee rate (in %s per 1000 bytes) used by legacy APIs (sendtoaddress, sendmany, and fundrawtransaction) when -paytxfee " - "has not been set and there is insufficient mempool data to estimate a fee according to the -txconfirmtarget option (default: %s)"), - CURRENCY_UNIT, FormatMoney(DEFAULT_TRANSACTION_MINFEE))); strUsage += HelpMessageOpt("-orchardactionlimit=", strprintf(_("Set the maximum number of Orchard actions permitted in a transaction (default %u)"), DEFAULT_ORCHARD_ACTION_LIMIT)); strUsage += HelpMessageOpt("-paytxfee=", strprintf(_("The preferred fee rate (in %s per 1000 bytes) used for transactions created by legacy APIs (sendtoaddress, sendmany, and fundrawtransaction). " - "If the transaction is less than 1000 bytes then the fee rate is applied as though it were 1000 bytes. See the descriptions of -txconfirmtarget " - "and -mintxfee options for how the fee is calculated when this option is not set."), + "If the transaction is less than 1000 bytes then the fee rate is applied as though it were 1000 bytes. When this option is not set, " + "the ZIP 317 fee calculation is used."), CURRENCY_UNIT)); strUsage += HelpMessageOpt("-rescan", _("Rescan the block chain for missing wallet transactions on startup")); strUsage += HelpMessageOpt("-salvagewallet", _("Attempt to recover private keys from a corrupt wallet on startup (implies -rescan)")); strUsage += HelpMessageOpt("-spendzeroconfchange", strprintf(_("Spend unconfirmed change when sending transactions (default: %u)"), DEFAULT_SPEND_ZEROCONF_CHANGE)); - strUsage += HelpMessageOpt("-txconfirmtarget=", strprintf(_("If -paytxfee is not set, include enough fee that transactions created by legacy APIs (sendtoaddress, sendmany, and fundrawtransaction) " - "begin confirmation on average within n blocks. This is only used if there is sufficient mempool data to estimate the fee; if not, the " - "fallback fee set by -mintxfee is used. (default: %u)"), - DEFAULT_TX_CONFIRM_TARGET)); strUsage += HelpMessageOpt("-txexpirydelta", strprintf(_("Set the number of blocks after which a transaction that has not been mined will become invalid (min: %u, default: %u (pre-Blossom) or %u (post-Blossom))"), TX_EXPIRING_SOON_THRESHOLD + 1, DEFAULT_PRE_BLOSSOM_TX_EXPIRY_DELTA, DEFAULT_POST_BLOSSOM_TX_EXPIRY_DELTA)); strUsage += HelpMessageOpt("-upgradewallet", _("Upgrade wallet to latest format on startup")); strUsage += HelpMessageOpt("-wallet=", _("Specify wallet file absolute path or a path relative to the data directory") + " " + strprintf(_("(default: %s)"), DEFAULT_WALLET_DAT)); @@ -6725,11 +6694,7 @@ bool CWallet::ParameterInteraction(const CChainParams& params) { if (mapArgs.count("-mintxfee")) { - CAmount n = 0; - if (ParseMoney(mapArgs["-mintxfee"], n) && n > 0) - CWallet::minTxFee = CFeeRate(n); - else - return UIError(AmountErrMsg("mintxfee", mapArgs["-mintxfee"])); + UIWarning(_("The argument -mintxfee is no longer supported.")); } if (mapArgs.count("-paytxfee")) { @@ -6753,13 +6718,15 @@ 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=: '%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=: '%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); + if (mapArgs.count("-txconfirmtarget")) { + UIWarning(_("The argument -txconfirmtarget is no longer supported.")); + } if (mapArgs.count("-txexpirydelta")) { int64_t expiryDelta = atoi64(mapArgs["-txexpirydelta"]); uint32_t minExpiryDelta = TX_EXPIRING_SOON_THRESHOLD + 1; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 236ce0419..d371eeca6 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -52,7 +52,6 @@ extern CWallet* pwalletMain; * Settings */ extern CFeeRate payTxFee; -extern unsigned int nTxConfirmTarget; extern bool bSpendZeroConfChange; extern bool fPayAtLeastCustomFee; extern unsigned int nAnchorConfirmations; @@ -63,14 +62,10 @@ extern unsigned int nOrchardActionLimit; static const unsigned int DEFAULT_KEYPOOL_SIZE = 100; //! -paytxfee default static const CAmount DEFAULT_TRANSACTION_FEE = 0; -//! -mintxfee default -static const CAmount DEFAULT_TRANSACTION_MINFEE = 1000; //! minimum change amount static const CAmount MIN_CHANGE = CENT; //! Default for -spendzeroconfchange static const bool DEFAULT_SPEND_ZEROCONF_CHANGE = true; -//! -txconfirmtarget default -static const unsigned int DEFAULT_TX_CONFIRM_TARGET = 2; static const bool DEFAULT_WALLETBROADCAST = true; //! Size of witness cache // Should be large enough that we can expect not to reorg beyond our cache @@ -1949,17 +1944,17 @@ public: bool CommitTransaction(CWalletTx& wtxNew, std::optional> reservekey, CValidationState& state); - static CFeeRate minTxFee; + /** Adjust the requested fee by bounding it below to the minimum relay fee required + * for a transaction of the given size and bounding it above to the maximum fee + * configured using the `-maxtxfee` configuration option. + */ + static CAmount ConstrainFee(CAmount requestedFee, unsigned int nTxBytes); + /** * Estimate the minimum fee considering user set parameters * and the required fee. */ - static CAmount GetMinimumFee(unsigned int nTxBytes, unsigned int nConfirmTarget, const CTxMemPool& pool); - /** - * Return the minimum required fee taking into account the - * floating relay fee rate and user set minimum transaction fee rate. - */ - static CAmount GetRequiredFee(unsigned int nTxBytes); + static CAmount GetMinimumFee(const CTransaction& tx, unsigned int nTxBytes); /** * The set of default receiver types used when the wallet generates diff --git a/src/wallet/wallet_tx_builder.cpp b/src/wallet/wallet_tx_builder.cpp index cb98426a4..fb992f663 100644 --- a/src/wallet/wallet_tx_builder.cpp +++ b/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" @@ -149,7 +150,9 @@ WalletTxBuilder::PrepareTransaction( std::optional fee, uint32_t anchorConfirmations) const { - assert(fee < MAX_MONEY); + if (fee.has_value() && maxTxFee < fee.value()) { + return tl::make_unexpected(MaxFeeError(fee.value())); + } int anchorHeight = GetAnchorHeight(chain, anchorConfirmations); bool afterNU5 = params.GetConsensus().NetworkUpgradeActive(anchorHeight, Consensus::UPGRADE_NU5); @@ -274,6 +277,19 @@ CalcZIP317Fee( return CalculateConventionalFee(logicalActionCount); } +CAmount GetConstrainedFee( + const std::optional& inputs, + const std::vector& payments, + const std::optional& changeAddr) +{ + // We know that minRelayFee <= MINIMUM_FEE <= conventional_fee, so we can use an arbitrary + // transaction size when constraining the fee, because we are guaranteed to already satisfy the + // lower bound. + constexpr unsigned int DUMMY_TX_SIZE = 1; + + return CWallet::ConstrainFee(CalcZIP317Fee(inputs, payments, changeAddr), DUMMY_TX_SIZE); +} + InvalidFundsError ReportInvalidFunds( const SpendableInputs& spendable, bool hasPhantomChange, @@ -350,7 +366,7 @@ WalletTxBuilder::IterateLimit( SpendableInputs spendableMut; auto previousFee = MINIMUM_FEE; - auto updatedFee = CalcZIP317Fee(std::nullopt, resolved.GetResolvedPayments(), std::nullopt); + auto updatedFee = GetConstrainedFee(std::nullopt, resolved.GetResolvedPayments(), std::nullopt); // This is used to increase the target amount just enough (generally by 0 or 1) to force // selection of additional notes. CAmount bumpTargetAmount{0}; @@ -393,7 +409,7 @@ WalletTxBuilder::IterateLimit( } } previousFee = updatedFee; - updatedFee = CalcZIP317Fee( + updatedFee = GetConstrainedFee( spendableMut, resolved.GetResolvedPayments(), changeAmount > 0 ? changeAddr : std::nullopt); @@ -937,7 +953,23 @@ TransactionBuilderResult TransactionEffects::ApproveAndBuild( } // Build the transaction - return builder.Build(); + auto result = builder.Build(); + + if (result.IsTx()) { + auto minRelayFee = + ::minRelayTxFee.GetFeeForRelay( + ::GetSerializeSize(result.GetTxOrThrow(), SER_NETWORK, PROTOCOL_VERSION)); + // This should only be possible if a user has provided an explicit fee. + if (fee < minRelayFee) { + return TransactionBuilderResult( + strprintf( + "Fee (%s) is below the minimum relay fee for this transaction (%s)", + DisplayMoney(fee), + DisplayMoney(minRelayFee))); + } + } + + return result; } // TODO: Lock Orchard notes (#6226) diff --git a/src/wallet/wallet_tx_builder.h b/src/wallet/wallet_tx_builder.h index d44b665bc..4a629ca57 100644 --- a/src/wallet/wallet_tx_builder.h +++ b/src/wallet/wallet_tx_builder.h @@ -305,6 +305,15 @@ public: conventionalFee(conventionalFee), fixedFee(fixedFee) { } }; +/// Error when a fee is higher than this instance allows. +class MaxFeeError { +public: + CAmount fixedFee; + + MaxFeeError(CAmount fixedFee): + fixedFee(fixedFee) { } +}; + enum ActionSide { Input, Output, @@ -326,6 +335,7 @@ typedef std::variant< InvalidFundsError, ChangeNotAllowedError, AbsurdFeeError, + MaxFeeError, ExcessOrchardActionsError> InputSelectionError; class InputSelection {