diff --git a/doc/release-notes-29278.md b/doc/release-notes-29278.md new file mode 100644 index 00000000000..4addbf2656d --- /dev/null +++ b/doc/release-notes-29278.md @@ -0,0 +1,7 @@ +wallet startup option +======================== + +- A new wallet startip option `-maxfeerate` is added. +- This option sets the upper limit for wallet transaction fee rate. +- The wallet will now refrain from creating transactions with a fee rate exceeding the `maxfeerate`. +- The default fee rate is 10,000 satoshis per virtual byte. \ No newline at end of file diff --git a/src/common/messages.cpp b/src/common/messages.cpp index 5fe3e9e4d86..2e38f199287 100644 --- a/src/common/messages.cpp +++ b/src/common/messages.cpp @@ -135,7 +135,9 @@ bilingual_str TransactionErrorString(const TransactionError err) case TransactionError::MEMPOOL_ERROR: return Untranslated("Mempool internal error"); case TransactionError::MAX_FEE_EXCEEDED: - return Untranslated("Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)"); + return Untranslated("Fee exceeds maximum configured by user (maxtxfee)"); + case TransactionError::MAX_FEE_RATE_EXCEEDED: + return Untranslated("Fee rate exceeds maximum configured by user (maxfeerate)"); case TransactionError::MAX_BURN_EXCEEDED: return Untranslated("Unspendable output exceeds maximum configured by user (maxburnamount)"); case TransactionError::INVALID_PACKAGE: 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/interfaces/chain.h b/src/interfaces/chain.h index c9ef46243c4..0fe32c2ac80 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 b7bcb431214..88c2f9d6213 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 50207c658d8..4cafd7a6e2b 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -362,9 +362,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 { @@ -684,11 +684,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..0c9902892b0 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,14 +70,16 @@ 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_RATE_EXCEEDED; } } // Try to submit the transaction to the mempool. 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/node/types.h b/src/node/types.h index 0f9b871084a..ce7bff8a5ac 100644 --- a/src/node/types.h +++ b/src/node/types.h @@ -27,6 +27,7 @@ enum class TransactionError { MEMPOOL_REJECTED, MEMPOOL_ERROR, MAX_FEE_EXCEEDED, + MAX_FEE_RATE_EXCEEDED, MAX_BURN_EXCEEDED, INVALID_PACKAGE, }; diff --git a/src/qt/psbtoperationsdialog.cpp b/src/qt/psbtoperationsdialog.cpp index 454d9287aea..8fa2d44b380 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/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/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/test/fuzz/kitchen_sink.cpp b/src/test/fuzz/kitchen_sink.cpp index 62b49106cdd..f6ff46b32a9 100644 --- a/src/test/fuzz/kitchen_sink.cpp +++ b/src/test/fuzz/kitchen_sink.cpp @@ -27,6 +27,7 @@ constexpr TransactionError ALL_TRANSACTION_ERROR[] = { TransactionError::MEMPOOL_REJECTED, TransactionError::MEMPOOL_ERROR, TransactionError::MAX_FEE_EXCEEDED, + TransactionError::MAX_FEE_RATE_EXCEEDED, }; }; // namespace diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp index b875461c9f2..2dac01b84c0 100644 --- a/src/wallet/feebumper.cpp +++ b/src/wallet/feebumper.cpp @@ -81,6 +81,13 @@ static feebumper::Result CheckFeeRate(const CWallet& wallet, const CMutableTrans return feebumper::Result::WALLET_ERROR; } + // check that new fee rate does not exceed maxfeerate + if (newFeerate > wallet.m_max_tx_fee_rate) { + errors.push_back(Untranslated(strprintf("New fee rate %s %s/kvB is too high (cannot be higher than -maxfeerate %s %s/kvB)", + FormatMoney(newFeerate.GetFeePerK()), CURRENCY_UNIT, FormatMoney(wallet.m_max_tx_fee_rate.GetFeePerK()), CURRENCY_UNIT))); + return feebumper::Result::WALLET_ERROR; + } + std::vector reused_inputs; reused_inputs.reserve(mtx.vin.size()); for (const CTxIn& txin : mtx.vin) { @@ -112,7 +119,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/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/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 0cba830e2ab..f3635d0a553 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_default_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; @@ -1506,9 +1505,12 @@ 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); } + if (CFeeRate(fee_from_size, tx_size.vsize) > pwallet->m_max_tx_fee_rate) { + throw JSONRPCError(RPC_WALLET_ERROR, TransactionErrorString(TransactionError::MAX_FEE_RATE_EXCEEDED).original); + } if (effective_value <= 0) { if (send_max) { diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 7b26cf15bd3..e8a3cb315b0 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -1339,10 +1339,15 @@ 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)}; } + CFeeRate tx_fee_rate = CFeeRate(current_fee, nBytes); + if (tx_fee_rate > wallet.m_max_tx_fee_rate) { + return util::Error{TransactionErrorString(TransactionError::MAX_FEE_RATE_EXCEEDED)}; + } + if (gArgs.GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS)) { // Lastly, ensure this tx will pass the mempool's chain limits auto result = wallet.chain().checkChainLimits(tx); 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