From e9adb96f8854d21a02d80b4295f1a2e57579cc18 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Fri, 28 Jun 2019 22:44:38 -0400 Subject: [wallet] abort when attempting to fund a transaction above maxtxfee FundTransaction calls GetMinimumFee which, when the fee rate is absurdly high, quietly reduced the fee to -maxtxfee. Becaue an absurdly high fee rate is usually the result of a fat finger, aborting seems safer behavior. Github-Pull: #16257 Rebased-From: 806b0052c3b45415862f74f20ba5f389e5b673de --- doc/release-notes-0.18.1-16257.md | 6 ++++++ src/policy/fees.cpp | 1 - src/policy/fees.h | 1 - src/wallet/fees.cpp | 8 +------- src/wallet/wallet.cpp | 5 +++++ test/functional/rpc_fundrawtransaction.py | 1 + test/functional/rpc_psbt.py | 19 ++++++++++++++++++- 7 files changed, 31 insertions(+), 10 deletions(-) create mode 100644 doc/release-notes-0.18.1-16257.md diff --git a/doc/release-notes-0.18.1-16257.md b/doc/release-notes-0.18.1-16257.md new file mode 100644 index 0000000000..21867b7fb2 --- /dev/null +++ b/doc/release-notes-0.18.1-16257.md @@ -0,0 +1,6 @@ +Wallet changes +-------------- +When creating a transaction with a fee above `-maxtxfee` (default 0.1 BTC), +the RPC commands `walletcreatefundedpsbt` and `fundrawtransaction` will now fail +instead of rounding down the fee. Beware that the `feeRate` argument is specified +in BTC per kilobyte, not satoshi per byte. diff --git a/src/policy/fees.cpp b/src/policy/fees.cpp index aee6fbee1a..d9a5474056 100644 --- a/src/policy/fees.cpp +++ b/src/policy/fees.cpp @@ -38,7 +38,6 @@ std::string StringForFeeReason(FeeReason reason) { {FeeReason::PAYTXFEE, "PayTxFee set"}, {FeeReason::FALLBACK, "Fallback fee"}, {FeeReason::REQUIRED, "Minimum Required Fee"}, - {FeeReason::MAXTXFEE, "MaxTxFee limit"} }; auto reason_string = fee_reason_strings.find(reason); diff --git a/src/policy/fees.h b/src/policy/fees.h index 136fb481f7..d46738cefb 100644 --- a/src/policy/fees.h +++ b/src/policy/fees.h @@ -88,7 +88,6 @@ enum class FeeReason { PAYTXFEE, FALLBACK, REQUIRED, - MAXTXFEE, }; std::string StringForFeeReason(FeeReason reason); diff --git a/src/wallet/fees.cpp b/src/wallet/fees.cpp index d620e25f2b..0cac42ba72 100644 --- a/src/wallet/fees.cpp +++ b/src/wallet/fees.cpp @@ -21,13 +21,7 @@ CAmount GetRequiredFee(const CWallet& wallet, unsigned int nTxBytes) CAmount GetMinimumFee(const CWallet& wallet, unsigned int nTxBytes, const CCoinControl& coin_control, const CTxMemPool& pool, const CBlockPolicyEstimator& estimator, FeeCalculation* feeCalc) { - CAmount fee_needed = GetMinimumFeeRate(wallet, coin_control, pool, estimator, feeCalc).GetFee(nTxBytes); - // Always obey the maximum - if (fee_needed > maxTxFee) { - fee_needed = maxTxFee; - if (feeCalc) feeCalc->reason = FeeReason::MAXTXFEE; - } - return fee_needed; + return GetMinimumFeeRate(wallet, coin_control, pool, estimator, feeCalc).GetFee(nTxBytes); } CFeeRate GetRequiredFeeRate(const CWallet& wallet) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 8d52f7eeca..92dc0f5d31 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2671,6 +2671,11 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC } } + if (nFeeRet > maxTxFee) { + strFailReason = _("Fee exceeds maximum configured by -maxtxfee"); + return false; + } + return true; } diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index 0c61e9ab62..019a9d4012 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -662,6 +662,7 @@ class RawTransactionsTest(BitcoinTestFramework): result = self.nodes[3].fundrawtransaction(rawtx) # uses min_relay_tx_fee (set by settxfee) result2 = self.nodes[3].fundrawtransaction(rawtx, {"feeRate": 2*min_relay_tx_fee}) result3 = self.nodes[3].fundrawtransaction(rawtx, {"feeRate": 10*min_relay_tx_fee}) + assert_raises_rpc_error(-4, "Fee exceeds maximum configured by -maxtxfee", self.nodes[3].fundrawtransaction, rawtx, {"feeRate": 1}) result_fee_rate = result['fee'] * 1000 / count_bytes(result['hex']) assert_fee_amount(result2['fee'], count_bytes(result2['hex']), 2 * result_fee_rate) assert_fee_amount(result3['fee'], count_bytes(result3['hex']), 10 * result_fee_rate) diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index ef895dd13a..24ac14162c 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -6,7 +6,15 @@ """ from test_framework.test_framework import BitcoinTestFramework -from test_framework.util import assert_equal, assert_raises_rpc_error, find_output, disconnect_nodes, connect_nodes_bi, sync_blocks +from test_framework.util import ( + assert_equal, + assert_greater_than, + assert_raises_rpc_error, + connect_nodes_bi, + disconnect_nodes, + find_output, + sync_blocks, +) import json import os @@ -122,6 +130,15 @@ class PSBTTest(BitcoinTestFramework): assert_equal(walletprocesspsbt_out['complete'], True) self.nodes[1].sendrawtransaction(self.nodes[1].finalizepsbt(walletprocesspsbt_out['psbt'])['hex']) + # feeRate of 0.1 BTC / KB produces a total fee slightly below -maxtxfee (~0.05280000): + res = self.nodes[1].walletcreatefundedpsbt([{"txid":txid,"vout":p2wpkh_pos},{"txid":txid,"vout":p2sh_p2wpkh_pos},{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():29.99}, 0, {"feeRate": 0.1}) + assert_greater_than(res["fee"], 0.05) + assert_greater_than(0.06, res["fee"]) + + # feeRate of 10 BTC / KB produces a total fee well above -maxtxfee + # previously this was silenty capped at -maxtxfee + assert_raises_rpc_error(-4, "Fee exceeds maximum configured by -maxtxfee", self.nodes[1].walletcreatefundedpsbt, [{"txid":txid,"vout":p2wpkh_pos},{"txid":txid,"vout":p2sh_p2wpkh_pos},{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():29.99}, 0, {"feeRate": 10}) + # partially sign multisig things with node 1 psbtx = self.nodes[1].walletcreatefundedpsbt([{"txid":txid,"vout":p2wsh_pos},{"txid":txid,"vout":p2sh_pos},{"txid":txid,"vout":p2sh_p2wsh_pos}], {self.nodes[1].getnewaddress():29.99})['psbt'] walletprocesspsbt_out = self.nodes[1].walletprocesspsbt(psbtx) -- cgit v1.2.3 From 0e7c7465bf2b9911da094d9790480f94e158f9a5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Barbosa?= Date: Tue, 2 Jul 2019 15:16:36 +0100 Subject: wallet: Fix -maxtxfee check by moving it to CWallet::CreateTransaction Github-Pull: #16322 Rebased-From: 5c1b9714cb0a13be28324f91f4ec9ca66a1de8c7 --- src/wallet/wallet.cpp | 10 +++++----- test/functional/rpc_psbt.py | 2 +- test/functional/wallet_bumpfee.py | 10 ++++++++++ 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 92dc0f5d31..d5f5df381a 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2671,11 +2671,6 @@ bool CWallet::FundTransaction(CMutableTransaction& tx, CAmount& nFeeRet, int& nC } } - if (nFeeRet > maxTxFee) { - strFailReason = _("Fee exceeds maximum configured by -maxtxfee"); - return false; - } - return true; } @@ -3080,6 +3075,11 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CTransac } } + if (nFeeRet > maxTxFee) { + strFailReason = _("Fee exceeds maximum configured by -maxtxfee"); + return false; + } + if (gArgs.GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS)) { // Lastly, ensure this tx will pass the mempool's chain limits LockPoints lp; diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index 24ac14162c..1beda6e832 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -136,7 +136,7 @@ class PSBTTest(BitcoinTestFramework): assert_greater_than(0.06, res["fee"]) # feeRate of 10 BTC / KB produces a total fee well above -maxtxfee - # previously this was silenty capped at -maxtxfee + # previously this was silently capped at -maxtxfee assert_raises_rpc_error(-4, "Fee exceeds maximum configured by -maxtxfee", self.nodes[1].walletcreatefundedpsbt, [{"txid":txid,"vout":p2wpkh_pos},{"txid":txid,"vout":p2sh_p2wpkh_pos},{"txid":txid,"vout":p2pkh_pos}], {self.nodes[1].getnewaddress():29.99}, 0, {"feeRate": 10}) # partially sign multisig things with node 1 diff --git a/test/functional/wallet_bumpfee.py b/test/functional/wallet_bumpfee.py index e478347cd7..944fc7b131 100755 --- a/test/functional/wallet_bumpfee.py +++ b/test/functional/wallet_bumpfee.py @@ -76,6 +76,7 @@ class BumpFeeTest(BitcoinTestFramework): test_unconfirmed_not_spendable(rbf_node, rbf_node_address) test_bumpfee_metadata(rbf_node, dest_address) test_locked_wallet_fails(rbf_node, dest_address) + test_maxtxfee_fails(self, rbf_node, dest_address) self.log.info("Success") @@ -206,6 +207,15 @@ def test_settxfee(rbf_node, dest_address): rbf_node.settxfee(Decimal("0.00000000")) # unset paytxfee +def test_maxtxfee_fails(test, rbf_node, dest_address): + test.restart_node(1, ['-maxtxfee=0.00003'] + test.extra_args[1]) + rbf_node.walletpassphrase(WALLET_PASSPHRASE, WALLET_PASSPHRASE_TIMEOUT) + rbfid = spend_one_input(rbf_node, dest_address) + assert_raises_rpc_error(-4, "Specified or calculated fee 0.0000332 is too high (cannot be higher than maxTxFee 0.00003)", rbf_node.bumpfee, rbfid) + test.restart_node(1, test.extra_args[1]) + rbf_node.walletpassphrase(WALLET_PASSPHRASE, WALLET_PASSPHRASE_TIMEOUT) + + def test_rebumping(rbf_node, dest_address): # check that re-bumping the original tx fails, but bumping the bumper succeeds rbfid = spend_one_input(rbf_node, dest_address) -- cgit v1.2.3