From 3db688201736e36bf39cb7569e618005ea6c31d0 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Mon, 26 Feb 2024 20:45:40 +0100 Subject: [PATCH 1/8] scripted-diff: rename `m_default_max_tx_fee` to `m_max_tx_fee` -BEGIN VERIFY SCRIPT- git grep -l "m_default_max_tx_fee" src | xargs sed -i "s/m_default_max_tx_fee/m_max_tx_fee/g" -END VERIFY SCRIPT- - The value is not always the default but can be configured during startup so `m_max_tx_fee` is the right name not `m_default_max_tx_fee`. --- src/qt/walletmodel.cpp | 2 +- src/wallet/feebumper.cpp | 2 +- src/wallet/interfaces.cpp | 2 +- src/wallet/rpc/spend.cpp | 4 ++-- src/wallet/spend.cpp | 2 +- src/wallet/wallet.cpp | 4 ++-- src/wallet/wallet.h | 2 +- 7 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp index 0a01c0a45b1..96f3de52040 100644 --- a/src/qt/walletmodel.cpp +++ b/src/qt/walletmodel.cpp @@ -228,7 +228,7 @@ WalletModel::SendCoinsReturn WalletModel::prepareTransaction(WalletModelTransact // Reject absurdly high fee. (This can never happen because the // wallet never creates transactions with fee greater than - // m_default_max_tx_fee. This merely a belt-and-suspenders check). + // m_max_tx_fee. This merely a belt-and-suspenders check). if (nFeeRequired > m_wallet->getDefaultMaxTxFee()) { return AbsurdFee; } diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index b875461c9f2..cd9159e48c5 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -112,7 +112,7 @@ static feebumper::Result CheckFeeRate(const CWallet& wallet, const CMutableTrans } // Check that in all cases the new fee doesn't violate maxTxFee - const CAmount max_tx_fee = wallet.m_default_max_tx_fee; + const CAmount max_tx_fee = wallet.m_max_tx_fee; if (new_total_fee > max_tx_fee) { errors.push_back(Untranslated(strprintf("Specified or calculated fee %s is too high (cannot be higher than -maxtxfee %s)", FormatMoney(new_total_fee), FormatMoney(max_tx_fee)))); diff --git a/src/wallet/interfaces.cpp b/src/wallet/interfaces.cpp index 21e8a0b3bd2..201f524e250 100644 --- a/src/wallet/interfaces.cpp +++ b/src/wallet/interfaces.cpp @@ -521,7 +521,7 @@ public: return spk_man != nullptr; } OutputType getDefaultAddressType() override { return m_wallet->m_default_address_type; } - CAmount getDefaultMaxTxFee() override { return m_wallet->m_default_max_tx_fee; } + CAmount getDefaultMaxTxFee() override { return m_wallet->m_max_tx_fee; } void remove() override { RemoveWallet(m_context, m_wallet, /*load_on_start=*/false); diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index bea9b2eec18..9499e1fbab5 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -439,7 +439,7 @@ RPCHelpMan settxfee() CAmount nAmount = AmountFromValue(request.params[0]); CFeeRate tx_fee_rate(nAmount, 1000); - CFeeRate max_tx_fee_rate(pwallet->m_default_max_tx_fee, 1000); + CFeeRate max_tx_fee_rate(pwallet->m_max_tx_fee, 1000); if (tx_fee_rate == CFeeRate(0)) { // automatic selection } else if (tx_fee_rate < pwallet->chain().relayMinFee()) { @@ -1504,7 +1504,7 @@ RPCHelpMan sendall() const std::optional total_bump_fees{pwallet->chain().calculateCombinedBumpFee(outpoints_spent, fee_rate)}; CAmount effective_value = total_input_value - fee_from_size - total_bump_fees.value_or(0); - if (fee_from_size > pwallet->m_default_max_tx_fee) { + if (fee_from_size > pwallet->m_max_tx_fee) { throw JSONRPCError(RPC_WALLET_ERROR, TransactionErrorString(TransactionError::MAX_FEE_EXCEEDED).original); } diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index f45db1c16f3..1bb77e61131 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -1339,7 +1339,7 @@ static util::Result CreateTransactionInternal( return util::Error{_("Transaction too large")}; } - if (current_fee > wallet.m_default_max_tx_fee) { + if (current_fee > wallet.m_max_tx_fee) { return util::Error{TransactionErrorString(TransactionError::MAX_FEE_EXCEEDED)}; } diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index ea59f1a9d57..19ef038c7a6 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2053,7 +2053,7 @@ bool CWallet::SubmitTxMemoryPoolAndRelay(CWalletTx& wtx, std::string& err_string // If broadcast fails for any reason, trying to set wtx.m_state here would be incorrect. // If transaction was previously in the mempool, it should be updated when // TransactionRemovedFromMempool fires. - bool ret = chain().broadcastTransaction(wtx.tx, m_default_max_tx_fee, relay, err_string); + bool ret = chain().broadcastTransaction(wtx.tx, m_max_tx_fee, relay, err_string); if (ret) wtx.m_state = TxStateInMempool{}; return ret; } @@ -3200,7 +3200,7 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri return nullptr; } - walletInstance->m_default_max_tx_fee = max_fee.value(); + walletInstance->m_max_tx_fee = max_fee.value(); } if (args.IsArgSet("-consolidatefeerate")) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index f8b914dd8ff..70d0587ef02 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -728,7 +728,7 @@ public: */ std::optional m_default_change_type{}; /** Absolute maximum transaction fee (in satoshis) used by default for the wallet */ - CAmount m_default_max_tx_fee{DEFAULT_TRANSACTION_MAXFEE}; + CAmount m_max_tx_fee{DEFAULT_TRANSACTION_MAXFEE}; /** Number of pre-generated keys/scripts by each spkm (part of the look-ahead process, used to detect payments) */ int64_t m_keypool_size{DEFAULT_KEYPOOL_SIZE}; From d3600accf966675c1b3592ab3809ab02308ffd1c Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Sun, 17 Mar 2024 12:45:59 +0100 Subject: [PATCH 2/8] [wallet]: update `max_fee` to `max_tx_fee` - Also update `-maxapsfee` option from `max_fee` to `max_aps_fee`. This fixes the ambiguity in the variable name. --- src/wallet/wallet.cpp | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 19ef038c7a6..f14bc6e21c0 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3124,17 +3124,17 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri } if (args.IsArgSet("-maxapsfee")) { - const std::string max_aps_fee{args.GetArg("-maxapsfee", "")}; - if (max_aps_fee == "-1") { + const std::string max_aps_fee_str{args.GetArg("-maxapsfee", "")}; + if (max_aps_fee_str == "-1") { walletInstance->m_max_aps_fee = -1; - } else if (std::optional max_fee = ParseMoney(max_aps_fee)) { - if (max_fee.value() > HIGH_APS_FEE) { + } else if (std::optional max_aps_fee = ParseMoney(max_aps_fee_str)) { + if (max_aps_fee.value() > HIGH_APS_FEE) { warnings.push_back(AmountHighWarn("-maxapsfee") + Untranslated(" ") + _("This is the maximum transaction fee you pay (in addition to the normal fee) to prioritize partial spend avoidance over regular coin selection.")); } - walletInstance->m_max_aps_fee = max_fee.value(); + walletInstance->m_max_aps_fee = max_aps_fee.value(); } else { - error = AmountErrMsg("maxapsfee", max_aps_fee); + error = AmountErrMsg("maxapsfee", max_aps_fee_str); return nullptr; } } @@ -3186,21 +3186,21 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri } if (args.IsArgSet("-maxtxfee")) { - std::optional max_fee = ParseMoney(args.GetArg("-maxtxfee", "")); - if (!max_fee) { + std::optional max_tx_fee = ParseMoney(args.GetArg("-maxtxfee", "")); + if (!max_tx_fee) { error = AmountErrMsg("maxtxfee", args.GetArg("-maxtxfee", "")); return nullptr; - } else if (max_fee.value() > HIGH_MAX_TX_FEE) { + } else if (max_tx_fee.value() > HIGH_MAX_TX_FEE) { warnings.push_back(strprintf(_("%s is set very high! Fees this large could be paid on a single transaction."), "-maxtxfee")); } - if (chain && CFeeRate{max_fee.value(), 1000} < chain->relayMinFee()) { + if (chain && CFeeRate{max_tx_fee.value(), 1000} < chain->relayMinFee()) { error = strprintf(_("Invalid amount for %s=: '%s' (must be at least the minrelay fee of %s to prevent stuck transactions)"), "-maxtxfee", args.GetArg("-maxtxfee", ""), chain->relayMinFee().ToString()); return nullptr; } - walletInstance->m_max_tx_fee = max_fee.value(); + walletInstance->m_max_tx_fee = max_tx_fee.value(); } if (args.IsArgSet("-consolidatefeerate")) { From 832d468c345e7547dace7e473f664a5144791886 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Thu, 23 Jan 2025 12:11:35 -0500 Subject: [PATCH 3/8] [wallet]: add `maxfeerate` wallet startup option - The commits adds a new wallet startup option `-maxfeerate` - The value will be used as the upper limit of wallet transaction fee rate. - The commit updates all instances where `-maxtxfee` value is utilized to check wallet transactions fee rate upper limit to now use `-maxfeerate` value. - `settxfee` RPC now only check if the fee rate user set is less `minrelaytxfee`, less than wallet min fee, or whether it exceed `maxfeerate`. We should not check whether `settxfee` RPC respects `maxtxfee` limit in in the functional test because its not enforced. We now only check if `settxfee` respects `maxfeerate` limit. --- src/dummywallet.cpp | 1 + src/wallet/init.cpp | 1 + src/wallet/rpc/spend.cpp | 5 ++--- src/wallet/wallet.cpp | 15 ++++++++++++--- src/wallet/wallet.h | 4 ++++ test/functional/wallet_bumpfee.py | 6 +++--- 6 files changed, 23 insertions(+), 9 deletions(-) diff --git a/src/dummywallet.cpp b/src/dummywallet.cpp index 42282c32d1b..10fdf3f706e 100644 --- a/src/dummywallet.cpp +++ b/src/dummywallet.cpp @@ -37,6 +37,7 @@ void DummyWalletInit::AddWalletOptions(ArgsManager& argsman) const "-keypool=", "-maxapsfee=", "-maxtxfee=", + "-maxfeerate=", "-mintxfee=", "-paytxfee=", "-signer=", diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp index cfd09a2e101..9b15cfe1e6a 100644 --- a/src/wallet/init.cpp +++ b/src/wallet/init.cpp @@ -64,6 +64,7 @@ void WalletInit::AddWalletOptions(ArgsManager& argsman) const argsman.AddArg("-maxapsfee=", strprintf("Spend up to this amount in additional (absolute) fees (in %s) if it allows the use of partial spend avoidance (default: %s)", CURRENCY_UNIT, FormatMoney(DEFAULT_MAX_AVOIDPARTIALSPEND_FEE)), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET); argsman.AddArg("-maxtxfee=", strprintf("Maximum total fees (in %s) to use in a single wallet transaction; setting this too low may abort large transactions (default: %s)", CURRENCY_UNIT, FormatMoney(DEFAULT_TRANSACTION_MAXFEE)), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); + argsman.AddArg("-maxfeerate=", strprintf("Maximum fee rate (in %s/kvB) for a wallet transactions (default: %s %s/kvB)", CURRENCY_UNIT, FormatMoney(DEFAULT_MAX_TRANSACTION_FEERATE.GetFeePerK()), CURRENCY_UNIT), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET); argsman.AddArg("-mintxfee=", strprintf("Fee rates (in %s/kvB) smaller than this are considered zero fee for transaction creation (default: %s)", CURRENCY_UNIT, FormatMoney(DEFAULT_TRANSACTION_MINFEE)), ArgsManager::ALLOW_ANY, OptionsCategory::WALLET); argsman.AddArg("-paytxfee=", strprintf("Fee rate (in %s/kvB) to add to transactions you send (default: %s)", diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index 9499e1fbab5..669000d0094 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -439,15 +439,14 @@ RPCHelpMan settxfee() CAmount nAmount = AmountFromValue(request.params[0]); CFeeRate tx_fee_rate(nAmount, 1000); - CFeeRate max_tx_fee_rate(pwallet->m_max_tx_fee, 1000); if (tx_fee_rate == CFeeRate(0)) { // automatic selection } else if (tx_fee_rate < pwallet->chain().relayMinFee()) { throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("txfee cannot be less than min relay tx fee (%s)", pwallet->chain().relayMinFee().ToString())); } else if (tx_fee_rate < pwallet->m_min_fee) { throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("txfee cannot be less than wallet min fee (%s)", pwallet->m_min_fee.ToString())); - } else if (tx_fee_rate > max_tx_fee_rate) { - throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("txfee cannot be more than wallet max tx fee (%s)", max_tx_fee_rate.ToString())); + } else if (tx_fee_rate > pwallet->m_max_tx_fee_rate) { + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("fee rate cannot be more than wallet max tx fee rate (%s)", pwallet->m_max_tx_fee_rate.ToString())); } pwallet->m_pay_tx_fee = tx_fee_rate; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index f14bc6e21c0..422e0278965 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3194,13 +3194,22 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri warnings.push_back(strprintf(_("%s is set very high! Fees this large could be paid on a single transaction."), "-maxtxfee")); } - if (chain && CFeeRate{max_tx_fee.value(), 1000} < chain->relayMinFee()) { + walletInstance->m_max_tx_fee = max_tx_fee.value(); + } + + if (args.IsArgSet("-maxfeerate")) { + std::optional max_tx_fee_rate = ParseMoney(args.GetArg("-maxfeerate", "")); + if (!max_tx_fee_rate) { + error = AmountErrMsg("maxfeerate", args.GetArg("-maxfeerate", "")); + return nullptr; + } + if (chain && CFeeRate(max_tx_fee_rate.value()) < chain->relayMinFee()) { error = strprintf(_("Invalid amount for %s=: '%s' (must be at least the minrelay fee of %s to prevent stuck transactions)"), - "-maxtxfee", args.GetArg("-maxtxfee", ""), chain->relayMinFee().ToString()); + "-maxfeerate", args.GetArg("-maxfeerate", ""), chain->relayMinFee().ToString()); return nullptr; } - walletInstance->m_max_tx_fee = max_tx_fee.value(); + walletInstance->m_max_tx_fee_rate = CFeeRate(max_tx_fee_rate.value()); } if (args.IsArgSet("-consolidatefeerate")) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 70d0587ef02..42fb7c22021 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -135,6 +135,8 @@ static const bool DEFAULT_DISABLE_WALLET = false; static const bool DEFAULT_WALLETCROSSCHAIN = false; //! -maxtxfee default constexpr CAmount DEFAULT_TRANSACTION_MAXFEE{COIN / 10}; +//! maxfeerate default +const CFeeRate DEFAULT_MAX_TRANSACTION_FEERATE(CFeeRate(COIN / 10)); //! Discourage users to set fees higher than this amount (in satoshis) per kB constexpr CAmount HIGH_TX_FEE_PER_KB{COIN / 100}; //! -maxtxfee will warn if called with a higher fee than this amount (in satoshis) @@ -730,6 +732,8 @@ public: /** Absolute maximum transaction fee (in satoshis) used by default for the wallet */ CAmount m_max_tx_fee{DEFAULT_TRANSACTION_MAXFEE}; + /** Maximum transaction fee rate used for the wallet */ + CFeeRate m_max_tx_fee_rate{DEFAULT_MAX_TRANSACTION_FEERATE}; /** Number of pre-generated keys/scripts by each spkm (part of the look-ahead process, used to detect payments) */ int64_t m_keypool_size{DEFAULT_KEYPOOL_SIZE}; diff --git a/test/functional/wallet_bumpfee.py b/test/functional/wallet_bumpfee.py index 061e9f2caa1..fb383cf3753 100755 --- a/test/functional/wallet_bumpfee.py +++ b/test/functional/wallet_bumpfee.py @@ -547,9 +547,9 @@ def test_settxfee(self, rbf_node, dest_address): assert_greater_than(Decimal("0.00001000"), abs(requested_feerate - actual_feerate)) rbf_node.settxfee(Decimal("0.00000000")) # unset paytxfee - # check that settxfee respects -maxtxfee - self.restart_node(1, ['-maxtxfee=0.000025'] + self.extra_args[1]) - assert_raises_rpc_error(-8, "txfee cannot be more than wallet max tx fee", rbf_node.settxfee, Decimal('0.00003')) + # check that settxfee respects -maxfeerate + self.restart_node(1, ['-maxfeerate=0.000025'] + self.extra_args[1]) + assert_raises_rpc_error(-8, "fee rate cannot be more than wallet max tx fee rate", rbf_node.settxfee, Decimal('0.00003')) self.restart_node(1, self.extra_args[1]) rbf_node.walletpassphrase(WALLET_PASSPHRASE, WALLET_PASSPHRASE_TIMEOUT) self.connect_nodes(1, 0) From 7182ec44e0d453e539d11d9a974632ea8e740438 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Thu, 23 Jan 2025 12:40:51 -0500 Subject: [PATCH 4/8] [node]: update `BroadcastTransaction` to check fee rate limit - `BroadcastTransaction` now accepts a fee rate limit as an additional input parameter. It ensures that the transaction with fee rate exceeding the specified limit are not added to mempool and not broadcasted to peers. - This will allow differentiating between the error messages when either `-maxtxfee` or `maxfeerate` is hit. - `TestSimpleSpend` creates transaction with base fee of `DEFAULT_TRANSACTION_MAXFEE`, since we now also check fee rate limit in broadcastTransaction, creating transaction with the base fee of `DEFAULT_TRANSACTION_MAXFEE` will prevent the transaction from being broadcasted because its fee rate will be above `DEFAULT_MAX_TRANSACTION_FEERATE`, hence update the maximum during broadcast to be the expected fee rate of the transaction. --- src/interfaces/chain.h | 1 + src/interfaces/node.h | 2 +- src/node/interfaces.cpp | 13 +++++++------ src/node/transaction.cpp | 9 ++++++--- src/node/transaction.h | 5 +++-- src/qt/psbtoperationsdialog.cpp | 3 ++- src/rpc/mempool.cpp | 6 ++---- src/wallet/test/wallet_tests.cpp | 7 +++++-- src/wallet/wallet.cpp | 2 +- 9 files changed, 28 insertions(+), 20 deletions(-) diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index 4e858d1f898..981d1260285 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -217,6 +217,7 @@ public: //! Return false if the transaction could not be added due to the fee or for another reason. virtual bool broadcastTransaction(const CTransactionRef& tx, const CAmount& max_tx_fee, + const CFeeRate& max_tx_fee_rate, bool relay, std::string& err_string) = 0; diff --git a/src/interfaces/node.h b/src/interfaces/node.h index aebb4386511..3c7f2e1e4d7 100644 --- a/src/interfaces/node.h +++ b/src/interfaces/node.h @@ -215,7 +215,7 @@ public: virtual std::optional getUnspentOutput(const COutPoint& output) = 0; //! Broadcast transaction. - virtual node::TransactionError broadcastTransaction(CTransactionRef tx, CAmount max_tx_fee, std::string& err_string) = 0; + virtual node::TransactionError broadcastTransaction(CTransactionRef tx, CAmount max_tx_fee, CFeeRate max_tx_fee_rate, std::string& err_string) = 0; //! Get wallet loader. virtual WalletLoader& walletLoader() = 0; diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index fe88a6dad11..b245ec2f601 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -360,9 +360,9 @@ public: LOCK(::cs_main); return chainman().ActiveChainstate().CoinsTip().GetCoin(output); } - TransactionError broadcastTransaction(CTransactionRef tx, CAmount max_tx_fee, std::string& err_string) override + TransactionError broadcastTransaction(CTransactionRef tx, CAmount max_tx_fee, CFeeRate max_tx_fee_rate, std::string& err_string) override { - return BroadcastTransaction(*m_context, std::move(tx), err_string, max_tx_fee, /*relay=*/ true, /*wait_callback=*/ false); + return BroadcastTransaction(*m_context, std::move(tx), err_string, max_tx_fee, max_tx_fee_rate, /*relay=*/true, /*wait_callback=*/false); } WalletLoader& walletLoader() override { @@ -682,11 +682,12 @@ public: return entry->GetCountWithDescendants() > 1; } bool broadcastTransaction(const CTransactionRef& tx, - const CAmount& max_tx_fee, - bool relay, - std::string& err_string) override + const CAmount& max_tx_fee, + const CFeeRate& max_tx_fee_rate, + bool relay, + std::string& err_string) override { - const TransactionError err = BroadcastTransaction(m_node, tx, err_string, max_tx_fee, relay, /*wait_callback=*/false); + const TransactionError err = BroadcastTransaction(m_node, tx, err_string, max_tx_fee, max_tx_fee_rate, relay, /*wait_callback=*/false); // Chain clients only care about failures to accept the tx to the mempool. Disregard non-mempool related failures. // Note: this will need to be updated if BroadcastTransactions() is updated to return other non-mempool failures // that Chain clients do not need to know about. diff --git a/src/node/transaction.cpp b/src/node/transaction.cpp index 666597391e3..95db5298eec 100644 --- a/src/node/transaction.cpp +++ b/src/node/transaction.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -31,7 +32,7 @@ static TransactionError HandleATMPError(const TxValidationState& state, std::str } } -TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef tx, std::string& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback) +TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef tx, std::string& err_string, const CAmount& max_tx_fee, const CFeeRate& max_tx_fee_rate, bool relay, bool wait_callback) { // BroadcastTransaction can be called by RPC or by the wallet. // chainman, mempool and peerman are initialized before the RPC server and wallet are started @@ -69,13 +70,15 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t wtxid = mempool_tx->GetWitnessHash(); } else { // Transaction is not already in the mempool. - if (max_tx_fee > 0) { + if (max_tx_fee > 0 || max_tx_fee_rate > CFeeRate(0)) { // First, call ATMP with test_accept and check the fee. If ATMP // fails here, return error immediately. const MempoolAcceptResult result = node.chainman->ProcessTransaction(tx, /*test_accept=*/ true); if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) { return HandleATMPError(result.m_state, err_string); - } else if (result.m_base_fees.value() > max_tx_fee) { + } else if (max_tx_fee > 0 && result.m_base_fees.value() > max_tx_fee) { + return TransactionError::MAX_FEE_EXCEEDED; + } else if (max_tx_fee_rate > CFeeRate(0) && CFeeRate(result.m_base_fees.value(), result.m_vsize.value()) > max_tx_fee_rate) { return TransactionError::MAX_FEE_EXCEEDED; } } diff --git a/src/node/transaction.h b/src/node/transaction.h index 5f524f4e28e..e91ec845c35 100644 --- a/src/node/transaction.h +++ b/src/node/transaction.h @@ -44,12 +44,13 @@ static const CAmount DEFAULT_MAX_BURN_AMOUNT{0}; * @param[in] node reference to node context * @param[in] tx the transaction to broadcast * @param[out] err_string reference to std::string to fill with error string if available - * @param[in] max_tx_fee reject txs with fees higher than this (if 0, accept any fee) + * @param[in] max_tx_fee reject txs with fees higher than this (if 0, the fee is not checked) + * @param[in] max_tx_fee_rate reject txs with fee rate higher than this (if CFeeRate(0), the fee rate is not checked) * @param[in] relay flag if both mempool insertion and p2p relay are requested * @param[in] wait_callback wait until callbacks have been processed to avoid stale result due to a sequentially RPC. * return error */ -[[nodiscard]] TransactionError BroadcastTransaction(NodeContext& node, CTransactionRef tx, std::string& err_string, const CAmount& max_tx_fee, bool relay, bool wait_callback); +[[nodiscard]] TransactionError BroadcastTransaction(NodeContext& node, CTransactionRef tx, std::string& err_string, const CAmount& max_tx_fee, const CFeeRate& max_tx_fee_rate, bool relay, bool wait_callback); /** * Return transaction with a given hash. diff --git a/src/qt/psbtoperationsdialog.cpp b/src/qt/psbtoperationsdialog.cpp index 5a4b4442f3d..e1fdd3d75fc 100644 --- a/src/qt/psbtoperationsdialog.cpp +++ b/src/qt/psbtoperationsdialog.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -120,7 +121,7 @@ void PSBTOperationsDialog::broadcastTransaction() CTransactionRef tx = MakeTransactionRef(mtx); std::string err_string; TransactionError error = - m_client_model->node().broadcastTransaction(tx, DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK(), err_string); + m_client_model->node().broadcastTransaction(tx, DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK(), CFeeRate(DEFAULT_MAX_RAW_TX_FEE_RATE.GetFeePerK()), err_string); if (error == TransactionError::OK) { showStatus(tr("Transaction broadcast successfully! Transaction ID: %1") diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 2b883322aa1..5b6efa0196b 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -90,13 +90,11 @@ static RPCHelpMan sendrawtransaction() const CFeeRate max_raw_tx_fee_rate{ParseFeeRate(self.Arg("maxfeerate"))}; - int64_t virtual_size = GetVirtualTransactionSize(*tx); - CAmount max_raw_tx_fee = max_raw_tx_fee_rate.GetFee(virtual_size); std::string err_string; AssertLockNotHeld(cs_main); NodeContext& node = EnsureAnyNodeContext(request.context); - const TransactionError err = BroadcastTransaction(node, tx, err_string, max_raw_tx_fee, /*relay=*/true, /*wait_callback=*/true); + const TransactionError err = BroadcastTransaction(node, tx, err_string, /*max_tx_fee=*/0, max_raw_tx_fee_rate, /*relay=*/true, /*wait_callback=*/true); if (TransactionError::OK != err) { throw JSONRPCTransactionError(err, err_string); } @@ -1060,7 +1058,7 @@ static RPCHelpMan submitpackage() // We do not expect an error here; we are only broadcasting things already/still in mempool std::string err_string; - const auto err = BroadcastTransaction(node, tx, err_string, /*max_tx_fee=*/0, /*relay=*/true, /*wait_callback=*/true); + const auto err = BroadcastTransaction(node, tx, err_string, /*max_tx_fee=*/0, /*max_tx_fee_rate=*/CFeeRate(0), /*relay=*/true, /*wait_callback=*/true); if (err != TransactionError::OK) { throw JSONRPCTransactionError(err, strprintf("transaction broadcast failed: %s (%d transactions were broadcast successfully)", diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index b5de4b4b3d3..9e32d8f49f6 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include